netdev - Re: [PATCH net-next v1 5/5] net: stmmac: xgmac: Complete FPE support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241017122936.GF1697@kernel.org>
Date: Thu, 17 Oct 2024 13:29:36 +0100
From: Simon Horman <horms@...nel.org>
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,
	Vladimir Oltean <olteanv@...il.com>, Andrew Lunn <andrew@...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 v1 5/5] net: stmmac: xgmac: Complete FPE support

On Tue, Oct 15, 2024 at 05:09:26PM +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
> 
> Refactor stmmac_fpe_ops callback functions to avoid code duplication
> between gmac4 and xgmac.
> 
> Signed-off-by: Furong Xu <0x1207@...il.com>

Hi Furong Xu,

I think it would be best to split this patch so that the refactor of dwmac4
code is in one patch, and adding xgmac code is in another.

...

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c
> index 6060a1d702c6..80f12b6e84e6 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c
> @@ -160,41 +160,54 @@ void stmmac_fpe_apply(struct stmmac_priv *priv)
>  	}
>  }
>  
> -static void dwmac5_fpe_configure(void __iomem *ioaddr,
> -				 struct stmmac_fpe_cfg *cfg,
> -				 u32 num_txq, u32 num_rxq,
> -				 bool tx_enable, bool pmac_enable)
> +static void common_fpe_configure(void __iomem *ioaddr,
> +				 struct stmmac_fpe_cfg *cfg, u32 rxq,
> +				 bool tx_enable, bool pmac_enable,
> +				 u32 rxq_addr, u32 fprq_mask, u32 fprq_shift,
> +				 u32 mac_fpe_addr, u32 int_en_addr,
> +				 u32 int_en_bit)

This function now has a lot of parameters. Could we consider another way?
One idea I had was that describes the addresses for different chips.

...

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> index 75ad2da1a37f..6a79e6a111ed 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> @@ -1290,8 +1290,8 @@ const struct stmmac_tc_ops dwxgmac_tc_ops = {
>  	.setup_cls_u32 = tc_setup_cls_u32,
>  	.setup_cbs = tc_setup_cbs,
>  	.setup_cls = tc_setup_cls,
> -	.setup_taprio = tc_setup_taprio_without_fpe,
> +	.setup_taprio = tc_setup_taprio,
>  	.setup_etf = tc_setup_etf,
>  	.query_caps = tc_query_caps,
> -	.setup_mqprio = tc_setup_mqprio_unimplemented,
> +	.setup_mqprio = tc_setup_dwmac510_mqprio,
>  };

It is not clear to me how this hunk relates to the rest of the patch.