linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] arch: fix ioport mapping on mips,sh
@ 2012-01-30 12:18 Michael S. Tsirkin
  2012-01-30 12:18 ` [PATCH 1/3] lib: add NO_GENERIC_PCI_IOPORT_MAP Michael S. Tsirkin
                   ` (3 more replies)
  0 siblings, 4 replies; 15+ messages in thread
From: Michael S. Tsirkin @ 2012-01-30 12:18 UTC (permalink / raw)
  To: Kevin Cernekee
  Cc: Ralf Baechle, Paul Mundt, Arnd Bergmann, Michael S. Tsirkin,
	Jesse Barnes, Myron Stowe, Paul Gortmaker, Lucas De Marchi,
	Dmitry Kasatkin, James Morris, John W. Linville, Michael Witten,
	linux-mips, linux-kernel, linux-sh, linux-arch

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/3] lib: add NO_GENERIC_PCI_IOPORT_MAP
  2012-01-30 12:18 [PATCH 0/3] arch: fix ioport mapping on mips,sh Michael S. Tsirkin
@ 2012-01-30 12:18 ` Michael S. Tsirkin
  2012-01-30 14:19   ` Shane McDonald
  2012-01-30 15:51   ` Arnd Bergmann
  2012-01-30 12:18 ` [PATCH 2/3] mips: use the the PCI controller's io_map_base Michael S. Tsirkin
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 15+ messages in thread
From: Michael S. Tsirkin @ 2012-01-30 12:18 UTC (permalink / raw)
  To: Kevin Cernekee
  Cc: Ralf Baechle, Paul Mundt, Arnd Bergmann, Michael S. Tsirkin,
	Jesse Barnes, Myron Stowe, Paul Gortmaker, Lucas De Marchi,
	Dmitry Kasatkin, James Morris, John W. Linville, Michael Witten,
	linux-mips, linux-kernel, linux-sh, linux-arch

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 <mst@redhat.com>
---
 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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 2/3] mips: use the the PCI controller's io_map_base
  2012-01-30 12:18 [PATCH 0/3] arch: fix ioport mapping on mips,sh Michael S. Tsirkin
  2012-01-30 12:18 ` [PATCH 1/3] lib: add NO_GENERIC_PCI_IOPORT_MAP Michael S. Tsirkin
@ 2012-01-30 12:18 ` Michael S. Tsirkin
  2012-01-30 17:49   ` Sergei Shtylyov
  2012-01-30 12:19 ` [PATCH 3/3] sh: use the the PCI channels's io_map_base Michael S. Tsirkin
  2012-01-30 16:09 ` [PATCH 0/3] arch: fix ioport mapping on mips,sh Arnd Bergmann
  3 siblings, 1 reply; 15+ messages in thread
From: Michael S. Tsirkin @ 2012-01-30 12:18 UTC (permalink / raw)
  To: Kevin Cernekee
  Cc: Ralf Baechle, Paul Mundt, Arnd Bergmann, Michael S. Tsirkin,
	Jesse Barnes, Myron Stowe, Paul Gortmaker, Lucas De Marchi,
	Dmitry Kasatkin, James Morris, John W. Linville, Michael Witten,
	linux-mips, linux-kernel, linux-sh, linux-arch

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 <cernekee@gmail.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 3/3] sh: use the the PCI channels's io_map_base
  2012-01-30 12:18 [PATCH 0/3] arch: fix ioport mapping on mips,sh Michael S. Tsirkin
  2012-01-30 12:18 ` [PATCH 1/3] lib: add NO_GENERIC_PCI_IOPORT_MAP Michael S. Tsirkin
  2012-01-30 12:18 ` [PATCH 2/3] mips: use the the PCI controller's io_map_base Michael S. Tsirkin
@ 2012-01-30 12:19 ` Michael S. Tsirkin
  2012-01-30 17:50   ` Sergei Shtylyov
  2012-01-30 16:09 ` [PATCH 0/3] arch: fix ioport mapping on mips,sh Arnd Bergmann
  3 siblings, 1 reply; 15+ messages in thread
From: Michael S. Tsirkin @ 2012-01-30 12:19 UTC (permalink / raw)
  To: Kevin Cernekee
  Cc: Ralf Baechle, Paul Mundt, Arnd Bergmann, Michael S. Tsirkin,
	Jesse Barnes, Myron Stowe, Paul Gortmaker, Lucas De Marchi,
	Dmitry Kasatkin, James Morris, John W. Linville, Michael Witten,
	linux-mips, linux-kernel, linux-sh, linux-arch

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 <cernekee@gmail.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 arch/sh/Kconfig           |    1 +
 arch/sh/drivers/pci/pci.c |    4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

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
 
-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;
 
-- 
1.7.8.2.325.g247f9

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] lib: add NO_GENERIC_PCI_IOPORT_MAP
  2012-01-30 12:18 ` [PATCH 1/3] lib: add NO_GENERIC_PCI_IOPORT_MAP Michael S. Tsirkin
@ 2012-01-30 14:19   ` Shane McDonald
  2012-01-30 14:25     ` Michael S. Tsirkin
  2012-01-30 15:51   ` Arnd Bergmann
  1 sibling, 1 reply; 15+ messages in thread
From: Shane McDonald @ 2012-01-30 14:19 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Kevin Cernekee, Ralf Baechle, Paul Mundt, Arnd Bergmann,
	Jesse Barnes, Myron Stowe, Paul Gortmaker, Lucas De Marchi,
	Dmitry Kasatkin, James Morris, John W. Linville, Michael Witten,
	linux-mips, linux-kernel, linux-sh, linux-arch

Just a minor nit on the comment:

On Mon, Jan 30, 2012 at 6:18 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 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."?

> Supply a generic function that calls
> ioport_map, and make it possible for architectures
> to override.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Shane

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] lib: add NO_GENERIC_PCI_IOPORT_MAP
  2012-01-30 14:19   ` Shane McDonald
@ 2012-01-30 14:25     ` Michael S. Tsirkin
  0 siblings, 0 replies; 15+ messages in thread
From: Michael S. Tsirkin @ 2012-01-30 14:25 UTC (permalink / raw)
  To: Shane McDonald
  Cc: Kevin Cernekee, Ralf Baechle, Paul Mundt, Arnd Bergmann,
	Jesse Barnes, Myron Stowe, Paul Gortmaker, Lucas De Marchi,
	Dmitry Kasatkin, James Morris, John W. Linville, Michael Witten,
	linux-mips, linux-kernel, linux-sh, linux-arch

On Mon, Jan 30, 2012 at 08:19:31AM -0600, Shane McDonald wrote:
> Just a minor nit on the comment:
> 
> On Mon, Jan 30, 2012 at 6:18 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > 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.

> > Supply a generic function that calls
> > ioport_map, and make it possible for architectures
> > to override.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> Shane

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] lib: add NO_GENERIC_PCI_IOPORT_MAP
  2012-01-30 12:18 ` [PATCH 1/3] lib: add NO_GENERIC_PCI_IOPORT_MAP Michael S. Tsirkin
  2012-01-30 14:19   ` Shane McDonald
@ 2012-01-30 15:51   ` Arnd Bergmann
  2012-01-30 16:18     ` Michael S. Tsirkin
  1 sibling, 1 reply; 15+ messages in thread
From: Arnd Bergmann @ 2012-01-30 15:51 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Kevin Cernekee, Ralf Baechle, Paul Mundt, Jesse Barnes,
	Myron Stowe, Paul Gortmaker, Lucas De Marchi, Dmitry Kasatkin,
	James Morris, John W. Linville, Michael Witten, linux-mips,
	linux-kernel, linux-sh, linux-arch

On Monday 30 January 2012, Michael S. Tsirkin wrote:
> --- 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)
>  {
> 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

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/3] arch: fix ioport mapping on mips,sh
  2012-01-30 12:18 [PATCH 0/3] arch: fix ioport mapping on mips,sh Michael S. Tsirkin
                   ` (2 preceding siblings ...)
  2012-01-30 12:19 ` [PATCH 3/3] sh: use the the PCI channels's io_map_base Michael S. Tsirkin
@ 2012-01-30 16:09 ` Arnd Bergmann
  3 siblings, 0 replies; 15+ messages in thread
From: Arnd Bergmann @ 2012-01-30 16:09 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Kevin Cernekee, Ralf Baechle, Paul Mundt, Jesse Barnes,
	Myron Stowe, Paul Gortmaker, Lucas De Marchi, Dmitry Kasatkin,
	James Morris, John W. Linville, Michael Witten, linux-mips,
	linux-kernel, linux-sh, linux-arch

On Monday 30 January 2012, Michael S. Tsirkin wrote:
> 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.

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

Acked-by: Arnd Bergmann <arnd@arndb.de>

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] lib: add NO_GENERIC_PCI_IOPORT_MAP
  2012-01-30 15:51   ` Arnd Bergmann
@ 2012-01-30 16:18     ` Michael S. Tsirkin
  2012-01-30 20:04       ` Arnd Bergmann
  0 siblings, 1 reply; 15+ messages in thread
From: Michael S. Tsirkin @ 2012-01-30 16:18 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Kevin Cernekee, Ralf Baechle, Paul Mundt, Jesse Barnes,
	Myron Stowe, Paul Gortmaker, Lucas De Marchi, Dmitry Kasatkin,
	James Morris, John W. Linville, Michael Witten, linux-mips,
	linux-kernel, linux-sh, linux-arch

On Mon, Jan 30, 2012 at 03:51:46PM +0000, Arnd Bergmann wrote:
> On Monday 30 January 2012, Michael S. Tsirkin wrote:
> > --- 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)
> >  {
> > 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
> 
> 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

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/3] mips: use the the PCI controller's io_map_base
  2012-01-30 12:18 ` [PATCH 2/3] mips: use the the PCI controller's io_map_base Michael S. Tsirkin
@ 2012-01-30 17:49   ` Sergei Shtylyov
  0 siblings, 0 replies; 15+ messages in thread
From: Sergei Shtylyov @ 2012-01-30 17:49 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Kevin Cernekee, Ralf Baechle, Paul Mundt, Arnd Bergmann,
	Jesse Barnes, Myron Stowe, Paul Gortmaker, Lucas De Marchi,
	Dmitry Kasatkin, James Morris, John W. Linville, Michael Witten,
	linux-mips, linux-kernel, linux-sh, linux-arch

Hello.

On 01/30/2012 03:18 PM, Michael S. Tsirkin wrote:

> commit eab90291d35438bcebf7c3dc85be66d0f24e3002

    Please add that commit's summary in parens.

> 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 <cernekee@gmail.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

WBR, Sergei

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/3] sh: use the the PCI channels's io_map_base
  2012-01-30 12:19 ` [PATCH 3/3] sh: use the the PCI channels's io_map_base Michael S. Tsirkin
@ 2012-01-30 17:50   ` Sergei Shtylyov
  0 siblings, 0 replies; 15+ messages in thread
From: Sergei Shtylyov @ 2012-01-30 17:50 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Kevin Cernekee, Ralf Baechle, Paul Mundt, Arnd Bergmann,
	Jesse Barnes, Myron Stowe, Paul Gortmaker, Lucas De Marchi,
	Dmitry Kasatkin, James Morris, John W. Linville, Michael Witten,
	linux-mips, linux-kernel, linux-sh, linux-arch

Hello.

On 01/30/2012 03:19 PM, Michael S. Tsirkin wrote:

> commit 43db595e8b5d78ce5ad2feab719814a76e3ad2e5

    Please add that commit's summary in parens.

> 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<cernekee@gmail.com>
> Signed-off-by: Michael S. Tsirkin<mst@redhat.com>

WBR, Sergei


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] lib: add NO_GENERIC_PCI_IOPORT_MAP
  2012-01-30 16:18     ` Michael S. Tsirkin
@ 2012-01-30 20:04       ` Arnd Bergmann
  2012-01-31  0:22         ` Michael S. Tsirkin
  2012-01-31 21:18         ` Michael S. Tsirkin
  0 siblings, 2 replies; 15+ messages in thread
From: Arnd Bergmann @ 2012-01-30 20:04 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Kevin Cernekee, Ralf Baechle, Paul Mundt, Jesse Barnes,
	Myron Stowe, Paul Gortmaker, Lucas De Marchi, Dmitry Kasatkin,
	James Morris, John W. Linville, Michael Witten, linux-mips,
	linux-kernel, linux-sh, linux-arch

On Monday 30 January 2012, Michael S. Tsirkin wrote:
> > 
> > +/*
> > + * 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
> 
> 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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] lib: add NO_GENERIC_PCI_IOPORT_MAP
  2012-01-30 20:04       ` Arnd Bergmann
@ 2012-01-31  0:22         ` Michael S. Tsirkin
  2012-01-31 15:59           ` Arnd Bergmann
  2012-01-31 21:18         ` Michael S. Tsirkin
  1 sibling, 1 reply; 15+ messages in thread
From: Michael S. Tsirkin @ 2012-01-31  0:22 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Kevin Cernekee, Ralf Baechle, Paul Mundt, Jesse Barnes,
	Myron Stowe, Paul Gortmaker, Lucas De Marchi, Dmitry Kasatkin,
	James Morris, John W. Linville, Michael Witten, linux-mips,
	linux-kernel, linux-sh, linux-arch

On Mon, Jan 30, 2012 at 08:04:32PM +0000, Arnd Bergmann wrote:
> On Monday 30 January 2012, Michael S. Tsirkin wrote:
> > > 
> > > +/*
> > > + * 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
> > 
> > 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

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);
 }

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] lib: add NO_GENERIC_PCI_IOPORT_MAP
  2012-01-31  0:22         ` Michael S. Tsirkin
@ 2012-01-31 15:59           ` Arnd Bergmann
  0 siblings, 0 replies; 15+ messages in thread
From: Arnd Bergmann @ 2012-01-31 15:59 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Kevin Cernekee, Ralf Baechle, Paul Mundt, Jesse Barnes,
	Myron Stowe, Paul Gortmaker, Lucas De Marchi, Dmitry Kasatkin,
	James Morris, John W. Linville, Michael Witten, linux-mips,
	linux-kernel, linux-sh, linux-arch

On Tuesday 31 January 2012, Michael S. Tsirkin wrote:
> 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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] lib: add NO_GENERIC_PCI_IOPORT_MAP
  2012-01-30 20:04       ` Arnd Bergmann
  2012-01-31  0:22         ` Michael S. Tsirkin
@ 2012-01-31 21:18         ` Michael S. Tsirkin
  1 sibling, 0 replies; 15+ messages in thread
From: Michael S. Tsirkin @ 2012-01-31 21:18 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Kevin Cernekee, Ralf Baechle, Paul Mundt, Jesse Barnes,
	Myron Stowe, Paul Gortmaker, Lucas De Marchi, Dmitry Kasatkin,
	James Morris, John W. Linville, Michael Witten, linux-mips,
	linux-kernel, linux-sh, linux-arch

On Mon, Jan 30, 2012 at 08:04:32PM +0000, Arnd Bergmann wrote:
> On Monday 30 January 2012, Michael S. Tsirkin wrote:
> > > 
> > > +/*
> > > + * 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
> > 
> > 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

OK, I did exactly that. Thanks!

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2012-01-31 21:19 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-01-30 12:18 [PATCH 0/3] arch: fix ioport mapping on mips,sh Michael S. Tsirkin
2012-01-30 12:18 ` [PATCH 1/3] lib: add NO_GENERIC_PCI_IOPORT_MAP Michael S. Tsirkin
2012-01-30 14:19   ` Shane McDonald
2012-01-30 14:25     ` Michael S. Tsirkin
2012-01-30 15:51   ` Arnd Bergmann
2012-01-30 16:18     ` Michael S. Tsirkin
2012-01-30 20:04       ` Arnd Bergmann
2012-01-31  0:22         ` Michael S. Tsirkin
2012-01-31 15:59           ` Arnd Bergmann
2012-01-31 21:18         ` Michael S. Tsirkin
2012-01-30 12:18 ` [PATCH 2/3] mips: use the the PCI controller's io_map_base Michael S. Tsirkin
2012-01-30 17:49   ` Sergei Shtylyov
2012-01-30 12:19 ` [PATCH 3/3] sh: use the the PCI channels's io_map_base Michael S. Tsirkin
2012-01-30 17:50   ` Sergei Shtylyov
2012-01-30 16:09 ` [PATCH 0/3] arch: fix ioport mapping on mips,sh Arnd Bergmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).