diff mbox

[v6,1/3] Fix ioport_map() for !CONFIG_GENERIC_IOMAP cases.

Message ID 1394020150-1875-2-git-send-email-Liviu.Dudau@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Liviu Dudau March 5, 2014, 11:49 a.m. UTC
The inline version of ioport_map() that gets used when !CONFIG_GENERIC_IOMAP
is wrong. It returns a mapped (i.e. virtual) address that can start from
zero and completely ignores the PCI_IOBASE and IO_SPACE_LIMIT that most
architectures that use !CONFIG_GENERIC_MAP define.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Tested-by: Tanmay Inamdar <tinamdar@apm.com>
---
 include/asm-generic/io.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Russell King - ARM Linux March 5, 2014, 11:31 p.m. UTC | #1
On Wed, Mar 05, 2014 at 11:49:08AM +0000, Liviu Dudau wrote:
> The inline version of ioport_map() that gets used when !CONFIG_GENERIC_IOMAP
> is wrong. It returns a mapped (i.e. virtual) address that can start from
> zero and completely ignores the PCI_IOBASE and IO_SPACE_LIMIT that most
> architectures that use !CONFIG_GENERIC_MAP define.

What value does PCI_IOBASE and IO_SPACE_LIMIT have on other architectures
who make use of asm-generic/io.h ?

$ git grep asm-generic/io.h arch/
arch/arc/include/asm/io.h:#include <asm-generic/io.h>
arch/blackfin/include/asm/io.h:#include <asm-generic/io.h>
arch/metag/include/asm/io.h:#include <asm-generic/io.h>
arch/microblaze/include/asm/io.h:/* from asm-generic/io.h */
arch/openrisc/include/asm/io.h:#include <asm-generic/io.h>
arch/s390/include/asm/io.h:#include <asm-generic/io.h>
arch/score/include/asm/io.h:#include <asm-generic/io.h>
arch/unicore32/include/asm/io.h:#include <asm-generic/io.h>
arch/xtensa/include/asm/io.h:#include <asm-generic/io.h>
$ arch/arc/include/asm/io.h:#define PCI_IOBASE ((void __iomem *)0)
arch/arm64/include/asm/io.h:#define PCI_IOBASE          ((void __iomem *)(MODULES_VADDR - SZ_2M))
arch/arm64/include/asm/io.h:    return readb(addr + PCI_IOBASE);
arch/arm64/include/asm/io.h:    return readw(addr + PCI_IOBASE);
arch/arm64/include/asm/io.h:    return readl(addr + PCI_IOBASE);
arch/arm64/include/asm/io.h:    writeb(b, addr + PCI_IOBASE);
arch/arm64/include/asm/io.h:    writew(b, addr + PCI_IOBASE);
arch/arm64/include/asm/io.h:    writel(b, addr + PCI_IOBASE);
arch/arm64/include/asm/io.h:            *buf++ = __raw_readb(addr + PCI_IOBASE);arch/arm64/include/asm/io.h:            *buf++ = __raw_readw(addr + PCI_IOBASE);arch/arm64/include/asm/io.h:            *buf++ = __raw_readl(addr + PCI_IOBASE);arch/arm64/include/asm/io.h:            __raw_writeb(*buf++, addr + PCI_IOBASE);arch/arm64/include/asm/io.h:            __raw_writew(*buf++, addr + PCI_IOBASE);arch/arm64/include/asm/io.h:            __raw_writel(*buf++, addr + PCI_IOBASE);arch/unicore32/include/asm/io.h:#define PCI_IOBASE      PKUNITY_PCILIO_BASE
arch/unicore32/include/asm/io.h:#define PIO_OFFSET              (unsigned int)(PCI_IOBASE)
Liviu Dudau March 6, 2014, 10:36 a.m. UTC | #2
On Wed, Mar 05, 2014 at 11:31:35PM +0000, Russell King - ARM Linux wrote:
> On Wed, Mar 05, 2014 at 11:49:08AM +0000, Liviu Dudau wrote:
> > The inline version of ioport_map() that gets used when !CONFIG_GENERIC_IOMAP
> > is wrong. It returns a mapped (i.e. virtual) address that can start from
> > zero and completely ignores the PCI_IOBASE and IO_SPACE_LIMIT that most
> > architectures that use !CONFIG_GENERIC_MAP define.
> 
> What value does PCI_IOBASE and IO_SPACE_LIMIT have on other architectures
> who make use of asm-generic/io.h ?

