Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
From: Mathias Nyman <mathias.nyman@linux.intel.com>
To: Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
	Felipe Balbi <balbi@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	Mathias Nyman <mathias.nyman@intel.com>
Cc: John Youn <John.Youn@synopsys.com>
Subject: Re: [PATCH v2 3/7] usb: xhci: Check for blocked disconnection
Date: Wed, 28 Apr 2021 16:32:15 +0300
Message-ID: <aa64a81c-8e60-f933-5d17-f511e47507f7@linux.intel.com> (raw)
In-Reply-To: <b5c8bfc8-20cf-fad1-f053-971cb98348ad@synopsys.com>

On 28.4.2021 1.30, Thinh Nguyen wrote:
>> On 10.4.2021 3.47, Thinh Nguyen wrote:
>>> If there is a device with active enhanced super-speed (eSS) isoc IN
>>> endpoint(s) behind one or more eSS hubs, DWC_usb31 (v1.90a and prior)
>>> host controller will not detect the device disconnection until no more
>>> isoc URB is submitted. If there's a device disconnection, internally
>>> the wait for tHostTransactionTimeout (USB 3.2 spec 8.13) blocks the
>>> other endpoints from being scheduled. So, it blocks the interrupt
>>> endpoint of the eSS hub indicating the port change status.
>>>
>>> This can be an issue for applications that continuously submitting isoc
>>> URBs to the xHCI. To work around this, stop processing new URBs after 3
>>> consecutive isoc transaction errors. If new isoc transfers are queued
>>> after the device is disconnected, the host will respond with USB
>>> transaction error. After 3 consecutive USB transaction errors, the
>>> driver can wait a period of time (at least 2 * largest periodic interval
>>> of the topology) without ringing isoc endpoint doorbell to detect the
>>> port change status. If there is no disconnection detected, ring the
>>> endpoint doorbell to resume isoc transfers.
>>
>> Is that enough? many Isoc URBs queue 16 - 32 Isoc TRBs per URB.
>> And drivers like UVC queue several URBs in advance.
> 
> That's fine as long as the driver stops ringing more doorbell for a
> certain period of time creating a gap that's enough to get the
> notification from the interrupt endpoint. We tested with 128 isoc URBs
> and was able to detect a disconnect after this delay.

Ok, if not ringing the endpoint is enough then that is better than
stopping the whole endpoint. 

>>> This workaround tracks the max eSS periodic interval every time there's
>>> an endpoint added or dropped, which happens when there's bandwidth
>>> check. So, scan the topology and update the xhci->max_ess_interval
>>> whenever there's a bandwidth check. Introduced a new flag
>>> VDEV_DISCONN_CHECK_PENDING to prevent ringing the doorbell while waiting
>>> for a disconnection status. After 2 * max_ess_interval time and no
>>> disconnection detected, a delayed work will ring the doorbell to resume
>>> the active isoc transfers.
>>
>> Sounds very elaborate for a vendor specific disconnect workaround.
>> Isn't there a simpler way?
>>
>> Maybe stop all isoc in endpoints if one them has 3 consecutive transaction error,
>> wait for 2x hub interrupt interval time, and then restart the endpoints if there is
>> no disconnect?
> 
> We can also do this (but without stop + restart the endpoints). It just
> creates a slightly larger gap that may be more noticeable to the user if
> there's no actual disconnection.

Ok, if blocking the doorbell is enough then it sounds better.

How about that max interval tracking, is it necessary?
It will block the doorbell from 250us up to several seconds.
Is there some reasonable single value that can be used instead?

>>
>> There is bigger concern with this series, it scatters a lot of vendor specific code 
>> around the generic xhci driver. It's not very clear afterwards what code is part of the
>> workaround and what is generic code.
>>
>> We just got a lot of the Mediatek code moved to xhci-mtk*, maybe its time to add xhci-snps.c
>> instead of using the generic platform driver with tons of workarounds and quirks.
>>
> 
> Thanks for the reviews. I need to look into how this can be done. May
> need your suggestion as not every scenarios can be overridden
> easily/cleanly.

true, no easy overrides for this transfer error / doorbell blocking workaround.
Needs a bit of work

-Mathias

  reply index

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-09  1:41 [PATCH 0/6] usb: Set quirks for xhci/dwc3 host mode Thinh Nguyen
2021-04-09  1:41 ` [PATCH 1/6] usb: xhci: Move quirks definitions to common usb header Thinh Nguyen
2021-04-09  6:50   ` Greg Kroah-Hartman
2021-04-09  8:01     ` Thinh Nguyen
2021-04-09 16:26   ` kernel test robot
2021-04-09 19:54   ` kernel test robot
2021-04-09  1:42 ` [PATCH 2/6] usb: xhci: Check for blocked disconnection Thinh Nguyen
2021-04-09  1:42 ` [PATCH 3/6] usb: xhci: Workaround undercalculated BW for fullspeed BI Thinh Nguyen
2021-04-09  1:42 ` [PATCH 4/6] usb: xhci: Rename Compliance mode timer quirk Thinh Nguyen
2021-04-09  1:42 ` [PATCH 5/6] usb: xhci: Workaround lost disconnect port status Thinh Nguyen
2021-04-09  1:42 ` [PATCH 6/6] usb: dwc3: host: Set quirks base on version Thinh Nguyen
2021-04-09  6:53   ` Greg Kroah-Hartman
2021-04-09  8:01     ` Thinh Nguyen
2021-04-09 13:22       ` Greg Kroah-Hartman
2021-04-10  0:44         ` Thinh Nguyen
2021-04-10  0:46 ` [PATCH v2 0/7] usb: Set quirks for xhci/dwc3 host mode Thinh Nguyen
2021-04-10  0:46   ` [PATCH v2 1/7] usb: xhci: Move quirks definitions to common usb header Thinh Nguyen
2021-04-10  0:46   ` [PATCH v2 2/7] usb: xhci: Move xhci-plat header " Thinh Nguyen
2021-04-10  0:47   ` [PATCH v2 3/7] usb: xhci: Check for blocked disconnection Thinh Nguyen
2021-04-27 13:08     ` Mathias Nyman
2021-04-27 22:30       ` Thinh Nguyen
2021-04-28 13:32         ` Mathias Nyman [this message]
2021-04-29  0:54           ` Thinh Nguyen
2021-04-10  0:47   ` [PATCH v2 4/7] usb: xhci: Workaround undercalculated BW for fullspeed BI Thinh Nguyen
2021-04-28 11:57     ` Mathias Nyman
2021-04-10  0:47   ` [PATCH v2 5/7] usb: xhci: Rename Compliance mode timer quirk Thinh Nguyen
2021-04-10  0:47   ` [PATCH v2 6/7] usb: xhci: Workaround lost disconnect port status Thinh Nguyen
2021-04-28 13:48     ` Mathias Nyman
2021-04-29  1:00       ` Thinh Nguyen
2021-04-10  0:47   ` [PATCH v2 7/7] usb: dwc3: host: Set quirks base on version Thinh Nguyen
2021-04-19 21:07   ` [PATCH v2 0/7] usb: Set quirks for xhci/dwc3 host mode Thinh Nguyen

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=aa64a81c-8e60-f933-5d17-f511e47507f7@linux.intel.com \
    --to=mathias.nyman@linux.intel.com \
    --cc=John.Youn@synopsys.com \
    --cc=Thinh.Nguyen@synopsys.com \
    --cc=balbi@kernel.org \
    --cc=gregkh@linuxfoundation.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

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git