[PATCH v6 0/5] media: uvcvideo: Two +1 fixes for async controls

[PATCH v6 0/5] media: uvcvideo: Two +1 fixes for async controls

[Ricardo Ribalda]

This patchset fixes two +1 bugs with the async controls for the uvc driver.

They were found while implementing the granular PM, but I am sending
them as a separate patches, so they can be reviewed sooner. They fix
real issues in the driver that need to be taken care.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Changes in v6:
- Swap order of patches
- Use uvc_ctrl_set_handle again
- Move loaded=0 to uvc_ctrl_status_event()
- Link to v5: https://lore.kernel.org/r/20241202-uvc-fix-async-v5-0-6658c1fe312b@chromium.org

Changes in v5:
- Move set handle to the entity_commit
- Replace uvc_ctrl_set_handle with get/put_handle.
- Add a patch to flush the cache of async controls.
- Link to v4: https://lore.kernel.org/r/20241129-uvc-fix-async-v4-0-f23784dba80f@chromium.org

Changes in v4:
- Fix implementation of uvc_ctrl_set_handle.
- Link to v3: https://lore.kernel.org/r/20241129-uvc-fix-async-v3-0-ab675ce66db7@chromium.org

Changes in v3:
- change again! order of patches.
- Introduce uvc_ctrl_set_handle.
- Do not change ctrl->handle if it is not NULL.

Changes in v2:
- Annotate lockdep
- ctrl->handle != handle
- Change order of patches
- Move documentation of mutex
- Link to v1: https://lore.kernel.org/r/20241127-uvc-fix-async-v1-0-eb8722531b8c@chromium.org

---
Ricardo Ribalda (5):
      media: uvcvideo: Only save async fh if success
      media: uvcvideo: Remove redundant NULL assignment
      media: uvcvideo: Remove dangling pointers
      media: uvcvideo: Annotate lock requirements for uvc_ctrl_set
      media: uvcvideo: Flush the control cache when we get an event

 drivers/media/usb/uvc/uvc_ctrl.c | 83 ++++++++++++++++++++++++++++++++++------
 drivers/media/usb/uvc/uvc_v4l2.c |  2 +
 drivers/media/usb/uvc/uvcvideo.h |  9 ++++-
 3 files changed, 82 insertions(+), 12 deletions(-)
---
base-commit: 291a8d98186f0a704cb954855d2ae3233971f07d
change-id: 20241127-uvc-fix-async-2c9d40413ad8

Best regards,
-- 
Ricardo Ribalda <ribalda@chromium.org>

[PATCH v6 1/5] media: uvcvideo: Only save async fh if success

[Ricardo Ribalda]

Now we keep a reference to the active fh for any call to uvc_ctrl_set,
regardless if it is an actual set or if it is a just a try or if the
device refused the operation.

We should only keep the file handle if the device actually accepted
applying the operation.

Cc: stable@vger.kernel.org
Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives")
Suggested-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 4fe26e82e3d1..9a80a7d8e73a 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1811,7 +1811,10 @@ int uvc_ctrl_begin(struct uvc_video_chain *chain)
 }
 
 static int uvc_ctrl_commit_entity(struct uvc_device *dev,
-	struct uvc_entity *entity, int rollback, struct uvc_control **err_ctrl)
+				  struct uvc_fh *handle,
+				  struct uvc_entity *entity,
+				  int rollback,
+				  struct uvc_control **err_ctrl)
 {
 	struct uvc_control *ctrl;
 	unsigned int i;
@@ -1859,6 +1862,10 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
 				*err_ctrl = ctrl;
 			return ret;
 		}
+
+		if (!rollback && handle &&
+		    ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
+			ctrl->handle = handle;
 	}
 
 	return 0;
@@ -1895,8 +1902,8 @@ int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
 
 	/* Find the control. */
 	list_for_each_entry(entity, &chain->entities, chain) {
-		ret = uvc_ctrl_commit_entity(chain->dev, entity, rollback,
-					     &err_ctrl);
+		ret = uvc_ctrl_commit_entity(chain->dev, handle, entity,
+					     rollback, &err_ctrl);
 		if (ret < 0) {
 			if (ctrls)
 				ctrls->error_idx =
@@ -2046,9 +2053,6 @@ 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;
-
 	ctrl->dirty = 1;
 	ctrl->modified = 1;
 	return 0;
@@ -2377,7 +2381,7 @@ int uvc_ctrl_restore_values(struct uvc_device *dev)
 			ctrl->dirty = 1;
 		}
 
-		ret = uvc_ctrl_commit_entity(dev, entity, 0, NULL);
+		ret = uvc_ctrl_commit_entity(dev, NULL, entity, 0, NULL);
 		if (ret < 0)
 			return ret;
 	}

-- 
2.47.0.338.g60cca15819-goog

[PATCH v6 2/5] media: uvcvideo: Remove redundant NULL assignment

[Ricardo Ribalda]

ctrl->handle will only be different than NULL for controls that have
mappings. This is because that assignment is only done inside
uvc_ctrl_set() for mapped controls.

Cc: stable@vger.kernel.org
Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives")
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 9a80a7d8e73a..42b0a0cdc51c 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1640,10 +1640,8 @@ bool uvc_ctrl_status_event_async(struct urb *urb, struct uvc_video_chain *chain,
 	struct uvc_device *dev = chain->dev;
 	struct uvc_ctrl_work *w = &dev->async_ctrl;
 
-	if (list_empty(&ctrl->info.mappings)) {
-		ctrl->handle = NULL;
+	if (list_empty(&ctrl->info.mappings))
 		return false;
-	}
 
 	w->data = data;
 	w->urb = urb;

-- 
2.47.0.338.g60cca15819-goog

[PATCH v6 3/5] media: uvcvideo: Remove dangling pointers

[Ricardo Ribalda]

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
that it is properly handled.

Cc: stable@vger.kernel.org
Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives")
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 58 ++++++++++++++++++++++++++++++++++++++--
 drivers/media/usb/uvc/uvc_v4l2.c |  2 ++
 drivers/media/usb/uvc/uvcvideo.h |  9 ++++++-
 3 files changed, 66 insertions(+), 3 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 42b0a0cdc51c..def502195528 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1579,6 +1579,40 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain,
 	uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes);
 }
 
+static void uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl,
+				struct uvc_fh *new_handle)
+{
+	lockdep_assert_held(&handle->chain->ctrl_mutex);
+
+	if (new_handle) {
+		if (ctrl->handle)
+			dev_warn_ratelimited(&handle->stream->dev->udev->dev,
+					     "UVC non compliance: Setting an async control with a pending operation.");
+
+		if (new_handle == ctrl->handle)
+			return;
+
+		if (ctrl->handle) {
+			WARN_ON(!ctrl->handle->pending_async_ctrls);
+			if (ctrl->handle->pending_async_ctrls)
+				ctrl->handle->pending_async_ctrls--;
+		}
+
+		ctrl->handle = new_handle;
+		handle->pending_async_ctrls++;
+		return;
+	}
+
+	/* Cannot clear the handle for a control not owned by us.*/
+	if (WARN_ON(ctrl->handle != handle))
+		return;
+
+	ctrl->handle = NULL;
+	if (WARN_ON(!handle->pending_async_ctrls))
+		return;
+	handle->pending_async_ctrls--;
+}
+
 void uvc_ctrl_status_event(struct uvc_video_chain *chain,
 			   struct uvc_control *ctrl, const u8 *data)
 {
@@ -1589,7 +1623,8 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain,
 	mutex_lock(&chain->ctrl_mutex);
 
 	handle = ctrl->handle;
-	ctrl->handle = NULL;
+	if (handle)
+		uvc_ctrl_set_handle(handle, ctrl, NULL);
 
 	list_for_each_entry(mapping, &ctrl->info.mappings, list) {
 		s32 value = __uvc_ctrl_get_value(mapping, data);
@@ -1863,7 +1898,7 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
 
 		if (!rollback && handle &&
 		    ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
-			ctrl->handle = handle;
+			uvc_ctrl_set_handle(handle, ctrl, handle);
 	}
 
 	return 0;
@@ -2772,6 +2807,25 @@ 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) {
+			if (entity->controls[i].handle != handle)
+				continue;
+			uvc_ctrl_set_handle(handle, &entity->controls[i], NULL);
+		}
+
+	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..92ecdd188587 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -337,7 +337,11 @@ 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,
+						 * ctrl.handle and
+						 * uvc_fh.pending_async_ctrls
+						 */
 
 	struct v4l2_prio_state prio;		/* V4L2 priority state */
 	u32 caps;				/* V4L2 chain-wide caps */
@@ -612,6 +616,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 +802,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);

-- 
2.47.0.338.g60cca15819-goog

[PATCH v6 4/5] media: uvcvideo: Annotate lock requirements for uvc_ctrl_set

[Ricardo Ribalda]

Make it explicit that the function is always called with ctrl_mutex
being held.

Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index def502195528..3dc9b7a49f64 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1981,6 +1981,8 @@ int uvc_ctrl_set(struct uvc_fh *handle,
 	s32 max;
 	int ret;
 
+	lockdep_assert_held(&chain->ctrl_mutex);
+
 	if (__uvc_query_v4l2_class(chain, xctrl->id, 0) >= 0)
 		return -EACCES;
 

-- 
2.47.0.338.g60cca15819-goog

[PATCH v6 5/5] media: uvcvideo: Flush the control cache when we get an event

[Ricardo Ribalda]

Asynchronous controls trigger an event when they have completed their
operation.

This can make that the control cached value does not match the value in
the device.

Let's flush the cache to be on the safe side.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 3dc9b7a49f64..db29e0e8bfd4 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1622,6 +1622,9 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain,
 
 	mutex_lock(&chain->ctrl_mutex);
 
+	/* Flush the control cache, the data might have changed. */
+	ctrl->loaded = 0;
+
 	handle = ctrl->handle;
 	if (handle)
 		uvc_ctrl_set_handle(handle, ctrl, NULL);

-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH v6 3/5] media: uvcvideo: Remove dangling pointers

[Ricardo Ribalda]

On Tue, 3 Dec 2024 at 22:20, Ricardo Ribalda <ribalda@chromium.org> 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
> that it is properly handled.
>
> Cc: stable@vger.kernel.org
> Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives")
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 58 ++++++++++++++++++++++++++++++++++++++--
>  drivers/media/usb/uvc/uvc_v4l2.c |  2 ++
>  drivers/media/usb/uvc/uvcvideo.h |  9 ++++++-
>  3 files changed, 66 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 42b0a0cdc51c..def502195528 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1579,6 +1579,40 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain,
>         uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes);
>  }
>
> +static void uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl,
> +                               struct uvc_fh *new_handle)
> +{
> +       lockdep_assert_held(&handle->chain->ctrl_mutex);
> +
> +       if (new_handle) {
> +               if (ctrl->handle)
> +                       dev_warn_ratelimited(&handle->stream->dev->udev->dev,
> +                                            "UVC non compliance: Setting an async control with a pending operation.");
> +
> +               if (new_handle == ctrl->handle)
> +                       return;
> +
> +               if (ctrl->handle) {
> +                       WARN_ON(!ctrl->handle->pending_async_ctrls);
> +                       if (ctrl->handle->pending_async_ctrls)
> +                               ctrl->handle->pending_async_ctrls--;
> +               }
> +
> +               ctrl->handle = new_handle;
> +               handle->pending_async_ctrls++;
> +               return;
> +       }
> +
> +       /* Cannot clear the handle for a control not owned by us.*/
> +       if (WARN_ON(ctrl->handle != handle))
> +               return;
> +
> +       ctrl->handle = NULL;
> +       if (WARN_ON(!handle->pending_async_ctrls))
> +               return;
> +       handle->pending_async_ctrls--;
> +}

Laurent,


If I have to redo the patch... would you be open to have two functions:
uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl)
uvc_ctrl_clear_handle(struct uvc_fh *handle, struct uvc_control *ctrl)

instead of this one? It might be me, but it looks uglier than before.

If you like this code just disregard this message.

Regards!


> +
>  void uvc_ctrl_status_event(struct uvc_video_chain *chain,
>                            struct uvc_control *ctrl, const u8 *data)
>  {
> @@ -1589,7 +1623,8 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain,
>         mutex_lock(&chain->ctrl_mutex);
>
>         handle = ctrl->handle;
> -       ctrl->handle = NULL;
> +       if (handle)
> +               uvc_ctrl_set_handle(handle, ctrl, NULL);
>
>         list_for_each_entry(mapping, &ctrl->info.mappings, list) {
>                 s32 value = __uvc_ctrl_get_value(mapping, data);
> @@ -1863,7 +1898,7 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
>
>                 if (!rollback && handle &&
>                     ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
> -                       ctrl->handle = handle;
> +                       uvc_ctrl_set_handle(handle, ctrl, handle);
>         }
>
>         return 0;
> @@ -2772,6 +2807,25 @@ 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) {
> +                       if (entity->controls[i].handle != handle)
> +                               continue;
> +                       uvc_ctrl_set_handle(handle, &entity->controls[i], NULL);
> +               }
> +
> +       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..92ecdd188587 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -337,7 +337,11 @@ 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,
> +                                                * ctrl.handle and
> +                                                * uvc_fh.pending_async_ctrls
> +                                                */
>
>         struct v4l2_prio_state prio;            /* V4L2 priority state */
>         u32 caps;                               /* V4L2 chain-wide caps */
> @@ -612,6 +616,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 +802,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);
>
> --
> 2.47.0.338.g60cca15819-goog
>


-- 
Ricardo Ribalda

Re: [PATCH v6 0/5] media: uvcvideo: Two +1 fixes for async controls

[Hans de Goede]

Hi,

On 3-Dec-24 10:20 PM, Ricardo Ribalda wrote:
> This patchset fixes two +1 bugs with the async controls for the uvc driver.
> 
> They were found while implementing the granular PM, but I am sending
> them as a separate patches, so they can be reviewed sooner. They fix
> real issues in the driver that need to be taken care.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Ricardo, Thank you for your patches.

I have merged patches 1-4 into:

https://gitlab.freedesktop.org/linux-media/users/uvc/-/commits/next/

now.

Regards,

Hans



> ---
> Changes in v6:
> - Swap order of patches
> - Use uvc_ctrl_set_handle again
> - Move loaded=0 to uvc_ctrl_status_event()
> - Link to v5: https://lore.kernel.org/r/20241202-uvc-fix-async-v5-0-6658c1fe312b@chromium.org
> 
> Changes in v5:
> - Move set handle to the entity_commit
> - Replace uvc_ctrl_set_handle with get/put_handle.
> - Add a patch to flush the cache of async controls.
> - Link to v4: https://lore.kernel.org/r/20241129-uvc-fix-async-v4-0-f23784dba80f@chromium.org
> 
> Changes in v4:
> - Fix implementation of uvc_ctrl_set_handle.
> - Link to v3: https://lore.kernel.org/r/20241129-uvc-fix-async-v3-0-ab675ce66db7@chromium.org
> 
> Changes in v3:
> - change again! order of patches.
> - Introduce uvc_ctrl_set_handle.
> - Do not change ctrl->handle if it is not NULL.
> 
> Changes in v2:
> - Annotate lockdep
> - ctrl->handle != handle
> - Change order of patches
> - Move documentation of mutex
> - Link to v1: https://lore.kernel.org/r/20241127-uvc-fix-async-v1-0-eb8722531b8c@chromium.org
> 
> ---
> Ricardo Ribalda (5):
>       media: uvcvideo: Only save async fh if success
>       media: uvcvideo: Remove redundant NULL assignment
>       media: uvcvideo: Remove dangling pointers
>       media: uvcvideo: Annotate lock requirements for uvc_ctrl_set
>       media: uvcvideo: Flush the control cache when we get an event
> 
>  drivers/media/usb/uvc/uvc_ctrl.c | 83 ++++++++++++++++++++++++++++++++++------
>  drivers/media/usb/uvc/uvc_v4l2.c |  2 +
>  drivers/media/usb/uvc/uvcvideo.h |  9 ++++-
>  3 files changed, 82 insertions(+), 12 deletions(-)
> ---
> base-commit: 291a8d98186f0a704cb954855d2ae3233971f07d
> change-id: 20241127-uvc-fix-async-2c9d40413ad8
> 
> Best regards,

Re: [PATCH v6 5/5] media: uvcvideo: Flush the control cache when we get an event

[Hans de Goede]

Hi Ricardo,

On 3-Dec-24 10:20 PM, Ricardo Ribalda wrote:
> Asynchronous controls trigger an event when they have completed their
> operation.
> 
> This can make that the control cached value does not match the value in
> the device.
> 
> Let's flush the cache to be on the safe side.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Thank you for your patch.

It seems that you have missed Laurent's reply asking to improve the commit message:

"Conceptually this change looks fine, but the commit message needs to
explain why this is safe to do without protecting ctrl->loaded with a
lock."

https://lore.kernel.org/linux-media/20241203203748.GD5196@pendragon.ideasonboard.com/

Or maybe the posting of this v6 and that reply have crossed each other.

Either way please post a new version addressing this comment.

Thanks & Regards,

Hans



> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 3dc9b7a49f64..db29e0e8bfd4 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1622,6 +1622,9 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain,
>  
>  	mutex_lock(&chain->ctrl_mutex);
>  
> +	/* Flush the control cache, the data might have changed. */
> +	ctrl->loaded = 0;
> +
>  	handle = ctrl->handle;
>  	if (handle)
>  		uvc_ctrl_set_handle(handle, ctrl, NULL);
> 

Re: [PATCH v6 5/5] media: uvcvideo: Flush the control cache when we get an event

[Ricardo Ribalda]

Hi Hans

On Mon, 9 Dec 2024 at 12:03, Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Ricardo,
>
> On 3-Dec-24 10:20 PM, Ricardo Ribalda wrote:
> > Asynchronous controls trigger an event when they have completed their
> > operation.
> >
> > This can make that the control cached value does not match the value in
> > the device.
> >
> > Let's flush the cache to be on the safe side.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>
> Thank you for your patch.
>
> It seems that you have missed Laurent's reply asking to improve the commit message:
>
> "Conceptually this change looks fine, but the commit message needs to
> explain why this is safe to do without protecting ctrl->loaded with a
> lock."
>
> https://lore.kernel.org/linux-media/20241203203748.GD5196@pendragon.ideasonboard.com/
>
> Or maybe the posting of this v6 and that reply have crossed each other.

In this v6 I moved loaded=0 from uvc_ctrl_status_event_async() to
uvc_ctrl_status_event()

Now setting loaded=0 is just after mutex_lock(&chain->ctrl_mutex);

Do we need a new version?

>
> Either way please post a new version addressing this comment.
>
> Thanks & Regards,
>
> Hans
>
>
>
> > ---
> >  drivers/media/usb/uvc/uvc_ctrl.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index 3dc9b7a49f64..db29e0e8bfd4 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -1622,6 +1622,9 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain,
> >
> >       mutex_lock(&chain->ctrl_mutex);
> >
> > +     /* Flush the control cache, the data might have changed. */
> > +     ctrl->loaded = 0;
> > +
> >       handle = ctrl->handle;
> >       if (handle)
> >               uvc_ctrl_set_handle(handle, ctrl, NULL);
> >
>


-- 
Ricardo Ribalda

Re: [PATCH v6 5/5] media: uvcvideo: Flush the control cache when we get an event

[Hans de Goede]

Hi,

On 9-Dec-24 12:31 PM, Ricardo Ribalda wrote:
> Hi Hans
> 
> On Mon, 9 Dec 2024 at 12:03, Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi Ricardo,
>>
>> On 3-Dec-24 10:20 PM, Ricardo Ribalda wrote:
>>> Asynchronous controls trigger an event when they have completed their
>>> operation.
>>>
>>> This can make that the control cached value does not match the value in
>>> the device.
>>>
>>> Let's flush the cache to be on the safe side.
>>>
>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>
>> Thank you for your patch.
>>
>> It seems that you have missed Laurent's reply asking to improve the commit message:
>>
>> "Conceptually this change looks fine, but the commit message needs to
>> explain why this is safe to do without protecting ctrl->loaded with a
>> lock."
>>
>> https://lore.kernel.org/linux-media/20241203203748.GD5196@pendragon.ideasonboard.com/
>>
>> Or maybe the posting of this v6 and that reply have crossed each other.
> 
> In this v6 I moved loaded=0 from uvc_ctrl_status_event_async() to
> uvc_ctrl_status_event()

Ah I missed that you moved it, my bad.

> Now setting loaded=0 is just after mutex_lock(&chain->ctrl_mutex);
> 
> Do we need a new version?

Since the setting is now clearly done under the lock the new version seems
good to me as is.

So I have now merged this into:

https://gitlab.freedesktop.org/linux-media/users/uvc/-/commits/next/

Regards,

Hans

> 
>>
>> Either way please post a new version addressing this comment.
>>
>> Thanks & Regards,
>>
>> Hans
>>
>>
>>
>>> ---
>>>  drivers/media/usb/uvc/uvc_ctrl.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
>>> index 3dc9b7a49f64..db29e0e8bfd4 100644
>>> --- a/drivers/media/usb/uvc/uvc_ctrl.c
>>> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
>>> @@ -1622,6 +1622,9 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain,
>>>
>>>       mutex_lock(&chain->ctrl_mutex);
>>>
>>> +     /* Flush the control cache, the data might have changed. */
>>> +     ctrl->loaded = 0;
>>> +
>>>       handle = ctrl->handle;
>>>       if (handle)
>>>               uvc_ctrl_set_handle(handle, ctrl, NULL);
>>>
>>
> 
> 

Re: [PATCH v6 3/5] media: uvcvideo: Remove dangling pointers

[Laurent Pinchart]

On Wed, Dec 04, 2024 at 01:10:14AM +0100, Ricardo Ribalda wrote:
> On Tue, 3 Dec 2024 at 22:20, 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
> > that it is properly handled.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives")
> > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_ctrl.c | 58 ++++++++++++++++++++++++++++++++++++++--
> >  drivers/media/usb/uvc/uvc_v4l2.c |  2 ++
> >  drivers/media/usb/uvc/uvcvideo.h |  9 ++++++-
> >  3 files changed, 66 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index 42b0a0cdc51c..def502195528 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -1579,6 +1579,40 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain,
> >         uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes);
> >  }
> >
> > +static void uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl,
> > +                               struct uvc_fh *new_handle)
> > +{
> > +       lockdep_assert_held(&handle->chain->ctrl_mutex);
> > +
> > +       if (new_handle) {
> > +               if (ctrl->handle)
> > +                       dev_warn_ratelimited(&handle->stream->dev->udev->dev,
> > +                                            "UVC non compliance: Setting an async control with a pending operation.");
> > +
> > +               if (new_handle == ctrl->handle)
> > +                       return;
> > +
> > +               if (ctrl->handle) {
> > +                       WARN_ON(!ctrl->handle->pending_async_ctrls);
> > +                       if (ctrl->handle->pending_async_ctrls)
> > +                               ctrl->handle->pending_async_ctrls--;
> > +               }
> > +
> > +               ctrl->handle = new_handle;
> > +               handle->pending_async_ctrls++;
> > +               return;
> > +       }
> > +
> > +       /* Cannot clear the handle for a control not owned by us.*/
> > +       if (WARN_ON(ctrl->handle != handle))
> > +               return;
> > +
> > +       ctrl->handle = NULL;
> > +       if (WARN_ON(!handle->pending_async_ctrls))
> > +               return;
> > +       handle->pending_async_ctrls--;
> > +}
> 
> Laurent,
> 
> 
> If I have to redo the patch... would you be open to have two functions:
> uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl)
> uvc_ctrl_clear_handle(struct uvc_fh *handle, struct uvc_control *ctrl)
> 
> instead of this one? It might be me, but it looks uglier than before.

I'd be OK with that naming. Hans has merged the series already though,
and I don't expect you'd like to delay upstreaming :-) We can always
clean things up on top, but I'd rather not address this unless you need
to rework this specific part of the driver, as there are more important
changes to upstream and additional syntaxic beautification patches would
just slow things down at this point.

> If you like this code just disregard this message.
> 
> Regards!
> 
> 
> > +
> >  void uvc_ctrl_status_event(struct uvc_video_chain *chain,
> >                            struct uvc_control *ctrl, const u8 *data)
> >  {
> > @@ -1589,7 +1623,8 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain,
> >         mutex_lock(&chain->ctrl_mutex);
> >
> >         handle = ctrl->handle;
> > -       ctrl->handle = NULL;
> > +       if (handle)
> > +               uvc_ctrl_set_handle(handle, ctrl, NULL);
> >
> >         list_for_each_entry(mapping, &ctrl->info.mappings, list) {
> >                 s32 value = __uvc_ctrl_get_value(mapping, data);
> > @@ -1863,7 +1898,7 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
> >
> >                 if (!rollback && handle &&
> >                     ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
> > -                       ctrl->handle = handle;
> > +                       uvc_ctrl_set_handle(handle, ctrl, handle);
> >         }
> >
> >         return 0;
> > @@ -2772,6 +2807,25 @@ 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) {
> > +                       if (entity->controls[i].handle != handle)
> > +                               continue;
> > +                       uvc_ctrl_set_handle(handle, &entity->controls[i], NULL);
> > +               }

Missing outer curly braces. I'll update that, no need for a new version.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > +
> > +       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..92ecdd188587 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -337,7 +337,11 @@ 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,
> > +                                                * ctrl.handle and
> > +                                                * uvc_fh.pending_async_ctrls
> > +                                                */
> >
> >         struct v4l2_prio_state prio;            /* V4L2 priority state */
> >         u32 caps;                               /* V4L2 chain-wide caps */
> > @@ -612,6 +616,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 +802,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

Re: [PATCH v6 5/5] media: uvcvideo: Flush the control cache when we get an event

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Tue, Dec 03, 2024 at 09:20:12PM +0000, Ricardo Ribalda wrote:
> Asynchronous controls trigger an event when they have completed their
> operation.
> 
> This can make that the control cached value does not match the value in
> the device.
> 
> Let's flush the cache to be on the safe side.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 3dc9b7a49f64..db29e0e8bfd4 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1622,6 +1622,9 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain,
>  
>  	mutex_lock(&chain->ctrl_mutex);
>  
> +	/* Flush the control cache, the data might have changed. */
> +	ctrl->loaded = 0;
> +

That's better, covered by ctrl_mutex. There are however quite a few code
paths through which ctrl->loaded is accessed without holding the mutex
:-( Not a candidate for this series, but this should eventually be
fixed.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

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

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 0/5] media: uvcvideo: Two +1 fixes for async controls

[Laurent Pinchart]

On Mon, Dec 09, 2024 at 12:01:16PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 3-Dec-24 10:20 PM, Ricardo Ribalda wrote:
> > This patchset fixes two +1 bugs with the async controls for the uvc driver.
> > 
> > They were found while implementing the granular PM, but I am sending
> > them as a separate patches, so they can be reviewed sooner. They fix
> > real issues in the driver that need to be taken care.
> > 
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> 
> Ricardo, Thank you for your patches.
> 
> I have merged patches 1-4 into:
> 
> https://gitlab.freedesktop.org/linux-media/users/uvc/-/commits/next/

At least patch 5/5 was applied incorrectly. Does that result from a
merge conflict ? Or did you apply v5 by mistake ? There doesn't seem to
be any other issue.

I've rebased the uvc/next branch to fix this. Once CI passes, I'll send
a pull request.

> > ---
> > Changes in v6:
> > - Swap order of patches
> > - Use uvc_ctrl_set_handle again
> > - Move loaded=0 to uvc_ctrl_status_event()
> > - Link to v5: https://lore.kernel.org/r/20241202-uvc-fix-async-v5-0-6658c1fe312b@chromium.org
> > 
> > Changes in v5:
> > - Move set handle to the entity_commit
> > - Replace uvc_ctrl_set_handle with get/put_handle.
> > - Add a patch to flush the cache of async controls.
> > - Link to v4: https://lore.kernel.org/r/20241129-uvc-fix-async-v4-0-f23784dba80f@chromium.org
> > 
> > Changes in v4:
> > - Fix implementation of uvc_ctrl_set_handle.
> > - Link to v3: https://lore.kernel.org/r/20241129-uvc-fix-async-v3-0-ab675ce66db7@chromium.org
> > 
> > Changes in v3:
> > - change again! order of patches.
> > - Introduce uvc_ctrl_set_handle.
> > - Do not change ctrl->handle if it is not NULL.
> > 
> > Changes in v2:
> > - Annotate lockdep
> > - ctrl->handle != handle
> > - Change order of patches
> > - Move documentation of mutex
> > - Link to v1: https://lore.kernel.org/r/20241127-uvc-fix-async-v1-0-eb8722531b8c@chromium.org
> > 
> > ---
> > Ricardo Ribalda (5):
> >       media: uvcvideo: Only save async fh if success
> >       media: uvcvideo: Remove redundant NULL assignment
> >       media: uvcvideo: Remove dangling pointers
> >       media: uvcvideo: Annotate lock requirements for uvc_ctrl_set
> >       media: uvcvideo: Flush the control cache when we get an event
> > 
> >  drivers/media/usb/uvc/uvc_ctrl.c | 83 ++++++++++++++++++++++++++++++++++------
> >  drivers/media/usb/uvc/uvc_v4l2.c |  2 +
> >  drivers/media/usb/uvc/uvcvideo.h |  9 ++++-
> >  3 files changed, 82 insertions(+), 12 deletions(-)
> > ---
> > base-commit: 291a8d98186f0a704cb954855d2ae3233971f07d
> > change-id: 20241127-uvc-fix-async-2c9d40413ad8

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 0/5] media: uvcvideo: Two +1 fixes for async controls

