[PATCH 1/2] Revert "xhci 1.0: Limit arbitrarily-aligned scatter gather."

[PATCH 1/2] Revert "xhci 1.0: Limit arbitrarily-aligned scatter gather."

[Mathias Nyman]

This reverts commit 247bf557273dd775505fb9240d2d152f4f20d304.

This commit, together with commit 3804fad45411b48233b48003e33a78f290d227c8
"USBNET: ax88179_178a: enable tso if usb host supports sg dma" 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.

USB 3.0 mass storage devices used to work before 3.14-rc1.  Theoretically,
the TD fragment rules could have caused an occasional disk glitch.
Now the devices *will* fail, instead of theoretically failing.
>From a user perspective, this looks like a regression; the USB device obviously
fails on 3.14-rc1, and may sometimes silently fail on prior kernels.

The proper soluition is to implement the TD fragment rules required, but for now
this patch needs to be reverted to get USB 3.0 mass storage devices working at the
level they used to.

Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
Cc: stable <stable@vger.kernel.org>
---
 drivers/usb/host/xhci.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 6fe577d..924a6cc 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4733,6 +4733,9 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
 	/* Accept arbitrarily long scatter-gather lists */
 	hcd->self.sg_tablesize = ~0;
 
+	/* support to build packet from discontinuous buffers */
+	hcd->self.no_sg_constraint = 1;
+
 	/* XHCI controllers don't stop the ep queue on short packets :| */
 	hcd->self.no_stop_on_short = 1;
 
@@ -4757,14 +4760,6 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
 		/* xHCI private pointer was set in xhci_pci_probe for the second
 		 * registered roothub.
 		 */
-		xhci = hcd_to_xhci(hcd);
-		/*
-		 * Support arbitrarily aligned sg-list entries on hosts without
-		 * TD fragment rules (which are currently unsupported).
-		 */
-		if (xhci->hci_version < 0x100)
-			hcd->self.no_sg_constraint = 1;
-
 		return 0;
 	}
 
@@ -4793,9 +4788,6 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
 	if (xhci->hci_version > 0x96)
 		xhci->quirks |= XHCI_SPURIOUS_SUCCESS;
 
-	if (xhci->hci_version < 0x100)
-		hcd->self.no_sg_constraint = 1;
-
 	/* Make sure the HC is halted. */
 	retval = xhci_halt(xhci);
 	if (retval)
-- 
1.8.1.2

RE: [PATCH 1/2] Revert "xhci 1.0: Limit arbitrarily-aligned scatter gather."

[David Laight]

From: Mathias Nyman
> This reverts commit 247bf557273dd775505fb9240d2d152f4f20d304.

You need to revert further.
Just don�t set hcd->self.no_sg_constraint ever - since it just
doesn't work.
That will stop the ax88179_178a driver sending fragmented packets.

With the check for aligned non-terminal fragments removed the 
code will be as bad as the earlier releases.

> This commit, together with commit 3804fad45411b48233b48003e33a78f290d227c8
> "USBNET: ax88179_178a: enable tso if usb host supports sg dma" 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.
> 
> USB 3.0 mass storage devices used to work before 3.14-rc1.  Theoretically,
> the TD fragment rules could have caused an occasional disk glitch.
> Now the devices *will* fail, instead of theoretically failing.
> From a user perspective, this looks like a regression; the USB device obviously
> fails on 3.14-rc1, and may sometimes silently fail on prior kernels.
> 
> The proper soluition is to implement the TD fragment rules required, but for now
> this patch needs to be reverted to get USB 3.0 mass storage devices working at the
> level they used to.
> 
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> Cc: stable <stable@vger.kernel.org>
> ---
>  drivers/usb/host/xhci.c | 14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 6fe577d..924a6cc 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -4733,6 +4733,9 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
>  	/* Accept arbitrarily long scatter-gather lists */
>  	hcd->self.sg_tablesize = ~0;
> 
> +	/* support to build packet from discontinuous buffers */
> +	hcd->self.no_sg_constraint = 1;
> +

Don't add the above - it only looked at by the usbnet code
and in particular the ax88179_178a driver

