From patchwork Tue Nov 7 23:06:35 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: "Rafael J. Wysocki" X-Patchwork-Id: 10047367 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 8DDD660360 for ; Tue, 7 Nov 2017 23:13:24 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 8127A29A93 for ; Tue, 7 Nov 2017 23:13:24 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 756ED29ADD; Tue, 7 Nov 2017 23:13:24 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id D4F7729A93 for ; Tue, 7 Nov 2017 23:13:23 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 924EB6E0B6; Tue, 7 Nov 2017 23:13:22 +0000 (UTC) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org X-Greylist: delayed 401 seconds by postgrey-1.35 at gabe; Tue, 07 Nov 2017 23:13:21 UTC Received: from cloudserver094114.home.net.pl (cloudserver094114.home.net.pl [79.96.170.134]) by gabe.freedesktop.org (Postfix) with ESMTPS id 434C36E0B6 for ; Tue, 7 Nov 2017 23:13:21 +0000 (UTC) Received: from 79.184.254.73.ipv4.supernova.orange.pl (79.184.254.73) (HELO aspire.rjw.lan) by serwer1319399.home.pl (79.96.170.134) with SMTP (IdeaSmtpServer 0.82) id 74a7d13025091090; Wed, 8 Nov 2017 00:06:38 +0100 From: "Rafael J. Wysocki" To: Ville Syrjala Date: Wed, 08 Nov 2017 00:06:35 +0100 Message-ID: <1551349.SyN5ciAXNe@aspire.rjw.lan> In-Reply-To: References: <20171107210810.1300-1-ville.syrjala@linux.intel.com> MIME-Version: 1.0 Cc: "Rafael J. Wysocki" , Peter Zijlstra , intel-gfx , "Rafael J . Wysocki" , Stable , ACPI Devel Maling List , Tejun Heo , Ingo Molnar , Len Brown Subject: Re: [Intel-gfx] [PATCH] ACPI / PM: Fix acpi_pm_notifier_lock vs. flush_workqueue() deadlock X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" X-Virus-Scanned: ClamAV using ClamSMTP 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 > wrote: > > From: Ville Syrjälä > > > > 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 > > Cc: Len Brown > > Cc: Peter Zijlstra > > Cc: Tejun Heo > > Cc: Ingo Molnar > > Fixes: c072530f391e ("ACPI / PM: Revork the handling of ACPI device wakeup notifications") > > Signed-off-by: Ville Syrjälä > > --- > > 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);