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