mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH net 0/4] Fix large frames in the Gemini ethernet driver
@ 2023-11-04 12:43 Linus Walleij
  2023-11-04 12:43 ` [PATCH net 1/4] net: ethernet: cortina: Fix MTU max setting Linus Walleij
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Linus Walleij @ 2023-11-04 12:43 UTC (permalink / raw)
  To: Hans Ulli Kroll, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Michał Mirosław, Vladimir Oltean
  Cc: linux-arm-kernel, netdev, linux-kernel, Linus Walleij

This is the result of a bug hunt for a problem with the
RTL8366RB DSA switch leading me wrong all over the place.

I am indebted to Vladimir Oltean who as usual pointed
out where the real problem was, many thanks!

Tryig to actually use big ("jumbo") frames on this
hardware uncovered the real bugs. Then I tested it on
the DSA switch and it indeed fixes the issue.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Linus Walleij (4):
      net: ethernet: cortina: Fix MTU max setting
      net: ethernet: cortina: Fix max RX frame define
      net: ethernet: cortina: Protect against oversized frames
      net: ethernet: cortina: Handle large frames

 drivers/net/ethernet/cortina/gemini.c | 32 ++++++++++++++++++++++++++------
 drivers/net/ethernet/cortina/gemini.h | 10 +++++-----
 2 files changed, 31 insertions(+), 11 deletions(-)
---
base-commit: 90b0c2b2edd1adff742c621e246562fbefa11b70
change-id: 20231104-gemini-largeframe-fix-c143d2c781b5

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH net 1/4] net: ethernet: cortina: Fix MTU max setting
  2023-11-04 12:43 [PATCH net 0/4] Fix large frames in the Gemini ethernet driver Linus Walleij
@ 2023-11-04 12:43 ` Linus Walleij
  2023-11-04 14:36   ` Andrew Lunn
  2023-11-04 12:43 ` [PATCH net 2/4] net: ethernet: cortina: Fix max RX frame define Linus Walleij
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 11+ messages in thread
From: Linus Walleij @ 2023-11-04 12:43 UTC (permalink / raw)
  To: Hans Ulli Kroll, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Michał Mirosław, Vladimir Oltean
  Cc: linux-arm-kernel, netdev, linux-kernel, Linus Walleij

The RX max frame size is over 10000 for the Gemini ethernet,
but the TX max frame size is actually just 2047 (0x7ff after
checking the datasheet). Reflect this in what we offer to Linux,
cap the MTU at the TX max frame minus ethernet headers.

Use the BIT() macro for related bit flags so these TX settings
are consistent.

Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/net/ethernet/cortina/gemini.c | 12 +++++++++---
 drivers/net/ethernet/cortina/gemini.h |  8 ++++----
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 5423fe26b4ef..e12d14359133 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -1151,6 +1151,11 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb,
 	if (skb->protocol == htons(ETH_P_8021Q))
 		mtu += VLAN_HLEN;
 
+	if (mtu > MTU_SIZE_BIT_MASK) {
+		netdev_err(netdev, "%s: MTU too big, max size 2047 bytes, capped\n", __func__);
+		mtu = MTU_SIZE_BIT_MASK;
+	}
+
 	word1 = skb->len;
 	word3 = SOF_BIT;
 
@@ -2464,11 +2469,12 @@ static int gemini_ethernet_port_probe(struct platform_device *pdev)
 
 	netdev->hw_features = GMAC_OFFLOAD_FEATURES;
 	netdev->features |= GMAC_OFFLOAD_FEATURES | NETIF_F_GRO;
-	/* We can handle jumbo frames up to 10236 bytes so, let's accept
-	 * payloads of 10236 bytes minus VLAN and ethernet header
+	/* We can receive jumbo frames up to 10236 bytes but only
+	 * transmit 2047 bytes so, let's accept payloads of 2047
+	 * bytes minus VLAN and ethernet header
 	 */
 	netdev->min_mtu = ETH_MIN_MTU;
