ACPI scan and hotplug fixes for 3.14

PATCH In review Rafael J. Wysocki 2 participants 29 messages Nov 4, 2013 – Nov 7, 2013

Subsystems (3)

ACPI PCI ACPI component architecture (acpica)

lore

Share & tools

lore

Rafael J. Wysocki

author

2013-11-04 00:10 UTC #0 lore raw

cover ACPI scan and hotplug fixes for 3.14

Hi,

The following three patches fix some issues that we have in the common ACPI hotplug infrastructure.

If anyone sees any problems with them, please let me know.

Thanks!

Signature · 2 lines

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

Rafael J. Wysocki

author

2013-11-04 00:10 UTC #1 lore raw

PATCH 1/3 ACPI / scan: Start matching drivers after trying scan handlers

From: Rafael J. Wysocki <[email protected]>

ACPI scan handlers should always be attached to struct acpi_device objects before any ACPI drivers, but there is a window during which a driver may be attached to a struct acpi_device before checking if there is a matching scan handler. Namely, that will happen if an ACPI driver module is loaded during acpi_bus_scan() right after the first namespace walk is complete and before the given device is processed by the second namespace walk.

To prevent that from happening, set the match_driver flags of struct acpi_device objects right before running device_attach() for them in acpi_bus_device_attach().

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/acpi/scan.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

drivers/acpi/scan.c +6 -3

Index: linux-pm/drivers/acpi/scan.c
===================================================================
@@ -1676,7 +1676,6 @@ void acpi_init_device_object(struct acpi
 
 void acpi_device_add_finalize(struct acpi_device *device)
 {
-	device->flags.match_driver = true;
 	dev_set_uevent_suppress(&device->dev, false);
 	kobject_uevent(&device->dev.kobj, KOBJ_ADD);
 }

@@ -1915,8 +1914,12 @@ static acpi_status acpi_bus_device_attac
 		return AE_OK;
 
 	ret = acpi_scan_attach_handler(device);
-	if (ret)
-		return ret > 0 ? AE_OK : AE_CTRL_DEPTH;
+	if (ret < 0)
+		return AE_CTRL_DEPTH;
+
+	device->flags.match_driver = true;
+	if (ret > 0)
+		return AE_OK;
 
 	ret = device_attach(&device->dev);
 	return ret >= 0 ? AE_OK : AE_CTRL_DEPTH;

Rafael J. Wysocki

author

2013-11-04 00:10 UTC #2 lore raw

PATCH 2/3 ACPI / hotplug: Refuse to hot-remove all objects with disabled hotplug

In reply to Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

In theory, an ACPI device object may be the parent of another device object whose hotplug is disabled by user space thorugh its scan handler. In that case, the eject operation targeting the parent should fail as though the parent's own hotplug was disabled, but currently this is not the case, because acpi_scan_hot_remove() doesn't check the disable/enable hotplug status of the children of the top-most object passed to it.

To fix this, modify acpi_bus_offline_companions() to return an error code if hotplug is disabled for the given device object. [Also change the name of the function to acpi_bus_offline(), because it is not only about companions any more, and change the name of acpi_bus_online_companions() accordingly.] Make acpi_scan_hot_remove() propagate that error to its callers.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/acpi/scan.c |   57 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 36 insertions(+), 21 deletions(-)

drivers/acpi/scan.c +36 ~~-21~~

Index: linux-pm/drivers/acpi/scan.c
===================================================================
@@ -125,8 +125,8 @@ acpi_device_modalias_show(struct device
 }
 static DEVICE_ATTR(modalias, 0444, acpi_device_modalias_show, NULL);
 
-static acpi_status acpi_bus_offline_companions(acpi_handle handle, u32 lvl,
-					       void *data, void **ret_p)
+static acpi_status acpi_bus_offline(acpi_handle handle, u32 lvl, void *data,
+				    void **ret_p)
 {
 	struct acpi_device *device = NULL;
 	struct acpi_device_physical_node *pn;

@@ -136,6 +136,11 @@ static acpi_status acpi_bus_offline_comp
 	if (acpi_bus_get_device(handle, &device))
 		return AE_OK;
 
+	if (device->handler && !device->handler->hotplug.enabled) {
+		*ret_p = &device->dev;
+		return AE_SUPPORT;
+	}
+
 	mutex_lock(&device->physical_node_lock);
 
 	list_for_each_entry(pn, &device->physical_node_list, node) {

@@ -168,8 +173,8 @@ static acpi_status acpi_bus_offline_comp
 	return status;
 }
 
-static acpi_status acpi_bus_online_companions(acpi_handle handle, u32 lvl,
-					      void *data, void **ret_p)
+static acpi_status acpi_bus_online(acpi_handle handle, u32 lvl, void *data,
+				   void **ret_p)
 {
 	struct acpi_device *device = NULL;
 	struct acpi_device_physical_node *pn;

@@ -214,26 +219,32 @@ static int acpi_scan_hot_remove(struct a
 	 * If the first pass is successful, the second one isn't needed, though.
 	 */
 	errdev = NULL;
-	acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
-			    NULL, acpi_bus_offline_companions,
-			    (void *)false, (void **)&errdev);
-	acpi_bus_offline_companions(handle, 0, (void *)false, (void **)&errdev);
+	status = acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
+				     NULL, acpi_bus_offline, (void *)false,
+				     (void **)&errdev);
+	if (status == AE_SUPPORT) {
+		dev_warn(errdev, "Offline disabled.\n");
+		acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
+				    acpi_bus_online, NULL, NULL, NULL);
+		put_device(&device->dev);
+		return -EPERM;
+	}
+	acpi_bus_offline(handle, 0, (void *)false, (void **)&errdev);
 	if (errdev) {
 		errdev = NULL;
 		acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
-				    NULL, acpi_bus_offline_companions,
-				    (void *)true , (void **)&errdev);
+				    NULL, acpi_bus_offline, (void *)true,
+				    (void **)&errdev);
 		if (!errdev || acpi_force_hot_remove)
-			acpi_bus_offline_companions(handle, 0, (void *)true,
-						    (void **)&errdev);
+			acpi_bus_offline(handle, 0, (void *)true,
+					 (void **)&errdev);
 
 		if (errdev && !acpi_force_hot_remove) {
 			dev_warn(errdev, "Offline failed.\n");
-			acpi_bus_online_companions(handle, 0, NULL, NULL);
+			acpi_bus_online(handle, 0, NULL, NULL);
 			acpi_walk_namespace(ACPI_TYPE_ANY, handle,
-					    ACPI_UINT32_MAX,
-					    acpi_bus_online_companions, NULL,
-					    NULL, NULL);
+					    ACPI_UINT32_MAX, acpi_bus_online,
+					    NULL, NULL, NULL);
 			put_device(&device->dev);
 			return -EBUSY;
 		}

