Why check for PCI_PROBE_ONLY in pci_common_init_dev()

Why check for PCI_PROBE_ONLY in pci_common_init_dev()

[Benjamin Herrenschmidt]

Hi !

So while trying to sort out & cleanup the business with PCI resource
allocation (and do the ground work to be able to revive 
https://lore.kernel.org/linux-pci/20150916085850.GA17510@red-moon/) I
stumbled upon this one:

arch/arm/kernel/bios32.c:pci_common_init_dev() checks for
PCI_PROBE_ONLY to decide whether to claim existing resources or
reallocate.

However, I can't see any code path leading to that function that would
have set that flag.

IE. Afaik, PCI_PROBE_ONLY is only set in a limited number of places
(short of a grep failure on my part):

arch/alpha/kernel/sys_marvel.c:	pci_set_flags(PCI_PROBE_ONLY);
arch/alpha/kernel/sys_titan.c:	pci_set_flags(PCI_PROBE_ONLY);
arch/mips/pci/pci-bcm1480.c:	pci_set_flags(PCI_PROBE_ONLY);
arch/mips/pci/pci-sb1250.c:	pci_set_flags(PCI_PROBE_ONLY);
arch/mips/pci/pci-virtio-guest.c:	pci_set_flags(PCI_PROBE_ONLY);
arch/mips/pci/pci-xlp.c:	pci_set_flags(PCI_PROBE_ONLY);
arch/mips/pci/pci-xlr.c:	pci_set_flags(PCI_PROBE_ONLY);
arch/mips/pci/pci-xtalk-bridge.c:	pci_set_flags(PCI_PROBE_ONLY);
arch/powerpc/platforms/maple/pci.c:	pci_add_flags(PCI_PROBE_ONLY);
arch/powerpc/platforms/pseries/setup.c:	pci_add_flags(PCI_PROBE_ONLY);
drivers/pci/of.c:		pci_add_flags(PCI_PROBE_ONLY);

The only one being of interest to arm32 here being the last one in
of_pci_check_probe_only().

Now that function is only called in two places:

arch/powerpc/platforms/pseries/setup.c: of_pci_check_probe_only();
drivers/pci/controller/pci-host-common.c:       of_pci_check_probe_only();

The only interesting call site here being pci-host-common.c, which
corresponds to the "new style" platform device based PCI host bridge
probing. Now those use pci_host_probe() in drivers/pci/probe.c, not the 
(legacy ?) arch/arm/kernel/bios32.c variant.

So unless I missed something, should I take out the PCI_PROBE_ONLY
case completely in the arm32 code as part of my series ?

ie:

index ed46ca69813d..f969a1a56ace 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -536,23 +536,13 @@ void pci_common_init_dev(struct device *parent, struct hw_pci *hw)
 
 	list_for_each_entry(sys, &head, node) {
 		struct pci_bus *bus = sys->bus;
+		struct pci_bus *child;
 
-		/*
-		 * We insert PCI resources into the iomem_resource and
-		 * ioport_resource trees in either pci_bus_claim_resources()
-		 * or pci_bus_assign_resources().
-		 */
-		if (pci_has_flag(PCI_PROBE_ONLY)) {
-			pci_bus_claim_resources(bus);
-		} else {
-			struct pci_bus *child;
+		pci_bus_size_bridges(bus);
+		pci_bus_assign_resources(bus);
 
-			pci_bus_size_bridges(bus);
-			pci_bus_assign_resources(bus);
-
-			list_for_each_entry(child, &bus->children, node)
-				pcie_bus_configure_settings(child);
-		}
+		list_for_each_entry(child, &bus->children, node)
+			pcie_bus_configure_settings(child);
 
 		pci_bus_add_devices(bus);
 	}

Cheers,
Ben.



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

PCI resource allocation hell, PCI_PROBE_ONLY, PCI_REASSIGN_ALL_BUS etc...

[Benjamin Herrenschmidt]

Sooo ... I need some feedback from a bunch of people who know about
their arch/platform PCIe controller code :-)

aardvark, mvebu, cadence-host and mediatek and alpha & mips folks...

[Long story short: trying to cleanup and consolidate resource
allocation/assignment accross archs etc... and finding a bunch of
interesting things that I'll need to address (or at least understand)
to be able to move forward, so I need input from the relevant
authors/maintainers. I've CCed a few.]

Please chime in and let me know what you think the intent is so I can
fix these things up.

So first thing: aardvark, mvebu, cadence-host and mediatek, you call
pci_host_probe() directly instead of pci_host_common_probe(), for good
reasons, but that make you miss these:

	of_pci_check_probe_only();

and
	/* Do not reassign resources if probe only */
	if (!pci_has_flag(PCI_PROBE_ONLY))
		pci_add_flags(PCI_REASSIGN_ALL_BUS);

Now, I think probe only should be a platform thing, so it should be
tested always, don't you agree ?

Also, from what I can tell, because you never have it set, you rely
on the generic code always reassigning all resources. That's also where
mips and alpha come in: those archs seem to also rely on all resources
being reallocated when PCI_PROBE_ONLY is not set (well, not 100% sure
about mips yet but alpha for sure).

However they all miss setting PCI_REASSIGN_ALL_BUS. So you will keep
whatever bus numbers were set by FW. I think the generic code is
somewhat smart enough to reassign them if they are completely bogus but
it's still not great... Is that intentional ?

Should we move the above pieces of code to pci_host_probe() instead ?
(That wont help mips and alpha, those would have to get a copy of the
same).

Cheers,
Ben.

Re: Why check for PCI_PROBE_ONLY in pci_common_init_dev()

[Russell King - ARM Linux admin]

On Thu, Jun 13, 2019 at 01:52:34PM +1000, Benjamin Herrenschmidt wrote:
> Hi !
> 
> So while trying to sort out & cleanup the business with PCI resource
> allocation (and do the ground work to be able to revive 
> https://lore.kernel.org/linux-pci/20150916085850.GA17510@red-moon/) I
> stumbled upon this one:
> 
> arch/arm/kernel/bios32.c:pci_common_init_dev() checks for
> PCI_PROBE_ONLY to decide whether to claim existing resources or
> reallocate.
> 
> However, I can't see any code path leading to that function that would
> have set that flag.

Someone probably wanted it at some point but either that's been removed
or the code was never merged.  Either way, if no one is using it on
32-bit ARM, it can be killed.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Why check for PCI_PROBE_ONLY in pci_common_init_dev()

[Benjamin Herrenschmidt]

On Thu, 2019-06-13 at 23:06 +0100, Russell King - ARM Linux admin wrote:
> On Thu, Jun 13, 2019 at 01:52:34PM +1000, Benjamin Herrenschmidt wrote:
> > Hi !
> > 
> > So while trying to sort out & cleanup the business with PCI resource
> > allocation (and do the ground work to be able to revive 
> > https://lore.kernel.org/linux-pci/20150916085850.GA17510@red-moon/) I
> > stumbled upon this one:
> > 
> > arch/arm/kernel/bios32.c:pci_common_init_dev() checks for
> > PCI_PROBE_ONLY to decide whether to claim existing resources or
> > reallocate.
> > 
> > However, I can't see any code path leading to that function that would
> > have set that flag.
> 
> Someone probably wanted it at some point but either that's been removed
> or the code was never merged.  Either way, if no one is using it on
> 32-bit ARM, it can be killed.

It's not used by code path using that function. "New style" stuff using
drivers/pci/controller/* can still use the flag, but at least I can
simplify that code.

Thanks !

Cheers,
Ben.



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel