2024-01-11 08:56:25

by Philipp Stanner

Subject: [PATCH v5 RESEND 0/5] Regather scattered PCI-Code

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

--
2.43.0

2024-01-11 08:56:47

by Philipp Stanner

Subject: [PATCH v5 RESEND 1/5] lib/pci_iomap.c: fix cleanup bugs in pci_iounmap()

pci_iounmap() in lib/pci_iomap.c is supposed to check whether an address
is within ioport-range IF the config specifies that ioports exist. If
so, the port should be unmapped with ioport_unmap(). If not, it's a
generic MMIO address that has to be passed to iounmap().

The bugs are:
1. ioport_unmap() is missing entirely, so this function will never
actually unmap a port.
2. the #ifdef for the ioport-ranges accidentally also guards
iounmap(), potentially compiling an empty function. This would
cause the mapping to be leaked.

Implement the missing call to ioport_unmap().

Move the guard so that iounmap() will always be part of the function.

CC: <[email protected]> # v5.15+
Fixes: 316e8d79a095 ("pci_iounmap'2: Electric Boogaloo: try to make sense of it all")
Reported-by: Danilo Krummrich <[email protected]>
Suggested-by: Arnd Bergmann <[email protected]>
Signed-off-by: Philipp Stanner <[email protected]>
Reviewed-by: Arnd Bergmann <[email protected]>
---
lib/pci_iomap.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/pci_iomap.c b/lib/pci_iomap.c
index ce39ce9f3526..6e144b017c48 100644
--- a/lib/pci_iomap.c
+++ b/lib/pci_iomap.c
@@ -168,10 +168,12 @@ void pci_iounmap(struct pci_dev *dev, void __iomem *p)
uintptr_t start = (uintptr_t) PCI_IOBASE;
uintptr_t addr = (uintptr_t) p;

- if (addr >= start && addr < start + IO_SPACE_LIMIT)
+ if (addr >= start && addr < start + IO_SPACE_LIMIT) {
+ ioport_unmap(p);
return;
- iounmap(p);
+ }
#endif
+ iounmap(p);
}
EXPORT_SYMBOL(pci_iounmap);

--
2.43.0

2024-01-11 08:57:40

by Philipp Stanner

Subject: [PATCH v5 RESEND 3/5] lib: move pci-specific devres code to drivers/pci/

The pcim_*() functions in lib/devres.c are guarded by an #ifdef
CONFIG_PCI and, thus, don't belong to this file. They are only ever used
for pci and are not generic infrastructure.

Move all pcim_*() functions in lib/devres.c to drivers/pci/devres.c.
Adjust the Makefile.

Suggested-by: Danilo Krummrich <[email protected]>
Signed-off-by: Philipp Stanner <[email protected]>
---
drivers/pci/Makefile | 2 +-
drivers/pci/devres.c | 207 ++++++++++++++++++++++++++++++++++++++++++
lib/devres.c | 208 +------------------------------------------
3 files changed, 209 insertions(+), 208 deletions(-)
create mode 100644 drivers/pci/devres.c

diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index 64dcedccfc87..ed65299b42b5 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -5,7 +5,7 @@
obj-$(CONFIG_PCI) += access.o bus.o probe.o host-bridge.o \
remove.o pci.o pci-driver.o search.o \
pci-sysfs.o rom.o setup-res.o irq.o vpd.o \
- setup-bus.o vc.o mmap.o setup-irq.o
+ setup-bus.o vc.o mmap.o setup-irq.o devres.o

obj-$(CONFIG_PCI) += msi/
obj-$(CONFIG_PCI) += pcie/
diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
new file mode 100644
index 000000000000..a3fd0d65cef1
--- /dev/null
+++ b/drivers/pci/devres.c
@@ -0,0 +1,207 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/pci.h>
+#include "pci.h"
+
+/*
+ * PCI iomap devres
+ */
+#define PCIM_IOMAP_MAX PCI_STD_NUM_BARS
+
+struct pcim_iomap_devres {
+ void __iomem *table[PCIM_IOMAP_MAX];
+};
+
+static void pcim_iomap_release(struct device *gendev, void *res)
+{
+ struct pci_dev *dev = to_pci_dev(gendev);
+ struct pcim_iomap_devres *this = res;
+ int i;
+
+ for (i = 0; i < PCIM_IOMAP_MAX; i++)
+ if (this->table[i])
+ pci_iounmap(dev, this->table[i]);
+}
+
+/**
+ * pcim_iomap_table - access iomap allocation table
+ * @pdev: PCI device to access iomap table for
+ *
+ * Access iomap allocation table for @dev. If iomap table doesn't
+ * exist and @pdev is managed, it will be allocated. All iomaps
+ * recorded in the iomap table are automatically unmapped on driver
+ * detach.
+ *
+ * This function might sleep when the table is first allocated but can
+ * be safely called without context and guaranteed to succeed once
+ * allocated.
+ */
+void __iomem * const *pcim_iomap_table(struct pci_dev *pdev)
+{
+ struct pcim_iomap_devres *dr, *new_dr;
+
+ dr = devres_find(&pdev->dev, pcim_iomap_release, NULL, NULL);
+ if (dr)
+ return dr->table;
+
+ new_dr = devres_alloc_node(pcim_iomap_release, sizeof(*new_dr), GFP_KERNEL,
+ dev_to_node(&pdev->dev));
+ if (!new_dr)
+ return NULL;
+ dr = devres_get(&pdev->dev, new_dr, NULL, NULL);
+ return dr->table;
+}
+EXPORT_SYMBOL(pcim_iomap_table);
+
+/**
+ * pcim_iomap - Managed pcim_iomap()
+ * @pdev: PCI device to iomap for
+ * @bar: BAR to iomap
+ * @maxlen: Maximum length of iomap
+ *
+ * Managed pci_iomap(). Map is automatically unmapped on driver
+ * detach.
+ */
+void __iomem *pcim_iomap(struct pci_dev *pdev, int bar, unsigned long maxlen)
+{
+ void __iomem **tbl;
+
+ BUG_ON(bar >= PCIM_IOMAP_MAX);
+
+ tbl = (void __iomem **)pcim_iomap_table(pdev);
+ if (!tbl || tbl[bar]) /* duplicate mappings not allowed */
+ return NULL;
+
+ tbl[bar] = pci_iomap(pdev, bar, maxlen);
+ return tbl[bar];
+}
+EXPORT_SYMBOL(pcim_iomap);
+
+/**
+ * pcim_iounmap - Managed pci_iounmap()
+ * @pdev: PCI device to iounmap for
+ * @addr: Address to unmap
+ *
+ * Managed pci_iounmap(). @addr must have been mapped using pcim_iomap().
+ */
+void pcim_iounmap(struct pci_dev *pdev, void __iomem *addr)
+{
+ void __iomem **tbl;
+ int i;
+
+ pci_iounmap(pdev, addr);
+
+ tbl = (void __iomem **)pcim_iomap_table(pdev);
+ BUG_ON(!tbl);
+
+ for (i = 0; i < PCIM_IOMAP_MAX; i++)
+ if (tbl[i] == addr) {
+ tbl[i] = NULL;
+ return;
+ }
+ WARN_ON(1);
+}
+EXPORT_SYMBOL(pcim_iounmap);
+
+/**
+ * pcim_iomap_regions - Request and iomap PCI BARs
+ * @pdev: PCI device to map IO resources for
+ * @mask: Mask of BARs to request and iomap
+ * @name: Name used when requesting regions
+ *
+ * Request and iomap regions specified by @mask.
+ */
+int pcim_iomap_regions(struct pci_dev *pdev, int mask, const char *name)
+{
+ void __iomem * const *iomap;
+ int i, rc;
+
+ iomap = pcim_iomap_table(pdev);
+ if (!iomap)
+ return -ENOMEM;
+
+ for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
+ unsigned long len;
+
+ if (!(mask & (1 << i)))
+ continue;
+
+ rc = -EINVAL;
+ len = pci_resource_len(pdev, i);
+ if (!len)
+ goto err_inval;
+
+ rc = pci_request_region(pdev, i, name);
+ if (rc)
+ goto err_inval;
+
+ rc = -ENOMEM;
+ if (!pcim_iomap(pdev, i, 0))
+ goto err_region;
+ }
+
+ return 0;
+
+ err_region:
+ pci_release_region(pdev, i);
+ err_inval:
+ while (--i >= 0) {
+ if (!(mask & (1 << i)))
+ continue;
+ pcim_iounmap(pdev, iomap[i]);
+ pci_release_region(pdev, i);
+ }
+
+ return rc;
+}
+EXPORT_SYMBOL(pcim_iomap_regions);
+
+/**
+ * pcim_iomap_regions_request_all - Request all BARs and iomap specified ones
+ * @pdev: PCI device to map IO resources for
+ * @mask: Mask of BARs to iomap
+ * @name: Name used when requesting regions
+ *
+ * Request all PCI BARs and iomap regions specified by @mask.
+ */
+int pcim_iomap_regions_request_all(struct pci_dev *pdev, int mask,
+ const char *name)
+{
+ int request_mask = ((1 << 6) - 1) & ~mask;
+ int rc;
+
+ rc = pci_request_selected_regions(pdev, request_mask, name);
+ if (rc)
+ return rc;
+
+ rc = pcim_iomap_regions(pdev, mask, name);
+ if (rc)
+ pci_release_selected_regions(pdev, request_mask);
+ return rc;
+}
+EXPORT_SYMBOL(pcim_iomap_regions_request_all);
+
+/**
+ * pcim_iounmap_regions - Unmap and release PCI BARs
+ * @pdev: PCI device to map IO resources for
+ * @mask: Mask of BARs to unmap and release
+ *
+ * Unmap and release regions specified by @mask.
+ */
+void pcim_iounmap_regions(struct pci_dev *pdev, int mask)
+{
+ void __iomem * const *iomap;
+ int i;
+
+ iomap = pcim_iomap_table(pdev);
+ if (!iomap)
+ return;
+
+ for (i = 0; i < PCIM_IOMAP_MAX; i++) {
+ if (!(mask & (1 << i)))
+ continue;
+
+ pcim_iounmap(pdev, iomap[i]);
+ pci_release_region(pdev, i);
+ }
+}
+EXPORT_SYMBOL(pcim_iounmap_regions);
diff --git a/lib/devres.c b/lib/devres.c
index c44f104b58d5..fe0c63caeb68 100644
--- a/lib/devres.c
+++ b/lib/devres.c
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
+#include <linux/device.h>
#include <linux/err.h>
-#include <linux/pci.h>
#include <linux/io.h>
#include <linux/gfp.h>
#include <linux/export.h>
@@ -311,212 +311,6 @@ void devm_ioport_unmap(struct device *dev, void __iomem *addr)
EXPORT_SYMBOL(devm_ioport_unmap);
#endif /* CONFIG_HAS_IOPORT_MAP */

-#ifdef CONFIG_PCI
-/*
- * PCI iomap devres
- */
-#define PCIM_IOMAP_MAX PCI_STD_NUM_BARS
-
-struct pcim_iomap_devres {
- void __iomem *table[PCIM_IOMAP_MAX];
-};
-
-static void pcim_iomap_release(struct device *gendev, void *res)
-{
- struct pci_dev *dev = to_pci_dev(gendev);
- struct pcim_iomap_devres *this = res;
- int i;
-
- for (i = 0; i < PCIM_IOMAP_MAX; i++)
- if (this->table[i])
- pci_iounmap(dev, this->table[i]);
-}
-
-/**
- * pcim_iomap_table - access iomap allocation table
- * @pdev: PCI device to access iomap table for
- *
- * Access iomap allocation table for @dev. If iomap table doesn't
- * exist and @pdev is managed, it will be allocated. All iomaps
- * recorded in the iomap table are automatically unmapped on driver
- * detach.
- *
- * This function might sleep when the table is first allocated but can
- * be safely called without context and guaranteed to succeed once
- * allocated.
- */
-void __iomem * const *pcim_iomap_table(struct pci_dev *pdev)
-{
- struct pcim_iomap_devres *dr, *new_dr;
-
- dr = devres_find(&pdev->dev, pcim_iomap_release, NULL, NULL);
- if (dr)
- return dr->table;
-
- new_dr = devres_alloc_node(pcim_iomap_release, sizeof(*new_dr), GFP_KERNEL,
- dev_to_node(&pdev->dev));
- if (!new_dr)
- return NULL;
- dr = devres_get(&pdev->dev, new_dr, NULL, NULL);
- return dr->table;
-}
-EXPORT_SYMBOL(pcim_iomap_table);
-
-/**
- * pcim_iomap - Managed pcim_iomap()
- * @pdev: PCI device to iomap for
- * @bar: BAR to iomap
- * @maxlen: Maximum length of iomap
- *
- * Managed pci_iomap(). Map is automatically unmapped on driver
- * detach.
- */
-void __iomem *pcim_iomap(struct pci_dev *pdev, int bar, unsigned long maxlen)
-{
- void __iomem **tbl;
-
- BUG_ON(bar >= PCIM_IOMAP_MAX);
-
- tbl = (void __iomem **)pcim_iomap_table(pdev);
- if (!tbl || tbl[bar]) /* duplicate mappings not allowed */
- return NULL;
-
- tbl[bar] = pci_iomap(pdev, bar, maxlen);
- return tbl[bar];
-}
-EXPORT_SYMBOL(pcim_iomap);
-
-/**
- * pcim_iounmap - Managed pci_iounmap()
- * @pdev: PCI device to iounmap for
- * @addr: Address to unmap
- *
- * Managed pci_iounmap(). @addr must have been mapped using pcim_iomap().
- */
-void pcim_iounmap(struct pci_dev *pdev, void __iomem *addr)
-{
- void __iomem **tbl;
- int i;
-
- pci_iounmap(pdev, addr);
-
- tbl = (void __iomem **)pcim_iomap_table(pdev);
- BUG_ON(!tbl);
-
- for (i = 0; i < PCIM_IOMAP_MAX; i++)
- if (tbl[i] == addr) {
- tbl[i] = NULL;
- return;
- }
- WARN_ON(1);
-}
-EXPORT_SYMBOL(pcim_iounmap);
-
-/**
- * pcim_iomap_regions - Request and iomap PCI BARs
- * @pdev: PCI device to map IO resources for
- * @mask: Mask of BARs to request and iomap
- * @name: Name used when requesting regions
- *
- * Request and iomap regions specified by @mask.
- */
-int pcim_iomap_regions(struct pci_dev *pdev, int mask, const char *name)
-{
- void __iomem * const *iomap;
- int i, rc;
-
- iomap = pcim_iomap_table(pdev);
- if (!iomap)
- return -ENOMEM;
-
- for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
- unsigned long len;
-
- if (!(mask & (1 << i)))
- continue;
-
- rc = -EINVAL;
- len = pci_resource_len(pdev, i);
- if (!len)
- goto err_inval;
-
- rc = pci_request_region(pdev, i, name);
- if (rc)
- goto err_inval;
-
- rc = -ENOMEM;
- if (!pcim_iomap(pdev, i, 0))
- goto err_region;
- }
-
- return 0;
-
- err_region:
- pci_release_region(pdev, i);
- err_inval:
- while (--i >= 0) {
- if (!(mask & (1 << i)))
- continue;
- pcim_iounmap(pdev, iomap[i]);
- pci_release_region(pdev, i);
- }
-
- return rc;
-}
-EXPORT_SYMBOL(pcim_iomap_regions);
-
-/**
- * pcim_iomap_regions_request_all - Request all BARs and iomap specified ones
- * @pdev: PCI device to map IO resources for
- * @mask: Mask of BARs to iomap
- * @name: Name used when requesting regions
- *
- * Request all PCI BARs and iomap regions specified by @mask.
- */
-int pcim_iomap_regions_request_all(struct pci_dev *pdev, int mask,
- const char *name)
-{
- int request_mask = ((1 << 6) - 1) & ~mask;
- int rc;
-
- rc = pci_request_selected_regions(pdev, request_mask, name);
- if (rc)
- return rc;
-
- rc = pcim_iomap_regions(pdev, mask, name);
- if (rc)
- pci_release_selected_regions(pdev, request_mask);
- return rc;
-}
-EXPORT_SYMBOL(pcim_iomap_regions_request_all);
-
-/**
- * pcim_iounmap_regions - Unmap and release PCI BARs
- * @pdev: PCI device to map IO resources for
- * @mask: Mask of BARs to unmap and release
- *
- * Unmap and release regions specified by @mask.
- */
-void pcim_iounmap_regions(struct pci_dev *pdev, int mask)
-{
- void __iomem * const *iomap;
- int i;
-
- iomap = pcim_iomap_table(pdev);
- if (!iomap)
- return;
-
- for (i = 0; i < PCIM_IOMAP_MAX; i++) {
- if (!(mask & (1 << i)))
- continue;
-
- pcim_iounmap(pdev, iomap[i]);
- pci_release_region(pdev, i);
- }
-}
-EXPORT_SYMBOL(pcim_iounmap_regions);
-#endif /* CONFIG_PCI */
-
static void devm_arch_phys_ac_add_release(struct device *dev, void *res)
{
arch_phys_wc_del(*((int *)res));
--
2.43.0

2024-01-11 08:58:11

by Philipp Stanner

Subject: [PATCH v5 RESEND 4/5] pci: move devres code from pci.c to devres.c

The file pci.c is very large and contains a number of devres-functions.
These functions should now reside in devres.c

There are a few callers left in pci.c that do devres operations. These
should be ported in the future. Corresponding TODOs are added by this
commit.

The reason they are not moved right now in this commit is that pci's
devres currently implements a sort of "hybrid-mode":
pci_request_region(), for instance, does not have a corresponding pcim_
equivalent, yet. Instead, the function can be made managed by previously
calling pcim_enable_device() (instead of pci_enable_device()). This
makes it unreasonable to move pci_request_region() to devres.c
Moving the functions would require changes to pci's API and is,
therefore, left for future work.

In summary, this commit serves as a preparation step for a following
patch-series that will cleanly separate the PCI's managed and unmanaged
API.

Move as much devres-specific code from pci.c to devres.c as possible.

Suggested-by: Danilo Krummrich <[email protected]>
Signed-off-by: Philipp Stanner <[email protected]>
---
drivers/pci/devres.c | 243 +++++++++++++++++++++++++++++++++++++++++
drivers/pci/pci.c | 249 -------------------------------------------
drivers/pci/pci.h | 24 +++++
3 files changed, 267 insertions(+), 249 deletions(-)

diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
index a3fd0d65cef1..4bd1e125bca1 100644
--- a/drivers/pci/devres.c
+++ b/drivers/pci/devres.c
@@ -1,4 +1,5 @@
// SPDX-License-Identifier: GPL-2.0
+#include <linux/device.h>
#include <linux/pci.h>
#include "pci.h"

@@ -11,6 +12,248 @@ struct pcim_iomap_devres {
void __iomem *table[PCIM_IOMAP_MAX];
};

+
+static void devm_pci_unmap_iospace(struct device *dev, void *ptr)
+{
+ struct resource **res = ptr;
+
+ pci_unmap_iospace(*res);
+}
+
+/**
+ * devm_pci_remap_iospace - Managed pci_remap_iospace()
+ * @dev: Generic device to remap IO address for
+ * @res: Resource describing the I/O space
+ * @phys_addr: physical address of range to be mapped
+ *
+ * Managed pci_remap_iospace(). Map is automatically unmapped on driver
+ * detach.
+ */
+int devm_pci_remap_iospace(struct device *dev, const struct resource *res,
+ phys_addr_t phys_addr)
+{
+ const struct resource **ptr;
+ int error;
+
+ ptr = devres_alloc(devm_pci_unmap_iospace, sizeof(*ptr), GFP_KERNEL);
+ if (!ptr)
+ return -ENOMEM;
+
+ error = pci_remap_iospace(res, phys_addr);
+ if (error) {
+ devres_free(ptr);
+ } else {
+ *ptr = res;
+ devres_add(dev, ptr);
+ }
+
+ return error;
+}
+EXPORT_SYMBOL(devm_pci_remap_iospace);
+
+/**
+ * devm_pci_remap_cfgspace - Managed pci_remap_cfgspace()
+ * @dev: Generic device to remap IO address for
+ * @offset: Resource address to map
+ * @size: Size of map
+ *
+ * Managed pci_remap_cfgspace(). Map is automatically unmapped on driver
+ * detach.
+ */
+void __iomem *devm_pci_remap_cfgspace(struct device *dev,
+ resource_size_t offset,
+ resource_size_t size)
+{
+ void __iomem **ptr, *addr;
+
+ ptr = devres_alloc(devm_ioremap_release, sizeof(*ptr), GFP_KERNEL);
+ if (!ptr)
+ return NULL;
+
+ addr = pci_remap_cfgspace(offset, size);
+ if (addr) {
+ *ptr = addr;
+ devres_add(dev, ptr);
+ } else
+ devres_free(ptr);
+
+ return addr;
+}
+EXPORT_SYMBOL(devm_pci_remap_cfgspace);
+
+/**
+ * devm_pci_remap_cfg_resource - check, request region and ioremap cfg resource
+ * @dev: generic device to handle the resource for
+ * @res: configuration space resource to be handled
+ *
+ * Checks that a resource is a valid memory region, requests the memory
+ * region and ioremaps with pci_remap_cfgspace() API that ensures the
+ * proper PCI configuration space memory attributes are guaranteed.
+ *
+ * All operations are managed and will be undone on driver detach.
+ *
+ * Returns a pointer to the remapped memory or an ERR_PTR() encoded error code
+ * on failure. Usage example::
+ *
+ * res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ * base = devm_pci_remap_cfg_resource(&pdev->dev, res);
+ * if (IS_ERR(base))
+ * return PTR_ERR(base);
+ */
+void __iomem *devm_pci_remap_cfg_resource(struct device *dev,
+ struct resource *res)
+{
+ resource_size_t size;
+ const char *name;
+ void __iomem *dest_ptr;
+
+ BUG_ON(!dev);
+
+ if (!res || resource_type(res) != IORESOURCE_MEM) {
+ dev_err(dev, "invalid resource\n");
+ return IOMEM_ERR_PTR(-EINVAL);
+ }
+
+ size = resource_size(res);
+
+ if (res->name)
+ name = devm_kasprintf(dev, GFP_KERNEL, "%s %s", dev_name(dev),
+ res->name);
+ else
+ name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
+ if (!name)
+ return IOMEM_ERR_PTR(-ENOMEM);
+
+ if (!devm_request_mem_region(dev, res->start, size, name)) {
+ dev_err(dev, "can't request region for resource %pR\n", res);
+ return IOMEM_ERR_PTR(-EBUSY);
+ }
+
+ dest_ptr = devm_pci_remap_cfgspace(dev, res->start, size);
+ if (!dest_ptr) {
+ dev_err(dev, "ioremap failed for resource %pR\n", res);
+ devm_release_mem_region(dev, res->start, size);
+ dest_ptr = IOMEM_ERR_PTR(-ENOMEM);
+ }
+
+ return dest_ptr;
+}
+EXPORT_SYMBOL(devm_pci_remap_cfg_resource);
+
+/**
+ * pcim_set_mwi - a device-managed pci_set_mwi()
+ * @dev: the PCI device for which MWI is enabled
+ *
+ * Managed pci_set_mwi().
+ *
+ * RETURNS: An appropriate -ERRNO error value on error, or zero for success.
+ */
+int pcim_set_mwi(struct pci_dev *dev)
+{
+ struct pci_devres *dr;
+
+ dr = find_pci_dr(dev);
+ if (!dr)
+ return -ENOMEM;
+
+ dr->mwi = 1;
+ return pci_set_mwi(dev);
+}
+EXPORT_SYMBOL(pcim_set_mwi);
+
+
+static void pcim_release(struct device *gendev, void *res)
+{
+ struct pci_dev *dev = to_pci_dev(gendev);
+ struct pci_devres *this = res;
+ int i;
+
+ for (i = 0; i < DEVICE_COUNT_RESOURCE; i++)
+ if (this->region_mask & (1 << i))
+ pci_release_region(dev, i);
+
+ if (this->mwi)
+ pci_clear_mwi(dev);
+
+ if (this->restore_intx)
+ pci_intx(dev, this->orig_intx);
+
+ if (this->enabled && !this->pinned)
+ pci_disable_device(dev);
+}
+
+/*
+ * TODO:
+ * Once the last four callers in pci.c are ported, this function here needs to
+ * be made static again.
+ */
+struct pci_devres *find_pci_dr(struct pci_dev *pdev)
+{
+ if (pci_is_managed(pdev))
+ return devres_find(&pdev->dev, pcim_release, NULL, NULL);
+ return NULL;
+}
+EXPORT_SYMBOL(find_pci_dr);
+
+static struct pci_devres *get_pci_dr(struct pci_dev *pdev)
+{
+ struct pci_devres *dr, *new_dr;
+
+ dr = devres_find(&pdev->dev, pcim_release, NULL, NULL);
+ if (dr)
+ return dr;
+
+ new_dr = devres_alloc(pcim_release, sizeof(*new_dr), GFP_KERNEL);
+ if (!new_dr)
+ return NULL;
+ return devres_get(&pdev->dev, new_dr, NULL, NULL);
+}
+
+/**
+ * pcim_enable_device - Managed pci_enable_device()
+ * @pdev: PCI device to be initialized
+ *
+ * Managed pci_enable_device().
+ */
+int pcim_enable_device(struct pci_dev *pdev)
+{
+ struct pci_devres *dr;
+ int rc;
+
+ dr = get_pci_dr(pdev);
+ if (unlikely(!dr))
+ return -ENOMEM;
+ if (dr->enabled)
+ return 0;
+
+ rc = pci_enable_device(pdev);
+ if (!rc) {
+ pdev->is_managed = 1;
+ dr->enabled = 1;
+ }
+ return rc;
+}
+EXPORT_SYMBOL(pcim_enable_device);
+
+/**
+ * pcim_pin_device - Pin managed PCI device
+ * @pdev: PCI device to pin
+ *
+ * Pin managed PCI device @pdev. Pinned device won't be disabled on
+ * driver detach. @pdev must have been enabled with
+ * pcim_enable_device().
+ */
+void pcim_pin_device(struct pci_dev *pdev)
+{
+ struct pci_devres *dr;
+
+ dr = find_pci_dr(pdev);
+ WARN_ON(!dr || !dr->enabled);
+ if (dr)
+ dr->pinned = 1;
+}
+EXPORT_SYMBOL(pcim_pin_device);
+
static void pcim_iomap_release(struct device *gendev, void *res)
{
struct pci_dev *dev = to_pci_dev(gendev);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 55bc3576a985..742b0a6545b6 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2088,107 +2088,6 @@ int pci_enable_device(struct pci_dev *dev)
}
EXPORT_SYMBOL(pci_enable_device);

-/*
- * Managed PCI resources. This manages device on/off, INTx/MSI/MSI-X
- * on/off and BAR regions. pci_dev itself records MSI/MSI-X status, so
- * there's no need to track it separately. pci_devres is initialized
- * when a device is enabled using managed PCI device enable interface.
- */
-struct pci_devres {
- unsigned int enabled:1;
- unsigned int pinned:1;
- unsigned int orig_intx:1;
- unsigned int restore_intx:1;
- unsigned int mwi:1;
- u32 region_mask;
-};
-
-static void pcim_release(struct device *gendev, void *res)
-{
- struct pci_dev *dev = to_pci_dev(gendev);
- struct pci_devres *this = res;
- int i;
-
- for (i = 0; i < DEVICE_COUNT_RESOURCE; i++)
- if (this->region_mask & (1 << i))
- pci_release_region(dev, i);
-
- if (this->mwi)
- pci_clear_mwi(dev);
-
- if (this->restore_intx)
- pci_intx(dev, this->orig_intx);
-
- if (this->enabled && !this->pinned)
- pci_disable_device(dev);
-}
-
-static struct pci_devres *get_pci_dr(struct pci_dev *pdev)
-{
- struct pci_devres *dr, *new_dr;
-
- dr = devres_find(&pdev->dev, pcim_release, NULL, NULL);
- if (dr)
- return dr;
-
- new_dr = devres_alloc(pcim_release, sizeof(*new_dr), GFP_KERNEL);
- if (!new_dr)
- return NULL;
- return devres_get(&pdev->dev, new_dr, NULL, NULL);
-}
-
-static struct pci_devres *find_pci_dr(struct pci_dev *pdev)
-{
- if (pci_is_managed(pdev))
- return devres_find(&pdev->dev, pcim_release, NULL, NULL);
- return NULL;
-}
-
-/**
- * pcim_enable_device - Managed pci_enable_device()
- * @pdev: PCI device to be initialized
- *
- * Managed pci_enable_device().
- */
-int pcim_enable_device(struct pci_dev *pdev)
-{
- struct pci_devres *dr;
- int rc;
-
- dr = get_pci_dr(pdev);
- if (unlikely(!dr))
- return -ENOMEM;
- if (dr->enabled)
- return 0;
-
- rc = pci_enable_device(pdev);
- if (!rc) {
- pdev->is_managed = 1;
- dr->enabled = 1;
- }
- return rc;
-}
-EXPORT_SYMBOL(pcim_enable_device);
-
-/**
- * pcim_pin_device - Pin managed PCI device
- * @pdev: PCI device to pin
- *
- * Pin managed PCI device @pdev. Pinned device won't be disabled on
- * driver detach. @pdev must have been enabled with
- * pcim_enable_device().
- */
-void pcim_pin_device(struct pci_dev *pdev)
-{
- struct pci_devres *dr;
-
- dr = find_pci_dr(pdev);
- WARN_ON(!dr || !dr->enabled);
- if (dr)
- dr->pinned = 1;
-}
-EXPORT_SYMBOL(pcim_pin_device);
-
/*
* pcibios_device_add - provide arch specific hooks when adding device dev
* @dev: the PCI device being added
@@ -4281,133 +4180,6 @@ void pci_unmap_iospace(struct resource *res)
}
EXPORT_SYMBOL(pci_unmap_iospace);

-static void devm_pci_unmap_iospace(struct device *dev, void *ptr)
-{
- struct resource **res = ptr;
-
- pci_unmap_iospace(*res);
-}
-
-/**
- * devm_pci_remap_iospace - Managed pci_remap_iospace()
- * @dev: Generic device to remap IO address for
- * @res: Resource describing the I/O space
- * @phys_addr: physical address of range to be mapped
- *
- * Managed pci_remap_iospace(). Map is automatically unmapped on driver
- * detach.
- */
-int devm_pci_remap_iospace(struct device *dev, const struct resource *res,
- phys_addr_t phys_addr)
-{
- const struct resource **ptr;
- int error;
-
- ptr = devres_alloc(devm_pci_unmap_iospace, sizeof(*ptr), GFP_KERNEL);
- if (!ptr)
- return -ENOMEM;
-
- error = pci_remap_iospace(res, phys_addr);
- if (error) {
- devres_free(ptr);
- } else {
- *ptr = res;
- devres_add(dev, ptr);
- }
-
- return error;
-}
-EXPORT_SYMBOL(devm_pci_remap_iospace);
-
-/**
- * devm_pci_remap_cfgspace - Managed pci_remap_cfgspace()
- * @dev: Generic device to remap IO address for
- * @offset: Resource address to map
- * @size: Size of map
- *
- * Managed pci_remap_cfgspace(). Map is automatically unmapped on driver
- * detach.
- */
-void __iomem *devm_pci_remap_cfgspace(struct device *dev,
- resource_size_t offset,
- resource_size_t size)
-{
- void __iomem **ptr, *addr;
-
- ptr = devres_alloc(devm_ioremap_release, sizeof(*ptr), GFP_KERNEL);
- if (!ptr)
- return NULL;
-
- addr = pci_remap_cfgspace(offset, size);
- if (addr) {
- *ptr = addr;
- devres_add(dev, ptr);
- } else
- devres_free(ptr);
-
- return addr;
-}
-EXPORT_SYMBOL(devm_pci_remap_cfgspace);
-
-/**
- * devm_pci_remap_cfg_resource - check, request region and ioremap cfg resource
- * @dev: generic device to handle the resource for
- * @res: configuration space resource to be handled
- *
- * Checks that a resource is a valid memory region, requests the memory
- * region and ioremaps with pci_remap_cfgspace() API that ensures the
- * proper PCI configuration space memory attributes are guaranteed.
- *
- * All operations are managed and will be undone on driver detach.
- *
- * Returns a pointer to the remapped memory or an ERR_PTR() encoded error code
- * on failure. Usage example::
- *
- * res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- * base = devm_pci_remap_cfg_resource(&pdev->dev, res);
- * if (IS_ERR(base))
- * return PTR_ERR(base);
- */
-void __iomem *devm_pci_remap_cfg_resource(struct device *dev,
- struct resource *res)
-{
- resource_size_t size;
- const char *name;
- void __iomem *dest_ptr;
-
- BUG_ON(!dev);
-
- if (!res || resource_type(res) != IORESOURCE_MEM) {
- dev_err(dev, "invalid resource\n");
- return IOMEM_ERR_PTR(-EINVAL);
- }
-
- size = resource_size(res);
-
- if (res->name)
- name = devm_kasprintf(dev, GFP_KERNEL, "%s %s", dev_name(dev),
- res->name);
- else
- name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
- if (!name)
- return IOMEM_ERR_PTR(-ENOMEM);
-
- if (!devm_request_mem_region(dev, res->start, size, name)) {
- dev_err(dev, "can't request region for resource %pR\n", res);
- return IOMEM_ERR_PTR(-EBUSY);
- }
-
- dest_ptr = devm_pci_remap_cfgspace(dev, res->start, size);
- if (!dest_ptr) {
- dev_err(dev, "ioremap failed for resource %pR\n", res);
- devm_release_mem_region(dev, res->start, size);
- dest_ptr = IOMEM_ERR_PTR(-ENOMEM);
- }
-
- return dest_ptr;
-}
-EXPORT_SYMBOL(devm_pci_remap_cfg_resource);
-
static void __pci_set_master(struct pci_dev *dev, bool enable)
{
u16 old_cmd, cmd;
@@ -4557,27 +4329,6 @@ int pci_set_mwi(struct pci_dev *dev)
}
EXPORT_SYMBOL(pci_set_mwi);

-/**
- * pcim_set_mwi - a device-managed pci_set_mwi()
- * @dev: the PCI device for which MWI is enabled
- *
- * Managed pci_set_mwi().
- *
- * RETURNS: An appropriate -ERRNO error value on error, or zero for success.
- */
-int pcim_set_mwi(struct pci_dev *dev)
-{
- struct pci_devres *dr;
-
- dr = find_pci_dr(dev);
- if (!dr)
- return -ENOMEM;
-
- dr->mwi = 1;
- return pci_set_mwi(dev);
-}
-EXPORT_SYMBOL(pcim_set_mwi);
-
/**
* pci_try_set_mwi - enables memory-write-invalidate PCI transaction
* @dev: the PCI device for which MWI is enabled
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 5ecbcf041179..69052059dbd2 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -793,6 +793,30 @@ static inline pci_power_t mid_pci_get_power_state(struct pci_dev *pdev)
}
#endif

+/*
+ * TODO:
+ * The following two components wouldn't need to be here if they weren't used at
+ * four last places in pci.c
+ * Port or move these functions to devres.c and then remove the
+ * pci_devres-components from this header file here.
+ */
+/*
+ * Managed PCI resources. This manages device on/off, INTx/MSI/MSI-X
+ * on/off and BAR regions. pci_dev itself records MSI/MSI-X status, so
+ * there's no need to track it separately. pci_devres is initialized
+ * when a device is enabled using managed PCI device enable interface.
+ */
+struct pci_devres {
+ unsigned int enabled:1;
+ unsigned int pinned:1;
+ unsigned int orig_intx:1;
+ unsigned int restore_intx:1;
+ unsigned int mwi:1;
+ u32 region_mask;
+};
+
+struct pci_devres *find_pci_dr(struct pci_dev *pdev);
+
/*
* Config Address for PCI Configuration Mechanism #1
*
--
2.43.0

2024-01-11 08:58:57

by Philipp Stanner

Subject: [PATCH v5 RESEND 5/5] lib, pci: unify generic pci_iounmap()

The implementation of pci_iounmap() is currently scattered over two
files, drivers/pci/iomap.c and lib/iomap.c. Additionally,
architectures can define their own version.

To have only one version, it's necessary to create a helper function,
iomem_is_ioport(), that tells pci_iounmap() whether the passed address
points to an ioport or normal memory.

iomem_is_ioport() can be provided through two different ways:
1. The architecture itself provides it. As of today, the version
coming from lib/iomap.c de facto is the x86-specific version and
comes into play when CONFIG_GENERIC_IOMAP is selected. This rather
confusing naming is an artifact left by the removal of IA64.
2. As a default version in include/asm-generic/io.h for those
architectures that don't use CONFIG_GENERIC_IOMAP, but also don't
provide their own version of iomem_is_ioport().

Once all architectures that support ports provide iomem_is_ioport(), the
arch-specific definitions for pci_iounmap() can be removed and the archs
can use the generic implementation, instead.

Create a unified version of pci_iounmap() in drivers/pci/iomap.c.
Provide the function iomem_is_ioport() in include/asm-generic/io.h
(generic) and lib/iomap.c ("pseudo-generic" for x86).

Remove the CONFIG_GENERIC_IOMAP guard around
ARCH_WANTS_GENERIC_PCI_IOUNMAP so that configs that set
CONFIG_GENERIC_PCI_IOMAP without CONFIG_GENERIC_IOMAP still get the
function.

Add TODOs for follow-up work on the "generic is not generic but
x86-specific"-Problem.

Suggested-by: Arnd Bergmann <[email protected]>
Signed-off-by: Philipp Stanner <[email protected]>
Reviewed-by: Arnd Bergmann <[email protected]>
---
drivers/pci/iomap.c | 46 +++++++++++++------------------------
include/asm-generic/io.h | 27 ++++++++++++++++++++--
include/asm-generic/iomap.h | 21 +++++++++++++++++
lib/iomap.c | 28 ++++++++++++++++------
4 files changed, 83 insertions(+), 39 deletions(-)

diff --git a/drivers/pci/iomap.c b/drivers/pci/iomap.c
index 91285fcff1ba..b7faf22ec8f5 100644
--- a/drivers/pci/iomap.c
+++ b/drivers/pci/iomap.c
@@ -135,44 +135,30 @@ void __iomem *pci_iomap_wc(struct pci_dev *dev, int bar, unsigned long maxlen)
EXPORT_SYMBOL_GPL(pci_iomap_wc);

/*
- * 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.
+ * This check is still necessary due to legacy reasons.
*
- * But if the architecture does not use the generic iomap code, and if
- * it has _not_ defined it's own private pci_iounmap function, we define
- * it here.
- *
- * NOTE! This default implementation assumes that if the architecture
- * support ioport mapping (HAS_IOPORT_MAP), the ioport mapping will
- * be fixed to the range [ PCI_IOBASE, PCI_IOBASE+IO_SPACE_LIMIT [,
- * and does not need unmapping with 'ioport_unmap()'.
- *
- * If you have different rules for your architecture, you need to
- * implement your own pci_iounmap() that knows the rules for where
- * and how IO vs MEM get mapped.
- *
- * This code is odd, and the ARCH_HAS/ARCH_WANTS #define logic comes
- * from legacy <asm-generic/io.h> header file behavior. In particular,
- * it would seem to make sense to do the iounmap(p) for the non-IO-space
- * case here regardless, but that's not what the old header file code
- * did. Probably incorrectly, but this is meant to be bug-for-bug
- * compatible.
+ * TODO: Have all architectures that provide their own pci_iounmap() provide
+ * iomem_is_ioport() instead. Remove this #if afterwards.
*/
#if defined(ARCH_WANTS_GENERIC_PCI_IOUNMAP)

-void pci_iounmap(struct pci_dev *dev, void __iomem *p)
+/**
+ * pci_iounmap - Unmapp a mapping
+ * @dev: PCI device the mapping belongs to
+ * @addr: start address of the mapping
+ *
+ * Unmapp a PIO or MMIO mapping.
+ */
+void pci_iounmap(struct pci_dev *dev, void __iomem *addr)
{
-#ifdef ARCH_HAS_GENERIC_IOPORT_MAP
- uintptr_t start = (uintptr_t) PCI_IOBASE;
- uintptr_t addr = (uintptr_t) p;
-
- if (addr >= start && addr < start + IO_SPACE_LIMIT) {
- ioport_unmap(p);
+#ifdef CONFIG_HAS_IOPORT_MAP
+ if (iomem_is_ioport(addr)) {
+ ioport_unmap(addr);
return;
}
#endif
- iounmap(p);
+
+ iounmap(addr);
}
EXPORT_SYMBOL(pci_iounmap);

diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index bac63e874c7b..58c7bf4080da 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -1129,11 +1129,34 @@ extern void ioport_unmap(void __iomem *p);
#endif /* CONFIG_GENERIC_IOMAP */
#endif /* CONFIG_HAS_IOPORT_MAP */

-#ifndef CONFIG_GENERIC_IOMAP
+/*
+ * TODO:
+ * remove this once all architectures replaced their pci_iounmap() with
+ * a custom implementation of iomem_is_ioport().
+ */
#ifndef pci_iounmap
+#define pci_iounmap pci_iounmap
#define ARCH_WANTS_GENERIC_PCI_IOUNMAP
+#endif /* pci_iounmap */
+
+/*
+ * This function is a helper only needed for the generic pci_iounmap().
+ * It's provided here if the architecture does not provide its own version.
+ */
+#ifndef iomem_is_ioport
+#define iomem_is_ioport iomem_is_ioport
+static inline bool iomem_is_ioport(void __iomem *addr_raw)
+{
+#ifdef CONFIG_HAS_IOPORT
+ uintptr_t start = (uintptr_t)PCI_IOBASE;
+ uintptr_t addr = (uintptr_t)addr_raw;
+
+ if (addr >= start && addr < start + IO_SPACE_LIMIT)
+ return true;
#endif
-#endif
+ return false;
+}
+#endif /* iomem_is_ioport */

#ifndef xlate_dev_mem_ptr
#define xlate_dev_mem_ptr xlate_dev_mem_ptr
diff --git a/include/asm-generic/iomap.h b/include/asm-generic/iomap.h
index 196087a8126e..2cdc6988a102 100644
--- a/include/asm-generic/iomap.h
+++ b/include/asm-generic/iomap.h
@@ -110,6 +110,27 @@ static inline void __iomem *ioremap_np(phys_addr_t offset, size_t size)
}
#endif

+/*
+ * If CONFIG_GENERIC_IOMAP is selected and the architecture does NOT provide its
+ * own version, ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT makes sure that the generic
+ * version from asm-generic/io.h is NOT used and instead the second "generic"
+ * version from lib/iomap.c is used.
+ *
+ * There are currently two generic versions because of a difficult cleanup
+ * process. Namely, the version in lib/iomap.c once was really generic when IA64
+ * still existed. Today, it's only really used by x86.
+ *
+ * TODO: Move the version from lib/iomap.c to x86 specific code. Then, remove
+ * this ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT-mechanism.
+ */
+#ifdef CONFIG_GENERIC_IOMAP
+#ifndef iomem_is_ioport
+#define iomem_is_ioport iomem_is_ioport
+bool iomem_is_ioport(void __iomem *addr);
+#define ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT
+#endif /* iomem_is_ioport */
+#endif /* CONFIG_GENERIC_IOMAP */
+
#include <asm-generic/pci_iomap.h>

#endif
diff --git a/lib/iomap.c b/lib/iomap.c
index 4f8b31baa575..eb9a879ebf42 100644
--- a/lib/iomap.c
+++ b/lib/iomap.c
@@ -418,12 +418,26 @@ EXPORT_SYMBOL(ioport_map);
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. */
-void pci_iounmap(struct pci_dev *dev, void __iomem * addr)
+/*
+ * If CONFIG_GENERIC_IOMAP is selected and the architecture does NOT provide its
+ * own version, ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT makes sure that the generic
+ * version from asm-generic/io.h is NOT used and instead the second "generic"
+ * version from this file here is used.
+ *
+ * There are currently two generic versions because of a difficult cleanup
+ * process. Namely, the version in lib/iomap.c once was really generic when IA64
+ * still existed. Today, it's only really used by x86.
+ *
+ * TODO: Move this function to x86-specific code.
+ */
+#if defined(ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT)
+bool iomem_is_ioport(void __iomem *addr)
{
- IO_COND(addr, /* nothing */, iounmap(addr));
+ unsigned long port = (unsigned long __force)addr;
+
+ if (port > PIO_OFFSET && port < PIO_RESERVED)
+ return true;
+
+ return false;
}
-EXPORT_SYMBOL(pci_iounmap);
-#endif /* CONFIG_PCI */
+#endif /* ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT */
--
2.43.0

2024-01-11 09:04:31

by Philipp Stanner

Subject: [PATCH v5 RESEND 2/5] lib: move pci_iomap.c to drivers/pci/

This file is guarded by an #ifdef CONFIG_PCI. It, consequently, does not
belong to lib/ because it is not generic infrastructure.

Move the file to drivers/pci/ and implement the necessary changes to
Makefiles and Kconfigs.

Update MAINTAINERS file.

Suggested-by: Danilo Krummrich <[email protected]>
Signed-off-by: Philipp Stanner <[email protected]>
Reviewed-by: Arnd Bergmann <[email protected]>
---
MAINTAINERS | 1 -
drivers/pci/Kconfig | 5 +++++
drivers/pci/Makefile | 1 +
lib/pci_iomap.c => drivers/pci/iomap.c | 3 ---
lib/Kconfig | 3 ---
lib/Makefile | 1 -
6 files changed, 6 insertions(+), 8 deletions(-)
rename lib/pci_iomap.c => drivers/pci/iomap.c (99%)

diff --git a/MAINTAINERS b/MAINTAINERS
index edae86acdfdc..efa37ee81d30 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16762,7 +16762,6 @@ F: include/asm-generic/pci*
F: include/linux/of_pci.h
F: include/linux/pci*
F: include/uapi/linux/pci*
-F: lib/pci*

PCIE DRIVER FOR AMAZON ANNAPURNA LABS
M: Jonathan Chocron <[email protected]>
diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
index 74147262625b..d35001589d88 100644
--- a/drivers/pci/Kconfig
+++ b/drivers/pci/Kconfig
@@ -13,6 +13,11 @@ config FORCE_PCI
select HAVE_PCI
select PCI

+# select this to provide a generic PCI iomap,
+# without PCI itself having to be defined
+config GENERIC_PCI_IOMAP
+ bool
+
menuconfig PCI
bool "PCI support"
depends on HAVE_PCI
diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index cc8b4e01e29d..64dcedccfc87 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -14,6 +14,7 @@ ifdef CONFIG_PCI
obj-$(CONFIG_PROC_FS) += proc.o
obj-$(CONFIG_SYSFS) += slot.o
obj-$(CONFIG_ACPI) += pci-acpi.o
+obj-$(CONFIG_GENERIC_PCI_IOMAP) += iomap.o
endif

obj-$(CONFIG_OF) += of.o
diff --git a/lib/pci_iomap.c b/drivers/pci/iomap.c
similarity index 99%
rename from lib/pci_iomap.c
rename to drivers/pci/iomap.c
index 6e144b017c48..91285fcff1ba 100644
--- a/lib/pci_iomap.c
+++ b/drivers/pci/iomap.c
@@ -9,7 +9,6 @@

#include <linux/export.h>

-#ifdef CONFIG_PCI
/**
* pci_iomap_range - create a virtual mapping cookie for a PCI BAR
* @dev: PCI device that owns the BAR
@@ -178,5 +177,3 @@ void pci_iounmap(struct pci_dev *dev, void __iomem *p)
EXPORT_SYMBOL(pci_iounmap);

#endif /* ARCH_WANTS_GENERIC_PCI_IOUNMAP */
-
-#endif /* CONFIG_PCI */
diff --git a/lib/Kconfig b/lib/Kconfig
index 3ea1c830efab..1bf859166ac7 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -70,9 +70,6 @@ source "lib/math/Kconfig"
config NO_GENERIC_PCI_IOPORT_MAP
bool

-config GENERIC_PCI_IOMAP
- bool
-
config GENERIC_IOMAP
bool
select GENERIC_PCI_IOMAP
diff --git a/lib/Makefile b/lib/Makefile
index 6b09731d8e61..0800289ec6c5 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -153,7 +153,6 @@ CFLAGS_debug_info.o += $(call cc-option, -femit-struct-debug-detailed=any)
obj-y += math/ crypto/

obj-$(CONFIG_GENERIC_IOMAP) += iomap.o
-obj-$(CONFIG_GENERIC_PCI_IOMAP) += pci_iomap.o
obj-$(CONFIG_HAS_IOMEM) += iomap_copy.o devres.o
obj-$(CONFIG_CHECK_SIGNATURE) += check_signature.o
obj-$(CONFIG_DEBUG_LOCKING_API_SELFTESTS) += locking-selftest.o
--
2.43.0

2024-01-11 09:05:22

by Johannes Berg

Subject: Re: [PATCH v5 RESEND 0/5] Regather scattered PCI-Code

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

2024-01-11 09:23:10

by Johannes Berg

Subject: Re: [PATCH v5 RESEND 0/5] Regather scattered PCI-Code

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
> [email protected], [email protected],
> [email protected], [email protected]
>
> Anyways, PCI is for sure who should merge this, since it's 100% about
> PCI.

Sounds good :)

johannes

2024-01-11 09:25:51

by Philipp Stanner

Subject: Re: [PATCH v5 RESEND 0/5] Regather scattered PCI-Code

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
[email protected], [email protected],
[email protected], [email protected]

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
>

2024-01-11 14:59:03

by Bjorn Helgaas

Subject: Re: [PATCH v5 RESEND 0/5] Regather scattered PCI-Code

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

2024-01-11 15:32:47

by Philipp Stanner

Subject: Re: [PATCH v5 RESEND 0/5] Regather scattered PCI-Code

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

2024-01-23 18:47:18

by Bjorn Helgaas

Subject: Re: [PATCH v5 RESEND 1/5] lib/pci_iomap.c: fix cleanup bugs in pci_iounmap()

On Thu, Jan 11, 2024 at 09:55:36AM +0100, Philipp Stanner wrote:
> pci_iounmap() in lib/pci_iomap.c is supposed to check whether an address
> is within ioport-range IF the config specifies that ioports exist. If
> so, the port should be unmapped with ioport_unmap(). If not, it's a
> generic MMIO address that has to be passed to iounmap().
>
> The bugs are:
> 1. ioport_unmap() is missing entirely, so this function will never
> actually unmap a port.

The preceding comment suggests that in this default implementation,
the ioport does not need unmapping, and it wasn't something it was
supposed to do but just failed to do:

