[RFC PATCH 0/3] acpi: Introduce prepare_remove device operation

[RFC PATCH 0/3] acpi: Introduce prepare_remove device operation

[Vasilis Liaskovitis]

As discussed in
https://patchwork.kernel.org/patch/1581581/
the driver core remove function needs to always succeed. This means we need
to know that the device can be successfully removed before acpi_bus_trim / 
acpi_bus_hot_remove_device are called. This can cause panics when OSPM-initiated
eject (echo 1 > /sys/bus/acpi/devices/PNP/eject) of memory devices fails, since
the ACPI core goes ahead and ejects the device regardless of whether the memory
is still in use or not.

For this reason a new acpi_device operation called prepare_remove is introduced.
This operation should be registered for acpi devices whose removal (from kernel
perspective) can fail.  Memory devices fall in this category.

acpi_bus_hot_remove_device is changed to handle removal in 2 steps:
- preparation for removal i.e. perform part of removal that can fail outside of
  ACPI core. Should succeed for device and all its children.
- if above step was successfull, proceed to actual ACPI removal

acpi_bus_trim is changed accordingly to handle preparation for removal and
actual removal.

With this patchset, only acpi memory devices use the new prepare_remove
device operation. The actual memory removal (VM-related offline and other memory
cleanups) is moved to prepare_remove. The old remove operation just cleans up
the acpi structures. Directly ejecting PNP0C80 memory devices works safely. I
haven't tested yet with an ACPI container which contains memory devices.

Other ACPI devices (e.g. CPU) do not register prepare_remove callbacks, and
their OSPM-side eject should not be affected.

I am not happy with the name prepare_remove. Comments welcome. Let me know if I
should work more in this direction (I think Yasuaki might also look into this
and might have a simpler idea)

Patches are on top of Rafael's linux-pm/linux-next

Vasilis Liaskovitis (3):
  acpi: Introduce prepare_remove operation in acpi_device_ops
  acpi: Make acpi_bus_trim handle device removal preparation
  acpi_memhotplug: Add prepare_remove operation

 drivers/acpi/acpi_memhotplug.c     |   24 +++++++++++++++++++++---
 drivers/acpi/dock.c                |    2 +-
 drivers/acpi/scan.c                |   32 +++++++++++++++++++++++++++++---
 drivers/pci/hotplug/acpiphp_glue.c |    4 ++--
 drivers/pci/hotplug/sgi_hotplug.c  |    2 +-
 include/acpi/acpi_bus.h            |    4 +++-
 6 files changed, 57 insertions(+), 11 deletions(-)

-- 
1.7.9

[RFC PATCH 1/3] acpi: Introduce prepare_remove operation in acpi_device_ops

[Vasilis Liaskovitis]

Signed-off-by: Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com>
---
 include/acpi/acpi_bus.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 2242c10..6ef1692 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -94,6 +94,7 @@ typedef int (*acpi_op_start) (struct acpi_device * device);
 typedef int (*acpi_op_bind) (struct acpi_device * device);
 typedef int (*acpi_op_unbind) (struct acpi_device * device);
 typedef void (*acpi_op_notify) (struct acpi_device * device, u32 event);
+typedef int (*acpi_op_prepare_remove) (struct acpi_device *device);
 
 struct acpi_bus_ops {
 	u32 acpi_op_add:1;
@@ -107,6 +108,7 @@ struct acpi_device_ops {
 	acpi_op_bind bind;
 	acpi_op_unbind unbind;
 	acpi_op_notify notify;
+	acpi_op_prepare_remove prepare_remove;
 };
 
 #define ACPI_DRIVER_ALL_NOTIFY_EVENTS	0x1	/* system AND device events */
-- 
1.7.9

[RFC PATCH 2/3] acpi: Make acpi_bus_trim handle device removal preparation

[Vasilis Liaskovitis]

A new argument is added to acpi_bus_trim, which indicates if we are preparing
for removal or performing the actual ACPI removal. This is needed for safe
removal of memory devices.

The argument change would not be needed if the existing argument rmdevice of
acpi_bus_trim could be used instead. What is the role of rmdevice argument? As
far as I can tell the rmdevice argument is never used at the moment
(acpi_bus_trim is called with rmdevice=1 from all its call sites. It is never
called with rmdevice=0)

Signed-off-by: Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com>
---
 drivers/acpi/acpi_memhotplug.c     |    2 +-
 drivers/acpi/dock.c                |    2 +-
 drivers/acpi/scan.c                |   32 +++++++++++++++++++++++++++++---
 drivers/pci/hotplug/acpiphp_glue.c |    4 ++--
 drivers/pci/hotplug/sgi_hotplug.c  |    2 +-
 include/acpi/acpi_bus.h            |    2 +-
 6 files changed, 35 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
index 92c973a..7fcc844 100644
--- a/drivers/acpi/acpi_memhotplug.c
+++ b/drivers/acpi/acpi_memhotplug.c
@@ -427,7 +427,7 @@ static void acpi_memory_device_notify(acpi_handle handle, u32 event, void *data)
 		/*
 		 * Invoke acpi_bus_trim() to remove memory device
 		 */
-		acpi_bus_trim(device, 1);
+		acpi_bus_trim(device, 1, 0);
 
 		/* _EJ0 succeeded; _OST is not necessary */
 		return;
diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
index ae4ebf2..9e37b49 100644
--- a/drivers/acpi/dock.c
+++ b/drivers/acpi/dock.c
@@ -345,7 +345,7 @@ static void dock_remove_acpi_device(acpi_handle handle)
 	int ret;
 
 	if (!acpi_bus_get_device(handle, &device)) {
-		ret = acpi_bus_trim(device, 1);
+		ret = acpi_bus_trim(device, 1, 0);
 		if (ret)
 			pr_debug("error removing bus, %x\n", -ret);
 	}
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 95ff1e8..b1001a4 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -121,7 +121,12 @@ void acpi_bus_hot_remove_device(void *context)
 	ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 		"Hot-removing device %s...\n", dev_name(&device->dev)));
 
-	if (acpi_bus_trim(device, 1)) {
+	if (acpi_bus_trim(device, 1, 1)) {
+		pr_err("Preparing to removing device failed\n");
+		goto err_out;
+	}
+
+	if (acpi_bus_trim(device, 1, 0)) {
 		printk(KERN_ERR PREFIX
 				"Removing device failed\n");
 		goto err_out;
@@ -1347,6 +1352,19 @@ static int acpi_device_set_context(struct acpi_device *device)
 	return -ENODEV;
 }
 
+static int acpi_bus_prepare_remove(struct acpi_device *dev)
+{
+	int ret = 0;
+
+	if (!dev)
+		return -EINVAL;
+
+	if (dev->driver && dev->driver->ops.prepare_remove)
+		ret = dev->driver->ops.prepare_remove(dev);
+
+	return ret;
+}
+
 static int acpi_bus_remove(struct acpi_device *dev, int rmdevice)
 {
 	if (!dev)
@@ -1640,7 +1658,11 @@ int acpi_bus_start(struct acpi_device *device)
 }
 EXPORT_SYMBOL(acpi_bus_start);
 
-int acpi_bus_trim(struct acpi_device *start, int rmdevice)
+/* acpi_bus_trim: Remove or prepare to remove a device and its children.
+ * @device: the device to remove or prepare to remove from.
+ * @prepare: If 1, prepare for removal. If 0, perform actual removal.
+ */
+int acpi_bus_trim(struct acpi_device *start, int rmdevice, int prepare)
 {
 	acpi_status status;
 	struct acpi_device *parent, *child;
@@ -1667,7 +1689,11 @@ int acpi_bus_trim(struct acpi_device *start, int rmdevice)
 			child = parent;
 			parent = parent->parent;
 
-			if (level == 0)
+			if (prepare) {
+				err = acpi_bus_prepare_remove(child);
+				if (err)
+					return err;
+			} else if (level == 0)
 				err = acpi_bus_remove(child, rmdevice);
 			else
 				err = acpi_bus_remove(child, 1);
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 3d6d4fd..bc10b61 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -748,7 +748,7 @@ static int acpiphp_bus_add(struct acpiphp_func *func)
 		/* this shouldn't be in here, so remove
 		 * the bus then re-add it...
 		 */
-		ret_val = acpi_bus_trim(device, 1);
+		ret_val = acpi_bus_trim(device, 1, 0);
 		dbg("acpi_bus_trim return %x\n", ret_val);
 	}
 
@@ -781,7 +781,7 @@ static int acpiphp_bus_trim(acpi_handle handle)
 		return retval;
 	}
 
-	retval = acpi_bus_trim(device, 1);
+	retval = acpi_bus_trim(device, 1, 0);
 	if (retval)
 		err("cannot remove from acpi list\n");
 
diff --git a/drivers/pci/hotplug/sgi_hotplug.c b/drivers/pci/hotplug/sgi_hotplug.c
index f64ca92..3655de3 100644
--- a/drivers/pci/hotplug/sgi_hotplug.c
+++ b/drivers/pci/hotplug/sgi_hotplug.c
@@ -539,7 +539,7 @@ static int disable_slot(struct hotplug_slot *bss_hotplug_slot)
 				ret = acpi_bus_get_device(chandle,
 							  &device);
 				if (ACPI_SUCCESS(ret))
-					acpi_bus_trim(device, 1);
+					acpi_bus_trim(device, 1, 0);
 			}
 		}
 
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 6ef1692..063c470 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -359,7 +359,7 @@ void acpi_bus_unregister_driver(struct acpi_driver *driver);
 int acpi_bus_add(struct acpi_device **child, struct acpi_device *parent,
 		 acpi_handle handle, int type);
 void acpi_bus_hot_remove_device(void *context);
-int acpi_bus_trim(struct acpi_device *start, int rmdevice);
+int acpi_bus_trim(struct acpi_device *start, int rmdevice, int prepare);
 int acpi_bus_start(struct acpi_device *device);
 acpi_status acpi_bus_get_ejd(acpi_handle handle, acpi_handle * ejd);
 int acpi_match_device_ids(struct acpi_device *device,
-- 
1.7.9

[RFC PATCH 3/3] acpi_memhotplug: Add prepare_remove operation

[Vasilis Liaskovitis]

Offlining and removal of memory is now done in the prepare_remove callback,
not in the remove callback.

Signed-off-by: Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com>
---
 drivers/acpi/acpi_memhotplug.c |   22 ++++++++++++++++++++--
 1 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
index 7fcc844..9b734a5 100644
--- a/drivers/acpi/acpi_memhotplug.c
+++ b/drivers/acpi/acpi_memhotplug.c
@@ -54,6 +54,7 @@ MODULE_LICENSE("GPL");
 
 static int acpi_memory_device_add(struct acpi_device *device);
 static int acpi_memory_device_remove(struct acpi_device *device, int type);
+static int acpi_memory_device_prepare_remove(struct acpi_device *device);
 
 static const struct acpi_device_id memory_device_ids[] = {
 	{ACPI_MEMORY_DEVICE_HID, 0},
@@ -68,6 +69,7 @@ static struct acpi_driver acpi_memory_device_driver = {
 	.ops = {
 		.add = acpi_memory_device_add,
 		.remove = acpi_memory_device_remove,
+		.prepare_remove = acpi_memory_device_prepare_remove,
 		},
 };
 
@@ -499,6 +501,20 @@ static int acpi_memory_device_add(struct acpi_device *device)
 static int acpi_memory_device_remove(struct acpi_device *device, int type)
 {
 	struct acpi_memory_device *mem_device = NULL;
+
+	if (!device || !acpi_driver_data(device))
+		return -EINVAL;
+
+	mem_device = acpi_driver_data(device);
+
+	kfree(mem_device);
+
+	return 0;
+}
+
+static int acpi_memory_device_prepare_remove(struct acpi_device *device)
+{
+	struct acpi_memory_device *mem_device = NULL;
 	int result;
 
 	if (!device || !acpi_driver_data(device))
@@ -506,12 +522,14 @@ static int acpi_memory_device_remove(struct acpi_device *device, int type)
 
 	mem_device = acpi_driver_data(device);
 
+	/*
+	 * offline and remove memory only when the memory device is
+	 * ejected.
+	 */
 	result = acpi_memory_remove_memory(mem_device);
 	if (result)
 		return result;
 
-	kfree(mem_device);
-
 	return 0;
 }
 
-- 
1.7.9

Re: [RFC PATCH 0/3] acpi: Introduce prepare_remove device operation

[Wen Congyang]

At 11/09/2012 02:29 AM, Vasilis Liaskovitis Wrote:
> As discussed in
> https://patchwork.kernel.org/patch/1581581/
> the driver core remove function needs to always succeed. This means we need
> to know that the device can be successfully removed before acpi_bus_trim / 
> acpi_bus_hot_remove_device are called. This can cause panics when OSPM-initiated
> eject (echo 1 > /sys/bus/acpi/devices/PNP/eject) of memory devices fails, since
> the ACPI core goes ahead and ejects the device regardless of whether the memory
> is still in use or not.
> 
> For this reason a new acpi_device operation called prepare_remove is introduced.
> This operation should be registered for acpi devices whose removal (from kernel
> perspective) can fail.  Memory devices fall in this category.
> 
> acpi_bus_hot_remove_device is changed to handle removal in 2 steps:
> - preparation for removal i.e. perform part of removal that can fail outside of
>   ACPI core. Should succeed for device and all its children.
> - if above step was successfull, proceed to actual ACPI removal

If we unbind the device from the driver, we still need to do preparation. But
you don't do it in your patch.

Thanks
Wen Congyang
> 
> acpi_bus_trim is changed accordingly to handle preparation for removal and
> actual removal.
> 
> With this patchset, only acpi memory devices use the new prepare_remove
> device operation. The actual memory removal (VM-related offline and other memory
> cleanups) is moved to prepare_remove. The old remove operation just cleans up
> the acpi structures. Directly ejecting PNP0C80 memory devices works safely. I
> haven't tested yet with an ACPI container which contains memory devices.
> 
> Other ACPI devices (e.g. CPU) do not register prepare_remove callbacks, and
> their OSPM-side eject should not be affected.
> 
> I am not happy with the name prepare_remove. Comments welcome. Let me know if I
> should work more in this direction (I think Yasuaki might also look into this
> and might have a simpler idea)
> 
> Patches are on top of Rafael's linux-pm/linux-next
> 
> Vasilis Liaskovitis (3):
>   acpi: Introduce prepare_remove operation in acpi_device_ops
>   acpi: Make acpi_bus_trim handle device removal preparation
>   acpi_memhotplug: Add prepare_remove operation
> 
>  drivers/acpi/acpi_memhotplug.c     |   24 +++++++++++++++++++++---
>  drivers/acpi/dock.c                |    2 +-
>  drivers/acpi/scan.c                |   32 +++++++++++++++++++++++++++++---
>  drivers/pci/hotplug/acpiphp_glue.c |    4 ++--
>  drivers/pci/hotplug/sgi_hotplug.c  |    2 +-
>  include/acpi/acpi_bus.h            |    4 +++-
>  6 files changed, 57 insertions(+), 11 deletions(-)
> 

Re: [RFC PATCH 0/3] acpi: Introduce prepare_remove device operation

[Vasilis Liaskovitis]

On Mon, Nov 12, 2012 at 12:00:55PM +0800, Wen Congyang wrote:
> At 11/09/2012 02:29 AM, Vasilis Liaskovitis Wrote:
> > As discussed in
> > https://patchwork.kernel.org/patch/1581581/
> > the driver core remove function needs to always succeed. This means we need
> > to know that the device can be successfully removed before acpi_bus_trim / 
> > acpi_bus_hot_remove_device are called. This can cause panics when OSPM-initiated
> > eject (echo 1 > /sys/bus/acpi/devices/PNP/eject) of memory devices fails, since
> > the ACPI core goes ahead and ejects the device regardless of whether the memory
> > is still in use or not.
> > 
> > For this reason a new acpi_device operation called prepare_remove is introduced.
> > This operation should be registered for acpi devices whose removal (from kernel
> > perspective) can fail.  Memory devices fall in this category.
> > 
> > acpi_bus_hot_remove_device is changed to handle removal in 2 steps:
> > - preparation for removal i.e. perform part of removal that can fail outside of
> >   ACPI core. Should succeed for device and all its children.
> > - if above step was successfull, proceed to actual ACPI removal
> 
> If we unbind the device from the driver, we still need to do preparation. But
> you don't do it in your patch.

yes, driver_unbind breaks with the current patchset. I 'll try to fix and
repost. However, I think this will require a new driver-core wide prepare_remove
callback (not only acpi-specific). I am not sure that would be acceptable.

thanks,

- Vasilis

> 
> Thanks
> Wen Congyang
> > 
> > acpi_bus_trim is changed accordingly to handle preparation for removal and
> > actual removal.
> > 
> > With this patchset, only acpi memory devices use the new prepare_remove
> > device operation. The actual memory removal (VM-related offline and other memory
> > cleanups) is moved to prepare_remove. The old remove operation just cleans up
> > the acpi structures. Directly ejecting PNP0C80 memory devices works safely. I
> > haven't tested yet with an ACPI container which contains memory devices.
> > 
> > Other ACPI devices (e.g. CPU) do not register prepare_remove callbacks, and
> > their OSPM-side eject should not be affected.
> > 
> > I am not happy with the name prepare_remove. Comments welcome. Let me know if I
> > should work more in this direction (I think Yasuaki might also look into this
> > and might have a simpler idea)
> > 
> > Patches are on top of Rafael's linux-pm/linux-next
> > 
> > Vasilis Liaskovitis (3):
> >   acpi: Introduce prepare_remove operation in acpi_device_ops
> >   acpi: Make acpi_bus_trim handle device removal preparation
> >   acpi_memhotplug: Add prepare_remove operation
> > 
> >  drivers/acpi/acpi_memhotplug.c     |   24 +++++++++++++++++++++---
> >  drivers/acpi/dock.c                |    2 +-
> >  drivers/acpi/scan.c                |   32 +++++++++++++++++++++++++++++---
> >  drivers/pci/hotplug/acpiphp_glue.c |    4 ++--
> >  drivers/pci/hotplug/sgi_hotplug.c  |    2 +-
> >  include/acpi/acpi_bus.h            |    4 +++-
> >  6 files changed, 57 insertions(+), 11 deletions(-)
> > 
> 

Re: [RFC PATCH 0/3] acpi: Introduce prepare_remove device operation

[Rafael J. Wysocki]

On Monday, November 12, 2012 06:20:47 PM Vasilis Liaskovitis wrote:
> On Mon, Nov 12, 2012 at 12:00:55PM +0800, Wen Congyang wrote:
> > At 11/09/2012 02:29 AM, Vasilis Liaskovitis Wrote:
> > > As discussed in
> > > https://patchwork.kernel.org/patch/1581581/
> > > the driver core remove function needs to always succeed. This means we need
> > > to know that the device can be successfully removed before acpi_bus_trim / 
> > > acpi_bus_hot_remove_device are called. This can cause panics when OSPM-initiated
> > > eject (echo 1 > /sys/bus/acpi/devices/PNP/eject) of memory devices fails, since
> > > the ACPI core goes ahead and ejects the device regardless of whether the memory
> > > is still in use or not.
> > > 
> > > For this reason a new acpi_device operation called prepare_remove is introduced.
> > > This operation should be registered for acpi devices whose removal (from kernel
> > > perspective) can fail.  Memory devices fall in this category.
> > > 
> > > acpi_bus_hot_remove_device is changed to handle removal in 2 steps:
> > > - preparation for removal i.e. perform part of removal that can fail outside of
> > >   ACPI core. Should succeed for device and all its children.
> > > - if above step was successfull, proceed to actual ACPI removal
> > 
> > If we unbind the device from the driver, we still need to do preparation. But
> > you don't do it in your patch.
> 
> yes, driver_unbind breaks with the current patchset. I 'll try to fix and
> repost. However, I think this will require a new driver-core wide prepare_remove
> callback (not only acpi-specific). I am not sure that would be acceptable.

However, you can't break driver_unbind either.

Thanks,
Rafael


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