[PATCH v8 00/13] Get rid of the ACPI PCI subdriver mechanism

[PATCH v8 00/13] Get rid of the ACPI PCI subdriver mechanism

[Jiang Liu]

PCI, ACPI: remove ACPI PCI subdriver mechanism

This patch set is directly derived from two sources:
1) '[PATCH 00/15] PCI/ACPI: Remove "pci_root" sub-driver support' at
https://lkml.org/lkml/2012/12/7/11 from Myron Stowe
The major goal is to resolve any potential sequencing inter-dependencies
by converting sub-driver functionality to being only supported as
statically built-in to the kernel as part of the "pci_root" driver
itself.

2) 'introduce PCI bus notifier chain to get rid of the ACPI PCI subdriver'
at http://lwn.net/Articles/533547/
The major goal is to update PCI slots(pci_slot), ACPI based PCI hotplug
slots(acpiphp), PCIe AER(aer) etc when hot-plugging PCI devices and P2P
bridges, and eventually get rid of the ACPI PCI subdriver interfaces.

This patchset applies to Bjorn's pci-next branch. Patch 1-4 are minor
fixups for 3.9-rc1. Patch 5-8 introduce two hooks into PCI core which
will be called when creating/destroying PCI buses. Patch 9-12 replace
ACPI PCI subdriver interfaces with the new hook mechanism for pci_slot
and acpiphp. Patch 13 removes the ACPI PCI subdriver interfaces.

v7->v8:
	Hook directly into the PCI core to replace pci_bus notification
	Remove ACPI PCI subdriver related code

Jiang Liu (10):
  PCI: do not check is_added flag in pci_remove_bus()
  PCI/acpiphp: don't rely on function 0 in disable_device()
  ACPI/acpiphp: replace local macros with standard ACPI macros
  PCI: introduce platform dependent hooks for creating/destroying PCI
    busses
  PCI, ACPI: prepare stub functions to handle ACPI PCI (hotplug) slots
  PCI, IA64: implement pcibios_{add|remove}_bus() hooks
  PCI, x86: implement pcibios_{add|remove}_bus() hooks
  PCI, ACPI: handle PCI slot devices when creating/destroying PCI
    busses
  PCI/acpiphp: do not use ACPI PCI subdriver mechanism
  PCI/acpiphp: protect acpiphp data structures from concurrent updating

Myron Stowe (1):
  PCI, ACPI: remove support of ACPI PCI subdrivers

Yijing Wang (2):
  PCI/acpiphp: use list_for_each_entry_safe() in acpiphp_sanitize_bus()
  PCI/acpiphp: use normal list to simplify implementation

 arch/ia64/pci/pci.c                |   11 +
 arch/x86/pci/common.c              |   11 +
 drivers/acpi/internal.h            |    5 -
 drivers/acpi/pci_root.c            |   48 +----
 drivers/acpi/pci_slot.c            |  177 +++-------------
 drivers/acpi/scan.c                |    1 -
 drivers/pci/bus.c                  |   11 +-
 drivers/pci/hotplug/Kconfig        |    7 +-
 drivers/pci/hotplug/acpiphp.h      |   12 +-
 drivers/pci/hotplug/acpiphp_core.c |   23 +-
 drivers/pci/hotplug/acpiphp_glue.c |  409 ++++++++++++------------------------
 drivers/pci/pci-acpi.c             |   30 +++
 drivers/pci/probe.c                |   15 +-
 drivers/pci/remove.c               |    4 +-
 include/linux/acpi.h               |    9 -
 include/linux/pci-acpi.h           |   26 +++
 include/linux/pci.h                |    2 +
 17 files changed, 268 insertions(+), 533 deletions(-)

-- 
1.7.9.5

[PATCH v8 01/13] PCI: do not check is_added flag in pci_remove_bus()

[Jiang Liu]

The is_added flag in struct pci_bus is always set after invoking
device_register() and pci_create_legacy_files(). The related code
paths are:
1) pci_create_root_bus() and pci_add_new_bus()
	bus = pci_alloc_bus()
	device_register(&bus->dev)
	pci_create_legacy_file(bus)
2.a) pci_scan_child_bus(bus)
	set is_added flag for PCI root buses
2.b) pci_bus_add_devices(bus)
	set is_added flag for normal PCI buses

So pci_remove_bus() shouldn't use is_added flag to guard invoking of
pci_remove_lagecy_files() and device_unregister().

One more step further, now bus->is_added is only used to guard invoking
of pcibios_fixup_bus(), so cleaup the code.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Cc: Toshi Kani <toshi.kani@hp.com>
Cc: linux-pci@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/pci/bus.c    |   11 ++---------
 drivers/pci/probe.c  |    3 +--
 drivers/pci/remove.c |    3 ---
 3 files changed, 3 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 8647dc6..bdc1e8b 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -206,16 +206,9 @@ void pci_bus_add_devices(const struct pci_bus *bus)
 
 	list_for_each_entry(dev, &bus->devices, bus_list) {
 		BUG_ON(!dev->is_added);
-
 		child = dev->subordinate;
-
-		if (!child)
-			continue;
-		pci_bus_add_devices(child);
-
-		if (child->is_added)
-			continue;
-		child->is_added = 1;
+		if (child)
+			pci_bus_add_devices(child);
 	}
 }
 
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index b494066..45c93b3 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1627,8 +1627,7 @@ unsigned int pci_scan_child_bus(struct pci_bus *bus)
 	if (!bus->is_added) {
 		dev_dbg(&bus->dev, "fixups for bus\n");
 		pcibios_fixup_bus(bus);
-		if (pci_is_root_bus(bus))
-			bus->is_added = 1;
+		bus->is_added = 1;
 	}
 
 	for (pass=0; pass < 2; pass++)
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index fc38c48..66967e5 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -48,9 +48,6 @@ void pci_remove_bus(struct pci_bus *bus)
 	list_del(&bus->node);
 	pci_bus_release_busn_res(bus);
 	up_write(&pci_bus_sem);
-	if (!bus->is_added)
-		return;
-
 	pci_remove_legacy_files(bus);
 	device_unregister(&bus->dev);
 }
-- 
1.7.9.5

[PATCH v8 02/13] PCI/acpiphp: use list_for_each_entry_safe() in acpiphp_sanitize_bus()

[Jiang Liu]

From: Yijing Wang <wangyijing@huawei.com>

Function acpiphp_sanitize_bus() may call pci_stop_and_remove_bus_device(),
which in turn may remove device from bus->devices list. So walk the
bus->devices list with list_for_each_entry_safe().

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Cc: Toshi Kani <toshi.kani@hp.com>
Cc: linux-pci@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/pci/hotplug/acpiphp_glue.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 4681d2c..514851d8 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -1086,11 +1086,11 @@ static void acpiphp_set_hpp_values(struct pci_bus *bus)
  */
 static void acpiphp_sanitize_bus(struct pci_bus *bus)
 {
-	struct pci_dev *dev;
+	struct pci_dev *dev, *tmp;
 	int i;
 	unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM;
 
-	list_for_each_entry(dev, &bus->devices, bus_list) {
+	list_for_each_entry_safe(dev, tmp, &bus->devices, bus_list) {
 		for (i=0; i<PCI_BRIDGE_RESOURCES; i++) {
 			struct resource *res = &dev->resource[i];
 			if ((res->flags & type_mask) && !res->start &&
-- 
1.7.9.5

[PATCH v8 03/13] PCI/acpiphp: don't rely on function 0 in disable_device()

[Jiang Liu]

Currently function disable_device() detects slot state by checking
existence of PCI device for function 0. It's unreliable because PCI
device for function 0 may be removed through sysfs interface. If that
happens, it will cause powering off a hotplug slot without destroying
all PCI devices.

On the other hand, it won't hurt us except wasting some computation
power if the check is removed, because all code of disable_device()
is self-protected. So remove the check.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Cc: Toshi Kani <toshi.kani@hp.com>
Cc: linux-pci@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/pci/hotplug/acpiphp_glue.c |    7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 514851d8..4101c15 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -918,13 +918,6 @@ static int disable_device(struct acpiphp_slot *slot)
 	struct pci_dev *pdev;
 	struct pci_bus *bus = slot->bridge->pci_bus;
 
-	/* The slot will be enabled when func 0 is added, so check
-	   func 0 before disable the slot. */
-	pdev = pci_get_slot(bus, PCI_DEVFN(slot->device, 0));
-	if (!pdev)
-		goto err_exit;
-	pci_dev_put(pdev);
-
 	list_for_each_entry(func, &slot->funcs, sibling) {
 		if (func->bridge) {
 			/* cleanup p2p bridges under this P2P bridge */
-- 
1.7.9.5

[PATCH v8 04/13] ACPI/acpiphp: replace local macros with standard ACPI macros

[Jiang Liu]

Replace local defined macros (ACPI_STA_xxx) with standard ACPI macros
(ACPI_STA_DEVICE_xxx).

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Cc: Toshi Kani <toshi.kani@hp.com>
Cc: linux-pci@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/pci/hotplug/acpiphp.h      |    4 ----
 drivers/pci/hotplug/acpiphp_glue.c |    4 ++--
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp.h b/drivers/pci/hotplug/acpiphp.h
index b70ac00..1b311f9 100644
--- a/drivers/pci/hotplug/acpiphp.h
+++ b/drivers/pci/hotplug/acpiphp.h
@@ -146,10 +146,6 @@ struct acpiphp_attention_info
 #define ACPI_PCI_HOST_HID		"PNP0A03"
 
 /* ACPI _STA method value (ignore bit 4; battery present) */
-#define ACPI_STA_PRESENT		(0x00000001)
-#define ACPI_STA_ENABLED		(0x00000002)
-#define ACPI_STA_SHOW_IN_UI		(0x00000004)
-#define ACPI_STA_FUNCTIONING		(0x00000008)
 #define ACPI_STA_ALL			(0x0000000f)
 
 /* bridge flags */
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 4101c15..89e0c36 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -487,7 +487,7 @@ static int add_bridge(struct acpi_pci_root *root)
 			dbg("%s: _STA evaluation failure\n", __func__);
 			return 0;
 		}
-		if ((tmp & ACPI_STA_FUNCTIONING) == 0)
+		if ((tmp & ACPI_STA_DEVICE_FUNCTIONING) == 0)
 			/* don't register this object */
 			return 0;
 	}
@@ -1387,7 +1387,7 @@ u8 acpiphp_get_latch_status(struct acpiphp_slot *slot)
 
 	sta = get_slot_status(slot);
 
-	return (sta & ACPI_STA_SHOW_IN_UI) ? 0 : 1;
+	return (sta & ACPI_STA_DEVICE_UI) ? 0 : 1;
 }
 
 
-- 
1.7.9.5

[PATCH v8 05/13] PCI: introduce platform dependent hooks for creating/destroying PCI busses

[Jiang Liu]

On ACPI based platforms, the pci_slot driver creates PCI slot devices
according to information from ACPI tables by registering an ACPI PCI
subdriver. The ACPI PCI subdriver will only be called when creating/
destroying PCI root buses, and it won't be called when hot-plugging
P2P bridges. It may cause stale PCI slot devices after hot-removing
a P2P bridge if that bridge has associated PCI slots. And the acpiphp
driver has the same issue too.

This patch introduces two hook points into the PCI core, which will
be invoked when creating/destroying PCI busses for PCI host and P2P
bridges. They could be used to setup/destroy platform dependent staffs
in a unified way, both at boot time and for PCI hotplug operations.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Cc: Toshi Kani <toshi.kani@hp.com>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Myron Stowe <myron.stowe@redhat.com>
Cc: linux-pci@vger.kernel.org
Cc: linux-ia64@vger.kernel.org
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/pci/probe.c  |   12 ++++++++++++
 drivers/pci/remove.c |    1 +
 include/linux/pci.h  |    2 ++
 3 files changed, 15 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 45c93b3..43ece5d 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -673,6 +673,8 @@ add_dev:
 	ret = device_register(&child->dev);
 	WARN_ON(ret < 0);
 
+	pcibios_add_bus(child);
+
 	/* Create legacy_io and legacy_mem files for this bus */
 	pci_create_legacy_files(child);
 
@@ -1660,6 +1662,14 @@ int __weak pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
 	return 0;
 }
 
+void __weak pcibios_add_bus(struct pci_bus *bus)
+{
+}
+
+void __weak pcibios_remove_bus(struct pci_bus *bus)
+{
+}
+
 struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 		struct pci_ops *ops, void *sysdata, struct list_head *resources)
 {
@@ -1714,6 +1724,8 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 	if (error)
 		goto class_dev_reg_err;
 
+	pcibios_add_bus(b);
+
 	/* Create legacy_io and legacy_mem files for this bus */
 	pci_create_legacy_files(b);
 
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index 66967e5..6ae6480 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -49,6 +49,7 @@ void pci_remove_bus(struct pci_bus *bus)
 	pci_bus_release_busn_res(bus);
 	up_write(&pci_bus_sem);
 	pci_remove_legacy_files(bus);
+	pcibios_remove_bus(bus);
 	device_unregister(&bus->dev);
 }
 EXPORT_SYMBOL(pci_remove_bus);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 7e87b1e..00d6ba7 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -678,6 +678,8 @@ extern struct list_head pci_root_buses;	/* list of all known PCI buses */
 extern int no_pci_devices(void);
 
 void pcibios_resource_survey_bus(struct pci_bus *bus);
+void pcibios_add_bus(struct pci_bus *bus);
+void pcibios_remove_bus(struct pci_bus *bus);
 void pcibios_fixup_bus(struct pci_bus *);
 int __must_check pcibios_enable_device(struct pci_dev *, int mask);
 /* Architecture specific versions may override this (weak) */
-- 
1.7.9.5

[PATCH v8 06/13] PCI, ACPI: prepare stub functions to handle ACPI PCI (hotplug) slots

[Jiang Liu]

Prepare two stub functions to handle ACPI PCI slots and ACPI PCI hotplug
slots, which will be invoked by the PCI core when creating/destroying
PCI buses.

It will be used to get rid of ACPI PCI subdrivers for pci_slot and
acpiphp, and eventually remove the ACPI PCI subdriver mechanism.

And it will also be used to handle ACPI PCI (hotplug) slots in a unified
way, both at boot time and for PCI hotplug operations.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Cc: Toshi Kani <toshi.kani@hp.com>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Myron Stowe <myron.stowe@redhat.com>
Cc: linux-pci@vger.kernel.org
Cc: linux-ia64@vger.kernel.org
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/pci/pci-acpi.c   |   24 ++++++++++++++++++++++++
 include/linux/pci-acpi.h |    8 +++++++-
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index c685ff5..99878ac 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -287,6 +287,30 @@ static struct pci_platform_pm_ops acpi_pci_platform_pm = {
 	.run_wake = acpi_pci_run_wake,
 };
 
+void acpi_pci_add_bus(struct pci_bus *bus)
+{
+	acpi_handle handle = NULL;
+
+	if (bus->bridge)
+		handle = ACPI_HANDLE(bus->bridge);
+	if (acpi_pci_disabled || handle == NULL)
+		return;
+
+	/* TODO: add PCI slots and acpiphp hotplug slots */
+}
+
+void acpi_pci_remove_bus(struct pci_bus *bus)
+{
+	/*
+	 * bus->bridge->acpi_node.handle has already been reset to NULL
+	 * when acpi_pci_remove_bus() is called, so don't check ACPI handle.
+	 */
+	if (acpi_pci_disabled)
+		return;
+
+	/* TODO: remove PCI slots and acpiphp hotplug slots */
+}
+
 /* ACPI bus type */
 static int acpi_pci_find_device(struct device *dev, acpi_handle *handle)
 {
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index 9a22b5e..2b1743c 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -41,7 +41,13 @@ static inline acpi_handle acpi_pci_get_bridge_handle(struct pci_bus *pbus)
 
 	return DEVICE_ACPI_HANDLE(dev);
 }
-#endif
+
+void acpi_pci_add_bus(struct pci_bus *bus);
+void acpi_pci_remove_bus(struct pci_bus *bus);
+#else	/* CONFIG_ACPI */
+static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
+static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
+#endif	/* CONFIG_ACPI */
 
 #ifdef CONFIG_ACPI_APEI
 extern bool aer_acpi_firmware_first(void);
-- 
1.7.9.5

[PATCH v8 07/13] PCI, IA64: implement pcibios_{add|remove}_bus() hooks

[Jiang Liu]

Implement pcibios_{add|remove}_bus() hooks for IA64 platforms.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Cc: Toshi Kani <toshi.kani@hp.com>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: linux-ia64@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 arch/ia64/pci/pci.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/ia64/pci/pci.c b/arch/ia64/pci/pci.c
index 00e59c7..e60aeb7 100644
--- a/arch/ia64/pci/pci.c
+++ b/arch/ia64/pci/pci.c
@@ -15,6 +15,7 @@
 #include <linux/types.h>
 #include <linux/kernel.h>
 #include <linux/pci.h>
+#include <linux/pci-acpi.h>
 #include <linux/init.h>
 #include <linux/ioport.h>
 #include <linux/slab.h>
@@ -463,6 +464,16 @@ pcibios_fixup_bus (struct pci_bus *b)
 	platform_pci_fixup_bus(b);
 }
 
+void pcibios_add_bus(struct pci_bus *bus)
+{
+	acpi_pci_add_bus(bus);
+}
+
+void pcibios_remove_bus(struct pci_bus *bus)
+{
+	acpi_pci_remove_bus(bus);
+}
+
 void pcibios_set_master (struct pci_dev *dev)
 {
 	/* No special bus mastering setup handling */
-- 
1.7.9.5

[PATCH v8 08/13] PCI, x86: implement pcibios_{add|remove}_bus() hooks

[Jiang Liu]

Implement pcibios_{add|remove}_bus() hooks for x86 platforms.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Cc: Toshi Kani <toshi.kani@hp.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Myron Stowe <myron.stowe@redhat.com>
Cc: linux-kernel@vger.kernel.org
Cc: x86@kernel.org
---
 arch/x86/pci/common.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
index 505731b..79f81f0 100644
--- a/arch/x86/pci/common.c
+++ b/arch/x86/pci/common.c
@@ -6,6 +6,7 @@
 
 #include <linux/sched.h>
 #include <linux/pci.h>
+#include <linux/pci-acpi.h>
 #include <linux/ioport.h>
 #include <linux/init.h>
 #include <linux/dmi.h>
@@ -170,6 +171,16 @@ void __devinit pcibios_fixup_bus(struct pci_bus *b)
 		pcibios_fixup_device_resources(dev);
 }
 
+void pcibios_add_bus(struct pci_bus *bus)
+{
+	acpi_pci_add_bus(bus);
+}
+
+void pcibios_remove_bus(struct pci_bus *bus)
+{
+	acpi_pci_remove_bus(bus);
+}
+
 /*
  * Only use DMI information to set this if nothing was passed
  * on the kernel command line (which was parsed earlier).
-- 
1.7.9.5

[PATCH v8 09/13] PCI, ACPI: handle PCI slot devices when creating/destroying PCI busses

[Jiang Liu]

Currently the pci_slot driver doesn't update PCI slot devices when PCI
device hotplug event happens, which may cause memory leak and returning
stale information to user.

Now the pci_slot driver has been changed as built-in driver, so invoke
PCI slot enumeration and destroy routines directly from the PCI core.
And remove ACPI PCI sub-driver related code because it isn't needed
any more.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: Toshi Kani <toshi.kani@hp.com>
Cc: linux-pci@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/acpi/internal.h  |    5 --
 drivers/acpi/pci_slot.c  |  177 +++++++---------------------------------------
 drivers/acpi/scan.c      |    1 -
 drivers/pci/pci-acpi.c   |    7 +-
 include/linux/pci-acpi.h |   12 ++++
 5 files changed, 44 insertions(+), 158 deletions(-)

diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index e09ce03..0f24148 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -67,11 +67,6 @@ struct acpi_ec {
 
 extern struct acpi_ec *first_ec;
 
-#ifdef	CONFIG_ACPI_PCI_SLOT
-void acpi_pci_slot_init(void);
-#else
-static inline void acpi_pci_slot_init(void) { }
-#endif
 int acpi_pci_root_init(void);
 void acpi_pci_root_hp_init(void);
 int acpi_ec_init(void);
diff --git a/drivers/acpi/pci_slot.c b/drivers/acpi/pci_slot.c
index a7d7e77..35d7227 100644
--- a/drivers/acpi/pci_slot.c
+++ b/drivers/acpi/pci_slot.c
@@ -9,6 +9,9 @@
  *  Copyright (C) 2007-2008 Hewlett-Packard Development Company, L.P.
  *  	Alex Chiang <achiang@hp.com>
  *
+ *  Copyright (C) 2013 Huawei Tech. Co., Ltd.
+ *	Jiang Liu <jiang.liu@huawei.com>
+ *
  *  This program is free software; you can redistribute it and/or modify it
  *  under the terms and conditions of the GNU General Public License,
  *  version 2, as published by the Free Software Foundation.
@@ -28,10 +31,9 @@
 #include <linux/init.h>
 #include <linux/slab.h>
 #include <linux/types.h>
+#include <linux/list.h>
 #include <linux/pci.h>
 #include <linux/acpi.h>
-#include <acpi/acpi_bus.h>
-#include <acpi/acpi_drivers.h>
 #include <linux/dmi.h>
 
 static bool debug;
@@ -50,32 +52,23 @@ module_param(debug, bool, 0644);
 ACPI_MODULE_NAME("pci_slot");
 
 #define MY_NAME "pci_slot"
-#define err(format, arg...) printk(KERN_ERR "%s: " format , MY_NAME , ## arg)
-#define info(format, arg...) printk(KERN_INFO "%s: " format , MY_NAME , ## arg)
+#define err(format, arg...) pr_err("%s: " format , MY_NAME , ## arg)
+#define info(format, arg...) pr_info("%s: " format , MY_NAME , ## arg)
 #define dbg(format, arg...)					\
 	do {							\
 		if (debug)					\
-			printk(KERN_DEBUG "%s: " format,	\
-				MY_NAME , ## arg);		\
+			pr_debug(KERN_DEBUG "%s: " format, MY_NAME , ## arg); \
 	} while (0)
 
 #define SLOT_NAME_SIZE 21		/* Inspired by #define in acpiphp.h */
 
 struct acpi_pci_slot {
-	acpi_handle root_handle;	/* handle of the root bridge */
 	struct pci_slot *pci_slot;	/* corresponding pci_slot */
 	struct list_head list;		/* node in the list of slots */
 };
 
-static int acpi_pci_slot_add(struct acpi_pci_root *root);
-static void acpi_pci_slot_remove(struct acpi_pci_root *root);
-
 static LIST_HEAD(slot_list);
 static DEFINE_MUTEX(slot_list_lock);
-static struct acpi_pci_driver acpi_pci_slot_driver = {
-	.add = acpi_pci_slot_add,
-	.remove = acpi_pci_slot_remove,
-};
 
 static int
 check_slot(acpi_handle handle, unsigned long long *sun)
@@ -114,21 +107,8 @@ out:
 	return device;
 }
 
