On Wed, Jan 22, 2014 at 09:46:58AM +0000, David Laight wrote:
> From: Sarah Sharp
> > On Tue, Jan 21, 2014 at 12:02:56PM +0000, David Laight wrote:
> > > Don't trace short receives if URB_SHORT_NOT_OK is set.
> > > Short receives are normal for USB ethernet devices.
> > >
> > > Don't trace unexpected incomplete receives if XHCI_TRUST_TX_LENGTH is set.
> > > Ratelimit the trace.
> >
> > Your patch does more than what you wrote here.
> >
> > > Signed-off-by: David Laight <david.laight@xxxxxxxxxx>
> > > ---
> > > If these two traces ever happen, then they will happen for every receive
> > > packet when using USB ethernet.
> > > If you need to enable the xhci_warn or xhci_dgb traces you don't want to
> > > be spammed with trace (syslogd will soon fill the disk).
> > >
> > > These patches won't apply to 3.12 because the trace texts have changed,
> > > however 3.12 also needs a kernel recompile to enable the traces and
> > > anyone doing that can probably manage to patch them out.
> >
> > This is a cleanup, so it won't go into 3.12. Only bug fixes get
> > backported to the stable kernels. The messages are annoying, but they
> > don't trigger a bug. People can work around them by turning off
> > CONFIG_USB_DEBUG.
>
> With these traces you really can't run USB ethernet with USB_DEBUG enabled.
> 3.12 probably requires a recompilation to get USB_DEBUG enabled (and I hope
> none of the main distributions enable it by default), so it may not be
> that important.
>
> If 3.13 has the dynamic debug code in it then these traces really need removing.
Dynamic debug is off by default in 3.13, even with CONFIG_USB_DEBUG
turned on.
> > > Changes for v2:
> > > Fixed so that it applies to Linus's current tree.
> > >
> > > drivers/usb/host/xhci-ring.c | 38 ++++++++++++++++++--------------------
> > > 1 file changed, 18 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> > > index a0b248c..0b3dd16 100644
> > > --- a/drivers/usb/host/xhci-ring.c
> > > +++ b/drivers/usb/host/xhci-ring.c
> > > @@ -2301,36 +2301,34 @@ static int process_bulk_intr_td(struct xhci_hcd *xhci, struct xhci_td *td,
> > > switch (trb_comp_code) {
> > > case COMP_SUCCESS:
> > > /* Double check that the HW transferred everything. */
> > > - if (event_trb != td->last_trb ||
> > > - EVENT_TRB_LEN(le32_to_cpu(event->transfer_len)) != 0) {
> > > - xhci_warn(xhci, "WARN Successful completion "
> > > - "on short TX\n");
> > > - if (td->urb->transfer_flags & URB_SHORT_NOT_OK)
> > > - *status = -EREMOTEIO;
> > > - else
> > > - *status = 0;
> > > - if ((xhci->quirks & XHCI_TRUST_TX_LENGTH))
> > > - trb_comp_code = COMP_SHORT_TX;
> > > - } else {
> > > + if (event_trb == td->last_trb &&
> > > + EVENT_TRB_LEN(le32_to_cpu(event->transfer_len)) == 0) {
> > > *status = 0;
> > > + break;
> >
> > Your patch changes the behavior of the code here, for when the status
> > variable is set to either zero or -EREMOTEIO. The code is hard to
> > reason about, so this really needs to be a separate patch from the one
> > that removes the traces. Please send a patchset with two patches: one
> > to remove the trace, and another to clean up setting *status, in
> > whichever order makes the code clearest in the *status patch.
>
> I've not deliberately changed the behaviour.
> I just inverted the first test so that that the test for URB_SHORT_NOT_OK
> was only done once.
You don't seem to understand that a patch description is a proof of
correctness. I need to verify, from just looking at the patch, that it
does not change behavior. E.g. "*status is set to -EREMOTEIO by foo
function, so it's safe to remove the assignment here" If there's
information I can't see by simply looking at the patch chunks, please
explain that.
> > > }
> > > - break;
> > > + if (xhci->quirks & XHCI_TRUST_TX_LENGTH)
> > > + trb_comp_code = COMP_SHORT_TX;
> >
> > I don't think changing the trb_comp_code is a good idea. There's a lot
> > of code that relies on it later, and it would take me a bit to figure
> > out if changing it is safe.
>
> That code was always there (look at the patch fragment above).
> It is in the code path where an inbound transfer is shorter than the
> size of the bulk rx buffer but the completion code was COMP_SUCCESS not
> COMP_SHORT_TX - which means that the xhci controller gave the wrong code.
> Any later code will expect the 'short transfer' result.
Again, this explanation needs to be in the patch description.
> I actually think that it should be done unconditionally (ie even if the
> the XHCI_TRUST_TX_LENGTH quirk isn't set). ie the code should either
> trust the completion code (unless the quirk is set) or treat the
> two completion codes as equivalent and check the length.
I'm not sure what you mean by this.
> Do you know why the quirk exists, I'm sure I've searched through the
> entire kernel tree for it and not found where it is set.
Git history is the key here.
sarah@xanatos:~/git/kernels/xhci/drivers/usb/host$ git grep XHCI_TRUST_TX_LENGTH
xhci-pci.c: xhci->quirks |= XHCI_TRUST_TX_LENGTH;
xhci-pci.c: xhci->quirks |= XHCI_TRUST_TX_LENGTH;
xhci-ring.c: if ((xhci->quirks & XHCI_TRUST_TX_LENGTH))
xhci-ring.c: if ((xhci->quirks & XHCI_TRUST_TX_LENGTH))
xhci-ring.c: if (xhci->quirks & XHCI_TRUST_TX_LENGTH)
xhci-ring.c: "WARN Successful completion on short TX: needs XHCI_TRUST_TX_LENGTH quirk?\n");
xhci.h:#define XHCI_TRUST_TX_LENGTH (1 << 10)
sarah@xanatos:~/git/kernels/xhci$ git blame drivers/usb/host/xhci.h
1530bbc6272d9 (Sarah Sharp 2012-05-08 09:22:49 -0700 1562) #define XHCI_TRUST_TX_LENGTH (1 << 10)
3b3db026414bb (Sarah Sharp 2012-05-09 10:55:03 -0700 1563) #define XHCI_LPM_SUPPORT (1 << 11)
sarah@xanatos:~/git/kernels/xhci$ git show 1530bbc6272d9
commit 1530bbc6272d9da1e39ef8e06190d42c13a02733
Author: Sarah Sharp <sarah.a.sharp@xxxxxxxxxxxxxxx>
Date: Tue May 8 09:22:49 2012 -0700
xhci: Add new short TX quirk for Fresco Logic host.
Sergio reported that when he recorded audio from a USB headset mic
plugged into the USB 3.0 port on his ASUS N53SV-DH72, the audio sounded
"robotic". When plugged into the USB 2.0 port under EHCI on the same
laptop, the audio sounded fine. The device is:
Bus 002 Device 004: ID 046d:0a0c Logitech, Inc. Clear Chat Comfort USB Headset
The problem was tracked down to the Fresco Logic xHCI host controller
not correctly reporting short transfers on isochronous IN endpoints.
The driver would submit a 96 byte transfer, the device would only send
88 or 90 bytes, and the xHCI host would report the transfer had a
"successful" completion code, with an untransferred buffer length of 8
or 6 bytes.
The successful completion code and non-zero untransferred length is a
contradiction. The xHCI host is supposed to only mark a transfer as
successful if all the bytes are transferred. Otherwise, the transfer
should be marked with a short packet completion code. Without the EHCI
bus trace, we wouldn't know whether the xHCI driver should trust the
completion code or the untransferred length. With it, we know to trust
the untransferred length.
Add a new xHCI quirk for the Fresco Logic host controller. If a
transfer is reported as successful, but the untransferred length is
non-zero, print a warning. For the Fresco Logic host, change the
completion code to COMP_SHORT_TX and process the transfer like a short
transfer.
This should be backported to stable kernels that contain the commit
f5182b4155b9d686c5540a6822486400e34ddd98 "xhci: Disable MSI for some
Fresco Logic hosts." That commit was marked for stable kernels as old
as 2.6.36.
Signed-off-by: Sarah Sharp <sarah.a.sharp@xxxxxxxxxxxxxxx>
Reported-by: Sergio Correia <lists@xxxxxxxx>
Tested-by: Sergio Correia <lists@xxxxxxxx>
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Sarah Sharp <sarah.a.sharp@xxxxxxxxxxxxxxx>
Sarah Sharp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
