On Wed, 2009-10-28 at 13:11 +0800, Bjorn Helgaas wrote: > 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. This patch set includes two parts. One is to detect the IPMI system interface defined in ACPI table and register it. The other is to allow the ACPI AML code to communicate with the IPMI system interface. As an device driver is already bound to the device in the second patch, it is impossible that we load another device driver for the same IPMI 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. Is it wrong to use acpi_walk_namespace to locate the device? Why we can't use the acpi_walk_resources directly to parse the corresponding resource? > 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. Why is it necessary that the reset of the system doesn't know ipmi_si_intf.c "owns" the IPMI system interface? In fact the main purpose of ipmi_si_intf.c is to detect the available IPMI system interface on a system. And then this interface can be used by system management software. > Bjorn > > -- 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