-struct callback_args {
-	acpi_walk_callback	user_function;	/* only for walk_p2p_bridge */
-	struct pci_bus		*pci_bus;
-	acpi_handle		root_handle;
-};
-
 /*
- * register_slot
- *
- * Called once for each SxFy object in the namespace. Don't worry about
- * calling pci_create_slot multiple times for the same pci_bus:device,
- * since each subsequent call simply bumps the refcount on the pci_slot.
- *
- * The number of calls to pci_destroy_slot from unregister_slot is
- * symmetrical.
+ * Check whether handle has an associated slot and create PCI slot if it has.
  */
 static acpi_status
 register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
@@ -138,13 +118,22 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
 	char name[SLOT_NAME_SIZE];
 	struct acpi_pci_slot *slot;
 	struct pci_slot *pci_slot;
-	struct callback_args *parent_context = context;
-	struct pci_bus *pci_bus = parent_context->pci_bus;
+	struct pci_bus *pci_bus = context;
 
 	device = check_slot(handle, &sun);
 	if (device < 0)
 		return AE_OK;
 
+	/*
+	 * There may be multiple PCI functions associated with the same slot.
+	 * Check whether PCI slot has already been created for this PCI device.
+	 */
+	list_for_each_entry(slot, &slot_list, list) {
+		pci_slot = slot->pci_slot;
+		if (pci_slot->bus == pci_bus && pci_slot->number == device)
+			return AE_OK;
+	}
+
 	slot = kmalloc(sizeof(*slot), GFP_KERNEL);
 	if (!slot) {
 		err("%s: cannot allocate memory\n", __func__);
@@ -159,12 +148,8 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
 		return AE_OK;
 	}
 
-	slot->root_handle = parent_context->root_handle;
 	slot->pci_slot = pci_slot;
-	INIT_LIST_HEAD(&slot->list);
-	mutex_lock(&slot_list_lock);
 	list_add(&slot->list, &slot_list);
-	mutex_unlock(&slot_list_lock);
 
 	get_device(&pci_bus->dev);
 
@@ -174,131 +159,24 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
 	return AE_OK;
 }
 
-/*
- * walk_p2p_bridge - discover and walk p2p bridges
- * @handle: points to an acpi_pci_root
- * @context: p2p_bridge_context pointer
- *
- * Note that when we call ourselves recursively, we pass a different
- * value of pci_bus in the child_context.
- */
-static acpi_status
-walk_p2p_bridge(acpi_handle handle, u32 lvl, void *context, void **rv)
-{
-	int device, function;
-	unsigned long long adr;
-	acpi_status status;
-	acpi_handle dummy_handle;
-	acpi_walk_callback user_function;
-
-	struct pci_dev *dev;
-	struct pci_bus *pci_bus;
-	struct callback_args child_context;
-	struct callback_args *parent_context = context;
-
-	pci_bus = parent_context->pci_bus;
-	user_function = parent_context->user_function;
-
-	status = acpi_get_handle(handle, "_ADR", &dummy_handle);
-	if (ACPI_FAILURE(status))
-		return AE_OK;
-
-	status = acpi_evaluate_integer(handle, "_ADR", NULL, &adr);
-	if (ACPI_FAILURE(status))
-		return AE_OK;
-
-	device = (adr >> 16) & 0xffff;
-	function = adr & 0xffff;
-
-	dev = pci_get_slot(pci_bus, PCI_DEVFN(device, function));
-	if (!dev || !dev->subordinate)
-		goto out;
-
-	child_context.pci_bus = dev->subordinate;
-	child_context.user_function = user_function;
-	child_context.root_handle = parent_context->root_handle;
-
-	dbg("p2p bridge walk, pci_bus = %x\n", dev->subordinate->number);
-	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
-				     user_function, NULL, &child_context, NULL);
-	if (ACPI_FAILURE(status))
-		goto out;
-
-	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
-				     walk_p2p_bridge, NULL, &child_context, NULL);
-out:
-	pci_dev_put(dev);
-	return AE_OK;
-}
-
-/*
- * walk_root_bridge - generic root bridge walker
- * @root: poiner of an acpi_pci_root
- * @user_function: user callback for slot objects
- *
- * Call user_function for all objects underneath this root bridge.
- * Walk p2p bridges underneath us and call user_function on those too.
- */
-static int
-walk_root_bridge(struct acpi_pci_root *root, acpi_walk_callback user_function)
-{
-	acpi_status status;
-	acpi_handle handle = root->device->handle;
-	struct pci_bus *pci_bus = root->bus;
-	struct callback_args context;
-
-	context.pci_bus = pci_bus;
-	context.user_function = user_function;
-	context.root_handle = handle;
-
-	dbg("root bridge walk, pci_bus = %x\n", pci_bus->number);
-	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
-				     user_function, NULL, &context, NULL);
-	if (ACPI_FAILURE(status))
-		return status;
-
-	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
-				     walk_p2p_bridge, NULL, &context, NULL);
-	if (ACPI_FAILURE(status))
-		err("%s: walk_p2p_bridge failure - %d\n", __func__, status);
-
-	return status;
-}
-
-/*
- * acpi_pci_slot_add
- * @handle: points to an acpi_pci_root
- */
-static int
-acpi_pci_slot_add(struct acpi_pci_root *root)
+void acpi_pci_slot_enumerate(struct pci_bus *bus, acpi_handle handle)
 {
-	acpi_status status;
-
-	status = walk_root_bridge(root, register_slot);
-	if (ACPI_FAILURE(status))
-		err("%s: register_slot failure - %d\n", __func__, status);
-
-	return status;
+	mutex_lock(&slot_list_lock);
+	acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
+			    register_slot, NULL, bus, NULL);
+	mutex_unlock(&slot_list_lock);
 }
 
-/*
- * acpi_pci_slot_remove
- * @handle: points to an acpi_pci_root
- */
-static void
-acpi_pci_slot_remove(struct acpi_pci_root *root)
+void acpi_pci_slot_remove(struct pci_bus *bus)
 {
 	struct acpi_pci_slot *slot, *tmp;
-	struct pci_bus *pbus;
-	acpi_handle handle = root->device->handle;
 
 	mutex_lock(&slot_list_lock);
 	list_for_each_entry_safe(slot, tmp, &slot_list, list) {
-		if (slot->root_handle == handle) {
+		if (slot->pci_slot->bus == bus) {
 			list_del(&slot->list);
-			pbus = slot->pci_slot->bus;
 			pci_destroy_slot(slot->pci_slot);
-			put_device(&pbus->dev);
+			put_device(&bus->dev);
 			kfree(slot);
 		}
 	}
@@ -333,5 +211,4 @@ static struct dmi_system_id acpi_pci_slot_dmi_table[] __initdata = {
 void __init acpi_pci_slot_init(void)
 {
 	dmi_check_system(acpi_pci_slot_dmi_table);
-	acpi_pci_register_driver(&acpi_pci_slot_driver);
 }
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index b643aed..bc2f337 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1687,7 +1687,6 @@ int __init acpi_scan_init(void)
 
 	acpi_power_init();
 	acpi_pci_root_init();
-	acpi_pci_slot_init();
 
 	/*
 	 * Enumerate devices in the ACPI namespace.
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 99878ac..2dbca38 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -296,7 +296,7 @@ void acpi_pci_add_bus(struct pci_bus *bus)
 	if (acpi_pci_disabled || handle == NULL)
 		return;
 
-	/* TODO: add PCI slots and acpiphp hotplug slots */
+	acpi_pci_slot_enumerate(bus, handle);
 }
 
 void acpi_pci_remove_bus(struct pci_bus *bus)
@@ -308,7 +308,7 @@ void acpi_pci_remove_bus(struct pci_bus *bus)
 	if (acpi_pci_disabled)
 		return;
 
-	/* TODO: remove PCI slots and acpiphp hotplug slots */
+	acpi_pci_slot_remove(bus);
 }
 
 /* ACPI bus type */
@@ -381,7 +381,10 @@ static int __init acpi_pci_init(void)
 	ret = register_acpi_bus_type(&acpi_pci_bus);
 	if (ret)
 		return 0;
+
 	pci_set_platform_pm(&acpi_pci_platform_pm);
+	acpi_pci_slot_init();
+
 	return 0;
 }
 arch_initcall(acpi_pci_init);
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index 2b1743c..1ef126f 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -44,6 +44,18 @@ static inline acpi_handle acpi_pci_get_bridge_handle(struct pci_bus *pbus)
 
 void acpi_pci_add_bus(struct pci_bus *bus);
 void acpi_pci_remove_bus(struct pci_bus *bus);
+
+#ifdef	CONFIG_ACPI_PCI_SLOT
+extern void acpi_pci_slot_init(void);
+extern void acpi_pci_slot_enumerate(struct pci_bus *bus, acpi_handle handle);
+extern void acpi_pci_slot_remove(struct pci_bus *bus);
+#else
+static inline void acpi_pci_slot_init(void) { }
+static inline void acpi_pci_slot_enumerate(struct pci_bus *bus,
+					   acpi_handle handle) { }
+static inline void acpi_pci_slot_remove(struct pci_bus *bus) { }
+#endif
+
 #else	/* CONFIG_ACPI */
 static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
 static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
-- 
1.7.9.5

[PATCH v8 10/13] PCI/acpiphp: do not use ACPI PCI subdriver mechanism

[Jiang Liu]

Previously the acpiphp driver registers itself as an ACPI PCI subdriver,
so it's callbacks will be invoked when creating/destroying PCI root
buses to manage ACPI based PCI hotplug slots. But it doesn't handle
P2P bridge hotplug events, so it will cause strange behavours if there
are hotplug slots associated with a hot-removed P2P bridge.

So this patch tries to fix this issue by:
1) Make acpiphp built-in only, not a module.
2) Directly hook into PCI core to update hotplug slot devices when
   creating/destroying PCI buses through:
	pci_{add|remove}_bus() -> acpi_pci_{add|remove}_bus()
3) Get rid of ACPI PCI subdriver related code, it's not used any more.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Cc: Toshi Kani <toshi.kani@hp.com>
Cc: linux-pci@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/pci/hotplug/Kconfig        |    7 +-
 drivers/pci/hotplug/acpiphp.h      |    3 -
 drivers/pci/hotplug/acpiphp_core.c |   23 +--
 drivers/pci/hotplug/acpiphp_glue.c |  282 +++++++-----------------------------
 drivers/pci/pci-acpi.c             |    3 +
 include/linux/pci-acpi.h           |   14 +-
 6 files changed, 67 insertions(+), 265 deletions(-)

diff --git a/drivers/pci/hotplug/Kconfig b/drivers/pci/hotplug/Kconfig
index 13e9e63..9fcb87f 100644
--- a/drivers/pci/hotplug/Kconfig
+++ b/drivers/pci/hotplug/Kconfig
@@ -52,15 +52,12 @@ config HOTPLUG_PCI_IBM
 	  When in doubt, say N.
 
 config HOTPLUG_PCI_ACPI
-	tristate "ACPI PCI Hotplug driver"
-	depends on (!ACPI_DOCK && ACPI) || (ACPI_DOCK)
+	bool "ACPI PCI Hotplug driver"
+	depends on HOTPLUG_PCI=y && ((!ACPI_DOCK && ACPI) || (ACPI_DOCK))
 	help
 	  Say Y here if you have a system that supports PCI Hotplug using
 	  ACPI.
 
-	  To compile this driver as a module, choose M here: the
-	  module will be called acpiphp.
-
 	  When in doubt, say N.
 
 config HOTPLUG_PCI_ACPI_IBM
diff --git a/drivers/pci/hotplug/acpiphp.h b/drivers/pci/hotplug/acpiphp.h
index 1b311f9..69b4aac 100644
--- a/drivers/pci/hotplug/acpiphp.h
+++ b/drivers/pci/hotplug/acpiphp.h
@@ -119,7 +119,6 @@ struct acpiphp_slot {
  */
 struct acpiphp_func {
 	struct acpiphp_slot *slot;	/* parent */
-	struct acpiphp_bridge *bridge;	/* Ejectable PCI-to-PCI bridge */
 
 	struct list_head sibling;
 	struct notifier_block nb;
@@ -176,8 +175,6 @@ extern int acpiphp_register_hotplug_slot(struct acpiphp_slot *slot);
 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);
 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 c2fd309..80d777c 100644
--- a/drivers/pci/hotplug/acpiphp_core.c
+++ b/drivers/pci/hotplug/acpiphp_core.c
@@ -37,6 +37,7 @@
 
 #include <linux/kernel.h>
 #include <linux/pci.h>
+#include <linux/pci-acpi.h>
 #include <linux/pci_hotplug.h>
 #include <linux/slab.h>
 #include <linux/smp.h>
@@ -351,27 +352,7 @@ void acpiphp_unregister_hotplug_slot(struct acpiphp_slot *acpiphp_slot)
 }
 
 
-static int __init acpiphp_init(void)
+void __init acpiphp_init(void)
 {
 	info(DRIVER_DESC " version: " DRIVER_VERSION "\n");
-
-	if (acpi_pci_disabled)
-		return 0;
-
-	/* read all the ACPI info from the system */
-	/* initialize internal data structure etc. */
-	return acpiphp_glue_init();
-}
-
-
-static void __exit acpiphp_exit(void)
-{
-	if (acpi_pci_disabled)
-		return;
-
-	/* deallocate internal data structures etc. */
-	acpiphp_glue_exit();
 }
-
-module_init(acpiphp_init);
-module_exit(acpiphp_exit);
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 89e0c36..3db84b6 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -157,6 +157,7 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
 	int device, function, retval;
 	struct pci_bus *pbus = bridge->pci_bus;
 	struct pci_dev *pdev;
+	u32 val;
 
 	if (!acpi_pci_check_ejectable(pbus, handle) && !is_dock_device(handle))
 		return AE_OK;
@@ -249,11 +250,9 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
 	newfunc->slot = slot;
 	list_add_tail(&newfunc->sibling, &slot->funcs);
 
-	pdev = pci_get_slot(pbus, PCI_DEVFN(device, function));
-	if (pdev) {
+	if (pci_bus_read_dev_vendor_id(pbus, PCI_DEVFN(device, function),
+				       &val, 60*1000))
 		slot->flags |= (SLOT_ENABLED | SLOT_POWEREDON);
-		pci_dev_put(pdev);
-	}
 
 	if (is_dock_device(handle)) {
 		/* we don't want to call this device's _EJ0
@@ -366,148 +365,6 @@ static struct acpiphp_func *acpiphp_bridge_handle_to_function(acpi_handle handle
 }
 
 
-static inline void config_p2p_bridge_flags(struct acpiphp_bridge *bridge)
-{
-	acpi_handle dummy_handle;
-	struct acpiphp_func *func;
-
-	if (ACPI_SUCCESS(acpi_get_handle(bridge->handle,
-					"_EJ0", &dummy_handle))) {
-		bridge->flags |= BRIDGE_HAS_EJ0;
-
-		dbg("found ejectable p2p bridge\n");
-
-		/* make link between PCI bridge and PCI function */
-		func = acpiphp_bridge_handle_to_function(bridge->handle);
-		if (!func)
-			return;
-		bridge->func = func;
-		func->bridge = bridge;
-	}
-}
-
-
-/* allocate and initialize host bridge data structure */
-static void add_host_bridge(struct acpi_pci_root *root)
-{
-	struct acpiphp_bridge *bridge;
-	acpi_handle handle = root->device->handle;
-
-	bridge = kzalloc(sizeof(struct acpiphp_bridge), GFP_KERNEL);
-	if (bridge == NULL)
-		return;
-
-	bridge->handle = handle;
-
-	bridge->pci_bus = root->bus;
-
-	init_bridge_misc(bridge);
-}
-
-
-/* allocate and initialize PCI-to-PCI bridge data structure */
-static void add_p2p_bridge(acpi_handle *handle)
-{
-	struct acpiphp_bridge *bridge;
-
-	bridge = kzalloc(sizeof(struct acpiphp_bridge), GFP_KERNEL);
-	if (bridge == NULL) {
-		err("out of memory\n");
-		return;
-	}
-
-	bridge->handle = handle;
-	config_p2p_bridge_flags(bridge);
-
-	bridge->pci_dev = acpi_get_pci_dev(handle);
-	bridge->pci_bus = bridge->pci_dev->subordinate;
-	if (!bridge->pci_bus) {
-		err("This is not a PCI-to-PCI bridge!\n");
-		goto err;
-	}
-
-	/*
-	 * Grab a ref to the subordinate PCI bus in case the bus is
-	 * removed via PCI core logical hotplug. The ref pins the bus
-	 * (which we access during module unload).
-	 */
-	get_device(&bridge->pci_bus->dev);
-
-	init_bridge_misc(bridge);
-	return;
- err:
-	pci_dev_put(bridge->pci_dev);
-	kfree(bridge);
-	return;
-}
-
-
-/* callback routine to find P2P bridges */
-static acpi_status
-find_p2p_bridge(acpi_handle handle, u32 lvl, void *context, void **rv)
-{
-	acpi_status status;
-	struct pci_dev *dev;
-
-	dev = acpi_get_pci_dev(handle);
-	if (!dev || !dev->subordinate)
-		goto out;
-
-	/* check if this bridge has ejectable slots */
-	if ((detect_ejectable_slots(handle) > 0)) {
-		dbg("found PCI-to-PCI bridge at PCI %s\n", pci_name(dev));
-		add_p2p_bridge(handle);
-	}
-
-	/* search P2P bridges under this p2p bridge */
-	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
-				     find_p2p_bridge, NULL, NULL, NULL);
-	if (ACPI_FAILURE(status))
-		warn("find_p2p_bridge failed (error code = 0x%x)\n", status);
-
- out:
-	pci_dev_put(dev);
-	return AE_OK;
-}
-
-
-/* find hot-pluggable slots, and then find P2P bridge */
-static int add_bridge(struct acpi_pci_root *root)
-{
-	acpi_status status;
-	unsigned long long tmp;
-	acpi_handle dummy_handle;
-	acpi_handle handle = root->device->handle;
-
-	/* if the bridge doesn't have _STA, we assume it is always there */
-	status = acpi_get_handle(handle, "_STA", &dummy_handle);
-	if (ACPI_SUCCESS(status)) {
-		status = acpi_evaluate_integer(handle, "_STA", NULL, &tmp);
-		if (ACPI_FAILURE(status)) {
-			dbg("%s: _STA evaluation failure\n", __func__);
-			return 0;
-		}
-		if ((tmp & ACPI_STA_DEVICE_FUNCTIONING) == 0)
-			/* don't register this object */
-			return 0;
-	}
-
-	/* check if this bridge has ejectable slots */
-	if (detect_ejectable_slots(handle) > 0) {
-		dbg("found PCI host-bus bridge with hot-pluggable slots\n");
-		add_host_bridge(root);
-	}
-
-	/* search P2P bridges under this host bridge */
-	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
-				     find_p2p_bridge, NULL, NULL, NULL);
-
-	if (ACPI_FAILURE(status))
-		warn("find_p2p_bridge failed (error code = 0x%x)\n", status);
-
-	return 0;
-}
-
 static struct acpiphp_bridge *acpiphp_handle_to_bridge(acpi_handle handle)
 {
 	struct acpiphp_bridge *bridge;
@@ -567,56 +424,12 @@ static void cleanup_bridge(struct acpiphp_bridge *bridge)
 		slot = next;
 	}
 
-	/*
-	 * Only P2P bridges have a pci_dev
-	 */
-	if (bridge->pci_dev)
-		put_device(&bridge->pci_bus->dev);
-
+	put_device(&bridge->pci_bus->dev);
 	pci_dev_put(bridge->pci_dev);
 	list_del(&bridge->list);
 	kfree(bridge);
 }
 
-static acpi_status
-cleanup_p2p_bridge(acpi_handle handle, u32 lvl, void *context, void **rv)
-{
-	struct acpiphp_bridge *bridge;
-
-	/* cleanup p2p bridges under this P2P bridge
-	   in a depth-first manner */
-	acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
-				cleanup_p2p_bridge, NULL, NULL, NULL);
-
-	bridge = acpiphp_handle_to_bridge(handle);
-	if (bridge)
-		cleanup_bridge(bridge);
-
-	return AE_OK;
-}
-
-static void remove_bridge(struct acpi_pci_root *root)
-{
-	struct acpiphp_bridge *bridge;
-	acpi_handle handle = root->device->handle;
-
-	/* cleanup p2p bridges under this host bridge
-	   in a depth-first manner */
-	acpi_walk_namespace(ACPI_TYPE_DEVICE, handle,
-				(u32)1, cleanup_p2p_bridge, NULL, NULL, NULL);
-
-	/*
-	 * On root bridges with hotplug slots directly underneath (ie,
-	 * no p2p bridge between), we call cleanup_bridge(). 
-	 *
-	 * The else clause cleans up root bridges that either had no
-	 * hotplug slots at all, or had a p2p bridge underneath.
-	 */
-	bridge = acpiphp_handle_to_bridge(handle);
-	if (bridge)
-		cleanup_bridge(bridge);
-}
-
 static int power_on_slot(struct acpiphp_slot *slot)
 {
 	acpi_status status;
@@ -802,6 +615,7 @@ static void check_hotplug_bridge(struct acpiphp_slot *slot, struct pci_dev *dev)
 		}
 	}
 }
+
 /**
  * enable_device - enable, configure a slot
  * @slot: slot to be enabled
@@ -816,7 +630,6 @@ static int __ref enable_device(struct acpiphp_slot *slot)
 	struct acpiphp_func *func;
 	int retval = 0;
 	int num, max, pass;
-	acpi_status status;
 
 	if (slot->flags & SLOT_ENABLED)
 		goto err_exit;
@@ -871,18 +684,6 @@ static int __ref enable_device(struct acpiphp_slot *slot)
 			slot->flags &= (~SLOT_ENABLED);
 			continue;
 		}
-
-		if (dev->hdr_type != PCI_HEADER_TYPE_BRIDGE &&
-		    dev->hdr_type != PCI_HEADER_TYPE_CARDBUS) {
-			pci_dev_put(dev);
-			continue;
-		}
-
-		status = find_p2p_bridge(func->handle, (u32)1, bus, NULL);
-		if (ACPI_FAILURE(status))
-			warn("find_p2p_bridge failed (error code = 0x%x)\n",
-				status);
-		pci_dev_put(dev);
 	}
 
 
@@ -916,16 +717,6 @@ static int disable_device(struct acpiphp_slot *slot)
 {
 	struct acpiphp_func *func;
 	struct pci_dev *pdev;
-	struct pci_bus *bus = slot->bridge->pci_bus;
-
-	list_for_each_entry(func, &slot->funcs, sibling) {
-		if (func->bridge) {
-			/* cleanup p2p bridges under this P2P bridge */
-			cleanup_p2p_bridge(func->bridge->handle,
-						(u32)1, NULL, NULL);
-			func->bridge = NULL;
-		}
-	}
 
 	/*
 	 * enable_device() enumerates all functions in this device via
@@ -944,7 +735,6 @@ static int disable_device(struct acpiphp_slot *slot)
 
 	slot->flags &= (~SLOT_ENABLED);
 
-err_exit:
 	return 0;
 }
 
@@ -1285,30 +1075,56 @@ static void handle_hotplug_event_func(acpi_handle handle, u32 type,
 	alloc_acpi_hp_work(handle, type, context, _handle_hotplug_event_func);
 }
 
-static struct acpi_pci_driver acpi_pci_hp_driver = {
-	.add =		add_bridge,
-	.remove =	remove_bridge,
-};
-
-/**
- * acpiphp_glue_init - initializes all PCI hotplug - ACPI glue data structures
+/*
+ * Create hotplug slots for the PCI bus.
+ * It should always return 0 to avoid skipping following notifiers.
  */
-int __init acpiphp_glue_init(void)
+void acpiphp_enumerate_slots(struct pci_bus *bus, acpi_handle handle)
 {
-	acpi_pci_register_driver(&acpi_pci_hp_driver);
+	acpi_handle dummy_handle;
+	struct acpiphp_bridge *bridge;
 
-	return 0;
-}
+	if (detect_ejectable_slots(handle) <= 0)
+		return;
 
+	bridge = kzalloc(sizeof(struct acpiphp_bridge), GFP_KERNEL);
+	if (bridge == NULL) {
+		err("out of memory\n");
+		return;
+	}
 
-/**
- * acpiphp_glue_exit - terminates all PCI hotplug - ACPI glue data structures
- *
- * This function frees all data allocated in acpiphp_glue_init().
- */
-void  acpiphp_glue_exit(void)
+	bridge->handle = handle;
+	bridge->pci_dev = pci_dev_get(bus->self);
+	bridge->pci_bus = bus;
+
+	/*
+	 * Grab a ref to the subordinate PCI bus in case the bus is
+	 * removed via PCI core logical hotplug. The ref pins the bus
+	 * (which we access during module unload).
+	 */
+	get_device(&bus->dev);
+
+	if (!pci_is_root_bus(bridge->pci_bus) &&
+	    ACPI_SUCCESS(acpi_get_handle(bridge->handle,
+					"_EJ0", &dummy_handle))) {
+		dbg("found ejectable p2p bridge\n");
+		bridge->flags |= BRIDGE_HAS_EJ0;
+		bridge->func = acpiphp_bridge_handle_to_function(handle);
+	}
+
+	init_bridge_misc(bridge);
+}
+
+/* Destroy hotplug slots associated with the PCI bus */
+void acpiphp_remove_slots(struct pci_bus *bus)
 {
-	acpi_pci_unregister_driver(&acpi_pci_hp_driver);
+	struct acpiphp_bridge *bridge, *tmp;
+
+	list_for_each_entry_safe(bridge, tmp, &bridge_list, list)
+		if (bridge->pci_bus == bus) {
+			cleanup_bridge(bridge);
+			break;
+		}
 }
 
 /**
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 2dbca38..6fe7bf8 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -297,6 +297,7 @@ void acpi_pci_add_bus(struct pci_bus *bus)
 		return;
 
 	acpi_pci_slot_enumerate(bus, handle);
+	acpiphp_enumerate_slots(bus, handle);
 }
 
 void acpi_pci_remove_bus(struct pci_bus *bus)
@@ -308,6 +309,7 @@ void acpi_pci_remove_bus(struct pci_bus *bus)
 	if (acpi_pci_disabled)
 		return;
 
+	acpiphp_remove_slots(bus);
 	acpi_pci_slot_remove(bus);
 }
 
@@ -384,6 +386,7 @@ static int __init acpi_pci_init(void)
 
 	pci_set_platform_pm(&acpi_pci_platform_pm);
 	acpi_pci_slot_init();
+	acpiphp_init();
 
 	return 0;
 }
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index 1ef126f..f1b2380 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -56,9 +56,17 @@ static inline void acpi_pci_slot_enumerate(struct pci_bus *bus,
 static inline void acpi_pci_slot_remove(struct pci_bus *bus) { }
 #endif
 
-#else	/* CONFIG_ACPI */
-static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
-static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
+#ifdef	CONFIG_HOTPLUG_PCI_ACPI
+void acpiphp_init(void);
+void acpiphp_enumerate_slots(struct pci_bus *bus, acpi_handle handle);
+void acpiphp_remove_slots(struct pci_bus *bus);
+#else
+static inline void acpiphp_init(void) { }
+static inline void acpiphp_enumerate_slots(struct pci_bus *bus,
+					   acpi_handle handle) { }
+static inline void acpiphp_remove_slots(struct pci_bus *bus) { }
+#endif
+
 #endif	/* CONFIG_ACPI */
 
 #ifdef CONFIG_ACPI_APEI
-- 
1.7.9.5

[PATCH v8 11/13] PCI/acpiphp: use normal list to simplify implementation

[Jiang Liu]

From: Yijing Wang <wangyijing@huawei.com>

Use normal list for struct acpiphp_slot to simplify implementation.

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Cc: Toshi Kani <toshi.kani@hp.com>
Cc: linux-pci@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/pci/hotplug/acpiphp.h      |    4 ++--
 drivers/pci/hotplug/acpiphp_glue.c |   23 ++++++++++-------------
 2 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp.h b/drivers/pci/hotplug/acpiphp.h