-	netdev->max_mtu = 10236 - VLAN_ETH_HLEN;
+	netdev->max_mtu = MTU_SIZE_BIT_MASK - VLAN_ETH_HLEN;
 
 	port->freeq_refill = 0;
 	netif_napi_add(netdev, &port->napi, gmac_napi_poll);
diff --git a/drivers/net/ethernet/cortina/gemini.h b/drivers/net/ethernet/cortina/gemini.h
index 9fdf77d5eb37..d7101bfcb4a0 100644
--- a/drivers/net/ethernet/cortina/gemini.h
+++ b/drivers/net/ethernet/cortina/gemini.h
@@ -499,10 +499,10 @@ union gmac_txdesc_3 {
 };
 
 #define SOF_EOF_BIT_MASK	0x3fffffff
-#define SOF_BIT			0x80000000
-#define EOF_BIT			0x40000000
-#define EOFIE_BIT		BIT(29)
-#define MTU_SIZE_BIT_MASK	0x1fff
+#define SOF_BIT			BIT(31) /* Start of Frame */
+#define EOF_BIT			BIT(30) /* End of Frame */
+#define EOFIE_BIT		BIT(29) /* End of Frame Interrupt Enable */
+#define MTU_SIZE_BIT_MASK	0x7ff /* Max MTU 2047 bytes */
 
 /* GMAC Tx Descriptor */
 struct gmac_txdesc {

-- 
2.34.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH net 2/4] net: ethernet: cortina: Fix max RX frame define
  2023-11-04 12:43 [PATCH net 0/4] Fix large frames in the Gemini ethernet driver Linus Walleij
  2023-11-04 12:43 ` [PATCH net 1/4] net: ethernet: cortina: Fix MTU max setting Linus Walleij
@ 2023-11-04 12:43 ` Linus Walleij
  2023-11-04 14:38   ` Andrew Lunn
  2023-11-04 12:43 ` [PATCH net 3/4] net: ethernet: cortina: Protect against oversized frames Linus Walleij
  2023-11-04 12:43 ` [PATCH net 4/4] net: ethernet: cortina: Handle large frames Linus Walleij
  3 siblings, 1 reply; 11+ messages in thread
From: Linus Walleij @ 2023-11-04 12:43 UTC (permalink / raw)
  To: Hans Ulli Kroll, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Michał Mirosław, Vladimir Oltean
  Cc: linux-arm-kernel, netdev, linux-kernel, Linus Walleij

Enumerator 3 is 1548 bytes according to the datasheet.
Not 1542.

Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/net/ethernet/cortina/gemini.c | 4 ++--
 drivers/net/ethernet/cortina/gemini.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index e12d14359133..fd08f098850b 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -432,8 +432,8 @@ static const struct gmac_max_framelen gmac_maxlens[] = {
 		.val = CONFIG0_MAXLEN_1536,
 	},
 	{
-		.max_l3_len = 1542,
-		.val = CONFIG0_MAXLEN_1542,
+		.max_l3_len = 1548,
+		.val = CONFIG0_MAXLEN_1548,
 	},
 	{
 		.max_l3_len = 9212,
diff --git a/drivers/net/ethernet/cortina/gemini.h b/drivers/net/ethernet/cortina/gemini.h
index d7101bfcb4a0..e4b9049b6400 100644
--- a/drivers/net/ethernet/cortina/gemini.h
+++ b/drivers/net/ethernet/cortina/gemini.h
@@ -787,7 +787,7 @@ union gmac_config0 {
 #define  CONFIG0_MAXLEN_1536	0
 #define  CONFIG0_MAXLEN_1518	1
 #define  CONFIG0_MAXLEN_1522	2
-#define  CONFIG0_MAXLEN_1542	3
+#define  CONFIG0_MAXLEN_1548	3
 #define  CONFIG0_MAXLEN_9k	4	/* 9212 */
 #define  CONFIG0_MAXLEN_10k	5	/* 10236 */
 #define  CONFIG0_MAXLEN_1518__6	6

-- 
2.34.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH net 3/4] net: ethernet: cortina: Protect against oversized frames
  2023-11-04 12:43 [PATCH net 0/4] Fix large frames in the Gemini ethernet driver Linus Walleij
  2023-11-04 12:43 ` [PATCH net 1/4] net: ethernet: cortina: Fix MTU max setting Linus Walleij
  2023-11-04 12:43 ` [PATCH net 2/4] net: ethernet: cortina: Fix max RX frame define Linus Walleij
@ 2023-11-04 12:43 ` Linus Walleij
  2023-11-04 14:46   ` Andrew Lunn
  2023-11-04 12:43 ` [PATCH net 4/4] net: ethernet: cortina: Handle large frames Linus Walleij
  3 siblings, 1 reply; 11+ messages in thread
From: Linus Walleij @ 2023-11-04 12:43 UTC (permalink / raw)
  To: Hans Ulli Kroll, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Michał Mirosław, Vladimir Oltean
  Cc: linux-arm-kernel, netdev, linux-kernel, Linus Walleij

The max size of a transfer no matter the MTU is 64KB-1 so immediately
bail out if the skb exceeds that.

Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/net/ethernet/cortina/gemini.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index fd08f098850b..23723c9c0f93 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -1156,6 +1156,12 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb,
 		mtu = MTU_SIZE_BIT_MASK;
 	}
 
+	if (skb->len > 65535) {
+		/* The field for length is only 16 bits */
+		netdev_err(netdev, "%s: frame too big, max size 65535 bytes\n", __func__);
+		return -EINVAL;
+	}
+
 	word1 = skb->len;
 	word3 = SOF_BIT;
 

-- 
2.34.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH net 4/4] net: ethernet: cortina: Handle large frames
  2023-11-04 12:43 [PATCH net 0/4] Fix large frames in the Gemini ethernet driver Linus Walleij
                   ` (2 preceding siblings ...)
  2023-11-04 12:43 ` [PATCH net 3/4] net: ethernet: cortina: Protect against oversized frames Linus Walleij
@ 2023-11-04 12:43 ` Linus Walleij
  2023-11-04 14:56   ` Andrew Lunn
  2023-11-04 15:18   ` kernel test robot
  3 siblings, 2 replies; 11+ messages in thread
From: Linus Walleij @ 2023-11-04 12:43 UTC (permalink / raw)
  To: Hans Ulli Kroll, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Michał Mirosław, Vladimir Oltean
  Cc: linux-arm-kernel, netdev, linux-kernel, Linus Walleij

The Gemini ethernet controller provides hardware checksumming
for frames up to 1514 bytes including ethernet headers but not
FCS.

If we start sending bigger frames (after first bumping up the MTU
on both interfaces sending and receiveing the frames), truncated
packets start to appear on the target such as in this tcpdump
resulting from ping -s 1474:

23:34:17.241983 14:d6:4d:a8:3c:4f (oui Unknown) > bc:ae:c5:6b:a8:3d (oui Unknown),
ethertype IPv4 (0x0800), length 1514: truncated-ip - 2 bytes missing!
(tos 0x0, ttl 64, id 32653, offset 0, flags [DF], proto ICMP (1), length 1502)
OpenWrt.lan > Fecusia: ICMP echo request, id 1672, seq 50, length 1482

If we bypass the hardware checksumming and provide a software
fallback, everything starts working fine up to the max TX MTU
of 2047 bytes, for example ping -s2000 192.168.1.2:

00:44:29.587598 bc:ae:c5:6b:a8:3d (oui Unknown) > 14:d6:4d:a8:3c:4f (oui Unknown),
ethertype IPv4 (0x0800), length 2042:
(tos 0x0, ttl 64, id 51828, offset 0, flags [none], proto ICMP (1), length 2028)
Fecusia > OpenWrt.lan: ICMP echo reply, id 1683, seq 4, length 2008

The bit enabling to bypass hardware checksum (or any of the
"TSS" bits) are undocumented in the hardware reference manual.
The entire hardware checksum unit appears undocumented. The
conclusion that we need to use the "bypass" bit was found by
trial-and-error.

On the D-Link DIR-685 router this fixes a bug on the conduit
interface to the RTL8366RB DSA switch: as the switch needs to add
space for its tag it increases the MTU on the conduit interface
to 1504 and that means that when the router sends packages
of 1500 bytes these get an extra 4 bytes of DSA tag and the
transfer fails because of the erroneous hardware checksumming,
affecting such basic functionality as the LuCI web inteface.

Suggested-by: Vladimir Oltean <olteanv@gmail.com>
Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/net/ethernet/cortina/gemini.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 23723c9c0f93..063e58639379 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -1145,6 +1145,7 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb,
 	dma_addr_t mapping;
 	unsigned short mtu;
 	void *buffer;
+	int ret;
 
 	mtu  = ETH_HLEN;
 	mtu += netdev->mtu;
@@ -1170,7 +1171,14 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb,
 		word3 |= mtu;
 	}
 
-	if (skb->ip_summed != CHECKSUM_NONE) {
+	if (skb->len >= ETH_FRAME_LEN) {
+		/* Hardware offloaded checksumming isn't working on frames
+		 * bigger than 1514 bytes. Perhaps the buffer is only 1518
+		 * bytes fitting mach a normal frame and a checksum?
+		 * Just bypass on bigger frames.
+		 */
+		word1 |= TSS_BYPASS_BIT;
+	} else if (skb->ip_summed != CHECKSUM_NONE) {
 		int tcp = 0;
 
 		if (skb->protocol == htons(ETH_P_IP)) {

-- 
2.34.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net 1/4] net: ethernet: cortina: Fix MTU max setting
  2023-11-04 12:43 ` [PATCH net 1/4] net: ethernet: cortina: Fix MTU max setting Linus Walleij
@ 2023-11-04 14:36   ` Andrew Lunn
  0 siblings, 0 replies; 11+ messages in thread
From: Andrew Lunn @ 2023-11-04 14:36 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Hans Ulli Kroll, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Michał Mirosław, Vladimir Oltean,
	linux-arm-kernel, netdev, linux-kernel

> +	if (mtu > MTU_SIZE_BIT_MASK) {
> +		netdev_err(netdev, "%s: MTU too big, max size 2047 bytes, capped\n", __func__);
> +		mtu = MTU_SIZE_BIT_MASK;
> +	}

So this should not actually happen. If it does, some protocol above is
ignoring the MTU. And if that happens, you are going to spam the log
at line rate.

Packets which are truncated are also pretty useless. They are likely
to be dropped by the receiver when the CRC fails. So i would suggest
drop them here, and increment a counter.

>  #define SOF_EOF_BIT_MASK	0x3fffffff
> -#define SOF_BIT			0x80000000
> -#define EOF_BIT			0x40000000
> -#define EOFIE_BIT		BIT(29)
> -#define MTU_SIZE_BIT_MASK	0x1fff
> +#define SOF_BIT			BIT(31) /* Start of Frame */
> +#define EOF_BIT			BIT(30) /* End of Frame */
> +#define EOFIE_BIT		BIT(29) /* End of Frame Interrupt Enable */
> +#define MTU_SIZE_BIT_MASK	0x7ff /* Max MTU 2047 bytes */

Apart from the MTU_SIZE_BIT_MASK, this looks mostly unrelated.


    Andrew

---
pw-bot: cr

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net 2/4] net: ethernet: cortina: Fix max RX frame define
  2023-11-04 12:43 ` [PATCH net 2/4] net: ethernet: cortina: Fix max RX frame define Linus Walleij
@ 2023-11-04 14:38   ` Andrew Lunn
  0 siblings, 0 replies; 11+ messages in thread
From: Andrew Lunn @ 2023-11-04 14:38 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Hans Ulli Kroll, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Michał Mirosław, Vladimir Oltean,
	linux-arm-kernel, netdev, linux-kernel

On Sat, Nov 04, 2023 at 01:43:49PM +0100, Linus Walleij wrote:
> Enumerator 3 is 1548 bytes according to the datasheet.
> Not 1542.
> 
> Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet")
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

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

    Andrew

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net 3/4] net: ethernet: cortina: Protect against oversized frames
  2023-11-04 12:43 ` [PATCH net 3/4] net: ethernet: cortina: Protect against oversized frames Linus Walleij
