[RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Thomas Petazzoni]

The pcim_*() functions are used by the libata-sff subsystem, and this
subsystem is used for many SATA drivers on ARM platforms that do not
necessarily have I/O ports.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: linux-kernel@vger.kernel.org
---
 lib/devres.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/devres.c b/lib/devres.c
index 80b9c76..5639c3e 100644
--- a/lib/devres.c
+++ b/lib/devres.c
@@ -195,6 +195,7 @@ void devm_ioport_unmap(struct device *dev, void __iomem *addr)
 			       devm_ioport_map_match, (void *)addr));
 }
 EXPORT_SYMBOL(devm_ioport_unmap);
+#endif /* CONFIG_HAS_IOPORT */
 
 #ifdef CONFIG_PCI
 /*
@@ -400,4 +401,3 @@ void pcim_iounmap_regions(struct pci_dev *pdev, int mask)
 }
 EXPORT_SYMBOL(pcim_iounmap_regions);
 #endif /* CONFIG_PCI */
-#endif /* CONFIG_HAS_IOPORT */
-- 
1.7.9.5

Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Arnd Bergmann]

On Friday 07 December 2012, Thomas Petazzoni wrote:
> The pcim_*() functions are used by the libata-sff subsystem, and this
> subsystem is used for many SATA drivers on ARM platforms that do not
> necessarily have I/O ports.

I think this one is wrong as the CONFIG_HAS_IOPORT does not refer to the
presence of PIO ports but to whether or not they provide an ioport_map
function. If there is no ioport_map(), devm_pci_iomap will fail to link
as far as I can tell.

	Arnd

> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Cc: Yinghai Lu <yinghai@kernel.org>
> Cc: linux-kernel@vger.kernel.org
> ---
>  lib/devres.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/devres.c b/lib/devres.c
> index 80b9c76..5639c3e 100644
> --- a/lib/devres.c
> +++ b/lib/devres.c
> @@ -195,6 +195,7 @@ void devm_ioport_unmap(struct device *dev, void __iomem *addr)
>  			       devm_ioport_map_match, (void *)addr));
>  }
>  EXPORT_SYMBOL(devm_ioport_unmap);
> +#endif /* CONFIG_HAS_IOPORT */
>  
>  #ifdef CONFIG_PCI
>  /*
> @@ -400,4 +401,3 @@ void pcim_iounmap_regions(struct pci_dev *pdev, int mask)
>  }
>  EXPORT_SYMBOL(pcim_iounmap_regions);
>  #endif /* CONFIG_PCI */
> -#endif /* CONFIG_HAS_IOPORT */

Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Thomas Petazzoni]

Dear Arnd Bergmann,

On Tue, 11 Dec 2012 10:43:49 +0000, Arnd Bergmann wrote:
> On Friday 07 December 2012, Thomas Petazzoni wrote:
> > The pcim_*() functions are used by the libata-sff subsystem, and this
> > subsystem is used for many SATA drivers on ARM platforms that do not
> > necessarily have I/O ports.
> 
> I think this one is wrong as the CONFIG_HAS_IOPORT does not refer to the
> presence of PIO ports but to whether or not they provide an ioport_map
> function. If there is no ioport_map(), devm_pci_iomap will fail to link
> as far as I can tell.

The problem is that on ARCH_MULTI_V7, ARCH_VEXPRESS is forcefully
enabled. And ARCH_VEXPRESS selects NO_IOPORT.. so you don't have the
pcim_*() functions, and therefore libata-sff.c (needed for many SATA
drivers) will not build. How do you solve this?

I'm not sure which devm_pci_iomap() you're referring to since my patch
makes only the pcim_iomap_table(), pcim_iomap(), pcim_iounmap(),
pcim_iomap_regions(), pcim_iomap_regions_request_all() and
pcim_iounmap_regions() available under CONFIG_PCI instead of CONFIG_PCI
&& CONFIG_HAS_IOPORT.

So maybe you were referring to pcim_iomap(). I haven't checked in
details, but I guess it builds because ioport_map() is implemented in
arch/arm/mm/iomap.c.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Arnd Bergmann]

On Tuesday 11 December 2012, Thomas Petazzoni wrote:
> On Tue, 11 Dec 2012 10:43:49 +0000, Arnd Bergmann wrote:
> > On Friday 07 December 2012, Thomas Petazzoni wrote:
> > > The pcim_*() functions are used by the libata-sff subsystem, and this
> > > subsystem is used for many SATA drivers on ARM platforms that do not
> > > necessarily have I/O ports.
> > 
> > I think this one is wrong as the CONFIG_HAS_IOPORT does not refer to the
> > presence of PIO ports but to whether or not they provide an ioport_map
> > function. If there is no ioport_map(), devm_pci_iomap will fail to link
> > as far as I can tell.
> 
> The problem is that on ARCH_MULTI_V7, ARCH_VEXPRESS is forcefully
> enabled. And ARCH_VEXPRESS selects NO_IOPORT.. so you don't have the
> pcim_*() functions, and therefore libata-sff.c (needed for many SATA
> drivers) will not build. How do you solve this?

What you describe here are probable two bugs, and we should fix both:

* ARCH_VEXPRESS should not select NO_IOPORT. It's generally wrong
  to select this in combination with ARCH_MULTIPLATFORM, when some
  of the other platforms you may enable actually have IOPORT mapping
  support.

* We should not unconditionally select ARCH_VEXPRESS from ARCH_MULTIPLATFORM.
  There is no reason why we would enable that platform for building
  a kernel that runs on only one other platform.

> I'm not sure which devm_pci_iomap() you're referring to since my patch
> makes only the pcim_iomap_table(), pcim_iomap(), pcim_iounmap(),
> pcim_iomap_regions(), pcim_iomap_regions_request_all() and
> pcim_iounmap_regions() available under CONFIG_PCI instead of CONFIG_PCI
> && CONFIG_HAS_IOPORT.

Sorry, I meant pcim_iomap.

> So maybe you were referring to pcim_iomap(). I haven't checked in
> details, but I guess it builds because ioport_map() is implemented in
> arch/arm/mm/iomap.c.

Right. If an ioport_map function is provided for a given platform,
we should also set HAVE_IOPORT and vice versa. This is probably
fallout of the io.h conversion for multiplatform.

	Arnd

Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Russell King - ARM Linux]

On Tue, Dec 11, 2012 at 04:15:02PM +0000, Arnd Bergmann wrote:
> On Tuesday 11 December 2012, Thomas Petazzoni wrote:
> > On Tue, 11 Dec 2012 10:43:49 +0000, Arnd Bergmann wrote:
> > > On Friday 07 December 2012, Thomas Petazzoni wrote:
> > > > The pcim_*() functions are used by the libata-sff subsystem, and this
> > > > subsystem is used for many SATA drivers on ARM platforms that do not
> > > > necessarily have I/O ports.
> > > 
> > > I think this one is wrong as the CONFIG_HAS_IOPORT does not refer to the
> > > presence of PIO ports but to whether or not they provide an ioport_map
> > > function. If there is no ioport_map(), devm_pci_iomap will fail to link
> > > as far as I can tell.
> > 
> > The problem is that on ARCH_MULTI_V7, ARCH_VEXPRESS is forcefully
> > enabled. And ARCH_VEXPRESS selects NO_IOPORT.. so you don't have the
> > pcim_*() functions, and therefore libata-sff.c (needed for many SATA
> > drivers) will not build. How do you solve this?
> 
> What you describe here are probable two bugs, and we should fix both:
> 
> * ARCH_VEXPRESS should not select NO_IOPORT. It's generally wrong
>   to select this in combination with ARCH_MULTIPLATFORM, when some
>   of the other platforms you may enable actually have IOPORT mapping
>   support.

No.  ARCH_VEXPRESS selects NO_IOPORT because it _does_ _not_ support
PCI/ISA IO space.  That in itself is reasonable, but what isn't
reasonable is the _negative_ logic being used.  Negative logic in
the config system always tends to provoke this kind of sillyness
because you're selecting something to be excluded which another
platform may require.

Positive logic is the only way to deal with this.

We should instead have HAVE_IOPORT and platforms which need ISA/PCI IO
space support should select this symbol instead - so it becomes an
_inclusive_ feature rather than an _exclusive_ feature.

Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Thomas Petazzoni]

Russell,

On Tue, 11 Dec 2012 16:23:25 +0000, Russell King - ARM Linux wrote:

> > * ARCH_VEXPRESS should not select NO_IOPORT. It's generally wrong
> >   to select this in combination with ARCH_MULTIPLATFORM, when some
> >   of the other platforms you may enable actually have IOPORT mapping
> >   support.
> 
> No.  ARCH_VEXPRESS selects NO_IOPORT because it _does_ _not_ support
> PCI/ISA IO space.  That in itself is reasonable, but what isn't
> reasonable is the _negative_ logic being used.  Negative logic in
> the config system always tends to provoke this kind of sillyness
> because you're selecting something to be excluded which another
> platform may require.

Could you enlighten my very naive understanding of things about PCI/ISA
IO space? On x86, I seem to understand this is the separate address
space accessed by the special in/out CPU instructions. Are there ARM
platforms with the same sort of things?

As far as I understand, on my ARM Marvell system, everything is
memory-mapped, so there isn't such a separate PCI/ISA IO space.

Therefore, why would I need to "select HAVE_IOPORT" simply to be able
to build libata-sff.c, that is used for PCI drivers that work fine with
purely memory-mapped registers?

Sorry for the stupid/naive questions, but it'll definitely help to
understand the matter.

Thanks,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Russell King - ARM Linux]

On Tue, Dec 11, 2012 at 05:38:19PM +0100, Thomas Petazzoni wrote:
> Russell,
> 
> On Tue, 11 Dec 2012 16:23:25 +0000, Russell King - ARM Linux wrote:
> 
> > > * ARCH_VEXPRESS should not select NO_IOPORT. It's generally wrong
> > >   to select this in combination with ARCH_MULTIPLATFORM, when some
> > >   of the other platforms you may enable actually have IOPORT mapping
> > >   support.
> > 
> > No.  ARCH_VEXPRESS selects NO_IOPORT because it _does_ _not_ support
> > PCI/ISA IO space.  That in itself is reasonable, but what isn't
> > reasonable is the _negative_ logic being used.  Negative logic in
> > the config system always tends to provoke this kind of sillyness
> > because you're selecting something to be excluded which another
> > platform may require.
> 
> Could you enlighten my very naive understanding of things about PCI/ISA
> IO space? On x86, I seem to understand this is the separate address
> space accessed by the special in/out CPU instructions. Are there ARM
> platforms with the same sort of things?
> 
> As far as I understand, on my ARM Marvell system, everything is
> memory-mapped, so there isn't such a separate PCI/ISA IO space.
> 
> Therefore, why would I need to "select HAVE_IOPORT" simply to be able
> to build libata-sff.c, that is used for PCI drivers that work fine with
> purely memory-mapped registers?

- PCI buses make the distinction between memory accesses and IO accesses.
- ARM doesn't have IO access instructions; everything is memory mapped.
- In order to make the two work together, whatever PCI interface provides
  a way to issue IO accesses via a contiguous memory mapped region.
- The PCI layer and drivers are written to keep these two spaces separate
  so that they remain portable across multiple different platforms.

Therefore, if you have PCI/ISA drivers that make use of IO space, you need
to provide the IO space accessors and mapping functions so that these
drivers can access the IO space on their devices in a platform independent
manner.

Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Alan Cox]

> Could you enlighten my very naive understanding of things about PCI/ISA
> IO space? On x86, I seem to understand this is the separate address
> space accessed by the special in/out CPU instructions. Are there ARM
> platforms with the same sort of things?

An x86 processor has two address spaces

A memory space
An I/O space

They are both visible to the PCI bus and both transaction types are
present on the bus and distinct.

> As far as I understand, on my ARM Marvell system, everything is
> memory-mapped, so there isn't such a separate PCI/ISA IO space.

There is if you ask the PCI bridge or beyond.

> Therefore, why would I need to "select HAVE_IOPORT" simply to be able
> to build libata-sff.c, that is used for PCI drivers that work fine with
> purely memory-mapped registers?

Because the drivers talk to the hardware beyond the bridge which knows
the difference.


So on x86 we have

	inb(0xf00);

	generates an I/O transaction on the PCI bus for I/O 0xf00

	*0xf00

	generates a memory transacion on the PCI bus for memory 0xf00
	and the two are *not* the same thing or address

On many other platforms we have 

	inb(0xf00)

	actually produces the equivalent of (*io_window+0xf00)

	which the PCI bridge turns into an I/O transaction for 0xf00

	(and for the out case may have an explicit barrier needed to get
	the I/O ordering rules)

	while

	*0xf00

	gets turned by the bridge into a memory transaction for 0xf00



The "no I/O space" case really applies to things like the S/390 mainframe
which simply have no such concept on the system or the devices. In the
ARM case the bus has an I/O space and the bridge glues the processors
simpler model to the bridge model.

For compatibility reasons (so you an boot old software) the PCI bus also
has some automatic magic mappings in I/O space, and a lot of LPC bus
devices also have magic I/O mappings. For example a SATA controller in
compatibility mode appears at a specified I/O address on the bus and is
always on a specific IRQ (matching the old ISA PC mappings which map the
old ISA MFM controller mappings whose offsets map compatiblity to the
register mappings for the WD MFM controller used on the PC/AT)

In PC space this stuff is important and it's why up until Windows 7 era
PC hardware you can boot DOS, as well as being why you can have one
generic kernel for almost any PC hardware instead of the one per board
problem some other platforms have.

Alan

Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Arnd Bergmann]

On Tuesday 11 December 2012, Alan Cox wrote:
> The "no I/O space" case really applies to things like the S/390 mainframe
> which simply have no such concept on the system or the devices. In the
> ARM case the bus has an I/O space and the bridge glues the processors
> simpler model to the bridge model.

While we are getting slightly offtopic, s390 is actually gaining PCI support
now: https://patchwork.kernel.org/patch/1740231/ For all I can tell, they
would theoretically support an I/O space, but none of the supported
add-on cards use it, so the kernel implementation doesn't need to bother.

One architecture that never has an I/O space is the arch/um, but that is
special in a lot of ways, e.g. since it also never has MMIO.

On ARM, we have platforms that fall into four categories:

1. Full PCI or PCMCIA or ISA support with directly mapped I/O space

2. No PCI or PCMCIA or ISA support, and consequently no I/O space

3. PCI support but no I/O space because of limitations or bugs in
   the PCI hardware implementation.

4. ISA-style I/O space that is not offsettable (CONFIG_NO_IOPORT)
   but still has inb/outb accessors.

For cases 2 and 3, we can undefine the __io() macro, which leads to
intentional build errors someone tries to build code that uses the
inb/outb accessors. One missing piece that I have been working on
in the past and been meaning to pick up again is a patch set to
globally rename CONFIG_NO_IOPORT to the more appropriate
CONFIG_NO_IOPORT_MAP, and introduce a new CONFIG_NO_IOPORT that
signifies whether inb/outb are supported or not, rather than whether
you can access the I/O ports through ioport_map() and ioread/iowrite.

	Arnd

Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Arnd Bergmann]

