Re: [PATCH] net: ti: icssg-prueth: Fix device_node refcount leak in prueth_probe()

[Simon Horman <horms@kernel.org>]

On Mon, Nov 09, 2026 at 04:52:35PM +0000, Simon Horman wrote:
> On Fri, Oct 30, 2026 at 04:56:51PM +0800, laixiangwu wrote:
> > prueth_probe() takes references on the eth0_node/eth1_node device nodes
> > with of_node_get() and stores them in prueth->eth_node[]. If either of
> > the "ti,mii-g-rt" or "ti,mii-rt" syscon regmap lookups fails, probe()
> > returns -ENODEV without dropping those references, leaking the device
> > nodes.
> > 
> > Jump to the existing put_cores label so the error path releases the
> > eth0_node/eth1_node references like all the other failure paths in the
> > function do.
> > 
> > Fixes: 128d5874c082 ("net: ti: icssg-prueth: Add ICSSG ethernet driver")
> > Signed-off-by: laixiangwu <laixiangwu@cmss.chinamobile.com>
> > ---
> >  drivers/net/ethernet/ti/icssg/icssg_prueth.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> > index 591be5c80..9b971d1df 100644
> > --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> > +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> > @@ -2258,13 +2258,15 @@ static int prueth_probe(struct platform_device *pdev)
> >  	prueth->miig_rt = syscon_regmap_lookup_by_phandle(np, "ti,mii-g-rt");
> >  	if (IS_ERR(prueth->miig_rt)) {
> >  		dev_err(dev, "couldn't get ti,mii-g-rt syscon regmap\n");
> > -		return -ENODEV;
> > +		ret = -ENODEV;
> > +		goto put_cores;
> >  	}
> >  
> >  	prueth->mii_rt = syscon_regmap_lookup_by_phandle(np, "ti,mii-rt");
> >  	if (IS_ERR(prueth->mii_rt)) {
> >  		dev_err(dev, "couldn't get ti,mii-rt syscon regmap\n");
> > -		return -ENODEV;
> > +		ret = -ENODEV;
> > +		goto put_cores;
> >  	}
> >  
> >  	prueth->pa_stats = syscon_regmap_lookup_by_phandle(np, "ti,pa-stats");
> 
> Hi Laixianwu,
> 
> Thanks, this patch looks correct to me.
> And I appreciate your feedback to the AI bots.
> 
> But I wonder if this nearby check also leaks a reference,
> because while it drops one, wouldn't there be two if the
> condition is met?
> 
>         if (eth0_node == eth1_node) {
>                 dev_err(dev, "port0 and port1 can't have same reg\n");
>                 of_node_put(eth0_node);
>                 return -ENODEV;
>         }

Sorry, one more thing. Which is not strictly related.  But as we're on the
topic of error paths in this function, I am not convinced of the validity
of the value of reg being used even when the call to of_property_read_u32
fails in this loop:

        for_each_child_of_node(eth_ports_node, eth_node) {
                u32 reg;

                if (strcmp(eth_node->name, "port"))
                        continue;
                ret = of_property_read_u32(eth_node, "reg", &reg);
                if (ret < 0) {
                        dev_err(dev, "%pOF error reading port_id %d\n",         
                                eth_node, ret);
                }

                of_node_get(eth_node);

                if (reg == 0) {
                        eth0_node = eth_node;
                        if (!of_device_is_available(eth0_node)) {
                                of_node_put(eth0_node);
                                eth0_node = NULL;
                        }
                } else if (reg == 1) {
                        eth1_node = eth_node;
                        if (!of_device_is_available(eth1_node)) {
                                of_node_put(eth1_node);
                                eth1_node = NULL;
                        }
                } else {
                        dev_err(dev, "port reg should be 0 or 1\n");
                }
        }

> 
> Also, for future reference, it would probably be better to put
> two similar fixes for related drivers in a two-patch patch-set,
> rather than posting them separately.