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

[Gabor Juhos <j4g8y7@gmail.com>]

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