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
