| Message ID | 20241203-uvc-fix-async-v6-5-26c867231118@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: uvcvideo: Two +1 fixes for async controls | expand |
Hi Ricardo, On 3-Dec-24 10:20 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> Thank you for your patch. It seems that you have missed Laurent's reply asking to improve the commit message: "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." https://lore.kernel.org/linux-media/20241203203748.GD5196@pendragon.ideasonboard.com/ Or maybe the posting of this v6 and that reply have crossed each other. Either way please post a new version addressing this comment. Thanks & Regards, Hans > --- > 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 3dc9b7a49f64..db29e0e8bfd4 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -1622,6 +1622,9 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain, > > mutex_lock(&chain->ctrl_mutex); > > + /* Flush the control cache, the data might have changed. */ > + ctrl->loaded = 0; > + > handle = ctrl->handle; > if (handle) > uvc_ctrl_set_handle(handle, ctrl, NULL); >
Hi Hans On Mon, 9 Dec 2024 at 12:03, Hans de Goede <hdegoede@redhat.com> wrote: > > Hi Ricardo, > > On 3-Dec-24 10:20 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> > > Thank you for your patch. > > It seems that you have missed Laurent's reply asking to improve the commit message: > > "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." > > https://lore.kernel.org/linux-media/20241203203748.GD5196@pendragon.ideasonboard.com/ > > Or maybe the posting of this v6 and that reply have crossed each other. In this v6 I moved loaded=0 from uvc_ctrl_status_event_async() to uvc_ctrl_status_event() Now setting loaded=0 is just after mutex_lock(&chain->ctrl_mutex); Do we need a new version? > > Either way please post a new version addressing this comment. > > Thanks & Regards, > > Hans > > > > > --- > > 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 3dc9b7a49f64..db29e0e8bfd4 100644 > > --- a/drivers/media/usb/uvc/uvc_ctrl.c > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > > @@ -1622,6 +1622,9 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain, > > > > mutex_lock(&chain->ctrl_mutex); > > > > + /* Flush the control cache, the data might have changed. */ > > + ctrl->loaded = 0; > > + > > handle = ctrl->handle; > > if (handle) > > uvc_ctrl_set_handle(handle, ctrl, NULL); > > >
Hi, On 9-Dec-24 12:31 PM, Ricardo Ribalda wrote: > Hi Hans > > On Mon, 9 Dec 2024 at 12:03, Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi Ricardo, >> >> On 3-Dec-24 10:20 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> >> >> Thank you for your patch. >> >> It seems that you have missed Laurent's reply asking to improve the commit message: >> >> "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." >> >> https://lore.kernel.org/linux-media/20241203203748.GD5196@pendragon.ideasonboard.com/ >> >> Or maybe the posting of this v6 and that reply have crossed each other. > > In this v6 I moved loaded=0 from uvc_ctrl_status_event_async() to > uvc_ctrl_status_event() Ah I missed that you moved it, my bad. > Now setting loaded=0 is just after mutex_lock(&chain->ctrl_mutex); > > Do we need a new version? Since the setting is now clearly done under the lock the new version seems good to me as is. So I have now merged this into: https://gitlab.freedesktop.org/linux-media/users/uvc/-/commits/next/ Regards, Hans > >> >> Either way please post a new version addressing this comment. >> >> Thanks & Regards, >> >> Hans >> >> >> >>> --- >>> 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 3dc9b7a49f64..db29e0e8bfd4 100644 >>> --- a/drivers/media/usb/uvc/uvc_ctrl.c >>> +++ b/drivers/media/usb/uvc/uvc_ctrl.c >>> @@ -1622,6 +1622,9 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain, >>> >>> mutex_lock(&chain->ctrl_mutex); >>> >>> + /* Flush the control cache, the data might have changed. */ >>> + ctrl->loaded = 0; >>> + >>> handle = ctrl->handle; >>> if (handle) >>> uvc_ctrl_set_handle(handle, ctrl, NULL); >>> >> > >
Hi Ricardo, Thank you for the patch. On Tue, Dec 03, 2024 at 09:20:12PM +0000, 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 3dc9b7a49f64..db29e0e8bfd4 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -1622,6 +1622,9 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain, > > mutex_lock(&chain->ctrl_mutex); > > + /* Flush the control cache, the data might have changed. */ > + ctrl->loaded = 0; > + That's better, covered by ctrl_mutex. There are however quite a few code paths through which ctrl->loaded is accessed without holding the mutex :-( Not a candidate for this series, but this should eventually be fixed. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > handle = ctrl->handle; > if (handle) > uvc_ctrl_set_handle(handle, ctrl, NULL); >
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 3dc9b7a49f64..db29e0e8bfd4 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1622,6 +1622,9 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain, mutex_lock(&chain->ctrl_mutex); + /* Flush the control cache, the data might have changed. */ + ctrl->loaded = 0; + handle = ctrl->handle; if (handle) uvc_ctrl_set_handle(handle, ctrl, NULL);
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(+)