ARM: kernel: bios32: implement PCI device resources claiming

When a device is scanned and added to the PCI bus, its resources should be claimed to validate the BARs configuration and to assign them a parent resource so that the resource hierarchy can be sanity checked.

This patch adds code that carries out PCI device resources claiming to the ARM64 pcibios_add_device implementation so that device resources are claimed by the core PCI layer upon PCI device initialization on ARM64 systems.

Signed-off-by Lorenzo Pieralisi <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Will Deacon <[email protected]>

Cc Liviu Dudau <[email protected]>

Cc Bjorn Helgaas <[email protected]>

---
  arch/arm64/kernel/pci.c | 10 ++++++++++
  1 file changed, 10 insertions(+)

arch/arm64/kernel/pci.c +3 -0

index 4095379..c0a88ca 100644
@@ -43,8 +43,18 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
   */
  int pcibios_add_device(struct pci_dev *dev)
  {
+	struct resource *res;
+	int i;
+
  	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
 

Quoted 33 lines · Lorenzo Pieralisi

When a device is scanned and added to the PCI bus, its resources should be claimed to validate the BARs configuration and to assign them a parent resource so that the resource hierarchy can be sanity checked.

This patch adds code that carries out PCI device resources claiming to the ARM64 pcibios_add_device implementation so that device resources are claimed by the core PCI layer upon PCI device initialization on ARM64 systems.

Signed-off-by Lorenzo Pieralisi <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Will Deacon <[email protected]>

Cc Liviu Dudau <[email protected]>

Cc Bjorn Helgaas <[email protected]>

---
  arch/arm64/kernel/pci.c | 10 ++++++++++
  1 file changed, 10 insertions(+)

arch/arm64/kernel/pci.c +3 -0

index 4095379..c0a88ca 100644
@@ -43,8 +43,18 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
   */
  int pcibios_add_device(struct pci_dev *dev)
  {
+	struct resource *res;
+	int i;
+
  	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
 

arch/arm64/kernel/pci.c resource_size_t pcibios_align_resource(void *data, const struct resource *res, · line 60

Quoted 10 lines

arch/arm64/kernel/pci.c +7 -0

+	for (i = 0; i < PCI_NUM_RESOURCES; i++) {
+		res = &dev->resource[i];
+		if (res->parent || !res->flags)
+			continue;
+		pci_claim_resource(dev, i);
+	}
+
  	return 0;
  }
 

Lorenzo/Bjorn,

I have tested this patch on top of Jayachandran's V2 patch series
(http://www.spinics.net/lists/linux-pci/msg40811.html) on AMD Seattle (w/ PROBE_ONLY and non-PROBE_ONLY mode), and your changes here works with additional changes below.

It seems that when booting w/ PROBE_ONLY case, we need to call pci_read_bridge_bases() at some point before claiming the resources of devices underneath the bridge. This is needed to determine the bridge bases (i.e. bridge io, mmio and mmio_pref bases), and update bridge resources accordingly.

---- BEGIN PATCH -----

arch/arm64/kernel/pci.c +5 -0

index c0a88ca..57be6aa 100644
@@ -48,6 +48,11 @@ int pcibios_add_device(struct pci_dev *dev)
 
         dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
 
+       if (pci_has_flag(PCI_PROBE_ONLY) &&
+           !pci_is_root_bus(dev->bus) &&
+           !pci_bridge_bases_is_read(dev->bus))
+               pci_read_bridge_bases(dev->bus);
+
         for (i = 0; i < PCI_NUM_RESOURCES; i++) {
                 res = &dev->resource[i];
                 if (res->parent || !res->flags)

drivers/pci/probe.c +7 -0

index 6675a7a..6cab8be 100644
@@ -447,6 +447,13 @@ static void pci_read_bridge_mmio_pref(struct 
 pci_bus *child)
         }
  }
 
+bool pci_bridge_bases_is_read(struct pci_bus *bus)
+{
+       return (bus->resource[0]->start ||
+               bus->resource[1]->start ||
+               bus->resource[2]->start);
+}
+
  void pci_read_bridge_bases(struct pci_bus *child)
  {

         struct pci_dev *dev = child->self;

include/linux/pci.h +1 -0

index 353db8d..11c674d 100644
@@ -798,6 +798,7 @@ void pci_device_add(struct pci_dev *dev, struct 
 pci_bus *bus);
  unsigned int pci_scan_child_bus(struct pci_bus *bus);
  void pci_bus_add_device(struct pci_dev *dev);
  void pci_read_bridge_bases(struct pci_bus *child);
+bool pci_bridge_bases_is_read(struct pci_bus *bus);
  struct resource *pci_find_parent_resource(const struct pci_dev *dev,
                                           struct resource *res);

u8 pci_swizzle_interrupt_pin(const struct pci_dev *dev, u8 pin);
---- END PATCH -----

I'm not sure if this is the best place to be reading the bridge bases. I guess we should be able to do this when adding bridge devices.

Thanks,

Suravee

Quoted 12 lines

Lorenzo/Bjorn,

Quoted 11 lines

I have tested this patch on top of Jayachandran's V2 patch series
(http://www.spinics.net/lists/linux-pci/msg40811.html) on AMD Seattle (w/ PROBE_ONLY and non-PROBE_ONLY mode), and your changes here works with additional changes below.

It seems that when booting w/ PROBE_ONLY case, we need to call pci_read_bridge_bases() at some point before claiming the resources of devices underneath the bridge. This is needed to determine the bridge bases (i.e. bridge io, mmio and mmio_pref bases), and update bridge resources accordingly.

Thanks for testing, I will give it a go on Seattle, if you can drop the log you get on PROBE_ONLY (without your patch below) that would be great so that I can have a look at the issue.

Thanks,
Lorenzo

Quoted 33 lines · Lorenzo Pieralisi

When a device is scanned and added to the PCI bus, its resources should be claimed to validate the BARs configuration and to assign them a parent resource so that the resource hierarchy can be sanity checked.

This patch adds code that carries out PCI device resources claiming to the ARM64 pcibios_add_device implementation so that device resources are claimed by the core PCI layer upon PCI device initialization on ARM64 systems.

Signed-off-by Lorenzo Pieralisi <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Will Deacon <[email protected]>

Cc Liviu Dudau <[email protected]>

Cc Bjorn Helgaas <[email protected]>

---
  arch/arm64/kernel/pci.c | 10 ++++++++++
  1 file changed, 10 insertions(+)

arch/arm64/kernel/pci.c +3 -0

index 4095379..c0a88ca 100644
@@ -43,8 +43,18 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
   */
  int pcibios_add_device(struct pci_dev *dev)
  {
+	struct resource *res;
+	int i;
+
  	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
 

arch/arm64/kernel/pci.c resource_size_t pcibios_align_resource(void *data, const struct resource *res, · line 60

Quoted 10 lines

arch/arm64/kernel/pci.c +7 -0

+	for (i = 0; i < PCI_NUM_RESOURCES; i++) {
+		res = &dev->resource[i];
+		if (res->parent || !res->flags)
+			continue;
+		pci_claim_resource(dev, i);
+	}
+
  	return 0;
  }
 

Lorenzo/Bjorn,

I have tested this patch on top of Jayachandran's V2 patch series
(http://www.spinics.net/lists/linux-pci/msg40811.html) on AMD Seattle (w/ PROBE_ONLY and non-PROBE_ONLY mode), and your changes here works with additional changes below.

It seems that when booting w/ PROBE_ONLY case, we need to call pci_read_bridge_bases() at some point before claiming the resources of devices underneath the bridge. This is needed to determine the bridge bases (i.e. bridge io, mmio and mmio_pref bases), and update bridge resources accordingly.

---- BEGIN PATCH -----

arch/arm64/kernel/pci.c +0 -0

index c0a88ca..57be6aa 100644

arch/arm64/kernel/pci.c +0 -0

         for (i = 0; i < PCI_NUM_RESOURCES; i++) {
                 res = &dev->resource[i];
                 if (res->parent || !res->flags)

drivers/pci/probe.c +7 -0

index 6675a7a..6cab8be 100644
@@ -447,6 +447,13 @@ static void pci_read_bridge_mmio_pref(struct 
 pci_bus *child)
         }
  }
 
+bool pci_bridge_bases_is_read(struct pci_bus *bus)
+{
+       return (bus->resource[0]->start ||
+               bus->resource[1]->start ||
+               bus->resource[2]->start);
+}
+
  void pci_read_bridge_bases(struct pci_bus *child)
  {

         struct pci_dev *dev = child->self;

include/linux/pci.h +1 -0

index 353db8d..11c674d 100644
@@ -798,6 +798,7 @@ void pci_device_add(struct pci_dev *dev, struct 
 pci_bus *bus);
  unsigned int pci_scan_child_bus(struct pci_bus *bus);
  void pci_bus_add_device(struct pci_dev *dev);
  void pci_read_bridge_bases(struct pci_bus *child);
+bool pci_bridge_bases_is_read(struct pci_bus *bus);
  struct resource *pci_find_parent_resource(const struct pci_dev *dev,
                                           struct resource *res);

u8 pci_swizzle_interrupt_pin(const struct pci_dev *dev, u8 pin);
---- END PATCH -----

I'm not sure if this is the best place to be reading the bridge bases. I guess we should be able to do this when adding bridge devices.

Thanks,

Suravee

Quoted 64 lines · Suravee Suthikulanit

Quoted 33 lines · Lorenzo Pieralisi

When a device is scanned and added to the PCI bus, its resources should be claimed to validate the BARs configuration and to assign them a parent resource so that the resource hierarchy can be sanity checked.

This patch adds code that carries out PCI device resources claiming to the ARM64 pcibios_add_device implementation so that device resources are claimed by the core PCI layer upon PCI device initialization on ARM64 systems.

Signed-off-by Lorenzo Pieralisi <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Will Deacon <[email protected]>

Cc Liviu Dudau <[email protected]>

Cc Bjorn Helgaas <[email protected]>

---
  arch/arm64/kernel/pci.c | 10 ++++++++++
  1 file changed, 10 insertions(+)

arch/arm64/kernel/pci.c +3 -0

index 4095379..c0a88ca 100644
@@ -43,8 +43,18 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
   */
  int pcibios_add_device(struct pci_dev *dev)
  {
+   struct resource *res;
+   int i;
+
    dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
 

arch/arm64/kernel/pci.c resource_size_t pcibios_align_resource(void *data, const struct resource *res, · line 60

Quoted 10 lines

arch/arm64/kernel/pci.c +7 -0

+   for (i = 0; i < PCI_NUM_RESOURCES; i++) {
+           res = &dev->resource[i];
+           if (res->parent || !res->flags)
+                   continue;
+           pci_claim_resource(dev, i);
+   }
+
    return 0;
  }
 

Lorenzo/Bjorn,

I have tested this patch on top of Jayachandran's V2 patch series
(http://www.spinics.net/lists/linux-pci/msg40811.html) on AMD Seattle (w/ PROBE_ONLY and non-PROBE_ONLY mode), and your changes here works with additional changes below.

It seems that when booting w/ PROBE_ONLY case, we need to call pci_read_bridge_bases() at some point before claiming the resources of devices underneath the bridge. This is needed to determine the bridge bases (i.e. bridge io, mmio and mmio_pref bases), and update bridge resources accordingly.

---- BEGIN PATCH -----

arch/arm64/kernel/pci.c +0 -0

index c0a88ca..57be6aa 100644

arch/arm64/kernel/pci.c int pcibios_add_device(struct pci_dev *dev) · line 51

Quoted 4 lines

arch/arm64/kernel/pci.c +1 -0

@@ -48,6 +48,11 @@ int pcibios_add_device(struct pci_dev *dev)
 
         dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
 
+       if (pci_has_flag(PCI_PROBE_ONLY) &&

Does it really matter if PCI_PROBE_ONLY is set ?

arch/arm64/kernel/pci.c int pcibios_add_device(struct pci_dev *dev) · line 52

Quoted 1 line

arch/arm64/kernel/pci.c +1 -0

+           !pci_is_root_bus(dev->bus) &&

This check is useless, since pci_read_bridge_bases checks that already.

arch/arm64/kernel/pci.c int pcibios_add_device(struct pci_dev *dev) · line 55

Quoted 3 lines

arch/arm64/kernel/pci.c +3 -0

+           !pci_bridge_bases_is_read(dev->bus))
+               pci_read_bridge_bases(dev->bus);
+

Ok. Most of the archs do that in pcibios_fixup_bus, I would like to understand:

1) Should we do it on PCI_PROBE_ONLY only
2) Can we move this to generic code - ie pci_scan_child_bus (I guess answer is no, because there are corner cases I am not aware of)

If I add it to arm64 (in pcibios_fixup_bus) I have to add it to arm too, more arch specific code that has nothing arch specific in it so I am not really keen on that.

Comments appreciated.

Lorenzo

Quoted 45 lines

arch/arm64/kernel/pci.c +0 -0

         for (i = 0; i < PCI_NUM_RESOURCES; i++) {
                 res = &dev->resource[i];
                 if (res->parent || !res->flags)

drivers/pci/probe.c +7 -0

index 6675a7a..6cab8be 100644
@@ -447,6 +447,13 @@ static void pci_read_bridge_mmio_pref(struct
 pci_bus *child)
         }
  }
 
+bool pci_bridge_bases_is_read(struct pci_bus *bus)
+{
+       return (bus->resource[0]->start ||
+               bus->resource[1]->start ||
+               bus->resource[2]->start);
+}
+
  void pci_read_bridge_bases(struct pci_bus *child)
  {

         struct pci_dev *dev = child->self;

include/linux/pci.h +1 -0

index 353db8d..11c674d 100644
@@ -798,6 +798,7 @@ void pci_device_add(struct pci_dev *dev, struct
 pci_bus *bus);
  unsigned int pci_scan_child_bus(struct pci_bus *bus);
  void pci_bus_add_device(struct pci_dev *dev);
  void pci_read_bridge_bases(struct pci_bus *child);
+bool pci_bridge_bases_is_read(struct pci_bus *bus);
  struct resource *pci_find_parent_resource(const struct pci_dev *dev,
                                           struct resource *res);

u8 pci_swizzle_interrupt_pin(const struct pci_dev *dev, u8 pin);
---- END PATCH -----

I'm not sure if this is the best place to be reading the bridge bases. I guess we should be able to do this when adding bridge devices.

Thanks,

Suravee

Quoted 16 lines

arch/arm64/kernel/pci.c int pcibios_add_device(struct pci_dev *dev) · line 51

Quoted 5 lines

arch/arm64/kernel/pci.c +1 -0

@@ -48,6 +48,11 @@ int pcibios_add_device(struct pci_dev *dev)
 
         dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
 
+       if (pci_has_flag(PCI_PROBE_ONLY) &&

Does it really matter if PCI_PROBE_ONLY is set ?

arch/arm64/kernel/pci.c int pcibios_add_device(struct pci_dev *dev) · line 52

Quoted 1 line

arch/arm64/kernel/pci.c +1 -0

+           !pci_is_root_bus(dev->bus) &&

This check is useless, since pci_read_bridge_bases checks that already.

arch/arm64/kernel/pci.c int pcibios_add_device(struct pci_dev *dev) · line 55

Quoted 3 lines

arch/arm64/kernel/pci.c +3 -0

+           !pci_bridge_bases_is_read(dev->bus))
+               pci_read_bridge_bases(dev->bus);
+

Quoted 6 lines

Ok. Most of the archs do that in pcibios_fixup_bus, I would like to understand:

1) Should we do it on PCI_PROBE_ONLY only
2) Can we move this to generic code - ie pci_scan_child_bus (I guess answer is no, because there are corner cases I am not aware of)

In my opinion, we should call pci_read_bridge_bases() in all cases, regardless of PCI_PROBE_ONLY. pci_read_bridge_bases() doesn't *change* anything in the hardware; it only reads what's there. (It should attempt writes to learn whether I/O and prefetchable memory apertures are implemented, but those should be done as in pci_bridge_check_ranges(), where the original value is restored.)

I also think this should be done in generic code, since there's nothing architecture-specific about bridge operation.

I've been hoping to get rid of pcibios_fixup_bus() completely for years, and doing pci_read_bridge_bases() in generic code would be a big step.

No doubt there are corner cases we'll trip over, but I'm not aware of them yet.

When a device is scanned and added to the PCI bus, its resources should be claimed to validate the BARs configuration and to assign them a parent resource so that the resource hierarchy can be sanity checked.

This patch adds code that carries out PCI device resources claiming to the ARM64 pcibios_add_device implementation so that device resources are claimed by the core PCI layer upon PCI device initialization on ARM64 systems.

Signed-off-by Lorenzo Pieralisi <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Will Deacon <[email protected]>

Cc Liviu Dudau <[email protected]>

Cc Bjorn Helgaas <[email protected]>

---
 arch/arm64/kernel/pci.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

arch/arm64/kernel/pci.c +10 -0

index 4095379..c0a88ca 100644
@@ -43,8 +43,18 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
  */
 int pcibios_add_device(struct pci_dev *dev)
 {
+	struct resource *res;
+	int i;
+
 	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
 
+	for (i = 0; i < PCI_NUM_RESOURCES; i++) {
+		res = &dev->resource[i];
+		if (res->parent || !res->flags)
+			continue;
+		pci_claim_resource(dev, i);
+	}
+
 	return 0;
 }
 

Signature · 1 line

2.2.1

Hi Lorenzo,

Quoted 10 lines · Lorenzo Pieralisi

When a device is scanned and added to the PCI bus, its resources should be claimed to validate the BARs configuration and to assign them a parent resource so that the resource hierarchy can be sanity checked.

This patch adds code that carries out PCI device resources claiming to the ARM64 pcibios_add_device implementation so that device resources are claimed by the core PCI layer upon PCI device initialization on ARM64 systems.

Quoted 5 lines

Signed-off-by Lorenzo Pieralisi <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Will Deacon <[email protected]>

Cc Liviu Dudau <[email protected]>

Cc Bjorn Helgaas <[email protected]>

Seems like Suravee proposed a tweak? Can you post a v2 with that incorporated if necessary?

Bjorn

Quoted 30 lines

---
 arch/arm64/kernel/pci.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

arch/arm64/kernel/pci.c +10 -0

index 4095379..c0a88ca 100644
@@ -43,8 +43,18 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
  */
 int pcibios_add_device(struct pci_dev *dev)
 {
+	struct resource *res;
+	int i;
+
 	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
 
+	for (i = 0; i < PCI_NUM_RESOURCES; i++) {
+		res = &dev->resource[i];
+		if (res->parent || !res->flags)
+			continue;
+		pci_claim_resource(dev, i);
+	}
+
 	return 0;
 }
 

Signature · 1 line

2.2.1