| Message ID | 1412687977-11742-1-git-send-email-LW@KARO-electronics.de |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| 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. ISTM that if you need to do the 'swap' you should copy the data regardless of the length. David > > 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 87975b5..eaaebad 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 < DIV_ROUND_UP(len, 4); i++, src++, dst++) > + *dst = cpu_to_be32(*src); > + > + return dst_buf; > +} > + > static void fec_dump(struct net_device *ndev) > { > struct fec_enet_private *fep = netdev_priv(ndev); > @@ -1348,7 +1360,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, int swap) > { > struct fec_enet_private *fep = netdev_priv(ndev); > struct sk_buff *new_skb; > @@ -1363,7 +1375,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; > @@ -1393,6 +1408,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 = id_entry->driver_data & FEC_QUIRK_SWAP_FRAME; > > #ifdef CONFIG_M532x > flush_cache_all(); > @@ -1456,7 +1472,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)) { > @@ -1471,7 +1488,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 (id_entry->driver_data & FEC_QUIRK_SWAP_FRAME) > + if (!is_copybreak && need_swap) > swap_buffer(data, pkt_len); > > /* 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
Hi Lothar, On Tue, Oct 7, 2014 at 10:19 AM, Lothar Waßmann <LW@karo-electronics.de> wrote: > 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> Yesterday night I was using linux-next on mx28evk and could not boot from NFS. I haven't had a chance to debug it, but I am glad you found a fix. Won't be able to have access to my mx28evk until Thursday to test it though. Thanks -- 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
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. > > ISTM that if you need to do the 'swap' you should copy the data regardless > of the length. > The swap function has to look at at most 3 bytes beyond the actual packet length. That is what the original swap_buffer() function does and what the new function swap_buffer2(), that does the endian swapping while copying to the new buffer, also does. Lothar Waßmann
From: Lothar > 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. > > > > ISTM that if you need to do the 'swap' you should copy the data regardless > > of the length. > > > The swap function has to look at at most 3 bytes beyond the actual > packet length. That is what the original swap_buffer() function does and > what the new function swap_buffer2(), that does the endian swapping > while copying to the new buffer, also does. I understood the bug. The point I was making is that if you have to do a read-write of the received data (to byteswap it) then you might as well always copy it into a new skb that is just big enough for the actual receive frame. David
On Tue, 2014-10-07 at 15:19 +0200, Lothar Waßmann wrote: > 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(-) > > diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c > index 87975b5..eaaebad 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 < DIV_ROUND_UP(len, 4); i++, src++, dst++) > + *dst = cpu_to_be32(*src); No need for the DIV : for (i = 0; i < len; i += sizeof(*dst), src++, dst++) *dst = cpu_to_be32(*src); Also are you sure both src/dst are aligned to word boundaries, or is this architecture OK with possible misalignment ? -- 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
On Tue, 2014-10-07 at 14:23 +0000, David Laight wrote: > The point I was making is that if you have to do a read-write of the received > data (to byteswap it) then you might as well always copy it into a new skb that > is just big enough for the actual receive frame. +1 -- 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
Hello. On 10/07/2014 05:19 PM, Lothar Waßmann wrote: > 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(-) > diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c > index 87975b5..eaaebad 100644 > --- a/drivers/net/ethernet/freescale/fec_main.c > +++ b/drivers/net/ethernet/freescale/fec_main.c [...] > @@ -1348,7 +1360,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, int swap) 'bool swap' perhaps? > @@ -1393,6 +1408,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 = id_entry->driver_data & FEC_QUIRK_SWAP_FRAME; ... especially talking this into account... WBR, Sergei -- 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 <LW@KARO-electronics.de> Date: Tue, 7 Oct 2014 15:19:37 +0200 > 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> Why don't you just round up the length fec_enet_copybreak() uses when need_swap is true? Then you will end up mimicking the original behavior and not require this new helper function. And in any case I agree with Sergei that if you do retain your approach, the new 'swap' argument to fec_enet_copybreak() should be a 'bool'. I'm really surprised there isn't a control register bit to adjust the endianness of the data DMA'd to/from the network. -- 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
Hi, David Miller wrote: > From: Lothar Waßmann <LW@KARO-electronics.de> > Date: Tue, 7 Oct 2014 15:19:37 +0200 > > > 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> > > Why don't you just round up the length fec_enet_copybreak() uses when > need_swap is true? Then you will end up mimicking the original behavior > and not require this new helper function. > I wanted to eliminate the need to access the buffer twice (once for copying and once for swapping). > And in any case I agree with Sergei that if you do retain your approach, > the new 'swap' argument to fec_enet_copybreak() should be a 'bool'. > Right. > I'm really surprised there isn't a control register bit to adjust the > endianness of the data DMA'd to/from the network. > This is a "feature" of the i.MX28. Lothar Waßmann
Hi, David Laight wrote: > From: Lothar > > 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. > > > > > > ISTM that if you need to do the 'swap' you should copy the data regardless > > > of the length. > > > > > The swap function has to look at at most 3 bytes beyond the actual > > packet length. That is what the original swap_buffer() function does and > > what the new function swap_buffer2(), that does the endian swapping > > while copying to the new buffer, also does. > > I understood the bug. > > The point I was making is that if you have to do a read-write of the received > data (to byteswap it) then you might as well always copy it into a new skb that > is just big enough for the actual receive frame. > I wanted to use the least intrusive solution. Lothar Waßmann
Hi, David Laight wrote: > From: Eric Dumazet > > On Tue, 2014-10-07 at 15:19 +0200, Lothar Wamann wrote: > > > 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 87975b5..eaaebad 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 < DIV_ROUND_UP(len, 4); i++, src++, dst++) > > > + *dst = cpu_to_be32(*src); > > > > No need for the DIV : > > > > for (i = 0; i < len; i += sizeof(*dst), src++, dst++) > > *dst = cpu_to_be32(*src); > > > > Also are you sure both src/dst are aligned to word boundaries, or is > > this architecture OK with possible misalignment ? > > I wondered about that as well. > I wouldn't have expected ppc to support misaligned transfers, and you'd also > want to make sure that cpu_to_be(*src) was using a byte-swapping instruction. > Hmmm... cpu_to_be() doesn't sound like the right 'swap' macro name. > ??? So what is cpu_to_be32() then? The new swap function is an exact copy of the original one already in use except for the fact that it uses distinct source and destination buffers. Lothar Waßmann
From: Lothar Waßmann > David Laight wrote: > > From: Eric Dumazet > > > On Tue, 2014-10-07 at 15:19 +0200, Lothar Wamann wrote: > > > > 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 87975b5..eaaebad 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 < DIV_ROUND_UP(len, 4); i++, src++, dst++) > > > > + *dst = cpu_to_be32(*src); > > > > > > No need for the DIV : > > > > > > for (i = 0; i < len; i += sizeof(*dst), src++, dst++) > > > *dst = cpu_to_be32(*src); > > > > > > Also are you sure both src/dst are aligned to word boundaries, or is > > > this architecture OK with possible misalignment ? > > > > I wondered about that as well. > > I wouldn't have expected ppc to support misaligned transfers, and you'd also > > want to make sure that cpu_to_be(*src) was using a byte-swapping instruction. > > Hmmm... cpu_to_be() doesn't sound like the right 'swap' macro name. > > > ??? So what is cpu_to_be32() then? > The new swap function is an exact copy of the original one already in > use except for the fact that it uses distinct source and destination > buffers. cpu_to_be32() is for converting a 'cpu' endianness value to 'big-endian'. Here you are processing receive data - so you probably want be32_to_cpu(). (Yes, I know they are functionally identical....) Alternatively, since these aren't actually 32bit numbers and you know whether you want to swap, something like the __swab32p() from swab.h - but I'm not entirely sure that one is expected to be used. Clearly you are well inside the ppc 'endianness' hell. David
From: Sergei Shtylyov > On 10/07/2014 05:19 PM, Lothar Wamann wrote: > > > 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 87975b5..eaaebad 100644 > > --- a/drivers/net/ethernet/freescale/fec_main.c > > +++ b/drivers/net/ethernet/freescale/fec_main.c > [...] > > @@ -1348,7 +1360,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, int swap) > > 'bool swap' perhaps? > > > @@ -1393,6 +1408,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 = id_entry->driver_data & FEC_QUIRK_SWAP_FRAME; > > ... especially talking this into account... Hmmm... in that case you may not want the compiler to convert the bit value to a 'bool' at all. Passing 'id_entry->driver_data' through (that doesn't look like a field name for 'quirk flags) would generate better code. Even better would be to reference the flag directly from 'ndev'. A pointer indirection for the test if probably faster then passing another argument. David
Hi, David Laight wrote: > From: Lothar Waßmann > > David Laight wrote: > > > From: Eric Dumazet > > > > On Tue, 2014-10-07 at 15:19 +0200, Lothar Wamann wrote: > > > > > 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 87975b5..eaaebad 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 < DIV_ROUND_UP(len, 4); i++, src++, dst++) > > > > > + *dst = cpu_to_be32(*src); > > > > > > > > No need for the DIV : > > > > > > > > for (i = 0; i < len; i += sizeof(*dst), src++, dst++) > > > > *dst = cpu_to_be32(*src); > > > > > > > > Also are you sure both src/dst are aligned to word boundaries, or is > > > > this architecture OK with possible misalignment ? > > > > > > I wondered about that as well. > > > I wouldn't have expected ppc to support misaligned transfers, and you'd also > > > want to make sure that cpu_to_be(*src) was using a byte-swapping instruction. > > > Hmmm... cpu_to_be() doesn't sound like the right 'swap' macro name. > > > > > ??? So what is cpu_to_be32() then? > > The new swap function is an exact copy of the original one already in > > use except for the fact that it uses distinct source and destination > > buffers. > > cpu_to_be32() is for converting a 'cpu' endianness value to 'big-endian'. > Here you are processing receive data - so you probably want be32_to_cpu(). > (Yes, I know they are functionally identical....) > > Alternatively, since these aren't actually 32bit numbers and you know > whether you want to swap, something like the __swab32p() from swab.h > - but I'm not entirely sure that one is expected to be used. > OK, but that should probably be a separate patch to fix the original swap_buffer() function as well. > Clearly you are well inside the ppc 'endianness' hell. > No. i.MX28 is not PPC. It only has a "feature" in the ethernet controller that stores all data in big endian format. Lothar Waßmann
On Wed, Oct 08, 2014 at 08:54:58AM +0000, David Laight wrote: > Hmmm... in that case you may not want the compiler to convert the bit value > to a 'bool' at all. > > Passing 'id_entry->driver_data' through (that doesn't look like a field name for > 'quirk flags) would generate better code. > > Even better would be to reference the flag directly from 'ndev'. > A pointer indirection for the test if probably faster then passing > another argument. A far better idea would be to copy the quirks into the fec_net_private structure, storing them as a 'unsigned int' value, and test them from there. This is /much/ more efficient than jumping through the hoops to retrieve id_entry, and then testing the 64-bit driver_data value. I've had such a patch since about the beginning of the year (and patches which add stuff like byte queue limits, which are really needed now that we have a /huge/ transmit ring), but I can't keep up with rebasing the patch set, and properly tested and performance impacts evaluated due to the rate of FEC changes. I never finished rebasing the set after Andy's TSO work...
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index 87975b5..eaaebad 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 < DIV_ROUND_UP(len, 4); i++, src++, dst++) + *dst = cpu_to_be32(*src); + + return dst_buf; +} + static void fec_dump(struct net_device *ndev) { struct fec_enet_private *fep = netdev_priv(ndev); @@ -1348,7 +1360,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, int swap) { struct fec_enet_private *fep = netdev_priv(ndev); struct sk_buff *new_skb; @@ -1363,7 +1375,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; @@ -1393,6 +1408,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 = id_entry->driver_data & FEC_QUIRK_SWAP_FRAME; #ifdef CONFIG_M532x flush_cache_all(); @@ -1456,7 +1472,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)) { @@ -1471,7 +1488,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 (id_entry->driver_data & 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(-)