[PATCH 0/3] ACPI / dock / PCI: Fix problems with dock and PCI hotplug

[PATCH 0/3] ACPI / dock / PCI: Fix problems with dock and PCI hotplug

[Rafael J. Wysocki]

Hi All,

Unfortunately, the Alexander's docking station for Sony VAIO VPCZ23A4R is
basically unusable with v3.10-rc6 and, as it turns out, there are multiple
problems with it.

First, the acpiphp initialization ordering has changed recently,
acpiphp_enumerate_slots() is now always run for the first time during the
initial ACPI namespace scan in acpi_scan_init() which has broken support
for PCI devices on ACPI-based docking stations, because the ACPI dock
subsystem has to be initialized *before* acpiphp_enumerate_slots() is first
run.

Fix that, and it turns out that undocking actually doesn't work correctly
because of some synchronization problems between the dock driver, acpiphp
and the PCI core.  [That part was actually the most difficult to fix.]

Fix that, and you'll find that PCI resources are not allocated correctly
after undocking and re-docking (when initially docked).

The three patches in this series make the Alexander's docking station kind
of usable, so if no one has objections, I'd like to put them on a fast track
to Linus.

[1/3] Initialize ACPI dock subsystem before enumerating the PCI hierarchy
      (that also makes the dock driver non-modular).

[2/3] Make acpiphp use the same rules for allocating PCI resources that are
      used at boot time.

[3/3] Make acpiphp handle dock events synchronously (instead of spawning
      separate work items to handle them).  [The changelog of this patch
      is likely one of the longest I've ever written and it sort of looks
      like a crime story.]

This series is mostly based on the work of Jiang Liu (thanks Gerry!) and
the Alexander's testing that we wouldn't have done much progress without
(thanks Alexander!).

Alexander, I've modified patch [3/3] a bit since you have tested it.
The modifications shouldn't affect the behavior, but if you could re-test it,
that would be great.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH 1/3] ACPI / dock: Initialize ACPI dock subsystem upfront

[Rafael J. Wysocki]

From: Jiang Liu <jiang.liu@huawei.com>

Commit 3b63aaa70e1 (PCI: acpiphp: Do not use ACPI PCI subdriver
mechanism) introduced an ACPI dock support regression, because it
changed the relative initialization order of the ACPI dock subsystem
and the ACPI-based PCI hotplug (acpiphp).

Namely, the ACPI dock subsystem has to be initialized before
acpiphp_enumerate_slots() is first run, which after commit
3b63aaa70e1 happens during the initial enumeration of the PCI
hierarchy triggered by the initial ACPI namespace scan in
acpi_scan_init().  For this reason, the dock subsystem has to be
initialized before the initial ACPI namespace scan in
acpi_scan_init().

To make that happen, modify the ACPI dock subsystem to be
non-modular and add the invocation of its initialization routine,
acpi_dock_init(), to acpi_scan_init() directly before the initial
namespace scan.

[rjw: Changelog, removal of dock_exit().]
References: https://bugzilla.kernel.org/show_bug.cgi?id=59501
Reported-and-tested-by: Alexander E. Patrakov <patrakov@gmail.com>
Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Cc: 3.9+ <stable@vger.kernel.org>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/dock.c     |   42 +-----------------------------------------
 drivers/acpi/internal.h |    5 +++++
 drivers/acpi/scan.c     |    1 +
 3 files changed, 7 insertions(+), 41 deletions(-)

Index: linux-pm/drivers/acpi/dock.c
===================================================================
--- linux-pm.orig/drivers/acpi/dock.c
+++ linux-pm/drivers/acpi/dock.c
@@ -994,30 +994,6 @@ err_unregister:
 }
 
 /**
- * dock_remove - free up resources related to the dock station
- */
-static int dock_remove(struct dock_station *ds)
-{
-	struct dock_dependent_device *dd, *tmp;
-	struct platform_device *dock_device = ds->dock_device;
-
-	if (!dock_station_count)
-		return 0;
-
-	/* remove dependent devices */
-	list_for_each_entry_safe(dd, tmp, &ds->dependent_devices, list)
-		kfree(dd);
-
-	list_del(&ds->sibling);
-
-	/* cleanup sysfs */
-	sysfs_remove_group(&dock_device->dev.kobj, &dock_attribute_group);
-	platform_device_unregister(dock_device);
-
-	return 0;
-}
-
-/**
  * find_dock_and_bay - look for dock stations and bays
  * @handle: acpi handle of a device
  * @lvl: unused
@@ -1035,7 +1011,7 @@ find_dock_and_bay(acpi_handle handle, u3
 	return AE_OK;
 }
 
-static int __init dock_init(void)
+int __init acpi_dock_init(void)
 {
 	if (acpi_disabled)
 		return 0;
@@ -1054,19 +1030,3 @@ static int __init dock_init(void)
 		ACPI_DOCK_DRIVER_DESCRIPTION, dock_station_count);
 	return 0;
 }
-
-static void __exit dock_exit(void)
-{
-	struct dock_station *tmp, *dock_station;
-
-	unregister_acpi_bus_notifier(&dock_acpi_notifier);
-	list_for_each_entry_safe(dock_station, tmp, &dock_stations, sibling)
-		dock_remove(dock_station);
-}
-
-/*
- * Must be called before drivers of devices in dock, otherwise we can't know
- * which devices are in a dock
- */
-subsys_initcall(dock_init);
-module_exit(dock_exit);
Index: linux-pm/drivers/acpi/internal.h
===================================================================
--- linux-pm.orig/drivers/acpi/internal.h
+++ linux-pm/drivers/acpi/internal.h
@@ -41,6 +41,11 @@ void acpi_container_init(void);
 #else
 static inline void acpi_container_init(void) {}
 #endif
+#ifdef CONFIG_ACPI_DOCK
+void acpi_dock_init(void);
+#else
+static inline void acpi_dock_init(void) {}
+#endif
 #ifdef CONFIG_ACPI_HOTPLUG_MEMORY
 void acpi_memory_hotplug_init(void);
 #else
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -2134,6 +2134,7 @@ int __init acpi_scan_init(void)
 	acpi_cmos_rtc_init();
 	acpi_container_init();
 	acpi_memory_hotplug_init();
+	acpi_dock_init();
 
 	mutex_lock(&acpi_scan_lock);
 	/*

[PATCH 2/3] PCI / ACPI: Use boot-time resource allocation rules during hotplug

[Rafael J. Wysocki]

From: Jiang Liu <jiang.liu@huawei.com>

On x86 platforms, the kernel respects PCI resource assignments from
the BIOS and only reassigns resources for unassigned BARs at boot
time.  However, with the ACPI-based hotplug (acpiphp), it ignores the
BIOS' PCI resource assignments completely and reassigns all resources
by itself.  This causes differences in PCI resource allocation
between boot time and runtime hotplug to occur, which is generally
undesirable and sometimes actively breaks things.

Namely, if there are enough resources, reassigning all PCI resources
during runtime hotplug should work, but it may fail if the resources
are constrained.  This may happen, for instance, when some PCI
devices with huge MMIO BARs are involved in the runtime hotplug
operations, because the current PCI MMIO alignment algorithm may
waste huge chunks of MMIO address space in those cases.

On the Alexander's Sony VAIO VPCZ23A4R the BIOS allocates limited
MMIO resources for the dock station which contains a device
(graphics adapter) with a 256MB MMIO BAR.  An attempt to reassign
that during runtime hotplug causes the dock station MMIO window to be
exhausted and acpiphp fails to allocate resources for the majority
of devices on the dock station as a result.

To prevent that from happening, modify acpiphp to follow the boot
time resources allocation behavior so that the BIOS' resource
assignments are respected during runtime hotplug too.

[rjw: Changelog]
References: https://bugzilla.kernel.org/show_bug.cgi?id=56531
Reported-and-tested-by: Alexander E. Patrakov <patrakov@gmail.com>
Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Acked-by: Yinghai Lu <yinghai@kernel.org>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/pci/hotplug/acpiphp_glue.c | 7 +++++--
 drivers/pci/hotplug/acpiphp_glue.c |    7 +++++--
 drivers/pci/pci.h                  |    5 +++++
 drivers/pci/setup-bus.c            |    8 ++++----
 3 files changed, 14 insertions(+), 6 deletions(-)

Index: linux-pm/drivers/pci/hotplug/acpiphp_glue.c
===================================================================
--- linux-pm.orig/drivers/pci/hotplug/acpiphp_glue.c
+++ linux-pm/drivers/pci/hotplug/acpiphp_glue.c
@@ -670,6 +670,7 @@ static int __ref enable_device(struct ac
 	struct pci_bus *bus = slot->bridge->pci_bus;
 	struct acpiphp_func *func;
 	int num, max, pass;
+	LIST_HEAD(add_list);
 
 	if (slot->flags & SLOT_ENABLED)
 		goto err_exit;
@@ -694,13 +695,15 @@ static int __ref enable_device(struct ac
 				max = pci_scan_bridge(bus, dev, max, pass);
 				if (pass && dev->subordinate) {
 					check_hotplug_bridge(slot, dev);
-					pci_bus_size_bridges(dev->subordinate);
+					pcibios_resource_survey_bus(dev->subordinate);
+					__pci_bus_size_bridges(dev->subordinate,
+							       &add_list);
 				}
 			}
 		}
 	}
 
-	pci_bus_assign_resources(bus);
+	__pci_bus_assign_resources(bus, &add_list, NULL);
 	acpiphp_sanitize_bus(bus);
 	acpiphp_set_hpp_values(bus);
 	acpiphp_set_acpi_region(slot);
Index: linux-pm/drivers/pci/pci.h
===================================================================
--- linux-pm.orig/drivers/pci/pci.h
+++ linux-pm/drivers/pci/pci.h
@@ -202,6 +202,11 @@ int __pci_read_base(struct pci_dev *dev,
 		    struct resource *res, unsigned int reg);
 int pci_resource_bar(struct pci_dev *dev, int resno, enum pci_bar_type *type);
 void pci_configure_ari(struct pci_dev *dev);
+void __ref __pci_bus_size_bridges(struct pci_bus *bus,
+			struct list_head *realloc_head);
+void __ref __pci_bus_assign_resources(const struct pci_bus *bus,
+				      struct list_head *realloc_head,
+				      struct list_head *fail_head);
 
 /**
  * pci_ari_enabled - query ARI forwarding status
Index: linux-pm/drivers/pci/setup-bus.c
===================================================================
--- linux-pm.orig/drivers/pci/setup-bus.c
+++ linux-pm/drivers/pci/setup-bus.c
@@ -1044,7 +1044,7 @@ handle_done:
 	;
 }
 
-static void __ref __pci_bus_size_bridges(struct pci_bus *bus,
+void __ref __pci_bus_size_bridges(struct pci_bus *bus,
 			struct list_head *realloc_head)
 {
 	struct pci_dev *dev;
@@ -1115,9 +1115,9 @@ void __ref pci_bus_size_bridges(struct p
 }
 EXPORT_SYMBOL(pci_bus_size_bridges);
 
-static void __ref __pci_bus_assign_resources(const struct pci_bus *bus,
-					 struct list_head *realloc_head,
-					 struct list_head *fail_head)
+void __ref __pci_bus_assign_resources(const struct pci_bus *bus,
+				      struct list_head *realloc_head,
+				      struct list_head *fail_head)
 {
 	struct pci_bus *b;
 	struct pci_dev *dev;

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

[Rafael J. Wysocki]

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

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, after 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-by: Alexander E. Patrakov <patrakov@gmail.com>
Tracked-down-by: Jiang Liu <jiang.liu@huawei.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: 3.9+ <stable@vger.kernel.org>
---
 drivers/acpi/dock.c                |  152 +++++++++++++++++++++++++------------
 drivers/pci/hotplug/acpiphp_glue.c |   46 +++++++----
 include/acpi/acpi_drivers.h        |    4 
 3 files changed, 138 insertions(+), 64 deletions(-)

Index: linux-pm/drivers/acpi/dock.c
===================================================================
--- linux-pm.orig/drivers/acpi/dock.c
+++ linux-pm/drivers/acpi/dock.c
@@ -66,20 +66,21 @@ struct dock_station {
 	spinlock_t dd_lock;
 	struct mutex hp_lock;
 	struct list_head dependent_devices;
-	struct list_head hotplug_devices;
 
 	struct list_head sibling;
 	struct platform_device *dock_device;
 };
 static LIST_HEAD(dock_stations);
 static int dock_station_count;
+static DEFINE_MUTEX(hotplug_lock);
 
 struct dock_dependent_device {
 	struct list_head list;
-	struct list_head hotplug_list;
 	acpi_handle handle;
-	const struct acpi_dock_ops *ops;
-	void *context;
+	const struct acpi_dock_ops *hp_ops;
+	void *hp_context;
+	unsigned int hp_refcount;
+	void (*hp_release)(void *);
 };
 
 #define DOCK_DOCKING	0x00000001
@@ -111,7 +112,6 @@ add_dock_dependent_device(struct dock_st
 
 	dd->handle = handle;
 	INIT_LIST_HEAD(&dd->list);
-	INIT_LIST_HEAD(&dd->hotplug_list);
 
 	spin_lock(&ds->dd_lock);
 	list_add_tail(&dd->list, &ds->dependent_devices);
@@ -121,35 +121,90 @@ add_dock_dependent_device(struct dock_st
 }
 
 /**
- * dock_add_hotplug_device - associate a hotplug handler with the dock station
- * @ds: The dock station
- * @dd: The dependent device struct
- *
- * Add the dependent device to the dock's hotplug device list
- */
-static void
-dock_add_hotplug_device(struct dock_station *ds,
-			struct dock_dependent_device *dd)
-{
-	mutex_lock(&ds->hp_lock);
-	list_add_tail(&dd->hotplug_list, &ds->hotplug_devices);
-	mutex_unlock(&ds->hp_lock);
+ * dock_init_hotplug - Initialize a hotplug device on a docking station.
+ * @dd: Dock-dependent device.
+ * @ops: Dock operations to attach to the dependent device.
+ * @context: Data to pass to the @ops callbacks and @release.
+ * @init: Optional initialization routine to run after setting up context.
+ * @release: Optional release routine to run on removal.
+ */
+static int dock_init_hotplug(struct dock_dependent_device *dd,
+			     const struct acpi_dock_ops *ops, void *context,
+			     void (*init)(void *), void (*release)(void *))
+{
+	int ret = 0;
+
+	mutex_lock(&hotplug_lock);
+
+	if (dd->hp_context) {
+		ret = -EEXIST;
+	} else {
+		dd->hp_refcount = 1;
+		dd->hp_ops = ops;
+		dd->hp_context = context;
+		dd->hp_release = release;
+	}
+
+	if (!WARN_ON(ret) && init)
+		init(context);
+
+	mutex_unlock(&hotplug_lock);
+	return ret;
 }
 
 /**
- * dock_del_hotplug_device - remove a hotplug handler from the dock station
- * @ds: The dock station
- * @dd: the dependent device struct
+ * dock_release_hotplug - Decrement hotplug reference counter of dock device.
+ * @dd: Dock-dependent device.
  *
- * Delete the dependent device from the dock's hotplug device list
+ * Decrement the reference counter of @dd and if 0, detach its hotplug
+ * operations from it, reset its context pointer and run the optional release
+ * routine if present.
  */
-static void
-dock_del_hotplug_device(struct dock_station *ds,
-			struct dock_dependent_device *dd)
+static void dock_release_hotplug(struct dock_dependent_device *dd)
 {
-	mutex_lock(&ds->hp_lock);
-	list_del(&dd->hotplug_list);
-	mutex_unlock(&ds->hp_lock);
+	void (*release)(void *) = NULL;
+	void *context = NULL;
+
+	mutex_lock(&hotplug_lock);
+
+	if (dd->hp_context && !--dd->hp_refcount) {
+		dd->hp_ops = NULL;
+		context = dd->hp_context;
+		dd->hp_context = NULL;
+		release = dd->hp_release;
+		dd->hp_release = NULL;
+	}
+
+	if (release && context)
+		release(context);
+
+	mutex_unlock(&hotplug_lock);
+}
+
+static void dock_hotplug_event(struct dock_dependent_device *dd, u32 event,
+			       bool uevent)
+{
+	acpi_notify_handler cb = NULL;
+	bool run = false;
+
+	mutex_lock(&hotplug_lock);
+
+	if (dd->hp_context) {
+		run = true;
+		dd->hp_refcount++;
+		if (dd->hp_ops)
+			cb = uevent ? dd->hp_ops->uevent : dd->hp_ops->handler;
+	}
+
+	mutex_unlock(&hotplug_lock);
+
+	if (!run)
+		return;
+
+	if (cb)
+		cb(dd->handle, event, dd->hp_context);
+
+	dock_release_hotplug(dd);
 }
 
 /**
@@ -360,9 +415,8 @@ static void hotplug_dock_devices(struct
 	/*
 	 * First call driver specific hotplug functions
 	 */
-	list_for_each_entry(dd, &ds->hotplug_devices, hotplug_list)
-		if (dd->ops && dd->ops->handler)
-			dd->ops->handler(dd->handle, event, dd->context);
+	list_for_each_entry(dd, &ds->dependent_devices, list)
+		dock_hotplug_event(dd, event, false);
 
 	/*
 	 * Now make sure that an acpi_device is created for each
@@ -398,9 +452,8 @@ static void dock_event(struct dock_stati
 	if (num == DOCK_EVENT)
 		kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
 
-	list_for_each_entry(dd, &ds->hotplug_devices, hotplug_list)
-		if (dd->ops && dd->ops->uevent)
-			dd->ops->uevent(dd->handle, event, dd->context);
+	list_for_each_entry(dd, &ds->dependent_devices, list)
+		dock_hotplug_event(dd, event, true);
 
 	if (num != DOCK_EVENT)
 		kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
@@ -570,18 +623,22 @@ EXPORT_SYMBOL_GPL(unregister_dock_notifi
  * @handle: the handle of the device
  * @ops: handlers to call after docking
  * @context: device specific data
+ * @init: Optional initialization routine to run after registration
+ * @release: Optional release routine to run on unregistration
  *
  * If a driver would like to perform a hotplug operation after a dock
  * event, they can register an acpi_notifiy_handler to be called by
  * the dock driver after _DCK is executed.
  */
-int
-register_hotplug_dock_device(acpi_handle handle, const struct acpi_dock_ops *ops,
-			     void *context)
+int register_hotplug_dock_device(acpi_handle handle,
+				 const struct acpi_dock_ops *ops, void *context,
+				 void (*init)(void *), void (*release)(void *))
 {
 	struct dock_dependent_device *dd;
 	struct dock_station *dock_station;
-	int ret = -EINVAL;
+
+	if (WARN_ON(!context))
+		return -EINVAL;
 
 	if (!dock_station_count)
 		return -ENODEV;
@@ -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);
 	}
-
-	return ret;
+	return -EINVAL;
 }
 EXPORT_SYMBOL_GPL(register_hotplug_dock_device);
 
@@ -623,8 +676,10 @@ void unregister_hotplug_dock_device(acpi
 
 	list_for_each_entry(dock_station, &dock_stations, sibling) {
 		dd = find_dock_dependent_device(dock_station, handle);
-		if (dd)
-			dock_del_hotplug_device(dock_station, dd);
+		if (dd) {
+			dock_release_hotplug(dd);
+			return;
+		}
 	}
 }
 EXPORT_SYMBOL_GPL(unregister_hotplug_dock_device);
@@ -953,7 +1008,6 @@ static int __init dock_add(acpi_handle h
 	mutex_init(&dock_station->hp_lock);
 	spin_lock_init(&dock_station->dd_lock);
 	INIT_LIST_HEAD(&dock_station->sibling);
-	INIT_LIST_HEAD(&dock_station->hotplug_devices);
 	ATOMIC_INIT_NOTIFIER_HEAD(&dock_notifier_list);
 	INIT_LIST_HEAD(&dock_station->dependent_devices);
 
Index: linux-pm/include/acpi/acpi_drivers.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_drivers.h
+++ linux-pm/include/acpi/acpi_drivers.h
@@ -123,7 +123,9 @@ extern int register_dock_notifier(struct
 extern void unregister_dock_notifier(struct notifier_block *nb);
 extern int register_hotplug_dock_device(acpi_handle handle,
 					const struct acpi_dock_ops *ops,
-					void *context);
+					void *context,
+					void (*init)(void *),
+					void (*release)(void *));
 extern void unregister_hotplug_dock_device(acpi_handle handle);
 #else
 static inline int is_dock_device(acpi_handle handle)
Index: linux-pm/drivers/pci/hotplug/acpiphp_glue.c
===================================================================
--- linux-pm.orig/drivers/pci/hotplug/acpiphp_glue.c
+++ linux-pm/drivers/pci/hotplug/acpiphp_glue.c
@@ -61,6 +61,7 @@ static DEFINE_MUTEX(bridge_mutex);
 static void handle_hotplug_event_bridge (acpi_handle, u32, void *);
 static void acpiphp_sanitize_bus(struct pci_bus *bus);
 static void acpiphp_set_hpp_values(struct pci_bus *bus);
+static void hotplug_event_func(acpi_handle handle, u32 type, void *context);
 static void handle_hotplug_event_func(acpi_handle handle, u32 type, void *context);
 static void free_bridge(struct kref *kref);
 
@@ -147,7 +148,7 @@ static int post_dock_fixups(struct notif
 
 
 static const struct acpi_dock_ops acpiphp_dock_ops = {
-	.handler = handle_hotplug_event_func,
+	.handler = hotplug_event_func,
 };
 
 /* Check whether the PCI device is managed by native PCIe hotplug driver */
@@ -179,6 +180,20 @@ static bool device_is_managed_by_native_
 	return true;
 }
 
+static void acpiphp_dock_init(void *data)
+{
+	struct acpiphp_func *func = data;
+
+	get_bridge(func->slot->bridge);
+}
+
+static void acpiphp_dock_release(void *data)
+{
+	struct acpiphp_func *func = data;
+
+	put_bridge(func->slot->bridge);
+}
+
 /* callback routine to register each ACPI PCI slot object */
 static acpi_status
 register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
@@ -298,7 +313,8 @@ register_slot(acpi_handle handle, u32 lv
 		 */
 		newfunc->flags &= ~FUNC_HAS_EJ0;
 		if (register_hotplug_dock_device(handle,
-			&acpiphp_dock_ops, newfunc))
+			&acpiphp_dock_ops, newfunc,
+			acpiphp_dock_init, acpiphp_dock_release))
 			dbg("failed to register dock device\n");
 
 		/* we need to be notified when dock events happen
@@ -1068,22 +1084,12 @@ static void handle_hotplug_event_bridge(
 	alloc_acpi_hp_work(handle, type, context, _handle_hotplug_event_bridge);
 }
 
-static void _handle_hotplug_event_func(struct work_struct *work)
+static void hotplug_event_func(acpi_handle handle, u32 type, void *context)
 {
-	struct acpiphp_func *func;
+	struct acpiphp_func *func = context;
 	char objname[64];
 	struct acpi_buffer buffer = { .length = sizeof(objname),
 				      .pointer = objname };
-	struct acpi_hp_work *hp_work;
-	acpi_handle handle;
-	u32 type;
-
-	hp_work = container_of(work, struct acpi_hp_work, work);
-	handle = hp_work->handle;
-	type = hp_work->type;
-	func = (struct acpiphp_func *)hp_work->context;
-
-	acpi_scan_lock_acquire();
 
 	acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
 
@@ -1116,6 +1122,18 @@ static void _handle_hotplug_event_func(s
 		warn("notify_handler: unknown event type 0x%x for %s\n", type, objname);
 		break;
 	}
+}
+
+static void _handle_hotplug_event_func(struct work_struct *work)
+{
+	struct acpi_hp_work *hp_work;
+	struct acpiphp_func *func;
+
+	hp_work = container_of(work, struct acpi_hp_work, work);
+	func = hp_work->context;
+	acpi_scan_lock_acquire();
+
+	hotplug_event_func(hp_work->handle, hp_work->type, func);
 
 	acpi_scan_lock_release();
 	kfree(hp_work); /* allocated in handle_hotplug_event_func */

Re: [PATCH 1/3] ACPI / dock: Initialize ACPI dock subsystem upfront

[Yinghai Lu]

On Sat, Jun 22, 2013 at 2:21 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> From: Jiang Liu <jiang.liu@huawei.com>
>
> Commit 3b63aaa70e1 (PCI: acpiphp: Do not use ACPI PCI subdriver
> mechanism) introduced an ACPI dock support regression, because it
> changed the relative initialization order of the ACPI dock subsystem
> and the ACPI-based PCI hotplug (acpiphp).
>
> Namely, the ACPI dock subsystem has to be initialized before
> acpiphp_enumerate_slots() is first run, which after commit
> 3b63aaa70e1 happens during the initial enumeration of the PCI
> hierarchy triggered by the initial ACPI namespace scan in
> acpi_scan_init().  For this reason, the dock subsystem has to be
> initialized before the initial ACPI namespace scan in
> acpi_scan_init().
>
> To make that happen, modify the ACPI dock subsystem to be
> non-modular and add the invocation of its initialization routine,
> acpi_dock_init(), to acpi_scan_init() directly before the initial
> namespace scan.
>
> [rjw: Changelog, removal of dock_exit().]
> References: https://bugzilla.kernel.org/show_bug.cgi?id=59501
> Reported-and-tested-by: Alexander E. Patrakov <patrakov@gmail.com>
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> Cc: 3.9+ <stable@vger.kernel.org>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/dock.c     |   42 +-----------------------------------------
>  drivers/acpi/internal.h |    5 +++++
>  drivers/acpi/scan.c     |    1 +
>  3 files changed, 7 insertions(+), 41 deletions(-)
>
> Index: linux-pm/drivers/acpi/dock.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/dock.c
> +++ linux-pm/drivers/acpi/dock.c
> @@ -994,30 +994,6 @@ err_unregister:
>  }
>
>  /**
> - * dock_remove - free up resources related to the dock station
> - */
> -static int dock_remove(struct dock_station *ds)
> -{
> -       struct dock_dependent_device *dd, *tmp;
> -       struct platform_device *dock_device = ds->dock_device;
> -
> -       if (!dock_station_count)
> -               return 0;
> -
> -       /* remove dependent devices */
> -       list_for_each_entry_safe(dd, tmp, &ds->dependent_devices, list)
> -               kfree(dd);
> -
> -       list_del(&ds->sibling);
> -
> -       /* cleanup sysfs */
> -       sysfs_remove_group(&dock_device->dev.kobj, &dock_attribute_group);
> -       platform_device_unregister(dock_device);
> -
> -       return 0;
> -}
> -
> -/**
>   * find_dock_and_bay - look for dock stations and bays
>   * @handle: acpi handle of a device
>   * @lvl: unused
> @@ -1035,7 +1011,7 @@ find_dock_and_bay(acpi_handle handle, u3
>         return AE_OK;
>  }
>
> -static int __init dock_init(void)
> +int __init acpi_dock_init(void)
>  {
>         if (acpi_disabled)
>                 return 0;
> @@ -1054,19 +1030,3 @@ static int __init dock_init(void)
>                 ACPI_DOCK_DRIVER_DESCRIPTION, dock_station_count);
>         return 0;
>  }
> -
> -static void __exit dock_exit(void)
> -{
> -       struct dock_station *tmp, *dock_station;
> -
> -       unregister_acpi_bus_notifier(&dock_acpi_notifier);
> -       list_for_each_entry_safe(dock_station, tmp, &dock_stations, sibling)
> -               dock_remove(dock_station);
> -}
> -
> -/*
> - * Must be called before drivers of devices in dock, otherwise we can't know
> - * which devices are in a dock
> - */
> -subsys_initcall(dock_init);
> -module_exit(dock_exit);

Finally that weird pair is gone.

Acked-by: Yinghai Lu <yinghai@kernel.org>

> Index: linux-pm/drivers/acpi/internal.h
> ===================================================================
> --- linux-pm.orig/drivers/acpi/internal.h
> +++ linux-pm/drivers/acpi/internal.h
> @@ -41,6 +41,11 @@ void acpi_container_init(void);
>  #else
>  static inline void acpi_container_init(void) {}
>  #endif
> +#ifdef CONFIG_ACPI_DOCK
> +void acpi_dock_init(void);
> +#else
> +static inline void acpi_dock_init(void) {}
> +#endif
>  #ifdef CONFIG_ACPI_HOTPLUG_MEMORY
>  void acpi_memory_hotplug_init(void);
>  #else
> Index: linux-pm/drivers/acpi/scan.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/scan.c
> +++ linux-pm/drivers/acpi/scan.c
> @@ -2134,6 +2134,7 @@ int __init acpi_scan_init(void)
>         acpi_cmos_rtc_init();
>         acpi_container_init();
>         acpi_memory_hotplug_init();
> +       acpi_dock_init();
>
>         mutex_lock(&acpi_scan_lock);
>         /*
>

Re: [PATCH 0/3] ACPI / dock / PCI: Fix problems with dock and PCI hotplug

[Illya Klymov]

Hi, guys. Sorry for entering your talk
I have a Sony Vaio SVZ1311Z9RXI and just tested your patched against
3.10-rc7 and they work like a charm
scenarios tested:
- boot with dock
- boot without dock, hotplug dock
- dock / undock / redock

All devices tested to be working in all 3 cases:
- video card
- ethernet controller
- blue ray drive
- USB controller

Thank you a lot!

On Sun, Jun 23, 2013 at 12:19 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> Hi All,
>
> Unfortunately, the Alexander's docking station for Sony VAIO VPCZ23A4R is
> basically unusable with v3.10-rc6 and, as it turns out, there are multiple
> problems with it.
>
> First, the acpiphp initialization ordering has changed recently,
> acpiphp_enumerate_slots() is now always run for the first time during the
> initial ACPI namespace scan in acpi_scan_init() which has broken support
> for PCI devices on ACPI-based docking stations, because the ACPI dock
> subsystem has to be initialized *before* acpiphp_enumerate_slots() is first
> run.
>
> Fix that, and it turns out that undocking actually doesn't work correctly
> because of some synchronization problems between the dock driver, acpiphp
> and the PCI core.  [That part was actually the most difficult to fix.]
>
> Fix that, and you'll find that PCI resources are not allocated correctly
> after undocking and re-docking (when initially docked).
>
> The three patches in this series make the Alexander's docking station kind
> of usable, so if no one has objections, I'd like to put them on a fast track
> to Linus.
>
> [1/3] Initialize ACPI dock subsystem before enumerating the PCI hierarchy
>       (that also makes the dock driver non-modular).
>
> [2/3] Make acpiphp use the same rules for allocating PCI resources that are
>       used at boot time.
>
> [3/3] Make acpiphp handle dock events synchronously (instead of spawning
>       separate work items to handle them).  [The changelog of this patch
>       is likely one of the longest I've ever written and it sort of looks
>       like a crime story.]
>
> This series is mostly based on the work of Jiang Liu (thanks Gerry!) and
> the Alexander's testing that we wouldn't have done much progress without
> (thanks Alexander!).
>
> Alexander, I've modified patch [3/3] a bit since you have tested it.
> The modifications shouldn't affect the behavior, but if you could re-test it,
> that would be great.
>
> Thanks,
> Rafael
>
>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] ACPI / dock / PCI: Fix problems with dock and PCI hotplug

[Rafael J. Wysocki]

On Sunday, June 23, 2013 12:43:08 AM Illya Klymov wrote:
> Hi, guys. Sorry for entering your talk

Hi,

No problem at all. :-)

> I have a Sony Vaio SVZ1311Z9RXI and just tested your patched against
> 3.10-rc7 and they work like a charm
> scenarios tested:
> - boot with dock
> - boot without dock, hotplug dock
> - dock / undock / redock
> 
> All devices tested to be working in all 3 cases:
> - video card
> - ethernet controller
> - blue ray drive
> - USB controller
> 
> Thank you a lot!

I'm glad that the patches work for you, thanks a lot for letting us know!

Rafael


> On Sun, Jun 23, 2013 at 12:19 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > Hi All,
> >
> > Unfortunately, the Alexander's docking station for Sony VAIO VPCZ23A4R is
> > basically unusable with v3.10-rc6 and, as it turns out, there are multiple
> > problems with it.
> >
> > First, the acpiphp initialization ordering has changed recently,
> > acpiphp_enumerate_slots() is now always run for the first time during the
> > initial ACPI namespace scan in acpi_scan_init() which has broken support
> > for PCI devices on ACPI-based docking stations, because the ACPI dock
> > subsystem has to be initialized *before* acpiphp_enumerate_slots() is first
> > run.
> >
> > Fix that, and it turns out that undocking actually doesn't work correctly
> > because of some synchronization problems between the dock driver, acpiphp
> > and the PCI core.  [That part was actually the most difficult to fix.]
> >
> > Fix that, and you'll find that PCI resources are not allocated correctly
> > after undocking and re-docking (when initially docked).
> >
> > The three patches in this series make the Alexander's docking station kind
> > of usable, so if no one has objections, I'd like to put them on a fast track
> > to Linus.
> >
> > [1/3] Initialize ACPI dock subsystem before enumerating the PCI hierarchy
> >       (that also makes the dock driver non-modular).
> >
> > [2/3] Make acpiphp use the same rules for allocating PCI resources that are
> >       used at boot time.
> >
> > [3/3] Make acpiphp handle dock events synchronously (instead of spawning
> >       separate work items to handle them).  [The changelog of this patch
> >       is likely one of the longest I've ever written and it sort of looks
> >       like a crime story.]
> >
> > This series is mostly based on the work of Jiang Liu (thanks Gerry!) and
> > the Alexander's testing that we wouldn't have done much progress without
> > (thanks Alexander!).
> >
> > Alexander, I've modified patch [3/3] a bit since you have tested it.
> > The modifications shouldn't affect the behavior, but if you could re-test it,
> > that would be great.
> >
> > Thanks,
> > Rafael
> >
> >
> > --
> > I speak only for myself.
> > Rafael J. Wysocki, Intel Open Source Technology Center.
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 3/3] ACPI / dock / PCI: Synchronous handling of dock events for PCI devices

[Yinghai Lu]

On Sat, Jun 22, 2013 at 2:25 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> 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, after 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.
..
> -static void
> -dock_del_hotplug_device(struct dock_station *ds,
> -                       struct dock_dependent_device *dd)
> +static void dock_release_hotplug(struct dock_dependent_device *dd)
>  {
> -       mutex_lock(&ds->hp_lock);
> -       list_del(&dd->hotplug_list);
> -       mutex_unlock(&ds->hp_lock);
> +       void (*release)(void *) = NULL;
> +       void *context = NULL;
> +
> +       mutex_lock(&hotplug_lock);
> +
> +       if (dd->hp_context && !--dd->hp_refcount) {
> +               dd->hp_ops = NULL;
> +               context = dd->hp_context;
> +               dd->hp_context = NULL;
> +               release = dd->hp_release;
> +               dd->hp_release = NULL;
> +       }
> +
> +       if (release && context)
> +               release(context);
> +
> +       mutex_unlock(&hotplug_lock);
> +}
> +
> +static void dock_hotplug_event(struct dock_dependent_device *dd, u32 event,
> +                              bool uevent)
> +{
> +       acpi_notify_handler cb = NULL;
> +       bool run = false;
> +
> +       mutex_lock(&hotplug_lock);
> +
> +       if (dd->hp_context) {
> +               run = true;
> +               dd->hp_refcount++;
> +               if (dd->hp_ops)
> +                       cb = uevent ? dd->hp_ops->uevent : dd->hp_ops->handler;
> +       }
> +
> +       mutex_unlock(&hotplug_lock);
> +
> +       if (!run)
> +               return;
> +
> +       if (cb)
> +               cb(dd->handle, event, dd->hp_context);
> +
> +       dock_release_hotplug(dd);

during DOCKING, dock_release_hotplug get called too?

Yinghai

Re: [PATCH 3/3] ACPI / dock / PCI: Synchronous handling of dock events for PCI devices

[Rafael J. Wysocki]

On Saturday, June 22, 2013 05:22:20 PM Yinghai Lu wrote:
> On Sat, Jun 22, 2013 at 2:25 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > 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, after 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.
> ..
> > -static void
> > -dock_del_hotplug_device(struct dock_station *ds,
> > -                       struct dock_dependent_device *dd)
> > +static void dock_release_hotplug(struct dock_dependent_device *dd)
> >  {
> > -       mutex_lock(&ds->hp_lock);
> > -       list_del(&dd->hotplug_list);
> > -       mutex_unlock(&ds->hp_lock);
> > +       void (*release)(void *) = NULL;
> > +       void *context = NULL;
> > +
> > +       mutex_lock(&hotplug_lock);
> > +
> > +       if (dd->hp_context && !--dd->hp_refcount) {
> > +               dd->hp_ops = NULL;
> > +               context = dd->hp_context;
> > +               dd->hp_context = NULL;
> > +               release = dd->hp_release;
> > +               dd->hp_release = NULL;
> > +       }
> > +
> > +       if (release && context)
> > +               release(context);
> > +
> > +       mutex_unlock(&hotplug_lock);
> > +}
> > +
> > +static void dock_hotplug_event(struct dock_dependent_device *dd, u32 event,
> > +                              bool uevent)
> > +{
> > +       acpi_notify_handler cb = NULL;
> > +       bool run = false;
> > +
> > +       mutex_lock(&hotplug_lock);
> > +
> > +       if (dd->hp_context) {
> > +               run = true;
> > +               dd->hp_refcount++;
> > +               if (dd->hp_ops)
> > +                       cb = uevent ? dd->hp_ops->uevent : dd->hp_ops->handler;
> > +       }
> > +
> > +       mutex_unlock(&hotplug_lock);
> > +
> > +       if (!run)
> > +               return;
> > +
> > +       if (cb)
> > +               cb(dd->handle, event, dd->hp_context);
> > +
> > +       dock_release_hotplug(dd);
> 
> during DOCKING, dock_release_hotplug get called too?

Yes, we need to drop down the refcount we've just bumped up.

Thanks,
Rafael

Re: [PATCH 3/3] ACPI / dock / PCI: Synchronous handling of dock events for PCI devices

[Jiang Liu]

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

Regards!
Gerry

>  
> -
> -	return ret;
> +	return -EINVAL;
>  }
>  EXPORT_SYMBOL_GPL(register_hotplug_dock_device);
>  
> @@ -623,8 +676,10 @@ void unregister_hotplug_dock_device(acpi
>  
>  	list_for_each_entry(dock_station, &dock_stations, sibling) {
>  		dd = find_dock_dependent_device(dock_station, handle);
> -		if (dd)
> -			dock_del_hotplug_device(dock_station, dd);
> +		if (dd) {
> +			dock_release_hotplug(dd);
> +			return;
> +		}
>  	}
>  }
>  EXPORT_SYMBOL_GPL(unregister_hotplug_dock_device);
> @@ -953,7 +1008,6 @@ static int __init dock_add(acpi_handle h
>  	mutex_init(&dock_station->hp_lock);
>  	spin_lock_init(&dock_station->dd_lock);
>  	INIT_LIST_HEAD(&dock_station->sibling);
> -	INIT_LIST_HEAD(&dock_station->hotplug_devices);
>  	ATOMIC_INIT_NOTIFIER_HEAD(&dock_notifier_list);
>  	INIT_LIST_HEAD(&dock_station->dependent_devices);
>  
> Index: linux-pm/include/acpi/acpi_drivers.h
> ===================================================================
> --- linux-pm.orig/include/acpi/acpi_drivers.h
> +++ linux-pm/include/acpi/acpi_drivers.h
> @@ -123,7 +123,9 @@ extern int register_dock_notifier(struct
>  extern void unregister_dock_notifier(struct notifier_block *nb);
>  extern int register_hotplug_dock_device(acpi_handle handle,
>  					const struct acpi_dock_ops *ops,
> -					void *context);
> +					void *context,
> +					void (*init)(void *),
> +					void (*release)(void *));
>  extern void unregister_hotplug_dock_device(acpi_handle handle);
>  #else
>  static inline int is_dock_device(acpi_handle handle)
> Index: linux-pm/drivers/pci/hotplug/acpiphp_glue.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/hotplug/acpiphp_glue.c
> +++ linux-pm/drivers/pci/hotplug/acpiphp_glue.c
> @@ -61,6 +61,7 @@ static DEFINE_MUTEX(bridge_mutex);
>  static void handle_hotplug_event_bridge (acpi_handle, u32, void *);
>  static void acpiphp_sanitize_bus(struct pci_bus *bus);
>  static void acpiphp_set_hpp_values(struct pci_bus *bus);
> +static void hotplug_event_func(acpi_handle handle, u32 type, void *context);
>  static void handle_hotplug_event_func(acpi_handle handle, u32 type, void *context);
>  static void free_bridge(struct kref *kref);
>  
> @@ -147,7 +148,7 @@ static int post_dock_fixups(struct notif
>  
>  
>  static const struct acpi_dock_ops acpiphp_dock_ops = {
> -	.handler = handle_hotplug_event_func,
> +	.handler = hotplug_event_func,
>  };
>  
>  /* Check whether the PCI device is managed by native PCIe hotplug driver */
> @@ -179,6 +180,20 @@ static bool device_is_managed_by_native_
>  	return true;
>  }
>  
> +static void acpiphp_dock_init(void *data)
> +{
> +	struct acpiphp_func *func = data;
> +
> +	get_bridge(func->slot->bridge);
> +}
> +
> +static void acpiphp_dock_release(void *data)
> +{
> +	struct acpiphp_func *func = data;
> +
> +	put_bridge(func->slot->bridge);
> +}
> +
>  /* callback routine to register each ACPI PCI slot object */
>  static acpi_status
>  register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
> @@ -298,7 +313,8 @@ register_slot(acpi_handle handle, u32 lv
>  		 */
>  		newfunc->flags &= ~FUNC_HAS_EJ0;
>  		if (register_hotplug_dock_device(handle,
> -			&acpiphp_dock_ops, newfunc))
> +			&acpiphp_dock_ops, newfunc,
> +			acpiphp_dock_init, acpiphp_dock_release))
>  			dbg("failed to register dock device\n");
>  
>  		/* we need to be notified when dock events happen
> @@ -1068,22 +1084,12 @@ static void handle_hotplug_event_bridge(
>  	alloc_acpi_hp_work(handle, type, context, _handle_hotplug_event_bridge);
>  }
>  
> -static void _handle_hotplug_event_func(struct work_struct *work)
> +static void hotplug_event_func(acpi_handle handle, u32 type, void *context)
>  {
> -	struct acpiphp_func *func;
> +	struct acpiphp_func *func = context;
>  	char objname[64];
>  	struct acpi_buffer buffer = { .length = sizeof(objname),
>  				      .pointer = objname };
> -	struct acpi_hp_work *hp_work;
> -	acpi_handle handle;
> -	u32 type;
> -
> -	hp_work = container_of(work, struct acpi_hp_work, work);
> -	handle = hp_work->handle;
> -	type = hp_work->type;
> -	func = (struct acpiphp_func *)hp_work->context;
> -
> -	acpi_scan_lock_acquire();
>  
>  	acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
>  
> @@ -1116,6 +1122,18 @@ static void _handle_hotplug_event_func(s
>  		warn("notify_handler: unknown event type 0x%x for %s\n", type, objname);
>  		break;
>  	}
> +}
> +
> +static void _handle_hotplug_event_func(struct work_struct *work)
> +{
> +	struct acpi_hp_work *hp_work;
> +	struct acpiphp_func *func;
> +
> +	hp_work = container_of(work, struct acpi_hp_work, work);
> +	func = hp_work->context;
> +	acpi_scan_lock_acquire();
> +
> +	hotplug_event_func(hp_work->handle, hp_work->type, func);
>  
>  	acpi_scan_lock_release();
>  	kfree(hp_work); /* allocated in handle_hotplug_event_func */
> 

