| Message ID | 2540040.4NPGdn6Df0@vostro.rjw.lan (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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", ¬_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. > + > + 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, ¬_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? Thanks, Yasuaki Ishimatsu > + if (ret) { > + ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE; > + } else { > + ost_code = ACPI_OST_SC_SUCCESS; > + ret = count; > } > -err: > + acpi_evaluate_hotplug_ost(acpi_device->handle, ACPI_OST_EC_OSPM_EJECT, > + ost_code, NULL); > + > + mutex_unlock(&acpi_scan_lock); > return ret; > } > > @@ -1548,6 +1677,30 @@ static struct acpi_scan_handler *acpi_sc > return NULL; > } > > +static void acpi_scan_init_hotplug(acpi_handle handle) > +{ > + struct acpi_device_info *info; > + struct acpi_scan_handler *handler; > + > + if (ACPI_FAILURE(acpi_get_object_info(handle, &info))) > + return; > + > + if (!(info->valid & ACPI_VALID_HID)) { > + kfree(info); > + return; > + } > + > + /* > + * This relies on the fact that acpi_install_notify_handler() will not > + * install the same notify handler routine twice for the same handle. > + */ > + handler = acpi_scan_match_handler(info->hardware_id.string, NULL); > + kfree(info); > + if (handler && handler->hotplug.enabled) > + acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY, > + acpi_hotplug_notify_cb, NULL); > +} > + > static acpi_status acpi_bus_check_add(acpi_handle handle, u32 lvl_not_used, > void *not_used, void **return_value) > { > @@ -1570,6 +1723,8 @@ static acpi_status acpi_bus_check_add(ac > return AE_OK; > } > > + acpi_scan_init_hotplug(handle); > + > if (!(sta & ACPI_STA_DEVICE_PRESENT) && > !(sta & ACPI_STA_DEVICE_FUNCTIONING)) { > struct acpi_device_wakeup wakeup; > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2013/02/19 15:43, 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", ¬_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. > >> + >> + 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); >> +} Why don't you check _STA method in acpi_scan_bus_device_check()? When hot adding new device, we must check _STA method of the device. Thanks, Yasuaki Ishimatsu >> + >> +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, ¬_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? > > Thanks, > Yasuaki Ishimatsu > >> + if (ret) { >> + ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE; >> + } else { >> + ost_code = ACPI_OST_SC_SUCCESS; >> + ret = count; >> } >> -err: >> + acpi_evaluate_hotplug_ost(acpi_device->handle, ACPI_OST_EC_OSPM_EJECT, >> + ost_code, NULL); >> + >> + mutex_unlock(&acpi_scan_lock); >> return ret; >> } >> >> @@ -1548,6 +1677,30 @@ static struct acpi_scan_handler *acpi_sc >> return NULL; >> } >> >> +static void acpi_scan_init_hotplug(acpi_handle handle) >> +{ >> + struct acpi_device_info *info; >> + struct acpi_scan_handler *handler; >> + >> + if (ACPI_FAILURE(acpi_get_object_info(handle, &info))) >> + return; >> + >> + if (!(info->valid & ACPI_VALID_HID)) { >> + kfree(info); >> + return; >> + } >> + >> + /* >> + * This relies on the fact that acpi_install_notify_handler() will not >> + * install the same notify handler routine twice for the same handle. >> + */ >> + handler = acpi_scan_match_handler(info->hardware_id.string, NULL); >> + kfree(info); >> + if (handler && handler->hotplug.enabled) >> + acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY, >> + acpi_hotplug_notify_cb, NULL); >> +} >> + >> static acpi_status acpi_bus_check_add(acpi_handle handle, u32 lvl_not_used, >> void *not_used, void **return_value) >> { >> @@ -1570,6 +1723,8 @@ static acpi_status acpi_bus_check_add(ac >> return AE_OK; >> } >> >> + acpi_scan_init_hotplug(handle); >> + >> if (!(sta & ACPI_STA_DEVICE_PRESENT) && >> !(sta & ACPI_STA_DEVICE_FUNCTIONING)) { >> struct acpi_device_wakeup wakeup; >> > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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", ¬_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, ¬_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
On Tuesday, February 19, 2013 04:10:15 PM Yasuaki Ishimatsu wrote: > 2013/02/19 15:43, 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(-) > >> [...] > > >> +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); > >> +} > > Why don't you check _STA method in acpi_scan_bus_device_check()? > When hot adding new device, we must check _STA method of the device. Yes, which is going to happen in acpi_bus_scan(). Thanks, Rafael
On Wed, 2013-02-20 at 14:23 +0100, Rafael J. Wysocki wrote: > On Tuesday, February 19, 2013 03:43:08 PM Yasuaki Ishimatsu wrote: : > > > > > - 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. Well, there is an issue... I just tested your patchset and hit the following hang when I tried to delete a container through its sysfs eject. This thread got stuck in trying to delete the sysfs eject file of the container. I believe this is because the shell is still opening this sysfs eject file. PID: 1518 TASK: ffff88005f09c950 CPU: 1 COMMAND: "bash" #0 [ffff88003392baf8] __schedule at ffffffff8151ba75 #1 [ffff88003392bb70] schedule at ffffffff8151bdc7 #2 [ffff88003392bb80] schedule_timeout at ffffffff8151aa55 #3 [ffff88003392bc00] wait_for_common at ffffffff8151bc43 #4 [ffff88003392bc70] wait_for_completion at ffffffff8151bd60 #5 [ffff88003392bc80] sysfs_addrm_finish at ffffffff811984ad #6 [ffff88003392bcd0] sysfs_hash_and_remove at ffffffff81196deb #7 [ffff88003392bd10] sysfs_remove_file at ffffffff81197051 #8 [ffff88003392bd40] device_remove_file at ffffffff81332950 #9 [ffff88003392bd50] acpi_device_unregister at ffffffff812a0556 #10 [ffff88003392bd80] acpi_bus_remove at ffffffff812a0658 #11 [ffff88003392bda0] acpi_bus_trim at ffffffff812a090e #12 [ffff88003392bdd0] acpi_scan_hot_remove at ffffffff812a09c9 #13 [ffff88003392be30] acpi_eject_store at ffffffff812a0b45 #14 [ffff88003392be70] dev_attr_store at ffffffff81332038 #15 [ffff88003392be80] sysfs_write_file at ffffffff81197212 #16 [ffff88003392bee0] vfs_write at ffffffff8113a3cb #17 [ffff88003392bf20] sys_write at ffffffff8113a5fd #18 [ffff88003392bf80] system_call_fastpath at ffffffff81523759 RIP: 00000033a16e4950 RSP: 00007fff4a5f5368 RFLAGS: 00000206 RAX: 0000000000000001 RBX: ffffffff81523759 RCX: ffffffffffffffff RDX: 0000000000000002 RSI: 00007f2f8a3d8000 RDI: 0000000000000001 RBP: 00007f2f8a3d8000 R8: 000000000000000a R9: 00007f2f8a3c4740 R10: 00007f2f8a3c4740 R11: 0000000000000246 R12: 00000033a19b1260 R13: 0000000000000002 R14: ffff880000000000 R15: ffff88003395d680 ORIG_RAX: 0000000000000001 CS: 0033 SS: 002b Thanks, -Toshi -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, February 20, 2013 01:23:34 PM Toshi Kani wrote: > On Wed, 2013-02-20 at 14:23 +0100, Rafael J. Wysocki wrote: > > On Tuesday, February 19, 2013 03:43:08 PM Yasuaki Ishimatsu wrote: > : > > > > > > > - 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. > > Well, there is an issue... I just tested your patchset and hit the > following hang when I tried to delete a container through its sysfs > eject. This thread got stuck in trying to delete the sysfs eject file > of the container. I believe this is because the shell is still opening > this sysfs eject file. You're right. > PID: 1518 TASK: ffff88005f09c950 CPU: 1 COMMAND: "bash" > #0 [ffff88003392baf8] __schedule at ffffffff8151ba75 > #1 [ffff88003392bb70] schedule at ffffffff8151bdc7 > #2 [ffff88003392bb80] schedule_timeout at ffffffff8151aa55 > #3 [ffff88003392bc00] wait_for_common at ffffffff8151bc43 > #4 [ffff88003392bc70] wait_for_completion at ffffffff8151bd60 > #5 [ffff88003392bc80] sysfs_addrm_finish at ffffffff811984ad > #6 [ffff88003392bcd0] sysfs_hash_and_remove at ffffffff81196deb > #7 [ffff88003392bd10] sysfs_remove_file at ffffffff81197051 > #8 [ffff88003392bd40] device_remove_file at ffffffff81332950 > #9 [ffff88003392bd50] acpi_device_unregister at ffffffff812a0556 > #10 [ffff88003392bd80] acpi_bus_remove at ffffffff812a0658 > #11 [ffff88003392bda0] acpi_bus_trim at ffffffff812a090e > #12 [ffff88003392bdd0] acpi_scan_hot_remove at ffffffff812a09c9 > #13 [ffff88003392be30] acpi_eject_store at ffffffff812a0b45 > #14 [ffff88003392be70] dev_attr_store at ffffffff81332038 > #15 [ffff88003392be80] sysfs_write_file at ffffffff81197212 > #16 [ffff88003392bee0] vfs_write at ffffffff8113a3cb > #17 [ffff88003392bf20] sys_write at ffffffff8113a5fd > #18 [ffff88003392bf80] system_call_fastpath at ffffffff81523759 > RIP: 00000033a16e4950 RSP: 00007fff4a5f5368 RFLAGS: 00000206 > RAX: 0000000000000001 RBX: ffffffff81523759 RCX: ffffffffffffffff > RDX: 0000000000000002 RSI: 00007f2f8a3d8000 RDI: 0000000000000001 > RBP: 00007f2f8a3d8000 R8: 000000000000000a R9: 00007f2f8a3c4740 > R10: 00007f2f8a3c4740 R11: 0000000000000246 R12: 00000033a19b1260 > R13: 0000000000000002 R14: ffff880000000000 R15: ffff88003395d680 > ORIG_RAX: 0000000000000001 CS: 0033 SS: 002b Well, admittedly, I didn't think about this situation. Since the eject attribute is under the device we're going to remove, the removal has to be done from a different thread (e.g. workqueue). OK, I'll fix up the patch. Thanks, Rafael
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", ¬_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; + } + + 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, ¬_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); + if (ret) { + ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE; + } else { + ost_code = ACPI_OST_SC_SUCCESS; + ret = count; } -err: + acpi_evaluate_hotplug_ost(acpi_device->handle, ACPI_OST_EC_OSPM_EJECT, + ost_code, NULL); + + mutex_unlock(&acpi_scan_lock); return ret; } @@ -1548,6 +1677,30 @@ static struct acpi_scan_handler *acpi_sc return NULL; } +static void acpi_scan_init_hotplug(acpi_handle handle) +{ + struct acpi_device_info *info; + struct acpi_scan_handler *handler; + + if (ACPI_FAILURE(acpi_get_object_info(handle, &info))) + return; + + if (!(info->valid & ACPI_VALID_HID)) { + kfree(info); + return; + } + + /* + * This relies on the fact that acpi_install_notify_handler() will not + * install the same notify handler routine twice for the same handle. + */ + handler = acpi_scan_match_handler(info->hardware_id.string, NULL); + kfree(info); + if (handler && handler->hotplug.enabled) + acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY, + acpi_hotplug_notify_cb, NULL); +} + static acpi_status acpi_bus_check_add(acpi_handle handle, u32 lvl_not_used, void *not_used, void **return_value) { @@ -1570,6 +1723,8 @@ static acpi_status acpi_bus_check_add(ac return AE_OK; } + acpi_scan_init_hotplug(handle); + if (!(sta & ACPI_STA_DEVICE_PRESENT) && !(sta & ACPI_STA_DEVICE_FUNCTIONING)) { struct acpi_device_wakeup wakeup;