| Message ID | 20241202-uvc-fix-async-v5-5-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: > Asynchronous controls trigger an event when they have completed their > operation. > > This can make that the control cached value does not match the value in > the device. > > Let's flush the cache to be on the safe side. > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/usb/uvc/uvc_ctrl.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > index e90bf2aeb5e4..75d534072f50 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -1672,6 +1672,9 @@ bool uvc_ctrl_status_event_async(struct urb *urb, struct uvc_video_chain *chain, > struct uvc_device *dev = chain->dev; > struct uvc_ctrl_work *w = &dev->async_ctrl; > > + /* Flush the control cache, the data might have changed. */ > + ctrl->loaded = 0; > + > if (list_empty(&ctrl->info.mappings)) > return false; > > Thank you for the patch. I'm not familiar enough with UVC yet to really have an opinion on this one, so I'll defer reviewing this one to Laurent. Regards, Hans
On Mon, Dec 02, 2024 at 03:45:57PM +0100, Hans de Goede wrote: > Hi, > > On 2-Dec-24 3:24 PM, Ricardo Ribalda wrote: > > Asynchronous controls trigger an event when they have completed their > > operation. > > > > This can make that the control cached value does not match the value in > > the device. > > > > Let's flush the cache to be on the safe side. > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > --- > > drivers/media/usb/uvc/uvc_ctrl.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > > index e90bf2aeb5e4..75d534072f50 100644 > > --- a/drivers/media/usb/uvc/uvc_ctrl.c > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > > @@ -1672,6 +1672,9 @@ bool uvc_ctrl_status_event_async(struct urb *urb, struct uvc_video_chain *chain, > > struct uvc_device *dev = chain->dev; > > struct uvc_ctrl_work *w = &dev->async_ctrl; > > > > + /* Flush the control cache, the data might have changed. */ > > + ctrl->loaded = 0; > > + > > if (list_empty(&ctrl->info.mappings)) > > return false; > > > > Thank you for the patch. > > I'm not familiar enough with UVC yet to really have an opinion on this one, > so I'll defer reviewing this one to Laurent. Conceptually this change looks fine, but the commit message needs to explain why this is safe to do without protecting ctrl->loaded with a lock.
On Tue, 3 Dec 2024 at 21:38, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Mon, Dec 02, 2024 at 03:45:57PM +0100, Hans de Goede wrote: > > Hi, > > > > On 2-Dec-24 3:24 PM, Ricardo Ribalda wrote: > > > Asynchronous controls trigger an event when they have completed their > > > operation. > > > > > > This can make that the control cached value does not match the value in > > > the device. > > > > > > Let's flush the cache to be on the safe side. > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > --- > > > drivers/media/usb/uvc/uvc_ctrl.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > > > index e90bf2aeb5e4..75d534072f50 100644 > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > > > @@ -1672,6 +1672,9 @@ bool uvc_ctrl_status_event_async(struct urb *urb, struct uvc_video_chain *chain, > > > struct uvc_device *dev = chain->dev; > > > struct uvc_ctrl_work *w = &dev->async_ctrl; > > > > > > + /* Flush the control cache, the data might have changed. */ > > > + ctrl->loaded = 0; > > > + > > > if (list_empty(&ctrl->info.mappings)) > > > return false; > > > > > > > Thank you for the patch. > > > > I'm not familiar enough with UVC yet to really have an opinion on this one, > > so I'll defer reviewing this one to Laurent. > > Conceptually this change looks fine, but the commit message needs to > explain why this is safe to do without protecting ctrl->loaded with a > lock. Just realised that loaded is a bit field... and we shall not be creative with lockless writes with those. I am going to move it to uvc_ctrl_status_event(). It will not work with controls without mappings... but I guess we do not care about the cache for those. Feel free to land the whole series without this patch if we need further discussion or changes. > > -- > Regards, > > Laurent Pinchart
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index e90bf2aeb5e4..75d534072f50 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1672,6 +1672,9 @@ bool uvc_ctrl_status_event_async(struct urb *urb, struct uvc_video_chain *chain, struct uvc_device *dev = chain->dev; struct uvc_ctrl_work *w = &dev->async_ctrl; + /* Flush the control cache, the data might have changed. */ + ctrl->loaded = 0; + if (list_empty(&ctrl->info.mappings)) return false;
Asynchronous controls trigger an event when they have completed their operation. This can make that the control cached value does not match the value in the device. Let's flush the cache to be on the safe side. Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/media/usb/uvc/uvc_ctrl.c | 3 +++ 1 file changed, 3 insertions(+)