Re: [PATCH 1/2] hwmon: Add amd_energy driver to report energy counters

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

Thank you for taking time to review and provide feedback.
If you can kindly address a few questions, i would start addressing
your comments and submit another version.

On Fri, 10 Apr 2020 at 21:27, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>
> On 4/9/20 8:39 AM, Naveen Krishna Chatradhi wrote:
> > This patch adds hwmon based amd_energy driver support for
> > family 17h processors from AMD.
> >
> > The driver provides following interface to the userspace
> > 1. Reports the per core and per socket energy consumption
> >    via sysfs entries created per core and per socket respectively.
> >     * per core energy consumed via "core_energy%d_input"
> >     * package/socket energy consumed via "sock_energy%d_input".
> > 2. Uses topology_max_packages() to get number of sockets.
> > 3. Uses num_present_cpus() to get the number of CPUs.
> > 4. Reports the energy units via energy_unit sysfs entry
> >
> > Cc: Guenter Roeck <linux@xxxxxxxxxxxx>
> > Signed-off-by: Naveen Krishna Chatradhi <nchatrad@xxxxxxx>
> > ---
> >  drivers/hwmon/Kconfig      |  10 ++
> >  drivers/hwmon/Makefile     |   1 +
> >  drivers/hwmon/amd_energy.c | 273 +++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 284 insertions(+)
> >  create mode 100644 drivers/hwmon/amd_energy.c
> >
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 05a30832c6ba..d83f1403b429 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -324,6 +324,16 @@ config SENSORS_FAM15H_POWER
> >         This driver can also be built as a module. If so, the module
> >         will be called fam15h_power.
> >
> > +config SENSORS_AMD_ENERGY
> > +     tristate "AMD RAPL MSR based Energy driver"
> > +     depends on X86
> > +     help
> > +       If you say yes here you get support for core and package energy
> > +       sensors, based on RAPL MSR for AMD family 17h and above CPUs.
> > +
> > +       This driver can also be built as a module. If so, the module
> > +       will be called as amd_energy.
> > +
> >  config SENSORS_APPLESMC
> >       tristate "Apple SMC (Motion sensor, light sensor, keyboard backlight)"
> >       depends on INPUT && X86
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index b0b9c8e57176..318f89dc7133 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -45,6 +45,7 @@ obj-$(CONFIG_SENSORS_ADT7411)       += adt7411.o
> >  obj-$(CONFIG_SENSORS_ADT7462)        += adt7462.o
> >  obj-$(CONFIG_SENSORS_ADT7470)        += adt7470.o
> >  obj-$(CONFIG_SENSORS_ADT7475)        += adt7475.o
> > +obj-$(CONFIG_SENSORS_AMD_ENERGY) += amd_energy.o
> >  obj-$(CONFIG_SENSORS_APPLESMC)       += applesmc.o
> >  obj-$(CONFIG_SENSORS_ARM_SCMI)       += scmi-hwmon.o
> >  obj-$(CONFIG_SENSORS_ARM_SCPI)       += scpi-hwmon.o
> > diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c
> > new file mode 100644
> > index 000000000000..ddb7073ae39b
> > --- /dev/null
> > +++ b/drivers/hwmon/amd_energy.c
> > @@ -0,0 +1,273 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +
> > +/*
> > + * Copyright (C) 2019 Advanced Micro Devices, Inc.
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +#include <linux/types.h>
> > +#include <linux/device.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/cpu.h>
> > +#include <linux/list.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/hwmon-sysfs.h>
> > +#include <linux/processor.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/cpumask.h>
>
> Alphabetic include file order, please.
I missed it, will correct it.
>
> > +
> > +#include <asm/cpu_device_id.h>
> > +
> > +#define DRVNAME      "amd_energy"
> > +
> > +#define ENERGY_PWR_UNIT_MSR  0xC0010299
> > +#define ENERGY_CORE_MSR      0xC001029A
> > +#define ENERGY_PCK_MSR               0xC001029B
> > +
> > +#define AMD_TIME_UNIT_MASK   0xF0000
> > +#define AMD_ENERGY_UNIT_MASK 0x01F00
> > +#define AMD_POWER_UNIT_MASK  0x0000F
> > +
> > +#define ENERGY_STATUS_MASK   0xffffffff
> > +
> > +#define AMD_FAM_17           0x17 /* ZP, SSP */
> > +
> > +/* Useful macros */
> > +#define AMD_CPU_FAM_ANY(_family, _model)     \
> > +{                                            \
> > +     .vendor         = X86_VENDOR_AMD,       \
> > +     .family         = _family,              \
> > +     .model          = _model,               \
> > +     .feature        = X86_FEATURE_ANY,      \
> > +}
> > +
> > +#define AMD_CPU_FAM(_model)                  \
> > +     AMD_CPU_FAM_ANY(AMD_FAM_##_model, X86_MODEL_ANY)
> > +
> > +static struct device_attribute units_attr;
> > +
> > +struct sensor_data {
> > +     unsigned int scale;
> > +     union {
> > +             unsigned int cpu_nr;
> > +             unsigned int sock_nr;
> > +     };
> > +     struct device_attribute dev_attr_input;
> > +     char input[32];
> > +};
> > +
> > +struct amd_energy_sensors {
> > +     struct sensor_data *data;
> > +     struct attribute **attrs;
> > +     struct attribute_group group;
> > +     const struct attribute_group *groups[1];
>
> Even if acceptable, this would be wrong. The list of groups
> ends with an empty entry, meaning this array would have to have
> at least two entries (one for the terminator).
Will correct it.
>
> > +};
> > +
> > +static ssize_t amd_units_u64_show(struct device *dev,
> > +     struct device_attribute *dev_attr, char *buffer)
> > +{
> > +     uint64_t rapl_units;
> > +     uint64_t energy_unit;
> > +     int ret = 0;
> > +
> > +     ret = rdmsrl_safe(ENERGY_PWR_UNIT_MSR, &rapl_units);
> > +     if (ret)
> > +             return -EAGAIN;
> > +
> > +     energy_unit = (rapl_units & AMD_ENERGY_UNIT_MASK) >> 8;
> > +
> > +     return snprintf(buffer, 24, "%llu\n", energy_unit);
> > +}
> > +
> > +static ssize_t amd_core_u64_show(struct device *dev,
> > +             struct device_attribute *dev_attr, char *buffer)
> > +{
> > +     unsigned long long value;
> > +     struct sensor_data *sensor;
> > +     int ret = 0;
> > +
> > +     sensor = container_of(dev_attr, struct sensor_data, dev_attr_input);
> > +     ret = rdmsrl_safe_on_cpu(sensor->cpu_nr, ENERGY_CORE_MSR, &value);
> > +     if (ret)
> > +             return -EAGAIN;
> > +
> > +     return snprintf(buffer, 24, "%llu\n", value);
>
> It seems to me this reports raw values. This is unacceptable. Values need
> to be scaled to match the ABI. Energy is reported in microJoule.
I was of the opinion that driver provides the raw values and user-space (can use
float/double)  will be able to scale the value, which would involve
calculation of
"1/2^ESU * RAW value"

>
> > +}
> > +
> > +static ssize_t amd_sock_u64_show(struct device *dev,
> > +             struct device_attribute *dev_attr, char *buffer)
> > +{
> > +     unsigned long long value;
> > +     struct sensor_data *sensor;
> > +     int ret = 0;
> > +     int cpu, cpu_nr;
> > +
> > +     sensor = container_of(dev_attr, struct sensor_data, dev_attr_input);
> > +
> > +     for_each_possible_cpu(cpu) {
> > +             struct cpuinfo_x86 *c = &cpu_data(cpu);
> > +
> > +             if (c->initialized && c->logical_die_id == sensor->sock_nr) {
> > +                     cpu_nr = cpu;
> > +                     break;
> > +             }
> > +             cpu_nr = 0;
> > +     }
> > +
> > +     ret = rdmsrl_safe_on_cpu(cpu_nr, ENERGY_PCK_MSR, &value);
> > +     if (ret)
> > +             return -EAGAIN;
> > +
> > +     return snprintf(buffer, 24, "%llu\n", value);
> > +}
> > +
> > +static int amd_energy_probe(struct platform_device *pdev)
> > +{
> > +     struct amd_energy_sensors *amd_sensors;
> > +     struct device *hwdev, *dev = &pdev->dev;
> > +     int nr_cpus, nr_socks, idx = 0;
> > +
> > +     nr_cpus = num_present_cpus();
> > +     nr_socks = topology_max_packages();
> > +
> > +     amd_sensors = devm_kzalloc(dev, sizeof(*amd_sensors), GFP_KERNEL);
> > +     if (!amd_sensors)
> > +             return -ENOMEM;
> > +
> > +     amd_sensors->data = devm_kcalloc(dev, nr_cpus + nr_socks,
> > +                             sizeof(*amd_sensors->data), GFP_KERNEL);
> > +     if (!amd_sensors->data)
> > +             return -ENOMEM;
> > +
> > +     amd_sensors->attrs = devm_kcalloc(dev, nr_cpus + nr_socks,
> > +                             sizeof(*amd_sensors->attrs), GFP_KERNEL);
> > +     if (!amd_sensors->attrs)
> > +             return -ENOMEM;
> > +
> > +     /* populate attributes for number of cpus. */
> > +     for (idx = 0; idx < ; idx++) {
> > +             struct sensor_data *sensor = &amd_sensors->data[idx];
> > +
> > +             snprintf(sensor->input, sizeof(sensor->input),
> > +                             "core_energy%d_input", idx);
> > +
>
> This is unacceptable. Please use standard attributes (energyX_input).
> If you want to report what the sensor is for, use labels.
Got it.
>
> > +             sensor->dev_attr_input.attr.mode = 0444;
> > +             sensor->dev_attr_input.attr.name = sensor->input;
> > +             sensor->dev_attr_input.show = amd_core_u64_show;
> > +
> > +             sensor->cpu_nr = idx;
> > +             amd_sensors->attrs[idx] = &sensor->dev_attr_input.attr;
> > +
> > +             sysfs_attr_init(amd_sensors->attrs[idx]);
> > +     }
> > +
> > +     /* populate attributes for number of sockets. */
> > +     for (idx = 0; idx < nr_socks; idx++) {
> > +             struct sensor_data *sensor =
> > +                     &amd_sensors->data[nr_cpus + idx];
> > +
> > +             snprintf(sensor->input,
> > +                     sizeof(sensor->input), "sock_energy%d_input", idx);
> > +
> > +             sensor->dev_attr_input.attr.mode = 0444;
> > +             sensor->dev_attr_input.attr.name = sensor->input;
> > +             sensor->dev_attr_input.show = amd_sock_u64_show;
> > +
> > +             sensor->sock_nr = idx;
> > +             amd_sensors->attrs[nr_cpus + idx] =
> > +                     &sensor->dev_attr_input.attr;
> > +
> > +             sysfs_attr_init(amd_sensors->attrs[nr_cpus + idx]);
> > +     }
>
> This all makes me wonder what is reported for inactive/disabled CPUs.
>
> > +
> > +     amd_sensors->group.attrs = amd_sensors->attrs;
> > +     amd_sensors->groups[0] = &amd_sensors->group;
> > +
> > +     platform_set_drvdata(pdev, amd_sensors);
> > +
> > +     hwdev = devm_hwmon_device_register_with_groups(dev,
> > +                     "amd_energy", amd_sensors, amd_sensors->groups);
>
> Please rework the driver to use the devm_hwmon_device_register_with_info() API.
>
> > +     if (!hwdev)
> > +             return PTR_ERR_OR_ZERO(hwdev);
> > +
> > +     /* populate attributes for energy units */
> > +     units_attr.attr.name = "energy_units";
> > +     units_attr.attr.mode = 0444;
> > +     units_attr.show = amd_units_u64_show;
> > +
> > +     return sysfs_create_file(&hwdev->kobj, &units_attr.attr);
>
> This is irrelevant for this driver. Units are fixed. Besides, registering
> sysfs attributes after driver registration is racy and won't be accepted.
> Non-standard attributes can be added with the groups argument of
> devm_hwmon_device_register_with_info() (not that I see any here).
I will try to change the driver around using
devm_hwmon_device_register_with_info()

>
> > +}
> > +
> > +static int amd_energy_remove(struct platform_device *pdev)
> > +{
> > +     sysfs_remove_file(&pdev->dev.kobj, &units_attr.attr);
> > +     return 0;
> > +}
> > +
> > +static const struct platform_device_id amd_energy_ids[] = {
> > +     { .name = DRVNAME, },
> > +     {}
> > +};
> > +MODULE_DEVICE_TABLE(platform, amd_energy_ids);
> > +
> > +static struct platform_driver amd_energy_driver = {
> > +     .probe = amd_energy_probe,
> > +     .remove = amd_energy_remove,
> > +     .id_table = amd_energy_ids,
> > +     .driver = {
> > +             .name = DRVNAME,
> > +     },
> > +};
> > +
> > +static struct platform_device *amd_energy_platdev;
> > +
> > +static const struct x86_cpu_id cpu_ids[] __initconst = {
> > +     AMD_CPU_FAM(17),
> > +     {}
> > +};
> > +MODULE_DEVICE_TABLE(x86cpu, cpu_ids);
> > +
> > +static int __init amd_energy_init(void)
> > +{
> > +     const struct x86_cpu_id *id;
> > +     int ret;
> > +
> > +     id = x86_match_cpu(cpu_ids);
> > +     if (!id) {
> > +             pr_err("driver does not support CPU family %d model %d\n",
> > +                     boot_cpu_data.x86, boot_cpu_data.x86_model);
> > +
>
> This is not an error and thus does not warrant an error message.
Ok
>
> > +             return -ENODEV;
> > +     }
> > +
> > +     ret = platform_driver_register(&amd_energy_driver);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     amd_energy_platdev = platform_device_alloc(DRVNAME, 0);
> > +     if (!amd_energy_platdev)
> > +             return -ENOMEM;
> > +
> > +     ret = platform_device_add(amd_energy_platdev);
> > +     if (ret) {
> > +             platform_device_unregister(amd_energy_platdev);
> > +             return ret;
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> > +static void __exit amd_energy_exit(void)
> > +{
> > +     platform_device_unregister(amd_energy_platdev);
> > +     platform_driver_unregister(&amd_energy_driver);
> > +}
> > +
> > +module_init(amd_energy_init);
> > +module_exit(amd_energy_exit);
> > +
> > +MODULE_DESCRIPTION("Driver for AMD Energy reporting from RAPL MSR via HWMON interface");
> > +MODULE_AUTHOR("Naveen Krishna Chatradhi <nchatrad@xxxxxxx>");
> > +MODULE_LICENSE("GPL");
> >
>


-- 
Shine bright,
Naveen



[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux