[Update,3/3] ACPI / dock / PCI: Synchronous handling of dock events for PCI devices

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

On Sunday, June 23, 2013 01:29:19 PM Yinghai Lu wrote:
> On Sun, Jun 23, 2013 at 12:57 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> > On Sun, Jun 23, 2013 at 8:54 AM, Jiang Liu <liuj97@gmail.com> wrote:
> >> On 06/23/2013 05:25 AM, Rafael J. Wysocki wrote:
> >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>>
> >>> The interactions between the ACPI dock driver and the ACPI-based PCI
> >>> hotplug (acpiphp) are currently problematic because of ordering
> >>> issues during hot-remove operations.
> >>>
> >>> First of all, the current ACPI glue code expects that physical
> >>> devices will always be deleted before deleting the companion ACPI
> >>> device objects.  Otherwise, acpi_unbind_one() will fail with a
> >>> warning message printed to the kernel log, for example:
> >>>
> >>> [  185.026073] usb usb5: Oops, 'acpi_handle' corrupt
> >>> [  185.035150] pci 0000:1b:00.0: Oops, 'acpi_handle' corrupt
> >>> [  185.035515] pci 0000:18:02.0: Oops, 'acpi_handle' corrupt
> >>> [  180.013656]  port1: Oops, 'acpi_handle' corrupt
> >>>
> >> [...]
> >>> @@ -597,15 +654,11 @@ register_hotplug_dock_device(acpi_handle
> >>>                * ops
> >>>                */
> >>>               dd = find_dock_dependent_device(dock_station, handle);
> >>> -             if (dd) {
> >>> -                     dd->ops = ops;
> >>> -                     dd->context = context;
> >>> -                     dock_add_hotplug_device(dock_station, dd);
> >>> -                     ret = 0;
> >>> -             }
> >>> +             if (dd)
> >>> +                     return dock_init_hotplug(dd, ops, context,
> >>> +                                              init, release);
> >> Hi Rafael,
> >>         Seems not an equivalent change. According to the comment just above the
> >> code, we shouldn't return but continue here.
> >> /*
> >>  * An ATA bay can be in a dock and itself can be ejected
> >>  * separately, so there are two 'dock stations' which need the
> >>  * ops
> >>  */
> >
> > two dock stations:
> > Do you mean two dock station has same handle?
> >
> > dock_add should add correctly flags for IS_DOCK and IS_ATA.
> > if one handle has _DCK and _GTF etc.
> >
> > or do you mean there are two dependent devices with same handle?
> > like one is for acpiphp slot and one is for ATA?
> 
> related commit:
> commit 61b836958371c717d1e6d4fea1d2c512969ad20b
> Author: Shaohua Li <shaohua.li@intel.com>
> Date:   Thu Aug 28 10:07:14 2008 +0800
> 
>     dock: fix for ATA bay in a dock station
> 
>     an ATA bay can be in a dock and itself can be ejected separately.
>     This patch handles such eject bay. Found by Holger.
> 
>     Signed-off-by: Shaohua Li <shaohua.li@intel.com>
>     Signed-off-by: Len Brown <len.brown@intel.com>
> @@ -618,16 +619,21 @@ register_hotplug_dock_device(acpi_handle handle, struct ac
> pi_dock_ops *ops,
>          * this would include the dock station itself
>          */
>         list_for_each_entry(dock_station, &dock_stations, sibiling) {
> +               /*
> +                * An ATA bay can be in a dock and itself can be ejected
> +                * seperately, so there are two 'dock stations' which need the
> +                * ops
> +                */
>                 dd = find_dock_dependent_device(dock_station, handle);
>                 if (dd) {
>                         dd->ops = ops;
>                         dd->context = context;
>                         dock_add_hotplug_device(dock_station, dd);
> -                       return 0;
> +                       ret = 0;
>                 }
>         }
> 
> -       return -EINVAL;
> +       return ret;
>  }
> 
> so two doc station with different handle.
> 
> and dependent devices in both...
> 
> looks like Rafael's change can not handle this case anymore.

Ah, I overlooked the fact that each dock station is on its own dependent_list
and can also be on another dock station's dependent_list.  I'm not sure if that
makes sense, but let's not break the backwards compatibility here.

Updated patch is appended.

Thanks,
Rafael

---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: ACPI / dock / PCI: Synchronous handling of dock events for PCI devices

The interactions between the ACPI dock driver and the ACPI-based PCI
hotplug (acpiphp) are currently problematic because of ordering
issues during hot-remove operations.

First of all, the current ACPI glue code expects that physical
devices will always be deleted before deleting the companion ACPI
device objects.  Otherwise, acpi_unbind_one() will fail with a
warning message printed to the kernel log, for example:

[  185.026073] usb usb5: Oops, 'acpi_handle' corrupt
[  185.035150] pci 0000:1b:00.0: Oops, 'acpi_handle' corrupt
[  185.035515] pci 0000:18:02.0: Oops, 'acpi_handle' corrupt
[  180.013656]  port1: Oops, 'acpi_handle' corrupt

This means, in particular, that struct pci_dev objects have to
be deleted before the struct acpi_device objects they are "glued"
with.

Now, the following happens the during the undocking of an ACPI-based
dock station:
 1) hotplug_dock_devices() invokes registered hotplug callbacks to
    destroy physical devices associated with the ACPI device objects
    depending on the dock station.  It calls dd->ops->handler() for
    each of those device objects.
 2) For PCI devices dd->ops->handler() points to
    handle_hotplug_event_func() that queues up a separate work item
    to execute _handle_hotplug_event_func() for the given device and
    returns immediately.  That work item will be executed later.
 3) hotplug_dock_devices() calls dock_remove_acpi_device() for each
    device depending on the dock station.  This runs acpi_bus_trim()
    for each of them, which causes the underlying ACPI device object
    to be destroyed, but the work items queued up by
    handle_hotplug_event_func() haven't been started yet.
 4) _handle_hotplug_event_func() queued up in step 2) are executed
    and cause the above failure to happen, because the PCI devices
    they handle do not have the companion ACPI device objects any
    more (they have been deleted in step 3).

