Re: [PATCH V4] usb: xhci: Add debugfs support for xHCI port bandwidth

[Mathias Nyman <mathias.nyman@linux.intel.com>]

On 6.3.2025 9.10, raoxu wrote:
> From: Xu Rao <raoxu@uniontech.com>
> 
> In many projects, you need to obtain the available bandwidth of the
> xhci roothub port. Refer to xhci rev1_2 and use the TRB_GET_BW
> command to obtain it.
> 
> Signed-off-by: Xu Rao <raoxu@uniontech.com>
> ---
> Changelog:
>   *v1->v2: modify the patch subject no code changes.
>    v2->v3: separate files in debugfs for each speed (SS HS FS).
> 	queue one command for each speed not queuing three commands on one file.
> 	print value from context not array on stack.
>    v3->v4: Fix compilation warnings for W=1 build. Delete unused variable
> ---
>   drivers/usb/host/xhci-debugfs.c | 93 +++++++++++++++++++++++++++++++++
>   drivers/usb/host/xhci-mem.c     |  8 +++
>   drivers/usb/host/xhci-ring.c    | 14 +++++
>   drivers/usb/host/xhci.c         | 26 +++++++++
>   drivers/usb/host/xhci.h         |  9 ++++
>   5 files changed, 150 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci-debugfs.c b/drivers/usb/host/xhci-debugfs.c
> index 1f5ef174abea..5751065d199c 100644
> --- a/drivers/usb/host/xhci-debugfs.c
> +++ b/drivers/usb/host/xhci-debugfs.c
> @@ -631,6 +631,97 @@ static void xhci_debugfs_create_ports(struct xhci_hcd *xhci,
>   	}
>   }
> 
> +static int xhci_port_bw_show(struct xhci_hcd *xhci, u8 dev_speed,
> +				struct seq_file *s)
> +{
> +	unsigned int			num_ports;
> +	unsigned int			i;
> +	int				ret;
> +	struct xhci_container_ctx	*ctx;
> +
> +	num_ports = HCS_MAX_PORTS(xhci->hcs_params1);
> +	ctx = xhci->get_bw_command->in_ctx;
> +

xHC might be runtime suspended when this debugfs file is read.
We should make sure xHC is running here by calling pm_runtime_get() or similar,
to make sure command can be processed.


> +	/* get roothub port bandwidth */
> +	ret = xhci_get_port_bandwidth(xhci, dev_speed);
> +	if (ret)
> +		return ret;
> +
> +	/* print all roothub ports available bandwidth */
> +	for (i = 1; i < num_ports+1; i++)
> +		seq_printf(s, "port[%d] available bw: %d%%.\n", i,
> +				ctx->bytes[i]);
> +
> +	return ret;
> +}
> +

...

> @@ -2490,6 +2494,10 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
>   	 */
>   	xhci->cmd_ring_reserved_trbs++;
> 
> +	xhci->get_bw_command = xhci_alloc_command_with_ctx(xhci, true, flags);
> +	if (!xhci->get_bw_command)
> +		goto fail;
> +

I think its better to create a new command structure with context for each time
we read port bandwidth instead of allocating one shared.

The port bandwidth won't be read at all in most cases, and sharing has
concurrency issues.

I'd suggest adding support for a new XHCI_CTX_TYPE_PORT_BW context type to
xhci_alloc_container_ctx(), which allocates and maps 256 bytes, 16byte aligned,
like xhci->small_streams_pool dma pool.

Then we could do something like:
(pm_runtime_get and put missing)

+static int xhci_port_bw_show(struct xhci_hcd *xhci, u8 dev_speed,
+                               struct seq_file *s)
+{
+       struct xhci_container_ctx *ctx;
+       unsigned int num_ports;
+       unsigned int i;
+       int ret;
+
+       num_ports = HCS_MAX_PORTS(xhci->hcs_params1);
+
+       ctx = xhci_alloc_container_ctx(xhci, XHCI_CTX_TYPE_PORT_BW, flags);
+       if (!ctx)
+               return -ENOMEM;
+
+       /* get roothub port bandwidth */
+       ret = xhci_get_port_bandwidth(xhci, ctx, dev_speed);
+       if (ret) {
+               xhci_free_container_ctx(xhci, ctx);
+               return ret;
+       }
+
+       /* print all roothub ports available bandwidth */
+       for (i = 1; i < num_ports + 1 && i < ctx->size; i++)
+               seq_printf(s, "port[%d] available bw: %d%%.\n", i,
+                          ctx->bytes[i]);
+
+       xhci_free_container_ctx(xhci, ctx);
+       return ret;
+}

and

+int xhci_get_port_bandwidth(struct xhci_hcd *xhci, struct xhci_container_ctx *ctx,
+                           u8 dev_speed)
+{
+       struct xhci_command *cmd;
+       unsigned long flags;
+       int ret;
+
+       if (!ctx || ctx->type != XHCI_CTX_TYPE_PORT_BW)
+               return -EINVAL;
+
+       cmd = xhci_alloc_command(xhci, true, GFP_KERNEL);
+       if (!cmd)
+                return -ENOMEM;
+
+       cmd->in_ctx = ctx;
+
+       /* get xhci port bandwidth, refer to xhci rev1_2 protocol 4.6.15 */
+       spin_lock_irqsave(&xhci->lock, flags);
+
+       ret = xhci_queue_get_port_bw(xhci, cmd, ctx->dma, dev_speed, 0);
+       if (ret) {
+               spin_unlock_irqrestore(&xhci->lock, flags);
+               goto err_out;
+       }
+       xhci_ring_cmd_db(xhci);
+       spin_unlock_irqrestore(&xhci->lock, flags);
+
+       wait_for_completion(cmd->completion);
+err_out:
+       kfree(cmd->completion);
+       kfree(cmd);
+
+       return ret;
+}

Thanks
Mathias