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 <[email protected]>
---
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);
}
--
2.34.1
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 <[email protected]>
> ---
> 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: [email protected]
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
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki