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

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

[yakui.zhao]

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

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

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

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

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

[Bjorn Helgaas]

On Monday 16 November 2009 02:18:09 am yakui.zhao@intel.com wrote:
> From: Zhao Yakui <yakui.zhao@intel.com>

When you post a new version of a patch set, please add a "v2", "v3",
etc., so we can tell that this is a new version of something we've
seen before.

When people have commented on previous versions of your patch, please
CC: them when you post a new version.  It's also nice if you mention
what changed in your new version if you addressed any feedback.

In fact, I gave you detailed feedback on previous versions of this
patch, and you've ignored it all.

This current patch is identical to the one you posted on October 26,
except for a minor change in acpi_find_bmc().

As I've pointed out before, an ACPI driver must not use
acpi_walk_namespace() to discover its devices.  It must use
acpi_bus_register_driver().

Bjorn

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

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

[ykzhao]

On Tue, 2009-11-17 at 01:51 +0800, Bjorn Helgaas wrote:
> On Monday 16 November 2009 02:18:09 am yakui.zhao@intel.com wrote:
> > From: Zhao Yakui <yakui.zhao@intel.com>
> 
> When you post a new version of a patch set, please add a "v2", "v3",
> etc., so we can tell that this is a new version of something we've
> seen before.
> 
> When people have commented on previous versions of your patch, please
> CC: them when you post a new version.  It's also nice if you mention
> what changed in your new version if you addressed any feedback.

Thanks for pointing out this issue.
I will pay attention to this issue.
> 
> In fact, I gave you detailed feedback on previous versions of this
> patch, and you've ignored it all.

Understand what you said. You feedback contains two points.
a. Load the Pnp device driver to detect the IPMI device so that we can
use the IO/memory resource parse mechanism of PNP device. 
b. Load a ACPI device driver to detect the IPMI device

In fact I also reply the above issues in my email. And I am sorry that I
don't mention this in this patch.
For a: There exist several types of IPMI IO/memory resource definition
for IPMI device in ACPI namespace. And this is also related with the
IPMI system interface type(KCS, BT, SMIC). Sometimes there exist two
different IO/memory resource definitions for one IPMI device. In such
case it is not easy to decide which IO/memory resource should be used.
So I prefer to parse the resource directly.

For b: This patch set contains two parts.
      One is to detect and register the IPMI system interface. After the
system interface is registered, the system management software can use
this interface to manage and control IPMI device. And this had better be
put in IPMI subsystem.  
      The second part is to load the IPMI opregion space handle for the
IPMI device so that the AML code can communicate with the IPMI device.
This belongs to the ACPI subsystem. This is a ACPI device driver. 

BTW:  The register function of IPMI system interface is declared as
static. If we can call it out of IPMI subsystem, we will have to change
its prototype(The corresponding data structure should also be changed.).
IMO it is unnecessary to make so big change about it.
> 
> This current patch is identical to the one you posted on October 26,
> except for a minor change in acpi_find_bmc().

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

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

[Bjorn Helgaas]

On Monday 16 November 2009 05:59:10 pm ykzhao wrote:
> On Tue, 2009-11-17 at 01:51 +0800, Bjorn Helgaas wrote:
> > On Monday 16 November 2009 02:18:09 am yakui.zhao@intel.com wrote:
> > > From: Zhao Yakui <yakui.zhao@intel.com>
> > 
> > In fact, I gave you detailed feedback on previous versions of this
> > patch, and you've ignored it all.
> 
> Understand what you said. You feedback contains two points.
> a. Load the Pnp device driver to detect the IPMI device so that we can
> use the IO/memory resource parse mechanism of PNP device. 
> b. Load a ACPI device driver to detect the IPMI device

Yes, you're right.  I'm sorry I forgot that you *did* respond to
those.  You didn't agree, and that's fine.  I'll post a sample
patch to make my suggestion more concrete.

Bjorn

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

[ykzhao]

