diff mbox series

[net] ravb: expand rx descriptor data to accommodate hw checksum

Message ID 20190110140222.32740-1-horms+renesas@verge.net.au
State Changes Requested
Delegated to: David Miller
Headers show
Series [net] ravb: expand rx descriptor data to accommodate hw checksum | expand

Commit Message

Simon Horman Jan. 10, 2019, 2:02 p.m. UTC 
EtherAVB may provide a checksum of packet data appended to packet data. In
order to allow this checksum to be received by the host descriptor data
needs to be enlarged by 2 bytes to accommodate the checksum.

In the case of MTU-sized packets without a VLAN tag the
checksum were already accommodated by virtue of the space reserved for the
VLAN tag. However, a packet of MTU-size with a  VLAN tag consumed all
packet data space provided by a descriptor leaving no space for the
trailing checksum.

This was not detected by the driver which incorrectly used the last two
bytes of packet data as the checksum and truncate the packet by two bytes.
This resulted all such packets being dropped.

A work around is to disable rx checksum offload
 # ethtool -K eth0 rx off

This patch resolves this problem by increasing the size available for
packet data in rx descriptors by two bytes. It also introduces
RAVB_CSUM_LEN to make things a little clearer than "2" sprinkled lightly
over the driver.

Tested on R-Car E3 (r8a77990) ES1.0 based Ebisu-4D board

Fixes: 4d86d3818627 ("ravb: RX checksum offload")
Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
---
 drivers/net/ethernet/renesas/ravb_main.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

 Change since previous posting: drop RFC designation. I have tested this
 patch and it is ready to bring it to a wider audience is via upstream

Comments

Sergei Shtylyov Jan. 10, 2019, 3:52 p.m. UTC | #1 
Hello!

On 01/10/2019 05:02 PM, Simon Horman wrote:

> EtherAVB may provide a checksum of packet data appended to packet data. In
> order to allow this checksum to be received by the host descriptor data
> needs to be enlarged by 2 bytes to accommodate the checksum.
> 
> In the case of MTU-sized packets without a VLAN tag the
> checksum were already accommodated by virtue of the space reserved for the
> VLAN tag. However, a packet of MTU-size with a  VLAN tag consumed all
> packet data space provided by a descriptor leaving no space for the
> trailing checksum.

   Wait! The gen3 manual is rather clear about the auto-checksumming not working
right in the presence of the VLAN tag. Where do you check for that case?

> This was not detected by the driver which incorrectly used the last two
> bytes of packet data as the checksum and truncate the packet by two bytes.
> This resulted all such packets being dropped.
> 
> A work around is to disable rx checksum offload
>  # ethtool -K eth0 rx off
> 
> This patch resolves this problem by increasing the size available for
> packet data in rx descriptors by two bytes. It also introduces
> RAVB_CSUM_LEN to make things a little clearer than "2" sprinkled lightly
> over the driver.

   What about using sizeof(__sum16) instead? That type is declared in
<linux/types.h> and used in 'struct iphdr'...

> 
> Tested on R-Car E3 (r8a77990) ES1.0 based Ebisu-4D board
> 
> Fixes: 4d86d3818627 ("ravb: RX checksum offload")
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
[...]

MBR, Sergei
Simon Horman Jan. 11, 2019, 1:35 p.m. UTC | #2 
On Thu, Jan 10, 2019 at 06:52:51PM +0300, Sergei Shtylyov wrote:
> Hello!
> 
> On 01/10/2019 05:02 PM, Simon Horman wrote:
> 
> > EtherAVB may provide a checksum of packet data appended to packet data. In
> > order to allow this checksum to be received by the host descriptor data
> > needs to be enlarged by 2 bytes to accommodate the checksum.
> > 
> > In the case of MTU-sized packets without a VLAN tag the
> > checksum were already accommodated by virtue of the space reserved for the
> > VLAN tag. However, a packet of MTU-size with a  VLAN tag consumed all
> > packet data space provided by a descriptor leaving no space for the
> > trailing checksum.
> 
>    Wait! The gen3 manual is rather clear about the auto-checksumming not working
> right in the presence of the VLAN tag. Where do you check for that case?

In my testing on E3 this works correctly. Which portion of
the manual are you referring to?

> 
> > This was not detected by the driver which incorrectly used the last two
> > bytes of packet data as the checksum and truncate the packet by two bytes.
> > This resulted all such packets being dropped.
> > 
> > A work around is to disable rx checksum offload
> >  # ethtool -K eth0 rx off
> > 
> > This patch resolves this problem by increasing the size available for
> > packet data in rx descriptors by two bytes. It also introduces
> > RAVB_CSUM_LEN to make things a little clearer than "2" sprinkled lightly
> > over the driver.
> 
>    What about using sizeof(__sum16) instead? That type is declared in
> <linux/types.h> and used in 'struct iphdr'...

As in the following?

#define RAVB_CSUM_LEN sizeof(__sum16)

> 
> > 
> > Tested on R-Car E3 (r8a77990) ES1.0 based Ebisu-4D board
> > 
> > Fixes: 4d86d3818627 ("ravb: RX checksum offload")
> > Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> [...]
> 
> MBR, Sergei
>
Sergei Shtylyov Jan. 11, 2019, 3:22 p.m. UTC | #3 
On 01/11/2019 04:35 PM, Simon Horman wrote:

>>> EtherAVB may provide a checksum of packet data appended to packet data. In
>>> order to allow this checksum to be received by the host descriptor data
>>> needs to be enlarged by 2 bytes to accommodate the checksum.
>>>
>>> In the case of MTU-sized packets without a VLAN tag the
>>> checksum were already accommodated by virtue of the space reserved for the
>>> VLAN tag. However, a packet of MTU-size with a  VLAN tag consumed all
>>> packet data space provided by a descriptor leaving no space for the
>>> trailing checksum.
>>
>>    Wait! The gen3 manual is rather clear about the auto-checksumming not working
>> right in the presence of the VLAN tag. Where do you check for that case?
> 
> In my testing on E3 this works correctly. Which portion of
> the manual are you referring to?

   Section 50.4.1 in R-Car Series, 3rd Generation User’s Manual: Hardware, Rev 1.00,
section 45A.3.14.1 in R-Car Series, 2nd Generation User’s Manual: Hardware, Rev 2.00.
   The problem is that the checksum is always calculated starting at byte 14, i.e.
amidst the VLAN header (if present).

>>
>>> This was not detected by the driver which incorrectly used the last two
>>> bytes of packet data as the checksum and truncate the packet by two bytes.
>>> This resulted all such packets being dropped.
>>>
>>> A work around is to disable rx checksum offload
>>>  # ethtool -K eth0 rx off
>>>
>>> This patch resolves this problem by increasing the size available for
>>> packet data in rx descriptors by two bytes. It also introduces
>>> RAVB_CSUM_LEN to make things a little clearer than "2" sprinkled lightly
>>> over the driver.
>>
>>    What about using sizeof(__sum16) instead? That type is declared in
>> <linux/types.h> and used in 'struct iphdr'...
> 
> As in the following?
> 
> #define RAVB_CSUM_LEN sizeof(__sum16)

   No, as I said, I'd prefer to avoid any driver specific #define's.

>>> Tested on R-Car E3 (r8a77990) ES1.0 based Ebisu-4D board
>>>
>>> Fixes: 4d86d3818627 ("ravb: RX checksum offload")
>>> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
>> [...]

MBR, Sergei
Simon Horman Jan. 11, 2019, 3:48 p.m. UTC | #4 
On Fri, Jan 11, 2019 at 06:22:12PM +0300, Sergei Shtylyov wrote:
> On 01/11/2019 04:35 PM, Simon Horman wrote:
> 
> >>> EtherAVB may provide a checksum of packet data appended to packet data. In
> >>> order to allow this checksum to be received by the host descriptor data
> >>> needs to be enlarged by 2 bytes to accommodate the checksum.
> >>>
> >>> In the case of MTU-sized packets without a VLAN tag the
> >>> checksum were already accommodated by virtue of the space reserved for the
> >>> VLAN tag. However, a packet of MTU-size with a  VLAN tag consumed all
> >>> packet data space provided by a descriptor leaving no space for the
> >>> trailing checksum.
> >>
> >>    Wait! The gen3 manual is rather clear about the auto-checksumming not working
> >> right in the presence of the VLAN tag. Where do you check for that case?
> > 
> > In my testing on E3 this works correctly. Which portion of
> > the manual are you referring to?
> 
>    Section 50.4.1 in R-Car Series, 3rd Generation User’s Manual: Hardware, Rev 1.00,
> section 45A.3.14.1 in R-Car Series, 2nd Generation User’s Manual: Hardware, Rev 2.00.
>    The problem is that the checksum is always calculated starting at byte
> 14, i.e.  amidst the VLAN header (if present).

