[RFC PATCH v2 0/3] ACPI: New eject flow to remove devices cautiously

[RFC PATCH v2 0/3] ACPI: New eject flow to remove devices cautiously

[Chester Lin]

Currently there are two ways to handle ACPI device ejection. When an eject
event happens on a container, the kernel just sends KOBJ_CHANGE to
userland and userland should handle offline operation. For other device
types, acpi_scan_try_to_offline() is called and it tries to put target
device(s) offline and then removes all nodes once they are all offline.

However we found that sometimes applications could intensively access
resources on ejectable devices therefore they could have risk if ejection
suddenly happens and removes devices without any notification. In stead
of executing the offline callbakcs directly, we want to introduce a new
approach, which sends change events to notify all target nodes beforehand
and hands over offline handling to userland so that userland can have a
chance to schedule an offline task based on current workload. The online
function to recover from failure is also changed, it follows the same
approach to send change events rather than putting devices online directly
, which means userland will also need to take care of online handling.

To ensure that eject function can work properly since normal users might
not have their own offline/online handling, we will submit a generic udev
rule to systemd upstream as default in order to deal with change events
and take [offline/online] action accordingly. But the Hot-Removing part
still remains so the hotplug function can run to it once target nodes are
all offline.

To easily monitor eject status and start over an eject process, there's a
status trace mechanism in this eject flow, which helps to count current
online devices under the ejectable target, and it can reschedule an eject
event when all nodes within the device tree have been put offline.

v2:
- device_sysfs: Add descriptions in /Document/ABI/testing/sysfs-bus-acpi
- device_sysfs: Replace the declartion with DEVICE_ATTR_RW and add cancel
  option in eject_store.
- scan: Add a retry mechanism when userspace fail to put device offline.
- scan: Add ready-to-remove state.

Chester Lin (3):
  ACPI / hotplug: Send change events for offline/online requests when
    eject is triggered
  ACPI / hotplug: Eject status trace and auto-remove approach
  ACPI / device_sysfs: Add eject_show and add a cancel option in
    eject_store

 Documentation/ABI/testing/sysfs-bus-acpi |   9 +-
 drivers/acpi/container.c                 |   2 +-
 drivers/acpi/device_sysfs.c              |  94 ++++++-
 drivers/acpi/glue.c                      | 146 +++++++++++
 drivers/acpi/internal.h                  |  34 ++-
 drivers/acpi/scan.c                      | 318 +++++++++++++++++------
 drivers/base/core.c                      |   4 +
 include/acpi/acpi_bus.h                  |   3 +-
 include/linux/acpi.h                     |   6 +
 9 files changed, 523 insertions(+), 93 deletions(-)

-- 
2.20.1

[RFC PATCH v2 1/3] ACPI / hotplug: Send change events for offline/online requests when eject is triggered