Re: [PATCH 0/3] ACPI / dock / PCI: Fix problems with dock and PCI hotplug

[Alexander E. Patrakov]

2013/6/23 Rafael J. Wysocki <rjw@sisk.pl>:
> Alexander, I've modified patch [3/3] a bit since you have tested it.
> The modifications shouldn't affect the behavior, but if you could re-test it,
> that would be great.

Retested, everything works just as with the previous version of your patch.

--
Alexander E. Patrakov

Re: [PATCH 3/3] ACPI / dock / PCI: Synchronous handling of dock events for PCI devices

[Yinghai Lu]

On Sun, Jun 23, 2013 at 2:59 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Saturday, June 22, 2013 05:22:20 PM Yinghai Lu wrote:
>> On Sat, Jun 22, 2013 at 2:25 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> >
>> > 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, after 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.
>> ..
>> > -static void
>> > -dock_del_hotplug_device(struct dock_station *ds,
>> > -                       struct dock_dependent_device *dd)
>> > +static void dock_release_hotplug(struct dock_dependent_device *dd)
>> >  {
>> > -       mutex_lock(&ds->hp_lock);
>> > -       list_del(&dd->hotplug_list);
>> > -       mutex_unlock(&ds->hp_lock);
>> > +       void (*release)(void *) = NULL;
>> > +       void *context = NULL;
>> > +
>> > +       mutex_lock(&hotplug_lock);
>> > +
>> > +       if (dd->hp_context && !--dd->hp_refcount) {
>> > +               dd->hp_ops = NULL;
>> > +               context = dd->hp_context;
>> > +               dd->hp_context = NULL;
>> > +               release = dd->hp_release;
>> > +               dd->hp_release = NULL;
>> > +       }
>> > +
>> > +       if (release && context)
>> > +               release(context);
>> > +
>> > +       mutex_unlock(&hotplug_lock);
>> > +}
>> > +
>> > +static void dock_hotplug_event(struct dock_dependent_device *dd, u32 event,
>> > +                              bool uevent)
>> > +{
>> > +       acpi_notify_handler cb = NULL;
>> > +       bool run = false;
>> > +
>> > +       mutex_lock(&hotplug_lock);
>> > +
>> > +       if (dd->hp_context) {
>> > +               run = true;
>> > +               dd->hp_refcount++;
>> > +               if (dd->hp_ops)
>> > +                       cb = uevent ? dd->hp_ops->uevent : dd->hp_ops->handler;
>> > +       }
>> > +
>> > +       mutex_unlock(&hotplug_lock);
>> > +
>> > +       if (!run)
>> > +               return;
>> > +
>> > +       if (cb)
>> > +               cb(dd->handle, event, dd->hp_context);
>> > +
>> > +       dock_release_hotplug(dd);
>>
>> during DOCKING, dock_release_hotplug get called too?
>
> Yes, we need to drop down the refcount we've just bumped up.
>

ok, I see.

Acked-by: Yinghai Lu <yinghai@kernel.org>

Re: [PATCH 3/3] ACPI / dock / PCI: Synchronous handling of dock events for PCI devices

[Yinghai Lu]

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?

Yinghai

Re: [PATCH 3/3] ACPI / dock / PCI: Synchronous handling of dock events for PCI devices

[Yinghai Lu]

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.

Yinghai

Re: [PATCH 0/3] ACPI / dock / PCI: Fix problems with dock and PCI hotplug

[Rafael J. Wysocki]

On Sunday, June 23, 2013 11:50:15 PM Alexander E. Patrakov wrote:
> 2013/6/23 Rafael J. Wysocki <rjw@sisk.pl>:
> > Alexander, I've modified patch [3/3] a bit since you have tested it.
> > The modifications shouldn't affect the behavior, but if you could re-test it,
> > that would be great.
> 
> Retested, everything works just as with the previous version of your patch.

Thanks for the confirmation!

Rafael

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

[Rafael J. Wysocki]

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(-)

Index: linux-pm/drivers/acpi/dock.c
===================================================================
--- linux-pm.orig/drivers/acpi/dock.c
+++ linux-pm/drivers/acpi/dock.c
@@ -66,20 +66,21 @@ struct dock_station {
 	spinlock_t dd_lock;
 	struct mutex hp_lock;
 	struct list_head dependent_devices;
-	struct list_head hotplug_devices;
 
 	struct list_head sibling;
 	struct platform_device *dock_device;
 };
 static LIST_HEAD(dock_stations);
 static int dock_station_count;
+static DEFINE_MUTEX(hotplug_lock);
 
 struct dock_dependent_device {
 	struct list_head list;
-	struct list_head hotplug_list;
 	acpi_handle handle;
-	const struct acpi_dock_ops *ops;
-	void *context;
+	const struct acpi_dock_ops *hp_ops;
+	void *hp_context;
+	unsigned int hp_refcount;
+	void (*hp_release)(void *);
 };
 
 #define DOCK_DOCKING	0x00000001
@@ -111,7 +112,6 @@ add_dock_dependent_device(struct dock_st
 
 	dd->handle = handle;
 	INIT_LIST_HEAD(&dd->list);
-	INIT_LIST_HEAD(&dd->hotplug_list);
 
 	spin_lock(&ds->dd_lock);
 	list_add_tail(&dd->list, &ds->dependent_devices);
@@ -121,35 +121,90 @@ add_dock_dependent_device(struct dock_st
 }
 
 /**
- * dock_add_hotplug_device - associate a hotplug handler with the dock station
- * @ds: The dock station
- * @dd: The dependent device struct
- *
- * Add the dependent device to the dock's hotplug device list
- */
-static void
-dock_add_hotplug_device(struct dock_station *ds,
-			struct dock_dependent_device *dd)
-{
-	mutex_lock(&ds->hp_lock);
-	list_add_tail(&dd->hotplug_list, &ds->hotplug_devices);
-	mutex_unlock(&ds->hp_lock);
+ * dock_init_hotplug - Initialize a hotplug device on a docking station.
+ * @dd: Dock-dependent device.
+ * @ops: Dock operations to attach to the dependent device.
+ * @context: Data to pass to the @ops callbacks and @release.
+ * @init: Optional initialization routine to run after setting up context.
+ * @release: Optional release routine to run on removal.
+ */
+static int dock_init_hotplug(struct dock_dependent_device *dd,
+			     const struct acpi_dock_ops *ops, void *context,
+			     void (*init)(void *), void (*release)(void *))
+{
+	int ret = 0;
+
+	mutex_lock(&hotplug_lock);
+
+	if (dd->hp_context) {
+		ret = -EEXIST;
+	} else {
+		dd->hp_refcount = 1;
+		dd->hp_ops = ops;
+		dd->hp_context = context;
+		dd->hp_release = release;
+	}
+
+	if (!WARN_ON(ret) && init)
+		init(context);
+
+	mutex_unlock(&hotplug_lock);
+	return ret;
 }
 
 /**
- * dock_del_hotplug_device - remove a hotplug handler from the dock station
- * @ds: The dock station
- * @dd: the dependent device struct
+ * dock_release_hotplug - Decrement hotplug reference counter of dock device.
+ * @dd: Dock-dependent device.
  *
- * Delete the dependent device from the dock's hotplug device list
+ * Decrement the reference counter of @dd and if 0, detach its hotplug
+ * operations from it, reset its context pointer and run the optional release
+ * routine if present.
  */
-static void
-dock_del_hotplug_device(struct dock_station *ds,
-			struct dock_dependent_device *dd)
+static void dock_release_hotplug(struct dock_dependent_device *dd)
 {
-	mutex_lock(&ds->hp_lock);
-	list_del(&dd->hotplug_list);
-	mutex_unlock(&ds->hp_lock);
+	void (*release)(void *) = NULL;
+	void *context = NULL;
+
+	mutex_lock(&hotplug_lock);
+
+	if (dd->hp_context && !--dd->hp_refcount) {
+		dd->hp_ops = NULL;
+		context = dd->hp_context;
+		dd->hp_context = NULL;
+		release = dd->hp_release;
+		dd->hp_release = NULL;
+	}
+
+	if (release && context)
+		release(context);
+
+	mutex_unlock(&hotplug_lock);
+}
+
+static void dock_hotplug_event(struct dock_dependent_device *dd, u32 event,
+			       bool uevent)
+{
+	acpi_notify_handler cb = NULL;
+	bool run = false;
+
+	mutex_lock(&hotplug_lock);
+
+	if (dd->hp_context) {
+		run = true;
+		dd->hp_refcount++;
+		if (dd->hp_ops)
+			cb = uevent ? dd->hp_ops->uevent : dd->hp_ops->handler;
+	}
+
+	mutex_unlock(&hotplug_lock);
+
+	if (!run)
+		return;
+
+	if (cb)
+		cb(dd->handle, event, dd->hp_context);
+
+	dock_release_hotplug(dd);
 }
 
 /**
@@ -360,9 +415,8 @@ static void hotplug_dock_devices(struct
 	/*
 	 * First call driver specific hotplug functions
 	 */
-	list_for_each_entry(dd, &ds->hotplug_devices, hotplug_list)
-		if (dd->ops && dd->ops->handler)
-			dd->ops->handler(dd->handle, event, dd->context);
+	list_for_each_entry(dd, &ds->dependent_devices, list)
+		dock_hotplug_event(dd, event, false);
 
 	/*
 	 * Now make sure that an acpi_device is created for each
@@ -398,9 +452,8 @@ static void dock_event(struct dock_stati
 	if (num == DOCK_EVENT)
 		kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
 
-	list_for_each_entry(dd, &ds->hotplug_devices, hotplug_list)
-		if (dd->ops && dd->ops->uevent)
-			dd->ops->uevent(dd->handle, event, dd->context);
+	list_for_each_entry(dd, &ds->dependent_devices, list)
+		dock_hotplug_event(dd, event, true);
 
 	if (num != DOCK_EVENT)
 		kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
@@ -570,19 +623,24 @@ EXPORT_SYMBOL_GPL(unregister_dock_notifi
  * @handle: the handle of the device
  * @ops: handlers to call after docking
  * @context: device specific data
+ * @init: Optional initialization routine to run after registration
+ * @release: Optional release routine to run on unregistration
  *
  * If a driver would like to perform a hotplug operation after a dock
  * event, they can register an acpi_notifiy_handler to be called by
  * the dock driver after _DCK is executed.
  */
-int
-register_hotplug_dock_device(acpi_handle handle, const struct acpi_dock_ops *ops,
-			     void *context)
+int register_hotplug_dock_device(acpi_handle handle,
+				 const struct acpi_dock_ops *ops, void *context,
+				 void (*init)(void *), void (*release)(void *))
 {
 	struct dock_dependent_device *dd;
 	struct dock_station *dock_station;
 	int ret = -EINVAL;
 
+	if (WARN_ON(!context))
+		return -EINVAL;
+
 	if (!dock_station_count)
 		return -ENODEV;
 
@@ -597,12 +655,8 @@ 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);
+		if (dd && !dock_init_hotplug(dd, ops, context, init, release))
 			ret = 0;
-		}
 	}
 
 	return ret;
@@ -624,7 +678,7 @@ void unregister_hotplug_dock_device(acpi
 	list_for_each_entry(dock_station, &dock_stations, sibling) {
 		dd = find_dock_dependent_device(dock_station, handle);
 		if (dd)
-			dock_del_hotplug_device(dock_station, dd);
+			dock_release_hotplug(dd);
 	}
 }
 EXPORT_SYMBOL_GPL(unregister_hotplug_dock_device);
@@ -953,7 +1007,6 @@ static int __init dock_add(acpi_handle h
 	mutex_init(&dock_station->hp_lock);
 	spin_lock_init(&dock_station->dd_lock);
 	INIT_LIST_HEAD(&dock_station->sibling);
-	INIT_LIST_HEAD(&dock_station->hotplug_devices);
 	ATOMIC_INIT_NOTIFIER_HEAD(&dock_notifier_list);
 	INIT_LIST_HEAD(&dock_station->dependent_devices);
 
Index: linux-pm/include/acpi/acpi_drivers.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_drivers.h
+++ linux-pm/include/acpi/acpi_drivers.h
@@ -123,7 +123,9 @@ extern int register_dock_notifier(struct
 extern void unregister_dock_notifier(struct notifier_block *nb);
 extern int register_hotplug_dock_device(acpi_handle handle,
 					const struct acpi_dock_ops *ops,
-					void *context);
+					void *context,
+					void (*init)(void *),
+					void (*release)(void *));
 extern void unregister_hotplug_dock_device(acpi_handle handle);
 #else
 static inline int is_dock_device(acpi_handle handle)
@@ -139,7 +141,9 @@ static inline void unregister_dock_notif
 }
 static inline int register_hotplug_dock_device(acpi_handle handle,
 					       const struct acpi_dock_ops *ops,
-					       void *context)
+					       void *context,
+					       void (*init)(void *),
+					       void (*release)(void *))
 {
 	return -ENODEV;
 }
Index: linux-pm/drivers/pci/hotplug/acpiphp_glue.c
===================================================================
--- linux-pm.orig/drivers/pci/hotplug/acpiphp_glue.c
+++ linux-pm/drivers/pci/hotplug/acpiphp_glue.c
@@ -61,6 +61,7 @@ static DEFINE_MUTEX(bridge_mutex);
 static void handle_hotplug_event_bridge (acpi_handle, u32, void *);
 static void acpiphp_sanitize_bus(struct pci_bus *bus);
 static void acpiphp_set_hpp_values(struct pci_bus *bus);
+static void hotplug_event_func(acpi_handle handle, u32 type, void *context);
 static void handle_hotplug_event_func(acpi_handle handle, u32 type, void *context);
 static void free_bridge(struct kref *kref);
 
@@ -147,7 +148,7 @@ static int post_dock_fixups(struct notif
 
 
 static const struct acpi_dock_ops acpiphp_dock_ops = {
-	.handler = handle_hotplug_event_func,
+	.handler = hotplug_event_func,
 };
 
 /* Check whether the PCI device is managed by native PCIe hotplug driver */
@@ -179,6 +180,20 @@ static bool device_is_managed_by_native_
 	return true;
 }
 
+static void acpiphp_dock_init(void *data)
+{
+	struct acpiphp_func *func = data;
+
+	get_bridge(func->slot->bridge);
+}
+
+static void acpiphp_dock_release(void *data)
+{
+	struct acpiphp_func *func = data;
+
+	put_bridge(func->slot->bridge);
+}
+
 /* callback routine to register each ACPI PCI slot object */
 static acpi_status
 register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
@@ -298,7 +313,8 @@ register_slot(acpi_handle handle, u32 lv
 		 */
 		newfunc->flags &= ~FUNC_HAS_EJ0;
 		if (register_hotplug_dock_device(handle,
-			&acpiphp_dock_ops, newfunc))
+			&acpiphp_dock_ops, newfunc,
+			acpiphp_dock_init, acpiphp_dock_release))
 			dbg("failed to register dock device\n");
 
 		/* we need to be notified when dock events happen
@@ -1068,22 +1084,12 @@ static void handle_hotplug_event_bridge(
 	alloc_acpi_hp_work(handle, type, context, _handle_hotplug_event_bridge);
 }
 
-static void _handle_hotplug_event_func(struct work_struct *work)
+static void hotplug_event_func(acpi_handle handle, u32 type, void *context)
 {
-	struct acpiphp_func *func;
+	struct acpiphp_func *func = context;
 	char objname[64];
 	struct acpi_buffer buffer = { .length = sizeof(objname),
 				      .pointer = objname };
-	struct acpi_hp_work *hp_work;
-	acpi_handle handle;
-	u32 type;
-
-	hp_work = container_of(work, struct acpi_hp_work, work);
-	handle = hp_work->handle;
-	type = hp_work->type;
-	func = (struct acpiphp_func *)hp_work->context;
-
-	acpi_scan_lock_acquire();
 
 	acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
 
@@ -1116,6 +1122,18 @@ static void _handle_hotplug_event_func(s
 		warn("notify_handler: unknown event type 0x%x for %s\n", type, objname);
 		break;
 	}
+}
+
+static void _handle_hotplug_event_func(struct work_struct *work)
+{
+	struct acpi_hp_work *hp_work;
+	struct acpiphp_func *func;
+
+	hp_work = container_of(work, struct acpi_hp_work, work);
+	func = hp_work->context;
+	acpi_scan_lock_acquire();
+
+	hotplug_event_func(hp_work->handle, hp_work->type, func);
 
 	acpi_scan_lock_release();
 	kfree(hp_work); /* allocated in handle_hotplug_event_func */

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

[Yinghai Lu]

On Sun, Jun 23, 2013 at 2:42 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> 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.

wonder if dock_release_hotplug with second dock_station and dd will
have problem.

as first one dock_station/dd, could have hp_context release already,
then second one could all release(context) again....

so looks like dock_release_hotplug should go over dock_station/dd list
to clear hp_context in other dock_station/... if they are the same?

Yinghai

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

[Rafael J. Wysocki]

On Sunday, June 23, 2013 04:04:52 PM Yinghai Lu wrote:
> On Sun, Jun 23, 2013 at 2:42 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > 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.
> 
> wonder if dock_release_hotplug with second dock_station and dd will
> have problem.
> 
> as first one dock_station/dd, could have hp_context release already,
> then second one could all release(context) again....
> 
> so looks like dock_release_hotplug should go over dock_station/dd list
> to clear hp_context in other dock_station/... if they are the same?

I'm not sure what you mean.  They are different dependent_device objects
and each of them has its own context pointer, although they both will point to
the same thing.

Both "init" and "release" will be called for each of them individually which
for for acpiphp (which is the only user of that ATM) actually means "get" and
"put", so it should be OK.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

[Yinghai Lu]

On Sun, Jun 23, 2013 at 5:40 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Sunday, June 23, 2013 04:04:52 PM Yinghai Lu wrote:
>> On Sun, Jun 23, 2013 at 2:42 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
...
>> > 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.
>>
>> wonder if dock_release_hotplug with second dock_station and dd will
>> have problem.
>>
>> as first one dock_station/dd, could have hp_context release already,
>> then second one could all release(context) again....
>>
>> so looks like dock_release_hotplug should go over dock_station/dd list
>> to clear hp_context in other dock_station/... if they are the same?
>
> I'm not sure what you mean.  They are different dependent_device objects
> and each of them has its own context pointer, although they both will point to
> the same thing.
>
> Both "init" and "release" will be called for each of them individually which
> for for acpiphp (which is the only user of that ATM) actually means "get" and
> "put", so it should be OK.

yes, then hp_context can never be  the same, just the acpi handle is the same.

Acked-by: Yinghai Lu <yinghai@kernel.org>

BTW, thank you very much for the whole acpi scan rework.

Yinghai

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

[Rafael J. Wysocki]

On Sunday, June 23, 2013 09:34:09 PM Yinghai Lu wrote:
> On Sun, Jun 23, 2013 at 5:40 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Sunday, June 23, 2013 04:04:52 PM Yinghai Lu wrote:
> >> On Sun, Jun 23, 2013 at 2:42 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> ...
> >> > 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.
> >>
> >> wonder if dock_release_hotplug with second dock_station and dd will
> >> have problem.
> >>
> >> as first one dock_station/dd, could have hp_context release already,
> >> then second one could all release(context) again....
> >>
> >> so looks like dock_release_hotplug should go over dock_station/dd list
> >> to clear hp_context in other dock_station/... if they are the same?
> >
> > I'm not sure what you mean.  They are different dependent_device objects
> > and each of them has its own context pointer, although they both will point to
> > the same thing.
> >
> > Both "init" and "release" will be called for each of them individually which
> > for for acpiphp (which is the only user of that ATM) actually means "get" and
> > "put", so it should be OK.
> 
> yes, then hp_context can never be  the same, just the acpi handle is the same.
> 
> Acked-by: Yinghai Lu <yinghai@kernel.org>

Thanks!

> BTW, thank you very much for the whole acpi scan rework.

Well, no problem, it was necessary for a number of reasons.

And honestly I think more along those lines is still needed. :-)

For example, the discussion here shows how fragile the design of acpiphp is.
Take hotplug_dock_devices() for instance.  It shouldn't even need to use those
"handlers", because ideally acpi_bus_trim() should automatically trigger the
removal of "physical" device objects depending on the stuff being trimmed.
And analogously for acpi_bus_scan().

The "trim" part should be possible to implement even now, because
struct acpi_device contains a "remove" callback pointer (that was added for
power resources IIRC), although perhaps it'll need to be called from
acpi_bus_device_detach().  The "scan" part should be doable too if we add
an "add child" callback to struct acpi_device, so that acpi_bus_device_attach()
can use it to handle devices that don't have scan handlers or ACPI drivers
(like PCI devices).

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.