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
