[PATCH v6 1/7] powercap/drivers/dtpm: Encapsulate even more the code

[PATCH v6 1/7] powercap/drivers/dtpm: Encapsulate even more the code

[Daniel Lezcano]

In order to increase the self-encapsulation of the dtpm generic code,
the following changes are adding a power update ops to the dtpm
ops. That allows the generic code to call directly the dtpm backend
function to update the power values.

The power update function does compute the power characteristics when
the function is invoked. In the case of the CPUs, the power
consumption depends on the number of online CPUs. The online CPUs mask
is not up to date at CPUHP_AP_ONLINE_DYN state in the tear down
callback. That is the reason why the online / offline are at separate
state. As there is already an existing state for DTPM, this one is
only moved to the DEAD state, so there is no addition of new state
with these changes. The dtpm node is not removed when the cpu is
unplugged.

That simplifies the code for the next changes and results in a more
self-encapsulated code.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
---
V4:
 - Replaced s/sprintf/snprintf/ for the dtpm node name
V2:
 - Updated the changelog with the CPU node not being removed
 - Commented the cpu hotplug callbacks to explain why there are two callbacks
 - Changed 'upt_power_uw' to 'update_power_uw'
 - Removed unused cpumask variable
---
 drivers/powercap/dtpm.c     |  54 ++++++-------
 drivers/powercap/dtpm_cpu.c | 148 ++++++++++++++++--------------------
 include/linux/cpuhotplug.h  |   2 +-
 include/linux/dtpm.h        |   3 +-
 4 files changed, 97 insertions(+), 110 deletions(-)

diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c
index c2185ec5f887..58433b8ef9a1 100644
--- a/drivers/powercap/dtpm.c
+++ b/drivers/powercap/dtpm.c
@@ -116,8 +116,6 @@ static void __dtpm_sub_power(struct dtpm *dtpm)
 		parent->power_limit -= dtpm->power_limit;
 		parent = parent->parent;
 	}
-
-	__dtpm_rebalance_weight(root);
 }
 
 static void __dtpm_add_power(struct dtpm *dtpm)
@@ -130,45 +128,45 @@ static void __dtpm_add_power(struct dtpm *dtpm)
 		parent->power_limit += dtpm->power_limit;
 		parent = parent->parent;
 	}
+}
+
+static int __dtpm_update_power(struct dtpm *dtpm)
+{
+	int ret;
+
+	__dtpm_sub_power(dtpm);
 
-	__dtpm_rebalance_weight(root);
+	ret = dtpm->ops->update_power_uw(dtpm);
+	if (ret)
+		pr_err("Failed to update power for '%s': %d\n",
+		       dtpm->zone.name, ret);
+
+	if (!test_bit(DTPM_POWER_LIMIT_FLAG, &dtpm->flags))
+		dtpm->power_limit = dtpm->power_max;
+
+	__dtpm_add_power(dtpm);
+
+	if (root)
+		__dtpm_rebalance_weight(root);
+
+	return ret;
 }
 
 /**
  * dtpm_update_power - Update the power on the dtpm
  * @dtpm: a pointer to a dtpm structure to update
- * @power_min: a u64 representing the new power_min value
- * @power_max: a u64 representing the new power_max value
  *
  * Function to update the power values of the dtpm node specified in
  * parameter. These new values will be propagated to the tree.
  *
  * Return: zero on success, -EINVAL if the values are inconsistent
  */
-int dtpm_update_power(struct dtpm *dtpm, u64 power_min, u64 power_max)
+int dtpm_update_power(struct dtpm *dtpm)
 {
-	int ret = 0;
+	int ret;
 
 	mutex_lock(&dtpm_lock);
-
-	if (power_min == dtpm->power_min && power_max == dtpm->power_max)
-		goto unlock;
-
-	if (power_max < power_min) {
-		ret = -EINVAL;
-		goto unlock;
-	}
-
-	__dtpm_sub_power(dtpm);
-
-	dtpm->power_min = power_min;
-	dtpm->power_max = power_max;
-	if (!test_bit(DTPM_POWER_LIMIT_FLAG, &dtpm->flags))
-		dtpm->power_limit = power_max;
-
-	__dtpm_add_power(dtpm);
-
-unlock:
+	ret = __dtpm_update_power(dtpm);
 	mutex_unlock(&dtpm_lock);
 
 	return ret;
@@ -436,6 +434,7 @@ int dtpm_register(const char *name, struct dtpm *dtpm, struct dtpm *parent)
 
 	if (dtpm->ops && !(dtpm->ops->set_power_uw &&
 			   dtpm->ops->get_power_uw &&
+			   dtpm->ops->update_power_uw &&
 			   dtpm->ops->release))
 		return -EINVAL;
 
@@ -455,7 +454,8 @@ int dtpm_register(const char *name, struct dtpm *dtpm, struct dtpm *parent)
 		root = dtpm;
 	}
 
-	__dtpm_add_power(dtpm);
+	if (dtpm->ops && !dtpm->ops->update_power_uw(dtpm))
+		__dtpm_add_power(dtpm);
 
 	pr_info("Registered dtpm node '%s' / %llu-%llu uW, \n",
 		dtpm->zone.name, dtpm->power_min, dtpm->power_max);
diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
index 51c366938acd..f6076de39540 100644
--- a/drivers/powercap/dtpm_cpu.c
+++ b/drivers/powercap/dtpm_cpu.c
@@ -14,6 +14,8 @@
  * The CPU hotplug is supported and the power numbers will be updated
  * if a CPU is hot plugged / unplugged.
  */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/cpumask.h>
 #include <linux/cpufreq.h>
 #include <linux/cpuhotplug.h>
@@ -23,8 +25,6 @@
 #include <linux/slab.h>
 #include <linux/units.h>
 
-static struct dtpm *__parent;
-
 static DEFINE_PER_CPU(struct dtpm *, dtpm_per_cpu);
 
 struct dtpm_cpu {
@@ -32,57 +32,16 @@ struct dtpm_cpu {
 	int cpu;
 };
 
-/*
- * When a new CPU is inserted at hotplug or boot time, add the power
- * contribution and update the dtpm tree.
- */
-static int power_add(struct dtpm *dtpm, struct em_perf_domain *em)
-{
-	u64 power_min, power_max;
-
-	power_min = em->table[0].power;
-	power_min *= MICROWATT_PER_MILLIWATT;
-	power_min += dtpm->power_min;
-
-	power_max = em->table[em->nr_perf_states - 1].power;
-	power_max *= MICROWATT_PER_MILLIWATT;
-	power_max += dtpm->power_max;
-
-	return dtpm_update_power(dtpm, power_min, power_max);
-}
-
-/*
- * When a CPU is unplugged, remove its power contribution from the
- * dtpm tree.
- */
-static int power_sub(struct dtpm *dtpm, struct em_perf_domain *em)
-{
-	u64 power_min, power_max;
-
-	power_min = em->table[0].power;
-	power_min *= MICROWATT_PER_MILLIWATT;
-	power_min = dtpm->power_min - power_min;
-
-	power_max = em->table[em->nr_perf_states - 1].power;
-	power_max *= MICROWATT_PER_MILLIWATT;
-	power_max = dtpm->power_max - power_max;
-
-	return dtpm_update_power(dtpm, power_min, power_max);
-}
-
 static u64 set_pd_power_limit(struct dtpm *dtpm, u64 power_limit)
 {
 	struct dtpm_cpu *dtpm_cpu = dtpm->private;
-	struct em_perf_domain *pd;
+	struct em_perf_domain *pd = em_cpu_get(dtpm_cpu->cpu);
 	struct cpumask cpus;
 	unsigned long freq;
 	u64 power;
 	int i, nr_cpus;
 
-	pd = em_cpu_get(dtpm_cpu->cpu);
-
 	cpumask_and(&cpus, cpu_online_mask, to_cpumask(pd->cpus));
-
 	nr_cpus = cpumask_weight(&cpus);
 
 	for (i = 0; i < pd->nr_perf_states; i++) {
@@ -113,6 +72,7 @@ static u64 get_pd_power_uw(struct dtpm *dtpm)
 
 	pd = em_cpu_get(dtpm_cpu->cpu);
 	freq = cpufreq_quick_get(dtpm_cpu->cpu);
+
 	cpumask_and(&cpus, cpu_online_mask, to_cpumask(pd->cpus));
 	nr_cpus = cpumask_weight(&cpus);
 
@@ -128,6 +88,27 @@ static u64 get_pd_power_uw(struct dtpm *dtpm)
 	return 0;
 }
 
+static int update_pd_power_uw(struct dtpm *dtpm)
+{
+	struct dtpm_cpu *dtpm_cpu = dtpm->private;
+	struct em_perf_domain *em = em_cpu_get(dtpm_cpu->cpu);
+	struct cpumask cpus;
+	int nr_cpus;
+
+	cpumask_and(&cpus, cpu_online_mask, to_cpumask(em->cpus));
+	nr_cpus = cpumask_weight(&cpus);
+
+	dtpm->power_min = em->table[0].power;
+	dtpm->power_min *= MICROWATT_PER_MILLIWATT;
+	dtpm->power_min *= nr_cpus;
+
+	dtpm->power_max = em->table[em->nr_perf_states - 1].power;
+	dtpm->power_max *= MICROWATT_PER_MILLIWATT;
+	dtpm->power_max *= nr_cpus;
+
+	return 0;
+}
+
 static void pd_release(struct dtpm *dtpm)
 {
 	struct dtpm_cpu *dtpm_cpu = dtpm->private;
@@ -139,39 +120,24 @@ static void pd_release(struct dtpm *dtpm)
 }
 
 static struct dtpm_ops dtpm_ops = {
-	.set_power_uw = set_pd_power_limit,
-	.get_power_uw = get_pd_power_uw,
-	.release = pd_release,
+	.set_power_uw	 = set_pd_power_limit,
+	.get_power_uw	 = get_pd_power_uw,
+	.update_power_uw = update_pd_power_uw,
+	.release	 = pd_release,
 };
 
 static int cpuhp_dtpm_cpu_offline(unsigned int cpu)
 {
-	struct cpufreq_policy *policy;
 	struct em_perf_domain *pd;
 	struct dtpm *dtpm;
 
-	policy = cpufreq_cpu_get(cpu);
-
-	if (!policy)
-		return 0;
-
 	pd = em_cpu_get(cpu);
 	if (!pd)
 		return -EINVAL;
 
 	dtpm = per_cpu(dtpm_per_cpu, cpu);
 
-	power_sub(dtpm, pd);
-
-	if (cpumask_weight(policy->cpus) != 1)
-		return 0;
-
-	for_each_cpu(cpu, policy->related_cpus)
-		per_cpu(dtpm_per_cpu, cpu) = NULL;
-
-	dtpm_unregister(dtpm);
-
-	return 0;
+	return dtpm_update_power(dtpm);
 }
 
 static int cpuhp_dtpm_cpu_online(unsigned int cpu)
@@ -184,7 +150,6 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
 	int ret = -ENOMEM;
 
 	policy = cpufreq_cpu_get(cpu);
-
 	if (!policy)
 		return 0;
 
@@ -194,7 +159,7 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
 
 	dtpm = per_cpu(dtpm_per_cpu, cpu);
 	if (dtpm)
-		return power_add(dtpm, pd);
+		return dtpm_update_power(dtpm);
 
 	dtpm = dtpm_alloc(&dtpm_ops);
 	if (!dtpm)
@@ -210,27 +175,20 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
 	for_each_cpu(cpu, policy->related_cpus)
 		per_cpu(dtpm_per_cpu, cpu) = dtpm;
 
-	sprintf(name, "cpu%d", dtpm_cpu->cpu);
+	snprintf(name, sizeof(name), "cpu%d-cpufreq", dtpm_cpu->cpu);
 
-	ret = dtpm_register(name, dtpm, __parent);
+	ret = dtpm_register(name, dtpm, NULL);
 	if (ret)
 		goto out_kfree_dtpm_cpu;
 
-	ret = power_add(dtpm, pd);
-	if (ret)
-		goto out_dtpm_unregister;
-
 	ret = freq_qos_add_request(&policy->constraints,
 				   &dtpm_cpu->qos_req, FREQ_QOS_MAX,
 				   pd->table[pd->nr_perf_states - 1].frequency);
 	if (ret)
-		goto out_power_sub;
+		goto out_dtpm_unregister;
 
 	return 0;
 
-out_power_sub:
-	power_sub(dtpm, pd);
-
 out_dtpm_unregister:
 	dtpm_unregister(dtpm);
 	dtpm_cpu = NULL;
@@ -248,10 +206,38 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
 
 int dtpm_register_cpu(struct dtpm *parent)
 {
-	__parent = parent;
+	int ret;
+
+	/*
+	 * The callbacks at CPU hotplug time are calling
+	 * dtpm_update_power() which in turns calls update_pd_power().
+	 *
+	 * The function update_pd_power() uses the online mask to
+	 * figure out the power consumption limits.
+	 *
+	 * At CPUHP_AP_ONLINE_DYN, the CPU is present in the CPU
+	 * online mask when the cpuhp_dtpm_cpu_online function is
+	 * called, but the CPU is still in the online mask for the
+	 * tear down callback. So the power can not be updated when
+	 * the CPU is unplugged.
+	 *
+	 * At CPUHP_AP_DTPM_CPU_DEAD, the situation is the opposite as
+	 * above. The CPU online mask is not up to date when the CPU
+	 * is plugged in.
+	 *
+	 * For this reason, we need to call the online and offline
+	 * callbacks at different moments when the CPU online mask is
+	 * consistent with the power numbers we want to update.
+	 */
+	ret = cpuhp_setup_state(CPUHP_AP_DTPM_CPU_DEAD, "dtpm_cpu:offline",
+				NULL, cpuhp_dtpm_cpu_offline);
+	if (ret < 0)
+		return ret;
+
+	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "dtpm_cpu:online",
+				cpuhp_dtpm_cpu_online, NULL);
+	if (ret < 0)
+		return ret;
 
-	return cpuhp_setup_state(CPUHP_AP_DTPM_CPU_ONLINE,
-				 "dtpm_cpu:online",
-				 cpuhp_dtpm_cpu_online,
-				 cpuhp_dtpm_cpu_offline);
+	return 0;
 }
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index f14adb882338..b9c50c1b5948 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -61,6 +61,7 @@ enum cpuhp_state {
 	CPUHP_LUSTRE_CFS_DEAD,
 	CPUHP_AP_ARM_CACHE_B15_RAC_DEAD,
 	CPUHP_PADATA_DEAD,
+	CPUHP_AP_DTPM_CPU_DEAD,
 	CPUHP_WORKQUEUE_PREP,
 	CPUHP_POWER_NUMA_PREPARE,
 	CPUHP_HRTIMERS_PREPARE,
@@ -195,7 +196,6 @@ enum cpuhp_state {
 	CPUHP_AP_ONLINE_DYN_END		= CPUHP_AP_ONLINE_DYN + 30,
 	CPUHP_AP_X86_HPET_ONLINE,
 	CPUHP_AP_X86_KVM_CLK_ONLINE,
-	CPUHP_AP_DTPM_CPU_ONLINE,
 	CPUHP_AP_ACTIVE,
 	CPUHP_ONLINE,
 };
diff --git a/include/linux/dtpm.h b/include/linux/dtpm.h
index e80a332e3d8a..acf8d3638988 100644
--- a/include/linux/dtpm.h
+++ b/include/linux/dtpm.h
@@ -29,6 +29,7 @@ struct dtpm {
 struct dtpm_ops {
 	u64 (*set_power_uw)(struct dtpm *, u64);
 	u64 (*get_power_uw)(struct dtpm *);
+	int (*update_power_uw)(struct dtpm *);
 	void (*release)(struct dtpm *);
 };
 
@@ -62,7 +63,7 @@ static inline struct dtpm *to_dtpm(struct powercap_zone *zone)
 	return container_of(zone, struct dtpm, zone);
 }
 
-int dtpm_update_power(struct dtpm *dtpm, u64 power_min, u64 power_max);
+int dtpm_update_power(struct dtpm *dtpm);
 
 int dtpm_release_zone(struct powercap_zone *pcz);
 
-- 
2.17.1

[PATCH v6 2/7] powercap/drivers/dtpm: Create a registering system

[Daniel Lezcano]

A SoC can be differently structured depending on the platform and the
kernel can not be aware of all the combinations, as well as the
specific tweaks for a particular board.

The creation of the hierarchy must be delegated to userspace.

These changes provide a registering mechanism where the different
subsystems will initialize their dtpm backends and register with a
name the dtpm node in a list.

The next changes will provide an userspace interface to create
hierarchically the different nodes. Those will be created by name and
found via the list filled by the different subsystem.

If a specified name is not found in the list, it is assumed to be a
virtual node which will have children and the default is to allocate
such node.

Cc: Greg KH <gregkh@linuxfoundation.org>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
---

V6:
  - Added the EXPORT_SYMBOL_GPL
V5:
  - Decrease log level from 'info' to 'debug'
  - Remove the refcount, it is pointless, lifetime cycle is already
    handled by the device refcounting. The dtpm node allocator is in
    charge of freeing it.
  - Rename the functions to 'dtpm_add, dtpm_del, dtpm_lookup'
  - Fix missing kfrees when deleting the node from the list
V4:
  - Fixed typo in the commit log
V2:
  - Fixed error code path by dropping lock
---
 drivers/powercap/dtpm.c     | 124 ++++++++++++++++++++++++++++++++++--
 drivers/powercap/dtpm_cpu.c |   8 +--
 include/linux/dtpm.h        |   6 ++
 3 files changed, 130 insertions(+), 8 deletions(-)

diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c
index 58433b8ef9a1..a707cc2965a1 100644
--- a/drivers/powercap/dtpm.c
+++ b/drivers/powercap/dtpm.c
@@ -34,6 +34,14 @@ static DEFINE_MUTEX(dtpm_lock);
 static struct powercap_control_type *pct;
 static struct dtpm *root;
 
+struct dtpm_node {
+	const char *name;
+	struct dtpm *dtpm;
+	struct list_head node;
+};
+
+static LIST_HEAD(dtpm_list);
+
 static int get_time_window_us(struct powercap_zone *pcz, int cid, u64 *window)
 {
 	return -ENOSYS;
@@ -152,6 +160,116 @@ static int __dtpm_update_power(struct dtpm *dtpm)
 	return ret;
 }
 
+static struct dtpm *__dtpm_lookup(const char *name)
+{
+	struct dtpm_node *node;
+
+	list_for_each_entry(node, &dtpm_list, node) {
+		if (!strcmp(name, node->name))
+			return node->dtpm;
+	}
+
+	return NULL;
+}
+
+/**
+ * dtpm_lookup - Lookup for a registered dtpm node given its name
+ * @name: the name of the dtpm device
+ *
+ * The function looks up in the list of the registered dtpm
+ * devices. This function must be called to create a dtpm node in the
+ * powercap hierarchy.
+ *
+ * Return: a pointer to a dtpm structure, NULL if not found.
+ */
+struct dtpm *dtpm_lookup(const char *name)
+{
+	struct dtpm *dtpm;
+
+	mutex_lock(&dtpm_lock);
+	dtpm = __dtpm_lookup(name);
+	mutex_unlock(&dtpm_lock);
+
+	return dtpm;
+}
+EXPORT_SYMBOL_GPL(dtpm_lookup);
+
+/**
+ * dtpm_add - Add the dtpm in the dtpm list
+ * @name: a name used as an identifier
+ * @dtpm: the dtpm node to be registered
+ *
+ * Stores the dtpm device in a list. The list contains all the devices
+ * which are power capable in terms of limitation and power
+ * consumption measurements. Even if conceptually, a power capable
+ * device won't register itself twice, the function will check if it
+ * was already registered in order to prevent a misuse of the API.
+ *
+ * Return: 0 on success, -EEXIST if the device name is already present
+ * in the list, -ENOMEM in case of memory allocation failure.
+ */
+int dtpm_add(const char *name, struct dtpm *dtpm)
+{
+	struct dtpm_node *node;
+	int ret;
+
+	mutex_lock(&dtpm_lock);
+
+	ret = -EEXIST;
+	if (__dtpm_lookup(name))
+		goto out_unlock;
+
+	ret = -ENOMEM;
+	node = kzalloc(sizeof(*node), GFP_KERNEL);
+	if (!node)
+		goto out_unlock;
+
+	node->name = kstrdup(name, GFP_KERNEL);
+	if (!node->name) {
+		kfree(node);
+		goto out_unlock;
+	}
+
+	node->dtpm = dtpm;
+
+	list_add(&node->node, &dtpm_list);
+
+	ret = 0;
+out_unlock:
+	mutex_unlock(&dtpm_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dtpm_add);
+
+/**
+ * dtpm_del - Remove the dtpm device from the list
+ * @name: the dtpm device name to be removed
+ *
+ * Remove the dtpm device from the list of the registered devices.
+ */
+void dtpm_del(const char *name)
+{
+	struct dtpm_node *node;
+
+	mutex_lock(&dtpm_lock);
+
+	list_for_each_entry(node, &dtpm_list, node) {
+
+		if (strcmp(name, node->name))
+			continue;
+
+		list_del(&node->node);
+		kfree(node->name);
+		kfree(node);
+
+		break;
+	}
+
+	mutex_unlock(&dtpm_lock);
+}
+EXPORT_SYMBOL_GPL(dtpm_del);
+
 /**
  * dtpm_update_power - Update the power on the dtpm
  * @dtpm: a pointer to a dtpm structure to update
@@ -208,8 +326,6 @@ int dtpm_release_zone(struct powercap_zone *pcz)
 	if (root == dtpm)
 		root = NULL;
 
-	kfree(dtpm);
-
 	return 0;
 }
 
@@ -388,7 +504,7 @@ void dtpm_unregister(struct dtpm *dtpm)
 {
 	powercap_unregister_zone(pct, &dtpm->zone);
 
-	pr_info("Unregistered dtpm node '%s'\n", dtpm->zone.name);
+	pr_debug("Unregistered dtpm node '%s'\n", dtpm->zone.name);
 }
 
 /**
@@ -457,7 +573,7 @@ int dtpm_register(const char *name, struct dtpm *dtpm, struct dtpm *parent)
 	if (dtpm->ops && !dtpm->ops->update_power_uw(dtpm))
 		__dtpm_add_power(dtpm);
 
-	pr_info("Registered dtpm node '%s' / %llu-%llu uW, \n",
+	pr_debug("Created dtpm node '%s' / %llu-%llu uW, \n",
 		dtpm->zone.name, dtpm->power_min, dtpm->power_max);
 
 	mutex_unlock(&dtpm_lock);
diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
index f6076de39540..9deafd16a197 100644
--- a/drivers/powercap/dtpm_cpu.c
+++ b/drivers/powercap/dtpm_cpu.c
@@ -177,7 +177,7 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
 
 	snprintf(name, sizeof(name), "cpu%d-cpufreq", dtpm_cpu->cpu);
 
-	ret = dtpm_register(name, dtpm, NULL);
+	ret = dtpm_add(name, dtpm);
 	if (ret)
 		goto out_kfree_dtpm_cpu;
 
@@ -185,12 +185,12 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
 				   &dtpm_cpu->qos_req, FREQ_QOS_MAX,
 				   pd->table[pd->nr_perf_states - 1].frequency);
 	if (ret)
-		goto out_dtpm_unregister;
+		goto out_dtpm_del;
 
 	return 0;
 
-out_dtpm_unregister:
-	dtpm_unregister(dtpm);
+out_dtpm_del:
+	dtpm_del(name);
 	dtpm_cpu = NULL;
 	dtpm = NULL;
 
diff --git a/include/linux/dtpm.h b/include/linux/dtpm.h
index acf8d3638988..577c71d4e098 100644
--- a/include/linux/dtpm.h
+++ b/include/linux/dtpm.h
@@ -75,4 +75,10 @@ int dtpm_register(const char *name, struct dtpm *dtpm, struct dtpm *parent);
 
 int dtpm_register_cpu(struct dtpm *parent);
 
+struct dtpm *dtpm_lookup(const char *name);
+
+int dtpm_add(const char *name, struct dtpm *dtpm);
+
+void dtpm_del(const char *name);
+
 #endif
-- 
2.17.1

[PATCH v6 3/7] powercap/drivers/dtpm: Simplify the dtpm table

[Daniel Lezcano]

The dtpm table is an array of pointers, that forces the user of the
table to define initdata along with the declaration of the table
entry. It is more efficient to create an array of dtpm structure, so
the declaration of the table entry can be done by initializing the
different fields.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
---
 drivers/powercap/dtpm.c     |  4 ++--
 drivers/powercap/dtpm_cpu.c |  4 +++-
 include/linux/dtpm.h        | 20 +++++++++-----------
 3 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c
index a707cc2965a1..d9aac107c927 100644
--- a/drivers/powercap/dtpm.c
+++ b/drivers/powercap/dtpm.c
@@ -583,7 +583,7 @@ int dtpm_register(const char *name, struct dtpm *dtpm, struct dtpm *parent)
 
 static int __init dtpm_init(void)
 {
-	struct dtpm_descr **dtpm_descr;
+	struct dtpm_descr *dtpm_descr;
 
 	pct = powercap_register_control_type(NULL, "dtpm", NULL);
 	if (IS_ERR(pct)) {
@@ -592,7 +592,7 @@ static int __init dtpm_init(void)
 	}
 
 	for_each_dtpm_table(dtpm_descr)
-		(*dtpm_descr)->init(*dtpm_descr);
+		dtpm_descr->init();
 
 	return 0;
 }
diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
index 9deafd16a197..74b39a1082e5 100644
--- a/drivers/powercap/dtpm_cpu.c
+++ b/drivers/powercap/dtpm_cpu.c
@@ -204,7 +204,7 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
 	return ret;
 }
 
-int dtpm_register_cpu(struct dtpm *parent)
+static int __init dtpm_cpu_init(void)
 {
 	int ret;
 
@@ -241,3 +241,5 @@ int dtpm_register_cpu(struct dtpm *parent)
 
 	return 0;
 }
+
+DTPM_DECLARE(dtpm_cpu, dtpm_cpu_init);
diff --git a/include/linux/dtpm.h b/include/linux/dtpm.h
index 577c71d4e098..2ec2821111d1 100644
--- a/include/linux/dtpm.h
+++ b/include/linux/dtpm.h
@@ -33,25 +33,23 @@ struct dtpm_ops {
 	void (*release)(struct dtpm *);
 };
 
-struct dtpm_descr;
-
-typedef int (*dtpm_init_t)(struct dtpm_descr *);
+typedef int (*dtpm_init_t)(void);
 
 struct dtpm_descr {
-	struct dtpm *parent;
-	const char *name;
 	dtpm_init_t init;
 };
 
 /* Init section thermal table */
-extern struct dtpm_descr *__dtpm_table[];
-extern struct dtpm_descr *__dtpm_table_end[];
+extern struct dtpm_descr __dtpm_table[];
+extern struct dtpm_descr __dtpm_table_end[];
 
-#define DTPM_TABLE_ENTRY(name)			\
-	static typeof(name) *__dtpm_table_entry_##name	\
-	__used __section("__dtpm_table") = &name
+#define DTPM_TABLE_ENTRY(name, __init)				\
+	static struct dtpm_descr __dtpm_table_entry_##name	\
+	__used __section("__dtpm_table") = {			\
+		.init = __init,					\
+	}
 
-#define DTPM_DECLARE(name)	DTPM_TABLE_ENTRY(name)
+#define DTPM_DECLARE(name, init)	DTPM_TABLE_ENTRY(name, init)
 
 #define for_each_dtpm_table(__dtpm)	\
 	for (__dtpm = __dtpm_table;	\
-- 
2.17.1

[PATCH v6 4/7] powercap/drivers/dtpm: Use container_of instead of a private data field

[Daniel Lezcano]

The dtpm framework provides an API to allocate a dtpm node. However
when a backend dtpm driver needs to allocate a dtpm node it must
define its own structure and store the pointer of this structure in
the private field of the dtpm structure.

It is more elegant to use the container_of macro and add the dtpm
structure inside the dtpm backend specific structure. The code will be
able to deal properly with the dtpm structure as a generic entity,
making all this even more self-encapsulated.

The dtpm_alloc() function does no longer make sense as the dtpm
structure will be allocated when allocating the device specific dtpm
structure. The dtpm_init() is provided instead.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
---
 drivers/powercap/dtpm.c     | 18 +++++----------
 drivers/powercap/dtpm_cpu.c | 46 ++++++++++++++++++-------------------
 include/linux/dtpm.h        |  3 +--
 3 files changed, 29 insertions(+), 38 deletions(-)

diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c
index d9aac107c927..b389bc397cdf 100644
--- a/drivers/powercap/dtpm.c
+++ b/drivers/powercap/dtpm.c
@@ -473,24 +473,18 @@ static struct powercap_zone_ops zone_ops = {
 };
 
 /**
- * dtpm_alloc - Allocate and initialize a dtpm struct
- * @name: a string specifying the name of the node
- *
- * Return: a struct dtpm pointer, NULL in case of error
+ * dtpm_init - Allocate and initialize a dtpm struct
+ * @dtpm: The dtpm struct pointer to be initialized
+ * @ops: The dtpm device specific ops, NULL for a virtual node
  */
-struct dtpm *dtpm_alloc(struct dtpm_ops *ops)
+void dtpm_init(struct dtpm *dtpm, struct dtpm_ops *ops)
 {
-	struct dtpm *dtpm;
-
-	dtpm = kzalloc(sizeof(*dtpm), GFP_KERNEL);
 	if (dtpm) {
 		INIT_LIST_HEAD(&dtpm->children);
 		INIT_LIST_HEAD(&dtpm->sibling);
 		dtpm->weight = 1024;
 		dtpm->ops = ops;
 	}
-
-	return dtpm;
 }
 
 /**
@@ -581,7 +575,7 @@ int dtpm_register(const char *name, struct dtpm *dtpm, struct dtpm *parent)
 	return 0;
 }
 
-static int __init dtpm_init(void)
+static int __init init_dtpm(void)
 {
 	struct dtpm_descr *dtpm_descr;
 
@@ -596,4 +590,4 @@ static int __init dtpm_init(void)
 
 	return 0;
 }
-late_initcall(dtpm_init);
+late_initcall(init_dtpm);
diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
index 74b39a1082e5..f4092d1b01d7 100644
--- a/drivers/powercap/dtpm_cpu.c
+++ b/drivers/powercap/dtpm_cpu.c
@@ -25,16 +25,22 @@
 #include <linux/slab.h>
 #include <linux/units.h>
 
-static DEFINE_PER_CPU(struct dtpm *, dtpm_per_cpu);
-
 struct dtpm_cpu {
+	struct dtpm dtpm;
 	struct freq_qos_request qos_req;
 	int cpu;
 };
 
+static DEFINE_PER_CPU(struct dtpm_cpu *, dtpm_per_cpu);
+
+static struct dtpm_cpu *to_dtpm_cpu(struct dtpm *dtpm)
+{
+	return container_of(dtpm, struct dtpm_cpu, dtpm);
+}
+
 static u64 set_pd_power_limit(struct dtpm *dtpm, u64 power_limit)
 {
-	struct dtpm_cpu *dtpm_cpu = dtpm->private;
+	struct dtpm_cpu *dtpm_cpu = to_dtpm_cpu(dtpm);
 	struct em_perf_domain *pd = em_cpu_get(dtpm_cpu->cpu);
 	struct cpumask cpus;
 	unsigned long freq;
@@ -64,7 +70,7 @@ static u64 set_pd_power_limit(struct dtpm *dtpm, u64 power_limit)
 
 static u64 get_pd_power_uw(struct dtpm *dtpm)
 {
-	struct dtpm_cpu *dtpm_cpu = dtpm->private;
+	struct dtpm_cpu *dtpm_cpu = to_dtpm_cpu(dtpm);
 	struct em_perf_domain *pd;
 	struct cpumask cpus;
 	unsigned long freq;
@@ -90,7 +96,7 @@ static u64 get_pd_power_uw(struct dtpm *dtpm)
 
 static int update_pd_power_uw(struct dtpm *dtpm)
 {
-	struct dtpm_cpu *dtpm_cpu = dtpm->private;
+	struct dtpm_cpu *dtpm_cpu = to_dtpm_cpu(dtpm);
 	struct em_perf_domain *em = em_cpu_get(dtpm_cpu->cpu);
 	struct cpumask cpus;
 	int nr_cpus;
@@ -111,7 +117,7 @@ static int update_pd_power_uw(struct dtpm *dtpm)
 
 static void pd_release(struct dtpm *dtpm)
 {
-	struct dtpm_cpu *dtpm_cpu = dtpm->private;
+	struct dtpm_cpu *dtpm_cpu = to_dtpm_cpu(dtpm);
 
 	if (freq_qos_request_active(&dtpm_cpu->qos_req))
 		freq_qos_remove_request(&dtpm_cpu->qos_req);
@@ -129,20 +135,19 @@ static struct dtpm_ops dtpm_ops = {
 static int cpuhp_dtpm_cpu_offline(unsigned int cpu)
 {
 	struct em_perf_domain *pd;
-	struct dtpm *dtpm;
+	struct dtpm_cpu *dtpm_cpu;
 
 	pd = em_cpu_get(cpu);
 	if (!pd)
 		return -EINVAL;
 
-	dtpm = per_cpu(dtpm_per_cpu, cpu);
+	dtpm_cpu = per_cpu(dtpm_per_cpu, cpu);
 
-	return dtpm_update_power(dtpm);
+	return dtpm_update_power(&dtpm_cpu->dtpm);
 }
 
 static int cpuhp_dtpm_cpu_online(unsigned int cpu)
 {
-	struct dtpm *dtpm;
 	struct dtpm_cpu *dtpm_cpu;
 	struct cpufreq_policy *policy;
 	struct em_perf_domain *pd;
@@ -157,27 +162,23 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
 	if (!pd)
 		return -EINVAL;
 
-	dtpm = per_cpu(dtpm_per_cpu, cpu);
-	if (dtpm)
-		return dtpm_update_power(dtpm);
-
-	dtpm = dtpm_alloc(&dtpm_ops);
-	if (!dtpm)
-		return -EINVAL;
+	dtpm_cpu = per_cpu(dtpm_per_cpu, cpu);
+	if (dtpm_cpu)
+		return dtpm_update_power(&dtpm_cpu->dtpm);
 
 	dtpm_cpu = kzalloc(sizeof(*dtpm_cpu), GFP_KERNEL);
 	if (!dtpm_cpu)
-		goto out_kfree_dtpm;
+		return -ENOMEM;
 
-	dtpm->private = dtpm_cpu;
+	dtpm_init(&dtpm_cpu->dtpm, &dtpm_ops);
 	dtpm_cpu->cpu = cpu;
 
 	for_each_cpu(cpu, policy->related_cpus)
-		per_cpu(dtpm_per_cpu, cpu) = dtpm;
+		per_cpu(dtpm_per_cpu, cpu) = dtpm_cpu;
 
 	snprintf(name, sizeof(name), "cpu%d-cpufreq", dtpm_cpu->cpu);
 
-	ret = dtpm_add(name, dtpm);
+	ret = dtpm_add(name, &dtpm_cpu->dtpm);
 	if (ret)
 		goto out_kfree_dtpm_cpu;
 
@@ -192,15 +193,12 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
 out_dtpm_del:
 	dtpm_del(name);
 	dtpm_cpu = NULL;
-	dtpm = NULL;
 
 out_kfree_dtpm_cpu:
 	for_each_cpu(cpu, policy->related_cpus)
 		per_cpu(dtpm_per_cpu, cpu) = NULL;
 	kfree(dtpm_cpu);
 
-out_kfree_dtpm:
-	kfree(dtpm);
 	return ret;
 }
 
diff --git a/include/linux/dtpm.h b/include/linux/dtpm.h
index 2ec2821111d1..e47bd5bbf56e 100644
--- a/include/linux/dtpm.h
+++ b/include/linux/dtpm.h
@@ -23,7 +23,6 @@ struct dtpm {
 	u64 power_max;
 	u64 power_min;
 	int weight;
-	void *private;
 };
 
 struct dtpm_ops {
@@ -65,7 +64,7 @@ int dtpm_update_power(struct dtpm *dtpm);
 
 int dtpm_release_zone(struct powercap_zone *pcz);
 
-struct dtpm *dtpm_alloc(struct dtpm_ops *ops);
+void dtpm_init(struct dtpm *dtpm, struct dtpm_ops *ops);
 
 void dtpm_unregister(struct dtpm *dtpm);
 
-- 
2.17.1

[PATCH v6 5/7] powercap/drivers/dtpm: Scale the power with the load

[Daniel Lezcano]

Currently the power consumption is based on the current OPP power
assuming the entire performance domain is fully loaded.

That gives very gross power estimation and we can do much better by
using the load to scale the power consumption.

Use the utilization to normalize and scale the power usage over the
max possible power.

Tested on a rock960 with 2 big CPUS, the power consumption estimation
conforms with the expected one.

Before this change:

~$ ~/dhrystone -t 1 -l 10000&
~$ cat /sys/devices/virtual/powercap/dtpm/dtpm:0/dtpm:0:1/constraint_0_max_power_uw
2260000

After this change:

~$ ~/dhrystone -t 1 -l 10000&
~$ cat /sys/devices/virtual/powercap/dtpm/dtpm:0/dtpm:0:1/constraint_0_max_power_uw
1130000

~$ ~/dhrystone -t 2 -l 10000&
~$ cat /sys/devices/virtual/powercap/dtpm/dtpm:0/dtpm:0:1/constraint_0_max_power_uw
2260000

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
---

V3:
  - Fixed uninitialized 'cpu' in scaled_power_uw()
V2:
  - Replaced cpumask by em_span_cpus
  - Changed 'util' metrics variable types
  - Optimized utilization scaling power computation
  - Renamed parameter name for scale_pd_power_uw()
---
 drivers/powercap/dtpm_cpu.c | 46 +++++++++++++++++++++++++++++++------
 1 file changed, 39 insertions(+), 7 deletions(-)

diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
index f4092d1b01d7..eae35ae3c42e 100644
--- a/drivers/powercap/dtpm_cpu.c
+++ b/drivers/powercap/dtpm_cpu.c
@@ -68,27 +68,59 @@ static u64 set_pd_power_limit(struct dtpm *dtpm, u64 power_limit)
 	return power_limit;
 }
 
+static u64 scale_pd_power_uw(struct cpumask *pd_mask, u64 power)
+{
+	unsigned long max = 0, sum_util = 0;
+	int cpu;
+
+	for_each_cpu_and(cpu, pd_mask, cpu_online_mask) {
+
+		/*
+		 * The capacity is the same for all CPUs belonging to
+		 * the same perf domain, so a single call to
+		 * arch_scale_cpu_capacity() is enough. However, we
+		 * need the CPU parameter to be initialized by the
+		 * loop, so the call ends up in this block.
+		 *
+		 * We can initialize 'max' with a cpumask_first() call
+		 * before the loop but the bits computation is not
+		 * worth given the arch_scale_cpu_capacity() just
+		 * returns a value where the resulting assembly code
+		 * will be optimized by the compiler.
+		 */
+		max = arch_scale_cpu_capacity(cpu);
+		sum_util += sched_cpu_util(cpu, max);
+	}
+
+	/*
+	 * In the improbable case where all the CPUs of the perf
+	 * domain are offline, 'max' will be zero and will lead to an
+	 * illegal operation with a zero division.
+	 */
+	return max ? (power * ((sum_util << 10) / max)) >> 10 : 0;
+}
+
 static u64 get_pd_power_uw(struct dtpm *dtpm)
 {
 	struct dtpm_cpu *dtpm_cpu = to_dtpm_cpu(dtpm);
 	struct em_perf_domain *pd;
-	struct cpumask cpus;
+	struct cpumask *pd_mask;
 	unsigned long freq;
-	int i, nr_cpus;
+	int i;
 
 	pd = em_cpu_get(dtpm_cpu->cpu);
-	freq = cpufreq_quick_get(dtpm_cpu->cpu);
 
-	cpumask_and(&cpus, cpu_online_mask, to_cpumask(pd->cpus));
-	nr_cpus = cpumask_weight(&cpus);
+	pd_mask = em_span_cpus(pd);
+
+	freq = cpufreq_quick_get(dtpm_cpu->cpu);
 
 	for (i = 0; i < pd->nr_perf_states; i++) {
 
 		if (pd->table[i].frequency < freq)
 			continue;
 
-		return pd->table[i].power *
-			MICROWATT_PER_MILLIWATT * nr_cpus;
+		return scale_pd_power_uw(pd_mask, pd->table[i].power *
+					 MICROWATT_PER_MILLIWATT);
 	}
 
 	return 0;
-- 
2.17.1

[PATCH v6 6/7] powercap/drivers/dtpm: Export the symbols for the modules

[Daniel Lezcano]

The DTPM framework provides a generic API to register devices which
power capable. The devices may be compiled as modules while the
framework is not.

Export the necessary API to let the drivers register themselves.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/powercap/dtpm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c
index b389bc397cdf..733567bbe0be 100644
--- a/drivers/powercap/dtpm.c
+++ b/drivers/powercap/dtpm.c
@@ -289,6 +289,7 @@ int dtpm_update_power(struct dtpm *dtpm)
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(dtpm_update_power);
 
 /**
  * dtpm_release_zone - Cleanup when the node is released
@@ -486,6 +487,7 @@ void dtpm_init(struct dtpm *dtpm, struct dtpm_ops *ops)
 		dtpm->ops = ops;
 	}
 }
+EXPORT_SYMBOL_GPL(dtpm_init);
 
 /**
  * dtpm_unregister - Unregister a dtpm node from the hierarchy tree
@@ -500,6 +502,7 @@ void dtpm_unregister(struct dtpm *dtpm)
 
 	pr_debug("Unregistered dtpm node '%s'\n", dtpm->zone.name);
 }
+EXPORT_SYMBOL_GPL(dtpm_unregister);
 
 /**
  * dtpm_register - Register a dtpm node in the hierarchy tree
@@ -574,6 +577,7 @@ int dtpm_register(const char *name, struct dtpm *dtpm, struct dtpm *parent)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(dtpm_register);
 
 static int __init init_dtpm(void)
 {
-- 
2.17.1

[PATCH v6 7/7] powercap/drivers/dtpm: Allow dtpm node device creation through configfs

[Daniel Lezcano]

The DTPM framework is built on top of the powercap framework as a new
controller to act on the power of the devices. The approach is to
provide an unified API to do power limitation on devices which are
capable of that with different techniques.

In addition, the DTPM framework allows to create a hierarchical
representation of the devices in order to reflect the dependencies of
the power constraints we want to apply to a group of devices.

The hierarchy can be different for the same platform as it will depend
on the form factor (tablet, notebook, phone, ...), on other components
and/or a policy, and application scenario.

The kernel can not have such knowledge and only the SoC vendor can
setup its platform to fulfill the objectives of its hardware.

This patch adds the ability to create a DTPM hierarchy via
configfs. All DTPM capable devices are registered in a list, and the
creation of a configfs directory with the name of one of this device
will create a DTPM node in the DTPM powercap sysfs. If the name is not
in the list, a virtual node will be created instead. This virtual node
in the DTPM semantic is to aggregate the power characteristics of the
children.

In order to do the connection between the configfs and sysfs easily, a
'device' file contains the path to the corresponding DTPM powercap
node in sysfs (cross filesystems symlink is not supported by
configfs).

In order to not block any new features in the future, the hierarchical
constraints are stored under a top folder 'constraints', so sibling
can be created for other purposes if needed.

When the configfs was populated, the module can not be unloaded until
all the elements in the tree have been removed.

1) Resulting creation via mkdir:

root@rock960:/sys/kernel/config# tree dtpm/
dtpm/
└── constraints
    └── platform
            ├── device
	    └── soc
	        ├── device
		└── pkg
		     ├── device
		     ├── cpu0-cpufreq
		     │   └── device
		     ├── cpu4-cpufreq
		     │   └── device
		     └── panfrost-devfreq
		            └── device

2) The content of the 'device' file above

root@rock960:/sys/kernel/config# find dtpm/constraints -name "device" -exec cat {} \;
/devices/virtual/powercap/dtpm/dtpm:0/dtpm:0:0/dtpm:0:0:1
/devices/virtual/powercap/dtpm/dtpm:0/dtpm:0:0/dtpm:0:0:0/dtpm:0:0:0:2
/devices/virtual/powercap/dtpm/dtpm:0/dtpm:0:0/dtpm:0:0:0/dtpm:0:0:0:1
/devices/virtual/powercap/dtpm/dtpm:0/dtpm:0:0/dtpm:0:0:0/dtpm:0:0:0:0
/devices/virtual/powercap/dtpm/dtpm:0/dtpm:0:0/dtpm:0:0:0
/devices/virtual/powercap/dtpm/dtpm:0/dtpm:0:0
/devices/virtual/powercap/dtpm/dtpm:0

3) The dtpm device creation node is reflected in sysfs:

root@rock960:/sys/devices/virtual/powercap/dtpm# find . -type d | grep dtpm
./dtpm:0
./dtpm:0/power
./dtpm:0/dtpm:0:0
./dtpm:0/dtpm:0:0/power
./dtpm:0/dtpm:0:0/dtpm:0:0:0
./dtpm:0/dtpm:0:0/dtpm:0:0:0/dtpm:0:0:0:1
./dtpm:0/dtpm:0:0/dtpm:0:0:0/dtpm:0:0:0:1/power
./dtpm:0/dtpm:0:0/dtpm:0:0:0/power
./dtpm:0/dtpm:0:0/dtpm:0:0:0/dtpm:0:0:0:2
./dtpm:0/dtpm:0:0/dtpm:0:0:0/dtpm:0:0:0:2/power
./dtpm:0/dtpm:0:0/dtpm:0:0:0/dtpm:0:0:0:0
./dtpm:0/dtpm:0:0/dtpm:0:0:0/dtpm:0:0:0:0/power
./dtpm:0/dtpm:0:0/dtpm:0:0:1
./dtpm:0/dtpm:0:0/dtpm:0:0:1/power

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/powercap/Kconfig         |   8 ++
 drivers/powercap/Makefile        |   1 +
 drivers/powercap/dtpm_configfs.c | 202 +++++++++++++++++++++++++++++++
 include/linux/dtpm.h             |   2 +
 4 files changed, 213 insertions(+)
 create mode 100644 drivers/powercap/dtpm_configfs.c

diff --git a/drivers/powercap/Kconfig b/drivers/powercap/Kconfig
index 8242e8c5ed77..599b41e4e0a7 100644
--- a/drivers/powercap/Kconfig
+++ b/drivers/powercap/Kconfig
@@ -50,6 +50,14 @@ config DTPM
 	  This enables support for the power capping for the dynamic
 	  thermal power management userspace engine.
 
+config DTPM_CONFIGFS
+	tristate "Dynamic Thermal Power Management configuration via configfs"
+	depends on DTPM && CONFIGFS_FS
+	help
+	  This enables support for creating the DTPM device hierarchy
+	  via configfs giving the userspace full control of the
+	  thermal constraint representation.
+
 config DTPM_CPU
 	bool "Add CPU power capping based on the energy model"
 	depends on DTPM && ENERGY_MODEL
diff --git a/drivers/powercap/Makefile b/drivers/powercap/Makefile
index fabcf388a8d3..519cabc624c3 100644
--- a/drivers/powercap/Makefile
+++ b/drivers/powercap/Makefile
@@ -1,5 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0-only
 obj-$(CONFIG_DTPM) += dtpm.o
+obj-$(CONFIG_DTPM_CONFIGFS) += dtpm_configfs.o
 obj-$(CONFIG_DTPM_CPU) += dtpm_cpu.o
 obj-$(CONFIG_POWERCAP)	+= powercap_sys.o
 obj-$(CONFIG_INTEL_RAPL_CORE) += intel_rapl_common.o
diff --git a/drivers/powercap/dtpm_configfs.c b/drivers/powercap/dtpm_configfs.c
new file mode 100644
index 000000000000..b8de71e94fc3
--- /dev/null
+++ b/drivers/powercap/dtpm_configfs.c
@@ -0,0 +1,202 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2021 Linaro Limited
+ *
+ * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
+ *
+ * The DTPM framework defines a set of devices which are power capable.
+ *
+ * The configfs allows to create a hierarchy of devices in order
+ * to reflect the constraints we want to apply to them.
+ *
+ * Each dtpm node is created via a mkdir operation in the configfs
+ * directory. It will create the corresponding dtpm device in the
+ * sysfs and the 'device' will contain the absolute path to the dtpm
+ * node in the sysfs, thus allowing to do the connection between the
+ * created dtpm node in the configfs hierarchy and the dtpm node in
+ * the powercap framework.
+ *
+ * The dtpm nodes can be real or virtual. The former is a real device
+ * where acting on its power is possible and is registered in a dtpm
+ * framework's list with an unique name. A creation with mkdir with
+ * one of the registered name will instanciate the dtpm device. If the
+ * name is not in the registered list, it will create a virtual node
+ * where its purpose is to aggregate the power characteristics of its
+ * children which can virtual or real.
+ *
+ * It is not allowed to create a node if another one in the hierarchy
+ * has the same name. That ensures the consistency and prevents
+ * multiple instanciation of the same dtpm device.
+ */
+#include <linux/dtpm.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/configfs.h>
+
+static struct config_group *cstrn_group;
+
+static struct config_item_type dtpm_cstrn_type;
+
+static const struct config_item_type dtpm_root_group_type = {
+	.ct_owner = THIS_MODULE,
+};
+
+static struct configfs_subsystem dtpm_subsys = {
+	.su_group = {
+		.cg_item = {
+			.ci_namebuf = "dtpm",
+			.ci_type = &dtpm_root_group_type,
+		},
+	},
+	.su_mutex = __MUTEX_INITIALIZER(dtpm_subsys.su_mutex),
+};
+
+static bool dtpm_configfs_exists(struct config_group *grp, const char *name)
+{
+	struct list_head *entry;
+
+	list_for_each(entry, &grp->cg_children) {
+		struct config_item *item =
+			container_of(entry, struct config_item, ci_entry);
+
+		if (config_item_name(item) &&
+		    !strcmp(config_item_name(item), name))
+			return true;
+
+		if (dtpm_configfs_exists(to_config_group(item), name))
+			return true;
+	}
+
+	return false;
+}
+
+static struct config_group *dtpm_cstrn_make_group(struct config_group *grp, const char *name)
+{
+	struct dtpm *d, *p;
+	int ret;
+
+	if (dtpm_configfs_exists(cstrn_group, name))
+		return ERR_PTR(-EEXIST);
+
+	d = dtpm_lookup(name);
+	if (!d) {
+		d = kzalloc(sizeof(*d), GFP_KERNEL);
+		if (!d)
+			return ERR_PTR(-ENOMEM);
+		dtpm_init(d, NULL);
+	}
+
+	config_group_init_type_name(&d->cfg, name, &dtpm_cstrn_type);
+
+	/*
+	 * Retrieve the dtpm parent node. The first dtpm node in the
+	 * hierarchy constraint is the root node, thus it does not
+	 * have a parent.
+	 */
+	p = (grp == cstrn_group) ? NULL :
+		container_of(grp, struct dtpm, cfg);
+
+	ret = dtpm_register(name, d, p);
+	if (ret)
+		goto dtpm_free;
+
+	if (!try_module_get(THIS_MODULE)) {
+		ret = -ENODEV;
+		goto dtpm_unregister;
+	}
+
+	return &d->cfg;
+
+dtpm_unregister:
+	dtpm_unregister(d);
+dtpm_free:
+	if (!d->ops)
+		kfree(d);
+
+	return ERR_PTR(ret);
+}
+
+static void dtpm_cstrn_drop_group(struct config_group *grp,
+				  struct config_item *cfg)
+{
+	struct config_group *cg = to_config_group(cfg);
+	struct dtpm *d = container_of(cg, struct dtpm, cfg);
+
+	dtpm_unregister(d);
+	if (!d->ops)
+		kfree(d);
+	module_put(THIS_MODULE);
+	config_item_put(cfg);
+}
+
+static struct configfs_group_operations dtpm_cstrn_group_ops = {
+	.make_group = dtpm_cstrn_make_group,
+	.drop_item = dtpm_cstrn_drop_group,
+};
+
+static ssize_t dtpm_cstrn_device_show(struct config_item *cfg, char *str)
+{
+	struct config_group *cg = to_config_group(cfg);
+	struct dtpm *d = container_of(cg, struct dtpm, cfg);
+	struct kobject *kobj = &d->zone.dev.kobj;
+	char *string = kobject_get_path(kobj, GFP_KERNEL);
+	ssize_t len;
+
+	if (!string)
+		return -EINVAL;
+
+	len = sprintf(str, "%s\n", string);
+
+	kfree(string);
+
+	return len;
+}
+
+CONFIGFS_ATTR_RO(dtpm_cstrn_, device);
+
+static struct configfs_attribute *dtpm_cstrn_attrs[] = {
+	&dtpm_cstrn_attr_device,
+	NULL
+};
+
+static struct config_item_type dtpm_cstrn_type = {
+	.ct_owner = THIS_MODULE,
+	.ct_group_ops = &dtpm_cstrn_group_ops,
+};
+
+static int __init dtpm_configfs_init(void)
+{
+	int ret;
+
+	config_group_init(&dtpm_subsys.su_group);
+
+	ret = configfs_register_subsystem(&dtpm_subsys);
+	if (ret)
+		return ret;
+
+	cstrn_group = configfs_register_default_group(&dtpm_subsys.su_group,
+						      "constraints",
+						      &dtpm_cstrn_type);
+
+	/*
+	 * The default group does not contain attributes but the other
+	 * group will
+	 */
+	dtpm_cstrn_type.ct_attrs = dtpm_cstrn_attrs;
+
+	return PTR_ERR_OR_ZERO(cstrn_group);
+}
+module_init(dtpm_configfs_init);
+
+static void __exit dtpm_configfs_exit(void)
+{
+	configfs_unregister_default_group(cstrn_group);
+	configfs_unregister_subsystem(&dtpm_subsys);
+}
+module_exit(dtpm_configfs_exit);
+
+MODULE_DESCRIPTION("DTPM configuration driver");
+MODULE_AUTHOR("Daniel Lezcano");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/dtpm.h b/include/linux/dtpm.h
index e47bd5bbf56e..d7bbb9fd97eb 100644
--- a/include/linux/dtpm.h
+++ b/include/linux/dtpm.h
@@ -8,11 +8,13 @@
 #define ___DTPM_H__
 
 #include <linux/powercap.h>
+#include <linux/configfs.h>
 
 #define MAX_DTPM_DESCR 8
 #define MAX_DTPM_CONSTRAINTS 1
 
 struct dtpm {
+	struct config_group cfg;
 	struct powercap_zone zone;
 	struct dtpm *parent;
 	struct list_head sibling;
-- 
2.17.1

Re: [PATCH v6 2/7] powercap/drivers/dtpm: Create a registering system

[Greg KH]

On Thu, Apr 01, 2021 at 08:36:49PM +0200, Daniel Lezcano wrote:
> A SoC can be differently structured depending on the platform and the
> kernel can not be aware of all the combinations, as well as the
> specific tweaks for a particular board.
> 
> The creation of the hierarchy must be delegated to userspace.

Why?  Isn't this what DT is for?

What "userspace tool" is going to be created to manage all of this?
Pointers to that codebase?

> These changes provide a registering mechanism where the different
> subsystems will initialize their dtpm backends and register with a
> name the dtpm node in a list.
> 
> The next changes will provide an userspace interface to create
> hierarchically the different nodes. Those will be created by name and
> found via the list filled by the different subsystem.
> 
> If a specified name is not found in the list, it is assumed to be a
> virtual node which will have children and the default is to allocate
> such node.

So userspace sets the name?

Why not use the name in the device itself?  I thought I asked that last
time...

thanks,

greg k-h

Re: [PATCH v6 7/7] powercap/drivers/dtpm: Allow dtpm node device creation through configfs

[Greg KH]

On Thu, Apr 01, 2021 at 08:36:54PM +0200, Daniel Lezcano wrote:
> The DTPM framework is built on top of the powercap framework as a new
> controller to act on the power of the devices. The approach is to
> provide an unified API to do power limitation on devices which are
> capable of that with different techniques.
> 
> In addition, the DTPM framework allows to create a hierarchical
> representation of the devices in order to reflect the dependencies of
> the power constraints we want to apply to a group of devices.
> 
> The hierarchy can be different for the same platform as it will depend
> on the form factor (tablet, notebook, phone, ...), on other components
> and/or a policy, and application scenario.
> 
> The kernel can not have such knowledge and only the SoC vendor can
> setup its platform to fulfill the objectives of its hardware.
> 
> This patch adds the ability to create a DTPM hierarchy via
> configfs. All DTPM capable devices are registered in a list, and the
> creation of a configfs directory with the name of one of this device
> will create a DTPM node in the DTPM powercap sysfs. If the name is not
> in the list, a virtual node will be created instead. This virtual node
> in the DTPM semantic is to aggregate the power characteristics of the
> children.
> 
> In order to do the connection between the configfs and sysfs easily, a
> 'device' file contains the path to the corresponding DTPM powercap
> node in sysfs (cross filesystems symlink is not supported by
> configfs).
> 
> In order to not block any new features in the future, the hierarchical
> constraints are stored under a top folder 'constraints', so sibling
> can be created for other purposes if needed.
> 
> When the configfs was populated, the module can not be unloaded until
> all the elements in the tree have been removed.
> 
> 1) Resulting creation via mkdir:
> 
> root@rock960:/sys/kernel/config# tree dtpm/
> dtpm/
> └── constraints
>     └── platform
>             ├── device
> 	    └── soc
> 	        ├── device
> 		└── pkg
> 		     ├── device
> 		     ├── cpu0-cpufreq
> 		     │   └── device
> 		     ├── cpu4-cpufreq
> 		     │   └── device
> 		     └── panfrost-devfreq
> 		            └── device
> 
> 2) The content of the 'device' file above
> 
> root@rock960:/sys/kernel/config# find dtpm/constraints -name "device" -exec cat {} \;
> /devices/virtual/powercap/dtpm/dtpm:0/dtpm:0:0/dtpm:0:0:1
> /devices/virtual/powercap/dtpm/dtpm:0/dtpm:0:0/dtpm:0:0:0/dtpm:0:0:0:2
> /devices/virtual/powercap/dtpm/dtpm:0/dtpm:0:0/dtpm:0:0:0/dtpm:0:0:0:1
> /devices/virtual/powercap/dtpm/dtpm:0/dtpm:0:0/dtpm:0:0:0/dtpm:0:0:0:0
> /devices/virtual/powercap/dtpm/dtpm:0/dtpm:0:0/dtpm:0:0:0
> /devices/virtual/powercap/dtpm/dtpm:0/dtpm:0:0
> /devices/virtual/powercap/dtpm/dtpm:0
> 
> 3) The dtpm device creation node is reflected in sysfs:
> 
> root@rock960:/sys/devices/virtual/powercap/dtpm# find . -type d | grep dtpm
> ./dtpm:0
> ./dtpm:0/power
> ./dtpm:0/dtpm:0:0
> ./dtpm:0/dtpm:0:0/power
> ./dtpm:0/dtpm:0:0/dtpm:0:0:0
> ./dtpm:0/dtpm:0:0/dtpm:0:0:0/dtpm:0:0:0:1
> ./dtpm:0/dtpm:0:0/dtpm:0:0:0/dtpm:0:0:0:1/power
> ./dtpm:0/dtpm:0:0/dtpm:0:0:0/power
> ./dtpm:0/dtpm:0:0/dtpm:0:0:0/dtpm:0:0:0:2
> ./dtpm:0/dtpm:0:0/dtpm:0:0:0/dtpm:0:0:0:2/power
> ./dtpm:0/dtpm:0:0/dtpm:0:0:0/dtpm:0:0:0:0
> ./dtpm:0/dtpm:0:0/dtpm:0:0:0/dtpm:0:0:0:0/power
> ./dtpm:0/dtpm:0:0/dtpm:0:0:1
> ./dtpm:0/dtpm:0:0/dtpm:0:0:1/power

Why have you not documented all of this in Documentation/ABI so that we
can find it later?


> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/powercap/Kconfig         |   8 ++
>  drivers/powercap/Makefile        |   1 +
>  drivers/powercap/dtpm_configfs.c | 202 +++++++++++++++++++++++++++++++
>  include/linux/dtpm.h             |   2 +
>  4 files changed, 213 insertions(+)
>  create mode 100644 drivers/powercap/dtpm_configfs.c
> 
> diff --git a/drivers/powercap/Kconfig b/drivers/powercap/Kconfig
> index 8242e8c5ed77..599b41e4e0a7 100644
> --- a/drivers/powercap/Kconfig
> +++ b/drivers/powercap/Kconfig
> @@ -50,6 +50,14 @@ config DTPM
>  	  This enables support for the power capping for the dynamic
>  	  thermal power management userspace engine.
>  
> +config DTPM_CONFIGFS
> +	tristate "Dynamic Thermal Power Management configuration via configfs"
> +	depends on DTPM && CONFIGFS_FS
> +	help
> +	  This enables support for creating the DTPM device hierarchy
> +	  via configfs giving the userspace full control of the
> +	  thermal constraint representation.

Why does this have to be a module, shouldn't it just be part of the DTPM
code itself?

> +
>  config DTPM_CPU
>  	bool "Add CPU power capping based on the energy model"
>  	depends on DTPM && ENERGY_MODEL
> diff --git a/drivers/powercap/Makefile b/drivers/powercap/Makefile
> index fabcf388a8d3..519cabc624c3 100644
> --- a/drivers/powercap/Makefile
> +++ b/drivers/powercap/Makefile
> @@ -1,5 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  obj-$(CONFIG_DTPM) += dtpm.o
> +obj-$(CONFIG_DTPM_CONFIGFS) += dtpm_configfs.o
>  obj-$(CONFIG_DTPM_CPU) += dtpm_cpu.o
>  obj-$(CONFIG_POWERCAP)	+= powercap_sys.o
>  obj-$(CONFIG_INTEL_RAPL_CORE) += intel_rapl_common.o
> diff --git a/drivers/powercap/dtpm_configfs.c b/drivers/powercap/dtpm_configfs.c
> new file mode 100644
> index 000000000000..b8de71e94fc3
> --- /dev/null
> +++ b/drivers/powercap/dtpm_configfs.c
> @@ -0,0 +1,202 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2021 Linaro Limited
> + *
> + * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
> + *
> + * The DTPM framework defines a set of devices which are power capable.
> + *
> + * The configfs allows to create a hierarchy of devices in order
> + * to reflect the constraints we want to apply to them.
> + *
> + * Each dtpm node is created via a mkdir operation in the configfs
> + * directory. It will create the corresponding dtpm device in the
> + * sysfs and the 'device' will contain the absolute path to the dtpm
> + * node in the sysfs, thus allowing to do the connection between the
> + * created dtpm node in the configfs hierarchy and the dtpm node in
> + * the powercap framework.

Tieing configfs and sysfs is very odd, and ripe for problems.  Those are
independant filesystems with independant devices and paths and object
lifetimes.  How can you guarantee they all work together ok?

> + *
> + * The dtpm nodes can be real or virtual. The former is a real device
> + * where acting on its power is possible and is registered in a dtpm
> + * framework's list with an unique name. A creation with mkdir with
> + * one of the registered name will instanciate the dtpm device. If the
> + * name is not in the registered list, it will create a virtual node
> + * where its purpose is to aggregate the power characteristics of its
> + * children which can virtual or real.
> + *
> + * It is not allowed to create a node if another one in the hierarchy
> + * has the same name. That ensures the consistency and prevents
> + * multiple instanciation of the same dtpm device.
> + */
> +#include <linux/dtpm.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/configfs.h>
> +
> +static struct config_group *cstrn_group;
> +
> +static struct config_item_type dtpm_cstrn_type;
> +
> +static const struct config_item_type dtpm_root_group_type = {
> +	.ct_owner = THIS_MODULE,
> +};
> +
> +static struct configfs_subsystem dtpm_subsys = {
> +	.su_group = {
> +		.cg_item = {
> +			.ci_namebuf = "dtpm",
> +			.ci_type = &dtpm_root_group_type,
> +		},
> +	},
> +	.su_mutex = __MUTEX_INITIALIZER(dtpm_subsys.su_mutex),
> +};
> +
> +static bool dtpm_configfs_exists(struct config_group *grp, const char *name)
> +{
> +	struct list_head *entry;
> +
> +	list_for_each(entry, &grp->cg_children) {
> +		struct config_item *item =
> +			container_of(entry, struct config_item, ci_entry);
> +
> +		if (config_item_name(item) &&
> +		    !strcmp(config_item_name(item), name))
> +			return true;
> +
> +		if (dtpm_configfs_exists(to_config_group(item), name))
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +static struct config_group *dtpm_cstrn_make_group(struct config_group *grp, const char *name)
> +{
> +	struct dtpm *d, *p;
> +	int ret;
> +
> +	if (dtpm_configfs_exists(cstrn_group, name))
> +		return ERR_PTR(-EEXIST);
> +
> +	d = dtpm_lookup(name);
> +	if (!d) {
> +		d = kzalloc(sizeof(*d), GFP_KERNEL);
> +		if (!d)
> +			return ERR_PTR(-ENOMEM);
> +		dtpm_init(d, NULL);
> +	}
> +
> +	config_group_init_type_name(&d->cfg, name, &dtpm_cstrn_type);
> +
> +	/*
> +	 * Retrieve the dtpm parent node. The first dtpm node in the
> +	 * hierarchy constraint is the root node, thus it does not
> +	 * have a parent.
> +	 */
> +	p = (grp == cstrn_group) ? NULL :
> +		container_of(grp, struct dtpm, cfg);
> +
> +	ret = dtpm_register(name, d, p);

Why isn't the "name" the same name of the sysfs object that the kernel
created already?

I still do not understand why you need 2 different names for the same
device.

> +	if (ret)
> +		goto dtpm_free;
> +
> +	if (!try_module_get(THIS_MODULE)) {

Why are you trying to increment your own module reference count?  That's
totally racy and does not work.  And you are doing it _way_ after the
function has been called, what is this for?


> +		ret = -ENODEV;
> +		goto dtpm_unregister;
> +	}
> +
> +	return &d->cfg;
> +
> +dtpm_unregister:
> +	dtpm_unregister(d);
> +dtpm_free:
> +	if (!d->ops)
> +		kfree(d);
> +
> +	return ERR_PTR(ret);
> +}
> +
> +static void dtpm_cstrn_drop_group(struct config_group *grp,
> +				  struct config_item *cfg)
> +{
> +	struct config_group *cg = to_config_group(cfg);
> +	struct dtpm *d = container_of(cg, struct dtpm, cfg);
> +
> +	dtpm_unregister(d);
> +	if (!d->ops)
> +		kfree(d);
> +	module_put(THIS_MODULE);
> +	config_item_put(cfg);
> +}
> +
> +static struct configfs_group_operations dtpm_cstrn_group_ops = {
> +	.make_group = dtpm_cstrn_make_group,
> +	.drop_item = dtpm_cstrn_drop_group,
> +};
> +
> +static ssize_t dtpm_cstrn_device_show(struct config_item *cfg, char *str)
> +{
> +	struct config_group *cg = to_config_group(cfg);
> +	struct dtpm *d = container_of(cg, struct dtpm, cfg);
> +	struct kobject *kobj = &d->zone.dev.kobj;

Why are you using the "raw" kobject?

And wait, a configfs and sysfs object are in the same structure?  You
just broke lifetime rules again :(

> +	char *string = kobject_get_path(kobj, GFP_KERNEL);
> +	ssize_t len;
> +
> +	if (!string)
> +		return -EINVAL;
> +
> +	len = sprintf(str, "%s\n", string);

What sysfs namespace is this in?  This feels really odd to me...

> +
> +	kfree(string);
> +
> +	return len;
> +}
> +
> +CONFIGFS_ATTR_RO(dtpm_cstrn_, device);
> +
> +static struct configfs_attribute *dtpm_cstrn_attrs[] = {
> +	&dtpm_cstrn_attr_device,
> +	NULL
> +};
> +
> +static struct config_item_type dtpm_cstrn_type = {
> +	.ct_owner = THIS_MODULE,
> +	.ct_group_ops = &dtpm_cstrn_group_ops,
> +};
> +
> +static int __init dtpm_configfs_init(void)
> +{
> +	int ret;
> +
> +	config_group_init(&dtpm_subsys.su_group);
> +
> +	ret = configfs_register_subsystem(&dtpm_subsys);
> +	if (ret)
> +		return ret;
> +
> +	cstrn_group = configfs_register_default_group(&dtpm_subsys.su_group,
> +						      "constraints",
> +						      &dtpm_cstrn_type);
> +
> +	/*
> +	 * The default group does not contain attributes but the other
> +	 * group will
> +	 */
> +	dtpm_cstrn_type.ct_attrs = dtpm_cstrn_attrs;
> +
> +	return PTR_ERR_OR_ZERO(cstrn_group);
> +}
> +module_init(dtpm_configfs_init);
> +
> +static void __exit dtpm_configfs_exit(void)
> +{
> +	configfs_unregister_default_group(cstrn_group);
> +	configfs_unregister_subsystem(&dtpm_subsys);
> +}
> +module_exit(dtpm_configfs_exit);
> +
> +MODULE_DESCRIPTION("DTPM configuration driver");
> +MODULE_AUTHOR("Daniel Lezcano");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/dtpm.h b/include/linux/dtpm.h
> index e47bd5bbf56e..d7bbb9fd97eb 100644
> --- a/include/linux/dtpm.h
> +++ b/include/linux/dtpm.h
> @@ -8,11 +8,13 @@
>  #define ___DTPM_H__
>  
>  #include <linux/powercap.h>
> +#include <linux/configfs.h>
>  
>  #define MAX_DTPM_DESCR 8
>  #define MAX_DTPM_CONSTRAINTS 1
>  
>  struct dtpm {
> +	struct config_group cfg;

You now have 2 reference counted structures trying to control the same
structure.  Your lifetime rules just got so intertwined and messed up
it's not going to work, sorry.

thanks,

greg k-h

Re: [PATCH v6 2/7] powercap/drivers/dtpm: Create a registering system

[Daniel Lezcano]

Hi Greg,

On 01/04/2021 21:28, Greg KH wrote:
> On Thu, Apr 01, 2021 at 08:36:49PM +0200, Daniel Lezcano wrote:
>> A SoC can be differently structured depending on the platform and the
>> kernel can not be aware of all the combinations, as well as the
>> specific tweaks for a particular board.
>>
>> The creation of the hierarchy must be delegated to userspace.
> 
> Why?  Isn't this what DT is for?

I've always been told the DT describes the hardware. Here we are more
describing a configuration, that is the reason why I've let the
userspace to handle that through configfs.

> What "userspace tool" is going to be created to manage all of this?
> Pointers to that codebase?

You are certainly aware of most of it but let me give a bit more of context.

The thermal framework has cooling devices which export their 'state', a
representation of their performance level, in sysfs. Unfortunately that
gives access from the user space to act on the performance as a power
limiter in total conflict with the in-kernel thermal governor decisions.

That is done from thermal daemon the different SoC vendors tweak for
their platform. Depending on the application running and identified as a
scenario, the daemon acts proactively on the different cooling devices
to ensure a skin temperature which is far below the thermal limit of the
components.

This usage of the cooling devices hijacked the real purpose of the
thermal framework which is to protect the silicon. Nobody to blame,
there is no alternative for userspace.

The use case falls under the power limitation framework prerogative and
that is the reason why we provided a new framework to limit the power
based on the powercap framework. The thermal daemon can then use it and
stop abusing the thermal framework.

This DTPM framework allows to read the power consumption and set a power
limit to a device.

While the powercap simple backend gives a single device entry, DTPM
aggregates the different devices power by summing their power and their
limits. The tree representation of the different DTPM nodes describe how
their limits are set and how the power is computed along the different
devices.

For more info, we did a presentation at ELC [1] and Linux PM
microconference [2] and there is an article talking about it [3].


To answer your questions, there is a SoC vendor thermal daemon using
DTPM and there is a tool created to watch the thermal framework and read
the DTPM values, it is available at [4]. It is currently under
development with the goal of doing power rebalancing / capping across
the different nodes when there is a violation of the parent's power limit.



[1]
https://ossna2020.sched.com/event/c3Wf/ideas-for-finer-grained-control-over-your-heat-budget-amit-kucheria-daniel-lezcano-linaro

[2]
https://www.linuxplumbersconf.org/event/7/page/80-accepted-microconferences#power-cr

[3] https://www.linaro.org/blog/using-energy-model-to-stay-in-tdp-budget/

[4] https://git.linaro.org/people/daniel.lezcano/dtpm.git


>> These changes provide a registering mechanism where the different
>> subsystems will initialize their dtpm backends and register with a
>> name the dtpm node in a list.
>>
>> The next changes will provide an userspace interface to create
>> hierarchically the different nodes. Those will be created by name and
>> found via the list filled by the different subsystem.
>>
>> If a specified name is not found in the list, it is assumed to be a
>> virtual node which will have children and the default is to allocate
>> such node.
> 
> So userspace sets the name?
> 
> Why not use the name in the device itself?  I thought I asked that last
> time...

I probably missed it, sorry for that.

When the userspace creates the directory in the configfs, there is a
lookup with the name in the device list name. If it is found, then the
device is used, otherwise a virtual node is created instead, its power
consumption is equal to the sum of the children.

The different drivers register themselves with their name and the
associated dtpm structure. The userspace pick in this list to create a
hierarchy via configfs.

For example, a big.Little system.

- little CPUs power limiter will have the name cpu0-cpufreq
- big CPUs will have the name cpu4-cpufreq
- gpu will have the name ff9a0000.gpu-devfreq
- charger will have the name power-supply-charge
- DDR memory controller can have the name dmc-devfreq

Userspace may want to create this hierarchy:

soc
 - package
   - cluster0
     - cpu0-cpufreq
   - cluster1
     - ff9a0000.gpu-devfreq
   - dmc-devfreq
 - battery
   - power-supply-charge

It will do:

mkdir soc (virtual node)
mkdir soc/cluster0 (virtual node)
mkdir soc/cluster0/cpu0-cpufreq (real device)
etc ...

The configfs does not represent the layout of the sensors or the floor
plan of the devices but only the constraints we want to tie together.

That is the reason why I think using configfs instead of OF is more
adequate and flexible as userspace deals with the power numbers.
Moreover, we won't be facing issues with devices initialization priority
when the daemon starts.

I thought we can add OF later, when the framework has more users and
more devices. The configfs and OF can certainly co-exist or be mutually
exclusive via the Kconfig option.


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v6 2/7] powercap/drivers/dtpm: Create a registering system

[Greg KH]

On Fri, Apr 02, 2021 at 12:08:49AM +0200, Daniel Lezcano wrote:
> 
> Hi Greg,
> 
> On 01/04/2021 21:28, Greg KH wrote:
> > On Thu, Apr 01, 2021 at 08:36:49PM +0200, Daniel Lezcano wrote:
> >> A SoC can be differently structured depending on the platform and the
> >> kernel can not be aware of all the combinations, as well as the
> >> specific tweaks for a particular board.
> >>
> >> The creation of the hierarchy must be delegated to userspace.
> > 
> > Why?  Isn't this what DT is for?
> 
> I've always been told the DT describes the hardware. Here we are more
> describing a configuration, that is the reason why I've let the
> userspace to handle that through configfs.

DT does describe how the hardware configuration is to be used.  You are
saying that the hardware description does not work and somehow you need
a magic userspace tool to reconfigure things instead?

> > What "userspace tool" is going to be created to manage all of this?
> > Pointers to that codebase?
> 
> You are certainly aware of most of it but let me give a bit more of context.

No, I am not aware of it at all, thanks :)

> The thermal framework has cooling devices which export their 'state', a
> representation of their performance level, in sysfs. Unfortunately that
> gives access from the user space to act on the performance as a power
> limiter in total conflict with the in-kernel thermal governor decisions.

Why not remove that conflict?

> That is done from thermal daemon the different SoC vendors tweak for
> their platform. Depending on the application running and identified as a
> scenario, the daemon acts proactively on the different cooling devices
> to ensure a skin temperature which is far below the thermal limit of the
> components.

So userspace is going to try to manage power settings in order to keep
thermal values under control?  Has no one learned from our past mistakes
when we used to have to do this 10 years ago and it turned out so bad
that it was just baked into the hardware instead?

{sigh}

> This usage of the cooling devices hijacked the real purpose of the
> thermal framework which is to protect the silicon. Nobody to blame,
> there is no alternative for userspace.

Userspace should not care.

> The use case falls under the power limitation framework prerogative and
> that is the reason why we provided a new framework to limit the power
> based on the powercap framework. The thermal daemon can then use it and
> stop abusing the thermal framework.
> 
> This DTPM framework allows to read the power consumption and set a power
> limit to a device.
> 
> While the powercap simple backend gives a single device entry, DTPM
> aggregates the different devices power by summing their power and their
> limits. The tree representation of the different DTPM nodes describe how
> their limits are set and how the power is computed along the different
> devices.

That's all great, doing this in-kernel is fine, it's now the "userspace
must set this up properly that I'm objecting to as no one will know how
to do this.

> For more info, we did a presentation at ELC [1] and Linux PM
> microconference [2] and there is an article talking about it [3].
> 
> 
> To answer your questions, there is a SoC vendor thermal daemon using
> DTPM and there is a tool created to watch the thermal framework and read
> the DTPM values, it is available at [4]. It is currently under
> development with the goal of doing power rebalancing / capping across
> the different nodes when there is a violation of the parent's power limit.

Crazy ideas aside, your implementation of this is my main objection
here.  You are creating a user/kernel api that you will have to support
for 20+ years, without a real userspace user just yet (from what I can
tell).  That's rough, and is going to mean that this gets messy over
time.

Also there's the whole "tying sysfs to configfs" mess and reference
counting that I object to as well...

> >> The next changes will provide an userspace interface to create
> >> hierarchically the different nodes. Those will be created by name and
> >> found via the list filled by the different subsystem.
> >>
> >> If a specified name is not found in the list, it is assumed to be a
> >> virtual node which will have children and the default is to allocate
> >> such node.
> > 
> > So userspace sets the name?
> > 
> > Why not use the name in the device itself?  I thought I asked that last
> > time...
> 
> I probably missed it, sorry for that.
> 
> When the userspace creates the directory in the configfs, there is a
> lookup with the name in the device list name. If it is found, then the
> device is used, otherwise a virtual node is created instead, its power
> consumption is equal to the sum of the children.
> 
> The different drivers register themselves with their name and the
> associated dtpm structure. The userspace pick in this list to create a
> hierarchy via configfs.
> 
> For example, a big.Little system.
> 
> - little CPUs power limiter will have the name cpu0-cpufreq
> - big CPUs will have the name cpu4-cpufreq
> - gpu will have the name ff9a0000.gpu-devfreq
> - charger will have the name power-supply-charge
> - DDR memory controller can have the name dmc-devfreq
> 
> Userspace may want to create this hierarchy:
> 
> soc
>  - package
>    - cluster0
>      - cpu0-cpufreq
>    - cluster1
>      - ff9a0000.gpu-devfreq
>    - dmc-devfreq
>  - battery
>    - power-supply-charge
> 
> It will do:
> 
> mkdir soc (virtual node)
> mkdir soc/cluster0 (virtual node)
> mkdir soc/cluster0/cpu0-cpufreq (real device)
> etc ...
> 
> The configfs does not represent the layout of the sensors or the floor
> plan of the devices but only the constraints we want to tie together.
> 
> That is the reason why I think using configfs instead of OF is more
> adequate and flexible as userspace deals with the power numbers.
> Moreover, we won't be facing issues with devices initialization priority
> when the daemon starts.
> 
> I thought we can add OF later, when the framework has more users and
> more devices. The configfs and OF can certainly co-exist or be mutually
> exclusive via the Kconfig option.

Kconfig options are not ok for this, you want to build a kernel that
works everywhere.

If you want to make virtual things, that's fine, but again, your tying
of sysfs to configfs in odd ways, along with the reference counting
bugs/nightmare the current implementation is creating, is a non-starter
for me at the moment, sorry.

thanks,

greg k-h

Re: [PATCH v6 7/7] powercap/drivers/dtpm: Allow dtpm node device creation through configfs

[Daniel Lezcano]

On 01/04/2021 21:37, Greg KH wrote:
> On Thu, Apr 01, 2021 at 08:36:54PM +0200, Daniel Lezcano wrote:

[ ... ]

> Why have you not documented all of this in Documentation/ABI so that we
> can find it later?

You are right, I will write some documentation.


>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>>  drivers/powercap/Kconfig         |   8 ++
>>  drivers/powercap/Makefile        |   1 +
>>  drivers/powercap/dtpm_configfs.c | 202 +++++++++++++++++++++++++++++++
>>  include/linux/dtpm.h             |   2 +
>>  4 files changed, 213 insertions(+)
>>  create mode 100644 drivers/powercap/dtpm_configfs.c
>>
>> diff --git a/drivers/powercap/Kconfig b/drivers/powercap/Kconfig
>> index 8242e8c5ed77..599b41e4e0a7 100644
>> --- a/drivers/powercap/Kconfig
>> +++ b/drivers/powercap/Kconfig
>> @@ -50,6 +50,14 @@ config DTPM
>>  	  This enables support for the power capping for the dynamic
>>  	  thermal power management userspace engine.
>>  
>> +config DTPM_CONFIGFS
>> +	tristate "Dynamic Thermal Power Management configuration via configfs"
>> +	depends on DTPM && CONFIGFS_FS
>> +	help
>> +	  This enables support for creating the DTPM device hierarchy
>> +	  via configfs giving the userspace full control of the
>> +	  thermal constraint representation.
> 
> Why does this have to be a module, shouldn't it just be part of the DTPM
> code itself?

All options I've seen around are tristate as well as CONFIGFS_FS, so
TBH, I don't know what is the best option.

>> +
>>  config DTPM_CPU
>>  	bool "Add CPU power capping based on the energy model"
>>  	depends on DTPM && ENERGY_MODEL
>> diff --git a/drivers/powercap/Makefile b/drivers/powercap/Makefile
>> index fabcf388a8d3..519cabc624c3 100644
>> --- a/drivers/powercap/Makefile
>> +++ b/drivers/powercap/Makefile
>> @@ -1,5 +1,6 @@
>>  # SPDX-License-Identifier: GPL-2.0-only
>>  obj-$(CONFIG_DTPM) += dtpm.o
>> +obj-$(CONFIG_DTPM_CONFIGFS) += dtpm_configfs.o
>>  obj-$(CONFIG_DTPM_CPU) += dtpm_cpu.o
>>  obj-$(CONFIG_POWERCAP)	+= powercap_sys.o
>>  obj-$(CONFIG_INTEL_RAPL_CORE) += intel_rapl_common.o
>> diff --git a/drivers/powercap/dtpm_configfs.c b/drivers/powercap/dtpm_configfs.c
>> new file mode 100644
>> index 000000000000..b8de71e94fc3
>> --- /dev/null
>> +++ b/drivers/powercap/dtpm_configfs.c
>> @@ -0,0 +1,202 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright 2021 Linaro Limited
>> + *
>> + * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
>> + *
>> + * The DTPM framework defines a set of devices which are power capable.
>> + *
>> + * The configfs allows to create a hierarchy of devices in order
>> + * to reflect the constraints we want to apply to them.
>> + *
>> + * Each dtpm node is created via a mkdir operation in the configfs
>> + * directory. It will create the corresponding dtpm device in the
>> + * sysfs and the 'device' will contain the absolute path to the dtpm
>> + * node in the sysfs, thus allowing to do the connection between the
>> + * created dtpm node in the configfs hierarchy and the dtpm node in
>> + * the powercap framework.
> 
> Tieing configfs and sysfs is very odd, and ripe for problems.  Those are
> independant filesystems with independant devices and paths and object
> lifetimes.  How can you guarantee they all work together ok?

I think my description is not correct. We do not create a sysfs entry,
we create a powercap device. So there is not interaction, just a 1:1
relationship with create/remove and the powercap device.

But I can understand the "path" for the device can be controversial. I
don't see how it can be inconsistent as it refers always to the device
path which does not change but we describe something from one filesystem
to another filesystem.

If it is a problem, it can be removed. Userspace can still do the
connection between the configfs entry name and the device in sysfs by
looking up the name in /sys/devices/virtual/powercap/dtpm/dtpm:*/name.

[ ... ]

>> +static struct config_group *dtpm_cstrn_make_group(struct config_group *grp, const char *name)
>> +{
>> +	struct dtpm *d, *p;
>> +	int ret;
>> +
>> +	if (dtpm_configfs_exists(cstrn_group, name))
>> +		return ERR_PTR(-EEXIST);
>> +
>> +	d = dtpm_lookup(name);
>> +	if (!d) {
>> +		d = kzalloc(sizeof(*d), GFP_KERNEL);
>> +		if (!d)
>> +			return ERR_PTR(-ENOMEM);
>> +		dtpm_init(d, NULL);
>> +	}
>> +
>> +	config_group_init_type_name(&d->cfg, name, &dtpm_cstrn_type);
>> +
>> +	/*
>> +	 * Retrieve the dtpm parent node. The first dtpm node in the
>> +	 * hierarchy constraint is the root node, thus it does not
>> +	 * have a parent.
>> +	 */
>> +	p = (grp == cstrn_group) ? NULL :
>> +		container_of(grp, struct dtpm, cfg);
>> +
>> +	ret = dtpm_register(name, d, p);
> 
> Why isn't the "name" the same name of the sysfs object that the kernel
> created already?
> 
> I still do not understand why you need 2 different names for the same
> device.

Perhaps I answered in the previous email or I am misunderstanding your
question.

The different devices at init time will initialize their power numbers
and allocate a dtpm structure with the ops to get/set the power.

They add the dtpm pointer to a list with the name which is an unique
identifier. At this point, the sysfs object does not exists.

That is what do dtpm_add() / dtpm_del().

When we call the function dtpm_cstrn_make_group(), it call
dtpm_register() with the name and that creates the sysfs object with the
name.


>> +	if (ret)
>> +		goto dtpm_free;
>> +
>> +	if (!try_module_get(THIS_MODULE)) {
> 
> Why are you trying to increment your own module reference count?  That's
> totally racy and does not work.  And you are doing it _way_ after the
> function has been called, what is this for?

Right, it does not make sense.


>> +		ret = -ENODEV;
>> +		goto dtpm_unregister;
>> +	}
>> +
>> +	return &d->cfg;
>> +
>> +dtpm_unregister:
>> +	dtpm_unregister(d);
>> +dtpm_free:
>> +	if (!d->ops)
>> +		kfree(d);
>> +
>> +	return ERR_PTR(ret);
>> +}
>> +
>> +static void dtpm_cstrn_drop_group(struct config_group *grp,
>> +				  struct config_item *cfg)
>> +{
>> +	struct config_group *cg = to_config_group(cfg);
>> +	struct dtpm *d = container_of(cg, struct dtpm, cfg);
>> +
>> +	dtpm_unregister(d);
>> +	if (!d->ops)
>> +		kfree(d);
>> +	module_put(THIS_MODULE);
>> +	config_item_put(cfg);
>> +}
>> +
>> +static struct configfs_group_operations dtpm_cstrn_group_ops = {
>> +	.make_group = dtpm_cstrn_make_group,
>> +	.drop_item = dtpm_cstrn_drop_group,
>> +};
>> +
>> +static ssize_t dtpm_cstrn_device_show(struct config_item *cfg, char *str)
>> +{
>> +	struct config_group *cg = to_config_group(cfg);
>> +	struct dtpm *d = container_of(cg, struct dtpm, cfg);
>> +	struct kobject *kobj = &d->zone.dev.kobj;
> 
> Why are you using the "raw" kobject?
> 
> And wait, a configfs and sysfs object are in the same structure?  You
> just broke lifetime rules again :(
> 
>> +	char *string = kobject_get_path(kobj, GFP_KERNEL);
>> +	ssize_t len;
>> +
>> +	if (!string)
>> +		return -EINVAL;
>> +
>> +	len = sprintf(str, "%s\n", string);
> 
> What sysfs namespace is this in?  This feels really odd to me...
> 
>> +
>> +	kfree(string);
>> +
>> +	return len;
>> +}
>> +
>> +CONFIGFS_ATTR_RO(dtpm_cstrn_, device);
>> +
>> +static struct configfs_attribute *dtpm_cstrn_attrs[] = {
>> +	&dtpm_cstrn_attr_device,
>> +	NULL
>> +};
>> +
>> +static struct config_item_type dtpm_cstrn_type = {
>> +	.ct_owner = THIS_MODULE,
>> +	.ct_group_ops = &dtpm_cstrn_group_ops,
>> +};
>> +
>> +static int __init dtpm_configfs_init(void)
>> +{
>> +	int ret;
>> +
>> +	config_group_init(&dtpm_subsys.su_group);
>> +
>> +	ret = configfs_register_subsystem(&dtpm_subsys);
>> +	if (ret)
>> +		return ret;
>> +
>> +	cstrn_group = configfs_register_default_group(&dtpm_subsys.su_group,
>> +						      "constraints",
>> +						      &dtpm_cstrn_type);
>> +
>> +	/*
>> +	 * The default group does not contain attributes but the other
>> +	 * group will
>> +	 */
>> +	dtpm_cstrn_type.ct_attrs = dtpm_cstrn_attrs;
>> +
>> +	return PTR_ERR_OR_ZERO(cstrn_group);
>> +}
>> +module_init(dtpm_configfs_init);
>> +
>> +static void __exit dtpm_configfs_exit(void)
>> +{
>> +	configfs_unregister_default_group(cstrn_group);
>> +	configfs_unregister_subsystem(&dtpm_subsys);
>> +}
>> +module_exit(dtpm_configfs_exit);
>> +
>> +MODULE_DESCRIPTION("DTPM configuration driver");
>> +MODULE_AUTHOR("Daniel Lezcano");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/dtpm.h b/include/linux/dtpm.h
>> index e47bd5bbf56e..d7bbb9fd97eb 100644
>> --- a/include/linux/dtpm.h
>> +++ b/include/linux/dtpm.h
>> @@ -8,11 +8,13 @@
>>  #define ___DTPM_H__
>>  
>>  #include <linux/powercap.h>
>> +#include <linux/configfs.h>
>>  
>>  #define MAX_DTPM_DESCR 8
>>  #define MAX_DTPM_CONSTRAINTS 1
>>  
>>  struct dtpm {
>> +	struct config_group cfg;
> 
> You now have 2 reference counted structures trying to control the same
> structure.  Your lifetime rules just got so intertwined and messed up
> it's not going to work, sorry.

Ok, I will rework this part.

Thanks for taking the time to review the patch

  -- Daniel


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v6 7/7] powercap/drivers/dtpm: Allow dtpm node device creation through configfs

[Daniel Lezcano]

On 01/04/2021 21:37, Greg KH wrote:
> On Thu, Apr 01, 2021 at 08:36:54PM +0200, Daniel Lezcano wrote:

[ ... ]

> Why have you not documented all of this in Documentation/ABI so that we
> can find it later?

You are right, I will write some documentation.


>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>>  drivers/powercap/Kconfig         |   8 ++
>>  drivers/powercap/Makefile        |   1 +
>>  drivers/powercap/dtpm_configfs.c | 202 +++++++++++++++++++++++++++++++
>>  include/linux/dtpm.h             |   2 +
>>  4 files changed, 213 insertions(+)
>>  create mode 100644 drivers/powercap/dtpm_configfs.c
>>
>> diff --git a/drivers/powercap/Kconfig b/drivers/powercap/Kconfig
>> index 8242e8c5ed77..599b41e4e0a7 100644
>> --- a/drivers/powercap/Kconfig
>> +++ b/drivers/powercap/Kconfig
>> @@ -50,6 +50,14 @@ config DTPM
>>  	  This enables support for the power capping for the dynamic
>>  	  thermal power management userspace engine.
>>  
>> +config DTPM_CONFIGFS
>> +	tristate "Dynamic Thermal Power Management configuration via configfs"
>> +	depends on DTPM && CONFIGFS_FS
>> +	help
>> +	  This enables support for creating the DTPM device hierarchy
>> +	  via configfs giving the userspace full control of the
>> +	  thermal constraint representation.
> 
> Why does this have to be a module, shouldn't it just be part of the DTPM
> code itself?

All options I've seen around are tristate as well as CONFIGFS_FS, so
TBH, I don't know what is the best option.

>> +
>>  config DTPM_CPU
>>  	bool "Add CPU power capping based on the energy model"
>>  	depends on DTPM && ENERGY_MODEL
>> diff --git a/drivers/powercap/Makefile b/drivers/powercap/Makefile
>> index fabcf388a8d3..519cabc624c3 100644
>> --- a/drivers/powercap/Makefile
>> +++ b/drivers/powercap/Makefile
>> @@ -1,5 +1,6 @@
>>  # SPDX-License-Identifier: GPL-2.0-only
>>  obj-$(CONFIG_DTPM) += dtpm.o
>> +obj-$(CONFIG_DTPM_CONFIGFS) += dtpm_configfs.o
>>  obj-$(CONFIG_DTPM_CPU) += dtpm_cpu.o
>>  obj-$(CONFIG_POWERCAP)	+= powercap_sys.o
>>  obj-$(CONFIG_INTEL_RAPL_CORE) += intel_rapl_common.o
>> diff --git a/drivers/powercap/dtpm_configfs.c b/drivers/powercap/dtpm_configfs.c
>> new file mode 100644
>> index 000000000000..b8de71e94fc3
>> --- /dev/null
>> +++ b/drivers/powercap/dtpm_configfs.c
>> @@ -0,0 +1,202 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright 2021 Linaro Limited
>> + *
>> + * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
>> + *
>> + * The DTPM framework defines a set of devices which are power capable.
>> + *
>> + * The configfs allows to create a hierarchy of devices in order
>> + * to reflect the constraints we want to apply to them.
>> + *
>> + * Each dtpm node is created via a mkdir operation in the configfs
>> + * directory. It will create the corresponding dtpm device in the
>> + * sysfs and the 'device' will contain the absolute path to the dtpm
>> + * node in the sysfs, thus allowing to do the connection between the
>> + * created dtpm node in the configfs hierarchy and the dtpm node in
>> + * the powercap framework.
> 
> Tieing configfs and sysfs is very odd, and ripe for problems.  Those are
> independant filesystems with independant devices and paths and object
> lifetimes.  How can you guarantee they all work together ok?

I think my description is not correct. We do not create a sysfs entry,
we create a powercap device. So there is not interaction, just a 1:1
relationship with create/remove and the powercap device.

But I can understand the "path" for the device can be controversial. I
don't see how it can be inconsistent as it refers always to the device
path which does not change but we describe something from one filesystem
to another filesystem.

If it is a problem, it can be removed. Userspace can still do the
connection between the configfs entry name and the device in sysfs by
looking up the name in /sys/devices/virtual/powercap/dtpm/dtpm:*/name.

[ ... ]

>> +static struct config_group *dtpm_cstrn_make_group(struct config_group *grp, const char *name)
>> +{
>> +	struct dtpm *d, *p;
>> +	int ret;
>> +
>> +	if (dtpm_configfs_exists(cstrn_group, name))
>> +		return ERR_PTR(-EEXIST);
>> +
>> +	d = dtpm_lookup(name);
>> +	if (!d) {
>> +		d = kzalloc(sizeof(*d), GFP_KERNEL);
>> +		if (!d)
>> +			return ERR_PTR(-ENOMEM);
>> +		dtpm_init(d, NULL);
>> +	}
>> +
>> +	config_group_init_type_name(&d->cfg, name, &dtpm_cstrn_type);
>> +
>> +	/*
>> +	 * Retrieve the dtpm parent node. The first dtpm node in the
>> +	 * hierarchy constraint is the root node, thus it does not
>> +	 * have a parent.
>> +	 */
>> +	p = (grp == cstrn_group) ? NULL :
>> +		container_of(grp, struct dtpm, cfg);
>> +
>> +	ret = dtpm_register(name, d, p);
> 
> Why isn't the "name" the same name of the sysfs object that the kernel
> created already?
> 
> I still do not understand why you need 2 different names for the same
> device.

Perhaps I answered in the previous email or I am misunderstanding your
question.

The different devices at init time will initialize their power numbers
and allocate a dtpm structure with the ops to get/set the power.

They add the dtpm pointer to a list with the name which is an unique
identifier. At this point, the sysfs object does not exists.

That is what do dtpm_add() / dtpm_del().

When we call the function dtpm_cstrn_make_group(), it call
dtpm_register() with the name and that creates the sysfs object with the
name.


>> +	if (ret)
>> +		goto dtpm_free;
>> +
>> +	if (!try_module_get(THIS_MODULE)) {
> 
> Why are you trying to increment your own module reference count?  That's
> totally racy and does not work.  And you are doing it _way_ after the
> function has been called, what is this for?

Right, it does not make sense.


>> +		ret = -ENODEV;
>> +		goto dtpm_unregister;
>> +	}
>> +
>> +	return &d->cfg;
>> +
>> +dtpm_unregister:
>> +	dtpm_unregister(d);
>> +dtpm_free:
>> +	if (!d->ops)
>> +		kfree(d);
>> +
>> +	return ERR_PTR(ret);
>> +}
>> +
>> +static void dtpm_cstrn_drop_group(struct config_group *grp,
>> +				  struct config_item *cfg)
>> +{
>> +	struct config_group *cg = to_config_group(cfg);
>> +	struct dtpm *d = container_of(cg, struct dtpm, cfg);
>> +
>> +	dtpm_unregister(d);
>> +	if (!d->ops)
>> +		kfree(d);
>> +	module_put(THIS_MODULE);
>> +	config_item_put(cfg);
>> +}
>> +
>> +static struct configfs_group_operations dtpm_cstrn_group_ops = {
>> +	.make_group = dtpm_cstrn_make_group,
>> +	.drop_item = dtpm_cstrn_drop_group,
>> +};
>> +
>> +static ssize_t dtpm_cstrn_device_show(struct config_item *cfg, char *str)
>> +{
>> +	struct config_group *cg = to_config_group(cfg);
>> +	struct dtpm *d = container_of(cg, struct dtpm, cfg);
>> +	struct kobject *kobj = &d->zone.dev.kobj;
> 
> Why are you using the "raw" kobject?
> 
> And wait, a configfs and sysfs object are in the same structure?  You
> just broke lifetime rules again :(
> 
>> +	char *string = kobject_get_path(kobj, GFP_KERNEL);
>> +	ssize_t len;
>> +
>> +	if (!string)
>> +		return -EINVAL;
>> +
>> +	len = sprintf(str, "%s\n", string);
> 
> What sysfs namespace is this in?  This feels really odd to me...
> 
>> +
>> +	kfree(string);
>> +
>> +	return len;
>> +}
>> +
>> +CONFIGFS_ATTR_RO(dtpm_cstrn_, device);
>> +
>> +static struct configfs_attribute *dtpm_cstrn_attrs[] = {
>> +	&dtpm_cstrn_attr_device,
>> +	NULL
>> +};
>> +
>> +static struct config_item_type dtpm_cstrn_type = {
>> +	.ct_owner = THIS_MODULE,
>> +	.ct_group_ops = &dtpm_cstrn_group_ops,
>> +};
>> +
>> +static int __init dtpm_configfs_init(void)
>> +{
>> +	int ret;
>> +
>> +	config_group_init(&dtpm_subsys.su_group);
>> +
>> +	ret = configfs_register_subsystem(&dtpm_subsys);
>> +	if (ret)
>> +		return ret;
>> +
>> +	cstrn_group = configfs_register_default_group(&dtpm_subsys.su_group,
>> +						      "constraints",
>> +						      &dtpm_cstrn_type);
>> +
>> +	/*
>> +	 * The default group does not contain attributes but the other
>> +	 * group will
>> +	 */
>> +	dtpm_cstrn_type.ct_attrs = dtpm_cstrn_attrs;
>> +
>> +	return PTR_ERR_OR_ZERO(cstrn_group);
>> +}
>> +module_init(dtpm_configfs_init);
>> +
>> +static void __exit dtpm_configfs_exit(void)
>> +{
>> +	configfs_unregister_default_group(cstrn_group);
>> +	configfs_unregister_subsystem(&dtpm_subsys);
>> +}
>> +module_exit(dtpm_configfs_exit);
>> +
>> +MODULE_DESCRIPTION("DTPM configuration driver");
>> +MODULE_AUTHOR("Daniel Lezcano");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/dtpm.h b/include/linux/dtpm.h
>> index e47bd5bbf56e..d7bbb9fd97eb 100644
>> --- a/include/linux/dtpm.h
>> +++ b/include/linux/dtpm.h
>> @@ -8,11 +8,13 @@
>>  #define ___DTPM_H__
>>  
>>  #include <linux/powercap.h>
>> +#include <linux/configfs.h>
>>  
>>  #define MAX_DTPM_DESCR 8
>>  #define MAX_DTPM_CONSTRAINTS 1
>>  
>>  struct dtpm {
>> +	struct config_group cfg;
> 
> You now have 2 reference counted structures trying to control the same
> structure.  Your lifetime rules just got so intertwined and messed up
> it's not going to work, sorry.

Ok, I will rework this part.

Thanks for taking the time to review the patch

  -- Daniel


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v6 2/7] powercap/drivers/dtpm: Create a registering system

[Daniel Lezcano]

On 02/04/2021 10:02, Greg KH wrote:
> On Fri, Apr 02, 2021 at 12:08:49AM +0200, Daniel Lezcano wrote:
>>
>> Hi Greg,
>>
>> On 01/04/2021 21:28, Greg KH wrote:
>>> On Thu, Apr 01, 2021 at 08:36:49PM +0200, Daniel Lezcano wrote:
>>>> A SoC can be differently structured depending on the platform and the
>>>> kernel can not be aware of all the combinations, as well as the
>>>> specific tweaks for a particular board.
>>>>
>>>> The creation of the hierarchy must be delegated to userspace.
>>>
>>> Why?  Isn't this what DT is for?
>>
>> I've always been told the DT describes the hardware. Here we are more
>> describing a configuration, that is the reason why I've let the
>> userspace to handle that through configfs.
> 
> DT does describe how the hardware configuration is to be used.  You are
> saying that the hardware description does not work and somehow you need
> a magic userspace tool to reconfigure things instead?
> 
>>> What "userspace tool" is going to be created to manage all of this?
>>> Pointers to that codebase?
>>
>> You are certainly aware of most of it but let me give a bit more of context.
> 
> No, I am not aware of it at all, thanks :)
> 
>> The thermal framework has cooling devices which export their 'state', a
>> representation of their performance level, in sysfs. Unfortunately that
>> gives access from the user space to act on the performance as a power
>> limiter in total conflict with the in-kernel thermal governor decisions.
> 
> Why not remove that conflict?

Because the cooling device state should have not be exported its state
in a writable way, that is a weakness different applications are
abusing. Moreover, the thermal framework is not designed for that. It is
not the purpose of the cooling device to be managed as a power limiter
by the userspace.

The powercap framework is there for that.

There are devices registering themselves as a cooling device while there
is no sensor and governor for them just for the sake of acting on their
power via opaque integer values.


>> That is done from thermal daemon the different SoC vendors tweak for
>> their platform. Depending on the application running and identified as a
>> scenario, the daemon acts proactively on the different cooling devices
>> to ensure a skin temperature which is far below the thermal limit of the
>> components.
> 
> So userspace is going to try to manage power settings in order to keep
> thermal values under control?  Has no one learned from our past mistakes
> when we used to have to do this 10 years ago and it turned out so bad
> that it was just baked into the hardware instead?

I agree in a desktop/server environment, that is true but on the
embedded world, things are a bit different because we are on battery,
the cooling devices are passive and the device is carried. This is why
there are different level of thermal control:

 - In the hardware / firmware to protect physically the silicon

 - In the kernel, to protect from a hard reset or reboot. Usually each
of them is represented with a thermal zone (and its sensor) and a
governor. There is a 1:1 relationship. IOW, a governor manage a thermal
zone without any knowledge of the other thermal zones temperature, just
the one it is monitoring and acts on the power (with different
techniques) when the temperature threshold is reached. Its action is
local and it is after crossing a certain temperature for this component.

 - In the userspace, the logic will get notified about which application
is running and can set the power limitation on different devices knowing
the performances are enough for that particular application, that saves
energy and consequently reduce the temperature. Its action is on
multiple places and happens far below the thermal limits. On the other
side, the skin temperature is a particular sensor placed on the
dissipation path and its temperature must be below 45°C (30 minutes skin
contact at 45°C causes a 1st degree burn). It is the results of the
dissipation of all the devices, the logic in userspace can know which
devices to act on to have the overall power dissipation low enough to
stay below this 45°C (eg. reduce camera resolution).  So here userspace
deals with performance, temperature, application profile, etc ... and
abuse the cooling device.

I'm not sure in the ARM/ARM64 embedded ecosystem, creating a complex
hardware mechanism, supported by a firmware is really possible.

In a desktop/server environment, we do not care about having this skin
temperature (and battery savings) aspects.

> {sigh}
> 
>> This usage of the cooling devices hijacked the real purpose of the
>> thermal framework which is to protect the silicon. Nobody to blame,
>> there is no alternative for userspace.
> 
> Userspace should not care.
> 
>> The use case falls under the power limitation framework prerogative and
>> that is the reason why we provided a new framework to limit the power
>> based on the powercap framework. The thermal daemon can then use it and
>> stop abusing the thermal framework.
>>
>> This DTPM framework allows to read the power consumption and set a power
>> limit to a device.
>>
>> While the powercap simple backend gives a single device entry, DTPM
>> aggregates the different devices power by summing their power and their
>> limits. The tree representation of the different DTPM nodes describe how
>> their limits are set and how the power is computed along the different
>> devices.
> 
> That's all great, doing this in-kernel is fine, it's now the "userspace
> must set this up properly that I'm objecting to as no one will know how
> to do this.
> 
>> For more info, we did a presentation at ELC [1] and Linux PM
>> microconference [2] and there is an article talking about it [3].
>>
>>
>> To answer your questions, there is a SoC vendor thermal daemon using
>> DTPM and there is a tool created to watch the thermal framework and read
>> the DTPM values, it is available at [4]. It is currently under
>> development with the goal of doing power rebalancing / capping across
>> the different nodes when there is a violation of the parent's power limit.
> 
> Crazy ideas aside, your implementation of this is my main objection
> here.  You are creating a user/kernel api that you will have to support
> for 20+ years, without a real userspace user just yet (from what I can
> tell).  That's rough, and is going to mean that this gets messy over
> time.

I'm not sure to understand, the API already exists since v3.3, it is the
powercap and DTPM is its backend. AFAICT, there are already users of it
except they create their own way to build the hierarchy today.

> Also there's the whole "tying sysfs to configfs" mess and reference
> counting that I object to as well...

Ok, I think my description was bad but I agree with the fact that we
must get rid of any adherence between sysfs and configfs. Thanks for
reporting the issue.

[ ... ]

>> The configfs does not represent the layout of the sensors or the floor
>> plan of the devices but only the constraints we want to tie together.
>>
>> That is the reason why I think using configfs instead of OF is more
>> adequate and flexible as userspace deals with the power numbers.
>> Moreover, we won't be facing issues with devices initialization priority
>> when the daemon starts.
>>
>> I thought we can add OF later, when the framework has more users and
>> more devices. The configfs and OF can certainly co-exist or be mutually
>> exclusive via the Kconfig option.
> 
> Kconfig options are not ok for this, you want to build a kernel that
> works everywhere.
> 
> If you want to make virtual things, that's fine, but again, your tying
> of sysfs to configfs in odd ways, along with the reference counting
> bugs/nightmare the current implementation is creating, is a non-starter
> for me at the moment, sorry.

I did not realize this reference counting issue. Thanks for spotting it.

The points you raised are IMO solvable and I can send a new version but
with all the comments I'm a bit lost with the approach with configfs.

Do you think it is preferable to use OF instead ?

Thanks again for your time

  -- Daniel

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v6 2/7] powercap/drivers/dtpm: Create a registering system

[Greg KH]

On Fri, Apr 02, 2021 at 01:10:51PM +0200, Daniel Lezcano wrote:
> >> To answer your questions, there is a SoC vendor thermal daemon using
> >> DTPM and there is a tool created to watch the thermal framework and read
> >> the DTPM values, it is available at [4]. It is currently under
> >> development with the goal of doing power rebalancing / capping across
> >> the different nodes when there is a violation of the parent's power limit.
> > 
> > Crazy ideas aside, your implementation of this is my main objection
> > here.  You are creating a user/kernel api that you will have to support
> > for 20+ years, without a real userspace user just yet (from what I can
> > tell).  That's rough, and is going to mean that this gets messy over
> > time.
> 
> I'm not sure to understand, the API already exists since v3.3, it is the
> powercap and DTPM is its backend. AFAICT, there are already users of it
> except they create their own way to build the hierarchy today.

The configfs api is what I am referring to here, the ones in this patch
series...

thanks,

greg k-h

Re: [PATCH v6 3/7] powercap/drivers/dtpm: Simplify the dtpm table

[Doug Smythies]

[-- Attachment #1: Type: text/plain, Size: 4041 bytes --]

Hi Daniel,

This patch introduces a regression, at least on my test system.
I can no longer change CPU frequency scaling drivers, for example
from intel_cpufreq (A.K.A intel_pstate in passive mode) to intel_pstate
(A.K.A. active mode). The task just hangs forever.

I bisected the kernel and got this commit as the result.
As a double check, I reverted this commit:
7a89d7eacf8e84f2afb94db5ae9d9f9faa93f01c
on kernel 5.16-rc2 and the issue was resolved.

While your email is fairly old, I observe that it was only included as of
kernel 5.16-rc1.

Command Example that never completes:

$ echo passive | sudo tee /sys/devices/system/cpu/intel_pstate/status

syslog excerpt attached.

... Doug

On Thu, Apr 1, 2021 at 12:24 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> The dtpm table is an array of pointers, that forces the user of the
> table to define initdata along with the declaration of the table
> entry. It is more efficient to create an array of dtpm structure, so
> the declaration of the table entry can be done by initializing the
> different fields.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
> ---
>  drivers/powercap/dtpm.c     |  4 ++--
>  drivers/powercap/dtpm_cpu.c |  4 +++-
>  include/linux/dtpm.h        | 20 +++++++++-----------
>  3 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c
> index a707cc2965a1..d9aac107c927 100644
> --- a/drivers/powercap/dtpm.c
> +++ b/drivers/powercap/dtpm.c
> @@ -583,7 +583,7 @@ int dtpm_register(const char *name, struct dtpm *dtpm, struct dtpm *parent)
>
>  static int __init dtpm_init(void)
>  {
> -       struct dtpm_descr **dtpm_descr;
> +       struct dtpm_descr *dtpm_descr;
>
>         pct = powercap_register_control_type(NULL, "dtpm", NULL);
>         if (IS_ERR(pct)) {
> @@ -592,7 +592,7 @@ static int __init dtpm_init(void)
>         }
>
>         for_each_dtpm_table(dtpm_descr)
> -               (*dtpm_descr)->init(*dtpm_descr);
> +               dtpm_descr->init();
>
>         return 0;
>  }
> diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
> index 9deafd16a197..74b39a1082e5 100644
> --- a/drivers/powercap/dtpm_cpu.c
> +++ b/drivers/powercap/dtpm_cpu.c
> @@ -204,7 +204,7 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
>         return ret;
>  }
>
> -int dtpm_register_cpu(struct dtpm *parent)
> +static int __init dtpm_cpu_init(void)
>  {
>         int ret;
>
> @@ -241,3 +241,5 @@ int dtpm_register_cpu(struct dtpm *parent)
>
>         return 0;
>  }
> +
> +DTPM_DECLARE(dtpm_cpu, dtpm_cpu_init);
> diff --git a/include/linux/dtpm.h b/include/linux/dtpm.h
> index 577c71d4e098..2ec2821111d1 100644
> --- a/include/linux/dtpm.h
> +++ b/include/linux/dtpm.h
> @@ -33,25 +33,23 @@ struct dtpm_ops {
>         void (*release)(struct dtpm *);
>  };
>
> -struct dtpm_descr;
> -
> -typedef int (*dtpm_init_t)(struct dtpm_descr *);
> +typedef int (*dtpm_init_t)(void);
>
>  struct dtpm_descr {
> -       struct dtpm *parent;
> -       const char *name;
>         dtpm_init_t init;
>  };
>
>  /* Init section thermal table */
> -extern struct dtpm_descr *__dtpm_table[];
> -extern struct dtpm_descr *__dtpm_table_end[];
> +extern struct dtpm_descr __dtpm_table[];
> +extern struct dtpm_descr __dtpm_table_end[];
>
> -#define DTPM_TABLE_ENTRY(name)                 \
> -       static typeof(name) *__dtpm_table_entry_##name  \
> -       __used __section("__dtpm_table") = &name
> +#define DTPM_TABLE_ENTRY(name, __init)                         \
> +       static struct dtpm_descr __dtpm_table_entry_##name      \
> +       __used __section("__dtpm_table") = {                    \
> +               .init = __init,                                 \
> +       }
>
> -#define DTPM_DECLARE(name)     DTPM_TABLE_ENTRY(name)
> +#define DTPM_DECLARE(name, init)       DTPM_TABLE_ENTRY(name, init)
>
>  #define for_each_dtpm_table(__dtpm)    \
>         for (__dtpm = __dtpm_table;     \
> --
> 2.17.1
>

[-- Attachment #2: syslog_example.txt --]
[-- Type: text/plain, Size: 3973 bytes --]

Nov 26 07:31:13 s19 systemd[1]: Finished Update UTMP about System Runlevel Changes.
Nov 26 07:31:13 s19 systemd[1]: Startup finished in 10.788s (firmware) + 12.351s (loader) + 1.808s (kernel) + 1min 38.047s (userspace) = 2min 2.995s.
Nov 26 07:31:58 s19 kernel: [  144.568026] INFO: task tee:1270 blocked for more than 20 seconds.
Nov 26 07:31:58 s19 kernel: [  144.568069]       Not tainted 5.15.0-rc1-n13 #977   <<<< 13th kernel of bisection
Nov 26 07:31:58 s19 kernel: [  144.568096] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
Nov 26 07:31:58 s19 kernel: [  144.568135] task:tee             state:D stack:    0 pid: 1270 ppid:  1269 flags:0x00004000
Nov 26 07:31:58 s19 kernel: [  144.568141] Call Trace:
Nov 26 07:31:58 s19 kernel: [  144.568145]  __schedule+0xce9/0x13f0
Nov 26 07:31:58 s19 kernel: [  144.568155]  ? __slab_free+0x217/0x400
Nov 26 07:31:58 s19 kernel: [  144.568162]  ? __slab_free+0x217/0x400
Nov 26 07:31:58 s19 kernel: [  144.568167]  ? radix_tree_lookup+0xd/0x10
Nov 26 07:31:58 s19 kernel: [  144.568172]  ? idr_find+0xf/0x20
Nov 26 07:31:58 s19 kernel: [  144.568177]  ? get_work_pool+0x2d/0x40
Nov 26 07:31:58 s19 kernel: [  144.568182]  schedule+0x67/0xe0
Nov 26 07:31:58 s19 kernel: [  144.568187]  schedule_timeout+0x200/0x290
Nov 26 07:31:58 s19 kernel: [  144.568191]  ? freq_qos_apply+0x2d/0x50
Nov 26 07:31:58 s19 kernel: [  144.568198]  wait_for_completion+0x8b/0xf0
Nov 26 07:31:58 s19 kernel: [  144.568202]  cpufreq_policy_put_kobj+0x4d/0x90
Nov 26 07:31:58 s19 kernel: [  144.568207]  cpufreq_policy_free+0x12f/0x160
Nov 26 07:31:58 s19 kernel: [  144.568211]  cpufreq_remove_dev+0x91/0xb0
Nov 26 07:31:58 s19 kernel: [  144.568216]  subsys_interface_unregister+0x9d/0x100
Nov 26 07:31:58 s19 kernel: [  144.568223]  cpufreq_unregister_driver+0x35/0xd0
Nov 26 07:31:58 s19 kernel: [  144.568227]  store_status+0x102/0x1c0
Nov 26 07:31:58 s19 kernel: [  144.568232]  kobj_attr_store+0xf/0x20
Nov 26 07:31:58 s19 kernel: [  144.568237]  sysfs_kf_write+0x3b/0x50
Nov 26 07:31:58 s19 kernel: [  144.568242]  kernfs_fop_write_iter+0x135/0x1c0
Nov 26 07:31:58 s19 kernel: [  144.568246]  new_sync_write+0x11d/0x1b0
Nov 26 07:31:58 s19 kernel: [  144.568253]  vfs_write+0x1a7/0x290
Nov 26 07:31:58 s19 kernel: [  144.568256]  ksys_write+0x67/0xe0
Nov 26 07:31:58 s19 kernel: [  144.568259]  __x64_sys_write+0x1a/0x20
Nov 26 07:31:58 s19 kernel: [  144.568261]  do_syscall_64+0x59/0xc0
Nov 26 07:31:58 s19 kernel: [  144.568268]  ? vfs_write+0x1a7/0x290
Nov 26 07:31:58 s19 kernel: [  144.568271]  ? exit_to_user_mode_prepare+0x3d/0x1c0
Nov 26 07:31:58 s19 kernel: [  144.568277]  ? ksys_write+0x67/0xe0
Nov 26 07:31:58 s19 kernel: [  144.568280]  ? syscall_exit_to_user_mode+0x27/0x50
Nov 26 07:31:58 s19 kernel: [  144.568283]  ? __x64_sys_write+0x1a/0x20
Nov 26 07:31:58 s19 kernel: [  144.568286]  ? do_syscall_64+0x69/0xc0
Nov 26 07:31:58 s19 kernel: [  144.568291]  entry_SYSCALL_64_after_hwframe+0x44/0xae
Nov 26 07:31:58 s19 kernel: [  144.568296] RIP: 0033:0x7fca551ca1e7
Nov 26 07:31:58 s19 kernel: [  144.568300] RSP: 002b:00007fffad47a6b8 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
Nov 26 07:31:58 s19 kernel: [  144.568304] RAX: ffffffffffffffda RBX: 0000000000000007 RCX: 00007fca551ca1e7
Nov 26 07:31:58 s19 kernel: [  144.568307] RDX: 0000000000000007 RSI: 00007fffad47a7b0 RDI: 0000000000000003
Nov 26 07:31:58 s19 kernel: [  144.568309] RBP: 00007fffad47a7b0 R08: 0000000000000007 R09: 0000000000000001
Nov 26 07:31:58 s19 kernel: [  144.568311] R10: 00000000000001b6 R11: 0000000000000246 R12: 0000000000000007
Nov 26 07:31:58 s19 kernel: [  144.568313] R13: 000055eb03a6f4a0 R14: 0000000000000007 R15: 00007fca552a58a0
Nov 26 07:32:13 s19 systemd[1]: Stopping Session 1 of user doug.
Nov 26 07:32:13 s19 systemd[1]: Stopping Session 3 of user doug.
Nov 26 07:32:13 s19 systemd[1]: Stopping Session 4 of user doug.
Nov 26 07:32:13 s19 systemd[1]: Removed slice system-modprobe.slice.
...

Re: [PATCH v6 3/7] powercap/drivers/dtpm: Simplify the dtpm table

[Rafael J. Wysocki]

Hi Doug,

On Fri, Nov 26, 2021 at 6:08 PM Doug Smythies <dsmythies@telus.net> wrote:
>
> Hi Daniel,
>
> This patch introduces a regression, at least on my test system.
> I can no longer change CPU frequency scaling drivers, for example
> from intel_cpufreq (A.K.A intel_pstate in passive mode) to intel_pstate
> (A.K.A. active mode). The task just hangs forever.
>
> I bisected the kernel and got this commit as the result.
> As a double check, I reverted this commit:
> 7a89d7eacf8e84f2afb94db5ae9d9f9faa93f01c
> on kernel 5.16-rc2 and the issue was resolved.
>
> While your email is fairly old, I observe that it was only included as of
> kernel 5.16-rc1.
>
> Command Example that never completes:
>
> $ echo passive | sudo tee /sys/devices/system/cpu/intel_pstate/status
>
> syslog excerpt attached.

This looks like it may be problematic:

diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
index f6076de39540..98841524a782 100644
--- a/drivers/powercap/dtpm_cpu.c
+++ b/drivers/powercap/dtpm_cpu.c
@@ -204,7 +204,7 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
       return ret;
}

-int dtpm_register_cpu(struct dtpm *parent)
+static int __init dtpm_cpu_init(void)
{
       int ret;

so please try to remove the __init annotation from dtpm_cpu_init() and
see if that helps.

Re: [PATCH v6 3/7] powercap/drivers/dtpm: Simplify the dtpm table

[Daniel Lezcano]

On 26/11/2021 18:08, Doug Smythies wrote:
> Hi Daniel,
> 
> This patch introduces a regression, at least on my test system.
> I can no longer change CPU frequency scaling drivers, for example
> from intel_cpufreq (A.K.A intel_pstate in passive mode) to intel_pstate
> (A.K.A. active mode). The task just hangs forever.
> 
> I bisected the kernel and got this commit as the result.
> As a double check, I reverted this commit:
> 7a89d7eacf8e84f2afb94db5ae9d9f9faa93f01c
> on kernel 5.16-rc2 and the issue was resolved.
> 
> While your email is fairly old, I observe that it was only included as of
> kernel 5.16-rc1.

Could it be related to and fixed by:

https://lore.kernel.org/all/20211108062345.273855-1-daniel.lezcano@linaro.org/

?

> Command Example that never completes:
> 
> $ echo passive | sudo tee /sys/devices/system/cpu/intel_pstate/status
> 
> syslog excerpt attached.
> 
> ... Doug



-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v6 3/7] powercap/drivers/dtpm: Simplify the dtpm table

[Daniel Lezcano]

On 26/11/2021 18:21, Rafael J. Wysocki wrote:
> Hi Doug,
> 
> On Fri, Nov 26, 2021 at 6:08 PM Doug Smythies <dsmythies@telus.net> wrote:
>>
>> Hi Daniel,
>>
>> This patch introduces a regression, at least on my test system.
>> I can no longer change CPU frequency scaling drivers, for example
>> from intel_cpufreq (A.K.A intel_pstate in passive mode) to intel_pstate
>> (A.K.A. active mode). The task just hangs forever.
>>
>> I bisected the kernel and got this commit as the result.
>> As a double check, I reverted this commit:
>> 7a89d7eacf8e84f2afb94db5ae9d9f9faa93f01c
>> on kernel 5.16-rc2 and the issue was resolved.
>>
>> While your email is fairly old, I observe that it was only included as of
>> kernel 5.16-rc1.
>>
>> Command Example that never completes:
>>
>> $ echo passive | sudo tee /sys/devices/system/cpu/intel_pstate/status
>>
>> syslog excerpt attached.
> 
> This looks like it may be problematic:
> 
> diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
> index f6076de39540..98841524a782 100644
> --- a/drivers/powercap/dtpm_cpu.c
> +++ b/drivers/powercap/dtpm_cpu.c
> @@ -204,7 +204,7 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
>        return ret;
> }
> 
> -int dtpm_register_cpu(struct dtpm *parent)
> +static int __init dtpm_cpu_init(void)
> {
>        int ret;
> 
> so please try to remove the __init annotation from dtpm_cpu_init() and
> see if that helps.

Yes, actually that should be called only if it is configured properly.
The dtpm_cpu just initializes itself unconditionally, I did not figured
out there is the usually allyesconfig used by default by the distros.

That should be fixed with a proper DT configuration [1]

[1]
https://lore.kernel.org/all/20211124125506.2971069-3-daniel.lezcano@linaro.org/

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v6 3/7] powercap/drivers/dtpm: Simplify the dtpm table

[Rafael J. Wysocki]

On Friday, November 26, 2021 6:43:24 PM CET Daniel Lezcano wrote:
> On 26/11/2021 18:21, Rafael J. Wysocki wrote:
> > Hi Doug,
> > 
> > On Fri, Nov 26, 2021 at 6:08 PM Doug Smythies <dsmythies@telus.net> wrote:
> >>
> >> Hi Daniel,
> >>
> >> This patch introduces a regression, at least on my test system.
> >> I can no longer change CPU frequency scaling drivers, for example
> >> from intel_cpufreq (A.K.A intel_pstate in passive mode) to intel_pstate
> >> (A.K.A. active mode). The task just hangs forever.
> >>
> >> I bisected the kernel and got this commit as the result.
> >> As a double check, I reverted this commit:
> >> 7a89d7eacf8e84f2afb94db5ae9d9f9faa93f01c
> >> on kernel 5.16-rc2 and the issue was resolved.
> >>
> >> While your email is fairly old, I observe that it was only included as of
> >> kernel 5.16-rc1.
> >>
> >> Command Example that never completes:
> >>
> >> $ echo passive | sudo tee /sys/devices/system/cpu/intel_pstate/status
> >>
> >> syslog excerpt attached.
> > 
> > This looks like it may be problematic:
> > 
> > diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
> > index f6076de39540..98841524a782 100644
> > --- a/drivers/powercap/dtpm_cpu.c
> > +++ b/drivers/powercap/dtpm_cpu.c
> > @@ -204,7 +204,7 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
> >        return ret;
> > }
> > 
> > -int dtpm_register_cpu(struct dtpm *parent)
> > +static int __init dtpm_cpu_init(void)
> > {
> >        int ret;
> > 
> > so please try to remove the __init annotation from dtpm_cpu_init() and
> > see if that helps.
> 
> Yes, actually that should be called only if it is configured properly.

What do you mean?

> The dtpm_cpu just initializes itself unconditionally, I did not figured
> out there is the usually allyesconfig used by default by the distros.

Well, it is.

> That should be fixed with a proper DT configuration [1]
> 
> [1]
> https://lore.kernel.org/all/20211124125506.2971069-3-daniel.lezcano@linaro.org/

No, we are talking about systems without any DT at all here.

Re: [PATCH v6 3/7] powercap/drivers/dtpm: Simplify the dtpm table

[Rafael J. Wysocki]

On Fri, Nov 26, 2021 at 6:40 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 26/11/2021 18:08, Doug Smythies wrote:
> > Hi Daniel,
> >
> > This patch introduces a regression, at least on my test system.
> > I can no longer change CPU frequency scaling drivers, for example
> > from intel_cpufreq (A.K.A intel_pstate in passive mode) to intel_pstate
> > (A.K.A. active mode). The task just hangs forever.
> >
> > I bisected the kernel and got this commit as the result.
> > As a double check, I reverted this commit:
> > 7a89d7eacf8e84f2afb94db5ae9d9f9faa93f01c
> > on kernel 5.16-rc2 and the issue was resolved.
> >
> > While your email is fairly old, I observe that it was only included as of
> > kernel 5.16-rc1.
>
> Could it be related to and fixed by:
>
> https://lore.kernel.org/all/20211108062345.273855-1-daniel.lezcano@linaro.org/
>
> ?

It seems so, but that patch is present in 5.16-rc2.

Re: [PATCH v6 3/7] powercap/drivers/dtpm: Simplify the dtpm table

[Doug Smythies]

On Fri, Nov 26, 2021 at 9:22 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> Hi Doug,
>
> On Fri, Nov 26, 2021 at 6:08 PM Doug Smythies <dsmythies@telus.net> wrote:
> >
> > Hi Daniel,
> >
> > This patch introduces a regression, at least on my test system.
> > I can no longer change CPU frequency scaling drivers, for example
> > from intel_cpufreq (A.K.A intel_pstate in passive mode) to intel_pstate
> > (A.K.A. active mode). The task just hangs forever.
> >
> > I bisected the kernel and got this commit as the result.
> > As a double check, I reverted this commit:
> > 7a89d7eacf8e84f2afb94db5ae9d9f9faa93f01c
> > on kernel 5.16-rc2 and the issue was resolved.
> >
> > While your email is fairly old, I observe that it was only included as of
> > kernel 5.16-rc1.
> >
> > Command Example that never completes:
> >
> > $ echo passive | sudo tee /sys/devices/system/cpu/intel_pstate/status
> >
> > syslog excerpt attached.
>
> This looks like it may be problematic:
>
> diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
> index f6076de39540..98841524a782 100644
> --- a/drivers/powercap/dtpm_cpu.c
> +++ b/drivers/powercap/dtpm_cpu.c
> @@ -204,7 +204,7 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
>        return ret;
> }
>
> -int dtpm_register_cpu(struct dtpm *parent)
> +static int __init dtpm_cpu_init(void)
> {
>        int ret;
>
> so please try to remove the __init annotation from dtpm_cpu_init() and
> see if that helps.

Hi Rafael,

That did not fix the issue.
Just to be clear this is what I did, on top of 5.16-rc2:

$ git diff
diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
index b740866b228d..26d1a87bdec6 100644
--- a/drivers/powercap/dtpm_cpu.c
+++ b/drivers/powercap/dtpm_cpu.c
@@ -231,7 +231,7 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
        return ret;
 }

-static int __init dtpm_cpu_init(void)
+static int dtpm_cpu_init(void)
 {
        int ret;

Re: [PATCH v6 3/7] powercap/drivers/dtpm: Simplify the dtpm table

[Rafael J. Wysocki]

On Fri, Nov 26, 2021 at 8:10 PM Doug Smythies <dsmythies@telus.net> wrote:
>
> On Fri, Nov 26, 2021 at 9:22 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > Hi Doug,
> >
> > On Fri, Nov 26, 2021 at 6:08 PM Doug Smythies <dsmythies@telus.net> wrote:
> > >
> > > Hi Daniel,
> > >
> > > This patch introduces a regression, at least on my test system.
> > > I can no longer change CPU frequency scaling drivers, for example
> > > from intel_cpufreq (A.K.A intel_pstate in passive mode) to intel_pstate
> > > (A.K.A. active mode). The task just hangs forever.
> > >
> > > I bisected the kernel and got this commit as the result.
> > > As a double check, I reverted this commit:
> > > 7a89d7eacf8e84f2afb94db5ae9d9f9faa93f01c
> > > on kernel 5.16-rc2 and the issue was resolved.
> > >
> > > While your email is fairly old, I observe that it was only included as of
> > > kernel 5.16-rc1.
> > >
> > > Command Example that never completes:
> > >
> > > $ echo passive | sudo tee /sys/devices/system/cpu/intel_pstate/status
> > >
> > > syslog excerpt attached.
> >
> > This looks like it may be problematic:
> >
> > diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
> > index f6076de39540..98841524a782 100644
> > --- a/drivers/powercap/dtpm_cpu.c
> > +++ b/drivers/powercap/dtpm_cpu.c
> > @@ -204,7 +204,7 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
> >        return ret;
> > }
> >
> > -int dtpm_register_cpu(struct dtpm *parent)
> > +static int __init dtpm_cpu_init(void)
> > {
> >        int ret;
> >
> > so please try to remove the __init annotation from dtpm_cpu_init() and
> > see if that helps.
>
> Hi Rafael,
>
> That did not fix the issue.
> Just to be clear this is what I did, on top of 5.16-rc2:
>
> $ git diff
> diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
> index b740866b228d..26d1a87bdec6 100644
> --- a/drivers/powercap/dtpm_cpu.c
> +++ b/drivers/powercap/dtpm_cpu.c
> @@ -231,7 +231,7 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
>         return ret;
>  }
>
> -static int __init dtpm_cpu_init(void)
> +static int dtpm_cpu_init(void)
>  {
>         int ret;

OK

This needs to be fixed or we'll need to revert commit
7a89d7eacf8e84f2afb94db5ae9d9f9faa93f01c for 5.16.

Re: [PATCH v6 3/7] powercap/drivers/dtpm: Simplify the dtpm table

[Doug Smythies]

On Fri, Nov 26, 2021 at 9:43 AM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 26/11/2021 18:21, Rafael J. Wysocki wrote:
> > Hi Doug,
> >
> > On Fri, Nov 26, 2021 at 6:08 PM Doug Smythies <dsmythies@telus.net> wrote:
> >>
> >> Hi Daniel,
> >>
> >> This patch introduces a regression, at least on my test system.
> >> I can no longer change CPU frequency scaling drivers, for example
> >> from intel_cpufreq (A.K.A intel_pstate in passive mode) to intel_pstate
> >> (A.K.A. active mode). The task just hangs forever.
> >>
> >> I bisected the kernel and got this commit as the result.
> >> As a double check, I reverted this commit:
> >> 7a89d7eacf8e84f2afb94db5ae9d9f9faa93f01c
> >> on kernel 5.16-rc2 and the issue was resolved.
> >>
> >> While your email is fairly old, I observe that it was only included as of
> >> kernel 5.16-rc1.
> >>
> >> Command Example that never completes:
> >>
> >> $ echo passive | sudo tee /sys/devices/system/cpu/intel_pstate/status
> >>
> >> syslog excerpt attached.
> >
> > This looks like it may be problematic:
> >
> > diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
> > index f6076de39540..98841524a782 100644
> > --- a/drivers/powercap/dtpm_cpu.c
> > +++ b/drivers/powercap/dtpm_cpu.c
> > @@ -204,7 +204,7 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
> >        return ret;
> > }
> >
> > -int dtpm_register_cpu(struct dtpm *parent)
> > +static int __init dtpm_cpu_init(void)
> > {
> >        int ret;
> >
> > so please try to remove the __init annotation from dtpm_cpu_init() and
> > see if that helps.
>
> Yes, actually that should be called only if it is configured properly.
> The dtpm_cpu just initializes itself unconditionally, I did not figured
> out there is the usually allyesconfig used by default by the distros.
>
> That should be fixed with a proper DT configuration [1]

I added your 5 patch set on top of 5.16-rc2 and confirm it fixes
the issue. I tested both ways, with CONFIG_OF not set, forcing the
CONFIG_DTPM stuff off, and with CONFIG_OF=y.

Oh, I used V2 of the patch set from earlier today.

... Doug

>
> [1]
> https://lore.kernel.org/all/20211124125506.2971069-3-daniel.lezcano@linaro.org/
>
> --
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v6 3/7] powercap/drivers/dtpm: Simplify the dtpm table

[Daniel Lezcano]

On 26/11/2021 22:56, Doug Smythies wrote:
> On Fri, Nov 26, 2021 at 9:43 AM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> On 26/11/2021 18:21, Rafael J. Wysocki wrote:
>>> Hi Doug,
>>>
>>> On Fri, Nov 26, 2021 at 6:08 PM Doug Smythies <dsmythies@telus.net> wrote:
>>>>
>>>> Hi Daniel,
>>>>
>>>> This patch introduces a regression, at least on my test system.
>>>> I can no longer change CPU frequency scaling drivers, for example
>>>> from intel_cpufreq (A.K.A intel_pstate in passive mode) to intel_pstate
>>>> (A.K.A. active mode). The task just hangs forever.
>>>>
>>>> I bisected the kernel and got this commit as the result.
>>>> As a double check, I reverted this commit:
>>>> 7a89d7eacf8e84f2afb94db5ae9d9f9faa93f01c
>>>> on kernel 5.16-rc2 and the issue was resolved.
>>>>
>>>> While your email is fairly old, I observe that it was only included as of
>>>> kernel 5.16-rc1.
>>>>
>>>> Command Example that never completes:
>>>>
>>>> $ echo passive | sudo tee /sys/devices/system/cpu/intel_pstate/status
>>>>
>>>> syslog excerpt attached.
>>>
>>> This looks like it may be problematic:
>>>
>>> diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
>>> index f6076de39540..98841524a782 100644
>>> --- a/drivers/powercap/dtpm_cpu.c
>>> +++ b/drivers/powercap/dtpm_cpu.c
>>> @@ -204,7 +204,7 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
>>>        return ret;
>>> }
>>>
>>> -int dtpm_register_cpu(struct dtpm *parent)
>>> +static int __init dtpm_cpu_init(void)
>>> {
>>>        int ret;
>>>
>>> so please try to remove the __init annotation from dtpm_cpu_init() and
>>> see if that helps.
>>
>> Yes, actually that should be called only if it is configured properly.
>> The dtpm_cpu just initializes itself unconditionally, I did not figured
>> out there is the usually allyesconfig used by default by the distros.
>>
>> That should be fixed with a proper DT configuration [1]
> 
> I added your 5 patch set on top of 5.16-rc2 and confirm it fixes
> the issue. I tested both ways, with CONFIG_OF not set, forcing the
> CONFIG_DTPM stuff off, and with CONFIG_OF=y.
> 
> Oh, I used V2 of the patch set from earlier today.

Thanks Doug for testing.

For a -rc I should prevent the cpu to setup at boot time with a simple fix.


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH] powercap/drivers/dtpm: Disable dtpm at boot time

[Daniel Lezcano]

The DTPM framework misses a mechanism to set it up. That is currently
under review but will come after the next cycle.

As the distro are enabling all the kernel options, the DTPM framework
is enabled on platforms where the energy model is not implemented,
thus making the framework inconsistent and disrupting the CPU
frequency scaling service.

Remove the initialization at boot time as a hot fix.

Fixes: 7a89d7eacf8e ("powercap/drivers/dtpm: Simplify the dtpm table")
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/powercap/dtpm.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c
index b9fac786246a..fb35c5828bfb 100644
--- a/drivers/powercap/dtpm.c
+++ b/drivers/powercap/dtpm.c
@@ -471,9 +471,6 @@ static int __init init_dtpm(void)
 		return PTR_ERR(pct);
 	}
 
-	for_each_dtpm_table(dtpm_descr)
-		dtpm_descr->init();
-
 	return 0;
 }
 late_initcall(init_dtpm);
-- 
2.25.1

Re: [PATCH] powercap/drivers/dtpm: Disable dtpm at boot time

[Daniel Lezcano]

Hi Doug,

I was unable to reproduce the issue because I don't have an x86 platform.

Is it possible to check if this fix is ok?

  -- Daniel

On 27/11/2021 00:08, Daniel Lezcano wrote:
> The DTPM framework misses a mechanism to set it up. That is currently
> under review but will come after the next cycle.
> 
> As the distro are enabling all the kernel options, the DTPM framework
> is enabled on platforms where the energy model is not implemented,
> thus making the framework inconsistent and disrupting the CPU
> frequency scaling service.
> 
> Remove the initialization at boot time as a hot fix.
> 
> Fixes: 7a89d7eacf8e ("powercap/drivers/dtpm: Simplify the dtpm table")
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/powercap/dtpm.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c
> index b9fac786246a..fb35c5828bfb 100644
> --- a/drivers/powercap/dtpm.c
> +++ b/drivers/powercap/dtpm.c
> @@ -471,9 +471,6 @@ static int __init init_dtpm(void)
>  		return PTR_ERR(pct);
>  	}
>  
> -	for_each_dtpm_table(dtpm_descr)
> -		dtpm_descr->init();
> -
>  	return 0;
>  }
>  late_initcall(init_dtpm);
> 


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH] powercap/drivers/dtpm: Disable dtpm at boot time

[Doug Smythies]

On Fri, Nov 26, 2021 at 3:10 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
>
> Hi Doug,
>
> I was unable to reproduce the issue because I don't have an x86 platform.
>
> Is it possible to check if this fix is ok?

Hi Daniel,

Yes, confirmed.

Tested-By: Doug Smythies <dsmythies@telus.net>

>
>   -- Daniel
>
> On 27/11/2021 00:08, Daniel Lezcano wrote:
> > The DTPM framework misses a mechanism to set it up. That is currently
> > under review but will come after the next cycle.
> >
> > As the distro are enabling all the kernel options, the DTPM framework
> > is enabled on platforms where the energy model is not implemented,
> > thus making the framework inconsistent and disrupting the CPU
> > frequency scaling service.
> >
> > Remove the initialization at boot time as a hot fix.
> >
> > Fixes: 7a89d7eacf8e ("powercap/drivers/dtpm: Simplify the dtpm table")
> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> > ---
> >  drivers/powercap/dtpm.c | 3 ---
> >  1 file changed, 3 deletions(-)
> >
> > diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c
> > index b9fac786246a..fb35c5828bfb 100644
> > --- a/drivers/powercap/dtpm.c
> > +++ b/drivers/powercap/dtpm.c
> > @@ -471,9 +471,6 @@ static int __init init_dtpm(void)
> >               return PTR_ERR(pct);
> >       }
> >
> > -     for_each_dtpm_table(dtpm_descr)
> > -             dtpm_descr->init();
> > -
> >       return 0;
> >  }
> >  late_initcall(init_dtpm);
> >
>
>
> --
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v6 3/7] powercap/drivers/dtpm: Simplify the dtpm table

[Daniel Lezcano]

On 26/11/2021 20:29, Rafael J. Wysocki wrote:
> On Fri, Nov 26, 2021 at 8:10 PM Doug Smythies <dsmythies@telus.net> wrote:
>>
>> On Fri, Nov 26, 2021 at 9:22 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>>>
>>> Hi Doug,
>>>
>>> On Fri, Nov 26, 2021 at 6:08 PM Doug Smythies <dsmythies@telus.net> wrote:
>>>>
>>>> Hi Daniel,
>>>>
>>>> This patch introduces a regression, at least on my test system.
>>>> I can no longer change CPU frequency scaling drivers, for example
>>>> from intel_cpufreq (A.K.A intel_pstate in passive mode) to intel_pstate
>>>> (A.K.A. active mode). The task just hangs forever.
>>>>
>>>> I bisected the kernel and got this commit as the result.
>>>> As a double check, I reverted this commit:
>>>> 7a89d7eacf8e84f2afb94db5ae9d9f9faa93f01c
>>>> on kernel 5.16-rc2 and the issue was resolved.

[ ... ]

>> -static int __init dtpm_cpu_init(void)
>> +static int dtpm_cpu_init(void)
>>  {
>>         int ret;
> 
> OK
> 
> This needs to be fixed or we'll need to revert commit
> 7a89d7eacf8e84f2afb94db5ae9d9f9faa93f01c for 5.16.

I've send a fix for that and Doug tested it (thanks) [1]

  -- Daniel

[1]
https://lore.kernel.org/all/CAAYoRsWEXoe_LjuHuQUL3Tdov0JVW887T4ciUTVOC410mZjgvA@mail.gmail.com/

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH] powercap/drivers/dtpm: Disable dtpm at boot time

[Rafael J. Wysocki]

On Sat, Nov 27, 2021 at 2:13 AM Doug Smythies <dsmythies@telus.net> wrote:
>
> On Fri, Nov 26, 2021 at 3:10 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
> >
> >
> > Hi Doug,
> >
> > I was unable to reproduce the issue because I don't have an x86 platform.
> >
> > Is it possible to check if this fix is ok?
>
> Hi Daniel,
>
> Yes, confirmed.
>
> Tested-By: Doug Smythies <dsmythies@telus.net>

Applied as 5.16-rc4 material, thanks!