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

["Arnd Bergmann" <arnd@arndb.de>]

On Mon, Jul 10, 2023, at 10:45, John Paul Adrian Glaubitz wrote:
> On Mon, 2023-07-10 at 10:17 +0200, Geert Uytterhoeven wrote:
>> Thanks for your patch, which is now commit edad4470b45298ba ("sh:
>> add <asm-generic/io.h> including") in next-20230710.
>> 
>> This break dreamcast_defconfig:
>> 
>>   CC      arch/sh/kernel/asm-offsets.s
>> In file included from ./arch/sh/include/asm/io.h:290,
>> ./include/asm-generic/io.h:636:15: error: redefinition of ‘inb_p’
>>   636 | #define inb_p inb_p
>>       |               ^~~~~

>> > index f7938fe0f911..5ba4116b4265 100644
>> > --- a/arch/sh/include/asm/io_noioport.h
>> > +++ b/arch/sh/include/asm/io_noioport.h
>> > @@ -53,6 +53,13 @@ static inline void ioport_unmap(void __iomem *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

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 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'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.

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.

      Arnd