Patchwork [3/8] PCI: Move out pci_enable_bridges out of assign_unsigned_bus_res

login
register
mail settings
Submitter Yinghai Lu
Date Sept. 27, 2012, 8:11 a.m.
Message ID <1348733519-24684-4-git-send-email-yinghai@kernel.org>
Download mbox | patch
Permalink /patch/187306/
State Accepted
Headers show

Comments

Yinghai Lu - Sept. 27, 2012, 8:11 a.m.
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(-)
Bjorn Helgaas - Sept. 28, 2012, 11:46 p.m.
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
Yinghai Lu - Sept. 29, 2012, 1:48 a.m.
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
Yinghai Lu - Sept. 29, 2012, 1:52 a.m.
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
Bjorn Helgaas - Sept. 29, 2012, 3:27 a.m.
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
Yinghai Lu - Sept. 29, 2012, 4:04 a.m.
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
Yinghai Lu - Sept. 29, 2012, 4:13 a.m.
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
Bjorn Helgaas - Oct. 1, 2012, 6:01 p.m.
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

Patch

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);
 }