When a PCI bus is scanned, upon PCI bridge detection the kernel
has to read the bridge registers to set-up its resources so that
the PCI resource hierarchy can be validated properly.
Most if not all architectures read PCI bridge registers in the
pcibios_fixup_bus hook, that is called by the PCI generic layer
whenever a PCI bus is scanned.
Since pci_read_bridge_bases is an arch agnostic operation (and it
is carried out on all architectures) it can be moved to the generic
PCI layer in order to consolidate code and remove the respective
calls from the architectures back-ends.
The PCI_PROBE_ONLY flag is not checked before calling
pci_read_bridge_buses in the generic layer since reading the bridge
bases is not related to resources assignment; this implies that it
can be carried out safely on PCI_PROBE_ONLY systems too and should
not affect architectures (alpha, mips) that check the PCI_PROBE_ONLY
flag before reading the bridge bases.
In order to validate the resource hierarchy as soon as the resources
themselves are probed (ie read from the bridge), this patch also adds
code to pci_read_bridge_bases that claims the bridge resources, so that
they are validated and inserted in the resource hierarchy as soon as
the bridge bases are probed.
Signed-off-by: Lorenzo Pieralisi <[email protected]>
Cc: Ralf Baechle <[email protected]>
Cc: James E.J. Bottomley <[email protected]>
Cc: Michael Ellerman <[email protected]>
Cc: Bjorn Helgaas <[email protected]>
Cc: Richard Henderson <[email protected]>
Cc: Benjamin Herrenschmidt <[email protected]>
Cc: David Howells <[email protected]>
Cc: Russell King <[email protected]>
Cc: Tony Luck <[email protected]>
Cc: David S. Miller <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Guenter Roeck <[email protected]>
Cc: Michal Simek <[email protected]>
Cc: Chris Zankel <[email protected]>
---
v1->v2:
- Moved pci_read_bridge_bases call to pci_scan_bridge to read bases before
scanning devices
- Added bridge resources claiming
v1: https://lkml.org/lkml/2015/5/21/359
arch/alpha/kernel/pci.c | 7 +------
arch/frv/mb93090-mb00/pci-vdk.c | 2 --
arch/ia64/pci/pci.c | 1 -
arch/microblaze/pci/pci-common.c | 9 +--------
arch/mips/pci/pci.c | 6 ------
arch/mn10300/unit-asb2305/pci.c | 1 -
arch/powerpc/kernel/pci-common.c | 8 +-------
arch/x86/pci/common.c | 1 -
arch/xtensa/kernel/pci.c | 4 ----
drivers/parisc/dino.c | 3 ---
drivers/parisc/lba_pci.c | 1 -
drivers/pci/probe.c | 26 ++++++++++++++++++++++++++
12 files changed, 29 insertions(+), 40 deletions(-)
diff --git a/arch/alpha/kernel/pci.c b/arch/alpha/kernel/pci.c
index 82f738e..cded02c 100644
--- a/arch/alpha/kernel/pci.c
+++ b/arch/alpha/kernel/pci.c
@@ -242,12 +242,7 @@ pci_restore_srm_config(void)
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);
- }
+ struct pci_dev *dev;
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 f211839..f9c86c4 100644
--- a/arch/frv/mb93090-mb00/pci-vdk.c
+++ b/arch/frv/mb93090-mb00/pci-vdk.c
@@ -294,8 +294,6 @@ 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 7cc3be9..563228b 100644
--- a/arch/ia64/pci/pci.c
+++ b/arch/ia64/pci/pci.c
@@ -534,7 +534,6 @@ void pcibios_fixup_bus(struct pci_bus *b)
struct pci_dev *dev;
if (b->self) {
- pci_read_bridge_bases(b);
pcibios_fixup_bridge_resources(b->self);
}
list_for_each_entry(dev, &b->devices, bus_list)
diff --git a/arch/microblaze/pci/pci-common.c b/arch/microblaze/pci/pci-common.c
index ae838ed..6b8b752 100644
--- a/arch/microblaze/pci/pci-common.c
+++ b/arch/microblaze/pci/pci-common.c
@@ -863,14 +863,7 @@ void pcibios_setup_bus_devices(struct pci_bus *bus)
void pcibios_fixup_bus(struct pci_bus *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 */
+ /* Fixup the 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 b8a0bf5..c6996cf 100644
--- a/arch/mips/pci/pci.c
+++ b/arch/mips/pci/pci.c
@@ -311,12 +311,6 @@ 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 3dfe2d3..deaa893 100644
--- a/arch/mn10300/unit-asb2305/pci.c
+++ b/arch/mn10300/unit-asb2305/pci.c
@@ -324,7 +324,6 @@ 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 0d05406..722dd5f 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -1043,13 +1043,7 @@ void pcibios_set_master(struct pci_dev *dev)
void pcibios_fixup_bus(struct pci_bus *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 */
+ /* Fixup the 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 8fd6f44..3bff244 100644
--- a/arch/x86/pci/common.c
+++ b/arch/x86/pci/common.c
@@ -166,7 +166,6 @@ 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 b848cc3..d27b4dc 100644
--- a/arch/xtensa/kernel/pci.c
+++ b/arch/xtensa/kernel/pci.c
@@ -210,10 +210,6 @@ 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 a0580af..baec33c 100644
--- a/drivers/parisc/dino.c
+++ b/drivers/parisc/dino.c
@@ -560,9 +560,6 @@ 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 dceb9dd..901e1a3 100644
--- a/drivers/parisc/lba_pci.c
+++ b/drivers/parisc/lba_pci.c
@@ -693,7 +693,6 @@ 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 6675a7a..1913e1b 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -332,6 +332,21 @@ static void pci_read_bases(struct pci_dev *dev, unsigned int howmany, int rom)
}
}
+static void pci_claim_bridge_resources(struct pci_bus *bus)
+{
+ struct pci_dev *dev = bus->self;
+ int idx;
+
+ for (idx = PCI_BRIDGE_RESOURCES; idx < PCI_NUM_RESOURCES; idx++) {
+ struct resource *r = &dev->resource[idx];
+
+ if (!r->flags || r->parent)
+ continue;
+
+ pci_claim_bridge_resource(dev, idx);
+ }
+}
+
static void pci_read_bridge_io(struct pci_bus *child)
{
struct pci_dev *dev = child->self;
@@ -479,6 +494,8 @@ void pci_read_bridge_bases(struct pci_bus *child)
}
}
}
+
+ pci_claim_bridge_resources(child);
}
static struct pci_bus *pci_alloc_bus(struct pci_bus *parent)
@@ -826,6 +843,11 @@ 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",
@@ -886,6 +908,10 @@ 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 {
/*
--
2.2.1
For ARM64 PROBE_ONLY and non-PROBE_ONLY modes:
Reviewed and Tested-by: Suravee Suthikulpanit
<[email protected]>
Please see minor comments below.
On 6/9/2015 4:01 AM, Lorenzo Pieralisi wrote:
> When a PCI bus is scanned, upon PCI bridge detection the kernel
> has to read the bridge registers to set-up its resources so that
> the PCI resource hierarchy can be validated properly.
>
> Most if not all architectures read PCI bridge registers in the
> pcibios_fixup_bus hook, that is called by the PCI generic layer
> whenever a PCI bus is scanned.
>
> Since pci_read_bridge_bases is an arch agnostic operation (and it
> is carried out on all architectures) it can be moved to the generic
> PCI layer in order to consolidate code and remove the respective
> calls from the architectures back-ends.
>
> The PCI_PROBE_ONLY flag is not checked before calling
> pci_read_bridge_buses in the generic layer since reading the bridge
> bases is not related to resources assignment; this implies that it
> can be carried out safely on PCI_PROBE_ONLY systems too and should
> not affect architectures (alpha, mips) that check the PCI_PROBE_ONLY
> flag before reading the bridge bases.
>
> In order to validate the resource hierarchy as soon as the resources
> themselves are probed (ie read from the bridge), this patch also adds
> code to pci_read_bridge_bases that claims the bridge resources, so that
> they are validated and inserted in the resource hierarchy as soon as
> the bridge bases are probed.
>
> Signed-off-by: Lorenzo Pieralisi <[email protected]>
> Cc: Ralf Baechle <[email protected]>
> Cc: James E.J. Bottomley <[email protected]>
> Cc: Michael Ellerman <[email protected]>
> Cc: Bjorn Helgaas <[email protected]>
> Cc: Richard Henderson <[email protected]>
> Cc: Benjamin Herrenschmidt <[email protected]>
> Cc: David Howells <[email protected]>
> Cc: Russell King <[email protected]>
> Cc: Tony Luck <[email protected]>
> Cc: David S. Miller <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Guenter Roeck <[email protected]>
> Cc: Michal Simek <[email protected]>
> Cc: Chris Zankel <[email protected]>
> ---
> v1->v2:
>
> - Moved pci_read_bridge_bases call to pci_scan_bridge to read bases before
> scanning devices
> - Added bridge resources claiming
>
> v1: https://lkml.org/lkml/2015/5/21/359
>
> arch/alpha/kernel/pci.c | 7 +------
> arch/frv/mb93090-mb00/pci-vdk.c | 2 --
> arch/ia64/pci/pci.c | 1 -
> arch/microblaze/pci/pci-common.c | 9 +--------
> arch/mips/pci/pci.c | 6 ------
> arch/mn10300/unit-asb2305/pci.c | 1 -
> arch/powerpc/kernel/pci-common.c | 8 +-------
> arch/x86/pci/common.c | 1 -
> arch/xtensa/kernel/pci.c | 4 ----
> drivers/parisc/dino.c | 3 ---
> drivers/parisc/lba_pci.c | 1 -
> drivers/pci/probe.c | 26 ++++++++++++++++++++++++++
> 12 files changed, 29 insertions(+), 40 deletions(-)
>
> diff --git a/arch/alpha/kernel/pci.c b/arch/alpha/kernel/pci.c
> index 82f738e..cded02c 100644
> --- a/arch/alpha/kernel/pci.c
> +++ b/arch/alpha/kernel/pci.c
> @@ -242,12 +242,7 @@ pci_restore_srm_config(void)
>
> 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);
> - }
> + struct pci_dev *dev;
>
> 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 f211839..f9c86c4 100644
> --- a/arch/frv/mb93090-mb00/pci-vdk.c
> +++ b/arch/frv/mb93090-mb00/pci-vdk.c
> @@ -294,8 +294,6 @@ 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 7cc3be9..563228b 100644
> --- a/arch/ia64/pci/pci.c
> +++ b/arch/ia64/pci/pci.c
> @@ -534,7 +534,6 @@ void pcibios_fixup_bus(struct pci_bus *b)
> struct pci_dev *dev;
>
> if (b->self) {
> - pci_read_bridge_bases(b);
> pcibios_fixup_bridge_resources(b->self);
> }
> list_for_each_entry(dev, &b->devices, bus_list)
> diff --git a/arch/microblaze/pci/pci-common.c b/arch/microblaze/pci/pci-common.c
> index ae838ed..6b8b752 100644
> --- a/arch/microblaze/pci/pci-common.c
> +++ b/arch/microblaze/pci/pci-common.c
> @@ -863,14 +863,7 @@ void pcibios_setup_bus_devices(struct pci_bus *bus)
>
> void pcibios_fixup_bus(struct pci_bus *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 */
> + /* Fixup the 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 b8a0bf5..c6996cf 100644
> --- a/arch/mips/pci/pci.c
> +++ b/arch/mips/pci/pci.c
> @@ -311,12 +311,6 @@ 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 3dfe2d3..deaa893 100644
> --- a/arch/mn10300/unit-asb2305/pci.c
> +++ b/arch/mn10300/unit-asb2305/pci.c
> @@ -324,7 +324,6 @@ 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 0d05406..722dd5f 100644
> --- a/arch/powerpc/kernel/pci-common.c
> +++ b/arch/powerpc/kernel/pci-common.c
> @@ -1043,13 +1043,7 @@ void pcibios_set_master(struct pci_dev *dev)
>
> void pcibios_fixup_bus(struct pci_bus *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 */
> + /* Fixup the 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 8fd6f44..3bff244 100644
> --- a/arch/x86/pci/common.c
> +++ b/arch/x86/pci/common.c
> @@ -166,7 +166,6 @@ 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 b848cc3..d27b4dc 100644
> --- a/arch/xtensa/kernel/pci.c
> +++ b/arch/xtensa/kernel/pci.c
> @@ -210,10 +210,6 @@ 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 a0580af..baec33c 100644
> --- a/drivers/parisc/dino.c
> +++ b/drivers/parisc/dino.c
> @@ -560,9 +560,6 @@ 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 dceb9dd..901e1a3 100644
> --- a/drivers/parisc/lba_pci.c
> +++ b/drivers/parisc/lba_pci.c
> @@ -693,7 +693,6 @@ 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 6675a7a..1913e1b 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -332,6 +332,21 @@ static void pci_read_bases(struct pci_dev *dev, unsigned int howmany, int rom)
> }
> }
>
> +static void pci_claim_bridge_resources(struct pci_bus *bus)
> +{
> + struct pci_dev *dev = bus->self;
> + int idx;
> +
> + for (idx = PCI_BRIDGE_RESOURCES; idx < PCI_NUM_RESOURCES; idx++) {
> + struct resource *r = &dev->resource[idx];
> +
> + if (!r->flags || r->parent)
> + continue;
> +
> + pci_claim_bridge_resource(dev, idx);
> + }
> +}
> +
Nitpicking: Since pci_claim_bridge_resources() is small, and only called
once from pci_read_brdige_bases(), should we just put the loop inside
the function?
> static void pci_read_bridge_io(struct pci_bus *child)
> {
> struct pci_dev *dev = child->self;
> @@ -479,6 +494,8 @@ void pci_read_bridge_bases(struct pci_bus *child)
> }
> }
> }
> +
> + pci_claim_bridge_resources(child);
> }
>
> static struct pci_bus *pci_alloc_bus(struct pci_bus *parent)
> @@ -826,6 +843,11 @@ 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",
> @@ -886,6 +908,10 @@ 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 {
> /*
>
On 06/09/2015 02:01 AM, Lorenzo Pieralisi wrote:
> When a PCI bus is scanned, upon PCI bridge detection the kernel
> has to read the bridge registers to set-up its resources so that
> the PCI resource hierarchy can be validated properly.
>
> Most if not all architectures read PCI bridge registers in the
> pcibios_fixup_bus hook, that is called by the PCI generic layer
> whenever a PCI bus is scanned.
>
> Since pci_read_bridge_bases is an arch agnostic operation (and it
> is carried out on all architectures) it can be moved to the generic
> PCI layer in order to consolidate code and remove the respective
> calls from the architectures back-ends.
>
> The PCI_PROBE_ONLY flag is not checked before calling
> pci_read_bridge_buses in the generic layer since reading the bridge
> bases is not related to resources assignment; this implies that it
> can be carried out safely on PCI_PROBE_ONLY systems too and should
> not affect architectures (alpha, mips) that check the PCI_PROBE_ONLY
> flag before reading the bridge bases.
>
> In order to validate the resource hierarchy as soon as the resources
> themselves are probed (ie read from the bridge), this patch also adds
> code to pci_read_bridge_bases that claims the bridge resources, so that
> they are validated and inserted in the resource hierarchy as soon as
> the bridge bases are probed.
>
> Signed-off-by: Lorenzo Pieralisi <[email protected]>
> Cc: Ralf Baechle <[email protected]>
> Cc: James E.J. Bottomley <[email protected]>
> Cc: Michael Ellerman <[email protected]>
> Cc: Bjorn Helgaas <[email protected]>
> Cc: Richard Henderson <[email protected]>
> Cc: Benjamin Herrenschmidt <[email protected]>
> Cc: David Howells <[email protected]>
> Cc: Russell King <[email protected]>
> Cc: Tony Luck <[email protected]>
> Cc: David S. Miller <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Guenter Roeck <[email protected]>
> Cc: Michal Simek <[email protected]>
> Cc: Chris Zankel <[email protected]>
Working fine on powerpc (tested with Freescale P2020 and P5020) as well as x86
(tested on two different server class systems).
Tested-by: Guenter Roeck <[email protected]>
Guenter
On Tue, Jun 09, 2015 at 10:01:45AM +0100, Lorenzo Pieralisi wrote:
> When a PCI bus is scanned, upon PCI bridge detection the kernel
> has to read the bridge registers to set-up its resources so that
> the PCI resource hierarchy can be validated properly.
>
> Most if not all architectures read PCI bridge registers in the
> pcibios_fixup_bus hook, that is called by the PCI generic layer
> whenever a PCI bus is scanned.
>
> Since pci_read_bridge_bases is an arch agnostic operation (and it
> is carried out on all architectures) it can be moved to the generic
> PCI layer in order to consolidate code and remove the respective
> calls from the architectures back-ends.
>
> The PCI_PROBE_ONLY flag is not checked before calling
> pci_read_bridge_buses in the generic layer since reading the bridge
> bases is not related to resources assignment; this implies that it
> can be carried out safely on PCI_PROBE_ONLY systems too and should
> not affect architectures (alpha, mips) that check the PCI_PROBE_ONLY
> flag before reading the bridge bases.
>
> In order to validate the resource hierarchy as soon as the resources
> themselves are probed (ie read from the bridge), this patch also adds
> code to pci_read_bridge_bases that claims the bridge resources, so that
> they are validated and inserted in the resource hierarchy as soon as
> the bridge bases are probed.
>
Hi Lorenzo,
on one of our systems, I see a lot of messages with your patch applied.
bart kernel: pci 0000:b0:00.0: can't claim BAR 7 [io 0x0000-0x0fff]: no compatible bridge window
bart kernel: pci 0000:b0:00.0: can't claim BAR 8 [mem 0x94000000-0x941fffff]: no compatible bridge window
bart kernel: pci 0000:b1:03.0: can't claim BAR 7 [io 0x0000-0x0fff]: no compatible bridge window
bart kernel: pci 0000:b1:03.0: can't claim BAR 8 [mem 0x95800000-0x959fffff]: no compatible bridge window
and so on. The final IO memory assignment is the same, though,
before and after your patch.
95800000-95bfffff : PCI Bus 0000:b0
95800000-959fffff : PCI Bus 0000:b1
95800000-959fffff : PCI Bus 0000:b2
95a00000-95a3ffff : 0000:b0:00.0
Does that have any relevance or is it just nuisance messages ?
Thanks,
Guenter
On Sat, Jun 13, 2015 at 02:47:55AM +0100, Guenter Roeck wrote:
> On Tue, Jun 09, 2015 at 10:01:45AM +0100, Lorenzo Pieralisi wrote:
> > When a PCI bus is scanned, upon PCI bridge detection the kernel
> > has to read the bridge registers to set-up its resources so that
> > the PCI resource hierarchy can be validated properly.
> >
> > Most if not all architectures read PCI bridge registers in the
> > pcibios_fixup_bus hook, that is called by the PCI generic layer
> > whenever a PCI bus is scanned.
> >
> > Since pci_read_bridge_bases is an arch agnostic operation (and it
> > is carried out on all architectures) it can be moved to the generic
> > PCI layer in order to consolidate code and remove the respective
> > calls from the architectures back-ends.
> >
> > The PCI_PROBE_ONLY flag is not checked before calling
> > pci_read_bridge_buses in the generic layer since reading the bridge
> > bases is not related to resources assignment; this implies that it
> > can be carried out safely on PCI_PROBE_ONLY systems too and should
> > not affect architectures (alpha, mips) that check the PCI_PROBE_ONLY
> > flag before reading the bridge bases.
> >
> > In order to validate the resource hierarchy as soon as the resources
> > themselves are probed (ie read from the bridge), this patch also adds
> > code to pci_read_bridge_bases that claims the bridge resources, so that
> > they are validated and inserted in the resource hierarchy as soon as
> > the bridge bases are probed.
> >
>
> Hi Lorenzo,
>
> on one of our systems, I see a lot of messages with your patch applied.
>
> bart kernel: pci 0000:b0:00.0: can't claim BAR 7 [io 0x0000-0x0fff]: no compatible bridge window
> bart kernel: pci 0000:b0:00.0: can't claim BAR 8 [mem 0x94000000-0x941fffff]: no compatible bridge window
> bart kernel: pci 0000:b1:03.0: can't claim BAR 7 [io 0x0000-0x0fff]: no compatible bridge window
> bart kernel: pci 0000:b1:03.0: can't claim BAR 8 [mem 0x95800000-0x959fffff]: no compatible bridge window
>
> and so on. The final IO memory assignment is the same, though,
> before and after your patch.
>
> 95800000-95bfffff : PCI Bus 0000:b0
> 95800000-959fffff : PCI Bus 0000:b1
> 95800000-959fffff : PCI Bus 0000:b2
> 95a00000-95a3ffff : 0000:b0:00.0
>
>
> Does that have any relevance or is it just nuisance messages ?
Yes, I knew this could happen. It should be just nuisance messages,
since we are claming bridge resources even on systems where they
are reassigned. We should remove those messages, this means that I
have to craft a function that claims resources without spitting too
much unwanted noise, I can't use pci_claim_bridge_resource for this
purpose as you have noticed, unless I refactor it, open to suggestions
(we claim bridge resources by default, regardless of PROBE_ONLY flag).
Thanks a lot for testing it, appreciated, I will prepare a v3.
Lorenzo
>
> Thanks,
> Guenter
>
On 06/13/2015 02:12 AM, Lorenzo Pieralisi wrote:
> On Sat, Jun 13, 2015 at 02:47:55AM +0100, Guenter Roeck wrote:
>> On Tue, Jun 09, 2015 at 10:01:45AM +0100, Lorenzo Pieralisi wrote:
>>> When a PCI bus is scanned, upon PCI bridge detection the kernel
>>> has to read the bridge registers to set-up its resources so that
>>> the PCI resource hierarchy can be validated properly.
>>>
>>> Most if not all architectures read PCI bridge registers in the
>>> pcibios_fixup_bus hook, that is called by the PCI generic layer
>>> whenever a PCI bus is scanned.
>>>
>>> Since pci_read_bridge_bases is an arch agnostic operation (and it
>>> is carried out on all architectures) it can be moved to the generic
>>> PCI layer in order to consolidate code and remove the respective
>>> calls from the architectures back-ends.
>>>
>>> The PCI_PROBE_ONLY flag is not checked before calling
>>> pci_read_bridge_buses in the generic layer since reading the bridge
>>> bases is not related to resources assignment; this implies that it
>>> can be carried out safely on PCI_PROBE_ONLY systems too and should
>>> not affect architectures (alpha, mips) that check the PCI_PROBE_ONLY
>>> flag before reading the bridge bases.
>>>
>>> In order to validate the resource hierarchy as soon as the resources
>>> themselves are probed (ie read from the bridge), this patch also adds
>>> code to pci_read_bridge_bases that claims the bridge resources, so that
>>> they are validated and inserted in the resource hierarchy as soon as
>>> the bridge bases are probed.
>>>
>>
>> Hi Lorenzo,
>>
>> on one of our systems, I see a lot of messages with your patch applied.
>>
>> bart kernel: pci 0000:b0:00.0: can't claim BAR 7 [io 0x0000-0x0fff]: no compatible bridge window
>> bart kernel: pci 0000:b0:00.0: can't claim BAR 8 [mem 0x94000000-0x941fffff]: no compatible bridge window
>> bart kernel: pci 0000:b1:03.0: can't claim BAR 7 [io 0x0000-0x0fff]: no compatible bridge window
>> bart kernel: pci 0000:b1:03.0: can't claim BAR 8 [mem 0x95800000-0x959fffff]: no compatible bridge window
>>
>> and so on. The final IO memory assignment is the same, though,
>> before and after your patch.
>>
>> 95800000-95bfffff : PCI Bus 0000:b0
>> 95800000-959fffff : PCI Bus 0000:b1
>> 95800000-959fffff : PCI Bus 0000:b2
>> 95a00000-95a3ffff : 0000:b0:00.0
>>
>>
>> Does that have any relevance or is it just nuisance messages ?
>
> Yes, I knew this could happen. It should be just nuisance messages,
> since we are claming bridge resources even on systems where they
> are reassigned. We should remove those messages, this means that I
> have to craft a function that claims resources without spitting too
> much unwanted noise, I can't use pci_claim_bridge_resource for this
> purpose as you have noticed, unless I refactor it, open to suggestions
> (we claim bridge resources by default, regardless of PROBE_ONLY flag).
>
> Thanks a lot for testing it, appreciated, I will prepare a v3.
>
Hi Lorenzo,
There is no need for v3 because of this. My patch set addresses the BAR 7
message, and the BAR 8 message is actually warranted since the memory
window on the upstream port is too small.
Guenter
On Sun, Jun 14, 2015 at 04:29:52PM +0100, Guenter Roeck wrote:
> On 06/13/2015 02:12 AM, Lorenzo Pieralisi wrote:
> > On Sat, Jun 13, 2015 at 02:47:55AM +0100, Guenter Roeck wrote:
> >> On Tue, Jun 09, 2015 at 10:01:45AM +0100, Lorenzo Pieralisi wrote:
> >>> When a PCI bus is scanned, upon PCI bridge detection the kernel
> >>> has to read the bridge registers to set-up its resources so that
> >>> the PCI resource hierarchy can be validated properly.
> >>>
> >>> Most if not all architectures read PCI bridge registers in the
> >>> pcibios_fixup_bus hook, that is called by the PCI generic layer
> >>> whenever a PCI bus is scanned.
> >>>
> >>> Since pci_read_bridge_bases is an arch agnostic operation (and it
> >>> is carried out on all architectures) it can be moved to the generic
> >>> PCI layer in order to consolidate code and remove the respective
> >>> calls from the architectures back-ends.
> >>>
> >>> The PCI_PROBE_ONLY flag is not checked before calling
> >>> pci_read_bridge_buses in the generic layer since reading the bridge
> >>> bases is not related to resources assignment; this implies that it
> >>> can be carried out safely on PCI_PROBE_ONLY systems too and should
> >>> not affect architectures (alpha, mips) that check the PCI_PROBE_ONLY
> >>> flag before reading the bridge bases.
> >>>
> >>> In order to validate the resource hierarchy as soon as the resources
> >>> themselves are probed (ie read from the bridge), this patch also adds
> >>> code to pci_read_bridge_bases that claims the bridge resources, so that
> >>> they are validated and inserted in the resource hierarchy as soon as
> >>> the bridge bases are probed.
> >>>
> >>
> >> Hi Lorenzo,
> >>
> >> on one of our systems, I see a lot of messages with your patch applied.
> >>
> >> bart kernel: pci 0000:b0:00.0: can't claim BAR 7 [io 0x0000-0x0fff]: no compatible bridge window
> >> bart kernel: pci 0000:b0:00.0: can't claim BAR 8 [mem 0x94000000-0x941fffff]: no compatible bridge window
> >> bart kernel: pci 0000:b1:03.0: can't claim BAR 7 [io 0x0000-0x0fff]: no compatible bridge window
> >> bart kernel: pci 0000:b1:03.0: can't claim BAR 8 [mem 0x95800000-0x959fffff]: no compatible bridge window
> >>
> >> and so on. The final IO memory assignment is the same, though,
> >> before and after your patch.
> >>
> >> 95800000-95bfffff : PCI Bus 0000:b0
> >> 95800000-959fffff : PCI Bus 0000:b1
> >> 95800000-959fffff : PCI Bus 0000:b2
> >> 95a00000-95a3ffff : 0000:b0:00.0
> >>
> >>
> >> Does that have any relevance or is it just nuisance messages ?
> >
> > Yes, I knew this could happen. It should be just nuisance messages,
> > since we are claming bridge resources even on systems where they
> > are reassigned. We should remove those messages, this means that I
> > have to craft a function that claims resources without spitting too
> > much unwanted noise, I can't use pci_claim_bridge_resource for this
> > purpose as you have noticed, unless I refactor it, open to suggestions
> > (we claim bridge resources by default, regardless of PROBE_ONLY flag).
> >
> > Thanks a lot for testing it, appreciated, I will prepare a v3.
> >
>
> Hi Lorenzo,
>
> There is no need for v3 because of this. My patch set addresses the BAR 7
> message, and the BAR 8 message is actually warranted since the memory
> window on the upstream port is too small.
I'd agree it is warranted, still we have to expect similar messages
printed on kernel logs for other platforms that are _not_ printed
by current mainline, I guess that's acceptable and it was expected
when we decided to claim bridge resources in core code by default.
I am open to feedback if we want to change this behaviour, I am more
concerned about the patch test coverage to check it does not affect
behaviour on archs we do not have HW to test on.
Thank you,
Lorenzo
On Thu, Jun 11, 2015 at 08:53:46PM +0100, Suravee Suthikulanit wrote:
> For ARM64 PROBE_ONLY and non-PROBE_ONLY modes:
>
> Reviewed and Tested-by: Suravee Suthikulpanit
> <[email protected]>
Thank you !
> Please see minor comments below.
[...]
> > +static void pci_claim_bridge_resources(struct pci_bus *bus)
> > +{
> > + struct pci_dev *dev = bus->self;
> > + int idx;
> > +
> > + for (idx = PCI_BRIDGE_RESOURCES; idx < PCI_NUM_RESOURCES; idx++) {
> > + struct resource *r = &dev->resource[idx];
> > +
> > + if (!r->flags || r->parent)
> > + continue;
> > +
> > + pci_claim_bridge_resource(dev, idx);
> > + }
> > +}
> > +
>
> Nitpicking: Since pci_claim_bridge_resources() is small, and only called
> once from pci_read_brdige_bases(), should we just put the loop inside
> the function?
I think it is ok to add a function for it, I am not fussed either
way I am more concerned by testing the patch on archs I do not have
access to.
Thank you,
Lorenzo