| Message ID | 20250114-malta-io-fixes-v1-1-74ef1dc402ec@flygoat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | df1b8d6e89db0edd572a1e375f5d3dd5575b9a9b |
| Headers | show |
| Series | MIPS: pci-legacy: Override pci_address_to_pio | expand |
W dniu 14.01.2025 o 19:11, Jiaxun Yang pisze: > pci-legacy systems are not using logic_pio to managed PIO > allocations, thus the generic pci_address_to_pio won't work > when PCI_IOBASE is defined. > > Override the function to use architecture implementation to > fix the problem. > > Cc: stable@vger.kernel.org > Fixes: 4bfb53e7d317 ("mips: add <asm-generic/io.h> including") > Reported-by: Mateusz Jończyk <mat.jonczyk@o2.pl> > Closes: https://lore.kernel.org/r/99f75c66-4c2d-45dc-a808-b5ba440c7551@app.fastmail.com/ > Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com> > --- > This is a quick fix for fixes tree and stable backporting. > In long term, we should get logic_pio accept fixed mapping, > and make PCI core code aware of platforms not using vmap > for PCI_IOBASE. > --- > arch/mips/pci/pci-legacy.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/arch/mips/pci/pci-legacy.c b/arch/mips/pci/pci-legacy.c > index ec2567f8efd83bff7b106cbbd9ec7a6de0308c4c..66898fd182dc1fec1d1e9ae4c908873d59777182 100644 > --- a/arch/mips/pci/pci-legacy.c > +++ b/arch/mips/pci/pci-legacy.c > @@ -29,6 +29,14 @@ static LIST_HEAD(controllers); > > static int pci_initialized; > > +unsigned long pci_address_to_pio(phys_addr_t address) > +{ > + if (address > IO_SPACE_LIMIT) > + return (unsigned long)-1; > + > + return (unsigned long) address; > +} > + Hello, Thank you for this patch, I'm testing it right now. Shouldn't this be #ifndef-ed CONFIG_MACH_LOONGSON64 and perhaps CONFIG_RALINK? Loongson64 explicitly calls logic_pio_register_range(), so seems not to need this. Ralink also defined PCI_IOBASE a long time ago. Greetings, Mateusz
On Tue, Jan 14, 2025 at 06:11:58PM +0000, Jiaxun Yang wrote: > pci-legacy systems are not using logic_pio to managed PIO > allocations, thus the generic pci_address_to_pio won't work > when PCI_IOBASE is defined. > > Override the function to use architecture implementation to > fix the problem. > > Cc: stable@vger.kernel.org > Fixes: 4bfb53e7d317 ("mips: add <asm-generic/io.h> including") > Reported-by: Mateusz Jończyk <mat.jonczyk@o2.pl> > Closes: https://lore.kernel.org/r/99f75c66-4c2d-45dc-a808-b5ba440c7551@app.fastmail.com/ > Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com> > --- > This is a quick fix for fixes tree and stable backporting. > In long term, we should get logic_pio accept fixed mapping, > and make PCI core code aware of platforms not using vmap > for PCI_IOBASE. Why not do the real work now. Don't worry about stable kernels, fix it properly. thanks, greg k-h
在2025年1月14日一月 下午6:42，Mateusz Jończyk写道： [...] > > Hello, > > Thank you for this patch, I'm testing it right now. > > Shouldn't this be #ifndef-ed CONFIG_MACH_LOONGSON64 and perhaps > CONFIG_RALINK? Hi Mateusz, Those platforms are not using PCI_DRIVERS_LEGACY, so won't be affected by this patch. PCI_DRIVERS_GENERIC systems are handling logic_pio properly, so no need for this workaround. Thanks > > Loongson64 explicitly calls logic_pio_register_range(), so seems not to > need this. Ralink also > defined PCI_IOBASE a long time ago. > > Greetings, > > Mateusz
在2025年1月14日一月 下午6:46，Greg KH写道： > On Tue, Jan 14, 2025 at 06:11:58PM +0000, Jiaxun Yang wrote: >> pci-legacy systems are not using logic_pio to managed PIO >> allocations, thus the generic pci_address_to_pio won't work >> when PCI_IOBASE is defined. >> >> Override the function to use architecture implementation to >> fix the problem. >> >> Cc: stable@vger.kernel.org >> Fixes: 4bfb53e7d317 ("mips: add <asm-generic/io.h> including") >> Reported-by: Mateusz Jończyk <mat.jonczyk@o2.pl> >> Closes: https://lore.kernel.org/r/99f75c66-4c2d-45dc-a808-b5ba440c7551@app.fastmail.com/ >> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com> >> --- >> This is a quick fix for fixes tree and stable backporting. >> In long term, we should get logic_pio accept fixed mapping, >> and make PCI core code aware of platforms not using vmap >> for PCI_IOBASE. > > Why not do the real work now. Don't worry about stable kernels, fix it > properly. :-( Sadly the long-term solution is going to be a huge effort across architectures, and I'm not even sure if people will agree with my approach. So I'd prefer get this fix applied first and kick up discussions on a long-term solution. Will post RFC shortly. Thanks > > thanks, > > greg k-h
On Tue, Jan 14, 2025, at 19:11, Jiaxun Yang wrote: > > +unsigned long pci_address_to_pio(phys_addr_t address) > +{ > + if (address > IO_SPACE_LIMIT) > + return (unsigned long)-1; > + > + return (unsigned long) address; > +} > + > /* Isn't the argument to this function a CPU physical address? I don't think there is a point comparing it to IO_SPACE_LIMIT on architectures where I/O space is memory mapped. I see that you copied the above from the the non-PCI_IOBASE case of drivers/pci/pci.c, but that only really makes sense for architectures that have special port I/O instructions (x86, ia64) or that use logic_pio. Arnd
在2025年1月14日一月 下午7:03，Arnd Bergmann写道： > On Tue, Jan 14, 2025, at 19:11, Jiaxun Yang wrote: >> >> +unsigned long pci_address_to_pio(phys_addr_t address) >> +{ >> + if (address > IO_SPACE_LIMIT) >> + return (unsigned long)-1; >> + >> + return (unsigned long) address; >> +} >> + >> /* > > Isn't the argument to this function a CPU physical address? I > don't think there is a point comparing it to IO_SPACE_LIMIT > on architectures where I/O space is memory mapped. Actually not. It seems like the argument here is just raw PIO offset, without applying mips_io_port_base. We should validate it to ensure it's within the range specified by mips_io_port_base (which is sized by IO_SPACE_LIMIT). Thanks > > I see that you copied the above from the the non-PCI_IOBASE case > of drivers/pci/pci.c, but that only really makes sense for > architectures that have special port I/O instructions (x86, > ia64) or that use logic_pio. > > Arnd
On Tue, Jan 14, 2025, at 20:20, Jiaxun Yang wrote: > 在2025年1月14日一月 下午7:03，Arnd Bergmann写道： >> On Tue, Jan 14, 2025, at 19:11, Jiaxun Yang wrote: >>> >>> +unsigned long pci_address_to_pio(phys_addr_t address) >>> +{ >>> + if (address > IO_SPACE_LIMIT) >>> + return (unsigned long)-1; >>> + >>> + return (unsigned long) address; >>> +} >>> + >>> /* >> >> Isn't the argument to this function a CPU physical address? I >> don't think there is a point comparing it to IO_SPACE_LIMIT >> on architectures where I/O space is memory mapped. > > Actually not. It seems like the argument here is just raw PIO offset, > without applying mips_io_port_base. > > We should validate it to ensure it's within the range specified by > mips_io_port_base (which is sized by IO_SPACE_LIMIT). I don't know what you mean with "raw PIO offset", but I don't think that's how it's supopsed to be used. Here is how this gets called in of_pci_range_to_resource() if (res->flags & IORESOURCE_IO) { unsigned long port; err = pci_register_io_range(&np->fwnode, range->cpu_addr, range->size); if (err) goto invalid_range; port = pci_address_to_pio(range->cpu_addr); if (port == (unsigned long)-1) { err = -EINVAL; goto invalid_range; } start = port; } Where "range->cpu_addr" is the phys_addr_t location of the MMIO window that maps the host controllers port ranges, i.e. the fully translated address from the "ranges" property. The point of the pci_address_to_pio() function is to convert this into the Linux-internal virtual port number that gets used as an argument to inb()/outb() and reported in /proc/ioports and that may or may not be the same as the address on the bus itself, depending on the how the translation gets set up. On loongson, we seem to have two port ranges that are set up like isa@18000000 { compatible = "isa"; ranges = <1 0x0 0x0 0x18000000 0x4000>; }; pci@1a000000 { ranges = <0x01000000 0x0 0x00020000 0x0 0x18020000 0x0 0x00020000>, <0x02000000 0x0 0x40000000 0x0 0x40000000 0x0 0x40000000>; } Here, the cpu_addr is 0x18000000 for the isa bus and 0x18020000 for the PCI bus, apparently the intention being that these are consecutive in physical space, though Linux is free to rearrange the logical port numbers in a different way, e.g. to ensure that the PCI bus can use port numbers below 0x10000. On Malta, I see a very strange isa { compatible = "isa"; ranges = <1 0 0 0x1000>; }; which maps the first 4096 port numbers into cpu_addr=0x0. The actual port window appears to be at a board specific location #define MALTA_GT_PORT_BASE get_gt_port_base(GT_PCI0IOLD_OFS) #define MALTA_BONITO_PORT_BASE ((unsigned long)ioremap (0x1fd00000, 0x10000)) #define MALTA_MSC_PORT_BASE get_msc_port_base(MSC01_PCI_SC2PIOBASL) So e.g. on Bonito, the ranges property would have to be ranges = <1 0 0x1fd00000 0x1000>; Not sure if this is patched in by the bootloader, or where the corresponding window for PCI gets defined, but I suspect that the reason for the regression is that the caller of pci_address_to_pio() accidentally passed in '0' instead of the physical address, and it happened to work because of the missing PCI_IOBASE definition but broke after that got defined. Arnd
On Wed, 15 Jan 2025, Arnd Bergmann wrote: > On Malta, I see a very strange > > isa { > compatible = "isa"; > ranges = <1 0 0 0x1000>; > }; > > which maps the first 4096 port numbers into cpu_addr=0x0. The > actual port window appears to be at a board specific location > > #define MALTA_GT_PORT_BASE get_gt_port_base(GT_PCI0IOLD_OFS) > #define MALTA_BONITO_PORT_BASE ((unsigned long)ioremap (0x1fd00000, 0x10000)) > #define MALTA_MSC_PORT_BASE get_msc_port_base(MSC01_PCI_SC2PIOBASL) The system controller (PCI host bridge) is on the CPU core card along with the CPU and DRAM, so you get what you plug into the Malta baseboard, which is where the rest of the system resides connected via PCI and CBUS (which is a platform I/O bus wiring boot Flash, an auxiliary debug UART, also usable by Linux, and a bunch of simple peripherals needed for board setup and diagnostic output before PCI can be accessed, all on the Malta baseboard). > So e.g. on Bonito, the ranges property would have to be > > ranges = <1 0 0x1fd00000 0x1000>; > > Not sure if this is patched in by the bootloader, or where the > corresponding window for PCI gets defined, but I suspect that > the reason for the regression is that the caller of > pci_address_to_pio() accidentally passed in '0' instead of > the physical address, and it happened to work because of the > missing PCI_IOBASE definition but broke after that got defined. The windows are retrieved from hardware; cf. arch/mips/pci/pci-malta.c. Maciej
在2025年1月15日一月 上午7:38，Arnd Bergmann写道： [...] > > Where "range->cpu_addr" is the phys_addr_t location of the > MMIO window that maps the host controllers port ranges, i.e. > the fully translated address from the "ranges" property. This is valid for MIPS's PCI_DRIVERS_GENERIC, but not for PCI_DRIVERS_LEGACY, which have it's own implementation of various functions. See arch/mips/pci/pci-legacy.c, pci_load_of_ranges. The address translation is handled by io_map_base, which usually comes from mips_io_port_base. > > The point of the pci_address_to_pio() function is to convert > this into the Linux-internal virtual port number that gets > used as an argument to inb()/outb() and reported in > /proc/ioports and that may or may not be the same as the > address on the bus itself, depending on the how the translation > gets set up. > > On loongson, we seem to have two port ranges that are set up > like > > isa@18000000 { > compatible = "isa"; > ranges = <1 0x0 0x0 0x18000000 0x4000>; > }; > > pci@1a000000 { > ranges = <0x01000000 0x0 0x00020000 0x0 > 0x18020000 0x0 0x00020000>, > <0x02000000 0x0 0x40000000 0x0 > 0x40000000 0x0 0x40000000>; > } > > Here, the cpu_addr is 0x18000000 for the isa bus and > 0x18020000 for the PCI bus, apparently the intention being that > these are consecutive in physical space, though Linux is free > to rearrange the logical port numbers in a different way, e.g. > to ensure that the PCI bus can use port numbers below 0x10000. > > On Malta, I see a very strange > > isa { > compatible = "isa"; > ranges = <1 0 0 0x1000>; > }; > > which maps the first 4096 port numbers into cpu_addr=0x0. The > actual port window appears to be at a board specific location > > #define MALTA_GT_PORT_BASE get_gt_port_base(GT_PCI0IOLD_OFS) > #define MALTA_BONITO_PORT_BASE ((unsigned long)ioremap (0x1fd00000, 0x10000)) > #define MALTA_MSC_PORT_BASE get_msc_port_base(MSC01_PCI_SC2PIOBASL) > > So e.g. on Bonito, the ranges property would have to be > > ranges = <1 0 0x1fd00000 0x1000>; > > Not sure if this is patched in by the bootloader, or where the > corresponding window for PCI gets defined, but I suspect that > the reason for the regression is that the caller of > pci_address_to_pio() accidentally passed in '0' instead of > the physical address, and it happened to work because of the > missing PCI_IOBASE definition but broke after that got defined. Many other pci-legacy drivers are also working this way, PCI core is not aware of virtual port to physical MMIO address mapping. PCI core just see it as x86 style port. Thanks > > Arnd
W dniu 14.01.2025 o 19:42, Mateusz Jończyk pisze: > W dniu 14.01.2025 o 19:11, Jiaxun Yang pisze: >> pci-legacy systems are not using logic_pio to managed PIO >> allocations, thus the generic pci_address_to_pio won't work >> when PCI_IOBASE is defined. >> >> Override the function to use architecture implementation to >> fix the problem. >> >> Cc: stable@vger.kernel.org >> Fixes: 4bfb53e7d317 ("mips: add <asm-generic/io.h> including") >> Reported-by: Mateusz Jończyk <mat.jonczyk@o2.pl> >> Closes: >> https://lore.kernel.org/r/99f75c66-4c2d-45dc-a808-b5ba440c7551@app.fastmail.com/ >> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com> Hello, Tested on: - mips64el, QEMU malta - RTC is working, no suspicious warnings in dmesg, - mipsel, QEMU malta - RTC is working, no suspicious warnings in dmesg, - fuloong2e_defconfig, in QEMU on Ubuntu 24.04 - kernel does not boot, with or without this patch: [...] pps_core: LinuxPPS API ver. 1 registered pps_core: Software ver. 5.3.6 - Copyright 2005-2007 Rodolfo Giometti <giometti@linux.it> PTP clock support registered PCI host bridge to bus 0000:00 pci_bus 0000:00: root bus resource [mem 0x14000000-0x1c000000] pci_bus 0000:00: root bus resource [io 0x4000-0xffff] pci_bus 0000:00: No busn resource found for root bus, will use [bus 00-ff] pci 0000:00:00.0: [df53:00d5] type 00 class 0x060000 conventional PCI endpoint pci 0000:00:05.0: [1106:0686] type 00 class 0x060100 conventional PCI endpoint qemu-system-mips64el: hw/pci/pci.c:297: pci_bus_change_irq_level: Assertion `irq_num < bus->nirq' failed. - loongson3_defconfig, in QEMU (target loongson3-virt) - no important differences in dmesg output, but this platform does not use RTC CMOS, but a Goldfish RTC, Greetings, Mateusz
On Tue, Jan 14, 2025 at 06:11:58PM +0000, Jiaxun Yang wrote: > pci-legacy systems are not using logic_pio to managed PIO > allocations, thus the generic pci_address_to_pio won't work > when PCI_IOBASE is defined. > > Override the function to use architecture implementation to > fix the problem. > > Cc: stable@vger.kernel.org > Fixes: 4bfb53e7d317 ("mips: add <asm-generic/io.h> including") > Reported-by: Mateusz Jończyk <mat.jonczyk@o2.pl> > Closes: https://lore.kernel.org/r/99f75c66-4c2d-45dc-a808-b5ba440c7551@app.fastmail.com/ > Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com> > --- > This is a quick fix for fixes tree and stable backporting. > In long term, we should get logic_pio accept fixed mapping, > and make PCI core code aware of platforms not using vmap > for PCI_IOBASE. > --- > arch/mips/pci/pci-legacy.c | 8 ++++++++ > 1 file changed, 8 insertions(+) applied to mips-next. Thomas.
diff --git a/arch/mips/pci/pci-legacy.c b/arch/mips/pci/pci-legacy.c index ec2567f8efd83bff7b106cbbd9ec7a6de0308c4c..66898fd182dc1fec1d1e9ae4c908873d59777182 100644 --- a/arch/mips/pci/pci-legacy.c +++ b/arch/mips/pci/pci-legacy.c @@ -29,6 +29,14 @@ static LIST_HEAD(controllers); static int pci_initialized; +unsigned long pci_address_to_pio(phys_addr_t address) +{ + if (address > IO_SPACE_LIMIT) + return (unsigned long)-1; + + return (unsigned long) address; +} + /* * We need to avoid collisions with `mirrored' VGA ports * and other strange ISA hardware, so we always want the
pci-legacy systems are not using logic_pio to managed PIO allocations, thus the generic pci_address_to_pio won't work when PCI_IOBASE is defined. Override the function to use architecture implementation to fix the problem. Cc: stable@vger.kernel.org Fixes: 4bfb53e7d317 ("mips: add <asm-generic/io.h> including") Reported-by: Mateusz Jończyk <mat.jonczyk@o2.pl> Closes: https://lore.kernel.org/r/99f75c66-4c2d-45dc-a808-b5ba440c7551@app.fastmail.com/ Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com> --- This is a quick fix for fixes tree and stable backporting. In long term, we should get logic_pio accept fixed mapping, and make PCI core code aware of platforms not using vmap for PCI_IOBASE. --- arch/mips/pci/pci-legacy.c | 8 ++++++++ 1 file changed, 8 insertions(+) --- base-commit: dab2734f8e9ecba609d66d1dd087a392a7774c04 change-id: 20250114-malta-io-fixes-85e14b1b9f8b Best regards,