On Tue, Sep 15, 2015 at 08:25:59PM +0100, Bjorn Helgaas wrote:
[...]
> commit d5da9d99d4d79d877815af96fdbfac829c4ce7b2
> Author: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> Date: Tue Sep 15 13:18:04 2015 -0500
>
> PCI: Revert "PCI: Call pci_read_bridge_bases() from core instead of arch code"
>
> Revert dff22d2054b5 ("PCI: Call pci_read_bridge_bases() from core instead
> of arch code").
>
> Reading PCI bridge windows is not arch-specific in itself, but there is PCI
> core code that doesn't work correctly if we read them too early. For
> example, Hannes found this case on an ARM Freescale i.mx6 board:
>
> pci_bus 0000:00: root bus resource [mem 0x01000000-0x01efffff]
> pci 0000:00:00.0: PCI bridge to [bus 01-ff]
> pci 0000:00:00.0: BAR 8: no space for [mem size 0x01000000] (mem window)
> pci 0000:01:00.0: BAR 2: failed to assign [mem size 0x00200000]
> pci 0000:01:00.0: BAR 1: failed to assign [mem size 0x00004000]
> pci 0000:01:00.0: BAR 0: failed to assign [mem size 0x00000100]
>
> The 00:00.0 mem window needs to be at least 3MB: the 01:00.0 device needs
> 0x204100 of space, and mem windows are megabyte-aligned.
>
> Bus sizing can increase a bridge window size, but never *decrease* it (see
> d65245c3297a ("PCI: don't shrink bridge resources")). Prior to
> dff22d2054b5, ARM didn't read bridge windows at all, so the "original size"
> was zero, and we assigned a 3MB window.
>
> After dff22d2054b5, we read the bridge windows before sizing the bus. The
> firmware programmed a 16MB window (size 0x01000000) in 00:00.0, and since
> we never decrease the size, we kept 16MB even though we only needed 3MB.
> But 16MB doesn't fit in the host bridge aperture, so we failed to assign
> space for the window and the downstream devices.
>
> I think this is a defect in the PCI core: we shouldn't rely on the firmware
> to assign sensible windows.
I share the same opinion, and I think I will resubmit the same patch we
are reverting again soon, when bridge sizing is sorted :)
Towards that goal, do you think there is a way for us to put together a
branch with all code consolidating resource validation in core PCI code
so that we can test it in all required HW ? What's the best way to do that in
your opinion ? I can set it up for arm and arm64 on my side.
> Ray reported a similar problem, also on ARM, with Broadcom iProc.
>
> Issues like this are too hard to fix right now, so revert dff22d2054b5.
>
> Reported-by: Hannes <oe5hpm@xxxxxxxxx>
> Reported-by: Ray Jui <rjui@xxxxxxxxxxxx>
> Link: http://lkml.kernel.org/r/CAAa04yFQEUJm7Jj1qMT57-LG7ZGtnhNDBe=PpSRa70Mj+XhW-A@xxxxxxxxxxxxxx
> Link: http://lkml.kernel.org/r/55F75BB8.4070405@xxxxxxxxxxxx
> Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
> diff --git a/arch/alpha/kernel/pci.c b/arch/alpha/kernel/pci.c
> index cded02c..5f387ee 100644
> --- a/arch/alpha/kernel/pci.c
> +++ b/arch/alpha/kernel/pci.c
> @@ -242,7 +242,12 @@ pci_restore_srm_config(void)
>
> void pcibios_fixup_bus(struct pci_bus *bus)
> {
> - struct pci_dev *dev;
> + struct pci_dev *dev = bus->self;
> +
> + if (pci_has_flag(PCI_PROBE_ONLY) && dev &&
> + (dev->class >> 8) == PCI_CLASS_BRIDGE_PCI) {
> + pci_read_bridge_bases(bus);
> + }
>
> list_for_each_entry(dev, &bus->devices, bus_list) {
> pdev_save_srm_config(dev);
> diff --git a/arch/frv/mb93090-mb00/pci-vdk.c b/arch/frv/mb93090-mb00/pci-vdk.c
> index f9c86c4..f211839 100644
> --- a/arch/frv/mb93090-mb00/pci-vdk.c
> +++ b/arch/frv/mb93090-mb00/pci-vdk.c
> @@ -294,6 +294,8 @@ void pcibios_fixup_bus(struct pci_bus *bus)
> printk("### PCIBIOS_FIXUP_BUS(%d)\n",bus->number);
> #endif
>
> + pci_read_bridge_bases(bus);
> +
> if (bus->number == 0) {
> struct pci_dev *dev;
> list_for_each_entry(dev, &bus->devices, bus_list) {
> diff --git a/arch/ia64/pci/pci.c b/arch/ia64/pci/pci.c
> index d89b601..7cc3be9 100644
> --- a/arch/ia64/pci/pci.c
> +++ b/arch/ia64/pci/pci.c
> @@ -533,9 +533,10 @@ void pcibios_fixup_bus(struct pci_bus *b)
> {
> struct pci_dev *dev;
>
> - if (b->self)
> + if (b->self) {
> + pci_read_bridge_bases(b);
> pcibios_fixup_bridge_resources(b->self);
> -
> + }
> list_for_each_entry(dev, &b->devices, bus_list)
> pcibios_fixup_device_resources(dev);
> platform_pci_fixup_bus(b);
> diff --git a/arch/microblaze/pci/pci-common.c b/arch/microblaze/pci/pci-common.c
> index 6b8b752..ae838ed 100644
> --- a/arch/microblaze/pci/pci-common.c
> +++ b/arch/microblaze/pci/pci-common.c
> @@ -863,7 +863,14 @@ void pcibios_setup_bus_devices(struct pci_bus *bus)
>
> void pcibios_fixup_bus(struct pci_bus *bus)
> {
> - /* Fixup the bus */
> + /* When called from the generic PCI probe, read PCI<->PCI bridge
> + * bases. This is -not- called when generating the PCI tree from
> + * the OF device-tree.
> + */
> + if (bus->self != NULL)
> + pci_read_bridge_bases(bus);
> +
> + /* Now fixup the bus bus */
> pcibios_setup_bus_self(bus);
>
> /* Now fixup devices on that bus */
> diff --git a/arch/mips/pci/pci.c b/arch/mips/pci/pci.c
> index c6996cf..b8a0bf5 100644
> --- a/arch/mips/pci/pci.c
> +++ b/arch/mips/pci/pci.c
> @@ -311,6 +311,12 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
>
> void pcibios_fixup_bus(struct pci_bus *bus)
> {
> + struct pci_dev *dev = bus->self;
> +
> + if (pci_has_flag(PCI_PROBE_ONLY) && dev &&
> + (dev->class >> 8) == PCI_CLASS_BRIDGE_PCI) {
> + pci_read_bridge_bases(bus);
> + }
> }
>
> EXPORT_SYMBOL(PCIBIOS_MIN_IO);
> diff --git a/arch/mn10300/unit-asb2305/pci.c b/arch/mn10300/unit-asb2305/pci.c
> index deaa893..3dfe2d3 100644
> --- a/arch/mn10300/unit-asb2305/pci.c
> +++ b/arch/mn10300/unit-asb2305/pci.c
> @@ -324,6 +324,7 @@ void pcibios_fixup_bus(struct pci_bus *bus)
> struct pci_dev *dev;
>
> if (bus->self) {
> + pci_read_bridge_bases(bus);
> pcibios_fixup_bridge_resources(bus->self);
> }
>
> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
> index a1d0632..7587b2a 100644
> --- a/arch/powerpc/kernel/pci-common.c
> +++ b/arch/powerpc/kernel/pci-common.c
> @@ -1032,7 +1032,13 @@ void pcibios_set_master(struct pci_dev *dev)
>
> void pcibios_fixup_bus(struct pci_bus *bus)
> {
> - /* Fixup the bus */
> + /* When called from the generic PCI probe, read PCI<->PCI bridge
> + * bases. This is -not- called when generating the PCI tree from
> + * the OF device-tree.
> + */
> + pci_read_bridge_bases(bus);
> +
> + /* Now fixup the bus bus */
> pcibios_setup_bus_self(bus);
>
> /* Now fixup devices on that bus */
> diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
> index 09d3afc..dc78a4a 100644
> --- a/arch/x86/pci/common.c
> +++ b/arch/x86/pci/common.c
> @@ -166,6 +166,7 @@ void pcibios_fixup_bus(struct pci_bus *b)
> {
> struct pci_dev *dev;
>
> + pci_read_bridge_bases(b);
> list_for_each_entry(dev, &b->devices, bus_list)
> pcibios_fixup_device_resources(dev);
> }
> diff --git a/arch/xtensa/kernel/pci.c b/arch/xtensa/kernel/pci.c
> index d27b4dc..b848cc3 100644
> --- a/arch/xtensa/kernel/pci.c
> +++ b/arch/xtensa/kernel/pci.c
> @@ -210,6 +210,10 @@ subsys_initcall(pcibios_init);
>
> void pcibios_fixup_bus(struct pci_bus *bus)
> {
> + if (bus->parent) {
> + /* This is a subordinate bridge */
> + pci_read_bridge_bases(bus);
> + }
> }
>
> void pcibios_set_master(struct pci_dev *dev)
> diff --git a/drivers/parisc/dino.c b/drivers/parisc/dino.c
> index baec33c..a0580af 100644
> --- a/drivers/parisc/dino.c
> +++ b/drivers/parisc/dino.c
> @@ -560,6 +560,9 @@ dino_fixup_bus(struct pci_bus *bus)
> } else if (bus->parent) {
> int i;
>
> + pci_read_bridge_bases(bus);
> +
> +
> for(i = PCI_BRIDGE_RESOURCES; i < PCI_NUM_RESOURCES; i++) {
> if((bus->self->resource[i].flags &
> (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
> diff --git a/drivers/parisc/lba_pci.c b/drivers/parisc/lba_pci.c
> index 7b9e89b..a32c1f6 100644
> --- a/drivers/parisc/lba_pci.c
> +++ b/drivers/parisc/lba_pci.c
> @@ -693,6 +693,7 @@ lba_fixup_bus(struct pci_bus *bus)
> if (bus->parent) {
> int i;
> /* PCI-PCI Bridge */
> + pci_read_bridge_bases(bus);
> for (i = PCI_BRIDGE_RESOURCES; i < PCI_NUM_RESOURCES; i++)
> pci_claim_bridge_resource(bus->self, i);
> } else {
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 0b2be17..c8cc0e62 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -855,9 +855,6 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
> child->bridge_ctl = bctl;
> }
>
> - /* Read and initialize bridge resources */
> - pci_read_bridge_bases(child);
> -
> cmax = pci_scan_child_bus(child);
> if (cmax > subordinate)
> dev_warn(&dev->dev, "bridge has subordinate %02x but max busn %02x\n",
> @@ -918,9 +915,6 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
>
> if (!is_cardbus) {
> child->bridge_ctl = bctl;
> -
> - /* Read and initialize bridge resources */
> - pci_read_bridge_bases(child);
> max = pci_scan_child_bus(child);
> } else {
> /*
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
