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

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

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