On Tue, Aug 15, 2017 at 07:23:30AM -0500, Bjorn Helgaas wrote: > On Tue, Aug 15, 2017 at 12:43:16PM +0100, Lorenzo Pieralisi wrote: > > On Tue, Aug 15, 2017 at 03:01:48PM +0800, Shawn Lin wrote: > > > Hi Bjorn, > > > > > > On 2017/8/12 5:17, Bjorn Helgaas wrote: > > > >[+cc Lorenzo, resending because I fat-fingered the cc line and subject] > > > > > > > >On Tue, Jun 27, 2017 at 08:31:13AM +0800, Shawn Lin wrote: > > > >>If not getting domain number from DT, the domain number will > > > >>keep increasing once doing unbind/bind RC drivers. This could > > > >>introduce pointless tree view of lspci as shows below: > > > >> > > > >>-+-[0001:00]---00.0-[01]----00.0 > > > >> \-[0000:00]- > > > >> > > > >>The more test we do, the lengthier it would be. The more serious > > > >>issue is that if attaching two hierarchies for two different domains > > > >>belonging to two root bridges, so when doing unbind/bind test for one > > > >>of them and keep the other, then the domain number would finally > > > >>overflow and make the two hierarchies of devices share the some domain > > > >>number but actually they shouldn't. So it looks like we need to invent > > > >>a new indexing ID mechanism to manage domain number. This patch > > > >>introduces idr to achieve our purpose. > > > >> > > > >>Signed-off-by: Shawn Lin <shawn.lin@xxxxxxxxxxxxxx> > > > > > > > >The "use_dt_domains" logic in of_pci_bus_find_domain_nr() is fairly > > > >obtuse. I *think*, now that we have pci_scan_root_bus_bridge() due to > > > >Lorenzo's excellent work, the time is ripe for moving the domain > > > >number from arch-specific places into struct pci_host_bridge. > > > > > > > >I suspect that will end up simplifying the CONFIG_PCI_DOMAINS vs > > > >CONFIG_PCI_DOMAINS_GENERIC situation, and I wonder whether it might > > > >enable some simplification of of_pci_bus_find_domain_nr() as well, > > > >which in turn, might make *this* patch simpler. > > > > > > > >This isn't that big a patch to begin with, so I could apply it as-is > > > >and we could do more domain cleanup later. It's just that it's > > > >intertwined with the PCI_DOMAINS #ifdefs and maybe there's an > > > >opportunity to make this story more readable if those are out of the > > > >way. Any thoughts? > > > > > > That sounds good to me that aftering add IDA domain, we could start > > > considering moving domain number from arch-specific places into the > > > bridge code and may be could also finally remove the macro > > > CONFIG_PCI_DOMAIN* both? > > > > I need to see how this can be implemented (another hook in > > pci_host_bridge ?) but I suspect we can't get away with arch > > specific bits - or maybe you are referring to having one single > > place where the domain is _assigned_ using an arch specific hook > > (in pci_host_bridge) ? I have to have a look into this, certainly > > this patch should be considered because that atomic counter deserved > > more thought, yes. > > What I was hoping (and I haven't thought this all through) was that we > could: > > - add "domain" to struct pci_host_bridge > > - have callers of pci_scan_root_bus_bridge() assign bridge->domain > alongside their existing bridge->busnr, bridge->ops, etc. > assignments. This would pull a little of the messiness of > pci_bus_find_domain_nr() into the bridge drivers, but they would > know a priori whether to use ACPI or DT, so we wouldn't need quite > as much guesswork. > > - replace the pci_bus_find_domain_nr() call in > pci_register_host_bridge() with "bus->domain_nr = bridge->domain" > > - replace the arch-specific pci_domain_nr() implementations with a > generic one > > - add IDA alloc to the DT domain number alloc path Yes, if we accept that arch code has to play a role in setting the domain number I think that's doable but I have to have a look into ACPI for this to work since this means that I have to convert x86/ia64 (and powerpc, not sure about this) to the new bus scanning API. For the DT host bridges and arches I have already converted that should be relatively easy (well, another big series), I have to have a proper look into it. Yes, overall it makes perfect sense. Thanks, Lorenzo
