[PATCH 00/10] PCI, ACPI: Use bus type notifier for root bus hotplug

[PATCH 00/10] PCI, ACPI: Use bus type notifier for root bus hotplug

[Yinghai Lu]

Add device drivers_autoprobe to make hotplug path more like booting path:
create all acpi device and add them to device tree at first
 then attach acpi drivers.
create all pci devices and add them to device tree at first
 then attach pci drivers.

1. kill acpi_pci_root_start.
2. register pci devices to device tree as soon as possible after pci devices
   and bus get created.
   After pci_scan_child_bus, all bus and devices get into devices already.
   pci_bus_add_devices will only attach driver to pci devices.

could get from
        git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git for-pci-next

Yinghai Lu (10):
  device: add drivers_autoprobe in struct device
  ACPI: use device drivers_autoprobe to delay loading acpi drivers
  PCI: prepare to use device drivers_autoprobe to delay attach drivers
  PCI: Use device_add for device and bus early
  PCI, ACPI: Separate out acpi_pci_root_osc_contorl_set
  PCI, ACPI: Move hot add root bus conf code to acpi_pci_root_add
  PCI, ACPI: Remove not used acpi_pci_root_start()
  PCI: Add dev_is_pci_host_bridge() helper
  PCI, ACPI: using acpi/pci bind path for pci_host_bridge
  PCI, ACPI: use bus_type notifier for acpi_pci_bind_notify

 drivers/acpi/glue.c         |    4 -
 drivers/acpi/pci_bind.c     |   99 +++++++++++++++----
 drivers/acpi/pci_root.c     |  223 +++++++++++++++++++++++--------------------
 drivers/acpi/scan.c         |   48 +++++++++-
 drivers/base/bus.c          |    2 +-
 drivers/base/core.c         |    1 +
 drivers/pci/bus.c           |   45 +++------
 drivers/pci/hotplug.c       |   25 +++++
 drivers/pci/iov.c           |    7 --
 drivers/pci/probe.c         |   25 ++++--
 include/acpi/acpi_drivers.h |    2 +
 include/linux/device.h      |    1 +
 include/linux/pci.h         |    1 +
 13 files changed, 310 insertions(+), 173 deletions(-)

-- 
1.7.7

[PATCH 01/10] device: add drivers_autoprobe in struct device

[Yinghai Lu]

To use to control the delay attach driver for specific device.

Will use bus notifier to toggle this bits when needed.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/base/bus.c     |    2 +-
 drivers/base/core.c    |    1 +
 include/linux/device.h |    1 +
 3 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 181ed26..22b826a 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -538,7 +538,7 @@ void bus_probe_device(struct device *dev)
 	if (!bus)
 		return;
 
-	if (bus->p->drivers_autoprobe) {
+	if (bus->p->drivers_autoprobe && dev->drivers_autoprobe) {
 		ret = device_attach(dev);
 		WARN_ON(ret < 0);
 	}
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 5e6e00b..8a71f88 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -656,6 +656,7 @@ void device_initialize(struct device *dev)
 	INIT_LIST_HEAD(&dev->devres_head);
 	device_pm_init(dev);
 	set_dev_node(dev, -1);
+	dev->drivers_autoprobe = true;
 }
 
 static struct kobject *virtual_device_parent(struct device *dev)
diff --git a/include/linux/device.h b/include/linux/device.h
index 52a5f15..e08db26 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -647,6 +647,7 @@ struct device {
 	struct bus_type	*bus;		/* type of bus device is on */
 	struct device_driver *driver;	/* which driver has allocated this
 					   device */
+	bool	drivers_autoprobe;
 	void		*platform_data;	/* Platform specific data, device
 					   core doesn't touch it */
 	struct dev_pm_info	power;
-- 
1.7.7

[PATCH 02/10] ACPI: use device drivers_autoprobe to delay loading acpi drivers

[Yinghai Lu]

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 <yinghai@kernel.org>
---
 drivers/acpi/scan.c |   48 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 47 insertions(+), 1 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 5dfec09..67785da 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1451,6 +1451,20 @@ 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;
+
+	ret = device_attach(&dev->dev);
+	/* make rescan working ? */
+	dev->dev.drivers_autoprobe = true;
+	WARN_ON(ret < 0);
+
+	list_for_each_entry(child, &dev->children, node)
+		acpi_bus_attach(child);
+}
+
 /*
  * acpi_bus_add and acpi_bus_start
  *
@@ -1468,11 +1482,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);
 
@@ -1610,3 +1630,29 @@ 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:
+		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

[PATCH 03/10] PCI: prepare to use device drivers_autoprobe to delay attach drivers

[Yinghai Lu]

Now pci enumerate the pci devices later and later use pci_bus_add_devices to
add them to devices and probe pci devices.

The reason for that two steps, want to do pci_assign_unassigned_resources
for hot add pci devices before pci drivers get loaded.

That step would leave pci devices out of devices tree for a while, and we
can not use bus iterator to loop pci devices.

We could device drivers_autoprobe and pci_bus_type notifier to delay pci
driver loading instead of keep of pci device out of device tree.

The patch add notifier to split out device attach out of device_add.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/bus.c     |   11 +++++++++++
 drivers/pci/hotplug.c |   25 +++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 6fe2115..3144262 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -160,6 +160,16 @@ pci_bus_alloc_resource(struct pci_bus *bus, struct resource *res,
 
 void __weak pcibios_resource_survey_bus(struct pci_bus *bus) { }
 
+static void pci_bus_attach_device(struct pci_dev *dev)
+{
+	int ret;
+
+	ret = device_attach(&dev->dev);
+	/* make rescan working ? */
+	dev->dev.drivers_autoprobe = true;
+	WARN_ON(ret < 0);
+}
+
 /**
  * pci_bus_add_device - add a single device
  * @dev: device to add
@@ -176,6 +186,7 @@ int pci_bus_add_device(struct pci_dev *dev)
 	if (retval)
 		return retval;
 
+	pci_bus_attach_device(dev);
 	dev->is_added = 1;
 	pci_proc_attach_device(dev);
 	pci_create_sysfs_dev_files(dev);
diff --git a/drivers/pci/hotplug.c b/drivers/pci/hotplug.c
index 2b5352a..3983934 100644
--- a/drivers/pci/hotplug.c
+++ b/drivers/pci/hotplug.c
@@ -35,3 +35,28 @@ int pci_uevent(struct device *dev, struct kobj_uevent_env *env)
 		return -ENOMEM;
 	return 0;
 }
+
+static int pci_hp_notifier(struct notifier_block *nb,
+				 unsigned long event, void *data)
+{
+	struct device *dev = data;
+
+	switch (event) {
+	case BUS_NOTIFY_ADD_DEVICE:
+		dev->drivers_autoprobe = false;
+		break;
+	}
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block pci_hp_nb = {
+	.notifier_call = &pci_hp_notifier,
+};
+
+static int __init pci_hp_init(void)
+{
+	return bus_register_notifier(&pci_bus_type, &pci_hp_nb);
+}
+
+fs_initcall(pci_hp_init);
-- 
1.7.7

[PATCH 04/10] PCI: Use device_add for device and bus early

[Yinghai Lu]

Move out device registering out of pci_bus_add_devices, so we could
put new created pci devices in device tree early.

new pci_bus_add_devices will do the device_attach work to load pci drivers
instead.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/bus.c   |   34 ++--------------------------------
 drivers/pci/iov.c   |    7 -------
 drivers/pci/probe.c |   25 ++++++++++++++++++-------
 3 files changed, 20 insertions(+), 46 deletions(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 3144262..5b98505 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -179,12 +179,7 @@ static void pci_bus_attach_device(struct pci_dev *dev)
  */
 int pci_bus_add_device(struct pci_dev *dev)
 {
-	int retval;
-
 	pci_fixup_device(pci_fixup_final, dev);
-	retval = device_add(&dev->dev);
-	if (retval)
-		return retval;
 
 	pci_bus_attach_device(dev);
 	dev->is_added = 1;
@@ -201,21 +196,12 @@ int pci_bus_add_device(struct pci_dev *dev)
  */
 int pci_bus_add_child(struct pci_bus *bus)
 {
-	int retval;
-
-	if (bus->bridge)
-		bus->dev.parent = bus->bridge;
-
-	retval = device_add(&bus->dev);
-	if (retval)
-		return retval;
-
 	bus->is_added = 1;
 
 	/* Create legacy_io and legacy_mem files for this bus */
 	pci_create_legacy_files(bus);
 
-	return retval;
+	return 0;
 }
 
 /**
@@ -241,36 +227,20 @@ void pci_bus_add_devices(const struct pci_bus *bus)
 		if (dev->is_added)
 			continue;
 		retval = pci_bus_add_device(dev);
-		if (retval)
-			dev_err(&dev->dev, "Error adding device, continuing\n");
 	}
 
 	list_for_each_entry(dev, &bus->devices, bus_list) {
 		BUG_ON(!dev->is_added);
 
 		child = dev->subordinate;
-		/*
-		 * If there is an unattached subordinate bus, attach
-		 * it and then scan for unattached PCI devices.
-		 */
+
 		if (!child)
 			continue;
-		if (list_empty(&child->node)) {
-			down_write(&pci_bus_sem);
-			list_add_tail(&child->node, &dev->bus->children);
-			up_write(&pci_bus_sem);
-		}
 		pci_bus_add_devices(child);
 
-		/*
-		 * register the bus with sysfs as the parent is now
-		 * properly registered.
-		 */
 		if (child->is_added)
 			continue;
 		retval = pci_bus_add_child(child);
-		if (retval)
-			dev_err(&dev->dev, "Error adding bus, continuing\n");
 	}
 }
 
diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index aeccc91..f286583 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -48,12 +48,7 @@ static struct pci_bus *virtfn_add_bus(struct pci_bus *bus, int busnr)
 		return NULL;
 
 	pci_bus_insert_busn_res(child, busnr, busnr);
-	child->dev.parent = bus->bridge;
 	rc = pci_bus_add_child(child);
