LinuxLists.cc - [PATCH] v1 of IBM power meter driver

2007-08-27 21:25:11

Subject: [PATCH] v1 of IBM power meter driver

Hi everyone,

Attached is a driver to export sensor readings from power meters that
are found in several IBM x86 systems. At the moment, the hwmon sysfs
documentation doesn't mention any naming conventions for sensors that
measure Watts, so I am proposing that they be called "powerX_input" in a
fashion similar to temperature/rpm/current sensors. If that is
agreeable to everyone, I'll post a follow-up patch to amend the
documentation.

The patch should apply against 2.6.23-rc3 and has been tested on the
x3550, x3650, x3655, x3755 and HS20 blades that support it. As far
as I know, those are the only systems in existence that have this
interface.
---
ibm_pex: Driver to export IBM PowerExecutive power meter sensors.

Signed-off-by: Darrick J. Wong <[email protected]>
---

drivers/hwmon/Kconfig | 12 +
drivers/hwmon/Makefile | 1
drivers/hwmon/ibmpex.c | 615 ++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 628 insertions(+), 0 deletions(-)

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 555f470..41ffa2e 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -275,6 +275,18 @@ config SENSORS_CORETEMP
sensor inside your CPU. Supported all are all known variants
of Intel Core family.

+config SENSORS_IBMPEX
+ tristate "IBM PowerExecutive temperature/power sensors"
+ depends on IPMI_SI
+ help
+ If you say yes here you get support for the temperature and
+ power sensors in various IBM System X servers that support
+ PowerExecutive. So far this includes the x3550, x3650, x3655,
+ x3755, and certain HS20 blades.
+
+ This driver can also be built as a module. If so, the module
+ will be called ibmpex.
+
config SENSORS_IT87
tristate "ITE IT87xx and compatibles"
select HWMON_VID
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index a133981..31da6fe 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -35,6 +35,7 @@ obj-$(CONFIG_SENSORS_FSCPOS) += fscpos.o
obj-$(CONFIG_SENSORS_GL518SM) += gl518sm.o
obj-$(CONFIG_SENSORS_GL520SM) += gl520sm.o
obj-$(CONFIG_SENSORS_HDAPS) += hdaps.o
+obj-$(CONFIG_SENSORS_IBMPEX) += ibmpex.o
obj-$(CONFIG_SENSORS_IT87) += it87.o
obj-$(CONFIG_SENSORS_K8TEMP) += k8temp.o
obj-$(CONFIG_SENSORS_LM63) += lm63.o
diff --git a/drivers/hwmon/ibmpex.c b/drivers/hwmon/ibmpex.c
new file mode 100644
index 0000000..15ae9ec
--- /dev/null
+++ b/drivers/hwmon/ibmpex.c
@@ -0,0 +1,615 @@
+/*
+ * A hwmon driver for the IBM PowerExecutive temperature/power sensors
+ * Copyright (C) 2007 IBM
+ *
+ * Author: Darrick J. Wong <[email protected]>
+ *
+ * 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; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#include <linux/ipmi.h>
+#include <linux/module.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/mutex.h>
+
+#define REFRESH_INTERVAL (5 * HZ)
+#define DRVNAME "ibmpex"
+
+static void ibmpex_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data);
+static void ibmpex_register_bmc(int iface, struct device *dev);
+static void ibmpex_bmc_gone(int iface);
+
+struct ibmpex_sensor_data {
+ int in_use;
+ s16 value;
+ s16 max;
+ s16 min;
+ int divisor;
+
+ struct sensor_device_attribute attr_value;
+ struct sensor_device_attribute attr_max;
+ struct sensor_device_attribute attr_min;
+};
+
+struct ibmpex_bmc_data {
+ struct list_head list;
+ struct class_device *class_dev;
+ struct device *bmc_device;
+ struct mutex lock;
+ char valid;
+ unsigned long last_updated; /* In jiffies */
+
+ struct ipmi_addr address;
+ struct completion read_complete;
+ ipmi_user_t user;
+ int interface;
+
+ struct kernel_ipmi_msg tx_message;
+ unsigned char tx_msg_data[IPMI_MAX_MSG_LENGTH];
+ long tx_msgid;
+
+ unsigned char rx_msg_data[IPMI_MAX_MSG_LENGTH];
+ unsigned long rx_msg_len;
+ unsigned char rx_result;
+ int rx_recv_type;
+
+ unsigned char sensor_major;
+ unsigned char sensor_minor;
+
+ unsigned char num_sensors;
+ struct ibmpex_sensor_data *sensors;
+};
+
+struct ibmpex_driver_data {
+ struct list_head bmc_data;
+ struct ipmi_smi_watcher bmc_events;
+ struct ipmi_user_hndl ipmi_hndlrs;
+};
+
+static struct ibmpex_driver_data driver_data = {
+ .bmc_data = LIST_HEAD_INIT(driver_data.bmc_data),
+ .bmc_events = {
+ .owner = THIS_MODULE,
+ .new_smi = ibmpex_register_bmc,
+ .smi_gone = ibmpex_bmc_gone,
+ },
+ .ipmi_hndlrs = {
+ .ipmi_recv_hndl = ibmpex_msg_handler,
+ },
+};
+
+static int ibmpex_send_message(struct ibmpex_bmc_data *data)
+{
+ int err;
+
+ err = ipmi_validate_addr(&data->address, sizeof(data->address));
+ if (err)
+ goto out;
+
+ data->tx_msgid++;
+ err = ipmi_request_settime(data->user, &data->address, data->tx_msgid,
+ &data->tx_message, data, 0, 0, 0);
+ if (err)
+ goto out1;
+
+ return 0;
+out1:
+ printk(KERN_ERR "%s: request_settime=%x\n", __FUNCTION__, err);
+ return err;
+out:
+ printk(KERN_ERR "%s: validate_addr=%x\n", __FUNCTION__, err);
+ return err;
+}
+
+static int ibmpex_ver_check(struct ibmpex_bmc_data *data)
+{
+ data->tx_msg_data[0] = 0x1;
+ data->tx_message.data_len = 1;
+ ibmpex_send_message(data);
+
+ wait_for_completion(&data->read_complete);
+
+ if (data->rx_result || data->rx_msg_len != 6)
+ return -ENOENT;
+
+ data->sensor_major = data->rx_msg_data[0];
+ data->sensor_minor = data->rx_msg_data[1];
+
+ printk(KERN_INFO DRVNAME ": Found BMC with sensor interface "
+ "v%d.%d %d-%02d-%02d on interface %d\n",
+ data->sensor_major,
+ data->sensor_minor,
+ data->rx_msg_data[3] | ((u16)data->rx_msg_data[2] << 8),
+ data->rx_msg_data[4],
+ data->rx_msg_data[5],
+ data->interface);
+
+ return 0;
+}
+
+static int ibmpex_query_sensor_count(struct ibmpex_bmc_data *data)
+{
+ data->tx_msg_data[0] = 0x2;
+ data->tx_message.data_len = 1;
+ ibmpex_send_message(data);
+
+ wait_for_completion(&data->read_complete);
+
+ if (data->rx_result || data->rx_msg_len != 1)
+ return -ENOENT;
+
+ return data->rx_msg_data[0];
+}
+
+static int ibmpex_query_sensor_name(struct ibmpex_bmc_data *data, int sensor)
+{
+ data->tx_msg_data[0] = 0x3;
+ data->tx_msg_data[1] = sensor;
+ data->tx_message.data_len = 2;
+ ibmpex_send_message(data);
+
+ wait_for_completion(&data->read_complete);
+
+ if (data->rx_result || data->rx_msg_len < 1)
+ return -ENOENT;
+
+ return 0;
+}
+
+static int ibmpex_query_sensor_data(struct ibmpex_bmc_data *data, int sensor)
+{
+ data->tx_msg_data[0] = 0x6;
+ data->tx_msg_data[1] = sensor;
+ data->tx_message.data_len = 2;
+ ibmpex_send_message(data);
+
+ wait_for_completion(&data->read_complete);
+
+ if (data->rx_result || data->rx_msg_len < 26) {
+ printk(KERN_ERR "Error reading sensor %d, please check.\n",
+ sensor);
+ return -ENOENT;
+ }
+
+ return 0;
+}
+
+static void ibmpex_update_device(struct ibmpex_bmc_data *data)
+{
+ int i, err;
+
+ mutex_lock(&data->lock);
+ if (time_before(jiffies, data->last_updated + REFRESH_INTERVAL) &&
+ data->valid)
+ goto out;
+
+ for (i = 0; i < data->num_sensors; i++) {
+ if (!data->sensors[i].in_use)
+ continue;
+ err = ibmpex_query_sensor_data(data, i);
+ if (err)
+ continue;
+ data->sensors[i].value = ((u16)data->rx_msg_data[16] << 8) |
+ data->rx_msg_data[17];
+ data->sensors[i].min = ((u16)data->rx_msg_data[18] << 8) |
+ data->rx_msg_data[19];
+ data->sensors[i].max = ((u16)data->rx_msg_data[20] << 8) |
+ data->rx_msg_data[21];
+ }
+
+ data->last_updated = jiffies;
+ data->valid = 1;
+
+out:
+ mutex_unlock(&data->lock);
+}
+
+static struct ibmpex_bmc_data *get_bmc_data(int iface)
+{
+ struct ibmpex_bmc_data *p, *next;
+
+ list_for_each_entry_safe(p, next, &driver_data.bmc_data, list)
+ if (p->interface == iface)
+ return p;
+
+ return NULL;
+}
+
+static ssize_t show_name(struct device *dev, struct device_attribute *devattr,
+ char *buf)
+{
+ return sprintf(buf, "%s\n", DRVNAME);
+}
+static SENSOR_DEVICE_ATTR(name, S_IRUGO, show_name, NULL, 0);
+
+static ssize_t ibmpex_show_sensor(struct device *dev,
+ struct device_attribute *devattr,
+ char *buf)
+{
+ struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+ int iface = attr->index >> 8;
+ int sensor = attr->index & 0xFF;
+ struct ibmpex_bmc_data *data = get_bmc_data(iface);
+ int divisor = data->sensors[sensor].divisor;
+ ibmpex_update_device(data);
+
+ if (divisor == 1)
+ return sprintf(buf, "%d\n", data->sensors[sensor].value);
+
+ return sprintf(buf, "%d.%d\n",
+ data->sensors[sensor].value / divisor,
+ data->sensors[sensor].value % divisor);
+}
+
+static ssize_t ibmpex_show_max(struct device *dev,
+ struct device_attribute *devattr,
+ char *buf)
+{
+ struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+ int iface = attr->index >> 8;
+ int sensor = attr->index & 0xFF;
+ struct ibmpex_bmc_data *data = get_bmc_data(iface);
+ int divisor = data->sensors[sensor].divisor;
+ ibmpex_update_device(data);
+
+ if (divisor == 1)
+ return sprintf(buf, "%d\n", data->sensors[sensor].max);
+
+ return sprintf(buf, "%d.%d\n",
+ data->sensors[sensor].max / divisor,
+ data->sensors[sensor].max % divisor);
+}
+
+static ssize_t ibmpex_show_min(struct device *dev,
+ struct device_attribute *devattr,
+ char *buf)
+{
+ struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+ int iface = attr->index >> 8;
+ int sensor = attr->index & 0xFF;
+ struct ibmpex_bmc_data *data = get_bmc_data(iface);
+ int divisor = data->sensors[sensor].divisor;
+ ibmpex_update_device(data);
+
+ if (divisor == 1)
+ return sprintf(buf, "%d\n", data->sensors[sensor].min);
+
+ return sprintf(buf, "%d.%d\n",
+ data->sensors[sensor].min / divisor,
+ data->sensors[sensor].min % divisor);
+}
+
+static int is_power_sensor(const char *sensor_id, int len)
+{
+ if (len < 3)
+ return 0;
+
+ if (sensor_id[0] == 0x70 &&
+ sensor_id[1] == 0x77 &&
+ sensor_id[2] == 0x72)
+ return 1;
+ return 0;
+}
+
+static int is_temp_sensor(const char *sensor_id, int len)
+{
+ if (len < 3)
+ return 0;
+
+ if (sensor_id[0] == 0x74 &&
+ sensor_id[1] == 0x65 &&
+ sensor_id[2] == 0x6D)
+ return 1;
+ return 0;
+}
+
+static int power_sensor_divisor(const char *sensor_id, int len)
+{
+ int i;
+
+ for (i = 3; i < len - 1; i++)
+ if (sensor_id[i] == 0x41 &&
+ sensor_id[i + 1] == 0x43)
+ return 1;
+
+ return 10;
+}
+
+static int ibmpex_find_sensors(struct ibmpex_bmc_data *data)
+{
+ int i, err;
+ char *n, *sensor_type;
+ int sensor_counter;
+ int num_power = 0;
+ int num_temp = 0;
+
+ err = ibmpex_query_sensor_count(data);
+ if (err < 0)
+ return -ENOENT;
+ data->num_sensors = err;
+
+ data->sensors = kzalloc(data->num_sensors * sizeof(*data->sensors),
+ GFP_KERNEL);
+ if (!data->sensors)
+ return -ENOMEM;
+
+ for (i = 0; i < data->num_sensors; i++) {
+ err = ibmpex_query_sensor_name(data, i);
+ if (err)
+ continue;
+
+ if (is_power_sensor(data->rx_msg_data, data->rx_msg_len)) {
+ sensor_type = "power";
+ num_power++;
+ sensor_counter = num_power;
+ data->sensors[i].divisor =
+ power_sensor_divisor(data->rx_msg_data,
+ data->rx_msg_len);
+ } else if (is_temp_sensor(data->rx_msg_data,
+ data->rx_msg_len)) {
+ sensor_type = "temp";
+ num_temp++;
+ sensor_counter = num_temp;
+ data->sensors[i].divisor = 1;
+ } else
+ continue;
+
+ data->sensors[i].in_use = 1;
+
+ /* Create value attribute */
+ n = kmalloc(32, GFP_KERNEL);
+ if (!n)
+ goto exit_remove;
+ sprintf(n, "%s%d_input", sensor_type, sensor_counter);
+ data->sensors[i].attr_value.dev_attr.attr.name = n;
+ data->sensors[i].attr_value.dev_attr.attr.mode = S_IRUGO;
+ data->sensors[i].attr_value.dev_attr.show = ibmpex_show_sensor;
+ data->sensors[i].attr_value.index = (data->interface << 8) |
+ (i & 0xFF);
+
+ err = device_create_file(data->bmc_device,
+ &data->sensors[i].attr_value.dev_attr);
+ if (err) {
+ data->sensors[i].attr_value.dev_attr.attr.name = NULL;
+ kfree(n);
+ goto exit_remove;
+ }
+
+ /* Create max attribute */
+ n = kmalloc(32, GFP_KERNEL);
+ if (!n)
+ goto exit_remove;
+ sprintf(n, "%s%d_max_input", sensor_type, sensor_counter);
+ data->sensors[i].attr_max.dev_attr.attr.name = n;
+ data->sensors[i].attr_max.dev_attr.attr.mode = S_IRUGO;
+ data->sensors[i].attr_max.dev_attr.show = ibmpex_show_max;
+ data->sensors[i].attr_max.index = (data->interface << 8) |
+ (i & 0xFF);
+
+ err = device_create_file(data->bmc_device,
+ &data->sensors[i].attr_max.dev_attr);
+ if (err) {
+ data->sensors[i].attr_max.dev_attr.attr.name = NULL;
+ kfree(n);
+ goto exit_remove;
+ }
+
+ /* Create min attribute */
+ n = kmalloc(32, GFP_KERNEL);
+ if (!n)
+ goto exit_remove;
+ sprintf(n, "%s%d_min_input", sensor_type, sensor_counter);
+ data->sensors[i].attr_min.dev_attr.attr.name = n;
+ data->sensors[i].attr_min.dev_attr.attr.mode = S_IRUGO;
+ data->sensors[i].attr_min.dev_attr.show = ibmpex_show_min;
+ data->sensors[i].attr_min.index = (data->interface << 8) |
+ (i & 0xFF);
+
+ err = device_create_file(data->bmc_device,
+ &data->sensors[i].attr_min.dev_attr);
+ if (err) {
+ data->sensors[i].attr_min.dev_attr.attr.name = NULL;
+ kfree(n);
+ goto exit_remove;
+ }
+ }
+
+ err = device_create_file(data->bmc_device,
+ &sensor_dev_attr_name.dev_attr);
+ if (err)
+ goto exit_remove;
+
+ return 0;
+
+exit_remove:
+ for (i = 0; i < data->num_sensors; i++) {
+ if (data->sensors[i].attr_value.dev_attr.attr.name)
+ device_remove_file(data->bmc_device,
+ &data->sensors[i].attr_value.dev_attr);
+ kfree(data->sensors[i].attr_value.dev_attr.attr.name);
+ if (data->sensors[i].attr_max.dev_attr.attr.name)
+ device_remove_file(data->bmc_device,
+ &data->sensors[i].attr_max.dev_attr);
+ kfree(data->sensors[i].attr_max.dev_attr.attr.name);
+ if (data->sensors[i].attr_min.dev_attr.attr.name)
+ device_remove_file(data->bmc_device,
+ &data->sensors[i].attr_min.dev_attr);
+ kfree(data->sensors[i].attr_min.dev_attr.attr.name);
+ }
+
+ kfree(data->sensors);
+ return -ENOENT;
+}
+
+static void ibmpex_register_bmc(int iface, struct device *dev)
+{
+ struct ibmpex_bmc_data *data;
+ int err;
+
+ data = kzalloc(sizeof(*data), GFP_KERNEL);
+ if (!data) {
+ printk(KERN_ERR DRVNAME ": Insufficient memory for BMC "
+ "interface %d.\n", data->interface);
+ return;
+ }
+
+ data->address.addr_type = IPMI_SYSTEM_INTERFACE_ADDR_TYPE;
+ data->address.channel = IPMI_BMC_CHANNEL;
+ data->address.data[0] = 0;
+ data->interface = iface;
+ data->bmc_device = dev;
+
+ /* Create IPMI messaging interface user */
+ err = ipmi_create_user(data->interface, &driver_data.ipmi_hndlrs,
+ data, &data->user);
+ if (err < 0) {
+ printk(KERN_ERR DRVNAME ": Error, unable to register user with "
+ "ipmi interface %d\n",
+ data->interface);
+ goto out;
+ }
+
+ mutex_init(&data->lock);
+
+ /* Initialize message */
+ data->tx_msgid = 0;
+ init_completion(&data->read_complete);
+ data->tx_message.netfn = 0x3A;
+ data->tx_message.cmd = 0x3C;
+ data->tx_message.data = data->tx_msg_data;
+
+ /* Does this BMC support PowerExecutive? */
+ err = ibmpex_ver_check(data);
+ if (err)
+ goto out_user;
+
+ /* Register the BMC as a HWMON class device */
+ data->class_dev = hwmon_device_register(data->bmc_device);
+
+ if (IS_ERR(data->class_dev)) {
+ printk(KERN_ERR DRVNAME ": Error, unable to register hwmon "
+ "class device for interface %d\n",
+ data->interface);
+ kfree(data);
+ return;
+ }
+
+ /* finally add the new bmc data to the bmc data list */
+ list_add_tail(&data->list, &driver_data.bmc_data);
+
+ /* Now go find all the sensors */
+ err = ibmpex_find_sensors(data);
+ if (err) {
+ printk(KERN_ERR "Error %d allocating memory\n", err);
+ goto out_register;
+ }
+
+ return;
+
+out_register:
+ hwmon_device_unregister(data->class_dev);
+out_user:
+ ipmi_destroy_user(data->user);
+out:
+ kfree(data);
+}
+
+static void ibmpex_bmc_delete(struct ibmpex_bmc_data *data)
+{
+ int i;
+
+ device_remove_file(data->bmc_device, &sensor_dev_attr_name.dev_attr);
+ for (i = 0; i < data->num_sensors; i++) {
+ if (data->sensors[i].attr_max.dev_attr.attr.name)
+ device_remove_file(data->bmc_device,
+ &data->sensors[i].attr_max.dev_attr);
+ kfree(data->sensors[i].attr_max.dev_attr.attr.name);
+ if (data->sensors[i].attr_min.dev_attr.attr.name)
+ device_remove_file(data->bmc_device,
+ &data->sensors[i].attr_min.dev_attr);
+ kfree(data->sensors[i].attr_min.dev_attr.attr.name);
+ if (data->sensors[i].attr_value.dev_attr.attr.name)
+ device_remove_file(data->bmc_device,
+ &data->sensors[i].attr_value.dev_attr);
+ kfree(data->sensors[i].attr_value.dev_attr.attr.name);
+ }
+ list_del(&data->list);
+ hwmon_device_unregister(data->class_dev);
+ ipmi_destroy_user(data->user);
+ if (data->sensors)
+ kfree(data->sensors);
+ kfree(data);
+}
+
+static void ibmpex_bmc_gone(int iface)
+{
+ struct ibmpex_bmc_data *data = get_bmc_data(iface);
+
+ if (!data)
+ return;
+
+ ibmpex_bmc_delete(data);
+}
+
+static void ibmpex_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data)
+{
+ struct ibmpex_bmc_data *data = (struct ibmpex_bmc_data *)user_msg_data;
+
+ if (msg->msgid != data->tx_msgid) {
+ printk(KERN_ERR "Received msgid (%02x) and transmitted "
+ "msgid (%02x) mismatch!\n",
+ (int)msg->msgid,
+ (int)data->tx_msgid);
+ ipmi_free_recv_msg(msg);
+ return;
+ }
+
+ data->rx_recv_type = msg->recv_type;
+ if (msg->msg.data_len > 0)
+ data->rx_result = msg->msg.data[0];
+ else
+ data->rx_result = IPMI_UNKNOWN_ERR_COMPLETION_CODE;
+
+ if (msg->msg.data_len > 1) {
+ data->rx_msg_len = msg->msg.data_len - 1;
+ memcpy(data->rx_msg_data, msg->msg.data + 1, data->rx_msg_len);
+ } else
+ data->rx_msg_len = 0;
+
+ ipmi_free_recv_msg(msg);
+ complete(&data->read_complete);
+}
+
+static int __init ibmpex_init(void)
+{
+ return ipmi_smi_watcher_register(&driver_data.bmc_events);
+}
+
+static void __exit ibmpex_exit(void)
+{
+ struct ibmpex_bmc_data *p, *next;
+
+ ipmi_smi_watcher_unregister(&driver_data.bmc_events);
+ list_for_each_entry_safe(p, next, &driver_data.bmc_data, list)
+ ibmpex_bmc_delete(p);
+}
+
+MODULE_AUTHOR("Darrick J. Wong <[email protected]>");
+MODULE_DESCRIPTION("IBM PowerExecutive power/temperature sensor driver");
+MODULE_LICENSE("GPL");
+
+module_init(ibmpex_init);
+module_exit(ibmpex_exit);

Attachments:

(No filename) (18.64 kB)
signature.asc (189.00 B)
Digital signature Download all attachments

2007-08-28 01:50:44

by Henrique de Moraes Holschuh

[permalink] [raw]

Subject: Re: [PATCH] v1 of IBM power meter driver

On Mon, 27 Aug 2007, Darrick J. Wong wrote:
> documentation doesn't mention any naming conventions for sensors that
> measure Watts, so I am proposing that they be called "powerX_input" in a
> fashion similar to temperature/rpm/current sensors. If that is
> agreeable to everyone, I'll post a follow-up patch to amend the
> documentation.

What unit should we use? Watts are way, way too big as there is no
floating/fixed point in sysfs. 10^-6 W is probably what is called for,
since we already need 10^-3 V and 10^-3 A. Small portable devices can
easily draw less than 10^-3 W nowadays.

--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh

2007-08-28 11:19:19

by Jean Delvare

Hi Darrick,

On Tue, 28 Aug 2007 09:44:40 -0700, Darrick J. Wong wrote:
> Update the hwmon sysfs interface documentation to include a specification
> for power meters.
>
> Signed-off-by: Darrick J. Wong <[email protected]>
> ---
>
> Documentation/hwmon/sysfs-interface | 15 +++++++++++++++
> 1 files changed, 15 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/hwmon/sysfs-interface b/Documentation/hwmon/sysfs-interface
> index b3a9e1b..da546ce 100644
> --- a/Documentation/hwmon/sysfs-interface
> +++ b/Documentation/hwmon/sysfs-interface
> @@ -304,6 +304,21 @@ curr[1-*]_input Current input value
> Unit: milliampere
> RO
>
> +*********
> +* Power *
> +*********
> +
> +power[1-*]_input Current power use
> + Unit: milliWatt
> + RO
> +
> +power[1-*]_max_input Historical maximum power use
> + Unit: milliWatt
> + RO
> +
> +power[1-*]_min_input Historical minimum power use
> + Unit: milliWatt
> + RO
>

