[PATCH 1/2] IPMI/ACPI: Locate the IPMI system interface in ACPI namespace

[PATCH 1/2] IPMI/ACPI: Locate the IPMI system interface in ACPI namespace

[yakui.zhao]

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

According to the IPMI 2.0 spec the IPMI system interface can be located with
ACPI. One is located in SPMI table(Service Processor Management Interface
table). Another is located in ACPI namespace.
This patch is to locate the IPMI system interface in ACPI namespace and
register it.
It includes the following two steps:
   1. enumerate the ACPI device tree to find the IPMI system interface
	The IPMI device type is IPI0001. When the device is found, it
will continue to parse the corresponding resources.
        For example:
		interface type (KCS, BT, SMIC) (SSIF is not supported)
		interrupt number and type (_GPE or GSI)
		Memory or IO base address
    2. register the IPMI system interface.

Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
---
 drivers/char/ipmi/ipmi_si_intf.c |  394 ++++++++++++++++++++++++++++++++++++++
 1 files changed, 394 insertions(+), 0 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index d2e6980..94d14bc 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -1813,6 +1813,35 @@ static __devinit void hardcode_find_bmc(void)
  * are no more.
  */
 static int acpi_failure;
+static LIST_HEAD(acpi_ipmi);
+
+struct acpi_device_ipmi {
+	struct list_head link;
+	u8 interfacetype;
+	/*
+	 * Bit 0 - SCI interrupt supported
+	 * Bit 1 - I/O APIC/SAPIC
+	 */
+	u8	interrupttype;
+	/*
+	 * If bit 0 of InterruptType is set, then this is the SCI
+	 * interrupt in the GPEx_STS register.
+	 */
+	u8	gpe;
+	/*
+	 * If bit 1 of InterruptType is set, then this is the I/O
+	 * APIC/SAPIC interrupt.
+	 */
+	u32	global_interrupt;
+
+	/* The actual register address. */
+	struct acpi_generic_address addr;
+	struct acpi_generic_address sm_addr;
+
+	u16 ipmi_revision;
+	u8 resource_count;
+	struct device *dev;
+};
 
 /* For GPE-type interrupts. */
 static u32 ipmi_acpi_gpe(void *context)
@@ -2002,6 +2031,367 @@ static __devinit int try_init_acpi(struct SPMITable *spmi)
 	return 0;
 }
 
+static __devinit int try_init_acpi_device(struct acpi_device_ipmi *spmi)
+{
+	struct smi_info  *info;
+	u8 		 addr_space;
+
+	if (spmi->addr.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
+		addr_space = IPMI_MEM_ADDR_SPACE;
+	else
+		addr_space = IPMI_IO_ADDR_SPACE;
+
+	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!info) {
+		printk(KERN_ERR "ipmi_si: Could not allocate SI data (3)\n");
+		return -ENOMEM;
+	}
+
+	info->addr_source = "ACPI";
+
+	/* Figure out the interface type. */
+	switch (spmi->interfacetype) {
+	case 1:	/* KCS */
+		info->si_type = SI_KCS;
+		break;
+	case 2:	/* SMIC */
+		info->si_type = SI_SMIC;
+		break;
+	case 3:	/* BT */
+		info->si_type = SI_BT;
+		break;
+	default:
+		printk(KERN_INFO "ipmi_si: Unknown ACPI/SPMI SI type %d\n",
+			spmi->interfacetype);
+		kfree(info);
+		return -EIO;
+	}
+
+	if (spmi->interrupttype & 1) {
+		/* We've got a GPE interrupt. */
+		info->irq = spmi->gpe;
+		info->irq_setup = acpi_gpe_irq_setup;
+	} else if (spmi->interrupttype & 2) {
+		/* We've got an APIC/SAPIC interrupt. */
+		info->irq = spmi->global_interrupt;
+		info->irq_setup = std_irq_setup;
+	} else {
+		/* Use the default interrupt setting. */
+		info->irq = 0;
+		info->irq_setup = NULL;
+	}
+
+	if (spmi->addr.bit_width) {
+		/* A (hopefully) properly formed register bit width. */
+		info->io.regspacing = spmi->addr.bit_width / 8;
+	} else {
+		info->io.regspacing = DEFAULT_REGSPACING;
+	}
+	info->io.regsize = info->io.regspacing;
+	info->io.regshift = spmi->addr.bit_offset;
+
+	if (spmi->addr.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
+		info->io_setup = mem_setup;
+		info->io.addr_type = IPMI_MEM_ADDR_SPACE;
+	} else if (spmi->addr.space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
+		info->io_setup = port_setup;
+		info->io.addr_type = IPMI_IO_ADDR_SPACE;
+	} else {
+		kfree(info);
+		printk(KERN_WARNING
+		       "ipmi_si: Unknown ACPI I/O Address type\n");
+		return -EIO;
+	}
+	info->io.addr_data = spmi->addr.address;
+	info->dev = spmi->dev;
+
+	try_smi_init(info);
+
+	return 0;
+}
+
+static acpi_status
+acpi_parse_io_ports(struct acpi_resource *resource, void *context)
+{
+	struct acpi_device_ipmi *p_ipmi = context;
+
+	if (resource->type == ACPI_RESOURCE_TYPE_EXTENDED_IRQ ||
+		resource->type == ACPI_RESOURCE_TYPE_IRQ) {
+		unsigned int irq_number;
+		if (p_ipmi->interrupttype) {
+			/*
+			 * If it already support the interrupt through GPE,
+			 * it is unnecessary to get this interrupt again.
+			 */
+			printk(KERN_WARNING "Interrupt through GPE is already"
+				" supported.\n");
+			return AE_OK;
+		}
+		if (resource->type == ACPI_RESOURCE_TYPE_IRQ) {
+			struct acpi_resource_irq *irq;
+			irq = &resource->data.irq;
+			if (irq->interrupt_count != 1) {
+				printk(KERN_WARNING "incorrect IRQ is "
+					"defined in _CRS\n");
+				return AE_OK;
+			}
+			irq_number = irq->interrupts[0];
+		} else {
+			struct acpi_resource_extended_irq *extended_irq;
+			extended_irq = &resource->data.extended_irq;
+			if (extended_irq->interrupt_count != 1) {
+				printk(KERN_WARNING "incorrect IRQ is "
+					"defined in _CRS\n");
+				return AE_OK;
+			}
+			irq_number = extended_irq->interrupts[0];
+		}
+		p_ipmi->global_interrupt = irq_number;
+		if (p_ipmi->global_interrupt) {
+			/* GSI interrupt type */
+			p_ipmi->interrupttype |= 0x02;
+		}
+		return AE_OK;
+	}
+	if (resource->type == ACPI_RESOURCE_TYPE_IO ||
+		resource->type == ACPI_RESOURCE_TYPE_FIXED_IO) {
+		u16 address;
+		struct acpi_resource_io *io;
+		struct acpi_resource_fixed_io *fixed_io;
+
+		fixed_io = &resource->data.fixed_io;
+		if (p_ipmi->resource_count) {
+			/*
+			 * Multiply definitions of IO/memory address are
+			 * obtained. It is incorrect. We will continue
+			 * to use the first IO/memory definition.
+			 * If not correct, please fix me.
+			 */
+			return AE_OK;
+		}
+		if (resource->type == ACPI_RESOURCE_TYPE_IO) {
+			io = &resource->data.io;
+			if (!io->minimum) {
+				/* when IO address is zero, return */
+				return AE_OK;
+			}
+			address = io->minimum;
+		} else {
+			fixed_io = &resource->data.fixed_io;
+			if (!fixed_io->address)
+				return AE_OK;
+			address = fixed_io->address;
+		}
+		p_ipmi->resource_count++;
+		p_ipmi->addr.space_id = ACPI_ADR_SPACE_SYSTEM_IO;
+		p_ipmi->addr.address = address;
+		return AE_OK;
+	}
+
+	if (resource->type == ACPI_RESOURCE_TYPE_MEMORY32 ||
+		resource->type == ACPI_RESOURCE_TYPE_MEMORY24 ||
+		resource->type == ACPI_RESOURCE_TYPE_FIXED_MEMORY32) {
+		u32 address;
+		if (p_ipmi->resource_count) {
+			/*
+			 * Multiply definitions of IO/memory address are
+			 * obtained. It is incorrect. We will continue
+			 * to use the first IO/memory definition.
+			 * If not correct, please fix me.
+			 */
+			return AE_OK;
+		}
+		if (resource->type == ACPI_RESOURCE_TYPE_MEMORY32) {
+			struct acpi_resource_memory32 *memory32;
+			memory32 = &resource->data.memory32;
+			address = memory32->minimum;
+		} else if (resource->type == ACPI_RESOURCE_TYPE_MEMORY24) {
+			struct acpi_resource_memory24 *memory24;
+			memory24 = &resource->data.memory24;
+			address = memory24->minimum;
+		} else {
+			struct acpi_resource_fixed_memory32 *fixed_memory32;
+			fixed_memory32 = &resource->data.fixed_memory32;
+			address = fixed_memory32->address;
+		}
+		p_ipmi->resource_count++;
+		p_ipmi->addr.address = (u64) address;
+		p_ipmi->addr.space_id = ACPI_ADR_SPACE_SYSTEM_MEMORY;
+		return AE_OK;
+	}
+	if (resource->type == ACPI_RESOURCE_TYPE_ADDRESS16 ||
+		resource->type == ACPI_RESOURCE_TYPE_ADDRESS32 ||
+		resource->type == ACPI_RESOURCE_TYPE_ADDRESS64) {
+		struct acpi_resource_address64 address64;
+		acpi_resource_to_address64(resource, &address64);
+		if (p_ipmi->resource_count) {
+			/*
+			 * Multiply definitions of IO/memory address are
+			 * obtained. It is incorrect. We will continue
+			 * to use the first IO/memory definition.
+			 * If not correct, please fix me.
+			 */
+			return AE_OK;
+		}
+		if (address64.resource_type != ACPI_MEMORY_RANGE &&
+			address64.resource_type != ACPI_IO_RANGE) {
+			/* ignore the incorrect resource type */
+			return AE_OK;
+		}
+		p_ipmi->addr.address = address64.minimum;
+		p_ipmi->resource_count++;
+		if (address64.resource_type == ACPI_MEMORY_RANGE)
+			p_ipmi->addr.space_id = ACPI_ADR_SPACE_SYSTEM_MEMORY;
+		else
+			p_ipmi->addr.space_id = ACPI_ADR_SPACE_SYSTEM_IO;
+
+		return AE_OK;
+	}
+
+	return AE_OK;
+}
+
+/*
+ *  acpi_parse_bmc_resource -- parse the BMC resources from ACPI
+ *  @p_ipmi: the memory to store the BCM resource
+ *  @handle: ACPI device handle
+ */
+static int acpi_parse_bmc_resource(struct acpi_device_ipmi *p_ipmi,
+			acpi_handle handle)
+{
+	int parse_ok = false;
+	unsigned long long	temp_data;
+	acpi_status status;
+
+	/* According to IPMI spec there should exist the _IFT method
+	 * for the IPMI device. So when there is no _IFT, it is regarded
+	 * as the incorrect BMC device and won't parse the resource again.
+	 */
+	status = acpi_evaluate_integer(handle, "_IFT", NULL, &temp_data);
+	if (ACPI_FAILURE(status))
+		return parse_ok;
+
+	p_ipmi->interfacetype = temp_data;
+	/* Figure out the interface type. If the interface type is not
+	 * KCS/SMIC/BT, it is regared as the incorrect IPMI device.
+	 * Of course the SSIF interface type is also defined, but we
+	 * can't handle it. So it is not supported */
+	switch (temp_data) {
+	case 1:	/* KCS */
+	case 2:	/* SMIC */
+	case 3:	/* BT */
+		break;
+	default:
+		printk(KERN_INFO "ipmi_si: Unknown ACPI/SPMI SI type %d\n",
+			p_ipmi->interfacetype);
+		return parse_ok;
+	}
+	/* check whether there exists the _GPE method. If it exists, it
+	 * means that interrupt through GPE is supported.
+	 */
+	temp_data = 0;
+	status = acpi_evaluate_integer(handle, "_GPE", NULL, &temp_data);
+	if (ACPI_SUCCESS(status)) {
+		p_ipmi->gpe = temp_data;
+		/* set the GPE interrupt type */
+		p_ipmi->interrupttype |= 0x01;
+	}
+	/* get the IPMI revision */
+	temp_data = 0;
+	status = acpi_evaluate_integer(handle, "_SRV", NULL,  &temp_data);
+	if (ACPI_SUCCESS(status))
+		p_ipmi->ipmi_revision = temp_data;
+
+	status = acpi_walk_resources(handle, METHOD_NAME__CRS,
+				acpi_parse_io_ports, p_ipmi);
+	if (ACPI_FAILURE(status)) {
+		printk(KERN_WARNING "Can't parse the _CRS object \n");
+		return parse_ok;
+	}
+	if (!p_ipmi->resource_count) {
+		/* The incorrect IO/Memory address is parsed */
+		printk(KERN_ERR "Incorrect IO/Memory address is parsed\n");
+		return parse_ok;
+	}
+	parse_ok = true;
+
+	return parse_ok;
+}
+
+const struct acpi_device_id ipmi_ids[] = {
+	{"IPI0001", 0},
+	{"", 0},
+};
+
+/*
+ * acpi_check_bmc_device -- check whether @handle is a BMC device and then
+ *		get its corresponding resource. For example: IO/Mem
+ *		address, interface type
+ * @handle: ACPI device handle
+ * @level : depth in the ACPI namespace tree
+ * @context: the number of bmc device. In theory there is not more than
+ * 	one ACPI BMC device.
+ * @rv: a return value to fill if desired (Not use)
+ */
+static acpi_status
+acpi_check_bmc_device(acpi_handle handle, u32 level, void *context,
+			void **return_value)
+{
+	struct acpi_device *acpi_dev;
+	struct acpi_device_ipmi *p_ipmi = NULL;
+	int *count = (int *)context;
+
+	acpi_dev = NULL;
+	/* Get the acpi device for device handle */
+	if (acpi_bus_get_device(handle, &acpi_dev) || !acpi_dev) {
+		/* If there is no ACPI device for handle, return */
+		return AE_OK;
+	}
+
+	if (acpi_match_device_ids(acpi_dev, ipmi_ids))
+		return AE_OK;
+
+	p_ipmi = kzalloc(sizeof(*p_ipmi), GFP_KERNEL);
+	if (!p_ipmi) {
+		printk(KERN_ERR "Can't allocate memory for IPMI device\n");
+		return AE_OK;
+	}
+	p_ipmi->dev = &acpi_dev->dev;
+	if (!acpi_parse_bmc_resource(p_ipmi, handle)) {
+		kfree(p_ipmi);
+	} else {
+		list_add_tail(&p_ipmi->link, &acpi_ipmi);
+		*count = *count + 1;
+	}
+
+	return AE_OK;
+}
+
+static __devinit void acpi_device_find_bmc(void)
+{
+	acpi_status      status;
+	int              device_count = 0;
+	struct acpi_device_ipmi *p_ipmi, *p_ipmi2;
+
+	if (acpi_disabled)
+		return;
+
+	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
+				ACPI_UINT32_MAX,
+				acpi_check_bmc_device, &device_count, NULL);
+	if (!device_count) {
+		/* when no IPMI device is found in ACPI namespace, return */
+		return;
+	}
+	list_for_each_entry_safe(p_ipmi, p_ipmi2, &acpi_ipmi, link) {
+		try_init_acpi_device(p_ipmi);
+		list_del(&p_ipmi->link);
+		kfree(p_ipmi);
+	}
+
+	return;
+}
+
 static __devinit void acpi_find_bmc(void)
 {
 	acpi_status      status;
@@ -2014,6 +2404,7 @@ static __devinit void acpi_find_bmc(void)
 	if (acpi_failure)
 		return;
 
+	/* locate the IPMI system interface in ACPI SPMI table */
 	for (i = 0; ; i++) {
 		status = acpi_get_table(ACPI_SIG_SPMI, i+1,
 					(struct acpi_table_header **)&spmi);
@@ -2022,6 +2413,9 @@ static __devinit void acpi_find_bmc(void)
 
 		try_init_acpi(spmi);
 	}
+
+	/* locate the IPMI system interface in ACPI device */
+	acpi_device_find_bmc();
 }
 #endif
 
-- 
1.5.4.5

[PATCH 2/2] IPMI/ACPI: Add the IPMI opregion driver to send IPMI message in AML code

[yakui.zhao]

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

Add the IPMI opregion driver so that the AML code can communicate with BMC
throught IPMI message.
     It will create IPMI user interface for every IPMI device detected
in ACPI namespace and install the corresponding IPMI opregion space handler.

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>
---
 drivers/acpi/Kconfig  |   12 +
 drivers/acpi/Makefile |    1 +
 drivers/acpi/ipmi.c   |  583 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 596 insertions(+), 0 deletions(-)
 create mode 100644 drivers/acpi/ipmi.c

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 93d2c79..23cd08a 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -204,6 +204,18 @@ 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
+	default n
+	select IPMI_HANDLER
+	select IPMI_SI
+	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 7702118..61077b7 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -57,6 +57,7 @@ obj-$(CONFIG_ACPI_BATTERY)	+= battery.o
 obj-$(CONFIG_ACPI_SBS)		+= sbshc.o
 obj-$(CONFIG_ACPI_SBS)		+= sbs.o
 obj-$(CONFIG_ACPI_POWER_METER)	+= power_meter.o
+obj-$(CONFIG_ACPI_IPMI)		+= ipmi.o
 
 # processor has its own "processor." module_param namespace
 processor-y			:= processor_core.o processor_throttling.o
