[PATCH net v2] bnxt_en: Don't keep MSI-X vectors added before FW reserves them

[PATCH net v2] bnxt_en: Don't keep MSI-X vectors added before FW reserves them

[Vlad Poenaru]

When dynamic MSI-X is supported, bnxt_check_rings() grows the MSI-X
vectors with bnxt_change_msix() once FW says the new rings would fit,
and keeps them.  FW only reserves the extra IRQs later, when
bnxt_reserve_rings() calls __bnxt_reserve_rings().  The MSI-X table
entries beyond the function's reserved IRQs are not backed until then:
the MSI message written when the new vectors are activated is lost, and
the entries read back as zero.  After FW reserves them they are live but
still zero, so the NQ rings bound to them signal address 0 and every
completion on those rings is lost.  The next ring change (for example
ethtool -G) then times out freeing each of those rings and the TX
watchdog fires:

  bnxt_en 0002:01:00.0 eth0: Resp cmpl intr err msg: 0x51
  bnxt_en 0002:01:00.0 eth0: hwrm_ring_free type 1 failed. rc:fffffff0 err:0
  ...
  bnxt_en 0002:01:00.0 eth0: NETDEV WATCHDOG: CPU: 17: transmit queue 18 timed out 5716 ms

On x86 this is hidden: GENERIC_IRQ_RESERVATION_MODE makes the PCI MSI
domains MSI_FLAG_MUST_REACTIVATE, so the message is composed and written
again when the IRQ is requested, after FW has reserved the vector.
Since commit 0d6a7c417983 ("irqchip/gic-v3-its: Enable dynamic MSI-X
allocation"), arm64 GICv3 systems take the dynamic path as well, but
write the message only once, at allocation.  On an NVIDIA Grace system
with a BCM57508, raising the channel count from 18 to 32 left MSI-X
entries 18-31 all zero, and the 14 rings using them lost their
interrupts.

Keep the allocation in bnxt_check_rings() as a check that enough vectors
are available, but free the new vectors again right away when FW manages
IRQ reservations (BNXT_NEW_RM).  bnxt_reserve_rings() then allocates
them after __bnxt_reserve_rings() has reserved them.  Without
BNXT_NEW_RM there is no IRQ reservation and bnxt_reserve_rings() does
not grow the vectors, so keep the current behaviour there.

In bnxt_reserve_rings(), size that allocation from the rings FW actually
reserved (bnxt_get_num_msix()) instead of the count computed before the
reservation, so that a partial grant does not activate unreserved
entries either, at least while no RoCE driver is registered.

The allocation can now fail after FW has committed the reservation.  A
later open would then find nothing to reserve and return before growing
the vectors, and bnxt_request_irq() would use irq_tbl[] entries past
bp->total_irqs, which still hold the freed virqs.  Grow the vectors in
that case as well, and clear irq_tbl[].vector when trimming.

Fixes: 2d51eb0bd81c ("bnxt_en: Add MSIX check in bnxt_check_rings()")
Reported-by: Andrew Preece <apreece@meta.com>
Closes: https://lore.kernel.org/all/20261022193620.2152038-1-vlad.wing@gmail.com/
Signed-off-by: Vlad Poenaru <vlad.wing@gmail.com>
---
Changes in v2:
- Size the bnxt_reserve_rings() allocation from what FW reserved
  (bnxt_get_num_msix()), so a partial grant does not activate
  unreserved entries while no RoCE driver is registered (Sashiko).
  With RoCE registered, __bnxt_reserve_rings() can still keep more L2
  rings than the grant covers; that accounting predates this patch.
- Grow the vectors in bnxt_reserve_rings() even when no reservation
  is needed, and clear irq_tbl[].vector on trim, so a failed allocation
  after the FW reservation no longer leaves stale virqs behind
  (Sashiko).
- Only free the probed vectors when BNXT_NEW_RM is set; reword the
  comment.

v1: https://lore.kernel.org/all/20261023121321.2127263-1-vlad.wing@gmail.com/

Tested on an NVIDIA Grace system (GICv3 ITS, ACPI) with a BCM57508,
FW 230.0.165.3: the boot-time channel change (18 -> 32) followed by a
ring resize no longer loses completions.  With instrumentation, MSI-X
entries 18-31 read back as all zero after bnxt_check_rings() allocated
them, and read back correctly once bnxt_reserve_rings() allocated them
after the FW reservation.

This conflicts with the "bnxt_en: Ring accounting fixes" series on
net-next, which also changes the dynamic MSI-X path in
bnxt_reserve_rings().

 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 34 ++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 0682060b048ce..55b11c7684313 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -11524,6 +11524,7 @@ static int bnxt_change_msix(struct bnxt *bp, int total)
 		map.index = i - 1;
 		map.virq = bp->irq_tbl[i - 1].vector;
 		pci_msix_free_irq(bp->pdev, map);
+		bp->irq_tbl[i - 1].vector = 0;
 		bp->total_irqs--;
 	}
 	return bp->total_irqs;
@@ -11701,8 +11702,23 @@ int bnxt_reserve_rings(struct bnxt *bp, bool irq_re_init)
 	int irqs_required;
 	int rc;
 
-	if (!bnxt_need_reserve_rings(bp))
+	if (!bnxt_need_reserve_rings(bp)) {
+		/* FW may already hold the reservation from an earlier
+		 * reservation that was not followed by growing the dynamic
+		 * MSI-X vectors, e.g. because the allocation failed.
+		 */
+		if (irq_re_init && BNXT_NEW_RM(bp) && bp->irq_tbl &&
+		    pci_msix_can_alloc_dyn(bp->pdev)) {
+			irqs_required = bnxt_get_num_msix(bp);
+			if (irqs_required > bp->total_irqs &&
+			    bnxt_change_msix(bp, irqs_required) != irqs_required) {
+				netdev_err(bp->dev, "Unable to allocate %d MSIX vectors, maximum available %d\n",
+					   irqs_required, bp->total_irqs);
+				return -ENOSPC;
+			}
+		}
 		return 0;
+	}
 
 	if (BNXT_NEW_RM(bp) && !bnxt_ulp_registered(edev)) {
 		int ulp_msix = bnxt_get_avail_msix(bp, bp->ulp_num_msix_want);
@@ -11728,6 +11744,10 @@ int bnxt_reserve_rings(struct bnxt *bp, bool irq_re_init)
 			rc = bnxt_init_int_mode(bp);
 		bnxt_ulp_irq_restart(bp, rc);
 	} else if (irq_change && !rc) {
+		/* Size the vectors from what FW actually reserved, which
+		 * can be less than what was requested.
+		 */
+		irqs_required = bnxt_get_num_msix(bp);
 		if (bnxt_change_msix(bp, irqs_required) != irqs_required)
 			rc = -ENOSPC;
 	}
@@ -15093,6 +15113,7 @@ int bnxt_check_rings(struct bnxt *bp, int tx, int rx, bool sh, int tcs,
 			hwr.cp = min_t(int, hwr.cp, bnxt_get_max_func_irqs(bp));
 		}
 		if (hwr.cp > bp->total_irqs) {
+			int old_total = bp->total_irqs;
 			int total_msix = bnxt_change_msix(bp, hwr.cp);
 
 			if (total_msix < hwr.cp) {
@@ -15100,6 +15121,17 @@ int bnxt_check_rings(struct bnxt *bp, int tx, int rx, bool sh, int tcs,
 					    hwr.cp, total_msix);
 				rc = -ENOSPC;
 			}
+			/* FW has not reserved the new vectors yet, and writes to
+			 * MSI-X table entries beyond the reserved IRQs don't
+			 * stick (seen on BCM57508).  Free them again;
+			 * bnxt_reserve_rings() allocates them after
+			 * __bnxt_reserve_rings().  Without the new resource
+			 * manager there is no IRQ reservation and
+			 * bnxt_reserve_rings() does not grow the vectors, so
+			 * keep them.
+			 */
+			if (BNXT_NEW_RM(bp))
+				bnxt_change_msix(bp, old_total);
 		}
 	}
 	return rc;

Re: [PATCH net v2] bnxt_en: Don't keep MSI-X vectors added before FW reserves them

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 1172 bytes --]