[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.

Signed-off-by: Chester Lin <clin@suse.com>
---
 drivers/acpi/container.c |   2 +-
 drivers/acpi/internal.h  |   2 +-
 drivers/acpi/scan.c      | 140 +++++++++++++++++----------------------
 include/acpi/acpi_bus.h  |   2 +-
 4 files changed, 65 insertions(+), 81 deletions(-)

diff --git a/drivers/acpi/container.c b/drivers/acpi/container.c
index 9ea5f55d97e3..53ca9b1ae1bf 100644
--- a/drivers/acpi/container.c
+++ b/drivers/acpi/container.c
@@ -33,7 +33,7 @@ static int acpi_container_offline(struct container_dev *cdev)
 
 	/* Check all of the dependent devices' physical companions. */
 	list_for_each_entry(child, &adev->children, node)
-		if (!acpi_scan_is_offline(child, false))
+		if (!acpi_scan_is_offline(child))
 			return -EBUSY;
 
 	return 0;
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index f6157d4d637a..656d237b073d 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -83,7 +83,7 @@ void acpi_apd_init(void);
 acpi_status acpi_hotplug_schedule(struct acpi_device *adev, u32 src);
 bool acpi_queue_hotplug_work(struct work_struct *work);
 void acpi_device_hotplug(struct acpi_device *adev, u32 src);
-bool acpi_scan_is_offline(struct acpi_device *adev, bool uevent);
+bool acpi_scan_is_offline(struct acpi_device *adev);
 
 acpi_status acpi_sysfs_table_handler(u32 event, void *table, void *context);
 void acpi_scan_table_handler(u32 event, void *table, void *context);
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 0e28270b0fd8..d7628146eb5f 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -113,11 +113,10 @@ int acpi_scan_add_handler_with_hotplug(struct acpi_scan_handler *handler,
 	return 0;
 }
 
-bool acpi_scan_is_offline(struct acpi_device *adev, bool uevent)
+bool acpi_scan_is_offline(struct acpi_device *adev)
 {
 	struct acpi_device_physical_node *pn;
 	bool offline = true;
-	char *envp[] = { "EVENT=offline", NULL };
 
 	/*
 	 * acpi_container_offline() calls this for all of the container's
@@ -127,9 +126,6 @@ bool acpi_scan_is_offline(struct acpi_device *adev, bool uevent)
 
 	list_for_each_entry(pn, &adev->physical_node_list, node)
 		if (device_supports_offline(pn->dev) && !pn->dev->offline) {
-			if (uevent)
-				kobject_uevent_env(&pn->dev->kobj, KOBJ_CHANGE, envp);
-
 			offline = false;
 			break;
 		}
@@ -138,13 +134,12 @@ bool acpi_scan_is_offline(struct acpi_device *adev, bool uevent)
 	return offline;
 }
 
-static acpi_status acpi_bus_offline(acpi_handle handle, u32 lvl, void *data,
-				    void **ret_p)
+static acpi_status acpi_bus_offline_notify(acpi_handle handle, u32 lvl,
+					void *data, void **ret_p)
 {
 	struct acpi_device *device = NULL;
 	struct acpi_device_physical_node *pn;
-	bool second_pass = (bool)data;
-	acpi_status status = AE_OK;
+	char *envp[] = { "EVENT=offline", NULL };
 
 	if (acpi_bus_get_device(handle, &device))
 		return AE_OK;
@@ -155,100 +150,93 @@ static acpi_status acpi_bus_offline(acpi_handle handle, u32 lvl, void *data,
 	}
 
 	mutex_lock(&device->physical_node_lock);
-
 	list_for_each_entry(pn, &device->physical_node_list, node) {
-		int ret;
-
-		if (second_pass) {
-			/* Skip devices offlined by the first pass. */
-			if (pn->put_online)
-				continue;
-		} else {
-			pn->put_online = false;
-		}
-		ret = device_offline(pn->dev);
-		if (ret >= 0) {
-			pn->put_online = !ret;
-		} else {
-			*ret_p = pn->dev;
-			if (second_pass) {
-				status = AE_ERROR;
-				break;
-			}
+		if (device_supports_offline(pn->dev) && !(pn->dev->offline)) {
+			kobject_uevent_env(&pn->dev->kobj, KOBJ_CHANGE, envp);
+			pn->changed_offline = true;
 		}
 	}
-
 	mutex_unlock(&device->physical_node_lock);
 
-	return status;
+	return AE_OK;
 }
 
-static acpi_status acpi_bus_online(acpi_handle handle, u32 lvl, void *data,
-				   void **ret_p)
+static acpi_status acpi_bus_online_notify(acpi_handle handle, u32 lvl,
+						void *data, void **ret_p)
 {
 	struct acpi_device *device = NULL;
 	struct acpi_device_physical_node *pn;
+	char *envp[] = { "EVENT=online", NULL };
 
 	if (acpi_bus_get_device(handle, &device))
 		return AE_OK;
 
 	mutex_lock(&device->physical_node_lock);
 
-	list_for_each_entry(pn, &device->physical_node_list, node)
-		if (pn->put_online) {
-			device_online(pn->dev);
-			pn->put_online = false;
+	list_for_each_entry(pn, &device->physical_node_list, node) {
+		if (pn->changed_offline) {
+			kobject_uevent_env(&pn->dev->kobj, KOBJ_CHANGE, envp);
+			pn->changed_offline = false;
 		}
+	}
 
 	mutex_unlock(&device->physical_node_lock);
-
 	return AE_OK;
 }
 
-static int acpi_scan_try_to_offline(struct acpi_device *device)
+static void acpi_scan_notify_online(struct acpi_device *device)
+{
+	acpi_handle handle = device->handle;
+
+	acpi_bus_online_notify(handle, 0, NULL, NULL);
+	acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
+				acpi_bus_online_notify, NULL, NULL, NULL);
+}
+
+static int acpi_scan_notify_offline(struct acpi_device *device)
 {
 	acpi_handle handle = device->handle;
 	struct device *errdev = NULL;
 	acpi_status status;
 
-	/*
-	 * Carry out two passes here and ignore errors in the first pass,
-	 * because if the devices in question are memory blocks and
-	 * CONFIG_MEMCG is set, one of the blocks may hold data structures
-	 * that the other blocks depend on, but it is not known in advance which
-	 * block holds them.
-	 *
-	 * If the first pass is successful, the second one isn't needed, though.
-	 */
 	status = acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
-				     NULL, acpi_bus_offline, (void *)false,
-				     (void **)&errdev);
-	if (status == AE_SUPPORT) {
+			NULL, acpi_bus_offline_notify,
+			NULL, (void **)&errdev);
+	if (errdev)
+		goto notify_error;
+
+	status = acpi_bus_offline_notify(handle, 0, NULL,
+				(void **)&errdev);
+
+	if (errdev)
+		goto notify_error;
+
+	return 0;
+
+notify_error:
+	acpi_scan_notify_online(device);
+
+	switch (status) {
+	case AE_SUPPORT:
 		dev_warn(errdev, "Offline disabled.\n");
-		acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
-				    acpi_bus_online, NULL, NULL, NULL);
 		return -EPERM;
