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

From: Mathias Nyman <mathias.nyman@linux.intel.com>
To: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Mike Galbraith <efault@gmx.de>,
	gregkh@linuxfoundation.org, linux-usb@vger.kernel.org,
	Hans de Goede <hdegoede@redhat.com>, Li Jun <jun.li@nxp.com>
Subject: Re: [PATCH 3/3] xhci: Don't create stream debugfs files with spinlock held.
Date: Thu, 29 Oct 2020 15:08:06 +0200	[thread overview]
Message-ID: <29cf8ca3-0fe7-da51-c8ae-ad5c67af4dde@linux.intel.com> (raw)
In-Reply-To: <20201029113805.tdsissnjq4acemle@linutronix.de>

On 29.10.2020 13.38, Sebastian Andrzej Siewior wrote:
> On 2020-10-29 13:22:20 [+0200], Mathias Nyman wrote:
>> On 29.10.2020 13.11, Mike Galbraith wrote:
>>> On Thu, 2020-10-29 at 11:41 +0200, Mathias Nyman wrote:
>>>> Can you check if this can be reproduced with 5.9 kernel?
>>>
>>> Nope, 5.9.2 didn't reproduce.
>>>
>>
>> Very odd.
>> It might be hard to reproduce as it requires xhci ring expansion to trigger it,
>> meaning at some point there is so much data queued to a device the current ring buffer
>> can't fit it.
>>
>> Other possibility is that there were some radix tree changes in 5.10-rc1, haven't looked into those.
> 
> This came with commit
>    673d74683627b ("usb: xhci: add debugfs support for ep with stream")
> 
> which is appeared in v5.10-rc1. This hunk below works around it:
> 
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 482fe8c5e3b47..699777fb523b6 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -3533,11 +3533,13 @@ static int xhci_alloc_streams(struct usb_hcd *hcd, struct usb_device *udev,
>  		xhci_dbg(xhci, "Slot %u ep ctx %u now has streams.\n",
>  			 udev->slot_id, ep_index);
>  		vdev->eps[ep_index].ep_state |= EP_HAS_STREAMS;
> -		xhci_debugfs_create_stream_files(xhci, vdev, ep_index);
>  	}
>  	xhci_free_command(xhci, config_cmd);
>  	spin_unlock_irqrestore(&xhci->lock, flags);
>  
> +	for (i = 0; i < num_eps; i++)
> +		xhci_debugfs_create_stream_files(xhci, vdev, ep_index);
> +
>  	/* Subtract 1 for stream 0, which drivers can't use */
>  	return num_streams - 1;
>  

Sebastian, I think you might be missing the first mail in this thread. 

https://lore.kernel.org/linux-usb/20201028203124.375344-4-mathias.nyman@linux.intel.com/

Let me do a quick recap to avoid confusion here. So far we have 2 issues.

1. Lockdep issue #1, stream debugfs entry created with local interrupts disabled and xhci->lock held
   Cause: 673d74683627b ("usb: xhci: add debugfs support for ep with stream") in v5.10-rc1
   Fix:  [PATCH 3/3] xhci: Don't create stream debugfs files with spinlock held. (i.e. the mail we are replying to)
   Comment: easily reproduced, enable lockdep and uas, mount debugfs and connect a UAS device.
            discovered by both Hans and Mike
.
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.

-Mathias