[PATCH net-next v2 0/5] net: ethernet: cortina: TSO and pause param

[PATCH net-next v2 0/5] net: ethernet: cortina: TSO and pause param

[Linus Walleij]

This restores the TSO support as we put it on the back
burner a while back. This version has been thoroughly
tested with iperf3 to make sure it really works.

Also included is a patch that implements setting the
RX or TX pause parameters.

To: Hans Ulli Kroll <ulli.kroll@googlemail.com>
To: David S. Miller <davem@davemloft.net>
To: Eric Dumazet <edumazet@google.com>
To: Jakub Kicinski <kuba@kernel.org>
To: Paolo Abeni <pabeni@redhat.com>
To: Andrew Lunn <andrew@lunn.ch>
Cc: netdev@vger.kernel.org
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Changes in v2:
- Fix up the issue in the previous version where I packed the
  TSO segments into too small packets: the TSO hardware
  expects the "MTU" to be set to the length of the resulting
  ethernet frame for each segment.
- Add a patch to make use of the TSO also when we do not have
  any explicit segmenting, which is what the vendor driver
  was always doing. This works fine with even frames with custom
  DSA tags.
- Add a new patch to rename the gmac_adjust_link callback to
  a recognized name.
- Add a new patch to make the driver use the autonegitiated
  RX and TX pause settings from phylib.
- Rewrite the set_pauseparam() patch to use the existing
  gmac_set_flow_control() function.
- Add a call to phy_set_asym_pause() in the set_pauseparam
  callback, so the phylib is informed of the new TX/RX setting.
- Link to v1: https://lore.kernel.org/r/20240509-gemini-ethernet-fix-tso-v1-0-10cd07b54d1c@linaro.org

---
Linus Walleij (5):
      net: ethernet: cortina: Restore TSO support
      net: ethernet: cortina: Use TSO also on common TCP
      net: ethernet: cortina: Rename adjust link callback
      net: ethernet: cortina: Use negotiated TX/RX pause
      net: ethernet: cortina: Implement .set_pauseparam()

 drivers/net/ethernet/cortina/gemini.c | 88 +++++++++++++++++++++++++----------
 1 file changed, 63 insertions(+), 25 deletions(-)
---
base-commit: 8eed0f55df3577f279dfa680f3c7f600072abc45
change-id: 20240506-gemini-ethernet-fix-tso-ebc71477c2fb

Best regards,
-- 
Linus Walleij <linus.walleij@linaro.org>

[PATCH net-next v2 1/5] net: ethernet: cortina: Restore TSO support

[Linus Walleij]

An earlier commit deleted the TSO support in the Cortina Gemini
driver because the driver was confusing gso_size and MTU,
probably because what the Linux kernel calls "gso_size" was
called "MTU" in the datasheet.

Restore the functionality properly reading the gso_size from
the skbuff.

Tested with iperf3, running a server on a different machine
and client on the device with the cortina gemini ethernet:

Connecting to host 192.168.1.2, port 5201
60008000.ethernet-port eth0: segment offloading mss = 05ea len=1c8a
60008000.ethernet-port eth0: segment offloading mss = 05ea len=1c8a
60008000.ethernet-port eth0: segment offloading mss = 05ea len=27da
60008000.ethernet-port eth0: segment offloading mss = 05ea len=0b92
60008000.ethernet-port eth0: segment offloading mss = 05ea len=2bda
(...)

(The hardware MSS 0x05ea here includes the ethernet headers.)

If I disable all segment offloading on the receiving host and
dump packets using tcpdump -xx like this:

ethtool -K enp2s0 gro off gso off tso off
tcpdump -xx -i enp2s0 host 192.168.1.136

I get segmented packages such as this when running iperf3:

23:16:54.024139 IP OpenWrt.lan.59168 > Fecusia.targus-getdata1:
Flags [.], seq 1486:2934, ack 1, win 4198,
options [nop,nop,TS val 3886192908 ecr 3601341877], length 1448
0x0000:  fc34 9701 a0c6 14d6 4da8 3c4f 0800 4500
0x0010:  05dc 16a0 4000 4006 9aa1 c0a8 0188 c0a8
0x0020:  0102 e720 1451 ff25 9822 4c52 29cf 8010
0x0030:  1066 ac8c 0000 0101 080a e7a2 990c d6a8
(...)
0x05c0:  5e49 e109 fe8c 4617 5e18 7a82 7eae d647
0x05d0:  e8ee ae64 dc88 c897 3f8a 07a4 3a33 6b1b
0x05e0:  3501 a30f 2758 cc44 4b4a

Several such packets often follow after each other verifying
the segmentation into 0x05a8 (1448) byte packages also on the
reveiving end. As can be seen, the ethernet frames are
0x05ea (1514) in size.

Performance with iperf3 before this patch: ~15.5 Mbit/s
Performance with iperf3 after this patch: ~175 Mbit/s

This was running a 60 second test (twice) the best measurement
was 179 Mbit/s.

For comparison if I run iperf3 with UDP I get around 1.05 Mbit/s
both before and after this patch.

While this is a gigabit ethernet interface, the CPU is a cheap
D-Link DIR-685 router (based on the ARMv5 Faraday FA526 at
~50 MHz), and the software is not supposed to drive traffic,
as the device has a DSA chip, so this kind of numbers can be
expected.

Fixes: ac631873c9e7 ("net: ethernet: cortina: Drop TSO support")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/net/ethernet/cortina/gemini.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index f9523022be9b..b2ac9dfe1aae 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -79,7 +79,8 @@ MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
 #define GMAC0_IRQ4_8 (GMAC0_MIB_INT_BIT | GMAC0_RX_OVERRUN_INT_BIT)
 
 #define GMAC_OFFLOAD_FEATURES (NETIF_F_SG | NETIF_F_IP_CSUM | \
-			       NETIF_F_IPV6_CSUM | NETIF_F_RXCSUM)
+			       NETIF_F_IPV6_CSUM | NETIF_F_RXCSUM | \
+			       NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6)
 
 /**
  * struct gmac_queue_page - page buffer per-page info
@@ -1148,13 +1149,25 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb,
 	skb_frag_t *skb_frag;
 	dma_addr_t mapping;
 	void *buffer;
+	u16 mss;
 	int ret;
 
-	/* TODO: implement proper TSO using MTU in word3 */
 	word1 = skb->len;
 	word3 = SOF_BIT;
 
