[PATCH net 1/2] net: stmmac: Limit FIFO size by hardware feature value

[PATCH net 1/2] net: stmmac: Limit FIFO size by hardware feature value

[Kunihiko Hayashi]

Tx/Rx FIFO size is specified by the parameter "{tx,rx}-fifo-depth" from
the 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.

Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 7bf275f127c9..2d69c3c4b329 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2375,9 +2375,9 @@ static void stmmac_dma_operation_mode(struct stmmac_priv *priv)
 	u32 chan = 0;
 	u8 qmode = 0;
 
-	if (rxfifosz == 0)
+	if (!rxfifosz || rxfifosz > priv->dma_cap.rx_fifo_size)
 		rxfifosz = priv->dma_cap.rx_fifo_size;
-	if (txfifosz == 0)
+	if (!txfifosz || txfifosz > priv->dma_cap.tx_fifo_size)
 		txfifosz = priv->dma_cap.tx_fifo_size;
 
 	/* Split up the shared Tx/Rx FIFO memory on DW QoS Eth and DW XGMAC */
@@ -2851,9 +2851,9 @@ static void stmmac_set_dma_operation_mode(struct stmmac_priv *priv, u32 txmode,
 	int rxfifosz = priv->plat->rx_fifo_size;
 	int txfifosz = priv->plat->tx_fifo_size;
 
-	if (rxfifosz == 0)
+	if (!rxfifosz || rxfifosz > priv->dma_cap.rx_fifo_size)
 		rxfifosz = priv->dma_cap.rx_fifo_size;
-	if (txfifosz == 0)
+	if (!txfifosz || txfifosz > priv->dma_cap.tx_fifo_size)
 		txfifosz = priv->dma_cap.tx_fifo_size;
 
 	/* Adjust for real per queue fifo size */
-- 
2.25.1

[PATCH net 2/2] net: stmmac: Limit the number of MTL queues to maximum value

[Kunihiko Hayashi]

The number of MTL queues to use is specified by the parameter
"snps,{tx,rx}-queues-to-use" from the platform layer.

However, the maximum number of queues is determined by
the macro MTL_MAX_{TX,RX}_QUEUES. It's appropriate to limit the
values not to exceed the upper limit values.

Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index ad868e8d195d..471eb1a99d90 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -165,6 +165,8 @@ static int stmmac_mtl_setup(struct platform_device *pdev,
 	if (of_property_read_u32(rx_node, "snps,rx-queues-to-use",
 				 &plat->rx_queues_to_use))
 		plat->rx_queues_to_use = 1;
+	if (plat->rx_queues_to_use > MTL_MAX_RX_QUEUES)
+		plat->rx_queues_to_use = MTL_MAX_RX_QUEUES;
 
 	if (of_property_read_bool(rx_node, "snps,rx-sched-sp"))
 		plat->rx_sched_algorithm = MTL_RX_ALGORITHM_SP;
@@ -224,6 +226,8 @@ static int stmmac_mtl_setup(struct platform_device *pdev,
 	if (of_property_read_u32(tx_node, "snps,tx-queues-to-use",
 				 &plat->tx_queues_to_use))
 		plat->tx_queues_to_use = 1;
+	if (plat->tx_queues_to_use > MTL_MAX_TX_QUEUES)
+		plat->tx_queues_to_use = MTL_MAX_TX_QUEUES;
 
 	if (of_property_read_bool(tx_node, "snps,tx-sched-wrr"))
 		plat->tx_sched_algorithm = MTL_TX_ALGORITHM_WRR;
-- 
2.25.1

Re: [PATCH net 2/2] net: stmmac: Limit the number of MTL queues to maximum value

[Furong Xu]

On Thu, 16 Jan 2025 11:08:53 +0900, Kunihiko Hayashi <hayashi.kunihiko@socionext.com> wrote:

> The number of MTL queues to use is specified by the parameter
> "snps,{tx,rx}-queues-to-use" from the platform layer.
> 
> However, the maximum number of queues is determined by
> the macro MTL_MAX_{TX,RX}_QUEUES. It's appropriate to limit the
> values not to exceed the upper limit values.
> 

The Fixes: tag is required too.

> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> index ad868e8d195d..471eb1a99d90 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> @@ -165,6 +165,8 @@ static int stmmac_mtl_setup(struct platform_device *pdev,
>  	if (of_property_read_u32(rx_node, "snps,rx-queues-to-use",
>  				 &plat->rx_queues_to_use))
>  		plat->rx_queues_to_use = 1;
> +	if (plat->rx_queues_to_use > MTL_MAX_RX_QUEUES)
> +		plat->rx_queues_to_use = MTL_MAX_RX_QUEUES;

MTL_MAX_RX_QUEUES, MTL_MAX_TX_QUEUES and STMMAC_CH_MAX are defined to 8,
this is correct for gmac4, but xgmac has 16 channels at most.

Drop these legacy defines and always use
priv->dma_cap.number_rx_queues,
priv->dma_cap.number_tx_queues,
priv->dma_cap.number_tx_channel,
priv->dma_cap.number_rx_channel,
seems like a good option.

>  
>  	if (of_property_read_bool(rx_node, "snps,rx-sched-sp"))
>  		plat->rx_sched_algorithm = MTL_RX_ALGORITHM_SP;
> @@ -224,6 +226,8 @@ static int stmmac_mtl_setup(struct platform_device *pdev,
>  	if (of_property_read_u32(tx_node, "snps,tx-queues-to-use",
>  				 &plat->tx_queues_to_use))
>  		plat->tx_queues_to_use = 1;
> +	if (plat->tx_queues_to_use > MTL_MAX_TX_QUEUES)
> +		plat->tx_queues_to_use = MTL_MAX_TX_QUEUES;
>  
>  	if (of_property_read_bool(tx_node, "snps,tx-sched-wrr"))
>  		plat->tx_sched_algorithm = MTL_TX_ALGORITHM_WRR;

Re: [PATCH net 2/2] net: stmmac: Limit the number of MTL queues to maximum value

[Kunihiko Hayashi]

Hi Furong,

Thank you for your comment.

On 2025/01/16 12:28, Furong Xu wrote:
> On Thu, 16 Jan 2025 11:08:53 +0900, Kunihiko Hayashi
> <hayashi.kunihiko@socionext.com> wrote:
> 
>> The number of MTL queues to use is specified by the parameter
>> "snps,{tx,rx}-queues-to-use" from the platform layer.
>>
>> However, the maximum number of queues is determined by
>> the macro MTL_MAX_{TX,RX}_QUEUES. It's appropriate to limit the
>> values not to exceed the upper limit values.
>>
> 
> The Fixes: tag is required too.

I see. I'll find it.

>> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
>> ---
>>   drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>> index ad868e8d195d..471eb1a99d90 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>> @@ -165,6 +165,8 @@ static int stmmac_mtl_setup(struct platform_device
> *pdev,
>>   	if (of_property_read_u32(rx_node, "snps,rx-queues-to-use",
>>   				 &plat->rx_queues_to_use))
>>   		plat->rx_queues_to_use = 1;
>> +	if (plat->rx_queues_to_use > MTL_MAX_RX_QUEUES)
>> +		plat->rx_queues_to_use = MTL_MAX_RX_QUEUES;
> 
> MTL_MAX_RX_QUEUES, MTL_MAX_TX_QUEUES and STMMAC_CH_MAX are defined to 8,
> this is correct for gmac4, but xgmac has 16 channels at most.

Yes, but these macros are used as the number of elements in some arrays.
(used in common.h and stmmac.h)

We can change these value to the maximum (16), though, the size of some
structures will increase so be careful.

> Drop these legacy defines and always use
> priv->dma_cap.number_rx_queues,
> priv->dma_cap.number_tx_queues,
> priv->dma_cap.number_tx_channel,
> priv->dma_cap.number_rx_channel,
> seems like a good option.

These values can be obtained in stmmac_dvr_probe() and this patch checks
the queue sizes before stmmac_dvr_probe(). I think we need to change the
way how to check them.

And when checking only with dma_cap values, the number of elements in 
the arrays might be exceeded.  It is necessary to take care of both the
arrays and the capabilities.

Thank you,

---
Best Regards
Kunihiko Hayashi

Re: [PATCH net 1/2] net: stmmac: Limit FIFO size by hardware feature value

[Furong Xu]

On Thu, 16 Jan 2025 11:08:52 +0900, Kunihiko Hayashi <hayashi.kunihiko@socionext.com> wrote:

> Tx/Rx FIFO size is specified by the parameter "{tx,rx}-fifo-depth" from
> the 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.
> 

Patch is fine, but the Fixes: tag is required here.

And if you like to group this patch and the another patch into one series,
it is better to add a cover letter.

> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 7bf275f127c9..2d69c3c4b329 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -2375,9 +2375,9 @@ static void stmmac_dma_operation_mode(struct stmmac_priv *priv)
>  	u32 chan = 0;
>  	u8 qmode = 0;
>  
> -	if (rxfifosz == 0)
> +	if (!rxfifosz || rxfifosz > priv->dma_cap.rx_fifo_size)
>  		rxfifosz = priv->dma_cap.rx_fifo_size;
> -	if (txfifosz == 0)
> +	if (!txfifosz || txfifosz > priv->dma_cap.tx_fifo_size)
>  		txfifosz = priv->dma_cap.tx_fifo_size;
>  
>  	/* Split up the shared Tx/Rx FIFO memory on DW QoS Eth and DW XGMAC */
> @@ -2851,9 +2851,9 @@ static void stmmac_set_dma_operation_mode(struct stmmac_priv *priv, u32 txmode,
>  	int rxfifosz = priv->plat->rx_fifo_size;
>  	int txfifosz = priv->plat->tx_fifo_size;
>  
> -	if (rxfifosz == 0)
> +	if (!rxfifosz || rxfifosz > priv->dma_cap.rx_fifo_size)
>  		rxfifosz = priv->dma_cap.rx_fifo_size;
> -	if (txfifosz == 0)
> +	if (!txfifosz || txfifosz > priv->dma_cap.tx_fifo_size)
>  		txfifosz = priv->dma_cap.tx_fifo_size;
>  
>  	/* Adjust for real per queue fifo size */

Re: [PATCH net 1/2] net: stmmac: Limit FIFO size by hardware feature value

[Kunihiko Hayashi]

Hi Furong,

Thank you for your comment.

On 2025/01/16 12:04, Furong Xu wrote:
> On Thu, 16 Jan 2025 11:08:52 +0900, Kunihiko Hayashi
> <hayashi.kunihiko@socionext.com> wrote:
> 
>> Tx/Rx FIFO size is specified by the parameter "{tx,rx}-fifo-depth" from
>> the 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.
>>
> 
> Patch is fine, but the Fixes: tag is required here.

I see. I'll find original commit and send the patch with Fixes:
tag next.

> And if you like to group this patch and the another patch into one series,
> it is better to add a cover letter.

Yes, I omitted to add a cover letter.
However, this patch has no dependency on the other one and
needs some consideration, so I'll send it separately.

Thank you,

---
Best Regards
Kunihiko Hayashi

Re: [PATCH net 1/2] net: stmmac: Limit FIFO size by hardware feature value

[Andrew Lunn]

On Thu, Jan 16, 2025 at 11:08:52AM +0900, Kunihiko Hayashi wrote:
> Tx/Rx FIFO size is specified by the parameter "{tx,rx}-fifo-depth" from
> the 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.

Are these values hard coded in the platform layer? Or can they come
from userspace?

If they are hard coded, we should also fix them. So maybe add a
netdev_warn(), and encourage the platform maintainers to fix their
platform. If they are coming from userspace, we should consider
failing the ethtool call with an -EINVAL, and maybe an extack with the
valid range?

	Andrew

Re: [PATCH net 1/2] net: stmmac: Limit FIFO size by hardware feature value

[Kunihiko Hayashi]

Hi Andrew,

On 2025/01/17 5:16, Andrew Lunn wrote:
> On Thu, Jan 16, 2025 at 11:08:52AM +0900, Kunihiko Hayashi wrote:
>> Tx/Rx FIFO size is specified by the parameter "{tx,rx}-fifo-depth" from
>> the 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.
> 
> Are these values hard coded in the platform layer? Or can they come
> from userspace?

My explanation is insufficient and misleading.
"From the platform layer" means the common layer of stmmac described in
"stmmac_platform.c".

> If they are hard coded, we should also fix them. So maybe add a
> netdev_warn(), and encourage the platform maintainers to fix their
> platform. If they are coming from userspace, we should consider
> failing the ethtool call with an -EINVAL, and maybe an extack with the
> valid range?

These values are derived from the devicetree and stored in the stmmac
private structure. They are hardware-specific values, so I think this
fix is sufficient.

Thank you,

---
Best Regards
Kunihiko Hayashi

Re: [PATCH net 1/2] net: stmmac: Limit FIFO size by hardware feature value

[Andrew Lunn]

On Mon, Jan 20, 2025 at 02:20:23PM +0900, Kunihiko Hayashi wrote:
> Hi Andrew,
> 
> On 2025/01/17 5:16, Andrew Lunn wrote:
> > On Thu, Jan 16, 2025 at 11:08:52AM +0900, Kunihiko Hayashi wrote:
> > > Tx/Rx FIFO size is specified by the parameter "{tx,rx}-fifo-depth" from
> > > the 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.
> > 
> > Are these values hard coded in the platform layer? Or can they come
> > from userspace?
> 
> My explanation is insufficient and misleading.
> "From the platform layer" means the common layer of stmmac described in
> "stmmac_platform.c".
> 
> > If they are hard coded, we should also fix them. So maybe add a
> > netdev_warn(), and encourage the platform maintainers to fix their
> > platform. If they are coming from userspace, we should consider
> > failing the ethtool call with an -EINVAL, and maybe an extack with the
> > valid range?
> 
> These values are derived from the devicetree and stored in the stmmac
> private structure. They are hardware-specific values, so I think this
> fix is sufficient.

But if they are coming from device tree, the device tree developer has
made an error, which has been silently ignored. Do we want to leave
the device tree broken? Or should we encourage developers to fix them?
Printing a warning would facilitate that.

	Andrew

Re: [PATCH net 1/2] net: stmmac: Limit FIFO size by hardware feature value

[Kunihiko Hayashi]

Hi Andrew,

On 2025/01/21 1:29, Andrew Lunn wrote:
> On Mon, Jan 20, 2025 at 02:20:23PM +0900, Kunihiko Hayashi wrote:
>> Hi Andrew,
>>
>> On 2025/01/17 5:16, Andrew Lunn wrote:
>>> On Thu, Jan 16, 2025 at 11:08:52AM +0900, Kunihiko Hayashi wrote:
>>>> Tx/Rx FIFO size is specified by the parameter "{tx,rx}-fifo-depth"
> from
>>>> the 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.
>>>
>>> Are these values hard coded in the platform layer? Or can they come
>>> from userspace?
>>
>> My explanation is insufficient and misleading.
>> "From the platform layer" means the common layer of stmmac described in
>> "stmmac_platform.c".
>>
>>> If they are hard coded, we should also fix them. So maybe add a
>>> netdev_warn(), and encourage the platform maintainers to fix their
>>> platform. If they are coming from userspace, we should consider
>>> failing the ethtool call with an -EINVAL, and maybe an extack with the
>>> valid range?
>>
>> These values are derived from the devicetree and stored in the stmmac
>> private structure. They are hardware-specific values, so I think this
>> fix is sufficient.
> 
> But if they are coming from device tree, the device tree developer has
> made an error, which has been silently ignored. Do we want to leave
> the device tree broken? Or should we encourage developers to fix them?
> Printing a warning would facilitate that.

I think that developers should fix the devicetree, so I'll add a warning
message if the specified value exceeds the hardware capability.

Thank you,

---
Best Regards
Kunihiko Hayashi