ACPI: New eject flow to remove devices cautiously

Here we change offline/online handling in device hotplug by sending change events to userland as notification so that userland can have control and determine when will be a good time to put them offline/online based on current workload. In this approach the real offline/online opertions are handed over to userland so that userland can have more time to prepare before any device change actually happens.

All child devices under the ejection target are traversed and notified hierarchically based on ACPI namespace in ascending order when an eject event happens.

Signed-off-by Chester Lin <[email protected]>

Quoted 11 lines · CET Chester Lin

Here we change offline/online handling in device hotplug by sending change events to userland as notification so that userland can have control and determine when will be a good time to put them offline/online based on current workload. In this approach the real offline/online opertions are handed over to userland so that userland can have more time to prepare before any device change actually happens.

All child devices under the ejection target are traversed and notified hierarchically based on ACPI namespace in ascending order when an eject event happens.

Quoted 1 line

Signed-off-by Chester Lin <[email protected]>

So you replace the old flow with the new one and make the new one mandatory AFAICS.

Thus if anyone has relied on the old flow, they now need to switch over.

This is unfriendly and generally unwelcome, so please avoid making changes like that.

Instead, I would consider adding a device attribute to allow user space to opt in for getting offline notifications for specific individual devices (by setting that attribute user space would tell the kernel that it wants to get offline notifications for the device in question and it would take care of offlining it as needed).

Add an eject_show attribute for users to monitor current status because sometimes it could take time to finish an ejection so we need to know whether it is still in progress or not. For userspace who might need to cancel an onging ejection, we also offer an option in eject_store.

Signed-off-by Chester Lin <[email protected]>

---
 Documentation/ABI/testing/sysfs-bus-acpi |  9 ++-
 drivers/acpi/device_sysfs.c              | 94 +++++++++++++++++++++---
 drivers/acpi/internal.h                  |  4 +-
 drivers/acpi/scan.c                      | 38 +++++++++-
 4 files changed, 129 insertions(+), 16 deletions(-)

Documentation/ABI/testing/sysfs-bus-acpi +0 -0

index e7898cfe5fb1..32fdf4af962e 100644
@@ -53,9 +53,12 @@ What:		/sys/bus/acpi/devices/.../eject
 Date:		December 2006
 Contact:	Rafael J. Wysocki <[email protected]>

Documentation/ABI/testing/sysfs-bus-acpi +0 -0

 What:		/sys/bus/acpi/devices/.../status
 Date:		Jan, 2014

drivers/acpi/device_sysfs.c +84 -10

index 96869f1538b9..6801b268fe9d 100644
@@ -365,17 +365,13 @@ static ssize_t power_state_show(struct device *dev,
 
 static DEVICE_ATTR_RO(power_state);
 
-static ssize_t
-acpi_eject_store(struct device *d, struct device_attribute *attr,
-		const char *buf, size_t count)
+static ssize_t eject_show(struct device *d,
+				struct device_attribute *attr, char *buf)
 {
 	struct acpi_device *acpi_device = to_acpi_device(d);
 	acpi_object_type not_used;
 	acpi_status status;
 
-	if (!count || buf[0] != '1')
-		return -EINVAL;
-
 	if ((!acpi_device->handler || !acpi_device->handler->hotplug.enabled)
 	    && !acpi_device->driver)
 		return -ENODEV;

@@ -384,18 +380,96 @@ acpi_eject_store(struct device *d, struct device_attribute *attr,
 	if (ACPI_FAILURE(status) || !acpi_device->flags.ejectable)
 		return -ENODEV;
 
+	return sprintf(buf, "%s\n", acpi_eject_status_string(acpi_device));
+}
+
+static ssize_t
+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 eject_data *eject_node = NULL;
+	acpi_object_type not_used;
+	acpi_status status;
+	bool cancel_eject = false;
+	ssize_t ret;
+
+	if (!count)
+		return -EINVAL;
+
+	switch (buf[0]) {
+	case '1':
+		break;
+	case '2':
+		acpi_scan_lock_acquire();
+		eject_node = (struct eject_data *)acpi_device->eject_stat;
+
+		if (!eject_node) {
+			acpi_scan_lock_release();
+			ret = -EINVAL;
+			goto eject_end;
+		}
+
+		/*
+		 * Find a root to start cancellation from the top
+		 */
+		if (eject_node->base.root_handle) {
+			acpi_device = acpi_bus_get_acpi_device(
+					eject_node->base.root_handle);
+
+			if (acpi_device)
+				eject_node =
+				   (struct eject_data *)acpi_device->eject_stat;
+			else
+				eject_node = NULL;
+
+		}
+
+		if (eject_node &&
+		   (eject_node->status == ACPI_EJECT_STATUS_GOING_OFFLINE ||
+		    eject_node->status == ACPI_EJECT_STATUS_READY_REMOVE)) {
+			eject_node->status = ACPI_EJECT_STATUS_CANCEL;
+			cancel_eject = true;
+		}
+
+		acpi_scan_lock_release();
+		if (cancel_eject)
+			break;
+	default:
+		ret = -EINVAL;
+		goto eject_end;
+	};
+
+	if ((!acpi_device->handler || !acpi_device->handler->hotplug.enabled)
+	    && !acpi_device->driver) {
+		ret = -ENODEV;
+		goto eject_end;
+	}
+
+	status = acpi_get_type(acpi_device->handle, &not_used);
+	if (ACPI_FAILURE(status) || !acpi_device->flags.ejectable) {
+		ret = -ENODEV;
+		goto eject_end;
+	}
+
 	get_device(&acpi_device->dev);
 	status = acpi_hotplug_schedule(acpi_device, ACPI_OST_EC_OSPM_EJECT);
-	if (ACPI_SUCCESS(status))
-		return count;
+	if (ACPI_SUCCESS(status)) {
+		ret = count;
+		goto eject_end;
+	}
 
 	put_device(&acpi_device->dev);
 	acpi_evaluate_ost(acpi_device->handle, ACPI_OST_EC_OSPM_EJECT,
 			  ACPI_OST_SC_NON_SPECIFIC_FAILURE, NULL);
-	return status == AE_NO_MEMORY ? -ENOMEM : -EAGAIN;
+	ret = (status == AE_NO_MEMORY) ? -ENOMEM : -EAGAIN;
+
+eject_end:
+	return ret;
+
 }
 
-static DEVICE_ATTR(eject, 0200, NULL, acpi_eject_store);
+static DEVICE_ATTR_RW(eject);
 
 static ssize_t
 acpi_device_hid_show(struct device *dev, struct device_attribute *attr, char *buf)

drivers/acpi/internal.h +3 -1

index 8154690b872b..e5d526402188 100644
@@ -265,7 +265,8 @@ enum acpi_eject_status {
 	ACPI_EJECT_STATUS_NONE = 0,
 	ACPI_EJECT_STATUS_GOING_OFFLINE,
 	ACPI_EJECT_STATUS_READY_REMOVE,
-	ACPI_EJECT_STATUS_FAIL
+	ACPI_EJECT_STATUS_FAIL,
+	ACPI_EJECT_STATUS_CANCEL
 };
 
 enum acpi_eject_node_type {

@@ -286,5 +287,6 @@ struct eject_data {
 };
 
 void acpi_eject_retry(struct acpi_device *adev);
+char *acpi_eject_status_string(struct acpi_device *adev);
 
 #endif /* _ACPI_INTERNAL_H_ */

drivers/acpi/scan.c +3 -2

index 13f16b6ad7a2..90983c067410 100644
@@ -366,8 +366,9 @@ static int acpi_scan_offline_check(struct acpi_device *device)
 			return -EBUSY;
 		}
 
-		if (eject_obj->status == ACPI_EJECT_STATUS_FAIL) {
-			dev_warn(&device->dev, "Eject failed. Recover all.\n");
+		if (eject_obj->status == ACPI_EJECT_STATUS_FAIL ||
+		    eject_obj->status == ACPI_EJECT_STATUS_CANCEL) {
+			dev_warn(&device->dev, "Eject stopped. Recover all.\n");
 			acpi_scan_notify_online(device);
 			return -EAGAIN;
 		}

drivers/acpi/scan.c +14 -0

+
+	if (eject_obj) {
+		switch (eject_obj->status) {
+		case ACPI_EJECT_STATUS_NONE:
+			break;
+		case ACPI_EJECT_STATUS_GOING_OFFLINE:
+			status_string = "going offline";
+			break;
+		case ACPI_EJECT_STATUS_READY_REMOVE:
+			status_string = "ready to remove";
+			break;
+		case ACPI_EJECT_STATUS_FAIL:
+			status_string = "failure";
+			break;

Quoted 20 lines · Chester Lin

Add an eject_show attribute for users to monitor current status because sometimes it could take time to finish an ejection so we need to know whether it is still in progress or not. For userspace who might need to cancel an onging ejection, we also offer an option in eject_store.

Signed-off-by Chester Lin <[email protected]>

---
 Documentation/ABI/testing/sysfs-bus-acpi |  9 ++-
 drivers/acpi/device_sysfs.c              | 94 +++++++++++++++++++++---
 drivers/acpi/internal.h                  |  4 +-
 drivers/acpi/scan.c                      | 38 +++++++++-
 4 files changed, 129 insertions(+), 16 deletions(-)

Documentation/ABI/testing/sysfs-bus-acpi +0 -0

index e7898cfe5fb1..32fdf4af962e 100644
@@ -53,9 +53,12 @@ What:		/sys/bus/acpi/devices/.../eject
 Date:		December 2006
 Contact:	Rafael J. Wysocki <[email protected]>

Documentation/ABI/testing/sysfs-bus-acpi What: /sys/bus/acpi/devices/.../eject · line 61

Quoted 10 lines

Documentation/ABI/testing/sysfs-bus-acpi +6 -3

 Description:
-		Writing 1 to this attribute will trigger hot removal of
-		this device object.  This file exists for every device
-		object that has _EJ0 method.
+		(R) Allows users to read eject status of the device object.
+		(W) Writing 1 to this attribute will trigger hot removal of
+		this device object. Writing 2 to this attribute will cancel hot
+		removal work if it's still in offline process and the original
+		state of this device object will be recovered. This file exists
+		for every device object that has _EJ0 method.

Oh that's going to cause problems :)

Why not just have a new file, "cancel_eject" that you can write to, to cancel the eject happening?

That way you don't have an odd "state" that this file needs to keep track of.

And what happens if you write a '2' here when eject is not happening? It didn't look like your code handled that state well.

Quoted 169 lines

Documentation/ABI/testing/sysfs-bus-acpi +0 -0

 What:		/sys/bus/acpi/devices/.../status
 Date:		Jan, 2014

drivers/acpi/device_sysfs.c +84 -10

index 96869f1538b9..6801b268fe9d 100644
@@ -365,17 +365,13 @@ static ssize_t power_state_show(struct device *dev,
 
 static DEVICE_ATTR_RO(power_state);
 
-static ssize_t
-acpi_eject_store(struct device *d, struct device_attribute *attr,
-		const char *buf, size_t count)
+static ssize_t eject_show(struct device *d,
+				struct device_attribute *attr, char *buf)
 {
 	struct acpi_device *acpi_device = to_acpi_device(d);
 	acpi_object_type not_used;
 	acpi_status status;
 
-	if (!count || buf[0] != '1')
-		return -EINVAL;
-
 	if ((!acpi_device->handler || !acpi_device->handler->hotplug.enabled)
 	    && !acpi_device->driver)
 		return -ENODEV;

@@ -384,18 +380,96 @@ acpi_eject_store(struct device *d, struct device_attribute *attr,
 	if (ACPI_FAILURE(status) || !acpi_device->flags.ejectable)
 		return -ENODEV;
 
+	return sprintf(buf, "%s\n", acpi_eject_status_string(acpi_device));
+}
+
+static ssize_t
+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 eject_data *eject_node = NULL;
+	acpi_object_type not_used;
+	acpi_status status;
+	bool cancel_eject = false;
+	ssize_t ret;
+
+	if (!count)
+		return -EINVAL;
+
+	switch (buf[0]) {
+	case '1':
+		break;
+	case '2':
+		acpi_scan_lock_acquire();
+		eject_node = (struct eject_data *)acpi_device->eject_stat;
+
+		if (!eject_node) {
+			acpi_scan_lock_release();
+			ret = -EINVAL;
+			goto eject_end;
+		}
+
+		/*
+		 * Find a root to start cancellation from the top
+		 */
+		if (eject_node->base.root_handle) {
+			acpi_device = acpi_bus_get_acpi_device(
+					eject_node->base.root_handle);
+
+			if (acpi_device)
+				eject_node =
+				   (struct eject_data *)acpi_device->eject_stat;
+			else
+				eject_node = NULL;
+
+		}
+
+		if (eject_node &&
+		   (eject_node->status == ACPI_EJECT_STATUS_GOING_OFFLINE ||
+		    eject_node->status == ACPI_EJECT_STATUS_READY_REMOVE)) {
+			eject_node->status = ACPI_EJECT_STATUS_CANCEL;
+			cancel_eject = true;
+		}
+
+		acpi_scan_lock_release();
+		if (cancel_eject)
+			break;
+	default:
+		ret = -EINVAL;
+		goto eject_end;
+	};
+
+	if ((!acpi_device->handler || !acpi_device->handler->hotplug.enabled)
+	    && !acpi_device->driver) {
+		ret = -ENODEV;
+		goto eject_end;
+	}
+
+	status = acpi_get_type(acpi_device->handle, &not_used);
+	if (ACPI_FAILURE(status) || !acpi_device->flags.ejectable) {
+		ret = -ENODEV;
+		goto eject_end;
+	}
+
 	get_device(&acpi_device->dev);
 	status = acpi_hotplug_schedule(acpi_device, ACPI_OST_EC_OSPM_EJECT);
-	if (ACPI_SUCCESS(status))
-		return count;
+	if (ACPI_SUCCESS(status)) {
+		ret = count;
+		goto eject_end;
+	}
 
 	put_device(&acpi_device->dev);
 	acpi_evaluate_ost(acpi_device->handle, ACPI_OST_EC_OSPM_EJECT,
 			  ACPI_OST_SC_NON_SPECIFIC_FAILURE, NULL);
-	return status == AE_NO_MEMORY ? -ENOMEM : -EAGAIN;
+	ret = (status == AE_NO_MEMORY) ? -ENOMEM : -EAGAIN;
+
+eject_end:
+	return ret;
+
 }
 
-static DEVICE_ATTR(eject, 0200, NULL, acpi_eject_store);
+static DEVICE_ATTR_RW(eject);
 
 static ssize_t
 acpi_device_hid_show(struct device *dev, struct device_attribute *attr, char *buf)

drivers/acpi/internal.h +3 -1

index 8154690b872b..e5d526402188 100644
@@ -265,7 +265,8 @@ enum acpi_eject_status {
 	ACPI_EJECT_STATUS_NONE = 0,
 	ACPI_EJECT_STATUS_GOING_OFFLINE,
 	ACPI_EJECT_STATUS_READY_REMOVE,
-	ACPI_EJECT_STATUS_FAIL
+	ACPI_EJECT_STATUS_FAIL,
+	ACPI_EJECT_STATUS_CANCEL
 };
 
 enum acpi_eject_node_type {

@@ -286,5 +287,6 @@ struct eject_data {
 };
 
 void acpi_eject_retry(struct acpi_device *adev);
+char *acpi_eject_status_string(struct acpi_device *adev);
 
 #endif /* _ACPI_INTERNAL_H_ */

drivers/acpi/scan.c +3 -2

index 13f16b6ad7a2..90983c067410 100644
@@ -366,8 +366,9 @@ static int acpi_scan_offline_check(struct acpi_device *device)
 			return -EBUSY;
 		}
 
-		if (eject_obj->status == ACPI_EJECT_STATUS_FAIL) {
-			dev_warn(&device->dev, "Eject failed. Recover all.\n");
+		if (eject_obj->status == ACPI_EJECT_STATUS_FAIL ||
+		    eject_obj->status == ACPI_EJECT_STATUS_CANCEL) {
+			dev_warn(&device->dev, "Eject stopped. Recover all.\n");
 			acpi_scan_notify_online(device);
 			return -EAGAIN;
 		}

@@ -383,6 +384,39 @@ static int acpi_scan_offline_check(struct acpi_device *device)
 	return ret;
 }
 

drivers/acpi/scan.c static int acpi_scan_offline_check(struct acpi_device *device) · line 393

Quoted 7 lines

drivers/acpi/scan.c +7 -0

+char *acpi_eject_status_string(struct acpi_device *adev)
+{
+	struct eject_data *eject_obj;
+	char *status_string = "none";
+
+	mutex_lock(&acpi_scan_lock);
+	eject_obj = (struct eject_data *) adev->eject_stat;

Always use checkpatch.pl so maintainers don't have to ask you to use checkpatch.pl :)

Quoted 14 lines

drivers/acpi/scan.c +14 -0

+
+	if (eject_obj) {
+		switch (eject_obj->status) {
+		case ACPI_EJECT_STATUS_NONE:
+			break;
+		case ACPI_EJECT_STATUS_GOING_OFFLINE:
+			status_string = "going offline";
+			break;
+		case ACPI_EJECT_STATUS_READY_REMOVE:
+			status_string = "ready to remove";
+			break;
+		case ACPI_EJECT_STATUS_FAIL:
+			status_string = "failure";
+			break;

drivers/acpi/scan.c static int acpi_scan_offline_check(struct acpi_device *device) · line 417

Quoted 10 lines

drivers/acpi/scan.c +10 -0

+		case ACPI_EJECT_STATUS_CANCEL:
+			status_string = "cancel";
+			break;
+		default:
+			status_string = "(unknown)";
+		}
+	}
+
+	mutex_unlock(&acpi_scan_lock);
+	return status_string;

So the state can change right after checking it and reporting it to userspace?

If so, what good is the lock here at all?

thanks,

greg k-h