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

[PATCH -v6 0/2] IPMI: 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]: Install the IPMI space handler to enable ACPI to access the BMC
controller
[Patch 02/02]: use ACPI detection mechanism firstly to detect IPMI system
interface.

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 -v6 1/2] IPMI: Install the IPMI space handler to enable ACPI to access the 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.

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

diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 35603dd..87b5722 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -65,7 +65,7 @@
 #include <linux/string.h>
 #include <linux/ctype.h>
 #include <linux/pnp.h>
-
+#include <linux/ipmi.h>
 #ifdef CONFIG_PPC_OF
 #include <linux/of_device.h>
 #include <linux/of_platform.h>
@@ -1894,6 +1894,429 @@ static __devinit void hardcode_find_bmc(void)
 
 #include <linux/acpi.h>
 
+#define IPMI_FLAGS_HANDLER_INSTALL	0
+#define ACPI_IPMI_OK			0
+#define ACPI_IPMI_TIMEOUT		0x10
+#define ACPI_IPMI_UNKNOWN		0x07
+/* the IPMI timeout is 30s */
+#define IPMI_TIMEOUT			(30 * HZ)
+
+struct acpi_ipmi_device {
+	acpi_handle handle;
+	/* the device list attached to driver_data.ipmi_devices */
+	struct list_head head;
+	ipmi_user_t 	user_interface;
+	struct mutex	mutex_lock;
+	struct list_head tx_msg_list;
+	long curr_msgid;
+	unsigned long flags;
+	int ipmi_ifnum;
+	struct smi_info *smi_info;
+	struct pnp_dev *pnp_dev;
+};
+
+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 {
+	/* message list */
+	struct list_head head;
+	/*
+	 * General speaking the addr type should be SI_ADDR_TYPE. And
+	 * the addr channel should be BMC.
+	 * In fact it can also be IPMB type. But we will have to
+	 * parse it from the Netfn command buffer. It is so complex
+	 * that it is skipped.
+	 */
+	struct ipmi_addr addr;
+	long tx_msgid;
+	/* it is used to track whether the IPMI message is finished */
+	struct completion tx_complete;
+	struct kernel_ipmi_msg tx_message;
+	int	msg_done;
+	/* tx data . And copy it from ACPI object buffer */
+	u8	tx_data[64];
+	int	tx_len;
+	/* get the response data */
+	u8	rx_data[64];
+	/* the response length. The netfn & cmd is excluded. */
+	int	rx_len;
+	struct acpi_ipmi_device *device;
+};
+
+/* IPMI request/response buffer 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,
+	},
+};
+
+static
+struct acpi_ipmi_msg *acpi_alloc_ipmi_msg(struct acpi_ipmi_device *ipmi)
+{
+	struct acpi_ipmi_msg *ipmi_msg;
+
+	ipmi_msg = kzalloc(sizeof(struct acpi_ipmi_msg), GFP_KERNEL);
+	if (!ipmi_msg)
+		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;
+
+	/*
+	 * 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 = &tx_msg->tx_message;
+	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) {
+		buffer->status = ACPI_IPMI_TIMEOUT;
+		return;
+	}
+	/*
+	 * If the flag of msg_done is not set, it means that the IPMI command
+	 * is not executed correctly.
+	 * The status will be ACPI_IPMI_UNKNOWN.
+	 */
+	if (!msg->msg_done) {
+		buffer->status = ACPI_IPMI_UNKNOWN;
+		return;
+	}
+
+	buffer->status = ACPI_IPMI_OK;
+	buffer->length = msg->rx_len;
+	memcpy(buffer->data, msg->rx_data, msg->rx_len);
+}
+
+static void ipmi_destroy_tx_msg(struct acpi_ipmi_device *ipmi)
+{
+	struct acpi_ipmi_msg *tx_msg = NULL, *temp;
+	int count = 20;
+	struct pnp_dev *dev;
+
+	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(&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 *dev;
+
+	dev = ipmi_device->pnp_dev;
+
+	if (msg->user != ipmi_device->user_interface) {
+		dev_warn(&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) {
+			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(&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, *temp;
+	struct pnp_dev *device;
+	ipmi_user_t		user;
+	int err;
+
+	if (list_empty(&driver_data.ipmi_devices))
+		return;
+
+	list_for_each_entry_safe(ipmi_device, temp,
+					&driver_data.ipmi_devices, head) {
+		/*
+		 * 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
+		 */
+		if (ipmi_device->user_interface)
+			continue;
+
+		device = ipmi_device->pnp_dev;
+		/*
+		 * If the dev is identical to the ACPI device,
+		 * create the user interface.
+		 */
+		if (dev == &device->dev) {
+			err = ipmi_create_user(iface, &driver_data.ipmi_hndlrs,
+						ipmi_device, &user);
+			if (err == 0) {
+				ipmi_device->user_interface = user;
+				ipmi_device->ipmi_ifnum = iface;
+			}
+		}
+	}
+}
+
+static void ipmi_bmc_gone(int iface)
+{
+	struct acpi_ipmi_device *ipmi_device, *temp;
+
+	if (list_empty(&driver_data.ipmi_devices))
+		return;
+
+	list_for_each_entry_safe(ipmi_device, temp,
+				&driver_data.ipmi_devices, head) {
+		if (ipmi_device->user_interface &&
+				(ipmi_device->ipmi_ifnum == iface)) {
+			ipmi_destroy_user(ipmi_device->user_interface);
+			ipmi_device->user_interface = NULL;
+			ipmi_destroy_tx_msg(ipmi_device);
+		}
+	}
+}
+/* --------------------------------------------------------------------------
+ * 			Address Space Management
+   -------------------------------------------------------------------------- */
+/*
+ * This is the IPMI opregion space handler.
+ * @function: indicates the read/write. In fact as the IPMI message is driven
+ * by command, only write is meaningful.
+ * @address: This contains the netfn/command of IPMI request message.
+ * @bits   : not used.
+ * @value  : it is an in/out parameter. It points to the IPMI message buffer.
+ *	     Before the IPMI message is sent, it represents the actual request
+ *	     IPMI message. After the IPMI message is finished, it represents
+ *	     the response IPMI message returned by IPMI command.
+ * @handler_context: IPMI device context.
+ */
+
+static acpi_status
+acpi_ipmi_space_handler(u32 function, acpi_physical_address address,
+		      u32 bits, acpi_integer *value,
+		      void *handler_context, void *region_context)
+{
+	struct acpi_ipmi_msg *tx_msg = NULL;
+	struct acpi_ipmi_device *ipmi_device =
+			(struct acpi_ipmi_device *) handler_context;
+	int err;
+	acpi_status status;
+
+	/*
+	 * IPMI opregion message.
+	 * IPMI message is firstly written to the BMC and system software
+	 * can get the respsonse. So it is unmeaningful for the IPMI read
+	 * access.
+	 */
+	if ((function & ACPI_IO_MASK) == ACPI_READ)
+		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 *dev;
+		dev = ipmi->pnp_dev;
+		dev_err(&dev->dev, "Can't register IPMI opregion for %s\n",
+					pnp_dev_name(dev));
+		return -EINVAL;
+	}
+
+	set_bit(IPMI_FLAGS_HANDLER_INSTALL, &ipmi->flags);
+
+	return 0;
+}
+
+static void acpi_register_ipmi_handler(struct acpi_ipmi_device *ipmi_device)
+{
+	if (list_empty(&driver_data.ipmi_devices)) {
+		/*
+		 * when we try to register the first IPI0001 device, register
+		 * the smi_watcher.
+		 */
+		ipmi_smi_watcher_register(&driver_data.bmc_events);
+	}
+
+	mutex_init(&ipmi_device->mutex_lock);
+	INIT_LIST_HEAD(&ipmi_device->tx_msg_list);
+
+	ipmi_install_handler(ipmi_device);
+
+	list_add(&ipmi_device->head, &driver_data.ipmi_devices);
+}
+
+static void acpi_remove_ipmi_handler(struct acpi_ipmi_device *ipmi_device)
+{
+	if (ipmi_device->user_interface) {
+		ipmi_destroy_user(ipmi_device->user_interface);
+		ipmi_device->user_interface = NULL;
+	}
+	list_del(&ipmi_device->head);
+
+	if (!list_empty(&ipmi_device->tx_msg_list))
+		ipmi_destroy_tx_msg(ipmi_device);
+
+	ipmi_remove_handler(ipmi_device);
+
+	if (list_empty(&driver_data.ipmi_devices))
+		ipmi_smi_watcher_unregister(&driver_data.bmc_events);
+
+}
+
 /*
  * Once we get an ACPI failure, we don't try any more, because we go
  * through the tables sequentially.  Once we don't find a table, there
@@ -2118,6 +2541,8 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
 	acpi_handle handle;
 	acpi_status status;
 	unsigned long long tmp;
+	int ret;
+	struct acpi_ipmi_device *p_ipmi;
 
 	acpi_dev = pnp_acpi_device(dev);
 	if (!acpi_dev)
@@ -2127,6 +2552,12 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
 	if (!info)
 		return -ENOMEM;
 
+	p_ipmi = kzalloc(sizeof(*p_ipmi), GFP_KERNEL);
+	if (!p_ipmi) {
+		kfree(info);
+		return -ENOMEM;
+	}
+
 	info->addr_source = SI_ACPI;
 	printk(KERN_INFO PFX "probing via ACPI\n");
 
@@ -2184,24 +2615,46 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
 	}
 
 	info->dev = &dev->dev;
-	pnp_set_drvdata(dev, info);
+
+	p_ipmi->smi_info = info;
+
+	p_ipmi->handle = handle;
+	p_ipmi->ipmi_ifnum = -1;
+	p_ipmi->pnp_dev = dev;
+
+	pnp_set_drvdata(dev, p_ipmi);
+	acpi_register_ipmi_handler(p_ipmi);
 
 	dev_info(info->dev, "%pR regsize %d spacing %d irq %d\n",
 		 res, info->io.regsize, info->io.regspacing,
 		 info->irq);
 
-	return add_smi(info);
+	ret = add_smi(info);
 
+	if (!ret)
+		return ret;
+	else {
+		acpi_remove_ipmi_handler(p_ipmi);
+		pnp_set_drvdata(dev, NULL);
+	}
 err_free:
+	kfree(p_ipmi);
 	kfree(info);
 	return -EINVAL;
 }
 
 static void __devexit ipmi_pnp_remove(struct pnp_dev *dev)
 {
-	struct smi_info *info = pnp_get_drvdata(dev);
+	struct acpi_ipmi_device *p_ipmi;
+	struct smi_info *info;
+
+	p_ipmi = pnp_get_drvdata(dev);
+	info = p_ipmi->smi_info;
 
 	cleanup_one_si(info);
+	acpi_remove_ipmi_handler(p_ipmi);
+	pnp_set_drvdata(dev, NULL);
+	kfree(p_ipmi);
 }
 
 static const struct pnp_device_id pnp_dev_table[] = {
-- 
1.5.4.5

[PATCH -v6 2/2] IPMI: use ACPI detection mechanism firstly to detect IPMI system interface

[yakui.zhao]

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

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

Use the ACPI detection mechanism firstly to detect the IPMI system interface
so that we can know which IPMI system interface is detected in ACPI namespace and
then install the IPMI opregion to enable ACPI to access the BMC controller.
But the hardcode detection mechanism is still put in the first order.

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

diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 87b5722..2807749 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -3763,16 +3763,16 @@ static __devinit int init_ipmi_si(void)
 	}
 	mutex_unlock(&smi_infos_lock);
 
+#ifdef CONFIG_ACPI
+	pnp_register_driver(&ipmi_pnp_driver);
+#endif
+
 #ifdef CONFIG_PCI
 	rv = pci_register_driver(&ipmi_pci_driver);
 	if (rv)
 		printk(KERN_ERR PFX "Unable to register PCI driver: %d\n", rv);
 #endif
 
-#ifdef CONFIG_ACPI
-	pnp_register_driver(&ipmi_pnp_driver);
-#endif
-
 #ifdef CONFIG_DMI
 	dmi_find_bmc();
 #endif
@@ -3933,6 +3933,7 @@ static __exit void cleanup_ipmi_si(void)
 #ifdef CONFIG_PCI
 	pci_unregister_driver(&ipmi_pci_driver);
 #endif
+
 #ifdef CONFIG_ACPI
 	pnp_unregister_driver(&ipmi_pnp_driver);
 #endif
-- 
1.5.4.5

Re: [PATCH -v6 2/2] IPMI: use ACPI detection mechanism firstly to detect IPMI system interface

[Matthew Garrett]

On Mon, Jun 07, 2010 at 05:27:22PM +0800, yakui.zhao@intel.com wrote:

> Use the ACPI detection mechanism firstly to detect the IPMI system interface
> so that we can know which IPMI system interface is detected in ACPI namespace and
> then install the IPMI opregion to enable ACPI to access the BMC controller.
> But the hardcode detection mechanism is still put in the first order.

This is less than ideal. I've seen several machines where the PCI IPMI 
device is the only one to provide interrupts, so using the ACPI one and 
refusing to register the PCI one isn't really an option.

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

Re: [PATCH -v6 2/2] IPMI: use ACPI detection mechanism firstly to detect IPMI system interface

[ykzhao]

On Mon, 2010-06-07 at 20:52 +0800, Matthew Garrett wrote:
> On Mon, Jun 07, 2010 at 05:27:22PM +0800, yakui.zhao@intel.com wrote:
> 
> > Use the ACPI detection mechanism firstly to detect the IPMI system interface
> > so that we can know which IPMI system interface is detected in ACPI namespace and
> > then install the IPMI opregion to enable ACPI to access the BMC controller.
> > But the hardcode detection mechanism is still put in the first order.
> 
> This is less than ideal. I've seen several machines where the PCI IPMI 
> device is the only one to provide interrupts, so using the ACPI one and 
> refusing to register the PCI one isn't really an option.

Does there exist the ACPI detection mechanism on the machines you
mentioned? If exists, does it detect the same IPMI interface with the
PCI IPMI detection mechanism?

If the two mechanisms will detect the same IPMI interface, I agree with
what you are concerned.  Do you have an idea/thought to set up the
relationship between ACPI and IPMI interface? In order to enable that
AML code can access the IPMI, it should know which IPMI interface will
be accessed and create the corresponding user interface. If ACPI
mechanism will fail to register the IPMI interface, maybe it is
difficult to create the correct user interface.

Thanks
    Yakui






--
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 -v6 2/2] IPMI: use ACPI detection mechanism firstly to detect IPMI system interface

[Matthew Garrett]

On Tue, Jun 08, 2010 at 09:28:51AM +0800, ykzhao wrote:

> Does there exist the ACPI detection mechanism on the machines you
> mentioned? If exists, does it detect the same IPMI interface with the
> PCI IPMI detection mechanism?

What is "the same"? It's not using the same ioport space, certainly.

> If the two mechanisms will detect the same IPMI interface, I agree with
> what you are concerned.  Do you have an idea/thought to set up the
> relationship between ACPI and IPMI interface? In order to enable that
> AML code can access the IPMI, it should know which IPMI interface will
> be accessed and create the corresponding user interface. If ACPI
> mechanism will fail to register the IPMI interface, maybe it is
> difficult to create the correct user interface.

Well, right now if you change the ordering then the PCI interface will 
never be exposed. It would be preferable to only expose the ACPI 
interface as a user-visible device if there's no prior device - if there 
is, I think the ideal solution would be for it to be an in-kernel only 
device without a corresponding UI.

-- 
Matthew Garrett | mjg59@srcf.ucam.org
--
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 -v6 2/2] IPMI: use ACPI detection mechanism firstly to detect IPMI system interface

[ykzhao]

On Tue, 2010-06-08 at 09:34 +0800, Matthew Garrett wrote:
> On Tue, Jun 08, 2010 at 09:28:51AM +0800, ykzhao wrote:
> 
> > Does there exist the ACPI detection mechanism on the machines you
> > mentioned? If exists, does it detect the same IPMI interface with the
> > PCI IPMI detection mechanism?
> 
> What is "the same"? It's not using the same ioport space, certainly.

"The same" means that they will use the same ioport space/address.
If they use the different ioport space/address, they will be regarded as
the different IPMI device.

> 
> > If the two mechanisms will detect the same IPMI interface, I agree with
> > what you are concerned.  Do you have an idea/thought to set up the
> > relationship between ACPI and IPMI interface? In order to enable that
> > AML code can access the IPMI, it should know which IPMI interface will
> > be accessed and create the corresponding user interface. If ACPI
> > mechanism will fail to register the IPMI interface, maybe it is
> > difficult to create the correct user interface.
> 
> Well, right now if you change the ordering then the PCI interface will 
> never be exposed. It would be preferable to only expose the ACPI 
> interface as a user-visible device if there's no prior device - if there 
> is, I think the ideal solution would be for it to be an in-kernel only 
> device without a corresponding UI.

Sorry that I don't explain it clearly. The concept of "user interface"
in IPMI interface is only a channel that can be used to communicate with
the IPMI controller. It has no relationship with whether the IPMI
interface should be exposed to user space. If one driver wants to
communicate with one IPMI interface, we should create one "user
interface" firstly and send the corresponding IPMI message by using the
"user interface".

If one IPMI interface(controller) is already detected by PCI mechanism,
then ACPI will fail to detect the same IPMI interface. In such case it
is difficult for ACPI to know which IPMI interface should be accessed
when the ACPI AML code need to communicate with the IPMI interface.

thanks
   Yakui

> 

--
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 -v6 2/2] IPMI: use ACPI detection mechanism firstly to detect IPMI system interface

[Myron Stowe]

On Tue, 2010-06-08 at 13:10 +0800, ykzhao wrote:
> On Tue, 2010-06-08 at 09:34 +0800, Matthew Garrett wrote:
> > On Tue, Jun 08, 2010 at 09:28:51AM +0800, ykzhao wrote:
> > 
> > > Does there exist the ACPI detection mechanism on the machines you
> > > mentioned? If exists, does it detect the same IPMI interface with the
> > > PCI IPMI detection mechanism?
> > 
> > What is "the same"? It's not using the same ioport space, certainly.
> 
> "The same" means that they will use the same ioport space/address.
> If they use the different ioport space/address, they will be regarded as
> the different IPMI device.
ACPI namespace* should not be enumerating a device that is discoverable
via standard PCI device enumeration so if there are multiple IPMI
devices in a system, where one is enumerated via ACPI namespace and
another is enumerated via PCI they should never be "the same" literal
device.

* I specifically denoted namespace since the ACPI SPMI table, yet
another ACPI based IPMI enumeration mechanism, seems as if it could
denote "the same" literal device.


Matthew's approach, giving a PCI based IPMI device precedence due to its
inclusion of interrupt information, does seem to have merit.

Myron
> 
> > 
> > > If the two mechanisms will detect the same IPMI interface, I agree with
> > > what you are concerned.  Do you have an idea/thought to set up the
> > > relationship between ACPI and IPMI interface? In order to enable that
> > > AML code can access the IPMI, it should know which IPMI interface will
> > > be accessed and create the corresponding user interface. If ACPI
> > > mechanism will fail to register the IPMI interface, maybe it is
> > > difficult to create the correct user interface.
> > 
> > Well, right now if you change the ordering then the PCI interface will 
> > never be exposed. It would be preferable to only expose the ACPI 
> > interface as a user-visible device if there's no prior device - if there 
> > is, I think the ideal solution would be for it to be an in-kernel only 
> > device without a corresponding UI.
> 
> Sorry that I don't explain it clearly. The concept of "user interface"
> in IPMI interface is only a channel that can be used to communicate with
> the IPMI controller. It has no relationship with whether the IPMI
> interface should be exposed to user space. If one driver wants to
> communicate with one IPMI interface, we should create one "user
> interface" firstly and send the corresponding IPMI message by using the
> "user interface".
> 
> If one IPMI interface(controller) is already detected by PCI mechanism,
> then ACPI will fail to detect the same IPMI interface. In such case it
> is difficult for ACPI to know which IPMI interface should be accessed
> when the ACPI AML code need to communicate with the IPMI interface.
> 
> thanks
>    Yakui
> 
> > 
> 


-- 
Myron Stowe                             HP Open Source Linux Lab (OSLL)

--
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 -v6 2/2] IPMI: use ACPI detection mechanism firstly to detect IPMI system interface

[Corey Minyard]

ykzhao wrote:
> On Tue, 2010-06-08 at 09:34 +0800, Matthew Garrett wrote:
>   
>> On Tue, Jun 08, 2010 at 09:28:51AM +0800, ykzhao wrote:
>>
>>     
>>> Does there exist the ACPI detection mechanism on the machines you
>>> mentioned? If exists, does it detect the same IPMI interface with the
>>> PCI IPMI detection mechanism?
>>>       
>> What is "the same"? It's not using the same ioport space, certainly.
>>     
>
> "The same" means that they will use the same ioport space/address.
> If they use the different ioport space/address, they will be regarded as
> the different IPMI device.
>
>   
>>> If the two mechanisms will detect the same IPMI interface, I agree with
>>> what you are concerned.  Do you have an idea/thought to set up the
>>> relationship between ACPI and IPMI interface? In order to enable that
>>> AML code can access the IPMI, it should know which IPMI interface will
>>> be accessed and create the corresponding user interface. If ACPI
>>> mechanism will fail to register the IPMI interface, maybe it is
>>> difficult to create the correct user interface.
>>>       
>> Well, right now if you change the ordering then the PCI interface will 
>> never be exposed. It would be preferable to only expose the ACPI 
>> interface as a user-visible device if there's no prior device - if there 
>> is, I think the ideal solution would be for it to be an in-kernel only 
>> device without a corresponding UI.
>>     
>
> Sorry that I don't explain it clearly. The concept of "user interface"
> in IPMI interface is only a channel that can be used to communicate with
> the IPMI controller. It has no relationship with whether the IPMI
> interface should be exposed to user space. If one driver wants to
> communicate with one IPMI interface, we should create one "user
> interface" firstly and send the corresponding IPMI message by using the
> "user interface".
>
> If one IPMI interface(controller) is already detected by PCI mechanism,
> then ACPI will fail to detect the same IPMI interface. In such case it
> is difficult for ACPI to know which IPMI interface should be accessed
> when the ACPI AML code need to communicate with the IPMI interface.
>   
This may all be true, but the IPMI specification clearly gives the 
detection order.  It's not a good idea to deviate from that without a 
very good reason.  I'm not sure this is a good enough reason.

Also, the bulk of this code clearly has nothing to do with the system 
interface itself and should be in its own file.  And is there any reason 
to tie this to the SMI code, anyway?  There are plenty of systems with 
IPMI and ACPI that use I2C or serial ports.  Can they not use this ACPI 
function (even though their drivers are not in the mainstream kernel)?

Can you look and see if there is some other way to detect the ACPI 
function on a BMC with some other mechanism?  I think that would be 
better all around.  If it's the same BMC, the I can't imagine it matters 
if you send the messages via the PCI-detected or ACPI-detected mechanisms.

-corey
--
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 -v6 2/2] IPMI: use ACPI detection mechanism firstly to detect IPMI system interface

[ykzhao]

On Wed, 2010-06-09 at 21:08 +0800, Corey Minyard wrote:
> ykzhao wrote:
> > On Tue, 2010-06-08 at 09:34 +0800, Matthew Garrett wrote:
> >   
> >> On Tue, Jun 08, 2010 at 09:28:51AM +0800, ykzhao wrote:
> >>
> >>     
> >>> Does there exist the ACPI detection mechanism on the machines you
> >>> mentioned? If exists, does it detect the same IPMI interface with the
> >>> PCI IPMI detection mechanism?
> >>>       
> >> What is "the same"? It's not using the same ioport space, certainly.
> >>     
> >
> > "The same" means that they will use the same ioport space/address.
> > If they use the different ioport space/address, they will be regarded as
> > the different IPMI device.
> >
> >   
> >>> If the two mechanisms will detect the same IPMI interface, I agree with
> >>> what you are concerned.  Do you have an idea/thought to set up the
> >>> relationship between ACPI and IPMI interface? In order to enable that
> >>> AML code can access the IPMI, it should know which IPMI interface will
> >>> be accessed and create the corresponding user interface. If ACPI
> >>> mechanism will fail to register the IPMI interface, maybe it is
> >>> difficult to create the correct user interface.
> >>>       
> >> Well, right now if you change the ordering then the PCI interface will 
> >> never be exposed. It would be preferable to only expose the ACPI 
> >> interface as a user-visible device if there's no prior device - if there 
> >> is, I think the ideal solution would be for it to be an in-kernel only 
> >> device without a corresponding UI.
> >>     
> >
> > Sorry that I don't explain it clearly. The concept of "user interface"
> > in IPMI interface is only a channel that can be used to communicate with
> > the IPMI controller. It has no relationship with whether the IPMI
> > interface should be exposed to user space. If one driver wants to
> > communicate with one IPMI interface, we should create one "user
> > interface" firstly and send the corresponding IPMI message by using the
> > "user interface".
> >
> > If one IPMI interface(controller) is already detected by PCI mechanism,
> > then ACPI will fail to detect the same IPMI interface. In such case it
> > is difficult for ACPI to know which IPMI interface should be accessed
> > when the ACPI AML code need to communicate with the IPMI interface.
> >   
> This may all be true, but the IPMI specification clearly gives the 
> detection order.  It's not a good idea to deviate from that without a 
> very good reason.  I'm not sure this is a good enough reason.

Sorry that I don't pay attention to the discovery order. I relook at the
IPMI spec and find that I am wrong.

> 
> Also, the bulk of this code clearly has nothing to do with the system 
> interface itself and should be in its own file.  And is there any reason 
> to tie this to the SMI code, anyway?  There are plenty of systems with 
> IPMI and ACPI that use I2C or serial ports.  Can they not use this ACPI 
> function (even though their drivers are not in the mainstream kernel)?

My first thought is to put the ACPI-IPMI opregion handler into ACPI
subsystem. But Bjorn suggests that it can be put into the smi code. Then
I follow his suggestion.

OK. I will rewrite the code and don't tie the ACPI-IPMI opregion handler
to the smi code.
    
> 
> Can you look and see if there is some other way to detect the ACPI 
> function on a BMC with some other mechanism?  I think that would be 
> better all around.  If it's the same BMC, the I can't imagine it matters 
> if you send the messages via the PCI-detected or ACPI-detected mechanisms.

OK. I will think about it again and try to figure out one solution that
identify the same BMC via PCI-detected or ACPI-detected and avoids
adjusting the discovery order. 

Thanks for your idea.


> 
> -corey

--
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 -v6 2/2] IPMI: use ACPI detection mechanism firstly to detect IPMI system interface

[Corey Minyard]

ykzhao wrote:
> My first thought is to put the ACPI-IPMI opregion handler into ACPI
> subsystem. But Bjorn suggests that it can be put into the smi code. Then
> I follow his suggestion.
>
> OK. I will rewrite the code and don't tie the ACPI-IPMI opregion handler
> to the smi code.
>   
I think it's ok to be in the ipmi directory, just not directly with the 
SMI code.  I'd put it in its own file, I think.

>     
>   
>> Can you look and see if there is some other way to detect the ACPI 
>> function on a BMC with some other mechanism?  I think that would be 
>> better all around.  If it's the same BMC, the I can't imagine it matters 
>> if you send the messages via the PCI-detected or ACPI-detected mechanisms.
>>     
>
> OK. I will think about it again and try to figure out one solution that
> identify the same BMC via PCI-detected or ACPI-detected and avoids
> adjusting the discovery order. 
>   
Thanks.  If there's some unique message for this function that 
idempotent, you can simply send that message and see if you get an error 
response or not.  This is used for detection ATCA function in some 
cases, for instance.

-corey

------------------------------------------------------------------------------
ThinkGeek and WIRED's GeekDad team up for the Ultimate 
GeekDad Father's Day Giveaway. ONE MASSIVE PRIZE to the 
lucky parental unit.  See the prize list and enter to win: 
http://p.sf.net/sfu/thinkgeek-promo