[PATCH] powerpc/pci: Enable PCI domains in /proc when PCI bus numbers are not unique

[PATCH] powerpc/pci: Enable PCI domains in /proc when PCI bus numbers are not unique

[Pali Rohár]

On 32-bit powerpc systems with more PCIe controllers and more PCI domains,
where on more PCI domains are same PCI numbers, when kernel is compiled
with CONFIG_PROC_FS=y and CONFIG_PPC_PCI_BUS_NUM_DOMAIN_DEPENDENT=y
options, kernel prints "proc_dir_entry 'pci/01' already registered" error
message.

  [    1.708861] ------------[ cut here ]------------
  [    1.713429] proc_dir_entry 'pci/01' already registered
  [    1.718595] WARNING: CPU: 0 PID: 1 at fs/proc/generic.c:377 proc_register+0x1a8/0x1ac
  [    1.726361] Modules linked in:
  [    1.729404] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W         5.19.0-rc5-0caacb197b677410bdac81bc34f05235+ #109
  [    1.740183] NIP:  c02846e8 LR: c02846e8 CTR: c0015154
  [    1.745225] REGS: c146fc90 TRAP: 0700   Tainted: G        W          (5.19.0-rc5-0caacb197b677410bdac81bc34f05235+)
  [    1.755657] MSR:  00029000 <CE,EE,ME>  CR: 28000822  XER: 00000000
  [    1.761829]
  [    1.761829] GPR00: c02846e8 c146fd80 c14a8000 0000002a 3fffefff c146fc40 c146fc38 00000000
  [    1.761829] GPR08: 3fffefff 00000000 00000000 c10ac04c 24000824 00000000 c0004548 00000000
  [    1.761829] GPR16: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000007
  [    1.761829] GPR24: c10000d0 c167da54 c167da00 c1120000 c167dd6c c10b4abc c167dc58 c167dd00
  [    1.796707] NIP [c02846e8] proc_register+0x1a8/0x1ac
  [    1.801663] LR [c02846e8] proc_register+0x1a8/0x1ac
  [    1.806532] Call Trace:
  [    1.808966] [c146fd80] [c02846e8] proc_register+0x1a8/0x1ac (unreliable)
  [    1.815659] [c146fdb0] [c028481c] _proc_mkdir+0x78/0xa4
  [    1.820875] [c146fdd0] [c05a92e4] pci_proc_attach_device+0x11c/0x168
  [    1.827221] [c146fe10] [c101f7a4] pci_proc_init+0x80/0x98
  [    1.832611] [c146fe30] [c0004150] do_one_initcall+0x80/0x284
  [    1.838262] [c146fea0] [c10011a8] kernel_init_freeable+0x1f4/0x2a0
  [    1.844434] [c146fee0] [c000456c] kernel_init+0x24/0x150
  [    1.849737] [c146ff00] [c001326c] ret_from_kernel_thread+0x5c/0x64
  [    1.855910] Instruction dump:
  [    1.858866] 83810020 83a10024 83c10028 83e1002c 38210030 4e800020 809a0064 3c60c0a8
  [    1.866602] 7f85e378 3863af28 4cc63182 4bdb8155 <0fe00000> 9421ffe0 39200000 7c0802a6
  [    1.874513] ---[ end trace 0000000000000000 ]---

