Re: [PATCH] PCI: brcmstb: Reserve only the MSI vectors that are handed out

[Han / 한상우Sangwoo]

Hi Bjorn,

Sorry for the delay in testing this.

I tested Thomas's patch on the same hardware setup with the 5-vector
MSI endpoint. The patch was applied as posted on top of 6.12.93
(rpi-6.12.y).

With the patch applied:

- The MSI base hwirq remained at 0x8 across 200 driver reload cycles.
- The driver got all 5 requested vectors on every cycle, with no
single-MSI fallback. Multiple Message Enable remained at 8.
- The brcmstb inner-domain mapping returned to baseline after each
unload/reload, with 12 mapped while the driver was loaded and 4 after
unload.
- A kprobe showed one allocation of 8 vectors followed by eight
single-vector frees, with nothing left over.

The MSI vector exhaustion issue I originally observed no longer
reproduces with the patch.

I also observed a KASAN report with managed affinity. Using a small
out-of-tree test module bound to the same endpoint and requesting 1..3
vectors with PCI_IRQ_MSI | PCI_IRQ_AFFINITY, a request for 3 vectors
resulted in:

BUG: KASAN: slab-out-of-bounds in __irq_alloc_descs+0x158/0x460

Requests for 5 and 7 vectors were capped to 4 on this 4-CPU system and
did not trigger the report. I have not checked this against the
unpatched kernel yet, so I cannot tell whether it is related to the
patch.

Best regards,
Sangwoo


