[PATCH 5.10.y-cip] can: rcar_canfd: rcar_canfd_channel_probe(): make sure we free CAN network device

[PATCH 5.10.y-cip] can: rcar_canfd: rcar_canfd_channel_probe(): make sure we free CAN network device

[Lad Prabhakar]

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(-)

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

RE: [PATCH 5.10.y-cip] can: rcar_canfd: rcar_canfd_channel_probe(): make sure we free CAN network device

[nobuhiro1.iwamatsu]

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

Re: [PATCH 5.10.y-cip] can: rcar_canfd: rcar_canfd_channel_probe(): make sure we free CAN network device

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1434 bytes --]

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
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

RE: [PATCH 5.10.y-cip] can: rcar_canfd: rcar_canfd_channel_probe(): make sure we free CAN network device

[nobuhiro1.iwamatsu]

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

RE: [PATCH 5.10.y-cip] can: rcar_canfd: rcar_canfd_channel_probe(): make sure we free CAN network device

[Prabhakar Mahadev Lad]

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&amp;sda
> > > > ta=irBP96Ho%2F7f4miUJe5UxR1%2BfbWJmTI8XsLXzT9DLid4%3D&amp;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&amp;data=04%7C01%7Cprabhakar.mahadev-lad.rj%40bp.renesas.co
> > > > m%7C2a6af30f6f9344d87f0508d9e178f087%7C53d82571da1947e49cb4625a166
> > > > a4a2a%7C0%7C0%7C637788731814018904%7CUnknown%7CTWFpbGZsb3d8eyJWIjo
> > > > iMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000
> > > > &amp;sdata=Qpo7dEOIdMguVEP%2Bu6Hxcd5SKJrdD%2BVSjBtsc2XVd6Y%3D&amp;
> > > > 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

Re: [PATCH 5.10.y-cip] can: rcar_canfd: rcar_canfd_channel_probe(): make sure we free CAN network device

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1307 bytes --]

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
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

RE: [PATCH 5.10.y-cip] can: rcar_canfd: rcar_canfd_channel_probe(): make sure we free CAN network device

[Prabhakar Mahadev Lad]

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

Re: [PATCH 5.10.y-cip] can: rcar_canfd: rcar_canfd_channel_probe(): make sure we free CAN network device

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 405 bytes --]

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
											
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]