[PATCH] staging: wilc1000: Remove unnecessary pointer check

[PATCH] staging: wilc1000: Remove unnecessary pointer check

[Nathan Chancellor]

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

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;
-- 
2.19.0

Re: [PATCH] staging: wilc1000: Remove unnecessary pointer check

[valdis.kletnieks]

[-- Attachment #1: Type: text/plain, Size: 1033 bytes --]

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



[-- Attachment #2: Type: application/pgp-signature, Size: 486 bytes --]

Re: [PATCH] staging: wilc1000: Remove unnecessary pointer check

[Nathan Chancellor]

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

Re: [PATCH] staging: wilc1000: Remove unnecessary pointer check

[Ajay Singh]

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

Re: [PATCH] staging: wilc1000: Remove unnecessary pointer check

[Dan Carpenter]

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