public inbox for linux-arch@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH v6 0/4] Regather scattered PCI-Code
@ 2024-01-31  9:00 Philipp Stanner
  2024-01-31  9:00 ` [PATCH v6 1/4] lib/pci_iomap.c: fix cleanup bug in pci_iounmap() Philipp Stanner
                   ` (4 more replies)
  0 siblings, 5 replies; 12+ messages in thread
From: Philipp Stanner @ 2024-01-31  9:00 UTC (permalink / raw)
  To: Bjorn Helgaas, Arnd Bergmann, Johannes Berg, Randy Dunlap,
	NeilBrown, John Sanpe, Kent Overstreet, Niklas Schnelle,
	Philipp Stanner, Dave Jiang, Uladzislau Koshchanka,
	Masami Hiramatsu (Google), David Gow, Kees Cook, Rae Moar,
	Geert Uytterhoeven, wuqiang.matt, Yury Norov, Jason Baron,
	Thomas Gleixner, Marco Elver, Andrew Morton, Ben Dooks, dakr
  Cc: linux-kernel, linux-pci, linux-arch, stable

@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%)

-- 
2.43.0


^ permalink raw reply	[flat|nested] 12+ messages in thread
end of thread, other threads:[~2024-02-06 15:34 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-01-31  9:00 [PATCH v6 0/4] Regather scattered PCI-Code Philipp Stanner
2024-01-31  9:00 ` [PATCH v6 1/4] lib/pci_iomap.c: fix cleanup bug in pci_iounmap() Philipp Stanner
2024-01-31 21:09   ` Bjorn Helgaas
2024-02-06  9:36     ` Philipp Stanner
2024-01-31  9:00 ` [PATCH v6 2/4] lib: move pci_iomap.c to drivers/pci/ Philipp Stanner
2024-01-31  9:00 ` [PATCH v6 3/4] lib: move pci-specific devres code " Philipp Stanner
2024-01-31  9:00 ` [PATCH v6 4/4] PCI: Move devres code from pci.c to devres.c Philipp Stanner
2024-01-31 21:12   ` Bjorn Helgaas
2024-02-06  9:37     ` Philipp Stanner
2024-01-31 21:08 ` [PATCH v6 0/4] Regather scattered PCI-Code Bjorn Helgaas
2024-02-06  9:41   ` Philipp Stanner
2024-02-06 15:34     ` Bjorn Helgaas

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox