[v6,0/4] Regather scattered PCI-Code

Message

Philipp Stanner Jan. 31, 2024, 9 a.m. UTC

@Stable-Kernel:
You receive this patch series because its first patch fixes a leak in PCI.

@Bjorn:
I decided that it's now actually possible to just embed the docu updates
to the respective patches, instead of a separate patch.
Also dropped the ioport_unmap() for now.

Changes in v6:
- Remove the addition of ioport_unmap() from patch #1, since this is not
  really a bug, as explained by the comment above pci_iounmap. (Bjorn)
- Drop the patch unifying the two versions of pci_iounmap(). (Bjorn)
- Make patch #4's style congruent with PCI style.
- Drop (in any case empty) ioport_unmap() again from pci_iounmap()
- Add forgotten updates to Documentation/ when moving files from lib/ to
  drivers/pci/

Changes in v5:
- Add forgotten update to MAINTAINERS file.

Changes in v4:
- Apply Arnd's Reviewed-by's
- Add ifdef CONFIG_HAS_IOPORT_MAP guard in drivers/pci/iomap.c (build
  error on openrisc)
- Fix typo in patch no.5

Changes in v3:
- Create a separate patch for the leaks in lib/iomap.c. Make it the
  series' first patch. (Arnd)
- Turns out the aforementioned bug wasn't just accidentally removing
  iounmap() with the ifdef, it was also missing ioport_unmap() to begin
  with. Add it.
- Move the ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT-mechanism from
  asm-generic/io.h to asm-generic/ioport.h. (Arnd)
- Adjust the implementation of iomem_is_ioport() in asm-generic/io.h so
  that it matches exactly what pci_iounmap() previously did in
  lib/pci_iomap.c. (Arnd)
- Move the CONFIG_HAS_IOPORT guard in asm-generic/io.h so that
  iomem_is_ioport() will always be compiled and just returns false if
  there are no ports.
- Add TODOs to several places informing about the generic
  iomem_is_ioport() in lib/iomap.c not being generic.
- Add TODO about the followup work to make drivers/pci/iomap.c's
  pci_iounmap() actually generic.

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.


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.


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/pci_iomap.c: fix cleanup bug in pci_iounmap()
  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

 Documentation/driver-api/device-io.rst |   2 +-
 Documentation/driver-api/pci/pci.rst   |   6 +
 MAINTAINERS                            |   1 -
 drivers/pci/Kconfig                    |   5 +
 drivers/pci/Makefile                   |   3 +-
 drivers/pci/devres.c                   | 450 +++++++++++++++++++++++++
 lib/pci_iomap.c => drivers/pci/iomap.c |   5 +-
 drivers/pci/pci.c                      | 249 --------------
 drivers/pci/pci.h                      |  24 ++
 lib/Kconfig                            |   3 -
 lib/Makefile                           |   1 -
 lib/devres.c                           | 208 +-----------
 12 files changed, 490 insertions(+), 467 deletions(-)
 create mode 100644 drivers/pci/devres.c
 rename lib/pci_iomap.c => drivers/pci/iomap.c (99%)

Comments

Bjorn Helgaas Jan. 31, 2024, 9:08 p.m. UTC | #1

On Wed, Jan 31, 2024 at 10:00:19AM +0100, Philipp Stanner wrote:
> @Bjorn:
> I decided that it's now actually possible to just embed the docu updates
> to the respective patches, instead of a separate patch.
> Also dropped the ioport_unmap() for now.

Thanks.  I didn't see any documentation updates (other than those
related to the changed path names) in this series, so I assume the
updates you mention would be in a future series.

> ...
> Philipp Stanner (4):
>   lib/pci_iomap.c: fix cleanup bug in pci_iounmap()
>   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
> 
>  Documentation/driver-api/device-io.rst |   2 +-
>  Documentation/driver-api/pci/pci.rst   |   6 +
>  MAINTAINERS                            |   1 -
>  drivers/pci/Kconfig                    |   5 +
>  drivers/pci/Makefile                   |   3 +-
>  drivers/pci/devres.c                   | 450 +++++++++++++++++++++++++
>  lib/pci_iomap.c => drivers/pci/iomap.c |   5 +-
>  drivers/pci/pci.c                      | 249 --------------
>  drivers/pci/pci.h                      |  24 ++
>  lib/Kconfig                            |   3 -
>  lib/Makefile                           |   1 -
>  lib/devres.c                           | 208 +-----------
>  12 files changed, 490 insertions(+), 467 deletions(-)
>  create mode 100644 drivers/pci/devres.c
>  rename lib/pci_iomap.c => drivers/pci/iomap.c (99%)

Applied to pci/devres for v6.9, thanks!

Philipp Stanner Feb. 6, 2024, 9:41 a.m. UTC | #2

On Wed, 2024-01-31 at 15:08 -0600, Bjorn Helgaas wrote:
> On Wed, Jan 31, 2024 at 10:00:19AM +0100, Philipp Stanner wrote:
> > @Bjorn:
> > I decided that it's now actually possible to just embed the docu
> > updates
> > to the respective patches, instead of a separate patch.
> > Also dropped the ioport_unmap() for now.
> 
> Thanks.  I didn't see any documentation updates (other than those
> related to the changed path names) in this series, so I assume the
> updates you mention would be in a future series.

No, I actually meant the changed path names.

The next series (new devres functions) just adds more docstrings to
iomap.c, devres.c and pci.c in drivers/pci/, which, after this series
here is applied, are all already added to the Docu.

> 
> > ...
> > Philipp Stanner (4):
> >   lib/pci_iomap.c: fix cleanup bug in pci_iounmap()
> >   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
> > 
> >  Documentation/driver-api/device-io.rst |   2 +-
> >  Documentation/driver-api/pci/pci.rst   |   6 +
> >  MAINTAINERS                            |   1 -
> >  drivers/pci/Kconfig                    |   5 +
> >  drivers/pci/Makefile                   |   3 +-
> >  drivers/pci/devres.c                   | 450
> > +++++++++++++++++++++++++
> >  lib/pci_iomap.c => drivers/pci/iomap.c |   5 +-
> >  drivers/pci/pci.c                      | 249 --------------
> >  drivers/pci/pci.h                      |  24 ++
> >  lib/Kconfig                            |   3 -
> >  lib/Makefile                           |   1 -
> >  lib/devres.c                           | 208 +-----------
> >  12 files changed, 490 insertions(+), 467 deletions(-)
> >  create mode 100644 drivers/pci/devres.c
> >  rename lib/pci_iomap.c => drivers/pci/iomap.c (99%)
> 
> Applied to pci/devres for v6.9, thanks!

Thx!

Bjorn Helgaas Feb. 6, 2024, 3:34 p.m. UTC | #3

On Tue, Feb 06, 2024 at 10:41:13AM +0100, Philipp Stanner wrote:
> On Wed, 2024-01-31 at 15:08 -0600, Bjorn Helgaas wrote:
> > On Wed, Jan 31, 2024 at 10:00:19AM +0100, Philipp Stanner wrote:
> > > @Bjorn:
> > > I decided that it's now actually possible to just embed the docu
> > > updates
> > > to the respective patches, instead of a separate patch.
> > > Also dropped the ioport_unmap() for now.
> > 
> > Thanks.  I didn't see any documentation updates (other than those
> > related to the changed path names) in this series, so I assume the
> > updates you mention would be in a future series.
> 
> No, I actually meant the changed path names.
> 
> The next series (new devres functions) just adds more docstrings to
> iomap.c, devres.c and pci.c in drivers/pci/, which, after this series
> here is applied, are all already added to the Docu.

OK.  Other doc issues, I'm sure you've seen already:
https://lore.kernel.org/r/[e-mail]

I'll squash the fixes into this series when they're ready.

Bjorn