Re: trouble with PCI: Call pci_read_bridge_bases() from core instead of arch code

[Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>]

On Tue, Sep 15, 2015 at 08:25:59PM +0100, Bjorn Helgaas wrote:

[...]

> commit d5da9d99d4d79d877815af96fdbfac829c4ce7b2
> Author: Bjorn Helgaas <bhelgaas@google.com>
> 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@gmail.com>
>     Reported-by: Ray Jui <rjui@broadcom.com>
>     Link: http://lkml.kernel.org/r/CAAa04yFQEUJm7Jj1qMT57-LG7ZGtnhNDBe=PpSRa70Mj+XhW-A@mail.gmail.com
>     Link: http://lkml.kernel.org/r/55F75BB8.4070405@broadcom.com
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

> 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 {
>                         /*
>