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

[Wesley Cheng <quic_wcheng@quicinc.com>]

Hi Mathias,

On 1/4/2024 6:48 AM, Mathias Nyman wrote:
> On 2.1.2024 23.45, 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 update the
>> dequeue pointer to the beginning of the event ring.
>>
>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> ...
>> +/*
>> + * 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
>> + * 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 *erdp_trb, *current_trb;
>> +    u64 erdp_reg;
>> +    u32 iman_reg;
>> +    dma_addr_t deq;
>> +
>> +    /* disable irq, ack pending interrupt and ack all pending events */
>> +    xhci_disable_interrupter(ir);
>> +    iman_reg = readl_relaxed(&ir->ir_set->irq_pending);
>> +    if (iman_reg & IMAN_IP)
>> +        writel_relaxed(iman_reg, &ir->ir_set->irq_pending);
>> +
>> +    /* last acked event trb is in erdp reg  */
>> +    erdp_reg = xhci_read_64(xhci, &ir->ir_set->erst_dequeue);
>> +    deq = (dma_addr_t)(erdp_reg & ERST_PTR_MASK);
>> +    if (!deq) {
>> +        xhci_err(xhci, "event ring handling not required\n");
>> +        return;
>> +    }
>> +
>> +    erdp_trb = current_trb = ir->event_ring->dequeue;
>> +    /* 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) {
>> +        inc_deq(xhci, ir->event_ring);
>> +        erdp_trb = ir->event_ring->dequeue;
>> +        /* cycle state transition */
>> +        if ((le32_to_cpu(erdp_trb->event_cmd.flags) & TRB_CYCLE) !=
>> +            ring->cycle_state)
>> +            break;
>> +    }
>> +
>> +    xhci_update_erst_dequeue(xhci, ir, current_trb, true);
>> +}
> 
> Code above is very similar to the existing event ring processing parts 
> of xhci_irq()
> and xhci_handle_event()
> 
> I'll see if I can refactor the existing event ring processing, decouple 
> it from
> event handling so that it could be used by primary and secondary 
> interrupters with
> handlers, and this case where we just want to clear the event ring.
> 

Thanks, that makes sense.  Will take a look as well.

Thanks
Wesley Cheng