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

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Tuesday, February 19, 2013 03:43:08 PM Yasuaki Ishimatsu wrote:
> Hi Rafael,
> 
> I have comments. Please see below.
> 
> 2013/02/18 0:21, 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>
> > ---
> >   drivers/acpi/scan.c     |  271 +++++++++++++++++++++++++++++++++++++-----------
> >   include/acpi/acpi_bus.h |    7 +
> >   2 files changed, 220 insertions(+), 58 deletions(-)
> >
> > Index: test/include/acpi/acpi_bus.h
> > ===================================================================
> > --- test.orig/include/acpi/acpi_bus.h
> > +++ test/include/acpi/acpi_bus.h
> > @@ -88,11 +88,18 @@ struct acpi_device;
> >    * -----------------
> >    */
> >
> > +struct acpi_hotplug_profile {
> > +	bool enabled:1;
> > +	bool uevents:1;
> > +	bool autoeject:1;
> > +};
> > +
> >   struct acpi_scan_handler {
> >   	const struct acpi_device_id *ids;
> >   	struct list_head list_node;
> >   	int (*attach)(struct acpi_device *dev, const struct acpi_device_id *id);
> >   	void (*detach)(struct acpi_device *dev);
> > +	struct acpi_hotplug_profile hotplug;
> >   };
> >
> >   /*
> > Index: test/drivers/acpi/scan.c
> > ===================================================================
> > --- test.orig/drivers/acpi/scan.c
> > +++ test/drivers/acpi/scan.c
> > @@ -107,32 +107,19 @@ acpi_device_modalias_show(struct device
> >   }
> >   static DEVICE_ATTR(modalias, 0444, acpi_device_modalias_show, NULL);
> >
> > -/**
> > - * acpi_bus_hot_remove_device: hot-remove a device and its children
> > - * @context: struct acpi_eject_event pointer (freed in this func)
> > - *
> > - * Hot-remove a device and its children. This function frees up the
> > - * memory space passed by arg context, so that the caller may call
> > - * this function asynchronously through acpi_os_hotplug_execute().
> > - */
> > -void acpi_bus_hot_remove_device(void *context)
> > +static int acpi_scan_hot_remove(struct acpi_device *device)
> >   {
> > -	struct acpi_eject_event *ej_event = context;
> > -	struct acpi_device *device = ej_event->device;
> >   	acpi_handle handle = device->handle;
> > -	acpi_handle temp;
> > +	acpi_handle not_used;
> >   	struct acpi_object_list arg_list;
> >   	union acpi_object arg;
> > -	acpi_status status = AE_OK;
> > -	u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE; /* default */
> > -
> > -	mutex_lock(&acpi_scan_lock);
> > +	acpi_status status;
> >
> >   	/* If there is no handle, the device node has been unregistered. */
> > -	if (!device->handle) {
> > +	if (!handle) {
> >   		dev_dbg(&device->dev, "ACPI handle missing\n");
> >   		put_device(&device->dev);
> > -		goto out;
> > +		return -EINVAL;
> >   	}
> >
> >   	ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> > @@ -143,7 +130,7 @@ void acpi_bus_hot_remove_device(void *co
> >   	put_device(&device->dev);
> >   	device = NULL;
> >
> > -	if (ACPI_SUCCESS(acpi_get_handle(handle, "_LCK", &temp))) {
> > +	if (ACPI_SUCCESS(acpi_get_handle(handle, "_LCK", &not_used))) {
> >   		arg_list.count = 1;
> >   		arg_list.pointer = &arg;
> >   		arg.type = ACPI_TYPE_INTEGER;
> > @@ -161,18 +148,162 @@ void acpi_bus_hot_remove_device(void *co
> >   	 */
> >   	status = acpi_evaluate_object(handle, "_EJ0", &arg_list, NULL);
> >   	if (ACPI_FAILURE(status)) {
> > -		if (status != AE_NOT_FOUND)
> > +		if (status == AE_NOT_FOUND) {
> > +			return -ENODEV;
> > +		} else {
> >   			acpi_handle_warn(handle, "Eject failed\n");
> > +			return -EIO;
> > +		}
> > +	}
> > +	return 0;
> > +}
> > +
> > +static void acpi_bus_device_eject(void *context)
> > +{
> > +	acpi_handle handle = context;
> > +	struct acpi_device *device = NULL;
> > +	struct acpi_scan_handler *handler;
> > +	u32 ost_source = ACPI_NOTIFY_EJECT_REQUEST;
> > +	u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE;
> > +
> > +	mutex_lock(&acpi_scan_lock);
> >
> > -		/* Tell the firmware the hot-remove operation has failed. */
> > -		acpi_evaluate_hotplug_ost(handle, ej_event->event,
> > -					  ost_code, NULL);
> > +	acpi_bus_get_device(handle, &device);
> > +	if (!device)
> > +		goto out;
> > +
> > +	handler = device->handler;
> > +	if (!handler || !handler->hotplug.enabled) {
> > +		ost_code = ACPI_OST_SC_EJECT_NOT_SUPPORTED;
> > +		goto out;
> >   	}
> > +	acpi_evaluate_hotplug_ost(handle, ACPI_NOTIFY_EJECT_REQUEST,
> > +				  ACPI_OST_SC_EJECT_IN_PROGRESS, NULL);
> 
> > +	if (handler->hotplug.uevents)
> > +		kobject_uevent(&device->dev.kobj, KOBJ_OFFLINE);
> > +
> > +	if (handler->hotplug.autoeject) {
> > +		int error;
> > +
> > +		ost_source = ACPI_OST_EC_OSPM_EJECT;
> > +		get_device(&device->dev);
> > +		error = acpi_scan_hot_remove(device);
> > +		if (!error)
> > +			ost_code = ACPI_OST_SC_SUCCESS;
> > +	} else {
> > +		device->flags.eject_pending = true;
> > +		goto out_unlock;
> > +	}
> 
> I want you to change the order of uevents and autoeject.
> When user caught OFFLINE event, user thinks devices were removed.
> But it is not guaranteed in this code since acpi_scan_hot_remove() may
> be running.

Sure, I can do that.

> > +
> > + out:
> > +	acpi_evaluate_hotplug_ost(handle, ost_source, ost_code, NULL);
> > +
> > + out_unlock:
> > +	mutex_unlock(&acpi_scan_lock);
> > +}
> > +
> > +static void acpi_scan_bus_device_check(acpi_handle handle, u32 ost_source)
> > +{
> > +	struct acpi_device *device = NULL;
> > +	u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE;
> > +	int error;
> > +
> > +	mutex_lock(&acpi_scan_lock);
> > +
> > +	acpi_bus_get_device(handle, &device);
> > +	if (device) {
> > +		dev_warn(&device->dev, "Attempt to re-insert\n");
> > +		goto out;
> > +	}
> > +	acpi_evaluate_hotplug_ost(handle, ost_source,
> > +				  ACPI_OST_SC_INSERT_IN_PROGRESS, NULL);
> > +	ost_source = ACPI_OST_EC_OSPM_INSERTION;
> > +	error = acpi_bus_scan(handle);
> > +	if (error) {
> > +		acpi_handle_warn(handle, "Namespace scan failure\n");
> > +		goto out;
> > +	}
> > +	error = acpi_bus_get_device(handle, &device);
> > +	if (error) {
> > +		acpi_handle_warn(handle, "Missing device node object\n");
> > +		goto out;
> > +	}
> > +	ost_code = ACPI_OST_SC_SUCCESS;
> > +	if (device->handler && device->handler->hotplug.uevents)
> > +		kobject_uevent(&device->dev.kobj, KOBJ_ONLINE);
> >
> >    out:
> > +	acpi_evaluate_hotplug_ost(handle, ost_source, ost_code, NULL);
> > +	mutex_unlock(&acpi_scan_lock);
> > +}
> > +
> > +static void acpi_scan_bus_check(void *context)
> > +{
> > +	acpi_scan_bus_device_check((acpi_handle)context,
> > +				   ACPI_NOTIFY_BUS_CHECK);
> > +}
> > +
> > +static void acpi_scan_device_check(void *context)
> > +{
> > +	acpi_scan_bus_device_check((acpi_handle)context,
> > +				   ACPI_NOTIFY_DEVICE_CHECK);
> > +}
> > +
> > +static void acpi_hotplug_notify_cb(acpi_handle handle, u32 type, void *not_used)
> > +{
> > +	acpi_osd_exec_callback callback;
> > +	acpi_status status;
> > +
> > +	switch (type) {
> > +	case ACPI_NOTIFY_BUS_CHECK:
> > +		acpi_handle_debug(handle, "ACPI_NOTIFY_BUS_CHECK event\n");
> > +		callback = acpi_scan_bus_check;
> > +		break;
> > +	case ACPI_NOTIFY_DEVICE_CHECK:
> > +		acpi_handle_debug(handle, "ACPI_NOTIFY_DEVICE_CHECK event\n");
> > +		callback = acpi_scan_device_check;
> > +		break;
> > +	case ACPI_NOTIFY_EJECT_REQUEST:
> > +		acpi_handle_debug(handle, "ACPI_NOTIFY_EJECT_REQUEST event\n");
> > +		callback = acpi_bus_device_eject;
> > +		break;
> > +	default:
> > +		/* non-hotplug event; possibly handled by other handler */
> > +		return;
> > +	}
> > +	status = acpi_os_hotplug_execute(callback, handle);
> > +	if (ACPI_FAILURE(status))
> > +		acpi_evaluate_hotplug_ost(handle, type,
> > +					  ACPI_OST_SC_NON_SPECIFIC_FAILURE,
> > +					  NULL);
> > +}
> > +
> > +/**
> > + * acpi_bus_hot_remove_device: hot-remove a device and its children
> > + * @context: struct acpi_eject_event pointer (freed in this func)
> > + *
> > + * Hot-remove a device and its children. This function frees up the
> > + * memory space passed by arg context, so that the caller may call
> > + * this function asynchronously through acpi_os_hotplug_execute().
> > + */
> > +void acpi_bus_hot_remove_device(void *context)
> > +{
> > +	struct acpi_eject_event *ej_event = context;
> > +	struct acpi_device *device = ej_event->device;
> > +	acpi_handle handle = device->handle;
> > +	u32 ost_code = ACPI_OST_SC_SUCCESS;
> > +	int error;
> > +
> > +	mutex_lock(&acpi_scan_lock);
> > +
> > +	error = acpi_scan_hot_remove(device);
> > +	if (error)
> > +		ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE;
> > +
> > +	acpi_evaluate_hotplug_ost(handle, ej_event->event, ost_code, NULL);
> > +
> >   	mutex_unlock(&acpi_scan_lock);
> >   	kfree(context);
> > -	return;
> >   }
> >   EXPORT_SYMBOL(acpi_bus_hot_remove_device);
> >
> > @@ -206,50 +337,48 @@ static ssize_t
> >   acpi_eject_store(struct device *d, struct device_attribute *attr,
> >   		const char *buf, size_t count)
> >   {
> > -	int ret = count;
> > -	acpi_status status;
> > -	acpi_object_type type = 0;
> >   	struct acpi_device *acpi_device = to_acpi_device(d);
> > -	struct acpi_eject_event *ej_event;
> > +	acpi_object_type not_used;
> > +	acpi_status status;
> > +	u32 ost_source;
> > +	u32 ost_code;
> > +	int ret;
> >
> > -	if ((!count) || (buf[0] != '1')) {
> > +	if (!count || buf[0] != '1')
> >   		return -EINVAL;
> > -	}
> > -	if (!acpi_device->driver && !acpi_device->handler) {
> > -		ret = -ENODEV;
> > -		goto err;
> > -	}
> > -	status = acpi_get_type(acpi_device->handle, &type);
> > -	if (ACPI_FAILURE(status) || (!acpi_device->flags.ejectable)) {
> > -		ret = -ENODEV;
> > -		goto err;
> > -	}
> >
> > -	ej_event = kmalloc(sizeof(*ej_event), GFP_KERNEL);
> > -	if (!ej_event) {
> > -		ret = -ENOMEM;
> > -		goto err;
> > -	}
> > +	if ((!acpi_device->handler || !acpi_device->handler->hotplug.enabled)
> > +	    && !acpi_device->driver)
> > +		return -ENODEV;
> > +
> > +	status = acpi_get_type(acpi_device->handle, &not_used);
> > +	if (ACPI_FAILURE(status) || !acpi_device->flags.ejectable)
> > +		return -ENODEV;
> > +
> > +	mutex_lock(&acpi_scan_lock);
> >
> > -	get_device(&acpi_device->dev);
> > -	ej_event->device = acpi_device;
> >   	if (acpi_device->flags.eject_pending) {
> > -		/* event originated from ACPI eject notification */
> > -		ej_event->event = ACPI_NOTIFY_EJECT_REQUEST;
> > +		/* ACPI eject notification event. */
> > +		ost_source = ACPI_NOTIFY_EJECT_REQUEST;
> >   		acpi_device->flags.eject_pending = 0;
> >   	} else {
> > -		/* event originated from user */
> > -		ej_event->event = ACPI_OST_EC_OSPM_EJECT;
> > -		(void) acpi_evaluate_hotplug_ost(acpi_device->handle,
> > -			ej_event->event, ACPI_OST_SC_EJECT_IN_PROGRESS, NULL);
> > +		/* Eject initiated by user space. */
> > +		ost_source = ACPI_OST_EC_OSPM_EJECT;
> >   	}
> > -
> 
> > -	status = acpi_os_hotplug_execute(acpi_bus_hot_remove_device, ej_event);
> > -	if (ACPI_FAILURE(status)) {
> > -		put_device(&acpi_device->dev);
> > -		kfree(ej_event);
> > +	acpi_evaluate_hotplug_ost(acpi_device->handle, ost_source,
> > +				  ACPI_OST_SC_EJECT_IN_PROGRESS, NULL);
> > +	get_device(&acpi_device->dev);
> > +	ret = acpi_scan_hot_remove(acpi_device);
> 
> Why don't you use acpi_os_hotplug_execute()? Do you have some reason?

Yes, I do.  acpi_eject_store() is run in a separate thread anyway (started by
user space), so there's no need to use the workqueue for delayed execution here
and we are under acpi_scan_lock anyway, so there shouldn't be any concurrency
issues.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.