| Message ID | 20240615103038.973-2-paul.barker.ct@bp.renesas.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fix maximum TX/RX frame sizes in ravb driver | expand |
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 >
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
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
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 :(
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,
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,
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;
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(-)