On Tue, 2009-11-17 at 22:59 +0800, Bjorn Helgaas wrote:
> On Monday 16 November 2009 05:59:10 pm ykzhao wrote:
> > On Tue, 2009-11-17 at 01:51 +0800, Bjorn Helgaas wrote:
> > > On Monday 16 November 2009 02:18:09 am yakui.zhao@intel.com wrote:
> > > > From: Zhao Yakui <yakui.zhao@intel.com>
> > > 
> > > In fact, I gave you detailed feedback on previous versions of this
> > > patch, and you've ignored it all.
> > 
> > Understand what you said. You feedback contains two points.
> > a. Load the Pnp device driver to detect the IPMI device so that we can
> > use the IO/memory resource parse mechanism of PNP device. 
> > b. Load a ACPI device driver to detect the IPMI device
> 
> Yes, you're right.  I'm sorry I forgot that you *did* respond to
> those.  You didn't agree, and that's fine.  I'll post a sample
> patch to make my suggestion more concrete.
Hi, Bjorn
    It is not meaningless to explain viewpoint again. And my explanation
can't make you accept my viewpoint.
    In fact this patch set contains two parts. 
   a. One is to add the support of detection IPMI system interface in
ACPI namespace, which is a complement of ACPI SPMI detection.(SPMI is
the abbreviation of Service Process Management Interface). In my patch I
use the acpi_walk_namepsace directly to detect the IPMI system interface
in ACPI namespace. I try to make two ACPI detection method depend on one
ACPI subsystem instead of the PNP subsystem or other subsystem. If you
think that this is not accepted, you can try to using other mechanism.
   b. make it possible to communicate between ACPI aml code and IPMI
subsystem so that the ACPI AML code can access the IPMI system
interface. 

    The first part belongs to the IPMI subsystem. And the second belongs
to ACPI subsystem. As this patch set belongs to the two different
subsystem, we can't assure that they are merged into upstream kernel at
the same time. In such case we will have to assure that the two patches
can work independently. 
   In my test the two patches can work well on one server with the IPMI
system interface defined ACPI namespace. And even when any patch is
applied, the system still can work.(Without the second patch the IPMI
system interface is also detected and registered correctly.  Without the
first patch it is still possible that ACPI AML code can communicate with
the IPMI subsystem.). Even when I add the boot option of "pnpacpi=off",
it still can work.

Hi, Bjorn
   If you think that you can't accept the detection mechanism in my
patch, you can go ahead.
   But you had better not touch the second patch. It is only to do the
communication between the ACPI AML code and IPMI system interface. And
it has no relationship with the IPMI system interface detection.

Thanks. 
   Yakui
> 
> Bjorn
> 
> 

IPMI device discovery [was Re: [PATCH ] IPMI: Locate the IPMI system interface in ACPI namespace]

[Bjorn Helgaas]

On Tuesday 17 November 2009 08:41:55 pm ykzhao wrote:
>    a. One is to add the support of detection IPMI system interface in
> ACPI namespace, which is a complement of ACPI SPMI detection.

This is not related to your current patch, which adds namespace
detection.  This is a new conversation about how I think we should
make future changes in IPMI device discovery.

The ACPI namespace is the canonical way to enumerate devices that
don't have a native enumeration protocol.  I think the only reason
the SPMI table exists at all is to allow an OS to find an IPMI system
interface during early boot, before the OS is prepared to parse the
ACPI namespace.  Anything described by the SPMI should also be described
in the namespace.

Linux does not need to use IPMI during that early boot phase, so I
think the SPMI detection should be dropped completely, and we should
rely *only* on the ACPI namespace and the PCI enumeration.  We may
have to use SPMI or SMBIOS to work around BIOS defects, but we
shouldn't use them in general.

I have been told that Windows is similar in that it does not use IPMI
during early boot, and that it does not look at the SPMI table at all,
so I think relying on the namespace would be fairly safe.

Bjorn

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day 
trial. Simplify your report design, integration and deployment - and focus on 
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july

Re: [Openipmi-developer] IPMI device discovery [was Re: [PATCH ] IPMI: Locate the IPMI system interface in ACPI namespace]

[Rocky Craig]

Bjorn Helgaas emitted:

> I think the only reason
> the SPMI table exists at all is to allow an OS to find an IPMI system
> interface during early boot, before the OS is prepared to parse the
> ACPI namespace. 

Correct.  This was driven by HP-UX as an extension to both ACPI and
IPMI specifications.  The HP-UX kernel is "IPMI-aware" (specifically,
writing the SEL) whereas Linux is not.

> Anything described by the SPMI should also be described in the namespace.

I believe that distinction/request/requirement was lost "back in the day".

> Linux does not need to use IPMI during that early boot phase, so I
> think the SPMI detection should be dropped completely, 

I don't know if SPMI caught on with other hardware vendors.  As it is in the
two specifications, dropping it should be given some thought.

Corey, any idea on general SPMI use?

