[PATCH 0/3] media: uvcvideo: Make sure the ctrl cache is in sync with the device

[PATCH 0/3] media: uvcvideo: Make sure the ctrl cache is in sync with the device

[Ricardo Ribalda]

Hans discovered a bug in the error handling of uvc_ctrl_commit(), lets
fix it.

It has been tested with v4l2-compliance -d /dev/video0

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Ricardo Ribalda (3):
      media: uvcvideo: Return the number of processed controls
      media: uvcvideo: Do not send events for not changed controls
      media: uvcvideo: Rollback non processed entities on error

 drivers/media/usb/uvc/uvc_ctrl.c | 54 +++++++++++++++++++++++++++++++---------
 1 file changed, 42 insertions(+), 12 deletions(-)
---
base-commit: 6c10d1adae82e1c8da16e7ebd2320e69f20b9d6f
change-id: 20241210-uvc-data-backup-a0a76df9aee9

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

[PATCH 1/3] media: uvcvideo: Return the number of processed controls

[Ricardo Ribalda]

If we let know our callers that we have not done anything, they will be
able to optimize their decisions.

Cc: stable@kernel.org
Fixes: b4012002f3a3 ("[media] uvcvideo: Add support for control events")
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 4fe26e82e3d1..7e2fc97c9f43 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1810,9 +1810,14 @@ int uvc_ctrl_begin(struct uvc_video_chain *chain)
 	return mutex_lock_interruptible(&chain->ctrl_mutex) ? -ERESTARTSYS : 0;
 }
 
+/*
+ * Returns the number of uvc controls that have been correctly set, or a
+ * negative number if there has been an error.
+ */
 static int uvc_ctrl_commit_entity(struct uvc_device *dev,
 	struct uvc_entity *entity, int rollback, struct uvc_control **err_ctrl)
 {
+	unsigned int processed_ctrls = 0;
 	struct uvc_control *ctrl;
 	unsigned int i;
 	int ret;
@@ -1847,6 +1852,9 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
 		else
 			ret = 0;
 
+		if (!ret)
+			processed_ctrls++;
+
 		if (rollback || ret < 0)
 			memcpy(uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
 			       uvc_ctrl_data(ctrl, UVC_CTRL_DATA_BACKUP),
@@ -1861,7 +1869,7 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
 		}
 	}
 
-	return 0;
+	return processed_ctrls;
 }
 
 static int uvc_ctrl_find_ctrl_idx(struct uvc_entity *entity,
@@ -1910,7 +1918,7 @@ int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
 		uvc_ctrl_send_events(handle, ctrls->controls, ctrls->count);
 done:
 	mutex_unlock(&chain->ctrl_mutex);
-	return ret;
+	return ret < 0 ? ret : 0;
 }
 
 int uvc_ctrl_get(struct uvc_video_chain *chain,

-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH 1/3] media: uvcvideo: Return the number of processed controls

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Tue, Dec 10, 2024 at 10:22:22PM +0000, Ricardo Ribalda wrote:
> If we let know our callers that we have not done anything, they will be
> able to optimize their decisions.
> 
> Cc: stable@kernel.org
> Fixes: b4012002f3a3 ("[media] uvcvideo: Add support for control events")

This patch doesn't fix any issue by itself.

> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 4fe26e82e3d1..7e2fc97c9f43 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1810,9 +1810,14 @@ int uvc_ctrl_begin(struct uvc_video_chain *chain)
>  	return mutex_lock_interruptible(&chain->ctrl_mutex) ? -ERESTARTSYS : 0;
>  }
>  
> +/*
> + * Returns the number of uvc controls that have been correctly set, or a
> + * negative number if there has been an error.
> + */
>  static int uvc_ctrl_commit_entity(struct uvc_device *dev,
>  	struct uvc_entity *entity, int rollback, struct uvc_control **err_ctrl)
>  {
> +	unsigned int processed_ctrls = 0;
>  	struct uvc_control *ctrl;
>  	unsigned int i;
>  	int ret;
> @@ -1847,6 +1852,9 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
>  		else
>  			ret = 0;
>  
> +		if (!ret)
> +			processed_ctrls++;
> +
>  		if (rollback || ret < 0)
>  			memcpy(uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
>  			       uvc_ctrl_data(ctrl, UVC_CTRL_DATA_BACKUP),
> @@ -1861,7 +1869,7 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
>  		}
>  	}
>  
> -	return 0;
> +	return processed_ctrls;
>  }
>  
>  static int uvc_ctrl_find_ctrl_idx(struct uvc_entity *entity,
> @@ -1910,7 +1918,7 @@ int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
>  		uvc_ctrl_send_events(handle, ctrls->controls, ctrls->count);

	ret = 0;

>  done:
>  	mutex_unlock(&chain->ctrl_mutex);
> -	return ret;
> +	return ret < 0 ? ret : 0;

And keep 'return ret;'.

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

>  }
>  
>  int uvc_ctrl_get(struct uvc_video_chain *chain,

-- 
Regards,

Laurent Pinchart

[PATCH 2/3] media: uvcvideo: Do not send events for not changed controls

[Ricardo Ribalda]

Only send events for controls that have actually changed.
E.g.: We are changing entities A, B and C. If we get an error while
we change B we do not continue setting C. But the current code sends an
event also for C.

Due to the fact that the controls are grouped by entities, and the user
might group them in different orders, we cannot send the events at the
end, but when we change an entity.

Cc: stable@kernel.org
Fixes: b4012002f3a3 ("[media] uvcvideo: Add support for control events")
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 7e2fc97c9f43..9496ac970267 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1669,7 +1669,9 @@ static bool uvc_ctrl_xctrls_has_control(const struct v4l2_ext_control *xctrls,
 }
 
 static void uvc_ctrl_send_events(struct uvc_fh *handle,
-	const struct v4l2_ext_control *xctrls, unsigned int xctrls_count)
+				 struct uvc_entity *entity,
+				 const struct v4l2_ext_control *xctrls,
+				 unsigned int xctrls_count)
 {
 	struct uvc_control_mapping *mapping;
 	struct uvc_control *ctrl;
@@ -1680,6 +1682,9 @@ static void uvc_ctrl_send_events(struct uvc_fh *handle,
 	for (i = 0; i < xctrls_count; ++i) {
 		ctrl = uvc_find_control(handle->chain, xctrls[i].id, &mapping);
 
+		if (ctrl->entity != entity)
+			continue;
+
 		if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
 			/* Notification will be sent from an Interrupt event. */
 			continue;
@@ -1911,11 +1916,12 @@ int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
 					uvc_ctrl_find_ctrl_idx(entity, ctrls,
 							       err_ctrl);
 			goto done;
+		} else if (ret > 0 && !rollback) {
+			uvc_ctrl_send_events(handle, entity,
+					     ctrls->controls, ctrls->count);
 		}
 	}
 
-	if (!rollback)
-		uvc_ctrl_send_events(handle, ctrls->controls, ctrls->count);
 done:
 	mutex_unlock(&chain->ctrl_mutex);
 	return ret < 0 ? ret : 0;

-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH 2/3] media: uvcvideo: Do not send events for not changed controls

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Tue, Dec 10, 2024 at 10:22:23PM +0000, Ricardo Ribalda wrote:
> Only send events for controls that have actually changed.
> E.g.: We are changing entities A, B and C. If we get an error while
> we change B we do not continue setting C. But the current code sends an
> event also for C.

Does it ? If uvc_ctrl_commit_entity() fails the 'goto done' statement
skips over uvc_ctrl_send_events().

