[PATCH 0/2] media: uvcvideo: Two fixes for async controls

[PATCH 0/2] media: uvcvideo: Two fixes for async controls

[Ricardo Ribalda]

This patchset fixes two 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 Ribalda (2):
      media: uvcvideo: Do not set an async control owned by other fh
      media: uvcvideo: Remove dangling pointers

 drivers/media/usb/uvc/uvc_ctrl.c | 44 ++++++++++++++++++++++++++++++++++++++--
 drivers/media/usb/uvc/uvc_v4l2.c |  2 ++
 drivers/media/usb/uvc/uvcvideo.h |  3 +++
 3 files changed, 47 insertions(+), 2 deletions(-)
---
base-commit: 72ad4ff638047bbbdf3232178fea4bec1f429319
change-id: 20241127-uvc-fix-async-2c9d40413ad8

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

[PATCH 1/2] media: uvcvideo: Do not set an async control owned by other fh

[Ricardo Ribalda]

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 to that operation.

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 4fe26e82e3d1..5d3a28edf7f0 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1950,6 +1950,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:

-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH 1/2] media: uvcvideo: Do not set an async control owned by other fh

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Wed, Nov 27, 2024 at 07:46:10AM +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 to that operation.

Please explain why that is an issue (both for the commit message and for
me, as I'm not sure what you're fixing here).

What may be an issue is that ctrl->handle seem to be accessed from
different contexts without proper locking :-S

> 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 4fe26e82e3d1..5d3a28edf7f0 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1950,6 +1950,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

Re: [PATCH 1/2] media: uvcvideo: Do not set an async control owned by other fh

[Ricardo Ribalda]

On Wed, 27 Nov 2024 at 10:12, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Wed, Nov 27, 2024 at 07:46:10AM +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 to that operation.
>
> Please explain why that is an issue (both for the commit message and for
> me, as I'm not sure what you're fixing here).

What about something like this:

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.

>
> What may be an issue is that ctrl->handle seem to be accessed from
> different contexts without proper locking :-S

Isn't it always protected by ctrl_mutex?

>
> > 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 4fe26e82e3d1..5d3a28edf7f0 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -1950,6 +1950,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



-- 
Ricardo Ribalda

Re: [PATCH 1/2] media: uvcvideo: Do not set an async control owned by other fh

[Laurent Pinchart]

On Wed, Nov 27, 2024 at 10:25:48AM +0100, Ricardo Ribalda wrote:
> On Wed, 27 Nov 2024 at 10:12, Laurent Pinchart wrote:
> > On Wed, Nov 27, 2024 at 07:46:10AM +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 to that operation.
> >
> > Please explain why that is an issue (both for the commit message and for
> > me, as I'm not sure what you're fixing here).
> 
> What about something like this:
> 
> 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.

So it's related to the userspace-visible behaviour, there are no issues
with this inside the kernel ?

Applications should in any case implement timeouts, as UVC devices are
fairly unreliable. What bothers me with this patch is that if the device
doesn't generate the event, ctrl->handle will never be reset to NULL,
and the control will never be settable again. I think the current
behaviour is a lesser evil.

> > What may be an issue is that ctrl->handle seem to be accessed from
> > different contexts without proper locking :-S
> 
> Isn't it always protected by ctrl_mutex?

Not that I can tell. At least uvc_ctrl_status_event_async() isn't called
with that lock held. uvc_ctrl_set() seems OK (a lockedep assert at the
beginning of the function wouldn't hurt).

As uvc_ctrl_status_event_async() is the URB completion handler, a
spinlock would be nicer than a mutex to protect ctrl->handle.

> > > 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 4fe26e82e3d1..5d3a28edf7f0 100644
> > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > @@ -1950,6 +1950,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

Re: [PATCH 1/2] media: uvcvideo: Do not set an async control owned by other fh

[Ricardo Ribalda]

On Wed, 27 Nov 2024 at 10:42, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Wed, Nov 27, 2024 at 10:25:48AM +0100, Ricardo Ribalda wrote:
> > On Wed, 27 Nov 2024 at 10:12, Laurent Pinchart wrote:
> > > On Wed, Nov 27, 2024 at 07:46:10AM +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 to that operation.
> > >
> > > Please explain why that is an issue (both for the commit message and for
> > > me, as I'm not sure what you're fixing here).
> >
> > What about something like this:
> >
> > 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.
>
> So it's related to the userspace-visible behaviour, there are no issues
> with this inside the kernel ?

It is also required by the granular PM patchset.


>
> Applications should in any case implement timeouts, as UVC devices are
> fairly unreliable. What bothers me with this patch is that if the device
> doesn't generate the event, ctrl->handle will never be reset to NULL,
> and the control will never be settable again. I think the current
> behaviour is a lesser evil.

The same app can set the control as many times as it wants, and if
that app closes the next patch will clean out the handle.
Maybe I should invert the patches?


>
> > > What may be an issue is that ctrl->handle seem to be accessed from
> > > different contexts without proper locking :-S
> >
> > Isn't it always protected by ctrl_mutex?
>
> Not that I can tell. At least uvc_ctrl_status_event_async() isn't called
> with that lock held. uvc_ctrl_set() seems OK (a lockedep assert at the
> beginning of the function wouldn't hurt).

ctrl->handle = NULL in uvc_ctrl_status_event_async() is completely redundant.

The only place we set the value of the handle is in uvc_ctrl_set, and
that can only happen for controls with mappings.
I am sending another patch to remove that set to make it clear.

>
> As uvc_ctrl_status_event_async() is the URB completion handler, a
> spinlock would be nicer than a mutex to protect ctrl->handle.
>
> > > > 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 4fe26e82e3d1..5d3a28edf7f0 100644
> > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > @@ -1950,6 +1950,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



-- 
Ricardo Ribalda

[PATCH 2/2] 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")
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 40 ++++++++++++++++++++++++++++++++++++++--
 drivers/media/usb/uvc/uvc_v4l2.c |  2 ++
 drivers/media/usb/uvc/uvcvideo.h |  3 +++
 3 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 5d3a28edf7f0..51a53ad25e9c 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,
 	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--;
+	}
 
 	list_for_each_entry(mapping, &ctrl->info.mappings, list) {
 		s32 value = __uvc_ctrl_get_value(mapping, data);
@@ -2050,8 +2055,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;
+	}
 
 	ctrl->dirty = 1;
 	ctrl->modified = 1;
@@ -2774,6 +2782,34 @@ 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) {
+		int i;
+
+		for (i = 0; i < entity->ncontrols; ++i) {
+			struct uvc_control *ctrl = &entity->controls[i];
+
+			if (!ctrl->handle || ctrl->handle != handle)
+				continue;
+
+			ctrl->handle = NULL;
+			if (WARN_ON(!handle->pending_async_ctrls))
+				continue;
+			handle->pending_async_ctrls--;
+		}
+	}
+
+	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..2f8a9c48e32a 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -612,6 +612,7 @@ struct uvc_fh {
 	struct uvc_video_chain *chain;
 	struct uvc_streaming *stream;
 	enum uvc_handle_state state;
+	unsigned int pending_async_ctrls; /* Protected by ctrl_mutex. */
 };
 
 struct uvc_driver {
@@ -797,6 +798,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

Re: [PATCH 2/2] media: uvcvideo: Remove dangling pointers

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Wed, Nov 27, 2024 at 07:46:11AM +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
> 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 | 40 ++++++++++++++++++++++++++++++++++++++--
>  drivers/media/usb/uvc/uvc_v4l2.c |  2 ++
>  drivers/media/usb/uvc/uvcvideo.h |  3 +++
>  3 files changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 5d3a28edf7f0..51a53ad25e9c 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,
>  	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--;
> +	}

There's at least one other location where ctrl->handle is reset to NULL.

