| Message ID | 1352406227-32629-2-git-send-email-toshi.kani@hp.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
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 <toshi.kani@hp.com> > --- > 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
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 <toshi.kani@hp.com> > > --- > > 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
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
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 <toshi.kani@hp.com> > > --- > > 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 > > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > { > > > - 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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 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
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
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
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, 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
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. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, 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
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 > > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, 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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, 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
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 */
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 <toshi.kani@hp.com> --- drivers/acpi/bus.c | 64 ++++++++++++++++++++++++++++---------- drivers/acpi/scan.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++ include/acpi/acpi_bus.h | 6 ++++ 3 files changed, 137 insertions(+), 16 deletions(-)