Re: Traceback with CONFIG_REGMAP_KUNIT=y+CONFIG_DEBUG_ATOMIC_SLEEP=y

[Guenter Roeck <linux@roeck-us.net>]

On 7/20/23 01:50, Dan Carpenter wrote:
> On Wed, Jul 19, 2023 at 03:37:54PM -0700, Guenter Roeck wrote:
>> Hi,
>>
>> when booting images with both CONFIG_REGMAP_KUNIT and
>> CONFIG_DEBUG_ATOMIC_SLEEP enabled, I get the following backtrace.
>>
>> [    4.994957]     # Subtest: regmap
>> [    4.995067]     1..14
>> [    4.995190]         KTAP version 1
>> [    4.995308]         # Subtest: basic_read_write
>> [    4.999402]         ok 1 none
>> [    5.003028]         ok 2 flat
>> [    5.005510] BUG: sleeping function called from invalid context at include/linux/sched/mm.h:306
>> [    5.005960] in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 117, name: kunit_try_catch
>> [    5.006219] preempt_count: 1, expected: 0
>> [    5.006414] 1 lock held by kunit_try_catch/117:
>> [    5.006590]  #0: 833b9010 (regmap_kunit:86:(config)->lock){....}-{2:2}, at: regmap_lock_spinlock+0x14/0x1c
>> [    5.007493] irq event stamp: 162
>> [    5.007627] hardirqs last  enabled at (161): [<80786738>] crng_make_state+0x1a0/0x294
>> [    5.007871] hardirqs last disabled at (162): [<80c531ec>] _raw_spin_lock_irqsave+0x7c/0x80
>> [    5.008119] softirqs last  enabled at (0): [<801110ac>] copy_process+0x810/0x2138
>> [    5.008356] softirqs last disabled at (0): [<00000000>] 0x0
>> [    5.008688] CPU: 0 PID: 117 Comm: kunit_try_catch Tainted: G                 N 6.4.4-rc3-g0e8d2fdfb188 #1
>> [    5.009011] Hardware name: Generic DT based system
>> [    5.009277]  unwind_backtrace from show_stack+0x18/0x1c
>> [    5.009497]  show_stack from dump_stack_lvl+0x38/0x5c
>> [    5.009676]  dump_stack_lvl from __might_resched+0x188/0x2d0
>> [    5.009860]  __might_resched from __kmem_cache_alloc_node+0x1dc/0x25c
>> [    5.010061]  __kmem_cache_alloc_node from kmalloc_trace+0x30/0xc8
>> [    5.010254]  kmalloc_trace from regcache_rbtree_write+0x26c/0x468
>> [    5.010446]  regcache_rbtree_write from _regmap_write+0x88/0x140
>> [    5.010634]  _regmap_write from regmap_write+0x44/0x68
>> [    5.010803]  regmap_write from basic_read_write+0x8c/0x270
>> [    5.010980]  basic_read_write from kunit_try_run_case+0x48/0xa0
>> [    5.011170]  kunit_try_run_case from kunit_generic_run_threadfn_adapter+0x1c/0x28
>> [    5.011396]  kunit_generic_run_threadfn_adapter from kthread+0xf8/0x120
>> [    5.011603]  kthread from ret_from_fork+0x14/0x3c
>> [    5.011801] Exception stack(0x881a5fb0 to 0x881a5ff8)
>> [    5.012024] 5fa0:                                     00000000 00000000 00000000 00000000
>> [    5.012269] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
>> [    5.012502] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
>> [    5.014546]         ok 3 rbtree
>> [    5.018988]         ok 4 maple
>>
>> This isn't surprising given that the regmap code allocates rbcache nodes
>> with GFP_KERNEL and the regmap configuration results in using spinlock
>> to lock regmap accesses.
>>
>> I'll be happy to submit a patch to fix the problem, but I would need
>> some advice. I could
>>
>> - Update the unit test regmap configuration to avoid using
>>    spinlock as locking mechanism for rbtree tests.
>>    That would work, but fail to catch situations where this happens
>>    in the real world.
>> - Use a different method to allocate memory in regcache_rbtree_node_alloc().
>>    - Replace GFP_KERNEL with GFP_NOWAIT or GFP_KERNEL | __GFP_NOWARN (?)
>>      or something else, possibly only if gfpflags_allow_blocking() is false
> 
> __GFP_NOWARN is only about allocation failure, not sleeping.  I would
> have thought it should be.
> 
> diff --git a/drivers/base/regmap/regcache-rbtree.c b/drivers/base/regmap/regcache-rbtree.c
> index fabf87058d80..d94e25120f2f 100644
> --- a/drivers/base/regmap/regcache-rbtree.c
> +++ b/drivers/base/regmap/regcache-rbtree.c
> @@ -187,7 +187,7 @@ static int regcache_rbtree_init(struct regmap *map)
>   	int i;
>   	int ret;
>   
> -	map->cache = kmalloc(sizeof *rbtree_ctx, GFP_KERNEL);
> +	map->cache = kmalloc(sizeof *rbtree_ctx, map->alloc_flags);

Yes, that might work as well (and after looking more deeply into the code
I wondered why it wasn't used in the first place).

Based on Mark's feedback I submitted
https://lore.kernel.org/lkml/20230720032848.1306349-1-linux@roeck-us.net/
Sorry, I forgot to copy you on that one.

Mark, please let me know what you prefer.

Thanks,
Guenter

>   	if (!map->cache)
>   		return -ENOMEM;
>   
> @@ -277,7 +277,7 @@ static int regcache_rbtree_insert_to_block(struct regmap *map,
>   
>   	blk = krealloc(rbnode->block,
>   		       blklen * map->cache_word_size,
> -		       GFP_KERNEL);
> +		       map->alloc_flags);
>   	if (!blk)
>   		return -ENOMEM;
>   
> @@ -286,7 +286,7 @@ static int regcache_rbtree_insert_to_block(struct regmap *map,
>   	if (BITS_TO_LONGS(blklen) > BITS_TO_LONGS(rbnode->blklen)) {
>   		present = krealloc(rbnode->cache_present,
>   				   BITS_TO_LONGS(blklen) * sizeof(*present),
> -				   GFP_KERNEL);
> +				   map->alloc_flags);
>   		if (!present)
>   			return -ENOMEM;
>   
> @@ -320,7 +320,7 @@ regcache_rbtree_node_alloc(struct regmap *map, unsigned int reg)
>   	const struct regmap_range *range;
>   	int i;
>   
> -	rbnode = kzalloc(sizeof(*rbnode), GFP_KERNEL);
> +	rbnode = kzalloc(sizeof(*rbnode), map->alloc_flags);
>   	if (!rbnode)
>   		return NULL;
>   
> @@ -346,13 +346,13 @@ regcache_rbtree_node_alloc(struct regmap *map, unsigned int reg)
>   	}
>   
>   	rbnode->block = kmalloc_array(rbnode->blklen, map->cache_word_size,
> -				      GFP_KERNEL);
> +				      map->alloc_flags);
>   	if (!rbnode->block)
>   		goto err_free;
>   
>   	rbnode->cache_present = kcalloc(BITS_TO_LONGS(rbnode->blklen),
>   					sizeof(*rbnode->cache_present),
> -					GFP_KERNEL);
> +					map->alloc_flags);
>   	if (!rbnode->cache_present)
>   		goto err_free_block;
>