ARM/ARM64: PCI: PCI_PROBE_ONLY clean-up
Subsystems (3)
v3 cover ARM/ARM64: PCI: PCI_PROBE_ONLY clean-up
Current arm/arm64 code prevents enabling resources in the respective pcibios_enable_device() callbacks if the PCI_PROBE_ONLY flag is set, in that on those platforms the resources tree was not validated properly since PCI resources are not currently claimed on arm/arm64 PCI_PROBE_ONLY systems.
This is a temporary kludge, in that PCI_PROBE_ONLY flag is used to describe systems with fixed resources, that can nonetheless be enabled through the standard pci_enable_resources() call present in PCI core code, (ie via the generic pcibios_enable_device() call).
To remove the PCI_PROBE_ONLY flag in arm/arm64 pcibios_enable_device() callbacks, the PCI host controllers that can be used with PCI_PROBE_ONLY configurations must make sure that devices resources are validated and inserted in the kernel resource tree even on PCI_PROBE_ONLY systems so that the generic pcibios_enable_device() generic code, while enabling resources (pci_enable_resources()), does not find dangling resources pointers (ie missing parent pointers) that are omens of an incomplete resource tree, causing failures in resources enablement.
PCI core code provides interfaces to assign/reassign/reallocate PCI bus resources but it is currently lacking an interface to claim resources for a specific bus. Arches implement resources claiming through ad-hoc code built on top of pci_claim_resource() API, but that code cannot be leveraged on architectures like arm/arm64 that rely on the generic PCI infrastructure to carry out resources claiming/assignment.
Therefore, to clean up the arm/arm64 resources enablement on PCI_PROBE_ONLY systems this patchset implements four patches:
PATCH 1: Create PCI core code infrastructure to claim bus resources PATCH 2: Leverage the infrastructure in PATCH 1 to claim resources in
the PCI generic host controller on PCI_PROBE_ONLY systems
PATCH 3: Remove the PCI_PROBE_ONLY kludge from the arm64 PCI back-end to
complete the clean-up and leverage the PCI generic pcibios_enable_device() implementation
PATCH 4: Remove the PCI_PROBE_ONLY kludge from the arm PCI back-end to
complete the clean-up and leverage the PCI generic pcibios_enable_device() implementation
Tested on arm/arm64 systems with kvmtool and PCI host generic.
v2 -> v3
- Split patch 3 in two patches so that arm and arm64 are handled in different patches
- Improved commits logs, clarified resource parent assignment
- Rebased against v4.7-rc2
v2: https://patchwork.ozlabs.org/patch/590556/
v1 -> v2
- Rewrote patch 1 to recursively claim resources for the whole PCI bus hierarchy
- Updated commits logs/tags
- Rebased against v4.5-rc6
v1: https://patchwork.ozlabs.org/patch/545669/
Lorenzo Pieralisi (4):
PCI: add generic code to claim bus resources
PCI: host-generic: claim bus resources on PCI_PROBE_ONLY set-ups ARM64/PCI: remove arch specific pcibios_enable_device() ARM/PCI: remove arch specific pcibios_enable_device()
arch/arm/kernel/bios32.c | 12 ------- arch/arm64/kernel/pci.c | 13 -------- drivers/pci/host/pci-host-common.c | 27 +++++++++++++-- drivers/pci/setup-bus.c | 68 ++++++++++++++++++++++++++++++++++++++ include/linux/pci.h | 1 + 5 files changed, 94 insertions(+), 27 deletions(-)
Signature · 1 line
2.6.4
PATCH v3 1/4 PCI: add generic code to claim bus resources
PCI core code contains a set of functions, eg:
pci_assign_unassigned_bus_resources()
that allow to assign the PCI resources for a given bus after enumeration.
On systems where the PCI BARs are immutable (ie they must not and can not be assigned), PCI bus resources should still be inserted in the PCI resources tree (even if they are not (re)-assigned), but there is no generic PCI kernel function for that purpose. Currently the PCI bus resources insertion in the resources tree is implemented in an arch specific fashion through arch specific callbacks that rely on the PCI resources claiming API (for bridges and devices) which resulted in arches implementations that contain duplicated code to claim resources for a given PCI bus hierarchy.
Based on the x86/ia64 resources claiming arch implementations, implement a set of functions in core PCI code that provides a PCI core interface for resources claiming for a given PCI bus hierarchy, paving the way for further resource claiming consolidation across architectures.
--- drivers/pci/setup-bus.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/pci.h | 1 + 2 files changed, 69 insertions(+)
drivers/pci/setup-bus.c +68 -0
@@ -1423,6 +1423,74 @@ void pci_bus_assign_resources(const struct pci_bus *bus) }EXPORT_SYMBOL (pci_bus_assign_resources); +static void pci_claim_device_resources (struct pci_dev *dev) +{ +int i; + +for (i =0 ; i <PCI_BRIDGE_RESOURCES ; i++) { +struct resource *r = &dev->resource[i]; + +if (!r->flags || r->parent) +continue ; + +pci_claim_resource (dev, i); + } +} + +static void pci_claim_bridge_resources (struct pci_dev *dev) +{ +int i; + +for (i =PCI_BRIDGE_RESOURCES ; i <PCI_NUM_RESOURCES ; i++) { +struct resource *r = &dev->resource[i]; + +if (!r->flags || r->parent) +continue ; + +pci_claim_bridge_resource (dev, i); + } +} + +static void pci_bus_allocate_dev_resources (struct pci_bus *b) +{ +struct pci_dev *dev; +struct pci_bus *child; + +list_for_each_entry (dev, &b->devices, bus_list) { +pci_claim_device_resources (dev); + + child = dev->subordinate; +if (child) +pci_bus_allocate_dev_resources (child); + } +} + +static void pci_bus_allocate_resources (struct pci_bus *b) +{ +struct pci_bus *child; + +/* +* Carry out a depth-first search on the PCI bus +* tree to allocate bridge apertures. Read the +* programmed bridge bases and recursively claim +* the respective bridge resources. +*/ +if (b->self) { +pci_read_bridge_bases (b); +pci_claim_bridge_resources (b->self); + } + +list_for_each_entry (child, &b->children, node) +pci_bus_allocate_resources (child); +} + +void pci_bus_claim_resources (struct pci_bus *b) +{ +pci_bus_allocate_resources (b); +pci_bus_allocate_dev_resources (b); +} +EXPORT_SYMBOL (pci_bus_claim_resources); +static void __pci_bridge_assign_resources (const struct pci_dev *bridge,struct list_head *add_head,struct list_head *fail_head)
include/linux/pci.h +1 -0
@@ -1114,6 +1114,7 @@ int pci_set_vpd_size(struct pci_dev *dev, size_t len);/* Helper functions for low-level code (drivers/pci/setup-[bus,res].c) */ resource_size_t pcibios_retrieve_fw_addr (struct pci_dev *dev,int idx);void pci_bus_assign_resources (const struct pci_bus *bus); +void pci_bus_claim_resources (struct pci_bus *bus);void pci_bus_size_bridges (struct pci_bus *bus);int pci_claim_resource (struct pci_dev *,int );int pci_claim_bridge_resource (struct pci_dev *bridge,int i);
Signature · 1 line
2.6.4
PATCH v3 2/4 PCI: host-generic: claim bus resources on PCI_PROBE_ONLY set-ups
In reply to Lorenzo PieralisiThe PCI host generic driver does not reassign bus resources on systems that require the BARs set-up to be immutable (ie PCI_PROBE_ONLY) since that would trigger system failures. Nonetheless, PCI bus resources allocated to PCI bridge and devices must be claimed in order to be validated and inserted in the kernel resource tree, but the current driver omits the resources claiming and relies on arch specific kludges to prevent probing failure (ie preventing resources enablement on PCI_PROBE_ONLY systems).
Add code to the PCI host generic driver that correctly claims bus resources upon probe on systems that are required to prevent reassignment after bus enumeration, so that the allocated resources can be enabled successfully upon PCI device drivers probing, without resorting to arch back-ends workarounds.
--- drivers/pci/host/pci-host-common.c | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-)
drivers/pci/host/pci-host-common.c +25 -2
@@ -154,8 +154,31 @@ int pci_host_common_probe(struct platform_device *pdev, }pci_fixup_irqs (pci_common_swizzle, of_irq_parse_and_map_pci); - -if (!pci_has_flag (PCI_PROBE_ONLY )) { +/* +* For PCI bridges and PCI devices to be fully set-up, their +* resources must be initialized and inserted in the kernel +* resource tree, which implicitly means that the resources +* are correctly inserted in the kernel resource tree and are +* assigned a parent pointer to create the resource hierarchy. +* +* On PCI_PROBE_ONLY systems, the pci_bus_claim_resource() +* API claims the resources as set-up by firmware, which takes +* care of inserting them in the resource tree and setting their +* parent pointers accordingly. +* +* On !PCI_PROBE_ONLY systems, the firmware set-up is discarded, +* and resource assignment and insertion in the resource tree is +* carried out through the PCI resources API: +* +* (ie pci_bus_size_bridges() and pci_bus_assign_resources()) +* +* that sizes the bridges and assigns and inserts resources in the +* kernel resource tree (which also implies setting up their parent +* pointers), completing the resources set-up. +*/ +if (pci_has_flag (PCI_PROBE_ONLY )) { +pci_bus_claim_resources (bus); + }else {pci_bus_size_bridges (bus);pci_bus_assign_resources (bus);
Signature · 1 line
2.6.4
PATCH v3 3/4 ARM64/PCI: remove arch specific pcibios_enable_device()
In reply to Lorenzo PieralisiThe arm64 pcibios_enable_device() implementation exists solely to prevent enabling PCI resources on PCI_PROBE_ONLY systems, since on those systems the PCI resources are currently not claimed (ie inserted in the PCI resource tree - which means their parent pointer is not correctly set-up) therefore they can not be enabled since this would trigger PCI set-ups failures.
By introducing resources claiming in the PCI host controllers set-ups that have PCI_PROBE_ONLY as a probe option, there is no need for arch specific pcibios_enable_device() implementations anymore in that the kernel can rely on the generic pcibios_enable_device() implementation without resorting to arch specific code to work around the missing resources claiming enumeration step.
On !PCI_PROBE_ONLY PCI bus set-ups, resources are always assigned in the PCI host controllers drivers; since the PCI resource assignment API takes care of inserting the assigned resources in the resource tree, the resources parent pointer is correctly set-up, which means that this patch leaves behaviour unchanged for all arm64 PCI set-ups that do not set the PCI_PROBE_ONLY flag.
Remove the pcibios_enable_device() function from the arm64 arch back-end so that the kernel now uses its generic implementation.
--- arch/arm64/kernel/pci.c | 13 ------------- 1 file changed, 13 deletions(-)
arch/arm64/kernel/pci.c +0 -13
@@ -36,19 +36,6 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,return res->start; } -/** -* pcibios_enable_device - Enable I/O and memory. -* @dev: PCI device to be enabled -* @mask: bitmask of BARs to enable -*/ -int pcibios_enable_device (struct pci_dev *dev,int mask) -{ -if (pci_has_flag (PCI_PROBE_ONLY )) -return 0 ; - -return pci_enable_resources (dev, mask); -} -/* * Try to assign the IRQ number from DT when adding a new device */
Signature · 1 line
2.6.4
PATCH v3 4/4 ARM/PCI: remove arch specific pcibios_enable_device()
In reply to Lorenzo PieralisiThe arm pcibios_enable_device() implementation exists solely to prevent enabling PCI resources on PCI_PROBE_ONLY systems, since on those systems the PCI resources are currently not claimed (ie inserted in the PCI resource tree - which means their parent pointer is not correctly set-up) therefore they can not be enabled since this would trigger PCI set-ups failures.
After removing the pci=firmware command line option in:
commit 903589ca7165 ("ARM: 8554/1: kernel: pci: remove pci=firmware command line parameter handling")
(that was used to set the PCI_PROBE_ONLY flag through the command line) and by introducing resources claiming in the PCI host controllers set-ups that have PCI_PROBE_ONLY as a probe option, there is no need for arch specific pcibios_enable_device() implementations anymore in that the kernel can rely on the generic pcibios_enable_device() implementation without resorting to arch specific code to work around the missing resources claiming enumeration step.
On !PCI_PROBE_ONLY PCI bus set-ups, resources are always assigned either in pcibios initialization code or PCI host controllers drivers; since the PCI resource assignment API takes care of inserting the assigned resources in the resource tree, the resources parent pointers are correctly set-up, which means that this patch leaves behaviour unchanged for all arm PCI set-ups that do not set the PCI_PROBE_ONLY flag.
Remove the pcibios_enable_device() function from the arm arch back-end so that the kernel now uses its generic implementation.
--- arch/arm/kernel/bios32.c | 12 ------------ 1 file changed, 12 deletions(-)
arch/arm/kernel/bios32.c +0 -12
@@ -590,18 +590,6 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,return start; } -/** -* pcibios_enable_device - Enable I/O and memory. -* @dev: PCI device to be enabled -*/ -int pcibios_enable_device (struct pci_dev *dev,int mask) -{ -if (pci_has_flag (PCI_PROBE_ONLY )) -return 0 ; - -return pci_enable_resources (dev, mask); -} -int pci_mmap_page_range (struct pci_dev *dev,struct vm_area_struct *vma,enum pci_mmap_state mmap_state,int write_combine) {
Signature · 1 line
2.6.4
PATCH v3 4/4 ARM/PCI: remove arch specific pcibios_enable_device()
Quoted 50 lines · Lorenzo Pieralisi
The arm
pcibios_enable_device()implementation exists solely to prevent enabling PCI resources onPCI_PROBE_ONLYsystems, since on those systems the PCI resources are currently not claimed (ie inserted in the PCI resource tree - which means their parent pointer is not correctly set-up) therefore they can not be enabled since this would trigger PCI set-ups failures.After removing the pci=firmware command line option in:
commit
903589ca7165("ARM: 8554/1: kernel: pci: remove pci=firmware command line parameter handling")(that was used to set the
PCI_PROBE_ONLYflag through the command line) and by introducing resources claiming in the PCI host controllers set-ups that havePCI_PROBE_ONLYas a probe option, there is no need for arch specificpcibios_enable_device()implementations anymore in that the kernel can rely on the genericpcibios_enable_device()implementation without resorting to arch specific code to work around the missing resources claiming enumeration step.On !PCI_PROBE_ONLY PCI bus set-ups, resources are always assigned either in pcibios initialization code or PCI host controllers drivers; since the PCI resource assignment API takes care of inserting the assigned resources in the resource tree, the resources parent pointers are correctly set-up, which means that this patch leaves behaviour unchanged for all arm PCI set-ups that do not set the
PCI_PROBE_ONLYflag.Remove the
pcibios_enable_device()function from the arm arch back-end so that the kernel now uses its generic implementation.Signed-off-by Lorenzo Pieralisi <[email protected]>Acked-by Will Deacon <[email protected]>Cc Arnd Bergmann <[email protected]>Cc Will Deacon <[email protected]>Cc Bjorn Helgaas <[email protected]>Cc Russell King <[email protected]>--- arch/arm/kernel/bios32.c | 12 ------------ 1 file changed, 12 deletions(-)arch/arm/kernel/bios32.c +0
-1@@ -590,18 +590,6 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,return start; } -/**
arch/arm/kernel/bios32.c resource_size_t pcibios_align_resource(void *data, const struct resource *res, · line 603
Quoted 10 lines
arch/arm/kernel/bios32.c +0
-10-* pcibios_enable_device - Enable I/O and memory. -* @dev: PCI device to be enabled -*/ -int pcibios_enable_device (struct pci_dev *dev,int mask) -{ -if (pci_has_flag (PCI_PROBE_ONLY )) -return 0 ; - -return pci_enable_resources (dev, mask); -}
This looks great.
What about the PCI_PROBE_ONLY test in pci_common_init_dev()? Don't we need to either remove that test (if it's impossible to get there with PCI_PROBE_ONLY set), or add a pci_bus_claim_resources() call as we did in pci_host_common_probe()?
I think it's unlikely that we'd get to pci_common_init_dev() with PCI_PROBE_ONLY set:
- the only way to set PCI_PROBE_ONLY on ARM is to call
of_pci_check_probe_only(),
- the only ARM caller of of_pci_check_probe_only() is pci_host_common_probe(),
- pci_host_common_probe() doesn't call pci_common_init_dev().
But I guess it's possible to imagine a platform with both a generic PCI bridge and a MVEBU, R-Car, or Tegra bridge. Then pci_host_common_probe() could set PCI_PROBE_ONLY, and we'd claim resources under the generic bridge via the previous patch, but still not claim those under the MVEBU bridge. Then enabling the MVEBU devices would fail.
I know this is a ridiculous scenario, but the code looks inconsistent as it is.
Quoted 11 lines
arch/arm/kernel/bios32.c +0
-0int pci_mmap_page_range (struct pci_dev *dev,struct vm_area_struct *vma,enum pci_mmap_state mmap_state,int write_combine) {Signature · 7 lines
2.6.4 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel at lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH v3 4/4 ARM/PCI: remove arch specific pcibios_enable_device()
Quoted 64 lines · Bjorn Helgaas
Quoted 50 lines · Lorenzo Pieralisi
The arm
pcibios_enable_device()implementation exists solely to prevent enabling PCI resources onPCI_PROBE_ONLYsystems, since on those systems the PCI resources are currently not claimed (ie inserted in the PCI resource tree - which means their parent pointer is not correctly set-up) therefore they can not be enabled since this would trigger PCI set-ups failures.After removing the pci=firmware command line option in:
commit
903589ca7165("ARM: 8554/1: kernel: pci: remove pci=firmware command line parameter handling")(that was used to set the
PCI_PROBE_ONLYflag through the command line) and by introducing resources claiming in the PCI host controllers set-ups that havePCI_PROBE_ONLYas a probe option, there is no need for arch specificpcibios_enable_device()implementations anymore in that the kernel can rely on the genericpcibios_enable_device()implementation without resorting to arch specific code to work around the missing resources claiming enumeration step.On !PCI_PROBE_ONLY PCI bus set-ups, resources are always assigned either in pcibios initialization code or PCI host controllers drivers; since the PCI resource assignment API takes care of inserting the assigned resources in the resource tree, the resources parent pointers are correctly set-up, which means that this patch leaves behaviour unchanged for all arm PCI set-ups that do not set the
PCI_PROBE_ONLYflag.Remove the
pcibios_enable_device()function from the arm arch back-end so that the kernel now uses its generic implementation.Signed-off-by Lorenzo Pieralisi <[email protected]>Acked-by Will Deacon <[email protected]>Cc Arnd Bergmann <[email protected]>Cc Will Deacon <[email protected]>Cc Bjorn Helgaas <[email protected]>Cc Russell King <[email protected]>--- arch/arm/kernel/bios32.c | 12 ------------ 1 file changed, 12 deletions(-)arch/arm/kernel/bios32.c +0
-1@@ -590,18 +590,6 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,return start; } -/** arch/arm/kernel/bios32.c resource_size_t pcibios_align_resource(void *data, const struct resource *res, · line 603
Quoted 10 lines
arch/arm/kernel/bios32.c +0
-10-* pcibios_enable_device - Enable I/O and memory. -* @dev: PCI device to be enabled -*/ -int pcibios_enable_device (struct pci_dev *dev,int mask) -{ -if (pci_has_flag (PCI_PROBE_ONLY )) -return 0 ; - -return pci_enable_resources (dev, mask); -}This looks great.
Quoted 4 lines
What about the
PCI_PROBE_ONLYtest inpci_common_init_dev()? Don't we need to either remove that test (if it's impossible to get there withPCI_PROBE_ONLYset), or add apci_bus_claim_resources()call as we did inpci_host_common_probe()?
Yes, you are right, I went for the second option given what you say below and sent you and Russell an additional patch that should be added to this series.
Quoted 18 lines
I think it's unlikely that we'd get to
pci_common_init_dev()withPCI_PROBE_ONLYset:- the only way to set
PCI_PROBE_ONLYon ARM is to callof_pci_check_probe_only(),- the only ARM caller of
of_pci_check_probe_only()ispci_host_common_probe(),-
pci_host_common_probe()doesn't callpci_common_init_dev().But I guess it's possible to imagine a platform with both a generic PCI bridge and a MVEBU, R-Car, or Tegra bridge. Then
pci_host_common_probe()could setPCI_PROBE_ONLY, and we'd claim resources under the generic bridge via the previous patch, but still not claim those under the MVEBU bridge. Then enabling the MVEBU devices would fail.
Quoted 2 lines
I know this is a ridiculous scenario, but the code looks inconsistent as it is.
Well, I do not think that's an *existing* scenario, but this does not mean that the code is correct, so you are right and it has to be fixed, please let me know if the patch I sent is fine.
I really have to remove PCI_PROBE_ONLY entirely from ARM/ARM64 kernels.
Thanks !
Lorenzo
Quoted 13 lines
Quoted 12 lines
arch/arm/kernel/bios32.c +0
-0int pci_mmap_page_range (struct pci_dev *dev,struct vm_area_struct *vma,enum pci_mmap_state mmap_state,int write_combine) {Signature · 7 lines
2.6.4 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel at lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH v3 4/4 ARM/PCI: remove arch specific pcibios_enable_device()
In reply to Lorenzo Pieralisi[+cc Guan]
Quoted 11 lines · Lorenzo Pieralisi
The arm
pcibios_enable_device()implementation exists solely to prevent enabling PCI resources onPCI_PROBE_ONLYsystems, since on those systems the PCI resources are currently not claimed (ie inserted in the PCI resource tree - which means their parent pointer is not correctly set-up) therefore they can not be enabled since this would trigger PCI set-ups failures.After removing the pci=firmware command line option in:
commit
903589ca7165("ARM: 8554/1: kernel: pci: remove pci=firmware command line parameter handling")
903589ca7165 removed "pci=firmware" from ARM and from Documentation/kernel-parameters.txt (where it was documented as ARM-specific).
I think unicore32 copied that code from arm, so unicore32 is one of the few remaining arches that can set PCI_PROBE_ONLY. I suspect this was just copied from arm and may not be necessary on unicore32.
Guan, can you confirm that PCI_PROBE_ONLY is necessary on unicore32, or that it can be removed?
Quoted 60 lines
(that was used to set the
PCI_PROBE_ONLYflag through the command line) and by introducing resources claiming in the PCI host controllers set-ups that havePCI_PROBE_ONLYas a probe option, there is no need for arch specificpcibios_enable_device()implementations anymore in that the kernel can rely on the genericpcibios_enable_device()implementation without resorting to arch specific code to work around the missing resources claiming enumeration step.On !PCI_PROBE_ONLY PCI bus set-ups, resources are always assigned either in pcibios initialization code or PCI host controllers drivers; since the PCI resource assignment API takes care of inserting the assigned resources in the resource tree, the resources parent pointers are correctly set-up, which means that this patch leaves behaviour unchanged for all arm PCI set-ups that do not set the
PCI_PROBE_ONLYflag.Remove the
pcibios_enable_device()function from the arm arch back-end so that the kernel now uses its generic implementation.Signed-off-by Lorenzo Pieralisi <[email protected]>Acked-by Will Deacon <[email protected]>Cc Arnd Bergmann <[email protected]>Cc Will Deacon <[email protected]>Cc Bjorn Helgaas <[email protected]>Cc Russell King <[email protected]>--- arch/arm/kernel/bios32.c | 12 ------------ 1 file changed, 12 deletions(-)arch/arm/kernel/bios32.c +0
-12@@ -590,18 +590,6 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,return start; } -/** -* pcibios_enable_device - Enable I/O and memory. -* @dev: PCI device to be enabled -*/ -int pcibios_enable_device (struct pci_dev *dev,int mask) -{ -if (pci_has_flag (PCI_PROBE_ONLY )) -return 0 ; - -return pci_enable_resources (dev, mask); -} -int pci_mmap_page_range (struct pci_dev *dev,struct vm_area_struct *vma,enum pci_mmap_state mmap_state,int write_combine) {Signature · 7 lines
2.6.4 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel at lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH v3 4/4 ARM/PCI: remove arch specific pcibios_enable_device()
Quoted 19 lines
[+cc Guan]
Quoted 11 lines · Lorenzo Pieralisi
The arm
pcibios_enable_device()implementation exists solely to prevent enabling PCI resources onPCI_PROBE_ONLYsystems, since on those systems the PCI resources are currently not claimed (ie inserted in the PCI resource tree - which means their parent pointer is not correctly set-up) therefore they can not be enabled since this would trigger PCI set-ups failures.After removing the pci=firmware command line option in:
commit
903589ca7165("ARM: 8554/1: kernel: pci: remove pci=firmware command line parameter handling")
903589ca7165removed "pci=firmware" from ARM and fromDocumentation/kernel-parameters.txt(where it was documented as ARM-specific).
Quoted 7 lines
I think unicore32 copied that code from arm, so unicore32 is one of the few remaining arches that can set
PCI_PROBE_ONLY. I suspect this was just copied from arm and may not be necessary on unicore32.Guan, can you confirm that
PCI_PROBE_ONLYis necessary on unicore32, or that it can be removed?
Yes, this code was copied from arm, and it's not necessary. It can be removed safely.
Guan Xuetao
PATCH v3 4/4 ARM/PCI: remove arch specific pcibios_enable_device()
Quoted 23 lines · Xuetao Guan
Quoted 23 lines
[+cc Guan]
Quoted 11 lines · Lorenzo Pieralisi
The arm
pcibios_enable_device()implementation exists solely to prevent enabling PCI resources onPCI_PROBE_ONLYsystems, since on those systems the PCI resources are currently not claimed (ie inserted in the PCI resource tree - which means their parent pointer is not correctly set-up) therefore they can not be enabled since this would trigger PCI set-ups failures.After removing the pci=firmware command line option in:
commit
903589ca7165("ARM: 8554/1: kernel: pci: remove pci=firmware command line parameter handling")
903589ca7165removed "pci=firmware" from ARM and fromDocumentation/kernel-parameters.txt(where it was documented as ARM-specific).I think unicore32 copied that code from arm, so unicore32 is one of the few remaining arches that can set
PCI_PROBE_ONLY. I suspect this was just copied from arm and may not be necessary on unicore32.
Quoted 6 lines
Quoted 3 lines
Guan, can you confirm that
PCI_PROBE_ONLYis necessary on unicore32, or that it can be removed?Yes, this code was copied from arm, and it's not necessary. It can be removed safely.
Great, thank you! I'll add the patch below to my pci/resource branch for v4.8. Let me know if you see any issues with it.
commit b9b8a53e24649e48c0a8bceb8d9d8fe1cfee018c
Author: Bjorn Helgaas <[email protected]> Date: Thu Jun 23 11:33:24 2016 -0500
unicore32/PCI: Remove pci=firmware command line parameter handling
Remove support for the "pci=firmware" command line parameter, which was way to keep the kernel from changing any PCI BAR assignments. This was copied from ARM, but is not actually needed on unicore32.
The corresponding ARM support was removed by 903589ca7165 ("ARM: 8554/1:
kernel: pci: remove pci=firmware command line parameter handling").
arch/unicore32/kernel/pci.c +2 -7
@@ -265,10 +265,8 @@ static int __init pci_common_init(void)pci_fixup_irqs (pci_common_swizzle, pci_puv3_map_irq); -if (!pci_has_flag (PCI_PROBE_ONLY )) { -pci_bus_size_bridges (puv3_bus); -pci_bus_assign_resources (puv3_bus); - } +pci_bus_size_bridges (puv3_bus); +pci_bus_assign_resources (puv3_bus);pci_bus_add_devices (puv3_bus);return 0 ; }
@@ -279,9 +277,6 @@ char * __init pcibios_setup(char *str)if (!strcmp (str,"debug" )) { debug_pci =1 ;return NULL ; - }else if (!strcmp (str,"firmware" )) { -pci_add_flags (PCI_PROBE_ONLY ); -return NULL ; }return str; }
PATCH v3 4/4 ARM/PCI: remove arch specific pcibios_enable_device()
Quoted 47 lines · Xuetao Guan
Quoted 23 lines
Quoted 23 lines
[+cc Guan]
Quoted 11 lines · Lorenzo Pieralisi
The arm
pcibios_enable_device()implementation exists solely to prevent enabling PCI resources onPCI_PROBE_ONLYsystems, since on those systems the PCI resources are currently not claimed (ie inserted in the PCI resource tree - which means their parent pointer is not correctly set-up) therefore they can not be enabled since this would trigger PCI set-ups failures.After removing the pci=firmware command line option in:
commit
903589ca7165("ARM: 8554/1: kernel: pci: remove pci=firmware command line parameter handling")
903589ca7165removed "pci=firmware" from ARM and fromDocumentation/kernel-parameters.txt(where it was documented as ARM-specific).I think unicore32 copied that code from arm, so unicore32 is one of the few remaining arches that can set
PCI_PROBE_ONLY. I suspect this was just copied from arm and may not be necessary on unicore32.Quoted 6 lines
Quoted 3 lines
Guan, can you confirm that
PCI_PROBE_ONLYis necessary on unicore32, or that it can be removed?Yes, this code was copied from arm, and it's not necessary. It can be removed safely.
Great, thank you! I'll add the patch below to my pci/resource branch for v4.8. Let me know if you see any issues with it.
commit
b9b8a53e24649e48c0a8bceb8d9d8fe1cfee018cAuthor: Bjorn Helgaas <[email protected]> Date: Thu Jun 23 11:33:24 2016 -0500unicore32/PCI: Remove pci=firmware command line parameter handling
Remove support for the "pci=firmware" command line parameter, which
was
way to keep the kernel from changing any PCI BAR assignments. This
was
copied from ARM, but is not actually needed on unicore32.
Quoted 5 lines
The corresponding ARM support was removed by
903589ca7165("ARM:
8554/1:
kernel: pci: remove pci=firmware command line parameter handling").Signed-off-by Bjorn Helgaas <[email protected]>
Sorry for late.
Please add my ack,
Quoted 29 lines
arch/unicore32/kernel/pci.c +2
-7@@ -265,10 +265,8 @@ static int __init pci_common_init(void)pci_fixup_irqs (pci_common_swizzle, pci_puv3_map_irq); -if (!pci_has_flag (PCI_PROBE_ONLY )) { -pci_bus_size_bridges (puv3_bus); -pci_bus_assign_resources (puv3_bus); - } +pci_bus_size_bridges (puv3_bus); +pci_bus_assign_resources (puv3_bus);pci_bus_add_devices (puv3_bus);return 0 ; }@@ -279,9 +277,6 @@ char * __init pcibios_setup(char *str)if (!strcmp (str,"debug" )) { debug_pci =1 ;return NULL ; - }else if (!strcmp (str,"firmware" )) { -pci_add_flags (PCI_PROBE_ONLY ); -return NULL ; }return str; }
Bjorn Helgaas
Appliedv3 cover ARM/ARM64: PCI: PCI_PROBE_ONLY clean-up
In reply to Lorenzo PieralisiQuoted 68 lines · Lorenzo Pieralisi
Current arm/arm64 code prevents enabling resources in the respective
pcibios_enable_device()callbacks if thePCI_PROBE_ONLYflag is set, in that on those platforms the resources tree was not validated properly since PCI resources are not currently claimed on arm/arm64PCI_PROBE_ONLYsystems.This is a temporary kludge, in that
PCI_PROBE_ONLYflag is used to describe systems with fixed resources, that can nonetheless be enabled through the standardpci_enable_resources()call present in PCI core code, (ie via the genericpcibios_enable_device()call).To remove the
PCI_PROBE_ONLYflag in arm/arm64pcibios_enable_device()callbacks, the PCI host controllers that can be used withPCI_PROBE_ONLYconfigurations must make sure that devices resources are validated and inserted in the kernel resource tree even onPCI_PROBE_ONLYsystems so that the genericpcibios_enable_device()generic code, while enabling resources (pci_enable_resources()), does not find dangling resources pointers (ie missing parent pointers) that are omens of an incomplete resource tree, causing failures in resources enablement.PCI core code provides interfaces to assign/reassign/reallocate PCI bus resources but it is currently lacking an interface to claim resources for a specific bus. Arches implement resources claiming through ad-hoc code built on top of
pci_claim_resource()API, but that code cannot be leveraged on architectures like arm/arm64 that rely on the generic PCI infrastructure to carry out resources claiming/assignment.Therefore, to clean up the arm/arm64 resources enablement on
PCI_PROBE_ONLYsystems this patchset implements four patches:PATCH 1: Create PCI core code infrastructure to claim bus resources PATCH 2: Leverage the infrastructure in PATCH 1 to claim resources in
the PCI generic host controller onPCI_PROBE_ONLYsystemsPATCH 3: Remove the
PCI_PROBE_ONLYkludge from the arm64 PCI back-end to
complete the clean-up and leverage the PCI genericpcibios_enable_device()implementationPATCH 4: Remove the
PCI_PROBE_ONLYkludge from the arm PCI back-end to
complete the clean-up and leverage the PCI genericpcibios_enable_device()implementationTested on arm/arm64 systems with kvmtool and PCI host generic.
v2 -> v3
- Split patch 3 in two patches so that arm and arm64 are handled in different patches
- Improved commits logs, clarified resource parent assignment
- Rebased against v4.7-rc2v2: https://patchwork.ozlabs.org/patch/590556/
v1 -> v2
- Rewrote patch 1 to recursively claim resources for the whole PCI bus hierarchy
- Updated commits logs/tags
- Rebased against v4.5-rc6v1: https://patchwork.ozlabs.org/patch/545669/
Lorenzo Pieralisi (4):
PCI: add generic code to claim bus resources
PCI: host-generic: claim bus resources onPCI_PROBE_ONLYset-ups ARM64/PCI: remove arch specificpcibios_enable_device()ARM/PCI: remove arch specificpcibios_enable_device()
Quoted 6 lines
arch/arm/kernel/bios32.c | 12 ------- arch/arm64/kernel/pci.c | 13 -------- drivers/pci/host/pci-host-common.c | 27 +++++++++++++-- drivers/pci/setup-bus.c | 68 ++++++++++++++++++++++++++++++++++++++ include/linux/pci.h | 1 + 5 files changed, 94 insertions(+), 27 deletions(-)
I applied these to pci/resource for v4.8, thanks, Lorenzo!
It's awesome to get rid of that PCI_PROBE_ONLY hack.
We might still tweak this a little based on my question about pci_common_init_dev(), so don't build things on top of this yet (good advice in general for my branches: I often rebase and remerge them, so it's best to base things on my "master" branch unless you actually depend on something that's not in there.)