[GIT PULL] NTB bug fixes for v5.11

[Jon Mason]

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(-)

[pr-trac...@kernel.org]

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

[Linus Torvalds]

On Sun, Dec 27, 2020 at 6:16 AM Jon Mason <jdm...@kudzu.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.

Linus

[Linus Torvalds]

On Sun, Dec 27, 2020 at 9:38 AM Linus Torvalds
<torv...@linux-foundation.org> 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

[Dan Carpenter]

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

[Jon Mason]

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+...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/linux-ntb/20210104082948.GR2831%40kadam.