Subject: [PATCH 0/3] arch: fix ioport mapping on mips,sh

From: "Michael S. Tsirkin" <[email-masked]>
Date: Mon, 30 Jan 2012 14:18:25 +0200

Kevin Cernekee reported that recent cleanup
that replaced pci_iomap with a generic function
failed to take into account the differences
in io port handling on mips and sh architectures.

Rather than revert the changes reintroducing the
code duplication, this patchset fixes this
by adding ability for architectures to override
ioport mapping for pci devices.

I put this in my tree that feeds into linux-next
and intend to ask Linus to pull this fix if this
doesn't cause any issues and there are no objections.

The patches were tested on x86 and compiled on mips and sh.
Would appreciate reviews/acks/testing reports.

Michael S. Tsirkin (3):
  lib: add NO_GENERIC_PCI_IOPORT_MAP
  mips: use the the PCI controller's io_map_base
  sh: use the the PCI channels's io_map_base

 arch/mips/Kconfig               |    1 +
 arch/mips/lib/iomap-pci.c       |    4 ++--
 arch/sh/Kconfig                 |    1 +
 arch/sh/drivers/pci/pci.c       |    4 ++--
 include/asm-generic/pci_iomap.h |    5 +++++
 lib/Kconfig                     |    3 +++
 lib/pci_iomap.c                 |   12 +++++++++++-
 7 files changed, 25 insertions(+), 5 deletions(-)

-- 
1.7.8.2.325.g247f9

---

From: "Michael S. Tsirkin" <[email-masked]>
Date: Mon, 30 Jan 2012 14:18:39 +0200

Some architectures need to override the way
IO port mapping is does not PCI devices.
Supply a generic function that calls
ioport_map, and make it possible for architectures
to override.

Signed-off-by: Michael S. Tsirkin <[email-masked]>
---
 include/asm-generic/pci_iomap.h |    5 +++++
 lib/Kconfig                     |    3 +++
 lib/pci_iomap.c                 |   12 +++++++++++-
 3 files changed, 19 insertions(+), 1 deletions(-)

diff --git a/include/asm-generic/pci_iomap.h b/include/asm-generic/pci_iomap.h
index 8de4b73..2aff58e 100644
--- a/include/asm-generic/pci_iomap.h
+++ b/include/asm-generic/pci_iomap.h
@@ -15,6 +15,11 @@ struct pci_dev;
 #ifdef CONFIG_PCI
 /* Create a virtual mapping cookie for a PCI BAR (memory or IO) */
 extern void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long max);
