On Thu, 28 Nov 2024 at 23:17, Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:

Hi Ricardo,

Thank you for the patch.

On Wed, Nov 27, 2024 at 12:14:49PM +0000, Ricardo Ribalda wrote:

...

When an async control is written, we copy a pointer to the file handle that started the operation. That pointer will be used when the device is done. Which could be anytime in the future.

If the user closes that file descriptor, its structure will be freed, and there will be one dangling pointer per pending async control, that the driver will try to use.

Clean all the dangling pointers during release().

To avoid adding a performance penalty in the most common case (no async operation). A counter has been introduced with some logic to make sure

s/). A/), a/

...

that it is properly handled.

Cc: stable@vger.kernel.org Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives") Signed-off-by: Ricardo Ribalda ribalda@chromium.org

---

drivers/media/usb/uvc/uvc_ctrl.c | 38 ++++++++++++++++++++++++++++++++++++-- drivers/media/usb/uvc/uvc_v4l2.c | 2 ++ drivers/media/usb/uvc/uvcvideo.h | 8 +++++++- 3 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 4fe26e82e3d1..b6af4ff92cbd 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1589,7 +1589,12 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain,

How about adding

SGTM.

static void uvc_ctrl_set_handle(struct uvc_control *ctrl, uvc_fh *handle) { if (handle) { if (!ctrl->handle) handle->pending_async_ctrls++; ctrl->handle = handle; } else if (ctrl->handle) { ctrl->handle = NULL; if (!WARN_ON(!handle->pending_async_ctrls))

Unless you think otherwise. I will make this:

WARN_ON(!handle->pending_async_ctrls); if (handle->pending_async_ctrls) handle->pending_async_ctrls--;

The double negation is difficult to read. I am pretty sure the compiler will do its magic and merge the two checks.

                    handle->pending_async_ctrls--;
    }

}

...

  mutex_lock(&chain->ctrl_mutex);

  handle = ctrl->handle;

• ctrl->handle = NULL;
  

• if (handle) {
  

•         ctrl->handle = NULL;
  

•         WARN_ON(!handle->pending_async_ctrls);
  

•         if (handle->pending_async_ctrls)
  

•                 handle->pending_async_ctrls--;
  

• }
  

This would become

    handle = ctrl->handle;
    uvc_ctrl_set_handle(ctrl, NULL);

...

  list_for_each_entry(mapping, &ctrl->info.mappings, list) {
          s32 value = __uvc_ctrl_get_value(mapping, data);

@@ -2046,8 +2051,11 @@ int uvc_ctrl_set(struct uvc_fh *handle, mapping->set(mapping, value, uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));

• if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
  

• if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) {
  

•         if (!ctrl->handle)
  

•                 handle->pending_async_ctrls++;
          ctrl->handle = handle;
  

• }
  

Here

    if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
            uvc_ctrl_set_handle(ctrl, handle);

...

  ctrl->dirty = 1;
  ctrl->modified = 1;

@@ -2770,6 +2778,32 @@ int uvc_ctrl_init_device(struct uvc_device *dev) return 0; }

+void uvc_ctrl_cleanup_fh(struct uvc_fh *handle) +{

• struct uvc_entity *entity;
  

• guard(mutex)(&handle->chain->ctrl_mutex);
  

• if (!handle->pending_async_ctrls)
  

•         return;
  

• list_for_each_entry(entity, &handle->chain->dev->entities, list) {
  

•         for (unsigned int i = 0; i < entity->ncontrols; ++i) {
  

•                 struct uvc_control *ctrl = &entity->controls[i];
  

•                 if (ctrl->handle != handle)
  

•                         continue;
  

•                 ctrl->handle = NULL;
  

•                 if (WARN_ON(!handle->pending_async_ctrls))
  

•                         continue;
  

•                 handle->pending_async_ctrls--;
  

And here

                    uvc_ctrl_set_handle(ctrl, NULL);

It seems less error-prone to centralize the logic. I'd add a lockdep_assert() in uvc_ctrl_set_handle(), but there's no easy access to the chain there.

...

•         }
  

• }
  

• WARN_ON(handle->pending_async_ctrls);
  

+}

/*

• Cleanup device controls.

*/ diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c index 97c5407f6603..b425306a3b8c 100644 --- a/drivers/media/usb/uvc/uvc_v4l2.c +++ b/drivers/media/usb/uvc/uvc_v4l2.c @@ -652,6 +652,8 @@ static int uvc_v4l2_release(struct file *file)

  uvc_dbg(stream->dev, CALLS, "%s\n", __func__);

• uvc_ctrl_cleanup_fh(handle);
  

• /* Only free resources if this is a privileged handle. */
  if (uvc_has_privileges(handle))
          uvc_queue_release(&stream->queue);
  

diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index 07f9921d83f2..c68659b70221 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -337,7 +337,10 @@ struct uvc_video_chain { struct uvc_entity *processing; /* Processing unit */ struct uvc_entity *selector; /* Selector unit */

• struct mutex ctrl_mutex;                /* Protects ctrl.info */
  

• struct mutex ctrl_mutex;                /*
  

•                                          * Protects ctrl.info and
  

•                                          * uvc_fh.pending_async_ctrls
  

•                                          */
  
  struct v4l2_prio_state prio;            /* V4L2 priority state */
  u32 caps;                               /* V4L2 chain-wide caps */
  

@@ -612,6 +615,7 @@ struct uvc_fh { struct uvc_video_chain *chain; struct uvc_streaming *stream; enum uvc_handle_state state;

• unsigned int pending_async_ctrls;
  

};

struct uvc_driver { @@ -797,6 +801,8 @@ int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id, int uvc_xu_ctrl_query(struct uvc_video_chain *chain, struct uvc_xu_control_query *xqry);

+void uvc_ctrl_cleanup_fh(struct uvc_fh *handle);

/* Utility functions */ struct usb_host_endpoint *uvc_find_endpoint(struct usb_host_interface *alts, u8 epaddr);

-- Regards,

Laurent Pinchart

On Thu, 28 Nov 2024 at 23:28, Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:

On Thu, Nov 28, 2024 at 11:25:25PM +0100, Ricardo Ribalda wrote:

...

On Thu, 28 Nov 2024 at 23:17, Laurent Pinchart wrote:

...

On Wed, Nov 27, 2024 at 12:14:49PM +0000, Ricardo Ribalda wrote:

...

When an async control is written, we copy a pointer to the file handle that started the operation. That pointer will be used when the device is done. Which could be anytime in the future.

If the user closes that file descriptor, its structure will be freed, and there will be one dangling pointer per pending async control, that the driver will try to use.

Clean all the dangling pointers during release().

To avoid adding a performance penalty in the most common case (no async operation). A counter has been introduced with some logic to make sure

s/). A/), a/

...

that it is properly handled.

Cc: stable@vger.kernel.org Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives") Signed-off-by: Ricardo Ribalda ribalda@chromium.org

---

drivers/media/usb/uvc/uvc_ctrl.c | 38 ++++++++++++++++++++++++++++++++++++-- drivers/media/usb/uvc/uvc_v4l2.c | 2 ++ drivers/media/usb/uvc/uvcvideo.h | 8 +++++++- 3 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 4fe26e82e3d1..b6af4ff92cbd 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1589,7 +1589,12 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain,

How about adding

SGTM.

...

static void uvc_ctrl_set_handle(struct uvc_control *ctrl, uvc_fh *handle) { if (handle) { if (!ctrl->handle) handle->pending_async_ctrls++; ctrl->handle = handle; } else if (ctrl->handle) { ctrl->handle = NULL; if (!WARN_ON(!handle->pending_async_ctrls))

Unless you think otherwise. I will make this:

WARN_ON(!handle->pending_async_ctrls); if (handle->pending_async_ctrls) handle->pending_async_ctrls--;

I'm fine with that, I went back and forth between the two.

...

The double negation is difficult to read. I am pretty sure the compiler will do its magic and merge the two checks.

...

                    handle->pending_async_ctrls--;
    }

}

Now that I think about it. Now that we have inverted the patches 1 and 2 we need to add the following change to your function:

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index d8d4bd2254ec..5ce9be812559 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1584,8 +1584,14 @@ static void uvc_ctrl_set_handle(struct uvc_control *ctrl, struct uvc_fh *handle) /* chain->ctrl_mutex must be held. */

if (handle) { - if (!ctrl->handle) - handle->pending_async_ctrls++; + if (ctrl->handle) { + if (ctrl->handle == handle) + return; + WARN_ON(!ctrl->handle->pending_async_ctrls); + if (ctrl->handle->pending_async_ctrls) + ctrl->handle->pending_async_ctrls--; + } + handle->pending_async_ctrls++; ctrl->handle = handle; } else if (ctrl->handle) { ctrl->handle = NULL;

Otherwise, if the control is handled by another fh, pending_async_ctrls will be unbalanced.

...

...

...

  mutex_lock(&chain->ctrl_mutex);

  handle = ctrl->handle;

• ctrl->handle = NULL;
  

• if (handle) {
  

•         ctrl->handle = NULL;
  

•         WARN_ON(!handle->pending_async_ctrls);
  

•         if (handle->pending_async_ctrls)
  

•                 handle->pending_async_ctrls--;
  

• }
  

This would become

    handle = ctrl->handle;
    uvc_ctrl_set_handle(ctrl, NULL);

...

  list_for_each_entry(mapping, &ctrl->info.mappings, list) {
          s32 value = __uvc_ctrl_get_value(mapping, data);

@@ -2046,8 +2051,11 @@ int uvc_ctrl_set(struct uvc_fh *handle, mapping->set(mapping, value, uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));

• if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
  

• if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) {
  

•         if (!ctrl->handle)
  

•                 handle->pending_async_ctrls++;
          ctrl->handle = handle;
  

• }
  

Here

    if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
            uvc_ctrl_set_handle(ctrl, handle);

...

  ctrl->dirty = 1;
  ctrl->modified = 1;

@@ -2770,6 +2778,32 @@ int uvc_ctrl_init_device(struct uvc_device *dev) return 0; }

+void uvc_ctrl_cleanup_fh(struct uvc_fh *handle) +{

• struct uvc_entity *entity;
  

• guard(mutex)(&handle->chain->ctrl_mutex);
  

• if (!handle->pending_async_ctrls)
  

•         return;
  

• list_for_each_entry(entity, &handle->chain->dev->entities, list) {
  

•         for (unsigned int i = 0; i < entity->ncontrols; ++i) {
  

•                 struct uvc_control *ctrl = &entity->controls[i];
  

•                 if (ctrl->handle != handle)
  

•                         continue;
  

•                 ctrl->handle = NULL;
  

•                 if (WARN_ON(!handle->pending_async_ctrls))
  

•                         continue;
  

•                 handle->pending_async_ctrls--;
  

And here

                    uvc_ctrl_set_handle(ctrl, NULL);

It seems less error-prone to centralize the logic. I'd add a lockdep_assert() in uvc_ctrl_set_handle(), but there's no easy access to the chain there.

...

•         }
  

• }
  

• WARN_ON(handle->pending_async_ctrls);
  

+}

/*

• Cleanup device controls.

*/ diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c index 97c5407f6603..b425306a3b8c 100644 --- a/drivers/media/usb/uvc/uvc_v4l2.c +++ b/drivers/media/usb/uvc/uvc_v4l2.c @@ -652,6 +652,8 @@ static int uvc_v4l2_release(struct file *file)

  uvc_dbg(stream->dev, CALLS, "%s\n", __func__);

• uvc_ctrl_cleanup_fh(handle);
  

• /* Only free resources if this is a privileged handle. */
  if (uvc_has_privileges(handle))
          uvc_queue_release(&stream->queue);
  

diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index 07f9921d83f2..c68659b70221 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -337,7 +337,10 @@ struct uvc_video_chain { struct uvc_entity *processing; /* Processing unit */ struct uvc_entity *selector; /* Selector unit */

• struct mutex ctrl_mutex;                /* Protects ctrl.info */
  

• struct mutex ctrl_mutex;                /*
  

•                                          * Protects ctrl.info and
  

•                                          * uvc_fh.pending_async_ctrls
  

•                                          */
  
  struct v4l2_prio_state prio;            /* V4L2 priority state */
  u32 caps;                               /* V4L2 chain-wide caps */
  

@@ -612,6 +615,7 @@ struct uvc_fh { struct uvc_video_chain *chain; struct uvc_streaming *stream; enum uvc_handle_state state;

• unsigned int pending_async_ctrls;
  

};

struct uvc_driver { @@ -797,6 +801,8 @@ int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id, int uvc_xu_ctrl_query(struct uvc_video_chain *chain, struct uvc_xu_control_query *xqry);

+void uvc_ctrl_cleanup_fh(struct uvc_fh *handle);

/* Utility functions */ struct usb_host_endpoint *uvc_find_endpoint(struct usb_host_interface *alts, u8 epaddr);

-- Regards,

Laurent Pinchart

On Thu, 28 Nov 2024 at 23:22, Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:

Hi Ricardo,

(CC'ing Hans Verkuil)

Thank you for the patch.

On Wed, Nov 27, 2024 at 12:14:50PM +0000, Ricardo Ribalda wrote:

...

If a file handle is waiting for a response from an async control, avoid that other file handle operate with it.

Without this patch, the first file handle will never get the event associated with that operation, which can lead to endless loops in applications. Eg: If an application A wants to change the zoom and to know when the operation has completed: it will open the video node, subscribe to the zoom event, change the control and wait for zoom to finish. If before the zoom operation finishes, another application B changes the zoom, the first app A will loop forever.

Hans, the V4L2 specification isn't very clear on this. I see pros and cons for both behaviours, with a preference for the current behaviour, as with this patch the control will remain busy until the file handle is closed if the device doesn't send the control event for any reason. What do you think ?

Just one small clarification. The same file handler can change the value of the async control as many times as it wants, even if the operation has not finished.

It will be other file handles that will get -EBUSY if they try to use an async control with an unfinished operation started by another fh.

...

Cc: stable@vger.kernel.org Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives") Signed-off-by: Ricardo Ribalda ribalda@chromium.org

---

drivers/media/usb/uvc/uvc_ctrl.c | 4 ++++ 1 file changed, 4 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index b6af4ff92cbd..3f8ae35cb3bc 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1955,6 +1955,10 @@ int uvc_ctrl_set(struct uvc_fh *handle, if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR)) return -EACCES;

• /* Other file handle is waiting a response from this async control. */
  

• if (ctrl->handle && ctrl->handle != handle)
  

•         return -EBUSY;
  

• /* Clamp out of range values. */
  switch (mapping->v4l2_type) {
  case V4L2_CTRL_TYPE_INTEGER:
  

-- Regards,

Laurent Pinchart

Hi Laurent, Ricardo,

On 28/11/2024 23:33, Laurent Pinchart wrote:

On Thu, Nov 28, 2024 at 11:28:29PM +0100, Ricardo Ribalda wrote:

...

On Thu, 28 Nov 2024 at 23:22, Laurent Pinchart wrote:

...

Hi Ricardo,

(CC'ing Hans Verkuil)

Thank you for the patch.

On Wed, Nov 27, 2024 at 12:14:50PM +0000, Ricardo Ribalda wrote:

...

If a file handle is waiting for a response from an async control, avoid that other file handle operate with it.

Without this patch, the first file handle will never get the event associated with that operation, which can lead to endless loops in applications. Eg: If an application A wants to change the zoom and to know when the operation has completed: it will open the video node, subscribe to the zoom event, change the control and wait for zoom to finish. If before the zoom operation finishes, another application B changes the zoom, the first app A will loop forever.

Hans, the V4L2 specification isn't very clear on this. I see pros and cons for both behaviours, with a preference for the current behaviour, as with this patch the control will remain busy until the file handle is closed if the device doesn't send the control event for any reason. What do you think ?

Just one small clarification. The same file handler can change the value of the async control as many times as it wants, even if the operation has not finished.

It will be other file handles that will get -EBUSY if they try to use an async control with an unfinished operation started by another fh.

Yes, I should have been more precised. If the device doesn't send the control event, then all other file handles will be prevented from setting the control until the file handle that set it first gets closed.

I think I need a bit more background here:

First of all, what is an asynchronous control in UVC? I think that means you can set it, but it takes time for that operation to finish, so you get an event later when the operation is done. So zoom and similar operations are examples of that.

And only when the operation finishes will the control event be sent, correct?

While the operation is ongoing, if you query the control value, is that reporting the current position or the final position?

E.g.: the zoom control is at value 0 and I set it to 10, then I poll the zoom control value: will that report the intermediate values until it reaches 10? And when it is at 10, the control event is sent?

Regards,

Hans

...

...

...

Cc: stable@vger.kernel.org Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives") Signed-off-by: Ricardo Ribalda ribalda@chromium.org

---

drivers/media/usb/uvc/uvc_ctrl.c | 4 ++++ 1 file changed, 4 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index b6af4ff92cbd..3f8ae35cb3bc 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1955,6 +1955,10 @@ int uvc_ctrl_set(struct uvc_fh *handle, if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR)) return -EACCES;

• /* Other file handle is waiting a response from this async control. */
  

• if (ctrl->handle && ctrl->handle != handle)
  

•         return -EBUSY;
  

• /* Clamp out of range values. */
  switch (mapping->v4l2_type) {
  case V4L2_CTRL_TYPE_INTEGER:
  

On Fri, Nov 29, 2024 at 11:59:27AM +0100, Ricardo Ribalda wrote:

On Fri, 29 Nov 2024 at 11:36, Hans Verkuil wrote:

...

On 28/11/2024 23:33, Laurent Pinchart wrote:

...

On Thu, Nov 28, 2024 at 11:28:29PM +0100, Ricardo Ribalda wrote:

...

On Thu, 28 Nov 2024 at 23:22, Laurent Pinchart wrote:

...

Hi Ricardo,

(CC'ing Hans Verkuil)

Thank you for the patch.

On Wed, Nov 27, 2024 at 12:14:50PM +0000, Ricardo Ribalda wrote:

...

If a file handle is waiting for a response from an async control, avoid that other file handle operate with it.

Without this patch, the first file handle will never get the event associated with that operation, which can lead to endless loops in applications. Eg: If an application A wants to change the zoom and to know when the operation has completed: it will open the video node, subscribe to the zoom event, change the control and wait for zoom to finish. If before the zoom operation finishes, another application B changes the zoom, the first app A will loop forever.

Hans, the V4L2 specification isn't very clear on this. I see pros and cons for both behaviours, with a preference for the current behaviour, as with this patch the control will remain busy until the file handle is closed if the device doesn't send the control event for any reason. What do you think ?

Just one small clarification. The same file handler can change the value of the async control as many times as it wants, even if the operation has not finished.

It will be other file handles that will get -EBUSY if they try to use an async control with an unfinished operation started by another fh.

Yes, I should have been more precised. If the device doesn't send the control event, then all other file handles will be prevented from setting the control until the file handle that set it first gets closed.

I think I need a bit more background here:

First of all, what is an asynchronous control in UVC? I think that means you can set it, but it takes time for that operation to finish, so you get an event later when the operation is done. So zoom and similar operations are examples of that.

And only when the operation finishes will the control event be sent, correct?

You are correct. This diagrams from the spec is more or less clear: https://ibb.co/MDGn7F3

...

While the operation is ongoing, if you query the control value, is that reporting the current position or the final position?

I'd expect hardware will return either the current position, the start position or the final position. I could not find anything in the spec that points in one direction or the others.

Figure 2-21 in UVC 1.5 indicates that the device should STALL the GET_CUR and SET_CUR requests if a state change is in progress.

And in the driver I believe that we might have a bug handling this case (will send a patch if I can confirm it) the zoom is at 0 and you set it 10 if you read the value 2 times before the camera reaches value 10:

• First value will come from the hardware and the response will be cached

Only if the control doesn't have the auto-update flag set, so it will be device-dependent. As GET_CUR should stall that's not really relevant, except for the fact that devices may not stall the request.

• Second value will be the cached one

now the camera is at zoom 10 If you read the value, you will read the cached value

...

E.g.: the zoom control is at value 0 and I set it to 10, then I poll the zoom control value: will that report the intermediate values until it reaches 10? And when it is at 10, the control event is sent?

...

...

...

...

Cc: stable@vger.kernel.org Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives") Signed-off-by: Ricardo Ribalda ribalda@chromium.org

---

drivers/media/usb/uvc/uvc_ctrl.c | 4 ++++ 1 file changed, 4 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index b6af4ff92cbd..3f8ae35cb3bc 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1955,6 +1955,10 @@ int uvc_ctrl_set(struct uvc_fh *handle, if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR)) return -EACCES;

• /* Other file handle is waiting a response from this async control. */
  

• if (ctrl->handle && ctrl->handle != handle)
  

•         return -EBUSY;
  

• /* Clamp out of range values. */
  switch (mapping->v4l2_type) {
  case V4L2_CTRL_TYPE_INTEGER:
  

On 29/11/2024 12:54, Ricardo Ribalda wrote:

On Fri, 29 Nov 2024 at 12:06, Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:

...

On Fri, Nov 29, 2024 at 11:59:27AM +0100, Ricardo Ribalda wrote:

...

On Fri, 29 Nov 2024 at 11:36, Hans Verkuil wrote:

...

On 28/11/2024 23:33, Laurent Pinchart wrote:

...

On Thu, Nov 28, 2024 at 11:28:29PM +0100, Ricardo Ribalda wrote:

...

On Thu, 28 Nov 2024 at 23:22, Laurent Pinchart wrote: > > Hi Ricardo, > > (CC'ing Hans Verkuil) > > Thank you for the patch. > > On Wed, Nov 27, 2024 at 12:14:50PM +0000, Ricardo Ribalda wrote: >> If a file handle is waiting for a response from an async control, avoid >> that other file handle operate with it. >> >> Without this patch, the first file handle will never get the event >> associated with that operation, which can lead to endless loops in >> applications. Eg: >> If an application A wants to change the zoom and to know when the >> operation has completed: >> it will open the video node, subscribe to the zoom event, change the >> control and wait for zoom to finish. >> If before the zoom operation finishes, another application B changes >> the zoom, the first app A will loop forever. > > Hans, the V4L2 specification isn't very clear on this. I see pros and > cons for both behaviours, with a preference for the current behaviour, > as with this patch the control will remain busy until the file handle is > closed if the device doesn't send the control event for any reason. What > do you think ?

Just one small clarification. The same file handler can change the value of the async control as many times as it wants, even if the operation has not finished.

It will be other file handles that will get -EBUSY if they try to use an async control with an unfinished operation started by another fh.

Yes, I should have been more precised. If the device doesn't send the control event, then all other file handles will be prevented from setting the control until the file handle that set it first gets closed.

I think I need a bit more background here:

First of all, what is an asynchronous control in UVC? I think that means you can set it, but it takes time for that operation to finish, so you get an event later when the operation is done. So zoom and similar operations are examples of that.

And only when the operation finishes will the control event be sent, correct?

You are correct. This diagrams from the spec is more or less clear: https://ibb.co/MDGn7F3

...

While the operation is ongoing, if you query the control value, is that reporting the current position or the final position?

I'd expect hardware will return either the current position, the start position or the final position. I could not find anything in the spec that points in one direction or the others.

Figure 2-21 in UVC 1.5 indicates that the device should STALL the GET_CUR and SET_CUR requests if a state change is in progress.

...

And in the driver I believe that we might have a bug handling this case (will send a patch if I can confirm it) the zoom is at 0 and you set it 10 if you read the value 2 times before the camera reaches value 10:

• First value will come from the hardware and the response will be cached

Only if the control doesn't have the auto-update flag set, so it will be device-dependent. As GET_CUR should stall that's not really relevant, except for the fact that devices may not stall the request.

I missed that the device will likely stall during async operations.

What do you think of something like this? I believe it can work with compliant and non compliant devices. Note that the event will be received by the device that originated the operation, not to the second one that might receive an error during write/read.

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 4fe26e82e3d1..9a86c912e7a2 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1826,14 +1826,15 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev, continue;

            /*

•            * Reset the loaded flag for auto-update controls that were
  

•            * Reset the loaded flag for auto-update controls and for
  

•            * asynchronous controls with pending operations, that were
             * marked as loaded in uvc_ctrl_get/uvc_ctrl_set to prevent
             * uvc_ctrl_get from using the cached value, and for write-only
             * controls to prevent uvc_ctrl_set from setting bits not
             * explicitly set by the user.
             */
            if (ctrl->info.flags & UVC_CTRL_FLAG_AUTO_UPDATE ||
  

•               !(ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR))
  

•               !(ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) || ctrl->handle)
                    ctrl->loaded = 0;
  
            if (!ctrl->dirty)
  

@@ -2046,8 +2047,18 @@ int uvc_ctrl_set(struct uvc_fh *handle, mapping->set(mapping, value, uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));

•   if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
  

•           ctrl->handle = handle;
  

•   if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) {
  

•           /*
  

•            * Other file handle is waiting for an operation on
  

•            * this asynchronous control. If the device is compliant
  

•            * this operation will fail.
  

•            *
  

•            * Do not replace the handle pointer, so the original file
  

•            * descriptor will get the completion event.
  

•            */
  

•           if (!ctrl->handle)
  

•                   ctrl->handle = handle;
  

I don't think this is right: you want the completion event for async controls to go to all filehandles that are subscribed to that control.

Which is what happens if handle == NULL (as I understand the code).

Regards,

Hans

•   }
  
    ctrl->dirty = 1;
    ctrl->modified = 1;
  
  

...

...

• Second value will be the cached one

now the camera is at zoom 10 If you read the value, you will read the cached value

...

E.g.: the zoom control is at value 0 and I set it to 10, then I poll the zoom control value: will that report the intermediate values until it reaches 10? And when it is at 10, the control event is sent?

...

...

>> Cc: stable@vger.kernel.org >> Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives") >> Signed-off-by: Ricardo Ribalda ribalda@chromium.org >> --- >> drivers/media/usb/uvc/uvc_ctrl.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c >> index b6af4ff92cbd..3f8ae35cb3bc 100644 >> --- a/drivers/media/usb/uvc/uvc_ctrl.c >> +++ b/drivers/media/usb/uvc/uvc_ctrl.c >> @@ -1955,6 +1955,10 @@ int uvc_ctrl_set(struct uvc_fh *handle, >> if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR)) >> return -EACCES; >> >> + /* Other file handle is waiting a response from this async control. */ >> + if (ctrl->handle && ctrl->handle != handle) >> + return -EBUSY; >> + >> /* Clamp out of range values. */ >> switch (mapping->v4l2_type) { >> case V4L2_CTRL_TYPE_INTEGER:

-- Regards,

Laurent Pinchart

On Mon, 2 Dec 2024 at 01:19, Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:

On Fri, Nov 29, 2024 at 11:18:54PM +0100, Ricardo Ribalda wrote:

...

On Fri, 29 Nov 2024 at 23:03, Laurent Pinchart wrote:

...

On Fri, Nov 29, 2024 at 07:47:31PM +0100, Ricardo Ribalda wrote:

...

Before we all go on a well deserved weekend, let me recap what we know. If I did not get something correctly, let me know.

1. Well behaved devices do not allow to set or get an incomplete async

control. They will stall instead (ref: Figure 2-21 in UVC 1.5 ) 2) Both Laurent and Ricardo consider that there is a big chance that some camera modules do not implement this properly. (ref: years of crying over broken module firmware :) )

3. ctrl->handle is designed to point to the fh that originated the

control. So the logic can decide if the originator needs to be notified or not. (ref: uvc_ctrl_send_event() ) 4) Right now we replace the originator in ctrl->handle for unfinished async controls. (ref: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/driv...)

My interpretation is that: A) We need to change 4). We shall not change the originator of unfinished ctrl->handle. B) Well behaved cameras do not need the patch "Do not set an async control owned by another fh" C) For badly behaved cameras, it is fine if we slightly break the v4l2-compliance in corner cases, if we do not break any internal data structure.

