RISC-V: Make port I/O string accessors actually work

Commit Message

Maciej W. Rozycki Sept. 22, 2022, 9:56 p.m. UTC

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 <[e-mail]>
Fixes: fab957c11efe2 ("RISC-V: Atomic and Locking Code")
Cc: [e-mail] # v4.15+
---
 arch/riscv/include/asm/io.h |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

linux-riscv-ins-outs-pci-iobase.diff

Comments

Arnd Bergmann Sept. 23, 2022, 10:31 a.m. UTC | #1

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

Maciej W. Rozycki Sept. 23, 2022, 11:38 a.m. UTC | #2

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

Björn Töpel Oct. 10, 2022, 3:41 p.m. UTC | #3

"Maciej W. Rozycki" <[e-mail]> 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

Arnd Bergmann Oct. 10, 2022, 5:53 p.m. UTC | #4

On Mon, Oct 10, 2022, at 5:41 PM, Björn Töpel wrote:
> "Maciej W. Rozycki" <[e-mail]> 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 <[e-mail]>

Patch

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>