> Due to the fact that the controls are grouped by entities, and the user
> might group them in different orders, we cannot send the events at the
> end, but when we change an entity.
> 
> Cc: stable@kernel.org
> Fixes: b4012002f3a3 ("[media] uvcvideo: Add support for control events")
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 7e2fc97c9f43..9496ac970267 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1669,7 +1669,9 @@ static bool uvc_ctrl_xctrls_has_control(const struct v4l2_ext_control *xctrls,
>  }
>  
>  static void uvc_ctrl_send_events(struct uvc_fh *handle,
> -	const struct v4l2_ext_control *xctrls, unsigned int xctrls_count)
> +				 struct uvc_entity *entity,
> +				 const struct v4l2_ext_control *xctrls,
> +				 unsigned int xctrls_count)
>  {
>  	struct uvc_control_mapping *mapping;
>  	struct uvc_control *ctrl;
> @@ -1680,6 +1682,9 @@ static void uvc_ctrl_send_events(struct uvc_fh *handle,
>  	for (i = 0; i < xctrls_count; ++i) {
>  		ctrl = uvc_find_control(handle->chain, xctrls[i].id, &mapping);
>  
> +		if (ctrl->entity != entity)
> +			continue;
> +
>  		if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
>  			/* Notification will be sent from an Interrupt event. */
>  			continue;
> @@ -1911,11 +1916,12 @@ int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
>  					uvc_ctrl_find_ctrl_idx(entity, ctrls,
>  							       err_ctrl);
>  			goto done;
> +		} else if (ret > 0 && !rollback) {
> +			uvc_ctrl_send_events(handle, entity,
> +					     ctrls->controls, ctrls->count);
>  		}
>  	}
>  
> -	if (!rollback)
> -		uvc_ctrl_send_events(handle, ctrls->controls, ctrls->count);
>  done:
>  	mutex_unlock(&chain->ctrl_mutex);
>  	return ret < 0 ? ret : 0;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] media: uvcvideo: Do not send events for not changed controls

[Ricardo Ribalda]

On Sun, 23 Feb 2025 at 18:10, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Tue, Dec 10, 2024 at 10:22:23PM +0000, Ricardo Ribalda wrote:
> > Only send events for controls that have actually changed.
> > E.g.: We are changing entities A, B and C. If we get an error while
> > we change B we do not continue setting C. But the current code sends an
> > event also for C.
>
> Does it ? If uvc_ctrl_commit_entity() fails the 'goto done' statement
> skips over uvc_ctrl_send_events().

I have no idea how that commit message ended up there :).
I mean, the code is correct (I believe), but it does not do what the
commit message says. I guess I was trying something different and
forgot to modify the commit message.

Please check v2. Sorry about that



