Re: [PATCH 4/4] lib/iomap.c: improve comment about pci anomaly

[Philipp Stanner]

Hi,

On Fri, 2023-11-24 at 20:08 +0100, Danilo Krummrich wrote:
> Hi Arnd,
> 
> On 11/21/23 11:03, Arnd Bergmann wrote:
> > On Mon, Nov 20, 2023, at 22:59, Philipp Stanner wrote:
> > > lib/iomap.c contains one of the definitions of pci_iounmap(). The
> > > current comment above this out-of-place function does not clarify
> > > WHY
> > > the function is defined here.
> > > 
> > > Linus's detailed comment above pci_iounmap() in
> > > drivers/pci/iomap.c
> > > clarifies that in a far better way.
> > > 
> > > Extend the existing comment with an excerpt from Linus's and hint
> > > at the
> > > other implementation in drivers/pci/iomap.c
> > > 
> > > Signed-off-by: Philipp Stanner <pstanner@xxxxxxxxxx>
> > 
> > I think instead of explaining why the code is so complicated
> > here, I'd prefer to make it more logical and not have to
> > explain it.
> > 
> > We should be able to define a generic version like
> > 
> > void pci_iounmap(struct pci_dev *dev, void __iomem * addr)
> 
> Let's shed some light on the different config options related to
> this.
> 
> To me it looks like GENERIC_IOMAP always implies GENERIC_PCI_IOMAP.
> 
> lib/iomap.c contains a definition of pci_iounmap() since it uses the
> common IO_COND() macro. This definitions wins if GENERIC_IOMAP was
> selected.

Yes. So it seems the only way the implementation in lib/pci_iomap.c can
ever win is when someone selects GENERIC_PCI_IOMAP *without* selecting
GENERIC_IOMAP.


> 
> lib/pci_iomap.c contains another definition of pci_iounmap() which is
> guarded by ARCH_WANTS_GENERIC_PCI_IOUNMAP to prevent multiple
> definitions
> in case either GENERIC_IOMAP is set or the architecture already
> defined
> pci_iounmap().

To clarify that, here's the relevant excerpt from include/asm-
generic/io.h:

#ifndef CONFIG_GENERIC_IOMAP
#ifndef pci_iounmap
#define ARCH_WANTS_GENERIC_PCI_IOUNMAP
#endif
#endif


> 
> What's the purpose of not having set ARCH_HAS_GENERIC_IOPORT_MAP
> producing
> an empty definition of pci_iounmap() though [1]?
> 
> And more generally, is there any other (subtle) logic behind this?

That's indeed also very hard to understand for me, because you'd expect
that if pci_iomap() exists (and does something), pci_iounmap() should
also exist and, of course, unmapp the memory again.

>From include/asm-generic/io.h:

#ifdef CONFIG_HAS_IOPORT_MAP
#ifndef CONFIG_GENERIC_IOMAP
#ifndef ioport_map
#define ioport_map ioport_map
static inline void __iomem *ioport_map(unsigned long port, unsigned int nr)
{
 port &= IO_SPACE_LIMIT;
 return (port > MMIO_UPPER_LIMIT) ? NULL : PCI_IOBASE + port;
}
#define ARCH_HAS_GENERIC_IOPORT_MAP
#endif

As far as I understand the logic, an empty pci_iounmap() is generated
(and used?) in lib/pci_iounmap.c if:
 * CONFIG_HAS_IOPORT_MAP has not been defined
 * CONFIG_GENERIC_IOMAP has been defined (makes sense, then we use the
   one from lib/iomap.c anyways)
 * ioport_map has been defined by someone other than asm-generic/io.h

Regarding the last point, a number of architectures define their own
ioport_map():

