[PATCH] net: fec: fix regression on i.MX28 introduced by rx_copybreak support

[PATCH] net: fec: fix regression on i.MX28 introduced by rx_copybreak support

[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 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);
+
+	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

RE: [PATCH] net: fec: fix regression on i.MX28 introduced by rx_copybreak support

[David Laight]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4040 bytes --]

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
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] net: fec: fix regression on i.MX28 introduced by rx_copybreak support

[Fabio Estevam]

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

Re: [PATCH] net: fec: fix regression on i.MX28 introduced by rx_copybreak support

[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.
> 
> 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

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

RE: [PATCH] net: fec: fix regression on i.MX28 introduced by rx_copybreak support

[David Laight]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1551 bytes --]

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Ç+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] net: fec: fix regression on i.MX28 introduced by rx_copybreak support

[Eric Dumazet]

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 ?

RE: [PATCH] net: fec: fix regression on i.MX28 introduced by rx_copybreak support

[Eric Dumazet]

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

RE: [PATCH] net: fec: fix regression on i.MX28 introduced by rx_copybreak support

[David Laight]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2601 bytes --]

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.

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Ç+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] net: fec: fix regression on i.MX28 introduced by rx_copybreak support

[Sergei Shtylyov]

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

Re: [PATCH] net: fec: fix regression on i.MX28 introduced by rx_copybreak support

[David Miller]

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.

Re: [PATCH] net: fec: fix regression on i.MX28 introduced by rx_copybreak support

[Lothar Waßmann]

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
-- 
___________________________________________________________

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

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

Re: [PATCH] net: fec: fix regression on i.MX28 introduced by rx_copybreak support

[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
-- 
___________________________________________________________

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

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

Re: [PATCH] net: fec: fix regression on i.MX28 introduced by rx_copybreak support

[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
-- 
___________________________________________________________

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

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

RE: [PATCH] net: fec: fix regression on i.MX28 introduced by rx_copybreak support

[David Laight]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3150 bytes --]

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

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH] net: fec: fix regression on i.MX28 introduced by rx_copybreak support

[David Laight]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2398 bytes --]

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

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] net: fec: fix regression on i.MX28 introduced by rx_copybreak support

[Lothar Waßmann]

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
-- 
___________________________________________________________

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

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

Re: [PATCH] net: fec: fix regression on i.MX28 introduced by rx_copybreak support

[Russell King - ARM Linux]

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.