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

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 [off-list ref]님이 작성:

On Mon, Sep 07, 2026 at 11:40:34PM +0200, Thomas Gleixner wrote:

quoted

On Mon, Sep 07 2026 at 11:34, Bjorn Helgaas wrote:

quoted

[+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.

quoted

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.

quoted

---

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;