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

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

[oe5hpm]

Hi Lorenzo,

today i tried to boot up the most recent vanilla kernel on my
Freescale i.mx6 board.
I ran into trouble regarding PCI enumeration.

[    0.431949] imx6q-pcie 1ffc000.pcie: PCI host bridge to bus 0000:00
[    0.431976] pci_bus 0000:00: root bus resource [io  0x1000-0xffff]
[    0.431996] pci_bus 0000:00: root bus resource [mem 0x01000000-0x01efffff]
[    0.432022] pci_bus 0000:00: root bus resource [bus 00-ff]
[    0.433271] PCI: bus0: Fast back to back transfers disabled
[    0.433629] pci 0000:00:00.0: PCI bridge to [bus 01-ff]
[    0.435181] PCI: bus1: Fast back to back transfers disabled
[    0.435564] pci 0000:00:00.0: BAR 0: assigned [mem 0x01000000-0x010fffff]
[    0.435593] pci 0000:00:00.0: BAR 8: no space for [mem size 0x01000000]
[    0.435613] pci 0000:00:00.0: BAR 8: failed to assign [mem size 0x01000000]
[    0.435635] pci 0000:00:00.0: BAR 9: assigned [mem
0x01100000-0x011fffff pref]
[    0.435655] pci 0000:00:00.0: BAR 6: assigned [mem
0x01200000-0x0120ffff pref]
[    0.435676] pci 0000:00:00.0: BAR 7: assigned [io  0x1000-0x1fff]
[    0.435705] pci 0000:01:00.0: BAR 2: no space for [mem size 0x00200000]
[    0.435722] pci 0000:01:00.0: BAR 2: failed to assign [mem size 0x00200000]
[    0.435739] pci 0000:01:00.0: BAR 1: no space for [mem size 0x00004000]
[    0.435754] pci 0000:01:00.0: BAR 1: failed to assign [mem size 0x00004000]
[    0.435770] pci 0000:01:00.0: BAR 0: no space for [mem size 0x00000100]
[    0.435786] pci 0000:01:00.0: BAR 0: failed to assign [mem size 0x00000100]
[    0.435804] pci 0000:00:00.0: PCI bridge to [bus 01]
[    0.435826] pci 0000:00:00.0:   bridge window [io  0x1000-0x1fff]
[    0.435855] pci 0000:00:00.0:   bridge window [mem
0x01100000-0x011fffff pref]

there are several fails assigning memory ressources to pci-devices.

i bisect down this trouble to commit id:
dff22d2054b5dbb1889f20c03959dd0c494fab8c : PCI: Call
pci_read_bridge_bases() from core instead of arch code

For testing purpose i've reverted this commit on a local branch and
everythings works fine, as before.

[    0.431976] imx6q-pcie 1ffc000.pcie: PCI host bridge to bus 0000:00
[    0.432004] pci_bus 0000:00: root bus resource [io  0x1000-0xffff]
[    0.432023] pci_bus 0000:00: root bus resource [mem 0x01000000-0x01efffff]
[    0.432047] pci_bus 0000:00: root bus resource [bus 00-ff]
[    0.433302] PCI: bus0: Fast back to back transfers disabled
[    0.435122] PCI: bus1: Fast back to back transfers disabled
[    0.435504] pci 0000:00:00.0: BAR 0: assigned [mem 0x01000000-0x010fffff]
[    0.435535] pci 0000:00:00.0: BAR 8: assigned [mem 0x01100000-0x013fffff]
[    0.435557] pci 0000:00:00.0: BAR 6: assigned [mem
0x01400000-0x0140ffff pref]
[    0.435585] pci 0000:01:00.0: BAR 2: assigned [mem 0x01200000-0x013fffff]
[    0.435626] pci 0000:01:00.0: BAR 1: assigned [mem 0x01100000-0x01103fff]
[    0.435665] pci 0000:01:00.0: BAR 0: assigned [mem 0x01104000-0x011040ff]
[    0.435703] pci 0000:00:00.0: PCI bridge to [bus 01]
[    0.435728] pci 0000:00:00.0:   bridge window [mem 0x01100000-0x013fffff]

Further i can break down the failure to "drivers/pci/probe.c" line #924.
If i comment out the "pci_read_bridge_bases(child);" also everything works well.

I have to confess, that my knowledge about the whole PCI thing in the
kernel is not very deep, so it is not possible for me to figure out
what is going wrong.

maybe some of you can help?

best regards,
Hannes

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

[Lorenzo Pieralisi]

Hi Hannes,

On Wed, Sep 02, 2015 at 10:51:18AM +0100, oe5hpm wrote:
> Hi Lorenzo,
> 
> today i tried to boot up the most recent vanilla kernel on my
> Freescale i.mx6 board.
> I ran into trouble regarding PCI enumeration.
> 
> [    0.431949] imx6q-pcie 1ffc000.pcie: PCI host bridge to bus 0000:00
> [    0.431976] pci_bus 0000:00: root bus resource [io  0x1000-0xffff]
> [    0.431996] pci_bus 0000:00: root bus resource [mem 0x01000000-0x01efffff]
> [    0.432022] pci_bus 0000:00: root bus resource [bus 00-ff]
> [    0.433271] PCI: bus0: Fast back to back transfers disabled
> [    0.433629] pci 0000:00:00.0: PCI bridge to [bus 01-ff]
> [    0.435181] PCI: bus1: Fast back to back transfers disabled
> [    0.435564] pci 0000:00:00.0: BAR 0: assigned [mem 0x01000000-0x010fffff]
> [    0.435593] pci 0000:00:00.0: BAR 8: no space for [mem size 0x01000000]
> [    0.435613] pci 0000:00:00.0: BAR 8: failed to assign [mem size 0x01000000]
> [    0.435635] pci 0000:00:00.0: BAR 9: assigned [mem
> 0x01100000-0x011fffff pref]
> [    0.435655] pci 0000:00:00.0: BAR 6: assigned [mem
> 0x01200000-0x0120ffff pref]
> [    0.435676] pci 0000:00:00.0: BAR 7: assigned [io  0x1000-0x1fff]
> [    0.435705] pci 0000:01:00.0: BAR 2: no space for [mem size 0x00200000]
> [    0.435722] pci 0000:01:00.0: BAR 2: failed to assign [mem size 0x00200000]
> [    0.435739] pci 0000:01:00.0: BAR 1: no space for [mem size 0x00004000]
> [    0.435754] pci 0000:01:00.0: BAR 1: failed to assign [mem size 0x00004000]
> [    0.435770] pci 0000:01:00.0: BAR 0: no space for [mem size 0x00000100]
> [    0.435786] pci 0000:01:00.0: BAR 0: failed to assign [mem size 0x00000100]
> [    0.435804] pci 0000:00:00.0: PCI bridge to [bus 01]
> [    0.435826] pci 0000:00:00.0:   bridge window [io  0x1000-0x1fff]
> [    0.435855] pci 0000:00:00.0:   bridge window [mem
> 0x01100000-0x011fffff pref]
> 
> there are several fails assigning memory ressources to pci-devices.
> 
> i bisect down this trouble to commit id:
> dff22d2054b5dbb1889f20c03959dd0c494fab8c : PCI: Call
> pci_read_bridge_bases() from core instead of arch code
> 
> For testing purpose i've reverted this commit on a local branch and
> everythings works fine, as before.
> 
> [    0.431976] imx6q-pcie 1ffc000.pcie: PCI host bridge to bus 0000:00
> [    0.432004] pci_bus 0000:00: root bus resource [io  0x1000-0xffff]
> [    0.432023] pci_bus 0000:00: root bus resource [mem 0x01000000-0x01efffff]
> [    0.432047] pci_bus 0000:00: root bus resource [bus 00-ff]
> [    0.433302] PCI: bus0: Fast back to back transfers disabled
> [    0.435122] PCI: bus1: Fast back to back transfers disabled
> [    0.435504] pci 0000:00:00.0: BAR 0: assigned [mem 0x01000000-0x010fffff]
> [    0.435535] pci 0000:00:00.0: BAR 8: assigned [mem 0x01100000-0x013fffff]
> [    0.435557] pci 0000:00:00.0: BAR 6: assigned [mem
> 0x01400000-0x0140ffff pref]
> [    0.435585] pci 0000:01:00.0: BAR 2: assigned [mem 0x01200000-0x013fffff]
> [    0.435626] pci 0000:01:00.0: BAR 1: assigned [mem 0x01100000-0x01103fff]
> [    0.435665] pci 0000:01:00.0: BAR 0: assigned [mem 0x01104000-0x011040ff]
> [    0.435703] pci 0000:00:00.0: PCI bridge to [bus 01]
> [    0.435728] pci 0000:00:00.0:   bridge window [mem 0x01100000-0x013fffff]
> 
> Further i can break down the failure to "drivers/pci/probe.c" line #924.
> If i comment out the "pci_read_bridge_bases(child);" also everything works well.
> 
> I have to confess, that my knowledge about the whole PCI thing in the
> kernel is not very deep, so it is not possible for me to figure out
> what is going wrong.

It looks like a bogus bridge aperture size is causing this to happen,
and this prevents reassignment on arm (bridge aperture is too big),
which proves that reading the bridge bases without vetting the corresponding
resources may break (on platforms that were not reading them before).

arm was the only platform not reading the bridge bases, here is an
answer why. So, to prevent reverting the commit I put together this
patch (to be reworked if we deem it reasonable), subject to discussion
(I fear it may end up breaking other arm platforms, I do not have all
ARM boards and required host controllers to test, I managed to test it on
an iMX6 Sabrelite though).

Here, please let me know if it works for you, I will keep on thinking
to find the best solution.

I will have to do this for arm64 too, comments very welcome.

Thanks,
Lorenzo

-- >8 --
Subject: [PATCH] arm: kernel: pci: fixup erroneous PCI bridge apertures

Bridge apertures read by core PCI code through pci_read_bridge_bases()
might be erroneous (bogus platform setup). If the arch code does not vet
the bridge resources (ie by trying to claim them), we can end up in a
situation where wrong bridge apertures can prevent resources assignment
for downstream devices causing enumeration failures (eg a bridge
aperture does not fit in the respective host controller resource window,
so it can't be assigned).

This patch adds arm arch code that vets bridge resources by trying
to claim them, and reset them on claiming failure so that they can
be properly reassigned.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
---
 arch/arm/kernel/bios32.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index 874e182..ebbe052 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -282,6 +282,27 @@ static inline int pdev_bad_for_parity(struct pci_dev *dev)
 
 }
 
+static void pcibios_fixup_bridge_resources(struct pci_dev *dev)
+{
+	int idx;
+
+	if (!dev->bus)
+		return;
+
+	for (idx = PCI_BRIDGE_RESOURCES; idx < PCI_NUM_RESOURCES; idx++) {
+		struct resource *r = &dev->resource[idx];
+
+		if (!r->flags || r->parent)
+			continue;
+
+		if (pci_claim_resource(dev, idx)) {
+			r->flags = 0;
+			r->start = 0;
+			r->end = -1;
+		}
+	}
+}
+
 /*
  * pcibios_fixup_bus - Called after each bus is probed,
  * but before its children are examined.
@@ -352,6 +373,9 @@ void pcibios_fixup_bus(struct pci_bus *bus)
 			bus->bridge_ctl |= PCI_BRIDGE_CTL_PARITY;
 	}
 
+	if (bus->self)
+		pcibios_fixup_bridge_resources(bus->self);
+
 	/*
 	 * Report what we did for this bus
 	 */
-- 
1.9.1

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

[Bjorn Helgaas]

Hi Lorenzo, thanks for jumping on this!

On Wed, Sep 02, 2015 at 06:47:16PM +0100, Lorenzo Pieralisi wrote:
> Hi Hannes,
> 
> On Wed, Sep 02, 2015 at 10:51:18AM +0100, oe5hpm wrote:
> > Hi Lorenzo,
> > 
> > today i tried to boot up the most recent vanilla kernel on my
> > Freescale i.mx6 board.
> > I ran into trouble regarding PCI enumeration.
> > 
> > [    0.431949] imx6q-pcie 1ffc000.pcie: PCI host bridge to bus 0000:00
> > [    0.431976] pci_bus 0000:00: root bus resource [io  0x1000-0xffff]
> > [    0.431996] pci_bus 0000:00: root bus resource [mem 0x01000000-0x01efffff]
> > [    0.432022] pci_bus 0000:00: root bus resource [bus 00-ff]
> > [    0.433271] PCI: bus0: Fast back to back transfers disabled
> > [    0.433629] pci 0000:00:00.0: PCI bridge to [bus 01-ff]
> > [    0.435181] PCI: bus1: Fast back to back transfers disabled
> > [    0.435564] pci 0000:00:00.0: BAR 0: assigned [mem 0x01000000-0x010fffff]
> > [    0.435593] pci 0000:00:00.0: BAR 8: no space for [mem size 0x01000000]
> > [    0.435613] pci 0000:00:00.0: BAR 8: failed to assign [mem size 0x01000000]
> > [    0.435635] pci 0000:00:00.0: BAR 9: assigned [mem
> > 0x01100000-0x011fffff pref]
> > [    0.435655] pci 0000:00:00.0: BAR 6: assigned [mem
> > 0x01200000-0x0120ffff pref]
> > [    0.435676] pci 0000:00:00.0: BAR 7: assigned [io  0x1000-0x1fff]
> > [    0.435705] pci 0000:01:00.0: BAR 2: no space for [mem size 0x00200000]
> > [    0.435722] pci 0000:01:00.0: BAR 2: failed to assign [mem size 0x00200000]
> > [    0.435739] pci 0000:01:00.0: BAR 1: no space for [mem size 0x00004000]
> > [    0.435754] pci 0000:01:00.0: BAR 1: failed to assign [mem size 0x00004000]
> > [    0.435770] pci 0000:01:00.0: BAR 0: no space for [mem size 0x00000100]
> > [    0.435786] pci 0000:01:00.0: BAR 0: failed to assign [mem size 0x00000100]
> > [    0.435804] pci 0000:00:00.0: PCI bridge to [bus 01]
> > [    0.435826] pci 0000:00:00.0:   bridge window [io  0x1000-0x1fff]
> > [    0.435855] pci 0000:00:00.0:   bridge window [mem
> > 0x01100000-0x011fffff pref]
> > 
> > there are several fails assigning memory ressources to pci-devices.
> > 
> > i bisect down this trouble to commit id:
> > dff22d2054b5dbb1889f20c03959dd0c494fab8c : PCI: Call
> > pci_read_bridge_bases() from core instead of arch code
> > 
> > For testing purpose i've reverted this commit on a local branch and
> > everythings works fine, as before.
> > 
> > [    0.431976] imx6q-pcie 1ffc000.pcie: PCI host bridge to bus 0000:00
> > [    0.432004] pci_bus 0000:00: root bus resource [io  0x1000-0xffff]
> > [    0.432023] pci_bus 0000:00: root bus resource [mem 0x01000000-0x01efffff]
> > [    0.432047] pci_bus 0000:00: root bus resource [bus 00-ff]
> > [    0.433302] PCI: bus0: Fast back to back transfers disabled
> > [    0.435122] PCI: bus1: Fast back to back transfers disabled
> > [    0.435504] pci 0000:00:00.0: BAR 0: assigned [mem 0x01000000-0x010fffff]
> > [    0.435535] pci 0000:00:00.0: BAR 8: assigned [mem 0x01100000-0x013fffff]
> > [    0.435557] pci 0000:00:00.0: BAR 6: assigned [mem
> > 0x01400000-0x0140ffff pref]
> > [    0.435585] pci 0000:01:00.0: BAR 2: assigned [mem 0x01200000-0x013fffff]
> > [    0.435626] pci 0000:01:00.0: BAR 1: assigned [mem 0x01100000-0x01103fff]
> > [    0.435665] pci 0000:01:00.0: BAR 0: assigned [mem 0x01104000-0x011040ff]
> > [    0.435703] pci 0000:00:00.0: PCI bridge to [bus 01]
> > [    0.435728] pci 0000:00:00.0:   bridge window [mem 0x01100000-0x013fffff]
> > 
> > Further i can break down the failure to "drivers/pci/probe.c" line #924.
> > If i comment out the "pci_read_bridge_bases(child);" also everything works well.
> > 
> > I have to confess, that my knowledge about the whole PCI thing in the
> > kernel is not very deep, so it is not possible for me to figure out
> > what is going wrong.
> 
> It looks like a bogus bridge aperture size is causing this to happen,
> and this prevents reassignment on arm (bridge aperture is too big),
> which proves that reading the bridge bases without vetting the corresponding
> resources may break (on platforms that were not reading them before).
> 
> arm was the only platform not reading the bridge bases, here is an
> answer why. So, to prevent reverting the commit I put together this
> patch (to be reworked if we deem it reasonable), subject to discussion
> (I fear it may end up breaking other arm platforms, I do not have all
> ARM boards and required host controllers to test, I managed to test it on
> an iMX6 Sabrelite though).
> 
> Here, please let me know if it works for you, I will keep on thinking
> to find the best solution.
> 
> I will have to do this for arm64 too, comments very welcome.
> 
> Thanks,
> Lorenzo
> 
> -- >8 --
> Subject: [PATCH] arm: kernel: pci: fixup erroneous PCI bridge apertures
> 
> Bridge apertures read by core PCI code through pci_read_bridge_bases()
> might be erroneous (bogus platform setup). If the arch code does not vet
> the bridge resources (ie by trying to claim them), we can end up in a
> situation where wrong bridge apertures can prevent resources assignment
> for downstream devices causing enumeration failures (eg a bridge
> aperture does not fit in the respective host controller resource window,
> so it can't be assigned).
> 
> This patch adds arm arch code that vets bridge resources by trying
> to claim them, and reset them on claiming failure so that they can
> be properly reassigned.

We definitely should not depend on the platform to set up the bridge
windows.  Do we know what the platform left in the 00:00.0 window
registers?

I see that bus 01 requires 0x204100 of mem space, which must be
rounded up to a megabyte boundary, so the window must be at least 3M
(0x00300000):

  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]

I don't understand the connection with dff22d2054b5 yet.  If we don't
call pci_read_bridge_bases(), apparently some assign-resources path
figures out the required size and assigns a 3M window.

If we *do* call pci_read_bridge_bases(), do we read a bogus 16M window
size, fail to assign that because the host controller window isn't big
enough, and then the assign-resources path just gives up?  I assume
clearing r->flags in your patch is the critical thing?  Is there
something in assign-resources that checks for r->flags == 0?

I think it would be ideal if we could someday claim the resource
immediately, as soon as we read it from a BAR or bridge window, and
mark it as IORESOURCE_UNSET if claiming it fails.  Then if the
platform set up reasonable windows, we could use them; if it didn't,
we could just assign our own.

I'd like to avoid adding things to pcibios_fixup_bus() if possible
because most of what is done there is arch-independent, and I'd like
to get the arch-independent stuff into the PCI core.

Bjorn

> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> ---
>  arch/arm/kernel/bios32.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> index 874e182..ebbe052 100644
> --- a/arch/arm/kernel/bios32.c
> +++ b/arch/arm/kernel/bios32.c
> @@ -282,6 +282,27 @@ static inline int pdev_bad_for_parity(struct pci_dev *dev)
>  
>  }
>  
> +static void pcibios_fixup_bridge_resources(struct pci_dev *dev)
> +{
> +	int idx;
> +
> +	if (!dev->bus)
> +		return;
> +
> +	for (idx = PCI_BRIDGE_RESOURCES; idx < PCI_NUM_RESOURCES; idx++) {
> +		struct resource *r = &dev->resource[idx];
> +
> +		if (!r->flags || r->parent)
> +			continue;
> +
> +		if (pci_claim_resource(dev, idx)) {
> +			r->flags = 0;
> +			r->start = 0;
> +			r->end = -1;
> +		}
> +	}
> +}
> +
>  /*
>   * pcibios_fixup_bus - Called after each bus is probed,
>   * but before its children are examined.
> @@ -352,6 +373,9 @@ void pcibios_fixup_bus(struct pci_bus *bus)
>  			bus->bridge_ctl |= PCI_BRIDGE_CTL_PARITY;
>  	}
>  
> +	if (bus->self)
> +		pcibios_fixup_bridge_resources(bus->self);
> +
>  	/*
>  	 * Report what we did for this bus
>  	 */
> -- 
> 1.9.1
> 
> 

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

[Lorenzo Pieralisi]

Hi Bjorn,

On Wed, Sep 02, 2015 at 09:32:50PM +0100, Bjorn Helgaas wrote:
> Hi Lorenzo, thanks for jumping on this!

:) I really want to get to the bottom of this resource allocation
issue.

> On Wed, Sep 02, 2015 at 06:47:16PM +0100, Lorenzo Pieralisi wrote:
> > Hi Hannes,
> >
> > On Wed, Sep 02, 2015 at 10:51:18AM +0100, oe5hpm wrote:
> > > Hi Lorenzo,
> > >
> > > today i tried to boot up the most recent vanilla kernel on my
> > > Freescale i.mx6 board.
> > > I ran into trouble regarding PCI enumeration.
> > >
> > > [    0.431949] imx6q-pcie 1ffc000.pcie: PCI host bridge to bus 0000:00
> > > [    0.431976] pci_bus 0000:00: root bus resource [io  0x1000-0xffff]
> > > [    0.431996] pci_bus 0000:00: root bus resource [mem 0x01000000-0x01efffff]
> > > [    0.432022] pci_bus 0000:00: root bus resource [bus 00-ff]
> > > [    0.433271] PCI: bus0: Fast back to back transfers disabled
> > > [    0.433629] pci 0000:00:00.0: PCI bridge to [bus 01-ff]
> > > [    0.435181] PCI: bus1: Fast back to back transfers disabled
> > > [    0.435564] pci 0000:00:00.0: BAR 0: assigned [mem 0x01000000-0x010fffff]
> > > [    0.435593] pci 0000:00:00.0: BAR 8: no space for [mem size 0x01000000]
> > > [    0.435613] pci 0000:00:00.0: BAR 8: failed to assign [mem size 0x01000000]
> > > [    0.435635] pci 0000:00:00.0: BAR 9: assigned [mem
> > > 0x01100000-0x011fffff pref]
> > > [    0.435655] pci 0000:00:00.0: BAR 6: assigned [mem
> > > 0x01200000-0x0120ffff pref]
> > > [    0.435676] pci 0000:00:00.0: BAR 7: assigned [io  0x1000-0x1fff]
> > > [    0.435705] pci 0000:01:00.0: BAR 2: no space for [mem size 0x00200000]
> > > [    0.435722] pci 0000:01:00.0: BAR 2: failed to assign [mem size 0x00200000]
> > > [    0.435739] pci 0000:01:00.0: BAR 1: no space for [mem size 0x00004000]
> > > [    0.435754] pci 0000:01:00.0: BAR 1: failed to assign [mem size 0x00004000]
> > > [    0.435770] pci 0000:01:00.0: BAR 0: no space for [mem size 0x00000100]
> > > [    0.435786] pci 0000:01:00.0: BAR 0: failed to assign [mem size 0x00000100]
> > > [    0.435804] pci 0000:00:00.0: PCI bridge to [bus 01]
> > > [    0.435826] pci 0000:00:00.0:   bridge window [io  0x1000-0x1fff]
> > > [    0.435855] pci 0000:00:00.0:   bridge window [mem
> > > 0x01100000-0x011fffff pref]
> > >
> > > there are several fails assigning memory ressources to pci-devices.
> > >
> > > i bisect down this trouble to commit id:
> > > dff22d2054b5dbb1889f20c03959dd0c494fab8c : PCI: Call
> > > pci_read_bridge_bases() from core instead of arch code
> > >
> > > For testing purpose i've reverted this commit on a local branch and
> > > everythings works fine, as before.
> > >
> > > [    0.431976] imx6q-pcie 1ffc000.pcie: PCI host bridge to bus 0000:00
> > > [    0.432004] pci_bus 0000:00: root bus resource [io  0x1000-0xffff]
> > > [    0.432023] pci_bus 0000:00: root bus resource [mem 0x01000000-0x01efffff]
> > > [    0.432047] pci_bus 0000:00: root bus resource [bus 00-ff]
> > > [    0.433302] PCI: bus0: Fast back to back transfers disabled
> > > [    0.435122] PCI: bus1: Fast back to back transfers disabled
> > > [    0.435504] pci 0000:00:00.0: BAR 0: assigned [mem 0x01000000-0x010fffff]
> > > [    0.435535] pci 0000:00:00.0: BAR 8: assigned [mem 0x01100000-0x013fffff]
> > > [    0.435557] pci 0000:00:00.0: BAR 6: assigned [mem
> > > 0x01400000-0x0140ffff pref]
> > > [    0.435585] pci 0000:01:00.0: BAR 2: assigned [mem 0x01200000-0x013fffff]
> > > [    0.435626] pci 0000:01:00.0: BAR 1: assigned [mem 0x01100000-0x01103fff]
> > > [    0.435665] pci 0000:01:00.0: BAR 0: assigned [mem 0x01104000-0x011040ff]
> > > [    0.435703] pci 0000:00:00.0: PCI bridge to [bus 01]
> > > [    0.435728] pci 0000:00:00.0:   bridge window [mem 0x01100000-0x013fffff]
> > >
> > > Further i can break down the failure to "drivers/pci/probe.c" line #924.
> > > If i comment out the "pci_read_bridge_bases(child);" also everything works well.
> > >
> > > I have to confess, that my knowledge about the whole PCI thing in the
> > > kernel is not very deep, so it is not possible for me to figure out
> > > what is going wrong.
> >
> > It looks like a bogus bridge aperture size is causing this to happen,
> > and this prevents reassignment on arm (bridge aperture is too big),
> > which proves that reading the bridge bases without vetting the corresponding
> > resources may break (on platforms that were not reading them before).
> >
> > arm was the only platform not reading the bridge bases, here is an
> > answer why. So, to prevent reverting the commit I put together this
> > patch (to be reworked if we deem it reasonable), subject to discussion
> > (I fear it may end up breaking other arm platforms, I do not have all
> > ARM boards and required host controllers to test, I managed to test it on
> > an iMX6 Sabrelite though).
> >
> > Here, please let me know if it works for you, I will keep on thinking
> > to find the best solution.
> >
> > I will have to do this for arm64 too, comments very welcome.
> >
> > Thanks,
> > Lorenzo
> >
> > -- >8 --
> > Subject: [PATCH] arm: kernel: pci: fixup erroneous PCI bridge apertures
> >
> > Bridge apertures read by core PCI code through pci_read_bridge_bases()
> > might be erroneous (bogus platform setup). If the arch code does not vet
> > the bridge resources (ie by trying to claim them), we can end up in a
> > situation where wrong bridge apertures can prevent resources assignment
> > for downstream devices causing enumeration failures (eg a bridge
> > aperture does not fit in the respective host controller resource window,
> > so it can't be assigned).
> >
> > This patch adds arm arch code that vets bridge resources by trying
> > to claim them, and reset them on claiming failure so that they can
> > be properly reassigned.
> 
> We definitely should not depend on the platform to set up the bridge
> windows.  Do we know what the platform left in the 00:00.0 window
> registers?

Well, I agree but the point here is, by reading the bridge bases
we are initializing the apertures resources and this is causing
issues, we have to have a way to nuke the initialized apertures resources
if they are bogus, more below. I wonder why we want to read the bridge
apertures at all on !PCI_PROBE_ONLY systems.

pci 0000:00:00.0:   bridge window [mem 0x01000000-0x01ffffff]

> I see that bus 01 requires 0x204100 of mem space, which must be
> rounded up to a megabyte boundary, so the window must be at least 3M
> (0x00300000):
> 
>   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]
> 
> I don't understand the connection with dff22d2054b5 yet.  If we don't
> call pci_read_bridge_bases(), apparently some assign-resources path
> figures out the required size and assigns a 3M window.
> 
> If we *do* call pci_read_bridge_bases(), do we read a bogus 16M window
> size, fail to assign that because the host controller window isn't big
> enough, and then the assign-resources path just gives up?  I assume
> clearing r->flags in your patch is the critical thing?  Is there
> something in assign-resources that checks for r->flags == 0?

You summed it up, but the point here is not about the flags, it is
about the bogus 16M bridge aperture (so r->start and r->end).

While sizing the bridge apertures, the code in pbus_size_mem() checks
the size required by devices and then set-up the bridge aperture.

Now, calculate_memsize() takes as an input the "old" aperture size (16M)
which means that the updated aperture will keep the old aperture
size instead of the one computed from the size of downstream devices
(because the old aperture - read from bridge bases - is larger, see
calculate_memsize()), hence the failure.

If the bridge aperture is reset (ie resource start and end are zeroed)
before sizing the bridge everything is back to normal.

x86 does the same thing I implement in the patch attached, and probably
we have also discovered why Alpha and MIPS were reading bridge bases
on PROBE_ONLY systems only.

> I think it would be ideal if we could someday claim the resource
> immediately, as soon as we read it from a BAR or bridge window, and
> mark it as IORESOURCE_UNSET if claiming it fails.  Then if the
> platform set up reasonable windows, we could use them; if it didn't,
> we could just assign our own.

Well, that's what my patch does and that's what x86 does. I am nervous
about adding this to core PCI code (in particular I am worried about
claiming the bridge windows ie when you say reasonable, it does not
necessarily mean optimal, claiming the bridge apertures can cause
issues in relation to resources allocation IMO since we claim
the aperture before sizing the bridge).

My question is: on !PCI_PROBE_ONLY systems, why do we want to "trust" the
bridge bases (that we want to reassign after sizing bridges *anyway*) ?
I understand on PCI_PROBE_ONLY systems they should be immutable, I would
like to understand why we have to read them on !PCI_PROBE_ONLY systems.

> I'd like to avoid adding things to pcibios_fixup_bus() if possible
> because most of what is done there is arch-independent, and I'd like
> to get the arch-independent stuff into the PCI core.

I agree, but if we move my patch to core code I expect failures owing
to the claiming of the bridge apertures (I am still very very
concerned about claiming the bridge apertures by default).

I do not think there is any other way of *validating* the bridge bases
read from HW, the question is whether we should read them on
!PCI_PROBE_ONLY systems at all, if the answer is yes options are
limited (and I am not entirely happy with my patch, again, claiming
the bridge apertures IMO is a risky business).

Lorenzo

> Bjorn
> 
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > ---
> >  arch/arm/kernel/bios32.c | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> >
> > diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> > index 874e182..ebbe052 100644
> > --- a/arch/arm/kernel/bios32.c
> > +++ b/arch/arm/kernel/bios32.c
> > @@ -282,6 +282,27 @@ static inline int pdev_bad_for_parity(struct pci_dev *dev)
> >
> >  }
> >
> > +static void pcibios_fixup_bridge_resources(struct pci_dev *dev)
> > +{
> > +     int idx;
> > +
> > +     if (!dev->bus)
> > +             return;
> > +
> > +     for (idx = PCI_BRIDGE_RESOURCES; idx < PCI_NUM_RESOURCES; idx++) {
> > +             struct resource *r = &dev->resource[idx];
> > +
> > +             if (!r->flags || r->parent)
> > +                     continue;
> > +
> > +             if (pci_claim_resource(dev, idx)) {
> > +                     r->flags = 0;
> > +                     r->start = 0;
> > +                     r->end = -1;
> > +             }
> > +     }
> > +}
> > +
> >  /*
> >   * pcibios_fixup_bus - Called after each bus is probed,
> >   * but before its children are examined.
> > @@ -352,6 +373,9 @@ void pcibios_fixup_bus(struct pci_bus *bus)
> >                       bus->bridge_ctl |= PCI_BRIDGE_CTL_PARITY;
> >       }
> >
> > +     if (bus->self)
> > +             pcibios_fixup_bridge_resources(bus->self);
> > +
> >       /*
> >        * Report what we did for this bus
> >        */
> > --
> > 1.9.1
> >
> >
> 

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

[Bjorn Helgaas]

[+cc Yinghai]

On Thu, Sep 03, 2015 at 11:01:29AM +0100, Lorenzo Pieralisi wrote:
> Hi Bjorn,
> 
> On Wed, Sep 02, 2015 at 09:32:50PM +0100, Bjorn Helgaas wrote:
> > Hi Lorenzo, thanks for jumping on this!
> 
> :) I really want to get to the bottom of this resource allocation
> issue.
> 
> > On Wed, Sep 02, 2015 at 06:47:16PM +0100, Lorenzo Pieralisi wrote:
> > > Hi Hannes,
> > >
> > > On Wed, Sep 02, 2015 at 10:51:18AM +0100, oe5hpm wrote:
> > > > Hi Lorenzo,
> > > >
> > > > today i tried to boot up the most recent vanilla kernel on my
> > > > Freescale i.mx6 board.
> > > > I ran into trouble regarding PCI enumeration.
> > > >
> > > > [    0.431949] imx6q-pcie 1ffc000.pcie: PCI host bridge to bus 0000:00
> > > > [    0.431976] pci_bus 0000:00: root bus resource [io  0x1000-0xffff]
> > > > [    0.431996] pci_bus 0000:00: root bus resource [mem 0x01000000-0x01efffff]
> > > > [    0.432022] pci_bus 0000:00: root bus resource [bus 00-ff]
> > > > [    0.433271] PCI: bus0: Fast back to back transfers disabled
> > > > [    0.433629] pci 0000:00:00.0: PCI bridge to [bus 01-ff]
> > > > [    0.435181] PCI: bus1: Fast back to back transfers disabled
> > > > [    0.435564] pci 0000:00:00.0: BAR 0: assigned [mem 0x01000000-0x010fffff]
> > > > [    0.435593] pci 0000:00:00.0: BAR 8: no space for [mem size 0x01000000]
> > > > [    0.435613] pci 0000:00:00.0: BAR 8: failed to assign [mem size 0x01000000]
> > > > [    0.435635] pci 0000:00:00.0: BAR 9: assigned [mem
> > > > 0x01100000-0x011fffff pref]
> > > > [    0.435655] pci 0000:00:00.0: BAR 6: assigned [mem
> > > > 0x01200000-0x0120ffff pref]
> > > > [    0.435676] pci 0000:00:00.0: BAR 7: assigned [io  0x1000-0x1fff]
> > > > [    0.435705] pci 0000:01:00.0: BAR 2: no space for [mem size 0x00200000]
> > > > [    0.435722] pci 0000:01:00.0: BAR 2: failed to assign [mem size 0x00200000]
> > > > [    0.435739] pci 0000:01:00.0: BAR 1: no space for [mem size 0x00004000]
> > > > [    0.435754] pci 0000:01:00.0: BAR 1: failed to assign [mem size 0x00004000]
> > > > [    0.435770] pci 0000:01:00.0: BAR 0: no space for [mem size 0x00000100]
> > > > [    0.435786] pci 0000:01:00.0: BAR 0: failed to assign [mem size 0x00000100]
> > > > [    0.435804] pci 0000:00:00.0: PCI bridge to [bus 01]
> > > > [    0.435826] pci 0000:00:00.0:   bridge window [io  0x1000-0x1fff]
> > > > [    0.435855] pci 0000:00:00.0:   bridge window [mem
> > > > 0x01100000-0x011fffff pref]
> > > >
> > > > there are several fails assigning memory ressources to pci-devices.
> > > >
> > > > i bisect down this trouble to commit id:
> > > > dff22d2054b5dbb1889f20c03959dd0c494fab8c : PCI: Call
> > > > pci_read_bridge_bases() from core instead of arch code
> > > >
> > > > For testing purpose i've reverted this commit on a local branch and
> > > > everythings works fine, as before.
> > > >
> > > > [    0.431976] imx6q-pcie 1ffc000.pcie: PCI host bridge to bus 0000:00
> > > > [    0.432004] pci_bus 0000:00: root bus resource [io  0x1000-0xffff]
> > > > [    0.432023] pci_bus 0000:00: root bus resource [mem 0x01000000-0x01efffff]
> > > > [    0.432047] pci_bus 0000:00: root bus resource [bus 00-ff]
> > > > [    0.433302] PCI: bus0: Fast back to back transfers disabled
> > > > [    0.435122] PCI: bus1: Fast back to back transfers disabled
> > > > [    0.435504] pci 0000:00:00.0: BAR 0: assigned [mem 0x01000000-0x010fffff]
> > > > [    0.435535] pci 0000:00:00.0: BAR 8: assigned [mem 0x01100000-0x013fffff]
> > > > [    0.435557] pci 0000:00:00.0: BAR 6: assigned [mem
> > > > 0x01400000-0x0140ffff pref]
> > > > [    0.435585] pci 0000:01:00.0: BAR 2: assigned [mem 0x01200000-0x013fffff]
> > > > [    0.435626] pci 0000:01:00.0: BAR 1: assigned [mem 0x01100000-0x01103fff]
> > > > [    0.435665] pci 0000:01:00.0: BAR 0: assigned [mem 0x01104000-0x011040ff]
> > > > [    0.435703] pci 0000:00:00.0: PCI bridge to [bus 01]
> > > > [    0.435728] pci 0000:00:00.0:   bridge window [mem 0x01100000-0x013fffff]
> > > >
> > > > Further i can break down the failure to "drivers/pci/probe.c" line #924.
> > > > If i comment out the "pci_read_bridge_bases(child);" also everything works well.
> > > >
> > > > I have to confess, that my knowledge about the whole PCI thing in the
> > > > kernel is not very deep, so it is not possible for me to figure out
> > > > what is going wrong.
> > >
> > > It looks like a bogus bridge aperture size is causing this to happen,
> > > and this prevents reassignment on arm (bridge aperture is too big),
> > > which proves that reading the bridge bases without vetting the corresponding
> > > resources may break (on platforms that were not reading them before).
> > >
> > > arm was the only platform not reading the bridge bases, here is an
> > > answer why. So, to prevent reverting the commit I put together this
> > > patch (to be reworked if we deem it reasonable), subject to discussion
> > > (I fear it may end up breaking other arm platforms, I do not have all
> > > ARM boards and required host controllers to test, I managed to test it on
> > > an iMX6 Sabrelite though).
> > >
> > > Here, please let me know if it works for you, I will keep on thinking
> > > to find the best solution.
> > >
> > > I will have to do this for arm64 too, comments very welcome.
> > >
> > > Thanks,
> > > Lorenzo
> > >
> > > -- >8 --
> > > Subject: [PATCH] arm: kernel: pci: fixup erroneous PCI bridge apertures
> > >
> > > Bridge apertures read by core PCI code through pci_read_bridge_bases()
> > > might be erroneous (bogus platform setup). If the arch code does not vet
> > > the bridge resources (ie by trying to claim them), we can end up in a
> > > situation where wrong bridge apertures can prevent resources assignment
> > > for downstream devices causing enumeration failures (eg a bridge
> > > aperture does not fit in the respective host controller resource window,
> > > so it can't be assigned).
> > >
> > > This patch adds arm arch code that vets bridge resources by trying
> > > to claim them, and reset them on claiming failure so that they can
> > > be properly reassigned.
> > 
> > We definitely should not depend on the platform to set up the bridge
> > windows.  Do we know what the platform left in the 00:00.0 window
> > registers?
> 
> Well, I agree but the point here is, by reading the bridge bases
> we are initializing the apertures resources and this is causing
> issues, we have to have a way to nuke the initialized apertures resources
> if they are bogus, more below. I wonder why we want to read the bridge
> apertures at all on !PCI_PROBE_ONLY systems.

I'm not quite sure I understand your question.  We have to know the
bridge apertures to know whether downstream device BARs are valid.
For PCI_PROBE_ONLY, that means reading the apertures, since we won't
assign them ourselves.

For !PCI_PROBE_ONLY, we *could* completely disregard the bridge
apertures (except to determine what kind of windows we have) and
assign them from scratch.  But I don't like that approach because
we're throwing away any assignment done by the firmware without even
considering whether it's valid.

I would like /proc/iomem to contain host bridge windows, P2P bridge
windows, and device BARs.  I think the contents should be identical
for PCI_PROBE_ONLY and !PCI_PROBE_ONLY unless we actually changed
something in the !PCI_PROBE_ONLY case.

> pci 0000:00:00.0:   bridge window [mem 0x01000000-0x01ffffff]
> 
> > I see that bus 01 requires 0x204100 of mem space, which must be
> > rounded up to a megabyte boundary, so the window must be at least 3M
> > (0x00300000):
> > 
> >   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]
> > 
> > I don't understand the connection with dff22d2054b5 yet.  If we don't
> > call pci_read_bridge_bases(), apparently some assign-resources path
> > figures out the required size and assigns a 3M window.
> > 
> > If we *do* call pci_read_bridge_bases(), do we read a bogus 16M window
> > size, fail to assign that because the host controller window isn't big
> > enough, and then the assign-resources path just gives up?  I assume
> > clearing r->flags in your patch is the critical thing?  Is there
> > something in assign-resources that checks for r->flags == 0?
> 
> You summed it up, but the point here is not about the flags, it is
> about the bogus 16M bridge aperture (so r->start and r->end).
> 
> While sizing the bridge apertures, the code in pbus_size_mem() checks
> the size required by devices and then set-up the bridge aperture.
> 
> Now, calculate_memsize() takes as an input the "old" aperture size (16M)
> which means that the updated aperture will keep the old aperture
> size instead of the one computed from the size of downstream devices
> (because the old aperture - read from bridge bases - is larger, see
> calculate_memsize()), hence the failure.

I don't see the point of sizing a bridge at all *unless* we find that
we need to reassign one of its windows.  If firmware gave us a working
assignment, we should read the bridge windows, claim them, read the
BARs of downstream devices, claim them, and be done.  If firmware
didn't give us a working assignment (as in this case), we should read
the window, attempt to claim it, fail, *then* figure out how big the
window needs to be to accommodate all the downstream BARs.  In that
case, the original window size is irrelevant.

So I'm dubious about the idea that calculate_memsize() should keep the
old size if it is larger.

> If the bridge aperture is reset (ie resource start and end are zeroed)
> before sizing the bridge everything is back to normal.
> 
> x86 does the same thing I implement in the patch attached, and probably
> we have also discovered why Alpha and MIPS were reading bridge bases
> on PROBE_ONLY systems only.
> 
> > I think it would be ideal if we could someday claim the resource
> > immediately, as soon as we read it from a BAR or bridge window, and
> > mark it as IORESOURCE_UNSET if claiming it fails.  Then if the
> > platform set up reasonable windows, we could use them; if it didn't,
> > we could just assign our own.
> 
> Well, that's what my patch does and that's what x86 does. I am nervous
> about adding this to core PCI code (in particular I am worried about
> claiming the bridge windows ie when you say reasonable, it does not
> necessarily mean optimal, claiming the bridge apertures can cause
> issues in relation to resources allocation IMO since we claim
> the aperture before sizing the bridge).

I think we should claim the resource immediately so the resource tree
reflects what the hardware is doing.  If we have to reassign things,
we can release the original assignment and claim the new one.

I agree we're going to trip over issues.  But I think those issues are
symptoms of things we're doing wrong, so I think we should find and
fix them instead of tip-toeing around them.  We might need short-term
workarounds, and I'm OK with that as long we try not to think of them
as the real fixes.

> My question is: on !PCI_PROBE_ONLY systems, why do we want to "trust" the
> bridge bases (that we want to reassign after sizing bridges *anyway*) ?
> I understand on PCI_PROBE_ONLY systems they should be immutable, I would
> like to understand why we have to read them on !PCI_PROBE_ONLY systems.

For the normal case (!PCI_PROBE_ONLY), we've historically used the
existing window assignments if they work.  We only reassign windows if
there's a reason why we have to, e.g., some device has no resources
but we can give it some by rearranging things.  I think it makes sense
to continue that practice -- why would we change something that is
already working?

> > I'd like to avoid adding things to pcibios_fixup_bus() if possible
> > because most of what is done there is arch-independent, and I'd like
> > to get the arch-independent stuff into the PCI core.
> 
> I agree, but if we move my patch to core code I expect failures owing
> to the claiming of the bridge apertures (I am still very very
> concerned about claiming the bridge apertures by default).
> 
> I do not think there is any other way of *validating* the bridge bases
> read from HW, the question is whether we should read them on
> !PCI_PROBE_ONLY systems at all, if the answer is yes options are
> limited (and I am not entirely happy with my patch, again, claiming
> the bridge apertures IMO is a risky business).
> 
> Lorenzo
> 
> > Bjorn
> > 
> > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > ---
> > >  arch/arm/kernel/bios32.c | 24 ++++++++++++++++++++++++
> > >  1 file changed, 24 insertions(+)
> > >
> > > diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> > > index 874e182..ebbe052 100644
> > > --- a/arch/arm/kernel/bios32.c
> > > +++ b/arch/arm/kernel/bios32.c
> > > @@ -282,6 +282,27 @@ static inline int pdev_bad_for_parity(struct pci_dev *dev)
> > >
> > >  }
> > >
> > > +static void pcibios_fixup_bridge_resources(struct pci_dev *dev)
> > > +{
> > > +     int idx;
> > > +
> > > +     if (!dev->bus)
> > > +             return;
> > > +
> > > +     for (idx = PCI_BRIDGE_RESOURCES; idx < PCI_NUM_RESOURCES; idx++) {
> > > +             struct resource *r = &dev->resource[idx];
> > > +
> > > +             if (!r->flags || r->parent)
> > > +                     continue;
> > > +
> > > +             if (pci_claim_resource(dev, idx)) {
> > > +                     r->flags = 0;
> > > +                     r->start = 0;
> > > +                     r->end = -1;
> > > +             }
> > > +     }
> > > +}
> > > +
> > >  /*
> > >   * pcibios_fixup_bus - Called after each bus is probed,
> > >   * but before its children are examined.
> > > @@ -352,6 +373,9 @@ void pcibios_fixup_bus(struct pci_bus *bus)
> > >                       bus->bridge_ctl |= PCI_BRIDGE_CTL_PARITY;
> > >       }
> > >
> > > +     if (bus->self)
> > > +             pcibios_fixup_bridge_resources(bus->self);
> > > +
> > >       /*
> > >        * Report what we did for this bus
> > >        */
> > > --
> > > 1.9.1
> > >
> > >
> > 

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

[Lorenzo Pieralisi]

On Thu, Sep 03, 2015 at 05:21:40PM +0100, Bjorn Helgaas wrote:

[...]

> > > > Subject: [PATCH] arm: kernel: pci: fixup erroneous PCI bridge apertures
> > > >
> > > > Bridge apertures read by core PCI code through pci_read_bridge_bases()
> > > > might be erroneous (bogus platform setup). If the arch code does not vet
> > > > the bridge resources (ie by trying to claim them), we can end up in a
> > > > situation where wrong bridge apertures can prevent resources assignment
> > > > for downstream devices causing enumeration failures (eg a bridge
> > > > aperture does not fit in the respective host controller resource window,
> > > > so it can't be assigned).
> > > >
> > > > This patch adds arm arch code that vets bridge resources by trying
> > > > to claim them, and reset them on claiming failure so that they can
> > > > be properly reassigned.
> > >
> > > We definitely should not depend on the platform to set up the bridge
> > > windows.  Do we know what the platform left in the 00:00.0 window
> > > registers?
> >
> > Well, I agree but the point here is, by reading the bridge bases
> > we are initializing the apertures resources and this is causing
> > issues, we have to have a way to nuke the initialized apertures resources
> > if they are bogus, more below. I wonder why we want to read the bridge
> > apertures at all on !PCI_PROBE_ONLY systems.
> 
> I'm not quite sure I understand your question.  We have to know the
> bridge apertures to know whether downstream device BARs are valid.
> For PCI_PROBE_ONLY, that means reading the apertures, since we won't
> assign them ourselves.
> 
> For !PCI_PROBE_ONLY, we *could* completely disregard the bridge
> apertures (except to determine what kind of windows we have) and
> assign them from scratch.  But I don't like that approach because
> we're throwing away any assignment done by the firmware without even
> considering whether it's valid.

Ok, here is the answer I was looking for, you understood my question :)

> I would like /proc/iomem to contain host bridge windows, P2P bridge
> windows, and device BARs.  I think the contents should be identical
> for PCI_PROBE_ONLY and !PCI_PROBE_ONLY unless we actually changed
> something in the !PCI_PROBE_ONLY case.

Understood, I am a bit dubious about FW set-up correctness of some
of the platforms I am dealing with (we have an example here) but that's
not your problem.

> > pci 0000:00:00.0:   bridge window [mem 0x01000000-0x01ffffff]
> >
> > > I see that bus 01 requires 0x204100 of mem space, which must be
> > > rounded up to a megabyte boundary, so the window must be at least 3M
> > > (0x00300000):
> > >
> > >   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]
> > >
> > > I don't understand the connection with dff22d2054b5 yet.  If we don't
> > > call pci_read_bridge_bases(), apparently some assign-resources path
> > > figures out the required size and assigns a 3M window.
> > >
> > > If we *do* call pci_read_bridge_bases(), do we read a bogus 16M window
> > > size, fail to assign that because the host controller window isn't big
> > > enough, and then the assign-resources path just gives up?  I assume
> > > clearing r->flags in your patch is the critical thing?  Is there
> > > something in assign-resources that checks for r->flags == 0?
> >
> > You summed it up, but the point here is not about the flags, it is
> > about the bogus 16M bridge aperture (so r->start and r->end).
> >
> > While sizing the bridge apertures, the code in pbus_size_mem() checks
> > the size required by devices and then set-up the bridge aperture.
> >
> > Now, calculate_memsize() takes as an input the "old" aperture size (16M)
> > which means that the updated aperture will keep the old aperture
> > size instead of the one computed from the size of downstream devices
> > (because the old aperture - read from bridge bases - is larger, see
> > calculate_memsize()), hence the failure.
> 
> I don't see the point of sizing a bridge at all *unless* we find that
> we need to reassign one of its windows.  If firmware gave us a working
> assignment, we should read the bridge windows, claim them, read the
> BARs of downstream devices, claim them, and be done.  If firmware
> didn't give us a working assignment (as in this case), we should read
> the window, attempt to claim it, fail, *then* figure out how big the
> window needs to be to accommodate all the downstream BARs.  In that
> case, the original window size is irrelevant.
> 
> So I'm dubious about the idea that calculate_memsize() should keep the
> old size if it is larger.

I did not say it should, I said this is what's happening and I would
like to understand if that's the behaviour we should expect.

> > If the bridge aperture is reset (ie resource start and end are zeroed)
> > before sizing the bridge everything is back to normal.
> >
> > x86 does the same thing I implement in the patch attached, and probably
> > we have also discovered why Alpha and MIPS were reading bridge bases
> > on PROBE_ONLY systems only.
> >
> > > I think it would be ideal if we could someday claim the resource
> > > immediately, as soon as we read it from a BAR or bridge window, and
> > > mark it as IORESOURCE_UNSET if claiming it fails.  Then if the
> > > platform set up reasonable windows, we could use them; if it didn't,
> > > we could just assign our own.
> >
> > Well, that's what my patch does and that's what x86 does. I am nervous
> > about adding this to core PCI code (in particular I am worried about
> > claiming the bridge windows ie when you say reasonable, it does not
> > necessarily mean optimal, claiming the bridge apertures can cause
> > issues in relation to resources allocation IMO since we claim
> > the aperture before sizing the bridge).
> 
> I think we should claim the resource immediately so the resource tree
> reflects what the hardware is doing.  If we have to reassign things,
> we can release the original assignment and claim the new one.
> 
> I agree we're going to trip over issues.  But I think those issues are
> symptoms of things we're doing wrong, so I think we should find and
> fix them instead of tip-toeing around them.  We might need short-term
> workarounds, and I'm OK with that as long we try not to think of them
> as the real fixes.

I agree with you, I just wanted to fix this regression promptly without
reverting the patch that triggered it because from what you are saying
above, reading the bridge bases from core code should be there to stay.
I need some time to understand how to claim resources safely especially
if the resulting code has to live in PCI core, in the interim to fix the
regression I need a temporary fix for ARM.

> > My question is: on !PCI_PROBE_ONLY systems, why do we want to "trust" the
> > bridge bases (that we want to reassign after sizing bridges *anyway*) ?
> > I understand on PCI_PROBE_ONLY systems they should be immutable, I would
> > like to understand why we have to read them on !PCI_PROBE_ONLY systems.
> 
> For the normal case (!PCI_PROBE_ONLY), we've historically used the
> existing window assignments if they work.  We only reassign windows if
> there's a reason why we have to, e.g., some device has no resources
> but we can give it some by rearranging things.  I think it makes sense
> to continue that practice -- why would we change something that is
> already working?

I did not say we should change it, it was a question for me to
understand. On ARM resources are not claimed, they are always
assigned or we leave them as they are (ie on PCI_PROBE_ONLY).

Reading the bridge apertures in core code is triggering this regression,
so, even if temporary, I have to find a solution which might be the patch
I sent or something more sophisticated, I am working on it.

Thank you,
Lorenzo

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

[Lorenzo Pieralisi]

Bjorn, Yinghai,

On Thu, Sep 03, 2015 at 05:21:40PM +0100, Bjorn Helgaas wrote:
> [+cc Yinghai]
> 
> On Thu, Sep 03, 2015 at 11:01:29AM +0100, Lorenzo Pieralisi wrote:
> > Hi Bjorn,
> >
> > On Wed, Sep 02, 2015 at 09:32:50PM +0100, Bjorn Helgaas wrote:
> > > Hi Lorenzo, thanks for jumping on this!
> >
> > :) I really want to get to the bottom of this resource allocation
> > issue.
> >
> > > On Wed, Sep 02, 2015 at 06:47:16PM +0100, Lorenzo Pieralisi wrote:
> > > > Hi Hannes,
> > > >
> > > > On Wed, Sep 02, 2015 at 10:51:18AM +0100, oe5hpm wrote:
> > > > > Hi Lorenzo,
> > > > >
> > > > > today i tried to boot up the most recent vanilla kernel on my
> > > > > Freescale i.mx6 board.
> > > > > I ran into trouble regarding PCI enumeration.
> > > > >
> > > > > [    0.431949] imx6q-pcie 1ffc000.pcie: PCI host bridge to bus 0000:00
> > > > > [    0.431976] pci_bus 0000:00: root bus resource [io  0x1000-0xffff]
> > > > > [    0.431996] pci_bus 0000:00: root bus resource [mem 0x01000000-0x01efffff]
> > > > > [    0.432022] pci_bus 0000:00: root bus resource [bus 00-ff]
> > > > > [    0.433271] PCI: bus0: Fast back to back transfers disabled
> > > > > [    0.433629] pci 0000:00:00.0: PCI bridge to [bus 01-ff]
> > > > > [    0.435181] PCI: bus1: Fast back to back transfers disabled
> > > > > [    0.435564] pci 0000:00:00.0: BAR 0: assigned [mem 0x01000000-0x010fffff]
> > > > > [    0.435593] pci 0000:00:00.0: BAR 8: no space for [mem size 0x01000000]
> > > > > [    0.435613] pci 0000:00:00.0: BAR 8: failed to assign [mem size 0x01000000]
> > > > > [    0.435635] pci 0000:00:00.0: BAR 9: assigned [mem
> > > > > 0x01100000-0x011fffff pref]
> > > > > [    0.435655] pci 0000:00:00.0: BAR 6: assigned [mem
> > > > > 0x01200000-0x0120ffff pref]
> > > > > [    0.435676] pci 0000:00:00.0: BAR 7: assigned [io  0x1000-0x1fff]
> > > > > [    0.435705] pci 0000:01:00.0: BAR 2: no space for [mem size 0x00200000]
> > > > > [    0.435722] pci 0000:01:00.0: BAR 2: failed to assign [mem size 0x00200000]
> > > > > [    0.435739] pci 0000:01:00.0: BAR 1: no space for [mem size 0x00004000]
> > > > > [    0.435754] pci 0000:01:00.0: BAR 1: failed to assign [mem size 0x00004000]
> > > > > [    0.435770] pci 0000:01:00.0: BAR 0: no space for [mem size 0x00000100]
> > > > > [    0.435786] pci 0000:01:00.0: BAR 0: failed to assign [mem size 0x00000100]
> > > > > [    0.435804] pci 0000:00:00.0: PCI bridge to [bus 01]
> > > > > [    0.435826] pci 0000:00:00.0:   bridge window [io  0x1000-0x1fff]
> > > > > [    0.435855] pci 0000:00:00.0:   bridge window [mem
> > > > > 0x01100000-0x011fffff pref]
> > > > >
> > > > > there are several fails assigning memory ressources to pci-devices.
> > > > >
> > > > > i bisect down this trouble to commit id:
> > > > > dff22d2054b5dbb1889f20c03959dd0c494fab8c : PCI: Call
> > > > > pci_read_bridge_bases() from core instead of arch code
> > > > >
> > > > > For testing purpose i've reverted this commit on a local branch and
> > > > > everythings works fine, as before.
> > > > >
> > > > > [    0.431976] imx6q-pcie 1ffc000.pcie: PCI host bridge to bus 0000:00
> > > > > [    0.432004] pci_bus 0000:00: root bus resource [io  0x1000-0xffff]
> > > > > [    0.432023] pci_bus 0000:00: root bus resource [mem 0x01000000-0x01efffff]
> > > > > [    0.432047] pci_bus 0000:00: root bus resource [bus 00-ff]
> > > > > [    0.433302] PCI: bus0: Fast back to back transfers disabled
> > > > > [    0.435122] PCI: bus1: Fast back to back transfers disabled
> > > > > [    0.435504] pci 0000:00:00.0: BAR 0: assigned [mem 0x01000000-0x010fffff]
> > > > > [    0.435535] pci 0000:00:00.0: BAR 8: assigned [mem 0x01100000-0x013fffff]
> > > > > [    0.435557] pci 0000:00:00.0: BAR 6: assigned [mem
> > > > > 0x01400000-0x0140ffff pref]
> > > > > [    0.435585] pci 0000:01:00.0: BAR 2: assigned [mem 0x01200000-0x013fffff]
> > > > > [    0.435626] pci 0000:01:00.0: BAR 1: assigned [mem 0x01100000-0x01103fff]
> > > > > [    0.435665] pci 0000:01:00.0: BAR 0: assigned [mem 0x01104000-0x011040ff]
> > > > > [    0.435703] pci 0000:00:00.0: PCI bridge to [bus 01]
> > > > > [    0.435728] pci 0000:00:00.0:   bridge window [mem 0x01100000-0x013fffff]
> > > > >
> > > > > Further i can break down the failure to "drivers/pci/probe.c" line #924.
> > > > > If i comment out the "pci_read_bridge_bases(child);" also everything works well.
> > > > >
> > > > > I have to confess, that my knowledge about the whole PCI thing in the
> > > > > kernel is not very deep, so it is not possible for me to figure out
> > > > > what is going wrong.
> > > >
> > > > It looks like a bogus bridge aperture size is causing this to happen,
> > > > and this prevents reassignment on arm (bridge aperture is too big),
> > > > which proves that reading the bridge bases without vetting the corresponding
> > > > resources may break (on platforms that were not reading them before).
> > > >
> > > > arm was the only platform not reading the bridge bases, here is an
> > > > answer why. So, to prevent reverting the commit I put together this
> > > > patch (to be reworked if we deem it reasonable), subject to discussion
> > > > (I fear it may end up breaking other arm platforms, I do not have all
> > > > ARM boards and required host controllers to test, I managed to test it on
> > > > an iMX6 Sabrelite though).
> > > >
> > > > Here, please let me know if it works for you, I will keep on thinking
> > > > to find the best solution.
> > > >
> > > > I will have to do this for arm64 too, comments very welcome.
> > > >
> > > > Thanks,
> > > > Lorenzo
> > > >
> > > > -- >8 --
> > > > Subject: [PATCH] arm: kernel: pci: fixup erroneous PCI bridge apertures
> > > >
> > > > Bridge apertures read by core PCI code through pci_read_bridge_bases()
> > > > might be erroneous (bogus platform setup). If the arch code does not vet
> > > > the bridge resources (ie by trying to claim them), we can end up in a
> > > > situation where wrong bridge apertures can prevent resources assignment
> > > > for downstream devices causing enumeration failures (eg a bridge
> > > > aperture does not fit in the respective host controller resource window,
> > > > so it can't be assigned).
> > > >
> > > > This patch adds arm arch code that vets bridge resources by trying
> > > > to claim them, and reset them on claiming failure so that they can
> > > > be properly reassigned.
> > >
> > > We definitely should not depend on the platform to set up the bridge
> > > windows.  Do we know what the platform left in the 00:00.0 window
> > > registers?
> >
> > Well, I agree but the point here is, by reading the bridge bases
> > we are initializing the apertures resources and this is causing
> > issues, we have to have a way to nuke the initialized apertures resources
> > if they are bogus, more below. I wonder why we want to read the bridge
> > apertures at all on !PCI_PROBE_ONLY systems.
> 
> I'm not quite sure I understand your question.  We have to know the
> bridge apertures to know whether downstream device BARs are valid.
> For PCI_PROBE_ONLY, that means reading the apertures, since we won't
> assign them ourselves.
> 
> For !PCI_PROBE_ONLY, we *could* completely disregard the bridge
> apertures (except to determine what kind of windows we have) and
> assign them from scratch.  But I don't like that approach because
> we're throwing away any assignment done by the firmware without even
> considering whether it's valid.
> 
> I would like /proc/iomem to contain host bridge windows, P2P bridge
> windows, and device BARs.  I think the contents should be identical
> for PCI_PROBE_ONLY and !PCI_PROBE_ONLY unless we actually changed
> something in the !PCI_PROBE_ONLY case.
> 
> > pci 0000:00:00.0:   bridge window [mem 0x01000000-0x01ffffff]
> >
> > > I see that bus 01 requires 0x204100 of mem space, which must be
> > > rounded up to a megabyte boundary, so the window must be at least 3M
> > > (0x00300000):
> > >
> > >   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]
> > >
> > > I don't understand the connection with dff22d2054b5 yet.  If we don't
> > > call pci_read_bridge_bases(), apparently some assign-resources path
> > > figures out the required size and assigns a 3M window.
> > >
> > > If we *do* call pci_read_bridge_bases(), do we read a bogus 16M window
> > > size, fail to assign that because the host controller window isn't big
> > > enough, and then the assign-resources path just gives up?  I assume
> > > clearing r->flags in your patch is the critical thing?  Is there
> > > something in assign-resources that checks for r->flags == 0?
> >
> > You summed it up, but the point here is not about the flags, it is
> > about the bogus 16M bridge aperture (so r->start and r->end).
> >
> > While sizing the bridge apertures, the code in pbus_size_mem() checks
> > the size required by devices and then set-up the bridge aperture.
> >
> > Now, calculate_memsize() takes as an input the "old" aperture size (16M)
> > which means that the updated aperture will keep the old aperture
> > size instead of the one computed from the size of downstream devices
> > (because the old aperture - read from bridge bases - is larger, see
> > calculate_memsize()), hence the failure.
> 
> I don't see the point of sizing a bridge at all *unless* we find that
> we need to reassign one of its windows.  If firmware gave us a working
> assignment, we should read the bridge windows, claim them, read the
> BARs of downstream devices, claim them, and be done.  If firmware
> didn't give us a working assignment (as in this case), we should read
> the window, attempt to claim it, fail, *then* figure out how big the
> window needs to be to accommodate all the downstream BARs.  In that
> case, the original window size is irrelevant.
> 
> So I'm dubious about the idea that calculate_memsize() should keep the
> old size if it is larger.

