Sooooooooo. I reworked v1.

Please review this carefully, the IO-Ranges are obviously a bit tricky,
as is the build-system / ifdef-ery.

Arnd has suggested that architectures defining a custom inb() need their
own iomem_is_ioport(), as well. I've grepped for inb() and found the
following list of archs that define their own:
  - alpha
  - arm
  - m68k <--
  - parisc
  - powerpc
  - sh
  - sparc
  - x86 <--

All of those have their own definitons of pci_iounmap(). Therefore, they
don't need our generic version in the first place and, thus, also need
no iomem_is_ioport().
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 ??

I wasn't entirely sure how to deal with the address ranges for the
generic implementation in asm-generic/io.h. It's marked with a TODO.
Input appreciated.

I removed the guard around define pci_iounmap in asm-generic/io.h. An
alternative would be to have it be guarded by CONFIG_GENERIC_IOMAP and
CONFIG_GENERIC_PCI_IOMAP, both. Without such a guard, there is no
collision however, because generic pci_iounmap() from
drivers/pci/iomap.c will only get pulled in when
CONFIG_GENERIC_PCI_IOMAP is actually set.

I cross-built this for a variety of architectures, including the usual
suspects (s390, m68k). So far successfully. But let's see what Intel's
robots say :O

P.

Changes in v2:
- Replace patch 4, previously extending the comment about pci_iounmap()
  in lib/iomap.c, with a patch that moves pci_iounmap() from that file
  to drivers/pci/iomap.c, creating a unified version there. (Arnd)
- Implement iomem_is_ioport() as a new helper in asm-generic/io.h and
  lib/iomap.c. (Arnd)
- Move the build rule in drivers/pci/Makefile for iomap.o under the
  guard of #if PCI. This had to be done because when just checking for
  GENERIC_PCI_IOMAP being defined, the functions don't disappear, which
  was the case previously in lib/pci_iomap.c, where the entire file was
  made empty if PCI was not set by the guard #ifdef PCI. (Intel's Bots)
- Rephares all patches' commit messages a little bit.


Original cover letter:

Hi!

So it seems that since ca. 2007 the PCI code has been scattered a bit.
PCI's devres code, which is only ever used by users of the entire
PCI-subsystem anyways, resides in lib/devres.c and is guarded by an
ifdef PCI, just as the content of lib/pci_iomap.c is.

It, thus, seems reasonable to move all of that.

As I were at it, I moved as much of the devres-specific code from pci.c
to devres.c, too. The only exceptions are four functions that are
currently difficult to move. More information about that can be read
here [1].

I noticed these scattered files while working on (new) PCI-specific
devres functions. If we can get this here merged, I'll soon send another
patch series that addresses some API-inconsistencies and could move the
devres-part of the four remaining functions.

I don't want to do that in this series as this here is only about moving
code, whereas the next series would have to actually change API
behavior.

I successfully (cross-)built this for x86, x86_64, AARCH64 and ARM
(allyesconfig). I booted a kernel with it on x86_64, with a Fedora
desktop environment as payload. The OS came up fine

I hope this is OK. If we can get it in, we'd soon have a very
consistent PCI API again.

Regards,
P.


Philipp Stanner (4):
  lib: move pci_iomap.c to drivers/pci/
  lib: move pci-specific devres code to drivers/pci/
  pci: move devres code from pci.c to devres.c
  lib, pci: unify generic pci_iounmap()

 drivers/pci/Kconfig                    |   5 +
 drivers/pci/Makefile                   |   3 +-
 drivers/pci/devres.c                   | 450 +++++++++++++++++++++++++
 lib/pci_iomap.c => drivers/pci/iomap.c |  43 +--
 drivers/pci/pci.c                      | 249 --------------
 drivers/pci/pci.h                      |  24 ++
 include/asm-generic/io.h               |  37 +-
 lib/Kconfig                            |   3 -
 lib/Makefile                           |   1 -
 lib/devres.c                           | 208 +-----------
 lib/iomap.c                            |  16 +-
 11 files changed, 536 insertions(+), 503 deletions(-)
 create mode 100644 drivers/pci/devres.c
 rename lib/pci_iomap.c => drivers/pci/iomap.c (75%)

