Re: [PATCH v3] usb: gadget: uvc: Fix crash when encoding data for usb request

[Dan Vacura <w36195@motorola.com>]

Hi Laurent,

Thanks for the input.

On Tue, Apr 19, 2022 at 11:46:37PM +0300, Laurent Pinchart wrote:
> 
> This indeed fixes an issue, so I think we can merge the patch, but I
> also believe we need further improvements on top (of course if you would
> like to improve the implementation in a v4, I won't complain :-))

It looks like Greg has already accepted the change and it's in
linux-next. We can discuss here how to better handle these -EXDEV errors
for future improvements, as it seems like it's been an issue in the past
as well:
https://www.mail-archive.com/linux-usb@vger.kernel.org/msg105615.html

> 
> As replied in v2 (sorry for the late reply), it seems that this error
> can occur under normal conditions. This means we shouldn't cancel the
> queue, at least when the error is intermitent (if all URBs fail that's
> another story).

My impression was that canceling the queue was still necessary as we may
be in progress for the current frame. Perhaps we don't need to flush all
the frames from the queue, but at a minimum we need to reset the
buf_used value.

> 
> 
> We likely need to differentiate between -EXDEV and other errors in
> uvc_video_complete(), as I'd like to be conservative and cancel the
> queue for unknown errors. We also need to improve the queue cancellation
> implementation so that userspace gets an error when queuing further
> buffers.

We already feedback to userspace the error, via the state of
vb2_buffer_done(). When userspace dequeues the buffer it can check if
v4l2_buffer.flags has V4L2_BUF_FLAG_ERROR to see if things failed, then
decide what to do like re-queue that frame. However, this appears to not
always occur since I believe the pump thread is independent of the
uvc_video_complete() callback. As a result, the complete callback of the
failed URB may be associated with a buffer that was already released
back to the userspace client. In this case, I don't know if there's
anything to be done, since a new buffer and subsequent URBs might
already be queued up. You suggested an error on a subsequent buffer
queue, but I don't know how helpful that'd be at this point, perhaps in
the scenario that all URBs are failing?

> 
> -- 
> Regards,
> 
> Laurent Pinchart

Appreciate the feedback,

Dan