[PATCH -v8 0/2] IPMI/ACPI: Install the ACPI IPMI opregion

[PATCH -v8 0/2] IPMI/ACPI: Install the ACPI IPMI opregion

[yakui.zhao]

From: Zhao Yakui <yakui.zhao@intel.com>

Hi, 
	
    ACPI 4.0 spec adds the ACPI IPMI opregion, which means that the ACPI AML
code can also communicate with the BMC controller. This patch set is to
install the ACPI IPMI opregion space handler and enable the ACPI to access
the BMC controller through the IPMI message.

[Patch 01/02]: Define acpi ipmi notifier hook function

[Patch 02/02]: Install the IPMI space handler to enable ACPI to access the BMC
controller

V7->V8: Based on Bjorn's comment, use acpi ipmi notifier hook function to
avoid the discovery of ACPI pnp IPMI device again. Then in course of binding
/unbinding ipmi pnp driver with the corresponding device, the notifier chain
function will be called to install/uninstall the ACPI IPMI opregion space
handler. 

V6->V7: Based on Corey Minyard's comments, the IPMI opregion handler is put
into the separate file again as it only uses the API interface provided
by IPMI system. Now it is put into the ACPI subsystem.

V5->V6: Adjust the patch order and refresh the patch set.

V4->V5: According to Bjorn's comment, remove the unnecessary comment.
The debug info will be printed by using dev_err/dev_warn instead of by
using printk directly.

V3->V4: According to Bjorn's comment, delete the meaningless variable
initialization and check. We also do some cleanup. 

V2->V3: According to Bjorn's comment, this IPMI opregion code is put into
the IPMI subsystem(ipmi_si_intf.c). In such case the part of IPMI opregion
code  will be installed automatically when IPMI detection module is loaded.
When the IPMI system interface is detected by loading PNP device driver, we
will record every IPMI system interface defined in ACPI namespace and then
install the corresponding IPMI opregion space handler, which is used to enable
ACPI AML code to access the BMC controller.

V1->V2: According to Bjorn's comment, we won't install the IPMI space handler
by loading an ACPI device driver. Instead we will enumerate the ACPI IPMI
device directly in ACPI device tree and then install the IPMI space handler.
Then ACPI AML code and access the BMC controller through the IPMI space
handler.

Best regards.
   Yakui

[PATCH_v8 1/2] IPMI/ACPI: Define acpi_ipmi notifier hook functions

[yakui.zhao]

From: Zhao yakui <yakui.zhao@intel.com>

Signed-off-by: Zhao yakui <yakui.zhao@intel.com>
cc: Bjorn Helgaas <bjorn.helgaas@hp.com>
---
 drivers/char/ipmi/ipmi_si_intf.c |   63 ++++++++++++++++++++++++++++++++++++++
 include/linux/ipmi.h             |    8 +++++
 2 files changed, 71 insertions(+), 0 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 094bdc3..91c5d37 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -65,6 +65,7 @@
 #include <linux/string.h>
 #include <linux/ctype.h>
 #include <linux/pnp.h>
+#include <linux/ipmi.h>
 
 #ifdef CONFIG_PPC_OF
 #include <linux/of_device.h>
@@ -1907,6 +1908,13 @@ static __devinit void hardcode_find_bmc(void)
  */
 static int acpi_failure;
 
+static BLOCKING_NOTIFIER_HEAD(pnp_ipmi_notifier);
+static LIST_HEAD(pnp_ipmi_list);
+struct pnp_ipmi_device {
+	struct list_head head;
+	struct pnp_dev *pnp_dev;
+};
+
 /* For GPE-type interrupts. */
 static u32 ipmi_acpi_gpe(void *context)
 {
@@ -2124,6 +2132,7 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
 	acpi_handle handle;
 	acpi_status status;
 	unsigned long long tmp;
+	struct pnp_ipmi_device *pnp_ipmi;
 
 	acpi_dev = pnp_acpi_device(dev);
 	if (!acpi_dev)
@@ -2133,6 +2142,11 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
 	if (!info)
 		return -ENOMEM;
 
+	pnp_ipmi = kzalloc(sizeof(*pnp_ipmi), GFP_KERNEL);
+	if (!pnp_ipmi) {
+		kfree(info);
+		return -ENOMEM;
+	}
 	info->addr_source = SI_ACPI;
 	printk(KERN_INFO PFX "probing via ACPI\n");
 
@@ -2196,20 +2210,69 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
 		 res, info->io.regsize, info->io.regspacing,
 		 info->irq);
 
+	pnp_ipmi->pnp_dev = dev;
+	list_add_tail(&pnp_ipmi->head, &pnp_ipmi_list);
+	blocking_notifier_call_chain(&pnp_ipmi_notifier, IPMI_PNP_ADD,
+				(void *)dev);
+
 	return add_smi(info);
 
 err_free:
 	kfree(info);
+	kfree(pnp_ipmi);
 	return -EINVAL;
 }
 
 static void __devexit ipmi_pnp_remove(struct pnp_dev *dev)
 {
 	struct smi_info *info = pnp_get_drvdata(dev);
+	struct pnp_ipmi_device *pnp_ipmi;
+
+	list_for_each_entry(pnp_ipmi, &pnp_ipmi_list, head) {
+		if (pnp_ipmi->pnp_dev == dev) {
+			list_del(&pnp_ipmi->head);
+			blocking_notifier_call_chain(&pnp_ipmi_notifier,
+				IPMI_PNP_REMOVE, (void *)dev);
+			break;
+		}
+	}
 
 	cleanup_one_si(info);
 }
 
