[PATCH 1/1] regmap: regcache-rbtree: Use GFP_ATOMIC when using spinlocks

[PATCH 1/1] regmap: regcache-rbtree: Use GFP_ATOMIC when using spinlocks

[Alexander Stein]

When regmap locking is done using spinlocks (e.g. using
devm_regmap_init_mmio_clk) access is protected using spin_lock_irqsave.
So when calling regmap_write the first time and a node is about to be
inserted kzalloc must not be called with GFP_KERNEL. At this point
interrupts are disabled. This fixes the following warning:
[    8.605433] WARNING: CPU: 0 PID: 130 at kernel/lockdep.c:2740 lockdep_trace_alloc+0x124/0x128()
[    8.614096] DEBUG_LOCKS_WARN_ON(irqs_disabled_flags(flags))
[    8.619469] Modules linked in: ads7846 gpio_mcp23s08 spidev lm75 ads1015 snd_soc_vf610_sgtl5000 snd_soc_sgtl5000 snd_soc_core snd_compress regmap_i2c snd_pcm_dmaengine snd_pcm snd_page_alloc regmap_spi snd_timer snd ahci_platform(+) soundcore libahci libata dwc3(+) fsl_dcu_fb(+) usb_common cfbfillrect cfbimgblt cfbcopyarea spi_fsl_dspi i2c_imx gpio_keys_polled input_polldev gpio_keys
[    8.653912] CPU: 0 PID: 130 Comm: udevd Not tainted 3.12.37-rt51+ #859
[    8.660413] Backtrace:
[    8.662890] [<800127a4>] (dump_backtrace+0x0/0x110) from [<800129bc>] (show_stack+0x18/0x1c)
[    8.671299]  r6:00000000 r5:bf1de028 r4:80536834 r3:00000000
[    8.677020] [<800129a4>] (show_stack+0x0/0x1c) from [<803a63bc>] (dump_stack+0xa0/0xdc)
[    8.685011] [<803a631c>] (dump_stack+0x0/0xdc) from [<80022890>] (warn_slowpath_common+0x74/0x90)
[    8.693842]  r6:00000ab4 r5:00000009 r4:bf1dfa80 r3:bf0adac0
[    8.699535] [<8002281c>] (warn_slowpath_common+0x0/0x90) from [<800228e4>] (warn_slowpath_fmt+0x38/0x40)
[    8.708970]  r8:000080d0 r7:600f0093 r6:000080d0 r5:bf1de020 r4:80536918
[    8.715703] [<800228ac>] (warn_slowpath_fmt+0x0/0x40) from [<80074444>] (lockdep_trace_alloc+0x124/0x128)
[    8.725221]  r3:80483004 r2:8047dad0
[    8.728809] [<80074320>] (lockdep_trace_alloc+0x0/0x128) from [<800c68b0>] (kmem_cache_alloc+0x34/0x17c)
[    8.738242]  r7:00000200 r6:00000200 r5:000080d0 r4:bf801f00
[    8.743929] [<800c687c>] (kmem_cache_alloc+0x0/0x17c) from [<80256ca0>] (regcache_rbtree_node_alloc+0x28/0x14c)
[    8.753975] [<80256c78>] (regcache_rbtree_node_alloc+0x0/0x14c) from [<802570e8>] (regcache_rbtree_write+0xe0/0x2c4)
[    8.764447]  r8:0000021c r7:00000200 r6:00000200 r5:bf1bce00 r4:00000000
[    8.764447] r3:00000000
[    8.772310] [<80257008>] (regcache_rbtree_write+0x0/0x2c4) from [<80255ed0>] (regcache_write+0x5c/0x64)
[    8.781666] [<80255e74>] (regcache_write+0x0/0x64) from [<802550d8>] (_regmap_raw_write+0x144/0x5e8)
[    8.790756]  r6:bf3cbb88 r5:00000004 r4:bf1bce00 r3:00000000
[    8.796432] [<80254f94>] (_regmap_raw_write+0x0/0x5e8) from [<802555fc>] (_regmap_bus_raw_write+0x80/0xa0)
[    8.806047] [<8025557c>] (_regmap_bus_raw_write+0x0/0xa0) from [<802548b8>] (_regmap_write+0x60/0x9c)
[    8.815224]  r6:00000000 r5:00000200 r4:bf1bce00
[    8.819854] [<80254858>] (_regmap_write+0x0/0x9c) from [<80255a34>] (regmap_write+0x48/0x68)
[    8.828254]  r7:c0ec0000 r6:00000000 r5:00000200 r4:bf1bce00
[    8.833949] [<802559ec>] (regmap_write+0x0/0x68) from [<7f035a68>] (fsl_dcu_probe+0x268/0x7d4 [fsl_dcu_fb])
[    8.843642]  r6:8128573c r5:bf3cb910 r4:00000200 r3:00000000
[    8.849324] [<7f035800>] (fsl_dcu_probe+0x0/0x7d4 [fsl_dcu_fb]) from [<8024dfe8>] (platform_drv_probe+0x20/0x24)
[    8.859461] [<8024dfc8>] (platform_drv_probe+0x0/0x24) from [<8024cb9c>] (driver_probe_device+0xa0/0x384)
[    8.868990] [<8024cafc>] (driver_probe_device+0x0/0x384) from [<8024cf68>] (__driver_attach+0x9c/0xa0)
[    8.878263] [<8024cecc>] (__driver_attach+0x0/0xa0) from [<8024ac34>] (bus_for_each_dev+0x68/0x9c)
[    8.887178]  r6:8024cecc r5:7f036c9c r4:00000000 r3:bf923f5c
[    8.892855] [<8024abcc>] (bus_for_each_dev+0x0/0x9c) from [<8024c604>] (driver_attach+0x24/0x28)
[    8.901599]  r6:bf36b700 r5:80540918 r4:7f036c9c
[    8.906236] [<8024c5e0>] (driver_attach+0x0/0x28) from [<8024c1b8>] (bus_add_driver+0x1f8/0x2b4)
[    8.914989] [<8024bfc0>] (bus_add_driver+0x0/0x2b4) from [<8024d384>] (driver_register+0x80/0x100)
[    8.923907]  r7:00000001 r6:7f036f28 r5:7f036f34 r4:7f036c9c
[    8.929584] [<8024d304>] (driver_register+0x0/0x100) from [<8024e5f0>] (__platform_driver_register+0x5c/0x64)
[    8.939451]  r5:7f036f34 r4:bf1dff48
[    8.943040] [<8024e594>] (__platform_driver_register+0x0/0x64) from [<7f03901c>] (fsl_dcu_driver_init+0x1c/0x24 [fsl_dcu_fb])
[    8.954297] [<7f039000>] (fsl_dcu_driver_init+0x0/0x24 [fsl_dcu_fb]) from [<800087fc>] (do_one_initcall+0xdc/0x188)
[    8.964690] [<80008720>] (do_one_initcall+0x0/0x188) from [<80081800>] (load_module+0x1794/0x1cb4)
[    8.973613] [<8008006c>] (load_module+0x0/0x1cb4) from [<80081ec0>] (SyS_finit_module+0x88/0xb8)
[    8.982365] [<80081e38>] (SyS_finit_module+0x0/0xb8) from [<8000f2c0>] (ret_fast_syscall+0x0/0x48)
[    8.991282]  r6:00000000 r5:00000000 r4:00000000

Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
Despite the fact that the backtrace is from a non-mainline, somewhat older
kernel, the code has not changed meanwhile and is still applicable.

 drivers/base/regmap/regcache-rbtree.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/base/regmap/regcache-rbtree.c b/drivers/base/regmap/regcache-rbtree.c
