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

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

[Sangwoo Han]

brcm_msi_alloc() reserves a naturally aligned power-of-two region with
bitmap_find_free_region(order_base_2(nr_irqs)), but the irqdomain core
releases the vectors of a block one at a time:

  /* irq_domain_free_irqs_hierarchy() */
  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);

That loop is the only caller of an irq_domain's ops->free(), so
brcm_irq_domain_free() always sees nr_irqs == 1 and
bitmap_release_region() clears exactly one bit per call. A request whose
vector count is not a power of two therefore reserves
roundup_pow_of_two(nr_irqs) bits but releases only nr_irqs of them, and
the difference stays set for the lifetime of the controller.

Multi-MSI regions are order-aligned and the controller has at most 32
MSIs, so the pool is quickly exhausted. Observed on a BCM2712 with a
5-vector endpoint behind a 4-port PCIe switch: the switch ports take
hwirq 0x0-0x3 and the endpoint's block walks 0x8 -> 0x10 -> 0x18 across
three driver reloads until no aligned order-3 region is left.
pci_alloc_irq_vectors() then falls back to a single vector for the rest
of the boot, silently multiplexing the endpoint's four completion
interrupts onto one hwirq.

Devices that ask for a non-power-of-two vector count are not exotic:
wil6210 asks for 3, the MHI modems for 5 (Quectel EM1xx, Foxconn SDX55,
Telit FN990, MediaTek MV3x) or 7 (Qualcomm v1), ath11k WCN6750 for 28
and ptp_ocp for 17. MSI-X is unaffected because it allocates one
descriptor per vector with nvec_used == 1, so the order is always zero.

Reserve exactly the vectors that are handed out, at a base found with
bitmap_find_next_zero_area(), and clear exactly the vectors that are
freed. The number of reserved bits then matches the number the core
releases, whatever arity it uses.

The base still has to be aligned: PCI Local Bus Specification 3.0
(section 6.8.1.6) lets the endpoint encode the vector number in the low
order_base_2(nr_irqs) bits of the Message Data register, and in this
controller those bits are the hwirq itself. The alignment mask has to be
roundup_pow_of_two(nr_irqs) - 1 rather than nr_irqs - 1:
bitmap_find_next_zero_area() requires a mask of the form 2^k - 1. No
align_offset is needed because the bitmap index is the value the
endpoint ORs in, see brcm_msi_compose_msi_msg().

For a power-of-two nr_irqs the alignment and the region length are both
nr_irqs, so the base returned is the same as before and those
allocations are unaffected.

Fixes: 198acab1772f ("PCI: brcmstb: Enable Multi-MSI")
Cc: stable@vger.kernel.org
Signed-off-by: Sangwoo Han <sangwoo.han@nearthlab.com>
---

