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

From: Rafael J. Wysocki
Date: Sun Jun 30 2013 - 10:09:27 EST


On Saturday, June 29, 2013 09:12:59 AM Yinghai Lu wrote:
> On Sat, Jun 29, 2013 at 4:16 AM, Rafael J. Wysocki <rjw@xxxxxxx> wrote:
> > On Friday, June 28, 2013 04:34:21 PM Yinghai Lu wrote:
> >> On Fri, Jun 28, 2013 at 3: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.
> >>
> >> original change to use register_acpi_bus_notifier, have two assumption
> >> 1. two dock_station will have same handle.
> >
> > Well, that would mean that dock_add() might be called twice for the same handle
> > and I don't see how that's possible.
> >
> > Moreover, even if that were possible, the loop in acpi_dock_notifier_call()
> > would break after finding the *first* matching handle anyway, so
> > acpi_dock_deferred_cb() wouldn't be called for the second dock station with
> > the same handle, if there were two.
>
> related commit:
> commit 6bd00a61ab63d4ceb635ae0316353c11c900b8d8
> Author: Shaohua Li <shaohua.li@xxxxxxxxx>
> Date: Thu Aug 28 10:04:29 2008 +0800
>
> ACPI: introduce notifier change to avoid duplicates
>
> The battery driver already registers notification handler.
> To avoid registering notification handler again,
> introduce a notifier chain in global system notifier handler
> and use it in dock driver.
>
> so it is not two dock station have same handle. it is battery acpi driver.
>
> but notitifer installing is changed
>
> commit d94066910943837558d2a461c6766da981260bf0
> Author: Bjorn Helgaas <bjorn.helgaas@xxxxxx>
> Date: Thu Apr 30 09:35:47 2009 -0600
>
> ACPI: battery: use .notify method instead of installing handler directly
>
> This patch adds a .notify() method. The presence of .notify() causes
> Linux/ACPI to manage event handlers and notify handlers on our behalf,
> so we don't have to install and remove them ourselves.
>
> This driver apparently relies on seeing ALL notify events, not just
> device-specific ones (because it used ACPI_ALL_NOTIFY). We use the
> ACPI_DRIVER_ALL_NOTIFY_EVENTS driver flag to request all events.
>
> Signed-off-by: Bjorn Helgaas <bjorn.helgaas@xxxxxx>
> CC: Alexey Starikovskiy <alexey.y.starikovskiy@xxxxxxxxxxxxxxx>
> Signed-off-by: Len Brown <len.brown@xxxxxxxxx>
>
>
> >
> >> 2. acpi subsystem: non root acpi device only can have one system
> >> notifier installed.
> >
> > No, that limitation is long gone. We removed it when we were working on ACPI
> > wakeup support for runtime PM.
>
> looks like i misread that...
>
> * NOTES: The Root namespace object may have only one handler for each
> * type of notify (System/Device). Device/Thermal/Processor objects
> * may have one device notify handler, and multiple system notify
> * handlers.
>
> device could have multiple system notify handlers.
>
> So
>
> Acked-by: Yinghai Lu <yinghai@xxxxxxxxxx>

Thanks (and for the other ACKs too)!

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/