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

[PATCH v2 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>
---
Changes in v2:
- Fix commit message (Thanks Laurent).
- Change logic for err_ctrl. It looks nicer now.
- Link to v1: https://lore.kernel.org/r/20241210-uvc-data-backup-v1-0-77141e439cc3@chromium.org

---
Ricardo Ribalda (3):
      media: uvcvideo: Return the number of processed controls
      media: uvcvideo: Send control events for partial succeeds
      media: uvcvideo: Rollback non processed entities on error

 drivers/media/usb/uvc/uvc_ctrl.c | 53 ++++++++++++++++++++++++++++++----------
 1 file changed, 40 insertions(+), 13 deletions(-)
---
base-commit: c2b96a6818159fba8a3bcc38262da9e77f9b3ec7
change-id: 20241210-uvc-data-backup-a0a76df9aee9

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

[PATCH v2 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")
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 4e58476d305e..f2484f6d21c1 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1846,12 +1846,17 @@ 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_fh *handle,
 				  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;
@@ -1886,6 +1891,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),
@@ -1904,7 +1912,7 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
 			uvc_ctrl_set_handle(handle, ctrl, handle);
 	}
 
-	return 0;
+	return processed_ctrls;
 }
 
 static int uvc_ctrl_find_ctrl_idx(struct uvc_entity *entity,
@@ -1951,6 +1959,7 @@ int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
 
 	if (!rollback)
 		uvc_ctrl_send_events(handle, ctrls->controls, ctrls->count);
+	ret = 0;
 done:
 	mutex_unlock(&chain->ctrl_mutex);
 	return ret;

-- 
2.48.1.601.g30ceb7b040-goog

[PATCH v2 2/3] media: uvcvideo: Send control events for partial succeeds

[Ricardo Ribalda]

Today, when we are applying a change to entities A, B. If A succeeds and B
fails the events for A are not sent.

This change changes the code so the events for A are send right after
they happen.

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 f2484f6d21c1..7d074686eef4 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1705,7 +1705,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;
@@ -1716,6 +1718,9 @@ static void uvc_ctrl_send_events(struct uvc_fh *handle,
 		u32 changes = V4L2_EVENT_CTRL_CH_VALUE;
 
 		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;
@@ -1954,11 +1959,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);
 	ret = 0;
 done:
 	mutex_unlock(&chain->ctrl_mutex);

-- 
2.48.1.601.g30ceb7b040-goog

[PATCH v2 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, 22 insertions(+), 10 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 7d074686eef4..89b946151b16 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1864,7 +1864,7 @@ 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;
@@ -1893,8 +1893,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++;
@@ -1906,10 +1904,14 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
 
 		ctrl->dirty = 0;
 
-		if (ret < 0) {
+		if (ret < 0 && !rollback) {
 			if (err_ctrl)
 				*err_ctrl = ctrl;
-			return ret;
+			/*
+			 * If we fail to set a control, we need to rollback
+			 * the next ones.
+			 */
+			rollback = 1;
 		}
 
 		if (!rollback && handle &&
@@ -1917,6 +1919,9 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
 			uvc_ctrl_set_handle(handle, ctrl, handle);
 	}
 
+	if (ret)
+		return ret;
+
 	return processed_ctrls;
 }
 
@@ -1947,7 +1952,8 @@ 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 = 0;
+	int ret_out = 0;
+	int ret;
 
 	/* Find the control. */
 	list_for_each_entry(entity, &chain->entities, chain) {
@@ -1958,17 +1964,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);
 		}
 	}
 
-	ret = 0;
-done:
 	mutex_unlock(&chain->ctrl_mutex);