index 69b4aac..0b30045 100644
--- a/drivers/pci/hotplug/acpiphp.h
+++ b/drivers/pci/hotplug/acpiphp.h
@@ -73,8 +73,8 @@ static inline const char *slot_name(struct slot *slot)
  */
 struct acpiphp_bridge {
 	struct list_head list;
+	struct list_head slots;
 	acpi_handle handle;
-	struct acpiphp_slot *slots;
 
 	/* Ejectable PCI-to-PCI bridge (PCI bridge and PCI function) */
 	struct acpiphp_func *func;
@@ -97,7 +97,7 @@ struct acpiphp_bridge {
  * PCI slot information for each *physical* PCI slot
  */
 struct acpiphp_slot {
-	struct acpiphp_slot *next;
+	struct list_head node;
 	struct acpiphp_bridge *bridge;	/* parent */
 	struct list_head funcs;		/* one slot may have different
 					   objects (i.e. for each function) */
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 3db84b6..3566f9a 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -154,7 +154,7 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
 	acpi_handle tmp;
 	acpi_status status = AE_OK;
 	unsigned long long adr, sun;
-	int device, function, retval;
+	int device, function, retval, found = 0;
 	struct pci_bus *pbus = bridge->pci_bus;
 	struct pci_dev *pdev;
 	u32 val;
@@ -208,14 +208,15 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
 	}
 
 	/* search for objects that share the same slot */
-	for (slot = bridge->slots; slot; slot = slot->next)
+	list_for_each_entry(slot, &bridge->slots, node)
 		if (slot->device == device) {
 			if (slot->sun != sun)
 				warn("sibling found, but _SUN doesn't match!\n");
+			found = 1;
 			break;
 		}
 
-	if (!slot) {
+	if (!found) {
 		slot = kzalloc(sizeof(struct acpiphp_slot), GFP_KERNEL);
 		if (!slot) {
 			kfree(newfunc);
@@ -228,9 +229,7 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
 		INIT_LIST_HEAD(&slot->funcs);
 		mutex_init(&slot->crit_sect);
 
-		slot->next = bridge->slots;
-		bridge->slots = slot;
-
+		list_add_tail(&slot->node, &bridge->slots);
 		bridge->nr_slots++;
 
 		dbg("found ACPI PCI Hotplug slot %llu at PCI %04x:%02x:%02x\n",
@@ -289,7 +288,7 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
 
  err_exit:
 	bridge->nr_slots--;
-	bridge->slots = slot->next;
+	list_del(&slot->node);
 	kfree(slot);
 	kfree(newfunc);
 
@@ -353,7 +352,7 @@ static struct acpiphp_func *acpiphp_bridge_handle_to_function(acpi_handle handle
 	struct acpiphp_func *func;
 
 	list_for_each_entry(bridge, &bridge_list, list) {
-		for (slot = bridge->slots; slot; slot = slot->next) {
+		list_for_each_entry(slot, &bridge->slots, node) {
 			list_for_each_entry(func, &slot->funcs, sibling) {
 				if (func->handle == handle)
 					return func;
@@ -400,9 +399,7 @@ static void cleanup_bridge(struct acpiphp_bridge *bridge)
 			err("failed to install interrupt notify handler\n");
 	}
 
-	slot = bridge->slots;
-	while (slot) {
-		next = slot->next;
+	list_for_each_entry_safe(slot, next, &bridge->slots, node) {
 		list_for_each_entry_safe(func, tmp, &slot->funcs, sibling) {
 			if (is_dock_device(func->handle)) {
 				unregister_hotplug_dock_device(func->handle);
@@ -421,7 +418,6 @@ static void cleanup_bridge(struct acpiphp_bridge *bridge)
 		acpiphp_unregister_hotplug_slot(slot);
 		list_del(&slot->funcs);
 		kfree(slot);
-		slot = next;
 	}
 
 	put_device(&bridge->pci_bus->dev);
@@ -824,7 +820,7 @@ static int acpiphp_check_bridge(struct acpiphp_bridge *bridge)
 
 	enabled = disabled = 0;
 
-	for (slot = bridge->slots; slot; slot = slot->next) {
+	list_for_each_entry(slot, &bridge->slots, node) {
 		unsigned int status = get_slot_status(slot);
 		if (slot->flags & SLOT_ENABLED) {
 			if (status == ACPI_STA_ALL)
@@ -1093,6 +1089,7 @@ void acpiphp_enumerate_slots(struct pci_bus *bus, acpi_handle handle)
 		return;
 	}
 
+	INIT_LIST_HEAD(&bridge->slots);
 	bridge->handle = handle;
 	bridge->pci_dev = pci_dev_get(bus->self);
 	bridge->pci_bus = bus;
-- 
1.7.9.5

[PATCH v8 12/13] PCI/acpiphp: protect acpiphp data structures from concurrent updating

[Jiang Liu]

Now acpiphp_enumerate_slots() and acpiphp_remove_slots() may be invoked
concurrently by the PCI core, so add a bridge_mutex and reference count
mechanism to protect acpiphp bridge/slot/function data structures.

To avoid deadlock, handle_hotplug_event_bridge() will requeue the
hotplug event onto the kacpi_hotplug_wq by calling alloc_acpi_hp_work().
But the workaround has introduced a minor race window because the
'bridge' passed to _handle_hotplug_event_bridge() may have already been
destroyed when _handle_hotplug_event_bridge() is actually executed by
the kacpi_hotplug_wq. So hold a reference count on the passed 'bridge'.
Fix the same issue for handle_hotplug_event_func() too.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Cc: Toshi Kani <toshi.kani@hp.com>
Cc: linux-pci@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/pci/hotplug/acpiphp.h      |    1 +
 drivers/pci/hotplug/acpiphp_glue.c |   95 +++++++++++++++++++++++++++++-------
 2 files changed, 78 insertions(+), 18 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp.h b/drivers/pci/hotplug/acpiphp.h
index 0b30045..7577bb3 100644
--- a/drivers/pci/hotplug/acpiphp.h
+++ b/drivers/pci/hotplug/acpiphp.h
@@ -74,6 +74,7 @@ static inline const char *slot_name(struct slot *slot)
 struct acpiphp_bridge {
 	struct list_head list;
 	struct list_head slots;
+	struct kref ref;
 	acpi_handle handle;
 
 	/* Ejectable PCI-to-PCI bridge (PCI bridge and PCI function) */
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 3566f9a..f3647ae 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -54,6 +54,7 @@
 #include "acpiphp.h"
 
 static LIST_HEAD(bridge_list);
+static DEFINE_MUTEX(bridge_mutex);
 
 #define MY_NAME "acpiphp_glue"
 
@@ -61,6 +62,7 @@ static void handle_hotplug_event_bridge (acpi_handle, u32, void *);
 static void acpiphp_sanitize_bus(struct pci_bus *bus);
 static void acpiphp_set_hpp_values(struct pci_bus *bus);
 static void handle_hotplug_event_func(acpi_handle handle, u32 type, void *context);
+static void free_bridge(struct kref *kref);
 
 /* callback routine to check for the existence of a pci dock device */
 static acpi_status
@@ -76,6 +78,39 @@ is_pci_dock_device(acpi_handle handle, u32 lvl, void *context, void **rv)
 	}
 }
 
+static inline void get_bridge(struct acpiphp_bridge *bridge)
+{
+	kref_get(&bridge->ref);
+}
+
+static inline void put_bridge(struct acpiphp_bridge *bridge)
+{
+	kref_put(&bridge->ref, free_bridge);
+}
+
+static void free_bridge(struct kref *kref)
+{
+	struct acpiphp_bridge *bridge;
+	struct acpiphp_slot *slot, *next;
+	struct acpiphp_func *func, *tmp;
+
+	bridge = container_of(kref, struct acpiphp_bridge, ref);
+
+	list_for_each_entry_safe(slot, next, &bridge->slots, node) {
+		list_for_each_entry_safe(func, tmp, &slot->funcs, sibling) {
+			kfree(func);
+		}
+		kfree(slot);
+	}
+
+	/* Release reference acquired by acpiphp_bridge_handle_to_function() */
+	if ((bridge->flags & BRIDGE_HAS_EJ0) && bridge->func)
+		put_bridge(bridge->func->slot->bridge);
+	put_device(&bridge->pci_bus->dev);
+	pci_dev_put(bridge->pci_dev);
+	kfree(bridge);
+}
+
 /*
  * the _DCK method can do funny things... and sometimes not
  * hah-hah funny.
@@ -171,7 +206,7 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
 	device = (adr >> 16) & 0xffff;
 	function = adr & 0xffff;
 
-	pdev = pbus->self;
+	pdev = bridge->pci_dev;
 	if (pdev && device_is_managed_by_native_pciehp(pdev))
 		return AE_OK;
 
@@ -179,7 +214,6 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
 	if (!newfunc)
 		return AE_NO_MEMORY;
 
-	INIT_LIST_HEAD(&newfunc->sibling);
 	newfunc->handle = handle;
 	newfunc->function = function;
 
@@ -229,7 +263,9 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
 		INIT_LIST_HEAD(&slot->funcs);
 		mutex_init(&slot->crit_sect);
 
+		mutex_lock(&bridge_mutex);
 		list_add_tail(&slot->node, &bridge->slots);
+		mutex_unlock(&bridge_mutex);
 		bridge->nr_slots++;
 
 		dbg("found ACPI PCI Hotplug slot %llu at PCI %04x:%02x:%02x\n",
@@ -247,7 +283,9 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
 	}
 
 	newfunc->slot = slot;
+	mutex_lock(&bridge_mutex);
 	list_add_tail(&newfunc->sibling, &slot->funcs);
+	mutex_unlock(&bridge_mutex);
 
 	if (pci_bus_read_dev_vendor_id(pbus, PCI_DEVFN(device, function),
 				       &val, 60*1000))
@@ -288,7 +326,9 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
 
  err_exit:
 	bridge->nr_slots--;
+	mutex_lock(&bridge_mutex);
 	list_del(&slot->node);
+	mutex_unlock(&bridge_mutex);
 	kfree(slot);
 	kfree(newfunc);
 
@@ -313,13 +353,17 @@ static void init_bridge_misc(struct acpiphp_bridge *bridge)
 	acpi_status status;
 
 	/* must be added to the list prior to calling register_slot */
+	mutex_lock(&bridge_mutex);
 	list_add(&bridge->list, &bridge_list);
+	mutex_unlock(&bridge_mutex);
 
 	/* register all slot objects under this bridge */
 	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, bridge->handle, (u32)1,
 				     register_slot, NULL, bridge, NULL);
 	if (ACPI_FAILURE(status)) {
+		mutex_lock(&bridge_mutex);
 		list_del(&bridge->list);
+		mutex_unlock(&bridge_mutex);
 		return;
 	}
 
@@ -349,16 +393,21 @@ static struct acpiphp_func *acpiphp_bridge_handle_to_function(acpi_handle handle
 {
 	struct acpiphp_bridge *bridge;
 	struct acpiphp_slot *slot;
-	struct acpiphp_func *func;
+	struct acpiphp_func *func = NULL;
 
+	mutex_lock(&bridge_mutex);
 	list_for_each_entry(bridge, &bridge_list, list) {
 		list_for_each_entry(slot, &bridge->slots, node) {
 			list_for_each_entry(func, &slot->funcs, sibling) {
-				if (func->handle == handle)
+				if (func->handle == handle) {
+					get_bridge(func->slot->bridge);
+					mutex_unlock(&bridge_mutex);
 					return func;
+				}
 			}
 		}
 	}
+	mutex_unlock(&bridge_mutex);
 
 	return NULL;
 }
@@ -368,17 +417,22 @@ static struct acpiphp_bridge *acpiphp_handle_to_bridge(acpi_handle handle)
 {
 	struct acpiphp_bridge *bridge;
 
+	mutex_lock(&bridge_mutex);
 	list_for_each_entry(bridge, &bridge_list, list)
-		if (bridge->handle == handle)
+		if (bridge->handle == handle) {
+			get_bridge(bridge);
+			mutex_unlock(&bridge_mutex);
 			return bridge;
+		}
+	mutex_unlock(&bridge_mutex);
 
 	return NULL;
 }
 
 static void cleanup_bridge(struct acpiphp_bridge *bridge)
 {
-	struct acpiphp_slot *slot, *next;
-	struct acpiphp_func *func, *tmp;
+	struct acpiphp_slot *slot;
+	struct acpiphp_func *func;
 	acpi_status status;
 	acpi_handle handle = bridge->handle;
 
@@ -399,8 +453,8 @@ static void cleanup_bridge(struct acpiphp_bridge *bridge)
 			err("failed to install interrupt notify handler\n");
 	}
 
-	list_for_each_entry_safe(slot, next, &bridge->slots, node) {
-		list_for_each_entry_safe(func, tmp, &slot->funcs, sibling) {
+	list_for_each_entry(slot, &bridge->slots, node) {
+		list_for_each_entry(func, &slot->funcs, sibling) {
 			if (is_dock_device(func->handle)) {
 				unregister_hotplug_dock_device(func->handle);
 				unregister_dock_notifier(&func->nb);
@@ -412,18 +466,13 @@ static void cleanup_bridge(struct acpiphp_bridge *bridge)
 				if (ACPI_FAILURE(status))
 					err("failed to remove notify handler\n");
 			}
-			list_del(&func->sibling);
-			kfree(func);
 		}
 		acpiphp_unregister_hotplug_slot(slot);
-		list_del(&slot->funcs);
-		kfree(slot);
 	}
 
-	put_device(&bridge->pci_bus->dev);
-	pci_dev_put(bridge->pci_dev);
+	mutex_lock(&bridge_mutex);
 	list_del(&bridge->list);
-	kfree(bridge);
+	mutex_unlock(&bridge_mutex);
 }
 
 static int power_on_slot(struct acpiphp_slot *slot)
@@ -624,7 +673,6 @@ static int __ref enable_device(struct acpiphp_slot *slot)
 	struct pci_dev *dev;
 	struct pci_bus *bus = slot->bridge->pci_bus;
 	struct acpiphp_func *func;
-	int retval = 0;
 	int num, max, pass;
 
 	if (slot->flags & SLOT_ENABLED)
@@ -684,7 +732,7 @@ static int __ref enable_device(struct acpiphp_slot *slot)
 
 
  err_exit:
-	return retval;
+	return 0;
 }
 
 /* return first device in slot, acquiring a reference on it */
@@ -901,6 +949,7 @@ check_sub_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
 		dbg("%s: re-enumerating slots under %s\n",
 			__func__, objname);
 		acpiphp_check_bridge(bridge);
+		put_bridge(bridge);
 	}
 	return AE_OK ;
 }
@@ -975,6 +1024,7 @@ static void _handle_hotplug_event_bridge(struct work_struct *work)
 	}
 
 	kfree(hp_work); /* allocated in handle_hotplug_event_bridge */
+	put_bridge(bridge);
 }
 
 /**
@@ -988,6 +1038,8 @@ static void _handle_hotplug_event_bridge(struct work_struct *work)
 static void handle_hotplug_event_bridge(acpi_handle handle, u32 type,
 					void *context)
 {
+	struct acpiphp_bridge *bridge = context;
+
 	/*
 	 * Currently the code adds all hotplug events to the kacpid_wq
 	 * queue when it should add hotplug events to the kacpi_hotplug_wq.
@@ -996,6 +1048,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.
 	 */
+	get_bridge(bridge);
 	alloc_acpi_hp_work(handle, type, context, _handle_hotplug_event_bridge);
 }
 
@@ -1047,6 +1100,7 @@ static void _handle_hotplug_event_func(struct work_struct *work)
 	}
 
 	kfree(hp_work); /* allocated in handle_hotplug_event_func */
+	put_bridge(func->slot->bridge);
 }
 
 /**
@@ -1060,6 +1114,8 @@ static void _handle_hotplug_event_func(struct work_struct *work)
 static void handle_hotplug_event_func(acpi_handle handle, u32 type,
 				      void *context)
 {
+	struct acpiphp_func *func = context;
+
 	/*
 	 * Currently the code adds all hotplug events to the kacpid_wq
 	 * queue when it should add hotplug events to the kacpi_hotplug_wq.
@@ -1068,6 +1124,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.
 	 */
+	get_bridge(func->slot->bridge);
 	alloc_acpi_hp_work(handle, type, context, _handle_hotplug_event_func);
 }
 
@@ -1090,6 +1147,7 @@ void acpiphp_enumerate_slots(struct pci_bus *bus, acpi_handle handle)
 	}
 
 	INIT_LIST_HEAD(&bridge->slots);
+	kref_init(&bridge->ref);
 	bridge->handle = handle;
 	bridge->pci_dev = pci_dev_get(bus->self);
 	bridge->pci_bus = bus;
@@ -1120,6 +1178,7 @@ void acpiphp_remove_slots(struct pci_bus *bus)
 	list_for_each_entry_safe(bridge, tmp, &bridge_list, list)
 		if (bridge->pci_bus == bus) {
 			cleanup_bridge(bridge);
+			put_bridge(bridge);
 			break;
 		}
 }
-- 
1.7.9.5

[PATCH v8 13/13] PCI, ACPI: remove support of ACPI PCI subdrivers

[Jiang Liu]

From: Myron Stowe <myron.stowe@redhat.com>

Both sub-drivers of the "PCI Root Bridge ("pci_bridge")" driver, "acpiphp"
and "pci_slot", have been converted to hook directly into the PCI core.

With the conversions there are no remaining usages of the 'struct
acpi_pci_driver' list based infrastructure.  This patch removes it.

Signed-off-by: Myron Stowe <myron.stowe@redhat.com>
Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Cc: Toshi Kani <toshi.kani@hp.com>
Cc: linux-pci@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/acpi/pci_root.c |   48 +----------------------------------------------
 include/linux/acpi.h    |    9 ---------
 2 files changed, 1 insertion(+), 56 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 8b5a73b..2ef0c91 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -69,44 +69,12 @@ static struct acpi_driver acpi_pci_root_driver = {
 		},
 };
 
-/* Lock to protect both acpi_pci_roots and acpi_pci_drivers lists */
+/* Lock to protect both acpi_pci_roots lists */
 static DEFINE_MUTEX(acpi_pci_root_lock);
 static LIST_HEAD(acpi_pci_roots);
-static LIST_HEAD(acpi_pci_drivers);
 
 static DEFINE_MUTEX(osc_lock);
 
-int acpi_pci_register_driver(struct acpi_pci_driver *driver)
-{
-	int n = 0;
-	struct acpi_pci_root *root;
-
-	mutex_lock(&acpi_pci_root_lock);
-	list_add_tail(&driver->node, &acpi_pci_drivers);
-	if (driver->add)
-		list_for_each_entry(root, &acpi_pci_roots, node) {
-			driver->add(root);
-			n++;
-		}
-	mutex_unlock(&acpi_pci_root_lock);
-
-	return n;
-}
-EXPORT_SYMBOL(acpi_pci_register_driver);
-
-void acpi_pci_unregister_driver(struct acpi_pci_driver *driver)
-{
-	struct acpi_pci_root *root;
-
-	mutex_lock(&acpi_pci_root_lock);
-	list_del(&driver->node);
-	if (driver->remove)
-		list_for_each_entry(root, &acpi_pci_roots, node)
-			driver->remove(root);
-	mutex_unlock(&acpi_pci_root_lock);
-}
-EXPORT_SYMBOL(acpi_pci_unregister_driver);
-
 /**
  * acpi_is_root_bridge - determine whether an ACPI CA node is a PCI root bridge
  * @handle - the ACPI CA node in question.
@@ -416,7 +384,6 @@ static int acpi_pci_root_add(struct acpi_device *device)
 	acpi_status status;
 	int result;
 	struct acpi_pci_root *root;
-	struct acpi_pci_driver *driver;
 	u32 flags, base_flags;
 	bool is_osc_granted = false;
 
@@ -576,12 +543,6 @@ static int acpi_pci_root_add(struct acpi_device *device)
 		pci_assign_unassigned_bus_resources(root->bus);
 	}
 
-	mutex_lock(&acpi_pci_root_lock);
-	list_for_each_entry(driver, &acpi_pci_drivers, node)
-		if (driver->add)
-			driver->add(root);
-	mutex_unlock(&acpi_pci_root_lock);
-
 	/* need to after hot-added ioapic is registered */
 	if (system_state != SYSTEM_BOOTING)
 		pci_enable_bridges(root->bus);
@@ -602,16 +563,9 @@ end:
 static int acpi_pci_root_remove(struct acpi_device *device, int type)
 {
 	struct acpi_pci_root *root = acpi_driver_data(device);
-	struct acpi_pci_driver *driver;
 
 	pci_stop_root_bus(root->bus);
 
-	mutex_lock(&acpi_pci_root_lock);
-	list_for_each_entry_reverse(driver, &acpi_pci_drivers, node)
-		if (driver->remove)
-			driver->remove(root);
-	mutex_unlock(&acpi_pci_root_lock);
-
 	device_set_run_wake(root->bus->bridge, false);
 	pci_acpi_remove_bus_pm_notifier(device);
 
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 8c1d6f2..eeb5600 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -147,15 +147,6 @@ void acpi_penalize_isa_irq(int irq, int active);
 
 void acpi_pci_irq_disable (struct pci_dev *dev);
 
-struct acpi_pci_driver {
-	struct list_head node;
-	int (*add)(struct acpi_pci_root *root);
-	void (*remove)(struct acpi_pci_root *root);
-};
-
-int acpi_pci_register_driver(struct acpi_pci_driver *driver);
-void acpi_pci_unregister_driver(struct acpi_pci_driver *driver);
-
 extern int ec_read(u8 addr, u8 *val);
 extern int ec_write(u8 addr, u8 val);
 extern int ec_transaction(u8 command,
-- 
1.7.9.5

Re: [PATCH v8 13/13] PCI, ACPI: remove support of ACPI PCI subdrivers

[Yinghai Lu]

On Tue, Feb 26, 2013 at 7:25 AM, Jiang Liu <liuj97@gmail.com> wrote:
> From: Myron Stowe <myron.stowe@redhat.com>
>
> Both sub-drivers of the "PCI Root Bridge ("pci_bridge")" driver, "acpiphp"
> and "pci_slot", have been converted to hook directly into the PCI core.
>
> With the conversions there are no remaining usages of the 'struct
> acpi_pci_driver' list based infrastructure.  This patch removes it.
>
> Signed-off-by: Myron Stowe <myron.stowe@redhat.com>
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> Cc: Yinghai Lu <yinghai@kernel.org>
> Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> Cc: Toshi Kani <toshi.kani@hp.com>
> Cc: linux-pci@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org

Nice cleanup!

for 1-13,
Reviewed-by: Yinghai Lu <yinghai@kernel.org>

Thanks

Yinghai

> ---
>  drivers/acpi/pci_root.c |   48 +----------------------------------------------
>  include/linux/acpi.h    |    9 ---------
>  2 files changed, 1 insertion(+), 56 deletions(-)
>
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 8b5a73b..2ef0c91 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -69,44 +69,12 @@ static struct acpi_driver acpi_pci_root_driver = {
>                 },
>  };
>
> -/* Lock to protect both acpi_pci_roots and acpi_pci_drivers lists */
> +/* Lock to protect both acpi_pci_roots lists */
>  static DEFINE_MUTEX(acpi_pci_root_lock);
>  static LIST_HEAD(acpi_pci_roots);
> -static LIST_HEAD(acpi_pci_drivers);
>
>  static DEFINE_MUTEX(osc_lock);
>
> -int acpi_pci_register_driver(struct acpi_pci_driver *driver)
> -{
> -       int n = 0;
> -       struct acpi_pci_root *root;
> -
> -       mutex_lock(&acpi_pci_root_lock);
> -       list_add_tail(&driver->node, &acpi_pci_drivers);
> -       if (driver->add)
> -               list_for_each_entry(root, &acpi_pci_roots, node) {
> -                       driver->add(root);
> -                       n++;
> -               }
> -       mutex_unlock(&acpi_pci_root_lock);
> -
> -       return n;
> -}
> -EXPORT_SYMBOL(acpi_pci_register_driver);
> -
> -void acpi_pci_unregister_driver(struct acpi_pci_driver *driver)
> -{
> -       struct acpi_pci_root *root;
> -
> -       mutex_lock(&acpi_pci_root_lock);
> -       list_del(&driver->node);
> -       if (driver->remove)
> -               list_for_each_entry(root, &acpi_pci_roots, node)
> -                       driver->remove(root);
> -       mutex_unlock(&acpi_pci_root_lock);
> -}
> -EXPORT_SYMBOL(acpi_pci_unregister_driver);
> -
>  /**
>   * acpi_is_root_bridge - determine whether an ACPI CA node is a PCI root bridge
>   * @handle - the ACPI CA node in question.
> @@ -416,7 +384,6 @@ static int acpi_pci_root_add(struct acpi_device *device)
>         acpi_status status;
>         int result;
>         struct acpi_pci_root *root;
> -       struct acpi_pci_driver *driver;
>         u32 flags, base_flags;
>         bool is_osc_granted = false;
>
> @@ -576,12 +543,6 @@ static int acpi_pci_root_add(struct acpi_device *device)
>                 pci_assign_unassigned_bus_resources(root->bus);
>         }
>
> -       mutex_lock(&acpi_pci_root_lock);
> -       list_for_each_entry(driver, &acpi_pci_drivers, node)
> -               if (driver->add)
> -                       driver->add(root);
> -       mutex_unlock(&acpi_pci_root_lock);
> -
>         /* need to after hot-added ioapic is registered */
>         if (system_state != SYSTEM_BOOTING)
>                 pci_enable_bridges(root->bus);
> @@ -602,16 +563,9 @@ end:
>  static int acpi_pci_root_remove(struct acpi_device *device, int type)
>  {
>         struct acpi_pci_root *root = acpi_driver_data(device);
> -       struct acpi_pci_driver *driver;
>
>         pci_stop_root_bus(root->bus);
>
> -       mutex_lock(&acpi_pci_root_lock);
> -       list_for_each_entry_reverse(driver, &acpi_pci_drivers, node)
> -               if (driver->remove)
> -                       driver->remove(root);
> -       mutex_unlock(&acpi_pci_root_lock);
> -
>         device_set_run_wake(root->bus->bridge, false);
>         pci_acpi_remove_bus_pm_notifier(device);
>
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 8c1d6f2..eeb5600 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -147,15 +147,6 @@ void acpi_penalize_isa_irq(int irq, int active);
>
>  void acpi_pci_irq_disable (struct pci_dev *dev);
>
> -struct acpi_pci_driver {
> -       struct list_head node;
> -       int (*add)(struct acpi_pci_root *root);
> -       void (*remove)(struct acpi_pci_root *root);
> -};
> -
> -int acpi_pci_register_driver(struct acpi_pci_driver *driver);
> -void acpi_pci_unregister_driver(struct acpi_pci_driver *driver);
> -
>  extern int ec_read(u8 addr, u8 *val);
>  extern int ec_write(u8 addr, u8 val);
>  extern int ec_transaction(u8 command,
> --
> 1.7.9.5
>

Re: [PATCH v8 13/13] PCI, ACPI: remove support of ACPI PCI subdrivers

[Jiang Liu]

On 2013-2-27 3:07, Yinghai Lu wrote:
> On Tue, Feb 26, 2013 at 7:25 AM, Jiang Liu <liuj97@gmail.com> wrote:
>> From: Myron Stowe <myron.stowe@redhat.com>
>>
>> Both sub-drivers of the "PCI Root Bridge ("pci_bridge")" driver, "acpiphp"
>> and "pci_slot", have been converted to hook directly into the PCI core.
>>
>> With the conversions there are no remaining usages of the 'struct
>> acpi_pci_driver' list based infrastructure.  This patch removes it.
>>
>> Signed-off-by: Myron Stowe <myron.stowe@redhat.com>
>> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
>> Cc: Yinghai Lu <yinghai@kernel.org>
>> Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
>> Cc: Toshi Kani <toshi.kani@hp.com>
>> Cc: linux-pci@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
> 
> Nice cleanup!
> 
> for 1-13,
> Reviewed-by: Yinghai Lu <yinghai@kernel.org>
HiYinghai,
	Thanks for review!
	Regards!
	Gerry

> 
> Thanks
> 
> Yinghai
> 
>> ---
>>  drivers/acpi/pci_root.c |   48 +----------------------------------------------
>>  include/linux/acpi.h    |    9 ---------
>>  2 files changed, 1 insertion(+), 56 deletions(-)
>>
>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>> index 8b5a73b..2ef0c91 100644
>> --- a/drivers/acpi/pci_root.c
>> +++ b/drivers/acpi/pci_root.c
>> @@ -69,44 +69,12 @@ static struct acpi_driver acpi_pci_root_driver = {
>>                 },
>>  };
>>
>> -/* Lock to protect both acpi_pci_roots and acpi_pci_drivers lists */
>> +/* Lock to protect both acpi_pci_roots lists */
>>  static DEFINE_MUTEX(acpi_pci_root_lock);
>>  static LIST_HEAD(acpi_pci_roots);
>> -static LIST_HEAD(acpi_pci_drivers);
>>
>>  static DEFINE_MUTEX(osc_lock);
>>
>> -int acpi_pci_register_driver(struct acpi_pci_driver *driver)
>> -{
>> -       int n = 0;
>> -       struct acpi_pci_root *root;
>> -
>> -       mutex_lock(&acpi_pci_root_lock);
>> -       list_add_tail(&driver->node, &acpi_pci_drivers);
>> -       if (driver->add)
>> -               list_for_each_entry(root, &acpi_pci_roots, node) {
>> -                       driver->add(root);
>> -                       n++;
>> -               }
>> -       mutex_unlock(&acpi_pci_root_lock);
>> -
>> -       return n;
>> -}
>> -EXPORT_SYMBOL(acpi_pci_register_driver);
>> -
>> -void acpi_pci_unregister_driver(struct acpi_pci_driver *driver)
>> -{
>> -       struct acpi_pci_root *root;
>> -
>> -       mutex_lock(&acpi_pci_root_lock);
>> -       list_del(&driver->node);
>> -       if (driver->remove)
>> -               list_for_each_entry(root, &acpi_pci_roots, node)
>> -                       driver->remove(root);
>> -       mutex_unlock(&acpi_pci_root_lock);
>> -}
>> -EXPORT_SYMBOL(acpi_pci_unregister_driver);
>> -
>>  /**
>>   * acpi_is_root_bridge - determine whether an ACPI CA node is a PCI root bridge
>>   * @handle - the ACPI CA node in question.
>> @@ -416,7 +384,6 @@ static int acpi_pci_root_add(struct acpi_device *device)
>>         acpi_status status;
>>         int result;
>>         struct acpi_pci_root *root;
>> -       struct acpi_pci_driver *driver;
>>         u32 flags, base_flags;
>>         bool is_osc_granted = false;
>>
>> @@ -576,12 +543,6 @@ static int acpi_pci_root_add(struct acpi_device *device)
>>                 pci_assign_unassigned_bus_resources(root->bus);
>>         }
>>
>> -       mutex_lock(&acpi_pci_root_lock);
>> -       list_for_each_entry(driver, &acpi_pci_drivers, node)
>> -               if (driver->add)
>> -                       driver->add(root);
>> -       mutex_unlock(&acpi_pci_root_lock);
>> -
>>         /* need to after hot-added ioapic is registered */
>>         if (system_state != SYSTEM_BOOTING)
>>                 pci_enable_bridges(root->bus);
>> @@ -602,16 +563,9 @@ end:
>>  static int acpi_pci_root_remove(struct acpi_device *device, int type)
>>  {
>>         struct acpi_pci_root *root = acpi_driver_data(device);
>> -       struct acpi_pci_driver *driver;
>>
>>         pci_stop_root_bus(root->bus);
>>
>> -       mutex_lock(&acpi_pci_root_lock);
>> -       list_for_each_entry_reverse(driver, &acpi_pci_drivers, node)
>> -               if (driver->remove)
>> -                       driver->remove(root);
>> -       mutex_unlock(&acpi_pci_root_lock);
>> -
>>         device_set_run_wake(root->bus->bridge, false);
>>         pci_acpi_remove_bus_pm_notifier(device);
>>
>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>> index 8c1d6f2..eeb5600 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -147,15 +147,6 @@ void acpi_penalize_isa_irq(int irq, int active);
>>
>>  void acpi_pci_irq_disable (struct pci_dev *dev);
>>
>> -struct acpi_pci_driver {
>> -       struct list_head node;
>> -       int (*add)(struct acpi_pci_root *root);
>> -       void (*remove)(struct acpi_pci_root *root);
>> -};
>> -
>> -int acpi_pci_register_driver(struct acpi_pci_driver *driver);
>> -void acpi_pci_unregister_driver(struct acpi_pci_driver *driver);
>> -
>>  extern int ec_read(u8 addr, u8 *val);
>>  extern int ec_write(u8 addr, u8 val);
>>  extern int ec_transaction(u8 command,
>> --
>> 1.7.9.5
>>
> 
> .
> 

Re: [PATCH v8 10/13] PCI/acpiphp: do not use ACPI PCI subdriver mechanism

[Bjorn Helgaas]

On Tue, Feb 26, 2013 at 8:25 AM, Jiang Liu <liuj97@gmail.com> wrote:
> Previously the acpiphp driver registers itself as an ACPI PCI subdriver,
> so it's callbacks will be invoked when creating/destroying PCI root
> buses to manage ACPI based PCI hotplug slots. But it doesn't handle
> P2P bridge hotplug events, so it will cause strange behavours if there
> are hotplug slots associated with a hot-removed P2P bridge.
>
> So this patch tries to fix this issue by:
> 1) Make acpiphp built-in only, not a module.
> 2) Directly hook into PCI core to update hotplug slot devices when
>    creating/destroying PCI buses through:
>         pci_{add|remove}_bus() -> acpi_pci_{add|remove}_bus()
> 3) Get rid of ACPI PCI subdriver related code, it's not used any more.
>
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> Cc: Yinghai Lu <yinghai@kernel.org>
> Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> Cc: Toshi Kani <toshi.kani@hp.com>
> Cc: linux-pci@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/pci/hotplug/Kconfig        |    7 +-
>  drivers/pci/hotplug/acpiphp.h      |    3 -
>  drivers/pci/hotplug/acpiphp_core.c |   23 +--
>  drivers/pci/hotplug/acpiphp_glue.c |  282 +++++++-----------------------------
>  drivers/pci/pci-acpi.c             |    3 +
>  include/linux/pci-acpi.h           |   14 +-
>  6 files changed, 67 insertions(+), 265 deletions(-)
>
> diff --git a/drivers/pci/hotplug/Kconfig b/drivers/pci/hotplug/Kconfig
> index 13e9e63..9fcb87f 100644
> --- a/drivers/pci/hotplug/Kconfig
> +++ b/drivers/pci/hotplug/Kconfig
> @@ -52,15 +52,12 @@ config HOTPLUG_PCI_IBM
>           When in doubt, say N.
>
>  config HOTPLUG_PCI_ACPI
> -       tristate "ACPI PCI Hotplug driver"
> -       depends on (!ACPI_DOCK && ACPI) || (ACPI_DOCK)
> +       bool "ACPI PCI Hotplug driver"

My goal is that a user should never have to specify a kernel boot
parameter or edit a modules.conf file, but the user did previously
have some way to influence whether we use pciehp or acpiphp.  I know
we still have some issues, particularly with acpiphp, so I'm a little
concerned that by removing the CONFIG_HOTPLUG_PCI_ACPI=m, we might be
removing a way to work around those issues.

A distro that previously used CONFIG_HOTPLUG_PCI_ACPI=m will now have
to use =y, so modules.conf is no longer applicable.  Can you convince
me that the user still has a way to work around issues?  I spent quite
a while trying to understand the pciehp/acpiphp dependencies, but it's
pretty tangled web.

I would definitely like to see the changelog mention this issue and
any changes a user might have to make, e.g., to replace a modules.conf
edit.  It might be that this involves the "pciehp_force" option, and
if it does, we should probably add a mention of this in
Documentation/kernel-parameters.txt.

Can the Kconfig change be split into its own small patch without all
the other stuff, or are they inseparable?

Bjorn

> +       depends on HOTPLUG_PCI=y && ((!ACPI_DOCK && ACPI) || (ACPI_DOCK))
>         help
>           Say Y here if you have a system that supports PCI Hotplug using
>           ACPI.
>
> -         To compile this driver as a module, choose M here: the
> -         module will be called acpiphp.
> -
>           When in doubt, say N.
>
>  config HOTPLUG_PCI_ACPI_IBM
> diff --git a/drivers/pci/hotplug/acpiphp.h b/drivers/pci/hotplug/acpiphp.h
> index 1b311f9..69b4aac 100644
> --- a/drivers/pci/hotplug/acpiphp.h
> +++ b/drivers/pci/hotplug/acpiphp.h
> @@ -119,7 +119,6 @@ struct acpiphp_slot {
>   */
>  struct acpiphp_func {
>         struct acpiphp_slot *slot;      /* parent */
> -       struct acpiphp_bridge *bridge;  /* Ejectable PCI-to-PCI bridge */
>
>         struct list_head sibling;
>         struct notifier_block nb;
> @@ -176,8 +175,6 @@ extern int acpiphp_register_hotplug_slot(struct acpiphp_slot *slot);
>  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);
>  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 c2fd309..80d777c 100644
> --- a/drivers/pci/hotplug/acpiphp_core.c
> +++ b/drivers/pci/hotplug/acpiphp_core.c
> @@ -37,6 +37,7 @@
>
>  #include <linux/kernel.h>
>  #include <linux/pci.h>
> +#include <linux/pci-acpi.h>
>  #include <linux/pci_hotplug.h>
>  #include <linux/slab.h>
>  #include <linux/smp.h>
> @@ -351,27 +352,7 @@ void acpiphp_unregister_hotplug_slot(struct acpiphp_slot *acpiphp_slot)
>  }
>
>
> -static int __init acpiphp_init(void)
> +void __init acpiphp_init(void)
>  {
>         info(DRIVER_DESC " version: " DRIVER_VERSION "\n");
> -
> -       if (acpi_pci_disabled)
> -               return 0;
> -
> -       /* read all the ACPI info from the system */
> -       /* initialize internal data structure etc. */
> -       return acpiphp_glue_init();
> -}
> -
> -
> -static void __exit acpiphp_exit(void)
> -{
> -       if (acpi_pci_disabled)
> -               return;
> -
> -       /* deallocate internal data structures etc. */
> -       acpiphp_glue_exit();
>  }
> -
> -module_init(acpiphp_init);
> -module_exit(acpiphp_exit);
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index 89e0c36..3db84b6 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -157,6 +157,7 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
>         int device, function, retval;
>         struct pci_bus *pbus = bridge->pci_bus;
>         struct pci_dev *pdev;
> +       u32 val;
>
>         if (!acpi_pci_check_ejectable(pbus, handle) && !is_dock_device(handle))
>                 return AE_OK;
> @@ -249,11 +250,9 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
>         newfunc->slot = slot;
>         list_add_tail(&newfunc->sibling, &slot->funcs);
>
> -       pdev = pci_get_slot(pbus, PCI_DEVFN(device, function));
> -       if (pdev) {
> +       if (pci_bus_read_dev_vendor_id(pbus, PCI_DEVFN(device, function),
> +                                      &val, 60*1000))
>                 slot->flags |= (SLOT_ENABLED | SLOT_POWEREDON);
> -               pci_dev_put(pdev);
> -       }
>
>         if (is_dock_device(handle)) {
>                 /* we don't want to call this device's _EJ0
> @@ -366,148 +365,6 @@ static struct acpiphp_func *acpiphp_bridge_handle_to_function(acpi_handle handle
>  }
>
>
> -static inline void config_p2p_bridge_flags(struct acpiphp_bridge *bridge)
> -{
> -       acpi_handle dummy_handle;
> -       struct acpiphp_func *func;
> -
> -       if (ACPI_SUCCESS(acpi_get_handle(bridge->handle,
> -                                       "_EJ0", &dummy_handle))) {
> -               bridge->flags |= BRIDGE_HAS_EJ0;
> -
> -               dbg("found ejectable p2p bridge\n");
> -
> -               /* make link between PCI bridge and PCI function */
> -               func = acpiphp_bridge_handle_to_function(bridge->handle);
> -               if (!func)
> -                       return;
> -               bridge->func = func;
> -               func->bridge = bridge;
> -       }
> -}
> -
> -
> -/* allocate and initialize host bridge data structure */
> -static void add_host_bridge(struct acpi_pci_root *root)
> -{
> -       struct acpiphp_bridge *bridge;
> -       acpi_handle handle = root->device->handle;
> -
> -       bridge = kzalloc(sizeof(struct acpiphp_bridge), GFP_KERNEL);
> -       if (bridge == NULL)
> -               return;
> -
> -       bridge->handle = handle;
> -
> -       bridge->pci_bus = root->bus;
> -
> -       init_bridge_misc(bridge);
> -}
> -
> -
> -/* allocate and initialize PCI-to-PCI bridge data structure */
> -static void add_p2p_bridge(acpi_handle *handle)
> -{
> -       struct acpiphp_bridge *bridge;
> -
> -       bridge = kzalloc(sizeof(struct acpiphp_bridge), GFP_KERNEL);
> -       if (bridge == NULL) {
> -               err("out of memory\n");
> -               return;
> -       }
> -
> -       bridge->handle = handle;
> -       config_p2p_bridge_flags(bridge);
> -
> -       bridge->pci_dev = acpi_get_pci_dev(handle);
> -       bridge->pci_bus = bridge->pci_dev->subordinate;
> -       if (!bridge->pci_bus) {
> -               err("This is not a PCI-to-PCI bridge!\n");
> -               goto err;
> -       }
> -
> -       /*
> -        * Grab a ref to the subordinate PCI bus in case the bus is
> -        * removed via PCI core logical hotplug. The ref pins the bus
> -        * (which we access during module unload).
> -        */
> -       get_device(&bridge->pci_bus->dev);
> -
> -       init_bridge_misc(bridge);
> -       return;
> - err:
> -       pci_dev_put(bridge->pci_dev);
> -       kfree(bridge);
> -       return;
> -}
> -
> -
> -/* callback routine to find P2P bridges */
> -static acpi_status
> -find_p2p_bridge(acpi_handle handle, u32 lvl, void *context, void **rv)
> -{
> -       acpi_status status;
> -       struct pci_dev *dev;
> -
> -       dev = acpi_get_pci_dev(handle);
> -       if (!dev || !dev->subordinate)
> -               goto out;
> -
> -       /* check if this bridge has ejectable slots */
> -       if ((detect_ejectable_slots(handle) > 0)) {
> -               dbg("found PCI-to-PCI bridge at PCI %s\n", pci_name(dev));
> -               add_p2p_bridge(handle);
> -       }
> -
> -       /* search P2P bridges under this p2p bridge */
> -       status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
> -                                    find_p2p_bridge, NULL, NULL, NULL);
> -       if (ACPI_FAILURE(status))
> -               warn("find_p2p_bridge failed (error code = 0x%x)\n", status);
> -
> - out:
> -       pci_dev_put(dev);
> -       return AE_OK;
> -}
> -
> -
> -/* find hot-pluggable slots, and then find P2P bridge */
> -static int add_bridge(struct acpi_pci_root *root)
> -{
> -       acpi_status status;
> -       unsigned long long tmp;
> -       acpi_handle dummy_handle;
> -       acpi_handle handle = root->device->handle;
> -
> -       /* if the bridge doesn't have _STA, we assume it is always there */
> -       status = acpi_get_handle(handle, "_STA", &dummy_handle);
> -       if (ACPI_SUCCESS(status)) {
> -               status = acpi_evaluate_integer(handle, "_STA", NULL, &tmp);
> -               if (ACPI_FAILURE(status)) {
> -                       dbg("%s: _STA evaluation failure\n", __func__);
> -                       return 0;
> -               }
> -               if ((tmp & ACPI_STA_DEVICE_FUNCTIONING) == 0)
> -                       /* don't register this object */
> -                       return 0;
> -       }
> -
> -       /* check if this bridge has ejectable slots */
> -       if (detect_ejectable_slots(handle) > 0) {
> -               dbg("found PCI host-bus bridge with hot-pluggable slots\n");
> -               add_host_bridge(root);
> -       }
> -
> -       /* search P2P bridges under this host bridge */
> -       status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
> -                                    find_p2p_bridge, NULL, NULL, NULL);
> -
> -       if (ACPI_FAILURE(status))
> -               warn("find_p2p_bridge failed (error code = 0x%x)\n", status);
> -
> -       return 0;
> -}
> -
>  static struct acpiphp_bridge *acpiphp_handle_to_bridge(acpi_handle handle)
>  {
>         struct acpiphp_bridge *bridge;
> @@ -567,56 +424,12 @@ static void cleanup_bridge(struct acpiphp_bridge *bridge)
>                 slot = next;
>         }
>
> -       /*
> -        * Only P2P bridges have a pci_dev
> -        */
> -       if (bridge->pci_dev)
> -               put_device(&bridge->pci_bus->dev);
> -
> +       put_device(&bridge->pci_bus->dev);
>         pci_dev_put(bridge->pci_dev);
>         list_del(&bridge->list);
>         kfree(bridge);
>  }
>
> -static acpi_status
> -cleanup_p2p_bridge(acpi_handle handle, u32 lvl, void *context, void **rv)
> -{
> -       struct acpiphp_bridge *bridge;
> -
> -       /* cleanup p2p bridges under this P2P bridge
> -          in a depth-first manner */
> -       acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
> -                               cleanup_p2p_bridge, NULL, NULL, NULL);
> -
> -       bridge = acpiphp_handle_to_bridge(handle);
> -       if (bridge)
> -               cleanup_bridge(bridge);
> -
> -       return AE_OK;
> -}
> -
> -static void remove_bridge(struct acpi_pci_root *root)
> -{
> -       struct acpiphp_bridge *bridge;
> -       acpi_handle handle = root->device->handle;
> -
> -       /* cleanup p2p bridges under this host bridge
> -          in a depth-first manner */
> -       acpi_walk_namespace(ACPI_TYPE_DEVICE, handle,
> -                               (u32)1, cleanup_p2p_bridge, NULL, NULL, NULL);
> -
> -       /*
> -        * On root bridges with hotplug slots directly underneath (ie,
> -        * no p2p bridge between), we call cleanup_bridge().
> -        *
> -        * The else clause cleans up root bridges that either had no
> -        * hotplug slots at all, or had a p2p bridge underneath.
> -        */
> -       bridge = acpiphp_handle_to_bridge(handle);
> -       if (bridge)
> -               cleanup_bridge(bridge);
> -}
> -
>  static int power_on_slot(struct acpiphp_slot *slot)
>  {
>         acpi_status status;
> @@ -802,6 +615,7 @@ static void check_hotplug_bridge(struct acpiphp_slot *slot, struct pci_dev *dev)
>                 }
>         }
>  }
> +
>  /**
>   * enable_device - enable, configure a slot
>   * @slot: slot to be enabled
> @@ -816,7 +630,6 @@ static int __ref enable_device(struct acpiphp_slot *slot)
>         struct acpiphp_func *func;
>         int retval = 0;
>         int num, max, pass;
> -       acpi_status status;
>
>         if (slot->flags & SLOT_ENABLED)
>                 goto err_exit;
> @@ -871,18 +684,6 @@ static int __ref enable_device(struct acpiphp_slot *slot)
>                         slot->flags &= (~SLOT_ENABLED);
>                         continue;
>                 }
> -
> -               if (dev->hdr_type != PCI_HEADER_TYPE_BRIDGE &&
> -                   dev->hdr_type != PCI_HEADER_TYPE_CARDBUS) {
> -                       pci_dev_put(dev);
> -                       continue;
> -               }
> -
> -               status = find_p2p_bridge(func->handle, (u32)1, bus, NULL);
> -               if (ACPI_FAILURE(status))
> -                       warn("find_p2p_bridge failed (error code = 0x%x)\n",
> -                               status);
> -               pci_dev_put(dev);
>         }
>
>
> @@ -916,16 +717,6 @@ static int disable_device(struct acpiphp_slot *slot)
>  {
>         struct acpiphp_func *func;
>         struct pci_dev *pdev;
> -       struct pci_bus *bus = slot->bridge->pci_bus;
> -
> -       list_for_each_entry(func, &slot->funcs, sibling) {
> -               if (func->bridge) {
> -                       /* cleanup p2p bridges under this P2P bridge */
> -                       cleanup_p2p_bridge(func->bridge->handle,
> -                                               (u32)1, NULL, NULL);
> -                       func->bridge = NULL;
> -               }
> -       }
>
>         /*
>          * enable_device() enumerates all functions in this device via
> @@ -944,7 +735,6 @@ static int disable_device(struct acpiphp_slot *slot)
>
>         slot->flags &= (~SLOT_ENABLED);
>
> -err_exit:
>         return 0;
>  }
>
> @@ -1285,30 +1075,56 @@ static void handle_hotplug_event_func(acpi_handle handle, u32 type,
>         alloc_acpi_hp_work(handle, type, context, _handle_hotplug_event_func);
>  }
>
> -static struct acpi_pci_driver acpi_pci_hp_driver = {
> -       .add =          add_bridge,
> -       .remove =       remove_bridge,
> -};
> -
> -/**
> - * acpiphp_glue_init - initializes all PCI hotplug - ACPI glue data structures
> +/*
> + * Create hotplug slots for the PCI bus.
> + * It should always return 0 to avoid skipping following notifiers.
>   */
> -int __init acpiphp_glue_init(void)
> +void acpiphp_enumerate_slots(struct pci_bus *bus, acpi_handle handle)
>  {
> -       acpi_pci_register_driver(&acpi_pci_hp_driver);
> +       acpi_handle dummy_handle;
> +       struct acpiphp_bridge *bridge;
>
> -       return 0;
> -}
> +       if (detect_ejectable_slots(handle) <= 0)
> +               return;
>
> +       bridge = kzalloc(sizeof(struct acpiphp_bridge), GFP_KERNEL);
> +       if (bridge == NULL) {
> +               err("out of memory\n");
> +               return;
> +       }
>
> -/**
> - * acpiphp_glue_exit - terminates all PCI hotplug - ACPI glue data structures
> - *
> - * This function frees all data allocated in acpiphp_glue_init().
> - */
> -void  acpiphp_glue_exit(void)
> +       bridge->handle = handle;
> +       bridge->pci_dev = pci_dev_get(bus->self);
> +       bridge->pci_bus = bus;
> +
> +       /*
> +        * Grab a ref to the subordinate PCI bus in case the bus is
> +        * removed via PCI core logical hotplug. The ref pins the bus
> +        * (which we access during module unload).
> +        */
> +       get_device(&bus->dev);
> +
> +       if (!pci_is_root_bus(bridge->pci_bus) &&
> +           ACPI_SUCCESS(acpi_get_handle(bridge->handle,
> +                                       "_EJ0", &dummy_handle))) {
> +               dbg("found ejectable p2p bridge\n");
> +               bridge->flags |= BRIDGE_HAS_EJ0;
> +               bridge->func = acpiphp_bridge_handle_to_function(handle);
> +       }
> +
> +       init_bridge_misc(bridge);
> +}
> +
> +/* Destroy hotplug slots associated with the PCI bus */
> +void acpiphp_remove_slots(struct pci_bus *bus)
>  {
> -       acpi_pci_unregister_driver(&acpi_pci_hp_driver);
> +       struct acpiphp_bridge *bridge, *tmp;
> +
> +       list_for_each_entry_safe(bridge, tmp, &bridge_list, list)
> +               if (bridge->pci_bus == bus) {
> +                       cleanup_bridge(bridge);
> +                       break;
> +               }
>  }
>
>  /**
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 2dbca38..6fe7bf8 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -297,6 +297,7 @@ void acpi_pci_add_bus(struct pci_bus *bus)
>                 return;
>
>         acpi_pci_slot_enumerate(bus, handle);
> +       acpiphp_enumerate_slots(bus, handle);
>  }
>
>  void acpi_pci_remove_bus(struct pci_bus *bus)
> @@ -308,6 +309,7 @@ void acpi_pci_remove_bus(struct pci_bus *bus)
>         if (acpi_pci_disabled)
>                 return;
>
> +       acpiphp_remove_slots(bus);
>         acpi_pci_slot_remove(bus);
>  }
>
> @@ -384,6 +386,7 @@ static int __init acpi_pci_init(void)
>
>         pci_set_platform_pm(&acpi_pci_platform_pm);
>         acpi_pci_slot_init();
> +       acpiphp_init();
>
>         return 0;
>  }
> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> index 1ef126f..f1b2380 100644
> --- a/include/linux/pci-acpi.h
> +++ b/include/linux/pci-acpi.h
> @@ -56,9 +56,17 @@ static inline void acpi_pci_slot_enumerate(struct pci_bus *bus,
>  static inline void acpi_pci_slot_remove(struct pci_bus *bus) { }
>  #endif
>
> -#else  /* CONFIG_ACPI */
> -static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
> -static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
> +#ifdef CONFIG_HOTPLUG_PCI_ACPI
> +void acpiphp_init(void);
> +void acpiphp_enumerate_slots(struct pci_bus *bus, acpi_handle handle);
> +void acpiphp_remove_slots(struct pci_bus *bus);
> +#else
> +static inline void acpiphp_init(void) { }
> +static inline void acpiphp_enumerate_slots(struct pci_bus *bus,
> +                                          acpi_handle handle) { }
> +static inline void acpiphp_remove_slots(struct pci_bus *bus) { }
> +#endif
> +
>  #endif /* CONFIG_ACPI */
>
>  #ifdef CONFIG_ACPI_APEI
> --
> 1.7.9.5
>

Re: [PATCH v8 10/13] PCI/acpiphp: do not use ACPI PCI subdriver mechanism

[Jiang Liu]

On 04/10/2013 07:38 AM, Bjorn Helgaas wrote:
> On Tue, Feb 26, 2013 at 8:25 AM, Jiang Liu <liuj97@gmail.com> wrote:
>> Previously the acpiphp driver registers itself as an ACPI PCI subdriver,
>> so it's callbacks will be invoked when creating/destroying PCI root
>> buses to manage ACPI based PCI hotplug slots. But it doesn't handle
>> P2P bridge hotplug events, so it will cause strange behavours if there
>> are hotplug slots associated with a hot-removed P2P bridge.
>>
>> So this patch tries to fix this issue by:
>> 1) Make acpiphp built-in only, not a module.
>> 2) Directly hook into PCI core to update hotplug slot devices when
>>    creating/destroying PCI buses through:
>>         pci_{add|remove}_bus() -> acpi_pci_{add|remove}_bus()
>> 3) Get rid of ACPI PCI subdriver related code, it's not used any more.
>>
>> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
>> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
>> Cc: Yinghai Lu <yinghai@kernel.org>
>> Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
>> Cc: Toshi Kani <toshi.kani@hp.com>
>> Cc: linux-pci@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> ---
>>  drivers/pci/hotplug/Kconfig        |    7 +-
>>  drivers/pci/hotplug/acpiphp.h      |    3 -
>>  drivers/pci/hotplug/acpiphp_core.c |   23 +--
>>  drivers/pci/hotplug/acpiphp_glue.c |  282 +++++++-----------------------------
>>  drivers/pci/pci-acpi.c             |    3 +
>>  include/linux/pci-acpi.h           |   14 +-
>>  6 files changed, 67 insertions(+), 265 deletions(-)
>>
>> diff --git a/drivers/pci/hotplug/Kconfig b/drivers/pci/hotplug/Kconfig
>> index 13e9e63..9fcb87f 100644
>> --- a/drivers/pci/hotplug/Kconfig
>> +++ b/drivers/pci/hotplug/Kconfig
>> @@ -52,15 +52,12 @@ config HOTPLUG_PCI_IBM
>>           When in doubt, say N.
>>
>>  config HOTPLUG_PCI_ACPI
>> -       tristate "ACPI PCI Hotplug driver"
>> -       depends on (!ACPI_DOCK && ACPI) || (ACPI_DOCK)
>> +       bool "ACPI PCI Hotplug driver"
> 
Hi Bjorn,
	Thanks for review.

> My goal is that a user should never have to specify a kernel boot
> parameter or edit a modules.conf file, but the user did previously
> have some way to influence whether we use pciehp or acpiphp.  I know
> we still have some issues, particularly with acpiphp, so I'm a little
> concerned that by removing the CONFIG_HOTPLUG_PCI_ACPI=m, we might be
> removing a way to work around those issues.
> 
> A distro that previously used CONFIG_HOTPLUG_PCI_ACPI=m will now have
> to use =y, so modules.conf is no longer applicable.  Can you convince
> me that the user still has a way to work around issues?  I spent quite
> a while trying to understand the pciehp/acpiphp dependencies, but it's
> pretty tangled web.
I will try my best to explain the relationships between pciehp and acpiphp
as of v3.9-rc6.

The pciehp driver always have priority over the acpiphp driver.
That is, the acpiphp driver rejects binding to an ACPI PCI hotplug slot if
a) The slot's parent is a PCIe port with native hotplug capability
b) OSPM has taken over PCIe native hotplug control from BIOS. 
	!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL)
The above check has no dependency on the loading order of pciehp and acpiphp
drivers. So converting acpiphp driver to builit-in should be ok.

On the other hand, I remember Yinghai has mentioned that some PCIe ports
with native hotplug capability doesn't work as expected with the pciehp driver
and should be managed by the acpiphp driver. Currently we could achieve that
by using boot param "pcie_ports=compat", but this will disable PCIe port
drivers altogether. And I also remember that Rafael has mentioned that
some BIOSes exhibit strange dependency among PCIe OSC controls, so it's
not feasible to only disable PCIe native hotplug. 

