diff mbox series

[RFC,2/2] regmap: Reject fast_io regmap configurations with RBTREE and MAPLE caches

Message ID 20230720032848.1306349-2-linux@roeck-us.net
State New
Headers
Series [1/2] regmap: Disable locking for RBTREE and MAPLE unit tests |

Commit Message

Guenter Roeck July 20, 2023, 3:28 a.m. UTC 
  REGCACHE_RBTREE and REGCACHE_MAPLE dynamically allocate memory for regmap
operations. This is incompatible with spinlock based locking which is used
for fast_io operations. Reject affected configurations.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
This seems prudent, given that accesses will be protected by spinlock
but may allocate memory with GFP_KERNEL. Another option might be to use
WARN_ON instead of rejecting the configuration to avoid hard regressions
(and I think both drivers/net/ieee802154/mcr20a.c and
sound/soc/codecs/sti-sas.c may be affected, though I can not test it).

 drivers/base/regmap/regmap.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Marek Szyprowski July 21, 2023, 2:53 p.m. UTC | #1 
Hi,

On 20.07.2023 05:28, Guenter Roeck wrote:
> REGCACHE_RBTREE and REGCACHE_MAPLE dynamically allocate memory for regmap
> operations. This is incompatible with spinlock based locking which is used
> for fast_io operations. Reject affected configurations.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> This seems prudent, given that accesses will be protected by spinlock
> but may allocate memory with GFP_KERNEL. Another option might be to use
> WARN_ON instead of rejecting the configuration to avoid hard regressions
> (and I think both drivers/net/ieee802154/mcr20a.c and
> sound/soc/codecs/sti-sas.c may be affected, though I can not test it).

This patch, which landed in today's linux-next, breaks operation of the 
RockChip's VOP2 DRM driver 
(drivers/gpu/drm/rockchip/rockchip_drm_vop2.c). I'm not sure what is the 
proper fix in this case. Should one change the cache type to REGCACHE_FLAT?


