Message-ID: <CAPoiz9zqKsOD8ywxgEjy5t9FB0Mv5uWgfJJCaqCyKxdb5E4wLw@mail.gmail.com>
Date:   Mon, 4 Jan 2021 10:41:32 -0500
From:   Jon Mason <jdmason@...zu.us>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-ntb <linux-ntb@...glegroups.com>
Subject: Re: [GIT PULL] NTB bug fixes for v5.11

On Mon, Jan 4, 2021 at 3:31 AM Dan Carpenter <dan.carpenter@...cle.com> wrote:
>
> On Sun, Dec 27, 2020 at 09:38:23AM -0800, Linus Torvalds wrote:
> > On Sun, Dec 27, 2020 at 6:16 AM Jon Mason <jdmason@...zu.us> wrote:
> > >
> > > Wang Qing (1):
> > >       ntb: idt: fix error check in ntb_hw_idt.c
> >
> > So this patch seems to be at least partially triggered by a smatch
> > warning that is a bit questionable.
> >
> > This part:
> >
> >      if (IS_ERR_OR_NULL(dbgfs_topdir)) {
> >          dev_info(&ndev->ntb.pdev->dev, "Top DebugFS directory absent");
> > -        return PTR_ERR(dbgfs_topdir);
> > +        return PTR_ERR_OR_ZERO(dbgfs_topdir);
> >      }
> >
> > works, but is very non-optimal and unnecessary.
> >
> > The thing is, "PTR_ERR()" works just fine on a IS_ERR_OR_NULL pointer.
> > It doesn't work on a _regular_ non-NULL and non-ERR pointer, and will
> > return random garbage for those. But if you've tested for
> > IS_ERR_OR_NULL(), then a regular PTR_ERR() is already fine.
> >
> > And PTR_ERR_OR_ZERO() potentially generates an extraneous pointless
> > tests against zero (to check for the ERR case).
> >
> > A compiler may be able to notice that the PTR_ERR_OR_ZERO() is
> > unnecessary and remove it (because of the IS_ERR_OR_NULL() checks),
> > but in general we should assume compilers are "not stupid" rather than
> > "really smart".
> >
> > So while this patch isn't _wrong_, and I've already pulled it, the
> > fact that apparently some smatch test triggers these pointless and
> > potentially expensive patches is not a good idea.
> >
> > I'm not sure what the smatch tests should be (NULL turns to 0, which
> > may be confusing), but I'm cc'ing Dan in case he has ideas.
> >
>
> The most common bug that this check finds is the other part of that same
> commit 91b8246de859 ("ntb: idt: fix error check in ntb_hw_idt.c"):
>
>         /* Allocate the memory for IDT NTB device data */
>         ndev = idt_create_dev(pdev, id);
> -       if (IS_ERR_OR_NULL(ndev))
> +       if (IS_ERR(ndev))
>                 return PTR_ERR(ndev);
>
> idt_create_dev() never returns NULL, but if it did then we don't want
> to return success.
>
> For the debugfs stuff, the caller doesn't check the return value anyway.
> Just make it a void function.  A lot of this debugfs code could be
> simplified.  It's not a bug to pass an error pointer or a NULL dbgfs_topdir
> pointer to debugfs_create_file().  There isn't any benefit in checking
> debugfs_initialized().
>
> diff --git a/drivers/ntb/hw/idt/ntb_hw_idt.c b/drivers/ntb/hw/idt/ntb_hw_idt.c
> index e7a4c2aa8baa..710c17b2a923 100644
> --- a/drivers/ntb/hw/idt/ntb_hw_idt.c
> +++ b/drivers/ntb/hw/idt/ntb_hw_idt.c
> @@ -2504,28 +2504,14 @@ static ssize_t idt_dbgfs_info_read(struct file *filp, char __user *ubuf,
>   *
>   * Return: zero on success, otherwise a negative error number.
>   */
> -static int idt_init_dbgfs(struct idt_ntb_dev *ndev)
> +static void idt_init_dbgfs(struct idt_ntb_dev *ndev)
>  {
>         char devname[64];
>
> -       /* If the top directory is not created then do nothing */
> -       if (IS_ERR_OR_NULL(dbgfs_topdir)) {
> -               dev_info(&ndev->ntb.pdev->dev, "Top DebugFS directory absent");
> -               return PTR_ERR_OR_ZERO(dbgfs_topdir);
> -       }
> -
>         /* Create the info file node */
>         snprintf(devname, 64, "info:%s", pci_name(ndev->ntb.pdev));
>         ndev->dbgfs_info = debugfs_create_file(devname, 0400, dbgfs_topdir,
> -               ndev, &idt_dbgfs_info_ops);
> -       if (IS_ERR(ndev->dbgfs_info)) {
> -               dev_dbg(&ndev->ntb.pdev->dev, "Failed to create DebugFS node");
> -               return PTR_ERR(ndev->dbgfs_info);
> -       }
> -
> -       dev_dbg(&ndev->ntb.pdev->dev, "NTB device DebugFS node created");
> -
> -       return 0;
> +                                              ndev, &idt_dbgfs_info_ops);
>  }
>
>  /*
> @@ -2792,7 +2778,7 @@ static int idt_pci_probe(struct pci_dev *pdev,
>                 goto err_deinit_isr;
>
>         /* Initialize DebugFS info node */
> -       (void)idt_init_dbgfs(ndev);
> +       idt_init_dbgfs(ndev);
>
>         /* IDT PCIe-switch NTB driver is finally initialized */
>         dev_info(&pdev->dev, "IDT NTB device is ready");
> @@ -2904,9 +2890,7 @@ static int __init idt_pci_driver_init(void)
>  {
>         pr_info("%s %s\n", NTB_DESC, NTB_VER);
>
> -       /* Create the top DebugFS directory if the FS is initialized */
> -       if (debugfs_initialized())
> -               dbgfs_topdir = debugfs_create_dir(KBUILD_MODNAME, NULL);
> +       dbgfs_topdir = debugfs_create_dir(KBUILD_MODNAME, NULL);
>
>         /* Register the NTB hardware driver to handle the PCI device */
>         return pci_register_driver(&idt_pci_driver);
> --
> 2.29.2

This seems logical and the patch looks fine to me.  If you send it as
a patch, I'll happily pull it in.

Thanks,
Jon

>
> --
> You received this message because you are subscribed to the Google Groups "linux-ntb" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-ntb+unsubscribe@...glegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/linux-ntb/20210104082948.GR2831%40kadam.

Powered by blists - more mailing lists