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.