index 81751a4..1480f96 100644
--- a/drivers/base/regmap/regcache-rbtree.c
+++ b/drivers/base/regmap/regcache-rbtree.c
@@ -285,19 +285,21 @@ static int regcache_rbtree_insert_to_block(struct regmap *map,
 	unsigned int pos, offset;
 	unsigned long *present;
 	u8 *blk;
+	gfp_t flags;
 
 	blklen = (top_reg - base_reg) / map->reg_stride + 1;
 	pos = (reg - base_reg) / map->reg_stride;
 	offset = (rbnode->base_reg - base_reg) / map->reg_stride;
+	flags = map->bus->fast_io ? GFP_ATOMIC : GFP_KERNEL;
 
 	blk = krealloc(rbnode->block,
 		       blklen * map->cache_word_size,
-		       GFP_KERNEL);
+		       flags);
 	if (!blk)
 		return -ENOMEM;
 
 	present = krealloc(rbnode->cache_present,
-		    BITS_TO_LONGS(blklen) * sizeof(*present), GFP_KERNEL);
+		    BITS_TO_LONGS(blklen) * sizeof(*present), flags);
 	if (!present) {
 		kfree(blk);
 		return -ENOMEM;
@@ -326,8 +328,9 @@ regcache_rbtree_node_alloc(struct regmap *map, unsigned int reg)
 	struct regcache_rbtree_node *rbnode;
 	const struct regmap_range *range;
 	int i;
+	gfp_t flags = map->bus->fast_io ? GFP_ATOMIC : GFP_KERNEL;
 
-	rbnode = kzalloc(sizeof(*rbnode), GFP_KERNEL);
+	rbnode = kzalloc(sizeof(*rbnode), flags);
 	if (!rbnode)
 		return NULL;
 
@@ -353,12 +356,12 @@ regcache_rbtree_node_alloc(struct regmap *map, unsigned int reg)
 	}
 
 	rbnode->block = kmalloc(rbnode->blklen * map->cache_word_size,
-				GFP_KERNEL);
+				flags);
 	if (!rbnode->block)
 		goto err_free;
 
 	rbnode->cache_present = kzalloc(BITS_TO_LONGS(rbnode->blklen) *
-		sizeof(*rbnode->cache_present), GFP_KERNEL);
+		sizeof(*rbnode->cache_present), flags);
 	if (!rbnode->cache_present)
 		goto err_free_block;
 
-- 
2.3.6

Re: [PATCH 1/1] regmap: regcache-rbtree: Use GFP_ATOMIC when using spinlocks

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1226 bytes --]

On Thu, Jul 16, 2015 at 05:48:52PM +0200, Alexander Stein wrote:

> When regmap locking is done using spinlocks (e.g. using
> devm_regmap_init_mmio_clk) access is protected using spin_lock_irqsave.
> So when calling regmap_write the first time and a node is about to be
> inserted kzalloc must not be called with GFP_KERNEL. At this point

The expectation here is that we should either be using no or a flat
cache here or (if we're using rbtree) providing register defaults to
ensure that we never do allocations in the spinlock.  The rbtree code is
written on the assumption that we only have to be faster than reading
from a serial bus so I'd be worried about it not behaving at all nicely
in a spinlock even ignoring this issue.

Why are you using a dynamically allocated rbtree for a device like this?

> interrupts are disabled. This fixes the following warning:
> [    8.605433] WARNING: CPU: 0 PID: 130 at kernel/lockdep.c:2740 lockdep_trace_alloc+0x124/0x128()
> [    8.614096] DEBUG_LOCKS_WARN_ON(irqs_disabled_flags(flags))

Please don't paste entire backtraces into commit messages, they're
enormous and don't add any value.  If you feel a backtrace is useful
edit down the relevant context.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/1] regmap: regcache-rbtree: Use GFP_ATOMIC when using spinlocks

[Alexander Stein]

