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@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.
> >
> > 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@xxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxxxxx
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > >
> >
> >
>
>
--
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
