v1 of IBM power meter driver

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.

Quoted 5 lines · Darrick J. Wong

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.

Hi Darrick, hi Henrique,

Good thing that manufacturers start including wattmeters in their hardware. Hopefully this will help users better control their power consumption in the long run.

Quoted 4 lines

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.

Good point. The driver currently exports non-integer values, which is not acceptable, so indeed it needs to be changed. We want at least a

resolution of 10^-3 W. Not sure about 10^-6 W. I am surprised that portable devices can really draw less than 1 mW, and be it the case, I

doubt that manufacturers will embed a wattmetter: it would probably draw more current than the rest of the device ;) so it may not be relevant for our decision.

So I think I'd go with 10^-3 W, but I welcome diverging opinions. Out of curiosity, what is the physical resolution of IBM's device?

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

Quoted 5 lines · Jean Delvare

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.

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

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.

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(-)

Documentation/hwmon/sysfs-interface +6 -0

index b3a9e1b..da546ce 100644
@@ -304,6 +304,21 @@ curr[1-*]_input	Current input value
 		Unit: milliampere
 		RO
 
+*********
+* Power *
+*********
+
+power[1-*]_input	Current power use
+			Unit: milliWatt

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

Quoted 3 lines · Jean Delvare

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

In the case of ibmpex, it is the _hardware_ that computes the historical data; the driver merely exports what it sees.

OK, that's a bit different then, but I'm still not sure that there is much value in exporting these values in sysfs, in particular if there is no way to reset them.

I am also not happy with the names you proposed: power1_max_input and power1_min_input are somewhat confusing IMHO, I'd suggest power1_input_highest and power1_input_lowest to make them clearly different from the min and max limits we have for other sensor types. If we have them at all, of course.

Documentation/hwmon/sysfs-interface +5 -0

index b3a9e1b..da546ce 100644
@@ -304,6 +304,21 @@ curr[1-*]_input	Current input value
 		Unit: milliampere
 		RO
 
+*********
+* Power *
+*********
+
+power[1-*]_input	Current power use

Quoted 18 lines

Documentation/hwmon/sysfs-interface +10 -0

index b3a9e1b..da546ce 100644
@@ -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

Documentation/hwmon/sysfs-interface curr[1-*]_input Current input value · line 321

Quoted 5 lines

Documentation/hwmon/sysfs-interface +5 -0

+                     RO
+
+power[1-*]_min_input Historical minimum power use
+                     Unit: milliWatt
+                     RO

Should we add power?_10sec_avg_input? IIRC thinkpads export that, too.

Quoted 2 lines

Should we add power?_10sec_avg_input? IIRC thinkpads export that, too.

It would be nice if the power meter interface uses the same conventions as the power supply interface, since the former is essentially a subset of the latter. Userspace app that read power meters via sysfs should work for power supplies too.

Quoted 2 lines · Shem Multinymous

Quoted 2 lines

Should we add power?_10sec_avg_input? IIRC thinkpads export that, too.

I'd say it is enough to know it is an average, you don't need to specify HOW it is averaged.

