Re: [RFC] usb: gadget: uvc: sane shutdown on soft streamoff

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux