On Wednesday, February 13, 2013 10:43:58 AM Toshi Kani wrote: > On Wed, 2013-02-13 at 14:16 +0100, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx> > > > > This changeset is aimed at fixing a few different but related > > problems in the ACPI hotplug infrastructure. > > > > First of all, since notify handlers may be run in parallel with > > acpi_bus_scan(), acpi_bus_trim() and acpi_bus_hot_remove_device() > > and some of them are installed for ACPI handles that have no struct > > acpi_device objects attached (i.e. before those objects are created), > > those notify handlers have to take acpi_scan_lock to prevent races > > from taking place (e.g. a struct acpi_device is found to be present > > for the given ACPI handle, but right after that it is removed by > > acpi_bus_trim() running in parallel to the given notify handler). > > Moreover, since some of them call acpi_bus_scan() and > > acpi_bus_trim(), this leads to the conclusion that acpi_scan_lock > > should be acquired by the callers of these two funtions rather by > > these functions themselves. > > > > For these reasons, make all notify handlers that can handle device > > addition and eject events take acpi_scan_lock and remove the > > acpi_scan_lock locking from acpi_bus_scan() and acpi_bus_trim(). > > Accordingly, update all of their users to make sure that they > > are always called under acpi_scan_lock. > > > > Furthermore, since eject operations are carried out asynchronously > > with respect to the notify events that trigger them, with the help > > of acpi_bus_hot_remove_device(), even if notify handlers take the > > ACPI scan lock, it still is possible that, for example, > > acpi_bus_trim() will run between acpi_bus_hot_remove_device() and > > the notify handler that scheduled its execution and that > > acpi_bus_trim() will remove the device node passed to > > acpi_bus_hot_remove_device() for ejection. In that case, the struct > > acpi_device object obtained by acpi_bus_hot_remove_device() will be > > invalid and not-so-funny things will ensue. To protect agaist that, > > make the users of acpi_bus_hot_remove_device() run get_device() on > > ACPI device node objects that are about to be passed to it and make > > acpi_bus_hot_remove_device() run put_device() on them and check if > > their ACPI handles are not NULL (make acpi_device_unregister() clear > > the device nodes' ACPI handles for that check to work). > > > > Finally, observe that acpi_os_hotplug_execute() actually can fail, > > in which case its caller ought to free memory allocated for the > > context object to prevent leaks from happening. It also needs to > > run put_device() on the device node that it ran get_device() on > > previously in that case. Modify the code accordingly. > > I am concerned with this approach. ACPICA calls notify handlers through > kacpi_notify_wq, which has the max active set to 1. We then use > kacpi_hotplug_wq (which also has the max active set to 1) so that a > hotplug procedure does not block the notify handlers since they can be > used for non-hotplug events as well. In fact we use kacpi_hotplug_wq for a different reason. Please read the comment in __acpi_os_execute() for more details. > Acquiring the scan lock in a notify handler means that a hotplug procedure > can block any notify events. Yes, it can. > So, I'd prefer the following approach. > > - Change all hot-plug procedures (i.e. both add and delete) to proceed > under kacpi_hotplug_wq by calling acpi_os_hotplug_execute(). This > serializes all hotplug procedures, and prevents blocking other notify > events. Yes, we can do that. I was thinking about doing that change, but not in v3.9. There are simply too many notify handlers already there to do that so late in the cycle. And doing that for acpiphp, for example, won't be straightforward at all. Please think about the $subject patch as a temporary measure until we can do something better (which we need to do anyway to reduce code duplication among other things). > (Ideally, we should also run all online/offline procedures > under a same work-queue, just like my RFC patchset did, but this is a > different topic for now.) No, I don't think it is appropriate to run online/offline from _any_ workqueue. In my opinion they should be run from user space. > - Revert 5993c4670 unless this change is absolutely necessary. From > the change log, it is not clear to me why this change was needed. It > changed acpi_bus_hot_remove_device() to take an acpi_device, instead of > an acpi_handle, which introduced a race condition with acpi_device. > acpi_bus_hot_remove_device() should take an acpi_handle, and then obtain > its acpi_device from the acpi_handle since this function is serialized. I thought about that, but actually there's no guarantee that the handle will be valid after _EJ0 as far as I can say. So the race condition is going to be there anyway and using struct acpi_device just makes it easier to avoid it. > - Remove sanity checks with an acpi_device in the notify handlers, > which have a race condition with acpi_device. These type-specific > checks will need to be removed when we have a common notify handler > anyway. The notify handler can continue to check the status of ACPI > device object with an acpi_handle. Type-specific sanity checks / > validations can be performed within a hotplug procedure, instead. Well, the sanest approach here would be to queue up a work item on kacpi_hotplug_wq if the event is of a "hotplug" type and let that work item do all checks, run acpi_bus_scan() etc. But not in v3.9. For v3.9, the most straightforward and least intrusive change we can do is the $subject patch as far as I can say. If you can suggest something less intrusive and more straightforward, please do. Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
