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

[Hans Verkuil]

On 17/07/17 16:26, Arnd Bergmann wrote:
> On Mon, Jul 17, 2017 at 3:45 PM, Hans Verkuil <hverkuil at 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
>