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