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

[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>
---
 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: [PATCH] net: usb: ax88179_178a: fix packet alignment padding

[David Miller]

From: Jeremy Kerr <jk@ozlabs.org>
Date: Mon, 15 Jun 2020 10:54:56 +0800

> 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.

Does this happen for non-tail packets in a multi-packet cluster?

Because that code in this loop makes the same calculations:

		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);
			ax_skb->truesize = pkt_len + sizeof(struct sk_buff);
			ax88179_rx_checksum(ax_skb, pkt_hdr);
			usbnet_skb_return(dev, ax_skb);

So if your change is right, it should be applied to this code block as
well.

And do we know that it's two extra tail bytes always?  Or some kind of
alignment padding the chip performs for every sub-packet?

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

[ASIX_Allan [Office]]

Added ASIX S/W, Louis in the CC loop. 

 
---
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: David Miller <davem@davemloft.net> 
Sent: Tuesday, June 16, 2020 3:52 AM
To: jk@ozlabs.org
Cc: netdev@vger.kernel.org; allan@asix.com.tw; freddy@asix.com.tw; pfink@christ-es.de; linux-usb@vger.kernel.org
Subject: Re: [PATCH] net: usb: ax88179_178a: fix packet alignment padding

From: Jeremy Kerr <jk@ozlabs.org>
Date: Mon, 15 Jun 2020 10:54:56 +0800

> 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.

Does this happen for non-tail packets in a multi-packet cluster?

Because that code in this loop makes the same calculations:

		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);
			ax_skb->truesize = pkt_len + sizeof(struct sk_buff);
			ax88179_rx_checksum(ax_skb, pkt_hdr);
			usbnet_skb_return(dev, ax_skb);

So if your change is right, it should be applied to this code block as well.

And do we know that it's two extra tail bytes always?  Or some kind of alignment padding the chip performs for every sub-packet?

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

[Jeremy Kerr]

Hi David,

> > Those last two bytes - 96 1f - aren't part of the original packet.
> 
> Does this happen for non-tail packets in a multi-packet cluster?

I believe so, yes. I haven't been able to reliably reproduce the multi-
packet behaviour though, so input from ASIX would be good.

> 
> Because that code in this loop makes the same calculations:
> 
>                 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);
>                         ax_skb->truesize = pkt_len + sizeof(struct sk_buff);
>                         ax88179_rx_checksum(ax_skb, pkt_hdr);
>                         usbnet_skb_return(dev, ax_skb);
> 
> So if your change is right, it should be applied to this code block
> as well.

Yep, my patch changes that block too (or did I miss something?)

> And do we know that it's two extra tail bytes always?  Or some kind
> of alignment padding the chip performs for every sub-packet?

I've assumed it's a constant two bytes of prefix padding, as that's all
I've seen. But it would be great to have more detail from ASIX if
possible.

Cheers,


Jeremy

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

[David Miller]

From: Jeremy Kerr <jk@ozlabs.org>
Date: Tue, 16 Jun 2020 17:08:23 +0800

>> Because that code in this loop makes the same calculations:
>> 
>>                 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);
>>                         ax_skb->truesize = pkt_len + sizeof(struct sk_buff);
>>                         ax88179_rx_checksum(ax_skb, pkt_hdr);
>>                         usbnet_skb_return(dev, ax_skb);
>> 
>> So if your change is right, it should be applied to this code block
>> as well.
> 
> Yep, my patch changes that block too (or did I miss something?)

Nope, you didn't miss anything.  I missed that completely.

>> And do we know that it's two extra tail bytes always?  Or some kind
>> of alignment padding the chip performs for every sub-packet?
> 
> I've assumed it's a constant two bytes of prefix padding, as that's all
> I've seen. But it would be great to have more detail from ASIX if
> possible.

I'll wait a bit for the ASIX folks to comment.

It seems logical to me that what the chip does is align up the total
sub-packet length to a multiple of 4 or larger, and then add those two
prefix padding bytes.  Otherwise the prefix padding won't necessarily
and reliably align the IP header after the link level header.

Thanks.

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

[Jeremy Kerr]

Hi David,

> It seems logical to me that what the chip does is align up the total
> sub-packet length to a multiple of 4 or larger, and then add those two
> prefix padding bytes.  Otherwise the prefix padding won't necessarily
> and reliably align the IP header after the link level header.

Yep, that makes sense, and is what the driver is currently doing;
between clustered packets, the header is aligned (up) to 8 bytes, then
the 2-byte padding is added to that.

For this change, I have assumed that the packet length behaviour (ie,
describing the un-padded length) is consistent between clustered
packets.

[If you have any hints for forcing clustered packets, I'll see if I can
probe the behaviour a little better to confirm]

Cheers,


Jeremy

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

[David Laight]

From: Jeremy Kerr
> Sent: 15 June 2020 03:55

While you are looking as this driver you might want to worry
about what it sets skp->truesize to.

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

IIRC the memory allocated to the packet is likely to be over 64k.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

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

[David Miller]

From: Jeremy Kerr <jk@ozlabs.org>
Date: Mon, 15 Jun 2020 10:54:56 +0800

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

Applied and queued up for -stable.

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

[David Miller]

From: Jeremy Kerr <jk@ozlabs.org>
Date: Wed, 17 Jun 2020 08:56:39 +0800

> [If you have any hints for forcing clustered packets, I'll see if I can
> probe the behaviour a little better to confirm]

Probably it would involve having packets arrive back to back faster
than some interval that either depends upon real time or some multiple
of a USB bus cycle.