Re: [PATCH v5] PCI: use IDA to manage domain number if not getting it from DT

[Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>]

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@rock-chips.com>
> > > >
> > > >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