Alexander E. Patrakov <[email protected]> reports two bugs related to
dock station support on Sony VAIO VPCZ23A4R. Actually there are at least
four bugs related to Sony VAIO VPCZ23A4R dock support.
1) can't correctly detect hotplug slot for dock state
2) resource leak on undocking
3) resource allocation failure for dock devices
4) one bug in intel_snd_hda driver
The first patch fixes issue 1, and the second patch fixes issue 2.
These two patches, if accepted, should be material for stable branches
too.
Patch 3-9 are code improvement for ACPI and dock driver.
I have found the root cause for issue three, but still working on
solutions, and seems can't be solve in short time. So please help
to review and test patches for issue 1) and 2) first.
Hi Alexander, could you please help to test the whole patchset?
Jiang Liu (9):
ACPI, DOCK: initialize dock subsystem before scanning PCI root buses
ACPIPHP: fix device destroying order issue when handling dock
notification
ACPI, DOCK: clean up unused module related code
ACPI, DOCK: avoid initializing acpi_dock_notifier_list multiple times
ACPI, DOCK: kill redundant spin lock in dock device object
ACPI, DOCK: mark initialization functions with __init
ACPI, DOCK: simplify implementation of dock_create_acpi_device()
ACPI: introduce several helper functions
ACPI: use new helper functions to simpilify code
drivers/acpi/dock.c | 193 +++++++++----------------------------
drivers/acpi/internal.h | 5 +
drivers/acpi/scan.c | 54 +++--------
drivers/acpi/utils.c | 74 ++++++++++++++
drivers/pci/hotplug/acpiphp_glue.c | 47 ++++-----
include/acpi/acpi_bus.h | 5 +
6 files changed, 162 insertions(+), 216 deletions(-)
--
1.8.1.2
Current ACPI glue logic expects that physical devices are destroyed
before destroying companion ACPI devices, otherwise it will break the
ACPI unbind logic and cause following warning messages:
[ 185.026073] usb usb5: Oops, 'acpi_handle' corrupt
[ 185.035150] pci 0000:1b:00.0: Oops, 'acpi_handle' corrupt
[ 185.035515] pci 0000:18:02.0: Oops, 'acpi_handle' corrupt
[ 180.013656] port1: Oops, 'acpi_handle' corrupt
Please refer to https://bugzilla.kernel.org/attachment.cgi?id=104321
for full log message.
Above warning messages are caused by following scenario:
1) acpi_dock_notifier_call() queues a task (T1) onto kacpi_hotplug_wq
2) kacpi_hotplug_wq handles T1, which invokes acpi_dock_deferred_cb()
->dock_notify()-> handle_eject_request()->hotplug_dock_devices()
3) hotplug_dock_devices() first invokes registered hotplug callbacks to
destroy physical devices, then destroys all affected ACPI devices.
Everything seems perfect until now. But the acpiphp dock notification
handler will queue another task (T2) onto kacpi_hotplug_wq to really
destroy affected physical devices.
4) kacpi_hotplug_wq finishes T1, and all affected ACPI devices have
been destroyed.
5) kacpi_hotplug_wq handles T2, which destroys all affected physical
devices.
So it breaks ACPI glue logic's expection because ACPI devices are destroyed
in step 3 and physical devices are destroyed in step 5.
Signed-off-by: Jiang Liu <[email protected]>
Reported-by: Alexander E. Patrakov <[email protected]>
Cc: Bjorn Helgaas <[email protected]>
Cc: Yinghai Lu <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
Hi Bjorn and Rafael,
The recursive lock changes haven't been tested yet, need help
from Alexander for testing.
---
drivers/acpi/dock.c | 33 +++++++++++++++++++++++++++------
drivers/pci/hotplug/acpiphp_glue.c | 32 ++++++++++++++++++--------------
2 files changed, 45 insertions(+), 20 deletions(-)
diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
index 02b0563..79c8d9e 100644
--- a/drivers/acpi/dock.c
+++ b/drivers/acpi/dock.c
@@ -65,6 +65,7 @@ struct dock_station {
u32 flags;
spinlock_t dd_lock;
struct mutex hp_lock;
+ struct task_struct *owner;
struct list_head dependent_devices;
struct list_head hotplug_devices;
@@ -131,9 +132,13 @@ 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);
+ if (mutex_is_locked(&ds->hp_lock) && ds->owner == current) {
+ list_add_tail(&dd->hotplug_list, &ds->hotplug_devices);
+ } else {
+ mutex_lock(&ds->hp_lock);
+ list_add_tail(&dd->hotplug_list, &ds->hotplug_devices);
+ mutex_unlock(&ds->hp_lock);
+ }
}
/**
@@ -147,9 +152,13 @@ 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);
+ if (mutex_is_locked(&ds->hp_lock) && ds->owner == current) {
+ list_del_init(&dd->hotplug_list);
+ } else {
+ mutex_lock(&ds->hp_lock);
+ list_del_init(&dd->hotplug_list);
+ mutex_unlock(&ds->hp_lock);
+ }
}
/**
@@ -355,7 +364,17 @@ static void hotplug_dock_devices(struct dock_station *ds, u32 event)
{
struct dock_dependent_device *dd;
+ /*
+ * There is a deadlock scenario as below:
+ * hotplug_dock_devices()
+ * mutex_lock(&ds->hp_lock)
+ * dd->ops->handler()
+ * register_hotplug_dock_device()
+ * mutex_lock(&ds->hp_lock)
+ * So we need recursive lock scematics here, do it by ourselves.
+ */
mutex_lock(&ds->hp_lock);
+ ds->owner = current;
/*
* First call driver specific hotplug functions
@@ -376,6 +395,8 @@ static void hotplug_dock_devices(struct dock_station *ds, u32 event)
else
dock_create_acpi_device(dd->handle);
}
+
+ ds->owner = NULL;
mutex_unlock(&ds->hp_lock);
}
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
Initialize acpi_dock_notifier_list from function acpi_dock_init()
instead of dock_add() to avoid initializing it multiple times.
Signed-off-by: Jiang Liu <[email protected]>
Cc: Shaohua Li <[email protected]>
Cc: Len Brown <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/acpi/dock.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
index 50e38b7..8e578aa 100644
--- a/drivers/acpi/dock.c
+++ b/drivers/acpi/dock.c
@@ -967,7 +967,6 @@ static int __init dock_add(acpi_handle handle)
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);
/* we want the dock device to send uevents */
@@ -1038,6 +1037,7 @@ int __init acpi_dock_init(void)
return 0;
}
+ ATOMIC_INIT_NOTIFIER_HEAD(&dock_notifier_list);
register_acpi_bus_notifier(&dock_acpi_notifier);
pr_info(PREFIX "%s: %d docks/bays found\n",
ACPI_DOCK_DRIVER_DESCRIPTION, dock_station_count);
--
1.8.1.2
Mark all initialization functions with __init to reduce memory
consumption at runtime.
Signed-off-by: Jiang Liu <[email protected]>
Cc: Shaohua Li <[email protected]>
Cc: Len Brown <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/acpi/dock.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
index cd2d5df..da3314d 100644
--- a/drivers/acpi/dock.c
+++ b/drivers/acpi/dock.c
@@ -94,7 +94,7 @@ struct dock_dependent_device {
*
* Add the dependent device to the dock's dependent device list.
*/
-static int
+static int __init
add_dock_dependent_device(struct dock_station *ds, acpi_handle handle)
{
struct dock_dependent_device *dd;
@@ -192,7 +192,7 @@ static int is_dock(acpi_handle handle)
return 1;
}
-static int is_ejectable(acpi_handle handle)
+static int __init is_ejectable(acpi_handle handle)
{
acpi_status status;
acpi_handle tmp;
@@ -203,7 +203,7 @@ static int is_ejectable(acpi_handle handle)
return 1;
}
-static int is_ata(acpi_handle handle)
+static int __init is_ata(acpi_handle handle)
{
acpi_handle tmp;
@@ -216,7 +216,7 @@ static int is_ata(acpi_handle handle)
return 0;
}
-static int is_battery(acpi_handle handle)
+static int __init is_battery(acpi_handle handle)
{
struct acpi_device_info *info;
int ret = 1;
@@ -232,7 +232,7 @@ static int is_battery(acpi_handle handle)
return ret;
}
-static int is_ejectable_bay(acpi_handle handle)
+static int __init is_ejectable_bay(acpi_handle handle)
{
acpi_handle phandle;
@@ -809,7 +809,7 @@ static struct notifier_block dock_acpi_notifier = {
* check to see if an object has an _EJD method. If it does, then it
* will see if it is dependent on the dock station.
*/
-static acpi_status
+static acpi_status __init
find_dock_devices(acpi_handle handle, u32 lvl, void *context, void **rv)
{
acpi_status status;
--
1.8.1.2
ACPI dock driver can't be built as a module any more, so clean up
module related code.
Signed-off-by: Jiang Liu <[email protected]>
Cc: Shaohua Li <[email protected]>
Cc: Len Brown <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/acpi/dock.c | 41 -----------------------------------------
1 file changed, 41 deletions(-)
diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
index 79c8d9e..50e38b7 100644
--- a/drivers/acpi/dock.c
+++ b/drivers/acpi/dock.c
@@ -53,12 +53,6 @@ MODULE_PARM_DESC(immediate_undock, "1 (default) will cause the driver to "
static struct atomic_notifier_head dock_notifier_list;
-static const struct acpi_device_id dock_device_ids[] = {
- {"LNXDOCK", 0},
- {"", 0},
-};
-MODULE_DEVICE_TABLE(acpi, dock_device_ids);
-
struct dock_station {
acpi_handle handle;
unsigned long last_dock_time;
@@ -1013,30 +1007,6 @@ err_unregister:
}
/**
- * dock_remove - free up resources related to the dock station
- */
-static int dock_remove(struct dock_station *ds)
-{
- struct dock_dependent_device *dd, *tmp;
- struct platform_device *dock_device = ds->dock_device;
-
- if (!dock_station_count)
- return 0;
-
- /* remove dependent devices */
- list_for_each_entry_safe(dd, tmp, &ds->dependent_devices, list)
- kfree(dd);
-
- list_del(&ds->sibling);
-
- /* cleanup sysfs */
- sysfs_remove_group(&dock_device->dev.kobj, &dock_attribute_group);
- platform_device_unregister(dock_device);
-
- return 0;
-}
-
-/**
* find_dock_and_bay - look for dock stations and bays
* @handle: acpi handle of a device
* @lvl: unused
@@ -1073,14 +1043,3 @@ int __init acpi_dock_init(void)
ACPI_DOCK_DRIVER_DESCRIPTION, dock_station_count);
return 0;
}
-
-static void __exit dock_exit(void)
-{
- struct dock_station *tmp, *dock_station;
-
- unregister_acpi_bus_notifier(&dock_acpi_notifier);
- list_for_each_entry_safe(dock_station, tmp, &dock_stations, sibling)
- dock_remove(dock_station);
-}
-
-module_exit(dock_exit);
--
1.8.1.2
All dock device related data structures are created during driver
initialization, so kill the redundant spin lock in dock device object.
Signed-off-by: Jiang Liu <[email protected]>
Cc: Shaohua Li <[email protected]>
Cc: Len Brown <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/acpi/dock.c | 15 +++------------
1 file changed, 3 insertions(+), 12 deletions(-)
diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
index 8e578aa..cd2d5df 100644
--- a/drivers/acpi/dock.c
+++ b/drivers/acpi/dock.c
@@ -57,7 +57,6 @@ struct dock_station {
acpi_handle handle;
unsigned long last_dock_time;
u32 flags;
- spinlock_t dd_lock;
struct mutex hp_lock;
struct task_struct *owner;
struct list_head dependent_devices;
@@ -107,10 +106,7 @@ 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);
- spin_unlock(&ds->dd_lock);
return 0;
}
@@ -168,14 +164,10 @@ find_dock_dependent_device(struct dock_station *ds, acpi_handle handle)
{
struct dock_dependent_device *dd;
- spin_lock(&ds->dd_lock);
- list_for_each_entry(dd, &ds->dependent_devices, list) {
- if (handle == dd->handle) {
- spin_unlock(&ds->dd_lock);
+ list_for_each_entry(dd, &ds->dependent_devices, list)
+ if (handle == dd->handle)
return dd;
- }
- }
- spin_unlock(&ds->dd_lock);
+
return NULL;
}
@@ -964,7 +956,6 @@ static int __init dock_add(acpi_handle handle)
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);
INIT_LIST_HEAD(&dock_station->dependent_devices);
--
1.8.1.2
The return value of dock_create_acpi_device() is unused, so simplify
it by returning void.
Signed-off-by: Jiang Liu <[email protected]>
Cc: Shaohua Li <[email protected]>
Cc: Len Brown <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/acpi/dock.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
index da3314d..72cf97e 100644
--- a/drivers/acpi/dock.c
+++ b/drivers/acpi/dock.c
@@ -299,10 +299,8 @@ static int dock_present(struct dock_station *ds)
* handle if one does not exist already. This should cause
* acpi to scan for drivers for the given devices, and call
* matching driver's add routine.
- *
- * Returns a pointer to the acpi_device corresponding to the handle.
*/
-static struct acpi_device * dock_create_acpi_device(acpi_handle handle)
+static void dock_create_acpi_device(acpi_handle handle)
{
struct acpi_device *device;
int ret;
@@ -315,10 +313,7 @@ static struct acpi_device * dock_create_acpi_device(acpi_handle handle)
ret = acpi_bus_scan(handle);
if (ret)
pr_debug("error adding bus, %x\n", -ret);
-
- acpi_bus_get_device(handle, &device);
}
- return device;
}
/**
--
1.8.1.2
Introduce several helper functions, which will be used to simplify code.
Signed-off-by: Jiang Liu <[email protected]>
---
drivers/acpi/utils.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++
include/acpi/acpi_bus.h | 5 ++++
2 files changed, 79 insertions(+)
diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index 74437130..cb66fce 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -495,3 +495,77 @@ acpi_handle_printk(const char *level, acpi_handle handle, const char *fmt, ...)
kfree(buffer.pointer);
}
EXPORT_SYMBOL(acpi_handle_printk);
+
+/**
+ * acpi_evaluate_ej0: Evaluate _EJ0 method for hotplug operations
+ * @handle: ACPI device handle
+ *
+ * Evaluate device's _EJ0 method for hotplug operations.
+ */
+acpi_status acpi_evaluate_ej0(acpi_handle handle)
+{
+ acpi_status status;
+ union acpi_object arg;
+ struct acpi_object_list arg_list;
+
+ arg.type = ACPI_TYPE_INTEGER;
+ arg.integer.value = 1;
+ arg_list.count = 1;
+ arg_list.pointer = &arg;
+ status = acpi_evaluate_object(handle, "_EJ0", &arg_list, NULL);
+ if (status == AE_NOT_FOUND) {
+ acpi_handle_warn(handle, "No _EJ0 support for device\n");
+ } else if (ACPI_FAILURE(status)) {
+ acpi_handle_warn(handle, "Eject failed (0x%x)\n", status);
+ }
+
+ return status;
+}
+
+/**
+ * acpi_evaluate_lck: Evaluate _LCK method to lock/unlock device
+ * @handle: ACPI device handle
+ * @lock: lock device if non-zero, otherwise unlock device
+ *
+ * Evaluate device's _LCK method if present to lock/unlock device
+ */
+acpi_status acpi_evaluate_lck(acpi_handle handle, int lock)
+{
+ acpi_status status;
+ union acpi_object arg;
+ struct acpi_object_list arg_list;
+
+ arg_list.count = 1;
+ arg_list.pointer = &arg;
+ arg.type = ACPI_TYPE_INTEGER;
+ arg.integer.value = !!lock;
+ status = acpi_evaluate_object(handle, "_LCK", &arg_list, NULL);
+ if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
+ if (lock)
+ acpi_handle_warn(handle,
+ "Locking device failed (0x%x)\n", status);
+ else
+ acpi_handle_warn(handle,
+ "Unlocking device failed (0x%x)\n", status);
+ }
+
+ return status;
+}
+
+/**
+ * acpi_has_method: Check whether @handle has a method named @name
+ * @handle: ACPI device handle
+ * @name: name of object or method
+ *
+ * Check whether @handle has a method named @name.
+ */
+int acpi_has_method(acpi_handle handle, char *name)
+{
+ acpi_status status;
+ acpi_handle tmp;
+
+ status = acpi_get_handle(handle, name, &tmp);
+
+ return ACPI_FAILURE(status) ? 0 : 1;
+}
+EXPORT_SYMBOL(acpi_has_method);
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 636c59f..5df5f89 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -56,6 +56,11 @@ acpi_evaluate_hotplug_ost(acpi_handle handle, u32 source_event,
acpi_status
acpi_get_physical_device_location(acpi_handle handle, struct acpi_pld_info **pld);
+
+acpi_status acpi_evaluate_ej0(acpi_handle handle);
+acpi_status acpi_evaluate_lck(acpi_handle handle, int lock);
+int acpi_has_method(acpi_handle handle, char *name);
+
#ifdef CONFIG_ACPI
#include <linux/proc_fs.h>
--
1.8.1.2
Use new helper functions to simpilify ACPI dock, acpiphp code.
Signed-off-by: Jiang Liu <[email protected]>
---
drivers/acpi/dock.c | 78 ++++----------------------------------
drivers/acpi/scan.c | 53 ++++++--------------------
drivers/pci/hotplug/acpiphp_glue.c | 15 ++------
3 files changed, 21 insertions(+), 125 deletions(-)
diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
index 72cf97e..1811f4f 100644
--- a/drivers/acpi/dock.c
+++ b/drivers/acpi/dock.c
@@ -181,26 +181,14 @@ find_dock_dependent_device(struct dock_station *ds, acpi_handle handle)
* If an acpi object has a _DCK method, then it is by definition a dock
* station, so return true.
*/
-static int is_dock(acpi_handle handle)
+static inline int is_dock(acpi_handle handle)
{
- acpi_status status;
- acpi_handle tmp;
-
- status = acpi_get_handle(handle, "_DCK", &tmp);
- if (ACPI_FAILURE(status))
- return 0;
- return 1;
+ return acpi_has_method(handle, "_DCK");
}
-static int __init is_ejectable(acpi_handle handle)
+static inline int is_ejectable(acpi_handle handle)
{
- acpi_status status;
- acpi_handle tmp;
-
- status = acpi_get_handle(handle, "_EJ0", &tmp);
- if (ACPI_FAILURE(status))
- return 0;
- return 1;
+ return acpi_has_method(handle, "_EJ0");
}
static int __init is_ata(acpi_handle handle)
@@ -409,37 +397,6 @@ static void dock_event(struct dock_station *ds, u32 event, int num)
}
/**
- * eject_dock - respond to a dock eject request
- * @ds: the dock station
- *
- * This is called after _DCK is called, to execute the dock station's
- * _EJ0 method.
- */
-static void eject_dock(struct dock_station *ds)
-{
- struct acpi_object_list arg_list;
- union acpi_object arg;
- acpi_status status;
- acpi_handle tmp;
-
- /* all dock devices should have _EJ0, but check anyway */
- status = acpi_get_handle(ds->handle, "_EJ0", &tmp);
- if (ACPI_FAILURE(status)) {
- pr_debug("No _EJ0 support for dock device\n");
- return;
- }
-
- arg_list.count = 1;
- arg_list.pointer = &arg;
- arg.type = ACPI_TYPE_INTEGER;
- arg.integer.value = 1;
-
- status = acpi_evaluate_object(ds->handle, "_EJ0", &arg_list, NULL);
- if (ACPI_FAILURE(status))
- pr_debug("Failed to evaluate _EJ0!\n");
-}
-
-/**
* handle_dock - handle a dock event
* @ds: the dock station
* @dock: to dock, or undock - that is the question
@@ -499,27 +456,6 @@ static inline void complete_undock(struct dock_station *ds)
ds->flags &= ~(DOCK_UNDOCKING);
}
-static void dock_lock(struct dock_station *ds, int lock)
-{
- struct acpi_object_list arg_list;
- union acpi_object arg;
- acpi_status status;
-
- arg_list.count = 1;
- arg_list.pointer = &arg;
- arg.type = ACPI_TYPE_INTEGER;
- arg.integer.value = !!lock;
- status = acpi_evaluate_object(ds->handle, "_LCK", &arg_list, NULL);
- if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
- if (lock)
- acpi_handle_warn(ds->handle,
- "Locking device failed (0x%x)\n", status);
- else
- acpi_handle_warn(ds->handle,
- "Unlocking device failed (0x%x)\n", status);
- }
-}
-
/**
* dock_in_progress - see if we are in the middle of handling a dock event
* @ds: the dock station
@@ -653,8 +589,8 @@ static int handle_eject_request(struct dock_station *ds, u32 event)
hotplug_dock_devices(ds, ACPI_NOTIFY_EJECT_REQUEST);
undock(ds);
- dock_lock(ds, 0);
- eject_dock(ds);
+ acpi_evaluate_lck(ds->handle, 0);
+ acpi_evaluate_ej0(ds->handle);
if (dock_present(ds)) {
acpi_handle_err(ds->handle, "Unable to undock!\n");
return -EBUSY;
@@ -713,7 +649,7 @@ static void dock_notify(acpi_handle handle, u32 event, void *data)
hotplug_dock_devices(ds, event);
complete_dock(ds);
dock_event(ds, event, DOCK_EVENT);
- dock_lock(ds, 1);
+ acpi_evaluate_lck(ds->handle, 1);
acpi_update_all_gpes();
break;
}
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 4148163..3372505 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -123,9 +123,6 @@ static DEVICE_ATTR(modalias, 0444, acpi_device_modalias_show, NULL);
static int acpi_scan_hot_remove(struct acpi_device *device)
{
acpi_handle handle = device->handle;
- acpi_handle not_used;
- struct acpi_object_list arg_list;
- union acpi_object arg;
acpi_status status;
unsigned long long sta;
@@ -144,31 +141,12 @@ static int acpi_scan_hot_remove(struct acpi_device *device)
put_device(&device->dev);
device = NULL;
- if (ACPI_SUCCESS(acpi_get_handle(handle, "_LCK", ¬_used))) {
- arg_list.count = 1;
- arg_list.pointer = &arg;
- arg.type = ACPI_TYPE_INTEGER;
- arg.integer.value = 0;
- acpi_evaluate_object(handle, "_LCK", &arg_list, NULL);
- }
-
- arg_list.count = 1;
- arg_list.pointer = &arg;
- arg.type = ACPI_TYPE_INTEGER;
- arg.integer.value = 1;
-
- /*
- * TBD: _EJD support.
- */
- status = acpi_evaluate_object(handle, "_EJ0", &arg_list, NULL);
- if (ACPI_FAILURE(status)) {
- if (status == AE_NOT_FOUND) {
- return -ENODEV;
- } else {
- acpi_handle_warn(handle, "Eject failed (0x%x)\n",
- status);
- return -EIO;
- }
+ acpi_evaluate_lck(handle, 0);
+ status = acpi_evaluate_ej0(handle);
+ if (status == AE_NOT_FOUND) {
+ return -ENODEV;
+ } else if (ACPI_FAILURE(status)) {
+ return -EIO;
}
/*
@@ -536,7 +514,6 @@ static int acpi_device_setup_files(struct acpi_device *dev)
{
struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
acpi_status status;
- acpi_handle temp;
unsigned long long sun;
int result = 0;
@@ -562,8 +539,7 @@ static int acpi_device_setup_files(struct acpi_device *dev)
/*
* If device has _STR, 'description' file is created
*/
- status = acpi_get_handle(dev->handle, "_STR", &temp);
- if (ACPI_SUCCESS(status)) {
+ if (acpi_has_method(dev->handle, "_STR")) {
status = acpi_evaluate_object(dev->handle, "_STR",
NULL, &buffer);
if (ACPI_FAILURE(status))
@@ -593,8 +569,7 @@ static int acpi_device_setup_files(struct acpi_device *dev)
* If device has _EJ0, 'eject' file is created that is used to trigger
* hot-removal function from userland.
*/
- status = acpi_get_handle(dev->handle, "_EJ0", &temp);
- if (ACPI_SUCCESS(status)) {
+ if (acpi_has_method(dev->handle, "_EJ0")) {
result = device_create_file(&dev->dev, &dev_attr_eject);
if (result)
return result;
@@ -616,9 +591,6 @@ end:
static void acpi_device_remove_files(struct acpi_device *dev)
{
- acpi_status status;
- acpi_handle temp;
-
if (dev->flags.power_manageable) {
device_remove_file(&dev->dev, &dev_attr_power_state);
if (dev->power.flags.power_resources)
@@ -629,20 +601,17 @@ static void acpi_device_remove_files(struct acpi_device *dev)
/*
* If device has _STR, remove 'description' file
*/
- status = acpi_get_handle(dev->handle, "_STR", &temp);
- if (ACPI_SUCCESS(status)) {
+ if (acpi_has_method(dev->handle, "_STR")) {
kfree(dev->pnp.str_obj);
device_remove_file(&dev->dev, &dev_attr_description);
}
/*
* If device has _EJ0, remove 'eject' file.
*/
- status = acpi_get_handle(dev->handle, "_EJ0", &temp);
- if (ACPI_SUCCESS(status))
+ if (acpi_has_method(dev->handle, "_EJ0"))
device_remove_file(&dev->dev, &dev_attr_eject);
- status = acpi_get_handle(dev->handle, "_SUN", &temp);
- if (ACPI_SUCCESS(status))
+ if (acpi_has_method(dev->handle, "_SUN"))
device_remove_file(&dev->dev, &dev_attr_sun);
if (dev->pnp.unique_id)
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 699b8ca..d0699ed 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -828,23 +828,14 @@ int acpiphp_eject_slot(struct acpiphp_slot *slot)
{
acpi_status status;
struct acpiphp_func *func;
- struct acpi_object_list arg_list;
- union acpi_object arg;
list_for_each_entry(func, &slot->funcs, sibling) {
/* We don't want to call _EJ0 on non-existing functions. */
if ((func->flags & FUNC_HAS_EJ0)) {
- /* _EJ0 method take one argument */
- arg_list.count = 1;
- arg_list.pointer = &arg;
- arg.type = ACPI_TYPE_INTEGER;
- arg.integer.value = 1;
-
- status = acpi_evaluate_object(func->handle, "_EJ0", &arg_list, NULL);
- if (ACPI_FAILURE(status)) {
- warn("%s: _EJ0 failed\n", __func__);
+ status = acpi_evaluate_ej0(func->handle);
+ if (ACPI_FAILURE(status))
return -1;
- } else
+ else
break;
}
}
--
1.8.1.2
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 <[email protected]>
Reported-by: Alexander E. Patrakov <[email protected]>
Tested-by: Alexander E. Patrakov <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: <[email protected]> # 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
2013/6/13 Jiang Liu <[email protected]>:
> Use new helper functions to simpilify ACPI dock, acpiphp code.
Not tested yet, just looking at the code
> - /*
> - * TBD: _EJD support.
> - */
You have removed this comment. Did it become invalid since it has been written?
--
Alexander E. Patrakov
2013/6/13 Jiang Liu <[email protected]>:
> Alexander E. Patrakov <[email protected]> reports two bugs related to
> dock station support on Sony VAIO VPCZ23A4R. Actually there are at least
> four bugs related to Sony VAIO VPCZ23A4R dock support.
> 1) can't correctly detect hotplug slot for dock state
> 2) resource leak on undocking
> 3) resource allocation failure for dock devices
> 4) one bug in intel_snd_hda driver
>
> The first patch fixes issue 1, and the second patch fixes issue 2.
> These two patches, if accepted, should be material for stable branches
> too.
> Patch 3-9 are code improvement for ACPI and dock driver.
>
> I have found the root cause for issue three, but still working on
> solutions, and seems can't be solve in short time. So please help
> to review and test patches for issue 1) and 2) first.
>
> Hi Alexander, could you please help to test the whole patchset?
Tested on top of 3.10-rc5, with no other patches applied.
Unfortunately, lockdep complains on the second docking attempt in the
initially-undocked case. Please see the attached dmesg output.
I have not tested the initially-docked case yet, will do that in 10 minutes.
--
Alexander E. Patrakov
On Friday, June 14, 2013 12:32:24 AM Jiang Liu 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 <[email protected]>
> Reported-by: Alexander E. Patrakov <[email protected]>
> Tested-by: Alexander E. Patrakov <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: <[email protected]> # 3.9+
Can you please send the whole series to linux-acpi next time?
First, because I believe it should go in through the ACPI tree. Second,
because it is kind of useful to have all of the patches in context.
Thanks,
Rafael
> ---
> 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);
> /*
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Friday, June 14, 2013 12:32:24 AM Jiang Liu 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.
This change makes sense to me.
Thanks,
Rafael
> Signed-off-by: Jiang Liu <[email protected]>
> Reported-by: Alexander E. Patrakov <[email protected]>
> Tested-by: Alexander E. Patrakov <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: <[email protected]> # 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);
> /*
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Friday, June 14, 2013 12:32:26 AM Jiang Liu wrote:
> ACPI dock driver can't be built as a module any more, so clean up
> module related code.
>
> Signed-off-by: Jiang Liu <[email protected]>
> Cc: Shaohua Li <[email protected]>
> Cc: Len Brown <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
How exactly does this depend on [2/9]? If it doesn't at all, it should go
after [1/9].
> ---
> drivers/acpi/dock.c | 41 -----------------------------------------
> 1 file changed, 41 deletions(-)
>
> diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
> index 79c8d9e..50e38b7 100644
> --- a/drivers/acpi/dock.c
> +++ b/drivers/acpi/dock.c
> @@ -53,12 +53,6 @@ MODULE_PARM_DESC(immediate_undock, "1 (default) will cause the driver to "
>
> static struct atomic_notifier_head dock_notifier_list;
>
> -static const struct acpi_device_id dock_device_ids[] = {
> - {"LNXDOCK", 0},
> - {"", 0},
> -};
> -MODULE_DEVICE_TABLE(acpi, dock_device_ids);
> -
Don't we actually need the device IDs?
> struct dock_station {
> acpi_handle handle;
> unsigned long last_dock_time;
> @@ -1013,30 +1007,6 @@ err_unregister:
> }
>
> /**
> - * dock_remove - free up resources related to the dock station
> - */
> -static int dock_remove(struct dock_station *ds)
> -{
> - struct dock_dependent_device *dd, *tmp;
> - struct platform_device *dock_device = ds->dock_device;
> -
> - if (!dock_station_count)
> - return 0;
> -
> - /* remove dependent devices */
> - list_for_each_entry_safe(dd, tmp, &ds->dependent_devices, list)
> - kfree(dd);
> -
> - list_del(&ds->sibling);
> -
> - /* cleanup sysfs */
> - sysfs_remove_group(&dock_device->dev.kobj, &dock_attribute_group);
> - platform_device_unregister(dock_device);
> -
> - return 0;
> -}
> -
> -/**
> * find_dock_and_bay - look for dock stations and bays
> * @handle: acpi handle of a device
> * @lvl: unused
> @@ -1073,14 +1043,3 @@ int __init acpi_dock_init(void)
> ACPI_DOCK_DRIVER_DESCRIPTION, dock_station_count);
> return 0;
> }
> -
> -static void __exit dock_exit(void)
> -{
> - struct dock_station *tmp, *dock_station;
> -
> - unregister_acpi_bus_notifier(&dock_acpi_notifier);
> - list_for_each_entry_safe(dock_station, tmp, &dock_stations, sibling)
> - dock_remove(dock_station);
> -}
> -
> -module_exit(dock_exit);
The other changes look OK to me.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Friday, June 14, 2013 12:32:27 AM Jiang Liu wrote:
> Initialize acpi_dock_notifier_list from function acpi_dock_init()
> instead of dock_add() to avoid initializing it multiple times.
Well, makes sense.
Thanks,
Rafael
> Signed-off-by: Jiang Liu <[email protected]>
> Cc: Shaohua Li <[email protected]>
> Cc: Len Brown <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> drivers/acpi/dock.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
> index 50e38b7..8e578aa 100644
> --- a/drivers/acpi/dock.c
> +++ b/drivers/acpi/dock.c
> @@ -967,7 +967,6 @@ static int __init dock_add(acpi_handle handle)
> 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);
>
> /* we want the dock device to send uevents */
> @@ -1038,6 +1037,7 @@ int __init acpi_dock_init(void)
> return 0;
> }
>
> + ATOMIC_INIT_NOTIFIER_HEAD(&dock_notifier_list);
> register_acpi_bus_notifier(&dock_acpi_notifier);
> pr_info(PREFIX "%s: %d docks/bays found\n",
> ACPI_DOCK_DRIVER_DESCRIPTION, dock_station_count);
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Friday, June 14, 2013 12:32:28 AM Jiang Liu wrote:
> All dock device related data structures are created during driver
> initialization, so kill the redundant spin lock in dock device object.
As a cleanup, it definitely makes sense, but does it fix anything?
Rafael
> Signed-off-by: Jiang Liu <[email protected]>
> Cc: Shaohua Li <[email protected]>
> Cc: Len Brown <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> drivers/acpi/dock.c | 15 +++------------
> 1 file changed, 3 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
> index 8e578aa..cd2d5df 100644
> --- a/drivers/acpi/dock.c
> +++ b/drivers/acpi/dock.c
> @@ -57,7 +57,6 @@ struct dock_station {
> acpi_handle handle;
> unsigned long last_dock_time;
> u32 flags;
> - spinlock_t dd_lock;
> struct mutex hp_lock;
> struct task_struct *owner;
> struct list_head dependent_devices;
> @@ -107,10 +106,7 @@ 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);
> - spin_unlock(&ds->dd_lock);
>
> return 0;
> }
> @@ -168,14 +164,10 @@ find_dock_dependent_device(struct dock_station *ds, acpi_handle handle)
> {
> struct dock_dependent_device *dd;
>
> - spin_lock(&ds->dd_lock);
> - list_for_each_entry(dd, &ds->dependent_devices, list) {
> - if (handle == dd->handle) {
> - spin_unlock(&ds->dd_lock);
> + list_for_each_entry(dd, &ds->dependent_devices, list)
> + if (handle == dd->handle)
> return dd;
> - }
> - }
> - spin_unlock(&ds->dd_lock);
> +
> return NULL;
> }
>
> @@ -964,7 +956,6 @@ static int __init dock_add(acpi_handle handle)
> 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);
> INIT_LIST_HEAD(&dock_station->dependent_devices);
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Friday, June 14, 2013 12:32:29 AM Jiang Liu wrote:
> Mark all initialization functions with __init to reduce memory
> consumption at runtime.
Again, this is a *cleanup*, not a fix. Please don't mix cleanups with fixes,
especially with ones you want to go into 3.10, because the cleanups aren't
3.10 material for sure.
Thanks,
Rafael
> Signed-off-by: Jiang Liu <[email protected]>
> Cc: Shaohua Li <[email protected]>
> Cc: Len Brown <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> drivers/acpi/dock.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
> index cd2d5df..da3314d 100644
> --- a/drivers/acpi/dock.c
> +++ b/drivers/acpi/dock.c
> @@ -94,7 +94,7 @@ struct dock_dependent_device {
> *
> * Add the dependent device to the dock's dependent device list.
> */
> -static int
> +static int __init
> add_dock_dependent_device(struct dock_station *ds, acpi_handle handle)
> {
> struct dock_dependent_device *dd;
> @@ -192,7 +192,7 @@ static int is_dock(acpi_handle handle)
> return 1;
> }
>
> -static int is_ejectable(acpi_handle handle)
> +static int __init is_ejectable(acpi_handle handle)
> {
> acpi_status status;
> acpi_handle tmp;
> @@ -203,7 +203,7 @@ static int is_ejectable(acpi_handle handle)
> return 1;
> }
>
> -static int is_ata(acpi_handle handle)
> +static int __init is_ata(acpi_handle handle)
> {
> acpi_handle tmp;
>
> @@ -216,7 +216,7 @@ static int is_ata(acpi_handle handle)
> return 0;
> }
>
> -static int is_battery(acpi_handle handle)
> +static int __init is_battery(acpi_handle handle)
> {
> struct acpi_device_info *info;
> int ret = 1;
> @@ -232,7 +232,7 @@ static int is_battery(acpi_handle handle)
> return ret;
> }
>
> -static int is_ejectable_bay(acpi_handle handle)
> +static int __init is_ejectable_bay(acpi_handle handle)
> {
> acpi_handle phandle;
>
> @@ -809,7 +809,7 @@ static struct notifier_block dock_acpi_notifier = {
> * check to see if an object has an _EJD method. If it does, then it
> * will see if it is dependent on the dock station.
> */
> -static acpi_status
> +static acpi_status __init
> find_dock_devices(acpi_handle handle, u32 lvl, void *context, void **rv)
> {
> acpi_status status;
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
2013/6/13 Alexander E. Patrakov <[email protected]>:
> 2013/6/13 Jiang Liu <[email protected]>:
>> Alexander E. Patrakov <[email protected]> reports two bugs related to
>> dock station support on Sony VAIO VPCZ23A4R. Actually there are at least
>> four bugs related to Sony VAIO VPCZ23A4R dock support.
>> 1) can't correctly detect hotplug slot for dock state
>> 2) resource leak on undocking
>> 3) resource allocation failure for dock devices
>> 4) one bug in intel_snd_hda driver
>>
>> The first patch fixes issue 1, and the second patch fixes issue 2.
>> These two patches, if accepted, should be material for stable branches
>> too.
>> Patch 3-9 are code improvement for ACPI and dock driver.
>>
>> I have found the root cause for issue three, but still working on
>> solutions, and seems can't be solve in short time. So please help
>> to review and test patches for issue 1) and 2) first.
>>
>> Hi Alexander, could you please help to test the whole patchset?
>
> Tested on top of 3.10-rc5, with no other patches applied.
> Unfortunately, lockdep complains on the second docking attempt in the
> initially-undocked case. Please see the attached dmesg output.
>
> I have not tested the initially-docked case yet, will do that in 10 minutes.
Tested. The device list is updated as expected if the radeon driver is
blacklisted and there are no open fds to the audio card.
If radeon is not blacklisted, I hit
https://bugzilla.kernel.org/show_bug.cgi?id=59681 , but it is not a
regression.
--
Alexander E. Patrakov
On Friday, June 14, 2013 12:32:31 AM Jiang Liu wrote:
> Introduce several helper functions, which will be used to simplify code.
>
> Signed-off-by: Jiang Liu <[email protected]>
Again, this is not 3.10 material.
And you could easily say "three" instead of "several" in the changelog. :-)
Thanks,
Rafael
> ---
> drivers/acpi/utils.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++
> include/acpi/acpi_bus.h | 5 ++++
> 2 files changed, 79 insertions(+)
>
> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
> index 74437130..cb66fce 100644
> --- a/drivers/acpi/utils.c
> +++ b/drivers/acpi/utils.c
> @@ -495,3 +495,77 @@ acpi_handle_printk(const char *level, acpi_handle handle, const char *fmt, ...)
> kfree(buffer.pointer);
> }
> EXPORT_SYMBOL(acpi_handle_printk);
> +
> +/**
> + * acpi_evaluate_ej0: Evaluate _EJ0 method for hotplug operations
> + * @handle: ACPI device handle
> + *
> + * Evaluate device's _EJ0 method for hotplug operations.
> + */
> +acpi_status acpi_evaluate_ej0(acpi_handle handle)
> +{
> + acpi_status status;
> + union acpi_object arg;
> + struct acpi_object_list arg_list;
> +
> + arg.type = ACPI_TYPE_INTEGER;
> + arg.integer.value = 1;
> + arg_list.count = 1;
> + arg_list.pointer = &arg;
> + status = acpi_evaluate_object(handle, "_EJ0", &arg_list, NULL);
> + if (status == AE_NOT_FOUND) {
> + acpi_handle_warn(handle, "No _EJ0 support for device\n");
> + } else if (ACPI_FAILURE(status)) {
> + acpi_handle_warn(handle, "Eject failed (0x%x)\n", status);
> + }
> +
> + return status;
> +}
> +
> +/**
> + * acpi_evaluate_lck: Evaluate _LCK method to lock/unlock device
> + * @handle: ACPI device handle
> + * @lock: lock device if non-zero, otherwise unlock device
> + *
> + * Evaluate device's _LCK method if present to lock/unlock device
> + */
> +acpi_status acpi_evaluate_lck(acpi_handle handle, int lock)
> +{
> + acpi_status status;
> + union acpi_object arg;
> + struct acpi_object_list arg_list;
> +
> + arg_list.count = 1;
> + arg_list.pointer = &arg;
> + arg.type = ACPI_TYPE_INTEGER;
> + arg.integer.value = !!lock;
> + status = acpi_evaluate_object(handle, "_LCK", &arg_list, NULL);
> + if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
> + if (lock)
> + acpi_handle_warn(handle,
> + "Locking device failed (0x%x)\n", status);
> + else
> + acpi_handle_warn(handle,
> + "Unlocking device failed (0x%x)\n", status);
> + }
> +
> + return status;
> +}
> +
> +/**
> + * acpi_has_method: Check whether @handle has a method named @name
> + * @handle: ACPI device handle
> + * @name: name of object or method
> + *
> + * Check whether @handle has a method named @name.
> + */
> +int acpi_has_method(acpi_handle handle, char *name)
> +{
> + acpi_status status;
> + acpi_handle tmp;
> +
> + status = acpi_get_handle(handle, name, &tmp);
> +
> + return ACPI_FAILURE(status) ? 0 : 1;
> +}
> +EXPORT_SYMBOL(acpi_has_method);
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index 636c59f..5df5f89 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -56,6 +56,11 @@ acpi_evaluate_hotplug_ost(acpi_handle handle, u32 source_event,
>
> acpi_status
> acpi_get_physical_device_location(acpi_handle handle, struct acpi_pld_info **pld);
> +
> +acpi_status acpi_evaluate_ej0(acpi_handle handle);
> +acpi_status acpi_evaluate_lck(acpi_handle handle, int lock);
> +int acpi_has_method(acpi_handle handle, char *name);
> +
> #ifdef CONFIG_ACPI
>
> #include <linux/proc_fs.h>
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Friday, June 14, 2013 12:32:32 AM Jiang Liu wrote:
> Use new helper functions to simpilify ACPI dock, acpiphp code.
>
> Signed-off-by: Jiang Liu <[email protected]>
Not 3.10 material.
For now, please *only* send the patches from this series that are *necessary*
to fix the regression. All of the other related stuff should be sent in a
separate series for 3.11, ideally later (hint: when the essential changes have
been tested and approved).
So it looks like we need to focus on [1-2/9] from this series for now.
Thanks,
Rafael
> ---
> drivers/acpi/dock.c | 78 ++++----------------------------------
> drivers/acpi/scan.c | 53 ++++++--------------------
> drivers/pci/hotplug/acpiphp_glue.c | 15 ++------
> 3 files changed, 21 insertions(+), 125 deletions(-)
>
> diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
> index 72cf97e..1811f4f 100644
> --- a/drivers/acpi/dock.c
> +++ b/drivers/acpi/dock.c
> @@ -181,26 +181,14 @@ find_dock_dependent_device(struct dock_station *ds, acpi_handle handle)
> * If an acpi object has a _DCK method, then it is by definition a dock
> * station, so return true.
> */
> -static int is_dock(acpi_handle handle)
> +static inline int is_dock(acpi_handle handle)
> {
> - acpi_status status;
> - acpi_handle tmp;
> -
> - status = acpi_get_handle(handle, "_DCK", &tmp);
> - if (ACPI_FAILURE(status))
> - return 0;
> - return 1;
> + return acpi_has_method(handle, "_DCK");
> }
>
> -static int __init is_ejectable(acpi_handle handle)
> +static inline int is_ejectable(acpi_handle handle)
> {
> - acpi_status status;
> - acpi_handle tmp;
> -
> - status = acpi_get_handle(handle, "_EJ0", &tmp);
> - if (ACPI_FAILURE(status))
> - return 0;
> - return 1;
> + return acpi_has_method(handle, "_EJ0");
> }
>
> static int __init is_ata(acpi_handle handle)
> @@ -409,37 +397,6 @@ static void dock_event(struct dock_station *ds, u32 event, int num)
> }
>
> /**
> - * eject_dock - respond to a dock eject request
> - * @ds: the dock station
> - *
> - * This is called after _DCK is called, to execute the dock station's
> - * _EJ0 method.
> - */
> -static void eject_dock(struct dock_station *ds)
> -{
> - struct acpi_object_list arg_list;
> - union acpi_object arg;
> - acpi_status status;
> - acpi_handle tmp;
> -
> - /* all dock devices should have _EJ0, but check anyway */
> - status = acpi_get_handle(ds->handle, "_EJ0", &tmp);
> - if (ACPI_FAILURE(status)) {
> - pr_debug("No _EJ0 support for dock device\n");
> - return;
> - }
> -
> - arg_list.count = 1;
> - arg_list.pointer = &arg;
> - arg.type = ACPI_TYPE_INTEGER;
> - arg.integer.value = 1;
> -
> - status = acpi_evaluate_object(ds->handle, "_EJ0", &arg_list, NULL);
> - if (ACPI_FAILURE(status))
> - pr_debug("Failed to evaluate _EJ0!\n");
> -}
> -
> -/**
> * handle_dock - handle a dock event
> * @ds: the dock station
> * @dock: to dock, or undock - that is the question
> @@ -499,27 +456,6 @@ static inline void complete_undock(struct dock_station *ds)
> ds->flags &= ~(DOCK_UNDOCKING);
> }
>
> -static void dock_lock(struct dock_station *ds, int lock)
> -{
> - struct acpi_object_list arg_list;
> - union acpi_object arg;
> - acpi_status status;
> -
> - arg_list.count = 1;
> - arg_list.pointer = &arg;
> - arg.type = ACPI_TYPE_INTEGER;
> - arg.integer.value = !!lock;
> - status = acpi_evaluate_object(ds->handle, "_LCK", &arg_list, NULL);
> - if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
> - if (lock)
> - acpi_handle_warn(ds->handle,
> - "Locking device failed (0x%x)\n", status);
> - else
> - acpi_handle_warn(ds->handle,
> - "Unlocking device failed (0x%x)\n", status);
> - }
> -}
> -
> /**
> * dock_in_progress - see if we are in the middle of handling a dock event
> * @ds: the dock station
> @@ -653,8 +589,8 @@ static int handle_eject_request(struct dock_station *ds, u32 event)
>
> hotplug_dock_devices(ds, ACPI_NOTIFY_EJECT_REQUEST);
> undock(ds);
> - dock_lock(ds, 0);
> - eject_dock(ds);
> + acpi_evaluate_lck(ds->handle, 0);
> + acpi_evaluate_ej0(ds->handle);
> if (dock_present(ds)) {
> acpi_handle_err(ds->handle, "Unable to undock!\n");
> return -EBUSY;
> @@ -713,7 +649,7 @@ static void dock_notify(acpi_handle handle, u32 event, void *data)
> hotplug_dock_devices(ds, event);
> complete_dock(ds);
> dock_event(ds, event, DOCK_EVENT);
> - dock_lock(ds, 1);
> + acpi_evaluate_lck(ds->handle, 1);
> acpi_update_all_gpes();
> break;
> }
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 4148163..3372505 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -123,9 +123,6 @@ static DEVICE_ATTR(modalias, 0444, acpi_device_modalias_show, NULL);
> static int acpi_scan_hot_remove(struct acpi_device *device)
> {
> acpi_handle handle = device->handle;
> - acpi_handle not_used;
> - struct acpi_object_list arg_list;
> - union acpi_object arg;
> acpi_status status;
> unsigned long long sta;
>
> @@ -144,31 +141,12 @@ static int acpi_scan_hot_remove(struct acpi_device *device)
> put_device(&device->dev);
> device = NULL;
>
> - if (ACPI_SUCCESS(acpi_get_handle(handle, "_LCK", ¬_used))) {
> - arg_list.count = 1;
> - arg_list.pointer = &arg;
> - arg.type = ACPI_TYPE_INTEGER;
> - arg.integer.value = 0;
> - acpi_evaluate_object(handle, "_LCK", &arg_list, NULL);
> - }
> -
> - arg_list.count = 1;
> - arg_list.pointer = &arg;
> - arg.type = ACPI_TYPE_INTEGER;
> - arg.integer.value = 1;
> -
> - /*
> - * TBD: _EJD support.
> - */
> - status = acpi_evaluate_object(handle, "_EJ0", &arg_list, NULL);
> - if (ACPI_FAILURE(status)) {
> - if (status == AE_NOT_FOUND) {
> - return -ENODEV;
> - } else {
> - acpi_handle_warn(handle, "Eject failed (0x%x)\n",
> - status);
> - return -EIO;
> - }
> + acpi_evaluate_lck(handle, 0);
> + status = acpi_evaluate_ej0(handle);
> + if (status == AE_NOT_FOUND) {
> + return -ENODEV;
> + } else if (ACPI_FAILURE(status)) {
> + return -EIO;
> }
>
> /*
> @@ -536,7 +514,6 @@ static int acpi_device_setup_files(struct acpi_device *dev)
> {
> struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
> acpi_status status;
> - acpi_handle temp;
> unsigned long long sun;
> int result = 0;
>
> @@ -562,8 +539,7 @@ static int acpi_device_setup_files(struct acpi_device *dev)
> /*
> * If device has _STR, 'description' file is created
> */
> - status = acpi_get_handle(dev->handle, "_STR", &temp);
> - if (ACPI_SUCCESS(status)) {
> + if (acpi_has_method(dev->handle, "_STR")) {
> status = acpi_evaluate_object(dev->handle, "_STR",
> NULL, &buffer);
> if (ACPI_FAILURE(status))
> @@ -593,8 +569,7 @@ static int acpi_device_setup_files(struct acpi_device *dev)
> * If device has _EJ0, 'eject' file is created that is used to trigger
> * hot-removal function from userland.
> */
> - status = acpi_get_handle(dev->handle, "_EJ0", &temp);
> - if (ACPI_SUCCESS(status)) {
> + if (acpi_has_method(dev->handle, "_EJ0")) {
> result = device_create_file(&dev->dev, &dev_attr_eject);
> if (result)
> return result;
> @@ -616,9 +591,6 @@ end:
>
> static void acpi_device_remove_files(struct acpi_device *dev)
> {
> - acpi_status status;
> - acpi_handle temp;
> -
> if (dev->flags.power_manageable) {
> device_remove_file(&dev->dev, &dev_attr_power_state);
> if (dev->power.flags.power_resources)
> @@ -629,20 +601,17 @@ static void acpi_device_remove_files(struct acpi_device *dev)
> /*
> * If device has _STR, remove 'description' file
> */
> - status = acpi_get_handle(dev->handle, "_STR", &temp);
> - if (ACPI_SUCCESS(status)) {
> + if (acpi_has_method(dev->handle, "_STR")) {
> kfree(dev->pnp.str_obj);
> device_remove_file(&dev->dev, &dev_attr_description);
> }
> /*
> * If device has _EJ0, remove 'eject' file.
> */
> - status = acpi_get_handle(dev->handle, "_EJ0", &temp);
> - if (ACPI_SUCCESS(status))
> + if (acpi_has_method(dev->handle, "_EJ0"))
> device_remove_file(&dev->dev, &dev_attr_eject);
>
> - status = acpi_get_handle(dev->handle, "_SUN", &temp);
> - if (ACPI_SUCCESS(status))
> + if (acpi_has_method(dev->handle, "_SUN"))
> device_remove_file(&dev->dev, &dev_attr_sun);
>
> if (dev->pnp.unique_id)
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index 699b8ca..d0699ed 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -828,23 +828,14 @@ int acpiphp_eject_slot(struct acpiphp_slot *slot)
> {
> acpi_status status;
> struct acpiphp_func *func;
> - struct acpi_object_list arg_list;
> - union acpi_object arg;
>
> list_for_each_entry(func, &slot->funcs, sibling) {
> /* We don't want to call _EJ0 on non-existing functions. */
> if ((func->flags & FUNC_HAS_EJ0)) {
> - /* _EJ0 method take one argument */
> - arg_list.count = 1;
> - arg_list.pointer = &arg;
> - arg.type = ACPI_TYPE_INTEGER;
> - arg.integer.value = 1;
> -
> - status = acpi_evaluate_object(func->handle, "_EJ0", &arg_list, NULL);
> - if (ACPI_FAILURE(status)) {
> - warn("%s: _EJ0 failed\n", __func__);
> + status = acpi_evaluate_ej0(func->handle);
> + if (ACPI_FAILURE(status))
> return -1;
> - } else
> + else
> break;
> }
> }
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Thursday, June 13, 2013 08:26:10 PM Rafael J. Wysocki wrote:
> On Friday, June 14, 2013 12:32:26 AM Jiang Liu wrote:
> > ACPI dock driver can't be built as a module any more, so clean up
> > module related code.
> >
> > Signed-off-by: Jiang Liu <[email protected]>
> > Cc: Shaohua Li <[email protected]>
> > Cc: Len Brown <[email protected]>
> > Cc: "Rafael J. Wysocki" <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
>
> How exactly does this depend on [2/9]? If it doesn't at all, it should go
> after [1/9].
However, this isn't even 3.10 material, so please stop sending it for now.
Thanks,
Rafael
> > ---
> > drivers/acpi/dock.c | 41 -----------------------------------------
> > 1 file changed, 41 deletions(-)
> >
> > diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
> > index 79c8d9e..50e38b7 100644
> > --- a/drivers/acpi/dock.c
> > +++ b/drivers/acpi/dock.c
> > @@ -53,12 +53,6 @@ MODULE_PARM_DESC(immediate_undock, "1 (default) will cause the driver to "
> >
> > static struct atomic_notifier_head dock_notifier_list;
> >
> > -static const struct acpi_device_id dock_device_ids[] = {
> > - {"LNXDOCK", 0},
> > - {"", 0},
> > -};
> > -MODULE_DEVICE_TABLE(acpi, dock_device_ids);
> > -
>
> Don't we actually need the device IDs?
>
> > struct dock_station {
> > acpi_handle handle;
> > unsigned long last_dock_time;
> > @@ -1013,30 +1007,6 @@ err_unregister:
> > }
> >
> > /**
> > - * dock_remove - free up resources related to the dock station
> > - */
> > -static int dock_remove(struct dock_station *ds)
> > -{
> > - struct dock_dependent_device *dd, *tmp;
> > - struct platform_device *dock_device = ds->dock_device;
> > -
> > - if (!dock_station_count)
> > - return 0;
> > -
> > - /* remove dependent devices */
> > - list_for_each_entry_safe(dd, tmp, &ds->dependent_devices, list)
> > - kfree(dd);
> > -
> > - list_del(&ds->sibling);
> > -
> > - /* cleanup sysfs */
> > - sysfs_remove_group(&dock_device->dev.kobj, &dock_attribute_group);
> > - platform_device_unregister(dock_device);
> > -
> > - return 0;
> > -}
> > -
> > -/**
> > * find_dock_and_bay - look for dock stations and bays
> > * @handle: acpi handle of a device
> > * @lvl: unused
> > @@ -1073,14 +1043,3 @@ int __init acpi_dock_init(void)
> > ACPI_DOCK_DRIVER_DESCRIPTION, dock_station_count);
> > return 0;
> > }
> > -
> > -static void __exit dock_exit(void)
> > -{
> > - struct dock_station *tmp, *dock_station;
> > -
> > - unregister_acpi_bus_notifier(&dock_acpi_notifier);
> > - list_for_each_entry_safe(dock_station, tmp, &dock_stations, sibling)
> > - dock_remove(dock_station);
> > -}
> > -
> > -module_exit(dock_exit);
>
> The other changes look OK to me.
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Thu, Jun 13, 2013 at 9:32 AM, Jiang Liu <[email protected]> wrote:
> Alexander E. Patrakov <[email protected]> reports two bugs related to
> dock station support on Sony VAIO VPCZ23A4R. Actually there are at least
> four bugs related to Sony VAIO VPCZ23A4R dock support.
> 1) can't correctly detect hotplug slot for dock state
> 2) resource leak on undocking
> 3) resource allocation failure for dock devices
> 4) one bug in intel_snd_hda driver
>
> The first patch fixes issue 1, and the second patch fixes issue 2.
> These two patches, if accepted, should be material for stable branches
> too.
> Patch 3-9 are code improvement for ACPI and dock driver.
>
> I have found the root cause for issue three, but still working on
> solutions, and seems can't be solve in short time. So please help
> to review and test patches for issue 1) and 2) first.
the 3) is about pci resource allocation?
because pcibios_add_bus is called too early?
If that is case, we should have something like attached patch for it.
With that, we will not need to worry about _OSC set for 3.10 etc.
On Thursday, June 13, 2013 11:42:16 AM Yinghai Lu wrote:
> On Thu, Jun 13, 2013 at 9:32 AM, Jiang Liu <[email protected]> wrote:
> > Alexander E. Patrakov <[email protected]> reports two bugs related to
> > dock station support on Sony VAIO VPCZ23A4R. Actually there are at least
> > four bugs related to Sony VAIO VPCZ23A4R dock support.
> > 1) can't correctly detect hotplug slot for dock state
> > 2) resource leak on undocking
> > 3) resource allocation failure for dock devices
> > 4) one bug in intel_snd_hda driver
> >
> > The first patch fixes issue 1, and the second patch fixes issue 2.
> > These two patches, if accepted, should be material for stable branches
> > too.
> > Patch 3-9 are code improvement for ACPI and dock driver.
> >
> > I have found the root cause for issue three, but still working on
> > solutions, and seems can't be solve in short time. So please help
> > to review and test patches for issue 1) and 2) first.
>
> the 3) is about pci resource allocation?
> because pcibios_add_bus is called too early?
>
> If that is case, we should have something like attached patch for it.
I'm including the patch below to make it easier to comment.
> With that, we will not need to worry about _OSC set for 3.10 etc.
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index b1ff02a..68ed5d8 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -186,6 +186,14 @@ int pci_bus_add_device(struct pci_dev *dev)
> return 0;
> }
>
> +void __weak pcibios_add_bus(struct pci_bus *bus)
> +{
> +}
> +
> +void __weak pcibios_remove_bus(struct pci_bus *bus)
> +{
> +}
> +
> /**
> * pci_bus_add_devices - start driver for PCI devices
> * @bus: bus to check for new devices
> @@ -198,6 +206,11 @@ void pci_bus_add_devices(const struct pci_bus *bus)
> struct pci_bus *child;
> int retval;
>
> + if (bus->is_added == 1) {
> + pcibios_add_bus(bus);
> + bus->is_added++;
> + }
Do we need that in all of the places pci_bus_add_devices() is called?
It looks like pci_scan_child_bus() might be a better place for adding this,
or am I overlooking something?
[Hint: the bus->is_added++ hack is <explicit content> ugly.]
> +
> list_for_each_entry(dev, &bus->devices, bus_list) {
> /* Skip already-added devices */
> if (dev->is_added)
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 3dfc907..51404e6 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -704,8 +704,6 @@ add_dev:
> ret = device_add(&child->dev);
> WARN_ON(ret < 0);
>
> - pcibios_add_bus(child);
> -
> /* Create legacy_io and legacy_mem files for this bus */
> pci_create_legacy_files(child);
>
> @@ -1688,14 +1686,6 @@ int __weak pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
> return 0;
> }
>
> -void __weak pcibios_add_bus(struct pci_bus *bus)
> -{
> -}
> -
> -void __weak pcibios_remove_bus(struct pci_bus *bus)
> -{
> -}
> -
> struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
> struct pci_ops *ops, void *sysdata, struct list_head *resources)
> {
> @@ -1742,8 +1732,6 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
> if (error)
> goto class_dev_reg_err;
>
> - pcibios_add_bus(b);
> -
> /* Create legacy_io and legacy_mem files for this bus */
> pci_create_legacy_files(b);
>
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Thu, Jun 13, 2013 at 12:02 PM, Rafael J. Wysocki <[email protected]> wrote:
> On Thursday, June 13, 2013 11:42:16 AM Yinghai Lu wrote:
>> On Thu, Jun 13, 2013 at 9:32 AM, Jiang Liu <[email protected]> wrote:
>> > Alexander E. Patrakov <[email protected]> reports two bugs related to
>> > dock station support on Sony VAIO VPCZ23A4R. Actually there are at least
>> > four bugs related to Sony VAIO VPCZ23A4R dock support.
>> > 1) can't correctly detect hotplug slot for dock state
>> > 2) resource leak on undocking
>> > 3) resource allocation failure for dock devices
>> > 4) one bug in intel_snd_hda driver
>> >
>> > The first patch fixes issue 1, and the second patch fixes issue 2.
>> > These two patches, if accepted, should be material for stable branches
>> > too.
>> > Patch 3-9 are code improvement for ACPI and dock driver.
>> >
>> > I have found the root cause for issue three, but still working on
>> > solutions, and seems can't be solve in short time. So please help
>> > to review and test patches for issue 1) and 2) first.
>>
>> the 3) is about pci resource allocation?
>> because pcibios_add_bus is called too early?
>>
>> If that is case, we should have something like attached patch for it.
>
> I'm including the patch below to make it easier to comment.
>
>> With that, we will not need to worry about _OSC set for 3.10 etc.
>
>> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
>> index b1ff02a..68ed5d8 100644
>> --- a/drivers/pci/bus.c
>> +++ b/drivers/pci/bus.c
>> @@ -186,6 +186,14 @@ int pci_bus_add_device(struct pci_dev *dev)
>> return 0;
>> }
>>
>> +void __weak pcibios_add_bus(struct pci_bus *bus)
>> +{
>> +}
>> +
>> +void __weak pcibios_remove_bus(struct pci_bus *bus)
>> +{
>> +}
>> +
>> /**
>> * pci_bus_add_devices - start driver for PCI devices
>> * @bus: bus to check for new devices
>> @@ -198,6 +206,11 @@ void pci_bus_add_devices(const struct pci_bus *bus)
>> struct pci_bus *child;
>> int retval;
>>
>> + if (bus->is_added == 1) {
>> + pcibios_add_bus(bus);
>> + bus->is_added++;
>> + }
>
> Do we need that in all of the places pci_bus_add_devices() is called?
Yes.
>
> It looks like pci_scan_child_bus() might be a better place for adding this,
> or am I overlooking something?
acpiphp and acpi_pci_slot used to get attached when pci devices
get enumerated and pci resources get assigned unallocated...
they some kind of pci drivers to pci bus (comparing with real pci diver to
pci devices).
in dock case, could be, we just got bus, and do not scan pci devices under it.
then we try to scan dock pci devices too early...
>
> [Hint: the bus->is_added++ hack is <explicit content> ugly.]
Yeah, if we could move pcibios_fixup_bus to right place, we can move
bus->is_added setting to same place like we set pci_dev->is_added.
Yinghai
On Friday, June 14, 2013 12:32:25 AM Jiang Liu wrote:
> Current ACPI glue logic expects that physical devices are destroyed
> before destroying companion ACPI devices, otherwise it will break the
> ACPI unbind logic and cause following warning messages:
> [ 185.026073] usb usb5: Oops, 'acpi_handle' corrupt
> [ 185.035150] pci 0000:1b:00.0: Oops, 'acpi_handle' corrupt
> [ 185.035515] pci 0000:18:02.0: Oops, 'acpi_handle' corrupt
> [ 180.013656] port1: Oops, 'acpi_handle' corrupt
> Please refer to https://bugzilla.kernel.org/attachment.cgi?id=104321
> for full log message.
So my question is, did we have this problem before commit 3b63aaa70e1?
If we did, then when did it start? Or was it present forever?
> Above warning messages are caused by following scenario:
> 1) acpi_dock_notifier_call() queues a task (T1) onto kacpi_hotplug_wq
> 2) kacpi_hotplug_wq handles T1, which invokes acpi_dock_deferred_cb()
> ->dock_notify()-> handle_eject_request()->hotplug_dock_devices()
> 3) hotplug_dock_devices() first invokes registered hotplug callbacks to
> destroy physical devices, then destroys all affected ACPI devices.
> Everything seems perfect until now. But the acpiphp dock notification
> handler will queue another task (T2) onto kacpi_hotplug_wq to really
> destroy affected physical devices.
Would not the solution be to modify it so that it didn't spawn the other
task (T2), but removed the affected physical devices synchronously?
> 4) kacpi_hotplug_wq finishes T1, and all affected ACPI devices have
> been destroyed.
> 5) kacpi_hotplug_wq handles T2, which destroys all affected physical
> devices.
>
> So it breaks ACPI glue logic's expection because ACPI devices are destroyed
> in step 3 and physical devices are destroyed in step 5.
>
> Signed-off-by: Jiang Liu <[email protected]>
> Reported-by: Alexander E. Patrakov <[email protected]>
> Cc: Bjorn Helgaas <[email protected]>
> Cc: Yinghai Lu <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
> Hi Bjorn and Rafael,
> The recursive lock changes haven't been tested yet, need help
> from Alexander for testing.
Well, let's just say I'm not a fan of recursive locks. Is that unavoidable
here?
Rafael
> ---
> drivers/acpi/dock.c | 33 +++++++++++++++++++++++++++------
> drivers/pci/hotplug/acpiphp_glue.c | 32 ++++++++++++++++++--------------
> 2 files changed, 45 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
> index 02b0563..79c8d9e 100644
> --- a/drivers/acpi/dock.c
> +++ b/drivers/acpi/dock.c
> @@ -65,6 +65,7 @@ struct dock_station {
> u32 flags;
> spinlock_t dd_lock;
> struct mutex hp_lock;
> + struct task_struct *owner;
> struct list_head dependent_devices;
> struct list_head hotplug_devices;
>
> @@ -131,9 +132,13 @@ 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);
> + if (mutex_is_locked(&ds->hp_lock) && ds->owner == current) {
> + list_add_tail(&dd->hotplug_list, &ds->hotplug_devices);
> + } else {
> + mutex_lock(&ds->hp_lock);
> + list_add_tail(&dd->hotplug_list, &ds->hotplug_devices);
> + mutex_unlock(&ds->hp_lock);
> + }
> }
>
> /**
> @@ -147,9 +152,13 @@ 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);
> + if (mutex_is_locked(&ds->hp_lock) && ds->owner == current) {
> + list_del_init(&dd->hotplug_list);
> + } else {
> + mutex_lock(&ds->hp_lock);
> + list_del_init(&dd->hotplug_list);
> + mutex_unlock(&ds->hp_lock);
> + }
> }
>
> /**
> @@ -355,7 +364,17 @@ static void hotplug_dock_devices(struct dock_station *ds, u32 event)
> {
> struct dock_dependent_device *dd;
>
> + /*
> + * There is a deadlock scenario as below:
> + * hotplug_dock_devices()
> + * mutex_lock(&ds->hp_lock)
> + * dd->ops->handler()
> + * register_hotplug_dock_device()
> + * mutex_lock(&ds->hp_lock)
> + * So we need recursive lock scematics here, do it by ourselves.
> + */
> mutex_lock(&ds->hp_lock);
> + ds->owner = current;
>
> /*
> * First call driver specific hotplug functions
> @@ -376,6 +395,8 @@ static void hotplug_dock_devices(struct dock_station *ds, u32 event)
> else
> dock_create_acpi_device(dd->handle);
> }
> +
> + ds->owner = NULL;
> mutex_unlock(&ds->hp_lock);
> }
>
> 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);
> }
>
> /*
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On 2013/6/14 2:42, Yinghai Lu wrote:
> On Thu, Jun 13, 2013 at 9:32 AM, Jiang Liu <[email protected]> wrote:
>> Alexander E. Patrakov <[email protected]> reports two bugs related to
>> dock station support on Sony VAIO VPCZ23A4R. Actually there are at least
>> four bugs related to Sony VAIO VPCZ23A4R dock support.
>> 1) can't correctly detect hotplug slot for dock state
>> 2) resource leak on undocking
>> 3) resource allocation failure for dock devices
>> 4) one bug in intel_snd_hda driver
>>
>> The first patch fixes issue 1, and the second patch fixes issue 2.
>> These two patches, if accepted, should be material for stable branches
>> too.
>> Patch 3-9 are code improvement for ACPI and dock driver.
>>
>> I have found the root cause for issue three, but still working on
>> solutions, and seems can't be solve in short time. So please help
>> to review and test patches for issue 1) and 2) first.
>
> the 3) is about pci resource allocation?
> because pcibios_add_bus is called too early?
>
> If that is case, we should have something like attached patch for it.
>
> With that, we will not need to worry about _OSC set for 3.10 etc.
>
Hi Yinghai,
Seems not related to pcibios_add_bus(). According to my
investigation, the issue is caused by difference in PCI resource
assignment between boot time and runtime hotplug. On x86 platforms,
it respects PCI resource assignment from BIOS and only reassign
resources for unassigned BARs. But with acpiphp, it ignores BIOS
resource assignment and reassign all resources by OS.
If we have enough resources, reassigning all PCI resources should
work too, but may fail if we are under resource constraints. On the
other handle, current PCI IOMM align algorithm may waste huge MMIO
address space if we have some PCI devices with huge IOMM BAR.
On this Sony laptop, BIOS allocates limited IOMM resources for
the dock station and the dock station has a gfx which has a 256MB
IOMM BAR. So current acpiphp driver fails to allocate resources
for most devices on the dock station.
Currently I'm trying to change acpiphp to respect BIOS resource
assignment by calling pcibios_survey_resource_bus(), as in pci_root.c.
The other way is to change the IOMM resource allocation algorithm,
but obviously it's much more risky of regressions if changing the
algorithm.
Regards!
Gerry
2013/6/14 Yinghai Lu <[email protected]>:
> the 3) is about pci resource allocation?
> because pcibios_add_bus is called too early?
(3) is https://bugzilla.kernel.org/show_bug.cgi?id=56531
> If that is case, we should have something like attached patch for it.
It does not even compile because the argument of pci_bus_add_devices()
points to a const struct. You can't increment a member of it. And even
if you remove the const from both the declaration and the definition
of that function, the patch doesn't help at all.
Here is /proc/ioports with the fixed-up patch:
0000-0cf7 : PCI Bus 0000:00
0000-001f : dma1
0020-0021 : pic1
0040-0043 : timer0
0050-0053 : timer1
0060-0060 : keyboard
0062-0062 : EC data
0064-0064 : keyboard
0066-0066 : EC cmd
0070-0077 : rtc0
0080-008f : dma page reg
00a0-00a1 : pic2
00c0-00df : dma2
00e1-00e1 : #ENUM hotswap signal register
00f0-00ff : fpu
03c0-03df : vga+
0400-0453 : pnp 00:04
0400-0403 : ACPI PM1a_EVT_BLK
0404-0405 : ACPI PM1a_CNT_BLK
0408-040b : ACPI PM_TMR
0410-0415 : ACPI CPU throttle
0420-042f : ACPI GPE0_BLK
0430-0433 : iTCO_wdt
0450-0450 : ACPI PM2_CNT_BLK
0454-0457 : pnp 00:06
0458-047f : pnp 00:04
0460-047f : iTCO_wdt
0500-057f : pnp 00:04
0680-069f : pnp 00:04
0cf8-0cff : PCI conf1
0d00-ffff : PCI Bus 0000:00
1000-100f : pnp 00:04
164e-164f : pnp 00:04
2000-2000 : pnp 00:04
2004-2004 : pnp 00:04
3000-5fff : PCI Bus 0000:08
6000-6fff : PCI Bus 0000:05
6000-60ff : 0000:05:00.0
6000-60ff : r8169
7000-7fff : PCI Bus 0000:04
8000-8fff : PCI Bus 0000:03
9000-9fff : PCI Bus 0000:02
a000-a03f : 0000:00:02.0
a040-a05f : 0000:00:1f.3
a060-a07f : 0000:00:1f.2
a060-a07f : ahci
a080-a087 : 0000:00:1f.2
a080-a087 : ahci
a088-a08f : 0000:00:1f.2
a088-a08f : ahci
a090-a093 : 0000:00:1f.2
a090-a093 : ahci
a094-a097 : 0000:00:1f.2
a094-a097 : ahci
ffff-ffff : pnp 00:04
ffff-ffff : pnp 00:04
And /proc/iomem:
00000000-00000fff : reserved
00001000-0008f3ff : System RAM
0008f400-0009ffff : reserved
000a0000-000bffff : PCI Bus 0000:00
000c0000-000cffff : Video ROM
000d0000-000da3ff : Adapter ROM
000e0000-000fffff : reserved
000f0000-000fffff : System ROM
00100000-9ae3efff : System RAM
01000000-0166bfff : Kernel code
0166c000-01cbca3f : Kernel data
01dba000-026b8fff : Kernel bss
9ae3f000-9aebefff : reserved
9aebf000-9afbefff : ACPI Non-volatile Storage
9afbf000-9affefff : ACPI Tables
9afff000-9affffff : System RAM
9b000000-9f9fffff : reserved
9fa00000-feafffff : PCI Bus 0000:00
9fa00000-9fa00fff : pnp 00:09
a0000000-afffffff : 0000:00:02.0
b0000000-cfffffff : PCI Bus 0000:08
b0000000-c06fffff : PCI Bus 0000:0a
b0000000-b01fffff : PCI Bus 0000:0b
b0200000-b03fffff : PCI Bus 0000:0b
b0400000-b05fffff : PCI Bus 0000:0c
b0600000-b07fffff : PCI Bus 0000:0c
d0000000-d03fffff : 0000:00:02.0
d0400000-d13fffff : PCI Bus 0000:02
d1400000-d23fffff : PCI Bus 0000:03
d2400000-d33fffff : PCI Bus 0000:04
d3400000-d43fffff : PCI Bus 0000:05
d3400000-d3403fff : 0000:05:00.0
d3400000-d3403fff : r8169
d3404000-d3404fff : 0000:05:00.0
d3404000-d3404fff : r8169
d4400000-d53fffff : PCI Bus 0000:08
d5400000-d63fffff : PCI Bus 0000:05
d6400000-d73fffff : PCI Bus 0000:04
d6400000-d6401fff : 0000:04:00.0
d6400000-d6401fff : xhci_hcd
d7400000-d83fffff : PCI Bus 0000:03
d7400000-d7400fff : 0000:03:00.0
d7400000-d7400fff : rtsx_pci
d8400000-d93fffff : PCI Bus 0000:02
d8400000-d8401fff : 0000:02:00.0
d8400000-d8401fff : iwlwifi
d9400000-d9403fff : 0000:00:1b.0
d9400000-d9403fff : ICH HD audio
d9404000-d940400f : 0000:00:16.0
d9404000-d940400f : mei_me
d9405000-d94050ff : 0000:00:1f.3
d9407000-d94077ff : 0000:00:1f.2
d9407000-d94077ff : ahci
d9408000-d94083ff : 0000:00:1d.0
d9408000-d94083ff : ehci_hcd
d9409000-d94093ff : 0000:00:1a.0
d9409000-d94093ff : ehci_hcd
e0000000-efffffff : PCI MMCONFIG 0000 [bus 00-ff]
e0000000-efffffff : reserved
e0000000-efffffff : pnp 00:09
feb00000-feb03fff : reserved
fec00000-fec00fff : reserved
fec00000-fec003ff : IOAPIC 0
fed00000-fed003ff : HPET 0
fed10000-fed19fff : reserved
fed10000-fed17fff : pnp 00:09
fed18000-fed18fff : pnp 00:09
fed19000-fed19fff : pnp 00:09
fed1c000-fed1ffff : reserved
fed1c000-fed1ffff : pnp 00:09
fed1f410-fed1f414 : iTCO_wdt
fed20000-fed3ffff : pnp 00:09
fed90000-fed93fff : pnp 00:09
fee00000-fee00fff : Local APIC
fee00000-fee00fff : reserved
ffd80000-ffffffff : reserved
100000000-25fdfffff : System RAM
25fe00000-25fffffff : RAM buffer
You can compare them with other files from
https://bugzilla.kernel.org/show_bug.cgi?id=56531
--
Alexander E. Patrakov
On Thu, Jun 13, 2013 at 7:09 PM, Jiang Liu (Gerry) <[email protected]> wrote:
> On 2013/6/14 2:42, Yinghai Lu wrote:
>>
>> On Thu, Jun 13, 2013 at 9:32 AM, Jiang Liu <[email protected]> wrote:
>>>
>>> Alexander E. Patrakov <[email protected]> reports two bugs related to
>>> dock station support on Sony VAIO VPCZ23A4R. Actually there are at least
>>> four bugs related to Sony VAIO VPCZ23A4R dock support.
>>> 1) can't correctly detect hotplug slot for dock state
>>> 2) resource leak on undocking
>>> 3) resource allocation failure for dock devices
>>> 4) one bug in intel_snd_hda driver
>>>
>>> The first patch fixes issue 1, and the second patch fixes issue 2.
>>> These two patches, if accepted, should be material for stable branches
>>> too.
>>> Patch 3-9 are code improvement for ACPI and dock driver.
>>>
>>> I have found the root cause for issue three, but still working on
>>> solutions, and seems can't be solve in short time. So please help
>>> to review and test patches for issue 1) and 2) first.
>>
>>
>> the 3) is about pci resource allocation?
>> because pcibios_add_bus is called too early?
>>
>> If that is case, we should have something like attached patch for it.
>>
>> With that, we will not need to worry about _OSC set for 3.10 etc.
>>
> Hi Yinghai,
> Seems not related to pcibios_add_bus(). According to my
> investigation, the issue is caused by difference in PCI resource
> assignment between boot time and runtime hotplug. On x86 platforms,
> it respects PCI resource assignment from BIOS and only reassign
> resources for unassigned BARs. But with acpiphp, it ignores BIOS
> resource assignment and reassign all resources by OS.
> If we have enough resources, reassigning all PCI resources should
> work too, but may fail if we are under resource constraints. On the
> other handle, current PCI IOMM align algorithm may waste huge MMIO
> address space if we have some PCI devices with huge IOMM BAR.
> On this Sony laptop, BIOS allocates limited IOMM resources for
> the dock station and the dock station has a gfx which has a 256MB
> IOMM BAR. So current acpiphp driver fails to allocate resources
> for most devices on the dock station.
Is it a regression?
> Currently I'm trying to change acpiphp to respect BIOS resource
> assignment by calling pcibios_survey_resource_bus(), as in pci_root.c.
> The other way is to change the IOMM resource allocation algorithm,
> but obviously it's much more risky of regressions if changing the
> algorithm.
that is not going to help, need to increase bridge resource.
please check if BIOS have setup option about hotplug MMIO pad size.
Yinghai
2013/6/14 Yinghai Lu <[email protected]>:
> Is it a regression?
No.
--
Alexander E. Patrakov
On 2013/6/14 10:30, Yinghai Lu wrote:
> On Thu, Jun 13, 2013 at 7:09 PM, Jiang Liu (Gerry) <[email protected]> wrote:
>> On 2013/6/14 2:42, Yinghai Lu wrote:
>>>
>>> On Thu, Jun 13, 2013 at 9:32 AM, Jiang Liu <[email protected]> wrote:
>>>>
>>>> Alexander E. Patrakov <[email protected]> reports two bugs related to
>>>> dock station support on Sony VAIO VPCZ23A4R. Actually there are at least
>>>> four bugs related to Sony VAIO VPCZ23A4R dock support.
>>>> 1) can't correctly detect hotplug slot for dock state
>>>> 2) resource leak on undocking
>>>> 3) resource allocation failure for dock devices
>>>> 4) one bug in intel_snd_hda driver
>>>>
>>>> The first patch fixes issue 1, and the second patch fixes issue 2.
>>>> These two patches, if accepted, should be material for stable branches
>>>> too.
>>>> Patch 3-9 are code improvement for ACPI and dock driver.
>>>>
>>>> I have found the root cause for issue three, but still working on
>>>> solutions, and seems can't be solve in short time. So please help
>>>> to review and test patches for issue 1) and 2) first.
>>>
>>>
>>> the 3) is about pci resource allocation?
>>> because pcibios_add_bus is called too early?
>>>
>>> If that is case, we should have something like attached patch for it.
>>>
>>> With that, we will not need to worry about _OSC set for 3.10 etc.
>>>
>> Hi Yinghai,
>> Seems not related to pcibios_add_bus(). According to my
>> investigation, the issue is caused by difference in PCI resource
>> assignment between boot time and runtime hotplug. On x86 platforms,
>> it respects PCI resource assignment from BIOS and only reassign
>> resources for unassigned BARs. But with acpiphp, it ignores BIOS
>> resource assignment and reassign all resources by OS.
>> If we have enough resources, reassigning all PCI resources should
>> work too, but may fail if we are under resource constraints. On the
>> other handle, current PCI IOMM align algorithm may waste huge MMIO
>> address space if we have some PCI devices with huge IOMM BAR.
>> On this Sony laptop, BIOS allocates limited IOMM resources for
>> the dock station and the dock station has a gfx which has a 256MB
>> IOMM BAR. So current acpiphp driver fails to allocate resources
>> for most devices on the dock station.
>
> Is it a regression?
Not sure. But a little concern about check_hotplug_bridge(), it treats
dock station and devices on dock station with _EJD as hot-plug-gable
PCI bus and reserve extra resources for possible hot-adding. But I
think we should only reserve extra resource for dock station, and should
not reserve resource for devices on station with _EJD method.
>
>> Currently I'm trying to change acpiphp to respect BIOS resource
>> assignment by calling pcibios_survey_resource_bus(), as in pci_root.c.
>> The other way is to change the IOMM resource allocation algorithm,
>> but obviously it's much more risky of regressions if changing the
>> algorithm.
>
> that is not going to help, need to increase bridge resource.
>
> please check if BIOS have setup option about hotplug MMIO pad size.
For the first step, I'm trying to make hotplug case work in the same way
as boot time. Do you think this patch help?
diff --git a/drivers/pci/hotplug/acpiphp_glue.c
b/drivers/pci/hotplug/acpiphp_gl
index 270fdba..12e3f6e 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -837,13 +837,13 @@ 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->subordi
}
}
}
}
- pci_bus_assign_resources(bus);
+ pci_assign_unassigned_bus_resources(bus);
acpiphp_sanitize_bus(bus);
acpiphp_set_hpp_values(bus);
acpiphp_set_acpi_region(slot);
---
>
> Yinghai
>
> .
>
On Thu, Jun 13, 2013 at 7:06 PM, Alexander E. Patrakov
<[email protected]> wrote:
> 2013/6/14 Yinghai Lu <[email protected]>:
>> the 3) is about pci resource allocation?
>> because pcibios_add_bus is called too early?
>
> (3) is https://bugzilla.kernel.org/show_bug.cgi?id=56531
>
>> If that is case, we should have something like attached patch for it.
>
> It does not even compile because the argument of pci_bus_add_devices()
> points to a const struct. You can't increment a member of it. And even
> if you remove the const from both the declaration and the definition
> of that function, the patch doesn't help at all.
>
> Here is /proc/ioports with the fixed-up patch:
dmesg with the fixed-up patch on v3.10-rc?
After look at the your dmesg v3.10-rc5, we really should move
pcibios_add_bus down.
as we do have children slots under ....
Yinghai
On Thu, Jun 13, 2013 at 7:51 PM, Jiang Liu (Gerry) <[email protected]> wrote:
> On 2013/6/14 10:30, Yinghai Lu wrote:
>>
>> On Thu, Jun 13, 2013 at 7:09 PM, Jiang Liu (Gerry) <[email protected]>
>> wrote:
>
> Not sure. But a little concern about check_hotplug_bridge(), it treats
> dock station and devices on dock station with _EJD as hot-plug-gable
> PCI bus and reserve extra resources for possible hot-adding. But I
> think we should only reserve extra resource for dock station, and should
> not reserve resource for devices on station with _EJD method.
That is should be...
if there is children hotplug slots, we should add extra...
>
>
>>
>>> Currently I'm trying to change acpiphp to respect BIOS resource
>>> assignment by calling pcibios_survey_resource_bus(), as in pci_root.c.
>>> The other way is to change the IOMM resource allocation algorithm,
>>> but obviously it's much more risky of regressions if changing the
>>> algorithm.
>>
>>
>> that is not going to help, need to increase bridge resource.
>>
>> please check if BIOS have setup option about hotplug MMIO pad size.
>
> For the first step, I'm trying to make hotplug case work in the same way as
> boot time. Do you think this patch help?
>
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c
> b/drivers/pci/hotplug/acpiphp_gl
> index 270fdba..12e3f6e 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -837,13 +837,13 @@ 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->subordi
> }
> }
> }
> }
>
> - pci_bus_assign_resources(bus);
> + pci_assign_unassigned_bus_resources(bus);
> acpiphp_sanitize_bus(bus);
> acpiphp_set_hpp_values(bus);
> acpiphp_set_acpi_region(slot);
> ---
Yes, it should help. but may need to after pcibios_add_bus move down patch.
otherwise, we could enable children slot and survery them and assign
unassigned for them at first
and then go to parent slots.
that will make children slots survey fails, that is too early and
should be done after parent slots.
Yinghai
On Thu, Jun 13, 2013 at 8:30 PM, Yinghai Lu <[email protected]> wrote:
> On Thu, Jun 13, 2013 at 7:51 PM, Jiang Liu (Gerry) <[email protected]> wrote:
>> For the first step, I'm trying to make hotplug case work in the same way as
>> boot time. Do you think this patch help?
>>
>> diff --git a/drivers/pci/hotplug/acpiphp_glue.c
>> b/drivers/pci/hotplug/acpiphp_gl
>> index 270fdba..12e3f6e 100644
>> --- a/drivers/pci/hotplug/acpiphp_glue.c
>> +++ b/drivers/pci/hotplug/acpiphp_glue.c
>> @@ -837,13 +837,13 @@ 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->subordi
>> }
>> }
>> }
>> }
>>
>> - pci_bus_assign_resources(bus);
>> + pci_assign_unassigned_bus_resources(bus);
can not use use pci_assign_unassigned_bus_resources directly. as bus
could have devices that is there already
before this enable_device and could have driver loaded before.
that is why we have checking
if (PCI_SLOT(dev->devfn) != slot->device)
On Thu, Jun 13, 2013 at 8:30 PM, Yinghai Lu <[email protected]> wrote:
>> diff --git a/drivers/pci/hotplug/acpiphp_glue.c
>> b/drivers/pci/hotplug/acpiphp_gl
>> index 270fdba..12e3f6e 100644
>> --- a/drivers/pci/hotplug/acpiphp_glue.c
>> +++ b/drivers/pci/hotplug/acpiphp_glue.c
>> @@ -837,13 +837,13 @@ 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->subordi
>> }
>> }
>> }
>> }
>>
>> - pci_bus_assign_resources(bus);
>> + pci_assign_unassigned_bus_resources(bus);
>> acpiphp_sanitize_bus(bus);
>> acpiphp_set_hpp_values(bus);
>> acpiphp_set_acpi_region(slot);
>> ---
>
> Yes, it should help. but may need to after pcibios_add_bus move down patch.
>
> otherwise, we could enable children slot and survery them and assign
> unassigned for them at first
> and then go to parent slots.
> that will make children slots survey fails, that is too early and
> should be done after parent slots.
oh, it seems we should get enable_slot called children slots before parent slots
finished.
so should not need "pcibios_add_bus move down" patch.
Yinghai
On 2013/6/14 11:43, Yinghai Lu wrote:
> On Thu, Jun 13, 2013 at 8:30 PM, Yinghai Lu <[email protected]> wrote:
>> On Thu, Jun 13, 2013 at 7:51 PM, Jiang Liu (Gerry) <[email protected]> wrote:
>
>>> For the first step, I'm trying to make hotplug case work in the same way as
>>> boot time. Do you think this patch help?
>>>
>>> diff --git a/drivers/pci/hotplug/acpiphp_glue.c
>>> b/drivers/pci/hotplug/acpiphp_gl
>>> index 270fdba..12e3f6e 100644
>>> --- a/drivers/pci/hotplug/acpiphp_glue.c
>>> +++ b/drivers/pci/hotplug/acpiphp_glue.c
>>> @@ -837,13 +837,13 @@ 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->subordi
>>> }
>>> }
>>> }
>>> }
>>>
>>> - pci_bus_assign_resources(bus);
>>> + pci_assign_unassigned_bus_resources(bus);
>
> can not use use pci_assign_unassigned_bus_resources directly. as bus
> could have devices that is there already
> before this enable_device and could have driver loaded before.
>
> that is why we have checking
> if (PCI_SLOT(dev->devfn) != slot->device)
>
Thanks for reminder.
This a quick solution for prove of concept and it should be OK for
this Sony laptop case.
On the other hand, we may need to enhance
pci_bus_size_bridges(dev->subordinate) and
pci_bus_assign_resources(bus) to support realloc_list.
> .
>
2013/6/14 Yinghai Lu <[email protected]>:
> dmesg with the fixed-up patch on v3.10-rc5?
Attached. This is on top of the whole initially-posted series of 9
patches, with your fixed-up patch, but without
https://lkml.org/lkml/2013/6/13/654, and with radeon blacklisted.
--
Alexander E. Patrakov
2013/6/14 Jiang Liu (Gerry) <[email protected]>:
> On 2013/6/14 10:30, Yinghai Lu wrote:
>>
>> On Thu, Jun 13, 2013 at 7:09 PM, Jiang Liu (Gerry) <[email protected]>
>> wrote:
>>>
>>> On 2013/6/14 2:42, Yinghai Lu wrote:
>>>>
>>>>
>>>> On Thu, Jun 13, 2013 at 9:32 AM, Jiang Liu <[email protected]> wrote:
>>>>>
>>>>>
>>>>> Alexander E. Patrakov <[email protected]> reports two bugs related to
>>>>> dock station support on Sony VAIO VPCZ23A4R. Actually there are at
>>>>> least
>>>>> four bugs related to Sony VAIO VPCZ23A4R dock support.
>>>>> 1) can't correctly detect hotplug slot for dock state
>>>>> 2) resource leak on undocking
>>>>> 3) resource allocation failure for dock devices
>>>>> 4) one bug in intel_snd_hda driver
>>>>>
>>>>> The first patch fixes issue 1, and the second patch fixes issue 2.
>>>>> These two patches, if accepted, should be material for stable branches
>>>>> too.
>>>>> Patch 3-9 are code improvement for ACPI and dock driver.
>>>>>
>>>>> I have found the root cause for issue three, but still working on
>>>>> solutions, and seems can't be solve in short time. So please help
>>>>> to review and test patches for issue 1) and 2) first.
>>>>
>>>>
>>>>
>>>> the 3) is about pci resource allocation?
>>>> because pcibios_add_bus is called too early?
>>>>
>>>> If that is case, we should have something like attached patch for it.
>>>>
>>>> With that, we will not need to worry about _OSC set for 3.10 etc.
>>>>
>>> Hi Yinghai,
>>> Seems not related to pcibios_add_bus(). According to my
>>> investigation, the issue is caused by difference in PCI resource
>>> assignment between boot time and runtime hotplug. On x86 platforms,
>>> it respects PCI resource assignment from BIOS and only reassign
>>> resources for unassigned BARs. But with acpiphp, it ignores BIOS
>>> resource assignment and reassign all resources by OS.
>>> If we have enough resources, reassigning all PCI resources should
>>> work too, but may fail if we are under resource constraints. On the
>>> other handle, current PCI IOMM align algorithm may waste huge MMIO
>>> address space if we have some PCI devices with huge IOMM BAR.
>>> On this Sony laptop, BIOS allocates limited IOMM resources for
>>> the dock station and the dock station has a gfx which has a 256MB
>>> IOMM BAR. So current acpiphp driver fails to allocate resources
>>> for most devices on the dock station.
>>
>>
>> Is it a regression?
>
> Not sure. But a little concern about check_hotplug_bridge(), it treats
> dock station and devices on dock station with _EJD as hot-plug-gable
> PCI bus and reserve extra resources for possible hot-adding. But I
> think we should only reserve extra resource for dock station, and should
> not reserve resource for devices on station with _EJD method.
>
>
>>
>>> Currently I'm trying to change acpiphp to respect BIOS resource
>>> assignment by calling pcibios_survey_resource_bus(), as in pci_root.c.
>>> The other way is to change the IOMM resource allocation algorithm,
>>> but obviously it's much more risky of regressions if changing the
>>> algorithm.
>>
>>
>> that is not going to help, need to increase bridge resource.
>>
>> please check if BIOS have setup option about hotplug MMIO pad size.
>
> For the first step, I'm trying to make hotplug case work in the same way as
> boot time. Do you think this patch help?
>
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c
> b/drivers/pci/hotplug/acpiphp_gl
> index 270fdba..12e3f6e 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -837,13 +837,13 @@ 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->subordi
> }
> }
> }
> }
>
> - pci_bus_assign_resources(bus);
> + pci_assign_unassigned_bus_resources(bus);
> acpiphp_sanitize_bus(bus);
> acpiphp_set_hpp_values(bus);
> acpiphp_set_acpi_region(slot);
> ---
The patch helped, thanks. Note: I have tested it together with
pci_move_pcibios_add_bus_down.patch, I don't know yet if
pci_move_pcibios_add_bus_down.patch is needed.
--
Alexander E. Patrakov
On 2013/6/14 12:07, Alexander E. Patrakov wrote:
> 2013/6/14 Jiang Liu (Gerry) <[email protected]>:
>> On 2013/6/14 10:30, Yinghai Lu wrote:
>>>
>>> On Thu, Jun 13, 2013 at 7:09 PM, Jiang Liu (Gerry) <[email protected]>
>>> wrote:
>>>>
>>>> On 2013/6/14 2:42, Yinghai Lu wrote:
>>>>>
>>>>>
>>>>> On Thu, Jun 13, 2013 at 9:32 AM, Jiang Liu <[email protected]> wrote:
>>>>>>
>>>>>>
>>>>>> Alexander E. Patrakov <[email protected]> reports two bugs related to
>>>>>> dock station support on Sony VAIO VPCZ23A4R. Actually there are at
>>>>>> least
>>>>>> four bugs related to Sony VAIO VPCZ23A4R dock support.
>>>>>> 1) can't correctly detect hotplug slot for dock state
>>>>>> 2) resource leak on undocking
>>>>>> 3) resource allocation failure for dock devices
>>>>>> 4) one bug in intel_snd_hda driver
>>>>>>
>>>>>> The first patch fixes issue 1, and the second patch fixes issue 2.
>>>>>> These two patches, if accepted, should be material for stable branches
>>>>>> too.
>>>>>> Patch 3-9 are code improvement for ACPI and dock driver.
>>>>>>
>>>>>> I have found the root cause for issue three, but still working on
>>>>>> solutions, and seems can't be solve in short time. So please help
>>>>>> to review and test patches for issue 1) and 2) first.
>>>>>
>>>>>
>>>>>
>>>>> the 3) is about pci resource allocation?
>>>>> because pcibios_add_bus is called too early?
>>>>>
>>>>> If that is case, we should have something like attached patch for it.
>>>>>
>>>>> With that, we will not need to worry about _OSC set for 3.10 etc.
>>>>>
>>>> Hi Yinghai,
>>>> Seems not related to pcibios_add_bus(). According to my
>>>> investigation, the issue is caused by difference in PCI resource
>>>> assignment between boot time and runtime hotplug. On x86 platforms,
>>>> it respects PCI resource assignment from BIOS and only reassign
>>>> resources for unassigned BARs. But with acpiphp, it ignores BIOS
>>>> resource assignment and reassign all resources by OS.
>>>> If we have enough resources, reassigning all PCI resources should
>>>> work too, but may fail if we are under resource constraints. On the
>>>> other handle, current PCI IOMM align algorithm may waste huge MMIO
>>>> address space if we have some PCI devices with huge IOMM BAR.
>>>> On this Sony laptop, BIOS allocates limited IOMM resources for
>>>> the dock station and the dock station has a gfx which has a 256MB
>>>> IOMM BAR. So current acpiphp driver fails to allocate resources
>>>> for most devices on the dock station.
>>>
>>>
>>> Is it a regression?
>>
>> Not sure. But a little concern about check_hotplug_bridge(), it treats
>> dock station and devices on dock station with _EJD as hot-plug-gable
>> PCI bus and reserve extra resources for possible hot-adding. But I
>> think we should only reserve extra resource for dock station, and should
>> not reserve resource for devices on station with _EJD method.
>>
>>
>>>
>>>> Currently I'm trying to change acpiphp to respect BIOS resource
>>>> assignment by calling pcibios_survey_resource_bus(), as in pci_root.c.
>>>> The other way is to change the IOMM resource allocation algorithm,
>>>> but obviously it's much more risky of regressions if changing the
>>>> algorithm.
>>>
>>>
>>> that is not going to help, need to increase bridge resource.
>>>
>>> please check if BIOS have setup option about hotplug MMIO pad size.
>>
>> For the first step, I'm trying to make hotplug case work in the same way as
>> boot time. Do you think this patch help?
>>
>> diff --git a/drivers/pci/hotplug/acpiphp_glue.c
>> b/drivers/pci/hotplug/acpiphp_gl
>> index 270fdba..12e3f6e 100644
>> --- a/drivers/pci/hotplug/acpiphp_glue.c
>> +++ b/drivers/pci/hotplug/acpiphp_glue.c
>> @@ -837,13 +837,13 @@ 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->subordi
>> }
>> }
>> }
>> }
>>
>> - pci_bus_assign_resources(bus);
>> + pci_assign_unassigned_bus_resources(bus);
>> acpiphp_sanitize_bus(bus);
>> acpiphp_set_hpp_values(bus);
>> acpiphp_set_acpi_region(slot);
>> ---
>
>
> The patch helped, thanks. Note: I have tested it together with
> pci_move_pcibios_add_bus_down.patch, I don't know yet if
> pci_move_pcibios_add_bus_down.patch is needed.
What's the situation now? Could you please send out dmesgs\ioports\iomem?
Thanks!
>
> --
> Alexander E. Patrakov
>
> .
>
2013/6/14 Jiang Liu (Gerry) <[email protected]>:
> What's the situation now? Could you please send out dmesgs\ioports\iomem?
> Thanks!
The requested information has been added to
https://bugzilla.kernel.org/show_bug.cgi?id=56531
The changes in pci_move_pcibios_add_bus_down.patch are not needed.
The radeon card in the dock station works after docking and restarting
the X server, and its HD audio device works, too. Just for
completeness, I have tested the CD-ROM drive, the USB adapters and the
Ethernet card - they all work. IOW, everything works after docking.
I did not test undocking, as that would surely hit
https://bugzilla.kernel.org/show_bug.cgi?id=59681 and the hda_intel
bug you mentioned in the first mail in this thread.
Note that, for the initial series of patches, I still don't have any
response to the lockdep warnings in
https://lkml.org/lkml/2013/6/13/398
--
Alexander E. Patrakov
On 2013/6/14 12:43, Alexander E. Patrakov wrote:
> 2013/6/14 Jiang Liu (Gerry) <[email protected]>:
>
>> What's the situation now? Could you please send out dmesgs\ioports\iomem?
>> Thanks!
>
> The requested information has been added to
> https://bugzilla.kernel.org/show_bug.cgi?id=56531
>
> The changes in pci_move_pcibios_add_bus_down.patch are not needed.
>
> The radeon card in the dock station works after docking and restarting
> the X server, and its HD audio device works, too. Just for
> completeness, I have tested the CD-ROM drive, the USB adapters and the
> Ethernet card - they all work. IOW, everything works after docking.
>
> I did not test undocking, as that would surely hit
> https://bugzilla.kernel.org/show_bug.cgi?id=59681 and the hda_intel
> bug you mentioned in the first mail in this thread.
>
> Note that, for the initial series of patches, I still don't have any
> response to the lockdep warnings in
> https://lkml.org/lkml/2013/6/13/398
Sounds great, I will rework the lockdep related patch tonight.
Thanks for testing.
>
On Thursday, June 13, 2013 09:59:44 PM Rafael J. Wysocki wrote:
> On Friday, June 14, 2013 12:32:25 AM Jiang Liu wrote:
> > Current ACPI glue logic expects that physical devices are destroyed
> > before destroying companion ACPI devices, otherwise it will break the
> > ACPI unbind logic and cause following warning messages:
> > [ 185.026073] usb usb5: Oops, 'acpi_handle' corrupt
> > [ 185.035150] pci 0000:1b:00.0: Oops, 'acpi_handle' corrupt
> > [ 185.035515] pci 0000:18:02.0: Oops, 'acpi_handle' corrupt
> > [ 180.013656] port1: Oops, 'acpi_handle' corrupt
> > Please refer to https://bugzilla.kernel.org/attachment.cgi?id=104321
> > for full log message.
>
> So my question is, did we have this problem before commit 3b63aaa70e1?
>
> If we did, then when did it start? Or was it present forever?
>
> > Above warning messages are caused by following scenario:
> > 1) acpi_dock_notifier_call() queues a task (T1) onto kacpi_hotplug_wq
> > 2) kacpi_hotplug_wq handles T1, which invokes acpi_dock_deferred_cb()
> > ->dock_notify()-> handle_eject_request()->hotplug_dock_devices()
> > 3) hotplug_dock_devices() first invokes registered hotplug callbacks to
> > destroy physical devices, then destroys all affected ACPI devices.
> > Everything seems perfect until now. But the acpiphp dock notification
> > handler will queue another task (T2) onto kacpi_hotplug_wq to really
> > destroy affected physical devices.
>
> Would not the solution be to modify it so that it didn't spawn the other
> task (T2), but removed the affected physical devices synchronously?
>
> > 4) kacpi_hotplug_wq finishes T1, and all affected ACPI devices have
> > been destroyed.
> > 5) kacpi_hotplug_wq handles T2, which destroys all affected physical
> > devices.
> >
> > So it breaks ACPI glue logic's expection because ACPI devices are destroyed
> > in step 3 and physical devices are destroyed in step 5.
> >
> > Signed-off-by: Jiang Liu <[email protected]>
> > Reported-by: Alexander E. Patrakov <[email protected]>
> > Cc: Bjorn Helgaas <[email protected]>
> > Cc: Yinghai Lu <[email protected]>
> > Cc: "Rafael J. Wysocki" <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
> > ---
> > Hi Bjorn and Rafael,
> > The recursive lock changes haven't been tested yet, need help
> > from Alexander for testing.
>
> Well, let's just say I'm not a fan of recursive locks. Is that unavoidable
> here?
What about the appended patch (on top of [1/9], untested)?
Rafael
---
drivers/pci/hotplug/acpiphp_glue.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
Index: linux-pm/drivers/pci/hotplug/acpiphp_glue.c
===================================================================
--- linux-pm.orig/drivers/pci/hotplug/acpiphp_glue.c
+++ linux-pm/drivers/pci/hotplug/acpiphp_glue.c
@@ -145,9 +145,20 @@ static int post_dock_fixups(struct notif
return NOTIFY_OK;
}
+static void handle_dock_event_func(acpi_handle handle, u32 event, void *context)
+{
+ if (event == ACPI_NOTIFY_EJECT_REQUEST) {
+ struct acpiphp_func *func = context;
+
+ if (!acpiphp_disable_slot(func->slot))
+ acpiphp_eject_slot(func->slot);
+ } else {
+ handle_hotplug_event_func(handle, event, context);
+ }
+}
static const struct acpi_dock_ops acpiphp_dock_ops = {
- .handler = handle_hotplug_event_func,
+ .handler = handle_dock_event_func,
};
/* Check whether the PCI device is managed by native PCIe hotplug driver */
2013/6/14 Rafael J. Wysocki <[email protected]>:
> What about the appended patch (on top of [1/9], untested)?
>
> Rafael
Sorry, I have lost the track of which patches, on top of 3.10-rc5, I
should include in my testing and which I shouldn't. Could you please
restore my understanding? I.e., please provide a full list of LKML or
Bugzilla links to patches which I should test during the next boot of
the laptop.
Thanks in advance.
> ---
> drivers/pci/hotplug/acpiphp_glue.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> Index: linux-pm/drivers/pci/hotplug/acpiphp_glue.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/hotplug/acpiphp_glue.c
> +++ linux-pm/drivers/pci/hotplug/acpiphp_glue.c
> @@ -145,9 +145,20 @@ static int post_dock_fixups(struct notif
> return NOTIFY_OK;
> }
>
> +static void handle_dock_event_func(acpi_handle handle, u32 event, void *context)
> +{
> + if (event == ACPI_NOTIFY_EJECT_REQUEST) {
> + struct acpiphp_func *func = context;
> +
> + if (!acpiphp_disable_slot(func->slot))
> + acpiphp_eject_slot(func->slot);
> + } else {
> + handle_hotplug_event_func(handle, event, context);
> + }
> +}
>
> static const struct acpi_dock_ops acpiphp_dock_ops = {
> - .handler = handle_hotplug_event_func,
> + .handler = handle_dock_event_func,
> };
>
> /* Check whether the PCI device is managed by native PCIe hotplug driver */
>
--
Alexander E. Patrakov
On Friday, June 14, 2013 06:30:58 PM Alexander E. Patrakov wrote:
> 2013/6/14 Rafael J. Wysocki <[email protected]>:
>
> > What about the appended patch (on top of [1/9], untested)?
> >
> > Rafael
>
> Sorry, I have lost the track of which patches, on top of 3.10-rc5, I
> should include in my testing and which I shouldn't. Could you please
> restore my understanding? I.e., please provide a full list of LKML or
> Bugzilla links to patches which I should test during the next boot of
> the laptop.
>
> Thanks in advance.
As far as I'm concerned, please test these two for now:
https://patchwork.kernel.org/patch/2717851/
https://patchwork.kernel.org/patch/2721271/
They are targeted at the ordering problems only, however, so they won't address
the resource allocation issues you're seeing.
There are some other patches Jiang Liu and Yinghai are working on as far as
I can say, but I'm not sure what their status is at the moment.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On 06/14/2013 03:59 AM, Rafael J. Wysocki wrote:
> On Friday, June 14, 2013 12:32:25 AM Jiang Liu wrote:
>> Current ACPI glue logic expects that physical devices are destroyed
>> before destroying companion ACPI devices, otherwise it will break the
>> ACPI unbind logic and cause following warning messages:
>> [ 185.026073] usb usb5: Oops, 'acpi_handle' corrupt
>> [ 185.035150] pci 0000:1b:00.0: Oops, 'acpi_handle' corrupt
>> [ 185.035515] pci 0000:18:02.0: Oops, 'acpi_handle' corrupt
>> [ 180.013656] port1: Oops, 'acpi_handle' corrupt
>> Please refer to https://bugzilla.kernel.org/attachment.cgi?id=104321
>> for full log message.
>
> So my question is, did we have this problem before commit 3b63aaa70e1?
>
> If we did, then when did it start? Or was it present forever?
I think this issue should exist before commit "PCI: acpiphp: Do not use
ACPI PCI subdriver mechanism". It may trace back to the changes to kill
acpi_pci_bind()/acpi_pci_unbind().
>
>> Above warning messages are caused by following scenario:
>> 1) acpi_dock_notifier_call() queues a task (T1) onto kacpi_hotplug_wq
>> 2) kacpi_hotplug_wq handles T1, which invokes acpi_dock_deferred_cb()
>> ->dock_notify()-> handle_eject_request()->hotplug_dock_devices()
>> 3) hotplug_dock_devices() first invokes registered hotplug callbacks to
>> destroy physical devices, then destroys all affected ACPI devices.
>> Everything seems perfect until now. But the acpiphp dock notification
>> handler will queue another task (T2) onto kacpi_hotplug_wq to really
>> destroy affected physical devices.
>
> Would not the solution be to modify it so that it didn't spawn the other
> task (T2), but removed the affected physical devices synchronously?
Yes, that's the way I'm going to fix this issue.
>
>> 4) kacpi_hotplug_wq finishes T1, and all affected ACPI devices have
>> been destroyed.
>> 5) kacpi_hotplug_wq handles T2, which destroys all affected physical
>> devices.
>>
>> So it breaks ACPI glue logic's expection because ACPI devices are destroyed
>> in step 3 and physical devices are destroyed in step 5.
>>
>> Signed-off-by: Jiang Liu <[email protected]>
>> Reported-by: Alexander E. Patrakov <[email protected]>
>> Cc: Bjorn Helgaas <[email protected]>
>> Cc: Yinghai Lu <[email protected]>
>> Cc: "Rafael J. Wysocki" <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> ---
>> Hi Bjorn and Rafael,
>> The recursive lock changes haven't been tested yet, need help
>> from Alexander for testing.
>
> Well, let's just say I'm not a fan of recursive locks. Is that unavoidable
> here?
Yeah, you are right, we encounter other deadlock issue here, as reported
by Alexander. So need to find new solution here.
>
> Rafael
>
>
>> ---
>> drivers/acpi/dock.c | 33 +++++++++++++++++++++++++++------
>> drivers/pci/hotplug/acpiphp_glue.c | 32 ++++++++++++++++++--------------
>> 2 files changed, 45 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
>> index 02b0563..79c8d9e 100644
>> --- a/drivers/acpi/dock.c
>> +++ b/drivers/acpi/dock.c
>> @@ -65,6 +65,7 @@ struct dock_station {
>> u32 flags;
>> spinlock_t dd_lock;
>> struct mutex hp_lock;
>> + struct task_struct *owner;
>> struct list_head dependent_devices;
>> struct list_head hotplug_devices;
>>
>> @@ -131,9 +132,13 @@ 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);
>> + if (mutex_is_locked(&ds->hp_lock) && ds->owner == current) {
>> + list_add_tail(&dd->hotplug_list, &ds->hotplug_devices);
>> + } else {
>> + mutex_lock(&ds->hp_lock);
>> + list_add_tail(&dd->hotplug_list, &ds->hotplug_devices);
>> + mutex_unlock(&ds->hp_lock);
>> + }
>> }
>>
>> /**
>> @@ -147,9 +152,13 @@ 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);
>> + if (mutex_is_locked(&ds->hp_lock) && ds->owner == current) {
>> + list_del_init(&dd->hotplug_list);
>> + } else {
>> + mutex_lock(&ds->hp_lock);
>> + list_del_init(&dd->hotplug_list);
>> + mutex_unlock(&ds->hp_lock);
>> + }
>> }
>>
>> /**
>> @@ -355,7 +364,17 @@ static void hotplug_dock_devices(struct dock_station *ds, u32 event)
>> {
>> struct dock_dependent_device *dd;
>>
>> + /*
>> + * There is a deadlock scenario as below:
>> + * hotplug_dock_devices()
>> + * mutex_lock(&ds->hp_lock)
>> + * dd->ops->handler()
>> + * register_hotplug_dock_device()
>> + * mutex_lock(&ds->hp_lock)
>> + * So we need recursive lock scematics here, do it by ourselves.
>> + */
>> mutex_lock(&ds->hp_lock);
>> + ds->owner = current;
>>
>> /*
>> * First call driver specific hotplug functions
>> @@ -376,6 +395,8 @@ static void hotplug_dock_devices(struct dock_station *ds, u32 event)
>> else
>> dock_create_acpi_device(dd->handle);
>> }
>> +
>> + ds->owner = NULL;
>> mutex_unlock(&ds->hp_lock);
>> }
>>
>> 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);
>> }
>>
>> /*
>>
On Friday, June 14, 2013 09:53:57 PM Jiang Liu wrote:
> On 06/14/2013 03:59 AM, Rafael J. Wysocki wrote:
> > On Friday, June 14, 2013 12:32:25 AM Jiang Liu wrote:
> >> Current ACPI glue logic expects that physical devices are destroyed
> >> before destroying companion ACPI devices, otherwise it will break the
> >> ACPI unbind logic and cause following warning messages:
> >> [ 185.026073] usb usb5: Oops, 'acpi_handle' corrupt
> >> [ 185.035150] pci 0000:1b:00.0: Oops, 'acpi_handle' corrupt
> >> [ 185.035515] pci 0000:18:02.0: Oops, 'acpi_handle' corrupt
> >> [ 180.013656] port1: Oops, 'acpi_handle' corrupt
> >> Please refer to https://bugzilla.kernel.org/attachment.cgi?id=104321
> >> for full log message.
> >
> > So my question is, did we have this problem before commit 3b63aaa70e1?
> >
> > If we did, then when did it start? Or was it present forever?
> I think this issue should exist before commit "PCI: acpiphp: Do not use
> ACPI PCI subdriver mechanism". It may trace back to the changes to kill
> acpi_pci_bind()/acpi_pci_unbind().
I thought so.
> >> Above warning messages are caused by following scenario:
> >> 1) acpi_dock_notifier_call() queues a task (T1) onto kacpi_hotplug_wq
> >> 2) kacpi_hotplug_wq handles T1, which invokes acpi_dock_deferred_cb()
> >> ->dock_notify()-> handle_eject_request()->hotplug_dock_devices()
> >> 3) hotplug_dock_devices() first invokes registered hotplug callbacks to
> >> destroy physical devices, then destroys all affected ACPI devices.
> >> Everything seems perfect until now. But the acpiphp dock notification
> >> handler will queue another task (T2) onto kacpi_hotplug_wq to really
> >> destroy affected physical devices.
> >
> > Would not the solution be to modify it so that it didn't spawn the other
> > task (T2), but removed the affected physical devices synchronously?
> Yes, that's the way I'm going to fix this issue.
>
> >
> >> 4) kacpi_hotplug_wq finishes T1, and all affected ACPI devices have
> >> been destroyed.
> >> 5) kacpi_hotplug_wq handles T2, which destroys all affected physical
> >> devices.
> >>
> >> So it breaks ACPI glue logic's expection because ACPI devices are destroyed
> >> in step 3 and physical devices are destroyed in step 5.
> >>
> >> Signed-off-by: Jiang Liu <[email protected]>
> >> Reported-by: Alexander E. Patrakov <[email protected]>
> >> Cc: Bjorn Helgaas <[email protected]>
> >> Cc: Yinghai Lu <[email protected]>
> >> Cc: "Rafael J. Wysocki" <[email protected]>
> >> Cc: [email protected]
> >> Cc: [email protected]
> >> Cc: [email protected]
> >> ---
> >> Hi Bjorn and Rafael,
> >> The recursive lock changes haven't been tested yet, need help
> >> from Alexander for testing.
> >
> > Well, let's just say I'm not a fan of recursive locks. Is that unavoidable
> > here?
> Yeah, you are right, we encounter other deadlock issue here, as reported
> by Alexander. So need to find new solution here.
Can you please have a look at the patch I posted earlier in this thread?
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On 06/14/2013 08:23 PM, Rafael J. Wysocki wrote:
> On Thursday, June 13, 2013 09:59:44 PM Rafael J. Wysocki wrote:
>> On Friday, June 14, 2013 12:32:25 AM Jiang Liu wrote:
>>> Current ACPI glue logic expects that physical devices are destroyed
>>> before destroying companion ACPI devices, otherwise it will break the
>>> ACPI unbind logic and cause following warning messages:
>>> [ 185.026073] usb usb5: Oops, 'acpi_handle' corrupt
>>> [ 185.035150] pci 0000:1b:00.0: Oops, 'acpi_handle' corrupt
>>> [ 185.035515] pci 0000:18:02.0: Oops, 'acpi_handle' corrupt
>>> [ 180.013656] port1: Oops, 'acpi_handle' corrupt
>>> Please refer to https://bugzilla.kernel.org/attachment.cgi?id=104321
>>> for full log message.
>>
>> So my question is, did we have this problem before commit 3b63aaa70e1?
>>
>> If we did, then when did it start? Or was it present forever?
>>
>>> Above warning messages are caused by following scenario:
>>> 1) acpi_dock_notifier_call() queues a task (T1) onto kacpi_hotplug_wq
>>> 2) kacpi_hotplug_wq handles T1, which invokes acpi_dock_deferred_cb()
>>> ->dock_notify()-> handle_eject_request()->hotplug_dock_devices()
>>> 3) hotplug_dock_devices() first invokes registered hotplug callbacks to
>>> destroy physical devices, then destroys all affected ACPI devices.
>>> Everything seems perfect until now. But the acpiphp dock notification
>>> handler will queue another task (T2) onto kacpi_hotplug_wq to really
>>> destroy affected physical devices.
>>
>> Would not the solution be to modify it so that it didn't spawn the other
>> task (T2), but removed the affected physical devices synchronously?
>>
>>> 4) kacpi_hotplug_wq finishes T1, and all affected ACPI devices have
>>> been destroyed.
>>> 5) kacpi_hotplug_wq handles T2, which destroys all affected physical
>>> devices.
>>>
>>> So it breaks ACPI glue logic's expection because ACPI devices are destroyed
>>> in step 3 and physical devices are destroyed in step 5.
>>>
>>> Signed-off-by: Jiang Liu <[email protected]>
>>> Reported-by: Alexander E. Patrakov <[email protected]>
>>> Cc: Bjorn Helgaas <[email protected]>
>>> Cc: Yinghai Lu <[email protected]>
>>> Cc: "Rafael J. Wysocki" <[email protected]>
>>> Cc: [email protected]
>>> Cc: [email protected]
>>> Cc: [email protected]
>>> ---
>>> Hi Bjorn and Rafael,
>>> The recursive lock changes haven't been tested yet, need help
>>> from Alexander for testing.
>>
>> Well, let's just say I'm not a fan of recursive locks. Is that unavoidable
>> here?
>
> What about the appended patch (on top of [1/9], untested)?
>
> Rafael
It should have similar effect as patch 2/9, and it will encounter the
same deadlock scenario as 2/9 too.
>
>
> ---
> drivers/pci/hotplug/acpiphp_glue.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> Index: linux-pm/drivers/pci/hotplug/acpiphp_glue.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/hotplug/acpiphp_glue.c
> +++ linux-pm/drivers/pci/hotplug/acpiphp_glue.c
> @@ -145,9 +145,20 @@ static int post_dock_fixups(struct notif
> return NOTIFY_OK;
> }
>
> +static void handle_dock_event_func(acpi_handle handle, u32 event, void *context)
> +{
> + if (event == ACPI_NOTIFY_EJECT_REQUEST) {
> + struct acpiphp_func *func = context;
> +
> + if (!acpiphp_disable_slot(func->slot))
> + acpiphp_eject_slot(func->slot);
> + } else {
> + handle_hotplug_event_func(handle, event, context);
> + }
> +}
>
> static const struct acpi_dock_ops acpiphp_dock_ops = {
> - .handler = handle_hotplug_event_func,
> + .handler = handle_dock_event_func,
> };
>
> /* Check whether the PCI device is managed by native PCIe hotplug driver */
>
On Friday, June 14, 2013 09:57:15 PM Jiang Liu wrote:
> On 06/14/2013 08:23 PM, Rafael J. Wysocki wrote:
> > On Thursday, June 13, 2013 09:59:44 PM Rafael J. Wysocki wrote:
> >> On Friday, June 14, 2013 12:32:25 AM Jiang Liu wrote:
> >>> Current ACPI glue logic expects that physical devices are destroyed
> >>> before destroying companion ACPI devices, otherwise it will break the
> >>> ACPI unbind logic and cause following warning messages:
> >>> [ 185.026073] usb usb5: Oops, 'acpi_handle' corrupt
> >>> [ 185.035150] pci 0000:1b:00.0: Oops, 'acpi_handle' corrupt
> >>> [ 185.035515] pci 0000:18:02.0: Oops, 'acpi_handle' corrupt
> >>> [ 180.013656] port1: Oops, 'acpi_handle' corrupt
> >>> Please refer to https://bugzilla.kernel.org/attachment.cgi?id=104321
> >>> for full log message.
> >>
> >> So my question is, did we have this problem before commit 3b63aaa70e1?
> >>
> >> If we did, then when did it start? Or was it present forever?
> >>
> >>> Above warning messages are caused by following scenario:
> >>> 1) acpi_dock_notifier_call() queues a task (T1) onto kacpi_hotplug_wq
> >>> 2) kacpi_hotplug_wq handles T1, which invokes acpi_dock_deferred_cb()
> >>> ->dock_notify()-> handle_eject_request()->hotplug_dock_devices()
> >>> 3) hotplug_dock_devices() first invokes registered hotplug callbacks to
> >>> destroy physical devices, then destroys all affected ACPI devices.
> >>> Everything seems perfect until now. But the acpiphp dock notification
> >>> handler will queue another task (T2) onto kacpi_hotplug_wq to really
> >>> destroy affected physical devices.
> >>
> >> Would not the solution be to modify it so that it didn't spawn the other
> >> task (T2), but removed the affected physical devices synchronously?
> >>
> >>> 4) kacpi_hotplug_wq finishes T1, and all affected ACPI devices have
> >>> been destroyed.
> >>> 5) kacpi_hotplug_wq handles T2, which destroys all affected physical
> >>> devices.
> >>>
> >>> So it breaks ACPI glue logic's expection because ACPI devices are destroyed
> >>> in step 3 and physical devices are destroyed in step 5.
> >>>
> >>> Signed-off-by: Jiang Liu <[email protected]>
> >>> Reported-by: Alexander E. Patrakov <[email protected]>
> >>> Cc: Bjorn Helgaas <[email protected]>
> >>> Cc: Yinghai Lu <[email protected]>
> >>> Cc: "Rafael J. Wysocki" <[email protected]>
> >>> Cc: [email protected]
> >>> Cc: [email protected]
> >>> Cc: [email protected]
> >>> ---
> >>> Hi Bjorn and Rafael,
> >>> The recursive lock changes haven't been tested yet, need help
> >>> from Alexander for testing.
> >>
> >> Well, let's just say I'm not a fan of recursive locks. Is that unavoidable
> >> here?
> >
> > What about the appended patch (on top of [1/9], untested)?
> >
> > Rafael
> It should have similar effect as patch 2/9, and it will encounter the
> same deadlock scenario as 2/9 too.
And why exactly?
I'm looking at acpiphp_disable_slot() and I'm not seeing where the
problematic lock is taken. Similarly for power_off_slot().
It should take the ACPI scan lock, but that's a different matter.
Thanks,
Rafael
> > ---
> > drivers/pci/hotplug/acpiphp_glue.c | 13 ++++++++++++-
> > 1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > Index: linux-pm/drivers/pci/hotplug/acpiphp_glue.c
> > ===================================================================
> > --- linux-pm.orig/drivers/pci/hotplug/acpiphp_glue.c
> > +++ linux-pm/drivers/pci/hotplug/acpiphp_glue.c
> > @@ -145,9 +145,20 @@ static int post_dock_fixups(struct notif
> > return NOTIFY_OK;
> > }
> >
> > +static void handle_dock_event_func(acpi_handle handle, u32 event, void *context)
> > +{
> > + if (event == ACPI_NOTIFY_EJECT_REQUEST) {
> > + struct acpiphp_func *func = context;
> > +
> > + if (!acpiphp_disable_slot(func->slot))
> > + acpiphp_eject_slot(func->slot);
> > + } else {
> > + handle_hotplug_event_func(handle, event, context);
> > + }
> > +}
> >
> > static const struct acpi_dock_ops acpiphp_dock_ops = {
> > - .handler = handle_hotplug_event_func,
> > + .handler = handle_dock_event_func,
> > };
> >
> > /* Check whether the PCI device is managed by native PCIe hotplug driver */
> >
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On 06/14/2013 02:26 AM, Rafael J. Wysocki wrote:
> On Friday, June 14, 2013 12:32:26 AM Jiang Liu wrote:
>> ACPI dock driver can't be built as a module any more, so clean up
>> module related code.
>>
>> Signed-off-by: Jiang Liu <[email protected]>
>> Cc: Shaohua Li <[email protected]>
>> Cc: Len Brown <[email protected]>
>> Cc: "Rafael J. Wysocki" <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>
> How exactly does this depend on [2/9]? If it doesn't at all, it should go
> after [1/9].
>
>> ---
>> drivers/acpi/dock.c | 41 -----------------------------------------
>> 1 file changed, 41 deletions(-)
>>
>> diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
>> index 79c8d9e..50e38b7 100644
>> --- a/drivers/acpi/dock.c
>> +++ b/drivers/acpi/dock.c
>> @@ -53,12 +53,6 @@ MODULE_PARM_DESC(immediate_undock, "1 (default) will cause the driver to "
>>
>> static struct atomic_notifier_head dock_notifier_list;
>>
>> -static const struct acpi_device_id dock_device_ids[] = {
>> - {"LNXDOCK", 0},
>> - {"", 0},
>> -};
>> -MODULE_DEVICE_TABLE(acpi, dock_device_ids);
>> -
>
> Don't we actually need the device IDs?
Now dock driver could only be built as built-in, and it doesn't really
bind to ACPI dock devices, so I think the device ids are not used any
more. Not sure whether any userspace tool has dependency on the device
IDs.
>
>> struct dock_station {
>> acpi_handle handle;
>> unsigned long last_dock_time;
>> @@ -1013,30 +1007,6 @@ err_unregister:
>> }
>>
>> /**
>> - * dock_remove - free up resources related to the dock station
>> - */
>> -static int dock_remove(struct dock_station *ds)
>> -{
>> - struct dock_dependent_device *dd, *tmp;
>> - struct platform_device *dock_device = ds->dock_device;
>> -
>> - if (!dock_station_count)
>> - return 0;
>> -
>> - /* remove dependent devices */
>> - list_for_each_entry_safe(dd, tmp, &ds->dependent_devices, list)
>> - kfree(dd);
>> -
>> - list_del(&ds->sibling);
>> -
>> - /* cleanup sysfs */
>> - sysfs_remove_group(&dock_device->dev.kobj, &dock_attribute_group);
>> - platform_device_unregister(dock_device);
>> -
>> - return 0;
>> -}
>> -
>> -/**
>> * find_dock_and_bay - look for dock stations and bays
>> * @handle: acpi handle of a device
>> * @lvl: unused
>> @@ -1073,14 +1043,3 @@ int __init acpi_dock_init(void)
>> ACPI_DOCK_DRIVER_DESCRIPTION, dock_station_count);
>> return 0;
>> }
>> -
>> -static void __exit dock_exit(void)
>> -{
>> - struct dock_station *tmp, *dock_station;
>> -
>> - unregister_acpi_bus_notifier(&dock_acpi_notifier);
>> - list_for_each_entry_safe(dock_station, tmp, &dock_stations, sibling)
>> - dock_remove(dock_station);
>> -}
>> -
>> -module_exit(dock_exit);
>
> The other changes look OK to me.
Thanks for review.
>
> Thanks,
> Rafael
>
>
On 06/14/2013 02:28 AM, Rafael J. Wysocki wrote:
> On Friday, June 14, 2013 12:32:28 AM Jiang Liu wrote:
>> All dock device related data structures are created during driver
>> initialization, so kill the redundant spin lock in dock device object.
>
> As a cleanup, it definitely makes sense, but does it fix anything?
>
> Rafael
Patch 3-9 are code cleanup only, I will resend them as separate patchset
later.
>
>
>> Signed-off-by: Jiang Liu <[email protected]>
>> Cc: Shaohua Li <[email protected]>
>> Cc: Len Brown <[email protected]>
>> Cc: "Rafael J. Wysocki" <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> ---
>> drivers/acpi/dock.c | 15 +++------------
>> 1 file changed, 3 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
>> index 8e578aa..cd2d5df 100644
>> --- a/drivers/acpi/dock.c
>> +++ b/drivers/acpi/dock.c
>> @@ -57,7 +57,6 @@ struct dock_station {
>> acpi_handle handle;
>> unsigned long last_dock_time;
>> u32 flags;
>> - spinlock_t dd_lock;
>> struct mutex hp_lock;
>> struct task_struct *owner;
>> struct list_head dependent_devices;
>> @@ -107,10 +106,7 @@ 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);
>> - spin_unlock(&ds->dd_lock);
>>
>> return 0;
>> }
>> @@ -168,14 +164,10 @@ find_dock_dependent_device(struct dock_station *ds, acpi_handle handle)
>> {
>> struct dock_dependent_device *dd;
>>
>> - spin_lock(&ds->dd_lock);
>> - list_for_each_entry(dd, &ds->dependent_devices, list) {
>> - if (handle == dd->handle) {
>> - spin_unlock(&ds->dd_lock);
>> + list_for_each_entry(dd, &ds->dependent_devices, list)
>> + if (handle == dd->handle)
>> return dd;
>> - }
>> - }
>> - spin_unlock(&ds->dd_lock);
>> +
>> return NULL;
>> }
>>
>> @@ -964,7 +956,6 @@ static int __init dock_add(acpi_handle handle)
>> 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);
>> INIT_LIST_HEAD(&dock_station->dependent_devices);
>>
On Friday, June 14, 2013 10:04:01 PM Jiang Liu wrote:
> On 06/14/2013 02:26 AM, Rafael J. Wysocki wrote:
> > On Friday, June 14, 2013 12:32:26 AM Jiang Liu wrote:
> >> ACPI dock driver can't be built as a module any more, so clean up
> >> module related code.
> >>
> >> Signed-off-by: Jiang Liu <[email protected]>
> >> Cc: Shaohua Li <[email protected]>
> >> Cc: Len Brown <[email protected]>
> >> Cc: "Rafael J. Wysocki" <[email protected]>
> >> Cc: [email protected]
> >> Cc: [email protected]
> >
> > How exactly does this depend on [2/9]? If it doesn't at all, it should go
> > after [1/9].
> >
> >> ---
> >> drivers/acpi/dock.c | 41 -----------------------------------------
> >> 1 file changed, 41 deletions(-)
> >>
> >> diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
> >> index 79c8d9e..50e38b7 100644
> >> --- a/drivers/acpi/dock.c
> >> +++ b/drivers/acpi/dock.c
> >> @@ -53,12 +53,6 @@ MODULE_PARM_DESC(immediate_undock, "1 (default) will cause the driver to "
> >>
> >> static struct atomic_notifier_head dock_notifier_list;
> >>
> >> -static const struct acpi_device_id dock_device_ids[] = {
> >> - {"LNXDOCK", 0},
> >> - {"", 0},
> >> -};
> >> -MODULE_DEVICE_TABLE(acpi, dock_device_ids);
> >> -
> >
> > Don't we actually need the device IDs?
> Now dock driver could only be built as built-in, and it doesn't really
> bind to ACPI dock devices, so I think the device ids are not used any
> more. Not sure whether any userspace tool has dependency on the device
> IDs.
I see. OK
Thanks,
Rafael
> >
> >> struct dock_station {
> >> acpi_handle handle;
> >> unsigned long last_dock_time;
> >> @@ -1013,30 +1007,6 @@ err_unregister:
> >> }
> >>
> >> /**
> >> - * dock_remove - free up resources related to the dock station
> >> - */
> >> -static int dock_remove(struct dock_station *ds)
> >> -{
> >> - struct dock_dependent_device *dd, *tmp;
> >> - struct platform_device *dock_device = ds->dock_device;
> >> -
> >> - if (!dock_station_count)
> >> - return 0;
> >> -
> >> - /* remove dependent devices */
> >> - list_for_each_entry_safe(dd, tmp, &ds->dependent_devices, list)
> >> - kfree(dd);
> >> -
> >> - list_del(&ds->sibling);
> >> -
> >> - /* cleanup sysfs */
> >> - sysfs_remove_group(&dock_device->dev.kobj, &dock_attribute_group);
> >> - platform_device_unregister(dock_device);
> >> -
> >> - return 0;
> >> -}
> >> -
> >> -/**
> >> * find_dock_and_bay - look for dock stations and bays
> >> * @handle: acpi handle of a device
> >> * @lvl: unused
> >> @@ -1073,14 +1043,3 @@ int __init acpi_dock_init(void)
> >> ACPI_DOCK_DRIVER_DESCRIPTION, dock_station_count);
> >> return 0;
> >> }
> >> -
> >> -static void __exit dock_exit(void)
> >> -{
> >> - struct dock_station *tmp, *dock_station;
> >> -
> >> - unregister_acpi_bus_notifier(&dock_acpi_notifier);
> >> - list_for_each_entry_safe(dock_station, tmp, &dock_stations, sibling)
> >> - dock_remove(dock_station);
> >> -}
> >> -
> >> -module_exit(dock_exit);
> >
> > The other changes look OK to me.
> Thanks for review.
>
> >
> > Thanks,
> > Rafael
> >
> >
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Friday, June 14, 2013 10:05:47 PM Jiang Liu wrote:
> On 06/14/2013 02:28 AM, Rafael J. Wysocki wrote:
> > On Friday, June 14, 2013 12:32:28 AM Jiang Liu wrote:
> >> All dock device related data structures are created during driver
> >> initialization, so kill the redundant spin lock in dock device object.
> >
> > As a cleanup, it definitely makes sense, but does it fix anything?
> >
> > Rafael
> Patch 3-9 are code cleanup only, I will resend them as separate patchset
> later.
Cool, thanks!
Rafael
> >> Signed-off-by: Jiang Liu <[email protected]>
> >> Cc: Shaohua Li <[email protected]>
> >> Cc: Len Brown <[email protected]>
> >> Cc: "Rafael J. Wysocki" <[email protected]>
> >> Cc: [email protected]
> >> Cc: [email protected]
> >> ---
> >> drivers/acpi/dock.c | 15 +++------------
> >> 1 file changed, 3 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
> >> index 8e578aa..cd2d5df 100644
> >> --- a/drivers/acpi/dock.c
> >> +++ b/drivers/acpi/dock.c
> >> @@ -57,7 +57,6 @@ struct dock_station {
> >> acpi_handle handle;
> >> unsigned long last_dock_time;
> >> u32 flags;
> >> - spinlock_t dd_lock;
> >> struct mutex hp_lock;
> >> struct task_struct *owner;
> >> struct list_head dependent_devices;
> >> @@ -107,10 +106,7 @@ 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);
> >> - spin_unlock(&ds->dd_lock);
> >>
> >> return 0;
> >> }
> >> @@ -168,14 +164,10 @@ find_dock_dependent_device(struct dock_station *ds, acpi_handle handle)
> >> {
> >> struct dock_dependent_device *dd;
> >>
> >> - spin_lock(&ds->dd_lock);
> >> - list_for_each_entry(dd, &ds->dependent_devices, list) {
> >> - if (handle == dd->handle) {
> >> - spin_unlock(&ds->dd_lock);
> >> + list_for_each_entry(dd, &ds->dependent_devices, list)
> >> + if (handle == dd->handle)
> >> return dd;
> >> - }
> >> - }
> >> - spin_unlock(&ds->dd_lock);
> >> +
> >> return NULL;
> >> }
> >>
> >> @@ -964,7 +956,6 @@ static int __init dock_add(acpi_handle handle)
> >> 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);
> >> INIT_LIST_HEAD(&dock_station->dependent_devices);
> >>
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On 06/14/2013 10:12 PM, Rafael J. Wysocki wrote:
> On Friday, June 14, 2013 09:57:15 PM Jiang Liu wrote:
>> On 06/14/2013 08:23 PM, Rafael J. Wysocki wrote:
>>> On Thursday, June 13, 2013 09:59:44 PM Rafael J. Wysocki wrote:
>>>> On Friday, June 14, 2013 12:32:25 AM Jiang Liu wrote:
>>>>> Current ACPI glue logic expects that physical devices are destroyed
>>>>> before destroying companion ACPI devices, otherwise it will break the
>>>>> ACPI unbind logic and cause following warning messages:
>>>>> [ 185.026073] usb usb5: Oops, 'acpi_handle' corrupt
>>>>> [ 185.035150] pci 0000:1b:00.0: Oops, 'acpi_handle' corrupt
>>>>> [ 185.035515] pci 0000:18:02.0: Oops, 'acpi_handle' corrupt
>>>>> [ 180.013656] port1: Oops, 'acpi_handle' corrupt
>>>>> Please refer to https://bugzilla.kernel.org/attachment.cgi?id=104321
>>>>> for full log message.
>>>>
>>>> So my question is, did we have this problem before commit 3b63aaa70e1?
>>>>
>>>> If we did, then when did it start? Or was it present forever?
>>>>
>>>>> Above warning messages are caused by following scenario:
>>>>> 1) acpi_dock_notifier_call() queues a task (T1) onto kacpi_hotplug_wq
>>>>> 2) kacpi_hotplug_wq handles T1, which invokes acpi_dock_deferred_cb()
>>>>> ->dock_notify()-> handle_eject_request()->hotplug_dock_devices()
>>>>> 3) hotplug_dock_devices() first invokes registered hotplug callbacks to
>>>>> destroy physical devices, then destroys all affected ACPI devices.
>>>>> Everything seems perfect until now. But the acpiphp dock notification
>>>>> handler will queue another task (T2) onto kacpi_hotplug_wq to really
>>>>> destroy affected physical devices.
>>>>
>>>> Would not the solution be to modify it so that it didn't spawn the other
>>>> task (T2), but removed the affected physical devices synchronously?
>>>>
>>>>> 4) kacpi_hotplug_wq finishes T1, and all affected ACPI devices have
>>>>> been destroyed.
>>>>> 5) kacpi_hotplug_wq handles T2, which destroys all affected physical
>>>>> devices.
>>>>>
>>>>> So it breaks ACPI glue logic's expection because ACPI devices are destroyed
>>>>> in step 3 and physical devices are destroyed in step 5.
>>>>>
>>>>> Signed-off-by: Jiang Liu <[email protected]>
>>>>> Reported-by: Alexander E. Patrakov <[email protected]>
>>>>> Cc: Bjorn Helgaas <[email protected]>
>>>>> Cc: Yinghai Lu <[email protected]>
>>>>> Cc: "Rafael J. Wysocki" <[email protected]>
>>>>> Cc: [email protected]
>>>>> Cc: [email protected]
>>>>> Cc: [email protected]
>>>>> ---
>>>>> Hi Bjorn and Rafael,
>>>>> The recursive lock changes haven't been tested yet, need help
>>>>> from Alexander for testing.
>>>>
>>>> Well, let's just say I'm not a fan of recursive locks. Is that unavoidable
>>>> here?
>>>
>>> What about the appended patch (on top of [1/9], untested)?
>>>
>>> Rafael
>> It should have similar effect as patch 2/9, and it will encounter the
>> same deadlock scenario as 2/9 too.
>
> And why exactly?
>
> I'm looking at acpiphp_disable_slot() and I'm not seeing where the
> problematic lock is taken. Similarly for power_off_slot().
>
> It should take the ACPI scan lock, but that's a different matter.
>
> Thanks,
> Rafael
The deadlock scenario is the same:
hotplug_dock_devices()
mutex_lock(&ds->hp_lock)
dd->ops->handler()
destroy pci bus
unregister_hotplug_dock_device()
mutex_lock(&ds->hp_lock)
>
>
>>> ---
>>> drivers/pci/hotplug/acpiphp_glue.c | 13 ++++++++++++-
>>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>>
>>> Index: linux-pm/drivers/pci/hotplug/acpiphp_glue.c
>>> ===================================================================
>>> --- linux-pm.orig/drivers/pci/hotplug/acpiphp_glue.c
>>> +++ linux-pm/drivers/pci/hotplug/acpiphp_glue.c
>>> @@ -145,9 +145,20 @@ static int post_dock_fixups(struct notif
>>> return NOTIFY_OK;
>>> }
>>>
>>> +static void handle_dock_event_func(acpi_handle handle, u32 event, void *context)
>>> +{
>>> + if (event == ACPI_NOTIFY_EJECT_REQUEST) {
>>> + struct acpiphp_func *func = context;
>>> +
>>> + if (!acpiphp_disable_slot(func->slot))
>>> + acpiphp_eject_slot(func->slot);
>>> + } else {
>>> + handle_hotplug_event_func(handle, event, context);
>>> + }
>>> +}
>>>
>>> static const struct acpi_dock_ops acpiphp_dock_ops = {
>>> - .handler = handle_hotplug_event_func,
>>> + .handler = handle_dock_event_func,
>>> };
>>>
>>> /* Check whether the PCI device is managed by native PCIe hotplug driver */
>>>
>>
2013/6/14 Rafael J. Wysocki <[email protected]>:
> As far as I'm concerned, please test these two for now:
>
> https://patchwork.kernel.org/patch/2717851/
> https://patchwork.kernel.org/patch/2721271/
Deadlocked on undocking, as already found by code inspection. Dmesg attached.
--
Alexander E. Patrakov
On Friday, June 14, 2013 11:30:12 PM Jiang Liu wrote:
> On 06/14/2013 10:12 PM, Rafael J. Wysocki wrote:
> > On Friday, June 14, 2013 09:57:15 PM Jiang Liu wrote:
> >> On 06/14/2013 08:23 PM, Rafael J. Wysocki wrote:
> >>> On Thursday, June 13, 2013 09:59:44 PM Rafael J. Wysocki wrote:
> >>>> On Friday, June 14, 2013 12:32:25 AM Jiang Liu wrote:
> >>>>> Current ACPI glue logic expects that physical devices are destroyed
> >>>>> before destroying companion ACPI devices, otherwise it will break the
> >>>>> ACPI unbind logic and cause following warning messages:
> >>>>> [ 185.026073] usb usb5: Oops, 'acpi_handle' corrupt
> >>>>> [ 185.035150] pci 0000:1b:00.0: Oops, 'acpi_handle' corrupt
> >>>>> [ 185.035515] pci 0000:18:02.0: Oops, 'acpi_handle' corrupt
> >>>>> [ 180.013656] port1: Oops, 'acpi_handle' corrupt
> >>>>> Please refer to https://bugzilla.kernel.org/attachment.cgi?id=104321
> >>>>> for full log message.
> >>>>
> >>>> So my question is, did we have this problem before commit 3b63aaa70e1?
> >>>>
> >>>> If we did, then when did it start? Or was it present forever?
> >>>>
> >>>>> Above warning messages are caused by following scenario:
> >>>>> 1) acpi_dock_notifier_call() queues a task (T1) onto kacpi_hotplug_wq
> >>>>> 2) kacpi_hotplug_wq handles T1, which invokes acpi_dock_deferred_cb()
> >>>>> ->dock_notify()-> handle_eject_request()->hotplug_dock_devices()
> >>>>> 3) hotplug_dock_devices() first invokes registered hotplug callbacks to
> >>>>> destroy physical devices, then destroys all affected ACPI devices.
> >>>>> Everything seems perfect until now. But the acpiphp dock notification
> >>>>> handler will queue another task (T2) onto kacpi_hotplug_wq to really
> >>>>> destroy affected physical devices.
> >>>>
> >>>> Would not the solution be to modify it so that it didn't spawn the other
> >>>> task (T2), but removed the affected physical devices synchronously?
> >>>>
> >>>>> 4) kacpi_hotplug_wq finishes T1, and all affected ACPI devices have
> >>>>> been destroyed.
> >>>>> 5) kacpi_hotplug_wq handles T2, which destroys all affected physical
> >>>>> devices.
> >>>>>
> >>>>> So it breaks ACPI glue logic's expection because ACPI devices are destroyed
> >>>>> in step 3 and physical devices are destroyed in step 5.
> >>>>>
> >>>>> Signed-off-by: Jiang Liu <[email protected]>
> >>>>> Reported-by: Alexander E. Patrakov <[email protected]>
> >>>>> Cc: Bjorn Helgaas <[email protected]>
> >>>>> Cc: Yinghai Lu <[email protected]>
> >>>>> Cc: "Rafael J. Wysocki" <[email protected]>
> >>>>> Cc: [email protected]
> >>>>> Cc: [email protected]
> >>>>> Cc: [email protected]
> >>>>> ---
> >>>>> Hi Bjorn and Rafael,
> >>>>> The recursive lock changes haven't been tested yet, need help
> >>>>> from Alexander for testing.
> >>>>
> >>>> Well, let's just say I'm not a fan of recursive locks. Is that unavoidable
> >>>> here?
> >>>
> >>> What about the appended patch (on top of [1/9], untested)?
> >>>
> >>> Rafael
> >> It should have similar effect as patch 2/9, and it will encounter the
> >> same deadlock scenario as 2/9 too.
> >
> > And why exactly?
> >
> > I'm looking at acpiphp_disable_slot() and I'm not seeing where the
> > problematic lock is taken. Similarly for power_off_slot().
> >
> > It should take the ACPI scan lock, but that's a different matter.
> >
> > Thanks,
> > Rafael
> The deadlock scenario is the same:
Well, you didn't answer my question.
> hotplug_dock_devices()
> mutex_lock(&ds->hp_lock)
> dd->ops->handler()
> destroy pci bus
And this wasn't particularly helpful.
What about mentioning acpi_pci_remove_bus()? I don't remember all changes
made recently, sorry.
> unregister_hotplug_dock_device()
> mutex_lock(&ds->hp_lock)
I see the problem.
ds->hp_lock is used to make both addition and removal of hotplug devices wait
for us to complete walking ds->hotplug_devices. However, if we are in the
process of removing hotplug devices, we don't need removals to block on
ds->hp_lock (in fact, we don't even want them to block on it). Conversely, if
we are in the process of adding hotplug devices, we don't want additions to
block on ds->hp_lock.
So, why don't we do the following:
(1) Introduce a 'hotplug_status' field into struct_dock station with possible
values representing "removal", "addition" and "no action" and a wait queue
associated with it. We can use ds->dd_lock to protect that field.
(2) hotplug_status will be modified by hotplug_dock_devices() depending on the
event. For example, on eject it will set hotplug_status to "removal".
Then, after completing the walk, it will reset hotplug_status to
"no action" and wake up its wait queue.
(3) dock_del_hotplug_device() will check if hotplug_status is "removal" or
"no_action" and if so, it will just do the removal under ds->dd_lock. If
it is "addition", though, it will go to sleep (in the hotplug_status' wait
queue) until hotplug_dock_devices() wakes it up.
(4) Analogously for dock_add_hotplug_device().
For this to work the "addition" and "deletion" of hotplug devices should better
be implemented as setting and clearing a 'hotplug' flag in
struct dock_dependent_device (instead of using ds->hotplug_devices).
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.