linux-kernel - RE: [PATCH v7 01/10] usb: gadget: udc: Add timer support for usb requests

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <Pine.LNX.4.44L0.1812071158570.1560-100000@iolanthe.rowland.org>
Date:   Fri, 7 Dec 2018 12:09:32 -0500 (EST)
From:   Alan Stern <stern@...land.harvard.edu>
To:     Felipe Balbi <balbi@...nel.org>
cc:     Anurag Kumar Vulisha <anuragku@...inx.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Shuah Khan <shuah@...nel.org>, Johan Hovold <johan@...nel.org>,
        Jaejoong Kim <climbbb.kim@...il.com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Roger Quadros <rogerq@...com>,
        Manu Gautam <mgautam@...eaurora.org>,
        "martin.petersen@...cle.com" <martin.petersen@...cle.com>,
        Bart Van Assche <bvanassche@....org>,
        Mike Christie <mchristi@...hat.com>,
        Matthew Wilcox <willy@...radead.org>,
        Colin Ian King <colin.king@...onical.com>,
        "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 v7 01/10] usb: gadget: udc: Add timer support for usb
 requests

On Fri, 7 Dec 2018, Felipe Balbi wrote:

> 
> hi,
> 
> Anurag Kumar Vulisha <anuragku@...inx.com> writes:
> >>Does the data book suggest a value for the timeout?
> >>
> >
> > No, the databook doesn't mention about the timeout value
> >
> >>> >At this point, it seems that the generic approach will be messier than having every
> >>> >controller driver implement its own fix.  At least, that's how it appears to me.
> 
> Why, if the UDC implementation will, anyway, be a timer?

It's mostly a question of what happens when the timer expires.  (After
all, starting a timer is just as easy to do in a UDC driver as it is in
the core.)  When the timer expires, what can the core do?

Don't say it can cancel the offending request and resubmit it.  That
leads to the sort of trouble we discussed earlier in this thread.  In
particular, we don't want the class driver's completion routine to be
called when the cancel occurs.  Furthermore, this leads to a race:  
Suppose the class driver decides to cancel the request at the same time
as the core does a cancel and resubmit.  Then the class driver's cancel
could get lost and the request would remain on the UDC's queue.

What you really want to do is issue the appropriate stop and restart
commands to the hardware, while leaving the request logically active
the entire time.  The UDC core can't do this, but a UDC driver can.

> >>(Especially if dwc3 is the only driver affected.)
> >
> > As discussed above, the issue may happen with other gadgets too. As I got divide opinions
> > on this implementation and both the implementations looks fine to me, I am little confused
> > on which should be implemented.
> >
> > @Felipe: Do you agree with Alan's implementation? Please let us know your suggestion
> > on this.
> 
> I still think a generic timer is a better solution since it has other uses.

Putting a struct timer into struct usb_request is okay with me, but I 
wouldn't go any farther than that.

> >>Since the purpose of the timeout is to detect a deadlock caused by a
> >>hardware bug, I suggest a fixed and relatively short timeout value such
> >>as one second.  Cancelling and requeuing a few requests at 1-second
> >>intervals shouldn't add very much overhead.
> 
> I wouldn't call this a HW bug though. This is just how the UDC
> behaves. There are N streams and host can move data in any stream at any
> time. This means that host & gadget _can_ disagree on what stream to
> start next.

But the USB 3 spec says what should happen when the host and gadget 
disagree in this way, doesn't it?  And it doesn't say that they should 
deadlock.  :-)  Or have I misread the spec?

> One way to avoid this would be to never pre-start any streams and always
> rely on XferNotReady, but that would mean greatly reduced throughput for
> streams.

It would be good if there was some way to actively detect the problem 
instead of passively waiting for a timer to expire.

Alan Stern