diff mbox

[14/14,media] fix warning on v4l2_subdev_call() result interpreted as bool

Message ID 20170714093938.1469319-1-arnd@arndb.de (mailing list archive)
State New, archived
Headers show

Commit Message

Arnd Bergmann July 14, 2017, 9:36 a.m. UTC
v4l2_subdev_call is a macro returning whatever the callback return
type is, usually 'int'. With gcc-7 and ccache, this can lead to
many wanings like:

media/platform/pxa_camera.c: In function 'pxa_mbus_build_fmts_xlate':
media/platform/pxa_camera.c:766:27: error: ?: using integer constants in boolean context [-Werror=int-in-bool-context]
  while (!v4l2_subdev_call(subdev, pad, enum_mbus_code, NULL, &code)) {
media/atomisp/pci/atomisp2/atomisp_cmd.c: In function 'atomisp_s_ae_window':
media/atomisp/pci/atomisp2/atomisp_cmd.c:6414:52: error: ?: using integer constants in boolean context [-Werror=int-in-bool-context]
  if (v4l2_subdev_call(isp->inputs[asd->input_curr].camera,

The best workaround I could come up with is to change all the
callers that use the return code from v4l2_subdev_call() in an
'if' or 'while' condition.

In case of simple 'if' checks, adding a temporary variable is
usually ok, and sometimes this can be used to propagate or
print an error code, so I do that.

For the 'while' loops, I ended up adding an otherwise useless
comparison with zero, which unfortunately makes the code a little
uglied.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/media/pci/cx18/cx18-ioctl.c                      |  6 ++++--
 drivers/media/pci/saa7146/mxb.c                          |  5 +++--
 drivers/media/platform/atmel/atmel-isc.c                 |  4 ++--
 drivers/media/platform/atmel/atmel-isi.c                 |  4 ++--
 drivers/media/platform/blackfin/bfin_capture.c           |  4 ++--
 drivers/media/platform/omap3isp/ispccdc.c                |  5 +++--
 drivers/media/platform/pxa_camera.c                      |  3 ++-
 drivers/media/platform/rcar-vin/rcar-core.c              |  2 +-
 drivers/media/platform/rcar-vin/rcar-dma.c               |  4 +++-
 drivers/media/platform/soc_camera/soc_camera.c           |  4 ++--
 drivers/media/platform/stm32/stm32-dcmi.c                |  4 ++--
 drivers/media/platform/ti-vpe/cal.c                      |  6 ++++--
 drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c | 13 +++++++------
 13 files changed, 37 insertions(+), 27 deletions(-)

Comments

Dan Carpenter July 14, 2017, 12:05 p.m. UTC | #1
Changing:

- if (!frob()) {
+ if (frob() == 0) {

is a totally pointless change.  They're both bad, because they're doing
success testing instead of failure testing, but probably the second one
is slightly worse.

This warning seems dumb.  I can't imagine it has even a 10% success rate
at finding real bugs.  Just disable it.

Changing the code to propagate error codes, is the right thing of course
so long as it doesn't introduce bugs.

regards,
dan carpenter
Arnd Bergmann July 14, 2017, 12:27 p.m. UTC | #2
On Fri, Jul 14, 2017 at 2:05 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> Changing:
>
> - if (!frob()) {
> + if (frob() == 0) {
>
> is a totally pointless change.  They're both bad, because they're doing
> success testing instead of failure testing, but probably the second one
> is slightly worse.
>
> This warning seems dumb.  I can't imagine it has even a 10% success rate
> at finding real bugs.  Just disable it.
>
> Changing the code to propagate error codes, is the right thing of course
> so long as it doesn't introduce bugs.

It found a two of bugs that I fixed earlier:

f0e8faa7a5e8 ("ARM: ux500: fix prcmu_is_cpu_in_wfi() calculation")
af15769ffab1 ("scsi: mvsas: fix command_active typo")

plus three patches from this series:

1. staging:iio:resolver:ad2s1210 fix negative IIO_ANGL_VEL read
2. isdn: isdnloop: suppress a gcc-7 warning (my patch is wrong,
   as Joe pointed out there is a real bug)
3. drm/vmwgfx: avoid gcc-7 parentheses (here, Linus had a better
   analysis of the problem, so we should consider that a bug as well)

I would estimate around 25% success rate here, which isn't that
bad for a new warning.

I agree that most of the false positives are really dumb though.

       Arnd
Dan Carpenter July 14, 2017, 12:41 p.m. UTC | #3
On Fri, Jul 14, 2017 at 11:36:56AM +0200, Arnd Bergmann wrote:
> @@ -1158,7 +1158,8 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc)
>  	 */
>  	fmt_src.pad = pad->index;
>  	fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> -	if (!v4l2_subdev_call(sensor, pad, get_fmt, NULL, &fmt_src)) {
> +	ret = v4l2_subdev_call(sensor, pad, get_fmt, NULL, &fmt_src);
> +	if (!ret) {
>  		fmt_info = omap3isp_video_format_info(fmt_src.format.code);
>  		depth_in = fmt_info->width;
>  	}

Is the original code buggy?

media/platform/omap3isp/ispccdc.c
  1156          /* Compute the lane shifter shift value and enable the bridge when the
  1157           * input format is a non-BT.656 YUV variant.
  1158           */
  1159          fmt_src.pad = pad->index;
  1160          fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE;
  1161          if (!v4l2_subdev_call(sensor, pad, get_fmt, NULL, &fmt_src)) {
  1162                  fmt_info = omap3isp_video_format_info(fmt_src.format.code);
  1163                  depth_in = fmt_info->width;
  1164          }

If v4l2_subdev_call() then depth_in is zero.

  1165  
  1166          fmt_info = omap3isp_video_format_info(format->code);
  1167          depth_out = fmt_info->width;
  1168          shift = depth_in - depth_out;
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

How do we know that this subtraction doesn't set "shift" to a very high
positive?

  1169  
  1170          if (ccdc->bt656)
  1171                  bridge = ISPCTRL_PAR_BRIDGE_DISABLE;
  1172          else if (fmt_info->code == MEDIA_BUS_FMT_YUYV8_2X8)
  1173                  bridge = ISPCTRL_PAR_BRIDGE_LENDIAN;
  1174          else if (fmt_info->code == MEDIA_BUS_FMT_UYVY8_2X8)
  1175                  bridge = ISPCTRL_PAR_BRIDGE_BENDIAN;
  1176          else
  1177                  bridge = ISPCTRL_PAR_BRIDGE_DISABLE;
  1178  
  1179          omap3isp_configure_bridge(isp, ccdc->input, parcfg, shift, bridge);

regards,
dan carpenter
Dan Carpenter July 14, 2017, 12:55 p.m. UTC | #4
Ah...  I see why it's complaining about these ones in particular...

I don't agree with it as a static analysis dev, and I don't like the
changes too much.  But since it's only generating a hand full of
warnings then I don't care.

regards,
dan carpenter
Dan Carpenter July 14, 2017, 1:09 p.m. UTC | #5
On Fri, Jul 14, 2017 at 03:55:26PM +0300, Dan Carpenter wrote:
> I don't agree with it as a static analysis dev...

What I mean is if it's a macro that returns -ENODEV or a function that
returns -ENODEV, they should both be treated the same.  The other
warnings this check prints are quite clever.

regards,
dan carpenter
Arnd Bergmann July 14, 2017, 7:32 p.m. UTC | #6
On Fri, Jul 14, 2017 at 3:09 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Fri, Jul 14, 2017 at 03:55:26PM +0300, Dan Carpenter wrote:
>> I don't agree with it as a static analysis dev...
>
> What I mean is if it's a macro that returns -ENODEV or a function that
> returns -ENODEV, they should both be treated the same.  The other
> warnings this check prints are quite clever.

I think this is what gcc tries to do, and it should work normally, but it
fails when using ccache. I know I had cases like that, not entirely sure
if this is one of them. Maybe it just means I should give up on using
ccache in preprocessor mode.

       Arnd
Hans Verkuil July 17, 2017, 1:45 p.m. UTC | #7
On 14/07/17 11:36, Arnd Bergmann wrote:
> v4l2_subdev_call is a macro returning whatever the callback return
> type is, usually 'int'. With gcc-7 and ccache, this can lead to
> many wanings like:
> 
> media/platform/pxa_camera.c: In function 'pxa_mbus_build_fmts_xlate':
> media/platform/pxa_camera.c:766:27: error: ?: using integer constants in boolean context [-Werror=int-in-bool-context]
>   while (!v4l2_subdev_call(subdev, pad, enum_mbus_code, NULL, &code)) {
> media/atomisp/pci/atomisp2/atomisp_cmd.c: In function 'atomisp_s_ae_window':
> media/atomisp/pci/atomisp2/atomisp_cmd.c:6414:52: error: ?: using integer constants in boolean context [-Werror=int-in-bool-context]
>   if (v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
> 
> The best workaround I could come up with is to change all the
> callers that use the return code from v4l2_subdev_call() in an
> 'if' or 'while' condition.
> 
> In case of simple 'if' checks, adding a temporary variable is
> usually ok, and sometimes this can be used to propagate or
> print an error code, so I do that.
> 
> For the 'while' loops, I ended up adding an otherwise useless
> comparison with zero, which unfortunately makes the code a little
> uglied.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/media/pci/cx18/cx18-ioctl.c                      |  6 ++++--
>  drivers/media/pci/saa7146/mxb.c                          |  5 +++--
>  drivers/media/platform/atmel/atmel-isc.c                 |  4 ++--
>  drivers/media/platform/atmel/atmel-isi.c                 |  4 ++--
>  drivers/media/platform/blackfin/bfin_capture.c           |  4 ++--
>  drivers/media/platform/omap3isp/ispccdc.c                |  5 +++--
>  drivers/media/platform/pxa_camera.c                      |  3 ++-
>  drivers/media/platform/rcar-vin/rcar-core.c              |  2 +-
>  drivers/media/platform/rcar-vin/rcar-dma.c               |  4 +++-
>  drivers/media/platform/soc_camera/soc_camera.c           |  4 ++--
>  drivers/media/platform/stm32/stm32-dcmi.c                |  4 ++--
>  drivers/media/platform/ti-vpe/cal.c                      |  6 ++++--
>  drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c | 13 +++++++------
>  13 files changed, 37 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/media/pci/cx18/cx18-ioctl.c b/drivers/media/pci/cx18/cx18-ioctl.c
> index 80b902b12a78..1803f28fc501 100644
> --- a/drivers/media/pci/cx18/cx18-ioctl.c
> +++ b/drivers/media/pci/cx18/cx18-ioctl.c
> @@ -188,6 +188,7 @@ static int cx18_g_fmt_sliced_vbi_cap(struct file *file, void *fh,
>  {
>  	struct cx18 *cx = fh2id(fh)->cx;
>  	struct v4l2_sliced_vbi_format *vbifmt = &fmt->fmt.sliced;
> +	int ret;
>  
>  	/* sane, V4L2 spec compliant, defaults */
>  	vbifmt->reserved[0] = 0;
> @@ -201,8 +202,9 @@ static int cx18_g_fmt_sliced_vbi_cap(struct file *file, void *fh,
>  	 * digitizer/slicer.  Note, cx18_av_vbi() wipes the passed in
>  	 * fmt->fmt.sliced under valid calling conditions
>  	 */
> -	if (v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced))
> -		return -EINVAL;
> +	ret = v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced);
> +	if (ret)
> +		return ret;

Please keep the -EINVAL here. I can't be 100% certain that returning 'ret' wouldn't
break something.

>  
>  	vbifmt->service_set = cx18_get_service_set(vbifmt);
>  	return 0;

<snip>

> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> index 97093baf28ac..fe56a037f065 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> @@ -6405,19 +6405,20 @@ int atomisp_s_ae_window(struct atomisp_sub_device *asd,
>  	struct atomisp_device *isp = asd->isp;
>  	/* Coverity CID 298071 - initialzize struct */
>  	struct v4l2_subdev_selection sel = { 0 };
> +	int ret;
>  
>  	sel.r.left = arg->x_left;
>  	sel.r.top = arg->y_top;
>  	sel.r.width = arg->x_right - arg->x_left + 1;
>  	sel.r.height = arg->y_bottom - arg->y_top + 1;
>  
> -	if (v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
> -			     pad, set_selection, NULL, &sel)) {
> -		dev_err(isp->dev, "failed to call sensor set_selection.\n");
> -		return -EINVAL;
> -	}
> +	ret = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
> +			       pad, set_selection, NULL, &sel);
> +	if (ret)
> +		dev_err(isp->dev, "failed to call sensor set_selection: %d\n",
> +			ret);

Same here: just keep the 'return -EINVAL'.

>  
> -	return 0;
> +	return ret;
>  }
>  
>  int atomisp_flash_enable(struct atomisp_sub_device *asd, int num_frames)
> 

This is all very hackish, though. I'm not terribly keen on this patch. It's not
clear to me *why* these warnings appear in your setup.

Regards,

	Hans
Arnd Bergmann July 17, 2017, 2:26 p.m. UTC | #8
On Mon, Jul 17, 2017 at 3:45 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On 14/07/17 11:36, Arnd Bergmann wrote:
>> @@ -201,8 +202,9 @@ static int cx18_g_fmt_sliced_vbi_cap(struct file *file, void *fh,
>>        * digitizer/slicer.  Note, cx18_av_vbi() wipes the passed in
>>        * fmt->fmt.sliced under valid calling conditions
>>        */
>> -     if (v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced))
>> -             return -EINVAL;
>> +     ret = v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced);
>> +     if (ret)
>> +             return ret;
>
> Please keep the -EINVAL here. I can't be 100% certain that returning 'ret' wouldn't
> break something.

I think Dan was recommending the opposite here, if I understood you
both correctly:
he said we should propagate the error code unless we know it's wrong, while you
want to keep the current behavior to avoid introducing changes ;-)

I guess in either case, looking at the callers more carefully would be
a good idea.

>> -     return 0;
>> +     return ret;
>>  }
>>
>>  int atomisp_flash_enable(struct atomisp_sub_device *asd, int num_frames)
>>
>
> This is all very hackish, though. I'm not terribly keen on this patch. It's not
> clear to me *why* these warnings appear in your setup.

it's possible that this only happened with 'ccache', which first preprocesses
the source and the passes it with v4l2_subdev_call expanded into the
compiler. This means the line looks like

        if ((!(cx->sd_av) ? -ENODEV :
            (((cx->sd_av)->ops->vbi && (cx->sd_av)->ops->vbi->g_sliced_fmt) ?
               (cx->sd_av)->ops->vbi->g_sliced_fmt(cx->sd_av)),
&fmt->fmt.sliced) :
               -ENOIOCTLCMD))

The compiler now complains about the sub-expression that it sees for
cx->sd_av==NULL:

   if (-ENODEV)

which it considers nonsense because it is always true and the value gets
ignored.

Let me try again without ccache for now and see what warnings remain.
We can find a solution for those first, and then decide how to deal with
ccache.

        Arnd
Dan Carpenter July 17, 2017, 2:28 p.m. UTC | #9
On Mon, Jul 17, 2017 at 04:26:23PM +0200, Arnd Bergmann wrote:
> On Mon, Jul 17, 2017 at 3:45 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > On 14/07/17 11:36, Arnd Bergmann wrote:
> >> @@ -201,8 +202,9 @@ static int cx18_g_fmt_sliced_vbi_cap(struct file *file, void *fh,
> >>        * digitizer/slicer.  Note, cx18_av_vbi() wipes the passed in
> >>        * fmt->fmt.sliced under valid calling conditions
> >>        */
> >> -     if (v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced))
> >> -             return -EINVAL;
> >> +     ret = v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced);
> >> +     if (ret)
> >> +             return ret;
> >
> > Please keep the -EINVAL here. I can't be 100% certain that returning 'ret' wouldn't
> > break something.
> 
> I think Dan was recommending the opposite here, if I understood you
> both correctly:
> he said we should propagate the error code unless we know it's wrong, while you
> want to keep the current behavior to avoid introducing changes ;-)
> 

I don't know the subsystem rules at all, so don't listen to me.

regards,
dan carpenter
Hans Verkuil July 17, 2017, 2:32 p.m. UTC | #10
On 17/07/17 16:26, Arnd Bergmann wrote:
> On Mon, Jul 17, 2017 at 3:45 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> On 14/07/17 11:36, Arnd Bergmann wrote:
>>> @@ -201,8 +202,9 @@ static int cx18_g_fmt_sliced_vbi_cap(struct file *file, void *fh,
>>>        * digitizer/slicer.  Note, cx18_av_vbi() wipes the passed in
>>>        * fmt->fmt.sliced under valid calling conditions
>>>        */
>>> -     if (v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced))
>>> -             return -EINVAL;
>>> +     ret = v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced);
>>> +     if (ret)
>>> +             return ret;
>>
>> Please keep the -EINVAL here. I can't be 100% certain that returning 'ret' wouldn't
>> break something.
> 
> I think Dan was recommending the opposite here, if I understood you
> both correctly:
> he said we should propagate the error code unless we know it's wrong, while you
> want to keep the current behavior to avoid introducing changes ;-)
> 
> I guess in either case, looking at the callers more carefully would be
> a good idea.

