* Re: [PATCH](memory hotplug) Repost remove useless message at boot time from 2.6.18-rc4.
2006-08-25 11:59 ` Yasunori Goto
@ 2006-08-27 12:50 ` Thomas Renninger
2006-08-28 14:12 ` Yasunori Goto
2006-08-27 15:19 ` [PATCH 2/2] ACPI memory_hotplug cleanups Thomas Renninger
` (2 subsequent siblings)
3 siblings, 1 reply; 18+ messages in thread
From: Thomas Renninger @ 2006-08-27 12:50 UTC (permalink / raw)
To: Yasunori Goto
Cc: akpm, Brown, Len, keith mannthey, ACPI-ML, Linux Kernel ML,
Linux Hotplug Memory Support, naveen.b.s
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...
>
-
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] 18+ messages in thread* Re: [PATCH](memory hotplug) Repost remove useless message at boot time from 2.6.18-rc4.
2006-08-27 12:50 ` Thomas Renninger
@ 2006-08-28 14:12 ` Yasunori Goto
2006-08-28 21:16 ` Thomas Renninger
2006-08-31 22:22 ` Bjorn Helgaas
0 siblings, 2 replies; 18+ messages in thread
From: Yasunori Goto @ 2006-08-28 14:12 UTC (permalink / raw)
To: Thomas Renninger
Cc: akpm, Brown, Len, keith mannthey, ACPI-ML, Linux Kernel ML,
Linux Hotplug Memory Support, naveen.b.s
> 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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH](memory hotplug) Repost remove useless message at boot time from 2.6.18-rc4.
2006-08-28 14:12 ` Yasunori Goto
@ 2006-08-28 21:16 ` Thomas Renninger
2006-08-29 1:57 ` Yasunori Goto
2006-08-31 22:22 ` Bjorn Helgaas
1 sibling, 1 reply; 18+ messages in thread
From: Thomas Renninger @ 2006-08-28 21:16 UTC (permalink / raw)
To: Yasunori Goto
Cc: Thomas Renninger, akpm, Brown, Len, keith mannthey, ACPI-ML,
Linux Kernel ML, Linux Hotplug Memory Support, naveen.b.s
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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH](memory hotplug) Repost remove useless message at boot time from 2.6.18-rc4.
2006-08-28 21:16 ` Thomas Renninger
@ 2006-08-29 1:57 ` Yasunori Goto
0 siblings, 0 replies; 18+ messages in thread
From: Yasunori Goto @ 2006-08-29 1:57 UTC (permalink / raw)
To: mail
Cc: Thomas Renninger, akpm, Brown, Len, keith mannthey, ACPI-ML,
Linux Kernel ML, Linux Hotplug Memory Support, naveen.b.s
> > 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH](memory hotplug) Repost remove useless message at boot time from 2.6.18-rc4.
2006-08-28 14:12 ` Yasunori Goto
2006-08-28 21:16 ` Thomas Renninger
@ 2006-08-31 22:22 ` Bjorn Helgaas
1 sibling, 0 replies; 18+ messages in thread
From: Bjorn Helgaas @ 2006-08-31 22:22 UTC (permalink / raw)
To: Yasunori Goto
Cc: Thomas Renninger, akpm, Brown, Len, keith mannthey, ACPI-ML,
Linux Kernel ML, Linux Hotplug Memory Support, naveen.b.s
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2] ACPI memory_hotplug cleanups
2006-08-25 11:59 ` Yasunori Goto
2006-08-27 12:50 ` Thomas Renninger
@ 2006-08-27 15:19 ` Thomas Renninger
2006-08-27 17:19 ` [PATCH 1/2] acpi hotplug cleanups, move install notifier to add function Thomas Renninger
2006-08-27 17:58 ` [PATCH 2/2] " Thomas Renninger
3 siblings, 0 replies; 18+ messages in thread
From: Thomas Renninger @ 2006-08-27 15:19 UTC (permalink / raw)
To: Yasunori Goto
Cc: akpm, Brown, Len, keith mannthey, ACPI-ML, Linux Kernel ML,
Linux Hotplug Memory Support
[-- 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, ¤t_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, ¤t_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;
}
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH 1/2] acpi hotplug cleanups, move install notifier to add function
2006-08-25 11:59 ` Yasunori Goto
2006-08-27 12:50 ` Thomas Renninger
2006-08-27 15:19 ` [PATCH 2/2] ACPI memory_hotplug cleanups Thomas Renninger
@ 2006-08-27 17:19 ` Thomas Renninger
2006-08-30 21:48 ` keith mannthey
2006-08-27 17:58 ` [PATCH 2/2] " Thomas Renninger
3 siblings, 1 reply; 18+ messages in thread
From: Thomas Renninger @ 2006-08-27 17:19 UTC (permalink / raw)
To: Yasunori Goto
Cc: trenn, akpm, Brown, Len, keith mannthey, ACPI-ML, Linux Kernel ML,
Linux Hotplug Memory Support, naveen.b.s
[-- 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;
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 1/2] acpi hotplug cleanups, move install notifier to add function
2006-08-27 17:19 ` [PATCH 1/2] acpi hotplug cleanups, move install notifier to add function Thomas Renninger
@ 2006-08-30 21:48 ` keith mannthey
0 siblings, 0 replies; 18+ messages in thread
From: keith mannthey @ 2006-08-30 21:48 UTC (permalink / raw)
To: mail
Cc: Yasunori Goto, trenn, andrew, Brown, Len, ACPI-ML,
Linux Kernel ML, Linux Hotplug Memory Support, naveen.b.s
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2] acpi hotplug cleanups, move install notifier to add function
2006-08-25 11:59 ` Yasunori Goto
` (2 preceding siblings ...)
2006-08-27 17:19 ` [PATCH 1/2] acpi hotplug cleanups, move install notifier to add function Thomas Renninger
@ 2006-08-27 17:58 ` Thomas Renninger
2006-08-28 8:08 ` Yasunori Goto
3 siblings, 1 reply; 18+ messages in thread
From: Thomas Renninger @ 2006-08-27 17:58 UTC (permalink / raw)
To: Yasunori Goto
Cc: trenn, akpm, Brown, Len, keith mannthey, ACPI-ML, Linux Kernel ML,
Linux Hotplug Memory Support, naveen.b.s
[-- 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, ¤t_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, ¤t_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, ¤t_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, ¤t_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;
}
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 2/2] acpi hotplug cleanups, move install notifier to add function
2006-08-27 17:58 ` [PATCH 2/2] " Thomas Renninger
@ 2006-08-28 8:08 ` Yasunori Goto
0 siblings, 0 replies; 18+ messages in thread
From: Yasunori Goto @ 2006-08-28 8:08 UTC (permalink / raw)
To: mail
Cc: trenn, akpm, Brown, Len, keith mannthey, ACPI-ML, Linux Kernel ML,
Linux Hotplug Memory Support, naveen.b.s
> @@ -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
^ permalink raw reply [flat|nested] 18+ messages in thread