RE: [PATCH 2/2] Revert "USBNET: ax88179_178a: enable tso if usb host supports sg dma"
From: > On Fri, 7 Mar 2014, David Laight wrote: > > > From: Alan Stern > > > On Fri, 7 Mar 2014, David Laight wrote: > > > > > > > From: Mathias Nyman > > > > > This reverts commit 3804fad45411b48233b48003e33a78f290d227c8. > > > > > > > > > > This commit, together with commit > > > > > 247bf557273dd775505fb9240d2d152f4f20d304 > > > > > "xhci 1.0: Limit arbitrarily-aligned scatter gather." were > > > > > origially added to get xHCI 1.0 hosts and usb ethernet ax88179_178a > > > > > devices > > > > > working together with scatter gather. xHCI 1.0 hosts pose some > > > > > requirement on how transfer > > > > > buffers are aligned, setting this requirement for 1.0 hosts caused > > > > > USB 3.0 mass > > > > > storage devices to fail more frequently. > > > > > > > > This patch doesn't need to be reverted. > > > > > > Yes, it does. > > > > > > > Provided the xhci driver doesn't set the flag to say that arbitrary > > > > scatter > > > > gather is supported (ie usb_device_no_sg_constraint(dev->udev)) is > > > > false) > > > > the ax88179_178a driver won't request transmits that need fragmenting. > > > > > > True. But xhci-hcd _will_ set the flag, because of patch 1 in this > > > series. In other words, patch 1 makes patch 2 necessary. > > > > I was reading patch 2 first. > > It would seem to be better to modify patch 1 - so it doesn't set the flag. > > Then the changes are limited to the usb tree, and the change to net wouldn't > > need to be reapplied in order to test scatter-gather when it is properly > > fixed. > > > The point is that the no_sg_constraint was added in order to allow > > the ax88179_178a driver to send arbitrarily aligned transfers. > > Nothing else looks at it (well didn't when I scanned the tree a while back). > > > > In effect all the other transfer requests were assumed to be suitably > > aligned. > > > > The check that caused things to fail was the one added to xhci relatively > > recently that verified the alignment of the fragments. > > (Actually the check was added to usbcore, not to xhci-hcd.) > > The _real_ problem here seems to be that "no_sg_constraint" is > ambiguous. Originally it was meant to refer to the constraint that all > SG elements except the last must be a multiple of the maxpacket size. > For that purpose, the check added to usbcore was entirely appropriate, > as was setting the flag in xhci-hcd. Probably true - but the only code that looked at it was in usbnet. The check in usbcore is very recent. > But now it turns out that the ax88179 driver is violating a _different_ > constraint: that Link TRBs must occur only at 1-KB boundaries. The > "no_sg_constraint" flag was never meant to describe this. In other > words, the flag issue is separate from the problem facing ax88179. Really that means that the xhci controller couldn't actually support the alignments it said it could - rather than the ax88179 driver sending packets that didn't meet that the rules. > The appropriate way to address this new problem for the future is to > remove the second constraint by adding correct support for TD > fragments into xhci-hcd. Indeed. > The appropriate way to address the problem > right now in the -stable kernels is to prevent ax88179 from using SG -- > and not by abusing the "no_sg_constraint" flag, which is not directly > related to the TD fragment problem. I'd say that if the no_sg_constraint flag is set the ax88179 driver could reasonably expect to send its fragment lists. So it is best not to set the no_sg_constraint flag, and then remove the checks against it that are needed to allow the transfers from the disk system not be rejected - even though we know that some of them might not actually work. Otherwise you'll need to add yet another flag when the sg support is fixed. David -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/2] Revert "USBNET: ax88179_178a: enable tso if usb host supports sg dma"
On Fri, 7 Mar 2014, David Laight wrote: > From: Alan Stern > > On Fri, 7 Mar 2014, David Laight wrote: > > > > > From: Mathias Nyman > > > > This reverts commit 3804fad45411b48233b48003e33a78f290d227c8. > > > > > > > > This commit, together with commit > > > > 247bf557273dd775505fb9240d2d152f4f20d304 > > > > "xhci 1.0: Limit arbitrarily-aligned scatter gather." were > > > > origially added to get xHCI 1.0 hosts and usb ethernet ax88179_178a > > > > devices > > > > working together with scatter gather. xHCI 1.0 hosts pose some > > > > requirement on how transfer > > > > buffers are aligned, setting this requirement for 1.0 hosts caused USB > > > > 3.0 mass > > > > storage devices to fail more frequently. > > > > > > This patch doesn't need to be reverted. > > > > Yes, it does. > > > > > Provided the xhci driver doesn't set the flag to say that arbitrary > > > scatter > > > gather is supported (ie usb_device_no_sg_constraint(dev->udev)) is false) > > > the ax88179_178a driver won't request transmits that need fragmenting. > > > > True. But xhci-hcd _will_ set the flag, because of patch 1 in this > > series. In other words, patch 1 makes patch 2 necessary. > > I was reading patch 2 first. > It would seem to be better to modify patch 1 - so it doesn't set the flag. > Then the changes are limited to the usb tree, and the change to net wouldn't > need to be reapplied in order to test scatter-gather when it is properly > fixed. > The point is that the no_sg_constraint was added in order to allow > the ax88179_178a driver to send arbitrarily aligned transfers. > Nothing else looks at it (well didn't when I scanned the tree a while back). > > In effect all the other transfer requests were assumed to be suitably > aligned. > > The check that caused things to fail was the one added to xhci relatively > recently that verified the alignment of the fragments. (Actually the check was added to usbcore, not to xhci-hcd.) The _real_ problem here seems to be that "no_sg_constraint" is ambiguous. Originally it was meant to refer to the constraint that all SG elements except the last must be a multiple of the maxpacket size. For that purpose, the check added to usbcore was entirely appropriate, as was setting the flag in xhci-hcd. But now it turns out that the ax88179 driver is violating a _different_ constraint: that Link TRBs must occur only at 1-KB boundaries. The "no_sg_constraint" flag was never meant to describe this. In other words, the flag issue is separate from the problem facing ax88179. The appropriate way to address this new problem for the future is to remove the second constraint by adding correct support for TD fragments into xhci-hcd. The appropriate way to address the problem right now in the -stable kernels is to prevent ax88179 from using SG -- and not by abusing the "no_sg_constraint" flag, which is not directly related to the TD fragment problem. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/2] Revert "USBNET: ax88179_178a: enable tso if usb host supports sg dma"
From: Alan Stern > On Fri, 7 Mar 2014, David Laight wrote: > > > From: Mathias Nyman > > > This reverts commit 3804fad45411b48233b48003e33a78f290d227c8. > > > > > > This commit, together with commit 247bf557273dd775505fb9240d2d152f4f20d304 > > > "xhci 1.0: Limit arbitrarily-aligned scatter gather." were > > > origially added to get xHCI 1.0 hosts and usb ethernet ax88179_178a > > > devices > > > working together with scatter gather. xHCI 1.0 hosts pose some > > > requirement on how transfer > > > buffers are aligned, setting this requirement for 1.0 hosts caused USB > > > 3.0 mass > > > storage devices to fail more frequently. > > > > This patch doesn't need to be reverted. > > Yes, it does. > > > Provided the xhci driver doesn't set the flag to say that arbitrary scatter > > gather is supported (ie usb_device_no_sg_constraint(dev->udev)) is false) > > the ax88179_178a driver won't request transmits that need fragmenting. > > True. But xhci-hcd _will_ set the flag, because of patch 1 in this > series. In other words, patch 1 makes patch 2 necessary. I was reading patch 2 first. It would seem to be better to modify patch 1 - so it doesn't set the flag. Then the changes are limited to the usb tree, and the change to net wouldn't need to be reapplied in order to test scatter-gather when it is properly fixed. David -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/2] Revert "USBNET: ax88179_178a: enable tso if usb host supports sg dma"
On Fri, 7 Mar 2014, David Laight wrote: > From: Mathias Nyman > > This reverts commit 3804fad45411b48233b48003e33a78f290d227c8. > > > > This commit, together with commit 247bf557273dd775505fb9240d2d152f4f20d304 > > "xhci 1.0: Limit arbitrarily-aligned scatter gather." were > > origially added to get xHCI 1.0 hosts and usb ethernet ax88179_178a devices > > working together with scatter gather. xHCI 1.0 hosts pose some requirement > > on how transfer > > buffers are aligned, setting this requirement for 1.0 hosts caused USB 3.0 > > mass > > storage devices to fail more frequently. > > This patch doesn't need to be reverted. Yes, it does. > Provided the xhci driver doesn't set the flag to say that arbitrary scatter > gather is supported (ie usb_device_no_sg_constraint(dev->udev)) is false) > the ax88179_178a driver won't request transmits that need fragmenting. True. But xhci-hcd _will_ set the flag, because of patch 1 in this series. In other words, patch 1 makes patch 2 necessary. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/2] Revert "USBNET: ax88179_178a: enable tso if usb host supports sg dma"
From: Mathias Nyman > This reverts commit 3804fad45411b48233b48003e33a78f290d227c8. > > This commit, together with commit 247bf557273dd775505fb9240d2d152f4f20d304 > "xhci 1.0: Limit arbitrarily-aligned scatter gather." were > origially added to get xHCI 1.0 hosts and usb ethernet ax88179_178a devices > working together with scatter gather. xHCI 1.0 hosts pose some requirement on > how transfer > buffers are aligned, setting this requirement for 1.0 hosts caused USB 3.0 > mass > storage devices to fail more frequently. This patch doesn't need to be reverted. Provided the xhci driver doesn't set the flag to say that arbitrary scatter gather is supported (ie usb_device_no_sg_constraint(dev->udev)) is false) the ax88179_178a driver won't request transmits that need fragmenting. There is a separate issue in that it does request receives that need fragmenting. However if the fragmented receives end up being split by a LINK TRB the device driver recovers. The ax88179 hardware doesn't recover when a tx is split. David -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/2] Revert USBNET: ax88179_178a: enable tso if usb host supports sg dma
From: Mathias Nyman This reverts commit 3804fad45411b48233b48003e33a78f290d227c8. This commit, together with commit 247bf557273dd775505fb9240d2d152f4f20d304 xhci 1.0: Limit arbitrarily-aligned scatter gather. were origially added to get xHCI 1.0 hosts and usb ethernet ax88179_178a devices working together with scatter gather. xHCI 1.0 hosts pose some requirement on how transfer buffers are aligned, setting this requirement for 1.0 hosts caused USB 3.0 mass storage devices to fail more frequently. This patch doesn't need to be reverted. Provided the xhci driver doesn't set the flag to say that arbitrary scatter gather is supported (ie usb_device_no_sg_constraint(dev-udev)) is false) the ax88179_178a driver won't request transmits that need fragmenting. There is a separate issue in that it does request receives that need fragmenting. However if the fragmented receives end up being split by a LINK TRB the device driver recovers. The ax88179 hardware doesn't recover when a tx is split. David -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/2] Revert USBNET: ax88179_178a: enable tso if usb host supports sg dma
On Fri, 7 Mar 2014, David Laight wrote: From: Mathias Nyman This reverts commit 3804fad45411b48233b48003e33a78f290d227c8. This commit, together with commit 247bf557273dd775505fb9240d2d152f4f20d304 xhci 1.0: Limit arbitrarily-aligned scatter gather. were origially added to get xHCI 1.0 hosts and usb ethernet ax88179_178a devices working together with scatter gather. xHCI 1.0 hosts pose some requirement on how transfer buffers are aligned, setting this requirement for 1.0 hosts caused USB 3.0 mass storage devices to fail more frequently. This patch doesn't need to be reverted. Yes, it does. Provided the xhci driver doesn't set the flag to say that arbitrary scatter gather is supported (ie usb_device_no_sg_constraint(dev-udev)) is false) the ax88179_178a driver won't request transmits that need fragmenting. True. But xhci-hcd _will_ set the flag, because of patch 1 in this series. In other words, patch 1 makes patch 2 necessary. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/2] Revert USBNET: ax88179_178a: enable tso if usb host supports sg dma
From: Alan Stern On Fri, 7 Mar 2014, David Laight wrote: From: Mathias Nyman This reverts commit 3804fad45411b48233b48003e33a78f290d227c8. This commit, together with commit 247bf557273dd775505fb9240d2d152f4f20d304 xhci 1.0: Limit arbitrarily-aligned scatter gather. were origially added to get xHCI 1.0 hosts and usb ethernet ax88179_178a devices working together with scatter gather. xHCI 1.0 hosts pose some requirement on how transfer buffers are aligned, setting this requirement for 1.0 hosts caused USB 3.0 mass storage devices to fail more frequently. This patch doesn't need to be reverted. Yes, it does. Provided the xhci driver doesn't set the flag to say that arbitrary scatter gather is supported (ie usb_device_no_sg_constraint(dev-udev)) is false) the ax88179_178a driver won't request transmits that need fragmenting. True. But xhci-hcd _will_ set the flag, because of patch 1 in this series. In other words, patch 1 makes patch 2 necessary. I was reading patch 2 first. It would seem to be better to modify patch 1 - so it doesn't set the flag. Then the changes are limited to the usb tree, and the change to net wouldn't need to be reapplied in order to test scatter-gather when it is properly fixed. David -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/2] Revert USBNET: ax88179_178a: enable tso if usb host supports sg dma
On Fri, 7 Mar 2014, David Laight wrote: From: Alan Stern On Fri, 7 Mar 2014, David Laight wrote: From: Mathias Nyman This reverts commit 3804fad45411b48233b48003e33a78f290d227c8. This commit, together with commit 247bf557273dd775505fb9240d2d152f4f20d304 xhci 1.0: Limit arbitrarily-aligned scatter gather. were origially added to get xHCI 1.0 hosts and usb ethernet ax88179_178a devices working together with scatter gather. xHCI 1.0 hosts pose some requirement on how transfer buffers are aligned, setting this requirement for 1.0 hosts caused USB 3.0 mass storage devices to fail more frequently. This patch doesn't need to be reverted. Yes, it does. Provided the xhci driver doesn't set the flag to say that arbitrary scatter gather is supported (ie usb_device_no_sg_constraint(dev-udev)) is false) the ax88179_178a driver won't request transmits that need fragmenting. True. But xhci-hcd _will_ set the flag, because of patch 1 in this series. In other words, patch 1 makes patch 2 necessary. I was reading patch 2 first. It would seem to be better to modify patch 1 - so it doesn't set the flag. Then the changes are limited to the usb tree, and the change to net wouldn't need to be reapplied in order to test scatter-gather when it is properly fixed. The point is that the no_sg_constraint was added in order to allow the ax88179_178a driver to send arbitrarily aligned transfers. Nothing else looks at it (well didn't when I scanned the tree a while back). In effect all the other transfer requests were assumed to be suitably aligned. The check that caused things to fail was the one added to xhci relatively recently that verified the alignment of the fragments. (Actually the check was added to usbcore, not to xhci-hcd.) The _real_ problem here seems to be that no_sg_constraint is ambiguous. Originally it was meant to refer to the constraint that all SG elements except the last must be a multiple of the maxpacket size. For that purpose, the check added to usbcore was entirely appropriate, as was setting the flag in xhci-hcd. But now it turns out that the ax88179 driver is violating a _different_ constraint: that Link TRBs must occur only at 1-KB boundaries. The no_sg_constraint flag was never meant to describe this. In other words, the flag issue is separate from the problem facing ax88179. The appropriate way to address this new problem for the future is to remove the second constraint by adding correct support for TD fragments into xhci-hcd. The appropriate way to address the problem right now in the -stable kernels is to prevent ax88179 from using SG -- and not by abusing the no_sg_constraint flag, which is not directly related to the TD fragment problem. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/2] Revert USBNET: ax88179_178a: enable tso if usb host supports sg dma
From: On Fri, 7 Mar 2014, David Laight wrote: From: Alan Stern On Fri, 7 Mar 2014, David Laight wrote: From: Mathias Nyman This reverts commit 3804fad45411b48233b48003e33a78f290d227c8. This commit, together with commit 247bf557273dd775505fb9240d2d152f4f20d304 xhci 1.0: Limit arbitrarily-aligned scatter gather. were origially added to get xHCI 1.0 hosts and usb ethernet ax88179_178a devices working together with scatter gather. xHCI 1.0 hosts pose some requirement on how transfer buffers are aligned, setting this requirement for 1.0 hosts caused USB 3.0 mass storage devices to fail more frequently. This patch doesn't need to be reverted. Yes, it does. Provided the xhci driver doesn't set the flag to say that arbitrary scatter gather is supported (ie usb_device_no_sg_constraint(dev-udev)) is false) the ax88179_178a driver won't request transmits that need fragmenting. True. But xhci-hcd _will_ set the flag, because of patch 1 in this series. In other words, patch 1 makes patch 2 necessary. I was reading patch 2 first. It would seem to be better to modify patch 1 - so it doesn't set the flag. Then the changes are limited to the usb tree, and the change to net wouldn't need to be reapplied in order to test scatter-gather when it is properly fixed. The point is that the no_sg_constraint was added in order to allow the ax88179_178a driver to send arbitrarily aligned transfers. Nothing else looks at it (well didn't when I scanned the tree a while back). In effect all the other transfer requests were assumed to be suitably aligned. The check that caused things to fail was the one added to xhci relatively recently that verified the alignment of the fragments. (Actually the check was added to usbcore, not to xhci-hcd.) The _real_ problem here seems to be that no_sg_constraint is ambiguous. Originally it was meant to refer to the constraint that all SG elements except the last must be a multiple of the maxpacket size. For that purpose, the check added to usbcore was entirely appropriate, as was setting the flag in xhci-hcd. Probably true - but the only code that looked at it was in usbnet. The check in usbcore is very recent. But now it turns out that the ax88179 driver is violating a _different_ constraint: that Link TRBs must occur only at 1-KB boundaries. The no_sg_constraint flag was never meant to describe this. In other words, the flag issue is separate from the problem facing ax88179. Really that means that the xhci controller couldn't actually support the alignments it said it could - rather than the ax88179 driver sending packets that didn't meet that the rules. The appropriate way to address this new problem for the future is to remove the second constraint by adding correct support for TD fragments into xhci-hcd. Indeed. The appropriate way to address the problem right now in the -stable kernels is to prevent ax88179 from using SG -- and not by abusing the no_sg_constraint flag, which is not directly related to the TD fragment problem. I'd say that if the no_sg_constraint flag is set the ax88179 driver could reasonably expect to send its fragment lists. So it is best not to set the no_sg_constraint flag, and then remove the checks against it that are needed to allow the transfers from the disk system not be rejected - even though we know that some of them might not actually work. Otherwise you'll need to add yet another flag when the sg support is fixed. David -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