Hi Russell,

Sorry for being a bit opaque in the commit message, I probably conflated two
issues into one. The first issue is that ioport_map() is supposed to return
a virtual address for the IO port. I struggle to believe that a virtual address
of zero for IO is valid for most architectures other than x86. My guess is that
most of the architectures that you have listed as including asm-generic/io.h
have no support for PCI whatsoever. The other issue is that *if* you specify an
PCI_IOBASE and don't come up with your own version of ioport_map() then you
get the wrong virtual addresses back.

One way to fix all this is to use PCI_IOBASE inside the generic version and to
define it to a non-zero values for architectures that have memory mapped IO.
That should reduce the number of custom versions of ioport_map() that we currently
have.

The other implied message that I'm getting is that you are suggesting that the
commit message is generalising a bit too much? With that I agree, in light of
your analysis. I can change it to something like:

   The inline version of ioport_map() that gets used for !CONFIG_GENERIC_IOMAP is
   wrong when PCI_IOBASE has a non-zero value. The function is supposed to return
   a virtual address for IO ports and for architectures that memory map the IO
   areas that is giving incorrect results as it ignores PCI_IOBASE. Fix this and
   also limit the port range to the IO_SPACE_LIMIT mask.

> 
> $ git grep asm-generic/io.h arch/
> arch/arc/include/asm/io.h:#include <asm-generic/io.h>
> arch/blackfin/include/asm/io.h:#include <asm-generic/io.h>
> arch/metag/include/asm/io.h:#include <asm-generic/io.h>
> arch/microblaze/include/asm/io.h:/* from asm-generic/io.h */
> arch/openrisc/include/asm/io.h:#include <asm-generic/io.h>
> arch/s390/include/asm/io.h:#include <asm-generic/io.h>
> arch/score/include/asm/io.h:#include <asm-generic/io.h>
> arch/unicore32/include/asm/io.h:#include <asm-generic/io.h>

PCI_IOBASE = PKUNITY_PCILIO_BASE = PKUNITY_PCI_BASE + 0x00030000 = 
    io_p2v(0x80000000) + 0x00030000

All other ones bar arm64 are either happy with PCI_IOBASE being zero (proxy for
"no PCI support") or define their own version of ioport_map().

Best regards,
Liviu

> arch/xtensa/include/asm/io.h:#include <asm-generic/io.h>
> $ arch/arc/include/asm/io.h:#define PCI_IOBASE ((void __iomem *)0)
> arch/arm64/include/asm/io.h:#define PCI_IOBASE          ((void __iomem *)(MODULES_VADDR - SZ_2M))
> arch/arm64/include/asm/io.h:    return readb(addr + PCI_IOBASE);
> arch/arm64/include/asm/io.h:    return readw(addr + PCI_IOBASE);
> arch/arm64/include/asm/io.h:    return readl(addr + PCI_IOBASE);
> arch/arm64/include/asm/io.h:    writeb(b, addr + PCI_IOBASE);
> arch/arm64/include/asm/io.h:    writew(b, addr + PCI_IOBASE);
> arch/arm64/include/asm/io.h:    writel(b, addr + PCI_IOBASE);
> arch/arm64/include/asm/io.h:            *buf++ = __raw_readb(addr + PCI_IOBASE);
> arch/arm64/include/asm/io.h:            *buf++ = __raw_readw(addr + PCI_IOBASE);
> arch/arm64/include/asm/io.h:            *buf++ = __raw_readl(addr + PCI_IOBASE);
> arch/arm64/include/asm/io.h:            __raw_writeb(*buf++, addr + PCI_IOBASE);
> arch/arm64/include/asm/io.h:            __raw_writew(*buf++, addr + PCI_IOBASE);
> arch/arm64/include/asm/io.h:            __raw_writel(*buf++, addr + PCI_IOBASE);
> arch/unicore32/include/asm/io.h:#define PCI_IOBASE      PKUNITY_PCILIO_BASE
> arch/unicore32/include/asm/io.h:#define PIO_OFFSET              (unsigned int)(PCI_IOBASE)
> 
> 
> -- 
> FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
> improving, and getting towards what was expected from it.
>
Arnd Bergmann March 7, 2014, 12:37 a.m. UTC | #3
On Thursday 06 March 2014, Russell King - ARM Linux wrote:
> On Wed, Mar 05, 2014 at 11:49:08AM +0000, Liviu Dudau wrote:
> > The inline version of ioport_map() that gets used when !CONFIG_GENERIC_IOMAP
> > is wrong. It returns a mapped (i.e. virtual) address that can start from
> > zero and completely ignores the PCI_IOBASE and IO_SPACE_LIMIT that most
> > architectures that use !CONFIG_GENERIC_MAP define.
> 
> What value does PCI_IOBASE and IO_SPACE_LIMIT have on other architectures
> who make use of asm-generic/io.h ?
> 
> $ git grep asm-generic/io.h arch/
> arch/arc/include/asm/io.h:#include <asm-generic/io.h>

