Hi Felipe, >-----Original Message----- >From: Felipe Balbi [mailto:balbi@xxxxxxxxxx] >Sent: Thursday, November 29, 2018 6:22 PM >To: Anurag Kumar Vulisha <anuragku@xxxxxxxxxx>; Greg Kroah-Hartman ><gregkh@xxxxxxxxxxxxxxxxxxx>; Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>; Johan >Hovold <johan@xxxxxxxxxx>; Jaejoong Kim <climbbb.kim@xxxxxxxxx>; Benjamin >Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx>; Roger Quadros <rogerq@xxxxxx> >Cc: linux-usb@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; >v.anuragkumar@xxxxxxxxx; Thinh Nguyen <thinhn@xxxxxxxxxxxx>; Tejas Joglekar ><tejas.joglekar@xxxxxxxxxxxx>; Ajay Yugalkishore Pandey <APANDEY@xxxxxxxxxx> >Subject: RE: [PATCH V6 01/10] usb: gadget: udc: Add timer for stream capable >endpoints > > >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. > 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
