| Message ID | 20241202-uvc-fix-async-v5-1-6658c1fe312b@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: uvcvideo: Two +1 fixes for async controls | expand |
Hi, On 2-Dec-24 3:24 PM, Ricardo Ribalda wrote: > Now we keep a reference to the active fh for any call to uvc_ctrl_set, > regardless if it is an actual set or if it is a just a try or if the > device refused the operation. > > We should only keep the file handle if the device actually accepted > applying the operation. > > Cc: stable@vger.kernel.org > Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives") > Suggested-by: Hans de Goede <hdegoede@redhat.com> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> Thank you, nice patch, better then my original suggestion :) Reviewed-by: Hans de Goede <hdegoede@redhat.com> I'll let this sit on the list to give others a chance to reply and if there are no remarks I'll merge this next Monday. Regards, Hans > --- > drivers/media/usb/uvc/uvc_ctrl.c | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > index 4fe26e82e3d1..9a80a7d8e73a 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -1811,7 +1811,10 @@ int uvc_ctrl_begin(struct uvc_video_chain *chain) > } > > static int uvc_ctrl_commit_entity(struct uvc_device *dev, > - struct uvc_entity *entity, int rollback, struct uvc_control **err_ctrl) > + struct uvc_fh *handle, > + struct uvc_entity *entity, > + int rollback, > + struct uvc_control **err_ctrl) > { > struct uvc_control *ctrl; > unsigned int i; > @@ -1859,6 +1862,10 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev, > *err_ctrl = ctrl; > return ret; > } > + > + if (!rollback && handle && > + ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) > + ctrl->handle = handle; > } > > return 0; > @@ -1895,8 +1902,8 @@ int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback, > > /* Find the control. */ > list_for_each_entry(entity, &chain->entities, chain) { > - ret = uvc_ctrl_commit_entity(chain->dev, entity, rollback, > - &err_ctrl); > + ret = uvc_ctrl_commit_entity(chain->dev, handle, entity, > + rollback, &err_ctrl); > if (ret < 0) { > if (ctrls) > ctrls->error_idx = > @@ -2046,9 +2053,6 @@ int uvc_ctrl_set(struct uvc_fh *handle, > mapping->set(mapping, value, > uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); > > - if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) > - ctrl->handle = handle; > - > ctrl->dirty = 1; > ctrl->modified = 1; > return 0; > @@ -2377,7 +2381,7 @@ int uvc_ctrl_restore_values(struct uvc_device *dev) > ctrl->dirty = 1; > } > > - ret = uvc_ctrl_commit_entity(dev, entity, 0, NULL); > + ret = uvc_ctrl_commit_entity(dev, NULL, entity, 0, NULL); > if (ret < 0) > return ret; > } >
On Mon, Dec 02, 2024 at 02:24:35PM +0000, Ricardo Ribalda wrote: > Now we keep a reference to the active fh for any call to uvc_ctrl_set, > regardless if it is an actual set or if it is a just a try or if the > device refused the operation. > > We should only keep the file handle if the device actually accepted > applying the operation. > > Cc: stable@vger.kernel.org > Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives") > Suggested-by: Hans de Goede <hdegoede@redhat.com> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/usb/uvc/uvc_ctrl.c | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > index 4fe26e82e3d1..9a80a7d8e73a 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -1811,7 +1811,10 @@ int uvc_ctrl_begin(struct uvc_video_chain *chain) > } > > static int uvc_ctrl_commit_entity(struct uvc_device *dev, > - struct uvc_entity *entity, int rollback, struct uvc_control **err_ctrl) > + struct uvc_fh *handle, > + struct uvc_entity *entity, > + int rollback, > + struct uvc_control **err_ctrl) > { > struct uvc_control *ctrl; > unsigned int i; > @@ -1859,6 +1862,10 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev, > *err_ctrl = ctrl; > return ret; > } > + > + if (!rollback && handle && > + ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) > + ctrl->handle = handle; > } > > return 0; > @@ -1895,8 +1902,8 @@ int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback, > > /* Find the control. */ > list_for_each_entry(entity, &chain->entities, chain) { > - ret = uvc_ctrl_commit_entity(chain->dev, entity, rollback, > - &err_ctrl); > + ret = uvc_ctrl_commit_entity(chain->dev, handle, entity, > + rollback, &err_ctrl); > if (ret < 0) { > if (ctrls) > ctrls->error_idx = > @@ -2046,9 +2053,6 @@ int uvc_ctrl_set(struct uvc_fh *handle, > mapping->set(mapping, value, > uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); > > - if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) > - ctrl->handle = handle; > - > ctrl->dirty = 1; > ctrl->modified = 1; > return 0; > @@ -2377,7 +2381,7 @@ int uvc_ctrl_restore_values(struct uvc_device *dev) > ctrl->dirty = 1; > } > > - ret = uvc_ctrl_commit_entity(dev, entity, 0, NULL); > + ret = uvc_ctrl_commit_entity(dev, NULL, entity, 0, NULL); > if (ret < 0) > return ret; > }
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 4fe26e82e3d1..9a80a7d8e73a 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1811,7 +1811,10 @@ int uvc_ctrl_begin(struct uvc_video_chain *chain) } static int uvc_ctrl_commit_entity(struct uvc_device *dev, - struct uvc_entity *entity, int rollback, struct uvc_control **err_ctrl) + struct uvc_fh *handle, + struct uvc_entity *entity, + int rollback, + struct uvc_control **err_ctrl) { struct uvc_control *ctrl; unsigned int i; @@ -1859,6 +1862,10 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev, *err_ctrl = ctrl; return ret; } + + if (!rollback && handle && + ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) + ctrl->handle = handle; } return 0; @@ -1895,8 +1902,8 @@ int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback, /* Find the control. */ list_for_each_entry(entity, &chain->entities, chain) { - ret = uvc_ctrl_commit_entity(chain->dev, entity, rollback, - &err_ctrl); + ret = uvc_ctrl_commit_entity(chain->dev, handle, entity, + rollback, &err_ctrl); if (ret < 0) { if (ctrls) ctrls->error_idx = @@ -2046,9 +2053,6 @@ int uvc_ctrl_set(struct uvc_fh *handle, mapping->set(mapping, value, uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); - if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) - ctrl->handle = handle; - ctrl->dirty = 1; ctrl->modified = 1; return 0; @@ -2377,7 +2381,7 @@ int uvc_ctrl_restore_values(struct uvc_device *dev) ctrl->dirty = 1; } - ret = uvc_ctrl_commit_entity(dev, entity, 0, NULL); + ret = uvc_ctrl_commit_entity(dev, NULL, entity, 0, NULL); if (ret < 0) return ret; }
Now we keep a reference to the active fh for any call to uvc_ctrl_set, regardless if it is an actual set or if it is a just a try or if the device refused the operation. We should only keep the file handle if the device actually accepted applying the operation. Cc: stable@vger.kernel.org Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives") Suggested-by: Hans de Goede <hdegoede@redhat.com> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/media/usb/uvc/uvc_ctrl.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)