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

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

[Gabor Juhos]

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.

    the existing dependency chain (in reverse order) is:

    -> #1 (fs_reclaim){+.+.}-{0:0}:
           fs_reclaim_acquire+0x7c/0xb8
           slab_alloc_node.isra.0+0x48/0x188
           __kmalloc_node_track_caller_noprof+0xa4/0x2b8
           devm_kmalloc+0x5c/0x138
           devm_kvasprintf+0x6c/0xb8
           devm_kasprintf+0x50/0x68
           icc_node_add+0xbc/0x160
           icc_clk_register+0x15c/0x230
           devm_icc_clk_register+0x20/0x90
           qcom_cc_really_probe+0x320/0x338
           nss_cc_ipq9574_probe+0xac/0x1e8
           platform_probe+0x70/0xd0
           really_probe+0xdc/0x3b8
           __driver_probe_device+0x94/0x178
           driver_probe_device+0x48/0xf0
           __driver_attach+0x13c/0x208
           bus_for_each_dev+0x6c/0xb8
           driver_attach+0x2c/0x40
           bus_add_driver+0x100/0x250
           driver_register+0x68/0x138
           __platform_driver_register+0x2c/0x40
           nss_cc_ipq9574_driver_init+0x24/0x38
           do_one_initcall+0x88/0x340
           kernel_init_freeable+0x2ac/0x4f8
           kernel_init+0x28/0x1e8
           ret_from_fork+0x10/0x20

    -> #0 (icc_bw_lock){+.+.}-{4:4}:
           __lock_acquire+0x1348/0x2090
           lock_acquire+0x108/0x2d8
           icc_init+0x50/0x108
           do_one_initcall+0x88/0x340
           kernel_init_freeable+0x2ac/0x4f8
           kernel_init+0x28/0x1e8
           ret_from_fork+0x10/0x20

    other info that might help us debug this:

     Possible unsafe locking scenario:

           CPU0                    CPU1
           ----                    ----
      lock(fs_reclaim);
                                   lock(icc_bw_lock);
                                   lock(fs_reclaim);
      lock(icc_bw_lock);

     *** DEADLOCK ***

    1 lock held by swapper/0/1:
     #0: ffffffc081d7db10 (fs_reclaim){+.+.}-{0:0}, at: icc_init+0x28/0x108

    stack backtrace:
    CPU: 3 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.15.0-next-20250529 #0 NONE
    Hardware name: Qualcomm Technologies, Inc. IPQ9574/AP-AL02-C7 (DT)
    Call trace:
     show_stack+0x20/0x38 (C)
     dump_stack_lvl+0x90/0xd0
     dump_stack+0x18/0x28
     print_circular_bug+0x334/0x448
     check_noncircular+0x12c/0x140
     __lock_acquire+0x1348/0x2090
     lock_acquire+0x108/0x2d8
     icc_init+0x50/0x108
     do_one_initcall+0x88/0x340
     kernel_init_freeable+0x2ac/0x4f8
     kernel_init+0x28/0x1e8
     ret_from_fork+0x10/0x20

The icc_node_add() functions is not designed to fail, and as such it
should not do any memory allocation. In order to avoid this, move the
name generation directly into the functions which are using the dynamic
id feature.

The change in the icc core has been tested on the IPQ9574 platform,
where it eliminates the lockdep splat indicated above. The changes in
the 'icc-rpmh' and 'osm-l3' drivers are compile tested only due to lack
of suitable hardware.

Fixes: d30f83d278a9 ("interconnect: core: Add dynamic id allocation support")
Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
---
Changes in v3:
  - move memory allocation out from the icc_node_add() function directly into
    the users of the dynamic id feature
  - adjust commit description according to the changes
  - Link to v2: https://lore.kernel.org/r/20250618-icc-bw-lockdep-v2-1-3223da346765@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
  - Link to v1: https://lore.kernel.org/r/20250529-icc-bw-lockdep-v1-1-3d714b6a9374@gmail.com
---
 drivers/interconnect/core.c          |  4 ----
 drivers/interconnect/qcom/icc-rpmh.c | 20 ++++++++++++++++++--
 drivers/interconnect/qcom/osm-l3.c   | 10 +++++++++-
 3 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
index 1a41e59c77f85a811f78986e98401625f4cadfa3..f0bdcdcf222af133fcb0a346fa347c5a829e62e6 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -1038,10 +1038,6 @@ void icc_node_add(struct icc_node *node, struct icc_provider *provider)
 	node->avg_bw = node->init_avg;
 	node->peak_bw = node->init_peak;
 
-	if (node->id >= ICC_DYN_ID_START)
-		node->name = devm_kasprintf(provider->dev, GFP_KERNEL, "%s@%s",
-					    node->name, dev_name(provider->dev));
-
 	if (node->avg_bw || node->peak_bw) {
 		if (provider->pre_aggregate)
 			provider->pre_aggregate(node);
diff --git a/drivers/interconnect/qcom/icc-rpmh.c b/drivers/interconnect/qcom/icc-rpmh.c
index 41bfc6e7ee1d53d34b919dd8afa97698bc69d79c..fa4ef78678eff10e83557035ba572010b51ff50c 100644
--- a/drivers/interconnect/qcom/icc-rpmh.c
+++ b/drivers/interconnect/qcom/icc-rpmh.c
@@ -276,13 +276,17 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev)
 		qcom_icc_bcm_init(qp->bcms[i], dev);
 
 	for (i = 0; i < num_nodes; i++) {
+		bool is_dyn_node = false;
+
 		qn = qnodes[i];
 		if (!qn)
 			continue;
 
 		if (desc->alloc_dyn_id) {
-			if (!qn->node)
+			if (!qn->node) {
 				qn->node = icc_node_create_dyn();
+				is_dyn_node = true;
+			}
 			node = qn->node;
 		} else {
 			node = icc_node_create(qn->id);
@@ -293,7 +297,19 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev)
 			goto err_remove_nodes;
 		}
 
