Re: [PATCH 3/3] PM / Domains: Add support for PM domain on/off notifiers for genpd
On Mon, 21 Sep 2020 at 18:11, Rafael J. Wysocki wrote: > > On Wed, Aug 26, 2020 at 8:38 AM Ulf Hansson wrote: > > > > On Tue, 25 Aug 2020 at 18:16, Lina Iyer wrote: > > > > > > On Wed, Aug 19 2020 at 04:41 -0600, Ulf Hansson wrote: > > > >A device may have specific HW constraints that must be obeyed to, before > > > >its corresponding PM domain (genpd) can be powered off - and vice verse > > > >at > > > >power on. These constraints can't be managed through the regular runtime > > > >PM > > > >based deployment for a device, because the access pattern for it, isn't > > > >always request based. In other words, using the runtime PM callbacks to > > > >deal with the constraints doesn't work for these cases. > > > > > > > >For these reasons, let's instead add a PM domain power on/off > > > >notification > > > >mechanism to genpd. To add/remove a notifier for a device, the device > > > >must > > > >already have been attached to the genpd, which also means that it needs > > > >to > > > >be a part of the PM domain topology. > > > > > > > >To add/remove a notifier, let's introduce two genpd specific functions: > > > > - dev_pm_genpd_add|remove_notifier() > > > > > > > >Note that, to further clarify when genpd power on/off notifiers may be > > > >used, one can compare with the existing CPU_CLUSTER_PM_ENTER|EXIT > > > >notifiers. In the long run, the genpd power on/off notifiers should be > > > >able > > > >to replace them, but that requires additional genpd based platform > > > >support > > > >for the current users. > > > > > > > >Signed-off-by: Ulf Hansson > > > >--- > > > > drivers/base/power/domain.c | 130 ++-- > > > > include/linux/pm_domain.h | 15 + > > > > 2 files changed, 141 insertions(+), 4 deletions(-) > > > > > > > >diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > > > >index 4b787e1ff188..9cb85a5e8342 100644 > > > >--- a/drivers/base/power/domain.c > > > >+++ b/drivers/base/power/domain.c > > > >@@ -545,13 +545,21 @@ static int genpd_power_off(struct > > > >generic_pm_domain *genpd, bool one_dev_on, > > > > if (!genpd->gov) > > > > genpd->state_idx = 0; > > > > > > > >+ /* Notify consumers that we are about to power off. */ > > > >+ ret = raw_notifier_call_chain(&genpd->power_notifiers, > > > >GENPD_STATE_OFF, > > > >+NULL); > > > >+ if (ret) > > > >+ return ret; > > > >+ > > > > /* Don't power off, if a child domain is waiting to power on. */ > > > >- if (atomic_read(&genpd->sd_count) > 0) > > > >- return -EBUSY; > > > >+ if (atomic_read(&genpd->sd_count) > 0) { > > > >+ ret = -EBUSY; > > > >+ goto busy; > > > >+ } > > > > > > > > ret = _genpd_power_off(genpd, true); > > > > if (ret) > > > >- return ret; > > > >+ goto busy; > > > > > > > > genpd->status = GENPD_STATE_OFF; > > > > genpd_update_accounting(genpd); > > > >@@ -564,6 +572,9 @@ static int genpd_power_off(struct generic_pm_domain > > > >*genpd, bool one_dev_on, > > > > } > > > > > > > > return 0; > > > >+busy: > > > >+ raw_notifier_call_chain(&genpd->power_notifiers, GENPD_STATE_ON, > > > >NULL); > > > It would be helpful to abstract these notification related calls into > > > functions of their own. So, for CPU PM domains, it would help to add > > > RCU_NONIDLE() around the notifiers, allowing the callbacks to add trace > > > functions. > > > > Thanks for the suggestion! It makes perfect sense to me - and would > > also be consistent with how CPU PM notifiers are managed, > > So I thought that you wanted to send a v2, but I cannot find it. Yes, I am about to post it. I just wanted to give people more time to comment. Kind regards Uffe
Re: [PATCH 3/3] PM / Domains: Add support for PM domain on/off notifiers for genpd
On Wed, Aug 26, 2020 at 8:38 AM Ulf Hansson wrote: > > On Tue, 25 Aug 2020 at 18:16, Lina Iyer wrote: > > > > On Wed, Aug 19 2020 at 04:41 -0600, Ulf Hansson wrote: > > >A device may have specific HW constraints that must be obeyed to, before > > >its corresponding PM domain (genpd) can be powered off - and vice verse at > > >power on. These constraints can't be managed through the regular runtime PM > > >based deployment for a device, because the access pattern for it, isn't > > >always request based. In other words, using the runtime PM callbacks to > > >deal with the constraints doesn't work for these cases. > > > > > >For these reasons, let's instead add a PM domain power on/off notification > > >mechanism to genpd. To add/remove a notifier for a device, the device must > > >already have been attached to the genpd, which also means that it needs to > > >be a part of the PM domain topology. > > > > > >To add/remove a notifier, let's introduce two genpd specific functions: > > > - dev_pm_genpd_add|remove_notifier() > > > > > >Note that, to further clarify when genpd power on/off notifiers may be > > >used, one can compare with the existing CPU_CLUSTER_PM_ENTER|EXIT > > >notifiers. In the long run, the genpd power on/off notifiers should be able > > >to replace them, but that requires additional genpd based platform support > > >for the current users. > > > > > >Signed-off-by: Ulf Hansson > > >--- > > > drivers/base/power/domain.c | 130 ++-- > > > include/linux/pm_domain.h | 15 + > > > 2 files changed, 141 insertions(+), 4 deletions(-) > > > > > >diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > > >index 4b787e1ff188..9cb85a5e8342 100644 > > >--- a/drivers/base/power/domain.c > > >+++ b/drivers/base/power/domain.c > > >@@ -545,13 +545,21 @@ static int genpd_power_off(struct generic_pm_domain > > >*genpd, bool one_dev_on, > > > if (!genpd->gov) > > > genpd->state_idx = 0; > > > > > >+ /* Notify consumers that we are about to power off. */ > > >+ ret = raw_notifier_call_chain(&genpd->power_notifiers, > > >GENPD_STATE_OFF, > > >+NULL); > > >+ if (ret) > > >+ return ret; > > >+ > > > /* Don't power off, if a child domain is waiting to power on. */ > > >- if (atomic_read(&genpd->sd_count) > 0) > > >- return -EBUSY; > > >+ if (atomic_read(&genpd->sd_count) > 0) { > > >+ ret = -EBUSY; > > >+ goto busy; > > >+ } > > > > > > ret = _genpd_power_off(genpd, true); > > > if (ret) > > >- return ret; > > >+ goto busy; > > > > > > genpd->status = GENPD_STATE_OFF; > > > genpd_update_accounting(genpd); > > >@@ -564,6 +572,9 @@ static int genpd_power_off(struct generic_pm_domain > > >*genpd, bool one_dev_on, > > > } > > > > > > return 0; > > >+busy: > > >+ raw_notifier_call_chain(&genpd->power_notifiers, GENPD_STATE_ON, > > >NULL); > > It would be helpful to abstract these notification related calls into > > functions of their own. So, for CPU PM domains, it would help to add > > RCU_NONIDLE() around the notifiers, allowing the callbacks to add trace > > functions. > > Thanks for the suggestion! It makes perfect sense to me - and would > also be consistent with how CPU PM notifiers are managed, So I thought that you wanted to send a v2, but I cannot find it. Cheers!
Re: [PATCH 3/3] PM / Domains: Add support for PM domain on/off notifiers for genpd
-Original Message- Date: Wed, 19 Aug 2020 12:40:57 +0200 From: Ulf Hansson To: "Rafael J . Wysocki" , Kevin Hilman , linux...@vger.kernel.org Cc: Sudeep Holla , Lorenzo Pieralisi , Daniel Lezcano , Lina Iyer , Lukasz Luba , Vincent Guittot , Stephen Boyd , Bjorn Andersson , Benjamin Gaignard , Ulf Hansson , linux-arm-ker...@lists.infradead.org, linux-kernel@vger.kernel.org Subject: [PATCH 3/3] PM / Domains: Add support for PM domain on/off notifiers for genpd A device may have specific HW constraints that must be obeyed to, before its corresponding PM domain (genpd) can be powered off - and vice verse at power on. These constraints can't be managed through the regular runtime PM based deployment for a device, because the access pattern for it, isn't always request based. In other words, using the runtime PM callbacks to deal with the constraints doesn't work for these cases. For these reasons, let's instead add a PM domain power on/off notification mechanism to genpd. To add/remove a notifier for a device, the device must already have been attached to the genpd, which also means that it needs to be a part of the PM domain topology. To add/remove a notifier, let's introduce two genpd specific functions: - dev_pm_genpd_add|remove_notifier() Note that, to further clarify when genpd power on/off notifiers may be used, one can compare with the existing CPU_CLUSTER_PM_ENTER|EXIT notifiers. In the long run, the genpd power on/off notifiers should be able to replace them, but that requires additional genpd based platform support for the current users. Signed-off-by: Ulf Hansson --- drivers/base/power/domain.c | 130 ++-- include/linux/pm_domain.h | 15 + 2 files changed, 141 insertions(+), 4 deletions(-) diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 4b787e1ff188..9cb85a5e8342 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -545,13 +545,21 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on, if (!genpd->gov) genpd->state_idx = 0; + /* Notify consumers that we are about to power off. */ + ret = raw_notifier_call_chain(&genpd->power_notifiers, GENPD_STATE_OFF, + NULL); + if (ret) + return ret; + /* Don't power off, if a child domain is waiting to power on. */ - if (atomic_read(&genpd->sd_count) > 0) - return -EBUSY; + if (atomic_read(&genpd->sd_count) > 0) { + ret = -EBUSY; + goto busy; + } ret = _genpd_power_off(genpd, true); if (ret) - return ret; + goto busy; genpd->status = GENPD_STATE_OFF; genpd_update_accounting(genpd); @@ -564,6 +572,9 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on, } return 0; +busy: + raw_notifier_call_chain(&genpd->power_notifiers, GENPD_STATE_ON, NULL); + return ret; } /** @@ -606,6 +617,9 @@ static int genpd_power_on(struct generic_pm_domain *genpd, unsigned int depth) if (ret) goto err; + /* Inform consumers that we have powered on. */ + raw_notifier_call_chain(&genpd->power_notifiers, GENPD_STATE_ON, NULL); + genpd->status = GENPD_STATE_ON; genpd_update_accounting(genpd); @@ -948,9 +962,18 @@ static void genpd_sync_power_off(struct generic_pm_domain *genpd, bool use_lock, /* Choose the deepest state when suspending */ genpd->state_idx = genpd->state_count - 1; - if (_genpd_power_off(genpd, false)) + + /* Notify consumers that we are about to power off. */ + if (raw_notifier_call_chain(&genpd->power_notifiers, + GENPD_STATE_OFF, NULL)) return; If any of the notified returns an error, we probably should inform that the genpd failed as well. -- Lina + if (_genpd_power_off(genpd, false)) { + raw_notifier_call_chain(&genpd->power_notifiers, + GENPD_STATE_ON, NULL); + return; + } + genpd->status = GENPD_STATE_OFF; list_for_each_entry(link, &genpd->child_links, child_node) { @@ -998,6 +1021,9 @@ static void genpd_sync_power_on(struct generic_pm_domain *genpd, bool use_lock, _genpd_power_on(genpd, false); + /* Inform consumers that we have powered on. */ + raw_notifier_call_chain(&genpd->power_notifiers, GENPD_STATE_ON, NULL); + genpd->status = GENPD_STATE_ON; } @@ -1593,6 +1619,101 @@ int pm_genpd_remove_device(struct device *dev) } EXPORT_SYMBOL_GPL(pm_genpd_remove_device); +/** + * dev_pm_genpd_add_notifier - Add a genpd power on/off notifie
Re: [PATCH 3/3] PM / Domains: Add support for PM domain on/off notifiers for genpd
On Tue, 25 Aug 2020 at 18:16, Lina Iyer wrote: > > On Wed, Aug 19 2020 at 04:41 -0600, Ulf Hansson wrote: > >A device may have specific HW constraints that must be obeyed to, before > >its corresponding PM domain (genpd) can be powered off - and vice verse at > >power on. These constraints can't be managed through the regular runtime PM > >based deployment for a device, because the access pattern for it, isn't > >always request based. In other words, using the runtime PM callbacks to > >deal with the constraints doesn't work for these cases. > > > >For these reasons, let's instead add a PM domain power on/off notification > >mechanism to genpd. To add/remove a notifier for a device, the device must > >already have been attached to the genpd, which also means that it needs to > >be a part of the PM domain topology. > > > >To add/remove a notifier, let's introduce two genpd specific functions: > > - dev_pm_genpd_add|remove_notifier() > > > >Note that, to further clarify when genpd power on/off notifiers may be > >used, one can compare with the existing CPU_CLUSTER_PM_ENTER|EXIT > >notifiers. In the long run, the genpd power on/off notifiers should be able > >to replace them, but that requires additional genpd based platform support > >for the current users. > > > >Signed-off-by: Ulf Hansson > >--- > > drivers/base/power/domain.c | 130 ++-- > > include/linux/pm_domain.h | 15 + > > 2 files changed, 141 insertions(+), 4 deletions(-) > > > >diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > >index 4b787e1ff188..9cb85a5e8342 100644 > >--- a/drivers/base/power/domain.c > >+++ b/drivers/base/power/domain.c > >@@ -545,13 +545,21 @@ static int genpd_power_off(struct generic_pm_domain > >*genpd, bool one_dev_on, > > if (!genpd->gov) > > genpd->state_idx = 0; > > > >+ /* Notify consumers that we are about to power off. */ > >+ ret = raw_notifier_call_chain(&genpd->power_notifiers, > >GENPD_STATE_OFF, > >+NULL); > >+ if (ret) > >+ return ret; > >+ > > /* Don't power off, if a child domain is waiting to power on. */ > >- if (atomic_read(&genpd->sd_count) > 0) > >- return -EBUSY; > >+ if (atomic_read(&genpd->sd_count) > 0) { > >+ ret = -EBUSY; > >+ goto busy; > >+ } > > > > ret = _genpd_power_off(genpd, true); > > if (ret) > >- return ret; > >+ goto busy; > > > > genpd->status = GENPD_STATE_OFF; > > genpd_update_accounting(genpd); > >@@ -564,6 +572,9 @@ static int genpd_power_off(struct generic_pm_domain > >*genpd, bool one_dev_on, > > } > > > > return 0; > >+busy: > >+ raw_notifier_call_chain(&genpd->power_notifiers, GENPD_STATE_ON, > >NULL); > It would be helpful to abstract these notification related calls into > functions of their own. So, for CPU PM domains, it would help to add > RCU_NONIDLE() around the notifiers, allowing the callbacks to add trace > functions. Thanks for the suggestion! It makes perfect sense to me - and would also be consistent with how CPU PM notifiers are managed, [,,,] Kind regards Uffe
Re: [PATCH 3/3] PM / Domains: Add support for PM domain on/off notifiers for genpd
On Wed, Aug 19 2020 at 04:41 -0600, Ulf Hansson wrote: A device may have specific HW constraints that must be obeyed to, before its corresponding PM domain (genpd) can be powered off - and vice verse at power on. These constraints can't be managed through the regular runtime PM based deployment for a device, because the access pattern for it, isn't always request based. In other words, using the runtime PM callbacks to deal with the constraints doesn't work for these cases. For these reasons, let's instead add a PM domain power on/off notification mechanism to genpd. To add/remove a notifier for a device, the device must already have been attached to the genpd, which also means that it needs to be a part of the PM domain topology. To add/remove a notifier, let's introduce two genpd specific functions: - dev_pm_genpd_add|remove_notifier() Note that, to further clarify when genpd power on/off notifiers may be used, one can compare with the existing CPU_CLUSTER_PM_ENTER|EXIT notifiers. In the long run, the genpd power on/off notifiers should be able to replace them, but that requires additional genpd based platform support for the current users. Signed-off-by: Ulf Hansson --- drivers/base/power/domain.c | 130 ++-- include/linux/pm_domain.h | 15 + 2 files changed, 141 insertions(+), 4 deletions(-) diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 4b787e1ff188..9cb85a5e8342 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -545,13 +545,21 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on, if (!genpd->gov) genpd->state_idx = 0; + /* Notify consumers that we are about to power off. */ + ret = raw_notifier_call_chain(&genpd->power_notifiers, GENPD_STATE_OFF, + NULL); + if (ret) + return ret; + /* Don't power off, if a child domain is waiting to power on. */ - if (atomic_read(&genpd->sd_count) > 0) - return -EBUSY; + if (atomic_read(&genpd->sd_count) > 0) { + ret = -EBUSY; + goto busy; + } ret = _genpd_power_off(genpd, true); if (ret) - return ret; + goto busy; genpd->status = GENPD_STATE_OFF; genpd_update_accounting(genpd); @@ -564,6 +572,9 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on, } return 0; +busy: + raw_notifier_call_chain(&genpd->power_notifiers, GENPD_STATE_ON, NULL); It would be helpful to abstract these notification related calls into functions of their own. So, for CPU PM domains, it would help to add RCU_NONIDLE() around the notifiers, allowing the callbacks to add trace functions. --Lina + return ret; } /** @@ -606,6 +617,9 @@ static int genpd_power_on(struct generic_pm_domain *genpd, unsigned int depth) if (ret) goto err; + /* Inform consumers that we have powered on. */ + raw_notifier_call_chain(&genpd->power_notifiers, GENPD_STATE_ON, NULL); + genpd->status = GENPD_STATE_ON; genpd_update_accounting(genpd); @@ -948,9 +962,18 @@ static void genpd_sync_power_off(struct generic_pm_domain *genpd, bool use_lock, /* Choose the deepest state when suspending */ genpd->state_idx = genpd->state_count - 1; - if (_genpd_power_off(genpd, false)) + + /* Notify consumers that we are about to power off. */ + if (raw_notifier_call_chain(&genpd->power_notifiers, + GENPD_STATE_OFF, NULL)) return; + if (_genpd_power_off(genpd, false)) { + raw_notifier_call_chain(&genpd->power_notifiers, + GENPD_STATE_ON, NULL); + return; + } + genpd->status = GENPD_STATE_OFF; list_for_each_entry(link, &genpd->child_links, child_node) { @@ -998,6 +1021,9 @@ static void genpd_sync_power_on(struct generic_pm_domain *genpd, bool use_lock, _genpd_power_on(genpd, false); + /* Inform consumers that we have powered on. */ + raw_notifier_call_chain(&genpd->power_notifiers, GENPD_STATE_ON, NULL); + genpd->status = GENPD_STATE_ON; } @@ -1593,6 +1619,101 @@ int pm_genpd_remove_device(struct device *dev) } EXPORT_SYMBOL_GPL(pm_genpd_remove_device); +/** + * dev_pm_genpd_add_notifier - Add a genpd power on/off notifier for @dev + * + * @dev: Device that should be associated with the notifier + * @nb: The notifier block to register + * + * Users may call this function to add a genpd power on/off notifier for an + * attached @dev. Only one notifier per device is allowed. The notifier is + * sent when genpd is powering on/off the PM domain. + * + * It is assumed that the user guarantee that the genpd wouldn't be detached + * while this routine is getting called. + * + *
[PATCH 3/3] PM / Domains: Add support for PM domain on/off notifiers for genpd
A device may have specific HW constraints that must be obeyed to, before its corresponding PM domain (genpd) can be powered off - and vice verse at power on. These constraints can't be managed through the regular runtime PM based deployment for a device, because the access pattern for it, isn't always request based. In other words, using the runtime PM callbacks to deal with the constraints doesn't work for these cases. For these reasons, let's instead add a PM domain power on/off notification mechanism to genpd. To add/remove a notifier for a device, the device must already have been attached to the genpd, which also means that it needs to be a part of the PM domain topology. To add/remove a notifier, let's introduce two genpd specific functions: - dev_pm_genpd_add|remove_notifier() Note that, to further clarify when genpd power on/off notifiers may be used, one can compare with the existing CPU_CLUSTER_PM_ENTER|EXIT notifiers. In the long run, the genpd power on/off notifiers should be able to replace them, but that requires additional genpd based platform support for the current users. Signed-off-by: Ulf Hansson --- drivers/base/power/domain.c | 130 ++-- include/linux/pm_domain.h | 15 + 2 files changed, 141 insertions(+), 4 deletions(-) diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 4b787e1ff188..9cb85a5e8342 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -545,13 +545,21 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on, if (!genpd->gov) genpd->state_idx = 0; + /* Notify consumers that we are about to power off. */ + ret = raw_notifier_call_chain(&genpd->power_notifiers, GENPD_STATE_OFF, + NULL); + if (ret) + return ret; + /* Don't power off, if a child domain is waiting to power on. */ - if (atomic_read(&genpd->sd_count) > 0) - return -EBUSY; + if (atomic_read(&genpd->sd_count) > 0) { + ret = -EBUSY; + goto busy; + } ret = _genpd_power_off(genpd, true); if (ret) - return ret; + goto busy; genpd->status = GENPD_STATE_OFF; genpd_update_accounting(genpd); @@ -564,6 +572,9 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on, } return 0; +busy: + raw_notifier_call_chain(&genpd->power_notifiers, GENPD_STATE_ON, NULL); + return ret; } /** @@ -606,6 +617,9 @@ static int genpd_power_on(struct generic_pm_domain *genpd, unsigned int depth) if (ret) goto err; + /* Inform consumers that we have powered on. */ + raw_notifier_call_chain(&genpd->power_notifiers, GENPD_STATE_ON, NULL); + genpd->status = GENPD_STATE_ON; genpd_update_accounting(genpd); @@ -948,9 +962,18 @@ static void genpd_sync_power_off(struct generic_pm_domain *genpd, bool use_lock, /* Choose the deepest state when suspending */ genpd->state_idx = genpd->state_count - 1; - if (_genpd_power_off(genpd, false)) + + /* Notify consumers that we are about to power off. */ + if (raw_notifier_call_chain(&genpd->power_notifiers, + GENPD_STATE_OFF, NULL)) return; + if (_genpd_power_off(genpd, false)) { + raw_notifier_call_chain(&genpd->power_notifiers, + GENPD_STATE_ON, NULL); + return; + } + genpd->status = GENPD_STATE_OFF; list_for_each_entry(link, &genpd->child_links, child_node) { @@ -998,6 +1021,9 @@ static void genpd_sync_power_on(struct generic_pm_domain *genpd, bool use_lock, _genpd_power_on(genpd, false); + /* Inform consumers that we have powered on. */ + raw_notifier_call_chain(&genpd->power_notifiers, GENPD_STATE_ON, NULL); + genpd->status = GENPD_STATE_ON; } @@ -1593,6 +1619,101 @@ int pm_genpd_remove_device(struct device *dev) } EXPORT_SYMBOL_GPL(pm_genpd_remove_device); +/** + * dev_pm_genpd_add_notifier - Add a genpd power on/off notifier for @dev + * + * @dev: Device that should be associated with the notifier + * @nb: The notifier block to register + * + * Users may call this function to add a genpd power on/off notifier for an + * attached @dev. Only one notifier per device is allowed. The notifier is + * sent when genpd is powering on/off the PM domain. + * + * It is assumed that the user guarantee that the genpd wouldn't be detached + * while this routine is getting called. + * + * Returns 0 on success and negative error values on failures. + */ +int dev_pm_genpd_add_notifier(struct device *dev, struct notifier_block *nb) +{ + struct generic_pm_domain *genpd; + struct generic_pm_domain_data *gpd_data; + int ret; + + genp