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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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