Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <c48f6f3c742eaf35e35882064ccf4ed07d4e63fe.camel@physik.fu-berlin.de>
Subject: Re: [PATCH v8 11/19] sh: add <asm-generic/io.h> including
From:   John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
To:     Arnd Bergmann <arnd@arndb.de>,
        Geert Uytterhoeven <geert@linux-m68k.org>,
        Baoquan He <bhe@redhat.com>
Cc:     linux-kernel@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org,
        Christoph Hellwig <hch@lst.de>,
        Christophe Leroy <christophe.leroy@csgroup.eu>,
        Mike Rapoport <rppt@kernel.org>,
        Matthew Wilcox <willy@infradead.org>,
        Alexander Gordeev <agordeev@linux.ibm.com>,
        Kefeng Wang <wangkefeng.wang@huawei.com>,
        Niklas Schnelle <schnelle@linux.ibm.com>,
        Stafford Horne <shorne@gmail.com>,
        David Laight <David.Laight@aculab.com>,
        Helge Deller <deller@gmx.de>,
        Nathan Chancellor <nathan@kernel.org>,
        Yoshinori Sato <ysato@users.sourceforge.jp>,
        Rich Felker <dalias@libc.org>, linux-sh@vger.kernel.org,
        Guenter Roeck <linux@roeck-us.net>
Date:   Tue, 01 Aug 2023 21:21:45 +0200
In-Reply-To: <09094baf-dadf-4bce-9f63-f2a1f255f9a8@app.fastmail.com>
References: <20230706154520.11257-1-bhe@redhat.com>
         <20230706154520.11257-12-bhe@redhat.com>
         <CAMuHMdWmv-Jdvi7a04JGXuA2QARj8c8mpUvY7TOcetPkG4pW7A@mail.gmail.com>
         <0f771b11539d5a3871691af901315ee2034b577c.camel@physik.fu-berlin.de>
         <09094baf-dadf-4bce-9f63-f2a1f255f9a8@app.fastmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
User-Agent: Evolution 3.48.4 
MIME-Version: 1.0
Precedence: bulk

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