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

From: Bjorn Helgaas
Date: Tue Apr 09 2013 - 19:38:50 EST

Next message: akpm: "mmotm 2013-04-09-16-37 uploaded"
Previous message: Cody P Schafer: "[PATCH v2 02/10] mm/page_alloc: prevent concurrent updaters of pcp ->batch and ->high"
Next in thread: Jiang Liu: "Re: [PATCH v8 10/13] PCI/acpiphp: do not use ACPI PCI subdriver mechanism"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, Feb 26, 2013 at 8:25 AM, Jiang Liu <liuj97@xxxxxxxxx> wrote:
> Previously the acpiphp driver registers itself as an ACPI PCI subdriver,
> so it's callbacks will be invoked when creating/destroying PCI root
> buses to manage ACPI based PCI hotplug slots. But it doesn't handle
> P2P bridge hotplug events, so it will cause strange behavours if there
> are hotplug slots associated with a hot-removed P2P bridge.
>
> So this patch tries to fix this issue by:
> 1) Make acpiphp built-in only, not a module.
> 2) Directly hook into PCI core to update hotplug slot devices when
> creating/destroying PCI buses through:
> pci_{add|remove}_bus() -> acpi_pci_{add|remove}_bus()
> 3) Get rid of ACPI PCI subdriver related code, it's not used any more.
>
> Signed-off-by: Jiang Liu <jiang.liu@xxxxxxxxxx>
> Signed-off-by: Yijing Wang <wangyijing@xxxxxxxxxx>
> Cc: Yinghai Lu <yinghai@xxxxxxxxxx>
> Cc: "Rafael J. Wysocki" <rafael.j.wysocki@xxxxxxxxx>
> Cc: Toshi Kani <toshi.kani@xxxxxx>
> Cc: linux-pci@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> ---
> 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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: akpm: "mmotm 2013-04-09-16-37 uploaded"
Previous message: Cody P Schafer: "[PATCH v2 02/10] mm/page_alloc: prevent concurrent updaters of pcp ->batch and ->high"
Next in thread: Jiang Liu: "Re: [PATCH v8 10/13] PCI/acpiphp: do not use ACPI PCI subdriver mechanism"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]