On Thursday 16 July 2015 20:06:57, Mark Brown wrote:
> On Thu, Jul 16, 2015 at 05:48:52PM +0200, Alexander Stein wrote:
> 
> > When regmap locking is done using spinlocks (e.g. using
> > devm_regmap_init_mmio_clk) access is protected using spin_lock_irqsave.
> > So when calling regmap_write the first time and a node is about to be
> > inserted kzalloc must not be called with GFP_KERNEL. At this point
> 
> The expectation here is that we should either be using no or a flat
> cache here or (if we're using rbtree) providing register defaults to
> ensure that we never do allocations in the spinlock.  The rbtree code is
> written on the assumption that we only have to be faster than reading
> from a serial bus so I'd be worried about it not behaving at all nicely
> in a spinlock even ignoring this issue.

AFAICS even a flat cache seems also only be usefull when providing defaults, no? (Or having volatile registers).
So how to handle this properly? Bail out, if fast_io is available and cache_type != (REGCACHE_NONE || REGCACHE_FLAT)?

> Why are you using a dynamically allocated rbtree for a device like this?

On my way home, I came to the same question. In fact this is not a driver written by myself, but from here http://git.freescale.com/git/cgit.cgi/ppc/sdk/linux.git/tree/drivers/video/fsl-dcu-fb.c#n1024.
I guess as this is a mmio device and things like regcache_cache_only() are used, REGCACHE_FLAT seems appropriate.

> > interrupts are disabled. This fixes the following warning:
> > [    8.605433] WARNING: CPU: 0 PID: 130 at kernel/lockdep.c:2740 lockdep_trace_alloc+0x124/0x128()
> > [    8.614096] DEBUG_LOCKS_WARN_ON(irqs_disabled_flags(flags))
> 
> Please don't paste entire backtraces into commit messages, they're
> enormous and don't add any value.  If you feel a backtrace is useful
> edit down the relevant context.

Thanks, noted.

Best regards,
Alexander
-- 
Dipl.-Inf. Alexander Stein
SYS TEC electronic GmbH
alexander.stein@systec-electronic.com

Legal and Commercial Address:
Am Windrad 2
08468 Heinsdorfergrund
Germany

Office: +49 (0) 3765 38600-11xx
Fax:    +49 (0) 0) 3765 38600-41xx
 
Managing Directors:
	Director Technology/CEO: Dipl.-Phys. Siegmar Schmidt;
	Director Commercial Affairs/COO: Dipl. Ing. (FH) Armin von Collrepp
Commercial Registry:
	Amtsgericht Chemnitz, HRB 28082; USt.-Id Nr. DE150534010

Re: [PATCH 1/1] regmap: regcache-rbtree: Use GFP_ATOMIC when using spinlocks

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1670 bytes --]

On Mon, Jul 20, 2015 at 09:04:05AM +0200, Alexander Stein wrote:
> On Thursday 16 July 2015 20:06:57, Mark Brown wrote:

Please fix your mailer to word wrap within paragraphs for legibility.

> > The expectation here is that we should either be using no or a flat
> > cache here or (if we're using rbtree) providing register defaults to
> > ensure that we never do allocations in the spinlock.  The rbtree code is
> > written on the assumption that we only have to be faster than reading
> > from a serial bus so I'd be worried about it not behaving at all nicely
> > in a spinlock even ignoring this issue.

> AFAICS even a flat cache seems also only be usefull when providing
> defaults, no? (Or having volatile registers).

Well, it's *better* to provide defaults since otherwise everything
defaults to 0 but it does avoid the whole allocation during fast path
issue since it allocates the cache on init and perhaps that's OK.

> So how to handle this properly? Bail out, if fast_io is available and
> cache_type != (REGCACHE_NONE || REGCACHE_FLAT)?

Or perhaps just if we have to do an allocation?  I can see that someone
might want to use an rbtree and would be careful enough to do the init,
though I *am* a bit dubious about it.

> > Why are you using a dynamically allocated rbtree for a device like this?

> On my way home, I came to the same question. In fact this is not a
> driver written by myself, but from here
> http://git.freescale.com/git/cgit.cgi/ppc/sdk/linux.git/tree/drivers/video/fsl-dcu-fb.c#n1024.
> I guess as this is a mmio device and things like regcache_cache_only()
> are used, REGCACHE_FLAT seems appropriate.

Ah, out of tree BSP code...

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/1] regmap: regcache-rbtree: Use GFP_ATOMIC when using spinlocks