On Tuesday 11 December 2012, Russell King - ARM Linux wrote:
> 
> On Tue, Dec 11, 2012 at 04:15:02PM +0000, Arnd Bergmann wrote:
> > On Tuesday 11 December 2012, Thomas Petazzoni wrote:
> > > On Tue, 11 Dec 2012 10:43:49 +0000, Arnd Bergmann wrote:
> > > > On Friday 07 December 2012, Thomas Petazzoni wrote:
> > > > > The pcim_*() functions are used by the libata-sff subsystem, and this
> > > > > subsystem is used for many SATA drivers on ARM platforms that do not
> > > > > necessarily have I/O ports.
> > > > 
> > > > I think this one is wrong as the CONFIG_HAS_IOPORT does not refer to the
> > > > presence of PIO ports but to whether or not they provide an ioport_map
> > > > function. If there is no ioport_map(), devm_pci_iomap will fail to link
> > > > as far as I can tell.
> > > 
> > > The problem is that on ARCH_MULTI_V7, ARCH_VEXPRESS is forcefully
> > > enabled. And ARCH_VEXPRESS selects NO_IOPORT.. so you don't have the
> > > pcim_*() functions, and therefore libata-sff.c (needed for many SATA
> > > drivers) will not build. How do you solve this?
> > 
> > What you describe here are probable two bugs, and we should fix both:
> > 
> > * ARCH_VEXPRESS should not select NO_IOPORT. It's generally wrong
> >   to select this in combination with ARCH_MULTIPLATFORM, when some
> >   of the other platforms you may enable actually have IOPORT mapping
> >   support.
> 
> No.  ARCH_VEXPRESS selects NO_IOPORT because it does not support
> PCI/ISA IO space.  That in itself is reasonable, but what isn't
> reasonable is the negative logic being used.  Negative logic in
> the config system always tends to provoke this kind of sillyness
> because you're selecting something to be excluded which another
> platform may require.

Exactly, that is what I meant. Sorry if I wasn't clear enough.

> We should instead have HAVE_IOPORT and platforms which need ISA/PCI IO
> space support should select this symbol instead - so it becomes an
> inclusive feature rather than an exclusive feature.

Right. Note that HAS_IOPORT already exists and is defined as

config HAS_IOPORT
        boolean
        depends on HAS_IOMEM && !NO_IOPORT
        default y

If we change it to

config HAS_IOPORT
        boolean
        depends on HAS_IOMEM
        default !NO_IOPORT

then we can actually select both NO_IOPORT and HAS_IOPORT with the result
of getting HAS_IOPORT. It is a bit confusing though to have both enabled,
so we might still want to use an approach where we only select NO_IOPORT
if we are sure that we can't have it.

	Arnd

Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Thomas Petazzoni]

Dear Arnd Bergmann,

On Tue, 11 Dec 2012 16:15:02 +0000, Arnd Bergmann wrote:

> What you describe here are probable two bugs, and we should fix both:
> 
> * ARCH_VEXPRESS should not select NO_IOPORT. It's generally wrong
>   to select this in combination with ARCH_MULTIPLATFORM, when some
>   of the other platforms you may enable actually have IOPORT mapping
>   support.

Indeed, but I guess the "select NO_IOPORT" on vexpress is here for a
reason, no?

That said, unless I don't understand what IOPORTs are, I don't think my
platform has any of them, so why should I "select HAVE_IOPORT" ?

> * We should not unconditionally select ARCH_VEXPRESS from ARCH_MULTIPLATFORM.
>   There is no reason why we would enable that platform for building
>   a kernel that runs on only one other platform.

This one is already being worked on by Fabio Estevam, see [PATCH v2]
ARM: Kconfig: Do not force selection of ARCH_VEXPRESS by ARCH_MULTI_V7.

> > I'm not sure which devm_pci_iomap() you're referring to since my patch
> > makes only the pcim_iomap_table(), pcim_iomap(), pcim_iounmap(),
> > pcim_iomap_regions(), pcim_iomap_regions_request_all() and
> > pcim_iounmap_regions() available under CONFIG_PCI instead of CONFIG_PCI
> > && CONFIG_HAS_IOPORT.
> 
> Sorry, I meant pcim_iomap.
> 
> > So maybe you were referring to pcim_iomap(). I haven't checked in
> > details, but I guess it builds because ioport_map() is implemented in
> > arch/arm/mm/iomap.c.
> 
> Right. If an ioport_map function is provided for a given platform,
> we should also set HAVE_IOPORT and vice versa. This is probably
> fallout of the io.h conversion for multiplatform.

arch/arm/mm/iomap.c is unconditionally compiled in all ARM kernels. And
in this file, ioport_map() and ioport_unmap() are implement as soon as
__io is defined. And basically, in arch/arm/include/asm/io.h, __io is
defined for all platforms, except maybe on some platforms having their
own mach/io.h file, but those are quite limited in number (ebsa110, rpc,
at91, s3c24xx, pxa, omap1, footbridge and ixp4xx). So if __io is
defined, says on VEXPRESS, why does it "select NO_IOPORT" ? Essentially
all ARM platforms should select HAVE_IOPORT, except the few ones that
don't define __io. Correct?

Thanks,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Russell King - ARM Linux]

On Tue, Dec 11, 2012 at 05:30:13PM +0100, Thomas Petazzoni wrote:
> arch/arm/mm/iomap.c is unconditionally compiled in all ARM kernels. And
> in this file, ioport_map() and ioport_unmap() are implement as soon as
> __io is defined. And basically, in arch/arm/include/asm/io.h, __io is
> defined for all platforms

That's an unfortunate side-effect of the single-zImage effort.

>, except maybe on some platforms having their
> own mach/io.h file, but those are quite limited in number (ebsa110, rpc,
> at91, s3c24xx, pxa, omap1, footbridge and ixp4xx). So if __io is
> defined, says on VEXPRESS, why does it "select NO_IOPORT" ? Essentially
> all ARM platforms should select HAVE_IOPORT, except the few ones that
> don't define __io. Correct?

No they damned well should not; it actively _breaks_ at least some of the
platforms you list above.

This whole area is a massive can of worms caused by people over the
years just not understanding what an x86 IO port is and how to deal
with it.  It's very simple.  The IO port space is for ISA/PCMCIA and
PCI IO port regions.  It is nothing more than that.

It's there because when you have a PCI peripheral in your system, you
need to set it up, and the address space used for IO ports is entirely
separate from that used for the rest of the memory mapped IO.  You
can't mix the IO ports amongst the MMIO ports - because most host PCI
bridges will only let you map into MMIO space one region of PCI IO
space.

The ISA case is a little harder to see, but believe me when I say that
in the early days of Linux, we used talk to all ISA chips on platforms
without ISA buses using the inb/outb/etc macros and it turned into one
hell of a disgusting mess.  Now these platforms all use the regular
readb/writeb acros and ioremap() for their accesses which has resulted
in much cleaner solutions - the result of that is these platforms no
longer need the "ISA IO space" region, so providing ISA IO space support
is additional unnecessary work for them.

Plus, if you _have_ IO space support, you must have some MMIO region for
them to target - doing what many platforms have done to date and targetted
ISA IO address 0 at virtual address 0 is just not on because as soon as
you build a device driver which probes ISA addresses into your kernel,
you will oops.  Moreover, userspace can open /dev/ioport and ask the
kernel to access IO port addresses that way too.  Having IO space support
enabled when the platform does not support it is a liability and an
additional unnecessary security problem.

Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Alan Cox]

> with it.  It's very simple.  The IO port space is for ISA/PCMCIA and
> PCI IO port regions.  It is nothing more than that.

And on a lot of devices the LPC bus.

> Plus, if you _have_ IO space support, you must have some MMIO region for
> them to target - doing what many platforms have done to date and targetted
> ISA IO address 0 at virtual address 0 is just not on because as soon as
> you build a device driver which probes ISA addresses into your kernel,
> you will oops.

