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. > > + /* 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? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
