linux-kernel - Re: [PATCH] net: stmmac: dwmac-loongson: Set correct {tx,rx}_fifo

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: <CAAhV-H6oGvixPkPJ+fr8edxAu8rgjPwgaDh508cSzdAFG8Hrqg@mail.gmail.com>
Date: Thu, 6 Feb 2025 15:52:21 +0800
From: Huacai Chen <chenhuacai@...nel.org>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Huacai Chen <chenhuacai@...ngson.cn>, Andrew Lunn <andrew+netdev@...n.ch>, 
	"David S . Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Paolo Abeni <pabeni@...hat.com>, loongarch@...ts.linux.dev, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org, stable@...r.kernel.org, 
	Chong Qiao <qiaochong@...ngson.cn>
Subject: Re: [PATCH] net: stmmac: dwmac-loongson: Set correct {tx,rx}_fifo_size

Hi, Jakub,

Sorry for the late reply.

On Tue, Jan 28, 2025 at 6:06 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Tue, 21 Jan 2025 17:37:03 +0800 Huacai Chen wrote:
> > Now for dwmac-loongson {tx,rx}_fifo_size are uninitialised, which means
> > zero. This means dwmac-loongson doesn't support changing MTU,
>
> Please provide more details here than "doesn't support changing".
> Does it return an error every time, but the device is operating
> correctly?
>
> Do the flow control thresholds also get programmed incorrectly?
OK, I will give more details, the root cause is stmmac_change_mtu()
fails if txfifosz is zero.

>
> > so set the
> > correct tx_fifo_size and rx_fifo_size for it (16KB multiplied by channel
> > counts).
> >
> > Note: the Fixes tag is not exactly right, but it is a key commit of the
> > dwmac-loongson series.
>
> Please pick a better one, then. Oldest commit where issue can be
> observed by the user is usually a safe choice.
Emmm, the commit I picked is the oldest in the "stmmac: Add Loongson
platform support".

>
> Please use 12 chars of the hash in the tag.
OK, will do.

>
> > +     plat->tx_fifo_size = SZ_16K * plat->tx_queues_to_use;
> > +     plat->rx_fifo_size = SZ_16K * plat->rx_queues_to_use;
>
> Is this really right? 16k times the number of queues seems like you're
> just trying to get the main driver to calculate 16k.
> What if user decreases the queue count? Maybe you should add a way to
> communicate the fifo size regardless of the queue count to the main
 > driver?
Here {tx,rx}_fifo_size is initialised before stmmac_dvr_probe(), so it
uses the maximum {tx,rx}_queues_to_use to calculate.

If we use ethtool to decrease queue count, stmmac_change_mtu() will
get txfifosz larger than 16K, but stmmac_change_mtu() can still work
because there is a condition "if ((txfifosz < new_mtu) || (new_mtu >
BUF_SIZE_16KiB))".

This is not perfect, but it seems a perfect solution needs to rework
the main driver, and dwmac-intel also uses {tx,rx}_queues_to_use to
calculate {tx,rx}_fifo_size now.


Huacai

> --
> pw-bot: cr