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

[Ricardo Ribalda <ribalda@chromium.org>]

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