Subject: Re: [PATCH v2] powerpc/powernv: hwmon driver for power values, fan rpm and temperature

From: Guenter Roeck <[email-masked]>
Date: Wed, 28 May 2014 00:23:46 -0700

On 05/19/2014 07:26 AM, Neelesh Gupta wrote:
> This patch adds basic kernel enablement for reading power values, fan
> speed rpm and temperature values on powernv platforms which will
> be exported to user space through sysfs interface.
>
> Test results:
> -------------
> [root@tul163p1 ~]# sensors
> ibmpowernv-isa-0000
> Adapter: ISA adapter
> fan1:        5487 RPM  (min =    0 RPM)
> fan2:        5152 RPM  (min =    0 RPM)
> fan3:        5590 RPM  (min =    0 RPM)
> fan4:        4963 RPM  (min =    0 RPM)
> fan5:           0 RPM  (min =    0 RPM)
> fan6:           0 RPM  (min =    0 RPM)
> fan7:        7488 RPM  (min =    0 RPM)
> fan8:        7944 RPM  (min =    0 RPM)
> temp1:        +39.0°C  (high =  +0.0°C)
> power1:      192.00 W
>
> [root@tul163p1 ~]# ls /sys/devices/platform/
> alarmtimer  ibmpowernv.0  rtc-generic  serial8250  uevent
> [root@tul163p1 ~]# ls /sys/devices/platform/ibmpowernv.0/
> driver/    hwmon/     modalias   subsystem/ uevent
> [root@tul163p1 ~]# ls /sys/devices/platform/ibmpowernv.0/hwmon/hwmon0/
> device	    fan2_min	fan4_min    fan6_min	fan8_min   power1_input
> fan1_fault  fan3_fault	fan5_fault  fan7_fault	in1_fault  subsystem
> fan1_input  fan3_input	fan5_input  fan7_input	in2_fault  temp1_input
> fan1_min    fan3_min	fan5_min    fan7_min	in3_fault  temp1_max
> fan2_fault  fan4_fault	fan6_fault  fan8_fault	in4_fault  uevent
> fan2_input  fan4_input	fan6_input  fan8_input	name
> [root@tul163p1 ~]#
> [root@tul163p1 ~]# ls /sys/class/hwmon/hwmon0/
> device	    fan2_min	fan4_min    fan6_min	fan8_min   power1_input
> fan1_fault  fan3_fault	fan5_fault  fan7_fault	in1_fault  subsystem
> fan1_input  fan3_input	fan5_input  fan7_input	in2_fault  temp1_input
> fan1_min    fan3_min	fan5_min    fan7_min	in3_fault  temp1_max
> fan2_fault  fan4_fault	fan6_fault  fan8_fault	in4_fault  uevent
> fan2_input  fan4_input	fan6_input  fan8_input	name
> [root@tul163p1 ~]#
>

The inX_fault attributes don't really make much sense. _fault attributes without
_input attributes don't have any value and are, as you noticed, not displayed
with the sensors command. Is this a problem in teh devicetree data or do you
really have voltage faults without voltages ?

> Signed-off-by: Neelesh Gupta <[email-masked]>
> ---
>
Checkpatch says:

total: 8 errors, 11 warnings, 389 lines checked

NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch or
       scripts/cleanfile

which should really not happen at this point.

Please make sure you follow CodingStyle. checkpatch --strict points to a number
of additional violations.

More comments inline.

> Changes in v2
> =============
> - Generic use of devm_* functions in hwmon like using devm_kzalloc() for dynamic
>    memory request, avoiding the need to explicit free of memory.
>    Adding 'struct attribute_group' as member of platform data structure to be
>    populated and then passed to devm_hwmon_device_register_with_groups().
>
>    Note: Having an array of pointers of 'attribute_group' and each group
>    corresponds to 'enum sensors' type. Not completely sure, if it's ideal or
>    could have just one group populated with attributes of sensor types?
>
Your call, really; whatever is easier for you. I won't dictate one or the other.

Question though is what happens if one group is not populated. If I understand
the code correctly this will result in the remaining groups to be 'dropped',
ie not displayed at all.

> - 'ibmpowernv' is not hot-pluggable device so moving 'platform_driver' callback
>    function (probe) as part of __init code.
> - Fixed issues related to coding style.
> - Other general comments in v1.
>
>   drivers/hwmon/Kconfig      |    8 +
>   drivers/hwmon/Makefile     |    1
>   drivers/hwmon/ibmpowernv.c |  368 ++++++++++++++++++++++++++++++++++++++++++++

You'll also need Documentation/hwmon/ibmpowernv and
Documentation/devicetree/bindings/hwmon/ibmpowernv.

The latter will need to get an Ack from the devicetree maintainers.

>   3 files changed, 377 insertions(+)
>   create mode 100644 drivers/hwmon/ibmpowernv.c
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index bc196f4..3e308fa 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -554,6 +554,14 @@ config SENSORS_IBMPEX
>   	  This driver can also be built as a module.  If so, the module
>   	  will be called ibmpex.
>
> +config SENSORS_IBMPOWERNV
> +	tristate "IBM POWERNV platform sensors"
> +	depends on PPC_POWERNV
> +	default y
> +	help
> +	  If you say yes here you get support for the temperature/fan/power
> +	  sensors on your platform.
> +
>   config SENSORS_IIO_HWMON
>   	tristate "Hwmon driver that uses channels specified via iio maps"
>   	depends on IIO
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index c48f987..199c401 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -71,6 +71,7 @@ obj-$(CONFIG_SENSORS_ULTRA45)	+= ultra45_env.o
>   obj-$(CONFIG_SENSORS_I5K_AMB)	+= i5k_amb.o
>   obj-$(CONFIG_SENSORS_IBMAEM)	+= ibmaem.o
>   obj-$(CONFIG_SENSORS_IBMPEX)	+= ibmpex.o
> +obj-$(CONFIG_SENSORS_IBMPOWERNV)+= ibmpowernv.o
>   obj-$(CONFIG_SENSORS_IIO_HWMON) += iio_hwmon.o
>   obj-$(CONFIG_SENSORS_INA209)	+= ina209.o
>   obj-$(CONFIG_SENSORS_INA2XX)	+= ina2xx.o
> diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
> new file mode 100644
> index 0000000..afce620
> --- /dev/null
> +++ b/drivers/hwmon/ibmpowernv.c
> @@ -0,0 +1,368 @@
> +/*
> + * IBM PowerNV platform sensors for temperature/fan/power
> + * Copyright (C) 2014 IBM
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/of.h>
> +#include <linux/slab.h>
> +
> +#include <linux/platform_device.h>
> +#include <asm/opal.h>
> +#include <linux/err.h>
> +
> +#define DRVNAME		"ibmpowernv"
> +#define MAX_ATTR_LEN	32
> +
> +/* Sensor suffix name from DT */
> +#define DT_FAULT_ATTR_SUFFIX		"faulted"
> +#define DT_DATA_ATTR_SUFFIX		"data"
> +#define DT_THRESHOLD_ATTR_SUFFIX	"thrs"
> +
> +/*
> + * Enumerates all the types of sensors in the POWERNV platform and does index
> + * into 'struct sensor_group'
> + */
> +enum sensors {
> +	FAN,
> +	AMBIENT_TEMP,
> +	POWER_SUPPLY,
> +	POWER_INPUT,
> +	MAX_SENSOR_TYPE,
> +};
> +
> +static struct sensor_group {
> +	const char *name;
> +	const char *compatible;
> +	struct attribute_group group;
> +	u32 attr_count;
> +} sensor_groups[] = {
> +	{"fan", "ibm,opal-sensor-cooling-fan", {0}, 0},
> +	{"temp", "ibm,opal-sensor-amb-temp", {0}, 0},
> +	{"in", "ibm,opal-sensor-power-supply", {0}, 0},
> +	{"power", "ibm,opal-sensor-power", {0}, 0}
> +};
> +
The '0' initializations should not be necessary.

> +struct sensor_data {
> +	u32 id; /* An opaque id of the firmware for each sensor */
> +	enum sensors type;
> +	char name[MAX_ATTR_LEN];
> +	struct device_attribute dev_attr;
> +};
> +
> +struct platform_data {
> +	const struct attribute_group *attr_groups[MAX_SENSOR_TYPE + 1];
> +	u32 sensors_count; /* Total count of sensors from each group */
> +};
> +
> +/* Platform device representing all the ibmpowernv sensors */
> +static struct platform_device *pdevice;
> +
> +static ssize_t show_sensor(struct device *dev, struct device_attribute *devattr,
> +			   char *buf)
> +{
> +	struct sensor_data *sdata = container_of(devattr, struct sensor_data,
> +						 dev_attr);
> +	ssize_t ret;
> +	u32 x;
> +
> +	ret = opal_get_sensor_data(sdata->id, &x);
> +	if (ret) {
> +		pr_err("%s: Failed to get opal sensor data\n", __func__);
> +		return ret;
> +	}
> +
> +	/* Convert temperature to milli-degrees */
> +	if (sdata->type == AMBIENT_TEMP)
> +		x *= 1000;
> +	/* Convert power to micro-watts */
> +	else if (sdata->type == POWER_INPUT)
> +		x *= 1000000;
> +
> +	return sprintf(buf, "%d\n", x);

x is unsigned, so %u.

> +}
> +
> +static void __init get_sensor_index_attr(const char *name, u32 *index, char *attr)
> +{
> +	char *hash_pos = strchr(name, '#');
> +	char *dash_pos;
> +	u32 copy_len;
> +	char buf[8];
> +
> +	memset(buf, 0, sizeof(buf));
> +	*index = 0;
> +	*attr = '\0';
> +
> +	if (hash_pos) {
> +		dash_pos = strchr(hash_pos, '-');
> +		if (dash_pos) {
> +			copy_len = dash_pos - hash_pos - 1;
> +			if (copy_len < sizeof(buf)) {
> +				strncpy(buf, hash_pos + 1, copy_len);
> +				sscanf(buf, "%d", index);

What if sscanf fails ? Might be an interesting exercise to try and create
multiple sensors with index 0 (or, for that matter, with the same index value).
Do you have any protection against bad input data ? Guess not; did you test
what happens if you pass bad data to the driver (such as duplicate sensor
entries) ?

> +			}
> +
> +			strncpy(attr, dash_pos + 1, MAX_ATTR_LEN);
> +		}
> +	}
> +}
> +
> +/*
> + * This function translates the DT node name into the 'hwmon' attribute name.
> + * IBMPOWERNV device node appear like cooling-fan#2-data, amb-temp#1-thrs etc.
> + * which need to be mapped as fan2_input, temp1_max respectively before
> + * populating them inside hwmon device class..
> + */
> +static int __init create_hwmon_attr_name(enum sensors type, const char *node_name,
> +				  char *hwmon_attr_name)
> +{
> +	char attr_suffix[MAX_ATTR_LEN];
> +	char *attr_name;
> +	u32 index;
> +
> +	get_sensor_index_attr(node_name, &index, attr_suffix);
> +	if (!index || !strlen(attr_suffix)) {
> +		pr_info("%s: Sensor device node name is invalid, name: %s\n",
> +				__func__, node_name);
> +		return -EINVAL;
> +	}
> +
> +	if (!strcmp(attr_suffix, DT_FAULT_ATTR_SUFFIX))
> +		attr_name = "fault";
> +	else if(!strcmp(attr_suffix, DT_DATA_ATTR_SUFFIX))
> +		attr_name = "input";
> +	else if (!strcmp(attr_suffix, DT_THRESHOLD_ATTR_SUFFIX)) {
> +		if (type == AMBIENT_TEMP)
> +			attr_name = "max";
> +		else if (type == FAN)
> +			attr_name = "min";
> +		else
> +			return -ENOENT;
> +	} else
> +		return -ENOENT;
> +
> +	snprintf(hwmon_attr_name, MAX_ATTR_LEN, "%s%d_%s",
> +			sensor_groups[type].name, index, attr_name);
> +	return 0;
> +}
> +
> +static int __init populate_attr_groups(struct platform_device *pdev)
> +{
> +	struct platform_data *pdata = platform_get_drvdata(pdev);
> +	const struct attribute_group **pgroups = pdata->attr_groups;
> +	struct device_node *opal, *np;
> +	enum sensors type;
> +	int err = 0;
> +
> +	opal = of_find_node_by_path("/ibm,opal/sensors");
> +        if (!opal) {

An obvious whitespace error here.

> +		pr_err("%s: Opal 'sensors' node not found\n", __func__);
> +		return -ENODEV;
> +        }
> +
> +	for_each_child_of_node(opal, np) {
> +		if (np->name == NULL)
> +			continue;
> +
> +		for (type = 0; type < MAX_SENSOR_TYPE; type++)
> +			if (of_device_is_compatible(np,
> +				sensor_groups[type].compatible)) {
> +				sensor_groups[type].attr_count++;
> +				break;
> +			}
> +	}

You should be able to do
	of_node_put(opal);

here. Then you can return immediately on error below.

> +
> +	for (type = 0; type < MAX_SENSOR_TYPE; type++) {
> +		if (!sensor_groups[type].attr_count)
> +			continue;
> +
> +		sensor_groups[type].group.attrs = devm_kzalloc(&pdev->dev,
> +					sizeof(struct attribute*) *
> +					(sensor_groups[type].attr_count + 1),
> +					GFP_KERNEL);
> +		if (!sensor_groups[type].group.attrs) {
> +			pr_err("%s: Failed to allocate memory for attribute"
> +					"array\n", __func__);

devm_kzalloc() already dumps an error message. Same for all other memory error messages.

> +			err = -ENOMEM;
> +			goto exit_put_node;
> +		}
> +
> +		pgroups[type] = &sensor_groups[type].group;
> +		pdata->sensors_count += sensor_groups[type].attr_count;
> +		sensor_groups[type].attr_count = 0;
> +	}
> +
> +exit_put_node:
> +	of_node_put(opal);
> +	return err;
> +}
> +
> +/*
> + * Iterate through the device tree for each child of sensor node, create
> + * a sysfs attribute file, the file is named by translating the DT node name
> + * to the name required by the higher 'hwmon' driver like fan1_input, temp1_max
> + * etc..
> + */
> +static int __init create_device_attrs(struct platform_device *pdev)
> +{
> +	struct platform_data *pdata = platform_get_drvdata(pdev);
> +	const struct attribute_group **pgroups = pdata->attr_groups;
> +	struct device_node *opal, *np;
> +	struct sensor_data *sdata;
> +	const u32 *sensor_id;
> +	enum sensors type;
> +	u32 count = 0;
> +	int err = 0;
> +
> +	opal = of_find_node_by_path("/ibm,opal/sensors");
> +        if (!opal) {
> +		pr_err("%s: Opal 'sensors' node not found\n", __func__);
> +		return -ENODEV;
> +        }
> +
> +	sdata = devm_kzalloc(&pdev->dev, (pdata->sensors_count) *
> +			     sizeof(*sdata), GFP_KERNEL);
> +	if (!sdata) {
> +		pr_err("%s: Failed to allocate memory for the sensor_data",
> +				__func__);
> +		err = -ENOMEM;
> +		goto exit_put_node;
> +	}
> +
> +	for_each_child_of_node(opal, np) {
> +                if (np->name == NULL)
> +                        continue;
> +
> +		for (type = 0; type < MAX_SENSOR_TYPE; type++)
> +			if (of_device_is_compatible(np,
> +					sensor_groups[type].compatible))
> +				break;
> +
> +		if (type == MAX_SENSOR_TYPE)
> +			continue;
> +
> +		sensor_id = of_get_property(np, "sensor-id", NULL);
> +		if (!sensor_id) {
> +			pr_info("%s: %s doesn't have sensor-id\n", __func__,
> +				np->name);
> +			continue;
> +		}
> +
Consider using of_property_read_u32().

> +		sdata[count].id = *sensor_id;
> +		sdata[count].type = type;
> +		err = create_hwmon_attr_name(type, np->name, sdata[count].name);
> +		if (err)
> +			goto exit_put_node;
> +
> +		sysfs_attr_init(&sdata[count].dev_attr.attr);
> +		sdata[count].dev_attr.attr.name = sdata[count].name;
> +		sdata[count].dev_attr.attr.mode = S_IRUGO;
> +		sdata[count].dev_attr.show = show_sensor;
> +
> +		pgroups[type]->attrs[sensor_groups[type].attr_count++] =
> +				&sdata[count++].dev_attr.attr;
> +	}
> +
> +exit_put_node:
> +	of_node_put(opal);
> +	return err;
> +}
> +
> +static int __init ibmpowernv_probe(struct platform_device *pdev)
> +{
> +	struct platform_data *pdata;
> +	struct device *hwmon_dev;
> +	int err;
> +
> +	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, pdata);
> +	pdata->sensors_count = 0;
> +	err = populate_attr_groups(pdev);
> +	if (err)
> +		return err;
> +
> +	/* Create sysfs attribute file for each sensor found in the DT */

Attribute data, not file

> +	err = create_device_attrs(pdev);
> +	if (err)
> +		return err;
> +
> +	/* Finally, register with hwmon */
> +	hwmon_dev = devm_hwmon_device_register_with_groups(&pdev->dev, DRVNAME,
> +							   pdata,
> +							   pdata->attr_groups);
> +
> +	return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static struct platform_driver ibmpowernv_driver = {
> +	.driver = {
> +		.owner = THIS_MODULE,
> +		.name = DRVNAME,
> +	},
> +};
> +
> +static int __init ibmpowernv_init(void)
> +{
> +	int err;
> +
> +	pdevice = platform_device_alloc(DRVNAME, 0);
> +	if (!pdevice) {
> +		pr_err("%s: Device allocation failed\n", __func__);
> +		err = -ENOMEM;
> +		goto exit;
> +	}
> +
> +	err = platform_device_add(pdevice);
> +	if (err) {
> +		pr_err("%s: Device addition failed (%d)\n", __func__, err);
> +		goto exit_device_put;
> +	}
> +
> +	err = platform_driver_probe(&ibmpowernv_driver, ibmpowernv_probe);
> +	if (err) {
> +		pr_err("%s: Platfrom driver probe failed\n", __func__);
> +		goto exit_device_del;
> +	}
> +
> +	return 0;
> +
> +exit_device_del:
> +	platform_device_del(pdevice);
> +exit_device_put:
> +	platform_device_put(pdevice);
> +exit:
> +	return err;
> +}
> +
> +static void __exit ibmpowernv_exit(void)
> +{
> +	platform_driver_unregister(&ibmpowernv_driver);
> +	platform_device_unregister(pdevice);
> +}
> +
> +MODULE_AUTHOR("Neelesh Gupta <[email-masked]>");
> +MODULE_DESCRIPTION("IBM POWERNV platform sensors");
> +MODULE_LICENSE("GPL");
> +
> +module_init(ibmpowernv_init);
> +module_exit(ibmpowernv_exit);
>
>
>