[PATCH 1/3] ipmi/acpi: Fix the building error in ipmi module

[PATCH 1/3] ipmi/acpi: Fix the building error in ipmi module

[yakui.zhao]

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

When the IPMI module is selected as module, it will complain the
following building error.
>ERROR: "pnpacpi_protocol" [drivers/char/ipmi/ipmi_si.ko] undefined!

Fix the above building error.

Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
cc: Bjorn Helgaas <bjorn.helgaas@hp.com>
---
 drivers/pnp/pnpacpi/core.c |    7 +++++++
 include/linux/pnp.h        |   11 +++--------
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/pnp/pnpacpi/core.c b/drivers/pnp/pnpacpi/core.c
index 8dd0f37..8eebbe4 100644
--- a/drivers/pnp/pnpacpi/core.c
+++ b/drivers/pnp/pnpacpi/core.c
@@ -155,6 +155,13 @@ struct pnp_protocol pnpacpi_protocol = {
 #endif
 };
 
+struct acpi_device *pnp_acpi_device(struct pnp_dev *dev)
+{
+	if (dev->protocol == &pnpacpi_protocol)
+		return dev->data;
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(pnp_acpi_device);
 static int __init pnpacpi_add_device(struct acpi_device *device)
 {
 	acpi_handle temp = NULL;
diff --git a/include/linux/pnp.h b/include/linux/pnp.h
index 7c4193e..0ae2c64 100644
--- a/include/linux/pnp.h
+++ b/include/linux/pnp.h
@@ -335,16 +335,11 @@ extern struct pnp_protocol pnpbios_protocol;
 #endif
 
 #ifdef CONFIG_PNPACPI
-extern struct pnp_protocol pnpacpi_protocol;
 
-static inline struct acpi_device *pnp_acpi_device(struct pnp_dev *dev)
-{
-	if (dev->protocol == &pnpacpi_protocol)
-		return dev->data;
-	return NULL;
-}
+extern struct acpi_device *pnp_acpi_device(struct pnp_dev *dev);
 #else
-#define pnp_acpi_device(dev) 0
+static inline struct acpi_device *pnp_acpi_device(struct pnp_dev *dev)
+{ return NULL; }
 #endif
 
 /* status */
-- 
1.5.4.5

[PATCH 2/3] ipmi/acpi: use ACPI detection mechanism firstly to detect IPMI system interface

[yakui.zhao]

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

Sometimes one IPMI system interface will be detected by several methods.
For example: ACPI mechanism, SPMI table, DMI or hardcode mechanism.
In such case when one IPMI system interface can be detected in two mechanism,
the second mechanism will fail in the detection and can't record which IPMI
system interface is detected by it.

Use the ACPI detection mechanism firstly to detect the IPMI system interface
so that we can know which IPMI system interface is detected in ACPI namespace.

Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
cc: Bjorn Helgaas <bjorn.helgaas@hp.com>
---
 drivers/char/ipmi/ipmi_si_intf.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 679cd08..99fecd2 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -3195,6 +3195,10 @@ static __devinit int init_ipmi_si(void)
 
 	printk(KERN_INFO "IPMI System Interface driver.\n");
 
+#ifdef CONFIG_PNP
+	pnp_register_driver(&ipmi_pnp_driver);
+#endif
+
 	hardcode_find_bmc();
 
 #ifdef CONFIG_DMI
@@ -3204,9 +3208,6 @@ static __devinit int init_ipmi_si(void)
 #ifdef CONFIG_ACPI
 	spmi_find_bmc();
 #endif
-#ifdef CONFIG_PNP
-	pnp_register_driver(&ipmi_pnp_driver);
-#endif
 
 #ifdef CONFIG_PCI
 	rv = pci_register_driver(&ipmi_pci_driver);
-- 
1.5.4.5

[PATCH 3/3] ipmi/acpi: Install the IPMI space handler to enable ACPI to access the BMC controller

[yakui.zhao]

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

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.

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.

Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
cc: Bjorn Helgaas <bjorn.helgaas@hp.com>
---
 drivers/char/ipmi/ipmi_si_intf.c |  505 +++++++++++++++++++++++++++++++++++++-
 1 files changed, 503 insertions(+), 2 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 99fecd2..9c1e212 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -65,7 +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>
 #include <linux/of_platform.h>
@@ -1808,6 +1808,477 @@ static __devinit void hardcode_find_bmc(void)
 
 #include <linux/acpi.h>
 
+#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 30s */
+#define IPMI_TIMEOUT			(30 * HZ)
+
+struct acpi_ipmi_device {
+	acpi_handle handle;
+	struct acpi_device *device;
+	/* the device list attached to driver_data.ipmi_devices */
+	struct list_head head;
+	ipmi_user_t 	user_interface;
+	struct mutex	mutex_lock;
+	/* the IPMI request message list */
+	struct list_head tx_msg_list;
+	long curr_msgid;
+	/* IPMI flags */
+	unsigned long flags;
+	/* IPMI interface number */
+	int ipmi_ifnum;
+};
+
+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 {
+	/* message list */
+	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;
+	/* tx message id */
+	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;
+	/* get the response data */
+	u8	rx_data[64];
+	/* the response length. The netfn & cmd is excluded. */
+	int	rx_len;
+	struct acpi_ipmi_device *device;
+};
+
+/*
+ * IPMI request/response buffer.
+ * The length is 66 bytes.
+ */
+struct acpi_ipmi_buffer {
+	/* status code of a given IPMI command */
+	u8 status_code;
+	/* the length of the payload */
+	u8 length;
+	/*
+	 * the payload. Before the operation is carried out, it represents the
+	 * request message payload. After the operation is carried out, it
+	 * stores the response message returned by IPMI command.
+	 */
+	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,
+	},
+};
+
+static
+struct acpi_ipmi_msg *acpi_alloc_ipmi_msg(struct acpi_ipmi_device *ipmi)
+{
+	struct acpi_ipmi_msg *ipmi_msg;
+
+	ipmi_msg = kzalloc(sizeof(struct acpi_ipmi_msg), GFP_KERNEL);
+	if (!ipmi_msg)	{
+		printk(KERN_DEBUG "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;
+}
+
+static void acpi_format_ipmi_msg(struct acpi_ipmi_msg *tx_msg,
+		acpi_physical_address address,
+		acpi_integer *value)
+{
+	struct kernel_ipmi_msg *msg;
+	u8	temp_value;
+	struct acpi_ipmi_buffer *buffer;
+	struct acpi_ipmi_device *device;
+
+	msg = &tx_msg->tx_message;
+	/* get the netfn */
+	temp_value = (address >> 8) & 0xff;
+	msg->netfn = temp_value;
+	/* get the command */
+	temp_value = address & 0xff;
+	msg->cmd = temp_value;
+	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.
+	 */
+	tx_msg->addr.addr_type = IPMI_SYSTEM_INTERFACE_ADDR_TYPE;
+	tx_msg->addr.channel = IPMI_BMC_CHANNEL;
+	tx_msg->addr.data[0] = 0;
+
+	/*
+	 * If the netfn is APP_REQUEST and the cmd is SEND_MESSAGE, we should
+	 * parse the IPMI request message buffer to get the IPMB address.
+	 * If so, please fix me.
+	 */
+
+	/* 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 timeout)
+{
+	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;
+	/* when timeout is zero, it means that the timeout happens */
+	if (!timeout) {
+		/* the status code is ACPI_IPMI_TIMEOUT */
+		buffer->status_code = ACPI_IPMI_TIMEOUT;
+		return;
+	}
+	/*
+	 * If the flag of msg_done is not set, it means that the IPMI command
+	 * is not executed correctly.
+	 * The status code will be ACPI_IPMI_UNKNOWN.
+	 */
+	if (!msg->msg_done) {
+		buffer->status_code = ACPI_IPMI_UNKNOWN;
+		return;
+	}
+	/*
+	 * If the IPMI response message is obtained correctly, the status code
+	 * will be ACPI_IPMI_OK
+	 */
+	buffer->status_code = ACPI_IPMI_OK;
+	buffer->length = msg->rx_len;
+	memcpy(buffer->data, msg->rx_data, msg->rx_len);
+	return;
+}
+static void ipmi_destroy_tx_msg(struct acpi_ipmi_device *ipmi)
+{
+	struct acpi_ipmi_msg *tx_msg = NULL, *temp;
+	int count = 20;
+
+	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))
+		printk(KERN_DEBUG "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 = NULL;
+
+	if (msg->user != ipmi_device->user_interface) {
+		printk(KERN_DEBUG "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);
+		printk(KERN_DEBUG "Incorrect MSG is found \n");
+		return;
+	}
+
+	if (msg->msg.data_len > 1) {
+		/* 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, *temp;
+	struct acpi_device *device;
+	ipmi_user_t		user;
+	int err;
+
+	if (list_empty(&driver_data.ipmi_devices))
+		return;
+
+	list_for_each_entry_safe(ipmi_device, temp,
+					&driver_data.ipmi_devices, head) {
+		device = ipmi_device->device;
+		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;
+		}
+		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;
+			}
+			continue;
+		}
+	}
+	return;
+}
+
+static void ipmi_bmc_gone(int iface)
+{
+	struct acpi_ipmi_device *ipmi_device, *temp;
+
+	if (list_empty(&driver_data.ipmi_devices))
+		return;
+
+	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;
+			/* we should also destory tx msg list */
+			ipmi_destroy_tx_msg(ipmi_device);
+		}
+	}
+}
+/* --------------------------------------------------------------------------
+ * 			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 = NULL;
+	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) {
+		/* Read function is not supported. AE_TYPE is returned. */
+		return AE_TYPE;
+	}
+	if (!ipmi_device->user_interface) {
+		/* there doesn't exist user interface*/
+		return AE_NOT_EXIST;
+	}
+	tx_msg = acpi_alloc_ipmi_msg(ipmi_device);
+	if (!tx_msg) {
+		/* no memory is allocated */
+		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);
+
+end_label:
+	acpi_format_ipmi_response(tx_msg, value, err);
+	status = AE_OK;
+	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)) {
+		printk(KERN_DEBUG "Can't register IPMI opregion %s\n",
+					acpi_device_bid(ipmi->device));
+		return -EINVAL;
+	}
+
+	test_bit(IPMI_FLAGS_HANDLER_INSTALL, &ipmi->flags);
+
+	return 0;
+}
+
+static void acpi_register_ipmi_handler(struct acpi_ipmi_device *ipmi_device)
+{
+
+	mutex_init(&ipmi_device->mutex_lock);
+	INIT_LIST_HEAD(&ipmi_device->tx_msg_list);
+
+	if (ipmi_install_handler(ipmi_device)) {
+		/*
+		 * can't register the IPMI opregion. We will print
+		 * some debug info and continue to register ipmi
+		 * opregion for next device.
+		 */
+		printk(KERN_DEBUG "Can't register IPMI opregion %s\n",
+				acpi_device_bid(ipmi_device->device));
+	}
+	list_add(&ipmi_device->head, &driver_data.ipmi_devices);
+	return;
+}
+
+static void acpi_remove_ipmi_handler(struct acpi_ipmi_device *ipmi_device)
+{
+	if (!p_ipmi)
+		return;
+
+	if (ipmi_device->user_interface) {
+		/*
+		 * If the IPMI user interface is created, it
+		 * should be destroyed.
+		 */
+		ipmi_destroy_user(ipmi_device->user_interface);
+		ipmi_device->user_interface = NULL;
+	}
+	list_del(&ipmi_device->head);
+	if (!list_empty(&ipmi_device->tx_msg_list)) {
+		/* destroy the Tx_msg list */
+		ipmi_destroy_tx_msg(ipmi_device);
+	}
+	ipmi_remove_handler(ipmi_device);
+	kfree(ipmi_device);
+	return;
+}
+
+static int acpi_ipmi_opregion_init(void)
+{
+	int result = 0;
+
+	if (acpi_disabled)
+		return result;
+
+	result = ipmi_smi_watcher_register(&driver_data.bmc_events);
+
+	return result;
+}
+
+static void acpi_ipmi_opregion_exit(void)
+{
+	if (acpi_disabled)
+		return;
+
+	ipmi_smi_watcher_unregister(&driver_data.bmc_events);
+
+	return;
+}
+
 /*
  * Once we get an ACPI failure, we don't try any more, because we go
  * through the tables sequentially.  Once we don't find a table, there
@@ -2033,6 +2504,8 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
 	acpi_handle handle;
 	acpi_status status;
 	unsigned long long tmp;
+	int ret;
+	struct acpi_ipmi_device *p_ipmi = NULL;
 
 	acpi_dev = pnp_acpi_device(dev);
 	if (!acpi_dev)
@@ -2042,6 +2515,13 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
 	if (!info)
 		return -ENOMEM;
 
+	p_ipmi = kzalloc(sizeof(*p_ipmi), GFP_KERNEL);
+	if (!p_ipmi) {
+		printk(KERN_ERR "Can't allocate memory for IPMI device\n");
+		kfree(info);
+		return -ENOMEM;
+	}
+
 	info->addr_source = "ACPI";
 
 	handle = acpi_dev->handle;
@@ -2096,17 +2576,33 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
 	info->dev = &acpi_dev->dev;
 	pnp_set_drvdata(dev, info);
 
-	return try_smi_init(info);
+	acpi_dev->driver_data = p_ipmi;
+	p_ipmi->device = acpi_dev;
+	p_ipmi->handle = handle;
+	p_ipmi->ipmi_ifnum = -1;
 
+	ret = try_smi_init(info);
+	if (!ret) {
+		acpi_register_ipmi_handler(p_ipmi);
+		return ret;
+	}
 err_free:
+	acpi_dev->driver_data = NULL;
+	kfree(p_ipmi);
 	kfree(info);
 	return -EINVAL;
 }
 
 static void __devexit ipmi_pnp_remove(struct pnp_dev *dev)
 {
+	struct acpi_device *acpi_dev;
 	struct smi_info *info = pnp_get_drvdata(dev);
+	struct acpi_ipmi_device *p_ipmi;
 
+	acpi_dev = pnp_acpi_device(dev);
+	p_ipmi = acpi_driver_data(acpi_dev);
+	acpi_remove_ipmi_handler(p_ipmi);
+	acpi_dev->driver_data = NULL;
 	cleanup_one_si(info);
 }
 
@@ -3207,6 +3703,7 @@ static __devinit int init_ipmi_si(void)
 
 #ifdef CONFIG_ACPI
 	spmi_find_bmc();
+	acpi_ipmi_opregion_init();
 #endif
 
 #ifdef CONFIG_PCI
@@ -3327,10 +3824,14 @@ static __exit void cleanup_ipmi_si(void)
 
 	if (!initialized)
 		return;
+#ifdef CONFIG_ACPI
+	acpi_ipmi_opregion_exit();
+#endif
 
 #ifdef CONFIG_PCI
 	pci_unregister_driver(&ipmi_pci_driver);
 #endif
+
 #ifdef CONFIG_PNP
 	pnp_unregister_driver(&ipmi_pnp_driver);
 #endif
-- 
1.5.4.5

Re: [PATCH 1/3] ipmi/acpi: Fix the building error in ipmi module

[Bjorn Helgaas]

On Wednesday 16 December 2009 07:40:29 am yakui.zhao@intel.com wrote:
> From: Zhao Yakui <yakui.zhao@intel.com>
> 
> When the IPMI module is selected as module, it will complain the
> following building error.
> >ERROR: "pnpacpi_protocol" [drivers/char/ipmi/ipmi_si.ko] undefined!
> 
> Fix the above building error.

This is already fixed in Len's tree, so we don't need this patch.

> Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
> cc: Bjorn Helgaas <bjorn.helgaas@hp.com>
> ---
>  drivers/pnp/pnpacpi/core.c |    7 +++++++
>  include/linux/pnp.h        |   11 +++--------
>  2 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pnp/pnpacpi/core.c b/drivers/pnp/pnpacpi/core.c
> index 8dd0f37..8eebbe4 100644
> --- a/drivers/pnp/pnpacpi/core.c
> +++ b/drivers/pnp/pnpacpi/core.c
> @@ -155,6 +155,13 @@ struct pnp_protocol pnpacpi_protocol = {
>  #endif
>  };
>  
> +struct acpi_device *pnp_acpi_device(struct pnp_dev *dev)
> +{
> +	if (dev->protocol == &pnpacpi_protocol)
> +		return dev->data;
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(pnp_acpi_device);
>  static int __init pnpacpi_add_device(struct acpi_device *device)
>  {
>  	acpi_handle temp = NULL;
> diff --git a/include/linux/pnp.h b/include/linux/pnp.h
> index 7c4193e..0ae2c64 100644
> --- a/include/linux/pnp.h
> +++ b/include/linux/pnp.h
> @@ -335,16 +335,11 @@ extern struct pnp_protocol pnpbios_protocol;
>  #endif
>  
>  #ifdef CONFIG_PNPACPI
> -extern struct pnp_protocol pnpacpi_protocol;
>  
> -static inline struct acpi_device *pnp_acpi_device(struct pnp_dev *dev)
> -{
> -	if (dev->protocol == &pnpacpi_protocol)
> -		return dev->data;
> -	return NULL;
> -}
> +extern struct acpi_device *pnp_acpi_device(struct pnp_dev *dev);
>  #else
> -#define pnp_acpi_device(dev) 0
> +static inline struct acpi_device *pnp_acpi_device(struct pnp_dev *dev)
> +{ return NULL; }
>  #endif
>  
>  /* status */

Re: [PATCH 3/3] ipmi/acpi: Install the IPMI space handler to enable ACPI to access the BMC controller

[Bjorn Helgaas]

On Wednesday 16 December 2009 07:40:31 am yakui.zhao@intel.com wrote:
> From: Zhao Yakui <yakui.zhao@intel.com>
> 
> 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.
> 
> 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.
> 
> Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
> cc: Bjorn Helgaas <bjorn.helgaas@hp.com>
> ---
>  drivers/char/ipmi/ipmi_si_intf.c |  505 +++++++++++++++++++++++++++++++++++++-
>  1 files changed, 503 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> index 99fecd2..9c1e212 100644
> --- a/drivers/char/ipmi/ipmi_si_intf.c
> +++ b/drivers/char/ipmi/ipmi_si_intf.c
> @@ -65,7 +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>
>  #include <linux/of_platform.h>
> @@ -1808,6 +1808,477 @@ static __devinit void hardcode_find_bmc(void)
>  
>  #include <linux/acpi.h>
>  
> +#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 30s */
> +#define IPMI_TIMEOUT			(30 * HZ)
> +
> +struct acpi_ipmi_device {
> +	acpi_handle handle;
> +	struct acpi_device *device;
> +	/* the device list attached to driver_data.ipmi_devices */
> +	struct list_head head;
> +	ipmi_user_t 	user_interface;
> +	struct mutex	mutex_lock;
> +	/* the IPMI request message list */
> +	struct list_head tx_msg_list;
> +	long curr_msgid;
> +	/* IPMI flags */
> +	unsigned long flags;
> +	/* IPMI interface number */
> +	int ipmi_ifnum;
> +};
> +
> +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 {
> +	/* message list */
> +	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;
> +	/* tx message id */
> +	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;
> +	/* get the response data */
> +	u8	rx_data[64];
> +	/* the response length. The netfn & cmd is excluded. */
> +	int	rx_len;
> +	struct acpi_ipmi_device *device;
> +};
> +
> +/*
> + * IPMI request/response buffer.
> + * The length is 66 bytes.
> + */
> +struct acpi_ipmi_buffer {
> +	/* status code of a given IPMI command */
> +	u8 status_code;
> +	/* the length of the payload */
> +	u8 length;
> +	/*
> +	 * the payload. Before the operation is carried out, it represents the
> +	 * request message payload. After the operation is carried out, it
> +	 * stores the response message returned by IPMI command.
> +	 */
> +	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,
> +	},
> +};
> +
> +static
> +struct acpi_ipmi_msg *acpi_alloc_ipmi_msg(struct acpi_ipmi_device *ipmi)
> +{
> +	struct acpi_ipmi_msg *ipmi_msg;
> +
> +	ipmi_msg = kzalloc(sizeof(struct acpi_ipmi_msg), GFP_KERNEL);
> +	if (!ipmi_msg)	{
> +		printk(KERN_DEBUG "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;
> +}
> +
> +static void acpi_format_ipmi_msg(struct acpi_ipmi_msg *tx_msg,
> +		acpi_physical_address address,
> +		acpi_integer *value)
> +{
> +	struct kernel_ipmi_msg *msg;
> +	u8	temp_value;
> +	struct acpi_ipmi_buffer *buffer;
> +	struct acpi_ipmi_device *device;
> +
> +	msg = &tx_msg->tx_message;
> +	/* get the netfn */
> +	temp_value = (address >> 8) & 0xff;
> +	msg->netfn = temp_value;
> +	/* get the command */
> +	temp_value = address & 0xff;
> +	msg->cmd = temp_value;
> +	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.
> +	 */
> +	tx_msg->addr.addr_type = IPMI_SYSTEM_INTERFACE_ADDR_TYPE;
> +	tx_msg->addr.channel = IPMI_BMC_CHANNEL;
> +	tx_msg->addr.data[0] = 0;
> +
> +	/*
> +	 * If the netfn is APP_REQUEST and the cmd is SEND_MESSAGE, we should
> +	 * parse the IPMI request message buffer to get the IPMB address.
> +	 * If so, please fix me.
> +	 */
> +
> +	/* 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 timeout)
> +{
> +	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;
> +	/* when timeout is zero, it means that the timeout happens */
> +	if (!timeout) {
> +		/* the status code is ACPI_IPMI_TIMEOUT */
> +		buffer->status_code = ACPI_IPMI_TIMEOUT;
> +		return;
> +	}
> +	/*
> +	 * If the flag of msg_done is not set, it means that the IPMI command
> +	 * is not executed correctly.
> +	 * The status code will be ACPI_IPMI_UNKNOWN.
> +	 */
> +	if (!msg->msg_done) {
> +		buffer->status_code = ACPI_IPMI_UNKNOWN;
> +		return;
> +	}
> +	/*
> +	 * If the IPMI response message is obtained correctly, the status code
> +	 * will be ACPI_IPMI_OK
> +	 */
> +	buffer->status_code = ACPI_IPMI_OK;
> +	buffer->length = msg->rx_len;
> +	memcpy(buffer->data, msg->rx_data, msg->rx_len);
> +	return;

"return" is not needed here.

> +}
> +static void ipmi_destroy_tx_msg(struct acpi_ipmi_device *ipmi)
> +{
> +	struct acpi_ipmi_msg *tx_msg = NULL, *temp;
> +	int count = 20;
> +
> +	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))
> +		printk(KERN_DEBUG "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 = NULL;
> +
> +	if (msg->user != ipmi_device->user_interface) {
> +		printk(KERN_DEBUG "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);
> +		printk(KERN_DEBUG "Incorrect MSG is found \n");
> +		return;
> +	}
> +
> +	if (msg->msg.data_len > 1) {
> +		/* 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, *temp;
> +	struct acpi_device *device;
> +	ipmi_user_t		user;
> +	int err;
> +
> +	if (list_empty(&driver_data.ipmi_devices))
> +		return;
> +
> +	list_for_each_entry_safe(ipmi_device, temp,
> +					&driver_data.ipmi_devices, head) {
> +		device = ipmi_device->device;
> +		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;
> +		}
> +		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;
> +			}
> +			continue;
> +		}
> +	}
> +	return;
> +}
> +
> +static void ipmi_bmc_gone(int iface)
> +{
> +	struct acpi_ipmi_device *ipmi_device, *temp;
> +
> +	if (list_empty(&driver_data.ipmi_devices))
> +		return;
> +
> +	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;
> +			/* we should also destory tx msg list */
> +			ipmi_destroy_tx_msg(ipmi_device);
> +		}
> +	}
> +}
> +/* --------------------------------------------------------------------------
> + * 			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 = NULL;

You don't need to initialize this, since the first reference is an
assignment.

> +	struct acpi_ipmi_device *ipmi_device =
> +			(struct acpi_ipmi_device *) handler_context;

I don't think you need this cast.

> +	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) {
> +		/* Read function is not supported. AE_TYPE is returned. */

Superfluous comment.

> +		return AE_TYPE;
> +	}
> +	if (!ipmi_device->user_interface) {
> +		/* there doesn't exist user interface*/

Superfluous comment.

> +		return AE_NOT_EXIST;
> +	}
> +	tx_msg = acpi_alloc_ipmi_msg(ipmi_device);
> +	if (!tx_msg) {
> +		/* no memory is allocated */

Superfluous comment.

> +		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);
> +
> +end_label:
> +	acpi_format_ipmi_response(tx_msg, value, err);
> +	status = AE_OK;

This clobbers the AE_ERROR you assigned in the "if (err)" above,
i.e., I don't think it's possible for this function to return AE_ERROR,
and I don't think that's what you intended.

> +	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)) {
> +		printk(KERN_DEBUG "Can't register IPMI opregion %s\n",
> +					acpi_device_bid(ipmi->device));
> +		return -EINVAL;
> +	}
> +
> +	test_bit(IPMI_FLAGS_HANDLER_INSTALL, &ipmi->flags);
> +
> +	return 0;
> +}
> +
> +static void acpi_register_ipmi_handler(struct acpi_ipmi_device *ipmi_device)
> +{
> +
> +	mutex_init(&ipmi_device->mutex_lock);
> +	INIT_LIST_HEAD(&ipmi_device->tx_msg_list);
> +
> +	if (ipmi_install_handler(ipmi_device)) {
> +		/*
> +		 * can't register the IPMI opregion. We will print
> +		 * some debug info and continue to register ipmi
> +		 * opregion for next device.
> +		 */
> +		printk(KERN_DEBUG "Can't register IPMI opregion %s\n",
> +				acpi_device_bid(ipmi_device->device));
> +	}
> +	list_add(&ipmi_device->head, &driver_data.ipmi_devices);
> +	return;
> +}
> +
> +static void acpi_remove_ipmi_handler(struct acpi_ipmi_device *ipmi_device)
> +{
> +	if (!p_ipmi)
> +		return;
> +
> +	if (ipmi_device->user_interface) {
> +		/*
> +		 * If the IPMI user interface is created, it
> +		 * should be destroyed.
> +		 */
> +		ipmi_destroy_user(ipmi_device->user_interface);
> +		ipmi_device->user_interface = NULL;
> +	}
> +	list_del(&ipmi_device->head);
> +	if (!list_empty(&ipmi_device->tx_msg_list)) {
> +		/* destroy the Tx_msg list */

You have good variable and function names, so the comments in this
function (and the previous one) are unnecessary and even distracting.

> +		ipmi_destroy_tx_msg(ipmi_device);
> +	}
> +	ipmi_remove_handler(ipmi_device);
> +	kfree(ipmi_device);
> +	return;
> +}
> +
> +static int acpi_ipmi_opregion_init(void)
> +{
> +	int result = 0;
> +
> +	if (acpi_disabled)
> +		return result;

I don't think you need this check.

> +
> +	result = ipmi_smi_watcher_register(&driver_data.bmc_events);
> +
> +	return result;
> +}
> +
> +static void acpi_ipmi_opregion_exit(void)
> +{
> +	if (acpi_disabled)
> +		return;

Or this one.

> +
> +	ipmi_smi_watcher_unregister(&driver_data.bmc_events);
> +
> +	return;

You don't need this "return" statement.

> +}
> +
>  /*
>   * Once we get an ACPI failure, we don't try any more, because we go
>   * through the tables sequentially.  Once we don't find a table, there
> @@ -2033,6 +2504,8 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
>  	acpi_handle handle;
>  	acpi_status status;
>  	unsigned long long tmp;
> +	int ret;
> +	struct acpi_ipmi_device *p_ipmi = NULL;
>  
>  	acpi_dev = pnp_acpi_device(dev);
>  	if (!acpi_dev)
> @@ -2042,6 +2515,13 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
>  	if (!info)
>  		return -ENOMEM;
>  
> +	p_ipmi = kzalloc(sizeof(*p_ipmi), GFP_KERNEL);
> +	if (!p_ipmi) {
> +		printk(KERN_ERR "Can't allocate memory for IPMI device\n");
> +		kfree(info);
> +		return -ENOMEM;
> +	}
> +
>  	info->addr_source = "ACPI";
>  
>  	handle = acpi_dev->handle;
> @@ -2096,17 +2576,33 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
>  	info->dev = &acpi_dev->dev;
>  	pnp_set_drvdata(dev, info);
>  
> -	return try_smi_init(info);
> +	acpi_dev->driver_data = p_ipmi;

I don't think we should use both the pnp_dev driver_data
and the acpi_dev driver_data.  There's only one underlying
physical device, and the fact that we have two ways to get
at it is a Linux design problem.  It would be better to add
the p_ipmi pointer to the smi_info structure, and use only
pnp_set_drvdata().

> +	p_ipmi->device = acpi_dev;
> +	p_ipmi->handle = handle;
> +	p_ipmi->ipmi_ifnum = -1;
>  
> +	ret = try_smi_init(info);
> +	if (!ret) {
> +		acpi_register_ipmi_handler(p_ipmi);
> +		return ret;
> +	}
>  err_free:
> +	acpi_dev->driver_data = NULL;
> +	kfree(p_ipmi);
>  	kfree(info);
>  	return -EINVAL;
>  }
>  
>  static void __devexit ipmi_pnp_remove(struct pnp_dev *dev)
>  {
> +	struct acpi_device *acpi_dev;
>  	struct smi_info *info = pnp_get_drvdata(dev);
> +	struct acpi_ipmi_device *p_ipmi;
>  
> +	acpi_dev = pnp_acpi_device(dev);
> +	p_ipmi = acpi_driver_data(acpi_dev);
> +	acpi_remove_ipmi_handler(p_ipmi);
> +	acpi_dev->driver_data = NULL;
>  	cleanup_one_si(info);
>  }
>  
> @@ -3207,6 +3703,7 @@ static __devinit int init_ipmi_si(void)
>  
>  #ifdef CONFIG_ACPI
>  	spmi_find_bmc();
> +	acpi_ipmi_opregion_init();

I don't think this belongs in the IPMI driver init.  It should be done
in ipmi_pnp_probe() when we actually discover an IPI0001 device.

>  #endif
>  
>  #ifdef CONFIG_PCI
> @@ -3327,10 +3824,14 @@ static __exit void cleanup_ipmi_si(void)
>  
>  	if (!initialized)
>  		return;
> +#ifdef CONFIG_ACPI
> +	acpi_ipmi_opregion_exit();

Likewise, this should be done in ipmi_pnp_remove(), not in the the
IPMI driver cleanup path.

> +#endif
>  
>  #ifdef CONFIG_PCI
>  	pci_unregister_driver(&ipmi_pci_driver);
>  #endif
> +
>  #ifdef CONFIG_PNP
>  	pnp_unregister_driver(&ipmi_pnp_driver);
>  #endif

Re: [PATCH 1/3] ipmi/acpi: Fix the building error in ipmi module

[ykzhao]

On Wed, 2009-12-16 at 23:52 +0800, Bjorn Helgaas wrote:
> On Wednesday 16 December 2009 07:40:29 am yakui.zhao@intel.com wrote:
> > From: Zhao Yakui <yakui.zhao@intel.com>
> > 
> > When the IPMI module is selected as module, it will complain the
> > following building error.
> > >ERROR: "pnpacpi_protocol" [drivers/char/ipmi/ipmi_si.ko] undefined!
> > 
> > Fix the above building error.
> 
> This is already fixed in Len's tree, so we don't need this patch.
Yes. Just now I checked Len's tree and this is already fixed.

Thanks.
> 
> > Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
> > cc: Bjorn Helgaas <bjorn.helgaas@hp.com>
> > ---
> >  drivers/pnp/pnpacpi/core.c |    7 +++++++
> >  include/linux/pnp.h        |   11 +++--------
> >  2 files changed, 10 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/pnp/pnpacpi/core.c b/drivers/pnp/pnpacpi/core.c
> > index 8dd0f37..8eebbe4 100644
> > --- a/drivers/pnp/pnpacpi/core.c
> > +++ b/drivers/pnp/pnpacpi/core.c
> > @@ -155,6 +155,13 @@ struct pnp_protocol pnpacpi_protocol = {
> >  #endif
> >  };
> >  
> > +struct acpi_device *pnp_acpi_device(struct pnp_dev *dev)
> > +{
> > +	if (dev->protocol == &pnpacpi_protocol)
> > +		return dev->data;
> > +	return NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(pnp_acpi_device);
> >  static int __init pnpacpi_add_device(struct acpi_device *device)
> >  {
> >  	acpi_handle temp = NULL;
> > diff --git a/include/linux/pnp.h b/include/linux/pnp.h
> > index 7c4193e..0ae2c64 100644
> > --- a/include/linux/pnp.h
> > +++ b/include/linux/pnp.h
> > @@ -335,16 +335,11 @@ extern struct pnp_protocol pnpbios_protocol;
> >  #endif
> >  
> >  #ifdef CONFIG_PNPACPI
> > -extern struct pnp_protocol pnpacpi_protocol;
> >  
> > -static inline struct acpi_device *pnp_acpi_device(struct pnp_dev *dev)
> > -{
> > -	if (dev->protocol == &pnpacpi_protocol)
> > -		return dev->data;
> > -	return NULL;
> > -}
> > +extern struct acpi_device *pnp_acpi_device(struct pnp_dev *dev);
> >  #else
> > -#define pnp_acpi_device(dev) 0
> > +static inline struct acpi_device *pnp_acpi_device(struct pnp_dev *dev)
> > +{ return NULL; }
> >  #endif
> >  
> >  /* status */
> 
> 

Re: [PATCH 3/3] ipmi/acpi: Install the IPMI space handler to enable ACPI to access the BMC controller

[ykzhao]

On Thu, 2009-12-17 at 00:40 +0800, Bjorn Helgaas wrote:
> On Wednesday 16 December 2009 07:40:31 am yakui.zhao@intel.com wrote:
> > From: Zhao Yakui <yakui.zhao@intel.com>
> >
> > 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.
> >
> > 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.
> >
> > Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
> > cc: Bjorn Helgaas <bjorn.helgaas@hp.com>
> > ---
> >  drivers/char/ipmi/ipmi_si_intf.c |  505 +++++++++++++++++++++++++++++++++++++-
> >  1 files changed, 503 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> > index 99fecd2..9c1e212 100644
> > --- a/drivers/char/ipmi/ipmi_si_intf.c
> > +++ b/drivers/char/ipmi/ipmi_si_intf.c
> > @@ -65,7 +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>
> >  #include <linux/of_platform.h>
> > @@ -1808,6 +1808,477 @@ static __devinit void hardcode_find_bmc(void)
> >
> >  #include <linux/acpi.h>
> >
> > +#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 30s */
> > +#define IPMI_TIMEOUT                 (30 * HZ)
> > +
> > +struct acpi_ipmi_device {
> > +     acpi_handle handle;
> > +     struct acpi_device *device;
> > +     /* the device list attached to driver_data.ipmi_devices */
> > +     struct list_head head;
> > +     ipmi_user_t     user_interface;
> > +     struct mutex    mutex_lock;
> > +     /* the IPMI request message list */
> > +     struct list_head tx_msg_list;
> > +     long curr_msgid;
> > +     /* IPMI flags */
> > +     unsigned long flags;
> > +     /* IPMI interface number */
> > +     int ipmi_ifnum;
> > +};
> > +
> > +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 {
> > +     /* message list */
> > +     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;
> > +     /* tx message id */
> > +     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;
> > +     /* get the response data */
> > +     u8      rx_data[64];
> > +     /* the response length. The netfn & cmd is excluded. */
> > +     int     rx_len;
> > +     struct acpi_ipmi_device *device;
> > +};
> > +
> > +/*
> > + * IPMI request/response buffer.
> > + * The length is 66 bytes.
> > + */
> > +struct acpi_ipmi_buffer {
> > +     /* status code of a given IPMI command */
> > +     u8 status_code;
> > +     /* the length of the payload */
> > +     u8 length;
> > +     /*
> > +      * the payload. Before the operation is carried out, it represents the
> > +      * request message payload. After the operation is carried out, it
> > +      * stores the response message returned by IPMI command.
> > +      */
> > +     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,
> > +     },
> > +};
> > +
> > +static
> > +struct acpi_ipmi_msg *acpi_alloc_ipmi_msg(struct acpi_ipmi_device *ipmi)
> > +{
> > +     struct acpi_ipmi_msg *ipmi_msg;
> > +
> > +     ipmi_msg = kzalloc(sizeof(struct acpi_ipmi_msg), GFP_KERNEL);
> > +     if (!ipmi_msg)  {
> > +             printk(KERN_DEBUG "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;
> > +}
> > +
> > +static void acpi_format_ipmi_msg(struct acpi_ipmi_msg *tx_msg,
> > +             acpi_physical_address address,
> > +             acpi_integer *value)
> > +{
> > +     struct kernel_ipmi_msg *msg;
> > +     u8      temp_value;
> > +     struct acpi_ipmi_buffer *buffer;
> > +     struct acpi_ipmi_device *device;
> > +
> > +     msg = &tx_msg->tx_message;
> > +     /* get the netfn */
> > +     temp_value = (address >> 8) & 0xff;
> > +     msg->netfn = temp_value;
> > +     /* get the command */
> > +     temp_value = address & 0xff;
> > +     msg->cmd = temp_value;
> > +     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.
> > +      */
> > +     tx_msg->addr.addr_type = IPMI_SYSTEM_INTERFACE_ADDR_TYPE;
> > +     tx_msg->addr.channel = IPMI_BMC_CHANNEL;
> > +     tx_msg->addr.data[0] = 0;
> > +
> > +     /*
> > +      * If the netfn is APP_REQUEST and the cmd is SEND_MESSAGE, we should
> > +      * parse the IPMI request message buffer to get the IPMB address.
> > +      * If so, please fix me.
> > +      */
> > +
> > +     /* 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 timeout)
> > +{
> > +     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;
> > +     /* when timeout is zero, it means that the timeout happens */
> > +     if (!timeout) {
> > +             /* the status code is ACPI_IPMI_TIMEOUT */
> > +             buffer->status_code = ACPI_IPMI_TIMEOUT;
> > +             return;
> > +     }
> > +     /*
> > +      * If the flag of msg_done is not set, it means that the IPMI command
> > +      * is not executed correctly.
> > +      * The status code will be ACPI_IPMI_UNKNOWN.
> > +      */
> > +     if (!msg->msg_done) {
> > +             buffer->status_code = ACPI_IPMI_UNKNOWN;
> > +             return;
> > +     }
> > +     /*
> > +      * If the IPMI response message is obtained correctly, the status code
> > +      * will be ACPI_IPMI_OK
> > +      */
> > +     buffer->status_code = ACPI_IPMI_OK;
> > +     buffer->length = msg->rx_len;
> > +     memcpy(buffer->data, msg->rx_data, msg->rx_len);
> > +     return;
> 
> "return" is not needed here.

OK. I can remove it.


> > +}
> > +static void ipmi_destroy_tx_msg(struct acpi_ipmi_device *ipmi)
> > +{
> > +     struct acpi_ipmi_msg *tx_msg = NULL, *temp;
> > +     int count = 20;
> > +
> > +     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))
> > +             printk(KERN_DEBUG "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 = NULL;
> > +
> > +     if (msg->user != ipmi_device->user_interface) {
> > +             printk(KERN_DEBUG "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);
> > +             printk(KERN_DEBUG "Incorrect MSG is found \n");
> > +             return;
> > +     }
> > +
> > +     if (msg->msg.data_len > 1) {
> > +             /* 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, *temp;
> > +     struct acpi_device *device;
> > +     ipmi_user_t             user;
> > +     int err;
> > +
> > +     if (list_empty(&driver_data.ipmi_devices))
> > +             return;
> > +
> > +     list_for_each_entry_safe(ipmi_device, temp,
> > +                                     &driver_data.ipmi_devices, head) {
> > +             device = ipmi_device->device;
> > +             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;
> > +             }
> > +             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;
> > +                     }
> > +                     continue;
> > +             }
> > +     }
> > +     return;
> > +}
> > +
> > +static void ipmi_bmc_gone(int iface)
> > +{
> > +     struct acpi_ipmi_device *ipmi_device, *temp;
> > +
> > +     if (list_empty(&driver_data.ipmi_devices))
> > +             return;
> > +
> > +     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;
> > +                     /* we should also destory tx msg list */
> > +                     ipmi_destroy_tx_msg(ipmi_device);
> > +             }
> > +     }
> > +}
> > +/* --------------------------------------------------------------------------
> > + *                   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 = NULL;
> 
> You don't need to initialize this, since the first reference is an
> assignment.
> 
> > +     struct acpi_ipmi_device *ipmi_device =
> > +                     (struct acpi_ipmi_device *) handler_context;
> 
> I don't think you need this cast.

Without the initialization or cast type, it still can work. But it is
not wrong.
Right? Of course I can remove it.

> 
> > +     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) {
> > +             /* Read function is not supported. AE_TYPE is returned. */
> 
> Superfluous comment.
> 
> > +             return AE_TYPE;
> > +     }
> > +     if (!ipmi_device->user_interface) {
> > +             /* there doesn't exist user interface*/
> 
> Superfluous comment.
> 
> > +             return AE_NOT_EXIST;
> > +     }
> > +     tx_msg = acpi_alloc_ipmi_msg(ipmi_device);
> > +     if (!tx_msg) {
> > +             /* no memory is allocated */
> 
> Superfluous comment.
> 
> > +             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);
> > +
> > +end_label:
> > +     acpi_format_ipmi_response(tx_msg, value, err);
> > +     status = AE_OK;
> 
> This clobbers the AE_ERROR you assigned in the "if (err)" above,
> i.e., I don't think it's possible for this function to return AE_ERROR,
> and I don't think that's what you intended.

