Netdev List
 help / color / mirror / Atom feed
* [PATCH iwl-next v2 1/2] i40e: move ATR sample rate from ring to PF level
@ 2026-07-01  9:38 mheib
  2026-07-01  9:38 ` [PATCH iwl-next v3 2/2] i40e: add devlink parameter for Flow Director ATR sample rate mheib
  2026-07-08 14:36 ` [PATCH iwl-next v2 1/2] i40e: move ATR sample rate from ring to PF level Simon Horman
  0 siblings, 2 replies; 6+ messages in thread
From: mheib @ 2026-07-01  9:38 UTC (permalink / raw)
  To: intel-wired-lan
  Cc: netdev, jiri, davem, edumazet, kuba, pabeni, horms, corbet,
	anthony.l.nguyen, przemyslaw.kitszel, andrew+netdev,
	Mohammad Heib

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>
---
 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 | 4 ++--
 drivers/net/ethernet/intel/i40e/i40e_txrx.h | 3 +--
 4 files changed, 7 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 1fb86bd1af8e..3834af6c09be 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -3457,12 +3457,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);
@@ -12745,6 +12740,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 61525ab7d21e..da94cb2ce94d 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
@@ -2882,7 +2882,7 @@ 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)
+	if (!pf->atr_sample_rate)
 		return;
 
 	/* Currently only IPv4/IPv6 with TCP is supported */
@@ -2934,7 +2934,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 < pf->atr_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.53.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH iwl-next v3 2/2] i40e: add devlink parameter for Flow Director ATR sample rate
  2026-07-01  9:38 [PATCH iwl-next v2 1/2] i40e: move ATR sample rate from ring to PF level mheib
@ 2026-07-01  9:38 ` mheib
  2026-07-08 14:36   ` Simon Horman
  2026-07-08 14:36 ` [PATCH iwl-next v2 1/2] i40e: move ATR sample rate from ring to PF level Simon Horman
  1 sibling, 1 reply; 6+ messages in thread
From: mheib @ 2026-07-01  9:38 UTC (permalink / raw)
  To: intel-wired-lan
  Cc: netdev, jiri, davem, edumazet, kuba, pabeni, horms, corbet,
	anthony.l.nguyen, przemyslaw.kitszel, andrew+netdev,
	Mohammad Heib

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>
---
 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..cf487efdd803 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;
+
+	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 = 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.53.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH iwl-next v2 1/2] i40e: move ATR sample rate from ring to PF level
  2026-07-01  9:38 [PATCH iwl-next v2 1/2] i40e: move ATR sample rate from ring to PF level mheib
  2026-07-01  9:38 ` [PATCH iwl-next v3 2/2] i40e: add devlink parameter for Flow Director ATR sample rate mheib
@ 2026-07-08 14:36 ` Simon Horman
  2026-07-20 11:36   ` mohammad heib
  1 sibling, 1 reply; 6+ messages in thread
From: Simon Horman @ 2026-07-08 14:36 UTC (permalink / raw)
  To: mheib
  Cc: Simon Horman, intel-wired-lan, netdev, jiri, davem, edumazet,
	kuba, pabeni, corbet, anthony.l.nguyen, przemyslaw.kitszel,
	andrew+netdev

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
i40e: move ATR sample rate from ring to PF level

This commit moves the ATR sample rate configuration from the per-ring level
to the PF level. It initializes the sample rate globally during software init
and updates the fast path to reference the PF structure.

> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index a04683004a567..4d4d4b07a0f3a 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
[ ... ]
> @@ -3454,12 +3454,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: High]
Does removing this per-ring initialization remove the protection against
dynamic ethtool flag toggles?

Previously, if the ring was configured while Flow Director ATR was disabled,
the ring's sample rate was cached to 0 in i40e_configure_tx_ring(). This
prevented descriptor injection even if the PF flag was later toggled on
without a ring reset.

[ ... ]
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> index 894f2d06d39d0..b36e0fc9b31d2 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
[ ... ]
> @@ -2879,7 +2879,7 @@ 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)
> +	if (!pf->atr_sample_rate)
>  		return;

[Severity: High]
Can this global check lead to a hardware Malicious Driver Detection (MDD)
event if ethtool flags are modified dynamically?

If an administrator performs the following sequence:

1. Disables flow-director-atr via ethtool.
2. Disables ntuple (which resets the ring and sets tx_ctx.fd_ena = 0).
3. Re-enables flow-director-atr.

