Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
On Tuesday 11 December 2012, Russell King - ARM Linux wrote: > > On Tue, Dec 11, 2012 at 04:15:02PM +, Arnd Bergmann wrote: > > On Tuesday 11 December 2012, Thomas Petazzoni wrote: > > > On Tue, 11 Dec 2012 10:43:49 +, 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
On Tue, Dec 11, 2012 at 05:45:09PM +, 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
0xfee0.
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.)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
> 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)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
On Tue, Dec 11, 2012 at 05:16:10PM +, 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...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
> 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]; -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
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 #elif defined(CONFIG_PCI) #define IO_SPACE_LIMIT ((resource_size_t)0xf) #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)0x) #elif defined(CONFIG_PCI) || defined(CONFIG_ISA) || defined(CONFIG_PCCARD) #define IO_SPACE_LIMIT ((resource_size_t)0x) #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.) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
On Tue, Dec 11, 2012 at 05:38:19PM +0100, Thomas Petazzoni wrote: > Russell, > > On Tue, 11 Dec 2012 16:23:25 +, 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
Russell, On Tue, 11 Dec 2012 16:23:25 +, 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
Dear Arnd Bergmann, On Tue, 11 Dec 2012 16:15:02 +, 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
On Tue, Dec 11, 2012 at 10:43:49AM +, 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
On Tue, Dec 11, 2012 at 04:15:02PM +, Arnd Bergmann wrote: > On Tuesday 11 December 2012, Thomas Petazzoni wrote: > > On Tue, 11 Dec 2012 10:43:49 +, 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
On Tuesday 11 December 2012, Thomas Petazzoni wrote: > On Tue, 11 Dec 2012 10:43:49 +, 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
Dear Arnd Bergmann, On Tue, 11 Dec 2012 10:43:49 +, 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
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 > Cc: Paul Gortmaker > Cc: Jesse Barnes > Cc: Yinghai Lu > 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 */ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
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.petazz...@free-electrons.com Cc: Paul Gortmaker paul.gortma...@windriver.com Cc: Jesse Barnes jbar...@virtuousgeek.org Cc: Yinghai Lu ying...@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 */ -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
Dear Arnd Bergmann, On Tue, 11 Dec 2012 10:43:49 +, 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
On Tuesday 11 December 2012, Thomas Petazzoni wrote: On Tue, 11 Dec 2012 10:43:49 +, 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
On Tue, Dec 11, 2012 at 04:15:02PM +, Arnd Bergmann wrote: On Tuesday 11 December 2012, Thomas Petazzoni wrote: On Tue, 11 Dec 2012 10:43:49 +, 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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
On Tue, Dec 11, 2012 at 10:43:49AM +, 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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
Dear Arnd Bergmann, On Tue, 11 Dec 2012 16:15:02 +, 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
Russell, On Tue, 11 Dec 2012 16:23:25 +, 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
On Tue, Dec 11, 2012 at 05:38:19PM +0100, Thomas Petazzoni wrote: Russell, On Tue, 11 Dec 2012 16:23:25 +, 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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
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)0xf) #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)0x) #elif defined(CONFIG_PCI) || defined(CONFIG_ISA) || defined(CONFIG_PCCARD) #define IO_SPACE_LIMIT ((resource_size_t)0x) #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.) -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
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]; -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
On Tue, Dec 11, 2012 at 05:16:10PM +, 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...
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
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)
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
On Tue, Dec 11, 2012 at 05:45:09PM +, 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
0xfee0.
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.)
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1 01/16] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT
On Tuesday 11 December 2012, Russell King - ARM Linux wrote: On Tue, Dec 11, 2012 at 04:15:02PM +, Arnd Bergmann wrote: On Tuesday 11 December 2012, Thomas Petazzoni wrote: On Tue, 11 Dec 2012 10:43:49 +, 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
