Hi All,
It turns out that some BIOSes add container device IDs as _CIDs to device
object that in principle may be matched against the other scan handlers (or
ACPI drivers, but that's not a problem, because the container scan handler
can co-exist with an ACPI driver). That's why our recent fix for an issue
related to the ACPI video driver had to be reverted right before -rc5.
Although I submitted an alternative fix for that bug, I think the problem
with the container scan handler possibly matching devices already having
some other scan handlers attached needs addressing, because we may need to
use the container hotplug profile for those devices. The following patch
series is supposed to address it.
[1/5] ACPI / scan: Do not bind ACPI drivers to objects with scan handlers
(this version shouldn't break the Tony's IA64 HP box the previous one broke)
[2/5] ACPI / scan: Separate hotplug profiles from scan handlers
[3/5] ACPI / scan: Add hotplug profile pointer to struct acpi_device
[4/5] ACPI / scan: Use container hotplug profile for matching device objects
[5/5] ACPI / ia64 / sba_iommu: Use ACPI scan handler for discovery
Patches [1-4/5] were run on my Toshiba test box and didn't break it, but it
really doesn't do any ACPI hotplug notifications.
Patch [5/5] is kind of additional, but it wouldn't work correctly without the
previous ones (to be honest, I haven't tried to compile it yet, but here it
goes for completness).
The patches are against the linux-next branch of the linux-pm.git tree.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
From: Rafael J. Wysocki <[email protected]>
On some systems (ia64 HP rx2600 in particular) the ACPI namespace
contains device objects with container compatibility IDs in addition
to some other hardware IDs. This means that those devices are
containers and if an ACPI scan handler without a hotplug profile is
attached to one of them, the container hotplug profile should be used
for that device.
Moreover, the container hotplug profile should be used for handling
bus check and device check notifications targeted at device handles
with matching scan handlers that don't provide hotplug profiles.
Modify the ACPI namespace scan code to ensure that the above will
happen.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/scan.c | 33 ++++++++++++++++++++++++++++-----
1 file changed, 28 insertions(+), 5 deletions(-)
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1855,11 +1855,26 @@ void acpi_scan_hotplug_enabled(struct ac
mutex_unlock(&acpi_scan_lock);
}
+static struct acpi_hotplug_profile *acpi_container_hotplug(
+ struct acpi_device_pnp *pnp)
+{
+ if (acpi_container_scan_handler()) {
+ struct acpi_scan_handler *handler;
+ struct acpi_hardware_id *hwid;
+
+ handler = acpi_container_scan_handler();
+ list_for_each_entry(hwid, &pnp->ids, list)
+ if (acpi_scan_handler_matching(handler, hwid->id, NULL))
+ return handler->hotplug;
+ }
+ return NULL;
+}
+
static void acpi_scan_init_hotplug(acpi_handle handle, int type)
{
struct acpi_device_pnp pnp = {};
struct acpi_hardware_id *hwid;
- struct acpi_scan_handler *handler;
+ struct acpi_hotplug_profile *hotplug = NULL;
INIT_LIST_HEAD(&pnp.ids);
acpi_set_pnp_ids(handle, &pnp, type);
@@ -1872,14 +1887,20 @@ static void acpi_scan_init_hotplug(acpi_
* install the same notify handler routine twice for the same handle.
*/
list_for_each_entry(hwid, &pnp.ids, list) {
+ struct acpi_scan_handler *handler;
+
handler = acpi_scan_match_handler(hwid->id, NULL);
if (handler && handler->hotplug) {
- acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
- acpi_hotplug_notify_cb,
- handler->hotplug);
+ hotplug = handler->hotplug;
break;
}
}
+ if (!hotplug)
+ hotplug = acpi_container_hotplug(&pnp);
+
+ if (hotplug)
+ acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
+ acpi_hotplug_notify_cb, hotplug);
out:
acpi_free_pnp_ids(&pnp);
@@ -1948,7 +1969,9 @@ static int acpi_scan_attach_handler(stru
ret = handler->attach(device, devid);
if (ret > 0) {
device->handler = handler;
- device->hotplug = handler->hotplug;
+ device->hotplug = handler->hotplug ?
+ handler->hotplug :
+ acpi_container_hotplug(&device->pnp);
break;
} else if (ret < 0) {
break;
From: Rafael J. Wysocki <[email protected]>
ACPI drivers generally should not be bound to device objects having
scan handlers attatched to them.
However, there is one exception which is the container scan handler.
Namely, it turns out that some BIOSes (on ia64 HP rx2600 in
particular) put container device IDs into the same objects along with
some other more specific ones which makes those device objects match
the container scan handler as well as some other scan handlers or
ACPI drivers. Fortunately, though, the container scan handler is
only needed for hotplug to work and it doesn't actually do anything
to the device objects in question, so it is really OK to have both
the container scan handler and something else attached to the same
device object at the same time.
Following that, make acpi_device_probe() fail with -EINVAL if the
device object being probed has an ACPI scan handler that is not the
container scan handler attached to it.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/container.c | 5 +++--
drivers/acpi/internal.h | 10 ++++++++++
drivers/acpi/scan.c | 4 ++++
drivers/acpi/video.c | 3 ---
4 files changed, 17 insertions(+), 5 deletions(-)
Index: linux-pm/drivers/acpi/container.c
===================================================================
--- linux-pm.orig/drivers/acpi/container.c
+++ linux-pm/drivers/acpi/container.c
@@ -51,7 +51,7 @@ static int container_device_attach(struc
return 1;
}
-static struct acpi_scan_handler container_handler = {
+struct acpi_scan_handler container_scan_handler = {
.ids = container_device_ids,
.attach = container_device_attach,
.hotplug = {
@@ -62,5 +62,6 @@ static struct acpi_scan_handler containe
void __init acpi_container_init(void)
{
- acpi_scan_add_handler_with_hotplug(&container_handler, "container");
+ acpi_scan_add_handler_with_hotplug(&container_scan_handler,
+ "container");
}
Index: linux-pm/drivers/acpi/internal.h
===================================================================
--- linux-pm.orig/drivers/acpi/internal.h
+++ linux-pm/drivers/acpi/internal.h
@@ -37,8 +37,18 @@ void acpi_processor_init(void);
void acpi_platform_init(void);
int acpi_sysfs_init(void);
#ifdef CONFIG_ACPI_CONTAINER
+extern struct acpi_scan_handler container_scan_handler;
+
+static inline struct acpi_scan_handler *acpi_container_scan_handler(void)
+{
+ return &container_scan_handler;
+}
void acpi_container_init(void);
#else
+static inline struct acpi_scan_handler *acpi_container_scan_handler(void)
+{
+ return NULL;
+}
static inline void acpi_container_init(void) {}
#endif
#ifdef CONFIG_ACPI_HOTPLUG_MEMORY
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -939,6 +939,10 @@ static int acpi_device_probe(struct devi
struct acpi_driver *acpi_drv = to_acpi_driver(dev->driver);
int ret;
+ if (acpi_dev->handler
+ && acpi_dev->handler != acpi_container_scan_handler())
+ return -EINVAL;
+
if (!acpi_drv->ops.add)
return -ENOSYS;
Index: linux-pm/drivers/acpi/video.c
===================================================================
--- linux-pm.orig/drivers/acpi/video.c
+++ linux-pm/drivers/acpi/video.c
@@ -1722,9 +1722,6 @@ static int acpi_video_bus_add(struct acp
int error;
acpi_status status;
- if (device->handler)
- return -EINVAL;
-
status = acpi_walk_namespace(ACPI_TYPE_DEVICE,
device->parent->handle, 1,
acpi_video_bus_match, NULL,
From: Rafael J. Wysocki <[email protected]>
The IA64 System Bus Adapter (SBA) I/O MMU driver uses an ACPI driver
object to look for device objects it needs in the ACPI namespace, but
the way that driver object is used by it is kind of abusive.
First of all, the ACPI driver is registered with the assumption that
its .add() routine (an equivalent of .probe()) will run immediately
during the registration, which in principle may or may not happen (it
happens in practice due to the way the kernel is built, but that's
not actually guaranteed). Second, .add() is the only functionality
provided by that driver object (which means that it probes devices
without really doing anything with them afterward) and that .add() is
only supposed to run during system initialization, so having the
driver object registered (and present in sysfs) throughout the
system's life time isn't particularly useful.
A cleaner way to achieve the same goal that driver object is for
is to use an ACPI scan handler instead of it, which at least
prevents sysfs from being littered with useless data and causes
an improved algorithm of matching ACPI device IDs to be used.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
arch/ia64/hp/common/sba_iommu.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
Index: linux-pm/arch/ia64/hp/common/sba_iommu.c
===================================================================
--- linux-pm.orig/arch/ia64/hp/common/sba_iommu.c
+++ linux-pm/arch/ia64/hp/common/sba_iommu.c
@@ -2042,7 +2042,8 @@ sba_map_ioc_to_node(struct ioc *ioc, acp
#endif
static int __init
-acpi_sba_ioc_add(struct acpi_device *device)
+acpi_sba_ioc_add(struct acpi_device *device,
+ const struct acpi_device_id *not_used)
{
struct ioc *ioc;
acpi_status status;
@@ -2090,14 +2091,18 @@ static const struct acpi_device_id hp_io
{"HWP0004", 0},
{"", 0},
};
-static struct acpi_driver acpi_sba_ioc_driver = {
- .name = "IOC IOMMU Driver",
- .ids = hp_ioc_iommu_device_ids,
- .ops = {
- .add = acpi_sba_ioc_add,
- },
+static struct acpi_scan_handler acpi_sba_ioc_handler = {
+ .ids = hp_ioc_iommu_device_ids,
+ .attach = acpi_sba_ioc_add,
};
+static int __init acpi_sba_ioc_init_acpi(void)
+{
+ return acpi_scan_add_handler(&acpi_sba_ioc_handler);
+}
+/* This has to run before acpi_scan_init(). */
+arch_initcall(acpi_sba_ioc_init_acpi);
+
extern struct dma_map_ops swiotlb_dma_ops;
static int __init
@@ -2122,7 +2127,10 @@ sba_init(void)
}
#endif
- acpi_bus_register_driver(&acpi_sba_ioc_driver);
+ /*
+ * ioc_list should be populated by the acpi_sba_ioc_handler's .attach()
+ * routine, but that only happens if acpi_scan_init() has already run.
+ */
if (!ioc_list) {
#ifdef CONFIG_IA64_GENERIC
/*
From: Rafael J. Wysocki <[email protected]>
Commit a33ec39 (ACPI / scan: Introduce common code for ACPI-based
device hotplug) decided to embed struct acpi_hotplug_profile into
struct acpi_scan_handler, because it looked like having a hotplug
profile without a scan handler wouldn't make sense.
While that still appears to be the case, it actually does make sense
to combine the container hotplug profile (that kind of plays the role
of a generic hotplug profile) with multiple different scan handlers.
To allow that to be done, replace the hotplug field of struct
acpi_scan_handler with a pointer and modify the code using that
field accordingly.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/acpi_memhotplug.c | 11 +++++++----
drivers/acpi/acpi_processor.c | 13 +++++++++----
drivers/acpi/container.c | 14 ++++++++------
drivers/acpi/internal.h | 5 +----
drivers/acpi/scan.c | 29 ++++++++++-------------------
drivers/acpi/sysfs.c | 10 ++++++----
include/acpi/acpi_bus.h | 3 ++-
7 files changed, 43 insertions(+), 42 deletions(-)
Index: linux-pm/drivers/acpi/internal.h
===================================================================
--- linux-pm.orig/drivers/acpi/internal.h
+++ linux-pm/drivers/acpi/internal.h
@@ -64,10 +64,7 @@ static inline void acpi_cmos_rtc_init(vo
extern bool acpi_force_hot_remove;
-void acpi_sysfs_add_hotplug_profile(struct acpi_hotplug_profile *hotplug,
- const char *name);
-int acpi_scan_add_handler_with_hotplug(struct acpi_scan_handler *handler,
- const char *hotplug_profile_name);
+void acpi_sysfs_add_hotplug_profile(struct acpi_hotplug_profile *hotplug);
void acpi_scan_hotplug_enabled(struct acpi_hotplug_profile *hotplug, bool val);
#ifdef CONFIG_DEBUG_FS
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -66,19 +66,9 @@ int acpi_scan_add_handler(struct acpi_sc
return -EINVAL;
list_add_tail(&handler->list_node, &acpi_scan_handlers_list);
- return 0;
-}
-
-int acpi_scan_add_handler_with_hotplug(struct acpi_scan_handler *handler,
- const char *hotplug_profile_name)
-{
- int error;
-
- error = acpi_scan_add_handler(handler);
- if (error)
- return error;
+ if (handler->hotplug)
+ acpi_sysfs_add_hotplug_profile(handler->hotplug);
- acpi_sysfs_add_hotplug_profile(&handler->hotplug, hotplug_profile_name);
return 0;
}
@@ -316,13 +306,13 @@ static void acpi_bus_device_eject(void *
goto err_out;
handler = device->handler;
- if (!handler || !handler->hotplug.enabled) {
+ if (!handler || !handler->hotplug || !handler->hotplug->enabled) {
ost_code = ACPI_OST_SC_EJECT_NOT_SUPPORTED;
goto err_out;
}
acpi_evaluate_hotplug_ost(handle, ACPI_NOTIFY_EJECT_REQUEST,
ACPI_OST_SC_EJECT_IN_PROGRESS, NULL);
- if (handler->hotplug.mode == AHM_CONTAINER) {
+ if (handler->hotplug->mode == AHM_CONTAINER) {
device->flags.eject_pending = true;
kobject_uevent(&device->dev.kobj, KOBJ_OFFLINE);
} else {
@@ -371,7 +361,8 @@ static void acpi_scan_bus_device_check(a
goto out;
}
ost_code = ACPI_OST_SC_SUCCESS;
- if (device->handler && device->handler->hotplug.mode == AHM_CONTAINER)
+ if (device->handler && device->handler->hotplug
+ && device->handler->hotplug->mode == AHM_CONTAINER)
kobject_uevent(&device->dev.kobj, KOBJ_ONLINE);
out:
@@ -426,7 +417,7 @@ static void acpi_hotplug_notify_cb(acpi_
struct acpi_scan_handler *handler = data;
acpi_status status;
- if (!handler->hotplug.enabled)
+ if (!handler->hotplug || !handler->hotplug->enabled)
return acpi_hotplug_unsupported(handle, type);
switch (type) {
@@ -521,8 +512,8 @@ acpi_eject_store(struct device *d, struc
if (!count || buf[0] != '1')
return -EINVAL;
- if ((!acpi_device->handler || !acpi_device->handler->hotplug.enabled)
- && !acpi_device->driver)
+ if ((!acpi_device->handler || !acpi_device->handler->hotplug
+ || !acpi_device->handler->hotplug->enabled) && !acpi_device->driver)
return -ENODEV;
status = acpi_get_type(acpi_device->handle, ¬_used);
@@ -1883,7 +1874,7 @@ static void acpi_scan_init_hotplug(acpi_
*/
list_for_each_entry(hwid, &pnp.ids, list) {
handler = acpi_scan_match_handler(hwid->id, NULL);
- if (handler) {
+ if (handler && handler->hotplug) {
acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
acpi_hotplug_notify_cb, handler);
break;
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -88,6 +88,7 @@ enum acpi_hotplug_mode {
};
struct acpi_hotplug_profile {
+ const char *name;
struct kobject kobj;
bool enabled:1;
enum acpi_hotplug_mode mode;
@@ -104,7 +105,7 @@ struct acpi_scan_handler {
struct list_head list_node;
int (*attach)(struct acpi_device *dev, const struct acpi_device_id *id);
void (*detach)(struct acpi_device *dev);
- struct acpi_hotplug_profile hotplug;
+ struct acpi_hotplug_profile *hotplug;
};
/*
Index: linux-pm/drivers/acpi/container.c
===================================================================
--- linux-pm.orig/drivers/acpi/container.c
+++ linux-pm/drivers/acpi/container.c
@@ -51,17 +51,19 @@ static int container_device_attach(struc
return 1;
}
+static struct acpi_hotplug_profile container_hotplug_profile = {
+ .name = "container",
+ .enabled = true,
+ .mode = AHM_CONTAINER,
+};
+
struct acpi_scan_handler container_scan_handler = {
.ids = container_device_ids,
.attach = container_device_attach,
- .hotplug = {
- .enabled = true,
- .mode = AHM_CONTAINER,
- },
+ .hotplug = &container_hotplug_profile,
};
void __init acpi_container_init(void)
{
- acpi_scan_add_handler_with_hotplug(&container_scan_handler,
- "container");
+ acpi_scan_add_handler(&container_scan_handler);
}
Index: linux-pm/drivers/acpi/sysfs.c
===================================================================
--- linux-pm.orig/drivers/acpi/sysfs.c
+++ linux-pm/drivers/acpi/sysfs.c
@@ -755,16 +755,18 @@ static struct kobj_type acpi_hotplug_pro
.default_attrs = hotplug_profile_attrs,
};
-void acpi_sysfs_add_hotplug_profile(struct acpi_hotplug_profile *hotplug,
- const char *name)
+void acpi_sysfs_add_hotplug_profile(struct acpi_hotplug_profile *hotplug)
{
int error;
+ if (!hotplug || !hotplug->name)
+ return;
+
if (!hotplug_kobj)
goto err_out;
kobject_init(&hotplug->kobj, &acpi_hotplug_profile_ktype);
- error = kobject_set_name(&hotplug->kobj, "%s", name);
+ error = kobject_set_name(&hotplug->kobj, "%s", hotplug->name);
if (error)
goto err_out;
@@ -777,7 +779,7 @@ void acpi_sysfs_add_hotplug_profile(stru
return;
err_out:
- pr_err(PREFIX "Unable to add hotplug profile '%s'\n", name);
+ pr_err(PREFIX "Unable to add hotplug profile '%s'\n", hotplug->name);
}
static ssize_t force_remove_show(struct kobject *kobj,
Index: linux-pm/drivers/acpi/acpi_memhotplug.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpi_memhotplug.c
+++ linux-pm/drivers/acpi/acpi_memhotplug.c
@@ -58,13 +58,16 @@ static const struct acpi_device_id memor
{"", 0},
};
+static struct acpi_hotplug_profile memory_hotplug_profile = {
+ .name = "memory",
+ .enabled = true,
+};
+
static struct acpi_scan_handler memory_device_handler = {
.ids = memory_device_ids,
.attach = acpi_memory_device_add,
.detach = acpi_memory_device_remove,
- .hotplug = {
- .enabled = true,
- },
+ .hotplug = &memory_hotplug_profile,
};
struct acpi_memory_info {
@@ -361,5 +364,5 @@ static void acpi_memory_device_remove(st
void __init acpi_memory_hotplug_init(void)
{
- acpi_scan_add_handler_with_hotplug(&memory_device_handler, "memory");
+ acpi_scan_add_handler(&memory_device_handler);
}
Index: linux-pm/drivers/acpi/acpi_processor.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpi_processor.c
+++ linux-pm/drivers/acpi/acpi_processor.c
@@ -477,18 +477,23 @@ static const struct acpi_device_id proce
{ }
};
+#ifdef CONFIG_ACPI_HOTPLUG_CPU
+static struct acpi_hotplug_profile processor_hotplug_profile = {
+ .name = "processor",
+ .enabled = true,
+};
+#endif
+
static struct acpi_scan_handler __refdata processor_handler = {
.ids = processor_device_ids,
.attach = acpi_processor_add,
#ifdef CONFIG_ACPI_HOTPLUG_CPU
.detach = acpi_processor_remove,
+ .hotplug = &processor_hotplug_profile,
#endif
- .hotplug = {
- .enabled = true,
- },
};
void __init acpi_processor_init(void)
{
- acpi_scan_add_handler_with_hotplug(&processor_handler, "processor");
+ acpi_scan_add_handler(&processor_handler);
}
From: Rafael J. Wysocki <[email protected]>
Since it generally make sense to combine different ACPI scan handlers
(that don't have their own hotplug profiles) with the container
hotplug profile, make it possible by adding a hotplug profile pointer
to struct acpi_device (so that the hotplug profile and scan handler
may be set for device objects independently of each other) and make
the generic hotplug code use the device object's hotplug profile
rather than the hotplug profile from its scan handler.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/scan.c | 24 +++++++++++++-----------
include/acpi/acpi_bus.h | 1 +
2 files changed, 14 insertions(+), 11 deletions(-)
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -296,7 +296,7 @@ static void acpi_bus_device_eject(void *
{
acpi_handle handle = context;
struct acpi_device *device = NULL;
- struct acpi_scan_handler *handler;
+ struct acpi_hotplug_profile *hotplug;
u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE;
mutex_lock(&acpi_scan_lock);
@@ -305,14 +305,14 @@ static void acpi_bus_device_eject(void *
if (!device)
goto err_out;
- handler = device->handler;
- if (!handler || !handler->hotplug || !handler->hotplug->enabled) {
+ hotplug = device->hotplug;
+ if (!hotplug || !hotplug->enabled) {
ost_code = ACPI_OST_SC_EJECT_NOT_SUPPORTED;
goto err_out;
}
acpi_evaluate_hotplug_ost(handle, ACPI_NOTIFY_EJECT_REQUEST,
ACPI_OST_SC_EJECT_IN_PROGRESS, NULL);
- if (handler->hotplug->mode == AHM_CONTAINER) {
+ if (hotplug->mode == AHM_CONTAINER) {
device->flags.eject_pending = true;
kobject_uevent(&device->dev.kobj, KOBJ_OFFLINE);
} else {
@@ -361,8 +361,7 @@ static void acpi_scan_bus_device_check(a
goto out;
}
ost_code = ACPI_OST_SC_SUCCESS;
- if (device->handler && device->handler->hotplug
- && device->handler->hotplug->mode == AHM_CONTAINER)
+ if (device->hotplug && device->hotplug->mode == AHM_CONTAINER)
kobject_uevent(&device->dev.kobj, KOBJ_ONLINE);
out:
@@ -414,10 +413,10 @@ static void acpi_hotplug_unsupported(acp
static void acpi_hotplug_notify_cb(acpi_handle handle, u32 type, void *data)
{
acpi_osd_exec_callback callback;
- struct acpi_scan_handler *handler = data;
+ struct acpi_hotplug_profile *hotplug = data;
acpi_status status;
- if (!handler->hotplug || !handler->hotplug->enabled)
+ if (!hotplug || !hotplug->enabled)
return acpi_hotplug_unsupported(handle, type);
switch (type) {
@@ -512,8 +511,8 @@ acpi_eject_store(struct device *d, struc
if (!count || buf[0] != '1')
return -EINVAL;
- if ((!acpi_device->handler || !acpi_device->handler->hotplug
- || !acpi_device->handler->hotplug->enabled) && !acpi_device->driver)
+ if ((!acpi_device->hotplug || !acpi_device->hotplug->enabled)
+ && !acpi_device->driver)
return -ENODEV;
status = acpi_get_type(acpi_device->handle, ¬_used);
@@ -1876,7 +1875,8 @@ static void acpi_scan_init_hotplug(acpi_
handler = acpi_scan_match_handler(hwid->id, NULL);
if (handler && handler->hotplug) {
acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
- acpi_hotplug_notify_cb, handler);
+ acpi_hotplug_notify_cb,
+ handler->hotplug);
break;
}
}
@@ -1948,6 +1948,7 @@ static int acpi_scan_attach_handler(stru
ret = handler->attach(device, devid);
if (ret > 0) {
device->handler = handler;
+ device->hotplug = handler->hotplug;
break;
} else if (ret < 0) {
break;
@@ -2028,6 +2029,7 @@ static acpi_status acpi_bus_device_detac
dev_handler->detach(device);
device->handler = NULL;
+ device->hotplug = NULL;
} else {
device_release_driver(&device->dev);
}
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -300,6 +300,7 @@ struct acpi_device {
struct acpi_device_perf performance;
struct acpi_device_dir dir;
struct acpi_scan_handler *handler;
+ struct acpi_hotplug_profile *hotplug;
struct acpi_driver *driver;
void *driver_data;
struct device dev;
On Thu, 2013-06-13 at 01:23 +0200, Rafael J. Wysocki wrote:
> Hi All,
>
> It turns out that some BIOSes add container device IDs as _CIDs to device
> object that in principle may be matched against the other scan handlers (or
> ACPI drivers, but that's not a problem, because the container scan handler
> can co-exist with an ACPI driver). That's why our recent fix for an issue
> related to the ACPI video driver had to be reverted right before -rc5.
I am familiar with this firmware, although I no longer have access to
the systems. An SBA device object has _HID with an HP-specific PNPID
and _CID with a generic container PNPID. The _HID allows an OS with the
HP SBA driver (which recognizes the _HID) to enable the SBA's I/O TLB
functionality, while the _CID allows an OS without the HP SBA driver to
boot-up by treating this SBA as a container. The _CID is needed because
some OS skips scanning underneath when it finds an unrecognized object.
> Although I submitted an alternative fix for that bug, I think the problem
> with the container scan handler possibly matching devices already having
> some other scan handlers attached needs addressing, because we may need to
> use the container hotplug profile for those devices. The following patch
> series is supposed to address it.
When the HP SBA driver is bound to the SBA object, this driver needs to
handle a hotplug request when it is supported. This is because the I/O
TLB functionality requires its hot-delete operation as well. The
container scan handler can be used only when this driver is bound to the
SBA object as a container and therefore its I/O TLB functionality is not
used.
> [1/5] ACPI / scan: Do not bind ACPI drivers to objects with scan handlers
> (this version shouldn't break the Tony's IA64 HP box the previous one broke)
> [2/5] ACPI / scan: Separate hotplug profiles from scan handlers
> [3/5] ACPI / scan: Add hotplug profile pointer to struct acpi_device
> [4/5] ACPI / scan: Use container hotplug profile for matching device objects
> [5/5] ACPI / ia64 / sba_iommu: Use ACPI scan handler for discovery
>
> Patches [1-4/5] were run on my Toshiba test box and didn't break it, but it
> really doesn't do any ACPI hotplug notifications.
>
> Patch [5/5] is kind of additional, but it wouldn't work correctly without the
> previous ones (to be honest, I haven't tried to compile it yet, but here it
> goes for completness).
I think we only need patch [5/5] to address the problem. We have
enhanced the match function of scan handlers to match a proper driver
with respect to their priority order, i.e. matching with _HID first and
then with _CIDs. Patch [5/5] should assure that the HP SBA driver is
bound to an SBA object when this driver is configured to the kernel.
Thanks,
-Toshi
On Thursday, June 13, 2013 03:28:59 PM Toshi Kani wrote:
> On Thu, 2013-06-13 at 01:23 +0200, Rafael J. Wysocki wrote:
> > Hi All,
> >
> > It turns out that some BIOSes add container device IDs as _CIDs to device
> > object that in principle may be matched against the other scan handlers (or
> > ACPI drivers, but that's not a problem, because the container scan handler
> > can co-exist with an ACPI driver). That's why our recent fix for an issue
> > related to the ACPI video driver had to be reverted right before -rc5.
>
> I am familiar with this firmware, although I no longer have access to
> the systems. An SBA device object has _HID with an HP-specific PNPID
> and _CID with a generic container PNPID. The _HID allows an OS with the
> HP SBA driver (which recognizes the _HID) to enable the SBA's I/O TLB
> functionality, while the _CID allows an OS without the HP SBA driver to
> boot-up by treating this SBA as a container. The _CID is needed because
> some OS skips scanning underneath when it finds an unrecognized object.
How cute.
> > Although I submitted an alternative fix for that bug, I think the problem
> > with the container scan handler possibly matching devices already having
> > some other scan handlers attached needs addressing, because we may need to
> > use the container hotplug profile for those devices. The following patch
> > series is supposed to address it.
>
> When the HP SBA driver is bound to the SBA object, this driver needs to
> handle a hotplug request when it is supported. This is because the I/O
> TLB functionality requires its hot-delete operation as well. The
> container scan handler can be used only when this driver is bound to the
> SBA object as a container and therefore its I/O TLB functionality is not
> used.
Ah, so in fact those device IDs are kind of mutually exclusive? That is,
we only should use the _CID if we don't use the _HID, right?
We have a bug there, then, but it probably is bening enough for 3.10 to be left
as is.
> > [1/5] ACPI / scan: Do not bind ACPI drivers to objects with scan handlers
> > (this version shouldn't break the Tony's IA64 HP box the previous one broke)
> > [2/5] ACPI / scan: Separate hotplug profiles from scan handlers
> > [3/5] ACPI / scan: Add hotplug profile pointer to struct acpi_device
> > [4/5] ACPI / scan: Use container hotplug profile for matching device objects
> > [5/5] ACPI / ia64 / sba_iommu: Use ACPI scan handler for discovery
> >
> > Patches [1-4/5] were run on my Toshiba test box and didn't break it, but it
> > really doesn't do any ACPI hotplug notifications.
> >
> > Patch [5/5] is kind of additional, but it wouldn't work correctly without the
> > previous ones (to be honest, I haven't tried to compile it yet, but here it
> > goes for completness).
>
> I think we only need patch [5/5] to address the problem. We have
> enhanced the match function of scan handlers to match a proper driver
> with respect to their priority order, i.e. matching with _HID first and
> then with _CIDs. Patch [5/5] should assure that the HP SBA driver is
> bound to an SBA object when this driver is configured to the kernel.
OK, but then I'd like to apply a modified version of [1/5] that won't
check if the scan handler is the container handler, but will just return
-EINVAL if any scan handler has been set already. And the changelog of
[5/5] needs to be modified slightly.
Tony promised me to test those patches on his box, so we'll know for sure
in a while.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Fri, 2013-06-14 at 00:13 +0200, Rafael J. Wysocki wrote:
> On Thursday, June 13, 2013 03:28:59 PM Toshi Kani wrote:
> > On Thu, 2013-06-13 at 01:23 +0200, Rafael J. Wysocki wrote:
> > > Hi All,
> > >
> > > It turns out that some BIOSes add container device IDs as _CIDs to device
> > > object that in principle may be matched against the other scan handlers (or
> > > ACPI drivers, but that's not a problem, because the container scan handler
> > > can co-exist with an ACPI driver). That's why our recent fix for an issue
> > > related to the ACPI video driver had to be reverted right before -rc5.
> >
> > I am familiar with this firmware, although I no longer have access to
> > the systems. An SBA device object has _HID with an HP-specific PNPID
> > and _CID with a generic container PNPID. The _HID allows an OS with the
> > HP SBA driver (which recognizes the _HID) to enable the SBA's I/O TLB
> > functionality, while the _CID allows an OS without the HP SBA driver to
> > boot-up by treating this SBA as a container. The _CID is needed because
> > some OS skips scanning underneath when it finds an unrecognized object.
>
> How cute.
>
> > > Although I submitted an alternative fix for that bug, I think the problem
> > > with the container scan handler possibly matching devices already having
> > > some other scan handlers attached needs addressing, because we may need to
> > > use the container hotplug profile for those devices. The following patch
> > > series is supposed to address it.
> >
> > When the HP SBA driver is bound to the SBA object, this driver needs to
> > handle a hotplug request when it is supported. This is because the I/O
> > TLB functionality requires its hot-delete operation as well. The
> > container scan handler can be used only when this driver is bound to the
> > SBA object as a container and therefore its I/O TLB functionality is not
> > used.
>
> Ah, so in fact those device IDs are kind of mutually exclusive? That is,
> we only should use the _CID if we don't use the _HID, right?
Yes and yes.
> We have a bug there, then, but it probably is bening enough for 3.10 to be left
> as is.
>
> > > [1/5] ACPI / scan: Do not bind ACPI drivers to objects with scan handlers
> > > (this version shouldn't break the Tony's IA64 HP box the previous one broke)
> > > [2/5] ACPI / scan: Separate hotplug profiles from scan handlers
> > > [3/5] ACPI / scan: Add hotplug profile pointer to struct acpi_device
> > > [4/5] ACPI / scan: Use container hotplug profile for matching device objects
> > > [5/5] ACPI / ia64 / sba_iommu: Use ACPI scan handler for discovery
> > >
> > > Patches [1-4/5] were run on my Toshiba test box and didn't break it, but it
> > > really doesn't do any ACPI hotplug notifications.
> > >
> > > Patch [5/5] is kind of additional, but it wouldn't work correctly without the
> > > previous ones (to be honest, I haven't tried to compile it yet, but here it
> > > goes for completness).
> >
> > I think we only need patch [5/5] to address the problem. We have
> > enhanced the match function of scan handlers to match a proper driver
> > with respect to their priority order, i.e. matching with _HID first and
> > then with _CIDs. Patch [5/5] should assure that the HP SBA driver is
> > bound to an SBA object when this driver is configured to the kernel.
>
> OK, but then I'd like to apply a modified version of [1/5] that won't
> check if the scan handler is the container handler, but will just return
> -EINVAL if any scan handler has been set already. And the changelog of
> [5/5] needs to be modified slightly.
Agreed.
> Tony promised me to test those patches on his box, so we'll know for sure
> in a while.
Cool.
Thanks,
-Toshi
>> Tony promised me to test those patches on his box, so we'll know for sure
>> in a while.
Tested this series - and the box boots just fine with no unexpected messages.
But I should note that this box doesn't have anything that is hot pluggable, so I
couldn't test hotplug (which seems to be deeply involved with things that this
patch is touching).
Of course that means that I haven't been testing hotplug - so it might have been
broken for years and I'd never have noticed.
-Tony
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Friday, June 14, 2013 06:01:41 PM Luck, Tony wrote:
> >> Tony promised me to test those patches on his box, so we'll know for sure
> >> in a while.
>
> Tested this series - and the box boots just fine with no unexpected messages.
Thanks!
> But I should note that this box doesn't have anything that is hot pluggable, so I
> couldn't test hotplug (which seems to be deeply involved with things that this
> patch is touching).
>
> Of course that means that I haven't been testing hotplug - so it might have been
> broken for years and I'd never have noticed.
Can you please just test patch [5/5] alone without patches [1-4/5]? We believe
that this should work too and if that's the case, we'll only need that patch
and a reworked [1/5].
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Fri, Jun 14, 2013 at 3:23 PM, Rafael J. Wysocki <[email protected]> wrote:
> Can you please just test patch [5/5] alone without patches [1-4/5]? We believe
> that this should work too and if that's the case, we'll only need that patch
> and a reworked [1/5].
Your belief is sound - I popped all five patches and then applied just
5/5 ... and
the system still works.
-Tony
On Friday, June 14, 2013 03:32:42 PM Tony Luck wrote:
> On Fri, Jun 14, 2013 at 3:23 PM, Rafael J. Wysocki <[email protected]> wrote:
> > Can you please just test patch [5/5] alone without patches [1-4/5]? We believe
> > that this should work too and if that's the case, we'll only need that patch
> > and a reworked [1/5].
>
> Your belief is sound - I popped all five patches and then applied just
> 5/5 ... and
> the system still works.
Great, thanks!
Can you please apply the appended patch on top of it and see if the system
still works then?
Rafael
---
From: Rafael J. Wysocki <[email protected]>
Subject: ACPI / scan: Do not bind ACPI drivers to objects with scan handlers
ACPI drivers must not be bound to device objects having scan handlers
attatched to them, so make acpi_device_probe() fail with -EINVAL if the
device object being probed has an ACPI scan handler.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/scan.c | 3 +++
drivers/acpi/video.c | 3 ---
2 files changed, 3 insertions(+), 3 deletions(-)
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -939,6 +939,9 @@ static int acpi_device_probe(struct devi
struct acpi_driver *acpi_drv = to_acpi_driver(dev->driver);
int ret;
+ if (acpi_dev->handler)
+ return -EINVAL;
+
if (!acpi_drv->ops.add)
return -ENOSYS;
Index: linux-pm/drivers/acpi/video.c
===================================================================
--- linux-pm.orig/drivers/acpi/video.c
+++ linux-pm/drivers/acpi/video.c
@@ -1722,9 +1722,6 @@ static int acpi_video_bus_add(struct acp
int error;
acpi_status status;
- if (device->handler)
- return -EINVAL;
-
status = acpi_walk_namespace(ACPI_TYPE_DEVICE,
device->parent->handle, 1,
acpi_video_bus_match, NULL,
> Can you please apply the appended patch on top of it and see if the system
> still works then?
Still works with this patch.
-Tony
> ---
> drivers/acpi/scan.c | 3 +++
> drivers/acpi/video.c | 3 ---
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> Index: linux-pm/drivers/acpi/scan.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/scan.c
> +++ linux-pm/drivers/acpi/scan.c
> @@ -939,6 +939,9 @@ static int acpi_device_probe(struct devi
> struct acpi_driver *acpi_drv = to_acpi_driver(dev->driver);
> int ret;
>
> + if (acpi_dev->handler)
> + return -EINVAL;
> +
> if (!acpi_drv->ops.add)
> return -ENOSYS;
>
> Index: linux-pm/drivers/acpi/video.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/video.c
> +++ linux-pm/drivers/acpi/video.c
> @@ -1722,9 +1722,6 @@ static int acpi_video_bus_add(struct acp
> int error;
> acpi_status status;
>
> - if (device->handler)
> - return -EINVAL;
> -
> status = acpi_walk_namespace(ACPI_TYPE_DEVICE,
> device->parent->handle, 1,
> acpi_video_bus_match, NULL,
>
On Wednesday, June 19, 2013 10:37:27 AM Tony Luck wrote:
> > Can you please apply the appended patch on top of it and see if the system
> > still works then?
>
> Still works with this patch.
Cool, thanks! :-)
If you don't mind, I'll queue up https://patchwork.kernel.org/patch/2712741/ and
this for 3.11.
Thanks,
Rafael
> > ---
> > drivers/acpi/scan.c | 3 +++
> > drivers/acpi/video.c | 3 ---
> > 2 files changed, 3 insertions(+), 3 deletions(-)
> >
> > Index: linux-pm/drivers/acpi/scan.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/scan.c
> > +++ linux-pm/drivers/acpi/scan.c
> > @@ -939,6 +939,9 @@ static int acpi_device_probe(struct devi
> > struct acpi_driver *acpi_drv = to_acpi_driver(dev->driver);
> > int ret;
> >
> > + if (acpi_dev->handler)
> > + return -EINVAL;
> > +
> > if (!acpi_drv->ops.add)
> > return -ENOSYS;
> >
> > Index: linux-pm/drivers/acpi/video.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/video.c
> > +++ linux-pm/drivers/acpi/video.c
> > @@ -1722,9 +1722,6 @@ static int acpi_video_bus_add(struct acp
> > int error;
> > acpi_status status;
> >
> > - if (device->handler)
> > - return -EINVAL;
> > -
> > status = acpi_walk_namespace(ACPI_TYPE_DEVICE,
> > device->parent->handle, 1,
> > acpi_video_bus_match, NULL,
> >
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
> If you don't mind, I'll queue up https://patchwork.kernel.org/patch/2712741/ and
> this for 3.11.
Mark them
Tested-by: Tony Luck <[email protected]>
if you like.
-Tony
On Wednesday, June 19, 2013 03:40:21 PM Tony Luck wrote:
> > If you don't mind, I'll queue up https://patchwork.kernel.org/patch/2712741/ and
> > this for 3.11.
>
> Mark them
>
> Tested-by: Tony Luck <[email protected]>
>
> if you like.
I will, thank you!
Rafael
On Thu, 2013-06-20 at 00:24 +0200, Rafael J. Wysocki wrote:
> On Wednesday, June 19, 2013 10:37:27 AM Tony Luck wrote:
> > > Can you please apply the appended patch on top of it and see if the system
> > > still works then?
> >
> > Still works with this patch.
>
> Cool, thanks! :-)
>
> If you don't mind, I'll queue up https://patchwork.kernel.org/patch/2712741/ and
> this for 3.11.
For both patches:
Acked-by: Toshi Kani <[email protected]>
Thanks,
-Toshi
>
> Thanks,
> Rafael
>
>
> > > ---
> > > drivers/acpi/scan.c | 3 +++
> > > drivers/acpi/video.c | 3 ---
> > > 2 files changed, 3 insertions(+), 3 deletions(-)
> > >
> > > Index: linux-pm/drivers/acpi/scan.c
> > > ===================================================================
> > > --- linux-pm.orig/drivers/acpi/scan.c
> > > +++ linux-pm/drivers/acpi/scan.c
> > > @@ -939,6 +939,9 @@ static int acpi_device_probe(struct devi
> > > struct acpi_driver *acpi_drv = to_acpi_driver(dev->driver);
> > > int ret;
> > >
> > > + if (acpi_dev->handler)
> > > + return -EINVAL;
> > > +
> > > if (!acpi_drv->ops.add)
> > > return -ENOSYS;
> > >
> > > Index: linux-pm/drivers/acpi/video.c
> > > ===================================================================
> > > --- linux-pm.orig/drivers/acpi/video.c
> > > +++ linux-pm/drivers/acpi/video.c
> > > @@ -1722,9 +1722,6 @@ static int acpi_video_bus_add(struct acp
> > > int error;
> > > acpi_status status;
> > >
> > > - if (device->handler)
> > > - return -EINVAL;
> > > -
> > > status = acpi_walk_namespace(ACPI_TYPE_DEVICE,
> > > device->parent->handle, 1,
> > > acpi_video_bus_match, NULL,
> > >
On Wednesday, June 19, 2013 05:22:04 PM Toshi Kani wrote:
> On Thu, 2013-06-20 at 00:24 +0200, Rafael J. Wysocki wrote:
> > On Wednesday, June 19, 2013 10:37:27 AM Tony Luck wrote:
> > > > Can you please apply the appended patch on top of it and see if the system
> > > > still works then?
> > >
> > > Still works with this patch.
> >
> > Cool, thanks! :-)
> >
> > If you don't mind, I'll queue up https://patchwork.kernel.org/patch/2712741/ and
> > this for 3.11.
>
> For both patches:
>
> Acked-by: Toshi Kani <[email protected]>
Thanks!