diff mbox series

[PATCH/RFT,net] ravb: expand rx descriptor data to accommodate hw checksum

Message ID 20181210085917.23056-1-horms+renesas@verge.net.au
State RFC, archived
Delegated to: David Miller
Headers show
Series [PATCH/RFT,net] ravb: expand rx descriptor data to accommodate hw checksum | expand

Commit Message

Simon Horman Dec. 10, 2018, 8:59 a.m. UTC 
From: Simon Horman <horms@verge.net.au>

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(-)

 I have marked this patch as RTF as I would like it to see further testing
 before being applied.

Comments

Sergei Shtylyov Dec. 10, 2018, 4:56 p.m. UTC | #1 
Hello!

On 12/10/2018 11:59 AM, Simon Horman wrote:

> From: Simon Horman <horms@verge.net.au>
> 
> 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.

   I think a comment would work better in this case.

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

   You need more testing (RFT specified in the subject)?

> 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(-)
> 
>  I have marked this patch as RTF as I would like it to see further testing
>  before being applied.
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index defed0d0c51d..f7f130cf61e4 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

   What's EtherAVB specific there, why RAVB prefix?

[...]
> @@ -350,7 +352,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;

   This is a fix per se. Let's add the #define (if we really need it) in
another patch.

[...]

MBR, Sergei
David Miller Dec. 10, 2018, 7:44 p.m. UTC | #2 
From: Simon Horman <horms+renesas@verge.net.au>
Date: Mon, 10 Dec 2018 09:59:17 +0100

> +#define RAVB_CSUM_LEN 2
> +
 ...
>  	priv->rx_buf_sz = (ndev->mtu <= 1492 ? PKT_BUF_SZ : ndev->mtu) +
> -		ETH_HLEN + VLAN_HLEN;
> +		ETH_HLEN + VLAN_HLEN + RAVB_CSUM_LEN;
 ...
> +	if (unlikely(skb->len < RAVB_CSUM_LEN))
 ...
> -	hw_csum = skb_tail_pointer(skb) - 2;
> +	hw_csum = skb_tail_pointer(skb) - RAVB_CSUM_LEN;
 ...
> -	skb_trim(skb, skb->len - 2);
> +	skb_trim(skb, skb->len - RAVB_CSUM_LEN);

Unlike Sergei, I think this macro define should be kept in the fix.

It is absolutely crucial for anyone reading this code to understand
what this value is all about.

People reading the code aren't able to go automatically back to a
commit to learn what this value means, and even if they could they
shouldn't have to do so for a bunch of magic '2' constants placed all
over.

Even in the most fundamental way, the macro is required to satisfy
the "no magic constants" rule for kernel code.
Sergei Shtylyov Dec. 10, 2018, 7:51 p.m. UTC | #3 
On 12/10/2018 10:44 PM, David Miller wrote:

>> +#define RAVB_CSUM_LEN 2
>> +
>  ...
>>  	priv->rx_buf_sz = (ndev->mtu <= 1492 ? PKT_BUF_SZ : ndev->mtu) +
>> -		ETH_HLEN + VLAN_HLEN;
>> +		ETH_HLEN + VLAN_HLEN + RAVB_CSUM_LEN;
>  ...
>> +	if (unlikely(skb->len < RAVB_CSUM_LEN))
>  ...
>> -	hw_csum = skb_tail_pointer(skb) - 2;
>> +	hw_csum = skb_tail_pointer(skb) - RAVB_CSUM_LEN;
>  ...
>> -	skb_trim(skb, skb->len - 2);
>> +	skb_trim(skb, skb->len - RAVB_CSUM_LEN);
> 
> Unlike Sergei, I think this macro define should be kept in the fix.
> 
> It is absolutely crucial for anyone reading this code to understand
> what this value is all about.
> 
> People reading the code aren't able to go automatically back to a
> commit to learn what this value means, and even if they could they
> shouldn't have to do so for a bunch of magic '2' constants placed all
> over.

   We already have a comment in ravb_tx_csum(), I only asked for another one
(the place where we fix up the packet size). 
 
> Even in the most fundamental way, the macro is required to satisfy
> the "no magic constants" rule for kernel code.

   I'm also somewhat opposed to the RAVB_ prefix on something not really h/w
or driver specific... but I guess we don't have such #define anywhere in the
TCP/IP stack. Well, your call, anyway...

MBR. Sergei
Sergei Shtylyov Dec. 10, 2018, 8:11 p.m. UTC | #4 
On 12/10/2018 10:51 PM, Sergei Shtylyov wrote:

>>> +#define RAVB_CSUM_LEN 2
>>> +
>>  ...
>>>  	priv->rx_buf_sz = (ndev->mtu <= 1492 ? PKT_BUF_SZ : ndev->mtu) +
>>> -		ETH_HLEN + VLAN_HLEN;
>>> +		ETH_HLEN + VLAN_HLEN + RAVB_CSUM_LEN;
>>  ...
>>> +	if (unlikely(skb->len < RAVB_CSUM_LEN))
>>  ...
>>> -	hw_csum = skb_tail_pointer(skb) - 2;
>>> +	hw_csum = skb_tail_pointer(skb) - RAVB_CSUM_LEN;
>>  ...
>>> -	skb_trim(skb, skb->len - 2);
>>> +	skb_trim(skb, skb->len - RAVB_CSUM_LEN);
>>
>> Unlike Sergei, I think this macro define should be kept in the fix.
>>
>> It is absolutely crucial for anyone reading this code to understand
>> what this value is all about.
>>
>> People reading the code aren't able to go automatically back to a
>> commit to learn what this value means, and even if they could they
>> shouldn't have to do so for a bunch of magic '2' constants placed all
>> over.
> 
>    We already have a comment in ravb_tx_csum(), I only asked for another one

   Sorry, ravb_rx_csum(), of course. Those keys are dangerously close to each other. :-)

> (the place where we fix up the packet size). 

[...]

MBR. Sergei
Simon Horman Dec. 16, 2018, 8:59 p.m. UTC | #5 
On Mon, Dec 10, 2018 at 07:56:14PM +0300, Sergei Shtylyov wrote:
> Hello!
> 
> On 12/10/2018 11:59 AM, Simon Horman wrote:
> 
> > From: Simon Horman <horms@verge.net.au>
> > 
> > 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.
> 
>    I think a comment would work better in this case.
> 
> > Tested on R-Car E3 (r8a77990) ES1.0 based Ebisu-4D board
> 
>    You need more testing (RFT specified in the subject)?

I was hoping that others would test the patch on the same or other boards.
That is all.

> 
> > 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(-)
> > 
> >  I have marked this patch as RTF as I would like it to see further testing
> >  before being applied.
> > 
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> > index defed0d0c51d..f7f130cf61e4 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
> 
>    What's EtherAVB specific there, why RAVB prefix?
> 
> [...]
> > @@ -350,7 +352,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;
> 
>    This is a fix per se. Let's add the #define (if we really need it) in
> another patch.

It seems that David likes the #define, lets keep it.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index defed0d0c51d..f7f130cf61e4 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 */
@@ -350,7 +352,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],
@@ -533,13 +535,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 */