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

[Felipe Balbi]

Hi,

Anurag Kumar Vulisha <anuragku@xxxxxxxxxx> 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.

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

Attachment: signature.asc
Description: PGP signature