Re: [PATCH net-next 4/4] i40e: Avoid repeating RX filter warning

[netdev-bot+sashiko@kernel.org]

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.

> 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?

>  		} 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