Re: [PATCH net-next 3/3] i40e: trigger PF reset when re-enabling ATR via ethtool

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 4 · 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).

- [High] The reset request the patch adds is stored in the shared scalar
  `reset_needed`, which a later, unrelated error path zeroes — so the…
- [Medium] The failure mode this commit fixes was introduced two commits
  earlier in the same series by 6f0c883685a0 ("i40e: move ATR sample…
- [Medium] i40e_set_priv_flags() now requests a full PF reset on every
  0->1 transition of the flow-director-atr private flag, with no check…
- [Medium] The new ATR block assigns `reset_needed` with `=` instead of
  combining with the existing request, silently downgrading the stronger…
- [Medium] i40e_atr()'s gate on I40E_FLAG_FD_ATR_ENA is published to the
  lockless Tx fast path before the hardware context is updated, so the…

--

i40e: trigger PF reset when re-enabling ATR via ethtool

This adds a PF reset request in i40e_set_priv_flags() whenever the
flow-director-atr private flag transitions from off to on, so that
tx_ctx.fd_ena is recomputed in i40e_configure_tx_ring(). A few questions
about the new reset request and how it interacts with the rest of
i40e_set_priv_flags() are below.

First, a question about the commit message itself:

>     Toggling flow-director-atr via ethtool private flags does not trigger a
>     ring reset. If ATR was disabled while fd_ena was cleared in the TX queue
>     HW context (e.g. after a ring reset with both ATR and ntuple off),
>     re-enabling ATR causes i40e_atr() to inject FDIR descriptors into a
>     queue where fd_ena is still 0.

[Severity: Medium]
Is this describing a long-standing defect, or one introduced two commits
earlier in this same series by 6f0c883685a0 ("i40e: move ATR sample rate
from ring to PF level")?

Before that commit, i40e_configure_tx_ring() cleared the per-ring sample
rate in exactly the scenario described here:

	if (test_bit(I40E_FLAG_FD_ATR_ENA, vsi->back->flags))
		ring->atr_sample_rate = vsi->back->atr_sample_rate;
	else
		ring->atr_sample_rate = 0;

and i40e_atr() returned early on it:

	if (!tx_ring->atr_sample_rate)
		return;

So a ring configured with both ATR and ntuple off had fd_ena == 0 and
atr_sample_rate == 0, and re-enabling the private flag could not reach the
descriptor-writing code. 6f0c883685a0 removed that gate in favour of
pf->atr_sample_rate, which i40e_sw_init() initialises once to
I40E_DEFAULT_ATR_SAMPLE_RATE and is therefore never zero.

If that reading is right, the tree is broken at 6f0c883685a0 and
41347643da7b and repaired only here, which makes a bisect land in a window
with the described misprogramming. Would it be better to keep the gate (or
program fd_ena unconditionally) in the patch that removes it, and to add a
Fixes: tag here so stable backporters do not pick up a disruptive PF reset
for kernels that never had the regression?

> diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> index ef9d1e78c16a2..79c2cdab0f516 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> @@ -5301,6 +5301,13 @@ static int i40e_set_priv_flags(struct net_device *dev, u32 flags)
>  	    test_bit(I40E_FLAG_SOURCE_PRUNING_DIS, changed_flags))
>  		reset_needed = BIT(__I40E_PF_RESET_REQUESTED);
>  
> +	/* Re-enabling ATR requires a reset to update fd_ena in the
> +	 * TX queue HW context.
> +	 */
> +	if (test_bit(I40E_FLAG_FD_ATR_ENA, changed_flags) &&
> +	    test_bit(I40E_FLAG_FD_ATR_ENA, new_flags))
> +		reset_needed = BIT(__I40E_PF_RESET_REQUESTED);
> +

[Severity: Medium]
Can this assignment discard the stronger reset type requested a few lines
above in the same function?

i40e_set_priv_flags() has already run:

	if (test_bit(I40E_FLAG_FW_LLDP_DIS, changed_flags))
		reset_needed = I40E_PF_RESET_AND_REBUILD_FLAG;

and the new block overwrites reset_needed with BIT(__I40E_PF_RESET_REQUESTED)
using plain assignment, so a single ethtool write that changes
disable-fw-lldp and flow-director-atr together ends up with the weaker
selector.

i40e_do_reset() then tests the two in an else-if chain, with the PF reset
branch first:

	} else if (reset_flags & I40E_PF_RESET_FLAG) {
		i40e_handle_reset_warning(pf, lock_acquired);

	} else if (reset_flags & I40E_PF_RESET_AND_REBUILD_FLAG) {
		i40e_prep_for_reset(pf);
		i40e_reset_and_rebuild(pf, true, lock_acquired);

so reinit is false and i40e_setup_pf_switch() skips the main VSI
re-initialisation that the FW LLDP change asked for:

	else if (reinit)
		main_vsi = i40e_vsi_reinit_setup(main_vsi);

The "FW LLDP is enabled/disabled" dev_info() in the rebuild branch is lost
as well. Note that changing = to |= would not be sufficient here, since
i40e_do_reset() prefers I40E_PF_RESET_FLAG over
I40E_PF_RESET_AND_REBUILD_FLAG; does the selection logic need reworking so
the stronger request wins?

[Severity: High]
Can the reset this patch depends on be dropped later in the same call while
the ATR flag is still committed?

Further down in i40e_set_priv_flags(), the FW LLDP re-enable path clears
reset_needed on a non-fatal admin queue status:

	status = i40e_aq_start_lldp(&pf->hw, false, NULL);
	if (status) {
		adq_err = pf->hw.aq.asq_last_status;
		switch (adq_err) {
		case LIBIE_AQ_RC_EEXIST:
			dev_warn(&pf->pdev->dev,
				 "FW LLDP agent is already running\n");
			reset_needed = 0;
			break;

and the flag publication is unconditional while the reset is not:

	bitmap_copy(pf->flags, new_flags, I40E_PF_FLAGS_NBITS);

	if (reset_needed)
		i40e_do_reset(pf, reset_needed, true);

Since tx_ctx.fd_ena is only recomputed when the Tx queue context is
rewritten:

i40e_configure_tx_ring()
	if (test_bit(I40E_FLAG_FD_SB_ENA, vsi->back->flags) ||
	    test_bit(I40E_FLAG_FD_ATR_ENA, vsi->back->flags))
		tx_ctx.fd_ena = 1;

does this leave I40E_FLAG_FD_ATR_ENA set in pf->flags with fd_ena still 0,
which is the state the commit message says it is closing? The private-flags
ioctl carries the whole bitmap, so disable-fw-lldp off and
flow-director-atr on can be changed in one command while the FW agent is
already running.

[Severity: Medium]
Is there a window between publishing the flag and quiescing Tx where
i40e_atr() can still post descriptors to a ring with fd_ena == 0?

The flag becomes visible to the lockless Tx path at bitmap_copy(pf->flags,
new_flags, ...), and only the subsequent i40e_do_reset() reaches
i40e_prep_for_reset() -> i40e_pf_quiesce_all_vsi(). Meanwhile i40e_atr(),
called from i40e_xmit_frame_ring() in softirq context on another CPU, gates
only on software state:

	/* make sure ATR is enabled */
	if (!test_bit(I40E_FLAG_FD_ATR_ENA, pf->flags))
		return;

	if (test_bit(__I40E_FD_ATR_AUTO_DISABLED, pf->state))
		return;

	sample_rate = READ_ONCE(pf->atr_sample_rate);
	if (!sample_rate)
		return;

__I40E_FD_ATR_AUTO_DISABLED does not appear to cover the window either, as
it is cleared asynchronously in i40e_fdir_flush_and_replay():

		if (!disable_atr && !pf->fd_tcp4_filter_cnt)
			clear_bit(__I40E_FD_ATR_AUTO_DISABLED, pf->state);

as well as in i40e_reenable_fdir_atr() and i40e_set_ntuple(). Would keeping
ATR suppressed until after the rebuild (for example holding
__I40E_FD_ATR_AUTO_DISABLED across the re-enable and clearing it once the Tx
context has been reprogrammed, or publishing the flag after the reset) close
this ordering gap?

[Severity: Medium]
Should the reset be conditional on the HW context actually being stale?

i40e_configure_tx_ring() sets tx_ctx.fd_ena = 1 when either
I40E_FLAG_FD_SB_ENA or I40E_FLAG_FD_ATR_ENA is set, so on a system with
ntuple/side-band filtering enabled, fd_ena is already 1 and nothing needs to
change. With this patch, the sequence

	ethtool --set-priv-flags ethX flow-director-atr off
	ethtool --set-priv-flags ethX flow-director-atr on

always runs i40e_do_reset(pf, BIT(__I40E_PF_RESET_REQUESTED), true) ->
i40e_handle_reset_warning() -> i40e_prep_for_reset(), which quiesces all
VSIs, calls i40e_vc_notify_reset() for VFs and shuts down the admin queue
and HMC, turning a previously non-disruptive flag write into a traffic stop,
link flap and VF reset.

Could the condition also test that FD_SB is off (so fd_ena can really be 0),
or alternatively could i40e_configure_tx_ring() program fd_ena from the FD
capability rather than the current flag state so no reset is needed at all?
The commit message says a reset is triggered but does not mention that it
happens unconditionally, including when fd_ena cannot be stale.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917204541.3072415-1-anthony.l.nguyen%40intel.com