[PATCH v9 00/11] PCI, ACPI: pci root bus hotplug support / pci match_driver

[PATCH v9 00/11] PCI, ACPI: pci root bus hotplug support / pci match_driver

[Yinghai Lu]

It includes
1. preparing patches for pci root bus hotadd/hotremove support
2. move root bus hotadd from acpiphp to pci_root.c
3. add hot-remove support
4. add acpi_hp_work to be shared with acpiphp and root-bus hotplug
5. add match_driver to add pci device to device tree early but
   not attach driver for hotplug path.

based on pci/next + pm/acpi-scan

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

-v9: merges several patches together for easy review, requested by Rafael.

After this patchset, will send out
         for_each_host_bridge support
         for_each_dev_addon_res

Jiang Liu (2):
  PCI: Fix a device reference count leakage issue in pci_dev_present()
  PCI: make PCI device create/destroy logic symmetric

Tang Chen (1):
  PCI, ACPI: debug print for installation of acpi root bridge's notifier

Yinghai Lu (8):
  PCI, acpiphp: Add is_hotplug_bridge detection
  PCI: Add root bus children dev's res to fail list
  PCI: Set dev_node early for pci_dev
  PCI, ACPI, acpiphp: Rename alloc_acpiphp_hp_work() to alloc_acpi_hp_work
  PCI, acpiphp: Move and enhance hotplug support of pci host bridge
  PCI, acpiphp: Don't bailout even no slots found yet.
  PCI: Add match_driver in struct pci_dev
  PCI: Put pci dev to device tree as early as possible

 drivers/acpi/osl.c                 |   24 +++++-
 drivers/acpi/pci_root.c            |  145 ++++++++++++++++++++++++++++++++++
 drivers/pci/bus.c                  |   57 ++++----------
 drivers/pci/hotplug/acpiphp.h      |    1 -
 drivers/pci/hotplug/acpiphp_core.c |   23 +-----
 drivers/pci/hotplug/acpiphp_glue.c |  150 +++++++++++-------------------------
 drivers/pci/iov.c                  |    7 --
 drivers/pci/pci-driver.c           |    6 +-
 drivers/pci/probe.c                |   36 +++++++--
 drivers/pci/remove.c               |    4 +-
 drivers/pci/search.c               |   10 +--
 drivers/pci/setup-bus.c            |    2 +-
 include/acpi/acpiosxf.h            |    9 ++-
 include/linux/pci.h                |    1 +
 14 files changed, 279 insertions(+), 196 deletions(-)

-- 
1.7.10.4

[PATCH v9 01/11] PCI, acpiphp: Add is_hotplug_bridge detection

[Yinghai Lu]

When system support hotplug bridge with children hotplug slots, we need
to make sure that parent bridge get preallocated resource so later when
device is plugged into children slot, those children devices will get
resource allocated.

We do not meet this problem, because for pcie hotplug card, when acpiphp
is used, pci_scan_bridge will set that for us when detect hotplug bit in
slot cap.

Reported-and-tested-by: Jason Baron <jbaron@redhat.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Acked-by: Jason Baron <jbaron@redhat.com>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/hotplug/acpiphp_glue.c |   27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 22006f2..79db296 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -797,6 +797,29 @@ static void acpiphp_set_acpi_region(struct acpiphp_slot *slot)
 	}
 }
 
+static void check_hotplug_bridge(struct acpiphp_slot *slot, struct pci_dev *dev)
+{
+	struct acpiphp_func *func;
+
+	if (!dev->subordinate)
+		return;
+
+	/* quirk, or pcie could set it already */
+	if (dev->is_hotplug_bridge)
+		return;
+
+	if (PCI_SLOT(dev->devfn) != slot->device)
+		return;
+
+	list_for_each_entry(func, &slot->funcs, sibling) {
+		if (PCI_FUNC(dev->devfn) == func->function) {
+			/* check if this bridge has ejectable slots */
+			if ((detect_ejectable_slots(func->handle) > 0))
+				dev->is_hotplug_bridge = 1;
+			break;
+		}
+	}
+}
 /**
  * enable_device - enable, configure a slot
  * @slot: slot to be enabled
@@ -831,8 +854,10 @@ static int __ref enable_device(struct acpiphp_slot *slot)
 			if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE ||
 			    dev->hdr_type == PCI_HEADER_TYPE_CARDBUS) {
 				max = pci_scan_bridge(bus, dev, max, pass);
-				if (pass && dev->subordinate)
+				if (pass && dev->subordinate) {
+					check_hotplug_bridge(slot, dev);
 					pci_bus_size_bridges(dev->subordinate);
+				}
 			}
 		}
 	}
-- 
1.7.10.4

[PATCH v9 02/11] PCI: Add root bus children dev's res to fail list

[Yinghai Lu]

We can stop trying according to try_number now and do not need to use
root_bus checking as stop sign.

In extreme case we could need to reallocate resource for device just
under root bus. For pci root bus hot-add, we need to retry to assign
resources to pci devices just under pci root bus.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/setup-bus.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 6d3591d..7e8739e 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -283,7 +283,7 @@ static void assign_requested_resources_sorted(struct list_head *head,
 		idx = res - &dev_res->dev->resource[0];
 		if (resource_size(res) &&
 		    pci_assign_resource(dev_res->dev, idx)) {
-			if (fail_head && !pci_is_root_bus(dev_res->dev->bus)) {
+			if (fail_head) {
 				/*
 				 * if the failed res is for ROM BAR, and it will
 				 * be enabled later, don't add it to the list
-- 
1.7.10.4

Re: [PATCH v9 02/11] PCI: Add root bus children dev's res to fail list

[Rafael J. Wysocki]

On Thursday, January 17, 2013 11:53:13 PM Yinghai Lu wrote:
> We can stop trying according to try_number now and do not need to use
> root_bus checking as stop sign.
> 
> In extreme case we could need to reallocate resource for device just
> under root bus. For pci root bus hot-add, we need to retry to assign
> resources to pci devices just under pci root bus.
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/pci/setup-bus.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 6d3591d..7e8739e 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -283,7 +283,7 @@ static void assign_requested_resources_sorted(struct list_head *head,
>  		idx = res - &dev_res->dev->resource[0];
>  		if (resource_size(res) &&
>  		    pci_assign_resource(dev_res->dev, idx)) {
> -			if (fail_head && !pci_is_root_bus(dev_res->dev->bus)) {
> +			if (fail_head) {
>  				/*
>  				 * if the failed res is for ROM BAR, and it will
>  				 * be enabled later, don't add it to the list
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH v9 03/11] PCI: Set dev_node early for pci_dev

[Yinghai Lu]

Otherwise irq_desc for pci bridge with hot-added ioapic can not be on
local node.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/probe.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index bbe4be7..dcef3b9 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1300,6 +1300,7 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
 	dev->dev.release = pci_release_dev;
 	pci_dev_get(dev);
 
+	set_dev_node(&dev->dev, pcibus_to_node(bus));
 	dev->dev.dma_mask = &dev->dma_mask;
 	dev->dev.dma_parms = &dev->dma_parms;
 	dev->dev.coherent_dma_mask = 0xffffffffull;
-- 
1.7.10.4

[PATCH v9 04/11] PCI: Fix a device reference count leakage issue in pci_dev_present()

[Yinghai Lu]

From: Jiang Liu <jiang.liu@huawei.com>

Function pci_get_dev_by_id() will hold a reference count on the pci device
returned, so pci_dev_present() should release the corresponding reference
count to avoid memory leakage.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/search.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/search.c b/drivers/pci/search.c
index bf969ba..d0627fa 100644
--- a/drivers/pci/search.c
+++ b/drivers/pci/search.c
@@ -319,13 +319,13 @@ int pci_dev_present(const struct pci_device_id *ids)
 	WARN_ON(in_interrupt());
 	while (ids->vendor || ids->subvendor || ids->class_mask) {
 		found = pci_get_dev_by_id(ids, NULL);
-		if (found)
-			goto exit;
+		if (found) {
+			pci_dev_put(found);
+			return 1;
+		}
 		ids++;
 	}
-exit:
-	if (found)
-		return 1;
+
 	return 0;
 }
 EXPORT_SYMBOL(pci_dev_present);
-- 
1.7.10.4

[PATCH v9 05/11] PCI: make PCI device create/destroy logic symmetric

[Yinghai Lu]

From: Jiang Liu <jiang.liu@huawei.com>

According to device model documentation, the way to create/destroy PCI
devices should be symmetric.

/**
 * device_del - delete device from system.
 * @dev: device.
 *
 * This is the first part of the device unregistration
 * sequence. This removes the device from the lists we control
 * from here, has it removed from the other driver model
 * subsystems it was added to in device_add(), and removes it
 * from the kobject hierarchy.
 *
 * NOTE: this should be called manually _iff_ device_add() was
 * also called manually.
 */

The rule is to either use
1) device_register()/device_unregister()
or
2) device_initialize()/device_add()/device_del()/put_device().

So change PCI core logic to follow the rule and get rid of the redundant
pci_dev_get()/pci_dev_put() pair.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-by: Yinghai Lu <yinghai@kernel.org>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/probe.c  |    1 -
 drivers/pci/remove.c |    4 ++--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index dcef3b9..9588207 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1298,7 +1298,6 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
 {
 	device_initialize(&dev->dev);
 	dev->dev.release = pci_release_dev;
-	pci_dev_get(dev);
 
 	set_dev_node(&dev->dev, pcibus_to_node(bus));
 	dev->dev.dma_mask = &dev->dma_mask;
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index 7c0fd92..fc38c48 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -22,7 +22,7 @@ static void pci_stop_dev(struct pci_dev *dev)
 	if (dev->is_added) {
 		pci_proc_detach_device(dev);
 		pci_remove_sysfs_dev_files(dev);
-		device_unregister(&dev->dev);
+		device_del(&dev->dev);
 		dev->is_added = 0;
 	}
 
@@ -37,7 +37,7 @@ static void pci_destroy_dev(struct pci_dev *dev)
 	up_write(&pci_bus_sem);
 
 	pci_free_resources(dev);
-	pci_dev_put(dev);
+	put_device(&dev->dev);
 }
 
 void pci_remove_bus(struct pci_bus *bus)
-- 
1.7.10.4

[PATCH v9 06/11] PCI, ACPI, acpiphp: Rename alloc_acpiphp_hp_work() to alloc_acpi_hp_work

[Yinghai Lu]

Will need to use it for pci root bridge hotplug support, rename
*acpiphp* to *acpi* and move to osc.c.
Also make kacpi_hotplug_wq static after that.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: Len Brown <lenb@kernel.org>
Cc: linux-acpi@vger.kernel.org
---
 drivers/acpi/osl.c                 |   24 +++++++++++++++++++--
 drivers/pci/hotplug/acpiphp_glue.c |   42 ++++++------------------------------
 include/acpi/acpiosxf.h            |    9 +++++++-
 3 files changed, 36 insertions(+), 39 deletions(-)

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 3ff2678..afcce46 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -84,8 +84,7 @@ static acpi_osd_handler acpi_irq_handler;
 static void *acpi_irq_context;
 static struct workqueue_struct *kacpid_wq;
 static struct workqueue_struct *kacpi_notify_wq;
-struct workqueue_struct *kacpi_hotplug_wq;
-EXPORT_SYMBOL(kacpi_hotplug_wq);
+static struct workqueue_struct *kacpi_hotplug_wq;
 
 /*
  * This list of permanent mappings is for memory that may be accessed from
@@ -1778,3 +1777,24 @@ void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state,
 {
 	__acpi_os_prepare_sleep = func;
 }
+
+void alloc_acpi_hp_work(acpi_handle handle, u32 type, void *context,
+			void (*func)(struct work_struct *work))
+{
+	struct acpi_hp_work *hp_work;
+	int ret;
+
+	hp_work = kmalloc(sizeof(*hp_work), GFP_KERNEL);
+	if (!hp_work)
+		return;
+
+	hp_work->handle = handle;
+	hp_work->type = type;
+	hp_work->context = context;
+
+	INIT_WORK(&hp_work->work, func);
+	ret = queue_work(kacpi_hotplug_wq, &hp_work->work);
+	if (!ret)
+		kfree(hp_work);
+}
+EXPORT_SYMBOL(alloc_acpi_hp_work);
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 79db296..55e03b6 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -1203,34 +1203,6 @@ check_sub_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
 	return AE_OK ;
 }
 
-struct acpiphp_hp_work {
-	struct work_struct work;
-	acpi_handle handle;
-	u32 type;
-	void *context;
-};
-
-static void alloc_acpiphp_hp_work(acpi_handle handle, u32 type,
-				  void *context,
-				  void (*func)(struct work_struct *work))
-{
-	struct acpiphp_hp_work *hp_work;
-	int ret;
-
-	hp_work = kmalloc(sizeof(*hp_work), GFP_KERNEL);
-	if (!hp_work)
-		return;
-
-	hp_work->handle = handle;
-	hp_work->type = type;
-	hp_work->context = context;
-
-	INIT_WORK(&hp_work->work, func);
-	ret = queue_work(kacpi_hotplug_wq, &hp_work->work);
-	if (!ret)
-		kfree(hp_work);
-}
-
 static void _handle_hotplug_event_bridge(struct work_struct *work)
 {
 	struct acpiphp_bridge *bridge;
@@ -1239,11 +1211,11 @@ static void _handle_hotplug_event_bridge(struct work_struct *work)
 				      .pointer = objname };
 	struct acpi_device *device;
 	int num_sub_bridges = 0;
-	struct acpiphp_hp_work *hp_work;
+	struct acpi_hp_work *hp_work;
 	acpi_handle handle;
 	u32 type;
 
-	hp_work = container_of(work, struct acpiphp_hp_work, work);
+	hp_work = container_of(work, struct acpi_hp_work, work);
 	handle = hp_work->handle;
 	type = hp_work->type;
 
@@ -1346,8 +1318,7 @@ static void handle_hotplug_event_bridge(acpi_handle handle, u32 type,
 	 * For now just re-add this work to the kacpi_hotplug_wq so we
 	 * don't deadlock on hotplug actions.
 	 */
-	alloc_acpiphp_hp_work(handle, type, context,
-			      _handle_hotplug_event_bridge);
+	alloc_acpi_hp_work(handle, type, context, _handle_hotplug_event_bridge);
 }
 
 static void _handle_hotplug_event_func(struct work_struct *work)
@@ -1356,12 +1327,12 @@ static void _handle_hotplug_event_func(struct work_struct *work)
 	char objname[64];
 	struct acpi_buffer buffer = { .length = sizeof(objname),
 				      .pointer = objname };
-	struct acpiphp_hp_work *hp_work;
+	struct acpi_hp_work *hp_work;
 	acpi_handle handle;
 	u32 type;
 	void *context;
 
-	hp_work = container_of(work, struct acpiphp_hp_work, work);
+	hp_work = container_of(work, struct acpi_hp_work, work);
 	handle = hp_work->handle;
 	type = hp_work->type;
 	context = hp_work->context;
@@ -1422,8 +1393,7 @@ static void handle_hotplug_event_func(acpi_handle handle, u32 type,
 	 * For now just re-add this work to the kacpi_hotplug_wq so we
 	 * don't deadlock on hotplug actions.
 	 */
-	alloc_acpiphp_hp_work(handle, type, context,
-			      _handle_hotplug_event_func);
+	alloc_acpi_hp_work(handle, type, context, _handle_hotplug_event_func);
 }
 
 static acpi_status
diff --git a/include/acpi/acpiosxf.h b/include/acpi/acpiosxf.h
index 4315274..adab63c 100644
--- a/include/acpi/acpiosxf.h
+++ b/include/acpi/acpiosxf.h
@@ -193,7 +193,14 @@ void acpi_os_fixed_event_count(u32 fixed_event_number);
 /*
  * Threads and Scheduling
  */
-extern struct workqueue_struct *kacpi_hotplug_wq;
+struct acpi_hp_work {
+	struct work_struct work;
+	acpi_handle handle;
+	u32 type;
+	void *context;
+};
+void alloc_acpi_hp_work(acpi_handle handle, u32 type, void *context,
+			void (*func)(struct work_struct *work));
 
 acpi_thread_id acpi_os_get_thread_id(void);
 
-- 
1.7.10.4

Re: [PATCH v9 06/11] PCI, ACPI, acpiphp: Rename alloc_acpiphp_hp_work() to alloc_acpi_hp_work

[Rafael J. Wysocki]

On Thursday, January 17, 2013 11:53:17 PM Yinghai Lu wrote:
> Will need to use it for pci root bridge hotplug support, rename
> *acpiphp* to *acpi* and move to osc.c.
> Also make kacpi_hotplug_wq static after that.
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> Cc: Len Brown <lenb@kernel.org>
> Cc: linux-acpi@vger.kernel.org
> ---
>  drivers/acpi/osl.c                 |   24 +++++++++++++++++++--
>  drivers/pci/hotplug/acpiphp_glue.c |   42 ++++++------------------------------
>  include/acpi/acpiosxf.h            |    9 +++++++-
>  3 files changed, 36 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 3ff2678..afcce46 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -84,8 +84,7 @@ static acpi_osd_handler acpi_irq_handler;
>  static void *acpi_irq_context;
>  static struct workqueue_struct *kacpid_wq;
>  static struct workqueue_struct *kacpi_notify_wq;
> -struct workqueue_struct *kacpi_hotplug_wq;
> -EXPORT_SYMBOL(kacpi_hotplug_wq);
> +static struct workqueue_struct *kacpi_hotplug_wq;
>  
>  /*
>   * This list of permanent mappings is for memory that may be accessed from
> @@ -1778,3 +1777,24 @@ void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state,
>  {
>  	__acpi_os_prepare_sleep = func;
>  }
> +
> +void alloc_acpi_hp_work(acpi_handle handle, u32 type, void *context,
> +			void (*func)(struct work_struct *work))
> +{
> +	struct acpi_hp_work *hp_work;
> +	int ret;
> +
> +	hp_work = kmalloc(sizeof(*hp_work), GFP_KERNEL);
> +	if (!hp_work)
> +		return;
> +
> +	hp_work->handle = handle;
> +	hp_work->type = type;
> +	hp_work->context = context;
> +
> +	INIT_WORK(&hp_work->work, func);
> +	ret = queue_work(kacpi_hotplug_wq, &hp_work->work);
> +	if (!ret)
> +		kfree(hp_work);
> +}
> +EXPORT_SYMBOL(alloc_acpi_hp_work);

That should be EXPORT_SYMBOL_GPL().

> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index 79db296..55e03b6 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -1203,34 +1203,6 @@ check_sub_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
>  	return AE_OK ;
>  }
>  
> -struct acpiphp_hp_work {
> -	struct work_struct work;
> -	acpi_handle handle;
> -	u32 type;
> -	void *context;
> -};
> -
> -static void alloc_acpiphp_hp_work(acpi_handle handle, u32 type,
> -				  void *context,
> -				  void (*func)(struct work_struct *work))
> -{
> -	struct acpiphp_hp_work *hp_work;
> -	int ret;
> -
> -	hp_work = kmalloc(sizeof(*hp_work), GFP_KERNEL);
> -	if (!hp_work)
> -		return;
> -
> -	hp_work->handle = handle;
> -	hp_work->type = type;
> -	hp_work->context = context;
> -
> -	INIT_WORK(&hp_work->work, func);
> -	ret = queue_work(kacpi_hotplug_wq, &hp_work->work);
> -	if (!ret)
> -		kfree(hp_work);
> -}
> -
>  static void _handle_hotplug_event_bridge(struct work_struct *work)
>  {
>  	struct acpiphp_bridge *bridge;
> @@ -1239,11 +1211,11 @@ static void _handle_hotplug_event_bridge(struct work_struct *work)
>  				      .pointer = objname };
>  	struct acpi_device *device;
>  	int num_sub_bridges = 0;
> -	struct acpiphp_hp_work *hp_work;
> +	struct acpi_hp_work *hp_work;
>  	acpi_handle handle;
>  	u32 type;
>  
> -	hp_work = container_of(work, struct acpiphp_hp_work, work);
> +	hp_work = container_of(work, struct acpi_hp_work, work);
>  	handle = hp_work->handle;
>  	type = hp_work->type;
>  
> @@ -1346,8 +1318,7 @@ static void handle_hotplug_event_bridge(acpi_handle handle, u32 type,
>  	 * For now just re-add this work to the kacpi_hotplug_wq so we
>  	 * don't deadlock on hotplug actions.
>  	 */
> -	alloc_acpiphp_hp_work(handle, type, context,
> -			      _handle_hotplug_event_bridge);
> +	alloc_acpi_hp_work(handle, type, context, _handle_hotplug_event_bridge);
>  }
>  
>  static void _handle_hotplug_event_func(struct work_struct *work)
> @@ -1356,12 +1327,12 @@ static void _handle_hotplug_event_func(struct work_struct *work)
>  	char objname[64];
>  	struct acpi_buffer buffer = { .length = sizeof(objname),
>  				      .pointer = objname };
> -	struct acpiphp_hp_work *hp_work;
> +	struct acpi_hp_work *hp_work;
>  	acpi_handle handle;
>  	u32 type;
>  	void *context;
>  
> -	hp_work = container_of(work, struct acpiphp_hp_work, work);
> +	hp_work = container_of(work, struct acpi_hp_work, work);
>  	handle = hp_work->handle;
>  	type = hp_work->type;
>  	context = hp_work->context;
> @@ -1422,8 +1393,7 @@ static void handle_hotplug_event_func(acpi_handle handle, u32 type,
>  	 * For now just re-add this work to the kacpi_hotplug_wq so we
>  	 * don't deadlock on hotplug actions.
>  	 */
> -	alloc_acpiphp_hp_work(handle, type, context,
> -			      _handle_hotplug_event_func);
> +	alloc_acpi_hp_work(handle, type, context, _handle_hotplug_event_func);
>  }
>  
>  static acpi_status
> diff --git a/include/acpi/acpiosxf.h b/include/acpi/acpiosxf.h
> index 4315274..adab63c 100644
> --- a/include/acpi/acpiosxf.h
> +++ b/include/acpi/acpiosxf.h
> @@ -193,7 +193,14 @@ void acpi_os_fixed_event_count(u32 fixed_event_number);
>  /*
>   * Threads and Scheduling
>   */
> -extern struct workqueue_struct *kacpi_hotplug_wq;

