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.
$ 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 arch/avr32/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff arch/frv/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff arch/ia64/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffffffffffffUL arch/m32r/include/asm/io.h:#define IO_SPACE_LIMIT 0xFFFFFFFF arch/m68k/include/asm/io_no.h:#define IO_SPACE_LIMIT 0xffffffff arch/microblaze/include/asm/io.h:#define IO_SPACE_LIMIT (0xFFFFFFFF) arch/mn10300/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff arch/sh/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff arch/sparc/include/asm/io_32.h:#define IO_SPACE_LIMIT 0xffffffff arch/sparc/include/asm/io_64.h:#define IO_SPACE_LIMIT 0xffffffffffffffffUL arch/tile/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff
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.
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.
I will send an updated series.
Best regards, Liviu
Arnd