Hi Felipe, >-----Original Message----- >From: Felipe Balbi [mailto:ba...@kernel.org] >Sent: Thursday, November 29, 2018 6:22 PM >To: Anurag Kumar Vulisha <anura...@xilinx.com>; Greg Kroah-Hartman ><gre...@linuxfoundation.org>; Alan Stern <st...@rowland.harvard.edu>; Johan >Hovold <jo...@kernel.org>; Jaejoong Kim <climbbb....@gmail.com>; Benjamin >Herrenschmidt <b...@kernel.crashing.org>; Roger Quadros <rog...@ti.com> >Cc: linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; >v.anuragku...@gmail.com; Thinh Nguyen <thi...@synopsys.com>; Tejas Joglekar ><tejas.jogle...@synopsys.com>; Ajay Yugalkishore Pandey <apan...@xilinx.com> >Subject: RE: [PATCH V6 01/10] usb: gadget: udc: Add timer for stream capable >endpoints > > >Hi, > >Anurag Kumar Vulisha <anura...@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
