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

[Johan Hovold <johan@kernel.org>]

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