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.
