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

From: David Laight
Date: Fri Mar 07 2014 - 12:05:20 EST


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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/