linux-kernel - Re: [PATCH net v2 2/3] net: stmmac: Limit FIFO size by hardware capability

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: <Z4_ZilVFKacuAUE8@shell.armlinux.org.uk>
Date: Tue, 21 Jan 2025 17:29:46 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Yanteng Si <si.yanteng@...ux.dev>
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>,
	Furong Xu <0x1207@...il.com>, Joao Pinto <Joao.Pinto@...opsys.com>,
	Vince Bridgers <vbridger@...nsource.altera.com>,
	netdev@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net v2 2/3] net: stmmac: Limit FIFO size by hardware
 capability

On Wed, Jan 22, 2025 at 01:14:25AM +0800, Yanteng Si wrote:
> 在 1/21/25 12:41, Kunihiko Hayashi 写道:
> > Tx/Rx FIFO size is specified by the parameter "{tx,rx}-fifo-depth" from
> > stmmac_platform layer.
> > 
> > However, these values are constrained by upper limits determined by the
> > capabilities of each hardware feature. There is a risk that the upper
> > bits will be truncated due to the calculation, so it's appropriate to
> > limit them to the upper limit values and display a warning message.
> > 
> > Fixes: e7877f52fd4a ("stmmac: Read tx-fifo-depth and rx-fifo-depth from the devicetree")
> > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@...ionext.com>
> > ---
> >   drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 13 +++++++++++++
> >   1 file changed, 13 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 251a8c15637f..da3316e3e93b 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -7245,6 +7245,19 @@ static int stmmac_hw_init(struct stmmac_priv *priv)
> >   		priv->plat->tx_queues_to_use = priv->dma_cap.number_tx_queues;
> >   	}
> 
> > +	if (priv->plat->rx_fifo_size > priv->dma_cap.rx_fifo_size) {
> 
> > +		dev_warn(priv->device,
> > +			 "Rx FIFO size exceeds dma capability (%d)\n",
> > +			 priv->plat->rx_fifo_size);
> > +		priv->plat->rx_fifo_size = priv->dma_cap.rx_fifo_size;
> I executed grep and found that only dwmac4 and dwxgmac2 have initialized
> dma_cap.rx_fifo_size. Can this code still work properly on hardware other
> than these two?

Looking at drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c:

        /* Compute minimum number of packets to make FIFO full */
        pkt_count = priv->plat->rx_fifo_size;
        if (!pkt_count)
                pkt_count = priv->dma_cap.rx_fifo_size;

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:

        int rxfifosz = priv->plat->rx_fifo_size;
        int txfifosz = priv->plat->tx_fifo_size;

        if (rxfifosz == 0)
                rxfifosz = priv->dma_cap.rx_fifo_size;
        if (txfifosz == 0)
                txfifosz = priv->dma_cap.tx_fifo_size;

(in two locations)

It looks to me like the intention is that priv->plat->rx_fifo_size is
supposed to _override_ whatever is in priv->dma_cap.rx_fifo_size.

Now looking at the defintions:

drivers/net/ethernet/stmicro/stmmac/dwmac4.h:#define GMAC_HW_RXFIFOSIZE        GENMASK(4, 0)
drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h:#define XGMAC_HWFEAT_RXFIFOSIZE GENMASK(4, 0)

So there's a 5-bit bitfield that describes the receive FIFO size for
these two MACs. Then we have:

drivers/net/ethernet/stmicro/stmmac/common.h:#define DMA_HW_FEAT_RXFIFOSIZE    0x00080000       /* Rx FIFO > 2048 Bytes */

which is used here:

drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c:    dma_cap->rxfifo_over_2048 = (hw_cap & DMA_HW_FEAT_RXFIFOSIZE) >> 19;

which is only used to print a Y/N value in a debugfs file, otherwise
having no bearing on driver behaviour.

So, I suspect MACs other than xgmac2 or dwmac4 do not have the ability
to describe the hardware FIFO sizes in hardware, thus why there's the
override and no checking of what the platform provided - and doing so
would break the driver. This is my interpretation from the code alone.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!