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

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

On Tue, 31 Mar 2026 12:59:50 +0300, Neronin, Niklas wrote:
> 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).

And this patch leaves command cleanup spread across suspend and resume.

If a future patch moves clearing the ring from suspend to resume then
it can also move completing and freeing stale commands with it.

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

And it's unnecessary because xhci_suspend() wiped the ring already.
Which means that xhci_init() calling ring_init() is unnecessary too.

> I would like to remove xhci_clear_command_ring() eventually,
> instead have:
> 
>    xhci_ring_reset()
>    ...
>    xhci_set_cmd_ring_deq()

Fair enough, in some cases one may be useful without the other.

> or
> 
>    xhci_ring_clear()
>    ...
>    xhci_ring_init()
>    xhci_set_cmd_ring_deq()

Not sure what ... would be, however?

If clear() doesn't include init(), is there ever a good reason to
maintain the ring in an invalid state? A state where forgetting
about ring_init() would cause the HC to escape the ring and IOMMU
fault or interpret random memory as commands?

Regards,
Michal