+int acpi_ipmi_notifier_register(struct notifier_block *nb)
+{
+	int ret;
+	struct pnp_ipmi_device *pnp_ipmi;
+
+	ret = blocking_notifier_chain_register(&pnp_ipmi_notifier, nb);
+	if (ret == 0) {
+		/*
+		 * Maybe we already get the corresponding pnp_ipmi_list before
+		 * registering the notifier chain. So call the notifer
+		 * chain list for every pnp_ipmi device.
+		 */
+		list_for_each_entry(pnp_ipmi, &pnp_ipmi_list, head) {
+			blocking_notifier_call_chain(&pnp_ipmi_notifier,
+				IPMI_PNP_ADD, (void *)(pnp_ipmi->pnp_dev));
+		}
+	}
+	return ret;
+}
+EXPORT_SYMBOL(acpi_ipmi_notifier_register);
+
+int acpi_ipmi_notifier_unregister(struct notifier_block *nb)
+{
+	struct pnp_ipmi_device *pnp_ipmi;
+
+	list_for_each_entry(pnp_ipmi, &pnp_ipmi_list, head) {
+		blocking_notifier_call_chain(&pnp_ipmi_notifier,
+				IPMI_PNP_REMOVE, (void *)(pnp_ipmi->pnp_dev));
+	}
+	return blocking_notifier_chain_unregister(&pnp_ipmi_notifier, nb);
+}
+EXPORT_SYMBOL(acpi_ipmi_notifier_unregister);
+
 static const struct pnp_device_id pnp_dev_table[] = {
 	{"IPI0001", 0},
 	{"", 0},
diff --git a/include/linux/ipmi.h b/include/linux/ipmi.h
index 65aae34..4ea2a69 100644
--- a/include/linux/ipmi.h
+++ b/include/linux/ipmi.h
@@ -694,4 +694,12 @@ struct ipmi_timing_parms {
 #define IPMICTL_GET_MAINTENANCE_MODE_CMD	_IOR(IPMI_IOC_MAGIC, 30, int)
 #define IPMICTL_SET_MAINTENANCE_MODE_CMD	_IOW(IPMI_IOC_MAGIC, 31, int)
 
+#ifdef CONFIG_ACPI
+#define IPMI_PNP_ADD		1
+#define IPMI_PNP_REMOVE		2
+extern int acpi_ipmi_notifier_register(struct notifier_block *nb);
+extern int acpi_ipmi_notifier_unregister(struct notifier_block *nb);
+
+#endif
+
 #endif /* __LINUX_IPMI_H */
-- 
1.5.4.5

[PATCH_v8 2/2] ACPI: Add the IPMI opregion driver to enable ACPI to access BMC controller

[yakui.zhao]

From: Zhao Yakui <yakui.zhao@intel.com>

ACPI 4.0 spec adds the ACPI IPMI opregion, which means that the ACPI AML
code can also communicate with the BMC controller. This is to install
the ACPI IPMI opregion and enable the ACPI to access the BMC controller
through the IPMI message.

     It will create IPMI user interface for every IPMI device detected
in ACPI namespace and install the corresponding IPMI opregion space handler.
Then it can enable ACPI to access the BMC controller through the IPMI
message.

The following describes how to process the IPMI request in IPMI space handler:
    1. format the IPMI message based on the request in AML code.
    IPMI system address. Now the address type is SYSTEM_INTERFACE_ADDR_TYPE
    IPMI net function & command
    IPMI message payload
    2. send the IPMI message by using the function of ipmi_request_settime
    3. wait for the completion of IPMI message. It can be done in different
routes: One is in handled in IPMI user recv callback function. Another is
handled in timeout function.
    4. format the IPMI response and return it to ACPI AML code.

At the same time it also addes the module dependency. The ACPI IPMI opregion
will depend on the IPMI subsystem.

Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
cc: Bjorn Helgaas <bjorn.helgaas@hp.com>
---
 drivers/acpi/Kconfig     |   10 +
 drivers/acpi/Makefile    |    1 +
 drivers/acpi/acpi_ipmi.c |  548 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 559 insertions(+), 0 deletions(-)
 create mode 100644 drivers/acpi/acpi_ipmi.c

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 7464115..0fa6a01 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -204,6 +204,16 @@ config ACPI_PROCESSOR
 
 	  To compile this driver as a module, choose M here:
 	  the module will be called processor.
+config ACPI_IPMI
+	tristate "IPMI"
+	depends on EXPERIMENTAL && IPMI_SI && IPMI_HANDLER
+	default n
+	help
+	  This driver enables the ACPI to access the BMC controller. And it
+	  uses the IPMI request/response message to communicate with BMC
+	  controller, which can be found on on the server.
+
+	  To compile this driver as a module, choose M here:
 
 config ACPI_HOTPLUG_CPU
 	bool
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 6ee3316..5ca31f1 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -67,5 +67,6 @@ processor-y			+= processor_idle.o processor_thermal.o
 processor-$(CONFIG_CPU_FREQ)	+= processor_perflib.o
 
 obj-$(CONFIG_ACPI_PROCESSOR_AGGREGATOR) += acpi_pad.o
+obj-$(CONFIG_ACPI_IPMI)		+= acpi_ipmi.o
 
 obj-$(CONFIG_ACPI_APEI)		+= apei/
diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c
new file mode 100644
index 0000000..f5a9412
--- /dev/null
+++ b/drivers/acpi/acpi_ipmi.c
@@ -0,0 +1,548 @@
+/*
+ *  ipmi.c - ACPI IPMI opregion
+ *
+ *  Copyright (C) 2010 Intel Corporation
+ *  Copyright (C) 2010 Zhao Yakui <yakui.zhao@intel.com>
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ *  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/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/types.h>
+#include <linux/delay.h>
+#include <linux/proc_fs.h>
+#include <linux/seq_file.h>
+#include <linux/interrupt.h>
+#include <linux/list.h>
+#include <linux/spinlock.h>
+#include <linux/io.h>
+#include <acpi/acpi_bus.h>
+#include <acpi/acpi_drivers.h>
+#include <linux/ipmi.h>
+#include <linux/device.h>
+#include <linux/pnp.h>
+
+MODULE_AUTHOR("Zhao Yakui");
+MODULE_DESCRIPTION("ACPI IPMI Opregion driver");
+MODULE_LICENSE("GPL");
+
+#define IPMI_FLAGS_HANDLER_INSTALL	0
+
+#define ACPI_IPMI_OK			0
+#define ACPI_IPMI_TIMEOUT		0x10
+#define ACPI_IPMI_UNKNOWN		0x07
+/* the IPMI timeout is 5s */
+#define IPMI_TIMEOUT			(5 * HZ)
+
+struct acpi_ipmi_device {
+	/* the device list attached to driver_data.ipmi_devices */
+	struct list_head head;
+	/* the IPMI request message list */
+	struct list_head tx_msg_list;
+	acpi_handle handle;
+	struct pnp_dev	*pnp_dev;
+	ipmi_user_t	user_interface;
+	struct mutex	mutex_lock;
+	int ipmi_ifnum; /* IPMI interface number */
+	long curr_msgid;
+	unsigned long flags;
+};
+
+struct ipmi_driver_data {
+	int device_count;
+	struct list_head	ipmi_devices;
+	struct ipmi_smi_watcher	bmc_events;
+	struct ipmi_user_hndl	ipmi_hndlrs;
+};
+
+struct acpi_ipmi_msg {
+	struct list_head head;
+	/*
+	 * General speaking the addr type should be SI_ADDR_TYPE. And
+	 * the addr channel should be BMC.
+	 * In fact it can also be IPMB type. But we will have to
+	 * parse it from the Netfn command buffer. It is so complex
+	 * that it is skipped.
+	 */
+	struct ipmi_addr addr;
+	long tx_msgid;
+	/* it is used to track whether the IPMI message is finished */
+	struct completion tx_complete;
+	struct kernel_ipmi_msg tx_message;
+	int	msg_done;
+	/* tx data . And copy it from ACPI object buffer */
+	u8	tx_data[64];
+	int	tx_len;
+	u8	rx_data[64];
+	int	rx_len;
+	struct acpi_ipmi_device *device;
+};
+
+/* IPMI request/response buffer per ACPI 4.0, sec 5.5.2.4.3.2 */
+struct acpi_ipmi_buffer {
+	u8 status;
+	u8 length;
+	u8 data[64];
+};
+
+static void ipmi_register_bmc(int iface, struct device *dev);
+static void ipmi_bmc_gone(int iface);
+static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data);
+
+static struct ipmi_driver_data driver_data = {
+	.ipmi_devices = LIST_HEAD_INIT(driver_data.ipmi_devices),
+	.bmc_events = {
+		.owner = THIS_MODULE,
+		.new_smi = ipmi_register_bmc,
+		.smi_gone = ipmi_bmc_gone,
+	},
+	.ipmi_hndlrs = {
+		.ipmi_recv_hndl = ipmi_msg_handler,
+	},
+};
+
+struct acpi_ipmi_msg *acpi_alloc_ipmi_msg(struct acpi_ipmi_device *ipmi)
+{
+	struct acpi_ipmi_msg *ipmi_msg;
+	struct pnp_dev *pnp_dev = ipmi->pnp_dev;
+
+	ipmi_msg = kzalloc(sizeof(struct acpi_ipmi_msg), GFP_KERNEL);
+	if (!ipmi_msg)	{
+		dev_warn(&pnp_dev->dev, "Can't allocate memory for ipmi_msg\n");
+		return NULL;
+	}
+	init_completion(&ipmi_msg->tx_complete);
+	INIT_LIST_HEAD(&ipmi_msg->head);
+	ipmi_msg->device = ipmi;
+	return ipmi_msg;
+}
+
+#define		IPMI_OP_RGN_NETFN(offset)	((offset >> 8) & 0xff)
+#define		IPMI_OP_RGN_CMD(offset)		(offset & 0xff)
+static void acpi_format_ipmi_msg(struct acpi_ipmi_msg *tx_msg,
+				acpi_physical_address address,
+				acpi_integer *value)
+{
+	struct kernel_ipmi_msg *msg;
+	struct acpi_ipmi_buffer *buffer;
+	struct acpi_ipmi_device *device;
+
+	msg = &tx_msg->tx_message;
+	/*
+	 * IPMI network function and command are encoded in the address
+	 * within the IPMI OpRegion; see ACPI 4.0, sec 5.5.2.4.3.
+	 */
+	msg->netfn = IPMI_OP_RGN_NETFN(address);
+	msg->cmd = IPMI_OP_RGN_CMD(address);
+	msg->data = tx_msg->tx_data;
+	/*
+	 * value is the parameter passed by the IPMI opregion space handler.
+	 * It points to the IPMI request message buffer
+	 */
+	buffer = (struct acpi_ipmi_buffer *)value;
+	/* copy the tx message data */
+	msg->data_len = buffer->length;
+	memcpy(tx_msg->tx_data, buffer->data, msg->data_len);
+	/*
+	 * now the default type is SYSTEM_INTERFACE and channel type is BMC.
+	 * If the netfn is APP_REQUEST and the cmd is SEND_MESSAGE,
+	 * the addr type should be changed to IPMB. Then we will have to parse
+	 * the IPMI request message buffer to get the IPMB address.
+	 * If so, please fix me.
+	 */
+	tx_msg->addr.addr_type = IPMI_SYSTEM_INTERFACE_ADDR_TYPE;
+	tx_msg->addr.channel = IPMI_BMC_CHANNEL;
+	tx_msg->addr.data[0] = 0;
+
+	/* Get the msgid */
+	device = tx_msg->device;
+	mutex_lock(&device->mutex_lock);
+	device->curr_msgid++;
+	tx_msg->tx_msgid = device->curr_msgid;
+	mutex_unlock(&device->mutex_lock);
+}
+
+static void acpi_format_ipmi_response(struct acpi_ipmi_msg *msg,
+		acpi_integer *value, int rem_time)
+{
+	struct acpi_ipmi_buffer *buffer;
+
+	/*
+	 * value is also used as output parameter. It represents the response
+	 * IPMI message returned by IPMI command.
+	 */
+	buffer = (struct acpi_ipmi_buffer *)value;
+	if (!rem_time && !msg->msg_done) {
+		buffer->status = ACPI_IPMI_TIMEOUT;
+		return;
+	}
+	/*
+	 * If the flag of msg_done is not set or the recv length is zero, it
+	 * means that the IPMI command is not executed correctly.
+	 * The status code will be ACPI_IPMI_UNKNOWN.
+	 */
+	if (!msg->msg_done || !msg->rx_len) {
+		buffer->status = ACPI_IPMI_UNKNOWN;
+		return;
+	}
+	/*
+	 * If the IPMI response message is obtained correctly, the status code
+	 * will be ACPI_IPMI_OK
+	 */
+	buffer->status = ACPI_IPMI_OK;
+	buffer->length = msg->rx_len;
+	memcpy(buffer->data, msg->rx_data, msg->rx_len);
+}
+
+static void ipmi_destroy_tx_msg(struct acpi_ipmi_device *ipmi)
+{
+	struct acpi_ipmi_msg *tx_msg, *temp;
+	int count = 20;
+	struct pnp_dev *pnp_dev = ipmi->pnp_dev;
+
+	list_for_each_entry_safe(tx_msg, temp, &ipmi->tx_msg_list, head) {
+		/* wake up the sleep thread on the Tx msg */
+		complete(&tx_msg->tx_complete);
+	}
+
+	/* wait for about 20 ticks to flush the tx message list */
+	while (count--) {
+		if (list_empty(&ipmi->tx_msg_list))
+			break;
+		schedule_timeout(1);
+	}
+	if (!list_empty(&ipmi->tx_msg_list))
+		dev_warn(&pnp_dev->dev, "tx msg list is not NULL\n");
+
+}
+
+static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data)
+{
+	struct acpi_ipmi_device *ipmi_device =
+			(struct acpi_ipmi_device *)user_msg_data;
+	int msg_found = 0;
+	struct acpi_ipmi_msg *tx_msg;
+	struct pnp_dev *pnp_dev = ipmi_device->pnp_dev;
+
+	if (msg->user != ipmi_device->user_interface) {
+		dev_warn(&pnp_dev->dev, "Incorrect IPMI user\n");
+		ipmi_free_recv_msg(msg);
+		return;
+	}
+	mutex_lock(&ipmi_device->mutex_lock);
+	list_for_each_entry(tx_msg, &ipmi_device->tx_msg_list, head) {
+		if (msg->msgid == tx_msg->tx_msgid) {
+			/* find the message id */
+			msg_found = 1;
+			break;
+		}
+	}
+
+	mutex_unlock(&ipmi_device->mutex_lock);
+	if (!msg_found) {
+		/* no matched msg is found . But we should free it */
+		ipmi_free_recv_msg(msg);
+		dev_warn(&pnp_dev->dev, "Incorrect MSG is found\n");
+		return;
+	}
+
+	if (msg->msg.data_len) {
+		/* copy the response data to Rx_data buffer */
+		memcpy(tx_msg->rx_data, msg->msg_data, msg->msg.data_len);
+		tx_msg->rx_len = msg->msg.data_len;
+		tx_msg->msg_done = 1;
+	}
+	complete(&tx_msg->tx_complete);
+	ipmi_free_recv_msg(msg);
+};
+
+static void ipmi_register_bmc(int iface, struct device *dev)
+{
+	struct acpi_ipmi_device *ipmi_device;
+	struct pnp_dev *device;
+	ipmi_user_t		user;
+	int err;
+
+	if (list_empty(&driver_data.ipmi_devices))
+		return;
+
+	list_for_each_entry(ipmi_device, &driver_data.ipmi_devices, head) {
+		if (ipmi_device->user_interface) {
+			/*
+			 * Only one user interface is allowed to be registered
+			 * for one IPMI device.
+			 * If we already create the user interface for
+			 * one IPMI device, skip it
+			 */
+			continue;
+		}
+		device = ipmi_device->pnp_dev;
+		if (dev == &device->dev) {
+			/*
+			 * If the dev is identical to the ACPI device,
+			 * create the user interface.
+			 */
+			err = ipmi_create_user(iface, &driver_data.ipmi_hndlrs,
+						ipmi_device, &user);
+			if (err == 0) {
+				ipmi_device->user_interface = user;
+				ipmi_device->ipmi_ifnum = iface;
+			}
+
+			break;
+		}
+	}
+}
+
+static void ipmi_bmc_gone(int iface)
+{
+	struct acpi_ipmi_device *ipmi_device, *temp;
+
+	list_for_each_entry_safe(ipmi_device, temp,
+				&driver_data.ipmi_devices, head) {
+		if (ipmi_device->user_interface &&
+			(ipmi_device->ipmi_ifnum == iface)) {
+			ipmi_destroy_user(ipmi_device->user_interface);
+			ipmi_device->user_interface = NULL;
+			ipmi_destroy_tx_msg(ipmi_device);
+			break;
+		}
+	}
+}
+/* --------------------------------------------------------------------------
+ *			Address Space Management
+   -------------------------------------------------------------------------- */
+/*
+ * This is the IPMI opregion space handler.
+ * @function: indicates the read/write. In fact as the IPMI message is driven
+ * by command, only write is meaningful.
+ * @address: This contains the netfn/command of IPMI request message.
+ * @bits   : not used.
+ * @value  : it is an in/out parameter. It points to the IPMI message buffer.
+ *	     Before the IPMI message is sent, it represents the actual request
+ *	     IPMI message. After the IPMI message is finished, it represents
+ *	     the response IPMI message returned by IPMI command.
+ * @handler_context: IPMI device context.
+ */
+
+static acpi_status
+acpi_ipmi_space_handler(u32 function, acpi_physical_address address,
+		      u32 bits, acpi_integer *value,
+		      void *handler_context, void *region_context)
+{
+	struct acpi_ipmi_msg *tx_msg;
+	struct acpi_ipmi_device *ipmi_device =
+			(struct acpi_ipmi_device *) handler_context;
+	int err;
+	acpi_status status;
+	/*
+	 * IPMI opregion message.
+	 * IPMI message is firstly written to the BMC and system software
+	 * can get the respsonse. So it is unmeaningful for the IPMI read
+	 * access.
+	 */
+	if ((function & ACPI_IO_MASK) == ACPI_READ)
+		return AE_TYPE;
+
+	if (!ipmi_device->user_interface)
+		return AE_NOT_EXIST;
+
+	tx_msg = acpi_alloc_ipmi_msg(ipmi_device);
+	if (!tx_msg)
+		return AE_NO_MEMORY;
+
+	acpi_format_ipmi_msg(tx_msg, address, value);
+	mutex_lock(&ipmi_device->mutex_lock);
+	list_add_tail(&tx_msg->head, &ipmi_device->tx_msg_list);
+	mutex_unlock(&ipmi_device->mutex_lock);
+	err = ipmi_request_settime(ipmi_device->user_interface,
+					&tx_msg->addr,
+					tx_msg->tx_msgid,
+					&tx_msg->tx_message,
+					NULL, 0, 0, 0);
+	if (err) {
+		status = AE_ERROR;
+		goto end_label;
+	}
+	err = wait_for_completion_timeout(&tx_msg->tx_complete, IPMI_TIMEOUT);
+	acpi_format_ipmi_response(tx_msg, value, err);
+	status = AE_OK;
+
+end_label:
+	mutex_lock(&ipmi_device->mutex_lock);
+	list_del(&tx_msg->head);
+	mutex_unlock(&ipmi_device->mutex_lock);
+	kfree(tx_msg);
+	return status;
+}
+
+static void ipmi_remove_handler(struct acpi_ipmi_device *ipmi)
+{
+	if (!test_bit(IPMI_FLAGS_HANDLER_INSTALL, &ipmi->flags))
+		return;
+
+	acpi_remove_address_space_handler(ipmi->handle,
+				ACPI_ADR_SPACE_IPMI, &acpi_ipmi_space_handler);
+
+	clear_bit(IPMI_FLAGS_HANDLER_INSTALL, &ipmi->flags);
+}
+
+static int ipmi_install_handler(struct acpi_ipmi_device *ipmi)
+{
+	acpi_status status;
+
+	if (test_bit(IPMI_FLAGS_HANDLER_INSTALL, &ipmi->flags))
+		return 0;
+
+	status = acpi_install_address_space_handler(ipmi->handle,
+						    ACPI_ADR_SPACE_IPMI,
+						    &acpi_ipmi_space_handler,
+						    NULL, ipmi);
+	if (ACPI_FAILURE(status)) {
+		struct pnp_dev *pnp_dev = ipmi->pnp_dev;
+		dev_warn(&pnp_dev->dev, "Can't register IPMI opregion for %s\n",
+					pnp_dev_name(pnp_dev));
+		return -EINVAL;
+	}
+	set_bit(IPMI_FLAGS_HANDLER_INSTALL, &ipmi->flags);
+	return 0;
+}
+
+
+static int acpi_add_ipmi_device(struct pnp_dev *pnp_dev)
+{
+	struct acpi_ipmi_device *ipmi_device;
+	struct acpi_device *acpi_dev;
+
+	/* check whether it is already registered */
+	list_for_each_entry(ipmi_device, &driver_data.ipmi_devices, head) {
+		if (ipmi_device->pnp_dev == pnp_dev) {
+			dev_warn(&pnp_dev->dev,
+				"device is already installed\n");
+			return NOTIFY_DONE;
+		}
+	}
+
+	ipmi_device = kzalloc(sizeof(*ipmi_device), GFP_KERNEL);
+	if (!ipmi_device)
+		return NOTIFY_DONE;
+
+	acpi_dev = pnp_acpi_device(pnp_dev);
+	INIT_LIST_HEAD(&ipmi_device->head);
+	ipmi_device->pnp_dev = pnp_dev;
+	ipmi_device->handle = acpi_dev->handle;
+	list_add_tail(&ipmi_device->head, &driver_data.ipmi_devices);
+
+	mutex_init(&ipmi_device->mutex_lock);
+	INIT_LIST_HEAD(&ipmi_device->tx_msg_list);
+	ipmi_install_handler(ipmi_device);
+
+	return NOTIFY_OK;
+}
+
+static int acpi_remove_ipmi_device(struct pnp_dev *pnp_dev)
+{
+	struct acpi_ipmi_device *ipmi_device, *temp;
+	int ret = NOTIFY_DONE;
+
+	list_for_each_entry_safe(ipmi_device, temp, &driver_data.ipmi_devices,
+					head) {
+		if (ipmi_device->pnp_dev != pnp_dev)
+			continue;
+
+		/*
+		 * If the IPMI user interface is created, it should be
+		 * destroyed.
+		 */
+		if (ipmi_device->user_interface) {
+			ipmi_destroy_user(ipmi_device->user_interface);
+			ipmi_device->user_interface = NULL;
+		}
+		list_del(&ipmi_device->head);
+
+		/* destroy the Tx_msg list */
+		if (!list_empty(&ipmi_device->tx_msg_list))
+			ipmi_destroy_tx_msg(ipmi_device);
+
+		ipmi_remove_handler(ipmi_device);
+		kfree(ipmi_device);
+		ret = NOTIFY_OK;
+		break;
+	}
+	return ret;
+}
+
+
+static int acpi_ipmi_notifier(struct notifier_block *nb,
+				       unsigned long event, void *data)
+{
+	struct pnp_dev *pnp_dev = data;
+	int ret = NOTIFY_DONE;
+
+	switch (event) {
+	case IPMI_PNP_ADD:
+		ret = acpi_add_ipmi_device(pnp_dev);
+		break;
+	case IPMI_PNP_REMOVE:
+		ret = acpi_remove_ipmi_device(pnp_dev);
+		break;
+	default:
+		dev_warn(&pnp_dev->dev, "Incorrect event\n");
+		break;
+	}
+	return ret;
+}
+
+static struct notifier_block acpi_ipmi_notifier_block = {
+	.notifier_call = acpi_ipmi_notifier,
+};
+
+static int __init acpi_ipmi_init(void)
+{
+	int result = 0;
+
+	if (acpi_disabled)
+		return result;
+
+	result = acpi_ipmi_notifier_register(&acpi_ipmi_notifier_block);
+	if (result)
+		return result;
+
+	result = ipmi_smi_watcher_register(&driver_data.bmc_events);
+
+	if (result)
+		acpi_ipmi_notifier_unregister(&acpi_ipmi_notifier_block);
+
+	return result;
+}
+
+static void __exit acpi_ipmi_exit(void)
+{
+	if (acpi_disabled)
+		return;
+
+	ipmi_smi_watcher_unregister(&driver_data.bmc_events);
+
+	acpi_ipmi_notifier_unregister(&acpi_ipmi_notifier_block);
+}
+
+module_init(acpi_ipmi_init);
+module_exit(acpi_ipmi_exit);
-- 
1.5.4.5

Re: [PATCH_v8 1/2] IPMI/ACPI: Define acpi_ipmi notifier hook functions

[Matthew Garrett]

On Mon, Jul 26, 2010 at 10:46:04PM +0800, yakui.zhao@intel.com wrote:
> @@ -2133,6 +2142,11 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
>  	if (!info)
>  		return -ENOMEM;
>  
> +	pnp_ipmi = kzalloc(sizeof(*pnp_ipmi), GFP_KERNEL);
> +	if (!pnp_ipmi) {
> +		kfree(info);
> +		return -ENOMEM;
> +	}

This will never get called if the hardware has a PCI IPMI interface.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH_v8 1/2] IPMI/ACPI: Define acpi_ipmi notifier hook functions

[Matthew Garrett]

Actually, ignore me - it will, it just won't be initialised.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH_v8 1/2] IPMI/ACPI: Define acpi_ipmi notifier hook functions

[Bjorn Helgaas]

On Monday, July 26, 2010 08:46:04 am yakui.zhao@intel.com wrote:
> From: Zhao yakui <yakui.zhao@intel.com>
> 

This needs a changelog.

This seems like a complicated solution to a simple problem.

I don't understand why the ACPI IPMI opregion stuff can't be made an
optional feature of the ACPI IPMI driver.  Trying to completely decouple
things is just going to add corner cases and weird dependencies.

Bjorn

