Re: [PATCH net-next 4/4] i40e: Avoid repeating RX filter warning
flat view
From: netdev-bot+sashiko@kernel.org
Date: 2026-09-04 09:19:59
Also in:
bpf
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: Critical: 0 · High: 0 · Medium: 0 (1) · Low: 1 - [Low] Two diagnostics that report different failure classes are now gated on one shared single-shot bit. Pre-existing issues: - [Medium] The overflow-promiscuous bit that the patch now also uses as a log-once latch lives in `vsi->state`, which is protected only by an… -- i40e: Avoid repeating RX filter warning The patch converts the two set_bit(__I40E_VSI_OVERFLOW_PROMISC, vsi->state) call sites in i40e_aqc_add_filters() and i40e_aqc_broadcast_filter() to test_and_set_bit(), so the "forcing overflow promiscuous" warning is only emitted on the transition into overflow promiscuous mode.
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c index 719f45b5cb409..abbc71e815ae3 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_main.c +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c@@ -2412,10 +2412,12 @@ void i40e_aqc_add_filters(struct i40e_vsi *vsi, const char *vsi_name, if (fcnt != num_add) { if (vsi->type == I40E_VSI_MAIN) { - set_bit(__I40E_VSI_OVERFLOW_PROMISC, vsi->state); - dev_warn(&vsi->back->pdev->dev, - "Error %s adding RX filters on %s, promiscuous mode forced on\n", - libie_aq_str(aq_status), vsi_name); + if (!test_and_set_bit(__I40E_VSI_OVERFLOW_PROMISC, + vsi->state)) { + dev_warn(&vsi->back->pdev->dev, + "Error %s adding RX filters on %s, promiscuous mode forced on\n", + libie_aq_str(aq_status), vsi_name); + }
[Severity: Medium] This isn't a bug introduced by this patch, but the warning is now latched on a bit whose lifetime is managed by an ad-hoc flag-as-lock scheme, so is the latch reliable? i40e_sync_vsi_filters() serializes its whole sleeping section with an open-coded acquire loop: while (test_and_set_bit(__I40E_VSI_SYNCING_FILTERS, vsi->state)) usleep_range(1000, 2000); pf = vsi->back; old_overflow = test_bit(__I40E_VSI_OVERFLOW_PROMISC, vsi->state); and releases it with a plain clear_bit(__I40E_VSI_SYNCING_FILTERS, vsi->state) on both the out: and err_no_memory: paths. i40e_add_vsi(), reached from i40e_rebuild() and i40e_vsi_setup(), clears the bit without participating in that gate at all: spin_unlock_bh(&vsi->mac_filter_hash_lock); clear_bit(__I40E_VSI_OVERFLOW_PROMISC, vsi->state); If that clear lands while i40e_sync_vsi_filters() is between its old_overflow snapshot and the new_overflow / promisc_threshold decision, can the warning re-fire, or the opposite, can the state read as non-overflowing while filter programming keeps failing?
quoted hunk ↗ jump to hunk
} else if (vsi->type == I40E_VSI_SRIOV || vsi->type == I40E_VSI_VMDQ1 || vsi->type == I40E_VSI_VMDQ2) {@@ -2465,10 +2467,11 @@ i40e_aqc_broadcast_filter(struct i40e_vsi *vsi, const char *vsi_name, } if (aq_ret) { - set_bit(__I40E_VSI_OVERFLOW_PROMISC, vsi->state); - dev_warn(&vsi->back->pdev->dev, - "Error %s, forcing overflow promiscuous on %s\n", - libie_aq_str(hw->aq.asq_last_status), vsi_name); + if (!test_and_set_bit(__I40E_VSI_OVERFLOW_PROMISC, vsi->state)) { + dev_warn(&vsi->back->pdev->dev, + "Error %s, forcing overflow promiscuous on %s\n", + libie_aq_str(hw->aq.asq_last_status), vsi_name); + } }
[Severity: Low]
Two different failure classes now share one single-shot bit. Is that
intended?
i40e_aqc_add_filters() warns when MAC/VLAN filter space is exhausted
(fcnt != num_add, typically LIBIE_AQ_RC_ENOSPC), while
i40e_aqc_broadcast_filter() warns on any AdminQ error returned by
i40e_aq_set_vsi_broadcast() or i40e_aq_set_vsi_bc_promisc_on_vlan(),
including errors that are not ENOSPC, and prints its own
libie_aq_str(hw->aq.asq_last_status) text.
In the scenario from the commit message (bridge vlan add vid 2-4094) the add
path sets __I40E_VSI_OVERFLOW_PROMISC first, so a later
i40e_aqc_broadcast_filter() failure of an unrelated class prints nothing
until the bit is cleared again.
For the delete path in i40e_sync_vsi_filters() the return value is
discarded:
if (is_broadcast_ether_addr(f->macaddr)) {
i40e_aqc_broadcast_filter(vsi, vsi_name, f);
hlist_del(&f->hlist);
kfree(f);
continue;
}
so with the warning suppressed, is that failure reported anywhere at all?
Would a per-site dev_warn_once() style latch, or a separate condition flag
per message, keep both diagnostics?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260901211752.1922993-1-anthony.l.nguyen%40intel.com