| Message ID | 20230402200122.2919202-1-m.grzeschik@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] usb: gadget: uvc: sane shutdown on soft streamoff | expand |
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@pengutronix.de> > --- > 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:
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:
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. 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@pengutronix.de> --- 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. 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? Thanks, Michael drivers/usb/gadget/function/f_uvc.c | 3 ++- drivers/usb/gadget/function/uvc_video.c | 5 +++-- 2 files changed, 5 insertions(+), 3 deletions(-)