Re: [Update 3][PATCH 2/7] ACPI / scan: Introduce common code for ACPI-based device hotplug

[Toshi Kani <toshi.kani@hp.com>]

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@intel.com>
> > >>>
> > >>> 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@intel.com>
> > >>> ---
> > >>>
> > >>> 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