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;
> }
