Re: [PATCH 2/2] usb: xhci: set page size to the xHCI-supported size

[Mathias Nyman <mathias.nyman@linux.intel.com>]

On 13.1.2025 12.16, Michał Pecio wrote:
> Hi,
> 
> On Fri, 10 Jan 2025 14:35:50 +0200, Mathias Nyman wrote:
>> On 8.1.2025 16.28, Niklas Neronin wrote:
>>> +	page_shift = readl(&xhci->op_regs->page_size) &
>>> XHCI_PAGE_SIZE_MASK;
>>
>> Should we check that page_shift value makes sense here?
> 
> Maybe it would make sense to validate it. Interpreting PAGESIZE wrong
> is potentially dangerous, because the xHC will assume that scratchpad
> buffers are of this size and it can write to them whatever it wants.
> 
> Before the buggy ffs() patch 81720ec5320c, the driver used to pick the
> lowest set bit or warn if all are zero, but then it still ignored the
> calculated size and used 4K.
> 
> I would probably be safer to use the highest bit, or just reject the
> xHC if it sets multiple bits (5.4.3 says: "the supported page size",
> not "a bitmask of supported sizes").

Checking that one, and only one bit is set sounds good. If so then use
that. Otherwise print a warning and use 4k page size.

This to avoid regression. I don't know why the page size was hardcoded
to 4k originally even if we first do all the gymnastics to read it from
hardware. But it was done for some reason.

> 
> 0xffffffff looks like a brain dead chip and not going to work anyway.

Reading 0xffffffff from a PCI device mmio registers is possible
if host is suddenly PCI hotplug removed (some Intel Alpine Ridge xHC), or
if I remember correctly also in PCI D3Cold power state.

> 
>> We used to hardcode page_size to 4k, and don't really know if all xHC
>> vendors have a sane op_regs->page_size value.
> 
> FWIW, all of mine report 4K as per debugfs:
> 
> /sys/kernel/debug/usb/xhci/0000:00:10.0/reg-op:PAGESIZE = 0x00000001
> /sys/kernel/debug/usb/xhci/0000:02:00.0/reg-op:PAGESIZE = 0x00000001
> ...
> 
> 00:10.0 USB controller: Advanced Micro Devices, Inc. [AMD] FCH USB XHCI Controller (rev 20)
> 02:00.0 USB controller: Advanced Micro Devices, Inc. [AMD] 300 Series Chipset USB 3.1 xHCI Controller (rev 02)
> 06:00.0 USB controller: ASMedia Technology Inc. ASM1142 USB 3.1 Host Controller
> 09:00.0 USB controller: Renesas Electronics Corp. uPD720202 USB 3.0 Host Controller (rev 02)
> 0a:00.0 USB controller: Etron Technology, Inc. EJ168 USB 3.0 Host Controller (rev 01)
> 0b:00.0 USB controller: NEC Corporation uPD720200 USB 3.0 Host Controller (rev 03)
> 0c:00.0 USB controller: VIA Technologies, Inc. VL805/806 xHCI USB 3.0 Controller (rev 01)
> 
> Also ASM1042 and ASM3142.
> 
> And I have an NVIDIA Tegra board which runs some antique kernel and
> doesn't warn, so PAGESIZE must at least be non-zero there.

Thanks, good to know these hosts work fine.

Thanks
Mathias