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

Vladimir Oltean
2024-08-20T12:36:34Z

[-- Attachment #1: Type: text/plain, Size: 12169 bytes --]

On Tue, Aug 20, 2024 at 05:38:31PM +0800, Furong Xu wrote:
> 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 <0x1207@gmail.com>
> ---
> 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;
> };
>
> 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;

The fpe_task may be scheduled here. When you unlock, it may run and
overwrite the fpe_cfg->status you've just set.

Although I don't actually recommend setting ETHTOOL_MM_VERIFY_STATUS_DISABLED
unless cfg->verify_enabled=false.

> +
> +__unlock_out:
> + 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;
> + }

With this block of code, you're saying that you're deliberately okay for
the ethtool-mm state to be lost after a stmmac_release() call. Mind you,
some of the call sites of this are:
- stmmac_change_mtu()
- stmmac_reinit_queues()
- stmmac_reinit_ringparam()

I disagree that it's okay to lose the state configured by user space.
Instead, you should reprogram the saved state once lost.

Note that because stmmac_release() calls phylink_stop(), I think that
restoring the state in stmmac_fpe_link_state_handle() is enough. Because
there will always be a link drop.

> +
> + 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);
>
> - /* 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;
>
> - /* If user has requested FPE enable, quickly response */
> - if (*hs_enable)
> - stmmac_fpe_send_mpacket(priv, priv->ioaddr,
> - fpe_cfg,
> - MPACKET_RESPONSE);
> - }
> + /* LP has sent verify mPacket */
> + if ((status & FPE_EVENT_RVER) == FPE_EVENT_RVER)
> + stmmac_fpe_send_mpacket(priv, priv->ioaddr, fpe_cfg,
> + MPACKET_RESPONSE);
>
> - /* If Local has sent verify mPacket, Local is FPE capable */
> - if ((status & FPE_EVENT_TVER) == FPE_EVENT_TVER) {
> - if (*lo_state < FPE_STATE_CAPABLE)
> - *lo_state = FPE_STATE_CAPABLE;
> - }
> + /* Local has sent verify mPacket */
> + if ((status & FPE_EVENT_TVER) == FPE_EVENT_TVER &&
> + fpe_cfg->status != ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED)
> + fpe_cfg->status = ETHTOOL_MM_VERIFY_STATUS_VERIFYING;
>
> - /* If LP has sent response mPacket, LP is entering FPE ON */
> + /* LP has sent response mPacket */
> if ((status & FPE_EVENT_RRSP) == FPE_EVENT_RRSP)
> - *lp_state = FPE_STATE_ENTERING_ON;
> -
> - /* If Local has sent response mPacket, Local is entering FPE ON */
> - if ((status & FPE_EVENT_TRSP) == FPE_EVENT_TRSP)
> - *lo_state = FPE_STATE_ENTERING_ON;
> + fpe_cfg->status = ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED;
>
> if (!test_bit(__FPE_REMOVING, &priv->fpe_task_state) &&
> !test_and_set_bit(__FPE_TASK_SCHED, &priv->fpe_task_state) &&
> priv->fpe_wq) {
> queue_work(priv->fpe_wq, &priv->fpe_task);
> }
> +
> +__unlock_out:
> + spin_unlock(&priv->fpe_cfg.lock);
> }
>
> static void stmmac_common_interrupt(struct stmmac_priv *priv)
> @@ -7372,50 +7380,57 @@ int stmmac_reinit_ringparam(struct net_device *dev, u32 rx_size, u32 tx_size)
> return ret;
> }
>
> -#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 */
> + 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);

Thanks for the added comment. But why don't you just use queue_delayed_work()
instead of queue_work() and sleeping at the very beginning?

With this, you really don't need to drop the lock and read fpe_cfg->verify_time
twice.

But I think what is needed here is better suited for a timer, especially
because of the required coordination with the IRQ. See the end and the
attachment for more details.

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

I don't understand why turn off verify_enabled after failure? Only the
user should be able to modify this.

> + 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);

I don't understand why turn off tx_enabled after failure, rather than
not turning it on at all until success?

This really has me thinking. This hardware does not have the explicit
notion of the verification state - it is purely a driver construct.
So I wonder if the EFPE bit in MAC_FPE_CTRL_STS isn't actually what
corresponds to "tx_active" rather than "tx_enabled"?
(definitions at https://docs.kernel.org/networking/ethtool-netlink.html)

And "tx_enabled" would just correspond to a state variable in the driver,
which does nothing until verification is actually complete.

There is a test in manual_failed_verification() which checks the
correctness of the tx_enabled/tx_active behavior. If tx_enabled=true but
verification fails (and also _up until_ that point), the MM layer is
supposed to send packets through the eMAC (because tx_active=false).
But for your driver, that test is inconclusive, because you don't report
ethtool stats broken down by eMAC/pMAC, just aggregate. So we don't know
unless we take a closer look manually at the driver in that state.

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

I took the liberty of rewriting the fpe_task to a timer, and delete the
workqueue. Here is a completely untested patch, which at least is less
complex, has less code and is easier to understand. What do you think?

[-- Attachment #2: 0001-net-stmmac-replace-FPE-workqueue-with-timer.patch --]
[-- Type: text/x-diff, Size: 14770 bytes --]

From 6ce277245128638160385d948583a3e6d2561a94 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Tue, 20 Aug 2024 14:50:32 +0300
Subject: [PATCH] net: stmmac: replace FPE workqueue with timer

What remains in the fpe_task after decoupling RX from TX appears
overengineered to use a workqueue. A timer which retransmits Verify
mPackets until the verify_limit expires, or enables transmission on
success, seems enough.

In the INITIAL state, the timer sends MPACKET_VERIFY. Eventually the
stmmac_fpe_event_status() IRQ fires and advances the state to VERIFYING,
then rearms the timer after verify_time ms. If a subsequent IRQ comes in
and modifies the state to SUCCEEDED after getting MPACKET_RESPONSE, the
timer sees this. It must enable the EFPE bit now. Otherwise, it
decrements the verify_limit counter and tries again. Eventually it
moves the status to FAILED, from which the IRQ cannot move it anywhere
else, except for another stmmac_fpe_apply() call.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
drivers/net/ethernet/stmicro/stmmac/stmmac.h | 16 +-
.../ethernet/stmicro/stmmac/stmmac_ethtool.c | 35 +--
.../net/ethernet/stmicro/stmmac/stmmac_main.c | 212 +++++++-----------
3 files changed, 100 insertions(+), 163 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 407b59f2783f..dd15f71e1663 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -152,21 +152,18 @@ enum stmmac_mpacket_type {
MPACKET_RESPONSE = 1,
};

-enum stmmac_fpe_task_state_t {
- __FPE_REMOVING,
- __FPE_TASK_SCHED,
-};
-
struct stmmac_fpe_cfg {
/* Serialize access to MAC Merge state between ethtool requests
* and link state updates.
*/
spinlock_t lock;
-
+ struct timer_list verify_timer;
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 */
+ bool tx_enabled;
+ int verify_limit;
enum ethtool_mm_verify_status status;
};

@@ -364,10 +361,6 @@ struct stmmac_priv {
struct work_struct service_task;

/* Frame Preemption feature (FPE) */
- unsigned long fpe_task_state;
- struct workqueue_struct *fpe_wq;
- struct work_struct fpe_task;
- char wq_name[IFNAMSIZ + 4];
struct stmmac_fpe_cfg fpe_cfg;

/* TC Handling */
@@ -422,7 +415,8 @@ bool stmmac_eee_init(struct stmmac_priv *priv);
int stmmac_reinit_queues(struct net_device *dev, u32 rx_cnt, u32 tx_cnt);
int stmmac_reinit_ringparam(struct net_device *dev, u32 rx_size, u32 tx_size);
int stmmac_bus_clks_config(struct stmmac_priv *priv, bool enabled);
-void stmmac_fpe_handshake(struct stmmac_priv *priv, bool enable);
+void stmmac_fpe_apply(struct stmmac_priv *priv);
+void stmmac_fpe_verify_timer_arm(struct stmmac_fpe_cfg *fpe_cfg);

static inline bool stmmac_xdp_is_enabled(struct stmmac_priv *priv)
{
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
index a8cdcacecc26..3eb5344e2412 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
@@ -1293,14 +1293,7 @@ static int stmmac_get_mm(struct net_device *ndev,
* variable has a range between 1 and 128 ms inclusive. Limit to that.
*/
state->max_verify_time = 128;
-
- /* Cannot read MAC_FPE_CTRL_STS register here, or FPE interrupt events
- * can be lost.
- *
- * See commit 37e4b8df27bc ("net: stmmac: fix FPE events losing")
- */
- state->tx_enabled = !!(priv->fpe_cfg.fpe_csr == EFPE);
-
+ state->tx_enabled = priv->fpe_cfg.tx_enabled;
/* FPE active if common tx_enabled and verification success or disabled (forced) */
state->tx_active = state->tx_enabled &&
(state->verify_status == ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED ||
@@ -1326,34 +1319,28 @@ static int stmmac_set_mm(struct net_device *ndev, struct ethtool_mm_cfg *cfg,
if (!priv->dma_cap.fpesel)
return -EOPNOTSUPP;

- /* Wait for the fpe_task that's currently in progress to finish */
- flush_workqueue(priv->fpe_wq);
-
err = ethtool_mm_frag_size_min_to_add(cfg->tx_min_frag_size,
&add_frag_size, extack);
if (err)
return err;

- spin_lock_irqsave(&priv->fpe_cfg.lock, flags);
+ /* Wait for the verification that's currently in progress to finish */
+ del_timer_sync(&fpe_cfg->verify_timer);
+
+ spin_lock_irqsave(&fpe_cfg->lock, flags);

fpe_cfg->pmac_enabled = cfg->pmac_enabled;
+ fpe_cfg->tx_enabled = cfg->tx_enabled;
fpe_cfg->verify_time = cfg->verify_time;
fpe_cfg->verify_enabled = cfg->verify_enabled;
-
- stmmac_fpe_configure(priv, priv->ioaddr, fpe_cfg,
- priv->plat->tx_queues_to_use,
- priv->plat->rx_queues_to_use,
- cfg->tx_enabled, cfg->pmac_enabled);
+ fpe_cfg->verify_limit = 3; /* IEEE 802.3 constant */
+ if (!cfg->verify_enabled)
+ fpe_cfg->status = ETHTOOL_MM_VERIFY_STATUS_DISABLED;

stmmac_fpe_set_add_frag_size(priv, priv->ioaddr, add_frag_size);
+ stmmac_fpe_apply(priv);

- if (cfg->verify_enabled)
- stmmac_fpe_send_mpacket(priv, priv->ioaddr, fpe_cfg,
- MPACKET_VERIFY);
- else
- fpe_cfg->status = ETHTOOL_MM_VERIFY_STATUS_DISABLED;
-
- spin_unlock_irqrestore(&priv->fpe_cfg.lock, flags);
+ spin_unlock_irqrestore(&fpe_cfg->lock, flags);

return 0;
}
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index a5d01162fcc5..fa74504f3ad5 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -971,19 +971,22 @@ static void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up)
struct stmmac_fpe_cfg *fpe_cfg = &priv->fpe_cfg;
unsigned long flags;

- spin_lock_irqsave(&priv->fpe_cfg.lock, flags);
+ del_timer_sync(&fpe_cfg->verify_timer);

- if (!fpe_cfg->pmac_enabled)
- goto __unlock_out;
+ spin_lock_irqsave(&fpe_cfg->lock, flags);

- if (is_up && fpe_cfg->verify_enabled)
- stmmac_fpe_send_mpacket(priv, priv->ioaddr, fpe_cfg,
- MPACKET_VERIFY);
- else
- fpe_cfg->status = ETHTOOL_MM_VERIFY_STATUS_DISABLED;
+ if (is_up) {
+ /* New link => maybe new partner => new verification process */
+ stmmac_fpe_apply(priv);
+ } else {
+ /* No link => turn off EFPE */
+ stmmac_fpe_configure(priv, priv->ioaddr, fpe_cfg,
+ priv->plat->tx_queues_to_use,
+ priv->plat->rx_queues_to_use,
+ false, false);
+ }

-__unlock_out:
- spin_unlock_irqrestore(&priv->fpe_cfg.lock, flags);
+ spin_unlock_irqrestore(&fpe_cfg->lock, flags);
}

static void stmmac_mac_link_down(struct phylink_config *config,
@@ -3362,27 +3365,6 @@ static void stmmac_safety_feat_configuration(struct stmmac_priv *priv)
}
}

-static int stmmac_fpe_start_wq(struct stmmac_priv *priv)
-{
- char *name;
-
- clear_bit(__FPE_TASK_SCHED, &priv->fpe_task_state);
- clear_bit(__FPE_REMOVING, &priv->fpe_task_state);
-
- name = priv->wq_name;
- sprintf(name, "%s-fpe", priv->dev->name);
-
- priv->fpe_wq = create_singlethread_workqueue(name);
- if (!priv->fpe_wq) {
- netdev_err(priv->dev, "%s: Failed to create workqueue\n", name);
-
- return -ENOMEM;
- }
- netdev_dbg(priv->dev, "FPE workqueue start");
-
- return 0;
-}
-
/**
* stmmac_hw_setup - setup mac in a usable state.
* @dev : pointer to the device structure.
@@ -3537,22 +3519,6 @@ static int stmmac_hw_setup(struct net_device *dev, bool ptp_register)

stmmac_set_hw_vlan_mode(priv, priv->hw);

- if (priv->dma_cap.fpesel) {
- /* A SW reset just happened in stmmac_init_dma_engine(),
- * we should restore fpe_cfg to HW, or FPE will stop working
- * from suspend/resume.
- */
- spin_lock(&priv->fpe_cfg.lock);
- stmmac_fpe_configure(priv, priv->ioaddr,
- &priv->fpe_cfg,
- priv->plat->tx_queues_to_use,
- priv->plat->rx_queues_to_use,
- false, priv->fpe_cfg.pmac_enabled);
- spin_unlock(&priv->fpe_cfg.lock);
-
- stmmac_fpe_start_wq(priv);
- }
-
return 0;
}

@@ -4049,18 +4015,6 @@ static int stmmac_open(struct net_device *dev)
return ret;
}

