Re: [PATCH iwl-next v6 2/2] i40e: add devlink parameter for Flow Director ATR sample rate

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] i40e: missing state check in devlink atr_sample_rate callback causes continuous flushes
--

commit dbdc3fe26acc90d8b2942b0ba0ea5222fd02c43b
Author: Mohammad Heib <mheib@redhat.com>

i40e: add devlink parameter for Flow Director ATR sample rate

This commit introduces a new devlink parameter, atr_sample_rate, to allow
administrators to tune the update frequency of Flow Director ATR dynamically.
It provides set and get callbacks to apply changes without rebuilding the
driver or disabling ATR functionality entirely.

> diff --git a/drivers/net/ethernet/intel/i40e/i40e_devlink.c b/drivers/net/ethernet/intel/i40e/i40e_devlink.c
> index 229179ccc1319..8af2487776f28 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_devlink.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_devlink.c
[ ... ]
> @@ -33,12 +33,58 @@ static int i40e_max_mac_per_vf_get(struct devlink *devlink,
>  	return 0;
>  }
>  
> +static int i40e_atr_sample_rate_set(struct devlink *devlink,
> +				    u32 id,
> +				    struct devlink_param_gset_ctx *ctx,
> +				    struct netlink_ext_ack *extack)
> +{
> +	struct i40e_pf *pf = devlink_priv(devlink);
> +	u32 sample_rate = ctx->val.vu32;
> +
> +	WRITE_ONCE(pf->atr_sample_rate, sample_rate);
> +
> +	/* Setting rate to 0 disables sampling but does not remove
> +	 * existing ATR entries. Flush them so stale filters do not
> +	 * persist on devices without HW ATR eviction.
> +	 */
> +	if (!sample_rate) {
> +		set_bit(__I40E_FD_ATR_AUTO_DISABLED, pf->state);
> +		set_bit(__I40E_FD_FLUSH_REQUESTED, pf->state);
> +	}

[Severity: Low]
Will this trigger unnecessary hardware flushes if a user or configuration
tool repeatedly sets the atr_sample_rate to 0 when it is already 0?

The devlink core in net/devlink/param.c:__devlink_nl_cmd_param_set_doit()
does not filter out unchanged values before invoking the driver's set
callback.

As a result, i40e_atr_sample_rate_set() unconditionally sets the
__I40E_FD_FLUSH_REQUESTED bit in pf->state, which then triggers a flush in
the background service task.

Would it be better to check if the new sample_rate differs from the
existing value before applying the change and requesting a flush?

> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011134952.54887-1-mheib@redhat.com?part=2