Fix port I/O string accessors such as `insb', `outsb', etc. which use
the physical PCI port I/O address rather than the corresponding memory
mapping to get at the requested location, which in turn breaks at least
accesses made by our parport driver to a PCIe parallel port such as:
PCI parallel port detected: 1415:c118, I/O at 0x1000(0x1008), IRQ 20
parport0: PC-style at 0x1000 (0x1008), irq 20, using FIFO [PCSPP,TRISTATE,COMPAT,EPP,ECP]
causing a memory access fault:
Unable to handle kernel access to user memory without uaccess routines at virtual address 0000000000001008
Oops [#1]
Modules linked in:
CPU: 1 PID: 350 Comm: cat Not tainted 6.0.0-rc2-00283-g10d4879f9ef0-dirty #23
Hardware name: SiFive HiFive Unmatched A00 (DT)
epc : parport_pc_fifo_write_block_pio+0x266/0x416
ra : parport_pc_fifo_write_block_pio+0xb4/0x416
epc : ffffffff80542c3e ra : ffffffff80542a8c sp : ffffffd88899fc60
gp : ffffffff80fa2700 tp : ffffffd882b1e900 t0 : ffffffd883d0b000
t1 : ffffffffff000002 t2 : 4646393043330a38 s0 : ffffffd88899fcf0
s1 : 0000000000001000 a0 : 0000000000000010 a1 : 0000000000000000
a2 : ffffffd883d0a010 a3 : 0000000000000023 a4 : 00000000ffff8fbb
a5 : ffffffd883d0a001 a6 : 0000000100000000 a7 : ffffffc800000000
s2 : ffffffffff000002 s3 : ffffffff80d28880 s4 : ffffffff80fa1f50
s5 : 0000000000001008 s6 : 0000000000000008 s7 : ffffffd883d0a000
s8 : 0004000000000000 s9 : ffffffff80dc1d80 s10: ffffffd8807e4000
s11: 0000000000000000 t3 : 00000000000000ff t4 : 393044410a303930
t5 : 0000000000001000 t6 : 0000000000040000
status: 0000000200000120 badaddr: 0000000000001008 cause: 000000000000000f
[<ffffffff80543212>] parport_pc_compat_write_block_pio+0xfe/0x200
[<ffffffff8053bbc0>] parport_write+0x46/0xf8
[<ffffffff8050530e>] lp_write+0x158/0x2d2
[<ffffffff80185716>] vfs_write+0x8e/0x2c2
[<ffffffff80185a74>] ksys_write+0x52/0xc2
[<ffffffff80185af2>] sys_write+0xe/0x16
[<ffffffff80003770>] ret_from_syscall+0x0/0x2
---[ end trace 0000000000000000 ]---
For simplicity address the problem by adding PCI_IOBASE to the physical
address requested in the respective wrapper macros only, observing that
the raw accessors such as `__insb', `__outsb', etc. are not supposed to
be used other than by said macros. Remove the cast to `long' that is no
longer needed on `addr' now that it is used as an offset from PCI_IOBASE
and add parentheses around `addr' needed for predictable evaluation in
macro expansion. No need to make said adjustments in separate changes
given that current code is gravely broken and does not ever work.
Signed-off-by: Maciej W. Rozycki <[email protected]>
Fixes: fab957c11efe2 ("RISC-V: Atomic and Locking Code")
Cc: [email protected] # v4.15+
---
arch/riscv/include/asm/io.h | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
linux-riscv-ins-outs-pci-iobase.diff
Index: linux-macro/arch/riscv/include/asm/io.h
===================================================================
--- linux-macro.orig/arch/riscv/include/asm/io.h
+++ linux-macro/arch/riscv/include/asm/io.h
@@ -101,9 +101,9 @@ __io_reads_ins(reads, u32, l, __io_br(),
__io_reads_ins(ins, u8, b, __io_pbr(), __io_par(addr))
__io_reads_ins(ins, u16, w, __io_pbr(), __io_par(addr))
__io_reads_ins(ins, u32, l, __io_pbr(), __io_par(addr))
-#define insb(addr, buffer, count) __insb((void __iomem *)(long)addr, buffer, count)
-#define insw(addr, buffer, count) __insw((void __iomem *)(long)addr, buffer, count)
-#define insl(addr, buffer, count) __insl((void __iomem *)(long)addr, buffer, count)
+#define insb(addr, buffer, count) __insb(PCI_IOBASE + (addr), buffer, count)
+#define insw(addr, buffer, count) __insw(PCI_IOBASE + (addr), buffer, count)
+#define insl(addr, buffer, count) __insl(PCI_IOBASE + (addr), buffer, count)
__io_writes_outs(writes, u8, b, __io_bw(), __io_aw())
__io_writes_outs(writes, u16, w, __io_bw(), __io_aw())
@@ -115,22 +115,22 @@ __io_writes_outs(writes, u32, l, __io_bw
__io_writes_outs(outs, u8, b, __io_pbw(), __io_paw())
__io_writes_outs(outs, u16, w, __io_pbw(), __io_paw())
__io_writes_outs(outs, u32, l, __io_pbw(), __io_paw())
-#define outsb(addr, buffer, count) __outsb((void __iomem *)(long)addr, buffer, count)
-#define outsw(addr, buffer, count) __outsw((void __iomem *)(long)addr, buffer, count)
-#define outsl(addr, buffer, count) __outsl((void __iomem *)(long)addr, buffer, count)
+#define outsb(addr, buffer, count) __outsb(PCI_IOBASE + (addr), buffer, count)
+#define outsw(addr, buffer, count) __outsw(PCI_IOBASE + (addr), buffer, count)
+#define outsl(addr, buffer, count) __outsl(PCI_IOBASE + (addr), buffer, count)
#ifdef CONFIG_64BIT
__io_reads_ins(reads, u64, q, __io_br(), __io_ar(addr))
#define readsq(addr, buffer, count) __readsq(addr, buffer, count)
__io_reads_ins(ins, u64, q, __io_pbr(), __io_par(addr))
-#define insq(addr, buffer, count) __insq((void __iomem *)addr, buffer, count)
+#define insq(addr, buffer, count) __insq(PCI_IOBASE + (addr), buffer, count)
__io_writes_outs(writes, u64, q, __io_bw(), __io_aw())
#define writesq(addr, buffer, count) __writesq(addr, buffer, count)
__io_writes_outs(outs, u64, q, __io_pbr(), __io_paw())
-#define outsq(addr, buffer, count) __outsq((void __iomem *)addr, buffer, count)
+#define outsq(addr, buffer, count) __outsq(PCI_IOBASE + (addr), buffer, count)
#endif
#include <asm-generic/io.h>
On Thu, Sep 22, 2022, at 11:56 PM, Maciej W. Rozycki wrote:
> Fix port I/O string accessors such as `insb', `outsb', etc. which use
> the physical PCI port I/O address rather than the corresponding memory
> mapping to get at the requested location, which in turn breaks at least
> accesses made by our parport driver to a PCIe parallel port such as:
>
> PCI parallel port detected: 1415:c118, I/O at 0x1000(0x1008), IRQ 20
> parport0: PC-style at 0x1000 (0x1008), irq 20, using FIFO
> [PCSPP,TRISTATE,COMPAT,EPP,ECP]
The patch looks correct to me, but I'm curious: Are you actually
using a parport device on your system, or just testing it to
make it work?
> +#define outsb(addr, buffer, count) __outsb(PCI_IOBASE + (addr),
> buffer, count)
> +#define outsw(addr, buffer, count) __outsw(PCI_IOBASE + (addr),
> buffer, count)
> +#define outsl(addr, buffer, count) __outsl(PCI_IOBASE + (addr),
> buffer, count)
>
I don't see anything actually risc-v specific in these definitions,
and it would be great to make the asm-generic version do the
right thing here. As far as I can tell, the only difference is
the barriers in the risc-v version, and we should really have the
same in the generic code anyway.
Arnd
On Fri, 23 Sep 2022, Arnd Bergmann wrote:
> > Fix port I/O string accessors such as `insb', `outsb', etc. which use
> > the physical PCI port I/O address rather than the corresponding memory
> > mapping to get at the requested location, which in turn breaks at least
> > accesses made by our parport driver to a PCIe parallel port such as:
> >
> > PCI parallel port detected: 1415:c118, I/O at 0x1000(0x1008), IRQ 20
> > parport0: PC-style at 0x1000 (0x1008), irq 20, using FIFO
> > [PCSPP,TRISTATE,COMPAT,EPP,ECP]
>
> The patch looks correct to me, but I'm curious: Are you actually
> using a parport device on your system, or just testing it to
> make it work?
I do, e.g. I have a MIPS Malta development board that uses an otherwise
standard Super I/O parallel port for firmware downloads and have had a
need recently to use it in the course of addressing a firmware bug. This
is how I actually triggered this bug; the Oops is from a download attempt.
For the time being I resorted to using the only other parallel port I
have in my lab, which is in my 25 years old x86 box, but I'd rather rely
on a fast modern machine that is subject to less risk to fail. And I
found my HiFive Unmatched board a perfect fit for this purpose.
I have a pair of Grammar Engine ROM emulators too that have both a serial
and a parallel port for the purpose of image downloads, either of which
can be used, except that the parallel interface is lightning fast compared
to the serial one (so possibly the best arrangement is to use the serial
port for control and the parallel one for data). I haven't used them for
a while now, but there's another firmware bug I plan to get at sometime,
with my Broadcom SWARM development board.
So yes, I have an ongoing interest in working parallel ports.
> > +#define outsb(addr, buffer, count) __outsb(PCI_IOBASE + (addr),
> > buffer, count)
> > +#define outsw(addr, buffer, count) __outsw(PCI_IOBASE + (addr),
> > buffer, count)
> > +#define outsl(addr, buffer, count) __outsl(PCI_IOBASE + (addr),
> > buffer, count)
> >
>
> I don't see anything actually risc-v specific in these definitions,
> and it would be great to make the asm-generic version do the
> right thing here. As far as I can tell, the only difference is
> the barriers in the risc-v version, and we should really have the
> same in the generic code anyway.
Fine with me as the next step, but we need to fix the bug at hand first
and get the fix backported with the RISC-V port.
Thanks for your review!
Maciej
"Maciej W. Rozycki" <[email protected]> writes:
> On Fri, 23 Sep 2022, Arnd Bergmann wrote:
>
[...]
> Thanks for your review!
>
Chasing stale threads! ;-)
Arnd, would you be OK adding your Reviewed-by tag to the patch?
Cheers,
Björn
On Mon, Oct 10, 2022, at 5:41 PM, Björn Töpel wrote:
> "Maciej W. Rozycki" <[email protected]> writes:
>
>> On Fri, 23 Sep 2022, Arnd Bergmann wrote:
>>
> [...]
>> Thanks for your review!
>>
>
> Chasing stale threads! ;-)
>
> Arnd, would you be OK adding your Reviewed-by tag to the patch?
Yes, please do.
Reviewed-by: Arnd Bergmann <[email protected]>