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.
> > 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.
>
> > }
> > - 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.
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.
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.
David
--
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