The subtle problem here is that v4l2_subdev_call will return -ENOIOCTLCMD if
ops->vbi->g_sliced_fmt == NULL, which typically is not returned to userspace
but either ignored or replaced by another error. It indicates that the
sub device doesn't implement this operation, and it depends on the context
and the operation whether or not that is to be considered an error.

I have no clue what is expected here, without digging deep in the code.

Better to keep it as-is. It really isn't important to waste time on this.

> 
>>> -     return 0;
>>> +     return ret;
>>>  }
>>>
>>>  int atomisp_flash_enable(struct atomisp_sub_device *asd, int num_frames)
>>>
>>
>> This is all very hackish, though. I'm not terribly keen on this patch. It's not
>> clear to me *why* these warnings appear in your setup.
> 
> it's possible that this only happened with 'ccache', which first preprocesses
> the source and the passes it with v4l2_subdev_call expanded into the
> compiler. This means the line looks like
> 
>         if ((!(cx->sd_av) ? -ENODEV :
>             (((cx->sd_av)->ops->vbi && (cx->sd_av)->ops->vbi->g_sliced_fmt) ?
>                (cx->sd_av)->ops->vbi->g_sliced_fmt(cx->sd_av)),
> &fmt->fmt.sliced) :
>                -ENOIOCTLCMD))
> 
> The compiler now complains about the sub-expression that it sees for
> cx->sd_av==NULL:
> 
>    if (-ENODEV)
> 
> which it considers nonsense because it is always true and the value gets
> ignored.
> 
> Let me try again without ccache for now and see what warnings remain.
> We can find a solution for those first, and then decide how to deal with
> ccache.

Sounds good.

I'm OK with applying this if there is no other way to prevent these warnings.

Regards,

	Hans

> 
>         Arnd
>
Hans Verkuil July 17, 2017, 2:35 p.m. UTC | #11
On 17/07/17 16:26, Arnd Bergmann wrote:
> On Mon, Jul 17, 2017 at 3:45 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> On 14/07/17 11:36, Arnd Bergmann wrote:
>>> @@ -201,8 +202,9 @@ static int cx18_g_fmt_sliced_vbi_cap(struct file *file, void *fh,
>>>        * digitizer/slicer.  Note, cx18_av_vbi() wipes the passed in
>>>        * fmt->fmt.sliced under valid calling conditions
>>>        */
>>> -     if (v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced))
>>> -             return -EINVAL;
>>> +     ret = v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced);
>>> +     if (ret)
>>> +             return ret;
>>
>> Please keep the -EINVAL here. I can't be 100% certain that returning 'ret' wouldn't
>> break something.
> 
> I think Dan was recommending the opposite here, if I understood you
> both correctly:
> he said we should propagate the error code unless we know it's wrong, while you
> want to keep the current behavior to avoid introducing changes ;-)
> 
> I guess in either case, looking at the callers more carefully would be
> a good idea.

The subtle problem here is that v4l2_subdev_call will return -ENOIOCTLCMD if
ops->vbi->g_sliced_fmt == NULL, which typically is not returned to userspace
but either ignored or replaced by another error. It indicates that the
sub device doesn't implement this operation, and it depends on the context
and the operation whether or not that is to be considered an error.