include/acpi/acpiosxf.h is related to ACPICA, so while removing kacpi_hotplug_wq
from it should be OK, please put the rest into acpi_bus.h, preferably next to the
definition of acpi_eject_event.

> +struct acpi_hp_work {
> +	struct work_struct work;
> +	acpi_handle handle;
> +	u32 type;
> +	void *context;
> +};
> +void alloc_acpi_hp_work(acpi_handle handle, u32 type, void *context,
> +			void (*func)(struct work_struct *work));
>  
>  acpi_thread_id acpi_os_get_thread_id(void);

Thanks,
Rafael


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

Re: [PATCH v9 06/11] PCI, ACPI, acpiphp: Rename alloc_acpiphp_hp_work() to alloc_acpi_hp_work

[Yinghai Lu]

On Sun, Jan 20, 2013 at 2:38 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Thursday, January 17, 2013 11:53:17 PM Yinghai Lu wrote:
>> Will need to use it for pci root bridge hotplug support, rename
>> *acpiphp* to *acpi* and move to osc.c.
>> Also make kacpi_hotplug_wq static after that.
>>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> Cc: Len Brown <lenb@kernel.org>
>> Cc: linux-acpi@vger.kernel.org
>> ---
>>  drivers/acpi/osl.c                 |   24 +++++++++++++++++++--
>>  drivers/pci/hotplug/acpiphp_glue.c |   42 ++++++------------------------------
>>  include/acpi/acpiosxf.h            |    9 +++++++-
>>  3 files changed, 36 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
>> index 3ff2678..afcce46 100644
>> --- a/drivers/acpi/osl.c
>> +++ b/drivers/acpi/osl.c
>> @@ -84,8 +84,7 @@ static acpi_osd_handler acpi_irq_handler;
>>  static void *acpi_irq_context;
>>  static struct workqueue_struct *kacpid_wq;
>>  static struct workqueue_struct *kacpi_notify_wq;
>> -struct workqueue_struct *kacpi_hotplug_wq;
>> -EXPORT_SYMBOL(kacpi_hotplug_wq);
>> +static struct workqueue_struct *kacpi_hotplug_wq;
>>
>>  /*
>>   * This list of permanent mappings is for memory that may be accessed from
>> @@ -1778,3 +1777,24 @@ void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state,
>>  {
>>       __acpi_os_prepare_sleep = func;
>>  }
>> +
>> +void alloc_acpi_hp_work(acpi_handle handle, u32 type, void *context,
>> +                     void (*func)(struct work_struct *work))
>> +{
>> +     struct acpi_hp_work *hp_work;
>> +     int ret;
>> +
>> +     hp_work = kmalloc(sizeof(*hp_work), GFP_KERNEL);
>> +     if (!hp_work)
>> +             return;
>> +
>> +     hp_work->handle = handle;
>> +     hp_work->type = type;
>> +     hp_work->context = context;
>> +
>> +     INIT_WORK(&hp_work->work, func);
>> +     ret = queue_work(kacpi_hotplug_wq, &hp_work->work);
>> +     if (!ret)
>> +             kfree(hp_work);
>> +}
>> +EXPORT_SYMBOL(alloc_acpi_hp_work);
>
> That should be EXPORT_SYMBOL_GPL().

Ok.

Chose that because kacpi_hotplug_wq does not have _GPL.

>
...
>> diff --git a/include/acpi/acpiosxf.h b/include/acpi/acpiosxf.h
>> index 4315274..adab63c 100644
>> --- a/include/acpi/acpiosxf.h
>> +++ b/include/acpi/acpiosxf.h
>> @@ -193,7 +193,14 @@ void acpi_os_fixed_event_count(u32 fixed_event_number);
>>  /*
>>   * Threads and Scheduling
>>   */
>> -extern struct workqueue_struct *kacpi_hotplug_wq;
>
> include/acpi/acpiosxf.h is related to ACPICA, so while removing kacpi_hotplug_wq
> from it should be OK, please put the rest into acpi_bus.h, preferably next to the
> definition of acpi_eject_event.
>

Ok.

Thanks

[PATCH v9 07/11] PCI, acpiphp: Move and enhance hotplug support of pci host bridge

[Yinghai Lu]

We have partial hot-add support in acpiphp driver, and it is confusing.

Move host bridge hot-add support to pci_root.c, and keep acpiphp simple,
also add hot-remove support in pci_root.c.

How to test it: if sci_emu patch is applied,

Find out root bus number to acpi root name mapping from dmesg or /sys

  echo "\_SB.PCIB 3" > /sys/kernel/debug/acpi/sci_notify
to remove root bus

  echo "\_SB.PCIB 1" > /sys/kernel/debug/acpi/sci_notify
to add back root bus

-v2: put back pci_root_hp change in one patch
-v3: add pcibios_resource_survey_bus() calling
-v4: remove not needed code with remove_bridge
-v5: put back support for acpiphp support for slots just on root bus.
-v6: change some functions to *_p2p_* to make it more clean.
-v7: split hot_added change out.
-v8: Move to pci_root.c instead of adding another file requested by Bjorn.
-v9: Fold three following patches into this one for easy review:
	a: Add missing hot_remove support for root device.
	b: Tang Chen noticed that hotplug through container will not update
	   acpi_root_bridge list. After closely checking, we don't need
	   that for struct for tracking and could use acpi_pci_root directly.
	c: Tang Chen found handle_root_bridge_removal is very similiar to
	   acpi_bus_hot_remove_device().  Change to handle_root_bridge_removal
	   to use acpi_bus_hot_remove_device.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/acpi/pci_root.c            |  139 ++++++++++++++++++++++++++++++++++++
 drivers/pci/hotplug/acpiphp_glue.c |   59 ++++-----------
 2 files changed, 154 insertions(+), 44 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index bf5108a..3ce5d80 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -655,3 +655,142 @@ int __init acpi_pci_root_init(void)
 
 	return 0;
 }
+
+/* Support root bridge hotplug */
+
+static void handle_root_bridge_insertion(acpi_handle handle)
+{
+	struct acpi_device *device, *pdevice;
+	acpi_handle phandle;
+	int ret_val;
+
+	acpi_get_parent(handle, &phandle);
+	if (acpi_bus_get_device(phandle, &pdevice)) {
+		printk(KERN_DEBUG "no parent device, assuming NULL\n");
+		pdevice = NULL;
+	}
+	if (!acpi_bus_get_device(handle, &device)) {
+		/* check if  pci root_bus is removed */
+		struct acpi_pci_root *root = acpi_driver_data(device);
+		if (pci_find_bus(root->segment, root->secondary.start))
+			return;
+
+		printk(KERN_DEBUG "bus exists... trim\n");
+		/* this shouldn't be in here, so remove
+		 * the bus then re-add it...
+		 */
+		ret_val = acpi_bus_trim(device);
+		printk(KERN_DEBUG "acpi_bus_trim return %x\n", ret_val);
+	}
+	if (acpi_bus_add(handle))
+		printk(KERN_ERR "cannot add bridge to acpi list\n");
+}
+
+static void handle_root_bridge_removal(struct acpi_device *device)
+{
+	struct acpi_eject_event *ej_event;
+
+	ej_event = kmalloc(sizeof(*ej_event), GFP_KERNEL);
+	if (!ej_event)
+		return;
+
+	ej_event->device = device;
+	ej_event->event = ACPI_NOTIFY_EJECT_REQUEST;
+
+	acpi_bus_hot_remove_device(ej_event);
+}
+
+static void _handle_hotplug_event_root(struct work_struct *work)
+{
+	struct acpi_pci_root *root;
+	char objname[64];
+	struct acpi_buffer buffer = { .length = sizeof(objname),
+				      .pointer = objname };
+	struct acpi_hp_work *hp_work;
+	acpi_handle handle;
+	u32 type;
+
+	hp_work = container_of(work, struct acpi_hp_work, work);
+	handle = hp_work->handle;
+	type = hp_work->type;
+
+	root = acpi_pci_find_root(handle);
+
+	acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
+
+	switch (type) {
+	case ACPI_NOTIFY_BUS_CHECK:
+		/* bus enumerate */
+		printk(KERN_DEBUG "%s: Bus check notify on %s\n", __func__,
+				 objname);
+		if (!root)
+			handle_root_bridge_insertion(handle);
+
+		break;
+
+	case ACPI_NOTIFY_DEVICE_CHECK:
+		/* device check */
+		printk(KERN_DEBUG "%s: Device check notify on %s\n", __func__,
+				 objname);
+		if (!root)
+			handle_root_bridge_insertion(handle);
+		break;
+
+	case ACPI_NOTIFY_EJECT_REQUEST:
+		/* request device eject */
+		printk(KERN_DEBUG "%s: Device eject notify on %s\n", __func__,
+				 objname);
+		if (root)
+			handle_root_bridge_removal(root->device);
+		break;
+	default:
+		printk(KERN_WARNING "notify_handler: unknown event type 0x%x for %s\n",
+				 type, objname);
+		break;
+	}
+
+	kfree(hp_work); /* allocated in handle_hotplug_event_bridge */
+}
+
+static void handle_hotplug_event_root(acpi_handle handle, u32 type,
+					void *context)
+{
+	alloc_acpi_hp_work(handle, type, context,
+				_handle_hotplug_event_root);
+}
+
+static acpi_status __init
+find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
+{
+	char objname[64];
+	struct acpi_buffer buffer = { .length = sizeof(objname),
+				      .pointer = objname };
+	int *count = (int *)context;
+
+	if (!acpi_is_root_bridge(handle))
+		return AE_OK;
+
+	(*count)++;
+
+	acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
+
+	acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
+				handle_hotplug_event_root, NULL);
+	printk(KERN_DEBUG "acpi root: %s notify handler installed\n", objname);
+
+	return AE_OK;
+}
+
+static int __init acpi_pci_root_hp_init(void)
+{
+	int num = 0;
+
+	acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
+		ACPI_UINT32_MAX, find_root_bridges, NULL, &num, NULL);
+
+	printk(KERN_DEBUG "Found %d acpi root devices\n", num);
+
+	return 0;
+}
+
+subsys_initcall(acpi_pci_root_hp_init);
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 55e03b6..45917a5 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -543,10 +543,13 @@ static void cleanup_bridge(struct acpiphp_bridge *bridge)
 	acpi_status status;
 	acpi_handle handle = bridge->handle;
 
-	status = acpi_remove_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
+	if (bridge->type != BRIDGE_TYPE_HOST) {
+		status = acpi_remove_notify_handler(handle,
+					    ACPI_SYSTEM_NOTIFY,
 					    handle_hotplug_event_bridge);
-	if (ACPI_FAILURE(status))
-		err("failed to remove notify handler\n");
+		if (ACPI_FAILURE(status))
+			err("failed to remove notify handler\n");
+	}
 
 	if ((bridge->type != BRIDGE_TYPE_HOST) &&
 	    ((bridge->flags & BRIDGE_HAS_EJ0) && bridge->func)) {
@@ -630,9 +633,6 @@ static void remove_bridge(struct acpi_pci_root *root)
 	bridge = acpiphp_handle_to_bridge(handle);
 	if (bridge)
 		cleanup_bridge(bridge);
-	else
-		acpi_remove_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
-					   handle_hotplug_event_bridge);
 }
 
 static int power_on_slot(struct acpiphp_slot *slot)
@@ -1123,18 +1123,12 @@ static void acpiphp_sanitize_bus(struct pci_bus *bus)
 }
 
 /* Program resources in newly inserted bridge */
-static int acpiphp_configure_bridge (acpi_handle handle)
+static int acpiphp_configure_p2p_bridge(acpi_handle handle)
 {
-	struct pci_bus *bus;
+	struct pci_dev *pdev = acpi_get_pci_dev(handle);
+	struct pci_bus *bus = pdev->subordinate;
 
-	if (acpi_is_root_bridge(handle)) {
-		struct acpi_pci_root *root = acpi_pci_find_root(handle);
-		bus = root->bus;
-	} else {
-		struct pci_dev *pdev = acpi_get_pci_dev(handle);
-		bus = pdev->subordinate;
-		pci_dev_put(pdev);
-	}
+	pci_dev_put(pdev);
 
 	pci_bus_size_bridges(bus);
 	pci_bus_assign_resources(bus);
@@ -1144,7 +1138,7 @@ static int acpiphp_configure_bridge (acpi_handle handle)
 	return 0;
 }
 
-static void handle_bridge_insertion(acpi_handle handle, u32 type)
+static void handle_p2p_bridge_insertion(acpi_handle handle, u32 type)
 {
 	struct acpi_device *device;
 
@@ -1162,8 +1156,8 @@ static void handle_bridge_insertion(acpi_handle handle, u32 type)
 		err("ACPI device object missing\n");
 		return;
 	}
-	if (!acpiphp_configure_bridge(handle))
-		add_bridge(handle);
+	if (!acpiphp_configure_p2p_bridge(handle))
+		add_p2p_bridge(handle);
 	else
 		err("cannot configure and start bridge\n");
 
@@ -1221,7 +1215,7 @@ static void _handle_hotplug_event_bridge(struct work_struct *work)
 
 	if (acpi_bus_get_device(handle, &device)) {
 		/* This bridge must have just been physically inserted */
-		handle_bridge_insertion(handle, type);
+		handle_p2p_bridge_insertion(handle, type);
 		goto out;
 	}
 
@@ -1396,21 +1390,6 @@ static void handle_hotplug_event_func(acpi_handle handle, u32 type,
 	alloc_acpi_hp_work(handle, type, context, _handle_hotplug_event_func);
 }
 
-static acpi_status
-find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
-{
-	int *count = (int *)context;
-
-	if (!acpi_is_root_bridge(handle))
-		return AE_OK;
-
-	(*count)++;
-	acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
-				    handle_hotplug_event_bridge, NULL);
-
-	return AE_OK ;
-}
-
 static struct acpi_pci_driver acpi_pci_hp_driver = {
 	.add =		add_bridge,
 	.remove =	remove_bridge,
@@ -1421,15 +1400,7 @@ static struct acpi_pci_driver acpi_pci_hp_driver = {
  */
 int __init acpiphp_glue_init(void)
 {
-	int num = 0;
-
-	acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
-			ACPI_UINT32_MAX, find_root_bridges, NULL, &num, NULL);
-
-	if (num <= 0)
-		return -1;
-	else
-		acpi_pci_register_driver(&acpi_pci_hp_driver);
+	acpi_pci_register_driver(&acpi_pci_hp_driver);
 
 	return 0;
 }
-- 
1.7.10.4

Re: [PATCH v9 07/11] PCI, acpiphp: Move and enhance hotplug support of pci host bridge

[Rafael J. Wysocki]

On Thursday, January 17, 2013 11:53:18 PM Yinghai Lu wrote:
> We have partial hot-add support in acpiphp driver, and it is confusing.
> 
> Move host bridge hot-add support to pci_root.c, and keep acpiphp simple,
> also add hot-remove support in pci_root.c.
> 
> How to test it: if sci_emu patch is applied,
> 
> Find out root bus number to acpi root name mapping from dmesg or /sys
> 
>   echo "\_SB.PCIB 3" > /sys/kernel/debug/acpi/sci_notify
> to remove root bus
> 
>   echo "\_SB.PCIB 1" > /sys/kernel/debug/acpi/sci_notify
> to add back root bus
> 
> -v2: put back pci_root_hp change in one patch
> -v3: add pcibios_resource_survey_bus() calling
> -v4: remove not needed code with remove_bridge
> -v5: put back support for acpiphp support for slots just on root bus.
> -v6: change some functions to *_p2p_* to make it more clean.
> -v7: split hot_added change out.
> -v8: Move to pci_root.c instead of adding another file requested by Bjorn.
> -v9: Fold three following patches into this one for easy review:
> 	a: Add missing hot_remove support for root device.
> 	b: Tang Chen noticed that hotplug through container will not update
> 	   acpi_root_bridge list. After closely checking, we don't need
> 	   that for struct for tracking and could use acpi_pci_root directly.
> 	c: Tang Chen found handle_root_bridge_removal is very similiar to
> 	   acpi_bus_hot_remove_device().  Change to handle_root_bridge_removal
> 	   to use acpi_bus_hot_remove_device.
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> ---
>  drivers/acpi/pci_root.c            |  139 ++++++++++++++++++++++++++++++++++++
>  drivers/pci/hotplug/acpiphp_glue.c |   59 ++++-----------
>  2 files changed, 154 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index bf5108a..3ce5d80 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -655,3 +655,142 @@ int __init acpi_pci_root_init(void)
>  
>  	return 0;
>  }
> +
> +/* Support root bridge hotplug */
> +
> +static void handle_root_bridge_insertion(acpi_handle handle)
> +{
> +	struct acpi_device *device, *pdevice;
> +	acpi_handle phandle;
> +	int ret_val;
> +
> +	acpi_get_parent(handle, &phandle);
> +	if (acpi_bus_get_device(phandle, &pdevice)) {
> +		printk(KERN_DEBUG "no parent device, assuming NULL\n");
> +		pdevice = NULL;
> +	}
> +	if (!acpi_bus_get_device(handle, &device)) {
> +		/* check if  pci root_bus is removed */
> +		struct acpi_pci_root *root = acpi_driver_data(device);
> +		if (pci_find_bus(root->segment, root->secondary.start))
> +			return;
> +
> +		printk(KERN_DEBUG "bus exists... trim\n");
> +		/* this shouldn't be in here, so remove
> +		 * the bus then re-add it...
> +		 */
> +		ret_val = acpi_bus_trim(device);

You said that this followed acpiphp, but the purpose of the trimming in there
seems to be to handle surprise removal and re-insertion, which I'm not sure is
OK with something like a host bridge.

The drawback is that if we have a spurious ACPI_NOTIFY_BUS_CHECK or
ACPI_NOTIFY_DEVICE_CHECK, we'll be trying to remove the whole bus here in
response.  That doesn't sound quite right.

> +		printk(KERN_DEBUG "acpi_bus_trim return %x\n", ret_val);
> +	}
> +	if (acpi_bus_add(handle))
> +		printk(KERN_ERR "cannot add bridge to acpi list\n");
> +}
> +
> +static void handle_root_bridge_removal(struct acpi_device *device)
> +{
> +	struct acpi_eject_event *ej_event;
> +
> +	ej_event = kmalloc(sizeof(*ej_event), GFP_KERNEL);
> +	if (!ej_event)

Shouldn't we do acpi_evaluate_hotplug_ost() here?

> +		return;
> +
> +	ej_event->device = device;
> +	ej_event->event = ACPI_NOTIFY_EJECT_REQUEST;
> +
> +	acpi_bus_hot_remove_device(ej_event);
> +}
> +
> +static void _handle_hotplug_event_root(struct work_struct *work)
> +{
> +	struct acpi_pci_root *root;
> +	char objname[64];
> +	struct acpi_buffer buffer = { .length = sizeof(objname),
> +				      .pointer = objname };
> +	struct acpi_hp_work *hp_work;
> +	acpi_handle handle;
> +	u32 type;
> +
> +	hp_work = container_of(work, struct acpi_hp_work, work);
> +	handle = hp_work->handle;
> +	type = hp_work->type;
> +
> +	root = acpi_pci_find_root(handle);
> +
> +	acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);

Is the path guaranteed not to be longer than 64 characters?