arch/alpha/kernel/io.c, line 684 (as a function)
arch/arc/include/asm/io.h, line 27 (as a function)
arch/arm/mm/iomap.c, line 19 (as a function)
arch/m68k/include/asm/kmap.h, line 60 (as a function)
arch/parisc/lib/iomap.c, line 504 (as a function)
arch/powerpc/kernel/iomap.c, line 14 (as a function)
arch/s390/include/asm/io.h, line 38 (as a function)
arch/sh/kernel/ioport.c, line 24 (as a function)
arch/sparc/lib/iomap.c, line 10 (as a function)

I grepped through those archs and as I see it, none of those specify an
empty pci_iomap() that could be a counterpart to the potentially empty
pci_iounmap() in lib/pci_iomap.c

All of them use the generic pci_iomap.c, which can _never_ be empty.
Perhaps when the functions returning always NULL in include/asm-
generic/pci_iomap.h were to be used...?
But I think they should never be used, because then pci_iomap.c wins.
Arnds seems to agree with that, because he pointed out that these
functions are now surplus relicts in his reply to my Patch Nr.1:

> From what I can tell looking at the header, I think we can
> just remove the "#elif defined(CONFIG_GENERIC_PCI_IOMAP)"
> bit entirely, as it no longer serves the purpose it originally
> had.

So it seems that the empty unmap-function in pci_iomap.c is the left-
over counterpart of those mapping functions always returning NULL.

@Arnd:
Your code draft

void pci_iounmap(struct pci_dev *dev, void __iomem * addr)
{
#ifdef CONFIG_HAS_IOPORT
if (iomem_is_ioport(addr)) {
ioport_unmap(addr);
return;
}
#endif
iounmap(addr)
}

seems to agree with that: There will never be the need for an empty
function that does nothing. Correct?


P.

> 
> [1]
> https://elixir.bootlin.com/linux/latest/source/lib/pci_iomap.c#L167
> 
> > {
> > #ifdef CONFIG_HAS_IOPORT
> >         if (iomem_is_ioport(addr)) {
> >                ioport_unmap(addr);
> >                return;
> >         }
> > #endif
> >        iounmap(addr)
> > }
> > 
> > and then define iomem_is_ioport() in lib/iomap.c for x86,
> > while defining it in asm-generic/io.h for the rest,
> > with an override in asm/io.h for those architectures
> > that need a custom inb().
> 
> So, that would be similar to IO_COND(), right? What would we need
> inb() for
> in this context?
> 
> - Danilo
> 
> > 
> > Note that with ia64 gone, GENERIC_IOMAP is not at all
> > generic any more and could just move it to x86 or name
> > it something else. This is what currently uses it:
> > 
> > arch/hexagon/Kconfig:   select GENERIC_IOMAP
> > arch/um/Kconfig:        select GENERIC_IOMAP
> > 
> > These have no port I/O at all, so it doesn't do anything.
> > 
> > arch/m68k/Kconfig:      select GENERIC_IOMAP
> > 
> > on m68knommu, the default implementation from asm-generic/io.h
> > as the same effect as GENERIC_IOMAP but is more efficient.
> > On classic m68k, GENERIC_IOMAP does not do what it is
> > meant to because I/O ports on ISA devices have port
> > numbers above PIO_OFFSET. Also they don't have PCI.
> > 
> > arch/mips/Kconfig:      select GENERIC_IOMAP
> > 
> > This looks completely bogus because it sets PIO_RESERVED
> > to 0 and always uses the mmio part of lib/iomap.c.
> > 
> > arch/powerpc/platforms/Kconfig: select GENERIC_IOMAP
> > 
> > This is only used for two platforms: cell and powernv,
> > though on Cell it no longer does anything after the
> > commit f4981a00636 ("powerpc: Remove the celleb support");
> > I think the entire io_workarounds code now be folded
> > back into spider_pci.c if we wanted to.
> > 
> > The PowerNV LPC support does seem to still rely on it.
> > This tries to do the exact same thing as lib/logic_pio.c
> > for Huawei arm64 servers. I suspect that neither of them
> > does it entirely correctly since the powerpc side appears
> > to just override any non-LPC PIO support while the arm64
> > side is missing the ioread/iowrite support.
> > 
> >       Arnd
> > 
>