Re: [PATCH v24 05/34] usb: host: xhci-mem: Cleanup pending secondary event ring events

Amadeusz Sławiński
2024-08-06T14:50:15Z

On 8/1/2024 3:17 AM, Wesley Cheng wrote:
> As part of xHCI bus suspend, the XHCI is halted. However, if there are

XHCI -> xHCI

> pending events in the secondary event ring, it is observed that the xHCI
> controller stops responding to further commands upon host or device
> initiated bus resume. Iterate through all pending events and update the
> dequeue pointer to the beginning of the event ring.
>
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> ---
> drivers/usb/host/xhci-mem.c | 7 ++++++-
> drivers/usb/host/xhci-ring.c | 33 ++++++++++++++++++++++++++++++++-
> drivers/usb/host/xhci.c | 2 +-
> drivers/usb/host/xhci.h | 6 ++++++
> 4 files changed, 45 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index 2ca5937b73f4..60dfc59260d8 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -1816,7 +1816,7 @@ xhci_remove_interrupter(struct xhci_hcd *xhci, struct xhci_interrupter *ir)
> tmp &= ERST_SIZE_MASK;
> writel(tmp, &ir->ir_set->erst_size);
>
> - xhci_write_64(xhci, ERST_EHB, &ir->ir_set->erst_dequeue);
> + xhci_update_erst_dequeue(xhci, ir, true);
> }
> }
>
> @@ -1859,6 +1859,11 @@ void xhci_remove_secondary_interrupter(struct usb_hcd *hcd, struct xhci_interrup
> return;
> }
>
> + /*
> + * Cleanup secondary interrupter to ensure there are no pending events.
> + * This also updates event ring dequeue pointer back to the start.
> + */
> + xhci_skip_sec_intr_events(xhci, ir->event_ring, ir);
> intr_num = ir->intr_num;
>
> xhci_remove_interrupter(xhci, ir);
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 80dca780317a..a012ddf94fb5 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -3012,7 +3012,7 @@ static int xhci_handle_event_trb(struct xhci_hcd *xhci, struct xhci_interrupter
> * - When all events have finished
> * - To avoid "Event Ring Full Error" condition
> */
> -static void xhci_update_erst_dequeue(struct xhci_hcd *xhci,
> +void xhci_update_erst_dequeue(struct xhci_hcd *xhci,
> struct xhci_interrupter *ir,
> bool clear_ehb)
> {
> @@ -3112,6 +3112,37 @@ static int xhci_handle_events(struct xhci_hcd *xhci, struct xhci_interrupter *ir
> return 0;
> }
>
> +/*
> + * Move the event ring dequeue pointer to skip events kept in the secondary
> + * event ring. This is used to ensure that pending events in the ring are
> + * acknowledged, so the XHCI HCD can properly enter suspend/resume. The

XHCI -> xHCI

> + * secondary ring is typically maintained by an external component.
> + */
> +void xhci_skip_sec_intr_events(struct xhci_hcd *xhci,
> + struct xhci_ring *ring, struct xhci_interrupter *ir)
> +{
> + union xhci_trb *current_trb;
> + u64 erdp_reg;
> + dma_addr_t deq;
> +
> + /* disable irq, ack pending interrupt and ack all pending events */
> + xhci_disable_interrupter(ir);

That seems like a bit weird place to disable interrupter? Perhaps
suspend and resume functions should be updated to handle more
interrupters instead?