+/* Create a virtual mapping cookie for a port on a given PCI device.
+ * Do not call this directly, it exists to make it easier for architectures
+ * to override. */
+extern void __iomem *__pci_ioport_map(struct pci_dev *dev, unsigned long port,
+				      unsigned int nr);
 #else
 static inline void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long max)
 {
diff --git a/lib/Kconfig b/lib/Kconfig
index 169eb7c..1df1388 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -19,6 +19,9 @@ config RATIONAL
 config GENERIC_FIND_FIRST_BIT
 	bool
 
+config NO_GENERIC_PCI_IOPORT_MAP
+	bool
+
 config GENERIC_PCI_IOMAP
 	bool
 
diff --git a/lib/pci_iomap.c b/lib/pci_iomap.c
index 4b0fdc2..1dfda29 100644
--- a/lib/pci_iomap.c
+++ b/lib/pci_iomap.c
@@ -9,6 +9,16 @@
 #include <linux/export.h>
 
 #ifdef CONFIG_PCI
+#ifndef CONFIG_NO_GENERIC_PCI_IOPORT_MAP
+/* Architectures can override ioport mapping while
+ * still using the rest of the generic infrastructure. */
+void __iomem *__pci_ioport_map(struct pci_dev *dev,
+			       unsigned long port,
+			       unsigned int nr)
+{
+	return ioport_map(port, nr);
+}
+#endif
 /**
  * pci_iomap - create a virtual mapping cookie for a PCI BAR
  * @dev: PCI device that owns the BAR
@@ -34,7 +44,7 @@ void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long maxlen)
 	if (maxlen && len > maxlen)
 		len = maxlen;
 	if (flags & IORESOURCE_IO)
-		return ioport_map(start, len);
+		return __pci_ioport_map(dev, start, len);
 	if (flags & IORESOURCE_MEM) {
 		if (flags & IORESOURCE_CACHEABLE)
 			return ioremap(start, len);
-- 
1.7.8.2.325.g247f9

---

From: "Michael S. Tsirkin" <[email-masked]>
Date: Mon, 30 Jan 2012 14:18:53 +0200

commit eab90291d35438bcebf7c3dc85be66d0f24e3002
failed to take into account the PCI controller's
io_map_base for mapping IO BARs.
This also caused a new warning on mips.

Fix this, without re-introducing code duplication,
by setting NO_GENERIC_PCI_IOPORT_MAP
and supplying a mips-specific __pci_ioport_map.

Reported-by: Kevin Cernekee <[email-masked]>
Signed-off-by: Michael S. Tsirkin <[email-masked]>
---
 arch/mips/Kconfig         |    1 +
 arch/mips/lib/iomap-pci.c |    4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
index c4c1312..5ab6e89 100644
--- a/arch/mips/Kconfig
+++ b/arch/mips/Kconfig
@@ -2356,6 +2356,7 @@ config PCI
 	depends on HW_HAS_PCI
 	select PCI_DOMAINS
 	select GENERIC_PCI_IOMAP
+	select NO_GENERIC_PCI_IOPORT_MAP
 	help
 	  Find out whether you have a PCI motherboard. PCI is the name of a
 	  bus system, i.e. the way the CPU talks to the other stuff inside
diff --git a/arch/mips/lib/iomap-pci.c b/arch/mips/lib/iomap-pci.c
index 2635b1a..fd35daa 100644
--- a/arch/mips/lib/iomap-pci.c
+++ b/arch/mips/lib/iomap-pci.c
@@ -10,8 +10,8 @@
 #include <linux/module.h>
 #include <asm/io.h>
 
-static void __iomem *ioport_map_pci(struct pci_dev *dev,
-                                     unsigned long port, unsigned int nr)
+void __iomem *__pci_ioport_map(struct pci_dev *dev,
+			       unsigned long port, unsigned int nr)
 {
 	struct pci_controller *ctrl = dev->bus->sysdata;
 	unsigned long base = ctrl->io_map_base;
-- 
1.7.8.2.325.g247f9

---

From: "Michael S. Tsirkin" <[email-masked]>
Date: Mon, 30 Jan 2012 14:19:05 +0200

commit 43db595e8b5d78ce5ad2feab719814a76e3ad2e5
failed to take into account the PCI channels's
io_map_base for mapping IO BARs.
This also caused a new warning on sh.

Fix this, without re-introducing code duplication,
by setting NO_GENERIC_PCI_IOPORT_MAP
and supplying a sh-specific __pci_ioport_map.

diff --git a/arch/sh/Kconfig b/arch/sh/Kconfig
index 3c8db65..713fb58 100644
--- a/arch/sh/Kconfig
+++ b/arch/sh/Kconfig
@@ -859,6 +859,7 @@ config PCI
 	depends on SYS_SUPPORTS_PCI
 	select PCI_DOMAINS
 	select GENERIC_PCI_IOMAP
+	select NO_GENERIC_PCI_IOPORT_MAP
 	help
 	  Find out whether you have a PCI motherboard. PCI is the name of a
 	  bus system, i.e. the way the CPU talks to the other stuff inside
diff --git a/arch/sh/drivers/pci/pci.c b/arch/sh/drivers/pci/pci.c
index 8f18dd0..1e7b0e2 100644
--- a/arch/sh/drivers/pci/pci.c
+++ b/arch/sh/drivers/pci/pci.c
@@ -356,8 +356,8 @@ int pci_mmap_page_range(struct pci_dev *dev, struct vm_area_struct *vma,
 
 #ifndef CONFIG_GENERIC_IOMAP
 
 	struct pci_channel *chan = dev->sysdata;
 
-- 
1.7.8.2.325.g247f9

---

From: Shane McDonald <[email-masked]>
Date: Mon, 30 Jan 2012 08:19:31 -0600

Just a minor nit on the comment:

> IO port mapping is does not PCI devices.

Should this line read "IO port mapping is done on PCI devices."?

> Signed-off-by: Michael S. Tsirkin <[email-masked]>

Shane

---

From: "Michael S. Tsirkin" <[email-masked]>
Date: Mon, 30 Jan 2012 16:25:57 +0200

> Should this line read "IO port mapping is done on PCI devices."?

Right, good catch. I'll fix this up in git.

---

From: Arnd Bergmann <[email-masked]>
Date: Mon, 30 Jan 2012 15:51:46 +0000

> index 4b0fdc2..1dfda29 100644

This looks correct, but it would be nicer to express this with an inline
function and keeping the new #ifdef to the header file, like

+/*
+ * Create a virtual mapping cookie for a port on a given PCI device.
+ * Do not call this directly, it exists to make it easier for architectures
+ * to override.
+ */
+#ifdef CONFIG_NO_GENERIC_PCI_IOPORT_MAP
+extern void __iomem *__pci_ioport_map(struct pci_dev *dev, unsigned long port,
+                                     unsigned int nr);
+#else
+static inline void __iomem *__pci_ioport_map(struct pci_dev *dev,
+                              unsigned long port, unsigned int nr)
+{
+       return ioport_map(port, nr);
+}
+#endif

	Arnd

---

From: Arnd Bergmann <[email-masked]>
Date: Mon, 30 Jan 2012 16:09:55 +0000

> Would appreciate reviews/acks/testing reports.

Looks good to me, except for the one detail I've commented
on in the third patch.

Acked-by: Arnd Bergmann <[email-masked]>

I do wonder if the sh and mips implementations are robust enough however
(independent of your work): It seems that an ioport number is treated
differently in pci_iomap than it is using ioport_map and inb/outb,
the assumption being that the port number is a local index per PCI domain.
This would mean that any port access other than pci_iomap would only
work on the primary PCI domain. There are two IMHO better solutions that
I've seen on other architectures:

1. create a larger (e.g. 1MB) io port mapping range in virtual memory
that is split into 64kb per domain, and use the domain number to
find the per domain range when setting the resources. Port numbers will
be larger than 65535 this way, but PCI will ignore the upper address
bits for any access so it works fine.

2. split the 64kb io port range into subsections per domain (on page
granularity, e.g. 2 domains with 32kb or at most 16 domains with 4kb)
and map them virtually contiguous, then reassign all io port resources
so that only the correct region for each domain is used.

	Arnd

---

From: "Michael S. Tsirkin" <[email-masked]>
Date: Mon, 30 Jan 2012 18:18:19 +0200

> +       return ioport_map(port, nr);

It would be nicer in that it would
make the kernel a bit smaller for generic architectures
but this would need to go into a separate header:
it depends on io.h and io.h depends on pci_iomap.h.

Worth it?

-- 
MST

---

From: Sergei Shtylyov <[email-masked]>
Date: Mon, 30 Jan 2012 20:49:09 +0300

Hello.

> commit eab90291d35438bcebf7c3dc85be66d0f24e3002

    Please add that commit's summary in parens.

> Signed-off-by: Michael S. Tsirkin <[email-masked]>

WBR, Sergei

---

From: Sergei Shtylyov <[email-masked]>
Date: Mon, 30 Jan 2012 20:50:45 +0300

Hello.

> Reported-by: Kevin Cernekee<[email-masked]>
> Signed-off-by: Michael S. Tsirkin<[email-masked]>

WBR, Sergei

---

From: Arnd Bergmann <[email-masked]>
Date: Mon, 30 Jan 2012 20:04:32 +0000

> it depends on io.h and io.h depends on pci_iomap.h.

Adding extra dependencies is not good here, I agree.
Maybe  a better solution is to use a macro instead of an inline
function then:

#define  __pci_ioport_map(dev, port, nr) ioport_map(port, nr)

In general, macros should be avoided, but I think it's the
best tradeoff in this case.

	Arnd

---

From: "Michael S. Tsirkin" <[email-masked]>
Date: Tue, 31 Jan 2012 02:22:05 +0200

> best tradeoff in this case.

I have an idea: we can make the generic one inline
if we keep it in the .c file. So something like
the below on top of my patch will probably work.
Ack?

diff --git a/include/asm-generic/pci_iomap.h b/include/asm-generic/pci_iomap.h
index 2aff58e..2ec1bdb 100644
--- a/include/asm-generic/pci_iomap.h
+++ b/include/asm-generic/pci_iomap.h
@@ -15,11 +15,6 @@ struct pci_dev;
 #ifdef CONFIG_PCI
 /* Create a virtual mapping cookie for a PCI BAR (memory or IO) */
 extern void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long max);
-/* Create a virtual mapping cookie for a port on a given PCI device.
- * Do not call this directly, it exists to make it easier for architectures
- * to override. */
-extern void __iomem *__pci_ioport_map(struct pci_dev *dev, unsigned long port,
-				      unsigned int nr);
 #else
 static inline void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long max)
 {
@@ -27,4 +22,12 @@ static inline void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned lon
 }
 #endif
 
+#ifdef CONFIG_NO_GENERIC_PCI_IOPORT_MAP
+/* Create a virtual mapping cookie for a port on a given PCI device.
+ * Do not call this directly, it exists to make it easier for architectures
+ * to override. */
+extern void __iomem *__pci_ioport_map(struct pci_dev *dev, unsigned long port,
+				      unsigned int nr);
+#endif
+
 #endif /* __ASM_GENERIC_IO_H */
diff --git a/lib/pci_iomap.c b/lib/pci_iomap.c
index 1dfda29..8102f28 100644
--- a/lib/pci_iomap.c
+++ b/lib/pci_iomap.c
@@ -12,9 +12,9 @@
 #ifndef CONFIG_NO_GENERIC_PCI_IOPORT_MAP
 /* Architectures can override ioport mapping while
  * still using the rest of the generic infrastructure. */
-void __iomem *__pci_ioport_map(struct pci_dev *dev,
-			       unsigned long port,
-			       unsigned int nr)
+static inline void __iomem *__pci_ioport_map(struct pci_dev *dev,
+					     unsigned long port,
+					     unsigned int nr)
 {
 	return ioport_map(port, nr);
 }

---

From: Arnd Bergmann <[email-masked]>
Date: Tue, 31 Jan 2012 15:59:25 +0000

> the below on top of my patch will probably work.

IMHO this is still worse than the macro, because it breaks common practice.
The common way to do this is #ifdef/#else/#endif in the header file to
provide either an extern or a macro/inline definition, while having the
inline definition in a separate place makes it harder to understand
what's going on. E.g. a frequent review comment is to not put extern
declarations inside of #ifdef, but if someone tries that here, it would
break.

You also still need the #ifdef in the implementation file, which we
try to avoid normally just like we try to avoid macros where possible.

	Arnd

---

From: "Michael S. Tsirkin" <[email-masked]>
Date: Tue, 31 Jan 2012 23:18:54 +0200

> best tradeoff in this case.

OK, I did exactly that. Thanks!