Re: [PATCH net-next 1/3] i40e: move ATR sample rate from ring to PF level

[Mohammad Heib <mheib@redhat.com>]

On Mon, Sep 21, 2026 at 09:32:20PM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 1 · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [High] Concern (discarded on baseline verification): removing the per-
>   ring atr_sample_rate decoupled the software ATR gate from…
> - [Medium] Undisclosed user-visible behaviour change: "i40e: move ATR
>   sample rate from ring to PF level" is written as a pure storage…
> 
> --
> 
> i40e: move ATR sample rate from ring to PF level
> 
> This moves atr_sample_rate out of struct i40e_ring and into struct i40e_pf,
> initializing it once in i40e_sw_init() instead of in i40e_configure_tx_ring().
> i40e_atr() now reads the PF-level value, and ring->atr_count grows from u8 to
> u32.
> 
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> > index abbc71e815ae3..0a4272e1ab070 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> > @@ -3458,12 +3458,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: Medium]
> The commit message describes this as a storage relocation: "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."  Does dropping this conditional also change
> when the writable "flow-director-atr" private flag takes effect?
> 
> Before this patch, the per-ring rate was latched from I40E_FLAG_FD_ATR_ENA at
> the time the Tx ring was configured, and i40e_atr() bailed out on
> !tx_ring->atr_sample_rate.  i40e_set_priv_flags() does not add
> I40E_FLAG_FD_ATR_ENA to reset_needed and only handles the disable direction:
> 
> 	/* Flush current ATR settings if ATR was disabled */
> 	if (test_bit(I40E_FLAG_FD_ATR_ENA, changed_flags) &&
> 	    !test_bit(I40E_FLAG_FD_ATR_ENA, new_flags)) {
> 		set_bit(__I40E_FD_ATR_AUTO_DISABLED, pf->state);
> 		set_bit(__I40E_FD_FLUSH_REQUESTED, pf->state);
> 	}
> 
> So on a live interface whose rings were configured while ATR was off,
> "ethtool --set-priv-flags ethX flow-director-atr on" did not resume sampling
> until an ifdown/ifup or a PF reset.  With the PF-level rate now always
> non-zero, the flag alone re-enables descriptor generation immediately.
> 
> Could the commit message mention this user-visible semantic change, and should
> it carry a Fixes tag if the old latching behaviour was considered a bug?

Yes, this is a real behavior change. The v5 commit message now documents
it explicitly.
> 
> [ ... ]
> 
> > @@ -12740,6 +12735,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);
> 
> [Severity: High]
> This assignment is unconditional and happens before the Flow Director
> capability detection below it.  Can that decouple the software ATR gate from
> the Flow Director enable bit programmed into the Tx queue context?
> 
> i40e_configure_tx_ring() still snapshots the flags into the HMC context:
> 
> 	if (test_bit(I40E_FLAG_FD_SB_ENA, vsi->back->flags) ||
> 	    test_bit(I40E_FLAG_FD_ATR_ENA, vsi->back->flags))
> 		tx_ctx.fd_ena = 1;
> 
> The matching software snapshot (ring->atr_sample_rate) is what this patch
> removes, so after toggling "flow-director-atr" on without a reconfiguration,
> i40e_atr() would pass the new gate:
> 
> 	sample_rate = READ_ONCE(pf->atr_sample_rate);
> 	if (!sample_rate)
> 		return;
> 
> and go on to write an I40E_TX_DESC_DTYPE_FILTER_PROG descriptor and advance
> tx_ring->next_to_use on a queue whose context was programmed with fd_ena = 0.
> Would that produce an MDD event or bogus FD programming status on that queue?
> 
> For completeness, the later patch in this series, "i40e: trigger PF reset when
> re-enabling ATR via ethtool", adds a reset on the enable transition in
> i40e_set_priv_flags(), which re-programs the queue contexts and addresses this
> for the series as a whole.  It does leave a window between
> bitmap_copy(pf->flags, new_flags) and i40e_do_reset(), and it means this
> intermediate commit alone is not bisect-clean in that respect.  Would it make
> sense to fold the reset change into this patch, or to order it first?

Addressed in v5 by folding the PF reset patch (was 3/3) into this
commit. The reset on ATR re-enable now lands in the same commit that
removes the per-ring gate, so the tree is bisect-clean at every commit.

> 
> [ ... ]
> 
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> > index 7f68adb5109b0..e045f9c71c944 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> > @@ -2882,7 +2883,8 @@ 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)
> > +	sample_rate = READ_ONCE(pf->atr_sample_rate);
> > +	if (!sample_rate)
> >  		return;
> >  
> >  	/* Currently only IPv4/IPv6 with TCP is supported */
> 
> [ ... ]
> 
pw-bot: cr
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917204541.3072415-1-anthony.l.nguyen%40intel.com