On 02/09/18 14:09, Sakari Ailus wrote:
> On Fri, Feb 09, 2018 at 01:48:49PM +0100, Hans Verkuil wrote:
>> On 02/09/18 13:38, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> On Fri, Feb 09, 2018 at 12:56:37PM +0100, Hans Verkuil wrote:
>>>> On 02/09/18 12:46, Sakari Ailus wrote:
>>>>> Hi Hans,
>>>>>
>>>>> On Thu, Feb 08, 2018 at 09:36:44AM +0100, Hans Verkuil wrote:
>>>>>> If the subdev did not define any controls, then return -ENOTTY if
>>>>>> userspace attempts to call these ioctls.
>>>>>>
>>>>>> The control framework functions will return -EINVAL, not -ENOTTY if
>>>>>> vfh->ctrl_handler is NULL.
>>>>>>
>>>>>> Several of these framework functions are also called directly from
>>>>>> drivers, so I don't want to change the error code there.
>>>>>>
>>>>>> Found with vimc and v4l2-compliance.
>>>>>>
>>>>>> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>>>>>
>>>>> Thanks for the patch.
>>>>>
>>>>> If the handler is NULL, can there be support for the IOCTL at all? I.e.
>>>>> should the missing handler as such result in returning -ENOTTY from these
>>>>> functions instead of -EINVAL?
>>>>
>>>> I didn't dare change the control framework. Some of these v4l2_... functions
>>>> are called by drivers and I didn't want to analyze them all. If these
>>>> functions were only called by v4l2-ioctl.c and v4l2-subdev.c, then I'd have
>>>> changed it in v4l2-ctrls.c, but that's not the case.
>>>>
>>>> It would be a useful project to replace all calls from drivers to these
>>>> functions (they really shouldn't be used by drivers), but that is out-of-scope
>>>> of this patch.
>>>
>>> Is your concern that the caller could check the return value and do
>>> something based on particular error code it gets?
>>
>> Or that the handler is NULL and it returns ENOTTY to userspace. You can have
>> multiple control handlers, some of which might be NULL. It's all unlikely,
>> but the code needs to be analyzed and that takes time. Hmm, atomisp is
>> definitely a big user of these functions.
>>
>> Also, the real issue is the use of these functions by drivers. What I want
>> to do is to have the drivers use the proper functions, then I can move those
>> functions to the core and stop exporting them. And at that moment they can
>> return -ENOTTY instead of -EINVAL.
>>
>> A worthwhile project, but right now I just want to fix v4l2-subdev.c.
>
> Fair enough. How about adding a TODO comment on this in either in the
> control framework or where the additional checks are now put, to avoid
> forgetting it?
It's certainly worth a TODO comment, I'll add that. Very good point.
Regards,
Hans
>
> With that,
>
> Acked-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
>
>>
>> Regards,
>>
>> Hans
>>
>>> Based on a quick glance there are a few tens of places these functions are
>>> used in drivers. Some seems legitimate; the caller having another device
>>> where a control needs to be accessed, for instance.
>>>
>>> And if handler is NULL, -ENOTTY appears to be a more suitable return value
>>> in a lot of the cases (and in many others it makes no difference).
>>>
>>> I wouldn't say this is something that should hold back addressing this in
>>> the control framework instead.
>>>
>>> I can submit a patch if you'd prefer that instead.
>>>
>>>>
>>>> Regards,
>>>>
>>>> Hans
>>>>
>>>>>
>>>>>> ---
>>>>>> drivers/media/v4l2-core/v4l2-subdev.c | 16 ++++++++++++++++
>>>>>> 1 file changed, 16 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>>>>>> index 43fefa73e0a3..be7a19272614 100644
>>>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>>>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>>>>>> @@ -187,27 +187,43 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>>>>>>
>>>>>> switch (cmd) {
>>>>>> case VIDIOC_QUERYCTRL:
>>>>>> + if (!vfh->ctrl_handler)
>>>>>> + return -ENOTTY;
>>>>>> return v4l2_queryctrl(vfh->ctrl_handler, arg);
>>>>>>
>>>>>> case VIDIOC_QUERY_EXT_CTRL:
>>>>>> + if (!vfh->ctrl_handler)
>>>>>> + return -ENOTTY;
>>>>>> return v4l2_query_ext_ctrl(vfh->ctrl_handler, arg);
>>>>>>
>>>>>> case VIDIOC_QUERYMENU:
>>>>>> + if (!vfh->ctrl_handler)
>>>>>> + return -ENOTTY;
>>>>>> return v4l2_querymenu(vfh->ctrl_handler, arg);
>>>>>>
>>>>>> case VIDIOC_G_CTRL:
>>>>>> + if (!vfh->ctrl_handler)
>>>>>> + return -ENOTTY;
>>>>>> return v4l2_g_ctrl(vfh->ctrl_handler, arg);
>>>>>>
>>>>>> case VIDIOC_S_CTRL:
>>>>>> + if (!vfh->ctrl_handler)
>>>>>> + return -ENOTTY;
>>>>>> return v4l2_s_ctrl(vfh, vfh->ctrl_handler, arg);
>>>>>>
>>>>>> case VIDIOC_G_EXT_CTRLS:
>>>>>> + if (!vfh->ctrl_handler)
>>>>>> + return -ENOTTY;
>>>>>> return v4l2_g_ext_ctrls(vfh->ctrl_handler, arg);
>>>>>>
>>>>>> case VIDIOC_S_EXT_CTRLS:
>>>>>> + if (!vfh->ctrl_handler)
>>>>>> + return -ENOTTY;
>>>>>> return v4l2_s_ext_ctrls(vfh, vfh->ctrl_handler, arg);
>>>>>>
>>>>>> case VIDIOC_TRY_EXT_CTRLS:
>>>>>> + if (!vfh->ctrl_handler)
>>>>>> + return -ENOTTY;
>>>>>> return v4l2_try_ext_ctrls(vfh->ctrl_handler, arg);
>>>>>>
>>>>>> case VIDIOC_DQEVENT:
>>>>>
>>>>
>>>
>>
>
