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

Reviewed-by: Ajay Singh <ajay.kathat@microchip.com>

On Thu, 20 Sep 2018 22:39:11 -0700
Nathan Chancellor [off-list ref] wrote:

On Fri, Sep 21, 2018 at 01:25:32AM -0400, valdis.kletnieks@vt.edu
wrote:

quoted

On Thu, 20 Sep 2018 14:26:49 -0700, Nathan Chancellor said:  

quoted

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