| Message ID | 1394020150-1875-2-git-send-email-Liviu.Dudau@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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)
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. >
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
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.
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 >
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 --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)