| Message ID | 9a2a2790acfd45bef2cd786dc92407bcd6c14448.camel@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: usb: ax88179_178a: corrects packet receiving | expand |
On Thu, 23 Jun 2022 11:06:05 -0300 Jose Alonso wrote:
> + * In current firmware there is 2 entries per packet.
Do you know what FW version you have?
No need to repost we can add it to the commit msg when applying.
On Thu, 2022-06-23 at 21:47 -0700, Jakub Kicinski wrote: > On Thu, 23 Jun 2022 11:06:05 -0300 Jose Alonso wrote: > > + * In current firmware there is 2 entries per packet. > > Do you know what FW version you have? I don't know.
On Thu, 2022-06-23 at 11:06 -0300, Jose Alonso wrote: > This patch corrects the packet receiving in ax88179_rx_fixup. > > - problem observed: > ifconfig shows always a lot of 'RX Errors' while packets > are received normally. > > This occurs because ax88179_rx_fixup does not recognize properly > the usb urb received. > The packets are normally processed and at the end, the code exits > with 'return 0', generating RX Errors. > (with pkt_cnt==-2 and ptk_hdr over the field rx_hdr trying to identify > another packet there) > > This is a usb urb received by "tcpdump -i usbmon2 -X": > 0x0000: eeee f8e3 3b19 87a0 94de 80e3 daac 0800 > ^ packet 1 start (pkt_len = 0x05ec) > ^^^^ IP alignment pseudo header > ^ ethernet frame start > last byte of packet vv > padding (8-bytes aligned) vvvv vvvv > 0x05e0: c92d d444 1420 8a69 83dd 272f e82b 9811 > 0x05f0: eeee f8e3 3b19 87a0 94de 80e3 daac 0800 > ... ^ packet 2 > 0x0be0: eeee f8e3 3b19 87a0 94de 80e3 daac 0800 > ... > 0x1130: 9d41 9171 8a38 0ec5 eeee f8e3 3b19 87a0 > ... > 0x1720: 8cfc 15ff 5e4c e85c eeee f8e3 3b19 87a0 > ... > 0x1d10: ecfa 2a3a 19ab c78c eeee f8e3 3b19 87a0 > ... > 0x2070: eeee f8e3 3b19 87a0 94de 80e3 daac 0800 > ... ^ packet 7 > 0x2120: 7c88 4ca5 5c57 7dcc 0d34 7577 f778 7e0a > 0x2130: f032 e093 7489 0740 3008 ec05 0000 0080 > ====1==== ====2==== > hdr_off ^ > pkt_len = 0x05ec ^^^^ > AX_RXHDR_*=0x00830 ^^^^ ^ > pkt_len = 0 ^^^^ > AX_RXHDR_DROP_ERR=0x80000000 ^^^^ ^ > 0x2140: 3008 ec05 0000 0080 3008 5805 0000 0080 > 0x2150: 3008 ec05 0000 0080 3008 ec05 0000 0080 > 0x2160: 3008 5803 0000 0080 3008 c800 0000 0080 > ===11==== ===12==== ===13==== ===14==== > 0x2170: 0000 0000 0e00 3821 > ^^^^ ^^^^ rx_hdr > ^^^^ pkt_cnt=14 > ^^^^ hdr_off=0x2138 > ^^^^ ^^^^ padding > > The dump shows that pkt_cnt is the number of entries in the > per-packet metadata. It is "2 * packet count". > Each packet have two entries. The first have a valid > value (pkt_len and AX_RXHDR_*) and the second have a > dummy-header 0x80000000 (pkt_len=0 with AX_RXHDR_DROP_ERR). > Why exists dummy-header for each packet?!? > My guess is that this was done probably to align the > entry for each packet to 64-bits and maintain compatibility > with old firmware. > There is also a padding (0x00000000) before the rx_hdr to > align the end of rx_hdr to 64-bit. > Note that packets have a alignment of 64-bits (8-bytes). > > This patch assumes that the dummy-header and the last > padding are optional. So it preserves semantics and > recognizes the same valid packets as the current code. > > This patch was made using only the dump file information and > tested with only one device: > 0b95:1790 ASIX Electronics Corp. AX88179 Gigabit Ethernet > > Signed-off-by: Jose Alonso <joalonsof@gmail.com> > > --- > > diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c > index 4704ed6f00ef..02bd113c5045 100644 > --- a/drivers/net/usb/ax88179_178a.c > +++ b/drivers/net/usb/ax88179_178a.c > @@ -1472,6 +1472,42 @@ static int ax88179_rx_fixup(struct usbnet *dev, struct sk_buff *skb) > * are bundled into this buffer and where we can find an array of > * per-packet metadata (which contains elements encoded into u16). > */ > + > + /* SKB contents for current firmware: > + * <packet 1> <padding> > + * ... > + * <packet N> <padding> > + * <per-packet metadata entry 1> <dummy header> > + * ... > + * <per-packet metadata entry N> <dummy header> > + * <padding2> <rx_hdr> > + * > + * where: > + * <packet N> contains pkt_len bytes: > + * 2 bytes of IP alignment pseudo header > + * packet received > + * <per-packet metadata entry N> contains 4 bytes: > + * pkt_len and fields AX_RXHDR_* > + * <padding> 0-7 bytes to terminate at > + * 8 bytes boundary (64-bit). > + * <padding2> 4 bytes to make rx_hdr terminate at > + * 8 bytes boundary (64-bit) > + * <dummy-header> contains 4 bytes: > + * pkt_len=0 and AX_RXHDR_DROP_ERR > + * <rx-hdr> contains 4 bytes: > + * pkt_cnt and hdr_off (offset of > + * <per-packet metadata entry 1>) > + * > + * pkt_cnt is number of entries in the per-packet metadata. > + * In current firmware there is 2 entries per packet. > + * The first points to the packet and the > + * second is a dummy header. > + * This was done probably to align fields in 64-bit and > + * maintain compatibility with old firmware. > + * This code assumes that <dummy header> and <padding2> are > + * optional. > + */ > + > if (skb->len < 4) > return 0; > skb_trim(skb, skb->len - 4); > @@ -1485,51 +1521,66 @@ static int ax88179_rx_fixup(struct usbnet *dev, struct sk_buff *skb) > /* Make sure that the bounds of the metadata array are inside the SKB > * (and in front of the counter at the end). > */ > - if (pkt_cnt * 2 + hdr_off > skb->len) > + if (pkt_cnt * 4 + hdr_off > skb->len) > return 0; > pkt_hdr = (u32 *)(skb->data + hdr_off); > > /* Packets must not overlap the metadata array */ > skb_trim(skb, hdr_off); > > - for (; ; pkt_cnt--, pkt_hdr++) { > + for (; pkt_cnt > 0; pkt_cnt--, pkt_hdr++) { > u16 pkt_len; > + u16 pkt_len_plus_padd; Very minor nit: please respect the reverse xmas tree order in variable declaration - in this case: u16 pkt_len_plus_padd; u16 pkt_len; Other then that, LGTM! Feel free to include my acked-by tag on re-spin. I understand this is targeting -net, if so please include the correct tag into the patch subj. Additionally adding a Fixes tag is advised, if your intention is push this patch to stable trees. Thanks! Paolo
diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c index 4704ed6f00ef..02bd113c5045 100644 --- a/drivers/net/usb/ax88179_178a.c +++ b/drivers/net/usb/ax88179_178a.c @@ -1472,6 +1472,42 @@ static int ax88179_rx_fixup(struct usbnet *dev, struct sk_buff *skb) * are bundled into this buffer and where we can find an array of * per-packet metadata (which contains elements encoded into u16). */ + + /* SKB contents for current firmware: + * <packet 1> <padding> + * ... + * <packet N> <padding> + * <per-packet metadata entry 1> <dummy header> + * ... + * <per-packet metadata entry N> <dummy header> + * <padding2> <rx_hdr> + * + * where: + * <packet N> contains pkt_len bytes: + * 2 bytes of IP alignment pseudo header + * packet received + * <per-packet metadata entry N> contains 4 bytes: + * pkt_len and fields AX_RXHDR_* + * <padding> 0-7 bytes to terminate at + * 8 bytes boundary (64-bit). + * <padding2> 4 bytes to make rx_hdr terminate at + * 8 bytes boundary (64-bit) + * <dummy-header> contains 4 bytes: + * pkt_len=0 and AX_RXHDR_DROP_ERR + * <rx-hdr> contains 4 bytes: + * pkt_cnt and hdr_off (offset of + * <per-packet metadata entry 1>) + * + * pkt_cnt is number of entries in the per-packet metadata. + * In current firmware there is 2 entries per packet. + * The first points to the packet and the + * second is a dummy header. + * This was done probably to align fields in 64-bit and + * maintain compatibility with old firmware. + * This code assumes that <dummy header> and <padding2> are + * optional. + */ + if (skb->len < 4) return 0; skb_trim(skb, skb->len - 4); @@ -1485,51 +1521,66 @@ static int ax88179_rx_fixup(struct usbnet *dev, struct sk_buff *skb) /* Make sure that the bounds of the metadata array are inside the SKB * (and in front of the counter at the end). */ - if (pkt_cnt * 2 + hdr_off > skb->len) + if (pkt_cnt * 4 + hdr_off > skb->len) return 0; pkt_hdr = (u32 *)(skb->data + hdr_off); /* Packets must not overlap the metadata array */ skb_trim(skb, hdr_off); - for (; ; pkt_cnt--, pkt_hdr++) { + for (; pkt_cnt > 0; pkt_cnt--, pkt_hdr++) { u16 pkt_len; + u16 pkt_len_plus_padd; le32_to_cpus(pkt_hdr); pkt_len = (*pkt_hdr >> 16) & 0x1fff; + pkt_len_plus_padd = (pkt_len + 7) & 0xfff8; - if (pkt_len > skb->len) + /* Skip dummy header used for alignment + */ + if (pkt_len == 0) + continue; + + if (pkt_len_plus_padd > skb->len) return 0; /* Check CRC or runt packet */ - if (((*pkt_hdr & (AX_RXHDR_CRC_ERR | AX_RXHDR_DROP_ERR)) == 0) && - pkt_len >= 2 + ETH_HLEN) { - bool last = (pkt_cnt == 0); - - if (last) { - ax_skb = skb; - } else { - ax_skb = skb_clone(skb, GFP_ATOMIC); - if (!ax_skb) - return 0; - } - ax_skb->len = 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); + if ((*pkt_hdr & (AX_RXHDR_CRC_ERR | AX_RXHDR_DROP_ERR)) || + pkt_len < 2 + ETH_HLEN) { + dev->net->stats.rx_errors++; + skb_pull(skb, pkt_len_plus_padd); + continue; + } - if (last) - return 1; + /* last packet */ + if (pkt_len_plus_padd == skb->len) { + skb_trim(skb, pkt_len); - usbnet_skb_return(dev, ax_skb); + /* Skip IP alignment pseudo header */ + skb_pull(skb, 2); + + skb->truesize = SKB_TRUESIZE(pkt_len_plus_padd); + ax88179_rx_checksum(skb, pkt_hdr); + return 1; } - /* Trim this packet away from the SKB */ - if (!skb_pull(skb, (pkt_len + 7) & 0xFFF8)) + ax_skb = skb_clone(skb, GFP_ATOMIC); + if (!ax_skb) return 0; + skb_trim(ax_skb, pkt_len); + + /* Skip IP alignment pseudo header */ + skb_pull(ax_skb, 2); + + skb->truesize = pkt_len_plus_padd + + SKB_DATA_ALIGN(sizeof(struct sk_buff)); + ax88179_rx_checksum(ax_skb, pkt_hdr); + usbnet_skb_return(dev, ax_skb); + + skb_pull(skb, pkt_len_plus_padd); } + + return 0; } static struct sk_buff *
This patch corrects the packet receiving in ax88179_rx_fixup. - problem observed: ifconfig shows always a lot of 'RX Errors' while packets are received normally. This occurs because ax88179_rx_fixup does not recognize properly the usb urb received. The packets are normally processed and at the end, the code exits with 'return 0', generating RX Errors. (with pkt_cnt==-2 and ptk_hdr over the field rx_hdr trying to identify another packet there) This is a usb urb received by "tcpdump -i usbmon2 -X": 0x0000: eeee f8e3 3b19 87a0 94de 80e3 daac 0800 ^ packet 1 start (pkt_len = 0x05ec) ^^^^ IP alignment pseudo header ^ ethernet frame start last byte of packet vv padding (8-bytes aligned) vvvv vvvv 0x05e0: c92d d444 1420 8a69 83dd 272f e82b 9811 0x05f0: eeee f8e3 3b19 87a0 94de 80e3 daac 0800 ... ^ packet 2 0x0be0: eeee f8e3 3b19 87a0 94de 80e3 daac 0800 ... 0x1130: 9d41 9171 8a38 0ec5 eeee f8e3 3b19 87a0 ... 0x1720: 8cfc 15ff 5e4c e85c eeee f8e3 3b19 87a0 ... 0x1d10: ecfa 2a3a 19ab c78c eeee f8e3 3b19 87a0 ... 0x2070: eeee f8e3 3b19 87a0 94de 80e3 daac 0800 ... ^ packet 7 0x2120: 7c88 4ca5 5c57 7dcc 0d34 7577 f778 7e0a 0x2130: f032 e093 7489 0740 3008 ec05 0000 0080 ====1==== ====2==== hdr_off ^ pkt_len = 0x05ec ^^^^ AX_RXHDR_*=0x00830 ^^^^ ^ pkt_len = 0 ^^^^ AX_RXHDR_DROP_ERR=0x80000000 ^^^^ ^ 0x2140: 3008 ec05 0000 0080 3008 5805 0000 0080 0x2150: 3008 ec05 0000 0080 3008 ec05 0000 0080 0x2160: 3008 5803 0000 0080 3008 c800 0000 0080 ===11==== ===12==== ===13==== ===14==== 0x2170: 0000 0000 0e00 3821 ^^^^ ^^^^ rx_hdr ^^^^ pkt_cnt=14 ^^^^ hdr_off=0x2138 ^^^^ ^^^^ padding The dump shows that pkt_cnt is the number of entries in the per-packet metadata. It is "2 * packet count". Each packet have two entries. The first have a valid value (pkt_len and AX_RXHDR_*) and the second have a dummy-header 0x80000000 (pkt_len=0 with AX_RXHDR_DROP_ERR). Why exists dummy-header for each packet?!? My guess is that this was done probably to align the entry for each packet to 64-bits and maintain compatibility with old firmware. There is also a padding (0x00000000) before the rx_hdr to align the end of rx_hdr to 64-bit. Note that packets have a alignment of 64-bits (8-bytes). This patch assumes that the dummy-header and the last padding are optional. So it preserves semantics and recognizes the same valid packets as the current code. This patch was made using only the dump file information and tested with only one device: 0b95:1790 ASIX Electronics Corp. AX88179 Gigabit Ethernet Signed-off-by: Jose Alonso <joalonsof@gmail.com> --- --