Re: [PATCH 1/3] ACPI / dock: Rework the handling of notifications

["Rafael J. Wysocki" <rjw@sisk.pl>]

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@sisk.pl> wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > 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@intel.com>
> > ---
> >  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.