Re: [PATCH iwl-next v2 1/2] i40e: move ATR sample rate from ring to PF level

[Simon Horman <horms@kernel.org>]

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.