public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: "Michał Pecio" <michal.pecio@gmail.com>
To: ki.chiang65@gmail.com
Cc: gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org,
	linux-usb@vger.kernel.org, mathias.nyman@intel.com
Subject: Re: [PATCH v2 5/5] xhci: Correct handling of one-TRB isoc TD on Etron xHCI host
Date: Mon, 28 Oct 2024 10:54:51 +0100	[thread overview]
Message-ID: <20241028105451.0e2e92a7@foxbook> (raw)
In-Reply-To: <20241028025337.6372-6-ki.chiang65@gmail.com>

Hi,

That's a bug I'm familiar with.

> Unplugging a USB3.0 webcam while streaming results in errors
> like this

Not only unplugging but also any random error due to EMI or bad cable.

> If an error is detected while processing an one-TRB isoc TD,
> the Etron xHC generates two transfer events for the TRB that
> the error was detected on. The first event is "USB Transcation
> Error", and the second event is "Success".

IIRC, it wasn't just Transaction Errors but any sort of error, like
Babble or Bandwidth Overrun. But not sure about Missed Service, etc.

And IIRC I confirmed that it was *not* the case on Short Packet.

Also, I'm 99% sure the problem is not limited to one-TRB TDs, but
it occurs every time there is an error on the last TRB of any TD.

> As a solution, we can set the flag after the first error event
> and don't print the error message after the second event if the
> flag is set.

Yes, but I think it would be better to use error_mid_td instead of
last_td_was_short, so that the TD is only freed on the final event,
not on the first one.

The spec is clear that we should only free TRBs when the xHC is done
with them. Maybe it wouldn't be a problem in this case, and it surely
wouldn't be worse than what happens with Etron today, but IMO it could
be a real (even if rare) problem in other cases when this flag is used,
so I would rather remove the flag and handle short packets as per spec.

Regards,
Michal

  reply	other threads:[~2024-10-28  9:54 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-10-28  2:53 [PATCH v2 0/5] xhci: Some improvement for Etron xHCI host Kuangyi Chiang
2024-10-28  2:53 ` [PATCH v2 1/5] xhci: Combine two if statements " Kuangyi Chiang
2024-10-30 12:04   ` Mathias Nyman
2024-11-01  2:30     ` Kuangyi Chiang
2024-11-01 12:57       ` Mathias Nyman
2024-10-28  2:53 ` [PATCH v2 2/5] xhci: Don't issue Reset Device command to " Kuangyi Chiang
2024-10-30 12:58   ` Mathias Nyman
2024-10-28  2:53 ` [PATCH v2 3/5] xhci: Fix control transfer error on " Kuangyi Chiang
2024-10-28  2:53 ` [PATCH v2 4/5] xhci: Don't perform Soft Retry for " Kuangyi Chiang
2024-10-28  2:53 ` [PATCH v2 5/5] xhci: Correct handling of one-TRB isoc TD on " Kuangyi Chiang
2024-10-28  9:54   ` Michał Pecio [this message]
2024-10-30  5:17     ` Kuangyi Chiang
2024-10-30 13:50   ` Mathias Nyman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20241028105451.0e2e92a7@foxbook \
    --to=michal.pecio@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=ki.chiang65@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=mathias.nyman@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox