Now acpi_pci_root_driver has two ops: .add and .start, aka acpi_pci_root_add
and acpi_pci_root_start.
That is for hotplug handling: .add need to return early to make sure all
acpi device could be created and added early. So .start could device_add
pci device that are found in acpi_pci_root_add/pci_acpi_scan_root().
That is holding pci devics to be out of devices for while.
We could use bus notifier to handle hotplug case.
CONFIG_HOTPLUG is enabled always now.
Need to add drivers_autoprobe bit in acpi_device to hold attaching drivers
for acpi_devices, so could make sure all acpi_devices get created at first.
Then acpi_bus_attach() that is called from acpi_bus_add will attach driver
for all acpi_devices that are just created.
That make the logic more simple: hotplug path handling just like booting path
that drivers are attached after all acpi device get created.
At last we could remove all acpi_bus_start workaround.
Thanks
Yinghai
Yinghai Lu (4):
ACPI: add drivers_autoprobe in struct acpi_device
ACPI: use device drivers_autoprobe to delay loading acpi drivers
PCI, ACPI: Remove not used acpi_pci_root_start()
ACPI: remove acpi_op_start workaround
drivers/acpi/pci_root.c | 27 +++------
drivers/acpi/scan.c | 145 ++++++++++++++++++++++-------------------------
include/acpi/acpi_bus.h | 9 +---
3 files changed, 77 insertions(+), 104 deletions(-)
--
1.7.7
To use to control the delay attach driver for acpi_device.
Will use bus notifier to toggle this bits when needed.
Signed-off-by: Yinghai Lu <[email protected]>
---
drivers/acpi/scan.c | 8 +++++++-
include/acpi/acpi_bus.h | 1 +
2 files changed, 8 insertions(+), 1 deletions(-)
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index d1ecca2..cbb3ed1 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -333,7 +333,12 @@ static void acpi_device_release(struct device *dev)
static int acpi_bus_match(struct device *dev, struct device_driver *drv)
{
struct acpi_device *acpi_dev = to_acpi_device(dev);
- struct acpi_driver *acpi_drv = to_acpi_driver(drv);
+ struct acpi_driver *acpi_drv;
+
+ if (!acpi_dev->drivers_autoprobe)
+ return 0;
+
+ acpi_drv = to_acpi_driver(drv);
return !acpi_match_device_ids(acpi_dev, acpi_drv->ids);
}
@@ -1268,6 +1273,7 @@ static int acpi_add_single_object(struct acpi_device **child,
device->parent = acpi_bus_get_parent(handle);
device->bus_ops = *ops; /* workround for not call .start */
STRUCT_TO_INT(device->status) = sta;
+ device->drivers_autoprobe = true;
acpi_device_get_busid(device);
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index bde976e..969544e 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -304,6 +304,7 @@ struct acpi_device {
struct device dev;
struct acpi_bus_ops bus_ops; /* workaround for different code path for hotplug */
enum acpi_bus_removal_type removal_type; /* indicate for different removal type */
+ bool drivers_autoprobe;
};
static inline void *acpi_driver_data(struct acpi_device *d)
--
1.7.7
Current acpi driver for pci_root is working like this way for hotplug:
acpi try to enumberate acpi device and create acpi_device for pci_root
and loading driver for it, and that drivers .add aka acpi_pci_root_add
calls pci_acpi_scan_root to enumerate pci devices but not add those pci
devices into device tree to prevent drivers for pci devices get probed.
Later .start aka acpi_pci_root_start will call pci_bus_add_devices to
add pci devices into the tree to make drivers for pci devices get
attached or probed.
The reason for that .add must get back for pci_root, so could create acpi_device
for other pci_devices. otherwise adding the pci device tree early than
acpi_device will cause binding for acpi/pci failing becuse pci_device can not
find acpi_dev that is not created yet.
booting path is working becasue driver for acpi driver is registered later,
that means all acpi_device get created at first, and later when acpi_driver
get registered, and .add get called, that probe pci devices, when pci devices
is found, it could find acpi_device and binding will be ok, even
pci_add_bus_devices in done in acpi_pci_root_add.
That .start design is broken, and it will leave pci devices out of device tree
for a while.
We could use device drivers_autoprobe and acpi_bus_type notifier to control
the process to make sure for hot adding path, will have all acpi_device get
created, then attach acpi driver for acpi_device for pci_root.
That will make the path more like booting path.
After that we could remove the workaround .start in acpi driver ops.
Signed-off-by: Yinghai Lu <[email protected]>
---
drivers/acpi/scan.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 45 insertions(+), 1 deletions(-)
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index cbb3ed1..1bafa2d 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1474,6 +1474,19 @@ static int acpi_bus_scan(acpi_handle handle, struct acpi_bus_ops *ops,
return -ENODEV;
}
+static void acpi_bus_attach(struct acpi_device *dev)
+{
+ struct acpi_device *child;
+ int ret;
+
+ dev->drivers_autoprobe = true;
+ ret = device_attach(&dev->dev);
+ WARN_ON(ret < 0);
+
+ list_for_each_entry(child, &dev->children, node)
+ acpi_bus_attach(child);
+}
+
/*
* acpi_bus_add and acpi_bus_start
*
@@ -1491,11 +1504,17 @@ acpi_bus_add(struct acpi_device **child,
struct acpi_device *parent, acpi_handle handle, int type)
{
struct acpi_bus_ops ops;
+ int result;
memset(&ops, 0, sizeof(ops));
ops.acpi_op_add = 1;
- return acpi_bus_scan(handle, &ops, child);
+ result = acpi_bus_scan(handle, &ops, child);
+
+ if (*child)
+ acpi_bus_attach(*child);
+
+ return result;
}
EXPORT_SYMBOL(acpi_bus_add);
@@ -1636,3 +1655,28 @@ int __init acpi_scan_init(void)
return result;
}
+
+static int acpi_hp_notifier(struct notifier_block *nb,
+ unsigned long event, void *data)
+{
+ struct device *dev = data;
+
+ switch (event) {
+ case BUS_NOTIFY_ADD_DEVICE:
+ to_acpi_device(dev)->drivers_autoprobe = false;
+ break;
+ }
+
+ return NOTIFY_OK;
+}
+
+static struct notifier_block acpi_hp_nb = {
+ .notifier_call = &acpi_hp_notifier,
+};
+
+static int __init acpi_hp_init(void)
+{
+ return bus_register_notifier(&acpi_bus_type, &acpi_hp_nb);
+}
+
+fs_initcall(acpi_hp_init);
--
1.7.7
Now with new acpi_bus_add, we could move code in acpi_pci_root_start into
acpi_pci_root_add.
After that we could remove acpi_pci_root_start.
Signed-off-by: Yinghai Lu <[email protected]>
---
drivers/acpi/pci_root.c | 27 +++++++++------------------
1 files changed, 9 insertions(+), 18 deletions(-)
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index bce469c..8d85287 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -47,7 +47,6 @@ ACPI_MODULE_NAME("pci_root");
#define ACPI_PCI_ROOT_DEVICE_NAME "PCI Root Bridge"
static int acpi_pci_root_add(struct acpi_device *device);
static int acpi_pci_root_remove(struct acpi_device *device, int type);
-static int acpi_pci_root_start(struct acpi_device *device);
#define ACPI_PCIE_REQ_SUPPORT (OSC_EXT_PCI_CONFIG_SUPPORT \
| OSC_ACTIVE_STATE_PWR_SUPPORT \
@@ -67,7 +66,6 @@ static struct acpi_driver acpi_pci_root_driver = {
.ops = {
.add = acpi_pci_root_add,
.remove = acpi_pci_root_remove,
- .start = acpi_pci_root_start,
},
};
@@ -451,6 +449,7 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
acpi_status status;
int result;
struct acpi_pci_root *root;
+ struct acpi_pci_driver *driver;
acpi_handle handle;
struct acpi_device *child;
u32 flags, base_flags;
@@ -628,22 +627,6 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
if (device->wakeup.flags.run_wake)
device_set_run_wake(root->bus->bridge, true);
- return 0;
-
-out_del_root:
- mutex_lock(&acpi_pci_root_lock);
- list_del(&root->node);
- mutex_unlock(&acpi_pci_root_lock);
-end:
- kfree(root);
- return result;
-}
-
-static int acpi_pci_root_start(struct acpi_device *device)
-{
- struct acpi_pci_root *root = acpi_driver_data(device);
- struct acpi_pci_driver *driver;
-
mutex_lock(&acpi_pci_root_lock);
list_for_each_entry(driver, &acpi_pci_drivers, node)
if (driver->add)
@@ -653,6 +636,14 @@ static int acpi_pci_root_start(struct acpi_device *device)
pci_bus_add_devices(root->bus);
return 0;
+
+out_del_root:
+ mutex_lock(&acpi_pci_root_lock);
+ list_del(&root->node);
+ mutex_unlock(&acpi_pci_root_lock);
+end:
+ kfree(root);
+ return result;
}
static int acpi_pci_root_remove(struct acpi_device *device, int type)
--
1.7.7
No .start on any acpi_driver ops anymore.
Signed-off-by: Yinghai Lu <[email protected]>
---
drivers/acpi/scan.c | 95 ++++++++--------------------------------------
include/acpi/acpi_bus.h | 8 ----
2 files changed, 17 insertions(+), 86 deletions(-)
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 1bafa2d..5e6d2ad 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -416,7 +416,6 @@ static void acpi_device_remove_notify_handler(struct acpi_device *device)
}
static int acpi_bus_driver_init(struct acpi_device *, struct acpi_driver *);
-static int acpi_start_single_object(struct acpi_device *);
static int acpi_device_probe(struct device * dev)
{
struct acpi_device *acpi_dev = to_acpi_device(dev);
@@ -425,9 +424,6 @@ static int acpi_device_probe(struct device * dev)
ret = acpi_bus_driver_init(acpi_dev, acpi_drv);
if (!ret) {
- if (acpi_dev->bus_ops.acpi_op_start)
- acpi_start_single_object(acpi_dev);
-
if (acpi_drv->ops.notify) {
ret = acpi_device_install_notify_handler(acpi_dev);
if (ret) {
@@ -604,24 +600,6 @@ acpi_bus_driver_init(struct acpi_device *device, struct acpi_driver *driver)
return 0;
}
-static int acpi_start_single_object(struct acpi_device *device)
-{
- int result = 0;
- struct acpi_driver *driver;
-
-
- if (!(driver = device->driver))
- return 0;
-
- if (driver->ops.start) {
- result = driver->ops.start(device);
- if (result && driver->ops.remove)
- driver->ops.remove(device, ACPI_BUS_REMOVAL_NORMAL);
- }
-
- return result;
-}
-
/**
* acpi_bus_register_driver - register a driver with the ACPI bus
* @driver: driver being registered
@@ -1254,8 +1232,7 @@ static int acpi_bus_remove(struct acpi_device *dev, int rmdevice)
static int acpi_add_single_object(struct acpi_device **child,
acpi_handle handle, int type,
- unsigned long long sta,
- struct acpi_bus_ops *ops)
+ unsigned long long sta)
{
int result;
struct acpi_device *device;
@@ -1271,7 +1248,6 @@ static int acpi_add_single_object(struct acpi_device **child,
device->device_type = type;
device->handle = handle;
device->parent = acpi_bus_get_parent(handle);
- device->bus_ops = *ops; /* workround for not call .start */
STRUCT_TO_INT(device->status) = sta;
device->drivers_autoprobe = true;
@@ -1354,16 +1330,12 @@ end:
static void acpi_bus_add_power_resource(acpi_handle handle)
{
- struct acpi_bus_ops ops = {
- .acpi_op_add = 1,
- .acpi_op_start = 1,
- };
struct acpi_device *device = NULL;
acpi_bus_get_device(handle, &device);
if (!device)
acpi_add_single_object(&device, handle, ACPI_BUS_TYPE_POWER,
- ACPI_STA_DEFAULT, &ops);
+ ACPI_STA_DEFAULT);
}
static int acpi_bus_type_and_status(acpi_handle handle, int *type,
@@ -1408,7 +1380,6 @@ static int acpi_bus_type_and_status(acpi_handle handle, int *type,
static acpi_status acpi_bus_check_add(acpi_handle handle, u32 lvl,
void *context, void **return_value)
{
- struct acpi_bus_ops *ops = context;
int type;
unsigned long long sta;
struct acpi_device *device;
@@ -1433,37 +1404,30 @@ static acpi_status acpi_bus_check_add(acpi_handle handle, u32 lvl,
/*
* We may already have an acpi_device from a previous enumeration. If
- * so, we needn't add it again, but we may still have to start it.
+ * so, we needn't add it again.
*/
device = NULL;
acpi_bus_get_device(handle, &device);
- if (ops->acpi_op_add && !device)
- acpi_add_single_object(&device, handle, type, sta, ops);
+ if (!device)
+ acpi_add_single_object(&device, handle, type, sta);
if (!device)
return AE_CTRL_DEPTH;
- if (ops->acpi_op_start && !(ops->acpi_op_add)) {
- status = acpi_start_single_object(device);
- if (ACPI_FAILURE(status))
- return AE_CTRL_DEPTH;
- }
-
if (!*return_value)
*return_value = device;
return AE_OK;
}
-static int acpi_bus_scan(acpi_handle handle, struct acpi_bus_ops *ops,
- struct acpi_device **child)
+static int acpi_bus_scan(acpi_handle handle, struct acpi_device **child)
{
acpi_status status;
void *device = NULL;
- status = acpi_bus_check_add(handle, 0, ops, &device);
+ status = acpi_bus_check_add(handle, 0, NULL, &device);
if (ACPI_SUCCESS(status))
acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
- acpi_bus_check_add, NULL, ops, &device);
+ acpi_bus_check_add, NULL, NULL, &device);
if (child)
*child = device;
@@ -1488,28 +1452,24 @@ static void acpi_bus_attach(struct acpi_device *dev)
}
/*
- * acpi_bus_add and acpi_bus_start
+ * acpi_bus_add
*
* scan a given ACPI tree and (probably recently hot-plugged)
- * create and add or starts found devices.
+ * create and add found devices.
*
* If no devices were found -ENODEV is returned which does not
* mean that this is a real error, there just have been no suitable
* ACPI objects in the table trunk from which the kernel could create
- * a device and add/start an appropriate driver.
+ * a device and add an appropriate driver.
*/
int
acpi_bus_add(struct acpi_device **child,
struct acpi_device *parent, acpi_handle handle, int type)
{
- struct acpi_bus_ops ops;
int result;
- memset(&ops, 0, sizeof(ops));
- ops.acpi_op_add = 1;
-
- result = acpi_bus_scan(handle, &ops, child);
+ result = acpi_bus_scan(handle, child);
if (*child)
acpi_bus_attach(*child);
@@ -1520,20 +1480,12 @@ EXPORT_SYMBOL(acpi_bus_add);
int acpi_bus_start(struct acpi_device *device)
{
- struct acpi_bus_ops ops;
- int result;
-
if (!device)
return -EINVAL;
- memset(&ops, 0, sizeof(ops));
- ops.acpi_op_start = 1;
-
- result = acpi_bus_scan(device->handle, &ops, NULL);
-
acpi_update_all_gpes();
- return result;
+ return 0;
}
EXPORT_SYMBOL(acpi_bus_start);
@@ -1596,11 +1548,6 @@ static int acpi_bus_scan_fixed(void)
{
int result = 0;
struct acpi_device *device = NULL;
- struct acpi_bus_ops ops;
-
- memset(&ops, 0, sizeof(ops));
- ops.acpi_op_add = 1;
- ops.acpi_op_start = 1;
/*
* Enumerate all fixed-feature devices.
@@ -1608,17 +1555,14 @@ static int acpi_bus_scan_fixed(void)
if ((acpi_gbl_FADT.flags & ACPI_FADT_POWER_BUTTON) == 0) {
result = acpi_add_single_object(&device, NULL,
ACPI_BUS_TYPE_POWER_BUTTON,
- ACPI_STA_DEFAULT,
- &ops);
+ ACPI_STA_DEFAULT);
device_init_wakeup(&device->dev, true);
}
- if ((acpi_gbl_FADT.flags & ACPI_FADT_SLEEP_BUTTON) == 0) {
+ if ((acpi_gbl_FADT.flags & ACPI_FADT_SLEEP_BUTTON) == 0)
result = acpi_add_single_object(&device, NULL,
ACPI_BUS_TYPE_SLEEP_BUTTON,
- ACPI_STA_DEFAULT,
- &ops);
- }
+ ACPI_STA_DEFAULT);
return result;
}
@@ -1626,11 +1570,6 @@ static int acpi_bus_scan_fixed(void)
int __init acpi_scan_init(void)
{
int result;
- struct acpi_bus_ops ops;
-
- memset(&ops, 0, sizeof(ops));
- ops.acpi_op_add = 1;
- ops.acpi_op_start = 1;
result = bus_register(&acpi_bus_type);
if (result) {
@@ -1643,7 +1582,7 @@ int __init acpi_scan_init(void)
/*
* Enumerate devices in the ACPI namespace.
*/
- result = acpi_bus_scan(ACPI_ROOT_OBJECT, &ops, &acpi_root);
+ result = acpi_bus_scan(ACPI_ROOT_OBJECT, &acpi_root);
if (!result)
result = acpi_bus_scan_fixed();
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 969544e..cc9363b 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -119,20 +119,13 @@ struct acpi_device;
typedef int (*acpi_op_add) (struct acpi_device * device);
typedef int (*acpi_op_remove) (struct acpi_device * device, int type);
-typedef int (*acpi_op_start) (struct acpi_device * device);
typedef int (*acpi_op_bind) (struct acpi_device * device);
typedef int (*acpi_op_unbind) (struct acpi_device * device);
typedef void (*acpi_op_notify) (struct acpi_device * device, u32 event);
-struct acpi_bus_ops {
- u32 acpi_op_add:1;
- u32 acpi_op_start:1;
-};
-
struct acpi_device_ops {
acpi_op_add add;
acpi_op_remove remove;
- acpi_op_start start;
acpi_op_bind bind;
acpi_op_unbind unbind;
acpi_op_notify notify;
@@ -302,7 +295,6 @@ struct acpi_device {
struct acpi_driver *driver;
void *driver_data;
struct device dev;
- struct acpi_bus_ops bus_ops; /* workaround for different code path for hotplug */
enum acpi_bus_removal_type removal_type; /* indicate for different removal type */
bool drivers_autoprobe;
};
--
1.7.7
On Wed, Oct 3, 2012 at 7:00 PM, Yinghai Lu <[email protected]> wrote:
> No .start on any acpi_driver ops anymore.
Could you include the git commit number (of the one that removed it)
and a little description of why it was removed please?
>
> Signed-off-by: Yinghai Lu <[email protected]>
> ---
> drivers/acpi/scan.c | 95 ++++++++--------------------------------------
> include/acpi/acpi_bus.h | 8 ----
> 2 files changed, 17 insertions(+), 86 deletions(-)
>
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 1bafa2d..5e6d2ad 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -416,7 +416,6 @@ static void acpi_device_remove_notify_handler(struct acpi_device *device)
> }
>
> static int acpi_bus_driver_init(struct acpi_device *, struct acpi_driver *);
> -static int acpi_start_single_object(struct acpi_device *);
> static int acpi_device_probe(struct device * dev)
> {
> struct acpi_device *acpi_dev = to_acpi_device(dev);
> @@ -425,9 +424,6 @@ static int acpi_device_probe(struct device * dev)
>
> ret = acpi_bus_driver_init(acpi_dev, acpi_drv);
> if (!ret) {
> - if (acpi_dev->bus_ops.acpi_op_start)
> - acpi_start_single_object(acpi_dev);
> -
> if (acpi_drv->ops.notify) {
> ret = acpi_device_install_notify_handler(acpi_dev);
> if (ret) {
> @@ -604,24 +600,6 @@ acpi_bus_driver_init(struct acpi_device *device, struct acpi_driver *driver)
> return 0;
> }
>
> -static int acpi_start_single_object(struct acpi_device *device)
> -{
> - int result = 0;
> - struct acpi_driver *driver;
> -
> -
> - if (!(driver = device->driver))
> - return 0;
> -
> - if (driver->ops.start) {
> - result = driver->ops.start(device);
> - if (result && driver->ops.remove)
> - driver->ops.remove(device, ACPI_BUS_REMOVAL_NORMAL);
> - }
> -
> - return result;
> -}
> -
> /**
> * acpi_bus_register_driver - register a driver with the ACPI bus
> * @driver: driver being registered
> @@ -1254,8 +1232,7 @@ static int acpi_bus_remove(struct acpi_device *dev, int rmdevice)
>
> static int acpi_add_single_object(struct acpi_device **child,
> acpi_handle handle, int type,
> - unsigned long long sta,
> - struct acpi_bus_ops *ops)
> + unsigned long long sta)
> {
> int result;
> struct acpi_device *device;
> @@ -1271,7 +1248,6 @@ static int acpi_add_single_object(struct acpi_device **child,
> device->device_type = type;
> device->handle = handle;
> device->parent = acpi_bus_get_parent(handle);
> - device->bus_ops = *ops; /* workround for not call .start */
> STRUCT_TO_INT(device->status) = sta;
> device->drivers_autoprobe = true;
>
> @@ -1354,16 +1330,12 @@ end:
>
> static void acpi_bus_add_power_resource(acpi_handle handle)
> {
> - struct acpi_bus_ops ops = {
> - .acpi_op_add = 1,
> - .acpi_op_start = 1,
> - };
> struct acpi_device *device = NULL;
>
> acpi_bus_get_device(handle, &device);
> if (!device)
> acpi_add_single_object(&device, handle, ACPI_BUS_TYPE_POWER,
> - ACPI_STA_DEFAULT, &ops);
> + ACPI_STA_DEFAULT);
> }
>
> static int acpi_bus_type_and_status(acpi_handle handle, int *type,
> @@ -1408,7 +1380,6 @@ static int acpi_bus_type_and_status(acpi_handle handle, int *type,
> static acpi_status acpi_bus_check_add(acpi_handle handle, u32 lvl,
> void *context, void **return_value)
> {
> - struct acpi_bus_ops *ops = context;
> int type;
> unsigned long long sta;
> struct acpi_device *device;
> @@ -1433,37 +1404,30 @@ static acpi_status acpi_bus_check_add(acpi_handle handle, u32 lvl,
>
> /*
> * We may already have an acpi_device from a previous enumeration. If
> - * so, we needn't add it again, but we may still have to start it.
> + * so, we needn't add it again.
> */
> device = NULL;
> acpi_bus_get_device(handle, &device);
> - if (ops->acpi_op_add && !device)
> - acpi_add_single_object(&device, handle, type, sta, ops);
> + if (!device)
> + acpi_add_single_object(&device, handle, type, sta);
>
> if (!device)
> return AE_CTRL_DEPTH;
>
> - if (ops->acpi_op_start && !(ops->acpi_op_add)) {
> - status = acpi_start_single_object(device);
> - if (ACPI_FAILURE(status))
> - return AE_CTRL_DEPTH;
> - }
> -
> if (!*return_value)
> *return_value = device;
> return AE_OK;
> }
>
> -static int acpi_bus_scan(acpi_handle handle, struct acpi_bus_ops *ops,
> - struct acpi_device **child)
> +static int acpi_bus_scan(acpi_handle handle, struct acpi_device **child)
> {
> acpi_status status;
> void *device = NULL;
>
> - status = acpi_bus_check_add(handle, 0, ops, &device);
> + status = acpi_bus_check_add(handle, 0, NULL, &device);
> if (ACPI_SUCCESS(status))
> acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
> - acpi_bus_check_add, NULL, ops, &device);
> + acpi_bus_check_add, NULL, NULL, &device);
>
> if (child)
> *child = device;
> @@ -1488,28 +1452,24 @@ static void acpi_bus_attach(struct acpi_device *dev)
> }
>
> /*
> - * acpi_bus_add and acpi_bus_start
> + * acpi_bus_add
> *
> * scan a given ACPI tree and (probably recently hot-plugged)
> - * create and add or starts found devices.
> + * create and add found devices.
> *
> * If no devices were found -ENODEV is returned which does not
> * mean that this is a real error, there just have been no suitable
> * ACPI objects in the table trunk from which the kernel could create
> - * a device and add/start an appropriate driver.
> + * a device and add an appropriate driver.
> */
>
> int
> acpi_bus_add(struct acpi_device **child,
> struct acpi_device *parent, acpi_handle handle, int type)
> {
> - struct acpi_bus_ops ops;
> int result;
>
> - memset(&ops, 0, sizeof(ops));
> - ops.acpi_op_add = 1;
> -
> - result = acpi_bus_scan(handle, &ops, child);
> + result = acpi_bus_scan(handle, child);
>
> if (*child)
> acpi_bus_attach(*child);
> @@ -1520,20 +1480,12 @@ EXPORT_SYMBOL(acpi_bus_add);
>
> int acpi_bus_start(struct acpi_device *device)
> {
> - struct acpi_bus_ops ops;
> - int result;
> -
> if (!device)
> return -EINVAL;
>
> - memset(&ops, 0, sizeof(ops));
> - ops.acpi_op_start = 1;
> -
> - result = acpi_bus_scan(device->handle, &ops, NULL);
> -
> acpi_update_all_gpes();
>
> - return result;
> + return 0;
> }
> EXPORT_SYMBOL(acpi_bus_start);
>
> @@ -1596,11 +1548,6 @@ static int acpi_bus_scan_fixed(void)
> {
> int result = 0;
> struct acpi_device *device = NULL;
> - struct acpi_bus_ops ops;
> -
> - memset(&ops, 0, sizeof(ops));
> - ops.acpi_op_add = 1;
> - ops.acpi_op_start = 1;
>
> /*
> * Enumerate all fixed-feature devices.
> @@ -1608,17 +1555,14 @@ static int acpi_bus_scan_fixed(void)
> if ((acpi_gbl_FADT.flags & ACPI_FADT_POWER_BUTTON) == 0) {
> result = acpi_add_single_object(&device, NULL,
> ACPI_BUS_TYPE_POWER_BUTTON,
> - ACPI_STA_DEFAULT,
> - &ops);
> + ACPI_STA_DEFAULT);
> device_init_wakeup(&device->dev, true);
> }
>
> - if ((acpi_gbl_FADT.flags & ACPI_FADT_SLEEP_BUTTON) == 0) {
> + if ((acpi_gbl_FADT.flags & ACPI_FADT_SLEEP_BUTTON) == 0)
> result = acpi_add_single_object(&device, NULL,
> ACPI_BUS_TYPE_SLEEP_BUTTON,
> - ACPI_STA_DEFAULT,
> - &ops);
> - }
> + ACPI_STA_DEFAULT);
>
> return result;
> }
> @@ -1626,11 +1570,6 @@ static int acpi_bus_scan_fixed(void)
> int __init acpi_scan_init(void)
> {
> int result;
> - struct acpi_bus_ops ops;
> -
> - memset(&ops, 0, sizeof(ops));
> - ops.acpi_op_add = 1;
> - ops.acpi_op_start = 1;
>
> result = bus_register(&acpi_bus_type);
> if (result) {
> @@ -1643,7 +1582,7 @@ int __init acpi_scan_init(void)
> /*
> * Enumerate devices in the ACPI namespace.
> */
> - result = acpi_bus_scan(ACPI_ROOT_OBJECT, &ops, &acpi_root);
> + result = acpi_bus_scan(ACPI_ROOT_OBJECT, &acpi_root);
>
> if (!result)
> result = acpi_bus_scan_fixed();
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index 969544e..cc9363b 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -119,20 +119,13 @@ struct acpi_device;
>
> typedef int (*acpi_op_add) (struct acpi_device * device);
> typedef int (*acpi_op_remove) (struct acpi_device * device, int type);
> -typedef int (*acpi_op_start) (struct acpi_device * device);
> typedef int (*acpi_op_bind) (struct acpi_device * device);
> typedef int (*acpi_op_unbind) (struct acpi_device * device);
> typedef void (*acpi_op_notify) (struct acpi_device * device, u32 event);
>
> -struct acpi_bus_ops {
> - u32 acpi_op_add:1;
> - u32 acpi_op_start:1;
> -};
> -
> struct acpi_device_ops {
> acpi_op_add add;
> acpi_op_remove remove;
> - acpi_op_start start;
> acpi_op_bind bind;
> acpi_op_unbind unbind;
> acpi_op_notify notify;
> @@ -302,7 +295,6 @@ struct acpi_device {
> struct acpi_driver *driver;
> void *driver_data;
> struct device dev;
> - struct acpi_bus_ops bus_ops; /* workaround for different code path for hotplug */
> enum acpi_bus_removal_type removal_type; /* indicate for different removal type */
> bool drivers_autoprobe;
> };
> --
> 1.7.7
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
On Wed, Oct 3, 2012 at 7:00 PM, Yinghai Lu <[email protected]> wrote:
> To use to control the delay attach driver for acpi_device.
<blinks>
I am not sure what this says. Can you please explain how it controls
the delaying of
attaching drivers?
>
> Will use bus notifier to toggle this bits when needed.
Will use ..? In a subsequent patch? Which patch? And when is this
needed? Is there a patch that needs this?
>
> Signed-off-by: Yinghai Lu <[email protected]>
> ---
> drivers/acpi/scan.c | 8 +++++++-
> include/acpi/acpi_bus.h | 1 +
> 2 files changed, 8 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index d1ecca2..cbb3ed1 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -333,7 +333,12 @@ static void acpi_device_release(struct device *dev)
> static int acpi_bus_match(struct device *dev, struct device_driver *drv)
> {
> struct acpi_device *acpi_dev = to_acpi_device(dev);
> - struct acpi_driver *acpi_drv = to_acpi_driver(drv);
> + struct acpi_driver *acpi_drv;
> +
> + if (!acpi_dev->drivers_autoprobe)
> + return 0;
> +
> + acpi_drv = to_acpi_driver(drv);
>
> return !acpi_match_device_ids(acpi_dev, acpi_drv->ids);
> }
> @@ -1268,6 +1273,7 @@ static int acpi_add_single_object(struct acpi_device **child,
> device->parent = acpi_bus_get_parent(handle);
> device->bus_ops = *ops; /* workround for not call .start */
> STRUCT_TO_INT(device->status) = sta;
> + device->drivers_autoprobe = true;
>
> acpi_device_get_busid(device);
>
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index bde976e..969544e 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -304,6 +304,7 @@ struct acpi_device {
> struct device dev;
> struct acpi_bus_ops bus_ops; /* workaround for different code path for hotplug */
> enum acpi_bus_removal_type removal_type; /* indicate for different removal type */
> + bool drivers_autoprobe;
> };
>
> static inline void *acpi_driver_data(struct acpi_device *d)
> --
> 1.7.7
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
On Thu, Oct 4, 2012 at 6:03 AM, Konrad Rzeszutek Wilk <[email protected]> wrote:
> On Wed, Oct 3, 2012 at 7:00 PM, Yinghai Lu <[email protected]> wrote:
>> To use to control the delay attach driver for acpi_device.
>
> <blinks>
> I am not sure what this says. Can you please explain how it controls
> the delaying of
> attaching drivers?
>>
>> Will use bus notifier to toggle this bits when needed.
>
> Will use ..? In a subsequent patch? Which patch? And when is this
> needed? Is there a patch that needs this?
please check patch 0-4 as a whole.
Yinghai
On Wed, Oct 03, 2012 at 04:00:10PM -0700, Yinghai Lu wrote:
> Now acpi_pci_root_driver has two ops: .add and .start, aka acpi_pci_root_add
> and acpi_pci_root_start.
>
> That is for hotplug handling: .add need to return early to make sure all
> acpi device could be created and added early. So .start could device_add
> pci device that are found in acpi_pci_root_add/pci_acpi_scan_root().
>
> That is holding pci devics to be out of devices for while.
>
> We could use bus notifier to handle hotplug case.
> CONFIG_HOTPLUG is enabled always now.
> Need to add drivers_autoprobe bit in acpi_device to hold attaching drivers
> for acpi_devices, so could make sure all acpi_devices get created at first.
> Then acpi_bus_attach() that is called from acpi_bus_add will attach driver
> for all acpi_devices that are just created.
>
> That make the logic more simple: hotplug path handling just like booting path
> that drivers are attached after all acpi device get created.
>
> At last we could remove all acpi_bus_start workaround.
I think we should get rid of ACPI .start() methods, but I'm opposed to this
approach of fiddling with driver binding order.
At hot-plug time, the pci_root.c driver is already loaded, then we enumerate
the new host bridge. Since pci_root.c is already loaded, we bind the driver
before descending the ACPI tree below the host bridge. We need to make that
same ordering work at boot-time.
At boot-time, we currently enumerate ALL ACPI devices before registering
the pci_root.c driver:
acpi_init # subsys_initcall
acpi_scan_init
acpi_bus_scan # enumerate ACPI devices
acpi_pci_root_init # subsys_initcall for pci_root.c
acpi_bus_register_driver
...
acpi_pci_root_add # pci_root.c add method
This is a fundamental difference: at boot-time, all the ACPI devices below the
host bridge already exist before the pci_root.c driver claims the bridge,
while at hot-add time, pci_root.c claims the bridge *before* those ACPI
devices exist.
I think this is wrong. The hot-plug case (where the driver is already
loaded and binds to the device as soon as it's discovered, before the
ACPI hierarchy below it is enumerated) seems like the obviously correct
order. I think we should change the boot-time order to match that, i.e.,
we should register pci_root.c *before* enumerating ACPI devices.
I realize that this will require changes in the way we bind PCI and ACPI
devices. I pointed that out in a previous response, appended below for
convenience:
On Tue, Oct 2, 2012 at 4:38 PM, Bjorn Helgaas <[email protected]> wrote:
>> We enumerate ACPI devices by doing a depth-first traversal of the
>> namespace. We should be able to bind a driver to a device as soon as
>> we discover it. For PNP0A03/08 host bridges, that will be the
>> pci_root.c driver. That driver's .add() method enumerates all the PCI
>> devices behind the host bridge, which is another depth-first
>> traversal, this time of the PCI hierarchy. Similarly, we ought to be
>> able to bind drivers to the PCI devices as we discover them.
>
>> But the PCI/ACPI binding is an issue here, because the ACPI
>> enumeration hasn't descended below the host bridge yet, so we have the
>> pci_dev structs but the acpi_device structs don't exist yet. I think
>> this is part of the reason for the .add()/.start() split. But I don't
>> think the split is the correct solution. I think we need to think
>> about the binding in a different way. Maybe we don't bind at all and
>> instead search the namespace every time we need the association. Or
>> maybe we do the binding but base it on the acpi_handle (which exists
>> before the ACPI subtree has been enumerated) rather than the
>> acpi_device (which doesn't exist until we enumerate the subtree).
>> It's not even clear to me that we should build acpi_device structs for
>> things that already have pci_dev structs.
On Thu, Oct 4, 2012 at 10:47 AM, Bjorn Helgaas <[email protected]> wrote:
> On Wed, Oct 03, 2012 at 04:00:10PM -0700, Yinghai Lu wrote:
> This is a fundamental difference: at boot-time, all the ACPI devices below the
> host bridge already exist before the pci_root.c driver claims the bridge,
> while at hot-add time, pci_root.c claims the bridge *before* those ACPI
> devices exist.
>
> I think this is wrong. The hot-plug case (where the driver is already
> loaded and binds to the device as soon as it's discovered, before the
> ACPI hierarchy below it is enumerated) seems like the obviously correct
> order. I think we should change the boot-time order to match that, i.e.,
> we should register pci_root.c *before* enumerating ACPI devices.
in booting path, all device get probed at first, and then register driver...
do you want to register all pci driver before probing pci devices?
Yinghai
On Thu, Oct 4, 2012 at 12:36 PM, Yinghai Lu <[email protected]> wrote:
> On Thu, Oct 4, 2012 at 10:47 AM, Bjorn Helgaas <[email protected]> wrote:
>> On Wed, Oct 03, 2012 at 04:00:10PM -0700, Yinghai Lu wrote:
>> This is a fundamental difference: at boot-time, all the ACPI devices below the
>> host bridge already exist before the pci_root.c driver claims the bridge,
>> while at hot-add time, pci_root.c claims the bridge *before* those ACPI
>> devices exist.
>>
>> I think this is wrong. The hot-plug case (where the driver is already
>> loaded and binds to the device as soon as it's discovered, before the
>> ACPI hierarchy below it is enumerated) seems like the obviously correct
>> order. I think we should change the boot-time order to match that, i.e.,
>> we should register pci_root.c *before* enumerating ACPI devices.
>
> in booting path, all device get probed at first, and then register driver...
> do you want to register all pci driver before probing pci devices?
I don't think we should have dependencies either way. It shouldn't
matter whether we enumerate devices first or we register the driver
first. That's why I think the current PCI/ACPI binding is broken --
it assumes that we fully enumerate ACPI before the driver is
registered.
To answer your specific question, yes, I do think drivers that are
statically built in probably should be registered before devices are
enumerated. That way, the boot-time case is more similar to the
hot-add case.
Obviously, for drivers that can be modules, the reverse must work as
well (enumerate devices, then load and register the driver). And then
the other order (register driver, then enumerate device) must also
work so future hot-adds of the same device type work.
Bjorn
On Thu, Oct 4, 2012 at 12:44 PM, Bjorn Helgaas <[email protected]> wrote:
>
> To answer your specific question, yes, I do think drivers that are
> statically built in probably should be registered before devices are
> enumerated. That way, the boot-time case is more similar to the
> hot-add case.
>
> Obviously, for drivers that can be modules, the reverse must work as
> well (enumerate devices, then load and register the driver). And then
> the other order (register driver, then enumerate device) must also
> work so future hot-adds of the same device type work.
so you will have to handle two paths instead one.
current booting path sequence are tested more than hot add path.
On Thursday 04 of October 2012 13:44:42 Bjorn Helgaas wrote:
> On Thu, Oct 4, 2012 at 12:36 PM, Yinghai Lu <[email protected]> wrote:
> > On Thu, Oct 4, 2012 at 10:47 AM, Bjorn Helgaas <[email protected]> wrote:
> >> On Wed, Oct 03, 2012 at 04:00:10PM -0700, Yinghai Lu wrote:
> >> This is a fundamental difference: at boot-time, all the ACPI devices below the
> >> host bridge already exist before the pci_root.c driver claims the bridge,
> >> while at hot-add time, pci_root.c claims the bridge *before* those ACPI
> >> devices exist.
> >>
> >> I think this is wrong. The hot-plug case (where the driver is already
> >> loaded and binds to the device as soon as it's discovered, before the
> >> ACPI hierarchy below it is enumerated) seems like the obviously correct
> >> order. I think we should change the boot-time order to match that, i.e.,
> >> we should register pci_root.c *before* enumerating ACPI devices.
> >
> > in booting path, all device get probed at first, and then register driver...
> > do you want to register all pci driver before probing pci devices?
>
> I don't think we should have dependencies either way. It shouldn't
> matter whether we enumerate devices first or we register the driver
> first. That's why I think the current PCI/ACPI binding is broken --
> it assumes that we fully enumerate ACPI before the driver is
> registered.
>
> To answer your specific question, yes, I do think drivers that are
> statically built in probably should be registered before devices are
> enumerated. That way, the boot-time case is more similar to the
> hot-add case.
>
> Obviously, for drivers that can be modules, the reverse must work as
> well (enumerate devices, then load and register the driver). And then
> the other order (register driver, then enumerate device) must also
> work so future hot-adds of the same device type work.
Agreed.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Thursday 04 of October 2012 11:47:46 Bjorn Helgaas wrote:
> On Wed, Oct 03, 2012 at 04:00:10PM -0700, Yinghai Lu wrote:
> > Now acpi_pci_root_driver has two ops: .add and .start, aka acpi_pci_root_add
> > and acpi_pci_root_start.
> >
> > That is for hotplug handling: .add need to return early to make sure all
> > acpi device could be created and added early. So .start could device_add
> > pci device that are found in acpi_pci_root_add/pci_acpi_scan_root().
> >
> > That is holding pci devics to be out of devices for while.
> >
> > We could use bus notifier to handle hotplug case.
> > CONFIG_HOTPLUG is enabled always now.
> > Need to add drivers_autoprobe bit in acpi_device to hold attaching drivers
> > for acpi_devices, so could make sure all acpi_devices get created at first.
> > Then acpi_bus_attach() that is called from acpi_bus_add will attach driver
> > for all acpi_devices that are just created.
> >
> > That make the logic more simple: hotplug path handling just like booting path
> > that drivers are attached after all acpi device get created.
> >
> > At last we could remove all acpi_bus_start workaround.
>
> I think we should get rid of ACPI .start() methods, but I'm opposed to this
> approach of fiddling with driver binding order.
>
> At hot-plug time, the pci_root.c driver is already loaded, then we enumerate
> the new host bridge. Since pci_root.c is already loaded, we bind the driver
> before descending the ACPI tree below the host bridge. We need to make that
> same ordering work at boot-time.
>
> At boot-time, we currently enumerate ALL ACPI devices before registering
> the pci_root.c driver:
>
> acpi_init # subsys_initcall
> acpi_scan_init
> acpi_bus_scan # enumerate ACPI devices
>
> acpi_pci_root_init # subsys_initcall for pci_root.c
> acpi_bus_register_driver
> ...
> acpi_pci_root_add # pci_root.c add method
>
> This is a fundamental difference: at boot-time, all the ACPI devices below the
> host bridge already exist before the pci_root.c driver claims the bridge,
> while at hot-add time, pci_root.c claims the bridge *before* those ACPI
> devices exist.
>
> I think this is wrong. The hot-plug case (where the driver is already
> loaded and binds to the device as soon as it's discovered, before the
> ACPI hierarchy below it is enumerated) seems like the obviously correct
> order. I think we should change the boot-time order to match that, i.e.,
> we should register pci_root.c *before* enumerating ACPI devices.
>
> I realize that this will require changes in the way we bind PCI and ACPI
> devices. I pointed that out in a previous response, appended below for
> convenience:
>
> On Tue, Oct 2, 2012 at 4:38 PM, Bjorn Helgaas <[email protected]> wrote:
>
> >> We enumerate ACPI devices by doing a depth-first traversal of the
> >> namespace. We should be able to bind a driver to a device as soon as
> >> we discover it. For PNP0A03/08 host bridges, that will be the
> >> pci_root.c driver. That driver's .add() method enumerates all the PCI
> >> devices behind the host bridge, which is another depth-first
> >> traversal, this time of the PCI hierarchy. Similarly, we ought to be
> >> able to bind drivers to the PCI devices as we discover them.
> >
> >> But the PCI/ACPI binding is an issue here, because the ACPI
> >> enumeration hasn't descended below the host bridge yet, so we have the
> >> pci_dev structs but the acpi_device structs don't exist yet. I think
> >> this is part of the reason for the .add()/.start() split. But I don't
> >> think the split is the correct solution. I think we need to think
> >> about the binding in a different way. Maybe we don't bind at all and
> >> instead search the namespace every time we need the association. Or
> >> maybe we do the binding but base it on the acpi_handle (which exists
> >> before the ACPI subtree has been enumerated) rather than the
> >> acpi_device (which doesn't exist until we enumerate the subtree).
> >> It's not even clear to me that we should build acpi_device structs for
> >> things that already have pci_dev structs.
No, we shouldn't.
We generally have problems in this area, not only with PCI. For
example, some platform x86 drivers cannot bind to the devices they
should handle, because there's a "generic" ACPI driver that binds to
the device in question earlier.
My personal opinion is that so called "ACPI devices" (i.e. things
represented by struct acpi_device) are generally partially redundant, because
either they already have "sibling" struct device objects representing "real"
devices, like PCI, SATA or PNP, or there should be struct platform_device
"sibling" objects corresponding to them. Moreover, all ACPI drivers can be
replaced with platform drivers and the only problem is the .notify() callback
that is not present in struct platform_driver. So perhaps we can create
a "real device" for every "ACPI device" we discover (if there isn't one
already) and stop using ACPI drivers entirely, eventually.
That also may help to address the problem of hypothetical future systems
with ACPI tables describing the same hardware that we already have device
tree representation for. In those cases drivers should work regardless of
whether the information on devices is read from ACPI tables or from device
trees and quite frankly I don't see how we can achieve this with the
existing ACPI drivers.
Perhaps we don't even need the ACPI bus type and struct acpi_device
objects can be treated simply as storage of information used by the generic
ACPI power management code etc. I'm not sure how to get there yet in the
least painful way, but I think it would make things generally more
straightforward.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Thu, Oct 4, 2012 at 2:14 PM, Yinghai Lu <[email protected]> wrote:
> On Thu, Oct 4, 2012 at 12:44 PM, Bjorn Helgaas <[email protected]> wrote:
>>
>> To answer your specific question, yes, I do think drivers that are
>> statically built in probably should be registered before devices are
>> enumerated. That way, the boot-time case is more similar to the
>> hot-add case.
>>
>> Obviously, for drivers that can be modules, the reverse must work as
>> well (enumerate devices, then load and register the driver). And then
>> the other order (register driver, then enumerate device) must also
>> work so future hot-adds of the same device type work.
>
> so you will have to handle two paths instead one.
I'm not proposing any additional requirements; I'm just describing the
way Linux driver modules work. Any module *already* must support both
orders (enumerate devices then register driver, as well as register
driver then enumerate and bind to a hot-added device). And this
should not be two paths, it should be one and the same path for both
orders.
> current booting path sequence are tested more than hot add path.
True. You're proposing fiddling with driver binding order so we can
use the current boot path ordering at hot-add time. I'm suggesting
that the "register driver then enumerate device" order is something we
have to support for hot-add anyway, and that we should use the same
ordering at boot-time. That's more change for the boot-time path, but
I think it's cleaner and more maintainable in the long term.
On Wednesday 03 of October 2012 16:00:10 Yinghai Lu wrote:
> Now acpi_pci_root_driver has two ops: .add and .start, aka acpi_pci_root_add
> and acpi_pci_root_start.
>
> That is for hotplug handling: .add need to return early to make sure all
> acpi device could be created and added early. So .start could device_add
> pci device that are found in acpi_pci_root_add/pci_acpi_scan_root().
>
> That is holding pci devics to be out of devices for while.
>
> We could use bus notifier to handle hotplug case.
> CONFIG_HOTPLUG is enabled always now.
> Need to add drivers_autoprobe bit in acpi_device to hold attaching drivers
> for acpi_devices, so could make sure all acpi_devices get created at first.
> Then acpi_bus_attach() that is called from acpi_bus_add will attach driver
> for all acpi_devices that are just created.
>
> That make the logic more simple: hotplug path handling just like booting path
> that drivers are attached after all acpi device get created.
>
> At last we could remove all acpi_bus_start workaround.
Do I understand correctly that you just want to prevent acpi_pci_root_driver
from binding to the host bridge's struct acpi_device created while we're
walking the ACPI namespace?
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Thu, Oct 4, 2012 at 2:23 PM, Rafael J. Wysocki <[email protected]> wrote:
>>
>> At last we could remove all acpi_bus_start workaround.
>
> Do I understand correctly that you just want to prevent acpi_pci_root_driver
> from binding to the host bridge's struct acpi_device created while we're
> walking the ACPI namespace?
yes, during hot add path.
Yinghai
On Thursday 04 of October 2012 14:31:46 Yinghai Lu wrote:
> On Thu, Oct 4, 2012 at 2:23 PM, Rafael J. Wysocki <[email protected]> wrote:
> >>
> >> At last we could remove all acpi_bus_start workaround.
> >
> > Do I understand correctly that you just want to prevent acpi_pci_root_driver
> > from binding to the host bridge's struct acpi_device created while we're
> > walking the ACPI namespace?
>
> yes, during hot add path.
Why exactly do you want to prevent that from happening?
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Thu, Oct 4, 2012 at 2:53 PM, Rafael J. Wysocki <[email protected]> wrote:
> On Thursday 04 of October 2012 14:31:46 Yinghai Lu wrote:
>> On Thu, Oct 4, 2012 at 2:23 PM, Rafael J. Wysocki <[email protected]> wrote:
>> >>
>> >> At last we could remove all acpi_bus_start workaround.
>> >
>> > Do I understand correctly that you just want to prevent acpi_pci_root_driver
>> > from binding to the host bridge's struct acpi_device created while we're
>> > walking the ACPI namespace?
>>
>> yes, during hot add path.
>
> Why exactly do you want to prevent that from happening?
>
during adding pci root bus hotplug, Bjorn found some unsafe searching
that caused by pci_bus_add_devices.
pci devices are created during pci scan root, but until very late
acpi_pci_root_start call pci_bus_add_devices.
To fill the gap, we need to move pci_bus_add_devices to acpi_pci_root_add
at first.
but after we move that there, pci device will be added to device tree, and it
will try to bind with acpi devices that should be under acpi pci root,
but are not
created yet. because device_add for acpi_device for acpi pci root is done yet.
it still calling the .add in the acpi_driver aka acpi_pci_root_add.
So I want to hold the driver attach for pci root acpi devices, and
later attach it
until pci devices created.
booting path, all acpi devices get created, and attach driver for them
one by one.
On Thursday 04 of October 2012 15:01:21 Yinghai Lu wrote:
> On Thu, Oct 4, 2012 at 2:53 PM, Rafael J. Wysocki <[email protected]> wrote:
> > On Thursday 04 of October 2012 14:31:46 Yinghai Lu wrote:
> >> On Thu, Oct 4, 2012 at 2:23 PM, Rafael J. Wysocki <[email protected]> wrote:
> >> >>
> >> >> At last we could remove all acpi_bus_start workaround.
> >> >
> >> > Do I understand correctly that you just want to prevent acpi_pci_root_driver
> >> > from binding to the host bridge's struct acpi_device created while we're
> >> > walking the ACPI namespace?
> >>
> >> yes, during hot add path.
> >
> > Why exactly do you want to prevent that from happening?
> >
>
> during adding pci root bus hotplug, Bjorn found some unsafe searching
> that caused by pci_bus_add_devices.
Do you have a link to a description of that problem?
> pci devices are created during pci scan root, but until very late
> acpi_pci_root_start call pci_bus_add_devices.
So you mean that pci_bus_add_devices() is called too late, right?
> To fill the gap, we need to move pci_bus_add_devices to acpi_pci_root_add
> at first.
>
> but after we move that there, pci device will be added to device tree, and it
> will try to bind with acpi devices that should be under acpi pci root,
> but are not
> created yet. because device_add for acpi_device for acpi pci root is done yet.
> it still calling the .add in the acpi_driver aka acpi_pci_root_add.
Quite obviously, we haven't walked the ACPI namespace below the host bridge
object yet at that point.
> So I want to hold the driver attach for pci root acpi devices, and
> later attach it
> until pci devices created.
>
> booting path, all acpi devices get created, and attach driver for them
> one by one.
I see.
Your patches seem to affect all devices in the ACPI namespace added after
boot, though, not only host bridges.
And the problem seems to be that the scanning of the ACPI namespace and
configuring the host bridge are kind of independent operations now. What
we should do, actually, seems to be something like this:
(1) Configure the host bridge when discovered (i.e. do what the current
acpi_pci_root_add() does.
(2) Parse the ACPI namespace under the host bridge (without binding ACPI
drivers to the struct acpi_device objects created in the process,
because they are known to correspond to PCI devices).
(3) Run pci_bus_add_devices() for the bridge.
in one routine.
What do you think?
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Thu, Oct 4, 2012 at 3:36 PM, Rafael J. Wysocki <[email protected]> wrote:
> On Thursday 04 of October 2012 15:01:21 Yinghai Lu wrote:
>> during adding pci root bus hotplug, Bjorn found some unsafe searching
>> that caused by pci_bus_add_devices.
>
> Do you have a link to a description of that problem?
Maybe bjorn could expand it more.
>
>> pci devices are created during pci scan root, but until very late
>> acpi_pci_root_start call pci_bus_add_devices.
>
> So you mean that pci_bus_add_devices() is called too late, right?
yes.
>
>> To fill the gap, we need to move pci_bus_add_devices to acpi_pci_root_add
>> at first.
>>
>> but after we move that there, pci device will be added to device tree, and it
>> will try to bind with acpi devices that should be under acpi pci root,
>> but are not
>> created yet. because device_add for acpi_device for acpi pci root is done yet.
>> it still calling the .add in the acpi_driver aka acpi_pci_root_add.
>
> Quite obviously, we haven't walked the ACPI namespace below the host bridge
> object yet at that point.
yes.
>
>> So I want to hold the driver attach for pci root acpi devices, and
>> later attach it
>> until pci devices created.
>>
>> booting path, all acpi devices get created, and attach driver for them
>> one by one.
>
> I see.
>
> Your patches seem to affect all devices in the ACPI namespace added after
> boot, though, not only host bridges.
yes, but it still should be safe.
>
> And the problem seems to be that the scanning of the ACPI namespace and
> configuring the host bridge are kind of independent operations now. What
> we should do, actually, seems to be something like this:
>
> (1) Configure the host bridge when discovered (i.e. do what the current
> acpi_pci_root_add() does.
> (2) Parse the ACPI namespace under the host bridge (without binding ACPI
> drivers to the struct acpi_device objects created in the process,
> because they are known to correspond to PCI devices).
> (3) Run pci_bus_add_devices() for the bridge.
>
> in one routine.
problem is still there. if 1 still has acpi_pci_root_add and pci_acpi_scan_root
that scan pci devices. what is need is we need to bind 1 and 3 together.
or we could move pci_scan_root_scan from acpi_pci_root_add to
acpi_pci_root_start?
On Thursday 04 of October 2012 15:46:39 Yinghai Lu wrote:
> On Thu, Oct 4, 2012 at 3:36 PM, Rafael J. Wysocki <[email protected]> wrote:
> > On Thursday 04 of October 2012 15:01:21 Yinghai Lu wrote:
> >> during adding pci root bus hotplug, Bjorn found some unsafe searching
> >> that caused by pci_bus_add_devices.
> >
> > Do you have a link to a description of that problem?
>
> Maybe bjorn could expand it more.
>
> >
> >> pci devices are created during pci scan root, but until very late
> >> acpi_pci_root_start call pci_bus_add_devices.
> >
> > So you mean that pci_bus_add_devices() is called too late, right?
>
> yes.
>
> >
> >> To fill the gap, we need to move pci_bus_add_devices to acpi_pci_root_add
> >> at first.
> >>
> >> but after we move that there, pci device will be added to device tree, and it
> >> will try to bind with acpi devices that should be under acpi pci root,
> >> but are not
> >> created yet. because device_add for acpi_device for acpi pci root is done yet.
> >> it still calling the .add in the acpi_driver aka acpi_pci_root_add.
> >
> > Quite obviously, we haven't walked the ACPI namespace below the host bridge
> > object yet at that point.
>
> yes.
>
> >
> >> So I want to hold the driver attach for pci root acpi devices, and
> >> later attach it
> >> until pci devices created.
> >>
> >> booting path, all acpi devices get created, and attach driver for them
> >> one by one.
> >
> > I see.
> >
> > Your patches seem to affect all devices in the ACPI namespace added after
> > boot, though, not only host bridges.
>
> yes, but it still should be safe.
I'm not really sure of that (what about undock/dock, for exmaple?) and it's
damn ugly.
> > And the problem seems to be that the scanning of the ACPI namespace and
> > configuring the host bridge are kind of independent operations now. What
> > we should do, actually, seems to be something like this:
> >
> > (1) Configure the host bridge when discovered (i.e. do what the current
> > acpi_pci_root_add() does.
> > (2) Parse the ACPI namespace under the host bridge (without binding ACPI
> > drivers to the struct acpi_device objects created in the process,
> > because they are known to correspond to PCI devices).
> > (3) Run pci_bus_add_devices() for the bridge.
> >
> > in one routine.
>
> problem is still there. if 1 still has acpi_pci_root_add and pci_acpi_scan_root
OK, so why don't we do (2) in acpi_pci_root_add(), before pci_acpi_scan_root()
is called?
> that scan pci devices. what is need is we need to bind 1 and 3 together.
I don't understand now. You said previously that we need the ACPI namespace
below the bridge to be scanned before (3), so why do you want to do (3) before
(2) now?
> or we could move pci_scan_root_scan from acpi_pci_root_add to
> acpi_pci_root_start?
No, I don't think so, because we call acpi_pci_bind_root() after that.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Fri, Oct 5, 2012 at 4:01 PM, Rafael J. Wysocki <[email protected]> wrote:
> On Thursday 04 of October 2012 15:46:39 Yinghai Lu wrote:
>> > Your patches seem to affect all devices in the ACPI namespace added after
>> > boot, though, not only host bridges.
>>
>> yes, but it still should be safe.
>
> I'm not really sure of that (what about undock/dock, for exmaple?) and it's
> damn ugly.
only one acpi_driver has .start , that is acpi_pci_root_driver.
should be clean than with .add/start pair.
>
>> > And the problem seems to be that the scanning of the ACPI namespace and
>> > configuring the host bridge are kind of independent operations now. What
>> > we should do, actually, seems to be something like this:
>> >
>> > (1) Configure the host bridge when discovered (i.e. do what the current
>> > acpi_pci_root_add() does.
>> > (2) Parse the ACPI namespace under the host bridge (without binding ACPI
>> > drivers to the struct acpi_device objects created in the process,
>> > because they are known to correspond to PCI devices).
>> > (3) Run pci_bus_add_devices() for the bridge.
>> >
>> > in one routine.
>>
>> problem is still there. if 1 still has acpi_pci_root_add and pci_acpi_scan_root
>
> OK, so why don't we do (2) in acpi_pci_root_add(), before pci_acpi_scan_root()
> is called?
>
>> that scan pci devices. what is need is we need to bind 1 and 3 together.
some one already walk the acpi space, and during that it create
acpi_device for pci_root
and then attach driver for that, aka acpi_pci_root_add is executing.
Now you want to walking the acpi acpi space to create children devices
before device_add really done for that pci root
acpi device. ?
is that some kind of nesting?
>
> I don't understand now. You said previously that we need the ACPI namespace
> below the bridge to be scanned before (3), so why do you want to do (3) before
> (2) now?
purpose is calling pci_bus_add_devices in pci_acpi_scan_root.
Yinghai
On Friday 05 of October 2012 16:10:43 Yinghai Lu wrote:
> On Fri, Oct 5, 2012 at 4:01 PM, Rafael J. Wysocki <[email protected]> wrote:
> > On Thursday 04 of October 2012 15:46:39 Yinghai Lu wrote:
> >> > Your patches seem to affect all devices in the ACPI namespace added after
> >> > boot, though, not only host bridges.
> >>
> >> yes, but it still should be safe.
> >
> > I'm not really sure of that (what about undock/dock, for exmaple?) and it's
> > damn ugly.
>
> only one acpi_driver has .start , that is acpi_pci_root_driver.
>
> should be clean than with .add/start pair.
>
> >
> >> > And the problem seems to be that the scanning of the ACPI namespace and
> >> > configuring the host bridge are kind of independent operations now. What
> >> > we should do, actually, seems to be something like this:
> >> >
> >> > (1) Configure the host bridge when discovered (i.e. do what the current
> >> > acpi_pci_root_add() does.
> >> > (2) Parse the ACPI namespace under the host bridge (without binding ACPI
> >> > drivers to the struct acpi_device objects created in the process,
> >> > because they are known to correspond to PCI devices).
> >> > (3) Run pci_bus_add_devices() for the bridge.
> >> >
> >> > in one routine.
> >>
> >> problem is still there. if 1 still has acpi_pci_root_add and pci_acpi_scan_root
> >
> > OK, so why don't we do (2) in acpi_pci_root_add(), before pci_acpi_scan_root()
> > is called?
> >
> >> that scan pci devices. what is need is we need to bind 1 and 3 together.
>
> some one already walk the acpi space, and during that it create
> acpi_device for pci_root
> and then attach driver for that, aka acpi_pci_root_add is executing.
>
> Now you want to walking the acpi acpi space to create children devices
> before device_add really done for that pci root
> acpi device. ?
>
> is that some kind of nesting?
Yes, basically. The idea is to do the scan of the host bridge's children in
the ACPI tree synchronously within acpi_pci_root_add() instead of trying to
delay the execution of it until the children have been scanned (and using
notifiers to kind of trigger the driver binding, i.e. the execution of .add()).
> > I don't understand now. You said previously that we need the ACPI namespace
> > below the bridge to be scanned before (3), so why do you want to do (3) before
> > (2) now?
>
> purpose is calling pci_bus_add_devices in pci_acpi_scan_root.
OK, but what's the reason?
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Thu, Oct 04, 2012 at 08:15:44AM -0700, Yinghai Lu wrote:
> On Thu, Oct 4, 2012 at 6:03 AM, Konrad Rzeszutek Wilk <[email protected]> wrote:
> > On Wed, Oct 3, 2012 at 7:00 PM, Yinghai Lu <[email protected]> wrote:
> >> To use to control the delay attach driver for acpi_device.
> >
> > <blinks>
> > I am not sure what this says. Can you please explain how it controls
> > the delaying of
> > attaching drivers?
> >>
> >> Will use bus notifier to toggle this bits when needed.
> >
> > Will use ..? In a subsequent patch? Which patch? And when is this
> > needed? Is there a patch that needs this?
>
> please check patch 0-4 as a whole.
But that is not how one is going to read the source code in a year or
so. In a year I will look at at file, run 'git gui annotate <file>' and
from the lines can find the corresponding patch. Then from there on
continue on to get the other patches.
The point is that in a year or so, the writeup you did will be
forgotten. If it is part of the _patch_ then it will not be.
If that means the git commit description has a couple of paragraphs
- that is OK.
>
> Yinghai
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>