Message ID | 20200615025456.30219-1-jk@ozlabs.org |
---|---|
State | Accepted |
Delegated to: | David Miller |
Headers | show |
Series | net: usb: ax88179_178a: fix packet alignment padding | expand |
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?
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?
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
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.
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
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)
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.
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.
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);
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(-)