[PATCH] ACPI / PM: Fix acpi_pm_notifier_lock vs. flush_workqueue() deadlock

[PATCH] ACPI / PM: Fix acpi_pm_notifier_lock vs. flush_workqueue() deadlock

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

acpi_remove_pm_notifier() ends up calling flush_workqueue() while
holding acpi_pm_notifier_lock, and that same lock is taken by
by the work via acpi_pm_notify_handler(). This can deadlock.

To fix the problem let's split the single lock into two: one to
protect the dev->wakeup between the work vs. add/remove, and
another one to handle notifier installation vs. removal.

After commit a1d14934ea4b ("workqueue/lockdep: 'Fix' flush_work()
annotation") I was able to kill the machine (Intel Braswell)
very easily with 'powertop --auto-tune', runtime suspending i915,
and trying to wake it up via the USB keyboard. The cases when
it didn't die are presumably explained by lockdep getting disabled
by something else (cpu hotplug locking issues usually).

Fortunately I still got a lockdep report over netconsole
(trickling in very slowly), even though the machine was
otherwise practically dead:

[  112.179806] ======================================================
[  114.670858] WARNING: possible circular locking dependency detected
[  117.155663] 4.13.0-rc6-bsw-bisect-00169-ga1d14934ea4b #119 Not tainted
[  119.658101] ------------------------------------------------------
[  121.310242] xhci_hcd 0000:00:14.0: xHCI host not responding to stop endpoint command.
[  121.313294] xhci_hcd 0000:00:14.0: xHCI host controller not responding, assume dead
[  121.313346] xhci_hcd 0000:00:14.0: HC died; cleaning up
[  121.313485] usb 1-6: USB disconnect, device number 3
[  121.313501] usb 1-6.2: USB disconnect, device number 4
[  134.747383] kworker/0:2/47 is trying to acquire lock:
[  137.220790]  (acpi_pm_notifier_lock){+.+.}, at: [<ffffffff813cafdf>] acpi_pm_notify_handler+0x2f/0x80
[  139.721524]
[  139.721524] but task is already holding lock:
[  144.672922]  ((&dpc->work)){+.+.}, at: [<ffffffff8109ce90>] process_one_work+0x160/0x720
[  147.184450]
[  147.184450] which lock already depends on the new lock.
[  147.184450]
[  154.604711]
[  154.604711] the existing dependency chain (in reverse order) is:
[  159.447888]
[  159.447888] -> #2 ((&dpc->work)){+.+.}:
[  164.183486]        __lock_acquire+0x1255/0x13f0
[  166.504313]        lock_acquire+0xb5/0x210
[  168.778973]        process_one_work+0x1b9/0x720
[  171.030316]        worker_thread+0x4c/0x440
[  173.257184]        kthread+0x154/0x190
[  175.456143]        ret_from_fork+0x27/0x40
[  177.624348]
[  177.624348] -> #1 ("kacpi_notify"){+.+.}:
[  181.850351]        __lock_acquire+0x1255/0x13f0
[  183.941695]        lock_acquire+0xb5/0x210
[  186.046115]        flush_workqueue+0xdd/0x510
[  190.408153]        acpi_os_wait_events_complete+0x31/0x40
[  192.625303]        acpi_remove_notify_handler+0x133/0x188
[  194.820829]        acpi_remove_pm_notifier+0x56/0x90
[  196.989068]        acpi_dev_pm_detach+0x5f/0xa0
[  199.145866]        dev_pm_domain_detach+0x27/0x30
[  201.285614]        i2c_device_probe+0x100/0x210
[  203.411118]        driver_probe_device+0x23e/0x310
[  205.522425]        __driver_attach+0xa3/0xb0
[  207.634268]        bus_for_each_dev+0x69/0xa0
[  209.714797]        driver_attach+0x1e/0x20
[  211.778258]        bus_add_driver+0x1bc/0x230
[  213.837162]        driver_register+0x60/0xe0
[  215.868162]        i2c_register_driver+0x42/0x70
[  217.869551]        0xffffffffa0172017
[  219.863009]        do_one_initcall+0x45/0x170
[  221.843863]        do_init_module+0x5f/0x204
[  223.817915]        load_module+0x225b/0x29b0
[  225.757234]        SyS_finit_module+0xc6/0xd0
[  227.661851]        do_syscall_64+0x5c/0x120
[  229.536819]        return_from_SYSCALL_64+0x0/0x7a
[  231.392444]
[  231.392444] -> #0 (acpi_pm_notifier_lock){+.+.}:
[  235.124914]        check_prev_add+0x44e/0x8a0
[  237.024795]        __lock_acquire+0x1255/0x13f0
[  238.937351]        lock_acquire+0xb5/0x210
[  240.840799]        __mutex_lock+0x75/0x940
[  242.709517]        mutex_lock_nested+0x1c/0x20
[  244.551478]        acpi_pm_notify_handler+0x2f/0x80
[  246.382052]        acpi_ev_notify_dispatch+0x44/0x5c
[  248.194412]        acpi_os_execute_deferred+0x14/0x30
[  250.003925]        process_one_work+0x1ec/0x720
[  251.803191]        worker_thread+0x4c/0x440
[  253.605307]        kthread+0x154/0x190
[  255.387498]        ret_from_fork+0x27/0x40
[  257.153175]
[  257.153175] other info that might help us debug this:
[  257.153175]
[  262.324392] Chain exists of:
[  262.324392]   acpi_pm_notifier_lock --> "kacpi_notify" --> (&dpc->work)
[  262.324392]
[  267.391997]  Possible unsafe locking scenario:
[  267.391997]
[  270.758262]        CPU0                    CPU1
[  272.431713]        ----                    ----
[  274.060756]   lock((&dpc->work));
[  275.646532]                                lock("kacpi_notify");
[  277.260772]                                lock((&dpc->work));
[  278.839146]   lock(acpi_pm_notifier_lock);
[  280.391902]
[  280.391902]  *** DEADLOCK ***
[  280.391902]
[  284.986385] 2 locks held by kworker/0:2/47:
[  286.524895]  #0:  ("kacpi_notify"){+.+.}, at: [<ffffffff8109ce90>] process_one_work+0x160/0x720
[  288.112927]  #1:  ((&dpc->work)){+.+.}, at: [<ffffffff8109ce90>] process_one_work+0x160/0x720
[  289.727725]

Cc: stable@vger.kernel.org
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Len Brown <lenb@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Ingo Molnar <mingo@kernel.org>
Fixes: c072530f391e ("ACPI / PM: Revork the handling of ACPI device wakeup notifications")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/acpi/device_pm.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index fbcc73f7a099..18af71057b44 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -387,6 +387,7 @@ EXPORT_SYMBOL(acpi_bus_power_manageable);
 
 #ifdef CONFIG_PM
 static DEFINE_MUTEX(acpi_pm_notifier_lock);
+static DEFINE_MUTEX(acpi_pm_notifier_install_lock);
 
 void acpi_pm_wakeup_event(struct device *dev)
 {
@@ -443,24 +444,25 @@ acpi_status acpi_add_pm_notifier(struct acpi_device *adev, struct device *dev,
 	if (!dev && !func)
 		return AE_BAD_PARAMETER;
 
-	mutex_lock(&acpi_pm_notifier_lock);
+	mutex_lock(&acpi_pm_notifier_install_lock);
 
 	if (adev->wakeup.flags.notifier_present)
 		goto out;
 
-	adev->wakeup.ws = wakeup_source_register(dev_name(&adev->dev));
-	adev->wakeup.context.dev = dev;
-	adev->wakeup.context.func = func;
-
 	status = acpi_install_notify_handler(adev->handle, ACPI_SYSTEM_NOTIFY,
 					     acpi_pm_notify_handler, NULL);
 	if (ACPI_FAILURE(status))
 		goto out;
 
+	mutex_lock(&acpi_pm_notifier_lock);
+	adev->wakeup.ws = wakeup_source_register(dev_name(&adev->dev));
+	adev->wakeup.context.dev = dev;
+	adev->wakeup.context.func = func;
 	adev->wakeup.flags.notifier_present = true;
+	mutex_unlock(&acpi_pm_notifier_lock);
 
  out:
-	mutex_unlock(&acpi_pm_notifier_lock);
+	mutex_unlock(&acpi_pm_notifier_install_lock);
 	return status;
 }
 
@@ -472,7 +474,7 @@ acpi_status acpi_remove_pm_notifier(struct acpi_device *adev)
 {
 	acpi_status status = AE_BAD_PARAMETER;
 
-	mutex_lock(&acpi_pm_notifier_lock);
+	mutex_lock(&acpi_pm_notifier_install_lock);
 
 	if (!adev->wakeup.flags.notifier_present)
 		goto out;
@@ -483,14 +485,15 @@ acpi_status acpi_remove_pm_notifier(struct acpi_device *adev)
 	if (ACPI_FAILURE(status))
 		goto out;
 
+	mutex_lock(&acpi_pm_notifier_lock);
 	adev->wakeup.context.func = NULL;
 	adev->wakeup.context.dev = NULL;
 	wakeup_source_unregister(adev->wakeup.ws);
-
 	adev->wakeup.flags.notifier_present = false;
+	mutex_unlock(&acpi_pm_notifier_lock);
 
  out:
-	mutex_unlock(&acpi_pm_notifier_lock);
+	mutex_unlock(&acpi_pm_notifier_install_lock);
 	return status;
 }
 
-- 
2.13.6

Re: [PATCH] ACPI / PM: Fix acpi_pm_notifier_lock vs. flush_workqueue() deadlock

[Rafael J. Wysocki]

On Tue, Nov 7, 2017 at 10:08 PM, Ville Syrjala
<ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> acpi_remove_pm_notifier() ends up calling flush_workqueue() while
> holding acpi_pm_notifier_lock, and that same lock is taken by
> by the work via acpi_pm_notify_handler(). This can deadlock.

OK, good catch!

[cut]

>
> Cc: stable@vger.kernel.org
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Len Brown <lenb@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> Fixes: c072530f391e ("ACPI / PM: Revork the handling of ACPI device wakeup notifications")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/acpi/device_pm.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> index fbcc73f7a099..18af71057b44 100644
> --- a/drivers/acpi/device_pm.c
> +++ b/drivers/acpi/device_pm.c
> @@ -387,6 +387,7 @@ EXPORT_SYMBOL(acpi_bus_power_manageable);
>
>  #ifdef CONFIG_PM
>  static DEFINE_MUTEX(acpi_pm_notifier_lock);
> +static DEFINE_MUTEX(acpi_pm_notifier_install_lock);
>
>  void acpi_pm_wakeup_event(struct device *dev)
>  {
> @@ -443,24 +444,25 @@ acpi_status acpi_add_pm_notifier(struct acpi_device *adev, struct device *dev,
>         if (!dev && !func)
>                 return AE_BAD_PARAMETER;
>
> -       mutex_lock(&acpi_pm_notifier_lock);
> +       mutex_lock(&acpi_pm_notifier_install_lock);
>
>         if (adev->wakeup.flags.notifier_present)
>                 goto out;
>
> -       adev->wakeup.ws = wakeup_source_register(dev_name(&adev->dev));
> -       adev->wakeup.context.dev = dev;
> -       adev->wakeup.context.func = func;
> -

But this doesn't look good to me.

notifier_present should be checked under acpi_pm_notifier_lock.

Actually, acpi_install_notify_handler() itself need not be called
under the lock, because it does sufficient checks of its own.

So say you do

mutex_lock(&acpi_pm_notifier_lock);

if (adev->wakeup.context.dev)
        goto out;

adev->wakeup.ws = wakeup_source_register(dev_name(&adev->dev));
adev->wakeup.context.dev = dev;
adev->wakeup.context.func = func;

mutex_unlock(&acpi_pm_notifier_lock);

>         status = acpi_install_notify_handler(adev->handle, ACPI_SYSTEM_NOTIFY,
>                                              acpi_pm_notify_handler, NULL);
>         if (ACPI_FAILURE(status))
>                 goto out;
>
> +       mutex_lock(&acpi_pm_notifier_lock);

And here you just set notifier_present under acpi_pm_notifier_lock.

> +       adev->wakeup.ws = wakeup_source_register(dev_name(&adev->dev));
> +       adev->wakeup.context.dev = dev;
> +       adev->wakeup.context.func = func;
>         adev->wakeup.flags.notifier_present = true;
> +       mutex_unlock(&acpi_pm_notifier_lock);
>
>   out:
> -       mutex_unlock(&acpi_pm_notifier_lock);
> +       mutex_unlock(&acpi_pm_notifier_install_lock);
>         return status;
>  }

Then on removal you can clear notifier_present first and drop the lock
around the acpi_remove_notify_handler() call and nothing bad will
happen.

If you call acpi_add_pm_notifier() twice in parallel, the first
instance will set context.dev and the second one will see it set and
bail out.  The first instance will then do the rest.

If you call acpi_remove_pm_notifier() twice in a row, the first
instance will see notifier_present set and will clear it, so the
second one will see notifier_present unset and it will bail out.

Now, if you call acpi_remove_pm_notifier() in parallel with
acpi_add_pm_notifier(), either the former will see notifier_present
unset and bail out, or the latter will see context.dev unset and bail
out.

It doesn't look like the outer lock is needed, or have I missed anything?

Thanks,
Rafael

Re: [PATCH] ACPI / PM: Fix acpi_pm_notifier_lock vs. flush_workqueue() deadlock

[Rafael J. Wysocki]

On Tuesday, November 7, 2017 11:47:54 PM CET Rafael J. Wysocki wrote:
> On Tue, Nov 7, 2017 at 10:08 PM, Ville Syrjala
> <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> >
> > acpi_remove_pm_notifier() ends up calling flush_workqueue() while
> > holding acpi_pm_notifier_lock, and that same lock is taken by
> > by the work via acpi_pm_notify_handler(). This can deadlock.
> 
> OK, good catch!
> 
> [cut]
> 
> >
> > Cc: stable@vger.kernel.org
> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Cc: Len Brown <lenb@kernel.org>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Tejun Heo <tj@kernel.org>
> > Cc: Ingo Molnar <mingo@kernel.org>
> > Fixes: c072530f391e ("ACPI / PM: Revork the handling of ACPI device wakeup notifications")
> > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/acpi/device_pm.c | 21 ++++++++++++---------
> >  1 file changed, 12 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> > index fbcc73f7a099..18af71057b44 100644
> > --- a/drivers/acpi/device_pm.c
> > +++ b/drivers/acpi/device_pm.c
> > @@ -387,6 +387,7 @@ EXPORT_SYMBOL(acpi_bus_power_manageable);
> >
> >  #ifdef CONFIG_PM
> >  static DEFINE_MUTEX(acpi_pm_notifier_lock);
> > +static DEFINE_MUTEX(acpi_pm_notifier_install_lock);
> >
> >  void acpi_pm_wakeup_event(struct device *dev)
> >  {
> > @@ -443,24 +444,25 @@ acpi_status acpi_add_pm_notifier(struct acpi_device *adev, struct device *dev,
> >         if (!dev && !func)
> >                 return AE_BAD_PARAMETER;
> >
> > -       mutex_lock(&acpi_pm_notifier_lock);
> > +       mutex_lock(&acpi_pm_notifier_install_lock);
> >
> >         if (adev->wakeup.flags.notifier_present)
> >                 goto out;
> >
> > -       adev->wakeup.ws = wakeup_source_register(dev_name(&adev->dev));
> > -       adev->wakeup.context.dev = dev;
> > -       adev->wakeup.context.func = func;
> > -
> 
> But this doesn't look good to me.
> 
> notifier_present should be checked under acpi_pm_notifier_lock.
> 
> Actually, acpi_install_notify_handler() itself need not be called
> under the lock, because it does sufficient checks of its own.
> 
> So say you do
> 
> mutex_lock(&acpi_pm_notifier_lock);
> 
> if (adev->wakeup.context.dev)
>         goto out;
> 
> adev->wakeup.ws = wakeup_source_register(dev_name(&adev->dev));
> adev->wakeup.context.dev = dev;
> adev->wakeup.context.func = func;
> 
> mutex_unlock(&acpi_pm_notifier_lock);
> 
> >         status = acpi_install_notify_handler(adev->handle, ACPI_SYSTEM_NOTIFY,
> >                                              acpi_pm_notify_handler, NULL);
> >         if (ACPI_FAILURE(status))
> >                 goto out;
> >
> > +       mutex_lock(&acpi_pm_notifier_lock);
> 
> And here you just set notifier_present under acpi_pm_notifier_lock.
> 
> > +       adev->wakeup.ws = wakeup_source_register(dev_name(&adev->dev));
> > +       adev->wakeup.context.dev = dev;
> > +       adev->wakeup.context.func = func;
> >         adev->wakeup.flags.notifier_present = true;
> > +       mutex_unlock(&acpi_pm_notifier_lock);
> >
> >   out:
> > -       mutex_unlock(&acpi_pm_notifier_lock);
> > +       mutex_unlock(&acpi_pm_notifier_install_lock);
> >         return status;
> >  }
> 
> Then on removal you can clear notifier_present first and drop the lock
> around the acpi_remove_notify_handler() call and nothing bad will
> happen.
> 
> If you call acpi_add_pm_notifier() twice in parallel, the first
> instance will set context.dev and the second one will see it set and
> bail out.  The first instance will then do the rest.
> 
> If you call acpi_remove_pm_notifier() twice in a row, the first
> instance will see notifier_present set and will clear it, so the
> second one will see notifier_present unset and it will bail out.
> 
> Now, if you call acpi_remove_pm_notifier() in parallel with
> acpi_add_pm_notifier(), either the former will see notifier_present
> unset and bail out, or the latter will see context.dev unset and bail
> out.
> 
> It doesn't look like the outer lock is needed, or have I missed anything?

So something like the below (totally untested) should work too, shouldn't it?

---
 drivers/acpi/device_pm.c |   35 +++++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -424,6 +424,13 @@ static void acpi_pm_notify_handler(acpi_
 	acpi_bus_put_acpi_device(adev);
 }
 
+static void acpi_dev_wakeup_cleanup(struct acpi_device *adev)
+{
+	adev->wakeup.context.func = NULL;
+	adev->wakeup.context.dev = NULL;
+	wakeup_source_unregister(adev->wakeup.ws);
+}
+
 /**
  * acpi_add_pm_notifier - Register PM notify handler for given ACPI device.
  * @adev: ACPI device to add the notify handler for.
@@ -445,17 +452,24 @@ acpi_status acpi_add_pm_notifier(struct
 
 	mutex_lock(&acpi_pm_notifier_lock);
 
-	if (adev->wakeup.flags.notifier_present)
+	if (adev->wakeup.context.dev || adev->wakeup.context.func)
 		goto out;
 
 	adev->wakeup.ws = wakeup_source_register(dev_name(&adev->dev));
 	adev->wakeup.context.dev = dev;
 	adev->wakeup.context.func = func;
 
+	mutex_unlock(&acpi_pm_notifier_lock);
+
 	status = acpi_install_notify_handler(adev->handle, ACPI_SYSTEM_NOTIFY,
 					     acpi_pm_notify_handler, NULL);
-	if (ACPI_FAILURE(status))
+
+	mutex_lock(&acpi_pm_notifier_lock);
+
+	if (ACPI_FAILURE(status)) {
+		acpi_dev_wakeup_cleanup(adev);
 		goto out;
+	}
 
 	adev->wakeup.flags.notifier_present = true;
 
@@ -477,17 +491,22 @@ acpi_status acpi_remove_pm_notifier(stru
 	if (!adev->wakeup.flags.notifier_present)
 		goto out;
 
+	adev->wakeup.flags.notifier_present = false;
+
+	mutex_unlock(&acpi_pm_notifier_lock);
+
 	status = acpi_remove_notify_handler(adev->handle,
 					    ACPI_SYSTEM_NOTIFY,
 					    acpi_pm_notify_handler);
-	if (ACPI_FAILURE(status))
-		goto out;
 
-	adev->wakeup.context.func = NULL;
-	adev->wakeup.context.dev = NULL;
-	wakeup_source_unregister(adev->wakeup.ws);
+	mutex_lock(&acpi_pm_notifier_lock);
 
-	adev->wakeup.flags.notifier_present = false;
+	if (ACPI_FAILURE(status)) {
+		adev->wakeup.flags.notifier_present = true;
+		goto out;
+	}
+
+	acpi_dev_wakeup_cleanup(adev);
 
  out:
 	mutex_unlock(&acpi_pm_notifier_lock);

Re: [PATCH] ACPI / PM: Fix acpi_pm_notifier_lock vs. flush_workqueue() deadlock

[Rafael J. Wysocki]

On Tue, Nov 7, 2017 at 11:47 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Tue, Nov 7, 2017 at 10:08 PM, Ville Syrjala
> <ville.syrjala@linux.intel.com> wrote:
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> acpi_remove_pm_notifier() ends up calling flush_workqueue() while
>> holding acpi_pm_notifier_lock, and that same lock is taken by
>> by the work via acpi_pm_notify_handler(). This can deadlock.
>
> OK, good catch!
>
> [cut]
>
>>
>> Cc: stable@vger.kernel.org
>> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Cc: Len Brown <lenb@kernel.org>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: Tejun Heo <tj@kernel.org>
>> Cc: Ingo Molnar <mingo@kernel.org>
>> Fixes: c072530f391e ("ACPI / PM: Revork the handling of ACPI device wakeup notifications")
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> ---
>>  drivers/acpi/device_pm.c | 21 ++++++++++++---------
>>  1 file changed, 12 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
>> index fbcc73f7a099..18af71057b44 100644
>> --- a/drivers/acpi/device_pm.c
>> +++ b/drivers/acpi/device_pm.c
>> @@ -387,6 +387,7 @@ EXPORT_SYMBOL(acpi_bus_power_manageable);
>>
>>  #ifdef CONFIG_PM
>>  static DEFINE_MUTEX(acpi_pm_notifier_lock);
>> +static DEFINE_MUTEX(acpi_pm_notifier_install_lock);
>>
>>  void acpi_pm_wakeup_event(struct device *dev)
>>  {
>> @@ -443,24 +444,25 @@ acpi_status acpi_add_pm_notifier(struct acpi_device *adev, struct device *dev,
>>         if (!dev && !func)
>>                 return AE_BAD_PARAMETER;
>>
>> -       mutex_lock(&acpi_pm_notifier_lock);
>> +       mutex_lock(&acpi_pm_notifier_install_lock);
>>
>>         if (adev->wakeup.flags.notifier_present)
>>                 goto out;
>>
>> -       adev->wakeup.ws = wakeup_source_register(dev_name(&adev->dev));
>> -       adev->wakeup.context.dev = dev;
>> -       adev->wakeup.context.func = func;
>> -
>
> But this doesn't look good to me.
>
> notifier_present should be checked under acpi_pm_notifier_lock.

Well, not really, so the above is OK.

However, I still would prefer to avoid adding the outer lock.

Thanks,
Rafael

Re: [PATCH] ACPI / PM: Fix acpi_pm_notifier_lock vs. flush_workqueue() deadlock

[Rafael J. Wysocki]

On Wed, Nov 8, 2017 at 12:06 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Tuesday, November 7, 2017 11:47:54 PM CET Rafael J. Wysocki wrote:
>> On Tue, Nov 7, 2017 at 10:08 PM, Ville Syrjala
>> <ville.syrjala@linux.intel.com> wrote:
>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >
>> > acpi_remove_pm_notifier() ends up calling flush_workqueue() while
>> > holding acpi_pm_notifier_lock, and that same lock is taken by
>> > by the work via acpi_pm_notify_handler(). This can deadlock.
>>
>> OK, good catch!
>>
>> [cut]
>>
>> >
>> > Cc: stable@vger.kernel.org
>> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> > Cc: Len Brown <lenb@kernel.org>
>> > Cc: Peter Zijlstra <peterz@infradead.org>
>> > Cc: Tejun Heo <tj@kernel.org>
>> > Cc: Ingo Molnar <mingo@kernel.org>
>> > Fixes: c072530f391e ("ACPI / PM: Revork the handling of ACPI device wakeup notifications")
>> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > ---
>> >  drivers/acpi/device_pm.c | 21 ++++++++++++---------
>> >  1 file changed, 12 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
>> > index fbcc73f7a099..18af71057b44 100644
>> > --- a/drivers/acpi/device_pm.c
>> > +++ b/drivers/acpi/device_pm.c
>> > @@ -387,6 +387,7 @@ EXPORT_SYMBOL(acpi_bus_power_manageable);
>> >
>> >  #ifdef CONFIG_PM
>> >  static DEFINE_MUTEX(acpi_pm_notifier_lock);
>> > +static DEFINE_MUTEX(acpi_pm_notifier_install_lock);
>> >
>> >  void acpi_pm_wakeup_event(struct device *dev)
>> >  {
>> > @@ -443,24 +444,25 @@ acpi_status acpi_add_pm_notifier(struct acpi_device *adev, struct device *dev,
>> >         if (!dev && !func)
>> >                 return AE_BAD_PARAMETER;
>> >
>> > -       mutex_lock(&acpi_pm_notifier_lock);
>> > +       mutex_lock(&acpi_pm_notifier_install_lock);
>> >
>> >         if (adev->wakeup.flags.notifier_present)
>> >                 goto out;
>> >
>> > -       adev->wakeup.ws = wakeup_source_register(dev_name(&adev->dev));
>> > -       adev->wakeup.context.dev = dev;
>> > -       adev->wakeup.context.func = func;
>> > -
>>
>> But this doesn't look good to me.
>>
>> notifier_present should be checked under acpi_pm_notifier_lock.
>>
>> Actually, acpi_install_notify_handler() itself need not be called
>> under the lock, because it does sufficient checks of its own.
>>
>> So say you do
>>
>> mutex_lock(&acpi_pm_notifier_lock);
>>
>> if (adev->wakeup.context.dev)
>>         goto out;
>>
>> adev->wakeup.ws = wakeup_source_register(dev_name(&adev->dev));
>> adev->wakeup.context.dev = dev;
>> adev->wakeup.context.func = func;
>>
>> mutex_unlock(&acpi_pm_notifier_lock);
>>
>> >         status = acpi_install_notify_handler(adev->handle, ACPI_SYSTEM_NOTIFY,
>> >                                              acpi_pm_notify_handler, NULL);
>> >         if (ACPI_FAILURE(status))
>> >                 goto out;
>> >
>> > +       mutex_lock(&acpi_pm_notifier_lock);
>>
>> And here you just set notifier_present under acpi_pm_notifier_lock.
>>
>> > +       adev->wakeup.ws = wakeup_source_register(dev_name(&adev->dev));
>> > +       adev->wakeup.context.dev = dev;
>> > +       adev->wakeup.context.func = func;
>> >         adev->wakeup.flags.notifier_present = true;
>> > +       mutex_unlock(&acpi_pm_notifier_lock);
>> >
>> >   out:
>> > -       mutex_unlock(&acpi_pm_notifier_lock);
>> > +       mutex_unlock(&acpi_pm_notifier_install_lock);
>> >         return status;
>> >  }
>>
>> Then on removal you can clear notifier_present first and drop the lock
>> around the acpi_remove_notify_handler() call and nothing bad will
>> happen.
>>
>> If you call acpi_add_pm_notifier() twice in parallel, the first
>> instance will set context.dev and the second one will see it set and
>> bail out.  The first instance will then do the rest.
>>
>> If you call acpi_remove_pm_notifier() twice in a row, the first
>> instance will see notifier_present set and will clear it, so the
>> second one will see notifier_present unset and it will bail out.
>>
>> Now, if you call acpi_remove_pm_notifier() in parallel with
>> acpi_add_pm_notifier(), either the former will see notifier_present
>> unset and bail out, or the latter will see context.dev unset and bail
>> out.
>>
>> It doesn't look like the outer lock is needed, or have I missed anything?
>
> So something like the below (totally untested) should work too, shouldn't it?

There is a problem if a device is removed while acpi_add_pm_notifier()
is still in progress, in which case with my patch the
acpi_remove_pm_notifier() called from the removal path may bail out
prematurely (that doesn't seem likely to happen, but still I don't see
why it cannot happen), so I'll just use your patch. :-)

Thanks,
Rafael

Re: [PATCH] ACPI / PM: Fix acpi_pm_notifier_lock vs. flush_workqueue() deadlock

[Ville Syrjälä]

On Wed, Nov 08, 2017 at 01:23:56PM +0100, Rafael J. Wysocki wrote:
> On Wed, Nov 8, 2017 at 12:06 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Tuesday, November 7, 2017 11:47:54 PM CET Rafael J. Wysocki wrote:
> >> On Tue, Nov 7, 2017 at 10:08 PM, Ville Syrjala
> >> <ville.syrjala@linux.intel.com> wrote:
> >> > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> >> >
> >> > acpi_remove_pm_notifier() ends up calling flush_workqueue() while
> >> > holding acpi_pm_notifier_lock, and that same lock is taken by
> >> > by the work via acpi_pm_notify_handler(). This can deadlock.
> >>
> >> OK, good catch!
> >>
> >> [cut]
> >>
> >> >
> >> > Cc: stable@vger.kernel.org
> >> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >> > Cc: Len Brown <lenb@kernel.org>
> >> > Cc: Peter Zijlstra <peterz@infradead.org>
> >> > Cc: Tejun Heo <tj@kernel.org>
> >> > Cc: Ingo Molnar <mingo@kernel.org>
> >> > Fixes: c072530f391e ("ACPI / PM: Revork the handling of ACPI device wakeup notifications")
> >> > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> >> > ---
> >> >  drivers/acpi/device_pm.c | 21 ++++++++++++---------
> >> >  1 file changed, 12 insertions(+), 9 deletions(-)
> >> >
> >> > diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> >> > index fbcc73f7a099..18af71057b44 100644
> >> > --- a/drivers/acpi/device_pm.c
> >> > +++ b/drivers/acpi/device_pm.c
> >> > @@ -387,6 +387,7 @@ EXPORT_SYMBOL(acpi_bus_power_manageable);
> >> >
> >> >  #ifdef CONFIG_PM
> >> >  static DEFINE_MUTEX(acpi_pm_notifier_lock);
> >> > +static DEFINE_MUTEX(acpi_pm_notifier_install_lock);
> >> >
> >> >  void acpi_pm_wakeup_event(struct device *dev)
> >> >  {
> >> > @@ -443,24 +444,25 @@ acpi_status acpi_add_pm_notifier(struct acpi_device *adev, struct device *dev,
> >> >         if (!dev && !func)
> >> >                 return AE_BAD_PARAMETER;
> >> >
> >> > -       mutex_lock(&acpi_pm_notifier_lock);
> >> > +       mutex_lock(&acpi_pm_notifier_install_lock);
> >> >
> >> >         if (adev->wakeup.flags.notifier_present)
> >> >                 goto out;
> >> >
> >> > -       adev->wakeup.ws = wakeup_source_register(dev_name(&adev->dev));
> >> > -       adev->wakeup.context.dev = dev;
> >> > -       adev->wakeup.context.func = func;
> >> > -
> >>
> >> But this doesn't look good to me.
> >>
> >> notifier_present should be checked under acpi_pm_notifier_lock.
> >>
> >> Actually, acpi_install_notify_handler() itself need not be called
> >> under the lock, because it does sufficient checks of its own.
> >>
> >> So say you do
> >>
> >> mutex_lock(&acpi_pm_notifier_lock);
> >>
> >> if (adev->wakeup.context.dev)
> >>         goto out;
> >>
> >> adev->wakeup.ws = wakeup_source_register(dev_name(&adev->dev));
> >> adev->wakeup.context.dev = dev;
> >> adev->wakeup.context.func = func;
> >>
> >> mutex_unlock(&acpi_pm_notifier_lock);
> >>
> >> >         status = acpi_install_notify_handler(adev->handle, ACPI_SYSTEM_NOTIFY,
> >> >                                              acpi_pm_notify_handler, NULL);
> >> >         if (ACPI_FAILURE(status))
> >> >                 goto out;
> >> >
> >> > +       mutex_lock(&acpi_pm_notifier_lock);
> >>
> >> And here you just set notifier_present under acpi_pm_notifier_lock.
> >>
> >> > +       adev->wakeup.ws = wakeup_source_register(dev_name(&adev->dev));
> >> > +       adev->wakeup.context.dev = dev;
> >> > +       adev->wakeup.context.func = func;
> >> >         adev->wakeup.flags.notifier_present = true;
> >> > +       mutex_unlock(&acpi_pm_notifier_lock);
> >> >
> >> >   out:
> >> > -       mutex_unlock(&acpi_pm_notifier_lock);
> >> > +       mutex_unlock(&acpi_pm_notifier_install_lock);
> >> >         return status;
> >> >  }
> >>
> >> Then on removal you can clear notifier_present first and drop the lock
> >> around the acpi_remove_notify_handler() call and nothing bad will
> >> happen.
> >>
> >> If you call acpi_add_pm_notifier() twice in parallel, the first
> >> instance will set context.dev and the second one will see it set and
> >> bail out.  The first instance will then do the rest.
> >>
> >> If you call acpi_remove_pm_notifier() twice in a row, the first
> >> instance will see notifier_present set and will clear it, so the
> >> second one will see notifier_present unset and it will bail out.
> >>
> >> Now, if you call acpi_remove_pm_notifier() in parallel with
> >> acpi_add_pm_notifier(), either the former will see notifier_present
> >> unset and bail out, or the latter will see context.dev unset and bail
> >> out.
> >>
> >> It doesn't look like the outer lock is needed, or have I missed anything?
> >
> > So something like the below (totally untested) should work too, shouldn't it?
> 
> There is a problem if a device is removed while acpi_add_pm_notifier()
> is still in progress, in which case with my patch the
> acpi_remove_pm_notifier() called from the removal path may bail out
> prematurely (that doesn't seem likely to happen, but still I don't see
> why it cannot happen), so I'll just use your patch. :-)

OK. I was just looking at your version and was pretty much convinced
that it would work. But I'll take your word that it might not :)

-- 
Ville Syrj�l�
Intel OTC

Re: [PATCH] ACPI / PM: Fix acpi_pm_notifier_lock vs. flush_workqueue() deadlock

[Rafael J. Wysocki]

On Wednesday, November 8, 2017 1:31:22 PM CET Ville Syrj�l� wrote:
> On Wed, Nov 08, 2017 at 01:23:56PM +0100, Rafael J. Wysocki wrote:
> > On Wed, Nov 8, 2017 at 12:06 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > On Tuesday, November 7, 2017 11:47:54 PM CET Rafael J. Wysocki wrote:
> > >> On Tue, Nov 7, 2017 at 10:08 PM, Ville Syrjala
> > >> <ville.syrjala@linux.intel.com> wrote:
> > >> > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > >> >
> > >> > acpi_remove_pm_notifier() ends up calling flush_workqueue() while
> > >> > holding acpi_pm_notifier_lock, and that same lock is taken by
> > >> > by the work via acpi_pm_notify_handler(). This can deadlock.
> > >>
> > >> OK, good catch!
> > >>
> > >> [cut]
> > >>
> > >> >
> > >> > Cc: stable@vger.kernel.org
> > >> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >> > Cc: Len Brown <lenb@kernel.org>
> > >> > Cc: Peter Zijlstra <peterz@infradead.org>
> > >> > Cc: Tejun Heo <tj@kernel.org>
> > >> > Cc: Ingo Molnar <mingo@kernel.org>
> > >> > Fixes: c072530f391e ("ACPI / PM: Revork the handling of ACPI device wakeup notifications")
> > >> > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > >> > ---
> > >> >  drivers/acpi/device_pm.c | 21 ++++++++++++---------
> > >> >  1 file changed, 12 insertions(+), 9 deletions(-)
> > >> >
> > >> > diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> > >> > index fbcc73f7a099..18af71057b44 100644
> > >> > --- a/drivers/acpi/device_pm.c
> > >> > +++ b/drivers/acpi/device_pm.c
> > >> > @@ -387,6 +387,7 @@ EXPORT_SYMBOL(acpi_bus_power_manageable);
> > >> >
> > >> >  #ifdef CONFIG_PM
> > >> >  static DEFINE_MUTEX(acpi_pm_notifier_lock);
> > >> > +static DEFINE_MUTEX(acpi_pm_notifier_install_lock);
> > >> >
> > >> >  void acpi_pm_wakeup_event(struct device *dev)
> > >> >  {
> > >> > @@ -443,24 +444,25 @@ acpi_status acpi_add_pm_notifier(struct acpi_device *adev, struct device *dev,
> > >> >         if (!dev && !func)
> > >> >                 return AE_BAD_PARAMETER;
> > >> >
> > >> > -       mutex_lock(&acpi_pm_notifier_lock);
> > >> > +       mutex_lock(&acpi_pm_notifier_install_lock);
> > >> >
> > >> >         if (adev->wakeup.flags.notifier_present)
> > >> >                 goto out;
> > >> >
> > >> > -       adev->wakeup.ws = wakeup_source_register(dev_name(&adev->dev));
> > >> > -       adev->wakeup.context.dev = dev;
> > >> > -       adev->wakeup.context.func = func;
> > >> > -
> > >>
> > >> But this doesn't look good to me.
> > >>
> > >> notifier_present should be checked under acpi_pm_notifier_lock.
> > >>
> > >> Actually, acpi_install_notify_handler() itself need not be called
> > >> under the lock, because it does sufficient checks of its own.
> > >>
> > >> So say you do
> > >>
> > >> mutex_lock(&acpi_pm_notifier_lock);
> > >>
> > >> if (adev->wakeup.context.dev)
> > >>         goto out;
> > >>
> > >> adev->wakeup.ws = wakeup_source_register(dev_name(&adev->dev));
> > >> adev->wakeup.context.dev = dev;
> > >> adev->wakeup.context.func = func;
> > >>
> > >> mutex_unlock(&acpi_pm_notifier_lock);
> > >>
> > >> >         status = acpi_install_notify_handler(adev->handle, ACPI_SYSTEM_NOTIFY,
> > >> >                                              acpi_pm_notify_handler, NULL);
> > >> >         if (ACPI_FAILURE(status))
> > >> >                 goto out;
> > >> >
> > >> > +       mutex_lock(&acpi_pm_notifier_lock);
> > >>
> > >> And here you just set notifier_present under acpi_pm_notifier_lock.
> > >>
> > >> > +       adev->wakeup.ws = wakeup_source_register(dev_name(&adev->dev));
> > >> > +       adev->wakeup.context.dev = dev;
> > >> > +       adev->wakeup.context.func = func;
> > >> >         adev->wakeup.flags.notifier_present = true;
> > >> > +       mutex_unlock(&acpi_pm_notifier_lock);
> > >> >
> > >> >   out:
> > >> > -       mutex_unlock(&acpi_pm_notifier_lock);
> > >> > +       mutex_unlock(&acpi_pm_notifier_install_lock);
> > >> >         return status;
> > >> >  }
> > >>
> > >> Then on removal you can clear notifier_present first and drop the lock
> > >> around the acpi_remove_notify_handler() call and nothing bad will
> > >> happen.
> > >>
> > >> If you call acpi_add_pm_notifier() twice in parallel, the first
> > >> instance will set context.dev and the second one will see it set and
> > >> bail out.  The first instance will then do the rest.
> > >>
> > >> If you call acpi_remove_pm_notifier() twice in a row, the first
> > >> instance will see notifier_present set and will clear it, so the
> > >> second one will see notifier_present unset and it will bail out.
> > >>
> > >> Now, if you call acpi_remove_pm_notifier() in parallel with
> > >> acpi_add_pm_notifier(), either the former will see notifier_present
> > >> unset and bail out, or the latter will see context.dev unset and bail
> > >> out.
> > >>
> > >> It doesn't look like the outer lock is needed, or have I missed anything?
> > >
> > > So something like the below (totally untested) should work too, shouldn't it?
> > 
> > There is a problem if a device is removed while acpi_add_pm_notifier()
> > is still in progress, in which case with my patch the
> > acpi_remove_pm_notifier() called from the removal path may bail out
> > prematurely (that doesn't seem likely to happen, but still I don't see
> > why it cannot happen), so I'll just use your patch. :-)
> 
> OK. I was just looking at your version and was pretty much convinced
> that it would work. But I'll take your word that it might not :)

Well, you don't have to. :-)

The scenario I have in mind is as follows:

1. acpi_add_pm_notifier() sets context.dev and context.func and drops the
   lock.  notifier_present is still unset.

2. acpi_remove_pm_notifier() checks notifier_present under the lock.
   It is (still) unset, so the function decides that there's nothing to do.

3. acpi_add_pm_notifier() continues with notifier installation and the
   device goes away at the same time.

Thanks,
Rafael

Re: [PATCH] ACPI / PM: Fix acpi_pm_notifier_lock vs. flush_workqueue() deadlock

[Rafael J. Wysocki]

On Wed, Nov 8, 2017 at 1:41 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Wednesday, November 8, 2017 1:31:22 PM CET Ville Syrjälä wrote:
>> On Wed, Nov 08, 2017 at 01:23:56PM +0100, Rafael J. Wysocki wrote:
>> > On Wed, Nov 8, 2017 at 12:06 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> > > On Tuesday, November 7, 2017 11:47:54 PM CET Rafael J. Wysocki wrote:
>> > >> On Tue, Nov 7, 2017 at 10:08 PM, Ville Syrjala
>> > >> <ville.syrjala@linux.intel.com> wrote:
>> > >> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > >> >
>> > >> > acpi_remove_pm_notifier() ends up calling flush_workqueue() while
>> > >> > holding acpi_pm_notifier_lock, and that same lock is taken by
>> > >> > by the work via acpi_pm_notify_handler(). This can deadlock.
>> > >>
>> > >> OK, good catch!
>> > >>
>> > >> [cut]
>> > >>
>> > >> >
>> > >> > Cc: stable@vger.kernel.org
>> > >> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> > >> > Cc: Len Brown <lenb@kernel.org>
>> > >> > Cc: Peter Zijlstra <peterz@infradead.org>
>> > >> > Cc: Tejun Heo <tj@kernel.org>
>> > >> > Cc: Ingo Molnar <mingo@kernel.org>
>> > >> > Fixes: c072530f391e ("ACPI / PM: Revork the handling of ACPI device wakeup notifications")
>> > >> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > >> > ---
>> > >> >  drivers/acpi/device_pm.c | 21 ++++++++++++---------
>> > >> >  1 file changed, 12 insertions(+), 9 deletions(-)
>> > >> >
>> > >> > diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
>> > >> > index fbcc73f7a099..18af71057b44 100644
>> > >> > --- a/drivers/acpi/device_pm.c
>> > >> > +++ b/drivers/acpi/device_pm.c
>> > >> > @@ -387,6 +387,7 @@ EXPORT_SYMBOL(acpi_bus_power_manageable);
>> > >> >
>> > >> >  #ifdef CONFIG_PM
>> > >> >  static DEFINE_MUTEX(acpi_pm_notifier_lock);
>> > >> > +static DEFINE_MUTEX(acpi_pm_notifier_install_lock);
>> > >> >
>> > >> >  void acpi_pm_wakeup_event(struct device *dev)
>> > >> >  {
>> > >> > @@ -443,24 +444,25 @@ acpi_status acpi_add_pm_notifier(struct acpi_device *adev, struct device *dev,
>> > >> >         if (!dev && !func)
>> > >> >                 return AE_BAD_PARAMETER;
>> > >> >
>> > >> > -       mutex_lock(&acpi_pm_notifier_lock);
>> > >> > +       mutex_lock(&acpi_pm_notifier_install_lock);
>> > >> >
>> > >> >         if (adev->wakeup.flags.notifier_present)
>> > >> >                 goto out;
>> > >> >
>> > >> > -       adev->wakeup.ws = wakeup_source_register(dev_name(&adev->dev));
>> > >> > -       adev->wakeup.context.dev = dev;
>> > >> > -       adev->wakeup.context.func = func;
>> > >> > -
>> > >>
>> > >> But this doesn't look good to me.
>> > >>
>> > >> notifier_present should be checked under acpi_pm_notifier_lock.
>> > >>
>> > >> Actually, acpi_install_notify_handler() itself need not be called
>> > >> under the lock, because it does sufficient checks of its own.
>> > >>
>> > >> So say you do
>> > >>
>> > >> mutex_lock(&acpi_pm_notifier_lock);
>> > >>
>> > >> if (adev->wakeup.context.dev)
>> > >>         goto out;
>> > >>
>> > >> adev->wakeup.ws = wakeup_source_register(dev_name(&adev->dev));
>> > >> adev->wakeup.context.dev = dev;
>> > >> adev->wakeup.context.func = func;
>> > >>
>> > >> mutex_unlock(&acpi_pm_notifier_lock);
>> > >>
>> > >> >         status = acpi_install_notify_handler(adev->handle, ACPI_SYSTEM_NOTIFY,
>> > >> >                                              acpi_pm_notify_handler, NULL);
>> > >> >         if (ACPI_FAILURE(status))
>> > >> >                 goto out;
>> > >> >
>> > >> > +       mutex_lock(&acpi_pm_notifier_lock);
>> > >>
>> > >> And here you just set notifier_present under acpi_pm_notifier_lock.
>> > >>
>> > >> > +       adev->wakeup.ws = wakeup_source_register(dev_name(&adev->dev));
>> > >> > +       adev->wakeup.context.dev = dev;
>> > >> > +       adev->wakeup.context.func = func;
>> > >> >         adev->wakeup.flags.notifier_present = true;
>> > >> > +       mutex_unlock(&acpi_pm_notifier_lock);
>> > >> >
>> > >> >   out:
>> > >> > -       mutex_unlock(&acpi_pm_notifier_lock);
>> > >> > +       mutex_unlock(&acpi_pm_notifier_install_lock);
>> > >> >         return status;
>> > >> >  }
>> > >>
>> > >> Then on removal you can clear notifier_present first and drop the lock
>> > >> around the acpi_remove_notify_handler() call and nothing bad will
>> > >> happen.
>> > >>
>> > >> If you call acpi_add_pm_notifier() twice in parallel, the first
>> > >> instance will set context.dev and the second one will see it set and
>> > >> bail out.  The first instance will then do the rest.
>> > >>
>> > >> If you call acpi_remove_pm_notifier() twice in a row, the first
>> > >> instance will see notifier_present set and will clear it, so the
>> > >> second one will see notifier_present unset and it will bail out.
>> > >>
>> > >> Now, if you call acpi_remove_pm_notifier() in parallel with
>> > >> acpi_add_pm_notifier(), either the former will see notifier_present
>> > >> unset and bail out, or the latter will see context.dev unset and bail
>> > >> out.
>> > >>
>> > >> It doesn't look like the outer lock is needed, or have I missed anything?
>> > >
>> > > So something like the below (totally untested) should work too, shouldn't it?
>> >
>> > There is a problem if a device is removed while acpi_add_pm_notifier()
>> > is still in progress, in which case with my patch the
>> > acpi_remove_pm_notifier() called from the removal path may bail out
>> > prematurely (that doesn't seem likely to happen, but still I don't see
>> > why it cannot happen), so I'll just use your patch. :-)
>>
>> OK. I was just looking at your version and was pretty much convinced
>> that it would work. But I'll take your word that it might not :)
>
> Well, you don't have to. :-)
>
> The scenario I have in mind is as follows:
>
> 1. acpi_add_pm_notifier() sets context.dev and context.func and drops the
>    lock.  notifier_present is still unset.
>
> 2. acpi_remove_pm_notifier() checks notifier_present under the lock.
>    It is (still) unset, so the function decides that there's nothing to do.
>
> 3. acpi_add_pm_notifier() continues with notifier installation and the
>    device goes away at the same time.

Of course, the outer lock doesn't help against
acpi_remove_pm_notifier() in the removal path running right before
acpi_add_pm_notifier(), so if there's no other mutual exclusion, we
still have a problem in there (need to check that).

Thanks,
Rafael

Re: [PATCH] ACPI / PM: Fix acpi_pm_notifier_lock vs. flush_workqueue() deadlock

[Ville Syrjälä]

On Wed, Nov 08, 2017 at 01:41:06PM +0100, Rafael J. Wysocki wrote:
> On Wednesday, November 8, 2017 1:31:22 PM CET Ville Syrj�l� wrote:
> > On Wed, Nov 08, 2017 at 01:23:56PM +0100, Rafael J. Wysocki wrote:
> > > On Wed, Nov 8, 2017 at 12:06 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > > On Tuesday, November 7, 2017 11:47:54 PM CET Rafael J. Wysocki wrote:
> > > >> On Tue, Nov 7, 2017 at 10:08 PM, Ville Syrjala
> > > >> <ville.syrjala@linux.intel.com> wrote:
> > > >> > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > > >> >
> > > >> > acpi_remove_pm_notifier() ends up calling flush_workqueue() while
> > > >> > holding acpi_pm_notifier_lock, and that same lock is taken by
> > > >> > by the work via acpi_pm_notify_handler(). This can deadlock.
> > > >>
> > > >> OK, good catch!
> > > >>
> > > >> [cut]
> > > >>
> > > >> >
> > > >> > Cc: stable@vger.kernel.org
> > > >> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > >> > Cc: Len Brown <lenb@kernel.org>
> > > >> > Cc: Peter Zijlstra <peterz@infradead.org>
> > > >> > Cc: Tejun Heo <tj@kernel.org>
> > > >> > Cc: Ingo Molnar <mingo@kernel.org>
> > > >> > Fixes: c072530f391e ("ACPI / PM: Revork the handling of ACPI device wakeup notifications")
> > > >> > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > > >> > ---
> > > >> >  drivers/acpi/device_pm.c | 21 ++++++++++++---------
> > > >> >  1 file changed, 12 insertions(+), 9 deletions(-)
> > > >> >
> > > >> > diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> > > >> > index fbcc73f7a099..18af71057b44 100644
> > > >> > --- a/drivers/acpi/device_pm.c
> > > >> > +++ b/drivers/acpi/device_pm.c
> > > >> > @@ -387,6 +387,7 @@ EXPORT_SYMBOL(acpi_bus_power_manageable);
> > > >> >
> > > >> >  #ifdef CONFIG_PM
> > > >> >  static DEFINE_MUTEX(acpi_pm_notifier_lock);
> > > >> > +static DEFINE_MUTEX(acpi_pm_notifier_install_lock);
> > > >> >
> > > >> >  void acpi_pm_wakeup_event(struct device *dev)
> > > >> >  {
> > > >> > @@ -443,24 +444,25 @@ acpi_status acpi_add_pm_notifier(struct acpi_device *adev, struct device *dev,
> > > >> >         if (!dev && !func)
> > > >> >                 return AE_BAD_PARAMETER;
> > > >> >
> > > >> > -       mutex_lock(&acpi_pm_notifier_lock);
> > > >> > +       mutex_lock(&acpi_pm_notifier_install_lock);
> > > >> >
> > > >> >         if (adev->wakeup.flags.notifier_present)
> > > >> >                 goto out;
> > > >> >
> > > >> > -       adev->wakeup.ws = wakeup_source_register(dev_name(&adev->dev));
> > > >> > -       adev->wakeup.context.dev = dev;
> > > >> > -       adev->wakeup.context.func = func;
> > > >> > -
> > > >>
> > > >> But this doesn't look good to me.
> > > >>
> > > >> notifier_present should be checked under acpi_pm_notifier_lock.
> > > >>
> > > >> Actually, acpi_install_notify_handler() itself need not be called
> > > >> under the lock, because it does sufficient checks of its own.
> > > >>
> > > >> So say you do
> > > >>
> > > >> mutex_lock(&acpi_pm_notifier_lock);
> > > >>
> > > >> if (adev->wakeup.context.dev)
> > > >>         goto out;
> > > >>
> > > >> adev->wakeup.ws = wakeup_source_register(dev_name(&adev->dev));
> > > >> adev->wakeup.context.dev = dev;
> > > >> adev->wakeup.context.func = func;
> > > >>
> > > >> mutex_unlock(&acpi_pm_notifier_lock);
> > > >>
> > > >> >         status = acpi_install_notify_handler(adev->handle, ACPI_SYSTEM_NOTIFY,
> > > >> >                                              acpi_pm_notify_handler, NULL);
> > > >> >         if (ACPI_FAILURE(status))
> > > >> >                 goto out;
> > > >> >
> > > >> > +       mutex_lock(&acpi_pm_notifier_lock);
> > > >>
> > > >> And here you just set notifier_present under acpi_pm_notifier_lock.
> > > >>
> > > >> > +       adev->wakeup.ws = wakeup_source_register(dev_name(&adev->dev));
> > > >> > +       adev->wakeup.context.dev = dev;
> > > >> > +       adev->wakeup.context.func = func;
> > > >> >         adev->wakeup.flags.notifier_present = true;
> > > >> > +       mutex_unlock(&acpi_pm_notifier_lock);
> > > >> >
> > > >> >   out:
> > > >> > -       mutex_unlock(&acpi_pm_notifier_lock);
> > > >> > +       mutex_unlock(&acpi_pm_notifier_install_lock);
> > > >> >         return status;
> > > >> >  }
> > > >>
> > > >> Then on removal you can clear notifier_present first and drop the lock
> > > >> around the acpi_remove_notify_handler() call and nothing bad will
> > > >> happen.
> > > >>
> > > >> If you call acpi_add_pm_notifier() twice in parallel, the first
> > > >> instance will set context.dev and the second one will see it set and
> > > >> bail out.  The first instance will then do the rest.
> > > >>
> > > >> If you call acpi_remove_pm_notifier() twice in a row, the first
> > > >> instance will see notifier_present set and will clear it, so the
> > > >> second one will see notifier_present unset and it will bail out.
> > > >>
> > > >> Now, if you call acpi_remove_pm_notifier() in parallel with
> > > >> acpi_add_pm_notifier(), either the former will see notifier_present
> > > >> unset and bail out, or the latter will see context.dev unset and bail
> > > >> out.
> > > >>
> > > >> It doesn't look like the outer lock is needed, or have I missed anything?
> > > >
> > > > So something like the below (totally untested) should work too, shouldn't it?
> > > 
> > > There is a problem if a device is removed while acpi_add_pm_notifier()
> > > is still in progress, in which case with my patch the
> > > acpi_remove_pm_notifier() called from the removal path may bail out
> > > prematurely (that doesn't seem likely to happen, but still I don't see
> > > why it cannot happen), so I'll just use your patch. :-)
> > 
> > OK. I was just looking at your version and was pretty much convinced
> > that it would work. But I'll take your word that it might not :)
> 
> Well, you don't have to. :-)
> 
> The scenario I have in mind is as follows:
> 
> 1. acpi_add_pm_notifier() sets context.dev and context.func and drops the
>    lock.  notifier_present is still unset.
> 
> 2. acpi_remove_pm_notifier() checks notifier_present under the lock.
>    It is (still) unset, so the function decides that there's nothing to do.
> 
> 3. acpi_add_pm_notifier() continues with notifier installation and the
>    device goes away at the same time.

Right. That makes sense. Though I don't know what would prevent racing
add_pm_notifier() against remove_pm_notifier() in a similar way even
with the outer lock. In that case the remove_pm_notifier() would just
have to get at the mutex first and then we'd still end up with the
notifier installed.

-- 
Ville Syrj�l�
Intel OTC