@ 2023-11-04 14:46   ` Andrew Lunn
  0 siblings, 0 replies; 11+ messages in thread
From: Andrew Lunn @ 2023-11-04 14:46 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Hans Ulli Kroll, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Michał Mirosław, Vladimir Oltean,
	linux-arm-kernel, netdev, linux-kernel

On Sat, Nov 04, 2023 at 01:43:50PM +0100, Linus Walleij wrote:
> The max size of a transfer no matter the MTU is 64KB-1 so immediately
> bail out if the skb exceeds that.
> 
> Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet")
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/net/ethernet/cortina/gemini.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index fd08f098850b..23723c9c0f93 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -1156,6 +1156,12 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb,
>  		mtu = MTU_SIZE_BIT_MASK;
>  	}
>  
> +	if (skb->len > 65535) {
> +		/* The field for length is only 16 bits */
> +		netdev_err(netdev, "%s: frame too big, max size 65535 bytes\n", __func__);
> +		return -EINVAL;
> +	}
> +

The caller of gmac_map_tx_bufs() is a but funky:

	if (gmac_map_tx_bufs(netdev, skb, txq, &w)) {
		if (skb_linearize(skb))
			goto out_drop;

		u64_stats_update_begin(&port->tx_stats_syncp);
		port->tx_frags_linearized++;
		u64_stats_update_end(&port->tx_stats_syncp);

		if (gmac_map_tx_bufs(netdev, skb, txq, &w))
			goto out_drop_free;
	}

So return -EINVAL is going to cause the skb to be linearised, and then
re-tried. Maybe you want to check the error code here, and go straight
to out_drop?

   Andrew

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net 4/4] net: ethernet: cortina: Handle large frames
  2023-11-04 12:43 ` [PATCH net 4/4] net: ethernet: cortina: Handle large frames Linus Walleij
@ 2023-11-04 14:56   ` Andrew Lunn
  2023-11-05 20:56     ` Linus Walleij
  2023-11-04 15:18   ` kernel test robot
  1 sibling, 1 reply; 11+ messages in thread
From: Andrew Lunn @ 2023-11-04 14:56 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Hans Ulli Kroll, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Michał Mirosław, Vladimir Oltean,
	linux-arm-kernel, netdev, linux-kernel

> @@ -1170,7 +1171,14 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb,
>  		word3 |= mtu;
>  	}
>  
> -	if (skb->ip_summed != CHECKSUM_NONE) {
> +	if (skb->len >= ETH_FRAME_LEN) {
> +		/* Hardware offloaded checksumming isn't working on frames
> +		 * bigger than 1514 bytes. Perhaps the buffer is only 1518
> +		 * bytes fitting mach a normal frame and a checksum?

mach ?

> +		 * Just bypass on bigger frames.
> +		 */
> +		word1 |= TSS_BYPASS_BIT;
> +	} else if (skb->ip_summed != CHECKSUM_NONE) {

I've never looked at how the network stack does checksums. But looking
at this patch, it made me wounder, how do you tell the stack it needs
to do a software checksum because the hardware cannot? Or for this
driver, is it always calculating a checksum, which is then ignored?
Maybe you can improve performance a little but disabling software
checksum when it is not needed?

	 Andrew

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net 4/4] net: ethernet: cortina: Handle large frames
  2023-11-04 12:43 ` [PATCH net 4/4] net: ethernet: cortina: Handle large frames Linus Walleij
  2023-11-04 14:56   ` Andrew Lunn
@ 2023-11-04 15:18   ` kernel test robot
  1 sibling, 0 replies; 11+ messages in thread
From: kernel test robot @ 2023-11-04 15:18 UTC (permalink / raw)
  To: Linus Walleij, Hans Ulli Kroll, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Michał Mirosław,
	Vladimir Oltean
  Cc: oe-kbuild-all, netdev, linux-arm-kernel, linux-kernel, Linus Walleij

Hi Linus,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 90b0c2b2edd1adff742c621e246562fbefa11b70]

url:    https://github.com/intel-lab-lkp/linux/commits/Linus-Walleij/net-ethernet-cortina-Fix-MTU-max-setting/20231104-204432
base:   90b0c2b2edd1adff742c621e246562fbefa11b70
patch link:    https://lore.kernel.org/r/20231104-gemini-largeframe-fix-v1-4-9c5513f22f33%40linaro.org
patch subject: [PATCH net 4/4] net: ethernet: cortina: Handle large frames
config: arc-randconfig-001-20231104 (https://download.01.org/0day-ci/archive/20231104/202311042310.Kl043sv2-lkp@intel.com/config)
compiler: arc-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231104/202311042310.Kl043sv2-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311042310.Kl043sv2-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/net/ethernet/cortina/gemini.c: In function 'gmac_map_tx_bufs':
>> drivers/net/ethernet/cortina/gemini.c:1148:13: warning: unused variable 'ret' [-Wunused-variable]
    1148 |         int ret;
         |             ^~~


vim +/ret +1148 drivers/net/ethernet/cortina/gemini.c

  1132	
  1133	static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb,
  1134				    struct gmac_txq *txq, unsigned short *desc)
  1135	{
  1136		struct gemini_ethernet_port *port = netdev_priv(netdev);
  1137		struct skb_shared_info *skb_si =  skb_shinfo(skb);
  1138		unsigned short m = (1 << port->txq_order) - 1;
  1139		short frag, last_frag = skb_si->nr_frags - 1;
  1140		struct gemini_ethernet *geth = port->geth;
  1141		unsigned int word1, word3, buflen;
  1142		unsigned short w = *desc;
  1143		struct gmac_txdesc *txd;
  1144		skb_frag_t *skb_frag;
  1145		dma_addr_t mapping;
  1146		unsigned short mtu;
  1147		void *buffer;
> 1148		int ret;
  1149	
  1150		mtu  = ETH_HLEN;
  1151		mtu += netdev->mtu;
  1152		if (skb->protocol == htons(ETH_P_8021Q))
  1153			mtu += VLAN_HLEN;
  1154	
  1155		if (mtu > MTU_SIZE_BIT_MASK) {
  1156			netdev_err(netdev, "%s: MTU too big, max size 2047 bytes, capped\n", __func__);
  1157			mtu = MTU_SIZE_BIT_MASK;
  1158		}
  1159	
  1160		if (skb->len > 65535) {
  1161			/* The field for length is only 16 bits */
  1162			netdev_err(netdev, "%s: frame too big, max size 65535 bytes\n", __func__);
  1163			return -EINVAL;
  1164		}
  1165	
  1166		word1 = skb->len;
  1167		word3 = SOF_BIT;
  1168	
  1169		if (word1 > mtu) {
  1170			word1 |= TSS_MTU_ENABLE_BIT;
  1171			word3 |= mtu;
  1172		}
  1173	
  1174		if (skb->len >= ETH_FRAME_LEN) {
  1175			/* Hardware offloaded checksumming isn't working on frames
  1176			 * bigger than 1514 bytes. Perhaps the buffer is only 1518
  1177			 * bytes fitting mach a normal frame and a checksum?
  1178			 * Just bypass on bigger frames.
  1179			 */
  1180			word1 |= TSS_BYPASS_BIT;
  1181		} else if (skb->ip_summed != CHECKSUM_NONE) {
  1182			int tcp = 0;
  1183	
  1184			if (skb->protocol == htons(ETH_P_IP)) {
  1185				word1 |= TSS_IP_CHKSUM_BIT;
  1186				tcp = ip_hdr(skb)->protocol == IPPROTO_TCP;
  1187			} else { /* IPv6 */
  1188				word1 |= TSS_IPV6_ENABLE_BIT;
  1189				tcp = ipv6_hdr(skb)->nexthdr == IPPROTO_TCP;
  1190			}
  1191	
  1192			word1 |= tcp ? TSS_TCP_CHKSUM_BIT : TSS_UDP_CHKSUM_BIT;
  1193		}
  1194	
  1195		frag = -1;
  1196		while (frag <= last_frag) {
  1197			if (frag == -1) {
  1198				buffer = skb->data;
  1199				buflen = skb_headlen(skb);
  1200			} else {
  1201				skb_frag = skb_si->frags + frag;
  1202				buffer = skb_frag_address(skb_frag);
  1203				buflen = skb_frag_size(skb_frag);
  1204			}
  1205	
  1206			if (frag == last_frag) {
  1207				word3 |= EOF_BIT;
  1208				txq->skb[w] = skb;
  1209			}
  1210	
  1211			mapping = dma_map_single(geth->dev, buffer, buflen,
  1212						 DMA_TO_DEVICE);
  1213			if (dma_mapping_error(geth->dev, mapping))
  1214				goto map_error;
  1215	
  1216			txd = txq->ring + w;
  1217			txd->word0.bits32 = buflen;
  1218			txd->word1.bits32 = word1;
  1219			txd->word2.buf_adr = mapping;
  1220			txd->word3.bits32 = word3;
  1221	
  1222			word3 &= MTU_SIZE_BIT_MASK;
  1223			w++;
  1224			w &= m;
  1225			frag++;
  1226		}
  1227	
  1228		*desc = w;
  1229		return 0;
  1230	
  1231	map_error:
  1232		while (w != *desc) {
  1233			w--;
  1234			w &= m;
  1235	
  1236			dma_unmap_page(geth->dev, txq->ring[w].word2.buf_adr,
  1237				       txq->ring[w].word0.bits.buffer_size,
  1238				       DMA_TO_DEVICE);
  1239		}
  1240		return -ENOMEM;
  1241	}
  1242	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net 4/4] net: ethernet: cortina: Handle large frames
  2023-11-04 14:56   ` Andrew Lunn
@ 2023-11-05 20:56     ` Linus Walleij
  0 siblings, 0 replies; 11+ messages in thread
From: Linus Walleij @ 2023-11-05 20:56 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Hans Ulli Kroll, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Michał Mirosław, Vladimir Oltean,
	linux-arm-kernel, netdev, linux-kernel

On Sat, Nov 4, 2023 at 3:57 PM Andrew Lunn <andrew@lunn.ch> wrote:

> > +              * Just bypass on bigger frames.
> > +              */
> > +             word1 |= TSS_BYPASS_BIT;
> > +     } else if (skb->ip_summed != CHECKSUM_NONE) {
>
> I've never looked at how the network stack does checksums. But looking
> at this patch, it made me wounder, how do you tell the stack it needs
> to do a software checksum because the hardware cannot?

I read up on it: the documentation is in
Documentation/networking/checksum-offloads.rst
and in the header for skbuff, include/linux/skbuff.h

Actually we should check for == CHECKSUM_PARTIAL which means
we need to do the checksum (!= CHECKSUM_NONE is not inclusive)
then I call a software fallback directly from the driver if I need to.

> Or for this
> driver, is it always calculating a checksum, which is then ignored?
> Maybe you can improve performance a little but disabling software
> checksum when it is not needed?

The ping was somehow working without proper checksum
before, but I think I'm doing the right thing now, also tested with
HTTP traffic, check out v2.

Thanks for pointing it out, the patch looks way better now.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2023-11-05 20:56 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-11-04 12:43 [PATCH net 0/4] Fix large frames in the Gemini ethernet driver Linus Walleij
2023-11-04 12:43 ` [PATCH net 1/4] net: ethernet: cortina: Fix MTU max setting Linus Walleij
2023-11-04 14:36   ` Andrew Lunn
2023-11-04 12:43 ` [PATCH net 2/4] net: ethernet: cortina: Fix max RX frame define Linus Walleij
2023-11-04 14:38   ` Andrew Lunn
2023-11-04 12:43 ` [PATCH net 3/4] net: ethernet: cortina: Protect against oversized frames Linus Walleij
2023-11-04 14:46   ` Andrew Lunn
2023-11-04 12:43 ` [PATCH net 4/4] net: ethernet: cortina: Handle large frames Linus Walleij
2023-11-04 14:56   ` Andrew Lunn
2023-11-05 20:56     ` Linus Walleij
2023-11-04 15:18   ` kernel test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®