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

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.