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

[netdev-bot+sashiko@kernel.org]

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

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

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] The fix is incomplete when FW grants fewer MSI-X vectors than
  requested.
- [Medium] New error path leaves the state inconsistent.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the DCB IEEE
  ETS path reaches bnxt_check_rings() and bnxt_change_msix() holding…

--

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

When dynamic MSI-X is supported, bnxt_check_rings() grows the MSI-X
vectors before FW has reserved them. On arm64 GICv3 the MSI message is
written only once, at allocation, so the message for those entries is
lost. This patch keeps the allocation in bnxt_check_rings() as an
availability check. When BNXT_NEW_RM is set it frees the new vectors
again right away, so that bnxt_reserve_rings() allocates them after
__bnxt_reserve_rings().

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..2d14d991a3ead 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -15093,6 +15093,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 +15101,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: Medium]

The comment says bnxt_reserve_rings() allocates the vectors after
__bnxt_reserve_rings(). Is that allocation capped at the number of
vectors FW actually granted?

In bnxt_reserve_rings(), irqs_required is computed before the
reservation is made:

	if (BNXT_NEW_RM(bp) && !bnxt_ulp_registered(edev)) {
		...
		irqs_required = ulp_msix + bp->cp_nr_rings;
	}
	...
	rc = __bnxt_reserve_rings(bp);
	...
	} else if (irq_change && !rc) {
		if (bnxt_change_msix(bp, irqs_required) != irqs_required)
			rc = -ENOSPC;
	}

If FW grants fewer MSI-X vectors than requested, bnxt_hwrm_get_rings()
records the smaller count:

	hw_resc->resv_irqs = le16_to_cpu(resp->alloc_msix);

__bnxt_reserve_rings() then handles the partial grant by trimming
cp_nr_rings and ulp_msix:

	resv_msix = hw_resc->resv_irqs - bp->cp_nr_rings;
	ulp_msix = min_t(int, resv_msix, ulp_msix);

However, bnxt_reserve_rings() still calls bnxt_change_msix() with the
untrimmed irqs_required. Wouldn't the entries from resv_irqs up to
irqs_required then be allocated and activated before FW backs them?
That is the state this patch is trying to avoid.

bnxt_change_msix() only programs indices at or above bp->total_irqs:

	for (i = bp->total_irqs; i < total; i++) {

so those entries would never be written again. FW could grant them
later, for example on a channel increase where the new count is at or
below total_irqs. Could the NQ rings bound to them then signal address
0, giving the same lost completions and TX watchdog timeouts?

It isn't clear how often FW returns a partial grant after
bnxt_hwrm_check_rings() has succeeded (NPAR, multi-host or VFs may be
cases). The driver does handle partial grants explicitly, though.

Would capping the growth in bnxt_reserve_rings() at hw_resc->resv_irqs
cover this case? Freeing and reallocating any vectors activated beyond
the reservation would be another option.

[Severity: Medium]

Does freeing the vectors here change how an MSI-X allocation failure
is handled?

Before this patch, if pci_msix_alloc_irq_at() failed, bnxt_check_rings()
caught it before any FW or ring state had changed. Now the growth
always happens in bnxt_reserve_rings(). By that point,
__bnxt_reserve_rings() has already committed the FW reservation and
updated bp->hw_resc.

The allocation can fail there, for example under memory pressure or if
another device takes the vectors released here. bnxt_change_msix() then
returns early with bp->total_irqs below irqs_required.
bnxt_reserve_rings() returns -ENOSPC and nothing is rolled back.

On the next open, bnxt_need_reserve_rings() compares only hw_resc
against the ring config:

	if ((bp->flags & BNXT_FLAG_CHIP_P5_PLUS) && BNXT_PF(bp) &&
	    hw_resc->resv_irqs != hwr.cp)
		return true;

It returns false, so bnxt_reserve_rings() returns before growing the
vectors:

	if (!bnxt_need_reserve_rings(bp))
		return 0;

bnxt_setup_int_mode() also skips reinitialization, because bp->irq_tbl
is non-NULL.

bnxt_request_irq() then requests an IRQ for every cp ring using
bp->irq_tbl[map_idx].vector, without checking map_idx against
bp->total_irqs. For entries at or above total_irqs, wouldn't .vector be
either 0 or a stale virq? The trim loop in bnxt_change_msix() frees the
vector but leaves irq_tbl[].vector set:

	for (i = bp->total_irqs; i > total; i--) {
		map.index = i - 1;
		map.virq = bp->irq_tbl[i - 1].vector;
		pci_msix_free_irq(bp->pdev, map);
		bp->total_irqs--;
	}

The open would then fail. If another device has reused the stale virq,
could the bnxt handler end up attached to an interrupt it doesn't own?

Running ethtool -L again with the same count seems to repeat the grow
and trim in bnxt_check_rings() and the early return in
bnxt_reserve_rings(). Is there a way to recover other than picking a
different ring configuration or a FW reset?

[Severity: High]

This is a pre-existing issue, not one introduced by this patch.
However, the new rollback adds another access to bp->irq_tbl in a
window that doesn't appear to be protected against PCI error recovery.

The DCB IEEE ETS path reaches this code holding only RTNL:

dcbnl_ieee_set()
  bnxt_dcbnl_ieee_setets()
    bnxt_setup_mq_tc()
      bnxt_check_rings()
        bnxt_change_msix()

net/dcb/dcbnl.c doesn't take the netdev instance lock.
bnxt_io_error_detected(), on the other hand, takes netdev_lock() but
not RTNL. For pci_channel_io_frozen it calls:

bnxt_io_error_detected()
  bnxt_fw_fatal_close()
    bnxt_clear_int_mode()

This frees bp->irq_tbl and sets it to NULL, but leaves bp->total_irqs
unchanged.

Suppose that runs between the grow in bnxt_check_rings() and the new
bnxt_change_msix(bp, old_total). The trim loop would see the stale
total_irqs, which is above old_total, and read
bp->irq_tbl[i - 1].vector through a NULL or freed pointer. The existing
write in the grow loop, bp->irq_tbl[i].vector = map.virq, has the same
race.

Should bnxt_dcbnl_ieee_setets() take the netdev instance lock? Taking
it inside bnxt_check_rings() would deadlock callers that already hold
it.

>  		}
>  	}
>  	return rc;

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