[net-next PATCH 0/2] Fix maximum TX/RX frame sizes in ravb driver

[net-next PATCH 0/2] Fix maximum TX/RX frame sizes in ravb driver

[Paul Barker]

These patches fix a couple of bugs in the maximum supported TX/RX frame sizes
in the ravb driver.

  * For the GbEth IP, we were advertising a maximum TX frame size/MTU that was
    larger that the maximum the hardware can transmit.

  * For the R-Car AVB IP, we were unnecessarily setting the maximum RX frame
    size/MRU based on the MTU, which by default is smaller than the maximum the
    hardware can receive.

Paul Barker (2):
  net: ravb: Fix maximum MTU for GbEth devices
  net: ravb: Set R-Car RX frame size limit correctly

 drivers/net/ethernet/renesas/ravb.h      |  1 +
 drivers/net/ethernet/renesas/ravb_main.c | 10 ++++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)


base-commit: 934c29999b57b835d65442da6f741d5e27f3b584
-- 
2.39.2

[net-next PATCH 1/2] net: ravb: Fix maximum MTU for GbEth devices

[Paul Barker]

The datasheets for all SoCs using the GbEth IP specify a maximum
transmission frame size of 1.5 kByte. I've confirmed through internal
discussions that support for 1522 byte frames has been validated, which
allows us to support the default MTU of 1500 bytes after reserving space
for the Ethernet header, frame checksums and an optional VLAN tag.

