Re: [PATCH 9/9] usb: xhci: optimize resuming from S4 (suspend-to-disk)

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

On 30/03/2026 12.45, Michal Pecio wrote:
> 
>>  
>> -		xhci_dbg(xhci, "// Disabling event ring interrupts\n");
>> -		temp = readl(&xhci->op_regs->status);
>> -		writel((temp & ~0x1fff) | STS_EINT, &xhci->op_regs->status);
>> -		xhci_disable_interrupter(xhci, xhci->interrupters[0]);
>> +		cancel_delayed_work_sync(&xhci->cmd_timer);
>> +
>> +		/* Delete all remaining commands */
>> +		xhci_cleanup_command_queue(xhci);
> 
> Considering that xhci_suspend() clears the command ring anyway, it
> could probably do this too so we don't need to. 

It makes more sense to have all clearing in one place, instead of spread
out over suspend and resume. This will be addressed in the next patch set,
i.e. remove clearing from suspend (if possible).

> 
> BTW, debugfs/port_bandwidth interface queues commands and I'm not sure
> if it's synchronized in any way with power management. IOW, it might be 
> possible that command are pending at suspend, but I'm not sure.
> 
>> +
>> +		/* Clear data which is re-initilized during runtime */
>> +		xhci_for_each_ring_seg(xhci->interrupters[0]->event_ring->first_seg, seg)
>> +			memset(seg->trbs, 0, sizeof(union xhci_trb) * TRBS_PER_SEGMENT);
>> +
>> +		for (int i = xhci->max_slots; i > 0; i--)
>> +			xhci_free_virt_devices_depth_first(xhci, i);
> 
> This loop is a bit ugly, it could be a function in xhci-mem.

Will be addressed in xhci_free_virt_devices_depth_first() rework.

> 
>> +
>> +		xhci_rh_bw_cleanup(xhci);
>> +
>> +		xhci->cmd_ring_reserved_trbs = 0;
>> +		xhci_for_each_ring_seg(xhci->cmd_ring->first_seg, seg)
>> +			memset(seg->trbs, 0, sizeof(union xhci_trb) * TRBS_PER_SEGMENT);
> 
> This looks like a bug because it nukes link TRBs. I know that
> xhci_init() will fix this up (unless somebody changes that without
> updating here), but it looks confusing.
I would like to remove xhci_clear_command_ring() eventually,
instead have:

   xhci_ring_reset()
   ...
   xhci_set_cmd_ring_deq()

or

   xhci_ring_clear()
   ...
   xhci_ring_init()
   xhci_set_cmd_ring_deq()

Then xhci_ring_*() functions can be used for all rings,
and now it is clear that the CRP is set.

Thanks,
Niklas