The final step does not trigger a ring reset, so fd_ena remains 0 in the
hardware queue context.

However, I40E_FLAG_FD_ATR_ENA is now true, and pf->atr_sample_rate is
globally set to a non-zero value.

Will i40e_atr() now proceed and inject FDIR descriptors into a TX queue
that is not configured for FDIR?

If so, does this cause the hardware to trigger an MDD event and hang the
TX queue?

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH iwl-next v3 2/2] i40e: add devlink parameter for Flow Director ATR sample rate
  2026-07-01  9:38 ` [PATCH iwl-next v3 2/2] i40e: add devlink parameter for Flow Director ATR sample rate mheib
@ 2026-07-08 14:36   ` Simon Horman
  0 siblings, 0 replies; 6+ messages in thread
From: Simon Horman @ 2026-07-08 14:36 UTC (permalink / raw)
  To: mheib
  Cc: Simon Horman, intel-wired-lan, netdev, jiri, davem, edumazet,
	kuba, pabeni, corbet, anthony.l.nguyen, przemyslaw.kitszel,
	andrew+netdev

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
i40e: add devlink parameter for Flow Director ATR sample rate

This commit introduces a runtime devlink parameter to tune the Flow Director
ATR sample rate. This allows dynamic adjustments to the sampling interval
without rebuilding the driver, which can help reduce TCP packet reordering
on systems with high-rate workloads.

> diff --git a/drivers/net/ethernet/intel/i40e/i40e_devlink.c b/drivers/net/ethernet/intel/i40e/i40e_devlink.c
> index 229179ccc1319..cf487efdd803d 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;
> +
> +	pf->atr_sample_rate = sample_rate;

[Severity: Low]
Does modifying pf->atr_sample_rate concurrently with the Tx fast path
introduce a data race?

In the Tx fast path, i40e_atr() reads this value locklessly. If the devlink
callback sets this to 0 between the two reads in i40e_atr():

drivers/net/ethernet/intel/i40e/i40e_txrx.c:i40e_atr() {
        if (!pf->atr_sample_rate)
                return;
        ...
        if (tx_ring->atr_count < pf->atr_sample_rate)
        ...
}

Could this race lead to a situation where the second check evaluates
tx_ring->atr_count against 0, unintentionally programming an extra Flow
Director filter?

While the functional impact might just be a single extra sampled packet,
should we use READ_ONCE() in the fast path and WRITE_ONCE() here to avoid
KCSAN data race warnings and ensure a consistent value is used throughout
the function?

> +	return 0;
> +}

