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

[Gabor Juhos <j4g8y7@gmail.com>]

2025. 06. 23. 10:58 keltezéssel, Johan Hovold írta:
> [ +CC: Bjorn ]
> 
> On Thu, Jun 19, 2025 at 03:03:50PM +0200, Gabor Juhos wrote:
>> 2025. 06. 19. 12:07 keltezéssel, Johan Hovold írta:
>>> On Wed, Jun 18, 2025 at 09:58:31PM +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
>>>
>>>> Move the memory allocation part of the code outside of the protected
>>>> path to eliminate the warning, and add a note about why it is moved
>>>> to there. Also add memory allocation failure handling, while we are
>>>> at it.
>>>>
>>>> Fixes: d30f83d278a9 ("interconnect: core: Add dynamic id allocation support")
>>>> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
>>>> ---
>>>> Changes in v2:
>>>>   - move memory allocation outside of icc_lock
>>>>   - issue a warning and return without modifying the node name in case of
>>>>     memory allocation failure, and adjust the commit description
>>>>   - remove offered tags from Johan and Bryan
>>>>     Note: since I was not sure that that the added WARN_ON() is a substantial
>>>>     change or not, I have removed the offered tags intentionally to be on the
>>>>     safe side
>>>
>>> Bah, what a mess (thanks for dropping the tags).
>>>
>>> This dynamic id feature looks like a very ad-hoc and badly designed
>>> interface.
>>>
>>> icc_node_add() should not be allocating memory in the first place as it
>>> is not designed to ever fail (e.g. does not return errors).
>>>
>>> Generating the name could have been done as part of of
>>> icc_node_create_dyn() or yet another helper for the caller could have
>>> been added for that. In any case, it should be done before calling
>>> icc_node_add().
>>>
>>> Perhaps the best minimal fix of the regression is to move the allocation
>>> into the two users of this interface. They already handle both dynamic
>>> and non-dynamic node allocation explicitly.
>>
>> Ok, I will change the patch. Just to be clear, do you mean the
>> qcom_icc_rpmh_probe() and qcom_osm_l3_probe() functions, right?
> 
> Yes, indeed.

Ok.

> 
> Apparently this is how it was done in the first six iterations of the
> series adding this and then the author was asked to generalise the name
> generation. That can still be done as a follow up (by the Qualcomm
> folks) after fixing the immediate issues:
> 
> 	https://lore.kernel.org/all/lm6gvcrnd2pcphex4pugxie7m47qlvrgvsvuf75w4uumwoouew@qcuvxeb3u72s


Thanks for digging this out, I have only checked the last two iterations.

>>> Then whoever cares about this code can come up with a common interface
>>> for allocating the name (e.g. move it into icc_node_create_dyn() or add
>>> a new icc_node_init() helper or similar).
> 

Regards,
Gabor