-	if (rc) {
-		pci_remove_bus(child);
-		return NULL;
-	}
 
 	return child;
 }
@@ -123,8 +118,6 @@ static int virtfn_add(struct pci_dev *dev, int id, int reset)
 	virtfn->is_virtfn = 1;
 
 	rc = pci_bus_add_device(virtfn);
-	if (rc)
-		goto failed1;
 	sprintf(buf, "virtfn%u", id);
 	rc = sysfs_create_link(&dev->dev.kobj, &virtfn->dev.kobj, buf);
 	if (rc)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index c3da8f5..c1b7292 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -616,6 +616,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
 {
 	struct pci_bus *child;
 	int i;
+	int ret;
 
 	/*
 	 * Allocate a new bus, and inherit stuff from the parent..
@@ -630,8 +631,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
 	child->bus_flags = parent->bus_flags;
 
 	/* initialize some portions of the bus device, but don't register it
-	 * now as the parent is not properly set up yet.  This device will get
-	 * registered later in pci_bus_add_devices()
+	 * now as the parent is not properly set up yet.
 	 */
 	child->dev.class = &pcibus_class;
 	dev_set_name(&child->dev, "%04x:%02x", pci_domain_nr(child), busnr);
@@ -645,11 +645,14 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
 	child->primary = parent->busn_res.start;
 	child->busn_res.end = 0xff;
 
-	if (!bridge)
-		return child;
+	if (!bridge) {
+		child->dev.parent = parent->bridge;
+		goto add_dev;
+	}
 
 	child->self = bridge;
 	child->bridge = get_device(&bridge->dev);
+	child->dev.parent = child->bridge;
 	pci_set_bus_of_node(child);
 	pci_set_bus_speed(child);
 
@@ -660,6 +663,10 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
 	}
 	bridge->subordinate = child;
 
+add_dev:
+	ret = device_add(&child->dev);
+	WARN_ON(ret < 0);
+
 	return child;
 }
 
@@ -1290,6 +1297,8 @@ static void pci_init_capabilities(struct pci_dev *dev)
 
 void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
 {
+	int ret;
+
 	device_initialize(&dev->dev);
 	dev->dev.release = pci_release_dev;
 
@@ -1320,6 +1329,10 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
 	down_write(&pci_bus_sem);
 	list_add_tail(&dev->bus_list, &bus->devices);
 	up_write(&pci_bus_sem);
+
+	/* notifier could use pci capabilities */
+	ret = device_add(&dev->dev);
+	WARN_ON(ret < 0);
 }
 
 struct pci_dev *__ref pci_scan_single_device(struct pci_bus *bus, int devfn)
@@ -1638,13 +1651,13 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 	char bus_addr[64];
 	char *fmt;
 
-
 	b = pci_alloc_bus();
 	if (!b)
 		return NULL;
 
 	b->sysdata = sysdata;
 	b->ops = ops;
+	b->number = b->busn_res.start = bus;
 	b2 = pci_find_bus(pci_domain_nr(b), bus);
 	if (b2) {
 		/* If we already got to this bus through a different bridge, ignore it */
@@ -1681,8 +1694,6 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 	/* Create legacy_io and legacy_mem files for this bus */
 	pci_create_legacy_files(b);
 
-	b->number = b->busn_res.start = bus;
-
 	if (parent)
 		dev_info(parent, "PCI host bridge to bus %s\n", dev_name(&b->dev));
 	else
-- 
1.7.7

[PATCH 05/10] PCI, ACPI: Separate out acpi_pci_root_osc_contorl_set

[Yinghai Lu]

acpi_pci_root_add is too big now. We could move out root osc related code
out of it.

Only code moving.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/acpi/pci_root.c |  136 +++++++++++++++++++++++++----------------------
 1 files changed, 73 insertions(+), 63 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 085fccf..75643ce 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -430,6 +430,76 @@ out:
 }
 EXPORT_SYMBOL(acpi_pci_osc_control_set);
 
+static void acpi_pci_root_osc_control_set(struct acpi_pci_root *root)
+{
+	u32 flags, base_flags;
+	acpi_status status;
+
+	flags = base_flags = OSC_PCI_SEGMENT_GROUPS_SUPPORT;
+
+	/* Indicate support for various _OSC capabilities. */
+	if (pci_ext_cfg_avail(root->bus->self))
+		flags |= OSC_EXT_PCI_CONFIG_SUPPORT;
+	if (pcie_aspm_support_enabled())
+		flags |= OSC_ACTIVE_STATE_PWR_SUPPORT |
+			OSC_CLOCK_PWR_CAPABILITY_SUPPORT;
+	if (pci_msi_enabled())
+		flags |= OSC_MSI_SUPPORT;
+	if (flags != base_flags) {
+		status = acpi_pci_osc_support(root, flags);
+		if (ACPI_FAILURE(status)) {
+			dev_info(root->bus->bridge, "ACPI _OSC support "
+				"notification failed, disabling PCIe ASPM\n");
+			pcie_no_aspm();
+			flags = base_flags;
+		}
+	}
+
+	if (!pcie_ports_disabled
+	    && (flags & ACPI_PCIE_REQ_SUPPORT) == ACPI_PCIE_REQ_SUPPORT) {
+		flags = OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL
+			| OSC_PCI_EXPRESS_NATIVE_HP_CONTROL
+			| OSC_PCI_EXPRESS_PME_CONTROL;
+
+		if (pci_aer_available()) {
+			if (aer_acpi_firmware_first())
+				dev_dbg(root->bus->bridge,
+					"PCIe errors handled by BIOS.\n");
+			else
+				flags |= OSC_PCI_EXPRESS_AER_CONTROL;
+		}
+
+		dev_info(root->bus->bridge,
+			"Requesting ACPI _OSC control (0x%02x)\n", flags);
+
+		status = acpi_pci_osc_control_set(root->device->handle, &flags,
+					OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL);
+		if (ACPI_SUCCESS(status)) {
+			dev_info(root->bus->bridge,
+				"ACPI _OSC control (0x%02x) granted\n", flags);
+			if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_ASPM) {
+				/*
+				 * We have ASPM control, but the FADT indicates
+				 * that it's unsupported. Clear it.
+				 */
+				pcie_clear_aspm(root->bus);
+			}
+		} else {
+			dev_info(root->bus->bridge,
+				"ACPI _OSC request failed (%s), "
+				"returned control mask: 0x%02x\n",
+				acpi_format_exception(status), flags);
+			pr_info("ACPI _OSC control for PCIe not granted, "
+				"disabling ASPM\n");
+			pcie_no_aspm();
+		}
+	} else {
+		dev_info(root->bus->bridge,
+			 "Unable to request _OSC control "
+			 "(_OSC support mask: 0x%02x)\n", flags);
+	}
+}
+
 static int __devinit acpi_pci_root_add(struct acpi_device *device)
 {
 	unsigned long long segment, bus;
@@ -437,7 +507,7 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
 	int result;
 	struct acpi_pci_root *root;
 	acpi_handle handle;
-	u32 flags, base_flags;
+	u32 flags;
 
 	root = kzalloc(sizeof(struct acpi_pci_root), GFP_KERNEL);
 	if (!root)
@@ -491,7 +561,7 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
 	 * All supported architectures that use ACPI have support for
 	 * PCI domains, so we indicate this in _OSC support capabilities.
 	 */
-	flags = base_flags = OSC_PCI_SEGMENT_GROUPS_SUPPORT;
+	flags = OSC_PCI_SEGMENT_GROUPS_SUPPORT;
 	acpi_pci_osc_support(root, flags);
 
 	/*
@@ -531,67 +601,7 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
 	if (ACPI_SUCCESS(status))
 		result = acpi_pci_irq_add_prt(device->handle, root->bus);
 
-	/* Indicate support for various _OSC capabilities. */
-	if (pci_ext_cfg_avail(root->bus->self))
-		flags |= OSC_EXT_PCI_CONFIG_SUPPORT;
-	if (pcie_aspm_support_enabled())
-		flags |= OSC_ACTIVE_STATE_PWR_SUPPORT |
-			OSC_CLOCK_PWR_CAPABILITY_SUPPORT;
-	if (pci_msi_enabled())
-		flags |= OSC_MSI_SUPPORT;
-	if (flags != base_flags) {
-		status = acpi_pci_osc_support(root, flags);
-		if (ACPI_FAILURE(status)) {
-			dev_info(root->bus->bridge, "ACPI _OSC support "
-				"notification failed, disabling PCIe ASPM\n");
-			pcie_no_aspm();
-			flags = base_flags;
-		}
-	}
-
-	if (!pcie_ports_disabled
-	    && (flags & ACPI_PCIE_REQ_SUPPORT) == ACPI_PCIE_REQ_SUPPORT) {
-		flags = OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL
-			| OSC_PCI_EXPRESS_NATIVE_HP_CONTROL
-			| OSC_PCI_EXPRESS_PME_CONTROL;
-
-		if (pci_aer_available()) {
-			if (aer_acpi_firmware_first())
-				dev_dbg(root->bus->bridge,
-					"PCIe errors handled by BIOS.\n");
-			else
-				flags |= OSC_PCI_EXPRESS_AER_CONTROL;
-		}
-
-		dev_info(root->bus->bridge,
-			"Requesting ACPI _OSC control (0x%02x)\n", flags);
-
-		status = acpi_pci_osc_control_set(device->handle, &flags,
-					OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL);
-		if (ACPI_SUCCESS(status)) {
-			dev_info(root->bus->bridge,
-				"ACPI _OSC control (0x%02x) granted\n", flags);
-			if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_ASPM) {
-				/*
-				 * We have ASPM control, but the FADT indicates
-				 * that it's unsupported. Clear it.
-				 */
-				pcie_clear_aspm(root->bus);
-			}
-		} else {
-			dev_info(root->bus->bridge,
-				"ACPI _OSC request failed (%s), "
-				"returned control mask: 0x%02x\n",
-				acpi_format_exception(status), flags);
-			pr_info("ACPI _OSC control for PCIe not granted, "
-				"disabling ASPM\n");
-			pcie_no_aspm();
-		}
-	} else {
-		dev_info(root->bus->bridge,
-			 "Unable to request _OSC control "
-			 "(_OSC support mask: 0x%02x)\n", flags);
-	}
+	acpi_pci_root_osc_control_set(root);
 
 	pci_acpi_add_bus_pm_notifier(device, root->bus);
 	if (device->wakeup.flags.run_wake)
