On Monday 10 March 2014 16:33:44 Liviu Dudau wrote: > On Mon, Mar 10, 2014 at 03:21:01PM +0000, Arnd Bergmann wrote: > > On Monday 10 March 2014 14:44:14 Liviu Dudau wrote: > > > I will try to improve the error handling in the next patchset version. > > > However I am still confused about the earlier discussion on > > > pci_register_io_range(). Your suggestion initially was to return an > > > error in the default weak implementation, but in your last email you > > > are talking about returning 'port'. > > > > You can do either one: 'port' should be positive or zero, while the > > error would always be negative. We do the same thing in many interfaces > > in the kernel. > > > > > My idea when I've introduced the > > > helper function was that it would return an error if it fails to > > > register the IO range and zero otherwise. I agree that we can treat > > > the default 'do nothing with the IO range' case as an error, with > > > the caveat that will force architectures that use this code to > > > provide their own implementation of pci_register_io_range() in order > > > to avoid failure, even for the cases where the architecture has a 1:1 > > > mapping between IO and CPU addresses. > > > > Which architectures are you thinking of? The only one I know that > > does this is ia64, and we won't ever have to support this helper > > on that architecture. > > I was thinking about architectures that have IO_SPACE_LIMIT >= 0xffffffff. > While not an absolute indicator, with the default pci_address_to_pio() > that means that they can use the CPU MMIO address as IO address directly. Not really, that would only work if they also have instructions to do raw accesses to physical memory addresses rather than virtual memory pointers that most architectures do. > $ git grep IO_SPACE_LIMIT | grep -i ffffffff > arch/arm/include/asm/io.h:#define IO_SPACE_LIMIT ((resource_size_t)0xffffffff) > arch/arm/mach-at91/include/mach/io.h:#define IO_SPACE_LIMIT 0xFFFFFFFF > arch/arm/mach-omap1/include/mach/io.h:#define IO_SPACE_LIMIT 0xffffffff > arch/arm/mach-pxa/include/mach/io.h:#define IO_SPACE_LIMIT 0xffffffff > arch/arm/mach-s3c24xx/include/mach/io.h:#define IO_SPACE_LIMIT 0xffffffff These use a special trick where an __iomem pointer is the same as the port number. This works most of the time, but breaks anything that assumes that port numbers are low, such as /dev/port or broken devices. Moreover, it means your code won't work because it depends on passing the virtual start address of the PIO mapping window as io_offset. > arch/avr32/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff > arch/frv/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff They have no MMU, and the code relies on the port number to match both the virtual and the physical address. You could be right about these, but I would guess that the code also needs some other changes if we want to make it work on nommu kernels. It also depends on whether the I/O bus address is the same as the CPU address, or if it starts at bus address 0. > arch/ia64/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffffffffffffUL Here, the definition is special, the token is just used to encode a space number and an offset within the I/O space. > arch/m32r/include/asm/io.h:#define IO_SPACE_LIMIT 0xFFFFFFFF no PCI here. > arch/m68k/include/asm/io_no.h:#define IO_SPACE_LIMIT 0xffffffff This looks like a mistake, it should be smaller > arch/microblaze/include/asm/io.h:#define IO_SPACE_LIMIT (0xFFFFFFFF) I suspect it doesn't actually work. microblaze copied large parts of this from PowerPC, but the parts that differ apparently get it wrong for the I/O space. > arch/mn10300/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff Same category as frv. We should ask David Howells whether he thinks I/O space actually works on these. > arch/sh/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff I think this should just be 0xffff. > arch/sparc/include/asm/io_32.h:#define IO_SPACE_LIMIT 0xffffffff > arch/sparc/include/asm/io_64.h:#define IO_SPACE_LIMIT 0xffffffffffffffffUL Sparc actually accesses the physical addresses, so in theory it could always work. In the 64-bit case it would however have to check that the port number is smaller than 0xffffffff, otherwise you couldn't set the BAR. This means you still need a custom function. > arch/tile/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff tile seems to support only ioport_map() but not inb/outb, if I'm reading this right. > > I did not ask to treat 'do nothing with the IO range' as an error, > > what I meant is that we should treat 'architecture cannot translate > > from I/O space to memory space but DT lists a translation anyway' > > as an error. On x86, you should never see an entry for the I/O space > > in "ranges", so we will not call this function unless there is a > > bug in DT. > > Ok, it's just that there is no "architecture cannot translate from I/O > space to memory space" indicator anywhere and I don't want to make x86 > a special case. Right. > So, my proposal is this: default weak implementation of pci_register_io_range() > returns an error (meaning I have no idea how to translate IO addresses > to memory space) and anyone that wants this function to return success will > have to provide their implementation. Another idea: make this conditional on the definition of PCI_IOBASE: If this is defined, we can use the arm64 version that uses this number. Otherwise we fall back to returning an error, which means that either on the architecture we shouldn't be calling that function, or we need a custom implementation. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