> +
> +	switch (type) {
> +	case ACPI_NOTIFY_BUS_CHECK:
> +		/* bus enumerate */
> +		printk(KERN_DEBUG "%s: Bus check notify on %s\n", __func__,
> +				 objname);

pr_debug() would be nicer (and analogously below).

> +		if (!root)
> +			handle_root_bridge_insertion(handle);
> +
> +		break;
> +
> +	case ACPI_NOTIFY_DEVICE_CHECK:
> +		/* device check */
> +		printk(KERN_DEBUG "%s: Device check notify on %s\n", __func__,
> +				 objname);
> +		if (!root)
> +			handle_root_bridge_insertion(handle);
> +		break;
> +
> +	case ACPI_NOTIFY_EJECT_REQUEST:
> +		/* request device eject */
> +		printk(KERN_DEBUG "%s: Device eject notify on %s\n", __func__,
> +				 objname);
> +		if (root)
> +			handle_root_bridge_removal(root->device);
> +		break;
> +	default:
> +		printk(KERN_WARNING "notify_handler: unknown event type 0x%x for %s\n",
> +				 type, objname);
> +		break;
> +	}
> +
> +	kfree(hp_work); /* allocated in handle_hotplug_event_bridge */
> +}
> +
> +static void handle_hotplug_event_root(acpi_handle handle, u32 type,
> +					void *context)
> +{
> +	alloc_acpi_hp_work(handle, type, context,
> +				_handle_hotplug_event_root);
> +}
> +
> +static acpi_status __init
> +find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
> +{
> +	char objname[64];
> +	struct acpi_buffer buffer = { .length = sizeof(objname),
> +				      .pointer = objname };
> +	int *count = (int *)context;
> +
> +	if (!acpi_is_root_bridge(handle))
> +		return AE_OK;
> +
> +	(*count)++;
> +
> +	acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
> +
> +	acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
> +				handle_hotplug_event_root, NULL);
> +	printk(KERN_DEBUG "acpi root: %s notify handler installed\n", objname);
> +
> +	return AE_OK;
> +}
> +
> +static int __init acpi_pci_root_hp_init(void)
> +{
> +	int num = 0;
> +
> +	acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
> +		ACPI_UINT32_MAX, find_root_bridges, NULL, &num, NULL);
> +
> +	printk(KERN_DEBUG "Found %d acpi root devices\n", num);
> +
> +	return 0;
> +}
> +
> +subsys_initcall(acpi_pci_root_hp_init);

Why do we need a separate initcall here?  Is it not enough to call
acpi_pci_root_hp_init() from acpi_init() or acpi_scan_init(), or
acpi_pci_root_init() even?

> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index 55e03b6..45917a5 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -543,10 +543,13 @@ static void cleanup_bridge(struct acpiphp_bridge *bridge)
>  	acpi_status status;
>  	acpi_handle handle = bridge->handle;
>  
> -	status = acpi_remove_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
> +	if (bridge->type != BRIDGE_TYPE_HOST) {
> +		status = acpi_remove_notify_handler(handle,
> +					    ACPI_SYSTEM_NOTIFY,
>  					    handle_hotplug_event_bridge);
> -	if (ACPI_FAILURE(status))
> -		err("failed to remove notify handler\n");
> +		if (ACPI_FAILURE(status))
> +			err("failed to remove notify handler\n");
> +	}
>  
>  	if ((bridge->type != BRIDGE_TYPE_HOST) &&
>  	    ((bridge->flags & BRIDGE_HAS_EJ0) && bridge->func)) {
> @@ -630,9 +633,6 @@ static void remove_bridge(struct acpi_pci_root *root)
>  	bridge = acpiphp_handle_to_bridge(handle);
>  	if (bridge)
>  		cleanup_bridge(bridge);
> -	else
> -		acpi_remove_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
> -					   handle_hotplug_event_bridge);
>  }
>  
>  static int power_on_slot(struct acpiphp_slot *slot)
> @@ -1123,18 +1123,12 @@ static void acpiphp_sanitize_bus(struct pci_bus *bus)
>  }
>  
>  /* Program resources in newly inserted bridge */
> -static int acpiphp_configure_bridge (acpi_handle handle)
> +static int acpiphp_configure_p2p_bridge(acpi_handle handle)
>  {
> -	struct pci_bus *bus;
> +	struct pci_dev *pdev = acpi_get_pci_dev(handle);
> +	struct pci_bus *bus = pdev->subordinate;
>  
> -	if (acpi_is_root_bridge(handle)) {
> -		struct acpi_pci_root *root = acpi_pci_find_root(handle);
> -		bus = root->bus;
> -	} else {
> -		struct pci_dev *pdev = acpi_get_pci_dev(handle);
> -		bus = pdev->subordinate;
> -		pci_dev_put(pdev);
> -	}
> +	pci_dev_put(pdev);
>  
>  	pci_bus_size_bridges(bus);
>  	pci_bus_assign_resources(bus);
> @@ -1144,7 +1138,7 @@ static int acpiphp_configure_bridge (acpi_handle handle)
>  	return 0;
>  }
>  
> -static void handle_bridge_insertion(acpi_handle handle, u32 type)
> +static void handle_p2p_bridge_insertion(acpi_handle handle, u32 type)
>  {
>  	struct acpi_device *device;
>  
> @@ -1162,8 +1156,8 @@ static void handle_bridge_insertion(acpi_handle handle, u32 type)
>  		err("ACPI device object missing\n");
>  		return;
>  	}
> -	if (!acpiphp_configure_bridge(handle))
> -		add_bridge(handle);
> +	if (!acpiphp_configure_p2p_bridge(handle))
> +		add_p2p_bridge(handle);
>  	else
>  		err("cannot configure and start bridge\n");
>  
> @@ -1221,7 +1215,7 @@ static void _handle_hotplug_event_bridge(struct work_struct *work)
>  
>  	if (acpi_bus_get_device(handle, &device)) {
>  		/* This bridge must have just been physically inserted */
> -		handle_bridge_insertion(handle, type);
> +		handle_p2p_bridge_insertion(handle, type);
>  		goto out;
>  	}
>  
> @@ -1396,21 +1390,6 @@ static void handle_hotplug_event_func(acpi_handle handle, u32 type,
>  	alloc_acpi_hp_work(handle, type, context, _handle_hotplug_event_func);
>  }
>  
> -static acpi_status
> -find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
> -{
> -	int *count = (int *)context;
> -
> -	if (!acpi_is_root_bridge(handle))
> -		return AE_OK;
> -
> -	(*count)++;
> -	acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
> -				    handle_hotplug_event_bridge, NULL);
> -
> -	return AE_OK ;
> -}
> -
>  static struct acpi_pci_driver acpi_pci_hp_driver = {
>  	.add =		add_bridge,
>  	.remove =	remove_bridge,
> @@ -1421,15 +1400,7 @@ static struct acpi_pci_driver acpi_pci_hp_driver = {
>   */
>  int __init acpiphp_glue_init(void)
>  {
> -	int num = 0;
> -
> -	acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
> -			ACPI_UINT32_MAX, find_root_bridges, NULL, &num, NULL);
> -
> -	if (num <= 0)
> -		return -1;
> -	else
> -		acpi_pci_register_driver(&acpi_pci_hp_driver);
> +	acpi_pci_register_driver(&acpi_pci_hp_driver);
>  
>  	return 0;
>  }

Thanks,
Rafael


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

Re: [PATCH v9 07/11] PCI, acpiphp: Move and enhance hotplug support of pci host bridge

[Yinghai Lu]

On Sun, Jan 20, 2013 at 2:55 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Thursday, January 17, 2013 11:53:18 PM Yinghai Lu wrote:
>> We have partial hot-add support in acpiphp driver, and it is confusing.
>>
>> Move host bridge hot-add support to pci_root.c, and keep acpiphp simple,
>> also add hot-remove support in pci_root.c.
>>
>> How to test it: if sci_emu patch is applied,
>>
>> Find out root bus number to acpi root name mapping from dmesg or /sys
>>
>>   echo "\_SB.PCIB 3" > /sys/kernel/debug/acpi/sci_notify
>> to remove root bus
>>
>>   echo "\_SB.PCIB 1" > /sys/kernel/debug/acpi/sci_notify
>> to add back root bus
>>
>> -v2: put back pci_root_hp change in one patch
>> -v3: add pcibios_resource_survey_bus() calling
>> -v4: remove not needed code with remove_bridge
>> -v5: put back support for acpiphp support for slots just on root bus.
>> -v6: change some functions to *_p2p_* to make it more clean.
>> -v7: split hot_added change out.
>> -v8: Move to pci_root.c instead of adding another file requested by Bjorn.
>> -v9: Fold three following patches into this one for easy review:
>>       a: Add missing hot_remove support for root device.
>>       b: Tang Chen noticed that hotplug through container will not update
>>          acpi_root_bridge list. After closely checking, we don't need
>>          that for struct for tracking and could use acpi_pci_root directly.
>>       c: Tang Chen found handle_root_bridge_removal is very similiar to
>>          acpi_bus_hot_remove_device().  Change to handle_root_bridge_removal
>>          to use acpi_bus_hot_remove_device.
>>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> ---
>>  drivers/acpi/pci_root.c            |  139 ++++++++++++++++++++++++++++++++++++
>>  drivers/pci/hotplug/acpiphp_glue.c |   59 ++++-----------
>>  2 files changed, 154 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>> index bf5108a..3ce5d80 100644
>> --- a/drivers/acpi/pci_root.c
>> +++ b/drivers/acpi/pci_root.c
>> @@ -655,3 +655,142 @@ int __init acpi_pci_root_init(void)
>>
>>       return 0;
>>  }
>> +
>> +/* Support root bridge hotplug */
>> +
>> +static void handle_root_bridge_insertion(acpi_handle handle)
>> +{
>> +     struct acpi_device *device, *pdevice;
>> +     acpi_handle phandle;
>> +     int ret_val;
>> +
>> +     acpi_get_parent(handle, &phandle);
>> +     if (acpi_bus_get_device(phandle, &pdevice)) {
>> +             printk(KERN_DEBUG "no parent device, assuming NULL\n");
>> +             pdevice = NULL;
>> +     }
>> +     if (!acpi_bus_get_device(handle, &device)) {
>> +             /* check if  pci root_bus is removed */
>> +             struct acpi_pci_root *root = acpi_driver_data(device);
>> +             if (pci_find_bus(root->segment, root->secondary.start))
>> +                     return;
>> +
>> +             printk(KERN_DEBUG "bus exists... trim\n");
>> +             /* this shouldn't be in here, so remove
>> +              * the bus then re-add it...
>> +              */
>> +             ret_val = acpi_bus_trim(device);
>
> You said that this followed acpiphp, but the purpose of the trimming in there
> seems to be to handle surprise removal and re-insertion, which I'm not sure is
> OK with something like a host bridge.

ok, will just bail out if it is there.

>
> The drawback is that if we have a spurious ACPI_NOTIFY_BUS_CHECK or
> ACPI_NOTIFY_DEVICE_CHECK, we'll be trying to remove the whole bus here in
> response.  That doesn't sound quite right.
>
>> +             printk(KERN_DEBUG "acpi_bus_trim return %x\n", ret_val);
>> +     }
>> +     if (acpi_bus_add(handle))
>> +             printk(KERN_ERR "cannot add bridge to acpi list\n");
>> +}
>> +
>> +static void handle_root_bridge_removal(struct acpi_device *device)
>> +{
>> +     struct acpi_eject_event *ej_event;
>> +
>> +     ej_event = kmalloc(sizeof(*ej_event), GFP_KERNEL);
>> +     if (!ej_event)
>
> Shouldn't we do acpi_evaluate_hotplug_ost() here?

ok. will add

                /* Inform firmware the hot-remove operation has error */
                (void) acpi_evaluate_hotplug_ost(device->handle,
                                        ACPI_NOTIFY_EJECT_REQUEST,
                                        ACPI_OST_SC_NON_SPECIFIC_FAILURE,
                                        NULL);

before return.

>
>> +             return;
>> +
>> +     ej_event->device = device;
>> +     ej_event->event = ACPI_NOTIFY_EJECT_REQUEST;
>> +
>> +     acpi_bus_hot_remove_device(ej_event);
>> +}
>> +
>> +static void _handle_hotplug_event_root(struct work_struct *work)
>> +{
>> +     struct acpi_pci_root *root;
>> +     char objname[64];
>> +     struct acpi_buffer buffer = { .length = sizeof(objname),
>> +                                   .pointer = objname };
>> +     struct acpi_hp_work *hp_work;
>> +     acpi_handle handle;
>> +     u32 type;
>> +
>> +     hp_work = container_of(work, struct acpi_hp_work, work);
>> +     handle = hp_work->handle;
>> +     type = hp_work->type;
>> +
>> +     root = acpi_pci_find_root(handle);
>> +
>> +     acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
>
> Is the path guaranteed not to be longer than 64 characters?

good. will use

struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER };

instead.

>
>> +
>> +     switch (type) {
>> +     case ACPI_NOTIFY_BUS_CHECK:
>> +             /* bus enumerate */
>> +             printk(KERN_DEBUG "%s: Bus check notify on %s\n", __func__,
>> +                              objname);
>
> pr_debug() would be nicer (and analogously below).

maybe we can change that later after it gets stable a bit.

>
>> +             if (!root)
>> +                     handle_root_bridge_insertion(handle);
>> +
>> +             break;
>> +
>> +     case ACPI_NOTIFY_DEVICE_CHECK:
>> +             /* device check */
>> +             printk(KERN_DEBUG "%s: Device check notify on %s\n", __func__,
>> +                              objname);
>> +             if (!root)
>> +                     handle_root_bridge_insertion(handle);
>> +             break;
>> +
>> +     case ACPI_NOTIFY_EJECT_REQUEST:
>> +             /* request device eject */
>> +             printk(KERN_DEBUG "%s: Device eject notify on %s\n", __func__,
>> +                              objname);
>> +             if (root)
>> +                     handle_root_bridge_removal(root->device);
>> +             break;
>> +     default:
>> +             printk(KERN_WARNING "notify_handler: unknown event type 0x%x for %s\n",
>> +                              type, objname);
>> +             break;
>> +     }
>> +
>> +     kfree(hp_work); /* allocated in handle_hotplug_event_bridge */
>> +}
>> +
>> +static void handle_hotplug_event_root(acpi_handle handle, u32 type,
>> +                                     void *context)
>> +{
>> +     alloc_acpi_hp_work(handle, type, context,
>> +                             _handle_hotplug_event_root);
>> +}
>> +
>> +static acpi_status __init
>> +find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
>> +{
>> +     char objname[64];
>> +     struct acpi_buffer buffer = { .length = sizeof(objname),
>> +                                   .pointer = objname };
>> +     int *count = (int *)context;
>> +
>> +     if (!acpi_is_root_bridge(handle))
>> +             return AE_OK;
>> +
>> +     (*count)++;
>> +
>> +     acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
>> +
>> +     acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
>> +                             handle_hotplug_event_root, NULL);
>> +     printk(KERN_DEBUG "acpi root: %s notify handler installed\n", objname);
>> +
>> +     return AE_OK;
>> +}
>> +
>> +static int __init acpi_pci_root_hp_init(void)
>> +{
>> +     int num = 0;
>> +
>> +     acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
>> +             ACPI_UINT32_MAX, find_root_bridges, NULL, &num, NULL);
>> +
>> +     printk(KERN_DEBUG "Found %d acpi root devices\n", num);
>> +
>> +     return 0;
>> +}
>> +
>> +subsys_initcall(acpi_pci_root_hp_init);
>
> Why do we need a separate initcall here?  Is it not enough to call
> acpi_pci_root_hp_init() from acpi_init() or acpi_scan_init(), or
> acpi_pci_root_init() even?

ok, will give it a try.

Thanks

Yinghai

[PATCH v9 08/11] PCI, ACPI: debug print for installation of acpi root bridge's notifier

[Yinghai Lu]

From: Tang Chen <tangchen@cn.fujitsu.com>

acpi_install_notify_handler() could fail. So check the exit status
and give a better debug info.

Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/acpi/pci_root.c |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 3ce5d80..f3ceb61 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -762,6 +762,7 @@ static void handle_hotplug_event_root(acpi_handle handle, u32 type,
 static acpi_status __init
 find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
 {
+	acpi_status status;
 	char objname[64];
 	struct acpi_buffer buffer = { .length = sizeof(objname),
 				      .pointer = objname };
@@ -774,9 +775,14 @@ find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
 
 	acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
 
-	acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
-				handle_hotplug_event_root, NULL);
-	printk(KERN_DEBUG "acpi root: %s notify handler installed\n", objname);
+	status = acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
+					handle_hotplug_event_root, NULL);
+	if (ACPI_FAILURE(status))
+		printk(KERN_DEBUG "acpi root: %s notify handler is not installed, exit status: %u\n",
+				  objname, (unsigned int)status);
+	else
+		printk(KERN_DEBUG "acpi root: %s notify handler is installed\n",
+				 objname);
 
 	return AE_OK;
 }
-- 
1.7.10.4

Re: [PATCH v9 08/11] PCI, ACPI: debug print for installation of acpi root bridge's notifier

[Rafael J. Wysocki]

On Thursday, January 17, 2013 11:53:19 PM Yinghai Lu wrote:
> From: Tang Chen <tangchen@cn.fujitsu.com>
> 
> acpi_install_notify_handler() could fail. So check the exit status
> and give a better debug info.
> 
> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> ---
>  drivers/acpi/pci_root.c |   12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 3ce5d80..f3ceb61 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -762,6 +762,7 @@ static void handle_hotplug_event_root(acpi_handle handle, u32 type,
>  static acpi_status __init
>  find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
>  {
> +	acpi_status status;
>  	char objname[64];
>  	struct acpi_buffer buffer = { .length = sizeof(objname),
>  				      .pointer = objname };
> @@ -774,9 +775,14 @@ find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
>  
>  	acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
>  
> -	acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
> -				handle_hotplug_event_root, NULL);
> -	printk(KERN_DEBUG "acpi root: %s notify handler installed\n", objname);
> +	status = acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
> +					handle_hotplug_event_root, NULL);
> +	if (ACPI_FAILURE(status))
> +		printk(KERN_DEBUG "acpi root: %s notify handler is not installed, exit status: %u\n",

Can you break that line, please?  And use pr_debug()?

> +				  objname, (unsigned int)status);
> +	else
> +		printk(KERN_DEBUG "acpi root: %s notify handler is installed\n",
> +				 objname);
>  
>  	return AE_OK;
>  }

Thanks,
Rafael


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

Re: [PATCH v9 08/11] PCI, ACPI: debug print for installation of acpi root bridge's notifier

[Yinghai Lu]

