| Message ID | 5090866.2BHgpRnuEZ@vostro.rjw.lan (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Mon, 2013-02-04 at 00:47 +0100, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > The only useful thing that the ACPI container driver does is to > install system notify handlers for all container and module device > objects it finds in the namespace. The driver structure, > acpi_container_driver, and the data structures created by its > .add() callback are in fact not used by the driver, so remove > them entirely. > > It also makes a little sense to build that driver as a module, > so make it non-modular and add its initialization to the > namespace scanning code. > > In addition to that, make the namespace walk callback used for > installing the notify handlers more straightforward. I think the container driver needs to be registered as an ACPI scan driver so that sysfs eject will continue to work for container devices, such as ACPI0004:XX/eject. Since the container driver does not support ACPI eject notification (and we have been discussing how system device hot-plug should work), this sysfs eject is the only way to eject a container device at this point. I will send an update patchset that applies on top of this patch. With the update in my patchset: Reviewed-by: Toshi Kani <toshi.kani@hp.com> Thanks, -Toshi > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/acpi/Kconfig | 2 > drivers/acpi/container.c | 158 ++++++----------------------------------------- > drivers/acpi/internal.h | 5 + > drivers/acpi/scan.c | 1 > 4 files changed, 30 insertions(+), 136 deletions(-) > > Index: test/drivers/acpi/container.c > =================================================================== > --- test.orig/drivers/acpi/container.c > +++ test/drivers/acpi/container.c > @@ -38,41 +38,15 @@ > > #define PREFIX "ACPI: " > > -#define ACPI_CONTAINER_DEVICE_NAME "ACPI container device" > -#define ACPI_CONTAINER_CLASS "container" > - > -#define INSTALL_NOTIFY_HANDLER 1 > -#define UNINSTALL_NOTIFY_HANDLER 2 > - > #define _COMPONENT ACPI_CONTAINER_COMPONENT > ACPI_MODULE_NAME("container"); > > -MODULE_AUTHOR("Anil S Keshavamurthy"); > -MODULE_DESCRIPTION("ACPI container driver"); > -MODULE_LICENSE("GPL"); > - > -static int acpi_container_add(struct acpi_device *device); > -static int acpi_container_remove(struct acpi_device *device); > - > static const struct acpi_device_id container_device_ids[] = { > {"ACPI0004", 0}, > {"PNP0A05", 0}, > {"PNP0A06", 0}, > {"", 0}, > }; > -MODULE_DEVICE_TABLE(acpi, container_device_ids); > - > -static struct acpi_driver acpi_container_driver = { > - .name = "container", > - .class = ACPI_CONTAINER_CLASS, > - .ids = container_device_ids, > - .ops = { > - .add = acpi_container_add, > - .remove = acpi_container_remove, > - }, > -}; > - > -/*******************************************************************/ > > static int is_device_present(acpi_handle handle) > { > @@ -92,49 +66,6 @@ static int is_device_present(acpi_handle > return ((sta & ACPI_STA_DEVICE_PRESENT) == ACPI_STA_DEVICE_PRESENT); > } > > -static bool is_container_device(const char *hid) > -{ > - const struct acpi_device_id *container_id; > - > - for (container_id = container_device_ids; > - container_id->id[0]; container_id++) { > - if (!strcmp((char *)container_id->id, hid)) > - return true; > - } > - > - return false; > -} > - > -/*******************************************************************/ > -static int acpi_container_add(struct acpi_device *device) > -{ > - struct acpi_container *container; > - > - container = kzalloc(sizeof(struct acpi_container), GFP_KERNEL); > - if (!container) > - return -ENOMEM; > - > - container->handle = device->handle; > - strcpy(acpi_device_name(device), ACPI_CONTAINER_DEVICE_NAME); > - strcpy(acpi_device_class(device), ACPI_CONTAINER_CLASS); > - device->driver_data = container; > - > - ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Device <%s> bid <%s>\n", > - acpi_device_name(device), acpi_device_bid(device))); > - > - return 0; > -} > - > -static int acpi_container_remove(struct acpi_device *device) > -{ > - acpi_status status = AE_OK; > - struct acpi_container *pc = NULL; > - > - pc = acpi_driver_data(device); > - kfree(pc); > - return status; > -} > - > static void container_notify_cb(acpi_handle handle, u32 type, void *context) > { > struct acpi_device *device = NULL; > @@ -199,84 +130,41 @@ static void container_notify_cb(acpi_han > return; > } > > -static acpi_status > -container_walk_namespace_cb(acpi_handle handle, > - u32 lvl, void *context, void **rv) > +static bool is_container(acpi_handle handle) > { > - char *hid = NULL; > struct acpi_device_info *info; > - acpi_status status; > - int *action = context; > + bool ret = false; > > - status = acpi_get_object_info(handle, &info); > - if (ACPI_FAILURE(status)) { > - return AE_OK; > - } > - > - if (info->valid & ACPI_VALID_HID) > - hid = info->hardware_id.string; > - > - if (hid == NULL) { > - goto end; > - } > + if (ACPI_FAILURE(acpi_get_object_info(handle, &info))) > + return false; > > - if (!is_container_device(hid)) > - goto end; > + if (info->valid & ACPI_VALID_HID) { > + const struct acpi_device_id *id; > > - switch (*action) { > - case INSTALL_NOTIFY_HANDLER: > - acpi_install_notify_handler(handle, > - ACPI_SYSTEM_NOTIFY, > - container_notify_cb, NULL); > - break; > - case UNINSTALL_NOTIFY_HANDLER: > - acpi_remove_notify_handler(handle, > - ACPI_SYSTEM_NOTIFY, > - container_notify_cb); > - break; > - default: > - break; > + for (id = container_device_ids; id->id[0]; id++) { > + ret = !strcmp((char *)id->id, info->hardware_id.string); > + if (ret) > + break; > + } > } > - > - end: > kfree(info); > - > - return AE_OK; > + return ret; > } > > -static int __init acpi_container_init(void) > +static acpi_status acpi_container_register_notify_handler(acpi_handle handle, > + u32 lvl, void *ctxt, > + void **retv) > { > - int result = 0; > - int action = INSTALL_NOTIFY_HANDLER; > - > - result = acpi_bus_register_driver(&acpi_container_driver); > - if (result < 0) { > - return (result); > - } > - > - /* register notify handler to every container device */ > - acpi_walk_namespace(ACPI_TYPE_DEVICE, > - ACPI_ROOT_OBJECT, > - ACPI_UINT32_MAX, > - container_walk_namespace_cb, NULL, &action, NULL); > + if (is_container(handle)) > + acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY, > + container_notify_cb, NULL); > > - return (0); > + return AE_OK; > } > > -static void __exit acpi_container_exit(void) > +void __init acpi_container_init(void) > { > - int action = UNINSTALL_NOTIFY_HANDLER; > - > - > - acpi_walk_namespace(ACPI_TYPE_DEVICE, > - ACPI_ROOT_OBJECT, > - ACPI_UINT32_MAX, > - container_walk_namespace_cb, NULL, &action, NULL); > - > - acpi_bus_unregister_driver(&acpi_container_driver); > - > - return; > + acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, ACPI_UINT32_MAX, > + acpi_container_register_notify_handler, NULL, > + NULL, NULL); > } > - > -module_init(acpi_container_init); > -module_exit(acpi_container_exit); > Index: test/drivers/acpi/Kconfig > =================================================================== > --- test.orig/drivers/acpi/Kconfig > +++ test/drivers/acpi/Kconfig > @@ -334,7 +334,7 @@ config X86_PM_TIMER > systems require this timer. > > config ACPI_CONTAINER > - tristate "Container and Module Devices (EXPERIMENTAL)" > + bool "Container and Module Devices (EXPERIMENTAL)" > depends on EXPERIMENTAL > default (ACPI_HOTPLUG_MEMORY || ACPI_HOTPLUG_CPU || ACPI_HOTPLUG_IO) > help > Index: test/drivers/acpi/internal.h > =================================================================== > --- test.orig/drivers/acpi/internal.h > +++ test/drivers/acpi/internal.h > @@ -40,6 +40,11 @@ void acpi_memory_hotplug_init(void); > #else > static inline void acpi_memory_hotplug_init(void) {} > #endif > +#ifdef ACPI_CONTAINER > +void acpi_container_init(void); > +#else > +static inline void acpi_container_init(void) {} > +#endif > > #ifdef CONFIG_DEBUG_FS > extern struct dentry *acpi_debugfs_dir; > Index: test/drivers/acpi/scan.c > =================================================================== > --- test.orig/drivers/acpi/scan.c > +++ test/drivers/acpi/scan.c > @@ -1763,6 +1763,7 @@ int __init acpi_scan_init(void) > acpi_platform_init(); > acpi_csrt_init(); > acpi_memory_hotplug_init(); > + acpi_container_init(); > > /* > * Enumerate devices in the ACPI namespace. > -- 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
On Thu, 2013-02-07 at 01:55 +0100, Rafael J. Wysocki wrote: > On Wednesday, February 06, 2013 03:32:18 PM Toshi Kani wrote: > > On Mon, 2013-02-04 at 00:47 +0100, Rafael J. Wysocki wrote: > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > The only useful thing that the ACPI container driver does is to > > > install system notify handlers for all container and module device > > > objects it finds in the namespace. The driver structure, > > > acpi_container_driver, and the data structures created by its > > > .add() callback are in fact not used by the driver, so remove > > > them entirely. > > > > > > It also makes a little sense to build that driver as a module, > > > so make it non-modular and add its initialization to the > > > namespace scanning code. > > > > > > In addition to that, make the namespace walk callback used for > > > installing the notify handlers more straightforward. > > > > I think the container driver needs to be registered as an ACPI scan > > driver so that sysfs eject will continue to work for container devices, > > such as ACPI0004:XX/eject. Since the container driver does not support > > ACPI eject notification (and we have been discussing how system device > > hot-plug should work), this sysfs eject is the only way to eject a > > container device at this point. I will send an update patchset that > > applies on top of this patch. > > > > With the update in my patchset: > > Reviewed-by: Toshi Kani <toshi.kani@hp.com> > > Thanks, but I'd like to (1) apply your patch from > https://patchwork.kernel.org/patch/2108851/ first and then (2) fold your [2/2] > into my [2/2], if you don't mind, and apply that next. That's fine by me. > Moreover, I'm wondering if the #ifndef FORCE_EJECT thing in acpi_eject_store() > actually makes sense after the recent changes to acpi_bus_trim(), because that > can't fail now, so the eject will always be carried out. So perhaps we can > simply remove the acpi_device->driver check from there entirely in the first > place? > > If we really want to be able to prevent ejects from happening in some cases, > we need to implement something along the lines discussed with Greg. acpi_bus_trim() cannot fail, but sysfs eject can fail. So, I think it makes sense to do some validation before calling acpi_bus_trim(). If we are to implement the no_eject flag thing, that check needs to be made before calling acpi_bus_trim(). Thanks, -Toshi -- 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
On Wednesday, February 06, 2013 03:32:18 PM Toshi Kani wrote: > On Mon, 2013-02-04 at 00:47 +0100, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > The only useful thing that the ACPI container driver does is to > > install system notify handlers for all container and module device > > objects it finds in the namespace. The driver structure, > > acpi_container_driver, and the data structures created by its > > .add() callback are in fact not used by the driver, so remove > > them entirely. > > > > It also makes a little sense to build that driver as a module, > > so make it non-modular and add its initialization to the > > namespace scanning code. > > > > In addition to that, make the namespace walk callback used for > > installing the notify handlers more straightforward. > > I think the container driver needs to be registered as an ACPI scan > driver so that sysfs eject will continue to work for container devices, > such as ACPI0004:XX/eject. Since the container driver does not support > ACPI eject notification (and we have been discussing how system device > hot-plug should work), this sysfs eject is the only way to eject a > container device at this point. I will send an update patchset that > applies on top of this patch. > > With the update in my patchset: > Reviewed-by: Toshi Kani <toshi.kani@hp.com> Thanks, but I'd like to (1) apply your patch from https://patchwork.kernel.org/patch/2108851/ first and then (2) fold your [2/2] into my [2/2], if you don't mind, and apply that next. Moreover, I'm wondering if the #ifndef FORCE_EJECT thing in acpi_eject_store() actually makes sense after the recent changes to acpi_bus_trim(), because that can't fail now, so the eject will always be carried out. So perhaps we can simply remove the acpi_device->driver check from there entirely in the first place? If we really want to be able to prevent ejects from happening in some cases, we need to implement something along the lines discussed with Greg. Thanks, Rafael
On Wednesday, February 06, 2013 05:51:42 PM Toshi Kani wrote: > On Thu, 2013-02-07 at 01:55 +0100, Rafael J. Wysocki wrote: > > On Wednesday, February 06, 2013 03:32:18 PM Toshi Kani wrote: > > > On Mon, 2013-02-04 at 00:47 +0100, Rafael J. Wysocki wrote: > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > The only useful thing that the ACPI container driver does is to > > > > install system notify handlers for all container and module device > > > > objects it finds in the namespace. The driver structure, > > > > acpi_container_driver, and the data structures created by its > > > > .add() callback are in fact not used by the driver, so remove > > > > them entirely. > > > > > > > > It also makes a little sense to build that driver as a module, > > > > so make it non-modular and add its initialization to the > > > > namespace scanning code. > > > > > > > > In addition to that, make the namespace walk callback used for > > > > installing the notify handlers more straightforward. > > > > > > I think the container driver needs to be registered as an ACPI scan > > > driver so that sysfs eject will continue to work for container devices, > > > such as ACPI0004:XX/eject. Since the container driver does not support > > > ACPI eject notification (and we have been discussing how system device > > > hot-plug should work), this sysfs eject is the only way to eject a > > > container device at this point. I will send an update patchset that > > > applies on top of this patch. > > > > > > With the update in my patchset: > > > Reviewed-by: Toshi Kani <toshi.kani@hp.com> > > > > Thanks, but I'd like to (1) apply your patch from > > https://patchwork.kernel.org/patch/2108851/ first and then (2) fold your [2/2] > > into my [2/2], if you don't mind, and apply that next. > > That's fine by me. > > > Moreover, I'm wondering if the #ifndef FORCE_EJECT thing in acpi_eject_store() > > actually makes sense after the recent changes to acpi_bus_trim(), because that > > can't fail now, so the eject will always be carried out. So perhaps we can > > simply remove the acpi_device->driver check from there entirely in the first > > place? > > > > If we really want to be able to prevent ejects from happening in some cases, > > we need to implement something along the lines discussed with Greg. > > acpi_bus_trim() cannot fail, but sysfs eject can fail. So, I think it > makes sense to do some validation before calling acpi_bus_trim(). If we > are to implement the no_eject flag thing, that check needs to be made > before calling acpi_bus_trim(). Sure, but now the logic seems to be "if FORCE_EJECT is not set, don't eject devices that have no ACPI drivers", so I'm wondering what the purpose of this is. It definitely isn't too obvious. :-) Thanks, Rafael
Hi Rafael, Sorry for late reply. 2013/02/04 8:47, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > The only useful thing that the ACPI container driver does is to > install system notify handlers for all container and module device > objects it finds in the namespace. The driver structure, > acpi_container_driver, and the data structures created by its > .add() callback are in fact not used by the driver, so remove > them entirely. > > It also makes a little sense to build that driver as a module, > so make it non-modular and add its initialization to the > namespace scanning code. > > In addition to that, make the namespace walk callback used for > installing the notify handlers more straightforward. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/acpi/Kconfig | 2 > drivers/acpi/container.c | 158 ++++++----------------------------------------- > drivers/acpi/internal.h | 5 + > drivers/acpi/scan.c | 1 > 4 files changed, 30 insertions(+), 136 deletions(-) > > Index: test/drivers/acpi/container.c > =================================================================== > --- test.orig/drivers/acpi/container.c > +++ test/drivers/acpi/container.c > @@ -38,41 +38,15 @@ > > #define PREFIX "ACPI: " > > -#define ACPI_CONTAINER_DEVICE_NAME "ACPI container device" > -#define ACPI_CONTAINER_CLASS "container" > - > -#define INSTALL_NOTIFY_HANDLER 1 > -#define UNINSTALL_NOTIFY_HANDLER 2 > - > #define _COMPONENT ACPI_CONTAINER_COMPONENT > ACPI_MODULE_NAME("container"); > > -MODULE_AUTHOR("Anil S Keshavamurthy"); > -MODULE_DESCRIPTION("ACPI container driver"); > -MODULE_LICENSE("GPL"); > - > -static int acpi_container_add(struct acpi_device *device); > -static int acpi_container_remove(struct acpi_device *device); > - > static const struct acpi_device_id container_device_ids[] = { > {"ACPI0004", 0}, > {"PNP0A05", 0}, > {"PNP0A06", 0}, > {"", 0}, > }; > -MODULE_DEVICE_TABLE(acpi, container_device_ids); > - > -static struct acpi_driver acpi_container_driver = { > - .name = "container", > - .class = ACPI_CONTAINER_CLASS, > - .ids = container_device_ids, > - .ops = { > - .add = acpi_container_add, > - .remove = acpi_container_remove, > - }, > -}; > - > -/*******************************************************************/ > > static int is_device_present(acpi_handle handle) > { > @@ -92,49 +66,6 @@ static int is_device_present(acpi_handle > return ((sta & ACPI_STA_DEVICE_PRESENT) == ACPI_STA_DEVICE_PRESENT); > } > > -static bool is_container_device(const char *hid) > -{ > - const struct acpi_device_id *container_id; > - > - for (container_id = container_device_ids; > - container_id->id[0]; container_id++) { > - if (!strcmp((char *)container_id->id, hid)) > - return true; > - } > - > - return false; > -} > - > -/*******************************************************************/ > -static int acpi_container_add(struct acpi_device *device) > -{ > - struct acpi_container *container; > - > - container = kzalloc(sizeof(struct acpi_container), GFP_KERNEL); > - if (!container) > - return -ENOMEM; > - > - container->handle = device->handle; > - strcpy(acpi_device_name(device), ACPI_CONTAINER_DEVICE_NAME); > - strcpy(acpi_device_class(device), ACPI_CONTAINER_CLASS); > - device->driver_data = container; > - > - ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Device <%s> bid <%s>\n", > - acpi_device_name(device), acpi_device_bid(device))); > - > - return 0; > -} > - > -static int acpi_container_remove(struct acpi_device *device) > -{ > - acpi_status status = AE_OK; > - struct acpi_container *pc = NULL; > - > - pc = acpi_driver_data(device); > - kfree(pc); > - return status; > -} > - > static void container_notify_cb(acpi_handle handle, u32 type, void *context) > { > struct acpi_device *device = NULL; > @@ -199,84 +130,41 @@ static void container_notify_cb(acpi_han > return; > } > > -static acpi_status > -container_walk_namespace_cb(acpi_handle handle, > - u32 lvl, void *context, void **rv) > +static bool is_container(acpi_handle handle) > { > - char *hid = NULL; > struct acpi_device_info *info; > - acpi_status status; > - int *action = context; > + bool ret = false; > > - status = acpi_get_object_info(handle, &info); > - if (ACPI_FAILURE(status)) { > - return AE_OK; > - } > - > - if (info->valid & ACPI_VALID_HID) > - hid = info->hardware_id.string; > - > - if (hid == NULL) { > - goto end; > - } > + if (ACPI_FAILURE(acpi_get_object_info(handle, &info))) > + return false; > > - if (!is_container_device(hid)) > - goto end; > + if (info->valid & ACPI_VALID_HID) { > + const struct acpi_device_id *id; > > - switch (*action) { > - case INSTALL_NOTIFY_HANDLER: > - acpi_install_notify_handler(handle, > - ACPI_SYSTEM_NOTIFY, > - container_notify_cb, NULL); > - break; > - case UNINSTALL_NOTIFY_HANDLER: > - acpi_remove_notify_handler(handle, > - ACPI_SYSTEM_NOTIFY, > - container_notify_cb); > - break; > - default: > - break; > + for (id = container_device_ids; id->id[0]; id++) { > + ret = !strcmp((char *)id->id, info->hardware_id.string); > + if (ret) > + break; > + } > } > - > - end: > kfree(info); > - > - return AE_OK; > + return ret; > } > > -static int __init acpi_container_init(void) > +static acpi_status acpi_container_register_notify_handler(acpi_handle handle, > + u32 lvl, void *ctxt, > + void **retv) > { > - int result = 0; > - int action = INSTALL_NOTIFY_HANDLER; > - > - result = acpi_bus_register_driver(&acpi_container_driver); > - if (result < 0) { > - return (result); > - } > - > - /* register notify handler to every container device */ > - acpi_walk_namespace(ACPI_TYPE_DEVICE, > - ACPI_ROOT_OBJECT, > - ACPI_UINT32_MAX, > - container_walk_namespace_cb, NULL, &action, NULL); > + if (is_container(handle)) > + acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY, > + container_notify_cb, NULL); > > - return (0); > + return AE_OK; > } > > -static void __exit acpi_container_exit(void) > +void __init acpi_container_init(void) > { > - int action = UNINSTALL_NOTIFY_HANDLER; > - > - > - acpi_walk_namespace(ACPI_TYPE_DEVICE, > - ACPI_ROOT_OBJECT, > - ACPI_UINT32_MAX, > - container_walk_namespace_cb, NULL, &action, NULL); > - > - acpi_bus_unregister_driver(&acpi_container_driver); > - > - return; > + acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, ACPI_UINT32_MAX, > + acpi_container_register_notify_handler, NULL, > + NULL, NULL); > } > - > -module_init(acpi_container_init); > -module_exit(acpi_container_exit); > Index: test/drivers/acpi/Kconfig > =================================================================== > --- test.orig/drivers/acpi/Kconfig > +++ test/drivers/acpi/Kconfig > @@ -334,7 +334,7 @@ config X86_PM_TIMER > systems require this timer. > > config ACPI_CONTAINER > - tristate "Container and Module Devices (EXPERIMENTAL)" > + bool "Container and Module Devices (EXPERIMENTAL)" > depends on EXPERIMENTAL > default (ACPI_HOTPLUG_MEMORY || ACPI_HOTPLUG_CPU || ACPI_HOTPLUG_IO) > help > Index: test/drivers/acpi/internal.h > =================================================================== > --- test.orig/drivers/acpi/internal.h > +++ test/drivers/acpi/internal.h > @@ -40,6 +40,11 @@ void acpi_memory_hotplug_init(void); > #else > static inline void acpi_memory_hotplug_init(void) {} > #endif > +#ifdef ACPI_CONTAINER It should be CONFIG_ACPI_CONTAINER. By this, acpi_container_init() do nothing. When I fix it and test the patch, the patch goes well. If you update the patch, I'll test again. Thanks, Yasuaki Ishimatsu > +void acpi_container_init(void); > +#else > +static inline void acpi_container_init(void) {} > +#endif > > #ifdef CONFIG_DEBUG_FS > extern struct dentry *acpi_debugfs_dir; > Index: test/drivers/acpi/scan.c > =================================================================== > --- test.orig/drivers/acpi/scan.c > +++ test/drivers/acpi/scan.c > @@ -1763,6 +1763,7 @@ int __init acpi_scan_init(void) > acpi_platform_init(); > acpi_csrt_init(); > acpi_memory_hotplug_init(); > + acpi_container_init(); > > /* > * Enumerate devices in the ACPI namespace. > > -- > 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 > -- 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
On Thursday, February 07, 2013 05:32:15 PM Yasuaki Ishimatsu wrote: > Hi Rafael, > > Sorry for late reply. No problem, thank you for the review. > 2013/02/04 8:47, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > The only useful thing that the ACPI container driver does is to > > install system notify handlers for all container and module device > > objects it finds in the namespace. The driver structure, > > acpi_container_driver, and the data structures created by its > > .add() callback are in fact not used by the driver, so remove > > them entirely. > > > > It also makes a little sense to build that driver as a module, > > so make it non-modular and add its initialization to the > > namespace scanning code. > > > > In addition to that, make the namespace walk callback used for > > installing the notify handlers more straightforward. > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > drivers/acpi/Kconfig | 2 > > drivers/acpi/container.c | 158 ++++++----------------------------------------- > > drivers/acpi/internal.h | 5 + > > drivers/acpi/scan.c | 1 > > 4 files changed, 30 insertions(+), 136 deletions(-) > > > > Index: test/drivers/acpi/container.c > > =================================================================== > > --- test.orig/drivers/acpi/container.c > > +++ test/drivers/acpi/container.c > > @@ -38,41 +38,15 @@ > > > > #define PREFIX "ACPI: " > > > > -#define ACPI_CONTAINER_DEVICE_NAME "ACPI container device" > > -#define ACPI_CONTAINER_CLASS "container" > > - > > -#define INSTALL_NOTIFY_HANDLER 1 > > -#define UNINSTALL_NOTIFY_HANDLER 2 > > - > > #define _COMPONENT ACPI_CONTAINER_COMPONENT > > ACPI_MODULE_NAME("container"); > > > > -MODULE_AUTHOR("Anil S Keshavamurthy"); > > -MODULE_DESCRIPTION("ACPI container driver"); > > -MODULE_LICENSE("GPL"); > > - > > -static int acpi_container_add(struct acpi_device *device); > > -static int acpi_container_remove(struct acpi_device *device); > > - > > static const struct acpi_device_id container_device_ids[] = { > > {"ACPI0004", 0}, > > {"PNP0A05", 0}, > > {"PNP0A06", 0}, > > {"", 0}, > > }; > > -MODULE_DEVICE_TABLE(acpi, container_device_ids); > > - > > -static struct acpi_driver acpi_container_driver = { > > - .name = "container", > > - .class = ACPI_CONTAINER_CLASS, > > - .ids = container_device_ids, > > - .ops = { > > - .add = acpi_container_add, > > - .remove = acpi_container_remove, > > - }, > > -}; > > - > > -/*******************************************************************/ > > > > static int is_device_present(acpi_handle handle) > > { > > @@ -92,49 +66,6 @@ static int is_device_present(acpi_handle > > return ((sta & ACPI_STA_DEVICE_PRESENT) == ACPI_STA_DEVICE_PRESENT); > > } > > > > -static bool is_container_device(const char *hid) > > -{ > > - const struct acpi_device_id *container_id; > > - > > - for (container_id = container_device_ids; > > - container_id->id[0]; container_id++) { > > - if (!strcmp((char *)container_id->id, hid)) > > - return true; > > - } > > - > > - return false; > > -} > > - > > -/*******************************************************************/ > > -static int acpi_container_add(struct acpi_device *device) > > -{ > > - struct acpi_container *container; > > - > > - container = kzalloc(sizeof(struct acpi_container), GFP_KERNEL); > > - if (!container) > > - return -ENOMEM; > > - > > - container->handle = device->handle; > > - strcpy(acpi_device_name(device), ACPI_CONTAINER_DEVICE_NAME); > > - strcpy(acpi_device_class(device), ACPI_CONTAINER_CLASS); > > - device->driver_data = container; > > - > > - ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Device <%s> bid <%s>\n", > > - acpi_device_name(device), acpi_device_bid(device))); > > - > > - return 0; > > -} > > - > > -static int acpi_container_remove(struct acpi_device *device) > > -{ > > - acpi_status status = AE_OK; > > - struct acpi_container *pc = NULL; > > - > > - pc = acpi_driver_data(device); > > - kfree(pc); > > - return status; > > -} > > - > > static void container_notify_cb(acpi_handle handle, u32 type, void *context) > > { > > struct acpi_device *device = NULL; > > @@ -199,84 +130,41 @@ static void container_notify_cb(acpi_han > > return; > > } > > > > -static acpi_status > > -container_walk_namespace_cb(acpi_handle handle, > > - u32 lvl, void *context, void **rv) > > +static bool is_container(acpi_handle handle) > > { > > - char *hid = NULL; > > struct acpi_device_info *info; > > - acpi_status status; > > - int *action = context; > > + bool ret = false; > > > > - status = acpi_get_object_info(handle, &info); > > - if (ACPI_FAILURE(status)) { > > - return AE_OK; > > - } > > - > > - if (info->valid & ACPI_VALID_HID) > > - hid = info->hardware_id.string; > > - > > - if (hid == NULL) { > > - goto end; > > - } > > + if (ACPI_FAILURE(acpi_get_object_info(handle, &info))) > > + return false; > > > > - if (!is_container_device(hid)) > > - goto end; > > + if (info->valid & ACPI_VALID_HID) { > > + const struct acpi_device_id *id; > > > > - switch (*action) { > > - case INSTALL_NOTIFY_HANDLER: > > - acpi_install_notify_handler(handle, > > - ACPI_SYSTEM_NOTIFY, > > - container_notify_cb, NULL); > > - break; > > - case UNINSTALL_NOTIFY_HANDLER: > > - acpi_remove_notify_handler(handle, > > - ACPI_SYSTEM_NOTIFY, > > - container_notify_cb); > > - break; > > - default: > > - break; > > + for (id = container_device_ids; id->id[0]; id++) { > > + ret = !strcmp((char *)id->id, info->hardware_id.string); > > + if (ret) > > + break; > > + } > > } > > - > > - end: > > kfree(info); > > - > > - return AE_OK; > > + return ret; > > } > > > > -static int __init acpi_container_init(void) > > +static acpi_status acpi_container_register_notify_handler(acpi_handle handle, > > + u32 lvl, void *ctxt, > > + void **retv) > > { > > - int result = 0; > > - int action = INSTALL_NOTIFY_HANDLER; > > - > > - result = acpi_bus_register_driver(&acpi_container_driver); > > - if (result < 0) { > > - return (result); > > - } > > - > > - /* register notify handler to every container device */ > > - acpi_walk_namespace(ACPI_TYPE_DEVICE, > > - ACPI_ROOT_OBJECT, > > - ACPI_UINT32_MAX, > > - container_walk_namespace_cb, NULL, &action, NULL); > > + if (is_container(handle)) > > + acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY, > > + container_notify_cb, NULL); > > > > - return (0); > > + return AE_OK; > > } > > > > -static void __exit acpi_container_exit(void) > > +void __init acpi_container_init(void) > > { > > - int action = UNINSTALL_NOTIFY_HANDLER; > > - > > - > > - acpi_walk_namespace(ACPI_TYPE_DEVICE, > > - ACPI_ROOT_OBJECT, > > - ACPI_UINT32_MAX, > > - container_walk_namespace_cb, NULL, &action, NULL); > > - > > - acpi_bus_unregister_driver(&acpi_container_driver); > > - > > - return; > > + acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, ACPI_UINT32_MAX, > > + acpi_container_register_notify_handler, NULL, > > + NULL, NULL); > > } > > - > > -module_init(acpi_container_init); > > -module_exit(acpi_container_exit); > > Index: test/drivers/acpi/Kconfig > > =================================================================== > > --- test.orig/drivers/acpi/Kconfig > > +++ test/drivers/acpi/Kconfig > > @@ -334,7 +334,7 @@ config X86_PM_TIMER > > systems require this timer. > > > > config ACPI_CONTAINER > > - tristate "Container and Module Devices (EXPERIMENTAL)" > > + bool "Container and Module Devices (EXPERIMENTAL)" > > depends on EXPERIMENTAL > > default (ACPI_HOTPLUG_MEMORY || ACPI_HOTPLUG_CPU || ACPI_HOTPLUG_IO) > > help > > Index: test/drivers/acpi/internal.h > > =================================================================== > > --- test.orig/drivers/acpi/internal.h > > +++ test/drivers/acpi/internal.h > > @@ -40,6 +40,11 @@ void acpi_memory_hotplug_init(void); > > #else > > static inline void acpi_memory_hotplug_init(void) {} > > #endif > > > +#ifdef ACPI_CONTAINER > > It should be CONFIG_ACPI_CONTAINER. Totally correct. > By this, acpi_container_init() do nothing. When I fix it and test the patch, > the patch goes well. > > If you update the patch, I'll test again. This fix from Toshi Kani is necessary in addition to it, though https://patchwork.kernel.org/patch/2108851/ because FORCE_EJECT is never set as far as I can say. Thanks, Rafael
On Thu, 2013-02-07 at 02:32 +0100, Rafael J. Wysocki wrote: > On Wednesday, February 06, 2013 05:51:42 PM Toshi Kani wrote: > > On Thu, 2013-02-07 at 01:55 +0100, Rafael J. Wysocki wrote: > > > On Wednesday, February 06, 2013 03:32:18 PM Toshi Kani wrote: > > > > On Mon, 2013-02-04 at 00:47 +0100, Rafael J. Wysocki wrote: > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > > > The only useful thing that the ACPI container driver does is to > > > > > install system notify handlers for all container and module device > > > > > objects it finds in the namespace. The driver structure, > > > > > acpi_container_driver, and the data structures created by its > > > > > .add() callback are in fact not used by the driver, so remove > > > > > them entirely. > > > > > > > > > > It also makes a little sense to build that driver as a module, > > > > > so make it non-modular and add its initialization to the > > > > > namespace scanning code. > > > > > > > > > > In addition to that, make the namespace walk callback used for > > > > > installing the notify handlers more straightforward. > > > > > > > > I think the container driver needs to be registered as an ACPI scan > > > > driver so that sysfs eject will continue to work for container devices, > > > > such as ACPI0004:XX/eject. Since the container driver does not support > > > > ACPI eject notification (and we have been discussing how system device > > > > hot-plug should work), this sysfs eject is the only way to eject a > > > > container device at this point. I will send an update patchset that > > > > applies on top of this patch. > > > > > > > > With the update in my patchset: > > > > Reviewed-by: Toshi Kani <toshi.kani@hp.com> > > > > > > Thanks, but I'd like to (1) apply your patch from > > > https://patchwork.kernel.org/patch/2108851/ first and then (2) fold your [2/2] > > > into my [2/2], if you don't mind, and apply that next. > > > > That's fine by me. > > > > > Moreover, I'm wondering if the #ifndef FORCE_EJECT thing in acpi_eject_store() > > > actually makes sense after the recent changes to acpi_bus_trim(), because that > > > can't fail now, so the eject will always be carried out. So perhaps we can > > > simply remove the acpi_device->driver check from there entirely in the first > > > place? > > > > > > If we really want to be able to prevent ejects from happening in some cases, > > > we need to implement something along the lines discussed with Greg. > > > > acpi_bus_trim() cannot fail, but sysfs eject can fail. So, I think it > > makes sense to do some validation before calling acpi_bus_trim(). If we > > are to implement the no_eject flag thing, that check needs to be made > > before calling acpi_bus_trim(). > > Sure, but now the logic seems to be "if FORCE_EJECT is not set, don't eject > devices that have no ACPI drivers", so I'm wondering what the purpose of this > is. It definitely isn't too obvious. :-) The check sounds odd for container, but is necessary for CPU and memory for now. CPU and memory go online without their ACPI drivers at boot. So, without this check (i.e. FORCE_EJECT is set), it simply ejects them without attempting to offline when the ACPI drivers are not bound. Of course, we have the issue of a failure in offline be ignored, so this offlining part needs to be moved out from acpi_bus_trim() in one way or the other. Thanks, -Toshi -- 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
On Thu, 2013-02-07 at 12:43 +0100, Rafael J. Wysocki wrote: > On Thursday, February 07, 2013 05:32:15 PM Yasuaki Ishimatsu wrote: : > > > Index: test/drivers/acpi/internal.h > > > =================================================================== > > > --- test.orig/drivers/acpi/internal.h > > > +++ test/drivers/acpi/internal.h > > > @@ -40,6 +40,11 @@ void acpi_memory_hotplug_init(void); > > > #else > > > static inline void acpi_memory_hotplug_init(void) {} > > > #endif > > > > > +#ifdef ACPI_CONTAINER > > > > It should be CONFIG_ACPI_CONTAINER. > > Totally correct. Just FYI, this change is included in my patch 2/2 as well. https://patchwork.kernel.org/patch/2108881/ Thanks, -Toshi > > By this, acpi_container_init() do nothing. When I fix it and test the patch, > > the patch goes well. > > > > If you update the patch, I'll test again. > > This fix from Toshi Kani is necessary in addition to it, though > > https://patchwork.kernel.org/patch/2108851/ > > because FORCE_EJECT is never set as far as I can say. > > Thanks, > Rafael > > > -- 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
On Thursday, February 07, 2013 07:32:07 AM Toshi Kani wrote: > On Thu, 2013-02-07 at 02:32 +0100, Rafael J. Wysocki wrote: > > On Wednesday, February 06, 2013 05:51:42 PM Toshi Kani wrote: > > > On Thu, 2013-02-07 at 01:55 +0100, Rafael J. Wysocki wrote: > > > > On Wednesday, February 06, 2013 03:32:18 PM Toshi Kani wrote: > > > > > On Mon, 2013-02-04 at 00:47 +0100, Rafael J. Wysocki wrote: > > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > > > > > The only useful thing that the ACPI container driver does is to > > > > > > install system notify handlers for all container and module device > > > > > > objects it finds in the namespace. The driver structure, > > > > > > acpi_container_driver, and the data structures created by its > > > > > > .add() callback are in fact not used by the driver, so remove > > > > > > them entirely. > > > > > > > > > > > > It also makes a little sense to build that driver as a module, > > > > > > so make it non-modular and add its initialization to the > > > > > > namespace scanning code. > > > > > > > > > > > > In addition to that, make the namespace walk callback used for > > > > > > installing the notify handlers more straightforward. > > > > > > > > > > I think the container driver needs to be registered as an ACPI scan > > > > > driver so that sysfs eject will continue to work for container devices, > > > > > such as ACPI0004:XX/eject. Since the container driver does not support > > > > > ACPI eject notification (and we have been discussing how system device > > > > > hot-plug should work), this sysfs eject is the only way to eject a > > > > > container device at this point. I will send an update patchset that > > > > > applies on top of this patch. > > > > > > > > > > With the update in my patchset: > > > > > Reviewed-by: Toshi Kani <toshi.kani@hp.com> > > > > > > > > Thanks, but I'd like to (1) apply your patch from > > > > https://patchwork.kernel.org/patch/2108851/ first and then (2) fold your [2/2] > > > > into my [2/2], if you don't mind, and apply that next. > > > > > > That's fine by me. > > > > > > > Moreover, I'm wondering if the #ifndef FORCE_EJECT thing in acpi_eject_store() > > > > actually makes sense after the recent changes to acpi_bus_trim(), because that > > > > can't fail now, so the eject will always be carried out. So perhaps we can > > > > simply remove the acpi_device->driver check from there entirely in the first > > > > place? > > > > > > > > If we really want to be able to prevent ejects from happening in some cases, > > > > we need to implement something along the lines discussed with Greg. > > > > > > acpi_bus_trim() cannot fail, but sysfs eject can fail. So, I think it > > > makes sense to do some validation before calling acpi_bus_trim(). If we > > > are to implement the no_eject flag thing, that check needs to be made > > > before calling acpi_bus_trim(). > > > > Sure, but now the logic seems to be "if FORCE_EJECT is not set, don't eject > > devices that have no ACPI drivers", so I'm wondering what the purpose of this > > is. It definitely isn't too obvious. :-) > > The check sounds odd for container, but is necessary for CPU and memory > for now. CPU and memory go online without their ACPI drivers at boot. > So, without this check (i.e. FORCE_EJECT is set), it simply ejects them > without attempting to offline when the ACPI drivers are not bound. Of > course, we have the issue of a failure in offline be ignored, so this > offlining part needs to be moved out from acpi_bus_trim() in one way or > the other. That was my point. I'm going to add that change for now, but I think we need to take a step back and talk about how we want the whole eject machinery to work, regardless of the offline/online problem. I think that it should work in the same way for all things that may be ejected or inserted. Namely, they all should use the same notify handler, for example, and if we generate a uevent for one, we should do that for all of them. Question is how that notify handler should work and here there are two chices in my view: Either we'll always emit a uevent and wait for user space to start the eject procedure via sysfs, or we won't emit uevents at all and rely on the "no_eject" flag to trigger if something is not ready. I'm basically fine with any of them (the "no_eject" flag may be useful even if we rely on user space to offline stuff before triggering the eject in my opinion), but if we're going to rely on user space, then there needs to be a timeout for letting the BIOS know that the eject has failed. [There may be a flag for the common code telling it whether to emit a uevent and wait for user space to trigger eject or to trigger eject by itself.] Next, I think there needs to be a global list of IDs for which we'll install hot-plug notify handlers and which we'll allow to be ejected via /sys/.../eject. So, if a device ID is on that list, we'll install the (common) hot-plug notify handler for its ACPI handle and we'll set an "eject_possible" flag in its struct acpi_device (when created). That will need to be done for every scan of the ACPI namespace and not just once, BTW. And we'll check the "eject_possible" flag in acpi_eject_store() instead of the "does it have a driver or scan handler" check. Then, the scan handlers for hot-plug devices will be able to add their IDs to that global list instead of walking the namespace and installing notify handlers by themselves (which as I said has a problem that it's done once, while it should be done every time acpi_bus_scan() runs). I wonder what you think. Thanks, Rafael
On Monday, February 04, 2013 12:47:31 AM Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > The only useful thing that the ACPI container driver does is to > install system notify handlers for all container and module device > objects it finds in the namespace. The driver structure, > acpi_container_driver, and the data structures created by its > .add() callback are in fact not used by the driver, so remove > them entirely. > > It also makes a little sense to build that driver as a module, > so make it non-modular and add its initialization to the > namespace scanning code. > > In addition to that, make the namespace walk callback used for > installing the notify handlers more straightforward. As pointed out by Toshi Kani, the above changes would make acpi_eject_store() fail for containers and it is the only way to eject them currently, so patch [2/2] is an improved version of this (with Toshi's changes folded in). Patch [1/2] is just a cleanup removing a useless #ifndef from acpi_eject_store(). Thanks, Rafael
On Thu, 2013-02-07 at 23:42 +0100, Rafael J. Wysocki wrote: > On Thursday, February 07, 2013 07:32:07 AM Toshi Kani wrote: > > On Thu, 2013-02-07 at 02:32 +0100, Rafael J. Wysocki wrote: > > > On Wednesday, February 06, 2013 05:51:42 PM Toshi Kani wrote: > > > > On Thu, 2013-02-07 at 01:55 +0100, Rafael J. Wysocki wrote: > > > > > On Wednesday, February 06, 2013 03:32:18 PM Toshi Kani wrote: > > > > > > On Mon, 2013-02-04 at 00:47 +0100, Rafael J. Wysocki wrote: : > > > > > Moreover, I'm wondering if the #ifndef FORCE_EJECT thing in acpi_eject_store() > > > > > actually makes sense after the recent changes to acpi_bus_trim(), because that > > > > > can't fail now, so the eject will always be carried out. So perhaps we can > > > > > simply remove the acpi_device->driver check from there entirely in the first > > > > > place? > > > > > > > > > > If we really want to be able to prevent ejects from happening in some cases, > > > > > we need to implement something along the lines discussed with Greg. > > > > > > > > acpi_bus_trim() cannot fail, but sysfs eject can fail. So, I think it > > > > makes sense to do some validation before calling acpi_bus_trim(). If we > > > > are to implement the no_eject flag thing, that check needs to be made > > > > before calling acpi_bus_trim(). > > > > > > Sure, but now the logic seems to be "if FORCE_EJECT is not set, don't eject > > > devices that have no ACPI drivers", so I'm wondering what the purpose of this > > > is. It definitely isn't too obvious. :-) > > > > The check sounds odd for container, but is necessary for CPU and memory > > for now. CPU and memory go online without their ACPI drivers at boot. > > So, without this check (i.e. FORCE_EJECT is set), it simply ejects them > > without attempting to offline when the ACPI drivers are not bound. Of > > course, we have the issue of a failure in offline be ignored, so this > > offlining part needs to be moved out from acpi_bus_trim() in one way or > > the other. > > That was my point. > > I'm going to add that change for now, but I think we need to take a step back > and talk about how we want the whole eject machinery to work, regardless of > the offline/online problem. Right. > I think that it should work in the same way for all things that may be ejected > or inserted. Namely, they all should use the same notify handler, for example, > and if we generate a uevent for one, we should do that for all of them. Agreed. > Question is how that notify handler should work and here there are two chices > in my view: Either we'll always emit a uevent and wait for user space to start > the eject procedure via sysfs, or we won't emit uevents at all and rely on the > "no_eject" flag to trigger if something is not ready. I'm basically fine with > any of them (the "no_eject" flag may be useful even if we rely on user space > to offline stuff before triggering the eject in my opinion), but if we're going > to rely on user space, then there needs to be a timeout for letting the BIOS > know that the eject has failed. > > [There may be a flag for the common code telling it whether to emit a uevent > and wait for user space to trigger eject or to trigger eject by itself.] IMHO, the kernel waiting for a user program to complete is a recipe for future problems. So, I think two possible implementation choices are: 1. Upon an eject request, off-line all devices and eject => Implement a kernel sequencer (my RFC patchset) 2. Upon an eject request, eject if all devices are off-lined beforehand => Implement the "no_eject" approach Since we are heading to the user space approach, we need to go with #2. There are some challenges with #2, ex. if sysfs memory online/offline interfaces can correspond with ACPI memory objects, which we will also need to look into. > Next, I think there needs to be a global list of IDs for which we'll install > hot-plug notify handlers and which we'll allow to be ejected via /sys/.../eject. > So, if a device ID is on that list, we'll install the (common) hot-plug notify > handler for its ACPI handle and we'll set an "eject_possible" flag in its > struct acpi_device (when created). That will need to be done for every scan > of the ACPI namespace and not just once, BTW. And we'll check the > "eject_possible" flag in acpi_eject_store() instead of the "does it have a > driver or scan handler" check. > > Then, the scan handlers for hot-plug devices will be able to add their IDs to > that global list instead of walking the namespace and installing notify handlers > by themselves (which as I said has a problem that it's done once, while it > should be done every time acpi_bus_scan() runs). I agree. I think we can use the global notify handler (as the common notify handler) to look up the eject_possible ID list, instead of installing a notify handler to each device. Thanks, -Toshi -- 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
Hi Rafael, 2013/02/08 9:24, Rafael J. Wysocki wrote: > On Monday, February 04, 2013 12:47:31 AM Rafael J. Wysocki wrote: >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> >> The only useful thing that the ACPI container driver does is to >> install system notify handlers for all container and module device >> objects it finds in the namespace. The driver structure, >> acpi_container_driver, and the data structures created by its >> .add() callback are in fact not used by the driver, so remove >> them entirely. >> >> It also makes a little sense to build that driver as a module, >> so make it non-modular and add its initialization to the >> namespace scanning code. >> >> In addition to that, make the namespace walk callback used for >> installing the notify handlers more straightforward. > > As pointed out by Toshi Kani, the above changes would make acpi_eject_store() > fail for containers and it is the only way to eject them currently, so patch > [2/2] is an improved version of this (with Toshi's changes folded in). > > Patch [1/2] is just a cleanup removing a useless #ifndef from acpi_eject_store(). I confimed the patch series works well. Acked-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com> Tested-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com> Thanks, Yasuaki Ishimatsu > > Thanks, > Rafael > > -- 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
On Friday, February 08, 2013 12:19:35 PM Yasuaki Ishimatsu wrote: > Hi Rafael, > > 2013/02/08 9:24, Rafael J. Wysocki wrote: > > On Monday, February 04, 2013 12:47:31 AM Rafael J. Wysocki wrote: > >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >> > >> The only useful thing that the ACPI container driver does is to > >> install system notify handlers for all container and module device > >> objects it finds in the namespace. The driver structure, > >> acpi_container_driver, and the data structures created by its > >> .add() callback are in fact not used by the driver, so remove > >> them entirely. > >> > >> It also makes a little sense to build that driver as a module, > >> so make it non-modular and add its initialization to the > >> namespace scanning code. > >> > >> In addition to that, make the namespace walk callback used for > >> installing the notify handlers more straightforward. > > > > As pointed out by Toshi Kani, the above changes would make acpi_eject_store() > > fail for containers and it is the only way to eject them currently, so patch > > [2/2] is an improved version of this (with Toshi's changes folded in). > > > > Patch [1/2] is just a cleanup removing a useless #ifndef from acpi_eject_store(). > > I confimed the patch series works well. > > Acked-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com> > Tested-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com> Thanks a lot! Rafael
On Thursday, February 07, 2013 06:05:19 PM Toshi Kani wrote: > On Thu, 2013-02-07 at 23:42 +0100, Rafael J. Wysocki wrote: > > On Thursday, February 07, 2013 07:32:07 AM Toshi Kani wrote: > > > On Thu, 2013-02-07 at 02:32 +0100, Rafael J. Wysocki wrote: > > > > On Wednesday, February 06, 2013 05:51:42 PM Toshi Kani wrote: > > > > > On Thu, 2013-02-07 at 01:55 +0100, Rafael J. Wysocki wrote: > > > > > > On Wednesday, February 06, 2013 03:32:18 PM Toshi Kani wrote: > > > > > > > On Mon, 2013-02-04 at 00:47 +0100, Rafael J. Wysocki wrote: > : > > > > > > Moreover, I'm wondering if the #ifndef FORCE_EJECT thing in acpi_eject_store() > > > > > > actually makes sense after the recent changes to acpi_bus_trim(), because that > > > > > > can't fail now, so the eject will always be carried out. So perhaps we can > > > > > > simply remove the acpi_device->driver check from there entirely in the first > > > > > > place? > > > > > > > > > > > > If we really want to be able to prevent ejects from happening in some cases, > > > > > > we need to implement something along the lines discussed with Greg. > > > > > > > > > > acpi_bus_trim() cannot fail, but sysfs eject can fail. So, I think it > > > > > makes sense to do some validation before calling acpi_bus_trim(). If we > > > > > are to implement the no_eject flag thing, that check needs to be made > > > > > before calling acpi_bus_trim(). > > > > > > > > Sure, but now the logic seems to be "if FORCE_EJECT is not set, don't eject > > > > devices that have no ACPI drivers", so I'm wondering what the purpose of this > > > > is. It definitely isn't too obvious. :-) > > > > > > The check sounds odd for container, but is necessary for CPU and memory > > > for now. CPU and memory go online without their ACPI drivers at boot. > > > So, without this check (i.e. FORCE_EJECT is set), it simply ejects them > > > without attempting to offline when the ACPI drivers are not bound. Of > > > course, we have the issue of a failure in offline be ignored, so this > > > offlining part needs to be moved out from acpi_bus_trim() in one way or > > > the other. > > > > That was my point. > > > > I'm going to add that change for now, but I think we need to take a step back > > and talk about how we want the whole eject machinery to work, regardless of > > the offline/online problem. > > Right. > > > I think that it should work in the same way for all things that may be ejected > > or inserted. Namely, they all should use the same notify handler, for example, > > and if we generate a uevent for one, we should do that for all of them. > > Agreed. > > > Question is how that notify handler should work and here there are two chices > > in my view: Either we'll always emit a uevent and wait for user space to start > > the eject procedure via sysfs, or we won't emit uevents at all and rely on the > > "no_eject" flag to trigger if something is not ready. I'm basically fine with > > any of them (the "no_eject" flag may be useful even if we rely on user space > > to offline stuff before triggering the eject in my opinion), but if we're going > > to rely on user space, then there needs to be a timeout for letting the BIOS > > know that the eject has failed. > > > > [There may be a flag for the common code telling it whether to emit a uevent > > and wait for user space to trigger eject or to trigger eject by itself.] > > IMHO, the kernel waiting for a user program to complete is a recipe for > future problems. So, I think two possible implementation choices are: > > 1. Upon an eject request, off-line all devices and eject > => Implement a kernel sequencer (my RFC patchset) > > 2. Upon an eject request, eject if all devices are off-lined beforehand > => Implement the "no_eject" approach > > Since we are heading to the user space approach, we need to go with #2. > There are some challenges with #2, ex. if sysfs memory online/offline > interfaces can correspond with ACPI memory objects, which we will also > need to look into. Sure. > > Next, I think there needs to be a global list of IDs for which we'll install > > hot-plug notify handlers and which we'll allow to be ejected via /sys/.../eject. > > So, if a device ID is on that list, we'll install the (common) hot-plug notify > > handler for its ACPI handle and we'll set an "eject_possible" flag in its > > struct acpi_device (when created). That will need to be done for every scan > > of the ACPI namespace and not just once, BTW. And we'll check the > > "eject_possible" flag in acpi_eject_store() instead of the "does it have a > > driver or scan handler" check. > > > > Then, the scan handlers for hot-plug devices will be able to add their IDs to > > that global list instead of walking the namespace and installing notify handlers > > by themselves (which as I said has a problem that it's done once, while it > > should be done every time acpi_bus_scan() runs). > > I agree. I think we can use the global notify handler (as the common > notify handler) to look up the eject_possible ID list, instead of > installing a notify handler to each device. Actually installing notify handlers for individual devices may be more efficient, because then we'll know what device the notification is for without looking it up. OK, so it looks like I need to sit down and cut some more patches. :-) Thanks, Rafael
On Fri, 2013-02-08 at 13:52 +0100, Rafael J. Wysocki wrote: > On Thursday, February 07, 2013 06:05:19 PM Toshi Kani wrote: > > On Thu, 2013-02-07 at 23:42 +0100, Rafael J. Wysocki wrote: > > > On Thursday, February 07, 2013 07:32:07 AM Toshi Kani wrote: > > > > On Thu, 2013-02-07 at 02:32 +0100, Rafael J. Wysocki wrote: > > > > > On Wednesday, February 06, 2013 05:51:42 PM Toshi Kani wrote: > > > > > > On Thu, 2013-02-07 at 01:55 +0100, Rafael J. Wysocki wrote: > > > > > > > On Wednesday, February 06, 2013 03:32:18 PM Toshi Kani wrote: > > > > > > > > On Mon, 2013-02-04 at 00:47 +0100, Rafael J. Wysocki wrote: > > : > > > > > > > Moreover, I'm wondering if the #ifndef FORCE_EJECT thing in acpi_eject_store() > > > > > > > actually makes sense after the recent changes to acpi_bus_trim(), because that > > > > > > > can't fail now, so the eject will always be carried out. So perhaps we can > > > > > > > simply remove the acpi_device->driver check from there entirely in the first > > > > > > > place? > > > > > > > > > > > > > > If we really want to be able to prevent ejects from happening in some cases, > > > > > > > we need to implement something along the lines discussed with Greg. > > > > > > > > > > > > acpi_bus_trim() cannot fail, but sysfs eject can fail. So, I think it > > > > > > makes sense to do some validation before calling acpi_bus_trim(). If we > > > > > > are to implement the no_eject flag thing, that check needs to be made > > > > > > before calling acpi_bus_trim(). > > > > > > > > > > Sure, but now the logic seems to be "if FORCE_EJECT is not set, don't eject > > > > > devices that have no ACPI drivers", so I'm wondering what the purpose of this > > > > > is. It definitely isn't too obvious. :-) > > > > > > > > The check sounds odd for container, but is necessary for CPU and memory > > > > for now. CPU and memory go online without their ACPI drivers at boot. > > > > So, without this check (i.e. FORCE_EJECT is set), it simply ejects them > > > > without attempting to offline when the ACPI drivers are not bound. Of > > > > course, we have the issue of a failure in offline be ignored, so this > > > > offlining part needs to be moved out from acpi_bus_trim() in one way or > > > > the other. > > > > > > That was my point. > > > > > > I'm going to add that change for now, but I think we need to take a step back > > > and talk about how we want the whole eject machinery to work, regardless of > > > the offline/online problem. > > > > Right. > > > > > I think that it should work in the same way for all things that may be ejected > > > or inserted. Namely, they all should use the same notify handler, for example, > > > and if we generate a uevent for one, we should do that for all of them. > > > > Agreed. > > > > > Question is how that notify handler should work and here there are two chices > > > in my view: Either we'll always emit a uevent and wait for user space to start > > > the eject procedure via sysfs, or we won't emit uevents at all and rely on the > > > "no_eject" flag to trigger if something is not ready. I'm basically fine with > > > any of them (the "no_eject" flag may be useful even if we rely on user space > > > to offline stuff before triggering the eject in my opinion), but if we're going > > > to rely on user space, then there needs to be a timeout for letting the BIOS > > > know that the eject has failed. > > > > > > [There may be a flag for the common code telling it whether to emit a uevent > > > and wait for user space to trigger eject or to trigger eject by itself.] > > > > IMHO, the kernel waiting for a user program to complete is a recipe for > > future problems. So, I think two possible implementation choices are: > > > > 1. Upon an eject request, off-line all devices and eject > > => Implement a kernel sequencer (my RFC patchset) > > > > 2. Upon an eject request, eject if all devices are off-lined beforehand > > => Implement the "no_eject" approach > > > > Since we are heading to the user space approach, we need to go with #2. > > There are some challenges with #2, ex. if sysfs memory online/offline > > interfaces can correspond with ACPI memory objects, which we will also > > need to look into. > > Sure. > > > > Next, I think there needs to be a global list of IDs for which we'll install > > > hot-plug notify handlers and which we'll allow to be ejected via /sys/.../eject. > > > So, if a device ID is on that list, we'll install the (common) hot-plug notify > > > handler for its ACPI handle and we'll set an "eject_possible" flag in its > > > struct acpi_device (when created). That will need to be done for every scan > > > of the ACPI namespace and not just once, BTW. And we'll check the > > > "eject_possible" flag in acpi_eject_store() instead of the "does it have a > > > driver or scan handler" check. > > > > > > Then, the scan handlers for hot-plug devices will be able to add their IDs to > > > that global list instead of walking the namespace and installing notify handlers > > > by themselves (which as I said has a problem that it's done once, while it > > > should be done every time acpi_bus_scan() runs). > > > > I agree. I think we can use the global notify handler (as the common > > notify handler) to look up the eject_possible ID list, instead of > > installing a notify handler to each device. > > Actually installing notify handlers for individual devices may be more > efficient, because then we'll know what device the notification is for without > looking it up. Right. I am fine with either way as long as we can get rid of the scanning from each driver and support dynamic ACPI namespace. > OK, so it looks like I need to sit down and cut some more patches. :-) Cool! Thanks, -Toshi -- 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
On Fri, 2013-02-08 at 01:24 +0100, Rafael J. Wysocki wrote: > On Monday, February 04, 2013 12:47:31 AM Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > The only useful thing that the ACPI container driver does is to > > install system notify handlers for all container and module device > > objects it finds in the namespace. The driver structure, > > acpi_container_driver, and the data structures created by its > > .add() callback are in fact not used by the driver, so remove > > them entirely. > > > > It also makes a little sense to build that driver as a module, > > so make it non-modular and add its initialization to the > > namespace scanning code. > > > > In addition to that, make the namespace walk callback used for > > installing the notify handlers more straightforward. > > As pointed out by Toshi Kani, the above changes would make acpi_eject_store() > fail for containers and it is the only way to eject them currently, so patch > [2/2] is an improved version of this (with Toshi's changes folded in). > > Patch [1/2] is just a cleanup removing a useless #ifndef from acpi_eject_store(). Thanks for the update! They look good. For the series: Reviewed-by: Toshi Kani <toshi.kani@hp.com> Tested-by: Toshi Kani <toshi.kani@hp.com> BTW, did you intentionally keep the struct acpi_container definition in <acpi/container.h>? I deleted that one in my patch. -Toshi -- 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
On Friday, February 08, 2013 09:57:18 AM Toshi Kani wrote: > On Fri, 2013-02-08 at 01:24 +0100, Rafael J. Wysocki wrote: > > On Monday, February 04, 2013 12:47:31 AM Rafael J. Wysocki wrote: > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > The only useful thing that the ACPI container driver does is to > > > install system notify handlers for all container and module device > > > objects it finds in the namespace. The driver structure, > > > acpi_container_driver, and the data structures created by its > > > .add() callback are in fact not used by the driver, so remove > > > them entirely. > > > > > > It also makes a little sense to build that driver as a module, > > > so make it non-modular and add its initialization to the > > > namespace scanning code. > > > > > > In addition to that, make the namespace walk callback used for > > > installing the notify handlers more straightforward. > > > > As pointed out by Toshi Kani, the above changes would make acpi_eject_store() > > fail for containers and it is the only way to eject them currently, so patch > > [2/2] is an improved version of this (with Toshi's changes folded in). > > > > Patch [1/2] is just a cleanup removing a useless #ifndef from acpi_eject_store(). > > Thanks for the update! They look good. For the series: > > Reviewed-by: Toshi Kani <toshi.kani@hp.com> > Tested-by: Toshi Kani <toshi.kani@hp.com> > > BTW, did you intentionally keep the struct acpi_container definition in > <acpi/container.h>? I deleted that one in my patch. No, I overlooked that part. I'll apply that part of your patch as a separate patch on top of this one. Thanks, Rafael
On Friday, February 08, 2013 08:59:44 PM Rafael J. Wysocki wrote: > On Friday, February 08, 2013 09:57:18 AM Toshi Kani wrote: > > On Fri, 2013-02-08 at 01:24 +0100, Rafael J. Wysocki wrote: > > > On Monday, February 04, 2013 12:47:31 AM Rafael J. Wysocki wrote: > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > The only useful thing that the ACPI container driver does is to > > > > install system notify handlers for all container and module device > > > > objects it finds in the namespace. The driver structure, > > > > acpi_container_driver, and the data structures created by its > > > > .add() callback are in fact not used by the driver, so remove > > > > them entirely. > > > > > > > > It also makes a little sense to build that driver as a module, > > > > so make it non-modular and add its initialization to the > > > > namespace scanning code. > > > > > > > > In addition to that, make the namespace walk callback used for > > > > installing the notify handlers more straightforward. > > > > > > As pointed out by Toshi Kani, the above changes would make acpi_eject_store() > > > fail for containers and it is the only way to eject them currently, so patch > > > [2/2] is an improved version of this (with Toshi's changes folded in). > > > > > > Patch [1/2] is just a cleanup removing a useless #ifndef from acpi_eject_store(). > > > > Thanks for the update! They look good. For the series: > > > > Reviewed-by: Toshi Kani <toshi.kani@hp.com> > > Tested-by: Toshi Kani <toshi.kani@hp.com> > > > > BTW, did you intentionally keep the struct acpi_container definition in > > <acpi/container.h>? I deleted that one in my patch. > > No, I overlooked that part. I'll apply that part of your patch as a separate > patch on top of this one. Actually, I prefer to remove container.h entirely. I'll post a patch for that shortly. Thanks, Rafael
Hi, The following two patches fix issues I've found revently in the ACPI device hot-removal code. Patch [1/2] makes acpi_bus_hot_remove_device() hold acpi_scan_lock around the whole removal procedure to prevent potential race condition between acpi_bus_scan() and a conflicting evaluation of _EJ0 from happening. Patch [2/2] makes acpi_device_unregister() put devices (supporting that) into D3cold to prevent potential problems with removing power from them without preparation from happening and removes the totally wrong evaluation of _PS3 from acpi_bus_hot_remove_device(). Thanks, Rafael
Index: test/drivers/acpi/container.c =================================================================== --- test.orig/drivers/acpi/container.c +++ test/drivers/acpi/container.c @@ -38,41 +38,15 @@ #define PREFIX "ACPI: " -#define ACPI_CONTAINER_DEVICE_NAME "ACPI container device" -#define ACPI_CONTAINER_CLASS "container" - -#define INSTALL_NOTIFY_HANDLER 1 -#define UNINSTALL_NOTIFY_HANDLER 2 - #define _COMPONENT ACPI_CONTAINER_COMPONENT ACPI_MODULE_NAME("container"); -MODULE_AUTHOR("Anil S Keshavamurthy"); -MODULE_DESCRIPTION("ACPI container driver"); -MODULE_LICENSE("GPL"); - -static int acpi_container_add(struct acpi_device *device); -static int acpi_container_remove(struct acpi_device *device); - static const struct acpi_device_id container_device_ids[] = { {"ACPI0004", 0}, {"PNP0A05", 0}, {"PNP0A06", 0}, {"", 0}, }; -MODULE_DEVICE_TABLE(acpi, container_device_ids); - -static struct acpi_driver acpi_container_driver = { - .name = "container", - .class = ACPI_CONTAINER_CLASS, - .ids = container_device_ids, - .ops = { - .add = acpi_container_add, - .remove = acpi_container_remove, - }, -}; - -/*******************************************************************/ static int is_device_present(acpi_handle handle) { @@ -92,49 +66,6 @@ static int is_device_present(acpi_handle return ((sta & ACPI_STA_DEVICE_PRESENT) == ACPI_STA_DEVICE_PRESENT); } -static bool is_container_device(const char *hid) -{ - const struct acpi_device_id *container_id; - - for (container_id = container_device_ids; - container_id->id[0]; container_id++) { - if (!strcmp((char *)container_id->id, hid)) - return true; - } - - return false; -} - -/*******************************************************************/ -static int acpi_container_add(struct acpi_device *device) -{ - struct acpi_container *container; - - container = kzalloc(sizeof(struct acpi_container), GFP_KERNEL); - if (!container) - return -ENOMEM; - - container->handle = device->handle; - strcpy(acpi_device_name(device), ACPI_CONTAINER_DEVICE_NAME); - strcpy(acpi_device_class(device), ACPI_CONTAINER_CLASS); - device->driver_data = container; - - ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Device <%s> bid <%s>\n", - acpi_device_name(device), acpi_device_bid(device))); - - return 0; -} - -static int acpi_container_remove(struct acpi_device *device) -{ - acpi_status status = AE_OK; - struct acpi_container *pc = NULL; - - pc = acpi_driver_data(device); - kfree(pc); - return status; -} - static void container_notify_cb(acpi_handle handle, u32 type, void *context) { struct acpi_device *device = NULL; @@ -199,84 +130,41 @@ static void container_notify_cb(acpi_han return; } -static acpi_status -container_walk_namespace_cb(acpi_handle handle, - u32 lvl, void *context, void **rv) +static bool is_container(acpi_handle handle) { - char *hid = NULL; struct acpi_device_info *info; - acpi_status status; - int *action = context; + bool ret = false; - status = acpi_get_object_info(handle, &info); - if (ACPI_FAILURE(status)) { - return AE_OK; - } - - if (info->valid & ACPI_VALID_HID) - hid = info->hardware_id.string; - - if (hid == NULL) { - goto end; - } + if (ACPI_FAILURE(acpi_get_object_info(handle, &info))) + return false; - if (!is_container_device(hid)) - goto end; + if (info->valid & ACPI_VALID_HID) { + const struct acpi_device_id *id; - switch (*action) { - case INSTALL_NOTIFY_HANDLER: - acpi_install_notify_handler(handle, - ACPI_SYSTEM_NOTIFY, - container_notify_cb, NULL); - break; - case UNINSTALL_NOTIFY_HANDLER: - acpi_remove_notify_handler(handle, - ACPI_SYSTEM_NOTIFY, - container_notify_cb); - break; - default: - break; + for (id = container_device_ids; id->id[0]; id++) { + ret = !strcmp((char *)id->id, info->hardware_id.string); + if (ret) + break; + } } - - end: kfree(info); - - return AE_OK; + return ret; } -static int __init acpi_container_init(void) +static acpi_status acpi_container_register_notify_handler(acpi_handle handle, + u32 lvl, void *ctxt, + void **retv) { - int result = 0; - int action = INSTALL_NOTIFY_HANDLER; - - result = acpi_bus_register_driver(&acpi_container_driver); - if (result < 0) { - return (result); - } - - /* register notify handler to every container device */ - acpi_walk_namespace(ACPI_TYPE_DEVICE, - ACPI_ROOT_OBJECT, - ACPI_UINT32_MAX, - container_walk_namespace_cb, NULL, &action, NULL); + if (is_container(handle)) + acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY, + container_notify_cb, NULL); - return (0); + return AE_OK; } -static void __exit acpi_container_exit(void) +void __init acpi_container_init(void) { - int action = UNINSTALL_NOTIFY_HANDLER; - - - acpi_walk_namespace(ACPI_TYPE_DEVICE, - ACPI_ROOT_OBJECT, - ACPI_UINT32_MAX, - container_walk_namespace_cb, NULL, &action, NULL); - - acpi_bus_unregister_driver(&acpi_container_driver); - - return; + acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, ACPI_UINT32_MAX, + acpi_container_register_notify_handler, NULL, + NULL, NULL); } - -module_init(acpi_container_init); -module_exit(acpi_container_exit); Index: test/drivers/acpi/Kconfig =================================================================== --- test.orig/drivers/acpi/Kconfig +++ test/drivers/acpi/Kconfig @@ -334,7 +334,7 @@ config X86_PM_TIMER systems require this timer. config ACPI_CONTAINER - tristate "Container and Module Devices (EXPERIMENTAL)" + bool "Container and Module Devices (EXPERIMENTAL)" depends on EXPERIMENTAL default (ACPI_HOTPLUG_MEMORY || ACPI_HOTPLUG_CPU || ACPI_HOTPLUG_IO) help Index: test/drivers/acpi/internal.h =================================================================== --- test.orig/drivers/acpi/internal.h +++ test/drivers/acpi/internal.h @@ -40,6 +40,11 @@ void acpi_memory_hotplug_init(void); #else static inline void acpi_memory_hotplug_init(void) {} #endif +#ifdef ACPI_CONTAINER +void acpi_container_init(void); +#else +static inline void acpi_container_init(void) {} +#endif #ifdef CONFIG_DEBUG_FS extern struct dentry *acpi_debugfs_dir; Index: test/drivers/acpi/scan.c =================================================================== --- test.orig/drivers/acpi/scan.c +++ test/drivers/acpi/scan.c @@ -1763,6 +1763,7 @@ int __init acpi_scan_init(void) acpi_platform_init(); acpi_csrt_init(); acpi_memory_hotplug_init(); + acpi_container_init(); /* * Enumerate devices in the ACPI namespace.