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.
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:
>>>
>>
>