@@ -290,10 +301,9 @@ static void acpi_bus_device_eject(void *
 		goto err_out;
 
 	handler = device->handler;
-	if (!handler || !handler->hotplug.enabled) {
-		ost_code = ACPI_OST_SC_EJECT_NOT_SUPPORTED;
-		goto err_out;
-	}
+	if (!handler || !handler->hotplug.enabled)
+		goto err_support;
+
 	acpi_evaluate_hotplug_ost(handle, ACPI_NOTIFY_EJECT_REQUEST,
 				  ACPI_OST_SC_EJECT_IN_PROGRESS, NULL);
 	if (handler->hotplug.mode == AHM_CONTAINER)

@@ -301,14 +311,19 @@ static void acpi_bus_device_eject(void *
 
 	get_device(&device->dev);
 	error = acpi_scan_hot_remove(device);
-	if (error)
+	if (error == -EPERM) {
+		goto err_support;
+	} else if (error) {
 		goto err_out;
+	}
 
  out:
 	mutex_unlock(&acpi_scan_lock);
 	unlock_device_hotplug();
 	return;
 
+ err_support:
+	ost_code = ACPI_OST_SC_EJECT_NOT_SUPPORTED;
  err_out:
 	acpi_evaluate_hotplug_ost(handle, ACPI_NOTIFY_EJECT_REQUEST, ost_code,
 				  NULL);

Rafael J. Wysocki

author

2013-11-04 00:10 UTC #3 lore raw

PATCH 3/3 ACPI / hotplug: Merge device hot-removal routines

In reply to Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

The only substantial difference between acpi_bus_device_eject() and acpi_bus_hot_remove_device() is the get_device() done by the former which is supposed to be done by callers of the latter. However, at least one caller of acpi_bus_hot_remove_device(), which is handle_root_bridge_removal(), doesn't do that and generally it won't be necessary to do that at all if ACPI handles, rather than struct acpi_device objects, are passed between ACPI hotplug routines, because the correctness of those routines already depends on the persistence of ACPI handles (that may not be an entirely correct assumption in theory, but in practice it has been good enough for a long time).

For this reason, modify acpi_bus_device_eject() to take two arguments, an ACPI handle and event code, and make acpi_bus_hot_remove_device() call it internally. Moreover, rework acpi_bus_hot_remove_device() so that it only takes one argument, an ACPI handle, and make acpi_scan_bus_device_check() queue up acpi_bus_hot_remove_device() instead of acpi_bus_device_eject() itself. After these changes, handle_root_bridge_removal() may be replaced with async execution of acpi_bus_hot_remove_device() with the ACPI handle of the root passed to it as the argument.

However, since acpi_eject_store() also needs to execute acpi_bus_device_eject() asynchronously and it needs to pass a special event code, ACPI_OST_EC_OSPM_EJECT, to that function, introduce a separate wrapper around acpi_bus_device_eject(), acpi_eject_store_work(), that will be queued up by acpi_eject_store() instead of acpi_bus_hot_remove_device(). This allows acpi_eject_store() to be simplified and it allows struct acpi_eject_event to be dropped entirely, as there's no more users of that structure.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/acpi/internal.h |    1 
 drivers/acpi/pci_root.c |   28 ++--------------
 drivers/acpi/scan.c     |   81 ++++++++++++++++--------------------------------
 include/acpi/acpi_bus.h |    6 ---
 4 files changed, 32 insertions(+), 84 deletions(-)

drivers/acpi/scan.c +27 ~~-54~~

Index: linux-pm/drivers/acpi/scan.c
===================================================================
@@ -285,9 +285,8 @@ static int acpi_scan_hot_remove(struct a
 	return 0;
 }
 
-static void acpi_bus_device_eject(void *context)
+static void acpi_bus_device_eject(acpi_handle handle, u32 ost_source)
 {
-	acpi_handle handle = context;
 	struct acpi_device *device = NULL;
 	struct acpi_scan_handler *handler;
 	u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE;

@@ -304,8 +303,10 @@ static void acpi_bus_device_eject(void *
 	if (!handler || !handler->hotplug.enabled)
 		goto err_support;
 
-	acpi_evaluate_hotplug_ost(handle, ACPI_NOTIFY_EJECT_REQUEST,
-				  ACPI_OST_SC_EJECT_IN_PROGRESS, NULL);
+	if (ost_source == ACPI_NOTIFY_EJECT_REQUEST)
+		acpi_evaluate_hotplug_ost(handle, ACPI_NOTIFY_EJECT_REQUEST,
+					  ACPI_OST_SC_EJECT_IN_PROGRESS, NULL);
+
 	if (handler->hotplug.mode == AHM_CONTAINER)
 		kobject_uevent(&device->dev.kobj, KOBJ_OFFLINE);

@@ -325,11 +326,22 @@ static void acpi_bus_device_eject(void *
  err_support:
 	ost_code = ACPI_OST_SC_EJECT_NOT_SUPPORTED;
  err_out:
-	acpi_evaluate_hotplug_ost(handle, ACPI_NOTIFY_EJECT_REQUEST, ost_code,
-				  NULL);
+	acpi_evaluate_hotplug_ost(handle, ost_source, ost_code, NULL);
 	goto out;
 }
 
+/**
+ * acpi_bus_hot_remove_device: Eject a device and its children.
+ * @context: ACPI handle of the device to eject.
+ *
+ * Hot-remove a device and its children. This function is to be called
+ * asynchronously through acpi_os_hotplug_execute().
+ */
+void acpi_bus_hot_remove_device(void *context)
+{
+	acpi_bus_device_eject(context, ACPI_NOTIFY_EJECT_REQUEST);
+}
+
 static void acpi_scan_bus_device_check(acpi_handle handle, u32 ost_source)
 {
 	struct acpi_device *device = NULL;

@@ -428,7 +440,7 @@ static void acpi_hotplug_notify_cb(acpi_
 		break;
 	case ACPI_NOTIFY_EJECT_REQUEST:
 		acpi_handle_debug(handle, "ACPI_NOTIFY_EJECT_REQUEST event\n");
-		callback = acpi_bus_device_eject;
+		callback = acpi_bus_hot_remove_device;
 		break;
 	default:
 		/* non-hotplug event; possibly handled by other handler */

@@ -441,36 +453,6 @@ static void acpi_hotplug_notify_cb(acpi_
 					  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;
-	int error;
-
-	lock_device_hotplug();
-	mutex_lock(&acpi_scan_lock);
-
-	error = acpi_scan_hot_remove(device);
-	if (error && handle)
-		acpi_evaluate_hotplug_ost(handle, ej_event->event,
-					  ACPI_OST_SC_NON_SPECIFIC_FAILURE,
-					  NULL);
-
-	mutex_unlock(&acpi_scan_lock);
-	unlock_device_hotplug();
-	kfree(context);
-}
-EXPORT_SYMBOL(acpi_bus_hot_remove_device);
-
 static ssize_t real_power_state_show(struct device *dev,
 				     struct device_attribute *attr, char *buf)
 {

@@ -497,15 +479,18 @@ static ssize_t power_state_show(struct d
 
 static DEVICE_ATTR(power_state, 0444, power_state_show, NULL);
 
+static void acpi_eject_store_work(void *context)
+{
+	acpi_bus_device_eject(context, ACPI_OST_EC_OSPM_EJECT);
+}
+
 static ssize_t
 acpi_eject_store(struct device *d, struct device_attribute *attr,
 		const char *buf, size_t count)
 {
 	struct acpi_device *acpi_device = to_acpi_device(d);
-	struct acpi_eject_event *ej_event;
 	acpi_object_type not_used;
 	acpi_status status;
-	int ret;
 
 	if (!count || buf[0] != '1')
 		return -EINVAL;

@@ -518,28 +503,16 @@ acpi_eject_store(struct device *d, struc
 	if (ACPI_FAILURE(status) || !acpi_device->flags.ejectable)
 		return -ENODEV;
 
-	ej_event = kmalloc(sizeof(*ej_event), GFP_KERNEL);
-	if (!ej_event) {
-		ret = -ENOMEM;
-		goto err_out;
-	}
 	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 = ACPI_OST_EC_OSPM_EJECT;
-	get_device(&acpi_device->dev);
-	status = acpi_os_hotplug_execute(acpi_bus_hot_remove_device, ej_event);
+	status = acpi_os_hotplug_execute(acpi_eject_store_work,
+					 acpi_device->handle);
 	if (ACPI_SUCCESS(status))
 		return count;
 
-	put_device(&acpi_device->dev);
-	kfree(ej_event);
-	ret = status == AE_NO_MEMORY ? -ENOMEM : -EAGAIN;
-
- err_out:
 	acpi_evaluate_hotplug_ost(acpi_device->handle, ACPI_OST_EC_OSPM_EJECT,
 				  ACPI_OST_SC_NON_SPECIFIC_FAILURE, NULL);
-	return ret;
+	return -EAGAIN;
 }
 
 static DEVICE_ATTR(eject, 0200, NULL, acpi_eject_store);

drivers/acpi/internal.h +1 -0

Index: linux-pm/drivers/acpi/internal.h
===================================================================
@@ -92,6 +92,7 @@ void acpi_device_add_finalize(struct acp
 void acpi_free_pnp_ids(struct acpi_device_pnp *pnp);
 int acpi_bind_one(struct device *dev, acpi_handle handle);
 int acpi_unbind_one(struct device *dev);
+void acpi_bus_hot_remove_device(void *context);
 
 /* --------------------------------------------------------------------------
                                   Power Resource

drivers/acpi/pci_root.c +4 ~~-24~~

Index: linux-pm/drivers/acpi/pci_root.c
===================================================================
@@ -39,6 +39,8 @@
 #include <acpi/acpi_drivers.h>
 #include <acpi/apei.h>
 
+#include "internal.h"
+
 #define PREFIX "ACPI: "
 
 #define _COMPONENT		ACPI_PCI_COMPONENT

@@ -590,29 +592,6 @@ static void handle_root_bridge_insertion
 		acpi_handle_err(handle, "cannot add bridge to acpi list\n");
 }
 
-static void handle_root_bridge_removal(struct acpi_device *device)
-{
-	acpi_status status;
-	struct acpi_eject_event *ej_event;
-
-	ej_event = kmalloc(sizeof(*ej_event), GFP_KERNEL);
-	if (!ej_event) {
-		/* Inform firmware the hot-remove operation has error */
-		(void) acpi_evaluate_hotplug_ost(device->handle,
-					ACPI_NOTIFY_EJECT_REQUEST,
-					ACPI_OST_SC_NON_SPECIFIC_FAILURE,
-					NULL);
-		return;
-	}
-
-	ej_event->device = device;
-	ej_event->event = ACPI_NOTIFY_EJECT_REQUEST;
-
-	status = acpi_os_hotplug_execute(acpi_bus_hot_remove_device, ej_event);
-	if (ACPI_FAILURE(status))
-		kfree(ej_event);
-}
-
 static void _handle_hotplug_event_root(struct work_struct *work)
 {
 	struct acpi_pci_root *root;

@@ -653,7 +632,8 @@ static void _handle_hotplug_event_root(s
 		acpi_handle_printk(KERN_DEBUG, handle,
 				   "Device eject notify on %s\n", __func__);
 		if (root)
-			handle_root_bridge_removal(root->device);
+			acpi_os_hotplug_execute(acpi_bus_hot_remove_device,
+						handle);
 		break;
 	default:
 		acpi_handle_warn(handle,

include/acpi/acpi_bus.h +0 -6

Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
@@ -339,11 +339,6 @@ struct acpi_bus_event {
 	u32 data;
 };
 
-struct acpi_eject_event {
-	struct acpi_device	*device;
-	u32		event;
-};
-
 struct acpi_hp_work {
 	struct work_struct work;
 	acpi_handle handle;

@@ -391,7 +386,6 @@ int acpi_scan_add_handler(struct acpi_sc
 int acpi_bus_register_driver(struct acpi_driver *driver);
 void acpi_bus_unregister_driver(struct acpi_driver *driver);
 int acpi_bus_scan(acpi_handle handle);
-void acpi_bus_hot_remove_device(void *context);
 void acpi_bus_trim(struct acpi_device *start);
 acpi_status acpi_bus_get_ejd(acpi_handle handle, acpi_handle * ejd);
 int acpi_match_device_ids(struct acpi_device *device,

Rafael J. Wysocki

author

2013-11-04 00:29 UTC #4 lore raw

PATCH 3/3 ACPI / hotplug: Remove unnecessary get_device() and put_device()

From: Rafael J. Wysocki <[email protected]>

The get_device() and put_device() calls made by acpi_bus_device_eject() and acpi_scan_hot_remove(), respectively, are not necessary any more, so remove them.

Signed-off-by Rafael J. Wysocki <[email protected]>

---

Actually, after https://patchwork.kernel.org/patch/3134091/ the get_device() and put_device() calls may be dropped just fine.

Thanks,
Rafael

---
 drivers/acpi/scan.c |    5 -----
 1 file changed, 5 deletions(-)

drivers/acpi/scan.c +0 -5

Index: linux-pm/drivers/acpi/scan.c
===================================================================
@@ -205,7 +205,6 @@ static int acpi_scan_hot_remove(struct a
 	/* If there is no handle, the device node has been unregistered. */
 	if (!handle) {
 		dev_dbg(&device->dev, "ACPI handle missing\n");
-		put_device(&device->dev);
 		return -EINVAL;
 	}

@@ -226,7 +225,6 @@ static int acpi_scan_hot_remove(struct a
 		dev_warn(errdev, "Offline disabled.\n");
 		acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
 				    acpi_bus_online, NULL, NULL, NULL);
-		put_device(&device->dev);
 		return -EPERM;
 	}
 	acpi_bus_offline(handle, 0, (void *)false, (void **)&errdev);

@@ -245,7 +243,6 @@ static int acpi_scan_hot_remove(struct a
 			acpi_walk_namespace(ACPI_TYPE_ANY, handle,
 					    ACPI_UINT32_MAX, acpi_bus_online,
 					    NULL, NULL, NULL);
-			put_device(&device->dev);
 			return -EBUSY;
 		}
 	}

@@ -256,7 +253,6 @@ static int acpi_scan_hot_remove(struct a
 	acpi_bus_trim(device);
 
 	/* Device node has been unregistered. */
-	put_device(&device->dev);
 	device = NULL;
 
 	acpi_evaluate_lck(handle, 0);

@@ -310,7 +306,6 @@ static void acpi_bus_device_eject(acpi_h
 	if (handler->hotplug.mode == AHM_CONTAINER)
 		kobject_uevent(&device->dev.kobj, KOBJ_OFFLINE);
 
-	get_device(&device->dev);
 	error = acpi_scan_hot_remove(device);
 	if (error == -EPERM) {
 		goto err_support;

Rafael J. Wysocki

author

2013-11-04 13:14 UTC #5 lore raw

Re: [PATCH 3/3] ACPI / hotplug: Merge device hot-removal routines

In reply to Rafael J. Wysocki

Quoted 24 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>
The only substantial difference between acpi_bus_device_eject() and acpi_bus_hot_remove_device() is the get_device() done by the former which is supposed to be done by callers of the latter. However, at least one caller of acpi_bus_hot_remove_device(), which is handle_root_bridge_removal(), doesn't do that and generally it won't be necessary to do that at all if ACPI handles, rather than struct acpi_device objects, are passed between ACPI hotplug routines, because the correctness of those routines already depends on the persistence of ACPI handles (that may not be an entirely correct assumption in theory, but in practice it has been good enough for a long time).
For this reason, modify acpi_bus_device_eject() to take two arguments, an ACPI handle and event code, and make acpi_bus_hot_remove_device() call it internally. Moreover, rework acpi_bus_hot_remove_device() so that it only takes one argument, an ACPI handle, and make acpi_scan_bus_device_check() queue up acpi_bus_hot_remove_device() instead of acpi_bus_device_eject() itself. After these changes, handle_root_bridge_removal() may be replaced with async execution of acpi_bus_hot_remove_device() with the ACPI handle of the root passed to it as the argument.

Quoted 9 lines

However, since acpi_eject_store() also needs to execute acpi_bus_device_eject() asynchronously and it needs to pass a special event code, ACPI_OST_EC_OSPM_EJECT, to that function, introduce a separate wrapper around acpi_bus_device_eject(), acpi_eject_store_work(), that will be queued up by acpi_eject_store() instead of acpi_bus_hot_remove_device(). This allows acpi_eject_store() to be simplified and it allows struct acpi_eject_event to be dropped entirely, as there's no more users of that structure.

Well, scratch this (and the follow-up), it didn't go in the right direction.

I'll send a new series shortly.

Rafael

Rafael J. Wysocki

author

2013-11-04 13:26 UTC #6 lore raw

cover ACPI scan and hotplug fixes

In reply to Rafael J. Wysocki

Quoted 6 lines · Rafael J. Wysocki

Hi,
The following three patches fix some issues that we have in the common ACPI hotplug infrastructure.
If anyone sees any problems with them, please let me know.

Well, the last patch from the last version didn't really do the right thing in my opinion, so new series follows.

I actually would like to squeeze it into 3.13 if there are no objections, since at least patch [3/6] is kind of urgent.

Thanks!

Signature · 2 lines

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

Rafael J. Wysocki

author

2013-11-04 13:26 UTC #7 lore raw

PATCH 1/6 ACPI / scan: Start matching drivers after trying scan handlers

From: Rafael J. Wysocki <[email protected]>

To prevent that from happening, set the match_driver flags of struct acpi_device objects right before running device_attach() for them in acpi_bus_device_attach().

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/acpi/scan.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

drivers/acpi/scan.c +6 -3

Index: linux-pm/drivers/acpi/scan.c
===================================================================
@@ -1676,7 +1676,6 @@ void acpi_init_device_object(struct acpi
 
 void acpi_device_add_finalize(struct acpi_device *device)
 {
-	device->flags.match_driver = true;
 	dev_set_uevent_suppress(&device->dev, false);
 	kobject_uevent(&device->dev.kobj, KOBJ_ADD);
 }

@@ -1915,8 +1914,12 @@ static acpi_status acpi_bus_device_attac
 		return AE_OK;
 
 	ret = acpi_scan_attach_handler(device);
-	if (ret)
-		return ret > 0 ? AE_OK : AE_CTRL_DEPTH;
+	if (ret < 0)
+		return AE_CTRL_DEPTH;
+
+	device->flags.match_driver = true;
+	if (ret > 0)
+		return AE_OK;
 
 	ret = device_attach(&device->dev);
 	return ret >= 0 ? AE_OK : AE_CTRL_DEPTH;

Toshi Kani

Acked-by on v1 1/6

2013-11-05 23:27 UTC #8 lore raw

Re: [Update][PATCH 1/6] ACPI / scan: Start matching drivers after trying scan handlers

Quoted 10 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>
ACPI scan handlers should always be attached to struct acpi_device objects before any ACPI drivers, but there is a window during which a driver may be attached to a struct acpi_device before checking if there is a matching scan handler. Namely, that will happen if an ACPI driver module is loaded during acpi_bus_scan() right after the first namespace walk is complete and before the given device is processed by the second namespace walk.

Quoted 5 lines

To prevent that from happening, set the match_driver flags of struct acpi_device objects right before running device_attach() for them in acpi_bus_device_attach().
Signed-off-by Rafael J. Wysocki <[email protected]>

Acked-by Toshi Kani <[email protected]>

Thanks,
-Toshi

Rafael J. Wysocki

author

2013-11-04 13:26 UTC #9 lore raw

PATCH 2/6 ACPI / hotplug: Refuse to hot-remove all objects with disabled hotplug

In reply to Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/acpi/scan.c |   57 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 36 insertions(+), 21 deletions(-)

drivers/acpi/scan.c +36 ~~-21~~

Index: linux-pm/drivers/acpi/scan.c
===================================================================
@@ -125,8 +125,8 @@ acpi_device_modalias_show(struct device
 }
 static DEVICE_ATTR(modalias, 0444, acpi_device_modalias_show, NULL);
 
-static acpi_status acpi_bus_offline_companions(acpi_handle handle, u32 lvl,
-					       void *data, void **ret_p)
+static acpi_status acpi_bus_offline(acpi_handle handle, u32 lvl, void *data,
+				    void **ret_p)
 {
 	struct acpi_device *device = NULL;
 	struct acpi_device_physical_node *pn;

@@ -136,6 +136,11 @@ static acpi_status acpi_bus_offline_comp
 	if (acpi_bus_get_device(handle, &device))
 		return AE_OK;
 
+	if (device->handler && !device->handler->hotplug.enabled) {
+		*ret_p = &device->dev;
+		return AE_SUPPORT;
+	}
+
 	mutex_lock(&device->physical_node_lock);
 
 	list_for_each_entry(pn, &device->physical_node_list, node) {

@@ -168,8 +173,8 @@ static acpi_status acpi_bus_offline_comp
 	return status;
 }
 
-static acpi_status acpi_bus_online_companions(acpi_handle handle, u32 lvl,
-					      void *data, void **ret_p)
+static acpi_status acpi_bus_online(acpi_handle handle, u32 lvl, void *data,
+				   void **ret_p)
 {
 	struct acpi_device *device = NULL;
 	struct acpi_device_physical_node *pn;

@@ -214,26 +219,32 @@ static int acpi_scan_hot_remove(struct a
 	 * If the first pass is successful, the second one isn't needed, though.
 	 */
 	errdev = NULL;
-	acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
-			    NULL, acpi_bus_offline_companions,
-			    (void *)false, (void **)&errdev);
-	acpi_bus_offline_companions(handle, 0, (void *)false, (void **)&errdev);
+	status = acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
+				     NULL, acpi_bus_offline, (void *)false,
+				     (void **)&errdev);
+	if (status == AE_SUPPORT) {
+		dev_warn(errdev, "Offline disabled.\n");
+		acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
+				    acpi_bus_online, NULL, NULL, NULL);
+		put_device(&device->dev);
+		return -EPERM;
+	}
+	acpi_bus_offline(handle, 0, (void *)false, (void **)&errdev);
 	if (errdev) {
 		errdev = NULL;
 		acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
-				    NULL, acpi_bus_offline_companions,
-				    (void *)true , (void **)&errdev);
+				    NULL, acpi_bus_offline, (void *)true,
+				    (void **)&errdev);
 		if (!errdev || acpi_force_hot_remove)
-			acpi_bus_offline_companions(handle, 0, (void *)true,
-						    (void **)&errdev);
+			acpi_bus_offline(handle, 0, (void *)true,
+					 (void **)&errdev);
 
 		if (errdev && !acpi_force_hot_remove) {
 			dev_warn(errdev, "Offline failed.\n");
-			acpi_bus_online_companions(handle, 0, NULL, NULL);
+			acpi_bus_online(handle, 0, NULL, NULL);
 			acpi_walk_namespace(ACPI_TYPE_ANY, handle,
-					    ACPI_UINT32_MAX,
-					    acpi_bus_online_companions, NULL,
-					    NULL, NULL);
+					    ACPI_UINT32_MAX, acpi_bus_online,
+					    NULL, NULL, NULL);
 			put_device(&device->dev);
 			return -EBUSY;
 		}

@@ -290,10 +301,9 @@ static void acpi_bus_device_eject(void *
 		goto err_out;
 
 	handler = device->handler;
-	if (!handler || !handler->hotplug.enabled) {
-		ost_code = ACPI_OST_SC_EJECT_NOT_SUPPORTED;
-		goto err_out;
-	}
+	if (!handler || !handler->hotplug.enabled)
+		goto err_support;
+
 	acpi_evaluate_hotplug_ost(handle, ACPI_NOTIFY_EJECT_REQUEST,
 				  ACPI_OST_SC_EJECT_IN_PROGRESS, NULL);
 	if (handler->hotplug.mode == AHM_CONTAINER)

@@ -301,14 +311,19 @@ static void acpi_bus_device_eject(void *
 
 	get_device(&device->dev);
 	error = acpi_scan_hot_remove(device);
-	if (error)
+	if (error == -EPERM) {
+		goto err_support;
+	} else if (error) {
 		goto err_out;
+	}
 
  out:
 	mutex_unlock(&acpi_scan_lock);
 	unlock_device_hotplug();
 	return;
 
+ err_support:
+	ost_code = ACPI_OST_SC_EJECT_NOT_SUPPORTED;
  err_out:
 	acpi_evaluate_hotplug_ost(handle, ACPI_NOTIFY_EJECT_REQUEST, ost_code,
 				  NULL);

Rafael J. Wysocki

author

2013-11-05 23:15 UTC #10 lore raw

PATCH 2/6 ACPI / hotplug: Refuse to hot-remove all objects with disabled hotplug

From: Rafael J. Wysocki <[email protected]>

In theory, an ACPI device object may be the parent of another device object whose hotplug is disabled by user space through its scan handler. In that case, the eject operation targeting the parent should fail as though the parent's own hotplug was disabled, but currently this is not the case, because acpi_scan_hot_remove() doesn't check the disable/enable hotplug status of the children of the top-most object passed to it.

Signed-off-by Rafael J. Wysocki <[email protected]>

---

acpi_force_hot_remove has to be honored by the new code.

Thanks,
Rafael

---
 drivers/acpi/scan.c |   58 +++++++++++++++++++++++++++++++++-------------------
 1 file changed, 37 insertions(+), 21 deletions(-)

drivers/acpi/scan.c +37 ~~-21~~

Index: linux-pm/drivers/acpi/scan.c
===================================================================
@@ -125,8 +125,8 @@ acpi_device_modalias_show(struct device
 }
 static DEVICE_ATTR(modalias, 0444, acpi_device_modalias_show, NULL);
 
-static acpi_status acpi_bus_offline_companions(acpi_handle handle, u32 lvl,
-					       void *data, void **ret_p)
+static acpi_status acpi_bus_offline(acpi_handle handle, u32 lvl, void *data,
+				    void **ret_p)
 {
 	struct acpi_device *device = NULL;
 	struct acpi_device_physical_node *pn;

@@ -136,6 +136,12 @@ static acpi_status acpi_bus_offline_comp
 	if (acpi_bus_get_device(handle, &device))
 		return AE_OK;
 
+	if (device->handler && !device->handler->hotplug.enabled
+	    && !acpi_force_hot_remove) {
+		*ret_p = &device->dev;
+		return AE_SUPPORT;
+	}
+
 	mutex_lock(&device->physical_node_lock);
 
 	list_for_each_entry(pn, &device->physical_node_list, node) {

@@ -168,8 +174,8 @@ static acpi_status acpi_bus_offline_comp
 	return status;
 }
 
-static acpi_status acpi_bus_online_companions(acpi_handle handle, u32 lvl,
-					      void *data, void **ret_p)
+static acpi_status acpi_bus_online(acpi_handle handle, u32 lvl, void *data,
+				   void **ret_p)
 {
 	struct acpi_device *device = NULL;
 	struct acpi_device_physical_node *pn;

@@ -214,26 +220,32 @@ static int acpi_scan_hot_remove(struct a
 	 * If the first pass is successful, the second one isn't needed, though.
 	 */
 	errdev = NULL;
-	acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
-			    NULL, acpi_bus_offline_companions,
-			    (void *)false, (void **)&errdev);
-	acpi_bus_offline_companions(handle, 0, (void *)false, (void **)&errdev);
+	status = acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
+				     NULL, acpi_bus_offline, (void *)false,
+				     (void **)&errdev);
+	if (status == AE_SUPPORT) {
+		dev_warn(errdev, "Offline disabled.\n");
+		acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
+				    acpi_bus_online, NULL, NULL, NULL);
+		put_device(&device->dev);
+		return -EPERM;
+	}
+	acpi_bus_offline(handle, 0, (void *)false, (void **)&errdev);
 	if (errdev) {
 		errdev = NULL;
 		acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
-				    NULL, acpi_bus_offline_companions,
-				    (void *)true , (void **)&errdev);
+				    NULL, acpi_bus_offline, (void *)true,
+				    (void **)&errdev);
 		if (!errdev || acpi_force_hot_remove)
-			acpi_bus_offline_companions(handle, 0, (void *)true,
-						    (void **)&errdev);
+			acpi_bus_offline(handle, 0, (void *)true,
+					 (void **)&errdev);
 
 		if (errdev && !acpi_force_hot_remove) {
 			dev_warn(errdev, "Offline failed.\n");
-			acpi_bus_online_companions(handle, 0, NULL, NULL);
+			acpi_bus_online(handle, 0, NULL, NULL);
 			acpi_walk_namespace(ACPI_TYPE_ANY, handle,
-					    ACPI_UINT32_MAX,
-					    acpi_bus_online_companions, NULL,
-					    NULL, NULL);
+					    ACPI_UINT32_MAX, acpi_bus_online,
+					    NULL, NULL, NULL);
 			put_device(&device->dev);
 			return -EBUSY;
 		}

@@ -290,10 +302,9 @@ static void acpi_bus_device_eject(void *
 		goto err_out;
 
 	handler = device->handler;
-	if (!handler || !handler->hotplug.enabled) {
-		ost_code = ACPI_OST_SC_EJECT_NOT_SUPPORTED;
-		goto err_out;
-	}
+	if (!handler || !handler->hotplug.enabled)
+		goto err_support;
+
 	acpi_evaluate_hotplug_ost(handle, ACPI_NOTIFY_EJECT_REQUEST,
 				  ACPI_OST_SC_EJECT_IN_PROGRESS, NULL);
 	if (handler->hotplug.mode == AHM_CONTAINER)

@@ -301,14 +312,19 @@ static void acpi_bus_device_eject(void *
 
 	get_device(&device->dev);
 	error = acpi_scan_hot_remove(device);
-	if (error)
+	if (error == -EPERM) {
+		goto err_support;
+	} else if (error) {
 		goto err_out;
+	}
 
  out:
 	mutex_unlock(&acpi_scan_lock);
 	unlock_device_hotplug();
 	return;
 
+ err_support:
+	ost_code = ACPI_OST_SC_EJECT_NOT_SUPPORTED;
  err_out:
 	acpi_evaluate_hotplug_ost(handle, ACPI_NOTIFY_EJECT_REQUEST, ost_code,
 				  NULL);

Toshi Kani

2013-11-06 00:43 UTC #11 lore raw

Re: [fixup][PATCH 2/6] ACPI / hotplug: Refuse to hot-remove all objects with disabled hotplug

Quoted 10 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>
In theory, an ACPI device object may be the parent of another device object whose hotplug is disabled by user space through its scan handler. In that case, the eject operation targeting the parent should fail as though the parent's own hotplug was disabled, but currently this is not the case, because acpi_scan_hot_remove() doesn't check the disable/enable hotplug status of the children of the top-most object passed to it.

Quoted 7 lines

To fix this, modify acpi_bus_offline_companions() to return an error code if hotplug is disabled for the given device object. [Also change the name of the function to acpi_bus_offline(), because it is not only about companions any more, and change the name of acpi_bus_online_companions() accordingly.] Make acpi_scan_hot_remove() propagate that error to its callers.

Quoted 15 lines

drivers/acpi/scan.c +4 -4

+static acpi_status acpi_bus_online(acpi_handle handle, u32 lvl, void *data,
+				   void **ret_p)
 {
 	struct acpi_device *device = NULL;
 	struct acpi_device_physical_node *pn;

@@ -214,26 +220,32 @@ static int acpi_scan_hot_remove(struct a
 	 * If the first pass is successful, the second one isn't needed, though.
 	 */
 	errdev = NULL;
-	acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
-			    NULL, acpi_bus_offline_companions,
-			    (void *)false, (void **)&errdev);
-	acpi_bus_offline_companions(handle, 0, (void *)false, (void **)&errdev);
+	status = acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
+				     NULL, acpi_bus_offline, (void *)false,

drivers/acpi/scan.c static int acpi_scan_hot_remove(struct a · line 234

Quoted 10 lines

drivers/acpi/scan.c +9 -0

+				     (void **)&errdev);
+	if (status == AE_SUPPORT) {
+		dev_warn(errdev, "Offline disabled.\n");
+		acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
+				    acpi_bus_online, NULL, NULL, NULL);
+		put_device(&device->dev);
+		return -EPERM;
+	}
+	acpi_bus_offline(handle, 0, (void *)false, (void **)&errdev);
 	if (errdev) {

If the target object failed with AE_SUPPORT, shouldn't we skip the 2nd pass and return with -EPERM after rollback?

Thanks,
-Toshi

Quoted 25 lines

drivers/acpi/scan.c +7 -8

 		errdev = NULL;
 		acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
-				    NULL, acpi_bus_offline_companions,
-				    (void *)true , (void **)&errdev);
+				    NULL, acpi_bus_offline, (void *)true,
+				    (void **)&errdev);
 		if (!errdev || acpi_force_hot_remove)
-			acpi_bus_offline_companions(handle, 0, (void *)true,
-						    (void **)&errdev);
+			acpi_bus_offline(handle, 0, (void *)true,
+					 (void **)&errdev);
 
 		if (errdev && !acpi_force_hot_remove) {
 			dev_warn(errdev, "Offline failed.\n");
-			acpi_bus_online_companions(handle, 0, NULL, NULL);
+			acpi_bus_online(handle, 0, NULL, NULL);
 			acpi_walk_namespace(ACPI_TYPE_ANY, handle,
-					    ACPI_UINT32_MAX,
-					    acpi_bus_online_companions, NULL,
-					    NULL, NULL);
+					    ACPI_UINT32_MAX, acpi_bus_online,
+					    NULL, NULL, NULL);
 			put_device(&device->dev);
 			return -EBUSY;
 		}

Rafael J. Wysocki

author

2013-11-06 01:23 UTC #12 lore raw

Re: [fixup][PATCH 2/6] ACPI / hotplug: Refuse to hot-remove all objects with disabled hotplug

Quoted 39 lines · Toshi Kani

Quoted 10 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
In theory, an ACPI device object may be the parent of another device object whose hotplug is disabled by user space through its scan handler. In that case, the eject operation targeting the parent should fail as though the parent's own hotplug was disabled, but currently this is not the case, because acpi_scan_hot_remove() doesn't check the disable/enable hotplug status of the children of the top-most object passed to it.
Quoted 7 lines
To fix this, modify acpi_bus_offline_companions() to return an error code if hotplug is disabled for the given device object. [Also change the name of the function to acpi_bus_offline(), because it is not only about companions any more, and change the name of acpi_bus_online_companions() accordingly.] Make acpi_scan_hot_remove() propagate that error to its callers.
:
Quoted 20 lines
drivers/acpi/scan.c +9 -4
+static acpi_status acpi_bus_online(acpi_handle handle, u32 lvl, void *data,
+				   void **ret_p)
 {
 	struct acpi_device *device = NULL;
 	struct acpi_device_physical_node *pn;
@@ -214,26 +220,32 @@ static int acpi_scan_hot_remove(struct a
 	 * If the first pass is successful, the second one isn't needed, though.
 	 */
 	errdev = NULL;
-	acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
-			    NULL, acpi_bus_offline_companions,
-			    (void *)false, (void **)&errdev);
-	acpi_bus_offline_companions(handle, 0, (void *)false, (void **)&errdev);
+	status = acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
+				     NULL, acpi_bus_offline, (void *)false,
+				     (void **)&errdev);
+	if (status == AE_SUPPORT) {
+		dev_warn(errdev, "Offline disabled.\n");
+		acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
+				    acpi_bus_online, NULL, NULL, NULL);

Quoted 8 lines

drivers/acpi/scan.c static int acpi_scan_hot_remove(struct a · line 234
Quoted 5 lines
drivers/acpi/scan.c +4 -0
+		put_device(&device->dev);
+		return -EPERM;
+	}
+	acpi_bus_offline(handle, 0, (void *)false, (void **)&errdev);
 	if (errdev) {
If the target object failed with AE_SUPPORT, shouldn't we skip the 2nd pass and return with -EPERM after rollback?

We've checked the target object already in acpi_hotplug_notify_cb() or in acpi_eject_store().

Which is telling me that the previous version of the patch was better after all, because the hotplug.enabled thing takes precedence over acpi_force_hot_remove in the other places. So this:

https://patchwork.kernel.org/patch/3135841/

is the right version. Sorry for the confusion.

Thanks,
Rafael

Toshi Kani

Acked-by

2013-11-06 01:32 UTC #13 lore raw

Re: [fixup][PATCH 2/6] ACPI / hotplug: Refuse to hot-remove all objects with disabled hotplug

Quoted 49 lines · Rafael J. Wysocki

Quoted 39 lines · Toshi Kani
Quoted 10 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
In theory, an ACPI device object may be the parent of another device object whose hotplug is disabled by user space through its scan handler. In that case, the eject operation targeting the parent should fail as though the parent's own hotplug was disabled, but currently this is not the case, because acpi_scan_hot_remove() doesn't check the disable/enable hotplug status of the children of the top-most object passed to it.
Quoted 7 lines
To fix this, modify acpi_bus_offline_companions() to return an error code if hotplug is disabled for the given device object. [Also change the name of the function to acpi_bus_offline(), because it is not only about companions any more, and change the name of acpi_bus_online_companions() accordingly.] Make acpi_scan_hot_remove() propagate that error to its callers.
:
Quoted 20 lines
drivers/acpi/scan.c +9 -4
+static acpi_status acpi_bus_online(acpi_handle handle, u32 lvl, void *data,
+				   void **ret_p)
 {
 	struct acpi_device *device = NULL;
 	struct acpi_device_physical_node *pn;
@@ -214,26 +220,32 @@ static int acpi_scan_hot_remove(struct a
 	 * If the first pass is successful, the second one isn't needed, though.
 	 */
 	errdev = NULL;
-	acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
-			    NULL, acpi_bus_offline_companions,
-			    (void *)false, (void **)&errdev);
-	acpi_bus_offline_companions(handle, 0, (void *)false, (void **)&errdev);
+	status = acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
+				     NULL, acpi_bus_offline, (void *)false,
+				     (void **)&errdev);
+	if (status == AE_SUPPORT) {
+		dev_warn(errdev, "Offline disabled.\n");
+		acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
+				    acpi_bus_online, NULL, NULL, NULL);
Quoted 8 lines
drivers/acpi/scan.c static int acpi_scan_hot_remove(struct a · line 234
Quoted 5 lines
drivers/acpi/scan.c +4 -0
+		put_device(&device->dev);
+		return -EPERM;
+	}
+	acpi_bus_offline(handle, 0, (void *)false, (void **)&errdev);
 	if (errdev) {
If the target object failed with AE_SUPPORT, shouldn't we skip the 2nd pass and return with -EPERM after rollback?

Quoted 2 lines

We've checked the target object already in acpi_hotplug_notify_cb() or in acpi_eject_store().

Oh, I see. Thanks for the clarification.

Quoted 8 lines

Which is telling me that the previous version of the patch was better after all, because the hotplug.enabled thing takes precedence over acpi_force_hot_remove in the other places. So this:
https://patchwork.kernel.org/patch/3135841/
is the right version. Sorry for the confusion.

Agreed.

Acked-by Toshi Kani <[email protected]>

-Toshi

Rafael J. Wysocki

author

2013-11-04 13:27 UTC #14 lore raw

PATCH 3/6 ACPI / hotplug: Fix handle_root_bridge_removal()

In reply to Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

It is required to do get_device() on the struct acpi_device in question before passing it to acpi_bus_hot_remove_device() through acpi_os_hotplug_execute(), because acpi_bus_hot_remove_device() calls acpi_scan_hot_remove() that does put_device() on that object.

The ACPI PCI root removal routine, handle_root_bridge_removal(), doesn't do that, which may lead to premature freeing of the device object or to executing put_device() on an object that has been freed already.

Fix this problem by making handle_root_bridge_removal() use get_device() as appropriate.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/acpi/pci_root.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/acpi/pci_root.c +4 -1

Index: linux-pm/drivers/acpi/pci_root.c
===================================================================
@@ -608,9 +608,12 @@ static void handle_root_bridge_removal(s
 	ej_event->device = device;
 	ej_event->event = ACPI_NOTIFY_EJECT_REQUEST;
 
+	get_device(&device->dev);
 	status = acpi_os_hotplug_execute(acpi_bus_hot_remove_device, ej_event);
-	if (ACPI_FAILURE(status))
+	if (ACPI_FAILURE(status)) {
+		put_device(&device->dev);
 		kfree(ej_event);
+	}
 }
 
 static void _handle_hotplug_event_root(struct work_struct *work)

Toshi Kani

Acked-by on v1 3/6

2013-11-06 23:21 UTC #15 lore raw

Re: [Update][PATCH 3/6] ACPI / hotplug: Fix handle_root_bridge_removal()

Quoted 13 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>
It is required to do get_device() on the struct acpi_device in question before passing it to acpi_bus_hot_remove_device() through acpi_os_hotplug_execute(), because acpi_bus_hot_remove_device() calls acpi_scan_hot_remove() that does put_device() on that object.
The ACPI PCI root removal routine, handle_root_bridge_removal(), doesn't do that, which may lead to premature freeing of the device object or to executing put_device() on an object that has been freed already.

Quoted 4 lines

Fix this problem by making handle_root_bridge_removal() use get_device() as appropriate.
Signed-off-by Rafael J. Wysocki <[email protected]>

Acked-by Toshi Kani <[email protected]>

Thanks,
-Toshi

Rafael J. Wysocki

author

2013-11-04 13:26 UTC #16 lore raw

PATCH 4/6 ACPI / hotplug: Simplify device ejection routines

In reply to Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Simplify handle_root_bridge_removal() and acpi_eject_store() by getting rid of struct acpi_eject_event and passing device objects directly to async routines executed via acpi_os_hotplug_execute().

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/acpi/pci_root.c |   20 ++------------------
 drivers/acpi/scan.c     |   46 ++++++++++++++++++----------------------------
 include/acpi/acpi_bus.h |    5 -----
 3 files changed, 20 insertions(+), 51 deletions(-)

drivers/acpi/pci_root.c +2 ~~-18~~

Index: linux-pm/drivers/acpi/pci_root.c
===================================================================
@@ -593,27 +593,11 @@ static void handle_root_bridge_insertion
 static void handle_root_bridge_removal(struct acpi_device *device)
 {
 	acpi_status status;
-	struct acpi_eject_event *ej_event;
-
-	ej_event = kmalloc(sizeof(*ej_event), GFP_KERNEL);
-	if (!ej_event) {
-		/* Inform firmware the hot-remove operation has error */
-		(void) acpi_evaluate_hotplug_ost(device->handle,
-					ACPI_NOTIFY_EJECT_REQUEST,
-					ACPI_OST_SC_NON_SPECIFIC_FAILURE,
-					NULL);
-		return;
-	}
-
-	ej_event->device = device;
-	ej_event->event = ACPI_NOTIFY_EJECT_REQUEST;
 
 	get_device(&device->dev);
-	status = acpi_os_hotplug_execute(acpi_bus_hot_remove_device, ej_event);
-	if (ACPI_FAILURE(status)) {
+	status = acpi_os_hotplug_execute(acpi_bus_hot_remove_device, device);
+	if (ACPI_FAILURE(status))
 		put_device(&device->dev);
-		kfree(ej_event);
-	}
 }
 
 static void _handle_hotplug_event_root(struct work_struct *work)

drivers/acpi/scan.c +18 ~~-28~~

92 lines not on this page · raw message

include/acpi/acpi_bus.h +0 -5

Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
@@ -339,11 +339,6 @@ struct acpi_bus_event {
 	u32 data;
 };
 
-struct acpi_eject_event {
-	struct acpi_device	*device;
-	u32		event;
-};
-
 struct acpi_hp_work {
 	struct work_struct work;
 	acpi_handle handle;

Toshi Kani

2013-11-06 23:31 UTC #17 lore raw

Re: [Update][PATCH 4/6] ACPI / hotplug: Simplify device ejection routines

Quoted 5 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>
Simplify handle_root_bridge_removal() and acpi_eject_store() by getting rid of struct acpi_eject_event and passing device objects directly to async routines executed via acpi_os_hotplug_execute().

Quoted 24 lines

static ssize_t
acpi_eject_store(struct device *d, struct device_attribute *attr,

drivers/acpi/scan.c +0 -8

 		const char *buf, size_t count)
 {
 	struct acpi_device *acpi_device = to_acpi_device(d);
-	struct acpi_eject_event *ej_event;
 	acpi_object_type not_used;
 	acpi_status status;
-	int ret;
 
 	if (!count || buf[0] != '1')
 		return -EINVAL;

@@ -518,28 +519,17 @@ acpi_eject_store(struct device *d, struc
 	if (ACPI_FAILURE(status) || !acpi_device->flags.ejectable)
 		return -ENODEV;
 
-	ej_event = kmalloc(sizeof(*ej_event), GFP_KERNEL);
-	if (!ej_event) {
-		ret = -ENOMEM;
-		goto err_out;
-	}
 	acpi_evaluate_hotplug_ost(acpi_device->handle, ACPI_OST_EC_OSPM_EJECT,
 				  ACPI_OST_SC_EJECT_IN_PROGRESS, NULL);
-	ej_event->device = acpi_device;

drivers/acpi/scan.c acpi_eject_store(struct device *d, struc · line 537

Quoted 10 lines

drivers/acpi/scan.c +1 -4

-	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);
+	status = acpi_os_hotplug_execute(acpi_eject_store_work, acpi_device);
 	if (ACPI_SUCCESS(status))
 		return count;
 
 	put_device(&acpi_device->dev);
-	kfree(ej_event);
-	ret = status == AE_NO_MEMORY ? -ENOMEM : -EAGAIN;

Why are you deleting the above line as part of this change?

Thanks,
-Toshi

Rafael J. Wysocki

author

2013-11-07 00:02 UTC #18 lore raw

Re: [Update][PATCH 4/6] ACPI / hotplug: Simplify device ejection routines

Quoted 38 lines · Toshi Kani

Quoted 5 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>
Simplify handle_root_bridge_removal() and acpi_eject_store() by getting rid of struct acpi_eject_event and passing device objects directly to async routines executed via acpi_os_hotplug_execute().

Quoted 31 lines

static ssize_t
acpi_eject_store(struct device *d, struct device_attribute *attr,

(quoted diff) +1 ~~-10~~

 		const char *buf, size_t count)
 {
 	struct acpi_device *acpi_device = to_acpi_device(d);
-	struct acpi_eject_event *ej_event;
 	acpi_object_type not_used;
 	acpi_status status;
-	int ret;
 
 	if (!count || buf[0] != '1')
 		return -EINVAL;

@@ -518,28 +519,17 @@ acpi_eject_store(struct device *d, struc
 	if (ACPI_FAILURE(status) || !acpi_device->flags.ejectable)
 		return -ENODEV;
 
-	ej_event = kmalloc(sizeof(*ej_event), GFP_KERNEL);
-	if (!ej_event) {
-		ret = -ENOMEM;
-		goto err_out;
-	}
 	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 = ACPI_OST_EC_OSPM_EJECT;
 	get_device(&acpi_device->dev);
-	status = acpi_os_hotplug_execute(acpi_bus_hot_remove_device, ej_event);
+	status = acpi_os_hotplug_execute(acpi_eject_store_work, acpi_device);
 	if (ACPI_SUCCESS(status))
 		return count;

Quoted 5 lines

Review acpi_eject_store(struct device *d, struc · line 537
Quoted 3 lines
(quoted diff) +0 -2
 	put_device(&acpi_device->dev);
-	kfree(ej_event);
-	ret = status == AE_NO_MEMORY ? -ENOMEM : -EAGAIN;
Why are you deleting the above line as part of this change?

Ah, because I forgot that acpi_os_hotplug_execute() can also return AE_NO_MEMORY. :-)

Good catch, updated pach follows.

Thanks,
Rafael

---
From: Rafael J. Wysocki <[email protected]>
Subject: ACPI / hotplug: Simplify device ejection routines

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/acpi/pci_root.c |   20 ++------------------
 drivers/acpi/scan.c     |   46 ++++++++++++++++++----------------------------
 include/acpi/acpi_bus.h |    5 -----
 3 files changed, 20 insertions(+), 51 deletions(-)

drivers/acpi/pci_root.c +2 ~~-18~~

Index: linux-pm/drivers/acpi/pci_root.c
===================================================================
@@ -593,27 +593,11 @@ static void handle_root_bridge_insertion
 static void handle_root_bridge_removal(struct acpi_device *device)
 {
 	acpi_status status;
-	struct acpi_eject_event *ej_event;
-
-	ej_event = kmalloc(sizeof(*ej_event), GFP_KERNEL);
-	if (!ej_event) {
-		/* Inform firmware the hot-remove operation has error */
-		(void) acpi_evaluate_hotplug_ost(device->handle,
-					ACPI_NOTIFY_EJECT_REQUEST,
-					ACPI_OST_SC_NON_SPECIFIC_FAILURE,
-					NULL);
-		return;
-	}
-
-	ej_event->device = device;
-	ej_event->event = ACPI_NOTIFY_EJECT_REQUEST;
 
 	get_device(&device->dev);
-	status = acpi_os_hotplug_execute(acpi_bus_hot_remove_device, ej_event);
-	if (ACPI_FAILURE(status)) {
+	status = acpi_os_hotplug_execute(acpi_bus_hot_remove_device, device);
+	if (ACPI_FAILURE(status))
 		put_device(&device->dev);
-		kfree(ej_event);
-	}
 }
 
 static void _handle_hotplug_event_root(struct work_struct *work)

drivers/acpi/scan.c +18 ~~-28~~

92 lines not on this page · raw message

include/acpi/acpi_bus.h +0 -5

Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
@@ -339,11 +339,6 @@ struct acpi_bus_event {
 	u32 data;
 };
 
-struct acpi_eject_event {
-	struct acpi_device	*device;
-	u32		event;
-};
-
 struct acpi_hp_work {
 	struct work_struct work;
 	acpi_handle handle;

Toshi Kani

Acked-by

2013-11-07 00:17 UTC #19 lore raw

Re: [Update][PATCH 4/6] ACPI / hotplug: Simplify device ejection routines

Quoted 45 lines · Rafael J. Wysocki

Quoted 38 lines · Toshi Kani
Quoted 5 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
Simplify handle_root_bridge_removal() and acpi_eject_store() by getting rid of struct acpi_eject_event and passing device objects directly to async routines executed via acpi_os_hotplug_execute().
:
Quoted 31 lines
static ssize_t
acpi_eject_store(struct device *d, struct device_attribute *attr,
drivers/acpi/scan.c +1 ~~-10~~
 		const char *buf, size_t count)
 {
 	struct acpi_device *acpi_device = to_acpi_device(d);
-	struct acpi_eject_event *ej_event;
 	acpi_object_type not_used;
 	acpi_status status;
-	int ret;
 
 	if (!count || buf[0] != '1')
 		return -EINVAL;
@@ -518,28 +519,17 @@ acpi_eject_store(struct device *d, struc
 	if (ACPI_FAILURE(status) || !acpi_device->flags.ejectable)
 		return -ENODEV;
 
-	ej_event = kmalloc(sizeof(*ej_event), GFP_KERNEL);
-	if (!ej_event) {
-		ret = -ENOMEM;
-		goto err_out;
-	}
 	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 = ACPI_OST_EC_OSPM_EJECT;
 	get_device(&acpi_device->dev);
-	status = acpi_os_hotplug_execute(acpi_bus_hot_remove_device, ej_event);
+	status = acpi_os_hotplug_execute(acpi_eject_store_work, acpi_device);
 	if (ACPI_SUCCESS(status))
 		return count;
 
Quoted 5 lines
drivers/acpi/scan.c acpi_eject_store(struct device *d, struc · line 537
Quoted 3 lines
drivers/acpi/scan.c +0 -2
 	put_device(&acpi_device->dev);
-	kfree(ej_event);
-	ret = status == AE_NO_MEMORY ? -ENOMEM : -EAGAIN;
Why are you deleting the above line as part of this change?

Quoted 3 lines

Ah, because I forgot that acpi_os_hotplug_execute() can also return AE_NO_MEMORY. :-)
Good catch, updated pach follows.

Looks good. The rest of the patches also look good. For patch 4-6:

Acked-by Toshi Kani <[email protected]>

Thanks,
-Toshi

Rafael J. Wysocki

author

2013-11-07 00:22 UTC #20 lore raw

Re: [Update][PATCH 4/6] ACPI / hotplug: Simplify device ejection routines

Quoted 46 lines · Toshi Kani

Quoted 45 lines · Rafael J. Wysocki
Quoted 43 lines · Toshi Kani
Quoted 5 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
Simplify handle_root_bridge_removal() and acpi_eject_store() by getting rid of struct acpi_eject_event and passing device objects directly to async routines executed via acpi_os_hotplug_execute().
:
Quoted 24 lines
static ssize_t
acpi_eject_store(struct device *d, struct device_attribute *attr,
drivers/acpi/scan.c +0 -8
 		const char *buf, size_t count)
 {
 	struct acpi_device *acpi_device = to_acpi_device(d);
-	struct acpi_eject_event *ej_event;
 	acpi_object_type not_used;
 	acpi_status status;
-	int ret;
 
 	if (!count || buf[0] != '1')
 		return -EINVAL;
@@ -518,28 +519,17 @@ acpi_eject_store(struct device *d, struc
 	if (ACPI_FAILURE(status) || !acpi_device->flags.ejectable)
 		return -ENODEV;
 
-	ej_event = kmalloc(sizeof(*ej_event), GFP_KERNEL);
-	if (!ej_event) {
-		ret = -ENOMEM;
-		goto err_out;
-	}
 	acpi_evaluate_hotplug_ost(acpi_device->handle, ACPI_OST_EC_OSPM_EJECT,
 				  ACPI_OST_SC_EJECT_IN_PROGRESS, NULL);
-	ej_event->device = acpi_device;
drivers/acpi/scan.c acpi_eject_store(struct device *d, struc · line 537
Quoted 10 lines
drivers/acpi/scan.c +1 -4
-	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);
+	status = acpi_os_hotplug_execute(acpi_eject_store_work, acpi_device);
 	if (ACPI_SUCCESS(status))
 		return count;
 
 	put_device(&acpi_device->dev);
-	kfree(ej_event);
-	ret = status == AE_NO_MEMORY ? -ENOMEM : -EAGAIN;
Why are you deleting the above line as part of this change?

Quoted 7 lines

Quoted 3 lines
Ah, because I forgot that acpi_os_hotplug_execute() can also return AE_NO_MEMORY. :-)
Good catch, updated pach follows.
Looks good. The rest of the patches also look good. For patch 4-6:
Acked-by Toshi Kani <[email protected]>

Awesome, thanks! :-)

Signature · 2 lines

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

Rafael J. Wysocki

author

2013-11-04 13:26 UTC #21 lore raw

PATCH 5/6 ACPI / hotplug: Make acpi_bus_hot_remove_device() internal

In reply to Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Notice that handle_root_bridge_removal() is the only user of acpi_bus_hot_remove_device(), so it doesn't have to be exported any more and can be made internal to the ACPI core.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/acpi/internal.h |    1 +
 drivers/acpi/pci_root.c |    2 ++
 drivers/acpi/scan.c     |    1 -
 include/acpi/acpi_bus.h |    1 -
 4 files changed, 3 insertions(+), 2 deletions(-)

drivers/acpi/internal.h +1 -0

Index: linux-pm/drivers/acpi/internal.h
===================================================================
@@ -92,6 +92,7 @@ void acpi_device_add_finalize(struct acp
 void acpi_free_pnp_ids(struct acpi_device_pnp *pnp);
 int acpi_bind_one(struct device *dev, acpi_handle handle);
 int acpi_unbind_one(struct device *dev);
+void acpi_bus_hot_remove_device(void *context);
 
 /* --------------------------------------------------------------------------
                                   Power Resource

include/acpi/acpi_bus.h +0 -1

Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
@@ -386,7 +386,6 @@ int acpi_scan_add_handler(struct acpi_sc
 int acpi_bus_register_driver(struct acpi_driver *driver);
 void acpi_bus_unregister_driver(struct acpi_driver *driver);
 int acpi_bus_scan(acpi_handle handle);
-void acpi_bus_hot_remove_device(void *context);
 void acpi_bus_trim(struct acpi_device *start);
 acpi_status acpi_bus_get_ejd(acpi_handle handle, acpi_handle * ejd);
 int acpi_match_device_ids(struct acpi_device *device,

drivers/acpi/scan.c +0 -1

Index: linux-pm/drivers/acpi/scan.c
===================================================================
@@ -467,7 +467,6 @@ void acpi_bus_hot_remove_device(void *co
 {
 	__acpi_bus_hot_remove_device(context, ACPI_NOTIFY_EJECT_REQUEST);
 }
-EXPORT_SYMBOL(acpi_bus_hot_remove_device);
 
 static ssize_t real_power_state_show(struct device *dev,
 				     struct device_attribute *attr, char *buf)

drivers/acpi/pci_root.c +2 -0

Index: linux-pm/drivers/acpi/pci_root.c
===================================================================
@@ -39,6 +39,8 @@
 #include <acpi/acpi_drivers.h>
 #include <acpi/apei.h>
 
+#include "internal.h"
+
 #define PREFIX "ACPI: "
 
 #define _COMPONENT		ACPI_PCI_COMPONENT

Rafael J. Wysocki

author

2013-11-04 13:26 UTC #22 lore raw

PATCH 6/6 ACPI / hotplug: Merge device hot-removal routines

In reply to Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

There is no real reasn why acpi_bus_device_eject() and acpi_bus_hot_remove_device() should work differently, so rework acpi_bus_device_eject() so that it can be called internally by both acpi_bus_hot_remove_device() and acpi_eject_store_work(). Accordingly, rework acpi_hotplug_notify_cb() to queue up the execution of acpi_bus_hot_remove_device() through acpi_os_hotplug_execute() on eject request notifications.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/acpi/scan.c |   84 +++++++++++++++++++++++-----------------------------
 1 file changed, 38 insertions(+), 46 deletions(-)

drivers/acpi/scan.c +38 ~~-46~~

Index: linux-pm/drivers/acpi/scan.c
===================================================================
@@ -285,10 +285,9 @@ static int acpi_scan_hot_remove(struct a
 	return 0;
 }
 
-static void acpi_bus_device_eject(void *context)
+static void acpi_bus_device_eject(struct acpi_device *device, u32 ost_src)
 {
-	acpi_handle handle = context;
-	struct acpi_device *device = NULL;
+	acpi_handle handle = device->handle;
 	struct acpi_scan_handler *handler;
 	u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE;
 	int error;

@@ -296,20 +295,19 @@ static void acpi_bus_device_eject(void *
 	lock_device_hotplug();
 	mutex_lock(&acpi_scan_lock);
 
-	acpi_bus_get_device(handle, &device);
-	if (!device)
-		goto err_out;
-
 	handler = device->handler;
-	if (!handler || !handler->hotplug.enabled)
+	if (!handler || !handler->hotplug.enabled) {
+		put_device(&device->dev);
 		goto err_support;
+	}
+
+	if (ost_src == ACPI_NOTIFY_EJECT_REQUEST)
+		acpi_evaluate_hotplug_ost(handle, ACPI_NOTIFY_EJECT_REQUEST,
+					  ACPI_OST_SC_EJECT_IN_PROGRESS, NULL);
 
-	acpi_evaluate_hotplug_ost(handle, ACPI_NOTIFY_EJECT_REQUEST,
-				  ACPI_OST_SC_EJECT_IN_PROGRESS, NULL);
 	if (handler->hotplug.mode == AHM_CONTAINER)
 		kobject_uevent(&device->dev.kobj, KOBJ_OFFLINE);
 
-	get_device(&device->dev);
 	error = acpi_scan_hot_remove(device);
 	if (error == -EPERM) {
 		goto err_support;

@@ -325,8 +323,7 @@ static void acpi_bus_device_eject(void *
  err_support:
 	ost_code = ACPI_OST_SC_EJECT_NOT_SUPPORTED;
  err_out:
-	acpi_evaluate_hotplug_ost(handle, ACPI_NOTIFY_EJECT_REQUEST, ost_code,
-				  NULL);
+	acpi_evaluate_hotplug_ost(handle, ost_src, ost_code, NULL);
 	goto out;
 }

@@ -408,10 +405,20 @@ static void acpi_hotplug_unsupported(acp
 	acpi_evaluate_hotplug_ost(handle, type, ost_status, NULL);
 }
 
+/**
+ * acpi_bus_hot_remove_device: Hot-remove a device and its children.
+ * @context: Address of the ACPI device object to hot-remove.
+ */
+void acpi_bus_hot_remove_device(void *context)
+{
+	acpi_bus_device_eject(context, ACPI_NOTIFY_EJECT_REQUEST);
+}
+
 static void acpi_hotplug_notify_cb(acpi_handle handle, u32 type, void *data)
 {
 	acpi_osd_exec_callback callback;
 	struct acpi_scan_handler *handler = data;
+	struct acpi_device *adev;
 	acpi_status status;
 
 	if (!handler->hotplug.enabled)

@@ -428,44 +435,29 @@ static void acpi_hotplug_notify_cb(acpi_
 		break;
 	case ACPI_NOTIFY_EJECT_REQUEST:
 		acpi_handle_debug(handle, "ACPI_NOTIFY_EJECT_REQUEST event\n");
-		callback = acpi_bus_device_eject;
-		break;
+		status = acpi_bus_get_device(handle, &adev);
+		if (ACPI_FAILURE(status))
+			goto err_out;
+
+		get_device(&adev->dev);
+		callback = acpi_bus_hot_remove_device;
+		status = acpi_os_hotplug_execute(callback, adev);
+		if (ACPI_SUCCESS(status))
+			return;
+
+		put_device(&adev->dev);
+		goto err_out;
 	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);
-}
-
-void __acpi_bus_hot_remove_device(struct acpi_device *device, u32 ost_src)
-{
-	acpi_handle handle = device->handle;
-	int error;
-
-	lock_device_hotplug();
-	mutex_lock(&acpi_scan_lock);
-
-	error = acpi_scan_hot_remove(device);
-	if (error && handle)
-		acpi_evaluate_hotplug_ost(handle, ost_src,
-					  ACPI_OST_SC_NON_SPECIFIC_FAILURE,
-					  NULL);
-
-	mutex_unlock(&acpi_scan_lock);
-	unlock_device_hotplug();
-}
+	if (ACPI_SUCCESS(status))
+		return;
 
-/**
- * acpi_bus_hot_remove_device: Hot-remove a device and its children.
- * @context: Address of the ACPI device object to hot-remove.
- */
-void acpi_bus_hot_remove_device(void *context)
-{
-	__acpi_bus_hot_remove_device(context, ACPI_NOTIFY_EJECT_REQUEST);
+ err_out:
+	acpi_evaluate_hotplug_ost(handle, type,
+				  ACPI_OST_SC_NON_SPECIFIC_FAILURE, NULL);
 }
 
 static ssize_t real_power_state_show(struct device *dev,

@@ -496,7 +488,7 @@ static DEVICE_ATTR(power_state, 0444, po
 
 static void acpi_eject_store_work(void *context)
 {
-	__acpi_bus_hot_remove_device(context, ACPI_OST_EC_OSPM_EJECT);
+	acpi_bus_device_eject(context, ACPI_OST_EC_OSPM_EJECT);
 }
 
 static ssize_t

Rafael J. Wysocki

author

2013-11-05 23:32 UTC #23 lore raw

cover More ACPI hotplug updates (was: [Update][PATCH 0/6] ACPI scan and hotplug fixes)

In reply to Rafael J. Wysocki

Quoted 6 lines · Rafael J. Wysocki

Quoted 5 lines · Rafael J. Wysocki
Hi,
The following three patches fix some issues that we have in the common ACPI hotplug infrastructure.

Quoted 7 lines

Quoted 1 line
If anyone sees any problems with them, please let me know.
Well, the last patch from the last version didn't really do the right thing in my opinion, so new series follows.
I actually would like to squeeze it into 3.13 if there are no objections, since at least patch [3/6] is kind of urgent.

Three more patches for 3.13 if people don't mind, on top of the previous series of six. [2/3] actually is a fix, the other two are more like cleanups, but they are worth doing at this point if no one sees any problems with them.

This is the last batch for 3.13, I promise.

Thanks!

Signature · 2 lines

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

Rafael J. Wysocki

author

2013-11-05 23:34 UTC #24 lore raw

PATCH 1/3 ACPI / hotplug: Carry out PCI root eject directly

From: Rafael J. Wysocki <[email protected]>

Since _handle_hotplug_event_root() is run from the ACPI hotplug workqueue, it doesn't need to queue up a work item to eject a PCI host bridge on the same workqueue. Instead, it can just carry out the eject by calling acpi_bus_device_eject() directly, so make that happen.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/acpi/internal.h |    2 +-
 drivers/acpi/pci_root.c |    6 +-----
 drivers/acpi/scan.c     |    4 ++--
 3 files changed, 4 insertions(+), 8 deletions(-)

drivers/acpi/internal.h +1 -1

Index: linux-pm/drivers/acpi/internal.h
===================================================================
@@ -92,7 +92,7 @@ void acpi_device_add_finalize(struct acp
 void acpi_free_pnp_ids(struct acpi_device_pnp *pnp);
 int acpi_bind_one(struct device *dev, acpi_handle handle);
 int acpi_unbind_one(struct device *dev);
-void acpi_bus_hot_remove_device(void *context);
+void acpi_bus_device_eject(struct acpi_device *device, u32 ost_src);
 
 /* --------------------------------------------------------------------------
                                   Power Resource

drivers/acpi/pci_root.c +1 -5

Index: linux-pm/drivers/acpi/pci_root.c
===================================================================
@@ -594,12 +594,8 @@ static void handle_root_bridge_insertion
 
 static void handle_root_bridge_removal(struct acpi_device *device)
 {
-	acpi_status status;
-
 	get_device(&device->dev);
-	status = acpi_os_hotplug_execute(acpi_bus_hot_remove_device, device);
-	if (ACPI_FAILURE(status))
-		put_device(&device->dev);
+	acpi_bus_device_eject(device, ACPI_NOTIFY_EJECT_REQUEST);
 }
 
 static void _handle_hotplug_event_root(struct work_struct *work)

drivers/acpi/scan.c +2 -2

Index: linux-pm/drivers/acpi/scan.c
===================================================================
@@ -286,7 +286,7 @@ static int acpi_scan_hot_remove(struct a
 	return 0;
 }
 
-static void acpi_bus_device_eject(struct acpi_device *device, u32 ost_src)
+void acpi_bus_device_eject(struct acpi_device *device, u32 ost_src)
 {
 	acpi_handle handle = device->handle;
 	struct acpi_scan_handler *handler;

@@ -410,7 +410,7 @@ static void acpi_hotplug_unsupported(acp
  * acpi_bus_hot_remove_device: Hot-remove a device and its children.
  * @context: Address of the ACPI device object to hot-remove.
  */
-void acpi_bus_hot_remove_device(void *context)
+static void acpi_bus_hot_remove_device(void *context)
 {
 	acpi_bus_device_eject(context, ACPI_NOTIFY_EJECT_REQUEST);
 }

Rafael J. Wysocki

author

2013-11-06 01:29 UTC #25 lore raw

PATCH 1/3 ACPI / hotplug: Carry out PCI root eject directly

From: Rafael J. Wysocki <[email protected]>
Subject: ACPI / hotplug: Carry out PCI root eject directly

Signed-off-by Rafael J. Wysocki <[email protected]>

---

It looks like I sent this too early. :-/

Obviously, acpi_bus_device_eject() cannot be called under acpi_scan_lock, so that lock has to be dropped before calling that function.

Patch [3/3] doesn't apply any more after this modification, so I'll send an update of it too.

Thanks,
Rafael

---
 drivers/acpi/internal.h |    2 +-
 drivers/acpi/pci_root.c |   24 ++++++++++--------------
 drivers/acpi/scan.c     |    4 ++--
 3 files changed, 13 insertions(+), 17 deletions(-)

drivers/acpi/internal.h +1 -1

Index: linux-pm/drivers/acpi/internal.h
===================================================================
@@ -92,7 +92,7 @@ void acpi_device_add_finalize(struct acp
 void acpi_free_pnp_ids(struct acpi_device_pnp *pnp);
 int acpi_bind_one(struct device *dev, acpi_handle handle);
 int acpi_unbind_one(struct device *dev);
-void acpi_bus_hot_remove_device(void *context);
+void acpi_bus_device_eject(struct acpi_device *device, u32 ost_src);
 
 /* --------------------------------------------------------------------------
                                   Power Resource

drivers/acpi/pci_root.c +10 ~~-14~~

Index: linux-pm/drivers/acpi/pci_root.c
===================================================================
@@ -592,16 +592,6 @@ static void handle_root_bridge_insertion
 		acpi_handle_err(handle, "cannot add bridge to acpi list\n");
 }
 
-static void handle_root_bridge_removal(struct acpi_device *device)
-{
-	acpi_status status;
-
-	get_device(&device->dev);
-	status = acpi_os_hotplug_execute(acpi_bus_hot_remove_device, device);
-	if (ACPI_FAILURE(status))
-		put_device(&device->dev);
-}
-
 static void _handle_hotplug_event_root(struct work_struct *work)
 {
 	struct acpi_pci_root *root;

@@ -612,6 +602,7 @@ static void _handle_hotplug_event_root(s
 	hp_work = container_of(work, struct acpi_hp_work, work);
 	handle = hp_work->handle;
 	type = hp_work->type;
+	kfree(hp_work); /* allocated in handle_hotplug_event_bridge */
 
 	acpi_scan_lock_acquire();

@@ -641,9 +632,15 @@ static void _handle_hotplug_event_root(s
 		/* request device eject */
 		acpi_handle_printk(KERN_DEBUG, handle,
 				   "Device eject notify on %s\n", __func__);
-		if (root)
-			handle_root_bridge_removal(root->device);
-		break;
+		if (!root)
+			break;
+
+		get_device(&root->device->dev);
+
+		acpi_scan_lock_release();
+
+		acpi_bus_device_eject(root->device, ACPI_NOTIFY_EJECT_REQUEST);
+		return;
 	default:
 		acpi_handle_warn(handle,
 				 "notify_handler: unknown event type 0x%x\n",

@@ -652,7 +649,6 @@ static void _handle_hotplug_event_root(s
 	}
 
 	acpi_scan_lock_release();
-	kfree(hp_work); /* allocated in handle_hotplug_event_bridge */
 }
 
 static void handle_hotplug_event_root(acpi_handle handle, u32 type,

drivers/acpi/scan.c +2 -2

Index: linux-pm/drivers/acpi/scan.c
===================================================================
@@ -285,7 +285,7 @@ static int acpi_scan_hot_remove(struct a
 	return 0;
 }
 
-static void acpi_bus_device_eject(struct acpi_device *device, u32 ost_src)
+void acpi_bus_device_eject(struct acpi_device *device, u32 ost_src)
 {
 	acpi_handle handle = device->handle;
 	struct acpi_scan_handler *handler;

@@ -409,7 +409,7 @@ static void acpi_hotplug_unsupported(acp
  * acpi_bus_hot_remove_device: Hot-remove a device and its children.
  * @context: Address of the ACPI device object to hot-remove.
  */
-void acpi_bus_hot_remove_device(void *context)
+static void acpi_bus_hot_remove_device(void *context)
 {
 	acpi_bus_device_eject(context, ACPI_NOTIFY_EJECT_REQUEST);
 }

Rafael J. Wysocki

author

2013-11-05 23:36 UTC #26 lore raw

PATCH 2/3 ACPI / hotplug: Do not execute "insert in progress" _OST

In reply to Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

According to the ACPI spec (5.0, Section 6.3.5), the "Device insertion in progress (pending)" (0x80) _OST status code is reserved for the "Insertion Processing" (0x200) source event which is "a result of an OSPM action". Specifically, it is not a notification, so that status code should not be used during notification processing, which unfortunately is done by acpi_scan_bus_device_check().

For this reason, drop the ACPI_OST_SC_INSERT_IN_PROGRESS _OST status evaluation from there (it was a mistake to put it in there in the first place).

Signed-off-by Rafael J. Wysocki <[email protected]>

---

Material for stable, in my opinion.

Thanks,
Rafael

---
 drivers/acpi/scan.c |    2 --
 1 file changed, 2 deletions(-)

drivers/acpi/scan.c +0 -2

Index: linux-pm/drivers/acpi/scan.c
===================================================================
@@ -344,8 +344,6 @@ static void acpi_scan_bus_device_check(a
 			goto out;
 		}
 	}
-	acpi_evaluate_hotplug_ost(handle, ost_source,
-				  ACPI_OST_SC_INSERT_IN_PROGRESS, NULL);
 	error = acpi_bus_scan(handle);
 	if (error) {
 		acpi_handle_warn(handle, "Namespace scan failure\n");

Rafael J. Wysocki

author

2013-11-05 23:35 UTC #27 lore raw

PATCH 3/3 ACPI / hotplug: Consolidate deferred execution of ACPI hotplug routines

In reply to Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

There are two different interfaces for queuing up work items on the ACPI hotplug workqueue, alloc_acpi_hp_work() used by PCI and PCI host bridge hotplug code and acpi_os_hotplug_execute() used by the common ACPI hotplug code and docking stations. They both are somewhat cumbersome to use and work slightly differently.

The users of alloc_acpi_hp_work() have to submit a work function that will extract the necessary data items from a struct acpi_hp_work object allocated by alloc_acpi_hp_work() and then will free that object, while it would be more straightforward to simply use a work function with one more argument and let the interface take care of the execution details.

The users of acpi_os_hotplug_execute() also have to deal with the fact that it takes only one argument in addition to the work function pointer, although acpi_os_execute_deferred() actually takes care of the allocation and freeing of memory, so it would have been able to pass more arguments to the work function if it hadn't been constrained by the connection with acpi_os_execute().

Moreover, while alloc_acpi_hp_work() makes GFP_KERNEL memory allocations, which is correct, because hotplug work items are always queued up from process context, acpi_os_hotplug_execute() uses GFP_ATOMIC, as that is needed by acpi_os_execute(). Also, acpi_os_execute_deferred() queued up by it waits for the ACPI event workqueues to flush before calling the work function, while alloc_acpi_hp_work() obviously can't do anything like that. That leads to somewhat arbitrary differences in behavior between various ACPI hotplug code paths and has to be straightened up.

For this reason, replace both alloc_acpi_hp_work() and acpi_os_hotplug_execute() with a single interface, acpi_hotplug_execute(), combining their behavior and being more friendly to its users than any of the two.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/acpi/dock.c                |   25 +--------
 drivers/acpi/internal.h            |    2 
 drivers/acpi/osl.c                 |   96 +++++++++++++++++--------------------
 drivers/acpi/pci_root.c            |   14 +----
 drivers/acpi/scan.c                |   43 +++-------------
 drivers/pci/hotplug/acpiphp_glue.c |   18 ++----
 include/acpi/acpi_bus.h            |   12 +---
 include/acpi/platform/aclinux.h    |    3 -
 8 files changed, 71 insertions(+), 142 deletions(-)

drivers/acpi/osl.c +46 ~~-50~~

Index: linux-pm/drivers/acpi/osl.c
===================================================================
@@ -61,7 +61,6 @@ struct acpi_os_dpc {
 	acpi_osd_exec_callback function;
 	void *context;
 	struct work_struct work;
-	int wait;
 };
 
 #ifdef CONFIG_ACPI_CUSTOM_DSDT

@@ -1087,9 +1086,6 @@ static void acpi_os_execute_deferred(str
 {
 	struct acpi_os_dpc *dpc = container_of(work, struct acpi_os_dpc, work);
 
-	if (dpc->wait)
-		acpi_os_wait_events_complete();
-
 	dpc->function(dpc->context);
 	kfree(dpc);
 }

@@ -1109,8 +1105,8 @@ static void acpi_os_execute_deferred(str
  *
  ******************************************************************************/
 
-static acpi_status __acpi_os_execute(acpi_execute_type type,
-	acpi_osd_exec_callback function, void *context, int hp)
+acpi_status acpi_os_execute(acpi_execute_type type,
+			    acpi_osd_exec_callback function, void *context)
 {
 	acpi_status status = AE_OK;
 	struct acpi_os_dpc *dpc;

@@ -1137,20 +1133,11 @@ static acpi_status __acpi_os_execute(acp
 	dpc->context = context;
 
 	/*
-	 * We can't run hotplug code in keventd_wq/kacpid_wq/kacpid_notify_wq
-	 * because the hotplug code may call driver .remove() functions,
-	 * which invoke flush_scheduled_work/acpi_os_wait_events_complete
-	 * to flush these workqueues.
-	 *
 	 * To prevent lockdep from complaining unnecessarily, make sure that
 	 * there is a different static lockdep key for each workqueue by using
 	 * INIT_WORK() for each of them separately.
 	 */
-	if (hp) {
-		queue = kacpi_hotplug_wq;
-		dpc->wait = 1;
-		INIT_WORK(&dpc->work, acpi_os_execute_deferred);
-	} else if (type == OSL_NOTIFY_HANDLER) {
+	if (type == OSL_NOTIFY_HANDLER) {
 		queue = kacpi_notify_wq;
 		INIT_WORK(&dpc->work, acpi_os_execute_deferred);
 	} else {

@@ -1175,28 +1162,59 @@ static acpi_status __acpi_os_execute(acp
 	}
 	return status;
 }
+EXPORT_SYMBOL(acpi_os_execute);
 
-acpi_status acpi_os_execute(acpi_execute_type type,
-			    acpi_osd_exec_callback function, void *context)
+void acpi_os_wait_events_complete(void)
 {
-	return __acpi_os_execute(type, function, context, 0);
+	flush_workqueue(kacpid_wq);
+	flush_workqueue(kacpi_notify_wq);
 }
-EXPORT_SYMBOL(acpi_os_execute);
 
-acpi_status acpi_os_hotplug_execute(acpi_osd_exec_callback function,
-	void *context)
+struct acpi_hp_work {
+	struct work_struct work;
+	acpi_hp_callback func;
+	void *data;
+	u32 src;
+};
+
+static void acpi_hotplug_work_fn(struct work_struct *work)
 {
-	return __acpi_os_execute(0, function, context, 1);
+	struct acpi_hp_work *hpw = container_of(work, struct acpi_hp_work, work);
+
+	acpi_os_wait_events_complete();
+	hpw->func(hpw->data, hpw->src);
+	kfree(hpw);
 }
-EXPORT_SYMBOL(acpi_os_hotplug_execute);
 
-void acpi_os_wait_events_complete(void)
+acpi_status acpi_hotplug_execute(acpi_hp_callback func, void *data, u32 src)
 {
-	flush_workqueue(kacpid_wq);
-	flush_workqueue(kacpi_notify_wq);
+	struct acpi_hp_work *hpw;
+
+	ACPI_DEBUG_PRINT((ACPI_DB_EXEC,
+		  "Scheduling function [%p(%p, %u)] for deferred execution.\n",
+		  func, data, src));
+
+	hpw = kmalloc(sizeof(*hpw), GFP_KERNEL);
+	if (!hpw)
+		return AE_NO_MEMORY;
+
+	INIT_WORK(&hpw->work, acpi_hotplug_work_fn);
+	hpw->func = func;
+	hpw->data = data;
+	hpw->src = src;
+	/*
+	 * We can't run hotplug code in kacpid_wq/kacpid_notify_wq etc., because
+	 * the hotplug code may call driver .remove() functions, which may
+	 * invoke flush_scheduled_work()/acpi_os_wait_events_complete() to flush
+	 * these workqueues.
+	 */
+	if (!queue_work(kacpi_hotplug_wq, &hpw->work)) {
+		kfree(hpw);
+		return AE_ERROR;
+	}
+	return AE_OK;
 }
 
-EXPORT_SYMBOL(acpi_os_wait_events_complete);
 
 acpi_status
 acpi_os_create_semaphore(u32 max_units, u32 initial_units, acpi_handle * handle)

@@ -1845,25 +1863,3 @@ void acpi_os_set_prepare_extended_sleep(
 {
 	__acpi_os_prepare_extended_sleep = func;
 }
-
-
-void alloc_acpi_hp_work(acpi_handle handle, u32 type, void *context,
-			void (*func)(struct work_struct *work))
-{
-	struct acpi_hp_work *hp_work;
-	int ret;
-
-	hp_work = kmalloc(sizeof(*hp_work), GFP_KERNEL);
-	if (!hp_work)
-		return;
-
-	hp_work->handle = handle;
-	hp_work->type = type;
-	hp_work->context = context;
-
-	INIT_WORK(&hp_work->work, func);
-	ret = queue_work(kacpi_hotplug_wq, &hp_work->work);
-	if (!ret)
-		kfree(hp_work);
-}
-EXPORT_SYMBOL_GPL(alloc_acpi_hp_work);

include/acpi/acpi_bus.h +3 -9

Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
@@ -339,15 +339,6 @@ struct acpi_bus_event {
 	u32 data;
 };
 
-struct acpi_hp_work {
-	struct work_struct work;
-	acpi_handle handle;
-	u32 type;
-	void *context;
-};
-void alloc_acpi_hp_work(acpi_handle handle, u32 type, void *context,
-			void (*func)(struct work_struct *work));
-
 extern struct kobject *acpi_kobj;
 extern int acpi_bus_generate_netlink_event(const char*, const char*, u8, int);
 void acpi_bus_private_data_handler(acpi_handle, void *);

@@ -393,6 +384,9 @@ int acpi_match_device_ids(struct acpi_de
 int acpi_create_dir(struct acpi_device *);
 void acpi_remove_dir(struct acpi_device *);
 
+typedef void (*acpi_hp_callback)(void *data, u32 src);
+
+acpi_status acpi_hotplug_execute(acpi_hp_callback func, void *data, u32 src);
 
 /**
  * module_acpi_driver(acpi_driver) - Helper macro for registering an ACPI driver

include/acpi/platform/aclinux.h +0 -3

Index: linux-pm/include/acpi/platform/aclinux.h
===================================================================
@@ -243,9 +243,6 @@ void acpi_os_gpe_count(u32 gpe_number);
 
 void acpi_os_fixed_event_count(u32 fixed_event_number);
 
-acpi_status
-acpi_os_hotplug_execute(acpi_osd_exec_callback function, void *context);
-
 #endif				/* __KERNEL__ */
 
 #endif				/* __ACLINUX_H__ */

drivers/acpi/scan.c +8 ~~-35~~

104 lines not on this page · raw message

drivers/acpi/pci_root.c +3 ~~-11~~

Index: linux-pm/drivers/acpi/pci_root.c
===================================================================
@@ -598,16 +598,10 @@ static void handle_root_bridge_removal(s
 	acpi_bus_device_eject(device, ACPI_NOTIFY_EJECT_REQUEST);
 }
 
-static void _handle_hotplug_event_root(struct work_struct *work)
+static void hotplug_event_root(void *data, u32 type)
 {
+	acpi_handle handle = data;
 	struct acpi_pci_root *root;
-	struct acpi_hp_work *hp_work;
-	acpi_handle handle;
-	u32 type;
-
-	hp_work = container_of(work, struct acpi_hp_work, work);
-	handle = hp_work->handle;
-	type = hp_work->type;
 
 	acpi_scan_lock_acquire();

@@ -648,14 +642,12 @@ static void _handle_hotplug_event_root(s
 	}
 
 	acpi_scan_lock_release();
-	kfree(hp_work); /* allocated in handle_hotplug_event_bridge */
 }
 
 static void handle_hotplug_event_root(acpi_handle handle, u32 type,
 					void *context)
 {
-	alloc_acpi_hp_work(handle, type, context,
-				_handle_hotplug_event_root);
+	acpi_hotplug_execute(hotplug_event_root, handle, type);
 }
 
 static acpi_status __init

drivers/acpi/dock.c +3 ~~-22~~

Index: linux-pm/drivers/acpi/dock.c
===================================================================
@@ -669,39 +669,20 @@ static void dock_notify(struct dock_stat
 	}
 }
 
-struct dock_data {
-	struct dock_station *ds;
-	u32 event;
-};
-
-static void acpi_dock_deferred_cb(void *context)
+static void acpi_dock_deferred_cb(void *data, u32 event)
 {
-	struct dock_data *data = context;
-
 	acpi_scan_lock_acquire();
-	dock_notify(data->ds, data->event);
+	dock_notify(data, event);
 	acpi_scan_lock_release();
-	kfree(data);
 }
 
 static void dock_notify_handler(acpi_handle handle, u32 event, void *data)
 {
-	struct dock_data *dd;
-
 	if (event != ACPI_NOTIFY_BUS_CHECK && event != ACPI_NOTIFY_DEVICE_CHECK
 	   && event != ACPI_NOTIFY_EJECT_REQUEST)
 		return;
 
-	dd = kmalloc(sizeof(*dd), GFP_KERNEL);
-	if (dd) {
-		acpi_status status;
-
-		dd->ds = data;
-		dd->event = event;
-		status = acpi_os_hotplug_execute(acpi_dock_deferred_cb, dd);
-		if (ACPI_FAILURE(status))
-			kfree(dd);
-	}
+	acpi_hotplug_execute(acpi_dock_deferred_cb, data, event);
 }
 
 /**

drivers/pci/hotplug/acpiphp_glue.c +7 ~~-11~~

Index: linux-pm/drivers/pci/hotplug/acpiphp_glue.c
===================================================================
@@ -871,21 +871,17 @@ static void hotplug_event(acpi_handle ha
 		put_bridge(bridge);
 }
 
-static void hotplug_event_work(struct work_struct *work)
+static void hotplug_event_work(void *data, u32 type)
 {
-	struct acpiphp_context *context;
-	struct acpi_hp_work *hp_work;
+	struct acpiphp_context *context = data;
+	acpi_handle handle = context->handle;
 
-	hp_work = container_of(work, struct acpi_hp_work, work);
-	context = hp_work->context;
 	acpi_scan_lock_acquire();
 
-	hotplug_event(hp_work->handle, hp_work->type, context);
+	hotplug_event(handle, type, context);
 
 	acpi_scan_lock_release();
-	acpi_evaluate_hotplug_ost(hp_work->handle, hp_work->type,
-				  ACPI_OST_SC_SUCCESS, NULL);
-	kfree(hp_work); /* allocated in handle_hotplug_event() */
+	acpi_evaluate_hotplug_ost(handle, type, ACPI_OST_SC_SUCCESS, NULL);
 	put_bridge(context->func.parent);
 }

@@ -936,10 +932,10 @@ static void handle_hotplug_event(acpi_ha
 
 	mutex_lock(&acpiphp_context_lock);
 	context = acpiphp_get_context(handle);
-	if (context) {
+	if (context && !WARN_ON(context->handle != handle)) {
 		get_bridge(context->func.parent);
 		acpiphp_put_context(context);
-		alloc_acpi_hp_work(handle, type, context, hotplug_event_work);
+		acpi_hotplug_execute(hotplug_event_work, context, type);
 		mutex_unlock(&acpiphp_context_lock);
 		return;
 	}

drivers/acpi/internal.h +1 -1

Index: linux-pm/drivers/acpi/internal.h
===================================================================
@@ -92,7 +92,7 @@ void acpi_device_add_finalize(struct acp
 void acpi_free_pnp_ids(struct acpi_device_pnp *pnp);
 int acpi_bind_one(struct device *dev, acpi_handle handle);
 int acpi_unbind_one(struct device *dev);
-void acpi_bus_device_eject(struct acpi_device *device, u32 ost_src);
+void acpi_bus_device_eject(void *data, u32 ost_src);
 
 /* --------------------------------------------------------------------------
                                   Power Resource

Rafael J. Wysocki

author

2013-11-06 01:32 UTC #28 lore raw

PATCH 3/3 ACPI / hotplug: Consolidate deferred execution of ACPI hotplug routines

From: Rafael J. Wysocki <[email protected]>

Moreover, while alloc_acpi_hp_work() makes GFP_KERNEL memory allocations, which is correct, because hotplug work items are always queued up from process context, acpi_os_hotplug_execute() uses GFP_ATOMIC, as that is needed by acpi_os_execute(). Also, acpi_os_execute_deferred() queued up by it waits for the ACPI event workqueues to flush before executing the work function, whereas alloc_acpi_hp_work() can't do anything similar. That leads to somewhat arbitrary differences in behavior between various ACPI hotplug code paths and has to be straightened up.

Signed-off-by Rafael J. Wysocki <[email protected]>

---

Updated after the fixup of patch [1/3].

Thanks,
Rafael

---
 drivers/acpi/dock.c                |   25 +--------
 drivers/acpi/internal.h            |    2 
 drivers/acpi/osl.c                 |   96 +++++++++++++++++--------------------
 drivers/acpi/pci_root.c            |   14 +----
 drivers/acpi/scan.c                |   43 +++-------------
 drivers/pci/hotplug/acpiphp_glue.c |   18 ++----
 include/acpi/acpi_bus.h            |   12 +---
 include/acpi/platform/aclinux.h    |    3 -
 8 files changed, 71 insertions(+), 142 deletions(-)

drivers/acpi/osl.c +46 ~~-50~~

Index: linux-pm/drivers/acpi/osl.c
===================================================================
@@ -61,7 +61,6 @@ struct acpi_os_dpc {
 	acpi_osd_exec_callback function;
 	void *context;
 	struct work_struct work;
-	int wait;
 };
 
 #ifdef CONFIG_ACPI_CUSTOM_DSDT

@@ -1087,9 +1086,6 @@ static void acpi_os_execute_deferred(str
 {
 	struct acpi_os_dpc *dpc = container_of(work, struct acpi_os_dpc, work);
 
-	if (dpc->wait)
-		acpi_os_wait_events_complete();
-
 	dpc->function(dpc->context);
 	kfree(dpc);
 }

@@ -1109,8 +1105,8 @@ static void acpi_os_execute_deferred(str
  *
  ******************************************************************************/
 
-static acpi_status __acpi_os_execute(acpi_execute_type type,
-	acpi_osd_exec_callback function, void *context, int hp)
+acpi_status acpi_os_execute(acpi_execute_type type,
+			    acpi_osd_exec_callback function, void *context)
 {
 	acpi_status status = AE_OK;
 	struct acpi_os_dpc *dpc;

@@ -1137,20 +1133,11 @@ static acpi_status __acpi_os_execute(acp
 	dpc->context = context;
 
 	/*
-	 * We can't run hotplug code in keventd_wq/kacpid_wq/kacpid_notify_wq
-	 * because the hotplug code may call driver .remove() functions,
-	 * which invoke flush_scheduled_work/acpi_os_wait_events_complete
-	 * to flush these workqueues.
-	 *
 	 * To prevent lockdep from complaining unnecessarily, make sure that
 	 * there is a different static lockdep key for each workqueue by using
 	 * INIT_WORK() for each of them separately.
 	 */
-	if (hp) {
-		queue = kacpi_hotplug_wq;
-		dpc->wait = 1;
-		INIT_WORK(&dpc->work, acpi_os_execute_deferred);
-	} else if (type == OSL_NOTIFY_HANDLER) {
+	if (type == OSL_NOTIFY_HANDLER) {
 		queue = kacpi_notify_wq;
 		INIT_WORK(&dpc->work, acpi_os_execute_deferred);
 	} else {

@@ -1175,28 +1162,59 @@ static acpi_status __acpi_os_execute(acp
 	}
 	return status;
 }
+EXPORT_SYMBOL(acpi_os_execute);
 
-acpi_status acpi_os_execute(acpi_execute_type type,
-			    acpi_osd_exec_callback function, void *context)
+void acpi_os_wait_events_complete(void)
 {
-	return __acpi_os_execute(type, function, context, 0);
+	flush_workqueue(kacpid_wq);
+	flush_workqueue(kacpi_notify_wq);
 }
-EXPORT_SYMBOL(acpi_os_execute);
 
-acpi_status acpi_os_hotplug_execute(acpi_osd_exec_callback function,
-	void *context)
+struct acpi_hp_work {
+	struct work_struct work;
+	acpi_hp_callback func;
+	void *data;
+	u32 src;
+};
+
+static void acpi_hotplug_work_fn(struct work_struct *work)
 {
-	return __acpi_os_execute(0, function, context, 1);
+	struct acpi_hp_work *hpw = container_of(work, struct acpi_hp_work, work);
+
+	acpi_os_wait_events_complete();
+	hpw->func(hpw->data, hpw->src);
+	kfree(hpw);
 }
-EXPORT_SYMBOL(acpi_os_hotplug_execute);
 
-void acpi_os_wait_events_complete(void)
+acpi_status acpi_hotplug_execute(acpi_hp_callback func, void *data, u32 src)
 {
-	flush_workqueue(kacpid_wq);
-	flush_workqueue(kacpi_notify_wq);
+	struct acpi_hp_work *hpw;
+
+	ACPI_DEBUG_PRINT((ACPI_DB_EXEC,
+		  "Scheduling function [%p(%p, %u)] for deferred execution.\n",
+		  func, data, src));
+
+	hpw = kmalloc(sizeof(*hpw), GFP_KERNEL);
+	if (!hpw)
+		return AE_NO_MEMORY;
+
+	INIT_WORK(&hpw->work, acpi_hotplug_work_fn);
+	hpw->func = func;
+	hpw->data = data;
+	hpw->src = src;
+	/*
+	 * We can't run hotplug code in kacpid_wq/kacpid_notify_wq etc., because
+	 * the hotplug code may call driver .remove() functions, which may
+	 * invoke flush_scheduled_work()/acpi_os_wait_events_complete() to flush
+	 * these workqueues.
+	 */
+	if (!queue_work(kacpi_hotplug_wq, &hpw->work)) {
+		kfree(hpw);
+		return AE_ERROR;
+	}
+	return AE_OK;
 }
 
-EXPORT_SYMBOL(acpi_os_wait_events_complete);
 
 acpi_status
 acpi_os_create_semaphore(u32 max_units, u32 initial_units, acpi_handle * handle)

@@ -1845,25 +1863,3 @@ void acpi_os_set_prepare_extended_sleep(
 {
 	__acpi_os_prepare_extended_sleep = func;
 }
-
-
-void alloc_acpi_hp_work(acpi_handle handle, u32 type, void *context,
-			void (*func)(struct work_struct *work))
-{
-	struct acpi_hp_work *hp_work;
-	int ret;
-
-	hp_work = kmalloc(sizeof(*hp_work), GFP_KERNEL);
-	if (!hp_work)
-		return;
-
-	hp_work->handle = handle;
-	hp_work->type = type;
-	hp_work->context = context;
-
-	INIT_WORK(&hp_work->work, func);
-	ret = queue_work(kacpi_hotplug_wq, &hp_work->work);
-	if (!ret)
-		kfree(hp_work);
-}
-EXPORT_SYMBOL_GPL(alloc_acpi_hp_work);

include/acpi/acpi_bus.h +3 -9

Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
@@ -339,15 +339,6 @@ struct acpi_bus_event {
 	u32 data;
 };
 
-struct acpi_hp_work {
-	struct work_struct work;
-	acpi_handle handle;
-	u32 type;
-	void *context;
-};
-void alloc_acpi_hp_work(acpi_handle handle, u32 type, void *context,
-			void (*func)(struct work_struct *work));
-
 extern struct kobject *acpi_kobj;
 extern int acpi_bus_generate_netlink_event(const char*, const char*, u8, int);
 void acpi_bus_private_data_handler(acpi_handle, void *);

@@ -393,6 +384,9 @@ int acpi_match_device_ids(struct acpi_de
 int acpi_create_dir(struct acpi_device *);
 void acpi_remove_dir(struct acpi_device *);
 
+typedef void (*acpi_hp_callback)(void *data, u32 src);
+
+acpi_status acpi_hotplug_execute(acpi_hp_callback func, void *data, u32 src);
 
 /**
  * module_acpi_driver(acpi_driver) - Helper macro for registering an ACPI driver

include/acpi/platform/aclinux.h +0 -3

Index: linux-pm/include/acpi/platform/aclinux.h
===================================================================
@@ -243,9 +243,6 @@ void acpi_os_gpe_count(u32 gpe_number);
 
 void acpi_os_fixed_event_count(u32 fixed_event_number);
 
-acpi_status
-acpi_os_hotplug_execute(acpi_osd_exec_callback function, void *context);
-
 #endif				/* __KERNEL__ */
 
 #endif				/* __ACLINUX_H__ */

drivers/acpi/scan.c +8 ~~-35~~

104 lines not on this page · raw message

drivers/acpi/pci_root.c +3 ~~-11~~

Index: linux-pm/drivers/acpi/pci_root.c
===================================================================
@@ -592,17 +592,10 @@ static void handle_root_bridge_insertion
 		acpi_handle_err(handle, "cannot add bridge to acpi list\n");
 }
 
-static void _handle_hotplug_event_root(struct work_struct *work)
+static void hotplug_event_root(void *data, u32 type)
 {
+	acpi_handle handle = data;
 	struct acpi_pci_root *root;
-	struct acpi_hp_work *hp_work;
-	acpi_handle handle;
-	u32 type;
-
-	hp_work = container_of(work, struct acpi_hp_work, work);
-	handle = hp_work->handle;
-	type = hp_work->type;
-	kfree(hp_work); /* allocated in handle_hotplug_event_bridge */
 
 	acpi_scan_lock_acquire();

@@ -654,8 +647,7 @@ static void _handle_hotplug_event_root(s
 static void handle_hotplug_event_root(acpi_handle handle, u32 type,
 					void *context)
 {
-	alloc_acpi_hp_work(handle, type, context,
-				_handle_hotplug_event_root);
+	acpi_hotplug_execute(hotplug_event_root, handle, type);
 }
 
 static acpi_status __init

drivers/acpi/dock.c +3 ~~-22~~

Index: linux-pm/drivers/acpi/dock.c
===================================================================
@@ -669,39 +669,20 @@ static void dock_notify(struct dock_stat
 	}
 }
 
-struct dock_data {
-	struct dock_station *ds;
-	u32 event;
-};
-
-static void acpi_dock_deferred_cb(void *context)
+static void acpi_dock_deferred_cb(void *data, u32 event)
 {
-	struct dock_data *data = context;
-
 	acpi_scan_lock_acquire();
-	dock_notify(data->ds, data->event);
+	dock_notify(data, event);
 	acpi_scan_lock_release();
-	kfree(data);
 }
 
 static void dock_notify_handler(acpi_handle handle, u32 event, void *data)
 {
-	struct dock_data *dd;
-
 	if (event != ACPI_NOTIFY_BUS_CHECK && event != ACPI_NOTIFY_DEVICE_CHECK
 	   && event != ACPI_NOTIFY_EJECT_REQUEST)
 		return;
 
-	dd = kmalloc(sizeof(*dd), GFP_KERNEL);
-	if (dd) {
-		acpi_status status;
-
-		dd->ds = data;
-		dd->event = event;
-		status = acpi_os_hotplug_execute(acpi_dock_deferred_cb, dd);
-		if (ACPI_FAILURE(status))
-			kfree(dd);
-	}
+	acpi_hotplug_execute(acpi_dock_deferred_cb, data, event);
 }
 
 /**

drivers/pci/hotplug/acpiphp_glue.c +7 ~~-11~~

Index: linux-pm/drivers/pci/hotplug/acpiphp_glue.c
===================================================================
@@ -871,21 +871,17 @@ static void hotplug_event(acpi_handle ha
 		put_bridge(bridge);
 }
 
-static void hotplug_event_work(struct work_struct *work)
+static void hotplug_event_work(void *data, u32 type)
 {
-	struct acpiphp_context *context;
-	struct acpi_hp_work *hp_work;
+	struct acpiphp_context *context = data;
+	acpi_handle handle = context->handle;
 
-	hp_work = container_of(work, struct acpi_hp_work, work);
-	context = hp_work->context;
 	acpi_scan_lock_acquire();
 
-	hotplug_event(hp_work->handle, hp_work->type, context);
+	hotplug_event(handle, type, context);
 
 	acpi_scan_lock_release();
-	acpi_evaluate_hotplug_ost(hp_work->handle, hp_work->type,
-				  ACPI_OST_SC_SUCCESS, NULL);
-	kfree(hp_work); /* allocated in handle_hotplug_event() */
+	acpi_evaluate_hotplug_ost(handle, type, ACPI_OST_SC_SUCCESS, NULL);
 	put_bridge(context->func.parent);
 }

@@ -936,10 +932,10 @@ static void handle_hotplug_event(acpi_ha
 
 	mutex_lock(&acpiphp_context_lock);
 	context = acpiphp_get_context(handle);
-	if (context) {
+	if (context && !WARN_ON(context->handle != handle)) {
 		get_bridge(context->func.parent);
 		acpiphp_put_context(context);
-		alloc_acpi_hp_work(handle, type, context, hotplug_event_work);
+		acpi_hotplug_execute(hotplug_event_work, context, type);
 		mutex_unlock(&acpiphp_context_lock);
 		return;
 	}

drivers/acpi/internal.h +1 -1

Index: linux-pm/drivers/acpi/internal.h
===================================================================
@@ -92,7 +92,7 @@ void acpi_device_add_finalize(struct acp
 void acpi_free_pnp_ids(struct acpi_device_pnp *pnp);
 int acpi_bind_one(struct device *dev, acpi_handle handle);
 int acpi_unbind_one(struct device *dev);
-void acpi_bus_device_eject(struct acpi_device *device, u32 ost_src);
+void acpi_bus_device_eject(void *data, u32 ost_src);
 
 /* --------------------------------------------------------------------------
                                   Power Resource