* Re: [PATCH] net: stmmac: dwmac-loongson: Set correct {tx,rx}_fifo_size
2025-01-21 9:37 [PATCH] net: stmmac: dwmac-loongson: Set correct {tx,rx}_fifo_size Huacai Chen
@ 2025-01-22 13:34 ` Simon Horman
2025-01-24 9:09 ` Huacai Chen
2025-01-22 14:49 ` Yanteng Si
2025-01-27 22:06 ` Jakub Kicinski
2 siblings, 1 reply; 6+ messages in thread
From: Simon Horman @ 2025-01-22 13:34 UTC (permalink / raw)
To: Huacai Chen
Cc: Huacai Chen, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, loongarch, netdev, linux-kernel,
stable, Chong Qiao, Feiyang Chen, Yanteng Si, Alexandre Torgue,
Maxime Coquelin, Serge Semin, linux-stm32, linux-arm-kernel
+ 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
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] net: stmmac: dwmac-loongson: Set correct {tx,rx}_fifo_size
2025-01-22 13:34 ` Simon Horman
@ 2025-01-24 9:09 ` Huacai Chen
0 siblings, 0 replies; 6+ messages in thread
From: Huacai Chen @ 2025-01-24 9:09 UTC (permalink / raw)
To: Simon Horman
Cc: Huacai Chen, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, loongarch, netdev, linux-kernel,
stable, Chong Qiao, Feiyang Chen, Yanteng Si, Alexandre Torgue,
Maxime Coquelin, Serge Semin, linux-stm32, linux-arm-kernel
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
> >
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: stmmac: dwmac-loongson: Set correct {tx,rx}_fifo_size
2025-01-21 9:37 [PATCH] net: stmmac: dwmac-loongson: Set correct {tx,rx}_fifo_size Huacai Chen
2025-01-22 13:34 ` Simon Horman
@ 2025-01-22 14:49 ` Yanteng Si
2025-01-27 22:06 ` Jakub Kicinski
2 siblings, 0 replies; 6+ messages in thread
From: Yanteng Si @ 2025-01-22 14:49 UTC (permalink / raw)
To: Huacai Chen, Huacai Chen, Andrew Lunn, David S . Miller,
Eric Dumazet
Cc: Jakub Kicinski, Paolo Abeni, loongarch, netdev, linux-kernel,
stable, Chong Qiao, linux-arm-kernel, fancer.lancer,
Alexandre Torgue, Maxime Coquelin
在 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] net: stmmac: dwmac-loongson: Set correct {tx,rx}_fifo_size
2025-01-21 9:37 [PATCH] net: stmmac: dwmac-loongson: Set correct {tx,rx}_fifo_size Huacai Chen
2025-01-22 13:34 ` Simon Horman
2025-01-22 14:49 ` Yanteng Si
@ 2025-01-27 22:06 ` Jakub Kicinski
2025-02-06 7:52 ` Huacai Chen
2 siblings, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2025-01-27 22:06 UTC (permalink / raw)
To: Huacai Chen
Cc: Huacai Chen, Andrew Lunn, David S . Miller, Eric Dumazet,
Paolo Abeni, loongarch, netdev, linux-kernel, stable, Chong Qiao
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] net: stmmac: dwmac-loongson: Set correct {tx,rx}_fifo_size
2025-01-27 22:06 ` Jakub Kicinski
@ 2025-02-06 7:52 ` Huacai Chen
0 siblings, 0 replies; 6+ messages in thread
From: Huacai Chen @ 2025-02-06 7:52 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Huacai Chen, Andrew Lunn, David S . Miller, Eric Dumazet,
Paolo Abeni, loongarch, netdev, linux-kernel, stable, Chong Qiao
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
^ permalink raw reply [flat|nested] 6+ messages in thread