-static void stmmac_fpe_stop_wq(struct stmmac_priv *priv)
-{
- set_bit(__FPE_REMOVING, &priv->fpe_task_state);
-
- if (priv->fpe_wq) {
- destroy_workqueue(priv->fpe_wq);
- priv->fpe_wq = NULL;
- }
-
- netdev_dbg(priv->dev, "FPE workqueue stop");
-}
-
/**
* stmmac_release - close entry point of the driver
* @dev : device pointer.
@@ -4108,22 +4062,8 @@ static int stmmac_release(struct net_device *dev)

stmmac_release_ptp(priv);

- 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;
- }
+ if (priv->dma_cap.fpesel)
+ del_timer_sync(&priv->fpe_cfg.verify_timer);

pm_runtime_put(priv->device);

@@ -6030,11 +5970,7 @@ static void stmmac_fpe_event_status(struct stmmac_priv *priv, int status)
if ((status & FPE_EVENT_RRSP) == FPE_EVENT_RRSP)
fpe_cfg->status = ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED;

- if (!test_bit(__FPE_REMOVING, &priv->fpe_task_state) &&
- !test_and_set_bit(__FPE_TASK_SCHED, &priv->fpe_task_state) &&
- priv->fpe_wq) {
- queue_work(priv->fpe_wq, &priv->fpe_task);
- }
+ stmmac_fpe_verify_timer_arm(fpe_cfg);

__unlock_out:
spin_unlock(&priv->fpe_cfg.lock);
@@ -7395,60 +7331,82 @@ int stmmac_reinit_ringparam(struct net_device *dev, u32 rx_size, u32 tx_size)
return ret;
}

-static void stmmac_fpe_verify_task(struct work_struct *work)
+/**
+ * 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;
- int verify_limit = 3; /* defined by 802.3 */
- unsigned long flags;
- u32 sleep_ms;
+ 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);
+ bool rearm = false;

- spin_lock(&priv->fpe_cfg.lock);
- sleep_ms = fpe_cfg->verify_time;
- spin_unlock(&priv->fpe_cfg.lock);
+ spin_lock(&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;
-
- spin_lock_irqsave(&priv->fpe_cfg.lock, flags);
-
- 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;
- }
-
- if (verify_limit == 0) {
- fpe_cfg->verify_enabled = false;
+ switch (fpe_cfg->status) {
+ case ETHTOOL_MM_VERIFY_STATUS_INITIAL:
+ case ETHTOOL_MM_VERIFY_STATUS_VERIFYING:
+ stmmac_fpe_send_mpacket(priv, priv->ioaddr, fpe_cfg,
+ MPACKET_VERIFY);
+ if (fpe_cfg->verify_limit != 0) {
+ fpe_cfg->status = ETHTOOL_MM_VERIFY_STATUS_VERIFYING;
+ rearm = true;
+ } else {
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, fpe_cfg->pmac_enabled);
- spin_unlock_irqrestore(&priv->fpe_cfg.lock, flags);
- break;
}
+ fpe_cfg->verify_limit--;
+ break;
+ 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;
+ default:
+ break;
+ }

- if (fpe_cfg->status == ETHTOOL_MM_VERIFY_STATUS_VERIFYING)
- stmmac_fpe_send_mpacket(priv, priv->ioaddr, fpe_cfg,
- MPACKET_VERIFY);
-
- sleep_ms = fpe_cfg->verify_time;
+ if (rearm) {
+ mod_timer(&fpe_cfg->verify_timer,
+ msecs_to_jiffies(fpe_cfg->verify_time));
+ }