> Signed-off-by: Zhao yakui <yakui.zhao@intel.com>
> cc: Bjorn Helgaas <bjorn.helgaas@hp.com>
> ---
>  drivers/char/ipmi/ipmi_si_intf.c |   63 ++++++++++++++++++++++++++++++++++++++
>  include/linux/ipmi.h             |    8 +++++
>  2 files changed, 71 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> index 094bdc3..91c5d37 100644
> --- a/drivers/char/ipmi/ipmi_si_intf.c
> +++ b/drivers/char/ipmi/ipmi_si_intf.c
> @@ -65,6 +65,7 @@
>  #include <linux/string.h>
>  #include <linux/ctype.h>
>  #include <linux/pnp.h>
> +#include <linux/ipmi.h>
>  
>  #ifdef CONFIG_PPC_OF
>  #include <linux/of_device.h>
> @@ -1907,6 +1908,13 @@ static __devinit void hardcode_find_bmc(void)
>   */
>  static int acpi_failure;
>  
> +static BLOCKING_NOTIFIER_HEAD(pnp_ipmi_notifier);
> +static LIST_HEAD(pnp_ipmi_list);
> +struct pnp_ipmi_device {
> +	struct list_head head;
> +	struct pnp_dev *pnp_dev;
> +};
> +
>  /* For GPE-type interrupts. */
>  static u32 ipmi_acpi_gpe(void *context)
>  {
> @@ -2124,6 +2132,7 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
>  	acpi_handle handle;
>  	acpi_status status;
>  	unsigned long long tmp;
> +	struct pnp_ipmi_device *pnp_ipmi;
>  
>  	acpi_dev = pnp_acpi_device(dev);
>  	if (!acpi_dev)
> @@ -2133,6 +2142,11 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
>  	if (!info)
>  		return -ENOMEM;
>  
> +	pnp_ipmi = kzalloc(sizeof(*pnp_ipmi), GFP_KERNEL);
> +	if (!pnp_ipmi) {
> +		kfree(info);
> +		return -ENOMEM;
> +	}
>  	info->addr_source = SI_ACPI;
>  	printk(KERN_INFO PFX "probing via ACPI\n");
>  
> @@ -2196,20 +2210,69 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
>  		 res, info->io.regsize, info->io.regspacing,
>  		 info->irq);
>  
> +	pnp_ipmi->pnp_dev = dev;
> +	list_add_tail(&pnp_ipmi->head, &pnp_ipmi_list);
> +	blocking_notifier_call_chain(&pnp_ipmi_notifier, IPMI_PNP_ADD,
> +				(void *)dev);
> +
>  	return add_smi(info);
>  
>  err_free:
>  	kfree(info);
> +	kfree(pnp_ipmi);
>  	return -EINVAL;
>  }
>  
>  static void __devexit ipmi_pnp_remove(struct pnp_dev *dev)
>  {
>  	struct smi_info *info = pnp_get_drvdata(dev);
> +	struct pnp_ipmi_device *pnp_ipmi;
> +
> +	list_for_each_entry(pnp_ipmi, &pnp_ipmi_list, head) {
> +		if (pnp_ipmi->pnp_dev == dev) {
> +			list_del(&pnp_ipmi->head);
> +			blocking_notifier_call_chain(&pnp_ipmi_notifier,
> +				IPMI_PNP_REMOVE, (void *)dev);
> +			break;
> +		}
> +	}
>  
>  	cleanup_one_si(info);
>  }
>  
> +int acpi_ipmi_notifier_register(struct notifier_block *nb)
> +{
> +	int ret;
> +	struct pnp_ipmi_device *pnp_ipmi;
> +
> +	ret = blocking_notifier_chain_register(&pnp_ipmi_notifier, nb);
> +	if (ret == 0) {
> +		/*
> +		 * Maybe we already get the corresponding pnp_ipmi_list before
> +		 * registering the notifier chain. So call the notifer
> +		 * chain list for every pnp_ipmi device.
> +		 */
> +		list_for_each_entry(pnp_ipmi, &pnp_ipmi_list, head) {
> +			blocking_notifier_call_chain(&pnp_ipmi_notifier,
> +				IPMI_PNP_ADD, (void *)(pnp_ipmi->pnp_dev));
> +		}
> +	}
> +	return ret;
> +}
> +EXPORT_SYMBOL(acpi_ipmi_notifier_register);
> +
> +int acpi_ipmi_notifier_unregister(struct notifier_block *nb)
> +{
> +	struct pnp_ipmi_device *pnp_ipmi;
> +
> +	list_for_each_entry(pnp_ipmi, &pnp_ipmi_list, head) {
> +		blocking_notifier_call_chain(&pnp_ipmi_notifier,
> +				IPMI_PNP_REMOVE, (void *)(pnp_ipmi->pnp_dev));
> +	}
> +	return blocking_notifier_chain_unregister(&pnp_ipmi_notifier, nb);
> +}
> +EXPORT_SYMBOL(acpi_ipmi_notifier_unregister);
> +
>  static const struct pnp_device_id pnp_dev_table[] = {
>  	{"IPI0001", 0},
>  	{"", 0},
> diff --git a/include/linux/ipmi.h b/include/linux/ipmi.h
> index 65aae34..4ea2a69 100644
> --- a/include/linux/ipmi.h
> +++ b/include/linux/ipmi.h
> @@ -694,4 +694,12 @@ struct ipmi_timing_parms {
>  #define IPMICTL_GET_MAINTENANCE_MODE_CMD	_IOR(IPMI_IOC_MAGIC, 30, int)
>  #define IPMICTL_SET_MAINTENANCE_MODE_CMD	_IOW(IPMI_IOC_MAGIC, 31, int)
>  
> +#ifdef CONFIG_ACPI
> +#define IPMI_PNP_ADD		1
> +#define IPMI_PNP_REMOVE		2
> +extern int acpi_ipmi_notifier_register(struct notifier_block *nb);
> +extern int acpi_ipmi_notifier_unregister(struct notifier_block *nb);
> +
> +#endif
> +
>  #endif /* __LINUX_IPMI_H */
> 

Re: [PATCH_v8 2/2] ACPI: Add the IPMI opregion driver to enable ACPI to access BMC controller

[Bjorn Helgaas]

On Monday, July 26, 2010 08:46:05 am yakui.zhao@intel.com wrote:
>  
>  	  To compile this driver as a module, choose M here:
>  	  the module will be called processor.
> +config ACPI_IPMI
> +	tristate "IPMI"
> +	depends on EXPERIMENTAL && IPMI_SI && IPMI_HANDLER
> +	default n
> +	help
> +	  This driver enables the ACPI to access the BMC controller. And it
> +	  uses the IPMI request/response message to communicate with BMC
> +	  controller, which can be found on on the server.
> +
> +	  To compile this driver as a module, choose M here:

I've mentioned this three times already.  Will you please fix it?
The sentence above is not complete.  It needs to say something like
this:

	To compile this driver as a module, choose M here:
	the module will be called acpi_ipmi.

There are a couple other comments below that I've mentioned in the
past, too, so don't stop reading here :-)

>  config ACPI_HOTPLUG_CPU
>  	bool
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index 6ee3316..5ca31f1 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -67,5 +67,6 @@ processor-y			+= processor_idle.o processor_thermal.o
>  processor-$(CONFIG_CPU_FREQ)	+= processor_perflib.o
>  
>  obj-$(CONFIG_ACPI_PROCESSOR_AGGREGATOR) += acpi_pad.o
> +obj-$(CONFIG_ACPI_IPMI)		+= acpi_ipmi.o
>  
>  obj-$(CONFIG_ACPI_APEI)		+= apei/
> diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c
> new file mode 100644
> index 0000000..f5a9412
> --- /dev/null
> +++ b/drivers/acpi/acpi_ipmi.c
> @@ -0,0 +1,548 @@
> +/*
> + *  ipmi.c - ACPI IPMI opregion
> + *
> + *  Copyright (C) 2010 Intel Corporation
> + *  Copyright (C) 2010 Zhao Yakui <yakui.zhao@intel.com>
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + *
> + *  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/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/types.h>
> +#include <linux/delay.h>
> +#include <linux/proc_fs.h>
> +#include <linux/seq_file.h>
> +#include <linux/interrupt.h>
> +#include <linux/list.h>
> +#include <linux/spinlock.h>
> +#include <linux/io.h>
> +#include <acpi/acpi_bus.h>
> +#include <acpi/acpi_drivers.h>
> +#include <linux/ipmi.h>
> +#include <linux/device.h>
> +#include <linux/pnp.h>
> +
> +MODULE_AUTHOR("Zhao Yakui");
> +MODULE_DESCRIPTION("ACPI IPMI Opregion driver");
> +MODULE_LICENSE("GPL");
> +
> +#define IPMI_FLAGS_HANDLER_INSTALL	0
> +
> +#define ACPI_IPMI_OK			0
> +#define ACPI_IPMI_TIMEOUT		0x10
> +#define ACPI_IPMI_UNKNOWN		0x07
> +/* the IPMI timeout is 5s */
> +#define IPMI_TIMEOUT			(5 * HZ)
> +
> +struct acpi_ipmi_device {
> +	/* the device list attached to driver_data.ipmi_devices */
> +	struct list_head head;
> +	/* the IPMI request message list */
> +	struct list_head tx_msg_list;
> +	acpi_handle handle;
> +	struct pnp_dev	*pnp_dev;
> +	ipmi_user_t	user_interface;
> +	struct mutex	mutex_lock;
> +	int ipmi_ifnum; /* IPMI interface number */
> +	long curr_msgid;
> +	unsigned long flags;
> +};
> +
> +struct ipmi_driver_data {
> +	int device_count;
> +	struct list_head	ipmi_devices;
> +	struct ipmi_smi_watcher	bmc_events;
> +	struct ipmi_user_hndl	ipmi_hndlrs;
> +};
> +
> +struct acpi_ipmi_msg {
> +	struct list_head head;
> +	/*
> +	 * General speaking the addr type should be SI_ADDR_TYPE. And
> +	 * the addr channel should be BMC.
> +	 * In fact it can also be IPMB type. But we will have to
> +	 * parse it from the Netfn command buffer. It is so complex
> +	 * that it is skipped.
> +	 */
> +	struct ipmi_addr addr;
> +	long tx_msgid;
> +	/* it is used to track whether the IPMI message is finished */
> +	struct completion tx_complete;
> +	struct kernel_ipmi_msg tx_message;
> +	int	msg_done;
> +	/* tx data . And copy it from ACPI object buffer */
> +	u8	tx_data[64];
> +	int	tx_len;
> +	u8	rx_data[64];
> +	int	rx_len;
> +	struct acpi_ipmi_device *device;
> +};
> +
> +/* IPMI request/response buffer per ACPI 4.0, sec 5.5.2.4.3.2 */
> +struct acpi_ipmi_buffer {
> +	u8 status;
> +	u8 length;
> +	u8 data[64];
> +};
> +
> +static void ipmi_register_bmc(int iface, struct device *dev);
> +static void ipmi_bmc_gone(int iface);
> +static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data);

Please reorder the function definitions below so you don't need
these forward declarations (if possible).

> +
> +static struct ipmi_driver_data driver_data = {
> +	.ipmi_devices = LIST_HEAD_INIT(driver_data.ipmi_devices),
> +	.bmc_events = {
> +		.owner = THIS_MODULE,
> +		.new_smi = ipmi_register_bmc,
> +		.smi_gone = ipmi_bmc_gone,
> +	},
> +	.ipmi_hndlrs = {
> +		.ipmi_recv_hndl = ipmi_msg_handler,
> +	},
> +};
> +
> +struct acpi_ipmi_msg *acpi_alloc_ipmi_msg(struct acpi_ipmi_device *ipmi)
> +{
> +	struct acpi_ipmi_msg *ipmi_msg;
> +	struct pnp_dev *pnp_dev = ipmi->pnp_dev;
> +
> +	ipmi_msg = kzalloc(sizeof(struct acpi_ipmi_msg), GFP_KERNEL);
> +	if (!ipmi_msg)	{
> +		dev_warn(&pnp_dev->dev, "Can't allocate memory for ipmi_msg\n");
> +		return NULL;
> +	}
> +	init_completion(&ipmi_msg->tx_complete);
> +	INIT_LIST_HEAD(&ipmi_msg->head);
> +	ipmi_msg->device = ipmi;
> +	return ipmi_msg;
> +}
> +
> +#define		IPMI_OP_RGN_NETFN(offset)	((offset >> 8) & 0xff)
> +#define		IPMI_OP_RGN_CMD(offset)		(offset & 0xff)
> +static void acpi_format_ipmi_msg(struct acpi_ipmi_msg *tx_msg,
> +				acpi_physical_address address,
> +				acpi_integer *value)
> +{
> +	struct kernel_ipmi_msg *msg;
> +	struct acpi_ipmi_buffer *buffer;
> +	struct acpi_ipmi_device *device;
> +
> +	msg = &tx_msg->tx_message;
> +	/*
> +	 * IPMI network function and command are encoded in the address
> +	 * within the IPMI OpRegion; see ACPI 4.0, sec 5.5.2.4.3.
> +	 */
> +	msg->netfn = IPMI_OP_RGN_NETFN(address);
> +	msg->cmd = IPMI_OP_RGN_CMD(address);
> +	msg->data = tx_msg->tx_data;
> +	/*
> +	 * value is the parameter passed by the IPMI opregion space handler.
> +	 * It points to the IPMI request message buffer
> +	 */
> +	buffer = (struct acpi_ipmi_buffer *)value;
> +	/* copy the tx message data */
> +	msg->data_len = buffer->length;
> +	memcpy(tx_msg->tx_data, buffer->data, msg->data_len);
> +	/*
> +	 * now the default type is SYSTEM_INTERFACE and channel type is BMC.
> +	 * If the netfn is APP_REQUEST and the cmd is SEND_MESSAGE,
> +	 * the addr type should be changed to IPMB. Then we will have to parse
> +	 * the IPMI request message buffer to get the IPMB address.
> +	 * If so, please fix me.
> +	 */
> +	tx_msg->addr.addr_type = IPMI_SYSTEM_INTERFACE_ADDR_TYPE;
> +	tx_msg->addr.channel = IPMI_BMC_CHANNEL;
> +	tx_msg->addr.data[0] = 0;
> +
> +	/* Get the msgid */
> +	device = tx_msg->device;
> +	mutex_lock(&device->mutex_lock);
> +	device->curr_msgid++;
> +	tx_msg->tx_msgid = device->curr_msgid;
> +	mutex_unlock(&device->mutex_lock);
> +}
> +
> +static void acpi_format_ipmi_response(struct acpi_ipmi_msg *msg,
> +		acpi_integer *value, int rem_time)
> +{
> +	struct acpi_ipmi_buffer *buffer;
> +
> +	/*
> +	 * value is also used as output parameter. It represents the response
> +	 * IPMI message returned by IPMI command.
> +	 */
> +	buffer = (struct acpi_ipmi_buffer *)value;
> +	if (!rem_time && !msg->msg_done) {
> +		buffer->status = ACPI_IPMI_TIMEOUT;
> +		return;
> +	}
> +	/*
> +	 * If the flag of msg_done is not set or the recv length is zero, it
> +	 * means that the IPMI command is not executed correctly.
> +	 * The status code will be ACPI_IPMI_UNKNOWN.
> +	 */
> +	if (!msg->msg_done || !msg->rx_len) {
> +		buffer->status = ACPI_IPMI_UNKNOWN;
> +		return;
> +	}
> +	/*
> +	 * If the IPMI response message is obtained correctly, the status code
> +	 * will be ACPI_IPMI_OK
> +	 */
> +	buffer->status = ACPI_IPMI_OK;
> +	buffer->length = msg->rx_len;
> +	memcpy(buffer->data, msg->rx_data, msg->rx_len);
> +}
> +
> +static void ipmi_destroy_tx_msg(struct acpi_ipmi_device *ipmi)
> +{
> +	struct acpi_ipmi_msg *tx_msg, *temp;
> +	int count = 20;
> +	struct pnp_dev *pnp_dev = ipmi->pnp_dev;
> +
> +	list_for_each_entry_safe(tx_msg, temp, &ipmi->tx_msg_list, head) {
> +		/* wake up the sleep thread on the Tx msg */
> +		complete(&tx_msg->tx_complete);
> +	}
> +
> +	/* wait for about 20 ticks to flush the tx message list */
> +	while (count--) {
> +		if (list_empty(&ipmi->tx_msg_list))
> +			break;
> +		schedule_timeout(1);

I mentioned this before, too.  This timeout is meaningless because
you don't know how long a tick is.  Please change this to something
that is related to the tick length.  For example, compute the number
of ticks using HZ or msec_to_jiffies().

> +	}
> +	if (!list_empty(&ipmi->tx_msg_list))
> +		dev_warn(&pnp_dev->dev, "tx msg list is not NULL\n");
> +

Remove this blank line.

> +}
> +
> +static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data)
> +{
> +	struct acpi_ipmi_device *ipmi_device =
> +			(struct acpi_ipmi_device *)user_msg_data;

You don't need a cast here because "user_msg_data" is a "void *".

> +	int msg_found = 0;
> +	struct acpi_ipmi_msg *tx_msg;
> +	struct pnp_dev *pnp_dev = ipmi_device->pnp_dev;
> +
> +	if (msg->user != ipmi_device->user_interface) {
> +		dev_warn(&pnp_dev->dev, "Incorrect IPMI user\n");
> +		ipmi_free_recv_msg(msg);
> +		return;
> +	}
> +	mutex_lock(&ipmi_device->mutex_lock);
> +	list_for_each_entry(tx_msg, &ipmi_device->tx_msg_list, head) {
> +		if (msg->msgid == tx_msg->tx_msgid) {
> +			/* find the message id */
> +			msg_found = 1;
> +			break;
> +		}
> +	}
> +
> +	mutex_unlock(&ipmi_device->mutex_lock);
> +	if (!msg_found) {
> +		/* no matched msg is found . But we should free it */
> +		ipmi_free_recv_msg(msg);
> +		dev_warn(&pnp_dev->dev, "Incorrect MSG is found\n");

Is there a MSG ID or something you can include in the warning message?

> +		return;
> +	}
> +
> +	if (msg->msg.data_len) {
> +		/* copy the response data to Rx_data buffer */
> +		memcpy(tx_msg->rx_data, msg->msg_data, msg->msg.data_len);
> +		tx_msg->rx_len = msg->msg.data_len;
> +		tx_msg->msg_done = 1;
> +	}
> +	complete(&tx_msg->tx_complete);
> +	ipmi_free_recv_msg(msg);
> +};
> +
> +static void ipmi_register_bmc(int iface, struct device *dev)
> +{
> +	struct acpi_ipmi_device *ipmi_device;
> +	struct pnp_dev *device;
> +	ipmi_user_t		user;
> +	int err;
> +
> +	if (list_empty(&driver_data.ipmi_devices))
> +		return;

You don't need to check the list for empty before using
list_for_each_entry().

> +
> +	list_for_each_entry(ipmi_device, &driver_data.ipmi_devices, head) {
> +		if (ipmi_device->user_interface) {
> +			/*
> +			 * Only one user interface is allowed to be registered
> +			 * for one IPMI device.
> +			 * If we already create the user interface for
> +			 * one IPMI device, skip it
> +			 */
> +			continue;
> +		}
> +		device = ipmi_device->pnp_dev;
> +		if (dev == &device->dev) {
> +			/*
> +			 * If the dev is identical to the ACPI device,
> +			 * create the user interface.
> +			 */
> +			err = ipmi_create_user(iface, &driver_data.ipmi_hndlrs,
> +						ipmi_device, &user);
> +			if (err == 0) {
> +				ipmi_device->user_interface = user;
> +				ipmi_device->ipmi_ifnum = iface;
> +			}
> +
> +			break;
> +		}
> +	}
> +}
> +
> +static void ipmi_bmc_gone(int iface)
> +{
> +	struct acpi_ipmi_device *ipmi_device, *temp;
> +
> +	list_for_each_entry_safe(ipmi_device, temp,
> +				&driver_data.ipmi_devices, head) {
> +		if (ipmi_device->user_interface &&
> +			(ipmi_device->ipmi_ifnum == iface)) {
> +			ipmi_destroy_user(ipmi_device->user_interface);
> +			ipmi_device->user_interface = NULL;
> +			ipmi_destroy_tx_msg(ipmi_device);
> +			break;
> +		}
> +	}
> +}
> +/* --------------------------------------------------------------------------
> + *			Address Space Management
> +   -------------------------------------------------------------------------- */
> +/*
> + * This is the IPMI opregion space handler.
> + * @function: indicates the read/write. In fact as the IPMI message is driven
> + * by command, only write is meaningful.
> + * @address: This contains the netfn/command of IPMI request message.
> + * @bits   : not used.
> + * @value  : it is an in/out parameter. It points to the IPMI message buffer.
> + *	     Before the IPMI message is sent, it represents the actual request
> + *	     IPMI message. After the IPMI message is finished, it represents
> + *	     the response IPMI message returned by IPMI command.
> + * @handler_context: IPMI device context.
> + */
> +
> +static acpi_status
> +acpi_ipmi_space_handler(u32 function, acpi_physical_address address,
> +		      u32 bits, acpi_integer *value,
> +		      void *handler_context, void *region_context)
> +{
> +	struct acpi_ipmi_msg *tx_msg;
> +	struct acpi_ipmi_device *ipmi_device =
> +			(struct acpi_ipmi_device *) handler_context;

You don't need a cast here, since "handler_context" is a "void *".

> +	int err;
> +	acpi_status status;
> +	/*
> +	 * IPMI opregion message.
> +	 * IPMI message is firstly written to the BMC and system software
> +	 * can get the respsonse. So it is unmeaningful for the IPMI read
> +	 * access.
> +	 */
> +	if ((function & ACPI_IO_MASK) == ACPI_READ)
> +		return AE_TYPE;
> +
> +	if (!ipmi_device->user_interface)
> +		return AE_NOT_EXIST;
> +
> +	tx_msg = acpi_alloc_ipmi_msg(ipmi_device);
> +	if (!tx_msg)
> +		return AE_NO_MEMORY;
> +
> +	acpi_format_ipmi_msg(tx_msg, address, value);
> +	mutex_lock(&ipmi_device->mutex_lock);
> +	list_add_tail(&tx_msg->head, &ipmi_device->tx_msg_list);
> +	mutex_unlock(&ipmi_device->mutex_lock);
> +	err = ipmi_request_settime(ipmi_device->user_interface,
> +					&tx_msg->addr,
> +					tx_msg->tx_msgid,
> +					&tx_msg->tx_message,
> +					NULL, 0, 0, 0);
> +	if (err) {
> +		status = AE_ERROR;
> +		goto end_label;
> +	}
> +	err = wait_for_completion_timeout(&tx_msg->tx_complete, IPMI_TIMEOUT);
> +	acpi_format_ipmi_response(tx_msg, value, err);
> +	status = AE_OK;
> +
> +end_label:
> +	mutex_lock(&ipmi_device->mutex_lock);
> +	list_del(&tx_msg->head);
> +	mutex_unlock(&ipmi_device->mutex_lock);
> +	kfree(tx_msg);
> +	return status;
> +}
> +
> +static void ipmi_remove_handler(struct acpi_ipmi_device *ipmi)
> +{
> +	if (!test_bit(IPMI_FLAGS_HANDLER_INSTALL, &ipmi->flags))
> +		return;
> +
> +	acpi_remove_address_space_handler(ipmi->handle,
> +				ACPI_ADR_SPACE_IPMI, &acpi_ipmi_space_handler);
> +
> +	clear_bit(IPMI_FLAGS_HANDLER_INSTALL, &ipmi->flags);
> +}
> +
> +static int ipmi_install_handler(struct acpi_ipmi_device *ipmi)
> +{
> +	acpi_status status;
> +
> +	if (test_bit(IPMI_FLAGS_HANDLER_INSTALL, &ipmi->flags))
> +		return 0;
> +
> +	status = acpi_install_address_space_handler(ipmi->handle,
> +						    ACPI_ADR_SPACE_IPMI,
> +						    &acpi_ipmi_space_handler,
> +						    NULL, ipmi);
> +	if (ACPI_FAILURE(status)) {
> +		struct pnp_dev *pnp_dev = ipmi->pnp_dev;
> +		dev_warn(&pnp_dev->dev, "Can't register IPMI opregion for %s\n",
> +					pnp_dev_name(pnp_dev));
> +		return -EINVAL;
> +	}
> +	set_bit(IPMI_FLAGS_HANDLER_INSTALL, &ipmi->flags);
> +	return 0;
> +}
> +
> +
> +static int acpi_add_ipmi_device(struct pnp_dev *pnp_dev)
> +{
> +	struct acpi_ipmi_device *ipmi_device;
> +	struct acpi_device *acpi_dev;
> +
> +	/* check whether it is already registered */
> +	list_for_each_entry(ipmi_device, &driver_data.ipmi_devices, head) {
> +		if (ipmi_device->pnp_dev == pnp_dev) {
> +			dev_warn(&pnp_dev->dev,
> +				"device is already installed\n");
> +			return NOTIFY_DONE;
> +		}
> +	}
> +
> +	ipmi_device = kzalloc(sizeof(*ipmi_device), GFP_KERNEL);
> +	if (!ipmi_device)
> +		return NOTIFY_DONE;
> +
> +	acpi_dev = pnp_acpi_device(pnp_dev);
> +	INIT_LIST_HEAD(&ipmi_device->head);
> +	ipmi_device->pnp_dev = pnp_dev;
> +	ipmi_device->handle = acpi_dev->handle;
> +	list_add_tail(&ipmi_device->head, &driver_data.ipmi_devices);
> +
> +	mutex_init(&ipmi_device->mutex_lock);
> +	INIT_LIST_HEAD(&ipmi_device->tx_msg_list);
> +	ipmi_install_handler(ipmi_device);
> +
> +	return NOTIFY_OK;
> +}
> +
> +static int acpi_remove_ipmi_device(struct pnp_dev *pnp_dev)
> +{
> +	struct acpi_ipmi_device *ipmi_device, *temp;
> +	int ret = NOTIFY_DONE;
> +
> +	list_for_each_entry_safe(ipmi_device, temp, &driver_data.ipmi_devices,
> +					head) {
> +		if (ipmi_device->pnp_dev != pnp_dev)
> +			continue;
> +
> +		/*
> +		 * If the IPMI user interface is created, it should be
> +		 * destroyed.
> +		 */
> +		if (ipmi_device->user_interface) {
> +			ipmi_destroy_user(ipmi_device->user_interface);
> +			ipmi_device->user_interface = NULL;
> +		}
> +		list_del(&ipmi_device->head);
> +
> +		/* destroy the Tx_msg list */
> +		if (!list_empty(&ipmi_device->tx_msg_list))
> +			ipmi_destroy_tx_msg(ipmi_device);
> +
> +		ipmi_remove_handler(ipmi_device);
> +		kfree(ipmi_device);
> +		ret = NOTIFY_OK;
> +		break;
> +	}
> +	return ret;
> +}
> +
> +
> +static int acpi_ipmi_notifier(struct notifier_block *nb,
> +				       unsigned long event, void *data)
> +{
> +	struct pnp_dev *pnp_dev = data;
> +	int ret = NOTIFY_DONE;
> +
> +	switch (event) {
> +	case IPMI_PNP_ADD:
> +		ret = acpi_add_ipmi_device(pnp_dev);
> +		break;
> +	case IPMI_PNP_REMOVE:
> +		ret = acpi_remove_ipmi_device(pnp_dev);
> +		break;
> +	default:
> +		dev_warn(&pnp_dev->dev, "Incorrect event\n");

Please print the invalid event value here.

> +		break;
> +	}
> +	return ret;
> +}
> +
> +static struct notifier_block acpi_ipmi_notifier_block = {
> +	.notifier_call = acpi_ipmi_notifier,
> +};
> +
> +static int __init acpi_ipmi_init(void)
> +{
> +	int result = 0;
> +
> +	if (acpi_disabled)
> +		return result;
> +
> +	result = acpi_ipmi_notifier_register(&acpi_ipmi_notifier_block);
> +	if (result)
> +		return result;
> +
> +	result = ipmi_smi_watcher_register(&driver_data.bmc_events);
> +
> +	if (result)
> +		acpi_ipmi_notifier_unregister(&acpi_ipmi_notifier_block);
> +
> +	return result;
> +}
> +
> +static void __exit acpi_ipmi_exit(void)
> +{
> +	if (acpi_disabled)
> +		return;
> +
> +	ipmi_smi_watcher_unregister(&driver_data.bmc_events);
> +
> +	acpi_ipmi_notifier_unregister(&acpi_ipmi_notifier_block);
> +}
> +
> +module_init(acpi_ipmi_init);
> +module_exit(acpi_ipmi_exit);
> 