You are right. Thanks for pointing out this issue.

> 
> > +     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)) {
> > +             printk(KERN_DEBUG "Can't register IPMI opregion %s\n",
> > +                                     acpi_device_bid(ipmi->device));
> > +             return -EINVAL;
> > +     }
> > +
> > +     test_bit(IPMI_FLAGS_HANDLER_INSTALL, &ipmi->flags);
> > +
> > +     return 0;
> > +}
> > +
> > +static void acpi_register_ipmi_handler(struct acpi_ipmi_device *ipmi_device)
> > +{
> > +
> > +     mutex_init(&ipmi_device->mutex_lock);
> > +     INIT_LIST_HEAD(&ipmi_device->tx_msg_list);
> > +
> > +     if (ipmi_install_handler(ipmi_device)) {
> > +             /*
> > +              * can't register the IPMI opregion. We will print
> > +              * some debug info and continue to register ipmi
> > +              * opregion for next device.
> > +              */
> > +             printk(KERN_DEBUG "Can't register IPMI opregion %s\n",
> > +                             acpi_device_bid(ipmi_device->device));
> > +     }
> > +     list_add(&ipmi_device->head, &driver_data.ipmi_devices);
> > +     return;
> > +}
> > +
> > +static void acpi_remove_ipmi_handler(struct acpi_ipmi_device *ipmi_device)
> > +{
> > +     if (!p_ipmi)
> > +             return;
> > +
> > +     if (ipmi_device->user_interface) {
> > +             /*
> > +              * If the IPMI user interface is created, it
> > +              * should be destroyed.
> > +              */
> > +             ipmi_destroy_user(ipmi_device->user_interface);
> > +             ipmi_device->user_interface = NULL;
> > +     }
> > +     list_del(&ipmi_device->head);
> > +     if (!list_empty(&ipmi_device->tx_msg_list)) {
> > +             /* destroy the Tx_msg list */
> 
> You have good variable and function names, so the comments in this
> function (and the previous one) are unnecessary and even distracting.

OK. I can remove the superfluous comment.
> 
> > +             ipmi_destroy_tx_msg(ipmi_device);
> > +     }
> > +     ipmi_remove_handler(ipmi_device);
> > +     kfree(ipmi_device);
> > +     return;
> > +}
> > +
> > +static int acpi_ipmi_opregion_init(void)
> > +{
> > +     int result = 0;
> > +
> > +     if (acpi_disabled)
> > +             return result;
> 
> I don't think you need this check.

It is also ok to delete this check. 
But in fact it is unnecessary to register the smi_watcher again when
ACPI is disabled.  Otherwise after we register it, it will call the
corresponding callback defined in smi_watcher when one IPMI system
interface is registered/unregistered. 

So IMO we can register the smi_watcher only when ACPI is enabled.

> > +
> > +     result = ipmi_smi_watcher_register(&driver_data.bmc_events);
> > +
> > +     return result;
> > +}
> > +
> > +static void acpi_ipmi_opregion_exit(void)
> > +{
> > +     if (acpi_disabled)
> > +             return;
> 
> Or this one.
> 
> > +
> > +     ipmi_smi_watcher_unregister(&driver_data.bmc_events);
> > +
> > +     return;
> 
> You don't need this "return" statement.
> 
> > +}
> > +
> >  /*
> >   * Once we get an ACPI failure, we don't try any more, because we go
> >   * through the tables sequentially.  Once we don't find a table, there
> > @@ -2033,6 +2504,8 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
> >       acpi_handle handle;
> >       acpi_status status;
> >       unsigned long long tmp;
> > +     int ret;
> > +     struct acpi_ipmi_device *p_ipmi = NULL;
> >
> >       acpi_dev = pnp_acpi_device(dev);
> >       if (!acpi_dev)
> > @@ -2042,6 +2515,13 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
> >       if (!info)
> >               return -ENOMEM;
> >
> > +     p_ipmi = kzalloc(sizeof(*p_ipmi), GFP_KERNEL);
> > +     if (!p_ipmi) {
> > +             printk(KERN_ERR "Can't allocate memory for IPMI device\n");
> > +             kfree(info);
> > +             return -ENOMEM;
> > +     }
> > +
> >       info->addr_source = "ACPI";
> >
> >       handle = acpi_dev->handle;
> > @@ -2096,17 +2576,33 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
> >       info->dev = &acpi_dev->dev;
> >       pnp_set_drvdata(dev, info);
> >
> > -     return try_smi_init(info);
> > +     acpi_dev->driver_data = p_ipmi;
> 
> I don't think we should use both the pnp_dev driver_data
> and the acpi_dev driver_data.  There's only one underlying
> physical device, and the fact that we have two ways to get
> at it is a Linux design problem.  It would be better to add
> the p_ipmi pointer to the smi_info structure, and use only
> pnp_set_drvdata().

Understand what you said.

It is not a good idea to add a new pointer in smi_info structure. In
fact the smi_info is the abstract layer of IPMI system interface. To add
a definition in smi_info seems a bit confusing.

> 
> > +     p_ipmi->device = acpi_dev;
> > +     p_ipmi->handle = handle;
> > +     p_ipmi->ipmi_ifnum = -1;
> >
> > +     ret = try_smi_init(info);
> > +     if (!ret) {
> > +             acpi_register_ipmi_handler(p_ipmi);
> > +             return ret;
> > +     }
> >  err_free:
> > +     acpi_dev->driver_data = NULL;
> > +     kfree(p_ipmi);
> >       kfree(info);
> >       return -EINVAL;
> >  }
> >
> >  static void __devexit ipmi_pnp_remove(struct pnp_dev *dev)
> >  {
> > +     struct acpi_device *acpi_dev;
> >       struct smi_info *info = pnp_get_drvdata(dev);
> > +     struct acpi_ipmi_device *p_ipmi;
> >
> > +     acpi_dev = pnp_acpi_device(dev);
> > +     p_ipmi = acpi_driver_data(acpi_dev);
> > +     acpi_remove_ipmi_handler(p_ipmi);
> > +     acpi_dev->driver_data = NULL;
> >       cleanup_one_si(info);
> >  }
> >
> > @@ -3207,6 +3703,7 @@ static __devinit int init_ipmi_si(void)
> >
> >  #ifdef CONFIG_ACPI
> >       spmi_find_bmc();
> > +     acpi_ipmi_opregion_init();
> 
> I don't think this belongs in the IPMI driver init.  It should be done
> in ipmi_pnp_probe() when we actually discover an IPI0001 device.

This function is mainly to register the smi_watcher. When one IPMI
system interface(smi_info) is registered/unregistered, it will call the
corresponding callback function to create/destroy the corresponding IPMI
user interface between the user and IPMI system interface.(In our code
the user is the ACPI IPMI device defined in ACPI namespace). 
That means that it can create the corresponding user interface for more
than one ACPI IPMI devices. 
So IMO this had better not be added in the ipmi_pnp_probe/remove
function.

Thanks.
> 
> >  #endif
> >
> >  #ifdef CONFIG_PCI
> > @@ -3327,10 +3824,14 @@ static __exit void cleanup_ipmi_si(void)
> >
> >       if (!initialized)
> >               return;
> > +#ifdef CONFIG_ACPI
> > +     acpi_ipmi_opregion_exit();
> 
> Likewise, this should be done in ipmi_pnp_remove(), not in the the
> IPMI driver cleanup path.
> 
> > +#endif
> >
> >  #ifdef CONFIG_PCI
> >       pci_unregister_driver(&ipmi_pci_driver);
> >  #endif
> > +
> >  #ifdef CONFIG_PNP
> >       pnp_unregister_driver(&ipmi_pnp_driver);
> >  #endif
> 
> 

Re: [PATCH 3/3] ipmi/acpi: Install the IPMI space handler to enable ACPI to access the BMC controller

[ykzhao]

On Thu, 2009-12-17 at 00:40 +0800, Bjorn Helgaas wrote:

Hi, Bjorn
    the following is updated patch. Please review it.

>From 5603592583fd4091a9bd257aa24136469cdbd12b Mon Sep 17 00:00:00 2001
From: Zhao Yakui <yakui.zhao@intel.com>
Date: Thu, 17 Dec 2009 11:03:53 +0800
Subject: [PATCH] IPMI/ACPI: Install the IPMI space handler to enable ACPI to access the BMC controller

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.

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.

Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
cc: Bjorn Helgaas <bjorn.helgaas@hp.com>
---
 drivers/char/ipmi/ipmi_si_intf.c |  503 +++++++++++++++++++++++++++++++++++++-
 1 files changed, 499 insertions(+), 4 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 99fecd2..6ce165d 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -65,7 +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>
 #include <linux/of_platform.h>
@@ -1808,6 +1808,471 @@ static __devinit void hardcode_find_bmc(void)
 
 #include <linux/acpi.h>
 
+#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 30s */
+#define IPMI_TIMEOUT			(30 * HZ)
+
+struct acpi_ipmi_device {
+	acpi_handle handle;
+	struct acpi_device *device;
+	/* the device list attached to driver_data.ipmi_devices */
+	struct list_head head;
+	ipmi_user_t 	user_interface;
+	struct mutex	mutex_lock;
+	/* the IPMI request message list */
+	struct list_head tx_msg_list;
+	long curr_msgid;
+	/* IPMI flags */
+	unsigned long flags;
+	/* IPMI interface number */
+	int ipmi_ifnum;
+	struct smi_info *smi_info;
+};
+
+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 {
+	/* message list */
+	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;
+	/* tx message id */
+	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;
+	/* get the response data */
+	u8	rx_data[64];
+	/* the response length. The netfn & cmd is excluded. */
+	int	rx_len;
+	struct acpi_ipmi_device *device;
+};
+
+/*
+ * IPMI request/response buffer.
+ * The length is 66 bytes.
+ */
+struct acpi_ipmi_buffer {
+	/* status code of a given IPMI command */
+	u8 status_code;
+	/* the length of the payload */
+	u8 length;
+	/*
+	 * the payload. Before the operation is carried out, it represents the
+	 * request message payload. After the operation is carried out, it
+	 * stores the response message returned by IPMI command.
+	 */
+	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,
+	},
+};
+
+static
+struct acpi_ipmi_msg *acpi_alloc_ipmi_msg(struct acpi_ipmi_device *ipmi)
+{
+	struct acpi_ipmi_msg *ipmi_msg;
+
+	ipmi_msg = kzalloc(sizeof(struct acpi_ipmi_msg), GFP_KERNEL);
+	if (!ipmi_msg)	{
+		printk(KERN_DEBUG "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;
+}
+
+static void acpi_format_ipmi_msg(struct acpi_ipmi_msg *tx_msg,
+		acpi_physical_address address,
+		acpi_integer *value)
+{
+	struct kernel_ipmi_msg *msg;
+	u8	temp_value;
+	struct acpi_ipmi_buffer *buffer;
+	struct acpi_ipmi_device *device;
+
+	msg = &tx_msg->tx_message;
+	/* get the netfn */
+	temp_value = (address >> 8) & 0xff;
+	msg->netfn = temp_value;
+	/* get the command */
+	temp_value = address & 0xff;
+	msg->cmd = temp_value;
+	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.
+	 */
+	tx_msg->addr.addr_type = IPMI_SYSTEM_INTERFACE_ADDR_TYPE;
+	tx_msg->addr.channel = IPMI_BMC_CHANNEL;
+	tx_msg->addr.data[0] = 0;
+
+	/*
+	 * If the netfn is APP_REQUEST and the cmd is SEND_MESSAGE, we should
+	 * parse the IPMI request message buffer to get the IPMB address.
+	 * If so, please fix me.
+	 */
+
+	/* 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 timeout)
+{
+	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;
+	/* when timeout is zero, it means that the timeout happens */
+	if (!timeout) {
+		/* the status code is ACPI_IPMI_TIMEOUT */
+		buffer->status_code = ACPI_IPMI_TIMEOUT;
+		return;
+	}
+	/*
+	 * If the flag of msg_done is not set, it means that the IPMI command
+	 * is not executed correctly.
+	 * The status code will be ACPI_IPMI_UNKNOWN.
+	 */
+	if (!msg->msg_done) {
+		buffer->status_code = ACPI_IPMI_UNKNOWN;
+		return;
+	}
+	/*
+	 * If the IPMI response message is obtained correctly, the status code
+	 * will be ACPI_IPMI_OK
+	 */
+	buffer->status_code = 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 = NULL, *temp;
+	int count = 20;
+
+	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))
+		printk(KERN_DEBUG "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 = user_msg_data;
+	int msg_found = 0;
+	struct acpi_ipmi_msg *tx_msg;
+
+	if (msg->user != ipmi_device->user_interface) {
+		printk(KERN_DEBUG "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);
+		printk(KERN_DEBUG "Incorrect MSG is found \n");
+		return;
+	}
+
+	if (msg->msg.data_len > 1) {
+		/* 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, *temp;
+	struct acpi_device *device;
+	ipmi_user_t		user;
+	int err;
+
+	if (list_empty(&driver_data.ipmi_devices))
+		return;
+
+	list_for_each_entry_safe(ipmi_device, temp,
+					&driver_data.ipmi_devices, head) {
+		device = ipmi_device->device;
+		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;
+		}
+		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;
+			}
+		}
+	}
+}
+
+static void ipmi_bmc_gone(int iface)
+{
+	struct acpi_ipmi_device *ipmi_device, *temp;
+
+	if (list_empty(&driver_data.ipmi_devices))
+		return;
+
+	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);
+		}
+	}
+}
+/* --------------------------------------------------------------------------
+ * 			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 = NULL;
+	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) {
+		/* Read function is not supported. AE_TYPE is returned. */
+		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)) {
+		printk(KERN_DEBUG "Can't register IPMI opregion %s\n",
+					acpi_device_bid(ipmi->device));
+		return -EINVAL;
+	}
+
+	test_bit(IPMI_FLAGS_HANDLER_INSTALL, &ipmi->flags);
+
+	return 0;
+}
+
+static void acpi_register_ipmi_handler(struct acpi_ipmi_device *ipmi_device)
+{
+
+	mutex_init(&ipmi_device->mutex_lock);
+	INIT_LIST_HEAD(&ipmi_device->tx_msg_list);
+
+	if (ipmi_install_handler(ipmi_device)) {
+		/*
+		 * can't register the IPMI opregion. We will print
+		 * some debug info and continue to register ipmi
+		 * opregion for next device.
+		 */
+		printk(KERN_DEBUG "Can't register IPMI opregion %s\n",
+				acpi_device_bid(ipmi_device->device));
+	}
+	list_add(&ipmi_device->head, &driver_data.ipmi_devices);
+}
+
+static void acpi_remove_ipmi_handler(struct acpi_ipmi_device *ipmi_device)
+{
+	if (!ipmi_device)
+		return;
+
+	if (ipmi_device->user_interface) {
+		/*
+		 * If the IPMI user interface is created, it
+		 * should be destroyed.
+		 */
+		ipmi_destroy_user(ipmi_device->user_interface);
+		ipmi_device->user_interface = NULL;
+	}
+	list_del(&ipmi_device->head);
+
+	if (!list_empty(&ipmi_device->tx_msg_list))
+		ipmi_destroy_tx_msg(ipmi_device);
+
+	ipmi_remove_handler(ipmi_device);
+	kfree(ipmi_device);
+}
+
+static int acpi_ipmi_opregion_init(void)
+{
+	int result = 0;
+
+	if (acpi_disabled)
+		return result;
+
+	result = ipmi_smi_watcher_register(&driver_data.bmc_events);
+
+	return result;
+}
+
+static void acpi_ipmi_opregion_exit(void)
+{
+	if (acpi_disabled)
+		return;
+
+	ipmi_smi_watcher_unregister(&driver_data.bmc_events);
+
+}
+
 /*
  * Once we get an ACPI failure, we don't try any more, because we go
  * through the tables sequentially.  Once we don't find a table, there
@@ -2033,6 +2498,8 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
 	acpi_handle handle;
 	acpi_status status;
 	unsigned long long tmp;
+	int ret;
+	struct acpi_ipmi_device *p_ipmi;
 
 	acpi_dev = pnp_acpi_device(dev);
 	if (!acpi_dev)
@@ -2042,6 +2509,13 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
 	if (!info)
 		return -ENOMEM;
 
+	p_ipmi = kzalloc(sizeof(*p_ipmi), GFP_KERNEL);
+	if (!p_ipmi) {
+		printk(KERN_ERR "Can't allocate memory for IPMI device\n");
+		kfree(info);
+		return -ENOMEM;
+	}
+
 	info->addr_source = "ACPI";
 
 	handle = acpi_dev->handle;
@@ -2094,20 +2568,36 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
 	}
 
 	info->dev = &acpi_dev->dev;
-	pnp_set_drvdata(dev, info);
+	p_ipmi->smi_info = info;
+	pnp_set_drvdata(dev, p_ipmi);
 
-	return try_smi_init(info);
+	p_ipmi->device = acpi_dev;
+	p_ipmi->handle = handle;
+	p_ipmi->ipmi_ifnum = -1;
 
+	ret = try_smi_init(info);
+	if (!ret) {
+		acpi_register_ipmi_handler(p_ipmi);
+		return ret;
+	}
 err_free:
+	pnp_set_drvdata(dev, NULL);
+	kfree(p_ipmi);
 	kfree(info);
 	return -EINVAL;
 }
 
 static void __devexit ipmi_pnp_remove(struct pnp_dev *dev)
 {
-	struct smi_info *info = pnp_get_drvdata(dev);
+	struct acpi_ipmi_device *p_ipmi;
+	struct smi_info *info;
+
+	p_ipmi = pnp_get_drvdata(dev);
+	info = p_ipmi->smi_info;
 
+	acpi_remove_ipmi_handler(p_ipmi);
 	cleanup_one_si(info);
+	pnp_set_drvdata(dev, NULL);
 }
 
 static const struct pnp_device_id pnp_dev_table[] = {
@@ -3207,6 +3697,7 @@ static __devinit int init_ipmi_si(void)
 
 #ifdef CONFIG_ACPI
 	spmi_find_bmc();
+	acpi_ipmi_opregion_init();
 #endif
 
 #ifdef CONFIG_PCI
@@ -3327,10 +3818,14 @@ static __exit void cleanup_ipmi_si(void)
 
 	if (!initialized)
 		return;
+#ifdef CONFIG_ACPI
+	acpi_ipmi_opregion_exit();
+#endif
 
 #ifdef CONFIG_PCI
 	pci_unregister_driver(&ipmi_pci_driver);
 #endif
+
 #ifdef CONFIG_PNP
 	pnp_unregister_driver(&ipmi_pnp_driver);
 #endif
-- 
1.5.4.5

Re: [PATCH 3/3] ipmi/acpi: Install the IPMI space handler to enable ACPI to access the BMC controller

[Bjorn Helgaas]

On Wednesday 16 December 2009 06:52:00 pm ykzhao wrote:
> On Thu, 2009-12-17 at 00:40 +0800, Bjorn Helgaas wrote:

> > > +     struct acpi_ipmi_device *ipmi_device =
> > > +                     (struct acpi_ipmi_device *) handler_context;
> > 
> > I don't think you need this cast.
> 
> Without the initialization or cast type, it still can work. But it is
> not wrong.
> Right? Of course I can remove it.

The initialization and cast aren't *wrong*,  they're just unnecessary.
In this case, it also makes it look different from other Linux code,
which makes it harder to read.

> > > +static int acpi_ipmi_opregion_init(void)
> > > +{
> > > +     int result = 0;
> > > +
> > > +     if (acpi_disabled)
> > > +             return result;
> > 
> > I don't think you need this check.
> 
> It is also ok to delete this check. 
> But in fact it is unnecessary to register the smi_watcher again when
> ACPI is disabled.  Otherwise after we register it, it will call the
> corresponding callback defined in smi_watcher when one IPMI system
> interface is registered/unregistered. 
> 
> So IMO we can register the smi_watcher only when ACPI is enabled.

This is connected with my opinion that you should call
acpi_ipmi_opregion_init() from ipmi_pnp_probe(), not from the
driver init function.

We only need the watcher when we have an IPI0001 device, so I
don't see the point of registering it before we find one.  And if
we find one, we know ACPI is enabled, so we don't need to check
acpi_disabled.

> > > @@ -2096,17 +2576,33 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
> > >       info->dev = &acpi_dev->dev;
> > >       pnp_set_drvdata(dev, info);
> > >
> > > -     return try_smi_init(info);
> > > +     acpi_dev->driver_data = p_ipmi;
> > 
> > I don't think we should use both the pnp_dev driver_data
> > and the acpi_dev driver_data.  There's only one underlying
> > physical device, and the fact that we have two ways to get
> > at it is a Linux design problem.  It would be better to add
> > the p_ipmi pointer to the smi_info structure, and use only
> > pnp_set_drvdata().
> 
> Understand what you said.
> 
> It is not a good idea to add a new pointer in smi_info structure. In
> fact the smi_info is the abstract layer of IPMI system interface. To add
> a definition in smi_info seems a bit confusing.

If you can figure out another place to put it, that's OK with me.
I just don't want to use acpi_dev->driver_data because someday I
want to get rid of the acpi_dev for PNP devices.  We shouldn't have
both a pnp_dev and and acpi_dev for the same device.

> > > @@ -3207,6 +3703,7 @@ static __devinit int init_ipmi_si(void)
> > >
> > >  #ifdef CONFIG_ACPI
> > >       spmi_find_bmc();
> > > +     acpi_ipmi_opregion_init();
> > 
> > I don't think this belongs in the IPMI driver init.  It should be done
> > in ipmi_pnp_probe() when we actually discover an IPI0001 device.
> 
> This function is mainly to register the smi_watcher. When one IPMI
> system interface(smi_info) is registered/unregistered, it will call the
> corresponding callback function to create/destroy the corresponding IPMI
> user interface between the user and IPMI system interface.(In our code
> the user is the ACPI IPMI device defined in ACPI namespace). 
> That means that it can create the corresponding user interface for more
> than one ACPI IPMI devices. 
> So IMO this had better not be added in the ipmi_pnp_probe/remove
> function.

We don't need the watcher unless we find an IPI0001 device, so I think
it makes sense to put it in ipmi_pnp_probe().  You might have to have
some static state so you only call acpi_ipmi_opregion_init() on the
*first* probe and do the cleanup on the *last* call to ipmi_pnp_remove().

Thanks for being so patient with all this.  I know I'm being picky,
but it's much easier to fix up as much as we can before it goes into
the tree.

Bjorn

Re: [PATCH 3/3] ipmi/acpi: Install the IPMI space handler to enable ACPI to access the BMC controller

[Bjorn Helgaas]

On Wednesday 16 December 2009 07:40:31 am yakui.zhao@intel.com wrote:
> From: Zhao Yakui <yakui.zhao@intel.com>
> 
> 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.

Oh, BTW, it's handy if you add a [0/n] email with the overview, and
put the V2->V3 stuff in there.  The [0/n] text doesn't become part of
the commit, so the logs don't get cluttered with all the development
history.  Also, please include the version in the subject somewhere,
e.g., "[PATCH v3 0/n] ..."

Bjorn

Re: [PATCH 3/3] ipmi/acpi: Install the IPMI space handler to enable ACPI to access the BMC controller

[ykzhao]

On Fri, 2009-12-18 at 06:00 +0800, Bjorn Helgaas wrote:
> On Wednesday 16 December 2009 06:52:00 pm ykzhao wrote:
> > On Thu, 2009-12-17 at 00:40 +0800, Bjorn Helgaas wrote:
> 
> > > > +     struct acpi_ipmi_device *ipmi_device =
> > > > +                     (struct acpi_ipmi_device *) handler_context;
> > > 
> > > I don't think you need this cast.
> > 
> > Without the initialization or cast type, it still can work. But it is
> > not wrong.
> > Right? Of course I can remove it.
> 
> The initialization and cast aren't *wrong*,  they're just unnecessary.
> In this case, it also makes it look different from other Linux code,
> which makes it harder to read.
> 
> > > > +static int acpi_ipmi_opregion_init(void)
> > > > +{
> > > > +     int result = 0;
> > > > +
> > > > +     if (acpi_disabled)
> > > > +             return result;
> > > 
> > > I don't think you need this check.
> > 
> > It is also ok to delete this check. 
> > But in fact it is unnecessary to register the smi_watcher again when
> > ACPI is disabled.  Otherwise after we register it, it will call the
> > corresponding callback defined in smi_watcher when one IPMI system
> > interface is registered/unregistered. 
> > 
> > So IMO we can register the smi_watcher only when ACPI is enabled.
> 
> This is connected with my opinion that you should call
> acpi_ipmi_opregion_init() from ipmi_pnp_probe(), not from the
> driver init function.
> 
> We only need the watcher when we have an IPI0001 device, so I
> don't see the point of registering it before we find one.  And if
> we find one, we know ACPI is enabled, so we don't need to check
> acpi_disabled.
> 
> > > > @@ -2096,17 +2576,33 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
> > > >       info->dev = &acpi_dev->dev;
> > > >       pnp_set_drvdata(dev, info);
> > > >
> > > > -     return try_smi_init(info);
> > > > +     acpi_dev->driver_data = p_ipmi;
> > > 
> > > I don't think we should use both the pnp_dev driver_data
> > > and the acpi_dev driver_data.  There's only one underlying
> > > physical device, and the fact that we have two ways to get
> > > at it is a Linux design problem.  It would be better to add
> > > the p_ipmi pointer to the smi_info structure, and use only
> > > pnp_set_drvdata().
> > 
> > Understand what you said.
> > 
> > It is not a good idea to add a new pointer in smi_info structure. In
> > fact the smi_info is the abstract layer of IPMI system interface. To add
> > a definition in smi_info seems a bit confusing.
> 
> If you can figure out another place to put it, that's OK with me.
> I just don't want to use acpi_dev->driver_data because someday I
> want to get rid of the acpi_dev for PNP devices.  We shouldn't have
> both a pnp_dev and and acpi_dev for the same device.
> 
> > > > @@ -3207,6 +3703,7 @@ static __devinit int init_ipmi_si(void)
> > > >
> > > >  #ifdef CONFIG_ACPI
> > > >       spmi_find_bmc();
> > > > +     acpi_ipmi_opregion_init();
> > > 
> > > I don't think this belongs in the IPMI driver init.  It should be done
> > > in ipmi_pnp_probe() when we actually discover an IPI0001 device.
> > 
> > This function is mainly to register the smi_watcher. When one IPMI
> > system interface(smi_info) is registered/unregistered, it will call the
> > corresponding callback function to create/destroy the corresponding IPMI
> > user interface between the user and IPMI system interface.(In our code
> > the user is the ACPI IPMI device defined in ACPI namespace). 
> > That means that it can create the corresponding user interface for more
> > than one ACPI IPMI devices. 
> > So IMO this had better not be added in the ipmi_pnp_probe/remove
> > function.
> 
> We don't need the watcher unless we find an IPI0001 device, so I think
> it makes sense to put it in ipmi_pnp_probe().  You might have to have
> some static state so you only call acpi_ipmi_opregion_init() on the
> *first* probe and do the cleanup on the *last* call to ipmi_pnp_remove().

If no IPI0001 device is not found, it is unnecessary to register the
smi_watcher. When the IPI0001 device is found, we should register the
smi_watcher. I think that your suggestion is also OK. 

But if we put it outside of ipmi_pnp_probe/ipmp_pnp_remove, then we
don't care when and how to register/unregister it. In such case if a new
IPMI smi_info is registered, it will call this smi_watcher.

In fact when we call the function of ipmi_smi_watcher_register to
register the smi_watcher, it will do two things. One is to enumerate the
current smi_info list to call the callback function. The second is to
add this smi_watcher and when one IPMI smi_info is registered, it can be
called.

Of course I can remove the unmeaningful check of acpi_disabled.


> 
> Thanks for being so patient with all this.  I know I'm being picky,
> but it's much easier to fix up as much as we can before it goes into
> the tree.
> 
> Bjorn