[PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool
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
---
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;
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,
};
-
Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool
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
Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool
On Fri, Jul 14, 2017 at 2:05 PM, Dan Carpenter 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
Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool
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
Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool
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
Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool
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
Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool
On Fri, Jul 14, 2017 at 3:09 PM, Dan Carpenter 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
Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool
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
> ---
> 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;
> 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,
Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool
On Mon, Jul 17, 2017 at 3:45 PM, Hans Verkuil 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
Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool
On Mon, Jul 17, 2017 at 04:26:23PM +0200, Arnd Bergmann wrote: > On Mon, Jul 17, 2017 at 3:45 PM, Hans Verkuil 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
Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool
On 17/07/17 16:26, Arnd Bergmann wrote: > On Mon, Jul 17, 2017 at 3:45 PM, Hans Verkuil 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 >
Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool
On 17/07/17 16:26, Arnd Bergmann wrote: > On Mon, Jul 17, 2017 at 3:45 PM, Hans Verkuil 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 >
Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool
On Mon, Jul 17, 2017 at 4:35 PM, Hans Verkuil 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