diff --git a/drivers/acpi/ipmi.c b/drivers/acpi/ipmi.c
new file mode 100644
index 0000000..015779c
--- /dev/null
+++ b/drivers/acpi/ipmi.c
@@ -0,0 +1,583 @@
+/*
+ *  ipmi.c - ACPI IPMI opregion
+ *
+ *  Copyright (C) 2009 Intel Corporation
+ *  Copyright (C) 2009 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>
+
+MODULE_AUTHOR("Zhao Yakui");
+MODULE_DESCRIPTION("ACPI IPMI Opregion driver");
+MODULE_LICENSE("GPL");
+
+#define ACPI_IPMI_CLASS			"IPMI"
+#define ACPI_IPMI_DEVICE_NAME		"IPMI_dev"
+#define IPMI_FLAGS_HANDLER_INSTALL	0
+
+#define ACPI_IPMI_OK			0
+#define ACPI_IPMI_TIMEOUT		0x10
+#define ACPI_IPMI_UNKNOWN		0x07
+/* the IPMI timeout is 30s */
+#define IPMI_TIMEOUT			(30 * HZ)
+
+
+struct acpi_ipmi_device {
+	acpi_handle handle;
+	struct acpi_device *device;
+	int if_type;
+	/* the device list attached to driver_data.ipmi_devices */
+	struct list_head head;
+	ipmi_user_t 	user_interface;
+	struct mutex	mutex_lock;
+	/* the IPMI request message list */
+	struct list_head tx_msg_list;
+	long curr_msgid;
+	/* IPMI flags */
+	unsigned long flags;
+};
+
+struct ipmi_driver_data {
+	int device_count;
+	struct list_head	ipmi_devices;
+	struct ipmi_smi_watcher	bmc_events;
+	struct ipmi_user_hndl	ipmi_hndlrs;
+};
+
+struct acpi_ipmi_msg {
+	/* message list */
+	struct list_head head;
+	/*
+	 * General speaking the addr type should be SI_ADDR_TYPE. And
+	 * the addr channel should be BMC.
+	 * In fact it can also be IPMB type. But we will have to
+	 * parse it from the Netfn command buffer. It is so complex
+	 * that it is skipped.
+	 */
+	struct ipmi_addr addr;
+	/* tx message id */
+	long tx_msgid;
+	/* it is used to track whether the IPMI message is finished */
+	struct completion tx_complete;
+	struct kernel_ipmi_msg tx_message;
+	int	msg_done;
+	/* tx data . And copy it from ACPI object buffer */
+	u8	tx_data[64];
+	int	tx_len;
+	/* get the response data */
+	u8	rx_data[64];
+	/* the response length. The netfn & cmd is excluded. */
+	int	rx_len;
+	struct acpi_ipmi_device *device;
+};
+
+/*
+ * IPMI request/response buffer.
+ * The length is 66 bytes.
+ */
+struct acpi_ipmi_buffer {
+	/* status code of a given IPMI command */
+	u8 status_code;
+	/* the length of the payload */
+	u8 length;
+	/*
+	 * the payload. Before the operatin is carried out, it represents the
+	 * request message payload. After the opration is carried out, it
+	 * stores the response message returned by IPMI command.
+	 */
+	u8 data[64];
+};
+
+static void ipmi_register_bmc(int iface, struct device *dev);
+static void ipmi_bmc_gone(int iface);
+static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data);
+
+static struct ipmi_driver_data driver_data = {
+	.ipmi_devices = LIST_HEAD_INIT(driver_data.ipmi_devices),
+	.bmc_events = {
+		.owner = THIS_MODULE,
+		.new_smi = ipmi_register_bmc,
+		.smi_gone = ipmi_bmc_gone,
+	},
+	.ipmi_hndlrs = {
+		.ipmi_recv_hndl = ipmi_msg_handler,
+	},
+};
+
+static
+struct acpi_ipmi_msg *acpi_alloc_ipmi_msg(struct acpi_ipmi_device *ipmi)
+{
+	struct acpi_ipmi_msg *ipmi_msg;
+
+	ipmi_msg = kzalloc(sizeof(struct acpi_ipmi_msg), GFP_KERNEL);
+	if (!ipmi_msg)	{
+		printk(KERN_DEBUG "Can't allocate memory for ipmi_msg\n");
+		return NULL;
+	}
+	init_completion(&ipmi_msg->tx_complete);
+	INIT_LIST_HEAD(&ipmi_msg->head);
+	ipmi_msg->device = ipmi;
+	return ipmi_msg;
+}
+
+static void acpi_format_ipmi_msg(struct acpi_ipmi_msg *tx_msg,
+		acpi_physical_address address,
+		acpi_integer *value)
+{
+	struct kernel_ipmi_msg *msg;
+	u8	temp_value;
+	struct acpi_ipmi_buffer *buffer;
+	struct acpi_ipmi_device *device;
+
+	msg = &tx_msg->tx_message;
+	/* get the netfn */
+	temp_value = (address >> 8) & 0xff;
+	msg->netfn = temp_value;
+	/* get the command */
+	temp_value = address & 0xff;
+	msg->cmd = temp_value;
+	msg->data = tx_msg->tx_data;
+	/*
+	 * value is the parameter passed by the IPMI opregion space handler.
+	 * It points to the IPMI request message buffer
+	 */
+	buffer = (struct acpi_ipmi_buffer *)value;
+	/* copy the tx message data */
+	msg->data_len = buffer->length;
+	memcpy(tx_msg->tx_data, buffer->data, msg->data_len);
+	/*
+	 * now the default type is SYSTEM_INTERFACE and channel type is BMC.
+	 * If the netfn is APP_REQUEST and the cmd is SEND_MESSAGE,
+	 * the addr type should be changed to IPMB.
+	 */
+	tx_msg->addr.addr_type = IPMI_SYSTEM_INTERFACE_ADDR_TYPE;
+	tx_msg->addr.channel = IPMI_BMC_CHANNEL;
+	tx_msg->addr.data[0] = 0;
+
+	/*
+	 * If the netfn is APP_REQUEST and the cmd is SEND_MESSAGE, we should
+	 * parse the IPMI request message buffer to get the IPMB address.
+	 * If so, please fix me.
+	 */
+
+	/* Get the msgid */
+	device = tx_msg->device;
+	mutex_lock(&device->mutex_lock);
+	device->curr_msgid++;
+	tx_msg->tx_msgid = device->curr_msgid;
+	mutex_unlock(&device->mutex_lock);
+}
+
+static void acpi_format_ipmi_response(struct acpi_ipmi_msg *msg,
+		acpi_integer *value, int timeout)
+{
+	struct acpi_ipmi_buffer *buffer;
+
+	/*
+	 * value is also used as output parameter. It represents the response
+	 * IPMI message returned by IPMI command.
+	 */
+	buffer = (struct acpi_ipmi_buffer *)value;
+	/* when timeout is zero, it means that the timeout happens */
+	if (!timeout) {
+		/* the status code is ACPI_IPMI_TIMEOUT */
+		buffer->status_code = ACPI_IPMI_TIMEOUT;
+		return;
+	}
+	/*
+	 * If the flag of msg_done is not set, it means that the IPMI command
+	 * is not executed correctly.
+	 * The status code will be ACPI_IPMI_UNKNOWN.
+	 */
+	if (!msg->msg_done) {
+		buffer->status_code = ACPI_IPMI_UNKNOWN;
+		return;
+	}
+	/*
+	 * If the IPMI response message is obtained correctly, the status code
+	 * will be ACPI_IPMI_OK
+	 */
+	buffer->status_code = ACPI_IPMI_OK;
+	buffer->length = msg->rx_len;
+	memcpy(buffer->data, msg->rx_data, msg->rx_len);
+	return;
+}
+static void ipmi_destroy_tx_msg(struct acpi_ipmi_device *ipmi)
+{
+	struct acpi_ipmi_msg *tx_msg = NULL;
+	int count = 20;
+
+	list_for_each_entry(tx_msg, &ipmi->tx_msg_list, head) {
+		/* wake up the sleep thread on the Tx msg */
+		complete(&tx_msg->tx_complete);
+	}
+	/* wait for about 20 ticks to flush the tx message list */
+	while (count--) {
+		if (list_empty(&ipmi->tx_msg_list))
+			break;
+		schedule_timeout(1);
+	}
+	if (!list_empty(&ipmi->tx_msg_list))
+		printk(KERN_DEBUG "tx msg list is not NULL\n");
+
+}
+
+static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data)
+{
+	struct acpi_ipmi_device *ipmi_device =
+			(struct acpi_ipmi_device *)user_msg_data;
+	int msg_found = 0;
+	struct acpi_ipmi_msg *tx_msg = NULL;
+
+	if (msg->user != ipmi_device->user_interface) {
+		printk(KERN_DEBUG "Incorrect IPMI user\n");
+		ipmi_free_recv_msg(msg);
+		return;
+	}
+	mutex_lock(&ipmi_device->mutex_lock);
+	list_for_each_entry(tx_msg, &ipmi_device->tx_msg_list, head) {
+		if (msg->msgid == tx_msg->tx_msgid) {
+			/* find the message id */
+			msg_found = 1;
+			break;
+		}
+	}
+
+	mutex_unlock(&ipmi_device->mutex_lock);
+	if (!msg_found) {
+		/* no matched msg is found . But we should free it */
+		ipmi_free_recv_msg(msg);
+		printk(KERN_DEBUG "Incorrect MSG is found \n");
+		return;
+	}
+
+	if (msg->msg.data_len > 1) {
+		/* copy the response data to Rx_data buffer */
+		memcpy(tx_msg->rx_data, msg->msg_data, msg->msg.data_len);
+		tx_msg->rx_len = msg->msg.data_len;
+		tx_msg->msg_done = 1;
+	}
+	complete(&tx_msg->tx_complete);
+	ipmi_free_recv_msg(msg);
+};
+
+static void ipmi_register_bmc(int iface, struct device *dev)
+{
+	struct acpi_ipmi_device *ipmi_device;
+	struct acpi_device *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) {
+		device = ipmi_device->device;
+		if (ipmi_device->user_interface) {
+			/*
+			 * Only one user interface is allowed to be registered
+			 * for one IPMI device.
+			 * If we already create the user interface for
+			 * one IPMI device, skip it
+			 */
+			continue;
+		}
+		if (dev == &device->dev) {
+			/*
+			 * If the dev is identical to the ACPI device,
+			 * create the user interface.
+			 */
+			err = ipmi_create_user(iface, &driver_data.ipmi_hndlrs,
+						ipmi_device, &user);
+			if (err == 0)
+				ipmi_device->user_interface = user;
+
+			continue;
+		}
+		/*
+		 * In fact maybe the IPMI interface can be registered by
+		 * other methods. For example: SPMI, DMI, PCI
+		 * So we should also create the user interface.
+		 */
+		err = ipmi_create_user(iface, &driver_data.ipmi_hndlrs,
+						ipmi_device, &user);
+		if (err == 0)
+			ipmi_device->user_interface = user;
+	}
+	return;
+}
+
+static void ipmi_bmc_gone(int iface)
+{
+	struct acpi_ipmi_device *ipmi_device;
+
+	if (list_empty(&driver_data.ipmi_devices))
+		return;
+
+	list_for_each_entry(ipmi_device, &driver_data.ipmi_devices, head) {
+		if (ipmi_device->user_interface) {
+			ipmi_destroy_user(ipmi_device->user_interface);
+			ipmi_device->user_interface = NULL;
+			/* we should also destory tx msg list */
+			ipmi_destroy_tx_msg(ipmi_device);
+		}
+	}
+}
+/* --------------------------------------------------------------------------
+ * 			Address Space Management
+   -------------------------------------------------------------------------- */
+/*
+ * This is the IPMI opregion space handler.
+ * @function: indicates the read/write. In fact as the IPMI message is driven
+ * by command, only write is meaningful.
+ * @address: This contains the netfn/command of IPMI request message.
+ * @bits   : not used.
+ * @value  : it is an in/out parameter. It points to the IPMI message buffer.
+ *	     Before the IPMI message is sent, it represents the actual request
+ *	     IPMI message. After the IPMI message is finished, it represents
+ *	     the response IPMI message returned by IPMI command.
+ * @handler_context: IPMI device context.
+ */
+
+static acpi_status
+acpi_ipmi_space_handler(u32 function, acpi_physical_address address,
+		      u32 bits, acpi_integer *value,
+		      void *handler_context, void *region_context)
+{
+	struct acpi_ipmi_msg *tx_msg = NULL;
+	struct acpi_ipmi_device *ipmi_device =
+			(struct acpi_ipmi_device *) handler_context;
+	int err;
+	acpi_status status;
+	/*
+	 * IPMI opregion message.
+	 * IPMI message is firstly written to the BMC and system software
+	 * can get the respsonse. So it is unmeaningful for the IPMI read
+	 * access.
+	 */
+	if ((function & ACPI_IO_MASK) == ACPI_READ) {
+		/* Read function is not supported. AE_TYPE is returned. */
+		return AE_TYPE;
+	}
+	if (!ipmi_device->user_interface) {
+		/* there doesn't exist user interface*/
+		return AE_NOT_EXIST;
+	}
+	tx_msg = acpi_alloc_ipmi_msg(ipmi_device);
+	if (!tx_msg) {
+		/* no memory is allocated */
+		return AE_NO_MEMORY;
+	}
+	acpi_format_ipmi_msg(tx_msg, address, value);
+	mutex_lock(&ipmi_device->mutex_lock);
+	list_add_tail(&tx_msg->head, &ipmi_device->tx_msg_list);
+	mutex_unlock(&ipmi_device->mutex_lock);
+	err = ipmi_request_settime(ipmi_device->user_interface,
+					&tx_msg->addr,
+					tx_msg->tx_msgid,
+					&tx_msg->tx_message,
+					NULL, 0, 0, 0);
+	if (err) {
+		status = AE_ERROR;
+		goto end_label;
+	}
+	err = wait_for_completion_timeout(&tx_msg->tx_complete, IPMI_TIMEOUT);
+
+end_label:
+	acpi_format_ipmi_response(tx_msg, value, err);
+	status = AE_OK;
+	mutex_lock(&ipmi_device->mutex_lock);
+	list_del(&tx_msg->head);
+	mutex_unlock(&ipmi_device->mutex_lock);
+	kfree(tx_msg);
+	return status;
+}
+
+static void ipmi_remove_handlers(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_handlers(struct acpi_ipmi_device *ipmi)
+{
+	acpi_status status;
+
+	if (test_bit(IPMI_FLAGS_HANDLER_INSTALL, &ipmi->flags))
+		return 0;
+
+	status = acpi_install_address_space_handler(ipmi->handle,
+						    ACPI_ADR_SPACE_IPMI,
+						    &acpi_ipmi_space_handler,
+						    NULL, ipmi);
+	if (ACPI_FAILURE(status)) {
+		printk(KERN_DEBUG "Can't register IPMI opregion %s\n",
+					acpi_device_bid(ipmi->device));
+		return -EINVAL;
+	}
+	set_bit(IPMI_FLAGS_HANDLER_INSTALL, &ipmi->flags);
+	return 0;
+}
+
+static int acpi_ipmi_add(struct acpi_device *device)
+{
+	struct acpi_ipmi_device *ipmi_device;
+	acpi_handle handle;
+	unsigned long long temp;
+	acpi_status status;
+	if (!device)
+		return -EINVAL;
+
+	handle = device->handle;
+	temp = 0;
+	status = acpi_evaluate_integer(handle, "_IFT", NULL, &temp);
+	if (ACPI_FAILURE(status)) {
+		printk(KERN_DEBUG "Incorrect _IFT object for %s\n",
+				acpi_device_bid(device));
+		return -ENODEV;
+	}
+	ipmi_device = kzalloc(sizeof(struct acpi_ipmi_device), GFP_KERNEL);
+	if (!ipmi_device) {
+		printk(KERN_DEBUG "Can't allocate memory space\n");
+		return -ENOMEM;
+	}
+	ipmi_device->if_type = temp;
+	switch (ipmi_device->if_type) {
+	case 1:
+	case 2:
+	case 3:
+		break;
+	default:
+		printk(KERN_DEBUG "Unknow IPMI:SI interface type %d\n",
+				ipmi_device->if_type);
+		kfree(ipmi_device);
+		return -EINVAL;
+	}
+	ipmi_device->handle = device->handle;
+	ipmi_device->device = device;
+	mutex_init(&ipmi_device->mutex_lock);
+	INIT_LIST_HEAD(&ipmi_device->head);
+	INIT_LIST_HEAD(&ipmi_device->tx_msg_list);
+
+	if (ipmi_install_handlers(ipmi_device)) {
+		/* can't register the IPMI opregion */
+		kfree(ipmi_device);
+		return -EINVAL;
+	}
+
+	/* add it to the IPMI device list */
+	list_add_tail(&ipmi_device->head, &driver_data.ipmi_devices);
+	device->driver_data = ipmi_device;
+	return 0;
+}
+
+static int acpi_ipmi_remove(struct acpi_device *device, int type)
+{
+	struct acpi_ipmi_device *ipmi_device;
+
+	ipmi_device = acpi_driver_data(device);
+	if (!ipmi_device)
+		return 0;
+
+	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_handlers(ipmi_device);
+	kfree(ipmi_device);
+	device->driver_data = NULL;
+	return 0;
+}
+
+static const struct acpi_device_id ipmi_device_ids[] = {
+	{"IPI0001", 0},
+	{"", 0},
+};
+
+static struct acpi_driver acpi_ipmi_driver = {
+	.name = "ipmi",
+	.class = ACPI_IPMI_CLASS,
+	.ids = ipmi_device_ids,
+	.ops = {
+		.add = acpi_ipmi_add,
+		.remove = acpi_ipmi_remove,
+		},
+};
+
+static int __init acpi_ipmi_init(void)
+{
+	int result = 0;
+
+	if (acpi_disabled)
+		return result;
+
+	result = acpi_bus_register_driver(&acpi_ipmi_driver);
+
+	if (result)
+		return result;
+
+	result = ipmi_smi_watcher_register(&driver_data.bmc_events);
+
+	if (result)
+		acpi_bus_unregister_driver(&acpi_ipmi_driver);
+
+	return result;
+}
+
+static void __exit acpi_ipmi_exit(void)
+{
+	if (acpi_disabled)
+		return;
+
+	ipmi_smi_watcher_unregister(&driver_data.bmc_events);
+	acpi_bus_unregister_driver(&acpi_ipmi_driver);
+
+	return;
+}
+
+module_init(acpi_ipmi_init);
+module_exit(acpi_ipmi_exit);
-- 
1.5.4.5

Re: [PATCH 1/2] IPMI/ACPI: Locate the IPMI system interface in ACPI namespace

[Bjorn Helgaas]

On Monday 26 October 2009 07:33:45 am yakui.zhao@intel.com wrote:
> From: Zhao Yakui <yakui.zhao@intel.com>
> 
> According to the IPMI 2.0 spec the IPMI system interface can be located with
> ACPI. One is located in SPMI table(Service Processor Management Interface
> table). Another is located in ACPI namespace.
> This patch is to locate the IPMI system interface in ACPI namespace and
> register it.
> It includes the following two steps:
>    1. enumerate the ACPI device tree to find the IPMI system interface
> 	The IPMI device type is IPI0001. When the device is found, it
> will continue to parse the corresponding resources.
>         For example:
> 		interface type (KCS, BT, SMIC) (SSIF is not supported)
> 		interrupt number and type (_GPE or GSI)
> 		Memory or IO base address
>     2. register the IPMI system interface.

As far as I can tell, these patches are unchanged from the original
ones you posted here:
  http://marc.info/?l=linux-acpi&m=125420227326811&w=2
  http://marc.info/?l=linux-acpi&m=125420227426814&w=2

Either you mistakenly posted the original patches again, or you
didn't address a single piece of the feedback we gave you.

Please don't waste my time asking for comments if you're just going
to ignore them.

It's OK if you think my suggestions are incorrect, but I expect you
to at least acknowledge them and explain why you disagree with me, so
we can have a discussion about it.

Bjorn

> Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
> ---
>  drivers/char/ipmi/ipmi_si_intf.c |  394 ++++++++++++++++++++++++++++++++++++++
>  1 files changed, 394 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> index d2e6980..94d14bc 100644
> --- a/drivers/char/ipmi/ipmi_si_intf.c
> +++ b/drivers/char/ipmi/ipmi_si_intf.c
> @@ -1813,6 +1813,35 @@ static __devinit void hardcode_find_bmc(void)
>   * are no more.
>   */
>  static int acpi_failure;
> +static LIST_HEAD(acpi_ipmi);
> +
> +struct acpi_device_ipmi {
> +	struct list_head link;
> +	u8 interfacetype;
> +	/*
> +	 * Bit 0 - SCI interrupt supported
> +	 * Bit 1 - I/O APIC/SAPIC
> +	 */
> +	u8	interrupttype;
> +	/*
> +	 * If bit 0 of InterruptType is set, then this is the SCI
> +	 * interrupt in the GPEx_STS register.
> +	 */
> +	u8	gpe;
> +	/*
> +	 * If bit 1 of InterruptType is set, then this is the I/O
> +	 * APIC/SAPIC interrupt.
> +	 */
> +	u32	global_interrupt;
> +
> +	/* The actual register address. */
> +	struct acpi_generic_address addr;
> +	struct acpi_generic_address sm_addr;
> +
> +	u16 ipmi_revision;
> +	u8 resource_count;
> +	struct device *dev;
> +};
>  
>  /* For GPE-type interrupts. */
>  static u32 ipmi_acpi_gpe(void *context)
> @@ -2002,6 +2031,367 @@ static __devinit int try_init_acpi(struct SPMITable *spmi)
>  	return 0;
>  }
>  
> +static __devinit int try_init_acpi_device(struct acpi_device_ipmi *spmi)
> +{
> +	struct smi_info  *info;
> +	u8 		 addr_space;
> +
> +	if (spmi->addr.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
> +		addr_space = IPMI_MEM_ADDR_SPACE;
> +	else
> +		addr_space = IPMI_IO_ADDR_SPACE;
> +
> +	info = kzalloc(sizeof(*info), GFP_KERNEL);
> +	if (!info) {
> +		printk(KERN_ERR "ipmi_si: Could not allocate SI data (3)\n");
> +		return -ENOMEM;
> +	}
> +
> +	info->addr_source = "ACPI";
> +
> +	/* Figure out the interface type. */
> +	switch (spmi->interfacetype) {
> +	case 1:	/* KCS */
> +		info->si_type = SI_KCS;
> +		break;
> +	case 2:	/* SMIC */
> +		info->si_type = SI_SMIC;
> +		break;
> +	case 3:	/* BT */
> +		info->si_type = SI_BT;
> +		break;
> +	default:
> +		printk(KERN_INFO "ipmi_si: Unknown ACPI/SPMI SI type %d\n",
> +			spmi->interfacetype);
> +		kfree(info);
> +		return -EIO;
> +	}
> +
> +	if (spmi->interrupttype & 1) {
> +		/* We've got a GPE interrupt. */
> +		info->irq = spmi->gpe;
> +		info->irq_setup = acpi_gpe_irq_setup;
> +	} else if (spmi->interrupttype & 2) {
> +		/* We've got an APIC/SAPIC interrupt. */
> +		info->irq = spmi->global_interrupt;
> +		info->irq_setup = std_irq_setup;
> +	} else {
> +		/* Use the default interrupt setting. */
> +		info->irq = 0;
> +		info->irq_setup = NULL;
> +	}
> +
> +	if (spmi->addr.bit_width) {
> +		/* A (hopefully) properly formed register bit width. */
> +		info->io.regspacing = spmi->addr.bit_width / 8;
> +	} else {
> +		info->io.regspacing = DEFAULT_REGSPACING;
> +	}
> +	info->io.regsize = info->io.regspacing;
> +	info->io.regshift = spmi->addr.bit_offset;
> +
> +	if (spmi->addr.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
> +		info->io_setup = mem_setup;
> +		info->io.addr_type = IPMI_MEM_ADDR_SPACE;
> +	} else if (spmi->addr.space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
> +		info->io_setup = port_setup;
> +		info->io.addr_type = IPMI_IO_ADDR_SPACE;
> +	} else {
> +		kfree(info);
> +		printk(KERN_WARNING
> +		       "ipmi_si: Unknown ACPI I/O Address type\n");
> +		return -EIO;
> +	}
> +	info->io.addr_data = spmi->addr.address;
> +	info->dev = spmi->dev;
> +
> +	try_smi_init(info);
> +
> +	return 0;
> +}
> +
> +static acpi_status
> +acpi_parse_io_ports(struct acpi_resource *resource, void *context)
> +{
> +	struct acpi_device_ipmi *p_ipmi = context;
> +
> +	if (resource->type == ACPI_RESOURCE_TYPE_EXTENDED_IRQ ||
> +		resource->type == ACPI_RESOURCE_TYPE_IRQ) {
> +		unsigned int irq_number;
> +		if (p_ipmi->interrupttype) {
> +			/*
> +			 * If it already support the interrupt through GPE,
> +			 * it is unnecessary to get this interrupt again.
> +			 */
> +			printk(KERN_WARNING "Interrupt through GPE is already"
> +				" supported.\n");
> +			return AE_OK;
> +		}
> +		if (resource->type == ACPI_RESOURCE_TYPE_IRQ) {
> +			struct acpi_resource_irq *irq;
> +			irq = &resource->data.irq;
> +			if (irq->interrupt_count != 1) {
> +				printk(KERN_WARNING "incorrect IRQ is "
> +					"defined in _CRS\n");
> +				return AE_OK;
> +			}
> +			irq_number = irq->interrupts[0];
> +		} else {
> +			struct acpi_resource_extended_irq *extended_irq;
> +			extended_irq = &resource->data.extended_irq;
> +			if (extended_irq->interrupt_count != 1) {
> +				printk(KERN_WARNING "incorrect IRQ is "
> +					"defined in _CRS\n");
> +				return AE_OK;
> +			}
> +			irq_number = extended_irq->interrupts[0];
> +		}
> +		p_ipmi->global_interrupt = irq_number;
> +		if (p_ipmi->global_interrupt) {
> +			/* GSI interrupt type */
> +			p_ipmi->interrupttype |= 0x02;
> +		}
> +		return AE_OK;
> +	}
> +	if (resource->type == ACPI_RESOURCE_TYPE_IO ||
> +		resource->type == ACPI_RESOURCE_TYPE_FIXED_IO) {
> +		u16 address;
> +		struct acpi_resource_io *io;
> +		struct acpi_resource_fixed_io *fixed_io;
> +
> +		fixed_io = &resource->data.fixed_io;
> +		if (p_ipmi->resource_count) {
> +			/*
> +			 * Multiply definitions of IO/memory address are
> +			 * obtained. It is incorrect. We will continue
> +			 * to use the first IO/memory definition.
> +			 * If not correct, please fix me.
> +			 */
> +			return AE_OK;
> +		}
> +		if (resource->type == ACPI_RESOURCE_TYPE_IO) {
> +			io = &resource->data.io;
> +			if (!io->minimum) {
> +				/* when IO address is zero, return */
> +				return AE_OK;
> +			}
> +			address = io->minimum;
> +		} else {
> +			fixed_io = &resource->data.fixed_io;
> +			if (!fixed_io->address)
> +				return AE_OK;
> +			address = fixed_io->address;
> +		}
> +		p_ipmi->resource_count++;
> +		p_ipmi->addr.space_id = ACPI_ADR_SPACE_SYSTEM_IO;
> +		p_ipmi->addr.address = address;
> +		return AE_OK;
> +	}
> +
> +	if (resource->type == ACPI_RESOURCE_TYPE_MEMORY32 ||
> +		resource->type == ACPI_RESOURCE_TYPE_MEMORY24 ||
> +		resource->type == ACPI_RESOURCE_TYPE_FIXED_MEMORY32) {
> +		u32 address;
> +		if (p_ipmi->resource_count) {
> +			/*
> +			 * Multiply definitions of IO/memory address are
> +			 * obtained. It is incorrect. We will continue
> +			 * to use the first IO/memory definition.
> +			 * If not correct, please fix me.
> +			 */
> +			return AE_OK;
> +		}
> +		if (resource->type == ACPI_RESOURCE_TYPE_MEMORY32) {
> +			struct acpi_resource_memory32 *memory32;
> +			memory32 = &resource->data.memory32;
> +			address = memory32->minimum;
> +		} else if (resource->type == ACPI_RESOURCE_TYPE_MEMORY24) {
> +			struct acpi_resource_memory24 *memory24;
> +			memory24 = &resource->data.memory24;
> +			address = memory24->minimum;
> +		} else {
> +			struct acpi_resource_fixed_memory32 *fixed_memory32;
> +			fixed_memory32 = &resource->data.fixed_memory32;
> +			address = fixed_memory32->address;
> +		}
> +		p_ipmi->resource_count++;
> +		p_ipmi->addr.address = (u64) address;
> +		p_ipmi->addr.space_id = ACPI_ADR_SPACE_SYSTEM_MEMORY;
> +		return AE_OK;
> +	}
> +	if (resource->type == ACPI_RESOURCE_TYPE_ADDRESS16 ||
> +		resource->type == ACPI_RESOURCE_TYPE_ADDRESS32 ||
> +		resource->type == ACPI_RESOURCE_TYPE_ADDRESS64) {
> +		struct acpi_resource_address64 address64;
> +		acpi_resource_to_address64(resource, &address64);
> +		if (p_ipmi->resource_count) {
> +			/*
> +			 * Multiply definitions of IO/memory address are
> +			 * obtained. It is incorrect. We will continue
> +			 * to use the first IO/memory definition.
> +			 * If not correct, please fix me.
> +			 */
> +			return AE_OK;
> +		}
> +		if (address64.resource_type != ACPI_MEMORY_RANGE &&
> +			address64.resource_type != ACPI_IO_RANGE) {
> +			/* ignore the incorrect resource type */
> +			return AE_OK;
> +		}
> +		p_ipmi->addr.address = address64.minimum;
> +		p_ipmi->resource_count++;
> +		if (address64.resource_type == ACPI_MEMORY_RANGE)
> +			p_ipmi->addr.space_id = ACPI_ADR_SPACE_SYSTEM_MEMORY;
> +		else
> +			p_ipmi->addr.space_id = ACPI_ADR_SPACE_SYSTEM_IO;
> +
> +		return AE_OK;
> +	}
> +
> +	return AE_OK;
> +}
> +
> +/*
> + *  acpi_parse_bmc_resource -- parse the BMC resources from ACPI
> + *  @p_ipmi: the memory to store the BCM resource
> + *  @handle: ACPI device handle
> + */
> +static int acpi_parse_bmc_resource(struct acpi_device_ipmi *p_ipmi,
> +			acpi_handle handle)
> +{
> +	int parse_ok = false;
> +	unsigned long long	temp_data;
> +	acpi_status status;
> +
> +	/* According to IPMI spec there should exist the _IFT method
> +	 * for the IPMI device. So when there is no _IFT, it is regarded
> +	 * as the incorrect BMC device and won't parse the resource again.
> +	 */
> +	status = acpi_evaluate_integer(handle, "_IFT", NULL, &temp_data);
> +	if (ACPI_FAILURE(status))
> +		return parse_ok;
> +
> +	p_ipmi->interfacetype = temp_data;
> +	/* Figure out the interface type. If the interface type is not
> +	 * KCS/SMIC/BT, it is regared as the incorrect IPMI device.
> +	 * Of course the SSIF interface type is also defined, but we
> +	 * can't handle it. So it is not supported */
> +	switch (temp_data) {
> +	case 1:	/* KCS */
> +	case 2:	/* SMIC */
> +	case 3:	/* BT */
> +		break;
> +	default:
> +		printk(KERN_INFO "ipmi_si: Unknown ACPI/SPMI SI type %d\n",
> +			p_ipmi->interfacetype);
> +		return parse_ok;
> +	}
> +	/* check whether there exists the _GPE method. If it exists, it
> +	 * means that interrupt through GPE is supported.
> +	 */
> +	temp_data = 0;
> +	status = acpi_evaluate_integer(handle, "_GPE", NULL, &temp_data);
> +	if (ACPI_SUCCESS(status)) {
> +		p_ipmi->gpe = temp_data;
> +		/* set the GPE interrupt type */
> +		p_ipmi->interrupttype |= 0x01;
> +	}
> +	/* get the IPMI revision */
> +	temp_data = 0;
> +	status = acpi_evaluate_integer(handle, "_SRV", NULL,  &temp_data);
> +	if (ACPI_SUCCESS(status))
> +		p_ipmi->ipmi_revision = temp_data;
> +
> +	status = acpi_walk_resources(handle, METHOD_NAME__CRS,
> +				acpi_parse_io_ports, p_ipmi);
> +	if (ACPI_FAILURE(status)) {
> +		printk(KERN_WARNING "Can't parse the _CRS object \n");
> +		return parse_ok;
> +	}
> +	if (!p_ipmi->resource_count) {
> +		/* The incorrect IO/Memory address is parsed */
> +		printk(KERN_ERR "Incorrect IO/Memory address is parsed\n");
> +		return parse_ok;
> +	}
> +	parse_ok = true;
> +
> +	return parse_ok;
> +}
> +
> +const struct acpi_device_id ipmi_ids[] = {
> +	{"IPI0001", 0},
> +	{"", 0},
> +};
> +
> +/*
> + * acpi_check_bmc_device -- check whether @handle is a BMC device and then
> + *		get its corresponding resource. For example: IO/Mem
> + *		address, interface type
> + * @handle: ACPI device handle
> + * @level : depth in the ACPI namespace tree
> + * @context: the number of bmc device. In theory there is not more than
> + * 	one ACPI BMC device.
> + * @rv: a return value to fill if desired (Not use)
> + */
> +static acpi_status
> +acpi_check_bmc_device(acpi_handle handle, u32 level, void *context,
> +			void **return_value)
> +{
> +	struct acpi_device *acpi_dev;
> +	struct acpi_device_ipmi *p_ipmi = NULL;
> +	int *count = (int *)context;
> +
> +	acpi_dev = NULL;
> +	/* Get the acpi device for device handle */
> +	if (acpi_bus_get_device(handle, &acpi_dev) || !acpi_dev) {
> +		/* If there is no ACPI device for handle, return */
> +		return AE_OK;
> +	}
> +
> +	if (acpi_match_device_ids(acpi_dev, ipmi_ids))
> +		return AE_OK;
> +
> +	p_ipmi = kzalloc(sizeof(*p_ipmi), GFP_KERNEL);
> +	if (!p_ipmi) {
> +		printk(KERN_ERR "Can't allocate memory for IPMI device\n");
> +		return AE_OK;
> +	}
> +	p_ipmi->dev = &acpi_dev->dev;
> +	if (!acpi_parse_bmc_resource(p_ipmi, handle)) {
> +		kfree(p_ipmi);
> +	} else {
> +		list_add_tail(&p_ipmi->link, &acpi_ipmi);
> +		*count = *count + 1;
> +	}
> +
> +	return AE_OK;
> +}
> +
> +static __devinit void acpi_device_find_bmc(void)
> +{
> +	acpi_status      status;
> +	int              device_count = 0;
> +	struct acpi_device_ipmi *p_ipmi, *p_ipmi2;
> +
> +	if (acpi_disabled)
> +		return;
> +
> +	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
> +				ACPI_UINT32_MAX,
> +				acpi_check_bmc_device, &device_count, NULL);
> +	if (!device_count) {
> +		/* when no IPMI device is found in ACPI namespace, return */
> +		return;
> +	}
> +	list_for_each_entry_safe(p_ipmi, p_ipmi2, &acpi_ipmi, link) {
> +		try_init_acpi_device(p_ipmi);
> +		list_del(&p_ipmi->link);
> +		kfree(p_ipmi);
> +	}
> +
> +	return;
> +}
> +
>  static __devinit void acpi_find_bmc(void)
>  {
>  	acpi_status      status;
> @@ -2014,6 +2404,7 @@ static __devinit void acpi_find_bmc(void)
>  	if (acpi_failure)
>  		return;
>  
> +	/* locate the IPMI system interface in ACPI SPMI table */
>  	for (i = 0; ; i++) {
>  		status = acpi_get_table(ACPI_SIG_SPMI, i+1,
>  					(struct acpi_table_header **)&spmi);
> @@ -2022,6 +2413,9 @@ static __devinit void acpi_find_bmc(void)
>  
>  		try_init_acpi(spmi);
>  	}
> +
> +	/* locate the IPMI system interface in ACPI device */
> +	acpi_device_find_bmc();
>  }
>  #endif
>  

