[RFC PATCH 0/2_v9] IPMI/ACPI: Install the ACPI IPMI opregion

[RFC PATCH 0/2_v9] IPMI/ACPI: Install the ACPI IPMI opregion

[yakui.zhao]

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

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

[RFC Patch 01/02]: Add one interface to get more info of low-level interface
	It will return the corresponding info of low-level interface based 
	on the expected type(For example: SI_ACPI, SI_PCI and so on.)

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

V8-V9: This patch set is based on the mechanism that add one interface that can 
get more info of low-level IPMI device. For example: the ACPI device handle will
be returned for the pnp_acpi. Then the second patch will use the added interface
to check whether the IPMI device is what ACPI wanted to communicated with in the
new_smi callback function of smi_watcher. If it is, we will install the IPMI
opregion handler and enable ACPI to communicate with BMC.

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

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

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

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

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

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

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

Best regards.
   Yakui

[RFC PATCH 1/2_v9] IPMI: Add one interface to get more info of low-level IPMI device

[yakui.zhao]

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

The IPMI smi_watcher will be used to catch the IPMI interface as they come or go.
In order to communicate with the correct IPMI device, it should be confirmed
 whether it is what we wanted especially on the system with multiple IPMI
devices. But the new_smi callback function of smi_watcher provides very
limited info(only the interface number and dev pointer) and there is no
detailed info about the low level interface. For example: which mechanism
registers the IPMI interface(ACPI, PCI, DMI and so on).

This is to add one interface that can get more info of low-level IPMI
device. For example: the ACPI device handle will be returned for the pnp_acpi
IPMI device.

Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
---
In this patch only the info of pnp_acpi IPMI is provided. If the detailed info
is also required for other mechanism, we can redesign the structure of
ipmi_smi_data and fill it correctly.

 drivers/char/ipmi/ipmi_msghandler.c |   26 ++++++++++++++++++++++++++
 drivers/char/ipmi/ipmi_si_intf.c    |   24 ++++++++++++++++++++----
 include/linux/ipmi.h                |   16 ++++++++++++++++
 include/linux/ipmi_smi.h            |   11 +++++++++++
 4 files changed, 73 insertions(+), 4 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index 4f3f8c9..f82d426 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -970,6 +970,32 @@ out_kfree:
 }
 EXPORT_SYMBOL(ipmi_create_user);
 