>
> > Due to the fact that the controls are grouped by entities, and the user
> > might group them in different orders, we cannot send the events at the
> > end, but when we change an entity.
> >
> > Cc: stable@kernel.org
> > Fixes: b4012002f3a3 ("[media] uvcvideo: Add support for control events")
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_ctrl.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index 7e2fc97c9f43..9496ac970267 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -1669,7 +1669,9 @@ static bool uvc_ctrl_xctrls_has_control(const struct v4l2_ext_control *xctrls,
> >  }
> >
> >  static void uvc_ctrl_send_events(struct uvc_fh *handle,
> > -     const struct v4l2_ext_control *xctrls, unsigned int xctrls_count)
> > +                              struct uvc_entity *entity,
> > +                              const struct v4l2_ext_control *xctrls,
> > +                              unsigned int xctrls_count)
> >  {
> >       struct uvc_control_mapping *mapping;
> >       struct uvc_control *ctrl;
> > @@ -1680,6 +1682,9 @@ static void uvc_ctrl_send_events(struct uvc_fh *handle,
> >       for (i = 0; i < xctrls_count; ++i) {
> >               ctrl = uvc_find_control(handle->chain, xctrls[i].id, &mapping);
> >
> > +             if (ctrl->entity != entity)
> > +                     continue;
> > +
> >               if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
> >                       /* Notification will be sent from an Interrupt event. */
> >                       continue;
> > @@ -1911,11 +1916,12 @@ int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
> >                                       uvc_ctrl_find_ctrl_idx(entity, ctrls,
> >                                                              err_ctrl);
> >                       goto done;
> > +             } else if (ret > 0 && !rollback) {
> > +                     uvc_ctrl_send_events(handle, entity,
> > +                                          ctrls->controls, ctrls->count);
> >               }
> >       }
> >
> > -     if (!rollback)
> > -             uvc_ctrl_send_events(handle, ctrls->controls, ctrls->count);
> >  done:
> >       mutex_unlock(&chain->ctrl_mutex);
> >       return ret < 0 ? ret : 0;
>
> --
> Regards,
>
> Laurent Pinchart



--
Ricardo Ribalda

[PATCH 3/3] media: uvcvideo: Rollback non processed entities on error

[Ricardo Ribalda]

If we wail to commit an entity, we need to restore the
UVC_CTRL_DATA_BACKUP for the other uncommitted entities. Otherwise the
control cache and the device would be out of sync.

Cc: stable@kernel.org
Fixes: b4012002f3a3 ("[media] uvcvideo: Add support for control events")
Reported-by: Hans de Goede <hdegoede@redhat.com>
Closes: https://lore.kernel.org/linux-media/fe845e04-9fde-46ee-9763-a6f00867929a@redhat.com/
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 9496ac970267..93c355b53101 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1825,11 +1825,14 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
 	unsigned int processed_ctrls = 0;
 	struct uvc_control *ctrl;
 	unsigned int i;
-	int ret;
+	int ret = 0;
 
 	if (entity == NULL)
 		return 0;
 
+	if (err_ctrl)
+		*err_ctrl = NULL;
+
 	for (i = 0; i < entity->ncontrols; ++i) {
 		ctrl = &entity->controls[i];
 		if (!ctrl->initialized)
@@ -1854,8 +1857,6 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
 				dev->intfnum, ctrl->info.selector,
 				uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
 				ctrl->info.size);
-		else
-			ret = 0;
 
 		if (!ret)
 			processed_ctrls++;
@@ -1868,12 +1869,19 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
 		ctrl->dirty = 0;
 
 		if (ret < 0) {
-			if (err_ctrl)
+			if (err_ctrl && !*err_ctrl)
 				*err_ctrl = ctrl;
-			return ret;
+			/*
+			 * If we fail to set a control, we need to rollback
+			 * the next ones.
+			 */
+			rollback = 1;
 		}
 	}
 
+	if (ret)
+		return ret;
+
 	return processed_ctrls;
 }
 
@@ -1904,6 +1912,7 @@ int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
 	struct uvc_video_chain *chain = handle->chain;
 	struct uvc_control *err_ctrl;
 	struct uvc_entity *entity;
+	int ret_out = 0;
 	int ret = 0;
 
 	/* Find the control. */
@@ -1915,16 +1924,23 @@ int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
 				ctrls->error_idx =
 					uvc_ctrl_find_ctrl_idx(entity, ctrls,
 							       err_ctrl);
-			goto done;
+			/*
+			 * When we fail to commit an entity, we need to
+			 * restore the UVC_CTRL_DATA_BACKUP for all the
+			 * controls in the other entities, otherwise our cache
+			 * and the hardware will be out of sync.
+			 */
+			rollback = 1;
+
+			ret_out = ret;
 		} else if (ret > 0 && !rollback) {
 			uvc_ctrl_send_events(handle, entity,
 					     ctrls->controls, ctrls->count);
 		}
 	}
 
-done:
 	mutex_unlock(&chain->ctrl_mutex);
-	return ret < 0 ? ret : 0;
+	return ret_out;
 }
 
 int uvc_ctrl_get(struct uvc_video_chain *chain,

-- 
2.47.0.338.g60cca15819-goog