[PATCH] usb: xhci: Fix lockdep warning when entering test mode

[PATCH] usb: xhci: Fix lockdep warning when entering test mode

[Yang Zi]

xhci_enter_test_mode() and xhci_set_port_power() are annotated
__must_hold(&xhci->lock), yet both drop the lock in the middle of the
function: xhci_enter_test_mode() around the slot-disable loop (because
xhci_disable_and_free_slot() takes the lock itself and can sleep) and
xhci_set_port_power() around the ACPI power-state calls.

xhci->lock is also taken from hardirq context in xhci_irq(), so it is a
hardirq-safe lock.  Dropping it with spin_unlock_irqrestore() re-enables
interrupts while the lock is still held, and lockdep's
trace_hardirqs_on() -> mark_held_locks() then records the lock as
HARDIRQ-ON-W, which conflicts with the IN-HARDIRQ-W usage registered by
xhci_irq():

    inconsistent {IN-HARDIRQ-W} -> {HARDIRQ-ON-W} usage.

Fix this by releasing the lock *before* re-enabling interrupts (and, on
the way back, disabling interrupts before re-acquiring the lock), so the
hardirq-safe lock is never held with IRQs enabled.  Also drop the
incorrect __must_hold() annotations and pass the saved IRQ state by
value so these helpers cannot clobber the caller's flags.

This patch is tentative and needs maintainer review.

Signed-off-by: Yang Zi <2959243019@qq.com>
---
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index b0264bd8577a..c28d278f45b0 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -638,12 +638,19 @@ struct xhci_hub *xhci_get_rhub(struct usb_hcd *hcd)
 
 /*
  * xhci_set_port_power() must be called with xhci->lock held.
- * It will release and re-acquire the lock while calling ACPI
- * method.
+ * It drops the lock while calling the ACPI method, which may sleep, and
+ * re-acquires it before returning.
+ *
+ * The lock is released *before* interrupts are re-enabled because
+ * xhci->lock is also taken in hardirq context (xhci_irq()) and must never
+ * be held with IRQs enabled.
+ *
+ * @flags is passed by value: it is the IRQ state saved by the caller's
+ * spin_lock_irqsave() and must not be clobbered by the lock/irqsave dance
+ * below, so the caller can later restore it with spin_unlock_irqrestore().
  */
 static void xhci_set_port_power(struct xhci_hcd *xhci, struct xhci_port *port,
-                bool on, unsigned long *flags)
-    __must_hold(&xhci->lock)
+                bool on, unsigned long flags)
 {
     struct usb_hcd *hcd;
     u32 temp;
@@ -665,13 +672,15 @@ static void xhci_set_port_power(struct xhci_hcd *xhci, struct xhci_port *port,
         xhci_portsc_writel(port, temp & ~PORT_POWER);
     }
 
-    spin_unlock_irqrestore(&xhci->lock, *flags);
+    spin_unlock(&xhci->lock);
+    local_irq_restore(flags);
     temp = usb_acpi_power_manageable(hcd->self.root_hub,
                      port->hcd_portnum);
     if (temp)
         usb_acpi_set_power_state(hcd->self.root_hub,
                      port->hcd_portnum, on);
-    spin_lock_irqsave(&xhci->lock, *flags);
+    local_irq_save(flags);
+    spin_lock(&xhci->lock);
 }
 
 static void xhci_port_set_test_mode(struct xhci_hcd *xhci, u16 test_mode, int portnum)
@@ -689,15 +698,24 @@ static void xhci_port_set_test_mode(struct xhci_hcd *xhci, u16 test_mode, int po
         xhci_start(xhci);
 }
 