On Sun, Jan 20, 2013 at 3:00 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Thursday, January 17, 2013 11:53:19 PM Yinghai Lu wrote:
>> From: Tang Chen <tangchen@cn.fujitsu.com>
>>
>> acpi_install_notify_handler() could fail. So check the exit status
>> and give a better debug info.
>>
>> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> ---
>>  drivers/acpi/pci_root.c |   12 +++++++++---
>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>> index 3ce5d80..f3ceb61 100644
>> --- a/drivers/acpi/pci_root.c
>> +++ b/drivers/acpi/pci_root.c
>> @@ -762,6 +762,7 @@ static void handle_hotplug_event_root(acpi_handle handle, u32 type,
>>  static acpi_status __init
>>  find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
>>  {
>> +     acpi_status status;
>>       char objname[64];
>>       struct acpi_buffer buffer = { .length = sizeof(objname),
>>                                     .pointer = objname };
>> @@ -774,9 +775,14 @@ find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
>>
>>       acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
>>
>> -     acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
>> -                             handle_hotplug_event_root, NULL);
>> -     printk(KERN_DEBUG "acpi root: %s notify handler installed\n", objname);
>> +     status = acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
>> +                                     handle_hotplug_event_root, NULL);
>> +     if (ACPI_FAILURE(status))
>> +             printk(KERN_DEBUG "acpi root: %s notify handler is not installed, exit status: %u\n",
>
> Can you break that line, please?  And use pr_debug()?

Long line should be ok, and checkpatch.pl is not complaining about that.

Also keep the complete print out in one line, could make git grep find
that code exactly.

Actually I really hate pr_debug(), that will make the generated code
different with DEBUG
defined or not. And need to end user to recompile kernel to get debug
output if needed.

Thanks

Yinghai

Re: [PATCH v9 08/11] PCI, ACPI: debug print for installation of acpi root bridge's notifier

[Rafael J. Wysocki]

On Sunday, January 20, 2013 06:37:59 PM Yinghai Lu wrote:
> On Sun, Jan 20, 2013 at 3:00 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Thursday, January 17, 2013 11:53:19 PM Yinghai Lu wrote:
> >> From: Tang Chen <tangchen@cn.fujitsu.com>
> >>
> >> acpi_install_notify_handler() could fail. So check the exit status
> >> and give a better debug info.
> >>
> >> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
> >> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> >> ---
> >>  drivers/acpi/pci_root.c |   12 +++++++++---
> >>  1 file changed, 9 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> >> index 3ce5d80..f3ceb61 100644
> >> --- a/drivers/acpi/pci_root.c
> >> +++ b/drivers/acpi/pci_root.c
> >> @@ -762,6 +762,7 @@ static void handle_hotplug_event_root(acpi_handle handle, u32 type,
> >>  static acpi_status __init
> >>  find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
> >>  {
> >> +     acpi_status status;
> >>       char objname[64];
> >>       struct acpi_buffer buffer = { .length = sizeof(objname),
> >>                                     .pointer = objname };
> >> @@ -774,9 +775,14 @@ find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
> >>
> >>       acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
> >>
> >> -     acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
> >> -                             handle_hotplug_event_root, NULL);
> >> -     printk(KERN_DEBUG "acpi root: %s notify handler installed\n", objname);
> >> +     status = acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
> >> +                                     handle_hotplug_event_root, NULL);
> >> +     if (ACPI_FAILURE(status))
> >> +             printk(KERN_DEBUG "acpi root: %s notify handler is not installed, exit status: %u\n",
> >
> > Can you break that line, please?  And use pr_debug()?
> 
> Long line should be ok, and checkpatch.pl is not complaining about that.
> 
> Also keep the complete print out in one line, could make git grep find
> that code exactly.
> 
> Actually I really hate pr_debug(), that will make the generated code
> different with DEBUG
> defined or not. And need to end user to recompile kernel to get debug
> output if needed.

OK, whatever.

Thanks,
Rafael


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

[PATCH v9 09/11] PCI, acpiphp: Don't bailout even no slots found yet.

[Yinghai Lu]

Could have root bus hot added later and there may be slots that need acpiphp.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/hotplug/acpiphp.h      |    1 -
 drivers/pci/hotplug/acpiphp_core.c |   23 ++---------------------
 drivers/pci/hotplug/acpiphp_glue.c |   22 ----------------------
 3 files changed, 2 insertions(+), 44 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp.h b/drivers/pci/hotplug/acpiphp.h
index a1afb5b..b3ead7a 100644
--- a/drivers/pci/hotplug/acpiphp.h
+++ b/drivers/pci/hotplug/acpiphp.h
@@ -193,7 +193,6 @@ extern void acpiphp_unregister_hotplug_slot(struct acpiphp_slot *slot);
 /* acpiphp_glue.c */
 extern int acpiphp_glue_init (void);
 extern void acpiphp_glue_exit (void);
-extern int acpiphp_get_num_slots (void);
 typedef int (*acpiphp_callback)(struct acpiphp_slot *slot, void *data);
 
 extern int acpiphp_enable_slot (struct acpiphp_slot *slot);
diff --git a/drivers/pci/hotplug/acpiphp_core.c b/drivers/pci/hotplug/acpiphp_core.c
index 96316b7..c2fd309 100644
--- a/drivers/pci/hotplug/acpiphp_core.c
+++ b/drivers/pci/hotplug/acpiphp_core.c
@@ -50,7 +50,6 @@
 bool acpiphp_debug;
 
 /* local variables */
-static int num_slots;
 static struct acpiphp_attention_info *attention_info;
 
 #define DRIVER_VERSION	"0.5"
@@ -272,25 +271,6 @@ static int get_adapter_status(struct hotplug_slot *hotplug_slot, u8 *value)
 	return 0;
 }
 
-static int __init init_acpi(void)
-{
-	int retval;
-
-	/* initialize internal data structure etc. */
-	retval = acpiphp_glue_init();
-
-	/* read initial number of slots */
-	if (!retval) {
-		num_slots = acpiphp_get_num_slots();
-		if (num_slots == 0) {
-			acpiphp_glue_exit();
-			retval = -ENODEV;
-		}
-	}
-
-	return retval;
-}
-
 /**
  * release_slot - free up the memory used by a slot
  * @hotplug_slot: slot to free
@@ -379,7 +359,8 @@ static int __init acpiphp_init(void)
 		return 0;
 
 	/* read all the ACPI info from the system */
-	return init_acpi();
+	/* initialize internal data structure etc. */
+	return acpiphp_glue_init();
 }
 
 
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 45917a5..25d6918 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -1416,28 +1416,6 @@ void  acpiphp_glue_exit(void)
 	acpi_pci_unregister_driver(&acpi_pci_hp_driver);
 }
 
-
-/**
- * acpiphp_get_num_slots - count number of slots in a system
- */
-int __init acpiphp_get_num_slots(void)
-{
-	struct acpiphp_bridge *bridge;
-	int num_slots = 0;
-
-	list_for_each_entry(bridge, &bridge_list, list) {
-		dbg("Bus %04x:%02x has %d slot%s\n",
-				pci_domain_nr(bridge->pci_bus),
-				bridge->pci_bus->number, bridge->nr_slots,
-				bridge->nr_slots == 1 ? "" : "s");
-		num_slots += bridge->nr_slots;
-	}
-
-	dbg("Total %d slots\n", num_slots);
-	return num_slots;
-}
-
-
 /**
  * acpiphp_enable_slot - power on slot
  * @slot: ACPI PHP slot
-- 
1.7.10.4

Re: [PATCH v9 09/11] PCI, acpiphp: Don't bailout even no slots found yet.

[Rafael J. Wysocki]

On Thursday, January 17, 2013 11:53:20 PM Yinghai Lu wrote:
> Could have root bus hot added later and there may be slots that need acpiphp.

The patch implies that acpiphp_get_num_slots() is not necessary any more,
but the changelog above doesn't really explain why that is so.

Do I guess correctly that the bridge the slots are under may not be in
bridge_list yet at the moment and so we don't really know whether or not
we will need acpiphp_glue going forward?

If that's the case:

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

but please say something like this in the changelog:

"The result returned by acpiphp_get_num_slots() is meaningless, because
 the bridge the slots are under may be added after this function has been
 called, so drop acpiphp_get_num_slots() and the code using it."

> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> ---
>  drivers/pci/hotplug/acpiphp.h      |    1 -
>  drivers/pci/hotplug/acpiphp_core.c |   23 ++---------------------
>  drivers/pci/hotplug/acpiphp_glue.c |   22 ----------------------
>  3 files changed, 2 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/acpiphp.h b/drivers/pci/hotplug/acpiphp.h
> index a1afb5b..b3ead7a 100644
> --- a/drivers/pci/hotplug/acpiphp.h
> +++ b/drivers/pci/hotplug/acpiphp.h
> @@ -193,7 +193,6 @@ extern void acpiphp_unregister_hotplug_slot(struct acpiphp_slot *slot);
>  /* acpiphp_glue.c */
>  extern int acpiphp_glue_init (void);
>  extern void acpiphp_glue_exit (void);
> -extern int acpiphp_get_num_slots (void);
>  typedef int (*acpiphp_callback)(struct acpiphp_slot *slot, void *data);
>  
>  extern int acpiphp_enable_slot (struct acpiphp_slot *slot);
> diff --git a/drivers/pci/hotplug/acpiphp_core.c b/drivers/pci/hotplug/acpiphp_core.c
> index 96316b7..c2fd309 100644
> --- a/drivers/pci/hotplug/acpiphp_core.c
> +++ b/drivers/pci/hotplug/acpiphp_core.c
> @@ -50,7 +50,6 @@
>  bool acpiphp_debug;
>  
>  /* local variables */
> -static int num_slots;
>  static struct acpiphp_attention_info *attention_info;
>  
>  #define DRIVER_VERSION	"0.5"
> @@ -272,25 +271,6 @@ static int get_adapter_status(struct hotplug_slot *hotplug_slot, u8 *value)
>  	return 0;
>  }
>  
> -static int __init init_acpi(void)
> -{
> -	int retval;
> -
> -	/* initialize internal data structure etc. */
> -	retval = acpiphp_glue_init();
> -
> -	/* read initial number of slots */
> -	if (!retval) {
> -		num_slots = acpiphp_get_num_slots();
> -		if (num_slots == 0) {
> -			acpiphp_glue_exit();
> -			retval = -ENODEV;
> -		}
> -	}
> -
> -	return retval;
> -}
> -
>  /**
>   * release_slot - free up the memory used by a slot
>   * @hotplug_slot: slot to free
> @@ -379,7 +359,8 @@ static int __init acpiphp_init(void)
>  		return 0;
>  
>  	/* read all the ACPI info from the system */
> -	return init_acpi();
> +	/* initialize internal data structure etc. */
> +	return acpiphp_glue_init();
>  }
>  
>  
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index 45917a5..25d6918 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -1416,28 +1416,6 @@ void  acpiphp_glue_exit(void)
>  	acpi_pci_unregister_driver(&acpi_pci_hp_driver);
>  }
>  
> -
> -/**
> - * acpiphp_get_num_slots - count number of slots in a system
> - */
> -int __init acpiphp_get_num_slots(void)
> -{
> -	struct acpiphp_bridge *bridge;
> -	int num_slots = 0;
> -
> -	list_for_each_entry(bridge, &bridge_list, list) {
> -		dbg("Bus %04x:%02x has %d slot%s\n",
> -				pci_domain_nr(bridge->pci_bus),
> -				bridge->pci_bus->number, bridge->nr_slots,
> -				bridge->nr_slots == 1 ? "" : "s");
> -		num_slots += bridge->nr_slots;
> -	}
> -
> -	dbg("Total %d slots\n", num_slots);
> -	return num_slots;
> -}
> -
> -
>  /**
>   * acpiphp_enable_slot - power on slot
>   * @slot: ACPI PHP slot
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v9 09/11] PCI, acpiphp: Don't bailout even no slots found yet.

[Yinghai Lu]

>
> If that's the case:
>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> but please say something like this in the changelog:
>
> "The result returned by acpiphp_get_num_slots() is meaningless, because
>  the bridge the slots are under may be added after this function has been
>  called, so drop acpiphp_get_num_slots() and the code using it."

yes, I add you inputs into change log.

Thanks a lot

[PATCH v9 10/11] PCI: Add match_driver in struct pci_dev

[Yinghai Lu]

with that we could move out attaching driver for pci device,
out of device_add for pci hot add path.

pci_bus_attach_device() will attach driver to pci device.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/bus.c        |   10 ++++++++++
 drivers/pci/pci-driver.c |    6 +++++-
 include/linux/pci.h      |    1 +
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 847f3ca..18c1c6d 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -160,6 +160,15 @@ 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;
+
+	dev->match_driver = true;
+	ret = device_attach(&dev->dev);
+	WARN_ON(ret < 0);
+}
+
 /**
  * pci_bus_add_device - add a single device
  * @dev: device to add
@@ -181,6 +190,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/pci-driver.c b/drivers/pci/pci-driver.c
index f79cbcd..acdcc3c 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -1186,9 +1186,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->match_driver)
+		return 0;
+
+	pci_drv = to_pci_driver(drv);
 	found_id = pci_match_device(pci_drv, pci_dev);
 	if (found_id)
 		return 1;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 6860f4d..e2aed11 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -286,6 +286,7 @@ struct pci_dev {
 	unsigned int	irq;
 	struct resource resource[DEVICE_COUNT_RESOURCE]; /* I/O and memory regions + expansion ROMs */
 
+	bool match_driver;
 	/* These fields are used by common fixups */
 	unsigned int	transparent:1;	/* Transparent PCI bridge */
 	unsigned int	multifunction:1;/* Part of multi-function device */
-- 
1.7.10.4

Re: [PATCH v9 10/11] PCI: Add match_driver in struct pci_dev

[Rafael J. Wysocki]

On Thursday, January 17, 2013 11:53:21 PM Yinghai Lu wrote:
> with that we could move out attaching driver for pci device,
> out of device_add for pci hot add path.
> 
> pci_bus_attach_device() will attach driver to pci device.

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

for the code, but you still aren't saying in the changelog why the change
is needed.

Thanks,
Rafael


> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> ---
>  drivers/pci/bus.c        |   10 ++++++++++
>  drivers/pci/pci-driver.c |    6 +++++-
>  include/linux/pci.h      |    1 +
>  3 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index 847f3ca..18c1c6d 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -160,6 +160,15 @@ 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;
> +
> +	dev->match_driver = true;
> +	ret = device_attach(&dev->dev);
> +	WARN_ON(ret < 0);
> +}
> +
>  /**
>   * pci_bus_add_device - add a single device
>   * @dev: device to add
> @@ -181,6 +190,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/pci-driver.c b/drivers/pci/pci-driver.c
> index f79cbcd..acdcc3c 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -1186,9 +1186,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->match_driver)
> +		return 0;
> +
> +	pci_drv = to_pci_driver(drv);
>  	found_id = pci_match_device(pci_drv, pci_dev);
>  	if (found_id)
>  		return 1;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 6860f4d..e2aed11 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -286,6 +286,7 @@ struct pci_dev {
>  	unsigned int	irq;
>  	struct resource resource[DEVICE_COUNT_RESOURCE]; /* I/O and memory regions + expansion ROMs */
>  
> +	bool match_driver;
>  	/* These fields are used by common fixups */
>  	unsigned int	transparent:1;	/* Transparent PCI bridge */
>  	unsigned int	multifunction:1;/* Part of multi-function device */
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v9 10/11] PCI: Add match_driver in struct pci_dev

[Yinghai Lu]

On Sun, Jan 20, 2013 at 3:15 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Thursday, January 17, 2013 11:53:21 PM Yinghai Lu wrote:
>> with that we could move out attaching driver for pci device,
>> out of device_add for pci hot add path.
>>
>> pci_bus_attach_device() will attach driver to pci device.
>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> for the code, but you still aren't saying in the changelog why the change
> is needed.

Thanks.

Please check if the changelog is good to you.

---
Subject: [PATCH] PCI: Skip attaching driver in device_add()

We want to add pci device to device tree as early as possible but
delay attach driver in next following path.

To make that patch smaller, in this patch:

We add match_driver field in pci_dev and default vaule is false, it will
make pci_bus_match fail, so device_add  will skip attaching driver,
then pci_bus_attach_device() will set match_driver to true so
pci_bus_match will return true and device_attach will attach driver
to pci device.

---

Re: [PATCH v9 10/11] PCI: Add match_driver in struct pci_dev

[Rafael J. Wysocki]

On Sunday, January 20, 2013 09:22:04 PM Yinghai Lu wrote:
> On Sun, Jan 20, 2013 at 3:15 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Thursday, January 17, 2013 11:53:21 PM Yinghai Lu wrote:
> >> with that we could move out attaching driver for pci device,
> >> out of device_add for pci hot add path.
> >>
> >> pci_bus_attach_device() will attach driver to pci device.
> >
> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > for the code, but you still aren't saying in the changelog why the change
> > is needed.
> 
> Thanks.
> 
> Please check if the changelog is good to you.

It's fine by me.

Thanks,
Rafael


> ---
> Subject: [PATCH] PCI: Skip attaching driver in device_add()
> 
> We want to add pci device to device tree as early as possible but
> delay attach driver in next following path.
> 
> To make that patch smaller, in this patch:
> 
> We add match_driver field in pci_dev and default vaule is false, it will
> make pci_bus_match fail, so device_add  will skip attaching driver,
> then pci_bus_attach_device() will set match_driver to true so
> pci_bus_match will return true and device_attach will attach driver
> to pci device.
> 
> ---
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH v9 11/11] PCI: Put pci dev to device tree as early as possible

[Yinghai Lu]

We want to put created pci device in the device tree as soon as possible.
- just after we find it and create pci_dev struct for it.
so for_pci_dev iteration will not miss them.

But at that time, we can not load driver for them yet. Need to be after
pci_assign_unsigned_resources() etc to make sure all pci devices get
resource allocated at first.

Move out device registering out of pci_bus_add_devices, and
new pci_bus_add_devices() will do the device_attach work to load pci drivers

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

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 18c1c6d..0a55845 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -178,22 +178,9 @@ 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 = pcibios_add_device(dev);
-	if (retval)
-		return retval;
-
-	retval = device_add(&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);
+
 	return 0;
 }
 
@@ -205,21 +192,9 @@ 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_register(&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;
 }
 
 /**
@@ -245,36 +220,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 bafd2bb..dbad72e 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 9588207..6392dac 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -623,6 +623,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..
@@ -637,8 +638,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);
@@ -651,11 +651,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);
 
@@ -666,6 +669,13 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
 	}
 	bridge->subordinate = child;
 
+add_dev:
+	ret = device_register(&child->dev);
+	WARN_ON(ret < 0);
+
+	/* Create legacy_io and legacy_mem files for this bus */
+	pci_create_legacy_files(child);
+
 	return child;
 }
 
@@ -1296,6 +1306,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;
 
@@ -1326,6 +1338,16 @@ 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);
+
+	pci_fixup_device(pci_fixup_final, dev);
+	ret = pcibios_add_device(dev);
+	WARN_ON(ret < 0);
+	/* notifier could use pci capabilities */
+	ret = device_add(&dev->dev);
+	WARN_ON(ret < 0);
+
+	pci_proc_attach_device(dev);
+	pci_create_sysfs_dev_files(dev);
 }
 
 struct pci_dev *__ref pci_scan_single_device(struct pci_bus *bus, int devfn)
@@ -1656,13 +1678,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 */
@@ -1701,8 +1723,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.10.4

Re: [PATCH v9 11/11] PCI: Put pci dev to device tree as early as possible

[Rafael J. Wysocki]

On Thursday, January 17, 2013 11:53:22 PM Yinghai Lu wrote:
> We want to put created pci device in the device tree as soon as possible.
> - just after we find it and create pci_dev struct for it.
> so for_pci_dev iteration will not miss them.
> 
> But at that time, we can not load driver for them yet. Need to be after
> pci_assign_unsigned_resources() etc to make sure all pci devices get
> resource allocated at first.
> 
> Move out device registering out of pci_bus_add_devices, and
> new pci_bus_add_devices() will do the device_attach work to load pci drivers
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> ---
>  drivers/pci/bus.c   |   47 +++--------------------------------------------
>  drivers/pci/iov.c   |    7 -------
>  drivers/pci/probe.c |   34 +++++++++++++++++++++++++++-------
>  3 files changed, 30 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index 18c1c6d..0a55845 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -178,22 +178,9 @@ 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 = pcibios_add_device(dev);
> -	if (retval)
> -		return retval;
> -
> -	retval = device_add(&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);
> +
>  	return 0;
>  }
>  
> @@ -205,21 +192,9 @@ 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_register(&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;
>  }

Well, what sense does this make to keep that function as is after removing
almost all of the code from it?

>  
>  /**
> @@ -245,36 +220,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);
> -		}

This doesn't seem to have a replacement.  Why isn't it necessary any more?

>  		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 bafd2bb..dbad72e 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 9588207..6392dac 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -623,6 +623,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..
> @@ -637,8 +638,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);
> @@ -651,11 +651,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);
>  
> @@ -666,6 +669,13 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
>  	}
>  	bridge->subordinate = child;
>  
> +add_dev:
> +	ret = device_register(&child->dev);
> +	WARN_ON(ret < 0);
> +
> +	/* Create legacy_io and legacy_mem files for this bus */
> +	pci_create_legacy_files(child);
> +
>  	return child;
>  }
>  
> @@ -1296,6 +1306,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;
>  
> @@ -1326,6 +1338,16 @@ 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);
> +
> +	pci_fixup_device(pci_fixup_final, dev);
> +	ret = pcibios_add_device(dev);
> +	WARN_ON(ret < 0);
> +	/* notifier could use pci capabilities */
> +	ret = device_add(&dev->dev);
> +	WARN_ON(ret < 0);
> +
> +	pci_proc_attach_device(dev);
> +	pci_create_sysfs_dev_files(dev);
>  }
>  
>  struct pci_dev *__ref pci_scan_single_device(struct pci_bus *bus, int devfn)
> @@ -1656,13 +1678,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 */
> @@ -1701,8 +1723,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
> 

Thanks,
Rafael


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

Re: [PATCH v9 11/11] PCI: Put pci dev to device tree as early as possible

[Yinghai Lu]

On Sun, Jan 20, 2013 at 3:23 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Thursday, January 17, 2013 11:53:22 PM Yinghai Lu wrote:
>> We want to put created pci device in the device tree as soon as possible.
>> - just after we find it and create pci_dev struct for it.
>> so for_pci_dev iteration will not miss them.
>>
>> But at that time, we can not load driver for them yet. Need to be after
>> pci_assign_unsigned_resources() etc to make sure all pci devices get
>> resource allocated at first.
>>
>> Move out device registering out of pci_bus_add_devices, and
>> new pci_bus_add_devices() will do the device_attach work to load pci drivers
>>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> ---
>>  drivers/pci/bus.c   |   47 +++--------------------------------------------
>>  drivers/pci/iov.c   |    7 -------
>>  drivers/pci/probe.c |   34 +++++++++++++++++++++++++++-------
>>  3 files changed, 30 insertions(+), 58 deletions(-)
>>
>> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
>> index 18c1c6d..0a55845 100644
>> --- a/drivers/pci/bus.c
>> +++ b/drivers/pci/bus.c
>> @@ -178,22 +178,9 @@ static void pci_bus_attach_device(struct pci_dev *dev)
>>   */
..
>> @@ -205,21 +192,9 @@ 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_register(&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;
>>  }
>
> Well, what sense does this make to keep that function as is after removing
> almost all of the code from it?

ok, will remove that function.

...
>>       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);
>> -             }
>
> This doesn't seem to have a replacement.  Why isn't it necessary any more?
>

add that in changelog, so related changlog will be:

---
Also remove unattached child bus handling in pci_bus_add_devices().
Because that is not needed, child bus via pci_add_new_bus() is already
in parent bus children list.
---