Re: [PATCH] can: ctucanfd: Remove a useless netif_napi_del() call

[Marc Kleine-Budde]

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   |

Attachment: signature.asc
Description: PGP signature