[Hans de Goede]

Hi,

On 19-Dec-24 1:37 AM, Laurent Pinchart wrote:
> On Mon, Dec 09, 2024 at 12:01:16PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 3-Dec-24 10:20 PM, Ricardo Ribalda wrote:
>>> This patchset fixes two +1 bugs with the async controls for the uvc driver.
>>>
>>> They were found while implementing the granular PM, but I am sending
>>> them as a separate patches, so they can be reviewed sooner. They fix
>>> real issues in the driver that need to be taken care.
>>>
>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>
>> Ricardo, Thank you for your patches.
>>
>> I have merged patches 1-4 into:
>>
>> https://gitlab.freedesktop.org/linux-media/users/uvc/-/commits/next/
> 
> At least patch 5/5 was applied incorrectly. Does that result from a
> merge conflict ? Or did you apply v5 by mistake ? There doesn't seem to
> be any other issue.

I think I applied v5 by mistake, sorry about that.

> I've rebased the uvc/next branch to fix this. Once CI passes, I'll send
> a pull request.

Great, thank you.

Regards,

Hans



>>> ---
>>> Changes in v6:
>>> - Swap order of patches
>>> - Use uvc_ctrl_set_handle again
>>> - Move loaded=0 to uvc_ctrl_status_event()
>>> - Link to v5: https://lore.kernel.org/r/20241202-uvc-fix-async-v5-0-6658c1fe312b@chromium.org
>>>
>>> Changes in v5:
>>> - Move set handle to the entity_commit
>>> - Replace uvc_ctrl_set_handle with get/put_handle.
>>> - Add a patch to flush the cache of async controls.
>>> - Link to v4: https://lore.kernel.org/r/20241129-uvc-fix-async-v4-0-f23784dba80f@chromium.org
>>>
>>> Changes in v4:
>>> - Fix implementation of uvc_ctrl_set_handle.
>>> - Link to v3: https://lore.kernel.org/r/20241129-uvc-fix-async-v3-0-ab675ce66db7@chromium.org
>>>
>>> Changes in v3:
>>> - change again! order of patches.
>>> - Introduce uvc_ctrl_set_handle.
>>> - Do not change ctrl->handle if it is not NULL.
>>>
>>> Changes in v2:
>>> - Annotate lockdep
>>> - ctrl->handle != handle
>>> - Change order of patches
>>> - Move documentation of mutex
>>> - Link to v1: https://lore.kernel.org/r/20241127-uvc-fix-async-v1-0-eb8722531b8c@chromium.org
>>>
>>> ---
>>> Ricardo Ribalda (5):
>>>       media: uvcvideo: Only save async fh if success
>>>       media: uvcvideo: Remove redundant NULL assignment
>>>       media: uvcvideo: Remove dangling pointers
>>>       media: uvcvideo: Annotate lock requirements for uvc_ctrl_set
>>>       media: uvcvideo: Flush the control cache when we get an event
>>>
>>>  drivers/media/usb/uvc/uvc_ctrl.c | 83 ++++++++++++++++++++++++++++++++++------
>>>  drivers/media/usb/uvc/uvc_v4l2.c |  2 +
>>>  drivers/media/usb/uvc/uvcvideo.h |  9 ++++-
>>>  3 files changed, 82 insertions(+), 12 deletions(-)
>>> ---
>>> base-commit: 291a8d98186f0a704cb954855d2ae3233971f07d
>>> change-id: 20241127-uvc-fix-async-2c9d40413ad8
>