-- 
1.7.7

[PATCH 06/10] PCI, ACPI: Move hot add root bus conf code to acpi_pci_root_add

[Yinghai Lu]

It is safe before we really put those pci devices in the tree.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc:Len Brown <lenb@kernel.org>
Cc: linux-acpi@vger.kernel.org
---
 drivers/acpi/pci_root.c |   31 ++++++++++++++++---------------
 1 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 75643ce..e63a58d 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -506,6 +506,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;
 	u32 flags;
 
@@ -607,6 +608,21 @@ 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);
 
+	if (system_state != SYSTEM_BOOTING) {
+		pcibios_resource_survey_bus(root->bus);
+		pci_assign_unassigned_bus_resources(root->bus);
+	}
+
+	mutex_lock(&acpi_pci_root_lock);
+	list_for_each_entry(driver, &acpi_pci_drivers, node)
+		if (driver->add)
+			driver->add(root);
+	mutex_unlock(&acpi_pci_root_lock);
+
+	/* need to after hot-added ioapic is registered */
+	if (system_state != SYSTEM_BOOTING)
+		pci_enable_bridges(root->bus);
+
 	return 0;
 
 out_del_root:
@@ -621,22 +637,7 @@ end:
 static int acpi_pci_root_start(struct acpi_device *device)
 {
 	struct acpi_pci_root *root = acpi_driver_data(device);
-	struct acpi_pci_driver *driver;
-
-	if (system_state != SYSTEM_BOOTING) {
-		pcibios_resource_survey_bus(root->bus);
-		pci_assign_unassigned_bus_resources(root->bus);
-	}
 
-	mutex_lock(&acpi_pci_root_lock);
-	list_for_each_entry(driver, &acpi_pci_drivers, node)
-		if (driver->add)
-			driver->add(root);
-	mutex_unlock(&acpi_pci_root_lock);
-
-	/* need to after hot-added ioapic is registered */
-	if (system_state != SYSTEM_BOOTING)
-		pci_enable_bridges(root->bus);
 
 	pci_bus_add_devices(root->bus);
 
-- 
1.7.7

[PATCH 07/10] PCI, ACPI: Remove not used acpi_pci_root_start()

[Yinghai Lu]

pci_add_bus_devices has been moved out.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: Len Brown <lenb@kernel.org>
Cc: linux-acpi@vger.kernel.org
---
 drivers/acpi/pci_root.c |   14 ++------------
 1 files changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index e63a58d..8134f72 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,
 		},
 };
 
@@ -623,6 +621,8 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
 	if (system_state != SYSTEM_BOOTING)
 		pci_enable_bridges(root->bus);
 
+	pci_bus_add_devices(root->bus);
+
 	return 0;
 
 out_del_root:
@@ -634,16 +634,6 @@ end:
 	return result;
 }
 
