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?
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:
> >>>
> >>
> >
>
--
Sakari Ailus
e-mail: sakari.ailus@xxxxxx
