netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH 0/2] net: macb: Disable TX checksum offloading on all Zynq
@ 2018-05-25 21:44 Jennifer Dahm
  2018-05-25 21:44 ` [RFC PATCH 1/2] net: macb: Add CAP to disable hardware TX checksum offloading Jennifer Dahm
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Jennifer Dahm @ 2018-05-25 21:44 UTC (permalink / raw)
  To: netdev, David S . Miller, Nicolas Ferre; +Cc: Nathan Sullivan, Jennifer Dahm

During testing, I discovered that the Zynq GEM hardware overwrites all
outgoing UDP packet checksums, which is illegal in packet forwarding
cases. This happens both with and without the checksum-zeroing
behavior  introduced  in  007e4ba3ee137f4700f39aa6dbaf01a71047c5f6
("net: macb: initialize checksum when using checksum offloading"). The
only solution to both the small packet bug and the packet forwarding
bug that I can find is to disable TX checksum offloading entirely.

There's still the possibility that these bugs are actually with the
driver software and not with the hardware. I've found several places
where the checksum is set to 0xFFFF (the incorrect checksum found in
small packets) when something goes wrong, and I can imagine a buggy
driver writing over the checksum blindly when TX checksum offloading
is enabled.

I would like feedback on two things:
1. Is it possible that the two bugs described above are caused by the
   driver and not by the hardware? If so, where should I look to
   implicate the driver?
2. Is this a problem we care enough about to completely disable TX
   checksum offloading?

Here is the testing procedure I used to reproduce these bugs on my
machine. Specifically, without this patchset, step 9 fails. Without
007e4ba3ee, step 8 also fails.

1. Set up the test environment:
  a. Acquire a Zynq device with two ethernet ports. This is the DUT.
  b. Acquire a USB-Ethernet adapter.
  c. Acquire two ethernet cables.
  d. Connect one Ethernet port on the DUT to your computer's network
     switch.
  e. Connect the other Ethernet port to the USB-Ethernet adapter and
     plug that adapter into your computer.
  f. Set up a Linux VM to send packets through the DUT. I recommend
     using a VM here so that you can easily detach it from the primary
     network to force outgoing traffic through the DUT.
  g. Set up a computer with a packet inspecting program to receive and
     inspect packets. This doesn't need to be a VM. For the purposes
     of this test, I'll be using a Windows instance with WireShark.
2. Load the kernel you want to test onto the DUT, making sure to
   include the `bridge` module.
3. Set up a bridge on the DUT. The following commands on the DUT
   should work, replacing `eth0` and `eth1` with the two ethernet
   interfaces on the DUT:
   ```
   brctl addbr test
   brctl addif test eth0 eth1
   ifconfig eth0 0.0.0.0
   ifconfig eth1 0.0.0.0
   dhclient test -v
   ```
4. Disconnect the Linux VM from your host computer's network and
   connect it to the USB-Ethernet adapter in order to force outgoing
   network traffic through the DUT. If necessary, run dhclient on the
   Linux VM to acquire an IP address.
5. Ensure that you can reach your Windows instance from your Linux VM
   through the DUT (e.g. ping).
6. Start WireShark on your Windows instance and start monitoring
   traffic on a specific, unused port (e.g. 61557).
7. Using netcat, send a few not-tiny UDP packets from your Linux VM to
   your Windows instance to ensure that valid UDP packets are properly
   forwarded. Ex:
   ```
   echo "hello world" | netcat -u <WindowsIP> 61557
   ```
   Inspect these packets to ensure that the data arrived intact and
   that the checksum looks reasonable (i.e. not 0x0000 or 0xFFFF).
8. Using netcat, send a few tiny UDP packets (2 bytes or fewer) from
   Linux VM to your Windows instance to ensure that the checksum is
   reasonable. Ex:
   ```
   echo "h" | netcat -u <WindowsIP> 61557
   ```
9. Using a custom program, send UDP packets with broken checksums
   (e.g. 0xABCD) from your Linux VM to your Windows instance. Inspect
   these packets with WireShark and make sure that the packet arrived
   with the same checksum you sent it with.

For step 9, I wrote a C program using the Linux socket API that will
send a properly formatted UDP packet with the payload "Hello!" and a
(broken) checksum of 0xABCD to port 61557 on the host provided at the
command line. I can send the full program if you would like, but here
is the important part of it:
```
struct custom_udp {
	int16_t s_port;
	int16_t d_port;
	int16_t length;
	int16_t check;
	char data[];
};

