Re: [PATCH 3/3] xhci: Don't create stream debugfs files with spinlock held.

[Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>]

On 2020-10-29 15:08:06 [+0200], Mathias Nyman wrote:
> 2. Lockdep issue #2, adding entries to radix tree during (stream) ring expansion with interrupts disabled and xhci->lock held.
>    Cause: unknown, probably a patch since we started using radix trees for finding streams
>    Fix: unknown.
>    Comment: Discovered by Mike when testing fix for issue#1. I suspect it can be reproduced on 5.9 but is 
>    probably really hard as it involves ring expansion.

So lockdep complains with this:
      CPU0                    CPU1
      ----                    ----
 lock(lock#2);
                              local_irq_disable();
                              lock(&xhci->lock);
                              lock(lock#2);
 <Interrupt>
   lock(&xhci->lock);

(https://lore.kernel.org/linux-usb/1cbb8b7defb1db1d4747995c11ebebb3dd9a66ec.camel@xxxxxx/)

lockdep does not like that preload is used with disabled interrupts (in
non-interrupt context) and acquires lock#2 (the local_lock) which could
lead to a dead-lock as described above.
The flaw in the logic above is that lock#2 on CPU0 != CPU1.

We could make lockdep happy and remove what it observes on CPU1 by doing
this:

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 138ba4528dd3a..2cb8874c1c51f 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -197,12 +197,15 @@ static int xhci_insert_segment_mapping(struct radix_tree_root *trb_address_map,
 	if (radix_tree_lookup(trb_address_map, key))
 		return 0;
 
-	ret = radix_tree_maybe_preload(mem_flags);
-	if (ret)
-		return ret;
-	ret = radix_tree_insert(trb_address_map,
-			key, ring);
-	radix_tree_preload_end();
+	if (gfpflags_allow_blocking(mem_flags)) {
+		ret = radix_tree_maybe_preload(mem_flags);
+		if (ret)
+			return ret;
+	}
+	ret = radix_tree_insert(trb_address_map, key, ring);
+
+	if (gfpflags_allow_blocking(mem_flags))
+		radix_tree_preload_end();
 	return ret;
 }
 

There is no pre-load in GFP_ATOMIC case but it still acquires the
local_lock.
We might be able to drop radix_tree_maybe_preload(). I saw GFP_KERNEL
only during enumeration from xhci_alloc_streams(). Later, while
transfers were pending it came always from the scsi stack with disabled
interrupts and xhci->lock acquired.

> -Mathias

Sebastian