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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
Hi Lothar,
On Tue, Oct 7, 2014 at 10:19 AM, Lothar Waßmann <[email protected]> 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 <[email protected]>
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
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
--
___________________________________________________________
Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996
http://www.karo-electronics.de | [email protected]
___________________________________________________________
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
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
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 <[email protected]>
> ---
> 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 ?
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
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 <[email protected]>
> > ---
> > 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.
It looks as though this is processing an entire ethernet frame, in which
case the data (destination address) would normally be on an 4n+2 boundary.
The two bytes before the data can be safely copied (as can the rounding bytes at
the end) - so provided the pointers allow for this it should be fine.
OTOH I've NFI what the actual data is.
David
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
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 <[email protected]>
> ---
> 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
From: Lothar Wa?mann <[email protected]>
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 <[email protected]>
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.
Hi,
David Miller wrote:
> From: Lothar Waßmann <[email protected]>
> 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 <[email protected]>
>
> 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
--
___________________________________________________________
Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996
http://www.karo-electronics.de | [email protected]
___________________________________________________________
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
--
___________________________________________________________
Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996
http://www.karo-electronics.de | [email protected]
___________________________________________________________
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 <[email protected]>
> > > ---
> > > 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
--
___________________________________________________________
Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996
http://www.karo-electronics.de | [email protected]
___________________________________________________________
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 <[email protected]>
> > > > ---
> > > > 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
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
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 <[email protected]>
> > ---
> > 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
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
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 <[email protected]>
> > > > > ---
> > > > > 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
--
___________________________________________________________
Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996
http://www.karo-electronics.de | [email protected]
___________________________________________________________
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...
--
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.