fs_enet: check for phydev existence in the ethtool handlers

PATCH Applied Anton Vorontsov 5 participants 10 messages Jan 8, 2008 – Jan 12, 2008

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Anton Vorontsov

author

2008-01-08 18:56 UTC #0 lore raw

PATCH 1/1 fs_enet: check for phydev existence in the ethtool handlers

Otherwise oops will happen if ethernet device has not been opened:

Unable to handle kernel paging request for data at address 0x0000014c
Faulting instruction address: 0xc016f7f0
Oops: Kernel access of bad area, sig: 11 [#1]

MPC85xx
NIP: c016f7f0 LR: c01722a0 CTR: 00000000
REGS: c79ddc70 TRAP: 0300 Not tainted (2.6.24-rc3-g820a386b)
MSR: 00029000 <EE,ME> CR: 20004428 XER: 20000000
DEAR: 0000014c, ESR: 00000000
TASK = c789f5e0[999] 'snmpd' THREAD: c79dc000
GPR00: c01aceb8 c79ddd20 c789f5e0 00000000 c79ddd3c 00000000 c79ddd64 00000000
GPR08: 00000000 c7845b60 c79dde3c c01ace80 20004422 200249fc 000002a0 100da728
GPR16: 100c0000 00000000 00000000 00000000 20022078 00000009 200220e0 bfc85558
GPR24: c79ddd3c 00000000 00000000 c02e0e70 c022fc64 ffffffff c7845800 bfc85498 NIP [c016f7f0] phy_ethtool_gset+0x0/0x4c
LR [c01722a0] fs_get_settings+0x18/0x28

Call Trace:

[c79ddd20] [c79dde38] 0xc79dde38 (unreliable)
[c79ddd30] [c01aceb8] dev_ethtool+0x294/0x11ec
[c79dde30] [c01aaa44] dev_ioctl+0x454/0x6a8
[c79ddeb0] [c019b9d4] sock_ioctl+0x84/0x230
[c79dded0] [c007ded8] do_ioctl+0x34/0x8c
[c79ddee0] [c007dfbc] vfs_ioctl+0x8c/0x41c
[c79ddf10] [c007e38c] sys_ioctl+0x40/0x74
[c79ddf40] [c000d4c0] ret_from_syscall+0x0/0x3c
Instruction dump:
81630000 800b0030 2f800000 419e0010 7c0803a6 4e800021 7c691b78 80010014 7d234b78 38210010 7c0803a6 4e800020 <8003014c> 7c6b1b78 38600000 90040004

Signed-off-by Anton Vorontsov <[email protected]>

Acked-by Vitaly Bordug <[email protected]>

---

Just resending it, it feels like it got lost during holidays.

 drivers/net/fs_enet/fs_enet-main.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

drivers/net/fs_enet/fs_enet-main.c +9 -2

index f2a4d39..23fddc3 100644
@@ -897,14 +897,21 @@ static void fs_get_regs(struct net_device *dev, struct ethtool_regs *regs,
 static int fs_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
 {
 	struct fs_enet_private *fep = netdev_priv(dev);
+
+	if (!fep->phydev)
+		return -ENODEV;
+
 	return phy_ethtool_gset(fep->phydev, cmd);
 }
 
 static int fs_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
 {
 	struct fs_enet_private *fep = netdev_priv(dev);
-	phy_ethtool_sset(fep->phydev, cmd);
-	return 0;
+
+	if (!fep->phydev)
+		return -ENODEV;
+
+	return phy_ethtool_sset(fep->phydev, cmd);
 }
 
 static int fs_nway_reset(struct net_device *dev)

Signature · 1 line

1.5.2.2

Sergej Stepanov

2008-01-09 11:15 UTC #1 lore raw

Am Dienstag, den 08.01.2008, 22:05 +0300 schrieb Anton Vorontsov:

Quoted 57 lines

Otherwise oops will happen if ethernet device has not been opened:
Unable to handle kernel paging request for data at address 0x0000014c
Faulting instruction address: 0xc016f7f0
Oops: Kernel access of bad area, sig: 11 [#1]
MPC85xx
NIP: c016f7f0 LR: c01722a0 CTR: 00000000
REGS: c79ddc70 TRAP: 0300 Not tainted (2.6.24-rc3-g820a386b)
MSR: 00029000 <EE,ME> CR: 20004428 XER: 20000000
DEAR: 0000014c, ESR: 00000000
TASK = c789f5e0[999] 'snmpd' THREAD: c79dc000
GPR00: c01aceb8 c79ddd20 c789f5e0 00000000 c79ddd3c 00000000 c79ddd64 00000000
GPR08: 00000000 c7845b60 c79dde3c c01ace80 20004422 200249fc 000002a0 100da728
GPR16: 100c0000 00000000 00000000 00000000 20022078 00000009 200220e0 bfc85558
GPR24: c79ddd3c 00000000 00000000 c02e0e70 c022fc64 ffffffff c7845800 bfc85498 NIP [c016f7f0] phy_ethtool_gset+0x0/0x4c
LR [c01722a0] fs_get_settings+0x18/0x28
Call Trace:
[c79ddd20] [c79dde38] 0xc79dde38 (unreliable)
[c79ddd30] [c01aceb8] dev_ethtool+0x294/0x11ec
[c79dde30] [c01aaa44] dev_ioctl+0x454/0x6a8
[c79ddeb0] [c019b9d4] sock_ioctl+0x84/0x230
[c79dded0] [c007ded8] do_ioctl+0x34/0x8c
[c79ddee0] [c007dfbc] vfs_ioctl+0x8c/0x41c
[c79ddf10] [c007e38c] sys_ioctl+0x40/0x74
[c79ddf40] [c000d4c0] ret_from_syscall+0x0/0x3c
Instruction dump:
81630000 800b0030 2f800000 419e0010 7c0803a6 4e800021 7c691b78 80010014 7d234b78 38210010 7c0803a6 4e800020 <8003014c> 7c6b1b78 38600000 90040004
Signed-off-by Anton Vorontsov <[email protected]>
Acked-by Vitaly Bordug <[email protected]>
---
Just resending it, it feels like it got lost during holidays.
 drivers/net/fs_enet/fs_enet-main.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)
drivers/net/fs_enet/fs_enet-main.c +4 -0
index f2a4d39..23fddc3 100644
@@ -897,14 +897,21 @@ static void fs_get_regs(struct net_device *dev, struct ethtool_regs *regs,
 static int fs_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
 {
 	struct fs_enet_private *fep = netdev_priv(dev);
+
+	if (!fep->phydev)
+		return -ENODEV;
+
 	return phy_ethtool_gset(fep->phydev, cmd);
 }
 
 static int fs_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
 {
 	struct fs_enet_private *fep = netdev_priv(dev);

drivers/net/fs_enet/fs_enet-main.c static void fs_get_regs(struct net_device *dev, struct ethtool_regs *regs, · line 917

Quoted 10 lines

drivers/net/fs_enet/fs_enet-main.c +5 -2

-	phy_ethtool_sset(fep->phydev, cmd);
-	return 0;
+
+	if (!fep->phydev)
+		return -ENODEV;
+
+	return phy_ethtool_sset(fep->phydev, cmd);
 }
 
 static int fs_nway_reset(struct net_device *dev)

I got also oops problem with the driver. What could be false?
After the following patch it functions. Thanks for any advance.

(quoted diff) +0 -0

b/drivers/net/fs_enet/fs_enet-main.c index f2a4d39..d5081b1 100644

drivers/net/fs_enet/fs_enet-main.c +0 -0

@@ -99,6 +99,8 @@ static int fs_enet_rx_napi(struct napi_struct *napi,

int budget)

        if (!netif_running(dev))
                return 0;

+ if (fep->cur_rx == NULL)
+ return 0;
/*
* First, grab all of the stats for the incoming packet.
* These get messed up if we get called due to a busy condition.

Heiko Schocher

2008-01-09 13:15 UTC #2 lore raw

Hello Sergej,

drivers/net/fs_enet/fs_enet-main.c static int fs_enet_rx_napi(struct napi_struct *napi, · line 99

Quoted 12 lines · Wed Jan 9 21:46:06 EST 2008 Sergej Stepanov [email protected] wrote:

I got also oops problem with the driver. What could be false?
After the following patch it functions. Thanks for any advance.
drivers/net/fs_enet/fs_enet-main.c +0 -0
index f2a4d39..d5081b1 100644
@@ -99,6 +99,8 @@ static int fs_enet_rx_napi(struct napi_struct *napi,
 int budget)

seems your mailer wraps long lines

Quoted 8 lines

drivers/net/fs_enet/fs_enet-main.c +2 -0

       if (!netif_running(dev))
               return 0;
 
+       if (fep->cur_rx == NULL)
+               return 0;
        /*
         * First, grab all of the stats for the incoming packet.

* These get messed up if we get called due to a busy condition.

Hmm... I had also a oops in fs_enet_rx_napi () because of an uninitialized fep->cur_rx. The following Patch solves this, also adds support for using Ethernet over SCC on a CPM2.

Quoted 1 line

From 62cd02d481eb772f4417e9ba17fb010d1954c330 Mon Sep 17 00:00:00 2001

From: Heiko Schocher <[email protected]>
Date: Mon, 7 Jan 2008 09:42:09 +0100
Subject: [PATCH] [POWERPC] Fix Ethernet over SCC on a CPM2

Signed-off-by Heiko Schocher <[email protected]>

---
 drivers/net/fs_enet/fs_enet-main.c |   11 +++++++++--
 drivers/net/fs_enet/mac-scc.c      |   18 ++++++++++++++++++
 2 files changed, 27 insertions(+), 2 deletions(-)

drivers/net/fs_enet/fs_enet-main.c +9 -2

index f2a4d39..b4a1480 100644
@@ -982,6 +982,7 @@ static struct net_device *fs_init_instance(struct device *dev,
 	fep = netdev_priv(ndev);
 
 	fep->dev = dev;
+	fep->ndev = ndev;
 	dev_set_drvdata(dev, ndev);
 	fep->fpi = fpi;
 	if (fpi->init_ioports)

@@ -1085,7 +1086,6 @@ static struct net_device *fs_init_instance(struct device *dev,
 	}
 	registered = 1;
 
-
 	return ndev;
 
 err:

@@ -1312,6 +1312,9 @@ static int __devinit fs_enet_probe(struct of_device *ofdev,
 	       ndev->dev_addr[0], ndev->dev_addr[1], ndev->dev_addr[2],
 	       ndev->dev_addr[3], ndev->dev_addr[4], ndev->dev_addr[5]);
 
+	/* to initialize the fep->cur_rx,... */
+	/* not doing this, will cause a crash in fs_enet_rx_napi */
+	fs_init_bds(ndev);
 	return 0;
 
 out_free_bd:

@@ -1342,9 +1345,13 @@ static int fs_enet_remove(struct of_device *ofdev)
 }
 
 static struct of_device_id fs_enet_match[] = {
-#ifdef CONFIG_FS_ENET_HAS_SCC
+#if defined(CONFIG_FS_ENET_HAS_SCC)
 	{
+#if defined(CONFIG_CPM1)
 		.compatible = "fsl,cpm1-scc-enet",
+#else
+		.compatible = "fsl,cpm2-scc-enet",
+#endif
 		.data = (void *)&fs_scc_ops,
 	},
 #endif

drivers/net/fs_enet/mac-scc.c +18 -0

index 48f2f30..3b5ca76 100644
@@ -50,6 +50,7 @@
 #include "fs_enet.h"
 
 /*************************************************/
+#define SCC_EB ((u_char)0x10)  /* Set big endian byte order */
 
 #if defined(CONFIG_CPM1)
 /* for a 8xx __raw_xxx's are sufficient */

@@ -65,6 +66,8 @@
 #define __fs_out16(addr, x)	out_be16(addr, x)
 #define __fs_in32(addr)	in_be32(addr)
 #define __fs_in16(addr)	in_be16(addr)
+#define __fs_out8(addr, x)	out_8(addr, x)
+#define __fs_in8(addr)	in_8(addr)
 #endif
 
 /* write, read, set bits, clear bits */

@@ -96,10 +99,18 @@ static inline int scc_cr_cmd(struct fs_enet_private *fep, u32 op)
 	const struct fs_platform_info *fpi = fep->fpi;
 	int i;
 
+#if defined(CONFIG_CPM1)
 	W16(cpmp, cp_cpcr, fpi->cp_command | CPM_CR_FLG | (op << 8));
 	for (i = 0; i < MAX_CR_CMD_LOOPS; i++)
 		if ((R16(cpmp, cp_cpcr) & CPM_CR_FLG) == 0)
 			return 0;
+#else
+	W32(cpmp, cp_cpcr, fpi->cp_command | CPM_CR_FLG | op);
+	for (i = 0; i < MAX_CR_CMD_LOOPS; i++)
+		if ((R32(cpmp, cp_cpcr) & CPM_CR_FLG) == 0)
+			return 0;
+
+#endif
 
 	printk(KERN_ERR "%s(): Not able to issue CPM command\n",
 		__FUNCTION__);

@@ -306,8 +317,15 @@ static void restart(struct net_device *dev)
 
 	/* Initialize function code registers for big-endian.
 	 */
+#ifdef CONFIG_CPM2
+	/* from oldstyle driver in arch/ppc */
+	/* seems necessary */
+	W8(ep, sen_genscc.scc_rfcr, SCC_EB | 0x20);
+	W8(ep, sen_genscc.scc_tfcr, SCC_EB | 0x20);
+#else
 	W8(ep, sen_genscc.scc_rfcr, SCC_EB);
 	W8(ep, sen_genscc.scc_tfcr, SCC_EB);
+#endif
 
 	/* Set maximum bytes per receive buffer.
 	 * This appears to be an Ethernet frame size, not the buffer

Signature · 8 lines

1.5.2.2


bye
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Scott Wood

2008-01-09 19:48 UTC #3 lore raw

drivers/net/fs_enet/fs_enet-main.c static int __devinit fs_enet_probe(struct of_device *ofdev, · line 1318

Quoted 8 lines · Heiko Schocher

drivers/net/fs_enet/fs_enet-main.c +3 -0

@@ -1312,6 +1312,9 @@ static int __devinit fs_enet_probe(struct of_device *ofdev,
 	       ndev->dev_addr[0], ndev->dev_addr[1], ndev->dev_addr[2],
 	       ndev->dev_addr[3], ndev->dev_addr[4], ndev->dev_addr[5]);
 
+	/* to initialize the fep->cur_rx,... */
+	/* not doing this, will cause a crash in fs_enet_rx_napi */
+	fs_init_bds(ndev);
 	return 0;

We don't want to allocate ring buffers for network interfaces that are never opened, especially given the small amount of memory on some boards that use this driver.

Instead, we should probably not be calling napi_enable() until the link is up and init_bds() has been called.

drivers/net/fs_enet/fs_enet-main.c static int fs_enet_remove(struct of_device *ofdev) · line 1354

Quoted 12 lines

drivers/net/fs_enet/fs_enet-main.c +5 -1

@@ -1342,9 +1345,13 @@ static int fs_enet_remove(struct of_device *ofdev)
 }
 
 static struct of_device_id fs_enet_match[] = {
-#ifdef CONFIG_FS_ENET_HAS_SCC
+#if defined(CONFIG_FS_ENET_HAS_SCC)
 	{
+#if defined(CONFIG_CPM1)
 		.compatible = "fsl,cpm1-scc-enet",
+#else
+		.compatible = "fsl,cpm2-scc-enet",
+#endif

I know there are already ifdefs of this sort, and that multiplatform cpm1/cpm2 is very unlikely to ever happen, but can we try to avoid introducing more such ifdefs?

We can have both match entries present at the same time.

drivers/net/fs_enet/mac-scc.c line 53

Quoted 12 lines

drivers/net/fs_enet/fs_enet-main.c +0 -0

 		.data = (void *)&fs_scc_ops,
 	},
 #endif

drivers/net/fs_enet/mac-scc.c +1 -0

index 48f2f30..3b5ca76 100644
@@ -50,6 +50,7 @@
 #include "fs_enet.h"
 
 /*************************************************/
+#define SCC_EB ((u_char)0x10)  /* Set big endian byte order */

This is already defined in asm-powerpc/commproc.h, and thus will cause a duplicate definition when building for 8xx. Please add this definition to asm-powerpc/cpm2.h.

drivers/net/fs_enet/mac-scc.c static inline int scc_cr_cmd(struct fs_enet_private *fep, u32 op) · line 113

Quoted 12 lines

drivers/net/fs_enet/mac-scc.c +8 -0

+#if defined(CONFIG_CPM1)
 	W16(cpmp, cp_cpcr, fpi->cp_command | CPM_CR_FLG | (op << 8));
 	for (i = 0; i < MAX_CR_CMD_LOOPS; i++)
 		if ((R16(cpmp, cp_cpcr) & CPM_CR_FLG) == 0)
 			return 0;
+#else
+	W32(cpmp, cp_cpcr, fpi->cp_command | CPM_CR_FLG | op);
+	for (i = 0; i < MAX_CR_CMD_LOOPS; i++)
+		if ((R32(cpmp, cp_cpcr) & CPM_CR_FLG) == 0)
+			return 0;
+
+#endif

Commit 362f9b6fa8c9670cc5496390845021c2865d049b in Paul's tree makes this unnecessary.

drivers/net/fs_enet/mac-scc.c static void restart(struct net_device *dev) · line 328

Quoted 13 lines

drivers/net/fs_enet/mac-scc.c +7 -0

@@ -306,8 +317,15 @@ static void restart(struct net_device *dev)
 
 	/* Initialize function code registers for big-endian.
 	 */
+#ifdef CONFIG_CPM2
+	/* from oldstyle driver in arch/ppc */
+	/* seems necessary */
+	W8(ep, sen_genscc.scc_rfcr, SCC_EB | 0x20);
+	W8(ep, sen_genscc.scc_tfcr, SCC_EB | 0x20);
+#else
 	W8(ep, sen_genscc.scc_rfcr, SCC_EB);
 	W8(ep, sen_genscc.scc_tfcr, SCC_EB);
+#endif

Please define 0x20 as SCC_GBL (Snooping Enabled) in cpm2.h, and conditionalize this on #ifndef CONFIG_NOT_COHERENT_CACHE.

You can remove the comment; it's really necessary, not just "seems" so. :-)

-Scott

Heiko Schocher

2008-01-10 08:11 UTC #4 lore raw

Hello Scott,

Quoted 14 lines · Scott Wood

drivers/net/fs_enet/fs_enet-main.c static int __devinit fs_enet_probe(struct of_device *ofdev, · line 1318
Quoted 8 lines · Heiko Schocher
drivers/net/fs_enet/fs_enet-main.c +3 -0
@@ -1312,6 +1312,9 @@ static int __devinit fs_enet_probe(struct of_device *ofdev,
 	       ndev->dev_addr[0], ndev->dev_addr[1], ndev->dev_addr[2],
 	       ndev->dev_addr[3], ndev->dev_addr[4], ndev->dev_addr[5]);
 
+	/* to initialize the fep->cur_rx,... */
+	/* not doing this, will cause a crash in fs_enet_rx_napi */
+	fs_init_bds(ndev);
 	return 0;
We don't want to allocate ring buffers for network interfaces that are never opened, especially given the small amount of memory on some boards that use this driver.

Quoted 2 lines

Instead, we should probably not be calling napi_enable() until the link is up and init_bds() has been called.

Ah, okay. I actually tried calling fs_init_bds(ndev); in fs_enet_open() after napi_enable, and this also works fine. I think there is the better place for it. Thanks.

Quoted 18 lines

drivers/net/fs_enet/fs_enet-main.c static int fs_enet_remove(struct of_device *ofdev) · line 1354
Quoted 13 lines
drivers/net/fs_enet/fs_enet-main.c +5 -1
@@ -1342,9 +1345,13 @@ static int fs_enet_remove(struct of_device *ofdev)
 }
 
 static struct of_device_id fs_enet_match[] = {
-#ifdef CONFIG_FS_ENET_HAS_SCC
+#if defined(CONFIG_FS_ENET_HAS_SCC)
 	{
+#if defined(CONFIG_CPM1)
 		.compatible = "fsl,cpm1-scc-enet",
+#else
+		.compatible = "fsl,cpm2-scc-enet",
+#endif
I know there are already ifdefs of this sort, and that multiplatform cpm1/cpm2 is very unlikely to ever happen, but can we try to avoid introducing more such ifdefs?

Quoted 1 line

We can have both match entries present at the same time.

OK, fix this.

Quoted 11 lines

Quoted 11 lines
drivers/net/fs_enet/fs_enet-main.c +0 -0
 		.data = (void *)&fs_scc_ops,
 	},
 #endif
drivers/net/fs_enet/mac-scc.c +0 -0
index 48f2f30..3b5ca76 100644
@@ -50,6 +50,7 @@
 #include "fs_enet.h"
 

Quoted 6 lines

drivers/net/fs_enet/mac-scc.c line 53
Quoted 2 lines
drivers/net/fs_enet/mac-scc.c +1 -0
 /*************************************************/
+#define SCC_EB ((u_char)0x10)  /* Set big endian byte order */
This is already defined in asm-powerpc/commproc.h, and thus will cause a duplicate definition when building for 8xx. Please add this definition to asm-powerpc/cpm2.h.

OK, will fix it.

Quoted 16 lines

drivers/net/fs_enet/mac-scc.c static inline int scc_cr_cmd(struct fs_enet_private *fep, u32 op) · line 113
Quoted 13 lines
drivers/net/fs_enet/mac-scc.c +8 -0
+#if defined(CONFIG_CPM1)
 	W16(cpmp, cp_cpcr, fpi->cp_command | CPM_CR_FLG | (op << 8));
 	for (i = 0; i < MAX_CR_CMD_LOOPS; i++)
 		if ((R16(cpmp, cp_cpcr) & CPM_CR_FLG) == 0)
 			return 0;
+#else
+	W32(cpmp, cp_cpcr, fpi->cp_command | CPM_CR_FLG | op);
+	for (i = 0; i < MAX_CR_CMD_LOOPS; i++)
+		if ((R32(cpmp, cp_cpcr) & CPM_CR_FLG) == 0)
+			return 0;
+
+#endif
Commit 362f9b6fa8c9670cc5496390845021c2865d049b in Paul's tree makes this unnecessary.

Tried this patch, works fine for me :-)

Quoted 18 lines

Quoted 4 lines
drivers/net/fs_enet/mac-scc.c +0 -0
@@ -306,8 +317,15 @@ static void restart(struct net_device *dev)
 
 	/* Initialize function code registers for big-endian.
drivers/net/fs_enet/mac-scc.c static void restart(struct net_device *dev) · line 328
Quoted 10 lines
drivers/net/fs_enet/mac-scc.c +7 -0
 	 */
+#ifdef CONFIG_CPM2
+	/* from oldstyle driver in arch/ppc */
+	/* seems necessary */
+	W8(ep, sen_genscc.scc_rfcr, SCC_EB | 0x20);
+	W8(ep, sen_genscc.scc_tfcr, SCC_EB | 0x20);
+#else
 	W8(ep, sen_genscc.scc_rfcr, SCC_EB);
 	W8(ep, sen_genscc.scc_tfcr, SCC_EB);
+#endif
Please define 0x20 as SCC_GBL (Snooping Enabled) in cpm2.h, and conditionalize this on #ifndef CONFIG_NOT_COHERENT_CACHE.

Quoted 1 line

You can remove the comment; it's really necessary, not just "seems" so. :-)

OK, fix it.

Will resend this fixed patch.

thanks
Heiko

Signature · 2 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Heiko Schocher

2008-01-10 09:03 UTC #5 lore raw

Hello Scott,

Quoted 1 line · Heiko Schocher

Hello Scott,

[...]

Quoted 1 line

Will resend this fixed patch.

Here it is:

[POWERPC] Fix Ethernet over SCC on a CPM2,
also Fix crash in fs_enet_rx_napi()

Signed-off-by Heiko Schocher <[email protected]>

drivers/net/fs_enet/fs_enet-main.c +9 -1

index f2a4d39..f432a18 100644
@@ -810,6 +810,10 @@ static int fs_enet_open(struct net_device *dev)
 	if (fep->fpi->use_napi)
 		napi_enable(&fep->napi);
 
+	/* to initialize the fep->cur_rx,... */
+	/* not doing this, will cause a crash in fs_enet_rx_napi */
+	fs_init_bds(fep->ndev);
+
 	/* Install our interrupt handler. */
 	r = fs_request_irq(dev, fep->interrupt, "fs_enet-mac", fs_enet_interrupt);
 	if (r != 0) {

@@ -982,6 +986,7 @@ static struct net_device *fs_init_instance(struct device *dev,
 	fep = netdev_priv(ndev);
 
 	fep->dev = dev;
+	fep->ndev = ndev;
 	dev_set_drvdata(dev, ndev);
 	fep->fpi = fpi;
 	if (fpi->init_ioports)

@@ -1085,7 +1090,6 @@ static struct net_device *fs_init_instance(struct device *dev,
 	}
 	registered = 1;
 
-
 	return ndev;
 
 err:

@@ -1347,6 +1351,10 @@ static struct of_device_id fs_enet_match[] = {
 		.compatible = "fsl,cpm1-scc-enet",
 		.data = (void *)&fs_scc_ops,
 	},
+	{
+		.compatible = "fsl,cpm2-scc-enet",
+		.data = (void *)&fs_scc_ops,
+	},
 #endif
 #ifdef CONFIG_FS_ENET_HAS_FCC
 	{

drivers/net/fs_enet/mac-scc.c +7 -1

index fe3d8a6..3889271 100644
@@ -50,7 +50,6 @@
 #include "fs_enet.h"
 
 /*************************************************/
-
 #if defined(CONFIG_CPM1)
 /* for a 8xx __raw_xxx's are sufficient */
 #define __fs_out32(addr, x)	__raw_writel(x, addr)

@@ -65,6 +64,8 @@
 #define __fs_out16(addr, x)	out_be16(addr, x)
 #define __fs_in32(addr)	in_be32(addr)
 #define __fs_in16(addr)	in_be16(addr)
+#define __fs_out8(addr, x)	out_8(addr, x)
+#define __fs_in8(addr)	in_8(addr)
 #endif
 
 /* write, read, set bits, clear bits */

@@ -297,8 +298,13 @@ static void restart(struct net_device *dev)
 
 	/* Initialize function code registers for big-endian.
 	 */
+#ifndef CONFIG_NOT_COHERENT_CACHE
+	W8(ep, sen_genscc.scc_rfcr, SCC_EB | SCC_GBL);
+	W8(ep, sen_genscc.scc_tfcr, SCC_EB | SCC_GBL);
+#else
 	W8(ep, sen_genscc.scc_rfcr, SCC_EB);
 	W8(ep, sen_genscc.scc_tfcr, SCC_EB);
+#endif
 
 	/* Set maximum bytes per receive buffer.
 	 * This appears to be an Ethernet frame size, not the buffer

include/asm-powerpc/cpm2.h +5 -0

index f1112c1..14c6496 100644
@@ -375,6 +375,11 @@ typedef struct scc_param {
 	uint	scc_tcrc;	/* Internal */
 } sccp_t;
 
+/* Function code bits.
+*/
+#define SCC_EB	((u_char) 0x10)	/* Set big endian byte order */
+#define SCC_GBL	((u_char) 0x20) /* Snooping enabled */
+
 /* CPM Ethernet through SCC1.
  */
 typedef struct scc_enet {

bye,
Heiko

Signature · 2 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Scott Wood

2008-01-10 17:28 UTC #6 lore raw

drivers/net/fs_enet/fs_enet-main.c static int fs_enet_open(struct net_device *dev) · line 815

Quoted 11 lines · Heiko Schocher

drivers/net/fs_enet/fs_enet-main.c +3 -0

index f2a4d39..f432a18 100644
@@ -810,6 +810,10 @@ static int fs_enet_open(struct net_device *dev)
 	if (fep->fpi->use_napi)
 		napi_enable(&fep->napi);
 
+	/* to initialize the fep->cur_rx,... */
+	/* not doing this, will cause a crash in fs_enet_rx_napi */
+	fs_init_bds(fep->ndev);

We should do this just before napi_enable() rather than just after, to eliminate any chance of a race window.

Looks good otherwise.

-Scott

Heiko Schocher

2008-01-10 18:38 UTC #7 lore raw

Hello Scott,

Quoted 10 lines · Scott Wood

Quoted 9 lines · Heiko Schocher

drivers/net/fs_enet/fs_enet-main.c +0 -0

index f2a4d39..f432a18 100644
@@ -810,6 +810,10 @@ static int fs_enet_open(struct net_device *dev)
     if (fep->fpi->use_napi)
         napi_enable(&fep->napi);

Quoted 6 lines

drivers/net/fs_enet/fs_enet-main.c static int fs_enet_open(struct net_device *dev) · line 815
Quoted 3 lines
drivers/net/fs_enet/fs_enet-main.c +3 -0
+    /* to initialize the fep->cur_rx,... */
+    /* not doing this, will cause a crash in fs_enet_rx_napi */
+    fs_init_bds(fep->ndev);
We should do this just before napi_enable() rather than just after, to eliminate any chance of a race window.

Yes, you are right.
Here is the new patch:

Quoted 1 line

From 3b8db4261199f0115d8e0188b6bffbc9f5e7673d Mon Sep 17 00:00:00 2001

From: Heiko Schocher <[email protected]>
Date: Thu, 10 Jan 2008 19:28:18 +0100
Subject: [PATCH] [POWERPC] Fix Ethernet over SCC on a CPM2,
also Fix crash in fs_enet_rx_napi()

Signed-off-by Heiko Schocher <[email protected]>

---
 drivers/net/fs_enet/fs_enet-main.c |   10 +++++++++-
 drivers/net/fs_enet/mac-scc.c      |    8 +++++++-
 include/asm-powerpc/cpm2.h         |    5 +++++
 3 files changed, 21 insertions(+), 2 deletions(-)

drivers/net/fs_enet/fs_enet-main.c +9 -1

index f2a4d39..70a0319 100644
@@ -807,6 +807,10 @@ static int fs_enet_open(struct net_device *dev)
 	int r;
 	int err;
 
+	/* to initialize the fep->cur_rx,... */
+	/* not doing this, will cause a crash in fs_enet_rx_napi */
+	fs_init_bds(fep->ndev);
+
 	if (fep->fpi->use_napi)
 		napi_enable(&fep->napi);

@@ -982,6 +986,7 @@ static struct net_device *fs_init_instance(struct device *dev,
 	fep = netdev_priv(ndev);
 
 	fep->dev = dev;
+	fep->ndev = ndev;
 	dev_set_drvdata(dev, ndev);
 	fep->fpi = fpi;
 	if (fpi->init_ioports)

@@ -1085,7 +1090,6 @@ static struct net_device *fs_init_instance(struct device *dev,
 	}
 	registered = 1;
 
-
 	return ndev;
 
 err:

@@ -1347,6 +1351,10 @@ static struct of_device_id fs_enet_match[] = {
 		.compatible = "fsl,cpm1-scc-enet",
 		.data = (void *)&fs_scc_ops,
 	},
+	{
+		.compatible = "fsl,cpm2-scc-enet",
+		.data = (void *)&fs_scc_ops,
+	},
 #endif
 #ifdef CONFIG_FS_ENET_HAS_FCC
 	{

drivers/net/fs_enet/mac-scc.c +7 -1

index fe3d8a6..3889271 100644
@@ -50,7 +50,6 @@
 #include "fs_enet.h"
 
 /*************************************************/
-
 #if defined(CONFIG_CPM1)
 /* for a 8xx __raw_xxx's are sufficient */
 #define __fs_out32(addr, x)	__raw_writel(x, addr)

@@ -65,6 +64,8 @@
 #define __fs_out16(addr, x)	out_be16(addr, x)
 #define __fs_in32(addr)	in_be32(addr)
 #define __fs_in16(addr)	in_be16(addr)
+#define __fs_out8(addr, x)	out_8(addr, x)
+#define __fs_in8(addr)	in_8(addr)
 #endif
 
 /* write, read, set bits, clear bits */

@@ -297,8 +298,13 @@ static void restart(struct net_device *dev)
 
 	/* Initialize function code registers for big-endian.
 	 */
+#ifndef CONFIG_NOT_COHERENT_CACHE
+	W8(ep, sen_genscc.scc_rfcr, SCC_EB | SCC_GBL);
+	W8(ep, sen_genscc.scc_tfcr, SCC_EB | SCC_GBL);
+#else
 	W8(ep, sen_genscc.scc_rfcr, SCC_EB);
 	W8(ep, sen_genscc.scc_tfcr, SCC_EB);
+#endif
 
 	/* Set maximum bytes per receive buffer.
 	 * This appears to be an Ethernet frame size, not the buffer

include/asm-powerpc/cpm2.h +5 -0

index f1112c1..14c6496 100644
@@ -375,6 +375,11 @@ typedef struct scc_param {
 	uint	scc_tcrc;	/* Internal */
 } sccp_t;
 
+/* Function code bits.
+*/
+#define SCC_EB	((u_char) 0x10)	/* Set big endian byte order */
+#define SCC_GBL	((u_char) 0x20) /* Snooping enabled */
+
 /* CPM Ethernet through SCC1.
  */
 typedef struct scc_enet {

Signature · 8 lines

1.5.2.2


bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Sergej Stepanov

2008-01-11 16:01 UTC #8 lore raw

Am Donnerstag, den 10.01.2008, 19:41 +0100 schrieb Heiko Schocher:

Quoted 20 lines

Hello Scott,
Quoted 10 lines · Scott Wood
Quoted 9 lines · Heiko Schocher
drivers/net/fs_enet/fs_enet-main.c +0 -0
index f2a4d39..f432a18 100644
@@ -810,6 +810,10 @@ static int fs_enet_open(struct net_device *dev)
     if (fep->fpi->use_napi)
         napi_enable(&fep->napi);
 
Quoted 6 lines
drivers/net/fs_enet/fs_enet-main.c static int fs_enet_open(struct net_device *dev) · line 812
Quoted 3 lines
drivers/net/fs_enet/fs_enet-main.c +3 -0
+    /* to initialize the fep->cur_rx,... */
+    /* not doing this, will cause a crash in fs_enet_rx_napi */
+    fs_init_bds(fep->ndev);
We should do this just before napi_enable() rather than just after, to eliminate any chance of a race window.

Quoted 3 lines

Yes, you are right.
Here is the new patch:

It works fine.

Sergej.

Jeff Garzik

Applied

2008-01-12 22:45 UTC #9 lore raw

In reply to Anton Vorontsov

Quoted 30 lines · Anton Vorontsov

Otherwise oops will happen if ethernet device has not been opened:
Unable to handle kernel paging request for data at address 0x0000014c
Faulting instruction address: 0xc016f7f0
Oops: Kernel access of bad area, sig: 11 [#1]
MPC85xx
NIP: c016f7f0 LR: c01722a0 CTR: 00000000
REGS: c79ddc70 TRAP: 0300 Not tainted (2.6.24-rc3-g820a386b)
MSR: 00029000 <EE,ME> CR: 20004428 XER: 20000000
DEAR: 0000014c, ESR: 00000000
TASK = c789f5e0[999] 'snmpd' THREAD: c79dc000
GPR00: c01aceb8 c79ddd20 c789f5e0 00000000 c79ddd3c 00000000 c79ddd64 00000000
GPR08: 00000000 c7845b60 c79dde3c c01ace80 20004422 200249fc 000002a0 100da728
GPR16: 100c0000 00000000 00000000 00000000 20022078 00000009 200220e0 bfc85558
GPR24: c79ddd3c 00000000 00000000 c02e0e70 c022fc64 ffffffff c7845800 bfc85498 NIP [c016f7f0] phy_ethtool_gset+0x0/0x4c
LR [c01722a0] fs_get_settings+0x18/0x28
Call Trace:
[c79ddd20] [c79dde38] 0xc79dde38 (unreliable)
[c79ddd30] [c01aceb8] dev_ethtool+0x294/0x11ec
[c79dde30] [c01aaa44] dev_ioctl+0x454/0x6a8
[c79ddeb0] [c019b9d4] sock_ioctl+0x84/0x230
[c79dded0] [c007ded8] do_ioctl+0x34/0x8c
[c79ddee0] [c007dfbc] vfs_ioctl+0x8c/0x41c
[c79ddf10] [c007e38c] sys_ioctl+0x40/0x74
[c79ddf40] [c000d4c0] ret_from_syscall+0x0/0x3c
Instruction dump:
81630000 800b0030 2f800000 419e0010 7c0803a6 4e800021 7c691b78 80010014 7d234b78 38210010 7c0803a6 4e800020 <8003014c> 7c6b1b78 38600000 90040004

Quoted 8 lines

Signed-off-by Anton Vorontsov <[email protected]>
Acked-by Vitaly Bordug <[email protected]>
---
Just resending it, it feels like it got lost during holidays.
 drivers/net/fs_enet/fs_enet-main.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

applied