2022-08-20 12:49:23

by Pali Rohár

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

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 <[email protected]>
---
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


2022-08-25 08:26:33

by Michael Ellerman

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

Pali Rohár <[email protected]> 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 <[email protected]>
> ---
> 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

2022-08-25 08:50:47

by Pali Rohár

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

On Thursday 25 August 2022 17:49:28 Michael Ellerman wrote:
> Pali Rohár <[email protected]> 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/[email protected]/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 <[email protected]>
> > ---
> > 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

2022-09-01 04:13:08

by Michael Ellerman

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

Pali Rohár <[email protected]> writes:
> On Thursday 25 August 2022 17:49:28 Michael Ellerman wrote:
>> Pali Rohár <[email protected]> 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/[email protected]/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

2022-09-01 07:30:57

by Pali Rohár

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

On Thursday 01 September 2022 13:53:56 Michael Ellerman wrote:
> Pali Rohár <[email protected]> writes:
> > On Thursday 25 August 2022 17:49:28 Michael Ellerman wrote:
> >> Pali Rohár <[email protected]> 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/[email protected]/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.

2022-09-23 03:31:43

by Benjamin Herrenschmidt

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

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/[email protected]/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.