linux-kernel - Re: [PATCH] can: ctucanfd: Remove a useless netif_napi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <202305180932.38815.pisa@cmp.felk.cvut.cz>
Date:   Thu, 18 May 2023 09:32:38 +0200
From:   Pavel Pisa <pisa@....felk.cvut.cz>
To:     Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc:     Ondrej Ille <ondrej.ille@...il.com>,
        Wolfgang Grandegger <wg@...ndegger.com>,
        "Marc Kleine-Budde" <mkl@...gutronix.de>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, linux-kernel@...r.kernel.org,
        kernel-janitors@...r.kernel.org, linux-can@...r.kernel.org,
        netdev@...r.kernel.org
Subject: Re: [PATCH] can: ctucanfd: Remove a useless netif_napi_del() call

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@...adoo.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@....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