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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <BL0PR02MB563301AF77DA0EDE524F0774A7D20@BL0PR02MB5633.namprd02.prod.outlook.com>
Date:   Thu, 29 Nov 2018 15:51:47 +0000
From:   Anurag Kumar Vulisha <anuragku@...inx.com>
To:     Felipe Balbi <balbi@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Alan Stern <stern@...land.harvard.edu>,
        Johan Hovold <johan@...nel.org>,
        Jaejoong Kim <climbbb.kim@...il.com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Roger Quadros <rogerq@...com>
CC:     "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "v.anuragkumar@...il.com" <v.anuragkumar@...il.com>,
        Thinh Nguyen <thinhn@...opsys.com>,
        Tejas Joglekar <tejas.joglekar@...opsys.com>,
        Ajay Yugalkishore Pandey <APANDEY@...inx.com>
Subject: RE: [PATCH V6 01/10] usb: gadget: udc: Add timer for stream capable
 endpoints

Hi Felipe,

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