I'm not sure if we want these "historical" files. We don't have them for
the other input types, and I believe that it's not the driver's job to
compute and export these values. If anyone cares about the history of
sensed values, that's something for a user-space application to
implement. This will also be much more flexible in user-space, as it
becomes possible to decide the exact time range to consider, to
remember at which time the peak occurred, etc.

--
Jean Delvare

2007-08-29 09:49:21

by Jean Delvare

[permalink] [raw]

Subject: Re: [lm-sensors] [PATCH] v1 of IBM power meter driver

Hi Darrick,

On Tue, 28 Aug 2007 09:28:51 -0700, Darrick J. Wong wrote:
> On Tue, Aug 28, 2007 at 01:19:42PM +0200, Jean Delvare wrote:
> > I see that the driver relies on IPMI. Can't it be merged with the
> > out-of-tree impisensors driver then? This would give that driver some
> > momentum so that it can finally be merged, and I would like to avoid
> > having two drivers if one is enough. Note though that I don't know
> > anything about IPMI so I might as well be totally wrong ;)
>
> The IBM PEx meters are accessible via custom IPMI commands, not the
> standard IPMI sensor interface. I considered modifying the ipmisensors
> driver, but reached the opinion that it would clutter that driver
> unnecessarily, particularly since there are a lot of systems with IPMI
> BMCs, but most of them will not have this interface.

OK, fine with me then.

> That said, what is standing in the way of ipmisensors being merged? I
> think I can shake out some free time to apply TLC.

I don't know exactly, check out the website for details:
http://bmcsensors-26.sourceforge.net/

It needs at least review and testing, but maybe there are a few more
steps before merging can be considered. Adding Yani (the author) to Cc,
please work with him if you're interested.

--
Jean Delvare

2007-08-29 12:46:13

by Frank Phillips

[permalink] [raw]

Subject: Re: [lm-sensors] [PATCH] v1 of IBM power meter driver

> It needs at least review and testing, but maybe there are a few more
> steps before merging can be considered. Adding Yani (the author) to Cc,
> please work with him if you're interested.

As a user, I can tell you that it completely fills my dmesg with this sort

ipmisensors: sensor 50 (type 1) reading 60
ipmisensors: Send 0x2d 0x30 0x0
ipmisensors: received message
ipmisensors: sensor 48 (type 1) reading 28
ipmisensors: Send 0x2d 0x1b 0x0
ipmisensors: received message
ipmisensors: sensor 27 (type 2) reading 34
ipmisensors: Send 0x2d 0x1a 0x0
ipmisensors: received message
ipmisensors: sensor 26 (type 2) reading 187
ipmisensors: Send 0x2d 0x18 0x0

