* [GIT PULL] NTB bug fixes for v5.11
@ 2020-12-27 14:16 Jon Mason
2020-12-27 17:27 ` pr-tracker-bot
2020-12-27 17:38 ` Linus Torvalds
0 siblings, 2 replies; 6+ messages in thread
From: Jon Mason @ 2020-12-27 14:16 UTC (permalink / raw)
To: torvalds; +Cc: linux-kernel, linux-ntb
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(-)
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [GIT PULL] NTB bug fixes for v5.11 2020-12-27 14:16 [GIT PULL] NTB bug fixes for v5.11 Jon Mason @ 2020-12-27 17:27 ` pr-tracker-bot 2020-12-27 17:38 ` Linus Torvalds 1 sibling, 0 replies; 6+ messages in thread From: pr-tracker-bot @ 2020-12-27 17:27 UTC (permalink / raw) To: Jon Mason; +Cc: torvalds, linux-kernel, linux-ntb 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [GIT PULL] NTB bug fixes for v5.11 2020-12-27 14:16 [GIT PULL] NTB bug fixes for v5.11 Jon Mason 2020-12-27 17:27 ` pr-tracker-bot @ 2020-12-27 17:38 ` Linus Torvalds 2020-12-27 17:42 ` Linus Torvalds 2021-01-04 8:29 ` Dan Carpenter 1 sibling, 2 replies; 6+ messages in thread From: Linus Torvalds @ 2020-12-27 17:38 UTC (permalink / raw) To: Jon Mason, Dan Carpenter; +Cc: Linux Kernel Mailing List, linux-ntb On Sun, Dec 27, 2020 at 6:16 AM Jon Mason <jdmason@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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [GIT PULL] NTB bug fixes for v5.11 2020-12-27 17:38 ` Linus Torvalds @ 2020-12-27 17:42 ` Linus Torvalds 2021-01-04 8:29 ` Dan Carpenter 1 sibling, 0 replies; 6+ messages in thread From: Linus Torvalds @ 2020-12-27 17:42 UTC (permalink / raw) To: Jon Mason, Dan Carpenter; +Cc: Linux Kernel Mailing List, linux-ntb On Sun, Dec 27, 2020 at 9:38 AM Linus Torvalds <torvalds@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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [GIT PULL] NTB bug fixes for v5.11 2020-12-27 17:38 ` Linus Torvalds 2020-12-27 17:42 ` Linus Torvalds @ 2021-01-04 8:29 ` Dan Carpenter 2021-01-04 15:41 ` Jon Mason 1 sibling, 1 reply; 6+ messages in thread From: Dan Carpenter @ 2021-01-04 8:29 UTC (permalink / raw) To: Linus Torvalds; +Cc: Jon Mason, Linux Kernel Mailing List, linux-ntb 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@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. > 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [GIT PULL] NTB bug fixes for v5.11 2021-01-04 8:29 ` Dan Carpenter @ 2021-01-04 15:41 ` Jon Mason 0 siblings, 0 replies; 6+ messages in thread From: Jon Mason @ 2021-01-04 15:41 UTC (permalink / raw) To: Dan Carpenter; +Cc: Linus Torvalds, Linux Kernel Mailing List, linux-ntb On Mon, Jan 4, 2021 at 3:31 AM Dan Carpenter <dan.carpenter@oracle.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@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. > > > > 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@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/linux-ntb/20210104082948.GR2831%40kadam. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-01-04 15:41 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-27 14:16 [GIT PULL] NTB bug fixes for v5.11 Jon Mason 2020-12-27 17:27 ` pr-tracker-bot 2020-12-27 17:38 ` Linus Torvalds 2020-12-27 17:42 ` Linus Torvalds 2021-01-04 8:29 ` Dan Carpenter 2021-01-04 15:41 ` Jon Mason
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).