Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
To:     Julia Lawall <julia.lawall@lip6.fr>
CC:     "kishon@ti.com" <kishon@ti.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-renesas-soc@vger.kernel.org" 
        <linux-renesas-soc@vger.kernel.org>
Subject: RE: [PATCH] phy: renesas: rcar-gen2: Fix memory leak at error paths
Thread-Topic: [PATCH] phy: renesas: rcar-gen2: Fix memory leak at error paths
Thread-Index: AQHUzmoRtGFFVYG3M06xnOGKQQhNVaXzTx6AgAAGzdA=
Date:   Wed, 27 Feb 2019 08:53:35 +0000
Message-ID: <TY2PR01MB4812FABCB851C22F4295B9D4D8740@TY2PR01MB4812.jpnprd01.prod.outlook.com>
References: <1551250826-10302-1-git-send-email-yoshihiro.shimoda.uh@renesas.com>
 <alpine.DEB.2.21.1902270920300.2671@hadrien>
In-Reply-To: <alpine.DEB.2.21.1902270920300.2671@hadrien>
Accept-Language: ja-JP, en-US
Content-Language: ja-JP
received-spf: None (protection.outlook.com: renesas.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: f9899241-c4f7-4750-38d7-08d69c910f42
X-MS-Exchange-CrossTenant-originalarrivaltime: 27 Feb 2019 08:53:35.7821
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 53d82571-da19-47e4-9cb4-625a166a4a2a
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-Transport-CrossTenantHeadersStamped: TY2PR01MB3275
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hello,

> From: Julia Lawall, Sent: Wednesday, February 27, 2019 5:25 PM
>=20
> On Wed, 27 Feb 2019, Yoshihiro Shimoda wrote:
>=20
> > 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@lip6.fr>
> > Fixes: 1233f59f745 ("phy: Renesas R-Car Gen2 PHY driver")
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> >  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_devi=
ce *pdev)
> >  		error =3D 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 =3D select_mask[channel_num];
> > @@ -300,6 +301,7 @@ static int rcar_gen2_phy_probe(struct platform_devi=
ce *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);
>=20
> Hello,
>=20
> I was concerned about the assignment channel->of_node =3D 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_ge=
t
> 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->chan=
nels[i].of_node.
The assignment is not used for any device tree APIs, just comparing the poi=
nter.
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 =3D args->np;
	int i;

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

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

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

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

Best regards,
Yoshihiro Shimoda

> julia