Notes:
    Tested on a BCM2712 (Raspberry Pi 5) with a 5-vector endpoint behind a
    4-port PCIe switch, running 6.12.25 where brcm_msi_alloc() and
    brcm_msi_free() are byte-identical to mainline. Without the patch the
    endpoint's block walks 0x8 -> 0x10 -> 0x18 over three driver reloads and
    then falls back to a single vector for the rest of the boot; with it the
    block returns to 0x8 on all of eight reload cycles and the inner
    domain's mapped count round-trips cleanly.
    
    Compile-tested on mainline for arm64 with W=1 and C=1 (sparse); no new
    warnings.

 drivers/pci/controller/pcie-brcmstb.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 8a0c353d2a..6c5166666d 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -595,11 +595,22 @@ static struct irq_chip brcm_msi_bottom_irq_chip = {
 
 static int brcm_msi_alloc(struct brcm_msi *msi, unsigned int nr_irqs)
 {
+	/*
+	 * brcm_msi_compose_msi_msg() puts hwirq in the low order bits of the
+	 * message data, which a Multi-MSI endpoint rewrites per vector, so a
+	 * block's base must be aligned to the Multiple Message Enable count.
+	 */
+	unsigned long align_mask = roundup_pow_of_two(nr_irqs) - 1;
 	int hwirq;
 
 	mutex_lock(&msi->lock);
-	hwirq = bitmap_find_free_region(msi->used, msi->nr,
-					order_base_2(nr_irqs));
+	hwirq = bitmap_find_next_zero_area(msi->used, msi->nr, 0, nr_irqs,
+					   align_mask);
+	if (hwirq >= msi->nr) {
+		mutex_unlock(&msi->lock);
+		return -ENOSPC;
+	}
+	bitmap_set(msi->used, hwirq, nr_irqs);
 	mutex_unlock(&msi->lock);
 
 	return hwirq;
@@ -609,7 +620,7 @@ static void brcm_msi_free(struct brcm_msi *msi, unsigned long hwirq,
 			  unsigned int nr_irqs)
 {
 	mutex_lock(&msi->lock);
-	bitmap_release_region(msi->used, hwirq, order_base_2(nr_irqs));
+	bitmap_clear(msi->used, hwirq, nr_irqs);
 	mutex_unlock(&msi->lock);
 }
 
-- 
2.53.0

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

[Manivannan Sadhasivam]

On Thu, 30 Jul 2026 16:22:15 +0900, Sangwoo Han wrote:
> brcm_msi_alloc() reserves a naturally aligned power-of-two region with
> bitmap_find_free_region(order_base_2(nr_irqs)), but the irqdomain core
> releases the vectors of a block one at a time:
> 
>   /* irq_domain_free_irqs_hierarchy() */
>   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);
> 
> [...]

Applied, thanks!

[1/1] PCI: brcmstb: Reserve only the MSI vectors that are handed out
      commit: 92bf60fbb80f94c1b0fc24c7ff67d7aee936a58c

Best regards,
-- 
மணிவண்ணன் சதாசிவம்

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

[Bjorn Helgaas]

On Thu, Jul 30, 2026 at 04:22:15PM +0900, Sangwoo Han wrote:
> brcm_msi_alloc() reserves a naturally aligned power-of-two region with
> bitmap_find_free_region(order_base_2(nr_irqs)), but the irqdomain core
> releases the vectors of a block one at a time:
> 
>   /* irq_domain_free_irqs_hierarchy() */
>   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);
> 
> That loop is the only caller of an irq_domain's ops->free(), so
> brcm_irq_domain_free() always sees nr_irqs == 1 and
> bitmap_release_region() clears exactly one bit per call. A request whose
> vector count is not a power of two therefore reserves
> roundup_pow_of_two(nr_irqs) bits but releases only nr_irqs of them, and
> the difference stays set for the lifetime of the controller.
> 
> Multi-MSI regions are order-aligned and the controller has at most 32
> MSIs, so the pool is quickly exhausted. Observed on a BCM2712 with a
> 5-vector endpoint behind a 4-port PCIe switch: the switch ports take
> hwirq 0x0-0x3 and the endpoint's block walks 0x8 -> 0x10 -> 0x18 across
> three driver reloads until no aligned order-3 region is left.
> pci_alloc_irq_vectors() then falls back to a single vector for the rest
> of the boot, silently multiplexing the endpoint's four completion
> interrupts onto one hwirq.
> 
> Devices that ask for a non-power-of-two vector count are not exotic:
> wil6210 asks for 3, the MHI modems for 5 (Quectel EM1xx, Foxconn SDX55,
> Telit FN990, MediaTek MV3x) or 7 (Qualcomm v1), ath11k WCN6750 for 28
> and ptp_ocp for 17. MSI-X is unaffected because it allocates one
> descriptor per vector with nvec_used == 1, so the order is always zero.
> 
> Reserve exactly the vectors that are handed out, at a base found with
> bitmap_find_next_zero_area(), and clear exactly the vectors that are
> freed. The number of reserved bits then matches the number the core
> releases, whatever arity it uses.
> 
> The base still has to be aligned: PCI Local Bus Specification 3.0
> (section 6.8.1.6) lets the endpoint encode the vector number in the low
> order_base_2(nr_irqs) bits of the Message Data register, and in this
> controller those bits are the hwirq itself. The alignment mask has to be
> roundup_pow_of_two(nr_irqs) - 1 rather than nr_irqs - 1:
> bitmap_find_next_zero_area() requires a mask of the form 2^k - 1. No
> align_offset is needed because the bitmap index is the value the
> endpoint ORs in, see brcm_msi_compose_msi_msg().
> 
> For a power-of-two nr_irqs the alignment and the region length are both
> nr_irqs, so the base returned is the same as before and those
> allocations are unaffected.

Several other PCI controller drivers have similar code.

I think we should fix them all at once (or explain why they don't need
similar fixes).  Might be worth a little helper so they all work the
same way (e.g., some use order_base_2(), others use get_count_order(),
which seems like a pointless difference).

> Fixes: 198acab1772f ("PCI: brcmstb: Enable Multi-MSI")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sangwoo Han <sangwoo.han@nearthlab.com>
> ---
> 
> Notes:
>     Tested on a BCM2712 (Raspberry Pi 5) with a 5-vector endpoint behind a
>     4-port PCIe switch, running 6.12.25 where brcm_msi_alloc() and
>     brcm_msi_free() are byte-identical to mainline. Without the patch the
>     endpoint's block walks 0x8 -> 0x10 -> 0x18 over three driver reloads and
>     then falls back to a single vector for the rest of the boot; with it the
>     block returns to 0x8 on all of eight reload cycles and the inner
>     domain's mapped count round-trips cleanly.
>     
>     Compile-tested on mainline for arm64 with W=1 and C=1 (sparse); no new
>     warnings.
> 
>  drivers/pci/controller/pcie-brcmstb.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 8a0c353d2a..6c5166666d 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -595,11 +595,22 @@ static struct irq_chip brcm_msi_bottom_irq_chip = {
>  
>  static int brcm_msi_alloc(struct brcm_msi *msi, unsigned int nr_irqs)
>  {
> +	/*
> +	 * brcm_msi_compose_msi_msg() puts hwirq in the low order bits of the
> +	 * message data, which a Multi-MSI endpoint rewrites per vector, so a
> +	 * block's base must be aligned to the Multiple Message Enable count.
> +	 */
> +	unsigned long align_mask = roundup_pow_of_two(nr_irqs) - 1;
>  	int hwirq;
>  
>  	mutex_lock(&msi->lock);
> -	hwirq = bitmap_find_free_region(msi->used, msi->nr,
> -					order_base_2(nr_irqs));
> +	hwirq = bitmap_find_next_zero_area(msi->used, msi->nr, 0, nr_irqs,
> +					   align_mask);
> +	if (hwirq >= msi->nr) {
> +		mutex_unlock(&msi->lock);
> +		return -ENOSPC;
> +	}
> +	bitmap_set(msi->used, hwirq, nr_irqs);
>  	mutex_unlock(&msi->lock);
>  
>  	return hwirq;
> @@ -609,7 +620,7 @@ static void brcm_msi_free(struct brcm_msi *msi, unsigned long hwirq,
>  			  unsigned int nr_irqs)
>  {
>  	mutex_lock(&msi->lock);
> -	bitmap_release_region(msi->used, hwirq, order_base_2(nr_irqs));
> +	bitmap_clear(msi->used, hwirq, nr_irqs);
>  	mutex_unlock(&msi->lock);
>  }
>  
> -- 
> 2.53.0
> 

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

[Han / 한상우Sangwoo]

> Several other PCI controller drivers have similar code.

I agree - eleven of them look like they can have the same problem.

> I think we should fix them all at once (or explain why they don't need
> similar fixes).  Might be worth a little helper so they all work the
> same way (e.g., some use order_base_2(), others use get_count_order(),
> which seems like a pointless difference).

I am working on a related fix for drivers/irqchip/irq-bcm2712-mip.c,
which I can test here.  I would rather not send changes to drivers I
cannot test, though, so I am leaving the eleven to whoever has the boards.
What I looked at is in the appendix, in case it is useful to them.

Two things seem more useful from where I am sitting.

 - The failure is silent.  Appendix A describes the symptom, so that
   anyone who hits it later can find this thread.

 - The contract is not written down.  It changed in 4615fbc3788d, where
   irq_domain_free_irqs_hierarchy() started freeing one vector at a time.

order_base_2() and get_count_order() give the same answer for any
nr_irqs >= 1, so consolidating those two is just cleanup.  Where a shared
helper should live I am not sure either.

Thanks,
Sangwoo


Appendix A - the symptom
========================

A PCIe device whose driver is unbound and rebound a few times, or whose
module is reloaded, ends up with fewer MSI vectors than it asked for.  It
does not recover until reboot.

Two things have to be true for it to happen: the device uses multi-MSI,
and the vector count it asks for is not a power of two.  Devices that ask
for a power of two are unaffected, and so is anything on MSI-X.

I have only seen the reduced-vector case.  A driver that insists on the
full count would fail to probe instead, but I have not seen that happen.


Appendix B - how I sorted the drivers
=====================================

I went through drivers/pci/controller/ while working out what my own fix
had to do, so this is that rather than a full audit.  Twenty of the
drivers own an MSI hwirq pool.  I asked three things about each:

 - does .alloc reserve a rounded-up block, or a single slot
 - does the core hand .free one vector at a time
 - can nr_irqs > 1 reach the driver at all

Eleven answer yes to all three:

  dwc/pcie-designware-host.c   pci-aardvark.c        pcie-apple.c
  pcie-aspeed.c                pcie-iproc-msi.c      pcie-mediatek-gen3.c
  pcie-rcar-host.c             pcie-rzg3s-host.c     pcie-xilinx-dma-pl.c
  pcie-xilinx-nwl.c            pci-hyperv.c

The other nine:

 - five reserve a single slot, so nothing rounds up:
   mobiveil/pcie-mobiveil-host.c, pci-xgene-msi.c, pcie-altera-msi.c,
   pcie-mediatek.c, plda/pcie-plda-host.c

 - vmd.c keeps a per-vector refcount instead of a bitmap

 - pci-tegra.c and pcie-xilinx.c reserve a rounded-up block, but neither
   lists MSI_FLAG_MULTI_PCI_MSI in msi_parent_ops.supported_flags

 - pcie-brcmstb.c is the one you applied

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

[Bjorn Helgaas]

[+cc Marc, author of 4615fbc3788d ("genirq/irqdomain: Don't try to
free an interrupt that has no mapping"), Thomas; beginning of thread
about multiple MSI vector alloc/free:
https://lore.kernel.org/all/20260730072215.2090974-1-sangwoo.han@nearthlab.com]

On Tue, Aug 04, 2026 at 08:31:23PM +0900, Han / 한상우Sangwoo wrote:
> > Several other PCI controller drivers have similar code.
> 
> I agree - eleven of them look like they can have the same problem.
> 
> > I think we should fix them all at once (or explain why they don't need
> > similar fixes).  Might be worth a little helper so they all work the
> > same way (e.g., some use order_base_2(), others use get_count_order(),
> > which seems like a pointless difference).
> 
> I am working on a related fix for drivers/irqchip/irq-bcm2712-mip.c,

Similar pattern there, but it uses ilog2(), which differs from
order_base_2() and get_count_order() in more cases.

> which I can test here.  I would rather not send changes to drivers I
> cannot test, though, so I am leaving the eleven to whoever has the boards.
> What I looked at is in the appendix, in case it is useful to them.
> 
> Two things seem more useful from where I am sitting.
> 
>  - The failure is silent.  Appendix A describes the symptom, so that
>    anyone who hits it later can find this thread.
> 
>  - The contract is not written down.  It changed in 4615fbc3788d, where
>    irq_domain_free_irqs_hierarchy() started freeing one vector at a time.
> 
> order_base_2() and get_count_order() give the same answer for any
> nr_irqs >= 1, so consolidating those two is just cleanup.  Where a shared
> helper should live I am not sure either.

I don't know either, but I hate fixing an issue in one place and
leaving the same issue unfixed nearby.

> Appendix A - the symptom
> ========================
> 
> A PCIe device whose driver is unbound and rebound a few times, or whose
> module is reloaded, ends up with fewer MSI vectors than it asked for.  It
> does not recover until reboot.
> 
> Two things have to be true for it to happen: the device uses multi-MSI,
> and the vector count it asks for is not a power of two.  Devices that ask
> for a power of two are unaffected, and so is anything on MSI-X.
> 
> I have only seen the reduced-vector case.  A driver that insists on the
> full count would fail to probe instead, but I have not seen that happen.
> 
> 
> Appendix B - how I sorted the drivers
> =====================================
> 
> I went through drivers/pci/controller/ while working out what my own fix
> had to do, so this is that rather than a full audit.  Twenty of the
> drivers own an MSI hwirq pool.  I asked three things about each:
> 
>  - does .alloc reserve a rounded-up block, or a single slot
>  - does the core hand .free one vector at a time
>  - can nr_irqs > 1 reach the driver at all
> 
> Eleven answer yes to all three:
> 
>   dwc/pcie-designware-host.c   pci-aardvark.c        pcie-apple.c
>   pcie-aspeed.c                pcie-iproc-msi.c      pcie-mediatek-gen3.c
>   pcie-rcar-host.c             pcie-rzg3s-host.c     pcie-xilinx-dma-pl.c
>   pcie-xilinx-nwl.c            pci-hyperv.c
> 
> The other nine:
> 
>  - five reserve a single slot, so nothing rounds up:
>    mobiveil/pcie-mobiveil-host.c, pci-xgene-msi.c, pcie-altera-msi.c,
>    pcie-mediatek.c, plda/pcie-plda-host.c
> 
>  - vmd.c keeps a per-vector refcount instead of a bitmap
> 
>  - pci-tegra.c and pcie-xilinx.c reserve a rounded-up block, but neither
>    lists MSI_FLAG_MULTI_PCI_MSI in msi_parent_ops.supported_flags
> 
>  - pcie-brcmstb.c is the one you applied