Re: [PATCH 01/15] PCI, acpiphp: Separate out hot-add support of pci host bridge
On Fri, Dec 7, 2012 at 1:36 PM, Bjorn Helgaas wrote: > On Fri, Dec 7, 2012 at 2:30 PM, Yinghai Lu wrote: > > Sorry I missed that; I had only looked at a few patches adjacent to > the one that split out this code. > > I still would like the code to be all in pci_root.c. ok, I will rebase that to put them in pci_root.c and send them out after you take [PATCH 0/8] PCI, ACPI, x86: Reserve fw allocated resource for hot-add root bus Yinghai -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/15] PCI, acpiphp: Separate out hot-add support of pci host bridge
On Fri, Dec 7, 2012 at 2:30 PM, Yinghai Lu wrote: > On Fri, Dec 7, 2012 at 11:32 AM, Bjorn Helgaas wrote: >> On Thu, Dec 6, 2012 at 11:25 PM, Myron Stowe wrote: >>> From: Yinghai Lu >>> >>> >> For example, as soon as you put this code in pci_root.c instead of >> pci_root_hp.c, it becomes obvious that we're keeping a list of host >> bridges in both places, and we probably don't need two lists. And it >> seems dubious that acpi_pci_root_hp_init() is an initcall that walks >> the namespace looking for host bridges, when acpi_pci_root_add() >> already exists and is called for every host bridge. > > removed the duplicated interface in following patch. > > http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=shortlog;h=refs/heads/for-pci-split-pci-root-hp-2 > > http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=a1d1ef3f5e4932bc672d96dedd11c5d58f7f20f5 > > | [PATCH] PCI, ACPI: remove acpi_root_bridge in pci_root_hp > | > | Tang noticed that hotplug through container will not update acpi_root_bridge > | list. > | > | After closely checking, we don't need that for struct for tracking and > | could use acpi_pci_root directly. Sorry I missed that; I had only looked at a few patches adjacent to the one that split out this code. I still would like the code to be all in pci_root.c. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/15] PCI, acpiphp: Separate out hot-add support of pci host bridge
On Fri, Dec 7, 2012 at 11:32 AM, Bjorn Helgaas wrote: > On Thu, Dec 6, 2012 at 11:25 PM, Myron Stowe wrote: >> From: Yinghai Lu >> >> > For example, as soon as you put this code in pci_root.c instead of > pci_root_hp.c, it becomes obvious that we're keeping a list of host > bridges in both places, and we probably don't need two lists. And it > seems dubious that acpi_pci_root_hp_init() is an initcall that walks > the namespace looking for host bridges, when acpi_pci_root_add() > already exists and is called for every host bridge. removed the duplicated interface in following patch. http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=shortlog;h=refs/heads/for-pci-split-pci-root-hp-2 http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=a1d1ef3f5e4932bc672d96dedd11c5d58f7f20f5 | [PATCH] PCI, ACPI: remove acpi_root_bridge in pci_root_hp | | Tang noticed that hotplug through container will not update acpi_root_bridge | list. | | After closely checking, we don't need that for struct for tracking and | could use acpi_pci_root directly. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/15] PCI, acpiphp: Separate out hot-add support of pci host bridge
On Friday, December 07, 2012 12:32:46 PM Bjorn Helgaas wrote: > On Thu, Dec 6, 2012 at 11:25 PM, Myron Stowe wrote: > > From: Yinghai Lu > > > > It causes confusion. > > I completely agree that acpiphp causes confusion :) > > > We may only need acpi hp for pci host bridge. > > > > Split host bridge hot-add support to pci_root_hp, and keep acpiphp simple. > > > > -v2: put back pci_root_hp change in one patch > > -v3: add pcibios_resource_survey_bus() calling > > -v4: remove not needed code with remove_bridge > > -v5: put back support for acpiphp support for slots just on root bus. > > -v6: change some functions to *_p2p_* to make it more clean. > > -v7: split hot_added change out. > > > > Signed-off-by: Yinghai Lu > > Signed-off-by: Myron Stowe > > --- > > > > drivers/acpi/Makefile |1 > > drivers/acpi/pci_root_hp.c | 228 > > > > drivers/pci/hotplug/acpiphp_glue.c | 59 ++--- > > 3 files changed, 244 insertions(+), 44 deletions(-) > > create mode 100644 drivers/acpi/pci_root_hp.c > > > > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile > > index 82422fe..4edfe41 100644 > > --- a/drivers/acpi/Makefile > > +++ b/drivers/acpi/Makefile > > @@ -36,6 +36,7 @@ acpi-y+= processor_core.o > > acpi-y += ec.o > > acpi-$(CONFIG_ACPI_DOCK) += dock.o > > acpi-y += pci_root.o pci_link.o pci_irq.o > > pci_bind.o > > +acpi-$(CONFIG_HOTPLUG) += pci_root_hp.o > > I absolutely, 110% agree with splitting out the host bridge hotplug > code from the PCI device hotplug code. > > But I don't want to put this in a separate file; I think it should go > directly in pci_root.c. Putting it in a separate file gives the > illusion that hotplug is something we only do in unusual > circumstances. But that's wrong -- even at boot-time we should be > using the same hot-plug flow as we do later. > > Plus, I want people to be forced to look at the ugliness of this code > every time they look at pci_root.c. Maybe that will help get it > cleaned up eventually. > > For example, as soon as you put this code in pci_root.c instead of > pci_root_hp.c, it becomes obvious that we're keeping a list of host > bridges in both places, and we probably don't need two lists. And it > seems dubious that acpi_pci_root_hp_init() is an initcall that walks > the namespace looking for host bridges, when acpi_pci_root_add() > already exists and is called for every host bridge. Agreed. Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/15] PCI, acpiphp: Separate out hot-add support of pci host bridge
On Thu, Dec 6, 2012 at 11:25 PM, Myron Stowe wrote:
> From: Yinghai Lu
>
> It causes confusion.
I completely agree that acpiphp causes confusion :)
> We may only need acpi hp for pci host bridge.
>
> Split host bridge hot-add support to pci_root_hp, and keep acpiphp simple.
>
> -v2: put back pci_root_hp change in one patch
> -v3: add pcibios_resource_survey_bus() calling
> -v4: remove not needed code with remove_bridge
> -v5: put back support for acpiphp support for slots just on root bus.
> -v6: change some functions to *_p2p_* to make it more clean.
> -v7: split hot_added change out.
>
> Signed-off-by: Yinghai Lu
> Signed-off-by: Myron Stowe
> ---
>
> drivers/acpi/Makefile |1
> drivers/acpi/pci_root_hp.c | 228
>
> drivers/pci/hotplug/acpiphp_glue.c | 59 ++---
> 3 files changed, 244 insertions(+), 44 deletions(-)
> create mode 100644 drivers/acpi/pci_root_hp.c
>
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index 82422fe..4edfe41 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -36,6 +36,7 @@ acpi-y+= processor_core.o
> acpi-y += ec.o
> acpi-$(CONFIG_ACPI_DOCK) += dock.o
> acpi-y += pci_root.o pci_link.o pci_irq.o pci_bind.o
> +acpi-$(CONFIG_HOTPLUG) += pci_root_hp.o
I absolutely, 110% agree with splitting out the host bridge hotplug
code from the PCI device hotplug code.
But I don't want to put this in a separate file; I think it should go
directly in pci_root.c. Putting it in a separate file gives the
illusion that hotplug is something we only do in unusual
circumstances. But that's wrong -- even at boot-time we should be
using the same hot-plug flow as we do later.
Plus, I want people to be forced to look at the ugliness of this code
every time they look at pci_root.c. Maybe that will help get it
cleaned up eventually.
For example, as soon as you put this code in pci_root.c instead of
pci_root_hp.c, it becomes obvious that we're keeping a list of host
bridges in both places, and we probably don't need two lists. And it
seems dubious that acpi_pci_root_hp_init() is an initcall that walks
the namespace looking for host bridges, when acpi_pci_root_add()
already exists and is called for every host bridge.
Bjorn
> acpi-y += power.o
> acpi-y += event.o
> acpi-y += sysfs.o
> diff --git a/drivers/acpi/pci_root_hp.c b/drivers/acpi/pci_root_hp.c
> new file mode 100644
> index 000..10c12aa
> --- /dev/null
> +++ b/drivers/acpi/pci_root_hp.c
> @@ -0,0 +1,228 @@
> +/*
> + * Separated from drivers/pci/hotplug/acpiphp_glue.c
> + * only support root bridge
> + */
> +
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +static LIST_HEAD(acpi_root_bridge_list);
> +struct acpi_root_bridge {
> + struct list_head list;
> + acpi_handle handle;
> + u32 flags;
> +};
> +
> +/* bridge flags */
> +#define ROOT_BRIDGE_HAS_EJ0(0x0002)
> +#define ROOT_BRIDGE_HAS_PS3(0x0080)
> +
> +#define ACPI_STA_FUNCTIONING (0x0008)
> +
> +static struct acpi_root_bridge *acpi_root_handle_to_bridge(acpi_handle
> handle)
> +{
> + struct acpi_root_bridge *bridge;
> +
> + list_for_each_entry(bridge, &acpi_root_bridge_list, list)
> + if (bridge->handle == handle)
> + return bridge;
> +
> + return NULL;
> +}
> +
> +/* allocate and initialize host bridge data structure */
> +static void add_acpi_root_bridge(acpi_handle handle)
> +{
> + struct acpi_root_bridge *bridge;
> + acpi_handle dummy_handle;
> + acpi_status status;
> +
> + /* 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)) {
> + unsigned long long tmp;
> +
> + status = acpi_evaluate_integer(handle, "_STA", NULL, &tmp);
> + if (ACPI_FAILURE(status)) {
> + printk(KERN_DEBUG "%s: _STA evaluation failure\n",
> +__func__);
> + return;
> + }
> + if ((tmp & ACPI_STA_FUNCTIONING) == 0)
> + /* don't register this object */
> + return;
> + }
> +
> + bridge = kzalloc(sizeof(struct acpi_root_bridge), GFP_KERNEL);
> + if (!bridge)
> + return;
> +
> + bridge->handle = handle;
> +
> + if (ACPI_SUCCESS(acpi_get_handle(handle, "_EJ0", &dummy_handle)))
> + bridge->flags |= ROOT_BRIDGE_HAS_EJ0;
> + if (ACPI_SUCCESS(acpi_get_handle(handle, "_PS3", &dummy_handle)))
> + bridge->flags |= ROOT_BRIDGE_HAS_PS3;
> +
> + list_add(&bridge->list, &acpi_ro
[PATCH 01/15] PCI, acpiphp: Separate out hot-add support of pci host bridge
From: Yinghai Lu
It causes confusion.
We may only need acpi hp for pci host bridge.
Split host bridge hot-add support to pci_root_hp, and keep acpiphp simple.
-v2: put back pci_root_hp change in one patch
-v3: add pcibios_resource_survey_bus() calling
-v4: remove not needed code with remove_bridge
-v5: put back support for acpiphp support for slots just on root bus.
-v6: change some functions to *_p2p_* to make it more clean.
-v7: split hot_added change out.
Signed-off-by: Yinghai Lu
Signed-off-by: Myron Stowe
---
drivers/acpi/Makefile |1
drivers/acpi/pci_root_hp.c | 228
drivers/pci/hotplug/acpiphp_glue.c | 59 ++---
3 files changed, 244 insertions(+), 44 deletions(-)
create mode 100644 drivers/acpi/pci_root_hp.c
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 82422fe..4edfe41 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -36,6 +36,7 @@ acpi-y+= processor_core.o
acpi-y += ec.o
acpi-$(CONFIG_ACPI_DOCK) += dock.o
acpi-y += pci_root.o pci_link.o pci_irq.o pci_bind.o
+acpi-$(CONFIG_HOTPLUG) += pci_root_hp.o
acpi-y += power.o
acpi-y += event.o
acpi-y += sysfs.o
diff --git a/drivers/acpi/pci_root_hp.c b/drivers/acpi/pci_root_hp.c
new file mode 100644
index 000..10c12aa
--- /dev/null
+++ b/drivers/acpi/pci_root_hp.c
@@ -0,0 +1,228 @@
+/*
+ * Separated from drivers/pci/hotplug/acpiphp_glue.c
+ * only support root bridge
+ */
+
+#include
+#include
+
+#include
+#include
+#include
+#include
+#include
+
+static LIST_HEAD(acpi_root_bridge_list);
+struct acpi_root_bridge {
+ struct list_head list;
+ acpi_handle handle;
+ u32 flags;
+};
+
+/* bridge flags */
+#define ROOT_BRIDGE_HAS_EJ0(0x0002)
+#define ROOT_BRIDGE_HAS_PS3(0x0080)
+
+#define ACPI_STA_FUNCTIONING (0x0008)
+
+static struct acpi_root_bridge *acpi_root_handle_to_bridge(acpi_handle handle)
+{
+ struct acpi_root_bridge *bridge;
+
+ list_for_each_entry(bridge, &acpi_root_bridge_list, list)
+ if (bridge->handle == handle)
+ return bridge;
+
+ return NULL;
+}
+
+/* allocate and initialize host bridge data structure */
+static void add_acpi_root_bridge(acpi_handle handle)
+{
+ struct acpi_root_bridge *bridge;
+ acpi_handle dummy_handle;
+ acpi_status status;
+
+ /* 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)) {
+ unsigned long long tmp;
+
+ status = acpi_evaluate_integer(handle, "_STA", NULL, &tmp);
+ if (ACPI_FAILURE(status)) {
+ printk(KERN_DEBUG "%s: _STA evaluation failure\n",
+__func__);
+ return;
+ }
+ if ((tmp & ACPI_STA_FUNCTIONING) == 0)
+ /* don't register this object */
+ return;
+ }
+
+ bridge = kzalloc(sizeof(struct acpi_root_bridge), GFP_KERNEL);
+ if (!bridge)
+ return;
+
+ bridge->handle = handle;
+
+ if (ACPI_SUCCESS(acpi_get_handle(handle, "_EJ0", &dummy_handle)))
+ bridge->flags |= ROOT_BRIDGE_HAS_EJ0;
+ if (ACPI_SUCCESS(acpi_get_handle(handle, "_PS3", &dummy_handle)))
+ bridge->flags |= ROOT_BRIDGE_HAS_PS3;
+
+ list_add(&bridge->list, &acpi_root_bridge_list);
+}
+
+struct acpi_root_hp_work {
+ struct work_struct work;
+ acpi_handle handle;
+ u32 type;
+ void *context;
+};
+
+static void alloc_acpi_root_hp_work(acpi_handle handle, u32 type,
+ void *context,
+ void (*func)(struct work_struct *work))
+{
+ struct acpi_root_hp_work *hp_work;
+ int ret;
+
+ hp_work = kmalloc(sizeof(*hp_work), GFP_KERNEL);
+ if (!hp_work)
+ return;
+
+ hp_work->handle = handle;
+ hp_work->type = type;
+ hp_work->context = context;
+
+ INIT_WORK(&hp_work->work, func);
+ ret = queue_work(kacpi_hotplug_wq, &hp_work->work);
+ if (!ret)
+ kfree(hp_work);
+}
+
+static void handle_root_bridge_insertion(acpi_handle handle)
+{
+ struct acpi_device *device, *pdevice;
+ acpi_handle phandle;
+ int ret_val;
+
+ acpi_get_parent(handle, &phandle);
+ if (acpi_bus_get_device(phandle, &pdevice)) {
+ printk(KERN_DEBUG "no parent device, assuming NULL\n");
+ pdevice = NULL;
+ }
+ if (!acpi_bus_get_device(handle, &device)) {
+ /* check if pci root_bus is removed */
+ struct acpi_pci_root
