Hi Shimoda-san,
On Wed, Feb 27, 2019 at 9:53 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@xxxxxxxxxxx> wrote:
> > 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;
> }
I think that is OK.
You could mark rcar_gen2_channel.o_node const to indicate this, but I
don't think that matters much.
To make it really safe for future extension, you could call of_node_get().
However, then you have to make sure of_node_put() is always called later,
in driver remove and in all probe error paths, complicating the code.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
