On 2019-06-24 3:13 a.m., Benjamin Herrenschmidt wrote: > So I'm staring at these three mostly at this point: > > void pci_assign_unassigned_root_bus_resources(struct pci_bus *bus) > void pci_assign_unassigned_bridge_resources(struct pci_dev *bridge) > void pci_assign_unassigned_bus_resources(struct pci_bus *bus) > > Now we have 3 functions that fundamentally have the same purpose, > assign what was left unassigned down a PCI hierarchy, but are going > about it in quite a different manner. > > Now to make things worse, there's little consistency in which one gets > called where. We have PCI controllers calling the first one sometimes, > the last one sometimes, or doing the manual: > > pci_bus_size_bridges(bus); > pci_bus_assign_resources(bus); > > Or variants with pci_bus_size_bridges sometimes missing etc... I suspect there isn't much rhyme or reason to it. None of this is well documented so developers writing the controller drivers probably didn't have a good idea of what the correct thing to do was, and just stuck with the first thing that worked. > Now I've consolidated a lot of that and removed all of those "manual" > cases in my work-in-progress branch, but I'd like to clarify and > possibly remove the 3 ones above. > > Let's start with the last one, pci_assign_unassigned_bus_resources, as > it's the easiest to remove from users in drivers/pci/controller/* (and > replace with pci_assign_unassigned_root_bus_resources typically). > > This leaves it used in a couple of corner cases, most of them I think > I can kill, and .... sysfs 'rescan'. > > The interesting thing about that function is that it tries to avoid > resizing the bridge of the bus passed as an argument, it will only > resize subordinate bridges. From the changelog it was created for > hotplug bridges, but almost none uses it (some powerpc stuff I can > probably kill) ... and sysfs rescan. > > I wonder what's the remaining purpose of it. sysfs rescan could > probably be cleaned up to use the two first... Also why avoid resizing > the bridge itself ? > > That leads to the difference between > pci_assign_unassigned_root_bus_resources() > and pci_assign_unassigned_bridge_resources(). > > The names are misleading. The former isn't just about the root bus > resources. It's about the entire tree underneath the root bus. > > The main difference that I can tell are: > > - pci_assign_unassigned_root_bus_resources() may or may not try to > realloc, depending on a combination of command line args, config > option, presence of IOV devices etc... while > pci_assign_unassigned_bridge_resources() always will > > - pci_assign_unassigned_bridge_resources() will call > pci_bridge_distribute_available_resources() to distribute resource to > child hotplug bridges, while pci_assign_unassigned_root_bus_resources() > won't. > > Now, are we 100% confident we want to keep those discrepancies ? > > It feels like the former function is intended for boot time resource > allocation, and the latter for hotplug, but I can't make sense of why > the resources of a device behind a hotplug bridge should be allocated > differently depending on whether that device was plugged at boot or > plugged later. I don't really know, but I kind of assumed reallocing any time but early in boot would be dangerous. It involves un-assigning a bunch of resources without any real check to see if a driver is using them or not. If they were being used by a driver (which is typical) and they were reassigned, everything would break. I mean, in theory the code could/should be the same for both paths and it could just make a single, better decision on whether to realloc or not. But that's going to be challenging to get there. > Also why not distribute available resources at boot between top level > hotplug bridges ? > > I'm not even going into the question of why the resource > sizing/assignment code is so obscure/cryptic/incomprehensible, that's > another kettle of fish, but I'd like to at least clarify the usage > patterns a bit better. I got the impression the code was designed to generally let the firmware set things up -- it just fixed things up if the firmware messed it up somehow. My guess would be it evolved out of a bunch of hacks designed to fix broken bioses into something new platforms used to do full enumeration (because it happened to work). Logan