> I have been told that Windows is similar in that it does not use IPMI
> during early boot, and that it does not look at the SPMI table at all,
> so I think relying on the namespace would be fairly safe.

Sounds like a good vote to drop.  Another vote for dropping comes from
incomplete info in the SPMI table regarding interrupt type and polarity.
Relying on SPMI almost forces you to run the driver in polled mode.

Rocky

Re: [Openipmi-developer] IPMI device discovery [was Re: [PATCH ] IPMI: Locate the IPMI system interface in ACPI namespace]

[Bjorn Helgaas]

On Wednesday 18 November 2009 01:40:57 pm Rocky Craig wrote:
> Bjorn Helgaas emitted:
> 
> > Anything described by the SPMI should also be described in the namespace.
> 
> I believe that distinction/request/requirement was lost "back in the day".

I disagree that this requirement was lost.  The ACPI spec is regrettably
vague, but I think it still contains the very general requirement that
the ACPI namespace should describe everything that cannot be found by a
standard hardware enumeration mechanism.

That requirement allows the OS to use a single coherent device
discovery, driver binding, and resource management scheme to cover
all these devices.  If we had to deal with all these devices piece-
meal, with an SPMI table here, an SPCR table there, we'd go more
mad than we already are.

There may be firmware that has an SPMI but neglects to put the device
in the namespace.  In my opinion, that's clearly a defect.  I think
it's unlikely since Windows relies on the namespace, but it's certainly
a risk.

Bjorn

Re: [Openipmi-developer] IPMI device discovery [was Re: [PATCH ] IPMI: Locate the IPMI system interface in ACPI namespace]

[Rocky Craig]

Bjorn Helgaas emitted:

> I disagree that this requirement was lost....

Sorry, my words did not convey my intended meaning, which was that
"conformance" or "coherence" was lost, with SPMI also being listed
in ACPI general namespace.

You actually stated this with greater clarity:

> There may be firmware that has an SPMI but neglects to put the device
> in the namespace.

That may be the case on some legacy servers.

Rocky

Re: [Openipmi-developer] IPMI device discovery [was Re: [PATCH ] IPMI: Locate the IPMI system interface in ACPI namespace]

[ykzhao]

On Thu, 2009-11-19 at 05:53 +0800, Rocky Craig wrote:
> Bjorn Helgaas emitted:
> 
> > I disagree that this requirement was lost....
> 
> Sorry, my words did not convey my intended meaning, which was that
> "conformance" or "coherence" was lost, with SPMI also being listed
> in ACPI general namespace.
> 
> You actually stated this with greater clarity:
> 
> > There may be firmware that has an SPMI but neglects to put the device
> > in the namespace.

> 
> That may be the case on some legacy servers.
Yes. In the IPMI 1.5 spec there is no definition of IPMI detection about
SPMI.
The SPMI is added in IPMI 2.0 spec, which is used to detect the IPMI
system interface. Maybe this mechanism is already used on some legacy
servers. 

Maybe there exist both SPMI and IPMI system interface defined in ACPI
namespace. But they are the different BMC controllers. 
In such case we had better not remove the detection of IPMI detection by
using SPMI. 
We can check whether they are the identical device in course of
registering IPMI system interface.

Thanks.


> 
> Rocky

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

RE: [Openipmi-developer] IPMI device discovery [was Re: [PATCH ] IPMI: Locate the IPMI system interface in ACPI namespace]

[Bela Lubkin]

> Yes. In the IPMI 1.5 spec there is no definition of IPMI 
> detection about SPMI.

It was added: <http://www.intel.com/design/servers/ipmi/IPMI1511E5.pdf>
defines IPMI detection via ACPI, SPMI etc.

>Bela<

