[v5,RESEND,0/5] Regather scattered PCI-Code

Message

Philipp Stanner Jan. 11, 2024, 8:55 a.m. UTC

Second Resend. Would be cool if someone could tell me what I'll have to
do so we can get this merged. This is blocking the followup work I've
got in the pipe

--

@Stable-Kernel:
You receive this patch series because its first patch fixes leaks in
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 (5):
  lib/pci_iomap.c: fix cleanup bugs 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
  lib, pci: unify generic pci_iounmap()

 MAINTAINERS                            |   1 -
 drivers/pci/Kconfig                    |   5 +
 drivers/pci/Makefile                   |   3 +-
 drivers/pci/devres.c                   | 450 +++++++++++++++++++++++++
 lib/pci_iomap.c => drivers/pci/iomap.c |  49 +--
 drivers/pci/pci.c                      | 249 --------------
 drivers/pci/pci.h                      |  24 ++
 include/asm-generic/io.h               |  27 +-
 include/asm-generic/iomap.h            |  21 ++
 lib/Kconfig                            |   3 -
 lib/Makefile                           |   1 -
 lib/devres.c                           | 208 +-----------
 lib/iomap.c                            |  28 +-
 13 files changed, 566 insertions(+), 503 deletions(-)
 create mode 100644 drivers/pci/devres.c
 rename lib/pci_iomap.c => drivers/pci/iomap.c (75%)

Comments

Johannes Berg Jan. 11, 2024, 9:03 a.m. UTC | #1

On Thu, 2024-01-11 at 09:55 +0100, Philipp Stanner wrote:
> Second Resend. Would be cool if someone could tell me what I'll have to
> do so we can get this merged.

I don't even know who'd merge it, but um doesn't seem appropriate...
> 
> @Stable-Kernel:
> You receive this patch series because its first patch fixes leaks in
> PCI.

Too early for that, stable just ignores stuff before it hits mainline.

johannes

Philipp Stanner Jan. 11, 2024, 9:14 a.m. UTC | #2

On Thu, 2024-01-11 at 10:03 +0100, Johannes Berg wrote:
> On Thu, 2024-01-11 at 09:55 +0100, Philipp Stanner wrote:
> > Second Resend. Would be cool if someone could tell me what I'll
> > have to
> > do so we can get this merged.
> 
> I don't even know who'd merge it, but um doesn't seem appropriate...

UM isn't a recipent, I'd guess it's some mail filter which might make
it appear that way :)
The lists I sent this to are
[e-mail], [e-mail],
[e-mail], [e-mail]

Anyways, PCI is for sure who should merge this, since it's 100% about
PCI.

> > 
> > @Stable-Kernel:
> > You receive this patch series because its first patch fixes leaks
> > in
> > PCI.
> 
> Too early for that, stable just ignores stuff before it hits
> mainline.

I know, they're in CC because of the "Fixes: "

P.

> 
> johannes
>

Johannes Berg Jan. 11, 2024, 9:22 a.m. UTC | #3

On Thu, 2024-01-11 at 10:14 +0100, Philipp Stanner wrote:
> On Thu, 2024-01-11 at 10:03 +0100, Johannes Berg wrote:
> > On Thu, 2024-01-11 at 09:55 +0100, Philipp Stanner wrote:
> > > Second Resend. Would be cool if someone could tell me what I'll
> > > have to
> > > do so we can get this merged.
> > 
> > I don't even know who'd merge it, but um doesn't seem appropriate...
> 
> UM isn't a recipent, I'd guess it's some mail filter which might make
> it appear that way :)

Oh. I guess I thought I was CC'ed as UM maintainer :)

> The lists I sent this to are
> [e-mail], [e-mail],
> [e-mail], [e-mail]
> 
> Anyways, PCI is for sure who should merge this, since it's 100% about
> PCI.

Sounds good :)

johannes

Bjorn Helgaas Jan. 11, 2024, 2:53 p.m. UTC | #4

On Thu, Jan 11, 2024 at 09:55:35AM +0100, Philipp Stanner wrote:
> Second Resend. Would be cool if someone could tell me what I'll have to
> do so we can get this merged. This is blocking the followup work I've
> got in the pipe

This seems PCI-focused, and I'll look at merging this after v6.8-rc1
is tagged and the merge window closes (probably Jan 21).  Then I'll
rebase it to v6.8-rc1, tidy the subject lines to look like the rest
of drivers/pci/, etc.

> Philipp Stanner (5):
>   lib/pci_iomap.c: fix cleanup bugs 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
>   lib, pci: unify generic pci_iounmap()
> 
>  MAINTAINERS                            |   1 -
>  drivers/pci/Kconfig                    |   5 +
>  drivers/pci/Makefile                   |   3 +-
>  drivers/pci/devres.c                   | 450 +++++++++++++++++++++++++
>  lib/pci_iomap.c => drivers/pci/iomap.c |  49 +--
>  drivers/pci/pci.c                      | 249 --------------
>  drivers/pci/pci.h                      |  24 ++
>  include/asm-generic/io.h               |  27 +-
>  include/asm-generic/iomap.h            |  21 ++
>  lib/Kconfig                            |   3 -
>  lib/Makefile                           |   1 -
>  lib/devres.c                           | 208 +-----------
>  lib/iomap.c                            |  28 +-
>  13 files changed, 566 insertions(+), 503 deletions(-)
>  create mode 100644 drivers/pci/devres.c
>  rename lib/pci_iomap.c => drivers/pci/iomap.c (75%)

Philipp Stanner Jan. 11, 2024, 3:32 p.m. UTC | #5

On Thu, 2024-01-11 at 08:53 -0600, Bjorn Helgaas wrote:
> On Thu, Jan 11, 2024 at 09:55:35AM +0100, Philipp Stanner wrote:
> > Second Resend. Would be cool if someone could tell me what I'll
> > have to
> > do so we can get this merged. This is blocking the followup work
> > I've
> > got in the pipe
> 
> This seems PCI-focused, and I'll look at merging this after v6.8-rc1
> is tagged and the merge window closes (probably Jan 21).  Then I'll
> rebase it to v6.8-rc1, tidy the subject lines to look like the rest
> of drivers/pci/, etc.

Cool!

Just ping you if you need me to do something

Regards,
P.

> 
> > Philipp Stanner (5):
> >   lib/pci_iomap.c: fix cleanup bugs 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
> >   lib, pci: unify generic pci_iounmap()
> > 
> >  MAINTAINERS                            |   1 -
> >  drivers/pci/Kconfig                    |   5 +
> >  drivers/pci/Makefile                   |   3 +-
> >  drivers/pci/devres.c                   | 450
> > +++++++++++++++++++++++++
> >  lib/pci_iomap.c => drivers/pci/iomap.c |  49 +--
> >  drivers/pci/pci.c                      | 249 --------------
> >  drivers/pci/pci.h                      |  24 ++
> >  include/asm-generic/io.h               |  27 +-
> >  include/asm-generic/iomap.h            |  21 ++
> >  lib/Kconfig                            |   3 -
> >  lib/Makefile                           |   1 -
> >  lib/devres.c                           | 208 +-----------
> >  lib/iomap.c                            |  28 +-
> >  13 files changed, 566 insertions(+), 503 deletions(-)
> >  create mode 100644 drivers/pci/devres.c
> >  rename lib/pci_iomap.c => drivers/pci/iomap.c (75%)
>