On Mon, Oct 12, 2026 at 01:50:32PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s)
> to consider:
> - [Medium] i40e: combined reset flags downgrade reset level to weaker PF_RESET
> --
>
> commit 6e65935fd380dc5da870cee78bd93449fc30b74c
> Author: Mohammad Heib <[email protected]>
>
> i40e: move ATR sample rate from ring to PF level
>
> This commit moves the ATR sample rate from the per-ring struct to the
> PF struct, initializing it once during software initialization. It also
> triggers a PF reset when re-enabling ATR so that the TX queue HW context
> is appropriately reprogrammed.
>
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> > b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> > index 0ae50d7f34651..bc9b4a2bc23b2 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> [ ... ]
> > if (test_bit(I40E_FLAG_FW_LLDP_DIS, changed_flags))
> > reset_needed = I40E_PF_RESET_AND_REBUILD_FLAG;
> >
> > if (test_bit(I40E_FLAG_VEB_STATS_ENA, changed_flags) ||
> > test_bit(I40E_FLAG_LEGACY_RX_ENA, changed_flags) ||
> > test_bit(I40E_FLAG_SOURCE_PRUNING_DIS, changed_flags))
> > reset_needed = BIT(__I40E_PF_RESET_REQUESTED);
>
> [Severity: Medium]
> This problem wasn't introduced by this patch, but does this unconditionally
> overwrite reset_needed? If I40E_FLAG_FW_LLDP_DIS is also in changed_flags,
> it looks like the I40E_PF_RESET_AND_REBUILD_FLAG will be silently dropped.
>
> >
Right, as you noted this is pre-existing — the plain '=' assignments
above already clobber earlier reset requests regardless of this patch.
> > + /* Re-enabling ATR requires a reset to reprogram fd_ena in the
> > + * TX queue HW context. Skip when FD_SB (ntuple) is already
> > + * enabled, since i40e_configure_tx_ring() sets fd_ena when
> > + * either FD flag is active.
> > + */
> > + if (test_bit(I40E_FLAG_FD_ATR_ENA, changed_flags) &&
> > + test_bit(I40E_FLAG_FD_ATR_ENA, new_flags) &&
> > + !test_bit(I40E_FLAG_FD_SB_ENA, new_flags))
> > + reset_needed |= BIT(__I40E_PF_RESET_REQUESTED);
>
> [Severity: Medium]
> If changed_flags contains multiple flags (for instance FW_LLDP_DIS and
> FD_ATR_ENA simultaneously via ethtool), this bitwise OR operation can cause
> reset_needed to contain both I40E_PF_RESET_AND_REBUILD_FLAG and
> BIT(__I40E_PF_RESET_REQUESTED).
>
> When both flags are set, does this cause the weaker reset to be executed?
> Looking at i40e_do_reset() in i40e_main.c, there is a cascade that checks
> the weaker flag before I40E_PF_RESET_AND_REBUILD_FLAG:
>
> } else if (reset_flags & I40E_PF_RESET_FLAG) {
> ...
> dev_dbg(&pf->pdev->dev, "PFR requested\n");
> i40e_handle_reset_warning(pf, lock_acquired);
>
> } else if (reset_flags & I40E_PF_RESET_AND_REBUILD_FLAG) {
>
> This means the requested rebuild from I40E_PF_RESET_AND_REBUILD_FLAG would
> be skipped.
>
> Also, looking at i40e_handle_reset_warning(), it appears to synchronously call
> i40e_reset_and_rebuild() with reinit set to false, instead of deferring it via
> __I40E_RESET_INTR_RECEIVED:
>
> static void i40e_handle_reset_warning(struct i40e_pf *pf, bool lock_acquired)
> {
> i40e_prep_for_reset(pf);
> i40e_reset_and_rebuild(pf, false, lock_acquired);
> }
>
> Can this lead to the VSI reinitialization being incorrectly skipped when
> both flags are present?
>
Good observation, but in practice this cannot happen with the current
code: the '=' assignment for the LLDP path already overwrites
reset_needed before our '|=' runs, so the REBUILD flag is never
present when we OR in the PF reset bit.
That said, I agree the reset_needed handling and the priority ordering
in i40e_do_reset() could use cleanup. I'll look into addressing both
in a separate patch.
> --
> Sashiko AI review ·
> https://sashiko.dev/#/patchset/[email protected]?part=1
