Re: INFO: possible circular locking dependency at cleanup_workqueue_thread

Re: INFO: possible circular locking dependency at cleanup_workqueue_thread

[Rafael J. Wysocki]

On Saturday 23 May 2009, Johannes Berg wrote:
> On Sat, 2009-05-23 at 00:23 +0200, Rafael J. Wysocki wrote:
> 
> > > I just arrived at the same conclusion, heh. I can't say I understand
> > > these changes though, the part about calling the platform differently
> > > may make sense, but calling why disable non-boot CPUs at a different
> > > place?
> > 
> > Because the ordering of platform callbacks and cpu[_up()|_down()] is also
> > important, at least on resume.
> > 
> > In principle we can call device_pm_unlock() right before calling
> > disable_nonboot_cpus() and take the lock again right after calling
> > enable_nonboot_cpus(), if that helps.
> 
> Probably, unless the cpu_add_remove_lock wasn't a red herring after all.
> I'd test, but I don't have much time today, will be travelling tomorrow
> and be at UDS all week next week so I don't know when I'll get to it --
> could you provide a patch and also attach it to
> http://bugzilla.kernel.org/show_bug.cgi?id=13245 please? Miles (the
> reporter of that bug) has been very helpful in testing before.

OK

The patch is appended for reference (Alan, please have a look; I can't recall
why exactly we have called device_pm_lock() from the core suspend/hibernation
code instead of acquiring the lock locally in drivers/base/power/main.c) and
I'll attach it to the bug entry too.

Thanks,
Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: PM: Do not hold dpm_list_mtx while disabling/enabling nonboot CPUs

We shouldn't hold dpm_list_mtx while executing
[disable|enable]_nonboot_cpus(), because theoretically this may lead
to a deadlock as shown by the following example (provided by Johannes
Berg):

CPU 3       CPU 2                     CPU 1
                                      suspend/hibernate
            something:
            rtnl_lock()               device_pm_lock()
                                       -> mutex_lock(&dpm_list_mtx)

            mutex_lock(&dpm_list_mtx)

linkwatch_work
 -> rtnl_lock()
                                      disable_nonboot_cpus()
                                       -> flush CPU 3 workqueue

Fortunately, device drivers are supposed to stop any activities that
might lead to the registration of new device objects and/or to the
removal of the existing ones way before disable_nonboot_cpus() is
called, so it shouldn't be necessary to hold dpm_list_mtx over the
entire late part of device suspend and early part of device resume.

Thus, during the late suspend and the early resume of devices acquire
dpm_list_mtx only when dpm_list is going to be traversed and release
it right after that.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/base/power/main.c |    4 ++++
 kernel/kexec.c            |    2 --
 kernel/power/disk.c       |   21 +++------------------
 kernel/power/main.c       |    7 +------
 4 files changed, 8 insertions(+), 26 deletions(-)

Index: linux-2.6/kernel/power/disk.c
===================================================================
--- linux-2.6.orig/kernel/power/disk.c
+++ linux-2.6/kernel/power/disk.c
@@ -215,8 +215,6 @@ static int create_image(int platform_mod
 	if (error)
 		return error;
 
-	device_pm_lock();
-
 	/* At this point, device_suspend() has been called, but *not*
 	 * device_power_down(). We *must* call device_power_down() now.
 	 * Otherwise, drivers for some devices (e.g. interrupt controllers)
@@ -227,7 +225,7 @@ static int create_image(int platform_mod
 	if (error) {
 		printk(KERN_ERR "PM: Some devices failed to power down, "
 			"aborting hibernation\n");
-		goto Unlock;
+		return error;
 	}
 
 	error = platform_pre_snapshot(platform_mode);
@@ -280,9 +278,6 @@ static int create_image(int platform_mod
 	device_power_up(in_suspend ?
 		(error ? PMSG_RECOVER : PMSG_THAW) : PMSG_RESTORE);
 
- Unlock:
-	device_pm_unlock();
-
 	return error;
 }
 
@@ -348,13 +343,11 @@ static int resume_target_kernel(bool pla
 {
 	int error;
 
-	device_pm_lock();
-
 	error = device_power_down(PMSG_QUIESCE);
 	if (error) {
 		printk(KERN_ERR "PM: Some devices failed to power down, "
 			"aborting resume\n");
-		goto Unlock;
+		return error;
 	}
 
 	error = platform_pre_restore(platform_mode);
@@ -407,9 +400,6 @@ static int resume_target_kernel(bool pla
 
 	device_power_up(PMSG_RECOVER);
 
- Unlock:
-	device_pm_unlock();
-
 	return error;
 }
 
@@ -468,11 +458,9 @@ int hibernation_platform_enter(void)
 		goto Resume_devices;
 	}
 
-	device_pm_lock();
-
 	error = device_power_down(PMSG_HIBERNATE);
 	if (error)
-		goto Unlock;
+		goto Resume_devices;
 
 	error = hibernation_ops->prepare();
 	if (error)
@@ -497,9 +485,6 @@ int hibernation_platform_enter(void)
 
 	device_power_up(PMSG_RESTORE);
 
- Unlock:
-	device_pm_unlock();
-
  Resume_devices:
 	entering_platform_hibernation = false;
 	device_resume(PMSG_RESTORE);
Index: linux-2.6/kernel/power/main.c
===================================================================
--- linux-2.6.orig/kernel/power/main.c
+++ linux-2.6/kernel/power/main.c
@@ -271,12 +271,10 @@ static int suspend_enter(suspend_state_t
 {
 	int error;
 
-	device_pm_lock();
-
 	if (suspend_ops->prepare) {
 		error = suspend_ops->prepare();
 		if (error)
-			goto Done;
+			return error;
 	}
 
 	error = device_power_down(PMSG_SUSPEND);
@@ -325,9 +323,6 @@ static int suspend_enter(suspend_state_t
 	if (suspend_ops->finish)
 		suspend_ops->finish();
 
- Done:
-	device_pm_unlock();
-
 	return error;
 }
 
Index: linux-2.6/drivers/base/power/main.c
===================================================================
--- linux-2.6.orig/drivers/base/power/main.c
+++ linux-2.6/drivers/base/power/main.c
@@ -357,6 +357,7 @@ static void dpm_power_up(pm_message_t st
 {
 	struct device *dev;
 
+	mutex_lock(&dpm_list_mtx);
 	list_for_each_entry(dev, &dpm_list, power.entry)
 		if (dev->power.status > DPM_OFF) {
 			int error;
@@ -366,6 +367,7 @@ static void dpm_power_up(pm_message_t st
 			if (error)
 				pm_dev_err(dev, state, " early", error);
 		}
+	mutex_unlock(&dpm_list_mtx);
 }
 
 /**
@@ -614,6 +616,7 @@ int device_power_down(pm_message_t state
 	int error = 0;
 
 	suspend_device_irqs();
+	mutex_lock(&dpm_list_mtx);
 	list_for_each_entry_reverse(dev, &dpm_list, power.entry) {
 		error = suspend_device_noirq(dev, state);
 		if (error) {
@@ -622,6 +625,7 @@ int device_power_down(pm_message_t state
 		}
 		dev->power.status = DPM_OFF_IRQ;
 	}
+	mutex_unlock(&dpm_list_mtx);
 	if (error)
 		device_power_up(resume_event(state));
 	return error;
Index: linux-2.6/kernel/kexec.c
===================================================================
--- linux-2.6.orig/kernel/kexec.c
+++ linux-2.6/kernel/kexec.c
@@ -1451,7 +1451,6 @@ int kernel_kexec(void)
 		error = device_suspend(PMSG_FREEZE);
 		if (error)
 			goto Resume_console;
-		device_pm_lock();
 		/* At this point, device_suspend() has been called,
 		 * but *not* device_power_down(). We *must*
 		 * device_power_down() now.  Otherwise, drivers for
@@ -1489,7 +1488,6 @@ int kernel_kexec(void)
 		enable_nonboot_cpus();
 		device_power_up(PMSG_RESTORE);
  Resume_devices:
-		device_pm_unlock();
 		device_resume(PMSG_RESTORE);
  Resume_console:
 		resume_console();

Re: INFO: possible circular locking dependency at cleanup_workqueue_thread

[Ming Lei]

于 Sun, 24 May 2009 01:20:29 +0200
"Rafael J. Wysocki" <rjw@sisk.pl> 写道:

> On Saturday 23 May 2009, Johannes Berg wrote:
> > On Sat, 2009-05-23 at 00:23 +0200, Rafael J. Wysocki wrote:
> > 
> > > > I just arrived at the same conclusion, heh. I can't say I
> > > > understand these changes though, the part about calling the
> > > > platform differently may make sense, but calling why disable
> > > > non-boot CPUs at a different place?
> > > 
> > > Because the ordering of platform callbacks and cpu[_up()|_down()]
> > > is also important, at least on resume.
> > > 
> > > In principle we can call device_pm_unlock() right before calling
> > > disable_nonboot_cpus() and take the lock again right after calling
> > > enable_nonboot_cpus(), if that helps.
> > 
> > Probably, unless the cpu_add_remove_lock wasn't a red herring after
> > all. I'd test, but I don't have much time today, will be travelling
> > tomorrow and be at UDS all week next week so I don't know when I'll
> > get to it -- could you provide a patch and also attach it to
> > http://bugzilla.kernel.org/show_bug.cgi?id=13245 please? Miles (the
> > reporter of that bug) has been very helpful in testing before.
> 
> OK
> 
> The patch is appended for reference (Alan, please have a look; I
> can't recall why exactly we have called device_pm_lock() from the
> core suspend/hibernation code instead of acquiring the lock locally
> in drivers/base/power/main.c) and I'll attach it to the bug entry too.
> 
> Thanks,
> Rafael
> 
> ---
> From: Rafael J. Wysocki <rjw@sisk.pl>
> Subject: PM: Do not hold dpm_list_mtx while disabling/enabling
> nonboot CPUs
> 
> We shouldn't hold dpm_list_mtx while executing
> [disable|enable]_nonboot_cpus(), because theoretically this may lead
> to a deadlock as shown by the following example (provided by Johannes
> Berg):
> 
> CPU 3       CPU 2                     CPU 1
>                                       suspend/hibernate
>             something:
>             rtnl_lock()               device_pm_lock()
>                                        -> mutex_lock(&dpm_list_mtx)
> 
>             mutex_lock(&dpm_list_mtx)
> 
> linkwatch_work
>  -> rtnl_lock()
>                                       disable_nonboot_cpus()
>                                        -> flush CPU 3 workqueue
> 
> Fortunately, device drivers are supposed to stop any activities that
> might lead to the registration of new device objects and/or to the
> removal of the existing ones way before disable_nonboot_cpus() is
> called, so it shouldn't be necessary to hold dpm_list_mtx over the
> entire late part of device suspend and early part of device resume.
> 
> Thus, during the late suspend and the early resume of devices acquire
> dpm_list_mtx only when dpm_list is going to be traversed and release
> it right after that.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  drivers/base/power/main.c |    4 ++++
>  kernel/kexec.c            |    2 --
>  kernel/power/disk.c       |   21 +++------------------
>  kernel/power/main.c       |    7 +------
>  4 files changed, 8 insertions(+), 26 deletions(-)
> 

I try to apply the patch against lastest next tree(2009-05-22), but
"patch -p1" is failured:


[lm@linux-lm linux-2.6]$ patch -p1 <  ../patch_rx/INFO_possible_circular_locking_dependency_at_cleanup_workqueue_thread.patch 
patching file kernel/power/disk.c
Hunk #1 succeeded at 215 with fuzz 2.
Hunk #3 succeeded at 278 with fuzz 1.
Hunk #4 FAILED at 343.
Hunk #5 succeeded at 396 with fuzz 2 (offset -4 lines).
Hunk #6 FAILED at 454.
Hunk #7 succeeded at 485 with fuzz 2.
2 out of 7 hunks FAILED -- saving rejects to file kernel/power/disk.c.rej
patching file kernel/power/main.c
Hunk #1 succeeded at 289 with fuzz 1 (offset 18 lines).
patching file drivers/base/power/main.c
Hunk #3 succeeded at 616 with fuzz 2.
Hunk #4 succeeded at 625 with fuzz 2.
patching file kernel/kexec.c
Hunk #1 succeeded at 1451 with fuzz 2.
Hunk #2 succeeded at 1488 with fuzz 2.



> Index: linux-2.6/kernel/power/disk.c
> ===================================================================
> --- linux-2.6.orig/kernel/power/disk.c
> +++ linux-2.6/kernel/power/disk.c
> @@ -215,8 +215,6 @@ static int create_image(int platform_mod
>  	if (error)
>  		return error;
>  
> -	device_pm_lock();
> -
>  	/* At this point, device_suspend() has been called, but *not*
>  	 * device_power_down(). We *must* call device_power_down()
> now.
>  	 * Otherwise, drivers for some devices (e.g. interrupt
> controllers) @@ -227,7 +225,7 @@ static int create_image(int
> platform_mod if (error) {
>  		printk(KERN_ERR "PM: Some devices failed to power
> down, " "aborting hibernation\n");
> -		goto Unlock;
> +		return error;
>  	}
>  
>  	error = platform_pre_snapshot(platform_mode);
> @@ -280,9 +278,6 @@ static int create_image(int platform_mod
>  	device_power_up(in_suspend ?
>  		(error ? PMSG_RECOVER : PMSG_THAW) : PMSG_RESTORE);
>  
> - Unlock:
> -	device_pm_unlock();
> -
>  	return error;
>  }
>  
> @@ -348,13 +343,11 @@ static int resume_target_kernel(bool pla
>  {
>  	int error;
>  
> -	device_pm_lock();
> -
>  	error = device_power_down(PMSG_QUIESCE);
>  	if (error) {
>  		printk(KERN_ERR "PM: Some devices failed to power
> down, " "aborting resume\n");
> -		goto Unlock;
> +		return error;
>  	}
>  
>  	error = platform_pre_restore(platform_mode);
> @@ -407,9 +400,6 @@ static int resume_target_kernel(bool pla
>  
>  	device_power_up(PMSG_RECOVER);
>  
> - Unlock:
> -	device_pm_unlock();
> -
>  	return error;
>  }
>  
> @@ -468,11 +458,9 @@ int hibernation_platform_enter(void)
>  		goto Resume_devices;
>  	}
>  
> -	device_pm_lock();
> -
>  	error = device_power_down(PMSG_HIBERNATE);
>  	if (error)
> -		goto Unlock;
> +		goto Resume_devices;
>  
>  	error = hibernation_ops->prepare();
>  	if (error)
> @@ -497,9 +485,6 @@ int hibernation_platform_enter(void)
>  
>  	device_power_up(PMSG_RESTORE);
>  
> - Unlock:
> -	device_pm_unlock();
> -
>   Resume_devices:
>  	entering_platform_hibernation = false;
>  	device_resume(PMSG_RESTORE);
> Index: linux-2.6/kernel/power/main.c
> ===================================================================
> --- linux-2.6.orig/kernel/power/main.c
> +++ linux-2.6/kernel/power/main.c
> @@ -271,12 +271,10 @@ static int suspend_enter(suspend_state_t
>  {
>  	int error;
>  
> -	device_pm_lock();
> -
>  	if (suspend_ops->prepare) {
>  		error = suspend_ops->prepare();
>  		if (error)
> -			goto Done;
> +			return error;
>  	}
>  
>  	error = device_power_down(PMSG_SUSPEND);
> @@ -325,9 +323,6 @@ static int suspend_enter(suspend_state_t
>  	if (suspend_ops->finish)
>  		suspend_ops->finish();
>  
> - Done:
> -	device_pm_unlock();
> -
>  	return error;
>  }
>  
> Index: linux-2.6/drivers/base/power/main.c
> ===================================================================
> --- linux-2.6.orig/drivers/base/power/main.c
> +++ linux-2.6/drivers/base/power/main.c
> @@ -357,6 +357,7 @@ static void dpm_power_up(pm_message_t st
>  {
>  	struct device *dev;
>  
> +	mutex_lock(&dpm_list_mtx);
>  	list_for_each_entry(dev, &dpm_list, power.entry)
>  		if (dev->power.status > DPM_OFF) {
>  			int error;
> @@ -366,6 +367,7 @@ static void dpm_power_up(pm_message_t st
>  			if (error)
>  				pm_dev_err(dev, state, " early",
> error); }
> +	mutex_unlock(&dpm_list_mtx);
>  }
>  
>  /**
> @@ -614,6 +616,7 @@ int device_power_down(pm_message_t state
>  	int error = 0;
>  
>  	suspend_device_irqs();
> +	mutex_lock(&dpm_list_mtx);
>  	list_for_each_entry_reverse(dev, &dpm_list, power.entry) {
>  		error = suspend_device_noirq(dev, state);
>  		if (error) {
> @@ -622,6 +625,7 @@ int device_power_down(pm_message_t state
>  		}
>  		dev->power.status = DPM_OFF_IRQ;
>  	}
> +	mutex_unlock(&dpm_list_mtx);
>  	if (error)
>  		device_power_up(resume_event(state));
>  	return error;
> Index: linux-2.6/kernel/kexec.c
> ===================================================================
> --- linux-2.6.orig/kernel/kexec.c
> +++ linux-2.6/kernel/kexec.c
> @@ -1451,7 +1451,6 @@ int kernel_kexec(void)
>  		error = device_suspend(PMSG_FREEZE);
>  		if (error)
>  			goto Resume_console;
> -		device_pm_lock();
>  		/* At this point, device_suspend() has been called,
>  		 * but *not* device_power_down(). We *must*
>  		 * device_power_down() now.  Otherwise, drivers for
> @@ -1489,7 +1488,6 @@ int kernel_kexec(void)
>  		enable_nonboot_cpus();
>  		device_power_up(PMSG_RESTORE);
>   Resume_devices:
> -		device_pm_unlock();
>  		device_resume(PMSG_RESTORE);
>   Resume_console:
>  		resume_console();
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-kernel" in the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
Lei Ming
_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/linux-pm

Re: INFO: possible circular locking dependency at cleanup_workqueue_thread

[Rafael J. Wysocki]

On Sunday 24 May 2009, Ming Lei wrote:
> 于 Sun, 24 May 2009 01:20:29 +0200
> "Rafael J. Wysocki" <rjw@sisk.pl> 写道:
> 
> > On Saturday 23 May 2009, Johannes Berg wrote:
> > > On Sat, 2009-05-23 at 00:23 +0200, Rafael J. Wysocki wrote:
> > > 
> > > > > I just arrived at the same conclusion, heh. I can't say I
> > > > > understand these changes though, the part about calling the
> > > > > platform differently may make sense, but calling why disable
> > > > > non-boot CPUs at a different place?
> > > > 
> > > > Because the ordering of platform callbacks and cpu[_up()|_down()]
> > > > is also important, at least on resume.
> > > > 
> > > > In principle we can call device_pm_unlock() right before calling
> > > > disable_nonboot_cpus() and take the lock again right after calling
> > > > enable_nonboot_cpus(), if that helps.
> > > 
> > > Probably, unless the cpu_add_remove_lock wasn't a red herring after
> > > all. I'd test, but I don't have much time today, will be travelling
> > > tomorrow and be at UDS all week next week so I don't know when I'll
> > > get to it -- could you provide a patch and also attach it to
> > > http://bugzilla.kernel.org/show_bug.cgi?id=13245 please? Miles (the
> > > reporter of that bug) has been very helpful in testing before.
> > 
> > OK
> > 
> > The patch is appended for reference (Alan, please have a look; I
> > can't recall why exactly we have called device_pm_lock() from the
> > core suspend/hibernation code instead of acquiring the lock locally
> > in drivers/base/power/main.c) and I'll attach it to the bug entry too.
> > 
> > Thanks,
> > Rafael
> > 
> > ---
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > Subject: PM: Do not hold dpm_list_mtx while disabling/enabling
> > nonboot CPUs
> > 
> > We shouldn't hold dpm_list_mtx while executing
> > [disable|enable]_nonboot_cpus(), because theoretically this may lead
> > to a deadlock as shown by the following example (provided by Johannes
> > Berg):
> > 
> > CPU 3       CPU 2                     CPU 1
> >                                       suspend/hibernate
> >             something:
> >             rtnl_lock()               device_pm_lock()
> >                                        -> mutex_lock(&dpm_list_mtx)
> > 
> >             mutex_lock(&dpm_list_mtx)
> > 
> > linkwatch_work
> >  -> rtnl_lock()
> >                                       disable_nonboot_cpus()
> >                                        -> flush CPU 3 workqueue
> > 
> > Fortunately, device drivers are supposed to stop any activities that
> > might lead to the registration of new device objects and/or to the
> > removal of the existing ones way before disable_nonboot_cpus() is
> > called, so it shouldn't be necessary to hold dpm_list_mtx over the
> > entire late part of device suspend and early part of device resume.
> > 
> > Thus, during the late suspend and the early resume of devices acquire
> > dpm_list_mtx only when dpm_list is going to be traversed and release
> > it right after that.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  drivers/base/power/main.c |    4 ++++
> >  kernel/kexec.c            |    2 --
> >  kernel/power/disk.c       |   21 +++------------------
> >  kernel/power/main.c       |    7 +------
> >  4 files changed, 8 insertions(+), 26 deletions(-)
> > 
> 
> I try to apply the patch against lastest next tree(2009-05-22), but
> "patch -p1" is failured:
> 
> 
> [lm@linux-lm linux-2.6]$ patch -p1 <  ../patch_rx/INFO_possible_circular_locking_dependency_at_cleanup_workqueue_thread.patch 
> patching file kernel/power/disk.c
> Hunk #1 succeeded at 215 with fuzz 2.
> Hunk #3 succeeded at 278 with fuzz 1.
> Hunk #4 FAILED at 343.
> Hunk #5 succeeded at 396 with fuzz 2 (offset -4 lines).
> Hunk #6 FAILED at 454.
> Hunk #7 succeeded at 485 with fuzz 2.
> 2 out of 7 hunks FAILED -- saving rejects to file kernel/power/disk.c.rej
> patching file kernel/power/main.c
> Hunk #1 succeeded at 289 with fuzz 1 (offset 18 lines).
> patching file drivers/base/power/main.c
> Hunk #3 succeeded at 616 with fuzz 2.
> Hunk #4 succeeded at 625 with fuzz 2.
> patching file kernel/kexec.c
> Hunk #1 succeeded at 1451 with fuzz 2.
> Hunk #2 succeeded at 1488 with fuzz 2.

The patch applies to the mainline, since it'll be a 2.6.30 candidate if it's
confirmed to fix the problem.

Thanks,
Rafael
_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/linux-pm

Re: INFO: possible circular locking dependency at cleanup_workqueue_thread

[Ming Lei]

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

On Sun, 24 May 2009 13:09:13 +0200
"Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> On Sunday 24 May 2009, Ming Lei wrote:
> > 于 Sun, 24 May 2009 01:20:29 +0200
> > "Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> > [lm@linux-lm linux-2.6]$ patch -p1
> > <  ../patch_rx/INFO_possible_circular_locking_dependency_at_cleanup_workqueue_thread.patch
> > patching file kernel/power/disk.c Hunk #1 succeeded at 215 with
> > fuzz 2. Hunk #3 succeeded at 278 with fuzz 1.
> > Hunk #4 FAILED at 343.
> > Hunk #5 succeeded at 396 with fuzz 2 (offset -4 lines).
> > Hunk #6 FAILED at 454.
> > Hunk #7 succeeded at 485 with fuzz 2.
> > 2 out of 7 hunks FAILED -- saving rejects to file
> > kernel/power/disk.c.rej patching file kernel/power/main.c
> > Hunk #1 succeeded at 289 with fuzz 1 (offset 18 lines).
> > patching file drivers/base/power/main.c
> > Hunk #3 succeeded at 616 with fuzz 2.
> > Hunk #4 succeeded at 625 with fuzz 2.
> > patching file kernel/kexec.c
> > Hunk #1 succeeded at 1451 with fuzz 2.
> > Hunk #2 succeeded at 1488 with fuzz 2.
> 
> The patch applies to the mainline, since it'll be a 2.6.30 candidate
> if it's confirmed to fix the problem.

After applying the patch against 2.6.30-rc7, my dell d630 box can resume
from suspend(s2ram) or hibernation successfully, and lockdep does not 
complain with the kind of message in

	http://bugzilla.kernel.org/show_bug.cgi?id=13245.

Another thing, during resume from hibernation, I can find the following
messages:

	[    1.849584] device: 'network_throughput': device_add
	[    1.849630] PM: Adding info for No Bus:network_throughput
	[    1.849788] PM: Resume from disk failed.
 
I do not known if there is really something wrong in PM, after all my
box can work as before. Attachment is the dmesg info.

Thanks.

-- 
Lei Ming

[-- Attachment #2: dmesg.tar.gz --]
[-- Type: application/x-gzip, Size: 58118 bytes --]

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: INFO: possible circular locking dependency at cleanup_workqueue_thread

[Rafael J. Wysocki]

On Sunday 24 May 2009, Ming Lei wrote:
> On Sun, 24 May 2009 13:09:13 +0200
> "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> 
> > On Sunday 24 May 2009, Ming Lei wrote:
> > > 于 Sun, 24 May 2009 01:20:29 +0200
> > > "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> 
> > > [lm@linux-lm linux-2.6]$ patch -p1
> > > <  ../patch_rx/INFO_possible_circular_locking_dependency_at_cleanup_workqueue_thread.patch
> > > patching file kernel/power/disk.c Hunk #1 succeeded at 215 with
> > > fuzz 2. Hunk #3 succeeded at 278 with fuzz 1.
> > > Hunk #4 FAILED at 343.
> > > Hunk #5 succeeded at 396 with fuzz 2 (offset -4 lines).
> > > Hunk #6 FAILED at 454.
> > > Hunk #7 succeeded at 485 with fuzz 2.
> > > 2 out of 7 hunks FAILED -- saving rejects to file
> > > kernel/power/disk.c.rej patching file kernel/power/main.c
> > > Hunk #1 succeeded at 289 with fuzz 1 (offset 18 lines).
> > > patching file drivers/base/power/main.c
> > > Hunk #3 succeeded at 616 with fuzz 2.
> > > Hunk #4 succeeded at 625 with fuzz 2.
> > > patching file kernel/kexec.c
> > > Hunk #1 succeeded at 1451 with fuzz 2.
> > > Hunk #2 succeeded at 1488 with fuzz 2.
> > 
> > The patch applies to the mainline, since it'll be a 2.6.30 candidate
> > if it's confirmed to fix the problem.
> 
> After applying the patch against 2.6.30-rc7, my dell d630 box can resume
> from suspend(s2ram) or hibernation successfully, and lockdep does not 
> complain with the kind of message in
> 
> 	http://bugzilla.kernel.org/show_bug.cgi?id=13245.

OK, great.  I'm going to push the patch to Linus.

> Another thing, during resume from hibernation, I can find the following
> messages:
> 
> 	[    1.849584] device: 'network_throughput': device_add
> 	[    1.849630] PM: Adding info for No Bus:network_throughput
> 	[    1.849788] PM: Resume from disk failed.
>  
> I do not known if there is really something wrong in PM,

No, this only means that there was no hibernation image present on the resume
partition while the kernel was starting.

Thanks,
Rafael
_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/linux-pm

Re: INFO: possible circular locking dependency at cleanup_workqueue_thread

[Alan Stern]

On Sun, 24 May 2009, Rafael J. Wysocki wrote:

> The patch is appended for reference (Alan, please have a look; I can't recall
> why exactly we have called device_pm_lock() from the core suspend/hibernation
> code instead of acquiring the lock locally in drivers/base/power/main.c) and
> I'll attach it to the bug entry too.

I can't remember the reason either.  Probably there wasn't any.  The 
patch looks fine, and it has the nice added benefit that now the only 
user of device_pm_lock() will be device_move().

> ---
> From: Rafael J. Wysocki <rjw@sisk.pl>
> Subject: PM: Do not hold dpm_list_mtx while disabling/enabling nonboot CPUs
> 
> We shouldn't hold dpm_list_mtx while executing
> [disable|enable]_nonboot_cpus(), because theoretically this may lead
> to a deadlock as shown by the following example (provided by Johannes
> Berg):
> 
> CPU 3       CPU 2                     CPU 1
>                                       suspend/hibernate
>             something:
>             rtnl_lock()               device_pm_lock()
>                                        -> mutex_lock(&dpm_list_mtx)
> 
>             mutex_lock(&dpm_list_mtx)
> 
> linkwatch_work
>  -> rtnl_lock()
>                                       disable_nonboot_cpus()
>                                        -> flush CPU 3 workqueue
> 
> Fortunately, device drivers are supposed to stop any activities that
> might lead to the registration of new device objects and/or to the
> removal of the existing ones way before disable_nonboot_cpus() is

Strictly speaking, drivers are still allowed to unregister existing 
devices.  They are forbidden only to register new ones.  This shouldn't 
hurt anything, though.

> called, so it shouldn't be necessary to hold dpm_list_mtx over the
> entire late part of device suspend and early part of device resume.
> 
> Thus, during the late suspend and the early resume of devices acquire
> dpm_list_mtx only when dpm_list is going to be traversed and release
> it right after that.

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Re: INFO: possible circular locking dependency at cleanup_workqueue_thread

[Rafael J. Wysocki]

On Sunday 24 May 2009, Alan Stern wrote:
> On Sun, 24 May 2009, Rafael J. Wysocki wrote:
> 
> > The patch is appended for reference (Alan, please have a look; I can't recall
> > why exactly we have called device_pm_lock() from the core suspend/hibernation
> > code instead of acquiring the lock locally in drivers/base/power/main.c) and
> > I'll attach it to the bug entry too.
> 
> I can't remember the reason either.  Probably there wasn't any.  The 
> patch looks fine, and it has the nice added benefit that now the only 
> user of device_pm_lock() will be device_move().
>
> > ---
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > Subject: PM: Do not hold dpm_list_mtx while disabling/enabling nonboot CPUs
> > 
> > We shouldn't hold dpm_list_mtx while executing
> > [disable|enable]_nonboot_cpus(), because theoretically this may lead
> > to a deadlock as shown by the following example (provided by Johannes
> > Berg):
> > 
> > CPU 3       CPU 2                     CPU 1
> >                                       suspend/hibernate
> >             something:
> >             rtnl_lock()               device_pm_lock()
> >                                        -> mutex_lock(&dpm_list_mtx)
> > 
> >             mutex_lock(&dpm_list_mtx)
> > 
> > linkwatch_work
> >  -> rtnl_lock()
> >                                       disable_nonboot_cpus()
> >                                        -> flush CPU 3 workqueue
> > 
> > Fortunately, device drivers are supposed to stop any activities that
> > might lead to the registration of new device objects and/or to the
> > removal of the existing ones way before disable_nonboot_cpus() is
> 
> Strictly speaking, drivers are still allowed to unregister existing 
> devices.  They are forbidden only to register new ones.  This shouldn't 
> hurt anything, though.

You're right, I'll fix the changelog.
 
> > called, so it shouldn't be necessary to hold dpm_list_mtx over the
> > entire late part of device suspend and early part of device resume.
> > 
> > Thus, during the late suspend and the early resume of devices acquire
> > dpm_list_mtx only when dpm_list is going to be traversed and release
> > it right after that.
> 
> Acked-by: Alan Stern <stern@rowland.harvard.edu>

Thanks!

Best,
Rafael