Hi Dan, On Thu, Mar 24, 2022 at 03:23:29PM -0500, Dan Vacura wrote: > On Thu, Mar 24, 2022 at 04:43:51PM +0200, Laurent Pinchart wrote: > > On Fri, Mar 18, 2022 at 11:47:06AM -0500, Dan Vacura wrote: > > > During the uvcg_video_pump() process, if an error occurs and > > > uvcg_queue_cancel() is called, the buffer queue will be cleared out, but > > > the current marker (queue->buf_used) of the active buffer (no longer > > > active) is not reset. On the next iteration of uvcg_video_pump() the > > > stale buf_used count will be used and the logic of min((unsigned > > > int)len, buf->bytesused - queue->buf_used) may incorrectly calculate a > > > nbytes size, causing an invalid memory access. > > > > When uvcg_queue_cancel() is called, it will empty the queue->irqqueue. > > The next uvcg_video_pump() iteration should thus get a NULL buffer when > > calling uvcg_queue_head(), and shouldn't proceed to calling > > video->encode(). Is the issue that the application queues further > > buffers after cancellation, which puts a new buffer in the irqqueue ? > > Yes, that's exactly what's happening. The application has one thread > that is receiving camera frames and queuing them to the gadget driver, > the other thread is waiting for empty buffers to dequeue, generated via > uvcg_queue_next_buffer(), to send back for camera to fill. Is there a > requirement to serialize this logic? Not in userspace, any required serialization should be handled in the kernel. Unless I'm mistaken we already have a lock to handle concurrent QBUF and DQBUF calls, so it should be fine. > In addition, I can check for the > state of the buffers that have just been dequeued, but not sure what to > do if there's a failure. That's a good question, discussed below. > > I wonder if we need to expand the discussion here to what should be done > > if an error occurs in uvcg_video_pump(). We currently cancel the queue > > and drop all queued buffers, but don't prevent more buffers to be > > queued. Should we force the application to stop streaming in case of > > error, clean up and restart ? Or are usb_ep_queue() errors expected to > > happen from time to time, with graceful error recovery a required > > feature of the gadget driver ? > > Good question, this is out of my expertise, but I can comment about what > I see in our current setup, qcom snapdragon chipsets with the dwc3 > drivers on 5.10-android. Depending on the host I connect to, the -18 > errors in uvc_video_complete() can occur during normal use, sometimes > several times in a span of a few seconds. I wasn't seeing usb_eq_queue() > errors. When the error occurs the device application doesn't do anything > special and continues queuing subsequent buffers. Sometimes there is > visible corruption in the received data, but the streaming gracefully > recovers, mpeg or yuv. So it looks like we need to handle this condition gracefully. This means that we shouldn't cancel the queue, but continue with the next transfer, doesn't it ? > > > [80802.185460][ T315] configfs-gadget gadget: uvc: VS request completed > > > with status -18. > > > [80802.185519][ T315] configfs-gadget gadget: uvc: VS request completed > > > with status -18. > > > ... > > > uvcg_queue_cancel() is called and the queue is cleared out, but the > > > marker queue->buf_used is not reset. > > > ... > > > [80802.262328][ T8682] Unable to handle kernel paging request at virtual > > > address ffffffc03af9f000 > > > ... > > > ... > > > [80802.263138][ T8682] Call trace: > > > [80802.263146][ T8682] __memcpy+0x12c/0x180 > > > [80802.263155][ T8682] uvcg_video_pump+0xcc/0x1e0 > > > [80802.263165][ T8682] process_one_work+0x2cc/0x568 > > > [80802.263173][ T8682] worker_thread+0x28c/0x518 > > > [80802.263181][ T8682] kthread+0x160/0x170 > > > [80802.263188][ T8682] ret_from_fork+0x10/0x18 > > > [80802.263198][ T8682] Code: a8c12829 a88130cb a8c130 > > > > > > Fixes: d692522577c0 ("usb: gadget/uvc: Port UVC webcam gadget to use videobuf2 framework") > > > Signed-off-by: Dan Vacura <w36195@xxxxxxxxxxxx> > > > > > > --- > > > Changes in v2: > > > - Add Fixes tag > > > > > > drivers/usb/gadget/function/uvc_queue.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c > > > index d852ac9e47e7..2cda982f3765 100644 > > > --- a/drivers/usb/gadget/function/uvc_queue.c > > > +++ b/drivers/usb/gadget/function/uvc_queue.c > > > @@ -264,6 +264,8 @@ void uvcg_queue_cancel(struct uvc_video_queue *queue, int disconnect) > > > buf->state = UVC_BUF_STATE_ERROR; > > > vb2_buffer_done(&buf->buf.vb2_buf, VB2_BUF_STATE_ERROR); > > > } > > > + queue->buf_used = 0; > > > + > > > /* This must be protected by the irqlock spinlock to avoid race > > > * conditions between uvc_queue_buffer and the disconnection event that > > > * could result in an interruptible wait in uvc_dequeue_buffer. Do not -- Regards, Laurent Pinchart