* [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:42 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).