[RFC PATCH] net: usb: ax88179_178a: fix packet alignment padding

[RFC PATCH] net: usb: ax88179_178a: fix packet alignment padding

[Jeremy Kerr]

Using a AX88179 device (0b95:1790), I see two bytes of appended data on
every RX packet. For example, this 48-byte ping, using 0xff as a
payload byte:

  04:20:22.528472 IP 192.168.1.1 > 192.168.1.2: ICMP echo request, id 2447, seq 1, length 64
	0x0000:  000a cd35 ea50 000a cd35 ea4f 0800 4500
	0x0010:  0054 c116 4000 4001 f63e c0a8 0101 c0a8
	0x0020:  0102 0800 b633 098f 0001 87ea cd5e 0000
	0x0030:  0000 dcf2 0600 0000 0000 ffff ffff ffff
	0x0040:  ffff ffff ffff ffff ffff ffff ffff ffff
	0x0050:  ffff ffff ffff ffff ffff ffff ffff ffff
	0x0060:  ffff 961f

Those last two bytes - 96 1f - aren't part of the original packet.

In the ax88179 RX path, the usbnet rx_fixup function trims a 2-byte
'alignment pseudo header' from the start of the packet, and sets the
length from a per-packet field populated by hardware. It looks like that
length field *includes* the 2-byte header; the current driver assumes
that it's excluded.

This change trims the 2-byte alignment header after we've set the packet
length, so the resulting packet length is correct. While we're moving
the comment around, this also fixes the spelling of 'pseudo'.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>

---
RFC: I don't have access to docs for this hardware, so this is all based
on observed behaviour of the reported packet length.
---
 drivers/net/usb/ax88179_178a.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
index 93044cf1417a..1fe4cc28d154 100644
--- a/drivers/net/usb/ax88179_178a.c
+++ b/drivers/net/usb/ax88179_178a.c
@@ -1414,10 +1414,10 @@ static int ax88179_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
 		}
 
 		if (pkt_cnt == 0) {
-			/* Skip IP alignment psudo header */
-			skb_pull(skb, 2);
 			skb->len = pkt_len;
-			skb_set_tail_pointer(skb, pkt_len);
+			/* Skip IP alignment pseudo header */
+			skb_pull(skb, 2);
+			skb_set_tail_pointer(skb, skb->len);
 			skb->truesize = pkt_len + sizeof(struct sk_buff);
 			ax88179_rx_checksum(skb, pkt_hdr);
 			return 1;
@@ -1426,8 +1426,9 @@ static int ax88179_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
 		ax_skb = skb_clone(skb, GFP_ATOMIC);
 		if (ax_skb) {
 			ax_skb->len = pkt_len;
-			ax_skb->data = skb->data + 2;
-			skb_set_tail_pointer(ax_skb, pkt_len);
+			/* Skip IP alignment pseudo header */
+			skb_pull(ax_skb, 2);
+			skb_set_tail_pointer(ax_skb, ax_skb->len);
 			ax_skb->truesize = pkt_len + sizeof(struct sk_buff);
 			ax88179_rx_checksum(ax_skb, pkt_hdr);
 			usbnet_skb_return(dev, ax_skb);
-- 
2.17.1

Re: [RFC PATCH] net: usb: ax88179_178a: fix packet alignment padding

[Jeremy Kerr]

Hi Freddy and Allan,

Just following up on the RFC patch below: Can you confirm whether the
packet len (in the hardware-provided packet RX metadata) includes the
two-byte padding field? Is this the same for all ax88179 devices?

Cheers,


Jeremy

> Using a AX88179 device (0b95:1790), I see two bytes of appended data on
> every RX packet. For example, this 48-byte ping, using 0xff as a
> payload byte:
> 
>   04:20:22.528472 IP 192.168.1.1 > 192.168.1.2: ICMP echo request, id 2447, seq 1, length 64
> 	0x0000:  000a cd35 ea50 000a cd35 ea4f 0800 4500
> 	0x0010:  0054 c116 4000 4001 f63e c0a8 0101 c0a8
> 	0x0020:  0102 0800 b633 098f 0001 87ea cd5e 0000
> 	0x0030:  0000 dcf2 0600 0000 0000 ffff ffff ffff
> 	0x0040:  ffff ffff ffff ffff ffff ffff ffff ffff
> 	0x0050:  ffff ffff ffff ffff ffff ffff ffff ffff
> 	0x0060:  ffff 961f
> 
> Those last two bytes - 96 1f - aren't part of the original packet.
> 
> In the ax88179 RX path, the usbnet rx_fixup function trims a 2-byte
> 'alignment pseudo header' from the start of the packet, and sets the
> length from a per-packet field populated by hardware. It looks like that
> length field *includes* the 2-byte header; the current driver assumes
> that it's excluded.
> 
> This change trims the 2-byte alignment header after we've set the packet
> length, so the resulting packet length is correct. While we're moving
> the comment around, this also fixes the spelling of 'pseudo'.
> 
> Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
> 
> ---
> RFC: I don't have access to docs for this hardware, so this is all based
> on observed behaviour of the reported packet length.
> ---
>  drivers/net/usb/ax88179_178a.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
> index 93044cf1417a..1fe4cc28d154 100644
> --- a/drivers/net/usb/ax88179_178a.c
> +++ b/drivers/net/usb/ax88179_178a.c
> @@ -1414,10 +1414,10 @@ static int ax88179_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
>  		}
>  
>  		if (pkt_cnt == 0) {
> -			/* Skip IP alignment psudo header */
> -			skb_pull(skb, 2);
>  			skb->len = pkt_len;
> -			skb_set_tail_pointer(skb, pkt_len);
> +			/* Skip IP alignment pseudo header */
> +			skb_pull(skb, 2);
> +			skb_set_tail_pointer(skb, skb->len);
>  			skb->truesize = pkt_len + sizeof(struct sk_buff);
>  			ax88179_rx_checksum(skb, pkt_hdr);
>  			return 1;
> @@ -1426,8 +1426,9 @@ static int ax88179_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
>  		ax_skb = skb_clone(skb, GFP_ATOMIC);
>  		if (ax_skb) {
>  			ax_skb->len = pkt_len;
> -			ax_skb->data = skb->data + 2;
> -			skb_set_tail_pointer(ax_skb, pkt_len);
> +			/* Skip IP alignment pseudo header */
> +			skb_pull(ax_skb, 2);
> +			skb_set_tail_pointer(ax_skb, ax_skb->len);
>  			ax_skb->truesize = pkt_len + sizeof(struct sk_buff);
>  			ax88179_rx_checksum(ax_skb, pkt_hdr);
>  			usbnet_skb_return(dev, ax_skb);
> 

RE: [RFC PATCH] net: usb: ax88179_178a: fix packet alignment padding

[ASIX_Allan [Office]]

Hi Louis,

Please help to take care of this problem. Thanks a lot.

 
---
Best regards,
Allan Chou
ASIX Electronics Corporation
TEL: 886-3-5799500 ext.228
FAX: 886-3-5799558
E-mail: allan@asix.com.tw 
https://www.asix.com.tw/ 



-----Original Message-----
From: Jeremy Kerr <jk@ozlabs.org> 
Sent: Tuesday, June 2, 2020 11:18 AM
To: Freddy Xin <freddy@asix.com.tw>; Allan Chou <allan@asix.com.tw>
Cc: Peter Fink <pfink@christ-es.de>; netdev@vger.kernel.org; linux-usb@vger.kernel.org
Subject: Re: [RFC PATCH] net: usb: ax88179_178a: fix packet alignment padding

Hi Freddy and Allan,

Just following up on the RFC patch below: Can you confirm whether the packet len (in the hardware-provided packet RX metadata) includes the two-byte padding field? Is this the same for all ax88179 devices?

Cheers,


Jeremy

> Using a AX88179 device (0b95:1790), I see two bytes of appended data 
> on every RX packet. For example, this 48-byte ping, using 0xff as a 
> payload byte:
> 
>   04:20:22.528472 IP 192.168.1.1 > 192.168.1.2: ICMP echo request, id 2447, seq 1, length 64
> 	0x0000:  000a cd35 ea50 000a cd35 ea4f 0800 4500
> 	0x0010:  0054 c116 4000 4001 f63e c0a8 0101 c0a8
> 	0x0020:  0102 0800 b633 098f 0001 87ea cd5e 0000
> 	0x0030:  0000 dcf2 0600 0000 0000 ffff ffff ffff
> 	0x0040:  ffff ffff ffff ffff ffff ffff ffff ffff
> 	0x0050:  ffff ffff ffff ffff ffff ffff ffff ffff
> 	0x0060:  ffff 961f
> 
> Those last two bytes - 96 1f - aren't part of the original packet.
> 
> In the ax88179 RX path, the usbnet rx_fixup function trims a 2-byte 
> 'alignment pseudo header' from the start of the packet, and sets the 
> length from a per-packet field populated by hardware. It looks like 
> that length field *includes* the 2-byte header; the current driver 
> assumes that it's excluded.
> 
> This change trims the 2-byte alignment header after we've set the 
> packet length, so the resulting packet length is correct. While we're 
> moving the comment around, this also fixes the spelling of 'pseudo'.
> 
> Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
> 
> ---
> RFC: I don't have access to docs for this hardware, so this is all 
> based on observed behaviour of the reported packet length.
> ---
>  drivers/net/usb/ax88179_178a.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/usb/ax88179_178a.c 
> b/drivers/net/usb/ax88179_178a.c index 93044cf1417a..1fe4cc28d154 
> 100644
> --- a/drivers/net/usb/ax88179_178a.c
> +++ b/drivers/net/usb/ax88179_178a.c
> @@ -1414,10 +1414,10 @@ static int ax88179_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
>  		}
>  
>  		if (pkt_cnt == 0) {
> -			/* Skip IP alignment psudo header */
> -			skb_pull(skb, 2);
>  			skb->len = pkt_len;
> -			skb_set_tail_pointer(skb, pkt_len);
> +			/* Skip IP alignment pseudo header */
> +			skb_pull(skb, 2);
> +			skb_set_tail_pointer(skb, skb->len);
>  			skb->truesize = pkt_len + sizeof(struct sk_buff);
>  			ax88179_rx_checksum(skb, pkt_hdr);
>  			return 1;
> @@ -1426,8 +1426,9 @@ static int ax88179_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
>  		ax_skb = skb_clone(skb, GFP_ATOMIC);
>  		if (ax_skb) {
>  			ax_skb->len = pkt_len;
> -			ax_skb->data = skb->data + 2;
> -			skb_set_tail_pointer(ax_skb, pkt_len);
> +			/* Skip IP alignment pseudo header */
> +			skb_pull(ax_skb, 2);
> +			skb_set_tail_pointer(ax_skb, ax_skb->len);
>  			ax_skb->truesize = pkt_len + sizeof(struct sk_buff);
>  			ax88179_rx_checksum(ax_skb, pkt_hdr);
>  			usbnet_skb_return(dev, ax_skb);
> 

RE: [RFC PATCH] net: usb: ax88179_178a: fix packet alignment padding

[louis]

Hi Jeremy,

Thanks for the correction.
Indeed, the hardware adds two bytes dummy data at beginning of Ethernet packet to make IP header aligned. 
The original patch made by Freddy contains the length of dummy header. 
Thanks for your concerning.
Please let us know if you have any other question.

Thank you,
Louis.

---
Best regards,
Allan Chou
ASIX Electronics Corporation
TEL: 886-3-5799500 ext.228
FAX: 886-3-5799558
E-mail: allan@asix.com.tw 
https://www.asix.com.tw/ 



-----Original Message-----
From: Jeremy Kerr <jk@ozlabs.org> 
Sent: Tuesday, June 2, 2020 11:18 AM
To: Freddy Xin <freddy@asix.com.tw>; Allan Chou <allan@asix.com.tw>
Cc: Peter Fink <pfink@christ-es.de>; netdev@vger.kernel.org; linux-usb@vger.kernel.org
Subject: Re: [RFC PATCH] net: usb: ax88179_178a: fix packet alignment padding

Hi Freddy and Allan,

Just following up on the RFC patch below: Can you confirm whether the packet len (in the hardware-provided packet RX metadata) includes the two-byte padding field? Is this the same for all ax88179 devices?

Cheers,


Jeremy

> Using a AX88179 device (0b95:1790), I see two bytes of appended data 
> on every RX packet. For example, this 48-byte ping, using 0xff as a 
> payload byte:
> 
>   04:20:22.528472 IP 192.168.1.1 > 192.168.1.2: ICMP echo request, id 2447, seq 1, length 64
> 	0x0000:  000a cd35 ea50 000a cd35 ea4f 0800 4500
> 	0x0010:  0054 c116 4000 4001 f63e c0a8 0101 c0a8
> 	0x0020:  0102 0800 b633 098f 0001 87ea cd5e 0000
> 	0x0030:  0000 dcf2 0600 0000 0000 ffff ffff ffff
> 	0x0040:  ffff ffff ffff ffff ffff ffff ffff ffff
> 	0x0050:  ffff ffff ffff ffff ffff ffff ffff ffff
> 	0x0060:  ffff 961f
> 
> Those last two bytes - 96 1f - aren't part of the original packet.
> 
> In the ax88179 RX path, the usbnet rx_fixup function trims a 2-byte 
> 'alignment pseudo header' from the start of the packet, and sets the 
> length from a per-packet field populated by hardware. It looks like 
> that length field *includes* the 2-byte header; the current driver 
> assumes that it's excluded.
> 
> This change trims the 2-byte alignment header after we've set the 
> packet length, so the resulting packet length is correct. While we're 
> moving the comment around, this also fixes the spelling of 'pseudo'.
> 
> Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
> 
> ---
> RFC: I don't have access to docs for this hardware, so this is all 
> based on observed behaviour of the reported packet length.
> ---
>  drivers/net/usb/ax88179_178a.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/usb/ax88179_178a.c 
> b/drivers/net/usb/ax88179_178a.c index 93044cf1417a..1fe4cc28d154 
> 100644
> --- a/drivers/net/usb/ax88179_178a.c
> +++ b/drivers/net/usb/ax88179_178a.c
> @@ -1414,10 +1414,10 @@ static int ax88179_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
>  		}
>  
>  		if (pkt_cnt == 0) {
> -			/* Skip IP alignment psudo header */
> -			skb_pull(skb, 2);
>  			skb->len = pkt_len;
> -			skb_set_tail_pointer(skb, pkt_len);
> +			/* Skip IP alignment pseudo header */
> +			skb_pull(skb, 2);
> +			skb_set_tail_pointer(skb, skb->len);
>  			skb->truesize = pkt_len + sizeof(struct sk_buff);
>  			ax88179_rx_checksum(skb, pkt_hdr);
>  			return 1;
> @@ -1426,8 +1426,9 @@ static int ax88179_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
>  		ax_skb = skb_clone(skb, GFP_ATOMIC);
>  		if (ax_skb) {
>  			ax_skb->len = pkt_len;
> -			ax_skb->data = skb->data + 2;
> -			skb_set_tail_pointer(ax_skb, pkt_len);
> +			/* Skip IP alignment pseudo header */
> +			skb_pull(ax_skb, 2);
> +			skb_set_tail_pointer(ax_skb, ax_skb->len);
>  			ax_skb->truesize = pkt_len + sizeof(struct sk_buff);
>  			ax88179_rx_checksum(ax_skb, pkt_hdr);
>  			usbnet_skb_return(dev, ax_skb);
> 

Re: [RFC PATCH] net: usb: ax88179_178a: fix packet alignment padding

[Jeremy Kerr]

Hi Louis,

> Thanks for the correction.
> Indeed, the hardware adds two bytes dummy data at beginning of
> Ethernet packet to make IP header aligned. 
> The original patch made by Freddy contains the length of dummy
> header. 

OK, thanks for checking. I understand this to mean that the fix is
correct, so I'll send it upstream without the RFC tag.

Regards,


Jeremy