netdev - RE: [PATCH] usb: xhci: Link TRB must not occur with a USB payload burst.

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]

Date:	Wed, 13 Nov 2013 11:27:21 -0500 (EST)
From:	Alan Stern <stern@...land.harvard.edu>
To:	David Laight <David.Laight@...LAB.COM>
cc:	Sarah Sharp <sarah.a.sharp@...ux.intel.com>,
	<netdev@...r.kernel.org>, <linux-usb@...r.kernel.org>
Subject: RE: [PATCH] usb: xhci: Link TRB must not occur with a USB payload
 burst.

On Wed, 13 Nov 2013, David Laight wrote:

> > > that doesn't matter; we don't get an interrupt when a ring segment is
> > > crossed.  Instead we set the interrupt-on-completion flag on the last
> > > TRB of the TD, not on any earlier fragment or link TRB.
> > 
> > That's because you don't worry about handling URBs which require too
> > many TRBs (i.e., more than are available).  You just add more ring
> > segments.  Instead, you could re-use segments on the fly.
> > 
> > For example, suppose you have only two ring segments and you get an URB
> > which requires enough TRBs to fill up four segments.  You could fill in
> > the first two segments worth, and get an interrupt when the controller
> > traverses the Link TRB between them.  At that point you store the third
> > set of TRBs in the first segment, which is now vacant.  Similarly, when
> > the second Link TRB is traversed, you fill in the fourth set of TRBs.
> 
> That isn't going to work.

Queuing URBs and processing them in pieces was _your_ suggestion.  I
merely repeated it to Sarah and filled in some detail.

> It might work for very long TD, but for very fragmented ones the interrupt
> rate would be stupid.

Not if the individual SG elements are reasonably large -- say at least 
512 bytes.  They have to be that big if they are to work with ehci-hcd, 
so why not also for xhci-hcd?

>  In any case you'd definitely need enough ring
> segments for the TRB that describe a single 'max packet size' block.

That would be two TRBs (for maxpacket = 1024), meaning you'd need at
least one ring segment.  That's not much of a restriction.

> > > Finally, it's interesting to note that the USB mass storage driver is
> > > using scatter gather lists just fine without the driver following the TD
> > > fragment rules.  Or at least no one has reported any issues.  I wonder
> > > why it works?
> > 
> > I'd guess this is because the hardware is actually a lot more flexible
> > than the "No Link TRBs in the middle of a TD fragment" rule.
> 
> With the hardware I have (Intel i7 Sandy bridge) Link TRB cannot
> be placed at arbitrary boundaries.
> I don't know whether the actual restriction is only to packet boundaries.

That's what I had in mind by "more flexible" above.

> I don't have a USB3 monitor and it would also require a more contrived
> test than I've been doing.

You wouldn't need a monitor to check it, but you would need some 
careful tests.

> > The whole idea of TD fragments makes no sense to begin with.  What
> > point is there in grouping packets into MaxBurst-sized collections?
> 
> It probably saved a few logic gates somewhere, either that or it is
> a bug in some hardware implementation that got documented in the spec
> instead of being fixed :-)

I can believe the guess about it reflecting a bug.  Particularly since 
it is so badly written.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html