Message-ID: <CAPwXWsCuMDTYwfSodKsrV1MZCtmLrM380ziQFdhcpTt9r+_gxQ@mail.gmail.com>
Date: Mon, 19 May 2025 23:47:27 +0800
From: 陳子潔 <shawn2000100@...il.com>
To: Mathias Nyman <mathias.nyman@...ux.intel.com>, Michał Pecio <michal.pecio@...il.com>
Cc: mathias.nyman@...el.com, gregkh@...uxfoundation.org, 
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org, jay.chen@...mens.com
Subject: Re: [PATCH v3] usb: xhci: Set avg_trb_len = 8 for EP0 during Address
 Device Command

Hi Michał and Mathias,

Thank you for the thoughtful feedback and review.

> Only the xHC internal firmware could crash or misbehave from that.

Yes, precisely. The potential crash or division-by-zero would happen
within the internal firmware of certain host controllers, rather than
in the Linux kernel itself.
I'll make this clearer in the commit description as well.

> The rest of ep0 tx_info is zero, so this could be = instead of |=.

Agreed, I'll update the next revision of this patch to use '=' instead
of '|=' for clarity and correctness.

---

> I'd skip the 'compliance with spec..' part as spec is a bit unclear on this
issue.

Sure, I will modify the commit message accordingly.

As noted, there is a subtle contradiction between two sections of the
xHCI 1.2 specification:
- Section 4.8.2 "Endpoint Context Initialization" states that all
fields of an Input Endpoint Context data structure (including the
Reserved fields) shall be initialized to 0.
- Section 6.2.3 "Endpoint Context" (p.453) specifies that the Average
TRB Length field shall be greater than ‘0’, and further notes (p.454):
“Software shall set Average TRB Length to ‘8’ for control endpoints.”

Strictly setting all fields to 0 during initialization conflicts with
the explicit recommendation that avg_trb_len should be set to 8 for
control endpoints. In practice, setting avg_trb_len = 0 is meaningless
for the hardware/firmware, as it defeats the purpose of the field,
which is used for transfer calculations and validation.

Motivation / Real-world context:
I am developing a custom Virtual xHC hardware platform that strictly
follows the xHCI specification and its recommendations.
During validation, we found that enumeration fails and a parameter
error (TRB Completion Code = 5) is reported if avg_trb_len for EP0 is
not set to 8 as recommended by Section 6.2.3.
This behavior aligns with the spec's intent and highlights the
importance of setting a meaningful, non-zero value for avg_trb_len,
even in virtualized or emulated environments.

Therefore, this patch is intended to better align Linux xHCI host
controller driver behavior with the recommendation in Section 6.2.3,
and to improve robustness and interoperability with both current and
future xHCI implementations—real or virtual—that may enforce spec
recommendations more strictly.

Thanks again for your feedback. I'll prepare and submit a v4 patch
shortly, incorporating your suggestions.
Let me know if you have further questions or suggestions.

Best regards,
Jay Chen

On Mon, May 19, 2025 at 9:14 PM Mathias Nyman
<mathias.nyman@...ux.intel.com> wrote:
>
> On 16.5.2025 6.39, Jay Chen wrote:
> > According to the xHCI 1.2 spec (Section 6.2.3, p.454), the Average
> > TRB Length (avg_trb_len) for control endpoints should be set to 8.
>
> Maybe add here "But section 4.8.2 "Endpoint Context Initialization"
> states that all fields of an Input Endpoint Context data structure
> (including the Reserved fields) shall be initialized to 0
> > > Currently, during the Address Device Command, EP0's avg_trb_len remains 0,
> > which may cause some xHCI hardware to reject the Input Context, resulting
> > in device enumeration failures. In extreme cases, using a zero avg_trb_len
> > in calculations may lead to division-by-zero errors and unexpected system
> > crashes.
>
> Would be good to specify here which exact hardware requires avg_trb_len to be
> set before the 'Address Device Command'. This way we can later create a
> quirk for it in case it turns out other existing controllers can't handle it.
>
> So far it seems other hosts can handle it well, and quirks may not be needed
> at all. Thanks to Michał for testing.
>
> Thanks
> Mathias
>
> >
> > This patch sets avg_trb_len to 8 for EP0 in
> > xhci_setup_addressable_virt_dev(), ensuring compliance with the spec
> > and improving compatibility across various host controller implementations.
>
> I'd skip the 'compliance with spec..' part as spec is a bit unclear on this
> issue.
>
> Thanks
> Mathias
>

Powered by blists - more mailing lists