Hi Michael, (CC'ing Dan) Thank you for the patch. On Sun, Apr 02, 2023 at 10:01:22PM +0200, Michael Grzeschik wrote: > Pending requests in the gadget hardware get dequeued and returned with > ECONNRESET when the available endpoint is not available anymore. This > can be caused by an unplugged cable or the decision to shutdown the > stream, e.g. by switching the alt setting. > > In both cases the returned completion handler is marking the gadget > with UVC_QUEUE_DISCONNECTED by calling uvcg_queue_cancel. > > Since in userspace applications there might be two threads, one for the > bufferqueueing and one to handle the uvc events. It is likely that the > bufferqueueing thread did not receive the UVC_EVENT_STREAMOFF coming > from the alt_setting change early enough and still tries to queue a > buffer into the already disconnected marked device. Does this require two threads in userspace, or can it also happen in a single-threaded application ? It seems like a typical race condition between a userspace ioctl and a kernel-generated event. > This leads buf_prepare to return ENODEV, which usually makes the > userspace application quit. > > To fix the soft-shutdown case this patch is marking the alt setting > change before disabling the endpoint. This way the still completing > requests on the disabled endpoint can call uvcg_queue_cancel without > marking the device disconnected. > > Signed-off-by: Michael Grzeschik <m.grzeschik@xxxxxxxxxxxxxx> > --- > Hi Laurent! > > We are running into this issue in gstreamer when the host is stopping > the stream. In fact I am unsure if this is not also an issue when the > real unplug will appear. Isn't this something that should be fixed in userspace (possibly in addition to a kernel change to make userspace's life easier) ? Userspace should be able to gracefully handle the device getting stopped. What GStreamer element are you using, and is it an issue with the GStreamer element, or with the application ? > Since the v4l2 device is available all the time, and the streamoff > callback is cleaning up all the pending buffers in uvc_video_enable(0), > also the ones that got queued in this short time window of: > > alt_setting(0) -> userspace event handling -> streamoff ioctl > > Would it not be also possible to just drop the whole > UVC_QUEUE_DISCONNECTED mechanism? Do you mean accepting the buffers queued by userspace, even if the driver knows (or can know) they will never be sent out ? > drivers/usb/gadget/function/f_uvc.c | 3 ++- > drivers/usb/gadget/function/uvc_video.c | 5 +++-- > 2 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c > index 5e919fb6583301..01ab8c07d85be9 100644 > --- a/drivers/usb/gadget/function/f_uvc.c > +++ b/drivers/usb/gadget/function/f_uvc.c > @@ -337,6 +337,8 @@ uvc_function_set_alt(struct usb_function *f, unsigned interface, unsigned alt) > if (uvc->state != UVC_STATE_STREAMING) > return 0; > > + uvc->state = UVC_STATE_CONNECTED; > + > if (uvc->video.ep) > usb_ep_disable(uvc->video.ep); > > @@ -344,7 +346,6 @@ uvc_function_set_alt(struct usb_function *f, unsigned interface, unsigned alt) > v4l2_event.type = UVC_EVENT_STREAMOFF; > v4l2_event_queue(&uvc->vdev, &v4l2_event); > > - uvc->state = UVC_STATE_CONNECTED; > return 0; > > case 1: > diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c > index dd1c6b2ca7c6f3..2f36fef3824f8e 100644 > --- a/drivers/usb/gadget/function/uvc_video.c > +++ b/drivers/usb/gadget/function/uvc_video.c > @@ -265,9 +265,10 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req) > queue->flags |= UVC_QUEUE_DROP_INCOMPLETE; > break; > > - case -ESHUTDOWN: /* disconnect from host. */ > + case -ESHUTDOWN: /* disconnect from host or streamoff pending */ > uvcg_dbg(&video->uvc->func, "VS request cancelled.\n"); > - uvcg_queue_cancel(queue, 1); > + uvcg_queue_cancel(queue, > + uvc->state != UVC_STATE_STREAMING ? 0 : 1); > break; > > default: -- Regards, Laurent Pinchart
