Re: [PATCH V6 01/10] usb: gadget: udc: Add timer for stream capable endpoints

[Felipe Balbi]

Hi,

Anurag Kumar Vulisha <anurag.kumar.vulisha@xxxxxxxxxx> writes:
> When bulk streams are enabled for an endpoint, there can
> be a condition where the gadget controller waits for the
> host to issue prime transaction and the host controller
> waits for the gadget to issue ERDY. This condition could
> create a deadlock. To avoid such potential deadlocks, a
> timer is started after queuing any request for the stream
> capable endpoint in usb_ep_queue(). The gadget driver is
> expected to stop the timer if a valid stream event is found.
> If no stream event is found, the timer expires after the
> STREAM_TIMEOUT_MS value and a callback function registered
> by gadget driver to endpoint ops->stream_timeout API would be
> called, so that the gadget driver can handle this situation.
> This kind of behaviour is observed in dwc3 controller and
> expected to be generic issue with other controllers supporting
> bulk streams also.
>
> Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xxxxxxxxxx>
> ---
>  Changes in v6:
> 	1. This patch is newly added in this series to add timer into udc/core.c
> ---
>  drivers/usb/gadget/udc/core.c | 71 ++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/usb/gadget.h    | 12 ++++++++
>  2 files changed, 82 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index af88b48..41cc23b 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -52,6 +52,24 @@ static int udc_bind_to_driver(struct usb_udc *udc,
>  /* ------------------------------------------------------------------------- */
>  
>  /**
> + * usb_ep_stream_timeout - callback function for endpoint stream timeout timer
> + * @arg: pointer to struct timer_list
> + *
> + * This function gets called only when bulk streams are enabled in the endpoint
> + * and only after ep->stream_timeout_timer has expired. The timer gets expired
> + * only when the gadget controller failed to find a valid stream event for this
> + * endpoint. On timer expiry, this function calls the endpoint-specific timeout
> + * handler registered to endpoint ops->stream_timeout API.
> + */
> +static void usb_ep_stream_timeout(struct timer_list *arg)
> +{
> +	struct usb_ep *ep = from_timer(ep, arg, stream_timeout_timer);
> +
> +	if (ep->stream_capable && ep->ops->stream_timeout)

why is the timer only for stream endpoints? What if we want to run this
on non-stream capable eps?

> +		ep->ops->stream_timeout(ep);

you don't ned an extra operation here, ep_dequeue should be enough.

> @@ -102,6 +132,10 @@ int usb_ep_enable(struct usb_ep *ep)
>  	if (ret)
>  		goto out;
>  
> +	if (ep->stream_capable)
> +		timer_setup(&ep->stream_timeout_timer,
> +			    usb_ep_stream_timeout, 0);

the timer should be per-request, not per-endpoint. Otherwise in case of
multiple requests being queued, you can't give them different timeouts

> @@ -269,6 +321,13 @@ int usb_ep_queue(struct usb_ep *ep,
>  
>  	ret = ep->ops->queue(ep, req, gfp_flags);
>  
> +	if (ep->stream_capable) {
> +		ep->stream_timeout_timer.expires = jiffies +
> +				msecs_to_jiffies(STREAM_TIMEOUT_MS);

timeout value should be passed by the gadget driver. Add a new
usb_ep_queue_timeout() that takes the new argument. Rename the current
usb_ep_queue() to static int __usb_ep_queue() with an extra argument for
timeout and introduce usb_ep_queue() without the argument, calling
__usb_ep_queue() passing timeout as 0.

> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index e5cd84a..2ebaef0 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -144,6 +144,7 @@ struct usb_ep_ops {
>  
>  	int (*fifo_status) (struct usb_ep *ep);
>  	void (*fifo_flush) (struct usb_ep *ep);
> +	void (*stream_timeout) (struct usb_ep *ep);

why?

-- 
balbi

Attachment: signature.asc
Description: PGP signature