| Message ID | 20180920212648.25181-1-natechancellor@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | staging: wilc1000: Remove unnecessary pointer check | expand |
On Thu, 20 Sep 2018 14:26:49 -0700, Nathan Chancellor said: > Clang warns that the address of a pointer will always evaluated as true > in a boolean context: > > drivers/staging/wilc1000/linux_wlan.c:267:20: warning: address of > 'vif->ndev->dev' will always evaluate to 'true' > [-Wpointer-bool-conversion] > if (!(&vif->ndev->dev)) > ~ ~~~~~~~~~~~^~~ > 1 warning generated. > > Since this statement always evaluates to false due to the logical not, > remove it. Often, "just nuke it because it's now dead code" isn't the best answer... At one time, that was likely intended to be checking whether ->dev was a null pointer, to make sure we don't pass request_firmware() a null pointer and oops the kernel, or other things that go pear-shaped.... So the question becomes: Is it safe to just remove it, or was it intended to test for something that could legitimately be null if we've hit an error along the way (which means we should fix the condition to be proper and acceptable to both gcc and clang)?
On Fri, Sep 21, 2018 at 01:25:32AM -0400, valdis.kletnieks@vt.edu wrote: > On Thu, 20 Sep 2018 14:26:49 -0700, Nathan Chancellor said: > > Clang warns that the address of a pointer will always evaluated as true > > in a boolean context: > > > > drivers/staging/wilc1000/linux_wlan.c:267:20: warning: address of > > 'vif->ndev->dev' will always evaluate to 'true' > > [-Wpointer-bool-conversion] > > if (!(&vif->ndev->dev)) > > ~ ~~~~~~~~~~~^~~ > > 1 warning generated. > > > > Since this statement always evaluates to false due to the logical not, > > remove it. > > Often, "just nuke it because it's now dead code" isn't the best answer... > > At one time, that was likely intended to be checking whether ->dev was a null > pointer, to make sure we don't pass request_firmware() a null pointer and oops > the kernel, or other things that go pear-shaped.... > > So the question becomes: Is it safe to just remove it, or was it intended to > test for something that could legitimately be null if we've hit an error along > the way (which means we should fix the condition to be proper and acceptable > to both gcc and clang)? > > I certainly considered whether or not removing the check versus fixing it was the correct answer. Given that this check can be traced back to the initial check in of the driver in 2015, I figured it was safe to remove it (since a null pointer dereference would most likely have been noticed by now). Most patches addressing this warning just remove the check given that it's not actually changing the code, such as commit a7dc662c6a7b ("ASoC: codecs: PCM1789: unconditionally flush work"). However, if the driver authors and/or maintainers think that this check should be something else (maybe checking that the contents of dev is not null versus the address, I'm perfectly happy to submit a v2 with this change. Thank you for the response and review! Nathan
Reviewed-by: Ajay Singh <ajay.kathat@microchip.com> On Thu, 20 Sep 2018 22:39:11 -0700 Nathan Chancellor <natechancellor@gmail.com> wrote: > On Fri, Sep 21, 2018 at 01:25:32AM -0400, valdis.kletnieks@vt.edu > wrote: > > On Thu, 20 Sep 2018 14:26:49 -0700, Nathan Chancellor said: > > > Clang warns that the address of a pointer will always evaluated > > > as true in a boolean context: > > > > > > drivers/staging/wilc1000/linux_wlan.c:267:20: warning: address of > > > 'vif->ndev->dev' will always evaluate to 'true' > > > [-Wpointer-bool-conversion] > > > if (!(&vif->ndev->dev)) > > > ~ ~~~~~~~~~~~^~~ > > > 1 warning generated. > > > > > > Since this statement always evaluates to false due to the logical > > > not, remove it. > > > > Often, "just nuke it because it's now dead code" isn't the best > > answer... > > > > At one time, that was likely intended to be checking whether ->dev > > was a null pointer, to make sure we don't pass request_firmware() a > > null pointer and oops the kernel, or other things that go > > pear-shaped.... > > > > So the question becomes: Is it safe to just remove it, or was it > > intended to test for something that could legitimately be null if > > we've hit an error along the way (which means we should fix the > > condition to be proper and acceptable to both gcc and clang)? > > > > > > I certainly considered whether or not removing the check versus fixing > it was the correct answer. Given that this check can be traced back to > the initial check in of the driver in 2015, I figured it was safe to > remove it (since a null pointer dereference would most likely have > been noticed by now). > > Most patches addressing this warning just remove the check given that > it's not actually changing the code, such as commit a7dc662c6a7b > ("ASoC: codecs: PCM1789: unconditionally flush work"). However, if > the driver authors and/or maintainers think that this check should be > something else (maybe checking that the contents of dev is not null > versus the address, I'm perfectly happy to submit a v2 with this > change. > The 'if' condition was intended to check the validity of net_device structure, but i think its not required here. The device pointer used in request_firmware(), was received in the probe functions and different from the one checked in 'if' condition. Thus its safe to remove the 'if (!(&vif->ndev->dev))' condition block. Regards, Ajay
On Fri, Sep 21, 2018 at 01:25:32AM -0400, valdis.kletnieks@vt.edu wrote: > On Thu, 20 Sep 2018 14:26:49 -0700, Nathan Chancellor said: > > Clang warns that the address of a pointer will always evaluated as true > > in a boolean context: > > > > drivers/staging/wilc1000/linux_wlan.c:267:20: warning: address of > > 'vif->ndev->dev' will always evaluate to 'true' > > [-Wpointer-bool-conversion] > > if (!(&vif->ndev->dev)) > > ~ ~~~~~~~~~~~^~~ > > 1 warning generated. > > > > Since this statement always evaluates to false due to the logical not, > > remove it. > > Often, "just nuke it because it's now dead code" isn't the best answer... > > At one time, that was likely intended to be checking whether ->dev was a null > pointer, to make sure we don't pass request_firmware() a null pointer and oops > the kernel, or other things that go pear-shaped.... > > So the question becomes: Is it safe to just remove it, or was it intended to > test for something that could legitimately be null if we've hit an error along > the way (which means we should fix the condition to be proper and acceptable > to both gcc and clang)? > Obviously, we hope that Nathan considered that. This driver has new competent maintainers so they would think about that too. I also review staging patches and I reviewed it a few minutes after it was sent. So it's not like anyone was going to just merge the patch without thinking about whether a different test was intended. I am on the kernel-janitors and we've had one or two of these recently where the warning indicate a bug so perhaps we do need to think about it from a "process perspective". The Fixes tag isn't appropiate because it's not a bug fix, but we could just say in the comments: "This unused variable was added in commit 123456789012 ("blah blah") so far as I can see it has never been useful." That would help reviewing because now I know that you thought about it and I also can just look at the original commit. For this patch I did git log -p and the scrolled to the original commit, and the function name had changed so I had to scroll back and forth a bit to see what the function was called originally. It wasn't a huge deal but having the original commit would be nice. regards, dan carpenter
diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c index 49afda669393..323593440e40 100644 --- a/drivers/staging/wilc1000/linux_wlan.c +++ b/drivers/staging/wilc1000/linux_wlan.c @@ -264,9 +264,6 @@ static int wilc_wlan_get_firmware(struct net_device *dev) netdev_info(dev, "loading firmware %s\n", firmware); - if (!(&vif->ndev->dev)) - goto fail; - if (request_firmware(&wilc_firmware, firmware, wilc->dev) != 0) { netdev_err(dev, "%s - firmware not available\n", firmware); ret = -1;
Clang warns that the address of a pointer will always evaluated as true in a boolean context: drivers/staging/wilc1000/linux_wlan.c:267:20: warning: address of 'vif->ndev->dev' will always evaluate to 'true' [-Wpointer-bool-conversion] if (!(&vif->ndev->dev)) ~ ~~~~~~~~~~~^~~ 1 warning generated. Since this statement always evaluates to false due to the logical not, remove it. Link: https://github.com/ClangBuiltLinux/linux/issues/121 Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> --- drivers/staging/wilc1000/linux_wlan.c | 3 --- 1 file changed, 3 deletions(-)