+/*
+ * xhci_enter_test_mode() is called with xhci->lock held. It drops the lock
+ * around the slot-disable loop because xhci_disable_and_free_slot() takes
+ * xhci->lock itself and can sleep, then re-acquires it for the remainder of
+ * the function. The lock is released before interrupts are re-enabled since
+ * xhci->lock is also taken in hardirq context (xhci_irq()).
+ *
+ * @flags is passed by value so the caller's saved IRQ state is preserved.
+ */
 static int xhci_enter_test_mode(struct xhci_hcd *xhci, u16 test_mode, int portnum,
-                unsigned long *flags)
-    __must_hold(&xhci->lock)
+                unsigned long flags)
 {
     int i, retval;
 
     /* Disable all Device Slots */
     xhci_dbg(xhci, "Disable all slots\n");
-    spin_unlock_irqrestore(&xhci->lock, *flags);
+    spin_unlock(&xhci->lock);
+    local_irq_restore(flags);
     for (i = 1; i <= xhci->max_slots; i++) {
         if (!xhci->devs[i])
             continue;
@@ -707,7 +725,8 @@ static int xhci_enter_test_mode(struct xhci_hcd *xhci, u16 test_mode, int portnu
             xhci_err(xhci, "Failed to disable slot %d, %d. Enter test mode anyway\n",
                  i, retval);
     }
-    spin_lock_irqsave(&xhci->lock, *flags);
+    local_irq_save(flags);
+    spin_lock(&xhci->lock);
     /* Put all ports to the Disable state by clear PP */
     xhci_dbg(xhci, "Disable all port (PP = 0)\n");
     /* Power off USB3 ports*/
@@ -1463,7 +1482,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
              * However, hub_wq will ignore the roothub events until
              * the roothub is registered.
              */
-            xhci_set_port_power(xhci, port, true, &flags);
+            xhci_set_port_power(xhci, port, true, flags);
             break;
         case USB_PORT_FEAT_RESET:
             portsc |= PORT_RESET;
@@ -1514,7 +1533,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, portnum, &flags);
+            retval = xhci_enter_test_mode(xhci, test_mode, portnum, flags);
             break;
         default:
             goto error;
@@ -1581,7 +1600,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
             xhci_disable_port(xhci, port);
             break;
         case USB_PORT_FEAT_POWER:
-            xhci_set_port_power(xhci, port, false, &flags);
+            xhci_set_port_power(xhci, port, false, flags);
             break;
         case USB_PORT_FEAT_TEST:
             retval = xhci_exit_test_mode(xhci);

Re: [PATCH] usb: xhci: Fix lockdep warning when entering test mode

[Mathias Nyman]

On 8/25/26 12:09, Yang Zi wrote:
> xhci_enter_test_mode() and xhci_set_port_power() are annotated
> __must_hold(&xhci->lock), yet both drop the lock in the middle of the
> function: xhci_enter_test_mode() around the slot-disable loop (because
> xhci_disable_and_free_slot() takes the lock itself and can sleep) and
> xhci_set_port_power() around the ACPI power-state calls.
> 
> xhci->lock is also taken from hardirq context in xhci_irq(), so it is a
> hardirq-safe lock.  Dropping it with spin_unlock_irqrestore() re-enables
> interrupts while the lock is still held, and lockdep's
> trace_hardirqs_on() -> mark_held_locks() then records the lock as
> HARDIRQ-ON-W, which conflicts with the IN-HARDIRQ-W usage registered by
> xhci_irq():
> 
>      inconsistent {IN-HARDIRQ-W} -> {HARDIRQ-ON-W} usage.
> 

This sounds odd, shouldn't spin_unlock_irqresore() first release the
spinlock and then enable interrupts?

Wouldn't this be an issue for every driver that shares a spinlock
in interrupt context and elsewhere?

spinlock_api_smp.h has:

static inline void __raw_spin_unlock_irqrestore(raw_spinlock_t *lock,
unsigned long flags)
	__releases(lock)
{
         spin_release(&lock->dep_map, _RET_IP_);
         do_raw_spin_unlock(lock);
	local_irq_restore(flags);
         preempt_enable();
}


To be fair it looks odd on a uniprocessor system:

spinlock_api_up.h:

#define __UNLOCK_IRQRESTORE(lock, flags, ...) \
   do { local_irq_restore(flags); __UNLOCK(lock, ##__VA_ARGS__); } while (0)

Are you running this on a single processor system?

> Fix this by releasing the lock *before* re-enabling interrupts (and, on
> the way back, disabling interrupts before re-acquiring the lock), so the
> hardirq-safe lock is never held with IRQs enabled.  Also drop the
> incorrect __must_hold() annotations and pass the saved IRQ state by
> value so these helpers cannot clobber the caller's flags.
> 
> This patch is tentative and needs maintainer review.
> 
> Signed-off-by: Yang Zi <2959243019@qq.com>
> ---
> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> index b0264bd8577a..c28d278f45b0 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -638,12 +638,19 @@ struct xhci_hub *xhci_get_rhub(struct usb_hcd *hcd)
>   
>   /*
>    * xhci_set_port_power() must be called with xhci->lock held.
> - * It will release and re-acquire the lock while calling ACPI
> - * method.
> + * It drops the lock while calling the ACPI method, which may sleep, and
> + * re-acquires it before returning.
> + *
> + * The lock is released *before* interrupts are re-enabled because
> + * xhci->lock is also taken in hardirq context (xhci_irq()) and must never
> + * be held with IRQs enabled.
> + *
> + * @flags is passed by value: it is the IRQ state saved by the caller's
> + * spin_lock_irqsave() and must not be clobbered by the lock/irqsave dance
> + * below, so the caller can later restore it with spin_unlock_irqrestore().

The caller should restore it to the flags value set during latest spin_lock_irqsave()
which is during the lock/irqsace dance.

Thanks
Mathias