| 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: <69ae0aec-1b70-4964-9f45-3e468fa277a5@intel.com>
Date: Tue, 20 Aug 2024 16:48:47 +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 4/7] net: stmmac: configure FPE via ethtool-mm
From: Furong Xu <0x1207@...il.com>
Date: Tue, 20 Aug 2024 17:38:32 +0800
> Implement ethtool --show-mm and --set-mm callbacks.
>
> NIC up/down, link up/down, suspend/resume, kselftest-ethtool_mm,
> all tested okay.
>
> Signed-off-by: Furong Xu <0x1207@...il.com>
[...]
> @@ -589,6 +589,21 @@ void dwmac5_fpe_configure(void __iomem *ioaddr, struct stmmac_fpe_cfg *cfg,
> cfg->fpe_csr = 0;
> }
> writel(cfg->fpe_csr, ioaddr + MAC_FPE_CTRL_STS);
> +
> + value = readl(ioaddr + GMAC_INT_EN);
> +
> + if (pmac_enable) {
> + if (!(value & GMAC_INT_FPE_EN)) {
> + /* Dummy read to clear any pending masked interrupts */
> + (void)readl(ioaddr + MAC_FPE_CTRL_STS);
Are you sure this cast to void is needed? Have you seen readl() with
__must_check anywhere?
> +
> + value |= GMAC_INT_FPE_EN;
> + }
> + } else {
> + value &= ~GMAC_INT_FPE_EN;
> + }
> +
> + writel(value, ioaddr + GMAC_INT_EN);
> }
>
> int dwmac5_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev)
> @@ -638,3 +653,20 @@ void dwmac5_fpe_send_mpacket(void __iomem *ioaddr, struct stmmac_fpe_cfg *cfg,
>
> writel(value, ioaddr + MAC_FPE_CTRL_STS);
> }
> +
> +int dwmac5_fpe_get_add_frag_size(void __iomem *ioaddr)
@ioaddr can be const.
> +{
> + return FIELD_GET(AFSZ, readl(ioaddr + MTL_FPE_CTRL_STS));
> +}
> +
> +void dwmac5_fpe_set_add_frag_size(void __iomem *ioaddr, u32 add_frag_size)
> +{
> + u32 value;
> +
> + value = readl(ioaddr + MTL_FPE_CTRL_STS);
> +
> + value &= ~AFSZ;
> + value |= FIELD_PREP(AFSZ, add_frag_size);
> +
> + writel(value, ioaddr + MTL_FPE_CTRL_STS);
value = readl(ioaddr + MTL_FPE_CTRL_STS);
writel(u32_replace_bits(value, add_frag_size, AFSZ),
ioaddr + MTL_FPE_CTRL_STS);
> +}
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac5.h b/drivers/net/ethernet/stmicro/stmmac/dwmac5.h
> index bf33a51d229e..e369e65920fc 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac5.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac5.h
> @@ -39,6 +39,9 @@
> #define MAC_PPSx_INTERVAL(x) (0x00000b88 + ((x) * 0x10))
> #define MAC_PPSx_WIDTH(x) (0x00000b8c + ((x) * 0x10))
>
> +#define MTL_FPE_CTRL_STS 0x00000c90
> +#define AFSZ GENMASK(1, 0)
Can you leave comments next to definitions explaining what this is?
I guessed AFSZ is "added frag size", but meh...
Also, I'd prefix every definition with some vendor prefix (STMMAC_ or
so), otherwise it may conflict one day with some generic one.
> +
> #define MTL_RXP_CONTROL_STATUS 0x00000ca0
> #define RXPI BIT(31)
> #define NPE GENMASK(23, 16)
[...]
> @@ -1270,6 +1271,112 @@ static int stmmac_set_tunable(struct net_device *dev,
> return ret;
> }
>
> +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 add_frag_size;
> +
> + if (!priv->dma_cap.fpesel)
> + return -EOPNOTSUPP;
> +
> + spin_lock_irqsave(&priv->fpe_cfg.lock, flags);
> +
> + state->pmac_enabled = priv->fpe_cfg.pmac_enabled;
> + state->verify_time = priv->fpe_cfg.verify_time;
> + state->verify_enabled = priv->fpe_cfg.verify_enabled;
> + state->verify_status = priv->fpe_cfg.status;
See, you could embed ðtool_mm_state into &fpe_cfg as I wrote under
the previous patch, so that you could do a direct assignment here :D
> + state->rx_min_frag_size = ETH_ZLEN;
> +
> + /* 802.3-2018 clause 30.14.1.6, says that the aMACMergeVerifyTime
> + * variable has a range between 1 and 128 ms inclusive. Limit to that.
> + */
Also make it a definition.
> + 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")
I think it's not needed to leave commit references in the code?
> + */
> + state->tx_enabled = !!(priv->fpe_cfg.fpe_csr == EFPE);
tx_enabled is bool, you don't need to add a double negation here (the
parenthesis are redundant as well).
> +
> + /* 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 ||
> + state->verify_status == ETHTOOL_MM_VERIFY_STATUS_DISABLED);
> +
> + add_frag_size = stmmac_fpe_get_add_frag_size(priv, priv->ioaddr);
> + state->tx_min_frag_size = ethtool_mm_frag_size_add_to_min(add_frag_size);
> +
> + spin_unlock_irqrestore(&priv->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 6ae95f20b24f..00ed0543f5cf 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -3537,8 +3537,21 @@ 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)
> + 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);
I don't think this happens in the interrupt context, so you need
_irqsave() version here?
> + 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;
> }
> @@ -7417,7 +7430,7 @@ static void stmmac_fpe_verify_task(struct work_struct *work)
> stmmac_fpe_configure(priv, priv->ioaddr, fpe_cfg,
> priv->plat->tx_queues_to_use,
> priv->plat->rx_queues_to_use,
> - false);
> + false, fpe_cfg->pmac_enabled);
> spin_unlock_irqrestore(&priv->fpe_cfg.lock, flags);
> break;
> }
[...]
Thanks,
Olek
Powered by blists - more mailing lists