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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html