On Mon, 2009-10-26 at 23:20 +0800, Bjorn Helgaas wrote: > On Monday 26 October 2009 07:33:45 am yakui.zhao@xxxxxxxxx wrote: > > From: Zhao Yakui <yakui.zhao@xxxxxxxxx> > > > > According to the IPMI 2.0 spec the IPMI system interface can be located with > > ACPI. One is located in SPMI table(Service Processor Management Interface > > table). Another is located in ACPI namespace. > > This patch is to locate the IPMI system interface in ACPI namespace and > > register it. > > It includes the following two steps: > > 1. enumerate the ACPI device tree to find the IPMI system interface > > The IPMI device type is IPI0001. When the device is found, it > > will continue to parse the corresponding resources. > > For example: > > interface type (KCS, BT, SMIC) (SSIF is not supported) > > interrupt number and type (_GPE or GSI) > > Memory or IO base address > > 2. register the IPMI system interface > As far as I can tell, these patches are unchanged from the original > ones you posted here: > http://marc.info/?l=linux-acpi&m=125420227326811&w=2 > http://marc.info/?l=linux-acpi&m=125420227426814&w=2 I refresh them based on the latest upstream branch. Of course I do the small change about the second patch. It is mainly about the coding style. In fact you mention two issues about the two patches: 1: Load a PNP driver for it to register the IPMI system interface. This is about the first patch. 2. coding style( for example: comments, the definition about some variables). For the first issue: Before I start the first patch, I consider using the PNP device driver. But I find that it is so complex because of the following two points: 1. One is that we can't register the IPMI system interface if the boot option of "pnpacpi=off" is added. This will also depend on the PNP module. 2. The second is that there exist so many cases about the IPMI IO/memory resource definition. Maybe there exist both IO/memory resource definition for one IPMI device. In such case we can't know which should be selected. At the same time we have similar issues about the interrupt type. So I decide to parse the IO/memory/interrupt resource independently. Very sorry that I don't reply the above points in the discussion. For the second patch: I update the coding style about the comments. But I don't change the coding style about the variable definition as it doesn't matter. At the same time it is easy to understand by using the explicit definition. For example: if (acpi_disabled) return; thanks. yakui > Either you mistakenly posted the original patches again, or you > didn't address a single piece of the feedback we gave you. > > Please don't waste my time asking for comments if you're just going > to ignore them. > > It's OK if you think my suggestions are incorrect, but I expect you > to at least acknowledge them and explain why you disagree with me, so > we can have a discussion about it. > > Bjorn > > > Signed-off-by: Zhao Yakui <yakui.zhao@xxxxxxxxx> > > --- > > drivers/char/ipmi/ipmi_si_intf.c | 394 ++++++++++++++++++++++++++++++++++++++ > > 1 files changed, 394 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c > > index d2e6980..94d14bc 100644 > > --- a/drivers/char/ipmi/ipmi_si_intf.c > > +++ b/drivers/char/ipmi/ipmi_si_intf.c > > @@ -1813,6 +1813,35 @@ static __devinit void hardcode_find_bmc(void) > > * are no more. > > */ > > static int acpi_failure; > > +static LIST_HEAD(acpi_ipmi); > > + > > +struct acpi_device_ipmi { > > + struct list_head link; > > + u8 interfacetype; > > + /* > > + * Bit 0 - SCI interrupt supported > > + * Bit 1 - I/O APIC/SAPIC > > + */ > > + u8 interrupttype; > > + /* > > + * If bit 0 of InterruptType is set, then this is the SCI > > + * interrupt in the GPEx_STS register. > > + */ > > + u8 gpe; > > + /* > > + * If bit 1 of InterruptType is set, then this is the I/O > > + * APIC/SAPIC interrupt. > > + */ > > + u32 global_interrupt; > > + > > + /* The actual register address. */ > > + struct acpi_generic_address addr; > > + struct acpi_generic_address sm_addr; > > + > > + u16 ipmi_revision; > > + u8 resource_count; > > + struct device *dev; > > +}; > > > > /* For GPE-type interrupts. */ > > static u32 ipmi_acpi_gpe(void *context) > > @@ -2002,6 +2031,367 @@ static __devinit int try_init_acpi(struct SPMITable *spmi) > > return 0; > > } > > > > +static __devinit int try_init_acpi_device(struct acpi_device_ipmi *spmi) > > +{ > > + struct smi_info *info; > > + u8 addr_space; > > + > > + if (spmi->addr.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) > > + addr_space = IPMI_MEM_ADDR_SPACE; > > + else > > + addr_space = IPMI_IO_ADDR_SPACE; > > + > > + info = kzalloc(sizeof(*info), GFP_KERNEL); > > + if (!info) { > > + printk(KERN_ERR "ipmi_si: Could not allocate SI data (3)\n"); > > + return -ENOMEM; > > + } > > + > > + info->addr_source = "ACPI"; > > + > > + /* Figure out the interface type. */ > > + switch (spmi->interfacetype) { > > + case 1: /* KCS */ > > + info->si_type = SI_KCS; > > + break; > > + case 2: /* SMIC */ > > + info->si_type = SI_SMIC; > > + break; > > + case 3: /* BT */ > > + info->si_type = SI_BT; > > + break; > > + default: > > + printk(KERN_INFO "ipmi_si: Unknown ACPI/SPMI SI type %d\n", > > + spmi->interfacetype); > > + kfree(info); > > + return -EIO; > > + } > > + > > + if (spmi->interrupttype & 1) { > > + /* We've got a GPE interrupt. */ > > + info->irq = spmi->gpe; > > + info->irq_setup = acpi_gpe_irq_setup; > > + } else if (spmi->interrupttype & 2) { > > + /* We've got an APIC/SAPIC interrupt. */ > > + info->irq = spmi->global_interrupt; > > + info->irq_setup = std_irq_setup; > > + } else { > > + /* Use the default interrupt setting. */ > > + info->irq = 0; > > + info->irq_setup = NULL; > > + } > > + > > + if (spmi->addr.bit_width) { > > + /* A (hopefully) properly formed register bit width. */ > > + info->io.regspacing = spmi->addr.bit_width / 8; > > + } else { > > + info->io.regspacing = DEFAULT_REGSPACING; > > + } > > + info->io.regsize = info->io.regspacing; > > + info->io.regshift = spmi->addr.bit_offset; > > + > > + if (spmi->addr.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) { > > + info->io_setup = mem_setup; > > + info->io.addr_type = IPMI_MEM_ADDR_SPACE; > > + } else if (spmi->addr.space_id == ACPI_ADR_SPACE_SYSTEM_IO) { > > + info->io_setup = port_setup; > > + info->io.addr_type = IPMI_IO_ADDR_SPACE; > > + } else { > > + kfree(info); > > + printk(KERN_WARNING > > + "ipmi_si: Unknown ACPI I/O Address type\n"); > > + return -EIO; > > + } > > + info->io.addr_data = spmi->addr.address; > > + info->dev = spmi->dev; > > + > > + try_smi_init(info); > > + > > + return 0; > > +} > > + > > +static acpi_status > > +acpi_parse_io_ports(struct acpi_resource *resource, void *context) > > +{ > > + struct acpi_device_ipmi *p_ipmi = context; > > + > > + if (resource->type == ACPI_RESOURCE_TYPE_EXTENDED_IRQ || > > + resource->type == ACPI_RESOURCE_TYPE_IRQ) { > > + unsigned int irq_number; > > + if (p_ipmi->interrupttype) { > > + /* > > + * If it already support the interrupt through GPE, > > + * it is unnecessary to get this interrupt again. > > + */ > > + printk(KERN_WARNING "Interrupt through GPE is already" > > + " supported.\n"); > > + return AE_OK; > > + } > > + if (resource->type == ACPI_RESOURCE_TYPE_IRQ) { > > + struct acpi_resource_irq *irq; > > + irq = &resource->data.irq; > > + if (irq->interrupt_count != 1) { > > + printk(KERN_WARNING "incorrect IRQ is " > > + "defined in _CRS\n"); > > + return AE_OK; > > + } > > + irq_number = irq->interrupts[0]; > > + } else { > > + struct acpi_resource_extended_irq *extended_irq; > > + extended_irq = &resource->data.extended_irq; > > + if (extended_irq->interrupt_count != 1) { > > + printk(KERN_WARNING "incorrect IRQ is " > > + "defined in _CRS\n"); > > + return AE_OK; > > + } > > + irq_number = extended_irq->interrupts[0]; > > + } > > + p_ipmi->global_interrupt = irq_number; > > + if (p_ipmi->global_interrupt) { > > + /* GSI interrupt type */ > > + p_ipmi->interrupttype |= 0x02; > > + } > > + return AE_OK; > > + } > > + if (resource->type == ACPI_RESOURCE_TYPE_IO || > > + resource->type == ACPI_RESOURCE_TYPE_FIXED_IO) { > > + u16 address; > > + struct acpi_resource_io *io; > > + struct acpi_resource_fixed_io *fixed_io; > > + > > + fixed_io = &resource->data.fixed_io; > > + if (p_ipmi->resource_count) { > > + /* > > + * Multiply definitions of IO/memory address are > > + * obtained. It is incorrect. We will continue > > + * to use the first IO/memory definition. > > + * If not correct, please fix me. > > + */ > > + return AE_OK; > > + } > > + if (resource->type == ACPI_RESOURCE_TYPE_IO) { > > + io = &resource->data.io; > > + if (!io->minimum) { > > + /* when IO address is zero, return */ > > + return AE_OK; > > + } > > + address = io->minimum; > > + } else { > > + fixed_io = &resource->data.fixed_io; > > + if (!fixed_io->address) > > + return AE_OK; > > + address = fixed_io->address; > > + } > > + p_ipmi->resource_count++; > > + p_ipmi->addr.space_id = ACPI_ADR_SPACE_SYSTEM_IO; > > + p_ipmi->addr.address = address; > > + return AE_OK; > > + } > > + > > + if (resource->type == ACPI_RESOURCE_TYPE_MEMORY32 || > > + resource->type == ACPI_RESOURCE_TYPE_MEMORY24 || > > + resource->type == ACPI_RESOURCE_TYPE_FIXED_MEMORY32) { > > + u32 address; > > + if (p_ipmi->resource_count) { > > + /* > > + * Multiply definitions of IO/memory address are > > + * obtained. It is incorrect. We will continue > > + * to use the first IO/memory definition. > > + * If not correct, please fix me. > > + */ > > + return AE_OK; > > + } > > + if (resource->type == ACPI_RESOURCE_TYPE_MEMORY32) { > > + struct acpi_resource_memory32 *memory32; > > + memory32 = &resource->data.memory32; > > + address = memory32->minimum; > > + } else if (resource->type == ACPI_RESOURCE_TYPE_MEMORY24) { > > + struct acpi_resource_memory24 *memory24; > > + memory24 = &resource->data.memory24; > > + address = memory24->minimum; > > + } else { > > + struct acpi_resource_fixed_memory32 *fixed_memory32; > > + fixed_memory32 = &resource->data.fixed_memory32; > > + address = fixed_memory32->address; > > + } > > + p_ipmi->resource_count++; > > + p_ipmi->addr.address = (u64) address; > > + p_ipmi->addr.space_id = ACPI_ADR_SPACE_SYSTEM_MEMORY; > > + return AE_OK; > > + } > > + if (resource->type == ACPI_RESOURCE_TYPE_ADDRESS16 || > > + resource->type == ACPI_RESOURCE_TYPE_ADDRESS32 || > > + resource->type == ACPI_RESOURCE_TYPE_ADDRESS64) { > > + struct acpi_resource_address64 address64; > > + acpi_resource_to_address64(resource, &address64); > > + if (p_ipmi->resource_count) { > > + /* > > + * Multiply definitions of IO/memory address are > > + * obtained. It is incorrect. We will continue > > + * to use the first IO/memory definition. > > + * If not correct, please fix me. > > + */ > > + return AE_OK; > > + } > > + if (address64.resource_type != ACPI_MEMORY_RANGE && > > + address64.resource_type != ACPI_IO_RANGE) { > > + /* ignore the incorrect resource type */ > > + return AE_OK; > > + } > > + p_ipmi->addr.address = address64.minimum; > > + p_ipmi->resource_count++; > > + if (address64.resource_type == ACPI_MEMORY_RANGE) > > + p_ipmi->addr.space_id = ACPI_ADR_SPACE_SYSTEM_MEMORY; > > + else > > + p_ipmi->addr.space_id = ACPI_ADR_SPACE_SYSTEM_IO; > > + > > + return AE_OK; > > + } > > + > > + return AE_OK; > > +} > > + > > +/* > > + * acpi_parse_bmc_resource -- parse the BMC resources from ACPI > > + * @p_ipmi: the memory to store the BCM resource > > + * @handle: ACPI device handle > > + */ > > +static int acpi_parse_bmc_resource(struct acpi_device_ipmi *p_ipmi, > > + acpi_handle handle) > > +{ > > + int parse_ok = false; > > + unsigned long long temp_data; > > + acpi_status status; > > + > > + /* According to IPMI spec there should exist the _IFT method > > + * for the IPMI device. So when there is no _IFT, it is regarded > > + * as the incorrect BMC device and won't parse the resource again. > > + */ > > + status = acpi_evaluate_integer(handle, "_IFT", NULL, &temp_data); > > + if (ACPI_FAILURE(status)) > > + return parse_ok; > > + > > + p_ipmi->interfacetype = temp_data; > > + /* Figure out the interface type. If the interface type is not > > + * KCS/SMIC/BT, it is regared as the incorrect IPMI device. > > + * Of course the SSIF interface type is also defined, but we > > + * can't handle it. So it is not supported */ > > + switch (temp_data) { > > + case 1: /* KCS */ > > + case 2: /* SMIC */ > > + case 3: /* BT */ > > + break; > > + default: > > + printk(KERN_INFO "ipmi_si: Unknown ACPI/SPMI SI type %d\n", > > + p_ipmi->interfacetype); > > + return parse_ok; > > + } > > + /* check whether there exists the _GPE method. If it exists, it > > + * means that interrupt through GPE is supported. > > + */ > > + temp_data = 0; > > + status = acpi_evaluate_integer(handle, "_GPE", NULL, &temp_data); > > + if (ACPI_SUCCESS(status)) { > > + p_ipmi->gpe = temp_data; > > + /* set the GPE interrupt type */ > > + p_ipmi->interrupttype |= 0x01; > > + } > > + /* get the IPMI revision */ > > + temp_data = 0; > > + status = acpi_evaluate_integer(handle, "_SRV", NULL, &temp_data); > > + if (ACPI_SUCCESS(status)) > > + p_ipmi->ipmi_revision = temp_data; > > + > > + status = acpi_walk_resources(handle, METHOD_NAME__CRS, > > + acpi_parse_io_ports, p_ipmi); > > + if (ACPI_FAILURE(status)) { > > + printk(KERN_WARNING "Can't parse the _CRS object \n"); > > + return parse_ok; > > + } > > + if (!p_ipmi->resource_count) { > > + /* The incorrect IO/Memory address is parsed */ > > + printk(KERN_ERR "Incorrect IO/Memory address is parsed\n"); > > + return parse_ok; > > + } > > + parse_ok = true; > > + > > + return parse_ok; > > +} > > + > > +const struct acpi_device_id ipmi_ids[] = { > > + {"IPI0001", 0}, > > + {"", 0}, > > +}; > > + > > +/* > > + * acpi_check_bmc_device -- check whether @handle is a BMC device and then > > + * get its corresponding resource. For example: IO/Mem > > + * address, interface type > > + * @handle: ACPI device handle > > + * @level : depth in the ACPI namespace tree > > + * @context: the number of bmc device. In theory there is not more than > > + * one ACPI BMC device. > > + * @rv: a return value to fill if desired (Not use) > > + */ > > +static acpi_status > > +acpi_check_bmc_device(acpi_handle handle, u32 level, void *context, > > + void **return_value) > > +{ > > + struct acpi_device *acpi_dev; > > + struct acpi_device_ipmi *p_ipmi = NULL; > > + int *count = (int *)context; > > + > > + acpi_dev = NULL; > > + /* Get the acpi device for device handle */ > > + if (acpi_bus_get_device(handle, &acpi_dev) || !acpi_dev) { > > + /* If there is no ACPI device for handle, return */ > > + return AE_OK; > > + } > > + > > + if (acpi_match_device_ids(acpi_dev, ipmi_ids)) > > + return AE_OK; > > + > > + p_ipmi = kzalloc(sizeof(*p_ipmi), GFP_KERNEL); > > + if (!p_ipmi) { > > + printk(KERN_ERR "Can't allocate memory for IPMI device\n"); > > + return AE_OK; > > + } > > + p_ipmi->dev = &acpi_dev->dev; > > + if (!acpi_parse_bmc_resource(p_ipmi, handle)) { > > + kfree(p_ipmi); > > + } else { > > + list_add_tail(&p_ipmi->link, &acpi_ipmi); > > + *count = *count + 1; > > + } > > + > > + return AE_OK; > > +} > > + > > +static __devinit void acpi_device_find_bmc(void) > > +{ > > + acpi_status status; > > + int device_count = 0; > > + struct acpi_device_ipmi *p_ipmi, *p_ipmi2; > > + > > + if (acpi_disabled) > > + return; > > + > > + status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, > > + ACPI_UINT32_MAX, > > + acpi_check_bmc_device, &device_count, NULL); > > + if (!device_count) { > > + /* when no IPMI device is found in ACPI namespace, return */ > > + return; > > + } > > + list_for_each_entry_safe(p_ipmi, p_ipmi2, &acpi_ipmi, link) { > > + try_init_acpi_device(p_ipmi); > > + list_del(&p_ipmi->link); > > + kfree(p_ipmi); > > + } > > + > > + return; > > +} > > + > > static __devinit void acpi_find_bmc(void) > > { > > acpi_status status; > > @@ -2014,6 +2404,7 @@ static __devinit void acpi_find_bmc(void) > > if (acpi_failure) > > return; > > > > + /* locate the IPMI system interface in ACPI SPMI table */ > > for (i = 0; ; i++) { > > status = acpi_get_table(ACPI_SIG_SPMI, i+1, > > (struct acpi_table_header **)&spmi); > > @@ -2022,6 +2413,9 @@ static __devinit void acpi_find_bmc(void) > > > > try_init_acpi(spmi); > > } > > + > > + /* locate the IPMI system interface in ACPI device */ > > + acpi_device_find_bmc(); > > } > > #endif > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