Re: [PATCH 1/2] IPMI/ACPI: Locate the IPMI system interface in ACPI namespace

[ykzhao]

On Mon, 2009-10-26 at 23:20 +0800, Bjorn Helgaas wrote:
> On Monday 26 October 2009 07:33:45 am yakui.zhao@intel.com wrote:
> > From: Zhao Yakui <yakui.zhao@intel.com>
> >
> > According to the IPMI 2.0 spec the IPMI system interface can be located with
> > ACPI. One is located in SPMI table(Service Processor Management Interface
> > table). Another is located in ACPI namespace.
> > This patch is to locate the IPMI system interface in ACPI namespace and
> > register it.
> > It includes the following two steps:
> >    1. enumerate the ACPI device tree to find the IPMI system interface
> >       The IPMI device type is IPI0001. When the device is found, it
> > will continue to parse the corresponding resources.
> >         For example:
> >               interface type (KCS, BT, SMIC) (SSIF is not supported)
> >               interrupt number and type (_GPE or GSI)
> >               Memory or IO base address
> >     2. register the IPMI system interface

> As far as I can tell, these patches are unchanged from the original
> ones you posted here:
>   http://marc.info/?l=linux-acpi&m=125420227326811&w=2
>   http://marc.info/?l=linux-acpi&m=125420227426814&w=2
I refresh them based on the latest upstream branch. Of course I do the
small change about the second patch. It is mainly about the coding
style.

In fact you mention two issues about the two patches:
   1: Load a PNP driver for it to register the IPMI system interface.
This is about the first patch.
   2. coding style( for example: comments, the definition about some
variables).

For the first issue: Before I start the first patch, I consider using
the PNP device driver. But I find that it is so complex because of the
following two points:
    1. One is that we can't register the IPMI system interface if the
boot option of "pnpacpi=off" is added. This will also depend on the PNP
module.
    2. The second is that there exist so many cases about the IPMI
IO/memory resource definition. Maybe there exist both IO/memory resource
definition for one IPMI device. In such case we can't know which should
be selected. At the same time we have similar issues about the interrupt
type. So I decide to parse the IO/memory/interrupt resource
independently.
Very sorry that I don't reply the above points in the discussion.

