Messages in this thread | | | From | Linus Torvalds <> | Date | Sun, 27 Dec 2020 09:38:23 -0800 | Subject | Re: [GIT PULL] NTB bug fixes for v5.11 |
| |
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
| |