From patchwork Mon Aug 28 13:40:48 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Rafael J. Wysocki" X-Patchwork-Id: 9925543 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 E6DFE60329 for ; Mon, 28 Aug 2017 13:50:01 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id D53FF28573 for ; Mon, 28 Aug 2017 13:50:01 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id C23FF286E0; Mon, 28 Aug 2017 13:50:01 +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=-2.6 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,RCVD_IN_DNSWL_LOW autolearn=unavailable version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [65.50.211.133]) (using TLSv1.2 with cipher AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 94CBF286EB for ; Mon, 28 Aug 2017 13:50:00 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-ID:Date:Subject:To:From:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=OpEC6EqgpwEe5CE90kJjiw/XzDyIF9fyV00RGCL5ZbA=; b=d6u6lXF+ngX+b9 oSRtKpVAxvomtiLEI+dK3O303AC3Ubnoz7CBHuXW7HRKW7GI5btmeLNEhIiR2uSF2AKFz+gVCM2gS iefTLZVmnpGkuDpcRnG6T+VjJ/HcBN42alHEYfpKsTJT5n/3DdnlbVZNh10tQ+tqMwHQKSklypaza P4xvcqCn0+/FgArPjkxFoZt3XNBJKy8f4nodJHHNAeLxchw387lB76Ce59aB1Cekmlz3lF73oQ9vi cCMzCmida3mwXU6SpumhbEyKIT01q23RHlIBxkKmIWwQmiVgC+REomkMQm5faPOC1BPFhZdN0UFY0 EvT1dtL0XKAru+6IpeqA==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux)) id 1dmKQN-0004yX-7N; Mon, 28 Aug 2017 13:49:55 +0000 Received: from cloudserver094114.home.net.pl ([79.96.170.134]) by bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux)) id 1dmKQI-0004xL-JA for linux-arm-kernel@lists.infradead.org; Mon, 28 Aug 2017 13:49:53 +0000 Received: from 79.184.253.199.ipv4.supernova.orange.pl (79.184.253.199) (HELO aspire.rjw.lan) by serwer1319399.home.pl (79.96.170.134) with SMTP (IdeaSmtpServer 0.82) id 3749177ccdc1dde5; Mon, 28 Aug 2017 15:49:25 +0200 From: "Rafael J. Wysocki" To: Ulf Hansson Subject: Re: [PATCH v2 5/9] PM / ACPI: Provide option to disable direct_complete for ACPI devices Date: Mon, 28 Aug 2017 15:40:48 +0200 Message-ID: <1644235.K1erQvOgW9@aspire.rjw.lan> In-Reply-To: References: <1503499329-28834-1-git-send-email-ulf.hansson@linaro.org> <26411805.tiHaCQcaUh@aspire.rjw.lan> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20170828_064950_819826_4D26A589 X-CRM114-Status: GOOD ( 35.14 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Jisheng Zhang , Guodong Xu , Linux PM , Mika Westerberg , Wolfram Sang , linux-i2c , ACPI Devel Maling List , Jarkko Nikula , Haojian Zhuang , "Rafael J. Wysocki" , John Stultz , Andy Shevchenko , Kevin Hilman , Sumit Semwal , "linux-arm-kernel@lists.infradead.org" , Len Brown Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP On Monday, August 28, 2017 2:54:40 PM CEST Ulf Hansson wrote: > On 28 August 2017 at 14:39, Rafael J. Wysocki wrote: > > On Monday, August 28, 2017 10:31:44 AM CEST Ulf Hansson wrote: > >> On 28 August 2017 at 03:30, Rafael J. Wysocki wrote: > >> > On Friday, August 25, 2017 3:42:35 PM CEST Rafael J. Wysocki wrote: > >> >> On Thursday, August 24, 2017 11:50:40 PM CEST Rafael J. Wysocki wrote: > >> >> > On Thursday, August 24, 2017 6:35:49 PM CEST Rafael J. Wysocki wrote: > >> >> > > On Thursday, August 24, 2017 11:15:26 AM CEST Ulf Hansson wrote: [cut] > >> > > >> > Well, not really, because if the device remains runtime suspended, > >> > ->runtime_suspend() will not be called by pm_runtime_force_suspend() and > >> > acpi_dev_suspend_late() should not be called then. > >> > > >> > So more changes in the ACPI PM domain are needed after all. > >> > >> Yes, that's what I thought as well. > >> > >> Anyway, let me cook a new version of the series - trying to address > >> the first bits you have pointed out. Then we can continue with > >> fine-tuning on top, addressing further optimizations of the ACPI PM > >> domain. > > > > Actually, please hold on and let me show you what I would like to do > > first. > > Hmm. > > I think I have almost done the work for the ACPI PM domain already. > It's just a matter of minor tweaks to the changes in patch 6 and 7 > (and of course to get them into a shape that you prefer) and then > dropping patch 5 altogether. > > Wouldn't it be better if you build upon my changes? > > Anyway, if you have strong opinion of driving this, I am fine stepping aside. OK, so see below. :-) If what you want is to make the i2c designware driver use _force_suspend() and _force_resume(), then to me this is only tangentially related to direct_complete and can be done without messing up with that one. So the problem is not when direct_complete is set (becasue the driver's and PM domain's callbacks will not be invoked then even), but when it is not set. If direct_complete is not set, the ACPI PM domain resumes the device in acpi_subsys_suspend(), because it doesn't know two things: (a) why direct_complete is not set and (b) whether or not the drivers PM callbacks can cope with a runtime suspended device. These two things are separate, so they need to be addressed separately. For (b) I'd like to use the SAFE_SUSPEND flag from my previous patch. As far as (a) is concerned, there are two possible reasons for not setting direct_complete. First, it may be necessary to change the power state of the device and in that case the device *should* be resumed in acpi_subsys_suspend(). Second, direct_complete may not be set for the device's children and in that case acpi_subsys_suspend() may not care as long as SAFE_SUSPEND is set. So the ACPI PM domain needs to distinguish these two cases and for that reason it has to track whether or not the power state of the device needs to be updated which is what the (untested) patch below does, but of course it doesn't cover the LPSS. --- Documentation/driver-api/pm/devices.rst | 7 ++ drivers/acpi/device_pm.c | 72 +++++++++++++++++++++------- drivers/base/dd.c | 2 drivers/i2c/busses/i2c-designware-platdrv.c | 4 + include/acpi/acpi_bus.h | 1 include/linux/pm.h | 16 ++++++ 6 files changed, 83 insertions(+), 19 deletions(-) Index: linux-pm/include/linux/pm.h =================================================================== --- linux-pm.orig/include/linux/pm.h +++ linux-pm/include/linux/pm.h @@ -550,6 +550,21 @@ struct pm_subsys_data { #endif }; +/* + * Driver flags to control system suspend/resume behavior. + * + * These flags can be set by device drivers at the probe time. They need not be + * cleared by the drivers as the driver core will take care of that. + * + * SAFE_SUSPEND: No need to runtime resume the device during system suspend. + * + * Setting SAFE_SUSPEND instructs bus types and PM domains which may want to + * runtime resume the device upfront during system suspend that doing so is not + * necessary from the driver's perspective, because the system suspend callbacks + * provided by it can cope with a runtime suspended device. + */ +#define DPM_FLAG_SAFE_SUSPEND BIT(0) + struct dev_pm_info { pm_message_t power_state; unsigned int can_wakeup:1; @@ -561,6 +576,7 @@ struct dev_pm_info { bool is_late_suspended:1; bool early_init:1; /* Owned by the PM core */ bool direct_complete:1; /* Owned by the PM core */ + unsigned int driver_flags; spinlock_t lock; #ifdef CONFIG_PM_SLEEP struct list_head entry; Index: linux-pm/drivers/acpi/device_pm.c =================================================================== --- linux-pm.orig/drivers/acpi/device_pm.c +++ linux-pm/drivers/acpi/device_pm.c @@ -899,6 +899,7 @@ int acpi_dev_runtime_resume(struct devic error = acpi_dev_pm_full_power(adev); acpi_device_wakeup_disable(adev); + adev->power.update_state = true; return error; } EXPORT_SYMBOL_GPL(acpi_dev_runtime_resume); @@ -989,33 +990,47 @@ int acpi_dev_resume_early(struct device } EXPORT_SYMBOL_GPL(acpi_dev_resume_early); -/** - * acpi_subsys_prepare - Prepare device for system transition to a sleep state. - * @dev: Device to prepare. - */ -int acpi_subsys_prepare(struct device *dev) +static bool acpi_dev_state_update_needed(struct device *dev) { struct acpi_device *adev = ACPI_COMPANION(dev); u32 sys_target; int ret, state; - ret = pm_generic_prepare(dev); - if (ret < 0) - return ret; + if (!adev || !pm_runtime_suspended(dev)) + return true; - if (!adev || !pm_runtime_suspended(dev) - || device_may_wakeup(dev) != !!adev->wakeup.prepare_count) - return 0; + if (device_may_wakeup(dev) != !!adev->wakeup.prepare_count) + return true; sys_target = acpi_target_system_state(); if (sys_target == ACPI_STATE_S0) - return 1; + return false; if (adev->power.flags.dsw_present) - return 0; + return true; ret = acpi_dev_pm_get_state(dev, adev, sys_target, NULL, &state); - return !ret && state == adev->power.state; + if (ret) + return true; + + return state != adev->power.state; +} + +/** + * acpi_subsys_prepare - Prepare device for system transition to a sleep state. + * @dev: Device to prepare. + */ +int acpi_subsys_prepare(struct device *dev) +{ + struct acpi_device *adev = ACPI_COMPANION(dev); + int ret; + + ret = pm_generic_prepare(dev); + if (ret < 0) + return ret; + + adev->power.update_state = acpi_dev_state_update_needed(dev); + return !adev->power.update_state; } EXPORT_SYMBOL_GPL(acpi_subsys_prepare); @@ -1024,11 +1039,20 @@ EXPORT_SYMBOL_GPL(acpi_subsys_prepare); * @dev: Device to handle. * * Follow PCI and resume devices suspended at run time before running their - * system suspend callbacks. + * system suspend callbacks, unless the DPM_FLAG_SAFE_SUSPEND driver flag is + * set for them. */ int acpi_subsys_suspend(struct device *dev) { - pm_runtime_resume(dev); + struct acpi_device *adev = ACPI_COMPANION(dev); + + if (!adev) + return 0; + + if (adev->power.update_state || + !(dev->power.driver_flags & DPM_FLAG_SAFE_SUSPEND)) + pm_runtime_resume(dev); + return pm_generic_suspend(dev); } EXPORT_SYMBOL_GPL(acpi_subsys_suspend); @@ -1042,8 +1066,20 @@ EXPORT_SYMBOL_GPL(acpi_subsys_suspend); */ int acpi_subsys_suspend_late(struct device *dev) { - int ret = pm_generic_suspend_late(dev); - return ret ? ret : acpi_dev_suspend_late(dev); + struct acpi_device *adev = ACPI_COMPANION(dev); + int ret; + + if (!adev) + return 0; + + ret = pm_generic_suspend_late(dev); + if (ret) + return ret; + + if (adev->power.update_state) + return acpi_dev_suspend_late(dev); + + return 0; } EXPORT_SYMBOL_GPL(acpi_subsys_suspend_late); Index: linux-pm/drivers/base/dd.c =================================================================== --- linux-pm.orig/drivers/base/dd.c +++ linux-pm/drivers/base/dd.c @@ -436,6 +436,7 @@ pinctrl_bind_failed: if (dev->pm_domain && dev->pm_domain->dismiss) dev->pm_domain->dismiss(dev); pm_runtime_reinit(dev); + dev->power.driver_flags = 0; switch (ret) { case -EPROBE_DEFER: @@ -841,6 +842,7 @@ static void __device_release_driver(stru if (dev->pm_domain && dev->pm_domain->dismiss) dev->pm_domain->dismiss(dev); pm_runtime_reinit(dev); + dev->power.driver_flags = 0; klist_remove(&dev->p->knode_driver); device_pm_check_callbacks(dev); Index: linux-pm/Documentation/driver-api/pm/devices.rst =================================================================== --- linux-pm.orig/Documentation/driver-api/pm/devices.rst +++ linux-pm/Documentation/driver-api/pm/devices.rst @@ -729,6 +729,13 @@ state temporarily, for example so that i disabled. This all depends on the hardware and the design of the subsystem and device driver in question. +Some bus types and PM domains have a policy to runtime resume all +devices upfront in their ``->suspend`` callbacks, but that may not be really +necessary if the system suspend-resume callbacks provided by the device's +driver can cope with a runtime-suspended device. The driver can indicate that +by setting ``DPM_FLAG_SAFE_SUSPEND`` in :c:member:`power.driver_flags` at the +probe time. + During system-wide resume from a sleep state it's easiest to put devices into the full-power state, as explained in :file:`Documentation/power/runtime_pm.txt`. Refer to that document for more information regarding this particular issue as Index: linux-pm/include/acpi/acpi_bus.h =================================================================== --- linux-pm.orig/include/acpi/acpi_bus.h +++ linux-pm/include/acpi/acpi_bus.h @@ -287,6 +287,7 @@ struct acpi_device_power { int state; /* Current state */ struct acpi_device_power_flags flags; struct acpi_device_power_state states[ACPI_D_STATE_COUNT]; /* Power states (D0-D3Cold) */ + bool update_state; }; /* Performance Management */ Index: linux-pm/drivers/i2c/busses/i2c-designware-platdrv.c =================================================================== --- linux-pm.orig/drivers/i2c/busses/i2c-designware-platdrv.c +++ linux-pm/drivers/i2c/busses/i2c-designware-platdrv.c @@ -358,6 +358,7 @@ static int dw_i2c_plat_probe(struct plat if (dev->pm_disabled) { pm_runtime_forbid(&pdev->dev); } else { + dev->power.driver_flags = DPM_FLAG_SAFE_SUSPEND; pm_runtime_set_autosuspend_delay(&pdev->dev, 1000); pm_runtime_use_autosuspend(&pdev->dev); pm_runtime_set_active(&pdev->dev); @@ -455,7 +456,8 @@ static int dw_i2c_plat_resume(struct dev static const struct dev_pm_ops dw_i2c_dev_pm_ops = { .prepare = dw_i2c_plat_prepare, .complete = dw_i2c_plat_complete, - SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume) + SET_LATE_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, + pm_runtime_force_resume) SET_RUNTIME_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume, NULL) };