-- 
2.43.0

On Fri, Dec 1, 2023, at 13:16, Philipp Stanner wrote:
>
> Arnd has suggested that architectures defining a custom inb() need their
> own iomem_is_ioport(), as well. I've grepped for inb() and found the
> following list of archs that define their own:
>   - alpha
>   - arm
>   - m68k <--
>   - parisc
>   - powerpc
>   - sh
>   - sparc
>   - x86 <--
>
> All of those have their own definitons of pci_iounmap(). Therefore, they
> don't need our generic version in the first place and, thus, also need
> no iomem_is_ioport().

What I meant of course is that they should define iomem_is_ioport()
in order to drop the custom pci_iounmap() and have only one remaining
definition of that function left.

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.

For classic m68k, there is no PCI, so nothing calls pci_iounmap().

> I wasn't entirely sure how to deal with the address ranges for the
> generic implementation in asm-generic/io.h. It's marked with a TODO.
> Input appreciated.

I commented on the function directly. To clarify, I think we should
be able to directly turn each pci_iounmap() definition into
a iomem_is_ioport() definition by keeping the logic unchanged
and just return 'true' for the PIO variant or 'false' for the MMIO
version.

> I removed the guard around define pci_iounmap in asm-generic/io.h. An
> alternative would be to have it be guarded by CONFIG_GENERIC_IOMAP and
> CONFIG_GENERIC_PCI_IOMAP, both. Without such a guard, there is no
> collision however, because generic pci_iounmap() from
> drivers/pci/iomap.c will only get pulled in when
> CONFIG_GENERIC_PCI_IOMAP is actually set.

The "#define pci_iomap" can be removed entirely I think.

     Arnd

On Fri, 2023-12-01 at 17:27 +0100, Arnd Bergmann wrote:
> On Fri, Dec 1, 2023, at 13:16, Philipp Stanner wrote:
> > 
> > Arnd has suggested that architectures defining a custom inb() need
> > their
> > own iomem_is_ioport(), as well. I've grepped for inb() and found
> > the
> > following list of archs that define their own:
> >   - alpha
> >   - arm
> >   - m68k <--
> >   - parisc
> >   - powerpc
> >   - sh
> >   - sparc
> >   - x86 <--
> > 
> > All of those have their own definitons of pci_iounmap(). Therefore,
> > they
> > don't need our generic version in the first place and, thus, also
> > need
> > no iomem_is_ioport().
> 
> What I meant of course is that they should define iomem_is_ioport()
> in order to drop the custom pci_iounmap() and have only one remaining
> definition of that function left.

Ah, gotcha!
Yes, that would be neat. Would also allow for droping
ARCH_WANTS_GENERIC_PCI_IOUNMAP.

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

> 
> For classic m68k, there is no PCI, so nothing calls pci_iounmap().
> 
> > I wasn't entirely sure how to deal with the address ranges for the
> > generic implementation in asm-generic/io.h. It's marked with a
> > TODO.
> > Input appreciated.
> 
> I commented on the function directly. To clarify, I think we should
> be able to directly turn each pci_iounmap() definition into
> a iomem_is_ioport() definition by keeping the logic unchanged
> and just return 'true' for the PIO variant or 'false' for the MMIO
> version.
> 
> > I removed the guard around define pci_iounmap in asm-generic/io.h.
> > An
> > alternative would be to have it be guarded by CONFIG_GENERIC_IOMAP
> > and
> > CONFIG_GENERIC_PCI_IOMAP, both. Without such a guard, there is no
> > collision however, because generic pci_iounmap() from
> > drivers/pci/iomap.c will only get pulled in when
> > CONFIG_GENERIC_PCI_IOMAP is actually set.
> 
> 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

P.

> 
>      Arnd
> 

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