On 08-06-17, 09:48, Ulf Hansson wrote:
> It's not a nightmare, just a tricky thing to solve. :-)
I may have just solved it actually :)
So the real locking problem was the case where a subdomain have multiple parent
domains and how do we access its performance_state field from all the paths that
originate from the parent's update and from the subdomains own path. And a
single performance_state field isn't going to help in that as multiple locks are
involved here. I have added another per parent-domain field and that will help
get the locking quite simple. Here is the new patch (compile tested):
---
drivers/base/power/domain.c | 193 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/pm_domain.h | 22 +++++
2 files changed, 215 insertions(+)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 71c95ad808d5..40815974287f 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -466,6 +466,187 @@ static int genpd_dev_pm_qos_notifier(struct
notifier_block *nb,
return NOTIFY_DONE;
}
+/*
+ * Returns true if anyone in genpd's parent hierarchy has
+ * set_performance_state() set.
+ */
+static bool genpd_has_set_performance_state(struct generic_pm_domain *genpd)
+{
+ struct gpd_link *link;
+
+ if (genpd->set_performance_state)
+ return true;
+
+ list_for_each_entry(link, &genpd->slave_links, slave_node) {
+ if (genpd_has_set_performance_state(link->master))
+ return true;
+ }
+
+ return false;
+}
+
+/**
+ * pm_genpd_has_performance_state - Checks if power domain does performance
+ * state management.
+ *
+ * @dev: Device whose power domain is getting inquired.
+ */
+bool pm_genpd_has_performance_state(struct device *dev)
+{
+ struct generic_pm_domain *genpd = dev_to_genpd(dev);
+
+ /* The parent domain must have set get_performance_state() */
+ if (!IS_ERR(genpd) && genpd->get_performance_state)
+ return true;
+
+ return false;
+}
+EXPORT_SYMBOL_GPL(pm_genpd_has_performance_state);
+
+/*
+ * Re-evaluate performance state of a power domain. Finds the highest requested
+ * performance state by the devices and subdomains within the power domain and
+ * then tries to change its performance state. If the power domain doesn't have
+ * a set_performance_state() callback, then we move the request to its parent
+ * power domain.
+ *
+ * Locking: Access (or update) to device's "pd_data->performance_state" field
+ * happens only with parent domain's lock held. Subdomains have their
+ * "genpd->performance_state" protected with their own lock (and they are the
+ * only user of this field) and their per-parent-domain
+ * "link->performance_state" field is protected with individual parent power
+ * domain's lock and is only accessed/updated with that lock held.
+ */
+static int genpd_update_performance_state(struct generic_pm_domain *genpd,
+ int depth)
+{
+ struct generic_pm_domain_data *pd_data;
+ struct generic_pm_domain *master;
+ struct pm_domain_data *pdd;
+ unsigned int state = 0, prev;
+ struct gpd_link *link;
+ int ret;
+
+ /* Traverse all devices within the domain */
+ list_for_each_entry(pdd, &genpd->dev_list, list_node) {
+ pd_data = to_gpd_data(pdd);
+
+ if (pd_data->performance_state > state)
+ state = pd_data->performance_state;
+ }
+
+ /* Traverse all subdomains within the domain */
+ list_for_each_entry(link, &genpd->master_links, master_node) {
+ if (link->performance_state > state)
+ state = link->performance_state;
+ }
+
+ if (genpd->performance_state == state)
+ return 0;
+
+ /*
+ * Not all domains support updating performance state. Move on to their
+ * parent domains in that case.
+ */
+ if (genpd->set_performance_state) {
+ ret = genpd->set_performance_state(genpd, state);
+ if (!ret)
+ genpd->performance_state = state;
+
+ return ret;
+ }
+
+ prev = genpd->performance_state;
+
+ list_for_each_entry(link, &genpd->slave_links, slave_node) {
+ master = link->master;
+
+ genpd_lock_nested(master, depth + 1);
+
+ link->performance_state = state;
+ ret = genpd_update_performance_state(master, depth + 1);
+ if (ret)
+ link->performance_state = prev;
+
+ genpd_unlock(master);
+
+ if (ret)
+ goto err;
+ }
+
+ /*
+ * All the parent domains are updated now, lets get genpd
+ * performance_state in sync with those.
+ */
+ genpd->performance_state = state;
+ return 0;
+
+err:
+ list_for_each_entry_continue_reverse(link, &genpd->slave_links,
+ slave_node) {
+ master = link->master;
+
+ genpd_lock_nested(master, depth + 1);
+ link->performance_state = prev;
+ if (genpd_update_performance_state(master, depth + 1))
+ pr_err("%s: Failed to roll back to %d performance
state\n",
+ genpd->name, prev);
+ genpd_unlock(master);
+ }
+
+ return ret;
+}
+
+/**
+ * pm_genpd_update_performance_state - Update performance state of parent power
+ * domain for the target frequency for the device.
+ *
+ * @dev: Device for which the performance-state needs to be adjusted.
+ * @rate: Device's next frequency.
+ */
+int pm_genpd_update_performance_state(struct device *dev, unsigned long rate)
+{
+ struct generic_pm_domain *genpd = ERR_PTR(-ENODATA);
+ struct generic_pm_domain_data *gpd_data;
+ int ret, prev;
+
+ spin_lock_irq(&dev->power.lock);
+ if (dev->power.subsys_data && dev->power.subsys_data->domain_data) {
+ gpd_data = to_gpd_data(dev->power.subsys_data->domain_data);
+ genpd = dev_to_genpd(dev);
+ }
+
+ if (IS_ERR(genpd)) {
+ ret = -ENODEV;
+ goto unlock;
+ }
+
+ if (!genpd->get_performance_state) {
+ dev_err(dev, "Performance states aren't supported by power
domain\n");
+ ret = -EINVAL;
+ goto unlock;
+ }
+
+ genpd_lock(genpd);
+ ret = genpd->get_performance_state(dev, rate);
+ if (ret < 0)
+ goto unlock_genpd;
+
+ prev = gpd_data->performance_state;
+ gpd_data->performance_state = ret;
+ ret = genpd_update_performance_state(genpd, 0);
+ if (ret)
+ gpd_data->performance_state = prev;
+
+unlock_genpd:
+ genpd_unlock(genpd);
+unlock:
+ spin_unlock_irq(&dev->power.lock);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(pm_genpd_update_performance_state);
+
/**
* genpd_power_off_work_fn - Power off PM domain whose subdomain count is 0.
* @work: Work structure used for scheduling the execution of this function.
@@ -1156,6 +1337,7 @@ static struct generic_pm_domain_data
*genpd_alloc_dev_data(struct device *dev,
gpd_data->td.constraint_changed = true;
gpd_data->td.effective_constraint_ns = -1;
gpd_data->nb.notifier_call = genpd_dev_pm_qos_notifier;
+ gpd_data->performance_state = 0;
spin_lock_irq(&dev->power.lock);
@@ -1502,6 +1684,17 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
genpd->dev_ops.start = pm_clk_resume;
}
+ /*
+ * If this genpd supports getting performance state, then someone in its
+ * hierarchy must support setting it too.
+ */
+ if (genpd->get_performance_state &&
+ !genpd_has_set_performance_state(genpd)) {
+ pr_err("%s: genpd doesn't support updating performance state\n",
+ genpd->name);
+ return -ENODEV;
+ }
+
/* Always-on domains must be powered on at initialization. */
if (genpd_is_always_on(genpd) && !genpd_status_on(genpd))
return -EINVAL;
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index b7803a251044..bf90177208a2 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -63,8 +63,12 @@ struct generic_pm_domain {
unsigned int device_count; /* Number of devices */
unsigned int suspended_count; /* System suspend device counter */
unsigned int prepared_count; /* Suspend counter of prepared devices
*/
+ unsigned int performance_state; /* Max requested performance state */
int (*power_off)(struct generic_pm_domain *domain);
int (*power_on)(struct generic_pm_domain *domain);
+ int (*get_performance_state)(struct device *dev, unsigned long rate);
+ int (*set_performance_state)(struct generic_pm_domain *domain,
+ unsigned int state);
struct gpd_dev_ops dev_ops;
s64 max_off_time_ns; /* Maximum allowed "suspended" time. */
bool max_off_time_changed;
@@ -99,6 +103,9 @@ struct gpd_link {
struct list_head master_node;
struct generic_pm_domain *slave;
struct list_head slave_node;
+
+ /* Sub-domain's per-parent domain performance state */
+ unsigned int performance_state;
};
struct gpd_timing_data {
@@ -118,6 +125,7 @@ struct generic_pm_domain_data {
struct pm_domain_data base;
struct gpd_timing_data td;
struct notifier_block nb;
+ unsigned int performance_state;
void *data;
};
@@ -148,6 +156,9 @@ extern int pm_genpd_remove(struct generic_pm_domain *genpd);
extern struct dev_power_governor simple_qos_governor;
extern struct dev_power_governor pm_domain_always_on_gov;
+extern bool pm_genpd_has_performance_state(struct device *dev);
+extern int pm_genpd_update_performance_state(struct device *dev,
+ unsigned long rate);
#else
static inline struct generic_pm_domain_data *dev_gpd_data(struct device *dev)
@@ -185,6 +196,17 @@ static inline int pm_genpd_remove(struct generic_pm_domain
*genpd)
return -ENOTSUPP;
}
+static inline bool pm_genpd_has_performance_state(struct device *dev)
+{
+ return false;
+}
+
+static inline int pm_genpd_update_performance_state(struct device *dev,
+ unsigned long rate)
+{
+ return -ENOTSUPP;
+}
+
#define simple_qos_governor (*(struct dev_power_governor *)(NULL))
#define pm_domain_always_on_gov (*(struct dev_power_governor
*)(NULL))
#endif
