[RFC PATCH v5 0/8] introduce PCI bus notifier chain to get rid of the ACPI PCI subdriver interfaces

[RFC PATCH v5 0/8] introduce PCI bus notifier chain to get rid of the ACPI PCI subdriver interfaces

[Jiang Liu]

This is an RFC patchset to address review comments in thread at:
https://patchwork.kernel.org/patch/1946851/. The patch just pasts
compilation. If no objection to the new implementation, I will
go on to modify acpiphp driver and conduct tests.

The main changes from V4 to V5 includes:
1) introduce a dedicated notifier chain for PCI buses
2) change pci_slot as built-in driver
3) unify the way to create/destroy PCI slots
4) introduce a kernel option to disable PCIe native hotplug

TODO:
1) change acpiphp as built-in and unify the way to create/destroy ACPI
   based hotplug slots.
2) change other ACPI PCI subdriver in Yinghai's root bridge hotplug series
   to use the PCI bus notifier chain.
3) Remove the ACPI PCI subdriver interface eventaully.

Jiang Liu (8):
  PCI: make PCI device create/destroy logic symmetric
  PCI: split registration of PCI bus devices into two stages
  PCI: add a blocking notifier chain for PCI bus addition/removal
  ACPI, PCI: avoid building pci_slot as module
  PCI, ACPI: hook PCI bus notifications to create/destroy PCI slots
  pci_slot: replace printk(KERN_xxx) with pr_xxx()
  PCI/PCIe: add "pci=nopciehp" to disable PCIe native hotplug
  PCI/PCIe: only claim PME from firmware when CONFIG_PCIE_PME is
    enabled

 Documentation/kernel-parameters.txt |    2 +
 drivers/acpi/Kconfig                |    5 +-
 drivers/acpi/internal.h             |    5 +
 drivers/acpi/pci_root.c             |    8 +-
 drivers/acpi/pci_slot.c             |  217 ++++++++++-------------------------
 drivers/acpi/scan.c                 |    1 +
 drivers/pci/bus.c                   |   26 ++++-
 drivers/pci/pci.c                   |    2 +
 drivers/pci/pci.h                   |    1 +
 drivers/pci/pcie/portdrv_core.c     |    7 +-
 drivers/pci/pcie/portdrv_pci.c      |    3 +
 drivers/pci/probe.c                 |    7 +-
 drivers/pci/remove.c                |   15 +--
 include/linux/pci.h                 |   21 ++++
 14 files changed, 142 insertions(+), 178 deletions(-)

-- 
1.7.9.5

[RFC PATCH v5 1/8] PCI: make PCI device create/destroy logic symmetric

[Jiang Liu]

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

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

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

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

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

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 4f7f963..ec5d28f 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1294,7 +1294,6 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
 {
 	device_initialize(&dev->dev);
 	dev->dev.release = pci_release_dev;
-	pci_dev_get(dev);
 
 	dev->dev.dma_mask = &dev->dma_mask;
 	dev->dev.dma_parms = &dev->dma_parms;
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index 513972f..10693f5 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -22,7 +22,7 @@ static void pci_stop_dev(struct pci_dev *dev)
 	if (dev->is_added) {
 		pci_proc_detach_device(dev);
 		pci_remove_sysfs_dev_files(dev);
-		device_unregister(&dev->dev);
+		device_del(&dev->dev);
 		dev->is_added = 0;
 	}
 
@@ -37,7 +37,7 @@ static void pci_destroy_dev(struct pci_dev *dev)
 	up_write(&pci_bus_sem);
 
 	pci_free_resources(dev);
-	pci_dev_put(dev);
+	put_device(&dev->dev);
 }
 
 void pci_remove_bus(struct pci_bus *bus)
-- 
1.7.9.5

[RFC PATCH v5 2/8] PCI: split registration of PCI bus devices into two stages

[Jiang Liu]

When handling BUS_NOTIFY_ADD_DEVICE event for a PCI bridge device,
the notification handler can't hold reference count to the new PCI bus
because the device object for the new bus (pci_dev->subordinate->dev)
hasn't been initialized yet.

Split the registration of PCI bus device into two stages as below,
so that the event handler could hold reference count to the new PCI bus
when handling BUS_NOTIFY_ADD_DEVICE event.

1) device_initialize(&pci_dev->dev)
2) device_initialize(&pci_dev->subordinate->dev)
3) notify BUS_NOTIFY_ADD_DEVICE event for pci_dev
4) device_add(&pci_dev->dev)
5) device_add(&pci_dev->subordinate->dev)

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

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index a543746..4c843cd 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -193,7 +193,7 @@ int pci_bus_add_child(struct pci_bus *bus)
 	if (bus->bridge)
 		bus->dev.parent = bus->bridge;
 
-	retval = device_register(&bus->dev);
+	retval = device_add(&bus->dev);
 	if (retval)
 		return retval;
 
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index ec5d28f..adb77c6 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -639,6 +639,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
 	 */
 	child->dev.class = &pcibus_class;
 	dev_set_name(&child->dev, "%04x:%02x", pci_domain_nr(child), busnr);
+	device_initialize(&child->dev);
 
 	/*
 	 * Set up the primary, secondary and subordinate
@@ -1673,7 +1674,8 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 	b->dev.class = &pcibus_class;
 	b->dev.parent = b->bridge;
 	dev_set_name(&b->dev, "%04x:%02x", pci_domain_nr(b), bus);
-	error = device_register(&b->dev);
+	device_initialize(&b->dev);
+	error = device_add(&b->dev);
 	if (error)
 		goto class_dev_reg_err;
 
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index 10693f5..0d03026 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -48,11 +48,11 @@ 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);
+	if (bus->is_added) {
+		pci_remove_legacy_files(bus);
+		device_del(&bus->dev);
+	}
+	put_device(&bus->dev);
 }
 EXPORT_SYMBOL(pci_remove_bus);
 
-- 
1.7.9.5

[RFC PATCH v5 3/8] PCI: add a blocking notifier chain for PCI bus addition/removal

[Jiang Liu]

When adding/removing a PCI bus, some other components want to be
notified so they could update their states. For example, the pci_slot
driver needs to create/destroy PCI slots attaching to the PCI bus.
So introduce a dedicate blocking notifier chain for PCI bus
addition/removal events. It could be extended to support PCI device
addition/removal events in the future when needed.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
---
 drivers/pci/bus.c    |   24 ++++++++++++++++++++++++
 drivers/pci/pci.h    |    1 +
 drivers/pci/probe.c  |    2 ++
 drivers/pci/remove.c |    1 +
 include/linux/pci.h  |   12 ++++++++++++
 5 files changed, 40 insertions(+)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 4c843cd..8bbd3f2 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -13,11 +13,33 @@
 #include <linux/errno.h>
 #include <linux/ioport.h>
 #include <linux/proc_fs.h>
+#include <linux/notifier.h>
 #include <linux/init.h>
 #include <linux/slab.h>
 
 #include "pci.h"
 
+static BLOCKING_NOTIFIER_HEAD(pci_bus_notifier_chain);
+
+int pci_bus_register_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(&pci_bus_notifier_chain, nb);
+}
+
+int pci_bus_unregister_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_unregister(&pci_bus_notifier_chain, nb);
+}
+
+void pci_bus_call_notifier(struct pci_bus *bus, unsigned long code)
+{
+	int ret;
+
+	ret = blocking_notifier_call_chain(&pci_bus_notifier_chain,
+					   code, bus);
+	WARN_ON(ret != NOTIFY_DONE && ret != NOTIFY_OK);
+}
+
 void pci_add_resource_offset(struct list_head *resources, struct resource *res,
 			     resource_size_t offset)
 {
@@ -197,6 +219,8 @@ int pci_bus_add_child(struct pci_bus *bus)
 	if (retval)
 		return retval;
 
+	pci_bus_call_notifier(bus, PCI_NOTIFY_POST_BUS_ADD);
+
 	bus->is_added = 1;
 
 	/* Create legacy_io and legacy_mem files for this bus */
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 7414094..988e4b4 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -208,6 +208,7 @@ extern int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 extern int pci_resource_bar(struct pci_dev *dev, int resno,
 			    enum pci_bar_type *type);
 extern int pci_bus_add_child(struct pci_bus *bus);
+extern void pci_bus_call_notifier(struct pci_bus *bus, unsigned long code);
 extern void pci_enable_ari(struct pci_dev *dev);
 /**
  * pci_ari_enabled - query ARI forwarding status
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index adb77c6..7db6528 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1679,6 +1679,8 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 	if (error)
 		goto class_dev_reg_err;
 
+	pci_bus_call_notifier(b, PCI_NOTIFY_POST_BUS_ADD);
+
 	/* 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 0d03026..2d8fbc8 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -50,6 +50,7 @@ void pci_remove_bus(struct pci_bus *bus)
 	up_write(&pci_bus_sem);
 	if (bus->is_added) {
 		pci_remove_legacy_files(bus);
+		pci_bus_call_notifier(bus, PCI_NOTIFY_PRE_BUS_DEL);
 		device_del(&bus->dev);
 	}
 	put_device(&bus->dev);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index ee21795..12e5447 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1033,6 +1033,18 @@ const struct pci_device_id *pci_match_id(const struct pci_device_id *ids,
 int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max,
 		    int pass);
 
+/*
+ * All notifiers below get called with the target struct pci_bus *bus as
+ * an argument.
+ * Note: all PCI bus notifier must return success. Currently there's no
+ * error handling if any notifier returns error code.
+ */
+#define	PCI_NOTIFY_POST_BUS_ADD		0x00000001 /* PCI bus has been added */
+#define	PCI_NOTIFY_PRE_BUS_DEL		0x00000002 /* PCI bus will be deleted */
+
+int pci_bus_register_notifier(struct notifier_block *nb);
+int pci_bus_unregister_notifier(struct notifier_block *nb);
+
 void pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
 		  void *userdata);
 int pci_cfg_space_size_ext(struct pci_dev *dev);
-- 
1.7.9.5

[RFC PATCH v5 4/8] ACPI, PCI: avoid building pci_slot as module

[Jiang Liu]

As discussed in thread at https://patchwork.kernel.org/patch/1946851/,
there's no value in supporting CONFIG_ACPI_PCI_SLOT=m any more.
So change Kconfig and code to only support building pci_slot as
built-in driver.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
---
 drivers/acpi/Kconfig    |    5 +----
 drivers/acpi/internal.h |    5 +++++
 drivers/acpi/pci_slot.c |   13 +------------
 drivers/acpi/scan.c     |    1 +
 4 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 0300bf6..7efd0d0 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -299,7 +299,7 @@ config ACPI_DEBUG_FUNC_TRACE
 	  is about half of the penalty and is rarely useful.
 
 config ACPI_PCI_SLOT
-	tristate "PCI slot detection driver"
+	bool "PCI slot detection driver"
 	depends on SYSFS
 	default n
 	help
@@ -308,9 +308,6 @@ config ACPI_PCI_SLOT
 	  i.e., segment/bus/device/function tuples, with physical slots in
 	  the system.  If you are unsure, say N.
 
-	  To compile this driver as a module, choose M here:
-	  the module will be called pci_slot.
-
 config X86_PM_TIMER
 	bool "Power Management Timer Support" if EXPERT
 	depends on X86
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index e050254..7374cfc 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -67,6 +67,11 @@ 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);
 int acpi_ec_init(void);
 int acpi_ec_ecdt_probe(void);
diff --git a/drivers/acpi/pci_slot.c b/drivers/acpi/pci_slot.c
index d22585f..a7d7e77 100644
--- a/drivers/acpi/pci_slot.c
+++ b/drivers/acpi/pci_slot.c
@@ -330,19 +330,8 @@ static struct dmi_system_id acpi_pci_slot_dmi_table[] __initdata = {
 	{}
 };
 
-static int __init
-acpi_pci_slot_init(void)
+void __init acpi_pci_slot_init(void)
 {
 	dmi_check_system(acpi_pci_slot_dmi_table);
 	acpi_pci_register_driver(&acpi_pci_slot_driver);
-	return 0;
 }
-
-static void __exit
-acpi_pci_slot_exit(void)
-{
-	acpi_pci_unregister_driver(&acpi_pci_slot_driver);
-}
-
-module_init(acpi_pci_slot_init);
-module_exit(acpi_pci_slot_exit);
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index f8a0d0f..cb964ac 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1732,6 +1732,7 @@ int __init acpi_scan_init(void)
 
 	acpi_power_init();
 	acpi_pci_root_init();
+	acpi_pci_slot_init();
 
 	/*
 	 * Enumerate devices in the ACPI namespace.
-- 
1.7.9.5

[RFC PATCH v5 5/8] PCI, ACPI: hook PCI bus notifications to create/destroy PCI slots

[Jiang Liu]

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

By hooking PCI bus notifications, this patch unifies the way to
create/destroy PCI slots when:
1) create PCI hierarchy at boot time
2) create PCI hierarchy for PCI root bridge hot-adding
3) create PCI hierarchy for PCI device hotplug hot-adding
4) destroy PCI hierarchy for PCI root bridge hot-removal
4) destroy PCI hierarchy for PCI device hot-removal

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 drivers/acpi/pci_slot.c |  197 +++++++++++++----------------------------------
 1 file changed, 54 insertions(+), 143 deletions(-)

diff --git a/drivers/acpi/pci_slot.c b/drivers/acpi/pci_slot.c
index a7d7e77..2b38f4d 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;
@@ -62,20 +64,12 @@ ACPI_MODULE_NAME("pci_slot");
 #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 +108,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 +119,23 @@ 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 +150,9 @@ 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,137 +162,60 @@ 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)
+static void acpi_pci_slot_notify_add(struct pci_bus *bus)
 {
-	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_handle handle = NULL;
 
-/*
- * acpi_pci_slot_add
- * @handle: points to an acpi_pci_root
- */
-static int
-acpi_pci_slot_add(struct acpi_pci_root *root)
-{
-	acpi_status status;
-
-	status = walk_root_bridge(root, register_slot);
-	if (ACPI_FAILURE(status))
-		err("%s: register_slot failure - %d\n", __func__, status);
+	if (bus->bridge)
+		handle = DEVICE_ACPI_HANDLE(bus->bridge);
+	if (!handle)
+		return;
 
-	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)
+static void acpi_pci_slot_notify_del(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);
 		}
 	}
 	mutex_unlock(&slot_list_lock);
 }
 
+static int acpi_pci_slot_notify_fn(struct notifier_block *nb,
+				   unsigned long event, void *data)
+{
+	struct pci_bus *bus = data;
+
+	switch (event) {
+	case PCI_NOTIFY_POST_BUS_ADD:
+		acpi_pci_slot_notify_add(bus);
+		break;
+	case PCI_NOTIFY_PRE_BUS_DEL:
+		acpi_pci_slot_notify_del(bus);
+		break;
+	default:
+		return NOTIFY_DONE;
+	}
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block acpi_pci_slot_notifier = {
+	.notifier_call = &acpi_pci_slot_notify_fn,
+};
+
 static int do_sta_before_sun(const struct dmi_system_id *d)
 {
 	info("%s detected: will evaluate _STA before calling _SUN\n", d->ident);
@@ -333,5 +244,5 @@ 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);
+	pci_bus_register_notifier(&acpi_pci_slot_notifier);
 }
-- 
1.7.9.5

[RFC PATCH v5 6/8] pci_slot: replace printk(KERN_xxx) with pr_xxx()

[Jiang Liu]

Trivial changes to replace printk(KERN_xxx) with pr_xxx().

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
---
 drivers/acpi/pci_slot.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/pci_slot.c b/drivers/acpi/pci_slot.c
index 2b38f4d..3b821db 100644
--- a/drivers/acpi/pci_slot.c
+++ b/drivers/acpi/pci_slot.c
@@ -52,13 +52,12 @@ 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("%s: " format,	MY_NAME , ## arg); \
 	} while (0)
 
 #define SLOT_NAME_SIZE 21		/* Inspired by #define in acpiphp.h */
-- 
1.7.9.5

[RFC PATCH v5 7/8] PCI/PCIe: add "pci=nopciehp" to disable PCIe native hotplug

[Jiang Liu]

If user specifies "pci=nopciehp" on kernel boot command line, OSPM
won't claim PCIe native hotplug service from firmware and no PCIe
port devices will be created for PCIe native hotplug service.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
---
 Documentation/kernel-parameters.txt |    2 ++
 drivers/acpi/pci_root.c             |    3 ++-
 drivers/pci/pci.c                   |    2 ++
 drivers/pci/pcie/portdrv_core.c     |    5 +++--
 drivers/pci/pcie/portdrv_pci.c      |    3 +++
 include/linux/pci.h                 |    9 +++++++++
 6 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 9776f06..28dd0ad 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -2106,6 +2106,8 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 		noaer		[PCIE] If the PCIEAER kernel config parameter is
 				enabled, this kernel boot option can be used to
 				disable the use of PCIE advanced error reporting.