I have no clue what is expected here, without digging deep in the code.

Better to keep it as-is. It really isn't important to waste time on this.

> 
>>> -     return 0;
>>> +     return ret;
>>>  }
>>>
>>>  int atomisp_flash_enable(struct atomisp_sub_device *asd, int num_frames)
>>>
>>
>> This is all very hackish, though. I'm not terribly keen on this patch. It's not
>> clear to me *why* these warnings appear in your setup.
> 
> it's possible that this only happened with 'ccache', which first preprocesses
> the source and the passes it with v4l2_subdev_call expanded into the
> compiler. This means the line looks like
> 
>         if ((!(cx->sd_av) ? -ENODEV :
>             (((cx->sd_av)->ops->vbi && (cx->sd_av)->ops->vbi->g_sliced_fmt) ?
>                (cx->sd_av)->ops->vbi->g_sliced_fmt(cx->sd_av)),
> &fmt->fmt.sliced) :
>                -ENOIOCTLCMD))
> 
> The compiler now complains about the sub-expression that it sees for
> cx->sd_av==NULL:
> 
>    if (-ENODEV)
> 
> which it considers nonsense because it is always true and the value gets
> ignored.
> 
> Let me try again without ccache for now and see what warnings remain.
> We can find a solution for those first, and then decide how to deal with
> ccache.

