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

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

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