-static int acpi_pci_root_start(struct acpi_device *device)
-{
-	struct acpi_pci_root *root = acpi_driver_data(device);
-
-
-	pci_bus_add_devices(root->bus);
-
-	return 0;
-}
-
 static int acpi_pci_root_remove(struct acpi_device *device, int type)
 {
 	acpi_status status;
-- 
1.7.7

[PATCH 08/10] PCI: Add dev_is_pci_host_bridge() helper

[Yinghai Lu]

Need to use it later for one function that is shared with pci device.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 include/linux/pci.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 14703c5..7ee1028 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -739,6 +739,7 @@ void pci_stop_root_bus(struct pci_bus *bus);
 void pci_remove_root_bus(struct pci_bus *bus);
 void pci_setup_cardbus(struct pci_bus *bus);
 extern void pci_sort_breadthfirst(void);
+#define dev_is_pci_host_bridge(d) ((d)->bus == &pci_host_bridge_bus_type)
 #define dev_is_pci(d) ((d)->bus == &pci_bus_type)
 #define dev_is_pf(d) ((dev_is_pci(d) ? to_pci_dev(d)->is_physfn : false))
 #define dev_num_vf(d) ((dev_is_pci(d) ? pci_num_vf(to_pci_dev(d)) : 0))
-- 
1.7.7

[PATCH 09/10] PCI, ACPI: using acpi/pci bind path for pci_host_bridge

[Yinghai Lu]

Now pci_dev is using:
	pci_bus_add_device
		==> device_add
			==> platform_notify aka acpi_platform_notify
				==> acpi_bind_one
					==> acpi_pci_bind_notify

pci_host_bridge calling till to acpi_bind_one but not in acpi_pci_bind_notify
because acpi_pci_bind_notify only handle pci_dev.

Extend acpi_pci_bind_notify to handle pci_host_bridge.

After that, We could remove same calling in acpi_pci_root_add and
acpi_pci_root_remove.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: Len Brown <lenb@kernel.org>
Cc: linux-acpi@vger.kernel.org
---
 drivers/acpi/pci_bind.c |   59 +++++++++++++++++++++++++++++++---------------
 drivers/acpi/pci_root.c |   21 ----------------
 2 files changed, 40 insertions(+), 40 deletions(-)

diff --git a/drivers/acpi/pci_bind.c b/drivers/acpi/pci_bind.c
index aac7f9a..b59a5df 100644
--- a/drivers/acpi/pci_bind.c
+++ b/drivers/acpi/pci_bind.c
@@ -35,33 +35,59 @@
 #define _COMPONENT		ACPI_PCI_COMPONENT
 ACPI_MODULE_NAME("pci_bind");
 
-static int acpi_pci_unbind(struct acpi_device *acpi_dev, struct pci_dev *dev)
+static int acpi_pci_unbind(struct acpi_device *acpi_dev, struct device *dev)
 {
+	struct pci_dev *pdev = NULL;
+	struct pci_bus *bus = NULL;
+
+	if (dev_is_pci(dev)) {
+		pdev = to_pci_dev(dev);
+		if (pdev->subordinate)
+			bus = pdev->subordinate;
+	} else
+			bus = to_pci_host_bridge(dev)->bus;
+
 	if (acpi_dev) {
-		device_set_run_wake(&dev->dev, false);
-		pci_acpi_remove_pm_notifier(acpi_dev);
+		device_set_run_wake(dev, false);
+		if (pdev)
+			pci_acpi_remove_pm_notifier(acpi_dev);
+		else
+			pci_acpi_remove_bus_pm_notifier(acpi_dev);
 	}
 
-	if (dev->subordinate)
-		acpi_pci_irq_del_prt(dev->subordinate);
+	if (bus)
+		acpi_pci_irq_del_prt(bus);
 
 	return 0;
 }
 
-static int acpi_pci_bind(struct acpi_device *acpi_dev, struct pci_dev *dev)
+static int acpi_pci_bind(struct acpi_device *acpi_dev, struct device *dev)
 {
 	acpi_status status;
-	struct pci_bus *bus;
+	struct pci_dev *pdev = NULL;
+	struct pci_bus *bus = NULL;
 	acpi_handle tmp_hdl;
 	acpi_handle handle;
 
+	if (dev_is_pci(dev)) {
+		pdev = to_pci_dev(dev);
+		if (pdev->subordinate)
+			bus = pdev->subordinate;
+		else
+			bus = pdev->bus;
+	} else
+			bus = to_pci_host_bridge(dev)->bus;
+
 	if (acpi_dev) {
-		pci_acpi_add_pm_notifier(acpi_dev, dev);
+		if (pdev)
+			pci_acpi_add_pm_notifier(acpi_dev, pdev);
+		else
+			pci_acpi_add_bus_pm_notifier(acpi_dev, bus);
 		if (acpi_dev->wakeup.flags.run_wake)
-			device_set_run_wake(&dev->dev, true);
+			device_set_run_wake(dev, true);
 		handle = acpi_dev->handle;
 	} else
-		handle = DEVICE_ACPI_HANDLE(&dev->dev);
+		handle = DEVICE_ACPI_HANDLE(dev);
 
 	/*
 	 * Evaluate and parse _PRT, if exists.  This code allows parsing of
@@ -72,13 +98,8 @@ static int acpi_pci_bind(struct acpi_device *acpi_dev, struct pci_dev *dev)
 	 * TBD: Can _PRTs exist within the scope of non-bridge PCI devices?
 	 */
 	status = acpi_get_handle(handle, METHOD_NAME__PRT, &tmp_hdl);
-	if (ACPI_SUCCESS(status)) {
-		if (dev->subordinate)
-			bus = dev->subordinate;
-		else
-			bus = dev->bus;
+	if (ACPI_SUCCESS(status))
 		acpi_pci_irq_add_prt(handle, bus);
-	}
 
 	return 0;
 }
@@ -86,10 +107,10 @@ static int acpi_pci_bind(struct acpi_device *acpi_dev, struct pci_dev *dev)
 void acpi_pci_bind_notify(struct acpi_device *acpi_dev, struct device *dev,
 			  bool bind)
 {
-	if (dev_is_pci(dev)) {
+	if (dev_is_pci(dev) || dev_is_pci_host_bridge(dev)) {
 		if (bind)
-			acpi_pci_bind(acpi_dev, to_pci_dev(dev));
+			acpi_pci_bind(acpi_dev, dev);
 		else
-			acpi_pci_unbind(acpi_dev, to_pci_dev(dev));
+			acpi_pci_unbind(acpi_dev, dev);
 	}
 }
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 8134f72..725ec10 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -505,7 +505,6 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
 	int result;
 	struct acpi_pci_root *root;
 	struct acpi_pci_driver *driver;
-	acpi_handle handle;
 	u32 flags;
 
 	root = kzalloc(sizeof(struct acpi_pci_root), GFP_KERNEL);
@@ -591,21 +590,8 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
 		goto out_del_root;
 	}
 
-	/*
-	 * PCI Routing Table
-	 * -----------------
-	 * Evaluate and parse _PRT, if exists.
-	 */
-	status = acpi_get_handle(device->handle, METHOD_NAME__PRT, &handle);
-	if (ACPI_SUCCESS(status))
-		result = acpi_pci_irq_add_prt(device->handle, root->bus);
-
 	acpi_pci_root_osc_control_set(root);
 
-	pci_acpi_add_bus_pm_notifier(device, root->bus);
-	if (device->wakeup.flags.run_wake)
-		device_set_run_wake(root->bus->bridge, true);
-
 	if (system_state != SYSTEM_BOOTING) {
 		pcibios_resource_survey_bus(root->bus);
 		pci_assign_unassigned_bus_resources(root->bus);
@@ -649,13 +635,6 @@ static int acpi_pci_root_remove(struct acpi_device *device, int type)
 			driver->remove(root);
 	mutex_unlock(&acpi_pci_root_lock);
 
-	device_set_run_wake(root->bus->bridge, false);
-	pci_acpi_remove_bus_pm_notifier(device);
-
-	status = acpi_get_handle(device->handle, METHOD_NAME__PRT, &handle);
-	if (ACPI_SUCCESS(status))
-		acpi_pci_irq_del_prt(root->bus);
-
 	pci_remove_root_bus(root->bus);
 
 	mutex_lock(&acpi_pci_root_lock);
-- 
1.7.7

[PATCH 10/10] PCI, ACPI: use bus_type notifier for acpi_pci_bind_notify

[Yinghai Lu]

We should not put  pci specific code in generic acpi glue code, especially
after we have bus_type for pci_host_bridge in addition to pci_dev.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: Len Brown <lenb@kernel.org>
Cc: linux-acpi@vger.kernel.org
---
 drivers/acpi/glue.c         |    4 ----
 drivers/acpi/pci_bind.c     |   40 ++++++++++++++++++++++++++++++++++++++++
 drivers/acpi/pci_root.c     |   39 +++++++++++++++++++++++++++++++++++++++
 include/acpi/acpi_drivers.h |    2 ++
 4 files changed, 81 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
index fe72b3e..3273178 100644
--- a/drivers/acpi/glue.c
+++ b/drivers/acpi/glue.c
@@ -164,8 +164,6 @@ static int acpi_bind_one(struct device *dev, acpi_handle handle)
 	if (ACPI_FAILURE(status))
 		acpi_dev = NULL;
 
-	acpi_pci_bind_notify(acpi_dev, dev, true);
-
 	if (acpi_dev) {
 		int ret;
 
@@ -197,8 +195,6 @@ static int acpi_unbind_one(struct device *dev)
 		} else
 			acpi_dev = NULL;
 
-		acpi_pci_bind_notify(acpi_dev, dev, false);
-
 		acpi_detach_data(dev->archdata.acpi_handle,
 				 acpi_glue_data_handler);
 		dev->archdata.acpi_handle = NULL;
diff --git a/drivers/acpi/pci_bind.c b/drivers/acpi/pci_bind.c
index b59a5df..f938b2e 100644
--- a/drivers/acpi/pci_bind.c
+++ b/drivers/acpi/pci_bind.c
@@ -114,3 +114,43 @@ void acpi_pci_bind_notify(struct acpi_device *acpi_dev, struct device *dev,
 			acpi_pci_unbind(acpi_dev, dev);
 	}
 }
+
+static int pci_dev_hp_notifier(struct notifier_block *nb,
+				 unsigned long event, void *data)
+{
+	struct device *dev = data;
+	acpi_handle handle = DEVICE_ACPI_HANDLE(dev);
+	struct acpi_device *acpi_dev;
+
+	if (!handle)
+		return NOTIFY_OK;
+
+	if (acpi_bus_get_device(handle, &acpi_dev))
+		return NOTIFY_OK;
+
+	if (!acpi_dev)
+		return NOTIFY_OK;
+
+	switch (event) {
+	case BUS_NOTIFY_ADD_DEVICE:
+		acpi_pci_bind_notify(acpi_dev, dev, true);
+		break;
+	case BUS_NOTIFY_DEL_DEVICE:
+		acpi_pci_bind_notify(acpi_dev, dev, false);
+		break;
+	}
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block pci_dev_hp_nb = {
+	.notifier_call = &pci_dev_hp_notifier,
+};
+
+static int __init pci_dev_hp_init(void)
+{
+	return bus_register_notifier(&pci_bus_type,
+					 &pci_dev_hp_nb);
+}
+
+arch_initcall(pci_dev_hp_init);
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 725ec10..d2c194b 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -659,3 +659,42 @@ static int __init acpi_pci_root_init(void)
 }
 
 subsys_initcall(acpi_pci_root_init);
+
+static int pci_host_bridge_hp_notifier(struct notifier_block *nb,
+				 unsigned long event, void *data)
+{
+	struct device *dev = data;
+	acpi_handle handle = DEVICE_ACPI_HANDLE(dev);
+	struct acpi_pci_root *root;
+	struct acpi_device *acpi_dev;
+
+	if (!handle)
+		return NOTIFY_OK;
+
+	root = acpi_pci_find_root(handle);
+	acpi_dev = root->device;
+
+	switch (event) {
+	case BUS_NOTIFY_ADD_DEVICE:
+		acpi_pci_bind_notify(acpi_dev, dev, true);
+		break;
+	case BUS_NOTIFY_DEL_DEVICE:
+		acpi_pci_bind_notify(acpi_dev, dev, true);
+		break;
+	}
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block pci_host_bridge_hp_nb = {
+	.notifier_call = &pci_host_bridge_hp_notifier,
+};
+
+static int __init pci_host_bridge_hp_init(void)
+{
+	return bus_register_notifier(&pci_host_bridge_bus_type,
+					 &pci_host_bridge_hp_nb);
+}
+
+arch_initcall(pci_host_bridge_hp_init);
+
diff --git a/include/acpi/acpi_drivers.h b/include/acpi/acpi_drivers.h
index fb1c0d5..c258500 100644
--- a/include/acpi/acpi_drivers.h
+++ b/include/acpi/acpi_drivers.h
@@ -100,6 +100,8 @@ void acpi_pci_irq_del_prt(struct pci_bus *bus);
 struct pci_bus;
 
 struct pci_dev *acpi_get_pci_dev(acpi_handle);
+void acpi_pci_bind_notify(struct acpi_device *acpi_dev, struct device *dev,
+			  bool bind);
 
 /* Arch-defined function to add a bus to the system */
 
-- 
1.7.7

Re: [PATCH 01/10] device: add drivers_autoprobe in struct device

[Greg Kroah-Hartman]

On Mon, Oct 01, 2012 at 11:32:59PM -0700, Yinghai Lu wrote:
> To use to control the delay attach driver for specific device.
> 
> Will use bus notifier to toggle this bits when needed.
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Without a whole lot more context as to why you want this change, no,
sorry, I'll not accept it.

greg k-h

Re: [PATCH 01/10] device: add drivers_autoprobe in struct device

[Yinghai Lu]

On Tue, Oct 2, 2012 at 6:33 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Mon, Oct 01, 2012 at 11:32:59PM -0700, Yinghai Lu wrote:
>> To use to control the delay attach driver for specific device.
>>
>> Will use bus notifier to toggle this bits when needed.
>>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> Without a whole lot more context as to why you want this change, no,
> sorry, I'll not accept it.

Sorry, I should put you on the to list for the whole patchset.

Those patches address Bjorn's request:
1. kill acpi_pci_root_start in drivers/acpi/pci_root.c
2. register pci device to device as soon as possible, so for_each_pci_device
   could be used early before pci_bus_add_devices.

please check change log patch2-4.

https://patchwork.kernel.org/patch/1535731/
https://patchwork.kernel.org/patch/1535821/
https://patchwork.kernel.org/patch/1535851/

Thanks

Yinghai

Re: [PATCH 01/10] device: add drivers_autoprobe in struct device

[Greg Kroah-Hartman]

On Tue, Oct 02, 2012 at 10:39:25AM -0700, Yinghai Lu wrote:
> On Tue, Oct 2, 2012 at 6:33 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Mon, Oct 01, 2012 at 11:32:59PM -0700, Yinghai Lu wrote:
> >> To use to control the delay attach driver for specific device.
> >>
> >> Will use bus notifier to toggle this bits when needed.
> >>
> >> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >
> > Without a whole lot more context as to why you want this change, no,
> > sorry, I'll not accept it.
> 
> Sorry, I should put you on the to list for the whole patchset.
> 
> Those patches address Bjorn's request:
> 1. kill acpi_pci_root_start in drivers/acpi/pci_root.c
> 2. register pci device to device as soon as possible, so for_each_pci_device
>    could be used early before pci_bus_add_devices.
> 
> please check change log patch2-4.

Sorry, I still don't see what is so special about PCI that they have to
do something different here.  What am I missing?

greg k-h

Re: [PATCH 01/10] device: add drivers_autoprobe in struct device

[Bjorn Helgaas]

On Tue, Oct 2, 2012 at 11:47 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Tue, Oct 02, 2012 at 10:39:25AM -0700, Yinghai Lu wrote:
>> On Tue, Oct 2, 2012 at 6:33 AM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> > On Mon, Oct 01, 2012 at 11:32:59PM -0700, Yinghai Lu wrote:
>> >> To use to control the delay attach driver for specific device.
>> >>
>> >> Will use bus notifier to toggle this bits when needed.
>> >>
>> >> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> >
>> > Without a whole lot more context as to why you want this change, no,
>> > sorry, I'll not accept it.
>>
>> Sorry, I should put you on the to list for the whole patchset.
>>
>> Those patches address Bjorn's request:
>> 1. kill acpi_pci_root_start in drivers/acpi/pci_root.c
>> 2. register pci device to device as soon as possible, so for_each_pci_device
>>    could be used early before pci_bus_add_devices.

Just to be clear, I'm in favor of eliminating acpi_pci_root_start()
eventually, but I didn't suggest this approach, and I don't like it
much either.  I don't have much constructive feedback yet, but this
feels like a special case that's not cleanly integrated into the
existing infrastructure.

>> please check change log patch2-4.
>
> Sorry, I still don't see what is so special about PCI that they have to
> do something different here.  What am I missing?

Re: [PATCH 01/10] device: add drivers_autoprobe in struct device

[Yinghai Lu]

On Tue, Oct 2, 2012 at 10:47 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Tue, Oct 02, 2012 at 10:39:25AM -0700, Yinghai Lu wrote:
>> On Tue, Oct 2, 2012 at 6:33 AM, Greg Kroah-Hartman
>> Those patches address Bjorn's request:
>> 1. kill acpi_pci_root_start in drivers/acpi/pci_root.c
>> 2. register pci device to device as soon as possible, so for_each_pci_device
>>    could be used early before pci_bus_add_devices.
>>
>> please check change log patch2-4.
>
> Sorry, I still don't see what is so special about PCI that they have to
> do something different here.  What am I missing?

That is something that core related with hotplug:

for booting and hotplug, we have different code path.
1. for booting, all device are enumerated and registered, and later
driver get registered, driver is attached to the devices.
2. for hotplug, one device on the bus is found, and then registered
and driver is attached, after that
                     another device on the bus is found, and then
registered and driver is attached...

The reason for the difference, device driver is not registered later
during booting path.

We should make the two path have same sequence. the solution that I
suggest is adding per device drivers_autoprobe.

and thanking for bus_type notifier that we could toggle that bit
during device_add, so could make all device on same
bus get probed and registered but driver get skipped. and after that
call device_attach for all device at one batch.

That will remove those hotplug related hacks like acpi_pci_root_start
that try to delay registering of pci devices.
and make the code lesser and readable.

Thanks

Yinghai

Re: [PATCH 01/10] device: add drivers_autoprobe in struct device

[Greg Kroah-Hartman]

On Tue, Oct 02, 2012 at 01:20:58PM -0700, Yinghai Lu wrote:
> On Tue, Oct 2, 2012 at 10:47 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Tue, Oct 02, 2012 at 10:39:25AM -0700, Yinghai Lu wrote:
> >> On Tue, Oct 2, 2012 at 6:33 AM, Greg Kroah-Hartman
> >> Those patches address Bjorn's request:
> >> 1. kill acpi_pci_root_start in drivers/acpi/pci_root.c
> >> 2. register pci device to device as soon as possible, so for_each_pci_device
> >>    could be used early before pci_bus_add_devices.
> >>
> >> please check change log patch2-4.
> >
> > Sorry, I still don't see what is so special about PCI that they have to
> > do something different here.  What am I missing?
> 
> That is something that core related with hotplug:
> 
> for booting and hotplug, we have different code path.
> 1. for booting, all device are enumerated and registered, and later
> driver get registered, driver is attached to the devices.
> 2. for hotplug, one device on the bus is found, and then registered
> and driver is attached, after that
>                      another device on the bus is found, and then
> registered and driver is attached...
> 
> The reason for the difference, device driver is not registered later
> during booting path.

But on each case, the code path in the driver core is the same, and you
don't know if for 1) a driver is not already registered in the system
(think drivers built into the kernel).

> We should make the two path have same sequence. the solution that I
> suggest is adding per device drivers_autoprobe.

No, now you have 2 code paths in the driver core, do not do that.

> and thanking for bus_type notifier that we could toggle that bit
> during device_add, so could make all device on same
> bus get probed and registered but driver get skipped. and after that
> call device_attach for all device at one batch.
> 
> That will remove those hotplug related hacks like acpi_pci_root_start
> that try to delay registering of pci devices.
> and make the code lesser and readable.

I don't know what you are doing in the acpi code, but I do not think it
requires a driver core change like this.

Again, why is PCI different from any other bus type?  (hint, it
shouldn't be...)

greg k-h

Re: [PATCH 01/10] device: add drivers_autoprobe in struct device

[Yinghai Lu]

On Tue, Oct 2, 2012 at 1:45 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Tue, Oct 02, 2012 at 01:20:58PM -0700, Yinghai Lu wrote:
>> On Tue, Oct 2, 2012 at 10:47 AM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> > On Tue, Oct 02, 2012 at 10:39:25AM -0700, Yinghai Lu wrote:
>> >> On Tue, Oct 2, 2012 at 6:33 AM, Greg Kroah-Hartman
>> >> Those patches address Bjorn's request:
>> >> 1. kill acpi_pci_root_start in drivers/acpi/pci_root.c
>> >> 2. register pci device to device as soon as possible, so for_each_pci_device
>> >>    could be used early before pci_bus_add_devices.
>> >>
>> >> please check change log patch2-4.
>> >
>> > Sorry, I still don't see what is so special about PCI that they have to
>> > do something different here.  What am I missing?
>>
>> That is something that core related with hotplug:
>>
>> for booting and hotplug, we have different code path.
>> 1. for booting, all device are enumerated and registered, and later
>> driver get registered, driver is attached to the devices.
>> 2. for hotplug, one device on the bus is found, and then registered
>> and driver is attached, after that
>>                      another device on the bus is found, and then
>> registered and driver is attached...
>>
>> The reason for the difference, device driver is not registered later
>> during booting path.
>
> But on each case, the code path in the driver core is the same, and you
> don't know if for 1) a driver is not already registered in the system
> (think drivers built into the kernel).

different level initcall or link sequence make sure that in order. aka
bus_type come first before driver of that bus type.

>
>> We should make the two path have same sequence. the solution that I
>> suggest is adding per device drivers_autoprobe.
>
> No, now you have 2 code paths in the driver core, do not do that.

no, reduce 2 to 1.

>
>> and thanking for bus_type notifier that we could toggle that bit
>> during device_add, so could make all device on same
>> bus get probed and registered but driver get skipped. and after that
>> call device_attach for all device at one batch.
>>
>> That will remove those hotplug related hacks like acpi_pci_root_start
>> that try to delay registering of pci devices.
>> and make the code lesser and readable.
>
> I don't know what you are doing in the acpi code, but I do not think it
> requires a driver core change like this.
>
> Again, why is PCI different from any other bus type?  (hint, it
> shouldn't be...)

it is not pci susbsystem problem.

the problem come from device-driver core for hotplug handling, and
acpi system is
forcing pci system to split the scan and registering.
aka: to scan all pci device under one pci_root from acpi_pci_root_add.
then do pci_bus_add_devices for that bus in acpi_pci_root_start.

on hotplug path:
when acpi enumerating acpi devices, it will create acpi_device for pci_root,
then probe driver for that device : acpi_pci_root_driver.
<at that time no acpi_device under that pci_root acpi device is created>
device_add for root acpi_device will call bus_probe_device and ... and execute
acpi_pci_root_driver .add aka acpi_pci_root_add.
acpi_pci_root_add will call pci_acpi_scan_root. that will scan pci
root bus to find all
pci devices but does not add those pci device into devices.
If at the time device_add for pci device get called, platform_notify
will try to bind pci_dev
with acpi devices, but corresponding acpi device is not created yet.
that bind will fail.
So acpi_driver introduce .start in ops to add pci device later.

The patches will hold that acpi_driver for root to be probed for root
to early. aka
make sure all acpi devices come first, then probe driver for acpi device one by.
In that case we don' t need subsystem to introduce .start to hold add
device for next level.

Now HOTPLUG is enabled forcely, and bus_type add notifier help to
control that device
drivers_autoprobe during device hot add.

Thanks

Yinghai

Re: [PATCH 01/10] device: add drivers_autoprobe in struct device

[Bjorn Helgaas]

On Tue, Oct 2, 2012 at 3:47 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Tue, Oct 2, 2012 at 1:45 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:

>> I don't know what you are doing in the acpi code, but I do not think it
>> requires a driver core change like this.
>>
>> Again, why is PCI different from any other bus type?  (hint, it
>> shouldn't be...)
>
> it is not pci susbsystem problem.
>
> the problem come from device-driver core for hotplug handling, and
> acpi system is
> forcing pci system to split the scan and registering.
> aka: to scan all pci device under one pci_root from acpi_pci_root_add.
> then do pci_bus_add_devices for that bus in acpi_pci_root_start.

I don't believe there's anything in ACPI that forces this split
between scanning and registering.

Certainly we have Linux *implementation* issues that currently lead us
to split them, but I don't think those are forced by the hardware or
the spec.  I'd rather address those implementation issues than add
band-aids.

> on hotplug path:
> when acpi enumerating acpi devices, it will create acpi_device for pci_root,
> then probe driver for that device : acpi_pci_root_driver.
> <at that time no acpi_device under that pci_root acpi device is created>
> device_add for root acpi_device will call bus_probe_device and ... and execute
> acpi_pci_root_driver .add aka acpi_pci_root_add.
> acpi_pci_root_add will call pci_acpi_scan_root. that will scan pci
> root bus to find all
> pci devices but does not add those pci device into devices.
> If at the time device_add for pci device get called, platform_notify
> will try to bind pci_dev
> with acpi devices, but corresponding acpi device is not created yet.
> that bind will fail.
> So acpi_driver introduce .start in ops to add pci device later.

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.

I don't know what the right answer is, but I do think it's better to
resolve it inside PCI and ACPI rather than playing games with driver
binding times in the driver core.

Bjorn

Re: [PATCH 01/10] device: add drivers_autoprobe in struct device

[Yinghai Lu]

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

On Tue, Oct 2, 2012 at 3:38 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Tue, Oct 2, 2012 at 3:47 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> On Tue, Oct 2, 2012 at 1:45 PM, Greg Kroah-Hartman
>
> 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.

before introducing this per device .drivers_autoprobe, i did one acpi
to pci bind version
as attached patch.

that mean that will have two version: for booting path: use pci to acpi binding.
and hot plug path will use acpi to pci binding.
Also that make code pretty ugly to consider platform_notifier at the same time.

and acp_get_pci_dev in that acpi_pci_hp_notifier is not very efficient.

-Yinghai

[-- Attachment #2: add_acpi_pci_notifier_xx.patch --]
[-- Type: application/octet-stream, Size: 2926 bytes --]

Subject: [PATCH] PCI, ACPI: Move pci_bus_add_devices into acpi_pci_root_add

That mean acpi device for pci devices are not added for pci root hot add path.

So we need to add acpi notifier to do acpi_pci binding there.

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

---
 drivers/acpi/pci_bind.c |   54 +++++++++++++++++++++++++++++++++++++++++++-----
 drivers/acpi/pci_root.c |    4 +--
 2 files changed, 51 insertions(+), 7 deletions(-)

Index: linux-2.6/drivers/acpi/pci_bind.c
===================================================================
--- linux-2.6.orig/drivers/acpi/pci_bind.c
+++ linux-2.6/drivers/acpi/pci_bind.c
@@ -121,15 +121,14 @@ static int pci_dev_hp_notifier(struct no
 	struct device *dev = data;
 	acpi_handle handle = DEVICE_ACPI_HANDLE(dev);
 	struct acpi_device *acpi_dev;
+	acpi_status status;
 
 	if (!handle)
 		return NOTIFY_OK;
 
-	if (acpi_bus_get_device(handle, &acpi_dev))
-		return NOTIFY_OK;
-
-	if (!acpi_dev)
-		return NOTIFY_OK;
+	status = acpi_bus_get_device(handle, &acpi_dev);
+	if (ACPI_FAILURE(status))
+		acpi_dev = NULL;
 
 	switch (event) {
 	case BUS_NOTIFY_ADD_DEVICE:
@@ -154,3 +153,48 @@ static int __init pci_dev_hp_init(void)
 }
 
 arch_initcall(pci_dev_hp_init);
+
+static int acpi_pci_hp_notifier(struct notifier_block *nb,
+				 unsigned long event, void *data)
+{
+	struct device *adev = data;
+	struct acpi_device *acpi_dev = to_acpi_device(adev);
+	struct pci_dev  *pdev;
+	struct device *dev;
+	int ret;
+
+	/* only need to worry about hot adding path */
+	if (event != BUS_NOTIFY_ADD_DEVICE)
+		return NOTIFY_OK;
+
+	pdev = acpi_get_pci_dev(acpi_dev->handle);
+	if (!pdev)
+		return NOTIFY_OK;
+
+	dev = &pdev->dev;
+	if (DEVICE_ACPI_HANDLE(dev) != acpi_dev->handle)
+		return NOTIFY_OK;
+
+	ret = sysfs_create_link(&dev->kobj, &acpi_dev->dev.kobj,
+                                "firmware_node");
+	ret = sysfs_create_link(&acpi_dev->dev.kobj, &dev->kobj,
+                                "physical_node");
+	if (acpi_dev->wakeup.flags.valid)
+		device_set_wakeup_capable(dev, true);
+
+	acpi_pci_bind_notify(acpi_dev, dev, true);
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block acpi_pci_hp_nb = {
+	.notifier_call = &acpi_pci_hp_notifier,
+};
+
+static int __init acpi_pci_hp_init(void)
+{
+	return bus_register_notifier(&acpi_bus_type,
+					 &acpi_pci_hp_nb);
+}
+
+fs_initcall(acpi_pci_hp_init);
Index: linux-2.6/drivers/acpi/pci_root.c
===================================================================
--- linux-2.6.orig/drivers/acpi/pci_root.c
+++ linux-2.6/drivers/acpi/pci_root.c
@@ -597,6 +597,8 @@ static int __devinit acpi_pci_root_add(s
 		pci_assign_unassigned_bus_resources(root->bus);
 	}
 
+	pci_bus_add_devices(root->bus);
+
 	return 0;
 
 out_del_root:
@@ -612,8 +614,6 @@ static int acpi_pci_root_start(struct ac
 {
 	struct acpi_pci_root *root = acpi_driver_data(device);
 
-	pci_bus_add_devices(root->bus);
-
 	return 0;
 }
 

Re: [PATCH 01/10] device: add drivers_autoprobe in struct device

[Yinghai Lu]

On Tue, Oct 2, 2012 at 3:38 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:

> I don't know what the right answer is, but I do think it's better to
> resolve it inside PCI and ACPI rather than playing games with driver
> binding times in the driver core.

is that ok to save that drivers_autoprobe bit in device.archdata for
ia64 and x86 that using acpi?

Re: [PATCH 01/10] device: add drivers_autoprobe in struct device

[Yinghai Lu]

On Tue, Oct 2, 2012 at 5:00 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Tue, Oct 2, 2012 at 3:38 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>
>> I don't know what the right answer is, but I do think it's better to
>> resolve it inside PCI and ACPI rather than playing games with driver
>> binding times in the driver core.
>
> is that ok to save that drivers_autoprobe bit in device.archdata for
> ia64 and x86 that using acpi?

updated patch to put the bit in pci_dev and acpi_device instead.

please check attached patches.

Yinghai

Re: [PATCH 01/10] device: add drivers_autoprobe in struct device

[Yinghai Lu]

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

On Tue, Oct 2, 2012 at 7:07 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Tue, Oct 2, 2012 at 5:00 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> On Tue, Oct 2, 2012 at 3:38 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>
>>> I don't know what the right answer is, but I do think it's better to
>>> resolve it inside PCI and ACPI rather than playing games with driver
>>> binding times in the driver core.
>>
>> is that ok to save that drivers_autoprobe bit in device.archdata for
>> ia64 and x86 that using acpi?
>
> updated patch to put the bit in pci_dev and acpi_device instead.
>
> please check attached patches.

attached.

[-- Attachment #2: autoprobe_stop_core_acpi_only.patch --]
[-- Type: application/octet-stream, Size: 1767 bytes --]

Subject: [PATCH] ACPI: add drivers_autoprobe in struct acpi_device

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 <yinghai@kernel.org>

---
 drivers/acpi/scan.c     |    8 +++++++-
 include/acpi/acpi_bus.h |    1 +
 2 files changed, 8 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/acpi/scan.c
===================================================================
--- linux-2.6.orig/drivers/acpi/scan.c
+++ linux-2.6/drivers/acpi/scan.c
@@ -333,7 +333,12 @@ static void acpi_device_release(struct d
 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);
 }
@@ -1259,6 +1264,7 @@ static int acpi_add_single_object(struct
 	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);
 
Index: linux-2.6/include/acpi/acpi_bus.h
===================================================================
--- linux-2.6.orig/include/acpi/acpi_bus.h
+++ linux-2.6/include/acpi/acpi_bus.h
@@ -300,6 +300,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)

[-- Attachment #3: autoprobe_stop_core_pci_only.patch --]
[-- Type: application/octet-stream, Size: 1920 bytes --]

Subject: [PATCH] PCI: add drivers_autoprobe in struct pci_dev

To use to control the delay attach driver for pci_dev

Will use bus notifier to toggle this bits when needed.

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

---
 drivers/pci/pci-driver.c |    6 +++++-
 drivers/pci/probe.c      |    1 +
 include/linux/pci.h      |    1 +
 3 files changed, 7 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/pci/pci-driver.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci-driver.c
+++ linux-2.6/drivers/pci/pci-driver.c
@@ -1206,9 +1206,13 @@ pci_dev_driver(const struct pci_dev *dev
 static int pci_bus_match(struct device *dev, struct device_driver *drv)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
-	struct pci_driver *pci_drv = to_pci_driver(drv);
+	struct pci_driver *pci_drv;
 	const struct pci_device_id *found_id;
 
+	if (!pci_dev->drivers_autoprobe)
+		return 0;
+
+	pci_drv = to_pci_driver(drv);
 	found_id = pci_match_device(pci_drv, pci_dev);
 	if (found_id)
 		return 1;
Index: linux-2.6/include/linux/pci.h
===================================================================
--- linux-2.6.orig/include/linux/pci.h
+++ linux-2.6/include/linux/pci.h
@@ -308,6 +308,7 @@ struct pci_dev {
 	unsigned int	irq;
 	struct resource resource[DEVICE_COUNT_RESOURCE]; /* I/O and memory regions + expansion ROMs */
 
+	bool drivers_autoprobe;
 	/* These fields are used by common fixups */
 	unsigned int	transparent:1;	/* Transparent PCI bridge */
 	unsigned int	multifunction:1;/* Part of multi-function device */
Index: linux-2.6/drivers/pci/probe.c
===================================================================
--- linux-2.6.orig/drivers/pci/probe.c
+++ linux-2.6/drivers/pci/probe.c
@@ -1191,6 +1191,7 @@ struct pci_dev *alloc_pci_dev(void)
 		return NULL;
 
 	INIT_LIST_HEAD(&dev->bus_list);
+	dev->drivers_autoprobe = true;
 
 	return dev;
 }

Re: [PATCH 01/10] device: add drivers_autoprobe in struct device

[Bjorn Helgaas]

On Tue, Oct 2, 2012 at 6:00 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Tue, Oct 2, 2012 at 3:38 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>
>> I don't know what the right answer is, but I do think it's better to
>> resolve it inside PCI and ACPI rather than playing games with driver
>> binding times in the driver core.
>
> is that ok to save that drivers_autoprobe bit in device.archdata for
> ia64 and x86 that using acpi?

You'll have to ask Len (cc'd) what he thinks about the ACPI piece, but
I don't like the PCI piece.  It's the same idea that Greg already said
he wouldn't put in the driver core.  Why would it be better to add
this complexity in the ACPI and PCI cores?

I still think you need to work on the ACPI/PCI binding issue.  Yes,
it's hard, but I'm not in favor of avoiding that hard work by adding
kludges everywhere.

Bjorn

Re: [PATCH 01/10] device: add drivers_autoprobe in struct device

[Yinghai Lu]

On Wed, Oct 3, 2012 at 12:48 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>
> You'll have to ask Len (cc'd) what he thinks about the ACPI piece, but
> I don't like the PCI piece.  It's the same idea that Greg already said
> he wouldn't put in the driver core.  Why would it be better to add
> this complexity in the ACPI and PCI cores?

It should be simplify acpi code a little.

I will reorder acpi parts as separate patcheset for review.

> I still think you need to work on the ACPI/PCI binding issue.  Yes,
> it's hard, but I'm not in favor of avoiding that hard work by adding
> kludges everywhere.

acpi pci binding should be solved if len could accept acpi changes.

for pci separate driver attach, that is for registering pci dev to
tree early instead of
waiting till pci_bus_add_devices.
Let me know if you still want us to pursue that.

Otherwise I'd like to move root bus and host bridge registering into
pci_bus_add_devices().

Thanks

Yinghai

[PATCH 0/4] ACPI: kill acpi_pci_root_start

[Yinghai Lu]

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

[PATCH 1/4] ACPI: add drivers_autoprobe in struct acpi_device

[Yinghai Lu]

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 <yinghai@kernel.org>
---
 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

[PATCH 2/4] ACPI: use device drivers_autoprobe to delay loading acpi drivers

[Yinghai Lu]

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 <yinghai@kernel.org>
---
 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

[PATCH 3/4] PCI, ACPI: Remove not used acpi_pci_root_start()

[Yinghai Lu]

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 <yinghai@kernel.org>
---
 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

[PATCH 4/4] ACPI: remove acpi_op_start workaround

[Yinghai Lu]

No .start on any acpi_driver ops anymore.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 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

Re: [PATCH 4/4] ACPI: remove acpi_op_start workaround

[Konrad Rzeszutek Wilk]

On Wed, Oct 3, 2012 at 7:00 PM, Yinghai Lu <yinghai@kernel.org> 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 <yinghai@kernel.org>
> ---
>  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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH 1/4] ACPI: add drivers_autoprobe in struct acpi_device

[Konrad Rzeszutek Wilk]

On Wed, Oct 3, 2012 at 7:00 PM, Yinghai Lu <yinghai@kernel.org> 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 <yinghai@kernel.org>
> ---
>  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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH 1/4] ACPI: add drivers_autoprobe in struct acpi_device

[Yinghai Lu]

On Thu, Oct 4, 2012 at 6:03 AM, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
> On Wed, Oct 3, 2012 at 7:00 PM, Yinghai Lu <yinghai@kernel.org> 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

Re: [PATCH 0/4] ACPI: kill acpi_pci_root_start

[Bjorn Helgaas]

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 <bhelgaas@google.com> 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.

Re: [PATCH 0/4] ACPI: kill acpi_pci_root_start

[Yinghai Lu]

On Thu, Oct 4, 2012 at 10:47 AM, Bjorn Helgaas <bhelgaas@google.com> 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

Re: [PATCH 0/4] ACPI: kill acpi_pci_root_start

[Bjorn Helgaas]

On Thu, Oct 4, 2012 at 12:36 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Thu, Oct 4, 2012 at 10:47 AM, Bjorn Helgaas <bhelgaas@google.com> 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

Re: [PATCH 0/4] ACPI: kill acpi_pci_root_start

[Rafael J. Wysocki]

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 <bhelgaas@google.com> 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.

Re: [PATCH 0/4] ACPI: kill acpi_pci_root_start

[Rafael J. Wysocki]

On Thursday 04 of October 2012 13:44:42 Bjorn Helgaas wrote:
> On Thu, Oct 4, 2012 at 12:36 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> > On Thu, Oct 4, 2012 at 10:47 AM, Bjorn Helgaas <bhelgaas@google.com> 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.

Re: [PATCH 0/4] ACPI: kill acpi_pci_root_start

[Yinghai Lu]

On Thu, Oct 4, 2012 at 12:44 PM, Bjorn Helgaas <bhelgaas@google.com> 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.

Re: [PATCH 0/4] ACPI: kill acpi_pci_root_start

[Bjorn Helgaas]

On Thu, Oct 4, 2012 at 2:14 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Thu, Oct 4, 2012 at 12:44 PM, Bjorn Helgaas <bhelgaas@google.com> 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.

Re: [PATCH 0/4] ACPI: kill acpi_pci_root_start

[Rafael J. Wysocki]

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.

Re: [PATCH 0/4] ACPI: kill acpi_pci_root_start

[Yinghai Lu]

On Thu, Oct 4, 2012 at 2:23 PM, Rafael J. Wysocki <rjw@sisk.pl> 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

Re: [PATCH 0/4] ACPI: kill acpi_pci_root_start

[Rafael J. Wysocki]

On Thursday 04 of October 2012 14:31:46 Yinghai Lu wrote:
> On Thu, Oct 4, 2012 at 2:23 PM, Rafael J. Wysocki <rjw@sisk.pl> 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.

Re: [PATCH 0/4] ACPI: kill acpi_pci_root_start

[Yinghai Lu]

On Thu, Oct 4, 2012 at 2:53 PM, Rafael J. Wysocki <rjw@sisk.pl> 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 <rjw@sisk.pl> 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.

Re: [PATCH 0/4] ACPI: kill acpi_pci_root_start

[Rafael J. Wysocki]

On Thursday 04 of October 2012 15:01:21 Yinghai Lu wrote:
> On Thu, Oct 4, 2012 at 2:53 PM, Rafael J. Wysocki <rjw@sisk.pl> 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 <rjw@sisk.pl> 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.

Re: [PATCH 0/4] ACPI: kill acpi_pci_root_start

[Yinghai Lu]

On Thu, Oct 4, 2012 at 3:36 PM, Rafael J. Wysocki <rjw@sisk.pl> 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?

Re: [PATCH 0/4] ACPI: kill acpi_pci_root_start

[Rafael J. Wysocki]

On Thursday 04 of October 2012 15:46:39 Yinghai Lu wrote:
> On Thu, Oct 4, 2012 at 3:36 PM, Rafael J. Wysocki <rjw@sisk.pl> 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.

Re: [PATCH 0/4] ACPI: kill acpi_pci_root_start

[Yinghai Lu]

On Fri, Oct 5, 2012 at 4:01 PM, Rafael J. Wysocki <rjw@sisk.pl> 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

Re: [PATCH 00/10] PCI, ACPI: Use bus type notifier for root bus hotplug

[Jiang Liu]

Hi Yinghai,
	I like your idea to get rid of acpi_device_ops->start() method, actually
that's on my TODO list too.
	I think the ACPI based system device hotplug framework we are working on
may help to solve this issue. We define CPU, memory, PCI host bridge, IOAPIC,
and CONTAINER as system devices. We have added some callbacks into acpi_device_ops
to support system device hotplug. With the new ACPI system device hotplug framework,
the sequence is:
1) Walk hot-added ACPI subtree, creates acpi devices and binds acpi drivers. Now
acpi_device_ops->add() method only setup basic data structure to manage the ACPI
device but without actually starting the system device.
2) Classify hot-added ACPI devices into classes, such CPU, MEM, PCI HOST BRIGE, IOAPIC,
CONTAINER.
3) Add all hot-added system device into running system in following order:
CONTAINER -> MEM -> CPU -> IOAPIC -> PCI_HOST_BRIDGE.

With above sequence, we could scan and start PCI devices in step 3 above.

We have refined ACPI container and processor driver to support the new framework, and
we have a prototype for pci_root and mem_hotplug drivers too. I will try to post them
as soon as possible.

Thanks
Gerry

1) scan and bind 
On 10/02/2012 02:32 PM, Yinghai Lu wrote:
> Add device drivers_autoprobe to make hotplug path more like booting path:
> create all acpi device and add them to device tree at first
>  then attach acpi drivers.
> create all pci devices and add them to device tree at first
>  then attach pci drivers.
> 
> 1. kill acpi_pci_root_start.
> 2. register pci devices to device tree as soon as possible after pci devices
>    and bus get created.
>    After pci_scan_child_bus, all bus and devices get into devices already.
>    pci_bus_add_devices will only attach driver to pci devices.
> 
> could get from
>         git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git for-pci-next
> 
> Yinghai Lu (10):
>   device: add drivers_autoprobe in struct device
>   ACPI: use device drivers_autoprobe to delay loading acpi drivers
>   PCI: prepare to use device drivers_autoprobe to delay attach drivers
>   PCI: Use device_add for device and bus early
>   PCI, ACPI: Separate out acpi_pci_root_osc_contorl_set
>   PCI, ACPI: Move hot add root bus conf code to acpi_pci_root_add
>   PCI, ACPI: Remove not used acpi_pci_root_start()
>   PCI: Add dev_is_pci_host_bridge() helper
>   PCI, ACPI: using acpi/pci bind path for pci_host_bridge
>   PCI, ACPI: use bus_type notifier for acpi_pci_bind_notify
> 
>  drivers/acpi/glue.c         |    4 -
>  drivers/acpi/pci_bind.c     |   99 +++++++++++++++----
>  drivers/acpi/pci_root.c     |  223 +++++++++++++++++++++++--------------------
>  drivers/acpi/scan.c         |   48 +++++++++-
>  drivers/base/bus.c          |    2 +-
>  drivers/base/core.c         |    1 +
>  drivers/pci/bus.c           |   45 +++------
>  drivers/pci/hotplug.c       |   25 +++++
>  drivers/pci/iov.c           |    7 --
>  drivers/pci/probe.c         |   25 ++++--
>  include/acpi/acpi_drivers.h |    2 +
>  include/linux/device.h      |    1 +
>  include/linux/pci.h         |    1 +
>  13 files changed, 310 insertions(+), 173 deletions(-)
> 

Re: [PATCH 00/10] PCI, ACPI: Use bus type notifier for root bus hotplug

[Yinghai Lu]

On Fri, Oct 5, 2012 at 7:57 PM, Jiang Liu <liuj97@gmail.com> wrote:
> Hi Yinghai,
>         I like your idea to get rid of acpi_device_ops->start() method, actually
> that's on my TODO list too.
>         I think the ACPI based system device hotplug framework we are working on
> may help to solve this issue. We define CPU, memory, PCI host bridge, IOAPIC,
> and CONTAINER as system devices. We have added some callbacks into acpi_device_ops
> to support system device hotplug. With the new ACPI system device hotplug framework,
> the sequence is:
> 1) Walk hot-added ACPI subtree, creates acpi devices and binds acpi drivers. Now
> acpi_device_ops->add() method only setup basic data structure to manage the ACPI
> device but without actually starting the system device.
> 2) Classify hot-added ACPI devices into classes, such CPU, MEM, PCI HOST BRIGE, IOAPIC,
> CONTAINER.
> 3) Add all hot-added system device into running system in following order:
> CONTAINER -> MEM -> CPU -> IOAPIC -> PCI_HOST_BRIDGE.
>
> With above sequence, we could scan and start PCI devices in step 3 above.

ioapic may need to be after pci device scan and pci assign-unassigned resource.

the same as iommu/dmar... need to after pci device scanning...

I have all pci root bus hot plug related patch in my tree

 for-pci-x86-pcibios-alloc-res
 for-pci-host-bridge-bus-type
 for-pci-kill-pci-root-buses

 for-pci-root-bus-hotplug

 for-pci-misc

 for-pci-jiang-hotplug

 for-pci-root-bus-hotadd-survey-pcibios-res
 for-pci-split-pci-root-hp
 for-pci-notifier


 for-acpi-next-pci
 for-pci-next

after those patches (about 61) go through pci/next

will need to push for-x86-irq for ioapic through tip
and for-iommu for iommu support through iommu : david.W

Thanks

Yinghai

Re: [PATCH 00/10] PCI, ACPI: Use bus type notifier for root bus hotplug

[Jiang Liu]

On 10/06/2012 03:25 PM, Yinghai Lu wrote:
> On Fri, Oct 5, 2012 at 7:57 PM, Jiang Liu <liuj97@gmail.com> wrote:
>> Hi Yinghai,
>>         I like your idea to get rid of acpi_device_ops->start() method, actually
>> that's on my TODO list too.
>>         I think the ACPI based system device hotplug framework we are working on
>> may help to solve this issue. We define CPU, memory, PCI host bridge, IOAPIC,
>> and CONTAINER as system devices. We have added some callbacks into acpi_device_ops
>> to support system device hotplug. With the new ACPI system device hotplug framework,
>> the sequence is:
>> 1) Walk hot-added ACPI subtree, creates acpi devices and binds acpi drivers. Now
>> acpi_device_ops->add() method only setup basic data structure to manage the ACPI
>> device but without actually starting the system device.
>> 2) Classify hot-added ACPI devices into classes, such CPU, MEM, PCI HOST BRIGE, IOAPIC,
>> CONTAINER.
>> 3) Add all hot-added system device into running system in following order:
>> CONTAINER -> MEM -> CPU -> IOAPIC -> PCI_HOST_BRIDGE.
>>
>> With above sequence, we could scan and start PCI devices in step 3 above.
> 
> ioapic may need to be after pci device scan and pci assign-unassigned resource.
> 
> the same as iommu/dmar... need to after pci device scanning...
Hi Yinghai,
	I have no experience with AMD IOAPIC yet, seems IOAPIC on AMD platforms
may have the special requirement above. So your solution is better and no need
for the ACPI system device hotplug framework to handle IOAPIC device any more.

> 
> I have all pci root bus hot plug related patch in my tree
> 
>  for-pci-x86-pcibios-alloc-res
>  for-pci-host-bridge-bus-type
>  for-pci-kill-pci-root-buses
> 
>  for-pci-root-bus-hotplug
> 
>  for-pci-misc
> 
>  for-pci-jiang-hotplug
> 
>  for-pci-root-bus-hotadd-survey-pcibios-res
>  for-pci-split-pci-root-hp
>  for-pci-notifier
> 
> 
>  for-acpi-next-pci
>  for-pci-next
> 
> after those patches (about 61) go through pci/next
> 
> will need to push for-x86-irq for ioapic through tip
> and for-iommu for iommu support through iommu : david.W
I have reviewed these branches, all seems great to me. It includes some nice clean
up other than the "PCI host bridge hotplug".
for-pci-split-pci-root-hp
for-pci-root-bus-hotplug
for-pci-misc
for-pci-host-bridge-bus-type
for-pci-jiang-hotplug
for-pci-notifier
for-pci-x86-pcibios-alloc-res
for-pci-root-bus-hotadd-survey-pcibios-res

And we I have several plan based on your work as below, what's your thoughts?
1) replace pci_root_hp.c with our new hotplug framework
2) enhance pci_root.c to support new hotplug framework
3) enable PCI host bridge hotplug for IA64 platforms.
4) try to make PCI host bridge hotplug work without "drivers_autoprobe".

Thanks!

Re: [PATCH 00/10] PCI, ACPI: Use bus type notifier for root bus hotplug

[Yinghai Lu]

On Sun, Oct 7, 2012 at 8:17 AM, Jiang Liu <liuj97@gmail.com> wrote:
> On 10/06/2012 03:25 PM, Yinghai Lu wrote:
>>> With above sequence, we could scan and start PCI devices in step 3 above.
>>
>> ioapic may need to be after pci device scan and pci assign-unassigned resource.
>>
>> the same as iommu/dmar... need to after pci device scanning...
> Hi Yinghai,
>         I have no experience with AMD IOAPIC yet, seems IOAPIC on AMD platforms
> may have the special requirement above. So your solution is better and no need
> for the ACPI system device hotplug framework to handle IOAPIC device any more.
>
>>
>> I have all pci root bus hot plug related patch in my tree
>>
...
>>
>> after those patches (about 61) go through pci/next
>>
>> will need to push for-x86-irq for ioapic through tip
>> and for-iommu for iommu support through iommu : david.W
> I have reviewed these branches, all seems great to me. It includes some nice clean
> up other than the "PCI host bridge hotplug".
> for-pci-split-pci-root-hp
> for-pci-root-bus-hotplug
> for-pci-misc
> for-pci-host-bridge-bus-type
> for-pci-jiang-hotplug
> for-pci-notifier
> for-pci-x86-pcibios-alloc-res
> for-pci-root-bus-hotadd-survey-pcibios-res
>
> And we I have several plan based on your work as below, what's your thoughts?
> 1) replace pci_root_hp.c with our new hotplug framework
> 2) enhance pci_root.c to support new hotplug framework
> 3) enable PCI host bridge hotplug for IA64 platforms.
> 4) try to make PCI host bridge hotplug work without "drivers_autoprobe".