- spin_unlock_irqrestore(&priv->fpe_cfg.lock, flags);
+ spin_unlock(&fpe_cfg->lock);
+}

- verify_limit--;
+void stmmac_fpe_verify_timer_arm(struct stmmac_fpe_cfg *fpe_cfg)
+{
+ if (fpe_cfg->pmac_enabled && fpe_cfg->tx_enabled &&
+ fpe_cfg->verify_enabled &&
+ fpe_cfg->status != ETHTOOL_MM_VERIFY_STATUS_FAILED &&
+ fpe_cfg->status != ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED) {
+ mod_timer(&fpe_cfg->verify_timer,
+ msecs_to_jiffies(fpe_cfg->verify_time));
}
+}

- clear_bit(__FPE_TASK_SCHED, &priv->fpe_task_state);
+void stmmac_fpe_apply(struct stmmac_priv *priv)
+{
+ struct stmmac_fpe_cfg *fpe_cfg = &priv->fpe_cfg;
+
+ /* If verification is disabled, configure FPE right away.
+ * Otherwise let the timer code do it.
+ */
+ if (!fpe_cfg->verify_enabled) {
+ stmmac_fpe_configure(priv, priv->ioaddr, fpe_cfg,
+ priv->plat->tx_queues_to_use,
+ priv->plat->rx_queues_to_use,
+ fpe_cfg->tx_enabled,
+ fpe_cfg->pmac_enabled);
+ } else {
+ fpe_cfg->status = ETHTOOL_MM_VERIFY_STATUS_INITIAL;
+ stmmac_fpe_verify_timer_arm(fpe_cfg);
+ }
}

static int stmmac_xdp_rx_timestamp(const struct xdp_md *_ctx, u64 *timestamp)
@@ -7565,9 +7523,6 @@ int stmmac_dvr_probe(struct device *device,

INIT_WORK(&priv->service_task, stmmac_service_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
*/
@@ -7733,6 +7688,7 @@ int stmmac_dvr_probe(struct device *device,
mutex_init(&priv->lock);

spin_lock_init(&priv->fpe_cfg.lock);
+ timer_setup(&priv->fpe_cfg.verify_timer, stmmac_fpe_verify_timer, 0);
priv->fpe_cfg.pmac_enabled = false;
priv->fpe_cfg.verify_time = 128; /* ethtool_mm_state.max_verify_time */
priv->fpe_cfg.verify_enabled = false;
@@ -7912,7 +7868,7 @@ int stmmac_suspend(struct device *dev)
rtnl_unlock();

if (priv->dma_cap.fpesel)
- stmmac_fpe_stop_wq(priv);
+ del_timer_sync(&priv->fpe_cfg.verify_timer);

priv->speed = SPEED_UNKNOWN;
return 0;
--
2.34.1