Re: [PATCH v4 04/32] usb: host: xhci-mem: Cleanup pending secondary event ring events

[Mathias Nyman]

On 25.7.2023 5.33, Wesley Cheng wrote:
> As part of xHCI bus suspend, the XHCI is halted.  However, if there are
> 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 updating the
> dequeue pointer to the last pending event trb.
>
> Signed-off-by: Wesley Cheng <quic_wcheng@xxxxxxxxxxx>
> ---
>   drivers/usb/host/xhci-mem.c | 74 ++++++++++++++++++++++++++++++++++---
>   1 file changed, 69 insertions(+), 5 deletions(-)

This sounds more like ring handling code.
Maybe xhci-ring.c would be a better place

> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index c51150af22f2..6b01d56c176f 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -1799,17 +1799,85 @@ int xhci_alloc_erst(struct xhci_hcd *xhci,
>   	return 0;
>   }
>   
> +static void xhci_handle_sec_intr_events(struct xhci_hcd *xhci,
> +	struct xhci_ring *ring,	struct xhci_intr_reg __iomem *ir_set,
> +	struct xhci_erst *erst)
> +{

The function name is a bit misleading as it doesn't handle
any of the pending events, it just marks them all handled.

> +	union xhci_trb *erdp_trb, *current_trb;
> +	struct xhci_segment	*seg;
> +	u64 erdp_reg;
> +	u32 iman_reg;
> +	dma_addr_t deq;
> +	unsigned long segment_offset;
> +
> +	/* disable irq, ack pending interrupt and ack all pending events */
> +	iman_reg = readl_relaxed(&ir_set->irq_pending);
> +	iman_reg &= ~IMAN_IE;
> +	writel_relaxed(iman_reg, &ir_set->irq_pending);
> +	iman_reg = readl_relaxed(&ir_set->irq_pending);
> +	if (iman_reg & IMAN_IP)
> +		writel_relaxed(iman_reg, &ir_set->irq_pending);

maybe use xhci_disable_interrupter() helper, it does most of this already.

> +
> +	/* last acked event trb is in erdp reg  */
> +	erdp_reg = xhci_read_64(xhci, &ir_set->erst_dequeue);
> +	deq = (dma_addr_t)(erdp_reg & ~ERST_PTR_MASK);
> +	if (!deq) {
> +		xhci_dbg(xhci, "event ring handling not required\n");
> +		return;
> +	}
> +
> +	seg = ring->first_seg;
> +	segment_offset = deq - seg->dma;
> +
> +	/* find out virtual address of the last acked event trb */
> +	erdp_trb = current_trb = &seg->trbs[0] +
> +				(segment_offset/sizeof(*current_trb));
> +
> +	/* read cycle state of the last acked trb to find out CCS */
> +	ring->cycle_state = le32_to_cpu(current_trb->event_cmd.flags) & TRB_CYCLE;
> +
> +	while (1) {
> +		/* last trb of the event ring: toggle cycle state */
> +		if (current_trb == &seg->trbs[TRBS_PER_SEGMENT - 1]) {
> +			ring->cycle_state ^= 1;
> +			current_trb = &seg->trbs[0];
> +		} else {
> +			current_trb++;
> +		}
> +
> +		/* cycle state transition */
> +		if ((le32_to_cpu(current_trb->event_cmd.flags) & TRB_CYCLE) !=
> +		    ring->cycle_state)
> +			break;
> +	}
> +
> +	if (erdp_trb != current_trb) {
> +		deq = xhci_trb_virt_to_dma(ring->deq_seg, current_trb);
> +		if (deq == 0)
> +			xhci_warn(xhci,
> +				"WARN invalid SW event ring dequeue ptr.\n");
> +		/* Update HC event ring dequeue pointer */
> +		erdp_reg &= ERST_PTR_MASK;
> +		erdp_reg |= ((u64) deq & (u64) ~ERST_PTR_MASK);
> +	}
> +
> +	/* Clear the event handler busy flag (RW1C); event ring is empty. */
> +	erdp_reg |= ERST_EHB;
> +	xhci_write_64(xhci, erdp_reg, &ir_set->erst_dequeue);

There are some helpers like inc_deq() and  xhci_update_erst_dequeue()
that could be used here.

> +}
> +
>   static void
>   xhci_free_interrupter(struct xhci_hcd *xhci, struct xhci_interrupter *ir)
>   {
>   	struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
>   	size_t erst_size;
> -	u64 tmp64;
>   	u32 tmp;
>   
>   	if (!ir)
>   		return;
>   
> +	xhci_handle_sec_intr_events(xhci, ir->event_ring, ir->ir_set, &ir->erst);

Cleaning up the interrupter event ring should be called earlier.
 
Probably from xhci_remove_secondary_interrupter(), before it calls xhci_free_interrupter()

Thanks
-Mathias