Re: [External] Re: [PATCH v4 1/3] riscv: io: avoid null-pointer arithmetic in PIO helpers

[yunhui cui]

Hi Arnd,

On Tue, May 5, 2026 at 2:34 PM Arnd Bergmann <arnd@xxxxxxxx> wrote:
>
> On Tue, May 5, 2026, at 08:20, Yunhui Cui wrote:
> > The RISC-V PIO helpers derive I/O addresses from PCI_IOBASE in ins*(),
> > outs*(), and ioport_map().
> >
> > Under configurations where I/O port support is not available, these
> > expressions can still be formed during compilation and trigger
> > -Wnull-pointer-arithmetic warnings from clang.
>
> If a driver attempts to use ISA port operations in a configuration
> without CONFIG_HAS_IOPORT, there is a NULL pointer warning because
> this is actually a NULL pointer access that will crash the
> kernel if the driver is ever loaded. You should not attempt
> to shut up the useful warning here but instead make sure every
> such code has a proper 'depends on HAS_IOPORT' dependency.

Thanks for the review.

I agree that NULL-pointer arithmetic warnings can point to real missing
HAS_IOPORT dependencies in drivers, and we should not hide those globally.

You are right about the generic ioport_map() change: the helper is already
guarded by CONFIG_HAS_IOPORT_MAP, so the extra CONFIG_HAS_IOPORT check is
redundant. I will drop that change.

For the RISC-V ins*/outs* helpers, they use PIO-specific fences
(__io_pbr()/__io_par() and __io_pbw()/__io_paw()), while the asm-generic
helpers would go through readsb()/writesb() and use normal MMIO string
ordering. So removing them would change ordering semantics.

I will keep the RISC-V helpers for now and only guard the port-I/O
variants with CONFIG_HAS_IOPORT.

>
> >
> > +#ifdef CONFIG_HAS_IOPORT
> >  __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(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)
> > +#define insb(addr, buffer, count) __insb(PCI_IO_ADDR(addr), buffer, count)
> > +#define insw(addr, buffer, count) __insw(PCI_IO_ADDR(addr), buffer, count)
> > +#define insl(addr, buffer, count) __insl(PCI_IO_ADDR(addr), buffer, count)
> > +#endif
>
> Can you try to remove the custom accessors altogether and
> use the ones from asm-generic instead? These should already
> have the correct behavior all the time, in particular they
> already come with the correct #ifdef check that is apparently
> missing in the riscv version.
>
> > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
> > index ca5a1ce6f0f89..d799e5ccc9437 100644
> > --- a/include/asm-generic/io.h
> > +++ b/include/asm-generic/io.h
> > @@ -1205,8 +1205,12 @@ static inline void __iomem
> > *ioremap_np(phys_addr_t offset, size_t size)
> >  #define ioport_map ioport_map
> >  static inline void __iomem *ioport_map(unsigned long port, unsigned
> > int nr)
> >  {
> > +#ifdef CONFIG_HAS_IOPORT
> >       port &= IO_SPACE_LIMIT;
> >       return (port > MMIO_UPPER_LIMIT) ? NULL : PCI_IOBASE + port;
> > +#else
> > +     return NULL;
> > +#endif
> >  }
>
> The function definition here is guarded by the #ifdef CONFIG_HAS_IOPORT_MAP
> check, which should already prevent it from being visible when
> CONFIG_HAS_IOPORT is disabled.
>
>      Arnd

Thanks,
Yunhui