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

[Rafael J. Wysocki]

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?

I'm not sure.

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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/