On Fri, 2013-02-22 at 13:37 +0100, Rafael J. Wysocki wrote:
> On Friday, February 22, 2013 05:51:28 PM Yasuaki Ishimatsu wrote:
> > 2013/02/22 10:50, Rafael J. Wysocki wrote:
> > > On Thursday, February 21, 2013 06:12:21 PM Toshi Kani wrote:
> > >> On Fri, 2013-02-22 at 00:06 +0100, Rafael J. Wysocki wrote:
> > >>> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > >>>
> > >>> Multiple drivers handling hotplug-capable ACPI device nodes install
> > >>> notify handlers covering the same types of events in a very similar
> > >>> way. Moreover, those handlers are installed in separate namespace
> > >>> walks, although that really should be done during namespace scans
> > >>> carried out by acpi_bus_scan(). This leads to substantial code
> > >>> duplication, unnecessary overhead and behavior that is hard to
> > >>> follow.
> > >>>
> > >>> For this reason, introduce common code in drivers/acpi/scan.c for
> > >>> handling hotplug-related notification and carrying out device
> > >>> insertion and eject operations in a generic fashion, such that it
> > >>> may be used by all of the relevant drivers in the future. To cover
> > >>> the existing differences between those drivers introduce struct
> > >>> acpi_hotplug_profile for representing collections of hotplug
> > >>> settings associated with different ACPI scan handlers that can be
> > >>> used by the drivers to make the common code reflect their current
> > >>> behavior.
> > >>>
> > >>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > >>> ---
> > >>>
> > >>> This update fixes an issue pointed out by Toshi Kani (that
> > >>> ACPI_OST_EC_OSPM_* event source codes should not be used with _OST for events
> > >>> that we received a notification for from the platform firmware).
> > >>>
> > >>> Thanks,
> > >>> Rafael
> > >>>
> > >>> ---
> > >>> drivers/acpi/scan.c | 270 ++++++++++++++++++++++++++++++++++++++----------
> > >>> include/acpi/acpi_bus.h | 7 +
> > >>> 2 files changed, 224 insertions(+), 53 deletions(-)
> > >> :
> > >>> +static void acpi_bus_device_eject(void *context)
> > >>> +{
> > >>> + acpi_handle handle = context;
> > >>> + struct acpi_device *device = NULL;
> > >>> + struct acpi_scan_handler *handler;
> > >>> + u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE;
> > >>> +
> > >>> + mutex_lock(&acpi_scan_lock);
> > >>> +
> > >>> + acpi_bus_get_device(handle, &device);
> > >>> + if (!device)
> > >>> + goto err_out;
> > >>> +
> > >>> + handler = device->handler;
> > >>> + if (!handler || !handler->hotplug.enabled) {
> > >>> + ost_code = ACPI_OST_SC_EJECT_NOT_SUPPORTED;
> > >>> + goto err_out;
> > >>> + }
> > >>> + acpi_evaluate_hotplug_ost(handle, ACPI_NOTIFY_EJECT_REQUEST,
> > >>> + ACPI_OST_SC_EJECT_IN_PROGRESS, NULL);
> > >>> + if (handler->hotplug.autoeject) {
> > >>> + int error;
> > >>> +
> > >>> + get_device(&device->dev);
> > >>> + error = acpi_scan_hot_remove(device);
> > >>> + if (error)
> > >>> + goto err_out;
> > >>> + } else {
> > >>> + device->flags.eject_pending = true;
> > >>> }
> > >>> + if (handler->hotplug.uevents)
> > >>> + kobject_uevent(&device->dev.kobj, KOBJ_OFFLINE);
> > >>
> > >> I confirmed that the _OST issue with hot-add is fixed. Here is a new
> > >> one. When autoeject is enabled, it crashes in kobject_uevent() since
> > >> the device is no longer valid.
> > >
> > > Well, this one is more difficult.
> > >
> > > I can change the ordering so that kobject_uevent() is called before
> > > acpi_scan_hot_remove(), but then user space may not know that the device is
> > > being removed at the moment (which still may fail). Still, maybe this is
> > > OK, because user space will get KOBJ_REMOVE when the device actually goes
> > > away anyway.
> > >
> > > Or perhaps we can emit KOBJ_OFFLINE before acpi_scan_hot_remove() and if
> > > it fails, emit KOBJ_ONLINE?
> >
> > How about following patch? My system cannot send EJECT notification.
> > So I have not tested this patch.
>
> No, that's not correct, acpi_scan_hot_remove(device) will remove the device
> from sysfs, if successful, among other things.
>
> We can't emit uevents for a device that has been, even though the data
> structure is still around.
>
> The following are the choices we have, in my opinion:
> - Emit KOBJ_OFFLINE before removal.
> - Emit KOBJ_OFFLINE before removal and KOBJ_ONLINE afterwards if it fails.
> - Do not emit KOBJ_OFFLINE at all with autoeject.
>
> Each of them has some disadvantages, so I'm not sure. The last one is the
> easiest, so I'll probably send another update implementing it.
I agree with the 3rd option. KOBJ_REMOVE is emitted when a device is
removed, so it should be OK to not emitting KOBJ_OFFLINE here. Besides,
we are going to rely on a target offlined beforehand, so this code path
won't have to do it anyway.
BTW, it appears that KOBJ_OFFLINE is used for two different purposes
today. sysfs cpu/memory emits KOBJ_OFFLINE when a target has been
offlined. The container driver (or autoeject is not set with this
patch) emits KOBJ_OFFLINE to request a user to offline a target.
Expected action from user space is different from these cases.
Thanks,
-Toshi
--
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
