Hi Kieran,
Thank you for the review.
On Thu, Jan 6, 2022 at 3:46 PM Kieran Bingham
<kieran.bingham@xxxxxxxxxxxxxxxx> wrote:
>
> Quoting Lad Prabhakar (2022-01-06 11:48:00)
> > 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")
> > Reported-by: Pavel Machek <pavel@xxxxxxx>
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > ---
> > 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 ff9d0f5ae0dd..388521e70837 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;
>
> Aha good - so we don't try to call free_candev() on a null pointer.
> (which doesn't look null-safe, in free_netdev).
>
Yep.
> > }
> > priv = netdev_priv(ndev);
> >
> > @@ -1735,8 +1734,8 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch,
> >
> > fail_candev:
>
> Is this label still appropriately named now that the free_candev is
> moved out of it? I wonder if it should be fail_netif:
>
I was tempted for this change, but wanted to keep the changes minimal.
Maybe I'll do it anyway to improve the readability.
> So aside from potential naming, the !ndev case is safely handled, so it
> looks fine to me.
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
>
> > netif_napi_del(&priv->napi);
> > - free_candev(ndev);
> > fail:
> > + free_candev(ndev);
>
>
>
> > return err;
> > }
> >
> > --
> > 2.17.1
> >
Cheers,
Prabhakar