The possible breakage doesn't end here, though, because
hotplug_dock_devices() may return before at least some of the
_handle_hotplug_event_func() work items spawned by it have a
chance to complete and then undock() will cause _DCK to be
evaluated and that will cause the devices handled by the
_handle_hotplug_event_func() to go away possibly while they are
being accessed.

This means that dd->ops->handler() for PCI devices should not point
to handle_hotplug_event_func().  Instead, it should point to a
function that will do the work of _handle_hotplug_event_func()
synchronously.  For this reason, introduce such a function,
hotplug_event_func(), and modity acpiphp_dock_ops to point to
it as the handler.

Unfortunately, however, this is not sufficient, because if the dock
code were not changed further, hotplug_event_func() would now
deadlock with hotplug_dock_devices() that called it, since it would
run unregister_hotplug_dock_device() which in turn would attempt to
acquire the dock station's hp_lock mutex already acquired by
hotplug_dock_devices().

To resolve that deadlock use the observation that
unregister_hotplug_dock_device() won't need to acquire hp_lock
if PCI bridges the devices on the dock station depend on are
prevented from being removed prematurely while the first loop in
hotplug_dock_devices() is in progress.

To make that possible, introduce a mechanism by which the callers of
register_hotplug_dock_device() can provide "init" and "release"
routines that will be executed, respectively, during the addition
and removal of the physical device object associated with the
given ACPI device handle.  Make acpiphp use two new functions,
acpiphp_dock_init() and acpiphp_dock_release(), respectively,
calling get_bridge() and put_bridge() on the PCI bridge holding the
given device, respectively, for this purpose.

In addition to that, remove the dock station's list of
"hotplug devices" and make the dock code always walk the whole list
of "dependent devices" instead in such a way that the loops in
hotplug_dock_devices() and dock_event() (replacing the loops over
"hotplug devices") will take references to the list entries that
register_hotplug_dock_device() has been called for.  That prevents
the "release" routines associated with those entries from being
called while the given entry is being processed and for PCI
devices this means that their bridges won't be removed (by a
concurrent thread) while hotplug_event_func() handling them is
being executed.

This change is based on two earlier patches from Jiang Liu.

References: https://bugzilla.kernel.org/show_bug.cgi?id=59501
Reported-and-tested-by: Alexander E. Patrakov <patrakov@gmail.com>
Tracked-down-by: Jiang Liu <jiang.liu@huawei.com>
Tested-by: Illya Klymov <xanf@xanf.me>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: 3.9+ <stable@vger.kernel.org>
---
 drivers/acpi/dock.c                |  141 +++++++++++++++++++++++++------------
 drivers/pci/hotplug/acpiphp_glue.c |   46 ++++++++----
 include/acpi/acpi_drivers.h        |    8 +-
 3 files changed, 135 insertions(+), 60 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Update,3/3] ACPI / dock / PCI: Synchronous handling of dock events for PCI devices

Commit Message

Comments

Patch