Yes, I understand that. What I'm unclear on is why that is a problem.
Empirically I have observed that RX csum offload works in the presence
of a VLAN tag and offers a significant benefit in terms of reduced CPU
utilisation.

> >>> This was not detected by the driver which incorrectly used the last two
> >>> bytes of packet data as the checksum and truncate the packet by two bytes.
> >>> This resulted all such packets being dropped.
> >>>
> >>> A work around is to disable rx checksum offload
> >>>  # ethtool -K eth0 rx off
> >>>
> >>> This patch resolves this problem by increasing the size available for
> >>> packet data in rx descriptors by two bytes. It also introduces
> >>> RAVB_CSUM_LEN to make things a little clearer than "2" sprinkled lightly
> >>> over the driver.
> >>
> >>    What about using sizeof(__sum16) instead? That type is declared in
> >> <linux/types.h> and used in 'struct iphdr'...
> > 
> > As in the following?
> > 
> > #define RAVB_CSUM_LEN sizeof(__sum16)
> 
>    No, as I said, I'd prefer to avoid any driver specific #define's.

If that is your preference I can update the patch accordingly.

> >>> Tested on R-Car E3 (r8a77990) ES1.0 based Ebisu-4D board
> >>>
> >>> Fixes: 4d86d3818627 ("ravb: RX checksum offload")
> >>> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> >> [...]
> 
> MBR, Sergei
>
Sergei Shtylyov Jan. 11, 2019, 4 p.m. UTC | #5 
On 01/11/2019 06:48 PM, Simon Horman wrote:

>>>>> EtherAVB may provide a checksum of packet data appended to packet data. In
>>>>> order to allow this checksum to be received by the host descriptor data
>>>>> needs to be enlarged by 2 bytes to accommodate the checksum.
>>>>>
>>>>> In the case of MTU-sized packets without a VLAN tag the
>>>>> checksum were already accommodated by virtue of the space reserved for the
>>>>> VLAN tag. However, a packet of MTU-size with a  VLAN tag consumed all
>>>>> packet data space provided by a descriptor leaving no space for the
>>>>> trailing checksum.
>>>>
>>>>    Wait! The gen3 manual is rather clear about the auto-checksumming not working
>>>> right in the presence of the VLAN tag. Where do you check for that case?
>>>
>>> In my testing on E3 this works correctly. Which portion of
>>> the manual are you referring to?
>>
>>    Section 50.4.1 in R-Car Series, 3rd Generation User’s Manual: Hardware, Rev 1.00,
>> section 45A.3.14.1 in R-Car Series, 2nd Generation User’s Manual: Hardware, Rev 2.00.
>>    The problem is that the checksum is always calculated starting at byte
>> 14, i.e.  amidst the VLAN header (if present).
> 
> Yes, I understand that. What I'm unclear on is why that is a problem.
> Empirically I have observed that RX csum offload works in the presence
> of a VLAN tag and offers a significant benefit in terms of reduced CPU
> utilisation.

   I thought we're offloading an IP checksum? And in case of VLAN the IP datagram
starts 4 bytes further into a packet? Maybe I'm just misunderstanding things...

>>>>> This was not detected by the driver which incorrectly used the last two
>>>>> bytes of packet data as the checksum and truncate the packet by two bytes.
>>>>> This resulted all such packets being dropped.
>>>>>
>>>>> A work around is to disable rx checksum offload
>>>>>  # ethtool -K eth0 rx off
>>>>>
>>>>> This patch resolves this problem by increasing the size available for
>>>>> packet data in rx descriptors by two bytes. It also introduces
>>>>> RAVB_CSUM_LEN to make things a little clearer than "2" sprinkled lightly
>>>>> over the driver.
>>>>
>>>>    What about using sizeof(__sum16) instead? That type is declared in
>>>> <linux/types.h> and used in 'struct iphdr'...
>>>
>>> As in the following?
>>>
>>> #define RAVB_CSUM_LEN sizeof(__sum16)
>>
>>    No, as I said, I'd prefer to avoid any driver specific #define's.
> 
> If that is your preference I can update the patch accordingly.

   Please do. :-)

