On Sat, 2010-07-17 at 00:01 +0800, Bjorn Helgaas wrote: > On Thursday, July 15, 2010 08:34:35 pm ykzhao wrote: > > On Fri, 2010-07-16 at 00:27 +0800, Bjorn Helgaas wrote: > > > On Wednesday, July 14, 2010 07:31:06 pm ykzhao wrote: > > > > On Thu, 2010-07-15 at 01:24 +0800, Bjorn Helgaas wrote: > > > > Will you please look at this thread > > > > http://marc.info/?l=linux-acpi&m=127608894610765&w=4? > > > > > > > > Before this patch, I followed your comment and put the ACPI IPMI > > > > opregion code into the IPMI_SI module. > > > > > > > > But in fact the IPMI opregion code has nothing to do with the ipmi > > > > system interface. It is enough to use the IPMI API interface. In such > > > > case maybe it is a better choice to put the IPMI opregion code into the > > > > separate file. (In fact Corey suggests that the IPMI opregion code be > > > > put into the separate file). > > > > > > The ACPI IPMI opregion code deals with an ACPI device, and the > > > obvious place where you have that device is the driver "add" > > > function, i.e., ipmi_pnp_probe(). That's the point where the > > > PNP core will help you out by matching device IDs and handling > > > hotplug for you. > > > > The "IPI0001" device defined in ACPI table serves the following two > > purposes: > > 1. IPMI detection and registration: This is defined in IPMI spec. Now > > it is discovered in the drivers/char/ipmi/ipmi_si_intf > > 2. IPMI opregion support: This is defined in ACPI4.0 spec.This > > is to enable that the ACPI AML code can communicate with the BMC > > controller. > > > > Yes. If we put the opregion code with the detection code together, it > > will be simple. And it is unnecessary to get the corresponding device > > list again. But as the IPMI opregion code only uses the API interface of > > IPMI subsystem, it will be better to put it into the separate file. In > > fact this is also Corey's idea. > > I don't care if you put it in a separate file. My only point is that > you shouldn't try to duplicate the device discovery that's already > done in the PNP core. > > > Yes. The hotplug scenario should be considered. This will be handled > > under two places: > > a. the IPMI pnp detection: > > b. install opregion handle for the hotplug IPI0001 device > > > > But in fact the acpi_pnp device is enumerated only once at the boot > > time. > > Drivers cannot rely on anything like "the acpi_pnp device is > enumerated only once at boot-time." Device enumeration happens > in the ACPI core and PNP core, not in drivers, and drivers can't > assume anything about when it happens. Yes. But in fact the ACPI pnp device is enumerated only once at the boot time. In such case it is impossible to get the corresponding pnp device when one "IPI0001" device is hot-plug. Is it still meaningful to consider the "hot-plug" scenario? > > > In such case it is impossible to get the corresponding pnp device > > when one "IPI0001" device is hotplug. Of course the "ipmi_si" can't see > > the hotplugged "IPI0001" device. If we really want to handle the > > hotplug scenario, IMO we should change the detection mechanism from pnp > > to ACPI again. Otherwise it is difficult to handle the hotplug scenario. > > No, you're missing the point. *All* drivers should be written in > the standard style that supports hotplug, because then all devices, > both those present at boot and any added later, are handled the same > way. > > ipmi_si can indeed see any hotplugged IPI0001 device, simply because > it uses pnp_register_driver(). The PNP core calls ipmi_pnp_probe() > when any IPI0001 device is discovered. This device could be present > at boot, or it could be added later. > > > It is an idea to maintain the device list of "impi_si". But if we look > > at the source code of "ipmi_si_intf", the corresponding device list is > > defined as *static*. > > This is a problem you're creating for yourself by trying to completely > separate acpi_ipmi from ipmi_pnp_probe(). I'm suggesting that you can > make your life easier by adding some kind of hook in the ipmi_pnp_probe() > path to set up the opregion handler. It is an idea to add some kind of hook. If so, If so, we will have to export more symbols in ipmi subsystem and the file of drivers/acpi/acpi_ipmi.c. But the main function in the drivers/char/ipmi/ipmi_si_intf.c is to discovery the BMC controller and register the corresponding IPMI system interface. Is it appropriate to export some symbols which is not related with IPMI discovery? > > Another matter is that the "info->dev" in ipmi_pnp_probe is changed from > > acpi_dev to pnp_dev. Now in order to *correctly* create the user > > interface between ACPI and BMC controller, it is realized by comparing > > the "device handle". And the prerequisite is that we should get the > > corresponding pnp device list of "IPI0001". > > If we don't export the symbol of "pnp_bus_type", do you have any idea to > > get the corresponding device list of "IPI0001"? > > If you have the pnp_dev for an IPI0001 device, you can always get the > ACPI handle with pnp_acpi_device(). > > > > > > > +static void ipmi_bmc_gone(int iface) > > > > > > +{ > > > > > > + struct acpi_ipmi_device *ipmi_device, *temp; > > > > > > + > > > > > > + if (list_empty(&driver_data.ipmi_devices)) > > > > > > + return; > > > > > > > > > > You don't need to test for list_empty before using > > > > > list_for_each_entry_safe() > > > > > > > > IMO it is needed as it does the operation of list deletion. > > > > > > I don't think so: > > > > > > * list_for_each_entry_safe - iterate over list of given type safe against removal of list entry > > > > There exist a lot of examples about the removal of list_entry by using > > list_for_each_entry_safe. For example: > > >drivers/char/ipmi/ipmi_msghandler.c > > That only means other drivers also contain this unnecessary code. > > There are many examples of list deletion using list_for_each_entry_safe() > where we don't check list_empty() first. For example: > > pnp_free_resources() > pnp_clean_resource_table() > pnp_free_options() > omap_mux_late_init() > vpe_module_exit()/release_vpe() > fsl_dma_slave_free() > cbe_ptcal_disable() > pmi_of_remove() > > Maybe you could remove the check and do some testing to convince > yourself that it's safe. > > 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