int send_message(int sockfd, in_port_t port, const char *message) {
	struct custom_udp *frame;
	int16_t message_len;
	int16_t frame_len;
	int ret;

	message_len = strlen(message) * sizeof(char);
	frame_len = sizeof(struct custom_udp) + message_len;
	frame = malloc(frame_len);
	frame->s_port = htons(0);
	frame->d_port = htons(port);
	frame->length = htons(frame_len);
	frame->check = htons(0xABCD);
	memmove(frame->data, message, message_len);

	ret = write(sockfd, frame, frame_len);
	free(frame);

	return ret;
}
```

Jennifer Dahm (1):
  net/macb: Disable TX checksum offloading on all Zynq-7000

 drivers/net/ethernet/cadence/macb.h      |  1 +
 drivers/net/ethernet/cadence/macb_main.c | 11 ++++++++---
 2 files changed, 9 insertions(+), 3 deletions(-)

-- 
2.7.4

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

* [RFC PATCH 1/2] net: macb: Add CAP to disable hardware TX checksum offloading
  2018-05-25 21:44 [RFC PATCH 0/2] net: macb: Disable TX checksum offloading on all Zynq Jennifer Dahm
@ 2018-05-25 21:44 ` Jennifer Dahm
  2018-06-04 15:13   ` Nicolas Ferre
  2018-05-25 21:44 ` [RFC PATCH 2/2] net: macb: Disable TX checksum offloading on all Zynq Jennifer Dahm
  2018-06-04 15:05 ` [RFC PATCH 0/2] " Nicolas Ferre
  2 siblings, 1 reply; 12+ messages in thread
From: Jennifer Dahm @ 2018-05-25 21:44 UTC (permalink / raw)
  To: netdev, David S . Miller, Nicolas Ferre; +Cc: Nathan Sullivan, Jennifer Dahm

Certain PHYs have significant bugs in their TX checksum offloading
that cannot be solved in software. In order to accommodate these PHYS,
add a CAP to disable this hardware.

Signed-off-by: Jennifer Dahm <jennifer.dahm@ni.com>
---
 drivers/net/ethernet/cadence/macb.h      | 1 +
 drivers/net/ethernet/cadence/macb_main.c | 8 ++++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 8665982..6b85e97 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -635,6 +635,7 @@
 #define MACB_CAPS_USRIO_DISABLED		0x00000010
 #define MACB_CAPS_JUMBO				0x00000020
 #define MACB_CAPS_GEM_HAS_PTP			0x00000040
+#define MACB_CAPS_DISABLE_TX_HW_CSUM		0x00000080
 #define MACB_CAPS_FIFO_MODE			0x10000000
 #define MACB_CAPS_GIGABIT_MODE_AVAILABLE	0x20000000
 #define MACB_CAPS_SG_DISABLED			0x40000000
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 3e93df5..a5d564b 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -3360,8 +3360,12 @@ static int macb_init(struct platform_device *pdev)
 		dev->hw_features |= MACB_NETIF_LSO;
 
 	/* Checksum offload is only available on gem with packet buffer */
-	if (macb_is_gem(bp) && !(bp->caps & MACB_CAPS_FIFO_MODE))
-		dev->hw_features |= NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
+	if (macb_is_gem(bp) && !(bp->caps & MACB_CAPS_FIFO_MODE)) {
+		if (!(bp->caps & MACB_CAPS_DISABLE_TX_HW_CSUM))
+			dev->hw_features |= NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
+		else
+			dev->hw_features |= NETIF_F_RXCSUM;
+	}
 	if (bp->caps & MACB_CAPS_SG_DISABLED)
 		dev->hw_features &= ~NETIF_F_SG;
 	dev->features = dev->hw_features;
-- 
2.7.4

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

* [RFC PATCH 2/2] net: macb: Disable TX checksum offloading on all Zynq
  2018-05-25 21:44 [RFC PATCH 0/2] net: macb: Disable TX checksum offloading on all Zynq Jennifer Dahm
  2018-05-25 21:44 ` [RFC PATCH 1/2] net: macb: Add CAP to disable hardware TX checksum offloading Jennifer Dahm
