| Message ID | 20180208083655.32248-5-hverkuil@xs4all.nl (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Hans Verkuil |
| Headers |
Received: from vger.kernel.org ([209.132.180.67]) by www.linuxtv.org with esmtp (Exim 4.84_2) (envelope-from <linux-media-owner@vger.kernel.org>) id 1ejhhv-0002tI-Ge; Thu, 08 Feb 2018 08:37:27 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752117AbeBHIhU (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Thu, 8 Feb 2018 03:37:20 -0500 Received: from lb1-smtp-cloud9.xs4all.net ([194.109.24.22]:49001 "EHLO lb1-smtp-cloud9.xs4all.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752013AbeBHIhB (ORCPT <rfc822;linux-media@vger.kernel.org>); Thu, 8 Feb 2018 03:37:01 -0500 Received: from tschai.fritz.box ([212.251.195.8]) by smtp-cloud9.xs4all.net with ESMTPA id jhhQefdbboWCOjhhUe2xdl; Thu, 08 Feb 2018 09:37:00 +0100 From: Hans Verkuil <hverkuil@xs4all.nl> To: linux-media@vger.kernel.org Cc: Hans Verkuil <hans.verkuil@cisco.com> Subject: [PATCHv2 04/15] v4l2-subdev: without controls return -ENOTTY Date: Thu, 8 Feb 2018 09:36:44 +0100 Message-Id: <20180208083655.32248-5-hverkuil@xs4all.nl> X-Mailer: git-send-email 2.15.1 In-Reply-To: <20180208083655.32248-1-hverkuil@xs4all.nl> References: <20180208083655.32248-1-hverkuil@xs4all.nl> X-CMAE-Envelope: MS4wfPUUkQxwJ8zH11igOMvWmSaMO1RsH7FHf0Z0l/7/eaJ2c6EnZu1S9dqRTSSRxmaHuNuduVseQB/49tWij8mT8ZZiZmJfl/XLnaOFNK17JtKia41EggKh 10/Y2pL/zprq2CCcpNM0dD+zDANjzgQXJEf2B7g21IwmInS81bcLcxr8cKGCBMqSI6Jq5zixZqpqJc2G6iFdbanrne+JQhVFrTU= Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org |
Commit Message
Hans Verkuil
Feb. 8, 2018, 8:36 a.m. UTC
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@cisco.com>
---
drivers/media/v4l2-core/v4l2-subdev.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
Comments
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@cisco.com> 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? > --- > 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:
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@cisco.com> > > 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. 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: >
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@cisco.com> > > > > 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? 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: > > >
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@cisco.com> >>> >>> 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: >>> >> >
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@cisco.com> > >>> > >>> 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@linux.intel.com> > > 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: > >>> > >> > > >
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@cisco.com> >>>>> >>>>> 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@linux.intel.com> > >> >> 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: >>>>> >>>> >>> >> >
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: