[BUGFIX v2 1/4] ACPI, DOCK: initialize dock subsystem before scanning PCI root buses

[BUGFIX v2 1/4] ACPI, DOCK: initialize dock subsystem before scanning PCI root buses

[Jiang Liu]

Changeset "3b63aaa70e1 PCI: acpiphp: Do not use ACPI PCI subdriver
mechanism" causes a regression which breaks ACPI dock support,
please refer to https://bugzilla.kernel.org/show_bug.cgi?id=59501

The root cause is that changeset 3b63aaa70e1 changed the relative
initialization order of ACPI dock subsystem and acpiphp driver,
and acpiphp driver has dependency on ACPI dock subsystem's
initialization result, so that acpiphp can't correctly detect ACPI
dock stations now.

On the other hand, ACPI dock is a built-in driver, so we could
explicitly initialize it before the acpiphp driver is used.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Reported-by: Alexander E. Patrakov <patrakov@gmail.com>
Tested-by: Alexander E. Patrakov <patrakov@gmail.com>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
Cc: linux-acpi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: <stable@vger.kernel.org> # 3.9+
---
 drivers/acpi/dock.c     | 7 +------
 drivers/acpi/internal.h | 5 +++++
 drivers/acpi/scan.c     | 1 +
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
index 4fdea38..02b0563 100644
--- a/drivers/acpi/dock.c
+++ b/drivers/acpi/dock.c
@@ -1033,7 +1033,7 @@ find_dock_and_bay(acpi_handle handle, u32 lvl, void *context, void **rv)
 	return AE_OK;
 }
 
-static int __init dock_init(void)
+int __init acpi_dock_init(void)
 {
 	if (acpi_disabled)
 		return 0;
@@ -1062,9 +1062,4 @@ static void __exit dock_exit(void)
 		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);
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 297cbf4..c610a76 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -40,6 +40,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
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 44225cb..4148163 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -2045,6 +2045,7 @@ int __init acpi_scan_init(void)
 	acpi_lpss_init();
 	acpi_container_init();
 	acpi_memory_hotplug_init();
+	acpi_dock_init();
 
 	mutex_lock(&acpi_scan_lock);
 	/*
-- 
1.8.1.2

Re: [BUGFIX v2 1/4] ACPI, DOCK: initialize dock subsystem before scanning PCI root buses

[Yinghai Lu]

On Fri, Jun 14, 2013 at 12:27 PM, Jiang Liu <jiang.liu@huawei.com> wrote:
> Changeset "3b63aaa70e1 PCI: acpiphp: Do not use ACPI PCI subdriver
> mechanism" causes a regression which breaks ACPI dock support,
> please refer to https://bugzilla.kernel.org/show_bug.cgi?id=59501
>
> The root cause is that changeset 3b63aaa70e1 changed the relative
> initialization order of ACPI dock subsystem and acpiphp driver,
> and acpiphp driver has dependency on ACPI dock subsystem's
> initialization result, so that acpiphp can't correctly detect ACPI
> dock stations now.
>
> On the other hand, ACPI dock is a built-in driver, so we could
> explicitly initialize it before the acpiphp driver is used.
>
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> Reported-by: Alexander E. Patrakov <patrakov@gmail.com>
> Tested-by: Alexander E. Patrakov <patrakov@gmail.com>
> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> Cc: linux-acpi@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: <stable@vger.kernel.org> # 3.9+
> ---
>  drivers/acpi/dock.c     | 7 +------
>  drivers/acpi/internal.h | 5 +++++
>  drivers/acpi/scan.c     | 1 +
>  3 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
> index 4fdea38..02b0563 100644
> --- a/drivers/acpi/dock.c
> +++ b/drivers/acpi/dock.c
> @@ -1033,7 +1033,7 @@ find_dock_and_bay(acpi_handle handle, u32 lvl, void *context, void **rv)
>         return AE_OK;
>  }
>
> -static int __init dock_init(void)
> +int __init acpi_dock_init(void)
>  {
>         if (acpi_disabled)
>                 return 0;
> @@ -1062,9 +1062,4 @@ static void __exit dock_exit(void)
>                 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);

why not remove dock_exit?

> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> index 297cbf4..c610a76 100644
> --- a/drivers/acpi/internal.h
> +++ b/drivers/acpi/internal.h
> @@ -40,6 +40,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
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 44225cb..4148163 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -2045,6 +2045,7 @@ int __init acpi_scan_init(void)
>         acpi_lpss_init();
>         acpi_container_init();
>         acpi_memory_hotplug_init();
> +       acpi_dock_init();
>
>         mutex_lock(&acpi_scan_lock);
>         /*
> --
> 1.8.1.2
>

Re: [BUGFIX v2 1/4] ACPI, DOCK: initialize dock subsystem before scanning PCI root buses

[Jiang Liu]

On 06/15/2013 02:51 PM, Yinghai Lu wrote:
> On Fri, Jun 14, 2013 at 12:27 PM, Jiang Liu <jiang.liu@huawei.com> wrote:
>> Changeset "3b63aaa70e1 PCI: acpiphp: Do not use ACPI PCI subdriver
>> mechanism" causes a regression which breaks ACPI dock support,
>> please refer to https://bugzilla.kernel.org/show_bug.cgi?id=59501
>>
>> The root cause is that changeset 3b63aaa70e1 changed the relative
>> initialization order of ACPI dock subsystem and acpiphp driver,
>> and acpiphp driver has dependency on ACPI dock subsystem's
>> initialization result, so that acpiphp can't correctly detect ACPI
>> dock stations now.
>>
>> On the other hand, ACPI dock is a built-in driver, so we could
>> explicitly initialize it before the acpiphp driver is used.
>>
>> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
>> Reported-by: Alexander E. Patrakov <patrakov@gmail.com>
>> Tested-by: Alexander E. Patrakov <patrakov@gmail.com>
>> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
>> Cc: linux-acpi@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Cc: <stable@vger.kernel.org> # 3.9+
>> ---
>>  drivers/acpi/dock.c     | 7 +------
>>  drivers/acpi/internal.h | 5 +++++
>>  drivers/acpi/scan.c     | 1 +
>>  3 files changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
>> index 4fdea38..02b0563 100644
>> --- a/drivers/acpi/dock.c
>> +++ b/drivers/acpi/dock.c
>> @@ -1033,7 +1033,7 @@ find_dock_and_bay(acpi_handle handle, u32 lvl, void *context, void **rv)
>>         return AE_OK;
>>  }
>>
>> -static int __init dock_init(void)
>> +int __init acpi_dock_init(void)
>>  {
>>         if (acpi_disabled)
>>                 return 0;
>> @@ -1062,9 +1062,4 @@ static void __exit dock_exit(void)
>>                 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);
> 
> why not remove dock_exit?
I have a pending patchset to clean up dock driver, Rafael suggested to
focus on bugfix first, so I will send out the clean up patchset later.

> 
>> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
>> index 297cbf4..c610a76 100644
>> --- a/drivers/acpi/internal.h
>> +++ b/drivers/acpi/internal.h
>> @@ -40,6 +40,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
>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>> index 44225cb..4148163 100644
>> --- a/drivers/acpi/scan.c
>> +++ b/drivers/acpi/scan.c
>> @@ -2045,6 +2045,7 @@ int __init acpi_scan_init(void)
>>         acpi_lpss_init();
>>         acpi_container_init();
>>         acpi_memory_hotplug_init();
>> +       acpi_dock_init();
>>
>>         mutex_lock(&acpi_scan_lock);
>>         /*
>> --
>> 1.8.1.2
>>

Re: [BUGFIX v2 1/4] ACPI, DOCK: initialize dock subsystem before scanning PCI root buses

[Rafael J. Wysocki]

On Saturday, June 15, 2013 06:05:31 PM Jiang Liu wrote:
> On 06/15/2013 02:51 PM, Yinghai Lu wrote:
> > On Fri, Jun 14, 2013 at 12:27 PM, Jiang Liu <jiang.liu@huawei.com> wrote:
> >> Changeset "3b63aaa70e1 PCI: acpiphp: Do not use ACPI PCI subdriver
> >> mechanism" causes a regression which breaks ACPI dock support,
> >> please refer to https://bugzilla.kernel.org/show_bug.cgi?id=59501
> >>
> >> The root cause is that changeset 3b63aaa70e1 changed the relative
> >> initialization order of ACPI dock subsystem and acpiphp driver,
> >> and acpiphp driver has dependency on ACPI dock subsystem's
> >> initialization result, so that acpiphp can't correctly detect ACPI
> >> dock stations now.
> >>
> >> On the other hand, ACPI dock is a built-in driver, so we could
> >> explicitly initialize it before the acpiphp driver is used.
> >>
> >> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> >> Reported-by: Alexander E. Patrakov <patrakov@gmail.com>
> >> Tested-by: Alexander E. Patrakov <patrakov@gmail.com>
> >> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> >> Cc: linux-acpi@vger.kernel.org
> >> Cc: linux-kernel@vger.kernel.org
> >> Cc: <stable@vger.kernel.org> # 3.9+
> >> ---
> >>  drivers/acpi/dock.c     | 7 +------
> >>  drivers/acpi/internal.h | 5 +++++
> >>  drivers/acpi/scan.c     | 1 +
> >>  3 files changed, 7 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
> >> index 4fdea38..02b0563 100644
> >> --- a/drivers/acpi/dock.c
> >> +++ b/drivers/acpi/dock.c
> >> @@ -1033,7 +1033,7 @@ find_dock_and_bay(acpi_handle handle, u32 lvl, void *context, void **rv)
> >>         return AE_OK;
> >>  }
> >>
> >> -static int __init dock_init(void)
> >> +int __init acpi_dock_init(void)
> >>  {
> >>         if (acpi_disabled)
> >>                 return 0;
> >> @@ -1062,9 +1062,4 @@ static void __exit dock_exit(void)
> >>                 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);
> > 
> > why not remove dock_exit?
> I have a pending patchset to clean up dock driver, Rafael suggested to
> focus on bugfix first, so I will send out the clean up patchset later.

Well, you can remove dock_exit() in the same patch.  This is kind of not
directly related to the fix, but since you're changing it from modular to
non-modular, it'd be prudent to remove all of the "modular" code in the
process.

Thanks,
Rafael


> >> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> >> index 297cbf4..c610a76 100644
> >> --- a/drivers/acpi/internal.h
> >> +++ b/drivers/acpi/internal.h
> >> @@ -40,6 +40,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
> >> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> >> index 44225cb..4148163 100644
> >> --- a/drivers/acpi/scan.c
> >> +++ b/drivers/acpi/scan.c
> >> @@ -2045,6 +2045,7 @@ int __init acpi_scan_init(void)
> >>         acpi_lpss_init();
> >>         acpi_container_init();
> >>         acpi_memory_hotplug_init();
> >> +       acpi_dock_init();
> >>
> >>         mutex_lock(&acpi_scan_lock);
> >>         /*
> >> --
> >> 1.8.1.2
> >>
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[BUGFIX v2 2/4] ACPI, DOCK: resolve possible deadlock scenarios

[Jiang Liu]

This is a preparation for next patch to avoid breaking bisecting.
If next patch is applied without this one, it will cause deadlock
as below:

Case 1:
[   31.015593]  Possible unsafe locking scenario:

[   31.018350]        CPU0                    CPU1
[   31.019691]        ----                    ----
[   31.021002]   lock(&dock_station->hp_lock);
[   31.022327]                                lock(&slot->crit_sect);
[   31.023650]                                lock(&dock_station->hp_lock);
[   31.025010]   lock(&slot->crit_sect);
[   31.026342]

Case 2:
        hotplug_dock_devices()
                mutex_lock(&ds->hp_lock)
                        dd->ops->handler()
                                register_hotplug_dock_device()
                                        mutex_lock(&ds->hp_lock)
[   34.316570] [ INFO: possible recursive locking detected ]
[   34.316573] 3.10.0-rc4 #6 Tainted: G         C
[   34.316575] ---------------------------------------------
[   34.316577] kworker/0:0/4 is trying to acquire lock:
[   34.316579]  (&dock_station->hp_lock){+.+.+.}, at:
[<ffffffff813c766b>] register_hotplug_dock_device+0x6a/0xbf
[   34.316588]
but task is already holding lock:
[   34.316590]  (&dock_station->hp_lock){+.+.+.}, at:
[<ffffffff813c7270>] hotplug_dock_devices+0x2c/0xda
[   34.316595]
other info that might help us debug this:
[   34.316597]  Possible unsafe locking scenario:

[   34.316599]        CPU0
[   34.316601]        ----
[   34.316602]   lock(&dock_station->hp_lock);
[   34.316605]   lock(&dock_station->hp_lock);
[   34.316608]
 *** DEADLOCK ***

So fix this deadlock by not taking ds->hp_lock in function
register_hotplug_dock_device(). This patch also fixes a possible
race conditions in function dock_event() because previously it
accesses ds->hotplug_devices list without holding ds->hp_lock.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Cc: Len Brown <lenb@kernel.org>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: Yijing Wang <wangyijing@huawei.com>
Cc: linux-acpi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-pci@vger.kernel.org
Cc: <stable@vger.kernel.org> # 3.8+
---
 drivers/acpi/dock.c                | 109 ++++++++++++++++++++++---------------
 drivers/pci/hotplug/acpiphp_glue.c |  15 +++++
 include/acpi/acpi_drivers.h        |   2 +
 3 files changed, 82 insertions(+), 44 deletions(-)

diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
index 02b0563..602bce5 100644
--- a/drivers/acpi/dock.c
+++ b/drivers/acpi/dock.c
@@ -66,7 +66,7 @@ struct dock_station {
 	spinlock_t dd_lock;
 	struct mutex hp_lock;
 	struct list_head dependent_devices;
-	struct list_head hotplug_devices;
+	struct klist hotplug_devices;
 
 	struct list_head sibling;
 	struct platform_device *dock_device;
@@ -76,12 +76,18 @@ static int dock_station_count;
 
 struct dock_dependent_device {
 	struct list_head list;
-	struct list_head hotplug_list;
+	acpi_handle handle;
+};
+
+struct dock_hotplug_info {
+	struct klist_node node;
 	acpi_handle handle;
 	const struct acpi_dock_ops *ops;
 	void *context;
 };
 
+#define node_to_info(n)	container_of((n), struct dock_hotplug_info, node)
+
 #define DOCK_DOCKING	0x00000001
 #define DOCK_UNDOCKING  0x00000002
 #define DOCK_IS_DOCK	0x00000010
@@ -111,7 +117,6 @@ add_dock_dependent_device(struct dock_station *ds, acpi_handle handle)
 
 	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);
@@ -120,36 +125,19 @@ add_dock_dependent_device(struct dock_station *ds, acpi_handle handle)
 	return 0;
 }
 
-/**
- * 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)
+static void hotplug_info_get(struct klist_node *node)
 {
-	mutex_lock(&ds->hp_lock);
-	list_add_tail(&dd->hotplug_list, &ds->hotplug_devices);
-	mutex_unlock(&ds->hp_lock);
+	struct dock_hotplug_info *info = node_to_info(node);
+
+	info->ops->get(info->context);
 }
 
-/**
- * dock_del_hotplug_device - remove a hotplug handler from the dock station
- * @ds: The dock station
- * @dd: the dependent device struct
- *
- * Delete the dependent device from the dock's hotplug device list
- */
-static void
-dock_del_hotplug_device(struct dock_station *ds,
-			struct dock_dependent_device *dd)
+static void hotplug_info_put(struct klist_node *node)
 {
-	mutex_lock(&ds->hp_lock);
-	list_del(&dd->hotplug_list);
-	mutex_unlock(&ds->hp_lock);
+	struct dock_hotplug_info *info = node_to_info(node);
+
+	info->ops->put(info->context);
+	kfree(info);
 }
 
 /**
@@ -354,15 +342,22 @@ static void dock_remove_acpi_device(acpi_handle handle)
 static void hotplug_dock_devices(struct dock_station *ds, u32 event)
 {
 	struct dock_dependent_device *dd;
+	struct klist_iter iter;
+	struct klist_node *node;
+	struct dock_hotplug_info *info;
 
 	mutex_lock(&ds->hp_lock);
 
 	/*
 	 * 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);
+	klist_iter_init(&ds->hotplug_devices, &iter);
+	while ((node = klist_next(&iter))) {
+		info = node_to_info(node);
+		if (info->ops && info->ops->handler)
+			info->ops->handler(info->handle, event, info->context);
+	}
+	klist_iter_exit(&iter);
 
 	/*
 	 * Now make sure that an acpi_device is created for each
@@ -384,7 +379,9 @@ static void dock_event(struct dock_station *ds, u32 event, int num)
 	struct device *dev = &ds->dock_device->dev;
 	char event_string[13];
 	char *envp[] = { event_string, NULL };
-	struct dock_dependent_device *dd;
+	struct klist_iter iter;
+	struct klist_node *node;
+	struct dock_hotplug_info *info;
 
 	if (num == UNDOCK_EVENT)
 		sprintf(event_string, "EVENT=undock");
@@ -398,9 +395,13 @@ static void dock_event(struct dock_station *ds, u32 event, int num)
 	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);
+	klist_iter_init(&ds->hotplug_devices, &iter);
+	while ((node = klist_next(&iter))) {
+		info = node_to_info(node);
+		if (info->ops && info->ops->handler)
+			info->ops->handler(info->handle, event, info->context);
+	}
+	klist_iter_exit(&iter);
 
 	if (num != DOCK_EVENT)
 		kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
@@ -580,12 +581,16 @@ register_hotplug_dock_device(acpi_handle handle, const struct acpi_dock_ops *ops
 			     void *context)
 {
 	struct dock_dependent_device *dd;
+	struct dock_hotplug_info *info;
 	struct dock_station *dock_station;
 	int ret = -EINVAL;
 
 	if (!dock_station_count)
 		return -ENODEV;
 
+	if (ops == NULL || ops->get == NULL || ops->put == NULL)
+		return -EINVAL;
+
 	/*
 	 * make sure this handle is for a device dependent on the dock,
 	 * this would include the dock station itself
@@ -598,9 +603,18 @@ register_hotplug_dock_device(acpi_handle handle, const struct acpi_dock_ops *ops
 		 */
 		dd = find_dock_dependent_device(dock_station, handle);
 		if (dd) {
-			dd->ops = ops;
-			dd->context = context;
-			dock_add_hotplug_device(dock_station, dd);
+			info = kzalloc(sizeof(*info), GFP_KERNEL);
+			if (!info) {
+				unregister_hotplug_dock_device(handle);
+				ret = -ENOMEM;
+				break;
+			}
+
+			info->ops = ops;
+			info->context = context;
+			info->handle = dd->handle;
+			klist_add_tail(&info->node,
+				       &dock_station->hotplug_devices);
 			ret = 0;
 		}
 	}
@@ -615,16 +629,22 @@ EXPORT_SYMBOL_GPL(register_hotplug_dock_device);
  */
 void unregister_hotplug_dock_device(acpi_handle handle)
 {
-	struct dock_dependent_device *dd;
 	struct dock_station *dock_station;
+	struct klist_iter iter;
+	struct klist_node *node;
+	struct dock_hotplug_info *info;
 
 	if (!dock_station_count)
 		return;
 
 	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);
+		klist_iter_init(&dock_station->hotplug_devices, &iter);
+		while ((node = klist_next(&iter))) {
+			info = node_to_info(node);
+			if (info->handle == handle)
+				klist_del(&info->node);
+		}
+		klist_iter_exit(&iter);
 	}
 }
 EXPORT_SYMBOL_GPL(unregister_hotplug_dock_device);
@@ -951,7 +971,8 @@ static int __init dock_add(acpi_handle handle)
 	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);
+	klist_init(&dock_station->hotplug_devices,
+		   hotplug_info_get, hotplug_info_put);
 	ATOMIC_INIT_NOTIFIER_HEAD(&dock_notifier_list);
 	INIT_LIST_HEAD(&dock_station->dependent_devices);
 
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 716aa93..5d696f5 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -145,9 +145,24 @@ static int post_dock_fixups(struct notifier_block *nb, unsigned long val,
 	return NOTIFY_OK;
 }
 
+static void acpiphp_dock_get(void *data)
+{
+	struct acpiphp_func *func = data;
+
+	get_bridge(func->slot->bridge);
+}
+
+static void acpiphp_dock_put(void *data)
+{
+	struct acpiphp_func *func = data;
+
+	put_bridge(func->slot->bridge);
+}
 
 static const struct acpi_dock_ops acpiphp_dock_ops = {
 	.handler = handle_hotplug_event_func,
+	.get = acpiphp_dock_get,
+	.put = acpiphp_dock_put,
 };
 
 /* Check whether the PCI device is managed by native PCIe hotplug driver */
diff --git a/include/acpi/acpi_drivers.h b/include/acpi/acpi_drivers.h
index e6168a2..8fcc9ac 100644
--- a/include/acpi/acpi_drivers.h
+++ b/include/acpi/acpi_drivers.h
@@ -115,6 +115,8 @@ void pci_acpi_crs_quirks(void);
 struct acpi_dock_ops {
 	acpi_notify_handler handler;
 	acpi_notify_handler uevent;
+	void (*get)(void *data);
+	void (*put)(void *data);
 };
 
 #if defined(CONFIG_ACPI_DOCK) || defined(CONFIG_ACPI_DOCK_MODULE)
-- 
1.8.1.2

Re: [BUGFIX v2 2/4] ACPI, DOCK: resolve possible deadlock scenarios

[Rafael J. Wysocki]

On Saturday, June 15, 2013 03:27:59 AM Jiang Liu wrote:
> This is a preparation for next patch to avoid breaking bisecting.
> If next patch is applied without this one, it will cause deadlock
> as below:
> 
> Case 1:
> [   31.015593]  Possible unsafe locking scenario:
> 
> [   31.018350]        CPU0                    CPU1
> [   31.019691]        ----                    ----
> [   31.021002]   lock(&dock_station->hp_lock);
> [   31.022327]                                lock(&slot->crit_sect);
> [   31.023650]                                lock(&dock_station->hp_lock);
> [   31.025010]   lock(&slot->crit_sect);
> [   31.026342]
> 
> Case 2:
>         hotplug_dock_devices()
>                 mutex_lock(&ds->hp_lock)
>                         dd->ops->handler()
>                                 register_hotplug_dock_device()
>                                         mutex_lock(&ds->hp_lock)
> [   34.316570] [ INFO: possible recursive locking detected ]
> [   34.316573] 3.10.0-rc4 #6 Tainted: G         C
> [   34.316575] ---------------------------------------------
> [   34.316577] kworker/0:0/4 is trying to acquire lock:
> [   34.316579]  (&dock_station->hp_lock){+.+.+.}, at:
> [<ffffffff813c766b>] register_hotplug_dock_device+0x6a/0xbf
> [   34.316588]
> but task is already holding lock:
> [   34.316590]  (&dock_station->hp_lock){+.+.+.}, at:
> [<ffffffff813c7270>] hotplug_dock_devices+0x2c/0xda
> [   34.316595]
> other info that might help us debug this:
> [   34.316597]  Possible unsafe locking scenario:
> 
> [   34.316599]        CPU0
> [   34.316601]        ----
> [   34.316602]   lock(&dock_station->hp_lock);
> [   34.316605]   lock(&dock_station->hp_lock);
> [   34.316608]
>  *** DEADLOCK ***
> 
> So fix this deadlock by not taking ds->hp_lock in function
> register_hotplug_dock_device(). This patch also fixes a possible
> race conditions in function dock_event() because previously it
> accesses ds->hotplug_devices list without holding ds->hp_lock.
> 
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> Cc: Len Brown <lenb@kernel.org>
> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Yinghai Lu <yinghai@kernel.org>
> Cc: Yijing Wang <wangyijing@huawei.com>
> Cc: linux-acpi@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-pci@vger.kernel.org
> Cc: <stable@vger.kernel.org> # 3.8+
> ---
>  drivers/acpi/dock.c                | 109 ++++++++++++++++++++++---------------
>  drivers/pci/hotplug/acpiphp_glue.c |  15 +++++
>  include/acpi/acpi_drivers.h        |   2 +
>  3 files changed, 82 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
> index 02b0563..602bce5 100644
> --- a/drivers/acpi/dock.c
> +++ b/drivers/acpi/dock.c
> @@ -66,7 +66,7 @@ struct dock_station {
>  	spinlock_t dd_lock;
>  	struct mutex hp_lock;
>  	struct list_head dependent_devices;
> -	struct list_head hotplug_devices;
> +	struct klist hotplug_devices;
>  
>  	struct list_head sibling;
>  	struct platform_device *dock_device;
> @@ -76,12 +76,18 @@ static int dock_station_count;
>  
>  struct dock_dependent_device {
>  	struct list_head list;
> -	struct list_head hotplug_list;
> +	acpi_handle handle;
> +};
> +
> +struct dock_hotplug_info {
> +	struct klist_node node;
>  	acpi_handle handle;
>  	const struct acpi_dock_ops *ops;
>  	void *context;
>  };

Can we please relax a bit and possibly take a step back?

So since your last reply to me wasn't particularly helpful, I went through the
code in dock.c and acpiphp_glue.c and I simply think that the whole
hotplug_list thing is simply redundant.

It looks like instead of using it (or the klist in this patch), we can add a
"hotlpug_device" flag to dock_dependent_device and set that flag instead of
adding dd to hotplug_devices or clear it instead of removing dd from that list.

That would allow us to avoid the deadlock, because we wouldn't need the hp_lock
any more and perhaps we could make the code simpler instead of making it more
complex.

How does that sound?

Rafael


> +#define node_to_info(n)	container_of((n), struct dock_hotplug_info, node)
> +
>  #define DOCK_DOCKING	0x00000001
>  #define DOCK_UNDOCKING  0x00000002
>  #define DOCK_IS_DOCK	0x00000010
> @@ -111,7 +117,6 @@ add_dock_dependent_device(struct dock_station *ds, acpi_handle handle)
>  
>  	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);
> @@ -120,36 +125,19 @@ add_dock_dependent_device(struct dock_station *ds, acpi_handle handle)
>  	return 0;
>  }
>  
> -/**
> - * 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)
> +static void hotplug_info_get(struct klist_node *node)
>  {
> -	mutex_lock(&ds->hp_lock);
> -	list_add_tail(&dd->hotplug_list, &ds->hotplug_devices);
> -	mutex_unlock(&ds->hp_lock);
> +	struct dock_hotplug_info *info = node_to_info(node);
> +
> +	info->ops->get(info->context);
>  }
>  
> -/**
> - * dock_del_hotplug_device - remove a hotplug handler from the dock station
> - * @ds: The dock station
> - * @dd: the dependent device struct
> - *
> - * Delete the dependent device from the dock's hotplug device list
> - */
> -static void
> -dock_del_hotplug_device(struct dock_station *ds,
> -			struct dock_dependent_device *dd)
> +static void hotplug_info_put(struct klist_node *node)
>  {
> -	mutex_lock(&ds->hp_lock);
> -	list_del(&dd->hotplug_list);
> -	mutex_unlock(&ds->hp_lock);
> +	struct dock_hotplug_info *info = node_to_info(node);
> +
> +	info->ops->put(info->context);
> +	kfree(info);
>  }
>  
>  /**
> @@ -354,15 +342,22 @@ static void dock_remove_acpi_device(acpi_handle handle)
>  static void hotplug_dock_devices(struct dock_station *ds, u32 event)
>  {
>  	struct dock_dependent_device *dd;
> +	struct klist_iter iter;
> +	struct klist_node *node;
> +	struct dock_hotplug_info *info;
>  
>  	mutex_lock(&ds->hp_lock);
>  
>  	/*
>  	 * 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);
> +	klist_iter_init(&ds->hotplug_devices, &iter);
> +	while ((node = klist_next(&iter))) {
> +		info = node_to_info(node);
> +		if (info->ops && info->ops->handler)
> +			info->ops->handler(info->handle, event, info->context);
> +	}
> +	klist_iter_exit(&iter);
>  
>  	/*
>  	 * Now make sure that an acpi_device is created for each
> @@ -384,7 +379,9 @@ static void dock_event(struct dock_station *ds, u32 event, int num)
>  	struct device *dev = &ds->dock_device->dev;
>  	char event_string[13];
>  	char *envp[] = { event_string, NULL };
> -	struct dock_dependent_device *dd;
> +	struct klist_iter iter;
> +	struct klist_node *node;
> +	struct dock_hotplug_info *info;
>  
>  	if (num == UNDOCK_EVENT)
>  		sprintf(event_string, "EVENT=undock");
> @@ -398,9 +395,13 @@ static void dock_event(struct dock_station *ds, u32 event, int num)
>  	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);
> +	klist_iter_init(&ds->hotplug_devices, &iter);
> +	while ((node = klist_next(&iter))) {
> +		info = node_to_info(node);
> +		if (info->ops && info->ops->handler)
> +			info->ops->handler(info->handle, event, info->context);
> +	}
> +	klist_iter_exit(&iter);
>  
>  	if (num != DOCK_EVENT)
>  		kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
> @@ -580,12 +581,16 @@ register_hotplug_dock_device(acpi_handle handle, const struct acpi_dock_ops *ops
>  			     void *context)
>  {
>  	struct dock_dependent_device *dd;
> +	struct dock_hotplug_info *info;
>  	struct dock_station *dock_station;
>  	int ret = -EINVAL;
>  
>  	if (!dock_station_count)
>  		return -ENODEV;
>  
> +	if (ops == NULL || ops->get == NULL || ops->put == NULL)
> +		return -EINVAL;
> +
>  	/*
>  	 * make sure this handle is for a device dependent on the dock,
>  	 * this would include the dock station itself
> @@ -598,9 +603,18 @@ register_hotplug_dock_device(acpi_handle handle, const struct acpi_dock_ops *ops
>  		 */
>  		dd = find_dock_dependent_device(dock_station, handle);
>  		if (dd) {
> -			dd->ops = ops;
> -			dd->context = context;
> -			dock_add_hotplug_device(dock_station, dd);
> +			info = kzalloc(sizeof(*info), GFP_KERNEL);
> +			if (!info) {
> +				unregister_hotplug_dock_device(handle);
> +				ret = -ENOMEM;
> +				break;
> +			}
> +
> +			info->ops = ops;
> +			info->context = context;
> +			info->handle = dd->handle;
> +			klist_add_tail(&info->node,
> +				       &dock_station->hotplug_devices);
>  			ret = 0;
>  		}
>  	}
> @@ -615,16 +629,22 @@ EXPORT_SYMBOL_GPL(register_hotplug_dock_device);
>   */
>  void unregister_hotplug_dock_device(acpi_handle handle)
>  {
> -	struct dock_dependent_device *dd;
>  	struct dock_station *dock_station;
> +	struct klist_iter iter;
> +	struct klist_node *node;
> +	struct dock_hotplug_info *info;
>  
>  	if (!dock_station_count)
>  		return;
>  
>  	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);
> +		klist_iter_init(&dock_station->hotplug_devices, &iter);
> +		while ((node = klist_next(&iter))) {
> +			info = node_to_info(node);
> +			if (info->handle == handle)
> +				klist_del(&info->node);
> +		}
> +		klist_iter_exit(&iter);
>  	}
>  }
>  EXPORT_SYMBOL_GPL(unregister_hotplug_dock_device);
> @@ -951,7 +971,8 @@ static int __init dock_add(acpi_handle handle)
>  	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);
> +	klist_init(&dock_station->hotplug_devices,
> +		   hotplug_info_get, hotplug_info_put);
>  	ATOMIC_INIT_NOTIFIER_HEAD(&dock_notifier_list);
>  	INIT_LIST_HEAD(&dock_station->dependent_devices);
>  
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index 716aa93..5d696f5 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -145,9 +145,24 @@ static int post_dock_fixups(struct notifier_block *nb, unsigned long val,
>  	return NOTIFY_OK;
>  }
>  
> +static void acpiphp_dock_get(void *data)
> +{
> +	struct acpiphp_func *func = data;
> +
> +	get_bridge(func->slot->bridge);
> +}
> +
> +static void acpiphp_dock_put(void *data)
> +{
> +	struct acpiphp_func *func = data;
> +
> +	put_bridge(func->slot->bridge);
> +}
>  
>  static const struct acpi_dock_ops acpiphp_dock_ops = {
>  	.handler = handle_hotplug_event_func,
> +	.get = acpiphp_dock_get,
> +	.put = acpiphp_dock_put,
>  };
>  
>  /* Check whether the PCI device is managed by native PCIe hotplug driver */
> diff --git a/include/acpi/acpi_drivers.h b/include/acpi/acpi_drivers.h
> index e6168a2..8fcc9ac 100644
> --- a/include/acpi/acpi_drivers.h
> +++ b/include/acpi/acpi_drivers.h
> @@ -115,6 +115,8 @@ void pci_acpi_crs_quirks(void);
>  struct acpi_dock_ops {
>  	acpi_notify_handler handler;
>  	acpi_notify_handler uevent;
> +	void (*get)(void *data);
> +	void (*put)(void *data);
>  };
>  
>  #if defined(CONFIG_ACPI_DOCK) || defined(CONFIG_ACPI_DOCK_MODULE)
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [BUGFIX v2 2/4] ACPI, DOCK: resolve possible deadlock scenarios

[Jiang Liu]

On Sat 15 Jun 2013 06:21:02 AM CST, Rafael J. Wysocki wrote:
> On Saturday, June 15, 2013 03:27:59 AM Jiang Liu wrote:
>> This is a preparation for next patch to avoid breaking bisecting.
>> If next patch is applied without this one, it will cause deadlock
>> as below:
>>
>> Case 1:
>> [   31.015593]  Possible unsafe locking scenario:
>>
>> [   31.018350]        CPU0                    CPU1
>> [   31.019691]        ----                    ----
>> [   31.021002]   lock(&dock_station->hp_lock);
>> [   31.022327]                                lock(&slot->crit_sect);
>> [   31.023650]                                lock(&dock_station->hp_lock);
>> [   31.025010]   lock(&slot->crit_sect);
>> [   31.026342]
>>
>> Case 2:
>>         hotplug_dock_devices()
>>                 mutex_lock(&ds->hp_lock)
>>                         dd->ops->handler()
>>                                 register_hotplug_dock_device()
>>                                         mutex_lock(&ds->hp_lock)
>> [   34.316570] [ INFO: possible recursive locking detected ]
>> [   34.316573] 3.10.0-rc4 #6 Tainted: G         C
>> [   34.316575] ---------------------------------------------
>> [   34.316577] kworker/0:0/4 is trying to acquire lock:
>> [   34.316579]  (&dock_station->hp_lock){+.+.+.}, at:
>> [<ffffffff813c766b>] register_hotplug_dock_device+0x6a/0xbf
>> [   34.316588]
>> but task is already holding lock:
>> [   34.316590]  (&dock_station->hp_lock){+.+.+.}, at:
>> [<ffffffff813c7270>] hotplug_dock_devices+0x2c/0xda
>> [   34.316595]
>> other info that might help us debug this:
>> [   34.316597]  Possible unsafe locking scenario:
>>
>> [   34.316599]        CPU0
>> [   34.316601]        ----
>> [   34.316602]   lock(&dock_station->hp_lock);
>> [   34.316605]   lock(&dock_station->hp_lock);
>> [   34.316608]
>>  *** DEADLOCK ***
>>
>> So fix this deadlock by not taking ds->hp_lock in function
>> register_hotplug_dock_device(). This patch also fixes a possible
>> race conditions in function dock_event() because previously it
>> accesses ds->hotplug_devices list without holding ds->hp_lock.
>>
>> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
>> Cc: Len Brown <lenb@kernel.org>
>> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>> Cc: Yinghai Lu <yinghai@kernel.org>
>> Cc: Yijing Wang <wangyijing@huawei.com>
>> Cc: linux-acpi@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Cc: linux-pci@vger.kernel.org
>> Cc: <stable@vger.kernel.org> # 3.8+
>> ---
>>  drivers/acpi/dock.c                | 109 ++++++++++++++++++++++---------------
>>  drivers/pci/hotplug/acpiphp_glue.c |  15 +++++
>>  include/acpi/acpi_drivers.h        |   2 +
>>  3 files changed, 82 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
>> index 02b0563..602bce5 100644
>> --- a/drivers/acpi/dock.c
>> +++ b/drivers/acpi/dock.c
>> @@ -66,7 +66,7 @@ struct dock_station {
>>  	spinlock_t dd_lock;
>>  	struct mutex hp_lock;
>>  	struct list_head dependent_devices;
>> -	struct list_head hotplug_devices;
>> +	struct klist hotplug_devices;
>>
>>  	struct list_head sibling;
>>  	struct platform_device *dock_device;
>> @@ -76,12 +76,18 @@ static int dock_station_count;
>>
>>  struct dock_dependent_device {
>>  	struct list_head list;
>> -	struct list_head hotplug_list;
>> +	acpi_handle handle;
>> +};
>> +
>> +struct dock_hotplug_info {
>> +	struct klist_node node;
>>  	acpi_handle handle;
>>  	const struct acpi_dock_ops *ops;
>>  	void *context;
>>  };
>
> Can we please relax a bit and possibly take a step back?
>
> So since your last reply to me wasn't particularly helpful, I went through the
> code in dock.c and acpiphp_glue.c and I simply think that the whole
> hotplug_list thing is simply redundant.
>
> It looks like instead of using it (or the klist in this patch), we can add a
> "hotlpug_device" flag to dock_dependent_device and set that flag instead of
> adding dd to hotplug_devices or clear it instead of removing dd from that list.
>
> That would allow us to avoid the deadlock, because we wouldn't need the hp_lock
> any more and perhaps we could make the code simpler instead of making it more
> complex.
>
> How does that sound?
>
> Rafael
Hi Rafael,
       Thanks for comments! It would be great if we could kill the 
hotplug_devices
list so thing gets simple. But there are still some special cases:(

As you have mentioned,  ds->hp_lock is used to make both addition and 
removal
of hotplug devices wait for us to complete walking ds->hotplug_devices.
So it acts as two roles:
1) protect the hotplug_devices list,
2) serialize unregister_hotplug_dock_device() and 
hotplug_dock_devices() so
the dock driver doesn't access registered handler and associated data 
structure
once returing from unregister_hotplug_dock_device().

If we simply use a flag to mark presence of registered callback, we 
can't achieve
the second goal. Take the sony laptop as an example. It has several PCI 
hotplug
slot associated with the dock station:
[   28.829316] acpiphp_glue: _handle_hotplug_event_func: Bus check 
notify on \_SB_.PCI0.RP07.LPMB
[   30.174964] acpiphp_glue: _handle_hotplug_event_func: Bus check 
notify on \_SB_.PCI0.RP07.LPMB.LPM0
[   30.174973] acpiphp_glue: _handle_hotplug_event_func: Bus check 
notify on \_SB_.PCI0.RP07.LPMB.LPM1
[   30.174979] acpiphp_glue: _handle_hotplug_event_func: Bus check 
notify on \_SB_.PCI0.RP07.LPMB.LPM2
[   30.174985] acpiphp_glue: _handle_hotplug_event_func: Bus check 
notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR0.GFXA
[   30.175020] acpiphp_glue: _handle_hotplug_event_func: Bus check 
notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR0.GHDA
[   30.175040] acpiphp_glue: _handle_hotplug_event_func: Bus check 
notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR1.LPCI.LPC0.DLAN
[   30.175050] acpiphp_glue: _handle_hotplug_event_func: Bus check 
notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR1.LPCI.LPC1.DODD
[   30.175060] acpiphp_glue: _handle_hotplug_event_func: Bus check 
notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR1.LPCI.LPC2.DUSB

So it still has some race windows if we undock the station while 
repeatedly rescanning/removing
the PCI bus for \_SB_.PCI0.RP07.LPMB.LPM0 through sysfs interfaces. For 
example, thread 1 is
handling undocking event, walking the dependent device list and 
invoking registered callback
handler with associated data. While that, thread 2 may step in to 
unregister the callback for
 \_SB_.PCI0.RP07.LPMB.LPM0. Then thread 1 may cause access-after-free 
issue.

The klist patch solves this issue by adding a "put" callback method to 
explicitly notify
dock client that the dock core has done with previously registered 
handler and associated
data.

>
>
>> +#define node_to_info(n)	container_of((n), struct dock_hotplug_info, node)
>> +
>>  #define DOCK_DOCKING	0x00000001
>>  #define DOCK_UNDOCKING  0x00000002
>>  #define DOCK_IS_DOCK	0x00000010
>> @@ -111,7 +117,6 @@ add_dock_dependent_device(struct dock_station *ds, acpi_handle handle)
>>
>>  	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);
>> @@ -120,36 +125,19 @@ add_dock_dependent_device(struct dock_station *ds, acpi_handle handle)
>>  	return 0;
>>  }
>>
>> -/**
>> - * 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)
>> +static void hotplug_info_get(struct klist_node *node)
>>  {
>> -	mutex_lock(&ds->hp_lock);
>> -	list_add_tail(&dd->hotplug_list, &ds->hotplug_devices);
>> -	mutex_unlock(&ds->hp_lock);
>> +	struct dock_hotplug_info *info = node_to_info(node);
>> +
>> +	info->ops->get(info->context);
>>  }
>>
>> -/**
>> - * dock_del_hotplug_device - remove a hotplug handler from the dock station
>> - * @ds: The dock station
>> - * @dd: the dependent device struct
>> - *
>> - * Delete the dependent device from the dock's hotplug device list
>> - */
>> -static void
>> -dock_del_hotplug_device(struct dock_station *ds,
>> -			struct dock_dependent_device *dd)
>> +static void hotplug_info_put(struct klist_node *node)
>>  {
>> -	mutex_lock(&ds->hp_lock);
>> -	list_del(&dd->hotplug_list);
>> -	mutex_unlock(&ds->hp_lock);
>> +	struct dock_hotplug_info *info = node_to_info(node);
>> +
>> +	info->ops->put(info->context);
>> +	kfree(info);
>>  }
>>
>>  /**
>> @@ -354,15 +342,22 @@ static void dock_remove_acpi_device(acpi_handle handle)
>>  static void hotplug_dock_devices(struct dock_station *ds, u32 event)
>>  {
>>  	struct dock_dependent_device *dd;
>> +	struct klist_iter iter;
>> +	struct klist_node *node;
>> +	struct dock_hotplug_info *info;
>>
>>  	mutex_lock(&ds->hp_lock);
>>
>>  	/*
>>  	 * 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);
>> +	klist_iter_init(&ds->hotplug_devices, &iter);
>> +	while ((node = klist_next(&iter))) {
>> +		info = node_to_info(node);
>> +		if (info->ops && info->ops->handler)
>> +			info->ops->handler(info->handle, event, info->context);
>> +	}
>> +	klist_iter_exit(&iter);
>>
>>  	/*
>>  	 * Now make sure that an acpi_device is created for each
>> @@ -384,7 +379,9 @@ static void dock_event(struct dock_station *ds, u32 event, int num)
>>  	struct device *dev = &ds->dock_device->dev;
>>  	char event_string[13];
>>  	char *envp[] = { event_string, NULL };
>> -	struct dock_dependent_device *dd;
>> +	struct klist_iter iter;
>> +	struct klist_node *node;
>> +	struct dock_hotplug_info *info;
>>
>>  	if (num == UNDOCK_EVENT)
>>  		sprintf(event_string, "EVENT=undock");
>> @@ -398,9 +395,13 @@ static void dock_event(struct dock_station *ds, u32 event, int num)
>>  	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);
>> +	klist_iter_init(&ds->hotplug_devices, &iter);
>> +	while ((node = klist_next(&iter))) {
>> +		info = node_to_info(node);
>> +		if (info->ops && info->ops->handler)
>> +			info->ops->handler(info->handle, event, info->context);
>> +	}
>> +	klist_iter_exit(&iter);
>>
>>  	if (num != DOCK_EVENT)
>>  		kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
>> @@ -580,12 +581,16 @@ register_hotplug_dock_device(acpi_handle handle, const struct acpi_dock_ops *ops
>>  			     void *context)
>>  {
>>  	struct dock_dependent_device *dd;
>> +	struct dock_hotplug_info *info;
>>  	struct dock_station *dock_station;
>>  	int ret = -EINVAL;
>>
>>  	if (!dock_station_count)
>>  		return -ENODEV;
>>
>> +	if (ops == NULL || ops->get == NULL || ops->put == NULL)
>> +		return -EINVAL;
>> +
>>  	/*
>>  	 * make sure this handle is for a device dependent on the dock,
>>  	 * this would include the dock station itself
>> @@ -598,9 +603,18 @@ register_hotplug_dock_device(acpi_handle handle, const struct acpi_dock_ops *ops
>>  		 */
>>  		dd = find_dock_dependent_device(dock_station, handle);
>>  		if (dd) {
>> -			dd->ops = ops;
>> -			dd->context = context;
>> -			dock_add_hotplug_device(dock_station, dd);
>> +			info = kzalloc(sizeof(*info), GFP_KERNEL);
>> +			if (!info) {
>> +				unregister_hotplug_dock_device(handle);
>> +				ret = -ENOMEM;
>> +				break;
>> +			}
>> +
>> +			info->ops = ops;
>> +			info->context = context;
>> +			info->handle = dd->handle;
>> +			klist_add_tail(&info->node,
>> +				       &dock_station->hotplug_devices);
>>  			ret = 0;
>>  		}
>>  	}
>> @@ -615,16 +629,22 @@ EXPORT_SYMBOL_GPL(register_hotplug_dock_device);
>>   */
>>  void unregister_hotplug_dock_device(acpi_handle handle)
>>  {
>> -	struct dock_dependent_device *dd;
>>  	struct dock_station *dock_station;
>> +	struct klist_iter iter;
>> +	struct klist_node *node;
>> +	struct dock_hotplug_info *info;
>>
>>  	if (!dock_station_count)
>>  		return;
>>
>>  	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);
>> +		klist_iter_init(&dock_station->hotplug_devices, &iter);
>> +		while ((node = klist_next(&iter))) {
>> +			info = node_to_info(node);
>> +			if (info->handle == handle)
>> +				klist_del(&info->node);
>> +		}
>> +		klist_iter_exit(&iter);
>>  	}
>>  }
>>  EXPORT_SYMBOL_GPL(unregister_hotplug_dock_device);
>> @@ -951,7 +971,8 @@ static int __init dock_add(acpi_handle handle)
>>  	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);
>> +	klist_init(&dock_station->hotplug_devices,
>> +		   hotplug_info_get, hotplug_info_put);
>>  	ATOMIC_INIT_NOTIFIER_HEAD(&dock_notifier_list);
>>  	INIT_LIST_HEAD(&dock_station->dependent_devices);
>>
>> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
>> index 716aa93..5d696f5 100644
>> --- a/drivers/pci/hotplug/acpiphp_glue.c
>> +++ b/drivers/pci/hotplug/acpiphp_glue.c
>> @@ -145,9 +145,24 @@ static int post_dock_fixups(struct notifier_block *nb, unsigned long val,
>>  	return NOTIFY_OK;
>>  }
>>
>> +static void acpiphp_dock_get(void *data)
>> +{
>> +	struct acpiphp_func *func = data;
>> +
>> +	get_bridge(func->slot->bridge);
>> +}
>> +
>> +static void acpiphp_dock_put(void *data)
>> +{
>> +	struct acpiphp_func *func = data;
>> +
>> +	put_bridge(func->slot->bridge);
>> +}
>>
>>  static const struct acpi_dock_ops acpiphp_dock_ops = {
>>  	.handler = handle_hotplug_event_func,
>> +	.get = acpiphp_dock_get,
>> +	.put = acpiphp_dock_put,
>>  };
>>
>>  /* Check whether the PCI device is managed by native PCIe hotplug driver */
>> diff --git a/include/acpi/acpi_drivers.h b/include/acpi/acpi_drivers.h
>> index e6168a2..8fcc9ac 100644
>> --- a/include/acpi/acpi_drivers.h
>> +++ b/include/acpi/acpi_drivers.h
>> @@ -115,6 +115,8 @@ void pci_acpi_crs_quirks(void);
>>  struct acpi_dock_ops {
>>  	acpi_notify_handler handler;
>>  	acpi_notify_handler uevent;
>> +	void (*get)(void *data);
>> +	void (*put)(void *data);
>>  };
>>
>>  #if defined(CONFIG_ACPI_DOCK) || defined(CONFIG_ACPI_DOCK_MODULE)
>>

Re: [BUGFIX v2 2/4] ACPI, DOCK: resolve possible deadlock scenarios

[Rafael J. Wysocki]

On Saturday, June 15, 2013 09:44:28 AM Jiang Liu wrote:
> On Sat 15 Jun 2013 06:21:02 AM CST, Rafael J. Wysocki wrote:
> > On Saturday, June 15, 2013 03:27:59 AM Jiang Liu wrote:
> >> This is a preparation for next patch to avoid breaking bisecting.
> >> If next patch is applied without this one, it will cause deadlock
> >> as below:
> >>
> >> Case 1:
> >> [   31.015593]  Possible unsafe locking scenario:
> >>
> >> [   31.018350]        CPU0                    CPU1
> >> [   31.019691]        ----                    ----
> >> [   31.021002]   lock(&dock_station->hp_lock);
> >> [   31.022327]                                lock(&slot->crit_sect);
> >> [   31.023650]                                lock(&dock_station->hp_lock);
> >> [   31.025010]   lock(&slot->crit_sect);
> >> [   31.026342]
> >>
> >> Case 2:
> >>         hotplug_dock_devices()
> >>                 mutex_lock(&ds->hp_lock)
> >>                         dd->ops->handler()
> >>                                 register_hotplug_dock_device()
> >>                                         mutex_lock(&ds->hp_lock)
> >> [   34.316570] [ INFO: possible recursive locking detected ]
> >> [   34.316573] 3.10.0-rc4 #6 Tainted: G         C
> >> [   34.316575] ---------------------------------------------
> >> [   34.316577] kworker/0:0/4 is trying to acquire lock:
> >> [   34.316579]  (&dock_station->hp_lock){+.+.+.}, at:
> >> [<ffffffff813c766b>] register_hotplug_dock_device+0x6a/0xbf
> >> [   34.316588]
> >> but task is already holding lock:
> >> [   34.316590]  (&dock_station->hp_lock){+.+.+.}, at:
> >> [<ffffffff813c7270>] hotplug_dock_devices+0x2c/0xda
> >> [   34.316595]
> >> other info that might help us debug this:
> >> [   34.316597]  Possible unsafe locking scenario:
> >>
> >> [   34.316599]        CPU0
> >> [   34.316601]        ----
> >> [   34.316602]   lock(&dock_station->hp_lock);
> >> [   34.316605]   lock(&dock_station->hp_lock);
> >> [   34.316608]
> >>  *** DEADLOCK ***
> >>
> >> So fix this deadlock by not taking ds->hp_lock in function
> >> register_hotplug_dock_device(). This patch also fixes a possible
> >> race conditions in function dock_event() because previously it
> >> accesses ds->hotplug_devices list without holding ds->hp_lock.
> >>
> >> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> >> Cc: Len Brown <lenb@kernel.org>
> >> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> >> Cc: Bjorn Helgaas <bhelgaas@google.com>
> >> Cc: Yinghai Lu <yinghai@kernel.org>
> >> Cc: Yijing Wang <wangyijing@huawei.com>
> >> Cc: linux-acpi@vger.kernel.org
> >> Cc: linux-kernel@vger.kernel.org
> >> Cc: linux-pci@vger.kernel.org
> >> Cc: <stable@vger.kernel.org> # 3.8+
> >> ---
> >>  drivers/acpi/dock.c                | 109 ++++++++++++++++++++++---------------
> >>  drivers/pci/hotplug/acpiphp_glue.c |  15 +++++
> >>  include/acpi/acpi_drivers.h        |   2 +
> >>  3 files changed, 82 insertions(+), 44 deletions(-)
> >>
> >> diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
> >> index 02b0563..602bce5 100644
> >> --- a/drivers/acpi/dock.c
> >> +++ b/drivers/acpi/dock.c
> >> @@ -66,7 +66,7 @@ struct dock_station {
> >>  	spinlock_t dd_lock;
> >>  	struct mutex hp_lock;
> >>  	struct list_head dependent_devices;
> >> -	struct list_head hotplug_devices;
> >> +	struct klist hotplug_devices;
> >>
> >>  	struct list_head sibling;
> >>  	struct platform_device *dock_device;
> >> @@ -76,12 +76,18 @@ static int dock_station_count;
> >>
> >>  struct dock_dependent_device {
> >>  	struct list_head list;
> >> -	struct list_head hotplug_list;
> >> +	acpi_handle handle;
> >> +};
> >> +
> >> +struct dock_hotplug_info {
> >> +	struct klist_node node;
> >>  	acpi_handle handle;
> >>  	const struct acpi_dock_ops *ops;
> >>  	void *context;
> >>  };
> >
> > Can we please relax a bit and possibly take a step back?
> >
> > So since your last reply to me wasn't particularly helpful, I went through the
> > code in dock.c and acpiphp_glue.c and I simply think that the whole
> > hotplug_list thing is simply redundant.
> >
> > It looks like instead of using it (or the klist in this patch), we can add a
> > "hotlpug_device" flag to dock_dependent_device and set that flag instead of
> > adding dd to hotplug_devices or clear it instead of removing dd from that list.
> >
> > That would allow us to avoid the deadlock, because we wouldn't need the hp_lock
> > any more and perhaps we could make the code simpler instead of making it more
> > complex.
> >
> > How does that sound?
> >
> > Rafael
> Hi Rafael,
>        Thanks for comments! It would be great if we could kill the 
> hotplug_devices
> list so thing gets simple. But there are still some special cases:(
> 
> As you have mentioned,  ds->hp_lock is used to make both addition and 
> removal
> of hotplug devices wait for us to complete walking ds->hotplug_devices.
> So it acts as two roles:
> 1) protect the hotplug_devices list,
> 2) serialize unregister_hotplug_dock_device() and 
> hotplug_dock_devices() so
> the dock driver doesn't access registered handler and associated data 
> structure
> once returing from unregister_hotplug_dock_device().

When it returns from unregister_hotplug_dock_device(), nothing prevents it
from accessing whatever it wants, because ds->hp_lock is not used outside
of the add/del and hotplug_dock_devices().  So, the actual role of
ds->hp_lock (not the one that it is supposed to play, but the real one)
is to prevent addition/deletion from happening when hotplug_dock_devices()
is running.  [Yes, it does protect the list, but since the list is in fact
unnecessary, that doesn't matter.]

> If we simply use a flag to mark presence of registered callback, we 
> can't achieve the second goal.

I don't mean using the flag *alone*.

> Take the sony laptop as an example. It has several PCI 
> hotplug
> slot associated with the dock station:
> [   28.829316] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> notify on \_SB_.PCI0.RP07.LPMB
> [   30.174964] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> notify on \_SB_.PCI0.RP07.LPMB.LPM0
> [   30.174973] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> notify on \_SB_.PCI0.RP07.LPMB.LPM1
> [   30.174979] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> notify on \_SB_.PCI0.RP07.LPMB.LPM2
> [   30.174985] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR0.GFXA
> [   30.175020] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR0.GHDA
> [   30.175040] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR1.LPCI.LPC0.DLAN
> [   30.175050] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR1.LPCI.LPC1.DODD
> [   30.175060] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR1.LPCI.LPC2.DUSB
> 
> So it still has some race windows if we undock the station while 
> repeatedly rescanning/removing
> the PCI bus for \_SB_.PCI0.RP07.LPMB.LPM0 through sysfs interfaces. For 
> example, thread 1 is
> handling undocking event, walking the dependent device list and 
> invoking registered callback
> handler with associated data. While that, thread 2 may step in to 
> unregister the callback for
>  \_SB_.PCI0.RP07.LPMB.LPM0. Then thread 1 may cause access-after-free 
> issue.

That should be handled in acpiphp_glue instead of dock.  What you're trying to
do is to make dock work around synchronization issues in the acpiphp driver.

> The klist patch solves this issue by adding a "put" callback method to 
> explicitly notify
> dock client that the dock core has done with previously registered 
> handler and associated
> data.

Honestly, don't you think this is overly compilcated?

Rafael


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

Re: [BUGFIX v2 2/4] ACPI, DOCK: resolve possible deadlock scenarios

[Rafael J. Wysocki]

On Saturday, June 15, 2013 10:17:42 PM Rafael J. Wysocki wrote:
> On Saturday, June 15, 2013 09:44:28 AM Jiang Liu wrote:
> > On Sat 15 Jun 2013 06:21:02 AM CST, Rafael J. Wysocki wrote:
> > > On Saturday, June 15, 2013 03:27:59 AM Jiang Liu wrote:
> > >> This is a preparation for next patch to avoid breaking bisecting.
> > >> If next patch is applied without this one, it will cause deadlock
> > >> as below:
> > >>
> > >> Case 1:
> > >> [   31.015593]  Possible unsafe locking scenario:
> > >>
> > >> [   31.018350]        CPU0                    CPU1
> > >> [   31.019691]        ----                    ----
> > >> [   31.021002]   lock(&dock_station->hp_lock);
> > >> [   31.022327]                                lock(&slot->crit_sect);
> > >> [   31.023650]                                lock(&dock_station->hp_lock);
> > >> [   31.025010]   lock(&slot->crit_sect);
> > >> [   31.026342]
> > >>
> > >> Case 2:
> > >>         hotplug_dock_devices()
> > >>                 mutex_lock(&ds->hp_lock)
> > >>                         dd->ops->handler()
> > >>                                 register_hotplug_dock_device()
> > >>                                         mutex_lock(&ds->hp_lock)
> > >> [   34.316570] [ INFO: possible recursive locking detected ]
> > >> [   34.316573] 3.10.0-rc4 #6 Tainted: G         C
> > >> [   34.316575] ---------------------------------------------
> > >> [   34.316577] kworker/0:0/4 is trying to acquire lock:
> > >> [   34.316579]  (&dock_station->hp_lock){+.+.+.}, at:
> > >> [<ffffffff813c766b>] register_hotplug_dock_device+0x6a/0xbf
> > >> [   34.316588]
> > >> but task is already holding lock:
> > >> [   34.316590]  (&dock_station->hp_lock){+.+.+.}, at:
> > >> [<ffffffff813c7270>] hotplug_dock_devices+0x2c/0xda
> > >> [   34.316595]
> > >> other info that might help us debug this:
> > >> [   34.316597]  Possible unsafe locking scenario:
> > >>
> > >> [   34.316599]        CPU0
> > >> [   34.316601]        ----
> > >> [   34.316602]   lock(&dock_station->hp_lock);
> > >> [   34.316605]   lock(&dock_station->hp_lock);
> > >> [   34.316608]
> > >>  *** DEADLOCK ***
> > >>
> > >> So fix this deadlock by not taking ds->hp_lock in function
> > >> register_hotplug_dock_device(). This patch also fixes a possible
> > >> race conditions in function dock_event() because previously it
> > >> accesses ds->hotplug_devices list without holding ds->hp_lock.
> > >>
> > >> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> > >> Cc: Len Brown <lenb@kernel.org>
> > >> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> > >> Cc: Bjorn Helgaas <bhelgaas@google.com>
> > >> Cc: Yinghai Lu <yinghai@kernel.org>
> > >> Cc: Yijing Wang <wangyijing@huawei.com>
> > >> Cc: linux-acpi@vger.kernel.org
> > >> Cc: linux-kernel@vger.kernel.org
> > >> Cc: linux-pci@vger.kernel.org
> > >> Cc: <stable@vger.kernel.org> # 3.8+
> > >> ---
> > >>  drivers/acpi/dock.c                | 109 ++++++++++++++++++++++---------------
> > >>  drivers/pci/hotplug/acpiphp_glue.c |  15 +++++
> > >>  include/acpi/acpi_drivers.h        |   2 +
> > >>  3 files changed, 82 insertions(+), 44 deletions(-)
> > >>
> > >> diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
> > >> index 02b0563..602bce5 100644
> > >> --- a/drivers/acpi/dock.c
> > >> +++ b/drivers/acpi/dock.c
> > >> @@ -66,7 +66,7 @@ struct dock_station {
> > >>  	spinlock_t dd_lock;
> > >>  	struct mutex hp_lock;
> > >>  	struct list_head dependent_devices;
> > >> -	struct list_head hotplug_devices;
> > >> +	struct klist hotplug_devices;
> > >>
> > >>  	struct list_head sibling;
> > >>  	struct platform_device *dock_device;
> > >> @@ -76,12 +76,18 @@ static int dock_station_count;
> > >>
> > >>  struct dock_dependent_device {
> > >>  	struct list_head list;
> > >> -	struct list_head hotplug_list;
> > >> +	acpi_handle handle;
> > >> +};
> > >> +
> > >> +struct dock_hotplug_info {
> > >> +	struct klist_node node;
> > >>  	acpi_handle handle;
> > >>  	const struct acpi_dock_ops *ops;
> > >>  	void *context;
> > >>  };
> > >
> > > Can we please relax a bit and possibly take a step back?
> > >
> > > So since your last reply to me wasn't particularly helpful, I went through the
> > > code in dock.c and acpiphp_glue.c and I simply think that the whole
> > > hotplug_list thing is simply redundant.
> > >
> > > It looks like instead of using it (or the klist in this patch), we can add a
> > > "hotlpug_device" flag to dock_dependent_device and set that flag instead of
> > > adding dd to hotplug_devices or clear it instead of removing dd from that list.
> > >
> > > That would allow us to avoid the deadlock, because we wouldn't need the hp_lock
> > > any more and perhaps we could make the code simpler instead of making it more
> > > complex.
> > >
> > > How does that sound?
> > >
> > > Rafael
> > Hi Rafael,
> >        Thanks for comments! It would be great if we could kill the 
> > hotplug_devices
> > list so thing gets simple. But there are still some special cases:(
> > 
> > As you have mentioned,  ds->hp_lock is used to make both addition and 
> > removal
> > of hotplug devices wait for us to complete walking ds->hotplug_devices.
> > So it acts as two roles:
> > 1) protect the hotplug_devices list,
> > 2) serialize unregister_hotplug_dock_device() and 
> > hotplug_dock_devices() so
> > the dock driver doesn't access registered handler and associated data 
> > structure
> > once returing from unregister_hotplug_dock_device().
> 
> When it returns from unregister_hotplug_dock_device(), nothing prevents it
> from accessing whatever it wants, because ds->hp_lock is not used outside
> of the add/del and hotplug_dock_devices().  So, the actual role of
> ds->hp_lock (not the one that it is supposed to play, but the real one)
> is to prevent addition/deletion from happening when hotplug_dock_devices()
> is running.  [Yes, it does protect the list, but since the list is in fact
> unnecessary, that doesn't matter.]
> 
> > If we simply use a flag to mark presence of registered callback, we 
> > can't achieve the second goal.
> 
> I don't mean using the flag *alone*.
> 
> > Take the sony laptop as an example. It has several PCI 
> > hotplug
> > slot associated with the dock station:
> > [   28.829316] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> > notify on \_SB_.PCI0.RP07.LPMB
> > [   30.174964] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> > notify on \_SB_.PCI0.RP07.LPMB.LPM0
> > [   30.174973] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> > notify on \_SB_.PCI0.RP07.LPMB.LPM1
> > [   30.174979] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> > notify on \_SB_.PCI0.RP07.LPMB.LPM2
> > [   30.174985] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> > notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR0.GFXA
> > [   30.175020] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> > notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR0.GHDA
> > [   30.175040] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> > notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR1.LPCI.LPC0.DLAN
> > [   30.175050] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> > notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR1.LPCI.LPC1.DODD
> > [   30.175060] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> > notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR1.LPCI.LPC2.DUSB
> > 
> > So it still has some race windows if we undock the station while 
> > repeatedly rescanning/removing
> > the PCI bus for \_SB_.PCI0.RP07.LPMB.LPM0 through sysfs interfaces.

Which sysfs interfaces do you mean, by the way?

If you mean "eject", then it takes acpi_scan_lock and hotplug_dock_devices()
should always be run under acpi_scan_lock too.  It isn't at the moment,
because write_undock() doesn't take acpi_scan_lock(), but this is an obvious
bug (so I'm going to send a patch to fix it in a while).

With that bug fixed, the possible race between acpi_eject_store() and
hotplug_dock_devices() should be prevented from happening, so perhaps we're
worrying about something that cannot happen?

Rafael


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

Re: [BUGFIX v2 2/4] ACPI, DOCK: resolve possible deadlock scenarios

[Rafael J. Wysocki]

On Saturday, June 15, 2013 11:20:40 PM Rafael J. Wysocki wrote:
> On Saturday, June 15, 2013 10:17:42 PM Rafael J. Wysocki wrote:

[...]

> 
> Which sysfs interfaces do you mean, by the way?
> 
> If you mean "eject", then it takes acpi_scan_lock and hotplug_dock_devices()
> should always be run under acpi_scan_lock too.  It isn't at the moment,
> because write_undock() doesn't take acpi_scan_lock(), but this is an obvious
> bug (so I'm going to send a patch to fix it in a while).
> 
> With that bug fixed, the possible race between acpi_eject_store() and
> hotplug_dock_devices() should be prevented from happening, so perhaps we're
> worrying about something that cannot happen?

So here's a question: What particular races are possible if we remove
ds->hp_lock entirely without doing anything else just yet?  I mean, how to
*trigger* them from the start to the end and not how they can possibly happen
but never do, because there's no way they can be actually triggered?

Rafael


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

Re: [BUGFIX v2 2/4] ACPI, DOCK: resolve possible deadlock scenarios

[Jiang Liu]

On 06/16/2013 06:54 AM, Rafael J. Wysocki wrote:
> On Saturday, June 15, 2013 11:20:40 PM Rafael J. Wysocki wrote:
>> On Saturday, June 15, 2013 10:17:42 PM Rafael J. Wysocki wrote:
> 
> [...]
> 
>>
>> Which sysfs interfaces do you mean, by the way?
>>
>> If you mean "eject", then it takes acpi_scan_lock and hotplug_dock_devices()
>> should always be run under acpi_scan_lock too.  It isn't at the moment,
>> because write_undock() doesn't take acpi_scan_lock(), but this is an obvious
>> bug (so I'm going to send a patch to fix it in a while).
>>
>> With that bug fixed, the possible race between acpi_eject_store() and
>> hotplug_dock_devices() should be prevented from happening, so perhaps we're
>> worrying about something that cannot happen?
> 
> So here's a question: What particular races are possible if we remove
> ds->hp_lock entirely without doing anything else just yet?  I mean, how to
> *trigger* them from the start to the end and not how they can possibly happen
> but never do, because there's no way they can be actually triggered?
Hi Rafael,
    I have no really platform which triggers this bug, but I may imagine
a possible platform if it's valid for explanation.
    Let's think about a laptop dock station with a thunderbolt
controller built-in. The dock station is managed by dock driver and
acpiphp driver. And the PCIe hierarchy managed by the thunderbolt
controller may be managed by dock driver and ACPIPHP driver too.
So it may trigger the issue by pressing the dock button and unplugging
thunderbolt cable concurrently.
    But after all, this is all by imagination:). We may need to find a
simple and quick solution for 3.10 and the stable trees and enhance the
solution later to avoid introducing new bugs while fixing a bug.
Regards!
Gerry
> 
> Rafael
> 
> 

Re: [BUGFIX v2 2/4] ACPI, DOCK: resolve possible deadlock scenarios

[Rafael J. Wysocki]

On Monday, June 17, 2013 01:12:00 AM Jiang Liu wrote:
> On 06/16/2013 06:54 AM, Rafael J. Wysocki wrote:
> > On Saturday, June 15, 2013 11:20:40 PM Rafael J. Wysocki wrote:
> >> On Saturday, June 15, 2013 10:17:42 PM Rafael J. Wysocki wrote:
> > 
> > [...]
> > 
> >>
> >> Which sysfs interfaces do you mean, by the way?
> >>
> >> If you mean "eject", then it takes acpi_scan_lock and hotplug_dock_devices()
> >> should always be run under acpi_scan_lock too.  It isn't at the moment,
> >> because write_undock() doesn't take acpi_scan_lock(), but this is an obvious
> >> bug (so I'm going to send a patch to fix it in a while).
> >>
> >> With that bug fixed, the possible race between acpi_eject_store() and
> >> hotplug_dock_devices() should be prevented from happening, so perhaps we're
> >> worrying about something that cannot happen?
> > 
> > So here's a question: What particular races are possible if we remove
> > ds->hp_lock entirely without doing anything else just yet?  I mean, how to
> > *trigger* them from the start to the end and not how they can possibly happen
> > but never do, because there's no way they can be actually triggered?
> Hi Rafael,
>     I have no really platform which triggers this bug, but I may imagine
> a possible platform if it's valid for explanation.
>     Let's think about a laptop dock station with a thunderbolt
> controller built-in. The dock station is managed by dock driver and
> acpiphp driver. And the PCIe hierarchy managed by the thunderbolt
> controller may be managed by dock driver and ACPIPHP driver too.
> So it may trigger the issue by pressing the dock button and unplugging
> thunderbolt cable concurrently.
>     But after all, this is all by imagination:). We may need to find a
> simple and quick solution for 3.10 and the stable trees and enhance the
> solution later to avoid introducing new bugs while fixing a bug.

Well, if that particular bug is not fixed in 3,10, it won't be a tragedy,
and I *really* don't want that code to get substantially more complex than
it is now.

Thanks,
Rafael


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

Re: [BUGFIX v2 2/4] ACPI, DOCK: resolve possible deadlock scenarios

[Jiang Liu]

[-- Attachment #1: Type: text/plain, Size: 2769 bytes --]

On 06/17/2013 07:40 PM, Rafael J. Wysocki wrote:
> On Monday, June 17, 2013 01:12:00 AM Jiang Liu wrote:
>> On 06/16/2013 06:54 AM, Rafael J. Wysocki wrote:
>>> On Saturday, June 15, 2013 11:20:40 PM Rafael J. Wysocki wrote:
>>>> On Saturday, June 15, 2013 10:17:42 PM Rafael J. Wysocki wrote:
>>>
>>> [...]
>>>
>>>>
>>>> Which sysfs interfaces do you mean, by the way?
>>>>
>>>> If you mean "eject", then it takes acpi_scan_lock and hotplug_dock_devices()
>>>> should always be run under acpi_scan_lock too.  It isn't at the moment,
>>>> because write_undock() doesn't take acpi_scan_lock(), but this is an obvious
>>>> bug (so I'm going to send a patch to fix it in a while).
>>>>
>>>> With that bug fixed, the possible race between acpi_eject_store() and
>>>> hotplug_dock_devices() should be prevented from happening, so perhaps we're
>>>> worrying about something that cannot happen?
>>>
>>> So here's a question: What particular races are possible if we remove
>>> ds->hp_lock entirely without doing anything else just yet?  I mean, how to
>>> *trigger* them from the start to the end and not how they can possibly happen
>>> but never do, because there's no way they can be actually triggered?
>> Hi Rafael,
>>     I have no really platform which triggers this bug, but I may imagine
>> a possible platform if it's valid for explanation.
>>     Let's think about a laptop dock station with a thunderbolt
>> controller built-in. The dock station is managed by dock driver and
>> acpiphp driver. And the PCIe hierarchy managed by the thunderbolt
>> controller may be managed by dock driver and ACPIPHP driver too.
>> So it may trigger the issue by pressing the dock button and unplugging
>> thunderbolt cable concurrently.
>>     But after all, this is all by imagination:). We may need to find a
>> simple and quick solution for 3.10 and the stable trees and enhance the
>> solution later to avoid introducing new bugs while fixing a bug.
> 
> Well, if that particular bug is not fixed in 3,10, it won't be a tragedy,
> and I *really* don't want that code to get substantially more complex than
> it is now.
Hi Rafael,
    I hope I could help to simplify the implementation too, but failed
until now:(. The PCI hotplug core has the same re-entrance issue too,
and I have struggled with that issue about one year now:(
    I have tried another solution by removing ds->hp_lock and
hotplug_devices list, please refer to the attachment. It could be used
to solve the deadlock issue in acpiphp, but may not be used to support
the coming fix for an ATA driver
regression(https://bugzilla.kernel.org/show_bug.cgi?id=59871).
    The time window for 3.10 is closing, it would be great if we could
reach a quick solution here.
Regards!
Gerry

> 
> Thanks,
> Rafael
> 
> 


[-- Attachment #2: 0001-.patch --]
[-- Type: text/x-patch, Size: 9549 bytes --]

>From d1a3f472ea81c780c370a98b553cf83f82d3e961 Mon Sep 17 00:00:00 2001
From: Jiang Liu <jiang.liu@huawei.com>
Date: Tue, 18 Jun 2013 22:22:23 +0800
Subject: [PATCH]

---
 drivers/acpi/dock.c                | 108 +++++++++++++++----------------------
 drivers/pci/hotplug/acpiphp_glue.c |  32 ++++++-----
 2 files changed, 62 insertions(+), 78 deletions(-)

diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
index 469ef56..fd8830a 100644
--- a/drivers/acpi/dock.c
+++ b/drivers/acpi/dock.c
@@ -58,10 +58,7 @@ struct dock_station {
 	unsigned long last_dock_time;
 	u32 flags;
 	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;
 };
@@ -70,7 +67,6 @@ static int dock_station_count;
 
 struct dock_dependent_device {
 	struct list_head list;
-	struct list_head hotplug_list;
 	acpi_handle handle;
 	const struct acpi_dock_ops *ops;
 	void *context;
@@ -105,7 +101,6 @@ add_dock_dependent_device(struct dock_station *ds, acpi_handle handle)
 
 	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);
@@ -115,38 +110,6 @@ add_dock_dependent_device(struct dock_station *ds, acpi_handle handle)
 }
 
 /**
- * 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_del_hotplug_device - remove a hotplug handler from the dock station
- * @ds: The dock station
- * @dd: the dependent device struct
- *
- * Delete the dependent device from the dock's hotplug device list
- */
-static void
-dock_del_hotplug_device(struct dock_station *ds,
-			struct dock_dependent_device *dd)
-{
-	mutex_lock(&ds->hp_lock);
-	list_del(&dd->hotplug_list);
-	mutex_unlock(&ds->hp_lock);
-}
-
-/**
  * find_dock_dependent_device - get a device dependent on this dock
  * @ds: the dock station
  * @handle: the acpi_handle of the device we want
@@ -349,12 +312,10 @@ static void hotplug_dock_devices(struct dock_station *ds, u32 event)
 {
 	struct dock_dependent_device *dd;
 
-	mutex_lock(&ds->hp_lock);
-
 	/*
 	 * First call driver specific hotplug functions
 	 */
-	list_for_each_entry(dd, &ds->hotplug_devices, hotplug_list)
+	list_for_each_entry(dd, &ds->dependent_devices, list)
 		if (dd->ops && dd->ops->handler)
 			dd->ops->handler(dd->handle, event, dd->context);
 
@@ -370,7 +331,6 @@ static void hotplug_dock_devices(struct dock_station *ds, u32 event)
 		else
 			dock_create_acpi_device(dd->handle);
 	}
-	mutex_unlock(&ds->hp_lock);
 }
 
 static void dock_event(struct dock_station *ds, u32 event, int num)
@@ -392,7 +352,7 @@ static void dock_event(struct dock_station *ds, u32 event, int num)
 	if (num == DOCK_EVENT)
 		kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
 
-	list_for_each_entry(dd, &ds->hotplug_devices, hotplug_list)
+	list_for_each_entry(dd, &ds->dependent_devices, list)
 		if (dd->ops && dd->ops->uevent)
 			dd->ops->uevent(dd->handle, event, dd->context);
 
@@ -559,19 +519,9 @@ void unregister_dock_notifier(struct notifier_block *nb)
 }
 EXPORT_SYMBOL_GPL(unregister_dock_notifier);
 
-/**
- * register_hotplug_dock_device - register a hotplug function
- * @handle: the handle of the device
- * @ops: handlers to call after docking
- * @context: device specific data
- *
- * 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)
+/* must be called with ACPI scan lock held */
+int __register_hotplug_dock_device(acpi_handle handle,
+	       const struct acpi_dock_ops *ops, void *context)
 {
 	struct dock_dependent_device *dd;
 	struct dock_station *dock_station;
@@ -594,20 +544,39 @@ register_hotplug_dock_device(acpi_handle handle, const struct acpi_dock_ops *ops
 		if (dd) {
 			dd->ops = ops;
 			dd->context = context;
-			dock_add_hotplug_device(dock_station, dd);
 			ret = 0;
 		}
 	}
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(register_hotplug_dock_device);
 
 /**
- * unregister_hotplug_dock_device - remove yourself from the hotplug list
- * @handle: the acpi handle of the device
+ * register_hotplug_dock_device - register a hotplug function
+ * @handle: the handle of the device
+ * @ops: handlers to call after docking
+ * @context: device specific data
+ *
+ * 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.
  */
-void unregister_hotplug_dock_device(acpi_handle handle)
+int
+register_hotplug_dock_device(acpi_handle handle,
+			     const struct acpi_dock_ops *ops, void *context)
+{
+	int ret;
+
+	acpi_scan_lock_acquire();
+	ret = __register_hotplug_dock_device(handle, ops, context);
+	acpi_scan_lock_release();
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(register_hotplug_dock_device);
+
+/* must be called with ACPI scan lock held */
+void __unregister_hotplug_dock_device(acpi_handle handle)
 {
 	struct dock_dependent_device *dd;
 	struct dock_station *dock_station;
@@ -617,10 +586,23 @@ void unregister_hotplug_dock_device(acpi_handle handle)
 
 	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) {
+			dd->ops = NULL;
+			dd->context = NULL;
+		}
 	}
 }
+
+/**
+ * unregister_hotplug_dock_device - remove yourself from the hotplug list
+ * @handle: the acpi handle of the device
+ */
+void unregister_hotplug_dock_device(acpi_handle handle)
+{
+	acpi_scan_lock_acquire();
+	__unregister_hotplug_dock_device(handle);
+	acpi_scan_lock_release();
+}
 EXPORT_SYMBOL_GPL(unregister_hotplug_dock_device);
 
 /**
@@ -945,10 +927,8 @@ static int __init dock_add(acpi_handle handle)
 	dock_station->dock_device = dd;
 	dock_station->last_dock_time = jiffies - HZ;
 
-	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);
 
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 716aa93..699b8ca 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -61,6 +61,8 @@ 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 _handle_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 +149,7 @@ static int post_dock_fixups(struct notifier_block *nb, unsigned long val,
 
 
 static const struct acpi_dock_ops acpiphp_dock_ops = {
-	.handler = handle_hotplug_event_func,
+	.handler = _handle_hotplug_event_func,
 };
 
 /* Check whether the PCI device is managed by native PCIe hotplug driver */
@@ -1065,22 +1067,13 @@ static void handle_hotplug_event_bridge(acpi_handle handle, u32 type,
 	alloc_acpi_hp_work(handle, type, context, _handle_hotplug_event_bridge);
 }
 
-static void _handle_hotplug_event_func(struct work_struct *work)
+static void _handle_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);
 
@@ -1113,7 +1106,18 @@ static void _handle_hotplug_event_func(struct work_struct *work)
 		warn("notify_handler: unknown event type 0x%x for %s\n", type, objname);
 		break;
 	}
+}
+
+static void _handle_hotplug_event_cb(struct work_struct *work)
+{
+	struct acpiphp_func *func;
+	struct acpi_hp_work *hp_work;
 
+	hp_work = container_of(work, struct acpi_hp_work, work);
+	func = (struct acpiphp_func *)hp_work->context;
+	acpi_scan_lock_acquire();
+	_handle_hotplug_event_func(hp_work->handle, hp_work->type,
+				    hp_work->context);
 	acpi_scan_lock_release();
 	kfree(hp_work); /* allocated in handle_hotplug_event_func */
 	put_bridge(func->slot->bridge);
@@ -1141,7 +1145,7 @@ static void handle_hotplug_event_func(acpi_handle handle, u32 type,
 	 * don't deadlock on hotplug actions.
 	 */
 	get_bridge(func->slot->bridge);
-	alloc_acpi_hp_work(handle, type, context, _handle_hotplug_event_func);
+	alloc_acpi_hp_work(handle, type, context, _handle_hotplug_event_cb);
 }
 
 /*
-- 
1.8.1.2

Re: [BUGFIX v2 2/4] ACPI, DOCK: resolve possible deadlock scenarios

[Rafael J. Wysocki]

On Wednesday, June 19, 2013 12:03:37 AM Jiang Liu wrote:
> On 06/17/2013 07:40 PM, Rafael J. Wysocki wrote:
> > On Monday, June 17, 2013 01:12:00 AM Jiang Liu wrote:
> >> On 06/16/2013 06:54 AM, Rafael J. Wysocki wrote:
> >>> On Saturday, June 15, 2013 11:20:40 PM Rafael J. Wysocki wrote:
> >>>> On Saturday, June 15, 2013 10:17:42 PM Rafael J. Wysocki wrote:

[...]

> Hi Rafael,

Hi,

>     I hope I could help to simplify the implementation too, but failed
> until now:(. The PCI hotplug core has the same re-entrance issue too,
> and I have struggled with that issue about one year now:(

This sounds like a design issue and we're not likely to fix design issues
in 2 weeks, with all due respect to everyone involved.  Even if someone has
an "Eureka!" moment and comes up with a really clever way to fix that issue,
we still need time to prepare patches, review them, test them etc.

>     I have tried another solution by removing ds->hp_lock and
> hotplug_devices list, please refer to the attachment. It could be used
> to solve the deadlock issue in acpiphp, but may not be used to support
> the coming fix for an ATA driver
> regression(https://bugzilla.kernel.org/show_bug.cgi?id=59871).

Please stop generating patches in a hurry.  That's not really useful.

Even if you think you know what you're doing, someone else has to understand
that too and be able to review your patches.  Honestly, my experience with
that code is kind of limited and I need more time.

>     The time window for 3.10 is closing, it would be great if we could
> reach a quick solution here.

That is clearly impossible.

My suggestion would be to apply the patches that everyone is reasonably
comfortable with at the moment and stop worrying about "time windows", because
in fact there are none.  We're talking about fixes here and we can do -stable
backports once we have a solid solution, but what matters is that this solution
has to be acceptable to *all* of us.

So please stop making it sound like the 3.10 release is a hard deadline or
something.  It is not.

Thanks,
Rafael


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

Re: [BUGFIX v2 2/4] ACPI, DOCK: resolve possible deadlock scenarios

[Jiang Liu]

On 06/16/2013 05:20 AM, Rafael J. Wysocki wrote:
> On Saturday, June 15, 2013 10:17:42 PM Rafael J. Wysocki wrote:
>> On Saturday, June 15, 2013 09:44:28 AM Jiang Liu wrote:
[...]
>> When it returns from unregister_hotplug_dock_device(), nothing prevents it
>> from accessing whatever it wants, because ds->hp_lock is not used outside
>> of the add/del and hotplug_dock_devices().  So, the actual role of
>> ds->hp_lock (not the one that it is supposed to play, but the real one)
>> is to prevent addition/deletion from happening when hotplug_dock_devices()
>> is running.  [Yes, it does protect the list, but since the list is in fact
>> unnecessary, that doesn't matter.]
>>
>>> If we simply use a flag to mark presence of registered callback, we 
>>> can't achieve the second goal.
>>
>> I don't mean using the flag *alone*.
>>
>>> Take the sony laptop as an example. It has several PCI 
>>> hotplug
>>> slot associated with the dock station:
>>> [   28.829316] acpiphp_glue: _handle_hotplug_event_func: Bus check 
>>> notify on \_SB_.PCI0.RP07.LPMB
>>> [   30.174964] acpiphp_glue: _handle_hotplug_event_func: Bus check 
>>> notify on \_SB_.PCI0.RP07.LPMB.LPM0
>>> [   30.174973] acpiphp_glue: _handle_hotplug_event_func: Bus check 
>>> notify on \_SB_.PCI0.RP07.LPMB.LPM1
>>> [   30.174979] acpiphp_glue: _handle_hotplug_event_func: Bus check 
>>> notify on \_SB_.PCI0.RP07.LPMB.LPM2
>>> [   30.174985] acpiphp_glue: _handle_hotplug_event_func: Bus check 
>>> notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR0.GFXA
>>> [   30.175020] acpiphp_glue: _handle_hotplug_event_func: Bus check 
>>> notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR0.GHDA
>>> [   30.175040] acpiphp_glue: _handle_hotplug_event_func: Bus check 
>>> notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR1.LPCI.LPC0.DLAN
>>> [   30.175050] acpiphp_glue: _handle_hotplug_event_func: Bus check 
>>> notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR1.LPCI.LPC1.DODD
>>> [   30.175060] acpiphp_glue: _handle_hotplug_event_func: Bus check 
>>> notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR1.LPCI.LPC2.DUSB
>>>
>>> So it still has some race windows if we undock the station while 
>>> repeatedly rescanning/removing
>>> the PCI bus for \_SB_.PCI0.RP07.LPMB.LPM0 through sysfs interfaces.
> 
> Which sysfs interfaces do you mean, by the way?
> 
> If you mean "eject", then it takes acpi_scan_lock and hotplug_dock_devices()
> should always be run under acpi_scan_lock too.  It isn't at the moment,
> because write_undock() doesn't take acpi_scan_lock(), but this is an obvious
> bug (so I'm going to send a patch to fix it in a while).
> 
> With that bug fixed, the possible race between acpi_eject_store() and
> hotplug_dock_devices() should be prevented from happening, so perhaps we're
> worrying about something that cannot happen?
Hi Rafael,
	I mean the "remove" method of each PCI device, and the "power" method
of PCI hotplug slot here.
	These methods may be used to remove P2P bridges with associated ACPIPHP
hotplug slots, which in turn will cause invoking of
unregister_hotplug_dock_device().
	So theoretical we may trigger the bug by undocking while repeatedly
adding/removing P2P bridges with ACPIPHP hotplug slot through PCI
"rescan" and "remove" sysfs interface,
Regards!
Gerry
> 
> Rafael
> 
> 

Re: [BUGFIX v2 2/4] ACPI, DOCK: resolve possible deadlock scenarios

[Rafael J. Wysocki]

On Monday, June 17, 2013 01:01:51 AM Jiang Liu wrote:
> On 06/16/2013 05:20 AM, Rafael J. Wysocki wrote:
> > On Saturday, June 15, 2013 10:17:42 PM Rafael J. Wysocki wrote:
> >> On Saturday, June 15, 2013 09:44:28 AM Jiang Liu wrote:
> [...]
> >> When it returns from unregister_hotplug_dock_device(), nothing prevents it
> >> from accessing whatever it wants, because ds->hp_lock is not used outside
> >> of the add/del and hotplug_dock_devices().  So, the actual role of
> >> ds->hp_lock (not the one that it is supposed to play, but the real one)
> >> is to prevent addition/deletion from happening when hotplug_dock_devices()
> >> is running.  [Yes, it does protect the list, but since the list is in fact
> >> unnecessary, that doesn't matter.]
> >>
> >>> If we simply use a flag to mark presence of registered callback, we 
> >>> can't achieve the second goal.
> >>
> >> I don't mean using the flag *alone*.
> >>
> >>> Take the sony laptop as an example. It has several PCI 
> >>> hotplug
> >>> slot associated with the dock station:
> >>> [   28.829316] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> >>> notify on \_SB_.PCI0.RP07.LPMB
> >>> [   30.174964] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> >>> notify on \_SB_.PCI0.RP07.LPMB.LPM0
> >>> [   30.174973] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> >>> notify on \_SB_.PCI0.RP07.LPMB.LPM1
> >>> [   30.174979] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> >>> notify on \_SB_.PCI0.RP07.LPMB.LPM2
> >>> [   30.174985] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> >>> notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR0.GFXA
> >>> [   30.175020] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> >>> notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR0.GHDA
> >>> [   30.175040] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> >>> notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR1.LPCI.LPC0.DLAN
> >>> [   30.175050] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> >>> notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR1.LPCI.LPC1.DODD
> >>> [   30.175060] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> >>> notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR1.LPCI.LPC2.DUSB
> >>>
> >>> So it still has some race windows if we undock the station while 
> >>> repeatedly rescanning/removing
> >>> the PCI bus for \_SB_.PCI0.RP07.LPMB.LPM0 through sysfs interfaces.
> > 
> > Which sysfs interfaces do you mean, by the way?
> > 
> > If you mean "eject", then it takes acpi_scan_lock and hotplug_dock_devices()
> > should always be run under acpi_scan_lock too.  It isn't at the moment,
> > because write_undock() doesn't take acpi_scan_lock(), but this is an obvious
> > bug (so I'm going to send a patch to fix it in a while).
> > 
> > With that bug fixed, the possible race between acpi_eject_store() and
> > hotplug_dock_devices() should be prevented from happening, so perhaps we're
> > worrying about something that cannot happen?
> Hi Rafael,
> 	I mean the "remove" method of each PCI device, and the "power" method
> of PCI hotplug slot here.
> 	These methods may be used to remove P2P bridges with associated ACPIPHP
> hotplug slots, which in turn will cause invoking of
> unregister_hotplug_dock_device().
> 	So theoretical we may trigger the bug by undocking while repeatedly
> adding/removing P2P bridges with ACPIPHP hotplug slot through PCI
> "rescan" and "remove" sysfs interface,

Why don't we make these things take acpi_scan_lock upfront, then?

Rafael


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

Re: [BUGFIX v2 2/4] ACPI, DOCK: resolve possible deadlock scenarios

[Rafael J. Wysocki]

On Monday, June 17, 2013 01:39:04 PM Rafael J. Wysocki wrote:
> On Monday, June 17, 2013 01:01:51 AM Jiang Liu wrote:
> > On 06/16/2013 05:20 AM, Rafael J. Wysocki wrote:
> > > On Saturday, June 15, 2013 10:17:42 PM Rafael J. Wysocki wrote:
> > >> On Saturday, June 15, 2013 09:44:28 AM Jiang Liu wrote:
> > [...]
> > >> When it returns from unregister_hotplug_dock_device(), nothing prevents it
> > >> from accessing whatever it wants, because ds->hp_lock is not used outside
> > >> of the add/del and hotplug_dock_devices().  So, the actual role of
> > >> ds->hp_lock (not the one that it is supposed to play, but the real one)
> > >> is to prevent addition/deletion from happening when hotplug_dock_devices()
> > >> is running.  [Yes, it does protect the list, but since the list is in fact
> > >> unnecessary, that doesn't matter.]
> > >>
> > >>> If we simply use a flag to mark presence of registered callback, we 
> > >>> can't achieve the second goal.
> > >>
> > >> I don't mean using the flag *alone*.
> > >>
> > >>> Take the sony laptop as an example. It has several PCI 
> > >>> hotplug
> > >>> slot associated with the dock station:
> > >>> [   28.829316] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> > >>> notify on \_SB_.PCI0.RP07.LPMB
> > >>> [   30.174964] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> > >>> notify on \_SB_.PCI0.RP07.LPMB.LPM0
> > >>> [   30.174973] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> > >>> notify on \_SB_.PCI0.RP07.LPMB.LPM1
> > >>> [   30.174979] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> > >>> notify on \_SB_.PCI0.RP07.LPMB.LPM2
> > >>> [   30.174985] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> > >>> notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR0.GFXA
> > >>> [   30.175020] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> > >>> notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR0.GHDA
> > >>> [   30.175040] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> > >>> notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR1.LPCI.LPC0.DLAN
> > >>> [   30.175050] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> > >>> notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR1.LPCI.LPC1.DODD
> > >>> [   30.175060] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> > >>> notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR1.LPCI.LPC2.DUSB
> > >>>
> > >>> So it still has some race windows if we undock the station while 
> > >>> repeatedly rescanning/removing
> > >>> the PCI bus for \_SB_.PCI0.RP07.LPMB.LPM0 through sysfs interfaces.
> > > 
> > > Which sysfs interfaces do you mean, by the way?
> > > 
> > > If you mean "eject", then it takes acpi_scan_lock and hotplug_dock_devices()
> > > should always be run under acpi_scan_lock too.  It isn't at the moment,
> > > because write_undock() doesn't take acpi_scan_lock(), but this is an obvious
> > > bug (so I'm going to send a patch to fix it in a while).
> > > 
> > > With that bug fixed, the possible race between acpi_eject_store() and
> > > hotplug_dock_devices() should be prevented from happening, so perhaps we're
> > > worrying about something that cannot happen?
> > Hi Rafael,
> > 	I mean the "remove" method of each PCI device, and the "power" method
> > of PCI hotplug slot here.
> > 	These methods may be used to remove P2P bridges with associated ACPIPHP
> > hotplug slots, which in turn will cause invoking of
> > unregister_hotplug_dock_device().
> > 	So theoretical we may trigger the bug by undocking while repeatedly
> > adding/removing P2P bridges with ACPIPHP hotplug slot through PCI
> > "rescan" and "remove" sysfs interface,
> 
> Why don't we make these things take acpi_scan_lock upfront, then?

Or perhaps (and maybe better) why don't we replace ds->hp_lock by another
lock that will be acquired upper in the call chain so that
dock_add_hotplug_device(), dock_del_hotplug_device(), hotplug_dock_devices()
and dock_event() are all guaranteed to be called under that lock?

Rafael


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

Re: [BUGFIX v2 2/4] ACPI, DOCK: resolve possible deadlock scenarios

[Jiang Liu]

On 06/17/2013 07:39 PM, Rafael J. Wysocki wrote:
> On Monday, June 17, 2013 01:01:51 AM Jiang Liu wrote:
>> On 06/16/2013 05:20 AM, Rafael J. Wysocki wrote:
>>> On Saturday, June 15, 2013 10:17:42 PM Rafael J. Wysocki wrote:
>>>> On Saturday, June 15, 2013 09:44:28 AM Jiang Liu wrote:
>> [...]
>>>> When it returns from unregister_hotplug_dock_device(), nothing prevents it
>>>> from accessing whatever it wants, because ds->hp_lock is not used outside
>>>> of the add/del and hotplug_dock_devices().  So, the actual role of
>>>> ds->hp_lock (not the one that it is supposed to play, but the real one)
>>>> is to prevent addition/deletion from happening when hotplug_dock_devices()
>>>> is running.  [Yes, it does protect the list, but since the list is in fact
>>>> unnecessary, that doesn't matter.]
>>>>
>>>>> If we simply use a flag to mark presence of registered callback, we 
>>>>> can't achieve the second goal.
>>>>
>>>> I don't mean using the flag *alone*.
>>>>
>>>>> Take the sony laptop as an example. It has several PCI 
>>>>> hotplug
>>>>> slot associated with the dock station:
>>>>> [   28.829316] acpiphp_glue: _handle_hotplug_event_func: Bus check 
>>>>> notify on \_SB_.PCI0.RP07.LPMB
>>>>> [   30.174964] acpiphp_glue: _handle_hotplug_event_func: Bus check 
>>>>> notify on \_SB_.PCI0.RP07.LPMB.LPM0
>>>>> [   30.174973] acpiphp_glue: _handle_hotplug_event_func: Bus check 
>>>>> notify on \_SB_.PCI0.RP07.LPMB.LPM1
>>>>> [   30.174979] acpiphp_glue: _handle_hotplug_event_func: Bus check 
>>>>> notify on \_SB_.PCI0.RP07.LPMB.LPM2
>>>>> [   30.174985] acpiphp_glue: _handle_hotplug_event_func: Bus check 
>>>>> notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR0.GFXA
>>>>> [   30.175020] acpiphp_glue: _handle_hotplug_event_func: Bus check 
>>>>> notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR0.GHDA
>>>>> [   30.175040] acpiphp_glue: _handle_hotplug_event_func: Bus check 
>>>>> notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR1.LPCI.LPC0.DLAN
>>>>> [   30.175050] acpiphp_glue: _handle_hotplug_event_func: Bus check 
>>>>> notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR1.LPCI.LPC1.DODD
>>>>> [   30.175060] acpiphp_glue: _handle_hotplug_event_func: Bus check 
>>>>> notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR1.LPCI.LPC2.DUSB
>>>>>
>>>>> So it still has some race windows if we undock the station while 
>>>>> repeatedly rescanning/removing
>>>>> the PCI bus for \_SB_.PCI0.RP07.LPMB.LPM0 through sysfs interfaces.
>>>
>>> Which sysfs interfaces do you mean, by the way?
>>>
>>> If you mean "eject", then it takes acpi_scan_lock and hotplug_dock_devices()
>>> should always be run under acpi_scan_lock too.  It isn't at the moment,t
>>> because write_undock() doesn't take acpi_scan_lock(), but this is an obvious
>>> bug (so I'm going to send a patch to fix it in a while).
>>>
>>> With that bug fixed, the possible race between acpi_eject_store() and
>>> hotplug_dock_devices() should be prevented from happening, so perhaps we're
>>> worrying about something that cannot happen?
>> Hi Rafael,
>> 	I mean the "remove" method of each PCI device, and the "power" method
>> of PCI hotplug slot here.
>> 	These methods may be used to remove P2P bridges with associated ACPIPHP
>> hotplug slots, which in turn will cause invoking of
>> unregister_hotplug_dock_device().
>> 	So theoretical we may trigger the bug by undocking while repeatedly
>> adding/removing P2P bridges with ACPIPHP hotplug slot through PCI
>> "rescan" and "remove" sysfs interface,
> 
> Why don't we make these things take acpi_scan_lock upfront, then?
Hi Rafael,
    Seems we can't rely on acpi_scan_lock here, it may cause another
deadlock scenario:
1) thread 1 acquired the acpi_scan_lock and tries to destroy all sysfs
interfaces for PCI devices.
2) thread 2 opens a PCI sysfs which then tries to acquire the
acpi_scan_lock.
Regards!
Gerry

> 
> Rafael
> 
> 

Re: [BUGFIX v2 2/4] ACPI, DOCK: resolve possible deadlock scenarios

[Rafael J. Wysocki]

On Tuesday, June 18, 2013 11:36:50 PM Jiang Liu wrote:
> On 06/17/2013 07:39 PM, Rafael J. Wysocki wrote:
> > On Monday, June 17, 2013 01:01:51 AM Jiang Liu wrote:
> >> On 06/16/2013 05:20 AM, Rafael J. Wysocki wrote:
> >>> On Saturday, June 15, 2013 10:17:42 PM Rafael J. Wysocki wrote:
> >>>> On Saturday, June 15, 2013 09:44:28 AM Jiang Liu wrote:
> >> [...]
> >>>> When it returns from unregister_hotplug_dock_device(), nothing prevents it
> >>>> from accessing whatever it wants, because ds->hp_lock is not used outside
> >>>> of the add/del and hotplug_dock_devices().  So, the actual role of
> >>>> ds->hp_lock (not the one that it is supposed to play, but the real one)
> >>>> is to prevent addition/deletion from happening when hotplug_dock_devices()
> >>>> is running.  [Yes, it does protect the list, but since the list is in fact
> >>>> unnecessary, that doesn't matter.]
> >>>>
> >>>>> If we simply use a flag to mark presence of registered callback, we 
> >>>>> can't achieve the second goal.
> >>>>
> >>>> I don't mean using the flag *alone*.
> >>>>
> >>>>> Take the sony laptop as an example. It has several PCI 
> >>>>> hotplug
> >>>>> slot associated with the dock station:
> >>>>> [   28.829316] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> >>>>> notify on \_SB_.PCI0.RP07.LPMB
> >>>>> [   30.174964] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> >>>>> notify on \_SB_.PCI0.RP07.LPMB.LPM0
> >>>>> [   30.174973] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> >>>>> notify on \_SB_.PCI0.RP07.LPMB.LPM1
> >>>>> [   30.174979] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> >>>>> notify on \_SB_.PCI0.RP07.LPMB.LPM2
> >>>>> [   30.174985] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> >>>>> notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR0.GFXA
> >>>>> [   30.175020] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> >>>>> notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR0.GHDA
> >>>>> [   30.175040] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> >>>>> notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR1.LPCI.LPC0.DLAN
> >>>>> [   30.175050] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> >>>>> notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR1.LPCI.LPC1.DODD
> >>>>> [   30.175060] acpiphp_glue: _handle_hotplug_event_func: Bus check 
> >>>>> notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR1.LPCI.LPC2.DUSB
> >>>>>
> >>>>> So it still has some race windows if we undock the station while 
> >>>>> repeatedly rescanning/removing
> >>>>> the PCI bus for \_SB_.PCI0.RP07.LPMB.LPM0 through sysfs interfaces.
> >>>
> >>> Which sysfs interfaces do you mean, by the way?
> >>>
> >>> If you mean "eject", then it takes acpi_scan_lock and hotplug_dock_devices()
> >>> should always be run under acpi_scan_lock too.  It isn't at the moment,t
> >>> because write_undock() doesn't take acpi_scan_lock(), but this is an obvious
> >>> bug (so I'm going to send a patch to fix it in a while).
> >>>
> >>> With that bug fixed, the possible race between acpi_eject_store() and
> >>> hotplug_dock_devices() should be prevented from happening, so perhaps we're
> >>> worrying about something that cannot happen?
> >> Hi Rafael,
> >> 	I mean the "remove" method of each PCI device, and the "power" method
> >> of PCI hotplug slot here.
> >> 	These methods may be used to remove P2P bridges with associated ACPIPHP
> >> hotplug slots, which in turn will cause invoking of
> >> unregister_hotplug_dock_device().
> >> 	So theoretical we may trigger the bug by undocking while repeatedly
> >> adding/removing P2P bridges with ACPIPHP hotplug slot through PCI
> >> "rescan" and "remove" sysfs interface,
> > 
> > Why don't we make these things take acpi_scan_lock upfront, then?
> Hi Rafael,
>     Seems we can't rely on acpi_scan_lock here, it may cause another
> deadlock scenario:
> 1) thread 1 acquired the acpi_scan_lock and tries to destroy all sysfs
> interfaces for PCI devices.
> 2) thread 2 opens a PCI sysfs which then tries to acquire the
> acpi_scan_lock.

Well, maybe, but you didn't explain how this was going to happen.  What code
paths are involved, etc.

Quite frankly, I've already run out of patience, sorry about that.  It looks
like I need to go through the code and understand all of these problems myself.
Yes, it will take time.

Thanks,
Rafael


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

Re: [BUGFIX v2 2/4] ACPI, DOCK: resolve possible deadlock scenarios

[Jiang Liu]

On 06/16/2013 04:17 AM, Rafael J. Wysocki wrote:
> On Saturday, June 15, 2013 09:44:28 AM Jiang Liu wrote:
>> On Sat 15 Jun 2013 06:21:02 AM CST, Rafael J. Wysocki wrote:
[...]
>>> Can we please relax a bit and possibly take a step back?
>>>
>>> So since your last reply to me wasn't particularly helpful, I went through the
>>> code in dock.c and acpiphp_glue.c and I simply think that the whole
>>> hotplug_list thing is simply redundant.
>>>
>>> It looks like instead of using it (or the klist in this patch), we can add a
>>> "hotlpug_device" flag to dock_dependent_device and set that flag instead of
>>> adding dd to hotplug_devices or clear it instead of removing dd from that list.
>>>
>>> That would allow us to avoid the deadlock, because we wouldn't need the hp_lock
>>> any more and perhaps we could make the code simpler instead of making it more
>>> complex.
>>>
>>> How does that sound?
>>>
>>> Rafael
>> Hi Rafael,
>>        Thanks for comments! It would be great if we could kill the 
>> hotplug_devices
>> list so thing gets simple. But there are still some special cases:(
>>
>> As you have mentioned,  ds->hp_lock is used to make both addition and 
>> removal
>> of hotplug devices wait for us to complete walking ds->hotplug_devices.
>> So it acts as two roles:
>> 1) protect the hotplug_devices list,
>> 2) serialize unregister_hotplug_dock_device() and 
>> hotplug_dock_devices() so
>> the dock driver doesn't access registered handler and associated data 
>> structure
>> once returing from unregister_hotplug_dock_device().
> 
> When it returns from unregister_hotplug_dock_device(), nothing prevents it
> from accessing whatever it wants, because ds->hp_lock is not used outside
> of the add/del and hotplug_dock_devices().  So, the actual role of
> ds->hp_lock (not the one that it is supposed to play, but the real one)
> is to prevent addition/deletion from happening when hotplug_dock_devices()
> is running.  [Yes, it does protect the list, but since the list is in fact
> unnecessary, that doesn't matter.]
Hi Rafael,
With current implementation function dock_add_hotplug_device(),
dock_del_hotplug_device(), hotplug_dock_devices() and dock_event()
access hotplug_devices list, registered callback(ops) and associated
data(context).

Caller may free the associated data(context) once returned from function
unregister_hotplug_dock_device(), so the dock core shouldn't access the
data(context) any more after returning from
unregister_hotplug_dock_device(). With current implementation, this is
guaranteed by acquiring ds->hp_lock in function
dock_del_hotplug_device(). But there's another issue with current
implementation here, we should use ds->hp_lock to protect dock_event() too.

> 
>> If we simply use a flag to mark presence of registered callback, we 
>> can't achieve the second goal.
> 
> I don't mean using the flag *alone*.
> 
>> Take the sony laptop as an example. It has several PCI 
>> hotplug
>> slot associated with the dock station:
>> [   28.829316] acpiphp_glue: _handle_hotplug_event_func: Bus check 
>> notify on \_SB_.PCI0.RP07.LPMB
>> [   30.174964] acpiphp_glue: _handle_hotplug_event_func: Bus check 
>> notify on \_SB_.PCI0.RP07.LPMB.LPM0
>> [   30.174973] acpiphp_glue: _handle_hotplug_event_func: Bus check 
>> notify on \_SB_.PCI0.RP07.LPMB.LPM1
>> [   30.174979] acpiphp_glue: _handle_hotplug_event_func: Btus check 
>> notify on \_SB_.PCI0.RP07.LPMB.LPM2
>> [   30.174985] acpiphp_glue: _handle_hotplug_event_func: Bus check 
>> notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR0.GFXA
>> [   30.175020] acpiphp_glue: _handle_hotplug_event_func: Bus check 
>> notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR0.GHDA
>> [   30.175040] acpiphp_glue: _handle_hotplug_event_func: Bus check 
>> notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR1.LPCI.LPC0.DLAN
>> [   30.175050] acpiphp_glue: _handle_hotplug_event_func: Bus check 
>> notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR1.LPCI.LPC1.DODD
>> [   30.175060] acpiphp_glue: _handle_hotplug_event_func: Bus check 
>> notify on \_SB_.PCI0.RP07.LPMB.LPM2.LPRI.LPR1.LPCI.LPC2.DUSB
>>
>> So it still has some race windows if we undock the station while 
>> repeatedly rescanning/removing
>> the PCI bus for \_SB_.PCI0.RP07.LPMB.LPM0 through sysfs interfaces. For 
>> example, thread 1 is
>> handling undocking event, walking the dependent device list and 
>> invoking registered callback
>> handler with associated data. While that, thread 2 may step in to 
>> unregister the callback for
>>  \_SB_.PCI0.RP07.LPMB.LPM0. Then thread 1 may cause access-after-free 
>> issue.
> 
> That should be handled in acpiphp_glue instead of dock.  What you're trying to
> do is to make dock work around synchronization issues in the acpiphp driver.
I'm not sure whether we could solve this issue from acpiphp_glue side.
If dock driver can't guarantee that it doesn't access registered
handler(ops) and associated data(context) after returning from
unregister_hotplug_dock_device(), it will be hard for acpiphp_glue to
manage the data structure(context). So I introduced a "put" method into
the acpi_dock_ops to help manage lifecycle of "context".

> 
>> The klist patch solves this issue by adding a "put" callback method to 
>> explicitly notify
>> dock client that the dock core has done with previously registered 
>> handler and associated
>> data.
> 
> Honestly, don't you think this is overly compilcated?
Yeah, I must admire that it's really a little over complicated, but I
can't find a simpler solution here:(

> 
> Rafael
> 
> 

[BUGFIX v2 4/4] ACPIPHP: fix bug 56531 Sony VAIO VPCZ23A4R: can't assign mem/io after docking

[Jiang Liu]

Please refer to https://bugzilla.kernel.org/show_bug.cgi?id=56531 for
more information.

This issue is caused by differences in PCI resource assignment between
boot time and runtime hotplug. On x86 platforms, OS respects PCI
resource assignment from BIOS and only reassign resources for unassigned
BARs at boot time. But with acpiphp, it ignores BIOS resource assignment
and reassign all resources by itself.

If we have enough resources, reassigning all PCI resources should work
too, but it may fail if we are under resource constraints. On the other
handle, current PCI MMIO alignment algorithm may waste huge MMIO address
space if we have some PCI devices with huge MMIO BARs.

On this Sony laptop, BIOS allocates limited MMIO resources for the dock
station and the dock station has a gfx which has a 256MB MMIO BAR.
So current acpiphp driver fails to allocate resources for most devices
on the dock station.

So change acpiphp driver to follow boot time behavior to respect BIOS
resource assignment.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Reported-by: Alexander E. Patrakov <patrakov@gmail.com>
Tested-by: Alexander E. Patrakov <patrakov@gmail.com>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
Cc: linux-acpi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: <stable@vger.kernel.org>
---
 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(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index a65203b..f4a53e9 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -687,6 +687,7 @@ static int __ref enable_device(struct acpiphp_slot *slot)
 	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;
@@ -711,13 +712,15 @@ static int __ref enable_device(struct acpiphp_slot *slot)
 				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);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 68678ed..d1182c4 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -202,6 +202,11 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 		    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
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 16abaaa..d254e23 100644
--- a/drivers/pci/setup-bus.c
+++ b/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 pci_bus *bus)
 }
 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;
-- 
1.8.1.2

Re: [BUGFIX v2 4/4] ACPIPHP: fix bug 56531 Sony VAIO VPCZ23A4R: can't assign mem/io after docking

[Yinghai Lu]

On Fri, Jun 14, 2013 at 12:28 PM, Jiang Liu <jiang.liu@huawei.com> wrote:
> Please refer to https://bugzilla.kernel.org/show_bug.cgi?id=56531 for
> more information.
>
> This issue is caused by differences in PCI resource assignment between
> boot time and runtime hotplug. On x86 platforms, OS respects PCI
> resource assignment from BIOS and only reassign resources for unassigned
> BARs at boot time. But with acpiphp, it ignores BIOS resource assignment
> and reassign all resources by itself.
>
> If we have enough resources, reassigning all PCI resources should work
> too, but it may fail if we are under resource constraints. On the other
> handle, current PCI MMIO alignment algorithm may waste huge MMIO address
> space if we have some PCI devices with huge MMIO BARs.
>
> On this Sony laptop, BIOS allocates limited MMIO resources for the dock
> station and the dock station has a gfx which has a 256MB MMIO BAR.
> So current acpiphp driver fails to allocate resources for most devices
> on the dock station.
>
> So change acpiphp driver to follow boot time behavior to respect BIOS
> resource assignment.
>
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> Reported-by: Alexander E. Patrakov <patrakov@gmail.com>
> Tested-by: Alexander E. Patrakov <patrakov@gmail.com>
> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> Cc: linux-acpi@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: <stable@vger.kernel.org>

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

> ---
>  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(-)
>
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index a65203b..f4a53e9 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -687,6 +687,7 @@ static int __ref enable_device(struct acpiphp_slot *slot)
>         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;
> @@ -711,13 +712,15 @@ static int __ref enable_device(struct acpiphp_slot *slot)
>                                 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);
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 68678ed..d1182c4 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -202,6 +202,11 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
>                     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
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 16abaaa..d254e23 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/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 pci_bus *bus)
>  }
>  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;
> --
> 1.8.1.2
>

Re: [BUGFIX v2 4/4] ACPIPHP: fix bug 56531 Sony VAIO VPCZ23A4R: can't assign mem/io after docking

[Rafael J. Wysocki]

On Saturday, June 15, 2013 03:28:01 AM Jiang Liu wrote:
> Please refer to https://bugzilla.kernel.org/show_bug.cgi?id=56531 for
> more information.
> 
> This issue is caused by differences in PCI resource assignment between
> boot time and runtime hotplug. On x86 platforms, OS respects PCI
> resource assignment from BIOS and only reassign resources for unassigned
> BARs at boot time. But with acpiphp, it ignores BIOS resource assignment
> and reassign all resources by itself.
> 
> If we have enough resources, reassigning all PCI resources should work
> too, but it may fail if we are under resource constraints. On the other
> handle, current PCI MMIO alignment algorithm may waste huge MMIO address
> space if we have some PCI devices with huge MMIO BARs.
> 
> On this Sony laptop, BIOS allocates limited MMIO resources for the dock
> station and the dock station has a gfx which has a 256MB MMIO BAR.
> So current acpiphp driver fails to allocate resources for most devices
> on the dock station.
> 
> So change acpiphp driver to follow boot time behavior to respect BIOS
> resource assignment.
> 
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> Reported-by: Alexander E. Patrakov <patrakov@gmail.com>
> Tested-by: Alexander E. Patrakov <patrakov@gmail.com>
> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> Cc: linux-acpi@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: <stable@vger.kernel.org>

The code is fine as far as I'm concerned, but I have one request. ->

> ---
>  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(-)
> 
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index a65203b..f4a53e9 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -687,6 +687,7 @@ static int __ref enable_device(struct acpiphp_slot *slot)
>  	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;
> @@ -711,13 +712,15 @@ static int __ref enable_device(struct acpiphp_slot *slot)
>  				max = pci_scan_bridge(bus, dev, max, pass);
>  				if (pass && dev->subordinate) {
>  					check_hotplug_bridge(slot, dev);
> -					pci_bus_size_bridges(dev->subordinate);

Please add a comment explaining why we need to do this here.

> +					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);
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 68678ed..d1182c4 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -202,6 +202,11 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
>  		    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
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 16abaaa..d254e23 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/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 pci_bus *bus)
>  }
>  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;

Thanks,
Rafael


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