Skip to contentSkip to conversation

arch: fix ioport mapping on mips,sh

PATCH In review Michael S. Tsirkin 4 participants 15 messages Jan 30, 2012 – Jan 31, 2012

Subsystems (5)
PCI Library code MIPS SuperH +1 more subsystems
lore
Share & tools
lore

Michael S. Tsirkin

author
#0 lore raw

cover arch: fix ioport mapping on mips,sh

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(-)
Signature · 1 line
1.7.8.2.325.g247f9
author
#1 lore raw

PATCH 1/3 lib: add NO_GENERIC_PCI_IOPORT_MAP

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 protected]>
---
 include/asm-generic/pci_iomap.h |    5 +++++
 lib/Kconfig                     |    3 +++
 lib/pci_iomap.c                 |   12 +++++++++++-
 3 files changed, 19 insertions(+), 1 deletions(-)
include/asm-generic/pci_iomap.h +5 -0
index 8de4b73..2aff58e 100644
@@ -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)
 {
lib/Kconfig +3 -0
index 169eb7c..1df1388 100644
@@ -19,6 +19,9 @@ config RATIONAL
 config GENERIC_FIND_FIRST_BIT
 	bool
 
+config NO_GENERIC_PCI_IOPORT_MAP
+	bool
+
 config GENERIC_PCI_IOMAP
 	bool
lib/pci_iomap.c +11 -1
index 4b0fdc2..1dfda29 100644
@@ -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);
Signature · 1 line
1.7.8.2.325.g247f9
#2 lore raw

Re: [PATCH 1/3] lib: add NO_GENERIC_PCI_IOPORT_MAP

Just a minor nit on the comment:

Quoted 2 lines · Michael S. Tsirkin

Some architectures need to override the way
IO port mapping is does not PCI devices.

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

Quoted 5 lines

Supply a generic function that calls ioport_map, and make it possible for architectures to override.

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

Shane

author
#3 lore raw

Re: [PATCH 1/3] lib: add NO_GENERIC_PCI_IOPORT_MAP

Quoted 7 lines · Shane McDonald

Just a minor nit on the comment:

Quoted 2 lines · Michael S. Tsirkin

Some architectures need to override the way
IO port mapping is does not PCI devices.

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

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

Quoted 7 lines
Quoted 5 lines

Supply a generic function that calls ioport_map, and make it possible for architectures to override.

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

Shane

maintainer
#4 lore raw

Re: [PATCH 1/3] lib: add NO_GENERIC_PCI_IOPORT_MAP

In reply to Michael S. Tsirkin
Quoted 24 lines · Michael S. Tsirkin
include/asm-generic/pci_iomap.h +5 -0
@@ -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)
 {

index 4b0fdc2..1dfda29 100644

lib/pci_iomap.c +3 -0
@@ -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. */

lib/pci_iomap.c line 24

Quoted 10 lines
lib/pci_iomap.c +7 -0
+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

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

author
#5 lore raw

Re: [PATCH 1/3] lib: add NO_GENERIC_PCI_IOPORT_MAP

Quoted 55 lines · Arnd Bergmann
Quoted 24 lines · Michael S. Tsirkin
include/asm-generic/pci_iomap.h +5 -0
@@ -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)
 {

index 4b0fdc2..1dfda29 100644

lib/pci_iomap.c +3 -0
@@ -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. */

lib/pci_iomap.c line 24

Quoted 10 lines
lib/pci_iomap.c +7 -0
+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

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

(quoted diff) +15 -0
+/*
+ * 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
Quoted 1 line

Arnd

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?

Signature · 1 line
MST
maintainer
#6 lore raw

Re: [PATCH 1/3] lib: add NO_GENERIC_PCI_IOPORT_MAP

Quoted 17 lines · Michael S. Tsirkin
Quoted 17 lines
(quoted diff) +15 -0
+/*
+ * 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
Quoted 6 lines
Quoted 1 line

Arnd

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.

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

author
#7 lore raw

Re: [PATCH 1/3] lib: add NO_GENERIC_PCI_IOPORT_MAP

Quoted 29 lines · Arnd Bergmann
Quoted 17 lines · Michael S. Tsirkin
Quoted 17 lines
(quoted diff) +15 -0
+/*
+ * 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
Quoted 6 lines
Quoted 1 line

Arnd

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.

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

Quoted 6 lines

#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

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?

include/asm-generic/pci_iomap.h +8 -5
index 2aff58e..2ec1bdb 100644
@@ -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 */
lib/pci_iomap.c +3 -3
index 1dfda29..8102f28 100644
@@ -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);
 }
maintainer
#8 lore raw

Re: [PATCH 1/3] lib: add NO_GENERIC_PCI_IOPORT_MAP

Quoted 4 lines · Michael S. Tsirkin

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?

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

author
#9 lore raw

Re: [PATCH 1/3] lib: add NO_GENERIC_PCI_IOPORT_MAP

In reply to Arnd Bergmann
Quoted 29 lines · Arnd Bergmann
Quoted 17 lines · Michael S. Tsirkin
Quoted 17 lines
(quoted diff) +15 -0
+/*
+ * 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
Quoted 6 lines
Quoted 1 line

Arnd

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.

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

Quoted 6 lines

#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

OK, I did exactly that. Thanks!

author
#10 lore raw

PATCH 2/3 mips: use the the PCI controller's io_map_base

In reply to Michael S. Tsirkin

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 protected]>
Signed-off-by Michael S. Tsirkin <[email protected]>
---
 arch/mips/Kconfig         |    1 +
 arch/mips/lib/iomap-pci.c |    4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)
arch/mips/Kconfig +1 -0
index c4c1312..5ab6e89 100644
@@ -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
arch/mips/lib/iomap-pci.c +2 -2
index 2635b1a..fd35daa 100644
@@ -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;
Signature · 1 line
1.7.8.2.325.g247f9
#11 lore raw

Re: [PATCH 2/3] mips: use the the PCI controller's io_map_base

Hello.

Quoted 1 line · Michael S. Tsirkin

commit eab90291d35438bcebf7c3dc85be66d0f24e3002

Please add that commit's summary in parens.

Quoted 10 lines

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 protected]>
Signed-off-by Michael S. Tsirkin <[email protected]>

WBR, Sergei

author
#12 lore raw

PATCH 3/3 sh: use the the PCI channels's io_map_base

In reply to Michael S. Tsirkin

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.

Reported-by Kevin Cernekee <[email protected]>
Signed-off-by Michael S. Tsirkin <[email protected]>
---
 arch/sh/Kconfig           |    1 +
 arch/sh/drivers/pci/pci.c |    4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)
arch/sh/Kconfig +1 -0
index 3c8db65..713fb58 100644
@@ -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
arch/sh/drivers/pci/pci.c +2 -2
index 8f18dd0..1e7b0e2 100644
@@ -356,8 +356,8 @@ int pci_mmap_page_range(struct pci_dev *dev, struct vm_area_struct *vma,
 
 #ifndef CONFIG_GENERIC_IOMAP
 
-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_channel *chan = dev->sysdata;
Signature · 1 line
1.7.8.2.325.g247f9
#13 lore raw

Re: [PATCH 3/3] sh: use the the PCI channels's io_map_base

Hello.

Quoted 1 line · Michael S. Tsirkin

commit 43db595e8b5d78ce5ad2feab719814a76e3ad2e5

Please add that commit's summary in parens.

Quoted 10 lines

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.

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

WBR, Sergei

maintainer Acked-by on the v1 series
#14 lore raw
In reply to Michael S. Tsirkin
Quoted 10 lines · Michael S. Tsirkin

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.

Quoted 6 lines

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.

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

Acked-by Arnd Bergmann <[email protected]>

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