| Message ID | 20220820115113.30581-1-pali@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | powerpc/pci: Enable PCI domains in /proc when PCI bus numbers are not unique | expand |
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
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
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
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.
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.
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;
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(+)