Re: [PATCH v7 01/33] xhci: add support to allocate several interrupters

[Wesley Cheng]

Hi Mathias,

On 10/4/2023 7:02 AM, Mathias Nyman wrote:
> On 2.10.2023 23.07, Wesley Cheng wrote:
> > Hi Mathias,
> >
> > On 9/28/2023 3:31 AM, Mathias Nyman wrote:
> > > On 22.9.2023 0.48, Wesley Cheng wrote:
> > > > From: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx>
> > > >
> > > > Modify the XHCI drivers to accommodate for handling multiple event 
> > > > rings in
> > > > case there are multiple interrupters.  Add the required APIs so 
> > > > clients are
> > > > able to allocate/request for an interrupter ring, and pass this 
> > > > information
> > > > back to the client driver.  This allows for users to handle the 
> > > > resource
> > > > accordingly, such as passing the event ring base address to an audio 
> > > > DSP.
> > > > There is no actual support for multiple MSI/MSI-X vectors.
> > > >
> > > > Factoring out XHCI interrupter APIs and structures done by Wesley 
> > > > Cheng, in
> > > > order to allow for USB class drivers to utilze them.
> > > >
> > > >   }
> > > > +void xhci_remove_secondary_interrupter(struct usb_hcd *hcd, struct 
> > > > xhci_interrupter *ir)
> > > > +{
> > > > +    struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> > > > +    unsigned int intr_num;
> > > > +
> > > > +    /* interrupter 0 is primary interrupter, don't touchit */
> > > > +    if (!ir || !ir->intr_num || ir->intr_num >= 
> > > > xhci->max_interrupters) {
> > > > +        xhci_dbg(xhci, "Invalid secondary interrupter, can't 
> > > > remove\n");
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    /* fixme, should we check xhci->interrupter[intr_num] == ir */
> > > > +    spin_lock(&xhci->lock);
> > >
> > > Needs to be spin_lock_irq() ir spin_lock_irqsave() as xhci->lock is 
> > > used in interrupt handler.
> > >
> > >
> > > > +    intr_num = ir->intr_num;
> > > > +    xhci_free_interrupter(xhci, ir);
> > > > +    xhci->interrupters[intr_num] = NULL;
> > > > +    spin_unlock(&xhci->lock);
> > >
> > > likewise
> >
> > Let me check these again.  In general, I think I will use both the 
> > xhci->mutex and xhci->lock where needed, because I believe we'd run 
> > into sleep while atomic issues
> > while freeing the DMA memory.  Will rework this and submit in the next 
> > rev.
>
> Maybe we need to split xhci_free_interrupter() into separate remove and 
> free functions

Thanks for sharing the work you've been doing.  Yes, I did something 
similar as well on my end, but will refactor in your code and re-test.

> Did some work on this, and on the sideband api in general.
>
> Code still has a lot of FIXMEs, and it's completely untested, but to 
> avoid us
> from doing duplicate work I pushed it to my feature_interrupters branch 
> anyway
>
> git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git 
> feature_interrupters
> https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/log/?h=feature_interrupters 

Ok.  Initial look at it seems like it will be fine, but will integrate 
and make changes where needed.

Thanks
Wesley Cheng