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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Niklas Söderlund <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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