On Tue, Feb 26, 2013 at 8:25 AM, Jiang Liu <[email protected]> wrote:
> Previously the acpiphp driver registers itself as an ACPI PCI subdriver,
> so it's callbacks will be invoked when creating/destroying PCI root
> buses to manage ACPI based PCI hotplug slots. But it doesn't handle
> P2P bridge hotplug events, so it will cause strange behavours if there
> are hotplug slots associated with a hot-removed P2P bridge.
>
> So this patch tries to fix this issue by:
> 1) Make acpiphp built-in only, not a module.
> 2) Directly hook into PCI core to update hotplug slot devices when
> creating/destroying PCI buses through:
> pci_{add|remove}_bus() -> acpi_pci_{add|remove}_bus()
> 3) Get rid of ACPI PCI subdriver related code, it's not used any more.
>
> Signed-off-by: Jiang Liu <[email protected]>
> Signed-off-by: Yijing Wang <[email protected]>
> Cc: Yinghai Lu <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Cc: Toshi Kani <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> drivers/pci/hotplug/Kconfig | 7 +-
> drivers/pci/hotplug/acpiphp.h | 3 -
> drivers/pci/hotplug/acpiphp_core.c | 23 +--
> drivers/pci/hotplug/acpiphp_glue.c | 282 +++++++-----------------------------
> drivers/pci/pci-acpi.c | 3 +
> include/linux/pci-acpi.h | 14 +-
> 6 files changed, 67 insertions(+), 265 deletions(-)
>
> diff --git a/drivers/pci/hotplug/Kconfig b/drivers/pci/hotplug/Kconfig
> index 13e9e63..9fcb87f 100644
> --- a/drivers/pci/hotplug/Kconfig
> +++ b/drivers/pci/hotplug/Kconfig
> @@ -52,15 +52,12 @@ config HOTPLUG_PCI_IBM
> When in doubt, say N.
>
> config HOTPLUG_PCI_ACPI
> - tristate "ACPI PCI Hotplug driver"
> - depends on (!ACPI_DOCK && ACPI) || (ACPI_DOCK)
> + bool "ACPI PCI Hotplug driver"
My goal is that a user should never have to specify a kernel boot
parameter or edit a modules.conf file, but the user did previously
have some way to influence whether we use pciehp or acpiphp. I know
we still have some issues, particularly with acpiphp, so I'm a little
concerned that by removing the CONFIG_HOTPLUG_PCI_ACPI=m, we might be
removing a way to work around those issues.
A distro that previously used CONFIG_HOTPLUG_PCI_ACPI=m will now have
to use =y, so modules.conf is no longer applicable. Can you convince
me that the user still has a way to work around issues? I spent quite
a while trying to understand the pciehp/acpiphp dependencies, but it's
pretty tangled web.
I would definitely like to see the changelog mention this issue and
any changes a user might have to make, e.g., to replace a modules.conf
edit. It might be that this involves the "pciehp_force" option, and
if it does, we should probably add a mention of this in
Documentation/kernel-parameters.txt.
Can the Kconfig change be split into its own small patch without all
the other stuff, or are they inseparable?
Bjorn
> + depends on HOTPLUG_PCI=y && ((!ACPI_DOCK && ACPI) || (ACPI_DOCK))
> help
> Say Y here if you have a system that supports PCI Hotplug using
> ACPI.
>
> - To compile this driver as a module, choose M here: the
> - module will be called acpiphp.
> -
> When in doubt, say N.
>
> config HOTPLUG_PCI_ACPI_IBM
> diff --git a/drivers/pci/hotplug/acpiphp.h b/drivers/pci/hotplug/acpiphp.h
> index 1b311f9..69b4aac 100644
> --- a/drivers/pci/hotplug/acpiphp.h
> +++ b/drivers/pci/hotplug/acpiphp.h
> @@ -119,7 +119,6 @@ struct acpiphp_slot {
> */
> struct acpiphp_func {
> struct acpiphp_slot *slot; /* parent */
> - struct acpiphp_bridge *bridge; /* Ejectable PCI-to-PCI bridge */
>
> struct list_head sibling;
> struct notifier_block nb;
> @@ -176,8 +175,6 @@ extern int acpiphp_register_hotplug_slot(struct acpiphp_slot *slot);
> extern void acpiphp_unregister_hotplug_slot(struct acpiphp_slot *slot);
>
> /* acpiphp_glue.c */
> -extern int acpiphp_glue_init (void);
> -extern void acpiphp_glue_exit (void);
> typedef int (*acpiphp_callback)(struct acpiphp_slot *slot, void *data);
>
> extern int acpiphp_enable_slot (struct acpiphp_slot *slot);
> diff --git a/drivers/pci/hotplug/acpiphp_core.c b/drivers/pci/hotplug/acpiphp_core.c
> index c2fd309..80d777c 100644
> --- a/drivers/pci/hotplug/acpiphp_core.c
> +++ b/drivers/pci/hotplug/acpiphp_core.c
> @@ -37,6 +37,7 @@
>
> #include <linux/kernel.h>
> #include <linux/pci.h>
> +#include <linux/pci-acpi.h>
> #include <linux/pci_hotplug.h>
> #include <linux/slab.h>
> #include <linux/smp.h>
> @@ -351,27 +352,7 @@ void acpiphp_unregister_hotplug_slot(struct acpiphp_slot *acpiphp_slot)
> }
>
>
> -static int __init acpiphp_init(void)
> +void __init acpiphp_init(void)
> {
> info(DRIVER_DESC " version: " DRIVER_VERSION "\n");
> -
> - if (acpi_pci_disabled)
> - return 0;
> -
> - /* read all the ACPI info from the system */
> - /* initialize internal data structure etc. */
> - return acpiphp_glue_init();
> -}
> -
> -
> -static void __exit acpiphp_exit(void)
> -{
> - if (acpi_pci_disabled)
> - return;
> -
> - /* deallocate internal data structures etc. */
> - acpiphp_glue_exit();
> }
> -
> -module_init(acpiphp_init);
> -module_exit(acpiphp_exit);
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index 89e0c36..3db84b6 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -157,6 +157,7 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
> int device, function, retval;
> struct pci_bus *pbus = bridge->pci_bus;
> struct pci_dev *pdev;
> + u32 val;
>
> if (!acpi_pci_check_ejectable(pbus, handle) && !is_dock_device(handle))
> return AE_OK;
> @@ -249,11 +250,9 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
> newfunc->slot = slot;
> list_add_tail(&newfunc->sibling, &slot->funcs);
>
> - pdev = pci_get_slot(pbus, PCI_DEVFN(device, function));
> - if (pdev) {
> + if (pci_bus_read_dev_vendor_id(pbus, PCI_DEVFN(device, function),
> + &val, 60*1000))
> slot->flags |= (SLOT_ENABLED | SLOT_POWEREDON);
> - pci_dev_put(pdev);
> - }
>
> if (is_dock_device(handle)) {
> /* we don't want to call this device's _EJ0
> @@ -366,148 +365,6 @@ static struct acpiphp_func *acpiphp_bridge_handle_to_function(acpi_handle handle
> }
>
>
> -static inline void config_p2p_bridge_flags(struct acpiphp_bridge *bridge)
> -{
> - acpi_handle dummy_handle;
> - struct acpiphp_func *func;
> -
> - if (ACPI_SUCCESS(acpi_get_handle(bridge->handle,
> - "_EJ0", &dummy_handle))) {
> - bridge->flags |= BRIDGE_HAS_EJ0;
> -
> - dbg("found ejectable p2p bridge\n");
> -
> - /* make link between PCI bridge and PCI function */
> - func = acpiphp_bridge_handle_to_function(bridge->handle);
> - if (!func)
> - return;
> - bridge->func = func;
> - func->bridge = bridge;
> - }
> -}
> -
> -
> -/* allocate and initialize host bridge data structure */
> -static void add_host_bridge(struct acpi_pci_root *root)
> -{
> - struct acpiphp_bridge *bridge;
> - acpi_handle handle = root->device->handle;
> -
> - bridge = kzalloc(sizeof(struct acpiphp_bridge), GFP_KERNEL);
> - if (bridge == NULL)
> - return;
> -
> - bridge->handle = handle;
> -
> - bridge->pci_bus = root->bus;
> -
> - init_bridge_misc(bridge);
> -}
> -
> -
> -/* allocate and initialize PCI-to-PCI bridge data structure */
> -static void add_p2p_bridge(acpi_handle *handle)
> -{
> - struct acpiphp_bridge *bridge;
> -
> - bridge = kzalloc(sizeof(struct acpiphp_bridge), GFP_KERNEL);
> - if (bridge == NULL) {
> - err("out of memory\n");
> - return;
> - }
> -
> - bridge->handle = handle;
> - config_p2p_bridge_flags(bridge);
> -
> - bridge->pci_dev = acpi_get_pci_dev(handle);
> - bridge->pci_bus = bridge->pci_dev->subordinate;
> - if (!bridge->pci_bus) {
> - err("This is not a PCI-to-PCI bridge!\n");
> - goto err;
> - }
> -
> - /*
> - * Grab a ref to the subordinate PCI bus in case the bus is
> - * removed via PCI core logical hotplug. The ref pins the bus
> - * (which we access during module unload).
> - */
> - get_device(&bridge->pci_bus->dev);
> -
> - init_bridge_misc(bridge);
> - return;
> - err:
> - pci_dev_put(bridge->pci_dev);
> - kfree(bridge);
> - return;
> -}
> -
> -
> -/* callback routine to find P2P bridges */
> -static acpi_status
> -find_p2p_bridge(acpi_handle handle, u32 lvl, void *context, void **rv)
> -{
> - acpi_status status;
> - struct pci_dev *dev;
> -
> - dev = acpi_get_pci_dev(handle);
> - if (!dev || !dev->subordinate)
> - goto out;
> -
> - /* check if this bridge has ejectable slots */
> - if ((detect_ejectable_slots(handle) > 0)) {
> - dbg("found PCI-to-PCI bridge at PCI %s\n", pci_name(dev));
> - add_p2p_bridge(handle);
> - }
> -
> - /* search P2P bridges under this p2p bridge */
> - status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
> - find_p2p_bridge, NULL, NULL, NULL);
> - if (ACPI_FAILURE(status))
> - warn("find_p2p_bridge failed (error code = 0x%x)\n", status);
> -
> - out:
> - pci_dev_put(dev);
> - return AE_OK;
> -}
> -
> -
> -/* find hot-pluggable slots, and then find P2P bridge */
> -static int add_bridge(struct acpi_pci_root *root)
> -{
> - acpi_status status;
> - unsigned long long tmp;
> - acpi_handle dummy_handle;
> - acpi_handle handle = root->device->handle;
> -
> - /* if the bridge doesn't have _STA, we assume it is always there */
> - status = acpi_get_handle(handle, "_STA", &dummy_handle);
> - if (ACPI_SUCCESS(status)) {
> - status = acpi_evaluate_integer(handle, "_STA", NULL, &tmp);
> - if (ACPI_FAILURE(status)) {
> - dbg("%s: _STA evaluation failure\n", __func__);
> - return 0;
> - }
> - if ((tmp & ACPI_STA_DEVICE_FUNCTIONING) == 0)
> - /* don't register this object */
> - return 0;
> - }
> -
> - /* check if this bridge has ejectable slots */
> - if (detect_ejectable_slots(handle) > 0) {
> - dbg("found PCI host-bus bridge with hot-pluggable slots\n");
> - add_host_bridge(root);
> - }
> -
> - /* search P2P bridges under this host bridge */
> - status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
> - find_p2p_bridge, NULL, NULL, NULL);
> -
> - if (ACPI_FAILURE(status))
> - warn("find_p2p_bridge failed (error code = 0x%x)\n", status);
> -
> - return 0;
> -}
> -
> static struct acpiphp_bridge *acpiphp_handle_to_bridge(acpi_handle handle)
> {
> struct acpiphp_bridge *bridge;
> @@ -567,56 +424,12 @@ static void cleanup_bridge(struct acpiphp_bridge *bridge)
> slot = next;
> }
>
> - /*
> - * Only P2P bridges have a pci_dev
> - */
> - if (bridge->pci_dev)
> - put_device(&bridge->pci_bus->dev);
> -
> + put_device(&bridge->pci_bus->dev);
> pci_dev_put(bridge->pci_dev);
> list_del(&bridge->list);
> kfree(bridge);
> }
>
> -static acpi_status
> -cleanup_p2p_bridge(acpi_handle handle, u32 lvl, void *context, void **rv)
> -{
> - struct acpiphp_bridge *bridge;
> -
> - /* cleanup p2p bridges under this P2P bridge
> - in a depth-first manner */
> - acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
> - cleanup_p2p_bridge, NULL, NULL, NULL);
> -
> - bridge = acpiphp_handle_to_bridge(handle);
> - if (bridge)
> - cleanup_bridge(bridge);
> -
> - return AE_OK;
> -}
> -
> -static void remove_bridge(struct acpi_pci_root *root)
> -{
> - struct acpiphp_bridge *bridge;
> - acpi_handle handle = root->device->handle;
> -
> - /* cleanup p2p bridges under this host bridge
> - in a depth-first manner */
> - acpi_walk_namespace(ACPI_TYPE_DEVICE, handle,
> - (u32)1, cleanup_p2p_bridge, NULL, NULL, NULL);
> -
> - /*
> - * On root bridges with hotplug slots directly underneath (ie,
> - * no p2p bridge between), we call cleanup_bridge().
> - *
> - * The else clause cleans up root bridges that either had no
> - * hotplug slots at all, or had a p2p bridge underneath.
> - */
> - bridge = acpiphp_handle_to_bridge(handle);
> - if (bridge)
> - cleanup_bridge(bridge);
> -}
> -
> static int power_on_slot(struct acpiphp_slot *slot)
> {
> acpi_status status;
> @@ -802,6 +615,7 @@ static void check_hotplug_bridge(struct acpiphp_slot *slot, struct pci_dev *dev)
> }
> }
> }
> +
> /**
> * enable_device - enable, configure a slot
> * @slot: slot to be enabled
> @@ -816,7 +630,6 @@ static int __ref enable_device(struct acpiphp_slot *slot)
> struct acpiphp_func *func;
> int retval = 0;
> int num, max, pass;
> - acpi_status status;
>
> if (slot->flags & SLOT_ENABLED)
> goto err_exit;
> @@ -871,18 +684,6 @@ static int __ref enable_device(struct acpiphp_slot *slot)
> slot->flags &= (~SLOT_ENABLED);
> continue;
> }
> -
> - if (dev->hdr_type != PCI_HEADER_TYPE_BRIDGE &&
> - dev->hdr_type != PCI_HEADER_TYPE_CARDBUS) {
> - pci_dev_put(dev);
> - continue;
> - }
> -
> - status = find_p2p_bridge(func->handle, (u32)1, bus, NULL);
> - if (ACPI_FAILURE(status))
> - warn("find_p2p_bridge failed (error code = 0x%x)\n",
> - status);
> - pci_dev_put(dev);
> }
>
>
> @@ -916,16 +717,6 @@ static int disable_device(struct acpiphp_slot *slot)
> {
> struct acpiphp_func *func;
> struct pci_dev *pdev;
> - struct pci_bus *bus = slot->bridge->pci_bus;
> -
> - list_for_each_entry(func, &slot->funcs, sibling) {
> - if (func->bridge) {
> - /* cleanup p2p bridges under this P2P bridge */
> - cleanup_p2p_bridge(func->bridge->handle,
> - (u32)1, NULL, NULL);
> - func->bridge = NULL;
> - }
> - }
>
> /*
> * enable_device() enumerates all functions in this device via
> @@ -944,7 +735,6 @@ static int disable_device(struct acpiphp_slot *slot)
>
> slot->flags &= (~SLOT_ENABLED);
>
> -err_exit:
> return 0;
> }
>
> @@ -1285,30 +1075,56 @@ static void handle_hotplug_event_func(acpi_handle handle, u32 type,
> alloc_acpi_hp_work(handle, type, context, _handle_hotplug_event_func);
> }
>
> -static struct acpi_pci_driver acpi_pci_hp_driver = {
> - .add = add_bridge,
> - .remove = remove_bridge,
> -};
> -
> -/**
> - * acpiphp_glue_init - initializes all PCI hotplug - ACPI glue data structures
> +/*
> + * Create hotplug slots for the PCI bus.
> + * It should always return 0 to avoid skipping following notifiers.
> */
> -int __init acpiphp_glue_init(void)
> +void acpiphp_enumerate_slots(struct pci_bus *bus, acpi_handle handle)
> {
> - acpi_pci_register_driver(&acpi_pci_hp_driver);
> + acpi_handle dummy_handle;
> + struct acpiphp_bridge *bridge;
>
> - return 0;
> -}
> + if (detect_ejectable_slots(handle) <= 0)
> + return;
>
> + bridge = kzalloc(sizeof(struct acpiphp_bridge), GFP_KERNEL);
> + if (bridge == NULL) {
> + err("out of memory\n");
> + return;
> + }
>
> -/**
> - * acpiphp_glue_exit - terminates all PCI hotplug - ACPI glue data structures
> - *
> - * This function frees all data allocated in acpiphp_glue_init().
> - */
> -void acpiphp_glue_exit(void)
> + bridge->handle = handle;
> + bridge->pci_dev = pci_dev_get(bus->self);
> + bridge->pci_bus = bus;
> +
> + /*
> + * Grab a ref to the subordinate PCI bus in case the bus is
> + * removed via PCI core logical hotplug. The ref pins the bus
> + * (which we access during module unload).
> + */
> + get_device(&bus->dev);
> +
> + if (!pci_is_root_bus(bridge->pci_bus) &&
> + ACPI_SUCCESS(acpi_get_handle(bridge->handle,
> + "_EJ0", &dummy_handle))) {
> + dbg("found ejectable p2p bridge\n");
> + bridge->flags |= BRIDGE_HAS_EJ0;
> + bridge->func = acpiphp_bridge_handle_to_function(handle);
> + }
> +
> + init_bridge_misc(bridge);
> +}
> +
> +/* Destroy hotplug slots associated with the PCI bus */
> +void acpiphp_remove_slots(struct pci_bus *bus)
> {
> - acpi_pci_unregister_driver(&acpi_pci_hp_driver);
> + struct acpiphp_bridge *bridge, *tmp;
> +
> + list_for_each_entry_safe(bridge, tmp, &bridge_list, list)
> + if (bridge->pci_bus == bus) {
> + cleanup_bridge(bridge);
> + break;
> + }
> }
>
> /**
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 2dbca38..6fe7bf8 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -297,6 +297,7 @@ void acpi_pci_add_bus(struct pci_bus *bus)
> return;
>
> acpi_pci_slot_enumerate(bus, handle);
> + acpiphp_enumerate_slots(bus, handle);
> }
>
> void acpi_pci_remove_bus(struct pci_bus *bus)
> @@ -308,6 +309,7 @@ void acpi_pci_remove_bus(struct pci_bus *bus)
> if (acpi_pci_disabled)
> return;
>
> + acpiphp_remove_slots(bus);
> acpi_pci_slot_remove(bus);
> }
>
> @@ -384,6 +386,7 @@ static int __init acpi_pci_init(void)
>
> pci_set_platform_pm(&acpi_pci_platform_pm);
> acpi_pci_slot_init();
> + acpiphp_init();
>
> return 0;
> }
> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> index 1ef126f..f1b2380 100644
> --- a/include/linux/pci-acpi.h
> +++ b/include/linux/pci-acpi.h
> @@ -56,9 +56,17 @@ static inline void acpi_pci_slot_enumerate(struct pci_bus *bus,
> static inline void acpi_pci_slot_remove(struct pci_bus *bus) { }
> #endif
>
> -#else /* CONFIG_ACPI */
> -static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
> -static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
> +#ifdef CONFIG_HOTPLUG_PCI_ACPI
> +void acpiphp_init(void);
> +void acpiphp_enumerate_slots(struct pci_bus *bus, acpi_handle handle);
> +void acpiphp_remove_slots(struct pci_bus *bus);
> +#else
> +static inline void acpiphp_init(void) { }
> +static inline void acpiphp_enumerate_slots(struct pci_bus *bus,
> + acpi_handle handle) { }
> +static inline void acpiphp_remove_slots(struct pci_bus *bus) { }
> +#endif
> +
> #endif /* CONFIG_ACPI */
>
> #ifdef CONFIG_ACPI_APEI
> --
> 1.7.9.5
>
On 04/10/2013 07:38 AM, Bjorn Helgaas wrote:
> On Tue, Feb 26, 2013 at 8:25 AM, Jiang Liu <[email protected]> wrote:
>> Previously the acpiphp driver registers itself as an ACPI PCI subdriver,
>> so it's callbacks will be invoked when creating/destroying PCI root
>> buses to manage ACPI based PCI hotplug slots. But it doesn't handle
>> P2P bridge hotplug events, so it will cause strange behavours if there
>> are hotplug slots associated with a hot-removed P2P bridge.
>>
>> So this patch tries to fix this issue by:
>> 1) Make acpiphp built-in only, not a module.
>> 2) Directly hook into PCI core to update hotplug slot devices when
>> creating/destroying PCI buses through:
>> pci_{add|remove}_bus() -> acpi_pci_{add|remove}_bus()
>> 3) Get rid of ACPI PCI subdriver related code, it's not used any more.
>>
>> Signed-off-by: Jiang Liu <[email protected]>
>> Signed-off-by: Yijing Wang <[email protected]>
>> Cc: Yinghai Lu <[email protected]>
>> Cc: "Rafael J. Wysocki" <[email protected]>
>> Cc: Toshi Kani <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> ---
>> drivers/pci/hotplug/Kconfig | 7 +-
>> drivers/pci/hotplug/acpiphp.h | 3 -
>> drivers/pci/hotplug/acpiphp_core.c | 23 +--
>> drivers/pci/hotplug/acpiphp_glue.c | 282 +++++++-----------------------------
>> drivers/pci/pci-acpi.c | 3 +
>> include/linux/pci-acpi.h | 14 +-
>> 6 files changed, 67 insertions(+), 265 deletions(-)
>>
>> diff --git a/drivers/pci/hotplug/Kconfig b/drivers/pci/hotplug/Kconfig
>> index 13e9e63..9fcb87f 100644
>> --- a/drivers/pci/hotplug/Kconfig
>> +++ b/drivers/pci/hotplug/Kconfig
>> @@ -52,15 +52,12 @@ config HOTPLUG_PCI_IBM
>> When in doubt, say N.
>>
>> config HOTPLUG_PCI_ACPI
>> - tristate "ACPI PCI Hotplug driver"
>> - depends on (!ACPI_DOCK && ACPI) || (ACPI_DOCK)
>> + bool "ACPI PCI Hotplug driver"
>
Hi Bjorn,
Thanks for review.
> My goal is that a user should never have to specify a kernel boot
> parameter or edit a modules.conf file, but the user did previously
> have some way to influence whether we use pciehp or acpiphp. I know
> we still have some issues, particularly with acpiphp, so I'm a little
> concerned that by removing the CONFIG_HOTPLUG_PCI_ACPI=m, we might be
> removing a way to work around those issues.
>
> A distro that previously used CONFIG_HOTPLUG_PCI_ACPI=m will now have
> to use =y, so modules.conf is no longer applicable. Can you convince
> me that the user still has a way to work around issues? I spent quite
> a while trying to understand the pciehp/acpiphp dependencies, but it's
> pretty tangled web.
I will try my best to explain the relationships between pciehp and acpiphp
as of v3.9-rc6.
The pciehp driver always have priority over the acpiphp driver.
That is, the acpiphp driver rejects binding to an ACPI PCI hotplug slot if
a) The slot's parent is a PCIe port with native hotplug capability
b) OSPM has taken over PCIe native hotplug control from BIOS.
!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL)
The above check has no dependency on the loading order of pciehp and acpiphp
drivers. So converting acpiphp driver to builit-in should be ok.
On the other hand, I remember Yinghai has mentioned that some PCIe ports
with native hotplug capability doesn't work as expected with the pciehp driver
and should be managed by the acpiphp driver. Currently we could achieve that
by using boot param "pcie_ports=compat", but this will disable PCIe port
drivers altogether. And I also remember that Rafael has mentioned that
some BIOSes exhibit strange dependency among PCIe OSC controls, so it's
not feasible to only disable PCIe native hotplug.
For "pciehp_force", it does only affect the way pciehp to detect a hotplug
slot, it doesn't affect acpiphp at all.
To sum up, converting acpiphp as built-in should not affect the relationship
between pciehp and acpiphp driver. So how about splitting this patch into
two and adding more comments for the Kconfig change?
Regards!
Gerry
>
> I would definitely like to see the changelog mention this issue and
> any changes a user might have to make, e.g., to replace a modules.conf
> edit. It might be that this involves the "pciehp_force" option, and
> if it does, we should probably add a mention of this in
> Documentation/kernel-parameters.txt.
>
> Can the Kconfig change be split into its own small patch without all
> the other stuff, or are they inseparable?
>
> Bjorn
>
>> + depends on HOTPLUG_PCI=y && ((!ACPI_DOCK && ACPI) || (ACPI_DOCK))
>> help
>> Say Y here if you have a system that supports PCI Hotplug using
>> ACPI.
>>
>> - To compile this driver as a module, choose M here: the
>> - module will be called acpiphp.
>> -
>> When in doubt, say N.
>>
>> config HOTPLUG_PCI_ACPI_IBM
>> diff --git a/drivers/pci/hotplug/acpiphp.h b/drivers/pci/hotplug/acpiphp.h
>> index 1b311f9..69b4aac 100644
>> --- a/drivers/pci/hotplug/acpiphp.h
>> +++ b/drivers/pci/hotplug/acpiphp.h
>> @@ -119,7 +119,6 @@ struct acpiphp_slot {
>> */
>> struct acpiphp_func {
>> struct acpiphp_slot *slot; /* parent */
>> - struct acpiphp_bridge *bridge; /* Ejectable PCI-to-PCI bridge */
>>
>> struct list_head sibling;
>> struct notifier_block nb;
>> @@ -176,8 +175,6 @@ extern int acpiphp_register_hotplug_slot(struct acpiphp_slot *slot);
>> extern void acpiphp_unregister_hotplug_slot(struct acpiphp_slot *slot);
>>
>> /* acpiphp_glue.c */
>> -extern int acpiphp_glue_init (void);
>> -extern void acpiphp_glue_exit (void);
>> typedef int (*acpiphp_callback)(struct acpiphp_slot *slot, void *data);
>>
>> extern int acpiphp_enable_slot (struct acpiphp_slot *slot);
>> diff --git a/drivers/pci/hotplug/acpiphp_core.c b/drivers/pci/hotplug/acpiphp_core.c
>> index c2fd309..80d777c 100644
>> --- a/drivers/pci/hotplug/acpiphp_core.c
>> +++ b/drivers/pci/hotplug/acpiphp_core.c
>> @@ -37,6 +37,7 @@
>>
>> #include <linux/kernel.h>
>> #include <linux/pci.h>
>> +#include <linux/pci-acpi.h>
>> #include <linux/pci_hotplug.h>
>> #include <linux/slab.h>
>> #include <linux/smp.h>
>> @@ -351,27 +352,7 @@ void acpiphp_unregister_hotplug_slot(struct acpiphp_slot *acpiphp_slot)
>> }
>>
>>
>> -static int __init acpiphp_init(void)
>> +void __init acpiphp_init(void)
>> {
>> info(DRIVER_DESC " version: " DRIVER_VERSION "\n");
>> -
>> - if (acpi_pci_disabled)
>> - return 0;
>> -
>> - /* read all the ACPI info from the system */
>> - /* initialize internal data structure etc. */
>> - return acpiphp_glue_init();
>> -}
>> -
>> -
>> -static void __exit acpiphp_exit(void)
>> -{
>> - if (acpi_pci_disabled)
>> - return;
>> -
>> - /* deallocate internal data structures etc. */
>> - acpiphp_glue_exit();
>> }
>> -
>> -module_init(acpiphp_init);
>> -module_exit(acpiphp_exit);
>> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
>> index 89e0c36..3db84b6 100644
>> --- a/drivers/pci/hotplug/acpiphp_glue.c
>> +++ b/drivers/pci/hotplug/acpiphp_glue.c
>> @@ -157,6 +157,7 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
>> int device, function, retval;
>> struct pci_bus *pbus = bridge->pci_bus;
>> struct pci_dev *pdev;
>> + u32 val;
>>
>> if (!acpi_pci_check_ejectable(pbus, handle) && !is_dock_device(handle))
>> return AE_OK;
>> @@ -249,11 +250,9 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
>> newfunc->slot = slot;
>> list_add_tail(&newfunc->sibling, &slot->funcs);
>>
>> - pdev = pci_get_slot(pbus, PCI_DEVFN(device, function));
>> - if (pdev) {
>> + if (pci_bus_read_dev_vendor_id(pbus, PCI_DEVFN(device, function),
>> + &val, 60*1000))
>> slot->flags |= (SLOT_ENABLED | SLOT_POWEREDON);
>> - pci_dev_put(pdev);
>> - }
>>
>> if (is_dock_device(handle)) {
>> /* we don't want to call this device's _EJ0
>> @@ -366,148 +365,6 @@ static struct acpiphp_func *acpiphp_bridge_handle_to_function(acpi_handle handle
>> }
>>
>>
>> -static inline void config_p2p_bridge_flags(struct acpiphp_bridge *bridge)
>> -{
>> - acpi_handle dummy_handle;
>> - struct acpiphp_func *func;
>> -
>> - if (ACPI_SUCCESS(acpi_get_handle(bridge->handle,
>> - "_EJ0", &dummy_handle))) {
>> - bridge->flags |= BRIDGE_HAS_EJ0;
>> -
>> - dbg("found ejectable p2p bridge\n");
>> -
>> - /* make link between PCI bridge and PCI function */
>> - func = acpiphp_bridge_handle_to_function(bridge->handle);
>> - if (!func)
>> - return;
>> - bridge->func = func;
>> - func->bridge = bridge;
>> - }
>> -}
>> -
>> -
>> -/* allocate and initialize host bridge data structure */
>> -static void add_host_bridge(struct acpi_pci_root *root)
>> -{
>> - struct acpiphp_bridge *bridge;
>> - acpi_handle handle = root->device->handle;
>> -
>> - bridge = kzalloc(sizeof(struct acpiphp_bridge), GFP_KERNEL);
>> - if (bridge == NULL)
>> - return;
>> -
>> - bridge->handle = handle;
>> -
>> - bridge->pci_bus = root->bus;
>> -
>> - init_bridge_misc(bridge);
>> -}
>> -
>> -
>> -/* allocate and initialize PCI-to-PCI bridge data structure */
>> -static void add_p2p_bridge(acpi_handle *handle)
>> -{
>> - struct acpiphp_bridge *bridge;
>> -
>> - bridge = kzalloc(sizeof(struct acpiphp_bridge), GFP_KERNEL);
>> - if (bridge == NULL) {
>> - err("out of memory\n");
>> - return;
>> - }
>> -
>> - bridge->handle = handle;
>> - config_p2p_bridge_flags(bridge);
>> -
>> - bridge->pci_dev = acpi_get_pci_dev(handle);
>> - bridge->pci_bus = bridge->pci_dev->subordinate;
>> - if (!bridge->pci_bus) {
>> - err("This is not a PCI-to-PCI bridge!\n");
>> - goto err;
>> - }
>> -
>> - /*
>> - * Grab a ref to the subordinate PCI bus in case the bus is
>> - * removed via PCI core logical hotplug. The ref pins the bus
>> - * (which we access during module unload).
>> - */
>> - get_device(&bridge->pci_bus->dev);
>> -
>> - init_bridge_misc(bridge);
>> - return;
>> - err:
>> - pci_dev_put(bridge->pci_dev);
>> - kfree(bridge);
>> - return;
>> -}
>> -
>> -
>> -/* callback routine to find P2P bridges */
>> -static acpi_status
>> -find_p2p_bridge(acpi_handle handle, u32 lvl, void *context, void **rv)
>> -{
>> - acpi_status status;
>> - struct pci_dev *dev;
>> -
>> - dev = acpi_get_pci_dev(handle);
>> - if (!dev || !dev->subordinate)
>> - goto out;
>> -
>> - /* check if this bridge has ejectable slots */
>> - if ((detect_ejectable_slots(handle) > 0)) {
>> - dbg("found PCI-to-PCI bridge at PCI %s\n", pci_name(dev));
>> - add_p2p_bridge(handle);
>> - }
>> -
>> - /* search P2P bridges under this p2p bridge */
>> - status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
>> - find_p2p_bridge, NULL, NULL, NULL);
>> - if (ACPI_FAILURE(status))
>> - warn("find_p2p_bridge failed (error code = 0x%x)\n", status);
>> -
>> - out:
>> - pci_dev_put(dev);
>> - return AE_OK;
>> -}
>> -
>> -
>> -/* find hot-pluggable slots, and then find P2P bridge */
>> -static int add_bridge(struct acpi_pci_root *root)
>> -{
>> - acpi_status status;
>> - unsigned long long tmp;
>> - acpi_handle dummy_handle;
>> - acpi_handle handle = root->device->handle;
>> -
>> - /* if the bridge doesn't have _STA, we assume it is always there */
>> - status = acpi_get_handle(handle, "_STA", &dummy_handle);
>> - if (ACPI_SUCCESS(status)) {
>> - status = acpi_evaluate_integer(handle, "_STA", NULL, &tmp);
>> - if (ACPI_FAILURE(status)) {
>> - dbg("%s: _STA evaluation failure\n", __func__);
>> - return 0;
>> - }
>> - if ((tmp & ACPI_STA_DEVICE_FUNCTIONING) == 0)
>> - /* don't register this object */
>> - return 0;
>> - }
>> -
>> - /* check if this bridge has ejectable slots */
>> - if (detect_ejectable_slots(handle) > 0) {
>> - dbg("found PCI host-bus bridge with hot-pluggable slots\n");
>> - add_host_bridge(root);
>> - }
>> -
>> - /* search P2P bridges under this host bridge */
>> - status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
>> - find_p2p_bridge, NULL, NULL, NULL);
>> -
>> - if (ACPI_FAILURE(status))
>> - warn("find_p2p_bridge failed (error code = 0x%x)\n", status);
>> -
>> - return 0;
>> -}
>> -
>> static struct acpiphp_bridge *acpiphp_handle_to_bridge(acpi_handle handle)
>> {
>> struct acpiphp_bridge *bridge;
>> @@ -567,56 +424,12 @@ static void cleanup_bridge(struct acpiphp_bridge *bridge)
>> slot = next;
>> }
>>
>> - /*
>> - * Only P2P bridges have a pci_dev
>> - */
>> - if (bridge->pci_dev)
>> - put_device(&bridge->pci_bus->dev);
>> -
>> + put_device(&bridge->pci_bus->dev);
>> pci_dev_put(bridge->pci_dev);
>> list_del(&bridge->list);
>> kfree(bridge);
>> }
>>
>> -static acpi_status
>> -cleanup_p2p_bridge(acpi_handle handle, u32 lvl, void *context, void **rv)
>> -{
>> - struct acpiphp_bridge *bridge;
>> -
>> - /* cleanup p2p bridges under this P2P bridge
>> - in a depth-first manner */
>> - acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
>> - cleanup_p2p_bridge, NULL, NULL, NULL);
>> -
>> - bridge = acpiphp_handle_to_bridge(handle);
>> - if (bridge)
>> - cleanup_bridge(bridge);
>> -
>> - return AE_OK;
>> -}
>> -
>> -static void remove_bridge(struct acpi_pci_root *root)
>> -{
>> - struct acpiphp_bridge *bridge;
>> - acpi_handle handle = root->device->handle;
>> -
>> - /* cleanup p2p bridges under this host bridge
>> - in a depth-first manner */
>> - acpi_walk_namespace(ACPI_TYPE_DEVICE, handle,
>> - (u32)1, cleanup_p2p_bridge, NULL, NULL, NULL);
>> -
>> - /*
>> - * On root bridges with hotplug slots directly underneath (ie,
>> - * no p2p bridge between), we call cleanup_bridge().
>> - *
>> - * The else clause cleans up root bridges that either had no
>> - * hotplug slots at all, or had a p2p bridge underneath.
>> - */
>> - bridge = acpiphp_handle_to_bridge(handle);
>> - if (bridge)
>> - cleanup_bridge(bridge);
>> -}
>> -
>> static int power_on_slot(struct acpiphp_slot *slot)
>> {
>> acpi_status status;
>> @@ -802,6 +615,7 @@ static void check_hotplug_bridge(struct acpiphp_slot *slot, struct pci_dev *dev)
>> }
>> }
>> }
>> +
>> /**
>> * enable_device - enable, configure a slot
>> * @slot: slot to be enabled
>> @@ -816,7 +630,6 @@ static int __ref enable_device(struct acpiphp_slot *slot)
>> struct acpiphp_func *func;
>> int retval = 0;
>> int num, max, pass;
>> - acpi_status status;
>>
>> if (slot->flags & SLOT_ENABLED)
>> goto err_exit;
>> @@ -871,18 +684,6 @@ static int __ref enable_device(struct acpiphp_slot *slot)
>> slot->flags &= (~SLOT_ENABLED);
>> continue;
>> }
>> -
>> - if (dev->hdr_type != PCI_HEADER_TYPE_BRIDGE &&
>> - dev->hdr_type != PCI_HEADER_TYPE_CARDBUS) {
>> - pci_dev_put(dev);
>> - continue;
>> - }
>> -
>> - status = find_p2p_bridge(func->handle, (u32)1, bus, NULL);
>> - if (ACPI_FAILURE(status))
>> - warn("find_p2p_bridge failed (error code = 0x%x)\n",
>> - status);
>> - pci_dev_put(dev);
>> }
>>
>>
>> @@ -916,16 +717,6 @@ static int disable_device(struct acpiphp_slot *slot)
>> {
>> struct acpiphp_func *func;
>> struct pci_dev *pdev;
>> - struct pci_bus *bus = slot->bridge->pci_bus;
>> -
>> - list_for_each_entry(func, &slot->funcs, sibling) {
>> - if (func->bridge) {
>> - /* cleanup p2p bridges under this P2P bridge */
>> - cleanup_p2p_bridge(func->bridge->handle,
>> - (u32)1, NULL, NULL);
>> - func->bridge = NULL;
>> - }
>> - }
>>
>> /*
>> * enable_device() enumerates all functions in this device via
>> @@ -944,7 +735,6 @@ static int disable_device(struct acpiphp_slot *slot)
>>
>> slot->flags &= (~SLOT_ENABLED);
>>
>> -err_exit:
>> return 0;
>> }
>>
>> @@ -1285,30 +1075,56 @@ static void handle_hotplug_event_func(acpi_handle handle, u32 type,
>> alloc_acpi_hp_work(handle, type, context, _handle_hotplug_event_func);
>> }
>>
>> -static struct acpi_pci_driver acpi_pci_hp_driver = {
>> - .add = add_bridge,
>> - .remove = remove_bridge,
>> -};
>> -
>> -/**
>> - * acpiphp_glue_init - initializes all PCI hotplug - ACPI glue data structures
>> +/*
>> + * Create hotplug slots for the PCI bus.
>> + * It should always return 0 to avoid skipping following notifiers.
>> */
>> -int __init acpiphp_glue_init(void)
>> +void acpiphp_enumerate_slots(struct pci_bus *bus, acpi_handle handle)
>> {
>> - acpi_pci_register_driver(&acpi_pci_hp_driver);
>> + acpi_handle dummy_handle;
>> + struct acpiphp_bridge *bridge;
>>
>> - return 0;
>> -}
>> + if (detect_ejectable_slots(handle) <= 0)
>> + return;
>>
>> + bridge = kzalloc(sizeof(struct acpiphp_bridge), GFP_KERNEL);
>> + if (bridge == NULL) {
>> + err("out of memory\n");
>> + return;
>> + }
>>
>> -/**
>> - * acpiphp_glue_exit - terminates all PCI hotplug - ACPI glue data structures
>> - *
>> - * This function frees all data allocated in acpiphp_glue_init().
>> - */
>> -void acpiphp_glue_exit(void)
>> + bridge->handle = handle;
>> + bridge->pci_dev = pci_dev_get(bus->self);
>> + bridge->pci_bus = bus;
>> +
>> + /*
>> + * Grab a ref to the subordinate PCI bus in case the bus is
>> + * removed via PCI core logical hotplug. The ref pins the bus
>> + * (which we access during module unload).
>> + */
>> + get_device(&bus->dev);
>> +
>> + if (!pci_is_root_bus(bridge->pci_bus) &&
>> + ACPI_SUCCESS(acpi_get_handle(bridge->handle,
>> + "_EJ0", &dummy_handle))) {
>> + dbg("found ejectable p2p bridge\n");
>> + bridge->flags |= BRIDGE_HAS_EJ0;
>> + bridge->func = acpiphp_bridge_handle_to_function(handle);
>> + }
>> +
>> + init_bridge_misc(bridge);
>> +}
>> +
>> +/* Destroy hotplug slots associated with the PCI bus */
>> +void acpiphp_remove_slots(struct pci_bus *bus)
>> {
>> - acpi_pci_unregister_driver(&acpi_pci_hp_driver);
>> + struct acpiphp_bridge *bridge, *tmp;
>> +
>> + list_for_each_entry_safe(bridge, tmp, &bridge_list, list)
>> + if (bridge->pci_bus == bus) {
>> + cleanup_bridge(bridge);
>> + break;
>> + }
>> }
>>
>> /**
>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
>> index 2dbca38..6fe7bf8 100644
>> --- a/drivers/pci/pci-acpi.c
>> +++ b/drivers/pci/pci-acpi.c
>> @@ -297,6 +297,7 @@ void acpi_pci_add_bus(struct pci_bus *bus)
>> return;
>>
>> acpi_pci_slot_enumerate(bus, handle);
>> + acpiphp_enumerate_slots(bus, handle);
>> }
>>
>> void acpi_pci_remove_bus(struct pci_bus *bus)
>> @@ -308,6 +309,7 @@ void acpi_pci_remove_bus(struct pci_bus *bus)
>> if (acpi_pci_disabled)
>> return;
>>
>> + acpiphp_remove_slots(bus);
>> acpi_pci_slot_remove(bus);
>> }
>>
>> @@ -384,6 +386,7 @@ static int __init acpi_pci_init(void)
>>
>> pci_set_platform_pm(&acpi_pci_platform_pm);
>> acpi_pci_slot_init();
>> + acpiphp_init();
>>
>> return 0;
>> }
>> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
>> index 1ef126f..f1b2380 100644
>> --- a/include/linux/pci-acpi.h
>> +++ b/include/linux/pci-acpi.h
>> @@ -56,9 +56,17 @@ static inline void acpi_pci_slot_enumerate(struct pci_bus *bus,
>> static inline void acpi_pci_slot_remove(struct pci_bus *bus) { }
>> #endif
>>
>> -#else /* CONFIG_ACPI */
>> -static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
>> -static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
>> +#ifdef CONFIG_HOTPLUG_PCI_ACPI
>> +void acpiphp_init(void);
>> +void acpiphp_enumerate_slots(struct pci_bus *bus, acpi_handle handle);
>> +void acpiphp_remove_slots(struct pci_bus *bus);
>> +#else
>> +static inline void acpiphp_init(void) { }
>> +static inline void acpiphp_enumerate_slots(struct pci_bus *bus,
>> + acpi_handle handle) { }
>> +static inline void acpiphp_remove_slots(struct pci_bus *bus) { }
>> +#endif
>> +
>> #endif /* CONFIG_ACPI */
>>
>> #ifdef CONFIG_ACPI_APEI
>> --
>> 1.7.9.5
>>
On Wed, Apr 10, 2013 at 10:14 AM, Jiang Liu <[email protected]> wrote:
> On 04/10/2013 07:38 AM, Bjorn Helgaas wrote:
>> On Tue, Feb 26, 2013 at 8:25 AM, Jiang Liu <[email protected]> wrote:
>>> Previously the acpiphp driver registers itself as an ACPI PCI subdriver,
>>> so it's callbacks will be invoked when creating/destroying PCI root
>>> buses to manage ACPI based PCI hotplug slots. But it doesn't handle
>>> P2P bridge hotplug events, so it will cause strange behavours if there
>>> are hotplug slots associated with a hot-removed P2P bridge.
>>>
>>> So this patch tries to fix this issue by:
>>> 1) Make acpiphp built-in only, not a module.
>>> 2) Directly hook into PCI core to update hotplug slot devices when
>>> creating/destroying PCI buses through:
>>> pci_{add|remove}_bus() -> acpi_pci_{add|remove}_bus()
>>> 3) Get rid of ACPI PCI subdriver related code, it's not used any more.
>>>
>>> Signed-off-by: Jiang Liu <[email protected]>
>>> Signed-off-by: Yijing Wang <[email protected]>
>>> Cc: Yinghai Lu <[email protected]>
>>> Cc: "Rafael J. Wysocki" <[email protected]>
>>> Cc: Toshi Kani <[email protected]>
>>> Cc: [email protected]
>>> Cc: [email protected]
>>> ---
>>> drivers/pci/hotplug/Kconfig | 7 +-
>>> drivers/pci/hotplug/acpiphp.h | 3 -
>>> drivers/pci/hotplug/acpiphp_core.c | 23 +--
>>> drivers/pci/hotplug/acpiphp_glue.c | 282 +++++++-----------------------------
>>> drivers/pci/pci-acpi.c | 3 +
>>> include/linux/pci-acpi.h | 14 +-
>>> 6 files changed, 67 insertions(+), 265 deletions(-)
>>>
>>> diff --git a/drivers/pci/hotplug/Kconfig b/drivers/pci/hotplug/Kconfig
>>> index 13e9e63..9fcb87f 100644
>>> --- a/drivers/pci/hotplug/Kconfig
>>> +++ b/drivers/pci/hotplug/Kconfig
>>> @@ -52,15 +52,12 @@ config HOTPLUG_PCI_IBM
>>> When in doubt, say N.
>>>
>>> config HOTPLUG_PCI_ACPI
>>> - tristate "ACPI PCI Hotplug driver"
>>> - depends on (!ACPI_DOCK && ACPI) || (ACPI_DOCK)
>>> + bool "ACPI PCI Hotplug driver"
>>
> Hi Bjorn,
> Thanks for review.
>
>> My goal is that a user should never have to specify a kernel boot
>> parameter or edit a modules.conf file, but the user did previously
>> have some way to influence whether we use pciehp or acpiphp. I know
>> we still have some issues, particularly with acpiphp, so I'm a little
>> concerned that by removing the CONFIG_HOTPLUG_PCI_ACPI=m, we might be
>> removing a way to work around those issues.
>>
>> A distro that previously used CONFIG_HOTPLUG_PCI_ACPI=m will now have
>> to use =y, so modules.conf is no longer applicable. Can you convince
>> me that the user still has a way to work around issues? I spent quite
>> a while trying to understand the pciehp/acpiphp dependencies, but it's
>> pretty tangled web.
> I will try my best to explain the relationships between pciehp and acpiphp
> as of v3.9-rc6.
>
> The pciehp driver always have priority over the acpiphp driver.
> That is, the acpiphp driver rejects binding to an ACPI PCI hotplug slot if
> a) The slot's parent is a PCIe port with native hotplug capability
> b) OSPM has taken over PCIe native hotplug control from BIOS.
> !(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL)
> The above check has no dependency on the loading order of pciehp and acpiphp
> drivers. So converting acpiphp driver to builit-in should be ok.
>
> On the other hand, I remember Yinghai has mentioned that some PCIe ports
> with native hotplug capability doesn't work as expected with the pciehp driver
> and should be managed by the acpiphp driver. Currently we could achieve that
> by using boot param "pcie_ports=compat", but this will disable PCIe port
> drivers altogether. And I also remember that Rafael has mentioned that
> some BIOSes exhibit strange dependency among PCIe OSC controls, so it's
> not feasible to only disable PCIe native hotplug.
>
> For "pciehp_force", it does only affect the way pciehp to detect a hotplug
> slot, it doesn't affect acpiphp at all.
>
> To sum up, converting acpiphp as built-in should not affect the relationship
> between pciehp and acpiphp driver.
My concern is that a user used to be able to remove acpiphp from
modules.conf. Now removing acpiphp will require a kernel rebuild.
But maybe that won't turn out to be a problem.
> So how about splitting this patch into
> two and adding more comments for the Kconfig change?
Yes, please at least split this into two. While you're at it, please
also split the first patch into "remove unnecessary is_added guard"
and "cleanup."
Bjorn
>> Hi Bjorn,
>> Thanks for review.
>>
>>> My goal is that a user should never have to specify a kernel boot
>>> parameter or edit a modules.conf file, but the user did previously
>>> have some way to influence whether we use pciehp or acpiphp. I know
>>> we still have some issues, particularly with acpiphp, so I'm a little
>>> concerned that by removing the CONFIG_HOTPLUG_PCI_ACPI=m, we might be
>>> removing a way to work around those issues.
>>>
>>> A distro that previously used CONFIG_HOTPLUG_PCI_ACPI=m will now have
>>> to use =y, so modules.conf is no longer applicable. Can you convince
>>> me that the user still has a way to work around issues? I spent quite
>>> a while trying to understand the pciehp/acpiphp dependencies, but it's
>>> pretty tangled web.
>> I will try my best to explain the relationships between pciehp and acpiphp
>> as of v3.9-rc6.
>>
>> The pciehp driver always have priority over the acpiphp driver.
>> That is, the acpiphp driver rejects binding to an ACPI PCI hotplug slot if
>> a) The slot's parent is a PCIe port with native hotplug capability
>> b) OSPM has taken over PCIe native hotplug control from BIOS.
>> !(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL)
>> The above check has no dependency on the loading order of pciehp and acpiphp
>> drivers. So converting acpiphp driver to builit-in should be ok.
>>
>> On the other hand, I remember Yinghai has mentioned that some PCIe ports
>> with native hotplug capability doesn't work as expected with the pciehp driver
>> and should be managed by the acpiphp driver. Currently we could achieve that
>> by using boot param "pcie_ports=compat", but this will disable PCIe port
>> drivers altogether. And I also remember that Rafael has mentioned that
>> some BIOSes exhibit strange dependency among PCIe OSC controls, so it's
>> not feasible to only disable PCIe native hotplug.
>>
>> For "pciehp_force", it does only affect the way pciehp to detect a hotplug
>> slot, it doesn't affect acpiphp at all.
>>
>> To sum up, converting acpiphp as built-in should not affect the relationship
>> between pciehp and acpiphp driver.
>
> My concern is that a user used to be able to remove acpiphp from
> modules.conf. Now removing acpiphp will require a kernel rebuild.
> But maybe that won't turn out to be a problem.
Hi Bjorn,
If user don't want to occupy the slot by acpiphp. Conservative approach, what about add a kernel parameter
to control acpiphp to enumerate slot ?
Thanks!
Yijing
>
>> So how about splitting this patch into
>> two and adding more comments for the Kconfig change?
>
> Yes, please at least split this into two. While you're at it, please
> also split the first patch into "remove unnecessary is_added guard"
> and "cleanup."
>
> Bjorn
>
> .
>
--
Thanks!
Yijing
On Wed, Apr 10, 2013 at 7:50 PM, Yijing Wang <[email protected]> wrote:
>>> Hi Bjorn,
>>> Thanks for review.
>>>
>>>> My goal is that a user should never have to specify a kernel boot
>>>> parameter or edit a modules.conf file, but the user did previously
>>>> have some way to influence whether we use pciehp or acpiphp. I know
>>>> we still have some issues, particularly with acpiphp, so I'm a little
>>>> concerned that by removing the CONFIG_HOTPLUG_PCI_ACPI=m, we might be
>>>> removing a way to work around those issues.
>>>>
>>>> A distro that previously used CONFIG_HOTPLUG_PCI_ACPI=m will now have
>>>> to use =y, so modules.conf is no longer applicable. Can you convince
>>>> me that the user still has a way to work around issues? I spent quite
>>>> a while trying to understand the pciehp/acpiphp dependencies, but it's
>>>> pretty tangled web.
>>> I will try my best to explain the relationships between pciehp and acpiphp
>>> as of v3.9-rc6.
>>>
>>> The pciehp driver always have priority over the acpiphp driver.
>>> That is, the acpiphp driver rejects binding to an ACPI PCI hotplug slot if
>>> a) The slot's parent is a PCIe port with native hotplug capability
>>> b) OSPM has taken over PCIe native hotplug control from BIOS.
>>> !(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL)
>>> The above check has no dependency on the loading order of pciehp and acpiphp
>>> drivers. So converting acpiphp driver to builit-in should be ok.
>>>
>>> On the other hand, I remember Yinghai has mentioned that some PCIe ports
>>> with native hotplug capability doesn't work as expected with the pciehp driver
>>> and should be managed by the acpiphp driver. Currently we could achieve that
>>> by using boot param "pcie_ports=compat", but this will disable PCIe port
>>> drivers altogether. And I also remember that Rafael has mentioned that
>>> some BIOSes exhibit strange dependency among PCIe OSC controls, so it's
>>> not feasible to only disable PCIe native hotplug.
>>>
>>> For "pciehp_force", it does only affect the way pciehp to detect a hotplug
>>> slot, it doesn't affect acpiphp at all.
>>>
>>> To sum up, converting acpiphp as built-in should not affect the relationship
>>> between pciehp and acpiphp driver.
>>
>> My concern is that a user used to be able to remove acpiphp from
>> modules.conf. Now removing acpiphp will require a kernel rebuild.
>> But maybe that won't turn out to be a problem.
>
> Hi Bjorn,
> If user don't want to occupy the slot by acpiphp. Conservative approach, what about add a kernel parameter
> to control acpiphp to enumerate slot ?
Yes, that's what I'm thinking. A "Please report a bug if you have to
use this parameter" chicken switch. Hopefully nobody will need to use
it, but if somebody *does* need it, it's a lot better than having to
tell him to rebuild the kernel.
Bjorn
On 2013/4/12 1:29, Bjorn Helgaas wrote:
> On Wed, Apr 10, 2013 at 7:50 PM, Yijing Wang <[email protected]> wrote:
>>>> Hi Bjorn,
>>>> Thanks for review.
>>>>
>>>>> My goal is that a user should never have to specify a kernel boot
>>>>> parameter or edit a modules.conf file, but the user did previously
>>>>> have some way to influence whether we use pciehp or acpiphp. I know
>>>>> we still have some issues, particularly with acpiphp, so I'm a little
>>>>> concerned that by removing the CONFIG_HOTPLUG_PCI_ACPI=m, we might be
>>>>> removing a way to work around those issues.
>>>>>
>>>>> A distro that previously used CONFIG_HOTPLUG_PCI_ACPI=m will now have
>>>>> to use =y, so modules.conf is no longer applicable. Can you convince
>>>>> me that the user still has a way to work around issues? I spent quite
>>>>> a while trying to understand the pciehp/acpiphp dependencies, but it's
>>>>> pretty tangled web.
>>>> I will try my best to explain the relationships between pciehp and acpiphp
>>>> as of v3.9-rc6.
>>>>
>>>> The pciehp driver always have priority over the acpiphp driver.
>>>> That is, the acpiphp driver rejects binding to an ACPI PCI hotplug slot if
>>>> a) The slot's parent is a PCIe port with native hotplug capability
>>>> b) OSPM has taken over PCIe native hotplug control from BIOS.
>>>> !(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL)
>>>> The above check has no dependency on the loading order of pciehp and acpiphp
>>>> drivers. So converting acpiphp driver to builit-in should be ok.
>>>>
>>>> On the other hand, I remember Yinghai has mentioned that some PCIe ports
>>>> with native hotplug capability doesn't work as expected with the pciehp driver
>>>> and should be managed by the acpiphp driver. Currently we could achieve that
>>>> by using boot param "pcie_ports=compat", but this will disable PCIe port
>>>> drivers altogether. And I also remember that Rafael has mentioned that
>>>> some BIOSes exhibit strange dependency among PCIe OSC controls, so it's
>>>> not feasible to only disable PCIe native hotplug.
>>>>
>>>> For "pciehp_force", it does only affect the way pciehp to detect a hotplug
>>>> slot, it doesn't affect acpiphp at all.
>>>>
>>>> To sum up, converting acpiphp as built-in should not affect the relationship
>>>> between pciehp and acpiphp driver.
>>>
>>> My concern is that a user used to be able to remove acpiphp from
>>> modules.conf. Now removing acpiphp will require a kernel rebuild.
>>> But maybe that won't turn out to be a problem.
>>
>> Hi Bjorn,
>> If user don't want to occupy the slot by acpiphp. Conservative approach, what about add a kernel parameter
>> to control acpiphp to enumerate slot ?
>
> Yes, that's what I'm thinking. A "Please report a bug if you have to
> use this parameter" chicken switch. Hopefully nobody will need to use
> it, but if somebody *does* need it, it's a lot better than having to
> tell him to rebuild the kernel.
Yes, I agree. We will add this patch to the patchset and resend soon.
Thanks!
Yijing.
>
> Bjorn
>
> .
>
--
Thanks!
Yijing