-	return ret;
+	return ret_out;
 }
 
 int uvc_ctrl_get(struct uvc_video_chain *chain,

-- 
2.48.1.601.g30ceb7b040-goog

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

[Hans de Goede]

Hi,

On 24-Feb-25 11:34, 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")
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 4e58476d305e..f2484f6d21c1 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1846,12 +1846,17 @@ 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_fh *handle,
>  				  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;
> @@ -1886,6 +1891,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),
> @@ -1904,7 +1912,7 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
>  			uvc_ctrl_set_handle(handle, ctrl, handle);
>  	}
>  
> -	return 0;
> +	return processed_ctrls;
>  }
>  
>  static int uvc_ctrl_find_ctrl_idx(struct uvc_entity *entity,
> @@ -1951,6 +1959,7 @@ int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
>  
>  	if (!rollback)
>  		uvc_ctrl_send_events(handle, ctrls->controls, ctrls->count);
> +	ret = 0;
>  done:
>  	mutex_unlock(&chain->ctrl_mutex);
>  	return ret;
> 

Re: [PATCH v2 2/3] media: uvcvideo: Send control events for partial succeeds

[Hans de Goede]

Hi,

On 24-Feb-25 11:34, Ricardo Ribalda wrote:
> Today, when we are applying a change to entities A, B. If A succeeds and B
> fails the events for A are not sent.
> 
> This change changes the code so the events for A are send right after
> they happen.
> 
> Cc: stable@kernel.org
> Fixes: b4012002f3a3 ("[media] uvcvideo: Add support for control events")
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
>  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 f2484f6d21c1..7d074686eef4 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1705,7 +1705,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;
> @@ -1716,6 +1718,9 @@ static void uvc_ctrl_send_events(struct uvc_fh *handle,
>  		u32 changes = V4L2_EVENT_CTRL_CH_VALUE;
>  
>  		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;
> @@ -1954,11 +1959,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);
>  	ret = 0;
>  done:
>  	mutex_unlock(&chain->ctrl_mutex);
> 

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

[Hans de Goede]

Hi,

On 24-Feb-25 11:34, Ricardo Ribalda wrote:
> 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>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans




> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 32 ++++++++++++++++++++++----------
>  1 file changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 7d074686eef4..89b946151b16 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1864,7 +1864,7 @@ 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;
> @@ -1893,8 +1893,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++;
> @@ -1906,10 +1904,14 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
>  
>  		ctrl->dirty = 0;
>  
> -		if (ret < 0) {
> +		if (ret < 0 && !rollback) {
>  			if (err_ctrl)
>  				*err_ctrl = ctrl;
> -			return ret;
> +			/*
> +			 * If we fail to set a control, we need to rollback
> +			 * the next ones.
> +			 */
> +			rollback = 1;
>  		}
>  
>  		if (!rollback && handle &&
> @@ -1917,6 +1919,9 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
>  			uvc_ctrl_set_handle(handle, ctrl, handle);
>  	}
>  
> +	if (ret)
> +		return ret;
> +
>  	return processed_ctrls;
>  }
>  
> @@ -1947,7 +1952,8 @@ 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 = 0;
> +	int ret_out = 0;
> +	int ret;
>  
>  	/* Find the control. */
>  	list_for_each_entry(entity, &chain->entities, chain) {
> @@ -1958,17 +1964,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);
>  		}
>  	}
>  
> -	ret = 0;
> -done:
>  	mutex_unlock(&chain->ctrl_mutex);
> -	return ret;
> +	return ret_out;
>  }
>  
>  int uvc_ctrl_get(struct uvc_video_chain *chain,
> 

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

[Hans de Goede]

Hi Ricardo,

On 24-Feb-25 11:34, Ricardo Ribalda wrote:
> If we wail to commit an entity, we need to restore the

s/wail/fail/

I've fixed this up while merging this series and
I've pushed the entire series to:
https://gitlab.freedesktop.org/linux-media/users/uvc/ next now.

Note I had to manually fix some conflicts due to the granular power
saving series being merged first. I'm pretty sure I got things correct
but a double check would be appreciated.

Regards,

Hans




> 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, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 7d074686eef4..89b946151b16 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1864,7 +1864,7 @@ 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;
> @@ -1893,8 +1893,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++;
> @@ -1906,10 +1904,14 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
>  
>  		ctrl->dirty = 0;
>  
> -		if (ret < 0) {
> +		if (ret < 0 && !rollback) {
>  			if (err_ctrl)
>  				*err_ctrl = ctrl;
> -			return ret;
> +			/*
> +			 * If we fail to set a control, we need to rollback
> +			 * the next ones.
> +			 */
> +			rollback = 1;
>  		}
>  
>  		if (!rollback && handle &&
> @@ -1917,6 +1919,9 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
>  			uvc_ctrl_set_handle(handle, ctrl, handle);
>  	}
>  
> +	if (ret)
> +		return ret;
> +
>  	return processed_ctrls;
>  }
>  
> @@ -1947,7 +1952,8 @@ 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 = 0;
> +	int ret_out = 0;
> +	int ret;
>  
>  	/* Find the control. */
>  	list_for_each_entry(entity, &chain->entities, chain) {
> @@ -1958,17 +1964,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);
>  		}
>  	}
>  
> -	ret = 0;
> -done:
>  	mutex_unlock(&chain->ctrl_mutex);
> -	return ret;
> +	return ret_out;
>  }
>  
>  int uvc_ctrl_get(struct uvc_video_chain *chain,
> 

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

[Ricardo Ribalda]

On Mon, 7 Apr 2025 at 15:02, Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Ricardo,
>
> On 24-Feb-25 11:34, Ricardo Ribalda wrote:
> > If we wail to commit an entity, we need to restore the
>
> s/wail/fail/
>
> I've fixed this up while merging this series and
> I've pushed the entire series to:
> https://gitlab.freedesktop.org/linux-media/users/uvc/ next now.
>
> Note I had to manually fix some conflicts due to the granular power
> saving series being merged first. I'm pretty sure I got things correct
> but a double check would be appreciated.

I reviewed the merge and I could not find any issue.
Thanks :)

>
> Regards,
>
> Hans
>
>
>
>
> > 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, 22 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index 7d074686eef4..89b946151b16 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -1864,7 +1864,7 @@ 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;
> > @@ -1893,8 +1893,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++;
> > @@ -1906,10 +1904,14 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
> >
> >               ctrl->dirty = 0;
> >
> > -             if (ret < 0) {
> > +             if (ret < 0 && !rollback) {
> >                       if (err_ctrl)
> >                               *err_ctrl = ctrl;
> > -                     return ret;
> > +                     /*
> > +                      * If we fail to set a control, we need to rollback
> > +                      * the next ones.
> > +                      */
> > +                     rollback = 1;
> >               }
> >
> >               if (!rollback && handle &&
> > @@ -1917,6 +1919,9 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
> >                       uvc_ctrl_set_handle(handle, ctrl, handle);
> >       }
> >
> > +     if (ret)
> > +             return ret;
> > +
> >       return processed_ctrls;
> >  }
> >
> > @@ -1947,7 +1952,8 @@ 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 = 0;
> > +     int ret_out = 0;
> > +     int ret;
> >
> >       /* Find the control. */
> >       list_for_each_entry(entity, &chain->entities, chain) {
> > @@ -1958,17 +1964,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);
> >               }
> >       }
> >
> > -     ret = 0;
> > -done:
> >       mutex_unlock(&chain->ctrl_mutex);
> > -     return ret;
> > +     return ret_out;
> >  }
> >
> >  int uvc_ctrl_get(struct uvc_video_chain *chain,
> >
>


-- 
Ricardo Ribalda

Re: [PATCH v2 2/3] media: uvcvideo: Send control events for partial succeeds

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Mon, Feb 24, 2025 at 10:34:54AM +0000, Ricardo Ribalda wrote:
> Today, when we are applying a change to entities A, B. If A succeeds and B
> fails the events for A are not sent.
> 
> This change changes the code so the events for A are send right after

s/send/sent/

> they happen.
> 
> Cc: stable@kernel.org
> Fixes: b4012002f3a3 ("[media] uvcvideo: Add support for control events")
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

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

> ---
>  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 f2484f6d21c1..7d074686eef4 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1705,7 +1705,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;
> @@ -1716,6 +1718,9 @@ static void uvc_ctrl_send_events(struct uvc_fh *handle,
>  		u32 changes = V4L2_EVENT_CTRL_CH_VALUE;
>  
>  		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;
> @@ -1954,11 +1959,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);
>  	ret = 0;
>  done:
>  	mutex_unlock(&chain->ctrl_mutex);

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Mon, Feb 24, 2025 at 10:34:55AM +0000, Ricardo Ribalda wrote:
> 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, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 7d074686eef4..89b946151b16 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1864,7 +1864,7 @@ 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;
> @@ -1893,8 +1893,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++;
> @@ -1906,10 +1904,14 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
>  
>  		ctrl->dirty = 0;
>  
> -		if (ret < 0) {
> +		if (ret < 0 && !rollback) {
>  			if (err_ctrl)
>  				*err_ctrl = ctrl;
> -			return ret;
> +			/*
> +			 * If we fail to set a control, we need to rollback
> +			 * the next ones.
> +			 */
> +			rollback = 1;
>  		}
>  
>  		if (!rollback && handle &&
> @@ -1917,6 +1919,9 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
>  			uvc_ctrl_set_handle(handle, ctrl, handle);
>  	}
>  
> +	if (ret)
> +		return ret;
> +
>  	return processed_ctrls;
>  }
>  
> @@ -1947,7 +1952,8 @@ 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 = 0;
> +	int ret_out = 0;
> +	int ret;
>  
>  	/* Find the control. */
>  	list_for_each_entry(entity, &chain->entities, chain) {
> @@ -1958,17 +1964,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.

The text can be reflowed to 80 columns.

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

> +			 */
> +			rollback = 1;
> +
> +			ret_out = ret;
>  		} else if (ret > 0 && !rollback) {
>  			uvc_ctrl_send_events(handle, entity,
>  					     ctrls->controls, ctrls->count);
>  		}
>  	}
>  
> -	ret = 0;
> -done:
>  	mutex_unlock(&chain->ctrl_mutex);
> -	return ret;
> +	return ret_out;
>  }
>  
>  int uvc_ctrl_get(struct uvc_video_chain *chain,

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

On Thu, Apr 10, 2025 at 09:32:42AM +0200, Ricardo Ribalda wrote:
> On Mon, 7 Apr 2025 at 15:02, Hans de Goede wrote:
> > On 24-Feb-25 11:34, Ricardo Ribalda wrote:
> > > If we wail to commit an entity, we need to restore the
> >
> > s/wail/fail/
> >
> > I've fixed this up while merging this series and
> > I've pushed the entire series to:
> > https://gitlab.freedesktop.org/linux-media/users/uvc/ next now.
> >
> > Note I had to manually fix some conflicts due to the granular power
> > saving series being merged first. I'm pretty sure I got things correct
> > but a double check would be appreciated.
> 
> I reviewed the merge and I could not find any issue.
> Thanks :)