PCI support hasn't been upstreamed.

> arch/blackfin/include/asm/io.h:#include <asm-generic/io.h>
> arch/metag/include/asm/io.h:#include <asm-generic/io.h>

No PCI support

> arch/microblaze/include/asm/io.h:/* from asm-generic/io.h */

PCI_IOBASE=0, IO_SPACE_LIMIT=0xffffffff, so no change.

> arch/openrisc/include/asm/io.h:#include <asm-generic/io.h>

No PCI support

> arch/s390/include/asm/io.h:#include <asm-generic/io.h>

s390 supports PCI but no I/O space

> arch/score/include/asm/io.h:#include <asm-generic/io.h>

No PCI support

> arch/unicore32/include/asm/io.h:#include <asm-generic/io.h>

unicore32 is broken currently, the patch fixes it.

> arch/xtensa/include/asm/io.h:#include <asm-generic/io.h>

PCI_IOBASE=0, IO_SPACE_LIMIT=0xffffffff, so no change.

For most of these, I assume we actually want to remove support
for inb/outb as they don't support I/O space accesses. The other
ones look correct to me.

	Arnd
Russell King - ARM Linux March 7, 2014, 1:09 a.m. UTC | #4
On Fri, Mar 07, 2014 at 01:37:38AM +0100, Arnd Bergmann wrote:
> On Thursday 06 March 2014, Russell King - ARM Linux wrote:
> > On Wed, Mar 05, 2014 at 11:49:08AM +0000, Liviu Dudau wrote:
> > > The inline version of ioport_map() that gets used when !CONFIG_GENERIC_IOMAP
> > > is wrong. It returns a mapped (i.e. virtual) address that can start from
> > > zero and completely ignores the PCI_IOBASE and IO_SPACE_LIMIT that most
> > > architectures that use !CONFIG_GENERIC_MAP define.
> > 
> > What value does PCI_IOBASE and IO_SPACE_LIMIT have on other architectures
> > who make use of asm-generic/io.h ?
> > 
> > $ git grep asm-generic/io.h arch/
> > arch/arc/include/asm/io.h:#include <asm-generic/io.h>
> 
> PCI support hasn't been upstreamed.
> 
> > arch/blackfin/include/asm/io.h:#include <asm-generic/io.h>
> > arch/metag/include/asm/io.h:#include <asm-generic/io.h>
> 
> No PCI support
> 
> > arch/microblaze/include/asm/io.h:/* from asm-generic/io.h */
> 
> PCI_IOBASE=0, IO_SPACE_LIMIT=0xffffffff, so no change.

Seems to define _IO_BASE not PCI_IOBASE.

