On Tue, May 08, 2012 at 12:43:24PM -0400, Sergio Correia wrote:
> Hi,
>
> On Tue, May 8, 2012 at 12:25 PM, Sarah Sharp
> <sarah.a.sharp@xxxxxxxxxxxxxxx> wrote:
> > On Tue, May 08, 2012 at 10:56:59AM -0400, Alan Stern wrote:
> >> On Tue, 8 May 2012, Sergio Correia wrote:
> >>
> >> > the output from USBmon for both EHCI and xHCI is available at:
> >> > http://www.uece.net/misc/usb-headset-problem/usbmon-usb2-bus2-dev6.txt
> >> > http://www.uece.net/misc/usb-headset-problem/usbmon-usb3-bus3-dev5.txt
> >>
> >> It's notable that the bus-2 trace (which I presume is the one using
> >> EHCI) shows that even though each packet requested 96 bytes of data,
> >> the device sent only 88 or 90 bytes. By contrast, the bus-3 trace
> >> (xHCI) shows that each packet contained 96 bytes. We should expect the
> >> two traces not to differ in this way.
> >>
> >> This may be a bug in the xhci-hcd driver -- it may say it got more
> >> bytes than actually were received.
> >
> > I actually suspect it might be a bug in the hardware itself. The lspci
> > shows it's a very early Fresco Logic chipset revision, and it wouldn't
> > surprise me if the xHCI host just doesn't report the short packet
> > completion code. Let's rule that out before I go digging around the
> > xHCI ring code (which looks like it's handling short packets properly,
> > BTW).
> >
> > Sergio, can you apply the attached patch, and see if the host is
> > actually reporting short packets?
>
> the dmesg is there:
> http://www.uece.net/misc/usb-headset-problem/dmesg-short-packet-completion-code.bz2
> it was too large, so I bzip'ed it.
Yeah, it looks like the Fresco Logic host controller is reporting a
successful completion status, with an untransferred length of 8 bytes,
which is a contradiction. Since we can tell from the EHCI logs that it
actually should be reporting a short packet status, I think we should be
able to just add a quirk for it, to trust the untransferred length.
I wonder if this is just for short control transfers, or if they have
this bug for other endpoint types?
Please revert the last patch, apply the next patch, and see if the audio
sounds any better.
How did you get access to this xHCI host controller? AFAIK, it was a
short run chip that Fresco Logic did just for the USB-IF PDK, back in
2008. Is it an off-the-shelf add-in card, or integrated in some system?
Sarah Sharp
>From 899ea187347c57e67686581bc8d8a4c0e49e1719 Mon Sep 17 00:00:00 2001
From: Sarah Sharp <sarah.a.sharp@xxxxxxxxxxxxxxx>
Date: Tue, 8 May 2012 09:22:49 -0700
Subject: [PATCH] xhci: Add new short TX quirk for Fresco Logic PDK.
Signed-off-by: Sarah Sharp <sarah.a.sharp@xxxxxxxxxxxxxxx>
---
drivers/usb/host/xhci-pci.c | 1 +
drivers/usb/host/xhci-ring.c | 20 +++++++++++++++++---
drivers/usb/host/xhci.h | 1 +
3 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 7a856a7..3c99b1f 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -72,6 +72,7 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
xhci_dbg(xhci, "QUIRK: Fresco Logic revision %u "
"has broken MSI implementation\n",
pdev->revision);
+ xhci->quirks |= XHCI_CHECK_SUCCESS;
}
if (pdev->vendor == PCI_VENDOR_ID_NEC)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 3d9422f..951a9c3 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1786,8 +1786,12 @@ static int process_isoc_td(struct xhci_hcd *xhci, struct xhci_td *td,
/* handle completion code */
switch (trb_comp_code) {
case COMP_SUCCESS:
- frame->status = 0;
- break;
+ if (TRB_LEN(le32_to_cpu(event->transfer_len)) == 0) {
+ frame->status = 0;
+ break;
+ }
+ if ((xhci->quirks & XHCI_TRUST_TX_LENGTH))
+ trb_comp_code = COMP_SHORT_TX;
case COMP_SHORT_TX:
frame->status = td->urb->transfer_flags & URB_SHORT_NOT_OK ?
-EREMOTEIO : 0;
@@ -1883,13 +1887,16 @@ 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) {
+ if (event_trb != td->last_trb ||
+ 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 {
*status = 0;
}
@@ -2048,6 +2055,13 @@ static int handle_tx_event(struct xhci_hcd *xhci,
* transfer type
*/
case COMP_SUCCESS:
+ if (TRB_LEN(le32_to_cpu(event->transfer_len)) == 0)
+ break;
+ if (xhci->quirks & XHCI_TRUST_TX_LENGTH)
+ trb_comp_code = COMP_SHORT_TX;
+ else
+ xhci_warn(xhci, "WARN Successful completion on short TX: "
+ "needs XHCI_TRUST_TX_LENGTH quirk?\n");
case COMP_SHORT_TX:
break;
case COMP_STOP:
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 3d69c4b..abfb321 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1479,6 +1479,7 @@ struct xhci_hcd {
#define XHCI_RESET_ON_RESUME (1 << 7)
#define XHCI_SW_BW_CHECKING (1 << 8)
#define XHCI_AMD_0x96_HOST (1 << 9)
+#define XHCI_TRUST_TX_LENGTH (1 << 10)
unsigned int num_active_eps;
unsigned int limit_active_eps;
/* There are two roothubs to keep track of bus suspend info for */
--
1.7.9