+		nopciehp	[PCIE] this kernel boot option can be used to
+				disable PCIe native hotplug.
 		nodomains	[PCI] Disable support for multiple PCI
 				root domains (aka PCI segments, in ACPI-speak).
 		nommconf	[X86] Disable use of MMCONFIG for PCI
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index c6200ff..c37eedb 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -551,8 +551,9 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
 	if (!pcie_ports_disabled
 	    && (flags & ACPI_PCIE_REQ_SUPPORT) == ACPI_PCIE_REQ_SUPPORT) {
 		flags = OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL
-			| OSC_PCI_EXPRESS_NATIVE_HP_CONTROL
 			| OSC_PCI_EXPRESS_PME_CONTROL;
+		if (!pcie_native_hotplug_disabled)
+			flags |= OSC_PCI_EXPRESS_NATIVE_HP_CONTROL;
 
 		if (pci_aer_available()) {
 			if (aer_acpi_firmware_first())
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 2f8f4c6..34b2c83 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3869,6 +3869,8 @@ static int __init pci_setup(char *str)
 				pci_no_msi();
 			} else if (!strcmp(str, "noaer")) {
 				pci_no_aer();
+			} else if (!strcmp(str, "nopciehp")) {
+				pcie_no_native_hotplug();
 			} else if (!strncmp(str, "realloc=", 8)) {
 				pci_realloc_get_opt(str + 8);
 			} else if (!strncmp(str, "realloc", 7)) {
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index ed129b4..e7e1679 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -263,8 +263,9 @@ static int get_port_device_capability(struct pci_dev *dev)
 
 	err = pcie_port_platform_notify(dev, &cap_mask);
 	if (!pcie_ports_auto) {
-		cap_mask = PCIE_PORT_SERVICE_PME | PCIE_PORT_SERVICE_HP
-				| PCIE_PORT_SERVICE_VC;
+		cap_mask = PCIE_PORT_SERVICE_PME | PCIE_PORT_SERVICE_VC;
+		if (!pcie_native_hotplug_disabled)
+			cap_mask |= PCIE_PORT_SERVICE_HP;
 		if (pci_aer_available())
 			cap_mask |= PCIE_PORT_SERVICE_AER;
 	} else if (err) {
diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index 0761d90..018cee0 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -40,6 +40,9 @@ bool pcie_ports_disabled;
  */
 bool pcie_ports_auto = true;
 
+/* If set, the PCIe native hotplug will not be used. */
+bool pcie_native_hotplug_disabled;
+
 static int __init pcie_port_setup(char *str)
 {
 	if (!strncmp(str, "compat", 6)) {
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 12e5447..715e17b 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1138,9 +1138,18 @@ extern int pci_msi_enabled(void);
 #ifdef CONFIG_PCIEPORTBUS
 extern bool pcie_ports_disabled;
 extern bool pcie_ports_auto;
+extern bool pcie_native_hotplug_disabled;
+
+static inline void pcie_no_native_hotplug(void)
+{
+	pcie_native_hotplug_disabled = true;
+}
 #else
 #define pcie_ports_disabled	true
 #define pcie_ports_auto		false
+#define pcie_native_hotplug_disabled	true
+
+static inline void pcie_no_native_hotplug(void) { }
 #endif
 
 #ifndef CONFIG_PCIEASPM
-- 
1.7.9.5

[RFC PATCH v5 8/8] PCI/PCIe: only claim PME from firmware when CONFIG_PCIE_PME is enabled

[Jiang Liu]

If CONFIG_PCIE_PME is not defined, system should avoid claiming PME from
firmware so firmware could still manage PME events for those devices.
Also don't create PCIe port device for PME service if CONFIG_PCIE_PME
is not defined.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
---
 drivers/acpi/pci_root.c         |    5 +++--
 drivers/pci/pcie/portdrv_core.c |    4 +++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index c37eedb..7f7e464 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -550,8 +550,9 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
 
 	if (!pcie_ports_disabled
 	    && (flags & ACPI_PCIE_REQ_SUPPORT) == ACPI_PCIE_REQ_SUPPORT) {
-		flags = OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL
-			| OSC_PCI_EXPRESS_PME_CONTROL;
+		flags = OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL;
+		if (IS_ENABLED(CONFIG_PCIE_PME))
+			flags |= OSC_PCI_EXPRESS_PME_CONTROL;
 		if (!pcie_native_hotplug_disabled)
 			flags |= OSC_PCI_EXPRESS_NATIVE_HP_CONTROL;
 
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index e7e1679..7e6546f 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -263,7 +263,9 @@ static int get_port_device_capability(struct pci_dev *dev)
 
 	err = pcie_port_platform_notify(dev, &cap_mask);
 	if (!pcie_ports_auto) {
-		cap_mask = PCIE_PORT_SERVICE_PME | PCIE_PORT_SERVICE_VC;
+		cap_mask = PCIE_PORT_SERVICE_VC;
+		if (IS_ENABLED(CONFIG_PCIE_PME))
+			cap_mask |= PCIE_PORT_SERVICE_PME;
 		if (!pcie_native_hotplug_disabled)
 			cap_mask |= PCIE_PORT_SERVICE_HP;
 		if (pci_aer_available())
-- 
1.7.9.5

Re: [RFC PATCH v5 7/8] PCI/PCIe: add "pci=nopciehp" to disable PCIe native hotplug

[Bjorn Helgaas]

On Fri, Jan 18, 2013 at 9:07 AM, Jiang Liu <liuj97@gmail.com> wrote:
> If user specifies "pci=nopciehp" on kernel boot command line, OSPM
> won't claim PCIe native hotplug service from firmware and no PCIe
> port devices will be created for PCIe native hotplug service.

Why do we need this option?

If I understand correctly, there are machines where it *looks* like we
should use pciehp, but pciehp doesn't work because we don't get the
interrupts we expect.  On those machines, we have to use acpiphp
instead.  It seems like many Dell XPS laptops have this issue with
ExpressCard slots, e.g.,
https://bugzilla.kernel.org/show_bug.cgi?id=40802 .

If you want "pci=nopciehp" as a way for users to deal with this
problem by forcing the use of acpiphp, I object.  Windows manages to
make these slots work without having users do anything special, so we
should be able to do it, too.

> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> ---
>  Documentation/kernel-parameters.txt |    2 ++
>  drivers/acpi/pci_root.c             |    3 ++-
>  drivers/pci/pci.c                   |    2 ++
>  drivers/pci/pcie/portdrv_core.c     |    5 +++--
>  drivers/pci/pcie/portdrv_pci.c      |    3 +++
>  include/linux/pci.h                 |    9 +++++++++
>  6 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 9776f06..28dd0ad 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -2106,6 +2106,8 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>                 noaer           [PCIE] If the PCIEAER kernel config parameter is
>                                 enabled, this kernel boot option can be used to
>                                 disable the use of PCIE advanced error reporting.
> +               nopciehp        [PCIE] this kernel boot option can be used to
> +                               disable PCIe native hotplug.
>                 nodomains       [PCI] Disable support for multiple PCI
>                                 root domains (aka PCI segments, in ACPI-speak).
>                 nommconf        [X86] Disable use of MMCONFIG for PCI
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index c6200ff..c37eedb 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -551,8 +551,9 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
>         if (!pcie_ports_disabled
>             && (flags & ACPI_PCIE_REQ_SUPPORT) == ACPI_PCIE_REQ_SUPPORT) {
>                 flags = OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL
> -                       | OSC_PCI_EXPRESS_NATIVE_HP_CONTROL
>                         | OSC_PCI_EXPRESS_PME_CONTROL;
> +               if (!pcie_native_hotplug_disabled)
> +                       flags |= OSC_PCI_EXPRESS_NATIVE_HP_CONTROL;
>
>                 if (pci_aer_available()) {
>                         if (aer_acpi_firmware_first())
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 2f8f4c6..34b2c83 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3869,6 +3869,8 @@ static int __init pci_setup(char *str)
>                                 pci_no_msi();
>                         } else if (!strcmp(str, "noaer")) {
>                                 pci_no_aer();
> +                       } else if (!strcmp(str, "nopciehp")) {
> +                               pcie_no_native_hotplug();
>                         } else if (!strncmp(str, "realloc=", 8)) {
>                                 pci_realloc_get_opt(str + 8);
>                         } else if (!strncmp(str, "realloc", 7)) {
> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> index ed129b4..e7e1679 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -263,8 +263,9 @@ static int get_port_device_capability(struct pci_dev *dev)
>
>         err = pcie_port_platform_notify(dev, &cap_mask);
>         if (!pcie_ports_auto) {
> -               cap_mask = PCIE_PORT_SERVICE_PME | PCIE_PORT_SERVICE_HP
> -                               | PCIE_PORT_SERVICE_VC;
> +               cap_mask = PCIE_PORT_SERVICE_PME | PCIE_PORT_SERVICE_VC;
> +               if (!pcie_native_hotplug_disabled)
> +                       cap_mask |= PCIE_PORT_SERVICE_HP;
>                 if (pci_aer_available())
>                         cap_mask |= PCIE_PORT_SERVICE_AER;
>         } else if (err) {
> diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> index 0761d90..018cee0 100644
> --- a/drivers/pci/pcie/portdrv_pci.c
> +++ b/drivers/pci/pcie/portdrv_pci.c
> @@ -40,6 +40,9 @@ bool pcie_ports_disabled;
>   */
>  bool pcie_ports_auto = true;
>
> +/* If set, the PCIe native hotplug will not be used. */
> +bool pcie_native_hotplug_disabled;
> +
>  static int __init pcie_port_setup(char *str)
>  {
>         if (!strncmp(str, "compat", 6)) {
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 12e5447..715e17b 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1138,9 +1138,18 @@ extern int pci_msi_enabled(void);
>  #ifdef CONFIG_PCIEPORTBUS
>  extern bool pcie_ports_disabled;
>  extern bool pcie_ports_auto;
> +extern bool pcie_native_hotplug_disabled;
> +
> +static inline void pcie_no_native_hotplug(void)
> +{
> +       pcie_native_hotplug_disabled = true;
> +}
>  #else
>  #define pcie_ports_disabled    true
>  #define pcie_ports_auto                false
> +#define pcie_native_hotplug_disabled   true
> +
> +static inline void pcie_no_native_hotplug(void) { }
>  #endif
>
>  #ifndef CONFIG_PCIEASPM
> --
> 1.7.9.5
>

Re: [RFC PATCH v5 7/8] PCI/PCIe: add "pci=nopciehp" to disable PCIe native hotplug

[Yinghai Lu]

On Fri, Jan 18, 2013 at 9:35 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> If you want "pci=nopciehp" as a way for users to deal with this
> problem by forcing the use of acpiphp, I object.  Windows manages to
> make these slots work without having users do anything special, so we
> should be able to do it, too.

Agreed.

We need to think that more.

I think that we should fix acpiphp, and should follow first come and
first serve for acpiphp and pciehp.

Yinghai

Re: [RFC PATCH v5 7/8] PCI/PCIe: add "pci=nopciehp" to disable PCIe native hotplug

[Rafael J. Wysocki]

On Friday, January 18, 2013 10:35:46 AM Bjorn Helgaas wrote:
> On Fri, Jan 18, 2013 at 9:07 AM, Jiang Liu <liuj97@gmail.com> wrote:
> > If user specifies "pci=nopciehp" on kernel boot command line, OSPM
> > won't claim PCIe native hotplug service from firmware and no PCIe
> > port devices will be created for PCIe native hotplug service.
> 
> Why do we need this option?
> 
> If I understand correctly, there are machines where it *looks* like we
> should use pciehp, but pciehp doesn't work because we don't get the
> interrupts we expect.  On those machines, we have to use acpiphp
> instead.  It seems like many Dell XPS laptops have this issue with
> ExpressCard slots, e.g.,
> https://bugzilla.kernel.org/show_bug.cgi?id=40802 .
> 
> If you want "pci=nopciehp" as a way for users to deal with this
> problem by forcing the use of acpiphp, I object.  Windows manages to
> make these slots work without having users do anything special, so we
> should be able to do it, too.

And one can use pcie_ports=compat to work around these problems anyway.

Thanks,
Rafael


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

Re: [RFC PATCH v5 7/8] PCI/PCIe: add "pci=nopciehp" to disable PCIe native hotplug

[Rafael J. Wysocki]

On Friday, January 18, 2013 09:50:59 AM Yinghai Lu wrote:
> On Fri, Jan 18, 2013 at 9:35 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > If you want "pci=nopciehp" as a way for users to deal with this
> > problem by forcing the use of acpiphp, I object.  Windows manages to
> > make these slots work without having users do anything special, so we
> > should be able to do it, too.
> 
> Agreed.
> 
> We need to think that more.
> 
> I think that we should fix acpiphp, and should follow first come and
> first serve for acpiphp and pciehp.

That would introduce regressions for some users, though.

I actually think we should merge acpiphp with pciehp and make one driver that
can use both types of signalling.  There would be a problem with getting
notifications for the same event from both sources, but that doesn't seem to
be unsolvable.

And I don't think that that one combined driver should be modular.

Thanks,
Rafael


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

Re: [RFC PATCH v5 7/8] PCI/PCIe: add "pci=nopciehp" to disable PCIe native hotplug

[Yijing Wang]

于 2013-01-19 1:35, Bjorn Helgaas 写道:
> On Fri, Jan 18, 2013 at 9:07 AM, Jiang Liu <liuj97@gmail.com> wrote:
>> If user specifies "pci=nopciehp" on kernel boot command line, OSPM
>> won't claim PCIe native hotplug service from firmware and no PCIe
>> port devices will be created for PCIe native hotplug service.
> 
> Why do we need this option?
> 
> If I understand correctly, there are machines where it *looks* like we
> should use pciehp, but pciehp doesn't work because we don't get the
> interrupts we expect.  On those machines, we have to use acpiphp
> instead.  It seems like many Dell XPS laptops have this issue with
> ExpressCard slots, e.g.,
> https://bugzilla.kernel.org/show_bug.cgi?id=40802 .

What about use modprobe pciehp pciehp_poll_mode=1?
If just cannot receive the interrupt, maybe this module parameter will fix it.

> 
> If you want "pci=nopciehp" as a way for users to deal with this
> problem by forcing the use of acpiphp, I object.  Windows manages to
> make these slots work without having users do anything special, so we
> should be able to do it, too.

In fact, pcie native hotplug may not be implemented perfectly,
for example, I found some x86 machines pcie native hotplug slots that have problems
with latch register. After inserted a pci card, the latch still report latch open state.
So we cannot use pciehp to hotplug, But this problem cannot detect while system bootup or
load pciehp module. And now kernel force to use pciehp if pci slotcap has hotplug capable.
So I agree as Yinghai said users should have choice to choose hotplug module to use.

> 
>> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
>> ---
>>  Documentation/kernel-parameters.txt |    2 ++
>>  drivers/acpi/pci_root.c             |    3 ++-
>>  drivers/pci/pci.c                   |    2 ++
>>  drivers/pci/pcie/portdrv_core.c     |    5 +++--
>>  drivers/pci/pcie/portdrv_pci.c      |    3 +++
>>  include/linux/pci.h                 |    9 +++++++++
>>  6 files changed, 21 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
>> index 9776f06..28dd0ad 100644
>> --- a/Documentation/kernel-parameters.txt
>> +++ b/Documentation/kernel-parameters.txt
>> @@ -2106,6 +2106,8 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>>                 noaer           [PCIE] If the PCIEAER kernel config parameter is
>>                                 enabled, this kernel boot option can be used to
>>                                 disable the use of PCIE advanced error reporting.
>> +               nopciehp        [PCIE] this kernel boot option can be used to
>> +                               disable PCIe native hotplug.
>>                 nodomains       [PCI] Disable support for multiple PCI
>>                                 root domains (aka PCI segments, in ACPI-speak).
>>                 nommconf        [X86] Disable use of MMCONFIG for PCI
>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>> index c6200ff..c37eedb 100644
>> --- a/drivers/acpi/pci_root.c
>> +++ b/drivers/acpi/pci_root.c
>> @@ -551,8 +551,9 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
>>         if (!pcie_ports_disabled
>>             && (flags & ACPI_PCIE_REQ_SUPPORT) == ACPI_PCIE_REQ_SUPPORT) {
>>                 flags = OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL
>> -                       | OSC_PCI_EXPRESS_NATIVE_HP_CONTROL
>>                         | OSC_PCI_EXPRESS_PME_CONTROL;
>> +               if (!pcie_native_hotplug_disabled)
>> +                       flags |= OSC_PCI_EXPRESS_NATIVE_HP_CONTROL;
>>
>>                 if (pci_aer_available()) {
>>                         if (aer_acpi_firmware_first())
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index 2f8f4c6..34b2c83 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -3869,6 +3869,8 @@ static int __init pci_setup(char *str)
>>                                 pci_no_msi();
>>                         } else if (!strcmp(str, "noaer")) {
>>                                 pci_no_aer();
>> +                       } else if (!strcmp(str, "nopciehp")) {
>> +                               pcie_no_native_hotplug();
>>                         } else if (!strncmp(str, "realloc=", 8)) {
>>                                 pci_realloc_get_opt(str + 8);
>>                         } else if (!strncmp(str, "realloc", 7)) {
>> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
>> index ed129b4..e7e1679 100644
>> --- a/drivers/pci/pcie/portdrv_core.c
>> +++ b/drivers/pci/pcie/portdrv_core.c
>> @@ -263,8 +263,9 @@ static int get_port_device_capability(struct pci_dev *dev)
>>
>>         err = pcie_port_platform_notify(dev, &cap_mask);
>>         if (!pcie_ports_auto) {
>> -               cap_mask = PCIE_PORT_SERVICE_PME | PCIE_PORT_SERVICE_HP
>> -                               | PCIE_PORT_SERVICE_VC;
>> +               cap_mask = PCIE_PORT_SERVICE_PME | PCIE_PORT_SERVICE_VC;
>> +               if (!pcie_native_hotplug_disabled)
>> +                       cap_mask |= PCIE_PORT_SERVICE_HP;
>>                 if (pci_aer_available())
>>                         cap_mask |= PCIE_PORT_SERVICE_AER;
>>         } else if (err) {
>> diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
>> index 0761d90..018cee0 100644
>> --- a/drivers/pci/pcie/portdrv_pci.c
>> +++ b/drivers/pci/pcie/portdrv_pci.c
>> @@ -40,6 +40,9 @@ bool pcie_ports_disabled;
>>   */
>>  bool pcie_ports_auto = true;
>>
>> +/* If set, the PCIe native hotplug will not be used. */
>> +bool pcie_native_hotplug_disabled;
>> +
>>  static int __init pcie_port_setup(char *str)
>>  {
>>         if (!strncmp(str, "compat", 6)) {
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 12e5447..715e17b 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -1138,9 +1138,18 @@ extern int pci_msi_enabled(void);
>>  #ifdef CONFIG_PCIEPORTBUS
>>  extern bool pcie_ports_disabled;
>>  extern bool pcie_ports_auto;
>> +extern bool pcie_native_hotplug_disabled;
>> +
>> +static inline void pcie_no_native_hotplug(void)
>> +{
>> +       pcie_native_hotplug_disabled = true;
>> +}
>>  #else
>>  #define pcie_ports_disabled    true
>>  #define pcie_ports_auto                false
>> +#define pcie_native_hotplug_disabled   true
>> +
>> +static inline void pcie_no_native_hotplug(void) { }
>>  #endif
>>
>>  #ifndef CONFIG_PCIEASPM
>> --
>> 1.7.9.5
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [RFC PATCH v5 7/8] PCI/PCIe: add "pci=nopciehp" to disable PCIe native hotplug

[Greg Kroah-Hartman]

On Sat, Jan 19, 2013 at 09:56:24AM +0800, Yijing Wang wrote:
> 于 2013-01-19 1:35, Bjorn Helgaas 写道:
> > On Fri, Jan 18, 2013 at 9:07 AM, Jiang Liu <liuj97@gmail.com> wrote:
> >> If user specifies "pci=nopciehp" on kernel boot command line, OSPM
> >> won't claim PCIe native hotplug service from firmware and no PCIe
> >> port devices will be created for PCIe native hotplug service.
> > 
> > Why do we need this option?
> > 
> > If I understand correctly, there are machines where it *looks* like we
> > should use pciehp, but pciehp doesn't work because we don't get the
> > interrupts we expect.  On those machines, we have to use acpiphp
> > instead.  It seems like many Dell XPS laptops have this issue with
> > ExpressCard slots, e.g.,
> > https://bugzilla.kernel.org/show_bug.cgi?id=40802 .
> 
> What about use modprobe pciehp pciehp_poll_mode=1?
> If just cannot receive the interrupt, maybe this module parameter will fix it.

You can't add a new option that you now force hardware that was working
with a different module to now define and use.  It needs to be
"automatic".

> > If you want "pci=nopciehp" as a way for users to deal with this
> > problem by forcing the use of acpiphp, I object.  Windows manages to
> > make these slots work without having users do anything special, so we
> > should be able to do it, too.
> 
> In fact, pcie native hotplug may not be implemented perfectly,

Oh, we know that is true, it's the problem here.  Fixing BIOSes would be
nice, but we can't do that :(

greg k-h

Re: [RFC PATCH v5 1/8] PCI: make PCI device create/destroy logic symmetric

[Rafael J. Wysocki]

On Saturday, January 19, 2013 12:07:39 AM Jiang Liu wrote:
> According to device model documentation, the way to create/destroy PCI
> devices should be symmetric.
> 
> /**
>  * device_del - delete device from system.
>  * @dev: device.
>  *
>  * This is the first part of the device unregistration
>  * sequence. This removes the device from the lists we control
>  * from here, has it removed from the other driver model
>  * subsystems it was added to in device_add(), and removes it
>  * from the kobject hierarchy.
>  *
>  * NOTE: this should be called manually _iff_ device_add() was
>  * also called manually.
>  */
> 
> The rule is to either use
> 1) device_register()/device_unregister()
> or
> 2) device_initialize()/device_add()/device_del()/put_device().
> 
> So change PCI core logic to follow the rule and get rid of the redundant
> pci_dev_get()/pci_dev_put() pair.

This seems to be the same as https://patchwork.kernel.org/patch/2000051/

Can you please avoid sending duplicates?

Rafael


> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> Reviewed-by: Yinghai Lu <yinghai@kernel.org>
> ---
>  drivers/pci/probe.c  |    1 -
>  drivers/pci/remove.c |    4 ++--
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 4f7f963..ec5d28f 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1294,7 +1294,6 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
>  {
>  	device_initialize(&dev->dev);
>  	dev->dev.release = pci_release_dev;
> -	pci_dev_get(dev);
>  
>  	dev->dev.dma_mask = &dev->dma_mask;
>  	dev->dev.dma_parms = &dev->dma_parms;
> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
> index 513972f..10693f5 100644
> --- a/drivers/pci/remove.c
> +++ b/drivers/pci/remove.c
> @@ -22,7 +22,7 @@ static void pci_stop_dev(struct pci_dev *dev)
>  	if (dev->is_added) {
>  		pci_proc_detach_device(dev);
>  		pci_remove_sysfs_dev_files(dev);
> -		device_unregister(&dev->dev);
> +		device_del(&dev->dev);
>  		dev->is_added = 0;
>  	}
>  
> @@ -37,7 +37,7 @@ static void pci_destroy_dev(struct pci_dev *dev)
>  	up_write(&pci_bus_sem);
>  
>  	pci_free_resources(dev);
> -	pci_dev_put(dev);
> +	put_device(&dev->dev);
>  }
>  
>  void pci_remove_bus(struct pci_bus *bus)
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH v5 8/8] PCI/PCIe: only claim PME from firmware when CONFIG_PCIE_PME is enabled

[Rafael J. Wysocki]

On Saturday, January 19, 2013 12:07:46 AM Jiang Liu wrote:
> If CONFIG_PCIE_PME is not defined, system should avoid claiming PME from
> firmware so firmware could still manage PME events for those devices.
> Also don't create PCIe port device for PME service if CONFIG_PCIE_PME
> is not defined.

No, this isn't correct.

We know from experience that it is in fact necessary to request control of
either all PCIe native features or none of them.  Anything else leads to
very "interesting" failure modes on some systems.

I think we should just remove CONFIG_PCIE_PME instead.

Thanks,
Rafael


> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> ---
>  drivers/acpi/pci_root.c         |    5 +++--
>  drivers/pci/pcie/portdrv_core.c |    4 +++-
>  2 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index c37eedb..7f7e464 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -550,8 +550,9 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
>  
>  	if (!pcie_ports_disabled
>  	    && (flags & ACPI_PCIE_REQ_SUPPORT) == ACPI_PCIE_REQ_SUPPORT) {
> -		flags = OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL
> -			| OSC_PCI_EXPRESS_PME_CONTROL;
> +		flags = OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL;
> +		if (IS_ENABLED(CONFIG_PCIE_PME))
> +			flags |= OSC_PCI_EXPRESS_PME_CONTROL;
>  		if (!pcie_native_hotplug_disabled)
>  			flags |= OSC_PCI_EXPRESS_NATIVE_HP_CONTROL;
>  
> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> index e7e1679..7e6546f 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -263,7 +263,9 @@ static int get_port_device_capability(struct pci_dev *dev)
>  
>  	err = pcie_port_platform_notify(dev, &cap_mask);
>  	if (!pcie_ports_auto) {
> -		cap_mask = PCIE_PORT_SERVICE_PME | PCIE_PORT_SERVICE_VC;
> +		cap_mask = PCIE_PORT_SERVICE_VC;
> +		if (IS_ENABLED(CONFIG_PCIE_PME))
> +			cap_mask |= PCIE_PORT_SERVICE_PME;
>  		if (!pcie_native_hotplug_disabled)
>  			cap_mask |= PCIE_PORT_SERVICE_HP;
>  		if (pci_aer_available())
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH v5 3/8] PCI: add a blocking notifier chain for PCI bus addition/removal

[Rafael J. Wysocki]

On Saturday, January 19, 2013 12:07:41 AM Jiang Liu wrote:
> When adding/removing a PCI bus, some other components want to be
> notified so they could update their states. For example, the pci_slot
> driver needs to create/destroy PCI slots attaching to the PCI bus.
> So introduce a dedicate blocking notifier chain for PCI bus
> addition/removal events. It could be extended to support PCI device
> addition/removal events in the future when needed.
> 
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> ---
>  drivers/pci/bus.c    |   24 ++++++++++++++++++++++++
>  drivers/pci/pci.h    |    1 +
>  drivers/pci/probe.c  |    2 ++
>  drivers/pci/remove.c |    1 +
>  include/linux/pci.h  |   12 ++++++++++++
>  5 files changed, 40 insertions(+)
> 
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index 4c843cd..8bbd3f2 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -13,11 +13,33 @@
>  #include <linux/errno.h>
>  #include <linux/ioport.h>
>  #include <linux/proc_fs.h>
> +#include <linux/notifier.h>
>  #include <linux/init.h>
>  #include <linux/slab.h>
>  
>  #include "pci.h"
>  
> +static BLOCKING_NOTIFIER_HEAD(pci_bus_notifier_chain);
> +
> +int pci_bus_register_notifier(struct notifier_block *nb)
> +{
> +	return blocking_notifier_chain_register(&pci_bus_notifier_chain, nb);
> +}
> +
> +int pci_bus_unregister_notifier(struct notifier_block *nb)
> +{
> +	return blocking_notifier_chain_unregister(&pci_bus_notifier_chain, nb);
> +}
> +
> +void pci_bus_call_notifier(struct pci_bus *bus, unsigned long code)
> +{
> +	int ret;
> +
> +	ret = blocking_notifier_call_chain(&pci_bus_notifier_chain,
> +					   code, bus);
> +	WARN_ON(ret != NOTIFY_DONE && ret != NOTIFY_OK);

I'm not sure if this is a good idea.  WARN_ON() is quite a heavy tool.

> +}
> +
>  void pci_add_resource_offset(struct list_head *resources, struct resource *res,
>  			     resource_size_t offset)
>  {
> @@ -197,6 +219,8 @@ int pci_bus_add_child(struct pci_bus *bus)
>  	if (retval)
>  		return retval;
>  
> +	pci_bus_call_notifier(bus, PCI_NOTIFY_POST_BUS_ADD);
> +
>  	bus->is_added = 1;
>  
>  	/* Create legacy_io and legacy_mem files for this bus */
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 7414094..988e4b4 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -208,6 +208,7 @@ extern int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
>  extern int pci_resource_bar(struct pci_dev *dev, int resno,
>  			    enum pci_bar_type *type);
>  extern int pci_bus_add_child(struct pci_bus *bus);
> +extern void pci_bus_call_notifier(struct pci_bus *bus, unsigned long code);
>  extern void pci_enable_ari(struct pci_dev *dev);
>  /**
>   * pci_ari_enabled - query ARI forwarding status
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index adb77c6..7db6528 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1679,6 +1679,8 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>  	if (error)
>  		goto class_dev_reg_err;
>  
> +	pci_bus_call_notifier(b, PCI_NOTIFY_POST_BUS_ADD);
> +
>  	/* 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 0d03026..2d8fbc8 100644
> --- a/drivers/pci/remove.c
> +++ b/drivers/pci/remove.c
> @@ -50,6 +50,7 @@ void pci_remove_bus(struct pci_bus *bus)
>  	up_write(&pci_bus_sem);
>  	if (bus->is_added) {
>  		pci_remove_legacy_files(bus);
> +		pci_bus_call_notifier(bus, PCI_NOTIFY_PRE_BUS_DEL);
>  		device_del(&bus->dev);
>  	}
>  	put_device(&bus->dev);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index ee21795..12e5447 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1033,6 +1033,18 @@ const struct pci_device_id *pci_match_id(const struct pci_device_id *ids,
>  int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max,
>  		    int pass);
>  
> +/*
> + * All notifiers below get called with the target struct pci_bus *bus as
> + * an argument.
> + * Note: all PCI bus notifier must return success. Currently there's no
> + * error handling if any notifier returns error code.
> + */
> +#define	PCI_NOTIFY_POST_BUS_ADD		0x00000001 /* PCI bus has been added */
> +#define	PCI_NOTIFY_PRE_BUS_DEL		0x00000002 /* PCI bus will be deleted */

I would call them PCI_EVENT_BUS_ADDED and PCI_EVENT_BUS_REMOVE, respectively.

> +
> +int pci_bus_register_notifier(struct notifier_block *nb);
> +int pci_bus_unregister_notifier(struct notifier_block *nb);
> +
>  void pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
>  		  void *userdata);
>  int pci_cfg_space_size_ext(struct pci_dev *dev);

Apart from the nitpicks above, looks good.

Thanks,
Rafael


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

Re: [RFC PATCH v5 4/8] ACPI, PCI: avoid building pci_slot as module

[Rafael J. Wysocki]

On Saturday, January 19, 2013 12:07:42 AM Jiang Liu wrote:
> As discussed in thread at https://patchwork.kernel.org/patch/1946851/,
> there's no value in supporting CONFIG_ACPI_PCI_SLOT=m any more.
> So change Kconfig and code to only support building pci_slot as
> built-in driver.
> 
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>

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

> ---
>  drivers/acpi/Kconfig    |    5 +----
>  drivers/acpi/internal.h |    5 +++++
>  drivers/acpi/pci_slot.c |   13 +------------
>  drivers/acpi/scan.c     |    1 +
>  4 files changed, 8 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 0300bf6..7efd0d0 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -299,7 +299,7 @@ config ACPI_DEBUG_FUNC_TRACE
>  	  is about half of the penalty and is rarely useful.
>  
>  config ACPI_PCI_SLOT
> -	tristate "PCI slot detection driver"
> +	bool "PCI slot detection driver"
>  	depends on SYSFS
>  	default n
>  	help
> @@ -308,9 +308,6 @@ config ACPI_PCI_SLOT
>  	  i.e., segment/bus/device/function tuples, with physical slots in
>  	  the system.  If you are unsure, say N.
>  
> -	  To compile this driver as a module, choose M here:
> -	  the module will be called pci_slot.
> -
>  config X86_PM_TIMER
>  	bool "Power Management Timer Support" if EXPERT
>  	depends on X86
> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> index e050254..7374cfc 100644
> --- a/drivers/acpi/internal.h
> +++ b/drivers/acpi/internal.h
> @@ -67,6 +67,11 @@ 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);
>  int acpi_ec_init(void);
>  int acpi_ec_ecdt_probe(void);
> diff --git a/drivers/acpi/pci_slot.c b/drivers/acpi/pci_slot.c
> index d22585f..a7d7e77 100644
> --- a/drivers/acpi/pci_slot.c
> +++ b/drivers/acpi/pci_slot.c
> @@ -330,19 +330,8 @@ static struct dmi_system_id acpi_pci_slot_dmi_table[] __initdata = {
>  	{}
>  };
>  
> -static int __init
> -acpi_pci_slot_init(void)
> +void __init acpi_pci_slot_init(void)
>  {
>  	dmi_check_system(acpi_pci_slot_dmi_table);
>  	acpi_pci_register_driver(&acpi_pci_slot_driver);
> -	return 0;
>  }
> -
> -static void __exit
> -acpi_pci_slot_exit(void)
> -{
> -	acpi_pci_unregister_driver(&acpi_pci_slot_driver);
> -}
> -
> -module_init(acpi_pci_slot_init);
> -module_exit(acpi_pci_slot_exit);
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index f8a0d0f..cb964ac 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1732,6 +1732,7 @@ int __init acpi_scan_init(void)
>  
>  	acpi_power_init();
>  	acpi_pci_root_init();
> +	acpi_pci_slot_init();
>  
>  	/*
>  	 * Enumerate devices in the ACPI namespace.
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH v5 5/8] PCI, ACPI: hook PCI bus notifications to create/destroy PCI slots

[Rafael J. Wysocki]

On Saturday, January 19, 2013 12:07:43 AM Jiang Liu wrote:
> Currently the pci_slot driver doesn't update PCI slot information
> when PCI device hotplug event happens, which may cause memory leak
> and returning stale information to user.
> 
> By hooking PCI bus notifications, this patch unifies the way to
> create/destroy PCI slots when:
> 1) create PCI hierarchy at boot time
> 2) create PCI hierarchy for PCI root bridge hot-adding
> 3) create PCI hierarchy for PCI device hotplug hot-adding
> 4) destroy PCI hierarchy for PCI root bridge hot-removal
> 4) destroy PCI hierarchy for PCI device hot-removal

This patch also removes some code which apparently is not necessary any more.
It would be good to write in the changelog _why_ that code isn't necessary.

Apart from this, looks good.

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

> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> ---
>  drivers/acpi/pci_slot.c |  197 +++++++++++++----------------------------------
>  1 file changed, 54 insertions(+), 143 deletions(-)
> 
> diff --git a/drivers/acpi/pci_slot.c b/drivers/acpi/pci_slot.c
> index a7d7e77..2b38f4d 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;
> @@ -62,20 +64,12 @@ ACPI_MODULE_NAME("pci_slot");
>  #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 +108,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 +119,23 @@ 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 +150,9 @@ 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,137 +162,60 @@ 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)
> +static void acpi_pci_slot_notify_add(struct pci_bus *bus)
>  {
> -	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_handle handle = NULL;
>  
> -/*
> - * acpi_pci_slot_add
> - * @handle: points to an acpi_pci_root
> - */
> -static int
> -acpi_pci_slot_add(struct acpi_pci_root *root)
> -{
> -	acpi_status status;
> -
> -	status = walk_root_bridge(root, register_slot);
> -	if (ACPI_FAILURE(status))
> -		err("%s: register_slot failure - %d\n", __func__, status);
> +	if (bus->bridge)
> +		handle = DEVICE_ACPI_HANDLE(bus->bridge);
> +	if (!handle)
> +		return;
>  
> -	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)
> +static void acpi_pci_slot_notify_del(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);
>  		}
>  	}
>  	mutex_unlock(&slot_list_lock);
>  }
>  
> +static int acpi_pci_slot_notify_fn(struct notifier_block *nb,
> +				   unsigned long event, void *data)
> +{
> +	struct pci_bus *bus = data;
> +
> +	switch (event) {
> +	case PCI_NOTIFY_POST_BUS_ADD:
> +		acpi_pci_slot_notify_add(bus);
> +		break;
> +	case PCI_NOTIFY_PRE_BUS_DEL:
> +		acpi_pci_slot_notify_del(bus);
> +		break;
> +	default:
> +		return NOTIFY_DONE;
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
> +static struct notifier_block acpi_pci_slot_notifier = {
> +	.notifier_call = &acpi_pci_slot_notify_fn,
> +};
> +
>  static int do_sta_before_sun(const struct dmi_system_id *d)
>  {
>  	info("%s detected: will evaluate _STA before calling _SUN\n", d->ident);
> @@ -333,5 +244,5 @@ 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);
> +	pci_bus_register_notifier(&acpi_pci_slot_notifier);
>  }
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH v5 3/8] PCI: add a blocking notifier chain for PCI bus addition/removal

[Jiang Liu]

On 01/21/2013 07:54 AM, Rafael J. Wysocki wrote:
> On Saturday, January 19, 2013 12:07:41 AM Jiang Liu wrote:
>> When adding/removing a PCI bus, some other components want to be
snip
>> +
>> +void pci_bus_call_notifier(struct pci_bus *bus, unsigned long code)
>> +{
>> +	int ret;
>> +
>> +	ret = blocking_notifier_call_chain(&pci_bus_notifier_chain,
>> +					   code, bus);
>> +	WARN_ON(ret != NOTIFY_DONE && ret != NOTIFY_OK);
> 
> I'm not sure if this is a good idea.  WARN_ON() is quite a heavy tool.
Hi Rafael,
	How about WARN_ONCE() here?

> 
>> +}
>> +
>>  void pci_add_resource_offset(struct list_head *resources, struct resource *res,
>>  			     resource_size_t offset)
>>  {
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index ee21795..12e5447 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -1033,6 +1033,18 @@ const struct pci_device_id *pci_match_id(const struct pci_device_id *ids,
>>  int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max,
>>  		    int pass);
>>  
>> +/*
>> + * All notifiers below get called with the target struct pci_bus *bus as
>> + * an argument.
>> + * Note: all PCI bus notifier must return success. Currently there's no
>> + * error handling if any notifier returns error code.
>> + */
>> +#define	PCI_NOTIFY_POST_BUS_ADD		0x00000001 /* PCI bus has been added */
>> +#define	PCI_NOTIFY_PRE_BUS_DEL		0x00000002 /* PCI bus will be deleted */
> 
> I would call them PCI_EVENT_BUS_ADDED and PCI_EVENT_BUS_REMOVE, respectively.
Sure, will use them in next version.

> 
>> +
>> +int pci_bus_register_notifier(struct notifier_block *nb);
>> +int pci_bus_unregister_notifier(struct notifier_block *nb);
>> +
>>  void pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
>>  		  void *userdata);
>>  int pci_cfg_space_size_ext(struct pci_dev *dev);
> 
> Apart from the nitpicks above, looks good.
Thanks for review!

Regards!
Gerry
> 
> Thanks,
> Rafael
> 
> 

Re: [RFC PATCH v5 8/8] PCI/PCIe: only claim PME from firmware when CONFIG_PCIE_PME is enabled

[Jiang Liu]

On 01/21/2013 07:43 AM, Rafael J. Wysocki wrote:
> On Saturday, January 19, 2013 12:07:46 AM Jiang Liu wrote:
>> If CONFIG_PCIE_PME is not defined, system should avoid claiming PME from
>> firmware so firmware could still manage PME events for those devices.
>> Also don't create PCIe port device for PME service if CONFIG_PCIE_PME
>> is not defined.
> 
> No, this isn't correct.
> 
> We know from experience that it is in fact necessary to request control of
> either all PCIe native features or none of them.  Anything else leads to
> very "interesting" failure modes on some systems.
> 
> I think we should just remove CONFIG_PCIE_PME instead.
Hi Rafael,
	Thanks for reminder, don't know these tricky things about BIOS:)
Seems we can't easily remove CONFIG_PCIE_PME currently, so I will just
drop this patch.
Regards!
Gerry

> 
> Thanks,
> Rafael
> 
> 
>> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
>> ---
>>  drivers/acpi/pci_root.c         |    5 +++--
>>  drivers/pci/pcie/portdrv_core.c |    4 +++-
>>  2 files changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>> index c37eedb..7f7e464 100644
>> --- a/drivers/acpi/pci_root.c
>> +++ b/drivers/acpi/pci_root.c
>> @@ -550,8 +550,9 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
>>  
>>  	if (!pcie_ports_disabled
>>  	    && (flags & ACPI_PCIE_REQ_SUPPORT) == ACPI_PCIE_REQ_SUPPORT) {
>> -		flags = OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL
>> -			| OSC_PCI_EXPRESS_PME_CONTROL;
>> +		flags = OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL;
>> +		if (IS_ENABLED(CONFIG_PCIE_PME))
>> +			flags |= OSC_PCI_EXPRESS_PME_CONTROL;
>>  		if (!pcie_native_hotplug_disabled)
>>  			flags |= OSC_PCI_EXPRESS_NATIVE_HP_CONTROL;
>>  
>> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
>> index e7e1679..7e6546f 100644
>> --- a/drivers/pci/pcie/portdrv_core.c
>> +++ b/drivers/pci/pcie/portdrv_core.c
>> @@ -263,7 +263,9 @@ static int get_port_device_capability(struct pci_dev *dev)
>>  
>>  	err = pcie_port_platform_notify(dev, &cap_mask);
>>  	if (!pcie_ports_auto) {
>> -		cap_mask = PCIE_PORT_SERVICE_PME | PCIE_PORT_SERVICE_VC;
>> +		cap_mask = PCIE_PORT_SERVICE_VC;
>> +		if (IS_ENABLED(CONFIG_PCIE_PME))
>> +			cap_mask |= PCIE_PORT_SERVICE_PME;
>>  		if (!pcie_native_hotplug_disabled)
>>  			cap_mask |= PCIE_PORT_SERVICE_HP;
>>  		if (pci_aer_available())
>>

Re: [RFC PATCH v5 3/8] PCI: add a blocking notifier chain for PCI bus addition/removal

[Rafael J. Wysocki]

On Tuesday, January 22, 2013 12:18:55 AM Jiang Liu wrote:
> On 01/21/2013 07:54 AM, Rafael J. Wysocki wrote:
> > On Saturday, January 19, 2013 12:07:41 AM Jiang Liu wrote:
> >> When adding/removing a PCI bus, some other components want to be
> snip
> >> +
> >> +void pci_bus_call_notifier(struct pci_bus *bus, unsigned long code)
> >> +{
> >> +	int ret;
> >> +
> >> +	ret = blocking_notifier_call_chain(&pci_bus_notifier_chain,
> >> +					   code, bus);
> >> +	WARN_ON(ret != NOTIFY_DONE && ret != NOTIFY_OK);
> > 
> > I'm not sure if this is a good idea.  WARN_ON() is quite a heavy tool.
> Hi Rafael,
> 	How about WARN_ONCE() here?

Well, that depends on what you think it will be useful for.

Thanks,
Rafael


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

Re: [RFC PATCH v5 7/8] PCI/PCIe: add "pci=nopciehp" to disable PCIe native hotplug

[Jiang Liu]

Hi Yinghai and Rafael,
	Thanks for your comments, seems you all don't like the proposed solution,
Then how about this solution:
1) keep acpiphp and pciehp as modules
2) introduce a new interface pci_for_each_bus() to walk all PCI buses.
3) use pci_for_each_bus()  to replace ACPI PCI subdriver for acpiphp.
4) use PCI bus notifier chain to support PCI device and host bridge hotplug.
5) add some quirks to handle PCIe bridges having issues with native PCIe hotplug.

Regards!
Gerry

On 01/19/2013 06:08 AM, Rafael J. Wysocki wrote:
> On Friday, January 18, 2013 09:50:59 AM Yinghai Lu wrote:
>> On Fri, Jan 18, 2013 at 9:35 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>> If you want "pci=nopciehp" as a way for users to deal with this
>>> problem by forcing the use of acpiphp, I object.  Windows manages to
>>> make these slots work without having users do anything special, so we
>>> should be able to do it, too.
>>
>> Agreed.
>>
>> We need to think that more.
>>
>> I think that we should fix acpiphp, and should follow first come and
>> first serve for acpiphp and pciehp.
> 
> That would introduce regressions for some users, though.
> 
> I actually think we should merge acpiphp with pciehp and make one driver that
> can use both types of signalling.  There would be a problem with getting
> notifications for the same event from both sources, but that doesn't seem to
> be unsolvable.
> 
> And I don't think that that one combined driver should be modular.
> 
> Thanks,
> Rafael
> 
> 

Re: [RFC PATCH v5 0/8] introduce PCI bus notifier chain to get rid of the ACPI PCI subdriver interfaces

[Bjorn Helgaas]

On Fri, Jan 18, 2013 at 9:07 AM, Jiang Liu <liuj97@gmail.com> wrote:
> This is an RFC patchset to address review comments in thread at:
> https://patchwork.kernel.org/patch/1946851/. The patch just pasts
> compilation. If no objection to the new implementation, I will
> go on to modify acpiphp driver and conduct tests.
>
> The main changes from V4 to V5 includes:
> 1) introduce a dedicated notifier chain for PCI buses
> 2) change pci_slot as built-in driver
> 3) unify the way to create/destroy PCI slots
> 4) introduce a kernel option to disable PCIe native hotplug
>
> TODO:
> 1) change acpiphp as built-in and unify the way to create/destroy ACPI
>    based hotplug slots.
> 2) change other ACPI PCI subdriver in Yinghai's root bridge hotplug series
>    to use the PCI bus notifier chain.
> 3) Remove the ACPI PCI subdriver interface eventaully.
>
> Jiang Liu (8):
>   PCI: make PCI device create/destroy logic symmetric
>   PCI: split registration of PCI bus devices into two stages
>   PCI: add a blocking notifier chain for PCI bus addition/removal
>   ACPI, PCI: avoid building pci_slot as module
>   PCI, ACPI: hook PCI bus notifications to create/destroy PCI slots
>   pci_slot: replace printk(KERN_xxx) with pr_xxx()
>   PCI/PCIe: add "pci=nopciehp" to disable PCIe native hotplug
>   PCI/PCIe: only claim PME from firmware when CONFIG_PCIE_PME is
>     enabled
>
>  Documentation/kernel-parameters.txt |    2 +
>  drivers/acpi/Kconfig                |    5 +-
>  drivers/acpi/internal.h             |    5 +
>  drivers/acpi/pci_root.c             |    8 +-
>  drivers/acpi/pci_slot.c             |  217 ++++++++++-------------------------
>  drivers/acpi/scan.c                 |    1 +
>  drivers/pci/bus.c                   |   26 ++++-
>  drivers/pci/pci.c                   |    2 +
>  drivers/pci/pci.h                   |    1 +
>  drivers/pci/pcie/portdrv_core.c     |    7 +-
>  drivers/pci/pcie/portdrv_pci.c      |    3 +
>  drivers/pci/probe.c                 |    7 +-
>  drivers/pci/remove.c                |   15 +--
>  include/linux/pci.h                 |   21 ++++
>  14 files changed, 142 insertions(+), 178 deletions(-)

I think the problem we're trying to solve is that we don't initialize
hot-added devices, correctly, e.g., we don't set up AER, we don't
update acpi/pci_slot stuff, we probably don't set up PME etc.  We also
have similar issues like IOMMU init on powerpc.

Notifier chains seem like an unnecessarily complicated way to deal
with this.  They're great for communicating between modules that stay
at arm's length from each other.  But that's not the case here --
everything is PCI and is quite closely coupled.  I think AER, PME,
slot, etc., should  be initialized directly in pci_device_add() or
somewhere nearby.

This might sound a bit radical because it implies some fairly
far-reaching changes.  It means this code can't be a module (the only
one that can be built as a module today is pciehp, and I think
everybody agrees that we should make it static as soon as we can
figure out the acpiphp/pciehp issue).  I think it also means the
pcieportdrv concept is of dubious value, since all the services should
be known at build-time and we probably don't need a registration
interface for them.

Bjorn

Re: [RFC PATCH v5 4/8] ACPI, PCI: avoid building pci_slot as module

[Bjorn Helgaas]

On Sun, Jan 20, 2013 at 5:01 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Saturday, January 19, 2013 12:07:42 AM Jiang Liu wrote:
>> As discussed in thread at https://patchwork.kernel.org/patch/1946851/,
>> there's no value in supporting CONFIG_ACPI_PCI_SLOT=m any more.
>> So change Kconfig and code to only support building pci_slot as
>> built-in driver.
>>
>> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

I think we should eventually get rid of acpi_pci_register_driver() and
do this initialization directly from acpi_pci_root_add().  But
removing the module option here is a good first step.

Rafael, do you want to apply this (and [6/8]) via your tree?  If not,
I can take it.

>> ---
>>  drivers/acpi/Kconfig    |    5 +----
>>  drivers/acpi/internal.h |    5 +++++
>>  drivers/acpi/pci_slot.c |   13 +------------
>>  drivers/acpi/scan.c     |    1 +
>>  4 files changed, 8 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>> index 0300bf6..7efd0d0 100644
>> --- a/drivers/acpi/Kconfig
>> +++ b/drivers/acpi/Kconfig
>> @@ -299,7 +299,7 @@ config ACPI_DEBUG_FUNC_TRACE
>>         is about half of the penalty and is rarely useful.
>>
>>  config ACPI_PCI_SLOT
>> -     tristate "PCI slot detection driver"
>> +     bool "PCI slot detection driver"
>>       depends on SYSFS
>>       default n
>>       help
>> @@ -308,9 +308,6 @@ config ACPI_PCI_SLOT
>>         i.e., segment/bus/device/function tuples, with physical slots in
>>         the system.  If you are unsure, say N.
>>
>> -       To compile this driver as a module, choose M here:
>> -       the module will be called pci_slot.
>> -
>>  config X86_PM_TIMER
>>       bool "Power Management Timer Support" if EXPERT
>>       depends on X86
>> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
>> index e050254..7374cfc 100644
>> --- a/drivers/acpi/internal.h
>> +++ b/drivers/acpi/internal.h
>> @@ -67,6 +67,11 @@ 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);
>>  int acpi_ec_init(void);
>>  int acpi_ec_ecdt_probe(void);
>> diff --git a/drivers/acpi/pci_slot.c b/drivers/acpi/pci_slot.c
>> index d22585f..a7d7e77 100644
>> --- a/drivers/acpi/pci_slot.c
>> +++ b/drivers/acpi/pci_slot.c
>> @@ -330,19 +330,8 @@ static struct dmi_system_id acpi_pci_slot_dmi_table[] __initdata = {
>>       {}
>>  };
>>
>> -static int __init
>> -acpi_pci_slot_init(void)
>> +void __init acpi_pci_slot_init(void)
>>  {
>>       dmi_check_system(acpi_pci_slot_dmi_table);
>>       acpi_pci_register_driver(&acpi_pci_slot_driver);
>> -     return 0;
>>  }
>> -
>> -static void __exit
>> -acpi_pci_slot_exit(void)
>> -{
>> -     acpi_pci_unregister_driver(&acpi_pci_slot_driver);
>> -}
>> -
>> -module_init(acpi_pci_slot_init);
>> -module_exit(acpi_pci_slot_exit);
>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>> index f8a0d0f..cb964ac 100644
>> --- a/drivers/acpi/scan.c
>> +++ b/drivers/acpi/scan.c
>> @@ -1732,6 +1732,7 @@ int __init acpi_scan_init(void)
>>
>>       acpi_power_init();
>>       acpi_pci_root_init();
>> +     acpi_pci_slot_init();
>>
>>       /*
>>        * Enumerate devices in the ACPI namespace.
>>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH v5 4/8] ACPI, PCI: avoid building pci_slot as module

[Yinghai Lu]

On Mon, Jan 28, 2013 at 1:09 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Sun, Jan 20, 2013 at 5:01 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> On Saturday, January 19, 2013 12:07:42 AM Jiang Liu wrote:
>>> As discussed in thread at https://patchwork.kernel.org/patch/1946851/,
>>> there's no value in supporting CONFIG_ACPI_PCI_SLOT=m any more.
>>> So change Kconfig and code to only support building pci_slot as
>>> built-in driver.
>>>
>>> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
>>
>> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>
> I think we should eventually get rid of acpi_pci_register_driver() and
> do this initialization directly from acpi_pci_root_add().  But
> removing the module option here is a good first step.
>
> Rafael, do you want to apply this (and [6/8]) via your tree?  If not,
> I can take it.

If bios have messed up slot name or idx, we will get strange 1-1....
other crazy name.

if you really need to put it as built-in, may need to some command
line that user could switch it off.

Yinghai

Re: [RFC PATCH v5 4/8] ACPI, PCI: avoid building pci_slot as module

[Bjorn Helgaas]

On Mon, Jan 28, 2013 at 2:29 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Mon, Jan 28, 2013 at 1:09 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Sun, Jan 20, 2013 at 5:01 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>>> On Saturday, January 19, 2013 12:07:42 AM Jiang Liu wrote:
>>>> As discussed in thread at https://patchwork.kernel.org/patch/1946851/,
>>>> there's no value in supporting CONFIG_ACPI_PCI_SLOT=m any more.
>>>> So change Kconfig and code to only support building pci_slot as
>>>> built-in driver.
>>>>
>>>> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
>>>
>>> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>>
>> I think we should eventually get rid of acpi_pci_register_driver() and
>> do this initialization directly from acpi_pci_root_add().  But
>> removing the module option here is a good first step.
>>
>> Rafael, do you want to apply this (and [6/8]) via your tree?  If not,
>> I can take it.
>
> If bios have messed up slot name or idx, we will get strange 1-1....
> other crazy name.
>
> if you really need to put it as built-in, may need to some command
> line that user could switch it off.

It would save us all a lot of time if you gave an example and worked
through the scenario where this is a problem.

We already have the choice of having pci_slot built-in, so if there's
a bug in that config, we already have the bug.  This patch merely
removes a config where the bug might be covered up.

I don't know why "adding a command line switch" appeals to you as the
solution to every problem.  As far as I'm concerned that's not a
solution to ANY problem.  It might be a band-aid to enable users to
limp along while we figure out a correct solution, but it's certainly
not a resolution.

Bjorn

Re: [RFC PATCH v5 4/8] ACPI, PCI: avoid building pci_slot as module

[Yinghai Lu]

On Mon, Jan 28, 2013 at 1:52 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Mon, Jan 28, 2013 at 2:29 PM, Yinghai Lu <yinghai@kernel.org> wrote:
...
>> If bios have messed up slot name or idx, we will get strange 1-1....
>> other crazy name.
>>
>> if you really need to put it as built-in, may need to some command
>> line that user could switch it off.
>
> It would save us all a lot of time if you gave an example and worked
> through the scenario where this is a problem.
>
> We already have the choice of having pci_slot built-in, so if there's
> a bug in that config, we already have the bug.  This patch merely
> removes a config where the bug might be covered up.


for distribution, current it is with module, so user could blacklist
in module.conf

Now with built-in or not, distribution will have it built-in, and user
have no chance to
disable it.

> I don't know why "adding a command line switch" appeals to you as the
> solution to every problem.  As far as I'm concerned that's not a
> solution to ANY problem.  It might be a band-aid to enable users to
> limp along while we figure out a correct solution, but it's certainly
> not a resolution.

Looks like you want to remove command line support, right ?

Yinghai

Re: [RFC PATCH v5 4/8] ACPI, PCI: avoid building pci_slot as module

[Bjorn Helgaas]

On Mon, Jan 28, 2013 at 3:00 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Mon, Jan 28, 2013 at 1:52 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Mon, Jan 28, 2013 at 2:29 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> ...
>>> If bios have messed up slot name or idx, we will get strange 1-1....
>>> other crazy name.
>>>
>>> if you really need to put it as built-in, may need to some command
>>> line that user could switch it off.
>>
>> It would save us all a lot of time if you gave an example and worked
>> through the scenario where this is a problem.
>>
>> We already have the choice of having pci_slot built-in, so if there's
>> a bug in that config, we already have the bug.  This patch merely
>> removes a config where the bug might be covered up.
>
>
> for distribution, current it is with module, so user could blacklist
> in module.conf
>
> Now with built-in or not, distribution will have it built-in, and user
> have no chance to
> disable it.

CONFIG_ACPI_PCI_SLOT=y in RHEL6, so evidently they have this problem.

Asking users to edit module.conf by hand is not a solution, just like
asking users to boot with a command line option is not a solution.
That sort of stuff is fine for a hobbyist OS intended only for techie
geeks.  It's not fine for Linux.

If you would give a concrete example of the ACPI namespace info and
device config, hotplug sequence, etc., required to show the problem,
we could have a useful discussion about ways to fix it.  But if all
you have is FUD about "this might break and users won't have the
ability to edit modules.conf," that doesn't help me see why this patch
is a bad idea.

>> I don't know why "adding a command line switch" appeals to you as the
>> solution to every problem.  As far as I'm concerned that's not a
>> solution to ANY problem.  It might be a band-aid to enable users to
>> limp along while we figure out a correct solution, but it's certainly
>> not a resolution.
>
> Looks like you want to remove command line support, right ?
>
> Yinghai

Re: [RFC PATCH v5 4/8] ACPI, PCI: avoid building pci_slot as module

[Yinghai Lu]

On Mon, Jan 28, 2013 at 2:14 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>
> CONFIG_ACPI_PCI_SLOT=y in RHEL6, so evidently they have this problem.

oh, I only checked opensuse that has that set to m.

>
> Asking users to edit module.conf by hand is not a solution, just like
> asking users to boot with a command line option is not a solution.
> That sort of stuff is fine for a hobbyist OS intended only for techie
> geeks.  It's not fine for Linux.

not sure. add something in command line or conf files.
but recompile kernel is another story.

>
> If you would give a concrete example of the ACPI namespace info and
> device config, hotplug sequence, etc., required to show the problem,
> we could have a useful discussion about ways to fix it.  But if all
> you have is FUD about "this might break and users won't have the
> ability to edit modules.conf," that doesn't help me see why this patch
> is a bad idea.

Never mind, We should save your bandwidth to more patches.

Yinghai

Re: [RFC PATCH v5 0/8] introduce PCI bus notifier chain to get rid of the ACPI PCI subdriver interfaces

[Rafael J. Wysocki]

On Monday, January 28, 2013 01:56:33 PM Bjorn Helgaas wrote:
> On Fri, Jan 18, 2013 at 9:07 AM, Jiang Liu <liuj97@gmail.com> wrote:
> > This is an RFC patchset to address review comments in thread at:
> > https://patchwork.kernel.org/patch/1946851/. The patch just pasts
> > compilation. If no objection to the new implementation, I will
> > go on to modify acpiphp driver and conduct tests.
> >
> > The main changes from V4 to V5 includes:
> > 1) introduce a dedicated notifier chain for PCI buses
> > 2) change pci_slot as built-in driver
> > 3) unify the way to create/destroy PCI slots
> > 4) introduce a kernel option to disable PCIe native hotplug
> >
> > TODO:
> > 1) change acpiphp as built-in and unify the way to create/destroy ACPI
> >    based hotplug slots.
> > 2) change other ACPI PCI subdriver in Yinghai's root bridge hotplug series
> >    to use the PCI bus notifier chain.
> > 3) Remove the ACPI PCI subdriver interface eventaully.
> >
> > Jiang Liu (8):
> >   PCI: make PCI device create/destroy logic symmetric
> >   PCI: split registration of PCI bus devices into two stages
> >   PCI: add a blocking notifier chain for PCI bus addition/removal
> >   ACPI, PCI: avoid building pci_slot as module
> >   PCI, ACPI: hook PCI bus notifications to create/destroy PCI slots
> >   pci_slot: replace printk(KERN_xxx) with pr_xxx()
> >   PCI/PCIe: add "pci=nopciehp" to disable PCIe native hotplug
> >   PCI/PCIe: only claim PME from firmware when CONFIG_PCIE_PME is
> >     enabled
> >
> >  Documentation/kernel-parameters.txt |    2 +
> >  drivers/acpi/Kconfig                |    5 +-
> >  drivers/acpi/internal.h             |    5 +
> >  drivers/acpi/pci_root.c             |    8 +-
> >  drivers/acpi/pci_slot.c             |  217 ++++++++++-------------------------
> >  drivers/acpi/scan.c                 |    1 +
> >  drivers/pci/bus.c                   |   26 ++++-
> >  drivers/pci/pci.c                   |    2 +
> >  drivers/pci/pci.h                   |    1 +
> >  drivers/pci/pcie/portdrv_core.c     |    7 +-
> >  drivers/pci/pcie/portdrv_pci.c      |    3 +
> >  drivers/pci/probe.c                 |    7 +-
> >  drivers/pci/remove.c                |   15 +--
> >  include/linux/pci.h                 |   21 ++++
> >  14 files changed, 142 insertions(+), 178 deletions(-)
> 
> I think the problem we're trying to solve is that we don't initialize
> hot-added devices, correctly, e.g., we don't set up AER, we don't
> update acpi/pci_slot stuff, we probably don't set up PME etc.  We also
> have similar issues like IOMMU init on powerpc.
> 
> Notifier chains seem like an unnecessarily complicated way to deal
> with this.  They're great for communicating between modules that stay
> at arm's length from each other.  But that's not the case here --
> everything is PCI and is quite closely coupled.  I think AER, PME,
> slot, etc., should  be initialized directly in pci_device_add() or
> somewhere nearby.

I agree.

> This might sound a bit radical because it implies some fairly
> far-reaching changes.  It means this code can't be a module (the only
> one that can be built as a module today is pciehp, and I think
> everybody agrees that we should make it static as soon as we can
> figure out the acpiphp/pciehp issue).  I think it also means the
> pcieportdrv concept is of dubious value, since all the services should
> be known at build-time and we probably don't need a registration
> interface for them.

It is of dubious value regardless.  It just adds complexity for no gain.
Moreover, these things are in fact not mutually independent.

I've had a lot of headaches trying to work around that when I was working
on PME support and later on _OSC for root bridges.  Let's just take that
stuff away once and for good. :-)

Thanks,
Rafael


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

Re: [RFC PATCH v5 4/8] ACPI, PCI: avoid building pci_slot as module

[Rafael J. Wysocki]

On Monday, January 28, 2013 02:09:22 PM Bjorn Helgaas wrote:
> On Sun, Jan 20, 2013 at 5:01 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Saturday, January 19, 2013 12:07:42 AM Jiang Liu wrote:
> >> As discussed in thread at https://patchwork.kernel.org/patch/1946851/,
> >> there's no value in supporting CONFIG_ACPI_PCI_SLOT=m any more.
> >> So change Kconfig and code to only support building pci_slot as
> >> built-in driver.
> >>
> >> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> >
> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> I think we should eventually get rid of acpi_pci_register_driver() and
> do this initialization directly from acpi_pci_root_add().  But
> removing the module option here is a good first step.
> 
> Rafael, do you want to apply this (and [6/8]) via your tree?  If not,
> I can take it.

I will, thanks.

Rafael


> >> ---
> >>  drivers/acpi/Kconfig    |    5 +----
> >>  drivers/acpi/internal.h |    5 +++++
> >>  drivers/acpi/pci_slot.c |   13 +------------
> >>  drivers/acpi/scan.c     |    1 +
> >>  4 files changed, 8 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> >> index 0300bf6..7efd0d0 100644
> >> --- a/drivers/acpi/Kconfig
> >> +++ b/drivers/acpi/Kconfig
> >> @@ -299,7 +299,7 @@ config ACPI_DEBUG_FUNC_TRACE
> >>         is about half of the penalty and is rarely useful.
> >>
> >>  config ACPI_PCI_SLOT
> >> -     tristate "PCI slot detection driver"
> >> +     bool "PCI slot detection driver"
> >>       depends on SYSFS
> >>       default n
> >>       help
> >> @@ -308,9 +308,6 @@ config ACPI_PCI_SLOT
> >>         i.e., segment/bus/device/function tuples, with physical slots in
> >>         the system.  If you are unsure, say N.
> >>
> >> -       To compile this driver as a module, choose M here:
> >> -       the module will be called pci_slot.
> >> -
> >>  config X86_PM_TIMER
> >>       bool "Power Management Timer Support" if EXPERT
> >>       depends on X86
> >> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> >> index e050254..7374cfc 100644
> >> --- a/drivers/acpi/internal.h
> >> +++ b/drivers/acpi/internal.h
> >> @@ -67,6 +67,11 @@ 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);
> >>  int acpi_ec_init(void);
> >>  int acpi_ec_ecdt_probe(void);
> >> diff --git a/drivers/acpi/pci_slot.c b/drivers/acpi/pci_slot.c
> >> index d22585f..a7d7e77 100644
> >> --- a/drivers/acpi/pci_slot.c
> >> +++ b/drivers/acpi/pci_slot.c
> >> @@ -330,19 +330,8 @@ static struct dmi_system_id acpi_pci_slot_dmi_table[] __initdata = {
> >>       {}
> >>  };
> >>
> >> -static int __init
> >> -acpi_pci_slot_init(void)
> >> +void __init acpi_pci_slot_init(void)
> >>  {
> >>       dmi_check_system(acpi_pci_slot_dmi_table);
> >>       acpi_pci_register_driver(&acpi_pci_slot_driver);
> >> -     return 0;
> >>  }
> >> -
> >> -static void __exit
> >> -acpi_pci_slot_exit(void)
> >> -{
> >> -     acpi_pci_unregister_driver(&acpi_pci_slot_driver);
> >> -}
> >> -
> >> -module_init(acpi_pci_slot_init);
> >> -module_exit(acpi_pci_slot_exit);
> >> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> >> index f8a0d0f..cb964ac 100644
> >> --- a/drivers/acpi/scan.c
> >> +++ b/drivers/acpi/scan.c
> >> @@ -1732,6 +1732,7 @@ int __init acpi_scan_init(void)
> >>
> >>       acpi_power_init();
> >>       acpi_pci_root_init();
> >> +     acpi_pci_slot_init();
> >>
> >>       /*
> >>        * Enumerate devices in the ACPI namespace.
> >>
> > --
> > I speak only for myself.
> > Rafael J. Wysocki, Intel Open Source Technology Center.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH v5 0/8] introduce PCI bus notifier chain to get rid of the ACPI PCI subdriver interfaces

[Jiang Liu]

On 2013-1-29 8:34, Rafael J. Wysocki wrote:
> On Monday, January 28, 2013 01:56:33 PM Bjorn Helgaas wrote:
>> On Fri, Jan 18, 2013 at 9:07 AM, Jiang Liu <liuj97@gmail.com> wrote:
>>> This is an RFC patchset to address review comments in thread at:
>>> https://patchwork.kernel.org/patch/1946851/. The patch just pasts
>>> compilation. If no objection to the new implementation, I will
>>> go on to modify acpiphp driver and conduct tests.
>>>
>>> The main changes from V4 to V5 includes:
>>> 1) introduce a dedicated notifier chain for PCI buses
>>> 2) change pci_slot as built-in driver
>>> 3) unify the way to create/destroy PCI slots
>>> 4) introduce a kernel option to disable PCIe native hotplug
>>>
>>> TODO:
>>> 1) change acpiphp as built-in and unify the way to create/destroy ACPI
>>>    based hotplug slots.
>>> 2) change other ACPI PCI subdriver in Yinghai's root bridge hotplug series
>>>    to use the PCI bus notifier chain.
>>> 3) Remove the ACPI PCI subdriver interface eventaully.
>>>
>>> Jiang Liu (8):
>>>   PCI: make PCI device create/destroy logic symmetric
>>>   PCI: split registration of PCI bus devices into two stages
>>>   PCI: add a blocking notifier chain for PCI bus addition/removal
>>>   ACPI, PCI: avoid building pci_slot as module
>>>   PCI, ACPI: hook PCI bus notifications to create/destroy PCI slots
>>>   pci_slot: replace printk(KERN_xxx) with pr_xxx()
>>>   PCI/PCIe: add "pci=nopciehp" to disable PCIe native hotplug
>>>   PCI/PCIe: only claim PME from firmware when CONFIG_PCIE_PME is
>>>     enabled
>>>
>>>  Documentation/kernel-parameters.txt |    2 +
>>>  drivers/acpi/Kconfig                |    5 +-
>>>  drivers/acpi/internal.h             |    5 +
>>>  drivers/acpi/pci_root.c             |    8 +-
>>>  drivers/acpi/pci_slot.c             |  217 ++++++++++-------------------------
>>>  drivers/acpi/scan.c                 |    1 +
>>>  drivers/pci/bus.c                   |   26 ++++-
>>>  drivers/pci/pci.c                   |    2 +
>>>  drivers/pci/pci.h                   |    1 +
>>>  drivers/pci/pcie/portdrv_core.c     |    7 +-
>>>  drivers/pci/pcie/portdrv_pci.c      |    3 +
>>>  drivers/pci/probe.c                 |    7 +-
>>>  drivers/pci/remove.c                |   15 +--
>>>  include/linux/pci.h                 |   21 ++++
>>>  14 files changed, 142 insertions(+), 178 deletions(-)
>>
>> I think the problem we're trying to solve is that we don't initialize
>> hot-added devices, correctly, e.g., we don't set up AER, we don't
>> update acpi/pci_slot stuff, we probably don't set up PME etc.  We also
>> have similar issues like IOMMU init on powerpc.
>>
>> Notifier chains seem like an unnecessarily complicated way to deal
>> with this.  They're great for communicating between modules that stay
>> at arm's length from each other.  But that's not the case here --
>> everything is PCI and is quite closely coupled.  I think AER, PME,
>> slot, etc., should  be initialized directly in pci_device_add() or
>> somewhere nearby.
> 
> I agree.
> 
>> This might sound a bit radical because it implies some fairly
>> far-reaching changes.  It means this code can't be a module (the only
>> one that can be built as a module today is pciehp, and I think
>> everybody agrees that we should make it static as soon as we can
>> figure out the acpiphp/pciehp issue).  I think it also means the
>> pcieportdrv concept is of dubious value, since all the services should
>> be known at build-time and we probably don't need a registration
>> interface for them.
> 
> It is of dubious value regardless.  It just adds complexity for no gain.
> Moreover, these things are in fact not mutually independent.
> 
> I've had a lot of headaches trying to work around that when I was working
> on PME support and later on _OSC for root bridges.  Let's just take that
> stuff away once and for good. :-)
Hi Bjorn and Rafael,
	Thanks for advice. We will go this direction to change those modules
as built-in.
Regards!
Gerry

> 
> Thanks,
> Rafael
> 
> 

Re: [RFC PATCH v5 4/8] ACPI, PCI: avoid building pci_slot as module

[Jiang Liu]

On 2013-1-29 6:58, Yinghai Lu wrote:
> On Mon, Jan 28, 2013 at 2:14 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>
>> CONFIG_ACPI_PCI_SLOT=y in RHEL6, so evidently they have this problem.
> 
> oh, I only checked opensuse that has that set to m.
> 
>>
>> Asking users to edit module.conf by hand is not a solution, just like
>> asking users to boot with a command line option is not a solution.
>> That sort of stuff is fine for a hobbyist OS intended only for techie
>> geeks.  It's not fine for Linux.
> 
> not sure. add something in command line or conf files.
> but recompile kernel is another story.
> 
>>
>> If you would give a concrete example of the ACPI namespace info and
>> device config, hotplug sequence, etc., required to show the problem,
>> we could have a useful discussion about ways to fix it.  But if all
>> you have is FUD about "this might break and users won't have the
>> ability to edit modules.conf," that doesn't help me see why this patch
>> is a bad idea.
> 
> Never mind, We should save your bandwidth to more patches.
Hi Yinghai,
	Could we use quirk to auto-disable PCIe native hotplug for
problematic platforms?
Regards!
Gerry

> 
> Yinghai
> 
> .
> 

Re: [RFC PATCH v5 4/8] ACPI, PCI: avoid building pci_slot as module

[Yinghai Lu]

On Mon, Jan 28, 2013 at 6:07 PM, Jiang Liu <jiang.liu@huawei.com> wrote:
>         Could we use quirk to auto-disable PCIe native hotplug for
> problematic platforms?

that is some kind of boot command line way?

Yinghai

Re: [RFC PATCH v5 4/8] ACPI, PCI: avoid building pci_slot as module

[Jiang Liu]

On 2013-1-29 10:21, Yinghai Lu wrote:
> On Mon, Jan 28, 2013 at 6:07 PM, Jiang Liu <jiang.liu@huawei.com> wrote:
>>         Could we use quirk to auto-disable PCIe native hotplug for
>> problematic platforms?
> 
> that is some kind of boot command line way?
We could negotiate between acpiphp and pciehp if those problematic 
platforms/chipsets could be identified by DMI info or PCI device ID.

> 
> Yinghai
> 
> 

Re: [RFC PATCH v5 4/8] ACPI, PCI: avoid building pci_slot as module

[Bjorn Helgaas]

On Mon, Jan 28, 2013 at 7:45 PM, Jiang Liu <jiang.liu@huawei.com> wrote:
> On 2013-1-29 10:21, Yinghai Lu wrote:
>> On Mon, Jan 28, 2013 at 6:07 PM, Jiang Liu <jiang.liu@huawei.com> wrote:
>>>         Could we use quirk to auto-disable PCIe native hotplug for
>>> problematic platforms?
>>
>> that is some kind of boot command line way?
> We could negotiate between acpiphp and pciehp if those problematic
> platforms/chipsets could be identified by DMI info or PCI device ID.

That's a sub-optimal solution because it requires us to identify all
the affected systems and also may require ongoing maintenance in the
future.  If we could get specifics on the issue, we might be able to
design a better solution that works for everybody.

(BTW, this thread is specifically about pci_slot, which I think is a
slightly different issue than the acpiphp/pciehp conflicts.)

Re: [RFC PATCH v5 4/8] ACPI, PCI: avoid building pci_slot as module

[Matthew Garrett]

On Mon, Jan 28, 2013 at 03:14:11PM -0700, Bjorn Helgaas wrote:

> CONFIG_ACPI_PCI_SLOT=y in RHEL6, so evidently they have this problem.

I'm not aware of anyone ever filing any bugs against it, either, though 
Myron would have a better idea. What's the worst case outcome of someone 
ending up with an incorrect slot number?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [RFC PATCH v5 4/8] ACPI, PCI: avoid building pci_slot as module

[Matthew Garrett]

On Tue, Jan 29, 2013 at 10:07:22AM +0800, Jiang Liu wrote:

> 	Could we use quirk to auto-disable PCIe native hotplug for
> problematic platforms?

Do these problematic platforms successfully boot Windows 7?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [RFC PATCH v5 0/8] introduce PCI bus notifier chain to get rid of the ACPI PCI subdriver interfaces

[Jiang Liu]

On 01/29/2013 10:04 AM, Jiang Liu wrote:
> On 2013-1-29 8:34, Rafael J. Wysocki wrote:
>> On Monday, January 28, 2013 01:56:33 PM Bjorn Helgaas wrote:
>>> On Fri, Jan 18, 2013 at 9:07 AM, Jiang Liu <liuj97@gmail.com> wrote:
>>>> This is an RFC patchset to address review comments in thread at:
>>>> https://patchwork.kernel.org/patch/1946851/. The patch just pasts
>>>> compilation. If no objection to the new implementation, I will
>>>> go on to modify acpiphp driver and conduct tests.
>>>>
>>>> The main changes from V4 to V5 includes:
>>>> 1) introduce a dedicated notifier chain for PCI buses
>>>> 2) change pci_slot as built-in driver
>>>> 3) unify the way to create/destroy PCI slots
>>>> 4) introduce a kernel option to disable PCIe native hotplug
>>>>
>>>> TODO:
>>>> 1) change acpiphp as built-in and unify the way to create/destroy ACPI
>>>>    based hotplug slots.
>>>> 2) change other ACPI PCI subdriver in Yinghai's root bridge hotplug series
>>>>    to use the PCI bus notifier chain.
>>>> 3) Remove the ACPI PCI subdriver interface eventaully.
>>>>
>>>> Jiang Liu (8):
>>>>   PCI: make PCI device create/destroy logic symmetric
>>>>   PCI: split registration of PCI bus devices into two stages
>>>>   PCI: add a blocking notifier chain for PCI bus addition/removal
>>>>   ACPI, PCI: avoid building pci_slot as module
>>>>   PCI, ACPI: hook PCI bus notifications to create/destroy PCI slots
>>>>   pci_slot: replace printk(KERN_xxx) with pr_xxx()
>>>>   PCI/PCIe: add "pci=nopciehp" to disable PCIe native hotplug
>>>>   PCI/PCIe: only claim PME from firmware when CONFIG_PCIE_PME is
>>>>     enabled
>>>>
>>>>  Documentation/kernel-parameters.txt |    2 +
>>>>  drivers/acpi/Kconfig                |    5 +-
>>>>  drivers/acpi/internal.h             |    5 +
>>>>  drivers/acpi/pci_root.c             |    8 +-
>>>>  drivers/acpi/pci_slot.c             |  217 ++++++++++-------------------------
>>>>  drivers/acpi/scan.c                 |    1 +
>>>>  drivers/pci/bus.c                   |   26 ++++-
>>>>  drivers/pci/pci.c                   |    2 +
>>>>  drivers/pci/pci.h                   |    1 +
>>>>  drivers/pci/pcie/portdrv_core.c     |    7 +-
>>>>  drivers/pci/pcie/portdrv_pci.c      |    3 +
>>>>  drivers/pci/probe.c                 |    7 +-
>>>>  drivers/pci/remove.c                |   15 +--
>>>>  include/linux/pci.h                 |   21 ++++
>>>>  14 files changed, 142 insertions(+), 178 deletions(-)
>>>
>>> I think the problem we're trying to solve is that we don't initialize
>>> hot-added devices, correctly, e.g., we don't set up AER, we don't
>>> update acpi/pci_slot stuff, we probably don't set up PME etc.  We also
>>> have similar issues like IOMMU init on powerpc.
>>>
>>> Notifier chains seem like an unnecessarily complicated way to deal
>>> with this.  They're great for communicating between modules that stay
>>> at arm's length from each other.  But that's not the case here --
>>> everything is PCI and is quite closely coupled.  I think AER, PME,
>>> slot, etc., should  be initialized directly in pci_device_add() or
>>> somewhere nearby.
>>
>> I agree.
>>
>>> This might sound a bit radical because it implies some fairly
>>> far-reaching changes.  It means this code can't be a module (the only
>>> one that can be built as a module today is pciehp, and I think
>>> everybody agrees that we should make it static as soon as we can
>>> figure out the acpiphp/pciehp issue).  I think it also means the
>>> pcieportdrv concept is of dubious value, since all the services should
>>> be known at build-time and we probably don't need a registration
>>> interface for them.
>>
>> It is of dubious value regardless.  It just adds complexity for no gain.
>> Moreover, these things are in fact not mutually independent.
>>
>> I've had a lot of headaches trying to work around that when I was working
>> on PME support and later on _OSC for root bridges.  Let's just take that
>> stuff away once and for good. :-)
> Hi Bjorn and Rafael,
> 	Thanks for advice. We will go this direction to change those modules
> as built-in.
> Regards!
> Gerry
> 
Hi Bjorn,
	I have done some investigation about how to implement this without
using notifier chain. Due to commit "PCI: Put pci_dev in device tree as early
as possible", a PCI device will be registered to the driver core before creating
the subordinate PCI bus. So we can't reply on the ACPI PCI device glue code
to create/destroy PCI slots or acpiphp hotplug slots. So my current plan is
to introduce two weak functions as below, is it acceptable?

Regards!
Gerry

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index b494066..a5c22e7 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);
 
@@ -1661,6 +1663,14 @@ int __weak pcibios_root_bridge_prepare(struct pci_host_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)
 {
@@ -1715,6 +1725,8 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int b
        if (error)
                goto class_dev_reg_err;
 
+       pcibios_remove_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 fc38c48..3dbdf82 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -52,6 +52,7 @@ void pci_remove_bus(struct pci_bus *bus)
                return;
 
        pci_remove_legacy_files(bus);
+       pcibios_remove_bus(child);
        device_unregister(&bus->dev);
 }
 EXPORT_SYMBOL(pci_remove_bus);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 056d3d6..fd8ba0c 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -380,6 +380,8 @@ void pci_set_host_bridge_release(struct pci_host_bridge *bridge,
                     void *release_data);
 
 int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge);
+void pcibios_add_bus(struct pci_bus *bus);
+void pcibios_remove_bus(struct pci_bus *bus);
 
 /*
  * The first PCI_BRIDGE_RESOURCE_NUM PCI bus resources (those that correspond


>>
>> Thanks,
>> Rafael
>>
>>
> 
> 

Re: [RFC PATCH v5 0/8] introduce PCI bus notifier chain to get rid of the ACPI PCI subdriver interfaces

[Bjorn Helgaas]

On Fri, Feb 1, 2013 at 9:13 AM, Jiang Liu <liuj97@gmail.com> wrote:
> On 01/29/2013 10:04 AM, Jiang Liu wrote:
>> On 2013-1-29 8:34, Rafael J. Wysocki wrote:
>>> On Monday, January 28, 2013 01:56:33 PM Bjorn Helgaas wrote:
>>>> On Fri, Jan 18, 2013 at 9:07 AM, Jiang Liu <liuj97@gmail.com> wrote:
>>>>> This is an RFC patchset to address review comments in thread at:
>>>>> https://patchwork.kernel.org/patch/1946851/. The patch just pasts
>>>>> compilation. If no objection to the new implementation, I will
>>>>> go on to modify acpiphp driver and conduct tests.
>>>>>
>>>>> The main changes from V4 to V5 includes:
>>>>> 1) introduce a dedicated notifier chain for PCI buses
>>>>> 2) change pci_slot as built-in driver
>>>>> 3) unify the way to create/destroy PCI slots
>>>>> 4) introduce a kernel option to disable PCIe native hotplug
>>>>>
>>>>> TODO:
>>>>> 1) change acpiphp as built-in and unify the way to create/destroy ACPI
>>>>>    based hotplug slots.
>>>>> 2) change other ACPI PCI subdriver in Yinghai's root bridge hotplug series
>>>>>    to use the PCI bus notifier chain.
>>>>> 3) Remove the ACPI PCI subdriver interface eventaully.
>>>>>
>>>>> Jiang Liu (8):
>>>>>   PCI: make PCI device create/destroy logic symmetric
>>>>>   PCI: split registration of PCI bus devices into two stages
>>>>>   PCI: add a blocking notifier chain for PCI bus addition/removal
>>>>>   ACPI, PCI: avoid building pci_slot as module
>>>>>   PCI, ACPI: hook PCI bus notifications to create/destroy PCI slots
>>>>>   pci_slot: replace printk(KERN_xxx) with pr_xxx()
>>>>>   PCI/PCIe: add "pci=nopciehp" to disable PCIe native hotplug
>>>>>   PCI/PCIe: only claim PME from firmware when CONFIG_PCIE_PME is
>>>>>     enabled
>>>>>
>>>>>  Documentation/kernel-parameters.txt |    2 +
>>>>>  drivers/acpi/Kconfig                |    5 +-
>>>>>  drivers/acpi/internal.h             |    5 +
>>>>>  drivers/acpi/pci_root.c             |    8 +-
>>>>>  drivers/acpi/pci_slot.c             |  217 ++++++++++-------------------------
>>>>>  drivers/acpi/scan.c                 |    1 +
>>>>>  drivers/pci/bus.c                   |   26 ++++-
>>>>>  drivers/pci/pci.c                   |    2 +
>>>>>  drivers/pci/pci.h                   |    1 +
>>>>>  drivers/pci/pcie/portdrv_core.c     |    7 +-
>>>>>  drivers/pci/pcie/portdrv_pci.c      |    3 +
>>>>>  drivers/pci/probe.c                 |    7 +-
>>>>>  drivers/pci/remove.c                |   15 +--
>>>>>  include/linux/pci.h                 |   21 ++++
>>>>>  14 files changed, 142 insertions(+), 178 deletions(-)
>>>>
>>>> I think the problem we're trying to solve is that we don't initialize
>>>> hot-added devices, correctly, e.g., we don't set up AER, we don't
>>>> update acpi/pci_slot stuff, we probably don't set up PME etc.  We also
>>>> have similar issues like IOMMU init on powerpc.
>>>>
>>>> Notifier chains seem like an unnecessarily complicated way to deal
>>>> with this.  They're great for communicating between modules that stay
>>>> at arm's length from each other.  But that's not the case here --
>>>> everything is PCI and is quite closely coupled.  I think AER, PME,
>>>> slot, etc., should  be initialized directly in pci_device_add() or
>>>> somewhere nearby.
>>>
>>> I agree.
>>>
>>>> This might sound a bit radical because it implies some fairly
>>>> far-reaching changes.  It means this code can't be a module (the only
>>>> one that can be built as a module today is pciehp, and I think
>>>> everybody agrees that we should make it static as soon as we can
>>>> figure out the acpiphp/pciehp issue).  I think it also means the
>>>> pcieportdrv concept is of dubious value, since all the services should
>>>> be known at build-time and we probably don't need a registration
>>>> interface for them.
>>>
>>> It is of dubious value regardless.  It just adds complexity for no gain.
>>> Moreover, these things are in fact not mutually independent.
>>>
>>> I've had a lot of headaches trying to work around that when I was working
>>> on PME support and later on _OSC for root bridges.  Let's just take that
>>> stuff away once and for good. :-)
>> Hi Bjorn and Rafael,
>>       Thanks for advice. We will go this direction to change those modules
>> as built-in.
>> Regards!
>> Gerry
>>
> Hi Bjorn,
>         I have done some investigation about how to implement this without
> using notifier chain. Due to commit "PCI: Put pci_dev in device tree as early
> as possible", a PCI device will be registered to the driver core before creating
> the subordinate PCI bus. So we can't reply on the ACPI PCI device glue code
> to create/destroy PCI slots or acpiphp hotplug slots. So my current plan is
> to introduce two weak functions as below, is it acceptable?

That seems fine to me.  I think you wrote "pcibios_remove_bus(b)"
below in pci_create_root_bus() when you probably meant
"pcibios_add_bus(b)."  But I'm sure you would have found that soon :)

Anyway, I think a directly-called weak function will be much easier to
understand than a notifier-based solution.

Bjorn

> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index b494066..a5c22e7 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);
>
> @@ -1661,6 +1663,14 @@ int __weak pcibios_root_bridge_prepare(struct pci_host_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)
>  {
> @@ -1715,6 +1725,8 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int b
>         if (error)
>                 goto class_dev_reg_err;
>
> +       pcibios_remove_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 fc38c48..3dbdf82 100644
> --- a/drivers/pci/remove.c
> +++ b/drivers/pci/remove.c
> @@ -52,6 +52,7 @@ void pci_remove_bus(struct pci_bus *bus)
>                 return;
>
>         pci_remove_legacy_files(bus);
> +       pcibios_remove_bus(child);
>         device_unregister(&bus->dev);
>  }
>  EXPORT_SYMBOL(pci_remove_bus);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 056d3d6..fd8ba0c 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -380,6 +380,8 @@ void pci_set_host_bridge_release(struct pci_host_bridge *bridge,
>                      void *release_data);
>
>  int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge);
> +void pcibios_add_bus(struct pci_bus *bus);
> +void pcibios_remove_bus(struct pci_bus *bus);
>
>  /*
>   * The first PCI_BRIDGE_RESOURCE_NUM PCI bus resources (those that correspond
>
>
>>>
>>> Thanks,
>>> Rafael
>>>
>>>
>>
>>
>

Re: [RFC PATCH v5 4/8] ACPI, PCI: avoid building pci_slot as module

[Rafael J. Wysocki]

On Monday, January 28, 2013 02:09:22 PM Bjorn Helgaas wrote:
> On Sun, Jan 20, 2013 at 5:01 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Saturday, January 19, 2013 12:07:42 AM Jiang Liu wrote:
> >> As discussed in thread at https://patchwork.kernel.org/patch/1946851/,
> >> there's no value in supporting CONFIG_ACPI_PCI_SLOT=m any more.
> >> So change Kconfig and code to only support building pci_slot as
> >> built-in driver.
> >>
> >> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> >
> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> I think we should eventually get rid of acpi_pci_register_driver() and
> do this initialization directly from acpi_pci_root_add().  But
> removing the module option here is a good first step.
> 
> Rafael, do you want to apply this (and [6/8]) via your tree?  If not,
> I can take it.

Both applied to bleeding-edge.  I've added your ACK to the [6/8] too, hope
that's OK.

Thanks,
Rafael


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

Re: [RFC PATCH v5 4/8] ACPI, PCI: avoid building pci_slot as module

[Bjorn Helgaas]

On Sun, Feb 3, 2013 at 1:18 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Monday, January 28, 2013 02:09:22 PM Bjorn Helgaas wrote:
>> On Sun, Jan 20, 2013 at 5:01 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> > On Saturday, January 19, 2013 12:07:42 AM Jiang Liu wrote:
>> >> As discussed in thread at https://patchwork.kernel.org/patch/1946851/,
>> >> there's no value in supporting CONFIG_ACPI_PCI_SLOT=m any more.
>> >> So change Kconfig and code to only support building pci_slot as
>> >> built-in driver.
>> >>
>> >> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
>> >
>> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>>
>> I think we should eventually get rid of acpi_pci_register_driver() and
>> do this initialization directly from acpi_pci_root_add().  But
>> removing the module option here is a good first step.
>>
>> Rafael, do you want to apply this (and [6/8]) via your tree?  If not,
>> I can take it.
>
> Both applied to bleeding-edge.  I've added your ACK to the [6/8] too, hope
> that's OK.

Yep, that's fine, thanks!

Bjorn

Re: [RFC PATCH v5 4/8] ACPI, PCI: avoid building pci_slot as module

[Myron Stowe]

On Mon, 2013-01-28 at 14:09 -0700, Bjorn Helgaas wrote:
> On Sun, Jan 20, 2013 at 5:01 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Saturday, January 19, 2013 12:07:42 AM Jiang Liu wrote:
> >> As discussed in thread at https://patchwork.kernel.org/patch/1946851/,
> >> there's no value in supporting CONFIG_ACPI_PCI_SLOT=m any more.
> >> So change Kconfig and code to only support building pci_slot as
> >> built-in driver.
> >>
> >> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> >
> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> I think we should eventually get rid of acpi_pci_register_driver() and
> do this initialization directly from acpi_pci_root_add().  But
> removing the module option here is a good first step.

Bjorn, Rafael:

If either of you are interested in that still let me know and I can
re-work any or all of the "PCI/ACPI: Remove "pci_root" sub-driver
support" series - https://lkml.org/lkml/2012/12/7/11

Note [PATCH 13/15] was effectively the same as below and continued on
with initializing directly from acpi_pci_root_add() in [PATCH 14/15].
There may have been worthwhile fixes in some of the earlier content such
as [PATCH 11/15] worth re-considering also.

Thanks,
 Myron

> 
> Rafael, do you want to apply this (and [6/8]) via your tree?  If not,
> I can take it.
> 
> >> ---
> >>  drivers/acpi/Kconfig    |    5 +----
> >>  drivers/acpi/internal.h |    5 +++++
> >>  drivers/acpi/pci_slot.c |   13 +------------
> >>  drivers/acpi/scan.c     |    1 +
> >>  4 files changed, 8 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> >> index 0300bf6..7efd0d0 100644
> >> --- a/drivers/acpi/Kconfig
> >> +++ b/drivers/acpi/Kconfig
> >> @@ -299,7 +299,7 @@ config ACPI_DEBUG_FUNC_TRACE
> >>         is about half of the penalty and is rarely useful.
> >>
> >>  config ACPI_PCI_SLOT
> >> -     tristate "PCI slot detection driver"
> >> +     bool "PCI slot detection driver"
> >>       depends on SYSFS
> >>       default n
> >>       help
> >> @@ -308,9 +308,6 @@ config ACPI_PCI_SLOT
> >>         i.e., segment/bus/device/function tuples, with physical slots in
> >>         the system.  If you are unsure, say N.
> >>
> >> -       To compile this driver as a module, choose M here:
> >> -       the module will be called pci_slot.
> >> -
> >>  config X86_PM_TIMER
> >>       bool "Power Management Timer Support" if EXPERT
> >>       depends on X86
> >> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> >> index e050254..7374cfc 100644
> >> --- a/drivers/acpi/internal.h
> >> +++ b/drivers/acpi/internal.h
> >> @@ -67,6 +67,11 @@ 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);
> >>  int acpi_ec_init(void);
> >>  int acpi_ec_ecdt_probe(void);
> >> diff --git a/drivers/acpi/pci_slot.c b/drivers/acpi/pci_slot.c
> >> index d22585f..a7d7e77 100644
> >> --- a/drivers/acpi/pci_slot.c
> >> +++ b/drivers/acpi/pci_slot.c
> >> @@ -330,19 +330,8 @@ static struct dmi_system_id acpi_pci_slot_dmi_table[] __initdata = {
> >>       {}
> >>  };
> >>
> >> -static int __init
> >> -acpi_pci_slot_init(void)
> >> +void __init acpi_pci_slot_init(void)
> >>  {
> >>       dmi_check_system(acpi_pci_slot_dmi_table);
> >>       acpi_pci_register_driver(&acpi_pci_slot_driver);
> >> -     return 0;
> >>  }
> >> -
> >> -static void __exit
> >> -acpi_pci_slot_exit(void)
> >> -{
> >> -     acpi_pci_unregister_driver(&acpi_pci_slot_driver);
> >> -}
> >> -
> >> -module_init(acpi_pci_slot_init);
> >> -module_exit(acpi_pci_slot_exit);
> >> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> >> index f8a0d0f..cb964ac 100644
> >> --- a/drivers/acpi/scan.c
> >> +++ b/drivers/acpi/scan.c
> >> @@ -1732,6 +1732,7 @@ int __init acpi_scan_init(void)
> >>
> >>       acpi_power_init();
> >>       acpi_pci_root_init();
> >> +     acpi_pci_slot_init();
> >>
> >>       /*
> >>        * Enumerate devices in the ACPI namespace.
> >>
> > --
> > I speak only for myself.
> > Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH v5 4/8] ACPI, PCI: avoid building pci_slot as module

[Rafael J. Wysocki]

On Sunday, February 03, 2013 03:47:15 PM Myron Stowe wrote:
> On Mon, 2013-01-28 at 14:09 -0700, Bjorn Helgaas wrote:
> > On Sun, Jan 20, 2013 at 5:01 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > On Saturday, January 19, 2013 12:07:42 AM Jiang Liu wrote:
> > >> As discussed in thread at https://patchwork.kernel.org/patch/1946851/,
> > >> there's no value in supporting CONFIG_ACPI_PCI_SLOT=m any more.
> > >> So change Kconfig and code to only support building pci_slot as
> > >> built-in driver.
> > >>
> > >> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> > >
> > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > I think we should eventually get rid of acpi_pci_register_driver() and
> > do this initialization directly from acpi_pci_root_add().  But
> > removing the module option here is a good first step.
> 
> Bjorn, Rafael:
> 
> If either of you are interested in that still let me know and I can
> re-work any or all of the "PCI/ACPI: Remove "pci_root" sub-driver
> support" series - https://lkml.org/lkml/2012/12/7/11

I am.  However, I think it's better to wait with the rework until our trees
are merged into the mainline, so that you can base the patchset on a common
tree.

> Note [PATCH 13/15] was effectively the same as below and continued on
> with initializing directly from acpi_pci_root_add() in [PATCH 14/15].
> There may have been worthwhile fixes in some of the earlier content such
> as [PATCH 11/15] worth re-considering also.

Well, as I said.  All of the patches in the series making sense after v3.9-rc1
will be interesting to me certainly.

Thanks,
Rafael


> > Rafael, do you want to apply this (and [6/8]) via your tree?  If not,
> > I can take it.
> > 
> > >> ---
> > >>  drivers/acpi/Kconfig    |    5 +----
> > >>  drivers/acpi/internal.h |    5 +++++
> > >>  drivers/acpi/pci_slot.c |   13 +------------
> > >>  drivers/acpi/scan.c     |    1 +
> > >>  4 files changed, 8 insertions(+), 16 deletions(-)
> > >>
> > >> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> > >> index 0300bf6..7efd0d0 100644
> > >> --- a/drivers/acpi/Kconfig
> > >> +++ b/drivers/acpi/Kconfig
> > >> @@ -299,7 +299,7 @@ config ACPI_DEBUG_FUNC_TRACE
> > >>         is about half of the penalty and is rarely useful.
> > >>
> > >>  config ACPI_PCI_SLOT
> > >> -     tristate "PCI slot detection driver"
> > >> +     bool "PCI slot detection driver"
> > >>       depends on SYSFS
> > >>       default n
> > >>       help
> > >> @@ -308,9 +308,6 @@ config ACPI_PCI_SLOT
> > >>         i.e., segment/bus/device/function tuples, with physical slots in
> > >>         the system.  If you are unsure, say N.
> > >>
> > >> -       To compile this driver as a module, choose M here:
> > >> -       the module will be called pci_slot.
> > >> -
> > >>  config X86_PM_TIMER
> > >>       bool "Power Management Timer Support" if EXPERT
> > >>       depends on X86
> > >> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> > >> index e050254..7374cfc 100644
> > >> --- a/drivers/acpi/internal.h
> > >> +++ b/drivers/acpi/internal.h
> > >> @@ -67,6 +67,11 @@ 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);
> > >>  int acpi_ec_init(void);
> > >>  int acpi_ec_ecdt_probe(void);
> > >> diff --git a/drivers/acpi/pci_slot.c b/drivers/acpi/pci_slot.c
> > >> index d22585f..a7d7e77 100644
> > >> --- a/drivers/acpi/pci_slot.c
> > >> +++ b/drivers/acpi/pci_slot.c
> > >> @@ -330,19 +330,8 @@ static struct dmi_system_id acpi_pci_slot_dmi_table[] __initdata = {
> > >>       {}
> > >>  };
> > >>
> > >> -static int __init
> > >> -acpi_pci_slot_init(void)
> > >> +void __init acpi_pci_slot_init(void)
> > >>  {
> > >>       dmi_check_system(acpi_pci_slot_dmi_table);
> > >>       acpi_pci_register_driver(&acpi_pci_slot_driver);
> > >> -     return 0;
> > >>  }
> > >> -
> > >> -static void __exit
> > >> -acpi_pci_slot_exit(void)
> > >> -{
> > >> -     acpi_pci_unregister_driver(&acpi_pci_slot_driver);
> > >> -}
> > >> -
> > >> -module_init(acpi_pci_slot_init);
> > >> -module_exit(acpi_pci_slot_exit);
> > >> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > >> index f8a0d0f..cb964ac 100644
> > >> --- a/drivers/acpi/scan.c
> > >> +++ b/drivers/acpi/scan.c
> > >> @@ -1732,6 +1732,7 @@ int __init acpi_scan_init(void)
> > >>
> > >>       acpi_power_init();
> > >>       acpi_pci_root_init();
> > >> +     acpi_pci_slot_init();
> > >>
> > >>       /*
> > >>        * Enumerate devices in the ACPI namespace.
> > >>
> > > --
> > > I speak only for myself.
> > > Rafael J. Wysocki, Intel Open Source Technology Center.
> 
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.