> > arch/openrisc/include/asm/io.h:#include <asm-generic/io.h>
> 
> No PCI support
> 
> > arch/s390/include/asm/io.h:#include <asm-generic/io.h>
> 
> s390 supports PCI but no I/O space
> 
> > arch/score/include/asm/io.h:#include <asm-generic/io.h>
> 
> No PCI support
> 
> > arch/unicore32/include/asm/io.h:#include <asm-generic/io.h>
> 
> unicore32 is broken currently, the patch fixes it.
> 
> > arch/xtensa/include/asm/io.h:#include <asm-generic/io.h>
> 
> PCI_IOBASE=0, IO_SPACE_LIMIT=0xffffffff, so no change.

Doesn't appear to define PCI_IOBASE.

Maybe there's other patches required for these?

> For most of these, I assume we actually want to remove support
> for inb/outb as they don't support I/O space accesses. The other
> ones look correct to me.

Right, so:

#ifdef CONFIG_HAS_IOPORT
#ifndef CONFIG_GENERIC_IOMAP
static inline void __iomem *ioport_map(unsigned long port, unsigned int nr)
{
        return (void __iomem *) port;
}

changing that to include PCI_IOBASE in there will result in a build
failure if the C compiler sees that.  In other words, when HAS_IOPORT=y
and GENERIC_IOMAP=n.

HAS_IOPORT is set when HAS_IOMEM is also set and NO_IOPORT unset.

It looks to me like blackfin doesn't set NO_IOPORT nor NO_IOMEM, so
this would have HAS_IOPORT set, and from what I can see doesn't set
GENERIC_IOMAP.  So, this change probably breaks blackfin.

I haven't looked deeply at the others.
Liviu Dudau March 7, 2014, 1:44 a.m. UTC | #5
On Fri, Mar 07, 2014 at 01:09:50AM +0000, Russell King - ARM Linux wrote:
> On Fri, Mar 07, 2014 at 01:37:38AM +0100, Arnd Bergmann wrote:
> > On Thursday 06 March 2014, Russell King - ARM Linux wrote:
> > > On Wed, Mar 05, 2014 at 11:49:08AM +0000, Liviu Dudau wrote:
> > > > The inline version of ioport_map() that gets used when !CONFIG_GENERIC_IOMAP
> > > > is wrong. It returns a mapped (i.e. virtual) address that can start from
> > > > zero and completely ignores the PCI_IOBASE and IO_SPACE_LIMIT that most
> > > > architectures that use !CONFIG_GENERIC_MAP define.
> > > 
> > > What value does PCI_IOBASE and IO_SPACE_LIMIT have on other architectures
> > > who make use of asm-generic/io.h ?
> > > 
> > > $ git grep asm-generic/io.h arch/
> > > arch/arc/include/asm/io.h:#include <asm-generic/io.h>
> > 
> > PCI support hasn't been upstreamed.
> > 
> > > arch/blackfin/include/asm/io.h:#include <asm-generic/io.h>
> > > arch/metag/include/asm/io.h:#include <asm-generic/io.h>
> > 
> > No PCI support
> > 
> > > arch/microblaze/include/asm/io.h:/* from asm-generic/io.h */
> > 
> > PCI_IOBASE=0, IO_SPACE_LIMIT=0xffffffff, so no change.
> 
> Seems to define _IO_BASE not PCI_IOBASE.
> 
> > > arch/openrisc/include/asm/io.h:#include <asm-generic/io.h>
> > 
> > No PCI support
> > 
> > > arch/s390/include/asm/io.h:#include <asm-generic/io.h>
> > 
> > s390 supports PCI but no I/O space
> > 
> > > arch/score/include/asm/io.h:#include <asm-generic/io.h>
> > 
> > No PCI support
> > 
> > > arch/unicore32/include/asm/io.h:#include <asm-generic/io.h>
> > 
> > unicore32 is broken currently, the patch fixes it.
> > 
> > > arch/xtensa/include/asm/io.h:#include <asm-generic/io.h>
> > 
> > PCI_IOBASE=0, IO_SPACE_LIMIT=0xffffffff, so no change.
> 
> Doesn't appear to define PCI_IOBASE.
> 
> Maybe there's other patches required for these?
> 
> > For most of these, I assume we actually want to remove support
> > for inb/outb as they don't support I/O space accesses. The other
> > ones look correct to me.
> 
> Right, so:
> 
> #ifdef CONFIG_HAS_IOPORT
> #ifndef CONFIG_GENERIC_IOMAP
> static inline void __iomem *ioport_map(unsigned long port, unsigned int nr)
> {
>         return (void __iomem *) port;
> }
> 
> changing that to include PCI_IOBASE in there will result in a build
> failure if the C compiler sees that.  In other words, when HAS_IOPORT=y
> and GENERIC_IOMAP=n.
> 
> HAS_IOPORT is set when HAS_IOMEM is also set and NO_IOPORT unset.
> 
> It looks to me like blackfin doesn't set NO_IOPORT nor NO_IOMEM, so
> this would have HAS_IOPORT set, and from what I can see doesn't set
> GENERIC_IOMAP.  So, this change probably breaks blackfin.
> 
> I haven't looked deeply at the others.

Maybe I'm missing something obvious, but line 118 in include/asm-generic/io.h has:

#ifndef PCI_IOBASE
#define PCI_IOBASE ((void __iomem *)0)
#endif

That doesn't seem to be guarded by any other #ifdef (other than the overall
__ASM_GENERIC_IO_H), so PCI_IOBASE is defined regardless of CONFIG_HAS_IOPORT and 
CONFIG_GENERIC_IOMAP values.

Best regards,
Liviu

> 
> -- 
> FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
> improving, and getting towards what was expected from it.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Arnd Bergmann March 14, 2014, 11:54 a.m. UTC | #6
On Friday 07 March 2014, Russell King - ARM Linux wrote:
> On Fri, Mar 07, 2014 at 01:37:38AM +0100, Arnd Bergmann wrote:
> > On Thursday 06 March 2014, Russell King - ARM Linux wrote:

> > > arch/microblaze/include/asm/io.h:/* from asm-generic/io.h */
> > 
> > PCI_IOBASE=0, IO_SPACE_LIMIT=0xffffffff, so no change.
> 
> Seems to define _IO_BASE not PCI_IOBASE.

It gets the implicit PCI_IOBASE from asm-generic/io.h at the moment.

> > 
> > PCI_IOBASE=0, IO_SPACE_LIMIT=0xffffffff, so no change.
> 
> Doesn't appear to define PCI_IOBASE.

Same here.

> > For most of these, I assume we actually want to remove support
> > for inb/outb as they don't support I/O space accesses. The other
> > ones look correct to me.
> 
> Right, so:
> 
> #ifdef CONFIG_HAS_IOPORT
> #ifndef CONFIG_GENERIC_IOMAP
> static inline void __iomem *ioport_map(unsigned long port, unsigned int nr)
> {
>         return (void __iomem *) port;
> }
> 
> changing that to include PCI_IOBASE in there will result in a build
> failure if the C compiler sees that.  In other words, when HAS_IOPORT=y
> and GENERIC_IOMAP=n.
> 
> HAS_IOPORT is set when HAS_IOMEM is also set and NO_IOPORT unset.
> 
> It looks to me like blackfin doesn't set NO_IOPORT nor NO_IOMEM, so
> this would have HAS_IOPORT set, and from what I can see doesn't set
> GENERIC_IOMAP.  So, this change probably breaks blackfin.

I also see the same thing that Liviu mentioned, that PCI_IOBASE=0
is always provided by asm-generic/io.h if not set otherwise.

On a related topic, Uwe Kleine-König has submitted a patch to
rename CONFIG_HAS_IOPORT to CONFIG_HAS_IOPORT_MAP to clarify what
it does, and to allow us to add a new CONFIG_HAS_IOPORT option
that will let us remove all the I/O port handling code for
architectures that don't have any I/O access method.

	Arnd
diff mbox

Patch

diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index d5afe96..df72051 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -331,7 +331,7 @@  static inline void iounmap(void __iomem *addr)
 #ifndef CONFIG_GENERIC_IOMAP
 static inline void __iomem *ioport_map(unsigned long port, unsigned int nr)
 {
-	return (void __iomem *) port;
+	return (void __iomem *)(PCI_IOBASE + (port & IO_SPACE_LIMIT));
 }
 
 static inline void ioport_unmap(void __iomem *p)