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

[Bjorn Helgaas <bjorn.helgaas@hp.com>]

On Wed, 2009-10-28 at 10:50 +0800, ykzhao wrote:
> On Tue, 2009-10-27 at 23:38 +0800, Bjorn Helgaas wrote:
> > On Monday 26 October 2009 07:02:55 pm ykzhao wrote:
> > > 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.
> > 
> > This is not a problem.  It is perfectly acceptable for the IPMI driver
> > to depend on PNP and PNPACPI in order to claim an ACPI device.  If the
> > users boots with "pnpacpi=off", we just won't find an IPMI device.
> > That is the way it works for TPM devices and serial devices described
> > by ACPI, and IPMI should work the same way.
> Yes. Several methods can be used to detect the IPMI system interface.
> In such case it still can be detected by using other method when the
> ACPI detection mechanism is disabled. But the acpi detection mechanism
> will depend on the ACPI and PNP subsystem if we detect the IMPI system
> interface defined in ACPI table by using PNP device driver. 
> 
> At the same time there exist two ACPI detection mechanisms. One is
> defined in SPMI table. The other is defined in ACPI table. We expect
> that the two ACPI detection mechanisms depend on the same judgement
> condition. 
> So I prefer to detect the IPMI system interface when ACPI enabled
> regardless of the boot option of "pnpacpi=off".

The IPMI driver is not special.  It should behave like all other
drivers.  There is no reason it should handle the "pnpacpi=off" case
differently than other drivers.

But if you change this to use acpi_bus_register_driver(), there will be
no PNP dependency, "pnpacpi=off" will have no effect either way, and
I'll be happy.  I don't think it's the cleanest solution, but it at
least gives us a chance to properly bind the driver to the device.

> > >     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.
> > 
> > This doesn't make any sense.  The fact that an IPMI device might have
> > a variety of IO/memory/IRQ resources is orthogonal to the question of
> > whether you should use pnp_register_driver() or acpi_walk_namespace().
> When we detect the IPMI system interface by loading PNP device driver,
> the advantage is that it can re-use the parse mechanism of
> IO/memory/interrupt. Right?
> In fact we will have to evaluate the following ACPI object:
> 	>_IFT, _GPE, _SRV
> 
> If there exists the _GPE object, it is unnecessary to parse the resource
> related with the interrupt.
> 
> At the same time as I mentioned in the previous email, sometimes we will
> get the two different IO address definitions after evaluating the _CRS
> object. Which should be selected?
> If there exist both IO and memory address definition in _CRS object,
> which should be selected?

You have to decide which address to use whether you learn the addresses
by using acpi_walk_resources() or by looking through the resources
decoded by PNPACPI.  Using acpi_walk_resources() doesn't make that
choice any easier.

> > PNPACPI parses the IPMI device resources for every ACPI device,
> > including the IPMI device, before we even know whether there will be
> > a PNP driver for the device.  It's much easier to look at the PNP
> > resources and figure out which to use than it is to use
> > acpi_walk_resources() manually.
> > 
> > The main point is that ipmi_si_intf.c is a device driver, and it should
> > use the normal driver registration mechanisms.  I think it would be
> > simplest and clearest to make a few PNP enhancements so it could use
> > pnp_register_driver(), but even using acpi_bus_register_driver() would
> > be fine.  Using acpi_walk_namespace() to do everything by hand is just
> > completely wrong.
> The main purpose is to detect the IPMI system interface defined in ACPI
> table.  If the device can be detected correctly, IMO it will be OK.

It is important to detect the device.  It is also important to have a
mechanism to prevent two drivers from thinking they own the same device.

> Why do think that it is wrong to use the acpi_walk_namespace to parse
> the resource?

You're using acpi_walk_namespace() to locate the device, not to parse
the resources.  You use acpi_walk_resources() to parse the resources.

The fact that your patch uses acpi_walk_namespace() to find the
device means that ipmi_si_intf.c can be talking to a device, but the
rest of the system doesn't know ipmi_si_intf.c "owns" it.  So another
driver B could come along and correctly use acpi_bus_register_driver()
with the IPMI IDs.  The Linux ACPI core knows nothing about the fact
that ipmi_si_intf.c thinks it "owns" the IPMI device, so it will call
the driver B "add" method.  Now we have two drivers that both think
they own the device.  This leads to chaos.

Bjorn


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