* [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool [not found] <20170714092540.1217397-1-arnd@arndb.de> @ 2017-07-14 9:36 ` Arnd Bergmann 2017-07-14 12:05 ` Dan Carpenter ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Arnd Bergmann @ 2017-07-14 9:36 UTC (permalink / raw) To: linux-arm-kernel 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; 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) -- 2.9.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool 2017-07-14 9:36 ` [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool Arnd Bergmann @ 2017-07-14 12:05 ` Dan Carpenter 2017-07-14 12:27 ` Arnd Bergmann 2017-07-14 12:41 ` Dan Carpenter 2017-07-17 13:45 ` Hans Verkuil 2 siblings, 1 reply; 13+ messages in thread From: Dan Carpenter @ 2017-07-14 12:05 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool 2017-07-14 12:05 ` Dan Carpenter @ 2017-07-14 12:27 ` Arnd Bergmann 2017-07-14 12:55 ` Dan Carpenter 0 siblings, 1 reply; 13+ messages in thread From: Arnd Bergmann @ 2017-07-14 12:27 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool 2017-07-14 12:27 ` Arnd Bergmann @ 2017-07-14 12:55 ` Dan Carpenter 2017-07-14 13:09 ` Dan Carpenter 0 siblings, 1 reply; 13+ messages in thread From: Dan Carpenter @ 2017-07-14 12:55 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool 2017-07-14 12:55 ` Dan Carpenter @ 2017-07-14 13:09 ` Dan Carpenter 2017-07-14 19:32 ` Arnd Bergmann 0 siblings, 1 reply; 13+ messages in thread From: Dan Carpenter @ 2017-07-14 13:09 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool 2017-07-14 13:09 ` Dan Carpenter @ 2017-07-14 19:32 ` Arnd Bergmann 0 siblings, 0 replies; 13+ messages in thread From: Arnd Bergmann @ 2017-07-14 19:32 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool 2017-07-14 9:36 ` [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool Arnd Bergmann 2017-07-14 12:05 ` Dan Carpenter @ 2017-07-14 12:41 ` Dan Carpenter 2017-07-17 13:45 ` Hans Verkuil 2 siblings, 0 replies; 13+ messages in thread From: Dan Carpenter @ 2017-07-14 12:41 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool 2017-07-14 9:36 ` [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool Arnd Bergmann 2017-07-14 12:05 ` Dan Carpenter 2017-07-14 12:41 ` Dan Carpenter @ 2017-07-17 13:45 ` Hans Verkuil 2017-07-17 14:26 ` Arnd Bergmann 2 siblings, 1 reply; 13+ messages in thread From: Hans Verkuil @ 2017-07-17 13:45 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool 2017-07-17 13:45 ` Hans Verkuil @ 2017-07-17 14:26 ` Arnd Bergmann 2017-07-17 14:28 ` Dan Carpenter ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Arnd Bergmann @ 2017-07-17 14:26 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool 2017-07-17 14:26 ` Arnd Bergmann @ 2017-07-17 14:28 ` Dan Carpenter 2017-07-17 14:32 ` Hans Verkuil 2017-07-17 14:35 ` Hans Verkuil 2 siblings, 0 replies; 13+ messages in thread From: Dan Carpenter @ 2017-07-17 14:28 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool 2017-07-17 14:26 ` Arnd Bergmann 2017-07-17 14:28 ` Dan Carpenter @ 2017-07-17 14:32 ` Hans Verkuil 2017-07-17 14:35 ` Hans Verkuil 2 siblings, 0 replies; 13+ messages in thread From: Hans Verkuil @ 2017-07-17 14:32 UTC (permalink / raw) To: linux-arm-kernel 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 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool 2017-07-17 14:26 ` Arnd Bergmann 2017-07-17 14:28 ` Dan Carpenter 2017-07-17 14:32 ` Hans Verkuil @ 2017-07-17 14:35 ` Hans Verkuil 2017-07-17 21:23 ` Arnd Bergmann 2 siblings, 1 reply; 13+ messages in thread From: Hans Verkuil @ 2017-07-17 14:35 UTC (permalink / raw) To: linux-arm-kernel 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 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool 2017-07-17 14:35 ` Hans Verkuil @ 2017-07-17 21:23 ` Arnd Bergmann 0 siblings, 0 replies; 13+ messages in thread From: Arnd Bergmann @ 2017-07-17 21:23 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-07-17 21:23 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20170714092540.1217397-1-arnd@arndb.de>
2017-07-14 9:36 ` [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool Arnd Bergmann
2017-07-14 12:05 ` Dan Carpenter
2017-07-14 12:27 ` Arnd Bergmann
2017-07-14 12:55 ` Dan Carpenter
2017-07-14 13:09 ` Dan Carpenter
2017-07-14 19:32 ` Arnd Bergmann
2017-07-14 12:41 ` Dan Carpenter
2017-07-17 13:45 ` Hans Verkuil
2017-07-17 14:26 ` Arnd Bergmann
2017-07-17 14:28 ` Dan Carpenter
2017-07-17 14:32 ` Hans Verkuil
2017-07-17 14:35 ` Hans Verkuil
2017-07-17 21:23 ` Arnd Bergmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox