| Message ID | 1594901321-6992-1-git-send-email-jun.li@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] usb: xhci: add debugfs support for ep with stream | expand |
Hi > -----Original Message----- > From: Jun Li <jun.li@nxp.com> > Sent: Thursday, July 16, 2020 8:40 PM > To: mathias.nyman@intel.com > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; dl-linux-imx > <linux-imx@nxp.com> > 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@nxp.com> > --- > 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", > + 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... Thanks Li Jun
On 13.8.2020 12.57, Jun Li wrote: > Hi > >> -----Original Message----- >> From: Jun Li <jun.li@nxp.com> >> Sent: Thursday, July 16, 2020 8:40 PM >> To: mathias.nyman@intel.com >> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; dl-linux-imx >> <linux-imx@nxp.com> >> 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@nxp.com> >> --- >> 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] >> + 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. 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. -Mathias
> -----Original Message----- > From: Mathias Nyman <mathias.nyman@linux.intel.com> > Sent: Monday, August 17, 2020 7:48 PM > To: Jun Li <jun.li@nxp.com>; mathias.nyman@intel.com > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; dl-linux-imx > <linux-imx@nxp.com> > 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@nxp.com> > >> Sent: Thursday, July 16, 2020 8:40 PM > >> To: mathias.nyman@intel.com > >> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; > >> dl-linux-imx <linux-imx@nxp.com> > >> 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@nxp.com> > >> --- > >> 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 > >
On 18.8.2020 4.54, Jun Li wrote: > > >> -----Original Message----- >> From: Mathias Nyman <mathias.nyman@linux.intel.com> >> Sent: Monday, August 17, 2020 7:48 PM >> To: Jun Li <jun.li@nxp.com>; mathias.nyman@intel.com >> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; dl-linux-imx >> <linux-imx@nxp.com> >> 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@nxp.com> >>>> Sent: Thursday, July 16, 2020 8:40 PM >>>> To: mathias.nyman@intel.com >>>> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; >>>> dl-linux-imx <linux-imx@nxp.com> >>>> 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@nxp.com> >>>> --- >>>> 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; >>>> + Ran some tests and the I suspect the above code causes issues. If an endpoint is dropped and added back the above code will store the old ring in epriv->show_ring as we have both a .ring and .new_ring present at that moment. Soon after this the old ring pointed to by .ring will be freed, and .ring = .new_ring will be set. Old code showed whatever ring buffer eps[i].ring pointer pointed to when the sysfs file was read, (as we saved the address, &eps[i].ring). I see now that it in theory it had a small gap before .ring = .new_ring was set where user could read the ring buffer and .ring would still be a NULL pointer. That needs to be fixed as well. I still like the old way of using double pointer more. xhci driver will also dig out the current ring from eps[i].ring, so I think we should as well. (in stream case it would be &ep->stream_info->stream_rings[stream_id]) -Mathias
> -----Original Message----- > From: Mathias Nyman <mathias.nyman@linux.intel.com> > Sent: Monday, August 31, 2020 9:11 PM > To: Jun Li <jun.li@nxp.com>; mathias.nyman@intel.com > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; dl-linux-imx > <linux-imx@nxp.com> > Subject: Re: [PATCH v2] usb: xhci: add debugfs support for ep with stream > > On 18.8.2020 4.54, Jun Li wrote: > > > > > >> -----Original Message----- > >> From: Mathias Nyman <mathias.nyman@linux.intel.com> > >> Sent: Monday, August 17, 2020 7:48 PM > >> To: Jun Li <jun.li@nxp.com>; mathias.nyman@intel.com > >> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; > >> dl-linux-imx <linux-imx@nxp.com> > >> 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@nxp.com> > >>>> Sent: Thursday, July 16, 2020 8:40 PM > >>>> To: mathias.nyman@intel.com > >>>> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; > >>>> dl-linux-imx <linux-imx@nxp.com> > >>>> 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@nxp.com> > >>>> --- > >>>> 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; > >>>> + > > Ran some tests and the I suspect the above code causes issues. > > If an endpoint is dropped and added back the above code will store the old ring > in epriv->show_ring as we have both a .ring and .new_ring present at that moment. > Soon after this the old ring pointed to by .ring will be freed, and .ring = .new_ring > will be set. Yes, in this case, eps[ep_index].ring still keeps the old ring address, but eps[ep_index].new_ring is pointing to the new/correct ring, my patch will store the old ring address. > > Old code showed whatever ring buffer eps[i].ring pointer pointed ,to when the sysfs > file was read, (as we saved the address, &eps[i].ring). I see now that it in theory > it had a small gap before .ring = .new_ring was set where user could read the ring > buffer and .ring would still be a NULL pointer. > That needs to be fixed as well. Yes, also in above drop-then-add-back case the old code eps[i].ring still points to the old ring. So considering we are creating debugfs file for ep before .ring = .new_ring; .new_ring = NULL; A solution of firstly check .new_ring seems can resolve the problems here: if (dev->eps[ep_index].new_ring) /* For first add of EP; or drop-then-add-back EP */ epriv->show_ring = dev->eps[ep_index].new_ring; else /* For EP0 */ epriv->show_ring = dev->eps[ep_index].ring; > > I still like the old way of using double pointer more. > xhci driver will also dig out the current ring from eps[i].ring, so I think we should > as well. > (in stream case it would be &ep->stream_info->stream_rings[stream_id]) stream case should no problem as it is epriv->show_ring = ep->stream_info->stream_rings[stream_id]; thanks Li Jun > > -Mathias
Hi >>>>>> 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; >>>>>> + >> >> Ran some tests and the I suspect the above code causes issues. >> >> If an endpoint is dropped and added back the above code will store the old ring >> in epriv->show_ring as we have both a .ring and .new_ring present at that moment. >> Soon after this the old ring pointed to by .ring will be freed, and .ring = .new_ring >> will be set. > > Yes, in this case, eps[ep_index].ring still keeps the old ring address, but > eps[ep_index].new_ring is pointing to the new/correct ring, my patch will > store the old ring address. > >> >> Old code showed whatever ring buffer eps[i].ring pointer pointed ,to when the sysfs >> file was read, (as we saved the address, &eps[i].ring). I see now that it in theory >> it had a small gap before .ring = .new_ring was set where user could read the ring >> buffer and .ring would still be a NULL pointer. >> That needs to be fixed as well. > > Yes, also in above drop-then-add-back case the old code eps[i].ring still points to > the old ring. yes, but only until for a short while until .ring = .new_ring is set. > > So considering we are creating debugfs file for ep before > .ring = .new_ring; > .new_ring = NULL; > > A solution of firstly check .new_ring seems can resolve the problems here: > > if (dev->eps[ep_index].new_ring) > /* For first add of EP; or drop-then-add-back EP */ > epriv->show_ring = dev->eps[ep_index].new_ring; > else > /* For EP0 */ > epriv->show_ring = dev->eps[ep_index].ring; > I think this debugfs code is just called too early. It shouldn't need to check new_ring pointer at all. I wrote a fix that changes the order and makes sure endpoint is enabled and ring pointer is set correctly before we call xhci_debugfs_create_endpoint() https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/commit/?h=for-usb-linus&id=cf99aef5624a592fd4f3374c7060bfa1a65f15df I think this streams support should be added on top of that >> >> I still like the old way of using double pointer more. >> xhci driver will also dig out the current ring from eps[i].ring, so I think we should >> as well. >> (in stream case it would be &ep->stream_info->stream_rings[stream_id]) > > stream case should no problem as it is > epriv->show_ring = ep->stream_info->stream_rings[stream_id]; Sounds good -Mathias
> -----Original Message----- > From: Mathias Nyman <mathias.nyman@linux.intel.com> > Sent: Wednesday, September 2, 2020 8:41 PM > To: Jun Li <jun.li@nxp.com>; mathias.nyman@intel.com > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; dl-linux-imx > <linux-imx@nxp.com> > Subject: Re: [PATCH v2] usb: xhci: add debugfs support for ep with stream > > Hi > > >>>>>> 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; > >>>>>> + > >> > >> Ran some tests and the I suspect the above code causes issues. > >> > >> If an endpoint is dropped and added back the above code will store > >> the old ring in epriv->show_ring as we have both a .ring and .new_ring present > at that moment. > >> Soon after this the old ring pointed to by .ring will be freed, and > >> .ring = .new_ring will be set. > > > > Yes, in this case, eps[ep_index].ring still keeps the old ring > > address, but eps[ep_index].new_ring is pointing to the new/correct > > ring, my patch will store the old ring address. > > > >> > >> Old code showed whatever ring buffer eps[i].ring pointer pointed ,to > >> when the sysfs file was read, (as we saved the address, > >> &eps[i].ring). I see now that it in theory it had a small gap before > >> .ring = .new_ring was set where user could read the ring buffer and .ring would > still be a NULL pointer. > >> That needs to be fixed as well. > > > > Yes, also in above drop-then-add-back case the old code eps[i].ring > > still points to the old ring. > > yes, but only until for a short while until .ring = .new_ring is set. > > > > > So considering we are creating debugfs file for ep before .ring = > > .new_ring; .new_ring = NULL; > > > > A solution of firstly check .new_ring seems can resolve the problems here: > > > > if (dev->eps[ep_index].new_ring) > > /* For first add of EP; or drop-then-add-back EP */ > > epriv->show_ring = dev->eps[ep_index].new_ring; else > > /* For EP0 */ > > epriv->show_ring = dev->eps[ep_index].ring; > > > > I think this debugfs code is just called too early. It shouldn't need to check new_ring > pointer at all. > > I wrote a fix that changes the order and makes sure endpoint is enabled and ring > pointer is set correctly before we call xhci_debugfs_create_endpoint() > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.o > rg%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fmnyman%2Fxhci.git%2Fcommit%2F%3Fh%3Dfo > r-usb-linus%26id%3Dcf99aef5624a592fd4f3374c7060bfa1a65f15df&data=02%7C01%7 > Cjun.li%40nxp.com%7C73e4663a6f6641fbb8b308d84f3d021a%7C686ea1d3bc2b4c6fa92cd99 > c5c301635%7C0%7C0%7C637346470803922895&sdata=i4DfCW8EVUSAWnzb8Ql4jPjAOD5wp > tfbaMrO8vKvtDc%3D&reserved=0 This is a good place for non-zero Eps, but does not cover ep0. Li Jun > > I think this streams support should be added on top of that > >> > >> I still like the old way of using double pointer more. > >> xhci driver will also dig out the current ring from eps[i].ring, so I > >> think we should as well. > >> (in stream case it would be > >> &ep->stream_info->stream_rings[stream_id]) > > > > stream case should no problem as it is > > epriv->show_ring = ep->stream_info->stream_rings[stream_id]; > > Sounds good > > -Mathias
>> I think this debugfs code is just called too early. It shouldn't need to check new_ring >> pointer at all. >> >> I wrote a fix that changes the order and makes sure endpoint is enabled and ring >> pointer is set correctly before we call xhci_debugfs_create_endpoint() >> >> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.o >> rg%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fmnyman%2Fxhci.git%2Fcommit%2F%3Fh%3Dfo >> r-usb-linus%26id%3Dcf99aef5624a592fd4f3374c7060bfa1a65f15df&data=02%7C01%7 >> Cjun.li%40nxp.com%7C73e4663a6f6641fbb8b308d84f3d021a%7C686ea1d3bc2b4c6fa92cd99 >> c5c301635%7C0%7C0%7C637346470803922895&sdata=i4DfCW8EVUSAWnzb8Ql4jPjAOD5wp >> tfbaMrO8vKvtDc%3D&reserved=0 > > This is a good place for non-zero Eps, but does not cover ep0. > ep0 is special, it's not touched in these add/drop endpoint or check bandwidth functions. ep0 ring is allocated earlier during slot creation in xhci_alloc_virt_device() ... /* Allocate endpoint 0 ring */ dev->eps[0].ring = xhci_ring_alloc(xhci, 2, 1, TYPE_CTRL, 0, flags); and for debugfs ep00 is added manually together with the slot xhci_debugfs_create_slot() ... xhci_debugfs_create_ring_dir(xhci, &dev->eps[0].ring, "ep00", priv->root); So regarding ep0 the change should be ok. -Mathias
> -----Original Message----- > From: Mathias Nyman <mathias.nyman@linux.intel.com> > Sent: Thursday, September 3, 2020 3:24 PM > To: Jun Li <jun.li@nxp.com>; mathias.nyman@intel.com > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; dl-linux-imx > <linux-imx@nxp.com> > Subject: Re: [PATCH v2] usb: xhci: add debugfs support for ep with stream > > >> I think this debugfs code is just called too early. It shouldn't need > >> to check new_ring pointer at all. > >> > >> I wrote a fix that changes the order and makes sure endpoint is > >> enabled and ring pointer is set correctly before we call > >> xhci_debugfs_create_endpoint() > >> > >> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit > >> .kernel.o > >> rg%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fmnyman%2Fxhci.git%2Fcommit%2F > >> %3Fh%3Dfo > >> r-usb-linus%26id%3Dcf99aef5624a592fd4f3374c7060bfa1a65f15df&data= > >> 02%7C01%7 > >> Cjun.li%40nxp.com%7C73e4663a6f6641fbb8b308d84f3d021a%7C686ea1d3bc2b4c > >> 6fa92cd99 > >> c5c301635%7C0%7C0%7C637346470803922895&sdata=i4DfCW8EVUSAWnzb8Ql4 > >> jPjAOD5wp > >> tfbaMrO8vKvtDc%3D&reserved=0 > > > > This is a good place for non-zero Eps, but does not cover ep0. > > > > ep0 is special, it's not touched in these add/drop endpoint or check bandwidth > functions. > > ep0 ring is allocated earlier during slot creation in > xhci_alloc_virt_device() > ... > /* Allocate endpoint 0 ring */ > dev->eps[0].ring = xhci_ring_alloc(xhci, 2, 1, TYPE_CTRL, 0, flags); > > and for debugfs ep00 is added manually together with the slot > xhci_debugfs_create_slot() > ... > xhci_debugfs_create_ring_dir(xhci, &dev->eps[0].ring, "ep00", priv->root); > > So regarding ep0 the change should be ok. Sorry, I forgot debugfs of ep0 is created via xhci_debugfs_create_slot(). Then I think your change is OK, also I gave a test with my stream/UAS device on top of your patch and it can work fine. Do you need I post a new version of my patch(to remove touch of .new_ring)? Thanks Li Jun > > -Mathias
On 3.9.2020 10.46, Jun Li wrote: > >> -----Original Message----- >> From: Mathias Nyman <mathias.nyman@linux.intel.com> >> Sent: Thursday, September 3, 2020 3:24 PM >> To: Jun Li <jun.li@nxp.com>; mathias.nyman@intel.com >> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; dl-linux-imx >> <linux-imx@nxp.com> >> Subject: Re: [PATCH v2] usb: xhci: add debugfs support for ep with stream >> >>>> I think this debugfs code is just called too early. It shouldn't need >>>> to check new_ring pointer at all. >>>> >>>> I wrote a fix that changes the order and makes sure endpoint is >>>> enabled and ring pointer is set correctly before we call >>>> xhci_debugfs_create_endpoint() >>>> >>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit >>>> .kernel.o >>>> rg%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fmnyman%2Fxhci.git%2Fcommit%2F >>>> %3Fh%3Dfo >>>> r-usb-linus%26id%3Dcf99aef5624a592fd4f3374c7060bfa1a65f15df&data= >>>> 02%7C01%7 >>>> Cjun.li%40nxp.com%7C73e4663a6f6641fbb8b308d84f3d021a%7C686ea1d3bc2b4c >>>> 6fa92cd99 >>>> c5c301635%7C0%7C0%7C637346470803922895&sdata=i4DfCW8EVUSAWnzb8Ql4 >>>> jPjAOD5wp >>>> tfbaMrO8vKvtDc%3D&reserved=0 >>> >>> This is a good place for non-zero Eps, but does not cover ep0. >>> >> >> ep0 is special, it's not touched in these add/drop endpoint or check bandwidth >> functions. >> >> ep0 ring is allocated earlier during slot creation in >> xhci_alloc_virt_device() >> ... >> /* Allocate endpoint 0 ring */ >> dev->eps[0].ring = xhci_ring_alloc(xhci, 2, 1, TYPE_CTRL, 0, flags); >> >> and for debugfs ep00 is added manually together with the slot >> xhci_debugfs_create_slot() >> ... >> xhci_debugfs_create_ring_dir(xhci, &dev->eps[0].ring, "ep00", priv->root); >> >> So regarding ep0 the change should be ok. > > Sorry, I forgot debugfs of ep0 is created via xhci_debugfs_create_slot(). > > Then I think your change is OK, also I gave a test with my stream/UAS device > on top of your patch and it can work fine. > > Do you need I post a new version of my patch(to remove touch of .new_ring)? If you could yes, and also change to a double pointer: struct xhci_ep_priv { ... + struct xhci_ring **show_ring; }; And modify other places this change affects, such as: epriv->show_ring = &dev->eps[ep_index].ring; -Mathias
> -----Original Message----- > From: Mathias Nyman <mathias.nyman@linux.intel.com> > Sent: Thursday, September 3, 2020 5:39 PM > To: Jun Li <jun.li@nxp.com>; mathias.nyman@intel.com > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; dl-linux-imx > <linux-imx@nxp.com> > Subject: Re: [PATCH v2] usb: xhci: add debugfs support for ep with stream > > On 3.9.2020 10.46, Jun Li wrote: > > > >> -----Original Message----- > >> From: Mathias Nyman <mathias.nyman@linux.intel.com> > >> Sent: Thursday, September 3, 2020 3:24 PM > >> To: Jun Li <jun.li@nxp.com>; mathias.nyman@intel.com > >> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; > >> dl-linux-imx <linux-imx@nxp.com> > >> Subject: Re: [PATCH v2] usb: xhci: add debugfs support for ep with > >> stream > >> > >>>> I think this debugfs code is just called too early. It shouldn't > >>>> need to check new_ring pointer at all. > >>>> > >>>> I wrote a fix that changes the order and makes sure endpoint is > >>>> enabled and ring pointer is set correctly before we call > >>>> xhci_debugfs_create_endpoint() > >>>> > >>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fg > >>>> it > >>>> .kernel.o > >>>> rg%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fmnyman%2Fxhci.git%2Fcommit% > >>>> 2F > >>>> %3Fh%3Dfo > >>>> r-usb-linus%26id%3Dcf99aef5624a592fd4f3374c7060bfa1a65f15df&dat > >>>> a= > >>>> 02%7C01%7 > >>>> Cjun.li%40nxp.com%7C73e4663a6f6641fbb8b308d84f3d021a%7C686ea1d3bc2b > >>>> 4c > >>>> 6fa92cd99 > >>>> c5c301635%7C0%7C0%7C637346470803922895&sdata=i4DfCW8EVUSAWnzb8Q > >>>> l4 > >>>> jPjAOD5wp > >>>> tfbaMrO8vKvtDc%3D&reserved=0 > >>> > >>> This is a good place for non-zero Eps, but does not cover ep0. > >>> > >> > >> ep0 is special, it's not touched in these add/drop endpoint or check > >> bandwidth functions. > >> > >> ep0 ring is allocated earlier during slot creation in > >> xhci_alloc_virt_device() > >> ... > >> /* Allocate endpoint 0 ring */ > >> dev->eps[0].ring = xhci_ring_alloc(xhci, 2, 1, TYPE_CTRL, 0, > >> flags); > >> > >> and for debugfs ep00 is added manually together with the slot > >> xhci_debugfs_create_slot() > >> ... > >> xhci_debugfs_create_ring_dir(xhci, &dev->eps[0].ring, "ep00", > >> priv->root); > >> > >> So regarding ep0 the change should be ok. > > > > Sorry, I forgot debugfs of ep0 is created via xhci_debugfs_create_slot(). > > > > Then I think your change is OK, also I gave a test with my stream/UAS > > device on top of your patch and it can work fine. > > > > Do you need I post a new version of my patch(to remove touch of .new_ring)? > > If you could yes, and also change to a double pointer: > > struct xhci_ep_priv { > ... > + struct xhci_ring **show_ring; With current: struct xhci_ring *show_ring; As I use one trb file to show different trb rings for one EP, so I need get the addr of trb ring pointed by show_ring(which can be updated). If I change it to be **show_ring, then I am passing the addr of dev->eps[i].ring to debugfs so I can't use show_ring's update value when show trb, makes me not easy to get the addr of target trb ring. Thanks Li Jun > }; > > And modify other places this change affects, such as: > epriv->show_ring = &dev->eps[ep_index].ring; > > -Mathias
On 4.9.2020 13.01, Jun Li wrote: > > >> -----Original Message----- >> From: Mathias Nyman <mathias.nyman@linux.intel.com> >> Sent: Thursday, September 3, 2020 5:39 PM >> To: Jun Li <jun.li@nxp.com>; mathias.nyman@intel.com >> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; dl-linux-imx >> <linux-imx@nxp.com> >> Subject: Re: [PATCH v2] usb: xhci: add debugfs support for ep with stream >> >> On 3.9.2020 10.46, Jun Li wrote: >>> >>>> -----Original Message----- >>>> From: Mathias Nyman <mathias.nyman@linux.intel.com> >>>> Sent: Thursday, September 3, 2020 3:24 PM >>>> To: Jun Li <jun.li@nxp.com>; mathias.nyman@intel.com >>>> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; >>>> dl-linux-imx <linux-imx@nxp.com> >>>> Subject: Re: [PATCH v2] usb: xhci: add debugfs support for ep with >>>> stream >>>> >>>>>> I think this debugfs code is just called too early. It shouldn't >>>>>> need to check new_ring pointer at all. >>>>>> >>>>>> I wrote a fix that changes the order and makes sure endpoint is >>>>>> enabled and ring pointer is set correctly before we call >>>>>> xhci_debugfs_create_endpoint() >>>>>> >>>>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fg >>>>>> it >>>>>> .kernel.o >>>>>> rg%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fmnyman%2Fxhci.git%2Fcommit% >>>>>> 2F >>>>>> %3Fh%3Dfo >>>>>> r-usb-linus%26id%3Dcf99aef5624a592fd4f3374c7060bfa1a65f15df&dat >>>>>> a= >>>>>> 02%7C01%7 >>>>>> Cjun.li%40nxp.com%7C73e4663a6f6641fbb8b308d84f3d021a%7C686ea1d3bc2b >>>>>> 4c >>>>>> 6fa92cd99 >>>>>> c5c301635%7C0%7C0%7C637346470803922895&sdata=i4DfCW8EVUSAWnzb8Q >>>>>> l4 >>>>>> jPjAOD5wp >>>>>> tfbaMrO8vKvtDc%3D&reserved=0 >>>>> >>>>> This is a good place for non-zero Eps, but does not cover ep0. >>>>> >>>> >>>> ep0 is special, it's not touched in these add/drop endpoint or check >>>> bandwidth functions. >>>> >>>> ep0 ring is allocated earlier during slot creation in >>>> xhci_alloc_virt_device() >>>> ... >>>> /* Allocate endpoint 0 ring */ >>>> dev->eps[0].ring = xhci_ring_alloc(xhci, 2, 1, TYPE_CTRL, 0, >>>> flags); >>>> >>>> and for debugfs ep00 is added manually together with the slot >>>> xhci_debugfs_create_slot() >>>> ... >>>> xhci_debugfs_create_ring_dir(xhci, &dev->eps[0].ring, "ep00", >>>> priv->root); >>>> >>>> So regarding ep0 the change should be ok. >>> >>> Sorry, I forgot debugfs of ep0 is created via xhci_debugfs_create_slot(). >>> >>> Then I think your change is OK, also I gave a test with my stream/UAS >>> device on top of your patch and it can work fine. >>> >>> Do you need I post a new version of my patch(to remove touch of .new_ring)? >> >> If you could yes, and also change to a double pointer: >> >> struct xhci_ep_priv { >> ... >> + struct xhci_ring **show_ring; > > With current: struct xhci_ring *show_ring; > As I use one trb file to show different trb rings for one EP, so I need get > the addr of trb ring pointed by show_ring(which can be updated). > > If I change it to be **show_ring, then I am passing the addr of dev->eps[i].ring > to debugfs so I can't use show_ring's update value when show trb, makes me > not easy to get the addr of target trb ring. Right, ok, lets not use the douple pointer. We just have to trust epriv->show_ring is up to date. -Mathias
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", + 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);
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@nxp.com> --- 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(-)