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

[Wesley Cheng]

Hi Mathias,

On 1/16/2024 12:24 PM, Wesley Cheng wrote:
> Hi Mathias,
>
> On 1/15/2024 6:01 AM, Mathias Nyman wrote:
> > On 10.1.2024 1.42, Wesley Cheng wrote:
> > > Hi Mathias,
> > >
> > > On 1/8/2024 12:51 PM, Wesley Cheng wrote:
> > > > 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@xxxxxxxxxxx>
> > > > > ...
> > > > > > +/*
> > > > > > + * 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.
> > >
> > > How about something like the below?  Tested this on my set up and 
> > > everything looks to be working fine.  Had to add another param to 
> > > struct xhci_interrupters to tell the XHCI interrupt handler to say if 
> > > that particular interrupter wants to skip_events (handling).  This 
> > > way, its something that the class driver utilizing the interrupter 
> > > will have to tell XHCI sideband.  It would allow the user to 
> > > determine if they want to use the interrupter to actually handle 
> > > events or not on the proc running Linux.
> >
> > Yes, I have something similar.
> > I'll share it soon, just need to
> > clean it up a bit fist.
>
> Sure, no worries.  Will test it when its available.  Thanks!

Was just wondering if you had the time to clean up the changes?  If not, 
maybe you can provide a patch with whatever you have, and I can try my 
best to clean it up to your liking?  Thanks!

Thanks
Wesley Cheng