This regression started appearing after commit 566356813082 ("powerpc/pci:
Add config option for using all 256 PCI buses") in case in each mPCIe slot
is connected PCIe card and therefore PCI bus 1 is populated in for every
PCIe controller / PCI domain.

The reason is that PCI procfs code expects that when PCI bus numbers are
not unique across all PCI domains, function pci_proc_domain() returns true
for domain dependent buses.

Fix this issue by setting PCI_ENABLE_PROC_DOMAINS and PCI_COMPAT_DOMAIN_0
flags for 32-bit powerpc code when CONFIG_PPC_PCI_BUS_NUM_DOMAIN_DEPENDENT
is enabled. Same approach is already implemented for 64-bit powerpc code
(where PCI bus numbers are always domain dependent).

Fixes: 566356813082 ("powerpc/pci: Add config option for using all 256 PCI buses")
Signed-off-by: Pali Rohár <pali@kernel.org>
---
 arch/powerpc/kernel/pci_32.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/powerpc/kernel/pci_32.c b/arch/powerpc/kernel/pci_32.c
index ffc4e1928c80..8acbc9592ebb 100644
--- a/arch/powerpc/kernel/pci_32.c
+++ b/arch/powerpc/kernel/pci_32.c
@@ -249,6 +249,15 @@ static int __init pcibios_init(void)
 
 	printk(KERN_INFO "PCI: Probing PCI hardware\n");
 
+#ifdef CONFIG_PPC_PCI_BUS_NUM_DOMAIN_DEPENDENT
+	/*
+	 * Enable PCI domains in /proc when PCI bus numbers are not unique
+	 * across all PCI domains to prevent conflicts. And keep PCI domain 0
+	 * backward compatible in /proc for video cards.
+	 */
+	pci_add_flags(PCI_ENABLE_PROC_DOMAINS | PCI_COMPAT_DOMAIN_0);
+#endif
+
 	if (pci_has_flag(PCI_REASSIGN_ALL_BUS))
 		pci_assign_all_buses = 1;
 
-- 
2.20.1

Re: [PATCH] powerpc/pci: Enable PCI domains in /proc when PCI bus numbers are not unique

[Michael Ellerman]

Pali Rohár <pali@kernel.org> writes:
> On 32-bit powerpc systems with more PCIe controllers and more PCI domains,
> where on more PCI domains are same PCI numbers, when kernel is compiled
> with CONFIG_PROC_FS=y and CONFIG_PPC_PCI_BUS_NUM_DOMAIN_DEPENDENT=y
> options, kernel prints "proc_dir_entry 'pci/01' already registered" error
> message.

Thanks, I'll pick this up.

> This regression started appearing after commit 566356813082 ("powerpc/pci:
> Add config option for using all 256 PCI buses") in case in each mPCIe slot
> is connected PCIe card and therefore PCI bus 1 is populated in for every
> PCIe controller / PCI domain.
>
> The reason is that PCI procfs code expects that when PCI bus numbers are
> not unique across all PCI domains, function pci_proc_domain() returns true
> for domain dependent buses.
>
> Fix this issue by setting PCI_ENABLE_PROC_DOMAINS and PCI_COMPAT_DOMAIN_0
> flags for 32-bit powerpc code when CONFIG_PPC_PCI_BUS_NUM_DOMAIN_DEPENDENT
> is enabled. Same approach is already implemented for 64-bit powerpc code
> (where PCI bus numbers are always domain dependent).

We also have the same in ppc4xx_pci_find_bridges().

And if we can eventually make CONFIG_PPC_PCI_BUS_NUM_DOMAIN_DEPENDENT
the standard behaviour on 32-bit then everything would behave the same
and we could simplify pci_proc_domain() to match what other arches do.

cheers


> Fixes: 566356813082 ("powerpc/pci: Add config option for using all 256 PCI buses")
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  arch/powerpc/kernel/pci_32.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/arch/powerpc/kernel/pci_32.c b/arch/powerpc/kernel/pci_32.c
> index ffc4e1928c80..8acbc9592ebb 100644
> --- a/arch/powerpc/kernel/pci_32.c
> +++ b/arch/powerpc/kernel/pci_32.c
> @@ -249,6 +249,15 @@ static int __init pcibios_init(void)
>  
>  	printk(KERN_INFO "PCI: Probing PCI hardware\n");
>  
> +#ifdef CONFIG_PPC_PCI_BUS_NUM_DOMAIN_DEPENDENT
> +	/*
> +	 * Enable PCI domains in /proc when PCI bus numbers are not unique
> +	 * across all PCI domains to prevent conflicts. And keep PCI domain 0
> +	 * backward compatible in /proc for video cards.
> +	 */
> +	pci_add_flags(PCI_ENABLE_PROC_DOMAINS | PCI_COMPAT_DOMAIN_0);
> +#endif
> +
>  	if (pci_has_flag(PCI_REASSIGN_ALL_BUS))
>  		pci_assign_all_buses = 1;
>  
> -- 
> 2.20.1

Re: [PATCH] powerpc/pci: Enable PCI domains in /proc when PCI bus numbers are not unique

[Pali Rohár]

On Thursday 25 August 2022 17:49:28 Michael Ellerman wrote:
> Pali Rohár <pali@kernel.org> writes:
> > On 32-bit powerpc systems with more PCIe controllers and more PCI domains,
> > where on more PCI domains are same PCI numbers, when kernel is compiled
> > with CONFIG_PROC_FS=y and CONFIG_PPC_PCI_BUS_NUM_DOMAIN_DEPENDENT=y
> > options, kernel prints "proc_dir_entry 'pci/01' already registered" error
> > message.
> 
> Thanks, I'll pick this up.
> 
> > This regression started appearing after commit 566356813082 ("powerpc/pci:
> > Add config option for using all 256 PCI buses") in case in each mPCIe slot
> > is connected PCIe card and therefore PCI bus 1 is populated in for every
> > PCIe controller / PCI domain.
> >
> > The reason is that PCI procfs code expects that when PCI bus numbers are
> > not unique across all PCI domains, function pci_proc_domain() returns true
> > for domain dependent buses.
> >
> > Fix this issue by setting PCI_ENABLE_PROC_DOMAINS and PCI_COMPAT_DOMAIN_0
> > flags for 32-bit powerpc code when CONFIG_PPC_PCI_BUS_NUM_DOMAIN_DEPENDENT
> > is enabled. Same approach is already implemented for 64-bit powerpc code
> > (where PCI bus numbers are always domain dependent).
> 
> We also have the same in ppc4xx_pci_find_bridges().
> 
> And if we can eventually make CONFIG_PPC_PCI_BUS_NUM_DOMAIN_DEPENDENT
> the standard behaviour on 32-bit then everything would behave the same
> and we could simplify pci_proc_domain() to match what other arches do.

I sent two patches which do another steps to achieve it:
https://lore.kernel.org/linuxppc-dev/20220817163927.24453-1-pali@kernel.org/t/#u

Main blocker is pci-OF-bus-map which is in direct conflict with
CONFIG_PPC_PCI_BUS_NUM_DOMAIN_DEPENDENT and which used on chrp and pmac.
And I have no idea if pci-OF-bus-map is still needed or not.

> cheers
> 
> 
> > Fixes: 566356813082 ("powerpc/pci: Add config option for using all 256 PCI buses")
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > ---
> >  arch/powerpc/kernel/pci_32.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/arch/powerpc/kernel/pci_32.c b/arch/powerpc/kernel/pci_32.c
> > index ffc4e1928c80..8acbc9592ebb 100644
> > --- a/arch/powerpc/kernel/pci_32.c
> > +++ b/arch/powerpc/kernel/pci_32.c
> > @@ -249,6 +249,15 @@ static int __init pcibios_init(void)
> >  
> >  	printk(KERN_INFO "PCI: Probing PCI hardware\n");
> >  
> > +#ifdef CONFIG_PPC_PCI_BUS_NUM_DOMAIN_DEPENDENT
> > +	/*
> > +	 * Enable PCI domains in /proc when PCI bus numbers are not unique
> > +	 * across all PCI domains to prevent conflicts. And keep PCI domain 0
> > +	 * backward compatible in /proc for video cards.
> > +	 */
> > +	pci_add_flags(PCI_ENABLE_PROC_DOMAINS | PCI_COMPAT_DOMAIN_0);
> > +#endif
> > +
> >  	if (pci_has_flag(PCI_REASSIGN_ALL_BUS))
> >  		pci_assign_all_buses = 1;
> >  
> > -- 
> > 2.20.1

Re: [PATCH] powerpc/pci: Enable PCI domains in /proc when PCI bus numbers are not unique

[Michael Ellerman]

Pali Rohár <pali@kernel.org> writes:
> On Thursday 25 August 2022 17:49:28 Michael Ellerman wrote:
>> Pali Rohár <pali@kernel.org> writes:
>> > On 32-bit powerpc systems with more PCIe controllers and more PCI domains,
>> > where on more PCI domains are same PCI numbers, when kernel is compiled
>> > with CONFIG_PROC_FS=y and CONFIG_PPC_PCI_BUS_NUM_DOMAIN_DEPENDENT=y
>> > options, kernel prints "proc_dir_entry 'pci/01' already registered" error
>> > message.
>> 
>> Thanks, I'll pick this up.
>> 
>> > This regression started appearing after commit 566356813082 ("powerpc/pci:
>> > Add config option for using all 256 PCI buses") in case in each mPCIe slot
>> > is connected PCIe card and therefore PCI bus 1 is populated in for every
>> > PCIe controller / PCI domain.
>> >
>> > The reason is that PCI procfs code expects that when PCI bus numbers are
>> > not unique across all PCI domains, function pci_proc_domain() returns true
>> > for domain dependent buses.
>> >
>> > Fix this issue by setting PCI_ENABLE_PROC_DOMAINS and PCI_COMPAT_DOMAIN_0
>> > flags for 32-bit powerpc code when CONFIG_PPC_PCI_BUS_NUM_DOMAIN_DEPENDENT
>> > is enabled. Same approach is already implemented for 64-bit powerpc code
>> > (where PCI bus numbers are always domain dependent).
>> 
>> We also have the same in ppc4xx_pci_find_bridges().
>> 
>> And if we can eventually make CONFIG_PPC_PCI_BUS_NUM_DOMAIN_DEPENDENT
>> the standard behaviour on 32-bit then everything would behave the same
>> and we could simplify pci_proc_domain() to match what other arches do.
>
> I sent two patches which do another steps to achieve it:
> https://lore.kernel.org/linuxppc-dev/20220817163927.24453-1-pali@kernel.org/t/#u
>
> Main blocker is pci-OF-bus-map which is in direct conflict with
> CONFIG_PPC_PCI_BUS_NUM_DOMAIN_DEPENDENT and which used on chrp and pmac.
> And I have no idea if pci-OF-bus-map is still needed or not.

Yeah thanks, I saw those patches.

I can't find any code that refers to pci-OF-bus-map, so I'm inclined to
remove it entirely.

But I'll do some more searching to see if I can find any references to
it in old code.

cheers

Re: [PATCH] powerpc/pci: Enable PCI domains in /proc when PCI bus numbers are not unique

[Pali Rohár]

On Thursday 01 September 2022 13:53:56 Michael Ellerman wrote:
> Pali Rohár <pali@kernel.org> writes:
> > On Thursday 25 August 2022 17:49:28 Michael Ellerman wrote:
> >> Pali Rohár <pali@kernel.org> writes:
> >> > On 32-bit powerpc systems with more PCIe controllers and more PCI domains,
> >> > where on more PCI domains are same PCI numbers, when kernel is compiled
> >> > with CONFIG_PROC_FS=y and CONFIG_PPC_PCI_BUS_NUM_DOMAIN_DEPENDENT=y
> >> > options, kernel prints "proc_dir_entry 'pci/01' already registered" error
> >> > message.
> >> 
> >> Thanks, I'll pick this up.
> >> 
> >> > This regression started appearing after commit 566356813082 ("powerpc/pci:
> >> > Add config option for using all 256 PCI buses") in case in each mPCIe slot
> >> > is connected PCIe card and therefore PCI bus 1 is populated in for every
> >> > PCIe controller / PCI domain.
> >> >
> >> > The reason is that PCI procfs code expects that when PCI bus numbers are
> >> > not unique across all PCI domains, function pci_proc_domain() returns true
> >> > for domain dependent buses.
> >> >
> >> > Fix this issue by setting PCI_ENABLE_PROC_DOMAINS and PCI_COMPAT_DOMAIN_0
> >> > flags for 32-bit powerpc code when CONFIG_PPC_PCI_BUS_NUM_DOMAIN_DEPENDENT
> >> > is enabled. Same approach is already implemented for 64-bit powerpc code
> >> > (where PCI bus numbers are always domain dependent).
> >> 
> >> We also have the same in ppc4xx_pci_find_bridges().
> >> 
> >> And if we can eventually make CONFIG_PPC_PCI_BUS_NUM_DOMAIN_DEPENDENT
> >> the standard behaviour on 32-bit then everything would behave the same
> >> and we could simplify pci_proc_domain() to match what other arches do.
> >
> > I sent two patches which do another steps to achieve it:
> > https://lore.kernel.org/linuxppc-dev/20220817163927.24453-1-pali@kernel.org/t/#u
> >
> > Main blocker is pci-OF-bus-map which is in direct conflict with
> > CONFIG_PPC_PCI_BUS_NUM_DOMAIN_DEPENDENT and which used on chrp and pmac.
> > And I have no idea if pci-OF-bus-map is still needed or not.
> 
> Yeah thanks, I saw those patches.
> 
> I can't find any code that refers to pci-OF-bus-map, so I'm inclined to
> remove it entirely.
> 
> But I'll do some more searching to see if I can find any references to
> it in old code.
> 
> cheers

From the code itself I have feeling that some external programs or maybe
some firmware can access or use this property. But I have really no idea.

Re: [PATCH] powerpc/pci: Enable PCI domains in /proc when PCI bus numbers are not unique

[Benjamin Herrenschmidt]

On Thu, 2022-09-01 at 13:53 +1000, Michael Ellerman wrote:
> > 
> > I sent two patches which do another steps to achieve it:
> > https://lore.kernel.org/linuxppc-dev/20220817163927.24453-1-pali@kernel.org/t/#u
> > 
> > Main blocker is pci-OF-bus-map which is in direct conflict with
> > CONFIG_PPC_PCI_BUS_NUM_DOMAIN_DEPENDENT and which used on chrp and pmac.
> > And I have no idea if pci-OF-bus-map is still needed or not.
> 
> Yeah thanks, I saw those patches.
> 
> I can't find any code that refers to pci-OF-bus-map, so I'm inclined to
> remove it entirely.
> 
> But I'll do some more searching to see if I can find any references to
> it in old code.

Trying to remember ... :-)

So this is what I recall at this point:

 - Ancient X11 didn't understand domains in /proc and thus would barf,
which was the primary reason for not enabling them always iirc...

 - There might be something else with early PowerMacs (Grand Central
chipset) where we have effectively two domains (gc and chaos) but
overlapping bus numbers. There might still be pre-historical code in
there that assumes it's that way though I can't see anything obvious.
Paul might still have one of these :-) (PowerMac 7200/7500/8500/9500
afaik).

 - pci-OF-bus-map predates the PCI layer keeping track of the PCI/OF
relationship. I don't believe it's still used anywhere in the kernel,
though it's possible (unlikely ?) that some garbage remains in
userspace that does.

At this point, I wouldn't object to tearing this all out and just
having domains always (and see what the fallout is).

Cheers,
Ben.