Re: [PATCH v2 0/4] Regather scattered PCI-Code

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

On Fri, Dec 1, 2023, at 20:09, Philipp Stanner wrote:
> On Fri, 2023-12-01 at 17:27 +0100, Arnd Bergmann wrote:
>> On Fri, Dec 1, 2023, at 13:16, Philipp Stanner wrote:
>> 
>> The one special case that I missed the last time is s390, which
>> does not use GENERIC_PCI_IOMAP and will just require a separate
>> copy of pci_iounmap() to go along with the is custom pci_iomap().
>> 
>> > The two exceptions are x86 and m68k. The former uses lib/iomap.c
>> > through
>> > CONFIG_GENERIC_IOMAP, as Arnd pointed out in the previous
>> > discussion
>> > (thus, CONFIG_GENERIC_IOMAP is not really generic in this regard).
>> > 
>> > So as I see it, only m68k WOULD need its own custom definition of
>> > iomem_is_ioport(). But as I understand it it doesn't because it
>> > uses the
>> > one from asm-generic/pci_iomap.h ??
>> 
>> At the moment, m68k gets the pci_iounmap() from lib/iomap.c
>> if PCI is enabled for coldfire, but that incorrectly calls
>> iounmap() on PCI_IO_PA if it gets passed a PIO address.
>> 
>> The version from asm-generic/io.h should fix this.
>
> So, to be sure: m68k will use the generic iomem_is_ioport() despite
> defining its own inb()?

It depends, as m68k has two separate asm/io.h implementations:

- arch/m68k/include/asm/io_no.h uses the default inb()
  from asm-generic/io.h, so it should use the asm-generic
  version of iomem_is_ioport().

- arch/m68k/include/asm/io_mm.h is rather special when
  it comes to inb()/outb(), but since there is no PCI,
  I would just use the default iomem_is_ioport() because
  it doesn't matter as long as there are no callers.
  If we ever need a working iomem_is_ioport() here, it would
  need the same special cases as isa_itb().
  
>> The "#define pci_iomap" can be removed entirely I think.
>
> I also think it can, because first arch/asm/io.h includes asm-
> generic/io.h.
> I was just wondering why many other functions in asm-generic/io.h
> always define their own names..
>
> It's obviously very hard to test which config will break, so I thought
> it's better safe than sorry here

I'm fairly sure it's not actually needed, but since the entire file
does it, there is probably no harm keeping it consistent for the next
added function.

This is one more thing to maybe clean up eventually in the future,
possibly as part of moving the contents of asm-generic/io.h into
linux/io.h, which is something I'd like to do now that all
architectures finally started using the asm-generic version.

      Arnd