* [PATCH 0/9] usb: xhci: rework USB request parameter handling
@ 2026-03-19 13:56 Niklas Neronin
2026-03-19 13:56 ` [PATCH 1/9] usb: xhci: stop treating 'wIndex' as a mutable port number Niklas Neronin
` (8 more replies)
0 siblings, 9 replies; 11+ messages in thread
From: Niklas Neronin @ 2026-03-19 13:56 UTC (permalink / raw)
To: mathias.nyman; +Cc: linux-usb, Niklas Neronin
Clean up how xhci_hub_control() handles USB hub-class request parameters.
The function receives the raw USB request fields (wValue, wIndex, wLength),
which must be decoded differently depending on the request type. Many of
the decoded values also have direct equivalents inside the xhci driver,
but the code mixes the USB-level representations with the internal xhci
ones.
Standardize the handling by decoding USB request parameters up front and
converting them into the xhci driver's expected internal forms.
Additionally, ensure that the correct macro set is used for the correct
source.
This results in clearer logic, fewer assumptions, and a reduced risk of
mixing USB Chapter 11 values with xhci-specific encodings.
Niklas Neronin (9):
usb: xhci: stop treating 'wIndex' as a mutable port number
usb: xhci: rename 'wIndex' parameters to 'portnum'
usb: xhci: clean up handling of upper bits in SetPortFeature wIndex
usb: xhci: clean up 'wValue' handling in xhci_hub_control()
usb: xhci: separate use of USB Chapter 11 PLS macros from
xHCI-specific PLS macros
usb: xhci: add PORTPMSC variable to xhci_hub_control()
usb: xhci: add PORTSC variable to xhci_hub_control()
usb: xhci: rename parameter to match argument 'portsc'
usb: xhci: cleanup xhci_hub_report_usb3_link_state()
drivers/usb/host/xhci-hub.c | 383 +++++++++++++++++-------------------
1 file changed, 178 insertions(+), 205 deletions(-)
--
2.50.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/9] usb: xhci: stop treating 'wIndex' as a mutable port number
2026-03-19 13:56 [PATCH 0/9] usb: xhci: rework USB request parameter handling Niklas Neronin
@ 2026-03-19 13:56 ` Niklas Neronin
2026-03-19 14:10 ` Oliver Neukum
2026-03-19 13:56 ` [PATCH 2/9] usb: xhci: rename 'wIndex' parameters to 'portnum' Niklas Neronin
` (7 subsequent siblings)
8 siblings, 1 reply; 11+ messages in thread
From: Niklas Neronin @ 2026-03-19 13:56 UTC (permalink / raw)
To: mathias.nyman; +Cc: linux-usb, Niklas Neronin
The USB request parameter 'wIndex' is a 16-bit field whose meaning depends
on the request type. For hub port operations, only bits 7:0 encode the port
number (1..MaxPorts). Despite this, the current code extracts the port
number into 'portnum1' while also modifying and using 'wIndex' directly as
a 0-based port index. This dual use is both confusing and error-prone,
since 'wIndex' is not always a pure port number.
Clean this up by deriving a single 0-based 'portnum' from 'wIndex' and
using it throughout the function. The original 'wIndex' value is no longer
modified or treated as a port number. This also matches existing xhci code.
Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
drivers/usb/host/xhci-hub.c | 65 +++++++++++++++++--------------------
1 file changed, 30 insertions(+), 35 deletions(-)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 04cc3d681495..4730beae2478 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -1218,13 +1218,12 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
struct xhci_hub *rhub;
struct xhci_port **ports;
struct xhci_port *port;
- int portnum1;
+ int portnum;
rhub = xhci_get_rhub(hcd);
ports = rhub->ports;
max_ports = rhub->num_ports;
bus_state = &rhub->bus_state;
- portnum1 = wIndex & 0xff;
spin_lock_irqsave(&xhci->lock, flags);
switch (typeReq) {
@@ -1258,11 +1257,11 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
spin_unlock_irqrestore(&xhci->lock, flags);
return retval;
case GetPortStatus:
- if (!portnum1 || portnum1 > max_ports)
+ portnum = (wIndex & 0xff) - 1;
+ if (!in_range(portnum, 0, max_ports))
goto error;
- wIndex--;
- port = ports[portnum1 - 1];
+ port = ports[portnum];
temp = xhci_portsc_readl(port);
if (temp == ~(u32)0) {
xhci_hc_died(xhci);
@@ -1270,13 +1269,12 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
break;
}
trace_xhci_get_port_status(port, temp);
- status = xhci_get_port_status(hcd, bus_state, wIndex, temp,
- &flags);
+ status = xhci_get_port_status(hcd, bus_state, portnum, temp, &flags);
if (status == 0xffffffff)
goto error;
xhci_dbg(xhci, "Get port status %d-%d read: 0x%x, return 0x%x",
- hcd->self.busnum, portnum1, temp, status);
+ hcd->self.busnum, portnum + 1, temp, status);
put_unaligned(cpu_to_le32(status), (__le32 *) buf);
/* if USB 3.1 extended port status return additional 4 bytes */
@@ -1303,12 +1301,11 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
/* The MSB of wIndex is the U1/U2 timeout */
timeout = (wIndex & 0xff00) >> 8;
- wIndex &= 0xff;
- if (!portnum1 || portnum1 > max_ports)
+ portnum = (wIndex & 0xff) - 1;
+ if (!in_range(portnum, 0, max_ports))
goto error;
- port = ports[portnum1 - 1];
- wIndex--;
+ port = ports[portnum];
temp = xhci_portsc_readl(port);
if (temp == ~(u32)0) {
xhci_hc_died(xhci);
@@ -1335,7 +1332,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
if ((temp & PORT_PE) == 0 || (temp & PORT_RESET)
|| (temp & PORT_PLS_MASK) >= XDEV_U3) {
xhci_warn(xhci, "USB core suspending port %d-%d not in U0/U1/U2\n",
- hcd->self.busnum, portnum1);
+ hcd->self.busnum, portnum + 1);
goto error;
}
@@ -1355,14 +1352,14 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
spin_lock_irqsave(&xhci->lock, flags);
temp = xhci_portsc_readl(port);
- bus_state->suspended_ports |= 1 << wIndex;
+ bus_state->suspended_ports |= 1 << portnum;
break;
case USB_PORT_FEAT_LINK_STATE:
temp = xhci_portsc_readl(port);
/* Disable port */
if (link_state == USB_SS_PORT_LS_SS_DISABLED) {
xhci_dbg(xhci, "Disable port %d-%d\n",
- hcd->self.busnum, portnum1);
+ hcd->self.busnum, portnum + 1);
temp = xhci_port_state_to_neutral(temp);
/*
* Clear all change bits, so that we get a new
@@ -1379,7 +1376,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
/* Put link in RxDetect (enable port) */
if (link_state == USB_SS_PORT_LS_RX_DETECT) {
xhci_dbg(xhci, "Enable port %d-%d\n",
- hcd->self.busnum, portnum1);
+ hcd->self.busnum, portnum + 1);
xhci_set_link_state(xhci, port, link_state);
temp = xhci_portsc_readl(port);
break;
@@ -1411,7 +1408,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
}
xhci_dbg(xhci, "Enable compliance mode transition for port %d-%d\n",
- hcd->self.busnum, portnum1);
+ hcd->self.busnum, portnum + 1);
xhci_set_link_state(xhci, port, link_state);
temp = xhci_portsc_readl(port);
@@ -1425,7 +1422,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
/* Can't set port link state above '3' (U3) */
if (link_state > USB_SS_PORT_LS_U3) {
xhci_warn(xhci, "Cannot set port %d-%d link state %d\n",
- hcd->self.busnum, portnum1, link_state);
+ hcd->self.busnum, portnum + 1, link_state);
goto error;
}
@@ -1460,7 +1457,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
if (!wait_for_completion_timeout(&port->u3exit_done,
msecs_to_jiffies(500)))
xhci_dbg(xhci, "missing U0 port change event for port %d-%d\n",
- hcd->self.busnum, portnum1);
+ hcd->self.busnum, portnum + 1);
spin_lock_irqsave(&xhci->lock, flags);
temp = xhci_portsc_readl(port);
break;
@@ -1486,7 +1483,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
}
spin_lock_irqsave(&xhci->lock, flags);
temp = xhci_portsc_readl(port);
- bus_state->suspended_ports |= 1 << wIndex;
+ bus_state->suspended_ports |= 1 << portnum;
}
break;
case USB_PORT_FEAT_POWER:
@@ -1504,13 +1501,13 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
temp = xhci_portsc_readl(port);
xhci_dbg(xhci, "set port reset, actual port %d-%d status = 0x%x\n",
- hcd->self.busnum, portnum1, temp);
+ hcd->self.busnum, portnum + 1, temp);
break;
case USB_PORT_FEAT_REMOTE_WAKE_MASK:
xhci_set_remote_wake_mask(xhci, port, wake_mask);
temp = xhci_portsc_readl(port);
xhci_dbg(xhci, "set port remote wake mask, actual port %d-%d status = 0x%x\n",
- hcd->self.busnum, portnum1, temp);
+ hcd->self.busnum, portnum + 1, temp);
break;
case USB_PORT_FEAT_BH_PORT_RESET:
temp |= PORT_WR;
@@ -1540,8 +1537,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
if (test_mode > USB_TEST_FORCE_ENABLE ||
test_mode < USB_TEST_J)
goto error;
- retval = xhci_enter_test_mode(xhci, test_mode, wIndex,
- &flags);
+ retval = xhci_enter_test_mode(xhci, test_mode, portnum, &flags);
break;
default:
goto error;
@@ -1550,12 +1546,11 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
temp = xhci_portsc_readl(port);
break;
case ClearPortFeature:
- if (!portnum1 || portnum1 > max_ports)
+ portnum = (wIndex & 0xff) - 1;
+ if (!in_range(portnum, 0, max_ports))
goto error;
- port = ports[portnum1 - 1];
-
- wIndex--;
+ port = ports[portnum];
temp = xhci_portsc_readl(port);
if (temp == ~(u32)0) {
xhci_hc_died(xhci);
@@ -1575,17 +1570,17 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
if ((temp & PORT_PE) == 0)
goto error;
- set_bit(wIndex, &bus_state->resuming_ports);
- usb_hcd_start_port_resume(&hcd->self, wIndex);
+ set_bit(portnum, &bus_state->resuming_ports);
+ usb_hcd_start_port_resume(&hcd->self, portnum);
xhci_set_link_state(xhci, port, XDEV_RESUME);
spin_unlock_irqrestore(&xhci->lock, flags);
msleep(USB_RESUME_TIMEOUT);
spin_lock_irqsave(&xhci->lock, flags);
xhci_set_link_state(xhci, port, XDEV_U0);
- clear_bit(wIndex, &bus_state->resuming_ports);
- usb_hcd_end_port_resume(&hcd->self, wIndex);
+ clear_bit(portnum, &bus_state->resuming_ports);
+ usb_hcd_end_port_resume(&hcd->self, portnum);
}
- bus_state->port_c_suspend |= 1 << wIndex;
+ bus_state->port_c_suspend |= 1 << portnum;
if (!port->slot_id) {
xhci_dbg(xhci, "slot_id is zero\n");
@@ -1594,7 +1589,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
xhci_ring_device(xhci, port->slot_id);
break;
case USB_PORT_FEAT_C_SUSPEND:
- bus_state->port_c_suspend &= ~(1 << wIndex);
+ bus_state->port_c_suspend &= ~(1 << portnum);
fallthrough;
case USB_PORT_FEAT_C_RESET:
case USB_PORT_FEAT_C_BH_PORT_RESET:
@@ -1603,7 +1598,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
case USB_PORT_FEAT_C_ENABLE:
case USB_PORT_FEAT_C_PORT_LINK_STATE:
case USB_PORT_FEAT_C_PORT_CONFIG_ERROR:
- xhci_clear_port_change_bit(xhci, wValue, wIndex, port, temp);
+ xhci_clear_port_change_bit(xhci, wValue, portnum, port, temp);
break;
case USB_PORT_FEAT_ENABLE:
xhci_disable_port(xhci, port);
--
2.50.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/9] usb: xhci: rename 'wIndex' parameters to 'portnum'
2026-03-19 13:56 [PATCH 0/9] usb: xhci: rework USB request parameter handling Niklas Neronin
2026-03-19 13:56 ` [PATCH 1/9] usb: xhci: stop treating 'wIndex' as a mutable port number Niklas Neronin
@ 2026-03-19 13:56 ` Niklas Neronin
2026-03-19 13:56 ` [PATCH 3/9] usb: xhci: clean up handling of upper bits in SetPortFeature wIndex Niklas Neronin
` (6 subsequent siblings)
8 siblings, 0 replies; 11+ messages in thread
From: Niklas Neronin @ 2026-03-19 13:56 UTC (permalink / raw)
To: mathias.nyman; +Cc: linux-usb, Niklas Neronin
Several helper functions take a parameter named 'wIndex', but the
value they receive is not the raw USB request wIndex field. The only
function that actually processes the USB hub request parameter is
xhci_hub_control(), which extracts the relevant port number (and other
upper-byte fields) before passing them down.
To avoid confusion between the USB request parameter and the derived
0-based port index, rename all such function parameters from 'wIndex'
to 'portnum'. This improves readability and makes the call intentions
clearer.
When a function accept struct 'xhci_port' pointer, use its port number
instead.
Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
drivers/usb/host/xhci-hub.c | 60 +++++++++++++++++--------------------
1 file changed, 27 insertions(+), 33 deletions(-)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 4730beae2478..de843ecc7d63 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -577,8 +577,8 @@ static void xhci_disable_port(struct xhci_hcd *xhci, struct xhci_port *port)
hcd->self.busnum, port->hcd_portnum + 1, portsc);
}
-static void xhci_clear_port_change_bit(struct xhci_hcd *xhci, u16 wValue,
- u16 wIndex, struct xhci_port *port, u32 port_status)
+static void xhci_clear_port_change_bit(struct xhci_hcd *xhci, u16 wValue, struct xhci_port *port,
+ u32 port_status)
{
char *port_change_bit;
u32 status;
@@ -625,7 +625,7 @@ static void xhci_clear_port_change_bit(struct xhci_hcd *xhci, u16 wValue,
port_status = xhci_portsc_readl(port);
xhci_dbg(xhci, "clear port%d %s change, portsc: 0x%x\n",
- wIndex + 1, port_change_bit, port_status);
+ port->hcd_portnum + 1, port_change_bit, port_status);
}
struct xhci_hub *xhci_get_rhub(struct usb_hcd *hcd)
@@ -675,14 +675,13 @@ static void xhci_set_port_power(struct xhci_hcd *xhci, struct xhci_port *port,
spin_lock_irqsave(&xhci->lock, *flags);
}
-static void xhci_port_set_test_mode(struct xhci_hcd *xhci,
- u16 test_mode, u16 wIndex)
+static void xhci_port_set_test_mode(struct xhci_hcd *xhci, u16 test_mode, int portnum)
{
u32 temp;
struct xhci_port *port;
/* xhci only supports test mode for usb2 ports */
- port = xhci->usb2_rhub.ports[wIndex];
+ port = xhci->usb2_rhub.ports[portnum];
temp = readl(&port->port_reg->portpmsc);
temp |= test_mode << PORT_TEST_MODE_SHIFT;
writel(temp, &port->port_reg->portpmsc);
@@ -691,8 +690,8 @@ static void xhci_port_set_test_mode(struct xhci_hcd *xhci,
xhci_start(xhci);
}
-static int xhci_enter_test_mode(struct xhci_hcd *xhci,
- u16 test_mode, u16 wIndex, unsigned long *flags)
+static int xhci_enter_test_mode(struct xhci_hcd *xhci, u16 test_mode, int portnum,
+ unsigned long *flags)
__must_hold(&xhci->lock)
{
int i, retval;
@@ -726,10 +725,8 @@ static int xhci_enter_test_mode(struct xhci_hcd *xhci,
/* Disable runtime PM for test mode */
pm_runtime_forbid(xhci_to_hcd(xhci)->self.controller);
/* Set PORTPMSC.PTC field to enter selected test mode */
- /* Port is selected by wIndex. port_id = wIndex + 1 */
- xhci_dbg(xhci, "Enter Test Mode: %d, Port_id=%d\n",
- test_mode, wIndex + 1);
- xhci_port_set_test_mode(xhci, test_mode, wIndex);
+ xhci_dbg(xhci, "Enter Test Mode: %u, Port_id=%d\n", test_mode, portnum + 1);
+ xhci_port_set_test_mode(xhci, test_mode, portnum);
return retval;
}
@@ -913,8 +910,7 @@ static void xhci_hub_report_usb3_link_state(struct xhci_hcd *xhci,
* the compliance mode timer is deleted. A port won't enter
* compliance mode if it has previously entered U0.
*/
-static void xhci_del_comp_mod_timer(struct xhci_hcd *xhci, u32 status,
- u16 wIndex)
+static void xhci_del_comp_mod_timer(struct xhci_hcd *xhci, u32 status, int portnum)
{
u32 all_ports_seen_u0 = ((1 << xhci->usb3_rhub.num_ports) - 1);
bool port_in_u0 = ((status & PORT_PLS_MASK) == XDEV_U0);
@@ -923,7 +919,7 @@ static void xhci_del_comp_mod_timer(struct xhci_hcd *xhci, u32 status,
return;
if ((xhci->port_status_u0 != all_ports_seen_u0) && port_in_u0) {
- xhci->port_status_u0 |= 1 << wIndex;
+ xhci->port_status_u0 |= 1 << portnum;
if (xhci->port_status_u0 == all_ports_seen_u0) {
timer_delete_sync(&xhci->comp_mode_recovery_timer);
xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
@@ -941,12 +937,12 @@ static int xhci_handle_usb2_port_link_resume(struct xhci_port *port,
struct xhci_bus_state *bus_state;
struct xhci_hcd *xhci;
struct usb_hcd *hcd;
- u32 wIndex;
+ int portnum;
hcd = port->rhub->hcd;
bus_state = &port->rhub->bus_state;
xhci = hcd_to_xhci(hcd);
- wIndex = port->hcd_portnum;
+ portnum = port->hcd_portnum;
if ((portsc & PORT_RESET) || !(portsc & PORT_PE)) {
return -EINVAL;
@@ -954,7 +950,7 @@ static int xhci_handle_usb2_port_link_resume(struct xhci_port *port,
/* did port event handler already start resume timing? */
if (!port->resume_timestamp) {
/* If not, maybe we are in a host initiated resume? */
- if (test_bit(wIndex, &bus_state->resuming_ports)) {
+ if (test_bit(portnum, &bus_state->resuming_ports)) {
/* Host initiated resume doesn't time the resume
* signalling using resume_done[].
* It manually sets RESUME state, sleeps 20ms
@@ -968,20 +964,20 @@ static int xhci_handle_usb2_port_link_resume(struct xhci_port *port,
unsigned long timeout = jiffies +
msecs_to_jiffies(USB_RESUME_TIMEOUT);
- set_bit(wIndex, &bus_state->resuming_ports);
+ set_bit(portnum, &bus_state->resuming_ports);
port->resume_timestamp = timeout;
mod_timer(&hcd->rh_timer, timeout);
- usb_hcd_start_port_resume(&hcd->self, wIndex);
+ usb_hcd_start_port_resume(&hcd->self, portnum);
}
/* Has resume been signalled for USB_RESUME_TIME yet? */
} else if (time_after_eq(jiffies, port->resume_timestamp)) {
int time_left;
xhci_dbg(xhci, "resume USB2 port %d-%d\n",
- hcd->self.busnum, wIndex + 1);
+ hcd->self.busnum, portnum + 1);
port->resume_timestamp = 0;
- clear_bit(wIndex, &bus_state->resuming_ports);
+ clear_bit(portnum, &bus_state->resuming_ports);
reinit_completion(&port->rexit_done);
port->rexit_active = true;
@@ -1005,7 +1001,7 @@ static int xhci_handle_usb2_port_link_resume(struct xhci_port *port,
int port_status = xhci_portsc_readl(port);
xhci_warn(xhci, "Port resume timed out, port %d-%d: 0x%x\n",
- hcd->self.busnum, wIndex + 1, port_status);
+ hcd->self.busnum, portnum + 1, port_status);
/*
* keep rexit_active set if U0 transition failed so we
* know to report PORT_STAT_SUSPEND status back to
@@ -1014,9 +1010,9 @@ static int xhci_handle_usb2_port_link_resume(struct xhci_port *port,
*/
}
- usb_hcd_end_port_resume(&hcd->self, wIndex);
- bus_state->port_c_suspend |= 1 << wIndex;
- bus_state->suspended_ports &= ~(1 << wIndex);
+ usb_hcd_end_port_resume(&hcd->self, portnum);
+ bus_state->port_c_suspend |= 1 << portnum;
+ bus_state->suspended_ports &= ~(1 << portnum);
}
return 0;
@@ -1153,10 +1149,8 @@ static void xhci_get_usb2_port_status(struct xhci_port *port, u32 *status,
* - Stop the Synopsys redriver Compliance Mode polling.
* - Drop and reacquire the xHCI lock, in order to wait for port resume.
*/
-static u32 xhci_get_port_status(struct usb_hcd *hcd,
- struct xhci_bus_state *bus_state,
- u16 wIndex, u32 raw_port_status,
- unsigned long *flags)
+static u32 xhci_get_port_status(struct usb_hcd *hcd, struct xhci_bus_state *bus_state,
+ int portnum, u32 raw_port_status, unsigned long *flags)
__releases(&xhci->lock)
__acquires(&xhci->lock)
{
@@ -1165,7 +1159,7 @@ static u32 xhci_get_port_status(struct usb_hcd *hcd,
struct xhci_port *port;
rhub = xhci_get_rhub(hcd);
- port = rhub->ports[wIndex];
+ port = rhub->ports[portnum];
/* common wPortChange bits */
if (raw_port_status & PORT_CSC)
@@ -1196,7 +1190,7 @@ static u32 xhci_get_port_status(struct usb_hcd *hcd,
xhci_get_usb2_port_status(port, &status, raw_port_status,
flags);
- if (bus_state->port_c_suspend & (1 << wIndex))
+ if (bus_state->port_c_suspend & (1 << portnum))
status |= USB_PORT_STAT_C_SUSPEND << 16;
return status;
@@ -1598,7 +1592,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
case USB_PORT_FEAT_C_ENABLE:
case USB_PORT_FEAT_C_PORT_LINK_STATE:
case USB_PORT_FEAT_C_PORT_CONFIG_ERROR:
- xhci_clear_port_change_bit(xhci, wValue, portnum, port, temp);
+ xhci_clear_port_change_bit(xhci, wValue, port, temp);
break;
case USB_PORT_FEAT_ENABLE:
xhci_disable_port(xhci, port);
--
2.50.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/9] usb: xhci: clean up handling of upper bits in SetPortFeature wIndex
2026-03-19 13:56 [PATCH 0/9] usb: xhci: rework USB request parameter handling Niklas Neronin
2026-03-19 13:56 ` [PATCH 1/9] usb: xhci: stop treating 'wIndex' as a mutable port number Niklas Neronin
2026-03-19 13:56 ` [PATCH 2/9] usb: xhci: rename 'wIndex' parameters to 'portnum' Niklas Neronin
@ 2026-03-19 13:56 ` Niklas Neronin
2026-03-19 13:56 ` [PATCH 4/9] usb: xhci: clean up 'wValue' handling in xhci_hub_control() Niklas Neronin
` (5 subsequent siblings)
8 siblings, 0 replies; 11+ messages in thread
From: Niklas Neronin @ 2026-03-19 13:56 UTC (permalink / raw)
To: mathias.nyman; +Cc: linux-usb, Niklas Neronin
In Set Port Feature requests, the upper byte of 'wIndex' encodes
feature-specific parameters. The current code reads these upper bits in
an early pre-processing block, and then the same feature is handled again
later in the main switch statement. This results in duplicated condition
checks and makes the control flow harder to follow.
Move all feature-specific extraction of 'wIndex' upper bits into the
main SetPortFeature logic so that each feature is handled in exactly one
place. This reduces duplication, makes the handling clearer, and keeps
'wIndex' parsing local to the code that actually uses the values.
Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
drivers/usb/host/xhci-hub.c | 25 ++++++++++++-------------
1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index de843ecc7d63..daa04ac811fc 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -1205,10 +1205,10 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
u32 temp, status;
int retval = 0;
struct xhci_bus_state *bus_state;
- u16 link_state = 0;
- u16 wake_mask = 0;
- u16 timeout = 0;
- u16 test_mode = 0;
+ u16 link_state;
+ u16 wake_mask;
+ u8 timeout;
+ u8 test_mode;
struct xhci_hub *rhub;
struct xhci_port **ports;
struct xhci_port *port;
@@ -1286,15 +1286,6 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
}
break;
case SetPortFeature:
- if (wValue == USB_PORT_FEAT_LINK_STATE)
- link_state = (wIndex & 0xff00) >> 3;
- if (wValue == USB_PORT_FEAT_REMOTE_WAKE_MASK)
- wake_mask = wIndex & 0xff00;
- if (wValue == USB_PORT_FEAT_TEST)
- test_mode = (wIndex & 0xff00) >> 8;
- /* The MSB of wIndex is the U1/U2 timeout */
- timeout = (wIndex & 0xff00) >> 8;
-
portnum = (wIndex & 0xff) - 1;
if (!in_range(portnum, 0, max_ports))
goto error;
@@ -1349,6 +1340,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
bus_state->suspended_ports |= 1 << portnum;
break;
case USB_PORT_FEAT_LINK_STATE:
+ link_state = (wIndex & 0xff00) >> 3;
temp = xhci_portsc_readl(port);
/* Disable port */
if (link_state == USB_SS_PORT_LS_SS_DISABLED) {
@@ -1498,6 +1490,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
hcd->self.busnum, portnum + 1, temp);
break;
case USB_PORT_FEAT_REMOTE_WAKE_MASK:
+ wake_mask = wIndex & 0xff00;
xhci_set_remote_wake_mask(xhci, port, wake_mask);
temp = xhci_portsc_readl(port);
xhci_dbg(xhci, "set port remote wake mask, actual port %d-%d status = 0x%x\n",
@@ -1511,6 +1504,8 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
case USB_PORT_FEAT_U1_TIMEOUT:
if (hcd->speed < HCD_USB3)
goto error;
+
+ timeout = (wIndex & 0xff00) >> 8;
temp = readl(&port->port_reg->portpmsc);
temp &= ~PORT_U1_TIMEOUT_MASK;
temp |= PORT_U1_TIMEOUT(timeout);
@@ -1519,6 +1514,8 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
case USB_PORT_FEAT_U2_TIMEOUT:
if (hcd->speed < HCD_USB3)
goto error;
+
+ timeout = (wIndex & 0xff00) >> 8;
temp = readl(&port->port_reg->portpmsc);
temp &= ~PORT_U2_TIMEOUT_MASK;
temp |= PORT_U2_TIMEOUT(timeout);
@@ -1528,6 +1525,8 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
/* 4.19.6 Port Test Modes (USB2 Test Mode) */
if (hcd->speed != HCD_USB2)
goto error;
+
+ test_mode = (wIndex & 0xff00) >> 8;
if (test_mode > USB_TEST_FORCE_ENABLE ||
test_mode < USB_TEST_J)
goto error;
--
2.50.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 4/9] usb: xhci: clean up 'wValue' handling in xhci_hub_control()
2026-03-19 13:56 [PATCH 0/9] usb: xhci: rework USB request parameter handling Niklas Neronin
` (2 preceding siblings ...)
2026-03-19 13:56 ` [PATCH 3/9] usb: xhci: clean up handling of upper bits in SetPortFeature wIndex Niklas Neronin
@ 2026-03-19 13:56 ` Niklas Neronin
2026-03-19 13:56 ` [PATCH 5/9] usb: xhci: separate use of USB Chapter 11 PLS macros from xHCI-specific PLS macros Niklas Neronin
` (4 subsequent siblings)
8 siblings, 0 replies; 11+ messages in thread
From: Niklas Neronin @ 2026-03-19 13:56 UTC (permalink / raw)
To: mathias.nyman; +Cc: linux-usb, Niklas Neronin
Several hub control requests encode a descriptor type in the upper byte
of 'wValue'. Clean this up by extracting the descriptor type into a local
variable and using it for all relevant requests.
Replace magic value (0x02) with the appropriate macro (HUB_EXT_PORT_STATUS)
This improves readability and makes the handling of 'wValue' consistent.
Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
drivers/usb/host/xhci-hub.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index daa04ac811fc..b57fe0967e10 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -1209,6 +1209,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
u16 wake_mask;
u8 timeout;
u8 test_mode;
+ u8 desc_type;
struct xhci_hub *rhub;
struct xhci_port **ports;
struct xhci_port *port;
@@ -1226,13 +1227,13 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
memset(buf, 0, 4);
break;
case GetHubDescriptor:
+ desc_type = (wValue & 0xff00) >> 8;
/* Check to make sure userspace is asking for the USB 3.0 hub
* descriptor for the USB 3.0 roothub. If not, we stall the
* endpoint, like external hubs do.
*/
if (hcd->speed >= HCD_USB3 &&
- (wLength < USB_DT_SS_HUB_SIZE ||
- wValue != (USB_DT_SS_HUB << 8))) {
+ (wLength < USB_DT_SS_HUB_SIZE || desc_type != USB_DT_SS_HUB)) {
xhci_dbg(xhci, "Wrong hub descriptor type for "
"USB 3.0 roothub.\n");
goto error;
@@ -1241,7 +1242,8 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
(struct usb_hub_descriptor *) buf);
break;
case DeviceRequest | USB_REQ_GET_DESCRIPTOR:
- if ((wValue & 0xff00) != (USB_DT_BOS << 8))
+ desc_type = (wValue & 0xff00) >> 8;
+ if (desc_type != USB_DT_BOS)
goto error;
if (hcd->speed < HCD_USB3)
@@ -1272,7 +1274,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
put_unaligned(cpu_to_le32(status), (__le32 *) buf);
/* if USB 3.1 extended port status return additional 4 bytes */
- if (wValue == 0x02) {
+ if (wValue == HUB_EXT_PORT_STATUS) {
u32 port_li;
if (hcd->speed < HCD_USB31 || wLength != 8) {
--
2.50.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 5/9] usb: xhci: separate use of USB Chapter 11 PLS macros from xHCI-specific PLS macros
2026-03-19 13:56 [PATCH 0/9] usb: xhci: rework USB request parameter handling Niklas Neronin
` (3 preceding siblings ...)
2026-03-19 13:56 ` [PATCH 4/9] usb: xhci: clean up 'wValue' handling in xhci_hub_control() Niklas Neronin
@ 2026-03-19 13:56 ` Niklas Neronin
2026-03-19 13:56 ` [PATCH 6/9] usb: xhci: add PORTPMSC variable to xhci_hub_control() Niklas Neronin
` (3 subsequent siblings)
8 siblings, 0 replies; 11+ messages in thread
From: Niklas Neronin @ 2026-03-19 13:56 UTC (permalink / raw)
To: mathias.nyman; +Cc: linux-usb, Niklas Neronin
The xhci driver uses two different sources for Port Link State (PLS):
1. The PLS field in the PORTSC register (bits 8:5).
2. The PLS value encoded in bits 15:8 of the USB request wIndex,
received by xhci_hub_control().
While both represent similar link states, they differ in a few details,
for example, xHCI's Resume State. Because of these differences, the xhci
driver defines its own set of PLS macros in xhci-port.h, which are intended
to be used when reading and writing PORTSC. The generic USB Chapter 11
macros in ch11.h should only be used when parsing or replying to hub-class
USB requests.
To avoid mixing these two representations and prevent incorrect state
reporting, replace all uses of Chapter 11 PLS macros with the xHCI
versions when interacting with the PORTSC register.
Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
drivers/usb/host/xhci-hub.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index b57fe0967e10..7fb17799cfdc 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -866,8 +866,8 @@ static void xhci_hub_report_usb3_link_state(struct xhci_hcd *xhci,
* unless we're already in compliance
* or the inactive state.
*/
- if (pls != USB_SS_PORT_LS_COMP_MOD &&
- pls != USB_SS_PORT_LS_SS_INACTIVE) {
+ if (pls != XDEV_COMP_MODE &&
+ pls != XDEV_INACTIVE) {
pls = USB_SS_PORT_LS_COMP_MOD;
}
/* Return also connection bit -
@@ -895,7 +895,7 @@ static void xhci_hub_report_usb3_link_state(struct xhci_hcd *xhci,
* caused by a delay on the host-device negotiation.
*/
if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) &&
- (pls == USB_SS_PORT_LS_COMP_MOD))
+ (pls == XDEV_COMP_MODE))
pls |= USB_PORT_STAT_CONNECTION;
}
@@ -1365,7 +1365,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
if (link_state == USB_SS_PORT_LS_RX_DETECT) {
xhci_dbg(xhci, "Enable port %d-%d\n",
hcd->self.busnum, portnum + 1);
- xhci_set_link_state(xhci, port, link_state);
+ xhci_set_link_state(xhci, port, XDEV_RXDETECT);
temp = xhci_portsc_readl(port);
break;
}
@@ -1397,7 +1397,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
xhci_dbg(xhci, "Enable compliance mode transition for port %d-%d\n",
hcd->self.busnum, portnum + 1);
- xhci_set_link_state(xhci, port, link_state);
+ xhci_set_link_state(xhci, port, XDEV_COMP_MODE);
temp = xhci_portsc_readl(port);
break;
@@ -1435,7 +1435,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
reinit_completion(&port->u3exit_done);
}
if (pls <= XDEV_U3) /* U1, U2, U3 */
- xhci_set_link_state(xhci, port, USB_SS_PORT_LS_U0);
+ xhci_set_link_state(xhci, port, XDEV_U0);
if (!wait_u0) {
if (pls > XDEV_U3)
goto error;
@@ -1461,7 +1461,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
xhci_stop_device(xhci, port->slot_id, 1);
spin_lock_irqsave(&xhci->lock, flags);
}
- xhci_set_link_state(xhci, port, USB_SS_PORT_LS_U3);
+ xhci_set_link_state(xhci, port, XDEV_U3);
spin_unlock_irqrestore(&xhci->lock, flags);
while (retries--) {
usleep_range(4000, 8000);
--
2.50.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 6/9] usb: xhci: add PORTPMSC variable to xhci_hub_control()
2026-03-19 13:56 [PATCH 0/9] usb: xhci: rework USB request parameter handling Niklas Neronin
` (4 preceding siblings ...)
2026-03-19 13:56 ` [PATCH 5/9] usb: xhci: separate use of USB Chapter 11 PLS macros from xHCI-specific PLS macros Niklas Neronin
@ 2026-03-19 13:56 ` Niklas Neronin
2026-03-19 13:56 ` [PATCH 7/9] usb: xhci: add PORTSC " Niklas Neronin
` (2 subsequent siblings)
8 siblings, 0 replies; 11+ messages in thread
From: Niklas Neronin @ 2026-03-19 13:56 UTC (permalink / raw)
To: mathias.nyman; +Cc: linux-usb, Niklas Neronin
The code handling U1/U2 timeout updates reads and modifies the PORTPMSC
register using the generic 'temp' variable, which is also used for
PORTSC. This makes the code hard to read and increases the risk of mixing
up register contents.
Introduce a dedicated 'portpmsc' variable for PORTPMSC accesses and use
it in both U1 and U2 timeout handlers. This makes the intent clearer and
keeps register operations logically separated.
Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
drivers/usb/host/xhci-hub.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 7fb17799cfdc..4da3b48dfce0 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -1202,7 +1202,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
int max_ports;
unsigned long flags;
- u32 temp, status;
+ u32 temp, portpmsc, status;
int retval = 0;
struct xhci_bus_state *bus_state;
u16 link_state;
@@ -1508,20 +1508,20 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
goto error;
timeout = (wIndex & 0xff00) >> 8;
- temp = readl(&port->port_reg->portpmsc);
- temp &= ~PORT_U1_TIMEOUT_MASK;
- temp |= PORT_U1_TIMEOUT(timeout);
- writel(temp, &port->port_reg->portpmsc);
+ portpmsc = readl(&port->port_reg->portpmsc);
+ portpmsc &= ~PORT_U1_TIMEOUT_MASK;
+ portpmsc |= PORT_U1_TIMEOUT(timeout);
+ writel(portpmsc, &port->port_reg->portpmsc);
break;
case USB_PORT_FEAT_U2_TIMEOUT:
if (hcd->speed < HCD_USB3)
goto error;
timeout = (wIndex & 0xff00) >> 8;
- temp = readl(&port->port_reg->portpmsc);
- temp &= ~PORT_U2_TIMEOUT_MASK;
- temp |= PORT_U2_TIMEOUT(timeout);
- writel(temp, &port->port_reg->portpmsc);
+ portpmsc = readl(&port->port_reg->portpmsc);
+ portpmsc &= ~PORT_U2_TIMEOUT_MASK;
+ portpmsc |= PORT_U2_TIMEOUT(timeout);
+ writel(portpmsc, &port->port_reg->portpmsc);
break;
case USB_PORT_FEAT_TEST:
/* 4.19.6 Port Test Modes (USB2 Test Mode) */
--
2.50.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 7/9] usb: xhci: add PORTSC variable to xhci_hub_control()
2026-03-19 13:56 [PATCH 0/9] usb: xhci: rework USB request parameter handling Niklas Neronin
` (5 preceding siblings ...)
2026-03-19 13:56 ` [PATCH 6/9] usb: xhci: add PORTPMSC variable to xhci_hub_control() Niklas Neronin
@ 2026-03-19 13:56 ` Niklas Neronin
2026-03-19 13:56 ` [PATCH 8/9] usb: xhci: rename parameter to match argument 'portsc' Niklas Neronin
2026-03-19 13:56 ` [PATCH 9/9] usb: xhci: cleanup xhci_hub_report_usb3_link_state() Niklas Neronin
8 siblings, 0 replies; 11+ messages in thread
From: Niklas Neronin @ 2026-03-19 13:56 UTC (permalink / raw)
To: mathias.nyman; +Cc: linux-usb, Niklas Neronin
The variable 'temp' is used multiple times throughout xhci_hub_control()
for holding only PORTSC register values.
As a follow-up to introducing a dedicated variable for PORTPMSC, rename
all remaining 'temp' to 'portsc'. This improves readability and clarifies
what is being modified.
Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
drivers/usb/host/xhci-hub.c | 102 ++++++++++++++++++------------------
1 file changed, 51 insertions(+), 51 deletions(-)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 4da3b48dfce0..9cd64d6989c9 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -1202,7 +1202,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
int max_ports;
unsigned long flags;
- u32 temp, portpmsc, status;
+ u32 portsc, portpmsc, status;
int retval = 0;
struct xhci_bus_state *bus_state;
u16 link_state;
@@ -1258,19 +1258,19 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
goto error;
port = ports[portnum];
- temp = xhci_portsc_readl(port);
- if (temp == ~(u32)0) {
+ portsc = xhci_portsc_readl(port);
+ if (portsc == ~(u32)0) {
xhci_hc_died(xhci);
retval = -ENODEV;
break;
}
- trace_xhci_get_port_status(port, temp);
- status = xhci_get_port_status(hcd, bus_state, portnum, temp, &flags);
+ trace_xhci_get_port_status(port, portsc);
+ status = xhci_get_port_status(hcd, bus_state, portnum, portsc, &flags);
if (status == 0xffffffff)
goto error;
xhci_dbg(xhci, "Get port status %d-%d read: 0x%x, return 0x%x",
- hcd->self.busnum, portnum + 1, temp, status);
+ hcd->self.busnum, portnum + 1, portsc, status);
put_unaligned(cpu_to_le32(status), (__le32 *) buf);
/* if USB 3.1 extended port status return additional 4 bytes */
@@ -1283,7 +1283,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
break;
}
port_li = readl(&port->port_reg->portli);
- status = xhci_get_ext_port_status(temp, port_li);
+ status = xhci_get_ext_port_status(portsc, port_li);
put_unaligned_le32(status, &buf[4]);
}
break;
@@ -1293,18 +1293,18 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
goto error;
port = ports[portnum];
- temp = xhci_portsc_readl(port);
- if (temp == ~(u32)0) {
+ portsc = xhci_portsc_readl(port);
+ if (portsc == ~(u32)0) {
xhci_hc_died(xhci);
retval = -ENODEV;
break;
}
- temp = xhci_port_state_to_neutral(temp);
+ portsc = xhci_port_state_to_neutral(portsc);
/* FIXME: What new port features do we need to support? */
switch (wValue) {
case USB_PORT_FEAT_SUSPEND:
- temp = xhci_portsc_readl(port);
- if ((temp & PORT_PLS_MASK) != XDEV_U0) {
+ portsc = xhci_portsc_readl(port);
+ if ((portsc & PORT_PLS_MASK) != XDEV_U0) {
/* Resume the port to U0 first */
xhci_set_link_state(xhci, port, XDEV_U0);
spin_unlock_irqrestore(&xhci->lock, flags);
@@ -1315,9 +1315,9 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
* a port unless the port reports that it is in the
* enabled (PED = ‘1’,PLS < ‘3’) state.
*/
- temp = xhci_portsc_readl(port);
- if ((temp & PORT_PE) == 0 || (temp & PORT_RESET)
- || (temp & PORT_PLS_MASK) >= XDEV_U3) {
+ portsc = xhci_portsc_readl(port);
+ if ((portsc & PORT_PE) == 0 || (portsc & PORT_RESET) ||
+ (portsc & PORT_PLS_MASK) >= XDEV_U3) {
xhci_warn(xhci, "USB core suspending port %d-%d not in U0/U1/U2\n",
hcd->self.busnum, portnum + 1);
goto error;
@@ -1338,26 +1338,26 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
msleep(10); /* wait device to enter */
spin_lock_irqsave(&xhci->lock, flags);
- temp = xhci_portsc_readl(port);
+ portsc = xhci_portsc_readl(port);
bus_state->suspended_ports |= 1 << portnum;
break;
case USB_PORT_FEAT_LINK_STATE:
link_state = (wIndex & 0xff00) >> 3;
- temp = xhci_portsc_readl(port);
+ portsc = xhci_portsc_readl(port);
/* Disable port */
if (link_state == USB_SS_PORT_LS_SS_DISABLED) {
xhci_dbg(xhci, "Disable port %d-%d\n",
hcd->self.busnum, portnum + 1);
- temp = xhci_port_state_to_neutral(temp);
+ portsc = xhci_port_state_to_neutral(portsc);
/*
* Clear all change bits, so that we get a new
* connection event.
*/
- temp |= PORT_CSC | PORT_PEC | PORT_WRC |
- PORT_OCC | PORT_RC | PORT_PLC |
- PORT_CEC;
- xhci_portsc_writel(port, temp | PORT_PE);
- temp = xhci_portsc_readl(port);
+ portsc |= PORT_CSC | PORT_PEC | PORT_WRC |
+ PORT_OCC | PORT_RC | PORT_PLC |
+ PORT_CEC;
+ xhci_portsc_writel(port, portsc | PORT_PE);
+ portsc = xhci_portsc_readl(port);
break;
}
@@ -1366,7 +1366,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
xhci_dbg(xhci, "Enable port %d-%d\n",
hcd->self.busnum, portnum + 1);
xhci_set_link_state(xhci, port, XDEV_RXDETECT);
- temp = xhci_portsc_readl(port);
+ portsc = xhci_portsc_readl(port);
break;
}
@@ -1390,7 +1390,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
break;
}
- if ((temp & PORT_CONNECT)) {
+ if ((portsc & PORT_CONNECT)) {
xhci_warn(xhci, "Can't set compliance mode when port is connected\n");
goto error;
}
@@ -1399,11 +1399,11 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
hcd->self.busnum, portnum + 1);
xhci_set_link_state(xhci, port, XDEV_COMP_MODE);
- temp = xhci_portsc_readl(port);
+ portsc = xhci_portsc_readl(port);
break;
}
/* Port must be enabled */
- if (!(temp & PORT_PE)) {
+ if (!(portsc & PORT_PE)) {
retval = -ENODEV;
break;
}
@@ -1422,7 +1422,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
* completion
*/
if (link_state == USB_SS_PORT_LS_U0) {
- u32 pls = temp & PORT_PLS_MASK;
+ u32 pls = portsc & PORT_PLS_MASK;
bool wait_u0 = false;
/* already in U0 */
@@ -1447,7 +1447,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
xhci_dbg(xhci, "missing U0 port change event for port %d-%d\n",
hcd->self.busnum, portnum + 1);
spin_lock_irqsave(&xhci->lock, flags);
- temp = xhci_portsc_readl(port);
+ portsc = xhci_portsc_readl(port);
break;
}
@@ -1465,12 +1465,12 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
spin_unlock_irqrestore(&xhci->lock, flags);
while (retries--) {
usleep_range(4000, 8000);
- temp = xhci_portsc_readl(port);
- if ((temp & PORT_PLS_MASK) == XDEV_U3)
+ portsc = xhci_portsc_readl(port);
+ if ((portsc & PORT_PLS_MASK) == XDEV_U3)
break;
}
spin_lock_irqsave(&xhci->lock, flags);
- temp = xhci_portsc_readl(port);
+ portsc = xhci_portsc_readl(port);
bus_state->suspended_ports |= 1 << portnum;
}
break;
@@ -1484,24 +1484,24 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
xhci_set_port_power(xhci, port, true, &flags);
break;
case USB_PORT_FEAT_RESET:
- temp = (temp | PORT_RESET);
- xhci_portsc_writel(port, temp);
+ portsc |= PORT_RESET;
+ xhci_portsc_writel(port, portsc);
- temp = xhci_portsc_readl(port);
+ portsc = xhci_portsc_readl(port);
xhci_dbg(xhci, "set port reset, actual port %d-%d status = 0x%x\n",
- hcd->self.busnum, portnum + 1, temp);
+ hcd->self.busnum, portnum + 1, portsc);
break;
case USB_PORT_FEAT_REMOTE_WAKE_MASK:
wake_mask = wIndex & 0xff00;
xhci_set_remote_wake_mask(xhci, port, wake_mask);
- temp = xhci_portsc_readl(port);
+ portsc = xhci_portsc_readl(port);
xhci_dbg(xhci, "set port remote wake mask, actual port %d-%d status = 0x%x\n",
- hcd->self.busnum, portnum + 1, temp);
+ hcd->self.busnum, portnum + 1, portsc);
break;
case USB_PORT_FEAT_BH_PORT_RESET:
- temp |= PORT_WR;
- xhci_portsc_writel(port, temp);
- temp = xhci_portsc_readl(port);
+ portsc |= PORT_WR;
+ xhci_portsc_writel(port, portsc);
+ portsc = xhci_portsc_readl(port);
break;
case USB_PORT_FEAT_U1_TIMEOUT:
if (hcd->speed < HCD_USB3)
@@ -1538,7 +1538,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
goto error;
}
/* unblock any posted writes */
- temp = xhci_portsc_readl(port);
+ portsc = xhci_portsc_readl(port);
break;
case ClearPortFeature:
portnum = (wIndex & 0xff) - 1;
@@ -1546,23 +1546,23 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
goto error;
port = ports[portnum];
- temp = xhci_portsc_readl(port);
- if (temp == ~(u32)0) {
+ portsc = xhci_portsc_readl(port);
+ if (portsc == ~(u32)0) {
xhci_hc_died(xhci);
retval = -ENODEV;
break;
}
/* FIXME: What new port features do we need to support? */
- temp = xhci_port_state_to_neutral(temp);
+ portsc = xhci_port_state_to_neutral(portsc);
switch (wValue) {
case USB_PORT_FEAT_SUSPEND:
- temp = xhci_portsc_readl(port);
+ portsc = xhci_portsc_readl(port);
xhci_dbg(xhci, "clear USB_PORT_FEAT_SUSPEND\n");
- xhci_dbg(xhci, "PORTSC %04x\n", temp);
- if (temp & PORT_RESET)
+ xhci_dbg(xhci, "PORTSC %04x\n", portsc);
+ if (portsc & PORT_RESET)
goto error;
- if ((temp & PORT_PLS_MASK) == XDEV_U3) {
- if ((temp & PORT_PE) == 0)
+ if ((portsc & PORT_PLS_MASK) == XDEV_U3) {
+ if ((portsc & PORT_PE) == 0)
goto error;
set_bit(portnum, &bus_state->resuming_ports);
@@ -1593,7 +1593,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
case USB_PORT_FEAT_C_ENABLE:
case USB_PORT_FEAT_C_PORT_LINK_STATE:
case USB_PORT_FEAT_C_PORT_CONFIG_ERROR:
- xhci_clear_port_change_bit(xhci, wValue, port, temp);
+ xhci_clear_port_change_bit(xhci, wValue, port, portsc);
break;
case USB_PORT_FEAT_ENABLE:
xhci_disable_port(xhci, port);
--
2.50.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 8/9] usb: xhci: rename parameter to match argument 'portsc'
2026-03-19 13:56 [PATCH 0/9] usb: xhci: rework USB request parameter handling Niklas Neronin
` (6 preceding siblings ...)
2026-03-19 13:56 ` [PATCH 7/9] usb: xhci: add PORTSC " Niklas Neronin
@ 2026-03-19 13:56 ` Niklas Neronin
2026-03-19 13:56 ` [PATCH 9/9] usb: xhci: cleanup xhci_hub_report_usb3_link_state() Niklas Neronin
8 siblings, 0 replies; 11+ messages in thread
From: Niklas Neronin @ 2026-03-19 13:56 UTC (permalink / raw)
To: mathias.nyman; +Cc: linux-usb, Niklas Neronin
A previous patch renamed the temporary variable holding the value read
from the PORTSC register from 'temp' to 'portsc'. This patch follows up
by updating the parameter names of all helper functions called from
xhci_hub_control() that receive a PORTSC value, as well as the functions
they call.
Function changed:
xhci_get_port_status()
L xhci_get_usb3_port_status()
L xhci_hub_report_usb3_link_state()
L xhci_del_comp_mod_timer()
xhci_get_ext_port_status()
xhci_port_state_to_neutral()
xhci_clear_port_change_bit()
xhci_port_speed()
The reason for the rename is to differentiate between port
status/change bit to be written to PORTSC and replying to hub-class
USB requests. Each of them use their specific macros.
Use "portsc" name for PORTSC values and "status" for values intended
for replying to hub-class USB request.
A dedicated structure for USB hub port status responses
('struct usb_port_status' from ch11.h) exists and will be integrated in
a later patch.
Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
drivers/usb/host/xhci-hub.c | 61 ++++++++++++++++++-------------------
1 file changed, 30 insertions(+), 31 deletions(-)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 9cd64d6989c9..8e134f6b4e37 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -375,11 +375,11 @@ static void xhci_hub_descriptor(struct usb_hcd *hcd, struct xhci_hcd *xhci,
}
-static unsigned int xhci_port_speed(unsigned int port_status)
+static unsigned int xhci_port_speed(int portsc)
{
- if (DEV_LOWSPEED(port_status))
+ if (DEV_LOWSPEED(portsc))
return USB_PORT_STAT_LOW_SPEED;
- if (DEV_HIGHSPEED(port_status))
+ if (DEV_HIGHSPEED(portsc))
return USB_PORT_STAT_HIGH_SPEED;
/*
* FIXME: Yes, we should check for full speed, but the core uses that as
@@ -429,9 +429,9 @@ static unsigned int xhci_port_speed(unsigned int port_status)
/**
* xhci_port_state_to_neutral() - Clean up read portsc value back into writeable
- * @state: u32 port value read from portsc register to be cleanup up
+ * @portsc: u32 port value read from portsc register to be cleanup up
*
- * Given a port state, this function returns a value that would result in the
+ * Given a portsc, this function returns a value that would result in the
* port being in the same state, if the value was written to the port status
* control register.
* Save Read Only (RO) bits and save read/write bits where
@@ -442,10 +442,10 @@ static unsigned int xhci_port_speed(unsigned int port_status)
* changing port state.
*/
-u32 xhci_port_state_to_neutral(u32 state)
+u32 xhci_port_state_to_neutral(u32 portsc)
{
/* Save read-only status and port state */
- return (state & XHCI_PORT_RO) | (state & XHCI_PORT_RWS);
+ return (portsc & XHCI_PORT_RO) | (portsc & XHCI_PORT_RWS);
}
EXPORT_SYMBOL_GPL(xhci_port_state_to_neutral);
@@ -578,7 +578,7 @@ static void xhci_disable_port(struct xhci_hcd *xhci, struct xhci_port *port)
}
static void xhci_clear_port_change_bit(struct xhci_hcd *xhci, u16 wValue, struct xhci_port *port,
- u32 port_status)
+ u32 portsc)
{
char *port_change_bit;
u32 status;
@@ -621,11 +621,11 @@ static void xhci_clear_port_change_bit(struct xhci_hcd *xhci, u16 wValue, struct
return;
}
/* Change bits are all write 1 to clear */
- xhci_portsc_writel(port, port_status | status);
- port_status = xhci_portsc_readl(port);
+ xhci_portsc_writel(port, portsc | status);
+ portsc = xhci_portsc_readl(port);
xhci_dbg(xhci, "clear port%d %s change, portsc: 0x%x\n",
- port->hcd_portnum + 1, port_change_bit, port_status);
+ port->hcd_portnum + 1, port_change_bit, portsc);
}
struct xhci_hub *xhci_get_rhub(struct usb_hcd *hcd)
@@ -851,14 +851,14 @@ void xhci_test_and_clear_bit(struct xhci_hcd *xhci, struct xhci_port *port,
/* Updates Link Status for super Speed port */
static void xhci_hub_report_usb3_link_state(struct xhci_hcd *xhci,
- u32 *status, u32 status_reg)
+ u32 *status, u32 portsc)
{
- u32 pls = status_reg & PORT_PLS_MASK;
+ u32 pls = portsc & PORT_PLS_MASK;
/* When the CAS bit is set then warm reset
* should be performed on port
*/
- if (status_reg & PORT_CAS) {
+ if (portsc & PORT_CAS) {
/* The CAS bit can be set while the port is
* in any link state.
* Only roothubs have CAS bit, so we
@@ -910,10 +910,10 @@ static void xhci_hub_report_usb3_link_state(struct xhci_hcd *xhci,
* the compliance mode timer is deleted. A port won't enter
* compliance mode if it has previously entered U0.
*/
-static void xhci_del_comp_mod_timer(struct xhci_hcd *xhci, u32 status, int portnum)
+static void xhci_del_comp_mod_timer(struct xhci_hcd *xhci, u32 portsc, int portnum)
{
u32 all_ports_seen_u0 = ((1 << xhci->usb3_rhub.num_ports) - 1);
- bool port_in_u0 = ((status & PORT_PLS_MASK) == XDEV_U0);
+ bool port_in_u0 = ((portsc & PORT_PLS_MASK) == XDEV_U0);
if (!(xhci->quirks & XHCI_COMP_MODE_QUIRK))
return;
@@ -1018,13 +1018,13 @@ static int xhci_handle_usb2_port_link_resume(struct xhci_port *port,
return 0;
}
-static u32 xhci_get_ext_port_status(u32 raw_port_status, u32 port_li)
+static u32 xhci_get_ext_port_status(u32 portsc, u32 port_li)
{
u32 ext_stat = 0;
int speed_id;
/* only support rx and tx lane counts of 1 in usb3.1 spec */
- speed_id = DEV_PORT_SPEED(raw_port_status);
+ speed_id = DEV_PORT_SPEED(portsc);
ext_stat |= speed_id; /* bits 3:0, RX speed id */
ext_stat |= speed_id << 4; /* bits 7:4, TX speed id */
@@ -1150,7 +1150,7 @@ static void xhci_get_usb2_port_status(struct xhci_port *port, u32 *status,
* - Drop and reacquire the xHCI lock, in order to wait for port resume.
*/
static u32 xhci_get_port_status(struct usb_hcd *hcd, struct xhci_bus_state *bus_state,
- int portnum, u32 raw_port_status, unsigned long *flags)
+ int portnum, u32 portsc, unsigned long *flags)
__releases(&xhci->lock)
__acquires(&xhci->lock)
{
@@ -1162,33 +1162,32 @@ static u32 xhci_get_port_status(struct usb_hcd *hcd, struct xhci_bus_state *bus_
port = rhub->ports[portnum];
/* common wPortChange bits */
- if (raw_port_status & PORT_CSC)
+ if (portsc & PORT_CSC)
status |= USB_PORT_STAT_C_CONNECTION << 16;
- if (raw_port_status & PORT_PEC)
+ if (portsc & PORT_PEC)
status |= USB_PORT_STAT_C_ENABLE << 16;
- if ((raw_port_status & PORT_OCC))
+ if (portsc & PORT_OCC)
status |= USB_PORT_STAT_C_OVERCURRENT << 16;
- if ((raw_port_status & PORT_RC))
+ if (portsc & PORT_RC)
status |= USB_PORT_STAT_C_RESET << 16;
/* common wPortStatus bits */
- if (raw_port_status & PORT_CONNECT) {
+ if (portsc & PORT_CONNECT) {
status |= USB_PORT_STAT_CONNECTION;
- status |= xhci_port_speed(raw_port_status);
+ status |= xhci_port_speed(portsc);
}
- if (raw_port_status & PORT_PE)
+ if (portsc & PORT_PE)
status |= USB_PORT_STAT_ENABLE;
- if (raw_port_status & PORT_OC)
+ if (portsc & PORT_OC)
status |= USB_PORT_STAT_OVERCURRENT;
- if (raw_port_status & PORT_RESET)
+ if (portsc & PORT_RESET)
status |= USB_PORT_STAT_RESET;
/* USB2 and USB3 specific bits, including Port Link State */
if (hcd->speed >= HCD_USB3)
- xhci_get_usb3_port_status(port, &status, raw_port_status);
+ xhci_get_usb3_port_status(port, &status, portsc);
else
- xhci_get_usb2_port_status(port, &status, raw_port_status,
- flags);
+ xhci_get_usb2_port_status(port, &status, portsc, flags);
if (bus_state->port_c_suspend & (1 << portnum))
status |= USB_PORT_STAT_C_SUSPEND << 16;
--
2.50.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 9/9] usb: xhci: cleanup xhci_hub_report_usb3_link_state()
2026-03-19 13:56 [PATCH 0/9] usb: xhci: rework USB request parameter handling Niklas Neronin
` (7 preceding siblings ...)
2026-03-19 13:56 ` [PATCH 8/9] usb: xhci: rename parameter to match argument 'portsc' Niklas Neronin
@ 2026-03-19 13:56 ` Niklas Neronin
8 siblings, 0 replies; 11+ messages in thread
From: Niklas Neronin @ 2026-03-19 13:56 UTC (permalink / raw)
To: mathias.nyman; +Cc: linux-usb, Niklas Neronin
Improve readability of xhci_hub_report_usb3_link_state().
Comments are shortened and clarified, and the code now makes it explicit
when the Port Link State (PLS) value is modified versus when other status
bits are updated.
No functional changes.
Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
drivers/usb/host/xhci-hub.c | 58 ++++++++++++++-----------------------
1 file changed, 21 insertions(+), 37 deletions(-)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 8e134f6b4e37..bacd0ddd0d09 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -850,53 +850,37 @@ void xhci_test_and_clear_bit(struct xhci_hcd *xhci, struct xhci_port *port,
}
/* Updates Link Status for super Speed port */
-static void xhci_hub_report_usb3_link_state(struct xhci_hcd *xhci,
- u32 *status, u32 portsc)
+static void xhci_hub_report_usb3_link_state(struct xhci_hcd *xhci, u32 *status, u32 portsc)
{
u32 pls = portsc & PORT_PLS_MASK;
- /* When the CAS bit is set then warm reset
- * should be performed on port
+ /*
+ * CAS indicates that a warm reset is required, it may be set in any
+ * link state and is only present on roothubs.
*/
if (portsc & PORT_CAS) {
- /* The CAS bit can be set while the port is
- * in any link state.
- * Only roothubs have CAS bit, so we
- * pretend to be in compliance mode
- * unless we're already in compliance
- * or the inactive state.
+ /*
+ * If not already in Compliance or Inactive state,
+ * report Compliance Mode so the hub logic triggers a warm reset.
*/
- if (pls != XDEV_COMP_MODE &&
- pls != XDEV_INACTIVE) {
+ if (pls != XDEV_COMP_MODE && pls != XDEV_INACTIVE)
pls = USB_SS_PORT_LS_COMP_MOD;
- }
- /* Return also connection bit -
- * hub state machine resets port
- * when this bit is set.
- */
- pls |= USB_PORT_STAT_CONNECTION;
- } else {
- /*
- * Resume state is an xHCI internal state. Do not report it to
- * usb core, instead, pretend to be U3, thus usb core knows
- * it's not ready for transfer.
- */
- if (pls == XDEV_RESUME) {
- *status |= USB_SS_PORT_LS_U3;
- return;
- }
+ /* Signal a connection change to force a reset */
+ *status |= USB_PORT_STAT_CONNECTION;
+ } else if (pls == XDEV_RESUME) {
/*
- * If CAS bit isn't set but the Port is already at
- * Compliance Mode, fake a connection so the USB core
- * notices the Compliance state and resets the port.
- * This resolves an issue generated by the SN65LVPE502CP
- * in which sometimes the port enters compliance mode
- * caused by a delay on the host-device negotiation.
+ * Resume is an internal xHCI-only state and must not be exposed
+ * to usbcore. Report it as U3 so transfers are blocked.
*/
- if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) &&
- (pls == XDEV_COMP_MODE))
- pls |= USB_PORT_STAT_CONNECTION;
+ pls = USB_SS_PORT_LS_U3;
+ } else if (pls == XDEV_COMP_MODE) {
+ /*
+ * Some hardware may enter Compliance Mode without CAS.
+ * Fake a connection event so usbcore notices and resets the port.
+ */
+ if (xhci->quirks & XHCI_COMP_MODE_QUIRK)
+ *status |= USB_PORT_STAT_CONNECTION;
}
/* update status field */
--
2.50.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/9] usb: xhci: stop treating 'wIndex' as a mutable port number
2026-03-19 13:56 ` [PATCH 1/9] usb: xhci: stop treating 'wIndex' as a mutable port number Niklas Neronin
@ 2026-03-19 14:10 ` Oliver Neukum
0 siblings, 0 replies; 11+ messages in thread
From: Oliver Neukum @ 2026-03-19 14:10 UTC (permalink / raw)
To: Niklas Neronin, mathias.nyman; +Cc: linux-usb
Hi,
On 19.03.26 14:56, Niklas Neronin wrote:
> @@ -1258,11 +1257,11 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
> spin_unlock_irqrestore(&xhci->lock, flags);
> return retval;
> case GetPortStatus:
> - if (!portnum1 || portnum1 > max_ports)
> + portnum = (wIndex & 0xff) - 1;
Should this be a macro like usb_index_to_portnum() or something
similar?
Regards
Oliver
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2026-03-19 14:10 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-03-19 13:56 [PATCH 0/9] usb: xhci: rework USB request parameter handling Niklas Neronin
2026-03-19 13:56 ` [PATCH 1/9] usb: xhci: stop treating 'wIndex' as a mutable port number Niklas Neronin
2026-03-19 14:10 ` Oliver Neukum
2026-03-19 13:56 ` [PATCH 2/9] usb: xhci: rename 'wIndex' parameters to 'portnum' Niklas Neronin
2026-03-19 13:56 ` [PATCH 3/9] usb: xhci: clean up handling of upper bits in SetPortFeature wIndex Niklas Neronin
2026-03-19 13:56 ` [PATCH 4/9] usb: xhci: clean up 'wValue' handling in xhci_hub_control() Niklas Neronin
2026-03-19 13:56 ` [PATCH 5/9] usb: xhci: separate use of USB Chapter 11 PLS macros from xHCI-specific PLS macros Niklas Neronin
2026-03-19 13:56 ` [PATCH 6/9] usb: xhci: add PORTPMSC variable to xhci_hub_control() Niklas Neronin
2026-03-19 13:56 ` [PATCH 7/9] usb: xhci: add PORTSC " Niklas Neronin
2026-03-19 13:56 ` [PATCH 8/9] usb: xhci: rename parameter to match argument 'portsc' Niklas Neronin
2026-03-19 13:56 ` [PATCH 9/9] usb: xhci: cleanup xhci_hub_report_usb3_link_state() Niklas Neronin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox