linux-kernel - Re: [PATCH net v4 3/3] net: stmmac: Specify hardware capability value when FIFO size isn't specified

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: <54f00871f7f4043474a4e83bea6b8935fcf5af88.camel@xry111.site>
Date: Sat, 01 Feb 2025 20:10:00 +0800
From: Xi Ruoyao <xry111@...111.site>
To: Steven Price <steven.price@....com>, Andrew Lunn <andrew@...n.ch>
Cc: Kunihiko Hayashi <hayashi.kunihiko@...ionext.com>, Alexandre Torgue	
 <alexandre.torgue@...s.st.com>, Jose Abreu <joabreu@...opsys.com>, Andrew
 Lunn	 <andrew+netdev@...n.ch>, "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>, Russell King	 <linux@...linux.org.uk>, Yanteng
 Si <si.yanteng@...ux.dev>, Furong Xu	 <0x1207@...il.com>, Joao Pinto
 <Joao.Pinto@...opsys.com>, 	netdev@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, 	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net v4 3/3] net: stmmac: Specify hardware capability
 value when FIFO size isn't specified

On Fri, 2025-01-31 at 15:54 +0000, Steven Price wrote:
> On 31/01/2025 15:29, Andrew Lunn wrote:
> > On Fri, Jan 31, 2025 at 03:03:11PM +0000, Steven Price wrote:
> > > On 31/01/2025 14:47, Andrew Lunn wrote:
> > > > > > I'm guessing, but in your setup, i assume the value is never written
> > > > > > to a register, hence 0 is O.K. e.g. dwmac1000_dma_operation_mode_rx(),
> > > > > > the fifosz value is used to determine if flow control can be used, but
> > > > > > is otherwise ignored.
> > > > > 
> > > > > I haven't traced the code, but that fits my assumptions too.
> > > > 
> > > > I could probably figure it out using code review, but do you know
> > > > which set of DMA operations your hardware uses? A quick look at
> > > > dwmac-rk.c i see:
> > > > 
> > > >         /* If the stmmac is not already selected as gmac4,
> > > >          * then make sure we fallback to gmac.
> > > >          */
> > > >         if (!plat_dat->has_gmac4)
> > > >                 plat_dat->has_gmac = true;
> > > 
> > > has_gmac4 is false on this board, so has_gmac will be set to true here.
> > 
> > Thanks. Looking in hwif.c, that means dwmac1000_dma_ops are used.
> > 
> > dwmac1000_dma_operation_mode_rx() just does a check:
> > 
> > 	if (rxfifosz < 4096) {
> > 		csr6 &= ~DMA_CONTROL_EFC;
> > 
> > but otherwise does not use the value.
> > 
> > dwmac1000_dma_operation_mode_tx() appears to completely ignore fifosz.
> > 
> > So i would say all zero is valid for has_gmac == true, but you might
> > gain flow control if a value was passed.
> > 
> > A quick look at dwmac100_dma_operation_mode_tx() suggests fifosz is
> > also ignored, and dwmac100_dma_operation_mode_rx() does not exist. So
> > all 0 is also valid for gmac == false, gmac4 == false, and xgmac ==
> > false.
> > 
> > dwmac4_dma_rx_chan_op_mode() does use the value to determine mtl_rx_op
> > which gets written to a register. So gmac4 == true looks to need
> > values. dwxgmac2_dma_rx_mode() is the same, so xgmac = true also need
> > valid values.
> > 
> > Could you cook up a fix based on this?
> 
> The below works for me, I haven't got the hardware to actually test the 
> gmac4/xgmac paths:
> 
> ----8<----
> > From 1ff2f1d5c35d95b38cdec02a283b039befdff0a4 Mon Sep 17 00:00:00 2001
> From: Steven Price <steven.price@....com>
> Date: Fri, 31 Jan 2025 15:45:50 +0000
> Subject: [PATCH] net: stmmac: Allow zero for [tr]x_fifo_size
> 
> Commit 8865d22656b4 ("net: stmmac: Specify hardware capability value
> when FIFO size isn't specified") modified the behaviour to bail out if
> both the FIFO size and the hardware capability were both set to zero.
> However devices where has_gmac4 and has_xgmac are both false don't use
> the fifo size and that commit breaks platforms for which these values
> were zero.
> 
> Only warn and error out when (has_gmac4 || has_xgmac) where the values
> are used and zero would cause problems, otherwise continue with the zero
> values.
> 
> Fixes: 8865d22656b4 ("net: stmmac: Specify hardware capability value when FIFO size isn't specified")
> Signed-off-by: Steven Price <steven.price@....com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index d04543e5697b..821404beb629 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -7222,7 +7222,7 @@ static int stmmac_hw_init(struct stmmac_priv *priv)
>  	if (!priv->plat->rx_fifo_size) {
>  		if (priv->dma_cap.rx_fifo_size) {
>  			priv->plat->rx_fifo_size = priv->dma_cap.rx_fifo_size;
> -		} else {
> +		} else if (priv->plat->has_gmac4 || priv->plat->has_xgmac) {
>  			dev_err(priv->device, "Can't specify Rx FIFO size\n");
>  			return -ENODEV;
>  		}
> @@ -7236,7 +7236,7 @@ static int stmmac_hw_init(struct stmmac_priv *priv)
>  	if (!priv->plat->tx_fifo_size) {
>  		if (priv->dma_cap.tx_fifo_size) {
>  			priv->plat->tx_fifo_size = priv->dma_cap.tx_fifo_size;
> -		} else {
> +		} else if (priv->plat->has_gmac4 || priv->plat->has_xgmac) {
>  			dev_err(priv->device, "Can't specify Tx FIFO size\n");
>  			return -ENODEV;
>  		}

Works for me on TH1520 (arch/riscv/boot/dts/thead/th1520.dtsi).

Tested-by: Xi Ruoyao <xry111@...111.site>

-- 
Xi Ruoyao <xry111@...111.site>
School of Aerospace Science and Technology, Xidian University