Re: [PATCH net-next v4 3/7] net: stmmac: refactor FPE verification process
From: Alexander Lobakin <aleksander.lobakin@intel.com>
Date: 2024-08-20 14:36:38
Also in:
lkml, netdev
From: Furong Xu <redacted> Date: Tue, 20 Aug 2024 17:38:31 +0800
quoted hunk ↗ jump to hunk
Drop driver defined stmmac_fpe_state, and switch to common ethtool_mm_verify_status for local TX verification status. Local side and remote side verification processes are completely independent. There is no reason at all to keep a local state and a remote state. Add a spinlock to avoid races among ISR, workqueue, link update and register configuration. Signed-off-by: Furong Xu <redacted> --- drivers/net/ethernet/stmicro/stmmac/stmmac.h | 21 +-- .../net/ethernet/stmicro/stmmac/stmmac_main.c | 172 ++++++++++-------- .../net/ethernet/stmicro/stmmac/stmmac_tc.c | 6 - 3 files changed, 102 insertions(+), 97 deletions(-)diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h index 458d6b16ce21..407b59f2783f 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h@@ -146,14 +146,6 @@ struct stmmac_channel { u32 index; }; -/* FPE link state */ -enum stmmac_fpe_state { - FPE_STATE_OFF = 0, - FPE_STATE_CAPABLE = 1, - FPE_STATE_ENTERING_ON = 2, - FPE_STATE_ON = 3, -}; - /* FPE link-partner hand-shaking mPacket type */ enum stmmac_mpacket_type { MPACKET_VERIFY = 0,@@ -166,11 +158,16 @@ enum stmmac_fpe_task_state_t { }; struct stmmac_fpe_cfg { - bool enable; /* FPE enable */ - bool hs_enable; /* FPE handshake enable */ - enum stmmac_fpe_state lp_fpe_state; /* Link Partner FPE state */ - enum stmmac_fpe_state lo_fpe_state; /* Local station FPE state */ + /* Serialize access to MAC Merge state between ethtool requests + * and link state updates. + */ + spinlock_t lock; + u32 fpe_csr; /* MAC_FPE_CTRL_STS reg cache */ + u32 verify_time; /* see ethtool_mm_state */ + bool pmac_enabled; /* see ethtool_mm_state */ + bool verify_enabled; /* see ethtool_mm_state */ + enum ethtool_mm_verify_status status;
Why not embed ðtool_mm_state here then?
quoted hunk ↗ jump to hunk
}; struct stmmac_tc_entry {diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 3072ad33b105..6ae95f20b24f 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c@@ -969,17 +969,21 @@ static void stmmac_mac_config(struct phylink_config *config, unsigned int mode, static void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up) { 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; + unsigned long flags; + + spin_lock_irqsave(&priv->fpe_cfg.lock, flags); + + if (!fpe_cfg->pmac_enabled) + goto __unlock_out; - if (is_up && *hs_enable) { + if (is_up && fpe_cfg->verify_enabled) stmmac_fpe_send_mpacket(priv, priv->ioaddr, fpe_cfg, MPACKET_VERIFY); - } else { - *lo_state = FPE_STATE_OFF; - *lp_state = FPE_STATE_OFF; - } + else + fpe_cfg->status = ETHTOOL_MM_VERIFY_STATUS_DISABLED; + +__unlock_out:
Why underscores?
quoted hunk ↗ jump to hunk
+ spin_unlock_irqrestore(&priv->fpe_cfg.lock, flags); } static void stmmac_mac_link_down(struct phylink_config *config,@@ -4091,11 +4095,25 @@ static int stmmac_release(struct net_device *dev) stmmac_release_ptp(priv); - pm_runtime_put(priv->device); - - if (priv->dma_cap.fpesel) + if (priv->dma_cap.fpesel) { stmmac_fpe_stop_wq(priv); + /* stmmac_ethtool_ops.begin() guarantees that all ethtool + * requests to fail with EBUSY when !netif_running() + * + * Prepare some params here, then fpe_cfg can keep consistent + * with the register states after a SW reset by __stmmac_open(). + */ + priv->fpe_cfg.pmac_enabled = false; + priv->fpe_cfg.verify_enabled = false; + priv->fpe_cfg.status = ETHTOOL_MM_VERIFY_STATUS_DISABLED; + + /* Reset MAC_FPE_CTRL_STS reg cache */ + priv->fpe_cfg.fpe_csr = 0; + } + + pm_runtime_put(priv->device); + return 0; }@@ -5979,44 +5997,34 @@ static int stmmac_set_features(struct net_device *netdev, static void stmmac_fpe_event_status(struct stmmac_priv *priv, int status) { 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; - if (status == FPE_EVENT_UNKNOWN || !*hs_enable) - return; + spin_lock(&priv->fpe_cfg.lock);
Is this ISR, so that you used the non-IRQ-safe variant?
- /* If LP has sent verify mPacket, LP is FPE capable */
- if ((status & FPE_EVENT_RVER) == FPE_EVENT_RVER) {
- if (*lp_state < FPE_STATE_CAPABLE)
- *lp_state = FPE_STATE_CAPABLE;
+ if (!fpe_cfg->pmac_enabled || status == FPE_EVENT_UNKNOWN)
+ goto __unlock_out;[...]
-#define SEND_VERIFY_MPAKCET_FMT "Send Verify mPacket lo_state=%d lp_state=%d\n"
-static void stmmac_fpe_lp_task(struct work_struct *work)
+static void stmmac_fpe_verify_task(struct work_struct *work)
{
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)
+ int verify_limit = 3; /* defined by 802.3 */If it's a generic/IEEE definition, then either put it somewhere in the generic headers or at least make a definition from it, doesn't open-code directly.
+ unsigned long flags;
+ u32 sleep_ms;
+
+ spin_lock(&priv->fpe_cfg.lock);
+ sleep_ms = fpe_cfg->verify_time;
+ spin_unlock(&priv->fpe_cfg.lock);
+
+ while (1) {
+ /* The initial VERIFY was triggered by linkup event or
+ * stmmac_set_mm(), sleep then check MM_VERIFY_STATUS.
+ */
+ msleep(sleep_ms);
+
+ if (!netif_running(priv->dev))
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);
+ spin_lock_irqsave(&priv->fpe_cfg.lock, flags);
- netdev_info(priv->dev, "configured FPE\n");
+ if (fpe_cfg->status == ETHTOOL_MM_VERIFY_STATUS_DISABLED ||
+ fpe_cfg->status == ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED ||
+ !fpe_cfg->pmac_enabled || !fpe_cfg->verify_enabled) {
+ spin_unlock_irqrestore(&priv->fpe_cfg.lock, flags);
+ break;
+ }
- *lo_state = FPE_STATE_ON;
- *lp_state = FPE_STATE_ON;
- netdev_info(priv->dev, "!!! BOTH FPE stations ON\n");
+ if (verify_limit == 0) {
+ fpe_cfg->verify_enabled = false;
+ fpe_cfg->status = ETHTOOL_MM_VERIFY_STATUS_FAILED;
+ stmmac_fpe_configure(priv, priv->ioaddr, fpe_cfg,
+ priv->plat->tx_queues_to_use,
+ priv->plat->rx_queues_to_use,
+ false);
+ spin_unlock_irqrestore(&priv->fpe_cfg.lock, flags);
break;
}
- 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);
- stmmac_fpe_send_mpacket(priv, priv->ioaddr,
- fpe_cfg,
+ if (fpe_cfg->status == ETHTOOL_MM_VERIFY_STATUS_VERIFYING)
+ stmmac_fpe_send_mpacket(priv, priv->ioaddr, fpe_cfg,
MPACKET_VERIFY);
- }
- /* Sleep then retry */
- msleep(500);
+
+ sleep_ms = fpe_cfg->verify_time;
+
+ spin_unlock_irqrestore(&priv->fpe_cfg.lock, flags);
+
+ verify_limit--;Are these 3 empty newlines needed? I'd remove at least some of them.
quoted hunk ↗ jump to hunk
} clear_bit(__FPE_TASK_SCHED, &priv->fpe_task_state);@@ -7535,8 +7550,8 @@ int stmmac_dvr_probe(struct device *device, INIT_WORK(&priv->service_task, stmmac_service_task); - /* Initialize Link Partner FPE workqueue */ - INIT_WORK(&priv->fpe_task, stmmac_fpe_lp_task); + /* Initialize FPE verify workqueue */ + INIT_WORK(&priv->fpe_task, stmmac_fpe_verify_task); /* Override with kernel parameters if supplied XXX CRS XXX * this needs to have multiple instances@@ -7702,6 +7717,12 @@ int stmmac_dvr_probe(struct device *device, mutex_init(&priv->lock); + spin_lock_init(&priv->fpe_cfg.lock); + priv->fpe_cfg.pmac_enabled = false;
I think it's kzalloc()'d? If so, why initialize booleans to false?
+ priv->fpe_cfg.verify_time = 128; /* ethtool_mm_state.max_verify_time */
Same as verify_limit above, make it a definition, don't open-code.
quoted hunk ↗ jump to hunk
+ priv->fpe_cfg.verify_enabled = false; + priv->fpe_cfg.status = ETHTOOL_MM_VERIFY_STATUS_DISABLED; + /* If a specific clk_csr value is passed from the platform * this means that the CSR Clock Range selection cannot be * changed at run-time and it is fixed. Viceversa the driver'll try to@@ -7875,15 +7896,8 @@ int stmmac_suspend(struct device *dev) } rtnl_unlock(); - if (priv->dma_cap.fpesel) { - /* Disable FPE */ - stmmac_fpe_configure(priv, priv->ioaddr, - &priv->fpe_cfg, - priv->plat->tx_queues_to_use, - priv->plat->rx_queues_to_use, false); - + if (priv->dma_cap.fpesel) stmmac_fpe_stop_wq(priv); - } priv->speed = SPEED_UNKNOWN; return 0;diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c index b0cc45331ff7..783829a6479c 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c@@ -1063,11 +1063,6 @@ static int tc_taprio_configure(struct stmmac_priv *priv, return -EOPNOTSUPP; } - /* Actual FPE register configuration will be done after FPE handshake - * is success. - */ - priv->fpe_cfg.enable = fpe; - ret = stmmac_est_configure(priv, priv, priv->est, priv->plat->clk_ptp_rate); mutex_unlock(&priv->est_lock);@@ -1094,7 +1089,6 @@ static int tc_taprio_configure(struct stmmac_priv *priv, mutex_unlock(&priv->est_lock); } - priv->fpe_cfg.enable = false; stmmac_fpe_configure(priv, priv->ioaddr, &priv->fpe_cfg, priv->plat->tx_queues_to_use,
Thanks, Olek