For the second patch: 
   I update the coding style about the comments. But I don't change the
coding style about the variable definition as it doesn't matter. At the
same time it is easy to understand by using the explicit definition.
   For example: 
      if (acpi_disabled)
               return;

thanks.
   yakui

> Either you mistakenly posted the original patches again, or you
> didn't address a single piece of the feedback we gave you.
> 
> Please don't waste my time asking for comments if you're just going
> to ignore them.
> 
> It's OK if you think my suggestions are incorrect, but I expect you
> to at least acknowledge them and explain why you disagree with me, so
> we can have a discussion about it.
> 
> Bjorn
> 
> > Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
> > ---
> >  drivers/char/ipmi/ipmi_si_intf.c |  394 ++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 394 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> > index d2e6980..94d14bc 100644
> > --- a/drivers/char/ipmi/ipmi_si_intf.c
> > +++ b/drivers/char/ipmi/ipmi_si_intf.c
> > @@ -1813,6 +1813,35 @@ static __devinit void hardcode_find_bmc(void)
> >   * are no more.
> >   */
> >  static int acpi_failure;
> > +static LIST_HEAD(acpi_ipmi);
> > +
> > +struct acpi_device_ipmi {
> > +     struct list_head link;
> > +     u8 interfacetype;
> > +     /*
> > +      * Bit 0 - SCI interrupt supported
> > +      * Bit 1 - I/O APIC/SAPIC
> > +      */
> > +     u8      interrupttype;
> > +     /*
> > +      * If bit 0 of InterruptType is set, then this is the SCI
> > +      * interrupt in the GPEx_STS register.
> > +      */
> > +     u8      gpe;
> > +     /*
> > +      * If bit 1 of InterruptType is set, then this is the I/O
> > +      * APIC/SAPIC interrupt.
> > +      */
> > +     u32     global_interrupt;
> > +
> > +     /* The actual register address. */
> > +     struct acpi_generic_address addr;
> > +     struct acpi_generic_address sm_addr;
> > +
> > +     u16 ipmi_revision;
> > +     u8 resource_count;
> > +     struct device *dev;
> > +};
> >
> >  /* For GPE-type interrupts. */
> >  static u32 ipmi_acpi_gpe(void *context)
> > @@ -2002,6 +2031,367 @@ static __devinit int try_init_acpi(struct SPMITable *spmi)
> >       return 0;
> >  }
> >
> > +static __devinit int try_init_acpi_device(struct acpi_device_ipmi *spmi)
> > +{
> > +     struct smi_info  *info;
> > +     u8               addr_space;
> > +
> > +     if (spmi->addr.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
> > +             addr_space = IPMI_MEM_ADDR_SPACE;
> > +     else
> > +             addr_space = IPMI_IO_ADDR_SPACE;
> > +
> > +     info = kzalloc(sizeof(*info), GFP_KERNEL);
> > +     if (!info) {
> > +             printk(KERN_ERR "ipmi_si: Could not allocate SI data (3)\n");
> > +             return -ENOMEM;
> > +     }
> > +
> > +     info->addr_source = "ACPI";
> > +
> > +     /* Figure out the interface type. */
> > +     switch (spmi->interfacetype) {
> > +     case 1: /* KCS */
> > +             info->si_type = SI_KCS;
> > +             break;
> > +     case 2: /* SMIC */
> > +             info->si_type = SI_SMIC;
> > +             break;
> > +     case 3: /* BT */
> > +             info->si_type = SI_BT;
> > +             break;
> > +     default:
> > +             printk(KERN_INFO "ipmi_si: Unknown ACPI/SPMI SI type %d\n",
> > +                     spmi->interfacetype);
> > +             kfree(info);
> > +             return -EIO;
> > +     }
> > +
> > +     if (spmi->interrupttype & 1) {
> > +             /* We've got a GPE interrupt. */
> > +             info->irq = spmi->gpe;
> > +             info->irq_setup = acpi_gpe_irq_setup;
> > +     } else if (spmi->interrupttype & 2) {
> > +             /* We've got an APIC/SAPIC interrupt. */
> > +             info->irq = spmi->global_interrupt;
> > +             info->irq_setup = std_irq_setup;
> > +     } else {
> > +             /* Use the default interrupt setting. */
> > +             info->irq = 0;
> > +             info->irq_setup = NULL;
> > +     }
> > +
> > +     if (spmi->addr.bit_width) {
> > +             /* A (hopefully) properly formed register bit width. */
> > +             info->io.regspacing = spmi->addr.bit_width / 8;
> > +     } else {
> > +             info->io.regspacing = DEFAULT_REGSPACING;
> > +     }
> > +     info->io.regsize = info->io.regspacing;
> > +     info->io.regshift = spmi->addr.bit_offset;
> > +
> > +     if (spmi->addr.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
> > +             info->io_setup = mem_setup;
> > +             info->io.addr_type = IPMI_MEM_ADDR_SPACE;
> > +     } else if (spmi->addr.space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
> > +             info->io_setup = port_setup;
> > +             info->io.addr_type = IPMI_IO_ADDR_SPACE;
> > +     } else {
> > +             kfree(info);
> > +             printk(KERN_WARNING
> > +                    "ipmi_si: Unknown ACPI I/O Address type\n");
> > +             return -EIO;
> > +     }
> > +     info->io.addr_data = spmi->addr.address;
> > +     info->dev = spmi->dev;
> > +
> > +     try_smi_init(info);
> > +
> > +     return 0;
> > +}
> > +
> > +static acpi_status
> > +acpi_parse_io_ports(struct acpi_resource *resource, void *context)
> > +{
> > +     struct acpi_device_ipmi *p_ipmi = context;
> > +
> > +     if (resource->type == ACPI_RESOURCE_TYPE_EXTENDED_IRQ ||
> > +             resource->type == ACPI_RESOURCE_TYPE_IRQ) {
> > +             unsigned int irq_number;
> > +             if (p_ipmi->interrupttype) {
> > +                     /*
> > +                      * If it already support the interrupt through GPE,
> > +                      * it is unnecessary to get this interrupt again.
> > +                      */
> > +                     printk(KERN_WARNING "Interrupt through GPE is already"
> > +                             " supported.\n");
> > +                     return AE_OK;
> > +             }
> > +             if (resource->type == ACPI_RESOURCE_TYPE_IRQ) {
> > +                     struct acpi_resource_irq *irq;
> > +                     irq = &resource->data.irq;
> > +                     if (irq->interrupt_count != 1) {
> > +                             printk(KERN_WARNING "incorrect IRQ is "
> > +                                     "defined in _CRS\n");
> > +                             return AE_OK;
> > +                     }
> > +                     irq_number = irq->interrupts[0];
> > +             } else {
> > +                     struct acpi_resource_extended_irq *extended_irq;
> > +                     extended_irq = &resource->data.extended_irq;
> > +                     if (extended_irq->interrupt_count != 1) {
> > +                             printk(KERN_WARNING "incorrect IRQ is "
> > +                                     "defined in _CRS\n");
> > +                             return AE_OK;
> > +                     }
> > +                     irq_number = extended_irq->interrupts[0];
> > +             }
> > +             p_ipmi->global_interrupt = irq_number;
> > +             if (p_ipmi->global_interrupt) {
> > +                     /* GSI interrupt type */
> > +                     p_ipmi->interrupttype |= 0x02;
> > +             }
> > +             return AE_OK;
> > +     }
> > +     if (resource->type == ACPI_RESOURCE_TYPE_IO ||
> > +             resource->type == ACPI_RESOURCE_TYPE_FIXED_IO) {
> > +             u16 address;
> > +             struct acpi_resource_io *io;
> > +             struct acpi_resource_fixed_io *fixed_io;
> > +
> > +             fixed_io = &resource->data.fixed_io;
> > +             if (p_ipmi->resource_count) {
> > +                     /*
> > +                      * Multiply definitions of IO/memory address are
> > +                      * obtained. It is incorrect. We will continue
> > +                      * to use the first IO/memory definition.
> > +                      * If not correct, please fix me.
> > +                      */
> > +                     return AE_OK;
> > +             }
> > +             if (resource->type == ACPI_RESOURCE_TYPE_IO) {
> > +                     io = &resource->data.io;
> > +                     if (!io->minimum) {
> > +                             /* when IO address is zero, return */
> > +                             return AE_OK;
> > +                     }
> > +                     address = io->minimum;
> > +             } else {
> > +                     fixed_io = &resource->data.fixed_io;
> > +                     if (!fixed_io->address)
> > +                             return AE_OK;
> > +                     address = fixed_io->address;
> > +             }
> > +             p_ipmi->resource_count++;
> > +             p_ipmi->addr.space_id = ACPI_ADR_SPACE_SYSTEM_IO;
> > +             p_ipmi->addr.address = address;
> > +             return AE_OK;
> > +     }
> > +
> > +     if (resource->type == ACPI_RESOURCE_TYPE_MEMORY32 ||
> > +             resource->type == ACPI_RESOURCE_TYPE_MEMORY24 ||
> > +             resource->type == ACPI_RESOURCE_TYPE_FIXED_MEMORY32) {
> > +             u32 address;
> > +             if (p_ipmi->resource_count) {
> > +                     /*
> > +                      * Multiply definitions of IO/memory address are
> > +                      * obtained. It is incorrect. We will continue
> > +                      * to use the first IO/memory definition.
> > +                      * If not correct, please fix me.
> > +                      */
> > +                     return AE_OK;
> > +             }
> > +             if (resource->type == ACPI_RESOURCE_TYPE_MEMORY32) {
> > +                     struct acpi_resource_memory32 *memory32;
> > +                     memory32 = &resource->data.memory32;
> > +                     address = memory32->minimum;
> > +             } else if (resource->type == ACPI_RESOURCE_TYPE_MEMORY24) {
> > +                     struct acpi_resource_memory24 *memory24;
> > +                     memory24 = &resource->data.memory24;
> > +                     address = memory24->minimum;
> > +             } else {
> > +                     struct acpi_resource_fixed_memory32 *fixed_memory32;
> > +                     fixed_memory32 = &resource->data.fixed_memory32;
> > +                     address = fixed_memory32->address;
> > +             }
> > +             p_ipmi->resource_count++;
> > +             p_ipmi->addr.address = (u64) address;
> > +             p_ipmi->addr.space_id = ACPI_ADR_SPACE_SYSTEM_MEMORY;
> > +             return AE_OK;
> > +     }
> > +     if (resource->type == ACPI_RESOURCE_TYPE_ADDRESS16 ||
> > +             resource->type == ACPI_RESOURCE_TYPE_ADDRESS32 ||
> > +             resource->type == ACPI_RESOURCE_TYPE_ADDRESS64) {
> > +             struct acpi_resource_address64 address64;
> > +             acpi_resource_to_address64(resource, &address64);
> > +             if (p_ipmi->resource_count) {
> > +                     /*
> > +                      * Multiply definitions of IO/memory address are
> > +                      * obtained. It is incorrect. We will continue
> > +                      * to use the first IO/memory definition.
> > +                      * If not correct, please fix me.
> > +                      */
> > +                     return AE_OK;
> > +             }
> > +             if (address64.resource_type != ACPI_MEMORY_RANGE &&
> > +                     address64.resource_type != ACPI_IO_RANGE) {
> > +                     /* ignore the incorrect resource type */
> > +                     return AE_OK;
> > +             }
> > +             p_ipmi->addr.address = address64.minimum;
> > +             p_ipmi->resource_count++;
> > +             if (address64.resource_type == ACPI_MEMORY_RANGE)
> > +                     p_ipmi->addr.space_id = ACPI_ADR_SPACE_SYSTEM_MEMORY;
> > +             else
> > +                     p_ipmi->addr.space_id = ACPI_ADR_SPACE_SYSTEM_IO;
> > +
> > +             return AE_OK;
> > +     }
> > +
> > +     return AE_OK;
> > +}
> > +
> > +/*
> > + *  acpi_parse_bmc_resource -- parse the BMC resources from ACPI
> > + *  @p_ipmi: the memory to store the BCM resource
> > + *  @handle: ACPI device handle
> > + */
> > +static int acpi_parse_bmc_resource(struct acpi_device_ipmi *p_ipmi,
> > +                     acpi_handle handle)
> > +{
> > +     int parse_ok = false;
> > +     unsigned long long      temp_data;
> > +     acpi_status status;
> > +
> > +     /* According to IPMI spec there should exist the _IFT method
> > +      * for the IPMI device. So when there is no _IFT, it is regarded
> > +      * as the incorrect BMC device and won't parse the resource again.
> > +      */
> > +     status = acpi_evaluate_integer(handle, "_IFT", NULL, &temp_data);
> > +     if (ACPI_FAILURE(status))
> > +             return parse_ok;
> > +
> > +     p_ipmi->interfacetype = temp_data;
> > +     /* Figure out the interface type. If the interface type is not
> > +      * KCS/SMIC/BT, it is regared as the incorrect IPMI device.
> > +      * Of course the SSIF interface type is also defined, but we
> > +      * can't handle it. So it is not supported */
> > +     switch (temp_data) {
> > +     case 1: /* KCS */
> > +     case 2: /* SMIC */
> > +     case 3: /* BT */
> > +             break;
> > +     default:
> > +             printk(KERN_INFO "ipmi_si: Unknown ACPI/SPMI SI type %d\n",
> > +                     p_ipmi->interfacetype);
> > +             return parse_ok;
> > +     }
> > +     /* check whether there exists the _GPE method. If it exists, it
> > +      * means that interrupt through GPE is supported.
> > +      */
> > +     temp_data = 0;
> > +     status = acpi_evaluate_integer(handle, "_GPE", NULL, &temp_data);
> > +     if (ACPI_SUCCESS(status)) {
> > +             p_ipmi->gpe = temp_data;
> > +             /* set the GPE interrupt type */
> > +             p_ipmi->interrupttype |= 0x01;
> > +     }
> > +     /* get the IPMI revision */
> > +     temp_data = 0;
> > +     status = acpi_evaluate_integer(handle, "_SRV", NULL,  &temp_data);
> > +     if (ACPI_SUCCESS(status))
> > +             p_ipmi->ipmi_revision = temp_data;
> > +
> > +     status = acpi_walk_resources(handle, METHOD_NAME__CRS,
> > +                             acpi_parse_io_ports, p_ipmi);
> > +     if (ACPI_FAILURE(status)) {
> > +             printk(KERN_WARNING "Can't parse the _CRS object \n");
> > +             return parse_ok;
> > +     }
> > +     if (!p_ipmi->resource_count) {
> > +             /* The incorrect IO/Memory address is parsed */
> > +             printk(KERN_ERR "Incorrect IO/Memory address is parsed\n");
> > +             return parse_ok;
> > +     }
> > +     parse_ok = true;
> > +
> > +     return parse_ok;
> > +}
> > +
> > +const struct acpi_device_id ipmi_ids[] = {
> > +     {"IPI0001", 0},
> > +     {"", 0},
> > +};
> > +
> > +/*
> > + * acpi_check_bmc_device -- check whether @handle is a BMC device and then
> > + *           get its corresponding resource. For example: IO/Mem
> > + *           address, interface type
> > + * @handle: ACPI device handle
> > + * @level : depth in the ACPI namespace tree
> > + * @context: the number of bmc device. In theory there is not more than
> > + *   one ACPI BMC device.
> > + * @rv: a return value to fill if desired (Not use)
> > + */
> > +static acpi_status
> > +acpi_check_bmc_device(acpi_handle handle, u32 level, void *context,
> > +                     void **return_value)
> > +{
> > +     struct acpi_device *acpi_dev;
> > +     struct acpi_device_ipmi *p_ipmi = NULL;
> > +     int *count = (int *)context;
> > +
> > +     acpi_dev = NULL;
> > +     /* Get the acpi device for device handle */
> > +     if (acpi_bus_get_device(handle, &acpi_dev) || !acpi_dev) {
> > +             /* If there is no ACPI device for handle, return */
> > +             return AE_OK;
> > +     }
> > +
> > +     if (acpi_match_device_ids(acpi_dev, ipmi_ids))
> > +             return AE_OK;
> > +
> > +     p_ipmi = kzalloc(sizeof(*p_ipmi), GFP_KERNEL);
> > +     if (!p_ipmi) {
> > +             printk(KERN_ERR "Can't allocate memory for IPMI device\n");
> > +             return AE_OK;
> > +     }
> > +     p_ipmi->dev = &acpi_dev->dev;
> > +     if (!acpi_parse_bmc_resource(p_ipmi, handle)) {
> > +             kfree(p_ipmi);
> > +     } else {
> > +             list_add_tail(&p_ipmi->link, &acpi_ipmi);
> > +             *count = *count + 1;
> > +     }
> > +
> > +     return AE_OK;
> > +}
> > +
> > +static __devinit void acpi_device_find_bmc(void)
> > +{
> > +     acpi_status      status;
> > +     int              device_count = 0;
> > +     struct acpi_device_ipmi *p_ipmi, *p_ipmi2;
> > +
> > +     if (acpi_disabled)
> > +             return;
> > +
> > +     status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
> > +                             ACPI_UINT32_MAX,
> > +                             acpi_check_bmc_device, &device_count, NULL);
> > +     if (!device_count) {
> > +             /* when no IPMI device is found in ACPI namespace, return */
> > +             return;
> > +     }
> > +     list_for_each_entry_safe(p_ipmi, p_ipmi2, &acpi_ipmi, link) {
> > +             try_init_acpi_device(p_ipmi);
> > +             list_del(&p_ipmi->link);
> > +             kfree(p_ipmi);
> > +     }
> > +
> > +     return;
> > +}
> > +
> >  static __devinit void acpi_find_bmc(void)
> >  {
> >       acpi_status      status;
> > @@ -2014,6 +2404,7 @@ static __devinit void acpi_find_bmc(void)
> >       if (acpi_failure)
> >               return;
> >
> > +     /* locate the IPMI system interface in ACPI SPMI table */
> >       for (i = 0; ; i++) {
> >               status = acpi_get_table(ACPI_SIG_SPMI, i+1,
> >                                       (struct acpi_table_header **)&spmi);
> > @@ -2022,6 +2413,9 @@ static __devinit void acpi_find_bmc(void)
> >
> >               try_init_acpi(spmi);
> >       }
> > +
> > +     /* locate the IPMI system interface in ACPI device */
> > +     acpi_device_find_bmc();
> >  }
> >  #endif
> >
> 
> 

Re: [PATCH 1/2] IPMI/ACPI: Locate the IPMI system interface in ACPI namespace

[Bjorn Helgaas]

On Monday 26 October 2009 07:02:55 pm ykzhao wrote:
> In fact you mention two issues about the two patches:
>    1: Load a PNP driver for it to register the IPMI system interface.
> This is about the first patch.
>    2. coding style( for example: comments, the definition about some
> variables).
> 
> For the first issue: Before I start the first patch, I consider using
> the PNP device driver. But I find that it is so complex because of the
> following two points:
>     1. One is that we can't register the IPMI system interface if the
> boot option of "pnpacpi=off" is added. This will also depend on the PNP
> module.

This is not a problem.  It is perfectly acceptable for the IPMI driver
to depend on PNP and PNPACPI in order to claim an ACPI device.  If the
users boots with "pnpacpi=off", we just won't find an IPMI device.
That is the way it works for TPM devices and serial devices described
by ACPI, and IPMI should work the same way.

>     2. The second is that there exist so many cases about the IPMI
> IO/memory resource definition. Maybe there exist both IO/memory resource
> definition for one IPMI device. In such case we can't know which should
> be selected. At the same time we have similar issues about the interrupt
> type. So I decide to parse the IO/memory/interrupt resource
> independently.

This doesn't make any sense.  The fact that an IPMI device might have
a variety of IO/memory/IRQ resources is orthogonal to the question of
whether you should use pnp_register_driver() or acpi_walk_namespace().

PNPACPI parses the IPMI device resources for every ACPI device,
including the IPMI device, before we even know whether there will be
a PNP driver for the device.  It's much easier to look at the PNP
resources and figure out which to use than it is to use
acpi_walk_resources() manually.

The main point is that ipmi_si_intf.c is a device driver, and it should
use the normal driver registration mechanisms.  I think it would be
simplest and clearest to make a few PNP enhancements so it could use
pnp_register_driver(), but even using acpi_bus_register_driver() would
be fine.  Using acpi_walk_namespace() to do everything by hand is just
completely wrong.

Bjorn

Re: [PATCH 1/2] IPMI/ACPI: Locate the IPMI system interface in ACPI namespace

[Bjorn Helgaas]

On Tuesday 27 October 2009 09:38:41 am Bjorn Helgaas wrote:
> On Monday 26 October 2009 07:02:55 pm ykzhao wrote:
> > In fact you mention two issues about the two patches:
> >    1: Load a PNP driver for it to register the IPMI system interface.
> > This is about the first patch.
> >    2. coding style( for example: comments, the definition about some
> > variables).
> > 
> > For the first issue: Before I start the first patch, I consider using
> > the PNP device driver. But I find that it is so complex because of the
> > following two points:
> >     1. One is that we can't register the IPMI system interface if the
> > boot option of "pnpacpi=off" is added. This will also depend on the PNP
> > module.
> 
> This is not a problem.  It is perfectly acceptable for the IPMI driver
> to depend on PNP and PNPACPI in order to claim an ACPI device.  If the
> users boots with "pnpacpi=off", we just won't find an IPMI device.
> That is the way it works for TPM devices and serial devices described
> by ACPI, and IPMI should work the same way.
> 
> >     2. The second is that there exist so many cases about the IPMI
> > IO/memory resource definition. Maybe there exist both IO/memory resource
> > definition for one IPMI device. In such case we can't know which should
> > be selected. At the same time we have similar issues about the interrupt
> > type. So I decide to parse the IO/memory/interrupt resource
> > independently.
> 
> This doesn't make any sense.  The fact that an IPMI device might have
> a variety of IO/memory/IRQ resources is orthogonal to the question of
> whether you should use pnp_register_driver() or acpi_walk_namespace().
> 
> PNPACPI parses the IPMI device resources for every ACPI device,
> including the IPMI device, before we even know whether there will be
> a PNP driver for the device.  It's much easier to look at the PNP
> resources and figure out which to use than it is to use
> acpi_walk_resources() manually.
> 
> The main point is that ipmi_si_intf.c is a device driver, and it should
> use the normal driver registration mechanisms.  I think it would be
> simplest and clearest to make a few PNP enhancements so it could use
> pnp_register_driver(), but even using acpi_bus_register_driver() would
> be fine.  Using acpi_walk_namespace() to do everything by hand is just
> completely wrong.

Somebody asked me to clarify this, so here's some explanation that
might make this more clear.

He pointed out:
> The IPMI driver "stack" is split into three levels.  The middle
> layer, ipmi_msghandler, routes commands/responses/etc between
> upper and lower levels.  It has no clue about hardware OR device
> files.  It must be loaded first.
> 
> Lowest level is ipmi_si_intf, and talks directly to hardware, and
> it cares about memory/ports/IRQs/etc.  It only talks to ipmi_msghandler.
> Embedded in it are the three subdrivers (BT, KCS, SMIC) that
> do actual bit twiddling.
> 
> The top layer presents a character device file.  It connects only
> to ipmi_msghandler.
> 
> So device file registration stuff occurs in an entirely different
> source module than ACPI/PNP lookup.

I'm not concerned with the top or middle layers or the device file
registration.  The bottom layer talks directly to hardware, and that
makes it a Linux driver.  It has to use the right I/O ports, MMIO
regions, IRQs, etc., so it avoids conflicts with other devices in
the system.

The fact that your patch uses acpi_walk_namespace() to find the
device means that ipmi_si_intf.c can be talking to a device, but the
rest of the system doesn't know ipmi_si_intf.c "owns" it.  So another
driver B could come along and correctly use acpi_bus_register_driver()
with the IPMI IDs.  The Linux ACPI core knows nothing about the fact
that ipmi_si_intf.c thinks it "owns" the IPMI device, so it will call
the driver B "add" method.  Now we have two drivers that both think
they own the device.  This leads to chaos.

Bjorn

EC driver registration (was: IPMI/ACPI: Locate the IPMI system interface in ACPI namespace)

[Alexey Starikovskiy]

Hi Bjorn,

Currently EC driver work by doing either ECDT parse or acpi_walk_namespace,
Do you think it will be appropriate to change it to use PNP layer?

Thanks in advance,
Alex.

Re: EC driver registration (was: IPMI/ACPI: Locate the IPMI system interface in ACPI namespace)

[Bjorn Helgaas]

On Tuesday 27 October 2009 12:02:01 pm Alexey Starikovskiy wrote:
> Currently EC driver work by doing either ECDT parse or acpi_walk_namespace,
> Do you think it will be appropriate to change it to use PNP layer?

I don't see this.  In acpi_ec_init(), there's an acpi_bus_register_driver().
There's also the ECDT parsing in acpi_ec_ecdt_probe().  There's a comment
near the acpi_ec_ecdt_probe() call that says it must happen before the EC
is found in the namespace.

There is an acpi_walk_namespace() inside acpi_ec_add(), but that's only
to find _Qx methods under an EC device we've already found.

It's conceivable that the EC driver could use PNP registration rather
than ACPI registration -- that would let you get rid of the
acpi_walk_resources() in ec_parse_device().  But I don't know that
it's worth it right now.

For IPMI, we're adding a bunch of new code and there are structural
problems that EC doesn't have.  So IPMI feels more urgent to me.

Bjorn

Re: [PATCH 1/2] IPMI/ACPI: Locate the IPMI system interface in ACPI namespace

[ykzhao]

On Tue, 2009-10-27 at 23:38 +0800, Bjorn Helgaas wrote:
> On Monday 26 October 2009 07:02:55 pm ykzhao wrote:
> > In fact you mention two issues about the two patches:
> >    1: Load a PNP driver for it to register the IPMI system interface.
> > This is about the first patch.
> >    2. coding style( for example: comments, the definition about some
> > variables).
> > 
> > For the first issue: Before I start the first patch, I consider using
> > the PNP device driver. But I find that it is so complex because of the
> > following two points:
> >     1. One is that we can't register the IPMI system interface if the
> > boot option of "pnpacpi=off" is added. This will also depend on the PNP
> > module.
> 
> This is not a problem.  It is perfectly acceptable for the IPMI driver
> to depend on PNP and PNPACPI in order to claim an ACPI device.  If the
> users boots with "pnpacpi=off", we just won't find an IPMI device.
> That is the way it works for TPM devices and serial devices described
> by ACPI, and IPMI should work the same way.
Yes. Several methods can be used to detect the IPMI system interface.
In such case it still can be detected by using other method when the
ACPI detection mechanism is disabled. But the acpi detection mechanism
will depend on the ACPI and PNP subsystem if we detect the IMPI system
interface defined in ACPI table by using PNP device driver. 

At the same time there exist two ACPI detection mechanisms. One is
defined in SPMI table. The other is defined in ACPI table. We expect
that the two ACPI detection mechanisms depend on the same judgement
condition. 
So I prefer to detect the IPMI system interface when ACPI enabled
regardless of the boot option of "pnpacpi=off".
> 
> >     2. The second is that there exist so many cases about the IPMI
> > IO/memory resource definition. Maybe there exist both IO/memory resource
> > definition for one IPMI device. In such case we can't know which should
> > be selected. At the same time we have similar issues about the interrupt
> > type. So I decide to parse the IO/memory/interrupt resource
> > independently.
> 
> This doesn't make any sense.  The fact that an IPMI device might have
> a variety of IO/memory/IRQ resources is orthogonal to the question of
> whether you should use pnp_register_driver() or acpi_walk_namespace().
When we detect the IPMI system interface by loading PNP device driver,
the advantage is that it can re-use the parse mechanism of
IO/memory/interrupt. Right?
In fact we will have to evaluate the following ACPI object:
	>_IFT, _GPE, _SRV

If there exists the _GPE object, it is unnecessary to parse the resource
related with the interrupt.

At the same time as I mentioned in the previous email, sometimes we will
get the two different IO address definitions after evaluating the _CRS
object. Which should be selected?
If there exist both IO and memory address definition in _CRS object,
which should be selected?

