All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/8] some fixes about acpi_memhotplug
@ 2012-06-26  9:16 Wen Congyang
  2012-06-26  9:19 ` [PATCH 1/8] fix memory leak when memory device is unbound from the module acpi_memhotplug Wen Congyang
                   ` (7 more replies)
  0 siblings, 8 replies; 21+ messages in thread
From: Wen Congyang @ 2012-06-26  9:16 UTC (permalink / raw)
  To: lenb, linux-acpi, linux-kernel; +Cc: Yasuaki ISIMATU

Wen Congyang (8):
  fix memory leak when memory device is unbound from the module
    acpi_memhotplug
  free memory device if acpi_memory_enable_device() failed
  remove memory info from list before freeing it
  donot allow to eject the memory device if it is being used
  don't print message if request_resource() failed
  bind the memory device when the driver is being loaded
  auto bind the memory device which is hotpluged before the driver is
    loaded
  release memory resources if hotadd_new_pgdat() failed

 drivers/acpi/acpi_memhotplug.c |  106 ++++++++++++++++++++++++++++++----------
 mm/memory_hotplug.c            |    3 +-
 2 files changed, 81 insertions(+), 28 deletions(-)

^ permalink raw reply	[flat|nested] 21+ messages in thread
* [PATCH 1/8] fix memory leak when memory device is unbound from the module acpi_memhotplug
  2012-06-26  9:16 [PATCH 0/8] some fixes about acpi_memhotplug Wen Congyang
@ 2012-06-26  9:19 ` Wen Congyang
  2012-06-27  4:10   ` David Rientjes
  2012-06-26  9:19 ` [PATCH 2/8] free memory device if acpi_memory_enable_device() failed Wen Congyang
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 21+ messages in thread
From: Wen Congyang @ 2012-06-26  9:19 UTC (permalink / raw)
  To: lenb, linux-acpi, linux-kernel; +Cc: Yasuaki ISIMATU

We allocate memory to store acpi_memory_info, so we should free it before
freeing mem_device.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 drivers/acpi/acpi_memhotplug.c |   18 +++++++++++++-----
 1 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
index d985713..f6831d1 100644
--- a/drivers/acpi/acpi_memhotplug.c
+++ b/drivers/acpi/acpi_memhotplug.c
@@ -399,6 +399,18 @@ static void acpi_memory_device_notify(acpi_handle handle, u32 event, void *data)
 	return;
 }
 
+static void acpi_free_memory_device(struct acpi_memory_device *mem_device)
+{
+	struct acpi_memory_info *info, *n;
+
+	if (!mem_device)
+		return;
+
+	list_for_each_entry_safe(info, n, &mem_device->res_list, list)
+		kfree(info);
+	kfree(mem_device);
+}
+
 static int acpi_memory_device_add(struct acpi_device *device)
 {
 	int result;
@@ -451,14 +463,10 @@ 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);
+	acpi_free_memory_device(acpi_driver_data(device));
 
 	return 0;
 }
-- 
1.7.1


^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH 2/8] free memory device if acpi_memory_enable_device() failed
  2012-06-26  9:16 [PATCH 0/8] some fixes about acpi_memhotplug Wen Congyang
  2012-06-26  9:19 ` [PATCH 1/8] fix memory leak when memory device is unbound from the module acpi_memhotplug Wen Congyang
@ 2012-06-26  9:19 ` Wen Congyang
  2012-06-27  4:12   ` David Rientjes
  2012-06-26  9:20 ` [PATCH 3/8] remove memory info from list before freeing it Wen Congyang
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 21+ messages in thread
From: Wen Congyang @ 2012-06-26  9:19 UTC (permalink / raw)
  To: lenb, linux-acpi, linux-kernel; +Cc: Yasuaki ISIMATU

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.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 drivers/acpi/acpi_memhotplug.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
index f6831d1..bb7bc66 100644
--- a/drivers/acpi/acpi_memhotplug.c
+++ b/drivers/acpi/acpi_memhotplug.c
@@ -454,9 +454,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_free_memory_device(mem_device);
+		}
 	}
 	return result;
 }
-- 
1.7.1


^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH 3/8] remove memory info from list before freeing it
  2012-06-26  9:16 [PATCH 0/8] some fixes about acpi_memhotplug Wen Congyang
  2012-06-26  9:19 ` [PATCH 1/8] fix memory leak when memory device is unbound from the module acpi_memhotplug Wen Congyang
  2012-06-26  9:19 ` [PATCH 2/8] free memory device if acpi_memory_enable_device() failed Wen Congyang
@ 2012-06-26  9:20 ` Wen Congyang
  2012-06-27  4:15   ` David Rientjes
  2012-06-26  9:20 ` [PATCH 4/8] donot allow to eject the memory device if it is being used Wen Congyang
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 21+ messages in thread
From: Wen Congyang @ 2012-06-26  9:20 UTC (permalink / raw)
  To: lenb, linux-acpi, linux-kernel; +Cc: Yasuaki ISIMATU

We free info, but we forget to remove it from the list. It will cause
unexpected problem when we access the list next time.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 drivers/acpi/acpi_memhotplug.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
index bb7bc66..a325bb9 100644
--- a/drivers/acpi/acpi_memhotplug.c
+++ b/drivers/acpi/acpi_memhotplug.c
@@ -322,6 +322,7 @@ static int acpi_memory_disable_device(struct acpi_memory_device *mem_device)
 			if (result)
 				return result;
 		}
+		list_del(&info->list);
 		kfree(info);
 	}
 
-- 
1.7.1


^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH 4/8] donot allow to eject the memory device if it is being used
  2012-06-26  9:16 [PATCH 0/8] some fixes about acpi_memhotplug Wen Congyang
                   ` (2 preceding siblings ...)
  2012-06-26  9:20 ` [PATCH 3/8] remove memory info from list before freeing it Wen Congyang
@ 2012-06-26  9:20 ` Wen Congyang
  2012-06-27  4:21   ` David Rientjes
  2012-06-26  9:21 ` [PATCH 5/8] don't print message if request_resource() failed Wen Congyang
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 21+ messages in thread
From: Wen Congyang @ 2012-06-26  9:20 UTC (permalink / raw)
  To: lenb, linux-acpi, linux-kernel; +Cc: Yasuaki ISIMATU

We eject the memory device even if it is in use. It is very dangerous,
and it will cause the kernel panicked.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 drivers/acpi/acpi_memhotplug.c |   34 +++++++++++++++++++++++++++-------
 1 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
index a325bb9..2e5d5ab 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 {
@@ -251,9 +252,19 @@ 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;
 		num_enabled++;
 	}
 	if (!num_enabled) {
@@ -317,11 +328,20 @@ static int acpi_memory_disable_device(struct acpi_memory_device *mem_device)
 	 * 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;
-		}
+		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);
 	}
-- 
1.7.1


^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH 5/8] don't print message if request_resource() failed
  2012-06-26  9:16 [PATCH 0/8] some fixes about acpi_memhotplug Wen Congyang
                   ` (3 preceding siblings ...)
  2012-06-26  9:20 ` [PATCH 4/8] donot allow to eject the memory device if it is being used Wen Congyang
@ 2012-06-26  9:21 ` Wen Congyang
  2012-06-27  4:23   ` David Rientjes
  2012-06-26  9:21 ` [PATCH 6/8] bind the memory device when the driver is being loaded Wen Congyang
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 21+ messages in thread
From: Wen Congyang @ 2012-06-26  9:21 UTC (permalink / raw)
  To: lenb, linux-acpi, linux-kernel; +Cc: Yasuaki ISIMATU

If register_memory_resource() fails, the caller(add_memory()) will
return -EEXIST, and add_memory() returns -EEXIST only when
register_memory_resource() fails. The function acpi_memory_enable_device()
doesn't treat such erro as a fetal error, and don't want this message.
The function that calls add_memory() has printed message if add_memory()
failed, so don't print message in register_memory_resource().

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 mm/memory_hotplug.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 0d7e3ec..2a14d35 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -74,7 +74,6 @@ static struct resource *register_memory_resource(u64 start, u64 size)
 	res->end = start + size - 1;
 	res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
 	if (request_resource(&iomem_resource, res) < 0) {
-		printk("System RAM resource %pR cannot be added\n", res);
 		kfree(res);
 		res = NULL;
 	}
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH 6/8] bind the memory device when the driver is being loaded
  2012-06-26  9:16 [PATCH 0/8] some fixes about acpi_memhotplug Wen Congyang
                   ` (4 preceding siblings ...)
  2012-06-26  9:21 ` [PATCH 5/8] don't print message if request_resource() failed Wen Congyang
@ 2012-06-26  9:21 ` Wen Congyang
  2012-06-26 15:53   ` Konrad Rzeszutek Wilk
  2012-06-27  4:24   ` David Rientjes
  2012-06-26  9:22 ` [PATCH 7/8] auto bind the memory device which is hotpluged before the driver is loaded Wen Congyang
  2012-06-26  9:23 ` [PATCH 8/8] release memory resources if hotadd_new_pgdat() failed Wen Congyang
  7 siblings, 2 replies; 21+ messages in thread
From: Wen Congyang @ 2012-06-26  9:21 UTC (permalink / raw)
  To: lenb, linux-acpi, linux-kernel; +Cc: Yasuaki ISIMATU

We introduce acpi_hotmem_initialized to avoid strange add_memory fail message.
The stranged add_memory fail message is printed in register_memory_resource().
We have removed this message in the previous patch, so we can remove
it(it is very useful to the next patch).

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 drivers/acpi/acpi_memhotplug.c |   12 ------------
 1 files changed, 0 insertions(+), 12 deletions(-)

diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
index 2e5d5ab..148c88a 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)
 {
@@ -463,15 +461,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);
@@ -578,7 +567,6 @@ static int __init acpi_memory_device_init(void)
 		return -ENODEV;
 	}
 
-	acpi_hotmem_initialized = 1;
 	return 0;
 }
 
-- 
1.7.1


^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH 7/8] auto bind the memory device which is hotpluged before the driver is loaded
  2012-06-26  9:16 [PATCH 0/8] some fixes about acpi_memhotplug Wen Congyang
                   ` (5 preceding siblings ...)
  2012-06-26  9:21 ` [PATCH 6/8] bind the memory device when the driver is being loaded Wen Congyang
@ 2012-06-26  9:22 ` Wen Congyang
  2012-06-26 17:06   ` Konrad Rzeszutek Wilk
  2012-06-26  9:23 ` [PATCH 8/8] release memory resources if hotadd_new_pgdat() failed Wen Congyang
  7 siblings, 1 reply; 21+ messages in thread
From: Wen Congyang @ 2012-06-26  9:22 UTC (permalink / raw)
  To: lenb, linux-acpi, linux-kernel; +Cc: Yasuaki ISIMATU

If the memory device is hotpluged 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 is driver
is being loaded.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 drivers/acpi/acpi_memhotplug.c |   37 ++++++++++++++++++++++++++++++++++++-
 1 files changed, 36 insertions(+), 1 deletions(-)

diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
index 148c88a..a682657 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);
 
@@ -515,12 +518,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) {
+		if (acpi_memory_get_device(handle, &mem_device))
+			goto install;
+
+		/* Get device present/absent information from the _STA */
+		status = acpi_evaluate_integer(mem_device->device->handle,
+					       "_STA", NULL, &current_status);
+		if (ACPI_FAILURE(status))
+			goto install;
+
+		/*
+		 * Check for device status. Device should be
+		 * present/enabled/functioning.
+		 */
+		if (!((current_status & ACPI_STA_DEVICE_PRESENT)
+		      && (current_status & ACPI_STA_DEVICE_ENABLED)
+		      && (current_status & ACPI_STA_DEVICE_FUNCTIONING)))
+			goto install;
+
+		/* We have bound this device while we register the dirver */
+		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.7.1


^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH 8/8] release memory resources if hotadd_new_pgdat() failed
  2012-06-26  9:16 [PATCH 0/8] some fixes about acpi_memhotplug Wen Congyang
                   ` (6 preceding siblings ...)
  2012-06-26  9:22 ` [PATCH 7/8] auto bind the memory device which is hotpluged before the driver is loaded Wen Congyang
@ 2012-06-26  9:23 ` Wen Congyang
  2012-06-27  4:29   ` David Rientjes
  7 siblings, 1 reply; 21+ messages in thread
From: Wen Congyang @ 2012-06-26  9:23 UTC (permalink / raw)
  To: lenb, linux-acpi, linux-kernel; +Cc: Yasuaki ISIMATU

We should goto error to release memory resource if hotadd_new_pgdat() failed.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 mm/memory_hotplug.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 2a14d35..3796690 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -617,7 +617,7 @@ int __ref add_memory(int nid, u64 start, u64 size)
 		pgdat = hotadd_new_pgdat(nid, start);
 		ret = -ENOMEM;
 		if (!pgdat)
-			goto out;
+			goto error;
 		new_pgdat = 1;
 	}
 
-- 
1.7.1


^ permalink raw reply related	[flat|nested] 21+ messages in thread
* Re: [PATCH 6/8] bind the memory device when the driver is being loaded
  2012-06-26  9:21 ` [PATCH 6/8] bind the memory device when the driver is being loaded Wen Congyang
@ 2012-06-26 15:53   ` Konrad Rzeszutek Wilk
  2012-06-27  4:24   ` David Rientjes
  1 sibling, 0 replies; 21+ messages in thread
From: Konrad Rzeszutek Wilk @ 2012-06-26 15:53 UTC (permalink / raw)
  To: Wen Congyang; +Cc: lenb, linux-acpi, linux-kernel, Yasuaki ISIMATU

On Tue, Jun 26, 2012 at 05:21:56PM +0800, Wen Congyang wrote:
> We introduce acpi_hotmem_initialized to avoid strange add_memory fail message.

We had

> The stranged add_memory fail message is printed in register_memory_resource().

The strange

> We have removed this message in the previous patch, so we can remove

Don't say previous patch as it is unclear in the git tree which one that
is. Instead say "remove this message in 'don't print message if request_resource() failed'
patch.

> it(it is very useful to the next patch).

And ditto on the 'next patch' - it is not clear in git what is the next patch.

> 
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> ---
>  drivers/acpi/acpi_memhotplug.c |   12 ------------
>  1 files changed, 0 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
> index 2e5d5ab..148c88a 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)
>  {
> @@ -463,15 +461,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);
> @@ -578,7 +567,6 @@ static int __init acpi_memory_device_init(void)
>  		return -ENODEV;
>  	}
>  
> -	acpi_hotmem_initialized = 1;
>  	return 0;
>  }
>  
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 7/8] auto bind the memory device which is hotpluged before the driver is loaded
  2012-06-26  9:22 ` [PATCH 7/8] auto bind the memory device which is hotpluged before the driver is loaded Wen Congyang
@ 2012-06-26 17:06   ` Konrad Rzeszutek Wilk
  2012-06-27  4:33     ` David Rientjes
  0 siblings, 1 reply; 21+ messages in thread
From: Konrad Rzeszutek Wilk @ 2012-06-26 17:06 UTC (permalink / raw)
  To: Wen Congyang; +Cc: lenb, linux-acpi, linux-kernel, Yasuaki ISIMATU

On Tue, Jun 26, 2012 at 05:22:40PM +0800, Wen Congyang wrote:
> If the memory device is hotpluged 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.

Why not? Can the user rmmod acpi_memhotplug and modprobe it?

> This patch introduces a new feature to bind such device when is driver

s/when is/when the/
> is being loaded.
> 
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> ---
>  drivers/acpi/acpi_memhotplug.c |   37 ++++++++++++++++++++++++++++++++++++-
>  1 files changed, 36 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
> index 148c88a..a682657 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);
>  
> @@ -515,12 +518,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;

Can it just be unsigned long?

>  
>  	status = is_memory_device(handle);
>  	if (ACPI_FAILURE(status))
>  		return AE_OK;	/* continue */
>  
> +	if (auto_probe) {
> +		if (acpi_memory_get_device(handle, &mem_device))
> +			goto install;
> +
> +		/* Get device present/absent information from the _STA */
> +		status = acpi_evaluate_integer(mem_device->device->handle,
> +					       "_STA", NULL, &current_status);
> +		if (ACPI_FAILURE(status))
> +			goto install;
> +
> +		/*
> +		 * Check for device status. Device should be
> +		 * present/enabled/functioning.
> +		 */
> +		if (!((current_status & ACPI_STA_DEVICE_PRESENT)
> +		      && (current_status & ACPI_STA_DEVICE_ENABLED)
> +		      && (current_status & ACPI_STA_DEVICE_FUNCTIONING)))

Can you make this:
	!(current_status & (ACPI_STA_DEVICE_PRESENT | ACPI_STA_DEVICE_ENABLED | ACPI_STA_DEVICE_FUNCTIONING))

instead?
> +			goto install;
> +
> +		/* We have bound this device while we register the dirver */

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");

Would it be prudent to provide more information? Like which device could
not be enabled? And perhaps the reason why it could not be enabled?

> +	}
> +
> +install:
>  	status = acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
>  					     acpi_memory_device_notify, NULL);
>  	/* continue */
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 1/8] fix memory leak when memory device is unbound from the module acpi_memhotplug
  2012-06-26  9:19 ` [PATCH 1/8] fix memory leak when memory device is unbound from the module acpi_memhotplug Wen Congyang
@ 2012-06-27  4:10   ` David Rientjes
  0 siblings, 0 replies; 21+ messages in thread
From: David Rientjes @ 2012-06-27  4:10 UTC (permalink / raw)
  To: Wen Congyang; +Cc: lenb, linux-acpi, linux-kernel, Yasuaki ISIMATU

On Tue, 26 Jun 2012, Wen Congyang wrote:

> We allocate memory to store acpi_memory_info, so we should free it before
> freeing mem_device.
> 
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> ---
>  drivers/acpi/acpi_memhotplug.c |   18 +++++++++++++-----
>  1 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
> index d985713..f6831d1 100644
> --- a/drivers/acpi/acpi_memhotplug.c
> +++ b/drivers/acpi/acpi_memhotplug.c
> @@ -399,6 +399,18 @@ static void acpi_memory_device_notify(acpi_handle handle, u32 event, void *data)
>  	return;
>  }
>  
> +static void acpi_free_memory_device(struct acpi_memory_device *mem_device)
> +{

The function that allocates struct acpi_memory_device is 
acpi_memory_device_add(), shouldn't this be called acpi_memory_device_free()?

> +	struct acpi_memory_info *info, *n;
> +
> +	if (!mem_device)
> +		return;
> +
> +	list_for_each_entry_safe(info, n, &mem_device->res_list, list)
> +		kfree(info);

Duplicates code from acpi_memory_get_device_resources(), should be moved 
into a seperate function.

> +	kfree(mem_device);
> +}
> +
>  static int acpi_memory_device_add(struct acpi_device *device)
>  {
>  	int result;
> @@ -451,14 +463,10 @@ 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);
> +	acpi_free_memory_device(acpi_driver_data(device));
>  
>  	return 0;
>  }

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 2/8] free memory device if acpi_memory_enable_device() failed
  2012-06-26  9:19 ` [PATCH 2/8] free memory device if acpi_memory_enable_device() failed Wen Congyang
@ 2012-06-27  4:12   ` David Rientjes
  0 siblings, 0 replies; 21+ messages in thread
From: David Rientjes @ 2012-06-27  4:12 UTC (permalink / raw)
  To: Wen Congyang; +Cc: lenb, linux-acpi, linux-kernel, Yasuaki ISIMATU

On Tue, 26 Jun 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.
> 
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>

Acked-by: David Rientjes <rientjes@google.com>

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 3/8] remove memory info from list before freeing it
  2012-06-26  9:20 ` [PATCH 3/8] remove memory info from list before freeing it Wen Congyang
@ 2012-06-27  4:15   ` David Rientjes
  0 siblings, 0 replies; 21+ messages in thread
From: David Rientjes @ 2012-06-27  4:15 UTC (permalink / raw)
  To: Wen Congyang; +Cc: lenb, linux-acpi, linux-kernel, Yasuaki ISIMATU

On Tue, 26 Jun 2012, Wen Congyang wrote:

> We free info, but we forget to remove it from the list. It will cause
> unexpected problem when we access the list next time.
> 
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>

Acked-by: David Rientjes <rientjes@google.com>

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 4/8] donot allow to eject the memory device if it is being used
  2012-06-26  9:20 ` [PATCH 4/8] donot allow to eject the memory device if it is being used Wen Congyang
@ 2012-06-27  4:21   ` David Rientjes
  2012-06-28  2:01     ` Wen Congyang
  0 siblings, 1 reply; 21+ messages in thread
From: David Rientjes @ 2012-06-27  4:21 UTC (permalink / raw)
  To: Wen Congyang; +Cc: lenb, linux-acpi, linux-kernel, Yasuaki ISIMATU

On Tue, 26 Jun 2012, Wen Congyang wrote:

> diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
> index a325bb9..2e5d5ab 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 {
> @@ -251,9 +252,19 @@ 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;
>  		num_enabled++;
>  	}
>  	if (!num_enabled) {

num_enabled should only be incremented for result == 0.

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 5/8] don't print message if request_resource() failed
  2012-06-26  9:21 ` [PATCH 5/8] don't print message if request_resource() failed Wen Congyang
@ 2012-06-27  4:23   ` David Rientjes
  2012-06-28  2:46     ` Wen Congyang
  0 siblings, 1 reply; 21+ messages in thread
From: David Rientjes @ 2012-06-27  4:23 UTC (permalink / raw)
  To: Wen Congyang; +Cc: lenb, linux-acpi, linux-kernel, Yasuaki ISIMATU

On Tue, 26 Jun 2012, Wen Congyang wrote:

> If register_memory_resource() fails, the caller(add_memory()) will
> return -EEXIST, and add_memory() returns -EEXIST only when
> register_memory_resource() fails. The function acpi_memory_enable_device()
> doesn't treat such erro as a fetal error, and don't want this message.
> The function that calls add_memory() has printed message if add_memory()
> failed, so don't print message in register_memory_resource().
> 

That may be true for acpi_memory_enable_device(), but have you checked 
other callers to add_memory()?

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 6/8] bind the memory device when the driver is being loaded
  2012-06-26  9:21 ` [PATCH 6/8] bind the memory device when the driver is being loaded Wen Congyang
  2012-06-26 15:53   ` Konrad Rzeszutek Wilk
@ 2012-06-27  4:24   ` David Rientjes
  1 sibling, 0 replies; 21+ messages in thread
From: David Rientjes @ 2012-06-27  4:24 UTC (permalink / raw)
  To: Wen Congyang; +Cc: lenb, linux-acpi, linux-kernel, Yasuaki ISIMATU

On Tue, 26 Jun 2012, Wen Congyang wrote:

> We introduce acpi_hotmem_initialized to avoid strange add_memory fail message.
> The stranged add_memory fail message is printed in register_memory_resource().
> We have removed this message in the previous patch, so we can remove
> it(it is very useful to the next patch).
> 

I don't think you can legitimately remove that message, see my comment in 
the previous patch.  There are many callers to add_memory() and it seems 
like this workaround solves most of the pain for acpi, so maybe we can 
just live with it?

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 8/8] release memory resources if hotadd_new_pgdat() failed
  2012-06-26  9:23 ` [PATCH 8/8] release memory resources if hotadd_new_pgdat() failed Wen Congyang
@ 2012-06-27  4:29   ` David Rientjes
  0 siblings, 0 replies; 21+ messages in thread
From: David Rientjes @ 2012-06-27  4:29 UTC (permalink / raw)
  To: Andrew Morton, Wen Congyang
  Cc: lenb, linux-acpi, linux-kernel, Yasuaki ISIMATU

On Tue, 26 Jun 2012, Wen Congyang wrote:

> We should goto error to release memory resource if hotadd_new_pgdat() failed.
> 
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> ---
>  mm/memory_hotplug.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 2a14d35..3796690 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -617,7 +617,7 @@ int __ref add_memory(int nid, u64 start, u64 size)
>  		pgdat = hotadd_new_pgdat(nid, start);
>  		ret = -ENOMEM;
>  		if (!pgdat)
> -			goto out;
> +			goto error;
>  		new_pgdat = 1;
>  	}
>  

This is a generic memory hotplug patch and doesn't rely on the rest of the 
series, so let's cc the maintainer, Andrew Morton 
<akpm@linux-foundation.org>.

Acked-by: David Rientjes <rientjes@google.com>

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 7/8] auto bind the memory device which is hotpluged before the driver is loaded
  2012-06-26 17:06   ` Konrad Rzeszutek Wilk
@ 2012-06-27  4:33     ` David Rientjes
  0 siblings, 0 replies; 21+ messages in thread
From: David Rientjes @ 2012-06-27  4:33 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk
  Cc: Wen Congyang, lenb, linux-acpi, linux-kernel, Yasuaki ISIMATU

On Tue, 26 Jun 2012, Konrad Rzeszutek Wilk wrote:

> > diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
> > index 148c88a..a682657 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);
> >  
> > @@ -515,12 +518,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;
> 
> Can it just be unsigned long?
> 

Not on 32-bit, the ACPI 2.0 and later specs can handle 64 bit integers.

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 4/8] donot allow to eject the memory device if it is being used
  2012-06-27  4:21   ` David Rientjes
@ 2012-06-28  2:01     ` Wen Congyang
  0 siblings, 0 replies; 21+ messages in thread
From: Wen Congyang @ 2012-06-28  2:01 UTC (permalink / raw)
  To: David Rientjes; +Cc: lenb, linux-acpi, linux-kernel, Yasuaki ISIMATU

At 06/27/2012 12:21 PM, David Rientjes Wrote:
> On Tue, 26 Jun 2012, Wen Congyang wrote:
> 
>> diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
>> index a325bb9..2e5d5ab 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 {
>> @@ -251,9 +252,19 @@ 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;
>>  		num_enabled++;
>>  	}
>>  	if (!num_enabled) {
> 
> num_enabled should only be incremented for result == 0.
> 

If num_enabled is not incremented for result == -EEXIST, this device is not
managed by this module, and it is not able to be hot removed. Another problem
is the user will see a strange message: add_memory failed

Thanks
Wen Congyang

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 5/8] don't print message if request_resource() failed
  2012-06-27  4:23   ` David Rientjes
@ 2012-06-28  2:46     ` Wen Congyang
  0 siblings, 0 replies; 21+ messages in thread
From: Wen Congyang @ 2012-06-28  2:46 UTC (permalink / raw)
  To: David Rientjes; +Cc: lenb, linux-acpi, linux-kernel, Yasuaki ISIMATU

At 06/27/2012 12:23 PM, David Rientjes Wrote:
> On Tue, 26 Jun 2012, Wen Congyang wrote:
> 
>> If register_memory_resource() fails, the caller(add_memory()) will
>> return -EEXIST, and add_memory() returns -EEXIST only when
>> register_memory_resource() fails. The function acpi_memory_enable_device()
>> doesn't treat such erro as a fetal error, and don't want this message.
>> The function that calls add_memory() has printed message if add_memory()
>> failed, so don't print message in register_memory_resource().
>>
> 
> That may be true for acpi_memory_enable_device(), but have you checked 
> other callers to add_memory()?
> 

There are four functions that call add_memory():
1. acpi_memory_enable_device()
2. memory_probe_store()
   This caller is the callback of system call write. The user can get the return
   value by errno.
3. add_memory_merged()
   This caller does not check the return value. I guess add_memory() always
   successes in this place.
4. bp_state reserve_additional_memory()
   This caller prints message if add_memory() failed.

So I think we can remove this message...

Thanks
Wen Congyang

^ permalink raw reply	[flat|nested] 21+ messages in thread
end of thread, other threads:[~2012-06-28  2:41 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-06-26  9:16 [PATCH 0/8] some fixes about acpi_memhotplug Wen Congyang
2012-06-26  9:19 ` [PATCH 1/8] fix memory leak when memory device is unbound from the module acpi_memhotplug Wen Congyang
2012-06-27  4:10   ` David Rientjes
2012-06-26  9:19 ` [PATCH 2/8] free memory device if acpi_memory_enable_device() failed Wen Congyang
2012-06-27  4:12   ` David Rientjes
2012-06-26  9:20 ` [PATCH 3/8] remove memory info from list before freeing it Wen Congyang
2012-06-27  4:15   ` David Rientjes
2012-06-26  9:20 ` [PATCH 4/8] donot allow to eject the memory device if it is being used Wen Congyang
2012-06-27  4:21   ` David Rientjes
2012-06-28  2:01     ` Wen Congyang
2012-06-26  9:21 ` [PATCH 5/8] don't print message if request_resource() failed Wen Congyang
2012-06-27  4:23   ` David Rientjes
2012-06-28  2:46     ` Wen Congyang
2012-06-26  9:21 ` [PATCH 6/8] bind the memory device when the driver is being loaded Wen Congyang
2012-06-26 15:53   ` Konrad Rzeszutek Wilk
2012-06-27  4:24   ` David Rientjes
2012-06-26  9:22 ` [PATCH 7/8] auto bind the memory device which is hotpluged before the driver is loaded Wen Congyang
2012-06-26 17:06   ` Konrad Rzeszutek Wilk
2012-06-27  4:33     ` David Rientjes
2012-06-26  9:23 ` [PATCH 8/8] release memory resources if hotadd_new_pgdat() failed Wen Congyang
2012-06-27  4:29   ` David Rientjes

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.