[PATCH v2] usb: xhci: add debugfs support for ep with stream

[PATCH v2] usb: xhci: add debugfs support for ep with stream

[Li Jun]

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

RE: [PATCH v2] usb: xhci: add debugfs support for ep with stream

[Jun Li]

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

Re: [PATCH v2] usb: xhci: add debugfs support for ep with stream

[Mathias Nyman]

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 

 

RE: [PATCH v2] usb: xhci: add debugfs support for ep with stream

[Jun Li]

> -----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
> 
> 

Re: [PATCH v2] usb: xhci: add debugfs support for ep with stream

[Mathias Nyman]

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

RE: [PATCH v2] usb: xhci: add debugfs support for ep with stream

[Jun Li]

> -----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

Re: [PATCH v2] usb: xhci: add debugfs support for ep with stream

[Mathias Nyman]

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

RE: [PATCH v2] usb: xhci: add debugfs support for ep with stream

[Jun Li]

> -----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&amp;data=02%7C01%7
> Cjun.li%40nxp.com%7C73e4663a6f6641fbb8b308d84f3d021a%7C686ea1d3bc2b4c6fa92cd99
> c5c301635%7C0%7C0%7C637346470803922895&amp;sdata=i4DfCW8EVUSAWnzb8Ql4jPjAOD5wp
> tfbaMrO8vKvtDc%3D&amp;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

Re: [PATCH v2] usb: xhci: add debugfs support for ep with stream

[Mathias Nyman]

>> 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

RE: [PATCH v2] usb: xhci: add debugfs support for ep with stream

[Jun Li]

> -----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&amp;data=
> >> 02%7C01%7
> >> Cjun.li%40nxp.com%7C73e4663a6f6641fbb8b308d84f3d021a%7C686ea1d3bc2b4c
> >> 6fa92cd99
> >> c5c301635%7C0%7C0%7C637346470803922895&amp;sdata=i4DfCW8EVUSAWnzb8Ql4
> >> jPjAOD5wp
> >> tfbaMrO8vKvtDc%3D&amp;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

Re: [PATCH v2] usb: xhci: add debugfs support for ep with stream

[Mathias Nyman]

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&amp;data=
>>>> 02%7C01%7
>>>> Cjun.li%40nxp.com%7C73e4663a6f6641fbb8b308d84f3d021a%7C686ea1d3bc2b4c
>>>> 6fa92cd99
>>>> c5c301635%7C0%7C0%7C637346470803922895&amp;sdata=i4DfCW8EVUSAWnzb8Ql4
>>>> jPjAOD5wp
>>>> tfbaMrO8vKvtDc%3D&amp;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

RE: [PATCH v2] usb: xhci: add debugfs support for ep with stream

[Jun Li]

> -----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&amp;dat
> >>>> a=
> >>>> 02%7C01%7
> >>>> Cjun.li%40nxp.com%7C73e4663a6f6641fbb8b308d84f3d021a%7C686ea1d3bc2b
> >>>> 4c
> >>>> 6fa92cd99
> >>>> c5c301635%7C0%7C0%7C637346470803922895&amp;sdata=i4DfCW8EVUSAWnzb8Q
> >>>> l4
> >>>> jPjAOD5wp
> >>>> tfbaMrO8vKvtDc%3D&amp;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

Re: [PATCH v2] usb: xhci: add debugfs support for ep with stream

[Mathias Nyman]

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&amp;dat
>>>>>> a=
>>>>>> 02%7C01%7
>>>>>> Cjun.li%40nxp.com%7C73e4663a6f6641fbb8b308d84f3d021a%7C686ea1d3bc2b
>>>>>> 4c
>>>>>> 6fa92cd99
>>>>>> c5c301635%7C0%7C0%7C637346470803922895&amp;sdata=i4DfCW8EVUSAWnzb8Q
>>>>>> l4
>>>>>> jPjAOD5wp
>>>>>> tfbaMrO8vKvtDc%3D&amp;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