Re: [PATCH v2 2/2] powerpc/pci: Prefer PCI domain assignment via DT 'linux,pci-domain' and alias

[Benjamin Herrenschmidt]

On Mon, 2022-08-15 at 15:46 +1000, Michael Ellerman wrote:
> Guenter Roeck <linux@xxxxxxxxxxxx> writes:
> > On Wed, Jul 06, 2022 at 12:21:48PM +0200, Pali Rohár wrote:
> > > Other Linux architectures use DT property 'linux,pci-domain' for specifying
> > > fixed PCI domain of PCI controller specified in Device-Tree.
> > > 
> > > And lot of Freescale powerpc boards have defined numbered pci alias in
> > > Device-Tree for every PCIe controller which number specify preferred PCI
> > > domain.
> > > 
> > > So prefer usage of DT property 'linux,pci-domain' (via function
> > > of_get_pci_domain_nr()) and DT pci alias (via function of_alias_get_id())
> > > on powerpc architecture for assigning PCI domain to PCI controller.
> > > 
> > > Fixes: 63a72284b159 ("powerpc/pci: Assign fixed PHB number based on device-tree properties")
> > > Signed-off-by: Pali Rohár <pali@xxxxxxxxxx>
> > 
> > This patch results in a number of boot warnings with various qemu
> > boot tests.
> 
> Thanks for the report.
> 
> I have automated qemu boot tests to catch things like this, they even
> have DEBUG_ATOMIC_SLEEP enabled ... but I inadvertantly broke my script
> that checks for "BUG:" in the console log. Sometimes you just can't
> win.

So the problem is

 	spin_lock(&hose_spinlock);

get_phb_number() relies on it for the phb_bitmap allocation. You can
move it out of the lock but you'll have to either:

 - Take the lock inside it to protect the allocation

 - Turn find_first_zero_bit/set_bit into a loop of
find_first_zero_bit+test_and_set_bit() which wouldn't require a lock.

Note about the other "reg" numbering conversation ... I'm pretty sure
that breaks some old PowerMac crap which shows nobody really uses these
things considering how long the patch has been there :-)

I'm pretty sure something somewhere assumes the primary bus is 0. Some
old userspace definitely does though that might no longer be relevant.
The whole business with "domain 0" being special and avoiding domain
numbers in /proc relies on this too...

Cheers,
Ben.