[PATCH] marvell sky2 driver: fix so it works without unaligned accesses

[PATCH] marvell sky2 driver: fix so it works without unaligned accesses

[Chris Metcalf]

The driver uses a receive_new() routine that ends up requiring unaligned
accesses in IP header processing.  If the architecture doesn't support
efficient unaligned accesses, just copy all ingress packets to the bounce
buffers instead.

This allows the driver to be used on the Tilera TILEmpower-Gx, since
the tile architecture doesn't currently handle kernel unaligned accesses,
just userspace.

Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
---
 drivers/net/ethernet/marvell/sky2.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c
index 879b0a4..7a15482 100644
--- a/drivers/net/ethernet/marvell/sky2.c
+++ b/drivers/net/ethernet/marvell/sky2.c
@@ -2490,6 +2490,7 @@ static struct sk_buff *receive_copy(struct sky2_port *sky2,
 	return skb;
 }
 
+#ifdef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
 /* Adjust length of skb with fragments to match received data */
 static void skb_put_frags(struct sk_buff *skb, unsigned int hdr_space,
 			  unsigned int length)
@@ -2555,6 +2556,7 @@ nomap:
 nobuf:
 	return NULL;
 }
+#endif /* CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS */
 
 /*
  * Receive one packet.
@@ -2598,10 +2600,12 @@ static struct sk_buff *sky2_receive(struct net_device *dev,
 		goto error;
 
 okay:
-	if (length < copybreak)
-		skb = receive_copy(sky2, re, length);
-	else
+#ifdef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
+	if (length >= copybreak)
 		skb = receive_new(sky2, re, length);
+	else
+#endif
+		skb = receive_copy(sky2, re, length);
 
 	dev->stats.rx_dropped += (skb == NULL);
 
-- 
1.6.5.2

Re: [PATCH] marvell sky2 driver: fix so it works without unaligned accesses

[Stephen Hemminger]

On Tue, 3 Apr 2012 11:13:56 -0400
Chris Metcalf <cmetcalf@tilera.com> wrote:

> The driver uses a receive_new() routine that ends up requiring unaligned
> accesses in IP header processing.  If the architecture doesn't support
> efficient unaligned accesses, just copy all ingress packets to the bounce
> buffers instead.
> 
> This allows the driver to be used on the Tilera TILEmpower-Gx, since
> the tile architecture doesn't currently handle kernel unaligned accesses,
> just userspace.
> 
> Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>

There are many other encapsulation cases where IP header won't be
aligned. Why not just set copybreak module option to a very large value?

Re: [PATCH] marvell sky2 driver: fix so it works without unaligned accesses

[Eric Dumazet]

On Tue, 2012-04-03 at 11:13 -0400, Chris Metcalf wrote:
> The driver uses a receive_new() routine that ends up requiring unaligned
> accesses in IP header processing.  If the architecture doesn't support
> efficient unaligned accesses, just copy all ingress packets to the bounce
> buffers instead.
> 
> This allows the driver to be used on the Tilera TILEmpower-Gx, since
> the tile architecture doesn't currently handle kernel unaligned accesses,
> just userspace.
> 
> Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
> ---
>  drivers/net/ethernet/marvell/sky2.c |   10 +++++++---
>  1 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c
> index 879b0a4..7a15482 100644
> --- a/drivers/net/ethernet/marvell/sky2.c
> +++ b/drivers/net/ethernet/marvell/sky2.c
> @@ -2490,6 +2490,7 @@ static struct sk_buff *receive_copy(struct sky2_port *sky2,
>  	return skb;
>  }
>  
> +#ifdef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
>  /* Adjust length of skb with fragments to match received data */
>  static void skb_put_frags(struct sk_buff *skb, unsigned int hdr_space,
>  			  unsigned int length)
> @@ -2555,6 +2556,7 @@ nomap:
>  nobuf:
>  	return NULL;
>  }
> +#endif /* CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS */
>  
>  /*
>   * Receive one packet.
> @@ -2598,10 +2600,12 @@ static struct sk_buff *sky2_receive(struct net_device *dev,
>  		goto error;
>  
>  okay:
> -	if (length < copybreak)
> -		skb = receive_copy(sky2, re, length);
> -	else
> +#ifdef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
> +	if (length >= copybreak)
>  		skb = receive_new(sky2, re, length);
> +	else
> +#endif
> +		skb = receive_copy(sky2, re, length);
>  
>  	dev->stats.rx_dropped += (skb == NULL);
>  

The (expensive ?) copy should be done only if (sky2->hw->flags &
SKY2_HW_RAM_BUFFER)

diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c
index b806d9b..3da3cdc 100644
--- a/drivers/net/ethernet/marvell/sky2.c
+++ b/drivers/net/ethernet/marvell/sky2.c
@@ -2599,7 +2599,11 @@ static struct sk_buff *sky2_receive(struct net_device *dev,
 		goto error;
 
 okay:
-	if (length < copybreak)
+	if ((length < copybreak) 
+#ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
+		|| (sky2->hw->flags & SKY2_HW_RAM_BUFFER)
+#endif
+		)
 		skb = receive_copy(sky2, re, length);
 	else
 		skb = receive_new(sky2, re, length);

Re: [PATCH] marvell sky2 driver: fix so it works without unaligned accesses

[David Miller]

From: Chris Metcalf <cmetcalf@tilera.com>
Date: Tue, 3 Apr 2012 11:13:56 -0400

> The driver uses a receive_new() routine that ends up requiring unaligned
> accesses in IP header processing.  If the architecture doesn't support
> efficient unaligned accesses, just copy all ingress packets to the bounce
> buffers instead.
> 
> This allows the driver to be used on the Tilera TILEmpower-Gx, since
> the tile architecture doesn't currently handle kernel unaligned accesses,
> just userspace.
> 
> Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>

As Stephen Hemminger alluded to, adjust the copybreak as needed.

If you look at how other device drivers use this CONFIG variable,
you'll get a better idea of what to do here if you want to adjust
the default copybreak.

Also keep in mind the issue brought up by Eric Dumazet in that
not all kinds of sky2 chips would need this copybreak adjustment,
only ones that have the SKY2_HW_RAM_BUFFER bit cleared.

[PATCH v2] marvell sky2 driver: fix so it works without unaligned accesses

[Chris Metcalf]

The driver uses a receive_new() routine that ends up requiring unaligned
accesses in IP header processing.  If the architecture doesn't support
efficient unaligned accesses, and SKY2_HW_RAM_BUFFER is set,
just copy all ingress packets to the bounce buffers instead.
Thanks to Eric Dumazet for pointing out the SKY2_HW_RAM_BUFFER issue.

This allows the driver to be used on the Tilera TILEmpower-Gx, since
the tile architecture doesn't currently handle kernel unaligned accesses,
just userspace.

Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
---
 drivers/net/ethernet/marvell/sky2.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c
index 423a1a2..2bc6a78 100644
--- a/drivers/net/ethernet/marvell/sky2.c
+++ b/drivers/net/ethernet/marvell/sky2.c
@@ -2598,7 +2598,11 @@ static struct sk_buff *sky2_receive(struct net_device *dev,
 		goto error;
 
 okay:
-	if (length < copybreak)
+	if ((length < copybreak)
+#ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
+	    || (sky2->hw->flags & SKY2_HW_RAM_BUFFER)
+#endif
+	    )
 		skb = receive_copy(sky2, re, length);
 	else
 		skb = receive_new(sky2, re, length);
-- 
1.6.5.2

Re: [PATCH v2] marvell sky2 driver: fix so it works without unaligned accesses

[Stephen Hemminger]

On Wed, 4 Apr 2012 10:13:32 -0400
Chris Metcalf <cmetcalf@tilera.com> wrote:

> The driver uses a receive_new() routine that ends up requiring unaligned
> accesses in IP header processing.  If the architecture doesn't support
> efficient unaligned accesses, and SKY2_HW_RAM_BUFFER is set,
> just copy all ingress packets to the bounce buffers instead.
> Thanks to Eric Dumazet for pointing out the SKY2_HW_RAM_BUFFER issue.
> 
> This allows the driver to be used on the Tilera TILEmpower-Gx, since
> the tile architecture doesn't currently handle kernel unaligned accesses,
> just userspace.
> 
> Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>

Still kind of ugly, I will cleanup

Re: [PATCH v2] marvell sky2 driver: fix so it works without unaligned accesses

[Stephen Hemminger]

On Wed, 4 Apr 2012 10:13:32 -0400
Chris Metcalf <cmetcalf@tilera.com> wrote:

> The driver uses a receive_new() routine that ends up requiring unaligned
> accesses in IP header processing.  If the architecture doesn't support
> efficient unaligned accesses, and SKY2_HW_RAM_BUFFER is set,
> just copy all ingress packets to the bounce buffers instead.
> Thanks to Eric Dumazet for pointing out the SKY2_HW_RAM_BUFFER issue.
> 
> This allows the driver to be used on the Tilera TILEmpower-Gx, since
> the tile architecture doesn't currently handle kernel unaligned accesses,
> just userspace.
> 
> Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
> ---
>  drivers/net/ethernet/marvell/sky2.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)

What about the following (compile tested only)
--- a/drivers/net/ethernet/marvell/sky2.c	2012-04-04 08:49:05.954853108 -0700
+++ b/drivers/net/ethernet/marvell/sky2.c	2012-04-04 11:30:54.201432707 -0700
@@ -2468,6 +2468,17 @@ static int sky2_change_mtu(struct net_de
 	return err;
 }
 
+static inline bool needs_copy(const struct rx_ring_info *re,
+			      unsigned length)
+{
+#ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
+	/* Some architectures need the IP header to be aligned */
+	if (!IS_ALIGNED(re->data_addr + ETH_HLEN, sizeof(u32)))
+		return 1;
+#endif
+	return length < copybreak;
+}
+
 /* For small just reuse existing skb for next receive */
 static struct sk_buff *receive_copy(struct sky2_port *sky2,
 				    const struct rx_ring_info *re,
@@ -2605,7 +2616,7 @@ static struct sk_buff *sky2_receive(stru
 		goto error;
 
 okay:
-	if (length < copybreak)
+	if (needs_copy(re, length))
 		skb = receive_copy(sky2, re, length);
 	else
 		skb = receive_new(sky2, re, length);

Re: [PATCH v2] marvell sky2 driver: fix so it works without unaligned accesses

[Chris Metcalf]

On 4/4/2012 2:42 PM, Stephen Hemminger wrote:
> On Wed, 4 Apr 2012 10:13:32 -0400
> Chris Metcalf <cmetcalf@tilera.com> wrote:
>
>> The driver uses a receive_new() routine that ends up requiring unaligned
>> accesses in IP header processing.  If the architecture doesn't support
>> efficient unaligned accesses, and SKY2_HW_RAM_BUFFER is set,
>> just copy all ingress packets to the bounce buffers instead.
>> Thanks to Eric Dumazet for pointing out the SKY2_HW_RAM_BUFFER issue.
>>
>> This allows the driver to be used on the Tilera TILEmpower-Gx, since
>> the tile architecture doesn't currently handle kernel unaligned accesses,
>> just userspace.
>>
>> Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
>> ---
>>  drivers/net/ethernet/marvell/sky2.c |    6 +++++-
>>  1 files changed, 5 insertions(+), 1 deletions(-)
> What about the following (compile tested only)
> --- a/drivers/net/ethernet/marvell/sky2.c	2012-04-04 08:49:05.954853108 -0700
> +++ b/drivers/net/ethernet/marvell/sky2.c	2012-04-04 11:30:54.201432707 -0700
> @@ -2468,6 +2468,17 @@ static int sky2_change_mtu(struct net_de
>  	return err;
>  }
>  
> +static inline bool needs_copy(const struct rx_ring_info *re,
> +			      unsigned length)
> +{
> +#ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
> +	/* Some architectures need the IP header to be aligned */
> +	if (!IS_ALIGNED(re->data_addr + ETH_HLEN, sizeof(u32)))
> +		return 1;
> +#endif
> +	return length < copybreak;
> +}
> +
>  /* For small just reuse existing skb for next receive */
>  static struct sk_buff *receive_copy(struct sky2_port *sky2,
>  				    const struct rx_ring_info *re,
> @@ -2605,7 +2616,7 @@ static struct sk_buff *sky2_receive(stru
>  		goto error;
>  
>  okay:
> -	if (length < copybreak)
> +	if (needs_copy(re, length))
>  		skb = receive_copy(sky2, re, length);
>  	else
>  		skb = receive_new(sky2, re, length);

Acked-by: Chris Metcalf <cmetcalf@tilera.com>

This works on our TILEmpower-Gx boxes and seems like it's likely more
efficient.  I believe kernel style says you should "return true" rather
than "return 1" from a "bool" routine, though.

Thanks!

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

[PATCH v3] sky2: copy received packets on inefficient unaligned architecture

[Stephen Hemminger]

Modified from original patch from Chris.

The sky2 driver has to have 8 byte alignment of receive buffer
on some chip versions. On architectures which don't support efficient
unaligned access this doesn't work very well. The solution is to
just copy all received packets which is what the driver already
does for small packets.

This allows the driver to be used on the Tilera TILEmpower-Gx, since
the tile architecture doesn't currently handle kernel unaligned accesses,
just userspace.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
Acked-by: Chris Metcalf <cmetcalf@tilera.com>

--- a/drivers/net/ethernet/marvell/sky2.c	2012-04-04 08:49:05.954853108 -0700
+++ b/drivers/net/ethernet/marvell/sky2.c	2012-04-04 15:03:41.725705876 -0700
@@ -2468,6 +2468,17 @@ static int sky2_change_mtu(struct net_de
 	return err;
 }
 
+static inline bool needs_copy(const struct rx_ring_info *re,
+			      unsigned length)
+{
+#ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
+	/* Some architectures need the IP header to be aligned */
+	if (!IS_ALIGNED(re->data_addr + ETH_HLEN, sizeof(u32)))
+		return true;
+#endif
+	return length < copybreak;
+}
+
 /* For small just reuse existing skb for next receive */
 static struct sk_buff *receive_copy(struct sky2_port *sky2,
 				    const struct rx_ring_info *re,
@@ -2605,7 +2616,7 @@ static struct sk_buff *sky2_receive(stru
 		goto error;
 
 okay:
-	if (length < copybreak)
+	if (needs_copy(re, length))
 		skb = receive_copy(sky2, re, length);
 	else
 		skb = receive_new(sky2, re, length);

Re: [PATCH v3] sky2: copy received packets on inefficient unaligned architecture

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 4 Apr 2012 15:10:27 -0700

> Modified from original patch from Chris.
> 
> The sky2 driver has to have 8 byte alignment of receive buffer
> on some chip versions. On architectures which don't support efficient
> unaligned access this doesn't work very well. The solution is to
> just copy all received packets which is what the driver already
> does for small packets.
> 
> This allows the driver to be used on the Tilera TILEmpower-Gx, since
> the tile architecture doesn't currently handle kernel unaligned accesses,
> just userspace.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> Acked-by: Chris Metcalf <cmetcalf@tilera.com>

Applied.