diff mbox series

[REGRESSION] mipsel: no RTC CMOS on the Malta platform in QEMU

Message ID 90b5b76d-25b6-4cdc-91ed-07ac930dc519@o2.pl (mailing list archive)
State New
Headers show
Series [REGRESSION] mipsel: no RTC CMOS on the Malta platform in QEMU | expand

Commit Message

Mateusz Jończyk Jan. 13, 2025, 10:16 p.m. UTC
Hello,

On Linux 6.13-rc6 for mipsel in QEMU on the Malta platform, the RTC CMOS
driver does not load and /sys/class/rtc is empty. I have tested this with
"make malta_defconfig", which compiles this driver into the kernel 
(CONFIG_RTC_DRV_CMOS=y).

I have bisected this down to:

commit 4bfb53e7d317c01f296b2feb2fae7c421c1d52dc
Author: Jiaxun Yang<jiaxun.yang@flygoat.com>
Date:   Thu Sep 21 19:04:22 2023 +0800

     mips: add <asm-generic/io.h> including
     With the adding, some default ioremap_xx methods defined in
     asm-generic/io.h can be used. E.g the default ioremap_uc() returning
     NULL.
     We also massaged various headers to avoid nested includes.

I have tried to debug this.

The fallout is apparently limited to the CMOS RTC driver, other
drivers that access IO ports seem to function correctly (e.g. the
PATA driver). Also, the read_persistent_clock64 function in
arch/mips/mti-malta/malta-time.c, which accesses the same hardware
works correctly.

The CMOS RTC driver is likely special because this device is defined
in a devicetree (arch/mips/boot/dts/mti/malta.dts) and there it is
the only defined device on the ISA bus.

That driver fails to load because the call to

platform_get_resource(pdev, IORESOURCE_IO, 0);

in cmos_platform_probe in drivers/rtc/rtc-cmos.c returns NULL.

The mediator seems to be that this bad commit causes 
arch/mips/include/asm/io.h
to #include <asm-generic/io.h> at the end. As a side effect, this causes
the PCI_IOBASE macro to be defined:

#ifndef PCI_IOBASE
#define PCI_IOBASE ((void __iomem *)0)
#endif

That PCI_IOBASE value above is AFAIK incorrect for MIPS (it should be
defined to mips_io_port_base as far as I can tell), but this does not 
matter much here.

When that macro is defined, pci_address_to_pio() in pci.c calls the
logic_pio_trans_cpuaddr() function, which fails. Removing that ifdef
in this function "fixes" the issue and allows that driver to load and work
apparently correctly:

----------8<------------------
LOGIC PIO: addr 0x00000070 not registered in io_range_list

(0x0070 is the IO port address of the CMOS RTC).

When I added dump_stack() in logic_pio_trans_cpuaddr(), which prints 
this warning,
I got the following:

----------8<------------------
Call Trace:
[<801185c8>] show_stack+0x38/0x118
[<8010be24>] dump_stack_lvl+0x7c/0xbc
[<80841060>] logic_pio_trans_cpuaddr+0x88/0x98
[<80604bec>] __of_address_to_resource+0x208/0x228
[<805ff45c>] of_device_alloc+0x7c/0x1ac
[<805ff778>] of_platform_device_create_pdata+0x60/0xf8
[<805ff9cc>] of_platform_bus_create+0x1b0/0x238
[<805ffa14>] of_platform_bus_create+0x1f8/0x238
[<805ffbd8>] of_platform_populate+0x80/0xf8
[<80a60008>] of_platform_default_populate_init+0xcc/0xe4
[<8011010c>] do_one_initcall+0x50/0x2b4
[<80a3d0f0>] kernel_init_freeable+0x1f8/0x2a0
[<8086975c>] kernel_init+0x24/0x118
[<801124f8>] ret_from_kernel_thread+0x14/0x1c
----------8<------------------

It appears that some call to logic_pio_register_range() from 
lib/logic_pio.c is missing.
Perhaps the reserve_pio_range() function in arch/mips/loongson64/init.c 
could
be reused, but that's too deep water for me.

Steps to reproduce:

----------8<------------------

wget 
https://ftp.debian.org/debian/dists/Debian12.9/main/installer-mipsel/current/images/malta/netboot/initrd.gz

