| Message ID | 1348733519-24684-4-git-send-email-yinghai@kernel.org |
|---|---|
| State | Accepted |
| Headers | show |
On Thu, Sep 27, 2012 at 2:11 AM, Yinghai Lu <yinghai@kernel.org> wrote: > So could use assign_unassigned_bus_res pci root bus add After your series, acpi_pci_root_start() looks like this: pci_assign_unassigned_bus_resources list_for_each_entry(driver, &acpi_pci_drivers, node) driver->add(root); pci_enable_bridges(root->bus); so apparently it's important that the driver->add() methods be run *before* the bridges are enabled. Why? > Signed-off-by: Yinghai Lu <yinghai@kernel.org> > --- > drivers/pci/probe.c | 1 + > drivers/pci/setup-bus.c | 2 -- > 2 files changed, 1 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 65f62e3..59cf1ba 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -1905,6 +1905,7 @@ unsigned int __ref pci_rescan_bus(struct pci_bus *bus) > > max = pci_scan_child_bus(bus); > pci_assign_unassigned_bus_resources(bus); > + pci_enable_bridges(bus); > pci_bus_add_devices(bus); > > return max; > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c > index 59e6c55..6d3591d 100644 > --- a/drivers/pci/setup-bus.c > +++ b/drivers/pci/setup-bus.c > @@ -1566,6 +1566,4 @@ void pci_assign_unassigned_bus_resources(struct pci_bus *bus) > up_read(&pci_bus_sem); > __pci_bus_assign_resources(bus, &add_list, NULL); > BUG_ON(!list_empty(&add_list)); > - > - pci_enable_bridges(bus); > } > -- > 1.7.7 > -- 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
On Fri, Sep 28, 2012 at 4:46 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > On Thu, Sep 27, 2012 at 2:11 AM, Yinghai Lu <yinghai@kernel.org> wrote: >> So could use assign_unassigned_bus_res pci root bus add > > After your series, acpi_pci_root_start() looks like this: > > pci_assign_unassigned_bus_resources > list_for_each_entry(driver, &acpi_pci_drivers, node) > driver->add(root); > pci_enable_bridges(root->bus); > > so apparently it's important that the driver->add() methods be run > *before* the bridges are enabled. Why? pci_enable_bridge -- 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
On Fri, Sep 28, 2012 at 6:48 PM, Yinghai Lu <yinghai@kernel.org> wrote: > On Fri, Sep 28, 2012 at 4:46 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: >> On Thu, Sep 27, 2012 at 2:11 AM, Yinghai Lu <yinghai@kernel.org> wrote: >>> So could use assign_unassigned_bus_res pci root bus add >> >> After your series, acpi_pci_root_start() looks like this: >> >> pci_assign_unassigned_bus_resources >> list_for_each_entry(driver, &acpi_pci_drivers, node) >> driver->add(root); >> pci_enable_bridges(root->bus); >> >> so apparently it's important that the driver->add() methods be run >> *before* the bridges are enabled. Why? > During rebase, the changelog get lost. pci_enable_bridges ==> pci_enable_device ==> ... ===> pcibios_enable_device ===> pcibios_enable_irq and one of driver in acpi_pci_drivers, is ioapic drivers and it will enable ioapic there. So we need to enable bridges that later. -Yinghai -- 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
On Fri, Sep 28, 2012 at 7:52 PM, Yinghai Lu <yinghai@kernel.org> wrote: > On Fri, Sep 28, 2012 at 6:48 PM, Yinghai Lu <yinghai@kernel.org> wrote: >> On Fri, Sep 28, 2012 at 4:46 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: >>> On Thu, Sep 27, 2012 at 2:11 AM, Yinghai Lu <yinghai@kernel.org> wrote: >>>> So could use assign_unassigned_bus_res pci root bus add >>> >>> After your series, acpi_pci_root_start() looks like this: >>> >>> pci_assign_unassigned_bus_resources >>> list_for_each_entry(driver, &acpi_pci_drivers, node) >>> driver->add(root); >>> pci_enable_bridges(root->bus); >>> >>> so apparently it's important that the driver->add() methods be run >>> *before* the bridges are enabled. Why? >> > > During rebase, the changelog get lost. > > pci_enable_bridges > ==> pci_enable_device > ==> ... > ===> pcibios_enable_device > ===> pcibios_enable_irq > > and one of driver in acpi_pci_drivers, is ioapic drivers and it will > enable ioapic there. > > So we need to enable bridges that later. Is it important that pci_assign_unassigned_bus_resources() be run before the driver->add() methods? Why? Do you have a pointer to this ioapic driver? I don't think it's in the tree yet. I know about drivers/pci/ioapic.c, but it doesn't use acpi_pci_register_driver(). -- 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
On Fri, Sep 28, 2012 at 8:27 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > > Is it important that pci_assign_unassigned_bus_resources() be run > before the driver->add() methods? Why? for ioapic controller that may need kernel to assign base bar. > > Do you have a pointer to this ioapic driver? I don't think it's in > the tree yet. I know about drivers/pci/ioapic.c, but it doesn't use > acpi_pci_register_driver(). it is in my for-x86-irq branch. http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=shortlog;h=refs/heads/for-x86-irq http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=c186df98f84336422088740ef8cedc1313ca2485 -- 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
On Fri, Sep 28, 2012 at 6:52 PM, Yinghai Lu <yinghai@kernel.org> wrote: > On Fri, Sep 28, 2012 at 6:48 PM, Yinghai Lu <yinghai@kernel.org> wrote: >> On Fri, Sep 28, 2012 at 4:46 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: >>> On Thu, Sep 27, 2012 at 2:11 AM, Yinghai Lu <yinghai@kernel.org> wrote: >>>> So could use assign_unassigned_bus_res pci root bus add >>> >>> After your series, acpi_pci_root_start() looks like this: >>> >>> pci_assign_unassigned_bus_resources >>> list_for_each_entry(driver, &acpi_pci_drivers, node) >>> driver->add(root); >>> pci_enable_bridges(root->bus); >>> >>> so apparently it's important that the driver->add() methods be run >>> *before* the bridges are enabled. Why? >> > > During rebase, the changelog get lost. > > pci_enable_bridges > ==> pci_enable_device > ==> ... > ===> pcibios_enable_device > ===> pcibios_enable_irq > > and one of driver in acpi_pci_drivers, is ioapic drivers and it will > enable ioapic there. > > So we need to enable bridges that later. old changelog before rebase: ----- Subject: [PATCH] PCI, x86: Move pci_enable_bridges() down After we get hot-added ioapic registered. pci_enable_bridges will try to enable ioapic irq for pci bridge. So need to move it down. Or We can move out pcibios_enable_irq() out of pci_enable_device() and call pcibios_enable_irq in pci_bus_add_devices ? also will need to move ... pcibios_resource_survey_bus(root->bus); pci_assign_unassigned_bus_resources(root->bus); to the start add .... ---- -- 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
On Fri, Sep 28, 2012 at 10:04 PM, Yinghai Lu <yinghai@kernel.org> wrote: > On Fri, Sep 28, 2012 at 8:27 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: >> >> Is it important that pci_assign_unassigned_bus_resources() be run >> before the driver->add() methods? Why? > > for ioapic controller that may need kernel to assign base bar. It seems like there are two cases of IOAPICs we need to worry about: 1) An IOAPIC device in the ACPI namespace (ACPI0009, ACPI000A, or ACPI000B). This device has a _CRS that contains the device base address. Since it's an ACPI device and has _CRS, I expect it should *not* appear in PCI config space and would have no BARs. 2) An IOAPIC device that appears in PCI config space. This has a BAR that we'll need to assign when hot-adding the device. The spec (ACPI v5.0, sec 9.17) implies that the second type (a bus-enumerated device) will not have an device in the ACPI namespace. However, sec 6.2.6 says we need a _GSB for both types of IOAPICs, and I don't know where the _GSB for bus-enumerated IOAPICs would live if there's no ACPI device for them. Since you're talking about assigning a BAR, I assume you're talking about the second type -- a bus-enumerated IOAPIC. I would also assume that a PCI IOAPIC would appear in the hierarchy *above* any devices that are connected to it, so we'd enumerate it *before* those devices. Maybe that will help ensure that we bind the driver to the IOAPIC before binding drivers to devices that use it. Can you help me understand this? What's the topology of the machine you're working on? E.g., what does this part of the ACPI namespace and PCI config space look like? >> Do you have a pointer to this ioapic driver? I don't think it's in >> the tree yet. I know about drivers/pci/ioapic.c, but it doesn't use >> acpi_pci_register_driver(). > > it is in my for-x86-irq branch. > > http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=shortlog;h=refs/heads/for-x86-irq > > http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=c186df98f84336422088740ef8cedc1313ca2485 -- 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
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 65f62e3..59cf1ba 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1905,6 +1905,7 @@ unsigned int __ref pci_rescan_bus(struct pci_bus *bus) max = pci_scan_child_bus(bus); pci_assign_unassigned_bus_resources(bus); + pci_enable_bridges(bus); pci_bus_add_devices(bus); return max; diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c index 59e6c55..6d3591d 100644 --- a/drivers/pci/setup-bus.c +++ b/drivers/pci/setup-bus.c @@ -1566,6 +1566,4 @@ void pci_assign_unassigned_bus_resources(struct pci_bus *bus) up_read(&pci_bus_sem); __pci_bus_assign_resources(bus, &add_list, NULL); BUG_ON(!list_empty(&add_list)); - - pci_enable_bridges(bus); }
So could use assign_unassigned_bus_res pci root bus add Signed-off-by: Yinghai Lu <yinghai@kernel.org> --- drivers/pci/probe.c | 1 + drivers/pci/setup-bus.c | 2 -- 2 files changed, 1 insertions(+), 2 deletions(-)