Re: [PATCH v2 2/4] media: uvcvideo: Do not set an async control owned by other fh

[Ricardo Ribalda <ribalda@chromium.org>]

On Fri, 29 Nov 2024 at 23:03, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Fri, Nov 29, 2024 at 07:47:31PM +0100, Ricardo Ribalda wrote:
> > Before we all go on a well deserved weekend, let me recap what we
> > know. If I did not get something correctly, let me know.
> >
> > 1) Well behaved devices do not allow to set or get an incomplete async
> > control. They will stall instead (ref: Figure 2-21 in UVC 1.5 )
> > 2) Both Laurent and Ricardo consider that there is a big chance that
> > some camera modules do not implement this properly. (ref: years of
> > crying over broken module firmware :) )
> >
> > 3) ctrl->handle is designed to point to the fh that originated the
> > control. So the logic can decide if the originator needs to be
> > notified or not. (ref: uvc_ctrl_send_event() )
> > 4) Right now we replace the originator in ctrl->handle for unfinished
> > async controls.  (ref:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/usb/uvc/uvc_ctrl.c#n2050)
> >
> > My interpretation is that:
> > A) We need to change 4). We shall not change the originator of
> > unfinished ctrl->handle.
> > B) Well behaved cameras do not need the patch "Do not set an async
> > control owned by another fh"
> > C) For badly behaved cameras, it is fine if we slightly break the
> > v4l2-compliance in corner cases, if we do not break any internal data
> > structure.
>
> The fact that some devices may not implement the documented behaviour
> correctly may not be a problem. Well-behaved devices will stall, which
> means we shouldn't query the device while as async update is in
> progress. Badly-behaved devices, whatever they do when queried, should
> not cause any issue if we don't query them.

I thought we could detect the stall and return safely. Isn't that the case?
Why we have not seen issues with this?


>
> We should not send GET_CUR and SET_CUR requests to the device while an
> async update is in progress, and use cached values instead. When we
> receive the async update event, we should clear the cache. This will be
> the same for both well-behaved and badly-behaved devices, so we can
> expose the same behaviour towards userspace.

seting ctrl->loaded = 0 when we get an event sounds like a good idea
and something we can implement right away.
If I have to resend the set I will add it to the end.

>
> We possibly also need some kind of timeout mechanism to cope with the
> async update event not being delivered by the device.

This is the part that worries me the most:
- timeouts make the code fragile
- What is a good value for timeout? 1 second, 30, 300? I do not think
that we can find a value.


>
> Regarding the userspace behaviour during an auto-update, we have
> multiple options:
>
> For control get,
>
> - We can return -EBUSY
> - We can return the old value from the cache
> - We can return the new value fromt he cache
>
> Returning -EBUSY would be simpler to implement.
Not only easy, I think it is the most correct,

>
> I don't think the behaviour should depend on whether the control is read
> on the file handle that initiated the async operation or on a different
> file handle.
>
> For control set, I don't think we can do much else than returning
> -EBUSY, regardless of which file handle the control is set on.

ACK.

>
> > I will send a new version with my interpretation.
> >
> > Thanks for a great discussion
>
> --
> Regards,
>
> Laurent Pinchart

Looking with some perspective... I believe that we should look into
the "userspace behaviour for auto controls" in a different patchset.
It is slightly unrelated to this discussion.


-- 
Ricardo Ribalda