[PATCH](memory hotplug) remove useless message at boot time from 2.6.18-rc3.

[PATCH](memory hotplug) remove useless message at boot time from 2.6.18-rc3.

[Yasunori Goto]

This is to remove noisy useless message at boot time from 2.6.18-rc3.
(-mm doesn't shows this message. I don't know why.)

The message is a ton of 
"ACPI Exception (acpi_memory-0492): AE_ERROR, handle is no memory device"

It is showed by acpi_memory_register_notify_handler() which
is called by acpi_walk_namespace().

acpi_walk_namespace() parses all of ACPI's namespace and 
executes acpi_memory_register_notify_handler(). So, it is called for
all of the device which is defined in namespace. Even if
the parsing device is not memory, it is normal route.
So, this message is useless.

I tested this patch for 2.6.18-rc3 on my box with hot-add emulation.

Please apply.

Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com>
------

 drivers/acpi/acpi_memhotplug.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

Index: hotmemtest1/drivers/acpi/acpi_memhotplug.c
===================================================================
--- hotmemtest1.orig/drivers/acpi/acpi_memhotplug.c	2006-08-03 20:00:58.000000000 +0900
+++ hotmemtest1/drivers/acpi/acpi_memhotplug.c	2006-08-04 21:06:52.000000000 +0900
@@ -487,10 +487,8 @@ acpi_memory_register_notify_handler(acpi
 
 
 	status = is_memory_device(handle);
-	if (ACPI_FAILURE(status)){
-		ACPI_EXCEPTION((AE_INFO, status, "handle is no memory device"));
+	if (ACPI_FAILURE(status))
 		return AE_OK;	/* continue */
-	}
 
 	status = acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
 					     acpi_memory_device_notify, NULL);
@@ -506,10 +504,8 @@ acpi_memory_deregister_notify_handler(ac
 
 
 	status = is_memory_device(handle);
-	if (ACPI_FAILURE(status)){
-		ACPI_EXCEPTION((AE_INFO, status, "handle is no memory device"));
+	if (ACPI_FAILURE(status))
 		return AE_OK;	/* continue */
-	}
 
 	status = acpi_remove_notify_handler(handle,
 					    ACPI_SYSTEM_NOTIFY,

-- 
Yasunori Goto 

[PATCH](memory hotplug) Repost remove useless message at boot time from 2.6.18-rc4.

[Yasunori Goto]

Hello.

I would like to repost this patch to remove noisy useless message at boot
time from 2.6.18-rc4.
(I said "-mm doesn't shows this message in previous post", but it was wrong.
 This messages are shown by -mm too.)

-------------------------
This is to remove noisy useless message at boot time from 2.6.18-rc4.
The message is a ton of
"ACPI Exception (acpi_memory-0492): AE_ERROR, handle is no memory device"

In my emulation, number of memory devices are not so many (only 6),
but, this messages are displayed 114 times.

It is showed by acpi_memory_register_notify_handler() which
is called by acpi_walk_namespace().

acpi_walk_namespace() parses all of ACPI's namespace and 
execute acpi_memory_register_notify_handler(). So, it is called for
all of the device which is defined in namespace. If
the parsing device is not memory, acpi_memhotplug ignores it
due to "no match" and will parse next device.
This is normal route.

But this message says it is exception. It is meaningless.

I tested this patch for 2.6.18-rc4 on my box with hot-add emulation.

Please apply.

Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com>
------


 drivers/acpi/acpi_memhotplug.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

Index: rc4test/drivers/acpi/acpi_memhotplug.c
===================================================================
--- rc4test.orig/drivers/acpi/acpi_memhotplug.c	2006-08-09 15:33:29.000000000 +0900
+++ rc4test/drivers/acpi/acpi_memhotplug.c	2006-08-09 16:26:29.000000000 +0900
@@ -484,10 +484,8 @@ acpi_memory_register_notify_handler(acpi
 
 
 	status = is_memory_device(handle);
-	if (ACPI_FAILURE(status)){
-		ACPI_EXCEPTION((AE_INFO, status, "handle is no memory device"));
+	if (ACPI_FAILURE(status))
 		return AE_OK;	/* continue */
-	}
 
 	status = acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
 					     acpi_memory_device_notify, NULL);
@@ -503,10 +501,8 @@ acpi_memory_deregister_notify_handler(ac
 
 
 	status = is_memory_device(handle);
-	if (ACPI_FAILURE(status)){
-		ACPI_EXCEPTION((AE_INFO, status, "handle is no memory device"));
+	if (ACPI_FAILURE(status))
 		return AE_OK;	/* continue */
-	}
 
 	status = acpi_remove_notify_handler(handle,
 					    ACPI_SYSTEM_NOTIFY,

-- 
Yasunori Goto 

Re: [PATCH](memory hotplug) Repost remove useless message at boot time from 2.6.18-rc4.

[Prarit Bhargava]

Yasunori Goto wrote:
> Hello.
>
> I would like to repost this patch to remove noisy useless message at boot
> time from 2.6.18-rc4.
> (I said "-mm doesn't shows this message in previous post", but it was wrong.
>  This messages are shown by -mm too.)
>
> -------------------------
> This is to remove noisy useless message at boot time from 2.6.18-rc4.
> The message is a ton of
> "ACPI Exception (acpi_memory-0492): AE_ERROR, handle is no memory device"
>
>   
I'm seeing this on some of my ia64 boxes, however, I see

ACPI Exception (acpi_memory-0491): AE_ERROR, handle is no memory device 
[20060707]

What's interesting is that last little bit looks an awful lot like a 
date.... It's almost as if we were
reading beyond the end of the ACPI table?

Still investigating,

P.

Re: [PATCH](memory hotplug) Repost remove useless message at boot time from 2.6.18-rc4.

[Len Brown]

On Thursday 10 August 2006 16:26, Prarit Bhargava wrote:
> 
> Yasunori Goto wrote:
> > Hello.
> >
> > I would like to repost this patch to remove noisy useless message at boot
> > time from 2.6.18-rc4.
> > (I said "-mm doesn't shows this message in previous post", but it was wrong.
> >  This messages are shown by -mm too.)
> >
> > -------------------------
> > This is to remove noisy useless message at boot time from 2.6.18-rc4.
> > The message is a ton of
> > "ACPI Exception (acpi_memory-0492): AE_ERROR, handle is no memory device"
> >
> >   
> I'm seeing this on some of my ia64 boxes, however, I see
> 
> ACPI Exception (acpi_memory-0491): AE_ERROR, handle is no memory device 
> [20060707]
> 
> What's interesting is that last little bit looks an awful lot like a 
> date.... It's almost as if we were
> reading beyond the end of the ACPI table?

[20060707] is the version (yes, it is a date) of the ACPICA core.
The ACPI_EXCEPTION() macro appends it to the exception message.

-Len

Re: [PATCH](memory hotplug) Repost remove useless message at boot time from 2.6.18-rc4.

[Prarit Bhargava]

> [20060707] is the version (yes, it is a date) of the ACPICA core.
> The ACPI_EXCEPTION() macro appends it to the exception message.
>
>   
I just figured that out :) ... Seems okay to me.

P.
> -Len
>   

Re: [PATCH](memory hotplug) Repost remove useless message at boot time from 2.6.18-rc4.

[Thomas Renninger]

On Thu, 2006-08-10 at 14:32 +0900, Yasunori Goto wrote:
> Hello.
> 
> I would like to repost this patch to remove noisy useless message at boot
> time from 2.6.18-rc4.
> (I said "-mm doesn't shows this message in previous post", but it was wrong.
>  This messages are shown by -mm too.)
> 
> -------------------------
> This is to remove noisy useless message at boot time from 2.6.18-rc4.
> The message is a ton of
> "ACPI Exception (acpi_memory-0492): AE_ERROR, handle is no memory device"

I sent a patch a while ago that gets rid of the whole namespace walking
by making acpi_memoryhotplug an acpi device and making use of the .add
callback function and the acpi_bus_register_driver call.

I am not sure whether this is possible if you have multiple memory
devices, though (if not maybe it should be made possible?)...

Yasunori even tested the patch and sent an Ok:
http://marc.theaimsgroup.com/?t=114065312400001&r=1&w=2

If this is acceptable I can rebase the patch on a current kernel.

    Thomas

Re: [PATCH](memory hotplug) Repost remove useless message at boot time from 2.6.18-rc4.

[Yasunori Goto]

> On Thu, 2006-08-10 at 14:32 +0900, Yasunori Goto wrote:
> > Hello.
> > 
> > I would like to repost this patch to remove noisy useless message at boot
> > time from 2.6.18-rc4.
> > (I said "-mm doesn't shows this message in previous post", but it was wrong.
> >  This messages are shown by -mm too.)
> > 
> > -------------------------
> > This is to remove noisy useless message at boot time from 2.6.18-rc4.
> > The message is a ton of
> > "ACPI Exception (acpi_memory-0492): AE_ERROR, handle is no memory device"
> 
> I sent a patch a while ago that gets rid of the whole namespace walking
> by making acpi_memoryhotplug an acpi device and making use of the .add
> callback function and the acpi_bus_register_driver call.
> 
> I am not sure whether this is possible if you have multiple memory
> devices, though (if not maybe it should be made possible?)...
> 
> Yasunori even tested the patch and sent an Ok:
> http://marc.theaimsgroup.com/?t=114065312400001&r=1&w=2
> 
> If this is acceptable I can rebase the patch on a current kernel.

Ahhhhh. Yes. Indeed.
I had forgotten it.

But, I can't test it in this week due to I'm in vacation.
(Now, I'm writting this mail at hotel in vacation.)
Please wait until next week. :-P

Thanks.

-- 
Yasunori Goto 

Re: [PATCH](memory hotplug) Repost remove useless message at boot time from 2.6.18-rc4.

[Yasunori Goto]

> I sent a patch a while ago that gets rid of the whole namespace walking
> by making acpi_memoryhotplug an acpi device and making use of the .add
> callback function and the acpi_bus_register_driver call.
> 
> I am not sure whether this is possible if you have multiple memory
> devices, though (if not maybe it should be made possible?)...
> 
> Yasunori even tested the patch and sent an Ok:
> http://marc.theaimsgroup.com/?t=114065312400001&r=1&w=2
> 
> If this is acceptable I can rebase the patch on a current kernel.

Hi. Thomas-san.
Did you rebase your patch?

I'm trying to do it now too. 
But, current code (2.6.18-rc4) seems to register handler for
only enable status devices at boot time.
So, notification is -discarded- due to no handler for new memory
device when hot-add event occurs. Hmmm. :-(


Bye.

-- 
Yasunori Goto 

Re: [PATCH](memory hotplug) Repost remove useless message at boot time from 2.6.18-rc4.

[Thomas Renninger]

Am Fr 25.08.2006 13:59 schrieb Yasunori Goto <y-goto@jp.fujitsu.com>:
>
>>
>> > I sent a patch a while ago that gets rid of the whole namespace
>> > walking
>> > by making acpi_memoryhotplug an acpi device and making use of the
>> > .add
>> > callback function and the acpi_bus_register_driver call.
>> >
>> > I am not sure whether this is possible if you have multiple memory
>> > devices, though (if not maybe it should be made possible?)...
>> >
>> > Yasunori even tested the patch and sent an Ok:
>> > http://marc.theaimsgroup.com/?t=114065312400001&r=1&w=2
>> >
>> > If this is acceptable I can rebase the patch on a current kernel.
>>
>> Hi. Thomas-san.
>> Did you rebase your patch?
>>
>> I'm trying to do it now too.
>> But, current code (2.6.18-rc4) seems to register handler for
>> only enable status devices at boot time.
>> So, notification is -discarded- due to no handler for new memory
>> device when hot-add event occurs. Hmmm. :-(
>No, what I see the notify handler is always installed.
>But there seem to be added some device state code, which I think is not
>correct:
>E.g. the state in acpi_memory_device_add():
>    mem_device->state = MEMORY_POWER_ON_STATE;
>should be evaluated from _STA function.
> When an ACPI memory device is added, the physical memory might not
>be added yet... IMO only one state, the one evaluated from _STA should
>be used.
>I'll have a closer look.
>
>This probably is nothing for 2.6.18 ... you never know..., but maybe it
>can be added
>to ACPI test branch or mm?
>Patch is against 2.6.18-rc4 and only compile
>tested (don't have hardware to test).
>
>The attached patch moves the install/remove notify handler to the
>.add/.remove callback functions that get invoked for every ACPI memory
>device
>(like it is done in other ACPI modules).
>No need for an additional walk_namespace() call.
>
>Sorry, I can only attach patch because of webmailer...
>

[PATCH 2/2] ACPI memory_hotplug cleanups

[Thomas Renninger]

[-- Attachment #1: Type: text/plain, Size: 1466 bytes --]

Am Fr 25.08.2006 13:59 schrieb Yasunori Goto <y-goto@jp.fujitsu.com>:
>
>>
>> > I sent a patch a while ago that gets rid of the whole namespace
>> > walking
>> > by making acpi_memoryhotplug an acpi device and making use of the
>> > .add
>> > callback function and the acpi_bus_register_driver call.
>> >
>> > I am not sure whether this is possible if you have multiple memory
>> > devices, though (if not maybe it should be made possible?)...
>> >
>> > Yasunori even tested the patch and sent an Ok:
>> > http://marc.theaimsgroup.com/?t=114065312400001&r=1&w=2
>> >
>> > If this is acceptable I can rebase the patch on a current kernel.
>>
>> Hi. Thomas-san.
>> Did you rebase your patch?
>>
>> I'm trying to do it now too.
>> But, current code (2.6.18-rc4) seems to register handler for
>> only enable status devices at boot time.
>> So, notification is -discarded- due to no handler for new memory
>> device when hot-add event occurs. Hmmm. :-(
>>
> Looking a bit deeper into this I think some more things can be
> optimised.
>The author's email address also seems to be invalid
>(naveen.b.s@intel.com)?
>
>More cleanups attached (on top of the last one).
>Again only compile tested and patch attached not inlined
>
>(I forgot to upload the patch in my previous mail? Will resend...).
>
>BTW: I will be on holidays the next days, expect that it may take
>some time until I answer...
>
>    Thomas
>     Thomas
>

[-- Attachment #2: acpi_memory_hotplug_state_cleanup.patch --]
[-- Type: application/octet-stream, Size: 8931 bytes --]

acpi_memory_hotplug cleanups [2/2]

 - get rid of unused acpi_memory_device->state (is set, but never evaluated)
 - Make use of global acpi_bus_get_status(device) func instead of evaluate
   _STA with own func -> gets rid of acpi_memory_check_device and _STA defines
 - pass mem_device pointer as callback data to notify handler func
   -> get rid of acpi_memory_get_device()
 - check for availabe _STA and _EJ0/EJD early in acpi_memory_device_add(),
   those are mandotary for a working memory device

Signed-off-by: Thomas Renninger <mail@renninger.de>

 acpi_memhotplug.c |  155 +++++++++++-------------------------------------------
 1 file changed, 34 insertions(+), 121 deletions(-)

Index: linux-2.6.18-rc4/drivers/acpi/acpi_memhotplug.c
===================================================================
--- linux-2.6.18-rc4.orig/drivers/acpi/acpi_memhotplug.c
+++ linux-2.6.18-rc4/drivers/acpi/acpi_memhotplug.c
@@ -45,16 +45,6 @@ ACPI_MODULE_NAME("acpi_memory")
 MODULE_DESCRIPTION(ACPI_MEMORY_DEVICE_DRIVER_NAME);
 MODULE_LICENSE("GPL");
 
-/* ACPI _STA method values */
-#define ACPI_MEMORY_STA_PRESENT		(0x00000001UL)
-#define ACPI_MEMORY_STA_ENABLED		(0x00000002UL)
-#define ACPI_MEMORY_STA_FUNCTIONAL	(0x00000008UL)
-
-/* Memory Device States */
-#define MEMORY_INVALID_STATE	0
-#define MEMORY_POWER_ON_STATE	1
-#define MEMORY_POWER_OFF_STATE	2
-
 static int acpi_memory_device_add(struct acpi_device *device);
 static int acpi_memory_device_remove(struct acpi_device *device, int type);
 static int acpi_memory_device_start(struct acpi_device *device);
@@ -81,7 +71,6 @@ struct acpi_memory_info {
 
 struct acpi_memory_device {
 	struct acpi_device * device;
-	unsigned int state;	/* State of the memory device */
 	struct list_head res_list;
 };
 
@@ -144,73 +133,6 @@ acpi_memory_get_device_resources(struct 
 	return 0;
 }
 
-static int
-acpi_memory_get_device(acpi_handle handle,
-		       struct acpi_memory_device **mem_device)
-{
-	acpi_status status;
-	acpi_handle phandle;
-	struct acpi_device *device = NULL;
-	struct acpi_device *pdevice = NULL;
-
-
-	if (!acpi_bus_get_device(handle, &device) && device)
-		goto end;
-
-	status = acpi_get_parent(handle, &phandle);
-	if (ACPI_FAILURE(status)) {
-		ACPI_EXCEPTION((AE_INFO, status, "Cannot find acpi parent"));
-		return -EINVAL;
-	}
-
-	/* Get the parent device */
-	status = acpi_bus_get_device(phandle, &pdevice);
-	if (ACPI_FAILURE(status)) {
-		ACPI_EXCEPTION((AE_INFO, status, "Cannot get acpi bus device"));
-		return -EINVAL;
-	}
-
-	/*
-	 * Now add the notified device.  This creates the acpi_device
-	 * and invokes .add function
-	 */
-	status = acpi_bus_add(&device, pdevice, handle, ACPI_BUS_TYPE_DEVICE);
-	if (ACPI_FAILURE(status)) {
-		ACPI_EXCEPTION((AE_INFO, status, "Cannot add acpi bus"));
-		return -EINVAL;
-	}
-
-      end:
-	*mem_device = acpi_driver_data(device);
-	if (!(*mem_device)) {
-		printk(KERN_ERR "\n driver data not found");
-		return -ENODEV;
-	}
-
-	return 0;
-}
-
-static int acpi_memory_check_device(struct acpi_memory_device *mem_device)
-{
-	unsigned long current_status;
-
-
-	/* Get device present/absent information from the _STA */
-	if (ACPI_FAILURE(acpi_evaluate_integer(mem_device->device->handle, "_STA",
-					       NULL, &current_status)))
-		return -ENODEV;
-	/*
-	 * Check for device status. Device should be
-	 * present/enabled/functioning.
-	 */
-	if (!((current_status & ACPI_MEMORY_STA_PRESENT)
-	      && (current_status & ACPI_MEMORY_STA_ENABLED)
-	      && (current_status & ACPI_MEMORY_STA_FUNCTIONAL)))
-		return -ENODEV;
-
-	return 0;
-}
-
 static int acpi_memory_enable_device(struct acpi_memory_device *mem_device)
 {
 	int result, num_enabled = 0;
@@ -222,7 +144,6 @@ static int acpi_memory_enable_device(str
 	result = acpi_memory_get_device_resources(mem_device);
 	if (result) {
 		printk(KERN_ERR PREFIX "get_device_resources failed\n");
-		mem_device->state = MEMORY_INVALID_STATE;
 		return result;
 	}
 
@@ -246,7 +167,6 @@ static int acpi_memory_enable_device(str
 	}
 	if (!num_enabled) {
 		printk(KERN_ERR PREFIX "add_memory failed\n");
-		mem_device->state = MEMORY_INVALID_STATE;
 		return -EINVAL;
 	}
 
@@ -257,16 +177,15 @@ static int acpi_memory_powerdown_device(
 {
 	acpi_status status;
 	struct acpi_object_list arg_list;
+	struct acpi_device *device = mem_device->device;
 	union acpi_object arg;
-	unsigned 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,
+	status = acpi_evaluate_object(device->handle,
 				      "_EJ0", &arg_list, NULL);
 	/* Return on _EJ0 failure */
 	if (ACPI_FAILURE(status)) {
@@ -275,15 +194,14 @@ static int acpi_memory_powerdown_device(
 	}
 
 	/* Evalute _STA to check if the device is disabled */
-	status = acpi_evaluate_integer(mem_device->device->handle, "_STA",
-				       NULL, &current_status);
-	if (ACPI_FAILURE(status))
-		return -ENODEV;
-
+	acpi_bus_get_status(device);
 	/* Check for device status.  Device should be disabled */
-	if (current_status & ACPI_MEMORY_STA_ENABLED)
+	if (device->status.enabled){
+		printk (KERN_ERR PREFIX "Could not powerdown memory"
+			"device %s",
+			acpi_device_bid(device));
 		return -EINVAL;
-
+	}
 	return 0;
 }
 
@@ -308,21 +226,20 @@ static int acpi_memory_disable_device(st
 
 	/* 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;
+	if (result)
 		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_memory_device *mem_device = (struct acpi_memory_device*) data;
 	struct acpi_device *device;
 
+	if (!mem_device || !mem_device->device)
+		return;
+	device = mem_device->device;
 
 	switch (event) {
 	case ACPI_NOTIFY_BUS_CHECK:
@@ -333,31 +250,20 @@ static void acpi_memory_device_notify(ac
 		if (event == ACPI_NOTIFY_DEVICE_CHECK)
 			ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 					  "\nReceived DEVICE CHECK notification for device\n"));
-		if (acpi_memory_get_device(handle, &mem_device)) {
-			printk(KERN_ERR PREFIX "Cannot find driver data\n");
-			return;
-		}
 
-		if (!acpi_memory_check_device(mem_device)) {
+		acpi_bus_get_status(device);
+		if (!(device->status.present &&
+		      device->status.enabled &&
+		      device->status.functional)){
 			if (acpi_memory_enable_device(mem_device))
 				printk(KERN_ERR PREFIX
-					    "Cannot enable memory device\n");
+				       "Cannot enable memory device\n");
 		}
 		break;
 	case ACPI_NOTIFY_EJECT_REQUEST:
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 				  "\nReceived EJECT REQUEST notification for device\n"));
 
-		if (acpi_bus_get_device(handle, &device)) {
-			printk(KERN_ERR PREFIX "Device doesn't exist\n");
-			break;
-		}
-		mem_device = acpi_driver_data(device);
-		if (!mem_device) {
-			printk(KERN_ERR PREFIX "Driver Data is NULL\n");
-			break;
-		}
-
 		/*
 		 * Currently disabling memory device from kernel mode
 		 * TBD: Can also be disabled from user mode scripts
@@ -390,6 +296,13 @@ static int acpi_memory_device_add(struct
 	if (!device)
 		return -EINVAL;
 
+	/* Check for _STA and EJ0 func */
+	if (!device->flags.dynamic_status || !device->flags.ejectable){
+		printk(KERN_INFO PREFIX "Memory device %s has no _STA or"
+		       "EJ0/EJD function", acpi_device_bid(device));
+		return -ENODEV;
+	}
+
 	mem_device = kmalloc(sizeof(struct acpi_memory_device), GFP_KERNEL);
 	if (!mem_device)
 		return -ENOMEM;
@@ -410,15 +323,13 @@ static int acpi_memory_device_add(struct
 
 	/* register notify handler */
 	status = acpi_install_notify_handler(device->handle, ACPI_SYSTEM_NOTIFY,
-					     acpi_memory_device_notify, NULL);
+					     acpi_memory_device_notify, mem_device);
 
 	if (ACPI_FAILURE(status)){
 		ACPI_EXCEPTION((AE_INFO, status, "Could not install notify"
 				"handler for memory device: %s",
 				acpi_device_bid(device)));
 	}
-	/* Set the device state */
-	mem_device->state = MEMORY_POWER_ON_STATE;
 
 	printk(KERN_INFO "%s \n", acpi_device_name(device));
 
@@ -450,13 +361,15 @@ static int acpi_memory_device_start (str
 
 	mem_device = acpi_driver_data(device);
 
-	if (!acpi_memory_check_device(mem_device)) {
-		/* call add_memory func */
-		result = acpi_memory_enable_device(mem_device);
-		if (result)
-			ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
-				"Error in acpi_memory_enable_device\n"));
+	acpi_bus_get_status(device);
+	if (!(device->status.present &&
+	      device->status.enabled &&
+	      device->status.functional)){
+		if (acpi_memory_enable_device(mem_device))
+			printk(KERN_ERR PREFIX
+			       "Error in acpi_memory_enable_device\n");
 	}
+
 	return result;
 }
 

[PATCH 1/2] acpi hotplug cleanups, move install notifier to add function

[Thomas Renninger]

[-- Attachment #1: Type: text/plain, Size: 5432 bytes --]

On Fri, 2006-08-25 at 20:59 +0900, Yasunori Goto wrote:
> > I sent a patch a while ago that gets rid of the whole namespace walking
> > by making acpi_memoryhotplug an acpi device and making use of the .add
> > callback function and the acpi_bus_register_driver call.
> > 
> > I am not sure whether this is possible if you have multiple memory
> > devices, though (if not maybe it should be made possible?)...
> > 
> > Yasunori even tested the patch and sent an Ok:
> > http://marc.theaimsgroup.com/?t=114065312400001&r=1&w=2
> > 
> > If this is acceptable I can rebase the patch on a current kernel.
> 
> Hi. Thomas-san.
> Did you rebase your patch?
> 
> I'm trying to do it now too. 
> But, current code (2.6.18-rc4) seems to register handler for
> only enable status devices at boot time.
> So, notification is -discarded- due to no handler for new memory
> device when hot-add event occurs. Hmmm. :-(

Trying again with a real mailer, sorry about the previous junk ...
The email address of the module author (naveen.b.s@intel.com) seems to
be invalid?

    Thomas


ACPI memhotplug cleanup: Move "install notify handler" to .add() function

Signed-off-by: Thomas Renninger <mail@renninger.de>

 acpi_memhotplug.c |  113 +++++++-----------------------------------------------
 1 file changed, 16 insertions(+), 97 deletions(-)

Index: linux-2.6.18-rc4/drivers/acpi/acpi_memhotplug.c
===================================================================
--- linux-2.6.18-rc4.orig/drivers/acpi/acpi_memhotplug.c
+++ linux-2.6.18-rc4/drivers/acpi/acpi_memhotplug.c
@@ -384,6 +384,7 @@ static int acpi_memory_device_add(struct
 {
 	int result;
 	struct acpi_memory_device *mem_device = NULL;
+	acpi_status status;
 

 	if (!device)
@@ -407,6 +408,15 @@ static int acpi_memory_device_add(struct
 		return result;
 	}
 
+	/* register notify handler */
+	status = acpi_install_notify_handler(device->handle, ACPI_SYSTEM_NOTIFY,
+					     acpi_memory_device_notify, NULL);
+
+	if (ACPI_FAILURE(status)){
+		ACPI_EXCEPTION((AE_INFO, status, "Could not install notify"
+				"handler for memory device: %s",
+				acpi_device_bid(device)));
+	}
 	/* Set the device state */
 	mem_device->state = MEMORY_POWER_ON_STATE;
 
@@ -423,6 +433,10 @@ static int acpi_memory_device_remove(str
 	if (!device || !acpi_driver_data(device))
 		return -EINVAL;
 
+	acpi_remove_notify_handler(device->handle,
+				   ACPI_SYSTEM_NOTIFY,
+				   acpi_memory_device_notify);
+
 	mem_device = (struct acpi_memory_device *)acpi_driver_data(device);
 	kfree(mem_device);
 
@@ -446,117 +460,22 @@ static int acpi_memory_device_start (str
 	return result;
 }
 
-/*
- * Helper function to check for memory device
- */
-static acpi_status is_memory_device(acpi_handle handle)
-{
-	char *hardware_id;
-	acpi_status status;
-	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
-	struct acpi_device_info *info;
-
-
-	status = acpi_get_object_info(handle, &buffer);
-	if (ACPI_FAILURE(status))
-		return status;
-
-	info = buffer.pointer;
-	if (!(info->valid & ACPI_VALID_HID)) {
-		kfree(buffer.pointer);
-		return AE_ERROR;
-	}
-
-	hardware_id = info->hardware_id.value;
-	if ((hardware_id == NULL) ||
-	    (strcmp(hardware_id, ACPI_MEMORY_DEVICE_HID)))
-		status = AE_ERROR;
-
-	kfree(buffer.pointer);
-	return status;
-}
-
-static acpi_status
-acpi_memory_register_notify_handler(acpi_handle handle,
-				    u32 level, void *ctxt, void **retv)
-{
-	acpi_status status;
-
-
-	status = is_memory_device(handle);
-	if (ACPI_FAILURE(status)){
-		ACPI_EXCEPTION((AE_INFO, status, "handle is no memory device"));
-		return AE_OK;	/* continue */
-	}
-
-	status = acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
-					     acpi_memory_device_notify, NULL);
-	/* continue */
-	return AE_OK;
-}
-
-static acpi_status
-acpi_memory_deregister_notify_handler(acpi_handle handle,
-				      u32 level, void *ctxt, void **retv)
-{
-	acpi_status status;
-
-
-	status = is_memory_device(handle);
-	if (ACPI_FAILURE(status)){
-		ACPI_EXCEPTION((AE_INFO, status, "handle is no memory device"));
-		return AE_OK;	/* continue */
-	}
-
-	status = acpi_remove_notify_handler(handle,
-					    ACPI_SYSTEM_NOTIFY,
-					    acpi_memory_device_notify);
-
-	return AE_OK;	/* continue */
-}
-
 static int __init acpi_memory_device_init(void)
 {
 	int result;
-	acpi_status status;
-
 
+	/* Register driver and notify handlers in .add func */
 	result = acpi_bus_register_driver(&acpi_memory_device_driver);
 
 	if (result < 0)
 		return -ENODEV;
 
-	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
-				     ACPI_UINT32_MAX,
-				     acpi_memory_register_notify_handler,
-				     NULL, NULL);
-
-	if (ACPI_FAILURE(status)) {
-		ACPI_EXCEPTION((AE_INFO, status, "walk_namespace failed"));
-		acpi_bus_unregister_driver(&acpi_memory_device_driver);
-		return -ENODEV;
-	}
-
 	return 0;
 }
 
 static void __exit acpi_memory_device_exit(void)
 {
-	acpi_status status;
-
-
-	/*
-	 * Adding this to un-install notification handlers for all the device
-	 * handles.
-	 */
-	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
-				     ACPI_UINT32_MAX,
-				     acpi_memory_deregister_notify_handler,
-				     NULL, NULL);
-
-	if (ACPI_FAILURE(status))
-		ACPI_EXCEPTION((AE_INFO, status, "walk_namespace failed"));
-
+	/* Unregister driver and notify handlers in .add func */
 	acpi_bus_unregister_driver(&acpi_memory_device_driver);
 
 	return;


[-- Attachment #2: acpi_memhotplug_cleanup.patch --]
[-- Type: text/x-patch, Size: 4362 bytes --]

ACPI memhotplug cleanup: Move "install notify handler" to .add() function

Signed-off-by: Thomas Renninger <mail@renninger.de>

 acpi_memhotplug.c |  113 +++++++-----------------------------------------------
 1 file changed, 16 insertions(+), 97 deletions(-)

Index: linux-2.6.18-rc4/drivers/acpi/acpi_memhotplug.c
===================================================================
--- linux-2.6.18-rc4.orig/drivers/acpi/acpi_memhotplug.c
+++ linux-2.6.18-rc4/drivers/acpi/acpi_memhotplug.c
@@ -384,6 +384,7 @@ static int acpi_memory_device_add(struct
 {
 	int result;
 	struct acpi_memory_device *mem_device = NULL;
+	acpi_status status;
 
 
 	if (!device)
@@ -407,6 +408,15 @@ static int acpi_memory_device_add(struct
 		return result;
 	}
 
+	/* register notify handler */
+	status = acpi_install_notify_handler(device->handle, ACPI_SYSTEM_NOTIFY,
+					     acpi_memory_device_notify, NULL);
+
+	if (ACPI_FAILURE(status)){
+		ACPI_EXCEPTION((AE_INFO, status, "Could not install notify"
+				"handler for memory device: %s",
+				acpi_device_bid(device)));
+	}
 	/* Set the device state */
 	mem_device->state = MEMORY_POWER_ON_STATE;
 
@@ -423,6 +433,10 @@ static int acpi_memory_device_remove(str
 	if (!device || !acpi_driver_data(device))
 		return -EINVAL;
 
+	acpi_remove_notify_handler(device->handle,
+				   ACPI_SYSTEM_NOTIFY,
+				   acpi_memory_device_notify);
+
 	mem_device = (struct acpi_memory_device *)acpi_driver_data(device);
 	kfree(mem_device);
 
@@ -446,117 +460,22 @@ static int acpi_memory_device_start (str
 	return result;
 }
 
-/*
- * Helper function to check for memory device
- */
-static acpi_status is_memory_device(acpi_handle handle)
-{
-	char *hardware_id;
-	acpi_status status;
-	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
-	struct acpi_device_info *info;
-
-
-	status = acpi_get_object_info(handle, &buffer);
-	if (ACPI_FAILURE(status))
-		return status;
-
-	info = buffer.pointer;
-	if (!(info->valid & ACPI_VALID_HID)) {
-		kfree(buffer.pointer);
-		return AE_ERROR;
-	}
-
-	hardware_id = info->hardware_id.value;
-	if ((hardware_id == NULL) ||
-	    (strcmp(hardware_id, ACPI_MEMORY_DEVICE_HID)))
-		status = AE_ERROR;
-
-	kfree(buffer.pointer);
-	return status;
-}
-
-static acpi_status
-acpi_memory_register_notify_handler(acpi_handle handle,
-				    u32 level, void *ctxt, void **retv)
-{
-	acpi_status status;
-
-
-	status = is_memory_device(handle);
-	if (ACPI_FAILURE(status)){
-		ACPI_EXCEPTION((AE_INFO, status, "handle is no memory device"));
-		return AE_OK;	/* continue */
-	}
-
-	status = acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
-					     acpi_memory_device_notify, NULL);
-	/* continue */
-	return AE_OK;
-}
-
-static acpi_status
-acpi_memory_deregister_notify_handler(acpi_handle handle,
-				      u32 level, void *ctxt, void **retv)
-{
-	acpi_status status;
-
-
-	status = is_memory_device(handle);
-	if (ACPI_FAILURE(status)){
-		ACPI_EXCEPTION((AE_INFO, status, "handle is no memory device"));
-		return AE_OK;	/* continue */
-	}
-
-	status = acpi_remove_notify_handler(handle,
-					    ACPI_SYSTEM_NOTIFY,
-					    acpi_memory_device_notify);
-
-	return AE_OK;	/* continue */
-}
-
 static int __init acpi_memory_device_init(void)
 {
 	int result;
-	acpi_status status;
-
 
+	/* Register driver and notify handlers in .add func */
 	result = acpi_bus_register_driver(&acpi_memory_device_driver);
 
 	if (result < 0)
 		return -ENODEV;
 
-	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
-				     ACPI_UINT32_MAX,
-				     acpi_memory_register_notify_handler,
-				     NULL, NULL);
-
-	if (ACPI_FAILURE(status)) {
-		ACPI_EXCEPTION((AE_INFO, status, "walk_namespace failed"));
-		acpi_bus_unregister_driver(&acpi_memory_device_driver);
-		return -ENODEV;
-	}
-
 	return 0;
 }
 
 static void __exit acpi_memory_device_exit(void)
 {
-	acpi_status status;
-
-
-	/*
-	 * Adding this to un-install notification handlers for all the device
-	 * handles.
-	 */
-	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
-				     ACPI_UINT32_MAX,
-				     acpi_memory_deregister_notify_handler,
-				     NULL, NULL);
-
-	if (ACPI_FAILURE(status))
-		ACPI_EXCEPTION((AE_INFO, status, "walk_namespace failed"));
-
+	/* Unregister driver and notify handlers in .add func */
 	acpi_bus_unregister_driver(&acpi_memory_device_driver);
 
 	return;

[PATCH 2/2] acpi hotplug cleanups, move install notifier to add function

[Thomas Renninger]

[-- Attachment #1: Type: text/plain, Size: 9930 bytes --]

On Fri, 2006-08-25 at 20:59 +0900, Yasunori Goto wrote:
> > I sent a patch a while ago that gets rid of the whole namespace walking
> > by making acpi_memoryhotplug an acpi device and making use of the .add
> > callback function and the acpi_bus_register_driver call.
> > 
> > I am not sure whether this is possible if you have multiple memory
> > devices, though (if not maybe it should be made possible?)...
> > 
> > Yasunori even tested the patch and sent an Ok:
> > http://marc.theaimsgroup.com/?t=114065312400001&r=1&w=2
> > 
> > If this is acceptable I can rebase the patch on a current kernel.
> 
> Hi. Thomas-san.
> Did you rebase your patch?
> 
> I'm trying to do it now too. 
> But, current code (2.6.18-rc4) seems to register handler for
> only enable status devices at boot time.
> So, notification is -discarded- due to no handler for new memory
> device when hot-add event occurs. Hmmm. :-(

Some more cleanups (on top of the last one)...
Compile tested, patched against 2.6.18-rc4.

acpi_memory_hotplug cleanups [2/2]

 - get rid of unused acpi_memory_device->state (is set, but never evaluated)
 - Make use of global acpi_bus_get_status(device) func instead of evaluate
   _STA with own func -> gets rid of acpi_memory_check_device and _STA defines
 - pass mem_device pointer as callback data to notify handler func
   -> get rid of acpi_memory_get_device()
 - check for availabe _STA and _EJ0/EJD early in acpi_memory_device_add(),
   those are mandotary for a working memory device

Signed-off-by: Thomas Renninger <mail@renninger.de>

 acpi_memhotplug.c |  155 +++++++++++-------------------------------------------
 1 file changed, 34 insertions(+), 121 deletions(-)

Index: linux-2.6.18-rc4/drivers/acpi/acpi_memhotplug.c
===================================================================
--- linux-2.6.18-rc4.orig/drivers/acpi/acpi_memhotplug.c
+++ linux-2.6.18-rc4/drivers/acpi/acpi_memhotplug.c
@@ -45,16 +45,6 @@ ACPI_MODULE_NAME("acpi_memory")
 MODULE_DESCRIPTION(ACPI_MEMORY_DEVICE_DRIVER_NAME);
 MODULE_LICENSE("GPL");
 
-/* ACPI _STA method values */
-#define ACPI_MEMORY_STA_PRESENT		(0x00000001UL)
-#define ACPI_MEMORY_STA_ENABLED		(0x00000002UL)
-#define ACPI_MEMORY_STA_FUNCTIONAL	(0x00000008UL)
-
-/* Memory Device States */
-#define MEMORY_INVALID_STATE	0
-#define MEMORY_POWER_ON_STATE	1
-#define MEMORY_POWER_OFF_STATE	2
-
 static int acpi_memory_device_add(struct acpi_device *device);
 static int acpi_memory_device_remove(struct acpi_device *device, int type);
 static int acpi_memory_device_start(struct acpi_device *device);
@@ -81,7 +71,6 @@ struct acpi_memory_info {
 
 struct acpi_memory_device {
 	struct acpi_device * device;
-	unsigned int state;	/* State of the memory device */
 	struct list_head res_list;
 };
 
@@ -144,73 +133,6 @@ acpi_memory_get_device_resources(struct 
 	return 0;
 }
 
-static int
-acpi_memory_get_device(acpi_handle handle,
-		       struct acpi_memory_device **mem_device)
-{
-	acpi_status status;
-	acpi_handle phandle;
-	struct acpi_device *device = NULL;
-	struct acpi_device *pdevice = NULL;
-
-
-	if (!acpi_bus_get_device(handle, &device) && device)
-		goto end;
-
-	status = acpi_get_parent(handle, &phandle);
-	if (ACPI_FAILURE(status)) {
-		ACPI_EXCEPTION((AE_INFO, status, "Cannot find acpi parent"));
-		return -EINVAL;
-	}
-
-	/* Get the parent device */
-	status = acpi_bus_get_device(phandle, &pdevice);
-	if (ACPI_FAILURE(status)) {
-		ACPI_EXCEPTION((AE_INFO, status, "Cannot get acpi bus device"));
-		return -EINVAL;
-	}
-
-	/*
-	 * Now add the notified device.  This creates the acpi_device
-	 * and invokes .add function
-	 */
-	status = acpi_bus_add(&device, pdevice, handle, ACPI_BUS_TYPE_DEVICE);
-	if (ACPI_FAILURE(status)) {
-		ACPI_EXCEPTION((AE_INFO, status, "Cannot add acpi bus"));
-		return -EINVAL;
-	}
-
-      end:
-	*mem_device = acpi_driver_data(device);
-	if (!(*mem_device)) {
-		printk(KERN_ERR "\n driver data not found");
-		return -ENODEV;
-	}
-
-	return 0;
-}
-
-static int acpi_memory_check_device(struct acpi_memory_device *mem_device)
-{
-	unsigned long current_status;
-
-
-	/* Get device present/absent information from the _STA */
-	if (ACPI_FAILURE(acpi_evaluate_integer(mem_device->device->handle, "_STA",
-					       NULL, &current_status)))
-		return -ENODEV;
-	/*
-	 * Check for device status. Device should be
-	 * present/enabled/functioning.
-	 */
-	if (!((current_status & ACPI_MEMORY_STA_PRESENT)
-	      && (current_status & ACPI_MEMORY_STA_ENABLED)
-	      && (current_status & ACPI_MEMORY_STA_FUNCTIONAL)))
-		return -ENODEV;
-
-	return 0;
-}
-
 static int acpi_memory_enable_device(struct acpi_memory_device *mem_device)
 {
 	int result, num_enabled = 0;
@@ -222,7 +144,6 @@ static int acpi_memory_enable_device(str
 	result = acpi_memory_get_device_resources(mem_device);
 	if (result) {
 		printk(KERN_ERR PREFIX "get_device_resources failed\n");
-		mem_device->state = MEMORY_INVALID_STATE;
 		return result;
 	}
 
@@ -246,7 +167,6 @@ static int acpi_memory_enable_device(str
 	}
 	if (!num_enabled) {
 		printk(KERN_ERR PREFIX "add_memory failed\n");
-		mem_device->state = MEMORY_INVALID_STATE;
 		return -EINVAL;
 	}
 
@@ -257,16 +177,15 @@ static int acpi_memory_powerdown_device(
 {
 	acpi_status status;
 	struct acpi_object_list arg_list;
+	struct acpi_device *device = mem_device->device;
 	union acpi_object arg;
-	unsigned 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,
+	status = acpi_evaluate_object(device->handle,
 				      "_EJ0", &arg_list, NULL);
 	/* Return on _EJ0 failure */
 	if (ACPI_FAILURE(status)) {
@@ -275,15 +194,14 @@ static int acpi_memory_powerdown_device(
 	}
 
 	/* Evalute _STA to check if the device is disabled */
-	status = acpi_evaluate_integer(mem_device->device->handle, "_STA",
-				       NULL, &current_status);
-	if (ACPI_FAILURE(status))
-		return -ENODEV;
-
+	acpi_bus_get_status(device);
 	/* Check for device status.  Device should be disabled */
-	if (current_status & ACPI_MEMORY_STA_ENABLED)
+	if (device->status.enabled){
+		printk (KERN_ERR PREFIX "Could not powerdown memory"
+			"device %s",
+			acpi_device_bid(device));
 		return -EINVAL;
-
+	}
 	return 0;
 }
 
@@ -308,21 +226,20 @@ static int acpi_memory_disable_device(st
 
 	/* 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;
+	if (result)
 		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_memory_device *mem_device = (struct acpi_memory_device*) data;
 	struct acpi_device *device;
 
+	if (!mem_device || !mem_device->device)
+		return;
+	device = mem_device->device;
 
 	switch (event) {
 	case ACPI_NOTIFY_BUS_CHECK:
@@ -333,31 +250,20 @@ static void acpi_memory_device_notify(ac
 		if (event == ACPI_NOTIFY_DEVICE_CHECK)
 			ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 					  "\nReceived DEVICE CHECK notification for device\n"));
-		if (acpi_memory_get_device(handle, &mem_device)) {
-			printk(KERN_ERR PREFIX "Cannot find driver data\n");
-			return;
-		}
 
-		if (!acpi_memory_check_device(mem_device)) {
+		acpi_bus_get_status(device);
+		if (!(device->status.present &&
+		      device->status.enabled &&
+		      device->status.functional)){
 			if (acpi_memory_enable_device(mem_device))
 				printk(KERN_ERR PREFIX
-					    "Cannot enable memory device\n");
+				       "Cannot enable memory device\n");
 		}
 		break;
 	case ACPI_NOTIFY_EJECT_REQUEST:
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 				  "\nReceived EJECT REQUEST notification for device\n"));
 
-		if (acpi_bus_get_device(handle, &device)) {
-			printk(KERN_ERR PREFIX "Device doesn't exist\n");
-			break;
-		}
-		mem_device = acpi_driver_data(device);
-		if (!mem_device) {
-			printk(KERN_ERR PREFIX "Driver Data is NULL\n");
-			break;
-		}
-
 		/*
 		 * Currently disabling memory device from kernel mode
 		 * TBD: Can also be disabled from user mode scripts
@@ -390,6 +296,13 @@ static int acpi_memory_device_add(struct
 	if (!device)
 		return -EINVAL;
 
+	/* Check for _STA and EJ0 func */
+	if (!device->flags.dynamic_status || !device->flags.ejectable){
+		printk(KERN_INFO PREFIX "Memory device %s has no _STA or"
+		       "EJ0/EJD function", acpi_device_bid(device));
+		return -ENODEV;
+	}
+
 	mem_device = kmalloc(sizeof(struct acpi_memory_device), GFP_KERNEL);
 	if (!mem_device)
 		return -ENOMEM;
@@ -410,15 +323,13 @@ static int acpi_memory_device_add(struct
 
 	/* register notify handler */
 	status = acpi_install_notify_handler(device->handle, ACPI_SYSTEM_NOTIFY,
-					     acpi_memory_device_notify, NULL);
+					     acpi_memory_device_notify, mem_device);
 
 	if (ACPI_FAILURE(status)){
 		ACPI_EXCEPTION((AE_INFO, status, "Could not install notify"
 				"handler for memory device: %s",
 				acpi_device_bid(device)));
 	}
-	/* Set the device state */
-	mem_device->state = MEMORY_POWER_ON_STATE;
 
 	printk(KERN_INFO "%s \n", acpi_device_name(device));
 
@@ -450,13 +361,15 @@ static int acpi_memory_device_start (str
 
 	mem_device = acpi_driver_data(device);
 
-	if (!acpi_memory_check_device(mem_device)) {
-		/* call add_memory func */
-		result = acpi_memory_enable_device(mem_device);
-		if (result)
-			ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
-				"Error in acpi_memory_enable_device\n"));
+	acpi_bus_get_status(device);
+	if (!(device->status.present &&
+	      device->status.enabled &&
+	      device->status.functional)){
+		if (acpi_memory_enable_device(mem_device))
+			printk(KERN_ERR PREFIX
+			       "Error in acpi_memory_enable_device\n");
 	}
+
 	return result;
 }
 


[-- Attachment #2: acpi_memory_hotplug_state_cleanup.patch --]
[-- Type: text/x-patch, Size: 8931 bytes --]

acpi_memory_hotplug cleanups [2/2]

 - get rid of unused acpi_memory_device->state (is set, but never evaluated)
 - Make use of global acpi_bus_get_status(device) func instead of evaluate
   _STA with own func -> gets rid of acpi_memory_check_device and _STA defines
 - pass mem_device pointer as callback data to notify handler func
   -> get rid of acpi_memory_get_device()
 - check for availabe _STA and _EJ0/EJD early in acpi_memory_device_add(),
   those are mandotary for a working memory device

Signed-off-by: Thomas Renninger <mail@renninger.de>

 acpi_memhotplug.c |  155 +++++++++++-------------------------------------------
 1 file changed, 34 insertions(+), 121 deletions(-)

Index: linux-2.6.18-rc4/drivers/acpi/acpi_memhotplug.c
===================================================================
--- linux-2.6.18-rc4.orig/drivers/acpi/acpi_memhotplug.c
+++ linux-2.6.18-rc4/drivers/acpi/acpi_memhotplug.c
@@ -45,16 +45,6 @@ ACPI_MODULE_NAME("acpi_memory")
 MODULE_DESCRIPTION(ACPI_MEMORY_DEVICE_DRIVER_NAME);
 MODULE_LICENSE("GPL");
 
-/* ACPI _STA method values */
-#define ACPI_MEMORY_STA_PRESENT		(0x00000001UL)
-#define ACPI_MEMORY_STA_ENABLED		(0x00000002UL)
-#define ACPI_MEMORY_STA_FUNCTIONAL	(0x00000008UL)
-
-/* Memory Device States */
-#define MEMORY_INVALID_STATE	0
-#define MEMORY_POWER_ON_STATE	1
-#define MEMORY_POWER_OFF_STATE	2
-
 static int acpi_memory_device_add(struct acpi_device *device);
 static int acpi_memory_device_remove(struct acpi_device *device, int type);
 static int acpi_memory_device_start(struct acpi_device *device);
@@ -81,7 +71,6 @@ struct acpi_memory_info {
 
 struct acpi_memory_device {
 	struct acpi_device * device;
-	unsigned int state;	/* State of the memory device */
 	struct list_head res_list;
 };
 
@@ -144,73 +133,6 @@ acpi_memory_get_device_resources(struct 
 	return 0;
 }
 
-static int
-acpi_memory_get_device(acpi_handle handle,
-		       struct acpi_memory_device **mem_device)
-{
-	acpi_status status;
-	acpi_handle phandle;
-	struct acpi_device *device = NULL;
-	struct acpi_device *pdevice = NULL;
-
-
-	if (!acpi_bus_get_device(handle, &device) && device)
-		goto end;
-
-	status = acpi_get_parent(handle, &phandle);
-	if (ACPI_FAILURE(status)) {
-		ACPI_EXCEPTION((AE_INFO, status, "Cannot find acpi parent"));
-		return -EINVAL;
-	}
-
-	/* Get the parent device */
-	status = acpi_bus_get_device(phandle, &pdevice);
-	if (ACPI_FAILURE(status)) {
-		ACPI_EXCEPTION((AE_INFO, status, "Cannot get acpi bus device"));
-		return -EINVAL;
-	}
-
-	/*
-	 * Now add the notified device.  This creates the acpi_device
-	 * and invokes .add function
-	 */
-	status = acpi_bus_add(&device, pdevice, handle, ACPI_BUS_TYPE_DEVICE);
-	if (ACPI_FAILURE(status)) {
-		ACPI_EXCEPTION((AE_INFO, status, "Cannot add acpi bus"));
-		return -EINVAL;
-	}
-
-      end:
-	*mem_device = acpi_driver_data(device);
-	if (!(*mem_device)) {
-		printk(KERN_ERR "\n driver data not found");
-		return -ENODEV;
-	}
-
-	return 0;
-}
-
-static int acpi_memory_check_device(struct acpi_memory_device *mem_device)
-{
-	unsigned long current_status;
-
-
-	/* Get device present/absent information from the _STA */
-	if (ACPI_FAILURE(acpi_evaluate_integer(mem_device->device->handle, "_STA",
-					       NULL, &current_status)))
-		return -ENODEV;
-	/*
-	 * Check for device status. Device should be
-	 * present/enabled/functioning.
-	 */
-	if (!((current_status & ACPI_MEMORY_STA_PRESENT)
-	      && (current_status & ACPI_MEMORY_STA_ENABLED)
-	      && (current_status & ACPI_MEMORY_STA_FUNCTIONAL)))
-		return -ENODEV;
-
-	return 0;
-}
-
 static int acpi_memory_enable_device(struct acpi_memory_device *mem_device)
 {
 	int result, num_enabled = 0;
@@ -222,7 +144,6 @@ static int acpi_memory_enable_device(str
 	result = acpi_memory_get_device_resources(mem_device);
 	if (result) {
 		printk(KERN_ERR PREFIX "get_device_resources failed\n");
-		mem_device->state = MEMORY_INVALID_STATE;
 		return result;
 	}
 
@@ -246,7 +167,6 @@ static int acpi_memory_enable_device(str
 	}
 	if (!num_enabled) {
 		printk(KERN_ERR PREFIX "add_memory failed\n");
-		mem_device->state = MEMORY_INVALID_STATE;
 		return -EINVAL;
 	}
 
@@ -257,16 +177,15 @@ static int acpi_memory_powerdown_device(
 {
 	acpi_status status;
 	struct acpi_object_list arg_list;
+	struct acpi_device *device = mem_device->device;
 	union acpi_object arg;
-	unsigned 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,
+	status = acpi_evaluate_object(device->handle,
 				      "_EJ0", &arg_list, NULL);
 	/* Return on _EJ0 failure */
 	if (ACPI_FAILURE(status)) {
@@ -275,15 +194,14 @@ static int acpi_memory_powerdown_device(
 	}
 
 	/* Evalute _STA to check if the device is disabled */
-	status = acpi_evaluate_integer(mem_device->device->handle, "_STA",
-				       NULL, &current_status);
-	if (ACPI_FAILURE(status))
-		return -ENODEV;
-
+	acpi_bus_get_status(device);
 	/* Check for device status.  Device should be disabled */
-	if (current_status & ACPI_MEMORY_STA_ENABLED)
+	if (device->status.enabled){
+		printk (KERN_ERR PREFIX "Could not powerdown memory"
+			"device %s",
+			acpi_device_bid(device));
 		return -EINVAL;
-
+	}
 	return 0;
 }
 
@@ -308,21 +226,20 @@ static int acpi_memory_disable_device(st
 
 	/* 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;
+	if (result)
 		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_memory_device *mem_device = (struct acpi_memory_device*) data;
 	struct acpi_device *device;
 
+	if (!mem_device || !mem_device->device)
+		return;
+	device = mem_device->device;
 
 	switch (event) {
 	case ACPI_NOTIFY_BUS_CHECK:
@@ -333,31 +250,20 @@ static void acpi_memory_device_notify(ac
 		if (event == ACPI_NOTIFY_DEVICE_CHECK)
 			ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 					  "\nReceived DEVICE CHECK notification for device\n"));
-		if (acpi_memory_get_device(handle, &mem_device)) {
-			printk(KERN_ERR PREFIX "Cannot find driver data\n");
-			return;
-		}
 
-		if (!acpi_memory_check_device(mem_device)) {
+		acpi_bus_get_status(device);
+		if (!(device->status.present &&
+		      device->status.enabled &&
+		      device->status.functional)){
 			if (acpi_memory_enable_device(mem_device))
 				printk(KERN_ERR PREFIX
-					    "Cannot enable memory device\n");
+				       "Cannot enable memory device\n");
 		}
 		break;
 	case ACPI_NOTIFY_EJECT_REQUEST:
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 				  "\nReceived EJECT REQUEST notification for device\n"));
 
-		if (acpi_bus_get_device(handle, &device)) {
-			printk(KERN_ERR PREFIX "Device doesn't exist\n");
-			break;
-		}
-		mem_device = acpi_driver_data(device);
-		if (!mem_device) {
-			printk(KERN_ERR PREFIX "Driver Data is NULL\n");
-			break;
-		}
-
 		/*
 		 * Currently disabling memory device from kernel mode
 		 * TBD: Can also be disabled from user mode scripts
@@ -390,6 +296,13 @@ static int acpi_memory_device_add(struct
 	if (!device)
 		return -EINVAL;
 
+	/* Check for _STA and EJ0 func */
+	if (!device->flags.dynamic_status || !device->flags.ejectable){
+		printk(KERN_INFO PREFIX "Memory device %s has no _STA or"
+		       "EJ0/EJD function", acpi_device_bid(device));
+		return -ENODEV;
+	}
+
 	mem_device = kmalloc(sizeof(struct acpi_memory_device), GFP_KERNEL);
 	if (!mem_device)
 		return -ENOMEM;
@@ -410,15 +323,13 @@ static int acpi_memory_device_add(struct
 
 	/* register notify handler */
 	status = acpi_install_notify_handler(device->handle, ACPI_SYSTEM_NOTIFY,
-					     acpi_memory_device_notify, NULL);
+					     acpi_memory_device_notify, mem_device);
 
 	if (ACPI_FAILURE(status)){
 		ACPI_EXCEPTION((AE_INFO, status, "Could not install notify"
 				"handler for memory device: %s",
 				acpi_device_bid(device)));
 	}
-	/* Set the device state */
-	mem_device->state = MEMORY_POWER_ON_STATE;
 
 	printk(KERN_INFO "%s \n", acpi_device_name(device));
 
@@ -450,13 +361,15 @@ static int acpi_memory_device_start (str
 
 	mem_device = acpi_driver_data(device);
 
-	if (!acpi_memory_check_device(mem_device)) {
-		/* call add_memory func */
-		result = acpi_memory_enable_device(mem_device);
-		if (result)
-			ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
-				"Error in acpi_memory_enable_device\n"));
+	acpi_bus_get_status(device);
+	if (!(device->status.present &&
+	      device->status.enabled &&
+	      device->status.functional)){
+		if (acpi_memory_enable_device(mem_device))
+			printk(KERN_ERR PREFIX
+			       "Error in acpi_memory_enable_device\n");
 	}
+
 	return result;
 }
 

Re: [PATCH 2/2] acpi hotplug cleanups, move install notifier to add function

[Yasunori Goto]

> @@ -390,6 +296,13 @@ static int acpi_memory_device_add(struct
>  	if (!device)
>  		return -EINVAL;
>  
> +	/* Check for _STA and EJ0 func */
> +	if (!device->flags.dynamic_status || !device->flags.ejectable){
> +		printk(KERN_INFO PREFIX "Memory device %s has no _STA or"
> +		       "EJ0/EJD function", acpi_device_bid(device));
> +		return -ENODEV;
> +	}
> +
>  	mem_device = kmalloc(sizeof(struct acpi_memory_device), GFP_KERNEL);
>  	if (!mem_device)
>  		return -ENOMEM;

One comment.

Memory device might not have _EJ0/_EJD, but parent device
(like one NUMA node) might be able to be ejectable.
In this case, only the parent device has _EJ0/_EJD.
So, one more check is necessary.

(If a node is hot-added, container driver of acpi calls acpi_memhotplug
 driver.)


Thanks.

-- 
Yasunori Goto 

Re: [PATCH](memory hotplug) Repost remove useless message at boot time from 2.6.18-rc4.

[Yasunori Goto]

> Am Fr 25.08.2006 13:59 schrieb Yasunori Goto <y-goto@jp.fujitsu.com>:
> >
> >>
> >> > I sent a patch a while ago that gets rid of the whole namespace
> >> > walking
> >> > by making acpi_memoryhotplug an acpi device and making use of the
> >> > .add
> >> > callback function and the acpi_bus_register_driver call.
> >> >
> >> > I am not sure whether this is possible if you have multiple memory
> >> > devices, though (if not maybe it should be made possible?)...
> >> >
> >> > Yasunori even tested the patch and sent an Ok:
> >> > http://marc.theaimsgroup.com/?t=114065312400001&r=1&w=2
> >> >
> >> > If this is acceptable I can rebase the patch on a current kernel.
> >>
> >> Hi. Thomas-san.
> >> Did you rebase your patch?
> >>
> >> I'm trying to do it now too.
> >> But, current code (2.6.18-rc4) seems to register handler for
> >> only enable status devices at boot time.
> >> So, notification is -discarded- due to no handler for new memory
> >> device when hot-add event occurs. Hmmm. :-(
> >No, what I see the notify handler is always installed.

Hmm.
Ok. Followings are current my understanding of sequence
with your patch.

At boot time, acpi_memory_device_init() is called.

acpi_memory_device_init()
   |
   +---> acpi_bus_register_driver()
           |
           +---> acpi_driver_attach()
                   |
                   +---> acpi_bus_driver_init()
                           |
                           +---> acpi_memory_device_add()
                                    |
                                    +---> acpi_install_notify_handler().


The problem is in acpi_driver_attach(). This function is using
"acpi_device_list" to call acpi_bus_driver_init().

This list is registered by acpi_device_register() which is called by
acpi_add_single_object().
However, acpi_add_single_object() skips calling it if _STA is not on.

1015         switch (type) {
1016         case ACPI_BUS_TYPE_PROCESSOR:
1017         case ACPI_BUS_TYPE_DEVICE:
1018                 result = acpi_bus_get_status(device);
1019                 if (ACPI_FAILURE(result) || !device->status.present) {
1020                         result = -ENOENT;
1021                         goto end;
1022                 }
1023                 break;

So, notify handler is registered just for memory device which is enable
at boot time.
If notify event occurs for new memory device, there is no notify handler
for it....

Old code registers handler for all of memory devices even if it is not
enabled.

If my understanding is wrong, please let me know. ;-)
Bye.

-- 
Yasunori Goto 

Re: [PATCH](memory hotplug) Repost remove useless message at boot time from 2.6.18-rc4.

[Thomas Renninger]

On Mon, 2006-08-28 at 23:12 +0900, Yasunori Goto wrote: 
> Hmm.
> Ok. Followings are current my understanding of sequence
> with your patch.
> 
> At boot time, acpi_memory_device_init() is called.
> 
> acpi_memory_device_init()
>    |
>    +---> acpi_bus_register_driver()
>            |
>            +---> acpi_driver_attach()
>                    |
>                    +---> acpi_bus_driver_init()
>                            |
>                            +---> acpi_memory_device_add()
>                                     |
>                                     +---> acpi_install_notify_handler().
> 
> 
> The problem is in acpi_driver_attach(). This function is using
> "acpi_device_list" to call acpi_bus_driver_init().
> 
> This list is registered by acpi_device_register() which is called by
> acpi_add_single_object().
> However, acpi_add_single_object() skips calling it if _STA is not on.
> 
> 1015         switch (type) {
> 1016         case ACPI_BUS_TYPE_PROCESSOR:
> 1017         case ACPI_BUS_TYPE_DEVICE:
> 1018                 result = acpi_bus_get_status(device);
> 1019                 if (ACPI_FAILURE(result) || !device->status.present) {
> 1020                         result = -ENOENT;
> 1021                         goto end;
> 1022                 }
> 1023                 break;
> 
> So, notify handler is registered just for memory device which is enable
> at boot time.
> If notify event occurs for new memory device, there is no notify handler
> for it....
> 
Ah, good point. That should also be the reason why a battery inserted
after module loading won't get noticed (I didn't try right now, but I
remember complains about that).
I wonder whether the "!device->status.present" condition can just be
deleted here or checking for device HIDs that potentially can be
ejected/inserted, not sure. Hopefully someone else could comment on that
one.

> Old code registers handler for all of memory devices even if it is not
> enabled.
Yeah, therefore the mem_device cannot be passed as callback data as it
might get generated in the notify handler func and all the additional
stuff is needed..., ouch.
> 
> If my understanding is wrong, please let me know. ;-)
It's me who is wrong, thanks a lot for checking!

> Memory device might not have _EJ0/_EJD, but parent device 
> (like one NUMA node) might be able to be ejectable.
> In this case, only the parent device has _EJ0/_EJD.
> So, one more check is necessary.

I feared something like that (should have add a comment...), as the EJ0
and _STA functions are only used on the device itself I thought checking
for them makes sense, but for a missing EJ0 func powering down the
device just fails and it should not be harmful.
So the only useful thing from my patch (as long as .add is only invoked
if device is present) is using the general acpi_bus_get_status() func. 
Hmm, it must be used if the _STA function on the memory device is also
missing and the parent _STA must be used then? Could make sense on a
machine where a whole node must be inserted/ejected? The
acpi_bus_get_status() function already contains the checking for the
parent's _STA function and uses this one if the device itself has none.

  Thomas

Re: [PATCH](memory hotplug) Repost remove useless message at boot time from 2.6.18-rc4.

[Yasunori Goto]

> > Old code registers handler for all of memory devices even if it is not
> > enabled.
> Yeah, therefore the mem_device cannot be passed as callback data as it
> might get generated in the notify handler func and all the additional
> stuff is needed..., ouch.
> > 
> > If my understanding is wrong, please let me know. ;-)
> It's me who is wrong, thanks a lot for checking!
> 
> > Memory device might not have _EJ0/_EJD, but parent device 
> > (like one NUMA node) might be able to be ejectable.
> > In this case, only the parent device has _EJ0/_EJD.
> > So, one more check is necessary.
> 
> I feared something like that (should have add a comment...), as the EJ0
> and _STA functions are only used on the device itself I thought checking
> for them makes sense, but for a missing EJ0 func powering down the
> device just fails and it should not be harmful.
> So the only useful thing from my patch (as long as .add is only invoked
> if device is present) is using the general acpi_bus_get_status() func. 
> Hmm, it must be used if the _STA function on the memory device is also
> missing and the parent _STA must be used then? Could make sense on a
> machine where a whole node must be inserted/ejected? The
> acpi_bus_get_status() function already contains the checking for the
> parent's _STA function and uses this one if the device itself has none.

I don't have any report like "no _STA on memory device" so far.
Current code assume each memory device has _STA.
I suppose each memory device should have _STA method. For example,
if a memory device is broken, its _STA should return disable status. 
So, basically checking _STA of only memory device should be ok now.

However I'm not sure that every vendor will do it in the future.
If there is no _STA on memory device, parents, grand-parents or
more ancestor might have _STA for it.
(See acpi_get_pxm() in driver/acpi/numa.c. It searches ancestor's _PXM.
 If insane vendor make like it, it will be good reference.)

Bye.
-- 
Yasunori Goto 

Re: [PATCH 1/2] acpi hotplug cleanups, move install notifier to add function

[keith mannthey]

On Sun, 2006-08-27 at 19:19 +0200, Thomas Renninger wrote:
> On Fri, 2006-08-25 at 20:59 +0900, Yasunori Goto wrote:
> > > I sent a patch a while ago that gets rid of the whole namespace walking
> > > by making acpi_memoryhotplug an acpi device and making use of the .add
> > > callback function and the acpi_bus_register_driver call.
> > > 
> > > I am not sure whether this is possible if you have multiple memory
> > > devices, though (if not maybe it should be made possible?)...
> > > 
> > > Yasunori even tested the patch and sent an Ok:
> > > http://marc.theaimsgroup.com/?t=114065312400001&r=1&w=2
> > > 
> > > If this is acceptable I can rebase the patch on a current kernel.
> > 
> > Hi. Thomas-san.
> > Did you rebase your patch?
> > 
> > I'm trying to do it now too. 
> > But, current code (2.6.18-rc4) seems to register handler for
> > only enable status devices at boot time.
> > So, notification is -discarded- due to no handler for new memory
> > device when hot-add event occurs. Hmmm. :-(
> 
> Trying again with a real mailer, sorry about the previous junk ...
> The email address of the module author (naveen.b.s@intel.com) seems to
> be invalid?
> 
>     Thomas

Sorry for taking so long to test this out.  I just hooked it up with
2.6.18-rc4-mm3 (I built with CONFIG_ACPI_HOTPLUG_MEMORY=y) and I get

ACPI Exception (acpi_bus-0071): AE_NOT_FOUND, No context for object
[ffff81107357a5d0] [20060707]

when I do a hot-add. It might be a day or two before I can sort out what
is going on but I think Yasunori's mail may be getting at the issue. (My
device is only presented to the OS at add time.)

Thanks,
  Keith 

Re: [PATCH](memory hotplug) Repost remove useless message at boot time from 2.6.18-rc4.

[Bjorn Helgaas]

On Monday 28 August 2006 08:12, Yasunori Goto wrote:
> > Am Fr 25.08.2006 13:59 schrieb Yasunori Goto <y-goto@jp.fujitsu.com>:
> Ok. Followings are current my understanding of sequence
> with your patch.
> 
> At boot time, acpi_memory_device_init() is called.
> 
> acpi_memory_device_init()
>    |
>    +---> acpi_bus_register_driver()
>            |
>            +---> acpi_driver_attach()
>                    |
>                    +---> acpi_bus_driver_init()
>                            |
>                            +---> acpi_memory_device_add()
>                                     |
>                                     +---> acpi_install_notify_handler().
> 
> 
> The problem is in acpi_driver_attach(). This function is using
> "acpi_device_list" to call acpi_bus_driver_init().
> 
> This list is registered by acpi_device_register() which is called by
> acpi_add_single_object().
> However, acpi_add_single_object() skips calling it if _STA is not on.
> 
> 1015         switch (type) {
> 1016         case ACPI_BUS_TYPE_PROCESSOR:
> 1017         case ACPI_BUS_TYPE_DEVICE:
> 1018                 result = acpi_bus_get_status(device);
> 1019                 if (ACPI_FAILURE(result) || !device->status.present) {
> 1020                         result = -ENOENT;
> 1021                         goto end;
> 1022                 }
> 1023                 break;
> 
> So, notify handler is registered just for memory device which is enable
> at boot time.
> If notify event occurs for new memory device, there is no notify handler
> for it....

I looked at this over a year ago, and my feeble recollection is
that if _STA says "not present", we don't do the device_add.  Later,
when _STA changes to "present", we get an ACPI notification.  I
expected that we would just do the device_add() at that time, and
there are even comments in acpi_bus_check_device() that suggest that,
but it just looks unfinished.

It seemed like it would be much cleaner to finish that up, and then
the driver's .add method would automatically get called, and the
memory driver wouldn't have to bother with all the notification stuff.

Bjorn