[PATCH 0/4] media: uvcvideo: Follow-up patches for next-media-uvc-20250509

[PATCH 0/4] media: uvcvideo: Follow-up patches for next-media-uvc-20250509

[Ricardo Ribalda]

CodeStyle and refactor patches after the last uvc Pull Request.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Ricardo Ribalda (4):
      media: uvcvideo: Refactor uvc_ctrl_set_handle()
      media: uvcvideo: Refactor uvc_queue_streamon
      media: uvcvideo: Refactor uvc_v4l2_compat_ioctl32
      media: uvcvideo: Populate all errors in uvc_probe()

 drivers/media/usb/uvc/uvc_ctrl.c   | 65 ++++++++++++++++++++------------------
 drivers/media/usb/uvc/uvc_driver.c | 15 +++------
 drivers/media/usb/uvc/uvc_v4l2.c   | 28 ++++++++--------
 3 files changed, 52 insertions(+), 56 deletions(-)
---
base-commit: 3328eb4dfec23cb3055cda24087cd1cdee925676
change-id: 20250509-uvc-followup-d97ff563df95

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

[PATCH 1/4] media: uvcvideo: Refactor uvc_ctrl_set_handle()

[Ricardo Ribalda]

Today uvc_ctrl_set_handle() covers two use-uses: setting the handle and
clearing the handle. The only common code between the two cases is the
lockdep_assert_held.

The code looks cleaner if we split these two usecases in two functions.

We also take this opportunity to use pending_async_ctrls from ctrl where
possible.

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

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 44b6513c526421943bb9841fb53dc5f8e9f93f02..26be1d0a1891cf3a9a7489f60f9a2931c78d3239 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1812,48 +1812,53 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain,
 	uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes);
 }
 
-static int uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl,
-			       struct uvc_fh *new_handle)
+static int uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl)
 {
-	lockdep_assert_held(&handle->chain->ctrl_mutex);
-
-	if (new_handle) {
-		int ret;
+	int ret;
 
-		if (ctrl->handle)
-			dev_warn_ratelimited(&handle->stream->dev->udev->dev,
-					     "UVC non compliance: Setting an async control with a pending operation.");
+	lockdep_assert_held(&handle->chain->ctrl_mutex);
 
-		if (new_handle == ctrl->handle)
-			return 0;
+	if (ctrl->handle) {
+		dev_warn_ratelimited(&handle->stream->dev->udev->dev,
+				     "UVC non compliance: Setting an async control with a pending operation.");
 
-		if (ctrl->handle) {
-			WARN_ON(!ctrl->handle->pending_async_ctrls);
-			if (ctrl->handle->pending_async_ctrls)
-				ctrl->handle->pending_async_ctrls--;
-			ctrl->handle = new_handle;
-			handle->pending_async_ctrls++;
+		if (ctrl->handle == handle)
 			return 0;
-		}
-
-		ret = uvc_pm_get(handle->chain->dev);
-		if (ret)
-			return ret;
 
-		ctrl->handle = new_handle;
-		handle->pending_async_ctrls++;
+		WARN_ON(!ctrl->handle->pending_async_ctrls);
+		if (ctrl->handle->pending_async_ctrls)
+			ctrl->handle->pending_async_ctrls--;
+		ctrl->handle = handle;
+		ctrl->handle->pending_async_ctrls++;
 		return 0;
 	}
 
+	ret = uvc_pm_get(handle->chain->dev);
+	if (ret)
+		return ret;
+
+	ctrl->handle = handle;
+	ctrl->handle->pending_async_ctrls++;
+	return 0;
+}
+
+static int uvc_ctrl_clear_handle(struct uvc_fh *handle,
+				 struct uvc_control *ctrl)
+{
+	lockdep_assert_held(&handle->chain->ctrl_mutex);
+
 	/* Cannot clear the handle for a control not owned by us.*/
 	if (WARN_ON(ctrl->handle != handle))
 		return -EINVAL;
 
-	ctrl->handle = NULL;
-	if (WARN_ON(!handle->pending_async_ctrls))
+	if (WARN_ON(!ctrl->handle->pending_async_ctrls)) {
+		ctrl->handle = NULL;
 		return -EINVAL;
-	handle->pending_async_ctrls--;
+	}
+
+	ctrl->handle->pending_async_ctrls--;
 	uvc_pm_put(handle->chain->dev);
+	ctrl->handle = NULL;
 	return 0;
 }
 
@@ -1871,7 +1876,7 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain,
 
 	handle = ctrl->handle;
 	if (handle)
-		uvc_ctrl_set_handle(handle, ctrl, NULL);
+		uvc_ctrl_clear_handle(handle, ctrl);
 
 	list_for_each_entry(mapping, &ctrl->info.mappings, list) {
 		s32 value;
@@ -2161,7 +2166,7 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
 
 		if (!rollback && handle && !ret &&
 		    ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
-			ret = uvc_ctrl_set_handle(handle, ctrl, handle);
+			ret = uvc_ctrl_set_handle(handle, ctrl);
 
 		if (ret < 0 && !rollback) {
 			if (err_ctrl)
@@ -3271,7 +3276,7 @@ void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
 		for (unsigned int i = 0; i < entity->ncontrols; ++i) {
 			if (entity->controls[i].handle != handle)
 				continue;
-			uvc_ctrl_set_handle(handle, &entity->controls[i], NULL);
+			uvc_ctrl_clear_handle(handle, &entity->controls[i]);
 		}
 	}
 

-- 
2.49.0.1015.ga840276032-goog

Re: [PATCH 1/4] media: uvcvideo: Refactor uvc_ctrl_set_handle()

[Laurent Pinchart]

Hi Ricardo,

On Fri, May 09, 2025 at 06:24:13PM +0000, Ricardo Ribalda wrote:
> Today uvc_ctrl_set_handle() covers two use-uses: setting the handle and
> clearing the handle. The only common code between the two cases is the
> lockdep_assert_held.
> 
> The code looks cleaner if we split these two usecases in two functions.

It does indeed. Thanks for pushing for this :-)

> We also take this opportunity to use pending_async_ctrls from ctrl where
> possible.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 65 +++++++++++++++++++++-------------------
>  1 file changed, 35 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 44b6513c526421943bb9841fb53dc5f8e9f93f02..26be1d0a1891cf3a9a7489f60f9a2931c78d3239 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1812,48 +1812,53 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain,
>  	uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes);
>  }
>  
> -static int uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl,
> -			       struct uvc_fh *new_handle)
> +static int uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl)
>  {
> -	lockdep_assert_held(&handle->chain->ctrl_mutex);
> -
> -	if (new_handle) {
> -		int ret;
> +	int ret;
>  
> -		if (ctrl->handle)
> -			dev_warn_ratelimited(&handle->stream->dev->udev->dev,
> -					     "UVC non compliance: Setting an async control with a pending operation.");
> +	lockdep_assert_held(&handle->chain->ctrl_mutex);
>  
> -		if (new_handle == ctrl->handle)
> -			return 0;
> +	if (ctrl->handle) {
> +		dev_warn_ratelimited(&handle->stream->dev->udev->dev,
> +				     "UVC non compliance: Setting an async control with a pending operation.");
>  
> -		if (ctrl->handle) {
> -			WARN_ON(!ctrl->handle->pending_async_ctrls);
> -			if (ctrl->handle->pending_async_ctrls)
> -				ctrl->handle->pending_async_ctrls--;
> -			ctrl->handle = new_handle;
> -			handle->pending_async_ctrls++;
> +		if (ctrl->handle == handle)
>  			return 0;
> -		}
> -
> -		ret = uvc_pm_get(handle->chain->dev);
> -		if (ret)
> -			return ret;
>  
> -		ctrl->handle = new_handle;
> -		handle->pending_async_ctrls++;
> +		WARN_ON(!ctrl->handle->pending_async_ctrls);
> +		if (ctrl->handle->pending_async_ctrls)
> +			ctrl->handle->pending_async_ctrls--;
> +		ctrl->handle = handle;
> +		ctrl->handle->pending_async_ctrls++;
>  		return 0;
>  	}
>  
> +	ret = uvc_pm_get(handle->chain->dev);
> +	if (ret)
> +		return ret;
> +
> +	ctrl->handle = handle;
> +	ctrl->handle->pending_async_ctrls++;
> +	return 0;
> +}
> +
> +static int uvc_ctrl_clear_handle(struct uvc_fh *handle,
> +				 struct uvc_control *ctrl)
> +{
> +	lockdep_assert_held(&handle->chain->ctrl_mutex);
> +
>  	/* Cannot clear the handle for a control not owned by us.*/

While at it, I'll add a space before */ when applying.

>  	if (WARN_ON(ctrl->handle != handle))
>  		return -EINVAL;

But actually, as the caller guarantees that handle == ctrl->handle in
both call sites (renaming the function makes this clear), can we drop
the handle argument to this function ?

If that's fine with you, I'd like to also change the
uvc_ctrl_set_handle() function to pass the ctrl argument first.

>  
> -	ctrl->handle = NULL;
> -	if (WARN_ON(!handle->pending_async_ctrls))
> +	if (WARN_ON(!ctrl->handle->pending_async_ctrls)) {
> +		ctrl->handle = NULL;
>  		return -EINVAL;
> -	handle->pending_async_ctrls--;
> +	}
> +
> +	ctrl->handle->pending_async_ctrls--;
>  	uvc_pm_put(handle->chain->dev);

This will need to become

 	uvc_pm_put(ctrl->handle->chain->dev);

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

> +	ctrl->handle = NULL;
>  	return 0;
>  }
>  
> @@ -1871,7 +1876,7 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain,
>  
>  	handle = ctrl->handle;
>  	if (handle)
> -		uvc_ctrl_set_handle(handle, ctrl, NULL);
> +		uvc_ctrl_clear_handle(handle, ctrl);
>  
>  	list_for_each_entry(mapping, &ctrl->info.mappings, list) {
>  		s32 value;
> @@ -2161,7 +2166,7 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
>  
>  		if (!rollback && handle && !ret &&
>  		    ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
> -			ret = uvc_ctrl_set_handle(handle, ctrl, handle);
> +			ret = uvc_ctrl_set_handle(handle, ctrl);
>  
>  		if (ret < 0 && !rollback) {
>  			if (err_ctrl)
> @@ -3271,7 +3276,7 @@ void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
>  		for (unsigned int i = 0; i < entity->ncontrols; ++i) {
>  			if (entity->controls[i].handle != handle)
>  				continue;
> -			uvc_ctrl_set_handle(handle, &entity->controls[i], NULL);
> +			uvc_ctrl_clear_handle(handle, &entity->controls[i]);
>  		}
>  	}
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/4] media: uvcvideo: Refactor uvc_ctrl_set_handle()

[Ricardo Ribalda]

Hi Laurent

On Fri, 23 May 2025 at 10:53, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> On Fri, May 09, 2025 at 06:24:13PM +0000, Ricardo Ribalda wrote:
> > Today uvc_ctrl_set_handle() covers two use-uses: setting the handle and
> > clearing the handle. The only common code between the two cases is the
> > lockdep_assert_held.
> >
> > The code looks cleaner if we split these two usecases in two functions.
>
> It does indeed. Thanks for pushing for this :-)
>
> > We also take this opportunity to use pending_async_ctrls from ctrl where
> > possible.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_ctrl.c | 65 +++++++++++++++++++++-------------------
> >  1 file changed, 35 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index 44b6513c526421943bb9841fb53dc5f8e9f93f02..26be1d0a1891cf3a9a7489f60f9a2931c78d3239 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -1812,48 +1812,53 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain,
> >       uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes);
> >  }
> >
> > -static int uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl,
> > -                            struct uvc_fh *new_handle)
> > +static int uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl)
> >  {
> > -     lockdep_assert_held(&handle->chain->ctrl_mutex);
> > -
> > -     if (new_handle) {
> > -             int ret;
> > +     int ret;
> >
> > -             if (ctrl->handle)
> > -                     dev_warn_ratelimited(&handle->stream->dev->udev->dev,
> > -                                          "UVC non compliance: Setting an async control with a pending operation.");
> > +     lockdep_assert_held(&handle->chain->ctrl_mutex);
> >
> > -             if (new_handle == ctrl->handle)
> > -                     return 0;
> > +     if (ctrl->handle) {
> > +             dev_warn_ratelimited(&handle->stream->dev->udev->dev,
> > +                                  "UVC non compliance: Setting an async control with a pending operation.");
> >
> > -             if (ctrl->handle) {
> > -                     WARN_ON(!ctrl->handle->pending_async_ctrls);
> > -                     if (ctrl->handle->pending_async_ctrls)
> > -                             ctrl->handle->pending_async_ctrls--;
> > -                     ctrl->handle = new_handle;
> > -                     handle->pending_async_ctrls++;
> > +             if (ctrl->handle == handle)
> >                       return 0;
> > -             }
> > -
> > -             ret = uvc_pm_get(handle->chain->dev);
> > -             if (ret)
> > -                     return ret;
> >
> > -             ctrl->handle = new_handle;
> > -             handle->pending_async_ctrls++;
> > +             WARN_ON(!ctrl->handle->pending_async_ctrls);
> > +             if (ctrl->handle->pending_async_ctrls)
> > +                     ctrl->handle->pending_async_ctrls--;
> > +             ctrl->handle = handle;
> > +             ctrl->handle->pending_async_ctrls++;
> >               return 0;
> >       }
> >
> > +     ret = uvc_pm_get(handle->chain->dev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ctrl->handle = handle;
> > +     ctrl->handle->pending_async_ctrls++;
> > +     return 0;
> > +}
> > +
> > +static int uvc_ctrl_clear_handle(struct uvc_fh *handle,
> > +                              struct uvc_control *ctrl)
> > +{
> > +     lockdep_assert_held(&handle->chain->ctrl_mutex);
> > +
> >       /* Cannot clear the handle for a control not owned by us.*/
>
> While at it, I'll add a space before */ when applying.
>
> >       if (WARN_ON(ctrl->handle != handle))
> >               return -EINVAL;
>
> But actually, as the caller guarantees that handle == ctrl->handle in
> both call sites (renaming the function makes this clear), can we drop
> the handle argument to this function ?
>
> If that's fine with you, I'd like to also change the
> uvc_ctrl_set_handle() function to pass the ctrl argument first.

SGTM, let me know if you have time to do this or if you prefer that I do it.

Cheers


>
> >
> > -     ctrl->handle = NULL;
> > -     if (WARN_ON(!handle->pending_async_ctrls))
> > +     if (WARN_ON(!ctrl->handle->pending_async_ctrls)) {
> > +             ctrl->handle = NULL;
> >               return -EINVAL;
> > -     handle->pending_async_ctrls--;
> > +     }
> > +
> > +     ctrl->handle->pending_async_ctrls--;
> >       uvc_pm_put(handle->chain->dev);
>
> This will need to become
>
>         uvc_pm_put(ctrl->handle->chain->dev);
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > +     ctrl->handle = NULL;
> >       return 0;
> >  }
> >
> > @@ -1871,7 +1876,7 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain,
> >
> >       handle = ctrl->handle;
> >       if (handle)
> > -             uvc_ctrl_set_handle(handle, ctrl, NULL);
> > +             uvc_ctrl_clear_handle(handle, ctrl);
> >
> >       list_for_each_entry(mapping, &ctrl->info.mappings, list) {
> >               s32 value;
> > @@ -2161,7 +2166,7 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
> >
> >               if (!rollback && handle && !ret &&
> >                   ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
> > -                     ret = uvc_ctrl_set_handle(handle, ctrl, handle);
> > +                     ret = uvc_ctrl_set_handle(handle, ctrl);
> >
> >               if (ret < 0 && !rollback) {
> >                       if (err_ctrl)
> > @@ -3271,7 +3276,7 @@ void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
> >               for (unsigned int i = 0; i < entity->ncontrols; ++i) {
> >                       if (entity->controls[i].handle != handle)
> >                               continue;
> > -                     uvc_ctrl_set_handle(handle, &entity->controls[i], NULL);
> > +                     uvc_ctrl_clear_handle(handle, &entity->controls[i]);
> >               }
> >       }
> >
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

Re: [PATCH 1/4] media: uvcvideo: Refactor uvc_ctrl_set_handle()

[Laurent Pinchart]

On Fri, May 23, 2025 at 12:58:31PM +0200, Ricardo Ribalda wrote:
> On Fri, 23 May 2025 at 10:53, Laurent Pinchart wrote:
> > On Fri, May 09, 2025 at 06:24:13PM +0000, Ricardo Ribalda wrote:
> > > Today uvc_ctrl_set_handle() covers two use-uses: setting the handle and
> > > clearing the handle. The only common code between the two cases is the
> > > lockdep_assert_held.
> > >
> > > The code looks cleaner if we split these two usecases in two functions.
> >
> > It does indeed. Thanks for pushing for this :-)
> >
> > > We also take this opportunity to use pending_async_ctrls from ctrl where
> > > possible.
> > >
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > >  drivers/media/usb/uvc/uvc_ctrl.c | 65 +++++++++++++++++++++-------------------
> > >  1 file changed, 35 insertions(+), 30 deletions(-)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > > index 44b6513c526421943bb9841fb53dc5f8e9f93f02..26be1d0a1891cf3a9a7489f60f9a2931c78d3239 100644
> > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > @@ -1812,48 +1812,53 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain,
> > >       uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes);
> > >  }
> > >
> > > -static int uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl,
> > > -                            struct uvc_fh *new_handle)
> > > +static int uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl)
> > >  {
> > > -     lockdep_assert_held(&handle->chain->ctrl_mutex);
> > > -
> > > -     if (new_handle) {
> > > -             int ret;
> > > +     int ret;
> > >
> > > -             if (ctrl->handle)
> > > -                     dev_warn_ratelimited(&handle->stream->dev->udev->dev,
> > > -                                          "UVC non compliance: Setting an async control with a pending operation.");
> > > +     lockdep_assert_held(&handle->chain->ctrl_mutex);
> > >
> > > -             if (new_handle == ctrl->handle)
> > > -                     return 0;
> > > +     if (ctrl->handle) {
> > > +             dev_warn_ratelimited(&handle->stream->dev->udev->dev,
> > > +                                  "UVC non compliance: Setting an async control with a pending operation.");
> > >
> > > -             if (ctrl->handle) {
> > > -                     WARN_ON(!ctrl->handle->pending_async_ctrls);
> > > -                     if (ctrl->handle->pending_async_ctrls)
> > > -                             ctrl->handle->pending_async_ctrls--;
> > > -                     ctrl->handle = new_handle;
> > > -                     handle->pending_async_ctrls++;
> > > +             if (ctrl->handle == handle)
> > >                       return 0;
> > > -             }
> > > -
> > > -             ret = uvc_pm_get(handle->chain->dev);
> > > -             if (ret)
> > > -                     return ret;
> > >
> > > -             ctrl->handle = new_handle;
> > > -             handle->pending_async_ctrls++;
> > > +             WARN_ON(!ctrl->handle->pending_async_ctrls);
> > > +             if (ctrl->handle->pending_async_ctrls)
> > > +                     ctrl->handle->pending_async_ctrls--;
> > > +             ctrl->handle = handle;
> > > +             ctrl->handle->pending_async_ctrls++;
> > >               return 0;
> > >       }
> > >
> > > +     ret = uvc_pm_get(handle->chain->dev);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     ctrl->handle = handle;
> > > +     ctrl->handle->pending_async_ctrls++;
> > > +     return 0;
> > > +}
> > > +
> > > +static int uvc_ctrl_clear_handle(struct uvc_fh *handle,
> > > +                              struct uvc_control *ctrl)
> > > +{
> > > +     lockdep_assert_held(&handle->chain->ctrl_mutex);
> > > +
> > >       /* Cannot clear the handle for a control not owned by us.*/
> >
> > While at it, I'll add a space before */ when applying.
> >
> > >       if (WARN_ON(ctrl->handle != handle))
> > >               return -EINVAL;
> >
> > But actually, as the caller guarantees that handle == ctrl->handle in
> > both call sites (renaming the function makes this clear), can we drop
> > the handle argument to this function ?
> >
> > If that's fine with you, I'd like to also change the
> > uvc_ctrl_set_handle() function to pass the ctrl argument first.
> 
> SGTM, let me know if you have time to do this or if you prefer that I do it.

I'll send a new version of this patch.

> > > -     ctrl->handle = NULL;
> > > -     if (WARN_ON(!handle->pending_async_ctrls))
> > > +     if (WARN_ON(!ctrl->handle->pending_async_ctrls)) {
> > > +             ctrl->handle = NULL;
> > >               return -EINVAL;
> > > -     handle->pending_async_ctrls--;
> > > +     }
> > > +
> > > +     ctrl->handle->pending_async_ctrls--;
> > >       uvc_pm_put(handle->chain->dev);
> >
> > This will need to become
> >
> >         uvc_pm_put(ctrl->handle->chain->dev);
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > > +     ctrl->handle = NULL;
> > >       return 0;
> > >  }
> > >
> > > @@ -1871,7 +1876,7 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain,
> > >
> > >       handle = ctrl->handle;
> > >       if (handle)
> > > -             uvc_ctrl_set_handle(handle, ctrl, NULL);
> > > +             uvc_ctrl_clear_handle(handle, ctrl);
> > >
> > >       list_for_each_entry(mapping, &ctrl->info.mappings, list) {
> > >               s32 value;
> > > @@ -2161,7 +2166,7 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
> > >
> > >               if (!rollback && handle && !ret &&
> > >                   ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
> > > -                     ret = uvc_ctrl_set_handle(handle, ctrl, handle);
> > > +                     ret = uvc_ctrl_set_handle(handle, ctrl);
> > >
> > >               if (ret < 0 && !rollback) {
> > >                       if (err_ctrl)
> > > @@ -3271,7 +3276,7 @@ void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
> > >               for (unsigned int i = 0; i < entity->ncontrols; ++i) {
> > >                       if (entity->controls[i].handle != handle)
> > >                               continue;
> > > -                     uvc_ctrl_set_handle(handle, &entity->controls[i], NULL);
> > > +                     uvc_ctrl_clear_handle(handle, &entity->controls[i]);
> > >               }
> > >       }
> > >

-- 
Regards,

Laurent Pinchart

[PATCH v1.1 1/4] media: uvcvideo: Refactor uvc_ctrl_set_handle()

[Laurent Pinchart]

From: Ricardo Ribalda <ribalda@chromium.org>

Today uvc_ctrl_set_handle() covers two use-uses: setting the handle and
clearing the handle. The only common code between the two cases is the
lockdep_assert_held.

The code looks cleaner if we split these two usecases in two functions.

We also take this opportunity to use pending_async_ctrls from ctrl where
possible.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Link: https://lore.kernel.org/r/20250509-uvc-followup-v1-1-73bcde30d2b5@chromium.org
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:

- Flip arguments order to uvc_ctrl_set_handle()
- Drop handle argument to uvc_ctrl_clear_handle()
---
 drivers/media/usb/uvc/uvc_ctrl.c | 68 ++++++++++++++++----------------
 1 file changed, 34 insertions(+), 34 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index f24272d483a2..303b7509ec47 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1851,48 +1851,48 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain,
 	uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes);
 }
 
-static int uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl,
-			       struct uvc_fh *new_handle)
+static int uvc_ctrl_set_handle(struct uvc_control *ctrl, struct uvc_fh *handle)
 {
+	int ret;
+
 	lockdep_assert_held(&handle->chain->ctrl_mutex);
 
-	if (new_handle) {
-		int ret;
+	if (ctrl->handle) {
+		dev_warn_ratelimited(&handle->stream->dev->udev->dev,
+				     "UVC non compliance: Setting an async control with a pending operation.");
 
-		if (ctrl->handle)
-			dev_warn_ratelimited(&handle->stream->dev->udev->dev,
-					     "UVC non compliance: Setting an async control with a pending operation.");
-
-		if (new_handle == ctrl->handle)
+		if (ctrl->handle == handle)
 			return 0;
 
-		if (ctrl->handle) {
-			WARN_ON(!ctrl->handle->pending_async_ctrls);
-			if (ctrl->handle->pending_async_ctrls)
-				ctrl->handle->pending_async_ctrls--;
-			ctrl->handle = new_handle;
-			handle->pending_async_ctrls++;
-			return 0;
-		}
-
-		ret = uvc_pm_get(handle->chain->dev);
-		if (ret)
-			return ret;
-
-		ctrl->handle = new_handle;
-		handle->pending_async_ctrls++;
+		WARN_ON(!ctrl->handle->pending_async_ctrls);
+		if (ctrl->handle->pending_async_ctrls)
+			ctrl->handle->pending_async_ctrls--;
+		ctrl->handle = handle;
+		ctrl->handle->pending_async_ctrls++;
 		return 0;
 	}
 
-	/* Cannot clear the handle for a control not owned by us.*/
-	if (WARN_ON(ctrl->handle != handle))
-		return -EINVAL;
+	ret = uvc_pm_get(handle->chain->dev);
+	if (ret)
+		return ret;
 
-	ctrl->handle = NULL;
-	if (WARN_ON(!handle->pending_async_ctrls))
+	ctrl->handle = handle;
+	ctrl->handle->pending_async_ctrls++;
+	return 0;
+}
+
+static int uvc_ctrl_clear_handle(struct uvc_control *ctrl)
+{
+	lockdep_assert_held(&ctrl->handle->chain->ctrl_mutex);
+
+	if (WARN_ON(!ctrl->handle->pending_async_ctrls)) {
+		ctrl->handle = NULL;
 		return -EINVAL;
-	handle->pending_async_ctrls--;
-	uvc_pm_put(handle->chain->dev);
+	}
+
+	ctrl->handle->pending_async_ctrls--;
+	uvc_pm_put(ctrl->handle->chain->dev);
+	ctrl->handle = NULL;
 	return 0;
 }
 
@@ -1910,7 +1910,7 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain,
 
 	handle = ctrl->handle;
 	if (handle)
-		uvc_ctrl_set_handle(handle, ctrl, NULL);
+		uvc_ctrl_clear_handle(ctrl);
 
 	list_for_each_entry(mapping, &ctrl->info.mappings, list) {
 		s32 value;
@@ -2200,7 +2200,7 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
 
 		if (!rollback && handle && !ret &&
 		    ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
-			ret = uvc_ctrl_set_handle(handle, ctrl, handle);
+			ret = uvc_ctrl_set_handle(ctrl, handle);
 
 		if (ret < 0 && !rollback) {
 			if (err_ctrl)
@@ -3310,7 +3310,7 @@ void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
 		for (unsigned int i = 0; i < entity->ncontrols; ++i) {
 			if (entity->controls[i].handle != handle)
 				continue;
-			uvc_ctrl_set_handle(handle, &entity->controls[i], NULL);
+			uvc_ctrl_clear_handle(&entity->controls[i]);
 		}
 	}
 

base-commit: 5e1ff2314797bf53636468a97719a8222deca9ae
prerequisite-patch-id: 43cdc0977a6b8dd7167a6b2f5810d655e710cbeb
prerequisite-patch-id: 56e97ab2a2a6948c72aed53f53752a78e85de7c7
-- 
Regards,

Laurent Pinchart

[PATCH 2/4] media: uvcvideo: Refactor uvc_queue_streamon

[Ricardo Ribalda]

Do uvc_pm_get before we call uvc_queue_streamon. Although the current
code is correct, uvc_ioctl_streamon is allways called after uvc_pm_get,
this change makes the code more resiliant to future changes.

Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_v4l2.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 668a4e9d772c6d91f045ca75e2744b3a6c69da6b..862b4e34e5b629cf324479a9bb59ebe8784ccd5d 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -853,15 +853,16 @@ static int uvc_ioctl_streamon(struct file *file, void *fh,
 	if (handle->is_streaming)
 		return 0;
 
-	ret = uvc_queue_streamon(&stream->queue, type);
+	ret = uvc_pm_get(stream->dev);
 	if (ret)
 		return ret;
 
-	ret = uvc_pm_get(stream->dev);
+	ret = uvc_queue_streamon(&stream->queue, type);
 	if (ret) {
-		uvc_queue_streamoff(&stream->queue, type);
+		uvc_pm_put(stream->dev);
 		return ret;
 	}
+
 	handle->is_streaming = true;
 
 	return 0;

-- 
2.49.0.1015.ga840276032-goog

Re: [PATCH 2/4] media: uvcvideo: Refactor uvc_queue_streamon

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Fri, May 09, 2025 at 06:24:14PM +0000, Ricardo Ribalda wrote:
> Do uvc_pm_get before we call uvc_queue_streamon. Although the current
> code is correct, uvc_ioctl_streamon is allways called after uvc_pm_get,
> this change makes the code more resiliant to future changes.
> 
> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

I've reviewed this as part of a separate series, and I'll copy

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

here for patchwork to pick.

> ---
>  drivers/media/usb/uvc/uvc_v4l2.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 668a4e9d772c6d91f045ca75e2744b3a6c69da6b..862b4e34e5b629cf324479a9bb59ebe8784ccd5d 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -853,15 +853,16 @@ static int uvc_ioctl_streamon(struct file *file, void *fh,
>  	if (handle->is_streaming)
>  		return 0;
>  
> -	ret = uvc_queue_streamon(&stream->queue, type);
> +	ret = uvc_pm_get(stream->dev);
>  	if (ret)
>  		return ret;
>  
> -	ret = uvc_pm_get(stream->dev);
> +	ret = uvc_queue_streamon(&stream->queue, type);
>  	if (ret) {
> -		uvc_queue_streamoff(&stream->queue, type);
> +		uvc_pm_put(stream->dev);
>  		return ret;
>  	}
> +
>  	handle->is_streaming = true;
>  
>  	return 0;

-- 
Regards,

Laurent Pinchart

[PATCH 3/4] media: uvcvideo: Refactor uvc_v4l2_compat_ioctl32

[Ricardo Ribalda]

Declaring a variable for doing automatic cleanup is not a very common
pattern. Replace the cleanup macro with manual cleanup to make the code
simpler.

Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_v4l2.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 862b4e34e5b629cf324479a9bb59ebe8784ccd5d..fe3db03d7458eeb4b9a846ae4ed141bb60ea46eb 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -1382,11 +1382,9 @@ static int uvc_v4l2_put_xu_query(const struct uvc_xu_control_query *kp,
 #define UVCIOC_CTRL_MAP32	_IOWR('u', 0x20, struct uvc_xu_control_mapping32)
 #define UVCIOC_CTRL_QUERY32	_IOWR('u', 0x21, struct uvc_xu_control_query32)
 
-DEFINE_FREE(uvc_pm_put, struct uvc_device *, if (_T) uvc_pm_put(_T))
 static long uvc_v4l2_compat_ioctl32(struct file *file,
 		     unsigned int cmd, unsigned long arg)
 {
-	struct uvc_device *uvc_device __free(uvc_pm_put) = NULL;
 	struct uvc_fh *handle = file->private_data;
 	union {
 		struct uvc_xu_control_mapping xmap;
@@ -1399,38 +1397,37 @@ static long uvc_v4l2_compat_ioctl32(struct file *file,
 	if (ret)
 		return ret;
 
-	uvc_device = handle->stream->dev;
-
 	switch (cmd) {
 	case UVCIOC_CTRL_MAP32:
 		ret = uvc_v4l2_get_xu_mapping(&karg.xmap, up);
 		if (ret)
-			return ret;
+			break;
 		ret = uvc_ioctl_xu_ctrl_map(handle->chain, &karg.xmap);
 		if (ret)
-			return ret;
+			break;
 		ret = uvc_v4l2_put_xu_mapping(&karg.xmap, up);
 		if (ret)
-			return ret;
-
+			break;
 		break;
 
 	case UVCIOC_CTRL_QUERY32:
 		ret = uvc_v4l2_get_xu_query(&karg.xqry, up);
 		if (ret)
-			return ret;
+			break;
 		ret = uvc_xu_ctrl_query(handle->chain, &karg.xqry);
 		if (ret)
-			return ret;
+			break;
 		ret = uvc_v4l2_put_xu_query(&karg.xqry, up);
 		if (ret)
-			return ret;
+			break;
 		break;
 
 	default:
-		return -ENOIOCTLCMD;
+		ret = -ENOIOCTLCMD;
 	}
 
+	uvc_pm_put(handle->stream->dev);
+
 	return ret;
 }
 #endif

-- 
2.49.0.1015.ga840276032-goog

Re: [PATCH 3/4] media: uvcvideo: Refactor uvc_v4l2_compat_ioctl32

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Fri, May 09, 2025 at 06:24:15PM +0000, Ricardo Ribalda wrote:
> Declaring a variable for doing automatic cleanup is not a very common
> pattern. Replace the cleanup macro with manual cleanup to make the code
> simpler.
> 
> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_v4l2.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 862b4e34e5b629cf324479a9bb59ebe8784ccd5d..fe3db03d7458eeb4b9a846ae4ed141bb60ea46eb 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -1382,11 +1382,9 @@ static int uvc_v4l2_put_xu_query(const struct uvc_xu_control_query *kp,
>  #define UVCIOC_CTRL_MAP32	_IOWR('u', 0x20, struct uvc_xu_control_mapping32)
>  #define UVCIOC_CTRL_QUERY32	_IOWR('u', 0x21, struct uvc_xu_control_query32)
>  
> -DEFINE_FREE(uvc_pm_put, struct uvc_device *, if (_T) uvc_pm_put(_T))
>  static long uvc_v4l2_compat_ioctl32(struct file *file,
>  		     unsigned int cmd, unsigned long arg)
>  {
> -	struct uvc_device *uvc_device __free(uvc_pm_put) = NULL;
>  	struct uvc_fh *handle = file->private_data;
>  	union {
>  		struct uvc_xu_control_mapping xmap;
> @@ -1399,38 +1397,37 @@ static long uvc_v4l2_compat_ioctl32(struct file *file,
>  	if (ret)
>  		return ret;
>  
> -	uvc_device = handle->stream->dev;
> -
>  	switch (cmd) {
>  	case UVCIOC_CTRL_MAP32:
>  		ret = uvc_v4l2_get_xu_mapping(&karg.xmap, up);
>  		if (ret)
> -			return ret;
> +			break;
>  		ret = uvc_ioctl_xu_ctrl_map(handle->chain, &karg.xmap);
>  		if (ret)
> -			return ret;
> +			break;
>  		ret = uvc_v4l2_put_xu_mapping(&karg.xmap, up);
>  		if (ret)
> -			return ret;
> -
> +			break;
>  		break;
>  
>  	case UVCIOC_CTRL_QUERY32:
>  		ret = uvc_v4l2_get_xu_query(&karg.xqry, up);
>  		if (ret)
> -			return ret;
> +			break;
>  		ret = uvc_xu_ctrl_query(handle->chain, &karg.xqry);
>  		if (ret)
> -			return ret;
> +			break;
>  		ret = uvc_v4l2_put_xu_query(&karg.xqry, up);
>  		if (ret)
> -			return ret;
> +			break;
>  		break;
>  
>  	default:
> -		return -ENOIOCTLCMD;
> +		ret = -ENOIOCTLCMD;

I'll add a

		break;

here.

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

>  	}
>  
> +	uvc_pm_put(handle->stream->dev);
> +
>  	return ret;
>  }
>  #endif

-- 
Regards,

Laurent Pinchart

[PATCH 4/4] media: uvcvideo: Populate all errors in uvc_probe()

[Ricardo Ribalda]

Now we are replacing most of the error codes with -ENODEV.
Instead, Populate the error code from the functions called by
uvc_probe().

Take this opportunity to replace a generic error code from
uvc_scan_device() into something more meaningful.

Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_driver.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index da24a655ab68cc0957762f2b67387677c22224d1..cdf4bbe582272277a6a95267e9752010adc51b6b 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1866,7 +1866,7 @@ static int uvc_scan_device(struct uvc_device *dev)
 
 	if (list_empty(&dev->chains)) {
 		dev_info(&dev->udev->dev, "No valid video chain found.\n");
-		return -1;
+		return -ENODEV;
 	}
 
 	/* Add GPIO entity to the first chain. */
@@ -2239,7 +2239,6 @@ static int uvc_probe(struct usb_interface *intf,
 	/* Parse the Video Class control descriptor. */
 	ret = uvc_parse_control(dev);
 	if (ret < 0) {
-		ret = -ENODEV;
 		uvc_dbg(dev, PROBE, "Unable to parse UVC descriptors\n");
 		goto error;
 	}
@@ -2275,22 +2274,16 @@ static int uvc_probe(struct usb_interface *intf,
 		goto error;
 
 	/* Scan the device for video chains. */
-	if (uvc_scan_device(dev) < 0) {
-		ret = -ENODEV;
+	if (uvc_scan_device(dev) < 0)
 		goto error;
-	}
 
 	/* Initialize controls. */
-	if (uvc_ctrl_init_device(dev) < 0) {
-		ret = -ENODEV;
+	if (uvc_ctrl_init_device(dev) < 0)
 		goto error;
-	}
 
 	/* Register video device nodes. */
-	if (uvc_register_chains(dev) < 0) {
-		ret = -ENODEV;
+	if (uvc_register_chains(dev) < 0)
 		goto error;
-	}
 
 #ifdef CONFIG_MEDIA_CONTROLLER
 	/* Register the media device node */

-- 
2.49.0.1015.ga840276032-goog

Re: [PATCH 4/4] media: uvcvideo: Populate all errors in uvc_probe()

[Laurent Pinchart]

Hi Ricardo,

On Fri, May 09, 2025 at 06:24:16PM +0000, Ricardo Ribalda wrote:
> Now we are replacing most of the error codes with -ENODEV.
> Instead, Populate the error code from the functions called by
> uvc_probe().
> 
> Take this opportunity to replace a generic error code from
> uvc_scan_device() into something more meaningful.
> 
> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index da24a655ab68cc0957762f2b67387677c22224d1..cdf4bbe582272277a6a95267e9752010adc51b6b 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1866,7 +1866,7 @@ static int uvc_scan_device(struct uvc_device *dev)
>  
>  	if (list_empty(&dev->chains)) {
>  		dev_info(&dev->udev->dev, "No valid video chain found.\n");
> -		return -1;
> +		return -ENODEV;
>  	}
>  
>  	/* Add GPIO entity to the first chain. */
> @@ -2239,7 +2239,6 @@ static int uvc_probe(struct usb_interface *intf,
>  	/* Parse the Video Class control descriptor. */
>  	ret = uvc_parse_control(dev);
>  	if (ret < 0) {
> -		ret = -ENODEV;
>  		uvc_dbg(dev, PROBE, "Unable to parse UVC descriptors\n");
>  		goto error;
>  	}
> @@ -2275,22 +2274,16 @@ static int uvc_probe(struct usb_interface *intf,
>  		goto error;
>  
>  	/* Scan the device for video chains. */
> -	if (uvc_scan_device(dev) < 0) {
> -		ret = -ENODEV;
> +	if (uvc_scan_device(dev) < 0)

That's not going to work. You probably meant

	ret = uvc_scan_device(dev);
	if (ret < 0)

Same elsewhere where applicable.

>  		goto error;
> -	}
>  
>  	/* Initialize controls. */
> -	if (uvc_ctrl_init_device(dev) < 0) {
> -		ret = -ENODEV;
> +	if (uvc_ctrl_init_device(dev) < 0)
>  		goto error;
> -	}
>  
>  	/* Register video device nodes. */
> -	if (uvc_register_chains(dev) < 0) {
> -		ret = -ENODEV;
> +	if (uvc_register_chains(dev) < 0)
>  		goto error;
> -	}
>  
>  #ifdef CONFIG_MEDIA_CONTROLLER
>  	/* Register the media device node */

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/4] media: uvcvideo: Populate all errors in uvc_probe()

[Ricardo Ribalda]

Hi Laurent

On Fri, 23 May 2025 at 13:58, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> On Fri, May 09, 2025 at 06:24:16PM +0000, Ricardo Ribalda wrote:
> > Now we are replacing most of the error codes with -ENODEV.
> > Instead, Populate the error code from the functions called by
> > uvc_probe().
> >
> > Take this opportunity to replace a generic error code from
> > uvc_scan_device() into something more meaningful.
> >
> > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_driver.c | 15 ++++-----------
> >  1 file changed, 4 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index da24a655ab68cc0957762f2b67387677c22224d1..cdf4bbe582272277a6a95267e9752010adc51b6b 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -1866,7 +1866,7 @@ static int uvc_scan_device(struct uvc_device *dev)
> >
> >       if (list_empty(&dev->chains)) {
> >               dev_info(&dev->udev->dev, "No valid video chain found.\n");
> > -             return -1;
> > +             return -ENODEV;
> >       }
> >
> >       /* Add GPIO entity to the first chain. */
> > @@ -2239,7 +2239,6 @@ static int uvc_probe(struct usb_interface *intf,
> >       /* Parse the Video Class control descriptor. */
> >       ret = uvc_parse_control(dev);
> >       if (ret < 0) {
> > -             ret = -ENODEV;
> >               uvc_dbg(dev, PROBE, "Unable to parse UVC descriptors\n");
> >               goto error;
> >       }
> > @@ -2275,22 +2274,16 @@ static int uvc_probe(struct usb_interface *intf,
> >               goto error;
> >
> >       /* Scan the device for video chains. */
> > -     if (uvc_scan_device(dev) < 0) {
> > -             ret = -ENODEV;
> > +     if (uvc_scan_device(dev) < 0)
>
> That's not going to work. You probably meant
>
>         ret = uvc_scan_device(dev);
>         if (ret < 0)

Ups, seems like I sent a partial path :S

Sorry about that. Shall I resend 4/4 or you want to take it as well?

>
> Same elsewhere where applicable.
>
> >               goto error;
> > -     }
> >
> >       /* Initialize controls. */
> > -     if (uvc_ctrl_init_device(dev) < 0) {
> > -             ret = -ENODEV;
> > +     if (uvc_ctrl_init_device(dev) < 0)
> >               goto error;
> > -     }
> >
> >       /* Register video device nodes. */
> > -     if (uvc_register_chains(dev) < 0) {
> > -             ret = -ENODEV;
> > +     if (uvc_register_chains(dev) < 0)
> >               goto error;
> > -     }
> >
> >  #ifdef CONFIG_MEDIA_CONTROLLER
> >       /* Register the media device node */
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

Re: [PATCH 4/4] media: uvcvideo: Populate all errors in uvc_probe()

[Laurent Pinchart]

On Fri, May 23, 2025 at 02:24:00PM +0200, Ricardo Ribalda wrote:
> On Fri, 23 May 2025 at 13:58, Laurent Pinchart wrote:
> > On Fri, May 09, 2025 at 06:24:16PM +0000, Ricardo Ribalda wrote:
> > > Now we are replacing most of the error codes with -ENODEV.
> > > Instead, Populate the error code from the functions called by
> > > uvc_probe().
> > >
> > > Take this opportunity to replace a generic error code from
> > > uvc_scan_device() into something more meaningful.
> > >
> > > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > >  drivers/media/usb/uvc/uvc_driver.c | 15 ++++-----------
> > >  1 file changed, 4 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > index da24a655ab68cc0957762f2b67387677c22224d1..cdf4bbe582272277a6a95267e9752010adc51b6b 100644
> > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > @@ -1866,7 +1866,7 @@ static int uvc_scan_device(struct uvc_device *dev)
> > >
> > >       if (list_empty(&dev->chains)) {
> > >               dev_info(&dev->udev->dev, "No valid video chain found.\n");
> > > -             return -1;
> > > +             return -ENODEV;
> > >       }
> > >
> > >       /* Add GPIO entity to the first chain. */
> > > @@ -2239,7 +2239,6 @@ static int uvc_probe(struct usb_interface *intf,
> > >       /* Parse the Video Class control descriptor. */
> > >       ret = uvc_parse_control(dev);
> > >       if (ret < 0) {
> > > -             ret = -ENODEV;
> > >               uvc_dbg(dev, PROBE, "Unable to parse UVC descriptors\n");
> > >               goto error;
> > >       }
> > > @@ -2275,22 +2274,16 @@ static int uvc_probe(struct usb_interface *intf,
> > >               goto error;
> > >
> > >       /* Scan the device for video chains. */
> > > -     if (uvc_scan_device(dev) < 0) {
> > > -             ret = -ENODEV;
> > > +     if (uvc_scan_device(dev) < 0)
> >
> > That's not going to work. You probably meant
> >
> >         ret = uvc_scan_device(dev);
> >         if (ret < 0)
> 
> Ups, seems like I sent a partial path :S
> 
> Sorry about that. Shall I resend 4/4 or you want to take it as well?

Could you resend just 4/4 ? I'll handle the rest.

> > Same elsewhere where applicable.
> >
> > >               goto error;
> > > -     }
> > >
> > >       /* Initialize controls. */
> > > -     if (uvc_ctrl_init_device(dev) < 0) {
> > > -             ret = -ENODEV;
> > > +     if (uvc_ctrl_init_device(dev) < 0)
> > >               goto error;
> > > -     }
> > >
> > >       /* Register video device nodes. */
> > > -     if (uvc_register_chains(dev) < 0) {
> > > -             ret = -ENODEV;
> > > +     if (uvc_register_chains(dev) < 0)
> > >               goto error;
> > > -     }
> > >
> > >  #ifdef CONFIG_MEDIA_CONTROLLER
> > >       /* Register the media device node */

-- 
Regards,

Laurent Pinchart