LinuxLists.cc - [PATCH] driver core / ACPI: Avoid device removal locking problems

by Gu Zheng

[permalink] [raw]

Subject: Re: [PATCH 2/2] ACPI / hotplug: Remove containers synchronously

On 08/28/2013 09:51 PM, Rafael J. Wysocki wrote:

> From: Rafael J. Wysocki <[email protected]>
>
> The current protocol for handling hot remove of containers is very
> fragile and causes acpi_eject_store() to acquire acpi_scan_lock
> which may deadlock with the removal of the device that it is called
> for (the reason is that device sysfs attributes cannot be removed
> while their callbacks are being executed and ACPI device objects
> are removed under acpi_scan_lock).
>
> The problem is related to the fact that containers are handled by
> acpi_bus_device_eject() in a special way, which is to emit an
> offline uevent instead of just removing the container. Then, user
> space is expected to handle that uevent and use the container's
> "eject" attribute to actually remove it. That is fragile, because
> user space may fail to complete the ejection (for example, by not
> using the container's "eject" attribute at all) leaving the BIOS
> kind of in a limbo. Moreover, if the eject event is not signaled
> for a container itself, but for its parent device object (or
> generally, for an ancestor above it in the ACPI namespace), the
> container will be removed straight away without doing that whole
> dance.
>
> For this reason, modify acpi_bus_device_eject() to remove containers
> synchronously like any other objects (user space will get its uevent
> anyway in case it does some other things in response to it) and
> remove the eject_pending ACPI device flag that is not used any more.
> This way acpi_eject_store() doesn't have a reason to acquire
> acpi_scan_lock any more and one possible deadlock scenario goes
> away (plus the code is simplified a bit).
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> Reported-by: Gu Zheng <[email protected]>

Tested-by: Gu Zheng <[email protected]>

> ---
> drivers/acpi/scan.c | 49 ++++++++++++++----------------------------------
> include/acpi/acpi_bus.h | 3 --
> 2 files changed, 16 insertions(+), 36 deletions(-)
>
> Index: linux-pm/drivers/acpi/scan.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/scan.c
> +++ linux-pm/drivers/acpi/scan.c
> @@ -287,6 +287,7 @@ static void acpi_bus_device_eject(void *
> struct acpi_device *device = NULL;
> struct acpi_scan_handler *handler;
> u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE;
> + int error;
>
> mutex_lock(&acpi_scan_lock);
>
> @@ -301,17 +302,13 @@ static void acpi_bus_device_eject(void *
> }
> acpi_evaluate_hotplug_ost(handle, ACPI_NOTIFY_EJECT_REQUEST,
> ACPI_OST_SC_EJECT_IN_PROGRESS, NULL);
> - if (handler->hotplug.mode == AHM_CONTAINER) {
> - device->flags.eject_pending = true;
> + if (handler->hotplug.mode == AHM_CONTAINER)
> kobject_uevent(&device->dev.kobj, KOBJ_OFFLINE);
> - } else {
> - int error;
>
> - get_device(&device->dev);
> - error = acpi_scan_hot_remove(device);
> - if (error)
> - goto err_out;
> - }
> + get_device(&device->dev);
> + error = acpi_scan_hot_remove(device);
> + if (error)
> + goto err_out;
>
> out:
> mutex_unlock(&acpi_scan_lock);
> @@ -496,7 +493,6 @@ acpi_eject_store(struct device *d, struc
> struct acpi_eject_event *ej_event;
> acpi_object_type not_used;
> acpi_status status;
> - u32 ost_source;
> int ret;
>
> if (!count || buf[0] != '1')
> @@ -510,43 +506,28 @@ acpi_eject_store(struct device *d, struc
> if (ACPI_FAILURE(status) || !acpi_device->flags.ejectable)
> return -ENODEV;
>
> - mutex_lock(&acpi_scan_lock);
> -
> - if (acpi_device->flags.eject_pending) {
> - /* ACPI eject notification event. */
> - ost_source = ACPI_NOTIFY_EJECT_REQUEST;
> - acpi_device->flags.eject_pending = 0;
> - } else {
> - /* Eject initiated by user space. */
> - ost_source = ACPI_OST_EC_OSPM_EJECT;
> - }
> ej_event = kmalloc(sizeof(*ej_event), GFP_KERNEL);
> if (!ej_event) {
> ret = -ENOMEM;
> goto err_out;
> }
> - acpi_evaluate_hotplug_ost(acpi_device->handle, ost_source,
> + acpi_evaluate_hotplug_ost(acpi_device->handle, ACPI_OST_EC_OSPM_EJECT,
> ACPI_OST_SC_EJECT_IN_PROGRESS, NULL);
> ej_event->device = acpi_device;
> - ej_event->event = ost_source;
> + ej_event->event = ACPI_OST_EC_OSPM_EJECT;
> get_device(&acpi_device->dev);
> status = acpi_os_hotplug_execute(acpi_bus_hot_remove_device, ej_event);
> - if (ACPI_FAILURE(status)) {
> - put_device(&acpi_device->dev);
> - kfree(ej_event);
> - ret = status == AE_NO_MEMORY ? -ENOMEM : -EAGAIN;
> - goto err_out;
> - }
> - ret = count;
> + if (ACPI_SUCCESS(status))
> + return count;
>
> - out:
> - mutex_unlock(&acpi_scan_lock);
> - return ret;
> + put_device(&acpi_device->dev);
> + kfree(ej_event);
> + ret = status == AE_NO_MEMORY ? -ENOMEM : -EAGAIN;
>
> err_out:
> - acpi_evaluate_hotplug_ost(acpi_device->handle, ost_source,
> + acpi_evaluate_hotplug_ost(acpi_device->handle, ACPI_OST_EC_OSPM_EJECT,
> ACPI_OST_SC_NON_SPECIFIC_FAILURE, NULL);
> - goto out;
> + return ret;
> }
>
> static DEVICE_ATTR(eject, 0200, NULL, acpi_eject_store);
> Index: linux-pm/include/acpi/acpi_bus.h
> ===================================================================
> --- linux-pm.orig/include/acpi/acpi_bus.h
> +++ linux-pm/include/acpi/acpi_bus.h
> @@ -167,9 +167,8 @@ struct acpi_device_flags {
> u32 removable:1;
> u32 ejectable:1;
> u32 power_manageable:1;
> - u32 eject_pending:1;
> u32 match_driver:1;
> - u32 reserved:26;
> + u32 reserved:27;
> };
>
> /* File System */
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>