For "pciehp_force", it does only affect the way pciehp to detect a hotplug
slot, it doesn't affect acpiphp at all.

To sum up, converting acpiphp as built-in should not affect the relationship
between pciehp and acpiphp driver. So how about splitting this patch into
two and adding more comments for the Kconfig change?

Regards!
Gerry

> 
> I would definitely like to see the changelog mention this issue and
> any changes a user might have to make, e.g., to replace a modules.conf
> edit.  It might be that this involves the "pciehp_force" option, and
> if it does, we should probably add a mention of this in
> Documentation/kernel-parameters.txt.
> 
> Can the Kconfig change be split into its own small patch without all
> the other stuff, or are they inseparable?
> 
> Bjorn
> 
>> +       depends on HOTPLUG_PCI=y && ((!ACPI_DOCK && ACPI) || (ACPI_DOCK))
>>         help
>>           Say Y here if you have a system that supports PCI Hotplug using
>>           ACPI.
>>
>> -         To compile this driver as a module, choose M here: the
>> -         module will be called acpiphp.
>> -
>>           When in doubt, say N.
>>
>>  config HOTPLUG_PCI_ACPI_IBM
>> diff --git a/drivers/pci/hotplug/acpiphp.h b/drivers/pci/hotplug/acpiphp.h
>> index 1b311f9..69b4aac 100644
>> --- a/drivers/pci/hotplug/acpiphp.h
>> +++ b/drivers/pci/hotplug/acpiphp.h
>> @@ -119,7 +119,6 @@ struct acpiphp_slot {
>>   */
>>  struct acpiphp_func {
>>         struct acpiphp_slot *slot;      /* parent */
>> -       struct acpiphp_bridge *bridge;  /* Ejectable PCI-to-PCI bridge */
>>
>>         struct list_head sibling;
>>         struct notifier_block nb;
>> @@ -176,8 +175,6 @@ extern int acpiphp_register_hotplug_slot(struct acpiphp_slot *slot);
>>  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);
>>  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 c2fd309..80d777c 100644
>> --- a/drivers/pci/hotplug/acpiphp_core.c
>> +++ b/drivers/pci/hotplug/acpiphp_core.c
>> @@ -37,6 +37,7 @@
>>
>>  #include <linux/kernel.h>
>>  #include <linux/pci.h>
>> +#include <linux/pci-acpi.h>
>>  #include <linux/pci_hotplug.h>
>>  #include <linux/slab.h>
>>  #include <linux/smp.h>
>> @@ -351,27 +352,7 @@ void acpiphp_unregister_hotplug_slot(struct acpiphp_slot *acpiphp_slot)
>>  }
>>
>>
>> -static int __init acpiphp_init(void)
>> +void __init acpiphp_init(void)
>>  {
>>         info(DRIVER_DESC " version: " DRIVER_VERSION "\n");
>> -
>> -       if (acpi_pci_disabled)
>> -               return 0;
>> -
>> -       /* read all the ACPI info from the system */
>> -       /* initialize internal data structure etc. */
>> -       return acpiphp_glue_init();
>> -}
>> -
>> -
>> -static void __exit acpiphp_exit(void)
>> -{
>> -       if (acpi_pci_disabled)
>> -               return;
>> -
>> -       /* deallocate internal data structures etc. */
>> -       acpiphp_glue_exit();
>>  }
>> -
>> -module_init(acpiphp_init);
>> -module_exit(acpiphp_exit);
>> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
>> index 89e0c36..3db84b6 100644
>> --- a/drivers/pci/hotplug/acpiphp_glue.c
>> +++ b/drivers/pci/hotplug/acpiphp_glue.c
>> @@ -157,6 +157,7 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
>>         int device, function, retval;
>>         struct pci_bus *pbus = bridge->pci_bus;
>>         struct pci_dev *pdev;
>> +       u32 val;
>>
>>         if (!acpi_pci_check_ejectable(pbus, handle) && !is_dock_device(handle))
>>                 return AE_OK;
>> @@ -249,11 +250,9 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
>>         newfunc->slot = slot;
>>         list_add_tail(&newfunc->sibling, &slot->funcs);
>>
>> -       pdev = pci_get_slot(pbus, PCI_DEVFN(device, function));
>> -       if (pdev) {
>> +       if (pci_bus_read_dev_vendor_id(pbus, PCI_DEVFN(device, function),
>> +                                      &val, 60*1000))
>>                 slot->flags |= (SLOT_ENABLED | SLOT_POWEREDON);
>> -               pci_dev_put(pdev);
>> -       }
>>
>>         if (is_dock_device(handle)) {
>>                 /* we don't want to call this device's _EJ0
>> @@ -366,148 +365,6 @@ static struct acpiphp_func *acpiphp_bridge_handle_to_function(acpi_handle handle
>>  }
>>
>>
>> -static inline void config_p2p_bridge_flags(struct acpiphp_bridge *bridge)
>> -{
>> -       acpi_handle dummy_handle;
>> -       struct acpiphp_func *func;
>> -
>> -       if (ACPI_SUCCESS(acpi_get_handle(bridge->handle,
>> -                                       "_EJ0", &dummy_handle))) {
>> -               bridge->flags |= BRIDGE_HAS_EJ0;
>> -
>> -               dbg("found ejectable p2p bridge\n");
>> -
>> -               /* make link between PCI bridge and PCI function */
>> -               func = acpiphp_bridge_handle_to_function(bridge->handle);
>> -               if (!func)
>> -                       return;
>> -               bridge->func = func;
>> -               func->bridge = bridge;
>> -       }
>> -}
>> -
>> -
>> -/* allocate and initialize host bridge data structure */
>> -static void add_host_bridge(struct acpi_pci_root *root)
>> -{
>> -       struct acpiphp_bridge *bridge;
>> -       acpi_handle handle = root->device->handle;
>> -
>> -       bridge = kzalloc(sizeof(struct acpiphp_bridge), GFP_KERNEL);
>> -       if (bridge == NULL)
>> -               return;
>> -
>> -       bridge->handle = handle;
>> -
>> -       bridge->pci_bus = root->bus;
>> -
>> -       init_bridge_misc(bridge);
>> -}
>> -
>> -
>> -/* allocate and initialize PCI-to-PCI bridge data structure */
>> -static void add_p2p_bridge(acpi_handle *handle)
>> -{
>> -       struct acpiphp_bridge *bridge;
>> -
>> -       bridge = kzalloc(sizeof(struct acpiphp_bridge), GFP_KERNEL);
>> -       if (bridge == NULL) {
>> -               err("out of memory\n");
>> -               return;
>> -       }
>> -
>> -       bridge->handle = handle;
>> -       config_p2p_bridge_flags(bridge);
>> -
>> -       bridge->pci_dev = acpi_get_pci_dev(handle);
>> -       bridge->pci_bus = bridge->pci_dev->subordinate;
>> -       if (!bridge->pci_bus) {
>> -               err("This is not a PCI-to-PCI bridge!\n");
>> -               goto err;
>> -       }
>> -
>> -       /*
>> -        * Grab a ref to the subordinate PCI bus in case the bus is
>> -        * removed via PCI core logical hotplug. The ref pins the bus
>> -        * (which we access during module unload).
>> -        */
>> -       get_device(&bridge->pci_bus->dev);
>> -
>> -       init_bridge_misc(bridge);
>> -       return;
>> - err:
>> -       pci_dev_put(bridge->pci_dev);
>> -       kfree(bridge);
>> -       return;
>> -}
>> -
>> -
>> -/* callback routine to find P2P bridges */
>> -static acpi_status
>> -find_p2p_bridge(acpi_handle handle, u32 lvl, void *context, void **rv)
>> -{
>> -       acpi_status status;
>> -       struct pci_dev *dev;
>> -
>> -       dev = acpi_get_pci_dev(handle);
>> -       if (!dev || !dev->subordinate)
>> -               goto out;
>> -
>> -       /* check if this bridge has ejectable slots */
>> -       if ((detect_ejectable_slots(handle) > 0)) {
>> -               dbg("found PCI-to-PCI bridge at PCI %s\n", pci_name(dev));
>> -               add_p2p_bridge(handle);
>> -       }
>> -
>> -       /* search P2P bridges under this p2p bridge */
>> -       status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
>> -                                    find_p2p_bridge, NULL, NULL, NULL);
>> -       if (ACPI_FAILURE(status))
>> -               warn("find_p2p_bridge failed (error code = 0x%x)\n", status);
>> -
>> - out:
>> -       pci_dev_put(dev);
>> -       return AE_OK;
>> -}
>> -
>> -
>> -/* find hot-pluggable slots, and then find P2P bridge */
>> -static int add_bridge(struct acpi_pci_root *root)
>> -{
>> -       acpi_status status;
>> -       unsigned long long tmp;
>> -       acpi_handle dummy_handle;
>> -       acpi_handle handle = root->device->handle;
>> -
>> -       /* if the bridge doesn't have _STA, we assume it is always there */
>> -       status = acpi_get_handle(handle, "_STA", &dummy_handle);
>> -       if (ACPI_SUCCESS(status)) {
>> -               status = acpi_evaluate_integer(handle, "_STA", NULL, &tmp);
>> -               if (ACPI_FAILURE(status)) {
>> -                       dbg("%s: _STA evaluation failure\n", __func__);
>> -                       return 0;
>> -               }
>> -               if ((tmp & ACPI_STA_DEVICE_FUNCTIONING) == 0)
>> -                       /* don't register this object */
>> -                       return 0;
>> -       }
>> -
>> -       /* check if this bridge has ejectable slots */
>> -       if (detect_ejectable_slots(handle) > 0) {
>> -               dbg("found PCI host-bus bridge with hot-pluggable slots\n");
>> -               add_host_bridge(root);
>> -       }
>> -
>> -       /* search P2P bridges under this host bridge */
>> -       status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
>> -                                    find_p2p_bridge, NULL, NULL, NULL);
>> -
>> -       if (ACPI_FAILURE(status))
>> -               warn("find_p2p_bridge failed (error code = 0x%x)\n", status);
>> -
>> -       return 0;
>> -}
>> -
>>  static struct acpiphp_bridge *acpiphp_handle_to_bridge(acpi_handle handle)
>>  {
>>         struct acpiphp_bridge *bridge;
>> @@ -567,56 +424,12 @@ static void cleanup_bridge(struct acpiphp_bridge *bridge)
>>                 slot = next;
>>         }
>>
>> -       /*
>> -        * Only P2P bridges have a pci_dev
>> -        */
>> -       if (bridge->pci_dev)
>> -               put_device(&bridge->pci_bus->dev);
>> -
>> +       put_device(&bridge->pci_bus->dev);
>>         pci_dev_put(bridge->pci_dev);
>>         list_del(&bridge->list);
>>         kfree(bridge);
>>  }
>>
>> -static acpi_status
>> -cleanup_p2p_bridge(acpi_handle handle, u32 lvl, void *context, void **rv)
>> -{
>> -       struct acpiphp_bridge *bridge;
>> -
>> -       /* cleanup p2p bridges under this P2P bridge
>> -          in a depth-first manner */
>> -       acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
>> -                               cleanup_p2p_bridge, NULL, NULL, NULL);
>> -
>> -       bridge = acpiphp_handle_to_bridge(handle);
>> -       if (bridge)
>> -               cleanup_bridge(bridge);
>> -
>> -       return AE_OK;
>> -}
>> -
>> -static void remove_bridge(struct acpi_pci_root *root)
>> -{
>> -       struct acpiphp_bridge *bridge;
>> -       acpi_handle handle = root->device->handle;
>> -
>> -       /* cleanup p2p bridges under this host bridge
>> -          in a depth-first manner */
>> -       acpi_walk_namespace(ACPI_TYPE_DEVICE, handle,
>> -                               (u32)1, cleanup_p2p_bridge, NULL, NULL, NULL);
>> -
>> -       /*
>> -        * On root bridges with hotplug slots directly underneath (ie,
>> -        * no p2p bridge between), we call cleanup_bridge().
>> -        *
>> -        * The else clause cleans up root bridges that either had no
>> -        * hotplug slots at all, or had a p2p bridge underneath.
>> -        */
>> -       bridge = acpiphp_handle_to_bridge(handle);
>> -       if (bridge)
>> -               cleanup_bridge(bridge);
>> -}
>> -
>>  static int power_on_slot(struct acpiphp_slot *slot)
>>  {
>>         acpi_status status;
>> @@ -802,6 +615,7 @@ static void check_hotplug_bridge(struct acpiphp_slot *slot, struct pci_dev *dev)
>>                 }
>>         }
>>  }
>> +
>>  /**
>>   * enable_device - enable, configure a slot
>>   * @slot: slot to be enabled
>> @@ -816,7 +630,6 @@ static int __ref enable_device(struct acpiphp_slot *slot)
>>         struct acpiphp_func *func;
>>         int retval = 0;
>>         int num, max, pass;
>> -       acpi_status status;
>>
>>         if (slot->flags & SLOT_ENABLED)
>>                 goto err_exit;
>> @@ -871,18 +684,6 @@ static int __ref enable_device(struct acpiphp_slot *slot)
>>                         slot->flags &= (~SLOT_ENABLED);
>>                         continue;
>>                 }
>> -
>> -               if (dev->hdr_type != PCI_HEADER_TYPE_BRIDGE &&
>> -                   dev->hdr_type != PCI_HEADER_TYPE_CARDBUS) {
>> -                       pci_dev_put(dev);
>> -                       continue;
>> -               }
>> -
>> -               status = find_p2p_bridge(func->handle, (u32)1, bus, NULL);
>> -               if (ACPI_FAILURE(status))
>> -                       warn("find_p2p_bridge failed (error code = 0x%x)\n",
>> -                               status);
>> -               pci_dev_put(dev);
>>         }
>>
>>
>> @@ -916,16 +717,6 @@ static int disable_device(struct acpiphp_slot *slot)
>>  {
>>         struct acpiphp_func *func;
>>         struct pci_dev *pdev;
>> -       struct pci_bus *bus = slot->bridge->pci_bus;
>> -
>> -       list_for_each_entry(func, &slot->funcs, sibling) {
>> -               if (func->bridge) {
>> -                       /* cleanup p2p bridges under this P2P bridge */
>> -                       cleanup_p2p_bridge(func->bridge->handle,
>> -                                               (u32)1, NULL, NULL);
>> -                       func->bridge = NULL;
>> -               }
>> -       }
>>
>>         /*
>>          * enable_device() enumerates all functions in this device via
>> @@ -944,7 +735,6 @@ static int disable_device(struct acpiphp_slot *slot)
>>
>>         slot->flags &= (~SLOT_ENABLED);
>>
>> -err_exit:
>>         return 0;
>>  }
>>
>> @@ -1285,30 +1075,56 @@ static void handle_hotplug_event_func(acpi_handle handle, u32 type,
>>         alloc_acpi_hp_work(handle, type, context, _handle_hotplug_event_func);
>>  }
>>
>> -static struct acpi_pci_driver acpi_pci_hp_driver = {
>> -       .add =          add_bridge,
>> -       .remove =       remove_bridge,
>> -};
>> -
>> -/**
>> - * acpiphp_glue_init - initializes all PCI hotplug - ACPI glue data structures
>> +/*
>> + * Create hotplug slots for the PCI bus.
>> + * It should always return 0 to avoid skipping following notifiers.
>>   */
>> -int __init acpiphp_glue_init(void)
>> +void acpiphp_enumerate_slots(struct pci_bus *bus, acpi_handle handle)
>>  {
>> -       acpi_pci_register_driver(&acpi_pci_hp_driver);
>> +       acpi_handle dummy_handle;
>> +       struct acpiphp_bridge *bridge;
>>
>> -       return 0;
>> -}
>> +       if (detect_ejectable_slots(handle) <= 0)
>> +               return;
>>
>> +       bridge = kzalloc(sizeof(struct acpiphp_bridge), GFP_KERNEL);
>> +       if (bridge == NULL) {
>> +               err("out of memory\n");
>> +               return;
>> +       }
>>
>> -/**
>> - * acpiphp_glue_exit - terminates all PCI hotplug - ACPI glue data structures
>> - *
>> - * This function frees all data allocated in acpiphp_glue_init().
>> - */
>> -void  acpiphp_glue_exit(void)
>> +       bridge->handle = handle;
>> +       bridge->pci_dev = pci_dev_get(bus->self);
>> +       bridge->pci_bus = bus;
>> +
>> +       /*
>> +        * Grab a ref to the subordinate PCI bus in case the bus is
>> +        * removed via PCI core logical hotplug. The ref pins the bus
>> +        * (which we access during module unload).
>> +        */
>> +       get_device(&bus->dev);
>> +
>> +       if (!pci_is_root_bus(bridge->pci_bus) &&
>> +           ACPI_SUCCESS(acpi_get_handle(bridge->handle,
>> +                                       "_EJ0", &dummy_handle))) {
>> +               dbg("found ejectable p2p bridge\n");
>> +               bridge->flags |= BRIDGE_HAS_EJ0;
>> +               bridge->func = acpiphp_bridge_handle_to_function(handle);
>> +       }
>> +
>> +       init_bridge_misc(bridge);
>> +}
>> +
>> +/* Destroy hotplug slots associated with the PCI bus */
>> +void acpiphp_remove_slots(struct pci_bus *bus)
>>  {
>> -       acpi_pci_unregister_driver(&acpi_pci_hp_driver);
>> +       struct acpiphp_bridge *bridge, *tmp;
>> +
>> +       list_for_each_entry_safe(bridge, tmp, &bridge_list, list)
>> +               if (bridge->pci_bus == bus) {
>> +                       cleanup_bridge(bridge);
>> +                       break;
>> +               }
>>  }
>>
>>  /**
>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
>> index 2dbca38..6fe7bf8 100644
>> --- a/drivers/pci/pci-acpi.c
>> +++ b/drivers/pci/pci-acpi.c
>> @@ -297,6 +297,7 @@ void acpi_pci_add_bus(struct pci_bus *bus)
>>                 return;
>>
>>         acpi_pci_slot_enumerate(bus, handle);
>> +       acpiphp_enumerate_slots(bus, handle);
>>  }
>>
>>  void acpi_pci_remove_bus(struct pci_bus *bus)
>> @@ -308,6 +309,7 @@ void acpi_pci_remove_bus(struct pci_bus *bus)
>>         if (acpi_pci_disabled)
>>                 return;
>>
>> +       acpiphp_remove_slots(bus);
>>         acpi_pci_slot_remove(bus);
>>  }
>>
>> @@ -384,6 +386,7 @@ static int __init acpi_pci_init(void)
>>
>>         pci_set_platform_pm(&acpi_pci_platform_pm);
>>         acpi_pci_slot_init();
>> +       acpiphp_init();
>>
>>         return 0;
>>  }
>> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
>> index 1ef126f..f1b2380 100644
>> --- a/include/linux/pci-acpi.h
>> +++ b/include/linux/pci-acpi.h
>> @@ -56,9 +56,17 @@ static inline void acpi_pci_slot_enumerate(struct pci_bus *bus,
>>  static inline void acpi_pci_slot_remove(struct pci_bus *bus) { }
>>  #endif
>>
>> -#else  /* CONFIG_ACPI */
>> -static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
>> -static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
>> +#ifdef CONFIG_HOTPLUG_PCI_ACPI
>> +void acpiphp_init(void);
>> +void acpiphp_enumerate_slots(struct pci_bus *bus, acpi_handle handle);
>> +void acpiphp_remove_slots(struct pci_bus *bus);
>> +#else
>> +static inline void acpiphp_init(void) { }
>> +static inline void acpiphp_enumerate_slots(struct pci_bus *bus,
>> +                                          acpi_handle handle) { }
>> +static inline void acpiphp_remove_slots(struct pci_bus *bus) { }
>> +#endif
>> +
>>  #endif /* CONFIG_ACPI */
>>
>>  #ifdef CONFIG_ACPI_APEI
>> --
>> 1.7.9.5
>>