-	if (skb->len >= ETH_FRAME_LEN) {
+	mss = skb_shinfo(skb)->gso_size;
+	if (mss) {
+		/* This means we are dealing with TCP and skb->len is the
+		 * sum total of all the segments. The TSO will deal with
+		 * chopping this up for us.
+		 */
+		/* The accelerator needs the full frame size here */
+		mss += skb_tcp_all_headers(skb);
+		netdev_dbg(netdev, "segment offloading mss = %04x len=%04x\n",
+			   mss, skb->len);
+		word1 |= TSS_MTU_ENABLE_BIT;
+		word3 |= mss;
+	} else if (skb->len >= ETH_FRAME_LEN) {
 		/* Hardware offloaded checksumming isn't working on frames
 		 * bigger than 1514 bytes. A hypothesis about this is that the
 		 * checksum buffer is only 1518 bytes, so when the frames get
@@ -1169,7 +1182,9 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb,
 				return ret;
 		}
 		word1 |= TSS_BYPASS_BIT;
-	} else if (skb->ip_summed == CHECKSUM_PARTIAL) {
+	}
+
+	if (skb->ip_summed == CHECKSUM_PARTIAL) {
 		int tcp = 0;
 
 		/* We do not switch off the checksumming on non TCP/UDP

-- 
2.45.0

[PATCH net-next v2 2/5] net: ethernet: cortina: Use TSO also on common TCP

[Linus Walleij]

It is possible to push the segment offloader to also
process non-segmented frames: just pass the skb->len
or desired MSS to the offloader and it will handle them.

This is especially good if the user sets up the MTU
and the frames get big, because the checksumming engine
cannot handle any frames bigger than 1518 bytes, so
segmenting them all to be at max that will be helpful
for the hardware, which only need to quirk odd frames
such as big UDP ping packets.

The vendor driver always uses the TSO like this, and
the driver seems more stable after this, so apparently
the hardware may have been engineered to always use
the TSO on anything it can handle.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/net/ethernet/cortina/gemini.c | 31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index b2ac9dfe1aae..3ba579550cdd 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -1148,6 +1148,7 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb,
 	struct gmac_txdesc *txd;
 	skb_frag_t *skb_frag;
 	dma_addr_t mapping;
+	bool tcp = false;
 	void *buffer;
 	u16 mss;
 	int ret;
@@ -1155,6 +1156,13 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb,
 	word1 = skb->len;
 	word3 = SOF_BIT;
 
+	/* Determine if we are doing TCP */
+	if (skb->protocol == htons(ETH_P_IP))
+		tcp = (ip_hdr(skb)->protocol == IPPROTO_TCP);
+	else
+		/* IPv6 */
+		tcp = (ipv6_hdr(skb)->nexthdr == IPPROTO_TCP);
+
 	mss = skb_shinfo(skb)->gso_size;
 	if (mss) {
 		/* This means we are dealing with TCP and skb->len is the
@@ -1167,6 +1175,20 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb,
 			   mss, skb->len);
 		word1 |= TSS_MTU_ENABLE_BIT;
 		word3 |= mss;
+	} else if (tcp) {
+		/* Even if we are not using TSO, use the segment offloader
+		 * for transferring the TCP frame: the TSO engine will deal
+		 * with chopping up frames that exceed ETH_DATA_LEN which
+		 * the checksumming engine cannot handle (see below) into
+		 * manageable chunks. It flawlessly deals with quite big
+		 * frames and frames containing custom DSA EtherTypes.
+		 */
+		mss = netdev->mtu + skb_tcp_all_headers(skb);
+		mss = min(mss, skb->len);
+		netdev_dbg(netdev, "botched TSO len %04x mtu %04x mss %04x\n",
+			   skb->len, netdev->mtu, mss);
+		word1 |= TSS_MTU_ENABLE_BIT;
+		word3 |= mss;
 	} else if (skb->len >= ETH_FRAME_LEN) {
 		/* Hardware offloaded checksumming isn't working on frames
 		 * bigger than 1514 bytes. A hypothesis about this is that the
@@ -1185,21 +1207,16 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb,
 	}
 
 	if (skb->ip_summed == CHECKSUM_PARTIAL) {
-		int tcp = 0;
-
 		/* We do not switch off the checksumming on non TCP/UDP
 		 * frames: as is shown from tests, the checksumming engine
 		 * is smart enough to see that a frame is not actually TCP
 		 * or UDP and then just pass it through without any changes
 		 * to the frame.
 		 */
-		if (skb->protocol == htons(ETH_P_IP)) {
+		if (skb->protocol == htons(ETH_P_IP))
 			word1 |= TSS_IP_CHKSUM_BIT;
-			tcp = ip_hdr(skb)->protocol == IPPROTO_TCP;
-		} else { /* IPv6 */
+		else
 			word1 |= TSS_IPV6_ENABLE_BIT;
-			tcp = ipv6_hdr(skb)->nexthdr == IPPROTO_TCP;
-		}
 
 		word1 |= tcp ? TSS_TCP_CHKSUM_BIT : TSS_UDP_CHKSUM_BIT;
 	}

-- 
2.45.0

[PATCH net-next v2 3/5] net: ethernet: cortina: Rename adjust link callback

[Linus Walleij]

The callback passed to of_phy_get_and_connect() in the
Cortina Gemini driver is called "gmac_speed_set" which is
archaic, rename it to "gmac_adjust_link" following the
pattern of most other drivers.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/net/ethernet/cortina/gemini.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 3ba579550cdd..e9b4946ec45f 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -288,7 +288,7 @@ static void gmac_set_flow_control(struct net_device *netdev, bool tx, bool rx)
 	spin_unlock_irqrestore(&port->config_lock, flags);
 }
 
-static void gmac_speed_set(struct net_device *netdev)
+static void gmac_adjust_link(struct net_device *netdev)
 {
 	struct gemini_ethernet_port *port = netdev_priv(netdev);
 	struct phy_device *phydev = netdev->phydev;
@@ -367,7 +367,7 @@ static int gmac_setup_phy(struct net_device *netdev)
 
 	phy = of_phy_get_and_connect(netdev,
 				     dev->of_node,
-				     gmac_speed_set);
+				     gmac_adjust_link);
 	if (!phy)
 		return -ENODEV;
 	netdev->phydev = phy;

-- 
2.45.0

[PATCH net-next v2 4/5] net: ethernet: cortina: Use negotiated TX/RX pause

[Linus Walleij]

Instead of directly poking into registers of the PHY, use
the existing function to query phylib about this directly.

Suggested-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/net/ethernet/cortina/gemini.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index e9b4946ec45f..d3134db032a2 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -293,8 +293,8 @@ static void gmac_adjust_link(struct net_device *netdev)
 	struct gemini_ethernet_port *port = netdev_priv(netdev);
 	struct phy_device *phydev = netdev->phydev;
 	union gmac_status status, old_status;
-	int pause_tx = 0;
-	int pause_rx = 0;
+	bool pause_tx = false;
+	bool pause_rx = false;
 
 	status.bits32 = readl(port->gmac_base + GMAC_STATUS);
 	old_status.bits32 = status.bits32;
@@ -328,19 +328,13 @@ static void gmac_adjust_link(struct net_device *netdev)
 			    phydev->speed, phydev_name(phydev));
 	}
 
-	if (phydev->duplex == DUPLEX_FULL) {
-		u16 lcladv = phy_read(phydev, MII_ADVERTISE);
-		u16 rmtadv = phy_read(phydev, MII_LPA);
-		u8 cap = mii_resolve_flowctrl_fdx(lcladv, rmtadv);
-
-		if (cap & FLOW_CTRL_RX)
-			pause_rx = 1;
-		if (cap & FLOW_CTRL_TX)
-			pause_tx = 1;
+	if (phydev->autoneg) {
+		phy_get_pause(phydev, &pause_tx, &pause_rx);
+		netdev_dbg(netdev, "set negotiated pause params pause TX = %s, pause RX = %s\n",
+			   pause_tx ? "ON" : "OFF", pause_rx ? "ON" : "OFF");
+		gmac_set_flow_control(netdev, pause_tx, pause_rx);
 	}
 
-	gmac_set_flow_control(netdev, pause_tx, pause_rx);
-
 	if (old_status.bits32 == status.bits32)
 		return;
 

-- 
2.45.0

[PATCH net-next v2 5/5] net: ethernet: cortina: Implement .set_pauseparam()

[Linus Walleij]

The Cortina Gemini ethernet can very well set up TX or RX
pausing, so add this functionality to the driver in a
.set_pauseparam() callback.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/net/ethernet/cortina/gemini.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index d3134db032a2..137242a4977c 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -2145,6 +2145,17 @@ static void gmac_get_pauseparam(struct net_device *netdev,
 	pparam->autoneg = true;
 }
 
+static int gmac_set_pauseparam(struct net_device *netdev,
+			       struct ethtool_pauseparam *pparam)
+{
+	struct phy_device *phydev = netdev->phydev;
+
+	gmac_set_flow_control(netdev, pparam->tx_pause, pparam->rx_pause);
+	phy_set_asym_pause(phydev, pparam->rx_pause, pparam->tx_pause);
+
+	return 0;
+}
+
 static void gmac_get_ringparam(struct net_device *netdev,
 			       struct ethtool_ringparam *rp,
 			       struct kernel_ethtool_ringparam *kernel_rp,
@@ -2265,6 +2276,7 @@ static const struct ethtool_ops gmac_351x_ethtool_ops = {
 	.set_link_ksettings = gmac_set_ksettings,
 	.nway_reset	= gmac_nway_reset,
 	.get_pauseparam	= gmac_get_pauseparam,
+	.set_pauseparam = gmac_set_pauseparam,
 	.get_ringparam	= gmac_get_ringparam,
 	.set_ringparam	= gmac_set_ringparam,
 	.get_coalesce	= gmac_get_coalesce,

-- 
2.45.0

Re: [PATCH net-next v2 5/5] net: ethernet: cortina: Implement .set_pauseparam()

[Andrew Lunn]

On Sat, May 11, 2024 at 12:08:43AM +0200, Linus Walleij wrote:
> The Cortina Gemini ethernet can very well set up TX or RX
> pausing, so add this functionality to the driver in a
> .set_pauseparam() callback.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/net/ethernet/cortina/gemini.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index d3134db032a2..137242a4977c 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -2145,6 +2145,17 @@ static void gmac_get_pauseparam(struct net_device *netdev,
>  	pparam->autoneg = true;
>  }
>  
> +static int gmac_set_pauseparam(struct net_device *netdev,
> +			       struct ethtool_pauseparam *pparam)
> +{
> +	struct phy_device *phydev = netdev->phydev;
> +
> +	gmac_set_flow_control(netdev, pparam->tx_pause, pparam->rx_pause);
> +	phy_set_asym_pause(phydev, pparam->rx_pause, pparam->tx_pause);

This is a bit of an odd implementation. Normally the value of the
pparam.autoneg is used to decide if to directly program the hardware,
or to pass it to phylib.

Also, the adjust link callback probably should know so that it does
not take the autoneg values when auto-neg of pause is disabled.  Or,
as in your first version, don't allow pparam.autoneg == False.

    Andrew

Re: [PATCH net-next v2 3/5] net: ethernet: cortina: Rename adjust link callback

[Andrew Lunn]

On Sat, May 11, 2024 at 12:08:41AM +0200, Linus Walleij wrote:
> The callback passed to of_phy_get_and_connect() in the
> Cortina Gemini driver is called "gmac_speed_set" which is
> archaic, rename it to "gmac_adjust_link" following the
> pattern of most other drivers.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v2 4/5] net: ethernet: cortina: Use negotiated TX/RX pause

[Andrew Lunn]

On Sat, May 11, 2024 at 12:08:42AM +0200, Linus Walleij wrote:
> Instead of directly poking into registers of the PHY, use
> the existing function to query phylib about this directly.
> 
> Suggested-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/net/ethernet/cortina/gemini.c | 20 +++++++-------------
>  1 file changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index e9b4946ec45f..d3134db032a2 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -293,8 +293,8 @@ static void gmac_adjust_link(struct net_device *netdev)
>  	struct gemini_ethernet_port *port = netdev_priv(netdev);
>  	struct phy_device *phydev = netdev->phydev;
>  	union gmac_status status, old_status;
> -	int pause_tx = 0;
> -	int pause_rx = 0;
> +	bool pause_tx = false;
> +	bool pause_rx = false;
>  
>  	status.bits32 = readl(port->gmac_base + GMAC_STATUS);
>  	old_status.bits32 = status.bits32;
> @@ -328,19 +328,13 @@ static void gmac_adjust_link(struct net_device *netdev)
>  			    phydev->speed, phydev_name(phydev));
>  	}
>  
> -	if (phydev->duplex == DUPLEX_FULL) {
> -		u16 lcladv = phy_read(phydev, MII_ADVERTISE);
> -		u16 rmtadv = phy_read(phydev, MII_LPA);
> -		u8 cap = mii_resolve_flowctrl_fdx(lcladv, rmtadv);
> -
> -		if (cap & FLOW_CTRL_RX)
> -			pause_rx = 1;
> -		if (cap & FLOW_CTRL_TX)
> -			pause_tx = 1;
> +	if (phydev->autoneg) {
> +		phy_get_pause(phydev, &pause_tx, &pause_rx);
> +		netdev_dbg(netdev, "set negotiated pause params pause TX = %s, pause RX = %s\n",
> +			   pause_tx ? "ON" : "OFF", pause_rx ? "ON" : "OFF");
> +		gmac_set_flow_control(netdev, pause_tx, pause_rx);
>  	}

This is a lot better, but not quite correct. The pause kAPi is more
complex than it probably needs to be...

phydev->autoneg is about overall autoneg, basically speed and duplex
negotiation. However:

 * If @autoneg is non-zero, the MAC is configured to send and/or
 * receive pause frames according to the result of autonegotiation.
 * Otherwise, it is configured directly based on the @rx_pause and
 * @tx_pause flags.
 */
struct ethtool_pauseparam {
	__u32	cmd;
	__u32	autoneg;
	__u32	rx_pause;
	__u32	tx_pause;
};

So if pauseparam.autoneg is false, it does not matter if
phydev->autoneg is true, you should not be touching the hardware
here. The hardware should of been set as part of ethtool_set_pause().

Or you can simplify this and return -EOPNOTSUPP in ethtool_set_pause()
is pauseparam.autoneg is false.

	Andrew