+int ipmi_get_smi_info(int if_num, enum ipmi_addr_src type,
+			union ipmi_smi_data *data)
+{
+	int           rv = 0;
+	ipmi_smi_t    intf;
+	struct ipmi_smi_handlers *handlers;
+
+	mutex_lock(&ipmi_interfaces_mutex);
+	list_for_each_entry_rcu(intf, &ipmi_interfaces, link) {
+		if (intf->intf_num == if_num)
+			goto found;
+	}
+	/* Not found, return an error */
+	rv = -EINVAL;
+	mutex_unlock(&ipmi_interfaces_mutex);
+	return rv;
+
+found:
+	handlers = intf->handlers;
+	rv = handlers->get_smi_info(intf->send_info, type, data);
+	mutex_unlock(&ipmi_interfaces_mutex);
+
+	return rv;
+}
+EXPORT_SYMBOL(ipmi_get_smi_info);
+
 static void free_user(struct kref *ref)
 {
 	ipmi_user_t user = container_of(ref, struct ipmi_user, refcount);
diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 3822b4f..ebb87b4 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -57,6 +57,7 @@
 #include <asm/irq.h>
 #include <linux/interrupt.h>
 #include <linux/rcupdate.h>
+#include <linux/ipmi.h>
 #include <linux/ipmi_smi.h>
 #include <asm/io.h>
 #include "ipmi_si_sm.h"
@@ -107,10 +108,6 @@ enum si_type {
 };
 static char *si_to_str[] = { "kcs", "smic", "bt" };
 
-enum ipmi_addr_src {
-	SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
-	SI_PCI,	SI_DEVICETREE, SI_DEFAULT
-};
 static char *ipmi_addr_src_to_str[] = { NULL, "hotmod", "hardcoded", "SPMI",
 					"ACPI", "SMBIOS", "PCI",
 					"device-tree", "default" };
@@ -291,6 +288,7 @@ struct smi_info {
 	struct task_struct *thread;
 
 	struct list_head link;
+	union ipmi_smi_data smi_data;
 };
 
 #define smi_inc_stat(smi, stat) \
@@ -1183,6 +1181,21 @@ static int smi_start_processing(void       *send_info,
 	return 0;
 }
 
+static int get_smi_info(void *send_info,
+			enum ipmi_addr_src type,
+			union ipmi_smi_data *data)
+{
+	struct smi_info *new_smi = send_info;
+	union ipmi_smi_data *smi_data = &new_smi->smi_data;
+
+	if (new_smi->addr_source != type)
+		return -EINVAL;
+
+	memcpy(data, smi_data, sizeof(*smi_data));
+
+	return 0;
+}
+
 static void set_maintenance_mode(void *send_info, int enable)
 {
 	struct smi_info   *smi_info = send_info;
@@ -1194,6 +1207,7 @@ static void set_maintenance_mode(void *send_info, int enable)
 static struct ipmi_smi_handlers handlers = {
 	.owner                  = THIS_MODULE,
 	.start_processing       = smi_start_processing,
+	.get_smi_info		= get_smi_info,
 	.sender			= sender,
 	.request_events		= request_events,
 	.set_maintenance_mode   = set_maintenance_mode,
@@ -2140,6 +2154,8 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
 		return -ENOMEM;
 
 	info->addr_source = SI_ACPI;
+	info->smi_data.acpi_data.acpi_handle = acpi_dev->handle;
+	info->smi_data.acpi_data.dev = &dev->dev;
 	printk(KERN_INFO PFX "probing via ACPI\n");
 
 	handle = acpi_dev->handle;
diff --git a/include/linux/ipmi.h b/include/linux/ipmi.h
index 65aae34..04e4a90 100644
--- a/include/linux/ipmi.h
+++ b/include/linux/ipmi.h
@@ -454,6 +454,22 @@ unsigned int ipmi_addr_length(int addr_type);
 /* Validate that the given IPMI address is valid. */
 int ipmi_validate_addr(struct ipmi_addr *addr, int len);
 
+enum ipmi_addr_src {
+	SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
+	SI_PCI,	SI_DEVICETREE, SI_DEFAULT
+};
+union ipmi_smi_data{
+	/* If the other type of data is required, please add it	 */
+	u8 data[16];
+	struct {
+		void *acpi_handle;
+		struct device *dev;
+	} acpi_data;
+};
+
+/* This is to get the private info of ipmi_smi_t */
+extern int ipmi_get_smi_info(int if_num, enum ipmi_addr_src type,
+		union ipmi_smi_data *data);
 #endif /* __KERNEL__ */
 
 
diff --git a/include/linux/ipmi_smi.h b/include/linux/ipmi_smi.h
index 4b48318..d5bae83 100644
--- a/include/linux/ipmi_smi.h
+++ b/include/linux/ipmi_smi.h
@@ -86,6 +86,17 @@ struct ipmi_smi_handlers {
 	int (*start_processing)(void       *send_info,
 				ipmi_smi_t new_intf);
 
+	/*
+	 * Get the detailed private info of the low level interface and store
+	 * it into the union structure of ipmi_smi_data. For example: the
+	 * ACPI device handle will be returned for the pnp_acpi IPMI device.
+	 * Of course it will firstly compare the interface type of low-level
+	 * interface(Hardcoded type, DMI, ACPI and so on). If the type doesn't
+	 * match, it will return -EINVAL.
+	 */
+	int (*get_smi_info)(void *send_info, enum ipmi_addr_src type,
+				union ipmi_smi_data *data);
+
 	/* Called to enqueue an SMI message to be sent.  This
 	   operation is not allowed to fail.  If an error occurs, it
 	   should report back the error in a received message.  It may
-- 
1.5.4.5

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

[yakui.zhao]

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

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

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

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

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

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

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index b811f21..a72af13 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -215,6 +215,17 @@ 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:
+	  the module will be called as acpi_ipmi.
 
 config ACPI_HOTPLUG_CPU
 	bool
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 3d031d0..df4c4f0 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -69,5 +69,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..af79087
--- /dev/null
+++ b/drivers/acpi/acpi_ipmi.c
@@ -0,0 +1,505 @@
+/*
+ *  acpi_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 {
+	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 void acpi_add_ipmi_device(struct acpi_ipmi_device *ipmi_device);
+static void acpi_remove_ipmi_device(struct acpi_ipmi_device *ipmi_device);
+
+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;
+	struct pnp_dev *pnp_dev = ipmi->pnp_dev;
+
+	ipmi_msg = kzalloc(sizeof(struct acpi_ipmi_msg), GFP_KERNEL);
+	if (!ipmi_msg)	{
+		dev_warn(&pnp_dev->dev, "Can't allocate memory for ipmi_msg\n");
+		return NULL;
+	}
+	init_completion(&ipmi_msg->tx_complete);
+	INIT_LIST_HEAD(&ipmi_msg->head);
+	ipmi_msg->device = ipmi;
+	return ipmi_msg;
+}
+
+#define		IPMI_OP_RGN_NETFN(offset)	((offset >> 8) & 0xff)
+#define		IPMI_OP_RGN_CMD(offset)		(offset & 0xff)
+static void acpi_format_ipmi_msg(struct acpi_ipmi_msg *tx_msg,
+				acpi_physical_address address,
+				acpi_integer *value)
+{
+	struct kernel_ipmi_msg *msg;
+	struct acpi_ipmi_buffer *buffer;
+	struct acpi_ipmi_device *device;
+
+	msg = &tx_msg->tx_message;
+	/*
+	 * IPMI network function and command are encoded in the address
+	 * within the IPMI OpRegion; see ACPI 4.0, sec 5.5.2.4.3.
+	 */
+	msg->netfn = IPMI_OP_RGN_NETFN(address);
+	msg->cmd = IPMI_OP_RGN_CMD(address);
+	msg->data = tx_msg->tx_data;
+	/*
+	 * value is the parameter passed by the IPMI opregion space handler.
+	 * It points to the IPMI request message buffer
+	 */
+	buffer = (struct acpi_ipmi_buffer *)value;
+	/* copy the tx message data */
+	msg->data_len = buffer->length;
+	memcpy(tx_msg->tx_data, buffer->data, msg->data_len);
+	/*
+	 * now the default type is SYSTEM_INTERFACE and channel type is BMC.
+	 * If the netfn is APP_REQUEST and the cmd is SEND_MESSAGE,
+	 * the addr type should be changed to IPMB. Then we will have to parse
+	 * the IPMI request message buffer to get the IPMB address.
+	 * If so, please fix me.
+	 */
+	tx_msg->addr.addr_type = IPMI_SYSTEM_INTERFACE_ADDR_TYPE;
+	tx_msg->addr.channel = IPMI_BMC_CHANNEL;
+	tx_msg->addr.data[0] = 0;
+
+	/* Get the msgid */
+	device = tx_msg->device;
+	mutex_lock(&device->mutex_lock);
+	device->curr_msgid++;
+	tx_msg->tx_msgid = device->curr_msgid;
+	mutex_unlock(&device->mutex_lock);
+}
+
+static void acpi_format_ipmi_response(struct acpi_ipmi_msg *msg,
+		acpi_integer *value, int rem_time)
+{
+	struct acpi_ipmi_buffer *buffer;
+
+	/*
+	 * value is also used as output parameter. It represents the response
+	 * IPMI message returned by IPMI command.
+	 */
+	buffer = (struct acpi_ipmi_buffer *)value;
+	if (!rem_time && !msg->msg_done) {
+		buffer->status = ACPI_IPMI_TIMEOUT;
+		return;
+	}
+	/*
+	 * If the flag of msg_done is not set or the recv length is zero, it
+	 * means that the IPMI command is not executed correctly.
+	 * The status code will be ACPI_IPMI_UNKNOWN.
+	 */
+	if (!msg->msg_done || !msg->rx_len) {
+		buffer->status = ACPI_IPMI_UNKNOWN;
+		return;
+	}
+	/*
+	 * If the IPMI response message is obtained correctly, the status code
+	 * will be ACPI_IPMI_OK
+	 */
+	buffer->status = ACPI_IPMI_OK;
+	buffer->length = msg->rx_len;
+	memcpy(buffer->data, msg->rx_data, msg->rx_len);
+}
+
+static void ipmi_flush_tx_msg(struct acpi_ipmi_device *ipmi)
+{
+	struct acpi_ipmi_msg *tx_msg, *temp;
+	int count = HZ / 10;
+	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 100ms 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 = user_msg_data;
+	int msg_found = 0;
+	struct acpi_ipmi_msg *tx_msg;
+	struct pnp_dev *pnp_dev = ipmi_device->pnp_dev;
+
+	if (msg->user != ipmi_device->user_interface) {
+		dev_warn(&pnp_dev->dev, "Unexpected response is returned. "
+			"returned user %p, expected user %p\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) {
+			msg_found = 1;
+			break;
+		}
+	}
+
+	mutex_unlock(&ipmi_device->mutex_lock);
+	if (!msg_found) {
+		ipmi_free_recv_msg(msg);
+		dev_warn(&pnp_dev->dev, "Unexpected response (msg id %ld) is "
+			"returned.\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, *temp;
+	struct pnp_dev *pnp_dev;
+	ipmi_user_t		user;
+	int err;
+	union ipmi_smi_data smi_data;
+	acpi_handle handle;
+
+	err = ipmi_get_smi_info(iface, SI_ACPI, &smi_data);
+
+	if (err)
+		return;
+
+	handle = smi_data.acpi_data.acpi_handle;
+
+	list_for_each_entry(temp, &driver_data.ipmi_devices, head) {
+		/*
+		 * if the corresponding ACPI handle is already added
+		 * to the device list, don't add it again.
+		 */
+		if (temp->handle == handle)
+			return;
+	}
+
+	ipmi_device = kzalloc(sizeof(*ipmi_device), GFP_KERNEL);
+
+	if (!ipmi_device)
+		return;
+
+	pnp_dev = to_pnp_dev(smi_data.acpi_data.dev);
+	ipmi_device->handle = handle;
+	ipmi_device->pnp_dev = pnp_dev;
+
+	err = ipmi_create_user(iface, &driver_data.ipmi_hndlrs,
+					ipmi_device, &user);
+	if (err) {
+		dev_warn(&pnp_dev->dev, "Can't create IPMI user interface\n");
+		kfree(ipmi_device);
+		return;
+	}
+	acpi_add_ipmi_device(ipmi_device);
+	ipmi_device->user_interface = user;
+	ipmi_device->ipmi_ifnum = iface;
+}
+
+static void ipmi_bmc_gone(int iface)
+{
+	struct acpi_ipmi_device *ipmi_device, *temp;
+
+	list_for_each_entry_safe(ipmi_device, temp,
+				&driver_data.ipmi_devices, head) {
+		if (ipmi_device->ipmi_ifnum != iface)
+			continue;
+
+		if (ipmi_device->user_interface) {
+			ipmi_destroy_user(ipmi_device->user_interface);
+			ipmi_device->user_interface = NULL;
+			ipmi_flush_tx_msg(ipmi_device);
+		}
+		acpi_remove_ipmi_device(ipmi_device);
+		kfree(ipmi_device);
+		break;
+	}
+}
+/* --------------------------------------------------------------------------
+ *			Address Space Management
+ * -------------------------------------------------------------------------- */
+/*
+ * This is the IPMI opregion space handler.
+ * @function: indicates the read/write. In fact as the IPMI message is driven
+ * by command, only write is meaningful.
+ * @address: This contains the netfn/command of IPMI request message.
+ * @bits   : not used.
+ * @value  : it is an in/out parameter. It points to the IPMI message buffer.
+ *	     Before the IPMI message is sent, it represents the actual request
+ *	     IPMI message. After the IPMI message is finished, it represents
+ *	     the response IPMI message returned by IPMI command.
+ * @handler_context: IPMI device context.
+ */
+
+static acpi_status
+acpi_ipmi_space_handler(u32 function, acpi_physical_address address,
+		      u32 bits, acpi_integer *value,
+		      void *handler_context, void *region_context)
+{
+	struct acpi_ipmi_msg *tx_msg;
+	struct acpi_ipmi_device *ipmi_device = handler_context;
+	int err, rem_time;
+	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 read access
+	 * of IPMI opregion.
+	 */
+	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;
+	}
+	rem_time = wait_for_completion_timeout(&tx_msg->tx_complete,
+					IPMI_TIMEOUT);
+	acpi_format_ipmi_response(tx_msg, value, rem_time);
+	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_space_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_space_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 space "
+			"handle\n");
+		return -EINVAL;
+	}
+	set_bit(IPMI_FLAGS_HANDLER_INSTALL, &ipmi->flags);
+	return 0;
+}
+
+static void acpi_add_ipmi_device(struct acpi_ipmi_device *ipmi_device)
+{
+
+	INIT_LIST_HEAD(&ipmi_device->head);
+	list_add_tail(&ipmi_device->head, &driver_data.ipmi_devices);
+
+	mutex_init(&ipmi_device->mutex_lock);
+	INIT_LIST_HEAD(&ipmi_device->tx_msg_list);
+	ipmi_install_space_handler(ipmi_device);
+}
+
+static void acpi_remove_ipmi_device(struct acpi_ipmi_device *ipmi_device)
+{
+	/*
+	 * If the IPMI user interface is created, it should be
+	 * destroyed.
+	 */
+	if (ipmi_device->user_interface) {
+		ipmi_destroy_user(ipmi_device->user_interface);
+		ipmi_device->user_interface = NULL;
+	}
+	/* flush the Tx_msg list */
+	if (!list_empty(&ipmi_device->tx_msg_list))
+		ipmi_flush_tx_msg(ipmi_device);
+
+	list_del(&ipmi_device->head);
+	ipmi_remove_space_handler(ipmi_device);
+}
+
+static int __init acpi_ipmi_init(void)
+{
+	int result = 0;
+
+	if (acpi_disabled)
+		return result;
+
+	result = ipmi_smi_watcher_register(&driver_data.bmc_events);
+
+	return result;
+}
+
+static void __exit acpi_ipmi_exit(void)
+{
+	struct acpi_ipmi_device *ipmi_device, *temp;
+
+	if (acpi_disabled)
+		return;
+
+	ipmi_smi_watcher_unregister(&driver_data.bmc_events);
+
+	/*
+	 * When one smi_watcher is unregistered, it is only deleted
+	 * from the smi_watcher list. But the smi_gone callback function
+	 * is not called. So explicitly uninstall the ACPI IPMI oregion
+	 * handler and free it.
+	 */
+	list_for_each_entry_safe(ipmi_device, temp,
+				&driver_data.ipmi_devices, head) {
+		acpi_remove_ipmi_device(ipmi_device);
+		kfree(ipmi_device);
+	}
+}
+
+module_init(acpi_ipmi_init);
+module_exit(acpi_ipmi_exit);
-- 
1.5.4.5

Re: [RFC PATCH 1/2_v9] IPMI: Add one interface to get more info of low-level IPMI device

[ykzhao]

On Wed, 2010-09-15 at 16:06 +0800, Zhao, Yakui wrote:
> From: Zhao Yakui <yakui.zhao@intel.com>
> 
> The IPMI smi_watcher will be used to catch the IPMI interface as they come or go.
> In order to communicate with the correct IPMI device, it should be confirmed
>  whether it is what we wanted especially on the system with multiple IPMI
> devices. But the new_smi callback function of smi_watcher provides very
> limited info(only the interface number and dev pointer) and there is no
> detailed info about the low level interface. For example: which mechanism
> registers the IPMI interface(ACPI, PCI, DMI and so on).
> 
> This is to add one interface that can get more info of low-level IPMI
> device. For example: the ACPI device handle will be returned for the pnp_acpi
> IPMI device.

Hi, Corey
	
	Any comment about this patch? Does the interface make sense?

Thanks.
> 
> Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
> ---
> In this patch only the info of pnp_acpi IPMI is provided. If the detailed info
> is also required for other mechanism, we can redesign the structure of
> ipmi_smi_data and fill it correctly.
> 
>  drivers/char/ipmi/ipmi_msghandler.c |   26 ++++++++++++++++++++++++++
>  drivers/char/ipmi/ipmi_si_intf.c    |   24 ++++++++++++++++++++----
>  include/linux/ipmi.h                |   16 ++++++++++++++++
>  include/linux/ipmi_smi.h            |   11 +++++++++++
>  4 files changed, 73 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> index 4f3f8c9..f82d426 100644
> --- a/drivers/char/ipmi/ipmi_msghandler.c
> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> @@ -970,6 +970,32 @@ out_kfree:
>  }
>  EXPORT_SYMBOL(ipmi_create_user);
>  
> +int ipmi_get_smi_info(int if_num, enum ipmi_addr_src type,
> +			union ipmi_smi_data *data)
> +{
> +	int           rv = 0;
> +	ipmi_smi_t    intf;
> +	struct ipmi_smi_handlers *handlers;
> +
> +	mutex_lock(&ipmi_interfaces_mutex);
> +	list_for_each_entry_rcu(intf, &ipmi_interfaces, link) {
> +		if (intf->intf_num == if_num)
> +			goto found;
> +	}
> +	/* Not found, return an error */
> +	rv = -EINVAL;
> +	mutex_unlock(&ipmi_interfaces_mutex);
> +	return rv;
> +
> +found:
> +	handlers = intf->handlers;
> +	rv = handlers->get_smi_info(intf->send_info, type, data);
> +	mutex_unlock(&ipmi_interfaces_mutex);
> +
> +	return rv;
> +}
> +EXPORT_SYMBOL(ipmi_get_smi_info);
> +
>  static void free_user(struct kref *ref)
>  {
>  	ipmi_user_t user = container_of(ref, struct ipmi_user, refcount);
> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> index 3822b4f..ebb87b4 100644
> --- a/drivers/char/ipmi/ipmi_si_intf.c
> +++ b/drivers/char/ipmi/ipmi_si_intf.c
> @@ -57,6 +57,7 @@
>  #include <asm/irq.h>
>  #include <linux/interrupt.h>
>  #include <linux/rcupdate.h>
> +#include <linux/ipmi.h>
>  #include <linux/ipmi_smi.h>
>  #include <asm/io.h>
>  #include "ipmi_si_sm.h"
> @@ -107,10 +108,6 @@ enum si_type {
>  };
>  static char *si_to_str[] = { "kcs", "smic", "bt" };
>  
> -enum ipmi_addr_src {
> -	SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
> -	SI_PCI,	SI_DEVICETREE, SI_DEFAULT
> -};
>  static char *ipmi_addr_src_to_str[] = { NULL, "hotmod", "hardcoded", "SPMI",
>  					"ACPI", "SMBIOS", "PCI",
>  					"device-tree", "default" };
> @@ -291,6 +288,7 @@ struct smi_info {
>  	struct task_struct *thread;
>  
>  	struct list_head link;
> +	union ipmi_smi_data smi_data;
>  };
>  
>  #define smi_inc_stat(smi, stat) \
> @@ -1183,6 +1181,21 @@ static int smi_start_processing(void       *send_info,
>  	return 0;
>  }
>  
> +static int get_smi_info(void *send_info,
> +			enum ipmi_addr_src type,
> +			union ipmi_smi_data *data)
> +{
> +	struct smi_info *new_smi = send_info;
> +	union ipmi_smi_data *smi_data = &new_smi->smi_data;
> +
> +	if (new_smi->addr_source != type)
> +		return -EINVAL;
> +
> +	memcpy(data, smi_data, sizeof(*smi_data));
> +
> +	return 0;
> +}
> +
>  static void set_maintenance_mode(void *send_info, int enable)
>  {
>  	struct smi_info   *smi_info = send_info;
> @@ -1194,6 +1207,7 @@ static void set_maintenance_mode(void *send_info, int enable)
>  static struct ipmi_smi_handlers handlers = {
>  	.owner                  = THIS_MODULE,
>  	.start_processing       = smi_start_processing,
> +	.get_smi_info		= get_smi_info,
>  	.sender			= sender,
>  	.request_events		= request_events,
>  	.set_maintenance_mode   = set_maintenance_mode,
> @@ -2140,6 +2154,8 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
>  		return -ENOMEM;
>  
>  	info->addr_source = SI_ACPI;
> +	info->smi_data.acpi_data.acpi_handle = acpi_dev->handle;
> +	info->smi_data.acpi_data.dev = &dev->dev;
>  	printk(KERN_INFO PFX "probing via ACPI\n");
>  
>  	handle = acpi_dev->handle;
> diff --git a/include/linux/ipmi.h b/include/linux/ipmi.h
> index 65aae34..04e4a90 100644
> --- a/include/linux/ipmi.h
> +++ b/include/linux/ipmi.h
> @@ -454,6 +454,22 @@ unsigned int ipmi_addr_length(int addr_type);
>  /* Validate that the given IPMI address is valid. */
>  int ipmi_validate_addr(struct ipmi_addr *addr, int len);
>  
> +enum ipmi_addr_src {
> +	SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
> +	SI_PCI,	SI_DEVICETREE, SI_DEFAULT
> +};
> +union ipmi_smi_data{
> +	/* If the other type of data is required, please add it	 */
> +	u8 data[16];
> +	struct {
> +		void *acpi_handle;
> +		struct device *dev;
> +	} acpi_data;
> +};
> +
> +/* This is to get the private info of ipmi_smi_t */
> +extern int ipmi_get_smi_info(int if_num, enum ipmi_addr_src type,
> +		union ipmi_smi_data *data);
>  #endif /* __KERNEL__ */
>  
> 
> diff --git a/include/linux/ipmi_smi.h b/include/linux/ipmi_smi.h
> index 4b48318..d5bae83 100644
> --- a/include/linux/ipmi_smi.h
> +++ b/include/linux/ipmi_smi.h
> @@ -86,6 +86,17 @@ struct ipmi_smi_handlers {
>  	int (*start_processing)(void       *send_info,
>  				ipmi_smi_t new_intf);
>  
> +	/*
> +	 * Get the detailed private info of the low level interface and store
> +	 * it into the union structure of ipmi_smi_data. For example: the
> +	 * ACPI device handle will be returned for the pnp_acpi IPMI device.
> +	 * Of course it will firstly compare the interface type of low-level
> +	 * interface(Hardcoded type, DMI, ACPI and so on). If the type doesn't
> +	 * match, it will return -EINVAL.
> +	 */
> +	int (*get_smi_info)(void *send_info, enum ipmi_addr_src type,
> +				union ipmi_smi_data *data);
> +
>  	/* Called to enqueue an SMI message to be sent.  This
>  	   operation is not allowed to fail.  If an error occurs, it
>  	   should report back the error in a received message.  It may

Re: [RFC PATCH 1/2_v9] IPMI: Add one interface to get more info of low-level IPMI device

[Corey Minyard]

On 09/26/2010 08:34 PM, ykzhao wrote:
> On Wed, 2010-09-15 at 16:06 +0800, Zhao, Yakui wrote:
>    
>> From: Zhao Yakui<yakui.zhao@intel.com>
>>
>> The IPMI smi_watcher will be used to catch the IPMI interface as they come or go.
>> In order to communicate with the correct IPMI device, it should be confirmed
>>   whether it is what we wanted especially on the system with multiple IPMI
>> devices. But the new_smi callback function of smi_watcher provides very
>> limited info(only the interface number and dev pointer) and there is no
>> detailed info about the low level interface. For example: which mechanism
>> registers the IPMI interface(ACPI, PCI, DMI and so on).
>>
>> This is to add one interface that can get more info of low-level IPMI
>> device. For example: the ACPI device handle will be returned for the pnp_acpi
>> IPMI device.
>>      
> Hi, Corey
> 	
> 	Any comment about this patch? Does the interface make sense?
>    
Well, not exactly.  I think you have things in the right places, but the 
actual design of the interface has some issues.

+enum ipmi_addr_src {
+	SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
+	SI_PCI,	SI_DEVICETREE, SI_DEFAULT
+};
+union ipmi_smi_data{
+	/* If the other type of data is required, please add it	 */
+	u8 data[16];
+	struct {
+		void *acpi_handle;
+		struct device *dev;
+	} acpi_data;
+};

There's no value in providing a union of this type.  Plus you call it 
"ipmi_smi_data" and the function is get_smi_info.  I've learned that 
consistency is a good thing.

The address source and device are fairly universal, and the address 
source can be used as a key for the other stuff, so I think the 
following makes more sense.  Just:

struct ipmi_smi_info
{
         enum ipmi_addr_src add_src;
         struct device *dev;

         union {
                 struct {
                         acpi_handle acpi_handle;
                 } acpi_addr_info;
         } addr_info;
};

In the future, if more information is needed, it can be added to the 
structure.  Since there's no guarantee of a stable binary interface, no 
need for the character array.

You will also need ifdefs in here for ACPI, includes so acpi_handle is 
available, and you need to decide  how to do refcounts on "dev" and 
document that.

-corey

> Thanks.
>    
>> Signed-off-by: Zhao Yakui<yakui.zhao@intel.com>
>> ---
>> In this patch only the info of pnp_acpi IPMI is provided. If the detailed info
>> is also required for other mechanism, we can redesign the structure of
>> ipmi_smi_data and fill it correctly.
>>
>>   drivers/char/ipmi/ipmi_msghandler.c |   26 ++++++++++++++++++++++++++
>>   drivers/char/ipmi/ipmi_si_intf.c    |   24 ++++++++++++++++++++----
>>   include/linux/ipmi.h                |   16 ++++++++++++++++
>>   include/linux/ipmi_smi.h            |   11 +++++++++++
>>   4 files changed, 73 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
>> index 4f3f8c9..f82d426 100644
>> --- a/drivers/char/ipmi/ipmi_msghandler.c
>> +++ b/drivers/char/ipmi/ipmi_msghandler.c
>> @@ -970,6 +970,32 @@ out_kfree:
>>   }
>>   EXPORT_SYMBOL(ipmi_create_user);
>>
>> +int ipmi_get_smi_info(int if_num, enum ipmi_addr_src type,
>> +			union ipmi_smi_data *data)
>> +{
>> +	int           rv = 0;
>> +	ipmi_smi_t    intf;
>> +	struct ipmi_smi_handlers *handlers;
>> +
>> +	mutex_lock(&ipmi_interfaces_mutex);
>> +	list_for_each_entry_rcu(intf,&ipmi_interfaces, link) {
>> +		if (intf->intf_num == if_num)
>> +			goto found;
>> +	}
>> +	/* Not found, return an error */
>> +	rv = -EINVAL;
>> +	mutex_unlock(&ipmi_interfaces_mutex);
>> +	return rv;
>> +
>> +found:
>> +	handlers = intf->handlers;
>> +	rv = handlers->get_smi_info(intf->send_info, type, data);
>> +	mutex_unlock(&ipmi_interfaces_mutex);
>> +
>> +	return rv;
>> +}
>> +EXPORT_SYMBOL(ipmi_get_smi_info);
>> +
>>   static void free_user(struct kref *ref)
>>   {
>>   	ipmi_user_t user = container_of(ref, struct ipmi_user, refcount);
>> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
>> index 3822b4f..ebb87b4 100644
>> --- a/drivers/char/ipmi/ipmi_si_intf.c
>> +++ b/drivers/char/ipmi/ipmi_si_intf.c
>> @@ -57,6 +57,7 @@
>>   #include<asm/irq.h>
>>   #include<linux/interrupt.h>
>>   #include<linux/rcupdate.h>
>> +#include<linux/ipmi.h>
>>   #include<linux/ipmi_smi.h>
>>   #include<asm/io.h>
>>   #include "ipmi_si_sm.h"
>> @@ -107,10 +108,6 @@ enum si_type {
>>   };
>>   static char *si_to_str[] = { "kcs", "smic", "bt" };
>>
>> -enum ipmi_addr_src {
>> -	SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
>> -	SI_PCI,	SI_DEVICETREE, SI_DEFAULT
>> -};
>>   static char *ipmi_addr_src_to_str[] = { NULL, "hotmod", "hardcoded", "SPMI",
>>   					"ACPI", "SMBIOS", "PCI",
>>   					"device-tree", "default" };
>> @@ -291,6 +288,7 @@ struct smi_info {
>>   	struct task_struct *thread;
>>
>>   	struct list_head link;
>> +	union ipmi_smi_data smi_data;
>>   };
>>
>>   #define smi_inc_stat(smi, stat) \
>> @@ -1183,6 +1181,21 @@ static int smi_start_processing(void       *send_info,
>>   	return 0;
>>   }
>>
>> +static int get_smi_info(void *send_info,
>> +			enum ipmi_addr_src type,
>> +			union ipmi_smi_data *data)
>> +{
>> +	struct smi_info *new_smi = send_info;
>> +	union ipmi_smi_data *smi_data =&new_smi->smi_data;
>> +
>> +	if (new_smi->addr_source != type)
>> +		return -EINVAL;
>> +
>> +	memcpy(data, smi_data, sizeof(*smi_data));
>> +
>> +	return 0;
>> +}
>> +
>>   static void set_maintenance_mode(void *send_info, int enable)
>>   {
>>   	struct smi_info   *smi_info = send_info;
>> @@ -1194,6 +1207,7 @@ static void set_maintenance_mode(void *send_info, int enable)
>>   static struct ipmi_smi_handlers handlers = {
>>   	.owner                  = THIS_MODULE,
>>   	.start_processing       = smi_start_processing,
>> +	.get_smi_info		= get_smi_info,
>>   	.sender			= sender,
>>   	.request_events		= request_events,
>>   	.set_maintenance_mode   = set_maintenance_mode,
>> @@ -2140,6 +2154,8 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
>>   		return -ENOMEM;
>>
>>   	info->addr_source = SI_ACPI;
>> +	info->smi_data.acpi_data.acpi_handle = acpi_dev->handle;
>> +	info->smi_data.acpi_data.dev =&dev->dev;
>>   	printk(KERN_INFO PFX "probing via ACPI\n");
>>
>>   	handle = acpi_dev->handle;
>> diff --git a/include/linux/ipmi.h b/include/linux/ipmi.h
>> index 65aae34..04e4a90 100644
>> --- a/include/linux/ipmi.h
>> +++ b/include/linux/ipmi.h
>> @@ -454,6 +454,22 @@ unsigned int ipmi_addr_length(int addr_type);
>>   /* Validate that the given IPMI address is valid. */
>>   int ipmi_validate_addr(struct ipmi_addr *addr, int len);
>>
>> +enum ipmi_addr_src {
>> +	SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
>> +	SI_PCI,	SI_DEVICETREE, SI_DEFAULT
>> +};
>> +union ipmi_smi_data{
>> +	/* If the other type of data is required, please add it	 */
>> +	u8 data[16];
>> +	struct {
>> +		void *acpi_handle;
>> +		struct device *dev;
>> +	} acpi_data;
>> +};
>> +
>> +/* This is to get the private info of ipmi_smi_t */
>> +extern int ipmi_get_smi_info(int if_num, enum ipmi_addr_src type,
>> +		union ipmi_smi_data *data);
>>   #endif /* __KERNEL__ */
>>
>>
>> diff --git a/include/linux/ipmi_smi.h b/include/linux/ipmi_smi.h
>> index 4b48318..d5bae83 100644
>> --- a/include/linux/ipmi_smi.h
>> +++ b/include/linux/ipmi_smi.h
>> @@ -86,6 +86,17 @@ struct ipmi_smi_handlers {
>>   	int (*start_processing)(void       *send_info,
>>   				ipmi_smi_t new_intf);
>>
>> +	/*
>> +	 * Get the detailed private info of the low level interface and store
>> +	 * it into the union structure of ipmi_smi_data. For example: the
>> +	 * ACPI device handle will be returned for the pnp_acpi IPMI device.
>> +	 * Of course it will firstly compare the interface type of low-level
>> +	 * interface(Hardcoded type, DMI, ACPI and so on). If the type doesn't
>> +	 * match, it will return -EINVAL.
>> +	 */
>> +	int (*get_smi_info)(void *send_info, enum ipmi_addr_src type,
>> +				union ipmi_smi_data *data);
>> +
>>   	/* Called to enqueue an SMI message to be sent.  This
>>   	   operation is not allowed to fail.  If an error occurs, it
>>   	   should report back the error in a received message.  It may
>>      
>    

Re: [RFC PATCH 1/2_v9] IPMI: Add one interface to get more info of low-level IPMI device

[ykzhao]

On Mon, 2010-09-27 at 21:27 +0800, Corey Minyard wrote:
> On 09/26/2010 08:34 PM, ykzhao wrote:
> > On Wed, 2010-09-15 at 16:06 +0800, Zhao, Yakui wrote:
> >    
> >> From: Zhao Yakui<yakui.zhao@intel.com>
> >>
> >> The IPMI smi_watcher will be used to catch the IPMI interface as they come or go.
> >> In order to communicate with the correct IPMI device, it should be confirmed
> >>   whether it is what we wanted especially on the system with multiple IPMI
> >> devices. But the new_smi callback function of smi_watcher provides very
> >> limited info(only the interface number and dev pointer) and there is no
> >> detailed info about the low level interface. For example: which mechanism
> >> registers the IPMI interface(ACPI, PCI, DMI and so on).
> >>
> >> This is to add one interface that can get more info of low-level IPMI
> >> device. For example: the ACPI device handle will be returned for the pnp_acpi
> >> IPMI device.
> >>      
> > Hi, Corey
> > 	
> > 	Any comment about this patch? Does the interface make sense?
> >    
> Well, not exactly.  I think you have things in the right places, but the 
> actual design of the interface has some issues.

Thanks for the comments. 

I will follow your comments and update the corresponding data
structure. 

> 
> +enum ipmi_addr_src {
> +	SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
> +	SI_PCI,	SI_DEVICETREE, SI_DEFAULT
> +};
> +union ipmi_smi_data{
> +	/* If the other type of data is required, please add it	 */
> +	u8 data[16];
> +	struct {
> +		void *acpi_handle;
> +		struct device *dev;
> +	} acpi_data;
> +};
> 
> There's no value in providing a union of this type.  Plus you call it 
> "ipmi_smi_data" and the function is get_smi_info.  I've learned that 
> consistency is a good thing.
> 
> The address source and device are fairly universal, and the address 
> source can be used as a key for the other stuff, so I think the 
> following makes more sense.  Just:
> 
> struct ipmi_smi_info
> {
>          enum ipmi_addr_src add_src;
>          struct device *dev;
> 
>          union {
>                  struct {
>                          acpi_handle acpi_handle;
>                  } acpi_addr_info;
>          } addr_info;
> };
> 
> In the future, if more information is needed, it can be added to the 
> structure.  Since there's no guarantee of a stable binary interface, no 
> need for the character array.
> 
> You will also need ifdefs in here for ACPI, includes so acpi_handle is 
> available, and you need to decide  how to do refcounts on "dev" and 
> document that.
> 
> -corey
> 
> > Thanks.
> >    
> >> Signed-off-by: Zhao Yakui<yakui.zhao@intel.com>
> >> ---
> >> In this patch only the info of pnp_acpi IPMI is provided. If the detailed info
> >> is also required for other mechanism, we can redesign the structure of
> >> ipmi_smi_data and fill it correctly.
> >>
> >>   drivers/char/ipmi/ipmi_msghandler.c |   26 ++++++++++++++++++++++++++
> >>   drivers/char/ipmi/ipmi_si_intf.c    |   24 ++++++++++++++++++++----
> >>   include/linux/ipmi.h                |   16 ++++++++++++++++
> >>   include/linux/ipmi_smi.h            |   11 +++++++++++
> >>   4 files changed, 73 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> >> index 4f3f8c9..f82d426 100644
> >> --- a/drivers/char/ipmi/ipmi_msghandler.c
> >> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> >> @@ -970,6 +970,32 @@ out_kfree:
> >>   }
> >>   EXPORT_SYMBOL(ipmi_create_user);
> >>
> >> +int ipmi_get_smi_info(int if_num, enum ipmi_addr_src type,
> >> +			union ipmi_smi_data *data)
> >> +{
> >> +	int           rv = 0;
> >> +	ipmi_smi_t    intf;
> >> +	struct ipmi_smi_handlers *handlers;
> >> +
> >> +	mutex_lock(&ipmi_interfaces_mutex);
> >> +	list_for_each_entry_rcu(intf,&ipmi_interfaces, link) {
> >> +		if (intf->intf_num == if_num)
> >> +			goto found;
> >> +	}
> >> +	/* Not found, return an error */
> >> +	rv = -EINVAL;
> >> +	mutex_unlock(&ipmi_interfaces_mutex);
> >> +	return rv;
> >> +
> >> +found:
> >> +	handlers = intf->handlers;
> >> +	rv = handlers->get_smi_info(intf->send_info, type, data);
> >> +	mutex_unlock(&ipmi_interfaces_mutex);
> >> +
> >> +	return rv;
> >> +}
> >> +EXPORT_SYMBOL(ipmi_get_smi_info);
> >> +
> >>   static void free_user(struct kref *ref)
> >>   {
> >>   	ipmi_user_t user = container_of(ref, struct ipmi_user, refcount);
> >> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> >> index 3822b4f..ebb87b4 100644
> >> --- a/drivers/char/ipmi/ipmi_si_intf.c
> >> +++ b/drivers/char/ipmi/ipmi_si_intf.c
> >> @@ -57,6 +57,7 @@
> >>   #include<asm/irq.h>
> >>   #include<linux/interrupt.h>
> >>   #include<linux/rcupdate.h>
> >> +#include<linux/ipmi.h>
> >>   #include<linux/ipmi_smi.h>
> >>   #include<asm/io.h>
> >>   #include "ipmi_si_sm.h"
> >> @@ -107,10 +108,6 @@ enum si_type {
> >>   };
> >>   static char *si_to_str[] = { "kcs", "smic", "bt" };
> >>
> >> -enum ipmi_addr_src {
> >> -	SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
> >> -	SI_PCI,	SI_DEVICETREE, SI_DEFAULT
> >> -};
> >>   static char *ipmi_addr_src_to_str[] = { NULL, "hotmod", "hardcoded", "SPMI",
> >>   					"ACPI", "SMBIOS", "PCI",
> >>   					"device-tree", "default" };
> >> @@ -291,6 +288,7 @@ struct smi_info {
> >>   	struct task_struct *thread;
> >>
> >>   	struct list_head link;
> >> +	union ipmi_smi_data smi_data;
> >>   };
> >>
> >>   #define smi_inc_stat(smi, stat) \
> >> @@ -1183,6 +1181,21 @@ static int smi_start_processing(void       *send_info,
> >>   	return 0;
> >>   }
> >>
> >> +static int get_smi_info(void *send_info,
> >> +			enum ipmi_addr_src type,
> >> +			union ipmi_smi_data *data)
> >> +{
> >> +	struct smi_info *new_smi = send_info;
> >> +	union ipmi_smi_data *smi_data =&new_smi->smi_data;
> >> +
> >> +	if (new_smi->addr_source != type)
> >> +		return -EINVAL;
> >> +
> >> +	memcpy(data, smi_data, sizeof(*smi_data));
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>   static void set_maintenance_mode(void *send_info, int enable)
> >>   {
> >>   	struct smi_info   *smi_info = send_info;
> >> @@ -1194,6 +1207,7 @@ static void set_maintenance_mode(void *send_info, int enable)
> >>   static struct ipmi_smi_handlers handlers = {
> >>   	.owner                  = THIS_MODULE,
> >>   	.start_processing       = smi_start_processing,
> >> +	.get_smi_info		= get_smi_info,
> >>   	.sender			= sender,
> >>   	.request_events		= request_events,
> >>   	.set_maintenance_mode   = set_maintenance_mode,
> >> @@ -2140,6 +2154,8 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
> >>   		return -ENOMEM;
> >>
> >>   	info->addr_source = SI_ACPI;
> >> +	info->smi_data.acpi_data.acpi_handle = acpi_dev->handle;
> >> +	info->smi_data.acpi_data.dev =&dev->dev;
> >>   	printk(KERN_INFO PFX "probing via ACPI\n");
> >>
> >>   	handle = acpi_dev->handle;
> >> diff --git a/include/linux/ipmi.h b/include/linux/ipmi.h
> >> index 65aae34..04e4a90 100644
> >> --- a/include/linux/ipmi.h
> >> +++ b/include/linux/ipmi.h
> >> @@ -454,6 +454,22 @@ unsigned int ipmi_addr_length(int addr_type);
> >>   /* Validate that the given IPMI address is valid. */
> >>   int ipmi_validate_addr(struct ipmi_addr *addr, int len);
> >>
> >> +enum ipmi_addr_src {
> >> +	SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
> >> +	SI_PCI,	SI_DEVICETREE, SI_DEFAULT
> >> +};
> >> +union ipmi_smi_data{
> >> +	/* If the other type of data is required, please add it	 */
> >> +	u8 data[16];
> >> +	struct {
> >> +		void *acpi_handle;
> >> +		struct device *dev;
> >> +	} acpi_data;
> >> +};
> >> +
> >> +/* This is to get the private info of ipmi_smi_t */
> >> +extern int ipmi_get_smi_info(int if_num, enum ipmi_addr_src type,
> >> +		union ipmi_smi_data *data);
> >>   #endif /* __KERNEL__ */
> >>
> >>
> >> diff --git a/include/linux/ipmi_smi.h b/include/linux/ipmi_smi.h
> >> index 4b48318..d5bae83 100644
> >> --- a/include/linux/ipmi_smi.h
> >> +++ b/include/linux/ipmi_smi.h
> >> @@ -86,6 +86,17 @@ struct ipmi_smi_handlers {
> >>   	int (*start_processing)(void       *send_info,
> >>   				ipmi_smi_t new_intf);
> >>
> >> +	/*
> >> +	 * Get the detailed private info of the low level interface and store
> >> +	 * it into the union structure of ipmi_smi_data. For example: the
> >> +	 * ACPI device handle will be returned for the pnp_acpi IPMI device.
> >> +	 * Of course it will firstly compare the interface type of low-level
> >> +	 * interface(Hardcoded type, DMI, ACPI and so on). If the type doesn't
> >> +	 * match, it will return -EINVAL.
> >> +	 */
> >> +	int (*get_smi_info)(void *send_info, enum ipmi_addr_src type,
> >> +				union ipmi_smi_data *data);
> >> +
> >>   	/* Called to enqueue an SMI message to be sent.  This
> >>   	   operation is not allowed to fail.  If an error occurs, it
> >>   	   should report back the error in a received message.  It may
> >>      
> >    
>