| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <29ccab1d-cf8b-4a5d-8d9a-54535925f71b@intel.com>
Date: Tue, 20 Aug 2024 16:36:26 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Furong Xu <0x1207@...il.com>
CC: Serge Semin <fancer.lancer@...il.com>, Andrew Lunn <andrew@...n.ch>,
Vladimir Oltean <olteanv@...il.com>, "David S. Miller" <davem@...emloft.net>,
Alexandre Torgue <alexandre.torgue@...s.st.com>, Jose Abreu
<joabreu@...opsys.com>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
<kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Maxime Coquelin
<mcoquelin.stm32@...il.com>, Joao Pinto <jpinto@...opsys.com>,
<netdev@...r.kernel.org>, <linux-stm32@...md-mailman.stormreply.com>,
<linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
<xfr@...look.com>
Subject: Re: [PATCH net-next v4 3/7] net: stmmac: refactor FPE verification
process
From: Furong Xu <0x1207@...il.com>
Date: Tue, 20 Aug 2024 17:38:31 +0800
> 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@...il.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;
Why not embed ðtool_mm_state here then?
> };
>
> 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?
> + 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.
> }
>
> 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.
> + 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
Powered by blists - more mailing lists