On Thursday, December 26, 2013 11:53:10 AM Yasuaki Ishimatsu wrote: > HI Rafael, > > (2013/12/26 10:01), Rafael J. Wysocki wrote: > > On Monday, December 23, 2013 02:58:38 PM Rafael J. Wysocki wrote: > >> On Saturday, December 14, 2013 06:07:06 AM Rafael J. Wysocki wrote: > >>> On Friday, December 13, 2013 02:17:32 PM Yasuaki Ishimatsu wrote: > >>>> (2013/12/13 13:56), Rafael J. Wysocki wrote: > >>>>> On Friday, December 13, 2013 11:56:32 AM Yasuaki Ishimatsu wrote: > >>>>>> Hi Rafael, > >>>>> > >>>>> Hi, > >>>>> > >>>>>> Please share your more detailed idea. I started to implement the > >>>>>> following > >>>>>> idea. But the idea has one problem. > >>>>>> > >>>>>>>>> The eject work flow can be: > >>>>>>>>> (1) an eject event occurs, > >>>>>>>>> (2) the container "physical" device fails offline in > >>>>>>>>> acpi_scan_hot_remove() > >>>>>>>>> emmitting, say, KOBJ_CHANGE for the "physical" device, > >>>>>>>>> (3) user space notices the KOBJ_CHANGE and does the cleanup > >>>>>>>>> as needed, > >>>>>>>>> (4) user space changes the "physical" container device flag > >>>>>>>>> controlling > >>>>>>>>> offline to 0, > >>>>>>>>> (5) user space uses the sysfs "eject" attribute of the ACPI > >>>>>>>>> container object > >>>>>>>>> to finally eject the container, > >>>>>>>>> (6) the offline in acpi_scan_hot_remove() is now successful, > >>>>>>>>> because the > >>>>>>>>> flag controlling it has been set to 0 in step (4), > >>>>>>>>> (7) the "physical" container device goes away before > >>>>>>>>> executing _EJ0, > >>>>>>>>> (8) the container is ejected. > >>>>>> > >>>>>> I want to emit KOBJ_CHANGE before offlining devices on container > >>>>>> device at (2). > >>>>>> But acpi_scan_hot_remove() offlines devices on container device at > >>>>>> first. > >>>>>> So when offline container device, devices on container has been > >>>>>> offlined. > >>>>>> > >>>>>> Thus the idea cannot fulfill my necessary feature. > >>>>> > >>>>> Well, in that case we need to treat containers in a special way at the > >>>>> ACPI > >>>>> level. Which is a bit unfortunate so to speak. > >>>>> > >>>>> To that end I'd try to add a new flag to struct acpi_hotplug_profile, > >>>>> say > >>>>> .verify_offline, such that if set, it would cause > >>>>> acpi_scan_hot_remove() to > >>>>> check if all of the "physical" companions of the top-level device are > >>>>> offline > >>>>> to start with, and if not, it would just emit KOBJ_CHANGE for the > >>>>> companions > >>>>> that are not offline and bail out. > >>>>> > >>>>> So the above algorithm would become: > >>>>> > >>>>> (1) an eject event occurs, > >>>>> (2) acpi_scan_hot_remove() checks the verify_offline flag in the target > >>>>> device's > >>>>> scan_handler structure, > >>>>> (3) if set (it would always be set for containers), > >>>>> acpi_scan_hot_remove() > >>>>> checks the status of the target device's "physical" companions; > >>>>> if at least > >>>>> one of them is offline, KOBJ_CHANGE is emitted for that > >>>>> "physical" device, > >>>>> and acpi_scan_hot_remove() returns, [I guess we can just emit > >>>>> KOBJ_CHANGE > >>>>> for the first companion that is not offline at this point.] > >>>>> (4) user space notices the KOBJ_CHANGE and does the cleanup as needed; > >>>>> in the > >>>>> process it carries out the offline operation for the container's > >>>>> "physical" > >>>>> companion (there's only one such companion for each container), > >>>>> [That > >>>>> operation for the container itself is trivial, but to succeed it > >>>>> requires > >>>>> all devices below the container to be taken offline in advance.] > >>>>> (5) user space uses the sysfs "eject" attribute of the ACPI container > >>>>> object > >>>>> to finally eject the container, > >>>>> (6) acpi_scan_hot_remove() is now successful, because the container's > >>>>> "physical" > >>>>> companion is now offline, > >>>>> (7) the "physical" container device goes away before executing _EJ0, > >>>>> (8) the container is ejected. > >>>>> > >>>>> I think that should work for you. > >>>> > >>>> This idea seems to same as your previous work. > >>>> http://lkml.org/lkml/2013/2/23/97 > >>> > >>> No, it is not. That one didn't involve physical device representations. > >>> > >>>> How about add autoremove flag into acpi_hotplug_profile and check it as > >>>> follow: > >>> > >>> This is very similar to "enable" except that it generates the uevent and > >>> "enable" doesn't. You might as well modify "enable" to trigger a uevent > >>> if > >>> eject is not enabled (note that with the latest patches in linux-next > >>> "enable" > >>> only applies to eject). > >>> > >>> That said I don't think we should generate any uevents for struct > >>> acpi_device > >>> objects, because they are not devices. > >>> > >>>> --- > >>>> drivers/acpi/scan.c | 5 +++++ > >>>> 1 file changed, 5 insertions(+) > >>>> > >>>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c > >>>> index 5383c81..c43d110 100644 > >>>> --- a/drivers/acpi/scan.c > >>>> +++ b/drivers/acpi/scan.c > >>>> @@ -409,6 +409,11 @@ static void acpi_hotplug_notify_cb(acpi_handle > >>>> handle, u32 type, void *data) > >>>> ost_code = ACPI_OST_SC_EJECT_NOT_SUPPORTED; > >>>> goto err_out; > >>>> } > >>>> + if (!handler->hotplug.autoremove) { > >>>> + kobject_uevent(&device->dev.kobj, KOBJ_CHANGE); > >>>> + ost_code = > >>>> ACPI_OST_SC_EJECT_NON_SPECIFIC_FAILURE; > >>>> + goto err_out; > >>>> + } > >>>> acpi_evaluate_hotplug_ost(handle, > >>>> ACPI_NOTIFY_EJECT_REQUEST, > >>>> > >>>> ACPI_OST_SC_EJECT_IN_PROGRESS, NULL); > >>>> break; > >>>> > >>>> Adding the check into "acpi_hotplug_notify_cb()", user need not change > >>>> the > >>>> flag for removing container device by "sysfs eject". > >>> > >>> Which is utterly confusing. There is no reason whatsoever why the sysfs > >>> eject > >>> attribute should work differently from the event-triggered eject. Quite > >>> the > >>> opposite is the case: it should work in the same way in my opinion so > >>> that it > >>> is possible to test the eject code path using that attribute. > >>> > >>> I'm traveling now, but when I get back home (next week), I'll try to > >>> implement > >>> the thing I was talking about above. > >> > >> It took some more time than I had expected, but I finally was able to get > >> to that. > >> > >> The following two patches implement the idea. This is the minimum (in my > >> opinion) > >> implementation and it may be extended in some ways. > >> > >> Patch [1/2] introduces a new demand_offline flag for struct > >> acpi_hotplug_profile > >> that makes acpi_scan_hot_remove() check the offline status of the device > >> object's > >> companion physical devices to start with and return -EBUSY if at least one > >> of them > >> is not offline. > >> > >> Patch [2/2] uses that flag to implement the container handling. The > >> details are > >> in the changelog, but that's how it is supposed to work. > >> > >> During the initial namespace scan the container ACPI scan handler should > >> create > >> "physical" system container device under /sys/devices/system/container/ for > >> each ACPI container object (the sysfs name of that device should be the > >> same as > >> the sysfs name of the corresponding container object and they should be > >> linked > >> to each other via the firmware_node and physical_node symbolic links, > >> respectively). > >> Those system container devices are initially online. > >> > >> When a container eject event happens, acpi_scan_hot_remove() will notice > >> that > >> hotplug.demand_offline is set in the device object's scan handler and will > >> check the online status of its "physical" companion device, which is online > >> (that is the system container device the above paragraph is about). That > >> will > >> cause KOBJ_CHANGE to be emitted for the system container device and -EBUSY > >> to > >> be returned by acpi_scan_hot_remove(). > >> > >> Now, user space needs to offline the system container device through its > >> online > >> sysfs attribute (that should be present, because the bus type for > >> containers > >> provides the online and offline callbacks). However, the offline for > >> system > >> container devices will only succeed if the physical devices right below the > >> container are all offline, so user space will have to offline those devices > >> before attempting to offline the system container device itself. When > >> finished, user space can trigger the container removal with the help of the > >> eject sysfs attribute of the ACPI container object pointed to by the system > >> container device's firmware_node link (this time the check in > >> acpi_scan_hot_remove() will succeed, because the system container device in > >> question is now offline). > >> > >> The way it is implemented is a bit hackish (the driver_data pointer is > >> slightly > >> abused), but that's a special case and I wanted to avoid adding new fields > >> to > >> struct device just for handling it. > >> > >> The patches haven't been tested yet. I'm going to do that later today, but > >> first I need to take care of some other things, so that has to wait. > > > > Thank you for implementing your idea.
You're very welcome! :-) > > The series of the two patches: > > > > [1/2] ACPI / hotplug: Add demand_offline hotplug profile flag > > https://patchwork.kernel.org/patch/3396711/ > > > > [2/2] ACPI / hotplug / driver core: Handle containers in a special way > > https://patchwork.kernel.org/patch/3399081/ > > > > has been tested now and seems to work as expected, at least for a container > > that has no children (that's one I could simulate easily in a meaningful > > way). > > > > For this reason, if there are no objections, I'll resend them as an official > > submission during the next couple of days. > > I'm testing these patches now. If I have a comment, I send it to these > threads. Thanks a lot! Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
