* [RFC Patch 1/2]IPMI/ACPI: Locate the IPMI system interface in ACPI namespace
@ 2009-09-29 5:29 yakui.zhao
2009-09-29 5:29 ` [RFC Patch 2/2]IPMI/ACPI: Add the IPMI opregion driver to send IPMI message in AML code yakui.zhao
2009-09-29 15:51 ` [RFC Patch 1/2]IPMI/ACPI: Locate the IPMI system interface in ACPI namespace Bjorn Helgaas
0 siblings, 2 replies; 8+ messages in thread
From: yakui.zhao @ 2009-09-29 5:29 UTC (permalink / raw)
To: linux-acpi, openipmi-developer; +Cc: lenb, minyard, Zhao Yakui
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 file changed, 394 insertions(+)
Index: linux-2.6/drivers/char/ipmi/ipmi_si_intf.c
===================================================================
--- linux-2.6.orig/drivers/char/ipmi/ipmi_si_intf.c 2009-09-28 16:46:07.000000000 +0800
+++ linux-2.6/drivers/char/ipmi/ipmi_si_intf.c 2009-09-29 13:18:17.000000000 +0800
@@ -1813,6 +1813,35 @@
* 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 @@
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 @@
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 @@
try_init_acpi(spmi);
}
+
+ /* locate the IPMI system interface in ACPI device */
+ acpi_device_find_bmc();
}
#endif
^ permalink raw reply [flat|nested] 8+ messages in thread
* [RFC Patch 2/2]IPMI/ACPI: Add the IPMI opregion driver to send IPMI message in AML code
2009-09-29 5:29 [RFC Patch 1/2]IPMI/ACPI: Locate the IPMI system interface in ACPI namespace yakui.zhao
@ 2009-09-29 5:29 ` yakui.zhao
2009-09-30 15:37 ` Bjorn Helgaas
2009-09-29 15:51 ` [RFC Patch 1/2]IPMI/ACPI: Locate the IPMI system interface in ACPI namespace Bjorn Helgaas
1 sibling, 1 reply; 8+ messages in thread
From: yakui.zhao @ 2009-09-29 5:29 UTC (permalink / raw)
To: linux-acpi, openipmi-developer; +Cc: lenb, minyard, Zhao Yakui
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 | 567 ++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 580 insertions(+)
Index: linux-2.6/drivers/acpi/ipmi.c
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6/drivers/acpi/ipmi.c 2009-09-28 15:53:24.000000000 +0800
@@ -0,0 +1,567 @@
+/*
+ * 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 = 5;
+
+ 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);
+ }
+ 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);
+ 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) {
+ /* AE_TYPE is returned. */
+ return AE_TYPE;
+ }
+ if (!ipmi_device->user_interface) {
+ /* not exist */
+ 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);
Index: linux-2.6/drivers/acpi/Kconfig
===================================================================
--- linux-2.6.orig/drivers/acpi/Kconfig 2009-09-27 14:35:38.000000000 +0800
+++ linux-2.6/drivers/acpi/Kconfig 2009-09-28 13:10:46.000000000 +0800
@@ -204,6 +204,18 @@
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
Index: linux-2.6/drivers/acpi/Makefile
===================================================================
--- linux-2.6.orig/drivers/acpi/Makefile 2009-09-27 14:35:38.000000000 +0800
+++ linux-2.6/drivers/acpi/Makefile 2009-09-28 13:09:09.000000000 +0800
@@ -57,6 +57,7 @@
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC Patch 1/2]IPMI/ACPI: Locate the IPMI system interface in ACPI namespace
2009-09-29 5:29 [RFC Patch 1/2]IPMI/ACPI: Locate the IPMI system interface in ACPI namespace yakui.zhao
2009-09-29 5:29 ` [RFC Patch 2/2]IPMI/ACPI: Add the IPMI opregion driver to send IPMI message in AML code yakui.zhao
@ 2009-09-29 15:51 ` Bjorn Helgaas
2009-09-30 1:57 ` ykzhao
1 sibling, 1 reply; 8+ messages in thread
From: Bjorn Helgaas @ 2009-09-29 15:51 UTC (permalink / raw)
To: yakui.zhao; +Cc: linux-acpi, openipmi-developer, lenb, minyard
On Monday 28 September 2009 11:29:53 pm 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.
I echo Corey's coding style comments. The lack of consistency makes
it hard to read and review.
I don't like the fact that in this patch, you use acpi_walk_namespace()
to look for an IPI0001 device, and in the second patch, you register a
driver for IPI0001 devices. You're making two bindings -- the one
done "by hand" here, and the other done by acpi_bus_register_driver().
There should only be one binding between the IPI0001 device and a driver,
so somehow the code in these two patches should be integrated.
The acpi_parse_io_ports() function is big and complicated, and that
points out another problem. We shouldn't have to write all that code
every time we write an ACPI device driver.
I think you should register a *PNP* driver for IPI0001 devices, not an
ACPI driver. That way, you can take advantage of all the resource
parsing code in PNPACPI, and all the acpi_parse_io_ports() code would
go away.
There is a problem here, though, because you still need ACPI so you
can evaluate _IFT, _GPE, _SRV, etc. I think you should fix this by
adding a PNP interface to get back the ACPI handle from a PNP device
(this interface would fail if the device is not a PNPACPI device).
Bjorn
> Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
> ---
> drivers/char/ipmi/ipmi_si_intf.c | 394 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 394 insertions(+)
>
> Index: linux-2.6/drivers/char/ipmi/ipmi_si_intf.c
> ===================================================================
> --- linux-2.6.orig/drivers/char/ipmi/ipmi_si_intf.c 2009-09-28 16:46:07.000000000 +0800
> +++ linux-2.6/drivers/char/ipmi/ipmi_si_intf.c 2009-09-29 13:18:17.000000000 +0800
> @@ -1813,6 +1813,35 @@
> * 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 @@
> 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 @@
> 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 @@
>
> try_init_acpi(spmi);
> }
> +
> + /* locate the IPMI system interface in ACPI device */
> + acpi_device_find_bmc();
> }
> #endif
>
> --
> 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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC Patch 1/2]IPMI/ACPI: Locate the IPMI system interface in ACPI namespace
2009-09-29 15:51 ` [RFC Patch 1/2]IPMI/ACPI: Locate the IPMI system interface in ACPI namespace Bjorn Helgaas
@ 2009-09-30 1:57 ` ykzhao
2009-09-30 15:19 ` Bjorn Helgaas
0 siblings, 1 reply; 8+ messages in thread
From: ykzhao @ 2009-09-30 1:57 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-acpi, openipmi-developer, lenb, minyard
On Tue, 2009-09-29 at 23:51 +0800, Bjorn Helgaas wrote:
> On Monday 28 September 2009 11:29:53 pm 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.
>
> I echo Corey's coding style comments. The lack of consistency makes
> it hard to read and review.
Do you mean the first patch or the second patch?
Can you point it out clearly?
>
> I don't like the fact that in this patch, you use acpi_walk_namespace()
> to look for an IPI0001 device, and in the second patch, you register a
> driver for IPI0001 devices. You're making two bindings -- the one
> done "by hand" here, and the other done by acpi_bus_register_driver().
> There should only be one binding between the IPI0001 device and a driver,
> so somehow the code in these two patches should be integrated.
To merge them together will make the problem complicated.
The IPMI system interface located in ACPI namespace is not only used by
Linux ACPI, but only is used by the System management software.
The two patches do the different things:
1. locate the IPMI system interface and register it.
In such case it still can work well even when we don't compile the
second patch.
2. create the IPMI user interface and handle the access to IPMI
opregion in AML code
When the IPI0001 device is detected, we will create the
corresponding IPMI user interface and install the IPMI opregion space
handler. Then it will handle the access to IPMI opregion in AML code and
construct/deliver the IPMI message to BMC device.
> The acpi_parse_io_ports() function is big and complicated, and that
> points out another problem. We shouldn't have to write all that code
> every time we write an ACPI device driver.
The function of acpi_parse_io_ports seems big. But in fact it only do
one thing that parses the required resources for the IPMI system
interface.
For example:
memory I/O address;
Get the GSI interrupt number when it is defined in _CRS method.
In fact as we don't know the exact definition about _CRS for the IPI0001
device, we will have to write its own code to parse the required
resource and set the corresponding flag(For example: address type,
Interrupt type)
>
> I think you should register a *PNP* driver for IPI0001 devices, not an
> ACPI driver. That way, you can take advantage of all the resource
> parsing code in PNPACPI, and all the acpi_parse_io_ports() code would
> go away.
Under what you said.
But it will depend on the PNP subsystem. If the boot option of
"pnpaci=off" is added, we won't add the Pnp ACPI device, which causes
that the IPMI system interface can't be located in ACPI namespace.
>
> There is a problem here, though, because you still need ACPI so you
> can evaluate _IFT, _GPE, _SRV, etc. I think you should fix this by
> adding a PNP interface to get back the ACPI handle from a PNP device
> (this interface would fail if the device is not a PNPACPI device).
>
> Bjorn
>
>
> > Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
> > ---
> > drivers/char/ipmi/ipmi_si_intf.c | 394 +++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 394 insertions(+)
> >
> > Index: linux-2.6/drivers/char/ipmi/ipmi_si_intf.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/char/ipmi/ipmi_si_intf.c 2009-09-28 16:46:07.000000000 +0800
> > +++ linux-2.6/drivers/char/ipmi/ipmi_si_intf.c 2009-09-29 13:18:17.000000000 +0800
> > @@ -1813,6 +1813,35 @@
> > * 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 @@
> > 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 @@
> > 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 @@
> >
> > try_init_acpi(spmi);
> > }
> > +
> > + /* locate the IPMI system interface in ACPI device */
> > + acpi_device_find_bmc();
> > }
> > #endif
> >
> > --
> > 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
> >
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC Patch 1/2]IPMI/ACPI: Locate the IPMI system interface in ACPI namespace
2009-09-30 1:57 ` ykzhao
@ 2009-09-30 15:19 ` Bjorn Helgaas
0 siblings, 0 replies; 8+ messages in thread
From: Bjorn Helgaas @ 2009-09-30 15:19 UTC (permalink / raw)
To: ykzhao; +Cc: linux-acpi, openipmi-developer, lenb, minyard
On Tuesday 29 September 2009 07:57:01 pm ykzhao wrote:
> On Tue, 2009-09-29 at 23:51 +0800, Bjorn Helgaas wrote:
> > On Monday 28 September 2009 11:29:53 pm 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.
> >
> > I echo Corey's coding style comments. The lack of consistency makes
> > it hard to read and review.
> Do you mean the first patch or the second patch?
> Can you point it out clearly?
Yes, I will respond in another mail and point out a few things. But
I don't want to spend a lot of time on those little things because
I want to sort out the big things first. For example, I feel strongly
that the structure is currently wrong, and we should address that
before worrying about trivial things like indentation and blank lines.
> > I don't like the fact that in this patch, you use acpi_walk_namespace()
> > to look for an IPI0001 device, and in the second patch, you register a
> > driver for IPI0001 devices. You're making two bindings -- the one
> > done "by hand" here, and the other done by acpi_bus_register_driver().
> > There should only be one binding between the IPI0001 device and a driver,
> > so somehow the code in these two patches should be integrated.
> To merge them together will make the problem complicated.
> The IPMI system interface located in ACPI namespace is not only used by
> Linux ACPI, but only is used by the System management software.
> The two patches do the different things:
> 1. locate the IPMI system interface and register it.
> In such case it still can work well even when we don't compile the
> second patch.
> 2. create the IPMI user interface and handle the access to IPMI
> opregion in AML code
> When the IPI0001 device is detected, we will create the
> corresponding IPMI user interface and install the IPMI opregion space
> handler. Then it will handle the access to IPMI opregion in AML code and
> construct/deliver the IPMI message to BMC device.
For case 1 (locate the IPMI system interface and register it), I
think you should definitely be using acpi_bus_register_driver() or
pnp_register_driver() because this is the driver: it claims the device
and must have exclusive access to it.
For case 2 (opregion stuff), you don't touch the device directly at
all, so that part should NOT be using acpi_bus_register_driver().
I think this part doesn't make any sense unless the IPMI driver is
already loaded. Maybe this opregion stuff could be an optional
part of the IPMI driver. For example, maybe CONFIG_ACPI_IPMI
should depend on CONFIG_ACPI && CONFIG_IPMI_SI.
> > The acpi_parse_io_ports() function is big and complicated, and that
> > points out another problem. We shouldn't have to write all that code
> > every time we write an ACPI device driver.
> The function of acpi_parse_io_ports seems big. But in fact it only do
> one thing that parses the required resources for the IPMI system
> interface.
> For example:
> memory I/O address;
> Get the GSI interrupt number when it is defined in _CRS method.
>
> In fact as we don't know the exact definition about _CRS for the IPI0001
> device, we will have to write its own code to parse the required
> resource and set the corresponding flag(For example: address type,
> Interrupt type)
I understand what acpi_parse_io_ports() does. I've written plenty
of that sort of code myself. But PNPACPI already contains all the
code to parse the _CRS (see rsparser.c). We should take advantage
of that rather than duplicating it, as acpi_parse_io_ports() does.
For example, take a look at 8250_pnp.c. It claims ACPI serial
devices via PNPACPI. Because we use PNPACPI, we were able to remove
8250_acpi.c (commit 111c9bf8c5cfa92363b3719c8956d29368b5b9de), which
had a bunch of _CRS parsing code very similar to what you're doing
in acpi_parse_io_ports().
> > I think you should register a *PNP* driver for IPI0001 devices, not an
> > ACPI driver. That way, you can take advantage of all the resource
> > parsing code in PNPACPI, and all the acpi_parse_io_ports() code would
> > go away.
> Under what you said.
I don't know what this means.
> But it will depend on the PNP subsystem. If the boot option of
> "pnpaci=off" is added, we won't add the Pnp ACPI device, which causes
> that the IPMI system interface can't be located in ACPI namespace.
Yes. That's the way it should work. This is not a problem.
Bjorn
> > There is a problem here, though, because you still need ACPI so you
> > can evaluate _IFT, _GPE, _SRV, etc. I think you should fix this by
> > adding a PNP interface to get back the ACPI handle from a PNP device
> > (this interface would fail if the device is not a PNPACPI device).
> >
> > Bjorn
> >
> >
> > > Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
> > > ---
> > > drivers/char/ipmi/ipmi_si_intf.c | 394 +++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 394 insertions(+)
> > >
> > > Index: linux-2.6/drivers/char/ipmi/ipmi_si_intf.c
> > > ===================================================================
> > > --- linux-2.6.orig/drivers/char/ipmi/ipmi_si_intf.c 2009-09-28 16:46:07.000000000 +0800
> > > +++ linux-2.6/drivers/char/ipmi/ipmi_si_intf.c 2009-09-29 13:18:17.000000000 +0800
> > > @@ -1813,6 +1813,35 @@
> > > * 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 @@
> > > 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 @@
> > > 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 @@
> > >
> > > try_init_acpi(spmi);
> > > }
> > > +
> > > + /* locate the IPMI system interface in ACPI device */
> > > + acpi_device_find_bmc();
> > > }
> > > #endif
> > >
> > > --
> > > 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
> > >
> >
> >
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC Patch 2/2]IPMI/ACPI: Add the IPMI opregion driver to send IPMI message in AML code
2009-09-29 5:29 ` [RFC Patch 2/2]IPMI/ACPI: Add the IPMI opregion driver to send IPMI message in AML code yakui.zhao
@ 2009-09-30 15:37 ` Bjorn Helgaas
2009-10-09 1:59 ` ykzhao
0 siblings, 1 reply; 8+ messages in thread
From: Bjorn Helgaas @ 2009-09-30 15:37 UTC (permalink / raw)
To: yakui.zhao; +Cc: linux-acpi, openipmi-developer, minyard, lenb
On Monday 28 September 2009 11:29:54 pm yakui.zhao@intel.com wrote:
These are a few coding style nits. As I mentioned, I think there
are much bigger issues to worry about first, so please don't just
fix the coding style problems below and repost this. Work on the
bigger things like driver registration first.
> Index: linux-2.6/drivers/acpi/ipmi.c
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/drivers/acpi/ipmi.c 2009-09-28 15:53:24.000000000 +0800
> @@ -0,0 +1,567 @@
> +/*
> + * 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"
I know this is common style, but I think it's stupid to add this
#define since the value is only used once.
> +#define ACPI_IPMI_DEVICE_NAME "IPMI_dev"
This one isn't used at all.
> +#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 */
Comment is superfluous.
> + u8 status_code;
> + /* the length of the payload */
Comment is superfluous.
> + 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.
"Operation" is spelled wrong twice.
> + */
> + 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;
We don't usually use tabs in local variable lists.
> +
> + 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
> + */
Use a consistent comment style, e.g.,
/*
* Value is ...
*/
(Applies many places.)
> + 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 = 5;
> +
> + 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);
> + }
> + 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;
No cast needed here.
> + 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);
> + ipmi_device->user_interface = user;
> + }
> + return;
No "return" statement needed here.
> +}
> +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;
No initialization needed.
> + struct acpi_ipmi_device *ipmi_device =
> + (struct acpi_ipmi_device *) handler_context;
No cast needed.
> + int err;
> + acpi_status status;
Need a blank line here.
> + /*
> + * 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) {
> + /* AE_TYPE is returned. */
Superfluous comment.
> + return AE_TYPE;
> + }
> + if (!ipmi_device->user_interface) {
> + /* not exist */
Superfluous comment.
> + return AE_NOT_EXIST;
> + }
> + tx_msg = acpi_alloc_ipmi_msg(ipmi_device);
> + if (!tx_msg) {
> + /* no memory is allocated */
Superfluous comment.
> + return AE_NO_MEMORY;
> + }
> + acpi_format_ipmi_msg(tx_msg, address, value);
> + mutex_lock(&ipmi_device->mutex_lock);
> + list_add_tail(&tx_msg->head, &ipmi_device->tx_msg_list);
> + mutex_unlock(&ipmi_device->mutex_lock);
> + err = ipmi_request_settime(ipmi_device->user_interface,
> + &tx_msg->addr,
> + tx_msg->tx_msgid,
> + &tx_msg->tx_message,
> + NULL, 0, 0, 0);
> + if (err) {
> + status = AE_ERROR;
> + goto end_label;
> + }
> + err = wait_for_completion_timeout(&tx_msg->tx_complete, IPMI_TIMEOUT);
> +
> +end_label:
> + acpi_format_ipmi_response(tx_msg, value, err);
> + status = AE_OK;
> + 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))
I don't think you need to use test_bit(), clear_bit(), and set_bit()
for this flag.
> + 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;
Need a blank line here.
> + if (!device)
> + return -EINVAL;
No need to test this.
> +
> + handle = device->handle;
> + temp = 0;
No need to initialize "temp".
> + 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 */
Superfluous comment.
> + kfree(ipmi_device);
> + return -EINVAL;
> + }
> +
> + /* add it to the IPMI device list */
Superfluous comment.
> + 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);
Put this in the local variable list, e.g.,
struct acpi_ipmi_device *ipmi_device = acpi_driver_data(device);
> + if (!ipmi_device)
> + return 0;
No need to test this; if we got here, it means acpi_ipmi_add()
succeeded, and it set the driver_data pointer.
> +
> + if (ipmi_device->user_interface) {
> + /*
> + * If the IPMI user interface is created, it should be
> + * destroyed.
> + */
Superfluous comment.
> + 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;
No need to initialize "result".
> +
> + if (acpi_disabled)
> + return result;
No need to check this; acpi_bus_register_driver() does it already.
> +
> + result = acpi_bus_register_driver(&acpi_ipmi_driver);
> +
Remove these blank lines.
> + 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;
Remove this check, too.
> +
> + ipmi_smi_watcher_unregister(&driver_data.bmc_events);
> + acpi_bus_unregister_driver(&acpi_ipmi_driver);
> +
> + return;
No "return" statement needed here.
> +}
> +
> +module_init(acpi_ipmi_init);
> +module_exit(acpi_ipmi_exit);
> Index: linux-2.6/drivers/acpi/Kconfig
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/Kconfig 2009-09-27 14:35:38.000000000 +0800
> +++ linux-2.6/drivers/acpi/Kconfig 2009-09-28 13:10:46.000000000 +0800
> @@ -204,6 +204,18 @@
>
> 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.
"on on" is a typo.
> +
> + To compile this driver as a module, choose M here:
Looks like you're missing some "module will be called" text here.
>
> config ACPI_HOTPLUG_CPU
> bool
> Index: linux-2.6/drivers/acpi/Makefile
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/Makefile 2009-09-27 14:35:38.000000000 +0800
> +++ linux-2.6/drivers/acpi/Makefile 2009-09-28 13:09:09.000000000 +0800
> @@ -57,6 +57,7 @@
> 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
> --
> 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
>
------------------------------------------------------------------------------
Come build with us! The BlackBerry® 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/devconf
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC Patch 2/2]IPMI/ACPI: Add the IPMI opregion driver to send IPMI message in AML code
2009-09-30 15:37 ` Bjorn Helgaas
@ 2009-10-09 1:59 ` ykzhao
2009-10-09 3:03 ` Bjorn Helgaas
0 siblings, 1 reply; 8+ messages in thread
From: ykzhao @ 2009-10-09 1:59 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-acpi, openipmi-developer, minyard, lenb
On Wed, 2009-09-30 at 23:37 +0800, Bjorn Helgaas wrote:
> On Monday 28 September 2009 11:29:54 pm yakui.zhao@intel.com wrote:
>
> These are a few coding style nits. As I mentioned, I think there
> are much bigger issues to worry about first, so please don't just
> fix the coding style problems below and repost this. Work on the
> bigger things like driver registration first.
Sorry for the late response.
Thanks for your comments.
And it seems that it is mainly about the coding style nits.
Although it is unnecessary to initialize some temporary variables, it is
harmless to initialize them.
Will you please point out the bigger issues you worried about?
Thanks.
> > Index: linux-2.6/drivers/acpi/ipmi.c
> > ===================================================================
> > --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> > +++ linux-2.6/drivers/acpi/ipmi.c 2009-09-28 15:53:24.000000000 +0800
> > @@ -0,0 +1,567 @@
> > +/*
> > + * 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"
>
> I know this is common style, but I think it's stupid to add this
> #define since the value is only used once.
>
> > +#define ACPI_IPMI_DEVICE_NAME "IPMI_dev"
>
> This one isn't used at all.
>
> > +#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.
> > + */
IMO it will be better to add some comments for the below definition.
It will be helpful to understand the meaning of acpi IPMI buffer.
for example: the status code. If we don't add the comment, maybe we will
be confusing. Is it the ACPI status code or IPMI status code?
> > +struct acpi_ipmi_buffer {
> > + /* status code of a given IPMI command */
>
> Comment is superfluous.
>
> > + u8 status_code;
> > + /* the length of the payload */
>
> Comment is superfluous.
>
> > + 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.
>
> "Operation" is spelled wrong twice.
>
> > + */
> > + 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;
>
> We don't usually use tabs in local variable lists.
>
> > +
> > + 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
> > + */
>
> Use a consistent comment style, e.g.,
>
> /*
> * Value is ...
> */
>
> (Applies many places.)
>
> > + 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 = 5;
> > +
> > + 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);
> > + }
> > + 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;
>
> No cast needed here.
>
> > + 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);
> > + ipmi_device->user_interface = user;
> > + }
> > + return;
>
> No "return" statement needed here.
>
> > +}
> > +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;
>
> No initialization needed.
>
> > + struct acpi_ipmi_device *ipmi_device =
> > + (struct acpi_ipmi_device *) handler_context;
>
> No cast needed.
>
> > + int err;
> > + acpi_status status;
>
> Need a blank line here.
>
> > + /*
> > + * 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) {
> > + /* AE_TYPE is returned. */
>
> Superfluous comment.
>
> > + return AE_TYPE;
> > + }
> > + if (!ipmi_device->user_interface) {
> > + /* not exist */
>
> Superfluous comment.
>
> > + return AE_NOT_EXIST;
> > + }
> > + tx_msg = acpi_alloc_ipmi_msg(ipmi_device);
> > + if (!tx_msg) {
> > + /* no memory is allocated */
>
> Superfluous comment.
>
> > + return AE_NO_MEMORY;
> > + }
> > + acpi_format_ipmi_msg(tx_msg, address, value);
> > + mutex_lock(&ipmi_device->mutex_lock);
> > + list_add_tail(&tx_msg->head, &ipmi_device->tx_msg_list);
> > + mutex_unlock(&ipmi_device->mutex_lock);
> > + err = ipmi_request_settime(ipmi_device->user_interface,
> > + &tx_msg->addr,
> > + tx_msg->tx_msgid,
> > + &tx_msg->tx_message,
> > + NULL, 0, 0, 0);
> > + if (err) {
> > + status = AE_ERROR;
> > + goto end_label;
> > + }
> > + err = wait_for_completion_timeout(&tx_msg->tx_complete, IPMI_TIMEOUT);
> > +
> > +end_label:
> > + acpi_format_ipmi_response(tx_msg, value, err);
> > + status = AE_OK;
> > + 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))
>
> I don't think you need to use test_bit(), clear_bit(), and set_bit()
> for this flag.
>
> > + 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;
>
> Need a blank line here.
>
> > + if (!device)
> > + return -EINVAL;
>
> No need to test this.
>
> > +
> > + handle = device->handle;
> > + temp = 0;
>
> No need to initialize "temp".
>
> > + 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 */
>
> Superfluous comment.
>
> > + kfree(ipmi_device);
> > + return -EINVAL;
> > + }
> > +
> > + /* add it to the IPMI device list */
>
> Superfluous comment.
>
> > + 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);
>
> Put this in the local variable list, e.g.,
>
> struct acpi_ipmi_device *ipmi_device = acpi_driver_data(device);
>
> > + if (!ipmi_device)
> > + return 0;
>
> No need to test this; if we got here, it means acpi_ipmi_add()
> succeeded, and it set the driver_data pointer.
>
> > +
> > + if (ipmi_device->user_interface) {
> > + /*
> > + * If the IPMI user interface is created, it should be
> > + * destroyed.
> > + */
>
> Superfluous comment.
>
> > + 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;
>
> No need to initialize "result".
>
> > +
> > + if (acpi_disabled)
> > + return result;
>
> No need to check this; acpi_bus_register_driver() does it already.
>
> > +
> > + result = acpi_bus_register_driver(&acpi_ipmi_driver);
> > +
>
> Remove these blank lines.
>
> > + 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;
>
> Remove this check, too.
>
> > +
> > + ipmi_smi_watcher_unregister(&driver_data.bmc_events);
> > + acpi_bus_unregister_driver(&acpi_ipmi_driver);
> > +
> > + return;
>
> No "return" statement needed here.
>
> > +}
> > +
> > +module_init(acpi_ipmi_init);
> > +module_exit(acpi_ipmi_exit);
> > Index: linux-2.6/drivers/acpi/Kconfig
> > ===================================================================
> > --- linux-2.6.orig/drivers/acpi/Kconfig 2009-09-27 14:35:38.000000000 +0800
> > +++ linux-2.6/drivers/acpi/Kconfig 2009-09-28 13:10:46.000000000 +0800
> > @@ -204,6 +204,18 @@
> >
> > 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.
>
> "on on" is a typo.
>
> > +
> > + To compile this driver as a module, choose M here:
>
> Looks like you're missing some "module will be called" text here.
Do you mean that the selected modules should also be listed?
>
> >
> > config ACPI_HOTPLUG_CPU
> > bool
> > Index: linux-2.6/drivers/acpi/Makefile
> > ===================================================================
> > --- linux-2.6.orig/drivers/acpi/Makefile 2009-09-27 14:35:38.000000000 +0800
> > +++ linux-2.6/drivers/acpi/Makefile 2009-09-28 13:09:09.000000000 +0800
> > @@ -57,6 +57,7 @@
> > 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
> > --
> > 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
> >
>
>
------------------------------------------------------------------------------
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC Patch 2/2]IPMI/ACPI: Add the IPMI opregion driver to send IPMI message in AML code
2009-10-09 1:59 ` ykzhao
@ 2009-10-09 3:03 ` Bjorn Helgaas
0 siblings, 0 replies; 8+ messages in thread
From: Bjorn Helgaas @ 2009-10-09 3:03 UTC (permalink / raw)
To: ykzhao; +Cc: linux-acpi, openipmi-developer, minyard, lenb
On Fri, 2009-10-09 at 09:59 +0800, ykzhao wrote:
> On Wed, 2009-09-30 at 23:37 +0800, Bjorn Helgaas wrote:
> > On Monday 28 September 2009 11:29:54 pm yakui.zhao@intel.com wrote:
> >
> > These are a few coding style nits. As I mentioned, I think there
> > are much bigger issues to worry about first, so please don't just
> > fix the coding style problems below and repost this. Work on the
> > bigger things like driver registration first.
> Sorry for the late response.
> Thanks for your comments.
> And it seems that it is mainly about the coding style nits.
>
> Although it is unnecessary to initialize some temporary variables, it is
> harmless to initialize them.
>
> Will you please point out the bigger issues you worried about?
I already pointed them out in a previous message, but I'll repeat them
here:
... For example, I feel strongly
that the structure is currently wrong, and we should address that
before worrying about trivial things like indentation and blank lines.
...
For case 1 (locate the IPMI system interface and register it), I
think you should definitely be using acpi_bus_register_driver() or
pnp_register_driver() because this is the driver: it claims the device
and must have exclusive access to it.
For case 2 (opregion stuff), you don't touch the device directly at
all, so that part should NOT be using acpi_bus_register_driver().
I think this part doesn't make any sense unless the IPMI driver is
already loaded. Maybe this opregion stuff could be an optional
part of the IPMI driver. For example, maybe CONFIG_ACPI_IPMI
should depend on CONFIG_ACPI && CONFIG_IPMI_SI.
...
I understand what acpi_parse_io_ports() does. I've written plenty
of that sort of code myself. But PNPACPI already contains all the
code to parse the _CRS (see rsparser.c). We should take advantage
of that rather than duplicating it, as acpi_parse_io_ports() does.
For example, take a look at 8250_pnp.c. It claims ACPI serial
devices via PNPACPI. Because we use PNPACPI, we were able to remove
8250_acpi.c (commit 111c9bf8c5cfa92363b3719c8956d29368b5b9de), which
had a bunch of _CRS parsing code very similar to what you're doing
in acpi_parse_io_ports().
And see one Kconfig response below.
Bjorn
> Thanks.
> > > Index: linux-2.6/drivers/acpi/ipmi.c
> > > ===================================================================
> > > --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> > > +++ linux-2.6/drivers/acpi/ipmi.c 2009-09-28 15:53:24.000000000 +0800
> > > @@ -0,0 +1,567 @@
> > > +/*
> > > + * 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"
> >
> > I know this is common style, but I think it's stupid to add this
> > #define since the value is only used once.
> >
> > > +#define ACPI_IPMI_DEVICE_NAME "IPMI_dev"
> >
> > This one isn't used at all.
> >
> > > +#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.
> > > + */
> IMO it will be better to add some comments for the below definition.
> It will be helpful to understand the meaning of acpi IPMI buffer.
> for example: the status code. If we don't add the comment, maybe we will
> be confusing. Is it the ACPI status code or IPMI status code?
> > > +struct acpi_ipmi_buffer {
> > > + /* status code of a given IPMI command */
> >
> > Comment is superfluous.
> >
> > > + u8 status_code;
> > > + /* the length of the payload */
> >
> > Comment is superfluous.
> >
> > > + 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.
> >
> > "Operation" is spelled wrong twice.
> >
> > > + */
> > > + 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;
> >
> > We don't usually use tabs in local variable lists.
> >
> > > +
> > > + 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
> > > + */
> >
> > Use a consistent comment style, e.g.,
> >
> > /*
> > * Value is ...
> > */
> >
> > (Applies many places.)
> >
> > > + 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 = 5;
> > > +
> > > + 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);
> > > + }
> > > + 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;
> >
> > No cast needed here.
> >
> > > + 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);
> > > + ipmi_device->user_interface = user;
> > > + }
> > > + return;
> >
> > No "return" statement needed here.
> >
> > > +}
> > > +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;
> >
> > No initialization needed.
> >
> > > + struct acpi_ipmi_device *ipmi_device =
> > > + (struct acpi_ipmi_device *) handler_context;
> >
> > No cast needed.
> >
> > > + int err;
> > > + acpi_status status;
> >
> > Need a blank line here.
> >
> > > + /*
> > > + * 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) {
> > > + /* AE_TYPE is returned. */
> >
> > Superfluous comment.
> >
> > > + return AE_TYPE;
> > > + }
> > > + if (!ipmi_device->user_interface) {
> > > + /* not exist */
> >
> > Superfluous comment.
> >
> > > + return AE_NOT_EXIST;
> > > + }
> > > + tx_msg = acpi_alloc_ipmi_msg(ipmi_device);
> > > + if (!tx_msg) {
> > > + /* no memory is allocated */
> >
> > Superfluous comment.
> >
> > > + return AE_NO_MEMORY;
> > > + }
> > > + acpi_format_ipmi_msg(tx_msg, address, value);
> > > + mutex_lock(&ipmi_device->mutex_lock);
> > > + list_add_tail(&tx_msg->head, &ipmi_device->tx_msg_list);
> > > + mutex_unlock(&ipmi_device->mutex_lock);
> > > + err = ipmi_request_settime(ipmi_device->user_interface,
> > > + &tx_msg->addr,
> > > + tx_msg->tx_msgid,
> > > + &tx_msg->tx_message,
> > > + NULL, 0, 0, 0);
> > > + if (err) {
> > > + status = AE_ERROR;
> > > + goto end_label;
> > > + }
> > > + err = wait_for_completion_timeout(&tx_msg->tx_complete, IPMI_TIMEOUT);
> > > +
> > > +end_label:
> > > + acpi_format_ipmi_response(tx_msg, value, err);
> > > + status = AE_OK;
> > > + 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))
> >
> > I don't think you need to use test_bit(), clear_bit(), and set_bit()
> > for this flag.
> >
> > > + 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;
> >
> > Need a blank line here.
> >
> > > + if (!device)
> > > + return -EINVAL;
> >
> > No need to test this.
> >
> > > +
> > > + handle = device->handle;
> > > + temp = 0;
> >
> > No need to initialize "temp".
> >
> > > + 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 */
> >
> > Superfluous comment.
> >
> > > + kfree(ipmi_device);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + /* add it to the IPMI device list */
> >
> > Superfluous comment.
> >
> > > + 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);
> >
> > Put this in the local variable list, e.g.,
> >
> > struct acpi_ipmi_device *ipmi_device = acpi_driver_data(device);
> >
> > > + if (!ipmi_device)
> > > + return 0;
> >
> > No need to test this; if we got here, it means acpi_ipmi_add()
> > succeeded, and it set the driver_data pointer.
> >
> > > +
> > > + if (ipmi_device->user_interface) {
> > > + /*
> > > + * If the IPMI user interface is created, it should be
> > > + * destroyed.
> > > + */
> >
> > Superfluous comment.
> >
> > > + 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;
> >
> > No need to initialize "result".
> >
> > > +
> > > + if (acpi_disabled)
> > > + return result;
> >
> > No need to check this; acpi_bus_register_driver() does it already.
> >
> > > +
> > > + result = acpi_bus_register_driver(&acpi_ipmi_driver);
> > > +
> >
> > Remove these blank lines.
> >
> > > + 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;
> >
> > Remove this check, too.
> >
> > > +
> > > + ipmi_smi_watcher_unregister(&driver_data.bmc_events);
> > > + acpi_bus_unregister_driver(&acpi_ipmi_driver);
> > > +
> > > + return;
> >
> > No "return" statement needed here.
> >
> > > +}
> > > +
> > > +module_init(acpi_ipmi_init);
> > > +module_exit(acpi_ipmi_exit);
> > > Index: linux-2.6/drivers/acpi/Kconfig
> > > ===================================================================
> > > --- linux-2.6.orig/drivers/acpi/Kconfig 2009-09-27 14:35:38.000000000 +0800
> > > +++ linux-2.6/drivers/acpi/Kconfig 2009-09-28 13:10:46.000000000 +0800
> > > @@ -204,6 +204,18 @@
> > >
> > > 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.
> >
> > "on on" is a typo.
> >
> > > +
> > > + To compile this driver as a module, choose M here:
> >
> > Looks like you're missing some "module will be called" text here.
> Do you mean that the selected modules should also be listed?
No. I meant that normally it looks like this:
To compile this driver as a module, choose M here:
the module will be called XXX
You copied & pasted the first line, but forgot the second line that
contains the module name.
> > >
> > > config ACPI_HOTPLUG_CPU
> > > bool
> > > Index: linux-2.6/drivers/acpi/Makefile
> > > ===================================================================
> > > --- linux-2.6.orig/drivers/acpi/Makefile 2009-09-27 14:35:38.000000000 +0800
> > > +++ linux-2.6/drivers/acpi/Makefile 2009-09-28 13:09:09.000000000 +0800
> > > @@ -57,6 +57,7 @@
> > > 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
> > > --
> > > 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
> > >
> >
> >
>
------------------------------------------------------------------------------
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2009-10-09 3:03 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-09-29 5:29 [RFC Patch 1/2]IPMI/ACPI: Locate the IPMI system interface in ACPI namespace yakui.zhao
2009-09-29 5:29 ` [RFC Patch 2/2]IPMI/ACPI: Add the IPMI opregion driver to send IPMI message in AML code yakui.zhao
2009-09-30 15:37 ` Bjorn Helgaas
2009-10-09 1:59 ` ykzhao
2009-10-09 3:03 ` Bjorn Helgaas
2009-09-29 15:51 ` [RFC Patch 1/2]IPMI/ACPI: Locate the IPMI system interface in ACPI namespace Bjorn Helgaas
2009-09-30 1:57 ` ykzhao
2009-09-30 15:19 ` Bjorn Helgaas
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.