On Wed, Oct 28, 2026 at 4:49 AM Vlad Poenaru <vlad.wing@gmail.com> wrote:
>
> When dynamic MSI-X is supported, bnxt_check_rings() grows the MSI-X
> vectors with bnxt_change_msix() once FW says the new rings would fit,
> and keeps them.  FW only reserves the extra IRQs later, when
> bnxt_reserve_rings() calls __bnxt_reserve_rings().  The MSI-X table
> entries beyond the function's reserved IRQs are not backed until then:
> the MSI message written when the new vectors are activated is lost, and
> the entries read back as zero.  After FW reserves them they are live but
> still zero, so the NQ rings bound to them signal address 0 and every
> completion on those rings is lost.  The next ring change (for example
> ethtool -G) then times out freeing each of those rings and the TX
> watchdog fires:
>
>   bnxt_en 0002:01:00.0 eth0: Resp cmpl intr err msg: 0x51
>   bnxt_en 0002:01:00.0 eth0: hwrm_ring_free type 1 failed. rc:fffffff0 err:0
>   ...
>   bnxt_en 0002:01:00.0 eth0: NETDEV WATCHDOG: CPU: 17: transmit queue 18 timed out 5716 ms

Thanks for the patch.  I'm discussing this issue with our FW team and
will provide a response tomorrow.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5469 bytes --]

Re: [PATCH net v2] bnxt_en: Don't keep MSI-X vectors added before FW reserves them

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 2620 bytes --]

On Wed, Oct 28, 2026 at 4:49 AM Vlad Poenaru <vlad.wing@gmail.com> wrote:
>
> When dynamic MSI-X is supported, bnxt_check_rings() grows the MSI-X
> vectors with bnxt_change_msix() once FW says the new rings would fit,
> and keeps them.  FW only reserves the extra IRQs later, when
> bnxt_reserve_rings() calls __bnxt_reserve_rings().  The MSI-X table
> entries beyond the function's reserved IRQs are not backed until then:
> the MSI message written when the new vectors are activated is lost, and
> the entries read back as zero.  After FW reserves them they are live but
> still zero, so the NQ rings bound to them signal address 0 and every
> completion on those rings is lost.  The next ring change (for example
> ethtool -G) then times out freeing each of those rings and the TX
> watchdog fires:
>
>   bnxt_en 0002:01:00.0 eth0: Resp cmpl intr err msg: 0x51
>   bnxt_en 0002:01:00.0 eth0: hwrm_ring_free type 1 failed. rc:fffffff0 err:0
>   ...
>   bnxt_en 0002:01:00.0 eth0: NETDEV WATCHDOG: CPU: 17: transmit queue 18 timed out 5716 ms
>

I've discussed this issue with our FW team and they have confirmed it.
Writing to an MSI-X entry that has not been reserved by the FW will be
silently ignored by the HW.

...

> @@ -11701,8 +11702,23 @@ int bnxt_reserve_rings(struct bnxt *bp, bool irq_re_init)
>         int irqs_required;
>         int rc;
>
> -       if (!bnxt_need_reserve_rings(bp))
> +       if (!bnxt_need_reserve_rings(bp)) {
> +               /* FW may already hold the reservation from an earlier
> +                * reservation that was not followed by growing the dynamic
> +                * MSI-X vectors, e.g. because the allocation failed.
> +                */
> +               if (irq_re_init && BNXT_NEW_RM(bp) && bp->irq_tbl &&
> +                   pci_msix_can_alloc_dyn(bp->pdev)) {
> +                       irqs_required = bnxt_get_num_msix(bp);
> +                       if (irqs_required > bp->total_irqs &&
> +                           bnxt_change_msix(bp, irqs_required) != irqs_required) {
> +                               netdev_err(bp->dev, "Unable to allocate %d MSIX vectors, maximum available %d\n",
> +                                          irqs_required, bp->total_irqs);
> +                               return -ENOSPC;

Once I rebase my net-next series, this will likely not be needed since
there will be infrastructure to scale the rings down.  I will make
sure to handle the case where dynamic MSI-X falls short.

The patch looks good to me.  Thanks.
Reviewed-by: Michael Chan <michael.chan@broadcom.com>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5469 bytes --]

Re: [PATCH net v2] bnxt_en: Don't keep MSI-X vectors added before FW reserves them

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 (1) · Medium: 1 (1) · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In bnxt_reserve_rings()
  (drivers/net/ethernet/broadcom/bnxt/bnxt.c), the new recompute in the…
- [Low] With BNXT_NEW_RM, bnxt_check_rings() now allocates the extra
  dynamic MSI-X vectors only to test availability, then frees them at…

Pre-existing issues:
- [High] bnxt_clear_int_mode() frees the vectors and sets bp->irq_tbl =
  NULL but leaves bp->total_irqs stale.
- [Medium] bnxt_hwrm_check_pf_rings() does not check the return value of
  __bnxt_hwrm_reserve_pf_rings().

--

bnxt_en: Don't keep MSI-X vectors added before FW reserves them

When BNXT_NEW_RM is set, bnxt_check_rings() now frees the dynamically
allocated MSI-X vectors right after the availability check.
bnxt_reserve_rings() then allocates them after __bnxt_reserve_rings(),
sized from bnxt_get_num_msix(). It also grows the vectors when no
reservation is needed but FW already holds one, and clears
irq_tbl[].vector when trimming.

Closes: https://lore.kernel.org/all/20261022193620.2152038-1-vlad.wing@gmail.com/

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 0682060b048ce..55b11c7684313 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c

[ ... ]

> @@ -11701,8 +11702,23 @@ int bnxt_reserve_rings(struct bnxt *bp, bool irq_re_init)
>  	int irqs_required;
>  	int rc;
>  
> -	if (!bnxt_need_reserve_rings(bp))
> +	if (!bnxt_need_reserve_rings(bp)) {
> +		/* FW may already hold the reservation from an earlier
> +		 * reservation that was not followed by growing the dynamic
> +		 * MSI-X vectors, e.g. because the allocation failed.
> +		 */
> +		if (irq_re_init && BNXT_NEW_RM(bp) && bp->irq_tbl &&
> +		    pci_msix_can_alloc_dyn(bp->pdev)) {

[ ... ]

> @@ -11728,6 +11744,10 @@ int bnxt_reserve_rings(struct bnxt *bp, bool irq_re_init)
>  			rc = bnxt_init_int_mode(bp);
>  		bnxt_ulp_irq_restart(bp, rc);
>  	} else if (irq_change && !rc) {
> +		/* Size the vectors from what FW actually reserved, which
> +		 * can be less than what was requested.
> +		 */
> +		irqs_required = bnxt_get_num_msix(bp);

[Severity: Medium]
The comment says this sizes the vectors from what FW actually reserved.
Under BNXT_NEW_RM, though, bnxt_get_num_msix() returns
bnxt_nq_rings_in_use(), which is bp->cp_nr_rings +
bnxt_get_ulp_msix_num(bp). It is not hw_resc->resv_irqs.

Can this still activate unreserved MSI-X entries when the RoCE driver is
registered?

In __bnxt_reserve_rings(), the FW IRQ grant (copied into hwr.cp by
bnxt_copy_reserved_rings()) only bounds the L2 count:

	hwr.cp = min_t(int, hwr.cp, bp->cp_nr_rings);

The re-clamp of the ULP vectors against resv_irqs is skipped when a ULP
is registered:

	if (!bnxt_ulp_registered(edev) && BNXT_NEW_RM(bp)) {
		...
		resv_msix = hw_resc->resv_irqs - bp->cp_nr_rings;
		ulp_msix = min_t(int, resv_msix, ulp_msix);
		bnxt_set_ulp_msix_num(bp, ulp_msix);

Take 8 ULP vectors, 32 L2 NQs and a FW grant of resv_irqs = 34.
bnxt_get_num_msix() returns 40, and bnxt_change_msix(bp, 40) activates
entries 34-39 while rc stays 0.

bnxt_alloc_cp_rings() maps L2 NQ i to MSI-X index ulp_msix + i. That
puts L2 NQs 26-31 on entries FW never reserved, which is the
lost-completion case this patch is fixing.

The commit message limits this with "at least while no RoCE driver is
registered" and says the accounting predates the patch. The in-code
comment doesn't mention the RoCE case.

Could the sizing be bounded by hw_resc->resv_irqs here? Or could the
registered ULP vectors be counted against resv_irqs in
__bnxt_reserve_rings()?

>  		if (bnxt_change_msix(bp, irqs_required) != irqs_required)
>  			rc = -ENOSPC;
>  	}

[Severity: High]
This is a pre-existing issue, but can this branch call bnxt_change_msix()
while bp->irq_tbl is NULL?

bnxt_clear_int_mode() frees the table but leaves bp->total_irqs as it
is:

static void bnxt_clear_int_mode(struct bnxt *bp)
{
	pci_free_irq_vectors(bp->pdev);

	kfree(bp->irq_tbl);
	bp->irq_tbl = NULL;
}

It is called from bnxt_hwrm_if_change() on fw_reset or caps_change, just
before bnxt_cancel_reservations(). It is also called from
bnxt_io_slot_reset().

The next bnxt_reserve_rings(bp, true) call comes from __bnxt_open_nic()
or bnxt_io_resume(). At that point bnxt_need_reserve_rings() is true and
pci_msix_can_alloc_dyn() is still true. So irq_change is set whenever
irqs_required differs from the stale bp->total_irqs.

If the count passed to bnxt_change_msix() is below the stale
bp->total_irqs, the trim loop then runs through a NULL bp->irq_tbl:

		map.virq = bp->irq_tbl[i - 1].vector;
		pci_msix_free_irq(bp->pdev, map);
		bp->irq_tbl[i - 1].vector = 0;

This could already happen when the pre-reservation count was below the
stale total. The recompute after __bnxt_reserve_rings() adds a new
trigger: pre-reservation count > stale total_irqs > post-reservation
count.

Before this patch that case took the grow loop instead. There,
pci_msix_alloc_irq_at() failed cleanly and the result was -ENOSPC.

The new !bnxt_need_reserve_rings() branch above checks bp->irq_tbl.
Should this branch check it as well?

[ ... ]

> @@ -15093,6 +15113,7 @@ int bnxt_check_rings(struct bnxt *bp, int tx, int rx, bool sh, int tcs,
>  			hwr.cp = min_t(int, hwr.cp, bnxt_get_max_func_irqs(bp));
>  		}
>  		if (hwr.cp > bp->total_irqs) {
> +			int old_total = bp->total_irqs;
>  			int total_msix = bnxt_change_msix(bp, hwr.cp);
>  
>  			if (total_msix < hwr.cp) {
> @@ -15100,6 +15121,17 @@ int bnxt_check_rings(struct bnxt *bp, int tx, int rx, bool sh, int tcs,
>  					    hwr.cp, total_msix);
>  				rc = -ENOSPC;
>  			}
> +			/* FW has not reserved the new vectors yet, and writes to
> +			 * MSI-X table entries beyond the reserved IRQs don't
> +			 * stick (seen on BCM57508).  Free them again;
> +			 * bnxt_reserve_rings() allocates them after
> +			 * __bnxt_reserve_rings().  Without the new resource
> +			 * manager there is no IRQ reservation and
> +			 * bnxt_reserve_rings() does not grow the vectors, so
> +			 * keep them.
> +			 */
> +			if (BNXT_NEW_RM(bp))
> +				bnxt_change_msix(bp, old_total);

[Severity: Low]
With the vectors freed again here, a 0 return from bnxt_check_rings() no
longer means they will still be available at reopen. Callers such as
bnxt_set_channels() act on that result:

bnxt_set_channels()
  bnxt_check_rings()          <- allocates, then frees the vectors
  bnxt_close_nic()
  (rx/tx ring counts updated)
  bnxt_open_nic()
    bnxt_reserve_rings()
      __bnxt_reserve_rings()  <- FW reservation committed
      bnxt_change_msix()      <- can now fail with -ENOSPC

Suppose the second allocation fails, for example because IRQ descriptor
allocation fails. Would an ethtool -L request that was already accepted
leave the interface down with "ring reservation/IRQ init failure"?

Before this patch, that failure would have come back from
bnxt_check_rings() before the device was closed.

The commit message notes that the allocation can now fail after FW has
committed the reservation, and the next open retries it.

>  		}
>  	}
>  	return rc;

[Severity: Medium]
This isn't a bug introduced by this patch, but bnxt_check_rings() goes
through bnxt_hwrm_check_rings()->bnxt_hwrm_check_pf_rings(), which uses
the request without a NULL check:

	req = __bnxt_hwrm_reserve_pf_rings(bp, hwr);
	flags = FUNC_CFG_REQ_FLAGS_TX_ASSETS_TEST;
	...
	req->flags = cpu_to_le32(flags);

__bnxt_hwrm_reserve_pf_rings() returns NULL when
bnxt_hwrm_func_cfg_short_req_init() fails. That happens, for example,
when dma_pool_alloc() fails in __hwrm_req_init().

Can this dereference a NULL req under memory pressure?
bnxt_hwrm_check_vf_rings() appears to follow the same pattern.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261028114933.256298-1-vlad.wing%40gmail.com