@ 2018-05-25 21:44 ` Jennifer Dahm
  2018-06-04 15:06   ` Nicolas Ferre
  2018-06-04 15:05 ` [RFC PATCH 0/2] " Nicolas Ferre
  2 siblings, 1 reply; 12+ messages in thread
From: Jennifer Dahm @ 2018-05-25 21:44 UTC (permalink / raw)
  To: netdev, David S . Miller, Nicolas Ferre; +Cc: Nathan Sullivan, Jennifer Dahm

The Zynq ethernet hardware has checksum offloading bugs that cause
small UDP packets (<= 2 bytes) to be sent with an incorrect checksum
(0xffff) and forwarded UDP packets to be re-checksummed, which is
illegal behavior. The best solution we have right now is to disable
hardware TX checksum offloading entirely.

Signed-off-by: Jennifer Dahm <jennifer.dahm@ni.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index a5d564b..e8cc68a 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -3807,7 +3807,8 @@ static const struct macb_config zynqmp_config = {
 };
 
 static const struct macb_config zynq_config = {
-	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_NO_GIGABIT_HALF,
+	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_NO_GIGABIT_HALF
+	      | MACB_CAPS_DISABLE_TX_HW_CSUM,
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
 	.init = macb_init,
-- 
2.7.4

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

* Re: [RFC PATCH 0/2] net: macb: Disable TX checksum offloading on all Zynq
  2018-05-25 21:44 [RFC PATCH 0/2] net: macb: Disable TX checksum offloading on all Zynq Jennifer Dahm
  2018-05-25 21:44 ` [RFC PATCH 1/2] net: macb: Add CAP to disable hardware TX checksum offloading Jennifer Dahm
  2018-05-25 21:44 ` [RFC PATCH 2/2] net: macb: Disable TX checksum offloading on all Zynq Jennifer Dahm
@ 2018-06-04 15:05 ` Nicolas Ferre
  2018-06-05  4:51   ` Harini Katakam
  2 siblings, 1 reply; 12+ messages in thread
From: Nicolas Ferre @ 2018-06-04 15:05 UTC (permalink / raw)
  To: Jennifer Dahm, netdev, David S . Miller
  Cc: Nathan Sullivan, Rafal Ozieblo, Claudiu Beznea, Harini Katakam

Jennifer,

On 25/05/2018 at 23:44, Jennifer Dahm wrote:
> During testing, I discovered that the Zynq GEM hardware overwrites all
> outgoing UDP packet checksums, which is illegal in packet forwarding
> cases. This happens both with and without the checksum-zeroing
> behavior  introduced  in  007e4ba3ee137f4700f39aa6dbaf01a71047c5f6
> ("net: macb: initialize checksum when using checksum offloading"). The
> only solution to both the small packet bug and the packet forwarding
> bug that I can find is to disable TX checksum offloading entirely.

Are the bugs listed above present in all revisions of the GEM IP, only 
for some revisions?
Is there an errata that describe this issue for the Zynq GEM?


> There's still the possibility that these bugs are actually with the
> driver software and not with the hardware. I've found several places
> where the checksum is set to 0xFFFF (the incorrect checksum found in
> small packets) when something goes wrong, and I can imagine a buggy
> driver writing over the checksum blindly when TX checksum offloading
> is enabled.
> 
> I would like feedback on two things:
> 1. Is it possible that the two bugs described above are caused by the
>     driver and not by the hardware? If so, where should I look to
>     implicate the driver?

Rafal,
Do you want to comment on this part?

> 2. Is this a problem we care enough about to completely disable TX
>     checksum offloading?

I would prefer to keep it enabled if possible. I mean I'm not against 
such a workaround if the HW is not able to handle such cases but 
offloading a CPU is always good when we have "low end" processors like 
ARM9 at 200MHz like the older AT91 that use this driver...

> Here is the testing procedure I used to reproduce these bugs on my
> machine. Specifically, without this patchset, step 9 fails. Without
> 007e4ba3ee, step 8 also fails.
> 
> 1. Set up the test environment:
>    a. Acquire a Zynq device with two ethernet ports. This is the DUT.
>    b. Acquire a USB-Ethernet adapter.
>    c. Acquire two ethernet cables.
>    d. Connect one Ethernet port on the DUT to your computer's network
>       switch.
>    e. Connect the other Ethernet port to the USB-Ethernet adapter and
>       plug that adapter into your computer.
>    f. Set up a Linux VM to send packets through the DUT. I recommend
>       using a VM here so that you can easily detach it from the primary
>       network to force outgoing traffic through the DUT.
>    g. Set up a computer with a packet inspecting program to receive and
>       inspect packets. This doesn't need to be a VM. For the purposes
>       of this test, I'll be using a Windows instance with WireShark.
> 2. Load the kernel you want to test onto the DUT, making sure to
>     include the `bridge` module.
> 3. Set up a bridge on the DUT. The following commands on the DUT
>     should work, replacing `eth0` and `eth1` with the two ethernet
>     interfaces on the DUT:
>     ```
>     brctl addbr test
>     brctl addif test eth0 eth1
>     ifconfig eth0 0.0.0.0
>     ifconfig eth1 0.0.0.0
>     dhclient test -v
>     ```
> 4. Disconnect the Linux VM from your host computer's network and
>     connect it to the USB-Ethernet adapter in order to force outgoing
>     network traffic through the DUT. If necessary, run dhclient on the
>     Linux VM to acquire an IP address.
> 5. Ensure that you can reach your Windows instance from your Linux VM
>     through the DUT (e.g. ping).
> 6. Start WireShark on your Windows instance and start monitoring
>     traffic on a specific, unused port (e.g. 61557).
> 7. Using netcat, send a few not-tiny UDP packets from your Linux VM to
>     your Windows instance to ensure that valid UDP packets are properly
>     forwarded. Ex:
>     ```
>     echo "hello world" | netcat -u <WindowsIP> 61557
>     ```
>     Inspect these packets to ensure that the data arrived intact and
>     that the checksum looks reasonable (i.e. not 0x0000 or 0xFFFF).
> 8. Using netcat, send a few tiny UDP packets (2 bytes or fewer) from
>     Linux VM to your Windows instance to ensure that the checksum is
>     reasonable. Ex:
>     ```
>     echo "h" | netcat -u <WindowsIP> 61557
>     ```
> 9. Using a custom program, send UDP packets with broken checksums
>     (e.g. 0xABCD) from your Linux VM to your Windows instance. Inspect
>     these packets with WireShark and make sure that the packet arrived
>     with the same checksum you sent it with.
> 
> For step 9, I wrote a C program using the Linux socket API that will
> send a properly formatted UDP packet with the payload "Hello!" and a
> (broken) checksum of 0xABCD to port 61557 on the host provided at the
> command line. I can send the full program if you would like, but here
> is the important part of it:
> ```
> struct custom_udp {
> 	int16_t s_port;
> 	int16_t d_port;
> 	int16_t length;
> 	int16_t check;
> 	char data[];
> };
> 
> int send_message(int sockfd, in_port_t port, const char *message) {
> 	struct custom_udp *frame;
> 	int16_t message_len;
> 	int16_t frame_len;
> 	int ret;
> 
> 	message_len = strlen(message) * sizeof(char);
> 	frame_len = sizeof(struct custom_udp) + message_len;
> 	frame = malloc(frame_len);
> 	frame->s_port = htons(0);
> 	frame->d_port = htons(port);
> 	frame->length = htons(frame_len);
> 	frame->check = htons(0xABCD);
> 	memmove(frame->data, message, message_len);
> 
> 	ret = write(sockfd, frame, frame_len);
> 	free(frame);
> 
> 	return ret;
> }

Thanks a lot for the detailed testing procedure. It will definitively 
help us reproduce the bug.

As a conclusion, I would say that we can use this patch as a last resort 
if we are sure that:
1/ the driver doesn't do weird things with TX CSUM in the code
2/ the hw is unable to handle these particular cases.


Best regards,
   Nicolas


> ```
> 
> Jennifer Dahm (1):
>    net/macb: Disable TX checksum offloading on all Zynq-7000
> 
>   drivers/net/ethernet/cadence/macb.h      |  1 +
>   drivers/net/ethernet/cadence/macb_main.c | 11 ++++++++---
>   2 files changed, 9 insertions(+), 3 deletions(-)
> 


-- 
Nicolas Ferre

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

* Re: [RFC PATCH 2/2] net: macb: Disable TX checksum offloading on all Zynq
  2018-05-25 21:44 ` [RFC PATCH 2/2] net: macb: Disable TX checksum offloading on all Zynq Jennifer Dahm
@ 2018-06-04 15:06   ` Nicolas Ferre
  2018-06-06  6:50     ` Michal Simek
  0 siblings, 1 reply; 12+ messages in thread
From: Nicolas Ferre @ 2018-06-04 15:06 UTC (permalink / raw)
  To: Jennifer Dahm, netdev, David S . Miller, Michal Simek, Harini Katakam
  Cc: Nathan Sullivan

On 25/05/2018 at 23:44, Jennifer Dahm wrote:
> The Zynq ethernet hardware has checksum offloading bugs that cause
> small UDP packets (<= 2 bytes) to be sent with an incorrect checksum
> (0xffff) and forwarded UDP packets to be re-checksummed, which is
> illegal behavior. The best solution we have right now is to disable
> hardware TX checksum offloading entirely.
> 
> Signed-off-by: Jennifer Dahm <jennifer.dahm@ni.com>

Adding some xilinx people I know...

> ---
>   drivers/net/ethernet/cadence/macb_main.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index a5d564b..e8cc68a 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -3807,7 +3807,8 @@ static const struct macb_config zynqmp_config = {
>   };
>   
>   static const struct macb_config zynq_config = {
> -	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_NO_GIGABIT_HALF,
> +	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_NO_GIGABIT_HALF
> +	      | MACB_CAPS_DISABLE_TX_HW_CSUM,
>   	.dma_burst_length = 16,
>   	.clk_init = macb_clk_init,
>   	.init = macb_init,
> 


-- 
Nicolas Ferre

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

* Re: [RFC PATCH 1/2] net: macb: Add CAP to disable hardware TX checksum offloading
  2018-05-25 21:44 ` [RFC PATCH 1/2] net: macb: Add CAP to disable hardware TX checksum offloading Jennifer Dahm
@ 2018-06-04 15:13   ` Nicolas Ferre
  2018-06-07 16:43     ` Jennifer Dahm
  0 siblings, 1 reply; 12+ messages in thread
From: Nicolas Ferre @ 2018-06-04 15:13 UTC (permalink / raw)
  To: Jennifer Dahm, netdev, David S . Miller; +Cc: Nathan Sullivan

On 25/05/2018 at 23:44, Jennifer Dahm wrote:
> Certain PHYs have significant bugs in their TX checksum offloading
> that cannot be solved in software. In order to accommodate these PHYS,
> add a CAP to disable this hardware.
> 
> Signed-off-by: Jennifer Dahm <jennifer.dahm@ni.com>
> ---
>   drivers/net/ethernet/cadence/macb.h      | 1 +
>   drivers/net/ethernet/cadence/macb_main.c | 8 ++++++--
>   2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 8665982..6b85e97 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -635,6 +635,7 @@
>   #define MACB_CAPS_USRIO_DISABLED		0x00000010
>   #define MACB_CAPS_JUMBO				0x00000020
>   #define MACB_CAPS_GEM_HAS_PTP			0x00000040
> +#define MACB_CAPS_DISABLE_TX_HW_CSUM		0x00000080

Nitpicking: "DISABLED" tends to be at the end of the name...

>   #define MACB_CAPS_FIFO_MODE			0x10000000
>   #define MACB_CAPS_GIGABIT_MODE_AVAILABLE	0x20000000
>   #define MACB_CAPS_SG_DISABLED			0x40000000
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 3e93df5..a5d564b 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -3360,8 +3360,12 @@ static int macb_init(struct platform_device *pdev)
>   		dev->hw_features |= MACB_NETIF_LSO;
>   
>   	/* Checksum offload is only available on gem with packet buffer */
> -	if (macb_is_gem(bp) && !(bp->caps & MACB_CAPS_FIFO_MODE))
> -		dev->hw_features |= NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
> +	if (macb_is_gem(bp) && !(bp->caps & MACB_CAPS_FIFO_MODE)) {
> +		if (!(bp->caps & MACB_CAPS_DISABLE_TX_HW_CSUM))

Why not the other way around? negating a "disabled" feature is always 
challenge ;-)

> +			dev->hw_features |= NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
> +		else
> +			dev->hw_features |= NETIF_F_RXCSUM;
> +	}
>   	if (bp->caps & MACB_CAPS_SG_DISABLED)
>   		dev->hw_features &= ~NETIF_F_SG;
>   	dev->features = dev->hw_features;
> 


-- 
Nicolas Ferre

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

* Re: [RFC PATCH 0/2] net: macb: Disable TX checksum offloading on all Zynq
  2018-06-04 15:05 ` [RFC PATCH 0/2] " Nicolas Ferre
@ 2018-06-05  4:51   ` Harini Katakam
  2018-08-01 12:53     ` Harini Katakam
  0 siblings, 1 reply; 12+ messages in thread
From: Harini Katakam @ 2018-06-05  4:51 UTC (permalink / raw)
  To: Nicolas Ferre
  Cc: Jennifer Dahm, netdev, David S . Miller, Nathan Sullivan,
	Rafal Ozieblo, Claudiu Beznea, Harini Katakam

Hi Jeniffer,

On Mon, Jun 4, 2018 at 8:35 PM, Nicolas Ferre
<nicolas.ferre@microchip.com> wrote:
> Jennifer,
>
> On 25/05/2018 at 23:44, Jennifer Dahm wrote:
>>
>> During testing, I discovered that the Zynq GEM hardware overwrites all
>> outgoing UDP packet checksums, which is illegal in packet forwarding
>> cases. This happens both with and without the checksum-zeroing
>> behavior  introduced  in  007e4ba3ee137f4700f39aa6dbaf01a71047c5f6
>> ("net: macb: initialize checksum when using checksum offloading"). The
>> only solution to both the small packet bug and the packet forwarding
>> bug that I can find is to disable TX checksum offloading entirely.
>
>

Thanks for the extensive testing.
I'll try to reproduce and see if it is something to be fixed in the driver.

> Are the bugs listed above present in all revisions of the GEM IP, only for
> some revisions?
> Is there an errata that describe this issue for the Zynq GEM?

@Nicolas, AFAIK, there is no errata for this in either Cadence or
Zynq documentation.

Regards,
Harini

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

* Re: [RFC PATCH 2/2] net: macb: Disable TX checksum offloading on all Zynq
  2018-06-04 15:06   ` Nicolas Ferre
@ 2018-06-06  6:50     ` Michal Simek
  0 siblings, 0 replies; 12+ messages in thread
From: Michal Simek @ 2018-06-06  6:50 UTC (permalink / raw)
  To: Nicolas Ferre, Jennifer Dahm, netdev, David S . Miller, Harini Katakam
  Cc: Nathan Sullivan

On 4.6.2018 17:06, Nicolas Ferre wrote:
> On 25/05/2018 at 23:44, Jennifer Dahm wrote:
>> The Zynq ethernet hardware has checksum offloading bugs that cause
>> small UDP packets (<= 2 bytes) to be sent with an incorrect checksum
>> (0xffff) and forwarded UDP packets to be re-checksummed, which is
>> illegal behavior. The best solution we have right now is to disable
>> hardware TX checksum offloading entirely.
>>
>> Signed-off-by: Jennifer Dahm <jennifer.dahm@ni.com>
> 
> Adding some xilinx people I know...

Harini: Please look at this.

Thanks,
Michal

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

* Re: [RFC PATCH 1/2] net: macb: Add CAP to disable hardware TX checksum offloading
  2018-06-04 15:13   ` Nicolas Ferre
@ 2018-06-07 16:43     ` Jennifer Dahm
  0 siblings, 0 replies; 12+ messages in thread
From: Jennifer Dahm @ 2018-06-07 16:43 UTC (permalink / raw)
  To: Nicolas Ferre, netdev, David S . Miller; +Cc: Nathan Sullivan

Hi Nicolas,

On 06/04/2018 10:13 AM, Nicolas Ferre wrote:
> On 25/05/2018 at 23:44, Jennifer Dahm wrote:
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c 
>> b/drivers/net/ethernet/cadence/macb_main.c
>> index 3e93df5..a5d564b 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -3360,8 +3360,12 @@ static int macb_init(struct platform_device 
>> *pdev)
>>           dev->hw_features |= MACB_NETIF_LSO;
>>         /* Checksum offload is only available on gem with packet 
>> buffer */
>> -    if (macb_is_gem(bp) && !(bp->caps & MACB_CAPS_FIFO_MODE))
>> -        dev->hw_features |= NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
>> +    if (macb_is_gem(bp) && !(bp->caps & MACB_CAPS_FIFO_MODE)) {
>> +        if (!(bp->caps & MACB_CAPS_DISABLE_TX_HW_CSUM))
>
> Why not the other way around? negating a "disabled" feature is always 
> challenge ;-)
>
>> +            dev->hw_features |= NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
>> +        else
>> +            dev->hw_features |= NETIF_F_RXCSUM;
>> +    }
>>       if (bp->caps & MACB_CAPS_SG_DISABLED)
>>           dev->hw_features &= ~NETIF_F_SG;
>>       dev->features = dev->hw_features;

