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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c48f6f3c742eaf35e35882064ccf4ed07d4e63fe.camel@physik.fu-berlin.de>
Date:   Tue, 01 Aug 2023 21:21:45 +0200
From:   John Paul Adrian Glaubitz <glaubitz@...sik.fu-berlin.de>
To:     Arnd Bergmann <arnd@...db.de>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Baoquan He <bhe@...hat.com>
Cc:     linux-kernel@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
        Christoph Hellwig <hch@....de>,
        Christophe Leroy <christophe.leroy@...roup.eu>,
        Mike Rapoport <rppt@...nel.org>,
        Matthew Wilcox <willy@...radead.org>,
        Alexander Gordeev <agordeev@...ux.ibm.com>,
        Kefeng Wang <wangkefeng.wang@...wei.com>,
        Niklas Schnelle <schnelle@...ux.ibm.com>,
        Stafford Horne <shorne@...il.com>,
        David Laight <David.Laight@...lab.com>,
        Helge Deller <deller@....de>,
        Nathan Chancellor <nathan@...nel.org>,
        Yoshinori Sato <ysato@...rs.sourceforge.jp>,
        Rich Felker <dalias@...c.org>, linux-sh@...r.kernel.org,
        Guenter Roeck <linux@...ck-us.net>
Subject: Re: [PATCH v8 11/19] sh: add <asm-generic/io.h> including

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@...sik.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