[PATCH 0/6] RFC: hwmon: few improvements to amd_energy driver

[PATCH 0/6] RFC: hwmon: few improvements to amd_energy driver

[Naveen Krishna Chatradhi]

Hi Guenter,

Would like to know your feedback on the following features
for the amd_energy driver.

1) Sysfs entry for dumping energy counters of all the cores
    - On latest CPUs there can be as many as 128 cores.
      An ABI for dumping all 128 counters using seq_printf()
      to a debugfs/sysfs file would save a lot of cycles.

2) Enable/Disable the accumulation, Disabled by default
    - The accumulator thread may introduce some noise.
      Providing a knob to enable/disable (start/stop) the
      accumulation in software.

3) Accumulator Interval change based on reported resolution
    - Frequency of the accumulator thread can be set during
      the probe based on fine grain (1.625 micro J) or course
      grain (0.125 milli J) resolutions.

Akshay Gupta (1):
  hwmon: amd_energy: Move label out of accumulation structure

Naveen Krishna Chatradhi (5):
  hwmon: amd_energy: optimize accumulation interval
  hwmon: amd_energy: Improve the accumulation logic
  hwmon: amd_energy: let user enable/disable the sw accumulation
  hwmon: amd_energy: dump energy counters via debugfs
  hwmon: (amd_energy) Update driver documentation

 Documentation/hwmon/amd_energy.rst |  19 ++
 drivers/hwmon/amd_energy.c         | 351 +++++++++++++++++++++--------
 2 files changed, 273 insertions(+), 97 deletions(-)

-- 
2.26.2

[PATCH 1/6] hwmon: amd_energy: Move label out of accumulation structure

[Naveen Krishna Chatradhi]

From: Akshay Gupta <Akshay.Gupta@amd.com>

At present, core & socket labels are defined in struct sensor_accumulator
This patch moves it to the amd_energy_data structure, which will
help in calling memset on struct sensor_accumulator to optimize the code.

Signed-off-by: Akshay Gupta <Akshay.Gupta@amd.com>
---
 drivers/hwmon/amd_energy.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c
index 29603742c858..9580a16185b8 100644
--- a/drivers/hwmon/amd_energy.c
+++ b/drivers/hwmon/amd_energy.c
@@ -35,7 +35,6 @@
 struct sensor_accumulator {
 	u64 energy_ctr;
 	u64 prev_value;
-	char label[10];
 };
 
 struct amd_energy_data {
@@ -52,6 +51,7 @@ struct amd_energy_data {
 	int nr_cpus;
 	int nr_socks;
 	int core_id;
+	char (*label)[10];
 };
 
 static int amd_energy_read_labels(struct device *dev,
@@ -61,7 +61,7 @@ static int amd_energy_read_labels(struct device *dev,
 {
 	struct amd_energy_data *data = dev_get_drvdata(dev);
 
-	*str = data->accums[channel].label;
+	*str = data->label[channel];
 	return 0;
 }
 
@@ -253,6 +253,7 @@ static int amd_create_sensor(struct device *dev,
 	struct sensor_accumulator *accums;
 	int i, num_siblings, cpus, sockets;
 	u32 *s_config;
+	char (*label_l)[10];
 
 	/* Identify the number of siblings per core */
 	num_siblings = ((cpuid_ebx(0x8000001e) >> 8) & 0xff) + 1;
@@ -276,21 +277,25 @@ static int amd_create_sensor(struct device *dev,
 	if (!accums)
 		return -ENOMEM;
 
+	label_l = devm_kcalloc(dev, cpus + sockets,
+			       sizeof(*label_l), GFP_KERNEL);
+	if (!label_l)
+		return -ENOMEM;
+
 	info->type = type;
 	info->config = s_config;
 
 	data->nr_cpus = cpus;
 	data->nr_socks = sockets;
 	data->accums = accums;
+	data->label = label_l;
 
 	for (i = 0; i < cpus + sockets; i++) {
 		s_config[i] = config;
 		if (i < cpus)
-			scnprintf(accums[i].label, 10,
-				  "Ecore%03u", i);
+			scnprintf(label_l[i], 10, "Ecore%03u", i);
 		else
-			scnprintf(accums[i].label, 10,
-				  "Esocket%u", (i - cpus));
+			scnprintf(label_l[i], 10, "Esocket%u", (i - cpus));
 	}
 
 	return 0;
-- 
2.26.2

[PATCH 2/6] hwmon: amd_energy: optimize accumulation interval

[Naveen Krishna Chatradhi]

On a system with course grain resolution of energy unit (milli J) the
accumulation thread can be executed less frequently than on the system
with fine grain resolution(micro J).

This patch sets the accumulation thread interval to an optimum value
calculated based on the (energy unit) resolution supported by the
hardware (assuming a peak wattage of 240W).

Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
---
 drivers/hwmon/amd_energy.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c
index 9580a16185b8..f0a13d6cc419 100644
--- a/drivers/hwmon/amd_energy.c
+++ b/drivers/hwmon/amd_energy.c
@@ -48,6 +48,7 @@ struct amd_energy_data {
 	struct sensor_accumulator *accums;
 	/* Energy Status Units */
 	u64 energy_units;
+	unsigned int timeout;
 	int nr_cpus;
 	int nr_socks;
 	int core_id;
@@ -215,6 +216,7 @@ static umode_t amd_energy_is_visible(const void *_data,
 static int energy_accumulator(void *p)
 {
 	struct amd_energy_data *data = (struct amd_energy_data *)p;
+	unsigned int timeout = data->timeout;
 
 	while (!kthread_should_stop()) {
 		/*
@@ -234,7 +236,7 @@ static int energy_accumulator(void *p)
 		 *
 		 * let us accumulate for every 100secs
 		 */
-		schedule_timeout(msecs_to_jiffies(100000));
+		schedule_timeout(msecs_to_jiffies(timeout));
 	}
 	return 0;
 }
@@ -331,6 +333,14 @@ static int amd_energy_probe(struct platform_device *pdev)
 	if (IS_ERR(hwmon_dev))
 		return PTR_ERR(hwmon_dev);
 
+	/* Once in 3 minutes for a resolution of 1/2*16 */
+	if (data->energy_units == 0x10)
+		data->timeout = 3 * 60;
+
+	/* Once in 3 days for a resolution of 1/2^6 */
+	if (data->energy_units == 0x6)
+		data->timeout = 3 * 24 * 60 * 60;
+
 	data->wrap_accumulate = kthread_run(energy_accumulator, data,
 					    "%s", dev_name(hwmon_dev));
 	if (IS_ERR(data->wrap_accumulate))
-- 
2.26.2

[PATCH 3/6] hwmon: amd_energy: Improve the accumulation logic

[Naveen Krishna Chatradhi]

Factor out the common code in the accumulation functions for core and
socket accumulation.

While at it, handle the return value of the amd_create_sensor() function.

Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
---
 drivers/hwmon/amd_energy.c | 126 +++++++++++++------------------------
 1 file changed, 45 insertions(+), 81 deletions(-)

diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c
index f0a13d6cc419..96c61784d05c 100644
--- a/drivers/hwmon/amd_energy.c
+++ b/drivers/hwmon/amd_energy.c
@@ -74,108 +74,67 @@ static void get_energy_units(struct amd_energy_data *data)
 	data->energy_units = (rapl_units & AMD_ENERGY_UNIT_MASK) >> 8;
 }
 
-static void accumulate_socket_delta(struct amd_energy_data *data,
-				    int sock, int cpu)
+static void accumulate_delta(struct amd_energy_data *data,
+			     int channel, int cpu, u32 reg)
 {
-	struct sensor_accumulator *s_accum;
+	struct sensor_accumulator *accum;
 	u64 input;
 
 	mutex_lock(&data->lock);
-	rdmsrl_safe_on_cpu(cpu, ENERGY_PKG_MSR, &input);
+	rdmsrl_safe_on_cpu(cpu, reg, &input);
 	input &= AMD_ENERGY_MASK;
 
-	s_accum = &data->accums[data->nr_cpus + sock];
-	if (input >= s_accum->prev_value)
-		s_accum->energy_ctr +=
-			input - s_accum->prev_value;
+	accum = &data->accums[channel];
+	if (input >= accum->prev_value)
+		accum->energy_ctr +=
+			input - accum->prev_value;
 	else
-		s_accum->energy_ctr += UINT_MAX -
-			s_accum->prev_value + input;
+		accum->energy_ctr += UINT_MAX -
+			accum->prev_value + input;
 
-	s_accum->prev_value = input;
+	accum->prev_value = input;
 	mutex_unlock(&data->lock);
 }
 
-static void accumulate_core_delta(struct amd_energy_data *data)
+static void read_accumulate(struct amd_energy_data *data)
 {
-	struct sensor_accumulator *c_accum;
-	u64 input;
-	int cpu;
+	int sock, scpu, cpu;
+
+	for (sock = 0; sock < data->nr_socks; sock++) {
+		scpu = cpumask_first_and(cpu_online_mask,
+					 cpumask_of_node(sock));
+
+		accumulate_delta(data, data->nr_cpus + sock,
+				 scpu, ENERGY_PKG_MSR);
+	}
 
-	mutex_lock(&data->lock);
 	if (data->core_id >= data->nr_cpus)
 		data->core_id = 0;
 
 	cpu = data->core_id;
+	if (cpu_online(cpu))
+		accumulate_delta(data, cpu, cpu, ENERGY_CORE_MSR);
 
-	if (!cpu_online(cpu))
-		goto out;
-
-	rdmsrl_safe_on_cpu(cpu, ENERGY_CORE_MSR, &input);
-	input &= AMD_ENERGY_MASK;
-
-	c_accum = &data->accums[cpu];
-
-	if (input >= c_accum->prev_value)
-		c_accum->energy_ctr +=
-			input - c_accum->prev_value;
-	else
-		c_accum->energy_ctr += UINT_MAX -
-			c_accum->prev_value + input;
-
-	c_accum->prev_value = input;
-
-out:
 	data->core_id++;
-	mutex_unlock(&data->lock);
-}
-
-static void read_accumulate(struct amd_energy_data *data)
-{
-	int sock;
-
-	for (sock = 0; sock < data->nr_socks; sock++) {
-		int cpu;
-
-		cpu = cpumask_first_and(cpu_online_mask,
-					cpumask_of_node(sock));
-
-		accumulate_socket_delta(data, sock, cpu);
-	}
-
-	accumulate_core_delta(data);
 }
 
 static void amd_add_delta(struct amd_energy_data *data, int ch,
-			  int cpu, long *val, bool is_core)
+			  int cpu, long *val, u32 reg)
 {
-	struct sensor_accumulator *s_accum, *c_accum;
+	struct sensor_accumulator *accum;
 	u64 input;
 
 	mutex_lock(&data->lock);
-	if (!is_core) {
-		rdmsrl_safe_on_cpu(cpu, ENERGY_PKG_MSR, &input);
-		input &= AMD_ENERGY_MASK;
-
-		s_accum = &data->accums[ch];
-		if (input >= s_accum->prev_value)
-			input += s_accum->energy_ctr -
-				  s_accum->prev_value;
-		else
-			input += UINT_MAX - s_accum->prev_value +
-				  s_accum->energy_ctr;
-	} else {
-		rdmsrl_safe_on_cpu(cpu, ENERGY_CORE_MSR, &input);
-		input &= AMD_ENERGY_MASK;
+	rdmsrl_safe_on_cpu(cpu, reg, &input);
+	input &= AMD_ENERGY_MASK;
 
-		c_accum = &data->accums[ch];
-		if (input >= c_accum->prev_value)
-			input += c_accum->energy_ctr -
-				 c_accum->prev_value;
-		else
-			input += UINT_MAX - c_accum->prev_value +
-				 c_accum->energy_ctr;
-	}
+	accum = &data->accums[ch];
+	if (input >= accum->prev_value)
+		input += accum->energy_ctr -
+				accum->prev_value;
+	else
+		input += UINT_MAX - accum->prev_value +
+				accum->energy_ctr;
 
 	/* Energy consumed = (1/(2^ESU) * RAW * 1000000UL) μJoules */
 	*val = div64_ul(input * 1000000UL, BIT(data->energy_units));
@@ -188,20 +147,22 @@ static int amd_energy_read(struct device *dev,
 			   u32 attr, int channel, long *val)
 {
 	struct amd_energy_data *data = dev_get_drvdata(dev);
+	u32 reg;
 	int cpu;
 
 	if (channel >= data->nr_cpus) {
 		cpu = cpumask_first_and(cpu_online_mask,
 					cpumask_of_node
 					(channel - data->nr_cpus));
-		amd_add_delta(data, channel, cpu, val, false);
+		reg = ENERGY_PKG_MSR;
 	} else {
 		cpu = channel;
 		if (!cpu_online(cpu))
 			return -ENODEV;
 
-		amd_add_delta(data, channel, cpu, val, true);
+		reg = ENERGY_CORE_MSR;
 	}
+	amd_add_delta(data, channel, cpu, val, reg);
 
 	return 0;
 }
@@ -249,7 +210,7 @@ static const struct hwmon_ops amd_energy_ops = {
 
 static int amd_create_sensor(struct device *dev,
 			     struct amd_energy_data *data,
-			     u8 type, u32 config)
+			     enum hwmon_sensor_types type, u32 config)
 {
 	struct hwmon_channel_info *info = &data->energy_info;
 	struct sensor_accumulator *accums;
@@ -308,6 +269,7 @@ static int amd_energy_probe(struct platform_device *pdev)
 	struct device *hwmon_dev;
 	struct amd_energy_data *data;
 	struct device *dev = &pdev->dev;
+	int ret;
 
 	data = devm_kzalloc(dev,
 			    sizeof(struct amd_energy_data), GFP_KERNEL);
@@ -320,8 +282,10 @@ static int amd_energy_probe(struct platform_device *pdev)
 	dev_set_drvdata(dev, data);
 	/* Populate per-core energy reporting */
 	data->info[0] = &data->energy_info;
-	amd_create_sensor(dev, data, hwmon_energy,
-			  HWMON_E_INPUT | HWMON_E_LABEL);
+	ret = amd_create_sensor(dev, data, hwmon_energy,
+				HWMON_E_INPUT | HWMON_E_LABEL);
+	if (ret)
+		return ret;
 
 	mutex_init(&data->lock);
 	get_energy_units(data);
@@ -346,7 +310,7 @@ static int amd_energy_probe(struct platform_device *pdev)
 	if (IS_ERR(data->wrap_accumulate))
 		return PTR_ERR(data->wrap_accumulate);
 
-	return PTR_ERR_OR_ZERO(data->wrap_accumulate);
+	return 0;
 }
 
 static int amd_energy_remove(struct platform_device *pdev)
-- 
2.26.2

[PATCH 4/6] hwmon: amd_energy: let user enable/disable the sw accumulation

[Naveen Krishna Chatradhi]

Provide an option "accumulator_status" via sysfs to enable/disable
the software accumulation of energy counters.

Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
---
 drivers/hwmon/amd_energy.c | 104 ++++++++++++++++++++++++++++++-------
 1 file changed, 86 insertions(+), 18 deletions(-)

diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c
index 96c61784d05c..c294bea56c02 100644
--- a/drivers/hwmon/amd_energy.c
+++ b/drivers/hwmon/amd_energy.c
@@ -32,6 +32,8 @@
 #define AMD_ENERGY_UNIT_MASK	0x01F00
 #define AMD_ENERGY_MASK		0xFFFFFFFF
 
+static struct device_attribute accumulate_attr;
+
 struct sensor_accumulator {
 	u64 energy_ctr;
 	u64 prev_value;
@@ -42,10 +44,12 @@ struct amd_energy_data {
 	const struct hwmon_channel_info *info[2];
 	struct hwmon_chip_info chip;
 	struct task_struct *wrap_accumulate;
+	struct device *hwmon_dev;
 	/* Lock around the accumulator */
 	struct mutex lock;
 	/* An accumulator for each core and socket */
 	struct sensor_accumulator *accums;
+	bool accumulator_status;
 	/* Energy Status Units */
 	u64 energy_units;
 	unsigned int timeout;
@@ -128,13 +132,15 @@ static void amd_add_delta(struct amd_energy_data *data, int ch,
 	rdmsrl_safe_on_cpu(cpu, reg, &input);
 	input &= AMD_ENERGY_MASK;
 
-	accum = &data->accums[ch];
-	if (input >= accum->prev_value)
-		input += accum->energy_ctr -
-				accum->prev_value;
-	else
-		input += UINT_MAX - accum->prev_value +
-				accum->energy_ctr;
+	if (data->accumulator_status) {
+		accum = &data->accums[ch];
+		if (input >= accum->prev_value)
+			input += accum->energy_ctr -
+					accum->prev_value;
+		else
+			input += UINT_MAX - accum->prev_value +
+					accum->energy_ctr;
+	}
 
 	/* Energy consumed = (1/(2^ESU) * RAW * 1000000UL) μJoules */
 	*val = div64_ul(input * 1000000UL, BIT(data->energy_units));
@@ -264,9 +270,67 @@ static int amd_create_sensor(struct device *dev,
 	return 0;
 }
 
+static ssize_t amd_energy_accumulate_show(struct device *dev,
+					  struct device_attribute *dev_attr,
+					  char *buf)
+{
+	struct amd_energy_data *data = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%d\n", data->accumulator_status);
+}
+
+static ssize_t amd_energy_accumulate_store(struct device *dev,
+					   struct device_attribute *dev_attr,
+					   const char *buf, size_t count)
+{
+	struct amd_energy_data *data = dev_get_drvdata(dev);
+	bool input;
+	int ret;
+
+	ret = kstrtobool(buf, &input);
+	if (ret)
+		return ret;
+
+	if (data->accumulator_status == input)
+		return count;
+
+	if (input) {
+		memset(data->accums, 0, (data->nr_cpus + data->nr_socks) *
+			sizeof(struct sensor_accumulator));
+
+		if (!data->wrap_accumulate) {
+			data->wrap_accumulate =
+				kthread_run(energy_accumulator,
+					    data, "%s", dev_name(dev));
+			if (IS_ERR(data->wrap_accumulate))
+				return PTR_ERR(data->wrap_accumulate);
+		}
+	} else {
+		if (data && data->wrap_accumulate) {
+			ret = kthread_stop(data->wrap_accumulate);
+			if (ret)
+				return ret;
+			data->wrap_accumulate = NULL;
+		}
+	}
+	data->accumulator_status = input;
+
+	return count;
+}
+
+static int create_accumulate_status_file(struct amd_energy_data *data)
+{
+	accumulate_attr.attr.name = "accumulator_status";
+	accumulate_attr.attr.mode = 0664;
+	accumulate_attr.show = amd_energy_accumulate_show;
+	accumulate_attr.store = amd_energy_accumulate_store;
+
+	return sysfs_create_file(&data->hwmon_dev->kobj,
+				 &accumulate_attr.attr);
+}
+
 static int amd_energy_probe(struct platform_device *pdev)
 {
-	struct device *hwmon_dev;
 	struct amd_energy_data *data;
 	struct device *dev = &pdev->dev;
 	int ret;
@@ -290,12 +354,12 @@ static int amd_energy_probe(struct platform_device *pdev)
 	mutex_init(&data->lock);
 	get_energy_units(data);
 
-	hwmon_dev = devm_hwmon_device_register_with_info(dev, DRVNAME,
-							 data,
-							 &data->chip,
-							 NULL);
-	if (IS_ERR(hwmon_dev))
-		return PTR_ERR(hwmon_dev);
+	data->hwmon_dev = devm_hwmon_device_register_with_info(dev, DRVNAME,
+							       data,
+							       &data->chip,
+							       NULL);
+	if (IS_ERR(data->hwmon_dev))
+		return PTR_ERR(data->hwmon_dev);
 
 	/* Once in 3 minutes for a resolution of 1/2*16 */
 	if (data->energy_units == 0x10)
@@ -305,10 +369,12 @@ static int amd_energy_probe(struct platform_device *pdev)
 	if (data->energy_units == 0x6)
 		data->timeout = 3 * 24 * 60 * 60;
 
-	data->wrap_accumulate = kthread_run(energy_accumulator, data,
-					    "%s", dev_name(hwmon_dev));
-	if (IS_ERR(data->wrap_accumulate))
-		return PTR_ERR(data->wrap_accumulate);
+	/* Disabling the energy accumulation by default */
+	data->accumulator_status = 0;
+
+	ret = create_accumulate_status_file(data);
+	if (ret)
+		return ret;
 
 	return 0;
 }
@@ -317,6 +383,8 @@ static int amd_energy_remove(struct platform_device *pdev)
 {
 	struct amd_energy_data *data = dev_get_drvdata(&pdev->dev);
 
+	sysfs_remove_file(&data->hwmon_dev->kobj, &accumulate_attr.attr);
+
 	if (data && data->wrap_accumulate)
 		kthread_stop(data->wrap_accumulate);
 
-- 
2.26.2

[PATCH 5/6] hwmon: amd_energy: dump energy counters via debugfs

[Naveen Krishna Chatradhi]

Use seq_printf to capture the core and socket energies under debugfs
path in '/sys/kernel/debug/amd_energy/'
file cenergy_dump: To print out the core energy counters
file senergy_dump: To print out the socket energy counters

Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
---
 drivers/hwmon/amd_energy.c | 110 +++++++++++++++++++++++++++++++++++++
 1 file changed, 110 insertions(+)

diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c
index c294bea56c02..2184bd4510ed 100644
--- a/drivers/hwmon/amd_energy.c
+++ b/drivers/hwmon/amd_energy.c
@@ -8,6 +8,7 @@
 #include <linux/bits.h>
 #include <linux/cpu.h>
 #include <linux/cpumask.h>
+#include <linux/debugfs.h>
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/hwmon.h>
@@ -20,6 +21,7 @@
 #include <linux/platform_device.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
+#include <linux/smp.h>
 #include <linux/topology.h>
 #include <linux/types.h>
 
@@ -57,6 +59,8 @@ struct amd_energy_data {
 	int nr_socks;
 	int core_id;
 	char (*label)[10];
+	u64 *cdump;
+	u64 *sdump;
 };
 
 static int amd_energy_read_labels(struct device *dev,
@@ -329,6 +333,108 @@ static int create_accumulate_status_file(struct amd_energy_data *data)
 				 &accumulate_attr.attr);
 }
 
+#ifdef CONFIG_DEBUG_FS
+static void dump_on_each_cpu(void *info)
+{
+	struct amd_energy_data *data = info;
+	int cpu = smp_processor_id();
+
+	amd_add_delta(data, cpu, cpu, (long *)&data->cdump[cpu],
+		      ENERGY_CORE_MSR);
+}
+
+static int cenergy_dump_show(struct seq_file *s, void *unused)
+{
+	struct amd_energy_data *data = s->private;
+	struct cpumask *cpus_mask;
+	int i;
+
+	cpus_mask = kmalloc(sizeof(*cpus_mask), GFP_KERNEL);
+	memset(data->cdump, 0, (data->nr_cpus) * sizeof(u64));
+	cpumask_clear(cpus_mask);
+	for (i = 0; i < data->nr_cpus; i++) {
+		if (cpu_online(i))
+			cpumask_set_cpu(i, cpus_mask);
+	}
+
+	on_each_cpu_mask(cpus_mask, dump_on_each_cpu, data, true);
+
+	for (i = 0; i < data->nr_cpus; i++) {
+		if (!(i & 3))
+			seq_printf(s, "Core %3d: ", i);
+
+		seq_printf(s, "%16llu ", data->cdump[i]);
+		if ((i & 3) == 3)
+			seq_puts(s, "\n");
+	}
+	seq_puts(s, "\n");
+
+	kfree(cpus_mask);
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(cenergy_dump);
+
+static int senergy_dump_show(struct seq_file *s, void *unused)
+{
+	struct amd_energy_data *data = s->private;
+	int i, cpu;
+
+	for (i = 0; i < data->nr_socks; i++) {
+		cpu = cpumask_first_and(cpu_online_mask,
+					cpumask_of_node(i));
+		amd_add_delta(data, data->nr_cpus + i, cpu,
+			      (long *)&data->sdump[i], ENERGY_PKG_MSR);
+		seq_printf(s, "Socket %1d: %16llu\n",
+			   i, data->sdump[i]);
+	}
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(senergy_dump);
+
+static void dump_debugfs_cleanup(void *ddir)
+{
+	debugfs_remove_recursive(ddir);
+}
+
+static int create_dump_file(struct device *dev,
+			    struct amd_energy_data *data)
+{
+	struct dentry *debugfs;
+	char name[] = "amd_energy";
+
+	data->cdump = devm_kcalloc(dev, data->nr_cpus,
+				   sizeof(u64), GFP_KERNEL);
+	if (!data->cdump)
+		return -ENOMEM;
+
+	data->sdump = devm_kcalloc(dev, data->nr_socks,
+				   sizeof(u64), GFP_KERNEL);
+	if (!data->sdump)
+		return -ENOMEM;
+
+	debugfs = debugfs_create_dir(name, NULL);
+	if (debugfs) {
+		debugfs_create_file("cenergy_dump", 0440,
+				    debugfs, data, &cenergy_dump_fops);
+		debugfs_create_file("senergy_dump", 0440,
+				    debugfs, data, &senergy_dump_fops);
+		devm_add_action_or_reset(data->hwmon_dev,
+					 dump_debugfs_cleanup, debugfs);
+	}
+
+	return 0;
+}
+#else
+
+static int create_dump_file(struct device *dev,
+			    struct amd_energy_data *data)
+{
+	return 0;
+}
+
+#endif //CONFIG_DEBUG_FS
+
 static int amd_energy_probe(struct platform_device *pdev)
 {
 	struct amd_energy_data *data;
@@ -376,6 +482,10 @@ static int amd_energy_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ret = create_dump_file(dev, data);
+	if (ret)
+		return ret;
+
 	return 0;
 }
 
-- 
2.26.2

[PATCH 6/6] hwmon: (amd_energy) Update driver documentation

[Naveen Krishna Chatradhi]

Update the documentation of the newly added features
1. Set the accumulation interval based on resolution
2. Debugfs entries to capture the counters of all
   the cores and sockets
3. Control the software accumulation

Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
---
 Documentation/hwmon/amd_energy.rst | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/Documentation/hwmon/amd_energy.rst b/Documentation/hwmon/amd_energy.rst
index f8288edff664..fe145ad158dd 100644
--- a/Documentation/hwmon/amd_energy.rst
+++ b/Documentation/hwmon/amd_energy.rst
@@ -84,9 +84,28 @@ per run to a respective 64-bit counter. The kernel thread starts
 running during probe, wakes up every 100secs and stops running
 when driver is removed.
 
+Frequency of the accumulator thread is set during the probe
+based on the chosen energy unit resolution.
+A. fine grain (1.625 micro J)
+B. course grain (0.125 milli J)
+
 A socket and core energy read would return the current register
 value added to the respective energy accumulator.
 
+The energy counters of all the core and sockets are available
+under debugfs path in '/sys/kernel/debug/amd_energy/'
+
+file cenergy_dump: To print out the core energy counters
+file senergy_dump: To print out the socket energy counters
+
+Control the Energy accumulation
+---------------------------------
+
+The software accumulation of energy counters is disabled by default.
+
+A sysfs entry "accumulator_status" is provided to enable/disable
+the same.
+
 Sysfs attributes
 ----------------
 
-- 
2.26.2

Re: [PATCH 2/6] hwmon: amd_energy: optimize accumulation interval

[Guenter Roeck]

On 9/5/20 7:32 AM, Naveen Krishna Chatradhi wrote:
> On a system with course grain resolution of energy unit (milli J) the
> accumulation thread can be executed less frequently than on the system
> with fine grain resolution(micro J).
> 
> This patch sets the accumulation thread interval to an optimum value
> calculated based on the (energy unit) resolution supported by the
> hardware (assuming a peak wattage of 240W).
> 
> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
> ---
>  drivers/hwmon/amd_energy.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c
> index 9580a16185b8..f0a13d6cc419 100644
> --- a/drivers/hwmon/amd_energy.c
> +++ b/drivers/hwmon/amd_energy.c
> @@ -48,6 +48,7 @@ struct amd_energy_data {
>  	struct sensor_accumulator *accums;
>  	/* Energy Status Units */
>  	u64 energy_units;
> +	unsigned int timeout;
>  	int nr_cpus;
>  	int nr_socks;
>  	int core_id;
> @@ -215,6 +216,7 @@ static umode_t amd_energy_is_visible(const void *_data,
>  static int energy_accumulator(void *p)
>  {
>  	struct amd_energy_data *data = (struct amd_energy_data *)p;
> +	unsigned int timeout = data->timeout;
>  
>  	while (!kthread_should_stop()) {
>  		/*
> @@ -234,7 +236,7 @@ static int energy_accumulator(void *p)
>  		 *
>  		 * let us accumulate for every 100secs
>  		 */
> -		schedule_timeout(msecs_to_jiffies(100000));
> +		schedule_timeout(msecs_to_jiffies(timeout));

Numbers below are in seconds, used as milli-seconds here.

>  	}
>  	return 0;
>  }
> @@ -331,6 +333,14 @@ static int amd_energy_probe(struct platform_device *pdev)
>  	if (IS_ERR(hwmon_dev))
>  		return PTR_ERR(hwmon_dev);
>  
> +	/* Once in 3 minutes for a resolution of 1/2*16 */

* 16 or ^ 16 ?

> +	if (data->energy_units == 0x10)
> +		data->timeout = 3 * 60;

180 ms ? I assume this is a bug and meant to be 3 * 60 * 1000.

> +
> +	/* Once in 3 days for a resolution of 1/2^6 */
> +	if (data->energy_units == 0x6)
> +		data->timeout = 3 * 24 * 60 * 60;
> +

... and else ?

This needs to cover all cases, including those not currently existing.
I would suggest to define a formula based on data->energy_units.
The energy units value can be anything from 0..31. Based on your numbers,
something like
    timeout_ms = BIT(34 - data->energy_units);
should do. It translates to about 3.1 days for energy_units=6, and
4.3 minutes for energy_units=16. If that is too much, you can make it
   timeout_ms = BIT(33 - data->energy_units);

To avoid overflow, it might make sense to max out at BIT(31).
    timeout_ms = BIT((min(31, 33 - data->energy_units));

Guenter

>  	data->wrap_accumulate = kthread_run(energy_accumulator, data,
>  					    "%s", dev_name(hwmon_dev));
>  	if (IS_ERR(data->wrap_accumulate))
> 

Re: [PATCH 3/6] hwmon: amd_energy: Improve the accumulation logic

[Guenter Roeck]

On 9/5/20 7:32 AM, Naveen Krishna Chatradhi wrote:
> Factor out the common code in the accumulation functions for core and
> socket accumulation.
> 
> While at it, handle the return value of the amd_create_sensor() function.
> 
> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
> ---
>  drivers/hwmon/amd_energy.c | 126 +++++++++++++------------------------
>  1 file changed, 45 insertions(+), 81 deletions(-)
> 
> diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c
> index f0a13d6cc419..96c61784d05c 100644
> --- a/drivers/hwmon/amd_energy.c
> +++ b/drivers/hwmon/amd_energy.c
> @@ -74,108 +74,67 @@ static void get_energy_units(struct amd_energy_data *data)
>  	data->energy_units = (rapl_units & AMD_ENERGY_UNIT_MASK) >> 8;
>  }
>  
> -static void accumulate_socket_delta(struct amd_energy_data *data,
> -				    int sock, int cpu)
> +static void accumulate_delta(struct amd_energy_data *data,
> +			     int channel, int cpu, u32 reg)
>  {
> -	struct sensor_accumulator *s_accum;
> +	struct sensor_accumulator *accum;
>  	u64 input;
>  
>  	mutex_lock(&data->lock);
> -	rdmsrl_safe_on_cpu(cpu, ENERGY_PKG_MSR, &input);
> +	rdmsrl_safe_on_cpu(cpu, reg, &input);
>  	input &= AMD_ENERGY_MASK;
>  
> -	s_accum = &data->accums[data->nr_cpus + sock];
> -	if (input >= s_accum->prev_value)
> -		s_accum->energy_ctr +=
> -			input - s_accum->prev_value;
> +	accum = &data->accums[channel];
> +	if (input >= accum->prev_value)
> +		accum->energy_ctr +=
> +			input - accum->prev_value;
>  	else
> -		s_accum->energy_ctr += UINT_MAX -
> -			s_accum->prev_value + input;
> +		accum->energy_ctr += UINT_MAX -
> +			accum->prev_value + input;
>  
> -	s_accum->prev_value = input;
> +	accum->prev_value = input;
>  	mutex_unlock(&data->lock);
>  }
>  
> -static void accumulate_core_delta(struct amd_energy_data *data)
> +static void read_accumulate(struct amd_energy_data *data)
>  {
> -	struct sensor_accumulator *c_accum;
> -	u64 input;
> -	int cpu;
> +	int sock, scpu, cpu;
> +
> +	for (sock = 0; sock < data->nr_socks; sock++) {
> +		scpu = cpumask_first_and(cpu_online_mask,
> +					 cpumask_of_node(sock));
> +
> +		accumulate_delta(data, data->nr_cpus + sock,
> +				 scpu, ENERGY_PKG_MSR);
> +	}
>  
> -	mutex_lock(&data->lock);
>  	if (data->core_id >= data->nr_cpus)
>  		data->core_id = 0;
>  
>  	cpu = data->core_id;
> +	if (cpu_online(cpu))
> +		accumulate_delta(data, cpu, cpu, ENERGY_CORE_MSR);
>  
> -	if (!cpu_online(cpu))
> -		goto out;
> -
> -	rdmsrl_safe_on_cpu(cpu, ENERGY_CORE_MSR, &input);
> -	input &= AMD_ENERGY_MASK;
> -
> -	c_accum = &data->accums[cpu];
> -
> -	if (input >= c_accum->prev_value)
> -		c_accum->energy_ctr +=
> -			input - c_accum->prev_value;
> -	else
> -		c_accum->energy_ctr += UINT_MAX -
> -			c_accum->prev_value + input;
> -
> -	c_accum->prev_value = input;
> -
> -out:
>  	data->core_id++;
> -	mutex_unlock(&data->lock);
> -}
> -
> -static void read_accumulate(struct amd_energy_data *data)
> -{
> -	int sock;
> -
> -	for (sock = 0; sock < data->nr_socks; sock++) {
> -		int cpu;
> -
> -		cpu = cpumask_first_and(cpu_online_mask,
> -					cpumask_of_node(sock));
> -
> -		accumulate_socket_delta(data, sock, cpu);
> -	}
> -
> -	accumulate_core_delta(data);
>  }
>  
>  static void amd_add_delta(struct amd_energy_data *data, int ch,
> -			  int cpu, long *val, bool is_core)
> +			  int cpu, long *val, u32 reg)
>  {
> -	struct sensor_accumulator *s_accum, *c_accum;
> +	struct sensor_accumulator *accum;
>  	u64 input;
>  
>  	mutex_lock(&data->lock);
> -	if (!is_core) {
> -		rdmsrl_safe_on_cpu(cpu, ENERGY_PKG_MSR, &input);
> -		input &= AMD_ENERGY_MASK;
> -
> -		s_accum = &data->accums[ch];
> -		if (input >= s_accum->prev_value)
> -			input += s_accum->energy_ctr -
> -				  s_accum->prev_value;
> -		else
> -			input += UINT_MAX - s_accum->prev_value +
> -				  s_accum->energy_ctr;
> -	} else {
> -		rdmsrl_safe_on_cpu(cpu, ENERGY_CORE_MSR, &input);
> -		input &= AMD_ENERGY_MASK;
> +	rdmsrl_safe_on_cpu(cpu, reg, &input);
> +	input &= AMD_ENERGY_MASK;
>  
> -		c_accum = &data->accums[ch];
> -		if (input >= c_accum->prev_value)
> -			input += c_accum->energy_ctr -
> -				 c_accum->prev_value;
> -		else
> -			input += UINT_MAX - c_accum->prev_value +
> -				 c_accum->energy_ctr;
> -	}
> +	accum = &data->accums[ch];
> +	if (input >= accum->prev_value)
> +		input += accum->energy_ctr -
> +				accum->prev_value;
> +	else
> +		input += UINT_MAX - accum->prev_value +
> +				accum->energy_ctr;
>  
>  	/* Energy consumed = (1/(2^ESU) * RAW * 1000000UL) μJoules */
>  	*val = div64_ul(input * 1000000UL, BIT(data->energy_units));
> @@ -188,20 +147,22 @@ static int amd_energy_read(struct device *dev,
>  			   u32 attr, int channel, long *val)
>  {
>  	struct amd_energy_data *data = dev_get_drvdata(dev);
> +	u32 reg;
>  	int cpu;
>  
>  	if (channel >= data->nr_cpus) {
>  		cpu = cpumask_first_and(cpu_online_mask,
>  					cpumask_of_node
>  					(channel - data->nr_cpus));
> -		amd_add_delta(data, channel, cpu, val, false);
> +		reg = ENERGY_PKG_MSR;
>  	} else {
>  		cpu = channel;
>  		if (!cpu_online(cpu))
>  			return -ENODEV;
>  
> -		amd_add_delta(data, channel, cpu, val, true);
> +		reg = ENERGY_CORE_MSR;
>  	}
> +	amd_add_delta(data, channel, cpu, val, reg);
>  
>  	return 0;
>  }
> @@ -249,7 +210,7 @@ static const struct hwmon_ops amd_energy_ops = {
>  
>  static int amd_create_sensor(struct device *dev,
>  			     struct amd_energy_data *data,
> -			     u8 type, u32 config)
> +			     enum hwmon_sensor_types type, u32 config)
>  {
>  	struct hwmon_channel_info *info = &data->energy_info;
>  	struct sensor_accumulator *accums;
> @@ -308,6 +269,7 @@ static int amd_energy_probe(struct platform_device *pdev)
>  	struct device *hwmon_dev;
>  	struct amd_energy_data *data;
>  	struct device *dev = &pdev->dev;
> +	int ret;
>  
>  	data = devm_kzalloc(dev,
>  			    sizeof(struct amd_energy_data), GFP_KERNEL);
> @@ -320,8 +282,10 @@ static int amd_energy_probe(struct platform_device *pdev)
>  	dev_set_drvdata(dev, data);
>  	/* Populate per-core energy reporting */
>  	data->info[0] = &data->energy_info;
> -	amd_create_sensor(dev, data, hwmon_energy,
> -			  HWMON_E_INPUT | HWMON_E_LABEL);
> +	ret = amd_create_sensor(dev, data, hwmon_energy,
> +				HWMON_E_INPUT | HWMON_E_LABEL);
> +	if (ret)
> +		return ret;
>  
>  	mutex_init(&data->lock);
>  	get_energy_units(data);
> @@ -346,7 +310,7 @@ static int amd_energy_probe(struct platform_device *pdev)
>  	if (IS_ERR(data->wrap_accumulate))
>  		return PTR_ERR(data->wrap_accumulate);
>  
> -	return PTR_ERR_OR_ZERO(data->wrap_accumulate);
> +	return 0;

Actually, what you want to remove is the above if() statement, since
PTR_ERR_OR_ZERO() is supposed to replace it.

>  }
>  
>  static int amd_energy_remove(struct platform_device *pdev)
> 

Re: [PATCH 4/6] hwmon: amd_energy: let user enable/disable the sw accumulation

[Guenter Roeck]

On 9/5/20 7:32 AM, Naveen Krishna Chatradhi wrote:
> Provide an option "accumulator_status" via sysfs to enable/disable
> the software accumulation of energy counters.
> 
> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>

I am quite substantially opposed to this. I'd be willing to
accept a module parameter. However, the code is there, and it is
enabled by default, and it should stay enabled by default.
I don't want to have to deal with people complaining that
it suddenly no longer works.

Also, there needs to be an explanation for why this is needed.

Guenter

> ---
>  drivers/hwmon/amd_energy.c | 104 ++++++++++++++++++++++++++++++-------
>  1 file changed, 86 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c
> index 96c61784d05c..c294bea56c02 100644
> --- a/drivers/hwmon/amd_energy.c
> +++ b/drivers/hwmon/amd_energy.c
> @@ -32,6 +32,8 @@
>  #define AMD_ENERGY_UNIT_MASK	0x01F00
>  #define AMD_ENERGY_MASK		0xFFFFFFFF
>  
> +static struct device_attribute accumulate_attr;
> +
>  struct sensor_accumulator {
>  	u64 energy_ctr;
>  	u64 prev_value;
> @@ -42,10 +44,12 @@ struct amd_energy_data {
>  	const struct hwmon_channel_info *info[2];
>  	struct hwmon_chip_info chip;
>  	struct task_struct *wrap_accumulate;
> +	struct device *hwmon_dev;
>  	/* Lock around the accumulator */
>  	struct mutex lock;
>  	/* An accumulator for each core and socket */
>  	struct sensor_accumulator *accums;
> +	bool accumulator_status;
>  	/* Energy Status Units */
>  	u64 energy_units;
>  	unsigned int timeout;
> @@ -128,13 +132,15 @@ static void amd_add_delta(struct amd_energy_data *data, int ch,
>  	rdmsrl_safe_on_cpu(cpu, reg, &input);
>  	input &= AMD_ENERGY_MASK;
>  
> -	accum = &data->accums[ch];
> -	if (input >= accum->prev_value)
> -		input += accum->energy_ctr -
> -				accum->prev_value;
> -	else
> -		input += UINT_MAX - accum->prev_value +
> -				accum->energy_ctr;
> +	if (data->accumulator_status) {
> +		accum = &data->accums[ch];
> +		if (input >= accum->prev_value)
> +			input += accum->energy_ctr -
> +					accum->prev_value;
> +		else
> +			input += UINT_MAX - accum->prev_value +
> +					accum->energy_ctr;
> +	}
>  
>  	/* Energy consumed = (1/(2^ESU) * RAW * 1000000UL) μJoules */
>  	*val = div64_ul(input * 1000000UL, BIT(data->energy_units));
> @@ -264,9 +270,67 @@ static int amd_create_sensor(struct device *dev,
>  	return 0;
>  }
>  
> +static ssize_t amd_energy_accumulate_show(struct device *dev,
> +					  struct device_attribute *dev_attr,
> +					  char *buf)
> +{
> +	struct amd_energy_data *data = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%d\n", data->accumulator_status);
> +}
> +
> +static ssize_t amd_energy_accumulate_store(struct device *dev,
> +					   struct device_attribute *dev_attr,
> +					   const char *buf, size_t count)
> +{
> +	struct amd_energy_data *data = dev_get_drvdata(dev);
> +	bool input;
> +	int ret;
> +
> +	ret = kstrtobool(buf, &input);
> +	if (ret)
> +		return ret;
> +
> +	if (data->accumulator_status == input)
> +		return count;
> +
> +	if (input) {
> +		memset(data->accums, 0, (data->nr_cpus + data->nr_socks) *
> +			sizeof(struct sensor_accumulator));
> +
> +		if (!data->wrap_accumulate) {
> +			data->wrap_accumulate =
> +				kthread_run(energy_accumulator,
> +					    data, "%s", dev_name(dev));
> +			if (IS_ERR(data->wrap_accumulate))
> +				return PTR_ERR(data->wrap_accumulate);
> +		}
> +	} else {
> +		if (data && data->wrap_accumulate) {
> +			ret = kthread_stop(data->wrap_accumulate);
> +			if (ret)
> +				return ret;
> +			data->wrap_accumulate = NULL;
> +		}
> +	}
> +	data->accumulator_status = input;
> +
> +	return count;
> +}
> +
> +static int create_accumulate_status_file(struct amd_energy_data *data)
> +{
> +	accumulate_attr.attr.name = "accumulator_status";
> +	accumulate_attr.attr.mode = 0664;
> +	accumulate_attr.show = amd_energy_accumulate_show;
> +	accumulate_attr.store = amd_energy_accumulate_store;
> +
> +	return sysfs_create_file(&data->hwmon_dev->kobj,
> +				 &accumulate_attr.attr);
> +}
> +
>  static int amd_energy_probe(struct platform_device *pdev)
>  {
> -	struct device *hwmon_dev;
>  	struct amd_energy_data *data;
>  	struct device *dev = &pdev->dev;
>  	int ret;
> @@ -290,12 +354,12 @@ static int amd_energy_probe(struct platform_device *pdev)
>  	mutex_init(&data->lock);
>  	get_energy_units(data);
>  
> -	hwmon_dev = devm_hwmon_device_register_with_info(dev, DRVNAME,
> -							 data,
> -							 &data->chip,
> -							 NULL);
> -	if (IS_ERR(hwmon_dev))
> -		return PTR_ERR(hwmon_dev);
> +	data->hwmon_dev = devm_hwmon_device_register_with_info(dev, DRVNAME,
> +							       data,
> +							       &data->chip,
> +							       NULL);
> +	if (IS_ERR(data->hwmon_dev))
> +		return PTR_ERR(data->hwmon_dev);
>  
>  	/* Once in 3 minutes for a resolution of 1/2*16 */
>  	if (data->energy_units == 0x10)
> @@ -305,10 +369,12 @@ static int amd_energy_probe(struct platform_device *pdev)
>  	if (data->energy_units == 0x6)
>  		data->timeout = 3 * 24 * 60 * 60;
>  
> -	data->wrap_accumulate = kthread_run(energy_accumulator, data,
> -					    "%s", dev_name(hwmon_dev));
> -	if (IS_ERR(data->wrap_accumulate))
> -		return PTR_ERR(data->wrap_accumulate);
> +	/* Disabling the energy accumulation by default */
> +	data->accumulator_status = 0;
> +
> +	ret = create_accumulate_status_file(data);
> +	if (ret)
> +		return ret;
>  
>  	return 0;
>  }
> @@ -317,6 +383,8 @@ static int amd_energy_remove(struct platform_device *pdev)
>  {
>  	struct amd_energy_data *data = dev_get_drvdata(&pdev->dev);
>  
> +	sysfs_remove_file(&data->hwmon_dev->kobj, &accumulate_attr.attr);
> +
>  	if (data && data->wrap_accumulate)
>  		kthread_stop(data->wrap_accumulate);
>  
> 

Re: [PATCH 5/6] hwmon: amd_energy: dump energy counters via debugfs

[Guenter Roeck]

On 9/5/20 7:32 AM, Naveen Krishna Chatradhi wrote:
> Use seq_printf to capture the core and socket energies under debugfs
> path in '/sys/kernel/debug/amd_energy/'
> file cenergy_dump: To print out the core energy counters
> file senergy_dump: To print out the socket energy counters
> 
> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>

Isn't this a duplicate of other functionality available in the kernel ?
I'd have to look it up, but I am quite sure that energy values are
already available. Besides that, what is the point of duplicating the
hwmon attributes ?

Guenter

> ---
>  drivers/hwmon/amd_energy.c | 110 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 110 insertions(+)
> 
> diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c
> index c294bea56c02..2184bd4510ed 100644
> --- a/drivers/hwmon/amd_energy.c
> +++ b/drivers/hwmon/amd_energy.c
> @@ -8,6 +8,7 @@
>  #include <linux/bits.h>
>  #include <linux/cpu.h>
>  #include <linux/cpumask.h>
> +#include <linux/debugfs.h>
>  #include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/hwmon.h>
> @@ -20,6 +21,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/sched.h>
>  #include <linux/slab.h>
> +#include <linux/smp.h>
>  #include <linux/topology.h>
>  #include <linux/types.h>
>  
> @@ -57,6 +59,8 @@ struct amd_energy_data {
>  	int nr_socks;
>  	int core_id;
>  	char (*label)[10];
> +	u64 *cdump;
> +	u64 *sdump;
>  };
>  
>  static int amd_energy_read_labels(struct device *dev,
> @@ -329,6 +333,108 @@ static int create_accumulate_status_file(struct amd_energy_data *data)
>  				 &accumulate_attr.attr);
>  }
>  
> +#ifdef CONFIG_DEBUG_FS
> +static void dump_on_each_cpu(void *info)
> +{
> +	struct amd_energy_data *data = info;
> +	int cpu = smp_processor_id();
> +
> +	amd_add_delta(data, cpu, cpu, (long *)&data->cdump[cpu],
> +		      ENERGY_CORE_MSR);
> +}
> +
> +static int cenergy_dump_show(struct seq_file *s, void *unused)
> +{
> +	struct amd_energy_data *data = s->private;
> +	struct cpumask *cpus_mask;
> +	int i;
> +
> +	cpus_mask = kmalloc(sizeof(*cpus_mask), GFP_KERNEL);
> +	memset(data->cdump, 0, (data->nr_cpus) * sizeof(u64));
> +	cpumask_clear(cpus_mask);
> +	for (i = 0; i < data->nr_cpus; i++) {
> +		if (cpu_online(i))
> +			cpumask_set_cpu(i, cpus_mask);
> +	}
> +
> +	on_each_cpu_mask(cpus_mask, dump_on_each_cpu, data, true);
> +
> +	for (i = 0; i < data->nr_cpus; i++) {
> +		if (!(i & 3))
> +			seq_printf(s, "Core %3d: ", i);
> +
> +		seq_printf(s, "%16llu ", data->cdump[i]);
> +		if ((i & 3) == 3)
> +			seq_puts(s, "\n");
> +	}
> +	seq_puts(s, "\n");
> +
> +	kfree(cpus_mask);
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(cenergy_dump);
> +
> +static int senergy_dump_show(struct seq_file *s, void *unused)
> +{
> +	struct amd_energy_data *data = s->private;
> +	int i, cpu;
> +
> +	for (i = 0; i < data->nr_socks; i++) {
> +		cpu = cpumask_first_and(cpu_online_mask,
> +					cpumask_of_node(i));
> +		amd_add_delta(data, data->nr_cpus + i, cpu,
> +			      (long *)&data->sdump[i], ENERGY_PKG_MSR);
> +		seq_printf(s, "Socket %1d: %16llu\n",
> +			   i, data->sdump[i]);
> +	}
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(senergy_dump);
> +
> +static void dump_debugfs_cleanup(void *ddir)
> +{
> +	debugfs_remove_recursive(ddir);
> +}
> +
> +static int create_dump_file(struct device *dev,
> +			    struct amd_energy_data *data)
> +{
> +	struct dentry *debugfs;
> +	char name[] = "amd_energy";
> +
> +	data->cdump = devm_kcalloc(dev, data->nr_cpus,
> +				   sizeof(u64), GFP_KERNEL);
> +	if (!data->cdump)
> +		return -ENOMEM;
> +
> +	data->sdump = devm_kcalloc(dev, data->nr_socks,
> +				   sizeof(u64), GFP_KERNEL);
> +	if (!data->sdump)
> +		return -ENOMEM;
> +
> +	debugfs = debugfs_create_dir(name, NULL);
> +	if (debugfs) {
> +		debugfs_create_file("cenergy_dump", 0440,
> +				    debugfs, data, &cenergy_dump_fops);
> +		debugfs_create_file("senergy_dump", 0440,
> +				    debugfs, data, &senergy_dump_fops);
> +		devm_add_action_or_reset(data->hwmon_dev,
> +					 dump_debugfs_cleanup, debugfs);
> +	}
> +
> +	return 0;
> +}
> +#else
> +
> +static int create_dump_file(struct device *dev,
> +			    struct amd_energy_data *data)
> +{
> +	return 0;
> +}
> +
> +#endif //CONFIG_DEBUG_FS
> +
>  static int amd_energy_probe(struct platform_device *pdev)
>  {
>  	struct amd_energy_data *data;
> @@ -376,6 +482,10 @@ static int amd_energy_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	ret = create_dump_file(dev, data);
> +	if (ret)
> +		return ret;
> +
>  	return 0;
>  }
>  
> 

Re: [PATCH 4/6] hwmon: amd_energy: let user enable/disable the sw accumulation

[Guenter Roeck]

On 9/5/20 8:17 AM, Guenter Roeck wrote:
> On 9/5/20 7:32 AM, Naveen Krishna Chatradhi wrote:
>> Provide an option "accumulator_status" via sysfs to enable/disable
>> the software accumulation of energy counters.
>>
>> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
> 
> I am quite substantially opposed to this. I'd be willing to
> accept a module parameter. However, the code is there, and it is
> enabled by default, and it should stay enabled by default.
> I don't want to have to deal with people complaining that
> it suddenly no longer works.
> 
> Also, there needs to be an explanation for why this is needed.
> 

No, wait, without accumulator this driver has zero value.
Users should just not load the driver if they don't want the
overhead associated with accumulation.

Guenter

> Guenter
> 
>> ---
>>  drivers/hwmon/amd_energy.c | 104 ++++++++++++++++++++++++++++++-------
>>  1 file changed, 86 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c
>> index 96c61784d05c..c294bea56c02 100644
>> --- a/drivers/hwmon/amd_energy.c
>> +++ b/drivers/hwmon/amd_energy.c
>> @@ -32,6 +32,8 @@
>>  #define AMD_ENERGY_UNIT_MASK	0x01F00
>>  #define AMD_ENERGY_MASK		0xFFFFFFFF
>>  
>> +static struct device_attribute accumulate_attr;
>> +
>>  struct sensor_accumulator {
>>  	u64 energy_ctr;
>>  	u64 prev_value;
>> @@ -42,10 +44,12 @@ struct amd_energy_data {
>>  	const struct hwmon_channel_info *info[2];
>>  	struct hwmon_chip_info chip;
>>  	struct task_struct *wrap_accumulate;
>> +	struct device *hwmon_dev;
>>  	/* Lock around the accumulator */
>>  	struct mutex lock;
>>  	/* An accumulator for each core and socket */
>>  	struct sensor_accumulator *accums;
>> +	bool accumulator_status;
>>  	/* Energy Status Units */
>>  	u64 energy_units;
>>  	unsigned int timeout;
>> @@ -128,13 +132,15 @@ static void amd_add_delta(struct amd_energy_data *data, int ch,
>>  	rdmsrl_safe_on_cpu(cpu, reg, &input);
>>  	input &= AMD_ENERGY_MASK;
>>  
>> -	accum = &data->accums[ch];
>> -	if (input >= accum->prev_value)
>> -		input += accum->energy_ctr -
>> -				accum->prev_value;
>> -	else
>> -		input += UINT_MAX - accum->prev_value +
>> -				accum->energy_ctr;
>> +	if (data->accumulator_status) {
>> +		accum = &data->accums[ch];
>> +		if (input >= accum->prev_value)
>> +			input += accum->energy_ctr -
>> +					accum->prev_value;
>> +		else
>> +			input += UINT_MAX - accum->prev_value +
>> +					accum->energy_ctr;
>> +	}
>>  
>>  	/* Energy consumed = (1/(2^ESU) * RAW * 1000000UL) μJoules */
>>  	*val = div64_ul(input * 1000000UL, BIT(data->energy_units));
>> @@ -264,9 +270,67 @@ static int amd_create_sensor(struct device *dev,
>>  	return 0;
>>  }
>>  
>> +static ssize_t amd_energy_accumulate_show(struct device *dev,
>> +					  struct device_attribute *dev_attr,
>> +					  char *buf)
>> +{
>> +	struct amd_energy_data *data = dev_get_drvdata(dev);
>> +
>> +	return sprintf(buf, "%d\n", data->accumulator_status);
>> +}
>> +
>> +static ssize_t amd_energy_accumulate_store(struct device *dev,
>> +					   struct device_attribute *dev_attr,
>> +					   const char *buf, size_t count)
>> +{
>> +	struct amd_energy_data *data = dev_get_drvdata(dev);
>> +	bool input;
>> +	int ret;
>> +
>> +	ret = kstrtobool(buf, &input);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (data->accumulator_status == input)
>> +		return count;
>> +
>> +	if (input) {
>> +		memset(data->accums, 0, (data->nr_cpus + data->nr_socks) *
>> +			sizeof(struct sensor_accumulator));
>> +
>> +		if (!data->wrap_accumulate) {
>> +			data->wrap_accumulate =
>> +				kthread_run(energy_accumulator,
>> +					    data, "%s", dev_name(dev));
>> +			if (IS_ERR(data->wrap_accumulate))
>> +				return PTR_ERR(data->wrap_accumulate);
>> +		}
>> +	} else {
>> +		if (data && data->wrap_accumulate) {
>> +			ret = kthread_stop(data->wrap_accumulate);
>> +			if (ret)
>> +				return ret;
>> +			data->wrap_accumulate = NULL;
>> +		}
>> +	}
>> +	data->accumulator_status = input;
>> +
>> +	return count;
>> +}
>> +
>> +static int create_accumulate_status_file(struct amd_energy_data *data)
>> +{
>> +	accumulate_attr.attr.name = "accumulator_status";
>> +	accumulate_attr.attr.mode = 0664;
>> +	accumulate_attr.show = amd_energy_accumulate_show;
>> +	accumulate_attr.store = amd_energy_accumulate_store;
>> +
>> +	return sysfs_create_file(&data->hwmon_dev->kobj,
>> +				 &accumulate_attr.attr);
>> +}
>> +
>>  static int amd_energy_probe(struct platform_device *pdev)
>>  {
>> -	struct device *hwmon_dev;
>>  	struct amd_energy_data *data;
>>  	struct device *dev = &pdev->dev;
>>  	int ret;
>> @@ -290,12 +354,12 @@ static int amd_energy_probe(struct platform_device *pdev)
>>  	mutex_init(&data->lock);
>>  	get_energy_units(data);
>>  
>> -	hwmon_dev = devm_hwmon_device_register_with_info(dev, DRVNAME,
>> -							 data,
>> -							 &data->chip,
>> -							 NULL);
>> -	if (IS_ERR(hwmon_dev))
>> -		return PTR_ERR(hwmon_dev);
>> +	data->hwmon_dev = devm_hwmon_device_register_with_info(dev, DRVNAME,
>> +							       data,
>> +							       &data->chip,
>> +							       NULL);
>> +	if (IS_ERR(data->hwmon_dev))
>> +		return PTR_ERR(data->hwmon_dev);
>>  
>>  	/* Once in 3 minutes for a resolution of 1/2*16 */
>>  	if (data->energy_units == 0x10)
>> @@ -305,10 +369,12 @@ static int amd_energy_probe(struct platform_device *pdev)
>>  	if (data->energy_units == 0x6)
>>  		data->timeout = 3 * 24 * 60 * 60;
>>  
>> -	data->wrap_accumulate = kthread_run(energy_accumulator, data,
>> -					    "%s", dev_name(hwmon_dev));
>> -	if (IS_ERR(data->wrap_accumulate))
>> -		return PTR_ERR(data->wrap_accumulate);
>> +	/* Disabling the energy accumulation by default */
>> +	data->accumulator_status = 0;
>> +
>> +	ret = create_accumulate_status_file(data);
>> +	if (ret)
>> +		return ret;
>>  
>>  	return 0;
>>  }
>> @@ -317,6 +383,8 @@ static int amd_energy_remove(struct platform_device *pdev)
>>  {
>>  	struct amd_energy_data *data = dev_get_drvdata(&pdev->dev);
>>  
>> +	sysfs_remove_file(&data->hwmon_dev->kobj, &accumulate_attr.attr);
>> +
>>  	if (data && data->wrap_accumulate)
>>  		kthread_stop(data->wrap_accumulate);
>>  
>>
> 

Re: [PATCH 2/6] hwmon: amd_energy: optimize accumulation interval

[Naveen Krishna Ch]

Hi Guenter,

> On 9/5/20 7:32 AM, Naveen Krishna Chatradhi wrote:
> > On a system with course grain resolution of energy unit (milli J) the
> > accumulation thread can be executed less frequently than on the system
> > with fine grain resolution(micro J).
> >
> > This patch sets the accumulation thread interval to an optimum value
> > calculated based on the (energy unit) resolution supported by the
> > hardware (assuming a peak wattage of 240W).
> >
> > Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
> > ---
> >  drivers/hwmon/amd_energy.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c
> > index 9580a16185b8..f0a13d6cc419 100644
> > --- a/drivers/hwmon/amd_energy.c
> > +++ b/drivers/hwmon/amd_energy.c
> > @@ -48,6 +48,7 @@ struct amd_energy_data {
> >       struct sensor_accumulator *accums;
> >       /* Energy Status Units */
> >       u64 energy_units;
> > +     unsigned int timeout;
> >       int nr_cpus;
> >       int nr_socks;
> >       int core_id;
> > @@ -215,6 +216,7 @@ static umode_t amd_energy_is_visible(const void
> > *_data,  static int energy_accumulator(void *p)  {
> >       struct amd_energy_data *data = (struct amd_energy_data *)p;
> > +     unsigned int timeout = data->timeout;
> >
> >       while (!kthread_should_stop()) {
> >               /*
> > @@ -234,7 +236,7 @@ static int energy_accumulator(void *p)
> >                *
> >                * let us accumulate for every 100secs
> >                */
> > -             schedule_timeout(msecs_to_jiffies(100000));
> > +             schedule_timeout(msecs_to_jiffies(timeout));
>
> Numbers below are in seconds, used as milli-seconds here.
>
> >       }
> >       return 0;
> >  }
> > @@ -331,6 +333,14 @@ static int amd_energy_probe(struct platform_device *pdev)
> >       if (IS_ERR(hwmon_dev))
> >               return PTR_ERR(hwmon_dev);
> >
> > +     /* Once in 3 minutes for a resolution of 1/2*16 */
>
> * 16 or ^ 16 ?
>
> > +     if (data->energy_units == 0x10)
> > +             data->timeout = 3 * 60;
>
> 180 ms ? I assume this is a bug and meant to be 3 * 60 * 1000.
>
> > +
> > +     /* Once in 3 days for a resolution of 1/2^6 */
> > +     if (data->energy_units == 0x6)
> > +             data->timeout = 3 * 24 * 60 * 60;
> > +
>
> ... and else ?
>
> This needs to cover all cases, including those not currently existing.
> I would suggest to define a formula based on data->energy_units.
> The energy units value can be anything from 0..31. Based on your numbers, something like
>     timeout_ms = BIT(34 - data->energy_units); should do. It translates to about 3.1 days for energy_units=6, and
> 4.3 minutes for energy_units=16. If that is too much, you can make it
>    timeout_ms = BIT(33 - data->energy_units);
>
> To avoid overflow, it might make sense to max out at BIT(31).
>     timeout_ms = BIT((min(31, 33 - data->energy_units));
>
Thank you for the feedback and suggestions. I will implement and
submit the updated version.

> Guenter
>
> >       data->wrap_accumulate = kthread_run(energy_accumulator, data,
> >                                           "%s", dev_name(hwmon_dev));
> >       if (IS_ERR(data->wrap_accumulate))
> >
Naveen

Re: FW: [PATCH 3/6] hwmon: amd_energy: Improve the accumulation logic

[Naveen Krishna Ch]

Hi Guenter,

> On 9/5/20 7:32 AM, Naveen Krishna Chatradhi wrote:
> > Factor out the common code in the accumulation functions for core and
> > socket accumulation.
> >
> > While at it, handle the return value of the amd_create_sensor() function.
> >
> > Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
> > ---
> >  drivers/hwmon/amd_energy.c | 126
> > +++++++++++++------------------------
> >  1 file changed, 45 insertions(+), 81 deletions(-)
> >
> > diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c
> > index f0a13d6cc419..96c61784d05c 100644
> > --- a/drivers/hwmon/amd_energy.c
> > +++ b/drivers/hwmon/amd_energy.c
> > @@ -74,108 +74,67 @@ static void get_energy_units(struct amd_energy_data *data)
> >       data->energy_units = (rapl_units & AMD_ENERGY_UNIT_MASK) >> 8;
> > }
> >
> > -static void accumulate_socket_delta(struct amd_energy_data *data,
> > -                                 int sock, int cpu)
> > +static void accumulate_delta(struct amd_energy_data *data,
> > +                          int channel, int cpu, u32 reg)
> >  {
> > -     struct sensor_accumulator *s_accum;
> > +     struct sensor_accumulator *accum;
> >       u64 input;
> >
> >       mutex_lock(&data->lock);
> > -     rdmsrl_safe_on_cpu(cpu, ENERGY_PKG_MSR, &input);
> > +     rdmsrl_safe_on_cpu(cpu, reg, &input);
> >       input &= AMD_ENERGY_MASK;
> >
> > -     s_accum = &data->accums[data->nr_cpus + sock];
> > -     if (input >= s_accum->prev_value)
> > -             s_accum->energy_ctr +=
> > -                     input - s_accum->prev_value;
> > +     accum = &data->accums[channel];
> > +     if (input >= accum->prev_value)
> > +             accum->energy_ctr +=
> > +                     input - accum->prev_value;
> >       else
> > -             s_accum->energy_ctr += UINT_MAX -
> > -                     s_accum->prev_value + input;
> > +             accum->energy_ctr += UINT_MAX -
> > +                     accum->prev_value + input;
> >
> > -     s_accum->prev_value = input;
> > +     accum->prev_value = input;
> >       mutex_unlock(&data->lock);
> >  }
> >
> > -static void accumulate_core_delta(struct amd_energy_data *data)
> > +static void read_accumulate(struct amd_energy_data *data)
> >  {
> > -     struct sensor_accumulator *c_accum;
> > -     u64 input;
> > -     int cpu;
> > +     int sock, scpu, cpu;
> > +
> > +     for (sock = 0; sock < data->nr_socks; sock++) {
> > +             scpu = cpumask_first_and(cpu_online_mask,
> > +                                      cpumask_of_node(sock));
> > +
> > +             accumulate_delta(data, data->nr_cpus + sock,
> > +                              scpu, ENERGY_PKG_MSR);
> > +     }
> >
> > -     mutex_lock(&data->lock);
> >       if (data->core_id >= data->nr_cpus)
> >               data->core_id = 0;
> >
> >       cpu = data->core_id;
> > +     if (cpu_online(cpu))
> > +             accumulate_delta(data, cpu, cpu, ENERGY_CORE_MSR);
> >
> > -     if (!cpu_online(cpu))
> > -             goto out;
> > -
> > -     rdmsrl_safe_on_cpu(cpu, ENERGY_CORE_MSR, &input);
> > -     input &= AMD_ENERGY_MASK;
> > -
> > -     c_accum = &data->accums[cpu];
> > -
> > -     if (input >= c_accum->prev_value)
> > -             c_accum->energy_ctr +=
> > -                     input - c_accum->prev_value;
> > -     else
> > -             c_accum->energy_ctr += UINT_MAX -
> > -                     c_accum->prev_value + input;
> > -
> > -     c_accum->prev_value = input;
> > -
> > -out:
> >       data->core_id++;
> > -     mutex_unlock(&data->lock);
> > -}
> > -
> > -static void read_accumulate(struct amd_energy_data *data) -{
> > -     int sock;
> > -
> > -     for (sock = 0; sock < data->nr_socks; sock++) {
> > -             int cpu;
> > -
> > -             cpu = cpumask_first_and(cpu_online_mask,
> > -                                     cpumask_of_node(sock));
> > -
> > -             accumulate_socket_delta(data, sock, cpu);
> > -     }
> > -
> > -     accumulate_core_delta(data);
> >  }
> >
> >  static void amd_add_delta(struct amd_energy_data *data, int ch,
> > -                       int cpu, long *val, bool is_core)
> > +                       int cpu, long *val, u32 reg)
> >  {
> > -     struct sensor_accumulator *s_accum, *c_accum;
> > +     struct sensor_accumulator *accum;
> >       u64 input;
> >
> >       mutex_lock(&data->lock);
> > -     if (!is_core) {
> > -             rdmsrl_safe_on_cpu(cpu, ENERGY_PKG_MSR, &input);
> > -             input &= AMD_ENERGY_MASK;
> > -
> > -             s_accum = &data->accums[ch];
> > -             if (input >= s_accum->prev_value)
> > -                     input += s_accum->energy_ctr -
> > -                               s_accum->prev_value;
> > -             else
> > -                     input += UINT_MAX - s_accum->prev_value +
> > -                               s_accum->energy_ctr;
> > -     } else {
> > -             rdmsrl_safe_on_cpu(cpu, ENERGY_CORE_MSR, &input);
> > -             input &= AMD_ENERGY_MASK;
> > +     rdmsrl_safe_on_cpu(cpu, reg, &input);
> > +     input &= AMD_ENERGY_MASK;
> >
> > -             c_accum = &data->accums[ch];
> > -             if (input >= c_accum->prev_value)
> > -                     input += c_accum->energy_ctr -
> > -                              c_accum->prev_value;
> > -             else
> > -                     input += UINT_MAX - c_accum->prev_value +
> > -                              c_accum->energy_ctr;
> > -     }
> > +     accum = &data->accums[ch];
> > +     if (input >= accum->prev_value)
> > +             input += accum->energy_ctr -
> > +                             accum->prev_value;
> > +     else
> > +             input += UINT_MAX - accum->prev_value +
> > +                             accum->energy_ctr;
> >
> >       /* Energy consumed = (1/(2^ESU) * RAW * 1000000UL) μJoules */
> >       *val = div64_ul(input * 1000000UL, BIT(data->energy_units)); @@
> > -188,20 +147,22 @@ static int amd_energy_read(struct device *dev,
> >                          u32 attr, int channel, long *val)  {
> >       struct amd_energy_data *data = dev_get_drvdata(dev);
> > +     u32 reg;
> >       int cpu;
> >
> >       if (channel >= data->nr_cpus) {
> >               cpu = cpumask_first_and(cpu_online_mask,
> >                                       cpumask_of_node
> >                                       (channel - data->nr_cpus));
> > -             amd_add_delta(data, channel, cpu, val, false);
> > +             reg = ENERGY_PKG_MSR;
> >       } else {
> >               cpu = channel;
> >               if (!cpu_online(cpu))
> >                       return -ENODEV;
> >
> > -             amd_add_delta(data, channel, cpu, val, true);
> > +             reg = ENERGY_CORE_MSR;
> >       }
> > +     amd_add_delta(data, channel, cpu, val, reg);
> >
> >       return 0;
> >  }
> > @@ -249,7 +210,7 @@ static const struct hwmon_ops amd_energy_ops = {
> >
> >  static int amd_create_sensor(struct device *dev,
> >                            struct amd_energy_data *data,
> > -                          u8 type, u32 config)
> > +                          enum hwmon_sensor_types type, u32 config)
> >  {
> >       struct hwmon_channel_info *info = &data->energy_info;
> >       struct sensor_accumulator *accums; @@ -308,6 +269,7 @@ static
> > int amd_energy_probe(struct platform_device *pdev)
> >       struct device *hwmon_dev;
> >       struct amd_energy_data *data;
> >       struct device *dev = &pdev->dev;
> > +     int ret;
> >
> >       data = devm_kzalloc(dev,
> >                           sizeof(struct amd_energy_data), GFP_KERNEL);
> > @@ -320,8 +282,10 @@ static int amd_energy_probe(struct platform_device *pdev)
> >       dev_set_drvdata(dev, data);
> >       /* Populate per-core energy reporting */
> >       data->info[0] = &data->energy_info;
> > -     amd_create_sensor(dev, data, hwmon_energy,
> > -                       HWMON_E_INPUT | HWMON_E_LABEL);
> > +     ret = amd_create_sensor(dev, data, hwmon_energy,
> > +                             HWMON_E_INPUT | HWMON_E_LABEL);
> > +     if (ret)
> > +             return ret;
> >
> >       mutex_init(&data->lock);
> >       get_energy_units(data);
> > @@ -346,7 +310,7 @@ static int amd_energy_probe(struct platform_device *pdev)
> >       if (IS_ERR(data->wrap_accumulate))
> >               return PTR_ERR(data->wrap_accumulate);
> >
> > -     return PTR_ERR_OR_ZERO(data->wrap_accumulate);
> > +     return 0;
>
> Actually, what you want to remove is the above if() statement, since
> PTR_ERR_OR_ZERO() is supposed to replace it.
Yes, that would have been better. Will resubmit.
>
> >  }
> >
> >  static int amd_energy_remove(struct platform_device *pdev)
> >

Naveen

Re: FW: [PATCH 5/6] hwmon: amd_energy: dump energy counters via debugfs

[Naveen Krishna Ch]

Hi Guenter,

> On 9/5/20 7:32 AM, Naveen Krishna Chatradhi wrote:
> > Use seq_printf to capture the core and socket energies under debugfs
> > path in '/sys/kernel/debug/amd_energy/'
> > file cenergy_dump: To print out the core energy counters file
> > senergy_dump: To print out the socket energy counters
> >
> > Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
>
> Isn't this a duplicate of other functionality available in the kernel ?
> I'd have to look it up, but I am quite sure that energy values are already available. Besides that, what is the point of duplicating the hwmon attributes ?

Idea is to reduce the latency in gathering all the counters (Rome has
128 cores on 2 sockets) from the user space.
If there is a better way to get this done, please let me know. I shall
implement and resubmit.

>
> Guenter
>
> > ---
> >  drivers/hwmon/amd_energy.c | 110
> > +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 110 insertions(+)
> >
> > diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c
> > index c294bea56c02..2184bd4510ed 100644
> > --- a/drivers/hwmon/amd_energy.c
> > +++ b/drivers/hwmon/amd_energy.c
> > @@ -8,6 +8,7 @@
> >  #include <linux/bits.h>
> >  #include <linux/cpu.h>
> >  #include <linux/cpumask.h>
> > +#include <linux/debugfs.h>
> >  #include <linux/delay.h>
> >  #include <linux/device.h>
> >  #include <linux/hwmon.h>
> > @@ -20,6 +21,7 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/sched.h>
> >  #include <linux/slab.h>
> > +#include <linux/smp.h>
> >  #include <linux/topology.h>
> >  #include <linux/types.h>
> >
> > @@ -57,6 +59,8 @@ struct amd_energy_data {
> >       int nr_socks;
> >       int core_id;
> >       char (*label)[10];
> > +     u64 *cdump;
> > +     u64 *sdump;
> >  };
> >
> >  static int amd_energy_read_labels(struct device *dev, @@ -329,6
> > +333,108 @@ static int create_accumulate_status_file(struct amd_energy_data *data)
> >                                &accumulate_attr.attr);  }
> >
> > +#ifdef CONFIG_DEBUG_FS
> > +static void dump_on_each_cpu(void *info) {
> > +     struct amd_energy_data *data = info;
> > +     int cpu = smp_processor_id();
> > +
> > +     amd_add_delta(data, cpu, cpu, (long *)&data->cdump[cpu],
> > +                   ENERGY_CORE_MSR);
> > +}
> > +
> > +static int cenergy_dump_show(struct seq_file *s, void *unused) {
> > +     struct amd_energy_data *data = s->private;
> > +     struct cpumask *cpus_mask;
> > +     int i;
> > +
> > +     cpus_mask = kmalloc(sizeof(*cpus_mask), GFP_KERNEL);
> > +     memset(data->cdump, 0, (data->nr_cpus) * sizeof(u64));
> > +     cpumask_clear(cpus_mask);
> > +     for (i = 0; i < data->nr_cpus; i++) {
> > +             if (cpu_online(i))
> > +                     cpumask_set_cpu(i, cpus_mask);
> > +     }
> > +
> > +     on_each_cpu_mask(cpus_mask, dump_on_each_cpu, data, true);
> > +
> > +     for (i = 0; i < data->nr_cpus; i++) {
> > +             if (!(i & 3))
> > +                     seq_printf(s, "Core %3d: ", i);
> > +
> > +             seq_printf(s, "%16llu ", data->cdump[i]);
> > +             if ((i & 3) == 3)
> > +                     seq_puts(s, "\n");
> > +     }
> > +     seq_puts(s, "\n");
> > +
> > +     kfree(cpus_mask);
> > +     return 0;
> > +}
> > +DEFINE_SHOW_ATTRIBUTE(cenergy_dump);
> > +
> > +static int senergy_dump_show(struct seq_file *s, void *unused) {
> > +     struct amd_energy_data *data = s->private;
> > +     int i, cpu;
> > +
> > +     for (i = 0; i < data->nr_socks; i++) {
> > +             cpu = cpumask_first_and(cpu_online_mask,
> > +                                     cpumask_of_node(i));
> > +             amd_add_delta(data, data->nr_cpus + i, cpu,
> > +                           (long *)&data->sdump[i], ENERGY_PKG_MSR);
> > +             seq_printf(s, "Socket %1d: %16llu\n",
> > +                        i, data->sdump[i]);
> > +     }
> > +
> > +     return 0;
> > +}
> > +DEFINE_SHOW_ATTRIBUTE(senergy_dump);
> > +
> > +static void dump_debugfs_cleanup(void *ddir) {
> > +     debugfs_remove_recursive(ddir);
> > +}
> > +
> > +static int create_dump_file(struct device *dev,
> > +                         struct amd_energy_data *data) {
> > +     struct dentry *debugfs;
> > +     char name[] = "amd_energy";
> > +
> > +     data->cdump = devm_kcalloc(dev, data->nr_cpus,
> > +                                sizeof(u64), GFP_KERNEL);
> > +     if (!data->cdump)
> > +             return -ENOMEM;
> > +
> > +     data->sdump = devm_kcalloc(dev, data->nr_socks,
> > +                                sizeof(u64), GFP_KERNEL);
> > +     if (!data->sdump)
> > +             return -ENOMEM;
> > +
> > +     debugfs = debugfs_create_dir(name, NULL);
> > +     if (debugfs) {
> > +             debugfs_create_file("cenergy_dump", 0440,
> > +                                 debugfs, data, &cenergy_dump_fops);
> > +             debugfs_create_file("senergy_dump", 0440,
> > +                                 debugfs, data, &senergy_dump_fops);
> > +             devm_add_action_or_reset(data->hwmon_dev,
> > +                                      dump_debugfs_cleanup, debugfs);
> > +     }
> > +
> > +     return 0;
> > +}
> > +#else
> > +
> > +static int create_dump_file(struct device *dev,
> > +                         struct amd_energy_data *data) {
> > +     return 0;
> > +}
> > +
> > +#endif //CONFIG_DEBUG_FS
> > +
> >  static int amd_energy_probe(struct platform_device *pdev)  {
> >       struct amd_energy_data *data;
> > @@ -376,6 +482,10 @@ static int amd_energy_probe(struct platform_device *pdev)
> >       if (ret)
> >               return ret;
> >
> > +     ret = create_dump_file(dev, data);
> > +     if (ret)
> > +             return ret;
> > +
> >       return 0;
> >  }
> >
> >
Naveen

Re: FW: [PATCH 5/6] hwmon: amd_energy: dump energy counters via debugfs

[Guenter Roeck]

On 9/5/20 9:41 AM, Naveen Krishna Ch wrote:
> Hi Guenter,
> 
>> On 9/5/20 7:32 AM, Naveen Krishna Chatradhi wrote:
>>> Use seq_printf to capture the core and socket energies under debugfs
>>> path in '/sys/kernel/debug/amd_energy/'
>>> file cenergy_dump: To print out the core energy counters file
>>> senergy_dump: To print out the socket energy counters
>>>
>>> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
>>
>> Isn't this a duplicate of other functionality available in the kernel ?
>> I'd have to look it up, but I am quite sure that energy values are already available. Besides that, what is the point of duplicating the hwmon attributes ?
> 
> Idea is to reduce the latency in gathering all the counters (Rome has
> 128 cores on 2 sockets) from the user space.
> If there is a better way to get this done, please let me know. I shall
> implement and resubmit.
> 

Isn't turbostat supposed to be able to do that ?

Guenter

>>
>> Guenter
>>
>>> ---
>>>  drivers/hwmon/amd_energy.c | 110
>>> +++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 110 insertions(+)
>>>
>>> diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c
>>> index c294bea56c02..2184bd4510ed 100644
>>> --- a/drivers/hwmon/amd_energy.c
>>> +++ b/drivers/hwmon/amd_energy.c
>>> @@ -8,6 +8,7 @@
>>>  #include <linux/bits.h>
>>>  #include <linux/cpu.h>
>>>  #include <linux/cpumask.h>
>>> +#include <linux/debugfs.h>
>>>  #include <linux/delay.h>
>>>  #include <linux/device.h>
>>>  #include <linux/hwmon.h>
>>> @@ -20,6 +21,7 @@
>>>  #include <linux/platform_device.h>
>>>  #include <linux/sched.h>
>>>  #include <linux/slab.h>
>>> +#include <linux/smp.h>
>>>  #include <linux/topology.h>
>>>  #include <linux/types.h>
>>>
>>> @@ -57,6 +59,8 @@ struct amd_energy_data {
>>>       int nr_socks;
>>>       int core_id;
>>>       char (*label)[10];
>>> +     u64 *cdump;
>>> +     u64 *sdump;
>>>  };
>>>
>>>  static int amd_energy_read_labels(struct device *dev, @@ -329,6
>>> +333,108 @@ static int create_accumulate_status_file(struct amd_energy_data *data)
>>>                                &accumulate_attr.attr);  }
>>>
>>> +#ifdef CONFIG_DEBUG_FS
>>> +static void dump_on_each_cpu(void *info) {
>>> +     struct amd_energy_data *data = info;
>>> +     int cpu = smp_processor_id();
>>> +
>>> +     amd_add_delta(data, cpu, cpu, (long *)&data->cdump[cpu],
>>> +                   ENERGY_CORE_MSR);
>>> +}
>>> +
>>> +static int cenergy_dump_show(struct seq_file *s, void *unused) {
>>> +     struct amd_energy_data *data = s->private;
>>> +     struct cpumask *cpus_mask;
>>> +     int i;
>>> +
>>> +     cpus_mask = kmalloc(sizeof(*cpus_mask), GFP_KERNEL);
>>> +     memset(data->cdump, 0, (data->nr_cpus) * sizeof(u64));
>>> +     cpumask_clear(cpus_mask);
>>> +     for (i = 0; i < data->nr_cpus; i++) {
>>> +             if (cpu_online(i))
>>> +                     cpumask_set_cpu(i, cpus_mask);
>>> +     }
>>> +
>>> +     on_each_cpu_mask(cpus_mask, dump_on_each_cpu, data, true);
>>> +
>>> +     for (i = 0; i < data->nr_cpus; i++) {
>>> +             if (!(i & 3))
>>> +                     seq_printf(s, "Core %3d: ", i);
>>> +
>>> +             seq_printf(s, "%16llu ", data->cdump[i]);
>>> +             if ((i & 3) == 3)
>>> +                     seq_puts(s, "\n");
>>> +     }
>>> +     seq_puts(s, "\n");
>>> +
>>> +     kfree(cpus_mask);
>>> +     return 0;
>>> +}
>>> +DEFINE_SHOW_ATTRIBUTE(cenergy_dump);
>>> +
>>> +static int senergy_dump_show(struct seq_file *s, void *unused) {
>>> +     struct amd_energy_data *data = s->private;
>>> +     int i, cpu;
>>> +
>>> +     for (i = 0; i < data->nr_socks; i++) {
>>> +             cpu = cpumask_first_and(cpu_online_mask,
>>> +                                     cpumask_of_node(i));
>>> +             amd_add_delta(data, data->nr_cpus + i, cpu,
>>> +                           (long *)&data->sdump[i], ENERGY_PKG_MSR);
>>> +             seq_printf(s, "Socket %1d: %16llu\n",
>>> +                        i, data->sdump[i]);
>>> +     }
>>> +
>>> +     return 0;
>>> +}
>>> +DEFINE_SHOW_ATTRIBUTE(senergy_dump);
>>> +
>>> +static void dump_debugfs_cleanup(void *ddir) {
>>> +     debugfs_remove_recursive(ddir);
>>> +}
>>> +
>>> +static int create_dump_file(struct device *dev,
>>> +                         struct amd_energy_data *data) {
>>> +     struct dentry *debugfs;
>>> +     char name[] = "amd_energy";
>>> +
>>> +     data->cdump = devm_kcalloc(dev, data->nr_cpus,
>>> +                                sizeof(u64), GFP_KERNEL);
>>> +     if (!data->cdump)
>>> +             return -ENOMEM;
>>> +
>>> +     data->sdump = devm_kcalloc(dev, data->nr_socks,
>>> +                                sizeof(u64), GFP_KERNEL);
>>> +     if (!data->sdump)
>>> +             return -ENOMEM;
>>> +
>>> +     debugfs = debugfs_create_dir(name, NULL);
>>> +     if (debugfs) {
>>> +             debugfs_create_file("cenergy_dump", 0440,
>>> +                                 debugfs, data, &cenergy_dump_fops);
>>> +             debugfs_create_file("senergy_dump", 0440,
>>> +                                 debugfs, data, &senergy_dump_fops);
>>> +             devm_add_action_or_reset(data->hwmon_dev,
>>> +                                      dump_debugfs_cleanup, debugfs);
>>> +     }
>>> +
>>> +     return 0;
>>> +}
>>> +#else
>>> +
>>> +static int create_dump_file(struct device *dev,
>>> +                         struct amd_energy_data *data) {
>>> +     return 0;
>>> +}
>>> +
>>> +#endif //CONFIG_DEBUG_FS
>>> +
>>>  static int amd_energy_probe(struct platform_device *pdev)  {
>>>       struct amd_energy_data *data;
>>> @@ -376,6 +482,10 @@ static int amd_energy_probe(struct platform_device *pdev)
>>>       if (ret)
>>>               return ret;
>>>
>>> +     ret = create_dump_file(dev, data);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>>       return 0;
>>>  }
>>>
>>>
> Naveen
> 

Re: FW: [PATCH 5/6] hwmon: amd_energy: dump energy counters via debugfs

[Naveen Krishna Ch]

Hi Guenter

On Sat, 5 Sep 2020 at 22:28, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 9/5/20 9:41 AM, Naveen Krishna Ch wrote:
> > Hi Guenter,
> >
> >> On 9/5/20 7:32 AM, Naveen Krishna Chatradhi wrote:
> >>> Use seq_printf to capture the core and socket energies under debugfs
> >>> path in '/sys/kernel/debug/amd_energy/'
> >>> file cenergy_dump: To print out the core energy counters file
> >>> senergy_dump: To print out the socket energy counters
> >>>
> >>> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
> >>
> >> Isn't this a duplicate of other functionality available in the kernel ?
> >> I'd have to look it up, but I am quite sure that energy values are already available. Besides that, what is the point of duplicating the hwmon attributes ?
> >
> > Idea is to reduce the latency in gathering all the counters (Rome has
> > 128 cores on 2 sockets) from the user space.
> > If there is a better way to get this done, please let me know. I shall
> > implement and resubmit.
> >
>
> Isn't turbostat supposed to be able to do that ?
Apologies, I was not aware of turbostat, took a look at the turbostat
code now, this is an elaborate tool which is dependent on msr.ko. At
present, this tool provides a lot of information in sequence. There is
no duplication of the code.

We need this functionality for the following reasons.
1. Reduced latency in gathering the energy counters of all cores and packages
2. Possible to provide an API to the user space to integrate into
other tools/frameworks

Please let me know your opinion.
>
> Guenter
>
> >>
> >> Guenter
> >>
> >>> ---
> >>>  drivers/hwmon/amd_energy.c | 110
> >>> +++++++++++++++++++++++++++++++++++++
> >>>  1 file changed, 110 insertions(+)
> >>>
> >>> diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c
> >>> index c294bea56c02..2184bd4510ed 100644
> >>> --- a/drivers/hwmon/amd_energy.c
> >>> +++ b/drivers/hwmon/amd_energy.c
> >>> @@ -8,6 +8,7 @@
> >>>  #include <linux/bits.h>
> >>>  #include <linux/cpu.h>
> >>>  #include <linux/cpumask.h>
> >>> +#include <linux/debugfs.h>
> >>>  #include <linux/delay.h>
> >>>  #include <linux/device.h>
> >>>  #include <linux/hwmon.h>
> >>> @@ -20,6 +21,7 @@
> >>>  #include <linux/platform_device.h>
> >>>  #include <linux/sched.h>
> >>>  #include <linux/slab.h>
> >>> +#include <linux/smp.h>
> >>>  #include <linux/topology.h>
> >>>  #include <linux/types.h>
> >>>
> >>> @@ -57,6 +59,8 @@ struct amd_energy_data {
> >>>       int nr_socks;
> >>>       int core_id;
> >>>       char (*label)[10];
> >>> +     u64 *cdump;
> >>> +     u64 *sdump;
> >>>  };
> >>>
> >>>  static int amd_energy_read_labels(struct device *dev, @@ -329,6
> >>> +333,108 @@ static int create_accumulate_status_file(struct amd_energy_data *data)
> >>>                                &accumulate_attr.attr);  }
> >>>
> >>> +#ifdef CONFIG_DEBUG_FS
> >>> +static void dump_on_each_cpu(void *info) {
> >>> +     struct amd_energy_data *data = info;
> >>> +     int cpu = smp_processor_id();
> >>> +
> >>> +     amd_add_delta(data, cpu, cpu, (long *)&data->cdump[cpu],
> >>> +                   ENERGY_CORE_MSR);
> >>> +}
> >>> +
> >>> +static int cenergy_dump_show(struct seq_file *s, void *unused) {
> >>> +     struct amd_energy_data *data = s->private;
> >>> +     struct cpumask *cpus_mask;
> >>> +     int i;
> >>> +
> >>> +     cpus_mask = kmalloc(sizeof(*cpus_mask), GFP_KERNEL);
> >>> +     memset(data->cdump, 0, (data->nr_cpus) * sizeof(u64));
> >>> +     cpumask_clear(cpus_mask);
> >>> +     for (i = 0; i < data->nr_cpus; i++) {
> >>> +             if (cpu_online(i))
> >>> +                     cpumask_set_cpu(i, cpus_mask);
> >>> +     }
> >>> +
> >>> +     on_each_cpu_mask(cpus_mask, dump_on_each_cpu, data, true);
> >>> +
> >>> +     for (i = 0; i < data->nr_cpus; i++) {
> >>> +             if (!(i & 3))
> >>> +                     seq_printf(s, "Core %3d: ", i);
> >>> +
> >>> +             seq_printf(s, "%16llu ", data->cdump[i]);
> >>> +             if ((i & 3) == 3)
> >>> +                     seq_puts(s, "\n");
> >>> +     }
> >>> +     seq_puts(s, "\n");
> >>> +
> >>> +     kfree(cpus_mask);
> >>> +     return 0;
> >>> +}
> >>> +DEFINE_SHOW_ATTRIBUTE(cenergy_dump);
> >>> +
> >>> +static int senergy_dump_show(struct seq_file *s, void *unused) {
> >>> +     struct amd_energy_data *data = s->private;
> >>> +     int i, cpu;
> >>> +
> >>> +     for (i = 0; i < data->nr_socks; i++) {
> >>> +             cpu = cpumask_first_and(cpu_online_mask,
> >>> +                                     cpumask_of_node(i));
> >>> +             amd_add_delta(data, data->nr_cpus + i, cpu,
> >>> +                           (long *)&data->sdump[i], ENERGY_PKG_MSR);
> >>> +             seq_printf(s, "Socket %1d: %16llu\n",
> >>> +                        i, data->sdump[i]);
> >>> +     }
> >>> +
> >>> +     return 0;
> >>> +}
> >>> +DEFINE_SHOW_ATTRIBUTE(senergy_dump);
> >>> +
> >>> +static void dump_debugfs_cleanup(void *ddir) {
> >>> +     debugfs_remove_recursive(ddir);
> >>> +}
> >>> +
> >>> +static int create_dump_file(struct device *dev,
> >>> +                         struct amd_energy_data *data) {
> >>> +     struct dentry *debugfs;
> >>> +     char name[] = "amd_energy";
> >>> +
> >>> +     data->cdump = devm_kcalloc(dev, data->nr_cpus,
> >>> +                                sizeof(u64), GFP_KERNEL);
> >>> +     if (!data->cdump)
> >>> +             return -ENOMEM;
> >>> +
> >>> +     data->sdump = devm_kcalloc(dev, data->nr_socks,
> >>> +                                sizeof(u64), GFP_KERNEL);
> >>> +     if (!data->sdump)
> >>> +             return -ENOMEM;
> >>> +
> >>> +     debugfs = debugfs_create_dir(name, NULL);
> >>> +     if (debugfs) {
> >>> +             debugfs_create_file("cenergy_dump", 0440,
> >>> +                                 debugfs, data, &cenergy_dump_fops);
> >>> +             debugfs_create_file("senergy_dump", 0440,
> >>> +                                 debugfs, data, &senergy_dump_fops);
> >>> +             devm_add_action_or_reset(data->hwmon_dev,
> >>> +                                      dump_debugfs_cleanup, debugfs);
> >>> +     }
> >>> +
> >>> +     return 0;
> >>> +}
> >>> +#else
> >>> +
> >>> +static int create_dump_file(struct device *dev,
> >>> +                         struct amd_energy_data *data) {
> >>> +     return 0;
> >>> +}
> >>> +
> >>> +#endif //CONFIG_DEBUG_FS
> >>> +
> >>>  static int amd_energy_probe(struct platform_device *pdev)  {
> >>>       struct amd_energy_data *data;
> >>> @@ -376,6 +482,10 @@ static int amd_energy_probe(struct platform_device *pdev)
> >>>       if (ret)
> >>>               return ret;
> >>>
> >>> +     ret = create_dump_file(dev, data);
> >>> +     if (ret)
> >>> +             return ret;
> >>> +
> >>>       return 0;
> >>>  }
> >>>
> >>>
> > Naveen
> >
>


-- 
Shine bright,
(: Nav :)

Re: FW: [PATCH 4/6] hwmon: amd_energy: let user enable/disable the sw accumulation

[Naveen Krishna Ch]

Hi Guenter

On Tue, 8 Sep 2020 at 21:42, Chatradhi, Naveen Krishna
<NaveenKrishna.Chatradhi@amd.com> wrote:
>
> [AMD Public Use]
>
> [CAUTION: External Email]
>
> On 9/5/20 8:17 AM, Guenter Roeck wrote:
> > On 9/5/20 7:32 AM, Naveen Krishna Chatradhi wrote:
> >> Provide an option "accumulator_status" via sysfs to enable/disable
> >> the software accumulation of energy counters.
> >>
> >> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
> >
> > I am quite substantially opposed to this. I'd be willing to accept a
> > module parameter. However, the code is there, and it is enabled by
> > default, and it should stay enabled by default.
Sure, I will change it back.

> > I don't want to have to deal with people complaining that it suddenly
> > no longer works.
Understood.
> >
> > Also, there needs to be an explanation for why this is needed.
> >
>
> No, wait, without accumulator this driver has zero value.
> Users should just not load the driver if they don't want the overhead associated with accumulation.

The use case we have is to provide an interface to enable/disable
accumulation by the admins, without having to reboot or install
something.
A userspace API is provided which will be used by the applications.

>
> Guenter
>
> > Guenter
> >
> >> ---
> >>  drivers/hwmon/amd_energy.c | 104
> >> ++++++++++++++++++++++++++++++-------
> >>  1 file changed, 86 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c
> >> index 96c61784d05c..c294bea56c02 100644
> >> --- a/drivers/hwmon/amd_energy.c
> >> +++ b/drivers/hwmon/amd_energy.c
> >> @@ -32,6 +32,8 @@
> >>  #define AMD_ENERGY_UNIT_MASK        0x01F00
> >>  #define AMD_ENERGY_MASK             0xFFFFFFFF
> >>
> >> +static struct device_attribute accumulate_attr;
> >> +
> >>  struct sensor_accumulator {
> >>      u64 energy_ctr;
> >>      u64 prev_value;
> >> @@ -42,10 +44,12 @@ struct amd_energy_data {
> >>      const struct hwmon_channel_info *info[2];
> >>      struct hwmon_chip_info chip;
> >>      struct task_struct *wrap_accumulate;
> >> +    struct device *hwmon_dev;
> >>      /* Lock around the accumulator */
> >>      struct mutex lock;
> >>      /* An accumulator for each core and socket */
> >>      struct sensor_accumulator *accums;
> >> +    bool accumulator_status;
> >>      /* Energy Status Units */
> >>      u64 energy_units;
> >>      unsigned int timeout;
> >> @@ -128,13 +132,15 @@ static void amd_add_delta(struct amd_energy_data *data, int ch,
> >>      rdmsrl_safe_on_cpu(cpu, reg, &input);
> >>      input &= AMD_ENERGY_MASK;
> >>
> >> -    accum = &data->accums[ch];
> >> -    if (input >= accum->prev_value)
> >> -            input += accum->energy_ctr -
> >> -                            accum->prev_value;
> >> -    else
> >> -            input += UINT_MAX - accum->prev_value +
> >> -                            accum->energy_ctr;
> >> +    if (data->accumulator_status) {
> >> +            accum = &data->accums[ch];
> >> +            if (input >= accum->prev_value)
> >> +                    input += accum->energy_ctr -
> >> +                                    accum->prev_value;
> >> +            else
> >> +                    input += UINT_MAX - accum->prev_value +
> >> +                                    accum->energy_ctr;
> >> +    }
> >>
> >>      /* Energy consumed = (1/(2^ESU) * RAW * 1000000UL) μJoules */
> >>      *val = div64_ul(input * 1000000UL, BIT(data->energy_units)); @@
> >> -264,9 +270,67 @@ static int amd_create_sensor(struct device *dev,
> >>      return 0;
> >>  }
> >>
> >> +static ssize_t amd_energy_accumulate_show(struct device *dev,
> >> +                                      struct device_attribute *dev_attr,
> >> +                                      char *buf) {
> >> +    struct amd_energy_data *data = dev_get_drvdata(dev);
> >> +
> >> +    return sprintf(buf, "%d\n", data->accumulator_status); }
> >> +
> >> +static ssize_t amd_energy_accumulate_store(struct device *dev,
> >> +                                       struct device_attribute *dev_attr,
> >> +                                       const char *buf, size_t
> >> +count) {
> >> +    struct amd_energy_data *data = dev_get_drvdata(dev);
> >> +    bool input;
> >> +    int ret;
> >> +
> >> +    ret = kstrtobool(buf, &input);
> >> +    if (ret)
> >> +            return ret;
> >> +
> >> +    if (data->accumulator_status == input)
> >> +            return count;
> >> +
> >> +    if (input) {
> >> +            memset(data->accums, 0, (data->nr_cpus + data->nr_socks) *
> >> +                    sizeof(struct sensor_accumulator));
> >> +
> >> +            if (!data->wrap_accumulate) {
> >> +                    data->wrap_accumulate =
> >> +                            kthread_run(energy_accumulator,
> >> +                                        data, "%s", dev_name(dev));
> >> +                    if (IS_ERR(data->wrap_accumulate))
> >> +                            return PTR_ERR(data->wrap_accumulate);
> >> +            }
> >> +    } else {
> >> +            if (data && data->wrap_accumulate) {
> >> +                    ret = kthread_stop(data->wrap_accumulate);
> >> +                    if (ret)
> >> +                            return ret;
> >> +                    data->wrap_accumulate = NULL;
> >> +            }
> >> +    }
> >> +    data->accumulator_status = input;
> >> +
> >> +    return count;
> >> +}
> >> +
> >> +static int create_accumulate_status_file(struct amd_energy_data
> >> +*data) {
> >> +    accumulate_attr.attr.name = "accumulator_status";
> >> +    accumulate_attr.attr.mode = 0664;
> >> +    accumulate_attr.show = amd_energy_accumulate_show;
> >> +    accumulate_attr.store = amd_energy_accumulate_store;
> >> +
> >> +    return sysfs_create_file(&data->hwmon_dev->kobj,
> >> +                             &accumulate_attr.attr); }
> >> +
> >>  static int amd_energy_probe(struct platform_device *pdev)  {
> >> -    struct device *hwmon_dev;
> >>      struct amd_energy_data *data;
> >>      struct device *dev = &pdev->dev;
> >>      int ret;
> >> @@ -290,12 +354,12 @@ static int amd_energy_probe(struct platform_device *pdev)
> >>      mutex_init(&data->lock);
> >>      get_energy_units(data);
> >>
> >> -    hwmon_dev = devm_hwmon_device_register_with_info(dev, DRVNAME,
> >> -                                                     data,
> >> -                                                     &data->chip,
> >> -                                                     NULL);
> >> -    if (IS_ERR(hwmon_dev))
> >> -            return PTR_ERR(hwmon_dev);
> >> +    data->hwmon_dev = devm_hwmon_device_register_with_info(dev, DRVNAME,
> >> +                                                           data,
> >> +                                                           &data->chip,
> >> +                                                           NULL);
> >> +    if (IS_ERR(data->hwmon_dev))
> >> +            return PTR_ERR(data->hwmon_dev);
> >>
> >>      /* Once in 3 minutes for a resolution of 1/2*16 */
> >>      if (data->energy_units == 0x10)
> >> @@ -305,10 +369,12 @@ static int amd_energy_probe(struct platform_device *pdev)
> >>      if (data->energy_units == 0x6)
> >>              data->timeout = 3 * 24 * 60 * 60;
> >>
> >> -    data->wrap_accumulate = kthread_run(energy_accumulator, data,
> >> -                                        "%s", dev_name(hwmon_dev));
> >> -    if (IS_ERR(data->wrap_accumulate))
> >> -            return PTR_ERR(data->wrap_accumulate);
> >> +    /* Disabling the energy accumulation by default */
> >> +    data->accumulator_status = 0;
> >> +
> >> +    ret = create_accumulate_status_file(data);
> >> +    if (ret)
> >> +            return ret;
> >>
> >>      return 0;
> >>  }
> >> @@ -317,6 +383,8 @@ static int amd_energy_remove(struct
> >> platform_device *pdev)  {
> >>      struct amd_energy_data *data = dev_get_drvdata(&pdev->dev);
> >>
> >> +    sysfs_remove_file(&data->hwmon_dev->kobj,
> >> + &accumulate_attr.attr);
> >> +
> >>      if (data && data->wrap_accumulate)
> >>              kthread_stop(data->wrap_accumulate);
> >>
> >>
> >



-- 
Shine bright,
(: Nav :)

Re: FW: [PATCH 5/6] hwmon: amd_energy: dump energy counters via debugfs

[Guenter Roeck]

On Tue, Sep 08, 2020 at 09:40:40PM +0530, Naveen Krishna Ch wrote:
> Hi Guenter
> 
> On Sat, 5 Sep 2020 at 22:28, Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > On 9/5/20 9:41 AM, Naveen Krishna Ch wrote:
> > > Hi Guenter,
> > >
> > >> On 9/5/20 7:32 AM, Naveen Krishna Chatradhi wrote:
> > >>> Use seq_printf to capture the core and socket energies under debugfs
> > >>> path in '/sys/kernel/debug/amd_energy/'
> > >>> file cenergy_dump: To print out the core energy counters file
> > >>> senergy_dump: To print out the socket energy counters
> > >>>
> > >>> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
> > >>
> > >> Isn't this a duplicate of other functionality available in the kernel ?
> > >> I'd have to look it up, but I am quite sure that energy values are already available. Besides that, what is the point of duplicating the hwmon attributes ?
> > >
> > > Idea is to reduce the latency in gathering all the counters (Rome has
> > > 128 cores on 2 sockets) from the user space.
> > > If there is a better way to get this done, please let me know. I shall
> > > implement and resubmit.
> > >
> >
> > Isn't turbostat supposed to be able to do that ?
> Apologies, I was not aware of turbostat, took a look at the turbostat
> code now, this is an elaborate tool which is dependent on msr.ko. At
> present, this tool provides a lot of information in sequence. There is
> no duplication of the code.
> 
> We need this functionality for the following reasons.
> 1. Reduced latency in gathering the energy counters of all cores and packages
> 2. Possible to provide an API to the user space to integrate into
> other tools/frameworks
> 
> Please let me know your opinion.

debugfs should only used for debugging and not to create a backdoor API.
What you are looking for is a more efficient API than the hwmon API
to access sensor data. There is an available API to do that: iio.
You have two options: Register the driver with iio directly, or better
implement a generic hwmon->iio bridge in the hwmon core. I have wanted
to do the latter forever, but never got around to impementing it.

Guenter

> >
> > Guenter
> >
> > >>
> > >> Guenter
> > >>
> > >>> ---
> > >>>  drivers/hwmon/amd_energy.c | 110
> > >>> +++++++++++++++++++++++++++++++++++++
> > >>>  1 file changed, 110 insertions(+)
> > >>>
> > >>> diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c
> > >>> index c294bea56c02..2184bd4510ed 100644
> > >>> --- a/drivers/hwmon/amd_energy.c
> > >>> +++ b/drivers/hwmon/amd_energy.c
> > >>> @@ -8,6 +8,7 @@
> > >>>  #include <linux/bits.h>
> > >>>  #include <linux/cpu.h>
> > >>>  #include <linux/cpumask.h>
> > >>> +#include <linux/debugfs.h>
> > >>>  #include <linux/delay.h>
> > >>>  #include <linux/device.h>
> > >>>  #include <linux/hwmon.h>
> > >>> @@ -20,6 +21,7 @@
> > >>>  #include <linux/platform_device.h>
> > >>>  #include <linux/sched.h>
> > >>>  #include <linux/slab.h>
> > >>> +#include <linux/smp.h>
> > >>>  #include <linux/topology.h>
> > >>>  #include <linux/types.h>
> > >>>
> > >>> @@ -57,6 +59,8 @@ struct amd_energy_data {
> > >>>       int nr_socks;
> > >>>       int core_id;
> > >>>       char (*label)[10];
> > >>> +     u64 *cdump;
> > >>> +     u64 *sdump;
> > >>>  };
> > >>>
> > >>>  static int amd_energy_read_labels(struct device *dev, @@ -329,6
> > >>> +333,108 @@ static int create_accumulate_status_file(struct amd_energy_data *data)
> > >>>                                &accumulate_attr.attr);  }
> > >>>
> > >>> +#ifdef CONFIG_DEBUG_FS
> > >>> +static void dump_on_each_cpu(void *info) {
> > >>> +     struct amd_energy_data *data = info;
> > >>> +     int cpu = smp_processor_id();
> > >>> +
> > >>> +     amd_add_delta(data, cpu, cpu, (long *)&data->cdump[cpu],
> > >>> +                   ENERGY_CORE_MSR);
> > >>> +}
> > >>> +
> > >>> +static int cenergy_dump_show(struct seq_file *s, void *unused) {
> > >>> +     struct amd_energy_data *data = s->private;
> > >>> +     struct cpumask *cpus_mask;
> > >>> +     int i;
> > >>> +
> > >>> +     cpus_mask = kmalloc(sizeof(*cpus_mask), GFP_KERNEL);
> > >>> +     memset(data->cdump, 0, (data->nr_cpus) * sizeof(u64));
> > >>> +     cpumask_clear(cpus_mask);
> > >>> +     for (i = 0; i < data->nr_cpus; i++) {
> > >>> +             if (cpu_online(i))
> > >>> +                     cpumask_set_cpu(i, cpus_mask);
> > >>> +     }
> > >>> +
> > >>> +     on_each_cpu_mask(cpus_mask, dump_on_each_cpu, data, true);
> > >>> +
> > >>> +     for (i = 0; i < data->nr_cpus; i++) {
> > >>> +             if (!(i & 3))
> > >>> +                     seq_printf(s, "Core %3d: ", i);
> > >>> +
> > >>> +             seq_printf(s, "%16llu ", data->cdump[i]);
> > >>> +             if ((i & 3) == 3)
> > >>> +                     seq_puts(s, "\n");
> > >>> +     }
> > >>> +     seq_puts(s, "\n");
> > >>> +
> > >>> +     kfree(cpus_mask);
> > >>> +     return 0;
> > >>> +}
> > >>> +DEFINE_SHOW_ATTRIBUTE(cenergy_dump);
> > >>> +
> > >>> +static int senergy_dump_show(struct seq_file *s, void *unused) {
> > >>> +     struct amd_energy_data *data = s->private;
> > >>> +     int i, cpu;
> > >>> +
> > >>> +     for (i = 0; i < data->nr_socks; i++) {
> > >>> +             cpu = cpumask_first_and(cpu_online_mask,
> > >>> +                                     cpumask_of_node(i));
> > >>> +             amd_add_delta(data, data->nr_cpus + i, cpu,
> > >>> +                           (long *)&data->sdump[i], ENERGY_PKG_MSR);
> > >>> +             seq_printf(s, "Socket %1d: %16llu\n",
> > >>> +                        i, data->sdump[i]);
> > >>> +     }
> > >>> +
> > >>> +     return 0;
> > >>> +}
> > >>> +DEFINE_SHOW_ATTRIBUTE(senergy_dump);
> > >>> +
> > >>> +static void dump_debugfs_cleanup(void *ddir) {
> > >>> +     debugfs_remove_recursive(ddir);
> > >>> +}
> > >>> +
> > >>> +static int create_dump_file(struct device *dev,
> > >>> +                         struct amd_energy_data *data) {
> > >>> +     struct dentry *debugfs;
> > >>> +     char name[] = "amd_energy";
> > >>> +
> > >>> +     data->cdump = devm_kcalloc(dev, data->nr_cpus,
> > >>> +                                sizeof(u64), GFP_KERNEL);
> > >>> +     if (!data->cdump)
> > >>> +             return -ENOMEM;
> > >>> +
> > >>> +     data->sdump = devm_kcalloc(dev, data->nr_socks,
> > >>> +                                sizeof(u64), GFP_KERNEL);
> > >>> +     if (!data->sdump)
> > >>> +             return -ENOMEM;
> > >>> +
> > >>> +     debugfs = debugfs_create_dir(name, NULL);
> > >>> +     if (debugfs) {
> > >>> +             debugfs_create_file("cenergy_dump", 0440,
> > >>> +                                 debugfs, data, &cenergy_dump_fops);
> > >>> +             debugfs_create_file("senergy_dump", 0440,
> > >>> +                                 debugfs, data, &senergy_dump_fops);
> > >>> +             devm_add_action_or_reset(data->hwmon_dev,
> > >>> +                                      dump_debugfs_cleanup, debugfs);
> > >>> +     }
> > >>> +
> > >>> +     return 0;
> > >>> +}
> > >>> +#else
> > >>> +
> > >>> +static int create_dump_file(struct device *dev,
> > >>> +                         struct amd_energy_data *data) {
> > >>> +     return 0;
> > >>> +}
> > >>> +
> > >>> +#endif //CONFIG_DEBUG_FS
> > >>> +
> > >>>  static int amd_energy_probe(struct platform_device *pdev)  {
> > >>>       struct amd_energy_data *data;
> > >>> @@ -376,6 +482,10 @@ static int amd_energy_probe(struct platform_device *pdev)
> > >>>       if (ret)
> > >>>               return ret;
> > >>>
> > >>> +     ret = create_dump_file(dev, data);
> > >>> +     if (ret)
> > >>> +             return ret;
> > >>> +
> > >>>       return 0;
> > >>>  }
> > >>>
> > >>>
> > > Naveen
> > >
> >
> 
> 
> -- 
> Shine bright,
> (: Nav :)

Re: FW: [PATCH 4/6] hwmon: amd_energy: let user enable/disable the sw accumulation

[Guenter Roeck]

On Tue, Sep 08, 2020 at 09:51:23PM +0530, Naveen Krishna Ch wrote:
> Hi Guenter
> 
> On Tue, 8 Sep 2020 at 21:42, Chatradhi, Naveen Krishna
> <NaveenKrishna.Chatradhi@amd.com> wrote:
> >
> > [AMD Public Use]
> >
> > [CAUTION: External Email]
> >
> > On 9/5/20 8:17 AM, Guenter Roeck wrote:
> > > On 9/5/20 7:32 AM, Naveen Krishna Chatradhi wrote:
> > >> Provide an option "accumulator_status" via sysfs to enable/disable
> > >> the software accumulation of energy counters.
> > >>
> > >> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
> > >
> > > I am quite substantially opposed to this. I'd be willing to accept a
> > > module parameter. However, the code is there, and it is enabled by
> > > default, and it should stay enabled by default.
> Sure, I will change it back.
> 
> > > I don't want to have to deal with people complaining that it suddenly
> > > no longer works.
> Understood.
> > >
> > > Also, there needs to be an explanation for why this is needed.
> > >
> >
> > No, wait, without accumulator this driver has zero value.
> > Users should just not load the driver if they don't want the overhead associated with accumulation.
> 
> The use case we have is to provide an interface to enable/disable
> accumulation by the admins, without having to reboot or install
> something.
> A userspace API is provided which will be used by the applications.
> 

Really, that should not be done. If accumulation is not wanted, the
driver is pointless and should be unloaded (or not be loaded to start
with).

Guenter

Re: FW: [PATCH 5/6] hwmon: amd_energy: dump energy counters via debugfs

[Naveen Krishna Ch]

Hi Guenter

On Tue, 8 Sep 2020 at 22:04, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Tue, Sep 08, 2020 at 09:40:40PM +0530, Naveen Krishna Ch wrote:
> > Hi Guenter
> >
> > On Sat, 5 Sep 2020 at 22:28, Guenter Roeck <linux@roeck-us.net> wrote:
> > >
> > > On 9/5/20 9:41 AM, Naveen Krishna Ch wrote:
> > > > Hi Guenter,
> > > >
> > > >> On 9/5/20 7:32 AM, Naveen Krishna Chatradhi wrote:
> > > >>> Use seq_printf to capture the core and socket energies under debugfs
> > > >>> path in '/sys/kernel/debug/amd_energy/'
> > > >>> file cenergy_dump: To print out the core energy counters file
> > > >>> senergy_dump: To print out the socket energy counters
> > > >>>
> > > >>> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
> > > >>
> > > >> Isn't this a duplicate of other functionality available in the kernel ?
> > > >> I'd have to look it up, but I am quite sure that energy values are already available. Besides that, what is the point of duplicating the hwmon attributes ?
> > > >
> > > > Idea is to reduce the latency in gathering all the counters (Rome has
> > > > 128 cores on 2 sockets) from the user space.
> > > > If there is a better way to get this done, please let me know. I shall
> > > > implement and resubmit.
> > > >
> > >
> > > Isn't turbostat supposed to be able to do that ?
> > Apologies, I was not aware of turbostat, took a look at the turbostat
> > code now, this is an elaborate tool which is dependent on msr.ko. At
> > present, this tool provides a lot of information in sequence. There is
> > no duplication of the code.
> >
> > We need this functionality for the following reasons.
> > 1. Reduced latency in gathering the energy counters of all cores and packages
> > 2. Possible to provide an API to the user space to integrate into
> > other tools/frameworks
> >
> > Please let me know your opinion.
>
> debugfs should only used for debugging and not to create a backdoor API.
> What you are looking for is a more efficient API than the hwmon API
Yes and when i looked around i found this debugfs implementation in
k10temp driver, providing the svi and thm information. So, I have
implemented accordingly.  Should have discussed this earlier.

> to access sensor data. There is an available API to do that: iio.
> You have two options: Register the driver with iio directly, or better
> implement a generic hwmon->iio bridge in the hwmon core. I have wanted
> to do the latter forever, but never got around to impementing it.
I've had some experience with iio drivers in the past, i will look
into this. In the meanwhile, can we keep this implementation here ?


>
> Guenter
>
> > >
> > > Guenter
> > >
> > > >>
> > > >> Guenter
> > > >>
> > > >>> ---
> > > >>>  drivers/hwmon/amd_energy.c | 110
> > > >>> +++++++++++++++++++++++++++++++++++++
> > > >>>  1 file changed, 110 insertions(+)
> > > >>>
> > > >>> diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c
> > > >>> index c294bea56c02..2184bd4510ed 100644
> > > >>> --- a/drivers/hwmon/amd_energy.c
> > > >>> +++ b/drivers/hwmon/amd_energy.c
> > > >>> @@ -8,6 +8,7 @@
> > > >>>  #include <linux/bits.h>
> > > >>>  #include <linux/cpu.h>
> > > >>>  #include <linux/cpumask.h>
> > > >>> +#include <linux/debugfs.h>
> > > >>>  #include <linux/delay.h>
> > > >>>  #include <linux/device.h>
> > > >>>  #include <linux/hwmon.h>
> > > >>> @@ -20,6 +21,7 @@
> > > >>>  #include <linux/platform_device.h>
> > > >>>  #include <linux/sched.h>
> > > >>>  #include <linux/slab.h>
> > > >>> +#include <linux/smp.h>
> > > >>>  #include <linux/topology.h>
> > > >>>  #include <linux/types.h>
> > > >>>
> > > >>> @@ -57,6 +59,8 @@ struct amd_energy_data {
> > > >>>       int nr_socks;
> > > >>>       int core_id;
> > > >>>       char (*label)[10];
> > > >>> +     u64 *cdump;
> > > >>> +     u64 *sdump;
> > > >>>  };
> > > >>>
> > > >>>  static int amd_energy_read_labels(struct device *dev, @@ -329,6
> > > >>> +333,108 @@ static int create_accumulate_status_file(struct amd_energy_data *data)
> > > >>>                                &accumulate_attr.attr);  }
> > > >>>
> > > >>> +#ifdef CONFIG_DEBUG_FS
> > > >>> +static void dump_on_each_cpu(void *info) {
> > > >>> +     struct amd_energy_data *data = info;
> > > >>> +     int cpu = smp_processor_id();
> > > >>> +
> > > >>> +     amd_add_delta(data, cpu, cpu, (long *)&data->cdump[cpu],
> > > >>> +                   ENERGY_CORE_MSR);
> > > >>> +}
> > > >>> +
> > > >>> +static int cenergy_dump_show(struct seq_file *s, void *unused) {
> > > >>> +     struct amd_energy_data *data = s->private;
> > > >>> +     struct cpumask *cpus_mask;
> > > >>> +     int i;
> > > >>> +
> > > >>> +     cpus_mask = kmalloc(sizeof(*cpus_mask), GFP_KERNEL);
> > > >>> +     memset(data->cdump, 0, (data->nr_cpus) * sizeof(u64));
> > > >>> +     cpumask_clear(cpus_mask);
> > > >>> +     for (i = 0; i < data->nr_cpus; i++) {
> > > >>> +             if (cpu_online(i))
> > > >>> +                     cpumask_set_cpu(i, cpus_mask);
> > > >>> +     }
> > > >>> +
> > > >>> +     on_each_cpu_mask(cpus_mask, dump_on_each_cpu, data, true);
> > > >>> +
> > > >>> +     for (i = 0; i < data->nr_cpus; i++) {
> > > >>> +             if (!(i & 3))
> > > >>> +                     seq_printf(s, "Core %3d: ", i);
> > > >>> +
> > > >>> +             seq_printf(s, "%16llu ", data->cdump[i]);
> > > >>> +             if ((i & 3) == 3)
> > > >>> +                     seq_puts(s, "\n");
> > > >>> +     }
> > > >>> +     seq_puts(s, "\n");
> > > >>> +
> > > >>> +     kfree(cpus_mask);
> > > >>> +     return 0;
> > > >>> +}
> > > >>> +DEFINE_SHOW_ATTRIBUTE(cenergy_dump);
> > > >>> +
> > > >>> +static int senergy_dump_show(struct seq_file *s, void *unused) {
> > > >>> +     struct amd_energy_data *data = s->private;
> > > >>> +     int i, cpu;
> > > >>> +
> > > >>> +     for (i = 0; i < data->nr_socks; i++) {
> > > >>> +             cpu = cpumask_first_and(cpu_online_mask,
> > > >>> +                                     cpumask_of_node(i));
> > > >>> +             amd_add_delta(data, data->nr_cpus + i, cpu,
> > > >>> +                           (long *)&data->sdump[i], ENERGY_PKG_MSR);
> > > >>> +             seq_printf(s, "Socket %1d: %16llu\n",
> > > >>> +                        i, data->sdump[i]);
> > > >>> +     }
> > > >>> +
> > > >>> +     return 0;
> > > >>> +}
> > > >>> +DEFINE_SHOW_ATTRIBUTE(senergy_dump);
> > > >>> +
> > > >>> +static void dump_debugfs_cleanup(void *ddir) {
> > > >>> +     debugfs_remove_recursive(ddir);
> > > >>> +}
> > > >>> +
> > > >>> +static int create_dump_file(struct device *dev,
> > > >>> +                         struct amd_energy_data *data) {
> > > >>> +     struct dentry *debugfs;
> > > >>> +     char name[] = "amd_energy";
> > > >>> +
> > > >>> +     data->cdump = devm_kcalloc(dev, data->nr_cpus,
> > > >>> +                                sizeof(u64), GFP_KERNEL);
> > > >>> +     if (!data->cdump)
> > > >>> +             return -ENOMEM;
> > > >>> +
> > > >>> +     data->sdump = devm_kcalloc(dev, data->nr_socks,
> > > >>> +                                sizeof(u64), GFP_KERNEL);
> > > >>> +     if (!data->sdump)
> > > >>> +             return -ENOMEM;
> > > >>> +
> > > >>> +     debugfs = debugfs_create_dir(name, NULL);
> > > >>> +     if (debugfs) {
> > > >>> +             debugfs_create_file("cenergy_dump", 0440,
> > > >>> +                                 debugfs, data, &cenergy_dump_fops);
> > > >>> +             debugfs_create_file("senergy_dump", 0440,
> > > >>> +                                 debugfs, data, &senergy_dump_fops);
> > > >>> +             devm_add_action_or_reset(data->hwmon_dev,
> > > >>> +                                      dump_debugfs_cleanup, debugfs);
> > > >>> +     }
> > > >>> +
> > > >>> +     return 0;
> > > >>> +}
> > > >>> +#else
> > > >>> +
> > > >>> +static int create_dump_file(struct device *dev,
> > > >>> +                         struct amd_energy_data *data) {
> > > >>> +     return 0;
> > > >>> +}
> > > >>> +
> > > >>> +#endif //CONFIG_DEBUG_FS
> > > >>> +
> > > >>>  static int amd_energy_probe(struct platform_device *pdev)  {
> > > >>>       struct amd_energy_data *data;
> > > >>> @@ -376,6 +482,10 @@ static int amd_energy_probe(struct platform_device *pdev)
> > > >>>       if (ret)
> > > >>>               return ret;
> > > >>>
> > > >>> +     ret = create_dump_file(dev, data);
> > > >>> +     if (ret)
> > > >>> +             return ret;
> > > >>> +
> > > >>>       return 0;
> > > >>>  }
> > > >>>
> > > >>>
> > > > Naveen
> > > >
> > >
> >
> >
> > --
> > Shine bright,
> > (: Nav :)



--
Shine bright,
(: Nav :)

Re: FW: [PATCH 5/6] hwmon: amd_energy: dump energy counters via debugfs

[Guenter Roeck]

On 9/8/20 9:46 AM, Naveen Krishna Ch wrote:
> Hi Guenter
> 
> On Tue, 8 Sep 2020 at 22:04, Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On Tue, Sep 08, 2020 at 09:40:40PM +0530, Naveen Krishna Ch wrote:
>>> Hi Guenter
>>>
>>> On Sat, 5 Sep 2020 at 22:28, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>
>>>> On 9/5/20 9:41 AM, Naveen Krishna Ch wrote:
>>>>> Hi Guenter,
>>>>>
>>>>>> On 9/5/20 7:32 AM, Naveen Krishna Chatradhi wrote:
>>>>>>> Use seq_printf to capture the core and socket energies under debugfs
>>>>>>> path in '/sys/kernel/debug/amd_energy/'
>>>>>>> file cenergy_dump: To print out the core energy counters file
>>>>>>> senergy_dump: To print out the socket energy counters
>>>>>>>
>>>>>>> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
>>>>>>
>>>>>> Isn't this a duplicate of other functionality available in the kernel ?
>>>>>> I'd have to look it up, but I am quite sure that energy values are already available. Besides that, what is the point of duplicating the hwmon attributes ?
>>>>>
>>>>> Idea is to reduce the latency in gathering all the counters (Rome has
>>>>> 128 cores on 2 sockets) from the user space.
>>>>> If there is a better way to get this done, please let me know. I shall
>>>>> implement and resubmit.
>>>>>
>>>>
>>>> Isn't turbostat supposed to be able to do that ?
>>> Apologies, I was not aware of turbostat, took a look at the turbostat
>>> code now, this is an elaborate tool which is dependent on msr.ko. At
>>> present, this tool provides a lot of information in sequence. There is
>>> no duplication of the code.
>>>
>>> We need this functionality for the following reasons.
>>> 1. Reduced latency in gathering the energy counters of all cores and packages
>>> 2. Possible to provide an API to the user space to integrate into
>>> other tools/frameworks
>>>
>>> Please let me know your opinion.
>>
>> debugfs should only used for debugging and not to create a backdoor API.
>> What you are looking for is a more efficient API than the hwmon API
> Yes and when i looked around i found this debugfs implementation in
> k10temp driver, providing the svi and thm information. So, I have
> implemented accordingly.  Should have discussed this earlier.
> 

That is purely for debugging, needed because AMD doesn't publish
technical documents describing the register values. If it is used
to argue for a backdoor API, I'll take it out. Actually, I'll submit
a patch to do just that.

>> to access sensor data. There is an available API to do that: iio.
>> You have two options: Register the driver with iio directly, or better
>> implement a generic hwmon->iio bridge in the hwmon core. I have wanted
>> to do the latter forever, but never got around to impementing it.
> I've had some experience with iio drivers in the past, i will look
> into this. In the meanwhile, can we keep this implementation here ?
> 

No.

Guenter

> 
>>
>> Guenter
>>
>>>>
>>>> Guenter
>>>>
>>>>>>
>>>>>> Guenter
>>>>>>
>>>>>>> ---
>>>>>>>  drivers/hwmon/amd_energy.c | 110
>>>>>>> +++++++++++++++++++++++++++++++++++++
>>>>>>>  1 file changed, 110 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c
>>>>>>> index c294bea56c02..2184bd4510ed 100644
>>>>>>> --- a/drivers/hwmon/amd_energy.c
>>>>>>> +++ b/drivers/hwmon/amd_energy.c
>>>>>>> @@ -8,6 +8,7 @@
>>>>>>>  #include <linux/bits.h>
>>>>>>>  #include <linux/cpu.h>
>>>>>>>  #include <linux/cpumask.h>
>>>>>>> +#include <linux/debugfs.h>
>>>>>>>  #include <linux/delay.h>
>>>>>>>  #include <linux/device.h>
>>>>>>>  #include <linux/hwmon.h>
>>>>>>> @@ -20,6 +21,7 @@
>>>>>>>  #include <linux/platform_device.h>
>>>>>>>  #include <linux/sched.h>
>>>>>>>  #include <linux/slab.h>
>>>>>>> +#include <linux/smp.h>
>>>>>>>  #include <linux/topology.h>
>>>>>>>  #include <linux/types.h>
>>>>>>>
>>>>>>> @@ -57,6 +59,8 @@ struct amd_energy_data {
>>>>>>>       int nr_socks;
>>>>>>>       int core_id;
>>>>>>>       char (*label)[10];
>>>>>>> +     u64 *cdump;
>>>>>>> +     u64 *sdump;
>>>>>>>  };
>>>>>>>
>>>>>>>  static int amd_energy_read_labels(struct device *dev, @@ -329,6
>>>>>>> +333,108 @@ static int create_accumulate_status_file(struct amd_energy_data *data)
>>>>>>>                                &accumulate_attr.attr);  }
>>>>>>>
>>>>>>> +#ifdef CONFIG_DEBUG_FS
>>>>>>> +static void dump_on_each_cpu(void *info) {
>>>>>>> +     struct amd_energy_data *data = info;
>>>>>>> +     int cpu = smp_processor_id();
>>>>>>> +
>>>>>>> +     amd_add_delta(data, cpu, cpu, (long *)&data->cdump[cpu],
>>>>>>> +                   ENERGY_CORE_MSR);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static int cenergy_dump_show(struct seq_file *s, void *unused) {
>>>>>>> +     struct amd_energy_data *data = s->private;
>>>>>>> +     struct cpumask *cpus_mask;
>>>>>>> +     int i;
>>>>>>> +
>>>>>>> +     cpus_mask = kmalloc(sizeof(*cpus_mask), GFP_KERNEL);
>>>>>>> +     memset(data->cdump, 0, (data->nr_cpus) * sizeof(u64));
>>>>>>> +     cpumask_clear(cpus_mask);
>>>>>>> +     for (i = 0; i < data->nr_cpus; i++) {
>>>>>>> +             if (cpu_online(i))
>>>>>>> +                     cpumask_set_cpu(i, cpus_mask);
>>>>>>> +     }
>>>>>>> +
>>>>>>> +     on_each_cpu_mask(cpus_mask, dump_on_each_cpu, data, true);
>>>>>>> +
>>>>>>> +     for (i = 0; i < data->nr_cpus; i++) {
>>>>>>> +             if (!(i & 3))
>>>>>>> +                     seq_printf(s, "Core %3d: ", i);
>>>>>>> +
>>>>>>> +             seq_printf(s, "%16llu ", data->cdump[i]);
>>>>>>> +             if ((i & 3) == 3)
>>>>>>> +                     seq_puts(s, "\n");
>>>>>>> +     }
>>>>>>> +     seq_puts(s, "\n");
>>>>>>> +
>>>>>>> +     kfree(cpus_mask);
>>>>>>> +     return 0;
>>>>>>> +}
>>>>>>> +DEFINE_SHOW_ATTRIBUTE(cenergy_dump);
>>>>>>> +
>>>>>>> +static int senergy_dump_show(struct seq_file *s, void *unused) {
>>>>>>> +     struct amd_energy_data *data = s->private;
>>>>>>> +     int i, cpu;
>>>>>>> +
>>>>>>> +     for (i = 0; i < data->nr_socks; i++) {
>>>>>>> +             cpu = cpumask_first_and(cpu_online_mask,
>>>>>>> +                                     cpumask_of_node(i));
>>>>>>> +             amd_add_delta(data, data->nr_cpus + i, cpu,
>>>>>>> +                           (long *)&data->sdump[i], ENERGY_PKG_MSR);
>>>>>>> +             seq_printf(s, "Socket %1d: %16llu\n",
>>>>>>> +                        i, data->sdump[i]);
>>>>>>> +     }
>>>>>>> +
>>>>>>> +     return 0;
>>>>>>> +}
>>>>>>> +DEFINE_SHOW_ATTRIBUTE(senergy_dump);
>>>>>>> +
>>>>>>> +static void dump_debugfs_cleanup(void *ddir) {
>>>>>>> +     debugfs_remove_recursive(ddir);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static int create_dump_file(struct device *dev,
>>>>>>> +                         struct amd_energy_data *data) {
>>>>>>> +     struct dentry *debugfs;
>>>>>>> +     char name[] = "amd_energy";
>>>>>>> +
>>>>>>> +     data->cdump = devm_kcalloc(dev, data->nr_cpus,
>>>>>>> +                                sizeof(u64), GFP_KERNEL);
>>>>>>> +     if (!data->cdump)
>>>>>>> +             return -ENOMEM;
>>>>>>> +
>>>>>>> +     data->sdump = devm_kcalloc(dev, data->nr_socks,
>>>>>>> +                                sizeof(u64), GFP_KERNEL);
>>>>>>> +     if (!data->sdump)
>>>>>>> +             return -ENOMEM;
>>>>>>> +
>>>>>>> +     debugfs = debugfs_create_dir(name, NULL);
>>>>>>> +     if (debugfs) {
>>>>>>> +             debugfs_create_file("cenergy_dump", 0440,
>>>>>>> +                                 debugfs, data, &cenergy_dump_fops);
>>>>>>> +             debugfs_create_file("senergy_dump", 0440,
>>>>>>> +                                 debugfs, data, &senergy_dump_fops);
>>>>>>> +             devm_add_action_or_reset(data->hwmon_dev,
>>>>>>> +                                      dump_debugfs_cleanup, debugfs);
>>>>>>> +     }
>>>>>>> +
>>>>>>> +     return 0;
>>>>>>> +}
>>>>>>> +#else
>>>>>>> +
>>>>>>> +static int create_dump_file(struct device *dev,
>>>>>>> +                         struct amd_energy_data *data) {
>>>>>>> +     return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>> +#endif //CONFIG_DEBUG_FS
>>>>>>> +
>>>>>>>  static int amd_energy_probe(struct platform_device *pdev)  {
>>>>>>>       struct amd_energy_data *data;
>>>>>>> @@ -376,6 +482,10 @@ static int amd_energy_probe(struct platform_device *pdev)
>>>>>>>       if (ret)
>>>>>>>               return ret;
>>>>>>>
>>>>>>> +     ret = create_dump_file(dev, data);
>>>>>>> +     if (ret)
>>>>>>> +             return ret;
>>>>>>> +
>>>>>>>       return 0;
>>>>>>>  }
>>>>>>>
>>>>>>>
>>>>> Naveen
>>>>>
>>>>
>>>
>>>
>>> --
>>> Shine bright,
>>> (: Nav :)
> 
> 
> 
> --
> Shine bright,
> (: Nav :)
> 

RE: FW: [PATCH 5/6] hwmon: amd_energy: dump energy counters via debugfs

[Chatradhi, Naveen Krishna]

[AMD Public Use]

Hi Guenter,

-----Original Message-----
From: linux-hwmon-owner@vger.kernel.org <linux-hwmon-owner@vger.kernel.org> On Behalf Of Guenter Roeck
Sent: Tuesday, September 8, 2020 10:42 PM
To: Naveen Krishna Ch <naveenkrishna.ch@gmail.com>
Cc: linux-hwmon@vger.kernel.org; Guenter Roeck <groeck7@gmail.com>
Subject: Re: FW: [PATCH 5/6] hwmon: amd_energy: dump energy counters via debugfs

[CAUTION: External Email]

On 9/8/20 9:46 AM, Naveen Krishna Ch wrote:
> Hi Guenter
>
> On Tue, 8 Sep 2020 at 22:04, Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On Tue, Sep 08, 2020 at 09:40:40PM +0530, Naveen Krishna Ch wrote:
>>> Hi Guenter
>>>
>>> On Sat, 5 Sep 2020 at 22:28, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>
>>>> On 9/5/20 9:41 AM, Naveen Krishna Ch wrote:
>>>>> Hi Guenter,
>>>>>
>>>>>> On 9/5/20 7:32 AM, Naveen Krishna Chatradhi wrote:
>>>>>>> Use seq_printf to capture the core and socket energies under 
>>>>>>> debugfs path in '/sys/kernel/debug/amd_energy/'
>>>>>>> file cenergy_dump: To print out the core energy counters file
>>>>>>> senergy_dump: To print out the socket energy counters
>>>>>>>
>>>>>>> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
>>>>>>
>>>>>> Isn't this a duplicate of other functionality available in the kernel ?
>>>>>> I'd have to look it up, but I am quite sure that energy values are already available. Besides that, what is the point of duplicating the hwmon attributes ?
>>>>>
>>>>> Idea is to reduce the latency in gathering all the counters (Rome 
>>>>> has
>>>>> 128 cores on 2 sockets) from the user space.
>>>>> If there is a better way to get this done, please let me know. I 
>>>>> shall implement and resubmit.
>>>>>
>>>>
>>>> Isn't turbostat supposed to be able to do that ?
>>> Apologies, I was not aware of turbostat, took a look at the 
>>> turbostat code now, this is an elaborate tool which is dependent on 
>>> msr.ko. At present, this tool provides a lot of information in 
>>> sequence. There is no duplication of the code.
>>>
>>> We need this functionality for the following reasons.
>>> 1. Reduced latency in gathering the energy counters of all cores and 
>>> packages 2. Possible to provide an API to the user space to 
>>> integrate into other tools/frameworks
>>>
>>> Please let me know your opinion.
>>
>> debugfs should only used for debugging and not to create a backdoor API.
>> What you are looking for is a more efficient API than the hwmon API
> Yes and when i looked around i found this debugfs implementation in 
> k10temp driver, providing the svi and thm information. So, I have 
> implemented accordingly.  Should have discussed this earlier.
>

That is purely for debugging, needed because AMD doesn't publish technical documents describing the register values. If it is used to argue for a backdoor API, I'll take it out. Actually, I'll submit a patch to do just that.
[naveenk:] Didn't mean to use it for an argument. Merely trying to give a reason why I chose to implement it this way. I agree, debugfs is not for this purpose. 

>> to access sensor data. There is an available API to do that: iio.
>> You have two options: Register the driver with iio directly, or 
>> better implement a generic hwmon->iio bridge in the hwmon core. I 
>> have wanted to do the latter forever, but never got around to impementing it.
> I've had some experience with iio drivers in the past, i will look 
> into this. In the meanwhile, can we keep this implementation here ?
[naveenk:] I explored the IIO subsystem on your suggestions. IIO seems to be supporting INT, Fractions(INT_PLUS_MICRO, INT_PLUS_NANO, FRACTIONAL) and IIO_VAL_INT_MULTIPLE for the reads. The energy counter values we are reporting are 64bits, there is no return multi support for the fractions at present.

Is it acceptable to add IIO_VAL_U64_MULTIPLE support first, any thoughts ?  If this is acceptable, i will create a bridge hwmon->iio to update those values from the existing driver.

>

No.

Guenter

>
>>
>> Guenter
>>
>>>>
>>>> Guenter
>>>>
>>>>>>
>>>>>> Guenter
>>>>>>
>>>>>>> ---
>>>>>>>  drivers/hwmon/amd_energy.c | 110
>>>>>>> +++++++++++++++++++++++++++++++++++++
>>>>>>>  1 file changed, 110 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/hwmon/amd_energy.c 
>>>>>>> b/drivers/hwmon/amd_energy.c index c294bea56c02..2184bd4510ed 
>>>>>>> 100644
>>>>>>> --- a/drivers/hwmon/amd_energy.c
>>>>>>> +++ b/drivers/hwmon/amd_energy.c
>>>>>>> @@ -8,6 +8,7 @@
>>>>>>>  #include <linux/bits.h>
>>>>>>>  #include <linux/cpu.h>
>>>>>>>  #include <linux/cpumask.h>
>>>>>>> +#include <linux/debugfs.h>
>>>>>>>  #include <linux/delay.h>
>>>>>>>  #include <linux/device.h>
>>>>>>>  #include <linux/hwmon.h>
>>>>>>> @@ -20,6 +21,7 @@
>>>>>>>  #include <linux/platform_device.h>  #include <linux/sched.h>  
>>>>>>> #include <linux/slab.h>
>>>>>>> +#include <linux/smp.h>
>>>>>>>  #include <linux/topology.h>
>>>>>>>  #include <linux/types.h>
>>>>>>>
>>>>>>> @@ -57,6 +59,8 @@ struct amd_energy_data {
>>>>>>>       int nr_socks;
>>>>>>>       int core_id;
>>>>>>>       char (*label)[10];
>>>>>>> +     u64 *cdump;
>>>>>>> +     u64 *sdump;
>>>>>>>  };
>>>>>>>
>>>>>>>  static int amd_energy_read_labels(struct device *dev, @@ -329,6
>>>>>>> +333,108 @@ static int create_accumulate_status_file(struct 
>>>>>>> +amd_energy_data *data)
>>>>>>>                                &accumulate_attr.attr);  }
>>>>>>>
>>>>>>> +#ifdef CONFIG_DEBUG_FS
>>>>>>> +static void dump_on_each_cpu(void *info) {
>>>>>>> +     struct amd_energy_data *data = info;
>>>>>>> +     int cpu = smp_processor_id();
>>>>>>> +
>>>>>>> +     amd_add_delta(data, cpu, cpu, (long *)&data->cdump[cpu],
>>>>>>> +                   ENERGY_CORE_MSR); }
>>>>>>> +
>>>>>>> +static int cenergy_dump_show(struct seq_file *s, void *unused) {
>>>>>>> +     struct amd_energy_data *data = s->private;
>>>>>>> +     struct cpumask *cpus_mask;
>>>>>>> +     int i;
>>>>>>> +
>>>>>>> +     cpus_mask = kmalloc(sizeof(*cpus_mask), GFP_KERNEL);
>>>>>>> +     memset(data->cdump, 0, (data->nr_cpus) * sizeof(u64));
>>>>>>> +     cpumask_clear(cpus_mask);
>>>>>>> +     for (i = 0; i < data->nr_cpus; i++) {
>>>>>>> +             if (cpu_online(i))
>>>>>>> +                     cpumask_set_cpu(i, cpus_mask);
>>>>>>> +     }
>>>>>>> +
>>>>>>> +     on_each_cpu_mask(cpus_mask, dump_on_each_cpu, data, true);
>>>>>>> +
>>>>>>> +     for (i = 0; i < data->nr_cpus; i++) {
>>>>>>> +             if (!(i & 3))
>>>>>>> +                     seq_printf(s, "Core %3d: ", i);
>>>>>>> +
>>>>>>> +             seq_printf(s, "%16llu ", data->cdump[i]);
>>>>>>> +             if ((i & 3) == 3)
>>>>>>> +                     seq_puts(s, "\n");
>>>>>>> +     }
>>>>>>> +     seq_puts(s, "\n");
>>>>>>> +
>>>>>>> +     kfree(cpus_mask);
>>>>>>> +     return 0;
>>>>>>> +}
>>>>>>> +DEFINE_SHOW_ATTRIBUTE(cenergy_dump);
>>>>>>> +
>>>>>>> +static int senergy_dump_show(struct seq_file *s, void *unused) {
>>>>>>> +     struct amd_energy_data *data = s->private;
>>>>>>> +     int i, cpu;
>>>>>>> +
>>>>>>> +     for (i = 0; i < data->nr_socks; i++) {
>>>>>>> +             cpu = cpumask_first_and(cpu_online_mask,
>>>>>>> +                                     cpumask_of_node(i));
>>>>>>> +             amd_add_delta(data, data->nr_cpus + i, cpu,
>>>>>>> +                           (long *)&data->sdump[i], ENERGY_PKG_MSR);
>>>>>>> +             seq_printf(s, "Socket %1d: %16llu\n",
>>>>>>> +                        i, data->sdump[i]);
>>>>>>> +     }
>>>>>>> +
>>>>>>> +     return 0;
>>>>>>> +}
>>>>>>> +DEFINE_SHOW_ATTRIBUTE(senergy_dump);
>>>>>>> +
>>>>>>> +static void dump_debugfs_cleanup(void *ddir) {
>>>>>>> +     debugfs_remove_recursive(ddir); }
>>>>>>> +
>>>>>>> +static int create_dump_file(struct device *dev,
>>>>>>> +                         struct amd_energy_data *data) {
>>>>>>> +     struct dentry *debugfs;
>>>>>>> +     char name[] = "amd_energy";
>>>>>>> +
>>>>>>> +     data->cdump = devm_kcalloc(dev, data->nr_cpus,
>>>>>>> +                                sizeof(u64), GFP_KERNEL);
>>>>>>> +     if (!data->cdump)
>>>>>>> +             return -ENOMEM;
>>>>>>> +
>>>>>>> +     data->sdump = devm_kcalloc(dev, data->nr_socks,
>>>>>>> +                                sizeof(u64), GFP_KERNEL);
>>>>>>> +     if (!data->sdump)
>>>>>>> +             return -ENOMEM;
>>>>>>> +
>>>>>>> +     debugfs = debugfs_create_dir(name, NULL);
>>>>>>> +     if (debugfs) {
>>>>>>> +             debugfs_create_file("cenergy_dump", 0440,
>>>>>>> +                                 debugfs, data, &cenergy_dump_fops);
>>>>>>> +             debugfs_create_file("senergy_dump", 0440,
>>>>>>> +                                 debugfs, data, &senergy_dump_fops);
>>>>>>> +             devm_add_action_or_reset(data->hwmon_dev,
>>>>>>> +                                      dump_debugfs_cleanup, debugfs);
>>>>>>> +     }
>>>>>>> +
>>>>>>> +     return 0;
>>>>>>> +}
>>>>>>> +#else
>>>>>>> +
>>>>>>> +static int create_dump_file(struct device *dev,
>>>>>>> +                         struct amd_energy_data *data) {
>>>>>>> +     return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>> +#endif //CONFIG_DEBUG_FS
>>>>>>> +
>>>>>>>  static int amd_energy_probe(struct platform_device *pdev)  {
>>>>>>>       struct amd_energy_data *data; @@ -376,6 +482,10 @@ static 
>>>>>>> int amd_energy_probe(struct platform_device *pdev)
>>>>>>>       if (ret)
>>>>>>>               return ret;
>>>>>>>
>>>>>>> +     ret = create_dump_file(dev, data);
>>>>>>> +     if (ret)
>>>>>>> +             return ret;
>>>>>>> +
>>>>>>>       return 0;
>>>>>>>  }
>>>>>>>
>>>>>>>
>>>>> Naveen
>>>>>
>>>>
>>>
>>>
>>> --
>>> Shine bright,
>>> (: Nav :)
>
>
>
> --
> Shine bright,
> (: Nav :)
>

RE: [PATCH 0/6] RFC: hwmon: few improvements to amd_energy driver

[Chatradhi, Naveen Krishna]

[AMD Public Use]

Hi Guenter,

-----Original Message-----
From: Chatradhi, Naveen Krishna <NaveenKrishna.Chatradhi@amd.com> 
Sent: Saturday, September 5, 2020 8:02 PM
To: linux-hwmon@vger.kernel.org
Cc: linux@roeck-us.net; Chatradhi, Naveen Krishna <NaveenKrishna.Chatradhi@amd.com>
Subject: [PATCH 0/6] RFC: hwmon: few improvements to amd_energy driver

Hi Guenter,

Would like to know your feedback on the following features for the amd_energy driver.

1) Sysfs entry for dumping energy counters of all the cores
    - On latest CPUs there can be as many as 128 cores.
      An ABI for dumping all 128 counters using seq_printf()
      to a debugfs/sysfs file would save a lot of cycles.

2) Enable/Disable the accumulation, Disabled by default
    - The accumulator thread may introduce some noise.
      Providing a knob to enable/disable (start/stop) the
      accumulation in software.

3) Accumulator Interval change based on reported resolution
    - Frequency of the accumulator thread can be set during
      the probe based on fine grain (1.625 micro J) or course
      grain (0.125 milli J) resolutions.
[naveenk:] I will submit the v2 dropping #1 and #2 for now. 

Akshay Gupta (1):
  hwmon: amd_energy: Move label out of accumulation structure

Naveen Krishna Chatradhi (5):
  hwmon: amd_energy: optimize accumulation interval
  hwmon: amd_energy: Improve the accumulation logic
  hwmon: amd_energy: let user enable/disable the sw accumulation
  hwmon: amd_energy: dump energy counters via debugfs
  hwmon: (amd_energy) Update driver documentation

 Documentation/hwmon/amd_energy.rst |  19 ++
 drivers/hwmon/amd_energy.c         | 351 +++++++++++++++++++++--------
 2 files changed, 273 insertions(+), 97 deletions(-)

--
2.26.2