There shouldn't really be anything poking around that is modern - this is
true of some PC stuff too.

In general however if its because you have a window partly mapped you
could just catch the exception and load 0xFF for reads (and probably
whine with a backtrace so you know who to moan at).

Alan

Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Arnd Bergmann]

On Tuesday 11 December 2012, Alan Cox wrote:
> > Plus, if you have IO space support, you must have some MMIO region for
> > them to target - doing what many platforms have done to date and targetted
> > ISA IO address 0 at virtual address 0 is just not on because as soon as
> > you build a device driver which probes ISA addresses into your kernel,
> > you will oops.
> 
> There shouldn't really be anything poking around that is modern - this is
> true of some PC stuff too.
> 
> In general however if its because you have a window partly mapped you
> could just catch the exception and load 0xFF for reads (and probably
> whine with a backtrace so you know who to moan at).

The problem that Russell refers to is that some platforms define a window
that is 4GB large and starts at NULL. They then ioremap their PCI
or PCMCIA I/O space window and use the virtual __iomem address as the
offset into the 4GB I/O space window. Any driver (e.g. vgacon, /dev/port
or some legacy alsa sound driver) that tries to access a low port number
then ends up in user space memory, typically in an unmapped area there.

We definitely don't want to catch and fixup those exceptions. The solution
is to change the platforms so they actually use the proper I/O window
at a virtual address that is not used for anything else.

	Arnd

Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Russell King - ARM Linux]

On Tue, Dec 11, 2012 at 05:30:13PM +0100, Thomas Petazzoni wrote:
> arch/arm/mm/iomap.c is unconditionally compiled in all ARM kernels. And
> in this file, ioport_map() and ioport_unmap() are implement as soon as
> __io is defined. And basically, in arch/arm/include/asm/io.h, __io is
> defined for all platforms

Actually, I think that's a major bug in the single-zImage conversion:

#ifdef CONFIG_NEED_MACH_IO_H
#include <mach/io.h>
#elif defined(CONFIG_PCI)
#define IO_SPACE_LIMIT  ((resource_size_t)0xfffff)
#define __io(a)         __typesafe_io(PCI_IO_VIRT_BASE + ((a) & IO_SPACE_LIMIT))
#else
#define __io(a)         __typesafe_io((a) & IO_SPACE_LIMIT)
#endif

#ifndef IO_SPACE_LIMIT
#if defined(CONFIG_PCMCIA_SOC_COMMON) || defined(CONFIG_PCMCIA_SOC_COMMON_MODULE)
#define IO_SPACE_LIMIT ((resource_size_t)0xffffffff)
#elif defined(CONFIG_PCI) || defined(CONFIG_ISA) || defined(CONFIG_PCCARD)
#define IO_SPACE_LIMIT ((resource_size_t)0xffff)
#else
#define IO_SPACE_LIMIT ((resource_size_t)0)
#endif
#endif

So, in the !PCI !PCMCIA !ISA !PCCARD case, IO_SPACE_LIMIT will be zero,
which means the above common __io() functions will end up pointing the
access at the NULL pointer - whereas we shouldn't be providing IO space
support at all.  (Okay, there's a couple of corner cases we've known
about for a few _years_ but it's about time that stuff got fixed before
we try and put yet more band-aids over this.  Fix the root problems
guys, don't create more problems.)

Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Russell King - ARM Linux]

On Tue, Dec 11, 2012 at 10:43:49AM +0000, Arnd Bergmann wrote:
> On Friday 07 December 2012, Thomas Petazzoni wrote:
> > The pcim_*() functions are used by the libata-sff subsystem, and this
> > subsystem is used for many SATA drivers on ARM platforms that do not
> > necessarily have I/O ports.
> 
> I think this one is wrong as the CONFIG_HAS_IOPORT does not refer to the
> presence of PIO ports but to whether or not they provide an ioport_map
> function. If there is no ioport_map(), devm_pci_iomap will fail to link
> as far as I can tell.

