| Message ID | 1348758980-21683-2-git-send-email-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Hans, On Thursday 27 September 2012 17:16:15 Laurent Pinchart wrote: > When one of the requested controls doesn't exist the error_idx field > must reflect that situation. For G_EXT_CTRLS and S_EXT_CTRLS, error_idx > must be set to the control count. For TRY_EXT_CTRLS, it must be set to > the index of the unexisting control. > > This issue was found by the v4l2-compliance tool. I'm revisiting this patch as it has been reverted in v3.8-rc1. > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/usb/uvc/uvc_ctrl.c | 17 ++++++++++------- > drivers/media/usb/uvc/uvc_v4l2.c | 19 ++++++++++++------- > 2 files changed, 22 insertions(+), 14 deletions(-) [snip] > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c > b/drivers/media/usb/uvc/uvc_v4l2.c index f00db30..e5817b9 100644 > --- a/drivers/media/usb/uvc/uvc_v4l2.c > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > @@ -591,8 +591,10 @@ static long uvc_v4l2_do_ioctl(struct file *file, [snip] > @@ -637,8 +639,9 @@ static long uvc_v4l2_do_ioctl(struct file *file, > unsigned int cmd, void *arg) ret = uvc_ctrl_get(chain, ctrl); > if (ret < 0) { > uvc_ctrl_rollback(handle); > - ctrls->error_idx = i; > - return ret; > + ctrls->error_idx = ret == -ENOENT > + ? ctrls->count : i; > + return ret == -ENOENT ? -EINVAL : ret; > } > } > ctrls->error_idx = 0; > @@ -661,8 +664,10 @@ static long uvc_v4l2_do_ioctl(struct file *file, > unsigned int cmd, void *arg) ret = uvc_ctrl_set(chain, ctrl); > if (ret < 0) { > uvc_ctrl_rollback(handle); > - ctrls->error_idx = i; > - return ret; > + ctrls->error_idx = (ret == -ENOENT && > + cmd == VIDIOC_S_EXT_CTRLS) > + ? ctrls->count : i; > + return ret == -ENOENT ? -EINVAL : ret; > } > } I've reread the V4L2 specification, and the least I can say is that the text is pretty ambiguous. Let's clarify it. Is there a reason to differentiate between invalid control IDs and other errors as far as error_idx is concerned ? It would be simpler if error_idx was set to the index of the first error for get and try operations, regardless of the error type. What do you think ?
On Mon December 24 2012 13:27:08 Laurent Pinchart wrote: > Hi Hans, > > On Thursday 27 September 2012 17:16:15 Laurent Pinchart wrote: > > When one of the requested controls doesn't exist the error_idx field > > must reflect that situation. For G_EXT_CTRLS and S_EXT_CTRLS, error_idx > > must be set to the control count. For TRY_EXT_CTRLS, it must be set to > > the index of the unexisting control. > > > > This issue was found by the v4l2-compliance tool. > > I'm revisiting this patch as it has been reverted in v3.8-rc1. > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > drivers/media/usb/uvc/uvc_ctrl.c | 17 ++++++++++------- > > drivers/media/usb/uvc/uvc_v4l2.c | 19 ++++++++++++------- > > 2 files changed, 22 insertions(+), 14 deletions(-) > > [snip] > > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c > > b/drivers/media/usb/uvc/uvc_v4l2.c index f00db30..e5817b9 100644 > > --- a/drivers/media/usb/uvc/uvc_v4l2.c > > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > > @@ -591,8 +591,10 @@ static long uvc_v4l2_do_ioctl(struct file *file, > > [snip] > > > @@ -637,8 +639,9 @@ static long uvc_v4l2_do_ioctl(struct file *file, > > unsigned int cmd, void *arg) ret = uvc_ctrl_get(chain, ctrl); > > if (ret < 0) { > > uvc_ctrl_rollback(handle); > > - ctrls->error_idx = i; > > - return ret; > > + ctrls->error_idx = ret == -ENOENT > > + ? ctrls->count : i; > > + return ret == -ENOENT ? -EINVAL : ret; > > } > > } > > ctrls->error_idx = 0; > > @@ -661,8 +664,10 @@ static long uvc_v4l2_do_ioctl(struct file *file, > > unsigned int cmd, void *arg) ret = uvc_ctrl_set(chain, ctrl); > > if (ret < 0) { > > uvc_ctrl_rollback(handle); > > - ctrls->error_idx = i; > > - return ret; > > + ctrls->error_idx = (ret == -ENOENT && > > + cmd == VIDIOC_S_EXT_CTRLS) > > + ? ctrls->count : i; > > + return ret == -ENOENT ? -EINVAL : ret; > > } > > } > > I've reread the V4L2 specification, and the least I can say is that the text > is pretty ambiguous. Let's clarify it. > > Is there a reason to differentiate between invalid control IDs and other > errors as far as error_idx is concerned ? It would be simpler if error_idx was > set to the index of the first error for get and try operations, regardless of > the error type. What do you think ? There is a good reason for doing this: the G/S_EXT_CTRLS ioctls have to be as atomic as possible, i.e. it should try hard to prevent leaving the hardware in an inconsistent state because not all controls could be set. It can never be fully atomic since writing multiple registers over usb or i2c can always return errors for one of those writes, but it should certainly check for all the obvious errors first that do not require actually writing to the hardware, such as whether all the controls in the control list actually exist. And for such errors error_idx should be set to the number of controls to indicate that none of the controls were actually set but that there was a problem with the list of controls itself. Since TRY_EXT_CTRLS doesn't touch the hardware at all the error_idx can be set to the index of the control that caused the problem. The documentation can definitely be improved as the difference between G/S and TRY with regards to error_idx is not made explicit. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, On Tuesday 25 December 2012 12:15:25 Hans Verkuil wrote: > On Mon December 24 2012 13:27:08 Laurent Pinchart wrote: > > On Thursday 27 September 2012 17:16:15 Laurent Pinchart wrote: > > > When one of the requested controls doesn't exist the error_idx field > > > must reflect that situation. For G_EXT_CTRLS and S_EXT_CTRLS, error_idx > > > must be set to the control count. For TRY_EXT_CTRLS, it must be set to > > > the index of the unexisting control. > > > > > > This issue was found by the v4l2-compliance tool. > > > > I'm revisiting this patch as it has been reverted in v3.8-rc1. > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > > > > drivers/media/usb/uvc/uvc_ctrl.c | 17 ++++++++++------- > > > drivers/media/usb/uvc/uvc_v4l2.c | 19 ++++++++++++------- > > > 2 files changed, 22 insertions(+), 14 deletions(-) > > > > [snip] > > > > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c > > > b/drivers/media/usb/uvc/uvc_v4l2.c index f00db30..e5817b9 100644 > > > --- a/drivers/media/usb/uvc/uvc_v4l2.c > > > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > > > @@ -591,8 +591,10 @@ static long uvc_v4l2_do_ioctl(struct file *file, > > > > [snip] > > > > > @@ -637,8 +639,9 @@ static long uvc_v4l2_do_ioctl(struct file *file, > > > unsigned int cmd, void *arg) ret = uvc_ctrl_get(chain, ctrl); > > > > > > if (ret < 0) { > > > > > > uvc_ctrl_rollback(handle); > > > > > > - ctrls->error_idx = i; > > > - return ret; > > > + ctrls->error_idx = ret == -ENOENT > > > + ? ctrls->count : i; > > > + return ret == -ENOENT ? -EINVAL : ret; > > > > > > } > > > > > > } > > > ctrls->error_idx = 0; > > > > > > @@ -661,8 +664,10 @@ static long uvc_v4l2_do_ioctl(struct file *file, > > > unsigned int cmd, void *arg) ret = uvc_ctrl_set(chain, ctrl); > > > > > > if (ret < 0) { > > > > > > uvc_ctrl_rollback(handle); > > > > > > - ctrls->error_idx = i; > > > - return ret; > > > + ctrls->error_idx = (ret == -ENOENT && > > > + cmd == VIDIOC_S_EXT_CTRLS) > > > + ? ctrls->count : i; > > > + return ret == -ENOENT ? -EINVAL : ret; > > > > > > } > > > > > > } > > > > I've reread the V4L2 specification, and the least I can say is that the > > text is pretty ambiguous. Let's clarify it. > > > > Is there a reason to differentiate between invalid control IDs and other > > errors as far as error_idx is concerned ? It would be simpler if error_idx > > was set to the index of the first error for get and try operations, > > regardless of the error type. What do you think ? > > There is a good reason for doing this: the G/S_EXT_CTRLS ioctls have to be > as atomic as possible, i.e. it should try hard to prevent leaving the > hardware in an inconsistent state because not all controls could be set. It > can never be fully atomic since writing multiple registers over usb or i2c > can always return errors for one of those writes, but it should certainly > check for all the obvious errors first that do not require actually writing > to the hardware, such as whether all the controls in the control list > actually exist. > > And for such errors error_idx should be set to the number of controls to > indicate that none of the controls were actually set but that there was a > problem with the list of controls itself. For S_EXT_CTRLS, sure, but G_EXT_CTRLS doesn't modify the hardware state, so it could get all controls up to the erroneous one. > Since TRY_EXT_CTRLS doesn't touch the hardware at all the error_idx can be > set to the index of the control that caused the problem. > > The documentation can definitely be improved as the difference between G/S > and TRY with regards to error_idx is not made explicit.
On Tue December 25 2012 12:23:00 Laurent Pinchart wrote: > Hi Hans, > > On Tuesday 25 December 2012 12:15:25 Hans Verkuil wrote: > > On Mon December 24 2012 13:27:08 Laurent Pinchart wrote: > > > On Thursday 27 September 2012 17:16:15 Laurent Pinchart wrote: > > > > When one of the requested controls doesn't exist the error_idx field > > > > must reflect that situation. For G_EXT_CTRLS and S_EXT_CTRLS, error_idx > > > > must be set to the control count. For TRY_EXT_CTRLS, it must be set to > > > > the index of the unexisting control. > > > > > > > > This issue was found by the v4l2-compliance tool. > > > > > > I'm revisiting this patch as it has been reverted in v3.8-rc1. > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > --- > > > > > > > > drivers/media/usb/uvc/uvc_ctrl.c | 17 ++++++++++------- > > > > drivers/media/usb/uvc/uvc_v4l2.c | 19 ++++++++++++------- > > > > 2 files changed, 22 insertions(+), 14 deletions(-) > > > > > > [snip] > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c > > > > b/drivers/media/usb/uvc/uvc_v4l2.c index f00db30..e5817b9 100644 > > > > --- a/drivers/media/usb/uvc/uvc_v4l2.c > > > > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > > > > @@ -591,8 +591,10 @@ static long uvc_v4l2_do_ioctl(struct file *file, > > > > > > [snip] > > > > > > > @@ -637,8 +639,9 @@ static long uvc_v4l2_do_ioctl(struct file *file, > > > > unsigned int cmd, void *arg) ret = uvc_ctrl_get(chain, ctrl); > > > > > > > > if (ret < 0) { > > > > > > > > uvc_ctrl_rollback(handle); > > > > > > > > - ctrls->error_idx = i; > > > > - return ret; > > > > + ctrls->error_idx = ret == -ENOENT > > > > + ? ctrls->count : i; > > > > + return ret == -ENOENT ? -EINVAL : ret; > > > > > > > > } > > > > > > > > } > > > > ctrls->error_idx = 0; > > > > > > > > @@ -661,8 +664,10 @@ static long uvc_v4l2_do_ioctl(struct file *file, > > > > unsigned int cmd, void *arg) ret = uvc_ctrl_set(chain, ctrl); > > > > > > > > if (ret < 0) { > > > > > > > > uvc_ctrl_rollback(handle); > > > > > > > > - ctrls->error_idx = i; > > > > - return ret; > > > > + ctrls->error_idx = (ret == -ENOENT && > > > > + cmd == VIDIOC_S_EXT_CTRLS) > > > > + ? ctrls->count : i; > > > > + return ret == -ENOENT ? -EINVAL : ret; > > > > > > > > } > > > > > > > > } > > > > > > I've reread the V4L2 specification, and the least I can say is that the > > > text is pretty ambiguous. Let's clarify it. > > > > > > Is there a reason to differentiate between invalid control IDs and other > > > errors as far as error_idx is concerned ? It would be simpler if error_idx > > > was set to the index of the first error for get and try operations, > > > regardless of the error type. What do you think ? > > > > There is a good reason for doing this: the G/S_EXT_CTRLS ioctls have to be > > as atomic as possible, i.e. it should try hard to prevent leaving the > > hardware in an inconsistent state because not all controls could be set. It > > can never be fully atomic since writing multiple registers over usb or i2c > > can always return errors for one of those writes, but it should certainly > > check for all the obvious errors first that do not require actually writing > > to the hardware, such as whether all the controls in the control list > > actually exist. > > > > And for such errors error_idx should be set to the number of controls to > > indicate that none of the controls were actually set but that there was a > > problem with the list of controls itself. > > For S_EXT_CTRLS, sure, but G_EXT_CTRLS doesn't modify the hardware state, so > it could get all controls up to the erroneous one. I have thought about that but I decided against it. One reason is to have get and set behave the same since both access the hardware. The other reason is that even getting a control value might change the hardware state, for example by resetting some internal hardware counter when a register is read (it's rare but there is hardware like that). Furthermore, reading hardware registers can be slow so why not do the sanity check first? Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, On Tuesday 25 December 2012 12:50:51 Hans Verkuil wrote: > On Tue December 25 2012 12:23:00 Laurent Pinchart wrote: > > On Tuesday 25 December 2012 12:15:25 Hans Verkuil wrote: > > > On Mon December 24 2012 13:27:08 Laurent Pinchart wrote: > > > > On Thursday 27 September 2012 17:16:15 Laurent Pinchart wrote: > > > > > When one of the requested controls doesn't exist the error_idx field > > > > > must reflect that situation. For G_EXT_CTRLS and S_EXT_CTRLS, > > > > > error_idx must be set to the control count. For TRY_EXT_CTRLS, it > > > > > must be set to the index of the unexisting control. > > > > > > > > > > This issue was found by the v4l2-compliance tool. > > > > > > > > I'm revisiting this patch as it has been reverted in v3.8-rc1. > > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > --- > > > > > > > > > > drivers/media/usb/uvc/uvc_ctrl.c | 17 ++++++++++------- > > > > > drivers/media/usb/uvc/uvc_v4l2.c | 19 ++++++++++++------- > > > > > 2 files changed, 22 insertions(+), 14 deletions(-) > > > > > > > > [snip] > > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c > > > > > b/drivers/media/usb/uvc/uvc_v4l2.c index f00db30..e5817b9 100644 > > > > > --- a/drivers/media/usb/uvc/uvc_v4l2.c > > > > > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > > > > > @@ -591,8 +591,10 @@ static long uvc_v4l2_do_ioctl(struct file > > > > > *file, > > > > > > > > [snip] > > > > > > > > > @@ -637,8 +639,9 @@ static long uvc_v4l2_do_ioctl(struct file *file, > > > > > unsigned int cmd, void *arg) ret = uvc_ctrl_get(chain, ctrl); > > > > > if (ret < 0) { > > > > > uvc_ctrl_rollback(handle); > > > > > > > > > > - ctrls->error_idx = i; > > > > > - return ret; > > > > > + ctrls->error_idx = ret == -ENOENT > > > > > + ? ctrls->count : i; > > > > > + return ret == -ENOENT ? -EINVAL : ret; > > > > > } > > > > > } > > > > > ctrls->error_idx = 0; > > > > > @@ -661,8 +664,10 @@ static long uvc_v4l2_do_ioctl(struct file > > > > > *file, > > > > > unsigned int cmd, void *arg) ret = uvc_ctrl_set(chain, ctrl); > > > > > if (ret < 0) { > > > > > uvc_ctrl_rollback(handle); > > > > > > > > > > - ctrls->error_idx = i; > > > > > - return ret; > > > > > + ctrls->error_idx = (ret == -ENOENT && > > > > > + cmd == VIDIOC_S_EXT_CTRLS) > > > > > + ? ctrls->count : i; > > > > > + return ret == -ENOENT ? -EINVAL : ret; > > > > > } > > > > > } > > > > > > > > I've reread the V4L2 specification, and the least I can say is that > > > > the text is pretty ambiguous. Let's clarify it. > > > > > > > > Is there a reason to differentiate between invalid control IDs and > > > > other errors as far as error_idx is concerned ? It would be simpler if > > > > error_idx was set to the index of the first error for get and try > > > > operations, regardless of the error type. What do you think ? > > > > > > There is a good reason for doing this: the G/S_EXT_CTRLS ioctls have to > > > be as atomic as possible, i.e. it should try hard to prevent leaving the > > > hardware in an inconsistent state because not all controls could be set. > > > It can never be fully atomic since writing multiple registers over usb > > > or i2c can always return errors for one of those writes, but it should > > > certainly check for all the obvious errors first that do not require > > > actually writing to the hardware, such as whether all the controls in > > > the control list actually exist. > > > > > > And for such errors error_idx should be set to the number of controls to > > > indicate that none of the controls were actually set but that there was > > > a problem with the list of controls itself. > > > > For S_EXT_CTRLS, sure, but G_EXT_CTRLS doesn't modify the hardware state, > > so it could get all controls up to the erroneous one. > > I have thought about that but I decided against it. One reason is to have > get and set behave the same since both access the hardware. The other > reason is that even getting a control value might change the hardware > state, for example by resetting some internal hardware counter when a > register is read (it's rare but there is hardware like that). Furthermore, > reading hardware registers can be slow so why not do the sanity check > first? Get can indeed change the device state in rare cases, but the information won't be lost, as the value of all controls before error_idx will be returned. What bothers me with the current G_EXT_CTRLS implementation (beside that it's very slightly more complex for the uvcvideo driver than the one I propose) is that an application will have no way to know for which control G_EXT_CTRLS failed. This is especially annoying during development. Maybe we could leave this behaviour as driver-specific ?
Em Wed, 26 Dec 2012 12:33:58 +0100 Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu: > Hi Hans, > > On Tuesday 25 December 2012 12:50:51 Hans Verkuil wrote: > > On Tue December 25 2012 12:23:00 Laurent Pinchart wrote: > > > On Tuesday 25 December 2012 12:15:25 Hans Verkuil wrote: > > > > On Mon December 24 2012 13:27:08 Laurent Pinchart wrote: > > > > > On Thursday 27 September 2012 17:16:15 Laurent Pinchart wrote: > > > > > > When one of the requested controls doesn't exist the error_idx field > > > > > > must reflect that situation. For G_EXT_CTRLS and S_EXT_CTRLS, > > > > > > error_idx must be set to the control count. For TRY_EXT_CTRLS, it > > > > > > must be set to the index of the unexisting control. > > > > > > > > > > > > This issue was found by the v4l2-compliance tool. > > > > > > > > > > I'm revisiting this patch as it has been reverted in v3.8-rc1. > > > > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > --- > > > > > > > > > > > > drivers/media/usb/uvc/uvc_ctrl.c | 17 ++++++++++------- > > > > > > drivers/media/usb/uvc/uvc_v4l2.c | 19 ++++++++++++------- > > > > > > 2 files changed, 22 insertions(+), 14 deletions(-) > > > > > > > > > > [snip] > > > > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c > > > > > > b/drivers/media/usb/uvc/uvc_v4l2.c index f00db30..e5817b9 100644 > > > > > > --- a/drivers/media/usb/uvc/uvc_v4l2.c > > > > > > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > > > > > > @@ -591,8 +591,10 @@ static long uvc_v4l2_do_ioctl(struct file > > > > > > *file, > > > > > > > > > > [snip] > > > > > > > > > > > @@ -637,8 +639,9 @@ static long uvc_v4l2_do_ioctl(struct file *file, > > > > > > unsigned int cmd, void *arg) ret = uvc_ctrl_get(chain, ctrl); > > > > > > if (ret < 0) { > > > > > > uvc_ctrl_rollback(handle); > > > > > > > > > > > > - ctrls->error_idx = i; > > > > > > - return ret; > > > > > > + ctrls->error_idx = ret == -ENOENT > > > > > > + ? ctrls->count : i; > > > > > > + return ret == -ENOENT ? -EINVAL : ret; > > > > > > } > > > > > > } > > > > > > ctrls->error_idx = 0; > > > > > > @@ -661,8 +664,10 @@ static long uvc_v4l2_do_ioctl(struct file > > > > > > *file, > > > > > > unsigned int cmd, void *arg) ret = uvc_ctrl_set(chain, ctrl); > > > > > > if (ret < 0) { > > > > > > uvc_ctrl_rollback(handle); > > > > > > > > > > > > - ctrls->error_idx = i; > > > > > > - return ret; > > > > > > + ctrls->error_idx = (ret == -ENOENT && > > > > > > + cmd == VIDIOC_S_EXT_CTRLS) > > > > > > + ? ctrls->count : i; > > > > > > + return ret == -ENOENT ? -EINVAL : ret; > > > > > > } > > > > > > } > > > > > > > > > > I've reread the V4L2 specification, and the least I can say is that > > > > > the text is pretty ambiguous. Let's clarify it. > > > > > > > > > > Is there a reason to differentiate between invalid control IDs and > > > > > other errors as far as error_idx is concerned ? It would be simpler if > > > > > error_idx was set to the index of the first error for get and try > > > > > operations, regardless of the error type. What do you think ? > > > > > > > > There is a good reason for doing this: the G/S_EXT_CTRLS ioctls have to > > > > be as atomic as possible, i.e. it should try hard to prevent leaving the > > > > hardware in an inconsistent state because not all controls could be set. > > > > It can never be fully atomic since writing multiple registers over usb > > > > or i2c can always return errors for one of those writes, but it should > > > > certainly check for all the obvious errors first that do not require > > > > actually writing to the hardware, such as whether all the controls in > > > > the control list actually exist. > > > > > > > > And for such errors error_idx should be set to the number of controls to > > > > indicate that none of the controls were actually set but that there was > > > > a problem with the list of controls itself. > > > > > > For S_EXT_CTRLS, sure, but G_EXT_CTRLS doesn't modify the hardware state, > > > so it could get all controls up to the erroneous one. > > > > I have thought about that but I decided against it. One reason is to have > > get and set behave the same since both access the hardware. The other > > reason is that even getting a control value might change the hardware > > state, for example by resetting some internal hardware counter when a > > register is read (it's rare but there is hardware like that). Furthermore, > > reading hardware registers can be slow so why not do the sanity check > > first? > > Get can indeed change the device state in rare cases, but the information > won't be lost, as the value of all controls before error_idx will be returned. Huh? reading a control should never alter the device's state. If the hardware is resetting a register, then such register should be shadowed, and some other way to explicitly reset its value should be used. > What bothers me with the current G_EXT_CTRLS implementation (beside that it's > very slightly more complex for the uvcvideo driver than the one I propose) is > that an application will have no way to know for which control G_EXT_CTRLS > failed. This is especially annoying during development. > > Maybe we could leave this behaviour as driver-specific ? driver-specific behavior for IOCTL's should be avoided, as applications will fail if they see something it doesn't expect.
Hi Mauro, On Wednesday 26 December 2012 12:00:35 Mauro Carvalho Chehab wrote: > Em Wed, 26 Dec 2012 12:33:58 +0100 Laurent Pinchart escreveu: > > On Tuesday 25 December 2012 12:50:51 Hans Verkuil wrote: > > > On Tue December 25 2012 12:23:00 Laurent Pinchart wrote: > > > > On Tuesday 25 December 2012 12:15:25 Hans Verkuil wrote: > > > > > On Mon December 24 2012 13:27:08 Laurent Pinchart wrote: > > > > > > On Thursday 27 September 2012 17:16:15 Laurent Pinchart wrote: > > > > > > > When one of the requested controls doesn't exist the error_idx > > > > > > > field must reflect that situation. For G_EXT_CTRLS and > > > > > > > S_EXT_CTRLS, error_idx must be set to the control count. For > > > > > > > TRY_EXT_CTRLS, it must be set to the index of the unexisting > > > > > > > control. > > > > > > > > > > > > > > This issue was found by the v4l2-compliance tool. > > > > > > > > > > > > I'm revisiting this patch as it has been reverted in v3.8-rc1. > > > > > > > > > > > > > Signed-off-by: Laurent Pinchart > > > > > > > <laurent.pinchart@ideasonboard.com> > > > > > > > --- > > > > > > > > > > > > > > drivers/media/usb/uvc/uvc_ctrl.c | 17 ++++++++++------- > > > > > > > drivers/media/usb/uvc/uvc_v4l2.c | 19 ++++++++++++------- > > > > > > > 2 files changed, 22 insertions(+), 14 deletions(-) > > > > > > > > > > > > [snip] > > > > > > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c > > > > > > > b/drivers/media/usb/uvc/uvc_v4l2.c index f00db30..e5817b9 100644 > > > > > > > --- a/drivers/media/usb/uvc/uvc_v4l2.c > > > > > > > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > > > > > > > @@ -591,8 +591,10 @@ static long uvc_v4l2_do_ioctl(struct file > > > > > > > *file, > > > > > > > > > > > > [snip] > > > > > > > > > > > > > @@ -637,8 +639,9 @@ static long uvc_v4l2_do_ioctl(struct file > > > > > > > *file, > > > > > > > unsigned int cmd, void *arg) ret = uvc_ctrl_get(chain, ctrl); > > > > > > > > > > > > > > if (ret < 0) { > > > > > > > > > > > > > > uvc_ctrl_rollback(handle); > > > > > > > > > > > > > > - ctrls->error_idx = i; > > > > > > > - return ret; > > > > > > > + ctrls->error_idx = ret == -ENOENT > > > > > > > + ? ctrls->count : i; > > > > > > > + return ret == -ENOENT ? -EINVAL : ret; > > > > > > > > > > > > > > } > > > > > > > > > > > > > > } > > > > > > > ctrls->error_idx = 0; > > > > > > > > > > > > > > @@ -661,8 +664,10 @@ static long uvc_v4l2_do_ioctl(struct file > > > > > > > *file, > > > > > > > unsigned int cmd, void *arg) ret = uvc_ctrl_set(chain, ctrl); > > > > > > > > > > > > > > if (ret < 0) { > > > > > > > > > > > > > > uvc_ctrl_rollback(handle); > > > > > > > > > > > > > > - ctrls->error_idx = i; > > > > > > > - return ret; > > > > > > > + ctrls->error_idx = (ret == -ENOENT && > > > > > > > + cmd == VIDIOC_S_EXT_CTRLS) > > > > > > > + ? ctrls->count : i; > > > > > > > + return ret == -ENOENT ? -EINVAL : ret; > > > > > > > > > > > > > > } > > > > > > > > > > > > > > } > > > > > > > > > > > > I've reread the V4L2 specification, and the least I can say is > > > > > > that the text is pretty ambiguous. Let's clarify it. > > > > > > > > > > > > Is there a reason to differentiate between invalid control IDs and > > > > > > other errors as far as error_idx is concerned ? It would be > > > > > > simpler if error_idx was set to the index of the first error for > > > > > > get and try operations, regardless of the error type. What do you > > > > > > think ? > > > > > > > > > > There is a good reason for doing this: the G/S_EXT_CTRLS ioctls have > > > > > to be as atomic as possible, i.e. it should try hard to prevent > > > > > leaving the hardware in an inconsistent state because not all > > > > > controls could be set. It can never be fully atomic since writing > > > > > multiple registers over usb or i2c can always return errors for one > > > > > of those writes, but it should certainly check for all the obvious > > > > > errors first that do not require actually writing to the hardware, > > > > > such as whether all the controls in the control list actually exist. > > > > > > > > > > And for such errors error_idx should be set to the number of > > > > > controls to indicate that none of the controls were actually set but > > > > > that there was a problem with the list of controls itself. > > > > > > > > For S_EXT_CTRLS, sure, but G_EXT_CTRLS doesn't modify the hardware > > > > state, so it could get all controls up to the erroneous one. > > > > > > I have thought about that but I decided against it. One reason is to > > > have get and set behave the same since both access the hardware. The > > > other reason is that even getting a control value might change the > > > hardware state, for example by resetting some internal hardware counter > > > when a register is read (it's rare but there is hardware like that). > > > Furthermore, reading hardware registers can be slow so why not do the > > > sanity check first? > > > > Get can indeed change the device state in rare cases, but the information > > won't be lost, as the value of all controls before error_idx will be > > returned. > > Huh? reading a control should never alter the device's state. If the > hardware is resetting a register, then such register should be shadowed, > and some other way to explicitly reset its value should be used. The hardware can expose a read-only counter in such a way that the counter is reset when read. That would be pretty rare for V4L devices though, I'm not aware of any such implementation in any of the devices we support. A common way to handle those registers is to turn then in software into a counter that is never reset, so we have a solution anyway (this is getting a bit out of scope). > > What bothers me with the current G_EXT_CTRLS implementation (beside that > > it's very slightly more complex for the uvcvideo driver than the one I > > propose) is that an application will have no way to know for which > > control G_EXT_CTRLS failed. This is especially annoying during > > development. > > > > Maybe we could leave this behaviour as driver-specific ? > > driver-specific behavior for IOCTL's should be avoided, as applications will > fail if they see something it doesn't expect. I'm not asking for an unspecified behaviour here, but for giving freedom to drivers to choose among the specified behaviours. The V4L specification explains that the error_idx can be set to the total number of controls, in which case no control was read or written, or to the index of the first erroneous control, in which case all controls before that index are read or written successfully. For get operations I believe that getting all controls up to the first error is the best behaviour (even when we can detect errors such as invalid control IDs up front), but I'm not opposed to drivers returning error_idx set to the total number of controls without getting any control.
On Wed December 26 2012 12:33:58 Laurent Pinchart wrote: > Hi Hans, > > On Tuesday 25 December 2012 12:50:51 Hans Verkuil wrote: > > On Tue December 25 2012 12:23:00 Laurent Pinchart wrote: > > > On Tuesday 25 December 2012 12:15:25 Hans Verkuil wrote: > > > > On Mon December 24 2012 13:27:08 Laurent Pinchart wrote: > > > > > On Thursday 27 September 2012 17:16:15 Laurent Pinchart wrote: > > > > > > When one of the requested controls doesn't exist the error_idx field > > > > > > must reflect that situation. For G_EXT_CTRLS and S_EXT_CTRLS, > > > > > > error_idx must be set to the control count. For TRY_EXT_CTRLS, it > > > > > > must be set to the index of the unexisting control. > > > > > > > > > > > > This issue was found by the v4l2-compliance tool. > > > > > > > > > > I'm revisiting this patch as it has been reverted in v3.8-rc1. > > > > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > --- > > > > > > > > > > > > drivers/media/usb/uvc/uvc_ctrl.c | 17 ++++++++++------- > > > > > > drivers/media/usb/uvc/uvc_v4l2.c | 19 ++++++++++++------- > > > > > > 2 files changed, 22 insertions(+), 14 deletions(-) > > > > > > > > > > [snip] > > > > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c > > > > > > b/drivers/media/usb/uvc/uvc_v4l2.c index f00db30..e5817b9 100644 > > > > > > --- a/drivers/media/usb/uvc/uvc_v4l2.c > > > > > > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > > > > > > @@ -591,8 +591,10 @@ static long uvc_v4l2_do_ioctl(struct file > > > > > > *file, > > > > > > > > > > [snip] > > > > > > > > > > > @@ -637,8 +639,9 @@ static long uvc_v4l2_do_ioctl(struct file *file, > > > > > > unsigned int cmd, void *arg) ret = uvc_ctrl_get(chain, ctrl); > > > > > > if (ret < 0) { > > > > > > uvc_ctrl_rollback(handle); > > > > > > > > > > > > - ctrls->error_idx = i; > > > > > > - return ret; > > > > > > + ctrls->error_idx = ret == -ENOENT > > > > > > + ? ctrls->count : i; > > > > > > + return ret == -ENOENT ? -EINVAL : ret; > > > > > > } > > > > > > } > > > > > > ctrls->error_idx = 0; > > > > > > @@ -661,8 +664,10 @@ static long uvc_v4l2_do_ioctl(struct file > > > > > > *file, > > > > > > unsigned int cmd, void *arg) ret = uvc_ctrl_set(chain, ctrl); > > > > > > if (ret < 0) { > > > > > > uvc_ctrl_rollback(handle); > > > > > > > > > > > > - ctrls->error_idx = i; > > > > > > - return ret; > > > > > > + ctrls->error_idx = (ret == -ENOENT && > > > > > > + cmd == VIDIOC_S_EXT_CTRLS) > > > > > > + ? ctrls->count : i; > > > > > > + return ret == -ENOENT ? -EINVAL : ret; > > > > > > } > > > > > > } > > > > > > > > > > I've reread the V4L2 specification, and the least I can say is that > > > > > the text is pretty ambiguous. Let's clarify it. > > > > > > > > > > Is there a reason to differentiate between invalid control IDs and > > > > > other errors as far as error_idx is concerned ? It would be simpler if > > > > > error_idx was set to the index of the first error for get and try > > > > > operations, regardless of the error type. What do you think ? > > > > > > > > There is a good reason for doing this: the G/S_EXT_CTRLS ioctls have to > > > > be as atomic as possible, i.e. it should try hard to prevent leaving the > > > > hardware in an inconsistent state because not all controls could be set. > > > > It can never be fully atomic since writing multiple registers over usb > > > > or i2c can always return errors for one of those writes, but it should > > > > certainly check for all the obvious errors first that do not require > > > > actually writing to the hardware, such as whether all the controls in > > > > the control list actually exist. > > > > > > > > And for such errors error_idx should be set to the number of controls to > > > > indicate that none of the controls were actually set but that there was > > > > a problem with the list of controls itself. > > > > > > For S_EXT_CTRLS, sure, but G_EXT_CTRLS doesn't modify the hardware state, > > > so it could get all controls up to the erroneous one. > > > > I have thought about that but I decided against it. One reason is to have > > get and set behave the same since both access the hardware. The other > > reason is that even getting a control value might change the hardware > > state, for example by resetting some internal hardware counter when a > > register is read (it's rare but there is hardware like that). Furthermore, > > reading hardware registers can be slow so why not do the sanity check > > first? > > Get can indeed change the device state in rare cases, but the information > won't be lost, as the value of all controls before error_idx will be returned. > > What bothers me with the current G_EXT_CTRLS implementation (beside that it's > very slightly more complex for the uvcvideo driver than the one I propose) is > that an application will have no way to know for which control G_EXT_CTRLS > failed. This is especially annoying during development. For S_EXT_CTRLS you can call TRY_EXT_CTRLS first to check which control failed, but you don't have that option for G_EXT_CTRLS. That's actually something I hadn't considered. > Maybe we could leave this behaviour as driver-specific ? I need to think about this some more. Is this urgent or can it wait until January 7th? I'm back at work by then. I am actually attempting to touch my computer as little as possible this vacation :-) Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, On Thursday 27 December 2012 12:59:15 Hans Verkuil wrote: > On Wed December 26 2012 12:33:58 Laurent Pinchart wrote: > > On Tuesday 25 December 2012 12:50:51 Hans Verkuil wrote: > > > On Tue December 25 2012 12:23:00 Laurent Pinchart wrote: > > > > On Tuesday 25 December 2012 12:15:25 Hans Verkuil wrote: > > > > > On Mon December 24 2012 13:27:08 Laurent Pinchart wrote: > > > > > > On Thursday 27 September 2012 17:16:15 Laurent Pinchart wrote: > > > > > > > When one of the requested controls doesn't exist the error_idx > > > > > > > field must reflect that situation. For G_EXT_CTRLS and > > > > > > > S_EXT_CTRLS, error_idx must be set to the control count. For > > > > > > > TRY_EXT_CTRLS, it must be set to the index of the unexisting > > > > > > > control. > > > > > > > > > > > > > > This issue was found by the v4l2-compliance tool. > > > > > > > > > > > > I'm revisiting this patch as it has been reverted in v3.8-rc1. > > > > > > > > > > > > > Signed-off-by: Laurent Pinchart > > > > > > > <laurent.pinchart@ideasonboard.com> > > > > > > > --- > > > > > > > > > > > > > > drivers/media/usb/uvc/uvc_ctrl.c | 17 ++++++++++------- > > > > > > > drivers/media/usb/uvc/uvc_v4l2.c | 19 ++++++++++++------- > > > > > > > 2 files changed, 22 insertions(+), 14 deletions(-) > > > > > > > > > > > > [snip] > > > > > > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c > > > > > > > b/drivers/media/usb/uvc/uvc_v4l2.c index f00db30..e5817b9 100644 > > > > > > > --- a/drivers/media/usb/uvc/uvc_v4l2.c > > > > > > > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > > > > > > > @@ -591,8 +591,10 @@ static long uvc_v4l2_do_ioctl(struct file > > > > > > > *file, > > > > > > > > > > > > [snip] > > > > > > > > > > > > > @@ -637,8 +639,9 @@ static long uvc_v4l2_do_ioctl(struct file > > > > > > > *file, > > > > > > > unsigned int cmd, void *arg) ret = uvc_ctrl_get(chain, ctrl); > > > > > > > > > > > > > > if (ret < 0) { > > > > > > > > > > > > > > uvc_ctrl_rollback(handle); > > > > > > > > > > > > > > - ctrls->error_idx = i; > > > > > > > - return ret; > > > > > > > + ctrls->error_idx = ret == -ENOENT > > > > > > > + ? ctrls->count : i; > > > > > > > + return ret == -ENOENT ? -EINVAL : ret; > > > > > > > > > > > > > > } > > > > > > > > > > > > > > } > > > > > > > ctrls->error_idx = 0; > > > > > > > > > > > > > > @@ -661,8 +664,10 @@ static long uvc_v4l2_do_ioctl(struct file > > > > > > > *file, > > > > > > > unsigned int cmd, void *arg) ret = uvc_ctrl_set(chain, ctrl); > > > > > > > > > > > > > > if (ret < 0) { > > > > > > > > > > > > > > uvc_ctrl_rollback(handle); > > > > > > > > > > > > > > - ctrls->error_idx = i; > > > > > > > - return ret; > > > > > > > + ctrls->error_idx = (ret == -ENOENT && > > > > > > > + cmd == VIDIOC_S_EXT_CTRLS) > > > > > > > + ? ctrls->count : i; > > > > > > > + return ret == -ENOENT ? -EINVAL : ret; > > > > > > > > > > > > > > } > > > > > > > > > > > > > > } > > > > > > > > > > > > I've reread the V4L2 specification, and the least I can say is > > > > > > that > > > > > > the text is pretty ambiguous. Let's clarify it. > > > > > > > > > > > > Is there a reason to differentiate between invalid control IDs and > > > > > > other errors as far as error_idx is concerned ? It would be > > > > > > simpler if error_idx was set to the index of the first error for > > > > > > get and try operations, regardless of the error type. What do you > > > > > > think ? > > > > > > > > > > There is a good reason for doing this: the G/S_EXT_CTRLS ioctls have > > > > > to be as atomic as possible, i.e. it should try hard to prevent > > > > > leaving the hardware in an inconsistent state because not all > > > > > controls could be set. It can never be fully atomic since writing > > > > > multiple registers over usb or i2c can always return errors for one > > > > > of those writes, but it should certainly check for all the obvious > > > > > errors first that do not require actually writing to the hardware, > > > > > such as whether all the controls in the control list actually exist. > > > > > > > > > > And for such errors error_idx should be set to the number of > > > > > controls to indicate that none of the controls were actually set but > > > > > that there was a problem with the list of controls itself. > > > > > > > > For S_EXT_CTRLS, sure, but G_EXT_CTRLS doesn't modify the hardware > > > > state, so it could get all controls up to the erroneous one. > > > > > > I have thought about that but I decided against it. One reason is to > > > have get and set behave the same since both access the hardware. The > > > other reason is that even getting a control value might change the > > > hardware state, for example by resetting some internal hardware counter > > > when a register is read (it's rare but there is hardware like that). > > > Furthermore, reading hardware registers can be slow so why not do the > > > sanity check first? > > > > Get can indeed change the device state in rare cases, but the information > > won't be lost, as the value of all controls before error_idx will be > > returned. > > > > What bothers me with the current G_EXT_CTRLS implementation (beside that > > it's very slightly more complex for the uvcvideo driver than the one I > > propose) is that an application will have no way to know for which > > control G_EXT_CTRLS failed. This is especially annoying during > > development. > > For S_EXT_CTRLS you can call TRY_EXT_CTRLS first to check which control > failed, but you don't have that option for G_EXT_CTRLS. That's actually > something I hadn't considered. > > > Maybe we could leave this behaviour as driver-specific ? > > I need to think about this some more. Is this urgent or can it wait until > January 7th? I'm back at work by then. I am actually attempting to touch my > computer as little as possible this vacation :-) There's a v3.8 related regression in uvcvideo that I need to fix, but that can certainly wait until January the 7th. Enjoy your holidays and get away from the keyboard now :-)
On Thu December 27 2012 12:59:15 Hans Verkuil wrote: > On Wed December 26 2012 12:33:58 Laurent Pinchart wrote: > > Hi Hans, > > > > On Tuesday 25 December 2012 12:50:51 Hans Verkuil wrote: > > > On Tue December 25 2012 12:23:00 Laurent Pinchart wrote: > > > > On Tuesday 25 December 2012 12:15:25 Hans Verkuil wrote: > > > > > On Mon December 24 2012 13:27:08 Laurent Pinchart wrote: > > > > > > On Thursday 27 September 2012 17:16:15 Laurent Pinchart wrote: > > > > > > > When one of the requested controls doesn't exist the error_idx field > > > > > > > must reflect that situation. For G_EXT_CTRLS and S_EXT_CTRLS, > > > > > > > error_idx must be set to the control count. For TRY_EXT_CTRLS, it > > > > > > > must be set to the index of the unexisting control. > > > > > > > > > > > > > > This issue was found by the v4l2-compliance tool. > > > > > > > > > > > > I'm revisiting this patch as it has been reverted in v3.8-rc1. > > > > > > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > --- > > > > > > > > > > > > > > drivers/media/usb/uvc/uvc_ctrl.c | 17 ++++++++++------- > > > > > > > drivers/media/usb/uvc/uvc_v4l2.c | 19 ++++++++++++------- > > > > > > > 2 files changed, 22 insertions(+), 14 deletions(-) > > > > > > > > > > > > [snip] > > > > > > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c > > > > > > > b/drivers/media/usb/uvc/uvc_v4l2.c index f00db30..e5817b9 100644 > > > > > > > --- a/drivers/media/usb/uvc/uvc_v4l2.c > > > > > > > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > > > > > > > @@ -591,8 +591,10 @@ static long uvc_v4l2_do_ioctl(struct file > > > > > > > *file, > > > > > > > > > > > > [snip] > > > > > > > > > > > > > @@ -637,8 +639,9 @@ static long uvc_v4l2_do_ioctl(struct file *file, > > > > > > > unsigned int cmd, void *arg) ret = uvc_ctrl_get(chain, ctrl); > > > > > > > if (ret < 0) { > > > > > > > uvc_ctrl_rollback(handle); > > > > > > > > > > > > > > - ctrls->error_idx = i; > > > > > > > - return ret; > > > > > > > + ctrls->error_idx = ret == -ENOENT > > > > > > > + ? ctrls->count : i; > > > > > > > + return ret == -ENOENT ? -EINVAL : ret; > > > > > > > } > > > > > > > } > > > > > > > ctrls->error_idx = 0; > > > > > > > @@ -661,8 +664,10 @@ static long uvc_v4l2_do_ioctl(struct file > > > > > > > *file, > > > > > > > unsigned int cmd, void *arg) ret = uvc_ctrl_set(chain, ctrl); > > > > > > > if (ret < 0) { > > > > > > > uvc_ctrl_rollback(handle); > > > > > > > > > > > > > > - ctrls->error_idx = i; > > > > > > > - return ret; > > > > > > > + ctrls->error_idx = (ret == -ENOENT && > > > > > > > + cmd == VIDIOC_S_EXT_CTRLS) > > > > > > > + ? ctrls->count : i; > > > > > > > + return ret == -ENOENT ? -EINVAL : ret; > > > > > > > } > > > > > > > } > > > > > > > > > > > > I've reread the V4L2 specification, and the least I can say is that > > > > > > the text is pretty ambiguous. Let's clarify it. > > > > > > > > > > > > Is there a reason to differentiate between invalid control IDs and > > > > > > other errors as far as error_idx is concerned ? It would be simpler if > > > > > > error_idx was set to the index of the first error for get and try > > > > > > operations, regardless of the error type. What do you think ? > > > > > > > > > > There is a good reason for doing this: the G/S_EXT_CTRLS ioctls have to > > > > > be as atomic as possible, i.e. it should try hard to prevent leaving the > > > > > hardware in an inconsistent state because not all controls could be set. > > > > > It can never be fully atomic since writing multiple registers over usb > > > > > or i2c can always return errors for one of those writes, but it should > > > > > certainly check for all the obvious errors first that do not require > > > > > actually writing to the hardware, such as whether all the controls in > > > > > the control list actually exist. > > > > > > > > > > And for such errors error_idx should be set to the number of controls to > > > > > indicate that none of the controls were actually set but that there was > > > > > a problem with the list of controls itself. > > > > > > > > For S_EXT_CTRLS, sure, but G_EXT_CTRLS doesn't modify the hardware state, > > > > so it could get all controls up to the erroneous one. > > > > > > I have thought about that but I decided against it. One reason is to have > > > get and set behave the same since both access the hardware. The other > > > reason is that even getting a control value might change the hardware > > > state, for example by resetting some internal hardware counter when a > > > register is read (it's rare but there is hardware like that). Furthermore, > > > reading hardware registers can be slow so why not do the sanity check > > > first? > > > > Get can indeed change the device state in rare cases, but the information > > won't be lost, as the value of all controls before error_idx will be returned. > > > > What bothers me with the current G_EXT_CTRLS implementation (beside that it's > > very slightly more complex for the uvcvideo driver than the one I propose) is > > that an application will have no way to know for which control G_EXT_CTRLS > > failed. This is especially annoying during development. > > For S_EXT_CTRLS you can call TRY_EXT_CTRLS first to check which control failed, > but you don't have that option for G_EXT_CTRLS. That's actually something I > hadn't considered. > > > Maybe we could leave this behaviour as driver-specific ? I don't like the idea of leaving this driver-specific. That always bites you in the end. I thought seriously about changing the spec so G_EXT_CTRLS behaves like TRY_EXT_CTRLS when it comes to setting error_idx, but I decided against that. The main reason is that if G_EXT_CTRLS returns an error_idx < count, then you no longer know whether the values of the controls up to error_idx-1 were actually retrieved or not. v4l2-ctrls.c does sanity checks up front, so if it returns an error for control index 2, does that mean that the sanity checks failed in which case no controls were retrieved yet, or that getting control index 2 failed due to some hardware-related problem, in which case controls 0 and 1 *were* successfully retrieved. In addition, changing the behavior means changing the API, albeit in a very minor way, and I don't think it is worth doing that in this case. I will prepare a patch that clarifies the API, though. It can certainly be improved. Also note that I agree that the situation is not ideal and if I would write the API from scratch I'd probably handle this a bit differently, most likely by adding some flags field that can be used to give more precise information. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index f7061a5..7879d30 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1061,7 +1061,7 @@ int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, ctrl = uvc_find_control(chain, v4l2_ctrl->id, &mapping); if (ctrl == NULL) { - ret = -EINVAL; + ret = -ENOENT; goto done; } @@ -1099,12 +1099,13 @@ int uvc_query_v4l2_menu(struct uvc_video_chain *chain, return -ERESTARTSYS; ctrl = uvc_find_control(chain, query_menu->id, &mapping); - if (ctrl == NULL || mapping->v4l2_type != V4L2_CTRL_TYPE_MENU) { - ret = -EINVAL; + if (ctrl == NULL) { + ret = -ENOENT; goto done; } - if (query_menu->index >= mapping->menu_count) { + if (mapping->v4l2_type != V4L2_CTRL_TYPE_MENU || + query_menu->index >= mapping->menu_count) { ret = -EINVAL; goto done; } @@ -1263,7 +1264,7 @@ static int uvc_ctrl_add_event(struct v4l2_subscribed_event *sev, unsigned elems) ctrl = uvc_find_control(handle->chain, sev->id, &mapping); if (ctrl == NULL) { - ret = -EINVAL; + ret = -ENOENT; goto done; } @@ -1414,7 +1415,7 @@ int uvc_ctrl_get(struct uvc_video_chain *chain, ctrl = uvc_find_control(chain, xctrl->id, &mapping); if (ctrl == NULL) - return -EINVAL; + return -ENOENT; return __uvc_ctrl_get(chain, ctrl, mapping, &xctrl->value); } @@ -1431,7 +1432,9 @@ int uvc_ctrl_set(struct uvc_video_chain *chain, int ret; ctrl = uvc_find_control(chain, xctrl->id, &mapping); - if (ctrl == NULL || (ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR) == 0) + if (ctrl == NULL) + return -ENOENT; + if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR)) return -EINVAL; /* Clamp out of range values. */ diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c index f00db30..e5817b9 100644 --- a/drivers/media/usb/uvc/uvc_v4l2.c +++ b/drivers/media/usb/uvc/uvc_v4l2.c @@ -591,8 +591,10 @@ static long uvc_v4l2_do_ioctl(struct file *file, unsigned int cmd, void *arg) ret = uvc_ctrl_get(chain, &xctrl); uvc_ctrl_rollback(handle); - if (ret >= 0) - ctrl->value = xctrl.value; + if (ret < 0) + return ret == -ENOENT ? -EINVAL : ret; + + ctrl->value = xctrl.value; break; } @@ -612,7 +614,7 @@ static long uvc_v4l2_do_ioctl(struct file *file, unsigned int cmd, void *arg) ret = uvc_ctrl_set(chain, &xctrl); if (ret < 0) { uvc_ctrl_rollback(handle); - return ret; + return ret == -ENOENT ? -EINVAL : ret; } ret = uvc_ctrl_commit(handle, &xctrl, 1); if (ret == 0) @@ -637,8 +639,9 @@ static long uvc_v4l2_do_ioctl(struct file *file, unsigned int cmd, void *arg) ret = uvc_ctrl_get(chain, ctrl); if (ret < 0) { uvc_ctrl_rollback(handle); - ctrls->error_idx = i; - return ret; + ctrls->error_idx = ret == -ENOENT + ? ctrls->count : i; + return ret == -ENOENT ? -EINVAL : ret; } } ctrls->error_idx = 0; @@ -661,8 +664,10 @@ static long uvc_v4l2_do_ioctl(struct file *file, unsigned int cmd, void *arg) ret = uvc_ctrl_set(chain, ctrl); if (ret < 0) { uvc_ctrl_rollback(handle); - ctrls->error_idx = i; - return ret; + ctrls->error_idx = (ret == -ENOENT && + cmd == VIDIOC_S_EXT_CTRLS) + ? ctrls->count : i; + return ret == -ENOENT ? -EINVAL : ret; } }
When one of the requested controls doesn't exist the error_idx field must reflect that situation. For G_EXT_CTRLS and S_EXT_CTRLS, error_idx must be set to the control count. For TRY_EXT_CTRLS, it must be set to the index of the unexisting control. This issue was found by the v4l2-compliance tool. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- drivers/media/usb/uvc/uvc_ctrl.c | 17 ++++++++++------- drivers/media/usb/uvc/uvc_v4l2.c | 19 ++++++++++++------- 2 files changed, 22 insertions(+), 14 deletions(-)