I can switch the ordering of the if-else clauses if that's what you're 
nitpicking. ;)

Alternatively, if you're asking why the flag is used to disable rather 
than enable checksum offloading: I was working under the assumption that 
this was an isolated bug, and so an opt-out would require less 
maintainance than an opt-in. If we discover that this is a problem 
across a wide variety of Cadence IP, it would definitely make sense to 
replace it with an opt-in (i.e. MACB_CAPS_TX_HW_CSUM_ENABLED).

Best,
Jennifer Dahm

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

* Re: [RFC PATCH 0/2] net: macb: Disable TX checksum offloading on all Zynq
  2018-06-05  4:51   ` Harini Katakam
@ 2018-08-01 12:53     ` Harini Katakam
  2018-08-07  8:50       ` Claudiu Beznea
  0 siblings, 1 reply; 12+ messages in thread
From: Harini Katakam @ 2018-08-01 12:53 UTC (permalink / raw)
  To: Jennifer Dahm
  Cc: netdev, David S . Miller, Nathan Sullivan, Rafal Ozieblo,
	Claudiu Beznea, Harini Katakam, Nicolas Ferre

Hi Jennifer,

On Tue, Jun 5, 2018 at 10:21 AM, Harini Katakam <harinik@xilinx.com> wrote:
> Hi Jeniffer,
>
> On Mon, Jun 4, 2018 at 8:35 PM, Nicolas Ferre
> <nicolas.ferre@microchip.com> wrote:
>> Jennifer,
>>
>> On 25/05/2018 at 23:44, Jennifer Dahm wrote:
>>>
>>> During testing, I discovered that the Zynq GEM hardware overwrites all
>>> outgoing UDP packet checksums, which is illegal in packet forwarding
>>> cases. This happens both with and without the checksum-zeroing
>>> behavior  introduced  in  007e4ba3ee137f4700f39aa6dbaf01a71047c5f6
>>> ("net: macb: initialize checksum when using checksum offloading"). The
>>> only solution to both the small packet bug and the packet forwarding
>>> bug that I can find is to disable TX checksum offloading entirely.
>>
>>
>
> Thanks for the extensive testing.
> I'll try to reproduce and see if it is something to be fixed in the driver.
>
>> Are the bugs listed above present in all revisions of the GEM IP, only for
>> some revisions?
>> Is there an errata that describe this issue for the Zynq GEM?
>
> @Nicolas, AFAIK, there is no errata for this in either Cadence or
> Zynq documentation.

I was unable to reproduce this issue on Zynq.
Although I do not have HW with two GEM ports,
I tried by routing one GEM via PL and another via on board RGMII.
Since there was no specific errata related to this, I also tried on
subsequent ZynqMP versions with multiple GEM ports but dint find any
checksum issues. I discussed the same with cadence and they
tried the test with 2 bytes of UDP payload on the Zynq GEM IP
version in their regressions and did not hit any issue either.

I tried to reach out earlier to see if you can share your exact
application. Could you please let me know if you have any
further updates?

Regards,
Harini

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

* Re: [RFC PATCH 0/2] net: macb: Disable TX checksum offloading on all Zynq
  2018-08-01 12:53     ` Harini Katakam
