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:

quoted

Hi Rafael,

Thanks for reviewing!  My comments are in-line.

On Sat, 2012-11-24 at 23:01 +0100, Rafael J. Wysocki wrote:

quoted

On Thursday, November 08, 2012 01:23:44 PM Toshi Kani wrote:

quoted

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.

quoted

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.

quoted

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.

quoted

quoted

quoted

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.

[...]

quoted

quoted

quoted

+
+/* 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.

quoted

quoted

quoted

+ */
+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