CROSS_COMPILE=mipsel-linux-gnu- ARCH=mips make malta_defconfig

CROSS_COMPILE=mipsel-linux-gnu- ARCH=mips make -j4

qemu-system-mipsel -machine malta -cpu 24Kf -m 1g -nographic -kernel 
vmlinuz \
        -initrd initrd.gz \
        -append "console=ttyS0 debug ignore_loglevel"

----------8<------------------


Greetings,

Mateusz

Comments

Jiaxun Yang Jan. 13, 2025, 11:29 p.m. UTC | #1
在2025年1月13日一月 下午10:16,Mateusz Jończyk写道:
> Hello,
>
> On Linux 6.13-rc6 for mipsel in QEMU on the Malta platform, the RTC CMOS
> driver does not load and /sys/class/rtc is empty. I have tested this with
> "make malta_defconfig", which compiles this driver into the kernel 
> (CONFIG_RTC_DRV_CMOS=y).

Hi Mateusz,

Thanks for tracking it down, this is indeed a huge effort.

>
> I have bisected this down to:
>
> commit 4bfb53e7d317c01f296b2feb2fae7c421c1d52dc
> Author: Jiaxun Yang<jiaxun.yang@flygoat.com>
> Date:   Thu Sep 21 19:04:22 2023 +0800
>
>      mips: add <asm-generic/io.h> including
>      With the adding, some default ioremap_xx methods defined in
>      asm-generic/io.h can be used. E.g the default ioremap_uc() returning
>      NULL.
>      We also massaged various headers to avoid nested includes.

#regzbot introduced: 4bfb53e7d317c01f296b2feb2fae7c421c1d52dc

>
> I have tried to debug this.
>
> The fallout is apparently limited to the CMOS RTC driver, other
> drivers that access IO ports seem to function correctly (e.g. the
> PATA driver). Also, the read_persistent_clock64 function in
> arch/mips/mti-malta/malta-time.c, which accesses the same hardware
> works correctly.
>
> The CMOS RTC driver is likely special because this device is defined
> in a devicetree (arch/mips/boot/dts/mti/malta.dts) and there it is
> the only defined device on the ISA bus.
>
> That driver fails to load because the call to
>
> platform_get_resource(pdev, IORESOURCE_IO, 0);
>
> in cmos_platform_probe in drivers/rtc/rtc-cmos.c returns NULL.
>
> The mediator seems to be that this bad commit causes 
> arch/mips/include/asm/io.h
> to #include <asm-generic/io.h> at the end. As a side effect, this causes
> the PCI_IOBASE macro to be defined:
>
> #ifndef PCI_IOBASE
> #define PCI_IOBASE ((void __iomem *)0)
> #endif
>
> That PCI_IOBASE value above is AFAIK incorrect for MIPS (it should be
> defined to mips_io_port_base as far as I can tell), but this does not 
> matter much here.

You are right, this is what should be done.

A quick fix would be #undef PCI_IOBASE in arch/mips/include/asm/io.h
just after including #include <asm-generic/io.h>, with ralink and loongson64
as exception.

In the long term, we should scrutinize platform usage of mips_io_base
following ralink's approach.

