Re: [PATCH iwl-next v7 1/9] net: ethtool: mm: extract stmmac verification logic into common library

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Mon, Mar 03, 2025 at 05:26:50AM -0500, Faizal Rahim wrote:
> +/**
> + * ethtool_mmsv_link_state_handle() - Inform MAC Merge Software Verification
> + *				      of link state changes
> + * @mmsv: MAC Merge Software Verification state
> + * @up: True if device carrier is up and able to pass verification packets
> + *
> + * Calling context is expected to be from a task, interrupts enabled.
> + */
> +void ethtool_mmsv_link_state_handle(struct ethtool_mmsv *mmsv, bool up)
> +{
> +	unsigned long flags;
> +
> +	ethtool_mmsv_stop(mmsv);
> +
> +	spin_lock_irqsave(&mmsv->lock, flags);
> +
> +	if (up && mmsv->pmac_enabled) {
> +		/* VERIFY process requires pMAC enabled when NIC comes up */
> +		ethtool_mmsv_configure_pmac(mmsv, true);
> +
> +		/* New link => maybe new partner => new verification process */
> +		ethtool_mmsv_apply(mmsv);
> +	} else {
> +		/* Reset the reported verification state while the link is down */
> +		if (mmsv->verify_enabled)
> +			mmsv->status = ETHTOOL_MM_VERIFY_STATUS_INITIAL;

As requested in v5, please make this a separate patch. It represents a
functional change for stmmac, and I don't want somebody who bisects it
to find a code movement commit and search for a needle through a haystack.

> +
> +		/* No link or pMAC not enabled */
> +		ethtool_mmsv_configure_pmac(mmsv, false);
> +		ethtool_mmsv_configure_tx(mmsv, false);
> +	}
> +
> +	spin_unlock_irqrestore(&mmsv->lock, flags);
> +}
> +EXPORT_SYMBOL_GPL(ethtool_mmsv_link_state_handle);
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> index 918a32f8fda8..25533d6a3175 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> @@ -1210,37 +1210,17 @@ static int stmmac_get_mm(struct net_device *ndev,
>  			 struct ethtool_mm_state *state)
>  {
>  	struct stmmac_priv *priv = netdev_priv(ndev);
> -	unsigned long flags;
>  	u32 frag_size;
>  
>  	if (!stmmac_fpe_supported(priv))
>  		return -EOPNOTSUPP;
>  
> -	spin_lock_irqsave(&priv->fpe_cfg.lock, flags);
> +	ethtool_mmsv_get_mm(&priv->fpe_cfg.mmsv, state);
>  
> -	state->max_verify_time = STMMAC_FPE_MM_MAX_VERIFY_TIME_MS;
> -	state->verify_enabled = priv->fpe_cfg.verify_enabled;
> -	state->pmac_enabled = priv->fpe_cfg.pmac_enabled;
> -	state->verify_time = priv->fpe_cfg.verify_time;
> -	state->tx_enabled = priv->fpe_cfg.tx_enabled;
> -	state->verify_status = priv->fpe_cfg.status;
>  	state->rx_min_frag_size = ETH_ZLEN;
> -
> -	/* FPE active if common tx_enabled and
> -	 * (verification success or disabled(forced))
> -	 */
> -	if (state->tx_enabled &&
> -	    (state->verify_status == ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED ||
> -	     state->verify_status == ETHTOOL_MM_VERIFY_STATUS_DISABLED))
> -		state->tx_active = true;
> -	else
> -		state->tx_active = false;
> -
>  	frag_size = stmmac_fpe_get_add_frag_size(priv);
>  	state->tx_min_frag_size = ethtool_mm_frag_size_add_to_min(frag_size);
>  
> -	spin_unlock_irqrestore(&priv->fpe_cfg.lock, flags);
> -
>  	return 0;
>  }
>  
> @@ -1248,8 +1228,6 @@ static int stmmac_set_mm(struct net_device *ndev, struct ethtool_mm_cfg *cfg,
>  			 struct netlink_ext_ack *extack)
>  {
>  	struct stmmac_priv *priv = netdev_priv(ndev);
> -	struct stmmac_fpe_cfg *fpe_cfg = &priv->fpe_cfg;
> -	unsigned long flags;
>  	u32 frag_size;
>  	int err;
>  
> @@ -1258,23 +1236,8 @@ static int stmmac_set_mm(struct net_device *ndev, struct ethtool_mm_cfg *cfg,
>  	if (err)
>  		return err;
>  
> -	/* Wait for the verification that's currently in progress to finish */
> -	timer_shutdown_sync(&fpe_cfg->verify_timer);
> -
> -	spin_lock_irqsave(&fpe_cfg->lock, flags);
> -
> -	fpe_cfg->verify_enabled = cfg->verify_enabled;
> -	fpe_cfg->pmac_enabled = cfg->pmac_enabled;
> -	fpe_cfg->verify_time = cfg->verify_time;
> -	fpe_cfg->tx_enabled = cfg->tx_enabled;
> -
> -	if (!cfg->verify_enabled)
> -		fpe_cfg->status = ETHTOOL_MM_VERIFY_STATUS_DISABLED;
> -
>  	stmmac_fpe_set_add_frag_size(priv, frag_size);

This is another change in behavior which unfortunately I am noticing
only now: stmmac_fpe_set_add_frag_size() and stmmac_fpe_get_add_frag_size()
used to execute under &fpe_cfg->lock, and now run outside of what
eventually became the mmsv->lock (but still under rtnl_lock() protection).

I am not seeing a need for the additional fragment size to be covered
by the mmsv->lock (the mmsv has no business with this parameter), and
rtnl_lock() should be sufficient to serialize stmmac_get_mm() with
stmmac_set_mm(). So I guess I am ok with the code structure, but again,
please move stmmac_fpe_set_add_frag_size() and stmmac_fpe_get_add_frag_size()
outside of the spin_lock_irqsave(&fpe_cfg->lock) context (essentially to
their current positions) as a preliminary separate patch. Again, it is
best for a "git blame" caused by a behavior change here to not point to
a commit that moves code from one file to another.

> -	stmmac_fpe_apply(priv);
> -
> -	spin_unlock_irqrestore(&fpe_cfg->lock, flags);
> +	ethtool_mmsv_set_mm(&priv->fpe_cfg.mmsv, cfg);
>  
>  	return 0;
>  }