linux-kernel - Re: [PATCH net-next v5 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241029185231.fgy6tofi2uoslp3l@skbuf>
Date: Tue, 29 Oct 2024 20:52:31 +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 v5 3/6] net: stmmac: Refactor FPE functions to
 generic version

On Mon, Oct 28, 2024 at 11:07:26AM +0800, Furong Xu wrote:
>  void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up)
>  {
>  	struct stmmac_fpe_cfg *fpe_cfg = &priv->fpe_cfg;
>  	unsigned long flags;
>  
> +	if (!priv->dma_cap.fpesel)
> +		return;
> +

Minor nitpick: all call sites also have this test already.

>  	timer_shutdown_sync(&fpe_cfg->verify_timer);
>  
>  	spin_lock_irqsave(&fpe_cfg->lock, flags);
>  
>  	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,
> +		stmmac_fpe_configure(priv, priv->plat->tx_queues_to_use,
>  				     priv->plat->rx_queues_to_use,
>  				     false, true);
>  
> @@ -154,6 +161,11 @@ 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_warn(priv->device, "FPE on this MAC is not supported by driver, force disable it.\n");
> +		priv->dma_cap.fpesel = 0;
> +	}

Let's not change the output of stmmac_dma_cap_show() sysfs attribute if
we don't have to. Who knows what depends on that. It's better to
introduce stmmac_fpe_supported(), which tests for both conditions,
and use it throughout (except, of course, for the sysfs, which should
still print the raw DMA capability).

Which devices would those even be, which support FPE but the driver
doesn't deal with them (after your XGMAC addition), do you have any idea?

>  }
>  
>  void stmmac_fpe_apply(struct stmmac_priv *priv)
> @@ -164,8 +176,7 @@ void stmmac_fpe_apply(struct stmmac_priv *priv)
>  	 * Otherwise let the timer code do it.
>  	 */
>  	if (!fpe_cfg->verify_enabled) {
> -		stmmac_fpe_configure(priv, priv->ioaddr, fpe_cfg,
> -				     priv->plat->tx_queues_to_use,
> +		stmmac_fpe_configure(priv, priv->plat->tx_queues_to_use,
>  				     priv->plat->rx_queues_to_use,
>  				     fpe_cfg->tx_enabled,
>  				     fpe_cfg->pmac_enabled);
> @@ -178,50 +189,54 @@ 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,
> +void stmmac_fpe_configure(struct stmmac_priv *priv, u32 num_txq, u32 num_rxq,
>  			  bool tx_enable, bool pmac_enable)

num_txq? not used anywhere. num_rxq? can be retrieved from the "priv"
pointer already provided.

The rest of the series looks good, I have no other comments.