That's the only problem I have. It would be great to see it merged.

Frank

2007-08-29 14:50:22

Hi Darrick,

On Fri, 14 Sep 2007 12:29:24 -0700, Darrick J. Wong wrote:
> Update the hwmon sysfs interface documentation to include a specification
> for power meters.

Thanks for the update. I have some more comments. And sorry if it looks
like nitpicking, but this is a standard interface we're defining so we
better make sure that we get it right.

First of all, please think of a better subject line for this patch.
"Update Documentation/hwmon/sysfs-interface" is too vague when your
patch is rather specific.

> Signed-off-by: Darrick J. Wong <[email protected]>
> ---
>
> Documentation/hwmon/sysfs-interface | 30 ++++++++++++++++++++++++++++++
> 1 files changed, 30 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/hwmon/sysfs-interface b/Documentation/hwmon/sysfs-interface
> index db7bb4a..5c98bee 100644
> --- a/Documentation/hwmon/sysfs-interface
> +++ b/Documentation/hwmon/sysfs-interface
> @@ -324,6 +324,36 @@ curr[1-*]_input Current input value
> Unit: milliampere
> RO
>
> +*********
> +* Power *
> +*********
> +
> +power[1-*]_average Average power use
> + Unit: microWatt
> + RO
> +
> +power[1-*]_average_highest Historical average maximum power use
> + Unit: microWatt
> + RO
> +
> +power[1-*]_average_lowest Historical average minimum power use
> + Unit: microWatt
> + RO

How useful are historical extremes of an average?

> +
> +power[1-*]_input Instantaneous power use
> + Unit: microWatt
> + RO
> +
> +power[1-*]_input_highest Historical maximum power use
> + Unit: microWatt
> + RO
> +
> +power[1-*]_input_lowest Historical minimum power use
> + Unit: microWatt
> + RO
> +
> +power[1-*]_high_low_reset Reset input_highest/input_lowest.
> + WO

I don't much like this name. It sounds like a name crafted for the
specific feature of a given chip, while we try to use generic names for
the standard interface. I would rather go for power[1-*]_reset_history
if we have a single file for resetting all the extremes of a given
channel, or power[1-*]_input_lowest_reset and
power[1-*]_input_highest_reset if we go for a per-value reset.

Alternatively, we could simply make the power[1-*]_input_lowest and
power[1-*]_input_highest files writable, and "cat power1_input >
power1_input_lowest" (or any write?) would reset the history.

>
> **********
> * Alarms *

Thanks,
--
Jean Delvare

2007-09-17 18:44:20

On Tue, Oct 09, 2007 at 08:00:44AM -0400, Mark M. Hoffman wrote:

> Sorry it took me so long to get back to this. I've applied this to my hwmon
> testing branch, but I would still like to get a review from an IPMI expert.

I'm working on finding somebody to do that.

> OBTW: please try to send patches which apply cleanly to some tree to which I
> have access. E.g. from the Makefile - preceding and trailing context are not
> present in any git tree that I know.

Oops, sorry about that.

The i5k_amb driver referenced in the Makefile diff is the same one that
I posted to lm-sensors a few weeks ago, though there hasn't been much
discussion after it was pointed out that someone had already written a
driver to achieve the same purpose. Unfortunately, nothing's happened
to either driver; any thoughts? I'm thinking that we ought to get one
of them into shape for upstream, though I don't particularly care which
one it is...

--D

Attachments:

(No filename) (939.00 B)
signature.asc (189.00 B)
Digital signature Download all attachments