[PATCH-RFC 0/2] tile: switch to generic pci_iomap

[PATCH-RFC 0/2] tile: switch to generic pci_iomap

[Michael S. Tsirkin]

Here's a completely untested patch to switch tile
over to the new pci_iomap (in -mm).

This is on top of pci: use generic pci_iomap in most architectures,
and converts just the tile architecture.

Seems straightforward enough but completely untested - could someone
with access to appropriate systems respond and say whether
the first patch in the series is OK?
The second one should then be trivial.

-- 
1.7.5.53.gc233e

[PATCH-RFC 0/2] tile: switch to generic pci_iomap

[Michael S. Tsirkin]

Here's a completely untested patch to switch tile
over to the new pci_iomap (in -mm).

This is on top of pci: use generic pci_iomap in most architectures,
and converts just the tile architecture.

Seems straightforward enough but completely untested - could someone
with access to appropriate systems respond and say whether
the first patch in the series is OK?
The second one should then be trivial.

-- 
1.7.5.53.gc233e

[PATCH-RFC 1/2] tile: don't panic on iomap

[Michael S. Tsirkin]

I think panic on iomap is there just for debugging.
If we return NULL instead, the generic pci_iomap will
DTRT so we don't need to roll our own.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 arch/tile/include/asm/io.h |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/tile/include/asm/io.h b/arch/tile/include/asm/io.h
index c9ea165..be6090d 100644
--- a/arch/tile/include/asm/io.h
+++ b/arch/tile/include/asm/io.h
@@ -204,7 +204,8 @@ static inline long ioport_panic(void)
 
 static inline void __iomem *ioport_map(unsigned long port, unsigned int len)
 {
-	return (void __iomem *) ioport_panic();
+	pr_info("Trying to map an IO resource - it does not exit on tile.\n");
+	return NULL;
 }
 
 static inline void ioport_unmap(void __iomem *addr)
-- 
1.7.5.53.gc233e

[PATCH-RFC 1/2] tile: don't panic on iomap

[Michael S. Tsirkin]

I think panic on iomap is there just for debugging.
If we return NULL instead, the generic pci_iomap will
DTRT so we don't need to roll our own.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 arch/tile/include/asm/io.h |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/tile/include/asm/io.h b/arch/tile/include/asm/io.h
index c9ea165..be6090d 100644
--- a/arch/tile/include/asm/io.h
+++ b/arch/tile/include/asm/io.h
@@ -204,7 +204,8 @@ static inline long ioport_panic(void)
 
 static inline void __iomem *ioport_map(unsigned long port, unsigned int len)
 {
-	return (void __iomem *) ioport_panic();
+	pr_info("Trying to map an IO resource - it does not exit on tile.\n");
+	return NULL;
 }
 
 static inline void ioport_unmap(void __iomem *addr)
-- 
1.7.5.53.gc233e

[PATCH-RFC 2/2] tile: switch to GENERIC_PCI_IOMAP

[Michael S. Tsirkin]

tile now has working stubs for ioport_map and ioremap
such that the generic pci_iomap will DTRT: cast to
pointer on memory and return NULL and log message on IO map.

Switch it over to GENERIC_PCI_IOMAP.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 arch/tile/Kconfig           |    1 +
 arch/tile/include/asm/pci.h |    2 +-
 arch/tile/kernel/pci.c      |   21 ---------------------
 3 files changed, 2 insertions(+), 22 deletions(-)

diff --git a/arch/tile/Kconfig b/arch/tile/Kconfig
index 70a0de4..11270ca 100644
--- a/arch/tile/Kconfig
+++ b/arch/tile/Kconfig
@@ -321,6 +321,7 @@ config PCI
 	bool "PCI support"
 	default y
 	select PCI_DOMAINS
+	select GENERIC_PCI_IOMAP
 	---help---
 	  Enable PCI root complex support, so PCIe endpoint devices can
 	  be attached to the Tile chip.  Many, but not all, PCI devices
diff --git a/arch/tile/include/asm/pci.h b/arch/tile/include/asm/pci.h
index 7f03cef..1d25fea 100644
--- a/arch/tile/include/asm/pci.h
+++ b/arch/tile/include/asm/pci.h
@@ -16,6 +16,7 @@
 #define _ASM_TILE_PCI_H
 
 #include <linux/pci.h>
