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

    \
     
     \ /
      Last update: 2009-05-24 01:23    [W:0.045 / U:0.216 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site