The fact that some devices may not implement the documented behaviour correctly may not be a problem. Well-behaved devices will stall, which means we shouldn't query the device while as async update is in progress. Badly-behaved devices, whatever they do when queried, should not cause any issue if we don't query them.

I thought we could detect the stall and return safely. Isn't that the case?

We could, but if we know the device will stall anyway, is there a reason not to avoid issuing the request in the first place ?

Because there are always going to be devices that do not send the event when the control has finished.

It is impossible to know the state of the device: Is the zoom still moving or the device is not compliant?

...

Why we have not seen issues with this?

I haven't tested a PTZ device for a very long time, and you would need to hit a small time window to see the issue.

Not that small, some devices take up to 10 seconds to go from the smallest zoom to the biggest zoom. I'd say that v4l2-ctl has a very big chance to hit any error.

BTW, homing can take an even longer time.

...

...

We should not send GET_CUR and SET_CUR requests to the device while an async update is in progress, and use cached values instead. When we receive the async update event, we should clear the cache. This will be the same for both well-behaved and badly-behaved devices, so we can expose the same behaviour towards userspace.

seting ctrl->loaded = 0 when we get an event sounds like a good idea and something we can implement right away. If I have to resend the set I will add it to the end.

...

We possibly also need some kind of timeout mechanism to cope with the async update event not being delivered by the device.

This is the part that worries me the most:

• timeouts make the code fragile
• What is a good value for timeout? 1 second, 30, 300? I do not think

that we can find a value.

I've been thinking about the implementation of uvc_fh cleanup over the weekend, and having a timeout would have the nice advantage that we could reference-count uvc_fh instead of implementing a cleanup that walks over all controls when closing a file handle. I think it would make the code simpler, and possibly safer too.

The problem with a timeout is: - We do not know what is the right value for the timeout. - We need to have one timeout per control, or implement a timeout dispatcher mechanism, and that is racy by nature - It will require introducing a new locking mechanism to avoid races between the timeout handler and the event completion - It introduces a new lifetime in the driver... I'd argue that it is best to have less, not more.

I do not see many benefits....

What we can introduce on top of my set is something like this (just pseudocode, do not scream at me :P) That code can prevent stalls and will work with misbehaving hardware.... (But I still do not know a good value for timeout) and solve some of the issues that I mention.

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index f0e8a436a306..a55bd4b3ac07 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1132,6 +1132,9 @@ static int __uvc_ctrl_get(struct uvc_video_chain *chain, if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0) return -EACCES;

+ if (ctrl->handle && ( NOW - ctrl->async_start_time ) < TIMEOUT) + return -EBUSY; + ret = __uvc_ctrl_load_cur(chain, ctrl); if (ret < 0) return ret; @@ -1591,6 +1594,7 @@ static int uvc_ctrl_get_handle(struct uvc_fh *handle, struct uvc_control *ctrl) return -EBUSY;

ctrl->handle = handle; + ctrl->async_start_time = NOW; handle->pending_async_ctrls++; return 0; } @@ -1982,6 +1986,9 @@ int uvc_ctrl_set(struct uvc_fh *handle, if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR)) return -EACCES;

+ if (ctrl->handle && ( NOW - ctrl->async_start_time ) < TIMEOUT) + return -EBUSY; + /* Clamp out of range values. */ switch (mapping->v4l2_type) { case V4L2_CTRL_TYPE_INTEGER: diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index e0e4f099a210..5c82fae94dff 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -154,6 +154,8 @@ struct uvc_control { * File handle that initially changed the * async control. */ + + TIME async_start_time; };

In any case. Can we solve this in incremental steps? I think the current patch seems simple and correct. We can introduce the timeout later.

Thanks!

...

...

Regarding the userspace behaviour during an auto-update, we have multiple options:

For control get,

• We can return -EBUSY
• We can return the old value from the cache
• We can return the new value fromt he cache

Returning -EBUSY would be simpler to implement.

Not only easy, I think it is the most correct,

...

I don't think the behaviour should depend on whether the control is read on the file handle that initiated the async operation or on a different file handle.

For control set, I don't think we can do much else than returning -EBUSY, regardless of which file handle the control is set on.

ACK.

...

...

I will send a new version with my interpretation.

Thanks for a great discussion

Looking with some perspective... I believe that we should look into the "userspace behaviour for auto controls" in a different patchset. It is slightly unrelated to this discussion.

-- Regards,

Laurent Pinchart

On Mon, 2 Dec 2024 at 09:38, Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:

On Mon, Dec 02, 2024 at 08:28:48AM +0100, Ricardo Ribalda wrote:

...

On Mon, 2 Dec 2024 at 01:19, Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:

...

On Fri, Nov 29, 2024 at 11:18:54PM +0100, Ricardo Ribalda wrote:

...

On Fri, 29 Nov 2024 at 23:03, Laurent Pinchart wrote:

...

On Fri, Nov 29, 2024 at 07:47:31PM +0100, Ricardo Ribalda wrote:

...

Before we all go on a well deserved weekend, let me recap what we know. If I did not get something correctly, let me know.

1. Well behaved devices do not allow to set or get an incomplete async

control. They will stall instead (ref: Figure 2-21 in UVC 1.5 ) 2) Both Laurent and Ricardo consider that there is a big chance that some camera modules do not implement this properly. (ref: years of crying over broken module firmware :) )

3. ctrl->handle is designed to point to the fh that originated the

control. So the logic can decide if the originator needs to be notified or not. (ref: uvc_ctrl_send_event() ) 4) Right now we replace the originator in ctrl->handle for unfinished async controls. (ref: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/driv...)

My interpretation is that: A) We need to change 4). We shall not change the originator of unfinished ctrl->handle. B) Well behaved cameras do not need the patch "Do not set an async control owned by another fh" C) For badly behaved cameras, it is fine if we slightly break the v4l2-compliance in corner cases, if we do not break any internal data structure.

The fact that some devices may not implement the documented behaviour correctly may not be a problem. Well-behaved devices will stall, which means we shouldn't query the device while as async update is in progress. Badly-behaved devices, whatever they do when queried, should not cause any issue if we don't query them.

I thought we could detect the stall and return safely. Isn't that the case?

We could, but if we know the device will stall anyway, is there a reason not to avoid issuing the request in the first place ?

Because there are always going to be devices that do not send the event when the control has finished.

It is impossible to know the state of the device: Is the zoom still moving or the device is not compliant?

Another type of broken behaviour would be devices that don't correctly handle UVC_GET and UVC_CUR requests while an async update is in progress. If you issue those requests, those devices will break. Are they more or less important than devices that don't send an event ?

All of those are partly theoretical problems. I'd rather keep it simple for now until we get feedback about broken devices.

...

...

...

Why we have not seen issues with this?

I haven't tested a PTZ device for a very long time, and you would need to hit a small time window to see the issue.

Not that small, some devices take up to 10 seconds to go from the smallest zoom to the biggest zoom. I'd say that v4l2-ctl has a very big chance to hit any error.

BTW, homing can take an even longer time.

...

...

...

We should not send GET_CUR and SET_CUR requests to the device while an async update is in progress, and use cached values instead. When we receive the async update event, we should clear the cache. This will be the same for both well-behaved and badly-behaved devices, so we can expose the same behaviour towards userspace.

seting ctrl->loaded = 0 when we get an event sounds like a good idea and something we can implement right away. If I have to resend the set I will add it to the end.

...

We possibly also need some kind of timeout mechanism to cope with the async update event not being delivered by the device.

This is the part that worries me the most:

• timeouts make the code fragile
• What is a good value for timeout? 1 second, 30, 300? I do not think

that we can find a value.

I've been thinking about the implementation of uvc_fh cleanup over the weekend, and having a timeout would have the nice advantage that we could reference-count uvc_fh instead of implementing a cleanup that walks over all controls when closing a file handle. I think it would make the code simpler, and possibly safer too.

The problem with a timeout is:

• We do not know what is the right value for the timeout.
• We need to have one timeout per control, or implement a timeout

dispatcher mechanism, and that is racy by nature

• It will require introducing a new locking mechanism to avoid races

between the timeout handler and the event completion

Timeouts don't come for free, that's for sure.

...

• It introduces a new lifetime in the driver... I'd argue that it is

best to have less, not more.

That I disagree with, my experience with V4L2 (as well as with DRM, or actually the kernel in general) is that we would be in a much better place today if object lifetimes were handled with reference counting more widely.

...

I do not see many benefits....

The main benefit is simplifying the close() implementation and complexity, as well as lowering lock contention (whether the latter brings a real advantage in practice, I haven't checked)..

...

What we can introduce on top of my set is something like this (just pseudocode, do not scream at me :P) That code can prevent stalls and will work with misbehaving hardware.... (But I still do not know a good value for timeout) and solve some of the issues that I mention.

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index f0e8a436a306..a55bd4b3ac07 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1132,6 +1132,9 @@ static int __uvc_ctrl_get(struct uvc_video_chain *chain, if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0) return -EACCES;

•   if (ctrl->handle && ( NOW - ctrl->async_start_time ) < TIMEOUT)
  

•           return -EBUSY;
  

•   ret = __uvc_ctrl_load_cur(chain, ctrl);
    if (ret < 0)
            return ret;
  

@@ -1591,6 +1594,7 @@ static int uvc_ctrl_get_handle(struct uvc_fh *handle, struct uvc_control *ctrl) return -EBUSY;

    ctrl->handle = handle;

•   ctrl->async_start_time = NOW;
    handle->pending_async_ctrls++;
    return 0;
  

} @@ -1982,6 +1986,9 @@ int uvc_ctrl_set(struct uvc_fh *handle, if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR)) return -EACCES;