+#include <asm-generic/pci_iomap.h>
 
 /*
  * Structure of a PCI controller (host bridge)
@@ -49,7 +50,6 @@ struct pci_controller {
 int __devinit tile_pci_init(void);
 int __devinit pcibios_init(void);
 
-void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long max);
 static inline void pci_iounmap(struct pci_dev *dev, void __iomem *addr) {}
 
 void __devinit pcibios_fixup_bus(struct pci_bus *bus);
diff --git a/arch/tile/kernel/pci.c b/arch/tile/kernel/pci.c
index 2a8014c..1b6244e 100644
--- a/arch/tile/kernel/pci.c
+++ b/arch/tile/kernel/pci.c
@@ -465,27 +465,6 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
 	return 0;
 }
 
-void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long max)
-{
-	unsigned long start = pci_resource_start(dev, bar);
-	unsigned long len = pci_resource_len(dev, bar);
-	unsigned long flags = pci_resource_flags(dev, bar);
-
-	if (!len)
-		return NULL;
-	if (max && len > max)
-		len = max;
-
-	if (!(flags & IORESOURCE_MEM)) {
-		pr_info("PCI: Trying to map invalid resource %#lx\n", flags);
-		start = 0;
-	}
-
-	return (void __iomem *)start;
-}
-EXPORT_SYMBOL(pci_iomap);
-
-
 /****************************************************************
  *
  * Tile PCI config space read/write routines
-- 
1.7.5.53.gc233e

[PATCH-RFC 2/2] tile: switch to GENERIC_PCI_IOMAP

[Michael S. Tsirkin]

tile now has working stubs for ioport_map and ioremap
such that the generic pci_iomap will DTRT: cast to
pointer on memory and return NULL and log message on IO map.

Switch it over to GENERIC_PCI_IOMAP.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 arch/tile/Kconfig           |    1 +
 arch/tile/include/asm/pci.h |    2 +-
 arch/tile/kernel/pci.c      |   21 ---------------------
 3 files changed, 2 insertions(+), 22 deletions(-)

diff --git a/arch/tile/Kconfig b/arch/tile/Kconfig
index 70a0de4..11270ca 100644
--- a/arch/tile/Kconfig
+++ b/arch/tile/Kconfig
@@ -321,6 +321,7 @@ config PCI
 	bool "PCI support"
 	default y
 	select PCI_DOMAINS
+	select GENERIC_PCI_IOMAP
 	---help---
 	  Enable PCI root complex support, so PCIe endpoint devices can
 	  be attached to the Tile chip.  Many, but not all, PCI devices
diff --git a/arch/tile/include/asm/pci.h b/arch/tile/include/asm/pci.h
index 7f03cef..1d25fea 100644
--- a/arch/tile/include/asm/pci.h
+++ b/arch/tile/include/asm/pci.h
@@ -16,6 +16,7 @@
 #define _ASM_TILE_PCI_H
 
 #include <linux/pci.h>
+#include <asm-generic/pci_iomap.h>
 
 /*
  * Structure of a PCI controller (host bridge)
@@ -49,7 +50,6 @@ struct pci_controller {
 int __devinit tile_pci_init(void);
 int __devinit pcibios_init(void);
 
-void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long max);
 static inline void pci_iounmap(struct pci_dev *dev, void __iomem *addr) {}
 
 void __devinit pcibios_fixup_bus(struct pci_bus *bus);
diff --git a/arch/tile/kernel/pci.c b/arch/tile/kernel/pci.c
index 2a8014c..1b6244e 100644
--- a/arch/tile/kernel/pci.c
+++ b/arch/tile/kernel/pci.c
@@ -465,27 +465,6 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
 	return 0;
 }
 
-void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long max)
-{
-	unsigned long start = pci_resource_start(dev, bar);
-	unsigned long len = pci_resource_len(dev, bar);
-	unsigned long flags = pci_resource_flags(dev, bar);
-
-	if (!len)
-		return NULL;
-	if (max && len > max)
-		len = max;
-
-	if (!(flags & IORESOURCE_MEM)) {
-		pr_info("PCI: Trying to map invalid resource %#lx\n", flags);
-		start = 0;
-	}
-
-	return (void __iomem *)start;
-}
-EXPORT_SYMBOL(pci_iomap);
-
-
 /****************************************************************
  *
  * Tile PCI config space read/write routines
-- 
1.7.5.53.gc233e

Re: [PATCH-RFC 1/2] tile: don't panic on iomap

[Bjorn Helgaas]

On Tue, Nov 29, 2011 at 10:54 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> I think panic on iomap is there just for debugging.
> If we return NULL instead, the generic pci_iomap will
> DTRT so we don't need to roll our own.

Just to be explicit about what "doing the right thing" means, here's
what I think is changing (I think the new behavior is OK, but it *is*
different):

Old behavior: Caller calls pci_iomap(), which panics in ioport_map().

New behavior: Caller calls pci_iomap(), ioport_map() returns NULL,
pci_iomap() returns NULL (failure), caller may check for failure.  If
caller does not check for failure and passes the NULL to
ioread()/iowrite(), we WARN in bad_io_access().

>  static inline void __iomem *ioport_map(unsigned long port, unsigned int len)
>  {
> -       return (void __iomem *) ioport_panic();
> +       pr_info("Trying to map an IO resource - it does not exit on tile.\n");
> +       return NULL;

s/exit/exist/

Since we only expect to see this message during debugging, maybe it
could be more informative, e.g., use dump_stack() to identify the
offending driver?  I don't think either the "Trying to map" message or
the "Bad IO access" message is enough to actually make progress in
debugging.

Bjorn

Re: [PATCH-RFC 1/2] tile: don't panic on iomap

[Arnd Bergmann]

On Tuesday 29 November 2011, Michael S. Tsirkin wrote:
> diff --git a/arch/tile/include/asm/io.h b/arch/tile/include/asm/io.h
> index c9ea165..be6090d 100644
> --- a/arch/tile/include/asm/io.h
> +++ b/arch/tile/include/asm/io.h
> @@ -204,7 +204,8 @@ static inline long ioport_panic(void)
>  
>  static inline void __iomem *ioport_map(unsigned long port, unsigned int len)
>  {
> -       return (void __iomem *) ioport_panic();
> +       pr_info("Trying to map an IO resource - it does not exit on tile.\n");
> +       return NULL;
>  }

Why not just set CONFIG_NO_IOPORT and make this unavailable at compile time?

	Arnd

Re: [PATCH-RFC 1/2] tile: don't panic on iomap

[Michael S. Tsirkin]

On Tue, Nov 29, 2011 at 01:04:12PM -0800, Bjorn Helgaas wrote:
> On Tue, Nov 29, 2011 at 10:54 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > I think panic on iomap is there just for debugging.
> > If we return NULL instead, the generic pci_iomap will
> > DTRT so we don't need to roll our own.
> 
> Just to be explicit about what "doing the right thing" means, here's
> what I think is changing (I think the new behavior is OK, but it *is*
> different):

I think the change is that anyone calling ioport_map *directly*
will fail. pci_iomap callers are mostly unaffected.

> 
> Old behavior: Caller calls pci_iomap(), which panics in ioport_map().

Not really, the old pci_iomap simply returned NULL in this case, it
did not call ioport_map.

> New behavior: Caller calls pci_iomap(), ioport_map() returns NULL,
> pci_iomap() returns NULL (failure), caller may check for failure.  If
> caller does not check for failure and passes the NULL to
> ioread()/iowrite(), we WARN in bad_io_access().
> 
> >  static inline void __iomem *ioport_map(unsigned long port, unsigned int len)
> >  {
> > -       return (void __iomem *) ioport_panic();
> > +       pr_info("Trying to map an IO resource - it does not exit on tile.\n");
> > +       return NULL;
> 
> s/exit/exist/
> 
> Since we only expect to see this message during debugging, maybe it
> could be more informative, e.g., use dump_stack() to identify the
> offending driver?  I don't think either the "Trying to map" message or
> the "Bad IO access" message is enough to actually make progress in
> debugging.
> 
> Bjorn

As explained above, only direct callers of ioport_map get a changed
behaviour. If we start dumping stack there we will hurt users of
pci_iomap which used to get a graceful failure and will start getting
scary messages. Is does not seem to be worth doing to simplify debugging, right?
How about sticking the function name in the pr_info message?
A simple grep for ioport_map will then get you the culprit ...
Like this:
+       pr_info("ioport_map: mapping IO resources is unsupported on tile.\n");
?

-- 
MST

Re: [PATCH-RFC 1/2] tile: don't panic on iomap

[Michael S. Tsirkin]

On Tue, Nov 29, 2011 at 10:05:11PM +0000, Arnd Bergmann wrote:
> On Tuesday 29 November 2011, Michael S. Tsirkin wrote:
> > diff --git a/arch/tile/include/asm/io.h b/arch/tile/include/asm/io.h
> > index c9ea165..be6090d 100644
> > --- a/arch/tile/include/asm/io.h
> > +++ b/arch/tile/include/asm/io.h
> > @@ -204,7 +204,8 @@ static inline long ioport_panic(void)
> >  
> >  static inline void __iomem *ioport_map(unsigned long port, unsigned int len)
> >  {
> > -       return (void __iomem *) ioport_panic();
> > +       pr_info("Trying to map an IO resource - it does not exit on tile.\n");
> > +       return NULL;
> >  }
> 
> Why not just set CONFIG_NO_IOPORT and make this unavailable at compile time?
> 
> 	Arnd

Well I think CONFIG_NO_IOPORT only has effect if you pull in asm-generic/io.h.
This might work but I have no idea. Presumably whoever wrote that architecture
considered and discarded setting GENERIC_IOMAP.

-- 
MST

[PATCH-RFC 1/2 v2] tile: don't panic on iomap

[Michael S. Tsirkin]

I think panic on iomap is there just for debugging.
If we return NULL instead, the generic pci_iomap will
do the same thing as the custom one that tile currently has
(that is, return NULL on an IO BAR)
so tile won't need to roll its own anymore.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Note: the other patch in the series is unchanged.
Changes from v1:
  - tweaked pr_info message

 arch/tile/include/asm/io.h |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/tile/include/asm/io.h b/arch/tile/include/asm/io.h
index c9ea165..d2152de 100644
--- a/arch/tile/include/asm/io.h
+++ b/arch/tile/include/asm/io.h
@@ -204,7 +204,8 @@ static inline long ioport_panic(void)
 
 static inline void __iomem *ioport_map(unsigned long port, unsigned int len)
 {
-	return (void __iomem *) ioport_panic();
+	pr_info("ioport_map: mapping IO resources is unsupported on tile.\n");
+	return NULL;
 }
 
 static inline void ioport_unmap(void __iomem *addr)
-- 
1.7.5.53.gc233e

Re: [PATCH-RFC 1/2] tile: don't panic on iomap

[Arnd Bergmann]

On Wednesday 30 November 2011, Michael S. Tsirkin wrote:
> 
> On Tue, Nov 29, 2011 at 10:05:11PM +0000, Arnd Bergmann wrote:
> > On Tuesday 29 November 2011, Michael S. Tsirkin wrote:
> > > diff --git a/arch/tile/include/asm/io.h b/arch/tile/include/asm/io.h
> > > index c9ea165..be6090d 100644
> > > --- a/arch/tile/include/asm/io.h
> > > +++ b/arch/tile/include/asm/io.h
> > > @@ -204,7 +204,8 @@ static inline long ioport_panic(void)
> > >  
> > >  static inline void __iomem *ioport_map(unsigned long port, unsigned int len)
> > >  {
> > > -       return (void __iomem *) ioport_panic();
> > > +       pr_info("Trying to map an IO resource - it does not exit on tile.\n");
> > > +       return NULL;
> > >  }
> > 
> > Why not just set CONFIG_NO_IOPORT and make this unavailable at compile time?
> > 
> >       Arnd
> 
> Well I think CONFIG_NO_IOPORT only has effect if you pull in asm-generic/io.h.
> This might work but I have no idea.

NO_IOPORT ha nothing to do with asm-generic, it just makes lib/iomap.c leave out
the calls to ioport_map, and prevent all drivers that call ioport_map from being
built.

> Presumably whoever wrote that architecture considered and discarded setting GENERIC_IOMAP.

The tile PCI controller is apparently special in that it does not map the
PIO region into MMIO space, or make it available in any other way, so generic
iomap with IOCOND is pointless. Setting NO_IPORT will still mean that drivers
using ioport_map will not be built. This mostly impacts the legacy (non-AHCI)
ATA controllers, which I assume are not used on tile anyway.

	Arnd

Re: [PATCH-RFC 1/2] tile: don't panic on iomap

[Michael S. Tsirkin]

On Wed, Nov 30, 2011 at 09:09:18AM +0000, Arnd Bergmann wrote:
> On Wednesday 30 November 2011, Michael S. Tsirkin wrote:
> > 
> > On Tue, Nov 29, 2011 at 10:05:11PM +0000, Arnd Bergmann wrote:
> > > On Tuesday 29 November 2011, Michael S. Tsirkin wrote:
> > > > diff --git a/arch/tile/include/asm/io.h b/arch/tile/include/asm/io.h
> > > > index c9ea165..be6090d 100644
> > > > --- a/arch/tile/include/asm/io.h
> > > > +++ b/arch/tile/include/asm/io.h
> > > > @@ -204,7 +204,8 @@ static inline long ioport_panic(void)
> > > >  
> > > >  static inline void __iomem *ioport_map(unsigned long port, unsigned int len)
> > > >  {
> > > > -       return (void __iomem *) ioport_panic();
> > > > +       pr_info("Trying to map an IO resource - it does not exit on tile.\n");
> > > > +       return NULL;
> > > >  }
> > > 
> > > Why not just set CONFIG_NO_IOPORT and make this unavailable at compile time?
> > > 
> > >       Arnd
> > 
> > Well I think CONFIG_NO_IOPORT only has effect if you pull in asm-generic/io.h.
> > This might work but I have no idea.
> 
> NO_IOPORT ha nothing to do with asm-generic, it just makes lib/iomap.c leave out
> the calls to ioport_map, and prevent all drivers that call ioport_map from being
> built.

Yes. But lib/iomap.c isn't even linked unless GENERIC_IOMAP is set.

> > Presumably whoever wrote that architecture considered and discarded setting GENERIC_IOMAP.
> 
> The tile PCI controller is apparently special in that it does not map the
> PIO region into MMIO space, or make it available in any other way, so generic
> iomap with IOCOND is pointless. Setting NO_IPORT will still mean that drivers
> using ioport_map will not be built. This mostly impacts the legacy (non-AHCI)
> ATA controllers, which I assume are not used on tile anyway.
> 
> 	Arnd


I don't object to setting NO_IPORT at all, but this won't help address
the problem this patch is fixing, which is to use a generic pci_iomap
on tile.

-- 
MST

Re: [PATCH-RFC 1/2] tile: don't panic on iomap

[Arnd Bergmann]

On Wednesday 30 November 2011, Michael S. Tsirkin wrote:
> > > Presumably whoever wrote that architecture considered and discarded setting GENERIC_IOMAP.
> > 
> > The tile PCI controller is apparently special in that it does not map the
> > PIO region into MMIO space, or make it available in any other way, so generic
> > iomap with IOCOND is pointless. Setting NO_IPORT will still mean that drivers
> > using ioport_map will not be built. This mostly impacts the legacy (non-AHCI)
> > ATA controllers, which I assume are not used on tile anyway.
> 
> I don't object to setting NO_IPORT at all, but this won't help address
> the problem this patch is fixing, which is to use a generic pci_iomap
> on tile.

Ah, right. I didn't realize that the generic pci_iomap still attempts
to call ioport_map(). It would probably make sense to enclose
the ioport_map() call in pci_iomap() inside of #ifdef CONFIG_HAS_IOPORT.
It's not exactly beautiful, but probably the most correct solution
so that we can make any call to ioport_map() a build-time error on
architectures that set CONFIG_NO_IOPORT.

	Arnd

Re: [PATCH-RFC 1/2] tile: don't panic on iomap

[Michael S. Tsirkin]

On Wed, Nov 30, 2011 at 02:04:41PM +0000, Arnd Bergmann wrote:
> On Wednesday 30 November 2011, Michael S. Tsirkin wrote:
> > > > Presumably whoever wrote that architecture considered and discarded setting GENERIC_IOMAP.
> > > 
> > > The tile PCI controller is apparently special in that it does not map the
> > > PIO region into MMIO space, or make it available in any other way, so generic
> > > iomap with IOCOND is pointless. Setting NO_IPORT will still mean that drivers
> > > using ioport_map will not be built. This mostly impacts the legacy (non-AHCI)
> > > ATA controllers, which I assume are not used on tile anyway.
> > 
> > I don't object to setting NO_IPORT at all, but this won't help address
> > the problem this patch is fixing, which is to use a generic pci_iomap
> > on tile.
> 
> Ah, right. I didn't realize that the generic pci_iomap still attempts
> to call ioport_map(). It would probably make sense to enclose
> the ioport_map() call in pci_iomap() inside of #ifdef CONFIG_HAS_IOPORT.
> It's not exactly beautiful, but probably the most correct solution
> so that we can make any call to ioport_map() a build-time error on
> architectures that set CONFIG_NO_IOPORT.
> 
> 	Arnd

I'm not sure why do you want to do that.

-- 
MST

Re: [PATCH-RFC 1/2] tile: don't panic on iomap

[Arnd Bergmann]

On Wednesday 30 November 2011, Michael S. Tsirkin wrote:
> On Wed, Nov 30, 2011 at 02:04:41PM +0000, Arnd Bergmann wrote:
>
> > Ah, right. I didn't realize that the generic pci_iomap still attempts
> > to call ioport_map(). It would probably make sense to enclose
> > the ioport_map() call in pci_iomap() inside of #ifdef CONFIG_HAS_IOPORT.
> > It's not exactly beautiful, but probably the most correct solution
> > so that we can make any call to ioport_map() a build-time error on
> > architectures that set CONFIG_NO_IOPORT.
> 
> I'm not sure why do you want to do that.
> 

The problem is that any definition of ioport_map on architectures
that can't do it is potentially harmful. Calling panic() is
bad style as you pointed out, but simply returning NULL can
also be harmful because it's likely that some drivers are written
under the (false) assumption that ioport_map can never fail.

Getting a build-time error would be more helpful here IMHO.

	Arnd

Re: [PATCH-RFC 1/2] tile: don't panic on iomap

[Michael S. Tsirkin]

On Wed, Nov 30, 2011 at 03:49:57PM +0000, Arnd Bergmann wrote:
> On Wednesday 30 November 2011, Michael S. Tsirkin wrote:
> > On Wed, Nov 30, 2011 at 02:04:41PM +0000, Arnd Bergmann wrote:
> >
> > > Ah, right. I didn't realize that the generic pci_iomap still attempts
> > > to call ioport_map(). It would probably make sense to enclose
> > > the ioport_map() call in pci_iomap() inside of #ifdef CONFIG_HAS_IOPORT.
> > > It's not exactly beautiful, but probably the most correct solution
> > > so that we can make any call to ioport_map() a build-time error on
> > > architectures that set CONFIG_NO_IOPORT.
> > 
> > I'm not sure why do you want to do that.
> > 
> 
> The problem is that any definition of ioport_map on architectures
> that can't do it is potentially harmful. Calling panic() is
> bad style as you pointed out, but simply returning NULL can
> also be harmful because it's likely that some drivers are written
> under the (false) assumption that ioport_map can never fail.
> Getting a build-time error would be more helpful here IMHO.
> 
> 	Arnd

Yes but uglifying these users is also bad, ifdefs in code are incredibly
fragile. Isn't it enough to declare ioport_map __must_check?

-- 
MST

Re: [PATCH-RFC 1/2] tile: don't panic on iomap

[Arnd Bergmann]

On Wednesday 30 November 2011, Michael S. Tsirkin wrote:
> On Wed, Nov 30, 2011 at 03:49:57PM +0000, Arnd Bergmann wrote:
> > On Wednesday 30 November 2011, Michael S. Tsirkin wrote:
> > > On Wed, Nov 30, 2011 at 02:04:41PM +0000, Arnd Bergmann wrote:
> > >
> > > > Ah, right. I didn't realize that the generic pci_iomap still attempts
> > > > to call ioport_map(). It would probably make sense to enclose
> > > > the ioport_map() call in pci_iomap() inside of #ifdef CONFIG_HAS_IOPORT.
> > > > It's not exactly beautiful, but probably the most correct solution
> > > > so that we can make any call to ioport_map() a build-time error on
> > > > architectures that set CONFIG_NO_IOPORT.
> > > 
> > > I'm not sure why do you want to do that.
> > > 
> > 
> > The problem is that any definition of ioport_map on architectures
> > that can't do it is potentially harmful. Calling panic() is
> > bad style as you pointed out, but simply returning NULL can
> > also be harmful because it's likely that some drivers are written
> > under the (false) assumption that ioport_map can never fail.
> > Getting a build-time error would be more helpful here IMHO.
> 
> Yes but uglifying these users is also bad, ifdefs in code are incredibly
> fragile. Isn't it enough to declare ioport_map __must_check?

I guess we already wasted too many electrons over this triviality,
I don't actually care all that much, and will probably have to
touch the same code again when I get to submit my patch to make
inb/outb optional for architectures. Just pick any solution (including
your original one). I'll revisit this when I'm bothered by the
presence of the ioport_map function and will keep you in the loop
on those patches.

	Arnd

Re: [PATCH-RFC 2/2] tile: switch to GENERIC_PCI_IOMAP

[Chris Metcalf]

On 11/29/2011 1:54 PM, Michael S. Tsirkin wrote:
> tile now has working stubs for ioport_map and ioremap
> such that the generic pci_iomap will DTRT: cast to
> pointer on memory and return NULL and log message on IO map.
>
> Switch it over to GENERIC_PCI_IOMAP.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  arch/tile/Kconfig           |    1 +
>  arch/tile/include/asm/pci.h |    2 +-
>  arch/tile/kernel/pci.c      |   21 ---------------------
>  3 files changed, 2 insertions(+), 22 deletions(-)

Acked-by: Chris Metcalf <cmetcalf@tilera.com>

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH-RFC 2/2] tile: switch to GENERIC_PCI_IOMAP

[Chris Metcalf]

On 11/29/2011 1:54 PM, Michael S. Tsirkin wrote:
> tile now has working stubs for ioport_map and ioremap
> such that the generic pci_iomap will DTRT: cast to
> pointer on memory and return NULL and log message on IO map.
>
> Switch it over to GENERIC_PCI_IOMAP.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  arch/tile/Kconfig           |    1 +
>  arch/tile/include/asm/pci.h |    2 +-
>  arch/tile/kernel/pci.c      |   21 ---------------------
>  3 files changed, 2 insertions(+), 22 deletions(-)

Acked-by: Chris Metcalf <cmetcalf@tilera.com>

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

[PATCH] lib/devres.c: allow specifying NO_IOPORT while using PCI

[Chris Metcalf]

The tile architecture does not support IOPORT, but it does support
PCI and IOMEM.  We were not specifying NO_IOPORT, though, because it
was causing a couple of functions (pcim_iomap_regions and pcim_iomap_table)
that are needed to not get provided.  Moving the #ifdef fixes this
so that those functions and some related ones are now always provided
if CONFIG_PCI is true.

Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
---
This commit was inspired by the recent linux-arch discussion about
what to do with ioport_map on architectures that can't do it, so I went
back to figure out why I hadn't set NO_IOPORT in the first place.
I'm willing to take this into the tile tree if the PCI folks are OK
with it, or if someone else can pick it up, that's fine too.

 arch/tile/Kconfig |    2 +-
 lib/devres.c      |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/tile/Kconfig b/arch/tile/Kconfig
index 70a0de4..e7b1e07 100644
--- a/arch/tile/Kconfig
+++ b/arch/tile/Kconfig
@@ -333,7 +333,7 @@ config NO_IOMEM
 	def_bool !PCI
 
 config NO_IOPORT
-	def_bool !PCI
+	def_bool y
 
 source "drivers/pci/Kconfig"
 
diff --git a/lib/devres.c b/lib/devres.c
index 7c0e953..5f36cfd 100644
--- a/lib/devres.c
+++ b/lib/devres.c
@@ -144,6 +144,7 @@ void devm_ioport_unmap(struct device *dev, void __iomem *addr)
 			       devm_ioport_map_match, (void *)addr));
 }
 EXPORT_SYMBOL(devm_ioport_unmap);
+#endif
 
 #ifdef CONFIG_PCI
 /*
@@ -349,4 +350,3 @@ void pcim_iounmap_regions(struct pci_dev *pdev, u16 mask)
 }
 EXPORT_SYMBOL(pcim_iounmap_regions);
 #endif
-#endif
-- 
1.6.5.2

[PATCH] lib/devres.c: allow specifying NO_IOPORT while using PCI

[Chris Metcalf]

The tile architecture does not support IOPORT, but it does support
PCI and IOMEM.  We were not specifying NO_IOPORT, though, because it
was causing a couple of functions (pcim_iomap_regions and pcim_iomap_table)
that are needed to not get provided.  Moving the #ifdef fixes this
so that those functions and some related ones are now always provided
if CONFIG_PCI is true.

Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
---
This commit was inspired by the recent linux-arch discussion about
what to do with ioport_map on architectures that can't do it, so I went
back to figure out why I hadn't set NO_IOPORT in the first place.
I'm willing to take this into the tile tree if the PCI folks are OK
with it, or if someone else can pick it up, that's fine too.

 arch/tile/Kconfig |    2 +-
 lib/devres.c      |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/tile/Kconfig b/arch/tile/Kconfig
index 70a0de4..e7b1e07 100644
--- a/arch/tile/Kconfig
+++ b/arch/tile/Kconfig
@@ -333,7 +333,7 @@ config NO_IOMEM
 	def_bool !PCI
 
 config NO_IOPORT
-	def_bool !PCI
+	def_bool y
 
 source "drivers/pci/Kconfig"
 
diff --git a/lib/devres.c b/lib/devres.c
index 7c0e953..5f36cfd 100644
--- a/lib/devres.c
+++ b/lib/devres.c
@@ -144,6 +144,7 @@ void devm_ioport_unmap(struct device *dev, void __iomem *addr)
 			       devm_ioport_map_match, (void *)addr));
 }
 EXPORT_SYMBOL(devm_ioport_unmap);
+#endif
 
 #ifdef CONFIG_PCI
 /*
@@ -349,4 +350,3 @@ void pcim_iounmap_regions(struct pci_dev *pdev, u16 mask)
 }
 EXPORT_SYMBOL(pcim_iounmap_regions);
 #endif
-#endif
-- 
1.6.5.2

Re: [PATCH] lib/devres.c: allow specifying NO_IOPORT while using PCI

[Tejun Heo]

Hello,

On Thu, Dec 01, 2011 at 12:59:19PM -0500, Chris Metcalf wrote:
> The tile architecture does not support IOPORT, but it does support
> PCI and IOMEM.  We were not specifying NO_IOPORT, though, because it
> was causing a couple of functions (pcim_iomap_regions and pcim_iomap_table)
> that are needed to not get provided.  Moving the #ifdef fixes this
> so that those functions and some related ones are now always provided
> if CONFIG_PCI is true.
> 
> Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>

  Acked-by: Tejun Heo <tj@kernel.org>

Please feel free to route the change through tile.  If not, Andrew,
can you please pick this up?

Thanks.

-- 
tejun

Re: [PATCH] lib/devres.c: allow specifying NO_IOPORT while using PCI

[Chris Metcalf]

On 12/1/2011 1:44 PM, Tejun Heo wrote:
> Hello,
>
> On Thu, Dec 01, 2011 at 12:59:19PM -0500, Chris Metcalf wrote:
>> The tile architecture does not support IOPORT, but it does support
>> PCI and IOMEM.  We were not specifying NO_IOPORT, though, because it
>> was causing a couple of functions (pcim_iomap_regions and pcim_iomap_table)
>> that are needed to not get provided.  Moving the #ifdef fixes this
>> so that those functions and some related ones are now always provided
>> if CONFIG_PCI is true.
>>
>> Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
>   Acked-by: Tejun Heo <tj@kernel.org>
>
> Please feel free to route the change through tile.  If not, Andrew,
> can you please pick this up?

Thanks.  I'll plan to push this to Linus through the tile tree with your
Acked-by.

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH] lib/devres.c: allow specifying NO_IOPORT while using PCI

[Chris Metcalf]

On 12/1/2011 1:44 PM, Tejun Heo wrote:
> Hello,
>
> On Thu, Dec 01, 2011 at 12:59:19PM -0500, Chris Metcalf wrote:
>> The tile architecture does not support IOPORT, but it does support
>> PCI and IOMEM.  We were not specifying NO_IOPORT, though, because it
>> was causing a couple of functions (pcim_iomap_regions and pcim_iomap_table)
>> that are needed to not get provided.  Moving the #ifdef fixes this
>> so that those functions and some related ones are now always provided
>> if CONFIG_PCI is true.
>>
>> Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
>   Acked-by: Tejun Heo <tj@kernel.org>
>
> Please feel free to route the change through tile.  If not, Andrew,
> can you please pick this up?

Thanks.  I'll plan to push this to Linus through the tile tree with your
Acked-by.

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH] lib/devres.c: allow specifying NO_IOPORT while using PCI

[Arnd Bergmann]

On Thursday 01 December 2011 12:59:19 Chris Metcalf wrote:
> The tile architecture does not support IOPORT, but it does support
> PCI and IOMEM.  We were not specifying NO_IOPORT, though, because it
> was causing a couple of functions (pcim_iomap_regions and pcim_iomap_table)
> that are needed to not get provided.  Moving the #ifdef fixes this
> so that those functions and some related ones are now always provided
> if CONFIG_PCI is true.
> 
> Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>

Hmm, I'm sure that Al Viro intentionally did it the way it is, but I'm
not sure exactly why that would be.

Any driver using the functions you now expose for NO_IOPORT is probably
prepared to handle both MMIO and PIO BARs on PCI, which you can consider
broken in the case where a card actually requires PIO.

On tile, it would be just as broken as other drivers that directly use
inb/outb to do the port access, but on platforms that actually have
PIO but are not able to map them (like arm/mach-rpc), you would now
enable people to write silently broken drivers.

However, assuming they check the return value of the *iomap, the
drivers could still easily detect this case at runtime. Also, at
least on tile, you already provide a pci_iomap function, which
has the exact same problem as the devres functions.

One area where your patch might actually break existing code is
architectures that allow the devres functions to be built *and*
have PCI support but don't provide a pci_iomap function, because
they do not support mapping the PIO range into addresses. I'm
not sure if that's actually still the case anywhere after Michael's
recent patches, but it used to be the case on RPC and probably others.

	Arnd

Re: [PATCH] lib/devres.c: allow specifying NO_IOPORT while using PCI

[Chris Metcalf]

On 12/1/2011 5:47 PM, Arnd Bergmann wrote:
> On Thursday 01 December 2011 12:59:19 Chris Metcalf wrote:
>> The tile architecture does not support IOPORT, but it does support
>> PCI and IOMEM.  We were not specifying NO_IOPORT, though, because it
>> was causing a couple of functions (pcim_iomap_regions and pcim_iomap_table)
>> that are needed to not get provided.  Moving the #ifdef fixes this
>> so that those functions and some related ones are now always provided
>> if CONFIG_PCI is true.
>>
>> Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
> Hmm, I'm sure that Al Viro intentionally did it the way it is, but I'm
> not sure exactly why that would be.
>
> Any driver using the functions you now expose for NO_IOPORT is probably
> prepared to handle both MMIO and PIO BARs on PCI, which you can consider
> broken in the case where a card actually requires PIO.
>
> On tile, it would be just as broken as other drivers that directly use
> inb/outb to do the port access, but on platforms that actually have
> PIO but are not able to map them (like arm/mach-rpc), you would now
> enable people to write silently broken drivers.
>
> However, assuming they check the return value of the *iomap, the
> drivers could still easily detect this case at runtime. Also, at
> least on tile, you already provide a pci_iomap function, which
> has the exact same problem as the devres functions.
>
> One area where your patch might actually break existing code is
> architectures that allow the devres functions to be built *and*
> have PCI support but don't provide a pci_iomap function, because
> they do not support mapping the PIO range into addresses. I'm
> not sure if that's actually still the case anywhere after Michael's
> recent patches, but it used to be the case on RPC and probably others.

Sounds sufficiently subtle that I'll plan to drop the patch unless someone
else (maybe Al Viro, now cc'ed) wants to weigh in.  For tile this just
means we'll continue not to set NO_IOPORT, at the cost of a few hundred
bytes of dead code; no big deal.

The specific issue I saw was calls to pcim_iomap_{regions,table} from
drivers/ata/libata-sff.c and drivers/ata/sata_sil24.c; we use the
sata_sil24 in one of our platforms (TILEmpower).

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH] lib/devres.c: allow specifying NO_IOPORT while using PCI

[Chris Metcalf]

On 12/1/2011 5:47 PM, Arnd Bergmann wrote:
> On Thursday 01 December 2011 12:59:19 Chris Metcalf wrote:
>> The tile architecture does not support IOPORT, but it does support
>> PCI and IOMEM.  We were not specifying NO_IOPORT, though, because it
>> was causing a couple of functions (pcim_iomap_regions and pcim_iomap_table)
>> that are needed to not get provided.  Moving the #ifdef fixes this
>> so that those functions and some related ones are now always provided
>> if CONFIG_PCI is true.
>>
>> Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
> Hmm, I'm sure that Al Viro intentionally did it the way it is, but I'm
> not sure exactly why that would be.
>
> Any driver using the functions you now expose for NO_IOPORT is probably
> prepared to handle both MMIO and PIO BARs on PCI, which you can consider
> broken in the case where a card actually requires PIO.
>
> On tile, it would be just as broken as other drivers that directly use
> inb/outb to do the port access, but on platforms that actually have
> PIO but are not able to map them (like arm/mach-rpc), you would now
> enable people to write silently broken drivers.
>
> However, assuming they check the return value of the *iomap, the
> drivers could still easily detect this case at runtime. Also, at
> least on tile, you already provide a pci_iomap function, which
> has the exact same problem as the devres functions.
>
> One area where your patch might actually break existing code is
> architectures that allow the devres functions to be built *and*
> have PCI support but don't provide a pci_iomap function, because
> they do not support mapping the PIO range into addresses. I'm
> not sure if that's actually still the case anywhere after Michael's
> recent patches, but it used to be the case on RPC and probably others.

Sounds sufficiently subtle that I'll plan to drop the patch unless someone
else (maybe Al Viro, now cc'ed) wants to weigh in.  For tile this just
means we'll continue not to set NO_IOPORT, at the cost of a few hundred
bytes of dead code; no big deal.

The specific issue I saw was calls to pcim_iomap_{regions,table} from
drivers/ata/libata-sff.c and drivers/ata/sata_sil24.c; we use the
sata_sil24 in one of our platforms (TILEmpower).

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH] lib/devres.c: allow specifying NO_IOPORT while using PCI

[Arnd Bergmann]

On Friday 02 December 2011, Chris Metcalf wrote:
> The specific issue I saw was calls to pcim_iomap_{regions,table} from
> drivers/ata/libata-sff.c and drivers/ata/sata_sil24.c; we use the
> sata_sil24 in one of our platforms (TILEmpower).

Hmm, so sata_sil24 does not actually use PIO, so we can probably find a
way to make that work. Why do you have to build libata-sff? Shouldn't
that only be needed if you actually have an sff-type controller?

	Arnd

Re: [PATCH] lib/devres.c: allow specifying NO_IOPORT while using PCI

[Chris Metcalf]

On 12/5/2011 10:14 AM, Arnd Bergmann wrote:
> On Friday 02 December 2011, Chris Metcalf wrote:
>> The specific issue I saw was calls to pcim_iomap_{regions,table} from
>> drivers/ata/libata-sff.c and drivers/ata/sata_sil24.c; we use the
>> sata_sil24 in one of our platforms (TILEmpower).
> Hmm, so sata_sil24 does not actually use PIO, so we can probably find a
> way to make that work.

OK, that would be good.  I'm not familiar enough with the driver (or indeed
the devres stuff) to really take an effective swing at this myself, though.

> Why do you have to build libata-sff? Shouldn't
> that only be needed if you actually have an sff-type controller?

Yes, I think this is an accidental legacy of the original development work
for PCI root complex three years ago.  I disabled ATA_SFF and booted up a
number of our inhouse platforms, and all the devices appear to work fine
without it.  I've now turned it off in our standard configuration.

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH] lib/devres.c: allow specifying NO_IOPORT while using PCI

[Chris Metcalf]

On 12/5/2011 10:14 AM, Arnd Bergmann wrote:
> On Friday 02 December 2011, Chris Metcalf wrote:
>> The specific issue I saw was calls to pcim_iomap_{regions,table} from
>> drivers/ata/libata-sff.c and drivers/ata/sata_sil24.c; we use the
>> sata_sil24 in one of our platforms (TILEmpower).
> Hmm, so sata_sil24 does not actually use PIO, so we can probably find a
> way to make that work.

OK, that would be good.  I'm not familiar enough with the driver (or indeed
the devres stuff) to really take an effective swing at this myself, though.

> Why do you have to build libata-sff? Shouldn't
> that only be needed if you actually have an sff-type controller?

Yes, I think this is an accidental legacy of the original development work
for PCI root complex three years ago.  I disabled ATA_SFF and booted up a
number of our inhouse platforms, and all the devices appear to work fine
without it.  I've now turned it off in our standard configuration.

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com