Message-ID: <20140130211816.GB3787@xanatos>
Date:	Thu, 30 Jan 2014 13:18:16 -0800
From:	Sarah Sharp <sarah.a.sharp@...ux.intel.com>
To:	David Laight <David.Laight@...LAB.COM>
Cc:	"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	David Miller <davem@...emloft.net>,
	Dan Williams <dan.j.williams@...el.com>,
	"Nyman, Mathias" <mathias.nyman@...el.com>,
	Mark Lord <mlord@...ox.com>,
	Alan Stern <stern@...land.harvard.edu>,
	Freddy Xin <freddy@...x.com.tw>
Subject: Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned

On Thu, Jan 30, 2014 at 05:35:08PM +0100, Peter Stuge wrote:
> David Laight wrote:
> > > Where's the 8k coming from?
> > 
> > My memory, I meant 16k :-(
> 
> No problem. But what about that alignment? It seems that userspace
> needs to start caring about alignment with xhci, right?

We need to step back and reassess the larger picture here, instead of
trying to fire-fight all the regressions caused by the link TRB commit
(35773dac5f86 "usb: xhci: Link TRB must not occur within a USB payload
burst").

We shouldn't need to make userspace start to worry about alignment at
all.  libusb worked in the past, before the link TRB fix went in.  We
*cannot* break userspace USB drivers.  The breakage needs to be fixed in
the USB core or the xHCI driver.

Commit 35773dac5f86 was meant to be a short-term bandaid fix, but it's
already caused at least four different regressions.  Some we've fixed,
some have proposed solutions that David has sent.

The storage layer is getting borked because it submits scatter-gather
lists longer than what will fit on a segment, and now libusb has the
same issue.  One xHCI host controller stopped responding to commands,
and reverting the bandaid fix helped.  The implications of this change
just keep coming in, and I'm not comfortable wall-papering over the
issues.

On the flip side, it seems that the only devices that have been helped
by the bandaid fix patch are USB ethernet devices using the ax88179_178a
driver.  (Mark Lord still needs to confirm which device he uses.)  I
have not seen any other reports that other USB ethernet chipsets were
broken in 3.12 by the USB networking layer adding scatter-gather
support.

It should not matter what alignment or length of scatter-gather list the
upper layers pass the xHCI driver, it should just work.  I want to do
this fix right, by changing the fundamental way we queue TRBs to the
rings to fit the TD rules.  We should break each TD into fragment-sized
chunks, and add a link TRB in the middle of a segment where necessary.

Let's do this fix the right way, instead of wall papering over the
issue.  Here's what we should do:

1. Disable scatter-gather for the ax88179_178a driver when it's under an
   xHCI host.

2. Revert the following commits:
   f2d9b991c549 xhci: Set scatter-gather limit to avoid failed block writes.
   d6c9ea9069af xhci: Avoid infinite loop when sg urb requires too many trbs
   35773dac5f86 usb: xhci: Link TRB must not occur within a USB payload burst

3. Dan and Mathias can work together to come up with an overall plan to
   change the xHCI driver architecture to be fully compliant with the TD
   fragment rules.  That can be done over the next few kernel releases.

The end result is that we don't destabilize storage or break userspace
USB drivers, we don't break people's xHCI host controllers,
the ax88179_178a USB ethernet devices still work under xHCI (a bit with
worse performance), and other USB ethernet devices still get the
performance improvement introduced in 3.12.

Sarah Sharp
--
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

Powered by blists - more mailing lists