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

[Anurag Kumar Vulisha <anuragku@xilinx.com>]

Hi Felipe,

Thanks a lot for spending your time in reviewing this patch. Please find
my comments inline

>-----Original Message-----
>From: Felipe Balbi [mailto:balbi@kernel.org]
>Sent: Wednesday, November 14, 2018 7:28 PM
>To: Anurag Kumar Vulisha <anuragku@xilinx.com>; Greg Kroah-Hartman
><gregkh@linuxfoundation.org>; Alan Stern <stern@rowland.harvard.edu>; Johan
>Hovold <johan@kernel.org>; Jaejoong Kim <climbbb.kim@gmail.com>; Benjamin
>Herrenschmidt <benh@kernel.crashing.org>; Roger Quadros <rogerq@ti.com>
>Cc: linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org;
>v.anuragkumar@gmail.com; Thinh Nguyen <thinhn@synopsys.com>; Tejas Joglekar
><tejas.joglekar@synopsys.com>; Ajay Yugalkishore Pandey <APANDEY@xilinx.com>;
>Anurag Kumar Vulisha <anuragku@xilinx.com>
>Subject: Re: [PATCH V6 01/10] usb: gadget: udc: Add timer for stream capable
>endpoints
>
>
>Hi,
>
>Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com> 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@xilinx.com>
>> ---
>>  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?
>

I have seen this issue only with stream capable eps between PRIME & EPRDY. But this issue
can potentially occur with non-stream endpoints also. Will remove this stream capable checks
in next series of patch.

>> +		ep->ops->stream_timeout(ep);
>
>you don't ned an extra operation here, ep_dequeue should be enough.
>

I think issuing ep_dequeue() would be more trickier than calling ep->ops->stream_timeout().
This is because, every gadget driver has their own way of handling ep_dequeue. Some
drivers (like dwc3) may sleep for an event (wait_event_lock_irq) in the ep_dequeue routine.
Since we are calling ep_dequeue from timer timeout callback which is in softirq context,
sleeping or waiting for an event would hang the system. Also adding ep->ops->stream_timeout()
would make the gadget drivers handle the issue in better way based on their implementation.
Another advantage is the drivers which doesn't have this timeout issue doesn't have to register the
timeout handler and can avoid the logic of deleting the timer for every request. So, I think
ep->ops->stream_timeout() would be better than having a generic handler which does
ep_dequeue() & ep_queue(). Please provide your suggestion on this implementation.

>> @@ -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

I agree with you. Will correct this in next series of patch.

>
>> @@ -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.
>

Thanks for correcting and providing the steps for implementing. Will modify as
suggested in next series of patch.

>> 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?
>

As discussed above, the common timeout handler with ep_dequeue() and
ep_queue() may not work for all the gadget drivers, adding the stream_timeout()
in ep->ops would enable the drivers to implement their own handler for
handling the stream timeout issue.

Thanks,
Anurag Kumar Vulisha
>--
>balbi