RE: [PATCH] phy: renesas: rcar-gen2: Fix memory leak at error paths

[Yoshihiro Shimoda]

Hello,

> From: Julia Lawall, Sent: Wednesday, February 27, 2019 5:25 PM
> 
> On Wed, 27 Feb 2019, Yoshihiro Shimoda wrote:
> 
> > This patch fixes memory leak at error paths of the probe function.
> > In for_each_child_of_node, if the loop returns, the driver should
> > call of_put_node() before returns.
> >
> > Reported-by: Julia Lawall <julia.lawall@xxxxxxx>
> > Fixes: 1233f59f745 ("phy: Renesas R-Car Gen2 PHY driver")
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
> > ---
> >  drivers/phy/renesas/phy-rcar-gen2.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/phy/renesas/phy-rcar-gen2.c b/drivers/phy/renesas/phy-rcar-gen2.c
> > index 72eeb06..570b4e4 100644
> > --- a/drivers/phy/renesas/phy-rcar-gen2.c
> > +++ b/drivers/phy/renesas/phy-rcar-gen2.c
> > @@ -285,6 +285,7 @@ static int rcar_gen2_phy_probe(struct platform_device *pdev)
> >  		error = of_property_read_u32(np, "reg", &channel_num);
> >  		if (error || channel_num > 2) {
> >  			dev_err(dev, "Invalid \"reg\" property\n");
> > +			of_node_put(np);
> >  			return error;
> >  		}
> >  		channel->select_mask = select_mask[channel_num];
> > @@ -300,6 +301,7 @@ static int rcar_gen2_phy_probe(struct platform_device *pdev)
> >  						   &rcar_gen2_phy_ops);
> >  			if (IS_ERR(phy->phy)) {
> >  				dev_err(dev, "Failed to create PHY\n");
> > +				of_node_put(np);
> >  				return PTR_ERR(phy->phy);
> >  			}
> >  			phy_set_drvdata(phy->phy, phy);
> 
> Hello,
> 
> I was concerned about the assignment channel->of_node = np;.  Because
> channels is allocated with a devm function, it will get freed on an error
> return, so this pointer doesn't matter.  But don't you need an of_node_get
> on this assignment?  Does the fact that you haven't seen a problem with
> this in testing mean that the field is actually never accessed?

The channel->of_node will be used in the rcar_gen2_phy_xlate() as drv->channels[i].of_node.
The assignment is not used for any device tree APIs, just comparing the pointer.
So, I don't think this driver needs an of_node_get() on this assignment.
Is my understanding incorrect?

---
static struct phy *rcar_gen2_phy_xlate(struct device *dev,
				       struct of_phandle_args *args)
{
	struct rcar_gen2_phy_driver *drv;
	struct device_node *np = args->np;
	int i;

	drv = dev_get_drvdata(dev);
	if (!drv)
		return ERR_PTR(-EINVAL);

	for (i = 0; i < drv->num_channels; i++) {
		if (np == drv->channels[i].of_node)	// <--- here only
			break;
	}

	if (i >= drv->num_channels || args->args[0] >= 2)
		return ERR_PTR(-ENODEV);

	return drv->channels[i].phys[args->args[0]].phy;
}
---

Best regards,
Yoshihiro Shimoda

> julia