@ 2018-08-07  8:50       ` Claudiu Beznea
  2018-08-07  9:09         ` Harini Katakam
  0 siblings, 1 reply; 12+ messages in thread
From: Claudiu Beznea @ 2018-08-07  8:50 UTC (permalink / raw)
  To: Harini Katakam, Jennifer Dahm
  Cc: netdev, David S . Miller, Nathan Sullivan, Rafal Ozieblo,
	Harini Katakam, Nicolas Ferre

Hi Harini,

On 01.08.2018 15:53, Harini Katakam wrote:
> Hi Jennifer,
> 
> On Tue, Jun 5, 2018 at 10:21 AM, Harini Katakam <harinik@xilinx.com> wrote:
>> Hi Jeniffer,
>>
>> On Mon, Jun 4, 2018 at 8:35 PM, Nicolas Ferre
>> <nicolas.ferre@microchip.com> wrote:
>>> Jennifer,
>>>
>>> On 25/05/2018 at 23:44, Jennifer Dahm wrote:
>>>>
>>>> During testing, I discovered that the Zynq GEM hardware overwrites all
>>>> outgoing UDP packet checksums, which is illegal in packet forwarding
>>>> cases. This happens both with and without the checksum-zeroing
>>>> behavior  introduced  in  007e4ba3ee137f4700f39aa6dbaf01a71047c5f6
>>>> ("net: macb: initialize checksum when using checksum offloading"). The
>>>> only solution to both the small packet bug and the packet forwarding
>>>> bug that I can find is to disable TX checksum offloading entirely.
>>>
>>>
>>
>> Thanks for the extensive testing.
>> I'll try to reproduce and see if it is something to be fixed in the driver.
>>
>>> Are the bugs listed above present in all revisions of the GEM IP, only for
>>> some revisions?
>>> Is there an errata that describe this issue for the Zynq GEM?
>>
>> @Nicolas, AFAIK, there is no errata for this in either Cadence or
>> Zynq documentation.
> 
> I was unable to reproduce this issue on Zynq.
> Although I do not have HW with two GEM ports,
> I tried by routing one GEM via PL and another via on board RGMII.
> Since there was no specific errata related to this, I also tried on
> subsequent ZynqMP versions with multiple GEM ports but dint find any
> checksum issues. I discussed the same with cadence and they
> tried the test with 2 bytes of UDP payload on the Zynq GEM IP
> version in their regressions and did not hit any issue either.
> 
> I tried to reach out earlier to see if you can share your exact
> application. Could you please let me know if you have any
> further updates?

I manage to reproduce the issue and provide a patch for this (see patch 3/3
from [1]).

[1] https://www.spinics.net/lists/netdev/msg513848.html

> 
> Regards,
> Harini
> 

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

* RE: [RFC PATCH 0/2] net: macb: Disable TX checksum offloading on all Zynq
  2018-08-07  8:50       ` Claudiu Beznea
@ 2018-08-07  9:09         ` Harini Katakam
  0 siblings, 0 replies; 12+ messages in thread
