This patchset updates the ACPI system-level (ex. hotplug)
notify handling with a new .sys_notify interface. It provides
the following changes:
- Allow ACPI drivers to register their system-level (hotplug)
notify handlers to a new .sys_notify interface through their
acpi_driver table. This removes redundant ACPI namespace
walks from ACPI drivers for faster booting. The global notify
handler acpi_bus_notify() is called for all system-level ACPI
device notifications, which then calls an appropriate driver's
handler if any. ACPI drivers no longer need to register or
unregister driver's handlers to each device object in ACPI
namespace.
- Support dynamic ACPI namespace with LoadTable & Unload opcode
without any changes in ACPI drivers. There is no need to
register / unregister handlers to ACPI device objects getting
loaded to / unloaded from ACPI namespace.
- Allow ACPI drivers to use a common hotplug handler when it is
implemented. It removes functional conflict between driver's
notify handler and the global notify handler acpi_bus_notify().
acpi_bus_notify() only calls an appropriate notify handler if
any.
Note that the changes maintain backward compatibility for ACPI
drivers. Any drivers registered their hotplug handlers through
the existing interfaces, such as acpi_install_notify_handler()
and register_acpi_bus_notifier(), will continue to work as before.
The patchset is based on the linux-next branch of linux-pm.git
tree.
v3:
- Fixed to free the ID list when releasing a temporary device.
v2:
- Protected unbound driver from unloading when calling .sys_notify.
- Changed acpi_bus_notify() to call acpi_bus_notify_list first for
maintaining the original order.
---
Toshi Kani (4):
ACPI: Support system notify handler via .sys_notify
ACPI: Update processor_driver to use .sys_notify
ACPI: Update acpi_memhotplug to use .sys_notify
ACPI: Update container to use .sys_notify
---
drivers/acpi/acpi_memhotplug.c | 93 ++---------------------------------------
drivers/acpi/bus.c | 64 +++++++++++++++++++++-------
drivers/acpi/container.c | 77 ++--------------------------------
drivers/acpi/processor_driver.c | 82 ++++--------------------------------
drivers/acpi/scan.c | 83 ++++++++++++++++++++++++++++++++++++
include/acpi/acpi_bus.h | 6 +++
6 files changed, 150 insertions(+), 255 deletions(-)
Added a new .sys_notify interface, which allows ACPI drivers to
register their system-level (ex. hotplug) notify handlers through
their acpi_driver table. This removes redundant ACPI namespace
walks from ACPI drivers for faster booting.
The global notify handler acpi_bus_notify() is called for all
system-level ACPI notifications, which then calls an appropriate
driver's handler if any. ACPI drivers no longer need to register
or unregister driver's handler to each ACPI device object. It also
supports dynamic ACPI namespace with LoadTable & Unload opcode
without any modification in ACPI drivers.
Added a common system notify handler acpi_bus_sys_notify(), which
allows ACPI drivers to set it to .sys_notify when this function is
fully implemented. It removes functional conflict between driver's
notify handler and the global notify handler acpi_bus_notify().
Note that the changes maintain backward compatibility for ACPI
drivers. Any drivers registered their hotplug handler through the
existing interfaces, such as acpi_install_notify_handler() and
register_acpi_bus_notifier(), will continue to work as before.
Signed-off-by: Toshi Kani <[email protected]>
---
drivers/acpi/bus.c | 64 ++++++++++++++++++++++++++++----------
drivers/acpi/scan.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++
include/acpi/acpi_bus.h | 6 ++++
3 files changed, 137 insertions(+), 16 deletions(-)
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 07a20ee..b256bcf2 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -779,21 +779,16 @@ void unregister_acpi_bus_notifier(struct notifier_block *nb)
EXPORT_SYMBOL_GPL(unregister_acpi_bus_notifier);
/**
- * acpi_bus_notify
- * ---------------
- * Callback for all 'system-level' device notifications (values 0x00-0x7F).
+ * acpi_bus_sys_notify: Common system notify handler
+ *
+ * ACPI drivers may specify this common handler to its sys_notify entry.
+ * TBD: This handler is not implemented yet.
*/
-static void acpi_bus_notify(acpi_handle handle, u32 type, void *data)
+void acpi_bus_sys_notify(acpi_handle handle, u32 type, void *data)
{
- struct acpi_device *device = NULL;
- struct acpi_driver *driver;
-
ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Notification %#02x to handle %p\n",
type, handle));
- blocking_notifier_call_chain(&acpi_bus_notify_list,
- type, (void *)handle);
-
switch (type) {
case ACPI_NOTIFY_BUS_CHECK:
@@ -842,14 +837,51 @@ static void acpi_bus_notify(acpi_handle handle, u32 type, void *data)
type));
break;
}
+}
+
+/**
+ * acpi_bus_drv_notify: Call driver's system-level notify handler
+ */
+void acpi_bus_drv_notify(struct acpi_driver *driver,
+ struct acpi_device *device, acpi_handle handle,
+ u32 type, void *data)
+{
+ BUG_ON(!driver);
+
+ if (driver->ops.sys_notify)
+ driver->ops.sys_notify(handle, type, data);
+ else if (device && driver->ops.notify &&
+ (driver->flags & ACPI_DRIVER_ALL_NOTIFY_EVENTS))
+ driver->ops.notify(device, type);
+
+ return;
+}
+
+/**
+ * acpi_bus_notify: The system-level global notify handler
+ *
+ * The global notify handler for all 'system-level' device notifications
+ * (values 0x00-0x7F). This handler calls a driver's notify handler for
+ * the notified ACPI device.
+ */
+static void acpi_bus_notify(acpi_handle handle, u32 type, void *data)
+{
+ struct acpi_device *device = NULL;
+
+ /* call registered handlers in the bus notify list */
+ blocking_notifier_call_chain(&acpi_bus_notify_list,
+ type, (void *)handle);
+ /* obtain an associated driver if already bound */
acpi_bus_get_device(handle, &device);
- if (device) {
- driver = device->driver;
- if (driver && driver->ops.notify &&
- (driver->flags & ACPI_DRIVER_ALL_NOTIFY_EVENTS))
- driver->ops.notify(device, type);
- }
+
+ /* call the driver's system-level notify handler */
+ if (device && device->driver)
+ acpi_bus_drv_notify(device->driver, device, handle, type, data);
+ else
+ acpi_lookup_drv_notify(handle, type, data);
+
+ return;
}
/* --------------------------------------------------------------------------
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 95ff1e8..333e57f 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1758,3 +1758,86 @@ int __init acpi_scan_init(void)
return result;
}
+
+/* callback args for acpi_match_drv_notify() */
+struct acpi_notify_args {
+ struct acpi_device *device;
+ acpi_handle handle;
+ u32 event;
+ void *data;
+};
+
+static int acpi_match_drv_notify(struct device_driver *drv, void *data)
+{
+ struct acpi_driver *driver = to_acpi_driver(drv);
+ struct acpi_notify_args *args = (struct acpi_notify_args *) data;
+
+ /* check if this driver matches with the device */
+ if (acpi_match_device_ids(args->device, driver->ids))
+ return 0;
+
+ /* call the driver's notify handler */
+ acpi_bus_drv_notify(driver, NULL, args->handle,
+ args->event, args->data);
+
+ return 1;
+}
+
+/**
+ * acpi_lookup_drv_notify: Look up and call driver's notify handler
+ * @handle: ACPI handle of the notified device object
+ * @event: Notify event
+ * @data: Context
+ *
+ * Look up and call the associated driver's notify handler for the ACPI
+ * device object by walking through the list of ACPI drivers.
+ */
+void acpi_lookup_drv_notify(acpi_handle handle, u32 event, void *data)
+{
+ struct acpi_notify_args args;
+ struct acpi_device *device;
+ unsigned long long sta;
+ int type;
+ int ret;
+
+ /* allocate a temporary device object */
+ device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
+ if (!device) {
+ pr_err(PREFIX "No memory to allocate a tmp device\n");
+ return;
+ }
+ INIT_LIST_HEAD(&device->pnp.ids);
+
+ /* obtain device type */
+ ret = acpi_bus_type_and_status(handle, &type, &sta);
+ if (ret) {
+ pr_err(PREFIX "Failed to get device type\n");
+ goto out;
+ }
+
+ /* setup this temporary device object */
+ device->device_type = type;
+ device->handle = handle;
+ device->parent = acpi_bus_get_parent(handle);
+ device->dev.bus = &acpi_bus_type;
+ device->driver = NULL;
+ STRUCT_TO_INT(device->status) = sta;
+ device->status.present = 1;
+
+ /* set HID to this device object */
+ acpi_device_set_id(device);
+
+ /* set args */
+ args.device = device;
+ args.handle = handle;
+ args.event = event;
+ args.data = data;
+
+ /* call a matched driver's notify handler */
+ (void) bus_for_each_drv(device->dev.bus, NULL,
+ &args, acpi_match_drv_notify);
+
+out:
+ acpi_device_release(&device->dev);
+ return;
+}
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 2242c10..4052406 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -94,6 +94,7 @@ typedef int (*acpi_op_start) (struct acpi_device * device);
typedef int (*acpi_op_bind) (struct acpi_device * device);
typedef int (*acpi_op_unbind) (struct acpi_device * device);
typedef void (*acpi_op_notify) (struct acpi_device * device, u32 event);
+typedef void (*acpi_op_sys_notify) (acpi_handle handle, u32 event, void *data);
struct acpi_bus_ops {
u32 acpi_op_add:1;
@@ -107,6 +108,7 @@ struct acpi_device_ops {
acpi_op_bind bind;
acpi_op_unbind unbind;
acpi_op_notify notify;
+ acpi_op_sys_notify sys_notify;
};
#define ACPI_DRIVER_ALL_NOTIFY_EVENTS 0x1 /* system AND device events */
@@ -328,6 +330,10 @@ extern int unregister_acpi_notifier(struct notifier_block *);
extern int register_acpi_bus_notifier(struct notifier_block *nb);
extern void unregister_acpi_bus_notifier(struct notifier_block *nb);
+extern void acpi_lookup_drv_notify(acpi_handle handle, u32 event, void *data);
+extern void acpi_bus_drv_notify(struct acpi_driver *driver,
+ struct acpi_device *device, acpi_handle handle, u32 type, void *data);
+
/*
* External Functions
*/
--
1.7.11.7
Changed the ACPI processor driver to use .sys_notify. Removed
ACPI namespace walks and their call-back functions that register
and unregister the hotplug handler to all processor objects
through acpi_[install|remove]_notify_handler().
Signed-off-by: Toshi Kani <[email protected]>
---
drivers/acpi/processor_driver.c | 82 ++++-------------------------------------
1 file changed, 7 insertions(+), 75 deletions(-)
diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
index 8cc33d0..27f15f7 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -85,6 +85,10 @@ static void acpi_processor_notify(struct acpi_device *device, u32 event);
static acpi_status acpi_processor_hotadd_init(struct acpi_processor *pr);
static int acpi_processor_handle_eject(struct acpi_processor *pr);
static int acpi_processor_start(struct acpi_processor *pr);
+#ifdef CONFIG_ACPI_HOTPLUG_CPU
+static void acpi_processor_hotplug_notify(acpi_handle handle, u32 event,
+ void *data);
+#endif
static const struct acpi_device_id processor_device_ids[] = {
{ACPI_PROCESSOR_OBJECT_HID, 0},
@@ -104,6 +108,9 @@ static struct acpi_driver acpi_processor_driver = {
.add = acpi_processor_add,
.remove = acpi_processor_remove,
.notify = acpi_processor_notify,
+#ifdef CONFIG_ACPI_HOTPLUG_CPU
+ .sys_notify = acpi_processor_hotplug_notify,
+#endif
},
.drv.pm = &acpi_processor_pm,
};
@@ -763,67 +770,6 @@ static void acpi_processor_hotplug_notify(acpi_handle handle,
return;
}
-static acpi_status is_processor_device(acpi_handle handle)
-{
- struct acpi_device_info *info;
- char *hid;
- acpi_status status;
-
- status = acpi_get_object_info(handle, &info);
- if (ACPI_FAILURE(status))
- return status;
-
- if (info->type == ACPI_TYPE_PROCESSOR) {
- kfree(info);
- return AE_OK; /* found a processor object */
- }
-
- if (!(info->valid & ACPI_VALID_HID)) {
- kfree(info);
- return AE_ERROR;
- }
-
- hid = info->hardware_id.string;
- if ((hid == NULL) || strcmp(hid, ACPI_PROCESSOR_DEVICE_HID)) {
- kfree(info);
- return AE_ERROR;
- }
-
- kfree(info);
- return AE_OK; /* found a processor device object */
-}
-
-static acpi_status
-processor_walk_namespace_cb(acpi_handle handle,
- u32 lvl, void *context, void **rv)
-{
- acpi_status status;
- int *action = context;
-
- status = is_processor_device(handle);
- if (ACPI_FAILURE(status))
- return AE_OK; /* not a processor; continue to walk */
-
- switch (*action) {
- case INSTALL_NOTIFY_HANDLER:
- acpi_install_notify_handler(handle,
- ACPI_SYSTEM_NOTIFY,
- acpi_processor_hotplug_notify,
- NULL);
- break;
- case UNINSTALL_NOTIFY_HANDLER:
- acpi_remove_notify_handler(handle,
- ACPI_SYSTEM_NOTIFY,
- acpi_processor_hotplug_notify);
- break;
- default:
- break;
- }
-
- /* found a processor; skip walking underneath */
- return AE_CTRL_DEPTH;
-}
-
static acpi_status acpi_processor_hotadd_init(struct acpi_processor *pr)
{
acpi_handle handle = pr->handle;
@@ -891,26 +837,12 @@ static int acpi_processor_handle_eject(struct acpi_processor *pr)
static
void acpi_processor_install_hotplug_notify(void)
{
-#ifdef CONFIG_ACPI_HOTPLUG_CPU
- int action = INSTALL_NOTIFY_HANDLER;
- acpi_walk_namespace(ACPI_TYPE_ANY,
- ACPI_ROOT_OBJECT,
- ACPI_UINT32_MAX,
- processor_walk_namespace_cb, NULL, &action, NULL);
-#endif
register_hotcpu_notifier(&acpi_cpu_notifier);
}
static
void acpi_processor_uninstall_hotplug_notify(void)
{
-#ifdef CONFIG_ACPI_HOTPLUG_CPU
- int action = UNINSTALL_NOTIFY_HANDLER;
- acpi_walk_namespace(ACPI_TYPE_ANY,
- ACPI_ROOT_OBJECT,
- ACPI_UINT32_MAX,
- processor_walk_namespace_cb, NULL, &action, NULL);
-#endif
unregister_hotcpu_notifier(&acpi_cpu_notifier);
}
--
1.7.11.7
Changed the ACPI memory hotplug driver to use .sys_notify. Removed
ACPI namespace walks and their call-back functions that register and
unregister the hotplug handler to all memory device objects through
acpi_[install|remove]_notify_handler().
Signed-off-by: Toshi Kani <[email protected]>
---
drivers/acpi/acpi_memhotplug.c | 93 ++----------------------------------------
1 file changed, 3 insertions(+), 90 deletions(-)
diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
index 92c973a..8af799f 100644
--- a/drivers/acpi/acpi_memhotplug.c
+++ b/drivers/acpi/acpi_memhotplug.c
@@ -54,6 +54,8 @@ MODULE_LICENSE("GPL");
static int acpi_memory_device_add(struct acpi_device *device);
static int acpi_memory_device_remove(struct acpi_device *device, int type);
+static void acpi_memory_device_notify(acpi_handle handle, u32 event,
+ void *data);
static const struct acpi_device_id memory_device_ids[] = {
{ACPI_MEMORY_DEVICE_HID, 0},
@@ -68,6 +70,7 @@ static struct acpi_driver acpi_memory_device_driver = {
.ops = {
.add = acpi_memory_device_add,
.remove = acpi_memory_device_remove,
+ .sys_notify = acpi_memory_device_notify,
},
};
@@ -515,111 +518,21 @@ static int acpi_memory_device_remove(struct acpi_device *device, int type)
return 0;
}
-/*
- * Helper function to check for memory device
- */
-static acpi_status is_memory_device(acpi_handle handle)
-{
- char *hardware_id;
- acpi_status status;
- struct acpi_device_info *info;
-
- status = acpi_get_object_info(handle, &info);
- if (ACPI_FAILURE(status))
- return status;
-
- if (!(info->valid & ACPI_VALID_HID)) {
- kfree(info);
- return AE_ERROR;
- }
-
- hardware_id = info->hardware_id.string;
- if ((hardware_id == NULL) ||
- (strcmp(hardware_id, ACPI_MEMORY_DEVICE_HID)))
- status = AE_ERROR;
-
- kfree(info);
- 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))
- 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))
- 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;
-
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, NULL);
-
- if (ACPI_FAILURE(status)) {
- ACPI_EXCEPTION((AE_INFO, status, "walk_namespace failed"));
- acpi_bus_unregister_driver(&acpi_memory_device_driver);
- return -ENODEV;
- }
-
acpi_hotmem_initialized = 1;
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, NULL);
-
- if (ACPI_FAILURE(status))
- ACPI_EXCEPTION((AE_INFO, status, "walk_namespace failed"));
-
acpi_bus_unregister_driver(&acpi_memory_device_driver);
return;
--
1.7.11.7
Changed the ACPI container driver to use .sys_notify. Removed
ACPI namespace walks and their call-back functions that register
and unregister the hotplug handler to all container objects
through acpi_[install|remove]_notify_handler(). Renamed the
notify handler to container_notify() for consistency.
Signed-off-by: Toshi Kani <[email protected]>
---
drivers/acpi/container.c | 77 ++----------------------------------------------
1 file changed, 3 insertions(+), 74 deletions(-)
diff --git a/drivers/acpi/container.c b/drivers/acpi/container.c
index 69e2d6b..d4e8b71 100644
--- a/drivers/acpi/container.c
+++ b/drivers/acpi/container.c
@@ -53,6 +53,7 @@ MODULE_LICENSE("GPL");
static int acpi_container_add(struct acpi_device *device);
static int acpi_container_remove(struct acpi_device *device, int type);
+static void container_notify(acpi_handle handle, u32 type, void *context);
static const struct acpi_device_id container_device_ids[] = {
{"ACPI0004", 0},
@@ -69,6 +70,7 @@ static struct acpi_driver acpi_container_driver = {
.ops = {
.add = acpi_container_add,
.remove = acpi_container_remove,
+ .sys_notify = container_notify,
},
};
@@ -92,19 +94,6 @@ static int is_device_present(acpi_handle 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)
{
@@ -165,7 +154,7 @@ static int container_device_add(struct acpi_device **device, acpi_handle handle)
return result;
}
-static void container_notify_cb(acpi_handle handle, u32 type, void *context)
+static void container_notify(acpi_handle handle, u32 type, void *context)
{
struct acpi_device *device = NULL;
int result;
@@ -224,80 +213,20 @@ static void container_notify_cb(acpi_handle handle, u32 type, void *context)
return;
}
-static acpi_status
-container_walk_namespace_cb(acpi_handle handle,
- u32 lvl, void *context, void **rv)
-{
- char *hid = NULL;
- struct acpi_device_info *info;
- acpi_status status;
- int *action = context;
-
- 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 (!is_container_device(hid))
- goto end;
-
- 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;
- }
-
- end:
- kfree(info);
-
- return AE_OK;
-}
-
static int __init acpi_container_init(void)
{
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);
-
return (0);
}
static void __exit acpi_container_exit(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;
--
1.7.11.7
On Thursday, November 08, 2012 01:23:44 PM Toshi Kani wrote:
> Added a new .sys_notify interface, which allows ACPI drivers to
> register their system-level (ex. hotplug) notify handlers through
> their acpi_driver table. This removes redundant ACPI namespace
> walks from ACPI drivers for faster booting.
>
> The global notify handler acpi_bus_notify() is called for all
> system-level ACPI notifications, which then calls an appropriate
> driver's handler if any. ACPI drivers no longer need to register
> or unregister driver's handler to each ACPI device object. It also
> supports dynamic ACPI namespace with LoadTable & Unload opcode
> without any modification in ACPI drivers.
>
> Added a common system notify handler acpi_bus_sys_notify(), which
> allows ACPI drivers to set it to .sys_notify when this function is
> fully implemented.
I don't really understand this.
> It removes functional conflict between driver's
> notify handler and the global notify handler acpi_bus_notify().
>
> Note that the changes maintain backward compatibility for ACPI
> drivers. Any drivers registered their hotplug handler through the
> existing interfaces, such as acpi_install_notify_handler() and
> register_acpi_bus_notifier(), will continue to work as before.
I really wouldn't like to add new callbacks to struct acpi_device_ops, because
I'd like that whole thing to go away entirely eventually, along with struct
acpi_driver.
Moreover, in this particular case, it really is not useful to have to define
a struct acpi_driver so that one can register for receiving system
notifications from ACPI. It would be really nice if non-ACPI drivers, such
as PCI or platform, could do that too.
Besides, acpi_os_execute_deferred() is always run on CPU0, because of some
SMI-related peculiarity, which is not very efficient as far as the events
handling is concerned, but we can improve the situation a bit by queing the
execution of the registered handlers in a different workqueue. Maybe it's
worth considering if we're going to change this code anyway?
> Signed-off-by: Toshi Kani <[email protected]>
> ---
> drivers/acpi/bus.c | 64 ++++++++++++++++++++++++++++----------
> drivers/acpi/scan.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++
> include/acpi/acpi_bus.h | 6 ++++
> 3 files changed, 137 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index 07a20ee..b256bcf2 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -779,21 +779,16 @@ void unregister_acpi_bus_notifier(struct notifier_block *nb)
> EXPORT_SYMBOL_GPL(unregister_acpi_bus_notifier);
>
> /**
> - * acpi_bus_notify
> - * ---------------
> - * Callback for all 'system-level' device notifications (values 0x00-0x7F).
> + * acpi_bus_sys_notify: Common system notify handler
> + *
> + * ACPI drivers may specify this common handler to its sys_notify entry.
> + * TBD: This handler is not implemented yet.
> */
> -static void acpi_bus_notify(acpi_handle handle, u32 type, void *data)
> +void acpi_bus_sys_notify(acpi_handle handle, u32 type, void *data)
This isn't used anywhere. Are drivers supposed to use it? If so, what about
the BUS_CHECK and DEVICE_CHECK notifications?
> {
> - struct acpi_device *device = NULL;
> - struct acpi_driver *driver;
> -
> ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Notification %#02x to handle %p\n",
> type, handle));
>
> - blocking_notifier_call_chain(&acpi_bus_notify_list,
> - type, (void *)handle);
> -
> switch (type) {
>
> case ACPI_NOTIFY_BUS_CHECK:
> @@ -842,14 +837,51 @@ static void acpi_bus_notify(acpi_handle handle, u32 type, void *data)
> type));
> break;
> }
> +}
> +
> +/**
> + * acpi_bus_drv_notify: Call driver's system-level notify handler
> + */
> +void acpi_bus_drv_notify(struct acpi_driver *driver,
> + struct acpi_device *device, acpi_handle handle,
> + u32 type, void *data)
> +{
> + BUG_ON(!driver);
Rule: Don't crash the kernel if you don't have to. Try to recover instead.
It seems that
if (WARN_ON(!driver))
return;
would be sufficient in this particulare case, wouldn't it?
> +
> + if (driver->ops.sys_notify)
> + driver->ops.sys_notify(handle, type, data);
> + else if (device && driver->ops.notify &&
Why "else if"? The existing code does this unconditionally. Is that incorrect?
> + (driver->flags & ACPI_DRIVER_ALL_NOTIFY_EVENTS))
> + driver->ops.notify(device, type);
> +
> + return;
> +}
> +
> +/**
> + * acpi_bus_notify: The system-level global notify handler
> + *
> + * The global notify handler for all 'system-level' device notifications
> + * (values 0x00-0x7F). This handler calls a driver's notify handler for
> + * the notified ACPI device.
> + */
> +static void acpi_bus_notify(acpi_handle handle, u32 type, void *data)
> +{
> + struct acpi_device *device = NULL;
> +
> + /* call registered handlers in the bus notify list */
> + blocking_notifier_call_chain(&acpi_bus_notify_list,
> + type, (void *)handle);
>
> + /* obtain an associated driver if already bound */
> acpi_bus_get_device(handle, &device);
> - if (device) {
> - driver = device->driver;
> - if (driver && driver->ops.notify &&
> - (driver->flags & ACPI_DRIVER_ALL_NOTIFY_EVENTS))
> - driver->ops.notify(device, type);
> - }
> +
> + /* call the driver's system-level notify handler */
> + if (device && device->driver)
> + acpi_bus_drv_notify(device->driver, device, handle, type, data);
> + else
> + acpi_lookup_drv_notify(handle, type, data);
> +
> + return;
> }
>
> /* --------------------------------------------------------------------------
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 95ff1e8..333e57f 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1758,3 +1758,86 @@ int __init acpi_scan_init(void)
>
> return result;
> }
> +
> +/* callback args for acpi_match_drv_notify() */
> +struct acpi_notify_args {
> + struct acpi_device *device;
> + acpi_handle handle;
> + u32 event;
> + void *data;
> +};
> +
> +static int acpi_match_drv_notify(struct device_driver *drv, void *data)
> +{
> + struct acpi_driver *driver = to_acpi_driver(drv);
> + struct acpi_notify_args *args = (struct acpi_notify_args *) data;
> +
> + /* check if this driver matches with the device */
> + if (acpi_match_device_ids(args->device, driver->ids))
> + return 0;
> +
> + /* call the driver's notify handler */
> + acpi_bus_drv_notify(driver, NULL, args->handle,
> + args->event, args->data);
> +
> + return 1;
> +}
> +
> +/**
> + * acpi_lookup_drv_notify: Look up and call driver's notify handler
> + * @handle: ACPI handle of the notified device object
> + * @event: Notify event
> + * @data: Context
> + *
> + * Look up and call the associated driver's notify handler for the ACPI
> + * device object by walking through the list of ACPI drivers.
What exactly is the purpose of this?
> + */
> +void acpi_lookup_drv_notify(acpi_handle handle, u32 event, void *data)
> +{
> + struct acpi_notify_args args;
> + struct acpi_device *device;
> + unsigned long long sta;
> + int type;
> + int ret;
> +
> + /* allocate a temporary device object */
> + device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
> + if (!device) {
> + pr_err(PREFIX "No memory to allocate a tmp device\n");
> + return;
> + }
> + INIT_LIST_HEAD(&device->pnp.ids);
> +
> + /* obtain device type */
> + ret = acpi_bus_type_and_status(handle, &type, &sta);
> + if (ret) {
> + pr_err(PREFIX "Failed to get device type\n");
> + goto out;
> + }
> +
> + /* setup this temporary device object */
> + device->device_type = type;
> + device->handle = handle;
> + device->parent = acpi_bus_get_parent(handle);
> + device->dev.bus = &acpi_bus_type;
> + device->driver = NULL;
> + STRUCT_TO_INT(device->status) = sta;
> + device->status.present = 1;
> +
> + /* set HID to this device object */
> + acpi_device_set_id(device);
> +
> + /* set args */
> + args.device = device;
> + args.handle = handle;
> + args.event = event;
> + args.data = data;
> +
> + /* call a matched driver's notify handler */
> + (void) bus_for_each_drv(device->dev.bus, NULL,
> + &args, acpi_match_drv_notify);
Excuse me? What makes you think I would accept anything like this?
> +
> +out:
> + acpi_device_release(&device->dev);
> + return;
> +}
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index 2242c10..4052406 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -94,6 +94,7 @@ typedef int (*acpi_op_start) (struct acpi_device * device);
> typedef int (*acpi_op_bind) (struct acpi_device * device);
> typedef int (*acpi_op_unbind) (struct acpi_device * device);
> typedef void (*acpi_op_notify) (struct acpi_device * device, u32 event);
> +typedef void (*acpi_op_sys_notify) (acpi_handle handle, u32 event, void *data);
>
> struct acpi_bus_ops {
> u32 acpi_op_add:1;
> @@ -107,6 +108,7 @@ struct acpi_device_ops {
> acpi_op_bind bind;
> acpi_op_unbind unbind;
> acpi_op_notify notify;
> + acpi_op_sys_notify sys_notify;
> };
>
> #define ACPI_DRIVER_ALL_NOTIFY_EVENTS 0x1 /* system AND device events */
> @@ -328,6 +330,10 @@ extern int unregister_acpi_notifier(struct notifier_block *);
>
> extern int register_acpi_bus_notifier(struct notifier_block *nb);
> extern void unregister_acpi_bus_notifier(struct notifier_block *nb);
> +extern void acpi_lookup_drv_notify(acpi_handle handle, u32 event, void *data);
> +extern void acpi_bus_drv_notify(struct acpi_driver *driver,
> + struct acpi_device *device, acpi_handle handle, u32 type, void *data);
> +
> /*
> * External Functions
> */
>
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Saturday, November 24, 2012 11:01:56 PM Rafael J. Wysocki wrote:
> On Thursday, November 08, 2012 01:23:44 PM Toshi Kani wrote:
> > Added a new .sys_notify interface, which allows ACPI drivers to
> > register their system-level (ex. hotplug) notify handlers through
> > their acpi_driver table. This removes redundant ACPI namespace
> > walks from ACPI drivers for faster booting.
> >
> > The global notify handler acpi_bus_notify() is called for all
> > system-level ACPI notifications, which then calls an appropriate
> > driver's handler if any. ACPI drivers no longer need to register
> > or unregister driver's handler to each ACPI device object. It also
> > supports dynamic ACPI namespace with LoadTable & Unload opcode
> > without any modification in ACPI drivers.
> >
> > Added a common system notify handler acpi_bus_sys_notify(), which
> > allows ACPI drivers to set it to .sys_notify when this function is
> > fully implemented.
>
> I don't really understand this.
>
> > It removes functional conflict between driver's
> > notify handler and the global notify handler acpi_bus_notify().
> >
> > Note that the changes maintain backward compatibility for ACPI
> > drivers. Any drivers registered their hotplug handler through the
> > existing interfaces, such as acpi_install_notify_handler() and
> > register_acpi_bus_notifier(), will continue to work as before.
>
> I really wouldn't like to add new callbacks to struct acpi_device_ops, because
> I'd like that whole thing to go away entirely eventually, along with struct
> acpi_driver.
>
> Moreover, in this particular case, it really is not useful to have to define
> a struct acpi_driver so that one can register for receiving system
> notifications from ACPI. It would be really nice if non-ACPI drivers, such
> as PCI or platform, could do that too.
>
> Besides, acpi_os_execute_deferred() is always run on CPU0, because of some
> SMI-related peculiarity, which is not very efficient as far as the events
> handling is concerned, but we can improve the situation a bit by queing the
> execution of the registered handlers in a different workqueue. Maybe it's
> worth considering if we're going to change this code anyway?
>
> > Signed-off-by: Toshi Kani <[email protected]>
> > ---
> > drivers/acpi/bus.c | 64 ++++++++++++++++++++++++++++----------
> > drivers/acpi/scan.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++
> > include/acpi/acpi_bus.h | 6 ++++
> > 3 files changed, 137 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> > index 07a20ee..b256bcf2 100644
> > --- a/drivers/acpi/bus.c
> > +++ b/drivers/acpi/bus.c
> > @@ -779,21 +779,16 @@ void unregister_acpi_bus_notifier(struct notifier_block *nb)
> > EXPORT_SYMBOL_GPL(unregister_acpi_bus_notifier);
> >
> > /**
> > - * acpi_bus_notify
> > - * ---------------
> > - * Callback for all 'system-level' device notifications (values 0x00-0x7F).
> > + * acpi_bus_sys_notify: Common system notify handler
> > + *
> > + * ACPI drivers may specify this common handler to its sys_notify entry.
> > + * TBD: This handler is not implemented yet.
> > */
> > -static void acpi_bus_notify(acpi_handle handle, u32 type, void *data)
> > +void acpi_bus_sys_notify(acpi_handle handle, u32 type, void *data)
>
> This isn't used anywhere. Are drivers supposed to use it? If so, what about
> the BUS_CHECK and DEVICE_CHECK notifications?
>
> > {
> > - struct acpi_device *device = NULL;
> > - struct acpi_driver *driver;
> > -
> > ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Notification %#02x to handle %p\n",
> > type, handle));
> >
> > - blocking_notifier_call_chain(&acpi_bus_notify_list,
> > - type, (void *)handle);
By the way, there is exacly one user of this chain, which is dock.c.
What about convering that to something different and dropping the chain to
start with?
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Saturday, November 24, 2012 11:01:56 PM Rafael J. Wysocki wrote:
> On Thursday, November 08, 2012 01:23:44 PM Toshi Kani wrote:
> > Added a new .sys_notify interface, which allows ACPI drivers to
> > register their system-level (ex. hotplug) notify handlers through
> > their acpi_driver table. This removes redundant ACPI namespace
> > walks from ACPI drivers for faster booting.
> >
> > The global notify handler acpi_bus_notify() is called for all
> > system-level ACPI notifications, which then calls an appropriate
> > driver's handler if any. ACPI drivers no longer need to register
> > or unregister driver's handler to each ACPI device object. It also
> > supports dynamic ACPI namespace with LoadTable & Unload opcode
> > without any modification in ACPI drivers.
> >
> > Added a common system notify handler acpi_bus_sys_notify(), which
> > allows ACPI drivers to set it to .sys_notify when this function is
> > fully implemented.
>
> I don't really understand this.
>
> > It removes functional conflict between driver's
> > notify handler and the global notify handler acpi_bus_notify().
> >
> > Note that the changes maintain backward compatibility for ACPI
> > drivers. Any drivers registered their hotplug handler through the
> > existing interfaces, such as acpi_install_notify_handler() and
> > register_acpi_bus_notifier(), will continue to work as before.
>
> I really wouldn't like to add new callbacks to struct acpi_device_ops, because
> I'd like that whole thing to go away entirely eventually, along with struct
> acpi_driver.
>
> Moreover, in this particular case, it really is not useful to have to define
> a struct acpi_driver so that one can register for receiving system
> notifications from ACPI. It would be really nice if non-ACPI drivers, such
> as PCI or platform, could do that too.
Which they do by using acpi_install_notify_handler() directly.
> Besides, acpi_os_execute_deferred() is always run on CPU0, because of some
> SMI-related peculiarity, which is not very efficient as far as the events
> handling is concerned, but we can improve the situation a bit by queing the
> execution of the registered handlers in a different workqueue. Maybe it's
> worth considering if we're going to change this code anyway?
Well, perhaps we really don't need to change it after all? Maybe we can just
switch everyone to using acpi_install_notify_handler() and then we can just
drop that code entirely?
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
Hi Rafael,
Thanks for reviewing! My comments are in-line.
On Sat, 2012-11-24 at 23:01 +0100, Rafael J. Wysocki wrote:
> On Thursday, November 08, 2012 01:23:44 PM Toshi Kani wrote:
> > Added a new .sys_notify interface, which allows ACPI drivers to
> > register their system-level (ex. hotplug) notify handlers through
> > their acpi_driver table. This removes redundant ACPI namespace
> > walks from ACPI drivers for faster booting.
> >
> > The global notify handler acpi_bus_notify() is called for all
> > system-level ACPI notifications, which then calls an appropriate
> > driver's handler if any. ACPI drivers no longer need to register
> > or unregister driver's handler to each ACPI device object. It also
> > supports dynamic ACPI namespace with LoadTable & Unload opcode
> > without any modification in ACPI drivers.
> >
> > Added a common system notify handler acpi_bus_sys_notify(), which
> > allows ACPI drivers to set it to .sys_notify when this function is
> > fully implemented.
>
> I don't really understand this.
Currently, acpi_bus_notify() is partially implemented as the common
notify handler, but it may not be fully implemented under the current
design. When a notify event is sent, ACPICA calls both
acpi_bus_notify() and driver's handler registered through
acpi_install_notify_handler(). However, a same event cannot be handled
by both handlers. Since acpi_bus_notify() may not know if an event has
already been handled by driver's handler, it cannot do anything that may
conflict with the driver's handler.
Therefore, the partially implemented common handler code in
acpi_bus_notify() is moved to a separate function acpi_bus_sys_notify()
in this patchset. This function can now be fully implemented as
necessary.
> > It removes functional conflict between driver's
> > notify handler and the global notify handler acpi_bus_notify().
> >
> > Note that the changes maintain backward compatibility for ACPI
> > drivers. Any drivers registered their hotplug handler through the
> > existing interfaces, such as acpi_install_notify_handler() and
> > register_acpi_bus_notifier(), will continue to work as before.
>
> I really wouldn't like to add new callbacks to struct acpi_device_ops, because
> I'd like that whole thing to go away entirely eventually, along with struct
> acpi_driver.
acpi_device may need to be changed, but I think ACPI drivers are still
necessary to support vendor-unique PNPIDs in an extendable way, which is
currently done by adding drivers, such as asus_acpi_driver,
cmpc_accel_acpi_driver, eeepc_acpi_driver, acpi_fujitsu_driver,
lis3lv02d_driver, etc...
> Moreover, in this particular case, it really is not useful to have to define
> a struct acpi_driver so that one can register for receiving system
> notifications from ACPI. It would be really nice if non-ACPI drivers, such
> as PCI or platform, could do that too.
>
> Besides, acpi_os_execute_deferred() is always run on CPU0, because of some
> SMI-related peculiarity, which is not very efficient as far as the events
> handling is concerned, but we can improve the situation a bit by queing the
> execution of the registered handlers in a different workqueue. Maybe it's
> worth considering if we're going to change this code anyway?
Will reply to other email.
> > Signed-off-by: Toshi Kani <[email protected]>
> > ---
> > drivers/acpi/bus.c | 64 ++++++++++++++++++++++++++++----------
> > drivers/acpi/scan.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++
> > include/acpi/acpi_bus.h | 6 ++++
> > 3 files changed, 137 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> > index 07a20ee..b256bcf2 100644
> > --- a/drivers/acpi/bus.c
> > +++ b/drivers/acpi/bus.c
> > @@ -779,21 +779,16 @@ void unregister_acpi_bus_notifier(struct notifier_block *nb)
> > EXPORT_SYMBOL_GPL(unregister_acpi_bus_notifier);
> >
> > /**
> > - * acpi_bus_notify
> > - * ---------------
> > - * Callback for all 'system-level' device notifications (values 0x00-0x7F).
> > + * acpi_bus_sys_notify: Common system notify handler
> > + *
> > + * ACPI drivers may specify this common handler to its sys_notify entry.
> > + * TBD: This handler is not implemented yet.
> > */
> > -static void acpi_bus_notify(acpi_handle handle, u32 type, void *data)
> > +void acpi_bus_sys_notify(acpi_handle handle, u32 type, void *data)
>
> This isn't used anywhere. Are drivers supposed to use it? If so, what about
> the BUS_CHECK and DEVICE_CHECK notifications?
This function is not fully implemented yet, as explained above. This
patchset only fixed the current structural issue. So, drivers are not
supposed to use it now.
> > {
> > - struct acpi_device *device = NULL;
> > - struct acpi_driver *driver;
> > -
> > ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Notification %#02x to handle %p\n",
> > type, handle));
> >
> > - blocking_notifier_call_chain(&acpi_bus_notify_list,
> > - type, (void *)handle);
> > -
> > switch (type) {
> >
> > case ACPI_NOTIFY_BUS_CHECK:
> > @@ -842,14 +837,51 @@ static void acpi_bus_notify(acpi_handle handle, u32 type, void *data)
> > type));
> > break;
> > }
> > +}
> > +
> > +/**
> > + * acpi_bus_drv_notify: Call driver's system-level notify handler
> > + */
> > +void acpi_bus_drv_notify(struct acpi_driver *driver,
> > + struct acpi_device *device, acpi_handle handle,
> > + u32 type, void *data)
> > +{
> > + BUG_ON(!driver);
>
> Rule: Don't crash the kernel if you don't have to. Try to recover instead.
>
> It seems that
>
> if (WARN_ON(!driver))
> return;
>
> would be sufficient in this particulare case, wouldn't it?
Agreed.
> > +
> > + if (driver->ops.sys_notify)
> > + driver->ops.sys_notify(handle, type, data);
> > + else if (device && driver->ops.notify &&
>
> Why "else if"? The existing code does this unconditionally. Is that incorrect?
Good point. I agree that this should be changed to "if".
ACPI_DRIVER_ALL_NOTIFY_EVENTS could be deprecated after .sys_notify is
added, though.
> > + (driver->flags & ACPI_DRIVER_ALL_NOTIFY_EVENTS))
> > + driver->ops.notify(device, type);
> > +
> > + return;
> > +}
> > +
> > +/**
> > + * acpi_bus_notify: The system-level global notify handler
> > + *
> > + * The global notify handler for all 'system-level' device notifications
> > + * (values 0x00-0x7F). This handler calls a driver's notify handler for
> > + * the notified ACPI device.
> > + */
> > +static void acpi_bus_notify(acpi_handle handle, u32 type, void *data)
> > +{
> > + struct acpi_device *device = NULL;
> > +
> > + /* call registered handlers in the bus notify list */
> > + blocking_notifier_call_chain(&acpi_bus_notify_list,
> > + type, (void *)handle);
> >
> > + /* obtain an associated driver if already bound */
> > acpi_bus_get_device(handle, &device);
> > - if (device) {
> > - driver = device->driver;
> > - if (driver && driver->ops.notify &&
> > - (driver->flags & ACPI_DRIVER_ALL_NOTIFY_EVENTS))
> > - driver->ops.notify(device, type);
> > - }
> > +
> > + /* call the driver's system-level notify handler */
> > + if (device && device->driver)
> > + acpi_bus_drv_notify(device->driver, device, handle, type, data);
> > + else
> > + acpi_lookup_drv_notify(handle, type, data);
> > +
> > + return;
> > }
> >
> > /* --------------------------------------------------------------------------
> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > index 95ff1e8..333e57f 100644
> > --- a/drivers/acpi/scan.c
> > +++ b/drivers/acpi/scan.c
> > @@ -1758,3 +1758,86 @@ int __init acpi_scan_init(void)
> >
> > return result;
> > }
> > +
> > +/* callback args for acpi_match_drv_notify() */
> > +struct acpi_notify_args {
> > + struct acpi_device *device;
> > + acpi_handle handle;
> > + u32 event;
> > + void *data;
> > +};
> > +
> > +static int acpi_match_drv_notify(struct device_driver *drv, void *data)
> > +{
> > + struct acpi_driver *driver = to_acpi_driver(drv);
> > + struct acpi_notify_args *args = (struct acpi_notify_args *) data;
> > +
> > + /* check if this driver matches with the device */
> > + if (acpi_match_device_ids(args->device, driver->ids))
> > + return 0;
> > +
> > + /* call the driver's notify handler */
> > + acpi_bus_drv_notify(driver, NULL, args->handle,
> > + args->event, args->data);
> > +
> > + return 1;
> > +}
> > +
> > +/**
> > + * acpi_lookup_drv_notify: Look up and call driver's notify handler
> > + * @handle: ACPI handle of the notified device object
> > + * @event: Notify event
> > + * @data: Context
> > + *
> > + * Look up and call the associated driver's notify handler for the ACPI
> > + * device object by walking through the list of ACPI drivers.
>
> What exactly is the purpose of this?
For hot-add, an acpi_device object is not created for the notified
object yet. Therefore, acpi_bus_notify() calls this function to find an
associated driver for the device. It walks thru the ACPI driver list to
find a matching driver.
> > + */
> > +void acpi_lookup_drv_notify(acpi_handle handle, u32 event, void *data)
> > +{
> > + struct acpi_notify_args args;
> > + struct acpi_device *device;
> > + unsigned long long sta;
> > + int type;
> > + int ret;
> > +
> > + /* allocate a temporary device object */
> > + device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
> > + if (!device) {
> > + pr_err(PREFIX "No memory to allocate a tmp device\n");
> > + return;
> > + }
> > + INIT_LIST_HEAD(&device->pnp.ids);
> > +
> > + /* obtain device type */
> > + ret = acpi_bus_type_and_status(handle, &type, &sta);
> > + if (ret) {
> > + pr_err(PREFIX "Failed to get device type\n");
> > + goto out;
> > + }
> > +
> > + /* setup this temporary device object */
> > + device->device_type = type;
> > + device->handle = handle;
> > + device->parent = acpi_bus_get_parent(handle);
> > + device->dev.bus = &acpi_bus_type;
> > + device->driver = NULL;
> > + STRUCT_TO_INT(device->status) = sta;
> > + device->status.present = 1;
> > +
> > + /* set HID to this device object */
> > + acpi_device_set_id(device);
> > +
> > + /* set args */
> > + args.device = device;
> > + args.handle = handle;
> > + args.event = event;
> > + args.data = data;
> > +
> > + /* call a matched driver's notify handler */
> > + (void) bus_for_each_drv(device->dev.bus, NULL,
> > + &args, acpi_match_drv_notify);
>
> Excuse me? What makes you think I would accept anything like this?
Sorry, I admit that allocating a temporary acpi_device object is a hack
since acpi_device_set_id() requires it. I tried to change
acpi_device_set_id(), but it needed more changes than I expected. I can
try to clean this up, if the overall design still makes sense.
Thanks,
-Toshi
> > +
> > +out:
> > + acpi_device_release(&device->dev);
> > + return;
> > +}
> > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> > index 2242c10..4052406 100644
> > --- a/include/acpi/acpi_bus.h
> > +++ b/include/acpi/acpi_bus.h
> > @@ -94,6 +94,7 @@ typedef int (*acpi_op_start) (struct acpi_device * device);
> > typedef int (*acpi_op_bind) (struct acpi_device * device);
> > typedef int (*acpi_op_unbind) (struct acpi_device * device);
> > typedef void (*acpi_op_notify) (struct acpi_device * device, u32 event);
> > +typedef void (*acpi_op_sys_notify) (acpi_handle handle, u32 event, void *data);
> >
> > struct acpi_bus_ops {
> > u32 acpi_op_add:1;
> > @@ -107,6 +108,7 @@ struct acpi_device_ops {
> > acpi_op_bind bind;
> > acpi_op_unbind unbind;
> > acpi_op_notify notify;
> > + acpi_op_sys_notify sys_notify;
> > };
> >
> > #define ACPI_DRIVER_ALL_NOTIFY_EVENTS 0x1 /* system AND device events */
> > @@ -328,6 +330,10 @@ extern int unregister_acpi_notifier(struct notifier_block *);
> >
> > extern int register_acpi_bus_notifier(struct notifier_block *nb);
> > extern void unregister_acpi_bus_notifier(struct notifier_block *nb);
> > +extern void acpi_lookup_drv_notify(acpi_handle handle, u32 event, void *data);
> > +extern void acpi_bus_drv_notify(struct acpi_driver *driver,
> > + struct acpi_device *device, acpi_handle handle, u32 type, void *data);
> > +
> > /*
> > * External Functions
> > */
> >
>
> Thanks,
> Rafael
>
>
On Sat, 2012-11-24 at 23:37 +0100, Rafael J. Wysocki wrote:
> On Saturday, November 24, 2012 11:01:56 PM Rafael J. Wysocki wrote:
> > On Thursday, November 08, 2012 01:23:44 PM Toshi Kani wrote:
> > > Added a new .sys_notify interface, which allows ACPI drivers to
> > > register their system-level (ex. hotplug) notify handlers through
> > > their acpi_driver table. This removes redundant ACPI namespace
> > > walks from ACPI drivers for faster booting.
> > >
> > > The global notify handler acpi_bus_notify() is called for all
> > > system-level ACPI notifications, which then calls an appropriate
> > > driver's handler if any. ACPI drivers no longer need to register
> > > or unregister driver's handler to each ACPI device object. It also
> > > supports dynamic ACPI namespace with LoadTable & Unload opcode
> > > without any modification in ACPI drivers.
> > >
> > > Added a common system notify handler acpi_bus_sys_notify(), which
> > > allows ACPI drivers to set it to .sys_notify when this function is
> > > fully implemented.
> >
> > I don't really understand this.
> >
> > > It removes functional conflict between driver's
> > > notify handler and the global notify handler acpi_bus_notify().
> > >
> > > Note that the changes maintain backward compatibility for ACPI
> > > drivers. Any drivers registered their hotplug handler through the
> > > existing interfaces, such as acpi_install_notify_handler() and
> > > register_acpi_bus_notifier(), will continue to work as before.
> >
> > I really wouldn't like to add new callbacks to struct acpi_device_ops, because
> > I'd like that whole thing to go away entirely eventually, along with struct
> > acpi_driver.
> >
> > Moreover, in this particular case, it really is not useful to have to define
> > a struct acpi_driver so that one can register for receiving system
> > notifications from ACPI. It would be really nice if non-ACPI drivers, such
> > as PCI or platform, could do that too.
>
> Which they do by using acpi_install_notify_handler() directly.
By using acpi_install_notify_handler(), each driver needs to walk
through the entire ACPI namespace to find its associated ACPI devices
and call it to register one by one. I think this is more work for
non-ACPI drivers than defining acpi_driver. Furthermore, this approach
will impose some major issues below. (NOTE: Hot-plug implementation
varies in platforms/virtual machines. These are examples from our IA64
platforms supported by other OS, but I hope Linux would support similar
capability in future.)
a) Node Hot-plug
When a new node is added, the FW may extend the ACPI namespace by
loading SSDT for the new node. Therefore, if we rely on drivers to call
acpi_install_notify_handler(), we need to make the drivers to walk the
namespace again to call it for new devices. Similarly, the drivers need
to call acpi_remove_notify_handler() when a node is removed.
b) Memory hot-plug
The FW may slice up the memory range with multiple memory device objects
so that logical hot-add/removal can be performed in finer granularity
for better resource balancing. For example, a system with 4TB memory
sliced up with 1GB memory device objects will have (4 * 1024) memory
device objects in ACPI namespace. If each driver walks ACPI namespace,
it can lead noticeable delay in such environment. The number of objects
can easily go up when supporting more finer granularity or more amount
of memory.
> > Besides, acpi_os_execute_deferred() is always run on CPU0, because of some
> > SMI-related peculiarity, which is not very efficient as far as the events
> > handling is concerned, but we can improve the situation a bit by queing the
> > execution of the registered handlers in a different workqueue. Maybe it's
> > worth considering if we're going to change this code anyway?
In my experience, serializing hot-plug operations within an OS instance
is not typically an issue, and makes it much easier for testing and
diagnosing an issue.
> Well, perhaps we really don't need to change it after all? Maybe we can just
> switch everyone to using acpi_install_notify_handler() and then we can just
> drop that code entirely?
I am concerned with the approach of each driver calling
acpi_install_notify_handler() directly, as described above.
Thanks,
-Toshi
> > > {
> > > - struct acpi_device *device = NULL;
> > > - struct acpi_driver *driver;
> > > -
> > > ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Notification %#02x to handle %p\n",
> > > type, handle));
> > >
> > > - blocking_notifier_call_chain(&acpi_bus_notify_list,
> > > - type, (void *)handle);
>
> By the way, there is exacly one user of this chain, which is dock.c.
>
> What about convering that to something different and dropping the chain to
> start with?
I agree. I will look further to see what interface we can use to
replace it.
Thanks,
-Toshi
On Monday, November 26, 2012 12:06:39 PM Toshi Kani wrote:
> On Sat, 2012-11-24 at 23:37 +0100, Rafael J. Wysocki wrote:
> > On Saturday, November 24, 2012 11:01:56 PM Rafael J. Wysocki wrote:
> > > On Thursday, November 08, 2012 01:23:44 PM Toshi Kani wrote:
> > > > Added a new .sys_notify interface, which allows ACPI drivers to
> > > > register their system-level (ex. hotplug) notify handlers through
> > > > their acpi_driver table. This removes redundant ACPI namespace
> > > > walks from ACPI drivers for faster booting.
> > > >
> > > > The global notify handler acpi_bus_notify() is called for all
> > > > system-level ACPI notifications, which then calls an appropriate
> > > > driver's handler if any. ACPI drivers no longer need to register
> > > > or unregister driver's handler to each ACPI device object. It also
> > > > supports dynamic ACPI namespace with LoadTable & Unload opcode
> > > > without any modification in ACPI drivers.
> > > >
> > > > Added a common system notify handler acpi_bus_sys_notify(), which
> > > > allows ACPI drivers to set it to .sys_notify when this function is
> > > > fully implemented.
> > >
> > > I don't really understand this.
> > >
> > > > It removes functional conflict between driver's
> > > > notify handler and the global notify handler acpi_bus_notify().
> > > >
> > > > Note that the changes maintain backward compatibility for ACPI
> > > > drivers. Any drivers registered their hotplug handler through the
> > > > existing interfaces, such as acpi_install_notify_handler() and
> > > > register_acpi_bus_notifier(), will continue to work as before.
> > >
> > > I really wouldn't like to add new callbacks to struct acpi_device_ops, because
> > > I'd like that whole thing to go away entirely eventually, along with struct
> > > acpi_driver.
> > >
> > > Moreover, in this particular case, it really is not useful to have to define
> > > a struct acpi_driver so that one can register for receiving system
> > > notifications from ACPI. It would be really nice if non-ACPI drivers, such
> > > as PCI or platform, could do that too.
> >
> > Which they do by using acpi_install_notify_handler() directly.
>
> By using acpi_install_notify_handler(), each driver needs to walk
> through the entire ACPI namespace to find its associated ACPI devices
> and call it to register one by one. I think this is more work for
> non-ACPI drivers than defining acpi_driver.
I'm not really sure what you mean. The drivers in question already know
what the relevant ACPI device nodes are (because they need them anyway
for other purposes), so they don't need to look for them specifically and
acpi_install_notify_handler() doesn't do any namespace walking. So what
you said above simply doesn't make sense from this viewpoint.
> Furthermore, this approach
> will impose some major issues below. (NOTE: Hot-plug implementation
> varies in platforms/virtual machines. These are examples from our IA64
> platforms supported by other OS, but I hope Linux would support similar
> capability in future.)
>
> a) Node Hot-plug
> When a new node is added, the FW may extend the ACPI namespace by
> loading SSDT for the new node. Therefore, if we rely on drivers to call
> acpi_install_notify_handler(), we need to make the drivers to walk the
> namespace again to call it for new devices. Similarly, the drivers need
> to call acpi_remove_notify_handler() when a node is removed.
I'm not sure how adding .sys_notify() is going to address this issue.
In order to use .sys_notify() your driver has to bind to a struct
acpi_device in the first place, so you need to know that object to use it
anyway. This isn't any different from having a struct device whose
ACPI_HANDLE() has been populated by the core and using
acpi_install_notify_handler() directly on that.
> b) Memory hot-plug
> The FW may slice up the memory range with multiple memory device objects
> so that logical hot-add/removal can be performed in finer granularity
> for better resource balancing. For example, a system with 4TB memory
> sliced up with 1GB memory device objects will have (4 * 1024) memory
> device objects in ACPI namespace. If each driver walks ACPI namespace,
> it can lead noticeable delay in such environment. The number of objects
> can easily go up when supporting more finer granularity or more amount
> of memory.
Again, I don't see why drivers would have to walk the namespace.
It would be great if you could give a specific example of that.
> > > Besides, acpi_os_execute_deferred() is always run on CPU0, because of some
> > > SMI-related peculiarity, which is not very efficient as far as the events
> > > handling is concerned, but we can improve the situation a bit by queing the
> > > execution of the registered handlers in a different workqueue. Maybe it's
> > > worth considering if we're going to change this code anyway?
>
> In my experience, serializing hot-plug operations within an OS instance
> is not typically an issue, and makes it much easier for testing and
> diagnosing an issue.
>
>
> > Well, perhaps we really don't need to change it after all? Maybe we can just
> > switch everyone to using acpi_install_notify_handler() and then we can just
> > drop that code entirely?
>
> I am concerned with the approach of each driver calling
> acpi_install_notify_handler() directly, as described above.
Depending on how it is implemented, it shouldn't be much more computationally
expensive than using .sys_notify() as proposed and the benefit would be
everyone using the same well tested interface that's being used already by
almost everyone anyway.
To make things clear, I'm actually going to drop that whole useless system
notification code from bus.c shortly. We can add something in its place later,
but this one is not worth fixing in my opinion. Let alone extending it.
And as far as acpi_drivers are concerned, please consider them as an obsolete
thing and try to avoid using them and extending their interfaces. If you have
problems with that, please let me know.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Mon, 2012-11-26 at 21:44 +0100, Rafael J. Wysocki wrote:
> On Monday, November 26, 2012 12:06:39 PM Toshi Kani wrote:
> > On Sat, 2012-11-24 at 23:37 +0100, Rafael J. Wysocki wrote:
> > > On Saturday, November 24, 2012 11:01:56 PM Rafael J. Wysocki wrote:
> > > > On Thursday, November 08, 2012 01:23:44 PM Toshi Kani wrote:
> > > > > Added a new .sys_notify interface, which allows ACPI drivers to
> > > > > register their system-level (ex. hotplug) notify handlers through
> > > > > their acpi_driver table. This removes redundant ACPI namespace
> > > > > walks from ACPI drivers for faster booting.
> > > > >
> > > > > The global notify handler acpi_bus_notify() is called for all
> > > > > system-level ACPI notifications, which then calls an appropriate
> > > > > driver's handler if any. ACPI drivers no longer need to register
> > > > > or unregister driver's handler to each ACPI device object. It also
> > > > > supports dynamic ACPI namespace with LoadTable & Unload opcode
> > > > > without any modification in ACPI drivers.
> > > > >
> > > > > Added a common system notify handler acpi_bus_sys_notify(), which
> > > > > allows ACPI drivers to set it to .sys_notify when this function is
> > > > > fully implemented.
> > > >
> > > > I don't really understand this.
> > > >
> > > > > It removes functional conflict between driver's
> > > > > notify handler and the global notify handler acpi_bus_notify().
> > > > >
> > > > > Note that the changes maintain backward compatibility for ACPI
> > > > > drivers. Any drivers registered their hotplug handler through the
> > > > > existing interfaces, such as acpi_install_notify_handler() and
> > > > > register_acpi_bus_notifier(), will continue to work as before.
> > > >
> > > > I really wouldn't like to add new callbacks to struct acpi_device_ops, because
> > > > I'd like that whole thing to go away entirely eventually, along with struct
> > > > acpi_driver.
> > > >
> > > > Moreover, in this particular case, it really is not useful to have to define
> > > > a struct acpi_driver so that one can register for receiving system
> > > > notifications from ACPI. It would be really nice if non-ACPI drivers, such
> > > > as PCI or platform, could do that too.
> > >
> > > Which they do by using acpi_install_notify_handler() directly.
> >
> > By using acpi_install_notify_handler(), each driver needs to walk
> > through the entire ACPI namespace to find its associated ACPI devices
> > and call it to register one by one. I think this is more work for
> > non-ACPI drivers than defining acpi_driver.
>
> I'm not really sure what you mean. The drivers in question already know
> what the relevant ACPI device nodes are (because they need them anyway
> for other purposes), so they don't need to look for them specifically and
> acpi_install_notify_handler() doesn't do any namespace walking. So what
> you said above simply doesn't make sense from this viewpoint.
Yes, if drivers already know the relevant ACPI devices, then walking the
ACPI namespace is not necessary. I was referring the case like
processor_driver.c, acpi_memhotplug.c, and container.c in my statement.
> > Furthermore, this approach
> > will impose some major issues below. (NOTE: Hot-plug implementation
> > varies in platforms/virtual machines. These are examples from our IA64
> > platforms supported by other OS, but I hope Linux would support similar
> > capability in future.)
> >
> > a) Node Hot-plug
> > When a new node is added, the FW may extend the ACPI namespace by
> > loading SSDT for the new node. Therefore, if we rely on drivers to call
> > acpi_install_notify_handler(), we need to make the drivers to walk the
> > namespace again to call it for new devices. Similarly, the drivers need
> > to call acpi_remove_notify_handler() when a node is removed.
>
> I'm not sure how adding .sys_notify() is going to address this issue.
> In order to use .sys_notify() your driver has to bind to a struct
> acpi_device in the first place, so you need to know that object to use it
> anyway. This isn't any different from having a struct device whose
> ACPI_HANDLE() has been populated by the core and using
> acpi_install_notify_handler() directly on that.
No, .sys_notify() does not take acpi_device as an argument. So, the
driver does not have to bind to an acpi_device previously. The only
requirement is that the driver needs to call acpi_bus_register_driver()
previously.
> > b) Memory hot-plug
> > The FW may slice up the memory range with multiple memory device objects
> > so that logical hot-add/removal can be performed in finer granularity
> > for better resource balancing. For example, a system with 4TB memory
> > sliced up with 1GB memory device objects will have (4 * 1024) memory
> > device objects in ACPI namespace. If each driver walks ACPI namespace,
> > it can lead noticeable delay in such environment. The number of objects
> > can easily go up when supporting more finer granularity or more amount
> > of memory.
>
> Again, I don't see why drivers would have to walk the namespace.
>
> It would be great if you could give a specific example of that.
Again, processor_driver.c, acpi_memhotplug.c, and container.c are
examples of such case.
> > > > Besides, acpi_os_execute_deferred() is always run on CPU0, because of some
> > > > SMI-related peculiarity, which is not very efficient as far as the events
> > > > handling is concerned, but we can improve the situation a bit by queing the
> > > > execution of the registered handlers in a different workqueue. Maybe it's
> > > > worth considering if we're going to change this code anyway?
> >
> > In my experience, serializing hot-plug operations within an OS instance
> > is not typically an issue, and makes it much easier for testing and
> > diagnosing an issue.
> >
> >
> > > Well, perhaps we really don't need to change it after all? Maybe we can just
> > > switch everyone to using acpi_install_notify_handler() and then we can just
> > > drop that code entirely?
> >
> > I am concerned with the approach of each driver calling
> > acpi_install_notify_handler() directly, as described above.
>
> Depending on how it is implemented, it shouldn't be much more computationally
> expensive than using .sys_notify() as proposed and the benefit would be
> everyone using the same well tested interface that's being used already by
> almost everyone anyway.
>
> To make things clear, I'm actually going to drop that whole useless system
> notification code from bus.c shortly. We can add something in its place later,
> but this one is not worth fixing in my opinion. Let alone extending it.
Thanks for the clarification. Yeah, if the new code will address the
above issues, that's great and I do not need to stick with my patchset.
I just need to know how it works. :)
> And as far as acpi_drivers are concerned, please consider them as an obsolete
> thing and try to avoid using them and extending their interfaces. If you have
> problems with that, please let me know.
Understood.
Thanks,
-Toshi
On Mon, 2012-11-26 at 14:09 -0700, Toshi Kani wrote:
> On Mon, 2012-11-26 at 21:44 +0100, Rafael J. Wysocki wrote:
> > On Monday, November 26, 2012 12:06:39 PM Toshi Kani wrote:
> > > On Sat, 2012-11-24 at 23:37 +0100, Rafael J. Wysocki wrote:
> > > > On Saturday, November 24, 2012 11:01:56 PM Rafael J. Wysocki wrote:
> > > > > On Thursday, November 08, 2012 01:23:44 PM Toshi Kani wrote:
> > > > > > Added a new .sys_notify interface, which allows ACPI drivers to
> > > > > > register their system-level (ex. hotplug) notify handlers through
> > > > > > their acpi_driver table. This removes redundant ACPI namespace
> > > > > > walks from ACPI drivers for faster booting.
> > > > > >
> > > > > > The global notify handler acpi_bus_notify() is called for all
> > > > > > system-level ACPI notifications, which then calls an appropriate
> > > > > > driver's handler if any. ACPI drivers no longer need to register
> > > > > > or unregister driver's handler to each ACPI device object. It also
> > > > > > supports dynamic ACPI namespace with LoadTable & Unload opcode
> > > > > > without any modification in ACPI drivers.
> > > > > >
> > > > > > Added a common system notify handler acpi_bus_sys_notify(), which
> > > > > > allows ACPI drivers to set it to .sys_notify when this function is
> > > > > > fully implemented.
> > > > >
> > > > > I don't really understand this.
> > > > >
> > > > > > It removes functional conflict between driver's
> > > > > > notify handler and the global notify handler acpi_bus_notify().
> > > > > >
> > > > > > Note that the changes maintain backward compatibility for ACPI
> > > > > > drivers. Any drivers registered their hotplug handler through the
> > > > > > existing interfaces, such as acpi_install_notify_handler() and
> > > > > > register_acpi_bus_notifier(), will continue to work as before.
> > > > >
> > > > > I really wouldn't like to add new callbacks to struct acpi_device_ops, because
> > > > > I'd like that whole thing to go away entirely eventually, along with struct
> > > > > acpi_driver.
> > > > >
> > > > > Moreover, in this particular case, it really is not useful to have to define
> > > > > a struct acpi_driver so that one can register for receiving system
> > > > > notifications from ACPI. It would be really nice if non-ACPI drivers, such
> > > > > as PCI or platform, could do that too.
> > > >
> > > > Which they do by using acpi_install_notify_handler() directly.
> > >
> > > By using acpi_install_notify_handler(), each driver needs to walk
> > > through the entire ACPI namespace to find its associated ACPI devices
> > > and call it to register one by one. I think this is more work for
> > > non-ACPI drivers than defining acpi_driver.
> >
> > I'm not really sure what you mean. The drivers in question already know
> > what the relevant ACPI device nodes are (because they need them anyway
> > for other purposes), so they don't need to look for them specifically and
> > acpi_install_notify_handler() doesn't do any namespace walking. So what
> > you said above simply doesn't make sense from this viewpoint.
>
> Yes, if drivers already know the relevant ACPI devices, then walking the
> ACPI namespace is not necessary. I was referring the case like
> processor_driver.c, acpi_memhotplug.c, and container.c in my statement.
BTW, when an ACPI device is marked as non-present, which is the case
before hot-add, we do not create an acpi_device object and therefore do
not bind it with a driver. This is why these drivers walk the ACPI
namespace and install their notify handlers regardless of device status.
Thanks,
-Toshi
On Monday, November 26, 2012 12:06:39 PM Toshi Kani wrote:
> On Sat, 2012-11-24 at 23:37 +0100, Rafael J. Wysocki wrote:
> > On Saturday, November 24, 2012 11:01:56 PM Rafael J. Wysocki wrote:
> > > On Thursday, November 08, 2012 01:23:44 PM Toshi Kani wrote:
> > > > Added a new .sys_notify interface, which allows ACPI drivers to
> > > > register their system-level (ex. hotplug) notify handlers through
> > > > their acpi_driver table. This removes redundant ACPI namespace
> > > > walks from ACPI drivers for faster booting.
> > > >
> > > > The global notify handler acpi_bus_notify() is called for all
> > > > system-level ACPI notifications, which then calls an appropriate
> > > > driver's handler if any. ACPI drivers no longer need to register
> > > > or unregister driver's handler to each ACPI device object. It also
> > > > supports dynamic ACPI namespace with LoadTable & Unload opcode
> > > > without any modification in ACPI drivers.
> > > >
> > > > Added a common system notify handler acpi_bus_sys_notify(), which
> > > > allows ACPI drivers to set it to .sys_notify when this function is
> > > > fully implemented.
> > >
> > > I don't really understand this.
> > >
> > > > It removes functional conflict between driver's
> > > > notify handler and the global notify handler acpi_bus_notify().
> > > >
> > > > Note that the changes maintain backward compatibility for ACPI
> > > > drivers. Any drivers registered their hotplug handler through the
> > > > existing interfaces, such as acpi_install_notify_handler() and
> > > > register_acpi_bus_notifier(), will continue to work as before.
> > >
> > > I really wouldn't like to add new callbacks to struct acpi_device_ops, because
> > > I'd like that whole thing to go away entirely eventually, along with struct
> > > acpi_driver.
> > >
> > > Moreover, in this particular case, it really is not useful to have to define
> > > a struct acpi_driver so that one can register for receiving system
> > > notifications from ACPI. It would be really nice if non-ACPI drivers, such
> > > as PCI or platform, could do that too.
> >
> > Which they do by using acpi_install_notify_handler() directly.
>
> By using acpi_install_notify_handler(), each driver needs to walk
> through the entire ACPI namespace to find its associated ACPI devices
> and call it to register one by one. I think this is more work for
> non-ACPI drivers than defining acpi_driver.
I'm not really sure what you mean. The drivers in question already know
what the relevant ACPI device nodes are (because they need them anyway
for other purposes), so they don't need to look for them specifically and
acpi_install_notify_handler() doesn't do any namespace walking. So what
you said above simply doesn't make sense from this viewpoint.
> Furthermore, this approach
> will impose some major issues below. (NOTE: Hot-plug implementation
> varies in platforms/virtual machines. These are examples from our IA64
> platforms supported by other OS, but I hope Linux would support similar
> capability in future.)
>
> a) Node Hot-plug
> When a new node is added, the FW may extend the ACPI namespace by
> loading SSDT for the new node. Therefore, if we rely on drivers to call
> acpi_install_notify_handler(), we need to make the drivers to walk the
> namespace again to call it for new devices. Similarly, the drivers need
> to call acpi_remove_notify_handler() when a node is removed.
I'm not sure how adding .sys_notify() is going to address this issue.
In order to use .sys_notify() your driver has to bind to a struct
acpi_device in the first place, so you need to know that object to use it
anyway. This isn't any different from having a struct device whose
ACPI_HANDLE() has been populated by the core and using
acpi_install_notify_handler() directly on that.
> b) Memory hot-plug
> The FW may slice up the memory range with multiple memory device objects
> so that logical hot-add/removal can be performed in finer granularity
> for better resource balancing. For example, a system with 4TB memory
> sliced up with 1GB memory device objects will have (4 * 1024) memory
> device objects in ACPI namespace. If each driver walks ACPI namespace,
> it can lead noticeable delay in such environment. The number of objects
> can easily go up when supporting more finer granularity or more amount
> of memory.
Again, I don't see why drivers would have to walk the namespace.
It would be great if you could give a specific example of that.
> > > Besides, acpi_os_execute_deferred() is always run on CPU0, because of some
> > > SMI-related peculiarity, which is not very efficient as far as the events
> > > handling is concerned, but we can improve the situation a bit by queing the
> > > execution of the registered handlers in a different workqueue. Maybe it's
> > > worth considering if we're going to change this code anyway?
>
> In my experience, serializing hot-plug operations within an OS instance
> is not typically an issue, and makes it much easier for testing and
> diagnosing an issue.
>
>
> > Well, perhaps we really don't need to change it after all? Maybe we can just
> > switch everyone to using acpi_install_notify_handler() and then we can just
> > drop that code entirely?
>
> I am concerned with the approach of each driver calling
> acpi_install_notify_handler() directly, as described above.
Depending on how it is implemented, it shouldn't be much more computationally
expensive than using .sys_notify() as proposed and the benefit would be
everyone using the same well tested interface that's being used already by
almost everyone anyway.
To make things clear, I'm actually going to drop that whole useless system
notification code from bus.c shortly. We can add something in its place later,
but this one is not worth fixing in my opinion. Let alone extending it.
And as far as acpi_drivers are concerned, please consider them as an obsolete
thing and try to avoid using them and extending their interfaces. If you have
problems with that, please let me know.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Monday, November 26, 2012 02:09:54 PM Toshi Kani wrote:
> On Mon, 2012-11-26 at 21:44 +0100, Rafael J. Wysocki wrote:
> > On Monday, November 26, 2012 12:06:39 PM Toshi Kani wrote:
> > > On Sat, 2012-11-24 at 23:37 +0100, Rafael J. Wysocki wrote:
> > > > On Saturday, November 24, 2012 11:01:56 PM Rafael J. Wysocki wrote:
> > > > > On Thursday, November 08, 2012 01:23:44 PM Toshi Kani wrote:
> > > > > > Added a new .sys_notify interface, which allows ACPI drivers to
> > > > > > register their system-level (ex. hotplug) notify handlers through
> > > > > > their acpi_driver table. This removes redundant ACPI namespace
> > > > > > walks from ACPI drivers for faster booting.
> > > > > >
> > > > > > The global notify handler acpi_bus_notify() is called for all
> > > > > > system-level ACPI notifications, which then calls an appropriate
> > > > > > driver's handler if any. ACPI drivers no longer need to register
> > > > > > or unregister driver's handler to each ACPI device object. It also
> > > > > > supports dynamic ACPI namespace with LoadTable & Unload opcode
> > > > > > without any modification in ACPI drivers.
> > > > > >
> > > > > > Added a common system notify handler acpi_bus_sys_notify(), which
> > > > > > allows ACPI drivers to set it to .sys_notify when this function is
> > > > > > fully implemented.
> > > > >
> > > > > I don't really understand this.
> > > > >
> > > > > > It removes functional conflict between driver's
> > > > > > notify handler and the global notify handler acpi_bus_notify().
> > > > > >
> > > > > > Note that the changes maintain backward compatibility for ACPI
> > > > > > drivers. Any drivers registered their hotplug handler through the
> > > > > > existing interfaces, such as acpi_install_notify_handler() and
> > > > > > register_acpi_bus_notifier(), will continue to work as before.
> > > > >
> > > > > I really wouldn't like to add new callbacks to struct acpi_device_ops, because
> > > > > I'd like that whole thing to go away entirely eventually, along with struct
> > > > > acpi_driver.
> > > > >
> > > > > Moreover, in this particular case, it really is not useful to have to define
> > > > > a struct acpi_driver so that one can register for receiving system
> > > > > notifications from ACPI. It would be really nice if non-ACPI drivers, such
> > > > > as PCI or platform, could do that too.
> > > >
> > > > Which they do by using acpi_install_notify_handler() directly.
> > >
> > > By using acpi_install_notify_handler(), each driver needs to walk
> > > through the entire ACPI namespace to find its associated ACPI devices
> > > and call it to register one by one. I think this is more work for
> > > non-ACPI drivers than defining acpi_driver.
> >
> > I'm not really sure what you mean. The drivers in question already know
> > what the relevant ACPI device nodes are (because they need them anyway
> > for other purposes), so they don't need to look for them specifically and
> > acpi_install_notify_handler() doesn't do any namespace walking. So what
> > you said above simply doesn't make sense from this viewpoint.
>
> Yes, if drivers already know the relevant ACPI devices, then walking the
> ACPI namespace is not necessary. I was referring the case like
> processor_driver.c, acpi_memhotplug.c, and container.c in my statement.
OK, I need to have a deeper look at those things and I'm not sure when that
happens (everybody is sending me patches like mad right now).
> > > Furthermore, this approach
> > > will impose some major issues below. (NOTE: Hot-plug implementation
> > > varies in platforms/virtual machines. These are examples from our IA64
> > > platforms supported by other OS, but I hope Linux would support similar
> > > capability in future.)
> > >
> > > a) Node Hot-plug
> > > When a new node is added, the FW may extend the ACPI namespace by
> > > loading SSDT for the new node. Therefore, if we rely on drivers to call
> > > acpi_install_notify_handler(), we need to make the drivers to walk the
> > > namespace again to call it for new devices. Similarly, the drivers need
> > > to call acpi_remove_notify_handler() when a node is removed.
> >
> > I'm not sure how adding .sys_notify() is going to address this issue.
> > In order to use .sys_notify() your driver has to bind to a struct
> > acpi_device in the first place, so you need to know that object to use it
> > anyway. This isn't any different from having a struct device whose
> > ACPI_HANDLE() has been populated by the core and using
> > acpi_install_notify_handler() directly on that.
>
> No, .sys_notify() does not take acpi_device as an argument.
acpi_bus_notify() finds a struct acpi_device for the given handle, however,
and calls .sys_notify() for the driver attached to it. So a driver has to
attach to a struct device to use it (except in the broken
acpi_lookup_drv_notify() case, but that's really broken, so let's just not
consider it even).
> So, the driver does not have to bind to an acpi_device previously. The only
> requirement is that the driver needs to call acpi_bus_register_driver()
> previously.
>
>
> > > b) Memory hot-plug
> > > The FW may slice up the memory range with multiple memory device objects
> > > so that logical hot-add/removal can be performed in finer granularity
> > > for better resource balancing. For example, a system with 4TB memory
> > > sliced up with 1GB memory device objects will have (4 * 1024) memory
> > > device objects in ACPI namespace. If each driver walks ACPI namespace,
> > > it can lead noticeable delay in such environment. The number of objects
> > > can easily go up when supporting more finer granularity or more amount
> > > of memory.
> >
> > Again, I don't see why drivers would have to walk the namespace.
> >
> > It would be great if you could give a specific example of that.
>
> Again, processor_driver.c, acpi_memhotplug.c, and container.c are
> examples of such case.
OK, I'll have a look.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Monday, November 26, 2012 02:24:09 PM Toshi Kani wrote:
> On Mon, 2012-11-26 at 14:09 -0700, Toshi Kani wrote:
> > On Mon, 2012-11-26 at 21:44 +0100, Rafael J. Wysocki wrote:
> > > On Monday, November 26, 2012 12:06:39 PM Toshi Kani wrote:
> > > > On Sat, 2012-11-24 at 23:37 +0100, Rafael J. Wysocki wrote:
> > > > > On Saturday, November 24, 2012 11:01:56 PM Rafael J. Wysocki wrote:
> > > > > > On Thursday, November 08, 2012 01:23:44 PM Toshi Kani wrote:
> > > > > > > Added a new .sys_notify interface, which allows ACPI drivers to
> > > > > > > register their system-level (ex. hotplug) notify handlers through
> > > > > > > their acpi_driver table. This removes redundant ACPI namespace
> > > > > > > walks from ACPI drivers for faster booting.
> > > > > > >
> > > > > > > The global notify handler acpi_bus_notify() is called for all
> > > > > > > system-level ACPI notifications, which then calls an appropriate
> > > > > > > driver's handler if any. ACPI drivers no longer need to register
> > > > > > > or unregister driver's handler to each ACPI device object. It also
> > > > > > > supports dynamic ACPI namespace with LoadTable & Unload opcode
> > > > > > > without any modification in ACPI drivers.
> > > > > > >
> > > > > > > Added a common system notify handler acpi_bus_sys_notify(), which
> > > > > > > allows ACPI drivers to set it to .sys_notify when this function is
> > > > > > > fully implemented.
> > > > > >
> > > > > > I don't really understand this.
> > > > > >
> > > > > > > It removes functional conflict between driver's
> > > > > > > notify handler and the global notify handler acpi_bus_notify().
> > > > > > >
> > > > > > > Note that the changes maintain backward compatibility for ACPI
> > > > > > > drivers. Any drivers registered their hotplug handler through the
> > > > > > > existing interfaces, such as acpi_install_notify_handler() and
> > > > > > > register_acpi_bus_notifier(), will continue to work as before.
> > > > > >
> > > > > > I really wouldn't like to add new callbacks to struct acpi_device_ops, because
> > > > > > I'd like that whole thing to go away entirely eventually, along with struct
> > > > > > acpi_driver.
> > > > > >
> > > > > > Moreover, in this particular case, it really is not useful to have to define
> > > > > > a struct acpi_driver so that one can register for receiving system
> > > > > > notifications from ACPI. It would be really nice if non-ACPI drivers, such
> > > > > > as PCI or platform, could do that too.
> > > > >
> > > > > Which they do by using acpi_install_notify_handler() directly.
> > > >
> > > > By using acpi_install_notify_handler(), each driver needs to walk
> > > > through the entire ACPI namespace to find its associated ACPI devices
> > > > and call it to register one by one. I think this is more work for
> > > > non-ACPI drivers than defining acpi_driver.
> > >
> > > I'm not really sure what you mean. The drivers in question already know
> > > what the relevant ACPI device nodes are (because they need them anyway
> > > for other purposes), so they don't need to look for them specifically and
> > > acpi_install_notify_handler() doesn't do any namespace walking. So what
> > > you said above simply doesn't make sense from this viewpoint.
> >
> > Yes, if drivers already know the relevant ACPI devices, then walking the
> > ACPI namespace is not necessary. I was referring the case like
> > processor_driver.c, acpi_memhotplug.c, and container.c in my statement.
>
> BTW, when an ACPI device is marked as non-present, which is the case
> before hot-add, we do not create an acpi_device object and therefore do
> not bind it with a driver. This is why these drivers walk the ACPI
> namespace and install their notify handlers regardless of device status.
So maybe we should create struct acpi_device objects in that case too?
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Monday, November 26, 2012 10:44:08 AM Toshi Kani wrote:
> Hi Rafael,
>
> Thanks for reviewing! My comments are in-line.
>
> On Sat, 2012-11-24 at 23:01 +0100, Rafael J. Wysocki wrote:
> > On Thursday, November 08, 2012 01:23:44 PM Toshi Kani wrote:
> > > Added a new .sys_notify interface, which allows ACPI drivers to
> > > register their system-level (ex. hotplug) notify handlers through
> > > their acpi_driver table. This removes redundant ACPI namespace
> > > walks from ACPI drivers for faster booting.
> > >
> > > The global notify handler acpi_bus_notify() is called for all
> > > system-level ACPI notifications, which then calls an appropriate
> > > driver's handler if any. ACPI drivers no longer need to register
> > > or unregister driver's handler to each ACPI device object. It also
> > > supports dynamic ACPI namespace with LoadTable & Unload opcode
> > > without any modification in ACPI drivers.
> > >
> > > Added a common system notify handler acpi_bus_sys_notify(), which
> > > allows ACPI drivers to set it to .sys_notify when this function is
> > > fully implemented.
> >
> > I don't really understand this.
>
> Currently, acpi_bus_notify() is partially implemented as the common
> notify handler, but it may not be fully implemented under the current
> design. When a notify event is sent, ACPICA calls both
> acpi_bus_notify() and driver's handler registered through
> acpi_install_notify_handler(). However, a same event cannot be handled
> by both handlers.
Yes, it can, as long as they don't do conflicting things. :-)
Usually, the event will be discarded by one of them.
> Since acpi_bus_notify() may not know if an event has
> already been handled by driver's handler, it cannot do anything that may
> conflict with the driver's handler.
Not really. acpi_bus_notify() is always called first, so it actually
knows that no one has done anything with that event before. However,
it doesn't know who else will be called for the same event going
forward.
But I agree, acpi_bus_notify() shouldn't register for the same types of
events that are handled by drivers individually. And we may not need
acpi_bus_notify() at all as far as I can say at the moment.
> Therefore, the partially implemented common handler code in
> acpi_bus_notify() is moved to a separate function acpi_bus_sys_notify()
> in this patchset. This function can now be fully implemented as
> necessary.
Not really, because notifiers that don't use it may be called for the
same events.
> > > It removes functional conflict between driver's
> > > notify handler and the global notify handler acpi_bus_notify().
> > >
> > > Note that the changes maintain backward compatibility for ACPI
> > > drivers. Any drivers registered their hotplug handler through the
> > > existing interfaces, such as acpi_install_notify_handler() and
> > > register_acpi_bus_notifier(), will continue to work as before.
> >
> > I really wouldn't like to add new callbacks to struct acpi_device_ops, because
> > I'd like that whole thing to go away entirely eventually, along with struct
> > acpi_driver.
>
> acpi_device may need to be changed, but I think ACPI drivers are still
> necessary to support vendor-unique PNPIDs in an extendable way, which is
> currently done by adding drivers, such as asus_acpi_driver,
> cmpc_accel_acpi_driver, eeepc_acpi_driver, acpi_fujitsu_driver,
> lis3lv02d_driver, etc...
Well, not really. Handling different PNPIDs has nothing to do with ACPI
drivers. You only need a way for drivers in general to specify the ACPI
device IDs they can handle. And after som changes that are waiting for the
v3.8 merge windown you'll be able to add a list of ACPI device IDs to other
types of drivers too (like platform drivers for one example).
[...]
> > > +
> > > +/* callback args for acpi_match_drv_notify() */
> > > +struct acpi_notify_args {
> > > + struct acpi_device *device;
> > > + acpi_handle handle;
> > > + u32 event;
> > > + void *data;
> > > +};
> > > +
> > > +static int acpi_match_drv_notify(struct device_driver *drv, void *data)
> > > +{
> > > + struct acpi_driver *driver = to_acpi_driver(drv);
> > > + struct acpi_notify_args *args = (struct acpi_notify_args *) data;
> > > +
> > > + /* check if this driver matches with the device */
> > > + if (acpi_match_device_ids(args->device, driver->ids))
> > > + return 0;
> > > +
> > > + /* call the driver's notify handler */
> > > + acpi_bus_drv_notify(driver, NULL, args->handle,
> > > + args->event, args->data);
> > > +
> > > + return 1;
> > > +}
> > > +
> > > +/**
> > > + * acpi_lookup_drv_notify: Look up and call driver's notify handler
> > > + * @handle: ACPI handle of the notified device object
> > > + * @event: Notify event
> > > + * @data: Context
> > > + *
> > > + * Look up and call the associated driver's notify handler for the ACPI
> > > + * device object by walking through the list of ACPI drivers.
> >
> > What exactly is the purpose of this?
>
> For hot-add, an acpi_device object is not created for the notified
> object yet. Therefore, acpi_bus_notify() calls this function to find an
> associated driver for the device. It walks thru the ACPI driver list to
> find a matching driver.
This is just broken and not only because you're creating a struct acpi_device
object on the fly in there.
> > > + */
> > > +void acpi_lookup_drv_notify(acpi_handle handle, u32 event, void *data)
> > > +{
> > > + struct acpi_notify_args args;
> > > + struct acpi_device *device;
> > > + unsigned long long sta;
> > > + int type;
> > > + int ret;
> > > +
> > > + /* allocate a temporary device object */
> > > + device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
> > > + if (!device) {
> > > + pr_err(PREFIX "No memory to allocate a tmp device\n");
> > > + return;
> > > + }
> > > + INIT_LIST_HEAD(&device->pnp.ids);
> > > +
> > > + /* obtain device type */
> > > + ret = acpi_bus_type_and_status(handle, &type, &sta);
> > > + if (ret) {
> > > + pr_err(PREFIX "Failed to get device type\n");
> > > + goto out;
> > > + }
> > > +
> > > + /* setup this temporary device object */
> > > + device->device_type = type;
> > > + device->handle = handle;
> > > + device->parent = acpi_bus_get_parent(handle);
> > > + device->dev.bus = &acpi_bus_type;
> > > + device->driver = NULL;
> > > + STRUCT_TO_INT(device->status) = sta;
> > > + device->status.present = 1;
> > > +
> > > + /* set HID to this device object */
> > > + acpi_device_set_id(device);
> > > +
> > > + /* set args */
> > > + args.device = device;
> > > + args.handle = handle;
> > > + args.event = event;
> > > + args.data = data;
> > > +
> > > + /* call a matched driver's notify handler */
> > > + (void) bus_for_each_drv(device->dev.bus, NULL,
> > > + &args, acpi_match_drv_notify);
> >
> > Excuse me? What makes you think I would accept anything like this?
>
> Sorry, I admit that allocating a temporary acpi_device object is a hack
> since acpi_device_set_id() requires it. I tried to change
> acpi_device_set_id(), but it needed more changes than I expected. I can
> try to clean this up, if the overall design still makes sense.
No, it doesn't. It is not correct to look for a random driver that happens to
match your handle from within a notification handler and call a notify method
from it. It's just bad design and please don't do that.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Wed, 2012-11-28 at 01:29 +0100, Rafael J. Wysocki wrote:
> On Monday, November 26, 2012 10:44:08 AM Toshi Kani wrote:
> > Hi Rafael,
> >
> > Thanks for reviewing! My comments are in-line.
> >
> > On Sat, 2012-11-24 at 23:01 +0100, Rafael J. Wysocki wrote:
> > > On Thursday, November 08, 2012 01:23:44 PM Toshi Kani wrote:
> > > > Added a new .sys_notify interface, which allows ACPI drivers to
> > > > register their system-level (ex. hotplug) notify handlers through
> > > > their acpi_driver table. This removes redundant ACPI namespace
> > > > walks from ACPI drivers for faster booting.
> > > >
> > > > The global notify handler acpi_bus_notify() is called for all
> > > > system-level ACPI notifications, which then calls an appropriate
> > > > driver's handler if any. ACPI drivers no longer need to register
> > > > or unregister driver's handler to each ACPI device object. It also
> > > > supports dynamic ACPI namespace with LoadTable & Unload opcode
> > > > without any modification in ACPI drivers.
> > > >
> > > > Added a common system notify handler acpi_bus_sys_notify(), which
> > > > allows ACPI drivers to set it to .sys_notify when this function is
> > > > fully implemented.
> > >
> > > I don't really understand this.
> >
> > Currently, acpi_bus_notify() is partially implemented as the common
> > notify handler, but it may not be fully implemented under the current
> > design. When a notify event is sent, ACPICA calls both
> > acpi_bus_notify() and driver's handler registered through
> > acpi_install_notify_handler(). However, a same event cannot be handled
> > by both handlers.
>
> Yes, it can, as long as they don't do conflicting things. :)
True, if the things are defined in such way. :)
> Usually, the event will be discarded by one of them.
>
> > Since acpi_bus_notify() may not know if an event has
> > already been handled by driver's handler, it cannot do anything that may
> > conflict with the driver's handler.
>
> Not really. acpi_bus_notify() is always called first, so it actually
> knows that no one has done anything with that event before. However,
> it doesn't know who else will be called for the same event going
> forward.
Right.
> But I agree, acpi_bus_notify() shouldn't register for the same types of
> events that are handled by drivers individually. And we may not need
> acpi_bus_notify() at all as far as I can say at the moment.
Yes, if we can replace blocking_notifier_call_chain() and
ACPI_DRIVER_ALL_NOTIFY_EVENTS interfaces.
> > Therefore, the partially implemented common handler code in
> > acpi_bus_notify() is moved to a separate function acpi_bus_sys_notify()
> > in this patchset. This function can now be fully implemented as
> > necessary.
>
> Not really, because notifiers that don't use it may be called for the
> same events.
>
> > > > It removes functional conflict between driver's
> > > > notify handler and the global notify handler acpi_bus_notify().
> > > >
> > > > Note that the changes maintain backward compatibility for ACPI
> > > > drivers. Any drivers registered their hotplug handler through the
> > > > existing interfaces, such as acpi_install_notify_handler() and
> > > > register_acpi_bus_notifier(), will continue to work as before.
> > >
> > > I really wouldn't like to add new callbacks to struct acpi_device_ops, because
> > > I'd like that whole thing to go away entirely eventually, along with struct
> > > acpi_driver.
> >
> > acpi_device may need to be changed, but I think ACPI drivers are still
> > necessary to support vendor-unique PNPIDs in an extendable way, which is
> > currently done by adding drivers, such as asus_acpi_driver,
> > cmpc_accel_acpi_driver, eeepc_acpi_driver, acpi_fujitsu_driver,
> > lis3lv02d_driver, etc...
>
> Well, not really. Handling different PNPIDs has nothing to do with ACPI
> drivers. You only need a way for drivers in general to specify the ACPI
> device IDs they can handle. And after som changes that are waiting for the
> v3.8 merge windown you'll be able to add a list of ACPI device IDs to other
> types of drivers too (like platform drivers for one example).
I see. My point is that we need to be able to support different PNPIDs
with drivers. So, that works for me.
> [...]
>
> > > > +
> > > > +/* callback args for acpi_match_drv_notify() */
> > > > +struct acpi_notify_args {
> > > > + struct acpi_device *device;
> > > > + acpi_handle handle;
> > > > + u32 event;
> > > > + void *data;
> > > > +};
> > > > +
> > > > +static int acpi_match_drv_notify(struct device_driver *drv, void *data)
> > > > +{
> > > > + struct acpi_driver *driver = to_acpi_driver(drv);
> > > > + struct acpi_notify_args *args = (struct acpi_notify_args *) data;
> > > > +
> > > > + /* check if this driver matches with the device */
> > > > + if (acpi_match_device_ids(args->device, driver->ids))
> > > > + return 0;
> > > > +
> > > > + /* call the driver's notify handler */
> > > > + acpi_bus_drv_notify(driver, NULL, args->handle,
> > > > + args->event, args->data);
> > > > +
> > > > + return 1;
> > > > +}
> > > > +
> > > > +/**
> > > > + * acpi_lookup_drv_notify: Look up and call driver's notify handler
> > > > + * @handle: ACPI handle of the notified device object
> > > > + * @event: Notify event
> > > > + * @data: Context
> > > > + *
> > > > + * Look up and call the associated driver's notify handler for the ACPI
> > > > + * device object by walking through the list of ACPI drivers.
> > >
> > > What exactly is the purpose of this?
> >
> > For hot-add, an acpi_device object is not created for the notified
> > object yet. Therefore, acpi_bus_notify() calls this function to find an
> > associated driver for the device. It walks thru the ACPI driver list to
> > find a matching driver.
>
> This is just broken and not only because you're creating a struct acpi_device
> object on the fly in there.
>
> > > > + */
> > > > +void acpi_lookup_drv_notify(acpi_handle handle, u32 event, void *data)
> > > > +{
> > > > + struct acpi_notify_args args;
> > > > + struct acpi_device *device;
> > > > + unsigned long long sta;
> > > > + int type;
> > > > + int ret;
> > > > +
> > > > + /* allocate a temporary device object */
> > > > + device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
> > > > + if (!device) {
> > > > + pr_err(PREFIX "No memory to allocate a tmp device\n");
> > > > + return;
> > > > + }
> > > > + INIT_LIST_HEAD(&device->pnp.ids);
> > > > +
> > > > + /* obtain device type */
> > > > + ret = acpi_bus_type_and_status(handle, &type, &sta);
> > > > + if (ret) {
> > > > + pr_err(PREFIX "Failed to get device type\n");
> > > > + goto out;
> > > > + }
> > > > +
> > > > + /* setup this temporary device object */
> > > > + device->device_type = type;
> > > > + device->handle = handle;
> > > > + device->parent = acpi_bus_get_parent(handle);
> > > > + device->dev.bus = &acpi_bus_type;
> > > > + device->driver = NULL;
> > > > + STRUCT_TO_INT(device->status) = sta;
> > > > + device->status.present = 1;
> > > > +
> > > > + /* set HID to this device object */
> > > > + acpi_device_set_id(device);
> > > > +
> > > > + /* set args */
> > > > + args.device = device;
> > > > + args.handle = handle;
> > > > + args.event = event;
> > > > + args.data = data;
> > > > +
> > > > + /* call a matched driver's notify handler */
> > > > + (void) bus_for_each_drv(device->dev.bus, NULL,
> > > > + &args, acpi_match_drv_notify);
> > >
> > > Excuse me? What makes you think I would accept anything like this?
> >
> > Sorry, I admit that allocating a temporary acpi_device object is a hack
> > since acpi_device_set_id() requires it. I tried to change
> > acpi_device_set_id(), but it needed more changes than I expected. I can
> > try to clean this up, if the overall design still makes sense.
>
> No, it doesn't. It is not correct to look for a random driver that happens to
> match your handle from within a notification handler and call a notify method
> from it. It's just bad design and please don't do that.
I got your point. The code is actually consistent with how we bind an
ACPI driver with device_attach(), which goes thru the ACPI driver list
to find a matching driver, but I agree that duplicating the code logic
here is not good.
Thanks,
-Toshi
> > > > > By using acpi_install_notify_handler(), each driver needs to walk
> > > > > through the entire ACPI namespace to find its associated ACPI devices
> > > > > and call it to register one by one. I think this is more work for
> > > > > non-ACPI drivers than defining acpi_driver.
> > > >
> > > > I'm not really sure what you mean. The drivers in question already know
> > > > what the relevant ACPI device nodes are (because they need them anyway
> > > > for other purposes), so they don't need to look for them specifically and
> > > > acpi_install_notify_handler() doesn't do any namespace walking. So what
> > > > you said above simply doesn't make sense from this viewpoint.
> > >
> > > Yes, if drivers already know the relevant ACPI devices, then walking the
> > > ACPI namespace is not necessary. I was referring the case like
> > > processor_driver.c, acpi_memhotplug.c, and container.c in my statement.
> >
> > BTW, when an ACPI device is marked as non-present, which is the case
> > before hot-add, we do not create an acpi_device object and therefore do
> > not bind it with a driver. This is why these drivers walk the ACPI
> > namespace and install their notify handlers regardless of device status.
>
> So maybe we should create struct acpi_device objects in that case too?
I think it has some challenge as well. We bind an ACPI driver with
device_register(), which calls device_add()-> kobject_add(). So, all
non-present ACPI device objects will show up in sysfs, unless we can
change the core. This will change user interface. There can be quite
many non-present devices in ACPI namespace depending on FW
implementation.
Thanks,
-Toshi
On Wednesday, November 28, 2012 09:54:43 AM Toshi Kani wrote:
> > > > > > By using acpi_install_notify_handler(), each driver needs to walk
> > > > > > through the entire ACPI namespace to find its associated ACPI devices
> > > > > > and call it to register one by one. I think this is more work for
> > > > > > non-ACPI drivers than defining acpi_driver.
> > > > >
> > > > > I'm not really sure what you mean. The drivers in question already know
> > > > > what the relevant ACPI device nodes are (because they need them anyway
> > > > > for other purposes), so they don't need to look for them specifically and
> > > > > acpi_install_notify_handler() doesn't do any namespace walking. So what
> > > > > you said above simply doesn't make sense from this viewpoint.
> > > >
> > > > Yes, if drivers already know the relevant ACPI devices, then walking the
> > > > ACPI namespace is not necessary. I was referring the case like
> > > > processor_driver.c, acpi_memhotplug.c, and container.c in my statement.
> > >
> > > BTW, when an ACPI device is marked as non-present, which is the case
> > > before hot-add, we do not create an acpi_device object and therefore do
> > > not bind it with a driver. This is why these drivers walk the ACPI
> > > namespace and install their notify handlers regardless of device status.
> >
> > So maybe we should create struct acpi_device objects in that case too?
>
> I think it has some challenge as well. We bind an ACPI driver with
> device_register(), which calls device_add()-> kobject_add(). So, all
> non-present ACPI device objects will show up in sysfs, unless we can
> change the core. This will change user interface. There can be quite
> many non-present devices in ACPI namespace depending on FW
> implementation.
If additional devices appear in sysfs, that's not a problem. If there
were fewer of them, that would be a real one. :-)
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Wed, 2012-11-28 at 19:28 +0100, Rafael J. Wysocki wrote:
> On Wednesday, November 28, 2012 09:54:43 AM Toshi Kani wrote:
> > > > > > > By using acpi_install_notify_handler(), each driver needs to walk
> > > > > > > through the entire ACPI namespace to find its associated ACPI devices
> > > > > > > and call it to register one by one. I think this is more work for
> > > > > > > non-ACPI drivers than defining acpi_driver.
> > > > > >
> > > > > > I'm not really sure what you mean. The drivers in question already know
> > > > > > what the relevant ACPI device nodes are (because they need them anyway
> > > > > > for other purposes), so they don't need to look for them specifically and
> > > > > > acpi_install_notify_handler() doesn't do any namespace walking. So what
> > > > > > you said above simply doesn't make sense from this viewpoint.
> > > > >
> > > > > Yes, if drivers already know the relevant ACPI devices, then walking the
> > > > > ACPI namespace is not necessary. I was referring the case like
> > > > > processor_driver.c, acpi_memhotplug.c, and container.c in my statement.
> > > >
> > > > BTW, when an ACPI device is marked as non-present, which is the case
> > > > before hot-add, we do not create an acpi_device object and therefore do
> > > > not bind it with a driver. This is why these drivers walk the ACPI
> > > > namespace and install their notify handlers regardless of device status.
> > >
> > > So maybe we should create struct acpi_device objects in that case too?
> >
> > I think it has some challenge as well. We bind an ACPI driver with
> > device_register(), which calls device_add()-> kobject_add(). So, all
> > non-present ACPI device objects will show up in sysfs, unless we can
> > change the core. This will change user interface. There can be quite
> > many non-present devices in ACPI namespace depending on FW
> > implementation.
>
> If additional devices appear in sysfs, that's not a problem. If there
> were fewer of them, that would be a real one. :-)
I see. I guess this means that once we expose all non-present devices
in sysfs, we cannot go back to the current way. So, we need to be very
careful. Anyway, this model requires separate handling for static ACPI
[1] and dynamic ACPI [2], which may make the state model complicated.
1. Static ACPI - No creation / deletion of acpi_device at hot-plug.
2. Dynamic ACPI - Create acpi_device at hot-add, delete at hot-remove.
Thanks,
-Toshi
[1] ACPI namespace is static and contains the maximum possible config.
[2] ACPI namespace is dynamic. SSDT is loaded at hot-add, and unloaded
at hot-remove.
On Wednesday, November 28, 2012 01:31:39 PM Toshi Kani wrote:
> On Wed, 2012-11-28 at 19:28 +0100, Rafael J. Wysocki wrote:
> > On Wednesday, November 28, 2012 09:54:43 AM Toshi Kani wrote:
> > > > > > > > By using acpi_install_notify_handler(), each driver needs to walk
> > > > > > > > through the entire ACPI namespace to find its associated ACPI devices
> > > > > > > > and call it to register one by one. I think this is more work for
> > > > > > > > non-ACPI drivers than defining acpi_driver.
> > > > > > >
> > > > > > > I'm not really sure what you mean. The drivers in question already know
> > > > > > > what the relevant ACPI device nodes are (because they need them anyway
> > > > > > > for other purposes), so they don't need to look for them specifically and
> > > > > > > acpi_install_notify_handler() doesn't do any namespace walking. So what
> > > > > > > you said above simply doesn't make sense from this viewpoint.
> > > > > >
> > > > > > Yes, if drivers already know the relevant ACPI devices, then walking the
> > > > > > ACPI namespace is not necessary. I was referring the case like
> > > > > > processor_driver.c, acpi_memhotplug.c, and container.c in my statement.
> > > > >
> > > > > BTW, when an ACPI device is marked as non-present, which is the case
> > > > > before hot-add, we do not create an acpi_device object and therefore do
> > > > > not bind it with a driver. This is why these drivers walk the ACPI
> > > > > namespace and install their notify handlers regardless of device status.
> > > >
> > > > So maybe we should create struct acpi_device objects in that case too?
> > >
> > > I think it has some challenge as well. We bind an ACPI driver with
> > > device_register(), which calls device_add()-> kobject_add(). So, all
> > > non-present ACPI device objects will show up in sysfs, unless we can
> > > change the core. This will change user interface. There can be quite
> > > many non-present devices in ACPI namespace depending on FW
> > > implementation.
> >
> > If additional devices appear in sysfs, that's not a problem. If there
> > were fewer of them, that would be a real one. :-)
>
> I see. I guess this means that once we expose all non-present devices
> in sysfs, we cannot go back to the current way. So, we need to be very
> careful. Anyway, this model requires separate handling for static ACPI
> [1] and dynamic ACPI [2], which may make the state model complicated.
>
> 1. Static ACPI - No creation / deletion of acpi_device at hot-plug.
> 2. Dynamic ACPI - Create acpi_device at hot-add, delete at hot-remove.
Sure. The complication here is that we'll need to handle the removal of
a bunch of struct acpi_device objects when a whole table goes away.
However, first, we don't seem to handle table unloading now. At least
acpi_unload_parent_table() is not called from anywhere as far as I can
say. Second, we'll need to handle the removal of struct acpi_device objects
_anyway_ on table unload, this way or another.
> [1] ACPI namespace is static and contains the maximum possible config.
> [2] ACPI namespace is dynamic. SSDT is loaded at hot-add, and unloaded
> at hot-remove.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Wed, 2012-11-28 at 22:09 +0100, Rafael J. Wysocki wrote:
> On Wednesday, November 28, 2012 01:31:39 PM Toshi Kani wrote:
> > On Wed, 2012-11-28 at 19:28 +0100, Rafael J. Wysocki wrote:
> > > On Wednesday, November 28, 2012 09:54:43 AM Toshi Kani wrote:
> > > > > > > > > By using acpi_install_notify_handler(), each driver needs to walk
> > > > > > > > > through the entire ACPI namespace to find its associated ACPI devices
> > > > > > > > > and call it to register one by one. I think this is more work for
> > > > > > > > > non-ACPI drivers than defining acpi_driver.
> > > > > > > >
> > > > > > > > I'm not really sure what you mean. The drivers in question already know
> > > > > > > > what the relevant ACPI device nodes are (because they need them anyway
> > > > > > > > for other purposes), so they don't need to look for them specifically and
> > > > > > > > acpi_install_notify_handler() doesn't do any namespace walking. So what
> > > > > > > > you said above simply doesn't make sense from this viewpoint.
> > > > > > >
> > > > > > > Yes, if drivers already know the relevant ACPI devices, then walking the
> > > > > > > ACPI namespace is not necessary. I was referring the case like
> > > > > > > processor_driver.c, acpi_memhotplug.c, and container.c in my statement.
> > > > > >
> > > > > > BTW, when an ACPI device is marked as non-present, which is the case
> > > > > > before hot-add, we do not create an acpi_device object and therefore do
> > > > > > not bind it with a driver. This is why these drivers walk the ACPI
> > > > > > namespace and install their notify handlers regardless of device status.
> > > > >
> > > > > So maybe we should create struct acpi_device objects in that case too?
> > > >
> > > > I think it has some challenge as well. We bind an ACPI driver with
> > > > device_register(), which calls device_add()-> kobject_add(). So, all
> > > > non-present ACPI device objects will show up in sysfs, unless we can
> > > > change the core. This will change user interface. There can be quite
> > > > many non-present devices in ACPI namespace depending on FW
> > > > implementation.
> > >
> > > If additional devices appear in sysfs, that's not a problem. If there
> > > were fewer of them, that would be a real one. :-)
> >
> > I see. I guess this means that once we expose all non-present devices
> > in sysfs, we cannot go back to the current way. So, we need to be very
> > careful. Anyway, this model requires separate handling for static ACPI
> > [1] and dynamic ACPI [2], which may make the state model complicated.
> >
> > 1. Static ACPI - No creation / deletion of acpi_device at hot-plug.
> > 2. Dynamic ACPI - Create acpi_device at hot-add, delete at hot-remove.
>
> Sure. The complication here is that we'll need to handle the removal of
> a bunch of struct acpi_device objects when a whole table goes away.
>
> However, first, we don't seem to handle table unloading now. At least
> acpi_unload_parent_table() is not called from anywhere as far as I can
> say. Second, we'll need to handle the removal of struct acpi_device objects
> _anyway_ on table unload, this way or another.
AML is the one that requests loading/unloading of SSDT for dynamic ACPI.
In hot-add, _Lxx method executes AML_LOAD_OP or AML_LOAD_TABLE_OP to
load SSDT and then sends a notification to the OS. In hot-remove, _EJ0
method executes AML_UNLOAD_OP to unload SSDT. Of course, ACPICA does
the actual work on behalf of AML. But this is not visible to ACPI core
or drivers, unless it checks ACPI namespace to see if any device objects
disappeared after _EJ0.
Thanks,
-Toshi
> > [1] ACPI namespace is static and contains the maximum possible config.
> > [2] ACPI namespace is dynamic. SSDT is loaded at hot-add, and unloaded
> > at hot-remove.
>
> Thanks,
> Rafael
>
>
On Wednesday, November 28, 2012 02:23:23 PM Toshi Kani wrote:
> On Wed, 2012-11-28 at 22:09 +0100, Rafael J. Wysocki wrote:
> > On Wednesday, November 28, 2012 01:31:39 PM Toshi Kani wrote:
> > > On Wed, 2012-11-28 at 19:28 +0100, Rafael J. Wysocki wrote:
> > > > On Wednesday, November 28, 2012 09:54:43 AM Toshi Kani wrote:
> > > > > > > > > > By using acpi_install_notify_handler(), each driver needs to walk
> > > > > > > > > > through the entire ACPI namespace to find its associated ACPI devices
> > > > > > > > > > and call it to register one by one. I think this is more work for
> > > > > > > > > > non-ACPI drivers than defining acpi_driver.
> > > > > > > > >
> > > > > > > > > I'm not really sure what you mean. The drivers in question already know
> > > > > > > > > what the relevant ACPI device nodes are (because they need them anyway
> > > > > > > > > for other purposes), so they don't need to look for them specifically and
> > > > > > > > > acpi_install_notify_handler() doesn't do any namespace walking. So what
> > > > > > > > > you said above simply doesn't make sense from this viewpoint.
> > > > > > > >
> > > > > > > > Yes, if drivers already know the relevant ACPI devices, then walking the
> > > > > > > > ACPI namespace is not necessary. I was referring the case like
> > > > > > > > processor_driver.c, acpi_memhotplug.c, and container.c in my statement.
> > > > > > >
> > > > > > > BTW, when an ACPI device is marked as non-present, which is the case
> > > > > > > before hot-add, we do not create an acpi_device object and therefore do
> > > > > > > not bind it with a driver. This is why these drivers walk the ACPI
> > > > > > > namespace and install their notify handlers regardless of device status.
> > > > > >
> > > > > > So maybe we should create struct acpi_device objects in that case too?
> > > > >
> > > > > I think it has some challenge as well. We bind an ACPI driver with
> > > > > device_register(), which calls device_add()-> kobject_add(). So, all
> > > > > non-present ACPI device objects will show up in sysfs, unless we can
> > > > > change the core. This will change user interface. There can be quite
> > > > > many non-present devices in ACPI namespace depending on FW
> > > > > implementation.
> > > >
> > > > If additional devices appear in sysfs, that's not a problem. If there
> > > > were fewer of them, that would be a real one. :-)
> > >
> > > I see. I guess this means that once we expose all non-present devices
> > > in sysfs, we cannot go back to the current way. So, we need to be very
> > > careful. Anyway, this model requires separate handling for static ACPI
> > > [1] and dynamic ACPI [2], which may make the state model complicated.
> > >
> > > 1. Static ACPI - No creation / deletion of acpi_device at hot-plug.
> > > 2. Dynamic ACPI - Create acpi_device at hot-add, delete at hot-remove.
> >
> > Sure. The complication here is that we'll need to handle the removal of
> > a bunch of struct acpi_device objects when a whole table goes away.
> >
> > However, first, we don't seem to handle table unloading now. At least
> > acpi_unload_parent_table() is not called from anywhere as far as I can
> > say. Second, we'll need to handle the removal of struct acpi_device objects
> > _anyway_ on table unload, this way or another.
>
> AML is the one that requests loading/unloading of SSDT for dynamic ACPI.
> In hot-add, _Lxx method executes AML_LOAD_OP or AML_LOAD_TABLE_OP to
> load SSDT and then sends a notification to the OS. In hot-remove, _EJ0
> method executes AML_UNLOAD_OP to unload SSDT. Of course, ACPICA does
> the actual work on behalf of AML. But this is not visible to ACPI core
> or drivers, unless it checks ACPI namespace to see if any device objects
> disappeared after _EJ0.
Oh, we have a handler for that event, but we don't really use it. :-)
And I wonder what happens to the struct acpi_device objects associated with
the ACPI handles in the table being unloaded?
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
> > > > > > I think it has some challenge as well. We bind an ACPI driver with
> > > > > > device_register(), which calls device_add()-> kobject_add(). So, all
> > > > > > non-present ACPI device objects will show up in sysfs, unless we can
> > > > > > change the core. This will change user interface. There can be quite
> > > > > > many non-present devices in ACPI namespace depending on FW
> > > > > > implementation.
> > > > >
> > > > > If additional devices appear in sysfs, that's not a problem. If there
> > > > > were fewer of them, that would be a real one. :-)
> > > >
> > > > I see. I guess this means that once we expose all non-present devices
> > > > in sysfs, we cannot go back to the current way. So, we need to be very
> > > > careful. Anyway, this model requires separate handling for static ACPI
> > > > [1] and dynamic ACPI [2], which may make the state model complicated.
> > > >
> > > > 1. Static ACPI - No creation / deletion of acpi_device at hot-plug.
> > > > 2. Dynamic ACPI - Create acpi_device at hot-add, delete at hot-remove.
> > >
> > > Sure. The complication here is that we'll need to handle the removal of
> > > a bunch of struct acpi_device objects when a whole table goes away.
> > >
> > > However, first, we don't seem to handle table unloading now. At least
> > > acpi_unload_parent_table() is not called from anywhere as far as I can
> > > say. Second, we'll need to handle the removal of struct acpi_device objects
> > > _anyway_ on table unload, this way or another.
> >
> > AML is the one that requests loading/unloading of SSDT for dynamic ACPI.
> > In hot-add, _Lxx method executes AML_LOAD_OP or AML_LOAD_TABLE_OP to
> > load SSDT and then sends a notification to the OS. In hot-remove, _EJ0
> > method executes AML_UNLOAD_OP to unload SSDT. Of course, ACPICA does
> > the actual work on behalf of AML. But this is not visible to ACPI core
> > or drivers, unless it checks ACPI namespace to see if any device objects
> > disappeared after _EJ0.
>
> Oh, we have a handler for that event, but we don't really use it. :-)
>
> And I wonder what happens to the struct acpi_device objects associated with
> the ACPI handles in the table being unloaded?
If we use an ACPI handle that does not associate with a device object,
ACPICA returns AE_NOT_FOUND or AE_NOT_EXIST. But, we should remove
acpi_device that does not have its associated ACPI object. Currently,
we create acpi_device on hot-add and remove it on hot-remove, so it is
OK. But if we start creating acpi_device objects for non-present
devices, we need to worry about if acpi_device objects indeed have their
associated ACPI objects. That's the complication I mentioned above.
Thanks,
-Toshi
On Wednesday, November 28, 2012 03:33:27 PM Toshi Kani wrote:
> > > > > > > I think it has some challenge as well. We bind an ACPI driver with
> > > > > > > device_register(), which calls device_add()-> kobject_add(). So, all
> > > > > > > non-present ACPI device objects will show up in sysfs, unless we can
> > > > > > > change the core. This will change user interface. There can be quite
> > > > > > > many non-present devices in ACPI namespace depending on FW
> > > > > > > implementation.
> > > > > >
> > > > > > If additional devices appear in sysfs, that's not a problem. If there
> > > > > > were fewer of them, that would be a real one. :-)
> > > > >
> > > > > I see. I guess this means that once we expose all non-present devices
> > > > > in sysfs, we cannot go back to the current way. So, we need to be very
> > > > > careful. Anyway, this model requires separate handling for static ACPI
> > > > > [1] and dynamic ACPI [2], which may make the state model complicated.
> > > > >
> > > > > 1. Static ACPI - No creation / deletion of acpi_device at hot-plug.
> > > > > 2. Dynamic ACPI - Create acpi_device at hot-add, delete at hot-remove.
> > > >
> > > > Sure. The complication here is that we'll need to handle the removal of
> > > > a bunch of struct acpi_device objects when a whole table goes away.
> > > >
> > > > However, first, we don't seem to handle table unloading now. At least
> > > > acpi_unload_parent_table() is not called from anywhere as far as I can
> > > > say. Second, we'll need to handle the removal of struct acpi_device objects
> > > > _anyway_ on table unload, this way or another.
> > >
> > > AML is the one that requests loading/unloading of SSDT for dynamic ACPI.
> > > In hot-add, _Lxx method executes AML_LOAD_OP or AML_LOAD_TABLE_OP to
> > > load SSDT and then sends a notification to the OS. In hot-remove, _EJ0
> > > method executes AML_UNLOAD_OP to unload SSDT. Of course, ACPICA does
> > > the actual work on behalf of AML. But this is not visible to ACPI core
> > > or drivers, unless it checks ACPI namespace to see if any device objects
> > > disappeared after _EJ0.
> >
> > Oh, we have a handler for that event, but we don't really use it. :-)
> >
> > And I wonder what happens to the struct acpi_device objects associated with
> > the ACPI handles in the table being unloaded?
>
> If we use an ACPI handle that does not associate with a device object,
> ACPICA returns AE_NOT_FOUND or AE_NOT_EXIST. But, we should remove
> acpi_device that does not have its associated ACPI object. Currently,
> we create acpi_device on hot-add and remove it on hot-remove, so it is
> OK. But if we start creating acpi_device objects for non-present
> devices, we need to worry about if acpi_device objects indeed have their
> associated ACPI objects. That's the complication I mentioned above.
My question was about something different. Is it actually guaranteed
that all struct device objects associated with the given ACPI table will
be removed before that table is unloaded?
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Wed, 2012-11-28 at 23:49 +0100, Rafael J. Wysocki wrote:
> On Wednesday, November 28, 2012 03:33:27 PM Toshi Kani wrote:
> > > > > > > > I think it has some challenge as well. We bind an ACPI driver with
> > > > > > > > device_register(), which calls device_add()-> kobject_add(). So, all
> > > > > > > > non-present ACPI device objects will show up in sysfs, unless we can
> > > > > > > > change the core. This will change user interface. There can be quite
> > > > > > > > many non-present devices in ACPI namespace depending on FW
> > > > > > > > implementation.
> > > > > > >
> > > > > > > If additional devices appear in sysfs, that's not a problem. If there
> > > > > > > were fewer of them, that would be a real one. :-)
> > > > > >
> > > > > > I see. I guess this means that once we expose all non-present devices
> > > > > > in sysfs, we cannot go back to the current way. So, we need to be very
> > > > > > careful. Anyway, this model requires separate handling for static ACPI
> > > > > > [1] and dynamic ACPI [2], which may make the state model complicated.
> > > > > >
> > > > > > 1. Static ACPI - No creation / deletion of acpi_device at hot-plug.
> > > > > > 2. Dynamic ACPI - Create acpi_device at hot-add, delete at hot-remove.
> > > > >
> > > > > Sure. The complication here is that we'll need to handle the removal of
> > > > > a bunch of struct acpi_device objects when a whole table goes away.
> > > > >
> > > > > However, first, we don't seem to handle table unloading now. At least
> > > > > acpi_unload_parent_table() is not called from anywhere as far as I can
> > > > > say. Second, we'll need to handle the removal of struct acpi_device objects
> > > > > _anyway_ on table unload, this way or another.
> > > >
> > > > AML is the one that requests loading/unloading of SSDT for dynamic ACPI.
> > > > In hot-add, _Lxx method executes AML_LOAD_OP or AML_LOAD_TABLE_OP to
> > > > load SSDT and then sends a notification to the OS. In hot-remove, _EJ0
> > > > method executes AML_UNLOAD_OP to unload SSDT. Of course, ACPICA does
> > > > the actual work on behalf of AML. But this is not visible to ACPI core
> > > > or drivers, unless it checks ACPI namespace to see if any device objects
> > > > disappeared after _EJ0.
> > >
> > > Oh, we have a handler for that event, but we don't really use it. :-)
> > >
> > > And I wonder what happens to the struct acpi_device objects associated with
> > > the ACPI handles in the table being unloaded?
> >
> > If we use an ACPI handle that does not associate with a device object,
> > ACPICA returns AE_NOT_FOUND or AE_NOT_EXIST. But, we should remove
> > acpi_device that does not have its associated ACPI object. Currently,
> > we create acpi_device on hot-add and remove it on hot-remove, so it is
> > OK. But if we start creating acpi_device objects for non-present
> > devices, we need to worry about if acpi_device objects indeed have their
> > associated ACPI objects. That's the complication I mentioned above.
>
> My question was about something different. Is it actually guaranteed
> that all struct device objects associated with the given ACPI table will
> be removed before that table is unloaded?
Yes, it is guaranteed currently. acpi_bus_hot_remove_device() calls
acpi_bus_trim(), which deletes all acpi_device objects under of the
notified object. It then calls _EJ0 of the notified object, which
unloads all ACPI objects under the notified object.
Thanks,
-Toshi