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

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

Hi Felipe,

>-----Original Message-----
>From: Felipe Balbi [mailto:balbi@kernel.org]
>Sent: Thursday, November 29, 2018 6:22 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>
>Subject: RE: [PATCH V6 01/10] usb: gadget: udc: Add timer for stream capable
>endpoints
>
>
>Hi,
>
>Anurag Kumar Vulisha <anuragku@xilinx.com> writes:
>>>> 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.
>
>you're adding support for transfer timeouts, which some gadgets may want
>regardless of endpoint type. One use is to correct cases of streams
>going out of sync.
>

Thanks for making me understand, will implement your suggestions and resend
the patches soon.

Best Regards,
Anurag Kumar Vulisha
 
>>>> +		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.
>
>not anymore. There's, now, a requirement that ->dequeue can be called
>from any context.
>
>> 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.
>
>no problems
>
>> 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.
>
>call ep_dequeue()
>
>--
>balbi