netdev - Re: [PATCH net-next v3 3/6] net: stmmac: Refactor FPE functions to generic version

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] [day] [month] [year] [list]

Message-ID: <20241023141700.niz3ow2xu6pbgbg4@skbuf>
Date: Wed, 23 Oct 2024 17:17:00 +0300
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>,
	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 v3 3/6] net: stmmac: Refactor FPE functions to
 generic version

On Wed, Oct 23, 2024 at 03:05:23PM +0800, Furong Xu wrote:
> -void dwmac5_fpe_configure(void __iomem *ioaddr, struct stmmac_fpe_cfg *cfg,
> -			  u32 num_txq, u32 num_rxq,
> +void stmmac_fpe_configure(struct stmmac_priv *priv, u32 num_txq, u32 num_rxq,
>  			  bool tx_enable, bool pmac_enable)
>  {
> +	struct stmmac_fpe_cfg *cfg = &priv->fpe_cfg;
> +	const struct stmmac_fpe_reg *reg = cfg->reg;
> +	void __iomem *ioaddr = priv->ioaddr;
>  	u32 value;
>  
> +	if (!reg)
> +		return;

What are all these "if (!reg) return" checks protecting against?
At all call sites you ensure that priv->dma_cap.fpesel is true.
If defense against driver writers is necessary, check only once at boot
time that if priv->dma_cap.fpesel is true, we have a way to handle it
(priv->fpe_cfg.reg is set). Or are there still supported DWMAC variants
with the FPE hardware capability but without driver support?

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index ab547430a717..7874a955ab60 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -5944,8 +5944,7 @@ static void stmmac_common_interrupt(struct stmmac_priv *priv)
>  				      &priv->xstats, tx_cnt);
>  
>  	if (priv->dma_cap.fpesel) {
> -		int status = stmmac_fpe_irq_status(priv, priv->ioaddr,
> -						   priv->dev);
> +		int status = stmmac_fpe_irq_status(priv);
>  
>  		stmmac_fpe_event_status(priv, status);
>  	}

I think this coding pattern is illogical, and the code refactoring makes
it more apparent. stmmac_common_interrupt() does nothing with "status",
it just takes it as a return code from stmmac_fpe_irq_status(), and
passes it to stmmac_fpe_event_status(), both of which are in
stmmac_fpe.c. Why isn't there a direct tail call from one function to
the other, to simplify the external API exposed by stmmac_fpe.h?

> -- 
> 2.34.1
>