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@xxxxxxxxx

Regards,
Gabor