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@redhat.com>
---
 lib/iomap.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/lib/iomap.c b/lib/iomap.c
index 4f8b31baa575..647aac8ea3e3 100644
--- a/lib/iomap.c
+++ b/lib/iomap.c
@@ -419,8 +419,17 @@ EXPORT_SYMBOL(ioport_unmap);
 #endif /* CONFIG_HAS_IOPORT_MAP */
 
 #ifdef CONFIG_PCI
-/* Hide the details if this is a MMIO or PIO address space and just do what
- * you expect in the correct way. */
+/*
+ * Hide the details if this is a MMIO or PIO address space and just do what
+ * you expect in the correct way.
+ *
+ * pci_iounmap() somewhat illogically comes from lib/iomap.c for the
+ * CONFIG_GENERIC_IOMAP case, because that's the code that knows about
+ * the different IOMAP ranges.
+ *
+ * For more details see also the pci_iounmap() implementation in
+ * drivers/pci/iomap.c
+ */
 void pci_iounmap(struct pci_dev *dev, void __iomem * addr)
 {
 	IO_COND(addr, /* nothing */, iounmap(addr));
-- 
2.41.0

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@redhat.com>

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)
{
#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().

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

Hi again,

so I thought about this for a while and want to ask some follow up
questions in addition to those by Danilo in the other mail.

(btw, -CC Herbert Xu, since the mailserver is bouncing)


On Tue, 2023-11-21 at 11:03 +0100, 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@redhat.com>
> 
> 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)
> {
> #ifdef CONFIG_HAS_IOPORT
>        if (iomem_is_ioport(addr)) {
>               ioport_unmap(addr);
>               return;
>        }
> #endif
>       iounmap(addr)
> }

ACK, I think this makes sense – if we agree (as in the other thread)
that we never need an empty pci_iounmap().

> 
> and then define iomem_is_ioport() in lib/iomap.c for x86,

Wait a minute.
lib/ should never contain architecture-specific code, should it? I
assume your argument is that we write a default version of
iomem_is_ioport(), that could, in theory, be used by many
architectures, but ultimately only x86 will use that default.

> while defining it in asm-generic/io.h for the rest,

So we're not talking about the function prototypes here, but about the
actual implementation that should reside in asm-generic/io.h, aye?
Because the prototype obviously always has to be identical.

> with an override in asm/io.h for those architectures
> that need a custom inb().

So like this in ARCH/include/asm/io.h:

#define iomem_is_ioport iomem_is_ioport
bool iomem_is_ioport(...);

and in include/asm-generic/io.h:

#ifndef iomem_is_ioport
bool iomem_is_ioport(...);

correct?

Still, as Danilo has asked in his email, the question about how inb()
is related to it still stands

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

I think by now I get what the issue with GENERIC_IOMAP is. But do you
want me to do something about GENERIC_IOMAP (besides the things
directly related to the PCI functionality I'm touching) for you to
approve of a modified version of this patch series?


P.

> 
>      Arnd
> 

On Wed, Nov 29, 2023, at 13:40, Philipp Stanner wrote:
> On Tue, 2023-11-21 at 11:03 +0100, Arnd Bergmann wrote:
>> On Mon, Nov 20, 2023, at 22:59, Philipp Stanner wrote:
>> We should be able to define a generic version like
>> 
>> 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)
>> }
>
> ACK, I think this makes sense – if we agree (as in the other thread)
> that we never need an empty pci_iounmap().
>
>> 
>> and then define iomem_is_ioport() in lib/iomap.c for x86,
>
> Wait a minute.
> lib/ should never contain architecture-specific code, should it? I
> assume your argument is that we write a default version of
> iomem_is_ioport(), that could, in theory, be used by many
> architectures, but ultimately only x86 will use that default.

I would hope that eventually we can build lib/iomap.c
only on x86, as everything else can live without it.

>> while defining it in asm-generic/io.h for the rest,
>
> So we're not talking about the function prototypes here, but about the
> actual implementation that should reside in asm-generic/io.h, aye?
> Because the prototype obviously always has to be identical.

It could live in lib/pci_iomap.c or in
include/asm-generic/pci_iomap.h, it doesn't really matter
since the definition is trivial. asm-generic/io.h is probably
not the right place, unless we want to merge all of
asm-generic/pci_iomap.h into asm-generic/io.h. We could
do that now that all architectures include asm-generic/io.h
and that includes asm-generic/pci_iomap.h unconditionally.

>> with an override in asm/io.h for those architectures
>> that need a custom inb().
>
> So like this in ARCH/include/asm/io.h:
>
> #define iomem_is_ioport iomem_is_ioport
> bool iomem_is_ioport(...);
>
> and in include/asm-generic/io.h:
>
> #ifndef iomem_is_ioport
> bool iomem_is_ioport(...);
>
> correct?

Yes.
 
>> 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.
>
> I think by now I get what the issue with GENERIC_IOMAP is. But do you
> want me to do something about GENERIC_IOMAP (besides the things
> directly related to the PCI functionality I'm touching) for you to
> approve of a modified version of this patch series?

It would be nice to clean up some of the architectures
that incorrectly select it at the moment, but that
can be a separate series if you want to get this one
done first, or I can take a look myself.

      Arnd

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@redhat.com>
> 
> 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.

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().

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?

[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
>

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@redhat.com>
> > 
> > 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
> > 
> 

On Wed, Nov 29, 2023, at 11:16, Philipp Stanner wrote:
> On Fri, 2023-11-24 at 20:08 +0100, Danilo Krummrich wrote:
>>
>> 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

Right, this was added fairly recently in an effort to
unify the architectures that can share a simple implementation
based on the way that modern PCI host bridges on non-x86
work.

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

Right, I think that was a leak introduced in 316e8d79a095
("pci_iounmap'2: Electric Boogaloo: try to make sense of
it all") that should be fixed like

--- a/lib/pci_iomap.c
+++ b/lib/pci_iomap.c
@@ -170,8 +170,8 @@ void pci_iounmap(struct pci_dev *dev, void __iomem *p)
 
        if (addr >= start && addr < start + IO_SPACE_LIMIT)
                return;
-       iounmap(p);
 #endif
+       iounmap(p);
 }
 EXPORT_SYMBOL(pci_iounmap);

i.e. architectures without port I/O just call iounmap() but those
that support the normal ioport_map() have to skip iounmap()
for ports in that special PIO range.

> 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

I'm trying to unwind what you are saying here, and there are
two separate issues:

- an empty unmap() function still makes sense if the map() function
  just returns a usable pointer like the asm-generic version
  of ioport_map(), it only has to be non-empty if the map function
  allocates a resource that has to be freed later, like the
  page table entries for most ioremap() implementations.

- pci_iounmap() in lib/pci_iomap.c being empty is probably
  just a bug

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

no

> @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?

Agreed, while arch/sparc/ currently has an empty pci_iounmap(),
that is just because the normal iounmap() on that architecture
is also empty, given that all MMIO memory is always mapped.

>> > {
>> > #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?

In general, any architecture that has a custom inb() also
needs a custom ioport_map() and iomem_is_ioport() in this
scheme, while the "normal" architectures like arm/arm64 and
riscv should be able to just use the asm-generic version.

IO_COND() is really specific to those architectures that
rely on the rather misnamed GENERIC_IOMAP for implementing
ioport_map().

     Arnd

On Tue, 2023-11-21 at 11:03 +0100, 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@redhat.com>
> 
> 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)
> {
> #ifdef CONFIG_HAS_IOPORT
>        if (iomem_is_ioport(addr)) {
>               ioport_unmap(addr);
>               return;
>        }
> #endif
>       iounmap(addr)
> }

And where would you like such a function to reside?
drivers/pci/iomap.c?

P.

> 
> 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().
> 
> 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
> 

On Tue, Nov 21, 2023, at 15:38, Philipp Stanner wrote:
> On Tue, 2023-11-21 at 11:03 +0100, Arnd Bergmann wrote:
>> On Mon, Nov 20, 2023, at 22:59, Philipp Stanner wrote:
 
>> We should be able to define a generic version like
>> 
>> 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)
>> }
>
> And where would you like such a function to reside?
> drivers/pci/iomap.c?

Yes, I think that would be the logical place. It could also
be an inline function but that's not great on architectures
that don't also have iomem_is_ioport() inline.

    Arnd