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

[Huacai Chen]

Hi, Jakub,

Sorry for the late reply.

On Tue, Jan 28, 2025 at 6:06 AM Jakub Kicinski <kuba@xxxxxxxxxx> 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