[{"id":1759448,"web_url":"http://patchwork.ozlabs.org/comment/1759448/","msgid":"<13933269.sbLfAJfmVU@aspire.rjw.lan>","list_archive_url":null,"date":"2017-08-29T15:15:02","subject":"Re: [PATCH v3 1/8] PM / Sleep: Make the runtime PM centric path\n\tknown to the PM core","submitter":{"id":26536,"url":"http://patchwork.ozlabs.org/api/people/26536/","name":"Rafael J. Wysocki","email":"rjw@rjwysocki.net"},"content":"On Tuesday, August 29, 2017 4:56:43 PM CEST Ulf Hansson wrote:\n> The main principle behind the runtime PM centric path, is to re-use the\n> runtime PM callbacks to implement system sleep - and while doing that also\n> achieve a fully optimized behaviour from PM point of view.\n> \n> More precisely, avoid to wake up a device from its low power state during\n> system sleep, unless the device is really needed to be operational. That\n> does not only mean avoiding to waste power, but may also decrease system\n> suspend/resume time for a device.\n> \n> However, using the runtime PM centric path for a device, does put some\n> requirements on the behaviour of the PM core and a potential PM domain that\n> may be attached to the device. So far, these requirements are not being\n> specially considered, except by the generic PM domain.\n> \n> To move forward and to make it possible to deploy the runtime PM centric\n> path for cross SoC drivers, which may have different PM domains attached to\n> its devices depending on the SoC, we must address how to deal with these\n> requirements. This change starts by making some adoptions to the PM core,\n> while other parts, such as the ACPI PM domain needs to be taken care of\n> separately.\n> \n> In the runtime PM centric path, the driver is expected to make use of the\n> pm_runtime_force_suspend|resume() helpers, to deploy system sleep support.\n> More precisely it may assign the system sleep callbacks to these helpers or\n> may call them from its own callbacks, in case it needs to perform\n> additional actions during system sleep.\n> \n> In other words, the PM core must always invoke the system sleep callbacks\n> for the device when they are present, to allow the driver to deal with\n> the system sleep operations.\n> \n> In case the PM core decides to run the direct_complete path for the device,\n> it skips invoking most of the system sleep callbacks, besides\n> ->prepare|complete(). Therefore using the direct_complete path in\n> combination with the runtime PM centric patch for a device, does not play\n> well.\n> \n> To deal with this issue, let's add a flag 'is_rpm_sleep', to the struct\n> dev_pm_info. The driver that deploys the runtime PM centric path, shall set\n> the flag for the device during ->probe(), to inform the PM core about that\n> it must not use the direct_complete path for the device.\n> \n> Note, not allowing the direct_complete path for a device, doesn't implicit\n> need to propagate to the device's parent/suppliers. Therefore make the PM\n> core check this condition in device_suspend(), before it decides to abandon\n> the direct_complete path for parent/suppliers.\n> \n> To make the is_rpm_sleep flag internal to the PM core, let's add two APIs.\n> \t- dev_pm_use_rpm_sleep(): It sets the flag and should be called by\n> \t the driver during ->probe().\n> \t- dev_pm_is_rpm_sleep(): Makes it possible for users of the device,\n> \t like a PM domain, to fetch the state of the flag.\n> \n> Signed-off-by: Ulf Hansson \n> ---\n> \n> Changes in v3:\n> \t- New patch.\n> \t- This replaces the earlier method of adding the no_direct_complete to\n> \tthe ACPI structures, according to comments from Rafael.\n> \t- This change also address the consern Rafael had around that\n> \tdirect_complete should not have to be disabled for parent/suppliers, in\n> \tcase a device use the runtime PM centric path for system sleep.\n> \n> ---\n> drivers/base/power/main.c | 49 +++++++++++++++++++++++++++++++++++++++-----\n> drivers/base/power/runtime.c | 1 +\n> include/linux/pm.h | 7 +++++++\n> 3 files changed, 52 insertions(+), 5 deletions(-)\n> \n> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c\n> index ea1732e..865737a 100644\n> --- a/drivers/base/power/main.c\n> +++ b/drivers/base/power/main.c\n> @@ -1549,14 +1549,16 @@ static int __device_suspend(struct device *dev, pm_message_t state, bool async)\n> \t\tif (parent) {\n> \t\t\tspin_lock_irq(&parent->power.lock);\n> \n> -\t\t\tdev->parent->power.direct_complete = false;\n> +\t\t\tif (!dev->power.is_rpm_sleep)\n> +\t\t\t\tdev->parent->power.direct_complete = false;\n\nThis doesn't look good to me at all.\n\nIt potentially breaks the fundamental assumption of the direct_complete\nmechanism that no devices with that flag set will ever be runtime resumed\nduring system suspend.\n\nWhich can happen with this change if a device with power.is_rpm_sleep is\nresumed causing the parent to be resumed too.\n\n> \t\t\tif (dev->power.wakeup_path\n> \t\t\t && !dev->parent->power.ignore_children)\n> \t\t\t\tdev->parent->power.wakeup_path = true;\n> \n> \t\t\tspin_unlock_irq(&parent->power.lock);\n> \t\t}\n> -\t\tdpm_clear_suppliers_direct_complete(dev);\n> +\t\tif (!dev->power.is_rpm_sleep)\n> +\t\t\tdpm_clear_suppliers_direct_complete(dev);\n\nAnd same here.\n\n> \t}\n> \n> \tdevice_unlock(dev);\n> @@ -1709,11 +1711,14 @@ static int device_prepare(struct device *dev, pm_message_t state)\n> \t * A positive return value from ->prepare() means \"this device appears\n> \t * to be runtime-suspended and its state is fine, so if it really is\n> \t * runtime-suspended, you can leave it in that state provided that you\n> -\t * will do the same thing with all of its descendants\". This only\n> -\t * applies to suspend transitions, however.\n> +\t * will do the same thing with all of its descendants\". To allow this,\n> +\t * the is_rpm_sleep flag must not be set, which is default false, but\n> +\t * may have been changed by a driver. This only applies to suspend\n> +\t * transitions, however.\n> \t */\n> \tspin_lock_irq(&dev->power.lock);\n> -\tdev->power.direct_complete = ret > 0 && state.event == PM_EVENT_SUSPEND;\n> +\tdev->power.direct_complete = ret > 0 && !dev->power.is_rpm_sleep &&\n> +\t\t\t\tstate.event == PM_EVENT_SUSPEND;\n\nThe only problem addressed by avoiding direct_complete is when runtime PM needs\nto work during __device_suspend() for devices with direct_complete set. You\nsaid that this was not the case with the i2c designware driver, so I'm not sure\nwhat the purpose of this is.\n\nIt doesn't solve any other problems at all.\n\n> \tspin_unlock_irq(&dev->power.lock);\n> \treturn 0;\n> }\n> @@ -1841,6 +1846,40 @@ void dpm_for_each_dev(void *data, void (*fn)(struct device *, void *))\n> }\n> EXPORT_SYMBOL_GPL(dpm_for_each_dev);\n> \n> +/**\n> + * dev_pm_use_rpm_sleep - use the runtime PM centric sleep path.\n> + * @dev: the device to use the path for.\n> + *\n> + * The runtime PM centric path requires the system sleep callbacks for the\n> + * driver of the device to be invoked. This function sets the is_rpm_sleep flag\n> + * to enable this path to be used, which makes the PM core to adopt to this\n> + * behaviour. More precisely the PM core makes sure to not run the\n> + * direct_complete path for the chosen device during system sleep, when the\n> + * is_rpm_sleep flag is set. A driver using the runtime PM centric path, shall\n> + * use the pm_runtime_force_suspend|resume() helpers, to deploy system sleep\n> + * support and call this function during ->probe().\n> + * The is_rpm_sleep flag is cleared when unbinding/bind-failure of the driver.\n> + */\n> +void dev_pm_use_rpm_sleep(struct device *dev)\n> +{\n> +\tdev->power.is_rpm_sleep = true;\n> +}\n> +EXPORT_SYMBOL_GPL(dev_pm_use_rpm_sleep);\n> +\n> +/**\n> + * dev_pm_is_rpm_sleep - Returns the value of the is_rpm_sleep flag.\n> + * @dev: the device to return the flag for.\n> + *\n> + * The caller of this function is typically a subsystem or a PM domain that\n> + * needs to know if the runtime PM centric path is used for the device. It may\n> + * be invoked during any of the system sleep phases.\n> + */\n> +bool dev_pm_is_rpm_sleep(struct device *dev)\n> +{\n> +\treturn dev->power.is_rpm_sleep;\n> +}\n> +EXPORT_SYMBOL_GPL(dev_pm_is_rpm_sleep);\n> +\n> static bool pm_ops_is_empty(const struct dev_pm_ops *ops)\n> {\n> \tif (!ops)\n> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c\n> index 7bcf80f..b2ab22c 100644\n> --- a/drivers/base/power/runtime.c\n> +++ b/drivers/base/power/runtime.c\n> @@ -1522,6 +1522,7 @@ void pm_runtime_reinit(struct device *dev)\n> \t\t\t\tpm_runtime_put(dev->parent);\n> \t\t}\n> \t}\n> +\tdev->power.is_rpm_sleep = false;\n> }\n> \n> /**\n> diff --git a/include/linux/pm.h b/include/linux/pm.h\n> index 47ded8a..5bf96d2 100644\n> --- a/include/linux/pm.h\n> +++ b/include/linux/pm.h\n> @@ -559,6 +559,7 @@ struct dev_pm_info {\n> \tbool\t\t\tis_suspended:1;\t/* Ditto */\n> \tbool\t\t\tis_noirq_suspended:1;\n> \tbool\t\t\tis_late_suspended:1;\n> +\tbool\t\t\tis_rpm_sleep:1;\t/* Owned by the PM core */\n\nIt is expected to be set by drivers, so by definition it is *not* owned by the\nPM core.\n\n> \tbool\t\t\tearly_init:1;\t/* Owned by the PM core */\n> \tbool\t\t\tdirect_complete:1;\t/* Owned by the PM core */\n> \tspinlock_t\t\tlock;\n> @@ -716,6 +717,9 @@ extern void __suspend_report_result(const char *function, void *fn, int ret);\n> extern int device_pm_wait_for_dev(struct device *sub, struct device *dev);\n> extern void dpm_for_each_dev(void *data, void (*fn)(struct device *, void *));\n> \n> +extern void dev_pm_use_rpm_sleep(struct device *dev);\n> +extern bool dev_pm_is_rpm_sleep(struct device *dev);\n> +\n> extern int pm_generic_prepare(struct device *dev);\n> extern int pm_generic_suspend_late(struct device *dev);\n> extern int pm_generic_suspend_noirq(struct device *dev);\n> @@ -759,6 +763,9 @@ static inline void dpm_for_each_dev(void *data, void (*fn)(struct device *, void\n> {\n> }\n> \n> +static inline void dev_pm_use_rpm_sleep(struct device *dev) {}\n> +static inline bool dev_pm_is_rpm_sleep(struct device *dev) { return false; }\n> +\n> #define pm_generic_prepare\t\tNULL\n> #define pm_generic_suspend_late\t\tNULL\n> #define pm_generic_suspend_noirq\tNULL\n> \n\nThanks,\nRafael","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-i2c-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xhXTt3pcNz9t33\n\tfor ;\n\tWed, 30 Aug 2017 01:23:46 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752094AbdH2PXo (ORCPT );\n\tTue, 29 Aug 2017 11:23:44 -0400","from cloudserver094114.home.net.pl ([79.96.170.134]:58590 \"EHLO\n\tcloudserver094114.home.net.pl\" rhost-flags-OK-OK-OK-OK)\n\tby vger.kernel.org with ESMTP id S1751514AbdH2PXn (ORCPT\n\t); Tue, 29 Aug 2017 11:23:43 -0400","from 79.184.253.199.ipv4.supernova.orange.pl (79.184.253.199)\n\t(HELO aspire.rjw.lan)\n\tby serwer1319399.home.pl (79.96.170.134) with SMTP (IdeaSmtpServer\n\t0.82) id e80b735aa4954770; Tue, 29 Aug 2017 17:23:41 +0200"],"From":"\"Rafael J. Wysocki\" ","To":"Ulf Hansson ","Cc":"Wolfram Sang , Len Brown ,\n\tlinux-acpi@vger.kernel.org, linux-pm@vger.kernel.org,\n\tKevin Hilman ,\n\tJarkko Nikula ,\n\tAndy Shevchenko ,\n\tMika Westerberg ,\n\tJisheng Zhang ,\n\tJohn Stultz , Guodong Xu ,\n\tSumit Semwal ,\n\tHaojian Zhuang ,\n\tJohannes Stezenbach ,\n\tlinux-arm-kernel@lists.infradead.org, linux-i2c@vger.kernel.org","Subject":"Re: [PATCH v3 1/8] PM / Sleep: Make the runtime PM centric path\n\tknown to the PM core","Date":"Tue, 29 Aug 2017 17:15:02 +0200","Message-ID":"<13933269.sbLfAJfmVU@aspire.rjw.lan>","In-Reply-To":"<1504018610-10822-2-git-send-email-ulf.hansson@linaro.org>","References":"<1504018610-10822-1-git-send-email-ulf.hansson@linaro.org>\n\t<1504018610-10822-2-git-send-email-ulf.hansson@linaro.org>","MIME-Version":"1.0","Content-Transfer-Encoding":"7Bit","Content-Type":"text/plain; charset=\"us-ascii\"","Sender":"linux-i2c-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-i2c@vger.kernel.org"}},{"id":1759923,"web_url":"http://patchwork.ozlabs.org/comment/1759923/","msgid":"","list_archive_url":null,"date":"2017-08-30T07:13:47","subject":"Re: [PATCH v3 1/8] PM / Sleep: Make the runtime PM centric path\n\tknown to the PM core","submitter":{"id":21036,"url":"http://patchwork.ozlabs.org/api/people/21036/","name":"Ulf Hansson","email":"ulf.hansson@linaro.org"},"content":"On 29 August 2017 at 17:15, Rafael J. Wysocki wrote:\n> On Tuesday, August 29, 2017 4:56:43 PM CEST Ulf Hansson wrote:\n>> The main principle behind the runtime PM centric path, is to re-use the\n>> runtime PM callbacks to implement system sleep - and while doing that also\n>> achieve a fully optimized behaviour from PM point of view.\n>>\n>> More precisely, avoid to wake up a device from its low power state during\n>> system sleep, unless the device is really needed to be operational. That\n>> does not only mean avoiding to waste power, but may also decrease system\n>> suspend/resume time for a device.\n>>\n>> However, using the runtime PM centric path for a device, does put some\n>> requirements on the behaviour of the PM core and a potential PM domain that\n>> may be attached to the device. So far, these requirements are not being\n>> specially considered, except by the generic PM domain.\n>>\n>> To move forward and to make it possible to deploy the runtime PM centric\n>> path for cross SoC drivers, which may have different PM domains attached to\n>> its devices depending on the SoC, we must address how to deal with these\n>> requirements. This change starts by making some adoptions to the PM core,\n>> while other parts, such as the ACPI PM domain needs to be taken care of\n>> separately.\n>>\n>> In the runtime PM centric path, the driver is expected to make use of the\n>> pm_runtime_force_suspend|resume() helpers, to deploy system sleep support.\n>> More precisely it may assign the system sleep callbacks to these helpers or\n>> may call them from its own callbacks, in case it needs to perform\n>> additional actions during system sleep.\n>>\n>> In other words, the PM core must always invoke the system sleep callbacks\n>> for the device when they are present, to allow the driver to deal with\n>> the system sleep operations.\n>>\n>> In case the PM core decides to run the direct_complete path for the device,\n>> it skips invoking most of the system sleep callbacks, besides\n>> ->prepare|complete(). Therefore using the direct_complete path in\n>> combination with the runtime PM centric patch for a device, does not play\n>> well.\n>>\n>> To deal with this issue, let's add a flag 'is_rpm_sleep', to the struct\n>> dev_pm_info. The driver that deploys the runtime PM centric path, shall set\n>> the flag for the device during ->probe(), to inform the PM core about that\n>> it must not use the direct_complete path for the device.\n>>\n>> Note, not allowing the direct_complete path for a device, doesn't implicit\n>> need to propagate to the device's parent/suppliers. Therefore make the PM\n>> core check this condition in device_suspend(), before it decides to abandon\n>> the direct_complete path for parent/suppliers.\n>>\n>> To make the is_rpm_sleep flag internal to the PM core, let's add two APIs.\n>> - dev_pm_use_rpm_sleep(): It sets the flag and should be called by\n>> the driver during ->probe().\n>> - dev_pm_is_rpm_sleep(): Makes it possible for users of the device,\n>> like a PM domain, to fetch the state of the flag.\n>>\n>> Signed-off-by: Ulf Hansson \n>> ---\n>>\n>> Changes in v3:\n>> - New patch.\n>> - This replaces the earlier method of adding the no_direct_complete to\n>> the ACPI structures, according to comments from Rafael.\n>> - This change also address the consern Rafael had around that\n>> direct_complete should not have to be disabled for parent/suppliers, in\n>> case a device use the runtime PM centric path for system sleep.\n>>\n>> ---\n>> drivers/base/power/main.c | 49 +++++++++++++++++++++++++++++++++++++++-----\n>> drivers/base/power/runtime.c | 1 +\n>> include/linux/pm.h | 7 +++++++\n>> 3 files changed, 52 insertions(+), 5 deletions(-)\n>>\n>> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c\n>> index ea1732e..865737a 100644\n>> --- a/drivers/base/power/main.c\n>> +++ b/drivers/base/power/main.c\n>> @@ -1549,14 +1549,16 @@ static int __device_suspend(struct device *dev, pm_message_t state, bool async)\n>> if (parent) {\n>> spin_lock_irq(&parent->power.lock);\n>>\n>> - dev->parent->power.direct_complete = false;\n>> + if (!dev->power.is_rpm_sleep)\n>> + dev->parent->power.direct_complete = false;\n>\n> This doesn't look good to me at all.\n>\n> It potentially breaks the fundamental assumption of the direct_complete\n> mechanism that no devices with that flag set will ever be runtime resumed\n> during system suspend.\n>\n> Which can happen with this change if a device with power.is_rpm_sleep is\n> resumed causing the parent to be resumed too.\n\nDoesn't the exact same problem you describe, already exist prior my change!?\n\nThat is, if the current device is runtime suspended and the\ndirect_complete flag is set for it, then __device_suspend() has\nalready disabled runtime PM for the device, when we reach this point.\nThat means, a following attempts to runtime resume the device will\nfail and thus also to runtime resume its parent.\n\nSo to me, this is a different problem, related how to work out the\ncorrect order of how to suspend devices.\n\n>\n>> if (dev->power.wakeup_path\n>> && !dev->parent->power.ignore_children)\n>> dev->parent->power.wakeup_path = true;\n>>\n>> spin_unlock_irq(&parent->power.lock);\n>> }\n>> - dpm_clear_suppliers_direct_complete(dev);\n>> + if (!dev->power.is_rpm_sleep)\n>> + dpm_clear_suppliers_direct_complete(dev);\n>\n> And same here.\n\nSee comment above.\n\n>\n>> }\n>>\n>> device_unlock(dev);\n>> @@ -1709,11 +1711,14 @@ static int device_prepare(struct device *dev, pm_message_t state)\n>> * A positive return value from ->prepare() means \"this device appears\n>> * to be runtime-suspended and its state is fine, so if it really is\n>> * runtime-suspended, you can leave it in that state provided that you\n>> - * will do the same thing with all of its descendants\". This only\n>> - * applies to suspend transitions, however.\n>> + * will do the same thing with all of its descendants\". To allow this,\n>> + * the is_rpm_sleep flag must not be set, which is default false, but\n>> + * may have been changed by a driver. This only applies to suspend\n>> + * transitions, however.\n>> */\n>> spin_lock_irq(&dev->power.lock);\n>> - dev->power.direct_complete = ret > 0 && state.event == PM_EVENT_SUSPEND;\n>> + dev->power.direct_complete = ret > 0 && !dev->power.is_rpm_sleep &&\n>> + state.event == PM_EVENT_SUSPEND;\n>\n> The only problem addressed by avoiding direct_complete is when runtime PM needs\n> to work during __device_suspend() for devices with direct_complete set. You\n> said that this was not the case with the i2c designware driver, so I'm not sure\n> what the purpose of this is.\n\nI should probably work more on my changelog, because I tried to\nexplain it there.\n\nAnyway, let me quote the important parts, and this is not specific for\nthe i2c-designware-driver, but generic to drivers using\npm_runtime_force_suspend|resume().\n\n\"In the runtime PM centric path, the driver is expected to make use of\nthe pm_runtime_force_suspend|resume() helpers, to deploy system sleep\nsupport. More precisely it may assign the system sleep callbacks to\nthese helpers or may call them from its own callbacks, in case it\nneeds to perform additional actions during system sleep.\n\nIn other words, the PM core must always invoke the system sleep\ncallbacks for the device when they are present, to allow the driver to\ndeal with the system sleep operations.\"\n\nA concrete example is drivers/spi/spi-fsl-espi.c, but one can easily find more.\n\n>\n> It doesn't solve any other problems at all.\n>\n>> spin_unlock_irq(&dev->power.lock);\n>> return 0;\n>> }\n>> @@ -1841,6 +1846,40 @@ void dpm_for_each_dev(void *data, void (*fn)(struct device *, void *))\n>> }\n>> EXPORT_SYMBOL_GPL(dpm_for_each_dev);\n>>\n>> +/**\n>> + * dev_pm_use_rpm_sleep - use the runtime PM centric sleep path.\n>> + * @dev: the device to use the path for.\n>> + *\n>> + * The runtime PM centric path requires the system sleep callbacks for the\n>> + * driver of the device to be invoked. This function sets the is_rpm_sleep flag\n>> + * to enable this path to be used, which makes the PM core to adopt to this\n>> + * behaviour. More precisely the PM core makes sure to not run the\n>> + * direct_complete path for the chosen device during system sleep, when the\n>> + * is_rpm_sleep flag is set. A driver using the runtime PM centric path, shall\n>> + * use the pm_runtime_force_suspend|resume() helpers, to deploy system sleep\n>> + * support and call this function during ->probe().\n>> + * The is_rpm_sleep flag is cleared when unbinding/bind-failure of the driver.\n>> + */\n>> +void dev_pm_use_rpm_sleep(struct device *dev)\n>> +{\n>> + dev->power.is_rpm_sleep = true;\n>> +}\n>> +EXPORT_SYMBOL_GPL(dev_pm_use_rpm_sleep);\n>> +\n>> +/**\n>> + * dev_pm_is_rpm_sleep - Returns the value of the is_rpm_sleep flag.\n>> + * @dev: the device to return the flag for.\n>> + *\n>> + * The caller of this function is typically a subsystem or a PM domain that\n>> + * needs to know if the runtime PM centric path is used for the device. It may\n>> + * be invoked during any of the system sleep phases.\n>> + */\n>> +bool dev_pm_is_rpm_sleep(struct device *dev)\n>> +{\n>> + return dev->power.is_rpm_sleep;\n>> +}\n>> +EXPORT_SYMBOL_GPL(dev_pm_is_rpm_sleep);\n>> +\n>> static bool pm_ops_is_empty(const struct dev_pm_ops *ops)\n>> {\n>> if (!ops)\n>> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c\n>> index 7bcf80f..b2ab22c 100644\n>> --- a/drivers/base/power/runtime.c\n>> +++ b/drivers/base/power/runtime.c\n>> @@ -1522,6 +1522,7 @@ void pm_runtime_reinit(struct device *dev)\n>> pm_runtime_put(dev->parent);\n>> }\n>> }\n>> + dev->power.is_rpm_sleep = false;\n>> }\n>>\n>> /**\n>> diff --git a/include/linux/pm.h b/include/linux/pm.h\n>> index 47ded8a..5bf96d2 100644\n>> --- a/include/linux/pm.h\n>> +++ b/include/linux/pm.h\n>> @@ -559,6 +559,7 @@ struct dev_pm_info {\n>> bool is_suspended:1; /* Ditto */\n>> bool is_noirq_suspended:1;\n>> bool is_late_suspended:1;\n>> + bool is_rpm_sleep:1; /* Owned by the PM core */\n>\n> It is expected to be set by drivers, so by definition it is *not* owned by the\n> PM core.\n\nAhh, I see. I didn't quite get those comments, thanks for clarifying.\n\n>\n>> bool early_init:1; /* Owned by the PM core */\n>> bool direct_complete:1; /* Owned by the PM core */\n>> spinlock_t lock;\n>> @@ -716,6 +717,9 @@ extern void __suspend_report_result(const char *function, void *fn, int ret);\n>> extern int device_pm_wait_for_dev(struct device *sub, struct device *dev);\n>> extern void dpm_for_each_dev(void *data, void (*fn)(struct device *, void *));\n>>\n>> +extern void dev_pm_use_rpm_sleep(struct device *dev);\n>> +extern bool dev_pm_is_rpm_sleep(struct device *dev);\n>> +\n>> extern int pm_generic_prepare(struct device *dev);\n>> extern int pm_generic_suspend_late(struct device *dev);\n>> extern int pm_generic_suspend_noirq(struct device *dev);\n>> @@ -759,6 +763,9 @@ static inline void dpm_for_each_dev(void *data, void (*fn)(struct device *, void\n>> {\n>> }\n>>\n>> +static inline void dev_pm_use_rpm_sleep(struct device *dev) {}\n>> +static inline bool dev_pm_is_rpm_sleep(struct device *dev) { return false; }\n>> +\n>> #define pm_generic_prepare NULL\n>> #define pm_generic_suspend_late NULL\n>> #define pm_generic_suspend_noirq NULL\n>>\n>\n> Thanks,\n> Rafael\n>\n\nThanks for reviewing!\n\nKind regards\nUffe","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-i2c-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (1024-bit key;\n\tunprotected) header.d=linaro.org header.i=@linaro.org\n\theader.b=\"UxlvsSKz\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xhxZ94gtgz9ryT\n\tfor ;\n\tWed, 30 Aug 2017 17:13:53 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751039AbdH3HNu (ORCPT );\n\tWed, 30 Aug 2017 03:13:50 -0400","from mail-it0-f52.google.com ([209.85.214.52]:37329 \"EHLO\n\tmail-it0-f52.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751186AbdH3HNs (ORCPT\n\t); Wed, 30 Aug 2017 03:13:48 -0400","by mail-it0-f52.google.com with SMTP id p2so3720649ite.0\n\tfor ; Wed, 30 Aug 2017 00:13:48 -0700 (PDT)","by 10.2.96.38 with HTTP; Wed, 30 Aug 2017 00:13:47 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc; bh=qX4ZAWHD4v7HW+t81YBNqLYgG7aNDSfM+Rbnnt4gAD4=;\n\tb=UxlvsSKzd+M0SFGxgsrTW2mYh6lgBQBUMYW3ecTb/0jr34J486YP/7CVGPC1gk4olX\n\tfB6VGC8PD4b3ZxVyVarIMZrVkTbGQ5sdrU17w+Y7C6+91+sAR7lSNgT41DOVISbi3z7M\n\t+xcZjeTczYCY9Tf1E5d02rSo7aOXNU/u0UPqM=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc;\n\tbh=qX4ZAWHD4v7HW+t81YBNqLYgG7aNDSfM+Rbnnt4gAD4=;\n\tb=ex0EkIh6WFbof7IhGh9IsyWu32aaXN+GIqsZvgxKI124ePfzzDoxZCt+eJMkFzmtlt\n\tUAQrgaA7A6xx9Gs5eO6gLUh7V8xGAN8lQIyB525Tnn+E/fwkbZlACqJ6er0n+LkF1RCG\n\tQA+d1cuDVh9CqN3ZPqDGSAJP835knJ+6t8Lje4+7hq9Qos0FTIo6qwMnS3P8RqUhRp6q\n\t49CRpzvmIOgbau6tTSMBmOpdKE0M+smnqxIPv1B4s/RRnLvdejMxhzhE1b2mrmB99wrF\n\tIkwIbEqDtV8qEw2zcZnIhcfbnuD31TgsmflF6qf0luCT7qhCZIdQMPoRRIMcHswi+yWa\n\tavlA==","X-Gm-Message-State":"AHYfb5hcqMCidXvq+CdKd/xgzb8spLpDuLjpdEdldqmLCVc9D8/vp6mL\n\tHHybJhp367TdTjQ/r8Tt1daLyh5JGlRF","X-Received":"by 10.36.173.11 with SMTP id c11mr568955itf.106.1504077227872;\n\tWed, 30 Aug 2017 00:13:47 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<13933269.sbLfAJfmVU@aspire.rjw.lan>","References":"<1504018610-10822-1-git-send-email-ulf.hansson@linaro.org>\n\t<1504018610-10822-2-git-send-email-ulf.hansson@linaro.org>\n\t<13933269.sbLfAJfmVU@aspire.rjw.lan>","From":"Ulf Hansson ","Date":"Wed, 30 Aug 2017 09:13:47 +0200","Message-ID":"","Subject":"Re: [PATCH v3 1/8] PM / Sleep: Make the runtime PM centric path\n\tknown to the PM core","To":"\"Rafael J. Wysocki\" ","Cc":"Wolfram Sang , Len Brown ,\n\tACPI Devel Maling List ,\n\t\"linux-pm@vger.kernel.org\" ,\n\tKevin Hilman ,\n\tJarkko Nikula ,\n\tAndy Shevchenko ,\n\tMika Westerberg ,\n\tJisheng Zhang ,\n\tJohn Stultz , Guodong Xu ,\n\tSumit Semwal ,\n\tHaojian Zhuang ,\n\tJohannes Stezenbach ,\n\t\"linux-arm-kernel@lists.infradead.org\" \n\t,\n\t\"linux-i2c@vger.kernel.org\" ","Content-Type":"text/plain; charset=\"UTF-8\"","Sender":"linux-i2c-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-i2c@vger.kernel.org"}},{"id":1760165,"web_url":"http://patchwork.ozlabs.org/comment/1760165/","msgid":"<2403318.HVB4k3qRsK@aspire.rjw.lan>","list_archive_url":null,"date":"2017-08-30T13:37:08","subject":"Re: [PATCH v3 1/8] PM / Sleep: Make the runtime PM centric path\n\tknown to the PM core","submitter":{"id":26536,"url":"http://patchwork.ozlabs.org/api/people/26536/","name":"Rafael J. Wysocki","email":"rjw@rjwysocki.net"},"content":"On Wednesday, August 30, 2017 9:13:47 AM CEST Ulf Hansson wrote:\n> On 29 August 2017 at 17:15, Rafael J. Wysocki wrote:\n> > On Tuesday, August 29, 2017 4:56:43 PM CEST Ulf Hansson wrote:\n> >> The main principle behind the runtime PM centric path, is to re-use the\n> >> runtime PM callbacks to implement system sleep - and while doing that also\n> >> achieve a fully optimized behaviour from PM point of view.\n> >>\n> >> More precisely, avoid to wake up a device from its low power state during\n> >> system sleep, unless the device is really needed to be operational. That\n> >> does not only mean avoiding to waste power, but may also decrease system\n> >> suspend/resume time for a device.\n> >>\n> >> However, using the runtime PM centric path for a device, does put some\n> >> requirements on the behaviour of the PM core and a potential PM domain that\n> >> may be attached to the device. So far, these requirements are not being\n> >> specially considered, except by the generic PM domain.\n> >>\n> >> To move forward and to make it possible to deploy the runtime PM centric\n> >> path for cross SoC drivers, which may have different PM domains attached to\n> >> its devices depending on the SoC, we must address how to deal with these\n> >> requirements. This change starts by making some adoptions to the PM core,\n> >> while other parts, such as the ACPI PM domain needs to be taken care of\n> >> separately.\n> >>\n> >> In the runtime PM centric path, the driver is expected to make use of the\n> >> pm_runtime_force_suspend|resume() helpers, to deploy system sleep support.\n> >> More precisely it may assign the system sleep callbacks to these helpers or\n> >> may call them from its own callbacks, in case it needs to perform\n> >> additional actions during system sleep.\n> >>\n> >> In other words, the PM core must always invoke the system sleep callbacks\n> >> for the device when they are present, to allow the driver to deal with\n> >> the system sleep operations.\n> >>\n> >> In case the PM core decides to run the direct_complete path for the device,\n> >> it skips invoking most of the system sleep callbacks, besides\n> >> ->prepare|complete(). Therefore using the direct_complete path in\n> >> combination with the runtime PM centric patch for a device, does not play\n> >> well.\n> >>\n> >> To deal with this issue, let's add a flag 'is_rpm_sleep', to the struct\n> >> dev_pm_info. The driver that deploys the runtime PM centric path, shall set\n> >> the flag for the device during ->probe(), to inform the PM core about that\n> >> it must not use the direct_complete path for the device.\n> >>\n> >> Note, not allowing the direct_complete path for a device, doesn't implicit\n> >> need to propagate to the device's parent/suppliers. Therefore make the PM\n> >> core check this condition in device_suspend(), before it decides to abandon\n> >> the direct_complete path for parent/suppliers.\n> >>\n> >> To make the is_rpm_sleep flag internal to the PM core, let's add two APIs.\n> >> - dev_pm_use_rpm_sleep(): It sets the flag and should be called by\n> >> the driver during ->probe().\n> >> - dev_pm_is_rpm_sleep(): Makes it possible for users of the device,\n> >> like a PM domain, to fetch the state of the flag.\n> >>\n> >> Signed-off-by: Ulf Hansson \n> >> ---\n> >>\n> >> Changes in v3:\n> >> - New patch.\n> >> - This replaces the earlier method of adding the no_direct_complete to\n> >> the ACPI structures, according to comments from Rafael.\n> >> - This change also address the consern Rafael had around that\n> >> direct_complete should not have to be disabled for parent/suppliers, in\n> >> case a device use the runtime PM centric path for system sleep.\n> >>\n> >> ---\n> >> drivers/base/power/main.c | 49 +++++++++++++++++++++++++++++++++++++++-----\n> >> drivers/base/power/runtime.c | 1 +\n> >> include/linux/pm.h | 7 +++++++\n> >> 3 files changed, 52 insertions(+), 5 deletions(-)\n> >>\n> >> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c\n> >> index ea1732e..865737a 100644\n> >> --- a/drivers/base/power/main.c\n> >> +++ b/drivers/base/power/main.c\n> >> @@ -1549,14 +1549,16 @@ static int __device_suspend(struct device *dev, pm_message_t state, bool async)\n> >> if (parent) {\n> >> spin_lock_irq(&parent->power.lock);\n> >>\n> >> - dev->parent->power.direct_complete = false;\n> >> + if (!dev->power.is_rpm_sleep)\n> >> + dev->parent->power.direct_complete = false;\n> >\n> > This doesn't look good to me at all.\n> >\n> > It potentially breaks the fundamental assumption of the direct_complete\n> > mechanism that no devices with that flag set will ever be runtime resumed\n> > during system suspend.\n> >\n> > Which can happen with this change if a device with power.is_rpm_sleep is\n> > resumed causing the parent to be resumed too.\n> \n> Doesn't the exact same problem you describe, already exist prior my change!?\n\nNo, it doesn't.\n\n> That is, if the current device is runtime suspended and the\n> direct_complete flag is set for it, then __device_suspend() has\n> already disabled runtime PM for the device, when we reach this point.\n> That means, a following attempts to runtime resume the device will\n> fail and thus also to runtime resume its parent.\n\nThat's because any devices with direct_complete set *cannot* be runtime\nresumed after __device_suspend(). That's intentional and how the thing\nis designed. It cannot work differently, sorry.\n\n> So to me, this is a different problem, related how to work out the\n> correct order of how to suspend devices.\n\nThe ordering matters here, but it is not the problem.\n\nSetting direct_complete means that PM callbacks for this device won't be\ninvoked going forward. However, if the state of the device was to change\n(like as a result of runtime PM resume), it would be necessary to run those\ncallbacks after all, but after __device_suspend() it may be too late for\nthat, because the ->suspend callback may have been skipped already.\n\nThe only way to address that is to prevent runtime PM resume of the device\nfrom happening at the point the PM core decides to use direct_complete for it,\nbut that requires runtime PM to be disabled for it. Moreover, since the\ndevice is now suspended with runtime PM disabled and its children might rely\non it if they were active, the children must be suspended with runtime PM\ndisabled at this point too. That's why direct_complete can only be set\nfor a device if it is set for the entire hierarchy below it.\n\nSummary: If you allow a device to be runtime resumed during system susped,\ndirect_complete cannot be set for it and it cannot be set for its parent and\nso on.\n\n[cut]\n\n> >> @@ -1709,11 +1711,14 @@ static int device_prepare(struct device *dev, pm_message_t state)\n> >> * A positive return value from ->prepare() means \"this device appears\n> >> * to be runtime-suspended and its state is fine, so if it really is\n> >> * runtime-suspended, you can leave it in that state provided that you\n> >> - * will do the same thing with all of its descendants\". This only\n> >> - * applies to suspend transitions, however.\n> >> + * will do the same thing with all of its descendants\". To allow this,\n> >> + * the is_rpm_sleep flag must not be set, which is default false, but\n> >> + * may have been changed by a driver. This only applies to suspend\n> >> + * transitions, however.\n> >> */\n> >> spin_lock_irq(&dev->power.lock);\n> >> - dev->power.direct_complete = ret > 0 && state.event == PM_EVENT_SUSPEND;\n> >> + dev->power.direct_complete = ret > 0 && !dev->power.is_rpm_sleep &&\n> >> + state.event == PM_EVENT_SUSPEND;\n> >\n> > The only problem addressed by avoiding direct_complete is when runtime PM needs\n> > to work during __device_suspend() for devices with direct_complete set. You\n> > said that this was not the case with the i2c designware driver, so I'm not sure\n> > what the purpose of this is.\n> \n> I should probably work more on my changelog, because I tried to\n> explain it there.\n> \n> Anyway, let me quote the important parts, and this is not specific for\n> the i2c-designware-driver, but generic to drivers using\n> pm_runtime_force_suspend|resume().\n> \n> \"In the runtime PM centric path, the driver is expected to make use of\n> the pm_runtime_force_suspend|resume() helpers, to deploy system sleep\n> support. More precisely it may assign the system sleep callbacks to\n> these helpers or may call them from its own callbacks, in case it\n> needs to perform additional actions during system sleep.\n> \n> In other words, the PM core must always invoke the system sleep\n> callbacks for the device when they are present, to allow the driver to\n> deal with the system sleep operations.\"\n> \n> A concrete example is drivers/spi/spi-fsl-espi.c, but one can easily find more.\n\nIn fact, the new flag introduced here means \"assume that that the driver of this\ndevice points ->late_suspend and ->early_resume to pm_runtime_force_suspend()\nand pm_runtime_force_resume(), respectively.\" Which then implies that\ndirect_complete cannot be used with it and (and with its parent and so on)\nand that it is not necessary to runtime resume it during system suspend.\n\nHowever, there are (or at least there may be) devices that need not be\nruntime resumed during system suspend even though their drivers don't use\n_force_suspend/resume().\n\nAlso there are devices whose drivers don't want direct_complete to be used with\nthem, even though they don't use _force_suspend/resume() and they *do* need\ntheir devices to be runtime resumed during system suspend.\n\nMoreover, some drivers may not mind direct_complete even though their devices\nneed not be runtime resumed during system suspend. All of that doesn't have\nto be related to using _force_suspend/resume() at all.\n\nSo I don't see any reason whatever for combining all of these three *different*\nconditions under one flag.\n\nThanks,\nRafael","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-i2c-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xj6Jd29TTz9sNc\n\tfor ;\n\tWed, 30 Aug 2017 23:47:45 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751387AbdH3NrS (ORCPT );\n\tWed, 30 Aug 2017 09:47:18 -0400","from cloudserver094114.home.net.pl ([79.96.170.134]:57112 \"EHLO\n\tcloudserver094114.home.net.pl\" rhost-flags-OK-OK-OK-OK)\n\tby vger.kernel.org with ESMTP id S1751302AbdH3Npv (ORCPT\n\t); Wed, 30 Aug 2017 09:45:51 -0400","from 79.184.253.199.ipv4.supernova.orange.pl (79.184.253.199)\n\t(HELO aspire.rjw.lan)\n\tby serwer1319399.home.pl (79.96.170.134) with SMTP (IdeaSmtpServer\n\t0.82) id 950554e770ce4363; Wed, 30 Aug 2017 15:45:49 +0200"],"From":"\"Rafael J. Wysocki\" ","To":"Ulf Hansson ","Cc":"Wolfram Sang , Len Brown ,\n\tACPI Devel Maling List ,\n\t\"linux-pm@vger.kernel.org\" ,\n\tKevin Hilman ,\n\tJarkko Nikula ,\n\tAndy Shevchenko ,\n\tMika Westerberg ,\n\tJisheng Zhang ,\n\tJohn Stultz , Guodong Xu ,\n\tSumit Semwal ,\n\tHaojian Zhuang ,\n\tJohannes Stezenbach ,\n\t\"linux-arm-kernel@lists.infradead.org\" \n\t,\n\t\"linux-i2c@vger.kernel.org\" ","Subject":"Re: [PATCH v3 1/8] PM / Sleep: Make the runtime PM centric path\n\tknown to the PM core","Date":"Wed, 30 Aug 2017 15:37:08 +0200","Message-ID":"<2403318.HVB4k3qRsK@aspire.rjw.lan>","In-Reply-To":"","References":"<1504018610-10822-1-git-send-email-ulf.hansson@linaro.org>\n\t<13933269.sbLfAJfmVU@aspire.rjw.lan>\n\t","MIME-Version":"1.0","Content-Transfer-Encoding":"7Bit","Content-Type":"text/plain; charset=\"us-ascii\"","Sender":"linux-i2c-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-i2c@vger.kernel.org"}},{"id":1760757,"web_url":"http://patchwork.ozlabs.org/comment/1760757/","msgid":"","list_archive_url":null,"date":"2017-08-31T09:06:05","subject":"Re: [PATCH v3 1/8] PM / Sleep: Make the runtime PM centric path\n\tknown to the PM core","submitter":{"id":21036,"url":"http://patchwork.ozlabs.org/api/people/21036/","name":"Ulf Hansson","email":"ulf.hansson@linaro.org"},"content":"[...]\n\n>> >> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c\n>> >> index ea1732e..865737a 100644\n>> >> --- a/drivers/base/power/main.c\n>> >> +++ b/drivers/base/power/main.c\n>> >> @@ -1549,14 +1549,16 @@ static int __device_suspend(struct device *dev, pm_message_t state, bool async)\n>> >> if (parent) {\n>> >> spin_lock_irq(&parent->power.lock);\n>> >>\n>> >> - dev->parent->power.direct_complete = false;\n>> >> + if (!dev->power.is_rpm_sleep)\n>> >> + dev->parent->power.direct_complete = false;\n>> >\n>> > This doesn't look good to me at all.\n>> >\n>> > It potentially breaks the fundamental assumption of the direct_complete\n>> > mechanism that no devices with that flag set will ever be runtime resumed\n>> > during system suspend.\n>> >\n>> > Which can happen with this change if a device with power.is_rpm_sleep is\n>> > resumed causing the parent to be resumed too.\n>>\n>> Doesn't the exact same problem you describe, already exist prior my change!?\n>\n> No, it doesn't.\n>\n>> That is, if the current device is runtime suspended and the\n>> direct_complete flag is set for it, then __device_suspend() has\n>> already disabled runtime PM for the device, when we reach this point.\n>> That means, a following attempts to runtime resume the device will\n>> fail and thus also to runtime resume its parent.\n>\n> That's because any devices with direct_complete set *cannot* be runtime\n> resumed after __device_suspend(). That's intentional and how the thing\n> is designed. It cannot work differently, sorry.\n>\n>> So to me, this is a different problem, related how to work out the\n>> correct order of how to suspend devices.\n>\n> The ordering matters here, but it is not the problem.\n>\n> Setting direct_complete means that PM callbacks for this device won't be\n> invoked going forward. However, if the state of the device was to change\n> (like as a result of runtime PM resume), it would be necessary to run those\n> callbacks after all, but after __device_suspend() it may be too late for\n> that, because the ->suspend callback may have been skipped already.\n>\n> The only way to address that is to prevent runtime PM resume of the device\n> from happening at the point the PM core decides to use direct_complete for it,\n> but that requires runtime PM to be disabled for it. Moreover, since the\n> device is now suspended with runtime PM disabled and its children might rely\n> on it if they were active, the children must be suspended with runtime PM\n> disabled at this point too. That's why direct_complete can only be set\n> for a device if it is set for the entire hierarchy below it.\n\nThanks, this was a very nice description of the direct_complete path!\n\n>\n> Summary: If you allow a device to be runtime resumed during system susped,\n> direct_complete cannot be set for it and it cannot be set for its parent and\n> so on.\n\nYes, this is what it seems to boils done to!\n\nPerhaps the ACPI PM domain is special in this case, because in its\n->prepare() callback it asks the ACPI FW about whether the\ndirect_complete path is possible for a device. In other words, the\nACPI FW seems to be capable of providing information about if a device\nmay become runtime resumed during system suspend. But, really, isn't\nthat just a best guess? No?\n\nOn those ARM SoCs I am working on, non-ACPI, the information about\nwhether a device may become runtime resumed during system suspend,\nmost often can't be find out in a deterministic way. That's because\nthis information depends on how a device is being used by other\ndevices, which changes dynamically and from one system suspend\nsequence to another. In cases like the i2c-designware-plat driver used\nin Hikey (ARM64 board), it can't ever know before hand, if someone is\ngoing to request an i2c transfer during system suspend or not.\n\nTo really deal with these devices properly, we have to suspend them in\nthe correct order.\n\nMy point is, doesn't the ordering problem exists also for a device,\nwhich the PM core runs the direct_complete path for, even if the ACPI\nPM domain is attached to the device? Just because runtime PM is\ndisabled for a device, doesn't mean the ordering issue disappears,\nright?\n\n>\n> [cut]\n>\n>> >> @@ -1709,11 +1711,14 @@ static int device_prepare(struct device *dev, pm_message_t state)\n>> >> * A positive return value from ->prepare() means \"this device appears\n>> >> * to be runtime-suspended and its state is fine, so if it really is\n>> >> * runtime-suspended, you can leave it in that state provided that you\n>> >> - * will do the same thing with all of its descendants\". This only\n>> >> - * applies to suspend transitions, however.\n>> >> + * will do the same thing with all of its descendants\". To allow this,\n>> >> + * the is_rpm_sleep flag must not be set, which is default false, but\n>> >> + * may have been changed by a driver. This only applies to suspend\n>> >> + * transitions, however.\n>> >> */\n>> >> spin_lock_irq(&dev->power.lock);\n>> >> - dev->power.direct_complete = ret > 0 && state.event == PM_EVENT_SUSPEND;\n>> >> + dev->power.direct_complete = ret > 0 && !dev->power.is_rpm_sleep &&\n>> >> + state.event == PM_EVENT_SUSPEND;\n>> >\n>> > The only problem addressed by avoiding direct_complete is when runtime PM needs\n>> > to work during __device_suspend() for devices with direct_complete set. You\n>> > said that this was not the case with the i2c designware driver, so I'm not sure\n>> > what the purpose of this is.\n>>\n>> I should probably work more on my changelog, because I tried to\n>> explain it there.\n>>\n>> Anyway, let me quote the important parts, and this is not specific for\n>> the i2c-designware-driver, but generic to drivers using\n>> pm_runtime_force_suspend|resume().\n>>\n>> \"In the runtime PM centric path, the driver is expected to make use of\n>> the pm_runtime_force_suspend|resume() helpers, to deploy system sleep\n>> support. More precisely it may assign the system sleep callbacks to\n>> these helpers or may call them from its own callbacks, in case it\n>> needs to perform additional actions during system sleep.\n>>\n>> In other words, the PM core must always invoke the system sleep\n>> callbacks for the device when they are present, to allow the driver to\n>> deal with the system sleep operations.\"\n>>\n>> A concrete example is drivers/spi/spi-fsl-espi.c, but one can easily find more.\n>\n> In fact, the new flag introduced here means \"assume that that the driver of this\n> device points ->late_suspend and ->early_resume to pm_runtime_force_suspend()\n> and pm_runtime_force_resume(), respectively.\" Which then implies that\n> direct_complete cannot be used with it and (and with its parent and so on)\n> and that it is not necessary to runtime resume it during system suspend.\n>\n> However, there are (or at least there may be) devices that need not be\n> runtime resumed during system suspend even though their drivers don't use\n> _force_suspend/resume().\n>\n> Also there are devices whose drivers don't want direct_complete to be used with\n> them, even though they don't use _force_suspend/resume() and they *do* need\n> their devices to be runtime resumed during system suspend.\n>\n> Moreover, some drivers may not mind direct_complete even though their devices\n> need not be runtime resumed during system suspend. All of that doesn't have\n> to be related to using _force_suspend/resume() at all.\n>\n> So I don't see any reason whatever for combining all of these three *different*\n> conditions under one flag.\n\nOkay!\n\n>\n> Thanks,\n> Rafael\n>\n\nKind regards\nUffe","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-i2c-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (1024-bit key;\n\tunprotected) header.d=linaro.org header.i=@linaro.org\n\theader.b=\"Nn+JMQ0H\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xjc1J1BXXz9sRW\n\tfor ;\n\tThu, 31 Aug 2017 19:06:12 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751040AbdHaJGJ (ORCPT );\n\tThu, 31 Aug 2017 05:06:09 -0400","from mail-io0-f175.google.com ([209.85.223.175]:38907 \"EHLO\n\tmail-io0-f175.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1750908AbdHaJGH (ORCPT\n\t); Thu, 31 Aug 2017 05:06:07 -0400","by mail-io0-f175.google.com with SMTP id 81so18843143ioj.5\n\tfor ; Thu, 31 Aug 2017 02:06:07 -0700 (PDT)","by 10.2.96.38 with HTTP; Thu, 31 Aug 2017 02:06:05 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc; bh=VVsCK1aj3pD2uRWPs14JsAtIjvGaVY7Dt4qRMLjk52I=;\n\tb=Nn+JMQ0HpLxNh+QPN2XJrawQzfvMKFkoWNYpUSyL5Cdgn0ZrZAn5vxgD6TuOqxOhyP\n\tUStRrFvHOJ+hUky57Hbvjc3og0EuFW7PJQBsbgTiKiAryga6IbUlOWPA//4PQUaN3TKf\n\tlkTQ/bFj+D59xm6g0I36JJSe01eWAXuStHXW0=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc;\n\tbh=VVsCK1aj3pD2uRWPs14JsAtIjvGaVY7Dt4qRMLjk52I=;\n\tb=JKKD1CgCyT2yaMAFxQOmR8lSi0WIvohDJ5213EgnvadXszDNFyj1iFc/vgfkZrLhEq\n\t3cqBCmK3XjcX3LD8QwdiMCRuMk1LifkYghqEkSo10bz0uBzcCdGIKmXdS2jt1cG7jF9m\n\tB7/Zl6QMLDalc+lRtC61PtKW74+CP2sESAsyj/J68Ec6IjGAvD6Zu1H2X52QEfswyhYa\n\tnTSzEFqs4txT7nIaJGvfcOYk4n8IXlznp+e9Jvk1qUp2HhZoCKKMsr/mMtNH7IxRp4r8\n\tvcdmLp4z0NxuhlRcGeyzkmGCPvCTq9z2aGus7qpT96TSxU1kdUo3We4hcak/fh9qEkTl\n\tQb7A==","X-Gm-Message-State":"AHPjjUgeXnPt9RX3XJIBDy4Te64Zp7ZQ5BYv9BGcayk2XiRTgW2YDHaZ\n\tH80ubEuNhcAg0T7zVCH6mDiE1B4j3j7d","X-Received":"by 10.107.138.155 with SMTP id c27mr4078391ioj.103.1504170366608;\n\tThu, 31 Aug 2017 02:06:06 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<2403318.HVB4k3qRsK@aspire.rjw.lan>","References":"<1504018610-10822-1-git-send-email-ulf.hansson@linaro.org>\n\t<13933269.sbLfAJfmVU@aspire.rjw.lan>\n\t\n\t<2403318.HVB4k3qRsK@aspire.rjw.lan>","From":"Ulf Hansson ","Date":"Thu, 31 Aug 2017 11:06:05 +0200","Message-ID":"","Subject":"Re: [PATCH v3 1/8] PM / Sleep: Make the runtime PM centric path\n\tknown to the PM core","To":"\"Rafael J. Wysocki\" ","Cc":"Wolfram Sang , Len Brown ,\n\tACPI Devel Maling List ,\n\t\"linux-pm@vger.kernel.org\" ,\n\tKevin Hilman ,\n\tJarkko Nikula ,\n\tAndy Shevchenko ,\n\tMika Westerberg ,\n\tJisheng Zhang ,\n\tJohn Stultz , Guodong Xu ,\n\tSumit Semwal ,\n\tHaojian Zhuang ,\n\tJohannes Stezenbach ,\n\t\"linux-arm-kernel@lists.infradead.org\" \n\t,\n\t\"linux-i2c@vger.kernel.org\" ","Content-Type":"text/plain; charset=\"UTF-8\"","Sender":"linux-i2c-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-i2c@vger.kernel.org"}},{"id":1762131,"web_url":"http://patchwork.ozlabs.org/comment/1762131/","msgid":"<1702559.yU4llTGfcI@aspire.rjw.lan>","list_archive_url":null,"date":"2017-09-02T14:48:26","subject":"Re: [PATCH v3 1/8] PM / Sleep: Make the runtime PM centric path\n\tknown to the PM core","submitter":{"id":26536,"url":"http://patchwork.ozlabs.org/api/people/26536/","name":"Rafael J. Wysocki","email":"rjw@rjwysocki.net"},"content":"On Thursday, August 31, 2017 11:06:05 AM CEST Ulf Hansson wrote:\n> [...]\n> \n> >> >> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c\n> >> >> index ea1732e..865737a 100644\n> >> >> --- a/drivers/base/power/main.c\n> >> >> +++ b/drivers/base/power/main.c\n> >> >> @@ -1549,14 +1549,16 @@ static int __device_suspend(struct device *dev, pm_message_t state, bool async)\n> >> >> if (parent) {\n> >> >> spin_lock_irq(&parent->power.lock);\n> >> >>\n> >> >> - dev->parent->power.direct_complete = false;\n> >> >> + if (!dev->power.is_rpm_sleep)\n> >> >> + dev->parent->power.direct_complete = false;\n> >> >\n> >> > This doesn't look good to me at all.\n> >> >\n> >> > It potentially breaks the fundamental assumption of the direct_complete\n> >> > mechanism that no devices with that flag set will ever be runtime resumed\n> >> > during system suspend.\n> >> >\n> >> > Which can happen with this change if a device with power.is_rpm_sleep is\n> >> > resumed causing the parent to be resumed too.\n> >>\n> >> Doesn't the exact same problem you describe, already exist prior my change!?\n> >\n> > No, it doesn't.\n> >\n> >> That is, if the current device is runtime suspended and the\n> >> direct_complete flag is set for it, then __device_suspend() has\n> >> already disabled runtime PM for the device, when we reach this point.\n> >> That means, a following attempts to runtime resume the device will\n> >> fail and thus also to runtime resume its parent.\n> >\n> > That's because any devices with direct_complete set *cannot* be runtime\n> > resumed after __device_suspend(). That's intentional and how the thing\n> > is designed. It cannot work differently, sorry.\n> >\n> >> So to me, this is a different problem, related how to work out the\n> >> correct order of how to suspend devices.\n> >\n> > The ordering matters here, but it is not the problem.\n> >\n> > Setting direct_complete means that PM callbacks for this device won't be\n> > invoked going forward. However, if the state of the device was to change\n> > (like as a result of runtime PM resume), it would be necessary to run those\n> > callbacks after all, but after __device_suspend() it may be too late for\n> > that, because the ->suspend callback may have been skipped already.\n> >\n> > The only way to address that is to prevent runtime PM resume of the device\n> > from happening at the point the PM core decides to use direct_complete for it,\n> > but that requires runtime PM to be disabled for it. Moreover, since the\n> > device is now suspended with runtime PM disabled and its children might rely\n> > on it if they were active, the children must be suspended with runtime PM\n> > disabled at this point too. That's why direct_complete can only be set\n> > for a device if it is set for the entire hierarchy below it.\n> \n> Thanks, this was a very nice description of the direct_complete path!\n> \n> >\n> > Summary: If you allow a device to be runtime resumed during system susped,\n> > direct_complete cannot be set for it and it cannot be set for its parent and\n> > so on.\n> \n> Yes, this is what it seems to boils done to!\n> \n> Perhaps the ACPI PM domain is special in this case, because in its\n> ->prepare() callback it asks the ACPI FW about whether the\n> direct_complete path is possible for a device. In other words, the\n> ACPI FW seems to be capable of providing information about if a device\n> may become runtime resumed during system suspend. But, really, isn't\n> that just a best guess? No?\n\nI wouldn't call it a guess, but it may go too far.\n\nThe part that the ACPI PM domain can figure out is whether or not the power\nstate of the device needs to be updated due to differences between runtime\nPM and system suspend configuration requirements. If it doesn't need to be\nupdated, acpi_subsys_prepare() returns 1 which may me overoptimistic.\n\nFor many devices that works, because the only possible reason why they may\nneed to be accessed during system suspend is to update their power state.\n\nIt may not work for some devices, though, because there may be other reasons\nfor accessing them during system suspend and that's where some opt-out way\nis needed.\n\n> On those ARM SoCs I am working on, non-ACPI, the information about\n> whether a device may become runtime resumed during system suspend,\n> most often can't be find out in a deterministic way. That's because\n> this information depends on how a device is being used by other\n> devices, which changes dynamically and from one system suspend\n> sequence to another. In cases like the i2c-designware-plat driver used\n> in Hikey (ARM64 board), it can't ever know before hand, if someone is\n> going to request an i2c transfer during system suspend or not.\n\nWell, I2C is somewhat special in that respect, because dependencies between\ndevices in there are not tracked AFAICS. \n\n> To really deal with these devices properly, we have to suspend them in\n> the correct order.\n\nRight, but in order to achieve that the right ordering actually needs\nto be known. If it is not known, there is no way to get that right in\ngeneral. :-)\n\nThe problem basically is that during system suspend you need to disable\nthe controller at one point, sooner or later, and keep it disabled from\nthat point on. Of course, that should happen when it is guaranteed that\nthere won't be any new transactions going forward, but you cannot actually\nguaranee that without knowing that all of its \"consumers\" have already\nbeen suspended.\n\n> My point is, doesn't the ordering problem exists also for a device,\n> which the PM core runs the direct_complete path for, even if the ACPI\n> PM domain is attached to the device? Just because runtime PM is\n> disabled for a device, doesn't mean the ordering issue disappears,\n> right?\n\nThe ordering issue is there if there are dependencies between devices the\nPM core doesn't know about. Otherwise, the rule that direct_complete can\nonly be used for a given device if it also is used for the entire hierarchy\nbelow it (and for all of the device's \"consumers\" and everything that depends\non them too for that matter) makes it go away (it serves as a guarantee that\nall of the device's \"consumers\" have already been suspended).\n\nThanks,\nRafael","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-i2c-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xkzjZ3nFvz9t0F\n\tfor ;\n\tSun, 3 Sep 2017 00:57:22 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752684AbdIBO5N (ORCPT );\n\tSat, 2 Sep 2017 10:57:13 -0400","from cloudserver094114.home.net.pl ([79.96.170.134]:52840 \"EHLO\n\tcloudserver094114.home.net.pl\" rhost-flags-OK-OK-OK-OK)\n\tby vger.kernel.org with ESMTP id S1752660AbdIBO5M (ORCPT\n\t); Sat, 2 Sep 2017 10:57:12 -0400","from 79.184.253.199.ipv4.supernova.orange.pl (79.184.253.199)\n\t(HELO aspire.rjw.lan)\n\tby serwer1319399.home.pl (79.96.170.134) with SMTP (IdeaSmtpServer\n\t0.82) id 35890705de9269f4; Sat, 2 Sep 2017 16:57:10 +0200"],"From":"\"Rafael J. Wysocki\" ","To":"Ulf Hansson ","Cc":"Wolfram Sang , Len Brown ,\n\tACPI Devel Maling List ,\n\t\"linux-pm@vger.kernel.org\" ,\n\tKevin Hilman ,\n\tJarkko Nikula ,\n\tAndy Shevchenko ,\n\tMika Westerberg ,\n\tJisheng Zhang ,\n\tJohn Stultz , Guodong Xu ,\n\tSumit Semwal ,\n\tHaojian Zhuang ,\n\tJohannes Stezenbach ,\n\t\"linux-arm-kernel@lists.infradead.org\" \n\t,\n\t\"linux-i2c@vger.kernel.org\" ","Subject":"Re: [PATCH v3 1/8] PM / Sleep: Make the runtime PM centric path\n\tknown to the PM core","Date":"Sat, 02 Sep 2017 16:48:26 +0200","Message-ID":"<1702559.yU4llTGfcI@aspire.rjw.lan>","In-Reply-To":"","References":"<1504018610-10822-1-git-send-email-ulf.hansson@linaro.org>\n\t<2403318.HVB4k3qRsK@aspire.rjw.lan>\n\t","MIME-Version":"1.0","Content-Transfer-Encoding":"7Bit","Content-Type":"text/plain; charset=\"us-ascii\"","Sender":"linux-i2c-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-i2c@vger.kernel.org"}}]