| Message ID | 20250108142822.649862-3-niklas.neronin@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: xhci: page size improvements | expand |
On 8.1.2025 16.28, Niklas Neronin wrote: > The current xHCI driver does not validate whether a page size of 4096 > bytes is supported. Address the issue by setting the page size to the > value supported by the xHCI controller, as read from the Page Size > register. > > Additionally, this commit removes unnecessary debug messages and instead > prints the supported and used page size once. > > The xHCI controller supports page sizes of (2^{(n+12)}) bytes, where 'n' > is the Page Size Bit. Only one page size is supported, with a maximum > page size of 128 KB. > > Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com> > --- > drivers/usb/host/xhci-mem.c | 28 ++++++++++++---------------- > drivers/usb/host/xhci.h | 8 ++++---- > 2 files changed, 16 insertions(+), 20 deletions(-) > > diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c > index 66584aafc513..6828b75ad77b 100644 > --- a/drivers/usb/host/xhci-mem.c > +++ b/drivers/usb/host/xhci-mem.c > @@ -1953,7 +1953,6 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci) > xhci->interrupters = NULL; > > xhci->page_size = 0; > - xhci->page_shift = 0; > xhci->usb2_rhub.bus_state.bus_suspended = 0; > xhci->usb3_rhub.bus_state.bus_suspended = 0; > } > @@ -2372,6 +2371,16 @@ xhci_create_secondary_interrupter(struct usb_hcd *hcd, unsigned int segs, > } > EXPORT_SYMBOL_GPL(xhci_create_secondary_interrupter); > > +static void xhci_hcd_page_size(struct xhci_hcd *xhci) > +{ > + u32 page_shift; > + > + page_shift = readl(&xhci->op_regs->page_size) & XHCI_PAGE_SIZE_MASK; Should we check that page_shift value makes sense here? 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. Register read might return 0 or 0xffffffff in some power states. To avoid regression we could add: if (!in_range(page_shift, 0x1, 0x8000)) page_shift = 0x1; Otherwise it looks good -Mathias
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"). 0xffffffff looks like a brain dead chip and not going to work anyway. > 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. Regards, Michal
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
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c index 66584aafc513..6828b75ad77b 100644 --- a/drivers/usb/host/xhci-mem.c +++ b/drivers/usb/host/xhci-mem.c @@ -1953,7 +1953,6 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci) xhci->interrupters = NULL; xhci->page_size = 0; - xhci->page_shift = 0; xhci->usb2_rhub.bus_state.bus_suspended = 0; xhci->usb3_rhub.bus_state.bus_suspended = 0; } @@ -2372,6 +2371,16 @@ xhci_create_secondary_interrupter(struct usb_hcd *hcd, unsigned int segs, } EXPORT_SYMBOL_GPL(xhci_create_secondary_interrupter); +static void xhci_hcd_page_size(struct xhci_hcd *xhci) +{ + u32 page_shift; + + page_shift = readl(&xhci->op_regs->page_size) & XHCI_PAGE_SIZE_MASK; + xhci->page_size = page_shift << 12; + xhci_dbg_trace(xhci, trace_xhci_dbg_init, "HCD page size set to %iK", + xhci->page_size >> 10); +} + int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags) { struct xhci_interrupter *ir; @@ -2379,7 +2388,7 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags) dma_addr_t dma; unsigned int val, val2; u64 val_64; - u32 page_size, temp; + u32 temp; int i; INIT_LIST_HEAD(&xhci->cmd_list); @@ -2388,20 +2397,7 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags) INIT_DELAYED_WORK(&xhci->cmd_timer, xhci_handle_command_timeout); init_completion(&xhci->cmd_ring_stop_completion); - page_size = readl(&xhci->op_regs->page_size); - xhci_dbg_trace(xhci, trace_xhci_dbg_init, - "Supported page size register = 0x%x", page_size); - i = ffs(page_size) - 1; - if (i < 16) - xhci_dbg_trace(xhci, trace_xhci_dbg_init, - "Supported page size of %iK", (1 << (i+12)) / 1024); - else - xhci_warn(xhci, "WARN: no supported page size\n"); - /* Use 4K pages, since that's common and the minimum the HC supports */ - xhci->page_shift = 12; - xhci->page_size = 1 << xhci->page_shift; - xhci_dbg_trace(xhci, trace_xhci_dbg_init, - "HCD page size set to %iK", xhci->page_size / 1024); + xhci_hcd_page_size(xhci); /* * Program the Number of Device Slots Enabled field in the CONFIG diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h index 4914f0a10cff..238cf54ac59f 100644 --- a/drivers/usb/host/xhci.h +++ b/drivers/usb/host/xhci.h @@ -211,6 +211,9 @@ struct xhci_op_regs { #define CONFIG_CIE (1 << 9) /* bits 10:31 - reserved and should be preserved */ +/* bits 15:0 - HCD page shift bit */ +#define XHCI_PAGE_SIZE_MASK 0xffff + /** * struct xhci_intr_reg - Interrupt Register Set * @irq_pending: IMAN - Interrupt Management Register. Used to enable @@ -1510,10 +1513,7 @@ struct xhci_hcd { u16 max_interrupters; /* imod_interval in ns (I * 250ns) */ u32 imod_interval; - /* 4KB min, 128MB max */ - int page_size; - /* Valid values are 12 to 20, inclusive */ - int page_shift; + u32 page_size; /* MSI-X/MSI vectors */ int nvecs; /* optional clocks */
The current xHCI driver does not validate whether a page size of 4096 bytes is supported. Address the issue by setting the page size to the value supported by the xHCI controller, as read from the Page Size register. Additionally, this commit removes unnecessary debug messages and instead prints the supported and used page size once. The xHCI controller supports page sizes of (2^{(n+12)}) bytes, where 'n' is the Page Size Bit. Only one page size is supported, with a maximum page size of 128 KB. Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com> --- drivers/usb/host/xhci-mem.c | 28 ++++++++++++---------------- drivers/usb/host/xhci.h | 8 ++++---- 2 files changed, 16 insertions(+), 20 deletions(-)