On Thursday, February 07, 2013 06:05:19 PM Toshi Kani wrote: > On Thu, 2013-02-07 at 23:42 +0100, Rafael J. Wysocki wrote: > > On Thursday, February 07, 2013 07:32:07 AM Toshi Kani wrote: > > > On Thu, 2013-02-07 at 02:32 +0100, Rafael J. Wysocki wrote: > > > > On Wednesday, February 06, 2013 05:51:42 PM Toshi Kani wrote: > > > > > On Thu, 2013-02-07 at 01:55 +0100, Rafael J. Wysocki wrote: > > > > > > On Wednesday, February 06, 2013 03:32:18 PM Toshi Kani wrote: > > > > > > > On Mon, 2013-02-04 at 00:47 +0100, Rafael J. Wysocki wrote: > : > > > > > > Moreover, I'm wondering if the #ifndef FORCE_EJECT thing in acpi_eject_store() > > > > > > actually makes sense after the recent changes to acpi_bus_trim(), because that > > > > > > can't fail now, so the eject will always be carried out. So perhaps we can > > > > > > simply remove the acpi_device->driver check from there entirely in the first > > > > > > place? > > > > > > > > > > > > If we really want to be able to prevent ejects from happening in some cases, > > > > > > we need to implement something along the lines discussed with Greg. > > > > > > > > > > acpi_bus_trim() cannot fail, but sysfs eject can fail. So, I think it > > > > > makes sense to do some validation before calling acpi_bus_trim(). If we > > > > > are to implement the no_eject flag thing, that check needs to be made > > > > > before calling acpi_bus_trim(). > > > > > > > > Sure, but now the logic seems to be "if FORCE_EJECT is not set, don't eject > > > > devices that have no ACPI drivers", so I'm wondering what the purpose of this > > > > is. It definitely isn't too obvious. :-) > > > > > > The check sounds odd for container, but is necessary for CPU and memory > > > for now. CPU and memory go online without their ACPI drivers at boot. > > > So, without this check (i.e. FORCE_EJECT is set), it simply ejects them > > > without attempting to offline when the ACPI drivers are not bound. Of > > > course, we have the issue of a failure in offline be ignored, so this > > > offlining part needs to be moved out from acpi_bus_trim() in one way or > > > the other. > > > > That was my point. > > > > I'm going to add that change for now, but I think we need to take a step back > > and talk about how we want the whole eject machinery to work, regardless of > > the offline/online problem. > > Right. > > > I think that it should work in the same way for all things that may be ejected > > or inserted. Namely, they all should use the same notify handler, for example, > > and if we generate a uevent for one, we should do that for all of them. > > Agreed. > > > Question is how that notify handler should work and here there are two chices > > in my view: Either we'll always emit a uevent and wait for user space to start > > the eject procedure via sysfs, or we won't emit uevents at all and rely on the > > "no_eject" flag to trigger if something is not ready. I'm basically fine with > > any of them (the "no_eject" flag may be useful even if we rely on user space > > to offline stuff before triggering the eject in my opinion), but if we're going > > to rely on user space, then there needs to be a timeout for letting the BIOS > > know that the eject has failed. > > > > [There may be a flag for the common code telling it whether to emit a uevent > > and wait for user space to trigger eject or to trigger eject by itself.] > > IMHO, the kernel waiting for a user program to complete is a recipe for > future problems. So, I think two possible implementation choices are: > > 1. Upon an eject request, off-line all devices and eject > => Implement a kernel sequencer (my RFC patchset) > > 2. Upon an eject request, eject if all devices are off-lined beforehand > => Implement the "no_eject" approach > > Since we are heading to the user space approach, we need to go with #2. > There are some challenges with #2, ex. if sysfs memory online/offline > interfaces can correspond with ACPI memory objects, which we will also > need to look into. Sure. > > Next, I think there needs to be a global list of IDs for which we'll install > > hot-plug notify handlers and which we'll allow to be ejected via /sys/.../eject. > > So, if a device ID is on that list, we'll install the (common) hot-plug notify > > handler for its ACPI handle and we'll set an "eject_possible" flag in its > > struct acpi_device (when created). That will need to be done for every scan > > of the ACPI namespace and not just once, BTW. And we'll check the > > "eject_possible" flag in acpi_eject_store() instead of the "does it have a > > driver or scan handler" check. > > > > Then, the scan handlers for hot-plug devices will be able to add their IDs to > > that global list instead of walking the namespace and installing notify handlers > > by themselves (which as I said has a problem that it's done once, while it > > should be done every time acpi_bus_scan() runs). > > I agree. I think we can use the global notify handler (as the common > notify handler) to look up the eject_possible ID list, instead of > installing a notify handler to each device. Actually installing notify handlers for individual devices may be more efficient, because then we'll know what device the notification is for without looking it up. OK, so it looks like I need to sit down and cut some more patches. :-) 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-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