Sounds good.

I'm OK with applying this if there is no other way to prevent these warnings.

Regards,

	Hans

> 
>         Arnd
>
Arnd Bergmann July 17, 2017, 9:23 p.m. UTC | #12
On Mon, Jul 17, 2017 at 4:35 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On 17/07/17 16:26, Arnd Bergmann wrote:

>> Let me try again without ccache for now and see what warnings remain.
>> We can find a solution for those first, and then decide how to deal with
>> ccache.
>
> Sounds good.
>
> I'm OK with applying this if there is no other way to prevent these warnings.

Small update: I noticed that having ccache being the default compiler
even with CCACHE_DISABLE=1 causes a lot of these warnings. Completely
taking ccache out of the picture however seems to have eliminated the
warnings about v4l2_subdev_call() and other silly warnings, but not
the interesting ones in the -Wint-in-bool-context category.

       Arnd
diff mbox

Patch

diff --git a/drivers/media/pci/cx18/cx18-ioctl.c b/drivers/media/pci/cx18/cx18-ioctl.c
index 80b902b12a78..1803f28fc501 100644
--- a/drivers/media/pci/cx18/cx18-ioctl.c
+++ b/drivers/media/pci/cx18/cx18-ioctl.c
@@ -188,6 +188,7 @@  static int cx18_g_fmt_sliced_vbi_cap(struct file *file, void *fh,
 {
 	struct cx18 *cx = fh2id(fh)->cx;
 	struct v4l2_sliced_vbi_format *vbifmt = &fmt->fmt.sliced;
+	int ret;
 
 	/* sane, V4L2 spec compliant, defaults */
 	vbifmt->reserved[0] = 0;
@@ -201,8 +202,9 @@  static int cx18_g_fmt_sliced_vbi_cap(struct file *file, void *fh,
 	 * digitizer/slicer.  Note, cx18_av_vbi() wipes the passed in
 	 * fmt->fmt.sliced under valid calling conditions
 	 */
-	if (v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced))
-		return -EINVAL;
+	ret = v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced);
+	if (ret)
+		return ret;
 
 	vbifmt->service_set = cx18_get_service_set(vbifmt);
 	return 0;
diff --git a/drivers/media/pci/saa7146/mxb.c b/drivers/media/pci/saa7146/mxb.c
index 504d78807639..d2d843c38579 100644
--- a/drivers/media/pci/saa7146/mxb.c
+++ b/drivers/media/pci/saa7146/mxb.c
@@ -525,8 +525,9 @@  static int vidioc_s_input(struct file *file, void *fh, unsigned int input)
 		return err;
 
 	/* switch video in saa7111a */
-	if (saa7111a_call(mxb, video, s_routing, i, SAA7111_FMT_CCIR, 0))
-		pr_err("VIDIOC_S_INPUT: could not address saa7111a\n");
+	err = saa7111a_call(mxb, video, s_routing, i, SAA7111_FMT_CCIR, 0);
+	if (err)
+		pr_err("VIDIOC_S_INPUT: could not address saa7111a: %d\n", err);
 
 	mxb->cur_audinput = video_audio_connect[input];
 	/* switch the audio-source only if necessary */
diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
index d6534252cdcd..704b34a0cc00 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -1475,8 +1475,8 @@  static int isc_formats_init(struct isc_device *isc)
 		fmt++;
 	}
 
-	while (!v4l2_subdev_call(subdev, pad, enum_mbus_code,
-	       NULL, &mbus_code)) {
+	while (v4l2_subdev_call(subdev, pad, enum_mbus_code,
+	       NULL, &mbus_code) == 0) {
 		mbus_code.index++;
 		fmt = find_format_by_code(mbus_code.code, &i);
 		if (!fmt)
diff --git a/drivers/media/platform/atmel/atmel-isi.c b/drivers/media/platform/atmel/atmel-isi.c
index 891fa2505efa..30b7e6f298ed 100644
--- a/drivers/media/platform/atmel/atmel-isi.c
+++ b/drivers/media/platform/atmel/atmel-isi.c
@@ -1013,8 +1013,8 @@  static int isi_formats_init(struct atmel_isi *isi)
 		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
 	};
 
-	while (!v4l2_subdev_call(subdev, pad, enum_mbus_code,
-				 NULL, &mbus_code)) {
+	while (v4l2_subdev_call(subdev, pad, enum_mbus_code,
+				NULL, &mbus_code) == 0) {
 		for (i = 0; i < ARRAY_SIZE(isi_formats); i++) {
 			if (isi_formats[i].mbus_code != mbus_code.code)
 				continue;
diff --git a/drivers/media/platform/blackfin/bfin_capture.c b/drivers/media/platform/blackfin/bfin_capture.c
index 1c5166df46f5..864c98f21a0e 100644
--- a/drivers/media/platform/blackfin/bfin_capture.c
+++ b/drivers/media/platform/blackfin/bfin_capture.c
@@ -157,8 +157,8 @@  static int bcap_init_sensor_formats(struct bcap_device *bcap_dev)
 	unsigned int num_formats = 0;
 	int i, j;
 
-	while (!v4l2_subdev_call(bcap_dev->sd, pad,
-				enum_mbus_code, NULL, &code)) {
+	while (v4l2_subdev_call(bcap_dev->sd, pad,
+				enum_mbus_code, NULL, &code) == 0) {
 		num_formats++;
 		code.index++;
 	}
diff --git a/drivers/media/platform/omap3isp/ispccdc.c b/drivers/media/platform/omap3isp/ispccdc.c
index 7207558d722c..a94157461f58 100644
--- a/drivers/media/platform/omap3isp/ispccdc.c
+++ b/drivers/media/platform/omap3isp/ispccdc.c
@@ -1132,6 +1132,7 @@  static void ccdc_configure(struct isp_ccdc_device *ccdc)
 	unsigned int sph;
 	u32 syn_mode;
 	u32 ccdc_pattern;
+	int ret;
 
 	ccdc->bt656 = false;
 	ccdc->fields = 0;
@@ -1140,7 +1141,6 @@  static void ccdc_configure(struct isp_ccdc_device *ccdc)
 	sensor = media_entity_to_v4l2_subdev(pad->entity);
 	if (ccdc->input == CCDC_INPUT_PARALLEL) {
 		struct v4l2_mbus_config cfg;
-		int ret;
 
 		ret = v4l2_subdev_call(sensor, video, g_mbus_config, &cfg);
 		if (!ret)
@@ -1158,7 +1158,8 @@  static void ccdc_configure(struct isp_ccdc_device *ccdc)
 	 */
 	fmt_src.pad = pad->index;
 	fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE;
-	if (!v4l2_subdev_call(sensor, pad, get_fmt, NULL, &fmt_src)) {
+	ret = v4l2_subdev_call(sensor, pad, get_fmt, NULL, &fmt_src);
+	if (!ret) {
 		fmt_info = omap3isp_video_format_info(fmt_src.format.code);
 		depth_in = fmt_info->width;
 	}
diff --git a/drivers/media/platform/pxa_camera.c b/drivers/media/platform/pxa_camera.c
index 399095170b6e..5236c7b171ea 100644
--- a/drivers/media/platform/pxa_camera.c
+++ b/drivers/media/platform/pxa_camera.c
@@ -763,7 +763,8 @@  static struct soc_camera_format_xlate *pxa_mbus_build_fmts_xlate(
 	};
 	struct soc_camera_format_xlate *user_formats;
 
-	while (!v4l2_subdev_call(subdev, pad, enum_mbus_code, NULL, &code)) {
+	while (v4l2_subdev_call(subdev, pad, enum_mbus_code, NULL, &code) ==
+               0) {
 		raw_fmts++;
 		code.index++;
 	}
diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 77dff047c41c..a41f4a3d9b69 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -54,7 +54,7 @@  static bool rvin_mbus_supported(struct rvin_graph_entity *entity)
 
 	code.index = 0;
 	code.pad = entity->source_pad;
-	while (!v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &code)) {
+	while (v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &code) == 0) {
 		code.index++;
 		switch (code.code) {
 		case MEDIA_BUS_FMT_YUYV8_1X16:
diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index b136844499f6..ee16e9886041 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -143,6 +143,7 @@  static int rvin_setup(struct rvin_dev *vin)
 	u32 vnmc, dmr, dmr2, interrupts;
 	v4l2_std_id std;
 	bool progressive = false, output_is_yuv = false, input_is_yuv = false;
+	int ret;
 
 	switch (vin->format.field) {
 	case V4L2_FIELD_TOP:
@@ -155,7 +156,8 @@  static int rvin_setup(struct rvin_dev *vin)
 		/* Default to TB */
 		vnmc = VNMC_IM_FULL;
 		/* Use BT if video standard can be read and is 60 Hz format */
-		if (!v4l2_subdev_call(vin_to_source(vin), video, g_std, &std)) {
+		ret = v4l2_subdev_call(vin_to_source(vin), video, g_std, &std);
+		if (ret) {
 			if (std & V4L2_STD_525_60)
 				vnmc = VNMC_IM_FULL | VNMC_FOC;
 		}
diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
index 45a0429d75bb..3ef648fc2db6 100644
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -454,7 +454,7 @@  static int soc_camera_init_user_formats(struct soc_camera_device *icd)
 		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
 	};
 
-	while (!v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &code)) {
+	while (v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &code) == 0) {
 		raw_fmts++;
 		code.index++;
 	}
@@ -1202,7 +1202,7 @@  static int soc_camera_probe_finish(struct soc_camera_device *icd)
 		goto evidstart;
 
 	/* Try to improve our guess of a reasonable window format */
-	if (!v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt)) {
+	if (v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt) == 0) {
 		icd->user_width		= mf->width;
 		icd->user_height	= mf->height;
 		icd->colorspace		= mf->colorspace;
diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
index 83d32a5d0f40..96084dfd5d11 100644
--- a/drivers/media/platform/stm32/stm32-dcmi.c
+++ b/drivers/media/platform/stm32/stm32-dcmi.c
@@ -1034,8 +1034,8 @@  static int dcmi_formats_init(struct stm32_dcmi *dcmi)
 		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
 	};
 
-	while (!v4l2_subdev_call(subdev, pad, enum_mbus_code,
-				 NULL, &mbus_code)) {
+	while (v4l2_subdev_call(subdev, pad, enum_mbus_code,
+				 NULL, &mbus_code) == 0) {
 		for (i = 0; i < ARRAY_SIZE(dcmi_formats); i++) {
 			if (dcmi_formats[i].mbus_code != mbus_code.code)
 				continue;
diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
index 177faa36bc16..df0216a6367c 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -1348,9 +1348,11 @@  static void cal_stop_streaming(struct vb2_queue *vq)
 	struct cal_dmaqueue *dma_q = &ctx->vidq;
 	struct cal_buffer *buf, *tmp;
 	unsigned long flags;
+	int ret;
 
-	if (v4l2_subdev_call(ctx->sensor, video, s_stream, 0))
-		ctx_err(ctx, "stream off failed in subdev\n");
+	ret = v4l2_subdev_call(ctx->sensor, video, s_stream, 0);
+	if (ret)
+		ctx_err(ctx, "stream off failed in subdev: %d\n", ret);
 
 	csi2_ppi_disable(ctx);
 	disable_irqs(ctx);
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
index 97093baf28ac..fe56a037f065 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
@@ -6405,19 +6405,20 @@  int atomisp_s_ae_window(struct atomisp_sub_device *asd,
 	struct atomisp_device *isp = asd->isp;
 	/* Coverity CID 298071 - initialzize struct */
 	struct v4l2_subdev_selection sel = { 0 };
+	int ret;
 
 	sel.r.left = arg->x_left;
 	sel.r.top = arg->y_top;
 	sel.r.width = arg->x_right - arg->x_left + 1;
 	sel.r.height = arg->y_bottom - arg->y_top + 1;
 
-	if (v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
-			     pad, set_selection, NULL, &sel)) {
-		dev_err(isp->dev, "failed to call sensor set_selection.\n");
-		return -EINVAL;
-	}
+	ret = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
+			       pad, set_selection, NULL, &sel);
+	if (ret)
+		dev_err(isp->dev, "failed to call sensor set_selection: %d\n",
+			ret);
 
-	return 0;
+	return ret;
 }
 
 int atomisp_flash_enable(struct atomisp_sub_device *asd, int num_frames)