Fixes: 2e95e08ac009 ("ravb: Add rx_max_buf_size to struct ravb_hw_info")
Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb.h      | 1 +
 drivers/net/ethernet/renesas/ravb_main.c | 6 +++++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 6b2444d31fcc..e592e89b0d96 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -1051,6 +1051,7 @@ struct ravb_hw_info {
 	netdev_features_t net_features;
 	int stats_len;
 	u32 tccr_mask;
+	u32 tx_max_frame_size;
 	u32 rx_max_frame_size;
 	u32 rx_buffer_size;
 	u32 rx_desc_size;
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index c1546b916e4e..02cbf850bd85 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2664,6 +2664,7 @@ static const struct ravb_hw_info ravb_gen3_hw_info = {
 	.net_features = NETIF_F_RXCSUM,
 	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),
 	.tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
+	.tx_max_frame_size = SZ_2K,
 	.rx_max_frame_size = SZ_2K,
 	.rx_buffer_size = SZ_2K +
 			  SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
@@ -2689,6 +2690,7 @@ static const struct ravb_hw_info ravb_gen2_hw_info = {
 	.net_features = NETIF_F_RXCSUM,
 	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),
 	.tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
+	.tx_max_frame_size = SZ_2K,
 	.rx_max_frame_size = SZ_2K,
 	.rx_buffer_size = SZ_2K +
 			  SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
@@ -2711,6 +2713,7 @@ static const struct ravb_hw_info ravb_rzv2m_hw_info = {
 	.net_features = NETIF_F_RXCSUM,
 	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),
 	.tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
+	.tx_max_frame_size = SZ_2K,
 	.rx_max_frame_size = SZ_2K,
 	.rx_buffer_size = SZ_2K +
 			  SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
@@ -2735,6 +2738,7 @@ static const struct ravb_hw_info gbeth_hw_info = {
 	.net_features = NETIF_F_RXCSUM | NETIF_F_HW_CSUM,
 	.stats_len = ARRAY_SIZE(ravb_gstrings_stats_gbeth),
 	.tccr_mask = TCCR_TSRQ0,
+	.tx_max_frame_size = 1522,
 	.rx_max_frame_size = SZ_8K,
 	.rx_buffer_size = SZ_2K,
 	.rx_desc_size = sizeof(struct ravb_rx_desc),
@@ -2946,7 +2950,7 @@ static int ravb_probe(struct platform_device *pdev)
 	priv->avb_link_active_low =
 		of_property_read_bool(np, "renesas,ether-link-active-low");
 
-	ndev->max_mtu = info->rx_max_frame_size -
+	ndev->max_mtu = info->tx_max_frame_size -
 		(ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN);
 	ndev->min_mtu = ETH_MIN_MTU;
 
-- 
2.39.2

Re: [net-next PATCH 1/2] net: ravb: Fix maximum MTU for GbEth devices

[Niklas Söderlund]

Hi Paul,

Thanks for your work.

On 2024-06-15 11:30:37 +0100, Paul Barker wrote:
> The datasheets for all SoCs using the GbEth IP specify a maximum
> transmission frame size of 1.5 kByte. I've confirmed through internal
> discussions that support for 1522 byte frames has been validated, which
> allows us to support the default MTU of 1500 bytes after reserving space
> for the Ethernet header, frame checksums and an optional VLAN tag.
> 
> Fixes: 2e95e08ac009 ("ravb: Add rx_max_buf_size to struct ravb_hw_info")
> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> ---
>  drivers/net/ethernet/renesas/ravb.h      | 1 +
>  drivers/net/ethernet/renesas/ravb_main.c | 6 +++++-
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index 6b2444d31fcc..e592e89b0d96 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -1051,6 +1051,7 @@ struct ravb_hw_info {
>  	netdev_features_t net_features;
>  	int stats_len;
>  	u32 tccr_mask;
> +	u32 tx_max_frame_size;
>  	u32 rx_max_frame_size;
>  	u32 rx_buffer_size;
>  	u32 rx_desc_size;
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index c1546b916e4e..02cbf850bd85 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -2664,6 +2664,7 @@ static const struct ravb_hw_info ravb_gen3_hw_info = {
>  	.net_features = NETIF_F_RXCSUM,
>  	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),
>  	.tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
> +	.tx_max_frame_size = SZ_2K,
>  	.rx_max_frame_size = SZ_2K,
>  	.rx_buffer_size = SZ_2K +
>  			  SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
> @@ -2689,6 +2690,7 @@ static const struct ravb_hw_info ravb_gen2_hw_info = {
>  	.net_features = NETIF_F_RXCSUM,
>  	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),
>  	.tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
> +	.tx_max_frame_size = SZ_2K,
>  	.rx_max_frame_size = SZ_2K,
>  	.rx_buffer_size = SZ_2K +
>  			  SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
> @@ -2711,6 +2713,7 @@ static const struct ravb_hw_info ravb_rzv2m_hw_info = {
>  	.net_features = NETIF_F_RXCSUM,
>  	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),
>  	.tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
> +	.tx_max_frame_size = SZ_2K,
>  	.rx_max_frame_size = SZ_2K,
>  	.rx_buffer_size = SZ_2K +
>  			  SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
> @@ -2735,6 +2738,7 @@ static const struct ravb_hw_info gbeth_hw_info = {
>  	.net_features = NETIF_F_RXCSUM | NETIF_F_HW_CSUM,
>  	.stats_len = ARRAY_SIZE(ravb_gstrings_stats_gbeth),
>  	.tccr_mask = TCCR_TSRQ0,
> +	.tx_max_frame_size = 1522,
>  	.rx_max_frame_size = SZ_8K,
>  	.rx_buffer_size = SZ_2K,
>  	.rx_desc_size = sizeof(struct ravb_rx_desc),
> @@ -2946,7 +2950,7 @@ static int ravb_probe(struct platform_device *pdev)
>  	priv->avb_link_active_low =
>  		of_property_read_bool(np, "renesas,ether-link-active-low");
>  
> -	ndev->max_mtu = info->rx_max_frame_size -
> +	ndev->max_mtu = info->tx_max_frame_size -
>  		(ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN);
>  	ndev->min_mtu = ETH_MIN_MTU;
>  
> -- 
> 2.39.2
> 

-- 
Kind Regards,
Niklas Söderlund

Re: [net-next PATCH 1/2] net: ravb: Fix maximum MTU for GbEth devices

[Sergey Shtylyov]

On 6/15/24 1:30 PM, Paul Barker wrote:

> The datasheets for all SoCs using the GbEth IP specify a maximum
> transmission frame size of 1.5 kByte. I've confirmed through internal
> discussions that support for 1522 byte frames has been validated, which
> allows us to support the default MTU of 1500 bytes after reserving space
> for the Ethernet header, frame checksums and an optional VLAN tag.
> 
> Fixes: 2e95e08ac009 ("ravb: Add rx_max_buf_size to struct ravb_hw_info")
> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
[...]

Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index c1546b916e4e..02cbf850bd85 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -2664,6 +2664,7 @@ static const struct ravb_hw_info ravb_gen3_hw_info = {
>  	.net_features = NETIF_F_RXCSUM,
>  	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),
>  	.tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
> +	.tx_max_frame_size = SZ_2K,

   The R-Car gen3 manual says 2047... Typo? :-)

[...]

MBR, Sergey

Re: [net-next PATCH 1/2] net: ravb: Fix maximum MTU for GbEth devices

[Sergey Shtylyov]

On 6/17/24 10:38 PM, Sergey Shtylyov wrote:
[...]

>> The datasheets for all SoCs using the GbEth IP specify a maximum
>> transmission frame size of 1.5 kByte. I've confirmed through internal
>> discussions that support for 1522 byte frames has been validated, which
>> allows us to support the default MTU of 1500 bytes after reserving space
>> for the Ethernet header, frame checksums and an optional VLAN tag.
>>
>> Fixes: 2e95e08ac009 ("ravb: Add rx_max_buf_size to struct ravb_hw_info")
>> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
> [...]
> 
> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

   Sounds like this is a also fix for the net.git tho?

[...]

MBR, Sergey

Re: [net-next PATCH 1/2] net: ravb: Fix maximum MTU for GbEth devices

[Paul Barker]

On 17/06/2024 20:38, Sergey Shtylyov wrote:
> On 6/15/24 1:30 PM, Paul Barker wrote:
> 
>> The datasheets for all SoCs using the GbEth IP specify a maximum
>> transmission frame size of 1.5 kByte. I've confirmed through internal
>> discussions that support for 1522 byte frames has been validated, which
>> allows us to support the default MTU of 1500 bytes after reserving space
>> for the Ethernet header, frame checksums and an optional VLAN tag.
>>
>> Fixes: 2e95e08ac009 ("ravb: Add rx_max_buf_size to struct ravb_hw_info")
>> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
> [...]
> 
> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> 
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index c1546b916e4e..02cbf850bd85 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -2664,6 +2664,7 @@ static const struct ravb_hw_info ravb_gen3_hw_info = {
>>  	.net_features = NETIF_F_RXCSUM,
>>  	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),
>>  	.tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
>> +	.tx_max_frame_size = SZ_2K,
> 
>    The R-Car gen3 manual says 2047... Typo? :-)

Apologies for the late response.

The maximum MTU is currently based on rx_max_frame_size which is SZ_2K
for the R-Car gen3 devices. So this should be addressed in two commits:

* This commit to address the GbEth MTU, leaving the R-Car gen3 MTU
  incorrect.

* A second commit to address the R-Car gen3 MTU.

Unfortunately I have no test environment where I can properly
investigate jumbo packet behaviour with a R-Car gen3 boards so I
recommend the second commit is sent by someone who can test it fully.

Thanks,

-- 
Paul Barker

Re: [net-next PATCH 1/2] net: ravb: Fix maximum MTU for GbEth devices

[Jakub Kicinski]

On Sat, 15 Jun 2024 11:30:37 +0100 Paul Barker wrote:
> The datasheets for all SoCs using the GbEth IP specify a maximum
> transmission frame size of 1.5 kByte. I've confirmed through internal
> discussions that support for 1522 byte frames has been validated, which
> allows us to support the default MTU of 1500 bytes after reserving space
> for the Ethernet header, frame checksums and an optional VLAN tag.


But what's the user impact? If we send a bigger frame the IP will hang?
Drop the packet? Something else?
"Validated" can also mean "officially supported" sometimes vendors just
say that to narrow down the test matrix :(

Re: [net-next PATCH 1/2] net: ravb: Fix maximum MTU for GbEth devices

[Paul Barker]

On 18/06/2024 01:07, Jakub Kicinski wrote:
> On Sat, 15 Jun 2024 11:30:37 +0100 Paul Barker wrote:
>> The datasheets for all SoCs using the GbEth IP specify a maximum
>> transmission frame size of 1.5 kByte. I've confirmed through internal
>> discussions that support for 1522 byte frames has been validated, which
>> allows us to support the default MTU of 1500 bytes after reserving space
>> for the Ethernet header, frame checksums and an optional VLAN tag.
> 
> 
> But what's the user impact? If we send a bigger frame the IP will hang?
> Drop the packet? Something else?
> "Validated" can also mean "officially supported" sometimes vendors just
> say that to narrow down the test matrix :(

Apologies for the late response.

I was able to hang the GbEth IP by attempting to transmit a 2kB frame,
no error condition was raised by the hardware but no further packets
could be sent or received. I was able to successfully send a 1.8kB
frame, but there is no guarantee that this will always work. The RZ/G2L
datasheet states that frames of up to 1.5kB can be transmitted so I
clarified with the HW team that 1522 bytes was ok to allow for the
default MTU plus headers.

I'm going to submit v2 of this series as bugfixes against net (instead
of net-next) shortly.

Thanks,

-- 
Paul Barker

[net-next PATCH 2/2] net: ravb: Fix R-Car RX frame size limit

[Paul Barker]

The RX frame size limit should not be based on the current MTU setting.
Instead it should be based on the hardware capabilities.

Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 02cbf850bd85..481c854cb305 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -555,8 +555,10 @@ static void ravb_emac_init_gbeth(struct net_device *ndev)
 
 static void ravb_emac_init_rcar(struct net_device *ndev)
 {
+	struct ravb_private *priv = netdev_priv(ndev);
+
 	/* Receive frame limit set register */
-	ravb_write(ndev, ndev->mtu + ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN, RFLR);
+	ravb_write(ndev, priv->info->rx_max_frame_size + ETH_FCS_LEN, RFLR);
 
 	/* EMAC Mode: PAUSE prohibition; Duplex; RX Checksum; TX; RX */
 	ravb_write(ndev, ECMR_ZPF | ECMR_DM |
-- 
2.39.2

Re: [net-next PATCH 2/2] net: ravb: Fix R-Car RX frame size limit

[Niklas Söderlund]

Hi Paul,

Thanks for your work.

On 2024-06-15 11:30:38 +0100, Paul Barker wrote:
> The RX frame size limit should not be based on the current MTU setting.
> Instead it should be based on the hardware capabilities.
> 
> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")

I agree with the change the RFLR.RFL setting should not be connected to 
the MTU setting. And this likely comes from the early days of the driver 
where neither Rx or Tx supported multiple descriptors for each packet.  
In those days the single descriptor used for each packet was tied to the 
MTU setting. So likely the fixes tag should point to a later commit?

This is a great find and shows a flaw in the driver. But limiting the 
size of each descriptor used for Tx is only half the solution right? As 
the driver now supports multiple descriptors for Tx (on GbEth) the 
driver allows setting an MTU larger then the maximum size for single 
descriptor on those devices. But the xmit function of the driver still 
hardcode the maximum of 2 descriptors for each Tx packet. And it only 
uses the two descriptors to align the data to hardware constrains.

Is it not incorrect for the driver to accept an MTU larger then the 
maximum size of a single descriptor with the current Tx implementation?  
The driver can only support larger MTU settings for Rx, but not Tx ATM.

I think the complete fix is to extend ravb_start_xmit() to fully support 
split descriptors for packets larger then the maximum single descriptor 
size. Not just to align the packet between a DT_FSTART and DT_FEND 
descriptor when needed.

> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 02cbf850bd85..481c854cb305 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -555,8 +555,10 @@ static void ravb_emac_init_gbeth(struct net_device *ndev)
>  
>  static void ravb_emac_init_rcar(struct net_device *ndev)
>  {
> +	struct ravb_private *priv = netdev_priv(ndev);
> +
>  	/* Receive frame limit set register */
> -	ravb_write(ndev, ndev->mtu + ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN, RFLR);
> +	ravb_write(ndev, priv->info->rx_max_frame_size + ETH_FCS_LEN, RFLR);
>  
>  	/* EMAC Mode: PAUSE prohibition; Duplex; RX Checksum; TX; RX */
>  	ravb_write(ndev, ECMR_ZPF | ECMR_DM |
> -- 
> 2.39.2
> 

-- 
Kind Regards,
Niklas Söderlund

Re: [net-next PATCH 2/2] net: ravb: Fix R-Car RX frame size limit

[Paul Barker]

[-- Attachment #1.1.1: Type: text/plain, Size: 2274 bytes --]

On 15/06/2024 14:04, Niklas Söderlund wrote:
> Hi Paul,
> 
> Thanks for your work.
> 
> On 2024-06-15 11:30:38 +0100, Paul Barker wrote:
>> The RX frame size limit should not be based on the current MTU setting.
>> Instead it should be based on the hardware capabilities.
>>
>> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
> 
> I agree with the change the RFLR.RFL setting should not be connected to 
> the MTU setting. And this likely comes from the early days of the driver 
> where neither Rx or Tx supported multiple descriptors for each packet.  
> In those days the single descriptor used for each packet was tied to the 
> MTU setting. So likely the fixes tag should point to a later commit?> 

If my understanding of MTU & MRU are correct, even with a single
descriptor we can always accept the same number of bytes regardless of
the current MTU.

> This is a great find and shows a flaw in the driver. But limiting the 
> size of each descriptor used for Tx is only half the solution right? As 
> the driver now supports multiple descriptors for Tx (on GbEth) the 
> driver allows setting an MTU larger then the maximum size for single 
> descriptor on those devices. But the xmit function of the driver still 
> hardcode the maximum of 2 descriptors for each Tx packet. And it only 
> uses the two descriptors to align the data to hardware constrains.
> 
> Is it not incorrect for the driver to accept an MTU larger then the 
> maximum size of a single descriptor with the current Tx implementation?  
> The driver can only support larger MTU settings for Rx, but not Tx ATM.
> 
> I think the complete fix is to extend ravb_start_xmit() to fully support 
> split descriptors for packets larger then the maximum single descriptor 
> size. Not just to align the packet between a DT_FSTART and DT_FEND 
> descriptor when needed.

For the RZ SoCs I have looked at, this isn't an issue. We support
transmitting frames of slightly over 1500 bytes on the SoCs with GbEth
IP, or 2047 bytes on the SoCs with R-Car AVB IP (e.g. RZ/G2H). Given
that a single descriptor can cover up to 4095 bytes of data (with its 12
bit DS field), we don't need split TX descriptors for either of these.

Thanks,

-- 
Paul Barker

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3577 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [net-next PATCH 2/2] net: ravb: Fix R-Car RX frame size limit

[Andrew Lunn]

On Sat, Jun 15, 2024 at 11:30:38AM +0100, Paul Barker wrote:
> The RX frame size limit should not be based on the current MTU setting.
> Instead it should be based on the hardware capabilities.

This is a bit odd. MTU is Maximum Transmission Unit, so clearly is
about Tx. MRU does not really exist. Does TCP allow for asymmetric
MTU/MRU? Does MTU discovery work correctly for this?

In general, it seems like drivers implement min(MTU, MRU) and nothing
more. Do you have a real use case for this asymmetry?

      Andrew

Re: [net-next PATCH 2/2] net: ravb: Fix R-Car RX frame size limit

[Paul Barker]

[-- Attachment #1.1.1: Type: text/plain, Size: 1868 bytes --]

On 16/06/2024 02:23, Andrew Lunn wrote:
> On Sat, Jun 15, 2024 at 11:30:38AM +0100, Paul Barker wrote:
>> The RX frame size limit should not be based on the current MTU setting.
>> Instead it should be based on the hardware capabilities.
> 
> This is a bit odd. MTU is Maximum Transmission Unit, so clearly is
> about Tx. MRU does not really exist. Does TCP allow for asymmetric
> MTU/MRU? Does MTU discovery work correctly for this?
> 
> In general, it seems like drivers implement min(MTU, MRU) and nothing
> more. Do you have a real use case for this asymmetry?
> 
>       Andrew

Hi Andrew,

This change is based on my understanding of MTU/MRU, on the specs of the
RZ SoCs I'm working with (primarily RZ/G2L family, RZ/G3S and RZ/G2H)
and on some testing. My goal here is just to make the capabilities of
the hardware available to users.

For the RZ/G2L family and RZ/G3S, we can only support an MTU of up to
1500 bytes, but we can receive frames of up to (IIRC) 8192 bytes. I have
tested sending jumbo frames to an RZ/G2L device using both iperf3 and
ping and I see no errors.

* For iperf3 RX testing, the RZ/G2L is only responding with acks. These
  are small regardless of the size of the received packets, so the
  mis-match in MTU between the two hosts causes no issue.

* For ping testing, the RZ/G2L will give a fragmented response to the
  ping packet which the other host can reassemble.

For the RZ/G2H, we support sending frames of up to 2047 bytes but we can
receive frames of up to 4092 bytes. The driver will need a few more
changes to handle reception of packets >2kB in size, but this is
something we can do in the near future.

Is there any reason why we shouldn't support this? I am by no means an
expert in the Linux networking internals so there may be things I'm
missing.

Thanks,

-- 
Paul Barker

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3577 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [net-next PATCH 2/2] net: ravb: Fix R-Car RX frame size limit

[Andrew Lunn]

On Mon, Jun 17, 2024 at 03:03:21PM +0100, Paul Barker wrote:
> On 16/06/2024 02:23, Andrew Lunn wrote:
> > On Sat, Jun 15, 2024 at 11:30:38AM +0100, Paul Barker wrote:
> >> The RX frame size limit should not be based on the current MTU setting.
> >> Instead it should be based on the hardware capabilities.
> > 
> > This is a bit odd. MTU is Maximum Transmission Unit, so clearly is
> > about Tx. MRU does not really exist. Does TCP allow for asymmetric
> > MTU/MRU? Does MTU discovery work correctly for this?
> > 
> > In general, it seems like drivers implement min(MTU, MRU) and nothing
> > more. Do you have a real use case for this asymmetry?
> > 
> >       Andrew
> 
> Hi Andrew,
> 
> This change is based on my understanding of MTU/MRU, on the specs of the
> RZ SoCs I'm working with (primarily RZ/G2L family, RZ/G3S and RZ/G2H)
> and on some testing. My goal here is just to make the capabilities of
> the hardware available to users.
> 
> For the RZ/G2L family and RZ/G3S, we can only support an MTU of up to
> 1500 bytes, but we can receive frames of up to (IIRC) 8192 bytes. I have
> tested sending jumbo frames to an RZ/G2L device using both iperf3 and
> ping and I see no errors.
> 
> * For iperf3 RX testing, the RZ/G2L is only responding with acks. These
>   are small regardless of the size of the received packets, so the
>   mis-match in MTU between the two hosts causes no issue.
> 
> * For ping testing, the RZ/G2L will give a fragmented response to the
>   ping packet which the other host can reassemble.
> 
> For the RZ/G2H, we support sending frames of up to 2047 bytes but we can
> receive frames of up to 4092 bytes. The driver will need a few more
> changes to handle reception of packets >2kB in size, but this is
> something we can do in the near future.
> 
> Is there any reason why we shouldn't support this? I am by no means an
> expert in the Linux networking internals so there may be things I'm
> missing.

I've no really experience what the Linux stack will do. I have seen a
number of devices which do not limit received packets to the MTU,
i.e. they are happy to receive bigger packets. And i don't think this
has caused an issue. But is it ever actually used? Do users setup
asymmetric jumbo systems? I've no idea.

One thing i suggest you test is you put this in the middle of two
systems which do support jumbo.

     4K       1.5K       4K
A <-----> B <-----> C <------> D.

Make your device B and C, setting the MTU as supported. But set A and
D with a jumbo MTU which your systems should be able to receive. Do IP
routing along the chain. And then do some iperf transfers. I would
test both IPv4 and IPv6, since MTU path discovery in IPv6 is used a
lot, and gateways are not supposed to fragment.

I'm assuming here your device does actually have two interfaces?  If
it is only ever an end system, that simplifies it a lot, and these
tests are not needed.

    Andrew

Re: [net-next PATCH 2/2] net: ravb: Fix R-Car RX frame size limit

[Paul Barker]

On 17/06/2024 15:29, Andrew Lunn wrote:
> On Mon, Jun 17, 2024 at 03:03:21PM +0100, Paul Barker wrote:
>> On 16/06/2024 02:23, Andrew Lunn wrote:
>>> On Sat, Jun 15, 2024 at 11:30:38AM +0100, Paul Barker wrote:
>>>> The RX frame size limit should not be based on the current MTU setting.
>>>> Instead it should be based on the hardware capabilities.
>>>
>>> This is a bit odd. MTU is Maximum Transmission Unit, so clearly is
>>> about Tx. MRU does not really exist. Does TCP allow for asymmetric
>>> MTU/MRU? Does MTU discovery work correctly for this?
>>>
>>> In general, it seems like drivers implement min(MTU, MRU) and nothing
>>> more. Do you have a real use case for this asymmetry?
>>>
>>>       Andrew
>>
>> Hi Andrew,
>>
>> This change is based on my understanding of MTU/MRU, on the specs of the
>> RZ SoCs I'm working with (primarily RZ/G2L family, RZ/G3S and RZ/G2H)
>> and on some testing. My goal here is just to make the capabilities of
>> the hardware available to users.
>>
>> For the RZ/G2L family and RZ/G3S, we can only support an MTU of up to
>> 1500 bytes, but we can receive frames of up to (IIRC) 8192 bytes. I have
>> tested sending jumbo frames to an RZ/G2L device using both iperf3 and
>> ping and I see no errors.
>>
>> * For iperf3 RX testing, the RZ/G2L is only responding with acks. These
>>   are small regardless of the size of the received packets, so the
>>   mis-match in MTU between the two hosts causes no issue.
>>
>> * For ping testing, the RZ/G2L will give a fragmented response to the
>>   ping packet which the other host can reassemble.
>>
>> For the RZ/G2H, we support sending frames of up to 2047 bytes but we can
>> receive frames of up to 4092 bytes. The driver will need a few more
>> changes to handle reception of packets >2kB in size, but this is
>> something we can do in the near future.
>>
>> Is there any reason why we shouldn't support this? I am by no means an
>> expert in the Linux networking internals so there may be things I'm
>> missing.
> 
> I've no really experience what the Linux stack will do. I have seen a
> number of devices which do not limit received packets to the MTU,
> i.e. they are happy to receive bigger packets. And i don't think this
> has caused an issue. But is it ever actually used? Do users setup
> asymmetric jumbo systems? I've no idea.
> 
> One thing i suggest you test is you put this in the middle of two
> systems which do support jumbo.
> 
>      4K       1.5K       4K
> A <-----> B <-----> C <------> D.
> 
> Make your device B and C, setting the MTU as supported. But set A and
> D with a jumbo MTU which your systems should be able to receive. Do IP
> routing along the chain. And then do some iperf transfers. I would
> test both IPv4 and IPv6, since MTU path discovery in IPv6 is used a
> lot, and gateways are not supposed to fragment.
> 
> I'm assuming here your device does actually have two interfaces?  If
> it is only ever an end system, that simplifies it a lot, and these
> tests are not needed.
> 
>     Andrew

Apologies, my response here is abysmally late due to illness, other
priorities and then the loss of my main dev box.

As you've said, a number of devices do not limit received packet size to
the MTU. There are many applications, other than a gateway, where using
jumbo packets in even just one direction would be beneficial. For
example if an application needs to receive large amounts of data but
only needs to send back control and acknowledgement messages. I think we
should support this where possible. This is the thought behind the first
patch in this series as the GbEth IP present in the RZ/G2L and other
Renesas SoCs has a very asymmetric capability (it can receive 8000 byte
frames but only transmit 1522 byte frames).

If we explicitly do not wish to support this, that restriction should be
documented and then (maybe over time) handled uniformly for all network
drivers.

I'm planning to submit v2 of this series shortly.

Thanks,

-- 
Paul Barker

Re: [net-next PATCH 2/2] net: ravb: Fix R-Car RX frame size limit

[Andrew Lunn]

> Apologies, my response here is abysmally late due to illness, other
> priorities and then the loss of my main dev box.

Not a problem, life happens.

> As you've said, a number of devices do not limit received packet size to
> the MTU. There are many applications, other than a gateway, where using
> jumbo packets in even just one direction would be beneficial. For
> example if an application needs to receive large amounts of data but
> only needs to send back control and acknowledgement messages. I think we
> should support this where possible. This is the thought behind the first
> patch in this series as the GbEth IP present in the RZ/G2L and other
> Renesas SoCs has a very asymmetric capability (it can receive 8000 byte
> frames but only transmit 1522 byte frames).
> 
> If we explicitly do not wish to support this, that restriction should be
> documented and then (maybe over time) handled uniformly for all network
> drivers.
> 
> I'm planning to submit v2 of this series shortly.

Does the hardware support scatter/gather? How does supporting jumbo
receive affect memory usage? Can you give the hardware a number of 2K
buffers, and it will use one for a typical packet, and 4 for a jumbo
frame?

	Andrew

Re: [net-next PATCH 2/2] net: ravb: Fix R-Car RX frame size limit

[Paul Barker]

[-- Attachment #1.1.1: Type: text/plain, Size: 1850 bytes --]

On 13/08/2024 15:06, Andrew Lunn wrote:
>> Apologies, my response here is abysmally late due to illness, other
>> priorities and then the loss of my main dev box.
> 
> Not a problem, life happens.
> 
>> As you've said, a number of devices do not limit received packet size to
>> the MTU. There are many applications, other than a gateway, where using
>> jumbo packets in even just one direction would be beneficial. For
>> example if an application needs to receive large amounts of data but
>> only needs to send back control and acknowledgement messages. I think we
>> should support this where possible. This is the thought behind the first
>> patch in this series as the GbEth IP present in the RZ/G2L and other
>> Renesas SoCs has a very asymmetric capability (it can receive 8000 byte
>> frames but only transmit 1522 byte frames).
>>
>> If we explicitly do not wish to support this, that restriction should be
>> documented and then (maybe over time) handled uniformly for all network
>> drivers.
>>
>> I'm planning to submit v2 of this series shortly.
> 
> Does the hardware support scatter/gather? How does supporting jumbo
> receive affect memory usage? Can you give the hardware a number of 2K
> buffers, and it will use one for a typical packet, and 4 for a jumbo
> frame?

This is exactly what happens. After recent changes [1], we use 2kB RX
buffers and an 8kB maximum RX frame size for the GbEth IP. The hardware
will split the received frame over one or more buffers as needed. As we
would allocate a ring of 2kB buffers in any case, supporting jumbo
packets doesn't cause any increase in memory usage or in CPU time spent
in memory management.

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=966726324b7b14009216fda33b47e0bc003944c6

Thanks,

-- 
Paul Barker

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3577 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [net-next PATCH 2/2] net: ravb: Fix R-Car RX frame size limit

[Sergey Shtylyov]

On 6/15/24 1:30 PM, Paul Barker wrote:

> The RX frame size limit should not be based on the current MTU setting.
> Instead it should be based on the hardware capabilities.
> 
> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>

Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

   Sounds like this is also a fix for net.git tho?

[...]

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 02cbf850bd85..481c854cb305 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -555,8 +555,10 @@ static void ravb_emac_init_gbeth(struct net_device *ndev)
>  
>  static void ravb_emac_init_rcar(struct net_device *ndev)
>  {
> +	struct ravb_private *priv = netdev_priv(ndev);
> +
>  	/* Receive frame limit set register */
> -	ravb_write(ndev, ndev->mtu + ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN, RFLR);
> +	ravb_write(ndev, priv->info->rx_max_frame_size + ETH_FCS_LEN, RFLR);

   Aha, that's what we're doing in ravb_emac_init_gbeth()...

[...]

MBR, Sergey

Re: [net-next PATCH 0/2] Fix maximum TX/RX frame sizes in ravb driver

[Sergey Shtylyov]

On 6/15/24 1:30 PM, Paul Barker wrote:

> These patches fix a couple of bugs in the maximum supported TX/RX frame sizes
> in the ravb driver.
> 
>   * For the GbEth IP, we were advertising a maximum TX frame size/MTU that was
>     larger that the maximum the hardware can transmit.
> 
>   * For the R-Car AVB IP, we were unnecessarily setting the maximum RX frame
>     size/MRU based on the MTU, which by default is smaller than the maximum the
>     hardware can receive.
> 
> Paul Barker (2):
>   net: ravb: Fix maximum MTU for GbEth devices
>   net: ravb: Set R-Car RX frame size limit correctly
> 
>  drivers/net/ethernet/renesas/ravb.h      |  1 +
>  drivers/net/ethernet/renesas/ravb_main.c | 10 ++++++++--
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> 
> base-commit: 934c29999b57b835d65442da6f741d5e27f3b584
> 

   DaveM & Co, I'm planning to review these patches on Monday evening...

MBR, Sergey