[PATCH net-next 0/3][pull request] i40e: add runtime-tunable ATR sample rate

[PATCH net-next 0/3][pull request] i40e: add runtime-tunable ATR sample rate

[Tony Nguyen]

Mohammad Heib says:

This series makes the Flow Director ATR sample rate configurable at
runtime through a devlink parameter.

On systems with many queues and high-rate TCP workloads, the fixed
default sampling interval can cause frequent Flow Director
reprogramming and TCP packet reordering. The optimal interval depends
on the workload and system configuration.

Patch 1 moves atr_sample_rate from per-ring to PF level, since it is
a global policy.

Patch 2 exposes it as a devlink runtime parameter so administrators
can tune it without rebuilding the driver.

Patch 3 fixes a pre-existing issue where re-enabling ATR via ethtool
private flags did not trigger a ring reset, leaving fd_ena stale in
the TX queue HW context.

The following are changes since commit 26ee8cd69d46a14b37ba5e512084fe80d730127a:
  net: qualcomm: rmnet: require CAP_NET_ADMIN in the real device netns for config ops
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/next-queue 40GbE

Mohammad Heib (3):
  i40e: move ATR sample rate from ring to PF level
  i40e: add devlink parameter for Flow Director ATR sample rate
  i40e: trigger PF reset when re-enabling ATR via ethtool

 Documentation/networking/devlink/i40e.rst     | 20 +++++++++++
 drivers/net/ethernet/intel/i40e/i40e.h        |  1 +
 .../net/ethernet/intel/i40e/i40e_devlink.c    | 36 +++++++++++++++++++
 .../net/ethernet/intel/i40e/i40e_ethtool.c    |  7 ++++
 drivers/net/ethernet/intel/i40e/i40e_main.c   |  9 ++---
 drivers/net/ethernet/intel/i40e/i40e_txrx.c   |  6 ++--
 drivers/net/ethernet/intel/i40e/i40e_txrx.h   |  3 +-
 7 files changed, 72 insertions(+), 10 deletions(-)

-- 
2.47.1

[PATCH net-next 1/3] i40e: move ATR sample rate from ring to PF level

[Tony Nguyen]

From: Mohammad Heib <mheib@redhat.com>

The ATR sample rate is currently stored per-ring and initialized when each
TX ring is configured. Since the sample rate is a global policy that
applies uniformly across all rings, it makes more sense to store it at
the PF level.

Move atr_sample_rate from struct i40e_ring to struct i40e_pf and initialize
it once during i40e_sw_init(). Update i40e_atr() to reference the PF-level
field. Change atr_count from u8 to u32 to match the sample rate type.

Signed-off-by: Mohammad Heib <mheib@redhat.com>
Tested-by: Rinitha S <sx.rinitha@intel.com> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e.h      | 1 +
 drivers/net/ethernet/intel/i40e/i40e_main.c | 9 +++------
 drivers/net/ethernet/intel/i40e/i40e_txrx.c | 6 ++++--
 drivers/net/ethernet/intel/i40e/i40e_txrx.h | 3 +--
 4 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e.h b/drivers/net/ethernet/intel/i40e/i40e.h
index 1b6a8fbaa648..88eb40ee45f0 100644
--- a/drivers/net/ethernet/intel/i40e/i40e.h
+++ b/drivers/net/ethernet/intel/i40e/i40e.h
@@ -487,6 +487,7 @@ struct i40e_pf {
 	u16 rss_size_max;          /* HW defined max RSS queues */
 	u16 fdir_pf_filter_count;  /* num of guaranteed filters for this PF */
 	u16 num_alloc_vsi;         /* num VSIs this driver supports */
+	u32 atr_sample_rate;
 	bool wol_en;
 
 	struct hlist_head fdir_filter_list;
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index abbc71e815ae..0a4272e1ab07 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -3458,12 +3458,7 @@ static int i40e_configure_tx_ring(struct i40e_ring *ring)
 		ring->xsk_pool = i40e_xsk_pool(ring);
 
 	/* some ATR related tx ring init */
-	if (test_bit(I40E_FLAG_FD_ATR_ENA, vsi->back->flags)) {
-		ring->atr_sample_rate = I40E_DEFAULT_ATR_SAMPLE_RATE;
-		ring->atr_count = 0;
-	} else {
-		ring->atr_sample_rate = 0;
-	}
+	ring->atr_count = 0;
 
 	/* configure XPS */
 	i40e_config_xps_tx_ring(ring);
@@ -12740,6 +12735,8 @@ static int i40e_sw_init(struct i40e_pf *pf)
 		}
 	}
 
+	pf->atr_sample_rate = I40E_DEFAULT_ATR_SAMPLE_RATE;
+
 	if ((pf->hw.func_caps.fd_filters_guaranteed > 0) ||
 	    (pf->hw.func_caps.fd_filters_best_effort > 0)) {
 		set_bit(I40E_FLAG_FD_ATR_ENA, pf->flags);
diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
index 7f68adb5109b..e045f9c71c94 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
@@ -2871,6 +2871,7 @@ static void i40e_atr(struct i40e_ring *tx_ring, struct sk_buff *skb,
 	struct tcphdr *th;
 	unsigned int hlen;
 	u32 flex_ptype, dtype_cmd;
+	u32 sample_rate;
 	int l4_proto;
 	u16 i;
 
@@ -2882,7 +2883,8 @@ static void i40e_atr(struct i40e_ring *tx_ring, struct sk_buff *skb,
 		return;
 
 	/* if sampling is disabled do nothing */
-	if (!tx_ring->atr_sample_rate)
+	sample_rate = READ_ONCE(pf->atr_sample_rate);
+	if (!sample_rate)
 		return;
 
 	/* Currently only IPv4/IPv6 with TCP is supported */
@@ -2934,7 +2936,7 @@ static void i40e_atr(struct i40e_ring *tx_ring, struct sk_buff *skb,
 	if (!th->fin &&
 	    !th->syn &&
 	    !th->rst &&
-	    (tx_ring->atr_count < tx_ring->atr_sample_rate))
+	    (tx_ring->atr_count < sample_rate))
 		return;
 
 	tx_ring->atr_count = 0;
diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.h b/drivers/net/ethernet/intel/i40e/i40e_txrx.h
index bb741ff3e5f2..be587f804e7a 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.h
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.h
@@ -372,8 +372,7 @@ struct i40e_ring {
 	u16 next_to_clean;
 	u16 xdp_tx_active;
 
-	u8 atr_sample_rate;
-	u8 atr_count;
+	u32 atr_count;
 
 	bool ring_active;		/* is ring online or not */
 	bool arm_wb;		/* do something to arm write back */
-- 
2.47.1

Re: [PATCH net-next 1/3] i40e: move ATR sample rate from ring to PF level

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 1 · 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] Concern (discarded on baseline verification): removing the per-
  ring atr_sample_rate decoupled the software ATR gate from…
- [Medium] Undisclosed user-visible behaviour change: "i40e: move ATR
  sample rate from ring to PF level" is written as a pure storage…

--

i40e: move ATR sample rate from ring to PF level

This moves atr_sample_rate out of struct i40e_ring and into struct i40e_pf,
initializing it once in i40e_sw_init() instead of in i40e_configure_tx_ring().
i40e_atr() now reads the PF-level value, and ring->atr_count grows from u8 to
u32.

> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index abbc71e815ae3..0a4272e1ab070 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -3458,12 +3458,7 @@ static int i40e_configure_tx_ring(struct i40e_ring *ring)
>  		ring->xsk_pool = i40e_xsk_pool(ring);
>  
>  	/* some ATR related tx ring init */
> -	if (test_bit(I40E_FLAG_FD_ATR_ENA, vsi->back->flags)) {
> -		ring->atr_sample_rate = I40E_DEFAULT_ATR_SAMPLE_RATE;
> -		ring->atr_count = 0;
> -	} else {
> -		ring->atr_sample_rate = 0;
> -	}
> +	ring->atr_count = 0;

[Severity: Medium]
The commit message describes this as a storage relocation: "Since the sample
rate is a global policy that applies uniformly across all rings, it makes more
sense to store it at the PF level."  Does dropping this conditional also change
when the writable "flow-director-atr" private flag takes effect?

Before this patch, the per-ring rate was latched from I40E_FLAG_FD_ATR_ENA at
the time the Tx ring was configured, and i40e_atr() bailed out on
!tx_ring->atr_sample_rate.  i40e_set_priv_flags() does not add
I40E_FLAG_FD_ATR_ENA to reset_needed and only handles the disable direction:

	/* Flush current ATR settings if ATR was disabled */
	if (test_bit(I40E_FLAG_FD_ATR_ENA, changed_flags) &&
	    !test_bit(I40E_FLAG_FD_ATR_ENA, new_flags)) {
		set_bit(__I40E_FD_ATR_AUTO_DISABLED, pf->state);
		set_bit(__I40E_FD_FLUSH_REQUESTED, pf->state);
	}

So on a live interface whose rings were configured while ATR was off,
"ethtool --set-priv-flags ethX flow-director-atr on" did not resume sampling
until an ifdown/ifup or a PF reset.  With the PF-level rate now always
non-zero, the flag alone re-enables descriptor generation immediately.

Could the commit message mention this user-visible semantic change, and should
it carry a Fixes tag if the old latching behaviour was considered a bug?

[ ... ]

> @@ -12740,6 +12735,8 @@ static int i40e_sw_init(struct i40e_pf *pf)
>  		}
>  	}
>  
> +	pf->atr_sample_rate = I40E_DEFAULT_ATR_SAMPLE_RATE;
> +
>  	if ((pf->hw.func_caps.fd_filters_guaranteed > 0) ||
>  	    (pf->hw.func_caps.fd_filters_best_effort > 0)) {
>  		set_bit(I40E_FLAG_FD_ATR_ENA, pf->flags);

[Severity: High]
This assignment is unconditional and happens before the Flow Director
capability detection below it.  Can that decouple the software ATR gate from
the Flow Director enable bit programmed into the Tx queue context?

i40e_configure_tx_ring() still snapshots the flags into the HMC context:

	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;

The matching software snapshot (ring->atr_sample_rate) is what this patch
removes, so after toggling "flow-director-atr" on without a reconfiguration,
i40e_atr() would pass the new gate:

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

and go on to write an I40E_TX_DESC_DTYPE_FILTER_PROG descriptor and advance
tx_ring->next_to_use on a queue whose context was programmed with fd_ena = 0.
Would that produce an MDD event or bogus FD programming status on that queue?

For completeness, the later patch in this series, "i40e: trigger PF reset when
re-enabling ATR via ethtool", adds a reset on the enable transition in
i40e_set_priv_flags(), which re-programs the queue contexts and addresses this
for the series as a whole.  It does leave a window between
bitmap_copy(pf->flags, new_flags) and i40e_do_reset(), and it means this
intermediate commit alone is not bisect-clean in that respect.  Would it make
sense to fold the reset change into this patch, or to order it first?

[ ... ]

> diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> index 7f68adb5109b0..e045f9c71c944 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> @@ -2882,7 +2883,8 @@ static void i40e_atr(struct i40e_ring *tx_ring, struct sk_buff *skb,
>  		return;
>  
>  	/* if sampling is disabled do nothing */
> -	if (!tx_ring->atr_sample_rate)
> +	sample_rate = READ_ONCE(pf->atr_sample_rate);
> +	if (!sample_rate)
>  		return;
>  
>  	/* Currently only IPv4/IPv6 with TCP is supported */

[ ... ]

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

Re: [PATCH net-next 1/3] i40e: move ATR sample rate from ring to PF level

[Mohammad Heib]

On Mon, Sep 21, 2026 at 09:32:20PM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 1 · 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] Concern (discarded on baseline verification): removing the per-
>   ring atr_sample_rate decoupled the software ATR gate from…
> - [Medium] Undisclosed user-visible behaviour change: "i40e: move ATR
>   sample rate from ring to PF level" is written as a pure storage…
> 
> --
> 
> i40e: move ATR sample rate from ring to PF level
> 
> This moves atr_sample_rate out of struct i40e_ring and into struct i40e_pf,
> initializing it once in i40e_sw_init() instead of in i40e_configure_tx_ring().
> i40e_atr() now reads the PF-level value, and ring->atr_count grows from u8 to
> u32.
> 
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> > index abbc71e815ae3..0a4272e1ab070 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> > @@ -3458,12 +3458,7 @@ static int i40e_configure_tx_ring(struct i40e_ring *ring)
> >  		ring->xsk_pool = i40e_xsk_pool(ring);
> >  
> >  	/* some ATR related tx ring init */
> > -	if (test_bit(I40E_FLAG_FD_ATR_ENA, vsi->back->flags)) {
> > -		ring->atr_sample_rate = I40E_DEFAULT_ATR_SAMPLE_RATE;
> > -		ring->atr_count = 0;
> > -	} else {
> > -		ring->atr_sample_rate = 0;
> > -	}
> > +	ring->atr_count = 0;
> 
> [Severity: Medium]
> The commit message describes this as a storage relocation: "Since the sample
> rate is a global policy that applies uniformly across all rings, it makes more
> sense to store it at the PF level."  Does dropping this conditional also change
> when the writable "flow-director-atr" private flag takes effect?
> 
> Before this patch, the per-ring rate was latched from I40E_FLAG_FD_ATR_ENA at
> the time the Tx ring was configured, and i40e_atr() bailed out on
> !tx_ring->atr_sample_rate.  i40e_set_priv_flags() does not add
> I40E_FLAG_FD_ATR_ENA to reset_needed and only handles the disable direction:
> 
> 	/* Flush current ATR settings if ATR was disabled */
> 	if (test_bit(I40E_FLAG_FD_ATR_ENA, changed_flags) &&
> 	    !test_bit(I40E_FLAG_FD_ATR_ENA, new_flags)) {
> 		set_bit(__I40E_FD_ATR_AUTO_DISABLED, pf->state);
> 		set_bit(__I40E_FD_FLUSH_REQUESTED, pf->state);
> 	}
> 
> So on a live interface whose rings were configured while ATR was off,
> "ethtool --set-priv-flags ethX flow-director-atr on" did not resume sampling
> until an ifdown/ifup or a PF reset.  With the PF-level rate now always
> non-zero, the flag alone re-enables descriptor generation immediately.
> 
> Could the commit message mention this user-visible semantic change, and should
> it carry a Fixes tag if the old latching behaviour was considered a bug?

Yes, this is a real behavior change. The v5 commit message now documents
it explicitly.
> 
> [ ... ]
> 
> > @@ -12740,6 +12735,8 @@ static int i40e_sw_init(struct i40e_pf *pf)
> >  		}
> >  	}
> >  
> > +	pf->atr_sample_rate = I40E_DEFAULT_ATR_SAMPLE_RATE;
> > +
> >  	if ((pf->hw.func_caps.fd_filters_guaranteed > 0) ||
> >  	    (pf->hw.func_caps.fd_filters_best_effort > 0)) {
> >  		set_bit(I40E_FLAG_FD_ATR_ENA, pf->flags);
> 
> [Severity: High]
> This assignment is unconditional and happens before the Flow Director
> capability detection below it.  Can that decouple the software ATR gate from
> the Flow Director enable bit programmed into the Tx queue context?
> 
> i40e_configure_tx_ring() still snapshots the flags into the HMC context:
> 
> 	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;
> 
> The matching software snapshot (ring->atr_sample_rate) is what this patch
> removes, so after toggling "flow-director-atr" on without a reconfiguration,
> i40e_atr() would pass the new gate:
> 
> 	sample_rate = READ_ONCE(pf->atr_sample_rate);
> 	if (!sample_rate)
> 		return;
> 
> and go on to write an I40E_TX_DESC_DTYPE_FILTER_PROG descriptor and advance
> tx_ring->next_to_use on a queue whose context was programmed with fd_ena = 0.
> Would that produce an MDD event or bogus FD programming status on that queue?
> 
> For completeness, the later patch in this series, "i40e: trigger PF reset when
> re-enabling ATR via ethtool", adds a reset on the enable transition in
> i40e_set_priv_flags(), which re-programs the queue contexts and addresses this
> for the series as a whole.  It does leave a window between
> bitmap_copy(pf->flags, new_flags) and i40e_do_reset(), and it means this
> intermediate commit alone is not bisect-clean in that respect.  Would it make
> sense to fold the reset change into this patch, or to order it first?

Addressed in v5 by folding the PF reset patch (was 3/3) into this
commit. The reset on ATR re-enable now lands in the same commit that
removes the per-ring gate, so the tree is bisect-clean at every commit.

> 
> [ ... ]
> 
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> > index 7f68adb5109b0..e045f9c71c944 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> > @@ -2882,7 +2883,8 @@ static void i40e_atr(struct i40e_ring *tx_ring, struct sk_buff *skb,
> >  		return;
> >  
> >  	/* if sampling is disabled do nothing */
> > -	if (!tx_ring->atr_sample_rate)
> > +	sample_rate = READ_ONCE(pf->atr_sample_rate);
> > +	if (!sample_rate)
> >  		return;
> >  
> >  	/* Currently only IPv4/IPv6 with TCP is supported */
> 
> [ ... ]
> 
pw-bot: cr
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917204541.3072415-1-anthony.l.nguyen%40intel.com

[PATCH net-next 2/3] i40e: add devlink parameter for Flow Director ATR sample rate

[Tony Nguyen]

From: Mohammad Heib <mheib@redhat.com>

The i40e driver uses Flow Director ATR to periodically update flow
steering information for active TCP flows. The update frequency is
currently controlled by I40E_DEFAULT_ATR_SAMPLE_RATE and is fixed at
driver build time.

On systems with a large number of queues and high-rate TCP workloads,
the default sampling interval can result in frequent Flow Director
reprogramming for long-lived flows.

The amount of TCP packet reordering observed on some systems is
sensitive to the ATR sampling interval. Increasing the interval reduces
Flow Director programming activity and can significantly reduce the
associated reordering.

Since the optimal sampling interval depends on the workload and system
configuration, a single fixed value is not suitable for all deployments.

Add a devlink parameter to allow administrators to tune the ATR sample
rate at runtime without rebuilding the driver or disabling ATR
functionality entirely.

Signed-off-by: Mohammad Heib <mheib@redhat.com>
Tested-by: Rinitha S <sx.rinitha@intel.com> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 Documentation/networking/devlink/i40e.rst     | 20 +++++++++++
 .../net/ethernet/intel/i40e/i40e_devlink.c    | 36 +++++++++++++++++++
 2 files changed, 56 insertions(+)

diff --git a/Documentation/networking/devlink/i40e.rst b/Documentation/networking/devlink/i40e.rst
index 51c887f0dc83..2cea98b631ba 100644
--- a/Documentation/networking/devlink/i40e.rst
+++ b/Documentation/networking/devlink/i40e.rst
@@ -40,6 +40,26 @@ Parameters
 
         The default value is ``0`` (internal calculation is used).
 
+.. list-table:: Driver specific parameters implemented
+    :widths: 5 5 90
+
+    * - Name
+      - Mode
+      - Description
+    * - ``atr_sample_rate``
+      - runtime
+      - Controls how frequently Flow Director ATR updates flow steering
+        information for active TCP flows.
+
+        ATR programs Flow Director entries based on sampled transmitted
+        packets. The sampling interval is specified as the number of
+        transmitted packets between ATR updates.
+
+        Lower values increase Flow Director programming activity, while
+        higher values reduce the update frequency.
+
+        Setting to ``0`` disables ATR sampling (no filters will be programmed)
+        The default value is ``20``.
 
 Info versions
 =============
diff --git a/drivers/net/ethernet/intel/i40e/i40e_devlink.c b/drivers/net/ethernet/intel/i40e/i40e_devlink.c
index 229179ccc131..6df7eb217073 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_devlink.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_devlink.c
@@ -33,12 +33,48 @@ static int i40e_max_mac_per_vf_get(struct devlink *devlink,
 	return 0;
 }
 
+static int i40e_atr_sample_rate_set(struct devlink *devlink,
+				    u32 id,
+				    struct devlink_param_gset_ctx *ctx,
+				    struct netlink_ext_ack *extack)
+{
+	struct i40e_pf *pf = devlink_priv(devlink);
+	u32 sample_rate = ctx->val.vu32;
+
+	WRITE_ONCE(pf->atr_sample_rate, sample_rate);
+	return 0;
+}
+
+static int i40e_atr_sample_rate_get(struct devlink *devlink,
+				    u32 id,
+				    struct devlink_param_gset_ctx *ctx,
+				    struct netlink_ext_ack *extack)
+{
+	struct i40e_pf *pf = devlink_priv(devlink);
+
+	ctx->val.vu32 = READ_ONCE(pf->atr_sample_rate);
+
+	return 0;
+}
+
+enum i40e_dl_param_id {
+	I40E_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX,
+	I40E_DEVLINK_PARAM_ID_ATR_SAMPLE_RATE,
+};
+
 static const struct devlink_param i40e_dl_params[] = {
 	DEVLINK_PARAM_GENERIC(MAX_MAC_PER_VF,
 			      BIT(DEVLINK_PARAM_CMODE_RUNTIME),
 			      i40e_max_mac_per_vf_get,
 			      i40e_max_mac_per_vf_set,
 			      NULL),
+	DEVLINK_PARAM_DRIVER(I40E_DEVLINK_PARAM_ID_ATR_SAMPLE_RATE,
+			     "atr_sample_rate",
+			     DEVLINK_PARAM_TYPE_U32,
+			     BIT(DEVLINK_PARAM_CMODE_RUNTIME),
+			     i40e_atr_sample_rate_get,
+			     i40e_atr_sample_rate_set,
+			     NULL),
 };
 
 static void i40e_info_get_dsn(struct i40e_pf *pf, char *buf, size_t len)
-- 
2.47.1

Re: [PATCH net-next 2/3] i40e: add devlink parameter for Flow Director ATR sample rate

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · 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] The new devlink `atr_sample_rate` setter
  (drivers/net/ethernet/intel/i40e/i40e_devlink.c:36-46) makes the value…
- [Medium] The new documentation
  (Documentation/networking/devlink/i40e.rst, lines 54-59) states that…
- [Low] In the new documentation entry the lines 'Setting to ``0``
  disables ATR sampling (no filters will be programmed)' and 'The…

--

i40e: add devlink parameter for Flow Director ATR sample rate

This adds a runtime devlink parameter, atr_sample_rate, that lets the
Flow Director ATR sampling interval be changed without rebuilding the
driver.  The value is stored in pf->atr_sample_rate and read by the Tx
path, and a new entry documents it in the i40e devlink documentation.

> diff --git a/Documentation/networking/devlink/i40e.rst b/Documentation/networking/devlink/i40e.rst
> index 51c887f0dc833..2cea98b631baa 100644
> --- a/Documentation/networking/devlink/i40e.rst
> +++ b/Documentation/networking/devlink/i40e.rst
> @@ -40,6 +40,26 @@ Parameters
>  
>          The default value is ``0`` (internal calculation is used).
>  
> +.. list-table:: Driver specific parameters implemented
> +    :widths: 5 5 90
> +
> +    * - Name
> +      - Mode
> +      - Description

[Severity: Low]
This isn't a bug, but the third column here is labelled Description while
the generic parameter table above it in the same file uses Notes.  Was the
difference intended?

> +    * - ``atr_sample_rate``
> +      - runtime
> +      - Controls how frequently Flow Director ATR updates flow steering
> +        information for active TCP flows.
> +
> +        ATR programs Flow Director entries based on sampled transmitted
> +        packets. The sampling interval is specified as the number of
> +        transmitted packets between ATR updates.
> +
> +        Lower values increase Flow Director programming activity, while
> +        higher values reduce the update frequency.

[Severity: Medium]
Is "the number of transmitted packets between ATR updates" the unit the
code actually implements?  Three details in i40e_atr() seem to differ.

The counter compared against the configured value is per Tx ring, not
per device:

drivers/net/ethernet/intel/i40e/i40e_txrx.c:i40e_atr() {
	...
	tx_ring->atr_count++;

	/* sample on all syn/fin/rst packets or once every atr sample rate */
	if (!th->fin &&
	    !th->syn &&
	    !th->rst &&
	    (tx_ring->atr_count < sample_rate))
		return;
	...
}

atr_count lives in struct i40e_ring, so on an N-queue device the
aggregate programming rate is up to N times what a device-wide packet
interval would suggest.  Given the commit message mentions "systems with
a large number of queues", should the documentation say the interval is
per Tx queue?

The increment is also only reached for TCP after the IP version and
protocol checks:

drivers/net/ethernet/intel/i40e/i40e_txrx.c:i40e_atr() {
	...
	if (l4_proto != IPPROTO_TCP)
		return;
	...
}

so non-TCP transmitted packets never advance the interval.

Finally, the threshold is bypassed for SYN, FIN and RST, so raising the
value does not reduce programming activity for connection-churn
workloads.  Should the text mention that exception?

> +
> +        Setting to ``0`` disables ATR sampling (no filters will be programmed)
> +        The default value is ``20``.

[Severity: Low]
These two lines have no blank line between them and the first one has no
terminating period, so reStructuredText joins them into a single
paragraph and renders as "... (no filters will be programmed) The default
value is 20.".  The max_mac_per_vf entry just above puts a blank line
before its "The default value is ..." sentence.

>  
>  Info versions
>  =============
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_devlink.c b/drivers/net/ethernet/intel/i40e/i40e_devlink.c
> index 229179ccc1319..6df7eb217073d 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_devlink.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_devlink.c
> @@ -33,12 +33,48 @@ static int i40e_max_mac_per_vf_get(struct devlink *devlink,
>  	return 0;
>  }
>  
> +static int i40e_atr_sample_rate_set(struct devlink *devlink,
> +				    u32 id,
> +				    struct devlink_param_gset_ctx *ctx,
> +				    struct netlink_ext_ack *extack)
> +{
> +	struct i40e_pf *pf = devlink_priv(devlink);
> +	u32 sample_rate = ctx->val.vu32;
> +
> +	WRITE_ONCE(pf->atr_sample_rate, sample_rate);
> +	return 0;
> +}

[Severity: Medium]
Setting the value to 0 is now reachable at runtime while
I40E_FLAG_FD_ATR_ENA stays set.  Does this leave already programmed ATR
entries resident in the Flow Director table with no way to remove them?

In i40e_atr() the zero-rate return happens before the FIN/RST handling
and before the descriptor build that emits the remove command:

drivers/net/ethernet/intel/i40e/i40e_txrx.c:i40e_atr() {
	...
	/* if sampling is disabled do nothing */
	sample_rate = READ_ONCE(pf->atr_sample_rate);
	if (!sample_rate)
		return;
	...
	dtype_cmd |= (th->fin || th->rst) ?
		     (I40E_FILTER_PROGRAM_DESC_PCMD_REMOVE <<
		      I40E_TXD_FLTR_QW1_PCMD_SHIFT) :
	...
}

That PCMD_REMOVE descriptor is the only software removal path, and it is
only redundant when hardware eviction is available:

drivers/net/ethernet/intel/i40e/i40e_txrx.c:i40e_atr() {
	...
	if (test_bit(I40E_FLAG_HW_ATR_EVICT_ENA, pf->flags)) {
		/* HW ATR eviction will take care of removing filters on FIN
		 * and RST packets.
		 */
	...
}

I40E_FLAG_HW_ATR_EVICT_ENA is only set in i40e_sw_init() when
pf->hw.caps has I40E_HW_CAP_ATR_EVICT, and it can also be cleared by the
hw-atr-eviction ethtool private flag.  On such a device, after any TCP
traffic has programmed ATR filters, a

  devlink dev param set pci/<bdf> name atr_sample_rate value 0 cmode runtime

appears to leave those entries steering received packets for flows that
have since closed, while fd_atr_cnt and i40e_get_current_atr_cnt() still
report ATR as active.

The existing way to turn ATR off via the flow-director-atr private flag
does the teardown explicitly:

drivers/net/ethernet/intel/i40e/i40e_ethtool.c:i40e_set_priv_flags() {
	...
	/* Flush current ATR settings if ATR was disabled */
	if (test_bit(I40E_FLAG_FD_ATR_ENA, changed_flags) &&
	    !test_bit(I40E_FLAG_FD_ATR_ENA, new_flags)) {
		set_bit(__I40E_FD_ATR_AUTO_DISABLED, pf->state);
		set_bit(__I40E_FD_FLUSH_REQUESTED, pf->state);
	}
	...
}

Should the devlink setter do the same when the new rate is 0, or should
0 be rejected by a validate callback instead?  Note the later patch in
the series that triggers a PF reset when re-enabling ATR via ethtool only
touches i40e_set_priv_flags(), so it does not add this flush.

> +
> +static int i40e_atr_sample_rate_get(struct devlink *devlink,

[ ... ]

>  static const struct devlink_param i40e_dl_params[] = {

[ ... ]

> +	DEVLINK_PARAM_DRIVER(I40E_DEVLINK_PARAM_ID_ATR_SAMPLE_RATE,
> +			     "atr_sample_rate",
> +			     DEVLINK_PARAM_TYPE_U32,
> +			     BIT(DEVLINK_PARAM_CMODE_RUNTIME),
> +			     i40e_atr_sample_rate_get,
> +			     i40e_atr_sample_rate_set,
> +			     NULL),
>  };

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

Re: [PATCH net-next 2/3] i40e: add devlink parameter for Flow Director ATR sample rate

[Mohammad Heib]

On Mon, Sep 21, 2026 at 09:32:21PM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 2 · 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] The new devlink `atr_sample_rate` setter
>   (drivers/net/ethernet/intel/i40e/i40e_devlink.c:36-46) makes the value…
> - [Medium] The new documentation
>   (Documentation/networking/devlink/i40e.rst, lines 54-59) states that…
> - [Low] In the new documentation entry the lines 'Setting to ``0``
>   disables ATR sampling (no filters will be programmed)' and 'The…
> 
> --
> 
> i40e: add devlink parameter for Flow Director ATR sample rate
> 
> This adds a runtime devlink parameter, atr_sample_rate, that lets the
> Flow Director ATR sampling interval be changed without rebuilding the
> driver.  The value is stored in pf->atr_sample_rate and read by the Tx
> path, and a new entry documents it in the i40e devlink documentation.
> 
> > diff --git a/Documentation/networking/devlink/i40e.rst b/Documentation/networking/devlink/i40e.rst
> > index 51c887f0dc833..2cea98b631baa 100644
> > --- a/Documentation/networking/devlink/i40e.rst
> > +++ b/Documentation/networking/devlink/i40e.rst
> > @@ -40,6 +40,26 @@ Parameters
> >  
> >          The default value is ``0`` (internal calculation is used).
> >  
> > +.. list-table:: Driver specific parameters implemented
> > +    :widths: 5 5 90
> > +
> > +    * - Name
> > +      - Mode
> > +      - Description
> 
> [Severity: Low]
> This isn't a bug, but the third column here is labelled Description while
> the generic parameter table above it in the same file uses Notes.  Was the
> difference intended?
Fixed in v5, changed to Notes for consistency.
> 
> > +    * - ``atr_sample_rate``
> > +      - runtime
> > +      - Controls how frequently Flow Director ATR updates flow steering
> > +        information for active TCP flows.
> > +
> > +        ATR programs Flow Director entries based on sampled transmitted
> > +        packets. The sampling interval is specified as the number of
> > +        transmitted packets between ATR updates.
> > +
> > +        Lower values increase Flow Director programming activity, while
> > +        higher values reduce the update frequency.
> 
> [Severity: Medium]
> Is "the number of transmitted packets between ATR updates" the unit the
> code actually implements?  Three details in i40e_atr() seem to differ.
> 
> The counter compared against the configured value is per Tx ring, not
> per device:
> 
> drivers/net/ethernet/intel/i40e/i40e_txrx.c:i40e_atr() {
> 	...
> 	tx_ring->atr_count++;
> 
> 	/* sample on all syn/fin/rst packets or once every atr sample rate */
> 	if (!th->fin &&
> 	    !th->syn &&
> 	    !th->rst &&
> 	    (tx_ring->atr_count < sample_rate))
> 		return;
> 	...
> }
> 
> atr_count lives in struct i40e_ring, so on an N-queue device the
> aggregate programming rate is up to N times what a device-wide packet
> interval would suggest.  Given the commit message mentions "systems with
> a large number of queues", should the documentation say the interval is
> per Tx queue?
> 
> The increment is also only reached for TCP after the IP version and
> protocol checks:
> 
> drivers/net/ethernet/intel/i40e/i40e_txrx.c:i40e_atr() {
> 	...
> 	if (l4_proto != IPPROTO_TCP)
> 		return;
> 	...
> }
> 
> so non-TCP transmitted packets never advance the interval.
> 
> Finally, the threshold is bypassed for SYN, FIN and RST, so raising the
> value does not reduce programming activity for connection-churn
> workloads.  Should the text mention that exception?
> 
> > +
> > +        Setting to ``0`` disables ATR sampling (no filters will be programmed)
> > +        The default value is ``20``.
> 

Fixed in v5. The documentation now says "TCP packets per Tx queue" and
clarifies the interval is per queue, not per device.

> [Severity: Low]
> These two lines have no blank line between them and the first one has no
> terminating period, so reStructuredText joins them into a single
> paragraph and renders as "... (no filters will be programmed) The default
> value is 20.".  The max_mac_per_vf entry just above puts a blank line
> before its "The default value is ..." sentence.
> 
> >  
> >  Info versions
> >  =============
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_devlink.c b/drivers/net/ethernet/intel/i40e/i40e_devlink.c
> > index 229179ccc1319..6df7eb217073d 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_devlink.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_devlink.c
> > @@ -33,12 +33,48 @@ static int i40e_max_mac_per_vf_get(struct devlink *devlink,
> >  	return 0;
> >  }
> >  
> > +static int i40e_atr_sample_rate_set(struct devlink *devlink,
> > +				    u32 id,
> > +				    struct devlink_param_gset_ctx *ctx,
> > +				    struct netlink_ext_ack *extack)
> > +{
> > +	struct i40e_pf *pf = devlink_priv(devlink);
> > +	u32 sample_rate = ctx->val.vu32;
> > +
> > +	WRITE_ONCE(pf->atr_sample_rate, sample_rate);
> > +	return 0;
> > +}
> 
> [Severity: Medium]
> Setting the value to 0 is now reachable at runtime while
> I40E_FLAG_FD_ATR_ENA stays set.  Does this leave already programmed ATR
> entries resident in the Flow Director table with no way to remove them?
> 

Fixed in v5. The devlink setter now flushes existing ATR entries when
rate is set to 0, using the same mechanism as the ethtool ATR-disable
path (__I40E_FD_ATR_AUTO_DISABLED + __I40E_FD_FLUSH_REQUESTED).
> In i40e_atr() the zero-rate return happens before the FIN/RST handling
> and before the descriptor build that emits the remove command:
> 
> drivers/net/ethernet/intel/i40e/i40e_txrx.c:i40e_atr() {
> 	...
> 	/* if sampling is disabled do nothing */
> 	sample_rate = READ_ONCE(pf->atr_sample_rate);
> 	if (!sample_rate)
> 		return;
> 	...
> 	dtype_cmd |= (th->fin || th->rst) ?
> 		     (I40E_FILTER_PROGRAM_DESC_PCMD_REMOVE <<
> 		      I40E_TXD_FLTR_QW1_PCMD_SHIFT) :
> 	...
> }
> 
> That PCMD_REMOVE descriptor is the only software removal path, and it is
> only redundant when hardware eviction is available:
> 
> drivers/net/ethernet/intel/i40e/i40e_txrx.c:i40e_atr() {
> 	...
> 	if (test_bit(I40E_FLAG_HW_ATR_EVICT_ENA, pf->flags)) {
> 		/* HW ATR eviction will take care of removing filters on FIN
> 		 * and RST packets.
> 		 */
> 	...
> }
> 
> I40E_FLAG_HW_ATR_EVICT_ENA is only set in i40e_sw_init() when
> pf->hw.caps has I40E_HW_CAP_ATR_EVICT, and it can also be cleared by the
> hw-atr-eviction ethtool private flag.  On such a device, after any TCP
> traffic has programmed ATR filters, a
> 
>   devlink dev param set pci/<bdf> name atr_sample_rate value 0 cmode runtime
> 
> appears to leave those entries steering received packets for flows that
> have since closed, while fd_atr_cnt and i40e_get_current_atr_cnt() still
> report ATR as active.
> 
> The existing way to turn ATR off via the flow-director-atr private flag
> does the teardown explicitly:
> 
> drivers/net/ethernet/intel/i40e/i40e_ethtool.c:i40e_set_priv_flags() {
> 	...
> 	/* Flush current ATR settings if ATR was disabled */
> 	if (test_bit(I40E_FLAG_FD_ATR_ENA, changed_flags) &&
> 	    !test_bit(I40E_FLAG_FD_ATR_ENA, new_flags)) {
> 		set_bit(__I40E_FD_ATR_AUTO_DISABLED, pf->state);
> 		set_bit(__I40E_FD_FLUSH_REQUESTED, pf->state);
> 	}
> 	...
> }
> 
> Should the devlink setter do the same when the new rate is 0, or should
> 0 be rejected by a validate callback instead?  Note the later patch in
> the series that triggers a PF reset when re-enabling ATR via ethtool only
> touches i40e_set_priv_flags(), so it does not add this flush.
> 
> > +
> > +static int i40e_atr_sample_rate_get(struct devlink *devlink,
> 
> [ ... ]
> 
> >  static const struct devlink_param i40e_dl_params[] = {
> 
> [ ... ]
> 
> > +	DEVLINK_PARAM_DRIVER(I40E_DEVLINK_PARAM_ID_ATR_SAMPLE_RATE,
> > +			     "atr_sample_rate",
> > +			     DEVLINK_PARAM_TYPE_U32,
> > +			     BIT(DEVLINK_PARAM_CMODE_RUNTIME),
> > +			     i40e_atr_sample_rate_get,
> > +			     i40e_atr_sample_rate_set,
> > +			     NULL),
> >  };
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917204541.3072415-1-anthony.l.nguyen%40intel.com

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

[Tony Nguyen]

From: Mohammad Heib <mheib@redhat.com>

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.

Trigger a PF reset when ATR is re-enabled so that fd_ena is re-evaluated
based on the current flag state.

Signed-off-by: Mohammad Heib <mheib@redhat.com>
Tested-by: Rinitha S <sx.rinitha@intel.com> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
index ef9d1e78c16a..79c2cdab0f51 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);
+
 	/* Before we finalize any flag changes, we need to perform some
 	 * checks to ensure that the changes are supported and safe.
 	 */
-- 
2.47.1

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

[netdev-bot+sashiko]

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

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

[Mohammad Heib]

On Mon, Sep 21, 2026 at 09:32:23PM +0000, netdev-bot+sashiko@kernel.org wrote:
> 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?
> 

Addressed in v5 by folding this patch into patch 1. The series is now
bisect-clean.
> > 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.
> 

This follows the existing pattern in i40e_set_priv_flags() -- the
VEB_STATS, LEGACY_RX and SOURCE_PRUNING blocks all use plain assignment
as well. Changing only the ATR block to |= would not help because
i40e_do_reset() checks I40E_PF_RESET_FLAG before
I40E_PF_RESET_AND_REBUILD_FLAG in an else-if chain, so the weaker reset
wins regardless of whether both bits are set. Reworking the reset
selection logic is a pre-existing issue outside the scope of this series.

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

Fixed in v5. The LLDP EEXIST path now uses
reset_needed &= ~I40E_PF_RESET_AND_REBUILD_FLAG instead of
reset_needed = 0, so it only cancels the LLDP rebuild without dropping
unrelated reset requests.

> 
> 	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);

This is inherent to the function's design -- bitmap_copy() followed by
i40e_do_reset() is the same pattern used by every other flag change in
i40e_set_priv_flags(). The window is a few instructions wide and matches
the existing code. Addressing it would require restructuring the
function's flag-commit-then-reset model, which is outside the scope of
this series.
> 	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
> 

Fixed in v5. The reset is now skipped when I40E_FLAG_FD_SB_ENA is set,
since i40e_configure_tx_ring() sets fd_ena when either FD flag is active.

> 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