-		node->name = qn->name;
+		if (is_dyn_node) {
+			node->name = devm_kasprintf(provider->dev, GFP_KERNEL,
+						    "%s@%s", qn->name,
+						    dev_name(provider->dev));
+			if (!node->name) {
+				icc_node_destroy(node->id);
+				ret = -ENOMEM;
+				goto err_remove_nodes;
+			}
+		} else {
+			node->name = qn->name;
+		}
+
 		node->data = qn;
 		icc_node_add(node, provider);
 
diff --git a/drivers/interconnect/qcom/osm-l3.c b/drivers/interconnect/qcom/osm-l3.c
index baecbf2533f76cbf92bb2451979c4db57f8e4a2b..ed59fa73f0d70a5dfdb658ff606ef82977f04bcb 100644
--- a/drivers/interconnect/qcom/osm-l3.c
+++ b/drivers/interconnect/qcom/osm-l3.c
@@ -236,7 +236,15 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
 			goto err;
 		}
 
-		node->name = qnodes[i]->name;
+		node->name = devm_kasprintf(provider->dev, GFP_KERNEL, "%s@%s",
+					    qnodes[i]->name,
+					    dev_name(provider->dev));
+		if (!node->name) {
+			icc_node_destroy(node->id);
+			ret = -ENOMEM;
+			goto err;
+		}
+
 		/* Cast away const and add it back in qcom_osm_l3_set() */
 		node->data = (void *)qnodes[i];
 		icc_node_add(node, provider);

---
base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
change-id: 20250529-icc-bw-lockdep-ed030d892a19

Best regards,
-- 
Gabor Juhos <j4g8y7@gmail.com>

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

[Johan Hovold]

On Wed, Jun 25, 2025 at 01:25:04PM +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):

> The icc_node_add() functions is not designed to fail, and as such it
> should not do any memory allocation. In order to avoid this, move the
> name generation directly into the functions which are using the dynamic
> id feature.
> 
> The change in the icc core has been tested on the IPQ9574 platform,
> where it eliminates the lockdep splat indicated above. The changes in
> the 'icc-rpmh' and 'osm-l3' drivers are compile tested only due to lack
> of suitable hardware.
> 
> Fixes: d30f83d278a9 ("interconnect: core: Add dynamic id allocation support")
> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
> ---
> Changes in v3:
>   - move memory allocation out from the icc_node_add() function directly into
>     the users of the dynamic id feature
>   - adjust commit description according to the changes
>   - Link to v2: https://lore.kernel.org/r/20250618-icc-bw-lockdep-v2-1-3223da346765@gmail.com

Thanks for the update. This looks correct to me, except possibly for an
existing issue in the rpmh driver (see below).

I've tested it on the sc8280xp-crd with the osm-l3 driver so with the
rpmh issue resolved, feel free to add:

Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
Tested-by: Johan Hovold <johan+linaro@kernel.org>

