Re: [PATCH] net: ethernet: bgmac-platform: fix an OF node reference leak

[Simon Horman <horms@kernel.org>]

On Fri, Dec 13, 2024 at 01:04:42PM +0100, Andrew Lunn wrote:
> > Hi Joe,
> > 
> > I agree this is a problem and that it was introduced by the
> > cited commit. But I wonder if we can consider a different approach.
> > 
> > I would suggest that rather than using __free the node is explicitly
> > released. Something like this (untested):
> > 
> > 	struct device_node *phy_node;
> > 
> > 	...
> > 
> > 	phy_node = of_parse_phandle(np, "phy-handle", 0);
> > 	if (phy_node) {
> > 		of_node_put(phy_node);
> > 		bgmac->phy_connect = platform_phy_connect;
> > 	} ...
> > 
> > That is, assuming that it is safe to release phy_node so early.
> > If not, some adjustment should be made to when of_node_put()
> > is called.
> > 
> > This is for several reasons;
> > 
> > 1. I could be wrong, but I believe your patch kfree's phy_node,
> >    but my understanding is that correct operation is to call
> >    of_node_put().
> 
> Hi Simon
> 
> I _think_ that is wrong. More of the magic which i don't really
> like. The cleanup subsystem has to be taught all the types, and what
> operation to perform for each type. Despite the name __free(), i think
> it does actually call of_node_put(). The magic would be more readable
> if it was actually __put(), not __free().

Thanks, TIL.

> > 2. More importantly, there is a preference in Newkorking code
> >    not to use __free and similar constructs.
> > 
> >      "Low level cleanup constructs (such as __free()) can be used when
> >       building APIs and helpers, especially scoped iterators. However,
> >       direct use of __free() within networking core and drivers is
> >       discouraged. Similar guidance applies to declaring variables
> >       mid-function.
> 
> And this is a good example of why.
> 
> 	Andrew
>