Re: [PATCH] interconnect: avoid memory allocation when 'icc_bw_lock' is held

[Gabor Juhos <j4g8y7@gmail.com>]

Hi Johan,

Thank you for taking a look into this.

2025. 06. 18. 14:43 keltezéssel, Johan Hovold írta:
> On Thu, May 29, 2025 at 04:46:22PM +0200, Gabor Juhos wrote:
>> The 'icc_bw_lock' mutex is introduced in commit af42269c3523
>> ("interconnect: Fix locking for runpm vs reclaim") in order
>> to decouple serialization of bw aggregation from codepaths
>> that require memory allocation.
>>
>> However commit d30f83d278a9 ("interconnect: core: Add dynamic
>> id allocation support") added a devm_kasprintf() call into a
>> path protected by the 'icc_bw_lock' which causes this lockdep
>> warning (at least on the IPQ9574 platform):
>>
>>     ======================================================
>>     WARNING: possible circular locking dependency detected
>>     6.15.0-next-20250529 #0 Not tainted
>>     ------------------------------------------------------
>>     swapper/0/1 is trying to acquire lock:
>>     ffffffc081df57d8 (icc_bw_lock){+.+.}-{4:4}, at: icc_init+0x8/0x108
>>
>>     but task is already holding lock:
>>     ffffffc081d7db10 (fs_reclaim){+.+.}-{0:0}, at: icc_init+0x28/0x108
>>
>>     which lock already depends on the new lock.
> 
> Thanks for fixing this. I get a similar splat with sc8280xp and the
> icc_ism_l3 driver since 6.16-rc1.
> 
> Georgi, this is a regression that prevents lockdep from being used on a
> bunch of Qualcomm platforms and should be fixed in mainline ASAP (e.g.
> to avoid further locking issues from being introduced).
> 
>> Move the memory allocation part of the code outside of the protected
>> path to eliminate the warning. Also add a note about why it is moved
>> to there,
>>
>> Fixes: d30f83d278a9 ("interconnect: core: Add dynamic id allocation support")
>> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
>> ---
>>  drivers/interconnect/core.c | 14 ++++++++++----
>>  1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
>> index 1a41e59c77f85a811f78986e98401625f4cadfa3..acdb3b8f1e54942dbb1b71ec2b170b08ad709e6b 100644
>> --- a/drivers/interconnect/core.c
>> +++ b/drivers/interconnect/core.c
>> @@ -1023,6 +1023,16 @@ void icc_node_add(struct icc_node *node, struct icc_provider *provider)
>>  		return;
>>  
>>  	mutex_lock(&icc_lock);
>> +
>> +	if (node->id >= ICC_DYN_ID_START) {
>> +		/*
>> +		 * Memory allocation must be done outside of codepaths
>> +		 * protected by icc_bw_lock.
>> +		 */
>> +		node->name = devm_kasprintf(provider->dev, GFP_KERNEL, "%s@%s",
>> +					    node->name, dev_name(provider->dev));
>> +	}
> 
> The node name has already been set by the caller and the node has not
> been added yet, so I think you should move this before taking the
> icc_lock.

It seems reasonable. I will send a modified version, which also contains
handling of memory allocation failures.

> 
>> +
>>  	mutex_lock(&icc_bw_lock);
>>  
>>  	node->provider = provider;
> 
> With that addressed, feel free to add my:
> 
> Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
> Tested-by: Johan Hovold <johan+linaro@kernel.org>


Regards,
Gabor