Correct.  If HAS_IOPORT is not selected then we are potentially missing
the dependent functions (because the platform has no IOPORT support) _or_
it does have ISA/PCI IO spaces _but_ they're not mappable via the
ioport_map() mechanism due to some non-linearity involved in the
translation.

To make that second point clear, that's platforms where:

	ioport_map(addr + 4) != ioport_map(addr) + 4.

Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Alan Cox]

> Correct.  If HAS_IOPORT is not selected then we are potentially missing
> the dependent functions (because the platform has no IOPORT support) _or_
> it does have ISA/PCI IO spaces _but_ they're not mappable via the
> ioport_map() mechanism due to some non-linearity involved in the
> translation.
> 
> To make that second point clear, that's platforms where:
> 
> 	ioport_map(addr + 4) != ioport_map(addr) + 4.

For inb/inw and friends this shouldn't matter.

You can implement inl for example 

as

	if (unlikely(addr & 3))
		// or in fact BUG on this for almost all h/w
		uninlined_hard_slow_inl(addr);
	else
		return *(u32 *)iospace_map[addr];

Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Russell King - ARM Linux]

On Tue, Dec 11, 2012 at 05:16:10PM +0000, Alan Cox wrote:
> > Correct.  If HAS_IOPORT is not selected then we are potentially missing
> > the dependent functions (because the platform has no IOPORT support) _or_
> > it does have ISA/PCI IO spaces _but_ they're not mappable via the
> > ioport_map() mechanism due to some non-linearity involved in the
> > translation.
> > 
> > To make that second point clear, that's platforms where:
> > 
> > 	ioport_map(addr + 4) != ioport_map(addr) + 4.
> 
> For inb/inw and friends this shouldn't matter.

Exactly, and you wouldn't be using inb/inw with ioport_map().

The problem comes when you end up trying to deal with stuff which
uses ioread{8,16} on ioport_map() cookies where it's assumed that
adding N to the cookie is the same as adding N to the port address.

Thankfully those platforms aren't going to be a part of the single
zImage project...

Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Alan Cox]

> The problem comes when you end up trying to deal with stuff which
> uses ioread{8,16} on ioport_map() cookies where it's assumed that
> adding N to the cookie is the same as adding N to the port address.

It's a cookie - this isn't a problem, you can support it via the mapping
approah. Whether you want to is another question.

> Thankfully those platforms aren't going to be a part of the single
> zImage project...

Don't blame you 8)

Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Russell King - ARM Linux]

On Tue, Dec 11, 2012 at 05:45:09PM +0000, Alan Cox wrote:
> > The problem comes when you end up trying to deal with stuff which
> > uses ioread{8,16} on ioport_map() cookies where it's assumed that
> > adding N to the cookie is the same as adding N to the port address.
> 
> It's a cookie - this isn't a problem, you can support it via the mapping
> approah. Whether you want to is another question.

We're drifting off the topic of whether these things should be provided
and whether the config symbols should be selected...

For a kernel configuration which includes platforms where there is are no
ISA peripherals, no PCI bridge present, and no PCMCIA support, there's
little point to having IO space support present.

However, the platform should still work correctly if IO space support is
configured into the kernel (in other words, it shouldn't cause a failure)
so that such a platform can co-operate with those platforms which do
require IO space support.

Now, there was an idea a while back about having a common virtual address
space to map PCI/ISA IO space to - which we have in the kernel - 1MB at
0xfee00000.

If PCI is disabled, the ends up being a 1:1 conversion between IO offset
and CPU virtual address - that's partly there because no one's fixed up
the soc-common PCMCIA stuff to dea with the PCMCIA socket IO spaces
there.  We really need to be fixing some of this stuff...

(Unfortunately, my SA11x0 platform with the dual PCMCIA/CF sockets is now
my firewall so its out of the question to mess around with it on an ad-hoc
basis.)