| Message ID | 20211013040349.2858773-1-mudongliangabcd@gmail.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | driver: net: can: delete napi if register_candev fails | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Single patches do not need cover letters |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | success | CCed 10 of 10 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 16 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | No static functions without inline keyword in header files |
On Wed, Oct 13, 2021 at 12:04 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote: > > If register_candev fails, xcan_probe does not clean the napi > created by netif_napi_add. > It seems the netif_napi_del operation is done in the free_candev (free_netdev precisely). list_for_each_entry_safe(p, n, &dev->napi_list, dev_list) netif_napi_del(p); And list_add_rcu(&napi->dev_list, &dev->napi_list) is done in the netif_napi_add. Therefore, I suggest removing "netif_napi_del" operation in the xcan_remove to match probe and remove function. > Fix this by adding error handling code to clean napi when > register_candev fails. > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > --- > drivers/net/can/xilinx_can.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c > index 3b883e607d8b..6ee0b5a8cdfc 100644 > --- a/drivers/net/can/xilinx_can.c > +++ b/drivers/net/can/xilinx_can.c > @@ -1807,7 +1807,7 @@ static int xcan_probe(struct platform_device *pdev) > ret = register_candev(ndev); > if (ret) { > dev_err(&pdev->dev, "fail to register failed (err=%d)\n", ret); > - goto err_disableclks; > + goto err_del_napi; > } > > devm_can_led_init(ndev); > @@ -1825,6 +1825,8 @@ static int xcan_probe(struct platform_device *pdev) > > return 0; > > +err_del_napi: > + netif_napi_del(&priv->napi); > err_disableclks: > pm_runtime_put(priv->dev); > pm_runtime_disable(&pdev->dev); > -- > 2.25.1 >
On 13.10.2021 13:21:09, Dongliang Mu wrote: > On Wed, Oct 13, 2021 at 12:04 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote: > > > > If register_candev fails, xcan_probe does not clean the napi > > created by netif_napi_add. > > > > It seems the netif_napi_del operation is done in the free_candev > (free_netdev precisely). > > list_for_each_entry_safe(p, n, &dev->napi_list, dev_list) > netif_napi_del(p); > > And list_add_rcu(&napi->dev_list, &dev->napi_list) is done in the > netif_napi_add. > > Therefore, I suggest removing "netif_napi_del" operation in the > xcan_remove to match probe and remove function. Sounds reasonable, can you create a patch for this. regards, Marc
On Sun, Oct 17, 2021 at 8:36 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote: > > On 13.10.2021 13:21:09, Dongliang Mu wrote: > > On Wed, Oct 13, 2021 at 12:04 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote: > > > > > > If register_candev fails, xcan_probe does not clean the napi > > > created by netif_napi_add. > > > > > > > It seems the netif_napi_del operation is done in the free_candev > > (free_netdev precisely). > > > > list_for_each_entry_safe(p, n, &dev->napi_list, dev_list) > > netif_napi_del(p); > > > > And list_add_rcu(&napi->dev_list, &dev->napi_list) is done in the > > netif_napi_add. > > > > Therefore, I suggest removing "netif_napi_del" operation in the > > xcan_remove to match probe and remove function. > > Sounds reasonable, can you create a patch for this. I have submitted one patch - https://lkml.org/lkml/2021/10/17/181 > > regards, > Marc > > -- > Pengutronix e.K. | Marc Kleine-Budde | > Embedded Linux | https://www.pengutronix.de | > Vertretung West/Dortmund | Phone: +49-231-2826-924 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On 17.10.2021 20:52:14, Dongliang Mu wrote: > On Sun, Oct 17, 2021 at 8:36 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote: > > > > On 13.10.2021 13:21:09, Dongliang Mu wrote: > > > On Wed, Oct 13, 2021 at 12:04 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote: > > > > > > > > If register_candev fails, xcan_probe does not clean the napi > > > > created by netif_napi_add. > > > > > > > > > > It seems the netif_napi_del operation is done in the free_candev > > > (free_netdev precisely). > > > > > > list_for_each_entry_safe(p, n, &dev->napi_list, dev_list) > > > netif_napi_del(p); > > > > > > And list_add_rcu(&napi->dev_list, &dev->napi_list) is done in the > > > netif_napi_add. > > > > > > Therefore, I suggest removing "netif_napi_del" operation in the > > > xcan_remove to match probe and remove function. > > > > Sounds reasonable, can you create a patch for this. > > I have submitted one patch - https://lkml.org/lkml/2021/10/17/181 Thanks for the patch. Regards, Marc BTW: Do you know the new kernel.org mailing list archive available at https://lore.kernel.org ? You can reference a mail using its Message-ID, in you case it's: https://lore.kernel.org/all/20211017125022.3100329-1-mudongliangabcd@gmail.com
diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c index 3b883e607d8b..6ee0b5a8cdfc 100644 --- a/drivers/net/can/xilinx_can.c +++ b/drivers/net/can/xilinx_can.c @@ -1807,7 +1807,7 @@ static int xcan_probe(struct platform_device *pdev) ret = register_candev(ndev); if (ret) { dev_err(&pdev->dev, "fail to register failed (err=%d)\n", ret); - goto err_disableclks; + goto err_del_napi; } devm_can_led_init(ndev); @@ -1825,6 +1825,8 @@ static int xcan_probe(struct platform_device *pdev) return 0; +err_del_napi: + netif_napi_del(&priv->napi); err_disableclks: pm_runtime_put(priv->dev); pm_runtime_disable(&pdev->dev);
If register_candev fails, xcan_probe does not clean the napi created by netif_napi_add. Fix this by adding error handling code to clean napi when register_candev fails. Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> --- drivers/net/can/xilinx_can.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)