2026년 9월 17일 (목) 오전 8:10, Bjorn Helgaas <helgaas@xxxxxxxxxx>님이 작성:
>
> On Mon, Sep 07, 2026 at 11:40:34PM +0200, Thomas Gleixner wrote:
> > On Mon, Sep 07 2026 at 11:34, Bjorn Helgaas wrote:
> > > [+cc Thomas, Inochi for MSI expertise]
> > >
> > > I want to revive this thread because I think there's a real problem
> > > here, and we should solve it for all the PCI controller drivers.
> > >
> > > There's nothing brcm-specific about the bitmap alloc/free except the
> > > size of the msi->used bitmap, so I don't want to copy/paste this sort
> > > of fix in all the affected drivers.
> > >
> > > I'd also like to avoid the extra align_mask and
> > > bitmap_find_next_zero_area() followed by manual bitmap_set().
> > > bitmap_find_free_region() already takes care of the alignment and
> > > setting the allocated bits.
> > >
> > > The MSI Multiple Message Enable situation of enabling more vectors in
> > > the device than the driver wants is generic to all devices that
> > > advertise Multiple Message Capable, and I don't think we should have
> > > to deal with this in every host controller driver.
> >
> > Correct.
> >
> > > If a driver requests 3 vectors, we have to enable 4 because MSI only
> > > supports power-of-two number of vectors.  This tells the device it is
> > > allowed to use all 4 vectors, and I think the PCI MSI core should
> > > assume they all *will* be used instead of relying on the driver's
> > > claim that it will only use 3.
> >
> > That's not really a good idea because e.g. the irq affinity stuff relies
> > on the accurate number of interrupts the driver requested with the
> > minvec/maxvec range. We can't magically spread more interrupts than the
> > driver is able/willing to handle.
> >
> > But we can fix that without changing the consumer side (device drivers)
> > visible behaviour and handle it solely in the core code.
> >
> >  1) MSI interrupts are special because they have msi_desc::nvec_used >
> >     1, so the allocation and the free path can take care of the power of
> >     two requirement. That just allocates more resources than the driver
> >     wants but they are just memory.
> >
> >  2) All MSI parent domain implementations should be able to handle
> >     domain_ops::free() with nr_irqs > 1. That's something which can be
> >     trivialy audited.
> >
> >     I really have no memories why the bulk remove function iterates the
> >     interrupts one by one instead of doing in one go, but this is also
> >     used by non MSI domains, which might have issues with a bulk remove.
> >
> >     If we establish that all MSI parent domain implementations can
> >     handle the free() callback with nr_irqs > 1, then
> >     irq_domain_free_irqs_hierarchy can check whether
> >     IRQ_DOMAIN_FLAG_MSI_PARENT is set in the domain_flags and avoid the
> >     loop for that case.
> >
> > Something like the completely untested below.
>
> Sangwoo, is there any chance you can test this and see whether it
> fixes the issue?
>
> If it does, I guess we'll have to ask Thomas to post it with the
> appropriate Signed-off-by, etc.
>
> > ---
> > diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> > index 4fdcb6df5306..b3f6cc6ae2ce 100644
> > --- a/kernel/irq/irqdomain.c
> > +++ b/kernel/irq/irqdomain.c
> > @@ -1611,6 +1611,13 @@ static void irq_domain_free_irqs_hierarchy(struct irq_domain *domain,
> >       if (!domain->ops->free)
> >               return;
> >
> > +     /* CHECKME: Are all MSI parent domains capable ? */
> > +     if (domain->flags & IRQ_DOMAIN_FLAG_MSI_PARENT) {
> > +             if (irq_domain_get_irq_data(domain, irq_base))
> > +                     domain->ops->free(domain, irq_base, nr_irqs);
> > +             return;
> > +     }
> > +
> >       for (i = 0; i < nr_irqs; i++) {
> >               if (irq_domain_get_irq_data(domain, irq_base + i))
> >                       domain->ops->free(domain, irq_base + i, 1);
> > diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
> > index fb5f372215bf..2835b09899ea 100644
> > --- a/kernel/irq/msi.c
> > +++ b/kernel/irq/msi.c
> > @@ -1333,20 +1333,28 @@ static int __msi_domain_alloc_irqs(struct device *dev, struct irq_domain *domain
> >
> >               ops->set_desc(&arg, desc);
> >
> > -             virq = __irq_domain_alloc_irqs(domain, -1, desc->nvec_used,
> > +             /* Make sure a MULTI-MSI allocation is power of two */
> > +             unsigned int nvec_aligned = roundup_pow_of_two(desc->nvec_used);
> > +
> > +             virq = __irq_domain_alloc_irqs(domain, -1, nvec_aligned,
> >                                              dev_to_node(dev), &arg, false,
> >                                              desc->affinity);
> >               if (virq < 0)
> >                       return msi_handle_pci_fail(domain, desc, allocated);
> >
> > -             for (i = 0; i < desc->nvec_used; i++) {
> > +             for (i = 0; i < nvec_aligned; i++) {
> >                       irq_set_msi_desc_off(virq, i, desc);
> >                       irq_debugfs_copy_devname(virq + i, dev);
> >                       ret = msi_init_virq(domain, virq + i, vflags);
> >                       if (ret)
> >                               return ret;
> >               }
> > +
> >               if (info->flags & MSI_FLAG_DEV_SYSFS) {
> > +                     /*
> > +                      * This only exposes desc->nvec_used and ignores the
> > +                      * overallocated MULTI-MSI ones.
> > +                      */
> >                       ret = msi_sysfs_populate_desc(dev, desc);
> >                       if (ret)
> >                               return ret;
> > @@ -1610,13 +1618,15 @@ static void __msi_domain_free_irqs(struct device *dev, struct irq_domain *domain
> >                       continue;
> >
> >               /* Make sure all interrupts are deactivated */
> > -             for (i = 0; i < desc->nvec_used; i++) {
> > +             unsigned int nvec_aligned = roundup_pow_of_two(desc->nvec_used);
> > +
> > +             for (i = 0; i < nvec_aligned; i++) {
> >                       irqd = irq_domain_get_irq_data(domain, desc->irq + i);
> >                       if (irqd && irqd_is_activated(irqd))
> >                               irq_domain_deactivate_irq(irqd);
> >               }
> >
> > -             irq_domain_free_irqs(desc->irq, desc->nvec_used);
> > +             irq_domain_free_irqs(desc->irq, nvec_aligned);
> >               if (info->flags & MSI_FLAG_DEV_SYSFS)
> >                       msi_sysfs_remove_desc(dev, desc);
> >               desc->irq = 0;