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


Lad Prabhakar
 

Hi Pavel,

Thank you for the review.

-----Original Message-----
From: Pavel Machek <pavel@...>
Sent: 26 January 2022 22:13
To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@...>
Cc: cip-dev@...; Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@...>; Pavel Machek
<pavel@...>; Biju Das <biju.das.jz@...>
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

Join cip-dev@lists.cip-project.org to automatically receive all group messages.