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

[Amadeusz Sławiński <amadeuszx.slawinski@xxxxxxxxxxxxxxx>]

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@xxxxxxxxxxx>
> ---
>   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?