> diff --git a/drivers/interconnect/qcom/icc-rpmh.c b/drivers/interconnect/qcom/icc-rpmh.c
> index 41bfc6e7ee1d53d34b919dd8afa97698bc69d79c..fa4ef78678eff10e83557035ba572010b51ff50c 100644
> --- a/drivers/interconnect/qcom/icc-rpmh.c
> +++ b/drivers/interconnect/qcom/icc-rpmh.c
> @@ -276,13 +276,17 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev)
>  		qcom_icc_bcm_init(qp->bcms[i], dev);
>  
>  	for (i = 0; i < num_nodes; i++) {
> +		bool is_dyn_node = false;
> +
>  		qn = qnodes[i];
>  		if (!qn)
>  			continue;
>  
>  		if (desc->alloc_dyn_id) {
> -			if (!qn->node)
> +			if (!qn->node) {

AFAICS, qn->node will currently never be set here and I'm not sure why
commit 7f9560a3bebe ("interconnect: qcom: icc-rpmh: Add dynamic icc node
id support") added this check, or even the node field to struct
qcom_icc_desc for that matter.

But if there's some future use case for this, then you may or may not
need to make sure that a name is allocated also in that case.

And that could be done by simply checking if node->id >=
ICC_DYN_ID_START instead of using a boolean flag below. That may be
preferred either way.

What do you think?

>  				qn->node = icc_node_create_dyn();
> +				is_dyn_node = true;
> +			}
>  			node = qn->node;
>  		} else {
>  			node = icc_node_create(qn->id);
> @@ -293,7 +297,19 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev)
>  			goto err_remove_nodes;
>  		}
>  
> -		node->name = qn->name;
> +		if (is_dyn_node) {
> +			node->name = devm_kasprintf(provider->dev, GFP_KERNEL,
> +						    "%s@%s", qn->name,
> +						    dev_name(provider->dev));
> +			if (!node->name) {
> +				icc_node_destroy(node->id);
> +				ret = -ENOMEM;
> +				goto err_remove_nodes;
> +			}
> +		} else {
> +			node->name = qn->name;
> +		}
> +
>  		node->data = qn;
>  		icc_node_add(node, provider);

Johan

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

[Johan Hovold]

On Wed, Jun 25, 2025 at 02:30:15PM +0200, Johan Hovold wrote:
> On Wed, Jun 25, 2025 at 01:25:04PM +0200, Gabor Juhos wrote:

> > @@ -276,13 +276,17 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev)
> >  		qcom_icc_bcm_init(qp->bcms[i], dev);
> >  
> >  	for (i = 0; i < num_nodes; i++) {
> > +		bool is_dyn_node = false;
> > +
> >  		qn = qnodes[i];
> >  		if (!qn)
> >  			continue;
> >  
> >  		if (desc->alloc_dyn_id) {
> > -			if (!qn->node)
> > +			if (!qn->node) {
> 
> AFAICS, qn->node will currently never be set here and I'm not sure why
> commit 7f9560a3bebe ("interconnect: qcom: icc-rpmh: Add dynamic icc node
> id support") added this check, or even the node field to struct
> qcom_icc_desc for that matter.
> 
> But if there's some future use case for this, then you may or may not
> need to make sure that a name is allocated also in that case.

Ok, I see what's going on. The qn->node may have been (pre-) allocated
in icc_link_nodes() dynamically, which means you need to make sure to
generate a name also in that case.

> And that could be done by simply checking if node->id >=
> ICC_DYN_ID_START instead of using a boolean flag below. That may be
> preferred either way.

So you should probably use node->id to determine this.

Johan

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

[Gabor Juhos]

2025. 06. 25. 14:41 keltezéssel, Johan Hovold írta:
> On Wed, Jun 25, 2025 at 02:30:15PM +0200, Johan Hovold wrote:
>> On Wed, Jun 25, 2025 at 01:25:04PM +0200, Gabor Juhos wrote:
> 
>>> @@ -276,13 +276,17 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev)
>>>  		qcom_icc_bcm_init(qp->bcms[i], dev);
>>>  
>>>  	for (i = 0; i < num_nodes; i++) {
>>> +		bool is_dyn_node = false;
>>> +
>>>  		qn = qnodes[i];
>>>  		if (!qn)
>>>  			continue;
>>>  
>>>  		if (desc->alloc_dyn_id) {
>>> -			if (!qn->node)
>>> +			if (!qn->node) {
>>
>> AFAICS, qn->node will currently never be set here and I'm not sure why
>> commit 7f9560a3bebe ("interconnect: qcom: icc-rpmh: Add dynamic icc node
>> id support") added this check, or even the node field to struct
>> qcom_icc_desc for that matter.
>>
>> But if there's some future use case for this, then you may or may not
>> need to make sure that a name is allocated also in that case.
> 
> Ok, I see what's going on. The qn->node may have been (pre-) allocated
> in icc_link_nodes() dynamically, which means you need to make sure to
> generate a name also in that case.
> 
>> And that could be done by simply checking if node->id >=
>> ICC_DYN_ID_START instead of using a boolean flag below. That may be
>> preferred either way.
> 
> So you should probably use node->id to determine this.

You are right. The problem is that ICC_DYN_ID_START is only visible from the
core code. Either we have to move that into the 'interconnect-provider.h' header
or we have to add an icc_node_is_dynamic() helper or something similar.

Which is the preferred solution?

Regards,
Gabor

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

[Johan Hovold]

On Wed, Jun 25, 2025 at 03:15:53PM +0200, Gabor Juhos wrote:
> 2025. 06. 25. 14:41 keltezéssel, Johan Hovold írta:
> > On Wed, Jun 25, 2025 at 02:30:15PM +0200, Johan Hovold wrote:
> >> On Wed, Jun 25, 2025 at 01:25:04PM +0200, Gabor Juhos wrote:
> > 
> >>> @@ -276,13 +276,17 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev)
> >>>  		qcom_icc_bcm_init(qp->bcms[i], dev);
> >>>  
> >>>  	for (i = 0; i < num_nodes; i++) {
> >>> +		bool is_dyn_node = false;
> >>> +
> >>>  		qn = qnodes[i];
> >>>  		if (!qn)
> >>>  			continue;
> >>>  
> >>>  		if (desc->alloc_dyn_id) {
> >>> -			if (!qn->node)
> >>> +			if (!qn->node) {
> >>
> >> AFAICS, qn->node will currently never be set here and I'm not sure why
> >> commit 7f9560a3bebe ("interconnect: qcom: icc-rpmh: Add dynamic icc node
> >> id support") added this check, or even the node field to struct
> >> qcom_icc_desc for that matter.
> >>
> >> But if there's some future use case for this, then you may or may not
> >> need to make sure that a name is allocated also in that case.
> > 
> > Ok, I see what's going on. The qn->node may have been (pre-) allocated
> > in icc_link_nodes() dynamically, which means you need to make sure to
> > generate a name also in that case.
> > 
> >> And that could be done by simply checking if node->id >=
> >> ICC_DYN_ID_START instead of using a boolean flag below. That may be
> >> preferred either way.
> > 
> > So you should probably use node->id to determine this.
> 
> You are right. The problem is that ICC_DYN_ID_START is only visible from the
> core code. Either we have to move that into the 'interconnect-provider.h' header
> or we have to add an icc_node_is_dynamic() helper or something similar.
> 
> Which is the preferred solution?

I think adding a helper like icc_node_is_dynamic() in a separate
preparatory patch is best here.

If it wasn't for nodes now being created also in icc_link_nodes() we
could otherwise perhaps just as well have moved the name generation into
icc_node_create_dyn(). Now it seems we'd need a new helper to set the
name (or add error handling for every icc_node_add()), but we've already
spent way too much time trying to clean up this mess...

Johan

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

[Gabor Juhos]

2025. 06. 25. 16:02 keltezéssel, Johan Hovold írta:
> On Wed, Jun 25, 2025 at 03:15:53PM +0200, Gabor Juhos wrote:
>> 2025. 06. 25. 14:41 keltezéssel, Johan Hovold írta:
>>> On Wed, Jun 25, 2025 at 02:30:15PM +0200, Johan Hovold wrote:
>>>> On Wed, Jun 25, 2025 at 01:25:04PM +0200, Gabor Juhos wrote:
>>>
>>>>> @@ -276,13 +276,17 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev)
>>>>>  		qcom_icc_bcm_init(qp->bcms[i], dev);
>>>>>  
>>>>>  	for (i = 0; i < num_nodes; i++) {
>>>>> +		bool is_dyn_node = false;
>>>>> +
>>>>>  		qn = qnodes[i];
>>>>>  		if (!qn)
>>>>>  			continue;
>>>>>  
>>>>>  		if (desc->alloc_dyn_id) {
>>>>> -			if (!qn->node)
>>>>> +			if (!qn->node) {
>>>>
>>>> AFAICS, qn->node will currently never be set here and I'm not sure why
>>>> commit 7f9560a3bebe ("interconnect: qcom: icc-rpmh: Add dynamic icc node
>>>> id support") added this check, or even the node field to struct
>>>> qcom_icc_desc for that matter.
>>>>
>>>> But if there's some future use case for this, then you may or may not
>>>> need to make sure that a name is allocated also in that case.
>>>
>>> Ok, I see what's going on. The qn->node may have been (pre-) allocated
>>> in icc_link_nodes() dynamically, which means you need to make sure to
>>> generate a name also in that case.
>>>
>>>> And that could be done by simply checking if node->id >=
>>>> ICC_DYN_ID_START instead of using a boolean flag below. That may be
>>>> preferred either way.
>>>
>>> So you should probably use node->id to determine this.
>>
>> You are right. The problem is that ICC_DYN_ID_START is only visible from the
>> core code. Either we have to move that into the 'interconnect-provider.h' header
>> or we have to add an icc_node_is_dynamic() helper or something similar.
>>
>> Which is the preferred solution?
> 
> I think adding a helper like icc_node_is_dynamic() in a separate
> preparatory patch is best here.

Ok, although i don't see why it should be done in a separate patch.

> If it wasn't for nodes now being created also in icc_link_nodes() we
> could otherwise perhaps just as well have moved the name generation into
> icc_node_create_dyn().

I already have tried to add the name allocation to the icc_node_create_dyn()
function, but I was not satisfied with the result. B


> Now it seems we'd need a new helper to set the
> name (or add error handling for every icc_node_add()), but we've already
> spent way too much time trying to clean up this mess...

True, and the patch is getting more and more complicated with each iteration. :)

Nevertheless, I think that we can have a simpler solution. We can create a
wrapper around icc_node_add(), and allocate the name from there. I mean
something like this:

int icc_node_add_dyn(struct icc_node *node, struct icc_provider *provider)
{
	if (node->id >= ICC_DYN_ID_START) {
		node->name = devm_kasprintf(provider->dev, GFP_KERNEL, "%s@%s",
					    node->name, dev_name(provider->dev));
		if (!node->name)
			return -ENOMEM;
	}

	icc_node_add(node, provider);
	return 0;
}

Then we can change the qcom_icc_rpmh_probe() and qcom_osm_l3_probe() to use the
wrapper instead of the plain version. Since the wrapper can return an error
code, it can be handled in the callers. And as a bonus, we don't have to touch
other users of icc_node_add() at all.

Of course we can still continue the previous approach.

What do you think?

Regards,
Gabor

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

[Johan Hovold]

On Wed, Jun 25, 2025 at 05:35:07PM +0200, Gabor Juhos wrote:
> 2025. 06. 25. 16:02 keltezéssel, Johan Hovold írta:
> > On Wed, Jun 25, 2025 at 03:15:53PM +0200, Gabor Juhos wrote:
> >> 2025. 06. 25. 14:41 keltezéssel, Johan Hovold írta:
> >>> On Wed, Jun 25, 2025 at 02:30:15PM +0200, Johan Hovold wrote:
> >>>> On Wed, Jun 25, 2025 at 01:25:04PM +0200, Gabor Juhos wrote:
> >>>
> >>>>> @@ -276,13 +276,17 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev)
> >>>>>  		qcom_icc_bcm_init(qp->bcms[i], dev);
> >>>>>  
> >>>>>  	for (i = 0; i < num_nodes; i++) {
> >>>>> +		bool is_dyn_node = false;
> >>>>> +
> >>>>>  		qn = qnodes[i];
> >>>>>  		if (!qn)
> >>>>>  			continue;
> >>>>>  
> >>>>>  		if (desc->alloc_dyn_id) {
> >>>>> -			if (!qn->node)
> >>>>> +			if (!qn->node) {
> >>>>
> >>>> AFAICS, qn->node will currently never be set here and I'm not sure why
> >>>> commit 7f9560a3bebe ("interconnect: qcom: icc-rpmh: Add dynamic icc node
> >>>> id support") added this check, or even the node field to struct
> >>>> qcom_icc_desc for that matter.
> >>>>
> >>>> But if there's some future use case for this, then you may or may not
> >>>> need to make sure that a name is allocated also in that case.
> >>>
> >>> Ok, I see what's going on. The qn->node may have been (pre-) allocated
> >>> in icc_link_nodes() dynamically, which means you need to make sure to
> >>> generate a name also in that case.
> >>>
> >>>> And that could be done by simply checking if node->id >=
> >>>> ICC_DYN_ID_START instead of using a boolean flag below. That may be
> >>>> preferred either way.
> >>>
> >>> So you should probably use node->id to determine this.
> >>
> >> You are right. The problem is that ICC_DYN_ID_START is only visible from the
> >> core code. Either we have to move that into the 'interconnect-provider.h' header
> >> or we have to add an icc_node_is_dynamic() helper or something similar.
> >>
> >> Which is the preferred solution?
> > 
> > I think adding a helper like icc_node_is_dynamic() in a separate
> > preparatory patch is best here.
> 
> Ok, although i don't see why it should be done in a separate patch.

Since this is a 6.16-rc1 regression (which does not need to be
backported) it should be fine to add the helper in the same patch.

> > If it wasn't for nodes now being created also in icc_link_nodes() we
> > could otherwise perhaps just as well have moved the name generation into
> > icc_node_create_dyn().
> 
> I already have tried to add the name allocation to the icc_node_create_dyn()
> function, but I was not satisfied with the result. B

Yeah, the preallocation complicates this too.

> > Now it seems we'd need a new helper to set the
> > name (or add error handling for every icc_node_add()), but we've already
> > spent way too much time trying to clean up this mess...
> 
> True, and the patch is getting more and more complicated with each iteration. :)
> 
> Nevertheless, I think that we can have a simpler solution. We can create a
> wrapper around icc_node_add(), and allocate the name from there. I mean
> something like this:
> 
> int icc_node_add_dyn(struct icc_node *node, struct icc_provider *provider)
> {
> 	if (node->id >= ICC_DYN_ID_START) {
> 		node->name = devm_kasprintf(provider->dev, GFP_KERNEL, "%s@%s",
> 					    node->name, dev_name(provider->dev));
> 		if (!node->name)
> 			return -ENOMEM;
> 	}
> 
> 	icc_node_add(node, provider);
> 	return 0;
> }
> 
> Then we can change the qcom_icc_rpmh_probe() and qcom_osm_l3_probe() to use the
> wrapper instead of the plain version. Since the wrapper can return an error
> code, it can be handled in the callers. And as a bonus, we don't have to touch
> other users of icc_node_add() at all.

That would be a smaller change indeed, but I don't think we should
change the current model of:

	node = icc_node_create()
	<manual initialisation of the node>
	icc_node_add(node)

So given that we need to add some new helper (or export the internal ID
define), I think we might as well add that icc_node_set_name() helper I
suggested might be the long term solution here directly.

I also don't like hiding device managed allocations (those should be done
explicitly with devm_ prefix helpers so that the callers can reason
about ordering) so I dropped that as well.

So something like the below. Note that this could be extended with a
name-allocated flag and an appropriate warning somewhere later if anyone
is worried about drivers failing to use the helper.

Note that we can't use kfree_const() unconditionally as I initially
intended as apparently some interconnect providers already allocate
names for non-dynamic nodes.

Johan


From 8f4f936ebd87f9ceb0a799f89f322a25c6fcd05b Mon Sep 17 00:00:00 2001
From: Gabor Juhos <j4g8y7@gmail.com>
Date: Wed, 25 Jun 2025 13:25:04 +0200
Subject: [PATCH] interconnect: avoid memory allocation when 'icc_bw_lock' is
 held

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.

    the existing dependency chain (in reverse order) is:

    -> #1 (fs_reclaim){+.+.}-{0:0}:
           fs_reclaim_acquire+0x7c/0xb8
           slab_alloc_node.isra.0+0x48/0x188
           __kmalloc_node_track_caller_noprof+0xa4/0x2b8
           devm_kmalloc+0x5c/0x138
           devm_kvasprintf+0x6c/0xb8
           devm_kasprintf+0x50/0x68
           icc_node_add+0xbc/0x160
           icc_clk_register+0x15c/0x230
           devm_icc_clk_register+0x20/0x90
           qcom_cc_really_probe+0x320/0x338
           nss_cc_ipq9574_probe+0xac/0x1e8
           platform_probe+0x70/0xd0
           really_probe+0xdc/0x3b8
           __driver_probe_device+0x94/0x178
           driver_probe_device+0x48/0xf0
           __driver_attach+0x13c/0x208
           bus_for_each_dev+0x6c/0xb8
           driver_attach+0x2c/0x40
           bus_add_driver+0x100/0x250
           driver_register+0x68/0x138
           __platform_driver_register+0x2c/0x40
           nss_cc_ipq9574_driver_init+0x24/0x38
           do_one_initcall+0x88/0x340
           kernel_init_freeable+0x2ac/0x4f8
           kernel_init+0x28/0x1e8
           ret_from_fork+0x10/0x20

    -> #0 (icc_bw_lock){+.+.}-{4:4}:
           __lock_acquire+0x1348/0x2090
           lock_acquire+0x108/0x2d8
           icc_init+0x50/0x108
           do_one_initcall+0x88/0x340
           kernel_init_freeable+0x2ac/0x4f8
           kernel_init+0x28/0x1e8
           ret_from_fork+0x10/0x20

    other info that might help us debug this:

     Possible unsafe locking scenario:

           CPU0                    CPU1
           ----                    ----
      lock(fs_reclaim);
                                   lock(icc_bw_lock);
                                   lock(fs_reclaim);
      lock(icc_bw_lock);

     *** DEADLOCK ***

    1 lock held by swapper/0/1:
     #0: ffffffc081d7db10 (fs_reclaim){+.+.}-{0:0}, at: icc_init+0x28/0x108

    stack backtrace:
    CPU: 3 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.15.0-next-20250529 #0 NONE
    Hardware name: Qualcomm Technologies, Inc. IPQ9574/AP-AL02-C7 (DT)
    Call trace:
     show_stack+0x20/0x38 (C)
     dump_stack_lvl+0x90/0xd0
     dump_stack+0x18/0x28
     print_circular_bug+0x334/0x448
     check_noncircular+0x12c/0x140
     __lock_acquire+0x1348/0x2090
     lock_acquire+0x108/0x2d8
     icc_init+0x50/0x108
     do_one_initcall+0x88/0x340
     kernel_init_freeable+0x2ac/0x4f8
     kernel_init+0x28/0x1e8
     ret_from_fork+0x10/0x20

The icc_node_add() functions is not designed to fail, and as such it
should not do any memory allocation. In order to avoid this, add a new
helper function for the name generation to be called by the functions
which are using the dynamic id feature.

Fixes: d30f83d278a9 ("interconnect: core: Add dynamic id allocation support")
Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
Link: https://lore.kernel.org/r/20250625-icc-bw-lockdep-v3-1-2b8f8b8987c4@gmail.com
Co-developed-by: Johan Hovold <johan+linaro@kernel.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/interconnect/core.c           | 29 +++++++++++++++++++++++----
 drivers/interconnect/qcom/icc-rpmh.c  |  7 ++++++-
 drivers/interconnect/qcom/osm-l3.c    |  7 ++++++-
 include/linux/interconnect-provider.h |  6 ++++++
 4 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
index 1a41e59c77f8..fdfc755bbd43 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -906,10 +906,35 @@ void icc_node_destroy(int id)
 		return;
 
 	kfree(node->links);
+	if (node->id >= ICC_DYN_ID_START)
+		kfree(node->name);
 	kfree(node);
 }
 EXPORT_SYMBOL_GPL(icc_node_destroy);
 
+/**
+ * icc_node_set_name() - set node name
+ * @node: node
+ * @provider: node provider
+ * @name: node name
+ *
+ * Return: 0 on success, or -ENOMEM on allocation failure
+ */
+int icc_node_set_name(struct icc_node *node, const struct icc_provider *provider, const char *name)
+{
+	if (node->id >= ICC_DYN_ID_START) {
+		node->name = kasprintf(GFP_KERNEL, "%s@%s", name,
+				       dev_name(provider->dev));
+		if (!node->name)
+			return -ENOMEM;
+	} else {
+		node->name = name;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(icc_node_set_name);
+
 /**
  * icc_link_nodes() - create link between two nodes
  * @src_node: source node
@@ -1038,10 +1063,6 @@ void icc_node_add(struct icc_node *node, struct icc_provider *provider)
 	node->avg_bw = node->init_avg;
 	node->peak_bw = node->init_peak;
 
-	if (node->id >= ICC_DYN_ID_START)
-		node->name = devm_kasprintf(provider->dev, GFP_KERNEL, "%s@%s",
-					    node->name, dev_name(provider->dev));
-
 	if (node->avg_bw || node->peak_bw) {
 		if (provider->pre_aggregate)
 			provider->pre_aggregate(node);
diff --git a/drivers/interconnect/qcom/icc-rpmh.c b/drivers/interconnect/qcom/icc-rpmh.c
index 41bfc6e7ee1d..001404e91041 100644
--- a/drivers/interconnect/qcom/icc-rpmh.c
+++ b/drivers/interconnect/qcom/icc-rpmh.c
@@ -293,7 +293,12 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev)
 			goto err_remove_nodes;
 		}
 
-		node->name = qn->name;
+		ret = icc_node_set_name(node, provider, qn->name);
+		if (ret) {
+			icc_node_destroy(node->id);
+			goto err_remove_nodes;
+		}
+
 		node->data = qn;
 		icc_node_add(node, provider);
 
diff --git a/drivers/interconnect/qcom/osm-l3.c b/drivers/interconnect/qcom/osm-l3.c
index baecbf2533f7..b33f00da1880 100644
--- a/drivers/interconnect/qcom/osm-l3.c
+++ b/drivers/interconnect/qcom/osm-l3.c
@@ -236,7 +236,12 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
 			goto err;
 		}
 
-		node->name = qnodes[i]->name;
+		ret = icc_node_set_name(node, provider, qnodes[i]->name);
+		if (ret) {
+			icc_node_destroy(node->id);
+			goto err;
+		}
+
 		/* Cast away const and add it back in qcom_osm_l3_set() */
 		node->data = (void *)qnodes[i];
 		icc_node_add(node, provider);
diff --git a/include/linux/interconnect-provider.h b/include/linux/interconnect-provider.h
index 55cfebc658e6..37093b6e1af9 100644
--- a/include/linux/interconnect-provider.h
+++ b/include/linux/interconnect-provider.h
@@ -119,6 +119,7 @@ int icc_std_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
 struct icc_node *icc_node_create_dyn(void);
 struct icc_node *icc_node_create(int id);
 void icc_node_destroy(int id);
+int icc_node_set_name(struct icc_node *node, const struct icc_provider *provider, const char *name);
 int icc_link_nodes(struct icc_node *src_node, struct icc_node **dst_node);
 int icc_link_create(struct icc_node *node, const int dst_id);
 void icc_node_add(struct icc_node *node, struct icc_provider *provider);
@@ -152,6 +153,11 @@ static inline void icc_node_destroy(int id)
 {
 }
 
+static int icc_node_set_name(struct icc_node *node, const struct icc_provider *provider, const char *name)
+{
+	return -EOPNOTSUPP;
+}
+
 static inline int icc_link_nodes(struct icc_node *src_node, struct icc_node **dst_node)
 {
 	return -EOPNOTSUPP;
-- 
2.49.0

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

[Gabor Juhos]

2025. 06. 26. 11:30 keltezéssel, Johan Hovold írta:

...

>> Nevertheless, I think that we can have a simpler solution. We can create a
>> wrapper around icc_node_add(), and allocate the name from there. I mean
>> something like this:
>>
>> int icc_node_add_dyn(struct icc_node *node, struct icc_provider *provider)
>> {
>> 	if (node->id >= ICC_DYN_ID_START) {
>> 		node->name = devm_kasprintf(provider->dev, GFP_KERNEL, "%s@%s",
>> 					    node->name, dev_name(provider->dev));
>> 		if (!node->name)
>> 			return -ENOMEM;
>> 	}
>>
>> 	icc_node_add(node, provider);
>> 	return 0;
>> }
>>
>> Then we can change the qcom_icc_rpmh_probe() and qcom_osm_l3_probe() to use the
>> wrapper instead of the plain version. Since the wrapper can return an error
>> code, it can be handled in the callers. And as a bonus, we don't have to touch
>> other users of icc_node_add() at all.
> 
> That would be a smaller change indeed, but I don't think we should
> change the current model of:
> 
> 	node = icc_node_create()
> 	<manual initialisation of the node>
> 	icc_node_add(node)
> 
> So given that we need to add some new helper (or export the internal ID
> define), I think we might as well add that icc_node_set_name() helper I
> suggested might be the long term solution here directly.

Ok, I see the reason behind that.

> 
> I also don't like hiding device managed allocations (those should be done
> explicitly with devm_ prefix helpers so that the callers can reason
> about ordering) so I dropped that as well.
> 
> So something like the below. 

It seems to be a cleaner solution indeed.

> Note that this could be extended with a
> name-allocated flag and an appropriate warning somewhere later if anyone
> is worried about drivers failing to use the helper.
> 
> Note that we can't use kfree_const() unconditionally as I initially
> intended as apparently some interconnect providers already allocate
> names for non-dynamic nodes.

Not that I want to worry about anything, but for the sake of completeness I have
to note something. Theoretically, freeing the name in icc_node_destroy() could
cause the following on IPQ9574 under some circumstances:

  [    4.003692] page: refcount:0 mapcount:0 mapping:0000000000000000 index:0xffffff80047e4180 pfn:0x447e4
  [    4.008439] flags: 0x0(zone=0)
  [    4.017545] raw: 0000000000000000 fffffffec0000448 ffffff803fdbb518 0000000000000000
  [    4.020480] raw: ffffff80047e4180 0000000000150000 00000000ffffffff 0000000000000000
  [    4.028413] page dumped because: Not a kmalloc allocation

It is not a problem of your patch though. The root cause of this is the same
as why I saw the lockdep warning on the platform originally. The reason is
that the static node ids used by the 'nsscc-ipq9574' driver are within the
range of dynamic ids. Nevertheless, I have sent a patch [1] to fix that
already.

Despite the note above, your proposal looks good to me. Would you like to
send it as a formal patch, or shall I do it?

[1] https://lore.kernel.org/r/20250625-icc-dyn-id-fix-v1-1-127cb5498449@gmail.com

Regards,
Gabor

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

[Johan Hovold]

On Thu, Jun 26, 2025 at 05:00:42PM +0200, Gabor Juhos wrote:
> 2025. 06. 26. 11:30 keltezéssel, Johan Hovold írta:

> > Note that this could be extended with a
> > name-allocated flag and an appropriate warning somewhere later if anyone
> > is worried about drivers failing to use the helper.
> > 
> > Note that we can't use kfree_const() unconditionally as I initially
> > intended as apparently some interconnect providers already allocate
> > names for non-dynamic nodes.
> 
> Not that I want to worry about anything, but for the sake of completeness I have
> to note something. Theoretically, freeing the name in icc_node_destroy() could
> cause the following on IPQ9574 under some circumstances:
> 
>   [    4.003692] page: refcount:0 mapcount:0 mapping:0000000000000000 index:0xffffff80047e4180 pfn:0x447e4
>   [    4.008439] flags: 0x0(zone=0)
>   [    4.017545] raw: 0000000000000000 fffffffec0000448 ffffff803fdbb518 0000000000000000
>   [    4.020480] raw: ffffff80047e4180 0000000000150000 00000000ffffffff 0000000000000000
>   [    4.028413] page dumped because: Not a kmalloc allocation
> 
> It is not a problem of your patch though. The root cause of this is the same
> as why I saw the lockdep warning on the platform originally. The reason is
> that the static node ids used by the 'nsscc-ipq9574' driver are within the
> range of dynamic ids. Nevertheless, I have sent a patch [1] to fix that
> already.

Yeah, I saw that patch of yours the other day. Good that you tracked
that down. I'll probably amend the commit message with a comment about
why this triggered on IPQ in the first place too.

> Despite the note above, your proposal looks good to me. Would you like to
> send it as a formal patch, or shall I do it?

I can post it in a bit.

Johan

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

[Gabor Juhos]

2025. 06. 27. 9:11 keltezéssel, Johan Hovold írta:
> On Thu, Jun 26, 2025 at 05:00:42PM +0200, Gabor Juhos wrote:
>> 2025. 06. 26. 11:30 keltezéssel, Johan Hovold írta:
> 
>>> Note that this could be extended with a
>>> name-allocated flag and an appropriate warning somewhere later if anyone
>>> is worried about drivers failing to use the helper.
>>>
>>> Note that we can't use kfree_const() unconditionally as I initially
>>> intended as apparently some interconnect providers already allocate
>>> names for non-dynamic nodes.
>>
>> Not that I want to worry about anything, but for the sake of completeness I have
>> to note something. Theoretically, freeing the name in icc_node_destroy() could
>> cause the following on IPQ9574 under some circumstances:
>>
>>   [    4.003692] page: refcount:0 mapcount:0 mapping:0000000000000000 index:0xffffff80047e4180 pfn:0x447e4
>>   [    4.008439] flags: 0x0(zone=0)
>>   [    4.017545] raw: 0000000000000000 fffffffec0000448 ffffff803fdbb518 0000000000000000
>>   [    4.020480] raw: ffffff80047e4180 0000000000150000 00000000ffffffff 0000000000000000
>>   [    4.028413] page dumped because: Not a kmalloc allocation
>>
>> It is not a problem of your patch though. The root cause of this is the same
>> as why I saw the lockdep warning on the platform originally. The reason is
>> that the static node ids used by the 'nsscc-ipq9574' driver are within the
>> range of dynamic ids. Nevertheless, I have sent a patch [1] to fix that
>> already.
> 
> Yeah, I saw that patch of yours the other day. Good that you tracked
> that down. I'll probably amend the commit message with a comment about
> why this triggered on IPQ in the first place too.
> 
>> Despite the note above, your proposal looks good to me. Would you like to
>> send it as a formal patch, or shall I do it?
> 
> I can post it in a bit.

Great, just saw the final patch. Thank you!

Regards,
Gabor

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

[Johan Hovold]

On Fri, Jun 27, 2025 at 11:36:26AM +0200, Gabor Juhos wrote:
> 2025. 06. 27. 9:11 keltezéssel, Johan Hovold írta:
> > On Thu, Jun 26, 2025 at 05:00:42PM +0200, Gabor Juhos wrote:

> >> Despite the note above, your proposal looks good to me. Would you like to
> >> send it as a formal patch, or shall I do it?
> > 
> > I can post it in a bit.
> 
> Great, just saw the final patch. Thank you!

Here it is for reference:

	https://lore.kernel.org/lkml/20250627075854.26943-1-johan+linaro@kernel.org/

It looks like Georgi picked up the patch from this thread yesterday,
which should be fine as well as I only changed the commit message
(replacing the ipc splat with one from sc8280xp) before posting it as
v4.

Johan

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

[Gabor Juhos]

2025. 06. 27. 11:43 keltezéssel, Johan Hovold írta:
> On Fri, Jun 27, 2025 at 11:36:26AM +0200, Gabor Juhos wrote:
>> 2025. 06. 27. 9:11 keltezéssel, Johan Hovold írta:
>>> On Thu, Jun 26, 2025 at 05:00:42PM +0200, Gabor Juhos wrote:
> 
>>>> Despite the note above, your proposal looks good to me. Would you like to
>>>> send it as a formal patch, or shall I do it?
>>>
>>> I can post it in a bit.
>>
>> Great, just saw the final patch. Thank you!
> 
> Here it is for reference:
> 
> 	https://lore.kernel.org/lkml/20250627075854.26943-1-johan+linaro@kernel.org/
> 
> It looks like Georgi picked up the patch from this thread yesterday,
> which should be fine as well as I only changed the commit message
> (replacing the ipc splat with one from sc8280xp) before posting it as
> v4.

Yes, it is fine. I have tested the icc-fixes branch which contains the IPQ9574
specific patch as well.

Regards,
Gabor