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

[Wesley Cheng]

Hi Mathias,

On 10/4/2023 11:35 AM, Wesley Cheng wrote:
> 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.

Had to make some minor tweaks here and there, but nothing major.  Was 
able to validate the changes on my end, and they look good.  Will test a 
bit more, and include these in my next submission.  Will try to address 
your FIXME tags as well.

Thanks
Wesley Cheng