| Message ID | 58500052a6740806e8af199ece45e97cb5eeb1b8.1684393811.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | can: ctucanfd: Remove a useless netif_napi_del() call | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Series ignored based on subject |
Dear Christophe, On Thursday 18 of May 2023 09:10:39 Christophe JAILLET wrote: > free_candev() already calls netif_napi_del(), so there is no need to call > it explicitly. It is harmless, but useless. > > This makes the code mode consistent with the error handling path of > ctucan_probe_common(). OK, but I would suggest to consider to keep sequence in sync with linux/drivers/net/can/ctucanfd/ctucanfd_pci.c where is netif_napi_del() used as well while ((priv = list_first_entry_or_null(&bdata->ndev_list_head, struct ctucan_priv, peers_on_pdev)) != NULL) { ndev = priv->can.dev; unregister_candev(ndev); netif_napi_del(&priv->napi); list_del_init(&priv->peers_on_pdev); free_candev(ndev); } On the other hand, if interrupt can be called for device between unregister_candev() and free_candev() or some other callback which is prevented by netif_napi_del() now then I would consider to keep explicit netif_napi_del() to ensure that no callback is activated to driver there. And for PCI integration it is more critical because list_del_init(&priv->peers_on_pdev); appears in between and I would prefer that no interrupt appears when instance is not on the peers list anymore. Even that would not be a problem for actual CTU CAN FD implementation, peers are accessed only during physical device remove, but I have worked on other controllers in past, which required to coordinate with peers in interrupt handling... So I would be happy for some feedback what is actual guarantee when device is stopped. May it be that it would be even more robust to run removal with two loop where the first one calls unregister_candev() and netif_napi_del() and only the second one removes from peers and call free_candev()... But I am not sure there and it is not problem in actual driver because peers are not used in any other place... > While at it, remove a wrong comment about the return value of this > function. OK, this has been caused probably by prototype change. > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > The comment went wrong after 45413bf75919 ("can: ctucanfd: Convert to > platform remove callback returning void") --- > drivers/net/can/ctucanfd/ctucanfd_platform.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/net/can/ctucanfd/ctucanfd_platform.c > b/drivers/net/can/ctucanfd/ctucanfd_platform.c index > 55bb10b157b4..8fe224b8dac0 100644 > --- a/drivers/net/can/ctucanfd/ctucanfd_platform.c > +++ b/drivers/net/can/ctucanfd/ctucanfd_platform.c > @@ -84,7 +84,6 @@ static int ctucan_platform_probe(struct platform_device > *pdev) * @pdev: Handle to the platform device structure > * > * This function frees all the resources allocated to the device. > - * Return: 0 always > */ > static void ctucan_platform_remove(struct platform_device *pdev) > { > @@ -95,7 +94,6 @@ static void ctucan_platform_remove(struct platform_device > *pdev) > > unregister_candev(ndev); > pm_runtime_disable(&pdev->dev); > - netif_napi_del(&priv->napi); > free_candev(ndev); > } Best wishes, Pavel Pisa phone: +420 603531357 e-mail: pisa@cmp.felk.cvut.cz Department of Control Engineering FEE CVUT Karlovo namesti 13, 121 35, Prague 2 university: http://control.fel.cvut.cz/ personal: http://cmp.felk.cvut.cz/~pisa projects: https://www.openhub.net/accounts/ppisa CAN related:http://canbus.pages.fel.cvut.cz/ RISC-V education: https://comparch.edu.cvut.cz/ Open Technologies Research Education and Exchange Services https://gitlab.fel.cvut.cz/otrees/org/-/wikis/home
On 18.05.2023 09:32:38, Pavel Pisa wrote: > Dear Christophe, > > On Thursday 18 of May 2023 09:10:39 Christophe JAILLET wrote: > > free_candev() already calls netif_napi_del(), so there is no need to call > > it explicitly. It is harmless, but useless. > > > > This makes the code mode consistent with the error handling path of > > ctucan_probe_common(). > > OK, but I would suggest to consider to keep sequence in sync with > > linux/drivers/net/can/ctucanfd/ctucanfd_pci.c > > where is netif_napi_del() used as well > > while ((priv = list_first_entry_or_null(&bdata->ndev_list_head, struct ctucan_priv, > peers_on_pdev)) != NULL) { > ndev = priv->can.dev; > > unregister_candev(ndev); > > netif_napi_del(&priv->napi); > > list_del_init(&priv->peers_on_pdev); > free_candev(ndev); > } > > On the other hand, if interrupt can be called for device between > unregister_candev() and free_candev() At least the case of an "interrupt during ctucan_pci_remove()" is a bug, as there is no IRQ handler registered. The IRQ handler is registered in ctucan_open() and freed in ctucan_close(). > or some other callback > which is prevented by netif_napi_del() now then I would consider > to keep explicit netif_napi_del() to ensure that no callback > is activated to driver there. Napi itself is shut down, too, as there is a call to napi_disable() in ctucan_close(). > And for PCI integration it is more > critical because list_del_init(&priv->peers_on_pdev); appears in > between and I would prefer that no interrupt appears when instance > is not on the peers list anymore. Even that would not be a problem > for actual CTU CAN FD implementation, peers are accessed only during > physical device remove, but I have worked on other controllers > in past, which required to coordinate with peers in interrupt > handling... > > So I would be happy for some feedback what is actual guarantee > when device is stopped. After a ifup; ifdown;, which corresponds to ctucan_open(), ctucan_close() in the driver, the device should be shut down, no interrupts active. You might even power down the device, although things get a little more complicated with HW timestamping or even PTP enabled. > May it be that it would be even more robust to run removal > with two loop where the first one calls unregister_candev() > and netif_napi_del() and only the second one removes from peers > and call free_candev()... But I am not sure there and it is not > problem in actual driver because peers are not used in any > other place... regards, Marc
Hi Christophe,
kernel test robot noticed the following build warnings:
[auto build test WARNING on mkl-can-next/testing]
[also build test WARNING on linus/master v6.4-rc2 next-20230518]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Christophe-JAILLET/can-ctucanfd-Remove-a-useless-netif_napi_del-call/20230518-151217
base: https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git testing
patch link: https://lore.kernel.org/r/58500052a6740806e8af199ece45e97cb5eeb1b8.1684393811.git.christophe.jaillet%40wanadoo.fr
patch subject: [PATCH] can: ctucanfd: Remove a useless netif_napi_del() call
config: x86_64-allmodconfig
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/aaed91df00cf16ff7783fa535c29228072d41554
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Christophe-JAILLET/can-ctucanfd-Remove-a-useless-netif_napi_del-call/20230518-151217
git checkout aaed91df00cf16ff7783fa535c29228072d41554
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 olddefconfig
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/net/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202305181949.ywS3ECIq-lkp@intel.com/
All warnings (new ones prefixed by >>):
drivers/net/can/ctucanfd/ctucanfd_platform.c: In function 'ctucan_platform_remove':
>> drivers/net/can/ctucanfd/ctucanfd_platform.c:91:29: warning: unused variable 'priv' [-Wunused-variable]
91 | struct ctucan_priv *priv = netdev_priv(ndev);
| ^~~~
vim +/priv +91 drivers/net/can/ctucanfd/ctucanfd_platform.c
e8f0c23a2415fa Pavel Pisa 2022-03-22 81
e8f0c23a2415fa Pavel Pisa 2022-03-22 82 /**
e8f0c23a2415fa Pavel Pisa 2022-03-22 83 * ctucan_platform_remove - Unregister the device after releasing the resources
e8f0c23a2415fa Pavel Pisa 2022-03-22 84 * @pdev: Handle to the platform device structure
e8f0c23a2415fa Pavel Pisa 2022-03-22 85 *
e8f0c23a2415fa Pavel Pisa 2022-03-22 86 * This function frees all the resources allocated to the device.
e8f0c23a2415fa Pavel Pisa 2022-03-22 87 */
45413bf759193d Uwe Kleine-König 2023-05-12 88 static void ctucan_platform_remove(struct platform_device *pdev)
e8f0c23a2415fa Pavel Pisa 2022-03-22 89 {
e8f0c23a2415fa Pavel Pisa 2022-03-22 90 struct net_device *ndev = platform_get_drvdata(pdev);
e8f0c23a2415fa Pavel Pisa 2022-03-22 @91 struct ctucan_priv *priv = netdev_priv(ndev);
e8f0c23a2415fa Pavel Pisa 2022-03-22 92
e8f0c23a2415fa Pavel Pisa 2022-03-22 93 netdev_dbg(ndev, "ctucan_remove");
e8f0c23a2415fa Pavel Pisa 2022-03-22 94
e8f0c23a2415fa Pavel Pisa 2022-03-22 95 unregister_candev(ndev);
e8f0c23a2415fa Pavel Pisa 2022-03-22 96 pm_runtime_disable(&pdev->dev);
e8f0c23a2415fa Pavel Pisa 2022-03-22 97 free_candev(ndev);
e8f0c23a2415fa Pavel Pisa 2022-03-22 98 }
e8f0c23a2415fa Pavel Pisa 2022-03-22 99
diff --git a/drivers/net/can/ctucanfd/ctucanfd_platform.c b/drivers/net/can/ctucanfd/ctucanfd_platform.c index 55bb10b157b4..8fe224b8dac0 100644 --- a/drivers/net/can/ctucanfd/ctucanfd_platform.c +++ b/drivers/net/can/ctucanfd/ctucanfd_platform.c @@ -84,7 +84,6 @@ static int ctucan_platform_probe(struct platform_device *pdev) * @pdev: Handle to the platform device structure * * This function frees all the resources allocated to the device. - * Return: 0 always */ static void ctucan_platform_remove(struct platform_device *pdev) { @@ -95,7 +94,6 @@ static void ctucan_platform_remove(struct platform_device *pdev) unregister_candev(ndev); pm_runtime_disable(&pdev->dev); - netif_napi_del(&priv->napi); free_candev(ndev); }
free_candev() already calls netif_napi_del(), so there is no need to call it explicitly. It is harmless, but useless. This makes the code mode consistent with the error handling path of ctucan_probe_common(). While at it, remove a wrong comment about the return value of this function. Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- The comment went wrong after 45413bf75919 ("can: ctucanfd: Convert to platform remove callback returning void") --- drivers/net/can/ctucanfd/ctucanfd_platform.c | 2 -- 1 file changed, 2 deletions(-)