•   if (ctrl->handle && ( NOW - ctrl->async_start_time ) < TIMEOUT)
  

•           return -EBUSY;
  

If I understand you correctly, this is meant to prevent accessing the device for an initial TIMEOUT period, based on the knowledge it will very likely STALL. After the TIMEOUT, if the async set is still not complete, SET_CUR will be issued, and a STALL will likely occur.

If we device to standardize on -EBUSY while the async set is in progress, we need to do so before and after the timeout. uvc_query_ctrl() will return -EBUSY in case of a STALL if the device reports the "Not ready" error. This is what I expect a device to report in this case. Of course the UVC specification, in section 2.4.4, documents that the device will update the request error code control, but doesn't tell which value should be used :-S I suppose we'll handle broken devices if the need arise.

...

•   /* Clamp out of range values. */
    switch (mapping->v4l2_type) {
    case V4L2_CTRL_TYPE_INTEGER:
  

diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index e0e4f099a210..5c82fae94dff 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -154,6 +154,8 @@ struct uvc_control { * File handle that initially changed the * async control. */

•   TIME async_start_time;
  

};

In any case. Can we solve this in incremental steps? I think the current patch seems simple and correct. We can introduce the timeout later.

The main reason for a timeout was to replace walking over all controls at close() time with reference counting of uvc_fh, which is an alternative approach to the current patch. Given that picking a good timeout value is difficult, and that the current implementation already skips the walk in the most common case when no async set is pending, I suppose we could start with that.

Cool. I am going to send a new version of: https://patchwork.linuxtv.org/project/linux-media/patch/20241129-uvc-fix-asy...

using uvc_ctrl_get_handle / uvc_ctrl_put_handle ad lockdep annotations.

Hopefully in the future we can get rid of: uvc_ctrl_cleanup_fh()

Thanks!

...

...

...

...

Regarding the userspace behaviour during an auto-update, we have multiple options:

For control get,

• We can return -EBUSY
• We can return the old value from the cache
• We can return the new value fromt he cache

Returning -EBUSY would be simpler to implement.

Not only easy, I think it is the most correct,

...

I don't think the behaviour should depend on whether the control is read on the file handle that initiated the async operation or on a different file handle.

For control set, I don't think we can do much else than returning -EBUSY, regardless of which file handle the control is set on.

ACK.

...

...

I will send a new version with my interpretation.

Thanks for a great discussion

Looking with some perspective... I believe that we should look into the "userspace behaviour for auto controls" in a different patchset. It is slightly unrelated to this discussion.

-- Regards,

Laurent Pinchart

Hi,

On 2-Dec-24 11:50 AM, Ricardo Ribalda wrote:

On Mon, 2 Dec 2024 at 11:27, Hans de Goede hdegoede@redhat.com wrote:

...

Hi,

On 2-Dec-24 9:44 AM, Hans Verkuil wrote:

...

On 02/12/2024 09:11, Laurent Pinchart wrote:

...

On Mon, Dec 02, 2024 at 09:05:07AM +0100, Hans Verkuil wrote:

...

On 02/12/2024 01:18, Laurent Pinchart wrote:

...

On Fri, Nov 29, 2024 at 11:18:54PM +0100, Ricardo Ribalda wrote: > On Fri, 29 Nov 2024 at 23:03, Laurent Pinchart wrote: >> On Fri, Nov 29, 2024 at 07:47:31PM +0100, Ricardo Ribalda wrote: >>> Before we all go on a well deserved weekend, let me recap what we >>> know. If I did not get something correctly, let me know. >>> >>> 1) Well behaved devices do not allow to set or get an incomplete async >>> control. They will stall instead (ref: Figure 2-21 in UVC 1.5 ) >>> 2) Both Laurent and Ricardo consider that there is a big chance that >>> some camera modules do not implement this properly. (ref: years of >>> crying over broken module firmware :) ) >>> >>> 3) ctrl->handle is designed to point to the fh that originated the >>> control. So the logic can decide if the originator needs to be >>> notified or not. (ref: uvc_ctrl_send_event() ) >>> 4) Right now we replace the originator in ctrl->handle for unfinished >>> async controls. (ref: >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/driv...) >>> >>> My interpretation is that: >>> A) We need to change 4). We shall not change the originator of >>> unfinished ctrl->handle. >>> B) Well behaved cameras do not need the patch "Do not set an async >>> control owned by another fh" >>> C) For badly behaved cameras, it is fine if we slightly break the >>> v4l2-compliance in corner cases, if we do not break any internal data >>> structure. >> >> The fact that some devices may not implement the documented behaviour >> correctly may not be a problem. Well-behaved devices will stall, which >> means we shouldn't query the device while as async update is in >> progress. Badly-behaved devices, whatever they do when queried, should >> not cause any issue if we don't query them. > > I thought we could detect the stall and return safely. Isn't that the case?

We could, but if we know the device will stall anyway, is there a reason not to avoid issuing the request in the first place ?

> Why we have not seen issues with this?

I haven't tested a PTZ device for a very long time, and you would need to hit a small time window to see the issue.

>> We should not send GET_CUR and SET_CUR requests to the device while an >> async update is in progress, and use cached values instead. When we >> receive the async update event, we should clear the cache. This will be >> the same for both well-behaved and badly-behaved devices, so we can >> expose the same behaviour towards userspace. > > seting ctrl->loaded = 0 when we get an event sounds like a good idea > and something we can implement right away. > If I have to resend the set I will add it to the end. > >> We possibly also need some kind of timeout mechanism to cope with the >> async update event not being delivered by the device. > > This is the part that worries me the most: > - timeouts make the code fragile > - What is a good value for timeout? 1 second, 30, 300? I do not think > that we can find a value.

I've been thinking about the implementation of uvc_fh cleanup over the weekend, and having a timeout would have the nice advantage that we could reference-count uvc_fh instead of implementing a cleanup that walks over all controls when closing a file handle. I think it would make the code simpler, and possibly safer too.

>> Regarding the userspace behaviour during an auto-update, we have >> multiple options: >> >> For control get, >> >> - We can return -EBUSY >> - We can return the old value from the cache

This would match the control behavior best. Only when the operation is done is the control updated and the control event sent.

Some questions: is any of this documented for UVC? Because this is non-standard

No this isn't documented.

...

behavior. Are there applications that rely on this? Should we perhaps add

I don't know.

...

proper support for this to the control framework? E.g. add an ASYNC flag and document this?

We could, but this is such a specific use case that I don't think is worth adding complexity to the already complex control framework would be worth it. What we could do is perhaps adding a flag for the userspace API, but even there, I never like modelling an API with a single user.

Well, it might be a single driver that uses this, but it is also the most used driver by far. I think the only change is to add a flag for this and describe how it should behave. And add v4l2-compliance tests for it.

Otherwise no changes to the control framework are needed, I think.

Controls with the ASYNC flag set would:

• return the old value from the cache.
• document that setting a new value while the operation is in progress results in EBUSY. Document that if the new value is equal to the old value, then return 0 and do nothing (alternative is to just immediately send the control changed event, but that might require a control framework change).
• when the operation finishes, update the cache to the new value and send the control changed event.
• document that userspace should specify V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK when subscribing to the control if you calling fh wants to know when the operation finishes.
• document how timeouts should be handled: this is tricky, especially with bad hardware. I.e. if the hw doesn't send the event, does that mean that you are never able to set the control since it will stall? In the end this will just reflect how UVC handles this.

I have been catching up on this thread (I have not read the v3 and v4 threads yet).

This all started with Ricardo noticing that ctrl->handle may get overwritten when another app sets the ctrl, causing the first app to set the ctrl to get a V4L2_EVENT for the ctrl (if subscribed) even though it set the ctrl itself.

My observations so far:

1. This is only hit when another app changes the ctrl after the first app,

in this case, if there is no stall issued by the hw for the second app's request, arguably the first app getting the event for the ctrl is correct

In other words, for non compliant cameras the current behaviour is correct. For compliant cameras it is broken.

...

since it was changed by the second app. IOW I think the current behavior is not only fine, but even desirable. Assuming we only override ctrl->handle after successfully sending the set-ctrl request to the hardware.

We are overriding ctrl->handle unconditionally, even if set-ctrl stalls.

Right I was just looking at that. Since we hold chain->ctrl_mutex from the start of uvc_ioctl_s_try_ext_ctrls() until we finish uvc_ctrl_commit() we can delay setting ctrl->handle until a successful commit.

Actually looking at this I think I've found another bug, uvc_ctrl_set() always sets ctrl->handle also in the VIDIOC_TRY_EXT_CTRLS case in which case we never send the SET_CUR command to the camera.

And the handle is not part of the data backed up / restored by a rollback.

Moving the storing of the handle to a successful commit fixes both the overriding of the handle in the stall case as well as the bogus setting of the handle on VIDIOC_TRY_EXT_CTRLS.

So my suggestion would be not touching ctrl->handle from ctrl_set() instead store the handle in a new ctrl->new_handle variable there and copy the new_handle values to handle for affected controls after a successful commit (before releasing the lock).