[ ... ]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH iwl-next v2 1/2] i40e: move ATR sample rate from ring to PF level
  2026-07-08 14:36 ` [PATCH iwl-next v2 1/2] i40e: move ATR sample rate from ring to PF level Simon Horman
@ 2026-07-20 11:36   ` mohammad heib
  2026-07-21 12:44     ` Simon Horman
  0 siblings, 1 reply; 6+ messages in thread
From: mohammad heib @ 2026-07-20 11:36 UTC (permalink / raw)
  To: Simon Horman
  Cc: intel-wired-lan, netdev, jiri, davem, edumazet, kuba, pabeni,
	corbet, anthony.l.nguyen, przemyslaw.kitszel, andrew+netdev

Hi Simon
Thank you for the review.

On 7/8/26 5:36 PM, Simon Horman wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://sashiko.dev
> ---
> i40e: move ATR sample rate from ring to PF level
> 
> This commit moves the ATR sample rate configuration from the per-ring level
> to the PF level. It initializes the sample rate globally during software init
> and updates the fast path to reference the PF structure.
> 
>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
>> index a04683004a567..4d4d4b07a0f3a 100644
>> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
>> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> [ ... ]
>> @@ -3454,12 +3454,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: High]
> Does removing this per-ring initialization remove the protection against
> dynamic ethtool flag toggles?
> 
> Previously, if the ring was configured while Flow Director ATR was disabled,
> the ring's sample rate was cached to 0 in i40e_configure_tx_ring(). This
> prevented descriptor injection even if the PF flag was later toggled on
> without a ring reset.
> 

The per-ring zeroing of atr_sample_rate was redundant. i40e_atr() 
already checks I40E_FLAG_FD_ATR_ENA before it ever looks at atr_sample_rate:

  /* 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;

   /* if sampling is disabled do nothing */
   if (!pf->atr_sample_rate)
       return;


When ATR is disabled via ethtool private flags, the function bails out 
at the I40E_FLAG_FD_ATR_ENA check regardless of what atr_sample_rate is 
set to. The flag check is the real protection the per-ring sample rate 
cache was never the thing providing protection against dynamic toggling.


> [ ... ]
>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
>> index 894f2d06d39d0..b36e0fc9b31d2 100644
>> --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
>> +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> [ ... ]
>> @@ -2879,7 +2879,7 @@ 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)
>> +	if (!pf->atr_sample_rate)
>>   		return;
> 
> [Severity: High]
> Can this global check lead to a hardware Malicious Driver Detection (MDD)
> event if ethtool flags are modified dynamically?
> 
> If an administrator performs the following sequence:
> 
> 1. Disables flow-director-atr via ethtool.
> 2. Disables ntuple (which resets the ring and sets tx_ctx.fd_ena = 0).
> 3. Re-enables flow-director-atr.
> 
> The final step does not trigger a ring reset, so fd_ena remains 0 in the
> hardware queue context.
> 
> However, I40E_FLAG_FD_ATR_ENA is now true, and pf->atr_sample_rate is
> globally set to a non-zero value.
> 
> Will i40e_atr() now proceed and inject FDIR descriptors into a TX queue
> that is not configured for FDIR?
> 
> If so, does this cause the hardware to trigger an MDD event and hang the
> TX queue?
> 
The scenario you described was already broken before this patch, walking 
through the old code with the same sequence:

   1. Disable ATR — flag cleared, __I40E_FD_ATR_AUTO_DISABLED set
   2. Disable ntuple — ring reset happens, i40e_configure_tx_ring() runs 
with ATR off, so ring->atr_sample_rate = 0 and fd_ena = 0
   3. Re-enable ATR — flag set, no ring reset

In the old code, ring->atr_sample_rate is stuck at 0 from step 2, so 
i40e_atr() bails out at the sample rate check. That avoids the fd_ena 
problem, but ATR is also silently non-functional
   — the user re-enabled it but it doesn't actually work until something 
triggers a ring reset.

This patch changes how that failure looks, instead of silently doing 
nothing, pf->atr_sample_rate is non-zero so i40e_atr() would proceed 
but the root cause is the same:
toggling ATR via ethtool private flags doesn't trigger a ring reset, so 
fd_ena can be stale.

Properly fixing this would mean triggering a reset when ATR is 
re-enabled. The reset calls i40e_configure_tx_ring(), which re-evaluates 
fd_ena based on the current flag state so fd_ena would be set to 1 since 
I40E_FLAG_FD_ATR_ENA is now on.

What do you think about addressing this as a follow-up patch on top of 
this series? Since it's a pre-existing issue, it feels like it belongs 
as a separate fix rather than being mixed into this refactor.



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH iwl-next v2 1/2] i40e: move ATR sample rate from ring to PF level
  2026-07-20 11:36   ` mohammad heib
@ 2026-07-21 12:44     ` Simon Horman
  0 siblings, 0 replies; 6+ messages in thread
From: Simon Horman @ 2026-07-21 12:44 UTC (permalink / raw)
  To: mohammad heib
  Cc: intel-wired-lan, netdev, jiri, davem, edumazet, kuba, pabeni,
	corbet, anthony.l.nguyen, przemyslaw.kitszel, andrew+netdev

Hi Mohammad,

Likewise, thanks for your response.

On Mon, Jul 20, 2026 at 02:36:52PM +0300, mohammad heib wrote:
> Hi Simon
> Thank you for the review.
> 
> On 7/8/26 5:36 PM, Simon Horman wrote:
> > This is an AI-generated review of your patch. The human sending this
> > email has considered the AI review valid, or at least plausible.
> > Full review at: https://sashiko.dev
> > ---
> > i40e: move ATR sample rate from ring to PF level
> > 
> > This commit moves the ATR sample rate configuration from the per-ring level
> > to the PF level. It initializes the sample rate globally during software init
> > and updates the fast path to reference the PF structure.
> > 
> > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> > > index a04683004a567..4d4d4b07a0f3a 100644
> > > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> > > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> > [ ... ]
> > > @@ -3454,12 +3454,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: High]
> > Does removing this per-ring initialization remove the protection against
> > dynamic ethtool flag toggles?
> > 
> > Previously, if the ring was configured while Flow Director ATR was disabled,
> > the ring's sample rate was cached to 0 in i40e_configure_tx_ring(). This
> > prevented descriptor injection even if the PF flag was later toggled on
> > without a ring reset.
> > 
> 
> The per-ring zeroing of atr_sample_rate was redundant. i40e_atr() already
> checks I40E_FLAG_FD_ATR_ENA before it ever looks at atr_sample_rate:
> 
>  /* 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;
> 
>   /* if sampling is disabled do nothing */
>   if (!pf->atr_sample_rate)
>       return;
> 
> 
> When ATR is disabled via ethtool private flags, the function bails out at
> the I40E_FLAG_FD_ATR_ENA check regardless of what atr_sample_rate is set to.
> The flag check is the real protection the per-ring sample rate cache was
> never the thing providing protection against dynamic toggling.

Thanks for the response and sorry for the false-positive.
I agree we can discard this concern.

> > [ ... ]
> > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> > > index 894f2d06d39d0..b36e0fc9b31d2 100644
> > > --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> > > +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> > [ ... ]
> > > @@ -2879,7 +2879,7 @@ 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)
> > > +	if (!pf->atr_sample_rate)
> > >   		return;
> > 
> > [Severity: High]
> > Can this global check lead to a hardware Malicious Driver Detection (MDD)
> > event if ethtool flags are modified dynamically?
> > 
> > If an administrator performs the following sequence:
> > 
> > 1. Disables flow-director-atr via ethtool.
> > 2. Disables ntuple (which resets the ring and sets tx_ctx.fd_ena = 0).
> > 3. Re-enables flow-director-atr.
> > 
> > The final step does not trigger a ring reset, so fd_ena remains 0 in the
> > hardware queue context.
> > 
> > However, I40E_FLAG_FD_ATR_ENA is now true, and pf->atr_sample_rate is
> > globally set to a non-zero value.
> > 
> > Will i40e_atr() now proceed and inject FDIR descriptors into a TX queue
> > that is not configured for FDIR?
> > 
> > If so, does this cause the hardware to trigger an MDD event and hang the
> > TX queue?
> > 
> The scenario you described was already broken before this patch, walking
> through the old code with the same sequence:
> 
>   1. Disable ATR — flag cleared, __I40E_FD_ATR_AUTO_DISABLED set
>   2. Disable ntuple — ring reset happens, i40e_configure_tx_ring() runs with
> ATR off, so ring->atr_sample_rate = 0 and fd_ena = 0
>   3. Re-enable ATR — flag set, no ring reset
> 
> In the old code, ring->atr_sample_rate is stuck at 0 from step 2, so
> i40e_atr() bails out at the sample rate check. That avoids the fd_ena
> problem, but ATR is also silently non-functional
>   — the user re-enabled it but it doesn't actually work until something
> triggers a ring reset.
> 
> This patch changes how that failure looks, instead of silently doing
> nothing, pf->atr_sample_rate is non-zero so i40e_atr() would proceed but the
> root cause is the same:
> toggling ATR via ethtool private flags doesn't trigger a ring reset, so
> fd_ena can be stale.
> 
> Properly fixing this would mean triggering a reset when ATR is re-enabled.
> The reset calls i40e_configure_tx_ring(), which re-evaluates fd_ena based on
> the current flag state so fd_ena would be set to 1 since
> I40E_FLAG_FD_ATR_ENA is now on.
> 
> What do you think about addressing this as a follow-up patch on top of this
> series? Since it's a pre-existing issue, it feels like it belongs as a
> separate fix rather than being mixed into this refactor.

Thanks for the detailed analysis, much appreciated.
I agree we can leave this to be addressed by a follow-up.


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2026-07-21 12:44 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-01  9:38 [PATCH iwl-next v2 1/2] i40e: move ATR sample rate from ring to PF level mheib
2026-07-01  9:38 ` [PATCH iwl-next v3 2/2] i40e: add devlink parameter for Flow Director ATR sample rate mheib
2026-07-08 14:36   ` Simon Horman
2026-07-08 14:36 ` [PATCH iwl-next v2 1/2] i40e: move ATR sample rate from ring to PF level Simon Horman
2026-07-20 11:36   ` mohammad heib
2026-07-21 12:44     ` Simon Horman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox