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@xxxxxxxxxx> 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@xxxxxxxxxxxx> >> >> 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@xxxxxxxxxxxxxxxxxxxxxxxxxx/ >> >> 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); >>> >> > >
