Re: [PATCH v8 11/19] sh: add <asm-generic/io.h> including

[John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>]

Hi Arnd!

On Mon, 2023-07-10 at 12:47 +0200, Arnd Bergmann wrote:
> It looks like only the "noioport" variant got some of the
> extra macro definitions, but the version for PCI still needs the
> same six macros, plus the ones of inb/outb etc, something like
> this:
> 
> diff --git a/arch/sh/include/asm/io.h b/arch/sh/include/asm/io.h
> index 24c560c065ec7..2135e32145c54 100644
> --- a/arch/sh/include/asm/io.h
> +++ b/arch/sh/include/asm/io.h
> @@ -241,6 +241,26 @@ __BUILD_IOPORT_STRING(q, u64)
>  
>  #endif
>  
> +#define inb(addr)      inb(addr)
> +#define inw(addr)      inw(addr)
> +#define inl(addr)      inl(addr)
> +#define outb(x, addr)  outb((x), (addr))
> +#define outw(x, addr)  outw((x), (addr))
> +#define outl(x, addr)  outl((x), (addr))
> +
> +#define inb_p(addr)    inb(addr)
> +#define inw_p(addr)    inw(addr)
> +#define inl_p(addr)    inl(addr)
> +#define outb_p(x, addr)        outb((x), (addr))
> +#define outw_p(x, addr)        outw((x), (addr))
> +#define outl_p(x, addr)        outl((x), (addr))
> +
> +#define insb insb
> +#define insw insw
> +#define insl insl
> +#define outsb outsb
> +#define outsw outsw
> +#define outsl outsl
>  
>  #define IO_SPACE_LIMIT 0xffffffff
>  
> diff --git a/arch/sh/include/asm/io_noioport.h b/arch/sh/include/asm/io_noioport.h
> index 5ba4116b4265c..12dad91f41c1e 100644
> --- a/arch/sh/include/asm/io_noioport.h
> +++ b/arch/sh/include/asm/io_noioport.h
> @@ -46,20 +46,6 @@ static inline void ioport_unmap(void __iomem *addr)
>         BUG();
>  }
>  
> -#define inb_p(addr)    inb(addr)
> -#define inw_p(addr)    inw(addr)
> -#define inl_p(addr)    inl(addr)
> -#define outb_p(x, addr)        outb((x), (addr))
> -#define outw_p(x, addr)        outw((x), (addr))
> -#define outl_p(x, addr)        outl((x), (addr))
> -
> -#define insb insb
> -#define insw insw
> -#define insl insl
> -#define outsb outsb
> -#define outsw outsw
> -#define outsl outsl
> -
>  static inline void insb(unsigned long port, void *dst, unsigned long count)
>  {
>         BUG();

I am fine with this fix.

Acked-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>

> I think ideally all the I/O port stuff in arch/sh/ could just be
> removed after the conversion to asm-generic/io.h, but the
> microdev_ioport_map() function oddity gets in the way of that,
> unless someone wants to clean up that platform. As far as I
> can tell, the ethernet, display, USB and PCI devices on it already
> broke at some point (afbb9d8d5266b, 46bc85872040a), so it might
> be easier to remove it entirely.

I don't have this particular hardware, so I cannot comment on this.

> > I'm not happy though that this patch is in linux-next without being Acked by me
> > or being reviewed by anyone. We should always make sure first that the code
> > actually builds and has been tested on real hardware.
> 
> I think that if the series has been posted eight times, you had
> your chance to do a review, especially since I pointed out that
> merging this one would have avoid the unxlate_dev_mem_ptr() bug
> as well.

I have only been the maintainer of arch/sh for a few weeks, so it's natural
that I am not doing a perfect job and might miss something. Also, I am not
getting paid for this work, I am doing this in my free time.

> Having the series go into linux-next sounds appropriate like this,
> the entire purpose of that is to find such bugs and Andrew can jus
> fold the fixup into the broken patch. 
> 
> Let me know if you prefer the simple version with the extra
> #defines or if we should just use the generic inb/outb implementation
> immediately and drop microdev in a separate patch.

Please go ahead with the simple version.

Thanks,
Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913