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 will 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.
> 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.
> }
> - 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.
Sarah Sharp
> + else
> + xhci_warn_ratelimited(xhci,
> + "WARN Successful completion on short TX\n");
> + /* FALLTHROUGH */
> case COMP_SHORT_TX:
> - if (td->urb->transfer_flags & URB_SHORT_NOT_OK)
> + if (td->urb->transfer_flags & URB_SHORT_NOT_OK) {
> + xhci_dbg(xhci,
> + "ep %#x - asked for %d bytes, %d bytes untransferred\n",
> + td->urb->ep->desc.bEndpointAddress,
> + td->urb->transfer_buffer_length,
> + EVENT_TRB_LEN(le32_to_cpu(event->transfer_len)));
> *status = -EREMOTEIO;
> - else
> + } else {
> *status = 0;
> + }
> break;
> default:
> /* Others already handled above */
> break;
> }
> - if (trb_comp_code == COMP_SHORT_TX)
> - xhci_dbg(xhci, "ep %#x - asked for %d bytes, "
> - "%d bytes untransferred\n",
> - td->urb->ep->desc.bEndpointAddress,
> - td->urb->transfer_buffer_length,
> - EVENT_TRB_LEN(le32_to_cpu(event->transfer_len)));
> +
> /* Fast path - was this the last TRB in the TD for this URB? */
> if (event_trb == td->last_trb) {
> if (EVENT_TRB_LEN(le32_to_cpu(event->transfer_len)) != 0) {
> --
> 1.8.1.2
>
>
>
--
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