>  	/* XHCI controllers don't stop the ep queue on short packets :| */
>  	hcd->self.no_stop_on_short = 1;
> 
> @@ -4757,14 +4760,6 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
>  		/* xHCI private pointer was set in xhci_pci_probe for the second
>  		 * registered roothub.
>  		 */
> -		xhci = hcd_to_xhci(hcd);
> -		/*
> -		 * Support arbitrarily aligned sg-list entries on hosts without
> -		 * TD fragment rules (which are currently unsupported).
> -		 */
> -		if (xhci->hci_version < 0x100)
> -			hcd->self.no_sg_constraint = 1;
> -
>  		return 0;
>  	}
> 
> @@ -4793,9 +4788,6 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
>  	if (xhci->hci_version > 0x96)
>  		xhci->quirks |= XHCI_SPURIOUS_SUCCESS;
> 
> -	if (xhci->hci_version < 0x100)
> -		hcd->self.no_sg_constraint = 1;
> -
>  	/* Make sure the HC is halted. */
>  	retval = xhci_halt(xhci);
>  	if (retval)
> --
> 1.8.1.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/2] Revert "xhci 1.0: Limit arbitrarily-aligned scatter gather."

[Alan Stern]

On Fri, 7 Mar 2014, David Laight wrote:

> From: Mathias Nyman
> > This reverts commit 247bf557273dd775505fb9240d2d152f4f20d304.
> 
> You need to revert further.
> Just don�t set hcd->self.no_sg_constraint ever - since it just
> doesn't work.

No; it does work most of the time.

If hcd->self.no_sg_constraint wasn't sent, then certain commands would 
fail always, instead of failing occasionally.

Alan Stern

RE: [PATCH 1/2] Revert "xhci 1.0: Limit arbitrarily-aligned scatter gather."

[David Laight]

From: Alan Stern
> On Fri, 7 Mar 2014, David Laight wrote:
> 
> > From: Mathias Nyman
> > > This reverts commit 247bf557273dd775505fb9240d2d152f4f20d304.
> >
> > You need to revert further.
> > Just don?t set hcd->self.no_sg_constraint ever - since it just
> > doesn't work.
> 
> No; it does work most of the time.
> 
> If hcd->self.no_sg_constraint wasn't sent, then certain commands would
> fail always, instead of failing occasionally.

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.

	David

[PATCH 2/2] Revert "USBNET: ax88179_178a: enable tso if usb host supports sg dma"

[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.

USB 3.0 mass storage devices used to work before 3.14-rc1.  Theoretically,
the TD fragment rules could have caused an occasional disk glitch.
Now the devices *will* fail, instead of theoretically failing.
>From a user perspective, this looks like a regression; the USB device obviously
fails on 3.14-rc1, and may sometimes silently fail on prior kernels.

The proper soluition is to implement the TD fragment rules for xHCI 1.0 hosts,
but for now, revert this patch until scatter gather can be properly supported.

Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
Cc: stable <stable@vger.kernel.org>
---
 drivers/net/usb/ax88179_178a.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
index 955df81..42085e6 100644
--- a/drivers/net/usb/ax88179_178a.c
+++ b/drivers/net/usb/ax88179_178a.c
@@ -1029,20 +1029,12 @@ static int ax88179_bind(struct usbnet *dev, struct usb_interface *intf)
 	dev->mii.phy_id = 0x03;
 	dev->mii.supports_gmii = 1;
 
-	if (usb_device_no_sg_constraint(dev->udev))
-		dev->can_dma_sg = 1;
-
 	dev->net->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
 			      NETIF_F_RXCSUM;
 
 	dev->net->hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
 				 NETIF_F_RXCSUM;
 
-	if (dev->can_dma_sg) {
-		dev->net->features |= NETIF_F_SG | NETIF_F_TSO;
-		dev->net->hw_features |= NETIF_F_SG | NETIF_F_TSO;
-	}
-
 	/* Enable checksum offload */
 	*tmp = AX_RXCOE_IP | AX_RXCOE_TCP | AX_RXCOE_UDP |
 	       AX_RXCOE_TCPV6 | AX_RXCOE_UDPV6;
-- 
1.8.1.2

RE: [PATCH 2/2] Revert "USBNET: ax88179_178a: enable tso if usb host supports sg dma"

[David Laight]

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

RE: [PATCH 2/2] Revert "USBNET: ax88179_178a: enable tso if usb host supports sg dma"

[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.

Alan Stern

RE: [PATCH 2/2] Revert "USBNET: ax88179_178a: enable tso if usb host supports sg dma"

[David Laight]

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

RE: [PATCH 2/2] Revert "USBNET: ax88179_178a: enable tso if usb host supports sg dma"

[Alan Stern]

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

RE: [PATCH 2/2] Revert "USBNET: ax88179_178a: enable tso if usb host supports sg dma"

[David Laight]

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