+	default:
+		dev_warn(errdev, "Offline failed. (status:%#x)\n", status);
+		return -EBUSY;
 	}
-	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, (void *)true,
-				    (void **)&errdev);
-		if (!errdev)
-			acpi_bus_offline(handle, 0, (void *)true,
-					 (void **)&errdev);
-
-		if (errdev) {
-			dev_warn(errdev, "Offline failed.\n");
-			acpi_bus_online(handle, 0, NULL, NULL);
-			acpi_walk_namespace(ACPI_TYPE_ANY, handle,
-					    ACPI_UINT32_MAX, acpi_bus_online,
-					    NULL, NULL, NULL);
+}
+
+static int acpi_scan_offline_check(struct acpi_device *device)
+{
+	int ret = 0;
+	/* Send offline request to userland if the container is not offline */
+	if (!acpi_scan_is_offline(device)) {
+		ret = acpi_scan_notify_offline(device);
+		if (!ret)
 			return -EBUSY;
-		}
 	}
-	return 0;
+
+	return ret;
 }
 
 static int acpi_scan_hot_remove(struct acpi_device *device)
@@ -256,15 +244,11 @@ static int acpi_scan_hot_remove(struct acpi_device *device)
 	acpi_handle handle = device->handle;
 	unsigned long long sta;
 	acpi_status status;
+	int ret;
 
-	if (device->handler && device->handler->hotplug.demand_offline) {
-		if (!acpi_scan_is_offline(device, true))
-			return -EBUSY;
-	} else {
-		int error = acpi_scan_try_to_offline(device);
-		if (error)
-			return error;
-	}
+	ret = acpi_scan_offline_check(device);
+	if (ret)
+		return ret;
 
 	ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 		"Hot-removing device %s...\n", dev_name(&device->dev)));
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 4752ff0a9d9b..57e4ad0483ca 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -330,7 +330,7 @@ struct acpi_device_physical_node {
 	unsigned int node_id;
 	struct list_head node;
 	struct device *dev;
-	bool put_online:1;
+	bool changed_offline:1;
 };
 
 struct acpi_device_properties {
-- 
2.20.1

Re: [RFC PATCH v2 1/3] ACPI / hotplug: Send change events for offline/online requests when eject is triggered

[Rafael J. Wysocki]

On Friday, January 3, 2020 5:40:09 AM CET Chester Lin wrote:
> 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 <clin@suse.com>

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).

Re: [RFC PATCH v2 1/3] ACPI / hotplug: Send change events for offline/online requests when eject is triggered

[Chester Lin]

Hi Rafael,

On Wed, Jan 15, 2020 at 11:15:08AM +0100, Rafael J. Wysocki wrote:
> On Friday, January 3, 2020 5:40:09 AM CET Chester Lin wrote:
> > 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 <clin@suse.com>
> 
> 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.
>
Thank you for the reminder.

> 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).
> 
Thanks for your advice. If no one is working on this device attribute [please free
feel to correct me if I'm wrong], I am willing to implement it and will send a new
RFC patch for code review.

Regards,
Chester

[RFC PATCH v2 2/3] ACPI / hotplug: Eject status trace and auto-remove approach

[Chester Lin]

This is an eject-status trace mechanism which helps to count current online
devices under the ejection target, and it can automatically reschedules an
eject event when all related devices are offline. The number of online
nodes can be updated when any node has been put offline successfully.
Any online event or offline failure within the whole device tree during
ejection will stop the whole process and devices who have been put offline
will need be online again as recovery.

Signed-off-by: Chester Lin <clin@suse.com>
---
 drivers/acpi/glue.c     | 146 ++++++++++++++++++++++++++++++++++++++
 drivers/acpi/internal.h |  30 ++++++++
 drivers/acpi/scan.c     | 152 +++++++++++++++++++++++++++++++++++++++-
 drivers/base/core.c     |   4 ++
 include/acpi/acpi_bus.h |   1 +
 include/linux/acpi.h    |   6 ++
 6 files changed, 336 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
index 36b24b0658cb..e1c419335128 100644
--- a/drivers/acpi/glue.c
+++ b/drivers/acpi/glue.c
@@ -360,6 +360,82 @@ static int acpi_device_notify_remove(struct device *dev)
 	return 0;
 }
 
+static void acpi_device_eject_tracer(struct device *dev,
+				enum kobject_action action)
+{
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+	struct acpi_device *adev_root = NULL;
+	struct eject_data *eject_node = NULL;
+	struct eject_data *eject_root = NULL;
+
+	if (!adev)
+		return;
+
+	acpi_scan_lock_acquire();
+
+	eject_node = (struct eject_data *)adev->eject_stat;
+
+	if (!eject_node)
+		goto tracer_end;
+
+	if (eject_node->base.root_handle)
+		adev_root =
+		    acpi_bus_get_acpi_device(eject_node->base.root_handle);
+
+	if (adev_root)
+		eject_root = (struct eject_data *)adev_root->eject_stat;
+
+	if (action == KOBJ_OFFLINE) {
+		eject_node->online_nodes--;
+
+		if (eject_node->online_nodes == 0)
+			eject_node->status = ACPI_EJECT_STATUS_READY_REMOVE;
+
+		/*
+		 * If the offline device is child, update its eject_root's
+		 * number of online nodes.
+		 */
+		if (eject_root) {
+			eject_root->online_nodes--;
+
+			goto tracer_end;
+		}
+		/*
+		 * Adjust number of online nodes when any physical node under
+		 * eject_root is offline. Trigger acpi_eject_try() once all
+		 * nodes are offline.
+		 */
+		if (eject_node->online_nodes == 0)
+			acpi_eject_retry(adev);
+
+	} else if (action == KOBJ_ONLINE) {
+		/* !eject_root means now eject_node is root. */
+		if (!eject_root)
+			eject_root = eject_node;
+
+		if (eject_root->status == ACPI_EJECT_STATUS_GOING_OFFLINE ||
+		    eject_root->status == ACPI_EJECT_STATUS_READY_REMOVE) {
+			eject_root->status = ACPI_EJECT_STATUS_FAIL;
+			acpi_eject_retry(adev_root);
+		}
+	}
+tracer_end:
+	if (adev_root)
+		acpi_bus_put_acpi_device(adev_root);
+
+	acpi_scan_lock_release();
+}
+
+static inline void acpi_device_check_eject_status(struct device *dev)
+{
+	acpi_device_eject_tracer(dev, KOBJ_OFFLINE);
+}
+
+static inline void acpi_device_check_eject_recover(struct device *dev)
+{
+	acpi_device_eject_tracer(dev, KOBJ_ONLINE);
+}
+
 int acpi_platform_notify(struct device *dev, enum kobject_action action)
 {
 	switch (action) {
@@ -369,8 +445,78 @@ int acpi_platform_notify(struct device *dev, enum kobject_action action)
 	case KOBJ_REMOVE:
 		acpi_device_notify_remove(dev);
 		break;
+	case KOBJ_OFFLINE:
+		acpi_device_check_eject_status(dev);
+		break;
+	case KOBJ_ONLINE:
+		acpi_device_check_eject_recover(dev);
+		break;
 	default:
 		break;
 	}
 	return 0;
 }
+
+static void acpi_device_check_offline_fail(struct device *dev)
+{
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+	struct acpi_device *adev_root = NULL;
+	struct acpi_device *adev_child = NULL;
+	struct eject_data *eject_node = NULL;
+	bool has_online_child = false;
+	char *envp[] = { "EVENT=offline", NULL };
+
+	if (!adev)
+		return;
+
+	acpi_scan_lock_acquire();
+
+	eject_node = (struct eject_data *)adev->eject_stat;
+
+	if (!eject_node ||
+	    eject_node->status != ACPI_EJECT_STATUS_GOING_OFFLINE)
+		goto check_end;
+
+	if (!eject_node->retry) {
+		/* Check if the device contains any online child. */
+		list_for_each_entry(adev_child, &adev->children, node)
+			if (!acpi_scan_is_offline(adev_child)) {
+				has_online_child = true;
+				break;
+			}
+
+		if (!has_online_child)
+			eject_node->retry = true;
+
+		/* Sent change event again. */
+		kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
+
+	} else {
+		/* Treat as failure if it still fails after retry. */
+		if (eject_node->base.root_handle) {
+			adev_root = acpi_bus_get_acpi_device(
+				eject_node->base.root_handle);
+
+			if (!adev_root || !adev_root->eject_stat)
+				goto check_end;
+
+			adev = adev_root;
+			eject_node = (struct eject_data *)adev_root->eject_stat;
+		}
+
+		eject_node->status = ACPI_EJECT_STATUS_FAIL;
+		acpi_eject_retry(adev);
+	}
+
+check_end:
+	if (adev_root)
+		acpi_bus_put_acpi_device(adev_root);
+
+	acpi_scan_lock_release();
+}
+
+void acpi_action_fail_notify(struct device *dev, enum kobject_action action)
+{
+	if (action == KOBJ_OFFLINE)
+		acpi_device_check_offline_fail(dev);
+}
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 656d237b073d..8154690b872b 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -257,4 +257,34 @@ void acpi_init_lpit(void);
 static inline void acpi_init_lpit(void) { }
 #endif
 
+/* --------------------------------------------------------------------------
+ *				Eject Status
+ * --------------------------------------------------------------------------
+ */
+enum acpi_eject_status {
+	ACPI_EJECT_STATUS_NONE = 0,
+	ACPI_EJECT_STATUS_GOING_OFFLINE,
+	ACPI_EJECT_STATUS_READY_REMOVE,
+	ACPI_EJECT_STATUS_FAIL
+};
+
+enum acpi_eject_node_type {
+	ACPI_EJECT_CHILD_NODE = 0,
+	ACPI_EJECT_ROOT_NODE
+};
+
+struct eject_data_base {
+	enum acpi_eject_node_type type;
+	acpi_handle root_handle;
+};
+
+struct eject_data {
+	struct eject_data_base base;
+	enum acpi_eject_status status;
+	u32 online_nodes;
+	bool retry;
+};
+
+void acpi_eject_retry(struct acpi_device *adev);
+
 #endif /* _ACPI_INTERNAL_H_ */
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index d7628146eb5f..13f16b6ad7a2 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -134,11 +134,116 @@ bool acpi_scan_is_offline(struct acpi_device *adev)
 	return offline;
 }
 
+void acpi_eject_retry(struct acpi_device *adev)
+{
+	acpi_status status;
+
+	get_device(&adev->dev);
+
+	status = acpi_hotplug_schedule(adev, ACPI_OST_EC_OSPM_EJECT);
+	if (ACPI_FAILURE(status)) {
+		pr_debug("Failed to reschedule an eject event\n");
+		put_device(&adev->dev);
+		acpi_evaluate_ost(adev->handle, ACPI_OST_EC_OSPM_EJECT,
+				ACPI_OST_SC_NON_SPECIFIC_FAILURE, NULL);
+		return;
+	}
+	pr_debug("Rescheduled an eject event\n");
+}
+
+static acpi_status acpi_init_eject_stat(struct acpi_device *adev,
+					enum acpi_eject_node_type type)
+{
+	struct eject_data *eject_node = (struct eject_data *) adev->eject_stat;
+
+	if (!eject_node) {
+		eject_node = kzalloc(sizeof(*eject_node), GFP_KERNEL);
+		if (!eject_node)
+			return AE_NO_MEMORY;
+		adev->eject_stat = eject_node;
+	}
+
+	eject_node->base.type = type;
+	eject_node->online_nodes = 0;
+
+	return AE_OK;
+}
+
+static void acpi_enable_eject_stat(struct acpi_device *adev,
+				struct acpi_device *root_adev, u32 online_nodes)
+{
+	struct eject_data *eject_node = NULL;
+	struct eject_data *eject_root = NULL;
+
+	if (adev)
+		eject_node = (struct eject_data *)adev->eject_stat;
+	if (root_adev)
+		eject_root = (struct eject_data *)root_adev->eject_stat;
+
+	if (!eject_node || !eject_root) {
+		pr_debug("No eject data\n");
+		return;
+	}
+
+	if (eject_node != eject_root)
+		eject_node->base.root_handle = root_adev->handle;
+	else
+		eject_node->base.root_handle = 0;
+
+	/* Update node status and online_nodes of root device */
+	if (online_nodes) {
+		eject_root->online_nodes += online_nodes;
+
+		if (eject_node->base.root_handle)
+			eject_node->online_nodes = online_nodes;
+
+		pr_debug("Online nodes:%u, Total:%u\n",
+			 eject_node->online_nodes, eject_root->online_nodes);
+
+		eject_node->status = ACPI_EJECT_STATUS_GOING_OFFLINE;
+
+	} else {
+		eject_node->status = ACPI_EJECT_STATUS_READY_REMOVE;
+	}
+}
+
+static void acpi_free_eject_stat(struct acpi_device *adev)
+{
+	kfree(adev->eject_stat);
+	adev->eject_stat = NULL;
+}
+
+static acpi_status acpi_bus_offline_prepare(acpi_handle handle, u32 lvl,
+					void *data, void **ret_p)
+{
+	struct acpi_device *device = NULL;
+	struct eject_data_base *eject_obj = (struct eject_data_base *)data;
+	acpi_status status = AE_OK;
+
+	if (acpi_bus_get_device(handle, &device))
+		return AE_OK;
+
+	if (device->handler && !device->handler->hotplug.enabled) {
+		*ret_p = &device->dev;
+		return AE_SUPPORT;
+	}
+
+	status = acpi_init_eject_stat(device, eject_obj->type);
+	if (ACPI_FAILURE(status))
+		*ret_p = &device->dev;
+
+	return status;
+}
+
+
 static acpi_status acpi_bus_offline_notify(acpi_handle handle, u32 lvl,
 					void *data, void **ret_p)
 {
 	struct acpi_device *device = NULL;
+	struct acpi_device *root_device = NULL;
 	struct acpi_device_physical_node *pn;
+	struct eject_data_base *eject_obj = (struct eject_data_base *)data;
+	u32 online_nodes = 0;
 	char *envp[] = { "EVENT=offline", NULL };
 
 	if (acpi_bus_get_device(handle, &device))
@@ -149,13 +254,22 @@ static acpi_status acpi_bus_offline_notify(acpi_handle handle, u32 lvl,
 		return AE_SUPPORT;
 	}
 
+	if (eject_obj->root_handle == device->handle) {
+		root_device = device;
+	} else if (acpi_bus_get_device(eject_obj->root_handle, &root_device)) {
+		*ret_p = &device->dev;
+		return AE_NULL_OBJECT;
+	}
+
 	mutex_lock(&device->physical_node_lock);
 	list_for_each_entry(pn, &device->physical_node_list, node) {
 		if (device_supports_offline(pn->dev) && !(pn->dev->offline)) {
 			kobject_uevent_env(&pn->dev->kobj, KOBJ_CHANGE, envp);
+			online_nodes++;
 			pn->changed_offline = true;
 		}
 	}
+	acpi_enable_eject_stat(device, root_device, online_nodes);
 	mutex_unlock(&device->physical_node_lock);
 
 	return AE_OK;
@@ -171,6 +285,8 @@ static acpi_status acpi_bus_online_notify(acpi_handle handle, u32 lvl,
 	if (acpi_bus_get_device(handle, &device))
 		return AE_OK;
 
+	acpi_free_eject_stat(device);
+
 	mutex_lock(&device->physical_node_lock);
 
 	list_for_each_entry(pn, &device->physical_node_list, node) {
@@ -197,15 +313,23 @@ static int acpi_scan_notify_offline(struct acpi_device *device)
 {
 	acpi_handle handle = device->handle;
 	struct device *errdev = NULL;
+	struct eject_data_base base_data = {ACPI_EJECT_ROOT_NODE, handle};
 	acpi_status status;
 
+	status = acpi_bus_offline_prepare(handle, 0, (void *)&base_data,
+					(void **)&errdev);
+	if (errdev)
+		goto notify_error;
+
+	base_data.type = ACPI_EJECT_CHILD_NODE;
 	status = acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
-			NULL, acpi_bus_offline_notify,
-			NULL, (void **)&errdev);
+			acpi_bus_offline_prepare, acpi_bus_offline_notify,
+			(void *)&base_data, (void **)&errdev);
 	if (errdev)
 		goto notify_error;
 
-	status = acpi_bus_offline_notify(handle, 0, NULL,
+	base_data.type = ACPI_EJECT_ROOT_NODE;
+	status = acpi_bus_offline_notify(handle, 0, (void *)&base_data,
 				(void **)&errdev);
 
 	if (errdev)
@@ -217,6 +341,10 @@ static int acpi_scan_notify_offline(struct acpi_device *device)
 	acpi_scan_notify_online(device);
 
 	switch (status) {
+	case AE_NO_MEMORY:
+		return -ENOMEM;
+	case AE_NULL_OBJECT:
+		return -EINVAL;
 	case AE_SUPPORT:
 		dev_warn(errdev, "Offline disabled.\n");
 		return -EPERM;
@@ -229,6 +357,22 @@ static int acpi_scan_notify_offline(struct acpi_device *device)
 static int acpi_scan_offline_check(struct acpi_device *device)
 {
 	int ret = 0;
+	struct eject_data *eject_obj = (struct eject_data *) device->eject_stat;
+
+	/* Send recovery events to userland if any failure occur */
+	if (eject_obj) {
+		if (eject_obj->base.type != ACPI_EJECT_ROOT_NODE) {
+			dev_warn(&device->dev, "The node is under ejection.\n");
+			return -EBUSY;
+		}
+
+		if (eject_obj->status == ACPI_EJECT_STATUS_FAIL) {
+			dev_warn(&device->dev, "Eject failed. Recover all.\n");
+			acpi_scan_notify_online(device);
+			return -EAGAIN;
+		}
+	}
+
 	/* Send offline request to userland if the container is not offline */
 	if (!acpi_scan_is_offline(device)) {
 		ret = acpi_scan_notify_offline(device);
@@ -2060,6 +2204,8 @@ void acpi_bus_trim(struct acpi_device *adev)
 	list_for_each_entry_reverse(child, &adev->children, node)
 		acpi_bus_trim(child);
 
+	acpi_free_eject_stat(adev);
+
 	adev->flags.match_driver = false;
 	if (handler) {
 		if (handler->detach)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index b4c64528f13c..2a85fbf0c666 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2570,6 +2570,9 @@ int device_offline(struct device *dev)
 			if (!ret) {
 				kobject_uevent(&dev->kobj, KOBJ_OFFLINE);
 				dev->offline = true;
+				device_platform_notify(dev, KOBJ_OFFLINE);
+			} else {
+				acpi_action_fail_notify(dev, KOBJ_OFFLINE);
 			}
 		}
 	}
@@ -2599,6 +2602,7 @@ int device_online(struct device *dev)
 			if (!ret) {
 				kobject_uevent(&dev->kobj, KOBJ_ONLINE);
 				dev->offline = false;
+				device_platform_notify(dev, KOBJ_ONLINE);
 			}
 		} else {
 			ret = 1;
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 57e4ad0483ca..29004b3fad87 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -378,6 +378,7 @@ struct acpi_device {
 	struct list_head physical_node_list;
 	struct mutex physical_node_lock;
 	void (*remove)(struct acpi_device *);
+	void *eject_stat;
 };
 
 /* Non-device subnode */
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index d315d86844e4..91ba75914ac7 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1321,12 +1321,18 @@ static inline int find_acpi_cpu_cache_topology(unsigned int cpu, int level)
 
 #ifdef CONFIG_ACPI
 extern int acpi_platform_notify(struct device *dev, enum kobject_action action);
+extern void acpi_action_fail_notify(struct device *dev,
+					enum kobject_action action);
 #else
 static inline int
 acpi_platform_notify(struct device *dev, enum kobject_action action)
 {
 	return 0;
 }
+static inline void
+void acpi_action_fail_notify(struct device *dev, enum kobject_action action)
+{
+}
 #endif
 
 #endif	/*_LINUX_ACPI_H*/
-- 
2.20.1

Re: [RFC PATCH v2 2/3] ACPI / hotplug: Eject status trace and auto-remove approach

[gregkh]

On Fri, Jan 03, 2020 at 04:40:13AM +0000, Chester Lin wrote:
> +	eject_node = (struct eject_data *)adev->eject_stat;

No need for this cast.

You do that all over this patch, it can be removed everywhere.

thanks,

greg k-h

[RFC PATCH v2 3/3] ACPI / device_sysfs: Add eject_show and add a cancel option in eject_store

[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 <clin@suse.com>
---
 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(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-acpi b/Documentation/ABI/testing/sysfs-bus-acpi
index e7898cfe5fb1..32fdf4af962e 100644
--- a/Documentation/ABI/testing/sysfs-bus-acpi
+++ b/Documentation/ABI/testing/sysfs-bus-acpi
@@ -53,9 +53,12 @@ What:		/sys/bus/acpi/devices/.../eject
 Date:		December 2006
 Contact:	Rafael J. Wysocki <rjw@rjwysocki.net>
 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.
 
 What:		/sys/bus/acpi/devices/.../status
 Date:		Jan, 2014
diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
index 96869f1538b9..6801b268fe9d 100644
--- a/drivers/acpi/device_sysfs.c
+++ b/drivers/acpi/device_sysfs.c
@@ -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)
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 8154690b872b..e5d526402188 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -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_ */
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 13f16b6ad7a2..90983c067410 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -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;
 }
 
+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;
+
+	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;
+		case ACPI_EJECT_STATUS_CANCEL:
+			status_string = "cancel";
+			break;
+		default:
+			status_string = "(unknown)";
+		}
+	}
+
+	mutex_unlock(&acpi_scan_lock);
+	return status_string;
+}
+
 static int acpi_scan_hot_remove(struct acpi_device *device)
 {
 	acpi_handle handle = device->handle;
-- 
2.20.1

Re: [RFC PATCH v2 3/3] ACPI / device_sysfs: Add eject_show and add a cancel option in eject_store

[gregkh]

On Fri, Jan 03, 2020 at 04:40:17AM +0000, Chester Lin wrote:
> 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 <clin@suse.com>
> ---
>  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(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-acpi b/Documentation/ABI/testing/sysfs-bus-acpi
> index e7898cfe5fb1..32fdf4af962e 100644
> --- a/Documentation/ABI/testing/sysfs-bus-acpi
> +++ b/Documentation/ABI/testing/sysfs-bus-acpi
> @@ -53,9 +53,12 @@ What:		/sys/bus/acpi/devices/.../eject
>  Date:		December 2006
>  Contact:	Rafael J. Wysocki <rjw@rjwysocki.net>
>  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.

>  
>  What:		/sys/bus/acpi/devices/.../status
>  Date:		Jan, 2014
> diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
> index 96869f1538b9..6801b268fe9d 100644
> --- a/drivers/acpi/device_sysfs.c
> +++ b/drivers/acpi/device_sysfs.c
> @@ -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)
> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> index 8154690b872b..e5d526402188 100644
> --- a/drivers/acpi/internal.h
> +++ b/drivers/acpi/internal.h
> @@ -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_ */
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 13f16b6ad7a2..90983c067410 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -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;
>  }
>  
> +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 :)

> +
> +	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;
> +		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

Re: [RFC PATCH v2 3/3] ACPI / device_sysfs: Add eject_show and add a cancel option in eject_store

[Chester Lin]

Hi Greg,

On Fri, Jan 03, 2020 at 09:37:28AM +0100, gregkh@linuxfoundation.org wrote:
> On Fri, Jan 03, 2020 at 04:40:17AM +0000, Chester Lin wrote:
> > 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 <clin@suse.com>
> > ---
> >  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(-)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-bus-acpi b/Documentation/ABI/testing/sysfs-bus-acpi
> > index e7898cfe5fb1..32fdf4af962e 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-acpi
> > +++ b/Documentation/ABI/testing/sysfs-bus-acpi
> > @@ -53,9 +53,12 @@ What:		/sys/bus/acpi/devices/.../eject
> >  Date:		December 2006
> >  Contact:	Rafael J. Wysocki <rjw@rjwysocki.net>
> >  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.
> 

Thank you for the advice and I will add it.

> 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.
>

When eject is not happening, the eject_stat is null so the procedure will go to
eject_end without changing anything.

> >  
> >  What:		/sys/bus/acpi/devices/.../status
> >  Date:		Jan, 2014
> > diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
> > index 96869f1538b9..6801b268fe9d 100644
> > --- a/drivers/acpi/device_sysfs.c
> > +++ b/drivers/acpi/device_sysfs.c
> > @@ -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)
> > diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> > index 8154690b872b..e5d526402188 100644
> > --- a/drivers/acpi/internal.h
> > +++ b/drivers/acpi/internal.h
> > @@ -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_ */
> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > index 13f16b6ad7a2..90983c067410 100644
> > --- a/drivers/acpi/scan.c
> > +++ b/drivers/acpi/scan.c
> > @@ -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;
> >  }
> >  
> > +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 :)
> 

Acutally I have used checkpatch.pl but haven't found a warning on this
line:
  total: 0 errors, 0 warnings, 199 lines checked

The last commit of checkpatch.pl in my code base is 184b8f7f91ca. Anyway,
thank you for the reminder I will remove the cast from next version :)

> > +
> > +	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;
> > +		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?
> 

I use this lock to prevent the eject_state from being freed during a query
since any status change might still happen. For example, the userland process
could be preempted before accessing eject_state and then the eject_state could
be quickly freed due to eject failure or eject completion. [The free opertion
is implemented in acpi_free_eject_stat()]

> 
> thanks,
> 
> greg k-h
>