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

[sashiko-bot@kernel.org]

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 <mheib@redhat.com>

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/20261011134952.54887-1-mheib@redhat.com?part=1