I had a look at this, and bumped into this old thread:

http://comments.gmane.org/gmane.linux.kernel.pci/33672

where Yinghai points at commit d65245c3297ac63abc51a976d92f,
that adds code that prevents bridge apertures from shrinking.

By keeping the bridge old size while sizing the bridge if the new size
is < old size (ie bridge never shrinks), we implicitly assume that upon
first scan of the bridge apertures that are not claimed should be set to
size = 0 (ie as Bjorn said the size should be irrelevant in that case
because we are sizing the bridge to figure out what the size should be),
otherwise we might end up triggering issues as this thread shows because
the old size is always taken into consideration.

It seems that if we can't validate the bridge apertures (by claiming
them or we want to reassign everything and ignore the FW set-up) we
have to reset the values in the bridge aperture resources read from
HW so that the resource size is actually ignored when the bridge is
first sized.

Comments welcome, thanks in advance.

Lorenzo

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

[Yinghai Lu]

On Fri, Sep 4, 2015 at 7:19 AM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:

> By keeping the bridge old size while sizing the bridge if the new size
> is < old size (ie bridge never shrinks), we implicitly assume that upon
> first scan of the bridge apertures that are not claimed should be set to
> size = 0 (ie as Bjorn said the size should be irrelevant in that case
> because we are sizing the bridge to figure out what the size should be),
> otherwise we might end up triggering issues as this thread shows because
> the old size is always taken into consideration.

oh, we have patches that will ignore old_size in last retry.

Please check

git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git
for-pci-v4.3-next

exact patches should be:
https://git.kernel.org/cgit/linux/kernel/git/yinghai/linux-yinghai.git/patch/?id=e8539db0a669fc076ac50800e274af165ce3c5fe
PCI: Treat optional as required in first try for bridge rescan

https://git.kernel.org/cgit/linux/kernel/git/yinghai/linux-yinghai.git/patch/?id=a7a2e984194a051e77b88f59e2db030dd4d99e64
PCI: Get new realloc size for bridge for last try

Thanks

Yinghai

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

[Lorenzo Pieralisi]

On Fri, Sep 04, 2015 at 05:00:35PM +0100, Yinghai Lu wrote:
> On Fri, Sep 4, 2015 at 7:19 AM, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> 
> > By keeping the bridge old size while sizing the bridge if the new size
> > is < old size (ie bridge never shrinks), we implicitly assume that upon
> > first scan of the bridge apertures that are not claimed should be set to
> > size = 0 (ie as Bjorn said the size should be irrelevant in that case
> > because we are sizing the bridge to figure out what the size should be),
> > otherwise we might end up triggering issues as this thread shows because
> > the old size is always taken into consideration.
> 
> oh, we have patches that will ignore old_size in last retry.

The problem here is not the last retry, it is the first bridge scan.

By moving pci_read_bridge_bases() to core PCI code, if we do not
vet the bridge apertures (ie claim them and reset them if the claiming
fails) we end up calling (on ARM) __pci_bus_size_bridges() with apertures
that can have sizes != 0, which does not make any sense since we are calling
__pci_bus_size_bridges() to *discover* what the aperture size should
be on first bridge scan, correct ?

__pci_bus_size_bridges() expects the "old_size" to be set to a sensible
value, and it seems to me that it should be 0 on first scan given
the current __pci_bus_size_bridges() implementation and my question
is where the bridge aperture resources must be reset for systems
(eg ARM) that reassign the whole PCI resource space.

The platform triggering this issue was working without any need for
realloc before, so your patches are indeed useful but I would like
to understand what you think about the issue described above.

Thanks you !
Lorenzo

> 
> Please check
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git
> for-pci-v4.3-next
> 
> exact patches should be:
> https://git.kernel.org/cgit/linux/kernel/git/yinghai/linux-yinghai.git/patch/?id=e8539db0a669fc076ac50800e274af165ce3c5fe
> PCI: Treat optional as required in first try for bridge rescan
> 
> https://git.kernel.org/cgit/linux/kernel/git/yinghai/linux-yinghai.git/patch/?id=a7a2e984194a051e77b88f59e2db030dd4d99e64
> PCI: Get new realloc size for bridge for last try
> 
> Thanks
> 
> Yinghai
> 

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

[Yinghai Lu]

On Fri, Sep 4, 2015 at 9:44 AM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> On Fri, Sep 04, 2015 at 05:00:35PM +0100, Yinghai Lu wrote:
>
> The problem here is not the last retry, it is the first bridge scan.
>
> By moving pci_read_bridge_bases() to core PCI code, if we do not
> vet the bridge apertures (ie claim them and reset them if the claiming
> fails) we end up calling (on ARM) __pci_bus_size_bridges() with apertures
> that can have sizes != 0, which does not make any sense since we are calling
> __pci_bus_size_bridges() to *discover* what the aperture size should
> be on first bridge scan, correct ?

for x86, in pcibios_allocate_bridge_resources(), we do validate
the bridge resources, and reset size to 1 (strange ?!).
and they are called before pci_asssign_unassigned_resources()

so arch ARM would support pcibios_allocate_bridge_resources or other
call to do the same thing?

wonder some arches even claim fails, they still does not want you to
reset it.

Thanks

Yinghai

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

[Lorenzo Pieralisi]

On Sat, Sep 05, 2015 at 12:53:48AM +0100, Yinghai Lu wrote:
> On Fri, Sep 4, 2015 at 9:44 AM, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> > On Fri, Sep 04, 2015 at 05:00:35PM +0100, Yinghai Lu wrote:
> >
> > The problem here is not the last retry, it is the first bridge scan.
> >
> > By moving pci_read_bridge_bases() to core PCI code, if we do not
> > vet the bridge apertures (ie claim them and reset them if the claiming
> > fails) we end up calling (on ARM) __pci_bus_size_bridges() with apertures
> > that can have sizes != 0, which does not make any sense since we are calling
> > __pci_bus_size_bridges() to *discover* what the aperture size should
> > be on first bridge scan, correct ?
> 
> for x86, in pcibios_allocate_bridge_resources(), we do validate
> the bridge resources, and reset size to 1 (strange ?!).

Yes, strange, but there is even a special case in calculate_memsize()
to handle that :) it seems !

> and they are called before pci_asssign_unassigned_resources()
> 
> so arch ARM would support pcibios_allocate_bridge_resources or other
> call to do the same thing?

I could do that, but:

1) Bjorn does not like this approach (ie it has nothing arch specific
   in it - actually zeroing the bridge size on first scan seems to be an
   implicit requirement of __pci_bus_size_bridges() and that's not
   documented)
2) I still do not understand why on first bridge scan we should care
   about the old bridge size. If the bridge windows are claimed
   __pci_bus_size_bridges() ignore them (and that's right). If they
   are not claimed (ie they are free) why, on first scan, would the old
   size matter ? I really do not like the implicit requirement that
   forces the bridge aperture size to be 0 (or 1 ;-)) on first scan,
   we end up zeroing it in arch code where it should not really matter.
   Is there a reason why old size matters on first bridge scan ? I guess
   it has to do with hotplug, but I need your input on this.
   I agree we have to find a way to detect the *first* scan (there are
   various ways of doing that - possibly a "pass" parameter or we can
   rely on resource flags to detect that), question is if we should.

> wonder some arches even claim fails, they still does not want you to
> reset it.

Yes, that's what I noticed too, I have no idea how they work, but IMO
they should be patched too (ia64 is an example). If the bridge size
read from pci_read_bridge_bases() is erroneous its size is kept even
when we try to reassign it (as this regression showed) so I guess
on those archs this bug just does not trigger because the bridge
apertures are programmed in FW in a *saner* way.

On platforms where we want to reassign everything the current approch
just does not make sense (ie keeping the old size on first bridge scan)
but please shout if there is a reason for that, I would like to put
together a fix asap.

Thank you !
Lorenzo

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

[Lorenzo Pieralisi]

Yinghai,

On Mon, Sep 07, 2015 at 10:12:30AM +0100, Lorenzo Pieralisi wrote:
> On Sat, Sep 05, 2015 at 12:53:48AM +0100, Yinghai Lu wrote:
> > On Fri, Sep 4, 2015 at 9:44 AM, Lorenzo Pieralisi
> > <lorenzo.pieralisi@arm.com> wrote:
> > > On Fri, Sep 04, 2015 at 05:00:35PM +0100, Yinghai Lu wrote:
> > >
> > > The problem here is not the last retry, it is the first bridge scan.
> > >
> > > By moving pci_read_bridge_bases() to core PCI code, if we do not
> > > vet the bridge apertures (ie claim them and reset them if the claiming
> > > fails) we end up calling (on ARM) __pci_bus_size_bridges() with apertures
> > > that can have sizes != 0, which does not make any sense since we are calling
> > > __pci_bus_size_bridges() to *discover* what the aperture size should
> > > be on first bridge scan, correct ?
> > 
> > for x86, in pcibios_allocate_bridge_resources(), we do validate
> > the bridge resources, and reset size to 1 (strange ?!).
> 
> Yes, strange, but there is even a special case in calculate_memsize()
> to handle that :) it seems !

Any thoughts on that ? I would like to understand if there is any
reason why on first scan the bridge aperture size is relevant, whereas
in my opinion it should not be, I need your input on that.

Put it differently, I do not see any reason why arch code should
reset the bridge resource on platforms that reassign the whole
PCI config space, if you see any please mention it here.

As I said, ARM is not the only platform affected, MIPS and possibly
Alpha suffer from the same issue and I think it should be fixed
in core code (I think __pci_bus_size_bridges() should ignore the
old aperture size on first scan).

Thanks !
Lorenzo

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

[Yinghai Lu]

On Mon, Sep 14, 2015 at 3:09 AM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> As I said, ARM is not the only platform affected, MIPS and possibly
> Alpha suffer from the same issue and I think it should be fixed
> in core code (I think __pci_bus_size_bridges() should ignore the
> old aperture size on first scan).

We could just revert
dff22d2054b5 (" PCI: Call pci_read_bridge_bases() from core instead of
arch code")
instead.

Yinghai

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

[Lorenzo Pieralisi]

On Mon, Sep 14, 2015 at 05:05:50PM +0100, Yinghai Lu wrote:
> On Mon, Sep 14, 2015 at 3:09 AM, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> > As I said, ARM is not the only platform affected, MIPS and possibly
> > Alpha suffer from the same issue and I think it should be fixed
> > in core code (I think __pci_bus_size_bridges() should ignore the
> > old aperture size on first scan).
> 
> We could just revert
> dff22d2054b5 (" PCI: Call pci_read_bridge_bases() from core instead of
> arch code")
> instead.

Sure, but still that does not explain why the old bridge aperture size
matters while sizing the bridge aperture on first bridge scan, I
still have no answer for that and I would like to get one.

As far as I understand we could just rework:

commit d65245c3297ac63abc51a976d9 ("PCI: don't shrink bridge resources")

to make sure that old size is taken into account only when the core
code tries to reallocate resources, I think that dff22d2054b5 unearthed
a problem instead of creating one but I am open to suggestions.

Thanks,
Lorenzo

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

[Yinghai Lu]

On Mon, Sep 14, 2015 at 9:28 AM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> On Mon, Sep 14, 2015 at 05:05:50PM +0100, Yinghai Lu wrote:
>> We could just revert
>> dff22d2054b5 (" PCI: Call pci_read_bridge_bases() from core instead of
>> arch code")
>> instead.
>
> Sure, but still that does not explain why the old bridge aperture size
> matters while sizing the bridge aperture on first bridge scan, I
> still have no answer for that and I would like to get one.
>
> As far as I understand we could just rework:
>
> commit d65245c3297ac63abc51a976d9 ("PCI: don't shrink bridge resources")
>
> to make sure that old size is taken into account only when the core
> code tries to reallocate resources, I think that dff22d2054b5 unearthed
> a problem instead of creating one but I am open to suggestions.

if arch code called pci_read_bridge_bases() via pcibios_fixup_bus(),
then it need to have
to call pcibios_allocate_bus_resources() later.

but now arm (mips ?)  does not have calling pcibios_allocate_bus_resources().

Yinghai

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

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 2765 bytes --]

On Mon, Sep 14, 2015 at 10:36 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Mon, Sep 14, 2015 at 9:28 AM, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
>> On Mon, Sep 14, 2015 at 05:05:50PM +0100, Yinghai Lu wrote:
>>> We could just revert
>>> dff22d2054b5 (" PCI: Call pci_read_bridge_bases() from core instead of
>>> arch code")
>>> instead.
>>

> if arch code called pci_read_bridge_bases() via pcibios_fixup_bus(),
> then it need to have
> to call pcibios_allocate_bus_resources() later.
>
> but now arm (mips ?)  does not have calling pcibios_allocate_bus_resources().

Found other problem that is caused by
dff22d2054b5 (" PCI: Call pci_read_bridge_bases() from core instead of
arch code")

If that commit does not get reverted, will need to have attached patch


Subject: [PATCH] PCI: Skip reading bridge bases with existing ones on rescan

Noticed there is noisy print out pci bridge bases read out for
existing devices that is caused by commit dff22d2054b5
("PCI: Call pci_read_bridge_bases() from core instead of arch code")

pci_bus 0000:06: scanning bus
pcieport 0000:06:00.0: scanning [bus 07-09] behind bridge, pass 0
pcieport 0000:06:00.0: PCI bridge to [bus 07-09]
pcieport 0000:06:00.0:   bridge window [io  0x1000-0x2fff]
pcieport 0000:06:00.0:   bridge window [mem 0xfda00000-0xfddfffff]
pcieport 0000:06:00.0:   bridge window [mem 0xfc000000-0xfc3fffff 64bit pref]
pci_bus 0000:07: scanning bus
pcieport 0000:07:00.0: scanning [bus 08-08] behind bridge, pass 0
pcieport 0000:07:00.0: PCI bridge to [bus 08]
pcieport 0000:07:00.0:   bridge window [io  0x2000-0x2fff]
pcieport 0000:07:00.0:   bridge window [mem 0xfdc00000-0xfddfffff]
pcieport 0000:07:00.0:   bridge window [mem 0xfc200000-0xfc3fffff 64bit pref]
pci_bus 0000:08: scanning bus
pcieport 0000:07:00.1: scanning [bus 09-09] behind bridge, pass 0
pcieport 0000:07:00.1: PCI bridge to [bus 09]
pcieport 0000:07:00.1:   bridge window [io  0x1000-0x1fff]
pcieport 0000:07:00.1:   bridge window [mem 0xfda00000-0xfdbfffff]
pcieport 0000:07:00.1:   bridge window [mem 0xfc000000-0xfc1fffff 64bit pref]

Add is_added checking to avoid those wrong reading.

Fixes: dff22d2054b5 ("PCI: Call pci_read_bridge_bases() from core
instead of arch code")
Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/pci/probe.c |    3 +++
 1 file changed, 3 insertions(+)

Index: linux-2.6/drivers/pci/probe.c
===================================================================
--- linux-2.6.orig/drivers/pci/probe.c
+++ linux-2.6/drivers/pci/probe.c
@@ -455,6 +455,9 @@ void pci_read_bridge_bases(struct pci_bu
     struct resource *res;
     int i;

+    if (child->is_added)
+        return;
+
     if (pci_is_root_bus(child))    /* It's a host bus, nothing to read */
         return;

[-- Attachment #2: no_pci_read_bridge_on_rescan.patch --]
[-- Type: text/x-patch, Size: 1960 bytes --]

Subject: [PATCH] PCI: Skip reading bridge bases with existing ones on rescan

Noticed there is noisy print out pci bridge bases read out for
existing devices that is caused by commit dff22d2054b5
("PCI: Call pci_read_bridge_bases() from core instead of arch code")

pci_bus 0000:06: scanning bus
pcieport 0000:06:00.0: scanning [bus 07-09] behind bridge, pass 0
pcieport 0000:06:00.0: PCI bridge to [bus 07-09]
pcieport 0000:06:00.0:   bridge window [io  0x1000-0x2fff]
pcieport 0000:06:00.0:   bridge window [mem 0xfda00000-0xfddfffff]
pcieport 0000:06:00.0:   bridge window [mem 0xfc000000-0xfc3fffff 64bit pref]
pci_bus 0000:07: scanning bus
pcieport 0000:07:00.0: scanning [bus 08-08] behind bridge, pass 0
pcieport 0000:07:00.0: PCI bridge to [bus 08]
pcieport 0000:07:00.0:   bridge window [io  0x2000-0x2fff]
pcieport 0000:07:00.0:   bridge window [mem 0xfdc00000-0xfddfffff]
pcieport 0000:07:00.0:   bridge window [mem 0xfc200000-0xfc3fffff 64bit pref]
pci_bus 0000:08: scanning bus
pcieport 0000:07:00.1: scanning [bus 09-09] behind bridge, pass 0
pcieport 0000:07:00.1: PCI bridge to [bus 09]
pcieport 0000:07:00.1:   bridge window [io  0x1000-0x1fff]
pcieport 0000:07:00.1:   bridge window [mem 0xfda00000-0xfdbfffff]
pcieport 0000:07:00.1:   bridge window [mem 0xfc000000-0xfc1fffff 64bit pref]

Add is_added checking to avoid those wrong reading.

Fixes: dff22d2054b5 ("PCI: Call pci_read_bridge_bases() from core instead of arch code")
Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/pci/probe.c |    3 +++
 1 file changed, 3 insertions(+)

Index: linux-2.6/drivers/pci/probe.c
===================================================================
--- linux-2.6.orig/drivers/pci/probe.c
+++ linux-2.6/drivers/pci/probe.c
@@ -455,6 +455,9 @@ void pci_read_bridge_bases(struct pci_bu
 	struct resource *res;
 	int i;
 
+	if (child->is_added)
+		return;
+
 	if (pci_is_root_bus(child))	/* It's a host bus, nothing to read */
 		return;
 

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

[Lorenzo Pieralisi]

On Tue, Sep 15, 2015 at 12:58:20AM +0100, Yinghai Lu wrote:
> On Mon, Sep 14, 2015 at 10:36 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> > On Mon, Sep 14, 2015 at 9:28 AM, Lorenzo Pieralisi
> > <lorenzo.pieralisi@arm.com> wrote:
> >> On Mon, Sep 14, 2015 at 05:05:50PM +0100, Yinghai Lu wrote:
> >>> We could just revert
> >>> dff22d2054b5 (" PCI: Call pci_read_bridge_bases() from core instead of
> >>> arch code")
> >>> instead.
> >>
> 
> > if arch code called pci_read_bridge_bases() via pcibios_fixup_bus(),
> > then it need to have
> > to call pcibios_allocate_bus_resources() later.
> >
> > but now arm (mips ?)  does not have calling pcibios_allocate_bus_resources().

pcibios_allocate_bus_resources() is an arch specific function and arm
and (and mips ?) does not need to create/call it because ARM reassigns
ALL resources in ALL platforms, hoping FW can provide a reasonable PCI
bridge apertures set-up on ARM is wishful thinking at present.

If PCI core code is written with that assumption (ie that arch code zeroes
the bridge apertures if they can't be claimed), pci_read_bridge_bases()
can't be moved to PCI core code at present, sad and simple.

I already asked many times why __pci_bus_size_bridges() cares about
the old bridge size on first scan and got no answer so I would ask Bjorn
to revert dff22d2054b5 (" PCI: Call pci_read_bridge_bases() from core
instead of arch code") or we apply an ARM specific plaster, we are making
no progress on this.

> Found other problem that is caused by
> dff22d2054b5 (" PCI: Call pci_read_bridge_bases() from core instead of
> arch code")
> 
> If that commit does not get reverted, will need to have attached patch

I see what you mean and I see why there is a reason to apply the patch
below if we do not revert dff22d2054b5 (" PCI: Call pci_read_bridge_bases()
from core instead of arch code"), but I am afraid the commit log has to
be rewritten to explain the problem in a way that properly describes
the issue, and that's not the first one I read in the last couple of
weeks to figure out how to fix this regression.

Thanks,
Lorenzo

> 
> 
> Subject: [PATCH] PCI: Skip reading bridge bases with existing ones on rescan
> 
> Noticed there is noisy print out pci bridge bases read out for
> existing devices that is caused by commit dff22d2054b5
> ("PCI: Call pci_read_bridge_bases() from core instead of arch code")
> 
> pci_bus 0000:06: scanning bus
> pcieport 0000:06:00.0: scanning [bus 07-09] behind bridge, pass 0
> pcieport 0000:06:00.0: PCI bridge to [bus 07-09]
> pcieport 0000:06:00.0:   bridge window [io  0x1000-0x2fff]
> pcieport 0000:06:00.0:   bridge window [mem 0xfda00000-0xfddfffff]
> pcieport 0000:06:00.0:   bridge window [mem 0xfc000000-0xfc3fffff 64bit pref]
> pci_bus 0000:07: scanning bus
> pcieport 0000:07:00.0: scanning [bus 08-08] behind bridge, pass 0
> pcieport 0000:07:00.0: PCI bridge to [bus 08]
> pcieport 0000:07:00.0:   bridge window [io  0x2000-0x2fff]
> pcieport 0000:07:00.0:   bridge window [mem 0xfdc00000-0xfddfffff]
> pcieport 0000:07:00.0:   bridge window [mem 0xfc200000-0xfc3fffff 64bit pref]
> pci_bus 0000:08: scanning bus
> pcieport 0000:07:00.1: scanning [bus 09-09] behind bridge, pass 0
> pcieport 0000:07:00.1: PCI bridge to [bus 09]
> pcieport 0000:07:00.1:   bridge window [io  0x1000-0x1fff]
> pcieport 0000:07:00.1:   bridge window [mem 0xfda00000-0xfdbfffff]
> pcieport 0000:07:00.1:   bridge window [mem 0xfc000000-0xfc1fffff 64bit pref]
> 
> Add is_added checking to avoid those wrong reading.
> 
> Fixes: dff22d2054b5 ("PCI: Call pci_read_bridge_bases() from core
> instead of arch code")
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> 
> ---
>  drivers/pci/probe.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> Index: linux-2.6/drivers/pci/probe.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/probe.c
> +++ linux-2.6/drivers/pci/probe.c
> @@ -455,6 +455,9 @@ void pci_read_bridge_bases(struct pci_bu
>      struct resource *res;
>      int i;
> 
> +    if (child->is_added)
> +        return;
> +
>      if (pci_is_root_bus(child))    /* It's a host bus, nothing to read */
>          return;

> Subject: [PATCH] PCI: Skip reading bridge bases with existing ones on rescan
> 
> Noticed there is noisy print out pci bridge bases read out for
> existing devices that is caused by commit dff22d2054b5
> ("PCI: Call pci_read_bridge_bases() from core instead of arch code")
> 
> pci_bus 0000:06: scanning bus
> pcieport 0000:06:00.0: scanning [bus 07-09] behind bridge, pass 0
> pcieport 0000:06:00.0: PCI bridge to [bus 07-09]
> pcieport 0000:06:00.0:   bridge window [io  0x1000-0x2fff]
> pcieport 0000:06:00.0:   bridge window [mem 0xfda00000-0xfddfffff]
> pcieport 0000:06:00.0:   bridge window [mem 0xfc000000-0xfc3fffff 64bit pref]
> pci_bus 0000:07: scanning bus
> pcieport 0000:07:00.0: scanning [bus 08-08] behind bridge, pass 0
> pcieport 0000:07:00.0: PCI bridge to [bus 08]
> pcieport 0000:07:00.0:   bridge window [io  0x2000-0x2fff]
> pcieport 0000:07:00.0:   bridge window [mem 0xfdc00000-0xfddfffff]
> pcieport 0000:07:00.0:   bridge window [mem 0xfc200000-0xfc3fffff 64bit pref]
> pci_bus 0000:08: scanning bus
> pcieport 0000:07:00.1: scanning [bus 09-09] behind bridge, pass 0
> pcieport 0000:07:00.1: PCI bridge to [bus 09]
> pcieport 0000:07:00.1:   bridge window [io  0x1000-0x1fff]
> pcieport 0000:07:00.1:   bridge window [mem 0xfda00000-0xfdbfffff]
> pcieport 0000:07:00.1:   bridge window [mem 0xfc000000-0xfc1fffff 64bit pref]
> 
> Add is_added checking to avoid those wrong reading.
> 
> Fixes: dff22d2054b5 ("PCI: Call pci_read_bridge_bases() from core instead of arch code")
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> 
> ---
>  drivers/pci/probe.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> Index: linux-2.6/drivers/pci/probe.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/probe.c
> +++ linux-2.6/drivers/pci/probe.c
> @@ -455,6 +455,9 @@ void pci_read_bridge_bases(struct pci_bu
>  	struct resource *res;
>  	int i;
>  
> +	if (child->is_added)
> +		return;
> +
>  	if (pci_is_root_bus(child))	/* It's a host bus, nothing to read */
>  		return;
>  

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

[Bjorn Helgaas]

On Tue, Sep 15, 2015 at 4:46 AM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> On Tue, Sep 15, 2015 at 12:58:20AM +0100, Yinghai Lu wrote:
>> On Mon, Sep 14, 2015 at 10:36 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>> > On Mon, Sep 14, 2015 at 9:28 AM, Lorenzo Pieralisi
>> > <lorenzo.pieralisi@arm.com> wrote:
>> >> On Mon, Sep 14, 2015 at 05:05:50PM +0100, Yinghai Lu wrote:
>> >>> We could just revert
>> >>> dff22d2054b5 (" PCI: Call pci_read_bridge_bases() from core instead of
>> >>> arch code")
>> >>> instead.
>> >>
>>
>> > if arch code called pci_read_bridge_bases() via pcibios_fixup_bus(),
>> > then it need to have
>> > to call pcibios_allocate_bus_resources() later.
>> >
>> > but now arm (mips ?)  does not have calling pcibios_allocate_bus_resources().
>
> pcibios_allocate_bus_resources() is an arch specific function and arm
> and (and mips ?) does not need to create/call it because ARM reassigns
> ALL resources in ALL platforms, hoping FW can provide a reasonable PCI
> bridge apertures set-up on ARM is wishful thinking at present.
>
> If PCI core code is written with that assumption (ie that arch code zeroes
> the bridge apertures if they can't be claimed), pci_read_bridge_bases()
> can't be moved to PCI core code at present, sad and simple.
>
> I already asked many times why __pci_bus_size_bridges() cares about
> the old bridge size on first scan and got no answer so I would ask Bjorn
> to revert dff22d2054b5 (" PCI: Call pci_read_bridge_bases() from core
> instead of arch code") or we apply an ARM specific plaster, we are making
> no progress on this.
>
>> Found other problem that is caused by
>> dff22d2054b5 (" PCI: Call pci_read_bridge_bases() from core instead of
>> arch code")
>>
>> If that commit does not get reverted, will need to have attached patch
>
> I see what you mean and I see why there is a reason to apply the patch
> below if we do not revert dff22d2054b5 (" PCI: Call pci_read_bridge_bases()
> from core instead of arch code"), but I am afraid the commit log has to
> be rewritten to explain the problem in a way that properly describes
> the issue, and that's not the first one I read in the last couple of
> weeks to figure out how to fix this regression.

I'm inclined to revert dff22d2054b5 ("PCI: Call
pci_read_bridge_bases() from core instead of arch code") until we can
figure this out.  I think the idea of moving that work from
arch-specific code to the core is good, but it seems like it leads to
more hacky workarounds than real cleanup right now.  What would break
if we simply reverted it?  Would that reintroduce any problems?

PCI enumeration and resource assignment is currently split, with some
ACPI stuff in the middle:

  - PCI enumeration
  - ACPI resource reservation
  - PCI resource assignment

I think it would make sense to do some PCI resource validation and
assignment during enumeration, but I don't think it's possible as long
as we have that ACPI stuff in the middle.  Sometimes ACPI reports
non-PCI devices in the middle of space we think is available for PCI
(either from _CRS or our legacy "everything after top-of-RAM" idea),
so we can't assign space to PCI devices until we know where those ACPI
things are.

I think this is totally broken; ACPI is logically more fundamental
than PCI, and we should look at all the ACPI resources *before* we
enumerate PCI devices, but there's a lot of ancient history there and
it's a lot of work to change it.

Bjorn

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

[Lorenzo Pieralisi]

On Tue, Sep 15, 2015 at 04:57:07PM +0100, Bjorn Helgaas wrote:
> On Tue, Sep 15, 2015 at 4:46 AM, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> > On Tue, Sep 15, 2015 at 12:58:20AM +0100, Yinghai Lu wrote:
> >> On Mon, Sep 14, 2015 at 10:36 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> >> > On Mon, Sep 14, 2015 at 9:28 AM, Lorenzo Pieralisi
> >> > <lorenzo.pieralisi@arm.com> wrote:
> >> >> On Mon, Sep 14, 2015 at 05:05:50PM +0100, Yinghai Lu wrote:
> >> >>> We could just revert
> >> >>> dff22d2054b5 (" PCI: Call pci_read_bridge_bases() from core instead of
> >> >>> arch code")
> >> >>> instead.
> >> >>
> >>
> >> > if arch code called pci_read_bridge_bases() via pcibios_fixup_bus(),
> >> > then it need to have
> >> > to call pcibios_allocate_bus_resources() later.
> >> >
> >> > but now arm (mips ?)  does not have calling pcibios_allocate_bus_resources().
> >
> > pcibios_allocate_bus_resources() is an arch specific function and arm
> > and (and mips ?) does not need to create/call it because ARM reassigns
> > ALL resources in ALL platforms, hoping FW can provide a reasonable PCI
> > bridge apertures set-up on ARM is wishful thinking at present.
> >
> > If PCI core code is written with that assumption (ie that arch code zeroes
> > the bridge apertures if they can't be claimed), pci_read_bridge_bases()
> > can't be moved to PCI core code at present, sad and simple.
> >
> > I already asked many times why __pci_bus_size_bridges() cares about
> > the old bridge size on first scan and got no answer so I would ask Bjorn
> > to revert dff22d2054b5 (" PCI: Call pci_read_bridge_bases() from core
> > instead of arch code") or we apply an ARM specific plaster, we are making
> > no progress on this.
> >
> >> Found other problem that is caused by
> >> dff22d2054b5 (" PCI: Call pci_read_bridge_bases() from core instead of
> >> arch code")
> >>
> >> If that commit does not get reverted, will need to have attached patch
> >
> > I see what you mean and I see why there is a reason to apply the patch
> > below if we do not revert dff22d2054b5 (" PCI: Call pci_read_bridge_bases()
> > from core instead of arch code"), but I am afraid the commit log has to
> > be rewritten to explain the problem in a way that properly describes
> > the issue, and that's not the first one I read in the last couple of
> > weeks to figure out how to fix this regression.
> 
> I'm inclined to revert dff22d2054b5 ("PCI: Call
> pci_read_bridge_bases() from core instead of arch code") until we can
> figure this out.  I think the idea of moving that work from
> arch-specific code to the core is good, but it seems like it leads to
> more hacky workarounds than real cleanup right now.  What would break
> if we simply reverted it?  Would that reintroduce any problems?

None that I am aware of, I know Guenter required it for this series:

https://lkml.org/lkml/2015/7/30/861

but it was not merged so, as far as I understand, reverting the patch
should get things to normal. I think it unearthed a couple of niggles
in core code though that should be changed regardless (eg I still
think that __pci_bus_size_bridges() taking the "old" bridge aperture
size for granted is wrong and that's the only reason why bridge apertures
can only be read in arch code, that claims them and reset them if the
claiming fail).

> PCI enumeration and resource assignment is currently split, with some
> ACPI stuff in the middle:
> 
>   - PCI enumeration
>   - ACPI resource reservation
>   - PCI resource assignment
> 
> I think it would make sense to do some PCI resource validation and
> assignment during enumeration, but I don't think it's possible as long
> as we have that ACPI stuff in the middle.  Sometimes ACPI reports
> non-PCI devices in the middle of space we think is available for PCI
> (either from _CRS or our legacy "everything after top-of-RAM" idea),
> so we can't assign space to PCI devices until we know where those ACPI
> things are.

I think there are too many hidden assumptions in the code that assigns
resources owing to history and they are really difficult to untangle,
that's certain and that's why the resource validation and assignment,
that should be arch-agnostic, is very arch dependent at present.

Reading bridge bases in core code, which should be harmless, triggered
a couple of regressions already, imagine what happens if we started
claiming resources in core PCI code.

Resource validation can be done in core code IMO, it will take time
to figure it out though but it is something worth doing.

Thank you !
Lorenzo

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

[Guenter Roeck]

On 09/15/2015 09:30 AM, Lorenzo Pieralisi wrote:
> On Tue, Sep 15, 2015 at 04:57:07PM +0100, Bjorn Helgaas wrote:
>> On Tue, Sep 15, 2015 at 4:46 AM, Lorenzo Pieralisi
>> <lorenzo.pieralisi@arm.com> wrote:
>>> On Tue, Sep 15, 2015 at 12:58:20AM +0100, Yinghai Lu wrote:
>>>> On Mon, Sep 14, 2015 at 10:36 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>>>>> On Mon, Sep 14, 2015 at 9:28 AM, Lorenzo Pieralisi
>>>>> <lorenzo.pieralisi@arm.com> wrote:
>>>>>> On Mon, Sep 14, 2015 at 05:05:50PM +0100, Yinghai Lu wrote:
>>>>>>> We could just revert
>>>>>>> dff22d2054b5 (" PCI: Call pci_read_bridge_bases() from core instead of
>>>>>>> arch code")
>>>>>>> instead.
>>>>>>
>>>>
>>>>> if arch code called pci_read_bridge_bases() via pcibios_fixup_bus(),
>>>>> then it need to have
>>>>> to call pcibios_allocate_bus_resources() later.
>>>>>
>>>>> but now arm (mips ?)  does not have calling pcibios_allocate_bus_resources().
>>>
>>> pcibios_allocate_bus_resources() is an arch specific function and arm
>>> and (and mips ?) does not need to create/call it because ARM reassigns
>>> ALL resources in ALL platforms, hoping FW can provide a reasonable PCI
>>> bridge apertures set-up on ARM is wishful thinking at present.
>>>
>>> If PCI core code is written with that assumption (ie that arch code zeroes
>>> the bridge apertures if they can't be claimed), pci_read_bridge_bases()
>>> can't be moved to PCI core code at present, sad and simple.
>>>
>>> I already asked many times why __pci_bus_size_bridges() cares about
>>> the old bridge size on first scan and got no answer so I would ask Bjorn
>>> to revert dff22d2054b5 (" PCI: Call pci_read_bridge_bases() from core
>>> instead of arch code") or we apply an ARM specific plaster, we are making
>>> no progress on this.
>>>
>>>> Found other problem that is caused by
>>>> dff22d2054b5 (" PCI: Call pci_read_bridge_bases() from core instead of
>>>> arch code")
>>>>
>>>> If that commit does not get reverted, will need to have attached patch
>>>
>>> I see what you mean and I see why there is a reason to apply the patch
>>> below if we do not revert dff22d2054b5 (" PCI: Call pci_read_bridge_bases()
>>> from core instead of arch code"), but I am afraid the commit log has to
>>> be rewritten to explain the problem in a way that properly describes
>>> the issue, and that's not the first one I read in the last couple of
>>> weeks to figure out how to fix this regression.
>>
>> I'm inclined to revert dff22d2054b5 ("PCI: Call
>> pci_read_bridge_bases() from core instead of arch code") until we can
>> figure this out.  I think the idea of moving that work from
>> arch-specific code to the core is good, but it seems like it leads to
>> more hacky workarounds than real cleanup right now.  What would break
>> if we simply reverted it?  Would that reintroduce any problems?
>
> None that I am aware of, I know Guenter required it for this series:
>
> https://lkml.org/lkml/2015/7/30/861
>
> but it was not merged so, as far as I understand, reverting the patch
> should get things to normal. I think it unearthed a couple of niggles

It looks like me and Yinghai disagree how the problem I was trying to fix
should be handled, I don't understand Yinghai's concerns, and unfortunately
I just don't have the time I would need to get a better understanding.
It is fine with me to revert your patch and abandon mine.

Guenter

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

[Bjorn Helgaas]

[+cc Ray]

On Tue, Sep 15, 2015 at 09:51:54AM -0700, Guenter Roeck wrote:
> On 09/15/2015 09:30 AM, Lorenzo Pieralisi wrote:
> >On Tue, Sep 15, 2015 at 04:57:07PM +0100, Bjorn Helgaas wrote:

> >>I'm inclined to revert dff22d2054b5 ("PCI: Call
> >>pci_read_bridge_bases() from core instead of arch code") until we can
> >>figure this out.  I think the idea of moving that work from
> >>arch-specific code to the core is good, but it seems like it leads to
> >>more hacky workarounds than real cleanup right now.  What would break
> >>if we simply reverted it?  Would that reintroduce any problems?
> >
> >None that I am aware of, I know Guenter required it for this series:
> >
> >https://lkml.org/lkml/2015/7/30/861
> >
> >but it was not merged so, as far as I understand, reverting the patch
> >should get things to normal. I think it unearthed a couple of niggles
> 
> It looks like me and Yinghai disagree how the problem I was trying to fix
> should be handled, I don't understand Yinghai's concerns, and unfortunately
> I just don't have the time I would need to get a better understanding.
> It is fine with me to revert your patch and abandon mine.

I don't understand those concerns either (yet).

I put the following on my for-linus branch for v4.3.  I'd appreciate any
corrections to my understanding of the problem.


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.
    
    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>

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

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

[Yinghai Lu]

On Tue, Sep 15, 2015 at 12:25 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> [+cc Ray]
>
> On Tue, Sep 15, 2015 at 09:51:54AM -0700, Guenter Roeck wrote:
>> On 09/15/2015 09:30 AM, Lorenzo Pieralisi wrote:
>> >On Tue, Sep 15, 2015 at 04:57:07PM +0100, Bjorn Helgaas wrote:
>
>> >>I'm inclined to revert dff22d2054b5 ("PCI: Call
>> >>pci_read_bridge_bases() from core instead of arch code") until we can
>> >>figure this out.  I think the idea of moving that work from
>> >>arch-specific code to the core is good, but it seems like it leads to
>> >>more hacky workarounds than real cleanup right now.  What would break
>> >>if we simply reverted it?  Would that reintroduce any problems?

Agreed, that is good if we can move pci_read_bridge_bases() to pci core.

Also hope that we can move pcibios_resource_survey() etc into
core too.


> I put the following on my for-linus branch for v4.3.  I'd appreciate any
> corrections to my understanding of the problem.
>
>
> 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.
>
>     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: Yinghai Lu <yinghai@kernel.org>

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

[Lorenzo Pieralisi]

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

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

[Yinghai Lu]

On Tue, Sep 15, 2015 at 9:51 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 09/15/2015 09:30 AM, Lorenzo Pieralisi wrote:
>
> It looks like me and Yinghai disagree how the problem I was trying to fix
> should be handled, I don't understand Yinghai's concerns, and unfortunately
> I just don't have the time I would need to get a better understanding.
> It is fine with me to revert your patch and abandon mine.

I put one simplified version in one my branch.

https://git.kernel.org/cgit/linux/kernel/git/yinghai/linux-yinghai.git/patch/?id=cdf4bfbc1ce12d826abd49a8987eb3ca7e129332

and will sent that later if needed.

>From cdf4bfbc1ce12d826abd49a8987eb3ca7e129332 Mon Sep 17 00:00:00 2001
From: Guenter Roeck <linux@roeck-us.net>
Date: Tue, 15 Sep 2015 12:59:11 -0700
Subject: PCI: Only try to assign io port only for root bus that support it

The PCI subsystem always assumes that I/O is supported on root bus and
tries to assign an I/O window to each child bus even if that is not the
case.

This may result in messages such as:

  pcieport 0000:02:00.0: res[7]=[io  0x1000-0x0fff] get_res_add_size
add_size 1000
  pcieport 0000:02:00.0: BAR 7: no space for [io  size 0x1000]
  pcieport 0000:02:00.0: BAR 7: failed to assign [io  size 0x1000]

for each bridge port, even if root does not support I/O in the first place.

To avoid this message, check if root bus supports I/O, then child bus
will inherit the setting from parent bus, and later during sizing and
assigning, check that bus flags and skip those resources.

[bhelgaas: reverse sense of new pci_bus_flags_t value]
[yinghai: only check root bus io port, and use flag on sizing and assigning]
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>
CC: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index c4c6947..5d946bd 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -339,6 +339,9 @@ static void pci_read_bridge_io(struct pci_bus *child)
  struct pci_bus_region region;
  struct resource *res;

+ if (!(child->bus_flags & PCI_BUS_FLAGS_ROOT_SUPPORTS_IO))
+ return;
+
  io_mask = PCI_IO_RANGE_MASK;
  io_granularity = 0x1000;
  if (dev->io_window_1k) {
@@ -2126,6 +2129,8 @@ struct pci_bus *pci_create_root_bus(struct
device *parent, int bus,
  } else
  bus_addr[0] = '\0';
  dev_info(&b->dev, "root bus resource %pR%s\n", res, bus_addr);
+ if (resource_type(res) == IORESOURCE_IO)
+ b->bus_flags |= PCI_BUS_FLAGS_ROOT_SUPPORTS_IO;
  }

  resource_list_for_each_entry(window, &bridge->windows) {
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 9d5e415..8ee5d17 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -223,6 +223,10 @@ static void pdev_assign_resources_prepare(struct
pci_dev *dev,
  if (r->flags & IORESOURCE_PCI_FIXED)
  continue;

+ if ((r->flags & IORESOURCE_IO) &&
+    !(dev->bus->bus_flags & PCI_BUS_FLAGS_ROOT_SUPPORTS_IO))
+ continue;
+
  if (resource_disabled(r) || r->parent)
  continue;

@@ -1178,6 +1182,11 @@ static void pbus_size_io(struct pci_bus *bus,
resource_size_t min_size,
  min_size = 0;
  }

+ if (!(bus->bus_flags & PCI_BUS_FLAGS_ROOT_SUPPORTS_IO)) {
+ b_res->flags |= IORESOURCE_UNSET | IORESOURCE_DISABLED;
+ return;
+ }
+
  min_align = window_alignment(bus, IORESOURCE_IO);
  list_for_each_entry(dev, &bus->devices, bus_list) {
  int i;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 5ed9bf1..790c534 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -193,6 +193,7 @@ typedef unsigned short __bitwise pci_bus_flags_t;
 enum pci_bus_flags {
  PCI_BUS_FLAGS_NO_MSI   = (__force pci_bus_flags_t) 1,
  PCI_BUS_FLAGS_NO_MMRBC = (__force pci_bus_flags_t) 2,
+ PCI_BUS_FLAGS_ROOT_SUPPORTS_IO = (__force pci_bus_flags_t) 4,
 };

 /* These values come from the PCI Express Spec */
-- 
cgit v0.10.2

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

[Guenter Roeck]

On 09/15/2015 01:17 PM, Yinghai Lu wrote:
> On Tue, Sep 15, 2015 at 9:51 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 09/15/2015 09:30 AM, Lorenzo Pieralisi wrote:
>>
>> It looks like me and Yinghai disagree how the problem I was trying to fix
>> should be handled, I don't understand Yinghai's concerns, and unfortunately
>> I just don't have the time I would need to get a better understanding.
>> It is fine with me to revert your patch and abandon mine.
>
> I put one simplified version in one my branch.
>
> https://git.kernel.org/cgit/linux/kernel/git/yinghai/linux-yinghai.git/patch/?id=cdf4bfbc1ce12d826abd49a8987eb3ca7e129332
>
> and will sent that later if needed.
>
>>From cdf4bfbc1ce12d826abd49a8987eb3ca7e129332 Mon Sep 17 00:00:00 2001
> From: Guenter Roeck <linux@roeck-us.net>

Yinghai,

that isn't really my patch, so you should not send it under my name.

Thanks,
Guenter

> Date: Tue, 15 Sep 2015 12:59:11 -0700
> Subject: PCI: Only try to assign io port only for root bus that support it
>
> The PCI subsystem always assumes that I/O is supported on root bus and
> tries to assign an I/O window to each child bus even if that is not the
> case.
>
> This may result in messages such as:
>
>    pcieport 0000:02:00.0: res[7]=[io  0x1000-0x0fff] get_res_add_size
> add_size 1000
>    pcieport 0000:02:00.0: BAR 7: no space for [io  size 0x1000]
>    pcieport 0000:02:00.0: BAR 7: failed to assign [io  size 0x1000]
>
> for each bridge port, even if root does not support I/O in the first place.
>
> To avoid this message, check if root bus supports I/O, then child bus
> will inherit the setting from parent bus, and later during sizing and
> assigning, check that bus flags and skip those resources.
>
> [bhelgaas: reverse sense of new pci_bus_flags_t value]
> [yinghai: only check root bus io port, and use flag on sizing and assigning]
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> CC: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index c4c6947..5d946bd 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -339,6 +339,9 @@ static void pci_read_bridge_io(struct pci_bus *child)
>    struct pci_bus_region region;
>    struct resource *res;
>
> + if (!(child->bus_flags & PCI_BUS_FLAGS_ROOT_SUPPORTS_IO))
> + return;
> +
>    io_mask = PCI_IO_RANGE_MASK;
>    io_granularity = 0x1000;
>    if (dev->io_window_1k) {
> @@ -2126,6 +2129,8 @@ struct pci_bus *pci_create_root_bus(struct
> device *parent, int bus,
>    } else
>    bus_addr[0] = '\0';
>    dev_info(&b->dev, "root bus resource %pR%s\n", res, bus_addr);
> + if (resource_type(res) == IORESOURCE_IO)
> + b->bus_flags |= PCI_BUS_FLAGS_ROOT_SUPPORTS_IO;
>    }
>
>    resource_list_for_each_entry(window, &bridge->windows) {
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 9d5e415..8ee5d17 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -223,6 +223,10 @@ static void pdev_assign_resources_prepare(struct
> pci_dev *dev,
>    if (r->flags & IORESOURCE_PCI_FIXED)
>    continue;
>
> + if ((r->flags & IORESOURCE_IO) &&
> +    !(dev->bus->bus_flags & PCI_BUS_FLAGS_ROOT_SUPPORTS_IO))
> + continue;
> +
>    if (resource_disabled(r) || r->parent)
>    continue;
>
> @@ -1178,6 +1182,11 @@ static void pbus_size_io(struct pci_bus *bus,
> resource_size_t min_size,
>    min_size = 0;
>    }
>
> + if (!(bus->bus_flags & PCI_BUS_FLAGS_ROOT_SUPPORTS_IO)) {
> + b_res->flags |= IORESOURCE_UNSET | IORESOURCE_DISABLED;
> + return;
> + }
> +
>    min_align = window_alignment(bus, IORESOURCE_IO);
>    list_for_each_entry(dev, &bus->devices, bus_list) {
>    int i;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 5ed9bf1..790c534 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -193,6 +193,7 @@ typedef unsigned short __bitwise pci_bus_flags_t;
>   enum pci_bus_flags {
>    PCI_BUS_FLAGS_NO_MSI   = (__force pci_bus_flags_t) 1,
>    PCI_BUS_FLAGS_NO_MMRBC = (__force pci_bus_flags_t) 2,
> + PCI_BUS_FLAGS_ROOT_SUPPORTS_IO = (__force pci_bus_flags_t) 4,
>   };
>
>   /* These values come from the PCI Express Spec */
>

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

[Yinghai Lu]

On Tue, Sep 15, 2015 at 2:07 PM, Guenter Roeck <linux@roeck-us.net> wrote:

>>
>> https://git.kernel.org/cgit/linux/kernel/git/yinghai/linux-yinghai.git/patch/?id=cdf4bfbc1ce12d826abd49a8987eb3ca7e129332
>>
>> and will sent that later if needed.
>>
>>> From cdf4bfbc1ce12d826abd49a8987eb3ca7e129332 Mon Sep 17 00:00:00 2001
>
> that isn't really my patch, so you should not send it under my name.
>

ok, I will drop that patch.

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

[Lorenzo Pieralisi]

On Sat, Sep 05, 2015 at 12:53:48AM +0100, Yinghai Lu wrote:
> On Fri, Sep 4, 2015 at 9:44 AM, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> > On Fri, Sep 04, 2015 at 05:00:35PM +0100, Yinghai Lu wrote:
> >
> > The problem here is not the last retry, it is the first bridge scan.
> >
> > By moving pci_read_bridge_bases() to core PCI code, if we do not
> > vet the bridge apertures (ie claim them and reset them if the claiming
> > fails) we end up calling (on ARM) __pci_bus_size_bridges() with apertures
> > that can have sizes != 0, which does not make any sense since we are calling
> > __pci_bus_size_bridges() to *discover* what the aperture size should
> > be on first bridge scan, correct ?
> 
> for x86, in pcibios_allocate_bridge_resources(), we do validate
> the bridge resources, and reset size to 1 (strange ?!).
> and they are called before pci_asssign_unassigned_resources()
> 
> so arch ARM would support pcibios_allocate_bridge_resources or other
> call to do the same thing?
> 
> wonder some arches even claim fails, they still does not want you to
> reset it.

While finding a fix for this issue, I bumped into another one
with current pci_claim_bridge_resource() behaviour. Described
and "fixed" (I actually did not fix it, patch below is just there to
show what the problem is) in the patch below.

Comments welcome,
Lorenzo

-- >8 --
Subject: [PATCH] PCI: remove dead code in pci_claim_bridge_resource()

Commit 8505e729a2f6eb ("PCI: Add pci_claim_bridge_resource() to clip
window if necessary") introduced a new API to claim bridge resources.

pci_claim_bridge_resource() tries to claim a bridge resource, and if
the claiming fails the function tries to clip the resource to make
it fit within the parent resource window.

If the clipping succeeds the bridge apertures are set-up accordingly
and the pci_claim_bridge_resource() tries to claim the resource
again.

Commit c770cb4cb505 ("PCI: Mark invalid BARs as unassigned") added
code that sets the IORESOURCE_UNSET flag on claiming failure.

This means that the second resource claiming after window clipping will
always fail, since the resource flags contain IORESOURCE_UNSET,
previously set on failure by pci_claim_resource(), so the subsequent
pci_claim_resource() call ends up spitting a log message and return
failure with no chance whatsoever to succeed.

This patch removes the second pci_claim_resource() call in
pci_claim_bridge_resource() since it basically has no chance to
succeed, leaving the current behaviour unchanged.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/setup-bus.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 508cc56..2bf4ac1 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -728,14 +728,10 @@ int pci_claim_bridge_resource(struct pci_dev *bridge, int i)
 		break;
 	case 2:
 		pci_setup_bridge_mmio_pref(bridge);
-		break;
 	default:
-		return -EINVAL;
+		break;
 	}
 
-	if (pci_claim_resource(bridge, i) == 0)
-		return 0;	/* claimed a smaller window */
-
 	return -EINVAL;
 }
 
-- 
2.2.1

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

[Yinghai Lu]

On Wed, Sep 9, 2015 at 4:32 AM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> Subject: [PATCH] PCI: remove dead code in pci_claim_bridge_resource()
>
> Commit 8505e729a2f6eb ("PCI: Add pci_claim_bridge_resource() to clip
> window if necessary") introduced a new API to claim bridge resources.
>
> pci_claim_bridge_resource() tries to claim a bridge resource, and if
> the claiming fails the function tries to clip the resource to make
> it fit within the parent resource window.
>
> If the clipping succeeds the bridge apertures are set-up accordingly
> and the pci_claim_bridge_resource() tries to claim the resource
> again.
>
> Commit c770cb4cb505 ("PCI: Mark invalid BARs as unassigned") added
> code that sets the IORESOURCE_UNSET flag on claiming failure.
>
> This means that the second resource claiming after window clipping will
> always fail, since the resource flags contain IORESOURCE_UNSET,
> previously set on failure by pci_claim_resource(), so the subsequent
> pci_claim_resource() call ends up spitting a log message and return
> failure with no chance whatsoever to succeed.
>
> This patch removes the second pci_claim_resource() call in
> pci_claim_bridge_resource() since it basically has no chance to
> succeed, leaving the current behaviour unchanged.
>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Yinghai Lu <yinghai@kernel.org>
> ---
>  drivers/pci/setup-bus.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 508cc56..2bf4ac1 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -728,14 +728,10 @@ int pci_claim_bridge_resource(struct pci_dev *bridge, int i)
>                 break;
>         case 2:
>                 pci_setup_bridge_mmio_pref(bridge);
> -               break;
>         default:
> -               return -EINVAL;
> +               break;
>         }
>
> -       if (pci_claim_resource(bridge, i) == 0)
> -               return 0;       /* claimed a smaller window */
> -
>         return -EINVAL;
>  }

That should be regression from c770cb4cb505 ("PCI: Mark invalid BARs
as unassigned")
so right fix should be:

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 508cc56..76b3349 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -733,6 +733,7 @@ int pci_claim_bridge_resource(struct pci_dev *bridge, int i)
                return -EINVAL;
        }

+       bridge->resource[i].flags &= ~IORESOURCE_UNSET;
        if (pci_claim_resource(bridge, i) == 0)
                return 0;       /* claimed a smaller window */

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

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 382 bytes --]

On Wed, Sep 9, 2015 at 9:59 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Wed, Sep 9, 2015 at 4:32 AM, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
>> Subject: [PATCH] PCI: remove dead code in pci_claim_bridge_resource()
>>
> That should be regression from c770cb4cb505 ("PCI: Mark invalid BARs
> as unassigned")
> so right fix should be:

completed format as attached:

[-- Attachment #2: fix_pci_clip_bridge.patch --]
[-- Type: text/x-patch, Size: 1701 bytes --]

From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Subject: [PATCH] PCI: Fix clipped bridge resource reserve

Commit 8505e729a2f6eb ("PCI: Add pci_claim_bridge_resource() to clip
window if necessary") introduced a new API to claim bridge resources.
pci_claim_bridge_resource() tries to claim a bridge resource, and if
the claiming fails the function tries to clip the resource to make
it fit within the parent resource window.
If the clipping succeeds the bridge apertures are set-up accordingly
and the pci_claim_bridge_resource() tries to claim the resource
again.

Commit c770cb4cb505 ("PCI: Mark invalid BARs as unassigned") added
code that sets the IORESOURCE_UNSET flag on claiming failure.

This means that the second resource claiming after window clipping will
always fail, since the resource flags contain IORESOURCE_UNSET,
previously set on failure by pci_claim_resource(), so the subsequent
pci_claim_resource() call ends up spitting a log message and return
failure with no chance whatsoever to succeed.

This patch clear the UNSET in resource flags, and it makes second
pci_claim_resource() work again.

Fixes: c770cb4cb505 ("PCI: Mark invalid BARs as unassigned")
Cc: stable@vger.kernel.org [v4.1+]
[change to clear UNSET instead, Yinghai]
Signed-off-by: Yinghai Lu <yinghai@kernel.org>

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 508cc56..76b3349 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -733,6 +733,7 @@ int pci_claim_bridge_resource(struct pci_dev *bridge, int i)
 		return -EINVAL;
 	}
 
+	bridge->resource[i].flags &= ~IORESOURCE_UNSET;
 	if (pci_claim_resource(bridge, i) == 0)
 		return 0;	/* claimed a smaller window */
 

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

[Lorenzo Pieralisi]

On Wed, Sep 09, 2015 at 06:22:34PM +0100, Yinghai Lu wrote:
> On Wed, Sep 9, 2015 at 9:59 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> > On Wed, Sep 9, 2015 at 4:32 AM, Lorenzo Pieralisi
> > <lorenzo.pieralisi@arm.com> wrote:
> >> Subject: [PATCH] PCI: remove dead code in pci_claim_bridge_resource()
> >>
> > That should be regression from c770cb4cb505 ("PCI: Mark invalid BARs
> > as unassigned")
> > so right fix should be:
> 
> completed format as attached:

> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Subject: [PATCH] PCI: Fix clipped bridge resource reserve
> 
> Commit 8505e729a2f6eb ("PCI: Add pci_claim_bridge_resource() to clip
> window if necessary") introduced a new API to claim bridge resources.
> pci_claim_bridge_resource() tries to claim a bridge resource, and if
> the claiming fails the function tries to clip the resource to make
> it fit within the parent resource window.
> If the clipping succeeds the bridge apertures are set-up accordingly
> and the pci_claim_bridge_resource() tries to claim the resource
> again.
> 
> Commit c770cb4cb505 ("PCI: Mark invalid BARs as unassigned") added
> code that sets the IORESOURCE_UNSET flag on claiming failure.
> 
> This means that the second resource claiming after window clipping will
> always fail, since the resource flags contain IORESOURCE_UNSET,
> previously set on failure by pci_claim_resource(), so the subsequent
> pci_claim_resource() call ends up spitting a log message and return
> failure with no chance whatsoever to succeed.
> 
> This patch clear the UNSET in resource flags, and it makes second
> pci_claim_resource() work again.
> 
> Fixes: c770cb4cb505 ("PCI: Mark invalid BARs as unassigned")
> Cc: stable@vger.kernel.org [v4.1+]
> [change to clear UNSET instead, Yinghai]
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>

My Signed-off-by is missing, if you do not mind given that I reported
it and I obviously knew this was the real fix (I said that) I will
send the updated patch to the list accordingly or Bjorn can fix it up,
as you both prefer.

Thanks,
Lorenzo

> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 508cc56..76b3349 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -733,6 +733,7 @@ int pci_claim_bridge_resource(struct pci_dev *bridge, int i)
>  		return -EINVAL;
>  	}
>  
> +	bridge->resource[i].flags &= ~IORESOURCE_UNSET;
>  	if (pci_claim_resource(bridge, i) == 0)
>  		return 0;	/* claimed a smaller window */
>  

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

[oe5hpm]

hi bjorn, Lorenzo,

should i do some testing with patch from yesterday?
please let me know if can test anything.

best regards,
Hannes


On Wed, Sep 2, 2015 at 10:32 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> Hi Lorenzo, thanks for jumping on this!
>
> On Wed, Sep 02, 2015 at 06:47:16PM +0100, Lorenzo Pieralisi wrote:
>> Hi Hannes,
>>
>> On Wed, Sep 02, 2015 at 10:51:18AM +0100, oe5hpm wrote:
>> > Hi Lorenzo,
>> >
>> > today i tried to boot up the most recent vanilla kernel on my
>> > Freescale i.mx6 board.
>> > I ran into trouble regarding PCI enumeration.
>> >
>> > [    0.431949] imx6q-pcie 1ffc000.pcie: PCI host bridge to bus 0000:00
>> > [    0.431976] pci_bus 0000:00: root bus resource [io  0x1000-0xffff]
>> > [    0.431996] pci_bus 0000:00: root bus resource [mem 0x01000000-0x01efffff]
>> > [    0.432022] pci_bus 0000:00: root bus resource [bus 00-ff]
>> > [    0.433271] PCI: bus0: Fast back to back transfers disabled
>> > [    0.433629] pci 0000:00:00.0: PCI bridge to [bus 01-ff]
>> > [    0.435181] PCI: bus1: Fast back to back transfers disabled
>> > [    0.435564] pci 0000:00:00.0: BAR 0: assigned [mem 0x01000000-0x010fffff]
>> > [    0.435593] pci 0000:00:00.0: BAR 8: no space for [mem size 0x01000000]
>> > [    0.435613] pci 0000:00:00.0: BAR 8: failed to assign [mem size 0x01000000]
>> > [    0.435635] pci 0000:00:00.0: BAR 9: assigned [mem
>> > 0x01100000-0x011fffff pref]
>> > [    0.435655] pci 0000:00:00.0: BAR 6: assigned [mem
>> > 0x01200000-0x0120ffff pref]
>> > [    0.435676] pci 0000:00:00.0: BAR 7: assigned [io  0x1000-0x1fff]
>> > [    0.435705] pci 0000:01:00.0: BAR 2: no space for [mem size 0x00200000]
>> > [    0.435722] pci 0000:01:00.0: BAR 2: failed to assign [mem size 0x00200000]
>> > [    0.435739] pci 0000:01:00.0: BAR 1: no space for [mem size 0x00004000]
>> > [    0.435754] pci 0000:01:00.0: BAR 1: failed to assign [mem size 0x00004000]
>> > [    0.435770] pci 0000:01:00.0: BAR 0: no space for [mem size 0x00000100]
>> > [    0.435786] pci 0000:01:00.0: BAR 0: failed to assign [mem size 0x00000100]
>> > [    0.435804] pci 0000:00:00.0: PCI bridge to [bus 01]
>> > [    0.435826] pci 0000:00:00.0:   bridge window [io  0x1000-0x1fff]
>> > [    0.435855] pci 0000:00:00.0:   bridge window [mem
>> > 0x01100000-0x011fffff pref]
>> >
>> > there are several fails assigning memory ressources to pci-devices.
>> >
>> > i bisect down this trouble to commit id:
>> > dff22d2054b5dbb1889f20c03959dd0c494fab8c : PCI: Call
>> > pci_read_bridge_bases() from core instead of arch code
>> >
>> > For testing purpose i've reverted this commit on a local branch and
>> > everythings works fine, as before.
>> >
>> > [    0.431976] imx6q-pcie 1ffc000.pcie: PCI host bridge to bus 0000:00
>> > [    0.432004] pci_bus 0000:00: root bus resource [io  0x1000-0xffff]
>> > [    0.432023] pci_bus 0000:00: root bus resource [mem 0x01000000-0x01efffff]
>> > [    0.432047] pci_bus 0000:00: root bus resource [bus 00-ff]
>> > [    0.433302] PCI: bus0: Fast back to back transfers disabled
>> > [    0.435122] PCI: bus1: Fast back to back transfers disabled
>> > [    0.435504] pci 0000:00:00.0: BAR 0: assigned [mem 0x01000000-0x010fffff]
>> > [    0.435535] pci 0000:00:00.0: BAR 8: assigned [mem 0x01100000-0x013fffff]
>> > [    0.435557] pci 0000:00:00.0: BAR 6: assigned [mem
>> > 0x01400000-0x0140ffff pref]
>> > [    0.435585] pci 0000:01:00.0: BAR 2: assigned [mem 0x01200000-0x013fffff]
>> > [    0.435626] pci 0000:01:00.0: BAR 1: assigned [mem 0x01100000-0x01103fff]
>> > [    0.435665] pci 0000:01:00.0: BAR 0: assigned [mem 0x01104000-0x011040ff]
>> > [    0.435703] pci 0000:00:00.0: PCI bridge to [bus 01]
>> > [    0.435728] pci 0000:00:00.0:   bridge window [mem 0x01100000-0x013fffff]
>> >
>> > Further i can break down the failure to "drivers/pci/probe.c" line #924.
>> > If i comment out the "pci_read_bridge_bases(child);" also everything works well.
>> >
>> > I have to confess, that my knowledge about the whole PCI thing in the
>> > kernel is not very deep, so it is not possible for me to figure out
>> > what is going wrong.
>>
>> It looks like a bogus bridge aperture size is causing this to happen,
>> and this prevents reassignment on arm (bridge aperture is too big),
>> which proves that reading the bridge bases without vetting the corresponding
>> resources may break (on platforms that were not reading them before).
>>
>> arm was the only platform not reading the bridge bases, here is an
>> answer why. So, to prevent reverting the commit I put together this
>> patch (to be reworked if we deem it reasonable), subject to discussion
>> (I fear it may end up breaking other arm platforms, I do not have all
>> ARM boards and required host controllers to test, I managed to test it on
>> an iMX6 Sabrelite though).
>>
>> Here, please let me know if it works for you, I will keep on thinking
>> to find the best solution.
>>
>> I will have to do this for arm64 too, comments very welcome.
>>
>> Thanks,
>> Lorenzo
>>
>> -- >8 --
>> Subject: [PATCH] arm: kernel: pci: fixup erroneous PCI bridge apertures
>>
>> Bridge apertures read by core PCI code through pci_read_bridge_bases()
>> might be erroneous (bogus platform setup). If the arch code does not vet
>> the bridge resources (ie by trying to claim them), we can end up in a
>> situation where wrong bridge apertures can prevent resources assignment
>> for downstream devices causing enumeration failures (eg a bridge
>> aperture does not fit in the respective host controller resource window,
>> so it can't be assigned).
>>
>> This patch adds arm arch code that vets bridge resources by trying
>> to claim them, and reset them on claiming failure so that they can
>> be properly reassigned.
>
> We definitely should not depend on the platform to set up the bridge
> windows.  Do we know what the platform left in the 00:00.0 window
> registers?
>
> I see that bus 01 requires 0x204100 of mem space, which must be
> rounded up to a megabyte boundary, so the window must be at least 3M
> (0x00300000):
>
>   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]
>
> I don't understand the connection with dff22d2054b5 yet.  If we don't
> call pci_read_bridge_bases(), apparently some assign-resources path
> figures out the required size and assigns a 3M window.
>
> If we *do* call pci_read_bridge_bases(), do we read a bogus 16M window
> size, fail to assign that because the host controller window isn't big
> enough, and then the assign-resources path just gives up?  I assume
> clearing r->flags in your patch is the critical thing?  Is there
> something in assign-resources that checks for r->flags == 0?
>
> I think it would be ideal if we could someday claim the resource
> immediately, as soon as we read it from a BAR or bridge window, and
> mark it as IORESOURCE_UNSET if claiming it fails.  Then if the
> platform set up reasonable windows, we could use them; if it didn't,
> we could just assign our own.
>
> I'd like to avoid adding things to pcibios_fixup_bus() if possible
> because most of what is done there is arch-independent, and I'd like
> to get the arch-independent stuff into the PCI core.
>
> Bjorn
>
>> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> ---
>>  arch/arm/kernel/bios32.c | 24 ++++++++++++++++++++++++
>>  1 file changed, 24 insertions(+)
>>
>> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
>> index 874e182..ebbe052 100644
>> --- a/arch/arm/kernel/bios32.c
>> +++ b/arch/arm/kernel/bios32.c
>> @@ -282,6 +282,27 @@ static inline int pdev_bad_for_parity(struct pci_dev *dev)
>>
>>  }
>>
>> +static void pcibios_fixup_bridge_resources(struct pci_dev *dev)
>> +{
>> +     int idx;
>> +
>> +     if (!dev->bus)
>> +             return;
>> +
>> +     for (idx = PCI_BRIDGE_RESOURCES; idx < PCI_NUM_RESOURCES; idx++) {
>> +             struct resource *r = &dev->resource[idx];
>> +
>> +             if (!r->flags || r->parent)
>> +                     continue;
>> +
>> +             if (pci_claim_resource(dev, idx)) {
>> +                     r->flags = 0;
>> +                     r->start = 0;
>> +                     r->end = -1;
>> +             }
>> +     }
>> +}
>> +
>>  /*
>>   * pcibios_fixup_bus - Called after each bus is probed,
>>   * but before its children are examined.
>> @@ -352,6 +373,9 @@ void pcibios_fixup_bus(struct pci_bus *bus)
>>                       bus->bridge_ctl |= PCI_BRIDGE_CTL_PARITY;
>>       }
>>
>> +     if (bus->self)
>> +             pcibios_fixup_bridge_resources(bus->self);
>> +
>>       /*
>>        * Report what we did for this bus
>>        */
>> --
>> 1.9.1
>>
>>

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

[oe5hpm]

Hi,

i've tested the patch supplied by Lorenzo yesterday.
Everything works well now.

Here is my kernel-printout:

[    0.432211] imx6q-pcie 1ffc000.pcie: PCI host bridge to bus 0000:00
[    0.432238] pci_bus 0000:00: root bus resource [io  0x1000-0xffff]
[    0.432258] pci_bus 0000:00: root bus resource [mem 0x01000000-0x01efffff]
[    0.432281] pci_bus 0000:00: root bus resource [bus 00-ff]
[    0.432302] pci_bus 0000:00: scanning bus
[    0.432443] pci 0000:00:00.0: [16c3:abcd] type 01 class 0x060400
[    0.432526] pci 0000:00:00.0: reg 0x10: [mem 0x00000000-0x000fffff]
[    0.432573] pci 0000:00:00.0: reg 0x38: [mem 0x00000000-0x0000ffff pref]
[    0.432649] pci 0000:00:00.0: calling pci_fixup_ide_bases+0x0/0x48
[    0.432789] pci 0000:00:00.0: supports D1
[    0.432806] pci 0000:00:00.0: PME# supported from D0 D1 D3hot D3cold
[    0.432876] pci 0000:00:00.0: PME# disabled
[    0.433513] pci_bus 0000:00: fixups for bus
[    0.433547] PCI: bus0: Fast back to back transfers disabled
[    0.433570] pci 0000:00:00.0: scanning [bus 01-01] behind bridge, pass 0
[    0.433604] pci 0000:00:00.0: scanning [bus 00-00] behind bridge, pass 1
[    0.433914] pci 0000:00:00.0: PCI bridge to [bus 01-ff]
[    0.433942] pci 0000:00:00.0:   bridge window [io  0x0000-0x0fff]
[    0.433965] pci 0000:00:00.0:   bridge window [mem 0x01000000-0x01ffffff]
[    0.433989] pci 0000:00:00.0:   bridge window [mem
0x00000000-0x000fffff pref]
[    0.434004] pci_bus 0000:01: scanning bus
[    0.434129] pci 0000:01:00.0: [1677:affe] type 00 class 0x088030
[    0.434326] pci 0000:01:00.0: reg 0x10: [mem 0x00000000-0x00000fff]
[    0.434390] pci 0000:01:00.0: reg 0x14: [mem 0x00000000-0x00003fff]
[    0.434450] pci 0000:01:00.0: reg 0x18: [mem 0x00000000-0x001fffff]
[    0.434698] pci 0000:01:00.0: calling pci_fixup_ide_bases+0x0/0x48
[    0.435397] pci_bus 0000:01: fixups for bus
[    0.435460] pci 0000:00:00.0: can't claim BAR 7 [io
0x0000-0x0fff]: no compatible bridge window
[    0.435479] pci 0000:00:00.0: can't claim BAR 8 [mem
0x01000000-0x01ffffff]: no compatible bridge window
[    0.435497] pci 0000:00:00.0: can't claim BAR 9 [mem
0x00000000-0x000fffff pref]: no compatible bridge window

[    0.436128] pci 0000:00:00.0: PCI bridge to [bus 01]
[    0.436153] pci 0000:00:00.0:   bridge window [mem 0x01100000-0x013fffff]

don't worry about the increased size of
pci 0000:01:00.0: BAR 0: assigned [mem 0x01104000-0x01104fff]
in between I've changed my FPGA device.

best regards,
Hannes


On Thu, Sep 3, 2015 at 12:03 PM, oe5hpm <oe5hpm@gmail.com> wrote:
> hi bjorn, Lorenzo,
>
> should i do some testing with patch from yesterday?
> please let me know if can test anything.
>
> best regards,
> Hannes
>
>
> On Wed, Sep 2, 2015 at 10:32 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> Hi Lorenzo, thanks for jumping on this!
>>
>> On Wed, Sep 02, 2015 at 06:47:16PM +0100, Lorenzo Pieralisi wrote:
>>> Hi Hannes,
>>>
>>> On Wed, Sep 02, 2015 at 10:51:18AM +0100, oe5hpm wrote:
>>> > Hi Lorenzo,
>>> >
>>> > today i tried to boot up the most recent vanilla kernel on my
>>> > Freescale i.mx6 board.
>>> > I ran into trouble regarding PCI enumeration.
>>> >
>>> > [    0.431949] imx6q-pcie 1ffc000.pcie: PCI host bridge to bus 0000:00
>>> > [    0.431976] pci_bus 0000:00: root bus resource [io  0x1000-0xffff]
>>> > [    0.431996] pci_bus 0000:00: root bus resource [mem 0x01000000-0x01efffff]
>>> > [    0.432022] pci_bus 0000:00: root bus resource [bus 00-ff]
>>> > [    0.433271] PCI: bus0: Fast back to back transfers disabled
>>> > [    0.433629] pci 0000:00:00.0: PCI bridge to [bus 01-ff]
>>> > [    0.435181] PCI: bus1: Fast back to back transfers disabled
>>> > [    0.435564] pci 0000:00:00.0: BAR 0: assigned [mem 0x01000000-0x010fffff]
>>> > [    0.435593] pci 0000:00:00.0: BAR 8: no space for [mem size 0x01000000]
>>> > [    0.435613] pci 0000:00:00.0: BAR 8: failed to assign [mem size 0x01000000]
>>> > [    0.435635] pci 0000:00:00.0: BAR 9: assigned [mem
>>> > 0x01100000-0x011fffff pref]
>>> > [    0.435655] pci 0000:00:00.0: BAR 6: assigned [mem
>>> > 0x01200000-0x0120ffff pref]
>>> > [    0.435676] pci 0000:00:00.0: BAR 7: assigned [io  0x1000-0x1fff]
>>> > [    0.435705] pci 0000:01:00.0: BAR 2: no space for [mem size 0x00200000]
>>> > [    0.435722] pci 0000:01:00.0: BAR 2: failed to assign [mem size 0x00200000]
>>> > [    0.435739] pci 0000:01:00.0: BAR 1: no space for [mem size 0x00004000]
>>> > [    0.435754] pci 0000:01:00.0: BAR 1: failed to assign [mem size 0x00004000]
>>> > [    0.435770] pci 0000:01:00.0: BAR 0: no space for [mem size 0x00000100]
>>> > [    0.435786] pci 0000:01:00.0: BAR 0: failed to assign [mem size 0x00000100]
>>> > [    0.435804] pci 0000:00:00.0: PCI bridge to [bus 01]
>>> > [    0.435826] pci 0000:00:00.0:   bridge window [io  0x1000-0x1fff]
>>> > [    0.435855] pci 0000:00:00.0:   bridge window [mem
>>> > 0x01100000-0x011fffff pref]
>>> >
>>> > there are several fails assigning memory ressources to pci-devices.
>>> >
>>> > i bisect down this trouble to commit id:
>>> > dff22d2054b5dbb1889f20c03959dd0c494fab8c : PCI: Call
>>> > pci_read_bridge_bases() from core instead of arch code
>>> >
>>> > For testing purpose i've reverted this commit on a local branch and
>>> > everythings works fine, as before.
>>> >
>>> > [    0.431976] imx6q-pcie 1ffc000.pcie: PCI host bridge to bus 0000:00
>>> > [    0.432004] pci_bus 0000:00: root bus resource [io  0x1000-0xffff]
>>> > [    0.432023] pci_bus 0000:00: root bus resource [mem 0x01000000-0x01efffff]
>>> > [    0.432047] pci_bus 0000:00: root bus resource [bus 00-ff]
>>> > [    0.433302] PCI: bus0: Fast back to back transfers disabled
>>> > [    0.435122] PCI: bus1: Fast back to back transfers disabled
>>> > [    0.435504] pci 0000:00:00.0: BAR 0: assigned [mem 0x01000000-0x010fffff]
>>> > [    0.435535] pci 0000:00:00.0: BAR 8: assigned [mem 0x01100000-0x013fffff]
>>> > [    0.435557] pci 0000:00:00.0: BAR 6: assigned [mem
>>> > 0x01400000-0x0140ffff pref]
>>> > [    0.435585] pci 0000:01:00.0: BAR 2: assigned [mem 0x01200000-0x013fffff]
>>> > [    0.435626] pci 0000:01:00.0: BAR 1: assigned [mem 0x01100000-0x01103fff]
>>> > [    0.435665] pci 0000:01:00.0: BAR 0: assigned [mem 0x01104000-0x011040ff]
>>> > [    0.435703] pci 0000:00:00.0: PCI bridge to [bus 01]
>>> > [    0.435728] pci 0000:00:00.0:   bridge window [mem 0x01100000-0x013fffff]
>>> >
>>> > Further i can break down the failure to "drivers/pci/probe.c" line #924.
>>> > If i comment out the "pci_read_bridge_bases(child);" also everything works well.
>>> >
>>> > I have to confess, that my knowledge about the whole PCI thing in the
>>> > kernel is not very deep, so it is not possible for me to figure out
>>> > what is going wrong.
>>>
>>> It looks like a bogus bridge aperture size is causing this to happen,
>>> and this prevents reassignment on arm (bridge aperture is too big),
>>> which proves that reading the bridge bases without vetting the corresponding
>>> resources may break (on platforms that were not reading them before).
>>>
>>> arm was the only platform not reading the bridge bases, here is an
>>> answer why. So, to prevent reverting the commit I put together this
>>> patch (to be reworked if we deem it reasonable), subject to discussion
>>> (I fear it may end up breaking other arm platforms, I do not have all
>>> ARM boards and required host controllers to test, I managed to test it on
>>> an iMX6 Sabrelite though).
>>>
>>> Here, please let me know if it works for you, I will keep on thinking
>>> to find the best solution.
>>>
>>> I will have to do this for arm64 too, comments very welcome.
>>>
>>> Thanks,
>>> Lorenzo
>>>
>>> -- >8 --
>>> Subject: [PATCH] arm: kernel: pci: fixup erroneous PCI bridge apertures
>>>
>>> Bridge apertures read by core PCI code through pci_read_bridge_bases()
>>> might be erroneous (bogus platform setup). If the arch code does not vet
>>> the bridge resources (ie by trying to claim them), we can end up in a
>>> situation where wrong bridge apertures can prevent resources assignment
>>> for downstream devices causing enumeration failures (eg a bridge
>>> aperture does not fit in the respective host controller resource window,
>>> so it can't be assigned).
>>>
>>> This patch adds arm arch code that vets bridge resources by trying
>>> to claim them, and reset them on claiming failure so that they can
>>> be properly reassigned.
>>
>> We definitely should not depend on the platform to set up the bridge
>> windows.  Do we know what the platform left in the 00:00.0 window
>> registers?
>>
>> I see that bus 01 requires 0x204100 of mem space, which must be
>> rounded up to a megabyte boundary, so the window must be at least 3M
>> (0x00300000):
>>
>>   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]
>>
>> I don't understand the connection with dff22d2054b5 yet.  If we don't
>> call pci_read_bridge_bases(), apparently some assign-resources path
>> figures out the required size and assigns a 3M window.
>>
>> If we *do* call pci_read_bridge_bases(), do we read a bogus 16M window
>> size, fail to assign that because the host controller window isn't big
>> enough, and then the assign-resources path just gives up?  I assume
>> clearing r->flags in your patch is the critical thing?  Is there
>> something in assign-resources that checks for r->flags == 0?
>>
>> I think it would be ideal if we could someday claim the resource
>> immediately, as soon as we read it from a BAR or bridge window, and
>> mark it as IORESOURCE_UNSET if claiming it fails.  Then if the
>> platform set up reasonable windows, we could use them; if it didn't,
>> we could just assign our own.
>>
>> I'd like to avoid adding things to pcibios_fixup_bus() if possible
>> because most of what is done there is arch-independent, and I'd like
>> to get the arch-independent stuff into the PCI core.
>>
>> Bjorn
>>
>>> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>>> ---
>>>  arch/arm/kernel/bios32.c | 24 ++++++++++++++++++++++++
>>>  1 file changed, 24 insertions(+)
>>>
>>> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
>>> index 874e182..ebbe052 100644
>>> --- a/arch/arm/kernel/bios32.c
>>> +++ b/arch/arm/kernel/bios32.c
>>> @@ -282,6 +282,27 @@ static inline int pdev_bad_for_parity(struct pci_dev *dev)
>>>
>>>  }
>>>
>>> +static void pcibios_fixup_bridge_resources(struct pci_dev *dev)
>>> +{
>>> +     int idx;
>>> +
>>> +     if (!dev->bus)
>>> +             return;
>>> +
>>> +     for (idx = PCI_BRIDGE_RESOURCES; idx < PCI_NUM_RESOURCES; idx++) {
>>> +             struct resource *r = &dev->resource[idx];
>>> +
>>> +             if (!r->flags || r->parent)
>>> +                     continue;
>>> +
>>> +             if (pci_claim_resource(dev, idx)) {
>>> +                     r->flags = 0;
>>> +                     r->start = 0;
>>> +                     r->end = -1;
>>> +             }
>>> +     }
>>> +}
>>> +
>>>  /*
>>>   * pcibios_fixup_bus - Called after each bus is probed,
>>>   * but before its children are examined.
>>> @@ -352,6 +373,9 @@ void pcibios_fixup_bus(struct pci_bus *bus)
>>>                       bus->bridge_ctl |= PCI_BRIDGE_CTL_PARITY;
>>>       }
>>>
>>> +     if (bus->self)
>>> +             pcibios_fixup_bridge_resources(bus->self);
>>> +
>>>       /*
>>>        * Report what we did for this bus
>>>        */
>>> --
>>> 1.9.1
>>>
>>>

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

[Lorenzo Pieralisi]

Hi Hannes,

On Thu, Sep 03, 2015 at 11:30:40AM +0100, oe5hpm wrote:
> Hi,
> 
> i've tested the patch supplied by Lorenzo yesterday.
> Everything works well now.

Thanks, comments below.

> Here is my kernel-printout:
> 
> [    0.432211] imx6q-pcie 1ffc000.pcie: PCI host bridge to bus 0000:00
> [    0.432238] pci_bus 0000:00: root bus resource [io  0x1000-0xffff]
> [    0.432258] pci_bus 0000:00: root bus resource [mem 0x01000000-0x01efffff]
> [    0.432281] pci_bus 0000:00: root bus resource [bus 00-ff]
> [    0.432302] pci_bus 0000:00: scanning bus
> [    0.432443] pci 0000:00:00.0: [16c3:abcd] type 01 class 0x060400
> [    0.432526] pci 0000:00:00.0: reg 0x10: [mem 0x00000000-0x000fffff]
> [    0.432573] pci 0000:00:00.0: reg 0x38: [mem 0x00000000-0x0000ffff pref]
> [    0.432649] pci 0000:00:00.0: calling pci_fixup_ide_bases+0x0/0x48
> [    0.432789] pci 0000:00:00.0: supports D1
> [    0.432806] pci 0000:00:00.0: PME# supported from D0 D1 D3hot D3cold
> [    0.432876] pci 0000:00:00.0: PME# disabled
> [    0.433513] pci_bus 0000:00: fixups for bus
> [    0.433547] PCI: bus0: Fast back to back transfers disabled
> [    0.433570] pci 0000:00:00.0: scanning [bus 01-01] behind bridge, pass 0
> [    0.433604] pci 0000:00:00.0: scanning [bus 00-00] behind bridge, pass 1
> [    0.433914] pci 0000:00:00.0: PCI bridge to [bus 01-ff]
> [    0.433942] pci 0000:00:00.0:   bridge window [io  0x0000-0x0fff]
> [    0.433965] pci 0000:00:00.0:   bridge window [mem 0x01000000-0x01ffffff]
> [    0.433989] pci 0000:00:00.0:   bridge window [mem
> 0x00000000-0x000fffff pref]
> [    0.434004] pci_bus 0000:01: scanning bus
> [    0.434129] pci 0000:01:00.0: [1677:affe] type 00 class 0x088030
> [    0.434326] pci 0000:01:00.0: reg 0x10: [mem 0x00000000-0x00000fff]
> [    0.434390] pci 0000:01:00.0: reg 0x14: [mem 0x00000000-0x00003fff]
> [    0.434450] pci 0000:01:00.0: reg 0x18: [mem 0x00000000-0x001fffff]
> [    0.434698] pci 0000:01:00.0: calling pci_fixup_ide_bases+0x0/0x48
> [    0.435397] pci_bus 0000:01: fixups for bus
> [    0.435460] pci 0000:00:00.0: can't claim BAR 7 [io
> 0x0000-0x0fff]: no compatible bridge window
> [    0.435479] pci 0000:00:00.0: can't claim BAR 8 [mem
> 0x01000000-0x01ffffff]: no compatible bridge window
> [    0.435497] pci 0000:00:00.0: can't claim BAR 9 [mem
> 0x00000000-0x000fffff pref]: no compatible bridge window

This is additional noise. Do we really think they are useful messages ?
I still think that the bridge resources claiming on !PCI_PROBE_ONLY
systems is arguable and can trigger regressions.

Thanks,
Lorenzo

> 
> [    0.436128] pci 0000:00:00.0: PCI bridge to [bus 01]
> [    0.436153] pci 0000:00:00.0:   bridge window [mem 0x01100000-0x013fffff]
> 
> don't worry about the increased size of
> pci 0000:01:00.0: BAR 0: assigned [mem 0x01104000-0x01104fff]
> in between I've changed my FPGA device.
> 
> best regards,
> Hannes
> 
> 
> On Thu, Sep 3, 2015 at 12:03 PM, oe5hpm <oe5hpm@gmail.com> wrote:
> > hi bjorn, Lorenzo,
> >
> > should i do some testing with patch from yesterday?
> > please let me know if can test anything.
> >
> > best regards,
> > Hannes
> >
> >
> > On Wed, Sep 2, 2015 at 10:32 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> >> Hi Lorenzo, thanks for jumping on this!
> >>
> >> On Wed, Sep 02, 2015 at 06:47:16PM +0100, Lorenzo Pieralisi wrote:
> >>> Hi Hannes,
> >>>
> >>> On Wed, Sep 02, 2015 at 10:51:18AM +0100, oe5hpm wrote:
> >>> > Hi Lorenzo,
> >>> >
> >>> > today i tried to boot up the most recent vanilla kernel on my
> >>> > Freescale i.mx6 board.
> >>> > I ran into trouble regarding PCI enumeration.
> >>> >
> >>> > [    0.431949] imx6q-pcie 1ffc000.pcie: PCI host bridge to bus 0000:00
> >>> > [    0.431976] pci_bus 0000:00: root bus resource [io  0x1000-0xffff]
> >>> > [    0.431996] pci_bus 0000:00: root bus resource [mem 0x01000000-0x01efffff]
> >>> > [    0.432022] pci_bus 0000:00: root bus resource [bus 00-ff]
> >>> > [    0.433271] PCI: bus0: Fast back to back transfers disabled
> >>> > [    0.433629] pci 0000:00:00.0: PCI bridge to [bus 01-ff]
> >>> > [    0.435181] PCI: bus1: Fast back to back transfers disabled
> >>> > [    0.435564] pci 0000:00:00.0: BAR 0: assigned [mem 0x01000000-0x010fffff]
> >>> > [    0.435593] pci 0000:00:00.0: BAR 8: no space for [mem size 0x01000000]
> >>> > [    0.435613] pci 0000:00:00.0: BAR 8: failed to assign [mem size 0x01000000]
> >>> > [    0.435635] pci 0000:00:00.0: BAR 9: assigned [mem
> >>> > 0x01100000-0x011fffff pref]
> >>> > [    0.435655] pci 0000:00:00.0: BAR 6: assigned [mem
> >>> > 0x01200000-0x0120ffff pref]
> >>> > [    0.435676] pci 0000:00:00.0: BAR 7: assigned [io  0x1000-0x1fff]
> >>> > [    0.435705] pci 0000:01:00.0: BAR 2: no space for [mem size 0x00200000]
> >>> > [    0.435722] pci 0000:01:00.0: BAR 2: failed to assign [mem size 0x00200000]
> >>> > [    0.435739] pci 0000:01:00.0: BAR 1: no space for [mem size 0x00004000]
> >>> > [    0.435754] pci 0000:01:00.0: BAR 1: failed to assign [mem size 0x00004000]
> >>> > [    0.435770] pci 0000:01:00.0: BAR 0: no space for [mem size 0x00000100]
> >>> > [    0.435786] pci 0000:01:00.0: BAR 0: failed to assign [mem size 0x00000100]
> >>> > [    0.435804] pci 0000:00:00.0: PCI bridge to [bus 01]
> >>> > [    0.435826] pci 0000:00:00.0:   bridge window [io  0x1000-0x1fff]
> >>> > [    0.435855] pci 0000:00:00.0:   bridge window [mem
> >>> > 0x01100000-0x011fffff pref]
> >>> >
> >>> > there are several fails assigning memory ressources to pci-devices.
> >>> >
> >>> > i bisect down this trouble to commit id:
> >>> > dff22d2054b5dbb1889f20c03959dd0c494fab8c : PCI: Call
> >>> > pci_read_bridge_bases() from core instead of arch code
> >>> >
> >>> > For testing purpose i've reverted this commit on a local branch and
> >>> > everythings works fine, as before.
> >>> >
> >>> > [    0.431976] imx6q-pcie 1ffc000.pcie: PCI host bridge to bus 0000:00
> >>> > [    0.432004] pci_bus 0000:00: root bus resource [io  0x1000-0xffff]
> >>> > [    0.432023] pci_bus 0000:00: root bus resource [mem 0x01000000-0x01efffff]
> >>> > [    0.432047] pci_bus 0000:00: root bus resource [bus 00-ff]
> >>> > [    0.433302] PCI: bus0: Fast back to back transfers disabled
> >>> > [    0.435122] PCI: bus1: Fast back to back transfers disabled
> >>> > [    0.435504] pci 0000:00:00.0: BAR 0: assigned [mem 0x01000000-0x010fffff]
> >>> > [    0.435535] pci 0000:00:00.0: BAR 8: assigned [mem 0x01100000-0x013fffff]
> >>> > [    0.435557] pci 0000:00:00.0: BAR 6: assigned [mem
> >>> > 0x01400000-0x0140ffff pref]
> >>> > [    0.435585] pci 0000:01:00.0: BAR 2: assigned [mem 0x01200000-0x013fffff]
> >>> > [    0.435626] pci 0000:01:00.0: BAR 1: assigned [mem 0x01100000-0x01103fff]
> >>> > [    0.435665] pci 0000:01:00.0: BAR 0: assigned [mem 0x01104000-0x011040ff]
> >>> > [    0.435703] pci 0000:00:00.0: PCI bridge to [bus 01]
> >>> > [    0.435728] pci 0000:00:00.0:   bridge window [mem 0x01100000-0x013fffff]
> >>> >
> >>> > Further i can break down the failure to "drivers/pci/probe.c" line #924.
> >>> > If i comment out the "pci_read_bridge_bases(child);" also everything works well.
> >>> >
> >>> > I have to confess, that my knowledge about the whole PCI thing in the
> >>> > kernel is not very deep, so it is not possible for me to figure out
> >>> > what is going wrong.
> >>>
> >>> It looks like a bogus bridge aperture size is causing this to happen,
> >>> and this prevents reassignment on arm (bridge aperture is too big),
> >>> which proves that reading the bridge bases without vetting the corresponding
> >>> resources may break (on platforms that were not reading them before).
> >>>
> >>> arm was the only platform not reading the bridge bases, here is an
> >>> answer why. So, to prevent reverting the commit I put together this
> >>> patch (to be reworked if we deem it reasonable), subject to discussion
> >>> (I fear it may end up breaking other arm platforms, I do not have all
> >>> ARM boards and required host controllers to test, I managed to test it on
> >>> an iMX6 Sabrelite though).
> >>>
> >>> Here, please let me know if it works for you, I will keep on thinking
> >>> to find the best solution.
> >>>
> >>> I will have to do this for arm64 too, comments very welcome.
> >>>
> >>> Thanks,
> >>> Lorenzo
> >>>
> >>> -- >8 --
> >>> Subject: [PATCH] arm: kernel: pci: fixup erroneous PCI bridge apertures
> >>>
> >>> Bridge apertures read by core PCI code through pci_read_bridge_bases()
> >>> might be erroneous (bogus platform setup). If the arch code does not vet
> >>> the bridge resources (ie by trying to claim them), we can end up in a
> >>> situation where wrong bridge apertures can prevent resources assignment
> >>> for downstream devices causing enumeration failures (eg a bridge
> >>> aperture does not fit in the respective host controller resource window,
> >>> so it can't be assigned).
> >>>
> >>> This patch adds arm arch code that vets bridge resources by trying
> >>> to claim them, and reset them on claiming failure so that they can
> >>> be properly reassigned.
> >>
> >> We definitely should not depend on the platform to set up the bridge
> >> windows.  Do we know what the platform left in the 00:00.0 window
> >> registers?
> >>
> >> I see that bus 01 requires 0x204100 of mem space, which must be
> >> rounded up to a megabyte boundary, so the window must be at least 3M
> >> (0x00300000):
> >>
> >>   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]
> >>
> >> I don't understand the connection with dff22d2054b5 yet.  If we don't
> >> call pci_read_bridge_bases(), apparently some assign-resources path
> >> figures out the required size and assigns a 3M window.
> >>
> >> If we *do* call pci_read_bridge_bases(), do we read a bogus 16M window
> >> size, fail to assign that because the host controller window isn't big
> >> enough, and then the assign-resources path just gives up?  I assume
> >> clearing r->flags in your patch is the critical thing?  Is there
> >> something in assign-resources that checks for r->flags == 0?
> >>
> >> I think it would be ideal if we could someday claim the resource
> >> immediately, as soon as we read it from a BAR or bridge window, and
> >> mark it as IORESOURCE_UNSET if claiming it fails.  Then if the
> >> platform set up reasonable windows, we could use them; if it didn't,
> >> we could just assign our own.
> >>
> >> I'd like to avoid adding things to pcibios_fixup_bus() if possible
> >> because most of what is done there is arch-independent, and I'd like
> >> to get the arch-independent stuff into the PCI core.
> >>
> >> Bjorn
> >>
> >>> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> >>> ---
> >>>  arch/arm/kernel/bios32.c | 24 ++++++++++++++++++++++++
> >>>  1 file changed, 24 insertions(+)
> >>>
> >>> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> >>> index 874e182..ebbe052 100644
> >>> --- a/arch/arm/kernel/bios32.c
> >>> +++ b/arch/arm/kernel/bios32.c
> >>> @@ -282,6 +282,27 @@ static inline int pdev_bad_for_parity(struct pci_dev *dev)
> >>>
> >>>  }
> >>>
> >>> +static void pcibios_fixup_bridge_resources(struct pci_dev *dev)
> >>> +{
> >>> +     int idx;
> >>> +
> >>> +     if (!dev->bus)
> >>> +             return;
> >>> +
> >>> +     for (idx = PCI_BRIDGE_RESOURCES; idx < PCI_NUM_RESOURCES; idx++) {
> >>> +             struct resource *r = &dev->resource[idx];
> >>> +
> >>> +             if (!r->flags || r->parent)
> >>> +                     continue;
> >>> +
> >>> +             if (pci_claim_resource(dev, idx)) {
> >>> +                     r->flags = 0;
> >>> +                     r->start = 0;
> >>> +                     r->end = -1;
> >>> +             }
> >>> +     }
> >>> +}
> >>> +
> >>>  /*
> >>>   * pcibios_fixup_bus - Called after each bus is probed,
> >>>   * but before its children are examined.
> >>> @@ -352,6 +373,9 @@ void pcibios_fixup_bus(struct pci_bus *bus)
> >>>                       bus->bridge_ctl |= PCI_BRIDGE_CTL_PARITY;
> >>>       }
> >>>
> >>> +     if (bus->self)
> >>> +             pcibios_fixup_bridge_resources(bus->self);
> >>> +
> >>>       /*
> >>>        * Report what we did for this bus
> >>>        */
> >>> --
> >>> 1.9.1
> >>>
> >>>
>