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