There's a set of small issues that may (or may not, pending discussions)
need to be addressed in each of the three series present in uvc/next.
Most review comments are small and don't require sending a new version,
while others may require respining some of the patches. Let's first
discuss the issues, and we can then check how to minimize the need to
send new versions.

I'll review other pendings series after finalizing these three, as
conflicts are complex enough to handle as-is. In the future, when you
send multiple series that depend on each other, please indicate in what
order they should be merged.

> > > 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, 22 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > > index 7d074686eef4..89b946151b16 100644
> > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > @@ -1864,7 +1864,7 @@ 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;
> > > @@ -1893,8 +1893,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++;
> > > @@ -1906,10 +1904,14 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
> > >
> > >               ctrl->dirty = 0;
> > >
> > > -             if (ret < 0) {
> > > +             if (ret < 0 && !rollback) {
> > >                       if (err_ctrl)
> > >                               *err_ctrl = ctrl;
> > > -                     return ret;
> > > +                     /*
> > > +                      * If we fail to set a control, we need to rollback
> > > +                      * the next ones.
> > > +                      */
> > > +                     rollback = 1;
> > >               }
> > >
> > >               if (!rollback && handle &&
> > > @@ -1917,6 +1919,9 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
> > >                       uvc_ctrl_set_handle(handle, ctrl, handle);
> > >       }
> > >
> > > +     if (ret)
> > > +             return ret;
> > > +
> > >       return processed_ctrls;
> > >  }
> > >
> > > @@ -1947,7 +1952,8 @@ 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 = 0;
> > > +     int ret_out = 0;
> > > +     int ret;
> > >
> > >       /* Find the control. */
> > >       list_for_each_entry(entity, &chain->entities, chain) {
> > > @@ -1958,17 +1964,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);
> > >               }
> > >       }
> > >
> > > -     ret = 0;
> > > -done:
> > >       mutex_unlock(&chain->ctrl_mutex);
> > > -     return ret;
> > > +     return ret_out;
> > >  }
> > >
> > >  int uvc_ctrl_get(struct uvc_video_chain *chain,

-- 
Regards,

Laurent Pinchart