| 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: <20241030114706.yaevtgpefwfxva5v@skbuf>
Date: Wed, 30 Oct 2024 13:47:06 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Furong Xu <0x1207@...il.com>
Cc: netdev@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
Andrew Lunn <andrew@...n.ch>, Simon Horman <horms@...nel.org>,
andrew+netdev@...n.ch,
Alexandre Torgue <alexandre.torgue@...s.st.com>,
Jose Abreu <joabreu@...opsys.com>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Maxime Coquelin <mcoquelin.stm32@...il.com>, xfr@...look.com
Subject: Re: [PATCH net-next v6 3/6] net: stmmac: Refactor FPE functions to
generic version
On Wed, Oct 30, 2024 at 01:36:12PM +0800, Furong Xu wrote:
> FPE implementation for DWMAC4 and DWXGMAC differs only for:
> 1) Offset address of MAC_FPE_CTRL_STS and MTL_FPE_CTRL_STS
> 2) FPRQ(Frame Preemption Residue Queue) field in MAC_RxQ_Ctrl1
> 3) Bit offset of Frame Preemption Interrupt Enable
>
> Refactor FPE functions to avoid code duplication.
I would add "and to simplify the code flow by avoiding the use of
function pointers".
>
> Signed-off-by: Furong Xu <0x1207@...il.com>
> ---
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c
> index 70ea475046f0..ee86658f77b4 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c
> @@ -27,6 +27,20 @@
> #define STMMAC_MAC_FPE_CTRL_STS_SVER BIT(1)
> #define STMMAC_MAC_FPE_CTRL_STS_EFPE BIT(0)
>
> +struct stmmac_fpe_reg {
> + const u32 mac_fpe_reg; /* offset of MAC_FPE_CTRL_STS */
> + const u32 mtl_fpe_reg; /* offset of MTL_FPE_CTRL_STS */
> + const u32 rxq_ctrl1_reg; /* offset of MAC_RxQ_Ctrl1 */
> + const u32 fprq_mask; /* Frame Preemption Residue Queue */
> + const u32 int_en_reg; /* offset of MAC_Interrupt_Enable */
> + const u32 int_en_bit; /* Frame Preemption Interrupt Enable */
> +};
> +
> +bool stmmac_fpe_supported(struct stmmac_priv *priv)
> +{
> + return (priv->dma_cap.fpesel && priv->fpe_cfg.reg);
> +}
This is a separate logical change from just refactoring. Refactoring
changes (which are noisy) should not have functional changes. Could
the introduction and use of stmmac_fpe_supported() please be a separate
patch?
Also, parentheses are not necessary.
> +
> void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up)
> {
> struct stmmac_fpe_cfg *fpe_cfg = &priv->fpe_cfg;
> @@ -38,25 +52,19 @@ void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up)
>
> if (is_up && fpe_cfg->pmac_enabled) {
> /* VERIFY process requires pmac enabled when NIC comes up */
> - stmmac_fpe_configure(priv, priv->ioaddr, fpe_cfg,
> - priv->plat->tx_queues_to_use,
> - priv->plat->rx_queues_to_use,
> - false, true);
> + stmmac_fpe_configure(priv, false, true);
>
> /* 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);
> + stmmac_fpe_configure(priv, false, false);
> }
>
> spin_unlock_irqrestore(&fpe_cfg->lock, flags);
> }
>
> -void stmmac_fpe_event_status(struct stmmac_priv *priv, int status)
> +static void stmmac_fpe_event_status(struct stmmac_priv *priv, int status)
> {
> struct stmmac_fpe_cfg *fpe_cfg = &priv->fpe_cfg;
>
> @@ -68,8 +76,7 @@ void stmmac_fpe_event_status(struct stmmac_priv *priv, int status)
>
> /* LP has sent verify mPacket */
> if ((status & FPE_EVENT_RVER) == FPE_EVENT_RVER)
> - stmmac_fpe_send_mpacket(priv, priv->ioaddr, fpe_cfg,
> - MPACKET_RESPONSE);
> + stmmac_fpe_send_mpacket(priv, MPACKET_RESPONSE);
>
> /* Local has sent verify mPacket */
> if ((status & FPE_EVENT_TVER) == FPE_EVENT_TVER &&
> @@ -107,8 +114,7 @@ static void stmmac_fpe_verify_timer(struct timer_list *t)
> 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);
> + stmmac_fpe_send_mpacket(priv, MPACKET_VERIFY);
> rearm = true;
> } else {
> fpe_cfg->status = ETHTOOL_MM_VERIFY_STATUS_FAILED;
> @@ -118,10 +124,7 @@ static void stmmac_fpe_verify_timer(struct timer_list *t)
> 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);
> + stmmac_fpe_configure(priv, true, true);
> break;
>
> default:
> @@ -154,6 +157,9 @@ void stmmac_fpe_init(struct stmmac_priv *priv)
> priv->fpe_cfg.status = ETHTOOL_MM_VERIFY_STATUS_DISABLED;
> timer_setup(&priv->fpe_cfg.verify_timer, stmmac_fpe_verify_timer, 0);
> spin_lock_init(&priv->fpe_cfg.lock);
> +
> + if (priv->dma_cap.fpesel && !priv->fpe_cfg.reg)
> + dev_info(priv->device, "FPE on this MAC is not supported by driver.\n");
This as well.
> }
>
> void stmmac_fpe_apply(struct stmmac_priv *priv)
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.h b/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.h
> index 25725fd5182f..00e616d7cbf1 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.h
> @@ -22,24 +22,21 @@ struct stmmac_priv;
> struct stmmac_fpe_cfg;
>
> void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up);
> -void stmmac_fpe_event_status(struct stmmac_priv *priv, int status);
> +bool stmmac_fpe_supported(struct stmmac_priv *priv);
> void stmmac_fpe_init(struct stmmac_priv *priv);
> void stmmac_fpe_apply(struct stmmac_priv *priv);
> -
> -void dwmac5_fpe_configure(void __iomem *ioaddr, struct stmmac_fpe_cfg *cfg,
> - u32 num_txq, u32 num_rxq,
> - bool tx_enable, bool pmac_enable);
> -void dwmac5_fpe_send_mpacket(void __iomem *ioaddr,
> - struct stmmac_fpe_cfg *cfg,
> +void stmmac_fpe_configure(struct stmmac_priv *priv, bool tx_enable,
> + bool pmac_enable);
> +void stmmac_fpe_send_mpacket(struct stmmac_priv *priv,
> enum stmmac_mpacket_type type);
Sorry I noticed this just now. After the refactoring, stmmac_fpe_send_mpacket()
is only used from stmmac_fpe.c, and thus can be unexported and made static.
Same goes for enum stmmac_mpacket_type.
> -int dwmac5_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev);
> -int dwmac5_fpe_get_add_frag_size(const void __iomem *ioaddr);
> -void dwmac5_fpe_set_add_frag_size(void __iomem *ioaddr, u32 add_frag_size);
> +void stmmac_fpe_irq_status(struct stmmac_priv *priv);
> +int stmmac_fpe_get_add_frag_size(struct stmmac_priv *priv);
> +void stmmac_fpe_set_add_frag_size(struct stmmac_priv *priv, u32 add_frag_size);
> +
> int dwmac5_fpe_map_preemption_class(struct net_device *ndev,
> struct netlink_ext_ack *extack, u32 pclass);
>
> -void dwxgmac3_fpe_configure(void __iomem *ioaddr, struct stmmac_fpe_cfg *cfg,
> - u32 num_txq, u32 num_rxq,
> - bool tx_enable, bool pmac_enable);
> +extern const struct stmmac_fpe_reg dwmac5_fpe_reg;
> +extern const struct stmmac_fpe_reg dwxgmac3_fpe_reg;
>
> #endif
Powered by blists - more mailing lists