> PNPACPI parses the IPMI device resources for every ACPI device,
> including the IPMI device, before we even know whether there will be
> a PNP driver for the device.  It's much easier to look at the PNP
> resources and figure out which to use than it is to use
> acpi_walk_resources() manually.
> 
> The main point is that ipmi_si_intf.c is a device driver, and it should
> use the normal driver registration mechanisms.  I think it would be
> simplest and clearest to make a few PNP enhancements so it could use
> pnp_register_driver(), but even using acpi_bus_register_driver() would
> be fine.  Using acpi_walk_namespace() to do everything by hand is just
> completely wrong.
The main purpose is to detect the IPMI system interface defined in ACPI
table.  If the device can be detected correctly, IMO it will be OK.

Why do think that it is wrong to use the acpi_walk_namespace to parse
the resource?

In the second patch it will register an ACPI device driver, which is
used to install the IPMI opregion space handler. After doing so, the
ACPI AML code can communicate with the IPMI system interface.

> 
> 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 1/2] IPMI/ACPI: Locate the IPMI system interface in ACPI namespace

[Bjorn Helgaas]

On Wed, 2009-10-28 at 10:50 +0800, ykzhao wrote:
> On Tue, 2009-10-27 at 23:38 +0800, Bjorn Helgaas wrote:
> > On Monday 26 October 2009 07:02:55 pm ykzhao wrote:
> > > In fact you mention two issues about the two patches:
> > >    1: Load a PNP driver for it to register the IPMI system interface.
> > > This is about the first patch.
> > >    2. coding style( for example: comments, the definition about some
> > > variables).
> > > 
> > > For the first issue: Before I start the first patch, I consider using
> > > the PNP device driver. But I find that it is so complex because of the
> > > following two points:
> > >     1. One is that we can't register the IPMI system interface if the
> > > boot option of "pnpacpi=off" is added. This will also depend on the PNP
> > > module.
> > 
> > This is not a problem.  It is perfectly acceptable for the IPMI driver
> > to depend on PNP and PNPACPI in order to claim an ACPI device.  If the
> > users boots with "pnpacpi=off", we just won't find an IPMI device.
> > That is the way it works for TPM devices and serial devices described
> > by ACPI, and IPMI should work the same way.
> Yes. Several methods can be used to detect the IPMI system interface.
> In such case it still can be detected by using other method when the
> ACPI detection mechanism is disabled. But the acpi detection mechanism
> will depend on the ACPI and PNP subsystem if we detect the IMPI system
> interface defined in ACPI table by using PNP device driver. 
> 
> At the same time there exist two ACPI detection mechanisms. One is
> defined in SPMI table. The other is defined in ACPI table. We expect
> that the two ACPI detection mechanisms depend on the same judgement
> condition. 
> So I prefer to detect the IPMI system interface when ACPI enabled
> regardless of the boot option of "pnpacpi=off".

The IPMI driver is not special.  It should behave like all other
drivers.  There is no reason it should handle the "pnpacpi=off" case
differently than other drivers.

But if you change this to use acpi_bus_register_driver(), there will be
no PNP dependency, "pnpacpi=off" will have no effect either way, and
I'll be happy.  I don't think it's the cleanest solution, but it at
least gives us a chance to properly bind the driver to the device.

> > >     2. The second is that there exist so many cases about the IPMI
> > > IO/memory resource definition. Maybe there exist both IO/memory resource
> > > definition for one IPMI device. In such case we can't know which should
> > > be selected. At the same time we have similar issues about the interrupt
> > > type. So I decide to parse the IO/memory/interrupt resource
> > > independently.
> > 
> > This doesn't make any sense.  The fact that an IPMI device might have
> > a variety of IO/memory/IRQ resources is orthogonal to the question of
> > whether you should use pnp_register_driver() or acpi_walk_namespace().
> When we detect the IPMI system interface by loading PNP device driver,
> the advantage is that it can re-use the parse mechanism of
> IO/memory/interrupt. Right?
> In fact we will have to evaluate the following ACPI object:
> 	>_IFT, _GPE, _SRV
> 
> If there exists the _GPE object, it is unnecessary to parse the resource
> related with the interrupt.
> 
> At the same time as I mentioned in the previous email, sometimes we will
> get the two different IO address definitions after evaluating the _CRS
> object. Which should be selected?
> If there exist both IO and memory address definition in _CRS object,
> which should be selected?

You have to decide which address to use whether you learn the addresses
by using acpi_walk_resources() or by looking through the resources
decoded by PNPACPI.  Using acpi_walk_resources() doesn't make that
choice any easier.

> > PNPACPI parses the IPMI device resources for every ACPI device,
> > including the IPMI device, before we even know whether there will be
> > a PNP driver for the device.  It's much easier to look at the PNP
> > resources and figure out which to use than it is to use
> > acpi_walk_resources() manually.
> > 
> > The main point is that ipmi_si_intf.c is a device driver, and it should
> > use the normal driver registration mechanisms.  I think it would be
> > simplest and clearest to make a few PNP enhancements so it could use
> > pnp_register_driver(), but even using acpi_bus_register_driver() would
> > be fine.  Using acpi_walk_namespace() to do everything by hand is just
> > completely wrong.
> The main purpose is to detect the IPMI system interface defined in ACPI
> table.  If the device can be detected correctly, IMO it will be OK.

It is important to detect the device.  It is also important to have a
mechanism to prevent two drivers from thinking they own the same device.

> Why do think that it is wrong to use the acpi_walk_namespace to parse
> the resource?

You're using acpi_walk_namespace() to locate the device, not to parse
the resources.  You use acpi_walk_resources() to parse the resources.

The fact that your patch uses acpi_walk_namespace() to find the
device means that ipmi_si_intf.c can be talking to a device, but the
rest of the system doesn't know ipmi_si_intf.c "owns" it.  So another
driver B could come along and correctly use acpi_bus_register_driver()
with the IPMI IDs.  The Linux ACPI core knows nothing about the fact
that ipmi_si_intf.c thinks it "owns" the IPMI device, so it will call
the driver B "add" method.  Now we have two drivers that both think
they own the device.  This leads to chaos.

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 1/2] IPMI/ACPI: Locate the IPMI system interface in ACPI namespace

[ykzhao]

On Wed, 2009-10-28 at 13:11 +0800, Bjorn Helgaas wrote:
> On Wed, 2009-10-28 at 10:50 +0800, ykzhao wrote:
> > On Tue, 2009-10-27 at 23:38 +0800, Bjorn Helgaas wrote:
> > > On Monday 26 October 2009 07:02:55 pm ykzhao wrote:
> > > > In fact you mention two issues about the two patches:
> > > >    1: Load a PNP driver for it to register the IPMI system interface.
> > > > This is about the first patch.
> > > >    2. coding style( for example: comments, the definition about some
> > > > variables).
> > > > 
> > > > For the first issue: Before I start the first patch, I consider using
> > > > the PNP device driver. But I find that it is so complex because of the
> > > > following two points:
> > > >     1. One is that we can't register the IPMI system interface if the
> > > > boot option of "pnpacpi=off" is added. This will also depend on the PNP
> > > > module.
> > > 
> > > This is not a problem.  It is perfectly acceptable for the IPMI driver
> > > to depend on PNP and PNPACPI in order to claim an ACPI device.  If the
> > > users boots with "pnpacpi=off", we just won't find an IPMI device.
> > > That is the way it works for TPM devices and serial devices described
> > > by ACPI, and IPMI should work the same way.
> > Yes. Several methods can be used to detect the IPMI system interface.
> > In such case it still can be detected by using other method when the
> > ACPI detection mechanism is disabled. But the acpi detection mechanism
> > will depend on the ACPI and PNP subsystem if we detect the IMPI system
> > interface defined in ACPI table by using PNP device driver. 
> > 
> > At the same time there exist two ACPI detection mechanisms. One is
> > defined in SPMI table. The other is defined in ACPI table. We expect
> > that the two ACPI detection mechanisms depend on the same judgement
> > condition. 
> > So I prefer to detect the IPMI system interface when ACPI enabled
> > regardless of the boot option of "pnpacpi=off".
> 
> The IPMI driver is not special.  It should behave like all other
> drivers.  There is no reason it should handle the "pnpacpi=off" case
> differently than other drivers.
> 
> But if you change this to use acpi_bus_register_driver(), there will be
> no PNP dependency, "pnpacpi=off" will have no effect either way, and
> I'll be happy.  I don't think it's the cleanest solution, but it at
> least gives us a chance to properly bind the driver to the device.
This patch set includes two parts. One is to detect the IPMI system
interface defined in ACPI table and register it. The other is to allow
the ACPI AML code to communicate with the IPMI system interface. 

As an device driver is already bound to the device in the second patch,
it is impossible that we load another device driver for the same IPMI
device.
> > > >     2. The second is that there exist so many cases about the IPMI
> > > > IO/memory resource definition. Maybe there exist both IO/memory resource
> > > > definition for one IPMI device. In such case we can't know which should
> > > > be selected. At the same time we have similar issues about the interrupt
> > > > type. So I decide to parse the IO/memory/interrupt resource
> > > > independently.
> > > 
> > > This doesn't make any sense.  The fact that an IPMI device might have
> > > a variety of IO/memory/IRQ resources is orthogonal to the question of
> > > whether you should use pnp_register_driver() or acpi_walk_namespace().
> > When we detect the IPMI system interface by loading PNP device driver,
> > the advantage is that it can re-use the parse mechanism of
> > IO/memory/interrupt. Right?
> > In fact we will have to evaluate the following ACPI object:
> > 	>_IFT, _GPE, _SRV
> > 
> > If there exists the _GPE object, it is unnecessary to parse the resource
> > related with the interrupt.
> > 
> > At the same time as I mentioned in the previous email, sometimes we will
> > get the two different IO address definitions after evaluating the _CRS
> > object. Which should be selected?
> > If there exist both IO and memory address definition in _CRS object,
> > which should be selected?
> 
> You have to decide which address to use whether you learn the addresses
> by using acpi_walk_resources() or by looking through the resources
> decoded by PNPACPI.  Using acpi_walk_resources() doesn't make that
> choice any easier.
> 
> > > PNPACPI parses the IPMI device resources for every ACPI device,
> > > including the IPMI device, before we even know whether there will be
> > > a PNP driver for the device.  It's much easier to look at the PNP
> > > resources and figure out which to use than it is to use
> > > acpi_walk_resources() manually.
> > > 
> > > The main point is that ipmi_si_intf.c is a device driver, and it should
> > > use the normal driver registration mechanisms.  I think it would be
> > > simplest and clearest to make a few PNP enhancements so it could use
> > > pnp_register_driver(), but even using acpi_bus_register_driver() would
> > > be fine.  Using acpi_walk_namespace() to do everything by hand is just
> > > completely wrong.
> > The main purpose is to detect the IPMI system interface defined in ACPI
> > table.  If the device can be detected correctly, IMO it will be OK.
> 
> It is important to detect the device.  It is also important to have a
> mechanism to prevent two drivers from thinking they own the same device.
> 
> > Why do think that it is wrong to use the acpi_walk_namespace to parse
> > the resource?
> 
> You're using acpi_walk_namespace() to locate the device, not to parse
> the resources.  You use acpi_walk_resources() to parse the resources.
Is it wrong to use acpi_walk_namespace to locate the device?
Why we can't use the acpi_walk_resources directly to parse the
corresponding resource? 
> The fact that your patch uses acpi_walk_namespace() to find the
> device means that ipmi_si_intf.c can be talking to a device, but the
> rest of the system doesn't know ipmi_si_intf.c "owns" it.  So another
> driver B could come along and correctly use acpi_bus_register_driver()
> with the IPMI IDs.  The Linux ACPI core knows nothing about the fact
> that ipmi_si_intf.c thinks it "owns" the IPMI device, so it will call
> the driver B "add" method.  Now we have two drivers that both think
> they own the device.  This leads to chaos.
Why is it necessary that the reset of the system doesn't know
ipmi_si_intf.c "owns" the IPMI system interface?
In fact the main purpose of ipmi_si_intf.c is to detect the available
IPMI system interface on a system. And then this interface can be used
by system management software.


> Bjorn
> 
> 


------------------------------------------------------------------------------
Come build with us! The BlackBerry(R) Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay 
ahead of the curve. Join us from November 9 - 12, 2009. Register now!
http://p.sf.net/sfu/devconference
_______________________________________________
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer

Re: [PATCH 1/2] IPMI/ACPI: Locate the IPMI system interface in ACPI namespace

[Bjorn Helgaas]

On Wed, 2009-10-28 at 17:51 +0800, ykzhao wrote:
> On Wed, 2009-10-28 at 13:11 +0800, Bjorn Helgaas wrote:
> > On Wed, 2009-10-28 at 10:50 +0800, ykzhao wrote:
> > > On Tue, 2009-10-27 at 23:38 +0800, Bjorn Helgaas wrote:
> > > > On Monday 26 October 2009 07:02:55 pm ykzhao wrote:
> > > > > In fact you mention two issues about the two patches:
> > > > >    1: Load a PNP driver for it to register the IPMI system interface.
> > > > > This is about the first patch.
> > > > >    2. coding style( for example: comments, the definition about some
> > > > > variables).
> > > > > 
> > > > > For the first issue: Before I start the first patch, I consider using
> > > > > the PNP device driver. But I find that it is so complex because of the
> > > > > following two points:
> > > > >     1. One is that we can't register the IPMI system interface if the
> > > > > boot option of "pnpacpi=off" is added. This will also depend on the PNP
> > > > > module.
> > > > 
> > > > This is not a problem.  It is perfectly acceptable for the IPMI driver
> > > > to depend on PNP and PNPACPI in order to claim an ACPI device.  If the
> > > > users boots with "pnpacpi=off", we just won't find an IPMI device.
> > > > That is the way it works for TPM devices and serial devices described
> > > > by ACPI, and IPMI should work the same way.
> > > Yes. Several methods can be used to detect the IPMI system interface.
> > > In such case it still can be detected by using other method when the
> > > ACPI detection mechanism is disabled. But the acpi detection mechanism
> > > will depend on the ACPI and PNP subsystem if we detect the IMPI system
> > > interface defined in ACPI table by using PNP device driver. 
> > > 
> > > At the same time there exist two ACPI detection mechanisms. One is
> > > defined in SPMI table. The other is defined in ACPI table. We expect
> > > that the two ACPI detection mechanisms depend on the same judgement
> > > condition. 
> > > So I prefer to detect the IPMI system interface when ACPI enabled
> > > regardless of the boot option of "pnpacpi=off".
> > 
> > The IPMI driver is not special.  It should behave like all other
> > drivers.  There is no reason it should handle the "pnpacpi=off" case
> > differently than other drivers.
> > 
> > But if you change this to use acpi_bus_register_driver(), there will be
> > no PNP dependency, "pnpacpi=off" will have no effect either way, and
> > I'll be happy.  I don't think it's the cleanest solution, but it at
> > least gives us a chance to properly bind the driver to the device.
> This patch set includes two parts. One is to detect the IPMI system
> interface defined in ACPI table and register it. The other is to allow
> the ACPI AML code to communicate with the IPMI system interface. 
> 
> As an device driver is already bound to the device in the second patch,
> it is impossible that we load another device driver for the same IPMI
> device.