I think my pci-root-bus hotplug should be all done.

need to flush all patches except last 7 patches about drivers_autoprobe.

my concern about your new acpihp framework:
please make sure you change will still make user have the chance not
to use acpiphp
for normal pci slot hotplug support instead of pciehp.

Also I would suggest you to start the new framework stuff after cpu,
memory, pci-root-bus work are all done separately.

Thanks

Yinghai

Re: [PATCH 00/10] PCI, ACPI: Use bus type notifier for root bus hotplug

[Jiang Liu]

On 10/08/2012 06:33 AM, Yinghai Lu wrote:
> On Sun, Oct 7, 2012 at 8:17 AM, Jiang Liu <liuj97@gmail.com> wrote:
>> On 10/06/2012 03:25 PM, Yinghai Lu wrote:
>>>> With above sequence, we could scan and start PCI devices in step 3 above.
>>>
>>> ioapic may need to be after pci device scan and pci assign-unassigned resource.
>>>
>>> the same as iommu/dmar... need to after pci device scanning...
>> Hi Yinghai,
>>         I have no experience with AMD IOAPIC yet, seems IOAPIC on AMD platforms
>> may have the special requirement above. So your solution is better and no need
>> for the ACPI system device hotplug framework to handle IOAPIC device any more.
>>
>>>
>>> I have all pci root bus hot plug related patch in my tree
>>>
> ...
>>>
>>> after those patches (about 61) go through pci/next
>>>
>>> will need to push for-x86-irq for ioapic through tip
>>> and for-iommu for iommu support through iommu : david.W
>> I have reviewed these branches, all seems great to me. It includes some nice clean
>> up other than the "PCI host bridge hotplug".
>> for-pci-split-pci-root-hp
>> for-pci-root-bus-hotplug
>> for-pci-misc
>> for-pci-host-bridge-bus-type
>> for-pci-jiang-hotplug
>> for-pci-notifier
>> for-pci-x86-pcibios-alloc-res
>> for-pci-root-bus-hotadd-survey-pcibios-res
>>
>> And we I have several plan based on your work as below, what's your thoughts?
>> 1) replace pci_root_hp.c with our new hotplug framework
>> 2) enhance pci_root.c to support new hotplug framework
>> 3) enable PCI host bridge hotplug for IA64 platforms.
>> 4) try to make PCI host bridge hotplug work without "drivers_autoprobe".
> 
> I think my pci-root-bus hotplug should be all done.
> 
> need to flush all patches except last 7 patches about drivers_autoprobe.
> 
> my concern about your new acpihp framework:
> please make sure you change will still make user have the chance not
> to use acpiphp
> for normal pci slot hotplug support instead of pciehp.
Hi Yinghai,
	The new hotplug framework only handles CPU, memory, container and PCI host bridges,
so it shouldn't affect acpiphp or pciehp drivers.	

> 
> Also I would suggest you to start the new framework stuff after cpu,
> memory, pci-root-bus work are all done separately.
OK, hope the PCI host bridge hotplug work be merged for 3.8.

Re: [PATCH 0/4] ACPI: kill acpi_pci_root_start

[Rafael J. Wysocki]

On Friday 05 of October 2012 16:10:43 Yinghai Lu wrote:
> On Fri, Oct 5, 2012 at 4:01 PM, Rafael J. Wysocki <rjw@sisk.pl> 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.

Re: [PATCH 1/4] ACPI: add drivers_autoprobe in struct acpi_device

[Konrad Rzeszutek Wilk]

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 <konrad@kernel.org> wrote:
> > On Wed, Oct 3, 2012 at 7:00 PM, Yinghai Lu <yinghai@kernel.org> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>