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: <c2aa354d-1bd5-4fb0-a8b8-48fcce3c1628@socionext.com>
Date: Thu, 23 Jan 2025 14:25:19 +0900
From: Kunihiko Hayashi <hayashi.kunihiko@...ionext.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>,
 Yanteng Si <si.yanteng@...ux.dev>
Cc: 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

Hi,

On 2025/01/22 2:29, Russell King (Oracle) wrote:
> 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.

Surely, other MACs than xgmac2 and dwmac4 don't have hardware capability
values, and the variables from the capabilities will have zero.
I can add a check whether a capability value is zero or not like that:

If priv->plat->rx-fifo-size is not specified:

     if (priv->dma_cap.rx_fifo_size)
         priv->plat->rx_fifo_size = priv->dma_cap.rx_fifo_size;
     else
         return; (with an error value and a message)

If priv->plat->rx-fifo-size is specified:

     if (priv->dma_cap.rx_fifo_size &&
         priv->plat->rx_fifo_size > priv->dma_cap.rx_fifo_size)
         priv->plat->rx_fifo_size = priv->dma_cap.rx_fifo_size;

Same as others.

Thank you,

---
Best Regards
Kunihiko Hayashi