>
> When that macro is defined, pci_address_to_pio() in pci.c calls the
> logic_pio_trans_cpuaddr() function, which fails. Removing that ifdef
> in this function "fixes" the issue and allows that driver to load and work
> apparently correctly:
>
> ----------8<------------------
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 661f98c6c63a..368cd9ca6801 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4213,14 +4213,10 @@ EXPORT_SYMBOL_GPL(pci_pio_to_address);
> unsigned long __weak pci_address_to_pio(phys_addr_t address)
> {
> -#ifdef PCI_IOBASE
> - return logic_pio_trans_cpuaddr(address);
> -#else
> if (address > IO_SPACE_LIMIT)
> return (unsigned long)-1;
> return (unsigned long) address;
> -#endif
> }
> /**
> ----------8<------------------
>
> Additionally, the following message appears in dmesg on affected kernels:
>
> LOGIC PIO: addr 0x00000070 not registered in io_range_list
>
> (0x0070 is the IO port address of the CMOS RTC).
>
> When I added dump_stack() in logic_pio_trans_cpuaddr(), which prints 
> this warning,
> I got the following:
>
> ----------8<------------------
> Call Trace:
> [<801185c8>] show_stack+0x38/0x118
> [<8010be24>] dump_stack_lvl+0x7c/0xbc
> [<80841060>] logic_pio_trans_cpuaddr+0x88/0x98
> [<80604bec>] __of_address_to_resource+0x208/0x228
> [<805ff45c>] of_device_alloc+0x7c/0x1ac
> [<805ff778>] of_platform_device_create_pdata+0x60/0xf8
> [<805ff9cc>] of_platform_bus_create+0x1b0/0x238
> [<805ffa14>] of_platform_bus_create+0x1f8/0x238
> [<805ffbd8>] of_platform_populate+0x80/0xf8
> [<80a60008>] of_platform_default_populate_init+0xcc/0xe4
> [<8011010c>] do_one_initcall+0x50/0x2b4
> [<80a3d0f0>] kernel_init_freeable+0x1f8/0x2a0
> [<8086975c>] kernel_init+0x24/0x118
> [<801124f8>] ret_from_kernel_thread+0x14/0x1c
> ----------8<------------------
>
> It appears that some call to logic_pio_register_range() from 
> lib/logic_pio.c is missing.
> Perhaps the reserve_pio_range() function in arch/mips/loongson64/init.c 
> could
> be reused, but that's too deep water for me.

Sadly that is not going to work as those MIPS platforms are not using
vmmap for PCI IO access :-(

>
> Steps to reproduce:
>
> ----------8<------------------
>
> wget 
> https://ftp.debian.org/debian/dists/Debian12.9/main/installer-mipsel/current/images/malta/netboot/initrd.gz
>
> CROSS_COMPILE=mipsel-linux-gnu- ARCH=mips make malta_defconfig
>
> CROSS_COMPILE=mipsel-linux-gnu- ARCH=mips make -j4
>
> qemu-system-mipsel -machine malta -cpu 24Kf -m 1g -nographic -kernel 
> vmlinuz \
>         -initrd initrd.gz \
>         -append "console=ttyS0 debug ignore_loglevel"
>
> ----------8<------------------
>
>
> Greetings,
>
> Mateusz
Arnd Bergmann Jan. 14, 2025, 7:01 a.m. UTC | #2
On Tue, Jan 14, 2025, at 00:29, Jiaxun Yang wrote:
> 在2025年1月13日一月 下午10:16,Mateusz Jończyk写道:
>> The mediator seems to be that this bad commit causes 
>> arch/mips/include/asm/io.h
>> to #include <asm-generic/io.h> at the end. As a side effect, this causes
>> the PCI_IOBASE macro to be defined:
>>
>> #ifndef PCI_IOBASE
>> #define PCI_IOBASE ((void __iomem *)0)
>> #endif
>>
>> That PCI_IOBASE value above is AFAIK incorrect for MIPS (it should be
>> defined to mips_io_port_base as far as I can tell), but this does not 
>> matter much here.
>
> You are right, this is what should be done.
>
> A quick fix would be #undef PCI_IOBASE in arch/mips/include/asm/io.h
> just after including #include <asm-generic/io.h>, with ralink and loongson64
> as exception.
>
> In the long term, we should scrutinize platform usage of mips_io_base
> following ralink's approach.

I think we are close to the point of being able to remove the broken
default PCI_IOBASE: the NULL pointer here is almost always wrong, and
mainly existed to shut up build failures on architectures that have
no port I/O at all. I know that sparc32 and m68k have cases that
actually rely on the broken PCI_IOBASE, so those need a local workaround,
not sure if some mips platform also falls into this category, as
I have not looked here in detail.

Hopefully we can get to a point where any reference to port I/O
(inb/outb, PCI_IOPORT, mips_io_port_base, ...) is guarded by
an #ifdef CONFIG_HAS_IOPORT check, and this is set exactly on
those platforms that set mips_io_port_base to a valid address.

      Arnd
Geert Uytterhoeven Jan. 14, 2025, 8:02 a.m. UTC | #3
Hi Jiaxun,

On Tue, Jan 14, 2025 at 12:32 AM Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
> 在2025年1月13日一月 下午10:16,Mateusz Jończyk写道:
> > On Linux 6.13-rc6 for mipsel in QEMU on the Malta platform, the RTC CMOS
> > driver does not load and /sys/class/rtc is empty. I have tested this with
> > "make malta_defconfig", which compiles this driver into the kernel
> > (CONFIG_RTC_DRV_CMOS=y).
>
> Hi Mateusz,
>
> Thanks for tracking it down, this is indeed a huge effort.
>
> >
> > I have bisected this down to:
> >
> > commit 4bfb53e7d317c01f296b2feb2fae7c421c1d52dc
> > Author: Jiaxun Yang<jiaxun.yang@flygoat.com>
> > Date:   Thu Sep 21 19:04:22 2023 +0800
> >
> >      mips: add <asm-generic/io.h> including
> >      With the adding, some default ioremap_xx methods defined in
> >      asm-generic/io.h can be used. E.g the default ioremap_uc() returning
> >      NULL.
> >      We also massaged various headers to avoid nested includes.
>
> #regzbot introduced: 4bfb53e7d317c01f296b2feb2fae7c421c1d52dc
>
> >
> > I have tried to debug this.
> >
> > The fallout is apparently limited to the CMOS RTC driver, other
> > drivers that access IO ports seem to function correctly (e.g. the
> > PATA driver). Also, the read_persistent_clock64 function in
> > arch/mips/mti-malta/malta-time.c, which accesses the same hardware
> > works correctly.
> >
> > The CMOS RTC driver is likely special because this device is defined
> > in a devicetree (arch/mips/boot/dts/mti/malta.dts) and there it is
> > the only defined device on the ISA bus.
> >
> > That driver fails to load because the call to
> >
> > platform_get_resource(pdev, IORESOURCE_IO, 0);
> >
> > in cmos_platform_probe in drivers/rtc/rtc-cmos.c returns NULL.
> >
> > The mediator seems to be that this bad commit causes
> > arch/mips/include/asm/io.h
> > to #include <asm-generic/io.h> at the end. As a side effect, this causes
> > the PCI_IOBASE macro to be defined:
> >
> > #ifndef PCI_IOBASE
> > #define PCI_IOBASE ((void __iomem *)0)
> > #endif
> >
> > That PCI_IOBASE value above is AFAIK incorrect for MIPS (it should be
> > defined to mips_io_port_base as far as I can tell), but this does not
> > matter much here.
>
> You are right, this is what should be done.
>
> A quick fix would be #undef PCI_IOBASE in arch/mips/include/asm/io.h
> just after including #include <asm-generic/io.h>, with ralink and loongson64
> as exception.

Shouldn't arch/mips/include/asm/io.h do

    #define PCI_IOBASE mips_io_port_base

unconditionally, _before_ including  <asm-generic/io.h>?

> In the long term, we should scrutinize platform usage of mips_io_base
> following ralink's approach.

Currently ralink handles that in a mach-specific include:

    arch/mips/include/asm/mach-ralink/spaces.h:#define PCI_IOBASE
mips_io_port_base

Loongson does it differently:

    arch/mips/include/asm/mach-loongson64/spaces.h:#define PCI_IOBASE
     _AC(0xc000000000000000 + SZ_128K, UL)

But still sets mips_io_port_base in prom_init():

    arch/mips/loongson64/init.c:    set_io_port_base(PCI_IOBASE);

so defining PCI_IOBASE to mips_io_port_base in the main <asm/io.h>
should work.

Gr{oetje,eeting}s,

                        Geert
Arnd Bergmann Jan. 14, 2025, 9:59 a.m. UTC | #4
On Tue, Jan 14, 2025, at 09:02, Geert Uytterhoeven wrote:
> On Tue, Jan 14, 2025 at 12:32 AM Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
>> 在2025年1月13日一月 下午10:16,Mateusz Jończyk写道:

>> > #ifndef PCI_IOBASE
>> > #define PCI_IOBASE ((void __iomem *)0)
>> > #endif
>> >
>> > That PCI_IOBASE value above is AFAIK incorrect for MIPS (it should be
>> > defined to mips_io_port_base as far as I can tell), but this does not
>> > matter much here.
>>
>> You are right, this is what should be done.
>>
>> A quick fix would be #undef PCI_IOBASE in arch/mips/include/asm/io.h
>> just after including #include <asm-generic/io.h>, with ralink and loongson64
>> as exception.
>
> Shouldn't arch/mips/include/asm/io.h do
>
>     #define PCI_IOBASE mips_io_port_base
>
> unconditionally, _before_ including  <asm-generic/io.h>?

Yes, I think this would make the most sense, but the ordering
with the PCI initialization needs to be done carefully,
to ensure that PCI_IOBASE has its final value before the first
call to pci_remap_iospace().

>> In the long term, we should scrutinize platform usage of mips_io_base
>> following ralink's approach.
>
> Currently ralink handles that in a mach-specific include:
>
>     arch/mips/include/asm/mach-ralink/spaces.h:#define PCI_IOBASE
> mips_io_port_base
>
> Loongson does it differently:
>
>     arch/mips/include/asm/mach-loongson64/spaces.h:#define PCI_IOBASE
>      _AC(0xc000000000000000 + SZ_128K, UL)
>
> But still sets mips_io_port_base in prom_init():
>
>     arch/mips/loongson64/init.c:    set_io_port_base(PCI_IOBASE);
>
> so defining PCI_IOBASE to mips_io_port_base in the main <asm/io.h>
> should work.

This probably requires calling set_io_port_base() with the
actual virtual address rather than self-assigning the
uninitialized mips_io_port_base.

I assume the reason for loongson64 being different from every
other mips platform is the same as why it calls into the
logic_pio_register_range() directly. I don't understand that
code, but it's probably because it has ISA/LPC devices that
are directly wired to a non-memory-mapped set of registers
instead of them being behind a PCI bridge like the other
platforms. The idea of logic_pio is to have a more generic
way to redirect arbitrary port ranges into bus specific
function calls, where normal PCI (on non-x86) assumes that
all I/O ports are mapped into a small contiguous ranges
of virtual addresses.

   Arnd
Jiaxun Yang Jan. 14, 2025, 10:09 a.m. UTC | #5
在2025年1月14日一月 上午8:02,Geert Uytterhoeven写道:
> Hi Jiaxun,
[...]
>>
>> You are right, this is what should be done.
>>
>> A quick fix would be #undef PCI_IOBASE in arch/mips/include/asm/io.h
>> just after including #include <asm-generic/io.h>, with ralink and loongson64
>> as exception.
>
> Shouldn't arch/mips/include/asm/io.h do
>
>     #define PCI_IOBASE mips_io_port_base
>
> unconditionally, _before_ including  <asm-generic/io.h>?

The problem here is defining PCI_IOBASE implied use of logic_pio and VM mapped
io access, which is not true for many MIPS systems...

>
>> In the long term, we should scrutinize platform usage of mips_io_base
>> following ralink's approach.
>
> Currently ralink handles that in a mach-specific include:
>
>     arch/mips/include/asm/mach-ralink/spaces.h:#define PCI_IOBASE
> mips_io_port_base
>
> Loongson does it differently:
>
>     arch/mips/include/asm/mach-loongson64/spaces.h:#define PCI_IOBASE
>      _AC(0xc000000000000000 + SZ_128K, UL)
>
> But still sets mips_io_port_base in prom_init():
>
>     arch/mips/loongson64/init.c:    set_io_port_base(PCI_IOBASE);
>
> so defining PCI_IOBASE to mips_io_port_base in the main <asm/io.h>
> should work.

Yes, we need to make sure malta problem won't happen on other platforms
before making this definition. 

Thanks

>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> -- 
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
Jiaxun Yang Jan. 14, 2025, 10:22 a.m. UTC | #6
在2025年1月14日一月 上午9:59,Arnd Bergmann写道:
[...]
>
> This probably requires calling set_io_port_base() with the
> actual virtual address rather than self-assigning the
> uninitialized mips_io_port_base.
>
> I assume the reason for loongson64 being different from every
> other mips platform is the same as why it calls into the
> logic_pio_register_range() directly. I don't understand that
> code, but it's probably because it has ISA/LPC devices that
> are directly wired to a non-memory-mapped set of registers
> instead of them being behind a PCI bridge like the other
> platforms. The idea of logic_pio is to have a more generic
> way to redirect arbitrary port ranges into bus specific
> function calls, where normal PCI (on non-x86) assumes that
> all I/O ports are mapped into a small contiguous ranges
> of virtual addresses.

That's correct, Loongson systems has a memory-mapped LPC bridge
accessible via MMIO. We are handling registration and mapping
process here.

It also has ioport capable PCI bridge, which will be taken care by
platform code.

I think current problem of logic_pio is it handles creation of
mappings and PIO range registration separately.

Thanks
>
>    Arnd
Arnd Bergmann Jan. 14, 2025, 10:30 a.m. UTC | #7
On Tue, Jan 14, 2025, at 11:09, Jiaxun Yang wrote:
> 在2025年1月14日一月 上午8:02,Geert Uytterhoeven写道:
>>
>> Shouldn't arch/mips/include/asm/io.h do
>>
>>     #define PCI_IOBASE mips_io_port_base
>>
>> unconditionally, _before_ including  <asm-generic/io.h>?
>
> The problem here is defining PCI_IOBASE implied use of logic_pio and VM mapped
> io access, which is not true for many MIPS systems...
>

I don't think that was ever meant to be the intention of the
#ifdef in drivers/pci/pci.c. Checking for PCI_IOBASE there
should mainly ensure the default
pci_pio_to_address/pci_address_to_pio don't cause a link
failure on architectures that don't have any memory mapped
PIO at all.

For MIPS platforms that don't need logic_pio because all of
the PIO space is mapped to a fixed physical address, the
default pci_remap_iospace/pci_pio_to_address/pci_address_to_pio/
pci_register_io_range could just be replaced with a trivial
architecture/platform specific version.

    Arnd
Maciej W. Rozycki Jan. 14, 2025, 4:11 p.m. UTC | #8
On Tue, 14 Jan 2025, Arnd Bergmann wrote:

> >> A quick fix would be #undef PCI_IOBASE in arch/mips/include/asm/io.h
> >> just after including #include <asm-generic/io.h>, with ralink and loongson64
> >> as exception.
> >
> > Shouldn't arch/mips/include/asm/io.h do
> >
> >     #define PCI_IOBASE mips_io_port_base
> >
> > unconditionally, _before_ including  <asm-generic/io.h>?
> 
> Yes, I think this would make the most sense, but the ordering
> with the PCI initialization needs to be done carefully,
> to ensure that PCI_IOBASE has its final value before the first
> call to pci_remap_iospace().

 Is defining PCI_IOBASE going to do the right thing for non-PCI MIPS 
platforms, or should the definition be #ifdef CONFIG_PCI rather than 
unconditional?  FWIW I think all PCI MIPS platforms support port I/O.

  Maciej
Arnd Bergmann Jan. 14, 2025, 5:04 p.m. UTC | #9
On Tue, Jan 14, 2025, at 17:11, Maciej W. Rozycki wrote:
> On Tue, 14 Jan 2025, Arnd Bergmann wrote:
>> >> A quick fix would be #undef PCI_IOBASE in arch/mips/include/asm/io.h
>> >> just after including #include <asm-generic/io.h>, with ralink and loongson64
>> >> as exception.
>> >
>> > Shouldn't arch/mips/include/asm/io.h do
>> >
>> >     #define PCI_IOBASE mips_io_port_base
>> >
>> > unconditionally, _before_ including  <asm-generic/io.h>?
>> 
>> Yes, I think this would make the most sense, but the ordering
>> with the PCI initialization needs to be done carefully,
>> to ensure that PCI_IOBASE has its final value before the first
>> call to pci_remap_iospace().
>
>  Is defining PCI_IOBASE going to do the right thing for non-PCI MIPS 
> platforms, or should the definition be #ifdef CONFIG_PCI rather than 
> unconditional?  FWIW I think all PCI MIPS platforms support port I/O.

PCI_IOBASE should be defined whenever CONFIG_HAS_IOPORT is set.
Ideally that should allow using the generic inb/outb and
ioread/iowrite helpers from include/asm-generic/io.h, but
unfortunately those don't support the address swizzling required
on SGI and Octeon platforms.

These platforms look like they currently set a NULL pointer
as the I/O port base:

arch/mips/alchemy/common/setup.c:       set_io_port_base(0);
arch/mips/ath79/setup.c:        set_io_port_base(KSEG1);
arch/mips/bcm63xx/setup.c:      set_io_port_base(0);
arch/mips/bmips/setup.c:        set_io_port_base(0);
arch/mips/lantiq/prom.c:        set_io_port_base((unsigned long) KSEG1);

At least some of these, possibly all, also have a PCI or PCMCIA
host controller driver that sets a different value later
when that bus is probed.

I don't see any I/O space getting set up for ath25, dec, ingenic,
loongson32, pic32, eyeq, nintendo64, and realtek-rtl. It looks to
me like any I/O port access on these turns into a misaligned NULL
pointer dereference, but there is a good chance I'm missing how
it gets set up there.

       Arnd.
Jiaxun Yang Jan. 14, 2025, 5:56 p.m. UTC | #10
在2025年1月14日一月 下午4:11,Maciej W. Rozycki写道:
> On Tue, 14 Jan 2025, Arnd Bergmann wrote:
>
>> >> A quick fix would be #undef PCI_IOBASE in arch/mips/include/asm/io.h
>> >> just after including #include <asm-generic/io.h>, with ralink and loongson64
>> >> as exception.
>> >
>> > Shouldn't arch/mips/include/asm/io.h do
>> >
>> >     #define PCI_IOBASE mips_io_port_base
>> >
>> > unconditionally, _before_ including  <asm-generic/io.h>?
>> 
>> Yes, I think this would make the most sense, but the ordering
>> with the PCI initialization needs to be done carefully,
>> to ensure that PCI_IOBASE has its final value before the first
>> call to pci_remap_iospace().
>
>  Is defining PCI_IOBASE going to do the right thing for non-PCI MIPS 
> platforms, or should the definition be #ifdef CONFIG_PCI rather than 
> unconditional?  FWIW I think all PCI MIPS platforms support port I/O.

I think the right thing to do is to unselect HAS_IOPORT for those
platforms.

Thanks

>
>   Maciej
Maciej W. Rozycki Jan. 14, 2025, 7:10 p.m. UTC | #11
On Tue, 14 Jan 2025, Arnd Bergmann wrote:

> >> Yes, I think this would make the most sense, but the ordering
> >> with the PCI initialization needs to be done carefully,
> >> to ensure that PCI_IOBASE has its final value before the first
> >> call to pci_remap_iospace().
> >
> >  Is defining PCI_IOBASE going to do the right thing for non-PCI MIPS 
> > platforms, or should the definition be #ifdef CONFIG_PCI rather than 
> > unconditional?  FWIW I think all PCI MIPS platforms support port I/O.
> 
> PCI_IOBASE should be defined whenever CONFIG_HAS_IOPORT is set.

 Right, this will be more correctly scoped, so #ifdef CONFIG_HAS_IOPORT 
then.

> Ideally that should allow using the generic inb/outb and
> ioread/iowrite helpers from include/asm-generic/io.h, but
> unfortunately those don't support the address swizzling required
> on SGI and Octeon platforms.

 Address swizzling should be generic: for an endianness boundary crossing 
depending on its wiring there's always either a byte-lane or a bit-lane 
matching policy (of course both may be implemented in the same system via 
different mapping windows, such as with the Broadcom SiByte BCM1250 SoC 
though we only use one in Linux, or with some kind of a bus configuration 
register) and either byte swapping or address swizzling is required 
accordingly for the relevant use cases.  But I guess for just a bunch of 
systems that implement the bit-lane matching policy there's little 
incentive for complicating the generic helpers.

> These platforms look like they currently set a NULL pointer
> as the I/O port base:
> 
> arch/mips/alchemy/common/setup.c:       set_io_port_base(0);
> arch/mips/ath79/setup.c:        set_io_port_base(KSEG1);
> arch/mips/bcm63xx/setup.c:      set_io_port_base(0);
> arch/mips/bmips/setup.c:        set_io_port_base(0);
> arch/mips/lantiq/prom.c:        set_io_port_base((unsigned long) KSEG1);
> 
> At least some of these, possibly all, also have a PCI or PCMCIA
> host controller driver that sets a different value later
> when that bus is probed.

$ grep set_io_port_base arch/mips/pci/*

reveals some, i.e. alchemy, and bmips I believe uses the generic handler.  
Other ones may not support port I/O after all, especially where the 
platform in question is meant to be used in PCI device mode.

> I don't see any I/O space getting set up for ath25, dec, ingenic,
> loongson32, pic32, eyeq, nintendo64, and realtek-rtl. It looks to
> me like any I/O port access on these turns into a misaligned NULL
> pointer dereference, but there is a good chance I'm missing how
> it gets set up there.

 There's no PCI with the dec platform (i.e. either no I/O bus at all or 
TURBOchannel), so the PCI I/O space is irrelevant, and obviously the MIPS 
architecture has no native I/O space.  I don't know about the other ones, 
but I supppose at least some of them could be SoCs or systems with no PCI 
either.

  Maciej
Arnd Bergmann Jan. 15, 2025, 8:15 a.m. UTC | #12
On Tue, Jan 14, 2025, at 20:10, Maciej W. Rozycki wrote:
> On Tue, 14 Jan 2025, Arnd Bergmann wrote:
>
>> Ideally that should allow using the generic inb/outb and
>> ioread/iowrite helpers from include/asm-generic/io.h, but
>> unfortunately those don't support the address swizzling required
>> on SGI and Octeon platforms.
>
>  Address swizzling should be generic: for an endianness boundary crossing 
> depending on its wiring there's always either a byte-lane or a bit-lane 
> matching policy (of course both may be implemented in the same system via 
> different mapping windows, such as with the Broadcom SiByte BCM1250 SoC 
> though we only use one in Linux, or with some kind of a bus configuration 
> register) and either byte swapping or address swizzling is required 
> accordingly for the relevant use cases.  But I guess for just a bunch of 
> systems that implement the bit-lane matching policy there's little 
> incentive for complicating the generic helpers.

I checked the other architectures as well now, and I'm fairly
sure we had some architectures in the past that implemented the
same swizzling, but it seems that those all got removed over
time, and now it's literally just MIPS with SGI (ip27/ip30/ip32,
not ip22) and Octeon. From the comment above octeon_should_swizzle_table[]
it appears that this is actually configurable in a register
but was set up at the time to do SGI-compatible address swizzling,
when it could have just use a software byteswap (CONFIG_SWAP_IO_SPACE)
like the rest of the world.

I double-checked the setting of CONFIG_SWAP_IO_SPACE, and found
that it's mostly consistent in setting this for all big-endian
platforms, the exceptions are:

- ath79 is big-endian but does not set SWAP_IO_SPACE. I assume
  it works because there is a very limited set of PCIe cards
  actually in use on this platform, notably ath9k wireless.
  If readl()/writel()/inl()/outl()  don't do a software byteswap, there
  is probably similar hardware logic in place as on octeon and SGI,
  and the lack of address swizzling may mean sub-word access to PCI
  registers is broken.
  The ath9k driver only uses ioread32()/iowrite32(), which works
  regardless of the address swizzling, but many other devices
  would be broken.

- eyeq and rb532 are always little-endian, so SWAP_IO_SPACE
  does nothing despite being selected.

- nintendo64 has no PCI, and I guess the readl()/writel()
  in drivers/block/n64cart.c, sound/mips/snd-n64.c etc
  end up using readl()/writel() as native on-chip register
  accessors instead of byteswapping ones.

       Arnd
diff mbox series

Patch

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 661f98c6c63a..368cd9ca6801 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4213,14 +4213,10 @@  EXPORT_SYMBOL_GPL(pci_pio_to_address);
unsigned long __weak pci_address_to_pio(phys_addr_t address)
{
-#ifdef PCI_IOBASE
- return logic_pio_trans_cpuaddr(address);
-#else
if (address > IO_SPACE_LIMIT)
return (unsigned long)-1;
return (unsigned long) address;
-#endif
}
/**
----------8<------------------

Additionally, the following message appears in dmesg on affected kernels: