On Monday, July 01, 2013 02:21:45 PM Bjorn Helgaas wrote:
> On Fri, Jun 28, 2013 at 4:53 PM, Rafael J. Wysocki <rjw@xxxxxxx> wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> >
> > The ACPI dock driver uses register_acpi_bus_notifier() which
> > installs a notifier triggered globally for all system notifications.
> > That first of all is inefficient, because the dock driver is only
> > interested in notifications associated with the devices it handles,
> > but it has to handle all system notifies for all devices. Moreover,
> > it does that even if no docking stations are present in the system
> > (CONFIG_ACPI_DOCK set is sufficient for that to happen). Besides,
> > that is inconvenient, because it requires the driver to do extra work
> > for each notification to find the target dock station object.
> >
> > For these reasons, rework the dock driver to install a notify
> > handler individually for each dock station in the system using
> > acpi_install_notify_handler(). This allows the dock station
> > object to be passed directly to the notify handler and makes it
> > possible to simplify the dock driver quite a bit. It also
> > reduces the overhead related to the handling of all system
> > notifies when CONFIG_ACPI_DOCK is set.
>
> I fully support what you're doing, even though I haven't read it in
> enough detail to actually review it. I'm pretty sure that whatever
> you do, you won't make things worse :)
Well, fair enough. :-)
> It sounds like you are keeping the approach of "look for certain AML
> features to identify a dock, and install notify handlers when you find
> one." I think acpiphp uses a similar approach, and I'm not sure it's
> a good one. The spec is not explicit about how the AML should be
> organized, and AML writers are very creative. I think acpiphp suffers
> because it only works with certain arrangements of _ADR, _EJ0, _RMV,
> etc., and I think that has led to a brittle design and possibly more
> separation between dock and acpiphp than is necessary.
I generally agree that this is not a robust approach, but for now I'm avoiding
making changes that may just go too far.
> I think it would be better if we *always* had a more generic notify
> handler installed and figured out where to route the notification
> based on its type and what services are configured in. The ultimate
> handler might do different things based on what methods are present,
> of course.
>
> I don't know if this rambling makes sense for docks (or even for
> acpiphp), so if it doesn't, feel free to just ignore it :)
Well, ideally, it would be great if we could handle all of the bus check,
device check and eject notifications through something like
acpi_hotplug_notify_cb() and dispatch more specific handling from
there depending on what's represented by the target handle. That's why I
introduced hotplug profiles.
That said it's a long way to that point from where we are now. :-)
Thanks,
Rafael
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > ---
> > drivers/acpi/dock.c | 69 ++++++++++++++++++----------------------------------
> > 1 file changed, 25 insertions(+), 44 deletions(-)
> >
> > Index: linux-pm/drivers/acpi/dock.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/dock.c
> > +++ linux-pm/drivers/acpi/dock.c
> > @@ -718,18 +718,17 @@ static int handle_eject_request(struct d
> >
> > /**
> > * dock_notify - act upon an acpi dock notification
> > - * @handle: the dock station handle
> > + * @ds: dock station
> > * @event: the acpi event
> > - * @data: our driver data struct
> > *
> > * If we are notified to dock, then check to see if the dock is
> > * present and then dock. Notify all drivers of the dock event,
> > * and then hotplug and devices that may need hotplugging.
> > */
> > -static void dock_notify(acpi_handle handle, u32 event, void *data)
> > +static void dock_notify(struct dock_station *ds, u32 event)
> > {
> > - struct dock_station *ds = data;
> > - struct acpi_device *tmp;
> > + acpi_handle handle = ds->handle;
> > + struct acpi_device *ad;
> > int surprise_removal = 0;
> >
> > /*
> > @@ -752,8 +751,7 @@ static void dock_notify(acpi_handle hand
> > switch (event) {
> > case ACPI_NOTIFY_BUS_CHECK:
> > case ACPI_NOTIFY_DEVICE_CHECK:
> > - if (!dock_in_progress(ds) && acpi_bus_get_device(ds->handle,
> > - &tmp)) {
> > + if (!dock_in_progress(ds) && acpi_bus_get_device(handle, &ad)) {
> > begin_dock(ds);
> > dock(ds);
> > if (!dock_present(ds)) {
> > @@ -790,9 +788,8 @@ static void dock_notify(acpi_handle hand
> > }
> >
> > struct dock_data {
> > - acpi_handle handle;
> > - unsigned long event;
> > struct dock_station *ds;
> > + u32 event;
> > };
> >
> > static void acpi_dock_deferred_cb(void *context)
> > @@ -800,52 +797,31 @@ static void acpi_dock_deferred_cb(void *
> > struct dock_data *data = context;
> >
> > acpi_scan_lock_acquire();
> > - dock_notify(data->handle, data->event, data->ds);
> > + dock_notify(data->ds, data->event);
> > acpi_scan_lock_release();
> > kfree(data);
> > }
> >
> > -static int acpi_dock_notifier_call(struct notifier_block *this,
> > - unsigned long event, void *data)
> > +static void dock_notify_handler(acpi_handle handle, u32 event, void *data)
> > {
> > - struct dock_station *dock_station;
> > - acpi_handle handle = data;
> > + struct dock_data *dd;
> >
> > if (event != ACPI_NOTIFY_BUS_CHECK && event != ACPI_NOTIFY_DEVICE_CHECK
> > && event != ACPI_NOTIFY_EJECT_REQUEST)
> > - return 0;
> > -
> > - acpi_scan_lock_acquire();
> > + return;
> >
> > - list_for_each_entry(dock_station, &dock_stations, sibling) {
> > - if (dock_station->handle == handle) {
> > - struct dock_data *dd;
> > - acpi_status status;
> > -
> > - dd = kmalloc(sizeof(*dd), GFP_KERNEL);
> > - if (!dd)
> > - break;
> > -
> > - dd->handle = handle;
> > - dd->event = event;
> > - dd->ds = dock_station;
> > - status = acpi_os_hotplug_execute(acpi_dock_deferred_cb,
> > - dd);
> > - if (ACPI_FAILURE(status))
> > - kfree(dd);
> > -
> > - break;
> > - }
> > + dd = kmalloc(sizeof(*dd), GFP_KERNEL);
> > + if (dd) {
> > + acpi_status status;
> > +
> > + dd->ds = data;
> > + dd->event = event;
> > + status = acpi_os_hotplug_execute(acpi_dock_deferred_cb, dd);
> > + if (ACPI_FAILURE(status))
> > + kfree(dd);
> > }
> > -
> > - acpi_scan_lock_release();
> > - return 0;
> > }
> >
> > -static struct notifier_block dock_acpi_notifier = {
> > - .notifier_call = acpi_dock_notifier_call,
> > -};
> > -
> > /**
> > * find_dock_devices - find devices on the dock station
> > * @handle: the handle of the device we are examining
> > @@ -979,6 +955,7 @@ static int __init dock_add(acpi_handle h
> > int ret, id;
> > struct dock_station ds, *dock_station;
> > struct platform_device *dd;
> > + acpi_status status;
> >
> > id = dock_station_count;
> > memset(&ds, 0, sizeof(ds));
> > @@ -1021,6 +998,11 @@ static int __init dock_add(acpi_handle h
> > if (ret)
> > goto err_rmgroup;
> >
> > + status = acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
> > + dock_notify_handler, dock_station);
> > + if (ACPI_FAILURE(status))
> > + goto err_rmgroup;
> > +
> > dock_station_count++;
> > list_add(&dock_station->sibling, &dock_stations);
> > return 0;
> > @@ -1065,7 +1047,6 @@ int __init acpi_dock_init(void)
> > return 0;
> > }
> >
> > - register_acpi_bus_notifier(&dock_acpi_notifier);
> > pr_info(PREFIX "%s: %d docks/bays found\n",
> > ACPI_DOCK_DRIVER_DESCRIPTION, dock_station_count);
> > return 0;
> >
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
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
