| Message ID | 1414494104-27943-7-git-send-email-LW@KARO-electronics.de |
|---|---|
| State | New |
| Headers | show |
From: Lothar Waßmann > commit 1b7bde6d659d ("net: fec: implement rx_copybreak to improve rx performance") > introduced a regression for i.MX28. The swap_buffer() function doing > the endian conversion of the received data on i.MX28 may access memory > beyond the actual packet size in the DMA buffer. fec_enet_copybreak() > does not copy those bytes, so that the last bytes of a packet may be > filled with invalid data after swapping. > This will likely lead to checksum errors on received packets. > E.g. when trying to mount an NFS rootfs: > UDP: bad checksum. From 192.168.1.225:111 to 192.168.100.73:44662 ulen 36 > > Do the byte swapping and copying to the new skb in one go if > necessary. > > Signed-off-by: Lothar Wamann <LW@KARO-electronics.de> > --- > drivers/net/ethernet/freescale/fec_main.c | 25 +++++++++++++++++++++---- > 1 file changed, 21 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c > index 404fb9d..b92324c 100644 > --- a/drivers/net/ethernet/freescale/fec_main.c > +++ b/drivers/net/ethernet/freescale/fec_main.c > @@ -339,6 +339,18 @@ static void *swap_buffer(void *bufaddr, int len) > return bufaddr; > } > > +static void *swap_buffer2(void *dst_buf, void *src_buf, int len) > +{ > + int i; > + unsigned int *src = src_buf; > + unsigned int *dst = dst_buf; > + > + for (i = 0; i < len; i += 4, src++, dst++) > + swab32s(src); This will probably benefit from being unrolled slightly. Neither 'dst' nor the return value is used. > + > + return dst_buf; > +} > + > static void fec_dump(struct net_device *ndev) > { > struct fec_enet_private *fep = netdev_priv(ndev); > @@ -1334,7 +1346,7 @@ fec_enet_new_rxbdp(struct net_device *ndev, struct bufdesc *bdp, struct sk_buff > } > > static bool fec_enet_copybreak(struct net_device *ndev, struct sk_buff **skb, > - struct bufdesc *bdp, u32 length) > + struct bufdesc *bdp, u32 length, bool swap) > { > struct fec_enet_private *fep = netdev_priv(ndev); > struct sk_buff *new_skb; > @@ -1349,7 +1361,10 @@ static bool fec_enet_copybreak(struct net_device *ndev, struct sk_buff **skb, > dma_sync_single_for_cpu(&fep->pdev->dev, bdp->cbd_bufaddr, > FEC_ENET_RX_FRSIZE - fep->rx_align, > DMA_FROM_DEVICE); > - memcpy(new_skb->data, (*skb)->data, length); > + if (!swap) > + memcpy(new_skb->data, (*skb)->data, length); > + else > + swap_buffer2(new_skb->data, (*skb)->data, length); > *skb = new_skb; > > return true; > @@ -1377,6 +1392,7 @@ fec_enet_rx_queue(struct net_device *ndev, int budget, u16 queue_id) > u16 vlan_tag; > int index = 0; > bool is_copybreak; > + bool need_swap = fep->quirks & FEC_QUIRK_SWAP_FRAME; > > #ifdef CONFIG_M532x > flush_cache_all(); > @@ -1440,7 +1456,8 @@ fec_enet_rx_queue(struct net_device *ndev, int budget, u16 queue_id) > * include that when passing upstream as it messes up > * bridging applications. > */ > - is_copybreak = fec_enet_copybreak(ndev, &skb, bdp, pkt_len - 4); > + is_copybreak = fec_enet_copybreak(ndev, &skb, bdp, pkt_len - 4, > + need_swap); > if (!is_copybreak) { > skb_new = netdev_alloc_skb(ndev, FEC_ENET_RX_FRSIZE); > if (unlikely(!skb_new)) { > @@ -1455,7 +1472,7 @@ fec_enet_rx_queue(struct net_device *ndev, int budget, u16 queue_id) > prefetch(skb->data - NET_IP_ALIGN); > skb_put(skb, pkt_len - 4); > data = skb->data; > - if (fep->quirks & FEC_QUIRK_SWAP_FRAME) > + if (!is_copybreak && need_swap) > swap_buffer(data, pkt_len); It has to be better to set the 'copybreak' limit to be larger than the maximum frame size and so always go through the 'copybreak' paths. > > /* Extract the enhanced buffer descriptor */ > -- > 1.7.10.4 > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Lothar Waßmann > commit 1b7bde6d659d ("net: fec: implement rx_copybreak to improve rx performance") > introduced a regression for i.MX28. The swap_buffer() function doing > the endian conversion of the received data on i.MX28 may access memory > beyond the actual packet size in the DMA buffer. fec_enet_copybreak() > does not copy those bytes, so that the last bytes of a packet may be > filled with invalid data after swapping. > This will likely lead to checksum errors on received packets. > E.g. when trying to mount an NFS rootfs: > UDP: bad checksum. From 192.168.1.225:111 to 192.168.100.73:44662 ulen 36 > > Do the byte swapping and copying to the new skb in one go if > necessary. > > Signed-off-by: Lothar Wamann <LW@KARO-electronics.de> > --- > drivers/net/ethernet/freescale/fec_main.c | 25 +++++++++++++++++++++---- > 1 file changed, 21 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c > index 404fb9d..b92324c 100644 > --- a/drivers/net/ethernet/freescale/fec_main.c > +++ b/drivers/net/ethernet/freescale/fec_main.c > @@ -339,6 +339,18 @@ static void *swap_buffer(void *bufaddr, int len) > return bufaddr; > } > > +static void *swap_buffer2(void *dst_buf, void *src_buf, int len) > +{ > + int i; > + unsigned int *src = src_buf; > + unsigned int *dst = dst_buf; > + > + for (i = 0; i < len; i += 4, src++, dst++) > + swab32s(src); > + > + return dst_buf; > +} > + Actually that is completely f*cked.... David
Hi, David Laight wrote: > From: Lothar Waßmann > > commit 1b7bde6d659d ("net: fec: implement rx_copybreak to improve rx performance") > > introduced a regression for i.MX28. The swap_buffer() function doing > > the endian conversion of the received data on i.MX28 may access memory > > beyond the actual packet size in the DMA buffer. fec_enet_copybreak() > > does not copy those bytes, so that the last bytes of a packet may be > > filled with invalid data after swapping. > > This will likely lead to checksum errors on received packets. > > E.g. when trying to mount an NFS rootfs: > > UDP: bad checksum. From 192.168.1.225:111 to 192.168.100.73:44662 ulen 36 > > > > Do the byte swapping and copying to the new skb in one go if > > necessary. > > > > Signed-off-by: Lothar Wamann <LW@KARO-electronics.de> > > --- > > drivers/net/ethernet/freescale/fec_main.c | 25 +++++++++++++++++++++---- > > 1 file changed, 21 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c > > index 404fb9d..b92324c 100644 > > --- a/drivers/net/ethernet/freescale/fec_main.c > > +++ b/drivers/net/ethernet/freescale/fec_main.c > > @@ -339,6 +339,18 @@ static void *swap_buffer(void *bufaddr, int len) > > return bufaddr; > > } > > > > +static void *swap_buffer2(void *dst_buf, void *src_buf, int len) > > +{ > > + int i; > > + unsigned int *src = src_buf; > > + unsigned int *dst = dst_buf; > > + > > + for (i = 0; i < len; i += 4, src++, dst++) > > + swab32s(src); > > + > > + return dst_buf; > > +} > > + > > Actually that is completely f*cked.... > Yeah, noticed that shortly after sending out. :( One change too many... Lothar Waßmann
Hi, David Laight wrote: > From: Lothar Waßmann > > commit 1b7bde6d659d ("net: fec: implement rx_copybreak to improve rx performance") > > introduced a regression for i.MX28. The swap_buffer() function doing > > the endian conversion of the received data on i.MX28 may access memory > > beyond the actual packet size in the DMA buffer. fec_enet_copybreak() > > does not copy those bytes, so that the last bytes of a packet may be > > filled with invalid data after swapping. > > This will likely lead to checksum errors on received packets. > > E.g. when trying to mount an NFS rootfs: > > UDP: bad checksum. From 192.168.1.225:111 to 192.168.100.73:44662 ulen 36 > > > > Do the byte swapping and copying to the new skb in one go if > > necessary. > > > > Signed-off-by: Lothar Wamann <LW@KARO-electronics.de> > > --- > > drivers/net/ethernet/freescale/fec_main.c | 25 +++++++++++++++++++++---- > > 1 file changed, 21 insertions(+), 4 deletions(-) > > > > @@ -1455,7 +1472,7 @@ fec_enet_rx_queue(struct net_device *ndev, int budget, u16 queue_id) > > prefetch(skb->data - NET_IP_ALIGN); > > skb_put(skb, pkt_len - 4); > > data = skb->data; > > - if (fep->quirks & FEC_QUIRK_SWAP_FRAME) > > + if (!is_copybreak && need_swap) > > swap_buffer(data, pkt_len); > > It has to be better to set the 'copybreak' limit to be larger than the > maximum frame size and so always go through the 'copybreak' paths. > Since the copybreak support is all about performance optimistation, we should IMO buy the additional advantage for i.MX28 by not having to access the buffer twice (once for copying and once again for swapping). Lothar Waßmann
From: Lothar Waßmann > David Laight wrote: > > From: Lothar Waßmann > > > commit 1b7bde6d659d ("net: fec: implement rx_copybreak to improve rx performance") > > > introduced a regression for i.MX28. The swap_buffer() function doing > > > the endian conversion of the received data on i.MX28 may access memory > > > beyond the actual packet size in the DMA buffer. fec_enet_copybreak() > > > does not copy those bytes, so that the last bytes of a packet may be > > > filled with invalid data after swapping. > > > This will likely lead to checksum errors on received packets. > > > E.g. when trying to mount an NFS rootfs: > > > UDP: bad checksum. From 192.168.1.225:111 to 192.168.100.73:44662 ulen 36 > > > > > > Do the byte swapping and copying to the new skb in one go if > > > necessary. > > > > > > Signed-off-by: Lothar Wamann <LW@KARO-electronics.de> > > > --- > > > drivers/net/ethernet/freescale/fec_main.c | 25 +++++++++++++++++++++---- > > > 1 file changed, 21 insertions(+), 4 deletions(-) > > > > > > @@ -1455,7 +1472,7 @@ fec_enet_rx_queue(struct net_device *ndev, int budget, u16 queue_id) > > > prefetch(skb->data - NET_IP_ALIGN); > > > skb_put(skb, pkt_len - 4); > > > data = skb->data; > > > - if (fep->quirks & FEC_QUIRK_SWAP_FRAME) > > > + if (!is_copybreak && need_swap) > > > swap_buffer(data, pkt_len); > > > > It has to be better to set the 'copybreak' limit to be larger than the > > maximum frame size and so always go through the 'copybreak' paths. > > > Since the copybreak support is all about performance optimistation, we > should IMO buy the additional advantage for i.MX28 by not having to > access the buffer twice (once for copying and once again for swapping). You definitely want to do the byteswap at the same time as the copy. The point I'm trying to make that if you need to do the byteswap you probably might as well copy the data to an skb of the correct size at the same time. Certainly I'd expect the 'break even' length will be much higher. David
Hi, David Laight wrote: > From: Lothar Waßmann > > David Laight wrote: > > > From: Lothar Waßmann > > > > commit 1b7bde6d659d ("net: fec: implement rx_copybreak to improve rx performance") > > > > introduced a regression for i.MX28. The swap_buffer() function doing > > > > the endian conversion of the received data on i.MX28 may access memory > > > > beyond the actual packet size in the DMA buffer. fec_enet_copybreak() > > > > does not copy those bytes, so that the last bytes of a packet may be > > > > filled with invalid data after swapping. > > > > This will likely lead to checksum errors on received packets. > > > > E.g. when trying to mount an NFS rootfs: > > > > UDP: bad checksum. From 192.168.1.225:111 to 192.168.100.73:44662 ulen 36 > > > > > > > > Do the byte swapping and copying to the new skb in one go if > > > > necessary. > > > > > > > > Signed-off-by: Lothar Wamann <LW@KARO-electronics.de> > > > > --- > > > > drivers/net/ethernet/freescale/fec_main.c | 25 +++++++++++++++++++++---- > > > > 1 file changed, 21 insertions(+), 4 deletions(-) > > > > > > > > @@ -1455,7 +1472,7 @@ fec_enet_rx_queue(struct net_device *ndev, int budget, u16 queue_id) > > > > prefetch(skb->data - NET_IP_ALIGN); > > > > skb_put(skb, pkt_len - 4); > > > > data = skb->data; > > > > - if (fep->quirks & FEC_QUIRK_SWAP_FRAME) > > > > + if (!is_copybreak && need_swap) > > > > swap_buffer(data, pkt_len); > > > > > > It has to be better to set the 'copybreak' limit to be larger than the > > > maximum frame size and so always go through the 'copybreak' paths. > > > > > Since the copybreak support is all about performance optimistation, we > > should IMO buy the additional advantage for i.MX28 by not having to > > access the buffer twice (once for copying and once again for swapping). > > You definitely want to do the byteswap at the same time as the copy. > > The point I'm trying to make that if you need to do the byteswap you > probably might as well copy the data to an skb of the correct size at > the same time. > Certainly I'd expect the 'break even' length will be much higher. > I didn't implement the copybreak support. I'm only trying to fix a bug it introduced for i.MX28. So, I won't mess with the copybreak parameters. Lothar Waßmann
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index 404fb9d..b92324c 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -339,6 +339,18 @@ static void *swap_buffer(void *bufaddr, int len) return bufaddr; } +static void *swap_buffer2(void *dst_buf, void *src_buf, int len) +{ + int i; + unsigned int *src = src_buf; + unsigned int *dst = dst_buf; + + for (i = 0; i < len; i += 4, src++, dst++) + swab32s(src); + + return dst_buf; +} + static void fec_dump(struct net_device *ndev) { struct fec_enet_private *fep = netdev_priv(ndev); @@ -1334,7 +1346,7 @@ fec_enet_new_rxbdp(struct net_device *ndev, struct bufdesc *bdp, struct sk_buff } static bool fec_enet_copybreak(struct net_device *ndev, struct sk_buff **skb, - struct bufdesc *bdp, u32 length) + struct bufdesc *bdp, u32 length, bool swap) { struct fec_enet_private *fep = netdev_priv(ndev); struct sk_buff *new_skb; @@ -1349,7 +1361,10 @@ static bool fec_enet_copybreak(struct net_device *ndev, struct sk_buff **skb, dma_sync_single_for_cpu(&fep->pdev->dev, bdp->cbd_bufaddr, FEC_ENET_RX_FRSIZE - fep->rx_align, DMA_FROM_DEVICE); - memcpy(new_skb->data, (*skb)->data, length); + if (!swap) + memcpy(new_skb->data, (*skb)->data, length); + else + swap_buffer2(new_skb->data, (*skb)->data, length); *skb = new_skb; return true; @@ -1377,6 +1392,7 @@ fec_enet_rx_queue(struct net_device *ndev, int budget, u16 queue_id) u16 vlan_tag; int index = 0; bool is_copybreak; + bool need_swap = fep->quirks & FEC_QUIRK_SWAP_FRAME; #ifdef CONFIG_M532x flush_cache_all(); @@ -1440,7 +1456,8 @@ fec_enet_rx_queue(struct net_device *ndev, int budget, u16 queue_id) * include that when passing upstream as it messes up * bridging applications. */ - is_copybreak = fec_enet_copybreak(ndev, &skb, bdp, pkt_len - 4); + is_copybreak = fec_enet_copybreak(ndev, &skb, bdp, pkt_len - 4, + need_swap); if (!is_copybreak) { skb_new = netdev_alloc_skb(ndev, FEC_ENET_RX_FRSIZE); if (unlikely(!skb_new)) { @@ -1455,7 +1472,7 @@ fec_enet_rx_queue(struct net_device *ndev, int budget, u16 queue_id) prefetch(skb->data - NET_IP_ALIGN); skb_put(skb, pkt_len - 4); data = skb->data; - if (fep->quirks & FEC_QUIRK_SWAP_FRAME) + if (!is_copybreak && need_swap) swap_buffer(data, pkt_len); /* Extract the enhanced buffer descriptor */
commit 1b7bde6d659d ("net: fec: implement rx_copybreak to improve rx performance") introduced a regression for i.MX28. The swap_buffer() function doing the endian conversion of the received data on i.MX28 may access memory beyond the actual packet size in the DMA buffer. fec_enet_copybreak() does not copy those bytes, so that the last bytes of a packet may be filled with invalid data after swapping. This will likely lead to checksum errors on received packets. E.g. when trying to mount an NFS rootfs: UDP: bad checksum. From 192.168.1.225:111 to 192.168.100.73:44662 ulen 36 Do the byte swapping and copying to the new skb in one go if necessary. Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> --- drivers/net/ethernet/freescale/fec_main.c | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-)