>   drivers/base/regmap/regmap.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
>
> diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
> index 89a7f1c459c1..b4640285c0b9 100644
> --- a/drivers/base/regmap/regmap.c
> +++ b/drivers/base/regmap/regmap.c
> @@ -777,6 +777,15 @@ struct regmap *__regmap_init(struct device *dev,
>   	} else {
>   		if ((bus && bus->fast_io) ||
>   		    config->fast_io) {
> +			/*
> +			 * fast_io is incompatible with REGCACHE_RBTREE and REGCACHE_MAPLE
> +			 * since both need to dynamically allocate memory.
> +			 */
> +			if (config->cache_type == REGCACHE_RBTREE ||
> +			    config->cache_type == REGCACHE_MAPLE) {
> +				ret = -EINVAL;
> +				goto err_name;
> +			}
>   			if (config->use_raw_spinlock) {
>   				raw_spin_lock_init(&map->raw_spinlock);
>   				map->lock = regmap_lock_raw_spinlock;

Best regards
Mark Brown July 21, 2023, 3:03 p.m. UTC | #2 
On Fri, Jul 21, 2023 at 04:53:42PM +0200, Marek Szyprowski wrote:

> This patch, which landed in today's linux-next, breaks operation of the 
> RockChip's VOP2 DRM driver 
> (drivers/gpu/drm/rockchip/rockchip_drm_vop2.c). I'm not sure what is the 
> proper fix in this case. Should one change the cache type to REGCACHE_FLAT?

Actually Guenter and Dan have made the required updates to support this
so the warning will be gone soon (hopefully Dan will send his patch
properly shortly).
Guenter Roeck July 21, 2023, 3:05 p.m. UTC | #3 
On 7/21/23 07:53, Marek Szyprowski wrote:
> Hi,
> 
> On 20.07.2023 05:28, Guenter Roeck wrote:
>> REGCACHE_RBTREE and REGCACHE_MAPLE dynamically allocate memory for regmap
>> operations. This is incompatible with spinlock based locking which is used
>> for fast_io operations. Reject affected configurations.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>> This seems prudent, given that accesses will be protected by spinlock
>> but may allocate memory with GFP_KERNEL. Another option might be to use
>> WARN_ON instead of rejecting the configuration to avoid hard regressions
>> (and I think both drivers/net/ieee802154/mcr20a.c and
>> sound/soc/codecs/sti-sas.c may be affected, though I can not test it).
> 
> This patch, which landed in today's linux-next, breaks operation of the
> RockChip's VOP2 DRM driver
> (drivers/gpu/drm/rockchip/rockchip_drm_vop2.c). I'm not sure what is the
> proper fix in this case. Should one change the cache type to REGCACHE_FLAT?
> 

Ah, I missed regcache_init_mmio() when looking for affected drivers.
This affects a larger number of drivers than I thought. In addition
to the drivers mentioned above,

  drivers/soc/qcom/icc-bwmon.c
  sound/soc/bcm/bcm2835-i2s.c
  sound/soc/codecs/jz4740.c
  sound/soc/fsl/fsl_aud2htx.c
  sound/soc/fsl/fsl_easrc.c
  sound/soc/fsl/fsl_micfil.c

all use unsafe locking (spinlock with REGCACHE_RBTREE).

Thanks,
Guenter

> 
>>    drivers/base/regmap/regmap.c | 9 +++++++++
>>    1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
>> index 89a7f1c459c1..b4640285c0b9 100644
>> --- a/drivers/base/regmap/regmap.c
>> +++ b/drivers/base/regmap/regmap.c
>> @@ -777,6 +777,15 @@ struct regmap *__regmap_init(struct device *dev,
>>    	} else {
>>    		if ((bus && bus->fast_io) ||
>>    		    config->fast_io) {
>> +			/*
>> +			 * fast_io is incompatible with REGCACHE_RBTREE and REGCACHE_MAPLE
>> +			 * since both need to dynamically allocate memory.
>> +			 */
>> +			if (config->cache_type == REGCACHE_RBTREE ||
>> +			    config->cache_type == REGCACHE_MAPLE) {
>> +				ret = -EINVAL;
>> +				goto err_name;
>> +			}
>>    			if (config->use_raw_spinlock) {
>>    				raw_spin_lock_init(&map->raw_spinlock);
>>    				map->lock = regmap_lock_raw_spinlock;
> 
> Best regards
Guenter Roeck July 21, 2023, 3:07 p.m. UTC | #4 
On 7/21/23 08:03, Mark Brown wrote:
> On Fri, Jul 21, 2023 at 04:53:42PM +0200, Marek Szyprowski wrote:
> 
>> This patch, which landed in today's linux-next, breaks operation of the
>> RockChip's VOP2 DRM driver
>> (drivers/gpu/drm/rockchip/rockchip_drm_vop2.c). I'm not sure what is the
>> proper fix in this case. Should one change the cache type to REGCACHE_FLAT?
> 
> Actually Guenter and Dan have made the required updates to support this
> so the warning will be gone soon (hopefully Dan will send his patch
> properly shortly).

Do you plan to revert this patch ? If not regmap_init() would still fail
for the affected drivers, even after my and Dan's patches have been applied.

Thanks,
Guenter
Mark Brown July 21, 2023, 3:13 p.m. UTC | #5 
On Fri, Jul 21, 2023 at 08:07:28AM -0700, Guenter Roeck wrote:
> On 7/21/23 08:03, Mark Brown wrote:

> > Actually Guenter and Dan have made the required updates to support this
> > so the warning will be gone soon (hopefully Dan will send his patch
> > properly shortly).

> Do you plan to revert this patch ? If not regmap_init() would still fail
> for the affected drivers, even after my and Dan's patches have been applied.

Yeah.  You *can* use the dynamically allocating caches safely if you
ensure that no new cache nodes are allocated during I/O.  I'd not
realised people were actually doing this.
Guenter Roeck July 21, 2023, 4:01 p.m. UTC | #6 
On 7/21/23 08:13, Mark Brown wrote:
> On Fri, Jul 21, 2023 at 08:07:28AM -0700, Guenter Roeck wrote:
>> On 7/21/23 08:03, Mark Brown wrote:
> 
>>> Actually Guenter and Dan have made the required updates to support this
>>> so the warning will be gone soon (hopefully Dan will send his patch
>>> properly shortly).
> 
>> Do you plan to revert this patch ? If not regmap_init() would still fail
>> for the affected drivers, even after my and Dan's patches have been applied.
> 
> Yeah.  You *can* use the dynamically allocating caches safely if you
> ensure that no new cache nodes are allocated during I/O.  I'd not
> realised people were actually doing this.

Ok.

Dan, let me know if you don't have time to send a proper patch.
I have one based on your suggestion prepared that I could send out
if needed.

Thanks,
Guenter
Mark Brown July 21, 2023, 4:14 p.m. UTC | #7 
On Fri, Jul 21, 2023 at 09:01:03AM -0700, Guenter Roeck wrote:
> On 7/21/23 08:13, Mark Brown wrote:

> > Yeah.  You *can* use the dynamically allocating caches safely if you
> > ensure that no new cache nodes are allocated during I/O.  I'd not
> > realised people were actually doing this.

> Ok.

> Dan, let me know if you don't have time to send a proper patch.
> I have one based on your suggestion prepared that I could send out
> if needed.

Dan sent the patch already, assuming my CI doesn't blow up unexpectedly
it should be applied tonight.
Guenter Roeck July 21, 2023, 4:16 p.m. UTC | #8 
On Fri, Jul 21, 2023 at 05:14:42PM +0100, Mark Brown wrote:
> On Fri, Jul 21, 2023 at 09:01:03AM -0700, Guenter Roeck wrote:
> > On 7/21/23 08:13, Mark Brown wrote:
> 
> > > Yeah.  You *can* use the dynamically allocating caches safely if you
> > > ensure that no new cache nodes are allocated during I/O.  I'd not
> > > realised people were actually doing this.
> 
> > Ok.
> 
> > Dan, let me know if you don't have time to send a proper patch.
> > I have one based on your suggestion prepared that I could send out
> > if needed.
> 
> Dan sent the patch already, assuming my CI doesn't blow up unexpectedly
> it should be applied tonight.

Excellent.

Thanks,
Guenter
Dan Carpenter July 21, 2023, 4:18 p.m. UTC | #9 
On Fri, Jul 21, 2023 at 09:01:03AM -0700, Guenter Roeck wrote:

> Dan, let me know if you don't have time to send a proper patch.
> I have one based on your suggestion prepared that I could send out
> if needed.

I sent it but, aww crud, I forgot to CC you.  Really, get_maintainer.pl
should add everyone from the tag section to the CC list...

https://lore.kernel.org/all/58f12a07-5f4b-4a8f-ab84-0a42d1908cb9@moroto.mountain/

regards,
dan carpenter
Mark Brown July 21, 2023, 4:22 p.m. UTC | #10 
On Fri, Jul 21, 2023 at 07:18:03PM +0300, Dan Carpenter wrote:
> On Fri, Jul 21, 2023 at 09:01:03AM -0700, Guenter Roeck wrote:

> > Dan, let me know if you don't have time to send a proper patch.
> > I have one based on your suggestion prepared that I could send out
> > if needed.

> I sent it but, aww crud, I forgot to CC you.  Really, get_maintainer.pl
> should add everyone from the tag section to the CC list...

b4 prep --auto-to-cc does that IIRC!
Guenter Roeck July 21, 2023, 4:29 p.m. UTC | #11 
On 7/21/23 09:18, Dan Carpenter wrote:
> On Fri, Jul 21, 2023 at 09:01:03AM -0700, Guenter Roeck wrote:
> 
>> Dan, let me know if you don't have time to send a proper patch.
>> I have one based on your suggestion prepared that I could send out
>> if needed.
> 
> I sent it but, aww crud, I forgot to CC you.  Really, get_maintainer.pl
> should add everyone from the tag section to the CC list...
> 
> https://lore.kernel.org/all/58f12a07-5f4b-4a8f-ab84-0a42d1908cb9@moroto.mountain/
> 

No worries.

Thanks,
Guenter
diff mbox series

Patch

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 89a7f1c459c1..b4640285c0b9 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -777,6 +777,15 @@  struct regmap *__regmap_init(struct device *dev,
 	} else {
 		if ((bus && bus->fast_io) ||
 		    config->fast_io) {
+			/*
+			 * fast_io is incompatible with REGCACHE_RBTREE and REGCACHE_MAPLE
+			 * since both need to dynamically allocate memory.
+			 */
+			if (config->cache_type == REGCACHE_RBTREE ||
+			    config->cache_type == REGCACHE_MAPLE) {
+				ret = -EINVAL;
+				goto err_name;
+			}
 			if (config->use_raw_spinlock) {
 				raw_spin_lock_init(&map->raw_spinlock);
 				map->lock = regmap_lock_raw_spinlock;