netdev - Re: [PATCH net-next v1] net: stmmac: Refactor Frame Preemption(FPE) implementation

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: <7cde7743-2a8c-4d12-aecb-d1e50d5099ea@lunn.ch>
Date: Mon, 8 Jul 2024 20:44:31 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Furong Xu <0x1207@...il.com>
Cc: "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, rock.xu@....com
Subject: Re: [PATCH net-next v1] net: stmmac: Refactor Frame Preemption(FPE)
 implementation

On Mon, Jul 08, 2024 at 04:22:20PM +0800, Furong Xu wrote:
> Refactor FPE implementation by moving common code for DWMAC4 and
> DWXGMAC into a separate FPE module.
> 
> FPE implementation for DWMAC4 and DWXGMAC differs only for:
> 1) Offset address of MAC_FPE_CTRL_STS
> 2) FPRQ(Frame Preemption Residue Queue) field in MAC_RxQ_Ctrl1

This quite a big patch. Could you split it up a bit please.

> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> @@ -84,8 +84,8 @@
>  #define XGMAC_MCBCQEN			BIT(15)
>  #define XGMAC_MCBCQ			GENMASK(11, 8)
>  #define XGMAC_MCBCQ_SHIFT		8
> -#define XGMAC_RQ			GENMASK(7, 4)
> -#define XGMAC_RQ_SHIFT			4
> +#define XGMAC_FPRQ			GENMASK(7, 4)
> +#define XGMAC_FPRQ_SHIFT		4

You could for example put renames like this into a patch. It should
then be obviously correct, so quick and easy to review.

> @@ -96,10 +96,11 @@
>  #define XGMAC_LPIIS			BIT(5)
>  #define XGMAC_PMTIS			BIT(4)
>  #define XGMAC_INT_EN			0x000000b4
> +#define XGMAC_FPEIE			BIT(15)
>  #define XGMAC_TSIE			BIT(12)
>  #define XGMAC_LPIIE			BIT(5)
>  #define XGMAC_PMTIE			BIT(4)
> -#define XGMAC_INT_DEFAULT_EN		(XGMAC_LPIIE | XGMAC_PMTIE)
> +#define XGMAC_INT_DEFAULT_EN		(XGMAC_FPEIE | XGMAC_LPIIE | XGMAC_PMTIE)

This change is not obvious. First off, it would of been easier to read
if you put XGMAC_LPIIE at the end. But i would also suggest you make
this a separate patch, and have the commit message explain why this is
needed.

> +static void fpe_configure(struct stmmac_priv *priv, struct stmmac_fpe_cfg *cfg,
> +			  u32 num_txq, u32 num_rxq, bool enable)
> +{
> +	u32 value;
> +
> +	if (enable) {
> +		cfg->fpe_csr = FPE_CTRL_STS_EFPE;
> +		if (priv->plat->has_xgmac) {
> +			value = readl(priv->ioaddr + XGMAC_RXQ_CTRL1);
> +			value &= ~XGMAC_FPRQ;
> +			value |= (num_rxq - 1) << XGMAC_FPRQ_SHIFT;
> +			writel(value, priv->ioaddr + XGMAC_RXQ_CTRL1);
> +		} else if (priv->plat->has_gmac4) {
> +			value = readl(priv->ioaddr + GMAC_RXQ_CTRL1);
> +			value &= ~GMAC_RXQCTRL_FPRQ;
> +			value |= (num_rxq - 1) << GMAC_RXQCTRL_FPRQ_SHIFT;
> +			writel(value, priv->ioaddr + GMAC_RXQ_CTRL1);
> +		}

Since you are using a structure of function pointers, it would seem
more logical to have a fpe_xgmac_configure() and a
fpe_gmac4_configure(), and then xgmac_fpe_ops and gmac4_fpe_ops.

> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -974,8 +974,7 @@ static void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up)
>  	bool *hs_enable = &fpe_cfg->hs_enable;
>  
>  	if (is_up && *hs_enable) {
> -		stmmac_fpe_send_mpacket(priv, priv->ioaddr, fpe_cfg,
> -					MPACKET_VERIFY);
> +		stmmac_fpe_send_mpacket(priv, priv, fpe_cfg, MPACKET_VERIFY);

passing priv twice looks very odd! It makes me think the API is
designed wrong. This could be because of the refactoring changes you
made? Maybe add another patch cleaning this up?

    Andrew

---
pw-bot: cr