Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751528AbdGQOfe (ORCPT ); Mon, 17 Jul 2017 10:35:34 -0400 Received: from lb1-smtp-cloud2.xs4all.net ([194.109.24.21]:49598 "EHLO lb1-smtp-cloud2.xs4all.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751489AbdGQOfb (ORCPT ); Mon, 17 Jul 2017 10:35:31 -0400 From: Hans Verkuil Subject: Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool To: Arnd Bergmann References: <20170714092540.1217397-1-arnd@arndb.de> <20170714093938.1469319-1-arnd@arndb.de> Cc: Linux Kernel Mailing List , Mauro Carvalho Chehab , Greg Kroah-Hartman , Linus Torvalds , Tejun Heo , Guenter Roeck , IDE-ML , Linux Media Mailing List , Andrew Morton , dri-devel , =?UTF-8?Q?Niklas_S=c3=b6derlund?= , Robert Jarzmik , Daeseok Youn , Alan Cox , adi-buildroot-devel@lists.sourceforge.net, Linux-Renesas , Linux ARM , devel@driverdev.osuosl.org Message-ID: <3a927e60-332a-f01d-f1af-98649e9f51b5@xs4all.nl> Date: Mon, 17 Jul 2017 16:35:27 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2819 Lines: 79 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 >