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

["Neronin, Niklas" <niklas.neronin@linux.intel.com>]

On 30/03/2026 11.29, Michal Pecio wrote:
>> +/* 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).

In my testing, when xhci_rh_bw_cleanup() is called after
xhci_free_virt_devices_depth_first() in xhci_resume(), all related
resources have already been freed.
That said, I have chosen to keep the existing freeing in this patch set.
Removing it would introduce an additional behavioral change and a
potential regression point, which I prefer to avoid at this stage.

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

I don't, mostly because it is recursive and complex. Which makes it
difficult to follow edge case issues (IMO).

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

Planning to rework xhci_free_virt_devices_depth_first() in a later patch
set, which will then result in the removal of xhci_rh_bw_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