Re: [PATCH v8 10/13] PCI/acpiphp: do not use ACPI PCI subdriver mechanism

[Bjorn Helgaas]

On Wed, Apr 10, 2013 at 10:14 AM, Jiang Liu <liuj97@gmail.com> wrote:
> On 04/10/2013 07:38 AM, Bjorn Helgaas wrote:
>> On Tue, Feb 26, 2013 at 8:25 AM, Jiang Liu <liuj97@gmail.com> wrote:
>>> Previously the acpiphp driver registers itself as an ACPI PCI subdriver,
>>> so it's callbacks will be invoked when creating/destroying PCI root
>>> buses to manage ACPI based PCI hotplug slots. But it doesn't handle
>>> P2P bridge hotplug events, so it will cause strange behavours if there
>>> are hotplug slots associated with a hot-removed P2P bridge.
>>>
>>> So this patch tries to fix this issue by:
>>> 1) Make acpiphp built-in only, not a module.
>>> 2) Directly hook into PCI core to update hotplug slot devices when
>>>    creating/destroying PCI buses through:
>>>         pci_{add|remove}_bus() -> acpi_pci_{add|remove}_bus()
>>> 3) Get rid of ACPI PCI subdriver related code, it's not used any more.
>>>
>>> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
>>> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
>>> Cc: Yinghai Lu <yinghai@kernel.org>
>>> Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
>>> Cc: Toshi Kani <toshi.kani@hp.com>
>>> Cc: linux-pci@vger.kernel.org
>>> Cc: linux-kernel@vger.kernel.org
>>> ---
>>>  drivers/pci/hotplug/Kconfig        |    7 +-
>>>  drivers/pci/hotplug/acpiphp.h      |    3 -
>>>  drivers/pci/hotplug/acpiphp_core.c |   23 +--
>>>  drivers/pci/hotplug/acpiphp_glue.c |  282 +++++++-----------------------------
>>>  drivers/pci/pci-acpi.c             |    3 +
>>>  include/linux/pci-acpi.h           |   14 +-
>>>  6 files changed, 67 insertions(+), 265 deletions(-)
>>>
>>> diff --git a/drivers/pci/hotplug/Kconfig b/drivers/pci/hotplug/Kconfig
>>> index 13e9e63..9fcb87f 100644
>>> --- a/drivers/pci/hotplug/Kconfig
>>> +++ b/drivers/pci/hotplug/Kconfig
>>> @@ -52,15 +52,12 @@ config HOTPLUG_PCI_IBM
>>>           When in doubt, say N.
>>>
>>>  config HOTPLUG_PCI_ACPI
>>> -       tristate "ACPI PCI Hotplug driver"
>>> -       depends on (!ACPI_DOCK && ACPI) || (ACPI_DOCK)
>>> +       bool "ACPI PCI Hotplug driver"
>>
> Hi Bjorn,
>         Thanks for review.
>
>> My goal is that a user should never have to specify a kernel boot
>> parameter or edit a modules.conf file, but the user did previously
>> have some way to influence whether we use pciehp or acpiphp.  I know
>> we still have some issues, particularly with acpiphp, so I'm a little
>> concerned that by removing the CONFIG_HOTPLUG_PCI_ACPI=m, we might be
>> removing a way to work around those issues.
>>
>> A distro that previously used CONFIG_HOTPLUG_PCI_ACPI=m will now have
>> to use =y, so modules.conf is no longer applicable.  Can you convince
>> me that the user still has a way to work around issues?  I spent quite
>> a while trying to understand the pciehp/acpiphp dependencies, but it's
>> pretty tangled web.
> I will try my best to explain the relationships between pciehp and acpiphp
> as of v3.9-rc6.
>
> The pciehp driver always have priority over the acpiphp driver.
> That is, the acpiphp driver rejects binding to an ACPI PCI hotplug slot if
> a) The slot's parent is a PCIe port with native hotplug capability
> b) OSPM has taken over PCIe native hotplug control from BIOS.
>         !(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL)
> The above check has no dependency on the loading order of pciehp and acpiphp
> drivers. So converting acpiphp driver to builit-in should be ok.
>
> On the other hand, I remember Yinghai has mentioned that some PCIe ports
> with native hotplug capability doesn't work as expected with the pciehp driver
> and should be managed by the acpiphp driver. Currently we could achieve that
> by using boot param "pcie_ports=compat", but this will disable PCIe port
> drivers altogether. And I also remember that Rafael has mentioned that
> some BIOSes exhibit strange dependency among PCIe OSC controls, so it's
> not feasible to only disable PCIe native hotplug.
>
> For "pciehp_force", it does only affect the way pciehp to detect a hotplug
> slot, it doesn't affect acpiphp at all.
>
> To sum up, converting acpiphp as built-in should not affect the relationship
> between pciehp and acpiphp driver.

My concern is that a user used to be able to remove acpiphp from
modules.conf.  Now removing acpiphp will require a kernel rebuild.
But maybe that won't turn out to be a problem.

> So how about splitting this patch into
> two and adding more comments for the Kconfig change?

Yes, please at least split this into two.  While you're at it, please
also split the first patch into "remove unnecessary is_added guard"
and "cleanup."

Bjorn

Re: [PATCH v8 10/13] PCI/acpiphp: do not use ACPI PCI subdriver mechanism

[Yijing Wang]

>> Hi Bjorn,
>>         Thanks for review.
>>
>>> My goal is that a user should never have to specify a kernel boot
>>> parameter or edit a modules.conf file, but the user did previously
>>> have some way to influence whether we use pciehp or acpiphp.  I know
>>> we still have some issues, particularly with acpiphp, so I'm a little
>>> concerned that by removing the CONFIG_HOTPLUG_PCI_ACPI=m, we might be
>>> removing a way to work around those issues.
>>>
>>> A distro that previously used CONFIG_HOTPLUG_PCI_ACPI=m will now have
>>> to use =y, so modules.conf is no longer applicable.  Can you convince
>>> me that the user still has a way to work around issues?  I spent quite
>>> a while trying to understand the pciehp/acpiphp dependencies, but it's
>>> pretty tangled web.
>> I will try my best to explain the relationships between pciehp and acpiphp
>> as of v3.9-rc6.
>>
>> The pciehp driver always have priority over the acpiphp driver.
>> That is, the acpiphp driver rejects binding to an ACPI PCI hotplug slot if
>> a) The slot's parent is a PCIe port with native hotplug capability
>> b) OSPM has taken over PCIe native hotplug control from BIOS.
>>         !(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL)
>> The above check has no dependency on the loading order of pciehp and acpiphp
>> drivers. So converting acpiphp driver to builit-in should be ok.
>>
>> On the other hand, I remember Yinghai has mentioned that some PCIe ports
>> with native hotplug capability doesn't work as expected with the pciehp driver
>> and should be managed by the acpiphp driver. Currently we could achieve that
>> by using boot param "pcie_ports=compat", but this will disable PCIe port
>> drivers altogether. And I also remember that Rafael has mentioned that
>> some BIOSes exhibit strange dependency among PCIe OSC controls, so it's
>> not feasible to only disable PCIe native hotplug.
>>
>> For "pciehp_force", it does only affect the way pciehp to detect a hotplug
>> slot, it doesn't affect acpiphp at all.
>>
>> To sum up, converting acpiphp as built-in should not affect the relationship
>> between pciehp and acpiphp driver.
> 
> My concern is that a user used to be able to remove acpiphp from
> modules.conf.  Now removing acpiphp will require a kernel rebuild.
> But maybe that won't turn out to be a problem.

Hi Bjorn,
   If user don't want to occupy the slot by acpiphp. Conservative approach, what about add a kernel parameter
to control acpiphp to enumerate slot ?

Thanks!
Yijing
> 
>> So how about splitting this patch into
>> two and adding more comments for the Kconfig change?
> 
> Yes, please at least split this into two.  While you're at it, please
> also split the first patch into "remove unnecessary is_added guard"
> and "cleanup."
> 
> Bjorn
> 
> .
> 


-- 
Thanks!
Yijing

Re: [PATCH v8 10/13] PCI/acpiphp: do not use ACPI PCI subdriver mechanism

[Bjorn Helgaas]

On Wed, Apr 10, 2013 at 7:50 PM, Yijing Wang <wangyijing@huawei.com> wrote:
>>> Hi Bjorn,
>>>         Thanks for review.
>>>
>>>> My goal is that a user should never have to specify a kernel boot
>>>> parameter or edit a modules.conf file, but the user did previously
>>>> have some way to influence whether we use pciehp or acpiphp.  I know
>>>> we still have some issues, particularly with acpiphp, so I'm a little
>>>> concerned that by removing the CONFIG_HOTPLUG_PCI_ACPI=m, we might be
>>>> removing a way to work around those issues.
>>>>
>>>> A distro that previously used CONFIG_HOTPLUG_PCI_ACPI=m will now have
>>>> to use =y, so modules.conf is no longer applicable.  Can you convince
>>>> me that the user still has a way to work around issues?  I spent quite
>>>> a while trying to understand the pciehp/acpiphp dependencies, but it's
>>>> pretty tangled web.
>>> I will try my best to explain the relationships between pciehp and acpiphp
>>> as of v3.9-rc6.
>>>
>>> The pciehp driver always have priority over the acpiphp driver.
>>> That is, the acpiphp driver rejects binding to an ACPI PCI hotplug slot if
>>> a) The slot's parent is a PCIe port with native hotplug capability
>>> b) OSPM has taken over PCIe native hotplug control from BIOS.
>>>         !(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL)
>>> The above check has no dependency on the loading order of pciehp and acpiphp
>>> drivers. So converting acpiphp driver to builit-in should be ok.
>>>
>>> On the other hand, I remember Yinghai has mentioned that some PCIe ports
>>> with native hotplug capability doesn't work as expected with the pciehp driver
>>> and should be managed by the acpiphp driver. Currently we could achieve that
>>> by using boot param "pcie_ports=compat", but this will disable PCIe port
>>> drivers altogether. And I also remember that Rafael has mentioned that
>>> some BIOSes exhibit strange dependency among PCIe OSC controls, so it's
>>> not feasible to only disable PCIe native hotplug.
>>>
>>> For "pciehp_force", it does only affect the way pciehp to detect a hotplug
>>> slot, it doesn't affect acpiphp at all.
>>>
>>> To sum up, converting acpiphp as built-in should not affect the relationship
>>> between pciehp and acpiphp driver.
>>
>> My concern is that a user used to be able to remove acpiphp from
>> modules.conf.  Now removing acpiphp will require a kernel rebuild.
>> But maybe that won't turn out to be a problem.
>
> Hi Bjorn,
>    If user don't want to occupy the slot by acpiphp. Conservative approach, what about add a kernel parameter
> to control acpiphp to enumerate slot ?

Yes, that's what I'm thinking.  A "Please report a bug if you have to
use this parameter" chicken switch.  Hopefully nobody will need to use
it, but if somebody *does* need it, it's a lot better than having to
tell him to rebuild the kernel.

Bjorn

Re: [PATCH v8 10/13] PCI/acpiphp: do not use ACPI PCI subdriver mechanism

[Yijing Wang]

On 2013/4/12 1:29, Bjorn Helgaas wrote:
> On Wed, Apr 10, 2013 at 7:50 PM, Yijing Wang <wangyijing@huawei.com> wrote:
>>>> Hi Bjorn,
>>>>         Thanks for review.
>>>>
>>>>> My goal is that a user should never have to specify a kernel boot
>>>>> parameter or edit a modules.conf file, but the user did previously
>>>>> have some way to influence whether we use pciehp or acpiphp.  I know
>>>>> we still have some issues, particularly with acpiphp, so I'm a little
>>>>> concerned that by removing the CONFIG_HOTPLUG_PCI_ACPI=m, we might be
>>>>> removing a way to work around those issues.
>>>>>
>>>>> A distro that previously used CONFIG_HOTPLUG_PCI_ACPI=m will now have
>>>>> to use =y, so modules.conf is no longer applicable.  Can you convince
>>>>> me that the user still has a way to work around issues?  I spent quite
>>>>> a while trying to understand the pciehp/acpiphp dependencies, but it's
>>>>> pretty tangled web.
>>>> I will try my best to explain the relationships between pciehp and acpiphp
>>>> as of v3.9-rc6.
>>>>
>>>> The pciehp driver always have priority over the acpiphp driver.
>>>> That is, the acpiphp driver rejects binding to an ACPI PCI hotplug slot if
>>>> a) The slot's parent is a PCIe port with native hotplug capability
>>>> b) OSPM has taken over PCIe native hotplug control from BIOS.
>>>>         !(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL)
>>>> The above check has no dependency on the loading order of pciehp and acpiphp
>>>> drivers. So converting acpiphp driver to builit-in should be ok.
>>>>
>>>> On the other hand, I remember Yinghai has mentioned that some PCIe ports
>>>> with native hotplug capability doesn't work as expected with the pciehp driver
>>>> and should be managed by the acpiphp driver. Currently we could achieve that
>>>> by using boot param "pcie_ports=compat", but this will disable PCIe port
>>>> drivers altogether. And I also remember that Rafael has mentioned that
>>>> some BIOSes exhibit strange dependency among PCIe OSC controls, so it's
>>>> not feasible to only disable PCIe native hotplug.
>>>>
>>>> For "pciehp_force", it does only affect the way pciehp to detect a hotplug
>>>> slot, it doesn't affect acpiphp at all.
>>>>
>>>> To sum up, converting acpiphp as built-in should not affect the relationship
>>>> between pciehp and acpiphp driver.
>>>
>>> My concern is that a user used to be able to remove acpiphp from
>>> modules.conf.  Now removing acpiphp will require a kernel rebuild.
>>> But maybe that won't turn out to be a problem.
>>
>> Hi Bjorn,
>>    If user don't want to occupy the slot by acpiphp. Conservative approach, what about add a kernel parameter
>> to control acpiphp to enumerate slot ?
> 
> Yes, that's what I'm thinking.  A "Please report a bug if you have to
> use this parameter" chicken switch.  Hopefully nobody will need to use
> it, but if somebody *does* need it, it's a lot better than having to
> tell him to rebuild the kernel.

Yes, I agree. We will add this patch to the patchset and resend soon.

Thanks!
Yijing.

> 
> Bjorn
> 
> .
> 


-- 
Thanks!
Yijing