[PATCH v1 0/5] IPMI devices from ACPI namespace

[PATCH v1 0/5] IPMI devices from ACPI namespace

[Bjorn Helgaas]

Here's a sample of how I think the ACPI namespace device detection should
be done.  I think this is almost functionally equivalent to your "Locate
the IPMI system interface in ACPI namespace" patch.

The difference I'm aware of is that your patch doesn't call acpi_register_gsi()
for IRQs from the device _CRS, so I don't think those interrupts would work.
(I can't test this because I don't have a box that reports an IRQ in _CRS.)
PNPACPI takes care of this GSI registration for us.

Patches 3 & 4 are not really related to the main point here, but I left
them in because without 4, we'll report both SPMI and ACPI namespace
devices as being from "ACPI".

---

Bjorn Helgaas (5):
      PNPACPI: save struct acpi_device, not just acpi_handle
      PNP: add interface to retrieve ACPI device from a PNPACPI device
      ipmi: remove unused PCI probe code
      ipmi: refer to table as "SPMI", not "ACPI"
      ipmi: add PNP discovery (ACPI namespace via PNPACPI)


 drivers/char/ipmi/ipmi_si_intf.c |  118 +++++++++++++++++++++++++++++++++++---
 drivers/pnp/pnpacpi/core.c       |   19 ++++--
 drivers/pnp/pnpacpi/rsparser.c   |    9 ++-
 include/linux/pnp.h              |   13 ++++
 4 files changed, 141 insertions(+), 18 deletions(-)

-- 
Bjorn

[PATCH v1 1/5] PNPACPI: save struct acpi_device, not just acpi_handle

[Bjorn Helgaas]