> -----Original Message-----
> From: ykzhao [mailto:yakui.zhao@intel.com] 
> Sent: Thursday, November 19, 2009 3:02 AM
> To: Rocky Craig
> Cc: minyard@acm.org; linux-acpi@vger.kernel.org; Myron Stowe; 
> openipmi-developer@lists.sourceforge.net; Bjorn Helgaas; 
> lenb@kernel.org
> Subject: Re: [Openipmi-developer] IPMI device discovery [was 
> Re: [PATCH ] IPMI: Locate the IPMI system interface in ACPI namespace]
> 
> On Thu, 2009-11-19 at 05:53 +0800, Rocky Craig wrote:
> > Bjorn Helgaas emitted:
> > 
> > > I disagree that this requirement was lost....
> > 
> > Sorry, my words did not convey my intended meaning, which was that
> > "conformance" or "coherence" was lost, with SPMI also being listed
> > in ACPI general namespace.
> > 
> > You actually stated this with greater clarity:
> > 
> > > There may be firmware that has an SPMI but neglects to 
> put the device
> > > in the namespace.
> 
> > 
> > That may be the case on some legacy servers.
> Yes. In the IPMI 1.5 spec there is no definition of IPMI 
> detection about
> SPMI.
> The SPMI is added in IPMI 2.0 spec, which is used to detect the IPMI
> system interface. Maybe this mechanism is already used on some legacy
> servers. 
> 
> Maybe there exist both SPMI and IPMI system interface defined in ACPI
> namespace. But they are the different BMC controllers. 
> In such case we had better not remove the detection of IPMI 
> detection by
> using SPMI. 
> We can check whether they are the identical device in course of
> registering IPMI system interface.
> 
> Thanks.
> 
> 
> > 
> > Rocky
> 
> 
> --------------------------------------------------------------
> ----------------
> Let Crystal Reports handle the reporting - Free Crystal 
> Reports 2008 30-Day 
> trial. Simplify your report design, integration and 
> deployment - and focus on 
> what you do best, core application coding. Discover what's new with
> Crystal Reports now.  http://p.sf.net/sfu/bobj-july
> _______________________________________________
> Openipmi-developer mailing list
> Openipmi-developer@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openipmi-developer
> 

Re: [Openipmi-developer] IPMI device discovery [was Re: [PATCH ] IPMI: Locate the IPMI system interface in ACPI namespace]

[ykzhao]

On Thu, 2009-11-19 at 05:19 +0800, Bjorn Helgaas wrote:
> On Wednesday 18 November 2009 01:40:57 pm Rocky Craig wrote:
> > Bjorn Helgaas emitted:
> > 
> > > Anything described by the SPMI should also be described in the namespace.
> > 
> > I believe that distinction/request/requirement was lost "back in the day".
> 
> I disagree that this requirement was lost.  The ACPI spec is regrettably
> vague, but I think it still contains the very general requirement that
> the ACPI namespace should describe everything that cannot be found by a
> standard hardware enumeration mechanism.
The detection of IPMI system interface in ACPI namespace is not defined
in ACPI spec. Instead it is defined in IPMI 2.0 spec. The SPMI detection
is also defined in IPMI 2.0 spec. 

ACPI 4.0 spec only defines that the ACPI aml code can access the BMC
controller. 
> 
> That requirement allows the OS to use a single coherent device
> discovery, driver binding, and resource management scheme to cover
> all these devices.  If we had to deal with all these devices piece-
> meal, with an SPMI table here, an SPCR table there, we'd go more
> mad than we already are.
> 
> There may be firmware that has an SPMI but neglects to put the device
> in the namespace.  In my opinion, that's clearly a defect.  I think
> it's unlikely since Windows relies on the namespace, but it's certainly
> a risk.
> 
> Bjorn

RE: [Openipmi-developer] [PATCH ] IPMI: Locate the IPMI system interface in ACPI namespace

[Bela Lubkin]

Hi Yakui,

>    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)

> +	/* 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 SSIF is a possible value defined by the standard which specifies ACPI
identification of IPMI, please display it in the error message.  Then if
it starts appearing in the future, it will be obvious what support needs
to be added.  e.g.:

   default:
      printk(KERN_INFO "ipmi_si: ACPI/SPMI SI type %d (%s) is not supported",
             spmi->interfacetype,
             (spmi->interfacetype == ??4?? /* SSIF */) ? "SSIF" : "Unknown");

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

if any other bits are set, should this warn about them?

> +		info->irq = 0;
> +		info->irq_setup = NULL;
> +	}
>
> +	if (spmi->addr.bit_width) {
> +		/* A (hopefully) properly formed register bit width. */

should this be checked (e.g. warn if bit_width % 8 != 0)?

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

"%d", io.addr_type
(or maybe %x? -- whatever's right in context)

I'm not going to go line-by-line over the rest, but in general,
if the code thinks something is wrong, have it show what it
objects to:

> +			if (irq->interrupt_count != 1) {
> +				printk(KERN_WARNING "incorrect IRQ is "
> +					"defined in _CRS\n");

"interrupt_count is %d, must be 1", irq->interrupt_count

etc.

>Bela<