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