[PATCH] Fix kernel unaligned access with r8169 on sparc64

[PATCH] Fix kernel unaligned access with r8169 on sparc64

[Florian Fainelli]

[-- Attachment #1.1: Type: text/plain, Size: 431 bytes --]

Hi all,

When using the r8169 gigabit ethernet network driver under a Sun Entreprise 
450, you will encounter a lot of kernel unaligned access on ip_rcv and 
ip_fast_csum. The full report is available here : 
http://www.mail-archive.com/debian-bugs-dist%40lists.debian.org/msg363433.htm

The following patch against 2.6.22-rc6 will fix this problem.

Signed-off-by: Florian Fainelli <florian.fainelli@telecomint.eu>
-- 

[-- Attachment #1.2: r8169-sparc64-fix-unaligned.patch --]
[-- Type: text/plain, Size: 2047 bytes --]

diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index 5ec7752..5095dbe 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -69,6 +69,7 @@ VERSION 2.2LK	<2005/01/25>
 #include <asm/system.h>
 #include <asm/io.h>
 #include <asm/irq.h>
+#include <asm/unaligned.h>
 
 #ifdef CONFIG_R8169_NAPI
 #define NAPI_SUFFIX	"-NAPI"
@@ -224,7 +225,9 @@ static struct pci_device_id rtl8169_pci_tbl[] = {
 
 MODULE_DEVICE_TABLE(pci, rtl8169_pci_tbl);
 
+#if !defined(__sparc__)
 static int rx_copybreak = 200;
+#endif
 static int use_dac;
 static struct {
 	u32 msg_enable;
@@ -465,8 +468,10 @@ MODULE_AUTHOR("Realtek and the Linux r8169 crew <netdev@vger.kernel.org>");
 MODULE_DESCRIPTION("RealTek RTL-8169 Gigabit Ethernet driver");
 module_param_array(media, int, &num_media, 0);
 MODULE_PARM_DESC(media, "force phy operation. Deprecated by ethtool (8).");
+#if !defined(__sparc__)
 module_param(rx_copybreak, int, 0);
 MODULE_PARM_DESC(rx_copybreak, "Copy breakpoint for copy-only-tiny-frames");
+#endif
 module_param(use_dac, int, 0);
 MODULE_PARM_DESC(use_dac, "Enable PCI DAC. Unsafe on 32 bit PCI slot.");
 module_param_named(debug, debug.msg_enable, int, 0);
@@ -2486,10 +2491,25 @@ static inline int rtl8169_try_rx_copy(struct sk_buff **sk_buff, int pkt_size,
 {
 	int ret = -1;
 
-	if (pkt_size < rx_copybreak) {
+#if defined(__sparc__)
+	if (pkt_size) {
 		struct sk_buff *skb;
+		int i;
+
+		skb = dev_alloc_skb(pkt_size + 4);
+
+		/* align the data to the ip header - should be faster than copying the entire packet to the stack */
+		for (i = pkt_size - (pkt_size % 4); i >= 0; i -= 4) {
+			put_unaligned(*((u32 *) (skb->data + i)), (u32 *) (skb->data + i + 2));
+		}
+		skb->data += 2;
+		skb->tail += 2;
+#else
+	if (pkt_size < rx_copybreak) {
+                struct sk_buff *skb;
 
 		skb = dev_alloc_skb(pkt_size + align);
+#endif
 		if (skb) {
 			skb_reserve(skb, (align - 1) & (unsigned long)skb->data);
 			eth_copy_and_sum(skb, sk_buff[0]->data, pkt_size, 0);

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Fix kernel unaligned access with r8169 on sparc64

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 490 bytes --]

Few things about this patch. It's pretty quick and dirty, more experienced 
people with a better knowledge of the ip stack would certainly do better.
Though the problem was reported with Debian's 2.6.18 kernel, it is also 
present in the 2.6.22-rc6 version.

Without the patch, I can hardly achieve 130Mbits between two sparcs on a 
dedicated r8169 link, with it we can achieve 300 Mbits, though it's not 
purely GigE, it's still three times better. Measures were made using iperf.

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Fix kernel unaligned access with r8169 on sparc64

[Philip Craig]

Florian Fainelli wrote:
> Hi all,
> 
> When using the r8169 gigabit ethernet network driver under a Sun Entreprise 
> 450, you will encounter a lot of kernel unaligned access on ip_rcv and 
> ip_fast_csum. The full report is available here : 
> http://www.mail-archive.com/debian-bugs-dist%40lists.debian.org/msg363433.htm

Try the patches at http://www.fr.zoreil.com/linux/kernel/2.6.x/2.6.22-rc6/
I think patches 0012 and/or 0017 will fix this.


> The following patch against 2.6.22-rc6 will fix this problem.
> 
> Signed-off-by: Florian Fainelli <florian.fainelli@telecomint.eu>
> 
> 
> ------------------------------------------------------------------------
> 
> diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
> index 5ec7752..5095dbe 100644
> --- a/drivers/net/r8169.c
> +++ b/drivers/net/r8169.c
> @@ -69,6 +69,7 @@ VERSION 2.2LK	<2005/01/25>
>  #include <asm/system.h>
>  #include <asm/io.h>
>  #include <asm/irq.h>
> +#include <asm/unaligned.h>
>  
>  #ifdef CONFIG_R8169_NAPI
>  #define NAPI_SUFFIX	"-NAPI"
> @@ -224,7 +225,9 @@ static struct pci_device_id rtl8169_pci_tbl[] = {
>  
>  MODULE_DEVICE_TABLE(pci, rtl8169_pci_tbl);
>  
> +#if !defined(__sparc__)
>  static int rx_copybreak = 200;
> +#endif
>  static int use_dac;
>  static struct {
>  	u32 msg_enable;
> @@ -465,8 +468,10 @@ MODULE_AUTHOR("Realtek and the Linux r8169 crew <netdev@vger.kernel.org>");
>  MODULE_DESCRIPTION("RealTek RTL-8169 Gigabit Ethernet driver");
>  module_param_array(media, int, &num_media, 0);
>  MODULE_PARM_DESC(media, "force phy operation. Deprecated by ethtool (8).");
> +#if !defined(__sparc__)
>  module_param(rx_copybreak, int, 0);
>  MODULE_PARM_DESC(rx_copybreak, "Copy breakpoint for copy-only-tiny-frames");
> +#endif
>  module_param(use_dac, int, 0);
>  MODULE_PARM_DESC(use_dac, "Enable PCI DAC. Unsafe on 32 bit PCI slot.");
>  module_param_named(debug, debug.msg_enable, int, 0);
> @@ -2486,10 +2491,25 @@ static inline int rtl8169_try_rx_copy(struct sk_buff **sk_buff, int pkt_size,
>  {
>  	int ret = -1;
>  
> -	if (pkt_size < rx_copybreak) {
> +#if defined(__sparc__)
> +	if (pkt_size) {
>  		struct sk_buff *skb;
> +		int i;
> +
> +		skb = dev_alloc_skb(pkt_size + 4);
> +
> +		/* align the data to the ip header - should be faster than copying the entire packet to the stack */
> +		for (i = pkt_size - (pkt_size % 4); i >= 0; i -= 4) {
> +			put_unaligned(*((u32 *) (skb->data + i)), (u32 *) (skb->data + i + 2));
> +		}
> +		skb->data += 2;
> +		skb->tail += 2;
> +#else
> +	if (pkt_size < rx_copybreak) {
> +                struct sk_buff *skb;
>  
>  		skb = dev_alloc_skb(pkt_size + align);
> +#endif
>  		if (skb) {
>  			skb_reserve(skb, (align - 1) & (unsigned long)skb->data);
>  			eth_copy_and_sum(skb, sk_buff[0]->data, pkt_size, 0);

Re: [PATCH] Fix kernel unaligned access with r8169 on sparc64

[Francois Romieu]

Philip Craig <philipc@snapgear.com> :
[...]
> Try the patches at http://www.fr.zoreil.com/linux/kernel/2.6.x/2.6.22-rc6/
> I think patches 0012 and/or 0017 will fix this.

They should but they will not apply directly against 2.6.18-something.

Florian, can you consider one of the option below:
- try current 2.6.23-git. It contains the changes which should fix your
  alignment issues.
- try http://www.fr.zoreil.com/people/francois/backport/r8169/20070604-00
  It is a backport against 2.6.18 which should bring you quite close to
  2.6.23-git wrt r8169. 
  
-- 
Ueimor

Re: [PATCH] Fix kernel unaligned access with r8169 on sparc64

[Francois Romieu]

[-- Attachment #1: Type: text/plain, Size: 521 bytes --]

Francois Romieu <romieu@fr.zoreil.com> :
[...]
> - try http://www.fr.zoreil.com/people/francois/backport/r8169/20070604-00
>   It is a backport against 2.6.18 which should bring you quite close to
>   2.6.23-git wrt r8169. 

Regarding the backport option, it was initially more a compiled-ok-untested-
use-at-your-own-risk thing than anything else. I have given it a try with
2.6.18 and it does not blow up immediately. I suggest adding the attached
patch at the end of the serie to fix a locking bug though.

-- 
Ueimor

[-- Attachment #2: r8169-set-mac-addr-locking.patch --]
[-- Type: text/plain, Size: 2268 bytes --]

diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index eaf5373..982a901 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -135,8 +135,8 @@ static const struct {
 	u32 RxConfigMask;	/* Clears the bits supported by this chip */
 } rtl_chip_info[] = {
 	_R("RTL8169",		RTL_GIGA_MAC_VER_01, 0xff7e1880), // 8169
-	_R("RTL8169s/8110s",	RTL_GIGA_MAC_VER_02, 0xff7e1880), // 8169S
-	_R("RTL8169s/8110s",	RTL_GIGA_MAC_VER_03, 0xff7e1880), // 8110S
+	_R("RTL8169s",		RTL_GIGA_MAC_VER_02, 0xff7e1880), // 8169S
+	_R("RTL8110s",		RTL_GIGA_MAC_VER_03, 0xff7e1880), // 8110S
 	_R("RTL8169sb/8110sb",	RTL_GIGA_MAC_VER_04, 0xff7e1880), // 8169SB
 	_R("RTL8169sc/8110sc",	RTL_GIGA_MAC_VER_05, 0xff7e1880), // 8110SCd
 	_R("RTL8169sc/8110sc",	RTL_GIGA_MAC_VER_06, 0xff7e1880), // 8110SCe
@@ -1391,22 +1380,26 @@ static void rtl8169_init_phy(struct net_device *dev, struct rtl8169_private *tp)
 		printk(KERN_INFO PFX "%s: TBI auto-negotiating\n", dev->name);
 }
 
-static void rtl8169_rar_set(struct rtl8169_private *tp, u8 *addr)
+static void rtl_rar_set(struct rtl8169_private *tp, u8 *addr)
 {
 	void __iomem *ioaddr = tp->mmio_addr;
-	u32 low;
 	u32 high;
+	u32 low;
 
 	low  = addr[0] | (addr[1] << 8) | (addr[2] << 16) | (addr[3] << 24);
 	high = addr[4] | (addr[5] << 8);
 
+	spin_lock_irq(&tp->lock);
+
 	RTL_W8(Cfg9346, Cfg9346_Unlock);
 	RTL_W32(MAC0, low);
 	RTL_W32(MAC4, high);
 	RTL_W8(Cfg9346, Cfg9346_Lock);
+
+	spin_unlock_irq(&tp->lock);
 }
 
-static int rtl8169_set_mac_address(struct net_device *dev, void *p)
+static int rtl_set_mac_address(struct net_device *dev, void *p)
 {
 	struct rtl8169_private *tp = netdev_priv(dev);
 	struct sockaddr *addr = p;
@@ -1416,7 +1409,7 @@ static int rtl8169_set_mac_address(struct net_device *dev, void *p)
 
 	memcpy(dev->dev_addr, addr->sa_data, dev->addr_len);
 
-	rtl8169_rar_set(tp, dev->dev_addr);
+	rtl_rar_set(tp, dev->dev_addr);
 
 	return 0;
 }
@@ -1650,7 +1645,7 @@ rtl8169_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	dev->irq = pdev->irq;
 	dev->base_addr = (unsigned long) ioaddr;
 	dev->change_mtu = rtl8169_change_mtu;
-	dev->set_mac_address = rtl8169_set_mac_address;
+	dev->set_mac_address = rtl_set_mac_address;
 
 #ifdef CONFIG_R8169_NAPI
 	dev->poll = rtl8169_poll;