Hello Linus,
Here are a few NTB bug fixes for v5.11. Please consider pulling them.
Thanks,
Jon
The following changes since commit 3650b228f83adda7e5ee532e2b90429c03f7b9ec:
Linux 5.10-rc1 (2020-10-25 15:14:11 -0700)
are available in the Git repository at:
git://github.com/jonmason/ntb tags/ntb-5.11
for you to fetch changes up to 75b6f6487cedd0e4c8e07d68b68b8f85cd352bfe:
ntb: intel: add Intel NTB LTR vendor support for gen4 NTB (2020-12-06 18:18:03 -0500)
----------------------------------------------------------------
Big fix for IDT NTB and Intel NTB LTR management support
----------------------------------------------------------------
Dave Jiang (1):
ntb: intel: add Intel NTB LTR vendor support for gen4 NTB
Wang Qing (1):
ntb: idt: fix error check in ntb_hw_idt.c
drivers/ntb/hw/idt/ntb_hw_idt.c | 4 ++--
drivers/ntb/hw/intel/ntb_hw_gen1.h | 1 +
drivers/ntb/hw/intel/ntb_hw_gen4.c | 27 ++++++++++++++++++++++++++-
drivers/ntb/hw/intel/ntb_hw_gen4.h | 15 +++++++++++++++
4 files changed, 44 insertions(+), 3 deletions(-)
The pull request you sent on Sun, 27 Dec 2020 09:16:38 -0500:
> git://github.com/jonmason/ntb tags/ntb-5.11
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/52cd5f9c22eeef26d05f9d9338ba4eb38f14dd3a
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html
On Sun, Dec 27, 2020 at 6:16 AM Jon Mason <[email protected]> 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.
Linus
On Sun, Dec 27, 2020 at 9:38 AM Linus Torvalds
<[email protected]> wrote:
>
> 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.
Side note: no, standard C does not guarantee that a NULL pointer would
cast to the integer 0 (despite a cast of the constant 0 turning into
NULL), but the kernel very much does. And our ERR_PTR() games in
particular already violate all the standard C rules, and we very much
depend on the pointer bit patterns to begin with.
Linus
On Sun, Dec 27, 2020 at 09:38:23AM -0800, Linus Torvalds wrote:
> On Sun, Dec 27, 2020 at 6:16 AM Jon Mason <[email protected]> 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
On Mon, Jan 4, 2021 at 3:31 AM Dan Carpenter <[email protected]> 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 <[email protected]> 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 [email protected].
> To view this discussion on the web visit https://groups.google.com/d/msgid/linux-ntb/20210104082948.GR2831%40kadam.