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

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

[yakui.zhao]

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

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

[Patch 01/02]: Export symbol of pnp_bus_type

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

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

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

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

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

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

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

Best regards.
   Yakui

[PATCH 1/2_v7] ACPI: Export symbol of pnp_bus_type

[yakui.zhao]

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

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

diff --git a/drivers/pnp/driver.c b/drivers/pnp/driver.c
index cd11b11..9aec98a 100644
--- a/drivers/pnp/driver.c
+++ b/drivers/pnp/driver.c
@@ -218,6 +218,8 @@ struct bus_type pnp_bus_type = {
 	.dev_attrs = pnp_interface_attrs,
 };
 
+EXPORT_SYMBOL(pnp_bus_type);
+
 int pnp_register_driver(struct pnp_driver *drv)
 {
 	drv->driver.name = drv->name;
-- 
1.5.4.5

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

[yakui.zhao]

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

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

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

The following describes how to process the IPMI request in IPMI space handler:
    1. format the IPMI message based on the request in AML code.
    IPMI system address. Now the address type is SYSTEM_INTERFACE_ADDR_TYPE
    IPMI net function & command
    IPMI message payload
    2. send the IPMI message by using the function of ipmi_request_settime
    3. wait for the completion of IPMI message.
    4. format the IPMI response and return it to ACPI AML code.

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

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

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 7464115..0fa6a01 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -204,6 +204,16 @@ config ACPI_PROCESSOR
 
 	  To compile this driver as a module, choose M here:
 	  the module will be called processor.
+config ACPI_IPMI
+	tristate "IPMI"
+	depends on EXPERIMENTAL && IPMI_SI && IPMI_HANDLER
+	default n
+	help
+	  This driver enables the ACPI to access the BMC controller. And it
+	  uses the IPMI request/response message to communicate with BMC
+	  controller, which can be found on on the server.
+
+	  To compile this driver as a module, choose M here:
 
 config ACPI_HOTPLUG_CPU
 	bool
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 6ee3316..5ca31f1 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -67,5 +67,6 @@ processor-y			+= processor_idle.o processor_thermal.o
 processor-$(CONFIG_CPU_FREQ)	+= processor_perflib.o
 
 obj-$(CONFIG_ACPI_PROCESSOR_AGGREGATOR) += acpi_pad.o
+obj-$(CONFIG_ACPI_IPMI)		+= acpi_ipmi.o
 
 obj-$(CONFIG_ACPI_APEI)		+= apei/
diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c
new file mode 100644
index 0000000..3e674df
--- /dev/null
+++ b/drivers/acpi/acpi_ipmi.c
@@ -0,0 +1,551 @@
+/*
+ *  ipmi.c - ACPI IPMI opregion
+ *
+ *  Copyright (C) 2010 Intel Corporation
+ *  Copyright (C) 2010 Zhao Yakui <yakui.zhao@intel.com>
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or (at
+ *  your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful, but
+ *  WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/types.h>
+#include <linux/delay.h>
+#include <linux/proc_fs.h>
+#include <linux/seq_file.h>
+#include <linux/interrupt.h>
+#include <linux/list.h>
+#include <linux/spinlock.h>
+#include <linux/io.h>
+#include <acpi/acpi_bus.h>
+#include <acpi/acpi_drivers.h>
+#include <linux/ipmi.h>
+#include <linux/device.h>
+#include <linux/pnp.h>
+
+MODULE_AUTHOR("Zhao Yakui");
+MODULE_DESCRIPTION("ACPI IPMI Opregion driver");
+MODULE_LICENSE("GPL");
+
+#define IPMI_FLAGS_HANDLER_INSTALL	0
+
+#define ACPI_IPMI_OK			0
+#define ACPI_IPMI_TIMEOUT		0x10
+#define ACPI_IPMI_UNKNOWN		0x07
+/* the IPMI timeout is 5s */
+#define IPMI_TIMEOUT			(5 * HZ)
+
+struct acpi_ipmi_device {
+	/* the device list attached to driver_data.ipmi_devices */
+	struct list_head head;
+	/* the IPMI request message list */
+	struct list_head tx_msg_list;
+	acpi_handle handle;
+	struct pnp_dev	*pnp_dev;
+	ipmi_user_t	user_interface;
+	struct mutex	mutex_lock;
+	int ipmi_ifnum; /* IPMI interface number */
+	long curr_msgid;
+	unsigned long flags;
+};
+
+struct ipmi_driver_data {
+	int device_count;
+	struct list_head	ipmi_devices;
+	struct ipmi_smi_watcher	bmc_events;
+	struct ipmi_user_hndl	ipmi_hndlrs;
+};
+
+struct acpi_ipmi_msg {
+	struct list_head head;
+	/*
+	 * General speaking the addr type should be SI_ADDR_TYPE. And
+	 * the addr channel should be BMC.
+	 * In fact it can also be IPMB type. But we will have to
+	 * parse it from the Netfn command buffer. It is so complex
+	 * that it is skipped.
+	 */
+	struct ipmi_addr addr;
+	long tx_msgid;
+	/* it is used to track whether the IPMI message is finished */
+	struct completion tx_complete;
+	struct kernel_ipmi_msg tx_message;
+	int	msg_done;
+	/* tx data . And copy it from ACPI object buffer */
+	u8	tx_data[64];
+	int	tx_len;
+	u8	rx_data[64];
+	int	rx_len;
+	struct acpi_ipmi_device *device;
+};
+
+/* IPMI request/response buffer per ACPI 4.0, sec 5.5.2.4.3.2 */
+struct acpi_ipmi_buffer {
+	u8 status;
+	u8 length;
+	u8 data[64];
+};
+
+static void ipmi_register_bmc(int iface, struct device *dev);
+static void ipmi_bmc_gone(int iface);
+static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data);
+
+static struct ipmi_driver_data driver_data = {
+	.ipmi_devices = LIST_HEAD_INIT(driver_data.ipmi_devices),
+	.bmc_events = {
+		.owner = THIS_MODULE,
+		.new_smi = ipmi_register_bmc,
+		.smi_gone = ipmi_bmc_gone,
+	},
+	.ipmi_hndlrs = {
+		.ipmi_recv_hndl = ipmi_msg_handler,
+	},
+};
+
+
+struct acpi_ipmi_msg *acpi_alloc_ipmi_msg(struct acpi_ipmi_device *ipmi)
+{
+	struct acpi_ipmi_msg *ipmi_msg;
+	struct pnp_dev *pnp_dev = ipmi->pnp_dev;
+
+	ipmi_msg = kzalloc(sizeof(struct acpi_ipmi_msg), GFP_KERNEL);
+	if (!ipmi_msg)	{
+		dev_warn(&pnp_dev->dev, "Can't allocate memory for ipmi_msg\n");
+		return NULL;
+	}
+	init_completion(&ipmi_msg->tx_complete);
+	INIT_LIST_HEAD(&ipmi_msg->head);
+	ipmi_msg->device = ipmi;
+	return ipmi_msg;
+}
+
+#define		IPMI_OP_RGN_NETFN(offset)	((offset >> 8) & 0xff)
+#define		IPMI_OP_RGN_CMD(offset)		(offset & 0xff)
+static void acpi_format_ipmi_msg(struct acpi_ipmi_msg *tx_msg,
+				acpi_physical_address address,
+				acpi_integer *value)
+{
+	struct kernel_ipmi_msg *msg;
+	struct acpi_ipmi_buffer *buffer;
+	struct acpi_ipmi_device *device;
+
+	msg = &tx_msg->tx_message;
+	/*
+	 * IPMI network function and command are encoded in the address
+	 * within the IPMI OpRegion; see ACPI 4.0, sec 5.5.2.4.3.
+	 */
+	msg->netfn = IPMI_OP_RGN_NETFN(address);
+	msg->cmd = IPMI_OP_RGN_CMD(address);
+	msg->data = tx_msg->tx_data;
+	/*
+	 * value is the parameter passed by the IPMI opregion space handler.
+	 * It points to the IPMI request message buffer
+	 */
+	buffer = (struct acpi_ipmi_buffer *)value;
+	/* copy the tx message data */
+	msg->data_len = buffer->length;
+	memcpy(tx_msg->tx_data, buffer->data, msg->data_len);
+	/*
+	 * now the default type is SYSTEM_INTERFACE and channel type is BMC.
+	 * If the netfn is APP_REQUEST and the cmd is SEND_MESSAGE,
+	 * the addr type should be changed to IPMB. Then we will have to parse
+	 * the IPMI request message buffer to get the IPMB address.
+	 * If so, please fix me.
+	 */
+	tx_msg->addr.addr_type = IPMI_SYSTEM_INTERFACE_ADDR_TYPE;
+	tx_msg->addr.channel = IPMI_BMC_CHANNEL;
+	tx_msg->addr.data[0] = 0;
+
+	/* Get the msgid */
+	device = tx_msg->device;
+	mutex_lock(&device->mutex_lock);
+	device->curr_msgid++;
+	tx_msg->tx_msgid = device->curr_msgid;
+	mutex_unlock(&device->mutex_lock);
+}
+
+static void acpi_format_ipmi_response(struct acpi_ipmi_msg *msg,
+		acpi_integer *value, int 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;
+	if (!timeout && !msg->msg_done) {
+		buffer->status = ACPI_IPMI_TIMEOUT;
+		return;
+	}
+	/*
+	 * If the flag of msg_done is not set or the recv length is zero, it
+	 * means that the IPMI command is not executed correctly.
+	 * The status code will be ACPI_IPMI_UNKNOWN.
+	 */
+	if (!msg->msg_done || !msg->rx_len) {
+		buffer->status = ACPI_IPMI_UNKNOWN;
+		return;
+	}
+	/*
+	 * If the IPMI response message is obtained correctly, the status code
+	 * will be ACPI_IPMI_OK
+	 */
+	buffer->status = ACPI_IPMI_OK;
+	buffer->length = msg->rx_len;
+	memcpy(buffer->data, msg->rx_data, msg->rx_len);
+	return;
+}
+static void ipmi_destroy_tx_msg(struct acpi_ipmi_device *ipmi)
+{
+	struct acpi_ipmi_msg *tx_msg = NULL, *temp;
+	int count = 20;
+	struct pnp_dev *pnp_dev = ipmi->pnp_dev;
+
+	list_for_each_entry_safe(tx_msg, temp, &ipmi->tx_msg_list, head) {
+		/* wake up the sleep thread on the Tx msg */
+		complete(&tx_msg->tx_complete);
+	}
+
+	/* wait for about 20 ticks to flush the tx message list */
+	while (count--) {
+		if (list_empty(&ipmi->tx_msg_list))
+			break;
+		schedule_timeout(1);
+	}
+	if (!list_empty(&ipmi->tx_msg_list))
+		dev_warn(&pnp_dev->dev, "tx msg list is not NULL\n");
+
+}
+
+static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data)
+{
+	struct acpi_ipmi_device *ipmi_device =
+			(struct acpi_ipmi_device *)user_msg_data;
+	int msg_found = 0;
+	struct acpi_ipmi_msg *tx_msg = NULL;
+	struct pnp_dev *pnp_dev = ipmi_device->pnp_dev;
+
+	if (msg->user != ipmi_device->user_interface) {
+		dev_warn(&pnp_dev->dev, "Incorrect IPMI user\n");
+		ipmi_free_recv_msg(msg);
+		return;
+	}
+	mutex_lock(&ipmi_device->mutex_lock);
+	list_for_each_entry(tx_msg, &ipmi_device->tx_msg_list, head) {
+		if (msg->msgid == tx_msg->tx_msgid) {
+			/* find the message id */
+			msg_found = 1;
+			break;
+		}
+	}
+
+	mutex_unlock(&ipmi_device->mutex_lock);
+	if (!msg_found) {
+		/* no matched msg is found . But we should free it */
+		ipmi_free_recv_msg(msg);
+		dev_warn(&pnp_dev->dev, "Incorrect MSG is found\n");
+		return;
+	}
+
+	if (msg->msg.data_len) {
+		/* copy the response data to Rx_data buffer */
+		memcpy(tx_msg->rx_data, msg->msg_data, msg->msg.data_len);
+		tx_msg->rx_len = msg->msg.data_len;
+		tx_msg->msg_done = 1;
+	}
+	complete(&tx_msg->tx_complete);
+	ipmi_free_recv_msg(msg);
+};
+
+static void ipmi_register_bmc(int iface, struct device *dev)
+{
+	struct acpi_ipmi_device *ipmi_device;
+	struct pnp_dev *device;
+	ipmi_user_t		user;
+	int err;
+
+	if (list_empty(&driver_data.ipmi_devices))
+		return;
+
+	list_for_each_entry(ipmi_device, &driver_data.ipmi_devices, head) {
+		if (ipmi_device->user_interface) {
+			/*
+			 * Only one user interface is allowed to be registered
+			 * for one IPMI device.
+			 * If we already create the user interface for
+			 * one IPMI device, skip it
+			 */
+			continue;
+		}
+		device = ipmi_device->pnp_dev;
+		if (dev == &device->dev) {
+			/*
+			 * If the dev is identical to the ACPI device,
+			 * create the user interface.
+			 */
+			err = ipmi_create_user(iface, &driver_data.ipmi_hndlrs,
+						ipmi_device, &user);
+			if (err == 0) {
+				ipmi_device->user_interface = user;
+				ipmi_device->ipmi_ifnum = iface;
+			}
+
+			break;
+		}
+	}
+}
+
+static void ipmi_bmc_gone(int iface)
+{
+	struct acpi_ipmi_device *ipmi_device, *temp;
+
+	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;
+	struct acpi_ipmi_device *ipmi_device =
+			(struct acpi_ipmi_device *) handler_context;
+	int err;
+	acpi_status status;
+	/*
+	 * IPMI opregion message.
+	 * IPMI message is firstly written to the BMC and system software
+	 * can get the respsonse. So it is unmeaningful for the IPMI read
+	 * access.
+	 */
+	if ((function & ACPI_IO_MASK) == ACPI_READ)
+		return AE_TYPE;
+
+	if (!ipmi_device->user_interface)
+		return AE_NOT_EXIST;
+
+	tx_msg = acpi_alloc_ipmi_msg(ipmi_device);
+	if (!tx_msg)
+		return AE_NO_MEMORY;
+
+	acpi_format_ipmi_msg(tx_msg, address, value);
+	mutex_lock(&ipmi_device->mutex_lock);
+	list_add_tail(&tx_msg->head, &ipmi_device->tx_msg_list);
+	mutex_unlock(&ipmi_device->mutex_lock);
+	err = ipmi_request_settime(ipmi_device->user_interface,
+					&tx_msg->addr,
+					tx_msg->tx_msgid,
+					&tx_msg->tx_message,
+					NULL, 0, 0, 0);
+	if (err) {
+		status = AE_ERROR;
+		goto end_label;
+	}
+	err = wait_for_completion_timeout(&tx_msg->tx_complete, IPMI_TIMEOUT);
+	acpi_format_ipmi_response(tx_msg, value, err);
+	status = AE_OK;
+
+end_label:
+	mutex_lock(&ipmi_device->mutex_lock);
+	list_del(&tx_msg->head);
+	mutex_unlock(&ipmi_device->mutex_lock);
+	kfree(tx_msg);
+	return status;
+}
+
+static void ipmi_remove_handler(struct acpi_ipmi_device *ipmi)
+{
+	if (!test_bit(IPMI_FLAGS_HANDLER_INSTALL, &ipmi->flags))
+		return;
+	acpi_remove_address_space_handler(ipmi->handle,
+				ACPI_ADR_SPACE_IPMI, &acpi_ipmi_space_handler);
+
+	clear_bit(IPMI_FLAGS_HANDLER_INSTALL, &ipmi->flags);
+}
+
+static int ipmi_install_handler(struct acpi_ipmi_device *ipmi)
+{
+	acpi_status status;
+
+	if (test_bit(IPMI_FLAGS_HANDLER_INSTALL, &ipmi->flags))
+		return 0;
+
+	status = acpi_install_address_space_handler(ipmi->handle,
+						    ACPI_ADR_SPACE_IPMI,
+						    &acpi_ipmi_space_handler,
+						    NULL, ipmi);
+	if (ACPI_FAILURE(status)) {
+		struct pnp_dev *pnp_dev = ipmi->pnp_dev;
+		dev_warn(&pnp_dev->dev, "Can't register IPMI opregion for %s\n",
+					pnp_dev_name(pnp_dev));
+		return -EINVAL;
+	}
+	set_bit(IPMI_FLAGS_HANDLER_INSTALL, &ipmi->flags);
+	return 0;
+}
+
+
+static const struct pnp_device_id ipmi_device_ids[] = {
+	{"IPI0001", 0},
+	{"", 0},
+};
+
+static int match_device(struct pnp_dev *dev, const struct pnp_device_id *ids)
+{
+	const struct pnp_device_id *id;
+	struct pnp_id *pnp_id;
+
+	for (id = ids;  id->id[0]; id++) {
+		pnp_id = dev->id;
+		if (!pnp_id)
+			return 0;
+
+		while (pnp_id) {
+			if (!strncasecmp(pnp_id->id, (char *)id->id,
+					strlen((char *)id->id)))
+				return 1;
+			pnp_id = pnp_id->next;
+		};
+	}
+	return 0;
+}
+
+static int pnp_ipmi_match(struct device *dev, void *data)
+{
+	struct pnp_dev *pnp_dev;
+	struct acpi_ipmi_device *ipmi_device;
+	struct acpi_device *acpi_dev;
+
+	pnp_dev = to_pnp_dev(dev);
+
+	if (!match_device(pnp_dev, ipmi_device_ids))
+		return 0;
+
+	ipmi_device = kzalloc(sizeof(*ipmi_device), GFP_KERNEL);
+	if (!ipmi_device) {
+		dev_warn(dev, "Can't allocate memory for acpi_ipmi_device\n");
+		return -ENOMEM;
+	}
+	acpi_dev = pnp_acpi_device(pnp_dev);
+	INIT_LIST_HEAD(&ipmi_device->head);
+	ipmi_device->pnp_dev = pnp_dev;
+	ipmi_device->handle = acpi_dev->handle;
+
+	list_add_tail(&ipmi_device->head, &driver_data.ipmi_devices);
+	return 0;
+}
+
+static void acpi_add_ipmi_devices(void)
+{
+	struct acpi_ipmi_device *ipmi_device;
+
+	bus_for_each_dev(&pnp_bus_type, NULL, NULL, pnp_ipmi_match);
+	if (list_empty(&driver_data.ipmi_devices))
+		return;
+
+	list_for_each_entry(ipmi_device, &driver_data.ipmi_devices, head) {
+		mutex_init(&ipmi_device->mutex_lock);
+		INIT_LIST_HEAD(&ipmi_device->tx_msg_list);
+		ipmi_install_handler(ipmi_device);
+	}
+}
+
+static void acpi_remove_ipmi_handlers(void)
+{
+	struct acpi_ipmi_device *ipmi_device, *temp;
+
+	list_for_each_entry_safe(ipmi_device, temp, &driver_data.ipmi_devices,
+					head) {
+		if (ipmi_device->user_interface) {
+			/*
+			 * 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 __init acpi_ipmi_init(void)
+{
+	int result = 0;
+
+	if (acpi_disabled)
+		return result;
+
+	acpi_add_ipmi_devices();
+
+	result = ipmi_smi_watcher_register(&driver_data.bmc_events);
+
+	if (result)
+		acpi_remove_ipmi_handlers();
+
+	return result;
+}
+
+static void __exit acpi_ipmi_exit(void)
+{
+	if (acpi_disabled)
+		return;
+
+	acpi_remove_ipmi_handlers();
+	ipmi_smi_watcher_unregister(&driver_data.bmc_events);
+
+	return;
+}
+
+module_init(acpi_ipmi_init);
+module_exit(acpi_ipmi_exit);
-- 
1.5.4.5

Re: [PATCH 1/2_v7] ACPI: Export symbol of pnp_bus_type

[Bjorn Helgaas]

On Wednesday, July 14, 2010 06:54:53 am yakui.zhao@intel.com wrote:
> From: Zhao Yakui <yakui.zhao@intel.com>

This should have a changelog saying *why* we need to export
pnp_bus_type.

I looked it up myself, and I see that the next patch adds
acpi_ipmi, which can be a module, and acpi_ipmi references
pnp_bus_type.

I don't like this exposure of pnp_bus_type.  I don't think
drivers should be using it.  I'll comment more in a response
to the next patch.

Bjorn

> Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
> cc: Bjorn Helgaas <bjorn.helgaas@hp.com>
> ---
>  drivers/pnp/driver.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/pnp/driver.c b/drivers/pnp/driver.c
> index cd11b11..9aec98a 100644
> --- a/drivers/pnp/driver.c
> +++ b/drivers/pnp/driver.c
> @@ -218,6 +218,8 @@ struct bus_type pnp_bus_type = {
>  	.dev_attrs = pnp_interface_attrs,
>  };
>  
> +EXPORT_SYMBOL(pnp_bus_type);
> +
>  int pnp_register_driver(struct pnp_driver *drv)
>  {
>  	drv->driver.name = drv->name;
> 

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

[Bjorn Helgaas]

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

This is missing the usual "the module will be called acpi_ipmi"
text.

But I don't think this is the right design.  I think it's fine to
put all this code in a separate file and have a separate config
option for it.

However, I don't think acpi_ipmi and ipmi_si should be different
modules, because they both want to be connected to the same ACPI
device.  Making acpi_ipmi a separate module means you have to
mess around with PNP core things that drivers shouldn't be using.
For example, you have match_device(), pnp_ipmi_match(), and
acpi_add_ipmi_devices() below.  These all reimplement things
the PNP core already does.

The problem with replicating this in acpi_ipmi is that it breaks
the modularity of PNP.  It means that hotplug of ACPI devices
won't work right, because acpi_ipmi won't find out about additions
or removals of IPI0001 devices.

This ACPI opregion stuff is basically an optional feature of
whatever driver claims IPI0001 devices, and I think we need to
set up and tear down the opregion handler at the same point we
claim and release the IPI0001 devices, i.e., in ipmi_si_intf.c

> +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;
> +	if (!timeout && !msg->msg_done) {
> +		buffer->status = ACPI_IPMI_TIMEOUT;
> +		return;
> +	}
> +	/*
> +	 * If the flag of msg_done is not set or the recv length is zero, it
> +	 * means that the IPMI command is not executed correctly.
> +	 * The status code will be ACPI_IPMI_UNKNOWN.
> +	 */
> +	if (!msg->msg_done || !msg->rx_len) {
> +		buffer->status = ACPI_IPMI_UNKNOWN;
> +		return;
> +	}
> +	/*
> +	 * If the IPMI response message is obtained correctly, the status code
> +	 * will be ACPI_IPMI_OK
> +	 */
> +	buffer->status = ACPI_IPMI_OK;
> +	buffer->length = msg->rx_len;
> +	memcpy(buffer->data, msg->rx_data, msg->rx_len);
> +	return;

You never need a "return" at the end of a void function.

> +}
> +static void ipmi_destroy_tx_msg(struct acpi_ipmi_device *ipmi)
> +{
> +	struct acpi_ipmi_msg *tx_msg = NULL, *temp;

No need to initialize tx_msg here; list_for_each_entry_safe()
always sets it before you can use it.

> +	int count = 20;
> +	struct pnp_dev *pnp_dev = ipmi->pnp_dev;
> +
> +	list_for_each_entry_safe(tx_msg, temp, &ipmi->tx_msg_list, head) {
> +		/* wake up the sleep thread on the Tx msg */
> +		complete(&tx_msg->tx_complete);
> +	}
> +
> +	/* wait for about 20 ticks to flush the tx message list */
> +	while (count--) {
> +		if (list_empty(&ipmi->tx_msg_list))
> +			break;
> +		schedule_timeout(1);

The length of time this waits depends on the arch's jiffy length.
The jiffy length is completely independent of how long it might
take to flush the message list.  Instead of waiting for "1" jiffy,
you should use some expression involving HZ, or maybe use
msecs_to_jiffies().

> +	}
> +	if (!list_empty(&ipmi->tx_msg_list))
> +		dev_warn(&pnp_dev->dev, "tx msg list is not NULL\n");
> +
> +}
> +
> +static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data)
> +{
> +	struct acpi_ipmi_device *ipmi_device =
> +			(struct acpi_ipmi_device *)user_msg_data;

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

> +	int msg_found = 0;
> +	struct acpi_ipmi_msg *tx_msg = NULL;

No need to initialize tx_msg here.

> +	struct pnp_dev *pnp_dev = ipmi_device->pnp_dev;
> +
> +	if (msg->user != ipmi_device->user_interface) {
> +		dev_warn(&pnp_dev->dev, "Incorrect IPMI user\n");

Include the user info here, e.g.,

    dev_warn(..., "unexpected user %#x (expected %#x)\n", msg->user,
             ipmi_device->user_interface);

> +		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 */

Superfluous comment; remove it.

> +			msg_found = 1;
> +			break;
> +		}
> +	}
> +
> +	mutex_unlock(&ipmi_device->mutex_lock);
> +	if (!msg_found) {
> +		/* no matched msg is found . But we should free it */

Superfluous comment; remove it.

> +		ipmi_free_recv_msg(msg);
> +		dev_warn(&pnp_dev->dev, "Incorrect MSG is found\n");

Move the dev_warn() up a line and make it say something like this:

    dev_warn(..., "unexpected response (msg ID %#x)\n", msg->msgid);

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

You don't need to test for list_empty before using
list_for_each_entry().

> +
> +	list_for_each_entry(ipmi_device, &driver_data.ipmi_devices, head) {
> +		if (ipmi_device->user_interface) {
> +			/*
> +			 * Only one user interface is allowed to be registered
> +			 * for one IPMI device.
> +			 * If we already create the user interface for
> +			 * one IPMI device, skip it
> +			 */
> +			continue;
> +		}
> +		device = ipmi_device->pnp_dev;
> +		if (dev == &device->dev) {
> +			/*
> +			 * If the dev is identical to the ACPI device,
> +			 * create the user interface.
> +			 */
> +			err = ipmi_create_user(iface, &driver_data.ipmi_hndlrs,
> +						ipmi_device, &user);
> +			if (err == 0) {
> +				ipmi_device->user_interface = user;
> +				ipmi_device->ipmi_ifnum = iface;
> +			}
> +
> +			break;
> +		}
> +	}
> +}
> +
> +static void ipmi_bmc_gone(int iface)
> +{
> +	struct acpi_ipmi_device *ipmi_device, *temp;
> +
> +	if (list_empty(&driver_data.ipmi_devices))
> +		return;

You don't need to test for list_empty before using
list_for_each_entry_safe().

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

> +	status = acpi_install_address_space_handler(ipmi->handle,
> +						    ACPI_ADR_SPACE_IPMI,
> +						    &acpi_ipmi_space_handler,
> +						    NULL, ipmi);
> +	if (ACPI_FAILURE(status)) {
> +		struct pnp_dev *pnp_dev = ipmi->pnp_dev;
> +		dev_warn(&pnp_dev->dev, "Can't register IPMI opregion for %s\n",
> +					pnp_dev_name(pnp_dev));

I'm actually not sure what pnp_dev_name() is for.  I'd rather not
add more uses of it.  I think dev_warn() prints all the information
we need, even without using pnp_dev_name().

> +static const struct pnp_device_id ipmi_device_ids[] = {
> +	{"IPI0001", 0},
> +	{"", 0},
> +};
> +
> +static int match_device(struct pnp_dev *dev, const struct pnp_device_id *ids)
> +{
> +	const struct pnp_device_id *id;
> +	struct pnp_id *pnp_id;
> +
> +	for (id = ids;  id->id[0]; id++) {
> +		pnp_id = dev->id;
> +		if (!pnp_id)
> +			return 0;
> +
> +		while (pnp_id) {
> +			if (!strncasecmp(pnp_id->id, (char *)id->id,
> +					strlen((char *)id->id)))
> +				return 1;
> +			pnp_id = pnp_id->next;
> +		};
> +	}
> +	return 0;
> +}
> +
> +static int pnp_ipmi_match(struct device *dev, void *data)
> +{
> +	struct pnp_dev *pnp_dev;
> +	struct acpi_ipmi_device *ipmi_device;
> +	struct acpi_device *acpi_dev;
> +
> +	pnp_dev = to_pnp_dev(dev);
> +
> +	if (!match_device(pnp_dev, ipmi_device_ids))
> +		return 0;
> +
> +	ipmi_device = kzalloc(sizeof(*ipmi_device), GFP_KERNEL);
> +	if (!ipmi_device) {
> +		dev_warn(dev, "Can't allocate memory for acpi_ipmi_device\n");
> +		return -ENOMEM;
> +	}
> +	acpi_dev = pnp_acpi_device(pnp_dev);
> +	INIT_LIST_HEAD(&ipmi_device->head);
> +	ipmi_device->pnp_dev = pnp_dev;
> +	ipmi_device->handle = acpi_dev->handle;
> +
> +	list_add_tail(&ipmi_device->head, &driver_data.ipmi_devices);
> +	return 0;
> +}
> +
> +static void acpi_add_ipmi_devices(void)
> +{
> +	struct acpi_ipmi_device *ipmi_device;
> +
> +	bus_for_each_dev(&pnp_bus_type, NULL, NULL, pnp_ipmi_match);
> +	if (list_empty(&driver_data.ipmi_devices))
> +		return;

You don't need to test for an empty list here.

> +
> +	list_for_each_entry(ipmi_device, &driver_data.ipmi_devices, head) {
> +		mutex_init(&ipmi_device->mutex_lock);
> +		INIT_LIST_HEAD(&ipmi_device->tx_msg_list);
> +		ipmi_install_handler(ipmi_device);
> +	}
> +}
> +
> +static void acpi_remove_ipmi_handlers(void)
> +{
> +	struct acpi_ipmi_device *ipmi_device, *temp;
> +
> +	list_for_each_entry_safe(ipmi_device, temp, &driver_data.ipmi_devices,
> +					head) {
> +		if (ipmi_device->user_interface) {
> +			/*
> +			 * 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;

No "return" needed here.

> +}
> +
> +static int __init acpi_ipmi_init(void)
> +{
> +	int result = 0;
> +
> +	if (acpi_disabled)
> +		return result;
> +
> +	acpi_add_ipmi_devices();
> +
> +	result = ipmi_smi_watcher_register(&driver_data.bmc_events);
> +
> +	if (result)
> +		acpi_remove_ipmi_handlers();
> +
> +	return result;
> +}
> +
> +static void __exit acpi_ipmi_exit(void)
> +{
> +	if (acpi_disabled)
> +		return;
> +
> +	acpi_remove_ipmi_handlers();
> +	ipmi_smi_watcher_unregister(&driver_data.bmc_events);
> +
> +	return;

No "return" needed here.

> +}
> +
> +module_init(acpi_ipmi_init);
> +module_exit(acpi_ipmi_exit);
> 

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

[ykzhao]

On Thu, 2010-07-15 at 01:24 +0800, Bjorn Helgaas wrote:
> On Wednesday, July 14, 2010 06:54:54 am yakui.zhao@intel.com wrote:
> > +config ACPI_IPMI
> > +	tristate "IPMI"
> > +	depends on EXPERIMENTAL && IPMI_SI && IPMI_HANDLER
> > +	default n
> > +	help
> > +	  This driver enables the ACPI to access the BMC controller. And it
> > +	  uses the IPMI request/response message to communicate with BMC
> > +	  controller, which can be found on on the server.
> > +
> > +	  To compile this driver as a module, choose M here:
> 
> This is missing the usual "the module will be called acpi_ipmi"
> text.

Will you please look at this thread
http://marc.info/?l=linux-acpi&m=127608894610765&w=4?

Before this patch, I followed your comment and put the ACPI IPMI
opregion code into the IPMI_SI module. 

But in fact the IPMI opregion code has nothing to do with the ipmi
system interface. It is enough to use the IPMI API interface. In such
case maybe it is a better choice to put the IPMI opregion code into the
separate file. (In fact Corey suggests that the IPMI opregion code be
put into the separate file). 

> 
> But I don't think this is the right design.  I think it's fine to
> put all this code in a separate file and have a separate config
> option for it.
> 
> However, I don't think acpi_ipmi and ipmi_si should be different
> modules, because they both want to be connected to the same ACPI
> device.  Making acpi_ipmi a separate module means you have to
> mess around with PNP core things that drivers shouldn't be using.
> For example, you have match_device(), pnp_ipmi_match(), and
> acpi_add_ipmi_devices() below.  These all reimplement things
> the PNP core already does.

If we decide to put the IPMI opregion code into the separate file, the
next is how to *correctly* create the IPMI user interface between ACPI
subsystem and the corresponding BMC controller.The user interface is the
communication channel used to send/receive IPMI message to/from the BMC
controller. 
In such case we will use the IPMI smi watcher to create/destroy the IPMI
user interface between ACPI and the corresponding BMC controller. But it
is noted that we should assure whether the corresponding IPMI BMC
controller is registered by using ACPI detection mechanism when creating
the relationship between ACPI and IPMI BMC controller. This is realized
by comparing the "device handle". And before comparing the "device
handle", maybe we will have to get the corresponding device lists with
the device id "IPI0001".

Do you have other idea about how to *correctly* create the user
interface between ACPI and IPMI BMC controller?

> 
> The problem with replicating this in acpi_ipmi is that it breaks
> the modularity of PNP.  It means that hotplug of ACPI devices
> won't work right, because acpi_ipmi won't find out about additions
> or removals of IPI0001 devices.
> 
> This ACPI opregion stuff is basically an optional feature of
> whatever driver claims IPI0001 devices, and I think we need to
> set up and tear down the opregion handler at the same point we
> claim and release the IPI0001 devices, i.e., in ipmi_si_intf.c

Thanks for your comments about the following code style. I will do some
cleanup next time. But the prerequisite is whether it is necessary to
put the IPMI opregion code  into the separate file. 

> 
> > +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;
> > +	if (!timeout && !msg->msg_done) {
> > +		buffer->status = ACPI_IPMI_TIMEOUT;
> > +		return;
> > +	}
> > +	/*
> > +	 * If the flag of msg_done is not set or the recv length is zero, it
> > +	 * means that the IPMI command is not executed correctly.
> > +	 * The status code will be ACPI_IPMI_UNKNOWN.
> > +	 */
> > +	if (!msg->msg_done || !msg->rx_len) {
> > +		buffer->status = ACPI_IPMI_UNKNOWN;
> > +		return;
> > +	}
> > +	/*
> > +	 * If the IPMI response message is obtained correctly, the status code
> > +	 * will be ACPI_IPMI_OK
> > +	 */
> > +	buffer->status = ACPI_IPMI_OK;
> > +	buffer->length = msg->rx_len;
> > +	memcpy(buffer->data, msg->rx_data, msg->rx_len);
> > +	return;
> 
> You never need a "return" at the end of a void function.
> 
> > +}
> > +static void ipmi_destroy_tx_msg(struct acpi_ipmi_device *ipmi)
> > +{
> > +	struct acpi_ipmi_msg *tx_msg = NULL, *temp;
> 
> No need to initialize tx_msg here; list_for_each_entry_safe()
> always sets it before you can use it.
> 
> > +	int count = 20;
> > +	struct pnp_dev *pnp_dev = ipmi->pnp_dev;
> > +
> > +	list_for_each_entry_safe(tx_msg, temp, &ipmi->tx_msg_list, head) {
> > +		/* wake up the sleep thread on the Tx msg */
> > +		complete(&tx_msg->tx_complete);
> > +	}
> > +
> > +	/* wait for about 20 ticks to flush the tx message list */
> > +	while (count--) {
> > +		if (list_empty(&ipmi->tx_msg_list))
> > +			break;
> > +		schedule_timeout(1);
> 
> The length of time this waits depends on the arch's jiffy length.
> The jiffy length is completely independent of how long it might
> take to flush the message list.  Instead of waiting for "1" jiffy,
> you should use some expression involving HZ, or maybe use
> msecs_to_jiffies().
> 
> > +	}
> > +	if (!list_empty(&ipmi->tx_msg_list))
> > +		dev_warn(&pnp_dev->dev, "tx msg list is not NULL\n");
> > +
> > +}
> > +
> > +static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data)
> > +{
> > +	struct acpi_ipmi_device *ipmi_device =
> > +			(struct acpi_ipmi_device *)user_msg_data;
> 
> You don't need a cast here, since user_msg_data is a "void *".
> 
> > +	int msg_found = 0;
> > +	struct acpi_ipmi_msg *tx_msg = NULL;
> 
> No need to initialize tx_msg here.
> 
> > +	struct pnp_dev *pnp_dev = ipmi_device->pnp_dev;
> > +
> > +	if (msg->user != ipmi_device->user_interface) {
> > +		dev_warn(&pnp_dev->dev, "Incorrect IPMI user\n");
> 
> Include the user info here, e.g.,
> 
>     dev_warn(..., "unexpected user %#x (expected %#x)\n", msg->user,
>              ipmi_device->user_interface);
> 
> > +		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 */
> 
> Superfluous comment; remove it.
> 
> > +			msg_found = 1;
> > +			break;
> > +		}
> > +	}
> > +
> > +	mutex_unlock(&ipmi_device->mutex_lock);
> > +	if (!msg_found) {
> > +		/* no matched msg is found . But we should free it */
> 
> Superfluous comment; remove it.
> 
> > +		ipmi_free_recv_msg(msg);
> > +		dev_warn(&pnp_dev->dev, "Incorrect MSG is found\n");
> 
> Move the dev_warn() up a line and make it say something like this:
> 
>     dev_warn(..., "unexpected response (msg ID %#x)\n", msg->msgid);
> 
> > +		return;
> > +	}
> > +
> > +	if (msg->msg.data_len) {
> > +		/* copy the response data to Rx_data buffer */
> > +		memcpy(tx_msg->rx_data, msg->msg_data, msg->msg.data_len);
> > +		tx_msg->rx_len = msg->msg.data_len;
> > +		tx_msg->msg_done = 1;
> > +	}
> > +	complete(&tx_msg->tx_complete);
> > +	ipmi_free_recv_msg(msg);
> > +};
> > +
> > +static void ipmi_register_bmc(int iface, struct device *dev)
> > +{
> > +	struct acpi_ipmi_device *ipmi_device;
> > +	struct pnp_dev *device;
> > +	ipmi_user_t		user;
> > +	int err;
> > +
> > +	if (list_empty(&driver_data.ipmi_devices))
> > +		return;
> 
> You don't need to test for list_empty before using
> list_for_each_entry().
> 
> > +
> > +	list_for_each_entry(ipmi_device, &driver_data.ipmi_devices, head) {
> > +		if (ipmi_device->user_interface) {
> > +			/*
> > +			 * Only one user interface is allowed to be registered
> > +			 * for one IPMI device.
> > +			 * If we already create the user interface for
> > +			 * one IPMI device, skip it
> > +			 */
> > +			continue;
> > +		}
> > +		device = ipmi_device->pnp_dev;
> > +		if (dev == &device->dev) {
> > +			/*
> > +			 * If the dev is identical to the ACPI device,
> > +			 * create the user interface.
> > +			 */
> > +			err = ipmi_create_user(iface, &driver_data.ipmi_hndlrs,
> > +						ipmi_device, &user);
> > +			if (err == 0) {
> > +				ipmi_device->user_interface = user;
> > +				ipmi_device->ipmi_ifnum = iface;
> > +			}
> > +
> > +			break;
> > +		}
> > +	}
> > +}
> > +
> > +static void ipmi_bmc_gone(int iface)
> > +{
> > +	struct acpi_ipmi_device *ipmi_device, *temp;
> > +
> > +	if (list_empty(&driver_data.ipmi_devices))
> > +		return;
> 
> You don't need to test for list_empty before using
> list_for_each_entry_safe()

IMO it is needed as it does the operation of list deletion.

> 
> > +
> > +	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);
> > +		}
> > +	}
> > +}
> 
> > +	status = acpi_install_address_space_handler(ipmi->handle,
> > +						    ACPI_ADR_SPACE_IPMI,
> > +						    &acpi_ipmi_space_handler,
> > +						    NULL, ipmi);
> > +	if (ACPI_FAILURE(status)) {
> > +		struct pnp_dev *pnp_dev = ipmi->pnp_dev;
> > +		dev_warn(&pnp_dev->dev, "Can't register IPMI opregion for %s\n",
> > +					pnp_dev_name(pnp_dev));
> 
> I'm actually not sure what pnp_dev_name() is for.  I'd rather not
> add more uses of it.  I think dev_warn() prints all the information
> we need, even without using pnp_dev_name().
> 
> > +static const struct pnp_device_id ipmi_device_ids[] = {
> > +	{"IPI0001", 0},
> > +	{"", 0},
> > +};
> > +
> > +static int match_device(struct pnp_dev *dev, const struct pnp_device_id *ids)
> > +{
> > +	const struct pnp_device_id *id;
> > +	struct pnp_id *pnp_id;
> > +
> > +	for (id = ids;  id->id[0]; id++) {
> > +		pnp_id = dev->id;
> > +		if (!pnp_id)
> > +			return 0;
> > +
> > +		while (pnp_id) {
> > +			if (!strncasecmp(pnp_id->id, (char *)id->id,
> > +					strlen((char *)id->id)))
> > +				return 1;
> > +			pnp_id = pnp_id->next;
> > +		};
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int pnp_ipmi_match(struct device *dev, void *data)
> > +{
> > +	struct pnp_dev *pnp_dev;
> > +	struct acpi_ipmi_device *ipmi_device;
> > +	struct acpi_device *acpi_dev;
> > +
> > +	pnp_dev = to_pnp_dev(dev);
> > +
> > +	if (!match_device(pnp_dev, ipmi_device_ids))
> > +		return 0;
> > +
> > +	ipmi_device = kzalloc(sizeof(*ipmi_device), GFP_KERNEL);
> > +	if (!ipmi_device) {
> > +		dev_warn(dev, "Can't allocate memory for acpi_ipmi_device\n");
> > +		return -ENOMEM;
> > +	}
> > +	acpi_dev = pnp_acpi_device(pnp_dev);
> > +	INIT_LIST_HEAD(&ipmi_device->head);
> > +	ipmi_device->pnp_dev = pnp_dev;
> > +	ipmi_device->handle = acpi_dev->handle;
> > +
> > +	list_add_tail(&ipmi_device->head, &driver_data.ipmi_devices);
> > +	return 0;
> > +}
> > +
> > +static void acpi_add_ipmi_devices(void)
> > +{
> > +	struct acpi_ipmi_device *ipmi_device;
> > +
> > +	bus_for_each_dev(&pnp_bus_type, NULL, NULL, pnp_ipmi_match);
> > +	if (list_empty(&driver_data.ipmi_devices))
> > +		return;
> 
> You don't need to test for an empty list here.
> 
> > +
> > +	list_for_each_entry(ipmi_device, &driver_data.ipmi_devices, head) {
> > +		mutex_init(&ipmi_device->mutex_lock);
> > +		INIT_LIST_HEAD(&ipmi_device->tx_msg_list);
> > +		ipmi_install_handler(ipmi_device);
> > +	}
> > +}
> > +
> > +static void acpi_remove_ipmi_handlers(void)
> > +{
> > +	struct acpi_ipmi_device *ipmi_device, *temp;
> > +
> > +	list_for_each_entry_safe(ipmi_device, temp, &driver_data.ipmi_devices,
> > +					head) {
> > +		if (ipmi_device->user_interface) {
> > +			/*
> > +			 * 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;
> 
> No "return" needed here.
> 
> > +}
> > +
> > +static int __init acpi_ipmi_init(void)
> > +{
> > +	int result = 0;
> > +
> > +	if (acpi_disabled)
> > +		return result;
> > +
> > +	acpi_add_ipmi_devices();
> > +
> > +	result = ipmi_smi_watcher_register(&driver_data.bmc_events);
> > +
> > +	if (result)
> > +		acpi_remove_ipmi_handlers();
> > +
> > +	return result;
> > +}
> > +
> > +static void __exit acpi_ipmi_exit(void)
> > +{
> > +	if (acpi_disabled)
> > +		return;
> > +
> > +	acpi_remove_ipmi_handlers();
> > +	ipmi_smi_watcher_unregister(&driver_data.bmc_events);
> > +
> > +	return;
> 
> No "return" needed here.
> 
> > +}
> > +
> > +module_init(acpi_ipmi_init);
> > +module_exit(acpi_ipmi_exit);
> > 

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

[Bjorn Helgaas]

On Wednesday, July 14, 2010 07:31:06 pm ykzhao wrote:
> On Thu, 2010-07-15 at 01:24 +0800, Bjorn Helgaas wrote:
> Will you please look at this thread
> http://marc.info/?l=linux-acpi&m=127608894610765&w=4?
> 
> Before this patch, I followed your comment and put the ACPI IPMI
> opregion code into the IPMI_SI module. 
> 
> But in fact the IPMI opregion code has nothing to do with the ipmi
> system interface. It is enough to use the IPMI API interface. In such
> case maybe it is a better choice to put the IPMI opregion code into the
> separate file. (In fact Corey suggests that the IPMI opregion code be
> put into the separate file). 

The ACPI IPMI opregion code deals with an ACPI device, and the
obvious place where you have that device is the driver "add"
function, i.e., ipmi_pnp_probe().  That's the point where the
PNP core will help you out by matching device IDs and handling
hotplug for you.

You can kludge around and do it elsewhere, but it's going to be
more work.

> > But I don't think this is the right design.  I think it's fine to
> > put all this code in a separate file and have a separate config
> > option for it.
> > 
> > However, I don't think acpi_ipmi and ipmi_si should be different
> > modules, because they both want to be connected to the same ACPI
> > device.  Making acpi_ipmi a separate module means you have to
> > mess around with PNP core things that drivers shouldn't be using.
> > For example, you have match_device(), pnp_ipmi_match(), and
> > acpi_add_ipmi_devices() below.  These all reimplement things
> > the PNP core already does.
> 
> If we decide to put the IPMI opregion code into the separate file, the
> next is how to *correctly* create the IPMI user interface between ACPI
> subsystem and the corresponding BMC controller.The user interface is the
> communication channel used to send/receive IPMI message to/from the BMC
> controller. 
> In such case we will use the IPMI smi watcher to create/destroy the IPMI
> user interface between ACPI and the corresponding BMC controller. But it
> is noted that we should assure whether the corresponding IPMI BMC
> controller is registered by using ACPI detection mechanism when creating
> the relationship between ACPI and IPMI BMC controller. This is realized
> by comparing the "device handle". And before comparing the "device
> handle", maybe we will have to get the corresponding device lists with
> the device id "IPI0001".
> 
> Do you have other idea about how to *correctly* create the user
> interface between ACPI and IPMI BMC controller?

I don't have time right now to design something new for this.

You should handle hotplug of IPI0001 devices, but you call
acpi_add_ipmi_devices() only once, at module-load time.  That
makes me think the sequence "load ipmi_si, load acpi_ipmi,
hot-add IPI0001" will not work.  I think ipmi_si will see the
new device, but I don't think acpi_ipmi will.

We should not export pnp_bus_type.  Maybe you can avoid this by
maintaining some sort of list in ipmi_si, although then you're back
to having to deal with hotplug yourself.  You'd have to have
coordinate ipmi_pnp_probe() and ipmi_pnp_remove() events between
ipmi_si and acpi_ipmi.

> > > +static void ipmi_bmc_gone(int iface)
> > > +{
> > > +	struct acpi_ipmi_device *ipmi_device, *temp;
> > > +
> > > +	if (list_empty(&driver_data.ipmi_devices))
> > > +		return;
> > 
> > You don't need to test for list_empty before using
> > list_for_each_entry_safe()
> 
> IMO it is needed as it does the operation of list deletion.

I don't think so:

 * list_for_each_entry_safe - iterate over list of given type safe against removal of list entry

Bjorn

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

[ykzhao]

On Fri, 2010-07-16 at 00:27 +0800, Bjorn Helgaas wrote:
> On Wednesday, July 14, 2010 07:31:06 pm ykzhao wrote:
> > On Thu, 2010-07-15 at 01:24 +0800, Bjorn Helgaas wrote:
> > Will you please look at this thread
> > http://marc.info/?l=linux-acpi&m=127608894610765&w=4?
> > 
> > Before this patch, I followed your comment and put the ACPI IPMI
> > opregion code into the IPMI_SI module. 
> > 
> > But in fact the IPMI opregion code has nothing to do with the ipmi
> > system interface. It is enough to use the IPMI API interface. In such
> > case maybe it is a better choice to put the IPMI opregion code into the
> > separate file. (In fact Corey suggests that the IPMI opregion code be
> > put into the separate file). 
> 
> The ACPI IPMI opregion code deals with an ACPI device, and the
> obvious place where you have that device is the driver "add"
> function, i.e., ipmi_pnp_probe().  That's the point where the
> PNP core will help you out by matching device IDs and handling
> hotplug for you.

The "IPI0001" device defined in ACPI table serves the following two
purposes:
 	1. IPMI detection and registration: This is defined in IPMI spec. Now
it is discovered in the drivers/char/ipmi/ipmi_si_intf
        2. IPMI opregion support: This is defined in ACPI4.0 spec.This
is to enable that the ACPI AML code can communicate with the BMC
controller. 

Yes. If we put the opregion code with the detection code together, it
will be simple. And it is unnecessary to get the corresponding device
list again. But as the IPMI opregion code only uses the API interface of
IPMI subsystem, it will be better to put it into the separate file. In
fact this is also Corey's idea. 

> 
> You can kludge around and do it elsewhere, but it's going to be
> more work.
> 
> > > But I don't think this is the right design.  I think it's fine to
> > > put all this code in a separate file and have a separate config
> > > option for it.
> > > 
> > > However, I don't think acpi_ipmi and ipmi_si should be different
> > > modules, because they both want to be connected to the same ACPI
> > > device.  Making acpi_ipmi a separate module means you have to
> > > mess around with PNP core things that drivers shouldn't be using.
> > > For example, you have match_device(), pnp_ipmi_match(), and
> > > acpi_add_ipmi_devices() below.  These all reimplement things
> > > the PNP core already does.
> > 
> > If we decide to put the IPMI opregion code into the separate file, the
> > next is how to *correctly* create the IPMI user interface between ACPI
> > subsystem and the corresponding BMC controller.The user interface is the
> > communication channel used to send/receive IPMI message to/from the BMC
> > controller. 
> > In such case we will use the IPMI smi watcher to create/destroy the IPMI
> > user interface between ACPI and the corresponding BMC controller. But it
> > is noted that we should assure whether the corresponding IPMI BMC
> > controller is registered by using ACPI detection mechanism when creating
> > the relationship between ACPI and IPMI BMC controller. This is realized
> > by comparing the "device handle". And before comparing the "device
> > handle", maybe we will have to get the corresponding device lists with
> > the device id "IPI0001".
> > 
> > Do you have other idea about how to *correctly* create the user
> > interface between ACPI and IPMI BMC controller?
> 
> I don't have time right now to design something new for this.

Yes. The hotplug scenario should be considered. This will be handled
under two places:
   a. the IPMI pnp detection: 
   b. install opregion handle for the hotplug IPI0001 device

But in fact the acpi_pnp device is enumerated only once at the boot
time. In such case it is impossible to get the corresponding pnp device
when one "IPI0001" device is hotplug. Of course the "ipmi_si" can't see
the hotplugged "IPI0001" device.  If we really want to handle the
hotplug scenario, IMO we should change the detection mechanism from pnp
to ACPI again. Otherwise it is difficult to handle the hotplug scenario.

It is an idea to maintain the device list of "impi_si". But if we look
at the source code of "ipmi_si_intf", the corresponding device list is
defined as *static*. 

Another matter is that the "info->dev" in ipmi_pnp_probe is changed from
acpi_dev to pnp_dev. Now in order to *correctly* create the user
interface between ACPI and BMC controller, it is realized by comparing
the "device handle". And the prerequisite is that we should get the
corresponding pnp device list of "IPI0001". 
If we don't export the symbol of "pnp_bus_type", do you have any idea to
get the corresponding device list of "IPI0001"? 

Of course I can firstly get the ACPI device list of "IPI0001" and then
get the corresponding "pnp_dev" for every ACPI device of "IPI0001". But
it is more complex. 

> 
> You should handle hotplug of IPI0001 devices, but you call
> acpi_add_ipmi_devices() only once, at module-load time.  That
> makes me think the sequence "load ipmi_si, load acpi_ipmi,
> hot-add IPI0001" will not work.  I think ipmi_si will see the
> new device, but I don't think acpi_ipmi will.
> 
> We should not export pnp_bus_type.  Maybe you can avoid this by
> maintaining some sort of list in ipmi_si, although then you're back
> to having to deal with hotplug yourself.  You'd have to have
> coordinate ipmi_pnp_probe() and ipmi_pnp_remove() events between
> ipmi_si and acpi_ipmi.
> 
> > > > +static void ipmi_bmc_gone(int iface)
> > > > +{
> > > > +	struct acpi_ipmi_device *ipmi_device, *temp;
> > > > +
> > > > +	if (list_empty(&driver_data.ipmi_devices))
> > > > +		return;
> > > 
> > > You don't need to test for list_empty before using
> > > list_for_each_entry_safe()
> > 
> > IMO it is needed as it does the operation of list deletion.
> 
> I don't think so:
> 
>  * list_for_each_entry_safe - iterate over list of given type safe against removal of list entry

There exist a lot of examples about the removal of list_entry by using
list_for_each_entry_safe. For example:
   >drivers/char/ipmi/ipmi_msghandler.c

> 
> Bjorn

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

[Bjorn Helgaas]

On Thursday, July 15, 2010 08:34:35 pm ykzhao wrote:
> On Fri, 2010-07-16 at 00:27 +0800, Bjorn Helgaas wrote:
> > On Wednesday, July 14, 2010 07:31:06 pm ykzhao wrote:
> > > On Thu, 2010-07-15 at 01:24 +0800, Bjorn Helgaas wrote:
> > > Will you please look at this thread
> > > http://marc.info/?l=linux-acpi&m=127608894610765&w=4?
> > > 
> > > Before this patch, I followed your comment and put the ACPI IPMI
> > > opregion code into the IPMI_SI module. 
> > > 
> > > But in fact the IPMI opregion code has nothing to do with the ipmi
> > > system interface. It is enough to use the IPMI API interface. In such
> > > case maybe it is a better choice to put the IPMI opregion code into the
> > > separate file. (In fact Corey suggests that the IPMI opregion code be
> > > put into the separate file). 
> > 
> > The ACPI IPMI opregion code deals with an ACPI device, and the
> > obvious place where you have that device is the driver "add"
> > function, i.e., ipmi_pnp_probe().  That's the point where the
> > PNP core will help you out by matching device IDs and handling
> > hotplug for you.
> 
> The "IPI0001" device defined in ACPI table serves the following two
> purposes:
>  	1. IPMI detection and registration: This is defined in IPMI spec. Now
> it is discovered in the drivers/char/ipmi/ipmi_si_intf
>         2. IPMI opregion support: This is defined in ACPI4.0 spec.This
> is to enable that the ACPI AML code can communicate with the BMC
> controller. 
> 
> Yes. If we put the opregion code with the detection code together, it
> will be simple. And it is unnecessary to get the corresponding device
> list again. But as the IPMI opregion code only uses the API interface of
> IPMI subsystem, it will be better to put it into the separate file. In
> fact this is also Corey's idea.

I don't care if you put it in a separate file.  My only point is that
you shouldn't try to duplicate the device discovery that's already
done in the PNP core.

> Yes. The hotplug scenario should be considered. This will be handled
> under two places:
>    a. the IPMI pnp detection: 
>    b. install opregion handle for the hotplug IPI0001 device
> 
> But in fact the acpi_pnp device is enumerated only once at the boot
> time.

Drivers cannot rely on anything like "the acpi_pnp device is
enumerated only once at boot-time."  Device enumeration happens
in the ACPI core and PNP core, not in drivers, and drivers can't
assume anything about when it happens.

> In such case it is impossible to get the corresponding pnp device
> when one "IPI0001" device is hotplug. Of course the "ipmi_si" can't see
> the hotplugged "IPI0001" device.  If we really want to handle the
> hotplug scenario, IMO we should change the detection mechanism from pnp
> to ACPI again. Otherwise it is difficult to handle the hotplug scenario.

No, you're missing the point.  *All* drivers should be written in
the standard style that supports hotplug, because then all devices,
both those present at boot and any added later, are handled the same
way.  

ipmi_si can indeed see any hotplugged IPI0001 device, simply because
it uses pnp_register_driver().  The PNP core calls ipmi_pnp_probe()
when any IPI0001 device is discovered.  This device could be present
at boot, or it could be added later.

> It is an idea to maintain the device list of "impi_si". But if we look
> at the source code of "ipmi_si_intf", the corresponding device list is
> defined as *static*. 

This is a problem you're creating for yourself by trying to completely
separate acpi_ipmi from ipmi_pnp_probe().  I'm suggesting that you can
make your life easier by adding some kind of hook in the ipmi_pnp_probe()
path to set up the opregion handler.

> Another matter is that the "info->dev" in ipmi_pnp_probe is changed from
> acpi_dev to pnp_dev. Now in order to *correctly* create the user
> interface between ACPI and BMC controller, it is realized by comparing
> the "device handle". And the prerequisite is that we should get the
> corresponding pnp device list of "IPI0001". 
> If we don't export the symbol of "pnp_bus_type", do you have any idea to
> get the corresponding device list of "IPI0001"? 

If you have the pnp_dev for an IPI0001 device, you can always get the
ACPI handle with pnp_acpi_device().

> > > > > +static void ipmi_bmc_gone(int iface)
> > > > > +{
> > > > > +	struct acpi_ipmi_device *ipmi_device, *temp;
> > > > > +
> > > > > +	if (list_empty(&driver_data.ipmi_devices))
> > > > > +		return;
> > > > 
> > > > You don't need to test for list_empty before using
> > > > list_for_each_entry_safe()
> > > 
> > > IMO it is needed as it does the operation of list deletion.
> > 
> > I don't think so:
> > 
> >  * list_for_each_entry_safe - iterate over list of given type safe against removal of list entry
> 
> There exist a lot of examples about the removal of list_entry by using
> list_for_each_entry_safe. For example:
>    >drivers/char/ipmi/ipmi_msghandler.c

That only means other drivers also contain this unnecessary code.

There are many examples of list deletion using list_for_each_entry_safe()
where we don't check list_empty() first.  For example:

  pnp_free_resources()
  pnp_clean_resource_table()
  pnp_free_options()
  omap_mux_late_init()
  vpe_module_exit()/release_vpe()
  fsl_dma_slave_free()
  cbe_ptcal_disable()
  pmi_of_remove()

Maybe you could remove the check and do some testing to convince
yourself that it's safe.

Bjorn

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

[ykzhao]

On Sat, 2010-07-17 at 00:01 +0800, Bjorn Helgaas wrote:
> On Thursday, July 15, 2010 08:34:35 pm ykzhao wrote:
> > On Fri, 2010-07-16 at 00:27 +0800, Bjorn Helgaas wrote:
> > > On Wednesday, July 14, 2010 07:31:06 pm ykzhao wrote:
> > > > On Thu, 2010-07-15 at 01:24 +0800, Bjorn Helgaas wrote:
> > > > Will you please look at this thread
> > > > http://marc.info/?l=linux-acpi&m=127608894610765&w=4?
> > > > 
> > > > Before this patch, I followed your comment and put the ACPI IPMI
> > > > opregion code into the IPMI_SI module. 
> > > > 
> > > > But in fact the IPMI opregion code has nothing to do with the ipmi
> > > > system interface. It is enough to use the IPMI API interface. In such
> > > > case maybe it is a better choice to put the IPMI opregion code into the
> > > > separate file. (In fact Corey suggests that the IPMI opregion code be
> > > > put into the separate file). 
> > > 
> > > The ACPI IPMI opregion code deals with an ACPI device, and the
> > > obvious place where you have that device is the driver "add"
> > > function, i.e., ipmi_pnp_probe().  That's the point where the
> > > PNP core will help you out by matching device IDs and handling
> > > hotplug for you.
> > 
> > The "IPI0001" device defined in ACPI table serves the following two
> > purposes:
> >  	1. IPMI detection and registration: This is defined in IPMI spec. Now
> > it is discovered in the drivers/char/ipmi/ipmi_si_intf
> >         2. IPMI opregion support: This is defined in ACPI4.0 spec.This
> > is to enable that the ACPI AML code can communicate with the BMC
> > controller. 
> > 
> > Yes. If we put the opregion code with the detection code together, it
> > will be simple. And it is unnecessary to get the corresponding device
> > list again. But as the IPMI opregion code only uses the API interface of
> > IPMI subsystem, it will be better to put it into the separate file. In
> > fact this is also Corey's idea.
> 
> I don't care if you put it in a separate file.  My only point is that
> you shouldn't try to duplicate the device discovery that's already
> done in the PNP core.
> 
> > Yes. The hotplug scenario should be considered. This will be handled
> > under two places:
> >    a. the IPMI pnp detection: 
> >    b. install opregion handle for the hotplug IPI0001 device
> > 
> > But in fact the acpi_pnp device is enumerated only once at the boot
> > time.
> 
> Drivers cannot rely on anything like "the acpi_pnp device is
> enumerated only once at boot-time."  Device enumeration happens
> in the ACPI core and PNP core, not in drivers, and drivers can't
> assume anything about when it happens.

Yes. But in fact the ACPI pnp device is enumerated only once at the
boot time. In such case it is impossible to get the corresponding pnp
device when one "IPI0001" device is hot-plug. Is it still meaningful to
consider the "hot-plug" scenario? 

> 
> > In such case it is impossible to get the corresponding pnp device
> > when one "IPI0001" device is hotplug. Of course the "ipmi_si" can't see
> > the hotplugged "IPI0001" device.  If we really want to handle the
> > hotplug scenario, IMO we should change the detection mechanism from pnp
> > to ACPI again. Otherwise it is difficult to handle the hotplug scenario.
> 
> No, you're missing the point.  *All* drivers should be written in
> the standard style that supports hotplug, because then all devices,
> both those present at boot and any added later, are handled the same
> way.  
> 
> ipmi_si can indeed see any hotplugged IPI0001 device, simply because
> it uses pnp_register_driver().  The PNP core calls ipmi_pnp_probe()
> when any IPI0001 device is discovered.  This device could be present
> at boot, or it could be added later.

> 
> > It is an idea to maintain the device list of "impi_si". But if we look
> > at the source code of "ipmi_si_intf", the corresponding device list is
> > defined as *static*. 
> 
> This is a problem you're creating for yourself by trying to completely
> separate acpi_ipmi from ipmi_pnp_probe().  I'm suggesting that you can
> make your life easier by adding some kind of hook in the ipmi_pnp_probe()
> path to set up the opregion handler.

It is an idea to add some kind of hook. If so, 
If so, we will have to export more symbols in ipmi subsystem and the
file of drivers/acpi/acpi_ipmi.c.  But the main function in the
drivers/char/ipmi/ipmi_si_intf.c is to discovery the BMC controller and
register the corresponding IPMI system interface. Is it appropriate to
export some symbols which is not related with IPMI discovery?

> > Another matter is that the "info->dev" in ipmi_pnp_probe is changed from
> > acpi_dev to pnp_dev. Now in order to *correctly* create the user
> > interface between ACPI and BMC controller, it is realized by comparing
> > the "device handle". And the prerequisite is that we should get the
> > corresponding pnp device list of "IPI0001". 
> > If we don't export the symbol of "pnp_bus_type", do you have any idea to
> > get the corresponding device list of "IPI0001"? 
> 
> If you have the pnp_dev for an IPI0001 device, you can always get the
> ACPI handle with pnp_acpi_device().
> 
> > > > > > +static void ipmi_bmc_gone(int iface)
> > > > > > +{
> > > > > > +	struct acpi_ipmi_device *ipmi_device, *temp;
> > > > > > +
> > > > > > +	if (list_empty(&driver_data.ipmi_devices))
> > > > > > +		return;
> > > > > 
> > > > > You don't need to test for list_empty before using
> > > > > list_for_each_entry_safe()
> > > > 
> > > > IMO it is needed as it does the operation of list deletion.
> > > 
> > > I don't think so:
> > > 
> > >  * list_for_each_entry_safe - iterate over list of given type safe against removal of list entry
> > 
> > There exist a lot of examples about the removal of list_entry by using
> > list_for_each_entry_safe. For example:
> >    >drivers/char/ipmi/ipmi_msghandler.c
> 
> That only means other drivers also contain this unnecessary code.
> 
> There are many examples of list deletion using list_for_each_entry_safe()
> where we don't check list_empty() first.  For example:
> 
>   pnp_free_resources()
>   pnp_clean_resource_table()
>   pnp_free_options()
>   omap_mux_late_init()
>   vpe_module_exit()/release_vpe()
>   fsl_dma_slave_free()
>   cbe_ptcal_disable()
>   pmi_of_remove()
> 
> Maybe you could remove the check and do some testing to convince
> yourself that it's safe.
> 
> Bjorn

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Bjorn Helgaas]

On Sunday, July 18, 2010 07:52:28 pm ykzhao wrote:
> On Sat, 2010-07-17 at 00:01 +0800, Bjorn Helgaas wrote:
> > On Thursday, July 15, 2010 08:34:35 pm ykzhao wrote:
> > > On Fri, 2010-07-16 at 00:27 +0800, Bjorn Helgaas wrote:
> > > > The ACPI IPMI opregion code deals with an ACPI device, and the
> > > > obvious place where you have that device is the driver "add"
> > > > function, i.e., ipmi_pnp_probe().  That's the point where the
> > > > PNP core will help you out by matching device IDs and handling
> > > > hotplug for you.
...
> > > Yes. The hotplug scenario should be considered. This will be handled
> > > under two places:
> > >    a. the IPMI pnp detection: 
> > >    b. install opregion handle for the hotplug IPI0001 device
> > > 
> > > But in fact the acpi_pnp device is enumerated only once at the boot
> > > time.
> > 
> > Drivers cannot rely on anything like "the acpi_pnp device is
> > enumerated only once at boot-time."  Device enumeration happens
> > in the ACPI core and PNP core, not in drivers, and drivers can't
> > assume anything about when it happens.
> 
> Yes. But in fact the ACPI pnp device is enumerated only once at the
> boot time.

NO!  You can NOT assume the ACPI PNP device is enumerated once at
boot time.

That may be the way the current implementation behaves, but the
driver interface does not specify that, and it is likely to change.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[ykzhao]

On Mon, 2010-07-19 at 22:54 +0800, Bjorn Helgaas wrote:
> On Sunday, July 18, 2010 07:52:28 pm ykzhao wrote:
> > On Sat, 2010-07-17 at 00:01 +0800, Bjorn Helgaas wrote:
> > > On Thursday, July 15, 2010 08:34:35 pm ykzhao wrote:
> > > > On Fri, 2010-07-16 at 00:27 +0800, Bjorn Helgaas wrote:
> > > > > The ACPI IPMI opregion code deals with an ACPI device, and the
> > > > > obvious place where you have that device is the driver "add"
> > > > > function, i.e., ipmi_pnp_probe().  That's the point where the
> > > > > PNP core will help you out by matching device IDs and handling
> > > > > hotplug for you.
> ...
> > > > Yes. The hotplug scenario should be considered. This will be handled
> > > > under two places:
> > > >    a. the IPMI pnp detection: 
> > > >    b. install opregion handle for the hotplug IPI0001 device
> > > > 
> > > > But in fact the acpi_pnp device is enumerated only once at the boot
> > > > time.
> > > 
> > > Drivers cannot rely on anything like "the acpi_pnp device is
> > > enumerated only once at boot-time."  Device enumeration happens
> > > in the ACPI core and PNP core, not in drivers, and drivers can't
> > > assume anything about when it happens.
> > 
> > Yes. But in fact the ACPI pnp device is enumerated only once at the
> > boot time.
> 
> NO!  You can NOT assume the ACPI PNP device is enumerated once at
> boot time.
> 
> That may be the way the current implementation behaves, but the
> driver interface does not specify that, and it is likely to change.

Ok. I will try to use the hook to implement it.

thanks.
	Yakui
> 
> Bjorn

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html