| Message ID | 20180921195736.7977-1-natechancellor@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: cx18: Don't check for address of video_dev | expand |
On Fri, Sep 21, 2018 at 1:03 PM Nathan Chancellor <natechancellor@gmail.com> wrote: > > Clang warns that the address of a pointer will always evaluated as true > in a boolean context. > > drivers/media/pci/cx18/cx18-driver.c:1255:23: warning: address of > 'cx->streams[i].video_dev' will always evaluate to 'true' > [-Wpointer-bool-conversion] > if (&cx->streams[i].video_dev) > ~~ ~~~~~~~~~~~~~~~^~~~~~~~~ > 1 warning generated. > > Presumably, the contents of video_dev should have been checked, not the > address. This check has been present since 2009, introduced by commit > 21a278b85d3c ("V4L/DVB (11619): cx18: Simplify the work handler for > outgoing mailbox commands") > > Reported-by: Nick Desaulniers <ndesaulniers@google.com> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > --- > > Alternatively, this if statement could just be removed since it has > evaluated to true since 2009 and I assume some issue with this would > have been discovered by now. > > drivers/media/pci/cx18/cx18-driver.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/pci/cx18/cx18-driver.c b/drivers/media/pci/cx18/cx18-driver.c > index 56763c4ea1a7..753a37c7100a 100644 > --- a/drivers/media/pci/cx18/cx18-driver.c > +++ b/drivers/media/pci/cx18/cx18-driver.c > @@ -1252,7 +1252,7 @@ static void cx18_cancel_out_work_orders(struct cx18 *cx) > { > int i; > for (i = 0; i < CX18_MAX_STREAMS; i++) > - if (&cx->streams[i].video_dev) > + if (cx->streams[i].video_dev) cx->streams[i].video_dev has the type `struct video_device video_dev`. So wouldn't this change always be true as well, since the struct is embedded? > cancel_work_sync(&cx->streams[i].out_work_order); > } > > -- > 2.19.0 >
On Fri, Sep 21, 2018 at 01:31:37PM -0700, Nick Desaulniers wrote: > On Fri, Sep 21, 2018 at 1:03 PM Nathan Chancellor > <natechancellor@gmail.com> wrote: > > > > Clang warns that the address of a pointer will always evaluated as true > > in a boolean context. > > > > drivers/media/pci/cx18/cx18-driver.c:1255:23: warning: address of > > 'cx->streams[i].video_dev' will always evaluate to 'true' > > [-Wpointer-bool-conversion] > > if (&cx->streams[i].video_dev) > > ~~ ~~~~~~~~~~~~~~~^~~~~~~~~ > > 1 warning generated. > > > > Presumably, the contents of video_dev should have been checked, not the > > address. This check has been present since 2009, introduced by commit > > 21a278b85d3c ("V4L/DVB (11619): cx18: Simplify the work handler for > > outgoing mailbox commands") > > > > Reported-by: Nick Desaulniers <ndesaulniers@google.com> > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > --- > > > > Alternatively, this if statement could just be removed since it has > > evaluated to true since 2009 and I assume some issue with this would > > have been discovered by now. > > > > drivers/media/pci/cx18/cx18-driver.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/media/pci/cx18/cx18-driver.c b/drivers/media/pci/cx18/cx18-driver.c > > index 56763c4ea1a7..753a37c7100a 100644 > > --- a/drivers/media/pci/cx18/cx18-driver.c > > +++ b/drivers/media/pci/cx18/cx18-driver.c > > @@ -1252,7 +1252,7 @@ static void cx18_cancel_out_work_orders(struct cx18 *cx) > > { > > int i; > > for (i = 0; i < CX18_MAX_STREAMS; i++) > > - if (&cx->streams[i].video_dev) > > + if (cx->streams[i].video_dev) > > cx->streams[i].video_dev has the type `struct video_device video_dev`. > So wouldn't this change always be true as well, since the struct is > embedded? > Guess I forgot to compile this with Clang before sending because I now get a build error (sigh...) drivers/media/pci/cx18/cx18-driver.c:1255:3: error: statement requires expression of scalar type ('struct video_device' invalid) if (cx->streams[i].video_dev) ^ ~~~~~~~~~~~~~~~~~~~~~~~~ 1 error generated. I guess the whole if statement should go unless I'm missing something else. Thanks for the review! Nathan > > cancel_work_sync(&cx->streams[i].out_work_order); > > } > > > > -- > > 2.19.0 > > > > > -- > Thanks, > ~Nick Desaulniers
Hi Nathan,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v4.19-rc4 next-20180921]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Nathan-Chancellor/media-cx18-Don-t-check-for-address-of-video_dev/20180922-192000
base: git://linuxtv.org/media_tree.git master
config: i386-randconfig-x077-201837 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All error/warnings (new ones prefixed by >>):
In file included from include/linux/kernel.h:10:0,
from include/linux/list.h:9,
from include/linux/module.h:9,
from drivers/media/pci/cx18/cx18-driver.h:23,
from drivers/media/pci/cx18/cx18-driver.c:20:
drivers/media/pci/cx18/cx18-driver.c: In function 'cx18_cancel_out_work_orders':
>> include/linux/compiler.h:58:28: error: wrong type argument to unary exclamation mark
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^
include/linux/compiler.h:56:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
>> drivers/media/pci/cx18/cx18-driver.c:1255:3: note: in expansion of macro 'if'
if (cx->streams[i].video_dev)
^~
include/linux/compiler.h:58:40: error: wrong type argument to unary exclamation mark
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^
include/linux/compiler.h:56:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
>> drivers/media/pci/cx18/cx18-driver.c:1255:3: note: in expansion of macro 'if'
if (cx->streams[i].video_dev)
^~
include/linux/compiler.h:69:14: error: wrong type argument to unary exclamation mark
______r = !!(cond); \
^
include/linux/compiler.h:56:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
>> drivers/media/pci/cx18/cx18-driver.c:1255:3: note: in expansion of macro 'if'
if (cx->streams[i].video_dev)
^~
--
In file included from include/linux/kernel.h:10:0,
from include/linux/list.h:9,
from include/linux/module.h:9,
from drivers/media//pci/cx18/cx18-driver.h:23,
from drivers/media//pci/cx18/cx18-driver.c:20:
drivers/media//pci/cx18/cx18-driver.c: In function 'cx18_cancel_out_work_orders':
>> include/linux/compiler.h:58:28: error: wrong type argument to unary exclamation mark
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^
include/linux/compiler.h:56:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
drivers/media//pci/cx18/cx18-driver.c:1255:3: note: in expansion of macro 'if'
if (cx->streams[i].video_dev)
^~
include/linux/compiler.h:58:40: error: wrong type argument to unary exclamation mark
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^
include/linux/compiler.h:56:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
drivers/media//pci/cx18/cx18-driver.c:1255:3: note: in expansion of macro 'if'
if (cx->streams[i].video_dev)
^~
include/linux/compiler.h:69:14: error: wrong type argument to unary exclamation mark
______r = !!(cond); \
^
include/linux/compiler.h:56:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
drivers/media//pci/cx18/cx18-driver.c:1255:3: note: in expansion of macro 'if'
if (cx->streams[i].video_dev)
^~
vim +/if +1255 drivers/media/pci/cx18/cx18-driver.c
1250
1251 static void cx18_cancel_out_work_orders(struct cx18 *cx)
1252 {
1253 int i;
1254 for (i = 0; i < CX18_MAX_STREAMS; i++)
> 1255 if (cx->streams[i].video_dev)
1256 cancel_work_sync(&cx->streams[i].out_work_order);
1257 }
1258
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Nathan,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v4.19-rc4 next-20180921]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Nathan-Chancellor/media-cx18-Don-t-check-for-address-of-video_dev/20180922-192000
base: git://linuxtv.org/media_tree.git master
config: xtensa-allmodconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 8.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=8.1.0 make.cross ARCH=xtensa
All errors (new ones prefixed by >>):
drivers/media//pci/cx18/cx18-driver.c: In function 'cx18_cancel_out_work_orders':
>> drivers/media//pci/cx18/cx18-driver.c:1255:7: error: used struct type value where scalar is required
if (cx->streams[i].video_dev)
^~
vim +1255 drivers/media//pci/cx18/cx18-driver.c
1250
1251 static void cx18_cancel_out_work_orders(struct cx18 *cx)
1252 {
1253 int i;
1254 for (i = 0; i < CX18_MAX_STREAMS; i++)
> 1255 if (cx->streams[i].video_dev)
1256 cancel_work_sync(&cx->streams[i].out_work_order);
1257 }
1258
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 09/21/2018 09:57 PM, Nathan Chancellor wrote: > Clang warns that the address of a pointer will always evaluated as true > in a boolean context. > > drivers/media/pci/cx18/cx18-driver.c:1255:23: warning: address of > 'cx->streams[i].video_dev' will always evaluate to 'true' > [-Wpointer-bool-conversion] > if (&cx->streams[i].video_dev) > ~~ ~~~~~~~~~~~~~~~^~~~~~~~~ > 1 warning generated. > > Presumably, the contents of video_dev should have been checked, not the > address. This check has been present since 2009, introduced by commit > 21a278b85d3c ("V4L/DVB (11619): cx18: Simplify the work handler for > outgoing mailbox commands") > > Reported-by: Nick Desaulniers <ndesaulniers@google.com> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > --- > > Alternatively, this if statement could just be removed since it has > evaluated to true since 2009 and I assume some issue with this would > have been discovered by now. > > drivers/media/pci/cx18/cx18-driver.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/pci/cx18/cx18-driver.c b/drivers/media/pci/cx18/cx18-driver.c > index 56763c4ea1a7..753a37c7100a 100644 > --- a/drivers/media/pci/cx18/cx18-driver.c > +++ b/drivers/media/pci/cx18/cx18-driver.c > @@ -1252,7 +1252,7 @@ static void cx18_cancel_out_work_orders(struct cx18 *cx) > { > int i; > for (i = 0; i < CX18_MAX_STREAMS; i++) > - if (&cx->streams[i].video_dev) > + if (cx->streams[i].video_dev) This should read: if (cx->streams[i].video_dev.v4l2_dev) If cx->streams[i].video_dev.v4l2_dev == NULL, then the stream is not in use and there is no need to cancel any work. Can you post a v2? > cancel_work_sync(&cx->streams[i].out_work_order); > } > > Regards, Hans
On Mon, Oct 01, 2018 at 10:30:54AM +0200, Hans Verkuil wrote: > On 09/21/2018 09:57 PM, Nathan Chancellor wrote: > > Clang warns that the address of a pointer will always evaluated as true > > in a boolean context. > > > > drivers/media/pci/cx18/cx18-driver.c:1255:23: warning: address of > > 'cx->streams[i].video_dev' will always evaluate to 'true' > > [-Wpointer-bool-conversion] > > if (&cx->streams[i].video_dev) > > ~~ ~~~~~~~~~~~~~~~^~~~~~~~~ > > 1 warning generated. > > > > Presumably, the contents of video_dev should have been checked, not the > > address. This check has been present since 2009, introduced by commit > > 21a278b85d3c ("V4L/DVB (11619): cx18: Simplify the work handler for > > outgoing mailbox commands") > > > > Reported-by: Nick Desaulniers <ndesaulniers@google.com> > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > --- > > > > Alternatively, this if statement could just be removed since it has > > evaluated to true since 2009 and I assume some issue with this would > > have been discovered by now. > > > > drivers/media/pci/cx18/cx18-driver.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/media/pci/cx18/cx18-driver.c b/drivers/media/pci/cx18/cx18-driver.c > > index 56763c4ea1a7..753a37c7100a 100644 > > --- a/drivers/media/pci/cx18/cx18-driver.c > > +++ b/drivers/media/pci/cx18/cx18-driver.c > > @@ -1252,7 +1252,7 @@ static void cx18_cancel_out_work_orders(struct cx18 *cx) > > { > > int i; > > for (i = 0; i < CX18_MAX_STREAMS; i++) > > - if (&cx->streams[i].video_dev) > > + if (cx->streams[i].video_dev) > > This should read: > > if (cx->streams[i].video_dev.v4l2_dev) > > If cx->streams[i].video_dev.v4l2_dev == NULL, then the stream is not in use > and there is no need to cancel any work. > > Can you post a v2? > Hi Hans, Yes, sorry, I completely forgot to look into this further. I will sent a v2 right now. Thank you for the review, Nathan > > cancel_work_sync(&cx->streams[i].out_work_order); > > } > > > > > > Regards, > > Hans
diff --git a/drivers/media/pci/cx18/cx18-driver.c b/drivers/media/pci/cx18/cx18-driver.c index 56763c4ea1a7..753a37c7100a 100644 --- a/drivers/media/pci/cx18/cx18-driver.c +++ b/drivers/media/pci/cx18/cx18-driver.c @@ -1252,7 +1252,7 @@ static void cx18_cancel_out_work_orders(struct cx18 *cx) { int i; for (i = 0; i < CX18_MAX_STREAMS; i++) - if (&cx->streams[i].video_dev) + if (cx->streams[i].video_dev) cancel_work_sync(&cx->streams[i].out_work_order); }
Clang warns that the address of a pointer will always evaluated as true in a boolean context. drivers/media/pci/cx18/cx18-driver.c:1255:23: warning: address of 'cx->streams[i].video_dev' will always evaluate to 'true' [-Wpointer-bool-conversion] if (&cx->streams[i].video_dev) ~~ ~~~~~~~~~~~~~~~^~~~~~~~~ 1 warning generated. Presumably, the contents of video_dev should have been checked, not the address. This check has been present since 2009, introduced by commit 21a278b85d3c ("V4L/DVB (11619): cx18: Simplify the work handler for outgoing mailbox commands") Reported-by: Nick Desaulniers <ndesaulniers@google.com> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> --- Alternatively, this if statement could just be removed since it has evaluated to true since 2009 and I assume some issue with this would have been discovered by now. drivers/media/pci/cx18/cx18-driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)