[Alexander Stein]

On Monday 20 July 2015 18:26:27, Mark Brown wrote:
> > AFAICS even a flat cache seems also only be usefull when providing
> > defaults, no? (Or having volatile registers).
> 
> Well, it's *better* to provide defaults since otherwise everything
> defaults to 0 but it does avoid the whole allocation during fast path
> issue since it allocates the cache on init and perhaps that's OK.

There is another reason for using REGCACHE_FLAT: Using regcache_cache_only 
(e.g. during suspend) which is not possible with REGCACHE_NONE.

> > So how to handle this properly? Bail out, if fast_io is available and
> > cache_type != (REGCACHE_NONE || REGCACHE_FLAT)?
> 
> Or perhaps just if we have to do an allocation?  I can see that someone
> might want to use an rbtree and would be careful enough to do the init,
> though I *am* a bit dubious about it.

I'm feeling uncomfortable this warning occured only when (at least) 
CONFIG_LOCKDEP is enabled. It warns right ahead but only if you begged for 
it...
Even if defaults are provided an extension to the register set (e.g. a more 
recent IP revision with more features) might not be synchronized with the 
defaults. Nobody might noticed until CONFIG_LOCKDEP is enabled and the 
register without defaults gets written.

Best regards,
Alexander
-- 
Dipl.-Inf. Alexander Stein
SYS TEC electronic GmbH
alexander.stein@systec-electronic.com

Legal and Commercial Address:
Am Windrad 2
08468 Heinsdorfergrund
Germany

Office: +49 (0) 3765 38600-0
Fax:    +49 (0) 3765 38600-4100
 
Managing Directors:
	Director Technology/CEO: Dipl.-Phys. Siegmar Schmidt;
	Director Commercial Affairs/COO: Dipl. Ing. (FH) Armin von Collrepp
Commercial Registry:
	Amtsgericht Chemnitz, HRB 28082; USt.-Id Nr. DE150534010

Re: [PATCH 1/1] regmap: regcache-rbtree: Use GFP_ATOMIC when using spinlocks

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1666 bytes --]

On Tue, Jul 21, 2015 at 08:14:32AM +0200, Alexander Stein wrote:
> On Monday 20 July 2015 18:26:27, Mark Brown wrote:

> > Well, it's *better* to provide defaults since otherwise everything
> > defaults to 0 but it does avoid the whole allocation during fast path
> > issue since it allocates the cache on init and perhaps that's OK.

> There is another reason for using REGCACHE_FLAT: Using regcache_cache_only 
> (e.g. during suspend) which is not possible with REGCACHE_NONE.

Sure, if you want a cache you need to use a cache.

> > > So how to handle this properly? Bail out, if fast_io is available and
> > > cache_type != (REGCACHE_NONE || REGCACHE_FLAT)?

> > Or perhaps just if we have to do an allocation?  I can see that someone
> > might want to use an rbtree and would be careful enough to do the init,
> > though I *am* a bit dubious about it.

> I'm feeling uncomfortable this warning occured only when (at least) 
> CONFIG_LOCKDEP is enabled. It warns right ahead but only if you begged for 
> it...

OTOH it'll splat with lockdep enabled just as soon as someone tries to
access a register that they didn't provide a default for.  An explicitly
added error and bomb out like I suggested above would do the same thing.

> Even if defaults are provided an extension to the register set (e.g. a more 
> recent IP revision with more features) might not be synchronized with the 
> defaults. Nobody might noticed until CONFIG_LOCKDEP is enabled and the 
> register without defaults gets written.

Sure, it's got sharp edges but the whole point with my suggestion above
is that detection wouldn't depend on CONFIG_LOCKDEP.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]