| Message ID | 20220126202213.31975-1-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [5.10.y-cip] can: rcar_canfd: rcar_canfd_channel_probe(): make sure we free CAN network device | expand |
Hi, > -----Original Message----- > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > Sent: Thursday, January 27, 2022 5:22 AM > To: cip-dev@lists.cip-project.org; iwamatsu nobuhiro(岩松 信洋 □ＳＷＣ◯Ａ > ＣＴ) <nobuhiro1.iwamatsu@toshiba.co.jp>; Pavel Machek > <pavel@denx.de> > Cc: Biju Das <biju.das.jz@bp.renesas.com> > Subject: [PATCH 5.10.y-cip] can: rcar_canfd: rcar_canfd_channel_probe(): > make sure we free CAN network device > > commit 72b1e360572f9fa7d08ee554f1da29abce23f288 upstream. > > Make sure we free CAN network device in the error path. There are several > jumps to fail label after allocating the CAN network device successfully. This > patch places the free_candev() under fail label so that in failure path a jump to > fail label frees the CAN network device. > > Fixes: 76e9353a80e9 ("can: rcar_canfd: Add support for RZ/G2L family") > Link: > https://lore.kernel.org/all/20220106114801.20563-1-prabhakar.mahadev-lad.r > j@bp.renesas.com > Reported-by: Pavel Machek <pavel@denx.de> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > Reviewed-by: Kieran Bingham > <kieran.bingham+renesas@ideasonboard.com> > Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > drivers/net/can/rcar/rcar_canfd.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) LGTM. If there is no other opinion, I can apply this. Test: https://gitlab.com/cip-project/cip-kernel/linux-cip/-/pipelines/457123034 Best regards, Nobuhiro > > diff --git a/drivers/net/can/rcar/rcar_canfd.c > b/drivers/net/can/rcar/rcar_canfd.c > index 52629c6c1970..6c8e7a51b04d 100644 > --- a/drivers/net/can/rcar/rcar_canfd.c > +++ b/drivers/net/can/rcar/rcar_canfd.c > @@ -1640,8 +1640,7 @@ static int rcar_canfd_channel_probe(struct > rcar_canfd_global *gpriv, u32 ch, > ndev = alloc_candev(sizeof(*priv), RCANFD_FIFO_DEPTH); > if (!ndev) { > dev_err(&pdev->dev, "alloc_candev() failed\n"); > - err = -ENOMEM; > - goto fail; > + return -ENOMEM; > } > priv = netdev_priv(ndev); > > @@ -1735,8 +1734,8 @@ static int rcar_canfd_channel_probe(struct > rcar_canfd_global *gpriv, u32 ch, > > fail_candev: > netif_napi_del(&priv->napi); > - free_candev(ndev); > fail: > + free_candev(ndev); > return err; > } > > -- > 2.17.1
Hi! > > make sure we free CAN network device > > > > commit 72b1e360572f9fa7d08ee554f1da29abce23f288 upstream. > > > > Make sure we free CAN network device in the error path. There are several > > jumps to fail label after allocating the CAN network device successfully. This > > patch places the free_candev() under fail label so that in failure path a jump to > > fail label frees the CAN network device. > > > > Fixes: 76e9353a80e9 ("can: rcar_canfd: Add support for RZ/G2L family") > > Link: > > https://lore.kernel.org/all/20220106114801.20563-1-prabhakar.mahadev-lad.r > > j@bp.renesas.com > > Reported-by: Pavel Machek <pavel@denx.de> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Reviewed-by: Kieran Bingham > > <kieran.bingham+renesas@ideasonboard.com> > > Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > drivers/net/can/rcar/rcar_canfd.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > LGTM. > If there is no other opinion, I can apply this. > Test: > > https://gitlab.com/cip-project/cip-kernel/linux-cip/-/pipelines/457123034 Looks good to me, go ahead :-). Best regards, Pavel
Hi! > commit 72b1e360572f9fa7d08ee554f1da29abce23f288 upstream. > > Make sure we free CAN network device in the error path. There are > several jumps to fail label after allocating the CAN network device > successfully. This patch places the free_candev() under fail label so > that in failure path a jump to fail label frees the CAN network > device. Are they? I see fail label being unused in our 5.10 tree (but mainline uses it and I don't think we need it removed). But more importantly... staring at the code some more: err = register_candev(ndev); if (err) { dev_err(&pdev->dev, "register_candev() failed, error %d\n", err); goto fail_candev; } spin_lock_init(&priv->tx_lock); devm_can_led_init(ndev); gpriv->ch[priv->channel] = priv; dev_info(&pdev->dev, "device registered (channel %u)\n", priv->channel)\ ; return 0; Device is registered before being fully ready, and I don't see anything preventing the device from being used. Should register_candev be done last? (And sorry for not noticing that earlier). Best regards, Pavel
Hi Pavel, Thank you for the review. > -----Original Message----- > From: Pavel Machek <pavel@denx.de> > Sent: 26 January 2022 22:13 > To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com> > Cc: cip-dev@lists.cip-project.org; Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>; Pavel Machek > <pavel@denx.de>; Biju Das <biju.das.jz@bp.renesas.com> > Subject: Re: [PATCH 5.10.y-cip] can: rcar_canfd: rcar_canfd_channel_probe(): make sure we free CAN > network device > > Hi! > > > commit 72b1e360572f9fa7d08ee554f1da29abce23f288 upstream. > > > > Make sure we free CAN network device in the error path. There are > > several jumps to fail label after allocating the CAN network device > > successfully. This patch places the free_candev() under fail label so > > that in failure path a jump to fail label frees the CAN network > > device. > > Are they? I see fail label being unused in our 5.10 tree (but mainline uses it and I don't think we > need it removed). > It is being used [0]. > But more importantly... staring at the code some more: > > err = register_candev(ndev); > if (err) { > dev_err(&pdev->dev, > "register_candev() failed, error %d\n", err); > goto fail_candev; > } > spin_lock_init(&priv->tx_lock); > devm_can_led_init(ndev); > gpriv->ch[priv->channel] = priv; > dev_info(&pdev->dev, "device registered (channel %u)\n", priv->channel)\ ; > return 0; > > Device is registered before being fully ready, and I don't see anything preventing the device from > being used. Should register_candev be done last? > Good catch, do agree register_candev() has to be done last. > (And sorry for not noticing that earlier). > No worries. [0] https://git.kernel.org/pub/scm/linux/kernel/git/cip/linux-cip.git/tree/drivers/net/can/rcar/rcar_canfd.c?h=linux-5.10.y-cip#n1664 Cheers, Prabhakar > Best regards, > Pavel > -- > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Hi, > -----Original Message----- > From: Pavel Machek <pavel@denx.de> > Sent: Thursday, January 27, 2022 7:05 AM > To: iwamatsu nobuhiro(岩松 信洋 □ＳＷＣ◯ＡＣＴ) > <nobuhiro1.iwamatsu@toshiba.co.jp> > Cc: prabhakar.mahadev-lad.rj@bp.renesas.com; cip-dev@lists.cip-project.org; > pavel@denx.de; biju.das.jz@bp.renesas.com > Subject: Re: [PATCH 5.10.y-cip] can: rcar_canfd: rcar_canfd_channel_probe(): > make sure we free CAN network device > > Hi! > > > > make sure we free CAN network device > > > > > > commit 72b1e360572f9fa7d08ee554f1da29abce23f288 upstream. > > > > > > Make sure we free CAN network device in the error path. There are > > > several jumps to fail label after allocating the CAN network device > > > successfully. This patch places the free_candev() under fail label > > > so that in failure path a jump to fail label frees the CAN network device. > > > > > > Fixes: 76e9353a80e9 ("can: rcar_canfd: Add support for RZ/G2L > > > family") > > > Link: > > > https://lore.kernel.org/all/20220106114801.20563-1-prabhakar.mahadev > > > -lad.r > > > j@bp.renesas.com > > > Reported-by: Pavel Machek <pavel@denx.de> > > > Signed-off-by: Lad Prabhakar > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > Reviewed-by: Kieran Bingham > > > <kieran.bingham+renesas@ideasonboard.com> > > > Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> > > > Signed-off-by: Lad Prabhakar > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > --- > > > drivers/net/can/rcar/rcar_canfd.c | 5 ++--- > > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > LGTM. > > If there is no other opinion, I can apply this. > > Test: > > > https://gitlab.com/cip-project/cip-kernel/linux-cip/-/pipelines/4571 > > > 23034 > > Looks good to me, go ahead :-). Thanks, I pushed to git.kernel.org. > > Best regards, > Pavel Best regards, Nobuhiro
Hi Nobuhiro, > -----Original Message----- > From: nobuhiro1.iwamatsu@toshiba.co.jp <nobuhiro1.iwamatsu@toshiba.co.jp> > Sent: 27 January 2022 09:40 > To: pavel@denx.de > Cc: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>; cip-dev@lists.cip-project.org; > Biju Das <biju.das.jz@bp.renesas.com> > Subject: RE: [PATCH 5.10.y-cip] can: rcar_canfd: rcar_canfd_channel_probe(): make sure we free CAN > network device > > Hi, > > > -----Original Message----- > > From: Pavel Machek <pavel@denx.de> > > Sent: Thursday, January 27, 2022 7:05 AM > > To: iwamatsu nobuhiro(岩松 信洋 □ＳＷＣ◯ＡＣＴ) > > <nobuhiro1.iwamatsu@toshiba.co.jp> > > Cc: prabhakar.mahadev-lad.rj@bp.renesas.com; > > cip-dev@lists.cip-project.org; pavel@denx.de; > > biju.das.jz@bp.renesas.com > > Subject: Re: [PATCH 5.10.y-cip] can: rcar_canfd: rcar_canfd_channel_probe(): > > make sure we free CAN network device > > > > Hi! > > > > > > make sure we free CAN network device > > > > > > > > commit 72b1e360572f9fa7d08ee554f1da29abce23f288 upstream. > > > > > > > > Make sure we free CAN network device in the error path. There are > > > > several jumps to fail label after allocating the CAN network > > > > device successfully. This patch places the free_candev() under > > > > fail label so that in failure path a jump to fail label frees the CAN network device. > > > > > > > > Fixes: 76e9353a80e9 ("can: rcar_canfd: Add support for RZ/G2L > > > > family") > > > > Link: > > > > https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2F > > > > lore.kernel.org%2Fall%2F20220106114801.20563-1-prabhakar.mahadev&a > > > > mp;data=04%7C01%7Cprabhakar.mahadev-lad.rj%40bp.renesas.com%7C2a6a > > > > f30f6f9344d87f0508d9e178f087%7C53d82571da1947e49cb4625a166a4a2a%7C > > > > 0%7C0%7C637788731814018904%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA > > > > wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sda > > > > ta=irBP96Ho%2F7f4miUJe5UxR1%2BfbWJmTI8XsLXzT9DLid4%3D&reserved > > > > =0 > > > > -lad.r > > > > j@bp.renesas.com > > > > Reported-by: Pavel Machek <pavel@denx.de> > > > > Signed-off-by: Lad Prabhakar > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > Reviewed-by: Kieran Bingham > > > > <kieran.bingham+renesas@ideasonboard.com> > > > > Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> > > > > Signed-off-by: Lad Prabhakar > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > --- > > > > drivers/net/can/rcar/rcar_canfd.c | 5 ++--- > > > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > > > LGTM. > > > If there is no other opinion, I can apply this. > > > Test: > > > > https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2F > > > > gitlab.com%2Fcip-project%2Fcip-kernel%2Flinux-cip%2F-%2Fpipelines% > > > > 2F4571&data=04%7C01%7Cprabhakar.mahadev-lad.rj%40bp.renesas.co > > > > m%7C2a6af30f6f9344d87f0508d9e178f087%7C53d82571da1947e49cb4625a166 > > > > a4a2a%7C0%7C0%7C637788731814018904%7CUnknown%7CTWFpbGZsb3d8eyJWIjo > > > > iMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000 > > > > &sdata=Qpo7dEOIdMguVEP%2Bu6Hxcd5SKJrdD%2BVSjBtsc2XVd6Y%3D& > > > > reserved=0 > > > > 23034 > > > > Looks good to me, go ahead :-). > > Thanks, I pushed to git.kernel.org. > Thank you for the review and acceptance. Cheers, Prabhakar
Hi! > > Are they? I see fail label being unused in our 5.10 tree (but mainline uses it and I don't think we > > need it removed). > > > It is being used [0]. Yes, sorry, I was looking at wrong tree. Best regards, Pavel
diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c index 52629c6c1970..6c8e7a51b04d 100644 --- a/drivers/net/can/rcar/rcar_canfd.c +++ b/drivers/net/can/rcar/rcar_canfd.c @@ -1640,8 +1640,7 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch, ndev = alloc_candev(sizeof(*priv), RCANFD_FIFO_DEPTH); if (!ndev) { dev_err(&pdev->dev, "alloc_candev() failed\n"); - err = -ENOMEM; - goto fail; + return -ENOMEM; } priv = netdev_priv(ndev); @@ -1735,8 +1734,8 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch, fail_candev: netif_napi_del(&priv->napi); - free_candev(ndev); fail: + free_candev(ndev); return err; }
commit 72b1e360572f9fa7d08ee554f1da29abce23f288 upstream. Make sure we free CAN network device in the error path. There are several jumps to fail label after allocating the CAN network device successfully. This patch places the free_candev() under fail label so that in failure path a jump to fail label frees the CAN network device. Fixes: 76e9353a80e9 ("can: rcar_canfd: Add support for RZ/G2L family") Link: https://lore.kernel.org/all/20220106114801.20563-1-prabhakar.mahadev-lad.rj@bp.renesas.com Reported-by: Pavel Machek <pavel@denx.de> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> --- drivers/net/can/rcar/rcar_canfd.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)