> -----Original Message----- > From: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx> > Sent: Monday, August 17, 2020 7:48 PM > To: Jun Li <jun.li@xxxxxxx>; mathias.nyman@xxxxxxxxx > Cc: gregkh@xxxxxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; dl-linux-imx > <linux-imx@xxxxxxx> > Subject: Re: [PATCH v2] usb: xhci: add debugfs support for ep with stream > > On 13.8.2020 12.57, Jun Li wrote: > > Hi > > > >> -----Original Message----- > >> From: Jun Li <jun.li@xxxxxxx> > >> Sent: Thursday, July 16, 2020 8:40 PM > >> To: mathias.nyman@xxxxxxxxx > >> Cc: gregkh@xxxxxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; > >> dl-linux-imx <linux-imx@xxxxxxx> > >> Subject: [PATCH v2] usb: xhci: add debugfs support for ep with stream > >> > >> To show the trb ring of streams, use the exsiting ring files of bulk > >> ep to show trb ring of one specific stream ID, which stream ID's trb > >> ring will be shown, is controlled by a new debugfs file stream_id, > >> this is to avoid to create a large number of dir for every allocate > >> stream IDs, another debugfs file stream_context_array is created to show all > the allocated stream context array entries. > >> > >> Signed-off-by: Li Jun <jun.li@xxxxxxx> > >> --- > >> chanages for v2: > >> - Drop stream files remove, the stream files will be removed > >> with ep dir removal, keep the ep but only remove streams > >> actually does not make sense in current code. > >> - Use the new_ring for show_ring pointer for non-zero ep. > >> > >> drivers/usb/host/xhci-debugfs.c | 112 > >> +++++++++++++++++++++++++++++++++++++++- > >> drivers/usb/host/xhci-debugfs.h | 10 ++++ > >> drivers/usb/host/xhci.c | 1 + > >> 3 files changed, 122 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/usb/host/xhci-debugfs.c > >> b/drivers/usb/host/xhci-debugfs.c index 65d8de4..708585c 100644 > >> --- a/drivers/usb/host/xhci-debugfs.c > >> +++ b/drivers/usb/host/xhci-debugfs.c > >> @@ -450,9 +450,14 @@ void xhci_debugfs_create_endpoint(struct xhci_hcd *xhci, > >> if (!epriv) > >> return; > >> > >> + if (dev->eps[ep_index].ring) > >> + epriv->show_ring = dev->eps[ep_index].ring; > >> + else > >> + epriv->show_ring = dev->eps[ep_index].new_ring; > >> + > >> snprintf(epriv->name, sizeof(epriv->name), "ep%02d", ep_index); > >> epriv->root = xhci_debugfs_create_ring_dir(xhci, > >> - &dev->eps[ep_index].ring, > >> + &epriv->show_ring, > >> epriv->name, > >> spriv->root); > >> spriv->eps[ep_index] = epriv; > >> @@ -474,6 +479,111 @@ void xhci_debugfs_remove_endpoint(struct xhci_hcd *xhci, > >> kfree(epriv); > >> } > >> > >> +static int xhci_stream_id_show(struct seq_file *s, void *unused) { > >> + struct xhci_ep_priv *epriv = s->private; > >> + > >> + if (!epriv->stream_info) > >> + return -EPERM; > >> + > >> + seq_printf(s, "Supported stream IDs are 1 ~ %d, trb ring to be > >> +shown is for > >> stream id %d\n", > > Minor change, but maybe something shorter like: > "Show stream ID %d trb ring, supported [1 - %d] Looks better, thanks. > > >> + epriv->stream_info->num_streams - 1, epriv->stream_id); > >> + > >> + return 0; > >> +} > >> + > >> +static int xhci_stream_id_open(struct inode *inode, struct file > >> +*file) { > >> + return single_open(file, xhci_stream_id_show, inode->i_private); } > >> + > >> +static ssize_t xhci_stream_id_write(struct file *file, const char __user > *ubuf, > >> + size_t count, loff_t *ppos) { > >> + struct seq_file *s = file->private_data; > >> + struct xhci_ep_priv *epriv = s->private; > >> + int ret; > >> + u16 stream_id; /* MaxPStreams + 1 <= 16 */ > >> + > >> + if (!epriv->stream_info) > >> + return -EPERM; > >> + > >> + /* Decimal number */ > >> + ret = kstrtou16_from_user(ubuf, count, 10, &stream_id); > >> + if (ret) > >> + return ret; > >> + > >> + if (stream_id == 0 || stream_id >= epriv->stream_info->num_streams) > >> + return -EINVAL; > >> + > >> + epriv->stream_id = stream_id; > >> + epriv->show_ring = epriv->stream_info->stream_rings[stream_id]; > >> + > >> + return count; > >> +} > >> + > >> +static const struct file_operations stream_id_fops = { > >> + .open = xhci_stream_id_open, > >> + .write = xhci_stream_id_write, > >> + .read = seq_read, > >> + .llseek = seq_lseek, > >> + .release = single_release, > >> +}; > >> + > >> +static int xhci_stream_context_array_show(struct seq_file *s, void > >> +*unused) { > >> + struct xhci_ep_priv *epriv = s->private; > >> + struct xhci_stream_ctx *stream_ctx; > >> + dma_addr_t dma; > >> + int id; > >> + > >> + if (!epriv->stream_info) > >> + return -EPERM; > >> + > >> + seq_printf(s, "Allocated %d streams and %d stream context array entries\n", > >> + epriv->stream_info->num_streams, > >> + epriv->stream_info->num_stream_ctxs); > >> + > >> + for (id = 0; id < epriv->stream_info->num_stream_ctxs; id++) { > >> + stream_ctx = epriv->stream_info->stream_ctx_array + id; > >> + dma = epriv->stream_info->ctx_array_dma + id * 16; > >> + if (id < epriv->stream_info->num_streams) > >> + seq_printf(s, "%pad stream id %d deq %016llx\n", &dma, > >> + id, le64_to_cpu(stream_ctx->stream_ring)); > >> + else > >> + seq_printf(s, "%pad stream context entry not used deq %016llx\n", > >> + &dma, le64_to_cpu(stream_ctx->stream_ring)); > >> + } > >> + > >> + return 0; > >> +} > >> +DEFINE_SHOW_ATTRIBUTE(xhci_stream_context_array); > >> + > >> +void xhci_debugfs_create_stream_files(struct xhci_hcd *xhci, > >> + struct xhci_virt_device *dev, > >> + int ep_index) > >> +{ > >> + struct xhci_slot_priv *spriv = dev->debugfs_private; > >> + struct xhci_ep_priv *epriv; > >> + > >> + if (!spriv || !spriv->eps[ep_index] || > >> + !dev->eps[ep_index].stream_info) > >> + return; > >> + > >> + epriv = spriv->eps[ep_index]; > >> + epriv->stream_info = dev->eps[ep_index].stream_info; > >> + > >> + /* Show trb ring of stream ID 1 by default */ > >> + epriv->stream_id = 1; > >> + epriv->show_ring = epriv->stream_info->stream_rings[1]; > >> + debugfs_create_file("stream_id", 0644, > >> + epriv->root, epriv, > >> + &stream_id_fops); > >> + debugfs_create_file("stream_context_array", 0444, > >> + epriv->root, epriv, > >> + &xhci_stream_context_array_fops); } > >> + > >> void xhci_debugfs_create_slot(struct xhci_hcd *xhci, int slot_id) { > >> struct xhci_slot_priv *priv; > >> diff --git a/drivers/usb/host/xhci-debugfs.h > >> b/drivers/usb/host/xhci-debugfs.h index f7a4e24..f3348da 100644 > >> --- a/drivers/usb/host/xhci-debugfs.h > >> +++ b/drivers/usb/host/xhci-debugfs.h > >> @@ -91,6 +91,9 @@ struct xhci_file_map { struct xhci_ep_priv { > >> char name[DEBUGFS_NAMELEN]; > >> struct dentry *root; > >> + struct xhci_stream_info *stream_info; > >> + struct xhci_ring *show_ring; > >> + unsigned int stream_id; > >> }; > >> > >> struct xhci_slot_priv { > >> @@ -113,6 +116,9 @@ void xhci_debugfs_create_endpoint(struct xhci_hcd > >> *xhci, void xhci_debugfs_remove_endpoint(struct xhci_hcd *xhci, > >> struct xhci_virt_device *virt_dev, > >> int ep_index); > >> +void xhci_debugfs_create_stream_files(struct xhci_hcd *xhci, > >> + struct xhci_virt_device *virt_dev, > >> + int ep_index); > >> #else > >> static inline void xhci_debugfs_init(struct xhci_hcd *xhci) { } > >> static inline void xhci_debugfs_exit(struct xhci_hcd *xhci) { } @@ > >> -128,6 +134,10 @@ static inline void xhci_debugfs_remove_endpoint(struct > xhci_hcd *xhci, > >> struct xhci_virt_device *virt_dev, > >> int ep_index) { } > >> +static inline void > >> +xhci_debugfs_create_stream_files(struct xhci_hcd *xhci, > >> + struct xhci_virt_device *virt_dev, > >> + int ep_index) { } > >> #endif /* CONFIG_DEBUG_FS */ > >> > >> #endif /* __LINUX_XHCI_DEBUGFS_H */ > >> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index > >> bee5dec..2d6584c 100644 > >> --- a/drivers/usb/host/xhci.c > >> +++ b/drivers/usb/host/xhci.c > >> @@ -3529,6 +3529,7 @@ 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); > >> -- > >> 2.7.4 > > > > A gentle ping... > > Looks good to me, If you approve I'll make the above change. Above change is fine for me. > It will take some time to actually test it, I haven't got a UAS or other stream > supporting usb device in my current location. > > Adding to queue to get some automated testing done on it. Thanks Li Jun > > -Mathias > >