Re: [PATCH net-next v7 3/7] net: stmmac: refactor FPE verification process

Vladimir Oltean
2024-09-04T14:04:54Z

On Wed, Sep 04, 2024 at 05:21:18PM +0800, Furong Xu wrote:
> +/**
> + * stmmac_fpe_verify_timer - Timer for MAC Merge verification
> + * @t: timer_list struct containing private info
> + *
> + * Verify the MAC Merge capability in the local TX direction, by
> + * transmitting Verify mPackets up to 3 times. Wait until link
> + * partner responds with a Response mPacket, otherwise fail.
> + */
> +static void stmmac_fpe_verify_timer(struct timer_list *t)
> {
> - struct stmmac_priv *priv = container_of(work, struct stmmac_priv,
> - fpe_task);
> - struct stmmac_fpe_cfg *fpe_cfg = &priv->fpe_cfg;
> - enum stmmac_fpe_state *lo_state = &fpe_cfg->lo_fpe_state;
> - enum stmmac_fpe_state *lp_state = &fpe_cfg->lp_fpe_state;
> - bool *hs_enable = &fpe_cfg->hs_enable;
> - bool *enable = &fpe_cfg->enable;
> - int retries = 20;
> -
> - while (retries-- > 0) {
> - /* Bail out immediately if FPE handshake is OFF */
> - if (*lo_state == FPE_STATE_OFF || !*hs_enable)
> - break;
> -
> - if (*lo_state == FPE_STATE_ENTERING_ON &&
> - *lp_state == FPE_STATE_ENTERING_ON) {
> - stmmac_fpe_configure(priv, priv->ioaddr,
> - fpe_cfg,
> - priv->plat->tx_queues_to_use,
> - priv->plat->rx_queues_to_use,
> - *enable);
> -
> - netdev_info(priv->dev, "configured FPE\n");
> + struct stmmac_fpe_cfg *fpe_cfg = from_timer(fpe_cfg, t, verify_timer);
> + struct stmmac_priv *priv = container_of(fpe_cfg, struct stmmac_priv,
> + fpe_cfg);
> + struct ethtool_mm_state *state = &fpe_cfg->state;
> + unsigned long flags;
> + bool rearm = false;
>
> - *lo_state = FPE_STATE_ON;
> - *lp_state = FPE_STATE_ON;
> - netdev_info(priv->dev, "!!! BOTH FPE stations ON\n");
> - break;
> - }
> + spin_lock_irqsave(&fpe_cfg->lock, flags);
>
> - if ((*lo_state == FPE_STATE_CAPABLE ||
> - *lo_state == FPE_STATE_ENTERING_ON) &&
> - *lp_state != FPE_STATE_ON) {
> - netdev_info(priv->dev, SEND_VERIFY_MPAKCET_FMT,
> - *lo_state, *lp_state);
> + switch (state->verify_status) {
> + case ETHTOOL_MM_VERIFY_STATUS_INITIAL:
> + case ETHTOOL_MM_VERIFY_STATUS_VERIFYING:
> + if (fpe_cfg->verify_retries != 0) {
> stmmac_fpe_send_mpacket(priv, priv->ioaddr,
> - fpe_cfg,
> - MPACKET_VERIFY);
> + fpe_cfg, MPACKET_VERIFY);
> + rearm = true;
> + } else {
> + state->verify_status = ETHTOOL_MM_VERIFY_STATUS_FAILED;
> }
> - /* Sleep then retry */
> - msleep(500);
> +
> + fpe_cfg->verify_retries--;
> + break;

Odd indentation... "break;" should be on the same level with the code,
not with the "case" statements. Not sure which editor you use, but even
if you hit "==" in vim on this line, it will shift it by one tab to the
right.

> +
> + case ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED:
> + stmmac_fpe_configure(priv, priv->ioaddr, fpe_cfg,
> + priv->plat->tx_queues_to_use,
> + priv->plat->rx_queues_to_use,
> + true, true);
> + break;

Same comment here and below.

> +
> + default:
> + break;
> + }
> +
> + if (rearm) {
> + mod_timer(&fpe_cfg->verify_timer,
> + jiffies + msecs_to_jiffies(state->verify_time));
> }
>
> - clear_bit(__FPE_TASK_SCHED, &priv->fpe_task_state);
> + spin_unlock_irqrestore(&fpe_cfg->lock, flags);
> +}