* NOTE! This default implementation assumes that if the architecture
* support ioport mapping (HAS_IOPORT_MAP), the ioport mapping will
* be fixed to the range [ PCI_IOBASE, PCI_IOBASE+IO_SPACE_LIMIT [,
* and does not need unmapping with 'ioport_unmap()'.
*
* If you have different rules for your architecture, you need to
* implement your own pci_iounmap() that knows the rules for where
* and how IO vs MEM get mapped.

Almost all ioport_unmap() implementations are empty, so in most cases
it's a no-op (parisc is an exception).

I'm happy to add the ioport_unmap() even just for symmetry, but if we
do, I think we should update or remove that comment.

> 2. the #ifdef for the ioport-ranges accidentally also guards
> iounmap(), potentially compiling an empty function. This would
> cause the mapping to be leaked.
>
> Implement the missing call to ioport_unmap().
>
> Move the guard so that iounmap() will always be part of the function.

I think we should fix this bug in a separate patch because the
ioport_unmap() is much more subtle and doesn't need to be complicated
with this fix.

> CC: <[email protected]> # v5.15+
> Fixes: 316e8d79a095 ("pci_iounmap'2: Electric Boogaloo: try to make sense of it all")
> Reported-by: Danilo Krummrich <[email protected]>

Is there a URL we can include for Danilo's report? I found
https://lore.kernel.org/all/[email protected]/,
but I'm not sure that's the right part of the conversation.

> Suggested-by: Arnd Bergmann <[email protected]>
> Signed-off-by: Philipp Stanner <[email protected]>
> Reviewed-by: Arnd Bergmann <[email protected]>
> ---
> lib/pci_iomap.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/lib/pci_iomap.c b/lib/pci_iomap.c
> index ce39ce9f3526..6e144b017c48 100644
> --- a/lib/pci_iomap.c
> +++ b/lib/pci_iomap.c
> @@ -168,10 +168,12 @@ void pci_iounmap(struct pci_dev *dev, void __iomem *p)
> uintptr_t start = (uintptr_t) PCI_IOBASE;
> uintptr_t addr = (uintptr_t) p;
>
> - if (addr >= start && addr < start + IO_SPACE_LIMIT)
> + if (addr >= start && addr < start + IO_SPACE_LIMIT) {
> + ioport_unmap(p);
> return;
> - iounmap(p);
> + }
> #endif
> + iounmap(p);
> }
> EXPORT_SYMBOL(pci_iounmap);
>
> --
> 2.43.0
>

2024-01-23 20:20:46

by Bjorn Helgaas

Subject: Re: [PATCH v5 RESEND 2/5] lib: move pci_iomap.c to drivers/pci/

On Thu, Jan 11, 2024 at 09:55:37AM +0100, Philipp Stanner wrote:
> This file is guarded by an #ifdef CONFIG_PCI. It, consequently, does not
> belong to lib/ because it is not generic infrastructure.
>
> Move the file to drivers/pci/ and implement the necessary changes to
> Makefiles and Kconfigs.
> ...

> --- a/drivers/pci/Kconfig
> +++ b/drivers/pci/Kconfig
> @@ -13,6 +13,11 @@ config FORCE_PCI
> select HAVE_PCI
> select PCI
>
> +# select this to provide a generic PCI iomap,
> +# without PCI itself having to be defined
> +config GENERIC_PCI_IOMAP
> + bool

> --- a/lib/pci_iomap.c
> +++ b/drivers/pci/iomap.c
> @@ -9,7 +9,6 @@
>
> #include <linux/export.h>
>
> -#ifdef CONFIG_PCI

IIUC, in the case where CONFIG_GENERIC_PCI_IOMAP=y but CONFIG_PCI was
not set, pci_iomap.c was compiled but produced no code because the
entire file was wrapped with this #ifdef.

But after this patch, it looks like pci_iomap_range(),
pci_iomap_wc_range(), etc., *will* be compiled?

Is that what you intend, or did I miss something?

Bjorn

2024-01-23 21:06:43

by Bjorn Helgaas

Subject: Re: [PATCH v5 RESEND 5/5] lib, pci: unify generic pci_iounmap()

On Thu, Jan 11, 2024 at 09:55:40AM +0100, Philipp Stanner wrote:
> The implementation of pci_iounmap() is currently scattered over two
> files, drivers/pci/iomap.c and lib/iomap.c. Additionally,
> architectures can define their own version.
>
> To have only one version, it's necessary to create a helper function,
> iomem_is_ioport(), that tells pci_iounmap() whether the passed address
> points to an ioport or normal memory.
>
> iomem_is_ioport() can be provided through two different ways:
> 1. The architecture itself provides it. As of today, the version
> coming from lib/iomap.c de facto is the x86-specific version and
> comes into play when CONFIG_GENERIC_IOMAP is selected. This rather
> confusing naming is an artifact left by the removal of IA64.
> 2. As a default version in include/asm-generic/io.h for those
> architectures that don't use CONFIG_GENERIC_IOMAP, but also don't
> provide their own version of iomem_is_ioport().
>
> Once all architectures that support ports provide iomem_is_ioport(), the
> arch-specific definitions for pci_iounmap() can be removed and the archs
> can use the generic implementation, instead.
>
> Create a unified version of pci_iounmap() in drivers/pci/iomap.c.
> Provide the function iomem_is_ioport() in include/asm-generic/io.h
> (generic) and lib/iomap.c ("pseudo-generic" for x86).
>
> Remove the CONFIG_GENERIC_IOMAP guard around
> ARCH_WANTS_GENERIC_PCI_IOUNMAP so that configs that set
> CONFIG_GENERIC_PCI_IOMAP without CONFIG_GENERIC_IOMAP still get the
> function.
>
> Add TODOs for follow-up work on the "generic is not generic but
> x86-specific"-Problem.
> ...

> +++ b/drivers/pci/iomap.c
> @@ -135,44 +135,30 @@ void __iomem *pci_iomap_wc(struct pci_dev *dev, int bar, unsigned long maxlen)
> EXPORT_SYMBOL_GPL(pci_iomap_wc);
>
> /*
> - * 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.
> + * This check is still necessary due to legacy reasons.
> *
> - * But if the architecture does not use the generic iomap code, and if
> - * it has _not_ defined it's own private pci_iounmap function, we define
> - * it here.
> - *
> - * NOTE! This default implementation assumes that if the architecture
> - * support ioport mapping (HAS_IOPORT_MAP), the ioport mapping will
> - * be fixed to the range [ PCI_IOBASE, PCI_IOBASE+IO_SPACE_LIMIT [,
> - * and does not need unmapping with 'ioport_unmap()'.
> - *
> - * If you have different rules for your architecture, you need to
> - * implement your own pci_iounmap() that knows the rules for where
> - * and how IO vs MEM get mapped.
> - *
> - * This code is odd, and the ARCH_HAS/ARCH_WANTS #define logic comes
> - * from legacy <asm-generic/io.h> header file behavior. In particular,
> - * it would seem to make sense to do the iounmap(p) for the non-IO-space
> - * case here regardless, but that's not what the old header file code
> - * did. Probably incorrectly, but this is meant to be bug-for-bug
> - * compatible.

Moving this comment update to the patch that adds the ioport_unmap()
call would make that patch more consistent and simplify this patch.

> + * TODO: Have all architectures that provide their own pci_iounmap() provide
> + * iomem_is_ioport() instead. Remove this #if afterwards.
> */
> #if defined(ARCH_WANTS_GENERIC_PCI_IOUNMAP)
>
> -void pci_iounmap(struct pci_dev *dev, void __iomem *p)
> +/**
> + * pci_iounmap - Unmapp a mapping
> + * @dev: PCI device the mapping belongs to
> + * @addr: start address of the mapping
> + *
> + * Unmapp a PIO or MMIO mapping.

s/Unmapp/Unmap/ (twice)

> + */
> +void pci_iounmap(struct pci_dev *dev, void __iomem *addr)

Maybe move the "p" to "addr" rename to the patch that fixes the
pci_iounmap() #ifdef problem, since that's a trivial change that
already has to do with handling both PIO and MMIO? Then this patch
would be a little more focused.

The kernel-doc addition could possibly also move there since it isn't
related to the unification.

> {
> -#ifdef ARCH_HAS_GENERIC_IOPORT_MAP
> - uintptr_t start = (uintptr_t) PCI_IOBASE;
> - uintptr_t addr = (uintptr_t) p;
> -
> - if (addr >= start && addr < start + IO_SPACE_LIMIT) {
> - ioport_unmap(p);
> +#ifdef CONFIG_HAS_IOPORT_MAP
> + if (iomem_is_ioport(addr)) {
> + ioport_unmap(addr);
> return;
> }
> #endif
> - iounmap(p);
> +
> + iounmap(addr);
> }

> + * If CONFIG_GENERIC_IOMAP is selected and the architecture does NOT provide its
> + * own version, ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT makes sure that the generic
> + * version from asm-generic/io.h is NOT used and instead the second "generic"
> + * version from this file here is used.
> + *
> + * There are currently two generic versions because of a difficult cleanup
> + * process. Namely, the version in lib/iomap.c once was really generic when IA64
> + * still existed. Today, it's only really used by x86.
> + *
> + * TODO: Move this function to x86-specific code.

Some of these TODOs look fairly simple. Are they actually hard, or
could they just be done now?

It seems like implementing iomem_is_ioport() for the other arches
would be straightforward and if done first, could make this patch look
tidier.

Or if the TODOs can't be done now, maybe the iomem_is_ioport()
addition could be done as a separate patch to make the unification
more obvious.

> + */
> +#if defined(ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT)
> +bool iomem_is_ioport(void __iomem *addr)
> {
> - IO_COND(addr, /* nothing */, iounmap(addr));
> + unsigned long port = (unsigned long __force)addr;
> +
> + if (port > PIO_OFFSET && port < PIO_RESERVED)
> + return true;
> +
> + return false;
> }
> -EXPORT_SYMBOL(pci_iounmap);
> -#endif /* CONFIG_PCI */
> +#endif /* ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT */
> --
> 2.43.0
>

2024-01-25 15:09:07

by Philipp Stanner

Subject: Re: [PATCH v5 RESEND 2/5] lib: move pci_iomap.c to drivers/pci/

On Tue, 2024-01-23 at 14:20 -0600, Bjorn Helgaas wrote:
> On Thu, Jan 11, 2024 at 09:55:37AM +0100, Philipp Stanner wrote:
> > This file is guarded by an #ifdef CONFIG_PCI. It, consequently,
> > does not
> > belong to lib/ because it is not generic infrastructure.
> >
> > Move the file to drivers/pci/ and implement the necessary changes
> > to
> > Makefiles and Kconfigs.
> > ...
>
> > --- a/drivers/pci/Kconfig
> > +++ b/drivers/pci/Kconfig
> > @@ -13,6 +13,11 @@ config FORCE_PCI
> >         select HAVE_PCI
> >         select PCI
> >  
> > +# select this to provide a generic PCI iomap,
> > +# without PCI itself having to be defined
> > +config GENERIC_PCI_IOMAP
> > +       bool
>
> > --- a/lib/pci_iomap.c
> > +++ b/drivers/pci/iomap.c
> > @@ -9,7 +9,6 @@
> >  
> >  #include <linux/export.h>
> >  
> > -#ifdef CONFIG_PCI
>
> IIUC, in the case where CONFIG_GENERIC_PCI_IOMAP=y but CONFIG_PCI was
> not set, pci_iomap.c was compiled but produced no code because the
> entire file was wrapped with this #ifdef.
>
> But after this patch, it looks like pci_iomap_range(),
> pci_iomap_wc_range(), etc., *will* be compiled?
>
> Is that what you intend, or did I miss something?

They *will* be compiled when BOTH, CONFIG_PCI and
CONFIG_GENERIC_PCI_IOMAP have been set. It's a bit hard to see that in
the patch's diff. Here, look closely:

--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -14,6 +14,7 @@ ifdef CONFIG_PCI <-----------
obj-$(CONFIG_PROC_FS) += proc.o
obj-$(CONFIG_SYSFS) += slot.o
obj-$(CONFIG_ACPI) += pci-acpi.o
+obj-$(CONFIG_GENERIC_PCI_IOMAP) += iomap.o <------------
endif

So if I am not mistaken it behaves 100% as it did before

I prefered Makefile-logic over even more C Preprocessor to implement
that. The preprocessor has caused us so much trouble... :(

P.


>
> Bjorn
>

2024-01-25 16:07:16

by Philipp Stanner

Subject: Re: [PATCH v5 RESEND 1/5] lib/pci_iomap.c: fix cleanup bugs in pci_iounmap()

On Tue, 2024-01-23 at 12:46 -0600, Bjorn Helgaas wrote:
> On Thu, Jan 11, 2024 at 09:55:36AM +0100, Philipp Stanner wrote:
> > pci_iounmap() in lib/pci_iomap.c is supposed to check whether an
> > address
> > is within ioport-range IF the config specifies that ioports exist.
> > If
> > so, the port should be unmapped with ioport_unmap(). If not, it's a
> > generic MMIO address that has to be passed to iounmap().
> >
> > The bugs are:
> >   1. ioport_unmap() is missing entirely, so this function will
> > never
> >      actually unmap a port.
>
> The preceding comment suggests that in this default implementation,
> the ioport does not need unmapping, and it wasn't something it was
> supposed to do but just failed to do:
>
>  * NOTE! This default implementation assumes that if the architecture
>  * support ioport mapping (HAS_IOPORT_MAP), the ioport mapping will
>  * be fixed to the range [ PCI_IOBASE, PCI_IOBASE+IO_SPACE_LIMIT [,
>  * and does not need unmapping with 'ioport_unmap()'.
>  *
>  * If you have different rules for your architecture, you need to
>  * implement your own pci_iounmap() that knows the rules for where
>  * and how IO vs MEM get mapped.
>
> Almost all ioport_unmap() implementations are empty, so in most cases
> it's a no-op (parisc is an exception).

That sounds correct.

>
> I'm happy to add the ioport_unmap() even just for symmetry, but if we
> do, I think we should update or remove that comment.

Yes, I think it's the right way: either all architectures should
provide ioport_unmap(), empty or not, or all should use a centralized
PCI function

I can remove the wrong statement.

>
> >   2. the #ifdef for the ioport-ranges accidentally also guards
> >      iounmap(), potentially compiling an empty function. This would
> >      cause the mapping to be leaked.
> >
> > Implement the missing call to ioport_unmap().
> >
> > Move the guard so that iounmap() will always be part of the
> > function.
>
> I think we should fix this bug in a separate patch because the
> ioport_unmap() is much more subtle and doesn't need to be complicated
> with this fix.

If we agree that one is a bug and the other isn't, then ACK, we should
probably split it.

>
> > CC: <[email protected]> # v5.15+
> > Fixes: 316e8d79a095 ("pci_iounmap'2: Electric Boogaloo: try to make
> > sense of it all")
> > Reported-by: Danilo Krummrich <[email protected]>
>
> Is there a URL we can include for Danilo's report?  I found
> https://lore.kernel.org/all/[email protected]/
> ,
> but I'm not sure that's the right part of the conversation.

He pointed out it's a bug in an offlist conversation with me. The link
you provided is his only public statement about the topic.
The Reported-by served more acknowledging the contribution than issue-
tracking


P.

>
> > Suggested-by: Arnd Bergmann <[email protected]>
> > Signed-off-by: Philipp Stanner <[email protected]>
> > Reviewed-by: Arnd Bergmann <[email protected]>
> > ---
> >  lib/pci_iomap.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/pci_iomap.c b/lib/pci_iomap.c
> > index ce39ce9f3526..6e144b017c48 100644
> > --- a/lib/pci_iomap.c
> > +++ b/lib/pci_iomap.c
> > @@ -168,10 +168,12 @@ void pci_iounmap(struct pci_dev *dev, void
> > __iomem *p)
> >         uintptr_t start = (uintptr_t) PCI_IOBASE;
> >         uintptr_t addr = (uintptr_t) p;
> >  
> > -       if (addr >= start && addr < start + IO_SPACE_LIMIT)
> > +       if (addr >= start && addr < start + IO_SPACE_LIMIT) {
> > +               ioport_unmap(p);
> >                 return;
> > -       iounmap(p);
> > +       }
> >  #endif
> > +       iounmap(p);
> >  }
> >  EXPORT_SYMBOL(pci_iounmap);
> >  
> > --
> > 2.43.0
> >
>

2024-01-25 18:34:10

by Bjorn Helgaas

Subject: Re: [PATCH v5 RESEND 2/5] lib: move pci_iomap.c to drivers/pci/

On Thu, Jan 25, 2024 at 03:54:51PM +0100, Philipp Stanner wrote:
> On Tue, 2024-01-23 at 14:20 -0600, Bjorn Helgaas wrote:
> > On Thu, Jan 11, 2024 at 09:55:37AM +0100, Philipp Stanner wrote:
> > > This file is guarded by an #ifdef CONFIG_PCI. It, consequently,
> > > does not
> > > belong to lib/ because it is not generic infrastructure.
> > >
> > > Move the file to drivers/pci/ and implement the necessary changes
> > > to
> > > Makefiles and Kconfigs.
> > > ...
> >
> > > --- a/drivers/pci/Kconfig
> > > +++ b/drivers/pci/Kconfig
> > > @@ -13,6 +13,11 @@ config FORCE_PCI
> > >         select HAVE_PCI
> > >         select PCI
> > >  
> > > +# select this to provide a generic PCI iomap,
> > > +# without PCI itself having to be defined
> > > +config GENERIC_PCI_IOMAP
> > > +       bool
> >
> > > --- a/lib/pci_iomap.c
> > > +++ b/drivers/pci/iomap.c
> > > @@ -9,7 +9,6 @@
> > >  
> > >  #include <linux/export.h>
> > >  
> > > -#ifdef CONFIG_PCI
> >
> > IIUC, in the case where CONFIG_GENERIC_PCI_IOMAP=y but CONFIG_PCI was
> > not set, pci_iomap.c was compiled but produced no code because the
> > entire file was wrapped with this #ifdef.
> >
> > But after this patch, it looks like pci_iomap_range(),
> > pci_iomap_wc_range(), etc., *will* be compiled?
> >
> > Is that what you intend, or did I miss something?
>
> They *will* be compiled when BOTH, CONFIG_PCI and
> CONFIG_GENERIC_PCI_IOMAP have been set.

I was asking about CONFIG_GENERIC_PCI_IOMAP=y but CONFIG_PCI unset.

But the Makefile contains this:

ifdef CONFIG_PCI
obj-$(CONFIG_GENERIC_PCI_IOMAP) += iomap.o
endif

So iomap.c will not be compiled when CONFIG_PCI is unset, which is
what I missed.

Bjorn

2024-01-26 13:59:44

by Philipp Stanner

Subject: Re: [PATCH v5 RESEND 5/5] lib, pci: unify generic pci_iounmap()

On Tue, 2024-01-23 at 15:05 -0600, Bjorn Helgaas wrote:
> On Thu, Jan 11, 2024 at 09:55:40AM +0100, Philipp Stanner wrote:
> > The implementation of pci_iounmap() is currently scattered over two
> > files, drivers/pci/iomap.c and lib/iomap.c. Additionally,
> > architectures can define their own version.
> >
> > To have only one version, it's necessary to create a helper
> > function,
> > iomem_is_ioport(), that tells pci_iounmap() whether the passed
> > address
> > points to an ioport or normal memory.
> >
> > iomem_is_ioport() can be provided through two different ways:
> >   1. The architecture itself provides it. As of today, the version
> >      coming from lib/iomap.c de facto is the x86-specific version
> > and
> >      comes into play when CONFIG_GENERIC_IOMAP is selected. This
> > rather
> >      confusing naming is an artifact left by the removal of IA64.
> >   2. As a default version in include/asm-generic/io.h for those
> >      architectures that don't use CONFIG_GENERIC_IOMAP, but also
> > don't
> >      provide their own version of iomem_is_ioport().
> >
> > Once all architectures that support ports provide
> > iomem_is_ioport(), the
> > arch-specific definitions for pci_iounmap() can be removed and the
> > archs
> > can use the generic implementation, instead.
> >
> > Create a unified version of pci_iounmap() in drivers/pci/iomap.c.
> > Provide the function iomem_is_ioport() in include/asm-generic/io.h
> > (generic) and lib/iomap.c ("pseudo-generic" for x86).
> >
> > Remove the CONFIG_GENERIC_IOMAP guard around
> > ARCH_WANTS_GENERIC_PCI_IOUNMAP so that configs that set
> > CONFIG_GENERIC_PCI_IOMAP without CONFIG_GENERIC_IOMAP still get the
> > function.
> >
> > Add TODOs for follow-up work on the "generic is not generic but
> > x86-specific"-Problem.
> > ...
>
> > +++ b/drivers/pci/iomap.c
> > @@ -135,44 +135,30 @@ void __iomem *pci_iomap_wc(struct pci_dev
> > *dev, int bar, unsigned long maxlen)
> >  EXPORT_SYMBOL_GPL(pci_iomap_wc);
> >  
> >  /*
> > - * 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.
> > + * This check is still necessary due to legacy reasons.
> >   *
> > - * But if the architecture does not use the generic iomap code,
> > and if
> > - * it has _not_ defined it's own private pci_iounmap function, we
> > define
> > - * it here.
> > - *
> > - * NOTE! This default implementation assumes that if the
> > architecture
> > - * support ioport mapping (HAS_IOPORT_MAP), the ioport mapping
> > will
> > - * be fixed to the range [ PCI_IOBASE, PCI_IOBASE+IO_SPACE_LIMIT
> > [,
> > - * and does not need unmapping with 'ioport_unmap()'.
> > - *
> > - * If you have different rules for your architecture, you need to
> > - * implement your own pci_iounmap() that knows the rules for where
> > - * and how IO vs MEM get mapped.
> > - *
> > - * This code is odd, and the ARCH_HAS/ARCH_WANTS #define logic
> > comes
> > - * from legacy <asm-generic/io.h> header file behavior. In
> > particular,
> > - * it would seem to make sense to do the iounmap(p) for the non-
> > IO-space
> > - * case here regardless, but that's not what the old header file
> > code
> > - * did. Probably incorrectly, but this is meant to be bug-for-bug
> > - * compatible.
>
> Moving this comment update to the patch that adds the ioport_unmap()
> call would make that patch more consistent and simplify this patch.

The bugfix from patch #1 you mean.
I can take care of that when splitting that patch as you suggested


>
> > + * TODO: Have all architectures that provide their own
> > pci_iounmap() provide
> > + * iomem_is_ioport() instead. Remove this #if afterwards.
> >   */
> >  #if defined(ARCH_WANTS_GENERIC_PCI_IOUNMAP)
> >  
> > -void pci_iounmap(struct pci_dev *dev, void __iomem *p)
> > +/**
> > + * pci_iounmap - Unmapp a mapping
> > + * @dev: PCI device the mapping belongs to
> > + * @addr: start address of the mapping
> > + *
> > + * Unmapp a PIO or MMIO mapping.
>
> s/Unmapp/Unmap/ (twice)

OK

>
> > + */
> > +void pci_iounmap(struct pci_dev *dev, void __iomem *addr)
>
> Maybe move the "p" to "addr" rename to the patch that fixes the
> pci_iounmap() #ifdef problem, since that's a trivial change that
> already has to do with handling both PIO and MMIO?  Then this patch
> would be a little more focused.

OK

>
> The kernel-doc addition could possibly also move there since it isn't
> related to the unification.

You mean the one from my devres-patch-series? Or documentation
specifically about pci_iounmap()?

>
> >  {
> > -#ifdef ARCH_HAS_GENERIC_IOPORT_MAP
> > -       uintptr_t start = (uintptr_t) PCI_IOBASE;
> > -       uintptr_t addr = (uintptr_t) p;
> > -
> > -       if (addr >= start && addr < start + IO_SPACE_LIMIT) {
> > -               ioport_unmap(p);
> > +#ifdef CONFIG_HAS_IOPORT_MAP
> > +       if (iomem_is_ioport(addr)) {
> > +               ioport_unmap(addr);
> >                 return;
> >         }
> >  #endif
> > -       iounmap(p);
> > +
> > +       iounmap(addr);
> >  }
>
> > + * If CONFIG_GENERIC_IOMAP is selected and the architecture does
> > NOT provide its
> > + * own version, ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT makes sure that
> > the generic
> > + * version from asm-generic/io.h is NOT used and instead the
> > second "generic"
> > + * version from this file here is used.
> > + *
> > + * There are currently two generic versions because of a difficult
> > cleanup
> > + * process. Namely, the version in lib/iomap.c once was really
> > generic when IA64
> > + * still existed. Today, it's only really used by x86.
> > + *
> > + * TODO: Move this function to x86-specific code.
>
> Some of these TODOs look fairly simple.  Are they actually hard, or
> could they just be done now?

If they were simple from my humble POV I would have implemented them.
The information about the x86-specficity is from Arnd Bergmann, the
header-maintainer.

I myself am not that sure how much work it would be to move the entire
lib/iomap.c file to x86. At least some (possibley "dead") hooks to it
still exist, for example here:
arch/powerpc/platforms/Kconfig # L.189

>
> It seems like implementing iomem_is_ioport() for the other arches
> would be straightforward and if done first, could make this patch
> look
> tidier.

That would be the cleanest solution. But the cleaner you want to be,
the more time you have to spend ;)
I can take another look and see if I could do that with reasonable
effort.
Otherwise I'd go for:

> Or if the TODOs can't be done now, maybe the iomem_is_ioport()
> addition could be done as a separate patch to make the unification
> more obvious.

sic

Thx,
P.

>
> > + */
> > +#if defined(ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT)
> > +bool iomem_is_ioport(void __iomem *addr)
> >  {
> > -       IO_COND(addr, /* nothing */, iounmap(addr));
> > +       unsigned long port = (unsigned long __force)addr;
> > +
> > +       if (port > PIO_OFFSET && port < PIO_RESERVED)
> > +               return true;
> > +
> > +       return false;
> >  }
> > -EXPORT_SYMBOL(pci_iounmap);
> > -#endif /* CONFIG_PCI */
> > +#endif /* ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT */
> > --
> > 2.43.0
> >
>

2024-01-26 14:31:34

by Arnd Bergmann

Subject: Re: [PATCH v5 RESEND 5/5] lib, pci: unify generic pci_iounmap()

On Fri, Jan 26, 2024, at 14:59, Philipp Stanner wrote:
> On Tue, 2024-01-23 at 15:05 -0600, Bjorn Helgaas wrote:
>> On Thu, Jan 11, 2024 at 09:55:40AM +0100, Philipp Stanner wrote:
>>
>> The kernel-doc addition could possibly also move there since it isn't
>> related to the unification.
>
> You mean the one from my devres-patch-series? Or documentation
> specifically about pci_iounmap()?
>
>>
>> >  {
>> > -#ifdef ARCH_HAS_GENERIC_IOPORT_MAP
>> > -       uintptr_t start = (uintptr_t) PCI_IOBASE;
>> > -       uintptr_t addr = (uintptr_t) p;
>> > -
>> > -       if (addr >= start && addr < start + IO_SPACE_LIMIT) {
>> > -               ioport_unmap(p);
>> > +#ifdef CONFIG_HAS_IOPORT_MAP
>> > +       if (iomem_is_ioport(addr)) {
>> > +               ioport_unmap(addr);
>> >                 return;
>> >         }
>> >  #endif
>> > -       iounmap(p);
>> > +
>> > +       iounmap(addr);
>> >  }
>>
>> > + * If CONFIG_GENERIC_IOMAP is selected and the architecture does
>> > NOT provide its
>> > + * own version, ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT makes sure that
>> > the generic
>> > + * version from asm-generic/io.h is NOT used and instead the
>> > second "generic"
>> > + * version from this file here is used.
>> > + *
>> > + * There are currently two generic versions because of a difficult
>> > cleanup
>> > + * process. Namely, the version in lib/iomap.c once was really
>> > generic when IA64
>> > + * still existed. Today, it's only really used by x86.
>> > + *
>> > + * TODO: Move this function to x86-specific code.
>>
>> Some of these TODOs look fairly simple.  Are they actually hard, or
>> could they just be done now?
>
> If they were simple from my humble POV I would have implemented them.
> The information about the x86-specficity is from Arnd Bergmann, the
> header-maintainer.
>
> I myself am not that sure how much work it would be to move the entire
> lib/iomap.c file to x86. At least some (possibley "dead") hooks to it
> still exist, for example here:
> arch/powerpc/platforms/Kconfig # L.189

This one definitely takes some work to untangle, it's selected
by two powerpc platforms, but one doesn't actually need it any
more, and the other one uses it only for non-PCI devices.

I think the other architectures are easier to change and do
fix real bugs, but it's probably best done one at a time.

Arnd

2024-01-27 22:39:39

by Bjorn Helgaas

Subject: Re: [PATCH v5 RESEND 5/5] lib, pci: unify generic pci_iounmap()

On Fri, Jan 26, 2024 at 02:59:20PM +0100, Philipp Stanner wrote:
> On Tue, 2024-01-23 at 15:05 -0600, Bjorn Helgaas wrote:
> > On Thu, Jan 11, 2024 at 09:55:40AM +0100, Philipp Stanner wrote:
> ...

> > > -void pci_iounmap(struct pci_dev *dev, void __iomem *p)
> > > +/**
> > > + * pci_iounmap - Unmapp a mapping
> > > + * @dev: PCI device the mapping belongs to
> > > + * @addr: start address of the mapping
> > > + *
> > > + * Unmapp a PIO or MMIO mapping.
> > > + */
> > > +void pci_iounmap(struct pci_dev *dev, void __iomem *addr)
> >
> > Maybe move the "p" to "addr" rename to the patch that fixes the
> > pci_iounmap() #ifdef problem, since that's a trivial change that
> > already has to do with handling both PIO and MMIO?  Then this patch
> > would be a little more focused.
> >
> > The kernel-doc addition could possibly also move there since it isn't
> > related to the unification.
>
> You mean the one from my devres-patch-series? Or documentation
> specifically about pci_iounmap()?

I had in mind the patch that fixes the pci_iounmap() #ifdef problem,
which (if you split it out from 1/5) would be a relatively trivial
patch. Or the kernel-doc addition could be its own separate patch.
The point is that this unification patch is fairly complicated, so
anything we can do to move things unrelated to unification elsewhere
makes this one easier to review.

> > It seems like implementing iomem_is_ioport() for the other arches
> > would be straightforward and if done first, could make this patch
> > look
> > tidier.
>
> That would be the cleanest solution. But the cleaner you want to be,
> the more time you have to spend ;)
> I can take another look and see if I could do that with reasonable
> effort.
> Otherwise I'd go for:
>
> > Or if the TODOs can't be done now, maybe the iomem_is_ioport()
> > addition could be done as a separate patch to make the unification
> > more obvious.

It looks like iomem_is_ioport() is basically the guards in
pci_iounmap() implementations that, if true, prevent calling
iounmap(), so it it seems like they should be trivial, e.g.,

return !__is_mmio(addr); # alpha

return (addr < VMALLOC_START || addr >= VMALLOC_END); # arm

return isa_vaddr_is_ioport(addr) || pcibios_vaddr_is_ioport(addr); # microblaze

Unless they're significantly more complicated than that, I don't see
the point of deferring them.

> > > + */
> > > +#if defined(ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT)
> > > +bool iomem_is_ioport(void __iomem *addr)
> > >  {
> > > -       IO_COND(addr, /* nothing */, iounmap(addr));
> > > +       unsigned long port = (unsigned long __force)addr;
> > > +
> > > +       if (port > PIO_OFFSET && port < PIO_RESERVED)
> > > +               return true;
> > > +
> > > +       return false;
> > >  }
> > > -EXPORT_SYMBOL(pci_iounmap);
> > > -#endif /* CONFIG_PCI */
> > > +#endif /* ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT */
> > > --
> > > 2.43.0
> > >
> >
>

2024-01-29 10:44:02

by Philipp Stanner

Subject: Re: [PATCH v5 RESEND 5/5] lib, pci: unify generic pci_iounmap()

On Sat, 2024-01-27 at 16:39 -0600, Bjorn Helgaas wrote:
> On Fri, Jan 26, 2024 at 02:59:20PM +0100, Philipp Stanner wrote:
> > On Tue, 2024-01-23 at 15:05 -0600, Bjorn Helgaas wrote:
> > > On Thu, Jan 11, 2024 at 09:55:40AM +0100, Philipp Stanner wrote:
> > ...
>
> > > > -void pci_iounmap(struct pci_dev *dev, void __iomem *p)
> > > > +/**
> > > > + * pci_iounmap - Unmapp a mapping
> > > > + * @dev: PCI device the mapping belongs to
> > > > + * @addr: start address of the mapping
> > > > + *
> > > > + * Unmapp a PIO or MMIO mapping.
> > > > + */
> > > > +void pci_iounmap(struct pci_dev *dev, void __iomem *addr)
> > >
> > > Maybe move the "p" to "addr" rename to the patch that fixes the
> > > pci_iounmap() #ifdef problem, since that's a trivial change that
> > > already has to do with handling both PIO and MMIO?  Then this
> > > patch
> > > would be a little more focused.
> > >
> > > The kernel-doc addition could possibly also move there since it
> > > isn't
> > > related to the unification.
> >
> > You mean the one from my devres-patch-series? Or documentation
> > specifically about pci_iounmap()?
>
> I had in mind the patch that fixes the pci_iounmap() #ifdef problem,
> which (if you split it out from 1/5) would be a relatively trivial
> patch.  Or the kernel-doc addition could be its own separate patch.
> The point is that this unification patch is fairly complicated, so
> anything we can do to move things unrelated to unification elsewhere
> makes this one easier to review.

I think it should be a separate patch, then, as it doesn't belong by
100% to any of the patches here. If I had to pick one, I'd have
included the docu into patch #2 or #3.

Let's make it a separate one, following as a 6th patch in this series

>
> > > It seems like implementing iomem_is_ioport() for the other arches
> > > would be straightforward and if done first, could make this patch
> > > look
> > > tidier.
> >
> > That would be the cleanest solution. But the cleaner you want to
> > be,
> > the more time you have to spend ;)
> > I can take another look and see if I could do that with reasonable
> > effort.
> > Otherwise I'd go for:
> >
> > > Or if the TODOs can't be done now, maybe the iomem_is_ioport()
> > > addition could be done as a separate patch to make the
> > > unification
> > > more obvious.
>
> It looks like iomem_is_ioport() is basically the guards in
> pci_iounmap() implementations that, if true, prevent calling
> iounmap(), so it it seems like they should be trivial, e.g.,
>
>   return !__is_mmio(addr); # alpha
>
>   return (addr < VMALLOC_START || addr >= VMALLOC_END); # arm
>
>   return isa_vaddr_is_ioport(addr) || pcibios_vaddr_is_ioport(addr);
> # microblaze
>
> Unless they're significantly more complicated than that, I don't see
> the point of deferring them.

Have you seen Arnd's reply from Friday?
Cleaning up Powerpc's use of lib/iomap.c will be significantly more
complicated.

This series' purpose actually always has been to move PCI functions to
where they belong, i.e. from lib/ to drivers/pci.
I originally didn't want to touch pci_iounmap(), since I deemed it too
complicated. Arnd pushed for unifying it.

Anyways, investing much more time into this is beyond my time budget. I
only started this series to have a cleaner basis to do the devres
functions.

So my suggestions is that we either go with this cleanup here, which
improves the situation at least somewhat, or we simply drop patch #5
and leave pci_iounmap() as the last pci_ function in lib/


P.

>
> > > > + */
> > > > +#if defined(ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT)
> > > > +bool iomem_is_ioport(void __iomem *addr)
> > > >  {
> > > > -       IO_COND(addr, /* nothing */, iounmap(addr));
> > > > +       unsigned long port = (unsigned long __force)addr;
> > > > +
> > > > +       if (port > PIO_OFFSET && port < PIO_RESERVED)
> > > > +               return true;
> > > > +
> > > > +       return false;
> > > >  }
> > > > -EXPORT_SYMBOL(pci_iounmap);
> > > > -#endif /* CONFIG_PCI */
> > > > +#endif /* ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT */
> > > > --
> > > > 2.43.0
> > > >
> > >
> >
>

2024-01-29 18:14:56

by Bjorn Helgaas

Subject: Re: [PATCH v5 RESEND 5/5] lib, pci: unify generic pci_iounmap()

On Mon, Jan 29, 2024 at 11:43:34AM +0100, Philipp Stanner wrote:
> On Sat, 2024-01-27 at 16:39 -0600, Bjorn Helgaas wrote:
> > On Fri, Jan 26, 2024 at 02:59:20PM +0100, Philipp Stanner wrote:
> > > On Tue, 2024-01-23 at 15:05 -0600, Bjorn Helgaas wrote:
> > > > On Thu, Jan 11, 2024 at 09:55:40AM +0100, Philipp Stanner wrote:
> > > ...
> >
> > > > > -void pci_iounmap(struct pci_dev *dev, void __iomem *p)
> > > > > +/**
> > > > > + * pci_iounmap - Unmapp a mapping
> > > > > + * @dev: PCI device the mapping belongs to
> > > > > + * @addr: start address of the mapping
> > > > > + *
> > > > > + * Unmapp a PIO or MMIO mapping.
> > > > > + */
> > > > > +void pci_iounmap(struct pci_dev *dev, void __iomem *addr)
> > > >
> > > > Maybe move the "p" to "addr" rename to the patch that fixes the
> > > > pci_iounmap() #ifdef problem, since that's a trivial change that
> > > > already has to do with handling both PIO and MMIO?  Then this
> > > > patch
> > > > would be a little more focused.
> > > >
> > > > The kernel-doc addition could possibly also move there since it
> > > > isn't
> > > > related to the unification.
> > >
> > > You mean the one from my devres-patch-series? Or documentation
> > > specifically about pci_iounmap()?
> >
> > I had in mind the patch that fixes the pci_iounmap() #ifdef problem,
> > which (if you split it out from 1/5) would be a relatively trivial
> > patch.  Or the kernel-doc addition could be its own separate patch.
> > The point is that this unification patch is fairly complicated, so
> > anything we can do to move things unrelated to unification elsewhere
> > makes this one easier to review.
>
> I think it should be a separate patch, then, as it doesn't belong by
> 100% to any of the patches here. If I had to pick one, I'd have
> included the docu into patch #2 or #3.
>
> Let's make it a separate one, following as a 6th patch in this series

Sounds good.

> > > > It seems like implementing iomem_is_ioport() for the other arches
> > > > would be straightforward and if done first, could make this patch
> > > > look
> > > > tidier.
> > >
> > > That would be the cleanest solution. But the cleaner you want to
> > > be,
> > > the more time you have to spend ;)
> > > I can take another look and see if I could do that with reasonable
> > > effort.
> > > Otherwise I'd go for:
> > >
> > > > Or if the TODOs can't be done now, maybe the iomem_is_ioport()
> > > > addition could be done as a separate patch to make the
> > > > unification
> > > > more obvious.
> >
> > It looks like iomem_is_ioport() is basically the guards in
> > pci_iounmap() implementations that, if true, prevent calling
> > iounmap(), so it it seems like they should be trivial, e.g.,
> >
> >   return !__is_mmio(addr); # alpha
> >
> >   return (addr < VMALLOC_START || addr >= VMALLOC_END); # arm
> >
> >   return isa_vaddr_is_ioport(addr) || pcibios_vaddr_is_ioport(addr);
> > # microblaze
> >
> > Unless they're significantly more complicated than that, I don't see
> > the point of deferring them.

> ...
> This series' purpose actually always has been to move PCI functions to
> where they belong, i.e. from lib/ to drivers/pci.
> I originally didn't want to touch pci_iounmap(), since I deemed it too
> complicated. Arnd pushed for unifying it.
>
> Anyways, investing much more time into this is beyond my time budget. I
> only started this series to have a cleaner basis to do the devres
> functions.
>
> So my suggestions is that we either go with this cleanup here, which
> improves the situation at least somewhat, or we simply drop patch #5
> and leave pci_iounmap() as the last pci_ function in lib/

OK, let's drop #5 for now. It definitely seems like where we should
go in the future, but I think it will make more sense when we can
include a few of the simple conversions that will show how it all fits
together.

Bjorn