RE: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
From: Mark Lord
On 14-02-01 09:18 AM, Ming Lei wrote:
Even real regressions are easily/often introduced, and we are discussing
how to fix that. I suggest to unset the flag only for the known buggy
controllers.
It is not the controllers that are particularly buggy here.
But rather the drivers and design of parts of the kernel.
I suspect that the documentation is describing the actual implementation
of a specific hardware implementation, not necessarily how the hardware was
intended to behave.
The requirement for two 32bit accesses to a 64bit register is very similar.
This also means that implementations of the hardware that claim conformance
to the 0.96 specification might have similar issues.
Given the small number of xhci controllers and the even smaller number of
VHDL (or similar) sources they will be based on, it really ought to be
possible to tabulate the controller versions and families to get a much
better idea of their behaviour.
I've got two systems with Intel USB3 controllers, linux reports one as
'panther point', the other as '7 Series/C210 Series' (seems to be a Xeon
chipset). I've no idea how the latter relates to the former.
David
N�r��yb�X��ǧv�^�){.n�+{��^n�r���z���h��� �G���h�(�階�ݢj���m��z�ޖ���f���h���~�m�
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
On Sat, Feb 01, 2014 at 03:05:21PM -0500, Mark Lord wrote: On 14-02-01 09:18 AM, Ming Lei wrote: Even real regressions are easily/often introduced, and we are discussing how to fix that. I suggest to unset the flag only for the known buggy controllers. Ming, the regression cannot be easily fixed in this case. We tried the easy, quick fix and it broke USB storage and usbfs. The patches to paper over those issues started to creep into the upper layers, and I'm not willing to add more code to hack around the issues caused by the quick fix. We need to do this right, not wall-paper over the issues. It is not the controllers that are particularly buggy here. But rather the drivers and design of parts of the kernel. As Mark mentioned, the host controllers aren't buggy. The xHCI driver simply doesn't handle a 1.0 host controller requirement, TD fragments, very well. Only the USB ethernet layer triggers this bug, because the USB storage layer hands down scatter-gather lists in multiples of the max packet size. You tested on a 1.0 host controller, and it apparently didn't need the TD fragments requirement. It seems that Intel 1.0 xHCI host controllers do need that requirement. Perhaps we can add an xHCI driver quirk for an exception so that your host can allow any kind of scatter-gather? Sarah Sharp -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
On Mon, Feb 03, 2014 at 09:54:09AM +, David Laight wrote: From: Mark Lord On 14-02-01 09:18 AM, Ming Lei wrote: Even real regressions are easily/often introduced, and we are discussing how to fix that. I suggest to unset the flag only for the known buggy controllers. It is not the controllers that are particularly buggy here. But rather the drivers and design of parts of the kernel. I suspect that the documentation is describing the actual implementation of a specific hardware implementation, not necessarily how the hardware was intended to behave. You are speculating. Please stop speculating without evidence. It does not add to this conversation. Sarah Sharp -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
On 14-02-01 02:54 AM, Ming Lei wrote: .. With SG enabled, for the iperf client test case, the average urb size for transmission will be increased from ~1500 to ~20K bytes in my test case: iperf -c $SRV -t 30 -P 4 -w 128K So I am wondering you guys do not care the improvement .. No, that's not it. Simply, the recent changes killed the driver for some users, something Linus calls a regression, and does not permit. Far better to have it continue to work than not to work. The plan discussed earlier calls for reintroduction of SG here once the problems are solved outside of the main tree. -- Mark Lord Real-Time Remedies Inc. ml...@pobox.com -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
On Sat, Feb 1, 2014 at 9:30 PM, Mark Lord ml...@pobox.com wrote: On 14-02-01 02:54 AM, Ming Lei wrote: .. With SG enabled, for the iperf client test case, the average urb size for transmission will be increased from ~1500 to ~20K bytes in my test case: iperf -c $SRV -t 30 -P 4 -w 128K So I am wondering you guys do not care the improvement .. No, that's not it. Simply, the recent changes killed the driver I just want to clarify the sg approach does improve performance, instead of no improvement mentioned by your guys. for some users, something Linus calls a regression, and does not permit. Even real regressions are easily/often introduced, and we are discussing how to fix that. I suggest to unset the flag only for the known buggy controllers. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
On 14-02-01 09:18 AM, Ming Lei wrote: Even real regressions are easily/often introduced, and we are discussing how to fix that. I suggest to unset the flag only for the known buggy controllers. It is not the controllers that are particularly buggy here. But rather the drivers and design of parts of the kernel. Cheers -- Mark Lord Real-Time Remedies Inc. ml...@pobox.com -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
From: Peter Stuge But what about that alignment? It seems that userspace needs to start caring about alignment with xhci, right? No because there is a copy_to/from_user() in the middle. The ehci/ohci/uhci all require that fragments be a multiple of the usb message size (512 bytes for USB2). So everything (until very recently) would always supply suitable aligned buffers. Mostly they are page aligned. For those who haven't read the xhci spec carefully: The xhci controller removes the requirement on dma segments being aligned to usb messages. However there are two alignment requirements: 1) dma segments must not cross 64k address boundaries. This is documented clearly, even though it is a slight pain. You'd have thought the address counter could have more than 16 bits these days! There only 17 bits for the length, but a length restriction would be less of a problem. 2) The v1.00 version of the specification adds that the end of the transfer ring can only occur at a 'TD fragment' boundary. These are aligned with the payload 'bursts' - which can be sixteen 1k packets. I think that breaking the second of these causes a usb message be split into two small pieces - which will terminate bulk xfers. The asix USB3 Ge silicon gets very confused when this happens. David -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
From: Peter Stuge [mailto:pe...@stuge.se] Userspace doesn't care since everything gets copied into aligned kernel fragments - otherwise the other usb controllers wouldn't work. OK, but not so great if someone wants to squeeze the most performance possible out of USB also from userspace. I'm going off on a tangent now but would it make sense to allow userspace to do alignment if it wants to, and have a way to tell the kernel when urb buffers are pre-aligned? I can only see that mattering if either: 1) The userspace buffers are (say) 4n+1 aligned and the kernel decides to align the copy_from_user(). 2) The code is doing buffer-loaning. Personally I'm not at all sure how often buffer-loaning helps (given the cost of the TLB shootdowns that it often implies). I guess it might be ok if the memory doesn't have to be given a KVA and the user program avoids any COW. In any case and such code could be limited to page-aligned transfers. And/or the user code would have to know at least some of the constraints. The other usb controllers only support 'message' aligned transfers (512 bytes for USB2). All the code I've found achieves this by using page aligned (maybe 4k aligned) fragments. xhci trivially supports this (it has since the 'ring expansion' code was added). xhci can also easily support: 1) Arbitrary fragmentation for a limited number of fragments (by constraining the fragments to a single ring segment). 2) Arbitrary fragmentation provided all the fragments (except the last) exceed some minimal length (by splitting the current or previous fragment at the appropriate boundary). The code for the second is probably worth adding just in case a 4k fragment crosses a 64k boundary. Since arbitrarily fragmented packets can't be sent to other controllers it does seem sensible for the code generating the urb so say that it is (or might be) fragmented like that. David -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
From: Sarah Sharp On Thu, Jan 30, 2014 at 10:50:21PM +0100, Bjørn Mork wrote: FWIW, the plan looks fine to me. Just adding a couple of hints to simplify the implementation. Sarah Sharp sarah.a.sh...@linux.intel.com writes: 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. No need to make this conditional. SG is only enabled in the ax88179_178a driver if udev-bus-no_sg_constraint is true, so it applies only to xHCI hosts in the first place. Leave the usbnet code alone and unset udev-bus-no_sg_constraint. David -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
On Fri, Jan 31, 2014 at 08:17:58AM +0800, Ming Lei wrote: On Fri, Jan 31, 2014 at 6:15 AM, Sarah Sharp sarah.a.sh...@linux.intel.com wrote: On Thu, Jan 30, 2014 at 10:50:21PM +0100, Bjørn Mork wrote: FWIW, the plan looks fine to me. Just adding a couple of hints to simplify the implementation. Sarah Sharp sarah.a.sh...@linux.intel.com writes: 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. No need to make this conditional. SG is only enabled in the ax88179_178a driver if udev-bus-no_sg_constraint is true, so it applies only to xHCI hosts in the first place. Ah, so you're suggesting just reverting commit 3804fad45411b48233b48003e33a78f290d227c8 USBNET: ax88179_178a: enable tso if usb host supports sg dma? If I understand the problem correctly, the current issue is that xhci driver doesn't support the arbitrary dma length not well, but per XHCI spec, it should be supported, right? If the above is correct, reverting the commit isn't correct since there isn't any issue about the commit, so I suggest to disable the flag in xhci for the buggy devices, and it may be enabled again if the problem is fixed. Ok, I like that plan, since it means I don't have to touch any networking code to fix this. :) I believe that means we'll have to disable the flag for all 1.0 xHCI hosts, since those are the ones that need TD fragments. 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. No other usbnet drivers has enabled SG... Which is why you have only seen this problem with the ax88179_178a devices. So there is no performance improvement to keep. In my test environment, the patch does improve both throughput and cpu utilization, if you search the previous email for the patch, you can see the data. Right, I did see the performance improvement note in that commit. Do you know if the ARM A15 dual core board was using a 0.96 xHCI host, or a 1.0 host? You can find out by reloading the xHCI driver with dynamic debugging turned on: # sudo modprobe xhci_hcd dyndbg and then look for lines like: [25296.765767] xhci_hcd :00:14.0: HCIVERSION: 0x100 Sarah Sharp -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
On Sat, Feb 1, 2014 at 3:00 AM, Sarah Sharp sarah.a.sh...@linux.intel.com wrote: On Fri, Jan 31, 2014 at 08:17:58AM +0800, Ming Lei wrote: On Fri, Jan 31, 2014 at 6:15 AM, Sarah Sharp sarah.a.sh...@linux.intel.com wrote: On Thu, Jan 30, 2014 at 10:50:21PM +0100, Bjørn Mork wrote: FWIW, the plan looks fine to me. Just adding a couple of hints to simplify the implementation. Sarah Sharp sarah.a.sh...@linux.intel.com writes: 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. No need to make this conditional. SG is only enabled in the ax88179_178a driver if udev-bus-no_sg_constraint is true, so it applies only to xHCI hosts in the first place. Ah, so you're suggesting just reverting commit 3804fad45411b48233b48003e33a78f290d227c8 USBNET: ax88179_178a: enable tso if usb host supports sg dma? If I understand the problem correctly, the current issue is that xhci driver doesn't support the arbitrary dma length not well, but per XHCI spec, it should be supported, right? If the above is correct, reverting the commit isn't correct since there isn't any issue about the commit, so I suggest to disable the flag in xhci for the buggy devices, and it may be enabled again if the problem is fixed. Ok, I like that plan, since it means I don't have to touch any networking code to fix this. :) I believe that means we'll have to disable the flag for all 1.0 xHCI hosts, since those are the ones that need TD fragments. 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. No other usbnet drivers has enabled SG... Which is why you have only seen this problem with the ax88179_178a devices. So there is no performance improvement to keep. In my test environment, the patch does improve both throughput and cpu utilization, if you search the previous email for the patch, you can see the data. With SG enabled, for the iperf client test case, the average urb size for transmission will be increased from ~1500 to ~20K bytes in my test case: iperf -c $SRV -t 30 -P 4 -w 128K So I am wondering you guys do not care the improvement, maybe the CPU is powerful enough to not degrade throughoutcpu utilization not much, but there is still the potential CPU wakeup issue, which means extra CPU power consumption might be introduced after disabling SG for usbnet. Right, I did see the performance improvement note in that commit. Do you know if the ARM A15 dual core board was using a 0.96 xHCI host, or a 1.0 host? You can find out by reloading the xHCI driver with dynamic debugging turned on: # sudo modprobe xhci_hcd dyndbg Looks I can't find the parameter 'dyndbg' for xhci_hcd. and then look for lines like: [25296.765767] xhci_hcd :00:14.0: HCIVERSION: 0x100 I change xhci_dbg.c manually with below: diff --git a/drivers/usb/host/xhci-dbg.c b/drivers/usb/host/xhci-dbg.c index b016d38..1ae1966 100644 --- a/drivers/usb/host/xhci-dbg.c +++ b/drivers/usb/host/xhci-dbg.c @@ -66,7 +66,7 @@ static void xhci_print_cap_regs(struct xhci_hcd *xhci) (unsigned int) temp); xhci_dbg(xhci, CAPLENGTH: 0x%x\n, (unsigned int) HC_LENGTH(temp)); - xhci_dbg(xhci, HCIVERSION: 0x%x\n, + dev_info(xhci_to_hcd(xhci)-self.controller, HCIVERSION: 0x%x\n, (unsigned int) HC_VERSION(temp)); and got the below output: [tom@ming ~]$ dmesg | grep HCIVERSION xhci-hcd xhci-hcd.2.auto: HCIVERSION: 0x100 Thanks, -- Ming Lei -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
Some xhci (USB3) controllers have a constraint on the offset within a
bulk transfer of the end of the transfer ring.
The xhci documentation (v1.00, but not the earlier versions) states that
the offset (from the beginning of the transfer) at end of the transfer
ring must be a multiple of the burst size (this is actually 16k for USB3
since the controller is configured for 16 message bursts).
However the effect is probably that the transfer is split at the ring end,
so the target will see correct messages provided the data is 1k aligned.
This mostly affects scatter-gather transfer requests, but can potentially
affect other requests as they must be split on 64k address boundaries.
(It might even affect non-bulk transfers.)
The only known current source of such misaligned transfers is the
ax88179_178a ethernet driver. The hardware stops transmitting ethernet
frames when the host controller (presumably) spilts a 1k message.
Not all host controllers behave this way.
The Intel Panther Point used on recent motherboards is affected.
A fix has been applied to the xhci driver (and backported), however this
has a side effect of limiting the number of fragments that can be sent.
(It works by putting all the buffer fragments in one ring segment.)
The SCSI system generates more fragments than was originally thought, and
code using libusb can generate arbitrarily long transfers that usually
get split into 8k fragments.
We've had reports of 4MB libusb requests failing. A 16MB request would
require 256 fragments (because of the requirement to not cross a 64k
address boundary) so could not be fitted into the 255 ring slots regarless
of the number and alignment of any fragments.
In fact libusb always uses 8k fragments. Anything over 1M can't be
split with the current limit of 128 fragments and is sent unfragmented.
This leads to kmalloc() failures.
This all means that the xhci driver needs to accept unlimited numbers
of 'aligned' fragments and only restrict the number of misaligned ones.
None of the other USB controllers allow buffer fragments that cross
USB message boundaries (512 bytes for USB2), so almost all the code
uses aligned buffers. Potentially these might cross 64k boundaries
at unaligned offsets, but I suspect that really doesn't happen.
So rather than change all the code that generates urbs, this patch
modifies the only code that generates misaligned transfares to tell
the host controller that the buffer might have alignment issues.
The patch:
- Adds the flag URB_UNCONSTRAINED_XFER to urb-transfer_flags.
This reuses the value of URB_ASYNC_UNLINK (removed in 2005).
- Sets the flag in usbnet.c for all transmit requests.
Since the buffer offsets aren't aligned an unfragmented message might
need splitting on a 64k boundary.
- Pass the transfer_flags down to prepare_ring() and only check for
the end of ring segments (filling with NOPs) if the flag is set.
- Remove the advertised restriction on the number fragments xhci supports.
This doesn't actually define what a 'constrained' transfer is - but
that wasn't defined when no_sg_constraint was added to struct usb_bus.
Possibly there should also be separate limits of the number of 'constrained'
and 'unconstrained' scatter-gather lists. But and the moment the former
is (more or less) required to be infinite, and the limit of the latter
won't be reached by any code that sets the flag.
Signed-off-by: David Laight david.lai...@aculab.com
---
drivers/net/usb/usbnet.c | 1 +
drivers/usb/host/xhci-ring.c | 12
drivers/usb/host/xhci.c | 8 ++--
include/linux/usb.h | 1 +
4 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 4671da7..504be5b 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1303,6 +1303,7 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb,
if (build_dma_sg(skb, urb) 0)
goto drop;
}
+ urb-transfer_flags |= URB_UNCONSTRAINED_XFER;
length = urb-transfer_buffer_length;
/* don't assume the hardware handles USB_ZERO_PACKET
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index a0b248c..5860874 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2932,7 +2932,8 @@ static void queue_trb(struct xhci_hcd *xhci, struct
xhci_ring *ring,
* FIXME allocate segments if the ring is full.
*/
static int prepare_ring(struct xhci_hcd *xhci, struct xhci_ring *ep_ring,
- u32 ep_state, unsigned int num_trbs, gfp_t mem_flags)
+ u32 ep_state, unsigned int num_trbs, gfp_t mem_flags,
+ unsigned int transfer_flags)
{
unsigned int num_trbs_needed;
@@ -2980,6 +2981,9 @@ static int prepare_ring(struct xhci_hcd *xhci, struct
xhci_ring *ep_ring,
* Simplest solution is to fill the trb before the
* LINK with nop
RE: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
From: Peter Stuge ... code using libusb can generate arbitrarily long transfers that usually get split into 8k fragments. libusb splits transfers into 16k urbs, or doesn't with newer code when both kernel and libusb support scatter-gather. In fact libusb always uses 8k fragments. Hm? Worst-case libusb-1.0 submits 16k urbs. libusb-0.1 I'm unsure about, but could check. ... Where's the 8k coming from? My memory, I meant 16k :-( David -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
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? //Peter -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
On 14-01-30 04:18 PM, Sarah Sharp wrote: 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. Performance before 3.12/3.13 was not all that bad either. My ax88179 dongle (yes, that one, using ax88179_178a.ko) manages very close to 1gbit/sec throughput even without SG, and without a huge cpu tax either. SG done Right will make it better eventually. I can wait. Cheers -- Mark Lord Real-Time Remedies Inc. ml...@pobox.com -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
Sarah, on a related note: Is there a parameter or knob of some kind to tell the XHCI driver to treat a specific port as USB2 (480mbit/sec max) rather than USB3 ? The Dell XPS-13 Ultrabooks all suffer from some kind of flaw, whereby the left side USB3 port is unreliable at SuperSpeed; the right side port works flawlessly. The MS-Windows driver has a workaround of some sort, but we don't. Cheers -- Mark Lord Real-Time Remedies Inc. ml...@pobox.com -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
On Thu, 30 Jan 2014, Sarah Sharp wrote: 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. That's a good plan. However _some_ restriction will turn out to be necessary. For example, what will you do if a driver submits an SG list containing 300 elements, each 3 bytes long? That's too many to fit in a single ring segment, but it's smaller than a TD fragment -- it's even smaller than maxpacket -- so there's no place to split it. (Not that I think drivers _will_ submit requests like this; this is just to demonstrate the point.) It ought to be acceptable to require, for example, that an SG URB contain no more than (say) 100 elements that are smaller than 512 bytes. ehci-hcd gets along okay with the restriction that each SG element except the last has to be a multiple of the maxpacket size. xhci-hcd can relax this quite a lot, but not all the way. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
On 14-01-30 04:43 PM, Alan Stern wrote: On Thu, 30 Jan 2014, Sarah Sharp wrote: 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. That's a good plan. However _some_ restriction will turn out to be necessary. For example, what will you do if a driver submits an SG list containing 300 elements, each 3 bytes long? Allocate a contiguous (bounce) buffer and copy the fragments to/from it? -- Mark Lord Real-Time Remedies Inc. ml...@pobox.com -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
FWIW, the plan looks fine to me. Just adding a couple of hints to simplify the implementation. Sarah Sharp sarah.a.sh...@linux.intel.com writes: 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. No need to make this conditional. SG is only enabled in the ax88179_178a driver if udev-bus-no_sg_constraint is true, so it applies only to xHCI hosts in the first place. 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. No other usbnet drivers has enabled SG... Which is why you have only seen this problem with the ax88179_178a devices. So there is no performance improvement to keep. Bjørn -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
On Thu, Jan 30, 2014 at 04:43:54PM -0500, Alan Stern wrote: On Thu, 30 Jan 2014, Sarah Sharp wrote: 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. That's a good plan. However _some_ restriction will turn out to be necessary. For example, what will you do if a driver submits an SG list containing 300 elements, each 3 bytes long? That's too many to fit in a single ring segment, but it's smaller than a TD fragment -- it's even smaller than maxpacket -- so there's no place to split it. (Not that I think drivers _will_ submit requests like this; this is just to demonstrate the point.) It ought to be acceptable to require, for example, that an SG URB contain no more than (say) 100 elements that are smaller than 512 bytes. At that point, the xHCI driver or USB core should probably use a bounce buffer. It feels like we should attempt to push down scatter-gather lists as far down in the stack as possible, so the upper layers don't have to care what alignment, length, or random 64KB boundary splits we need. ehci-hcd gets along okay with the restriction that each SG element except the last has to be a multiple of the maxpacket size. xhci-hcd can relax this quite a lot, but not all the way. What does the EHCI driver do when it receives a SG list from the USB networking layer that violates this restriction? Sarah Sharp -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
Sarah Sharp sarah.a.sh...@linux.intel.com writes: On Thu, Jan 30, 2014 at 04:43:54PM -0500, Alan Stern wrote: ehci-hcd gets along okay with the restriction that each SG element except the last has to be a multiple of the maxpacket size. xhci-hcd can relax this quite a lot, but not all the way. What does the EHCI driver do when it receives a SG list from the USB networking layer that violates this restriction? The USB networking layer won't use SG with the EHCI driver. Commit bcc48f1a7a0d4 introduced no_sg_constraint so that usbnet could enable SG only for host controllers with no such restrictions. I.e. currently for xHCI only. Bjørn -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
On Thu, 30 Jan 2014, Sarah Sharp wrote: That's a good plan. However _some_ restriction will turn out to be necessary. For example, what will you do if a driver submits an SG list containing 300 elements, each 3 bytes long? That's too many to fit in a single ring segment, but it's smaller than a TD fragment -- it's even smaller than maxpacket -- so there's no place to split it. (Not that I think drivers _will_ submit requests like this; this is just to demonstrate the point.) It ought to be acceptable to require, for example, that an SG URB contain no more than (say) 100 elements that are smaller than 512 bytes. At that point, the xHCI driver or USB core should probably use a bounce buffer. It feels like we should attempt to push down scatter-gather lists as far down in the stack as possible, so the upper layers don't have to care what alignment, length, or random 64KB boundary splits we need. Okay. That should be doable, if awkward. ehci-hcd gets along okay with the restriction that each SG element except the last has to be a multiple of the maxpacket size. xhci-hcd can relax this quite a lot, but not all the way. What does the EHCI driver do when it receives a SG list from the USB networking layer that violates this restriction? It never receives such lists. usb_submit_urb() returns -EINVAL before the request gets sent to ehci-hcd. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
On Fri, Jan 31, 2014 at 6:15 AM, Sarah Sharp sarah.a.sh...@linux.intel.com wrote: On Thu, Jan 30, 2014 at 10:50:21PM +0100, Bjørn Mork wrote: FWIW, the plan looks fine to me. Just adding a couple of hints to simplify the implementation. Sarah Sharp sarah.a.sh...@linux.intel.com writes: 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. No need to make this conditional. SG is only enabled in the ax88179_178a driver if udev-bus-no_sg_constraint is true, so it applies only to xHCI hosts in the first place. Ah, so you're suggesting just reverting commit 3804fad45411b48233b48003e33a78f290d227c8 USBNET: ax88179_178a: enable tso if usb host supports sg dma? If I understand the problem correctly, the current issue is that xhci driver doesn't support the arbitrary dma length not well, but per XHCI spec, it should be supported, right? If the above is correct, reverting the commit isn't correct since there isn't any issue about the commit, so I suggest to disable the flag in xhci for the buggy devices, and it may be enabled again if the problem is fixed. 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. No other usbnet drivers has enabled SG... Which is why you have only seen this problem with the ax88179_178a devices. So there is no performance improvement to keep. In my test environment, the patch does improve both throughput and cpu utilization, if you search the previous email for the patch, you can see the data. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