From: Harini Katakam @ 2018-08-07  9:09 UTC (permalink / raw)
  To: Claudiu Beznea, Jennifer Dahm
  Cc: netdev, David S . Miller, Nathan Sullivan, Rafal Ozieblo,
	Nicolas Ferre, 'harinikatakamlinux@gmail.com'

Hi Claudiu,

> -----Original Message-----
> From: Claudiu Beznea [mailto:Claudiu.Beznea@microchip.com]
> Sent: Tuesday, August 7, 2018 2:21 PM
> To: Harini Katakam <harinik@xilinx.com>; Jennifer Dahm
> <jennifer.dahm@ni.com>
> Cc: netdev@vger.kernel.org; David S . Miller <davem@davemloft.net>; Nathan
> Sullivan <nathan.sullivan@ni.com>; Rafal Ozieblo <rafalo@cadence.com>;
> Harini Katakam <harinik@xilinx.com>; Nicolas Ferre
> <nicolas.ferre@microchip.com>
> Subject: Re: [RFC PATCH 0/2] net: macb: Disable TX checksum offloading on all
> Zynq
> 
> Hi Harini,
> 
> On 01.08.2018 15:53, Harini Katakam wrote:
> > Hi Jennifer,
> >
> > On Tue, Jun 5, 2018 at 10:21 AM, Harini Katakam <harinik@xilinx.com> wrote:
> >> Hi Jeniffer,
> >>
> >> On Mon, Jun 4, 2018 at 8:35 PM, Nicolas Ferre
> >> <nicolas.ferre@microchip.com> wrote:
> >>> Jennifer,
> >>>
> >>> On 25/05/2018 at 23:44, Jennifer Dahm wrote:
> >>>>
> >>>> During testing, I discovered that the Zynq GEM hardware overwrites
> >>>> all outgoing UDP packet checksums, which is illegal in packet
> >>>> forwarding cases. This happens both with and without the
> >>>> checksum-zeroing behavior  introduced  in
> >>>> 007e4ba3ee137f4700f39aa6dbaf01a71047c5f6
> >>>> ("net: macb: initialize checksum when using checksum offloading").
> >>>> The only solution to both the small packet bug and the packet
> >>>> forwarding bug that I can find is to disable TX checksum offloading
> entirely.
> >>>
> >>>
> >>
> >> Thanks for the extensive testing.
> >> I'll try to reproduce and see if it is something to be fixed in the driver.
> >>
> >>> Are the bugs listed above present in all revisions of the GEM IP,
> >>> only for some revisions?
> >>> Is there an errata that describe this issue for the Zynq GEM?
> >>
> >> @Nicolas, AFAIK, there is no errata for this in either Cadence or
> >> Zynq documentation.
> >
> > I was unable to reproduce this issue on Zynq.
> > Although I do not have HW with two GEM ports, I tried by routing one
> > GEM via PL and another via on board RGMII.
> > Since there was no specific errata related to this, I also tried on
> > subsequent ZynqMP versions with multiple GEM ports but dint find any
> > checksum issues. I discussed the same with cadence and they tried the
> > test with 2 bytes of UDP payload on the Zynq GEM IP version in their
> > regressions and did not hit any issue either.
> >
> > I tried to reach out earlier to see if you can share your exact
> > application. Could you please let me know if you have any further
> > updates?
> 
> I manage to reproduce the issue and provide a patch for this (see patch 3/3 from
> [1]).
> 
> [1] https://www.spinics.net/lists/netdev/msg513848.html
 
Sorry, I missed your series. Thanks.

Regards,
Harini

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

end of thread, other threads:[~2018-08-07 11:22 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-25 21:44 [RFC PATCH 0/2] net: macb: Disable TX checksum offloading on all Zynq Jennifer Dahm
2018-05-25 21:44 ` [RFC PATCH 1/2] net: macb: Add CAP to disable hardware TX checksum offloading Jennifer Dahm
2018-06-04 15:13   ` Nicolas Ferre
2018-06-07 16:43     ` Jennifer Dahm
2018-05-25 21:44 ` [RFC PATCH 2/2] net: macb: Disable TX checksum offloading on all Zynq Jennifer Dahm
2018-06-04 15:06   ` Nicolas Ferre
2018-06-06  6:50     ` Michal Simek
2018-06-04 15:05 ` [RFC PATCH 0/2] " Nicolas Ferre
2018-06-05  4:51   ` Harini Katakam
2018-08-01 12:53     ` Harini Katakam
2018-08-07  8:50       ` Claudiu Beznea
2018-08-07  9:09         ` Harini Katakam

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).