Re: [PATCH_v8 1/2] IPMI/ACPI: Define acpi_ipmi notifier hook functions

[ykzhao]

On Tue, 2010-07-27 at 00:52 +0800, Bjorn Helgaas wrote:
> On Monday, July 26, 2010 08:46:04 am yakui.zhao@intel.com wrote:
> > From: Zhao yakui <yakui.zhao@intel.com>
> > 
> 
> This needs a changelog.

OK. I will try to add more changelog about this patch.

> 
> This seems like a complicated solution to a simple problem.

Do you have any simple idea to fix the issue?

Yes. It will be very simple to merge the IPMI opregion into the
drivers/char/ipmi/ipmi_si_intf.c 
But in fact the a lot of IPMI opregion code has nothing related with the
IPMI detection. Maybe it is appropriate to separate the IPMI opregion
code into the separated file. BTW: This is also the Corey's viewpoint.

So IMO the next discussion had better be based on the condition that the
IPMI opregion code is put into the separated file.

> 
> I don't understand why the ACPI IPMI opregion stuff can't be made an
> optional feature of the ACPI IPMI driver.  Trying to completely decouple
> things is just going to add corner cases and weird dependencies.

Can the following patch handle the "hot-plug" case of PNP IPI0001 as
what you mentioned in last thread?

> 
> Bjorn
> 
> > Signed-off-by: Zhao yakui <yakui.zhao@intel.com>
> > cc: Bjorn Helgaas <bjorn.helgaas@hp.com>
> > ---
> >  drivers/char/ipmi/ipmi_si_intf.c |   63 ++++++++++++++++++++++++++++++++++++++
> >  include/linux/ipmi.h             |    8 +++++
> >  2 files changed, 71 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> > index 094bdc3..91c5d37 100644
> > --- a/drivers/char/ipmi/ipmi_si_intf.c
> > +++ b/drivers/char/ipmi/ipmi_si_intf.c
> > @@ -65,6 +65,7 @@
> >  #include <linux/string.h>
> >  #include <linux/ctype.h>
> >  #include <linux/pnp.h>
> > +#include <linux/ipmi.h>
> >  
> >  #ifdef CONFIG_PPC_OF
> >  #include <linux/of_device.h>
> > @@ -1907,6 +1908,13 @@ static __devinit void hardcode_find_bmc(void)
> >   */
> >  static int acpi_failure;
> >  
> > +static BLOCKING_NOTIFIER_HEAD(pnp_ipmi_notifier);
> > +static LIST_HEAD(pnp_ipmi_list);
> > +struct pnp_ipmi_device {
> > +	struct list_head head;
> > +	struct pnp_dev *pnp_dev;
> > +};
> > +
> >  /* For GPE-type interrupts. */
> >  static u32 ipmi_acpi_gpe(void *context)
> >  {
> > @@ -2124,6 +2132,7 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
> >  	acpi_handle handle;
> >  	acpi_status status;
> >  	unsigned long long tmp;
> > +	struct pnp_ipmi_device *pnp_ipmi;
> >  
> >  	acpi_dev = pnp_acpi_device(dev);
> >  	if (!acpi_dev)
> > @@ -2133,6 +2142,11 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
> >  	if (!info)
> >  		return -ENOMEM;
> >  
> > +	pnp_ipmi = kzalloc(sizeof(*pnp_ipmi), GFP_KERNEL);
> > +	if (!pnp_ipmi) {
> > +		kfree(info);
> > +		return -ENOMEM;
> > +	}
> >  	info->addr_source = SI_ACPI;
> >  	printk(KERN_INFO PFX "probing via ACPI\n");
> >  
> > @@ -2196,20 +2210,69 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
> >  		 res, info->io.regsize, info->io.regspacing,
> >  		 info->irq);
> >  
> > +	pnp_ipmi->pnp_dev = dev;
> > +	list_add_tail(&pnp_ipmi->head, &pnp_ipmi_list);
> > +	blocking_notifier_call_chain(&pnp_ipmi_notifier, IPMI_PNP_ADD,
> > +				(void *)dev);
> > +
> >  	return add_smi(info);
> >  
> >  err_free:
> >  	kfree(info);
> > +	kfree(pnp_ipmi);
> >  	return -EINVAL;
> >  }
> >  
> >  static void __devexit ipmi_pnp_remove(struct pnp_dev *dev)
> >  {
> >  	struct smi_info *info = pnp_get_drvdata(dev);
> > +	struct pnp_ipmi_device *pnp_ipmi;
> > +
> > +	list_for_each_entry(pnp_ipmi, &pnp_ipmi_list, head) {
> > +		if (pnp_ipmi->pnp_dev == dev) {
> > +			list_del(&pnp_ipmi->head);
> > +			blocking_notifier_call_chain(&pnp_ipmi_notifier,
> > +				IPMI_PNP_REMOVE, (void *)dev);
> > +			break;
> > +		}
> > +	}
> >  
> >  	cleanup_one_si(info);
> >  }
> >  
> > +int acpi_ipmi_notifier_register(struct notifier_block *nb)
> > +{
> > +	int ret;
> > +	struct pnp_ipmi_device *pnp_ipmi;
> > +
> > +	ret = blocking_notifier_chain_register(&pnp_ipmi_notifier, nb);
> > +	if (ret == 0) {
> > +		/*
> > +		 * Maybe we already get the corresponding pnp_ipmi_list before
> > +		 * registering the notifier chain. So call the notifer
> > +		 * chain list for every pnp_ipmi device.
> > +		 */
> > +		list_for_each_entry(pnp_ipmi, &pnp_ipmi_list, head) {
> > +			blocking_notifier_call_chain(&pnp_ipmi_notifier,
> > +				IPMI_PNP_ADD, (void *)(pnp_ipmi->pnp_dev));
> > +		}
> > +	}
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL(acpi_ipmi_notifier_register);
> > +
> > +int acpi_ipmi_notifier_unregister(struct notifier_block *nb)
> > +{
> > +	struct pnp_ipmi_device *pnp_ipmi;
> > +
> > +	list_for_each_entry(pnp_ipmi, &pnp_ipmi_list, head) {
> > +		blocking_notifier_call_chain(&pnp_ipmi_notifier,
> > +				IPMI_PNP_REMOVE, (void *)(pnp_ipmi->pnp_dev));
> > +	}
> > +	return blocking_notifier_chain_unregister(&pnp_ipmi_notifier, nb);
> > +}
> > +EXPORT_SYMBOL(acpi_ipmi_notifier_unregister);
> > +
> >  static const struct pnp_device_id pnp_dev_table[] = {
> >  	{"IPI0001", 0},
> >  	{"", 0},
> > diff --git a/include/linux/ipmi.h b/include/linux/ipmi.h
> > index 65aae34..4ea2a69 100644
> > --- a/include/linux/ipmi.h
> > +++ b/include/linux/ipmi.h
> > @@ -694,4 +694,12 @@ struct ipmi_timing_parms {
> >  #define IPMICTL_GET_MAINTENANCE_MODE_CMD	_IOR(IPMI_IOC_MAGIC, 30, int)
> >  #define IPMICTL_SET_MAINTENANCE_MODE_CMD	_IOW(IPMI_IOC_MAGIC, 31, int)
> >  
> > +#ifdef CONFIG_ACPI
> > +#define IPMI_PNP_ADD		1
> > +#define IPMI_PNP_REMOVE		2
> > +extern int acpi_ipmi_notifier_register(struct notifier_block *nb);
> > +extern int acpi_ipmi_notifier_unregister(struct notifier_block *nb);
> > +
> > +#endif
> > +
> >  #endif /* __LINUX_IPMI_H */
> > 


------------------------------------------------------------------------------
The Palm PDK Hot Apps Program offers developers who use the
Plug-In Development Kit to bring their C/C++ apps to Palm for a share 
of $1 Million in cash or HP Products. Visit us here for more details:
http://ad.doubleclick.net/clk;226879339;13503038;l?
http://clk.atdmt.com/CRS/go/247765532/direct/01/
_______________________________________________
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer

Re: [PATCH_v8 2/2] ACPI: Add the IPMI opregion driver to enable ACPI to access BMC controller

[ykzhao]

On Tue, 2010-07-27 at 00:54 +0800, Bjorn Helgaas wrote:
> On Monday, July 26, 2010 08:46:05 am yakui.zhao@intel.com wrote:
> >
> >         To compile this driver as a module, choose M here:
> >         the module will be called processor.
> > +config ACPI_IPMI
> > +     tristate "IPMI"
> > +     depends on EXPERIMENTAL && IPMI_SI && IPMI_HANDLER
> > +     default n
> > +     help
> > +       This driver enables the ACPI to access the BMC controller. And it
> > +       uses the IPMI request/response message to communicate with BMC
> > +       controller, which can be found on on the server.
> > +
> > +       To compile this driver as a module, choose M here:
> 
> I've mentioned this three times already.  Will you please fix it?
> The sentence above is not complete.  It needs to say something like
> this:

Sorry that I miss the above comment as I am focused on the discussion of
hook function.

> 
>         To compile this driver as a module, choose M here:
>         the module will be called acpi_ipmi.
> 
> There are a couple other comments below that I've mentioned in the
> past, too, so don't stop reading here :-)
> 
> >  config ACPI_HOTPLUG_CPU
> >       bool
> > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> > index 6ee3316..5ca31f1 100644
> > --- a/drivers/acpi/Makefile
> > +++ b/drivers/acpi/Makefile
> > @@ -67,5 +67,6 @@ processor-y                 += processor_idle.o processor_thermal.o
> >  processor-$(CONFIG_CPU_FREQ) += processor_perflib.o
> >
> >  obj-$(CONFIG_ACPI_PROCESSOR_AGGREGATOR) += acpi_pad.o
> > +obj-$(CONFIG_ACPI_IPMI)              += acpi_ipmi.o
> >
> >  obj-$(CONFIG_ACPI_APEI)              += apei/
> > diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c
> > new file mode 100644
> > index 0000000..f5a9412
> > --- /dev/null
> > +++ b/drivers/acpi/acpi_ipmi.c
> > @@ -0,0 +1,548 @@
> > +/*
> > + *  ipmi.c - ACPI IPMI opregion
> > + *
> > + *  Copyright (C) 2010 Intel Corporation
> > + *  Copyright (C) 2010 Zhao Yakui <yakui.zhao@intel.com>
> > + *
> > + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > + *
> > + *  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/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/init.h>
> > +#include <linux/types.h>
> > +#include <linux/delay.h>
> > +#include <linux/proc_fs.h>
> > +#include <linux/seq_file.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/list.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/io.h>
> > +#include <acpi/acpi_bus.h>
> > +#include <acpi/acpi_drivers.h>
> > +#include <linux/ipmi.h>
> > +#include <linux/device.h>
> > +#include <linux/pnp.h>
> > +
> > +MODULE_AUTHOR("Zhao Yakui");
> > +MODULE_DESCRIPTION("ACPI IPMI Opregion driver");
> > +MODULE_LICENSE("GPL");
> > +
> > +#define IPMI_FLAGS_HANDLER_INSTALL   0
> > +
> > +#define ACPI_IPMI_OK                 0
> > +#define ACPI_IPMI_TIMEOUT            0x10
> > +#define ACPI_IPMI_UNKNOWN            0x07
> > +/* the IPMI timeout is 5s */
> > +#define IPMI_TIMEOUT                 (5 * HZ)
> > +
> > +struct acpi_ipmi_device {
> > +     /* the device list attached to driver_data.ipmi_devices */
> > +     struct list_head head;
> > +     /* the IPMI request message list */
> > +     struct list_head tx_msg_list;
> > +     acpi_handle handle;
> > +     struct pnp_dev  *pnp_dev;
> > +     ipmi_user_t     user_interface;
> > +     struct mutex    mutex_lock;
> > +     int ipmi_ifnum; /* IPMI interface number */
> > +     long curr_msgid;
> > +     unsigned long flags;
> > +};
> > +
> > +struct ipmi_driver_data {
> > +     int device_count;
> > +     struct list_head        ipmi_devices;
> > +     struct ipmi_smi_watcher bmc_events;
> > +     struct ipmi_user_hndl   ipmi_hndlrs;
> > +};
> > +
> > +struct acpi_ipmi_msg {
> > +     struct list_head head;
> > +     /*
> > +      * General speaking the addr type should be SI_ADDR_TYPE. And
> > +      * the addr channel should be BMC.
> > +      * In fact it can also be IPMB type. But we will have to
> > +      * parse it from the Netfn command buffer. It is so complex
> > +      * that it is skipped.
> > +      */
> > +     struct ipmi_addr addr;
> > +     long tx_msgid;
> > +     /* it is used to track whether the IPMI message is finished */
> > +     struct completion tx_complete;
> > +     struct kernel_ipmi_msg tx_message;
> > +     int     msg_done;
> > +     /* tx data . And copy it from ACPI object buffer */
> > +     u8      tx_data[64];
> > +     int     tx_len;
> > +     u8      rx_data[64];
> > +     int     rx_len;
> > +     struct acpi_ipmi_device *device;
> > +};
> > +
> > +/* IPMI request/response buffer per ACPI 4.0, sec 5.5.2.4.3.2 */
> > +struct acpi_ipmi_buffer {
> > +     u8 status;
> > +     u8 length;
> > +     u8 data[64];
> > +};
> > +
> > +static void ipmi_register_bmc(int iface, struct device *dev);
> > +static void ipmi_bmc_gone(int iface);
> > +static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data);
> 
> Please reorder the function definitions below so you don't need
> these forward declarations (if possible).

thanks for the comment. 

I tried what you mentioned. But it will fail in compilation without the
forward declarations. So I use the forward declaration as used in other
code.(For example: drivers/hwmon/ibmaem.c)

> 
> > +
> > +static struct ipmi_driver_data driver_data = {
> > +     .ipmi_devices = LIST_HEAD_INIT(driver_data.ipmi_devices),
> > +     .bmc_events = {
> > +             .owner = THIS_MODULE,
> > +             .new_smi = ipmi_register_bmc,
> > +             .smi_gone = ipmi_bmc_gone,
> > +     },
> > +     .ipmi_hndlrs = {
> > +             .ipmi_recv_hndl = ipmi_msg_handler,
> > +     },
> > +};
> > +
> > +struct acpi_ipmi_msg *acpi_alloc_ipmi_msg(struct acpi_ipmi_device *ipmi)
> > +{
> > +     struct acpi_ipmi_msg *ipmi_msg;
> > +     struct pnp_dev *pnp_dev = ipmi->pnp_dev;
> > +
> > +     ipmi_msg = kzalloc(sizeof(struct acpi_ipmi_msg), GFP_KERNEL);
> > +     if (!ipmi_msg)  {
> > +             dev_warn(&pnp_dev->dev, "Can't allocate memory for ipmi_msg\n");
> > +             return NULL;
> > +     }
> > +     init_completion(&ipmi_msg->tx_complete);
> > +     INIT_LIST_HEAD(&ipmi_msg->head);
> > +     ipmi_msg->device = ipmi;
> > +     return ipmi_msg;
> > +}
> > +
> > +#define              IPMI_OP_RGN_NETFN(offset)       ((offset >> 8) & 0xff)
> > +#define              IPMI_OP_RGN_CMD(offset)         (offset & 0xff)
> > +static void acpi_format_ipmi_msg(struct acpi_ipmi_msg *tx_msg,
> > +                             acpi_physical_address address,
> > +                             acpi_integer *value)
> > +{
> > +     struct kernel_ipmi_msg *msg;
> > +     struct acpi_ipmi_buffer *buffer;
> > +     struct acpi_ipmi_device *device;
> > +
> > +     msg = &tx_msg->tx_message;
> > +     /*
> > +      * IPMI network function and command are encoded in the address
> > +      * within the IPMI OpRegion; see ACPI 4.0, sec 5.5.2.4.3.
> > +      */
> > +     msg->netfn = IPMI_OP_RGN_NETFN(address);
> > +     msg->cmd = IPMI_OP_RGN_CMD(address);
> > +     msg->data = tx_msg->tx_data;
> > +     /*
> > +      * value is the parameter passed by the IPMI opregion space handler.
> > +      * It points to the IPMI request message buffer
> > +      */
> > +     buffer = (struct acpi_ipmi_buffer *)value;
> > +     /* copy the tx message data */
> > +     msg->data_len = buffer->length;
> > +     memcpy(tx_msg->tx_data, buffer->data, msg->data_len);
> > +     /*
> > +      * now the default type is SYSTEM_INTERFACE and channel type is BMC.
> > +      * If the netfn is APP_REQUEST and the cmd is SEND_MESSAGE,
> > +      * the addr type should be changed to IPMB. Then we will have to parse
> > +      * the IPMI request message buffer to get the IPMB address.
> > +      * If so, please fix me.
> > +      */
> > +     tx_msg->addr.addr_type = IPMI_SYSTEM_INTERFACE_ADDR_TYPE;
> > +     tx_msg->addr.channel = IPMI_BMC_CHANNEL;
> > +     tx_msg->addr.data[0] = 0;
> > +
> > +     /* Get the msgid */
> > +     device = tx_msg->device;
> > +     mutex_lock(&device->mutex_lock);
> > +     device->curr_msgid++;
> > +     tx_msg->tx_msgid = device->curr_msgid;
> > +     mutex_unlock(&device->mutex_lock);
> > +}
> > +
> > +static void acpi_format_ipmi_response(struct acpi_ipmi_msg *msg,
> > +             acpi_integer *value, int rem_time)
> > +{
> > +     struct acpi_ipmi_buffer *buffer;
> > +
> > +     /*
> > +      * value is also used as output parameter. It represents the response
> > +      * IPMI message returned by IPMI command.
> > +      */
> > +     buffer = (struct acpi_ipmi_buffer *)value;
> > +     if (!rem_time && !msg->msg_done) {
> > +             buffer->status = ACPI_IPMI_TIMEOUT;
> > +             return;
> > +     }
> > +     /*
> > +      * If the flag of msg_done is not set or the recv length is zero, it
> > +      * means that the IPMI command is not executed correctly.
> > +      * The status code will be ACPI_IPMI_UNKNOWN.
> > +      */
> > +     if (!msg->msg_done || !msg->rx_len) {
> > +             buffer->status = ACPI_IPMI_UNKNOWN;
> > +             return;
> > +     }
> > +     /*
> > +      * If the IPMI response message is obtained correctly, the status code
> > +      * will be ACPI_IPMI_OK
> > +      */
> > +     buffer->status = ACPI_IPMI_OK;
> > +     buffer->length = msg->rx_len;
> > +     memcpy(buffer->data, msg->rx_data, msg->rx_len);
> > +}
> > +
> > +static void ipmi_destroy_tx_msg(struct acpi_ipmi_device *ipmi)
> > +{
> > +     struct acpi_ipmi_msg *tx_msg, *temp;
> > +     int count = 20;
> > +     struct pnp_dev *pnp_dev = ipmi->pnp_dev;
> > +
> > +     list_for_each_entry_safe(tx_msg, temp, &ipmi->tx_msg_list, head) {
> > +             /* wake up the sleep thread on the Tx msg */
> > +             complete(&tx_msg->tx_complete);
> > +     }
> > +
> > +     /* wait for about 20 ticks to flush the tx message list */
> > +     while (count--) {
> > +             if (list_empty(&ipmi->tx_msg_list))
> > +                     break;
> > +             schedule_timeout(1);
> 
> I mentioned this before, too.  This timeout is meaningless because
> you don't know how long a tick is.  Please change this to something
> that is related to the tick length.  For example, compute the number
> of ticks using HZ or msec_to_jiffies().
> 

OK. I will add one time definition and then transfer it to the
corresponding jiffies.


> > +     }
> > +     if (!list_empty(&ipmi->tx_msg_list))
> > +             dev_warn(&pnp_dev->dev, "tx msg list is not NULL\n");
> > +
> 
> Remove this blank line.
> 
> > +}
> > +
> > +static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data)
> > +{
> > +     struct acpi_ipmi_device *ipmi_device =
> > +                     (struct acpi_ipmi_device *)user_msg_data;
> 
> You don't need a cast here because "user_msg_data" is a "void *".
> 
> > +     int msg_found = 0;
> > +     struct acpi_ipmi_msg *tx_msg;
> > +     struct pnp_dev *pnp_dev = ipmi_device->pnp_dev;
> > +
> > +     if (msg->user != ipmi_device->user_interface) {
> > +             dev_warn(&pnp_dev->dev, "Incorrect IPMI user\n");
> > +             ipmi_free_recv_msg(msg);
> > +             return;
> > +     }
> > +     mutex_lock(&ipmi_device->mutex_lock);
> > +     list_for_each_entry(tx_msg, &ipmi_device->tx_msg_list, head) {
> > +             if (msg->msgid == tx_msg->tx_msgid) {
> > +                     /* find the message id */
> > +                     msg_found = 1;
> > +                     break;
> > +             }
> > +     }
> > +
> > +     mutex_unlock(&ipmi_device->mutex_lock);
> > +     if (!msg_found) {
> > +             /* no matched msg is found . But we should free it */
> > +             ipmi_free_recv_msg(msg);
> > +             dev_warn(&pnp_dev->dev, "Incorrect MSG is found\n");
> 
> Is there a MSG ID or something you can include in the warning message?
> 
> > +             return;
> > +     }
> > +
> > +     if (msg->msg.data_len) {
> > +             /* copy the response data to Rx_data buffer */
> > +             memcpy(tx_msg->rx_data, msg->msg_data, msg->msg.data_len);
> > +             tx_msg->rx_len = msg->msg.data_len;
> > +             tx_msg->msg_done = 1;
> > +     }
> > +     complete(&tx_msg->tx_complete);
> > +     ipmi_free_recv_msg(msg);
> > +};
> > +
> > +static void ipmi_register_bmc(int iface, struct device *dev)
> > +{
> > +     struct acpi_ipmi_device *ipmi_device;
> > +     struct pnp_dev *device;
> > +     ipmi_user_t             user;
> > +     int err;
> > +
> > +     if (list_empty(&driver_data.ipmi_devices))
> > +             return;
> 
> You don't need to check the list for empty before using
> list_for_each_entry().

OK. I will delete it.

> 
> > +
> > +     list_for_each_entry(ipmi_device, &driver_data.ipmi_devices, head) {
> > +             if (ipmi_device->user_interface) {
> > +                     /*
> > +                      * Only one user interface is allowed to be registered
> > +                      * for one IPMI device.
> > +                      * If we already create the user interface for
> > +                      * one IPMI device, skip it
> > +                      */
> > +                     continue;
> > +             }
> > +             device = ipmi_device->pnp_dev;
> > +             if (dev == &device->dev) {
> > +                     /*
> > +                      * If the dev is identical to the ACPI device,
> > +                      * create the user interface.
> > +                      */
> > +                     err = ipmi_create_user(iface, &driver_data.ipmi_hndlrs,
> > +                                             ipmi_device, &user);
> > +                     if (err == 0) {
> > +                             ipmi_device->user_interface = user;
> > +                             ipmi_device->ipmi_ifnum = iface;
> > +                     }
> > +
> > +                     break;
> > +             }
> > +     }
> > +}
> > +
> > +static void ipmi_bmc_gone(int iface)
> > +{
> > +     struct acpi_ipmi_device *ipmi_device, *temp;
> > +
> > +     list_for_each_entry_safe(ipmi_device, temp,
> > +                             &driver_data.ipmi_devices, head) {
> > +             if (ipmi_device->user_interface &&
> > +                     (ipmi_device->ipmi_ifnum == iface)) {
> > +                     ipmi_destroy_user(ipmi_device->user_interface);
> > +                     ipmi_device->user_interface = NULL;
> > +                     ipmi_destroy_tx_msg(ipmi_device);
> > +                     break;
> > +             }
> > +     }
> > +}
> > +/* --------------------------------------------------------------------------
> > + *                   Address Space Management
> > +   -------------------------------------------------------------------------- */
> > +/*
> > + * This is the IPMI opregion space handler.
> > + * @function: indicates the read/write. In fact as the IPMI message is driven
> > + * by command, only write is meaningful.
> > + * @address: This contains the netfn/command of IPMI request message.
> > + * @bits   : not used.
> > + * @value  : it is an in/out parameter. It points to the IPMI message buffer.
> > + *        Before the IPMI message is sent, it represents the actual request
> > + *        IPMI message. After the IPMI message is finished, it represents
> > + *        the response IPMI message returned by IPMI command.
> > + * @handler_context: IPMI device context.
> > + */
> > +
> > +static acpi_status
> > +acpi_ipmi_space_handler(u32 function, acpi_physical_address address,
> > +                   u32 bits, acpi_integer *value,
> > +                   void *handler_context, void *region_context)
> > +{
> > +     struct acpi_ipmi_msg *tx_msg;
> > +     struct acpi_ipmi_device *ipmi_device =
> > +                     (struct acpi_ipmi_device *) handler_context;
> 
> You don't need a cast here, since "handler_context" is a "void *".

OK. I can delete the redundant cast transform. In fact IMO it will be
easy to understand. 

> 
> > +     int err;
> > +     acpi_status status;
> > +     /*
> > +      * IPMI opregion message.
> > +      * IPMI message is firstly written to the BMC and system software
> > +      * can get the respsonse. So it is unmeaningful for the IPMI read
> > +      * access.
> > +      */
> > +     if ((function & ACPI_IO_MASK) == ACPI_READ)
> > +             return AE_TYPE;
> > +
> > +     if (!ipmi_device->user_interface)
> > +             return AE_NOT_EXIST;
> > +
> > +     tx_msg = acpi_alloc_ipmi_msg(ipmi_device);
> > +     if (!tx_msg)
> > +             return AE_NO_MEMORY;
> > +
> > +     acpi_format_ipmi_msg(tx_msg, address, value);
> > +     mutex_lock(&ipmi_device->mutex_lock);
> > +     list_add_tail(&tx_msg->head, &ipmi_device->tx_msg_list);
> > +     mutex_unlock(&ipmi_device->mutex_lock);
> > +     err = ipmi_request_settime(ipmi_device->user_interface,
> > +                                     &tx_msg->addr,
> > +                                     tx_msg->tx_msgid,
> > +                                     &tx_msg->tx_message,
> > +                                     NULL, 0, 0, 0);
> > +     if (err) {
> > +             status = AE_ERROR;
> > +             goto end_label;
> > +     }
> > +     err = wait_for_completion_timeout(&tx_msg->tx_complete, IPMI_TIMEOUT);
> > +     acpi_format_ipmi_response(tx_msg, value, err);
> > +     status = AE_OK;
> > +
> > +end_label:
> > +     mutex_lock(&ipmi_device->mutex_lock);
> > +     list_del(&tx_msg->head);
> > +     mutex_unlock(&ipmi_device->mutex_lock);
> > +     kfree(tx_msg);
> > +     return status;
> > +}
> > +
> > +static void ipmi_remove_handler(struct acpi_ipmi_device *ipmi)
> > +{
> > +     if (!test_bit(IPMI_FLAGS_HANDLER_INSTALL, &ipmi->flags))
> > +             return;
> > +
> > +     acpi_remove_address_space_handler(ipmi->handle,
> > +                             ACPI_ADR_SPACE_IPMI, &acpi_ipmi_space_handler);
> > +
> > +     clear_bit(IPMI_FLAGS_HANDLER_INSTALL, &ipmi->flags);
> > +}
> > +
> > +static int ipmi_install_handler(struct acpi_ipmi_device *ipmi)
> > +{
> > +     acpi_status status;
> > +
> > +     if (test_bit(IPMI_FLAGS_HANDLER_INSTALL, &ipmi->flags))
> > +             return 0;
> > +
> > +     status = acpi_install_address_space_handler(ipmi->handle,
> > +                                                 ACPI_ADR_SPACE_IPMI,
> > +                                                 &acpi_ipmi_space_handler,
> > +                                                 NULL, ipmi);
> > +     if (ACPI_FAILURE(status)) {
> > +             struct pnp_dev *pnp_dev = ipmi->pnp_dev;
> > +             dev_warn(&pnp_dev->dev, "Can't register IPMI opregion for %s\n",
> > +                                     pnp_dev_name(pnp_dev));
> > +             return -EINVAL;
> > +     }
> > +     set_bit(IPMI_FLAGS_HANDLER_INSTALL, &ipmi->flags);
> > +     return 0;
> > +}
> > +
> > +
> > +static int acpi_add_ipmi_device(struct pnp_dev *pnp_dev)
> > +{
> > +     struct acpi_ipmi_device *ipmi_device;
> > +     struct acpi_device *acpi_dev;
> > +
> > +     /* check whether it is already registered */
> > +     list_for_each_entry(ipmi_device, &driver_data.ipmi_devices, head) {
> > +             if (ipmi_device->pnp_dev == pnp_dev) {
> > +                     dev_warn(&pnp_dev->dev,
> > +                             "device is already installed\n");
> > +                     return NOTIFY_DONE;
> > +             }
> > +     }
> > +
> > +     ipmi_device = kzalloc(sizeof(*ipmi_device), GFP_KERNEL);
> > +     if (!ipmi_device)
> > +             return NOTIFY_DONE;
> > +
> > +     acpi_dev = pnp_acpi_device(pnp_dev);
> > +     INIT_LIST_HEAD(&ipmi_device->head);
> > +     ipmi_device->pnp_dev = pnp_dev;
> > +     ipmi_device->handle = acpi_dev->handle;
> > +     list_add_tail(&ipmi_device->head, &driver_data.ipmi_devices);
> > +
> > +     mutex_init(&ipmi_device->mutex_lock);
> > +     INIT_LIST_HEAD(&ipmi_device->tx_msg_list);
> > +     ipmi_install_handler(ipmi_device);
> > +
> > +     return NOTIFY_OK;
> > +}
> > +
> > +static int acpi_remove_ipmi_device(struct pnp_dev *pnp_dev)
> > +{
> > +     struct acpi_ipmi_device *ipmi_device, *temp;
> > +     int ret = NOTIFY_DONE;
> > +
> > +     list_for_each_entry_safe(ipmi_device, temp, &driver_data.ipmi_devices,
> > +                                     head) {
> > +             if (ipmi_device->pnp_dev != pnp_dev)
> > +                     continue;
> > +
> > +             /*
> > +              * If the IPMI user interface is created, it should be
> > +              * destroyed.
> > +              */
> > +             if (ipmi_device->user_interface) {
> > +                     ipmi_destroy_user(ipmi_device->user_interface);
> > +                     ipmi_device->user_interface = NULL;
> > +             }
> > +             list_del(&ipmi_device->head);
> > +
> > +             /* destroy the Tx_msg list */
> > +             if (!list_empty(&ipmi_device->tx_msg_list))
> > +                     ipmi_destroy_tx_msg(ipmi_device);
> > +
> > +             ipmi_remove_handler(ipmi_device);
> > +             kfree(ipmi_device);
> > +             ret = NOTIFY_OK;
> > +             break;
> > +     }
> > +     return ret;
> > +}
> > +
> > +
> > +static int acpi_ipmi_notifier(struct notifier_block *nb,
> > +                                    unsigned long event, void *data)
> > +{
> > +     struct pnp_dev *pnp_dev = data;
> > +     int ret = NOTIFY_DONE;
> > +
> > +     switch (event) {
> > +     case IPMI_PNP_ADD:
> > +             ret = acpi_add_ipmi_device(pnp_dev);
> > +             break;
> > +     case IPMI_PNP_REMOVE:
> > +             ret = acpi_remove_ipmi_device(pnp_dev);
> > +             break;
> > +     default:
> > +             dev_warn(&pnp_dev->dev, "Incorrect event\n");
> 
> Please print the invalid event value here.

OK. I will add it.

> 
> > +             break;
> > +     }
> > +     return ret;
> > +}
> > +
> > +static struct notifier_block acpi_ipmi_notifier_block = {
> > +     .notifier_call = acpi_ipmi_notifier,
> > +};
> > +
> > +static int __init acpi_ipmi_init(void)
> > +{
> > +     int result = 0;
> > +
> > +     if (acpi_disabled)
> > +             return result;
> > +
> > +     result = acpi_ipmi_notifier_register(&acpi_ipmi_notifier_block);
> > +     if (result)
> > +             return result;
> > +
> > +     result = ipmi_smi_watcher_register(&driver_data.bmc_events);
> > +
> > +     if (result)
> > +             acpi_ipmi_notifier_unregister(&acpi_ipmi_notifier_block);
> > +
> > +     return result;
> > +}
> > +
> > +static void __exit acpi_ipmi_exit(void)
> > +{
> > +     if (acpi_disabled)
> > +             return;
> > +
> > +     ipmi_smi_watcher_unregister(&driver_data.bmc_events);
> > +
> > +     acpi_ipmi_notifier_unregister(&acpi_ipmi_notifier_block);
> > +}
> > +
> > +module_init(acpi_ipmi_init);
> > +module_exit(acpi_ipmi_exit);
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH_v8 1/2] IPMI/ACPI: Define acpi_ipmi notifier hook functions

[Corey Minyard]

On 07/26/2010 11:52 AM, Bjorn Helgaas wrote:
> On Monday, July 26, 2010 08:46:04 am yakui.zhao@intel.com wrote:
>    
>> From: Zhao yakui<yakui.zhao@intel.com>
>>
>>      
> This needs a changelog.
>
> This seems like a complicated solution to a simple problem.
>
> I don't understand why the ACPI IPMI opregion stuff can't be made an
> optional feature of the ACPI IPMI driver.  Trying to completely decouple
> things is just going to add corner cases and weird dependencies.
>    
Well, ipmi_si_intf is pretty darn big as it is.  I would like to break 
it up.

I spent a little time reading the ACPI spec to understand just what the 
heck this thing is.  So now my head hurts a little.  But from what I can 
tell, this provides a way for the ACPI system to specify operations that 
are done by sending IPMI messages.  For instance, if power control was 
done via IPMI, the various control methods for power control would work 
their way down the to access to opregion interfaces mapping to IPMI 
functions, and that's where this piece of code takes over.

If so, this code has nothing to do with the IPMI system interface.  It's 
really more ACPI than IPMI.  It's sending messages at the very top of 
the IPMI stack, where it should.  The only reason it cares about ipmi_si 
at all is the discovery of the IPMI interfaces.

I agree that a notifier framework seems like massive overkill for this 
interface.  I will note that there are already interfaces for 
registering to receive callbacks when an IPMI device is added or 
removed.  What's missing is a way to ask "Is this an ACPI PNP device?".

Since this same function will be needed for IPMI SMBus interfaces, if 
that ever becomes a reality in the kernel, it seems more reasonable to 
provide some type of addition to the IPMI interface to be able to store 
this in the low-level code and retrieve this from the IPMI user 
interface.  So you can use the standard mechanisms to watch for devices 
being added, and then query to see if they are PNP at that point.

Does that make sense?

-corey

Re: [PATCH_v8 1/2] IPMI/ACPI: Define acpi_ipmi notifier hook functions

[Matthew Garrett]

On Mon, Jul 26, 2010 at 08:19:10PM -0500, Corey Minyard wrote:

> I agree that a notifier framework seems like massive overkill for this  
> interface.  I will note that there are already interfaces for  
> registering to receive callbacks when an IPMI device is added or  
> removed.  What's missing is a way to ask "Is this an ACPI PNP device?".

So we just want something that returns addr_source, along with moving 
that enum into a global header?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH_v8 1/2] IPMI/ACPI: Define acpi_ipmi notifier hook functions

[Corey Minyard]

On 07/27/2010 08:22 AM, Matthew Garrett wrote:
> On Mon, Jul 26, 2010 at 08:19:10PM -0500, Corey Minyard wrote:
>
>    
>> I agree that a notifier framework seems like massive overkill for this
>> interface.  I will note that there are already interfaces for
>> registering to receive callbacks when an IPMI device is added or
>> removed.  What's missing is a way to ask "Is this an ACPI PNP device?".
>>      
> So we just want something that returns addr_source, along with moving
> that enum into a global header?
>
>    

Yes, I think that makes sense.  A new function in struct 
ipmi_smi_handlers and a new interface in ipmi.h/ipmi_msghandler.c.  And 
move ipmi_addr_src into ipmi.h.

-corey

Re: [PATCH_v8 1/2] IPMI/ACPI: Define acpi_ipmi notifier hook functions

[ykzhao]

On Tue, 2010-07-27 at 09:19 +0800, Corey Minyard wrote:
> On 07/26/2010 11:52 AM, Bjorn Helgaas wrote:
> > On Monday, July 26, 2010 08:46:04 am yakui.zhao@intel.com wrote:
> >    
> >> From: Zhao yakui<yakui.zhao@intel.com>
> >>
> >>      
> > This needs a changelog.
> >
> > This seems like a complicated solution to a simple problem.
> >
> > I don't understand why the ACPI IPMI opregion stuff can't be made an
> > optional feature of the ACPI IPMI driver.  Trying to completely decouple
> > things is just going to add corner cases and weird dependencies.
> >    
> Well, ipmi_si_intf is pretty darn big as it is.  I would like to break 
> it up.
> 
> I spent a little time reading the ACPI spec to understand just what the 
> heck this thing is.  So now my head hurts a little.  But from what I can 
> tell, this provides a way for the ACPI system to specify operations that 
> are done by sending IPMI messages.  For instance, if power control was 
> done via IPMI, the various control methods for power control would work 
> their way down the to access to opregion interfaces mapping to IPMI 
> functions, and that's where this piece of code takes over.

> If so, this code has nothing to do with the IPMI system interface.  It's 
> really more ACPI than IPMI.  It's sending messages at the very top of 
> the IPMI stack, where it should.  The only reason it cares about ipmi_si 
> at all is the discovery of the IPMI interfaces.

Yes. Agree with what you said.
The trouble in the patch set is to confirm whether the IPMI device is
what ACPI expects to communicate especially when there exist multiple
IPMI devices.

> 
> I agree that a notifier framework seems like massive overkill for this 
> interface.  I will note that there are already interfaces for 
> registering to receive callbacks when an IPMI device is added or 
> removed.  What's missing is a way to ask "Is this an ACPI PNP device?".

Not sure whether the ipmi_smi_watcher is the interfaces you mentioned?

The ipmi_smi_watcher is already used in the second patch in order to
create the user interface. But now the callback function of new_smi only
provides very limited info about the IPMI device. In order to assure
that ACPI can communicate with the IPMI device, I have to compare the
corresponding device pointer to confirm whether this is what ACPI want
to communicate. Then I tried the several mechanisms to make it work
while touch IPMI code/structure as little as possible.
     >manually discover the pnp device with the pnpid of "IPI0001"
     >using one notifier in course of loading IPMI pnp device driver.
     >Merge the opregion code into the PNP IPMI discovery code.
     
> 
> Since this same function will be needed for IPMI SMBus interfaces, if 
> that ever becomes a reality in the kernel, it seems more reasonable to 
> provide some type of addition to the IPMI interface to be able to store 
> this in the low-level code and retrieve this from the IPMI user 
> interface.  So you can use the standard mechanisms to watch for devices 
> being added, and then query to see if they are PNP at that point.
> 
> Does that make sense?

Yes. If this interface can be available, it will be easier for me to
write the patch to enable that the ACPI can communicate with the BMC. 

Best regards.
   Yakui

> -corey

Re: [PATCH_v8 1/2] IPMI/ACPI: Define acpi_ipmi notifier hook functions

[Corey Minyard]

On 07/27/2010 08:09 PM, ykzhao wrote:
>
>> I agree that a notifier framework seems like massive overkill for this
>> interface.  I will note that there are already interfaces for
>> registering to receive callbacks when an IPMI device is added or
>> removed.  What's missing is a way to ask "Is this an ACPI PNP device?".
>>      
> Not sure whether the ipmi_smi_watcher is the interfaces you mentioned?
>
> The ipmi_smi_watcher is already used in the second patch in order to
> create the user interface. But now the callback function of new_smi only
> provides very limited info about the IPMI device. In order to assure
> that ACPI can communicate with the IPMI device, I have to compare the
> corresponding device pointer to confirm whether this is what ACPI want
> to communicate. Then I tried the several mechanisms to make it work
> while touch IPMI code/structure as little as possible.
>       >manually discover the pnp device with the pnpid of "IPI0001"
>       >using one notifier in course of loading IPMI pnp device driver.
>       >Merge the opregion code into the PNP IPMI discovery code.
>    
Yes ipmi_smi_watcher is the interface, and you obviously have to use 
that to catch the interfaces as the come and go.

>
>    
>> Since this same function will be needed for IPMI SMBus interfaces, if
>> that ever becomes a reality in the kernel, it seems more reasonable to
>> provide some type of addition to the IPMI interface to be able to store
>> this in the low-level code and retrieve this from the IPMI user
>> interface.  So you can use the standard mechanisms to watch for devices
>> being added, and then query to see if they are PNP at that point.
>>
>> Does that make sense?
>>      
> Yes. If this interface can be available, it will be easier for me to
> write the patch to enable that the ACPI can communicate with the BMC.
>    
I was actually hoping you would provide that interface :).  I'm not sure 
exactly what you need, but it's not that hard, really.  A new function 
in ipmi.h and ipmi_msghandler.c which calls a new function pointer in 
struct ipmi_smi_handlers (in ipmi_smi.h).  ipmi_si.c would tie into that 
new function pointer, and I believe all the data is already available there.

-corey

------------------------------------------------------------------------------
The Palm PDK Hot Apps Program offers developers who use the
Plug-In Development Kit to bring their C/C++ apps to Palm for a share 
of $1 Million in cash or HP Products. Visit us here for more details:
http://ad.doubleclick.net/clk;226879339;13503038;l?
http://clk.atdmt.com/CRS/go/247765532/direct/01/