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;
 

-- 
2.47.0.338.g60cca15819-goog

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.

-- 
Regards,

Laurent Pinchart

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



-- 
Ricardo Ribalda