>>>>> Tested on R-Car E3 (r8a77990) ES1.0 based Ebisu-4D board
>>>>>
>>>>> Fixes: 4d86d3818627 ("ravb: RX checksum offload")
>>>>> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
>>>> [...]

MBR, Sergei
Sergei Shtylyov Jan. 18, 2019, 2:58 p.m. UTC | #6 
Hello!

On 01/11/2019 06:48 PM, Simon Horman wrote:

>>>>> EtherAVB may provide a checksum of packet data appended to packet data. In
>>>>> order to allow this checksum to be received by the host descriptor data
>>>>> needs to be enlarged by 2 bytes to accommodate the checksum.
>>>>>
>>>>> In the case of MTU-sized packets without a VLAN tag the
>>>>> checksum were already accommodated by virtue of the space reserved for the
>>>>> VLAN tag. However, a packet of MTU-size with a  VLAN tag consumed all
>>>>> packet data space provided by a descriptor leaving no space for the
>>>>> trailing checksum.
>>>>
>>>>    Wait! The gen3 manual is rather clear about the auto-checksumming not working
>>>> right in the presence of the VLAN tag. Where do you check for that case?
>>>
>>> In my testing on E3 this works correctly. Which portion of
>>> the manual are you referring to?
>>
>>    Section 50.4.1 in R-Car Series, 3rd Generation User’s Manual: Hardware, Rev 1.00,
>> section 45A.3.14.1 in R-Car Series, 2nd Generation User’s Manual: Hardware, Rev 2.00.
>>    The problem is that the checksum is always calculated starting at byte
>> 14, i.e.  amidst the VLAN header (if present).
> 
> Yes, I understand that. What I'm unclear on is why that is a problem.
> Empirically I have observed that RX csum offload works in the presence
> of a VLAN tag and offers a significant benefit in terms of reduced CPU
> utilisation.

   OK, after having looked a the networking code, I agree there shouldn't be issues
with VLAN packets.

[...]

MBR, Sergei
diff mbox series

Patch

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index ffc1ada4e6da..545633128718 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -40,6 +40,8 @@ 
 		 NETIF_MSG_RX_ERR | \
 		 NETIF_MSG_TX_ERR)
 
+#define RAVB_CSUM_LEN 2
+
 static const char *ravb_rx_irqs[NUM_RX_QUEUE] = {
 	"ch0", /* RAVB_BE */
 	"ch1", /* RAVB_NC */
@@ -343,7 +345,7 @@  static int ravb_ring_init(struct net_device *ndev, int q)
 	int i;
 
 	priv->rx_buf_sz = (ndev->mtu <= 1492 ? PKT_BUF_SZ : ndev->mtu) +
-		ETH_HLEN + VLAN_HLEN;
+		ETH_HLEN + VLAN_HLEN + RAVB_CSUM_LEN;
 
 	/* Allocate RX and TX skb rings */
 	priv->rx_skb[q] = kcalloc(priv->num_rx_ring[q],
@@ -524,13 +526,15 @@  static void ravb_rx_csum(struct sk_buff *skb)
 {
 	u8 *hw_csum;
 
-	/* The hardware checksum is 2 bytes appended to packet data */
-	if (unlikely(skb->len < 2))
+	/* The hardware checksum is contained in RAVB_CSUM_LEN (2) bytes
+	 * appended to packet data
+	 */
+	if (unlikely(skb->len < RAVB_CSUM_LEN))
 		return;
-	hw_csum = skb_tail_pointer(skb) - 2;
+	hw_csum = skb_tail_pointer(skb) - RAVB_CSUM_LEN;
 	skb->csum = csum_unfold((__force __sum16)get_unaligned_le16(hw_csum));
 	skb->ip_summed = CHECKSUM_COMPLETE;
-	skb_trim(skb, skb->len - 2);
+	skb_trim(skb, skb->len - RAVB_CSUM_LEN);
 }
 
 /* Packet receive function for Ethernet AVB */