This will also successfully replace the handle for buggy devices which do not report a stall but instead accept the second SET_CUR. This replacement of handle is the correct thing todo in this case since after the second SET_CUR is accepted there is a change of the ctrl happening from the pov of the issuer of the first SET_CUR.

...

2. This adds a lot of complexity for not sending an event to the app

which made the change. Hans V. suggested maybe adding some sort of flag for async ctrls to the userspace API. I wonder if we should not just get rid of this complexity and document that these controls will always generate events independent of V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK ? That would certainly simplify things, but it raises the questions if this will cause issues for existing applications.

To be honest, I am more concerned about the dangling pointers than the event.

Ack.

Updating the doc to say that ASYC controls always generate events sounds good to me. But until we reach an agreement on the specifics I'd rather land this fix and then we can take time to design an API that works for compliant and non compliant hardware.

I agree that we should focus on fixing the dangling pointer problem and your v4 series is heading in the right direction there.

I'm not sure if we should take your v4 series as is though, see above.

At a minimum I think the issue with setting ctrl->handle in the VIDIOC_TRY_EXT_CTRLS case needs to be fixed.

...

Note that if we simply return -EBUSY on set until acked by a status event we also avoid the issue of ctrl->handle getting overwritten, but that relies on reliable status events; or requires timeout handling.

3. I agree with Ricardo that a timeout based approach for cameras which

to not properly send status events for async ctrls is going to be problematic. Things like pan/tilt homing can take multiple seconds which is really long to use as a timeout if we plan to return -EBUSY until the timeout triggers. I think it would be better to just rely on the hardware sending a stall, or it accepting and correctly handling a new CUR_SET command while the previous one is still being processed.

I guess we can track if the hw does send status events when async ctrls complete and then do the -EBUSY thing without going out to the hw after the first time an async ctrl has been acked by a status event.

And then combine that with the current behavior of overwriting ctrl->handle until the ctrl has been marked as having working status events. So:

a) In case we do not know yet if a ctrl gets status-event acks; and on devices without reliable status events keep current behavior.

b) As soon as we know a ctrl has reliable status events, switch to returning -EBUSY if a set is pending (as indicated by ctrl->handle being set).

I don't like the fact that this changes the behavior after the first status event acking an async ctrl, but I don't really see another way.

If I understood you correctly, you are proposing the following quirk:

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index f0e8a436a306..1a554afeaa2f 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1132,6 +1132,9 @@ static int __uvc_ctrl_get(struct uvc_video_chain *chain, if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0) return -EACCES;

•   if (ctrl->handle && ctrl->async_event_works)
  

•           return -EBUSY;
  

•   ret = __uvc_ctrl_load_cur(chain, ctrl);
    if (ret < 0)
            return ret;
  

@@ -1672,6 +1675,8 @@ bool uvc_ctrl_status_event_async(struct urb> *urb, struct uvc_video_chain *chain, /* Flush the control cache, the data might have changed. */ ctrl->loaded = 0;

•   ctrl->async_event_works = true;
  

•   if (list_empty(&ctrl->info.mappings))
            return false;
  
  

@@ -1982,6 +1987,9 @@ int uvc_ctrl_set(struct uvc_fh *handle, if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR)) return -EACCES;

•   if (ctrl->handle && ctrl->async_event_works)
  

•           return -EBUSY;
  

Yes this is what I'm proposing, except that this check should be skipped in the VIDIOC_TRY_EXT_CTRLS case. I think we need to add a "bool try" parameter to uvc_ctrl_set() and not look at / set [new_]handle when this is set.

    /* Clamp out of range values. */
    switch (mapping->v4l2_type) {
    case V4L2_CTRL_TYPE_INTEGER:

diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index e0e4f099a210..0ef7c594eecb 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -154,6 +154,7 @@ struct uvc_control { * File handle that initially changed the * async control. */

•   bool async_event_works;
  

};

The benefit is that we can predict a device returning STALL without having to actually do the set/get operation.

We can add it as a follow-up patch.

Another benefit would be correctly returning -EBUSY when trying to get the ctrl. I agree this could be done as a follow-up.

Regards,

Hans

On Mon, Dec 02, 2024 at 02:29:24PM +0100, Ricardo Ribalda wrote:

On Mon, 2 Dec 2024 at 13:19, Hans de Goede wrote:

...

On 2-Dec-24 11:50 AM, Ricardo Ribalda wrote:

...

On Mon, 2 Dec 2024 at 11:27, Hans de Goede wrote:

...

On 2-Dec-24 9:44 AM, Hans Verkuil wrote:

...

On 02/12/2024 09:11, Laurent Pinchart wrote:

...

On Mon, Dec 02, 2024 at 09:05:07AM +0100, Hans Verkuil wrote: > On 02/12/2024 01:18, Laurent Pinchart wrote: >> On Fri, Nov 29, 2024 at 11:18:54PM +0100, Ricardo Ribalda wrote: >>> On Fri, 29 Nov 2024 at 23:03, Laurent Pinchart wrote: >>>> On Fri, Nov 29, 2024 at 07:47:31PM +0100, Ricardo Ribalda wrote: >>>>> Before we all go on a well deserved weekend, let me recap what we >>>>> know. If I did not get something correctly, let me know. >>>>> >>>>> 1) Well behaved devices do not allow to set or get an incomplete async >>>>> control. They will stall instead (ref: Figure 2-21 in UVC 1.5 ) >>>>> 2) Both Laurent and Ricardo consider that there is a big chance that >>>>> some camera modules do not implement this properly. (ref: years of >>>>> crying over broken module firmware :) ) >>>>> >>>>> 3) ctrl->handle is designed to point to the fh that originated the >>>>> control. So the logic can decide if the originator needs to be >>>>> notified or not. (ref: uvc_ctrl_send_event() ) >>>>> 4) Right now we replace the originator in ctrl->handle for unfinished >>>>> async controls. (ref: >>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/driv...) >>>>> >>>>> My interpretation is that: >>>>> A) We need to change 4). We shall not change the originator of >>>>> unfinished ctrl->handle. >>>>> B) Well behaved cameras do not need the patch "Do not set an async >>>>> control owned by another fh" >>>>> C) For badly behaved cameras, it is fine if we slightly break the >>>>> v4l2-compliance in corner cases, if we do not break any internal data >>>>> structure. >>>> >>>> The fact that some devices may not implement the documented behaviour >>>> correctly may not be a problem. Well-behaved devices will stall, which >>>> means we shouldn't query the device while as async update is in >>>> progress. Badly-behaved devices, whatever they do when queried, should >>>> not cause any issue if we don't query them. >>> >>> I thought we could detect the stall and return safely. Isn't that the case? >> >> We could, but if we know the device will stall anyway, is there a reason >> not to avoid issuing the request in the first place ? >> >>> Why we have not seen issues with this? >> >> I haven't tested a PTZ device for a very long time, and you would need >> to hit a small time window to see the issue. >> >>>> We should not send GET_CUR and SET_CUR requests to the device while an >>>> async update is in progress, and use cached values instead. When we >>>> receive the async update event, we should clear the cache. This will be >>>> the same for both well-behaved and badly-behaved devices, so we can >>>> expose the same behaviour towards userspace. >>> >>> seting ctrl->loaded = 0 when we get an event sounds like a good idea >>> and something we can implement right away. >>> If I have to resend the set I will add it to the end. >>> >>>> We possibly also need some kind of timeout mechanism to cope with the >>>> async update event not being delivered by the device. >>> >>> This is the part that worries me the most: >>> - timeouts make the code fragile >>> - What is a good value for timeout? 1 second, 30, 300? I do not think >>> that we can find a value. >> >> I've been thinking about the implementation of uvc_fh cleanup over the >> weekend, and having a timeout would have the nice advantage that we >> could reference-count uvc_fh instead of implementing a cleanup that >> walks over all controls when closing a file handle. I think it would >> make the code simpler, and possibly safer too. >> >>>> Regarding the userspace behaviour during an auto-update, we have >>>> multiple options: >>>> >>>> For control get, >>>> >>>> - We can return -EBUSY >>>> - We can return the old value from the cache > > This would match the control behavior best. Only when the operation is > done is the control updated and the control event sent. > > Some questions: is any of this documented for UVC? Because this is non-standard

No this isn't documented.

> behavior. Are there applications that rely on this? Should we perhaps add

I don't know.

> proper support for this to the control framework? E.g. add an ASYNC flag and > document this?

We could, but this is such a specific use case that I don't think is worth adding complexity to the already complex control framework would be worth it. What we could do is perhaps adding a flag for the userspace API, but even there, I never like modelling an API with a single user.

Well, it might be a single driver that uses this, but it is also the most used driver by far. I think the only change is to add a flag for this and describe how it should behave. And add v4l2-compliance tests for it.

Otherwise no changes to the control framework are needed, I think.

Controls with the ASYNC flag set would:

• return the old value from the cache.
• document that setting a new value while the operation is in progress results in EBUSY. Document that if the new value is equal to the old value, then return 0 and do nothing (alternative is to just immediately send the control changed event, but that might require a control framework change).
• when the operation finishes, update the cache to the new value and send the control changed event.
• document that userspace should specify V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK when subscribing to the control if you calling fh wants to know when the operation finishes.
• document how timeouts should be handled: this is tricky, especially with bad hardware. I.e. if the hw doesn't send the event, does that mean that you are never able to set the control since it will stall? In the end this will just reflect how UVC handles this.

