The memory device can be removed by 2 ways:
1. send eject request by SCI
2. echo 1 >/sys/bus/pci/devices/PNP0C80:XX/eject
In the 1st case, acpi_memory_disable_device() will be called.
In the 2nd case, acpi_memory_device_remove() will be called.
acpi_memory_device_remove() will also be called when we unbind the
memory device from the driver acpi_memhotplug or a driver initialization
fails.
acpi_memory_disable_device() has already implemented a code which
offlines memory and releases acpi_memory_info struct . But
acpi_memory_device_remove() has not implemented it yet.
So the patch prepares the framework for hot removing memory and
adds the framework into acpi_memory_device_remove().
We may hotremove the memory device by this 2 ways at the same time.
So we remove the function acpi_memory_disable_device(), and use
acpi_bus_hot_remove_device() which is used by 2nd case to implement it.
We lock device in acpi_bus_hot_remove_device(), so there is no
need to add lock in acpi_memhotplug.
The last version of this patchset is here:
https://lkml.org/lkml/2012/11/8/121
Note:
1. The following commit in pm tree can be dropped now(The other two patches
are already dropped):
54c4c7db6cb94d7d1217df6d7fca6847c61744ab
2. This patchset requires the following patch(It is in pm tree now)
https://lkml.org/lkml/2012/11/1/225
Changes from v4 to v5:
1. patch2: new patch. use acpi_bus_hot_remove_device() to implement memory
device hotremove.
Changes from v3 to v4:
1. patch1: unlock list_lock when removing memory fails.
2. patch2: just rebase them
3. patch3-7: these patches are in -mm tree, and they conflict with this
patchset, so Adrew Morton drop them from -mm tree. I rebase and merge
them into this patchset.
Wen Congyang (6):
acpi,memory-hotplug: deal with eject request in hotplug queue
acpi_memhotplug.c: fix memory leak when memory device is unbound from
the module acpi_memhotplug
acpi_memhotplug.c: free memory device if acpi_memory_enable_device()
failed
acpi_memhotplug.c: don't allow to eject the memory device if it is
being used
acpi_memhotplug.c: bind the memory device when the driver is being
loaded
acpi_memhotplug.c: auto bind the memory device which is hotplugged
before the driver is loaded
Yasuaki Ishimatsu (1):
acpi,memory-hotplug : add memory offline code to
acpi_memory_device_remove()
drivers/acpi/acpi_memhotplug.c | 206 ++++++++++++++++++++++-------------------
1 file changed, 109 insertions(+), 97 deletions(-)
--
1.8.0
We had introduced acpi_hotmem_initialized to avoid strange add_memory fail
message. But the memory device may not be used by the kernel, and the
device should be bound when the driver is being loaded. Remove
acpi_hotmem_initialized to allow that the device can be bound when the
driver is being loaded.
CC: David Rientjes <[email protected]>
CC: Jiang Liu <[email protected]>
CC: Len Brown <[email protected]>
CC: Benjamin Herrenschmidt <[email protected]>
CC: Paul Mackerras <[email protected]>
CC: Christoph Lameter <[email protected]>
Cc: Minchan Kim <[email protected]>
CC: Andrew Morton <[email protected]>
CC: KOSAKI Motohiro <[email protected]>
CC: Yasuaki Ishimatsu <[email protected]>
CC: Rafael J. Wysocki <[email protected]>
CC: Konrad Rzeszutek Wilk <[email protected]>
Signed-off-by: Wen Congyang <[email protected]>
---
drivers/acpi/acpi_memhotplug.c | 12 ------------
1 file changed, 12 deletions(-)
diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
index f7e3007..e0f7425 100644
--- a/drivers/acpi/acpi_memhotplug.c
+++ b/drivers/acpi/acpi_memhotplug.c
@@ -87,8 +87,6 @@ struct acpi_memory_device {
struct list_head res_list;
};
-static int acpi_hotmem_initialized;
-
static acpi_status
acpi_memory_get_resource(struct acpi_resource *resource, void *context)
{
@@ -433,15 +431,6 @@ static int acpi_memory_device_add(struct acpi_device *device)
printk(KERN_DEBUG "%s \n", acpi_device_name(device));
- /*
- * Early boot code has recognized memory area by EFI/E820.
- * If DSDT shows these memory devices on boot, hotplug is not necessary
- * for them. So, it just returns until completion of this driver's
- * start up.
- */
- if (!acpi_hotmem_initialized)
- return 0;
-
if (!acpi_memory_check_device(mem_device)) {
/* call add_memory func */
result = acpi_memory_enable_device(mem_device);
@@ -557,7 +546,6 @@ static int __init acpi_memory_device_init(void)
return -ENODEV;
}
- acpi_hotmem_initialized = 1;
return 0;
}
--
1.8.0
If acpi_memory_enable_device() fails, acpi_memory_enable_device() will
return a non-zero value, which means we fail to bind the memory device to
this driver. So we should free memory device before
acpi_memory_device_add() returns.
CC: David Rientjes <[email protected]>
CC: Jiang Liu <[email protected]>
CC: Len Brown <[email protected]>
CC: Benjamin Herrenschmidt <[email protected]>
CC: Paul Mackerras <[email protected]>
CC: Christoph Lameter <[email protected]>
Cc: Minchan Kim <[email protected]>
CC: Andrew Morton <[email protected]>
CC: KOSAKI Motohiro <[email protected]>
CC: Yasuaki Ishimatsu <[email protected]>
CC: Rafael J. Wysocki <[email protected]>
CC: Konrad Rzeszutek Wilk <[email protected]>
Signed-off-by: Wen Congyang <[email protected]>
---
drivers/acpi/acpi_memhotplug.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
index c5e7b6d..e52ad5d 100644
--- a/drivers/acpi/acpi_memhotplug.c
+++ b/drivers/acpi/acpi_memhotplug.c
@@ -421,9 +421,11 @@ static int acpi_memory_device_add(struct acpi_device *device)
if (!acpi_memory_check_device(mem_device)) {
/* call add_memory func */
result = acpi_memory_enable_device(mem_device);
- if (result)
+ if (result) {
printk(KERN_ERR PREFIX
"Error in acpi_memory_enable_device\n");
+ acpi_memory_device_free(mem_device);
+ }
}
return result;
}
--
1.8.0
From: Yasuaki Ishimatsu <[email protected]>
The memory device can be removed by 2 ways:
1. send eject request by SCI
2. echo 1 >/sys/bus/pci/devices/PNP0C80:XX/eject
In the 1st case, acpi_memory_disable_device() will be called.
In the 2nd case, acpi_memory_device_remove() will be called.
acpi_memory_device_remove() will also be called when we unbind the
memory device from the driver acpi_memhotplug or a driver initialization
fails.
acpi_memory_disable_device() has already implemented a code which
offlines memory and releases acpi_memory_info struct. But
acpi_memory_device_remove() has not implemented it yet.
So the patch move offlining memory and releasing acpi_memory_info struct
codes to a new function acpi_memory_remove_memory(). And it is used by both
acpi_memory_device_remove() and acpi_memory_disable_device().
CC: David Rientjes <[email protected]>
CC: Jiang Liu <[email protected]>
CC: Len Brown <[email protected]>
CC: Benjamin Herrenschmidt <[email protected]>
CC: Paul Mackerras <[email protected]>
CC: Christoph Lameter <[email protected]>
Cc: Minchan Kim <[email protected]>
CC: Andrew Morton <[email protected]>
CC: KOSAKI Motohiro <[email protected]>
CC: Rafael J. Wysocki <[email protected]>
CC: Konrad Rzeszutek Wilk <[email protected]>
Signed-off-by: Yasuaki Ishimatsu <[email protected]>
Signed-off-by: Wen Congyang <[email protected]>
---
drivers/acpi/acpi_memhotplug.c | 31 ++++++++++++++++++++++++-------
1 file changed, 24 insertions(+), 7 deletions(-)
diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
index 24c807f..2918be1 100644
--- a/drivers/acpi/acpi_memhotplug.c
+++ b/drivers/acpi/acpi_memhotplug.c
@@ -306,25 +306,37 @@ static int acpi_memory_powerdown_device(struct acpi_memory_device *mem_device)
return 0;
}
-static int acpi_memory_disable_device(struct acpi_memory_device *mem_device)
+static int acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
{
int result;
struct acpi_memory_info *info, *n;
-
- /*
- * Ask the VM to offline this memory range.
- * Note: Assume that this function returns zero on success
- */
list_for_each_entry_safe(info, n, &mem_device->res_list, list) {
if (info->enabled) {
result = remove_memory(info->start_addr, info->length);
if (result)
return result;
}
+
+ list_del(&info->list);
kfree(info);
}
+ return 0;
+}
+
+static int acpi_memory_disable_device(struct acpi_memory_device *mem_device)
+{
+ int result;
+
+ /*
+ * Ask the VM to offline this memory range.
+ * Note: Assume that this function returns zero on success
+ */
+ result = acpi_memory_remove_memory(mem_device);
+ if (result)
+ return result;
+
/* Power-off and eject the device */
result = acpi_memory_powerdown_device(mem_device);
if (result) {
@@ -473,12 +485,17 @@ 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;
-
+ int result;
if (!device || !acpi_driver_data(device))
return -EINVAL;
mem_device = acpi_driver_data(device);
+
+ result = acpi_memory_remove_memory(mem_device);
+ if (result)
+ return result;
+
kfree(mem_device);
return 0;
--
1.8.0
The memory device can be removed by 2 ways:
1. send eject request by SCI
2. echo 1 >/sys/bus/pci/devices/PNP0C80:XX/eject
We handle the 1st case in the module acpi_memhotplug, and handle
the 2nd case in ACPI eject notification. This 2 events may happen
at the same time, so we may touch acpi_memory_device.res_list at
the same time. This patch reimplements memory-hotremove support
through an ACPI eject notification. Now the memory device is
offlined and hotremoved only in the function acpi_memory_device_remove()
which is protected by device_lock().
CC: David Rientjes <[email protected]>
CC: Jiang Liu <[email protected]>
CC: Len Brown <[email protected]>
CC: Benjamin Herrenschmidt <[email protected]>
CC: Paul Mackerras <[email protected]>
CC: Christoph Lameter <[email protected]>
Cc: Minchan Kim <[email protected]>
CC: Andrew Morton <[email protected]>
CC: KOSAKI Motohiro <[email protected]>
CC: Yasuaki Ishimatsu <[email protected]>
CC: Rafael J. Wysocki <[email protected]>
CC: Konrad Rzeszutek Wilk <[email protected]>
Signed-off-by: Wen Congyang <[email protected]>
---
drivers/acpi/acpi_memhotplug.c | 87 +++++-------------------------------------
1 file changed, 9 insertions(+), 78 deletions(-)
diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
index 2918be1..6e12042 100644
--- a/drivers/acpi/acpi_memhotplug.c
+++ b/drivers/acpi/acpi_memhotplug.c
@@ -272,40 +272,6 @@ static int acpi_memory_enable_device(struct acpi_memory_device *mem_device)
return 0;
}
-static int acpi_memory_powerdown_device(struct acpi_memory_device *mem_device)
-{
- acpi_status status;
- struct acpi_object_list arg_list;
- union acpi_object arg;
- unsigned long long current_status;
-
-
- /* Issue the _EJ0 command */
- arg_list.count = 1;
- arg_list.pointer = &arg;
- arg.type = ACPI_TYPE_INTEGER;
- arg.integer.value = 1;
- status = acpi_evaluate_object(mem_device->device->handle,
- "_EJ0", &arg_list, NULL);
- /* Return on _EJ0 failure */
- if (ACPI_FAILURE(status)) {
- ACPI_EXCEPTION((AE_INFO, status, "_EJ0 failed"));
- return -ENODEV;
- }
-
- /* Evalute _STA to check if the device is disabled */
- status = acpi_evaluate_integer(mem_device->device->handle, "_STA",
- NULL, ¤t_status);
- if (ACPI_FAILURE(status))
- return -ENODEV;
-
- /* Check for device status. Device should be disabled */
- if (current_status & ACPI_STA_DEVICE_ENABLED)
- return -EINVAL;
-
- return 0;
-}
-
static int acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
{
int result;
@@ -325,34 +291,11 @@ static int acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
return 0;
}
-static int acpi_memory_disable_device(struct acpi_memory_device *mem_device)
-{
- int result;
-
- /*
- * Ask the VM to offline this memory range.
- * Note: Assume that this function returns zero on success
- */
- result = acpi_memory_remove_memory(mem_device);
- if (result)
- return result;
-
- /* Power-off and eject the device */
- result = acpi_memory_powerdown_device(mem_device);
- if (result) {
- /* Set the status of the device to invalid */
- mem_device->state = MEMORY_INVALID_STATE;
- return result;
- }
-
- mem_device->state = MEMORY_POWER_OFF_STATE;
- return result;
-}
-
static void acpi_memory_device_notify(acpi_handle handle, u32 event, void *data)
{
struct acpi_memory_device *mem_device;
struct acpi_device *device;
+ struct acpi_eject_event *ej_event = NULL;
u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE; /* default */
switch (event) {
@@ -394,31 +337,19 @@ static void acpi_memory_device_notify(acpi_handle handle, u32 event, void *data)
break;
}
- /*
- * Currently disabling memory device from kernel mode
- * TBD: Can also be disabled from user mode scripts
- * TBD: Can also be disabled by Callback registration
- * with generic sysfs driver
- */
- if (acpi_memory_disable_device(mem_device)) {
- printk(KERN_ERR PREFIX "Disable memory device\n");
- /*
- * If _EJ0 was called but failed, _OST is not
- * necessary.
- */
- if (mem_device->state == MEMORY_INVALID_STATE)
- return;
-
+ ej_event = kmalloc(sizeof(*ej_event), GFP_KERNEL);
+ if (!ej_event) {
+ pr_err(PREFIX "No memory, dropping EJECT\n");
break;
}
- /*
- * TBD: Invoke acpi_bus_remove to cleanup data structures
- */
+ ej_event->handle = handle;
+ ej_event->event = ACPI_NOTIFY_EJECT_REQUEST;
+ acpi_os_hotplug_execute(acpi_bus_hot_remove_device,
+ (void *)ej_event);
- /* _EJ0 succeeded; _OST is not necessary */
+ /* eject is performed asynchronously */
return;
-
default:
ACPI_DEBUG_PRINT((ACPI_DB_INFO,
"Unsupported event [0x%x]\n", event));
--
1.8.0
If the memory device is hotplugged before the driver is loaded, the user
cannot see this device under the directory /sys/bus/acpi/devices/, and the
user cannot bind it by hand after the driver is loaded. This patch
introduces a new feature to bind such device when the driver is being
loaded.
CC: David Rientjes <[email protected]>
CC: Jiang Liu <[email protected]>
CC: Len Brown <[email protected]>
CC: Benjamin Herrenschmidt <[email protected]>
CC: Paul Mackerras <[email protected]>
CC: Christoph Lameter <[email protected]>
Cc: Minchan Kim <[email protected]>
CC: Andrew Morton <[email protected]>
CC: KOSAKI Motohiro <[email protected]>
CC: Yasuaki Ishimatsu <[email protected]>
CC: Rafael J. Wysocki <[email protected]>
CC: Konrad Rzeszutek Wilk <[email protected]>
Signed-off-by: Wen Congyang <[email protected]>
---
drivers/acpi/acpi_memhotplug.c | 37 ++++++++++++++++++++++++++++++++++++-
1 file changed, 36 insertions(+), 1 deletion(-)
diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
index e0f7425..9f1d107 100644
--- a/drivers/acpi/acpi_memhotplug.c
+++ b/drivers/acpi/acpi_memhotplug.c
@@ -52,6 +52,9 @@ MODULE_LICENSE("GPL");
#define MEMORY_POWER_ON_STATE 1
#define MEMORY_POWER_OFF_STATE 2
+static bool auto_probe;
+module_param(auto_probe, bool, S_IRUGO | S_IWUSR);
+
static int acpi_memory_device_add(struct acpi_device *device);
static int acpi_memory_device_remove(struct acpi_device *device, int type);
@@ -494,12 +497,44 @@ acpi_memory_register_notify_handler(acpi_handle handle,
u32 level, void *ctxt, void **retv)
{
acpi_status status;
-
+ struct acpi_memory_device *mem_device = NULL;
+ unsigned long long current_status;
status = is_memory_device(handle);
if (ACPI_FAILURE(status))
return AE_OK; /* continue */
+ if (auto_probe) {
+ /* Get device present/absent information from the _STA */
+ status = acpi_evaluate_integer(handle, "_STA", NULL,
+ ¤t_status);
+ if (ACPI_FAILURE(status))
+ goto install;
+
+ /*
+ * Check for device status. Device should be
+ * present/enabled/functioning.
+ */
+ if (!(current_status &
+ (ACPI_STA_DEVICE_PRESENT | ACPI_STA_DEVICE_ENABLED |
+ ACPI_STA_DEVICE_FUNCTIONING)))
+ goto install;
+
+ if (acpi_memory_get_device(handle, &mem_device))
+ goto install;
+
+ /* We have bound this device while we register the driver */
+ if (mem_device->state == MEMORY_POWER_ON_STATE)
+ goto install;
+
+ ACPI_DEBUG_PRINT((ACPI_DB_INFO,
+ "\nauto probe memory device\n"));
+
+ if (acpi_memory_enable_device(mem_device))
+ pr_err(PREFIX "Cannot enable memory device\n");
+ }
+
+install:
status = acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
acpi_memory_device_notify, NULL);
/* continue */
--
1.8.0
We eject the memory device even if it is in use. It is very dangerous,
and it will cause the kernel to be panicked.
CC: David Rientjes <[email protected]>
CC: Jiang Liu <[email protected]>
CC: Len Brown <[email protected]>
CC: Benjamin Herrenschmidt <[email protected]>
CC: Paul Mackerras <[email protected]>
CC: Christoph Lameter <[email protected]>
Cc: Minchan Kim <[email protected]>
CC: Andrew Morton <[email protected]>
CC: KOSAKI Motohiro <[email protected]>
CC: Yasuaki Ishimatsu <[email protected]>
CC: Rafael J. Wysocki <[email protected]>
CC: Konrad Rzeszutek Wilk <[email protected]>
Signed-off-by: Wen Congyang <[email protected]>
---
drivers/acpi/acpi_memhotplug.c | 42 +++++++++++++++++++++++++++++++++---------
1 file changed, 33 insertions(+), 9 deletions(-)
diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
index e52ad5d..f7e3007 100644
--- a/drivers/acpi/acpi_memhotplug.c
+++ b/drivers/acpi/acpi_memhotplug.c
@@ -78,6 +78,7 @@ struct acpi_memory_info {
unsigned short caching; /* memory cache attribute */
unsigned short write_protect; /* memory read/write attribute */
unsigned int enabled:1;
+ unsigned int failed:1;
};
struct acpi_memory_device {
@@ -257,9 +258,23 @@ static int acpi_memory_enable_device(struct acpi_memory_device *mem_device)
node = memory_add_physaddr_to_nid(info->start_addr);
result = add_memory(node, info->start_addr, info->length);
- if (result)
+
+ /*
+ * If the memory block has been used by the kernel, add_memory()
+ * returns -EEXIST. If add_memory() returns the other error, it
+ * means that this memory block is not used by the kernel.
+ */
+ if (result && result != -EEXIST) {
+ info->failed = 1;
continue;
- info->enabled = 1;
+ }
+
+ if (!result)
+ info->enabled = 1;
+ /*
+ * Add num_enable even if add_memory() returns -EEXIST, so the
+ * device is bound to this driver.
+ */
num_enabled++;
}
if (!num_enabled) {
@@ -280,21 +295,30 @@ static int acpi_memory_enable_device(struct acpi_memory_device *mem_device)
static int acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
{
- int result;
+ int result = 0;
struct acpi_memory_info *info, *n;
list_for_each_entry_safe(info, n, &mem_device->res_list, list) {
- if (info->enabled) {
- result = remove_memory(info->start_addr, info->length);
- if (result)
- return result;
- }
+ if (info->failed)
+ /* The kernel does not use this memory block */
+ continue;
+
+ if (!info->enabled)
+ /*
+ * The kernel uses this memory block, but it may be not
+ * managed by us.
+ */
+ return -EBUSY;
+
+ result = remove_memory(info->start_addr, info->length);
+ if (result)
+ return result;
list_del(&info->list);
kfree(info);
}
- return 0;
+ return result;
}
static void acpi_memory_device_notify(acpi_handle handle, u32 event, void *data)
--
1.8.0
We allocate memory to store acpi_memory_info, so we should free it before
freeing mem_device.
CC: David Rientjes <[email protected]>
CC: Jiang Liu <[email protected]>
CC: Len Brown <[email protected]>
CC: Benjamin Herrenschmidt <[email protected]>
CC: Paul Mackerras <[email protected]>
CC: Christoph Lameter <[email protected]>
Cc: Minchan Kim <[email protected]>
CC: Andrew Morton <[email protected]>
CC: KOSAKI Motohiro <[email protected]>
CC: Yasuaki Ishimatsu <[email protected]>
CC: Rafael J. Wysocki <[email protected]>
CC: Konrad Rzeszutek Wilk <[email protected]>
Signed-off-by: Wen Congyang <[email protected]>
---
drivers/acpi/acpi_memhotplug.c | 27 +++++++++++++++++++++------
1 file changed, 21 insertions(+), 6 deletions(-)
diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
index 6e12042..c5e7b6d 100644
--- a/drivers/acpi/acpi_memhotplug.c
+++ b/drivers/acpi/acpi_memhotplug.c
@@ -125,12 +125,20 @@ acpi_memory_get_resource(struct acpi_resource *resource, void *context)
return AE_OK;
}
+static void
+acpi_memory_free_device_resources(struct acpi_memory_device *mem_device)
+{
+ struct acpi_memory_info *info, *n;
+
+ list_for_each_entry_safe(info, n, &mem_device->res_list, list)
+ kfree(info);
+ INIT_LIST_HEAD(&mem_device->res_list);
+}
+
static int
acpi_memory_get_device_resources(struct acpi_memory_device *mem_device)
{
acpi_status status;
- struct acpi_memory_info *info, *n;
-
if (!list_empty(&mem_device->res_list))
return 0;
@@ -138,9 +146,7 @@ acpi_memory_get_device_resources(struct acpi_memory_device *mem_device)
status = acpi_walk_resources(mem_device->device->handle, METHOD_NAME__CRS,
acpi_memory_get_resource, mem_device);
if (ACPI_FAILURE(status)) {
- list_for_each_entry_safe(info, n, &mem_device->res_list, list)
- kfree(info);
- INIT_LIST_HEAD(&mem_device->res_list);
+ acpi_memory_free_device_resources(mem_device);
return -EINVAL;
}
@@ -363,6 +369,15 @@ static void acpi_memory_device_notify(acpi_handle handle, u32 event, void *data)
return;
}
+static void acpi_memory_device_free(struct acpi_memory_device *mem_device)
+{
+ if (!mem_device)
+ return;
+
+ acpi_memory_free_device_resources(mem_device);
+ kfree(mem_device);
+}
+
static int acpi_memory_device_add(struct acpi_device *device)
{
int result;
@@ -427,7 +442,7 @@ static int acpi_memory_device_remove(struct acpi_device *device, int type)
if (result)
return result;
- kfree(mem_device);
+ acpi_memory_device_free(mem_device);
return 0;
}
--
1.8.0
2012/11/15 15:59, Wen Congyang wrote:
> The memory device can be removed by 2 ways:
> 1. send eject request by SCI
> 2. echo 1 >/sys/bus/pci/devices/PNP0C80:XX/eject
>
> We handle the 1st case in the module acpi_memhotplug, and handle
> the 2nd case in ACPI eject notification. This 2 events may happen
> at the same time, so we may touch acpi_memory_device.res_list at
> the same time. This patch reimplements memory-hotremove support
> through an ACPI eject notification. Now the memory device is
> offlined and hotremoved only in the function acpi_memory_device_remove()
> which is protected by device_lock().
>
> CC: David Rientjes <[email protected]>
> CC: Jiang Liu <[email protected]>
> CC: Len Brown <[email protected]>
> CC: Benjamin Herrenschmidt <[email protected]>
> CC: Paul Mackerras <[email protected]>
> CC: Christoph Lameter <[email protected]>
> Cc: Minchan Kim <[email protected]>
> CC: Andrew Morton <[email protected]>
> CC: KOSAKI Motohiro <[email protected]>
> CC: Yasuaki Ishimatsu <[email protected]>
> CC: Rafael J. Wysocki <[email protected]>
> CC: Konrad Rzeszutek Wilk <[email protected]>
> Signed-off-by: Wen Congyang <[email protected]>
> ---
Reviewed-by: Yasuaki Ishimatsu <[email protected]>
Thanks,
Yasuaki Ishimatsu
> drivers/acpi/acpi_memhotplug.c | 87 +++++-------------------------------------
> 1 file changed, 9 insertions(+), 78 deletions(-)
>
> diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
> index 2918be1..6e12042 100644
> --- a/drivers/acpi/acpi_memhotplug.c
> +++ b/drivers/acpi/acpi_memhotplug.c
> @@ -272,40 +272,6 @@ static int acpi_memory_enable_device(struct acpi_memory_device *mem_device)
> return 0;
> }
>
> -static int acpi_memory_powerdown_device(struct acpi_memory_device *mem_device)
> -{
> - acpi_status status;
> - struct acpi_object_list arg_list;
> - union acpi_object arg;
> - unsigned long long current_status;
> -
> -
> - /* Issue the _EJ0 command */
> - arg_list.count = 1;
> - arg_list.pointer = &arg;
> - arg.type = ACPI_TYPE_INTEGER;
> - arg.integer.value = 1;
> - status = acpi_evaluate_object(mem_device->device->handle,
> - "_EJ0", &arg_list, NULL);
> - /* Return on _EJ0 failure */
> - if (ACPI_FAILURE(status)) {
> - ACPI_EXCEPTION((AE_INFO, status, "_EJ0 failed"));
> - return -ENODEV;
> - }
> -
> - /* Evalute _STA to check if the device is disabled */
> - status = acpi_evaluate_integer(mem_device->device->handle, "_STA",
> - NULL, ¤t_status);
> - if (ACPI_FAILURE(status))
> - return -ENODEV;
> -
> - /* Check for device status. Device should be disabled */
> - if (current_status & ACPI_STA_DEVICE_ENABLED)
> - return -EINVAL;
> -
> - return 0;
> -}
> -
> static int acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
> {
> int result;
> @@ -325,34 +291,11 @@ static int acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
> return 0;
> }
>
> -static int acpi_memory_disable_device(struct acpi_memory_device *mem_device)
> -{
> - int result;
> -
> - /*
> - * Ask the VM to offline this memory range.
> - * Note: Assume that this function returns zero on success
> - */
> - result = acpi_memory_remove_memory(mem_device);
> - if (result)
> - return result;
> -
> - /* Power-off and eject the device */
> - result = acpi_memory_powerdown_device(mem_device);
> - if (result) {
> - /* Set the status of the device to invalid */
> - mem_device->state = MEMORY_INVALID_STATE;
> - return result;
> - }
> -
> - mem_device->state = MEMORY_POWER_OFF_STATE;
> - return result;
> -}
> -
> static void acpi_memory_device_notify(acpi_handle handle, u32 event, void *data)
> {
> struct acpi_memory_device *mem_device;
> struct acpi_device *device;
> + struct acpi_eject_event *ej_event = NULL;
> u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE; /* default */
>
> switch (event) {
> @@ -394,31 +337,19 @@ static void acpi_memory_device_notify(acpi_handle handle, u32 event, void *data)
> break;
> }
>
> - /*
> - * Currently disabling memory device from kernel mode
> - * TBD: Can also be disabled from user mode scripts
> - * TBD: Can also be disabled by Callback registration
> - * with generic sysfs driver
> - */
> - if (acpi_memory_disable_device(mem_device)) {
> - printk(KERN_ERR PREFIX "Disable memory device\n");
> - /*
> - * If _EJ0 was called but failed, _OST is not
> - * necessary.
> - */
> - if (mem_device->state == MEMORY_INVALID_STATE)
> - return;
> -
> + ej_event = kmalloc(sizeof(*ej_event), GFP_KERNEL);
> + if (!ej_event) {
> + pr_err(PREFIX "No memory, dropping EJECT\n");
> break;
> }
>
> - /*
> - * TBD: Invoke acpi_bus_remove to cleanup data structures
> - */
> + ej_event->handle = handle;
> + ej_event->event = ACPI_NOTIFY_EJECT_REQUEST;
> + acpi_os_hotplug_execute(acpi_bus_hot_remove_device,
> + (void *)ej_event);
>
> - /* _EJ0 succeeded; _OST is not necessary */
> + /* eject is performed asynchronously */
> return;
> -
> default:
> ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> "Unsupported event [0x%x]\n", event));
>
2012/11/15 15:59, Wen Congyang wrote:
> We allocate memory to store acpi_memory_info, so we should free it before
> freeing mem_device.
>
> CC: David Rientjes <[email protected]>
> CC: Jiang Liu <[email protected]>
> CC: Len Brown <[email protected]>
> CC: Benjamin Herrenschmidt <[email protected]>
> CC: Paul Mackerras <[email protected]>
> CC: Christoph Lameter <[email protected]>
> Cc: Minchan Kim <[email protected]>
> CC: Andrew Morton <[email protected]>
> CC: KOSAKI Motohiro <[email protected]>
> CC: Yasuaki Ishimatsu <[email protected]>
> CC: Rafael J. Wysocki <[email protected]>
> CC: Konrad Rzeszutek Wilk <[email protected]>
> Signed-off-by: Wen Congyang <[email protected]>
> ---
Reviewed-by: Yasuaki Ishimatsu <[email protected]>
Thanks,
Yasuaki Ishimatsu
> drivers/acpi/acpi_memhotplug.c | 27 +++++++++++++++++++++------
> 1 file changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
> index 6e12042..c5e7b6d 100644
> --- a/drivers/acpi/acpi_memhotplug.c
> +++ b/drivers/acpi/acpi_memhotplug.c
> @@ -125,12 +125,20 @@ acpi_memory_get_resource(struct acpi_resource *resource, void *context)
> return AE_OK;
> }
>
> +static void
> +acpi_memory_free_device_resources(struct acpi_memory_device *mem_device)
> +{
> + struct acpi_memory_info *info, *n;
> +
> + list_for_each_entry_safe(info, n, &mem_device->res_list, list)
> + kfree(info);
> + INIT_LIST_HEAD(&mem_device->res_list);
> +}
> +
> static int
> acpi_memory_get_device_resources(struct acpi_memory_device *mem_device)
> {
> acpi_status status;
> - struct acpi_memory_info *info, *n;
> -
>
> if (!list_empty(&mem_device->res_list))
> return 0;
> @@ -138,9 +146,7 @@ acpi_memory_get_device_resources(struct acpi_memory_device *mem_device)
> status = acpi_walk_resources(mem_device->device->handle, METHOD_NAME__CRS,
> acpi_memory_get_resource, mem_device);
> if (ACPI_FAILURE(status)) {
> - list_for_each_entry_safe(info, n, &mem_device->res_list, list)
> - kfree(info);
> - INIT_LIST_HEAD(&mem_device->res_list);
> + acpi_memory_free_device_resources(mem_device);
> return -EINVAL;
> }
>
> @@ -363,6 +369,15 @@ static void acpi_memory_device_notify(acpi_handle handle, u32 event, void *data)
> return;
> }
>
> +static void acpi_memory_device_free(struct acpi_memory_device *mem_device)
> +{
> + if (!mem_device)
> + return;
> +
> + acpi_memory_free_device_resources(mem_device);
> + kfree(mem_device);
> +}
> +
> static int acpi_memory_device_add(struct acpi_device *device)
> {
> int result;
> @@ -427,7 +442,7 @@ static int acpi_memory_device_remove(struct acpi_device *device, int type)
> if (result)
> return result;
>
> - kfree(mem_device);
> + acpi_memory_device_free(mem_device);
>
> return 0;
> }
>
2012/11/15 15:59, Wen Congyang wrote:
> If acpi_memory_enable_device() fails, acpi_memory_enable_device() will
> return a non-zero value, which means we fail to bind the memory device to
> this driver. So we should free memory device before
> acpi_memory_device_add() returns.
>
> CC: David Rientjes <[email protected]>
> CC: Jiang Liu <[email protected]>
> CC: Len Brown <[email protected]>
> CC: Benjamin Herrenschmidt <[email protected]>
> CC: Paul Mackerras <[email protected]>
> CC: Christoph Lameter <[email protected]>
> Cc: Minchan Kim <[email protected]>
> CC: Andrew Morton <[email protected]>
> CC: KOSAKI Motohiro <[email protected]>
> CC: Yasuaki Ishimatsu <[email protected]>
> CC: Rafael J. Wysocki <[email protected]>
> CC: Konrad Rzeszutek Wilk <[email protected]>
> Signed-off-by: Wen Congyang <[email protected]>
> ---
Reviewed-by: Yasuaki Ishimatsu <[email protected]>
Thanks,
Yasuaki Ishimatsu
> drivers/acpi/acpi_memhotplug.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
> index c5e7b6d..e52ad5d 100644
> --- a/drivers/acpi/acpi_memhotplug.c
> +++ b/drivers/acpi/acpi_memhotplug.c
> @@ -421,9 +421,11 @@ static int acpi_memory_device_add(struct acpi_device *device)
> if (!acpi_memory_check_device(mem_device)) {
> /* call add_memory func */
> result = acpi_memory_enable_device(mem_device);
> - if (result)
> + if (result) {
> printk(KERN_ERR PREFIX
> "Error in acpi_memory_enable_device\n");
> + acpi_memory_device_free(mem_device);
> + }
> }
> return result;
> }
>
2012/11/15 15:59, Wen Congyang wrote:
> We eject the memory device even if it is in use. It is very dangerous,
> and it will cause the kernel to be panicked.
>
> CC: David Rientjes <[email protected]>
> CC: Jiang Liu <[email protected]>
> CC: Len Brown <[email protected]>
> CC: Benjamin Herrenschmidt <[email protected]>
> CC: Paul Mackerras <[email protected]>
> CC: Christoph Lameter <[email protected]>
> Cc: Minchan Kim <[email protected]>
> CC: Andrew Morton <[email protected]>
> CC: KOSAKI Motohiro <[email protected]>
> CC: Yasuaki Ishimatsu <[email protected]>
> CC: Rafael J. Wysocki <[email protected]>
> CC: Konrad Rzeszutek Wilk <[email protected]>
> Signed-off-by: Wen Congyang <[email protected]>
> ---
Reviewed-by: Yasuaki Ishimatsu <[email protected]>
Thanks,
Yasuaki Ishimatsu
> drivers/acpi/acpi_memhotplug.c | 42 +++++++++++++++++++++++++++++++++---------
> 1 file changed, 33 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
> index e52ad5d..f7e3007 100644
> --- a/drivers/acpi/acpi_memhotplug.c
> +++ b/drivers/acpi/acpi_memhotplug.c
> @@ -78,6 +78,7 @@ struct acpi_memory_info {
> unsigned short caching; /* memory cache attribute */
> unsigned short write_protect; /* memory read/write attribute */
> unsigned int enabled:1;
> + unsigned int failed:1;
> };
>
> struct acpi_memory_device {
> @@ -257,9 +258,23 @@ static int acpi_memory_enable_device(struct acpi_memory_device *mem_device)
> node = memory_add_physaddr_to_nid(info->start_addr);
>
> result = add_memory(node, info->start_addr, info->length);
> - if (result)
> +
> + /*
> + * If the memory block has been used by the kernel, add_memory()
> + * returns -EEXIST. If add_memory() returns the other error, it
> + * means that this memory block is not used by the kernel.
> + */
> + if (result && result != -EEXIST) {
> + info->failed = 1;
> continue;
> - info->enabled = 1;
> + }
> +
> + if (!result)
> + info->enabled = 1;
> + /*
> + * Add num_enable even if add_memory() returns -EEXIST, so the
> + * device is bound to this driver.
> + */
> num_enabled++;
> }
> if (!num_enabled) {
> @@ -280,21 +295,30 @@ static int acpi_memory_enable_device(struct acpi_memory_device *mem_device)
>
> static int acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
> {
> - int result;
> + int result = 0;
> struct acpi_memory_info *info, *n;
>
> list_for_each_entry_safe(info, n, &mem_device->res_list, list) {
> - if (info->enabled) {
> - result = remove_memory(info->start_addr, info->length);
> - if (result)
> - return result;
> - }
> + if (info->failed)
> + /* The kernel does not use this memory block */
> + continue;
> +
> + if (!info->enabled)
> + /*
> + * The kernel uses this memory block, but it may be not
> + * managed by us.
> + */
> + return -EBUSY;
> +
> + result = remove_memory(info->start_addr, info->length);
> + if (result)
> + return result;
>
> list_del(&info->list);
> kfree(info);
> }
>
> - return 0;
> + return result;
> }
>
> static void acpi_memory_device_notify(acpi_handle handle, u32 event, void *data)
>
2012/11/15 15:59, Wen Congyang wrote:
> We had introduced acpi_hotmem_initialized to avoid strange add_memory fail
> message. But the memory device may not be used by the kernel, and the
> device should be bound when the driver is being loaded. Remove
> acpi_hotmem_initialized to allow that the device can be bound when the
> driver is being loaded.
>
> CC: David Rientjes <[email protected]>
> CC: Jiang Liu <[email protected]>
> CC: Len Brown <[email protected]>
> CC: Benjamin Herrenschmidt <[email protected]>
> CC: Paul Mackerras <[email protected]>
> CC: Christoph Lameter <[email protected]>
> Cc: Minchan Kim <[email protected]>
> CC: Andrew Morton <[email protected]>
> CC: KOSAKI Motohiro <[email protected]>
> CC: Yasuaki Ishimatsu <[email protected]>
> CC: Rafael J. Wysocki <[email protected]>
> CC: Konrad Rzeszutek Wilk <[email protected]>
> Signed-off-by: Wen Congyang <[email protected]>
> ---
Reviewed-by: Yasuaki Ishimatsu <[email protected]>
Thanks,
Yasuaki Ishimatsu
> drivers/acpi/acpi_memhotplug.c | 12 ------------
> 1 file changed, 12 deletions(-)
>
> diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
> index f7e3007..e0f7425 100644
> --- a/drivers/acpi/acpi_memhotplug.c
> +++ b/drivers/acpi/acpi_memhotplug.c
> @@ -87,8 +87,6 @@ struct acpi_memory_device {
> struct list_head res_list;
> };
>
> -static int acpi_hotmem_initialized;
> -
> static acpi_status
> acpi_memory_get_resource(struct acpi_resource *resource, void *context)
> {
> @@ -433,15 +431,6 @@ static int acpi_memory_device_add(struct acpi_device *device)
>
> printk(KERN_DEBUG "%s \n", acpi_device_name(device));
>
> - /*
> - * Early boot code has recognized memory area by EFI/E820.
> - * If DSDT shows these memory devices on boot, hotplug is not necessary
> - * for them. So, it just returns until completion of this driver's
> - * start up.
> - */
> - if (!acpi_hotmem_initialized)
> - return 0;
> -
> if (!acpi_memory_check_device(mem_device)) {
> /* call add_memory func */
> result = acpi_memory_enable_device(mem_device);
> @@ -557,7 +546,6 @@ static int __init acpi_memory_device_init(void)
> return -ENODEV;
> }
>
> - acpi_hotmem_initialized = 1;
> return 0;
> }
>
>
2012/11/15 15:59, Wen Congyang wrote:
> If the memory device is hotplugged before the driver is loaded, the user
> cannot see this device under the directory /sys/bus/acpi/devices/, and the
> user cannot bind it by hand after the driver is loaded. This patch
> introduces a new feature to bind such device when the driver is being
> loaded.
>
> CC: David Rientjes <[email protected]>
> CC: Jiang Liu <[email protected]>
> CC: Len Brown <[email protected]>
> CC: Benjamin Herrenschmidt <[email protected]>
> CC: Paul Mackerras <[email protected]>
> CC: Christoph Lameter <[email protected]>
> Cc: Minchan Kim <[email protected]>
> CC: Andrew Morton <[email protected]>
> CC: KOSAKI Motohiro <[email protected]>
> CC: Yasuaki Ishimatsu <[email protected]>
> CC: Rafael J. Wysocki <[email protected]>
> CC: Konrad Rzeszutek Wilk <[email protected]>
> Signed-off-by: Wen Congyang <[email protected]>
> ---
Reviewed-by: Yasuaki Ishimatsu <[email protected]>
Thanks,
Yasuaki Ishimatsu
> drivers/acpi/acpi_memhotplug.c | 37 ++++++++++++++++++++++++++++++++++++-
> 1 file changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
> index e0f7425..9f1d107 100644
> --- a/drivers/acpi/acpi_memhotplug.c
> +++ b/drivers/acpi/acpi_memhotplug.c
> @@ -52,6 +52,9 @@ MODULE_LICENSE("GPL");
> #define MEMORY_POWER_ON_STATE 1
> #define MEMORY_POWER_OFF_STATE 2
>
> +static bool auto_probe;
> +module_param(auto_probe, bool, S_IRUGO | S_IWUSR);
> +
> static int acpi_memory_device_add(struct acpi_device *device);
> static int acpi_memory_device_remove(struct acpi_device *device, int type);
>
> @@ -494,12 +497,44 @@ acpi_memory_register_notify_handler(acpi_handle handle,
> u32 level, void *ctxt, void **retv)
> {
> acpi_status status;
> -
> + struct acpi_memory_device *mem_device = NULL;
> + unsigned long long current_status;
>
> status = is_memory_device(handle);
> if (ACPI_FAILURE(status))
> return AE_OK; /* continue */
>
> + if (auto_probe) {
> + /* Get device present/absent information from the _STA */
> + status = acpi_evaluate_integer(handle, "_STA", NULL,
> + ¤t_status);
> + if (ACPI_FAILURE(status))
> + goto install;
> +
> + /*
> + * Check for device status. Device should be
> + * present/enabled/functioning.
> + */
> + if (!(current_status &
> + (ACPI_STA_DEVICE_PRESENT | ACPI_STA_DEVICE_ENABLED |
> + ACPI_STA_DEVICE_FUNCTIONING)))
> + goto install;
> +
> + if (acpi_memory_get_device(handle, &mem_device))
> + goto install;
> +
> + /* We have bound this device while we register the driver */
> + if (mem_device->state == MEMORY_POWER_ON_STATE)
> + goto install;
> +
> + ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> + "\nauto probe memory device\n"));
> +
> + if (acpi_memory_enable_device(mem_device))
> + pr_err(PREFIX "Cannot enable memory device\n");
> + }
> +
> +install:
> status = acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
> acpi_memory_device_notify, NULL);
> /* continue */
>
On Thu, 2012-11-15 at 14:59 +0800, Wen Congyang wrote:
> The memory device can be removed by 2 ways:
> 1. send eject request by SCI
> 2. echo 1 >/sys/bus/pci/devices/PNP0C80:XX/eject
>
> We handle the 1st case in the module acpi_memhotplug, and handle
> the 2nd case in ACPI eject notification. This 2 events may happen
> at the same time, so we may touch acpi_memory_device.res_list at
> the same time. This patch reimplements memory-hotremove support
> through an ACPI eject notification. Now the memory device is
> offlined and hotremoved only in the function acpi_memory_device_remove()
> which is protected by device_lock().
>
> CC: David Rientjes <[email protected]>
> CC: Jiang Liu <[email protected]>
> CC: Len Brown <[email protected]>
> CC: Benjamin Herrenschmidt <[email protected]>
> CC: Paul Mackerras <[email protected]>
> CC: Christoph Lameter <[email protected]>
> Cc: Minchan Kim <[email protected]>
> CC: Andrew Morton <[email protected]>
> CC: KOSAKI Motohiro <[email protected]>
> CC: Yasuaki Ishimatsu <[email protected]>
> CC: Rafael J. Wysocki <[email protected]>
> CC: Konrad Rzeszutek Wilk <[email protected]>
> Signed-off-by: Wen Congyang <[email protected]>
Thanks for the update. It looks good.
Reviewed-by: Toshi Kani <[email protected]>
-Toshi
On Thu, 15 Nov 2012, Wen Congyang wrote:
> Note:
> 1. The following commit in pm tree can be dropped now(The other two patches
> are already dropped):
> 54c4c7db6cb94d7d1217df6d7fca6847c61744ab
> 2. This patchset requires the following patch(It is in pm tree now)
> https://lkml.org/lkml/2012/11/1/225
>
So this is based on the acpi-general branch of
git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git correct?
And the branch's HEAD commit 54c4c7db6cb9 ("ACPI / memory-hotplug: call
acpi_bus_trim() to remove memory device") can be reverted before this
series is applied?
On Thu, 2012-11-15 at 14:59 +0800, Wen Congyang wrote:
> If the memory device is hotplugged before the driver is loaded, the user
> cannot see this device under the directory /sys/bus/acpi/devices/, and the
> user cannot bind it by hand after the driver is loaded. This patch
> introduces a new feature to bind such device when the driver is being
> loaded.
Hi Wen,
While I can see the problem, I am a little concerned about this
approach. If we go this way, all ACPI hotplug drivers need to do it on
their own way and walk the entire ACPI namespace when they are
installed. Such common issue should be dealt by ACPI core. I am
wondering if we can simply set MEMORY_HOTPLUG to default y for now.
Processor and Container are already set to y by default.
Thanks,
-Toshi
> CC: David Rientjes <[email protected]>
> CC: Jiang Liu <[email protected]>
> CC: Len Brown <[email protected]>
> CC: Benjamin Herrenschmidt <[email protected]>
> CC: Paul Mackerras <[email protected]>
> CC: Christoph Lameter <[email protected]>
> Cc: Minchan Kim <[email protected]>
> CC: Andrew Morton <[email protected]>
> CC: KOSAKI Motohiro <[email protected]>
> CC: Yasuaki Ishimatsu <[email protected]>
> CC: Rafael J. Wysocki <[email protected]>
> CC: Konrad Rzeszutek Wilk <[email protected]>
> Signed-off-by: Wen Congyang <[email protected]>
> ---
> drivers/acpi/acpi_memhotplug.c | 37 ++++++++++++++++++++++++++++++++++++-
> 1 file changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
> index e0f7425..9f1d107 100644
> --- a/drivers/acpi/acpi_memhotplug.c
> +++ b/drivers/acpi/acpi_memhotplug.c
> @@ -52,6 +52,9 @@ MODULE_LICENSE("GPL");
> #define MEMORY_POWER_ON_STATE 1
> #define MEMORY_POWER_OFF_STATE 2
>
> +static bool auto_probe;
> +module_param(auto_probe, bool, S_IRUGO | S_IWUSR);
> +
> static int acpi_memory_device_add(struct acpi_device *device);
> static int acpi_memory_device_remove(struct acpi_device *device, int type);
>
> @@ -494,12 +497,44 @@ acpi_memory_register_notify_handler(acpi_handle handle,
> u32 level, void *ctxt, void **retv)
> {
> acpi_status status;
> -
> + struct acpi_memory_device *mem_device = NULL;
> + unsigned long long current_status;
>
> status = is_memory_device(handle);
> if (ACPI_FAILURE(status))
> return AE_OK; /* continue */
>
> + if (auto_probe) {
> + /* Get device present/absent information from the _STA */
> + status = acpi_evaluate_integer(handle, "_STA", NULL,
> + ¤t_status);
> + if (ACPI_FAILURE(status))
> + goto install;
> +
> + /*
> + * Check for device status. Device should be
> + * present/enabled/functioning.
> + */
> + if (!(current_status &
> + (ACPI_STA_DEVICE_PRESENT | ACPI_STA_DEVICE_ENABLED |
> + ACPI_STA_DEVICE_FUNCTIONING)))
> + goto install;
> +
> + if (acpi_memory_get_device(handle, &mem_device))
> + goto install;
> +
> + /* We have bound this device while we register the driver */
> + if (mem_device->state == MEMORY_POWER_ON_STATE)
> + goto install;
> +
> + ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> + "\nauto probe memory device\n"));
> +
> + if (acpi_memory_enable_device(mem_device))
> + pr_err(PREFIX "Cannot enable memory device\n");
> + }
> +
> +install:
> status = acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
> acpi_memory_device_notify, NULL);
> /* continue */
On Thu, 15 Nov 2012, Wen Congyang wrote:
> From: Yasuaki Ishimatsu <[email protected]>
>
> The memory device can be removed by 2 ways:
> 1. send eject request by SCI
> 2. echo 1 >/sys/bus/pci/devices/PNP0C80:XX/eject
>
> In the 1st case, acpi_memory_disable_device() will be called.
> In the 2nd case, acpi_memory_device_remove() will be called.
> acpi_memory_device_remove() will also be called when we unbind the
> memory device from the driver acpi_memhotplug or a driver initialization
> fails.
>
> acpi_memory_disable_device() has already implemented a code which
> offlines memory and releases acpi_memory_info struct. But
> acpi_memory_device_remove() has not implemented it yet.
>
> So the patch move offlining memory and releasing acpi_memory_info struct
> codes to a new function acpi_memory_remove_memory(). And it is used by both
> acpi_memory_device_remove() and acpi_memory_disable_device().
>
> CC: David Rientjes <[email protected]>
> CC: Jiang Liu <[email protected]>
> CC: Len Brown <[email protected]>
> CC: Benjamin Herrenschmidt <[email protected]>
> CC: Paul Mackerras <[email protected]>
> CC: Christoph Lameter <[email protected]>
> Cc: Minchan Kim <[email protected]>
> CC: Andrew Morton <[email protected]>
> CC: KOSAKI Motohiro <[email protected]>
> CC: Rafael J. Wysocki <[email protected]>
> CC: Konrad Rzeszutek Wilk <[email protected]>
> Signed-off-by: Yasuaki Ishimatsu <[email protected]>
> Signed-off-by: Wen Congyang <[email protected]>
Acked-by: David Rientjes <[email protected]>
On Thursday, November 15, 2012 02:51:52 PM David Rientjes wrote:
> On Thu, 15 Nov 2012, Wen Congyang wrote:
>
> > Note:
> > 1. The following commit in pm tree can be dropped now(The other two patches
> > are already dropped):
> > 54c4c7db6cb94d7d1217df6d7fca6847c61744ab
> > 2. This patchset requires the following patch(It is in pm tree now)
> > https://lkml.org/lkml/2012/11/1/225
> >
>
> So this is based on the acpi-general branch of
> git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git correct?
It should be based on that, yes.
> And the branch's HEAD commit 54c4c7db6cb9 ("ACPI / memory-hotplug: call
> acpi_bus_trim() to remove memory device") can be reverted before this
> series is applied?
Why?
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Friday, November 16, 2012 12:22:58 AM Rafael J. Wysocki wrote:
> On Thursday, November 15, 2012 02:51:52 PM David Rientjes wrote:
> > On Thu, 15 Nov 2012, Wen Congyang wrote:
> >
> > > Note:
> > > 1. The following commit in pm tree can be dropped now(The other two patches
> > > are already dropped):
> > > 54c4c7db6cb94d7d1217df6d7fca6847c61744ab
> > > 2. This patchset requires the following patch(It is in pm tree now)
> > > https://lkml.org/lkml/2012/11/1/225
> > >
> >
> > So this is based on the acpi-general branch of
> > git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git correct?
>
> It should be based on that, yes.
>
> > And the branch's HEAD commit 54c4c7db6cb9 ("ACPI / memory-hotplug: call
> > acpi_bus_trim() to remove memory device") can be reverted before this
> > series is applied?
>
> Why?
Ah, because of patch [2/7]. I wonder what tree this one is supposed to
apply to. Surely not to linux-pm.git/acpi-general.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Thu, 15 Nov 2012, Wen Congyang wrote:
> The memory device can be removed by 2 ways:
> 1. send eject request by SCI
> 2. echo 1 >/sys/bus/pci/devices/PNP0C80:XX/eject
>
> We handle the 1st case in the module acpi_memhotplug, and handle
> the 2nd case in ACPI eject notification. This 2 events may happen
> at the same time, so we may touch acpi_memory_device.res_list at
> the same time. This patch reimplements memory-hotremove support
> through an ACPI eject notification. Now the memory device is
> offlined and hotremoved only in the function acpi_memory_device_remove()
> which is protected by device_lock().
>
You mean it's protected by device_lock() before calling the remove()
function for eject?
> CC: David Rientjes <[email protected]>
> CC: Jiang Liu <[email protected]>
> CC: Len Brown <[email protected]>
> CC: Benjamin Herrenschmidt <[email protected]>
> CC: Paul Mackerras <[email protected]>
> CC: Christoph Lameter <[email protected]>
> Cc: Minchan Kim <[email protected]>
> CC: Andrew Morton <[email protected]>
> CC: KOSAKI Motohiro <[email protected]>
> CC: Yasuaki Ishimatsu <[email protected]>
> CC: Rafael J. Wysocki <[email protected]>
> CC: Konrad Rzeszutek Wilk <[email protected]>
> Signed-off-by: Wen Congyang <[email protected]>
> ---
> drivers/acpi/acpi_memhotplug.c | 87 +++++-------------------------------------
> 1 file changed, 9 insertions(+), 78 deletions(-)
>
> diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
> index 2918be1..6e12042 100644
> --- a/drivers/acpi/acpi_memhotplug.c
> +++ b/drivers/acpi/acpi_memhotplug.c
> @@ -272,40 +272,6 @@ static int acpi_memory_enable_device(struct acpi_memory_device *mem_device)
> return 0;
> }
>
> -static int acpi_memory_powerdown_device(struct acpi_memory_device *mem_device)
> -{
> - acpi_status status;
> - struct acpi_object_list arg_list;
> - union acpi_object arg;
> - unsigned long long current_status;
> -
> -
> - /* Issue the _EJ0 command */
> - arg_list.count = 1;
> - arg_list.pointer = &arg;
> - arg.type = ACPI_TYPE_INTEGER;
> - arg.integer.value = 1;
> - status = acpi_evaluate_object(mem_device->device->handle,
> - "_EJ0", &arg_list, NULL);
> - /* Return on _EJ0 failure */
> - if (ACPI_FAILURE(status)) {
> - ACPI_EXCEPTION((AE_INFO, status, "_EJ0 failed"));
> - return -ENODEV;
> - }
> -
> - /* Evalute _STA to check if the device is disabled */
> - status = acpi_evaluate_integer(mem_device->device->handle, "_STA",
> - NULL, ¤t_status);
> - if (ACPI_FAILURE(status))
> - return -ENODEV;
> -
> - /* Check for device status. Device should be disabled */
> - if (current_status & ACPI_STA_DEVICE_ENABLED)
> - return -EINVAL;
> -
> - return 0;
> -}
> -
> static int acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
> {
> int result;
> @@ -325,34 +291,11 @@ static int acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
> return 0;
> }
>
> -static int acpi_memory_disable_device(struct acpi_memory_device *mem_device)
> -{
> - int result;
> -
> - /*
> - * Ask the VM to offline this memory range.
> - * Note: Assume that this function returns zero on success
> - */
> - result = acpi_memory_remove_memory(mem_device);
> - if (result)
> - return result;
> -
> - /* Power-off and eject the device */
> - result = acpi_memory_powerdown_device(mem_device);
> - if (result) {
> - /* Set the status of the device to invalid */
> - mem_device->state = MEMORY_INVALID_STATE;
> - return result;
> - }
> -
> - mem_device->state = MEMORY_POWER_OFF_STATE;
> - return result;
> -}
> -
> static void acpi_memory_device_notify(acpi_handle handle, u32 event, void *data)
> {
> struct acpi_memory_device *mem_device;
> struct acpi_device *device;
> + struct acpi_eject_event *ej_event = NULL;
> u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE; /* default */
>
> switch (event) {
> @@ -394,31 +337,19 @@ static void acpi_memory_device_notify(acpi_handle handle, u32 event, void *data)
> break;
> }
>
> - /*
> - * Currently disabling memory device from kernel mode
> - * TBD: Can also be disabled from user mode scripts
> - * TBD: Can also be disabled by Callback registration
> - * with generic sysfs driver
> - */
> - if (acpi_memory_disable_device(mem_device)) {
> - printk(KERN_ERR PREFIX "Disable memory device\n");
> - /*
> - * If _EJ0 was called but failed, _OST is not
> - * necessary.
> - */
> - if (mem_device->state == MEMORY_INVALID_STATE)
> - return;
> -
> + ej_event = kmalloc(sizeof(*ej_event), GFP_KERNEL);
> + if (!ej_event) {
> + pr_err(PREFIX "No memory, dropping EJECT\n");
> break;
> }
>
> - /*
> - * TBD: Invoke acpi_bus_remove to cleanup data structures
> - */
> + ej_event->handle = handle;
> + ej_event->event = ACPI_NOTIFY_EJECT_REQUEST;
> + acpi_os_hotplug_execute(acpi_bus_hot_remove_device,
> + (void *)ej_event);
>
> - /* _EJ0 succeeded; _OST is not necessary */
> + /* eject is performed asynchronously */
> return;
> -
> default:
> ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> "Unsupported event [0x%x]\n", event));
On Thu, 15 Nov 2012, Wen Congyang wrote:
> We allocate memory to store acpi_memory_info, so we should free it before
> freeing mem_device.
>
> CC: David Rientjes <[email protected]>
> CC: Jiang Liu <[email protected]>
> CC: Len Brown <[email protected]>
> CC: Benjamin Herrenschmidt <[email protected]>
> CC: Paul Mackerras <[email protected]>
> CC: Christoph Lameter <[email protected]>
> Cc: Minchan Kim <[email protected]>
> CC: Andrew Morton <[email protected]>
> CC: KOSAKI Motohiro <[email protected]>
> CC: Yasuaki Ishimatsu <[email protected]>
> CC: Rafael J. Wysocki <[email protected]>
> CC: Konrad Rzeszutek Wilk <[email protected]>
> Signed-off-by: Wen Congyang <[email protected]>
Acked-by: David Rientjes <[email protected]>
It's a little strange to be re-initializing mem_device->res_list right
before freeing mem_device in acpi_memory_device_free(), though. Owell :)
On Thu, 15 Nov 2012, Wen Congyang wrote:
> If acpi_memory_enable_device() fails, acpi_memory_enable_device() will
> return a non-zero value, which means we fail to bind the memory device to
> this driver. So we should free memory device before
> acpi_memory_device_add() returns.
>
> CC: David Rientjes <[email protected]>
> CC: Jiang Liu <[email protected]>
> CC: Len Brown <[email protected]>
> CC: Benjamin Herrenschmidt <[email protected]>
> CC: Paul Mackerras <[email protected]>
> CC: Christoph Lameter <[email protected]>
> Cc: Minchan Kim <[email protected]>
> CC: Andrew Morton <[email protected]>
> CC: KOSAKI Motohiro <[email protected]>
> CC: Yasuaki Ishimatsu <[email protected]>
> CC: Rafael J. Wysocki <[email protected]>
> CC: Konrad Rzeszutek Wilk <[email protected]>
> Signed-off-by: Wen Congyang <[email protected]>
Acked-by: David Rientjes <[email protected]>
On Thu, 15 Nov 2012, Wen Congyang wrote:
> We eject the memory device even if it is in use. It is very dangerous,
> and it will cause the kernel to be panicked.
>
> CC: David Rientjes <[email protected]>
> CC: Jiang Liu <[email protected]>
> CC: Len Brown <[email protected]>
> CC: Benjamin Herrenschmidt <[email protected]>
> CC: Paul Mackerras <[email protected]>
> CC: Christoph Lameter <[email protected]>
> Cc: Minchan Kim <[email protected]>
> CC: Andrew Morton <[email protected]>
> CC: KOSAKI Motohiro <[email protected]>
> CC: Yasuaki Ishimatsu <[email protected]>
> CC: Rafael J. Wysocki <[email protected]>
> CC: Konrad Rzeszutek Wilk <[email protected]>
> Signed-off-by: Wen Congyang <[email protected]>
Acked-by: David Rientjes <[email protected]>
Thanks for adding the comment about why num_enabled is incremented for
-EEXIST.
On Thu, 15 Nov 2012, Wen Congyang wrote:
> We had introduced acpi_hotmem_initialized to avoid strange add_memory fail
> message. But the memory device may not be used by the kernel, and the
> device should be bound when the driver is being loaded. Remove
> acpi_hotmem_initialized to allow that the device can be bound when the
> driver is being loaded.
>
> CC: David Rientjes <[email protected]>
> CC: Jiang Liu <[email protected]>
> CC: Len Brown <[email protected]>
> CC: Benjamin Herrenschmidt <[email protected]>
> CC: Paul Mackerras <[email protected]>
> CC: Christoph Lameter <[email protected]>
> Cc: Minchan Kim <[email protected]>
> CC: Andrew Morton <[email protected]>
> CC: KOSAKI Motohiro <[email protected]>
> CC: Yasuaki Ishimatsu <[email protected]>
> CC: Rafael J. Wysocki <[email protected]>
> CC: Konrad Rzeszutek Wilk <[email protected]>
> Signed-off-by: Wen Congyang <[email protected]>
Acked-by: David Rientjes <[email protected]>
On Thursday, November 15, 2012 02:59:30 PM Wen Congyang wrote:
> The memory device can be removed by 2 ways:
> 1. send eject request by SCI
> 2. echo 1 >/sys/bus/pci/devices/PNP0C80:XX/eject
>
> In the 1st case, acpi_memory_disable_device() will be called.
> In the 2nd case, acpi_memory_device_remove() will be called.
> acpi_memory_device_remove() will also be called when we unbind the
> memory device from the driver acpi_memhotplug or a driver initialization
> fails.
>
> acpi_memory_disable_device() has already implemented a code which
> offlines memory and releases acpi_memory_info struct . But
> acpi_memory_device_remove() has not implemented it yet.
>
> So the patch prepares the framework for hot removing memory and
> adds the framework into acpi_memory_device_remove().
>
> We may hotremove the memory device by this 2 ways at the same time.
> So we remove the function acpi_memory_disable_device(), and use
> acpi_bus_hot_remove_device() which is used by 2nd case to implement it.
> We lock device in acpi_bus_hot_remove_device(), so there is no
> need to add lock in acpi_memhotplug.
>
> The last version of this patchset is here:
> https://lkml.org/lkml/2012/11/8/121
>
> Note:
> 1. The following commit in pm tree can be dropped now(The other two patches
> are already dropped):
> 54c4c7db6cb94d7d1217df6d7fca6847c61744ab
> 2. This patchset requires the following patch(It is in pm tree now)
> https://lkml.org/lkml/2012/11/1/225
>
> Changes from v4 to v5:
> 1. patch2: new patch. use acpi_bus_hot_remove_device() to implement memory
> device hotremove.
>
> Changes from v3 to v4:
> 1. patch1: unlock list_lock when removing memory fails.
> 2. patch2: just rebase them
> 3. patch3-7: these patches are in -mm tree, and they conflict with this
> patchset, so Adrew Morton drop them from -mm tree. I rebase and merge
> them into this patchset.
>
> Wen Congyang (6):
> acpi,memory-hotplug: deal with eject request in hotplug queue
> acpi_memhotplug.c: fix memory leak when memory device is unbound from
> the module acpi_memhotplug
> acpi_memhotplug.c: free memory device if acpi_memory_enable_device()
> failed
> acpi_memhotplug.c: don't allow to eject the memory device if it is
> being used
> acpi_memhotplug.c: bind the memory device when the driver is being
> loaded
> acpi_memhotplug.c: auto bind the memory device which is hotplugged
> before the driver is loaded
>
> Yasuaki Ishimatsu (1):
> acpi,memory-hotplug : add memory offline code to
> acpi_memory_device_remove()
Well, I have tried _really_ hard to apply this patchset, but pretty much
none of the patches except for [1/7] applied for me. I have no idea what
tree they are against, but I'm pretty sure it's not my tree.
I _have_ applied patches [1-4/7] and pushed them to linux-pm.git/linux-next.
I needed to fix up almost all of them so that they applied, so please check
if my fixups make sense (and let me know ASAP if that's not the case).
If they are OK, please rebase the rest of the series on top of
linux-pm.git/linux-next and repost. I'm not going to take any more
patches that don't apply from you.
Moreover, I'm not going to take any more ACPI memory hotplug patches
for v3.8 except for the [5-7/7] from this series (after they have been
rebased and _if_ they apply), so please don't submit any until the v3.8
merge window closes (of course, you're free to post RFCs, but I will
ignore them).
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Friday, November 16, 2012 01:28:43 AM Rafael J. Wysocki wrote:
> On Thursday, November 15, 2012 02:59:30 PM Wen Congyang wrote:
> > The memory device can be removed by 2 ways:
> > 1. send eject request by SCI
> > 2. echo 1 >/sys/bus/pci/devices/PNP0C80:XX/eject
> >
> > In the 1st case, acpi_memory_disable_device() will be called.
> > In the 2nd case, acpi_memory_device_remove() will be called.
> > acpi_memory_device_remove() will also be called when we unbind the
> > memory device from the driver acpi_memhotplug or a driver initialization
> > fails.
> >
> > acpi_memory_disable_device() has already implemented a code which
> > offlines memory and releases acpi_memory_info struct . But
> > acpi_memory_device_remove() has not implemented it yet.
> >
> > So the patch prepares the framework for hot removing memory and
> > adds the framework into acpi_memory_device_remove().
> >
> > We may hotremove the memory device by this 2 ways at the same time.
> > So we remove the function acpi_memory_disable_device(), and use
> > acpi_bus_hot_remove_device() which is used by 2nd case to implement it.
> > We lock device in acpi_bus_hot_remove_device(), so there is no
> > need to add lock in acpi_memhotplug.
> >
> > The last version of this patchset is here:
> > https://lkml.org/lkml/2012/11/8/121
> >
> > Note:
> > 1. The following commit in pm tree can be dropped now(The other two patches
> > are already dropped):
> > 54c4c7db6cb94d7d1217df6d7fca6847c61744ab
> > 2. This patchset requires the following patch(It is in pm tree now)
> > https://lkml.org/lkml/2012/11/1/225
> >
> > Changes from v4 to v5:
> > 1. patch2: new patch. use acpi_bus_hot_remove_device() to implement memory
> > device hotremove.
> >
> > Changes from v3 to v4:
> > 1. patch1: unlock list_lock when removing memory fails.
> > 2. patch2: just rebase them
> > 3. patch3-7: these patches are in -mm tree, and they conflict with this
> > patchset, so Adrew Morton drop them from -mm tree. I rebase and merge
> > them into this patchset.
> >
> > Wen Congyang (6):
> > acpi,memory-hotplug: deal with eject request in hotplug queue
> > acpi_memhotplug.c: fix memory leak when memory device is unbound from
> > the module acpi_memhotplug
> > acpi_memhotplug.c: free memory device if acpi_memory_enable_device()
> > failed
> > acpi_memhotplug.c: don't allow to eject the memory device if it is
> > being used
> > acpi_memhotplug.c: bind the memory device when the driver is being
> > loaded
> > acpi_memhotplug.c: auto bind the memory device which is hotplugged
> > before the driver is loaded
> >
> > Yasuaki Ishimatsu (1):
> > acpi,memory-hotplug : add memory offline code to
> > acpi_memory_device_remove()
>
> Well, I have tried _really_ hard to apply this patchset, but pretty much
> none of the patches except for [1/7] applied for me. I have no idea what
> tree they are against, but I'm pretty sure it's not my tree.
>
> I _have_ applied patches [1-4/7] and pushed them to linux-pm.git/linux-next.
> I needed to fix up almost all of them so that they applied, so please check
> if my fixups make sense (and let me know ASAP if that's not the case).
>
> If they are OK, please rebase the rest of the series on top of
> linux-pm.git/linux-next and repost. I'm not going to take any more
> patches that don't apply from you.
>
> Moreover, I'm not going to take any more ACPI memory hotplug patches
> for v3.8 except for the [5-7/7] from this series (after they have been
> rebased and _if_ they apply), so please don't submit any until the v3.8
> merge window closes (of course, you're free to post RFCs, but I will
> ignore them).
And by the way, if someone gives a "Reviewed-by" to a patch that _obviously_
doesn't apply, I will ignore any "Reviewed-by" from that person going forward,
because that quite obviously means you haven't even compared the patch with the
existing code and thus your "review" is worthless.
If you just want to say you agree with the patch, use "Acked-by".
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
> > Well, I have tried _really_ hard to apply this patchset, but pretty much
> > none of the patches except for [1/7] applied for me. I have no idea what
> > tree they are against, but I'm pretty sure it's not my tree.
> >
> > I _have_ applied patches [1-4/7] and pushed them to linux-pm.git/linux-next.
> > I needed to fix up almost all of them so that they applied, so please check
> > if my fixups make sense (and let me know ASAP if that's not the case).
> >
> > If they are OK, please rebase the rest of the series on top of
> > linux-pm.git/linux-next and repost. I'm not going to take any more
> > patches that don't apply from you.
> >
> > Moreover, I'm not going to take any more ACPI memory hotplug patches
> > for v3.8 except for the [5-7/7] from this series (after they have been
> > rebased and _if_ they apply), so please don't submit any until the v3.8
> > merge window closes (of course, you're free to post RFCs, but I will
> > ignore them).
>
> And by the way, if someone gives a "Reviewed-by" to a patch that _obviously_
> doesn't apply, I will ignore any "Reviewed-by" from that person going forward,
> because that quite obviously means you haven't even compared the patch with the
> existing code and thus your "review" is worthless.
I was able to apply all his patches on top of the Linus's tree...
> If you just want to say you agree with the patch, use "Acked-by".
Got it.
Thanks,
-Toshi
> Thanks,
> Rafael
>
>
On Thursday, November 15, 2012 05:40:59 PM Toshi Kani wrote:
> > > Well, I have tried _really_ hard to apply this patchset, but pretty much
> > > none of the patches except for [1/7] applied for me. I have no idea what
> > > tree they are against, but I'm pretty sure it's not my tree.
> > >
> > > I _have_ applied patches [1-4/7] and pushed them to linux-pm.git/linux-next.
> > > I needed to fix up almost all of them so that they applied, so please check
> > > if my fixups make sense (and let me know ASAP if that's not the case).
> > >
> > > If they are OK, please rebase the rest of the series on top of
> > > linux-pm.git/linux-next and repost. I'm not going to take any more
> > > patches that don't apply from you.
> > >
> > > Moreover, I'm not going to take any more ACPI memory hotplug patches
> > > for v3.8 except for the [5-7/7] from this series (after they have been
> > > rebased and _if_ they apply), so please don't submit any until the v3.8
> > > merge window closes (of course, you're free to post RFCs, but I will
> > > ignore them).
> >
> > And by the way, if someone gives a "Reviewed-by" to a patch that _obviously_
> > doesn't apply, I will ignore any "Reviewed-by" from that person going forward,
> > because that quite obviously means you haven't even compared the patch with the
> > existing code and thus your "review" is worthless.
>
> I was able to apply all his patches on top of the Linus's tree...
Ah, that may be because of my mistake. Sorry about that.
I'll try again and see if that works (sigh).
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
Hi Rafael,
2012/11/16 9:28, Rafael J. Wysocki wrote:
> On Thursday, November 15, 2012 02:59:30 PM Wen Congyang wrote:
>> The memory device can be removed by 2 ways:
>> 1. send eject request by SCI
>> 2. echo 1 >/sys/bus/pci/devices/PNP0C80:XX/eject
>>
>> In the 1st case, acpi_memory_disable_device() will be called.
>> In the 2nd case, acpi_memory_device_remove() will be called.
>> acpi_memory_device_remove() will also be called when we unbind the
>> memory device from the driver acpi_memhotplug or a driver initialization
>> fails.
>>
>> acpi_memory_disable_device() has already implemented a code which
>> offlines memory and releases acpi_memory_info struct . But
>> acpi_memory_device_remove() has not implemented it yet.
>>
>> So the patch prepares the framework for hot removing memory and
>> adds the framework into acpi_memory_device_remove().
>>
>> We may hotremove the memory device by this 2 ways at the same time.
>> So we remove the function acpi_memory_disable_device(), and use
>> acpi_bus_hot_remove_device() which is used by 2nd case to implement it.
>> We lock device in acpi_bus_hot_remove_device(), so there is no
>> need to add lock in acpi_memhotplug.
>>
>> The last version of this patchset is here:
>> https://lkml.org/lkml/2012/11/8/121
>>
>> Note:
>> 1. The following commit in pm tree can be dropped now(The other two patches
>> are already dropped):
>> 54c4c7db6cb94d7d1217df6d7fca6847c61744ab
>> 2. This patchset requires the following patch(It is in pm tree now)
>> https://lkml.org/lkml/2012/11/1/225
>>
>> Changes from v4 to v5:
>> 1. patch2: new patch. use acpi_bus_hot_remove_device() to implement memory
>> device hotremove.
>>
>> Changes from v3 to v4:
>> 1. patch1: unlock list_lock when removing memory fails.
>> 2. patch2: just rebase them
>> 3. patch3-7: these patches are in -mm tree, and they conflict with this
>> patchset, so Adrew Morton drop them from -mm tree. I rebase and merge
>> them into this patchset.
>>
>> Wen Congyang (6):
>> acpi,memory-hotplug: deal with eject request in hotplug queue
>> acpi_memhotplug.c: fix memory leak when memory device is unbound from
>> the module acpi_memhotplug
>> acpi_memhotplug.c: free memory device if acpi_memory_enable_device()
>> failed
>> acpi_memhotplug.c: don't allow to eject the memory device if it is
>> being used
>> acpi_memhotplug.c: bind the memory device when the driver is being
>> loaded
>> acpi_memhotplug.c: auto bind the memory device which is hotplugged
>> before the driver is loaded
>>
>> Yasuaki Ishimatsu (1):
>> acpi,memory-hotplug : add memory offline code to
>> acpi_memory_device_remove()
>
> Well, I have tried _really_ hard to apply this patchset, but pretty much
> none of the patches except for [1/7] applied for me. I have no idea what
> tree they are against, but I'm pretty sure it's not my tree.
>
> I _have_ applied patches [1-4/7] and pushed them to linux-pm.git/linux-next.
I checked your tree and found a mistake.
You merged a following patch into your tree.
commitid:2ba281f1
ACPI / memory-hotplug: introduce a mutex lock to protect the list
in acpi_memory_device
But it is wrong.
[1/7] patch is "acpi,memory-hotplug : add memory offline code to
acpi_memory_device_remove()". So we would like you to merge it
instead of commitid:2ba281f1.
Thanks,
Yasuaki Ishimatsu
> I needed to fix up almost all of them so that they applied, so please check
> if my fixups make sense (and let me know ASAP if that's not the case).
>
> If they are OK, please rebase the rest of the series on top of
> linux-pm.git/linux-next and repost. I'm not going to take any more
> patches that don't apply from you.
>
> Moreover, I'm not going to take any more ACPI memory hotplug patches
> for v3.8 except for the [5-7/7] from this series (after they have been
> rebased and _if_ they apply), so please don't submit any until the v3.8
> merge window closes (of course, you're free to post RFCs, but I will
> ignore them).
>
> Thanks,
> Rafael
>
>
On Friday, November 16, 2012 10:07:49 AM Yasuaki Ishimatsu wrote:
> Hi Rafael,
>
> 2012/11/16 9:28, Rafael J. Wysocki wrote:
> > On Thursday, November 15, 2012 02:59:30 PM Wen Congyang wrote:
> >> The memory device can be removed by 2 ways:
> >> 1. send eject request by SCI
> >> 2. echo 1 >/sys/bus/pci/devices/PNP0C80:XX/eject
> >>
> >> In the 1st case, acpi_memory_disable_device() will be called.
> >> In the 2nd case, acpi_memory_device_remove() will be called.
> >> acpi_memory_device_remove() will also be called when we unbind the
> >> memory device from the driver acpi_memhotplug or a driver initialization
> >> fails.
> >>
> >> acpi_memory_disable_device() has already implemented a code which
> >> offlines memory and releases acpi_memory_info struct . But
> >> acpi_memory_device_remove() has not implemented it yet.
> >>
> >> So the patch prepares the framework for hot removing memory and
> >> adds the framework into acpi_memory_device_remove().
> >>
> >> We may hotremove the memory device by this 2 ways at the same time.
> >> So we remove the function acpi_memory_disable_device(), and use
> >> acpi_bus_hot_remove_device() which is used by 2nd case to implement it.
> >> We lock device in acpi_bus_hot_remove_device(), so there is no
> >> need to add lock in acpi_memhotplug.
> >>
> >> The last version of this patchset is here:
> >> https://lkml.org/lkml/2012/11/8/121
> >>
> >> Note:
> >> 1. The following commit in pm tree can be dropped now(The other two patches
> >> are already dropped):
> >> 54c4c7db6cb94d7d1217df6d7fca6847c61744ab
> >> 2. This patchset requires the following patch(It is in pm tree now)
> >> https://lkml.org/lkml/2012/11/1/225
> >>
> >> Changes from v4 to v5:
> >> 1. patch2: new patch. use acpi_bus_hot_remove_device() to implement memory
> >> device hotremove.
> >>
> >> Changes from v3 to v4:
> >> 1. patch1: unlock list_lock when removing memory fails.
> >> 2. patch2: just rebase them
> >> 3. patch3-7: these patches are in -mm tree, and they conflict with this
> >> patchset, so Adrew Morton drop them from -mm tree. I rebase and merge
> >> them into this patchset.
> >>
> >> Wen Congyang (6):
> >> acpi,memory-hotplug: deal with eject request in hotplug queue
> >> acpi_memhotplug.c: fix memory leak when memory device is unbound from
> >> the module acpi_memhotplug
> >> acpi_memhotplug.c: free memory device if acpi_memory_enable_device()
> >> failed
> >> acpi_memhotplug.c: don't allow to eject the memory device if it is
> >> being used
> >> acpi_memhotplug.c: bind the memory device when the driver is being
> >> loaded
> >> acpi_memhotplug.c: auto bind the memory device which is hotplugged
> >> before the driver is loaded
> >>
> >> Yasuaki Ishimatsu (1):
> >> acpi,memory-hotplug : add memory offline code to
> >> acpi_memory_device_remove()
> >
> > Well, I have tried _really_ hard to apply this patchset, but pretty much
> > none of the patches except for [1/7] applied for me. I have no idea what
> > tree they are against, but I'm pretty sure it's not my tree.
> >
> > I _have_ applied patches [1-4/7] and pushed them to linux-pm.git/linux-next.
>
> I checked your tree and found a mistake.
> You merged a following patch into your tree.
>
> commitid:2ba281f1
> ACPI / memory-hotplug: introduce a mutex lock to protect the list
> in acpi_memory_device
>
> But it is wrong.
>
> [1/7] patch is "acpi,memory-hotplug : add memory offline code to
> acpi_memory_device_remove()". So we would like you to merge it
> instead of commitid:2ba281f1.
Yes, I've found it too.
Now applied patches [1-6/7], because I agree with Toshi Kani that patch [7/7]
goes too far, so I'm not going to apply it.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
Hi Rafael,
2012/11/16 10:28, Rafael J. Wysocki wrote:
> On Friday, November 16, 2012 10:07:49 AM Yasuaki Ishimatsu wrote:
>> Hi Rafael,
>>
>> 2012/11/16 9:28, Rafael J. Wysocki wrote:
>>> On Thursday, November 15, 2012 02:59:30 PM Wen Congyang wrote:
>>>> The memory device can be removed by 2 ways:
>>>> 1. send eject request by SCI
>>>> 2. echo 1 >/sys/bus/pci/devices/PNP0C80:XX/eject
>>>>
>>>> In the 1st case, acpi_memory_disable_device() will be called.
>>>> In the 2nd case, acpi_memory_device_remove() will be called.
>>>> acpi_memory_device_remove() will also be called when we unbind the
>>>> memory device from the driver acpi_memhotplug or a driver initialization
>>>> fails.
>>>>
>>>> acpi_memory_disable_device() has already implemented a code which
>>>> offlines memory and releases acpi_memory_info struct . But
>>>> acpi_memory_device_remove() has not implemented it yet.
>>>>
>>>> So the patch prepares the framework for hot removing memory and
>>>> adds the framework into acpi_memory_device_remove().
>>>>
>>>> We may hotremove the memory device by this 2 ways at the same time.
>>>> So we remove the function acpi_memory_disable_device(), and use
>>>> acpi_bus_hot_remove_device() which is used by 2nd case to implement it.
>>>> We lock device in acpi_bus_hot_remove_device(), so there is no
>>>> need to add lock in acpi_memhotplug.
>>>>
>>>> The last version of this patchset is here:
>>>> https://lkml.org/lkml/2012/11/8/121
>>>>
>>>> Note:
>>>> 1. The following commit in pm tree can be dropped now(The other two patches
>>>> are already dropped):
>>>> 54c4c7db6cb94d7d1217df6d7fca6847c61744ab
>>>> 2. This patchset requires the following patch(It is in pm tree now)
>>>> https://lkml.org/lkml/2012/11/1/225
>>>>
>>>> Changes from v4 to v5:
>>>> 1. patch2: new patch. use acpi_bus_hot_remove_device() to implement memory
>>>> device hotremove.
>>>>
>>>> Changes from v3 to v4:
>>>> 1. patch1: unlock list_lock when removing memory fails.
>>>> 2. patch2: just rebase them
>>>> 3. patch3-7: these patches are in -mm tree, and they conflict with this
>>>> patchset, so Adrew Morton drop them from -mm tree. I rebase and merge
>>>> them into this patchset.
>>>>
>>>> Wen Congyang (6):
>>>> acpi,memory-hotplug: deal with eject request in hotplug queue
>>>> acpi_memhotplug.c: fix memory leak when memory device is unbound from
>>>> the module acpi_memhotplug
>>>> acpi_memhotplug.c: free memory device if acpi_memory_enable_device()
>>>> failed
>>>> acpi_memhotplug.c: don't allow to eject the memory device if it is
>>>> being used
>>>> acpi_memhotplug.c: bind the memory device when the driver is being
>>>> loaded
>>>> acpi_memhotplug.c: auto bind the memory device which is hotplugged
>>>> before the driver is loaded
>>>>
>>>> Yasuaki Ishimatsu (1):
>>>> acpi,memory-hotplug : add memory offline code to
>>>> acpi_memory_device_remove()
>>>
>>> Well, I have tried _really_ hard to apply this patchset, but pretty much
>>> none of the patches except for [1/7] applied for me. I have no idea what
>>> tree they are against, but I'm pretty sure it's not my tree.
>>>
>>> I _have_ applied patches [1-4/7] and pushed them to linux-pm.git/linux-next.
>>
>> I checked your tree and found a mistake.
>> You merged a following patch into your tree.
>>
>> commitid:2ba281f1
>> ACPI / memory-hotplug: introduce a mutex lock to protect the list
>> in acpi_memory_device
>>
>> But it is wrong.
>>
>> [1/7] patch is "acpi,memory-hotplug : add memory offline code to
>> acpi_memory_device_remove()". So we would like you to merge it
>> instead of commitid:2ba281f1.
>
> Yes, I've found it too.
>
> Now applied patches [1-6/7], because I agree with Toshi Kani that patch [7/7]
> goes too far, so I'm not going to apply it.
I confirmed that patches were merged into your tree correctly.
Thanks,
Yasuaki Ishimatsu
>
> Thanks,
> Rafael
>
>
At 11/16/2012 07:32 AM, David Rientjes Wrote:
> On Thu, 15 Nov 2012, Wen Congyang wrote:
>
>> The memory device can be removed by 2 ways:
>> 1. send eject request by SCI
>> 2. echo 1 >/sys/bus/pci/devices/PNP0C80:XX/eject
>>
>> We handle the 1st case in the module acpi_memhotplug, and handle
>> the 2nd case in ACPI eject notification. This 2 events may happen
>> at the same time, so we may touch acpi_memory_device.res_list at
>> the same time. This patch reimplements memory-hotremove support
>> through an ACPI eject notification. Now the memory device is
>> offlined and hotremoved only in the function acpi_memory_device_remove()
>> which is protected by device_lock().
>>
>
> You mean it's protected by device_lock() before calling the remove()
> function for eject?
Yes. We call device_release_driver() to unbind the device from the driver
(the memory will be offlined/removed when it is unbound from the driver).
we call device_lock() in device_release_driver(), so we only unbind it
from the driver once.
Thanks
Wen Congyang
>
>> CC: David Rientjes <[email protected]>
>> CC: Jiang Liu <[email protected]>
>> CC: Len Brown <[email protected]>
>> CC: Benjamin Herrenschmidt <[email protected]>
>> CC: Paul Mackerras <[email protected]>
>> CC: Christoph Lameter <[email protected]>
>> Cc: Minchan Kim <[email protected]>
>> CC: Andrew Morton <[email protected]>
>> CC: KOSAKI Motohiro <[email protected]>
>> CC: Yasuaki Ishimatsu <[email protected]>
>> CC: Rafael J. Wysocki <[email protected]>
>> CC: Konrad Rzeszutek Wilk <[email protected]>
>> Signed-off-by: Wen Congyang <[email protected]>
>> ---
>> drivers/acpi/acpi_memhotplug.c | 87 +++++-------------------------------------
>> 1 file changed, 9 insertions(+), 78 deletions(-)
>>
>> diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
>> index 2918be1..6e12042 100644
>> --- a/drivers/acpi/acpi_memhotplug.c
>> +++ b/drivers/acpi/acpi_memhotplug.c
>> @@ -272,40 +272,6 @@ static int acpi_memory_enable_device(struct acpi_memory_device *mem_device)
>> return 0;
>> }
>>
>> -static int acpi_memory_powerdown_device(struct acpi_memory_device *mem_device)
>> -{
>> - acpi_status status;
>> - struct acpi_object_list arg_list;
>> - union acpi_object arg;
>> - unsigned long long current_status;
>> -
>> -
>> - /* Issue the _EJ0 command */
>> - arg_list.count = 1;
>> - arg_list.pointer = &arg;
>> - arg.type = ACPI_TYPE_INTEGER;
>> - arg.integer.value = 1;
>> - status = acpi_evaluate_object(mem_device->device->handle,
>> - "_EJ0", &arg_list, NULL);
>> - /* Return on _EJ0 failure */
>> - if (ACPI_FAILURE(status)) {
>> - ACPI_EXCEPTION((AE_INFO, status, "_EJ0 failed"));
>> - return -ENODEV;
>> - }
>> -
>> - /* Evalute _STA to check if the device is disabled */
>> - status = acpi_evaluate_integer(mem_device->device->handle, "_STA",
>> - NULL, ¤t_status);
>> - if (ACPI_FAILURE(status))
>> - return -ENODEV;
>> -
>> - /* Check for device status. Device should be disabled */
>> - if (current_status & ACPI_STA_DEVICE_ENABLED)
>> - return -EINVAL;
>> -
>> - return 0;
>> -}
>> -
>> static int acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
>> {
>> int result;
>> @@ -325,34 +291,11 @@ static int acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
>> return 0;
>> }
>>
>> -static int acpi_memory_disable_device(struct acpi_memory_device *mem_device)
>> -{
>> - int result;
>> -
>> - /*
>> - * Ask the VM to offline this memory range.
>> - * Note: Assume that this function returns zero on success
>> - */
>> - result = acpi_memory_remove_memory(mem_device);
>> - if (result)
>> - return result;
>> -
>> - /* Power-off and eject the device */
>> - result = acpi_memory_powerdown_device(mem_device);
>> - if (result) {
>> - /* Set the status of the device to invalid */
>> - mem_device->state = MEMORY_INVALID_STATE;
>> - return result;
>> - }
>> -
>> - mem_device->state = MEMORY_POWER_OFF_STATE;
>> - return result;
>> -}
>> -
>> static void acpi_memory_device_notify(acpi_handle handle, u32 event, void *data)
>> {
>> struct acpi_memory_device *mem_device;
>> struct acpi_device *device;
>> + struct acpi_eject_event *ej_event = NULL;
>> u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE; /* default */
>>
>> switch (event) {
>> @@ -394,31 +337,19 @@ static void acpi_memory_device_notify(acpi_handle handle, u32 event, void *data)
>> break;
>> }
>>
>> - /*
>> - * Currently disabling memory device from kernel mode
>> - * TBD: Can also be disabled from user mode scripts
>> - * TBD: Can also be disabled by Callback registration
>> - * with generic sysfs driver
>> - */
>> - if (acpi_memory_disable_device(mem_device)) {
>> - printk(KERN_ERR PREFIX "Disable memory device\n");
>> - /*
>> - * If _EJ0 was called but failed, _OST is not
>> - * necessary.
>> - */
>> - if (mem_device->state == MEMORY_INVALID_STATE)
>> - return;
>> -
>> + ej_event = kmalloc(sizeof(*ej_event), GFP_KERNEL);
>> + if (!ej_event) {
>> + pr_err(PREFIX "No memory, dropping EJECT\n");
>> break;
>> }
>>
>> - /*
>> - * TBD: Invoke acpi_bus_remove to cleanup data structures
>> - */
>> + ej_event->handle = handle;
>> + ej_event->event = ACPI_NOTIFY_EJECT_REQUEST;
>> + acpi_os_hotplug_execute(acpi_bus_hot_remove_device,
>> + (void *)ej_event);
>>
>> - /* _EJ0 succeeded; _OST is not necessary */
>> + /* eject is performed asynchronously */
>> return;
>> -
>> default:
>> ACPI_DEBUG_PRINT((ACPI_DB_INFO,
>> "Unsupported event [0x%x]\n", event));
>
2012/11/15 Wen Congyang <[email protected]>
>
> We eject the memory device even if it is in use. It is very dangerous,
> and it will cause the kernel to be panicked.
>
> CC: David Rientjes <[email protected]>
> CC: Jiang Liu <[email protected]>
> CC: Len Brown <[email protected]>
> CC: Benjamin Herrenschmidt <[email protected]>
> CC: Paul Mackerras <[email protected]>
> CC: Christoph Lameter <[email protected]>
> Cc: Minchan Kim <[email protected]>
> CC: Andrew Morton <[email protected]>
> CC: KOSAKI Motohiro <[email protected]>
> CC: Yasuaki Ishimatsu <[email protected]>
> CC: Rafael J. Wysocki <[email protected]>
> CC: Konrad Rzeszutek Wilk <[email protected]>
> Signed-off-by: Wen Congyang <[email protected]>
> ---
> drivers/acpi/acpi_memhotplug.c | 42 +++++++++++++++++++++++++++++++++---------
> 1 file changed, 33 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
> index e52ad5d..f7e3007 100644
> --- a/drivers/acpi/acpi_memhotplug.c
> +++ b/drivers/acpi/acpi_memhotplug.c
> @@ -78,6 +78,7 @@ struct acpi_memory_info {
> unsigned short caching; /* memory cache attribute */
> unsigned short write_protect; /* memory read/write attribute */
> unsigned int enabled:1;
> + unsigned int failed:1;
> };
>
> struct acpi_memory_device {
> @@ -257,9 +258,23 @@ static int acpi_memory_enable_device(struct acpi_memory_device *mem_device)
> node = memory_add_physaddr_to_nid(info->start_addr);
>
> result = add_memory(node, info->start_addr, info->length);
> - if (result)
> +
> + /*
> + * If the memory block has been used by the kernel, add_memory()
> + * returns -EEXIST. If add_memory() returns the other error, it
> + * means that this memory block is not used by the kernel.
> + */
> + if (result && result != -EEXIST) {
> + info->failed = 1;
> continue;
> - info->enabled = 1;
> + }
> +
> + if (!result)
> + info->enabled = 1;
Hi Congyang,
If a memory device booting with the system, the info->enable will equal to 0,
and can not be hot-removed in acpi_memory_remove_memory(), right?
Thanks,
Jianguo Wu
>
> + /*
> + * Add num_enable even if add_memory() returns -EEXIST, so the
> + * device is bound to this driver.
> + */
> num_enabled++;
> }
> if (!num_enabled) {
> @@ -280,21 +295,30 @@ static int acpi_memory_enable_device(struct acpi_memory_device *mem_device)
>
> static int acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
> {
> - int result;
> + int result = 0;
> struct acpi_memory_info *info, *n;
>
> list_for_each_entry_safe(info, n, &mem_device->res_list, list) {
> - if (info->enabled) {
> - result = remove_memory(info->start_addr, info->length);
> - if (result)
> - return result;
> - }
> + if (info->failed)
> + /* The kernel does not use this memory block */
> + continue;
> +
> + if (!info->enabled)
> + /*
> + * The kernel uses this memory block, but it may be not
> + * managed by us.
> + */
> + return -EBUSY;
> +
> + result = remove_memory(info->start_addr, info->length);
> + if (result)
> + return result;
>
> list_del(&info->list);
> kfree(info);
> }
>
> - return 0;
> + return result;
> }
>
> static void acpi_memory_device_notify(acpi_handle handle, u32 event, void *data)
> --
> 1.8.0
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to [email protected]. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"[email protected]"> [email protected] </a>