I know.  That's why in my original responses a month ago, I suggested
that you integrate these two parts.  From the Linux point of view, one
piece of hardware gets one driver.  If you need two parts internally,
that's an implementation concern of the driver.  From the point of view
of the Linux/ACPI and PNP code, there must be a single driver.

> > > > >     2. The second is that there exist so many cases about the IPMI
> > > > > IO/memory resource definition. Maybe there exist both IO/memory resource
> > > > > definition for one IPMI device. In such case we can't know which should
> > > > > be selected. At the same time we have similar issues about the interrupt
> > > > > type. So I decide to parse the IO/memory/interrupt resource
> > > > > independently.
> > > > 
> > > > This doesn't make any sense.  The fact that an IPMI device might have
> > > > a variety of IO/memory/IRQ resources is orthogonal to the question of
> > > > whether you should use pnp_register_driver() or acpi_walk_namespace().
> > > When we detect the IPMI system interface by loading PNP device driver,
> > > the advantage is that it can re-use the parse mechanism of
> > > IO/memory/interrupt. Right?
> > > In fact we will have to evaluate the following ACPI object:
> > > 	>_IFT, _GPE, _SRV
> > > 
> > > If there exists the _GPE object, it is unnecessary to parse the resource
> > > related with the interrupt.
> > > 
> > > At the same time as I mentioned in the previous email, sometimes we will
> > > get the two different IO address definitions after evaluating the _CRS
> > > object. Which should be selected?
> > > If there exist both IO and memory address definition in _CRS object,
> > > which should be selected?
> > 
> > You have to decide which address to use whether you learn the addresses
> > by using acpi_walk_resources() or by looking through the resources
> > decoded by PNPACPI.  Using acpi_walk_resources() doesn't make that
> > choice any easier.
> > 
> > > > PNPACPI parses the IPMI device resources for every ACPI device,
> > > > including the IPMI device, before we even know whether there will be
> > > > a PNP driver for the device.  It's much easier to look at the PNP
> > > > resources and figure out which to use than it is to use
> > > > acpi_walk_resources() manually.
> > > > 
> > > > The main point is that ipmi_si_intf.c is a device driver, and it should
> > > > use the normal driver registration mechanisms.  I think it would be
> > > > simplest and clearest to make a few PNP enhancements so it could use
> > > > pnp_register_driver(), but even using acpi_bus_register_driver() would
> > > > be fine.  Using acpi_walk_namespace() to do everything by hand is just
> > > > completely wrong.
> > > The main purpose is to detect the IPMI system interface defined in ACPI
> > > table.  If the device can be detected correctly, IMO it will be OK.
> > 
> > It is important to detect the device.  It is also important to have a
> > mechanism to prevent two drivers from thinking they own the same device.
> > 
> > > Why do think that it is wrong to use the acpi_walk_namespace to parse
> > > the resource?
> > 
> > You're using acpi_walk_namespace() to locate the device, not to parse
> > the resources.  You use acpi_walk_resources() to parse the resources.
> Is it wrong to use acpi_walk_namespace to locate the device?

Yes, for the reason I explained below.  To repeat myself, you must use
acpi_bus_register_driver() (or pnp_register_driver()) so we can prevent
multiple drivers from claiming the same piece of hardware.

[There is an existing PNPACPI problem in that I think we currently allow
a driver using acpi_bus_register_driver() and a driver using
pnp_register_driver() to both claim the same device.  But this is an
existing issue with the Linux/ACPI and PNPACPI core, not with the
drivers themselves.]

> Why we can't use the acpi_walk_resources directly to parse the
> corresponding resource?

You can.  I think it would be easier in the long run to rely on the
resource parsing done by PNPACPI, but you can use acpi_walk_resources()
if you want to.

> > The fact that your patch uses acpi_walk_namespace() to find the
> > device means that ipmi_si_intf.c can be talking to a device, but the
> > rest of the system doesn't know ipmi_si_intf.c "owns" it.  So another
> > driver B could come along and correctly use acpi_bus_register_driver()
> > with the IPMI IDs.  The Linux ACPI core knows nothing about the fact
> > that ipmi_si_intf.c thinks it "owns" the IPMI device, so it will call
> > the driver B "add" method.  Now we have two drivers that both think
> > they own the device.  This leads to chaos.
> Why is it necessary that the reset of the system doesn't know
> ipmi_si_intf.c "owns" the IPMI system interface?

Using acpi_bus_register_driver() lets the Linux/ACPI core make sure that
only a single driver is bound to the device.  If two drivers bind to it,
they will interfere with each other.

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 1/2] IPMI/ACPI: Locate the IPMI system interface in ACPI namespace

[ykzhao]

On Thu, 2009-10-29 at 00:49 +0800, Bjorn Helgaas wrote:
> On Wed, 2009-10-28 at 17:51 +0800, ykzhao wrote:
> > On Wed, 2009-10-28 at 13:11 +0800, Bjorn Helgaas wrote:
> > > On Wed, 2009-10-28 at 10:50 +0800, ykzhao wrote:
> > > > On Tue, 2009-10-27 at 23:38 +0800, Bjorn Helgaas wrote:
> > > > > On Monday 26 October 2009 07:02:55 pm ykzhao wrote:
> > > > > > In fact you mention two issues about the two patches:
> > > > > >    1: Load a PNP driver for it to register the IPMI system interface.
> > > > > > This is about the first patch.
> > > > > >    2. coding style( for example: comments, the definition about some
> > > > > > variables).
> > > > > > 
> > > > > > For the first issue: Before I start the first patch, I consider using
> > > > > > the PNP device driver. But I find that it is so complex because of the
> > > > > > following two points:
> > > > > >     1. One is that we can't register the IPMI system interface if the
> > > > > > boot option of "pnpacpi=off" is added. This will also depend on the PNP
> > > > > > module.
> > > > > 
> > > > > This is not a problem.  It is perfectly acceptable for the IPMI driver
> > > > > to depend on PNP and PNPACPI in order to claim an ACPI device.  If the
> > > > > users boots with "pnpacpi=off", we just won't find an IPMI device.
> > > > > That is the way it works for TPM devices and serial devices described
> > > > > by ACPI, and IPMI should work the same way.
> > > > Yes. Several methods can be used to detect the IPMI system interface.
> > > > In such case it still can be detected by using other method when the
> > > > ACPI detection mechanism is disabled. But the acpi detection mechanism
> > > > will depend on the ACPI and PNP subsystem if we detect the IMPI system
> > > > interface defined in ACPI table by using PNP device driver. 
> > > > 
> > > > At the same time there exist two ACPI detection mechanisms. One is
> > > > defined in SPMI table. The other is defined in ACPI table. We expect
> > > > that the two ACPI detection mechanisms depend on the same judgement
> > > > condition. 
> > > > So I prefer to detect the IPMI system interface when ACPI enabled
> > > > regardless of the boot option of "pnpacpi=off".
> > > 
> > > The IPMI driver is not special.  It should behave like all other
> > > drivers.  There is no reason it should handle the "pnpacpi=off" case
> > > differently than other drivers.
> > > 
> > > But if you change this to use acpi_bus_register_driver(), there will be
> > > no PNP dependency, "pnpacpi=off" will have no effect either way, and
> > > I'll be happy.  I don't think it's the cleanest solution, but it at
> > > least gives us a chance to properly bind the driver to the device.
> > This patch set includes two parts. One is to detect the IPMI system
> > interface defined in ACPI table and register it. The other is to allow
> > the ACPI AML code to communicate with the IPMI system interface. 
> > 
> > As an device driver is already bound to the device in the second patch,
> > it is impossible that we load another device driver for the same IPMI
> > device.
> 
> I know.  That's why in my original responses a month ago, I suggested
> that you integrate these two parts.  From the Linux point of view, one
> piece of hardware gets one driver.  If you need two parts internally,
> that's an implementation concern of the driver.  From the point of view
> of the Linux/ACPI and PNP code, there must be a single driver.
Two patches can work independently. One it to detect and register the
IPMI system interface. Then the interface can be used by system
management software. Of course the ACPI AML code can create a channel to
communicate with the system interface.
The second is to create the relationship between ACPI AML code and IPMI
system interface(BMC). If the same IPMI system interface can be
registered by several methods(DMI, hardcode, default . etc). This patch
can work even without the first patch.

At the same time we will have to consider another issue. As we know, the
IPMI system interface is registered by using the function of
try_smi_init. But this function is defined as static function and we
can't call it out of ipmi_si_intf.c.
If we add the corresponding ACPI driver in the file of ipmi_si_intf.c,
it will be OK to merge them into one part. But this is a bad idea. The
acpi driver is located in the IPMI subsystem. Confusing?
> 
> > > > > >     2. The second is that there exist so many cases about the IPMI
> > > > > > IO/memory resource definition. Maybe there exist both IO/memory resource
> > > > > > definition for one IPMI device. In such case we can't know which should
> > > > > > be selected. At the same time we have similar issues about the interrupt
> > > > > > type. So I decide to parse the IO/memory/interrupt resource
> > > > > > independently.
> > > > > 
> > > > > This doesn't make any sense.  The fact that an IPMI device might have
> > > > > a variety of IO/memory/IRQ resources is orthogonal to the question of
> > > > > whether you should use pnp_register_driver() or acpi_walk_namespace().
> > > > When we detect the IPMI system interface by loading PNP device driver,
> > > > the advantage is that it can re-use the parse mechanism of
> > > > IO/memory/interrupt. Right?
> > > > In fact we will have to evaluate the following ACPI object:
> > > > 	>_IFT, _GPE, _SRV
> > > > 
> > > > If there exists the _GPE object, it is unnecessary to parse the resource
> > > > related with the interrupt.
> > > > 
> > > > At the same time as I mentioned in the previous email, sometimes we will
> > > > get the two different IO address definitions after evaluating the _CRS
> > > > object. Which should be selected?
> > > > If there exist both IO and memory address definition in _CRS object,
> > > > which should be selected?
> > > 
> > > You have to decide which address to use whether you learn the addresses
> > > by using acpi_walk_resources() or by looking through the resources
> > > decoded by PNPACPI.  Using acpi_walk_resources() doesn't make that
> > > choice any easier.
> > > 
> > > > > PNPACPI parses the IPMI device resources for every ACPI device,
> > > > > including the IPMI device, before we even know whether there will be
> > > > > a PNP driver for the device.  It's much easier to look at the PNP
> > > > > resources and figure out which to use than it is to use
> > > > > acpi_walk_resources() manually.
> > > > > 
> > > > > The main point is that ipmi_si_intf.c is a device driver, and it should
> > > > > use the normal driver registration mechanisms.  I think it would be
> > > > > simplest and clearest to make a few PNP enhancements so it could use
> > > > > pnp_register_driver(), but even using acpi_bus_register_driver() would
> > > > > be fine.  Using acpi_walk_namespace() to do everything by hand is just
> > > > > completely wrong.
> > > > The main purpose is to detect the IPMI system interface defined in ACPI
> > > > table.  If the device can be detected correctly, IMO it will be OK.
> > > 
> > > It is important to detect the device.  It is also important to have a
> > > mechanism to prevent two drivers from thinking they own the same device.
> > > 
> > > > Why do think that it is wrong to use the acpi_walk_namespace to parse
> > > > the resource?
> > > 
> > > You're using acpi_walk_namespace() to locate the device, not to parse
> > > the resources.  You use acpi_walk_resources() to parse the resources.
> > Is it wrong to use acpi_walk_namespace to locate the device?
> 
> Yes, for the reason I explained below.  To repeat myself, you must use
> acpi_bus_register_driver() (or pnp_register_driver()) so we can prevent
> multiple drivers from claiming the same piece of hardware.
> 
> [There is an existing PNPACPI problem in that I think we currently allow
> a driver using acpi_bus_register_driver() and a driver using
> pnp_register_driver() to both claim the same device.  But this is an
> existing issue with the Linux/ACPI and PNPACPI core, not with the
> drivers themselves.]
> 
> > Why we can't use the acpi_walk_resources directly to parse the
> > corresponding resource?
> 
> You can.  I think it would be easier in the long run to rely on the
> resource parsing done by PNPACPI, but you can use acpi_walk_resources()
> if you want to.
> 
> > > The fact that your patch uses acpi_walk_namespace() to find the
> > > device means that ipmi_si_intf.c can be talking to a device, but the
> > > rest of the system doesn't know ipmi_si_intf.c "owns" it.  So another
> > > driver B could come along and correctly use acpi_bus_register_driver()
> > > with the IPMI IDs.  The Linux ACPI core knows nothing about the fact
> > > that ipmi_si_intf.c thinks it "owns" the IPMI device, so it will call
> > > the driver B "add" method.  Now we have two drivers that both think
> > > they own the device.  This leads to chaos.
> > Why is it necessary that the reset of the system doesn't know
> > ipmi_si_intf.c "owns" the IPMI system interface?
> 
> Using acpi_bus_register_driver() lets the Linux/ACPI core make sure that
> only a single driver is bound to the device.  If two drivers bind to it,
> they will interfere with each other.
> 
> 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