I have been catching up on this thread (I have not read the v3 and v4 threads yet).

This all started with Ricardo noticing that ctrl->handle may get overwritten when another app sets the ctrl, causing the first app to set the ctrl to get a V4L2_EVENT for the ctrl (if subscribed) even though it set the ctrl itself.

My observations so far:

1. This is only hit when another app changes the ctrl after the first app,

in this case, if there is no stall issued by the hw for the second app's request, arguably the first app getting the event for the ctrl is correct

In other words, for non compliant cameras the current behaviour is correct. For compliant cameras it is broken.

...

since it was changed by the second app. IOW I think the current behavior is not only fine, but even desirable. Assuming we only override ctrl->handle after successfully sending the set-ctrl request to the hardware.

We are overriding ctrl->handle unconditionally, even if set-ctrl stalls.

Right I was just looking at that. Since we hold chain->ctrl_mutex from the start of uvc_ioctl_s_try_ext_ctrls() until we finish uvc_ctrl_commit() we can delay setting ctrl->handle until a successful commit.

Actually looking at this I think I've found another bug, uvc_ctrl_set() always sets ctrl->handle also in the VIDIOC_TRY_EXT_CTRLS case in which case we never send the SET_CUR command to the camera.

ack

...

And the handle is not part of the data backed up / restored by a rollback.

Moving the storing of the handle to a successful commit fixes both the overriding of the handle in the stall case as well as the bogus setting of the handle on VIDIOC_TRY_EXT_CTRLS.

So my suggestion would be not touching ctrl->handle from ctrl_set() instead store the handle in a new ctrl->new_handle variable there and copy the new_handle values to handle for affected controls after a successful commit (before releasing the lock).

We do not need to do that. Instead, we can move the set to uvc_ctrl_commit_entity().

I have a patchset ready with this change.

I find it fascinating that we're spending so much time discussing this to implement the control events API, which is both complex and not very useful in its current form :-) If I could, I'd drop all control events except for async controls, and wouldn't support not setting V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK for those. But that's a digression.

...

This will also successfully replace the handle for buggy devices which do not report a stall but instead accept the second SET_CUR. This replacement of handle is the correct thing todo in this case since after the second SET_CUR is accepted there is a change of the ctrl happening from the pov of the issuer of the first SET_CUR.

...

...

2. This adds a lot of complexity for not sending an event to the app

which made the change. Hans V. suggested maybe adding some sort of flag for async ctrls to the userspace API. I wonder if we should not just get rid of this complexity and document that these controls will always generate events independent of V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK ? That would certainly simplify things, but it raises the questions if this will cause issues for existing applications.

To be honest, I am more concerned about the dangling pointers than the event.

Ack.

...

Updating the doc to say that ASYC controls always generate events sounds good to me. But until we reach an agreement on the specifics I'd rather land this fix and then we can take time to design an API that works for compliant and non compliant hardware.

I agree that we should focus on fixing the dangling pointer problem and your v4 series is heading in the right direction there.

I'm not sure if we should take your v4 series as is though, see above.

At a minimum I think the issue with setting ctrl->handle in the VIDIOC_TRY_EXT_CTRLS case needs to be fixed.

We have three clear issues that are not related to changing behaviour of the async controls (aside from side effects of the fixes):

- Not setting ctrl->handle when trying controls - Not setting ctrl->handle when setting a control fails - Fixing the dangling pointer bug

The first two issues can probably be fixed with a single patch, and the third issue with a second patch. I don't mind much about the order, whatever makes development easier should be fine. We should backport both patches in any case.

...

...

...

Note that if we simply return -EBUSY on set until acked by a status event we also avoid the issue of ctrl->handle getting overwritten, but that relies on reliable status events; or requires timeout handling.

3. I agree with Ricardo that a timeout based approach for cameras which

to not properly send status events for async ctrls is going to be problematic. Things like pan/tilt homing can take multiple seconds which is really long to use as a timeout if we plan to return -EBUSY until the timeout triggers. I think it would be better to just rely on the hardware sending a stall, or it accepting and correctly handling a new CUR_SET command while the previous one is still being processed.

I guess we can track if the hw does send status events when async ctrls complete and then do the -EBUSY thing without going out to the hw after the first time an async ctrl has been acked by a status event.

That sounds quite complex, and wouldn't guard against the status event being occasionally lost. I'm more concerned about devices that only occasionally mess up sending the status event, not devices that never send it.

...

...

...

And then combine that with the current behavior of overwriting ctrl->handle until the ctrl has been marked as having working status events. So:

a) In case we do not know yet if a ctrl gets status-event acks; and on devices without reliable status events keep current behavior.

b) As soon as we know a ctrl has reliable status events, switch to returning -EBUSY if a set is pending (as indicated by ctrl->handle being set).

I don't like the fact that this changes the behavior after the first status event acking an async ctrl, but I don't really see another way.

If I understood you correctly, you are proposing the following quirk:

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index f0e8a436a306..1a554afeaa2f 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1132,6 +1132,9 @@ static int __uvc_ctrl_get(struct uvc_video_chain *chain, if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0) return -EACCES;

•   if (ctrl->handle && ctrl->async_event_works)
  

•           return -EBUSY;
  

•   ret = __uvc_ctrl_load_cur(chain, ctrl);
    if (ret < 0)
            return ret;
  

@@ -1672,6 +1675,8 @@ bool uvc_ctrl_status_event_async(struct urb> *urb, struct uvc_video_chain *chain, /* Flush the control cache, the data might have changed. */ ctrl->loaded = 0;

•   ctrl->async_event_works = true;
  

•   if (list_empty(&ctrl->info.mappings))
            return false;
  
  

@@ -1982,6 +1987,9 @@ int uvc_ctrl_set(struct uvc_fh *handle, if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR)) return -EACCES;

•   if (ctrl->handle && ctrl->async_event_works)
  

•           return -EBUSY;
  

Yes this is what I'm proposing, except that this check should be skipped in the VIDIOC_TRY_EXT_CTRLS case. I think we need to add a "bool try" parameter to uvc_ctrl_set() and not look at / set [new_]handle when this is set.

SGTM. Let's add this improvement later.

Let's handle behavioural changes, optimizations, and handling of broken devices on top of the fixes, it's getting complex enough as-is.

...

...

    /* Clamp out of range values. */
    switch (mapping->v4l2_type) {
    case V4L2_CTRL_TYPE_INTEGER:

diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index e0e4f099a210..0ef7c594eecb 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -154,6 +154,7 @@ struct uvc_control { * File handle that initially changed the * async control. */

•   bool async_event_works;
  

};

The benefit is that we can predict a device returning STALL without having to actually do the set/get operation.

We can add it as a follow-up patch.

Another benefit would be correctly returning -EBUSY when trying to get the ctrl. I agree this could be done as a follow-up.

On 29/11/2024 12:06, Laurent Pinchart wrote:

On Fri, Nov 29, 2024 at 11:59:27AM +0100, Ricardo Ribalda wrote:

...

On Fri, 29 Nov 2024 at 11:36, Hans Verkuil wrote:

...

On 28/11/2024 23:33, Laurent Pinchart wrote:

...

On Thu, Nov 28, 2024 at 11:28:29PM +0100, Ricardo Ribalda wrote:

...

On Thu, 28 Nov 2024 at 23:22, Laurent Pinchart wrote:

...

Hi Ricardo,

(CC'ing Hans Verkuil)

Thank you for the patch.

On Wed, Nov 27, 2024 at 12:14:50PM +0000, Ricardo Ribalda wrote: > If a file handle is waiting for a response from an async control, avoid > that other file handle operate with it. > > Without this patch, the first file handle will never get the event > associated with that operation, which can lead to endless loops in > applications. Eg: > If an application A wants to change the zoom and to know when the > operation has completed: > it will open the video node, subscribe to the zoom event, change the > control and wait for zoom to finish. > If before the zoom operation finishes, another application B changes > the zoom, the first app A will loop forever.

Hans, the V4L2 specification isn't very clear on this. I see pros and cons for both behaviours, with a preference for the current behaviour, as with this patch the control will remain busy until the file handle is closed if the device doesn't send the control event for any reason. What do you think ?

Just one small clarification. The same file handler can change the value of the async control as many times as it wants, even if the operation has not finished.

It will be other file handles that will get -EBUSY if they try to use an async control with an unfinished operation started by another fh.

Yes, I should have been more precised. If the device doesn't send the control event, then all other file handles will be prevented from setting the control until the file handle that set it first gets closed.

I think I need a bit more background here:

First of all, what is an asynchronous control in UVC? I think that means you can set it, but it takes time for that operation to finish, so you get an event later when the operation is done. So zoom and similar operations are examples of that.

And only when the operation finishes will the control event be sent, correct?

You are correct. This diagrams from the spec is more or less clear: https://ibb.co/MDGn7F3

...

While the operation is ongoing, if you query the control value, is that reporting the current position or the final position?

I'd expect hardware will return either the current position, the start position or the final position. I could not find anything in the spec that points in one direction or the others.

Figure 2-21 in UVC 1.5 indicates that the device should STALL the GET_CUR and SET_CUR requests if a state change is in progress.

...

And in the driver I believe that we might have a bug handling this case (will send a patch if I can confirm it) the zoom is at 0 and you set it 10 if you read the value 2 times before the camera reaches value 10:

• First value will come from the hardware and the response will be cached

Only if the control doesn't have the auto-update flag set, so it will be device-dependent. As GET_CUR should stall that's not really relevant, except for the fact that devices may not stall the request.

OK, that helps a lot.

If an operation is in progress, then setting a new control value should result in -EBUSY. Based on the description above, I gather that even the same fh that made the request cannot update it while the operation is ongoing?

Getting the control should just return the value that was set. I assume that is cached in uvc?

Regards,

Hans

...

• Second value will be the cached one

now the camera is at zoom 10 If you read the value, you will read the cached value

...

E.g.: the zoom control is at value 0 and I set it to 10, then I poll the zoom control value: will that report the intermediate values until it reaches 10? And when it is at 10, the control event is sent?

...

...

...

> Cc: stable@vger.kernel.org > Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives") > Signed-off-by: Ricardo Ribalda ribalda@chromium.org > --- > drivers/media/usb/uvc/uvc_ctrl.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > index b6af4ff92cbd..3f8ae35cb3bc 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -1955,6 +1955,10 @@ int uvc_ctrl_set(struct uvc_fh *handle, > if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR)) > return -EACCES; > > + /* Other file handle is waiting a response from this async control. */ > + if (ctrl->handle && ctrl->handle != handle) > + return -EBUSY; > + > /* Clamp out of range values. */ > switch (mapping->v4l2_type) { > case V4L2_CTRL_TYPE_INTEGER:

On Fri, 29 Nov 2024 at 14:10, Hans Verkuil hverkuil-cisco@xs4all.nl wrote:

On 29/11/2024 12:06, Laurent Pinchart wrote:

...

On Fri, Nov 29, 2024 at 11:59:27AM +0100, Ricardo Ribalda wrote:

...

On Fri, 29 Nov 2024 at 11:36, Hans Verkuil wrote:

...

On 28/11/2024 23:33, Laurent Pinchart wrote:

...

On Thu, Nov 28, 2024 at 11:28:29PM +0100, Ricardo Ribalda wrote:

...

On Thu, 28 Nov 2024 at 23:22, Laurent Pinchart wrote: > > Hi Ricardo, > > (CC'ing Hans Verkuil) > > Thank you for the patch. > > On Wed, Nov 27, 2024 at 12:14:50PM +0000, Ricardo Ribalda wrote: >> If a file handle is waiting for a response from an async control, avoid >> that other file handle operate with it. >> >> Without this patch, the first file handle will never get the event >> associated with that operation, which can lead to endless loops in >> applications. Eg: >> If an application A wants to change the zoom and to know when the >> operation has completed: >> it will open the video node, subscribe to the zoom event, change the >> control and wait for zoom to finish. >> If before the zoom operation finishes, another application B changes >> the zoom, the first app A will loop forever. > > Hans, the V4L2 specification isn't very clear on this. I see pros and > cons for both behaviours, with a preference for the current behaviour, > as with this patch the control will remain busy until the file handle is > closed if the device doesn't send the control event for any reason. What > do you think ?

Just one small clarification. The same file handler can change the value of the async control as many times as it wants, even if the operation has not finished.

It will be other file handles that will get -EBUSY if they try to use an async control with an unfinished operation started by another fh.

Yes, I should have been more precised. If the device doesn't send the control event, then all other file handles will be prevented from setting the control until the file handle that set it first gets closed.

I think I need a bit more background here:

First of all, what is an asynchronous control in UVC? I think that means you can set it, but it takes time for that operation to finish, so you get an event later when the operation is done. So zoom and similar operations are examples of that.

And only when the operation finishes will the control event be sent, correct?

You are correct. This diagrams from the spec is more or less clear: https://ibb.co/MDGn7F3

...

While the operation is ongoing, if you query the control value, is that reporting the current position or the final position?

I'd expect hardware will return either the current position, the start position or the final position. I could not find anything in the spec that points in one direction or the others.

Figure 2-21 in UVC 1.5 indicates that the device should STALL the GET_CUR and SET_CUR requests if a state change is in progress.

...

And in the driver I believe that we might have a bug handling this case (will send a patch if I can confirm it) the zoom is at 0 and you set it 10 if you read the value 2 times before the camera reaches value 10:

• First value will come from the hardware and the response will be cached

Only if the control doesn't have the auto-update flag set, so it will be device-dependent. As GET_CUR should stall that's not really relevant, except for the fact that devices may not stall the request.

OK, that helps a lot.

If an operation is in progress, then setting a new control value should result in -EBUSY. Based on the description above, I gather that even the same fh that made the request cannot update it while the operation is ongoing?

That is correct according to the spec. But both Laurent (correct me if I am wrong) and me suspect that there are devices that do not implement this properly.

Getting the control should just return the value that was set. I assume that is cached in uvc?

If I get the code right... we only return the cached value when the field "loaded" is true. That happens when we read the device. So if the driver is just loaded

write(full controlA) read(full controlA) read(full controlA)

The first read will get the value from the hardware, the second will be cached.

Regards,

    Hans

...

...

• Second value will be the cached one

now the camera is at zoom 10 If you read the value, you will read the cached value

...

E.g.: the zoom control is at value 0 and I set it to 10, then I poll the zoom control value: will that report the intermediate values until it reaches 10? And when it is at 10, the control event is sent?

...

...

>> Cc: stable@vger.kernel.org >> Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives") >> Signed-off-by: Ricardo Ribalda ribalda@chromium.org >> --- >> drivers/media/usb/uvc/uvc_ctrl.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c >> index b6af4ff92cbd..3f8ae35cb3bc 100644 >> --- a/drivers/media/usb/uvc/uvc_ctrl.c >> +++ b/drivers/media/usb/uvc/uvc_ctrl.c >> @@ -1955,6 +1955,10 @@ int uvc_ctrl_set(struct uvc_fh *handle, >> if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR)) >> return -EACCES; >> >> + /* Other file handle is waiting a response from this async control. */ >> + if (ctrl->handle && ctrl->handle != handle) >> + return -EBUSY; >> + >> /* Clamp out of range values. */ >> switch (mapping->v4l2_type) { >> case V4L2_CTRL_TYPE_INTEGER:

-- Ricardo Ribalda

On Fri, Nov 29, 2024 at 11:36:36AM +0100, Hans Verkuil wrote:

Hi Laurent, Ricardo,

On 28/11/2024 23:33, Laurent Pinchart wrote:

...

On Thu, Nov 28, 2024 at 11:28:29PM +0100, Ricardo Ribalda wrote:

...

On Thu, 28 Nov 2024 at 23:22, Laurent Pinchart wrote:

...

Hi Ricardo,

(CC'ing Hans Verkuil)

Thank you for the patch.

On Wed, Nov 27, 2024 at 12:14:50PM +0000, Ricardo Ribalda wrote:

...

If a file handle is waiting for a response from an async control, avoid that other file handle operate with it.

Without this patch, the first file handle will never get the event associated with that operation, which can lead to endless loops in applications. Eg: If an application A wants to change the zoom and to know when the operation has completed: it will open the video node, subscribe to the zoom event, change the control and wait for zoom to finish. If before the zoom operation finishes, another application B changes the zoom, the first app A will loop forever.

Hans, the V4L2 specification isn't very clear on this. I see pros and cons for both behaviours, with a preference for the current behaviour, as with this patch the control will remain busy until the file handle is closed if the device doesn't send the control event for any reason. What do you think ?

Just one small clarification. The same file handler can change the value of the async control as many times as it wants, even if the operation has not finished.

It will be other file handles that will get -EBUSY if they try to use an async control with an unfinished operation started by another fh.

Yes, I should have been more precised. If the device doesn't send the control event, then all other file handles will be prevented from setting the control until the file handle that set it first gets closed.

I think I need a bit more background here:

First of all, what is an asynchronous control in UVC? I think that means you can set it, but it takes time for that operation to finish, so you get an event later when the operation is done. So zoom and similar operations are examples of that.

Correct. Physical PTZ (pan/tilt/zoom) is the prime use case. The UVC specification states that any control that requires more than 10ms to respond to a SET_CUR request must be reported by the device as an asynchronous control, but does not tell which control must or must not be asynchronous.

And only when the operation finishes will the control event be sent, correct?

Correct.

While the operation is ongoing, if you query the control value, is that reporting the current position or the final position?

The UVC specification indicates that a GET_CUR or SET_CUR request on an asynchronous control will stall until the control value has been applied. This means you won't be able to poll the control value.

In practice, devices often take liberties with the implementation of the specification, so some may let you get or set the value of an asynchronous control while a SET_CUR request is in progress.

E.g.: the zoom control is at value 0 and I set it to 10, then I poll the zoom control value: will that report the intermediate values until it reaches 10? And when it is at 10, the control event is sent?

...

...

...

...

Cc: stable@vger.kernel.org Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives") Signed-off-by: Ricardo Ribalda ribalda@chromium.org

---

drivers/media/usb/uvc/uvc_ctrl.c | 4 ++++ 1 file changed, 4 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index b6af4ff92cbd..3f8ae35cb3bc 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1955,6 +1955,10 @@ int uvc_ctrl_set(struct uvc_fh *handle, if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR)) return -EACCES;

• /* Other file handle is waiting a response from this async control. */
  

• if (ctrl->handle && ctrl->handle != handle)
  

•         return -EBUSY;
  

• /* Clamp out of range values. */
  switch (mapping->v4l2_type) {
  case V4L2_CTRL_TYPE_INTEGER: