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

[Thomas Gleixner <tglx@kernel.org>]

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.

Thanks,

        tglx
---
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;