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

[Bjorn Helgaas <helgaas@kernel.org>]

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;