lkml.org 
[lkml]   [2009]   [May]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: INFO: possible circular locking dependency at cleanup_workqueue_thread
    于 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
    --
    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/

    \
     
     \ /
      Last update: 2009-05-24 05:39    [W:0.056 / U:121.548 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site