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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The patch only reserves `nr_irqs` bits instead of the full power-of-two block in the controller's bitmap, breaking hardware IRQ isolation and allowing cross-device IRQ collisions if trailing vectors are reallocated.
--

--- Patch [1]: [PATCH] PCI: brcmstb: Reserve only the MSI vectors that are handed out ---
commit 627a58c7a939ca8de261414f13dbb9fd516289b4
Author: Sangwoo Han <[email protected]>
Subject: PCI: brcmstb: Reserve only the MSI vectors that are handed out

This commit modifies the Broadcom STB PCIe MSI allocator to reserve and free
exactly the requested number of interrupt vectors rather than rounding up to
the nearest power of two. This aims to prevent exhaustion of the 32-vector
pool when endpoints request non-power-of-two vector counts.

> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> --- 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)
>  {

[ ... ]

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

[Severity: High]
Does this leave the unallocated trailing vectors in the power-of-two block
vulnerable to cross-device IRQ collisions?

When a device requests a non-power-of-two number of MSIs, the generic PCI
core rounds the allocation up and programs the device's Multiple Message
Enable capability to the next power of two. The device is then
hardware-authorized to modify the lower bits of its MSI Message Data to
distinguish between vectors.

Because bitmap_set only reserves exactly nr_irqs bits, the remaining bits in
the device's hardware window remain marked as free in the allocator. If
another device is subsequently assigned one of these trailing vectors, can
the first device's interrupts be incorrectly routed to the second device?

>  	mutex_unlock(&msi->lock);
>  
>  	return hwirq;
>  }
>  
>  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);

[Severity: High]
If the allocation needs to reserve the full power-of-two block to prevent
collisions, does this also need to clear the full power-of-two block when
the vectors are freed?

>  	mutex_unlock(&msi->lock);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1