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

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

[Huacai Chen]

Now for dwmac-loongson {tx,rx}_fifo_size are uninitialised, which means
zero. This means dwmac-loongson doesn't support changing MTU, 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.

Cc: stable@vger.kernel.org
Fixes: ad72f783de06827a1f ("net: stmmac: Add multi-channel support")
Signed-off-by: Chong Qiao <qiaochong@loongson.cn>
Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
index bfe6e2d631bd..79acdf38c525 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
@@ -574,6 +574,9 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
 	if (ret)
 		goto err_disable_device;
 
+	plat->tx_fifo_size = SZ_16K * plat->tx_queues_to_use;
+	plat->rx_fifo_size = SZ_16K * plat->rx_queues_to_use;
+
 	if (dev_of_node(&pdev->dev))
 		ret = loongson_dwmac_dt_config(pdev, plat, &res);
 	else
-- 
2.47.1

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

[Simon Horman]

+ Feiyang Chen, Yanteng Si, Alexandre Torgue, Maxime Coquelin, Serge Semin,
  linux-arm-kernel

On Tue, Jan 21, 2025 at 05:37:03PM +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, 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.
> 
> Cc: stable@vger.kernel.org
> Fixes: ad72f783de06827a1f ("net: stmmac: Add multi-channel support")
> Signed-off-by: Chong Qiao <qiaochong@loongson.cn>
> Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>

Thanks, this change looks good to me.
And I agree that MTU setting cannot succeed without it.

Reviewed-by: Simon Horman <horms@kernel.org>

Some process notes regarding Networking patches to keep
in mind for next time.

1. Please set the target tree. In this case, as this is a fix
   for code present in net. In general, otherwise it would be net-next.

   Subject: [PATCH net] ...

2. Please generate a CC list using

   ./scripts/get_maintainer.pl this.patch

   The b4 tool can help with this.

Link: https://docs.kernel.org/process/maintainer-netdev.html

> ---
>  drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> index bfe6e2d631bd..79acdf38c525 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> @@ -574,6 +574,9 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
>  	if (ret)
>  		goto err_disable_device;
>  
> +	plat->tx_fifo_size = SZ_16K * plat->tx_queues_to_use;
> +	plat->rx_fifo_size = SZ_16K * plat->rx_queues_to_use;
> +
>  	if (dev_of_node(&pdev->dev))
>  		ret = loongson_dwmac_dt_config(pdev, plat, &res);
>  	else
> -- 
> 2.47.1
> 
> 

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

[Yanteng Si]

在 1/21/25 17:37, Huacai Chen 写道:
> Now for dwmac-loongson {tx,rx}_fifo_size are uninitialised, which means
> zero. This means dwmac-loongson doesn't support changing MTU, 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.
> 
> Cc: stable@vger.kernel.org
> Fixes: ad72f783de06827a1f ("net: stmmac: Add multi-channel support")
> Signed-off-by: Chong Qiao <qiaochong@loongson.cn>
> Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
Acked-by: Yanteng Si <si.yanteng@linux.dev>

Thanks,
Yanteng
> ---
>   drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> index bfe6e2d631bd..79acdf38c525 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> @@ -574,6 +574,9 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
>   	if (ret)
>   		goto err_disable_device;
>   
> +	plat->tx_fifo_size = SZ_16K * plat->tx_queues_to_use;
> +	plat->rx_fifo_size = SZ_16K * plat->rx_queues_to_use;
> +
>   	if (dev_of_node(&pdev->dev))
>   		ret = loongson_dwmac_dt_config(pdev, plat, &res);
>   	else

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

[Huacai Chen]

On Wed, Jan 22, 2025 at 9:34 PM Simon Horman <horms@kernel.org> wrote:
>
> + Feiyang Chen, Yanteng Si, Alexandre Torgue, Maxime Coquelin, Serge Semin,
>   linux-arm-kernel
>
> On Tue, Jan 21, 2025 at 05:37:03PM +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, 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.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: ad72f783de06827a1f ("net: stmmac: Add multi-channel support")
> > Signed-off-by: Chong Qiao <qiaochong@loongson.cn>
> > Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
>
> Thanks, this change looks good to me.
> And I agree that MTU setting cannot succeed without it.
>
> Reviewed-by: Simon Horman <horms@kernel.org>
>
> Some process notes regarding Networking patches to keep
> in mind for next time.
>
> 1. Please set the target tree. In this case, as this is a fix
>    for code present in net. In general, otherwise it would be net-next.
>
>    Subject: [PATCH net] ...
>
> 2. Please generate a CC list using
>
>    ./scripts/get_maintainer.pl this.patch
>
>    The b4 tool can help with this.
>
> Link: https://docs.kernel.org/process/maintainer-netdev.html
OK, thanks.

Huacai
>
> > ---
> >  drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> > index bfe6e2d631bd..79acdf38c525 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> > @@ -574,6 +574,9 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
> >       if (ret)
> >               goto err_disable_device;
> >
> > +     plat->tx_fifo_size = SZ_16K * plat->tx_queues_to_use;
> > +     plat->rx_fifo_size = SZ_16K * plat->rx_queues_to_use;
> > +
> >       if (dev_of_node(&pdev->dev))
> >               ret = loongson_dwmac_dt_config(pdev, plat, &res);
> >       else
> > --
> > 2.47.1
> >
> >

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

[Jakub Kicinski]

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?

> 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.

Please use 12 chars of the hash in the tag.

> +	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?
-- 
pw-bot: cr

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@kernel.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