> -----Original Message-----
> From: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> Sent: Thursday, October 29, 2020 7:38 PM
> To: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx>
> Cc: Mike Galbraith <efault@xxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx;
> linux-usb@xxxxxxxxxxxxxxx; Hans de Goede <hdegoede@xxxxxxxxxx>; Jun Li
> <jun.li@xxxxxxx>
> Subject: Re: [PATCH 3/3] xhci: Don't create stream debugfs files with spinlock
> held.
>
> 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;
>
> This makes the warning go away here, I'm not 100% sure that this okay.
> One thing I noticed: free_streams() does not remove the debugfs file so:
> | $ cd /sys/bus/usb/drivers/uas
> | $ echo 2-1:1.0 > bind
> | $ echo 2-1:1.0 > unbind
>
> and the kernel log shows:
> |sd 7:0:0:0: [sdb] Synchronizing SCSI cache sd 7:0:0:0: [sdb]
> |Synchronize Cache(10) failed: Result: hostbyte=DID_ERROR
> |driverbyte=DRIVER_OK
> |debugfs: File 'stream_id' in directory 'ep07' already present!
> |debugfs: File 'stream_context_array' in directory 'ep07' already present!
> |debugfs: File 'stream_id' in directory 'ep07' already present!
> |debugfs: File 'stream_context_array' in directory 'ep07' already present!
> |scsi host7: uas
> |scsi 7:0:0:0: Direct-Access QEMU QEMU HARDDISK 2.5+ PQ: 0 ANSI:
> 5
> |sd 7:0:0:0: Attached scsi generic sg2 type 0 sd 7:0:0:0: [sdb] 262144
> |512-byte logical blocks: (134 MB/128 MiB) sd 7:0:0:0: [sdb] Write
> |Protect is off sd 7:0:0:0: [sdb] Mode Sense: 63 00 00 08 sd 7:0:0:0:
> |[sdb] Write cache: enabled, read cache: enabled, doesn't support DPO or
> |FUA sd 7:0:0:0: [sdb] Attached SCSI disk
This is because your change is using wrong *ep_index* when create
stream files, if you use the below patch from Mathias, you will not
have this problem.
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 482fe8c5e3b4..d4a8d0efbbc4 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3533,11 +3533,14 @@ 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++) {
+ ep_index = xhci_get_endpoint_index(&eps[i]->desc);
+ xhci_debugfs_create_stream_files(xhci, vdev, ep_index);
+ }
/* Subtract 1 for stream 0, which drivers can't use */
return num_streams - 1;
Li Jun
>
> > -Mathias
>
> Sebastian
