Re: [PATCH v2 1/2] software node: fix refcount leak in software_node_get_next_child()

[Xu Yang]

On Thu, Jun 04, 2026 at 08:50:16PM +0300, Andy Shevchenko wrote:
> On Thu, Jun 04, 2026 at 07:15:26PM +0800, Xu Yang wrote:
> > On Wed, Jun 03, 2026 at 12:40:27PM +0300, Andy Shevchenko wrote:
> > > On Wed, Jun 03, 2026 at 04:44:31PM +0800, Xu Yang wrote:
> 
> ...
> 
> > > >  	struct swnode *p = to_swnode(fwnode);
> > > >  	struct swnode *c = to_swnode(child);
> > > >  
> > > > -	if (!p || list_empty(&p->children) ||
> > > > -	    (c && list_is_last(&c->entry, &p->children))) {
> > > > -		fwnode_handle_put(child);
> > > 
> > > Wouldn't be better to use swnode_get() / swnode_put() instead?
> > > *Yes, we might need to add some NULL checks there.
> > 
> > It's not newly added by me. The software_node_get_next_child() has been using
> > fwnode_handle_get() / fwnode_handle_put() before. In my opinion, this should
> > be fine since they do the same thing here for a swnode.
> 
> It doesn't matter who added that. But according to the point of this patch
> (correct me if I am wrong) is to avoid bumping or dropping reference count for
> the nodes that are *not* of swnode type. Moving away from fwnode_handle_*()
> loop we make the point clear.

Yes.

> 
> See the of_get_next_status_child() implementation, it does *not* use
> fwnode_handle_*() at all. So, making it here to use same approach should
> fix your issue, no?

You are right. I had also noticed this before. Actually, the difference between
OF node and swnode is that OF node uses to_of_node() to filter out non-OF type
fwnodes. Similarly, swnode uses to_swnode() to filter out non-swnode type fwnodes.
So replace fwnode_handle_get() / fwnode_handle_put() with software_node_get() /
software_node_put() does fix the issue.

When I reviewed patch #1 again, I found it already fixes the refcount leak issue
because when it switches to the secondary fwnode, it no longer passes the primary
child to secondary fwnode. So the patch #1 is not needed anymore. I will remove
it in v3.

Thanks for your review!

Thanks,
Xu Yang

> 
> > > > +	if (!p || list_empty(&p->children))
> > > >  		return NULL;
> > > > -	}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>