But there is a real need to export both instantaneous and averaged values in power supplies/power sources, so we should have a standard way to do so in hwmon, and power_supply class should use it (or the opposite, it doesn't matter, as long as everyone does it the same way).

Quoted 6 lines · Henrique de Moraes Holschuh

Quoted 2 lines · Shem Multinymous

Quoted 2 lines

Should we add power?_10sec_avg_input? IIRC thinkpads export that, too.

I'd say it is enough to know it is an average, you don't need to specify HOW it is averaged.

Agreed.

Quoted 4 lines

But there is a real need to export both instantaneous and averaged values in power supplies/power sources, so we should have a standard way to do so in hwmon, and power_supply class should use it (or the opposite, it doesn't matter, as long as everyone does it the same way).

I guess power[1-*]_average would be OK?

Quoted 1 line · Jean Delvare

I guess power[1-*]_average would be OK?

AFAIK, yes. It is probably not 100% in sync with the power supply class, though.

Quoted 3 lines · Henrique de Moraes Holschuh

Quoted 1 line · Jean Delvare

I guess power[1-*]_average would be OK?

Quoted 2 lines

AFAIK, yes. It is probably not 100% in sync with the power supply class, though.

Is the power supply class creating sysfs files? I see a number of attributes listed in Documentation/power_supply_class.txt, but they are all uppercase, which doesn't seem suitable for sysfs file names. That document also doesn't list the numbering convention when multiple channels are present.

My main worry is that we will have to add support for power measurement in libsensors, and I would like it to be as easy as possible. Thus sticking to the same naming convention hwmon have been using for years appears to be the best solution.

I see that the power supply class units are 10^-6 A and 10^-6 W, so if we are supposed to be compatible, I guess we'll have to use this too.

Quoted 2 lines · Jean Delvare

I see that the power supply class units are 10^-6 A and 10^-6 W, so if we are supposed to be compatible, I guess we'll have to use this too.

There was a good reason for that, and people who deal with small portables said that they needed 10^-4 A or somesuch, at which point it makes more sense to just go to 10^-6 already. I don't recall why 10^-6 W, though.

Quoted 4 lines · Henrique de Moraes Holschuh

Quoted 2 lines · Jean Delvare

I see that the power supply class units are 10^-6 A and 10^-6 W, so if we are supposed to be compatible, I guess we'll have to use this too.

Quoted 3 lines

There was a good reason for that, and people who deal with small portables said that they needed 10^-4 A or somesuch, at which point it makes more sense to just go to 10^-6 already. I don't recall why 10^-6 W, though.

Well, if we need 10^-4 A, and we use voltages in the 1-2 V range (that's what CPU, AGP etc. use these days), then we obviously need 10^-4 W as well.

Dave Hansen complained about the magic numbers, repetitive code, and various other minor problems with the driver code, so here's a v3 with the magic numbers migrated to the top of the file and #define'd, helper macros taking place of the bit shifting/masking activities, and the compression of the value/min/max sysfs code into parameterized functions.
--
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 |  564 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 577 insertions(+), 0 deletions(-)

drivers/hwmon/Kconfig +0 -0

index 555f470..41ffa2e 100644

drivers/hwmon/Kconfig +9 -0

+	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

drivers/hwmon/Makefile +1 -0

index a133981..31da6fe 100644
@@ -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

drivers/hwmon/ibmpex.c new file +15 -0

new file mode 100644
index 0000000..632f897
@@ -0,0 +1,564 @@
+/*
+ * 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.

drivers/hwmon/ibmpex.c new file +7 -0

+#include <linux/mutex.h>
+
+#define REFRESH_INTERVAL	(5 * HZ)
+#define DRVNAME			"ibmpex"
+
+#define PEX_GET_VERSION		1
+#define PEX_GET_SENSOR_COUNT	2

drivers/hwmon/ibmpex.c new file +4 -0

+}
+
+#define PEX_INTERFACE(idx)		((idx) >> 16)
+#define PEX_SENSOR(idx)			(((idx) >> 8) & 0xFF)

drivers/hwmon/ibmpex.c new file +5 -0

+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 {

drivers/hwmon/ibmpex.c new file +168 -0

+	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] = PEX_GET_VERSION;
+	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,
+	       extract_value(data->rx_msg_data, 2),
+	       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] = PEX_GET_SENSOR_COUNT;
+	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] = PEX_GET_SENSOR_NAME;
+	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] = PEX_GET_SENSOR_DATA;
+	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].values[0] > +			extract_value(data->rx_msg_data, 16);
+		data->sensors[i].values[1] > +			extract_value(data->rx_msg_data, 18);
+		data->sensors[i].values[2] > +			extract_value(data->rx_msg_data, 20);
+	}
+
+	data->last_updated = jiffies;
+	data->valid = 1;
+
+out:
+	mutex_unlock(&data->lock);
+}
+
+static struct ibmpex_bmc_data *get_bmc_data(int iface)

drivers/hwmon/ibmpex.c new file +6 -0

+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);

drivers/hwmon/ibmpex.c new file +68 -0

+	int multiplier = data->sensors[sensor].multiplier;
+	ibmpex_update_device(data);
+
+	return sprintf(buf, "%d\n",
+		       data->sensors[sensor].values[func] * multiplier);
+}
+
+static int is_power_sensor(const char *sensor_id, int len)
+{
+	if (len < PEX_SENSOR_TYPE_LEN)
+		return 0;
+
+	if (!memcmp(sensor_id, power_sensor_sig, PEX_SENSOR_TYPE_LEN))
+		return 1;
+	return 0;
+}
+
+static int is_temp_sensor(const char *sensor_id, int len)
+{
+	if (len < PEX_SENSOR_TYPE_LEN)
+		return 0;
+
+	if (!memcmp(sensor_id, temp_sensor_sig, PEX_SENSOR_TYPE_LEN))
+		return 1;
+	return 0;
+}
+
+static int power_sensor_multiplier(const char *sensor_id, int len)
+{
+	int i;
+
+	for (i = PEX_SENSOR_TYPE_LEN; i < len - 1; i++)
+		if (!memcmp(&sensor_id[i], watt_sensor_sig, PEX_MULT_LEN))
+			return 1000;
+
+	return 100;
+}
+
+static int create_sensor(struct ibmpex_bmc_data *data, const char *type,
+			 int counter, int sensor, int func)
+{
+	int err;
+	char *n;
+
+	n = kmalloc(32, GFP_KERNEL);
+	if (!n)
+		return -ENOMEM;
+
+	sprintf(n, sensor_name_templates[func], type, counter);
+	data->sensors[sensor].attr[func].dev_attr.attr.name = n;
+	data->sensors[sensor].attr[func].dev_attr.attr.mode = S_IRUGO;
+	data->sensors[sensor].attr[func].dev_attr.show = ibmpex_show_sensor;
+	data->sensors[sensor].attr[func].index > +		PEX_INDEX(data->interface, sensor, func);
+
+	err = device_create_file(data->bmc_device,
+				 &data->sensors[sensor].attr[func].dev_attr);
+	if (err) {
+		data->sensors[sensor].attr[func].dev_attr.attr.name = NULL;
+		kfree(n);
+		return err;
+	}
+
+	return 0;
+}
+
+static int ibmpex_find_sensors(struct ibmpex_bmc_data *data)
+{
+	int i, j, err;

drivers/hwmon/ibmpex.c new file +21 -0

+	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].multiplier > +				power_sensor_multiplier(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;

drivers/hwmon/ibmpex.c new file +12 -0

+				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++)

drivers/hwmon/ibmpex.c new file +146 -0

+}
+
+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 = PEX_NET_FUNCTION;
+	data->tx_message.cmd = PEX_COMMAND;
+	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, j;
+
+	device_remove_file(data->bmc_device, &sensor_dev_attr_name.dev_attr);
+	for (i = 0; i < data->num_sensors; i++)
+		for (j = 0; j < PEX_NUM_SENSOR_FUNCS; j++) {
+			if (!data->sensors[i].attr[j].dev_attr.attr.name)
+				continue;
+			device_remove_file(data->bmc_device,
+				&data->sensors[i].attr[j].dev_attr);
+			kfree(data->sensors[i].attr[j].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);

Quoted 6 lines · Darrick J. Wong

drivers/hwmon/Kconfig +0 -0

@@ -275,6 +275,18 @@ config SENSORS_CORETEMP
 	  sensor inside your CPU. Supported all are all known variants
 	  of Intel Core family.
 

drivers/hwmon/Kconfig config SENSORS_CORETEMP · line 280

Quoted 3 lines

drivers/hwmon/Kconfig +3 -0

+config SENSORS_IBMPEX
+	tristate "IBM PowerExecutive temperature/power sensors"
+	depends on IPMI_SI

Open question: can we use "select" here? As written, it took some hunting to even get this driver to show up as an option in menuconfig.

I am not an IPMI expert, so I would appreciate getting an Acked-by from someone who knows more about that subsystem.

Anyway, some comments are below. This is nowhere near a complete review yet.

drivers/hwmon/Kconfig config SENSORS_CORETEMP · line 280

Quoted 3 lines

drivers/hwmon/Kconfig +3 -0

+config SENSORS_IBMPEX
+	tristate "IBM PowerExecutive temperature/power sensors"
+	depends on IPMI_SI

Open question: can we use "select" here? As written, it took some hunting to even get this driver to show up as an option in menuconfig.

drivers/hwmon/ibmpex.c line 78

Quoted 3 lines

drivers/hwmon/ibmpex.c new file +3 -0

+struct ibmpex_bmc_data {
+	struct list_head	list;
+	struct class_device	*class_dev;

My current stack of patches includes one which requires that this be changed to 'struct device *hwmon_dev', as 'struct class_device' is going away soon. You may rebase on my testing tree[1], or else I will just follow up with a patch to fix this up after I eventually merge yours.

[1] http://lm-sensors.org/kernel?p=kernel/mhoffman/hwmon-2.6.git;a=shortlog;h=testing

Quoted 4 lines

drivers/hwmon/ibmpex.c new file +4 -0

+static ssize_t ibmpex_show_sensor(struct device *dev,
+				  struct device_attribute *devattr,
+				  char *buf)
+{

drivers/hwmon/ibmpex.c line 275

Quoted 5 lines

drivers/hwmon/ibmpex.c new file +5 -0

+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	int iface = PEX_INTERFACE(attr->index);
+	int sensor = PEX_SENSOR(attr->index);
+	int func = PEX_FUNC(attr->index);
+	struct ibmpex_bmc_data *data = get_bmc_data(iface);

... especially given how many times you're going to call it. Is there any reason you can't use the driver_data field of struct device *dev for that?

E.g. i2c based hwmon drivers do this at some point during the probe:

	i2c_set_clientdata(new_client, data);

(which becomes)

	dev_set_drvdata(&new_client->dev, data);

If you could do that, then you no longer need 'iface' at all in the function above... *that* may allow you to use the SENSOR_ATTR_2 mechanism from hwmon-sysfs.h - much easier to read than the manual number packing for 'sensor' and 'func'.

drivers/hwmon/ibmpex.c line 354

Quoted 5 lines

drivers/hwmon/ibmpex.c new file +5 -0

+	err = ibmpex_query_sensor_count(data);
+	if (err < 0)
+		return -ENOENT;
+	data->num_sensors = err;
+

Did you mean 'if (err <= 0)' ?

drivers/hwmon/ibmpex.c line 386

Quoted 4 lines

drivers/hwmon/ibmpex.c new file +4 -0

+		/* Create attributes */
+		for (j = 0; j < PEX_NUM_SENSOR_FUNCS; j++)
+			if (create_sensor(data, sensor_type, sensor_counter,
+					  i, j))

Why not 'err = create_sensor(...)' and propagate the actual error here?

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 |   30 ++++++++++++++++++++++++++++++
 1 files changed, 30 insertions(+), 0 deletions(-)

Documentation/hwmon/sysfs-interface +5 -0

index db7bb4a..5c98bee 100644
@@ -324,6 +324,36 @@ curr[1-*]_input	Current input value
 		Unit: milliampere
 		RO
 
+*********
+* Power *
+*********
+
+power[1-*]_average		Average power use

Documentation/hwmon/sysfs-interface +5 -0

+
+power[1-*]_input		Instantaneous power use
+				Unit: microWatt
+				RO
+

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.

Documentation/hwmon/sysfs-interface curr[1-*]_input Current input value · line 341

Quoted 3 lines

Documentation/hwmon/sysfs-interface +3 -0

+power[1-*]_average_lowest	Historical average minimum power use
+				Unit: microWatt
+				RO

How useful are historical extremes of an average?

Documentation/hwmon/sysfs-interface curr[1-*]_input Current input value · line 356

Quoted 2 lines

Documentation/hwmon/sysfs-interface +2 -0

+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.

Quoted 3 lines · Jean Delvare

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.

Will change to:
"Add power meter spec to Documentation/hwmon/sysfs-interface"

Quoted 5 lines

Documentation/hwmon/sysfs-interface curr[1-*]_input Current input value · line 341

Quoted 3 lines

Documentation/hwmon/sysfs-interface +3 -0

+power[1-*]_average_lowest	Historical average minimum power use
+				Unit: microWatt
+				RO

How useful are historical extremes of an average?

I don't know if anybody ever really requires this data, but the device I'm talking to records the extremes in hardware, so I put it in the driver.

Quoted 5 lines

Documentation/hwmon/sysfs-interface curr[1-*]_input Current input value · line 356

Quoted 2 lines

Documentation/hwmon/sysfs-interface +2 -0

+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

Guilty as charged. Since ibmpex is the only on-board power meter hardware to which I have access, the interface proposal includes the various odd pieces that the hardware provides in addition to the meters. I could take the historical extreme and reset bits out of the proposal and put them in something like Documentation/hwmon/ibmpex.txt as extra chip-specific features if people prefer that.

Quoted 1 line

the standard interface. I would rather go for power[1-*]_reset_history

Me too.

Quoted 3 lines

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.

The ibmpex hardware only knows how to reset all of them.

Quoted 3 lines

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.

I'd prefer to leave it explicitly called out as a separate sysfs knob, unless there are established precedents for resetting a sensor by writing something to its sysfs file.

Here's a fourth revision where I've tried to clean up the things that people complained about, as well as shifted the sysfs file names to match the spec that we've been drifting towards.

---
ibm_pex: Driver to export IBM PowerExecutive power meter sensors.

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

---

 drivers/hwmon/Kconfig  |   13 +
 drivers/hwmon/Makefile |    1 
 drivers/hwmon/ibmpex.c |  608 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 622 insertions(+), 0 deletions(-)

drivers/hwmon/Kconfig +13 -0

index 5ca5d95..591b666 100644
@@ -315,6 +315,19 @@ 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"
+	select IPMI_SI
+	depends on IPMI_HANDLER
+	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

drivers/hwmon/Makefile +1 -0

index 5070cf7..0fcbcb4 100644
@@ -38,6 +38,7 @@ obj-$(CONFIG_SENSORS_GL518SM)	+= gl518sm.o
 obj-$(CONFIG_SENSORS_GL520SM)	+= gl520sm.o
 obj-$(CONFIG_SENSORS_HDAPS)	+= hdaps.o
 obj-$(CONFIG_SENSORS_I5K_AMB)	+= i5k_amb.o
+obj-$(CONFIG_SENSORS_IBMPEX)	+= ibmpex.o
 obj-$(CONFIG_SENSORS_IPMI)	+= ipmisensors.o
 obj-$(CONFIG_SENSORS_IT87)	+= it87.o
 obj-$(CONFIG_SENSORS_K8TEMP)	+= k8temp.o

drivers/hwmon/ibmpex.c new file +594 -0

new file mode 100644
index 0000000..fe2c261
@@ -0,0 +1,608 @@
+/*
+ * 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/jiffies.h>
+#include <linux/mutex.h>
+
+#define REFRESH_INTERVAL	(2 * HZ)
+#define DRVNAME			"ibmpex"
+
+#define PEX_GET_VERSION		1
+#define PEX_GET_SENSOR_COUNT	2
+#define PEX_GET_SENSOR_NAME	3
+#define PEX_RESET_HIGH_LOW	4
+#define PEX_GET_SENSOR_DATA	6
+
+#define PEX_NET_FUNCTION	0x3A
+#define PEX_COMMAND		0x3C
+
+static inline u16 extract_value(const char *data, int offset)
+{
+	return be16_to_cpup((u16 *)&data[offset]);
+}
+
+#define TEMP_SENSOR		1
+#define POWER_SENSOR		2
+
+#define PEX_SENSOR_TYPE_LEN	3
+static u8 const power_sensor_sig[] = {0x70, 0x77, 0x72};
+static u8 const temp_sensor_sig[]  = {0x74, 0x65, 0x6D};
+
+#define PEX_MULT_LEN		2
+static u8 const watt_sensor_sig[]  = {0x41, 0x43};
+
+#define PEX_NUM_SENSOR_FUNCS	3
+static char const * const power_sensor_name_templates[] = {
+	"%s%d_average",
+	"%s%d_average_lowest",
+	"%s%d_average_highest"
+};
+static char const * const temp_sensor_name_templates[] = {
+	"%s%d_input",
+	"%s%d_input_lowest",
+	"%s%d_input_highest"
+};
+
+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			values[PEX_NUM_SENSOR_FUNCS];
+	int			multiplier;
+
+	struct sensor_device_attribute_2	attr[PEX_NUM_SENSOR_FUNCS];
+};
+
+struct ibmpex_bmc_data {
+	struct list_head	list;
+	struct device		*hwmon_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] = PEX_GET_VERSION;
+	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,
+	       extract_value(data->rx_msg_data, 2),
+	       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] = PEX_GET_SENSOR_COUNT;
+	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] = PEX_GET_SENSOR_NAME;
+	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] = PEX_GET_SENSOR_DATA;
+	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 int ibmpex_reset_high_low_data(struct ibmpex_bmc_data *data)
+{
+	data->tx_msg_data[0] = PEX_RESET_HIGH_LOW;
+	data->tx_message.data_len = 1;
+	ibmpex_send_message(data);
+
+	wait_for_completion(&data->read_complete);
+
+	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].values[0] > +			extract_value(data->rx_msg_data, 16);
+		data->sensors[i].values[1] > +			extract_value(data->rx_msg_data, 18);
+		data->sensors[i].values[2] > +			extract_value(data->rx_msg_data, 20);
+	}
+
+	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_2 *attr = to_sensor_dev_attr_2(devattr);
+	struct ibmpex_bmc_data *data = dev_get_drvdata(dev);
+	int mult = data->sensors[attr->index].multiplier;
+	ibmpex_update_device(data);
+
+	return sprintf(buf, "%d\n",
+		       data->sensors[attr->index].values[attr->nr] * mult);
+}
+
+static ssize_t ibmpex_reset_high_low(struct device *dev,
+				     struct device_attribute *devattr,
+				     const char *buf,
+				     size_t count)
+{
+	struct ibmpex_bmc_data *data = dev_get_drvdata(dev);
+
+	ibmpex_reset_high_low_data(data);
+
+	return count;
+}
+
+static SENSOR_DEVICE_ATTR(reset_high_low, S_IWUSR, NULL,
+			  ibmpex_reset_high_low, 0);
+
+static int is_power_sensor(const char *sensor_id, int len)
+{
+	if (len < PEX_SENSOR_TYPE_LEN)
+		return 0;
+
+	if (!memcmp(sensor_id, power_sensor_sig, PEX_SENSOR_TYPE_LEN))
+		return 1;
+	return 0;
+}
+
+static int is_temp_sensor(const char *sensor_id, int len)
+{
+	if (len < PEX_SENSOR_TYPE_LEN)
+		return 0;
+
+	if (!memcmp(sensor_id, temp_sensor_sig, PEX_SENSOR_TYPE_LEN))
+		return 1;
+	return 0;
+}
+
+static int power_sensor_multiplier(const char *sensor_id, int len)
+{
+	int i;
+
+	for (i = PEX_SENSOR_TYPE_LEN; i < len - 1; i++)
+		if (!memcmp(&sensor_id[i], watt_sensor_sig, PEX_MULT_LEN))
+			return 1000000;
+
+	return 100000;
+}
+
+static int create_sensor(struct ibmpex_bmc_data *data, int type,
+			 int counter, int sensor, int func)
+{
+	int err;
+	char *n;
+
+	n = kmalloc(32, GFP_KERNEL);
+	if (!n)
+		return -ENOMEM;
+
+	if (type = TEMP_SENSOR)
+		sprintf(n, temp_sensor_name_templates[func], "temp", counter);
+	else if (type = POWER_SENSOR)
+		sprintf(n, power_sensor_name_templates[func], "power", counter);
+
+	data->sensors[sensor].attr[func].dev_attr.attr.name = n;
+	data->sensors[sensor].attr[func].dev_attr.attr.mode = S_IRUGO;
+	data->sensors[sensor].attr[func].dev_attr.show = ibmpex_show_sensor;
+	data->sensors[sensor].attr[func].index = sensor;
+	data->sensors[sensor].attr[func].nr = func;
+
+	err = device_create_file(data->bmc_device,
+				 &data->sensors[sensor].attr[func].dev_attr);
+	if (err) {
+		data->sensors[sensor].attr[func].dev_attr.attr.name = NULL;
+		kfree(n);
+		return err;
+	}
+
+	return 0;
+}
+
+static int ibmpex_find_sensors(struct ibmpex_bmc_data *data)
+{
+	int i, j, err;
+	int 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_SENSOR;
+			num_power++;
+			sensor_counter = num_power;
+			data->sensors[i].multiplier > +				power_sensor_multiplier(data->rx_msg_data,
+						     data->rx_msg_len);
+		} else if (is_temp_sensor(data->rx_msg_data,
+					  data->rx_msg_len)) {
+			sensor_type = TEMP_SENSOR;
+			num_temp++;
+			sensor_counter = num_temp;
+			data->sensors[i].multiplier = 1;
+		} else
+			continue;
+
+		data->sensors[i].in_use = 1;
+
+		/* Create attributes */
+		for (j = 0; j < PEX_NUM_SENSOR_FUNCS; j++) {
+			err = create_sensor(data, sensor_type, sensor_counter,
+					    i, j);
+			if (err)
+				goto exit_remove;
+		}
+	}
+
+	err = device_create_file(data->bmc_device,
+			&sensor_dev_attr_reset_high_low.dev_attr);
+	if (err)
+		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:
+	device_remove_file(data->bmc_device,
+			   &sensor_dev_attr_reset_high_low.dev_attr);
+	device_remove_file(data->bmc_device, &sensor_dev_attr_name.dev_attr);
+	for (i = 0; i < data->num_sensors; i++)
+		for (j = 0; j < PEX_NUM_SENSOR_FUNCS; j++) {
+			if (!data->sensors[i].attr[j].dev_attr.attr.name)
+				continue;
+			device_remove_file(data->bmc_device,
+				&data->sensors[i].attr[j].dev_attr);
+			kfree(data->sensors[i].attr[j].dev_attr.attr.name);
+		}
+
+	kfree(data->sensors);
+	return err;
+}
+
+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 = PEX_NET_FUNCTION;
+	data->tx_message.cmd = PEX_COMMAND;
+	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->hwmon_dev = hwmon_device_register(data->bmc_device);
+
+	if (IS_ERR(data->hwmon_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 */
+	dev_set_drvdata(dev, data);
+	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->hwmon_dev);
+out_user:
+	ipmi_destroy_user(data->user);
+out:
+	kfree(data);
+}
+
+static void ibmpex_bmc_delete(struct ibmpex_bmc_data *data)
+{
+	int i, j;
+
+	device_remove_file(data->bmc_device,
+			   &sensor_dev_attr_reset_high_low.dev_attr);
+	device_remove_file(data->bmc_device, &sensor_dev_attr_name.dev_attr);
+	for (i = 0; i < data->num_sensors; i++)
+		for (j = 0; j < PEX_NUM_SENSOR_FUNCS; j++) {
+			if (!data->sensors[i].attr[j].dev_attr.attr.name)
+				continue;
+			device_remove_file(data->bmc_device,
+				&data->sensors[i].attr[j].dev_attr);
+			kfree(data->sensors[i].attr[j].dev_attr.attr.name);
+		}
+
+	list_del(&data->list);
+	dev_set_drvdata(data->bmc_device, NULL);
+	hwmon_device_unregister(data->hwmon_dev);
+	ipmi_destroy_user(data->user);
+	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);

Quoted 43 lines

drivers/hwmon/ibmpex.c new file +43 -0

+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 = PEX_NET_FUNCTION;
+	data->tx_message.cmd = PEX_COMMAND;
+	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 */

drivers/hwmon/ibmpex.c line 502

Quoted 8 lines

drivers/hwmon/ibmpex.c new file +8 -0

+	data->hwmon_dev = hwmon_device_register(data->bmc_device);
+
+	if (IS_ERR(data->hwmon_dev)) {
+		printk(KERN_ERR DRVNAME ": Error, unable to register hwmon "
+		       "class device for interface %d\n",
+		       data->interface);
+		kfree(data);
+		return;

Quoted 14 lines

drivers/hwmon/ibmpex.c new file +14 -0

+	}
+
+	/* finally add the new bmc data to the bmc data list */
+	dev_set_drvdata(dev, data);
+	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;

drivers/hwmon/ibmpex.c line 524

Quoted 8 lines

drivers/hwmon/ibmpex.c new file +8 -0

+
+out_register:
+	hwmon_device_unregister(data->hwmon_dev);
+out_user:
+	ipmi_destroy_user(data->user);
+out:
+	kfree(data);
+}

Quoted 6 lines

drivers/hwmon/ibmpex.c line 502

Quoted 6 lines

drivers/hwmon/ibmpex.c new file +6 -0

+	if (IS_ERR(data->hwmon_dev)) {
+		printk(KERN_ERR DRVNAME ": Error, unable to register hwmon "
+		       "class device for interface %d\n",
+		       data->interface);
+		kfree(data);
+		return;

don't you want to goto out_user here instead?

Roel Kluin <[email protected]> found a minor defect in the init code if hwmon device registration fails.

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

---

 drivers/hwmon/ibmpex.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

drivers/hwmon/ibmpex.c +0 -0

index fe2c261..c462824 100644

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.

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.