>  
>  	list_for_each_entry(mapping, &ctrl->info.mappings, list) {
>  		s32 value = __uvc_ctrl_get_value(mapping, data);
> @@ -2050,8 +2055,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;

Is this protected by ctrl_mutex ?

Please be careful about locking and race conditions, taking the time to
double check will help getting your patches merged faster.

> +	}
>  
>  	ctrl->dirty = 1;
>  	ctrl->modified = 1;
> @@ -2774,6 +2782,34 @@ 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) {
> +		int i;

unsigned int

I wonder if these days you could event write

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

> +
> +		for (i = 0; i < entity->ncontrols; ++i) {
> +			struct uvc_control *ctrl = &entity->controls[i];
> +
> +			if (!ctrl->handle || ctrl->handle != handle)

Given that handle can't be null, you can write

			if (ctrl->handle != handle)

> +				continue;
> +
> +			ctrl->handle = NULL;
> +			if (WARN_ON(!handle->pending_async_ctrls))
> +				continue;

Is this needed ? If we find more controls for this handle than
pending_async_ctrls, decrementing it below 0 will case the WARN_ON() at
the end of this function to trigger, isn't that enough ?

> +			handle->pending_async_ctrls--;
> +		}
> +	}
> +
> +	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..2f8a9c48e32a 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -612,6 +612,7 @@ struct uvc_fh {
>  	struct uvc_video_chain *chain;
>  	struct uvc_streaming *stream;
>  	enum uvc_handle_state state;
> +	unsigned int pending_async_ctrls; /* Protected by ctrl_mutex. */

The kernel does it the other way around, it lists in the documentation
of the lock what data it protects.

>  };
>  
>  struct uvc_driver {
> @@ -797,6 +798,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 2/2] media: uvcvideo: Remove dangling pointers

[Ricardo Ribalda]

On Wed, 27 Nov 2024 at 10:35, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Wed, Nov 27, 2024 at 07:46:11AM +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
> > 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 | 40 ++++++++++++++++++++++++++++++++++++++--
> >  drivers/media/usb/uvc/uvc_v4l2.c |  2 ++
> >  drivers/media/usb/uvc/uvcvideo.h |  3 +++
> >  3 files changed, 43 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index 5d3a28edf7f0..51a53ad25e9c 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,
> >       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--;
> > +     }
>
> There's at least one other location where ctrl->handle is reset to NULL.

That assignment is not needed. I added a patch to remove it in the next version.

>
> >
> >       list_for_each_entry(mapping, &ctrl->info.mappings, list) {
> >               s32 value = __uvc_ctrl_get_value(mapping, data);
> > @@ -2050,8 +2055,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;
>
> Is this protected by ctrl_mutex ?

yes, uvc_ctrl_set is only called by uvc_ioctl_s_try_ext_ctrls that
calls uvc_ctrl_begin

I will send another patch to add an annotation to the function to make
it explicit.

>
> Please be careful about locking and race conditions, taking the time to
> double check will help getting your patches merged faster.
>
> > +     }
> >
> >       ctrl->dirty = 1;
> >       ctrl->modified = 1;
> > @@ -2774,6 +2782,34 @@ 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) {
> > +             int i;
>
> unsigned int
>
> I wonder if these days you could event write
>
>                 for (unsigned int i = 0; i < entity->ncontrols; ++i) {
>
> > +
> > +             for (i = 0; i < entity->ncontrols; ++i) {
> > +                     struct uvc_control *ctrl = &entity->controls[i];
> > +
> > +                     if (!ctrl->handle || ctrl->handle != handle)
>
> Given that handle can't be null, you can write
>
>                         if (ctrl->handle != handle)
>
> > +                             continue;
> > +
> > +                     ctrl->handle = NULL;
> > +                     if (WARN_ON(!handle->pending_async_ctrls))
> > +                             continue;
>
> Is this needed ? If we find more controls for this handle than
> pending_async_ctrls, decrementing it below 0 will case the WARN_ON() at
> the end of this function to trigger, isn't that enough ?

I want to know if the warning is triggered because I have too many
pending_async_ctrls or too little.


>
> > +                     handle->pending_async_ctrls--;
> > +             }
> > +     }
> > +
> > +     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..2f8a9c48e32a 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -612,6 +612,7 @@ struct uvc_fh {
> >       struct uvc_video_chain *chain;
> >       struct uvc_streaming *stream;
> >       enum uvc_handle_state state;
> > +     unsigned int pending_async_ctrls; /* Protected by ctrl_mutex. */
>
> The kernel does it the other way around, it lists in the documentation
> of the lock what data it protects.
>
> >  };
> >
> >  struct uvc_driver {
> > @@ -797,6 +798,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



--
Ricardo Ribalda

Re: [PATCH 2/2] media: uvcvideo: Remove dangling pointers

[Laurent Pinchart]

On Wed, Nov 27, 2024 at 11:23:48AM +0100, Ricardo Ribalda wrote:
> On Wed, 27 Nov 2024 at 10:35, Laurent Pinchart wrote:
> > On Wed, Nov 27, 2024 at 07:46:11AM +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
> > > 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 | 40 ++++++++++++++++++++++++++++++++++++++--
> > >  drivers/media/usb/uvc/uvc_v4l2.c |  2 ++
> > >  drivers/media/usb/uvc/uvcvideo.h |  3 +++
> > >  3 files changed, 43 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > > index 5d3a28edf7f0..51a53ad25e9c 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,
> > >       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--;
> > > +     }
> >
> > There's at least one other location where ctrl->handle is reset to NULL.
> 
> That assignment is not needed. I added a patch to remove it in the next version.
> 
> > >
> > >       list_for_each_entry(mapping, &ctrl->info.mappings, list) {
> > >               s32 value = __uvc_ctrl_get_value(mapping, data);
> > > @@ -2050,8 +2055,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;
> >
> > Is this protected by ctrl_mutex ?
> 
> yes, uvc_ctrl_set is only called by uvc_ioctl_s_try_ext_ctrls that
> calls uvc_ctrl_begin

You're right. I think I figured out after writing this part of the
review, and forgot to delete it. Sorry.

> I will send another patch to add an annotation to the function to make
> it explicit.
> 
> > Please be careful about locking and race conditions, taking the time to
> > double check will help getting your patches merged faster.
> >
> > > +     }
> > >
> > >       ctrl->dirty = 1;
> > >       ctrl->modified = 1;
> > > @@ -2774,6 +2782,34 @@ 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) {
> > > +             int i;
> >
> > unsigned int
> >
> > I wonder if these days you could event write
> >
> >                 for (unsigned int i = 0; i < entity->ncontrols; ++i) {
> >
> > > +
> > > +             for (i = 0; i < entity->ncontrols; ++i) {
> > > +                     struct uvc_control *ctrl = &entity->controls[i];
> > > +
> > > +                     if (!ctrl->handle || ctrl->handle != handle)
> >
> > Given that handle can't be null, you can write
> >
> >                         if (ctrl->handle != handle)
> >
> > > +                             continue;
> > > +
> > > +                     ctrl->handle = NULL;
> > > +                     if (WARN_ON(!handle->pending_async_ctrls))
> > > +                             continue;
> >
> > Is this needed ? If we find more controls for this handle than
> > pending_async_ctrls, decrementing it below 0 will case the WARN_ON() at
> > the end of this function to trigger, isn't that enough ?
> 
> I want to know if the warning is triggered because I have too many
> pending_async_ctrls or too little.

You could also print the value of pending_async_ctrls at the end, it
would give you that information, and tell you how many you're missing.
Not a big deal, and I don't expect that warning to be triggered.

> > > +                     handle->pending_async_ctrls--;
> > > +             }
> > > +     }
> > > +
> > > +     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..2f8a9c48e32a 100644
> > > --- a/drivers/media/usb/uvc/uvcvideo.h
> > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > @@ -612,6 +612,7 @@ struct uvc_fh {
> > >       struct uvc_video_chain *chain;
> > >       struct uvc_streaming *stream;
> > >       enum uvc_handle_state state;
> > > +     unsigned int pending_async_ctrls; /* Protected by ctrl_mutex. */
> >
> > The kernel does it the other way around, it lists in the documentation
> > of the lock what data it protects.
> >
> > >  };
> > >
> > >  struct uvc_driver {
> > > @@ -797,6 +798,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 2/2] media: uvcvideo: Remove dangling pointers

[Ricardo Ribalda]

On Thu, 28 Nov 2024 at 21:51, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Wed, Nov 27, 2024 at 11:23:48AM +0100, Ricardo Ribalda wrote:
> > On Wed, 27 Nov 2024 at 10:35, Laurent Pinchart wrote:
> > > On Wed, Nov 27, 2024 at 07:46:11AM +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
> > > > 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 | 40 ++++++++++++++++++++++++++++++++++++++--
> > > >  drivers/media/usb/uvc/uvc_v4l2.c |  2 ++
> > > >  drivers/media/usb/uvc/uvcvideo.h |  3 +++
> > > >  3 files changed, 43 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > index 5d3a28edf7f0..51a53ad25e9c 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,
> > > >       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--;
> > > > +     }
> > >
> > > There's at least one other location where ctrl->handle is reset to NULL.
> >
> > That assignment is not needed. I added a patch to remove it in the next version.
> >
> > > >
> > > >       list_for_each_entry(mapping, &ctrl->info.mappings, list) {
> > > >               s32 value = __uvc_ctrl_get_value(mapping, data);
> > > > @@ -2050,8 +2055,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;
> > >
> > > Is this protected by ctrl_mutex ?
> >
> > yes, uvc_ctrl_set is only called by uvc_ioctl_s_try_ext_ctrls that
> > calls uvc_ctrl_begin
>
> You're right. I think I figured out after writing this part of the
> review, and forgot to delete it. Sorry.
>
> > I will send another patch to add an annotation to the function to make
> > it explicit.
> >
> > > Please be careful about locking and race conditions, taking the time to
> > > double check will help getting your patches merged faster.
> > >
> > > > +     }
> > > >
> > > >       ctrl->dirty = 1;
> > > >       ctrl->modified = 1;
> > > > @@ -2774,6 +2782,34 @@ 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) {
> > > > +             int i;
> > >
> > > unsigned int
> > >
> > > I wonder if these days you could event write
> > >
> > >                 for (unsigned int i = 0; i < entity->ncontrols; ++i) {
> > >
> > > > +
> > > > +             for (i = 0; i < entity->ncontrols; ++i) {
> > > > +                     struct uvc_control *ctrl = &entity->controls[i];
> > > > +
> > > > +                     if (!ctrl->handle || ctrl->handle != handle)
> > >
> > > Given that handle can't be null, you can write
> > >
> > >                         if (ctrl->handle != handle)
> > >
> > > > +                             continue;
> > > > +
> > > > +                     ctrl->handle = NULL;
> > > > +                     if (WARN_ON(!handle->pending_async_ctrls))
> > > > +                             continue;
> > >
> > > Is this needed ? If we find more controls for this handle than
> > > pending_async_ctrls, decrementing it below 0 will case the WARN_ON() at
> > > the end of this function to trigger, isn't that enough ?
> >
> > I want to know if the warning is triggered because I have too many
> > pending_async_ctrls or too little.
>
> You could also print the value of pending_async_ctrls at the end, it
> would give you that information, and tell you how many you're missing.
> Not a big deal, and I don't expect that warning to be triggered.

The issue is that the variable is unsigned. it will show a weird
number... And making it signed seems incorrect.

I also do not expect it to be triggered. For now I will keep it as is.
If you have a strong opinion against it I will change it.


>
> > > > +                     handle->pending_async_ctrls--;
> > > > +             }
> > > > +     }
> > > > +
> > > > +     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..2f8a9c48e32a 100644
> > > > --- a/drivers/media/usb/uvc/uvcvideo.h
> > > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > > @@ -612,6 +612,7 @@ struct uvc_fh {
> > > >       struct uvc_video_chain *chain;
> > > >       struct uvc_streaming *stream;
> > > >       enum uvc_handle_state state;
> > > > +     unsigned int pending_async_ctrls; /* Protected by ctrl_mutex. */
> > >
> > > The kernel does it the other way around, it lists in the documentation
> > > of the lock what data it protects.
> > >
> > > >  };
> > > >
> > > >  struct uvc_driver {
> > > > @@ -797,6 +798,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



-- 
Ricardo Ribalda