phylib: kernel oops on resume with 2.6.33-rc4

Hello all,

I am hitting a kernel oops when resuming from suspend-to-RAM on 2.6.33-rc4 kernel.

The MAC is TI DaVinci EMAC and PHY is SMSC LAN8710. However, I also hit the issue even when using generic PHY driver (CONFIG_SMSC_PHY turned off).

On reverting commit 541cd3e "phylib: Fix deadlock on resume" which was introduced in 2.6.33-rc4, the issue doesn't come about. So far, I haven't figured out how this particular commit is causing the oops. Full oops report is attached. Appreciate any help in debugging this.

Thanks,
Sekhar

-----8<---------

root@da850-omapl138-evm:~# rtcwake -m mem -d /dev/rtc0 -s 15
wakeup from "mem" at Sat Jan  1 00:02:01 2000
PM: Syncing filesystems ... done.
Freezing user space processes ... (elapsed 0.01 seconds) done.
Freezing remaining freezable tasks ... (elapsed 0.01 seconds) done.
Suspending console(s) (use no_console_suspend to debug)
PM: suspend of devices complete after 212.456 msecs
PM: late suspend of devices complete after 0.697 msecs
PM: early resume of devices complete after 0.220 msecs
eth0: attached PHY driver [SMSC LAN8710/LAN8720] (mii_bus:phy_addr=1:00, id=7c0f1)
PM: resume of devices complete after 237.098 msecs
Restarting tasks ... done.
kernel BUG at kernel/workqueue.c:354!
Unable to handle kernel NULL pointer dereference at virtual address 00000000
pgd = c0004000
[00000000] *pgd=00000000
Internal error: Oops: 817 [#1] PREEMPT
last sysfs file: /sys/power/state
Modules linked in:
CPU: 0    Not tainted  (2.6.33-rc4 #1830)
PC is at __bug+0x20/0x2c
LR is at release_console_sem+0xf0/0xfc
pc : [<c002c5b8>]    lr : [<c003ed68>]    psr: 60000013
sp : c7833ef0  ip : c7833e30  fp : c7833efc
r10: 00000000  r9 : 00000000  r8 : c78dfb30
r7 : 00000000  r6 : 00000064  r5 : ffffffff  r4 : c78dfb04
r3 : 00000000  r2 : 00000000  r1 : 0000182b  r0 : 0000002c
Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
Control: 0005317f  Table: c7b64000  DAC: 00000017
Process events/0 (pid: 4, stack limit = 0xc7832270)
Stack: (0xc7833ef0 to 0xc7834000)
3ee0:                                     c7833f1c c7833f00 c0052a54 c002c5a8
3f00: 00000000 c78dfb04 c78dfa00 00000000 c7833f2c c7833f20 c0052b30 c00529f0
3f20: c7833f3c c7833f30 c0052b58 c0052b14 c7833f64 c7833f40 c01cf8b4 c0052b44
3f40: c78dfb08 c7832000 c78dfb04 c7800380 c01cf4e0 00000000 c7833f8c c7833f68
3f60: c0052578 c01cf4f0 c7832000 c7800388 c7832000 c7800380 00000000 00000000
3f80: c7833fc4 c7833f90 c0052730 c00524c4 c02a6440 00000000 c7820960 c0056428
3fa0: c7833fa0 c7833fa0 c7833fcc c7823f18 c7800380 c005263c c7833ff4 c7833fc8
3fc0: c00560c0 c005264c 00000000 00000000 c7833fd0 c7833fd0 00000000 00000000
3fe0: 00000000 00000000 00000000 c7833ff8 c0042218 c0056048 44004012 00300000
Backtrace:
[<c002c598>] (__bug+0x0/0x2c) from [<c0052a54>] (queue_delayed_work_on+0x74/0xf8)
[<c00529e0>] (queue_delayed_work_on+0x0/0xf8) from [<c0052b30>] (queue_delayed_work+0x2c/0x30)
 r7:00000000 r6:c78dfa00 r5:c78dfb04 r4:00000000
[<c0052b04>] (queue_delayed_work+0x0/0x30) from [<c0052b58>] (schedule_delayed_work+0x24/0x2c)
[<c0052b34>] (schedule_delayed_work+0x0/0x2c) from [<c01cf8b4>] (phy_state_machine+0x3d4/0x3e8)
[<c01cf4e0>] (phy_state_machine+0x0/0x3e8) from [<c0052578>] (run_workqueue+0xc4/0x188)
 r9:00000000 r8:c01cf4e0 r7:c7800380 r6:c78dfb04 r5:c7832000
r4:c78dfb08
[<c00524b4>] (run_workqueue+0x0/0x188) from [<c0052730>] (worker_thread+0xf4/0x108)
 r9:00000000 r8:00000000 r7:c7800380 r6:c7832000 r5:c7800388
r4:c7832000
[<c005263c>] (worker_thread+0x0/0x108) from [<c00560c0>] (kthread+0x88/0x90)
 r7:c005263c r6:c7800380 r5:c7823f18 r4:c7833fcc
[<c0056038>] (kthread+0x0/0x90) from [<c0042218>] (do_exit+0x0/0x280)
 r7:00000000 r6:00000000 r5:00000000 r4:00000000
Code: e1a01000 e59f000c eb09e6f5 e3a03000 (e5833000)
---[ end trace e88b3bd0ec06cbd0 ]---
nfs: server 172.24.190.197 not responding, still trying

Hello,

It seems phy_start_machine() needlessly reinitialized work struct. This is OK for ucc_geth and gianfar driver because these drivers do not detach PHYs on suspend, but TI davinci does. :-/

Can you try the patch down below?

Thanks!

index b0e9f9c..0295097 100644
@@ -410,7 +410,6 @@ EXPORT_SYMBOL(phy_start_aneg);
 
 
 static void phy_change(struct work_struct *work);
-static void phy_state_machine(struct work_struct *work);
 
 /**
  * phy_start_machine - start PHY state machine tracking

@@ -430,7 +429,6 @@ void phy_start_machine(struct phy_device *phydev,
 {
 	phydev->adjust_state = handler;
 
-	INIT_DELAYED_WORK(&phydev->state_queue, phy_state_machine);
 	schedule_delayed_work(&phydev->state_queue, HZ);
 }
 

@@ -761,7 +759,7 @@ EXPORT_SYMBOL(phy_start);
  * phy_state_machine - Handle the state machine
  * @work: work_struct that describes the work to be done
  */
-static void phy_state_machine(struct work_struct *work)
+void phy_state_machine(struct work_struct *work)
 {
 	struct delayed_work *dwork = to_delayed_work(work);
 	struct phy_device *phydev =

index 8212b2b..adbc0fd 100644
@@ -177,6 +177,7 @@ struct phy_device* phy_device_create(struct mii_bus *bus, int addr, int phy_id)
 	dev->state = PHY_DOWN;
 
 	mutex_init(&dev->lock);
+	INIT_DELAYED_WORK(&dev->state_queue, phy_state_machine);
 
 	return dev;
 }

index 7968def..6a7eb40 100644
@@ -485,6 +485,7 @@ void phy_driver_unregister(struct phy_driver *drv);
 int phy_driver_register(struct phy_driver *new_driver);
 void phy_prepare_link(struct phy_device *phydev,
 		void (*adjust_link)(struct net_device *));
+void phy_state_machine(struct work_struct *work);
 void phy_start_machine(struct phy_device *phydev,
 		void (*handler)(struct net_device *));
 void phy_stop_machine(struct phy_device *phydev);

This seems to have fixed the problem. Thanks! I hope this can be included for the next rc. FWIW,

Thanks,
Sekhar

From: "Nori, Sekhar" <[email protected]>
Date: Sun, 17 Jan 2010 13:58:39 +0530

Anton, please resubmit with your signoff.

Thanks.

commit 541cd3ee00a4fe975b22fac6a3bc846bacef37f7 ("phylib: Fix deadlock on resume") caused TI DaVinci EMAC ethernet driver to oops upon resume:

PM: resume of devices complete after 237.098 msecs Restarting tasks ... done.
kernel BUG at kernel/workqueue.c:354!

 Unable to handle kernel NULL pointer dereference at virtual address 00000000
 [...]
 Backtrace:
 [<c002c598>] (__bug+0x0/0x2c) from [<c0052a54>] (queue_delayed_work_on+0x74/0xf8)
 [<c00529e0>] (queue_delayed_work_on+0x0/0xf8) from [<c0052b30>] (queue_delayed_work+0x2c/0x30)

The oops pops up because TI DaVinci EMAC driver detaches PHY on suspend and attaches it back on resume. Attaching makes phylib call phy_start_machine() that initializes a workqueue. On the other hand, PHY's resume routine will call phy_start_machine() again, and that will cause the oops since we just destroyed the already scheduled workqueue.

This patch fixes the issue by moving workqueue initialization to phy_device_create().

p.s. We don't see this oops with ucc_geth and gianfar drivers because they perform a fine-grained suspend, i.e. they just stop the PHYs without detaching.

---

On Sun, Jan 17, 2010 at 07:23:53PM -0800, David Miller wrote:
[...]

Here it is. Thanks!

 drivers/net/phy/phy.c        |    4 +---
 drivers/net/phy/phy_device.c |    1 +
 include/linux/phy.h          |    1 +
 3 files changed, 3 insertions(+), 3 deletions(-)

index b0e9f9c..0295097 100644
@@ -410,7 +410,6 @@ EXPORT_SYMBOL(phy_start_aneg);
 
 
 static void phy_change(struct work_struct *work);
-static void phy_state_machine(struct work_struct *work);
 
 /**
  * phy_start_machine - start PHY state machine tracking

@@ -430,7 +429,6 @@ void phy_start_machine(struct phy_device *phydev,
 {
 	phydev->adjust_state = handler;
 
-	INIT_DELAYED_WORK(&phydev->state_queue, phy_state_machine);
 	schedule_delayed_work(&phydev->state_queue, HZ);
 }
 

@@ -761,7 +759,7 @@ EXPORT_SYMBOL(phy_start);
  * phy_state_machine - Handle the state machine
  * @work: work_struct that describes the work to be done
  */
-static void phy_state_machine(struct work_struct *work)
+void phy_state_machine(struct work_struct *work)
 {
 	struct delayed_work *dwork = to_delayed_work(work);
 	struct phy_device *phydev =

index 8212b2b..adbc0fd 100644
@@ -177,6 +177,7 @@ struct phy_device* phy_device_create(struct mii_bus *bus, int addr, int phy_id)
 	dev->state = PHY_DOWN;
 
 	mutex_init(&dev->lock);
+	INIT_DELAYED_WORK(&dev->state_queue, phy_state_machine);
 
 	return dev;
 }

index 7968def..6a7eb40 100644
@@ -485,6 +485,7 @@ void phy_driver_unregister(struct phy_driver *drv);
 int phy_driver_register(struct phy_driver *new_driver);
 void phy_prepare_link(struct phy_device *phydev,
 		void (*adjust_link)(struct net_device *));
+void phy_state_machine(struct work_struct *work);
 void phy_start_machine(struct phy_device *phydev,
 		void (*handler)(struct net_device *));
 void phy_stop_machine(struct phy_device *phydev);

1.6.5.7

From: Anton Vorontsov <[email protected]>
Date: Mon, 18 Jan 2010 18:37:16 +0300

...

Applied, thanks.