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

Guenter Roeck
2020-09-05T15:19:45Z

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;
> }
>
>