Re: [PATCH 3/9] usb: xhci: factor out roothub bandwidth cleanup

[Michal Pecio <michal.pecio@gmail.com>]

On Fri, 27 Mar 2026 13:34:34 +0100, Niklas Neronin wrote:
> Introduce xhci_rh_bw_cleanup() to release all bandwidth tracking
> structures associated with xHCI roothub ports.
> 
> The new helper clears:
>  * TT bandwidth entries
>  * Per-interval endpoint lists
> 
> This refactors and consolidates the existing per-port cleanup logic
> previously embedded in xhci_mem_cleanup(), reducing duplication and
> making the teardown sequence easier to follow.
> 
> The helper will also be reused for upcoming S4 resume handling.
> 
> Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
> ---
>  drivers/usb/host/xhci-mem.c | 50 +++++++++++++++++++++----------------
>  1 file changed, 29 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index 75bc1eb98b76..d4a9dbed8f16 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -1895,10 +1895,36 @@ void xhci_remove_secondary_interrupter(struct usb_hcd *hcd, struct xhci_interrup
>  }
>  EXPORT_SYMBOL_GPL(xhci_remove_secondary_interrupter);
>  
> +/* Cleanup roothub bandwidth data */
> +static void xhci_rh_bw_cleanup(struct xhci_hcd *xhci)
> +{
> +	struct xhci_root_port_bw_info *rh_bw;
> +	struct xhci_tt_bw_info *tt_info, *tt_next;
> +	struct list_head *eps, *ep, *ep_next;
> +
> +	for (int i = 0; i < xhci->max_ports; i++) {
> +		rh_bw = &xhci->rh_bw[i];
> +
> +		/* Clear and free all TT bandwidth entries */
> +		list_for_each_entry_safe(tt_info, tt_next, &rh_bw->tts, tt_list) {
> +			list_del(&tt_info->tt_list);
> +			kfree(tt_info);
> +		}

This loop is theoretically pointless, because each tt_info corresponds
to a hub virtual device, and all this stuff should have been destroyed
by xhci_free_virt_devices_depth_first() earlier.

If anything, it seems to paper over potential memory leaks in there.

> +
> +		/* Clear per-interval endpoint lists */
> +		for (int j = 0; j < XHCI_MAX_INTERVAL; j++) {
> +			eps = &rh_bw->bw_table.interval_bw[j].endpoints;
> +
> +			list_for_each_safe(ep, ep_next, eps)
> +				list_del_init(ep);
> +		}

This loop used to run before xhci_free_virt_devices_depth_first(), but
now it will run after. It seems that the endpoints here are virt_ep
which also should be gone already, so this loop likely does nothing
(empty list) or writes to virtual devices after free (somebody forgot
to unlink some endpoints from the list).

Do we trust xhci_free_virt_devices_depth_first() to work correctly?
If yes then it seems this whole function is unnecessary.

If not, perhaps delete this monstrosity and write a simpler cleanup:

1. for each slot_id
  - disable debugfs
  - free virt_device and child allocations but don't worry about tt_info
2. for each root hub port
  - free all tt_info allocations but don't worry about eps or vdevs

And since this will be used by reset on resume:

3. zero out DCBAA and xhci->devs
4. reinitialize xhci->rb_hw

> +	}
> +}
> +
>  void xhci_mem_cleanup(struct xhci_hcd *xhci)
>  {
>  	struct device	*dev = xhci_to_hcd(xhci)->self.sysdev;
> -	int i, j;
> +	int i;
>  
>  	cancel_delayed_work_sync(&xhci->cmd_timer);
>  
> @@ -1917,15 +1943,6 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
>  	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Freed command ring");
>  	xhci_cleanup_command_queue(xhci);
>  
> -	for (i = 0; i < xhci->max_ports && xhci->rh_bw; i++) {
> -		struct xhci_interval_bw_table *bwt = &xhci->rh_bw[i].bw_table;
> -		for (j = 0; j < XHCI_MAX_INTERVAL; j++) {
> -			struct list_head *ep = &bwt->interval_bw[j].endpoints;
> -			while (!list_empty(ep))
> -				list_del_init(ep->next);
> -		}
> -	}
> -
>  	for (i = xhci->max_slots; i > 0; i--)
>  		xhci_free_virt_devices_depth_first(xhci, i);
>  
> @@ -1959,18 +1976,9 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
>  
>  	scratchpad_free(xhci);
>  
> -	if (!xhci->rh_bw)
> -		goto no_bw;
> +	if (xhci->rh_bw)
> +		xhci_rh_bw_cleanup(xhci);
>  
> -	for (i = 0; i < xhci->max_ports; i++) {
> -		struct xhci_tt_bw_info *tt, *n;
> -		list_for_each_entry_safe(tt, n, &xhci->rh_bw[i].tts, tt_list) {
> -			list_del(&tt->tt_list);
> -			kfree(tt);
> -		}
> -	}
> -
> -no_bw:
>  	xhci->cmd_ring_reserved_trbs = 0;
>  	xhci->usb2_rhub.num_ports = 0;
>  	xhci->usb3_rhub.num_ports = 0;
> -- 
> 2.50.1
>