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

[Johan Hovold <johan@kernel.org>]

[ +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.

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

> > 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).

Johan