[PATCH v2 0/2] usb: xhci: page size improvements

[PATCH v2 0/2] usb: xhci: page size improvements

[Niklas Neronin]

Correct off-by-one page size debug message.
Set page size to the xHCI-supported size, instead of 4KB.

Clarified the interpretation of the xHCI spec 1.9 regarding page size.
The correct interpretation is that only one bit is set, indicating the
only supported page size. This is supported by the following sources:

Section 6.6.1, PSZ:
  The PSZ calculation uses the page size bit and would not work with
  multiple bits set.

Section 7.7, Implementation Notes:
  "This version of the xHCI spec only allows an implementation to support
   a single page size, as reported by the PAGESIZE register."


Version 2 changes:
* Added handling for invalid page size register values.

Niklas Neronin (2):
  usb: xhci: correct debug message page size calculation
  usb: xhci: set page size to the xHCI-supported size

 drivers/usb/host/xhci-mem.c | 34 ++++++++++++++++++----------------
 drivers/usb/host/xhci.h     |  8 ++++----
 2 files changed, 22 insertions(+), 20 deletions(-)

-- 
2.45.2

[PATCH v2 1/2] usb: xhci: correct debug message page size calculation

[Niklas Neronin]

The ffs() function returns the index of the first set bit, starting from 1.
If no bits are set, it returns zero. This behavior causes an off-by-one
page size in the debug message, as the page size calculation [1]
is zero-based, while ffs() is one-based.

Fix this by subtracting one from the result of ffs(). Note that since
variable 'i' is unsigned, subtracting one from zero will result in the
maximum unsigned integer value. Consequently, the condition 'if (i < 16)'
will still function correctly.

[1], Page size: (2^(n+12)), where 'n' is the set page size bit.

Fixes: 81720ec5320c ("usb: host: xhci: use ffs() in xhci_mem_init()")
Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
 drivers/usb/host/xhci-mem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 92703efda1f7..66584aafc513 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -2391,7 +2391,7 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
 	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);
+	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);
-- 
2.45.2

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

[Niklas Neronin]

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. In the event of an unexpected value; default to a 4K page size.

Additionally, this commit removes unnecessary debug messages and instead
prints the used page size once, and a warning in case of unexpected value.

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>
---
v2:
 Added handling for invlalid page size register value.

 drivers/usb/host/xhci-mem.c | 34 ++++++++++++++++++----------------
 drivers/usb/host/xhci.h     |  8 ++++----
 2 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 66584aafc513..f1243017effa 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,22 @@ 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;
+	if (!page_shift || page_shift & (page_shift - 1)) {
+		xhci_warn(xhci, "Invalid page size register = 0x%x\n", page_shift);
+		/* Fallback to 4K page size, since that's common */
+		page_shift = 1;
+	}
+
+	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 +2394,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 +2403,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 */
-- 
2.45.2

Re: [PATCH v2 1/2] usb: xhci: correct debug message page size calculation

[Michał Pecio]

On Mon, 13 Jan 2025 16:30:36 +0200, Niklas Neronin wrote:
> Fix this by subtracting one from the result of ffs(). Note that since
> variable 'i' is unsigned, subtracting one from zero will result in the
> maximum unsigned integer value. Consequently, the condition 'if (i <
> 16)' will still function correctly.

Is it unsigned? I see a simple 'int' here, so this wouldn't work.

Michal

Re: [PATCH v2 1/2] usb: xhci: correct debug message page size calculation

[Neronin, Niklas]

On 13/01/2025 18.00, Michał Pecio wrote:
> On Mon, 13 Jan 2025 16:30:36 +0200, Niklas Neronin wrote:
>> Fix this by subtracting one from the result of ffs(). Note that since
>> variable 'i' is unsigned, subtracting one from zero will result in the
>> maximum unsigned integer value. Consequently, the condition 'if (i <
>> 16)' will still function correctly.
> 
> Is it unsigned? I see a simple 'int' here, so this wouldn't work.
> 

You are correct, it is not unsigned.
Thank you for catching it, don't know how I missed it.

Best Regards,
Niklas Neronin

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

[Michał Pecio]

On Mon, 13 Jan 2025 16:30:37 +0200, Niklas Neronin wrote:
> +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;

If there is a v3 of this series coming for other reasons, I think it
would also make sense to rename this to simply 'page_size' or something
like that.

Shift suggests that it's the logarithm of page size, but it is not.

Regards,
Michal