Some drivers need to look at things in the acpi_device structure
besides the handle.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
---
 drivers/pnp/pnpacpi/core.c     |   17 ++++++++++++-----
 drivers/pnp/pnpacpi/rsparser.c |    9 ++++++---
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/pnp/pnpacpi/core.c b/drivers/pnp/pnpacpi/core.c
index 83b8b5a..b2348fc 100644
--- a/drivers/pnp/pnpacpi/core.c
+++ b/drivers/pnp/pnpacpi/core.c
@@ -80,7 +80,8 @@ static int pnpacpi_get_resources(struct pnp_dev *dev)
 
 static int pnpacpi_set_resources(struct pnp_dev *dev)
 {
-	acpi_handle handle = dev->data;
+	struct acpi_device *acpi_dev = dev->data;
+	acpi_handle handle = acpi_dev->handle;
 	struct acpi_buffer buffer;
 	int ret;
 
@@ -103,7 +104,8 @@ static int pnpacpi_set_resources(struct pnp_dev *dev)
 
 static int pnpacpi_disable_resources(struct pnp_dev *dev)
 {
-	acpi_handle handle = dev->data;
+	struct acpi_device *acpi_dev = dev->data;
+	acpi_handle handle = acpi_dev->handle;
 	int ret;
 
 	dev_dbg(&dev->dev, "disable resources\n");
@@ -121,6 +123,8 @@ static int pnpacpi_disable_resources(struct pnp_dev *dev)
 #ifdef CONFIG_ACPI_SLEEP
 static int pnpacpi_suspend(struct pnp_dev *dev, pm_message_t state)
 {
+	struct acpi_device *acpi_dev = dev->data;
+	acpi_handle handle = acpi_dev->handle;
 	int power_state;
 
 	power_state = acpi_pm_device_sleep_state(&dev->dev, NULL);
@@ -128,12 +132,15 @@ static int pnpacpi_suspend(struct pnp_dev *dev, pm_message_t state)
 		power_state = (state.event == PM_EVENT_ON) ?
 				ACPI_STATE_D0 : ACPI_STATE_D3;
 
-	return acpi_bus_set_power((acpi_handle) dev->data, power_state);
+	return acpi_bus_set_power(handle, power_state);
 }
 
 static int pnpacpi_resume(struct pnp_dev *dev)
 {
-	return acpi_bus_set_power((acpi_handle) dev->data, ACPI_STATE_D0);
+	struct acpi_device *acpi_dev = dev->data;
+	acpi_handle handle = acpi_dev->handle;
+
+	return acpi_bus_set_power(handle, ACPI_STATE_D0);
 }
 #endif
 
@@ -168,7 +175,7 @@ static int __init pnpacpi_add_device(struct acpi_device *device)
 	if (!dev)
 		return -ENOMEM;
 
-	dev->data = device->handle;
+	dev->data = device;
 	/* .enabled means the device can decode the resources */
 	dev->active = device->status.enabled;
 	status = acpi_get_handle(device->handle, "_SRS", &temp);
diff --git a/drivers/pnp/pnpacpi/rsparser.c b/drivers/pnp/pnpacpi/rsparser.c
index ef3a2cd..5702b2c 100644
--- a/drivers/pnp/pnpacpi/rsparser.c
+++ b/drivers/pnp/pnpacpi/rsparser.c
@@ -465,7 +465,8 @@ static acpi_status pnpacpi_allocated_resource(struct acpi_resource *res,
 
 int pnpacpi_parse_allocated_resource(struct pnp_dev *dev)
 {
-	acpi_handle handle = dev->data;
+	struct acpi_device *acpi_dev = dev->data;
+	acpi_handle handle = acpi_dev->handle;
 	acpi_status status;
 
 	pnp_dbg(&dev->dev, "parse allocated resources\n");
@@ -773,7 +774,8 @@ static __init acpi_status pnpacpi_option_resource(struct acpi_resource *res,
 
 int __init pnpacpi_parse_resource_option_data(struct pnp_dev *dev)
 {
-	acpi_handle handle = dev->data;
+	struct acpi_device *acpi_dev = dev->data;
+	acpi_handle handle = acpi_dev->handle;
 	acpi_status status;
 	struct acpipnp_parse_option_s parse_data;
 
@@ -845,7 +847,8 @@ static acpi_status pnpacpi_type_resources(struct acpi_resource *res, void *data)
 int pnpacpi_build_resource_template(struct pnp_dev *dev,
 				    struct acpi_buffer *buffer)
 {
-	acpi_handle handle = dev->data;
+	struct acpi_device *acpi_dev = dev->data;
+	acpi_handle handle = acpi_dev->handle;
 	struct acpi_resource *resource;
 	int res_cnt = 0;
 	acpi_status status;

[PATCH v1 2/5] PNP: add interface to retrieve ACPI device from a PNPACPI device

[Bjorn Helgaas]

Add pnp_acpi_device(pnp_dev), which takes a PNP device and returns the
associated ACPI device (or NULL, if the device is not a PNPACPI device).

This allows us to write a PNP driver that can manage both traditional
PNPBIOS and ACPI devices, treating ACPI-only functionality as an optional
extension.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
---
 drivers/pnp/pnpacpi/core.c |    2 +-
 include/linux/pnp.h        |   13 +++++++++++++
 2 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/drivers/pnp/pnpacpi/core.c b/drivers/pnp/pnpacpi/core.c
index b2348fc..8dd0f37 100644
--- a/drivers/pnp/pnpacpi/core.c
+++ b/drivers/pnp/pnpacpi/core.c
@@ -144,7 +144,7 @@ static int pnpacpi_resume(struct pnp_dev *dev)
 }
 #endif
 
-static struct pnp_protocol pnpacpi_protocol = {
+struct pnp_protocol pnpacpi_protocol = {
 	.name	 = "Plug and Play ACPI",
 	.get	 = pnpacpi_get_resources,
 	.set	 = pnpacpi_set_resources,
diff --git a/include/linux/pnp.h b/include/linux/pnp.h
index fddfafa..7c4193e 100644
--- a/include/linux/pnp.h
+++ b/include/linux/pnp.h
@@ -334,6 +334,19 @@ extern struct pnp_protocol pnpbios_protocol;
 #define pnp_device_is_pnpbios(dev) 0
 #endif
 
+#ifdef CONFIG_PNPACPI
+extern struct pnp_protocol pnpacpi_protocol;
+
+static inline struct acpi_device *pnp_acpi_device(struct pnp_dev *dev)
+{
+	if (dev->protocol == &pnpacpi_protocol)
+		return dev->data;
+	return NULL;
+}
+#else
+#define pnp_acpi_device(dev) 0
+#endif
+
 /* status */
 #define PNP_READY		0x0000
 #define PNP_ATTACHED		0x0001

[PATCH v1 3/5] ipmi: remove unused PCI probe code

[Bjorn Helgaas]

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
---
 drivers/char/ipmi/ipmi_si_intf.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index d2e6980..aaf6ead 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -2202,7 +2202,6 @@ static int __devinit ipmi_pci_probe(struct pci_dev *pdev,
 	int rv;
 	int class_type = pdev->class & PCI_ERMC_CLASSCODE_TYPE_MASK;
 	struct smi_info *info;
-	int first_reg_offset = 0;
 
 	info = kzalloc(sizeof(*info), GFP_KERNEL);
 	if (!info)
@@ -2241,9 +2240,6 @@ static int __devinit ipmi_pci_probe(struct pci_dev *pdev,
 	info->addr_source_cleanup = ipmi_pci_cleanup;
 	info->addr_source_data = pdev;
 
-	if (pdev->subsystem_vendor == PCI_HP_VENDOR_ID)
-		first_reg_offset = 1;
-
 	if (pci_resource_flags(pdev, 0) & IORESOURCE_IO) {
 		info->io_setup = port_setup;
 		info->io.addr_type = IPMI_IO_ADDR_SPACE;

Re: [PATCH v1 3/5] ipmi: remove unused PCI probe coded

[Corey Minyard]

On Tue, Nov 17, 2009 at 05:05:24PM -0700, Bjorn Helgaas wrote:
> --- a/drivers/char/ipmi/ipmi_si_intf.c
> +++ b/drivers/char/ipmi/ipmi_si_intf.c
> @@ -2202,7 +2202,6 @@ static int __devinit ipmi_pci_probe(struct pci_dev *pdev,
>  	int rv;
>  	int class_type = pdev->class & PCI_ERMC_CLASSCODE_TYPE_MASK;
>  	struct smi_info *info;
> -	int first_reg_offset = 0;
>  
>  	info = kzalloc(sizeof(*info), GFP_KERNEL);
>  	if (!info)
> @@ -2241,9 +2240,6 @@ static int __devinit ipmi_pci_probe(struct pci_dev *pdev,
>  	info->addr_source_cleanup = ipmi_pci_cleanup;
>  	info->addr_source_data = pdev;
>  
> -	if (pdev->subsystem_vendor == PCI_HP_VENDOR_ID)
> -		first_reg_offset = 1;
> -
>  	if (pci_resource_flags(pdev, 0) & IORESOURCE_IO) {
>  		info->io_setup = port_setup;
>  		info->io.addr_type = IPMI_IO_ADDR_SPACE;
> 

Unfortunately, the above patch points to some missing code later, not dead
code.  The patch that follows will set it back to the original function.
Since no one has noticed, it may be best to remove the code, but as far
as I know, that HP system is the only one that uses PCI.

I looked over the other patches in this series and they look fine.


On a PCI update, the offset for HP PCI interfaces to the IPMI controller
was left off.  Add the offset back in.

Signed-off-by: Corey Minyard <cminyard@mvista.com>

Index: linux-2.6.30/drivers/char/ipmi/ipmi_si_intf.c
===================================================================
--- linux-2.6.30.orig/drivers/char/ipmi/ipmi_si_intf.c
+++ linux-2.6.30/drivers/char/ipmi/ipmi_si_intf.c
@@ -2293,7 +2293,7 @@ static int __devinit ipmi_pci_probe(stru
 		info->io_setup = mem_setup;
 		info->io.addr_type = IPMI_MEM_ADDR_SPACE;
 	}
-	info->io.addr_data = pci_resource_start(pdev, 0);
+	info->io.addr_data = pci_resource_start(pdev, 0) + first_reg_offset;
 
 	info->io.regspacing = DEFAULT_REGSPACING;
 	info->io.regsize = DEFAULT_REGSPACING;

Re: [PATCH v1 3/5] ipmi: remove unused PCI probe coded

[Bjorn Helgaas]

On Tuesday 01 December 2009 04:18:41 pm Corey Minyard wrote:
> On Tue, Nov 17, 2009 at 05:05:24PM -0700, Bjorn Helgaas wrote:
> > --- a/drivers/char/ipmi/ipmi_si_intf.c
> > +++ b/drivers/char/ipmi/ipmi_si_intf.c
> > @@ -2202,7 +2202,6 @@ static int __devinit ipmi_pci_probe(struct pci_dev *pdev,
> >  	int rv;
> >  	int class_type = pdev->class & PCI_ERMC_CLASSCODE_TYPE_MASK;
> >  	struct smi_info *info;
> > -	int first_reg_offset = 0;
> >  
> >  	info = kzalloc(sizeof(*info), GFP_KERNEL);
> >  	if (!info)
> > @@ -2241,9 +2240,6 @@ static int __devinit ipmi_pci_probe(struct pci_dev *pdev,
> >  	info->addr_source_cleanup = ipmi_pci_cleanup;
> >  	info->addr_source_data = pdev;
> >  
> > -	if (pdev->subsystem_vendor == PCI_HP_VENDOR_ID)
> > -		first_reg_offset = 1;
> > -
> >  	if (pci_resource_flags(pdev, 0) & IORESOURCE_IO) {
> >  		info->io_setup = port_setup;
> >  		info->io.addr_type = IPMI_IO_ADDR_SPACE;
> > 
> 
> Unfortunately, the above patch points to some missing code later, not dead
> code.  The patch that follows will set it back to the original function.
> Since no one has noticed, it may be best to remove the code, but as far
> as I know, that HP system is the only one that uses PCI.

That's what we get for sticking an unrelated cleanup in the middle
of this series :-).

> I looked over the other patches in this series and they look fine.

Great, thanks!

> On a PCI update, the offset for HP PCI interfaces to the IPMI controller
> was left off.  Add the offset back in.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> 
> Index: linux-2.6.30/drivers/char/ipmi/ipmi_si_intf.c
> ===================================================================
> --- linux-2.6.30.orig/drivers/char/ipmi/ipmi_si_intf.c
> +++ linux-2.6.30/drivers/char/ipmi/ipmi_si_intf.c
> @@ -2293,7 +2293,7 @@ static int __devinit ipmi_pci_probe(stru
>  		info->io_setup = mem_setup;
>  		info->io.addr_type = IPMI_MEM_ADDR_SPACE;
>  	}
> -	info->io.addr_data = pci_resource_start(pdev, 0);
> +	info->io.addr_data = pci_resource_start(pdev, 0) + first_reg_offset;
>  
>  	info->io.regspacing = DEFAULT_REGSPACING;
>  	info->io.regsize = DEFAULT_REGSPACING;
> 

I guess you're referring to b0defcdbd2b7d?

Prior to that commit, we did this:

	int              fe_rmc = 0;
	...
              if (pci_dev && (pci_dev->subsystem_vendor == PCI_HP_VENDOR_ID))
                      fe_rmc = 1;
	...
        if (! fe_rmc)
              /* Data register starts at base address + 1 in eRMC */
              ++base_addr;
	...
	if (! is_new_interface(-1, IPMI_IO_ADDR_SPACE, base_addr)) {

Your patch above reverses the sense of this adjustment -- the old code
increments the base for everything *except* HP, while the new code
increments the base for *only* HP.

The original 5-patch series leaves the PCI base address alone.  That's
the same as the old behavior for HP devices, and we verified that it
works on an HP DL380G6 by disabling SMBIOS/SMPI/PNP detection.  (We
also verified that, as you would expect, it did NOT work if we increment
the base address).

Using the address straight from the PCI BAR, we located the IPMI interface
correctly, and we were able to exercise it with ipmitool:

    ipmi message handler version 39.2
    ipmi device interface
    IPMI System Interface driver.
    ACPI: PCI Interrupt Link [LNKF] enabled at IRQ 10
    PCI: setting IRQ 10 as level-triggered
    ipmi_si 0000:01:04.6: PCI INT A -> Link[LNKF] -> GSI 10 (level, low) -> IRQ 10
    ipmi_si: Trying PCI-specified kcs state machine at mem address 0xf1ef0000, slave address 0x0, irq 10
    IRQ 10/ipmi_si: IRQF_DISABLED is not guaranteed on shared IRQs
      Using irq 10
    ipmi: Found new BMC (man_id: 0x00000b,  prod_id: 0x0000, dev_id: 0x11)
    IPMI kcs interface initialized

    dl380g6a:~# ipmitool sensor
    UID Light        | 0x0        | discrete   | 0x0080| na        | na        | na        | na        | na        | na
    Sys. Health LED  | 0x0        | discrete   | 0x0080| na        | na        | na        | na        | na        | na
    ...

So the question is what to do about non-HP PCI IPMI interfaces.  The
pre-b0defcdbd2b7d code increments the base address, but that's been
gone for several years.  Since we've had no complaints, and we don't
know about any non-HP PCI interfaces, I propose that we just remove
that HP-specific adjustment completely, i.e., use this series as-is.

Bjorn

RE: [PATCH v1 3/5] ipmi: remove unused PCI probe coded

[Bela Lubkin]

Bjorn Helgaas wrote:

> The original 5-patch series leaves the PCI base address alone.  That's
> the same as the old behavior for HP devices, and we verified that it
> works on an HP DL380G6 by disabling SMBIOS/SMPI/PNP detection.  (We
> also verified that, as you would expect, it did NOT work if we increment
> the base address).
...
> So the question is what to do about non-HP PCI IPMI interfaces.  The
> pre-b0defcdbd2b7d code increments the base address, but that's been
> gone for several years.  Since we've had no complaints, and we don't
> know about any non-HP PCI interfaces, I propose that we just remove
> that HP-specific adjustment completely, i.e., use this series as-is.

Much older HP systems had a PCI SMIC interface.  Are you
sure those haven't been broken somewhere along the way?

(Please don't say "we don't care about things that old".)

>Bela<

Re: [PATCH v1 3/5] ipmi: remove unused PCI probe coded

[Corey Minyard]

Bela Lubkin wrote:
> Bjorn Helgaas wrote:
>
>   
>> The original 5-patch series leaves the PCI base address alone.  That's
>> the same as the old behavior for HP devices, and we verified that it
>> works on an HP DL380G6 by disabling SMBIOS/SMPI/PNP detection.  (We
>> also verified that, as you would expect, it did NOT work if we increment
>> the base address).
>>     
> ...
>   
>> So the question is what to do about non-HP PCI IPMI interfaces.  The
>> pre-b0defcdbd2b7d code increments the base address, but that's been
>> gone for several years.  Since we've had no complaints, and we don't
>> know about any non-HP PCI interfaces, I propose that we just remove
>> that HP-specific adjustment completely, i.e., use this series as-is.
>>     
>
> Much older HP systems had a PCI SMIC interface.  Are you
> sure those haven't been broken somewhere along the way?
>
> (Please don't say "we don't care about things that old".)
>   
Well, I do care, but if it's HP, it will have the same PCI vendor id, so 
it should be the same.  The change that Bjorn proposed won't actually 
change anything in the current kernel, it will just remove some dead 
code.  So I think everything is ok.  If the older HP systems won't work, 
we actually haven't changed anything, but I'll take a patch to fix them.

-corey

Re: [PATCH v1 3/5] ipmi: remove unused PCI probe coded

[Bjorn Helgaas]

On Wednesday 02 December 2009 02:04:09 pm Bela Lubkin wrote:
> Bjorn Helgaas wrote:
> 
> > The original 5-patch series leaves the PCI base address alone.  That's
> > the same as the old behavior for HP devices, and we verified that it
> > works on an HP DL380G6 by disabling SMBIOS/SMPI/PNP detection.  (We
> > also verified that, as you would expect, it did NOT work if we increment
> > the base address).
> ...
> > So the question is what to do about non-HP PCI IPMI interfaces.  The
> > pre-b0defcdbd2b7d code increments the base address, but that's been
> > gone for several years.  Since we've had no complaints, and we don't
> > know about any non-HP PCI interfaces, I propose that we just remove
> > that HP-specific adjustment completely, i.e., use this series as-is.
> 
> Much older HP systems had a PCI SMIC interface.  Are you
> sure those haven't been broken somewhere along the way?
> 
> (Please don't say "we don't care about things that old".)

I want to make all machines, including old ones, work :-)

Restoring the behavior of incrementing the base address for HP
interfaces *might* fix an old machine that has been broken since
mid 2006 (when b0defcdbd2b7d went in), but we *know* it would
break current machines.

At this point, I think the only real option is to preserve the
behavior we've had since mid 2006 and fix old machines if and
when we discover them.

Bjorn

Re: [PATCH v1 3/5] ipmi: remove unused PCI probe coded

[Bjorn Helgaas]

On Wednesday 02 December 2009 02:04:09 pm Bela Lubkin wrote:
> Much older HP systems had a PCI SMIC interface.  Are you
> sure those haven't been broken somewhere along the way?

It looks like this would be a 103c:121a NetServer SMIC Controller,
which Google thinks was used in LT 6000r and LH 3000 HP NetServers.

Unfortunately, those are pretty ancient boxes, and I haven't been
able to dig one up to play with it.  If anybody can find one, I'll
be happy to do whatever we need to get that SMIC controller working.

Bjorn

Re: [PATCH v1 3/5] ipmi: remove unused PCI probe coded

[Corey Minyard]

Bjorn Helgaas wrote:
> I guess you're referring to b0defcdbd2b7d?
>
> Prior to that commit, we did this:
>
> 	int              fe_rmc = 0;
> 	...
>               if (pci_dev && (pci_dev->subsystem_vendor == PCI_HP_VENDOR_ID))
>                       fe_rmc = 1;
> 	...
>         if (! fe_rmc)
>               /* Data register starts at base address + 1 in eRMC */
>               ++base_addr;
> 	...
> 	if (! is_new_interface(-1, IPMI_IO_ADDR_SPACE, base_addr)) {
>
> Your patch above reverses the sense of this adjustment -- the old code
> increments the base for everything *except* HP, while the new code
> increments the base for *only* HP.
>   
You are correct.

> The original 5-patch series leaves the PCI base address alone.  That's
> the same as the old behavior for HP devices, and we verified that it
> works on an HP DL380G6 by disabling SMBIOS/SMPI/PNP detection.  (We
> also verified that, as you would expect, it did NOT work if we increment
> the base address).
>   
Ok, then your patch is fine as stands.

> Using the address straight from the PCI BAR, we located the IPMI interface
> correctly, and we were able to exercise it with ipmitool:
>
>     ipmi message handler version 39.2
>     ipmi device interface
>     IPMI System Interface driver.
>     ACPI: PCI Interrupt Link [LNKF] enabled at IRQ 10
>     PCI: setting IRQ 10 as level-triggered
>     ipmi_si 0000:01:04.6: PCI INT A -> Link[LNKF] -> GSI 10 (level, low) -> IRQ 10
>     ipmi_si: Trying PCI-specified kcs state machine at mem address 0xf1ef0000, slave address 0x0, irq 10
>     IRQ 10/ipmi_si: IRQF_DISABLED is not guaranteed on shared IRQs
>       Using irq 10
>     ipmi: Found new BMC (man_id: 0x00000b,  prod_id: 0x0000, dev_id: 0x11)
>     IPMI kcs interface initialized
>
>     dl380g6a:~# ipmitool sensor
>     UID Light        | 0x0        | discrete   | 0x0080| na        | na        | na        | na        | na        | na
>     Sys. Health LED  | 0x0        | discrete   | 0x0080| na        | na        | na        | na        | na        | na
>     ...
>
> So the question is what to do about non-HP PCI IPMI interfaces.  The
> pre-b0defcdbd2b7d code increments the base address, but that's been
> gone for several years.  Since we've had no complaints, and we don't
> know about any non-HP PCI interfaces, I propose that we just remove
> that HP-specific adjustment completely, i.e., use this series as-is.
>   
To tell you the truth, I don't think there are any.  If there are, no 
one has complained.

So I'm happy with your original patch.

-corey

[PATCH v1 4/5] ipmi: refer to table as "SPMI", not "ACPI"

[Bjorn Helgaas]

This discovery method uses the SPMI table, not the ACPI namespace.  In
the future, we will look in the namespace, so let's refer to the table
as "SPMI" and save "ACPI" for the namespace.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
---
 drivers/char/ipmi/ipmi_si_intf.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index aaf6ead..b58e587 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -1919,7 +1919,7 @@ struct SPMITable {
 	s8      spmi_id[1]; /* A '\0' terminated array starts here. */
 };
 
-static __devinit int try_init_acpi(struct SPMITable *spmi)
+static __devinit int try_init_spmi(struct SPMITable *spmi)
 {
 	struct smi_info  *info;
 	u8 		 addr_space;
@@ -1940,7 +1940,7 @@ static __devinit int try_init_acpi(struct SPMITable *spmi)
 		return -ENOMEM;
 	}
 
-	info->addr_source = "ACPI";
+	info->addr_source = "SPMI";
 
 	/* Figure out the interface type. */
 	switch (spmi->InterfaceType) {
@@ -2002,7 +2002,7 @@ static __devinit int try_init_acpi(struct SPMITable *spmi)
 	return 0;
 }
 
-static __devinit void acpi_find_bmc(void)
+static __devinit void spmi_find_bmc(void)
 {
 	acpi_status      status;
 	struct SPMITable *spmi;
@@ -2020,7 +2020,7 @@ static __devinit void acpi_find_bmc(void)
 		if (status != AE_OK)
 			return;
 
-		try_init_acpi(spmi);
+		try_init_spmi(spmi);
 	}
 }
 #endif
@@ -3104,7 +3104,7 @@ static __devinit int init_ipmi_si(void)
 #endif
 
 #ifdef CONFIG_ACPI
-	acpi_find_bmc();
+	spmi_find_bmc();
 #endif
 
 #ifdef CONFIG_PCI

[PATCH v1 5/5] ipmi: add PNP discovery (ACPI namespace via PNPACPI)

[Bjorn Helgaas]

This allows ipmi_si_intf.c to claim IPMI devices described in the ACPI
namespace.  Using PNP makes it simpler to parse the IRQ/IO/memory resources
of the device.

We look at any SPMI tables before looking for devices in the namespace.

This is based on ipmi_pci_probe().

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
Signed-off-by: Myron Stowe <myron.stowe@hp.com>
---
 drivers/char/ipmi/ipmi_si_intf.c |  104 ++++++++++++++++++++++++++++++++++++++
 1 files changed, 104 insertions(+), 0 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index b58e587..679cd08 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -64,6 +64,7 @@
 #include <linux/dmi.h>
 #include <linux/string.h>
 #include <linux/ctype.h>
+#include <linux/pnp.h>
 
 #ifdef CONFIG_PPC_OF
 #include <linux/of_device.h>
@@ -2023,6 +2024,103 @@ static __devinit void spmi_find_bmc(void)
 		try_init_spmi(spmi);
 	}
 }
+
+static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
+				    const struct pnp_device_id *dev_id)
+{
+	struct acpi_device *acpi_dev;
+	struct smi_info *info;
+	acpi_handle handle;
+	acpi_status status;
+	unsigned long long tmp;
+
+	acpi_dev = pnp_acpi_device(dev);
+	if (!acpi_dev)
+		return -ENODEV;
+
+	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	info->addr_source = "ACPI";
+
+	handle = acpi_dev->handle;
+
+	/* _IFT tells us the interface type: KCS, BT, etc */
+	status = acpi_evaluate_integer(handle, "_IFT", NULL, &tmp);
+	if (ACPI_FAILURE(status))
+		goto err_free;
+
+	switch (tmp) {
+	case 1:
+		info->si_type = SI_KCS;
+		break;
+	case 2:
+		info->si_type = SI_SMIC;
+		break;
+	case 3:
+		info->si_type = SI_BT;
+		break;
+	default:
+		dev_info(&dev->dev, "unknown interface type %lld\n", tmp);
+		goto err_free;
+	}
+
+	if (pnp_port_valid(dev, 0)) {
+		info->io_setup = port_setup;
+		info->io.addr_type = IPMI_IO_ADDR_SPACE;
+		info->io.addr_data = pnp_port_start(dev, 0);
+	} else if (pnp_mem_valid(dev, 0)) {
+		info->io_setup = mem_setup;
+		info->io.addr_type = IPMI_MEM_ADDR_SPACE;
+		info->io.addr_data = pnp_mem_start(dev, 0);
+	} else {
+		dev_err(&dev->dev, "no I/O or memory address\n");
+		goto err_free;
+	}
+
+	info->io.regspacing = DEFAULT_REGSPACING;
+	info->io.regsize = DEFAULT_REGSPACING;
+	info->io.regshift = 0;
+
+	/* If _GPE exists, use it; otherwise use standard interrupts */
+	status = acpi_evaluate_integer(handle, "_GPE", NULL, &tmp);
+	if (ACPI_SUCCESS(status)) {
+		info->irq = tmp;
+		info->irq_setup = acpi_gpe_irq_setup;
+	} else if (pnp_irq_valid(dev, 0)) {
+		info->irq = pnp_irq(dev, 0);
+		info->irq_setup = std_irq_setup;
+	}
+
+	info->dev = &acpi_dev->dev;
+	pnp_set_drvdata(dev, info);
+
+	return try_smi_init(info);
+
+err_free:
+	kfree(info);
+	return -EINVAL;
+}
+
+static void __devexit ipmi_pnp_remove(struct pnp_dev *dev)
+{
+	struct smi_info *info = pnp_get_drvdata(dev);
+
+	cleanup_one_si(info);
+}
+
+static const struct pnp_device_id pnp_dev_table[] = {
+	{"IPI0001", 0},
+	{"", 0},
+};
+
+static struct pnp_driver ipmi_pnp_driver = {
+	.name		= DEVICE_NAME,
+	.probe		= ipmi_pnp_probe,
+	.remove		= __devexit_p(ipmi_pnp_remove),
+	.id_table	= pnp_dev_table,
+};
 #endif
 
 #ifdef CONFIG_DMI
@@ -3106,6 +3204,9 @@ static __devinit int init_ipmi_si(void)
 #ifdef CONFIG_ACPI
 	spmi_find_bmc();
 #endif
+#ifdef CONFIG_PNP
+	pnp_register_driver(&ipmi_pnp_driver);
+#endif
 
 #ifdef CONFIG_PCI
 	rv = pci_register_driver(&ipmi_pci_driver);
@@ -3229,6 +3330,9 @@ static __exit void cleanup_ipmi_si(void)
 #ifdef CONFIG_PCI
 	pci_unregister_driver(&ipmi_pci_driver);
 #endif
+#ifdef CONFIG_PNP
+	pnp_unregister_driver(&ipmi_pnp_driver);
+#endif
 
 #ifdef CONFIG_PPC_OF
 	of_unregister_platform_driver(&ipmi_of_platform_driver);

Re: [PATCH v1 5/5] ipmi: add PNP discovery (ACPI namespace via PNPACPI)

[ykzhao]

On Wed, 2009-11-18 at 08:05 +0800, Bjorn Helgaas wrote:
> This allows ipmi_si_intf.c to claim IPMI devices described in the ACPI
> namespace.  Using PNP makes it simpler to parse the IRQ/IO/memory resources
> of the device.

Hi, Bjorn
    Will you please try to push this patch set that detects the IPMI
device defined in ACPI namespace by using Pnp device driver? This is
related with the IPMI subsystem.
    I test it on one server. It can detect the IPMI system interface
defined in ACPI namespace correctly. And it can also work correctly with
the my second patch that enables the ACPI AML code to communicate with
the IPMI device.  

    A small concern is whether we can complain some warning message when
there exist both the GPE and interrupt for the IPMI device. According to
the IPMI 2.0 spec this is disallowed to define both the GPE and
interrupt irq for the same IPMI system interface.

Thanks.
   Yakui

> 
> We look at any SPMI tables before looking for devices in the namespace.
> 
> This is based on ipmi_pci_probe().
> 
> Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> Signed-off-by: Myron Stowe <myron.stowe@hp.com>
> ---
>  drivers/char/ipmi/ipmi_si_intf.c |  104 ++++++++++++++++++++++++++++++++++++++
>  1 files changed, 104 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> index b58e587..679cd08 100644
> --- a/drivers/char/ipmi/ipmi_si_intf.c
> +++ b/drivers/char/ipmi/ipmi_si_intf.c
> @@ -64,6 +64,7 @@
>  #include <linux/dmi.h>
>  #include <linux/string.h>
>  #include <linux/ctype.h>
> +#include <linux/pnp.h>
>  
>  #ifdef CONFIG_PPC_OF
>  #include <linux/of_device.h>
> @@ -2023,6 +2024,103 @@ static __devinit void spmi_find_bmc(void)
>  		try_init_spmi(spmi);
>  	}
>  }
> +
> +static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
> +				    const struct pnp_device_id *dev_id)
> +{
> +	struct acpi_device *acpi_dev;
> +	struct smi_info *info;
> +	acpi_handle handle;
> +	acpi_status status;
> +	unsigned long long tmp;
> +
> +	acpi_dev = pnp_acpi_device(dev);
> +	if (!acpi_dev)
> +		return -ENODEV;
> +
> +	info = kzalloc(sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +
> +	info->addr_source = "ACPI";
> +
> +	handle = acpi_dev->handle;
> +
> +	/* _IFT tells us the interface type: KCS, BT, etc */
> +	status = acpi_evaluate_integer(handle, "_IFT", NULL, &tmp);
> +	if (ACPI_FAILURE(status))
> +		goto err_free;
> +
> +	switch (tmp) {
> +	case 1:
> +		info->si_type = SI_KCS;
> +		break;
> +	case 2:
> +		info->si_type = SI_SMIC;
> +		break;
> +	case 3:
> +		info->si_type = SI_BT;
> +		break;
> +	default:
> +		dev_info(&dev->dev, "unknown interface type %lld\n", tmp);
> +		goto err_free;
> +	}
> +
> +	if (pnp_port_valid(dev, 0)) {
> +		info->io_setup = port_setup;
> +		info->io.addr_type = IPMI_IO_ADDR_SPACE;
> +		info->io.addr_data = pnp_port_start(dev, 0);
> +	} else if (pnp_mem_valid(dev, 0)) {
> +		info->io_setup = mem_setup;
> +		info->io.addr_type = IPMI_MEM_ADDR_SPACE;
> +		info->io.addr_data = pnp_mem_start(dev, 0);
> +	} else {
> +		dev_err(&dev->dev, "no I/O or memory address\n");
> +		goto err_free;
> +	}
> +
> +	info->io.regspacing = DEFAULT_REGSPACING;
> +	info->io.regsize = DEFAULT_REGSPACING;
> +	info->io.regshift = 0;
> +
> +	/* If _GPE exists, use it; otherwise use standard interrupts */
> +	status = acpi_evaluate_integer(handle, "_GPE", NULL, &tmp);
> +	if (ACPI_SUCCESS(status)) {
> +		info->irq = tmp;
> +		info->irq_setup = acpi_gpe_irq_setup;

Had better complain some warning message when the interrupt irq is found
for IPMI system interface.
> +	} else if (pnp_irq_valid(dev, 0)) {
> +		info->irq = pnp_irq(dev, 0);
> +		info->irq_setup = std_irq_setup;
> +	}
> +
> +	info->dev = &acpi_dev->dev;
> +	pnp_set_drvdata(dev, info);
> +
> +	return try_smi_init(info);
> +
> +err_free:
> +	kfree(info);
> +	return -EINVAL;
> +}
> +
> +static void __devexit ipmi_pnp_remove(struct pnp_dev *dev)
> +{
> +	struct smi_info *info = pnp_get_drvdata(dev);
> +
> +	cleanup_one_si(info);
> +}
> +
> +static const struct pnp_device_id pnp_dev_table[] = {
> +	{"IPI0001", 0},
> +	{"", 0},
> +};
> +
> +static struct pnp_driver ipmi_pnp_driver = {
> +	.name		= DEVICE_NAME,
> +	.probe		= ipmi_pnp_probe,
> +	.remove		= __devexit_p(ipmi_pnp_remove),
> +	.id_table	= pnp_dev_table,
> +};
>  #endif
>  
>  #ifdef CONFIG_DMI
> @@ -3106,6 +3204,9 @@ static __devinit int init_ipmi_si(void)
>  #ifdef CONFIG_ACPI
>  	spmi_find_bmc();
>  #endif
> +#ifdef CONFIG_PNP
> +	pnp_register_driver(&ipmi_pnp_driver);
> +#endif
>  
>  #ifdef CONFIG_PCI
>  	rv = pci_register_driver(&ipmi_pci_driver);
> @@ -3229,6 +3330,9 @@ static __exit void cleanup_ipmi_si(void)
>  #ifdef CONFIG_PCI
>  	pci_unregister_driver(&ipmi_pci_driver);
>  #endif
> +#ifdef CONFIG_PNP
> +	pnp_unregister_driver(&ipmi_pnp_driver);
> +#endif
>  
>  #ifdef CONFIG_PPC_OF
>  	of_unregister_platform_driver(&ipmi_of_platform_driver);
> 
> --
> 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 v1 0/5] IPMI devices from ACPI namespace

[ykzhao]

On Wed, 2009-11-18 at 08:05 +0800, Bjorn Helgaas wrote:
> Here's a sample of how I think the ACPI namespace device detection should
> be done.  I think this is almost functionally equivalent to your "Locate
> the IPMI system interface in ACPI namespace" patch.
> 
> The difference I'm aware of is that your patch doesn't call acpi_register_gsi()
> for IRQs from the device _CRS, so I don't think those interrupts would work.
> (I can't test this because I don't have a box that reports an IRQ in _CRS.)
> PNPACPI takes care of this GSI registration for us.
> 
> Patches 3 & 4 are not really related to the main point here, but I left
> them in because without 4, we'll report both SPMI and ACPI namespace
> devices as being from "ACPI".
Hi, Bjorn
    Thanks for your work.
    In this patch set the IPMI system interface will be detected by
using pnp device driver. In theory it is ok to detect the IPMI system
interface by using pnp device driver.
   But we will have to consider the following two problems:
   a. how to detect the IPMI system interface defined in ACPI table if
the pnp subsystem is disabled? For example: by adding the boot option of
"pnpacpi=off". Why does this need to depend on two subsystems(ACPI and
pnp)? 
   b. There exist several exceptions about the _CRS for the IPMI system
interface defined in ACPI table. Maybe there exist two IO/memory address
definition for the IPMI system interface and the memory type is declared
before IO type. In such case we can't know which should be selected.
    
At the same time in order to enable the communication between the ACPI
AML code and IPMI subsystem, too strict dependency is added.
   In such case if the ACPI IPMI driver is not selected, the IPMI
subsystem can't be compiled correctly.

thanks.
   Yakui
    
> 
> ---
> 
> Bjorn Helgaas (5):
>       PNPACPI: save struct acpi_device, not just acpi_handle
>       PNP: add interface to retrieve ACPI device from a PNPACPI device
>       ipmi: remove unused PCI probe code
>       ipmi: refer to table as "SPMI", not "ACPI"
>       ipmi: add PNP discovery (ACPI namespace via PNPACPI)
> 
> 
>  drivers/char/ipmi/ipmi_si_intf.c |  118 +++++++++++++++++++++++++++++++++++---
>  drivers/pnp/pnpacpi/core.c       |   19 ++++--
>  drivers/pnp/pnpacpi/rsparser.c   |    9 ++-
>  include/linux/pnp.h              |   13 ++++
>  4 files changed, 141 insertions(+), 18 deletions(-)
> 

Re: [PATCH v1 0/5] IPMI devices from ACPI namespace

[Bjorn Helgaas]

On Tuesday 17 November 2009 07:29:49 pm ykzhao wrote:
>     In this patch set the IPMI system interface will be detected by
> using pnp device driver. In theory it is ok to detect the IPMI system
> interface by using pnp device driver.
>    But we will have to consider the following two problems:
>    a. how to detect the IPMI system interface defined in ACPI table if
> the pnp subsystem is disabled? For example: by adding the boot option of
> "pnpacpi=off". Why does this need to depend on two subsystems(ACPI and
> pnp)?

This is not a problem.  On an ACPI system, *all* PNP drivers depend
on PNPACPI.  There's no reason IPMI needs to be handled differently.
Treating the IPMI system interface the same as every other device
makes the kernel easier to understand and easier to maintain.

>    b. There exist several exceptions about the _CRS for the IPMI system
> interface defined in ACPI table.

What exceptions are these?  If you're talking about BIOS defects we
need to work around, we should make the workarounds explicit in the
code.  If they're not explicit, we're likely to accidentally break
the workaround later.

> Maybe there exist two IO/memory address 
> definition for the IPMI system interface and the memory type is declared
> before IO type. In such case we can't know which should be selected.

Your patch uses the first address (either I/O or memory) from _CRS.
My patch as posted uses an I/O port address if it exists (even if it
wasn't the first in _CRS) and falls back to using a memory address if
there was no I/O port address.

If this is an important difference, we can walk the struct pnp_dev
resources list, since PNP is careful to maintain that in the same
order as _CRS.  For example, we could do this:

        struct pnp_resource *pnp_res;
        struct resource *res;

        list_for_each_entry(pnp_res, &dev->resources, list) {
                res = &pnp_res->res;
                if (pnp_resource_type(res) == IORESOURCE_IO) {
			info->io_setup = port_setup;
			info->io.addr_type = IPMI_IO_ADDR_SPACE;
			info->io.addr_data = res->start;
			break;
		} else if (pnp_resource_type(res) == IORESOURCE_MEM) {
			info->io_setup = mem_setup;
			info->io.addr_type = IPMI_MEM_ADDR_SPACE;
			info->io.addr_data = res->start;
			break;
		}
	}

But you haven't given an example that proves this is necessary,
so I don't think the extra complication is worthwhile.

> At the same time in order to enable the communication between the ACPI
> AML code and IPMI subsystem, too strict dependency is added.
>    In such case if the ACPI IPMI driver is not selected, the IPMI
> subsystem can't be compiled correctly.

You are right that in my sample patch, ipmi_si_intf.c won't build
unless CONFIG_ACPI_IPMI=y.  But that's easily fixed by an ifdef in
ipmi_si_intf.c.  Or ipmi_si_intf.c could export an interface that
drivers/acpi/ipmi.c could use to register itself.

I don't care as much about those details because they're internal to
the IPMI driver piece, and they don't affect the ACPI core.

The important thing is that drivers/acpi/ipmi.c is not a driver for
the IPMI system interface, since it doesn't deal with interrupts or
IPMI registers, so it shouldn't use acpi_bus_register_driver() or
pnp_register_driver().

Bjorn

Re: [PATCH v1 0/5] IPMI devices from ACPI namespace

[Bjorn Helgaas]

Len, Cory,

Can you take a look at these?  The latest from Yakui (Nov 27) is that
he agrees with this patch set.

Bjorn

Re: [PATCH v1 0/5] IPMI devices from ACPI namespace

[Len Brown]

Corey, Bjorn,

To keep things simple, I've put the 5 patches in this series
together in the acpi-test tree.  Let me know if that is
a bad idea.

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH v1 0/5] IPMI devices from ACPI namespace

[Corey Minyard]

Len Brown wrote:
> Corey, Bjorn,
>
> To keep things simple, I've put the 5 patches in this series
> together in the acpi-test tree.  Let me know if that is
> a bad idea.
>
> thanks,
> Len Brown, Intel Open Source Technology Center
>
>   
That sounds good to me.  I have no way to test them.

-corey