Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
On Thu, Apr 06, 2017 at 11:47:37AM +0100, Russell King - ARM Linux wrote: > On Thu, Apr 06, 2017 at 11:26:36AM +0100, Lorenzo Pieralisi wrote: > > On Wed, Apr 05, 2017 at 01:38:12PM +0100, Lorenzo Pieralisi wrote: > > > eg: openrisc > > > > > > >> include/asm-generic/io.h:922:9: error: implicit declaration of > > > >> function 'ioremap_nocache' [-Werror=implicit-function-declaration] > > > return ioremap_nocache(offset, size); > > > > Indeed, the static inline ioremap_nocache() fallback does not work > > on all arches (whether I add the fallback in linux/io.h or > > asm-generic/io.h is irrelevant), I bump into issues such as the one > > reported above. > > From what I can see: > > (a) openrisc does define ioremap_nocache() in its asm/io.h > (b) these do not: > > $ grep -L 'ioremap_nocache' arch/*/include/asm/io.h > arch/blackfin/include/asm/io.h > arch/h8300/include/asm/io.h > arch/m68k/include/asm/io.h > arch/score/include/asm/io.h > arch/sparc/include/asm/io.h > > Out of those, blackfin, h8300 and score do not define it, whereas m68k > and sparc do in other headers included by asm/io.h. So it looks like > we have three problem architectures that don't define an ioremap_nocache(). > > PCI on blackfin depends on BROKEN, so it's not selectable. From what I > can tell, h8300 and score do not allow PCI to be enabled (but maybe its > buried elsewhere in their Kconfig files, I didn't check.) > > So, I think a way around this is to make ioremap_nopost() conditional > on PCI in linux/io.h for the time being - if its scope wants to be > enlarged, then these three architectures would need to have either an > ioremap_nopost() implementation added, or an ioremap_nocache() > implementation. Ok, I implemented asm-generic/ioremap-nopost.h, I do not think we need a CONFIG_PCI guard (kbuild has not barfed at it, yet), given that blackfin and h8300 are !MMU and score selects NO_IOMEM. Patch below is all arches squashed into one commit, I probably have to split it one per arch which will make this a 50-patch series in total: https://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/linux.git/commit/?h=pci/config-io-mappings-fix-v3=acc0be820c809101e02ab7cb170f802db53af934 If I hear no complaints I will split patch above one per arch and repost the series shortly (even though I think I should make two series out of it, one asm-generic/ioremap-nopost.h boilerplate and second ARM/ARM64 implementation + PCI drivers). Lorenzo
Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
On Thu, Apr 06, 2017 at 11:47:37AM +0100, Russell King - ARM Linux wrote: > On Thu, Apr 06, 2017 at 11:26:36AM +0100, Lorenzo Pieralisi wrote: > > On Wed, Apr 05, 2017 at 01:38:12PM +0100, Lorenzo Pieralisi wrote: > > > eg: openrisc > > > > > > >> include/asm-generic/io.h:922:9: error: implicit declaration of > > > >> function 'ioremap_nocache' [-Werror=implicit-function-declaration] > > > return ioremap_nocache(offset, size); > > > > Indeed, the static inline ioremap_nocache() fallback does not work > > on all arches (whether I add the fallback in linux/io.h or > > asm-generic/io.h is irrelevant), I bump into issues such as the one > > reported above. > > From what I can see: > > (a) openrisc does define ioremap_nocache() in its asm/io.h > (b) these do not: > > $ grep -L 'ioremap_nocache' arch/*/include/asm/io.h > arch/blackfin/include/asm/io.h > arch/h8300/include/asm/io.h > arch/m68k/include/asm/io.h > arch/score/include/asm/io.h > arch/sparc/include/asm/io.h > > Out of those, blackfin, h8300 and score do not define it, whereas m68k > and sparc do in other headers included by asm/io.h. So it looks like > we have three problem architectures that don't define an ioremap_nocache(). > > PCI on blackfin depends on BROKEN, so it's not selectable. From what I > can tell, h8300 and score do not allow PCI to be enabled (but maybe its > buried elsewhere in their Kconfig files, I didn't check.) > > So, I think a way around this is to make ioremap_nopost() conditional > on PCI in linux/io.h for the time being - if its scope wants to be > enlarged, then these three architectures would need to have either an > ioremap_nopost() implementation added, or an ioremap_nocache() > implementation. Ok, I implemented asm-generic/ioremap-nopost.h, I do not think we need a CONFIG_PCI guard (kbuild has not barfed at it, yet), given that blackfin and h8300 are !MMU and score selects NO_IOMEM. Patch below is all arches squashed into one commit, I probably have to split it one per arch which will make this a 50-patch series in total: https://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/linux.git/commit/?h=pci/config-io-mappings-fix-v3=acc0be820c809101e02ab7cb170f802db53af934 If I hear no complaints I will split patch above one per arch and repost the series shortly (even though I think I should make two series out of it, one asm-generic/ioremap-nopost.h boilerplate and second ARM/ARM64 implementation + PCI drivers). Lorenzo
Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
On Thu, Apr 06, 2017 at 06:09:51PM +0100, Lorenzo Pieralisi wrote: > On Thu, Apr 06, 2017 at 05:40:16PM +0100, Russell King - ARM Linux wrote: > > On Thu, Apr 06, 2017 at 05:21:56PM +0100, Lorenzo Pieralisi wrote: > > > Ok, so: > > > > > > (1) I can do asm-generic/ioremap-nopost.h, which I assume you want to > > > contain something like > > > > > > static inline void __iomem *ioremap_nopost(phys_addr_t offset, size_t > > > size) > > > { > > > return ioremap_nocache(offset, size); > > > } > > > > > > Funny bit is that it has to be included by asm*/io.h files _after_ > > > ioremap_nocache has been #defined (that's the reason my approach was > > > failing miserably even on arches like eg powerpc (see [1] below) that > > > does have ioremap_nocache), > > > > PowerPC does have ioremap_nocache() though: > > > > /** > > * ioremap - map bus memory into CPU space > > ... > > * * ioremap_nocache is identical to ioremap > > extern void __iomem *ioremap(phys_addr_t address, unsigned long size); > > #define ioremap_nocache(addr, size) ioremap((addr), (size)) > > > > and this include file is included very early on in linux/io.h. I don't > > see anything that conditionalises it on anything except __KERNEL__. So, > > the report from 0-day really doesn't make any sense to me. > > > > Do we know how we're ending up in linux/io.h line 169 without having > > picked up the ioremap_nocache() definition provided by PowerPC's > > asm/io.h ? > > I will debug it further but I *think* it is because: > > eg arch/powerpc/oprofile/op_model_cell.c includes > > and includes before ioremap_nocache is defined Oh, that's just very wrong. asm/foo.h should never include linux/foo.h especially when linux/foo.h already includes asm/foo.h. I think we need PowerPC folk to fix this. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
On Thu, Apr 06, 2017 at 06:09:51PM +0100, Lorenzo Pieralisi wrote: > On Thu, Apr 06, 2017 at 05:40:16PM +0100, Russell King - ARM Linux wrote: > > On Thu, Apr 06, 2017 at 05:21:56PM +0100, Lorenzo Pieralisi wrote: > > > Ok, so: > > > > > > (1) I can do asm-generic/ioremap-nopost.h, which I assume you want to > > > contain something like > > > > > > static inline void __iomem *ioremap_nopost(phys_addr_t offset, size_t > > > size) > > > { > > > return ioremap_nocache(offset, size); > > > } > > > > > > Funny bit is that it has to be included by asm*/io.h files _after_ > > > ioremap_nocache has been #defined (that's the reason my approach was > > > failing miserably even on arches like eg powerpc (see [1] below) that > > > does have ioremap_nocache), > > > > PowerPC does have ioremap_nocache() though: > > > > /** > > * ioremap - map bus memory into CPU space > > ... > > * * ioremap_nocache is identical to ioremap > > extern void __iomem *ioremap(phys_addr_t address, unsigned long size); > > #define ioremap_nocache(addr, size) ioremap((addr), (size)) > > > > and this include file is included very early on in linux/io.h. I don't > > see anything that conditionalises it on anything except __KERNEL__. So, > > the report from 0-day really doesn't make any sense to me. > > > > Do we know how we're ending up in linux/io.h line 169 without having > > picked up the ioremap_nocache() definition provided by PowerPC's > > asm/io.h ? > > I will debug it further but I *think* it is because: > > eg arch/powerpc/oprofile/op_model_cell.c includes > > and includes before ioremap_nocache is defined Oh, that's just very wrong. asm/foo.h should never include linux/foo.h especially when linux/foo.h already includes asm/foo.h. I think we need PowerPC folk to fix this. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
On Thu, Apr 06, 2017 at 05:40:16PM +0100, Russell King - ARM Linux wrote: > On Thu, Apr 06, 2017 at 05:21:56PM +0100, Lorenzo Pieralisi wrote: > > Ok, so: > > > > (1) I can do asm-generic/ioremap-nopost.h, which I assume you want to > > contain something like > > > > static inline void __iomem *ioremap_nopost(phys_addr_t offset, size_t size) > > { > > return ioremap_nocache(offset, size); > > } > > > > Funny bit is that it has to be included by asm*/io.h files _after_ > > ioremap_nocache has been #defined (that's the reason my approach was > > failing miserably even on arches like eg powerpc (see [1] below) that > > does have ioremap_nocache), > > PowerPC does have ioremap_nocache() though: > > /** > * ioremap - map bus memory into CPU space > ... > * * ioremap_nocache is identical to ioremap > extern void __iomem *ioremap(phys_addr_t address, unsigned long size); > #define ioremap_nocache(addr, size) ioremap((addr), (size)) > > and this include file is included very early on in linux/io.h. I don't > see anything that conditionalises it on anything except __KERNEL__. So, > the report from 0-day really doesn't make any sense to me. > > Do we know how we're ending up in linux/io.h line 169 without having > picked up the ioremap_nocache() definition provided by PowerPC's > asm/io.h ? I will debug it further but I *think* it is because: eg arch/powerpc/oprofile/op_model_cell.c includes and includes before ioremap_nocache is defined > > not sure it is going to be very nice to have > > an include in the middle of asm*/io.h include files (and I have to patch > > all arches which I can do). > > You mean like we already have to do with this asm-generic/io.h thing in > the ARM io.h header file, because we need to define all the accessors > first, to prevent the asm-generic/io.h thing defining them for us? > Given how asm-generic has headed in this regard, having include files > at all sorts of strange locations within the architecture asm/*.h > header files has become quite normal, unfortunately. Yes we won't make it any nicer that's for certain, my worry is that it would end up being even harder to read. > > (2) I add: > > > > #ifndef ioremap_nopost > > static inline void __iomem *ioremap_nopost(phys_addr_t offset, size_t size) > > { > > return NULL; <-(making it return ioremap_nocache() does not > > work, see [1] for the reason) > > } > > #endif > > > > in linux/io.h > > ... which breaks the kernel if ioremap_nopost is missing from the arch > header, and one of the drivers that you're modifying to use this new > ioremap variant happens to be built and used on such an architecture. Yes agree. > > (3) ioremap_nopost follows Luis' ioremap_uc approach > > Same problem as (2), as I understand correctly. Agreed. We have to find the lesser evil, that's it. Thanks ! Lorenzo
Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
On Thu, Apr 06, 2017 at 05:40:16PM +0100, Russell King - ARM Linux wrote: > On Thu, Apr 06, 2017 at 05:21:56PM +0100, Lorenzo Pieralisi wrote: > > Ok, so: > > > > (1) I can do asm-generic/ioremap-nopost.h, which I assume you want to > > contain something like > > > > static inline void __iomem *ioremap_nopost(phys_addr_t offset, size_t size) > > { > > return ioremap_nocache(offset, size); > > } > > > > Funny bit is that it has to be included by asm*/io.h files _after_ > > ioremap_nocache has been #defined (that's the reason my approach was > > failing miserably even on arches like eg powerpc (see [1] below) that > > does have ioremap_nocache), > > PowerPC does have ioremap_nocache() though: > > /** > * ioremap - map bus memory into CPU space > ... > * * ioremap_nocache is identical to ioremap > extern void __iomem *ioremap(phys_addr_t address, unsigned long size); > #define ioremap_nocache(addr, size) ioremap((addr), (size)) > > and this include file is included very early on in linux/io.h. I don't > see anything that conditionalises it on anything except __KERNEL__. So, > the report from 0-day really doesn't make any sense to me. > > Do we know how we're ending up in linux/io.h line 169 without having > picked up the ioremap_nocache() definition provided by PowerPC's > asm/io.h ? I will debug it further but I *think* it is because: eg arch/powerpc/oprofile/op_model_cell.c includes and includes before ioremap_nocache is defined > > not sure it is going to be very nice to have > > an include in the middle of asm*/io.h include files (and I have to patch > > all arches which I can do). > > You mean like we already have to do with this asm-generic/io.h thing in > the ARM io.h header file, because we need to define all the accessors > first, to prevent the asm-generic/io.h thing defining them for us? > Given how asm-generic has headed in this regard, having include files > at all sorts of strange locations within the architecture asm/*.h > header files has become quite normal, unfortunately. Yes we won't make it any nicer that's for certain, my worry is that it would end up being even harder to read. > > (2) I add: > > > > #ifndef ioremap_nopost > > static inline void __iomem *ioremap_nopost(phys_addr_t offset, size_t size) > > { > > return NULL; <-(making it return ioremap_nocache() does not > > work, see [1] for the reason) > > } > > #endif > > > > in linux/io.h > > ... which breaks the kernel if ioremap_nopost is missing from the arch > header, and one of the drivers that you're modifying to use this new > ioremap variant happens to be built and used on such an architecture. Yes agree. > > (3) ioremap_nopost follows Luis' ioremap_uc approach > > Same problem as (2), as I understand correctly. Agreed. We have to find the lesser evil, that's it. Thanks ! Lorenzo
Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
On Thu, Apr 06, 2017 at 05:21:56PM +0100, Lorenzo Pieralisi wrote: > Ok, so: > > (1) I can do asm-generic/ioremap-nopost.h, which I assume you want to > contain something like > > static inline void __iomem *ioremap_nopost(phys_addr_t offset, size_t size) > { > return ioremap_nocache(offset, size); > } > > Funny bit is that it has to be included by asm*/io.h files _after_ > ioremap_nocache has been #defined (that's the reason my approach was > failing miserably even on arches like eg powerpc (see [1] below) that > does have ioremap_nocache), PowerPC does have ioremap_nocache() though: /** * ioremap - map bus memory into CPU space ... * * ioremap_nocache is identical to ioremap extern void __iomem *ioremap(phys_addr_t address, unsigned long size); #define ioremap_nocache(addr, size) ioremap((addr), (size)) and this include file is included very early on in linux/io.h. I don't see anything that conditionalises it on anything except __KERNEL__. So, the report from 0-day really doesn't make any sense to me. Do we know how we're ending up in linux/io.h line 169 without having picked up the ioremap_nocache() definition provided by PowerPC's asm/io.h ? > not sure it is going to be very nice to have > an include in the middle of asm*/io.h include files (and I have to patch > all arches which I can do). You mean like we already have to do with this asm-generic/io.h thing in the ARM io.h header file, because we need to define all the accessors first, to prevent the asm-generic/io.h thing defining them for us? Given how asm-generic has headed in this regard, having include files at all sorts of strange locations within the architecture asm/*.h header files has become quite normal, unfortunately. > (2) I add: > > #ifndef ioremap_nopost > static inline void __iomem *ioremap_nopost(phys_addr_t offset, size_t size) > { > return NULL; <-(making it return ioremap_nocache() does not > work, see [1] for the reason) > } > #endif > > in linux/io.h ... which breaks the kernel if ioremap_nopost is missing from the arch header, and one of the drivers that you're modifying to use this new ioremap variant happens to be built and used on such an architecture. > (3) ioremap_nopost follows Luis' ioremap_uc approach Same problem as (2), as I understand correctly. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
On Thu, Apr 06, 2017 at 05:21:56PM +0100, Lorenzo Pieralisi wrote: > Ok, so: > > (1) I can do asm-generic/ioremap-nopost.h, which I assume you want to > contain something like > > static inline void __iomem *ioremap_nopost(phys_addr_t offset, size_t size) > { > return ioremap_nocache(offset, size); > } > > Funny bit is that it has to be included by asm*/io.h files _after_ > ioremap_nocache has been #defined (that's the reason my approach was > failing miserably even on arches like eg powerpc (see [1] below) that > does have ioremap_nocache), PowerPC does have ioremap_nocache() though: /** * ioremap - map bus memory into CPU space ... * * ioremap_nocache is identical to ioremap extern void __iomem *ioremap(phys_addr_t address, unsigned long size); #define ioremap_nocache(addr, size) ioremap((addr), (size)) and this include file is included very early on in linux/io.h. I don't see anything that conditionalises it on anything except __KERNEL__. So, the report from 0-day really doesn't make any sense to me. Do we know how we're ending up in linux/io.h line 169 without having picked up the ioremap_nocache() definition provided by PowerPC's asm/io.h ? > not sure it is going to be very nice to have > an include in the middle of asm*/io.h include files (and I have to patch > all arches which I can do). You mean like we already have to do with this asm-generic/io.h thing in the ARM io.h header file, because we need to define all the accessors first, to prevent the asm-generic/io.h thing defining them for us? Given how asm-generic has headed in this regard, having include files at all sorts of strange locations within the architecture asm/*.h header files has become quite normal, unfortunately. > (2) I add: > > #ifndef ioremap_nopost > static inline void __iomem *ioremap_nopost(phys_addr_t offset, size_t size) > { > return NULL; <-(making it return ioremap_nocache() does not > work, see [1] for the reason) > } > #endif > > in linux/io.h ... which breaks the kernel if ioremap_nopost is missing from the arch header, and one of the drivers that you're modifying to use this new ioremap variant happens to be built and used on such an architecture. > (3) ioremap_nopost follows Luis' ioremap_uc approach Same problem as (2), as I understand correctly. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
On Thu, Apr 06, 2017 at 02:07:27PM +0100, Russell King - ARM Linux wrote: > On Thu, Apr 06, 2017 at 01:59:07PM +0200, Luis R. Rodriguez wrote: > > On Thu, Apr 06, 2017 at 12:38:45PM +0100, Lorenzo Pieralisi wrote: > > > Ok but where ? linux/io.h or asm-generic/io.h ? As I replied to Russell, > > > having it in linux/io.h is a bit odd given that it would be the only > > > ioremap_* implementation declared there, I think we need more > > > consistency here instead of deviating again from what you did. > > > > asm-generic/io.h is the right place for generics which let you override. > > I disagree for two reasons, and I will refer you to Linus' comments back > in 2005 at http://yarchive.net/comp/linux/generic.html > > 1) asm-generic/io.h has become an ifdef mess, every single definition in >there is conditional. The reason for this has happened is that >architectures can't pick and choose what they want from a single file >unless something like that is done. It would be _far_ better to >split this file up by functional group - eg, ISA IO accessors, >io(read|write)*(), read|write*(), and so forth. > > 2) We're at the point where we have various .c files around the kernel >_directly_ including asm-generic header files, which means the >use of asm-generic headers is no longer a choice of the architecture. > > 3) asm-generic/ started out life as the place where example >implementations of asm/*.h header files were found, and but has >grown into a place where we dump default implementations. We had >a place for default implementations for years, which were the >linux/*.h headers. We have now ended up with a mixture of both >techniques, even for something like io.h, we have the messy >asm-generic/io.h, the architecture's asm/io.h, and then linux/io.h. >We have ended up with both linux/io.h and asm-generic/io.h containing >default definitions. > > We've had the rule that where both linux/foo.h and asm/foo.h exist, the > linux/ counterpart is the preferred include file. That didn't really > happen with asm/io.h due to the number of users that there were, but > when new stuff was added to asm/io.h which we wanted to be generic, > linux/io.h was created to contain that. This actually pre-dates the > "let's clutter up asm-generic with shared arch stuff" push. > > Now, what you say _may_ make sense if we have an > asm-generic/ioremap-nopost.h header which just defines a default > ioremap_nopost.h implementation, and architectures would then be able to > choose whether to include that or not depending on whether they provide > their own implementation. No ugly ifdefs are necessary with that > approach. > > Out of the three choices, I would much rather see the > asm-generic/ioremap-nopost.h approach. However, the down-side to that > is it means all architectures asm/io.h would need to be touched to > explicitly include that. > > What I'm absolutely certain of, though, is that making asm-generic/io.h > even worse by adding yet more conditional stuff to it is not a sane way > forward. Ok, so: (1) I can do asm-generic/ioremap-nopost.h, which I assume you want to contain something like static inline void __iomem *ioremap_nopost(phys_addr_t offset, size_t size) { return ioremap_nocache(offset, size); } Funny bit is that it has to be included by asm*/io.h files _after_ ioremap_nocache has been #defined (that's the reason my approach was failing miserably even on arches like eg powerpc (see [1] below) that does have ioremap_nocache), not sure it is going to be very nice to have an include in the middle of asm*/io.h include files (and I have to patch all arches which I can do). Or (2) I add: #ifndef ioremap_nopost static inline void __iomem *ioremap_nopost(phys_addr_t offset, size_t size) { return NULL; <-(making it return ioremap_nocache() does not work, see [1] for the reason) } #endif in linux/io.h (3) ioremap_nopost follows Luis' ioremap_uc approach (1)-(2) bypass completely what Luis did for ioremap_uc(), which implies that we have yet another way of implementing ioremap_*. I would like to get this patchset done so if you have an opinion it is time to state it. Thanks, Lorenzo [1] powerpc build report: tree: https://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/linux.git pci/config-io-mappings-fix-v3 head: 283a324b549a662346c95c05b08983dd5b83354b commit: e66b493fe93226c02b1a33355f79db7ed6efe718 [2/23] linux/io.h: add ioremap_nopost remap interface config: powerpc-defconfig (attached as .config) compiler: powerpc64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705 reproduce: wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross git checkout e66b493fe93226c02b1a33355f79db7ed6efe718 # save the attached .config to linux build tree make.cross ARCH=powerpc All errors (new ones
Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
On Thu, Apr 06, 2017 at 02:07:27PM +0100, Russell King - ARM Linux wrote: > On Thu, Apr 06, 2017 at 01:59:07PM +0200, Luis R. Rodriguez wrote: > > On Thu, Apr 06, 2017 at 12:38:45PM +0100, Lorenzo Pieralisi wrote: > > > Ok but where ? linux/io.h or asm-generic/io.h ? As I replied to Russell, > > > having it in linux/io.h is a bit odd given that it would be the only > > > ioremap_* implementation declared there, I think we need more > > > consistency here instead of deviating again from what you did. > > > > asm-generic/io.h is the right place for generics which let you override. > > I disagree for two reasons, and I will refer you to Linus' comments back > in 2005 at http://yarchive.net/comp/linux/generic.html > > 1) asm-generic/io.h has become an ifdef mess, every single definition in >there is conditional. The reason for this has happened is that >architectures can't pick and choose what they want from a single file >unless something like that is done. It would be _far_ better to >split this file up by functional group - eg, ISA IO accessors, >io(read|write)*(), read|write*(), and so forth. > > 2) We're at the point where we have various .c files around the kernel >_directly_ including asm-generic header files, which means the >use of asm-generic headers is no longer a choice of the architecture. > > 3) asm-generic/ started out life as the place where example >implementations of asm/*.h header files were found, and but has >grown into a place where we dump default implementations. We had >a place for default implementations for years, which were the >linux/*.h headers. We have now ended up with a mixture of both >techniques, even for something like io.h, we have the messy >asm-generic/io.h, the architecture's asm/io.h, and then linux/io.h. >We have ended up with both linux/io.h and asm-generic/io.h containing >default definitions. > > We've had the rule that where both linux/foo.h and asm/foo.h exist, the > linux/ counterpart is the preferred include file. That didn't really > happen with asm/io.h due to the number of users that there were, but > when new stuff was added to asm/io.h which we wanted to be generic, > linux/io.h was created to contain that. This actually pre-dates the > "let's clutter up asm-generic with shared arch stuff" push. > > Now, what you say _may_ make sense if we have an > asm-generic/ioremap-nopost.h header which just defines a default > ioremap_nopost.h implementation, and architectures would then be able to > choose whether to include that or not depending on whether they provide > their own implementation. No ugly ifdefs are necessary with that > approach. > > Out of the three choices, I would much rather see the > asm-generic/ioremap-nopost.h approach. However, the down-side to that > is it means all architectures asm/io.h would need to be touched to > explicitly include that. > > What I'm absolutely certain of, though, is that making asm-generic/io.h > even worse by adding yet more conditional stuff to it is not a sane way > forward. Ok, so: (1) I can do asm-generic/ioremap-nopost.h, which I assume you want to contain something like static inline void __iomem *ioremap_nopost(phys_addr_t offset, size_t size) { return ioremap_nocache(offset, size); } Funny bit is that it has to be included by asm*/io.h files _after_ ioremap_nocache has been #defined (that's the reason my approach was failing miserably even on arches like eg powerpc (see [1] below) that does have ioremap_nocache), not sure it is going to be very nice to have an include in the middle of asm*/io.h include files (and I have to patch all arches which I can do). Or (2) I add: #ifndef ioremap_nopost static inline void __iomem *ioremap_nopost(phys_addr_t offset, size_t size) { return NULL; <-(making it return ioremap_nocache() does not work, see [1] for the reason) } #endif in linux/io.h (3) ioremap_nopost follows Luis' ioremap_uc approach (1)-(2) bypass completely what Luis did for ioremap_uc(), which implies that we have yet another way of implementing ioremap_*. I would like to get this patchset done so if you have an opinion it is time to state it. Thanks, Lorenzo [1] powerpc build report: tree: https://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/linux.git pci/config-io-mappings-fix-v3 head: 283a324b549a662346c95c05b08983dd5b83354b commit: e66b493fe93226c02b1a33355f79db7ed6efe718 [2/23] linux/io.h: add ioremap_nopost remap interface config: powerpc-defconfig (attached as .config) compiler: powerpc64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705 reproduce: wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross git checkout e66b493fe93226c02b1a33355f79db7ed6efe718 # save the attached .config to linux build tree make.cross ARCH=powerpc All errors (new ones
Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
On Thu, Apr 06, 2017 at 01:59:07PM +0200, Luis R. Rodriguez wrote: > On Thu, Apr 06, 2017 at 12:38:45PM +0100, Lorenzo Pieralisi wrote: > > Ok but where ? linux/io.h or asm-generic/io.h ? As I replied to Russell, > > having it in linux/io.h is a bit odd given that it would be the only > > ioremap_* implementation declared there, I think we need more > > consistency here instead of deviating again from what you did. > > asm-generic/io.h is the right place for generics which let you override. I disagree for two reasons, and I will refer you to Linus' comments back in 2005 at http://yarchive.net/comp/linux/generic.html 1) asm-generic/io.h has become an ifdef mess, every single definition in there is conditional. The reason for this has happened is that architectures can't pick and choose what they want from a single file unless something like that is done. It would be _far_ better to split this file up by functional group - eg, ISA IO accessors, io(read|write)*(), read|write*(), and so forth. 2) We're at the point where we have various .c files around the kernel _directly_ including asm-generic header files, which means the use of asm-generic headers is no longer a choice of the architecture. 3) asm-generic/ started out life as the place where example implementations of asm/*.h header files were found, and but has grown into a place where we dump default implementations. We had a place for default implementations for years, which were the linux/*.h headers. We have now ended up with a mixture of both techniques, even for something like io.h, we have the messy asm-generic/io.h, the architecture's asm/io.h, and then linux/io.h. We have ended up with both linux/io.h and asm-generic/io.h containing default definitions. We've had the rule that where both linux/foo.h and asm/foo.h exist, the linux/ counterpart is the preferred include file. That didn't really happen with asm/io.h due to the number of users that there were, but when new stuff was added to asm/io.h which we wanted to be generic, linux/io.h was created to contain that. This actually pre-dates the "let's clutter up asm-generic with shared arch stuff" push. Now, what you say _may_ make sense if we have an asm-generic/ioremap-nopost.h header which just defines a default ioremap_nopost.h implementation, and architectures would then be able to choose whether to include that or not depending on whether they provide their own implementation. No ugly ifdefs are necessary with that approach. Out of the three choices, I would much rather see the asm-generic/ioremap-nopost.h approach. However, the down-side to that is it means all architectures asm/io.h would need to be touched to explicitly include that. What I'm absolutely certain of, though, is that making asm-generic/io.h even worse by adding yet more conditional stuff to it is not a sane way forward. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
On Thu, Apr 06, 2017 at 01:59:07PM +0200, Luis R. Rodriguez wrote: > On Thu, Apr 06, 2017 at 12:38:45PM +0100, Lorenzo Pieralisi wrote: > > Ok but where ? linux/io.h or asm-generic/io.h ? As I replied to Russell, > > having it in linux/io.h is a bit odd given that it would be the only > > ioremap_* implementation declared there, I think we need more > > consistency here instead of deviating again from what you did. > > asm-generic/io.h is the right place for generics which let you override. I disagree for two reasons, and I will refer you to Linus' comments back in 2005 at http://yarchive.net/comp/linux/generic.html 1) asm-generic/io.h has become an ifdef mess, every single definition in there is conditional. The reason for this has happened is that architectures can't pick and choose what they want from a single file unless something like that is done. It would be _far_ better to split this file up by functional group - eg, ISA IO accessors, io(read|write)*(), read|write*(), and so forth. 2) We're at the point where we have various .c files around the kernel _directly_ including asm-generic header files, which means the use of asm-generic headers is no longer a choice of the architecture. 3) asm-generic/ started out life as the place where example implementations of asm/*.h header files were found, and but has grown into a place where we dump default implementations. We had a place for default implementations for years, which were the linux/*.h headers. We have now ended up with a mixture of both techniques, even for something like io.h, we have the messy asm-generic/io.h, the architecture's asm/io.h, and then linux/io.h. We have ended up with both linux/io.h and asm-generic/io.h containing default definitions. We've had the rule that where both linux/foo.h and asm/foo.h exist, the linux/ counterpart is the preferred include file. That didn't really happen with asm/io.h due to the number of users that there were, but when new stuff was added to asm/io.h which we wanted to be generic, linux/io.h was created to contain that. This actually pre-dates the "let's clutter up asm-generic with shared arch stuff" push. Now, what you say _may_ make sense if we have an asm-generic/ioremap-nopost.h header which just defines a default ioremap_nopost.h implementation, and architectures would then be able to choose whether to include that or not depending on whether they provide their own implementation. No ugly ifdefs are necessary with that approach. Out of the three choices, I would much rather see the asm-generic/ioremap-nopost.h approach. However, the down-side to that is it means all architectures asm/io.h would need to be touched to explicitly include that. What I'm absolutely certain of, though, is that making asm-generic/io.h even worse by adding yet more conditional stuff to it is not a sane way forward. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
On Thu, Apr 06, 2017 at 01:11:57PM +0100, Russell King - ARM Linux wrote: > On Thu, Apr 06, 2017 at 12:53:12PM +0200, Luis R. Rodriguez wrote: > > On Thu, Apr 06, 2017 at 11:26:36AM +0100, Lorenzo Pieralisi wrote: > > > Indeed, the static inline ioremap_nocache() fallback does not work > > > on all arches (whether I add the fallback in linux/io.h or > > > asm-generic/io.h is irrelevant), I bump into issues such as the one > > > reported above. > > > > Its also not *safe* to assume on behalf of all architectures a new ioremap > > call is both a good idea and proper. > > You may be right in general, but not in this case. > > Currently, many drivers use plain ioremap() to map this resource. We > are replacing that existing call - which is known to work in the majority > of cases - with a new call to cater for different semantics required by > an architecture. > > Doing a replace of these ioremap() calls with ioremap_nopost() in this > situation, and then having ioremap_nopost() fail is a recipe for causing > lots and lots of regressions. > > The only sane approach is to have ioremap_post() default to modelling the > _existing_ behaviour everywhere that it is used. > > Requiring it to fail until architecture folk trip over the failure is > totally insane, and I very strongly disagree with you on this. Ah yes, what if with this modulo rule of thumb: The litmus test then is if an existing set of calls are changed to use a new ioremap then all archs that support those drivers where the new call is being added must be modified to also have a correct corresponding API call ? This is more work on the new person introducing the new API, and should require review still on arch maintainers but it seems like a fair compromise. Then if an API is *new* though then things can move forward without requiring all archs to add the respective call. Luis
Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
On Thu, Apr 06, 2017 at 01:11:57PM +0100, Russell King - ARM Linux wrote: > On Thu, Apr 06, 2017 at 12:53:12PM +0200, Luis R. Rodriguez wrote: > > On Thu, Apr 06, 2017 at 11:26:36AM +0100, Lorenzo Pieralisi wrote: > > > Indeed, the static inline ioremap_nocache() fallback does not work > > > on all arches (whether I add the fallback in linux/io.h or > > > asm-generic/io.h is irrelevant), I bump into issues such as the one > > > reported above. > > > > Its also not *safe* to assume on behalf of all architectures a new ioremap > > call is both a good idea and proper. > > You may be right in general, but not in this case. > > Currently, many drivers use plain ioremap() to map this resource. We > are replacing that existing call - which is known to work in the majority > of cases - with a new call to cater for different semantics required by > an architecture. > > Doing a replace of these ioremap() calls with ioremap_nopost() in this > situation, and then having ioremap_nopost() fail is a recipe for causing > lots and lots of regressions. > > The only sane approach is to have ioremap_post() default to modelling the > _existing_ behaviour everywhere that it is used. > > Requiring it to fail until architecture folk trip over the failure is > totally insane, and I very strongly disagree with you on this. Ah yes, what if with this modulo rule of thumb: The litmus test then is if an existing set of calls are changed to use a new ioremap then all archs that support those drivers where the new call is being added must be modified to also have a correct corresponding API call ? This is more work on the new person introducing the new API, and should require review still on arch maintainers but it seems like a fair compromise. Then if an API is *new* though then things can move forward without requiring all archs to add the respective call. Luis
Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
On Thu, Apr 06, 2017 at 12:53:12PM +0200, Luis R. Rodriguez wrote: > On Thu, Apr 06, 2017 at 11:26:36AM +0100, Lorenzo Pieralisi wrote: > > Indeed, the static inline ioremap_nocache() fallback does not work > > on all arches (whether I add the fallback in linux/io.h or > > asm-generic/io.h is irrelevant), I bump into issues such as the one > > reported above. > > Its also not *safe* to assume on behalf of all architectures a new ioremap > call is both a good idea and proper. You may be right in general, but not in this case. Currently, many drivers use plain ioremap() to map this resource. We are replacing that existing call - which is known to work in the majority of cases - with a new call to cater for different semantics required by an architecture. Doing a replace of these ioremap() calls with ioremap_nopost() in this situation, and then having ioremap_nopost() fail is a recipe for causing lots and lots of regressions. The only sane approach is to have ioremap_post() default to modelling the _existing_ behaviour everywhere that it is used. Requiring it to fail until architecture folk trip over the failure is totally insane, and I very strongly disagree with you on this. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
On Thu, Apr 06, 2017 at 12:53:12PM +0200, Luis R. Rodriguez wrote: > On Thu, Apr 06, 2017 at 11:26:36AM +0100, Lorenzo Pieralisi wrote: > > Indeed, the static inline ioremap_nocache() fallback does not work > > on all arches (whether I add the fallback in linux/io.h or > > asm-generic/io.h is irrelevant), I bump into issues such as the one > > reported above. > > Its also not *safe* to assume on behalf of all architectures a new ioremap > call is both a good idea and proper. You may be right in general, but not in this case. Currently, many drivers use plain ioremap() to map this resource. We are replacing that existing call - which is known to work in the majority of cases - with a new call to cater for different semantics required by an architecture. Doing a replace of these ioremap() calls with ioremap_nopost() in this situation, and then having ioremap_nopost() fail is a recipe for causing lots and lots of regressions. The only sane approach is to have ioremap_post() default to modelling the _existing_ behaviour everywhere that it is used. Requiring it to fail until architecture folk trip over the failure is totally insane, and I very strongly disagree with you on this. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
On Thu, Apr 06, 2017 at 12:38:45PM +0100, Lorenzo Pieralisi wrote: > On Thu, Apr 06, 2017 at 12:53:12PM +0200, Luis R. Rodriguez wrote: > > On Thu, Apr 06, 2017 at 11:26:36AM +0100, Lorenzo Pieralisi wrote: > > > On Wed, Apr 05, 2017 at 01:38:12PM +0100, Lorenzo Pieralisi wrote: > > > > On Wed, Apr 05, 2017 at 11:58:41AM +0100, Russell King - ARM Linux > > > > wrote: > > > > > On Tue, Mar 28, 2017 at 03:45:43PM +0100, Lorenzo Pieralisi wrote: > > > > > > On Mon, Mar 27, 2017 at 08:41:10PM -0500, Bjorn Helgaas wrote: > > > > > > > On Mon, Mar 27, 2017 at 10:49:30AM +0100, Lorenzo Pieralisi wrote: > > > > > > > > diff --git a/arch/x86/include/asm/io.h > > > > > > > > b/arch/x86/include/asm/io.h > > > > > > > > index 7afb0e2..50b292f 100644 > > > > > > > > --- a/arch/x86/include/asm/io.h > > > > > > > > +++ b/arch/x86/include/asm/io.h > > > > > > > > @@ -171,6 +171,7 @@ static inline unsigned int > > > > > > > > isa_virt_to_bus(volatile void *address) > > > > > > > > extern void __iomem *ioremap_nocache(resource_size_t offset, > > > > > > > > unsigned long size); > > > > > > > > extern void __iomem *ioremap_uc(resource_size_t offset, > > > > > > > > unsigned long size); > > > > > > > > #define ioremap_uc ioremap_uc > > > > > > > > +#define ioremap_nopost ioremap_nocache > > > > > > > > > > > > > > These are all the same as the default from asm-generic.h. Do we > > > > > > > really > > > > > > > need these definitions in alpha, avr32, frv, ia64, x86? > > > > > > > > > > > > Those arches do not include asm-generic.h (I suppose for a good > > > > > > reason) > > > > > > but a definition is needed anyway if we want code using > > > > > > ioremap_nopost() > > > > > > to be unconditional. This is one way of doing it, there are others > > > > > > not > > > > > > sure they are any better, I am open to suggestions. > > > > > > > > > > We do have linux/io.h, which should be included in preference to > > > > > asm/io.h. > > > > > linux/io.h has existed for years, but I still see (from time to time) > > > > > patches adding drivers that (imho incorrectly) use asm/io.h. > > > > > > > > > > Also, this: > > > > > > > > > > > > > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h > > > > > > > > index 7ef015e..0e81938 100644 > > > > > > > > --- a/include/asm-generic/io.h > > > > > > > > +++ b/include/asm-generic/io.h > > > > > > > > @@ -915,6 +915,10 @@ extern void ioport_unmap(void __iomem *p); > > > > > > > > #endif /* CONFIG_GENERIC_IOMAP */ > > > > > > > > #endif /* CONFIG_HAS_IOPORT_MAP */ > > > > > > > > > > > > > > > > +#ifndef ioremap_nopost > > > > > > > > +#define ioremap_nopost ioremap_nocache > > > > > > > > +#endif > > > > > > > > + > > > > > > > > #ifndef xlate_dev_kmem_ptr > > > > > > > > #define xlate_dev_kmem_ptr xlate_dev_kmem_ptr > > > > > > > > static inline void *xlate_dev_kmem_ptr(void *addr) > > > > > > > > > > could well be located in linux/io.h, which would make it available > > > > > everywhere. > > > > > > > > > > I'd suggest one change to this though: > > > > > > > > > > #ifndef ioremap_nopost > > > > > static inline void __iomem *ioremap_nopost(resource_size_t offset, > > > > > unsigned long size) > > > > > { > > > > > return ioremap_nocache(offset, size); > > > > > } > > > > > #endif > > > > > > > > > > This way, if someone forgets to define ioremap_nopost() in their > > > > > asm/io.h but provides a definition or extern prototype for > > > > > ioremap_nopost(), we end up with a compile error to highlight the > > > > > error, rather than the error being hidden by the preprocessor. > > > > > > > > Yes, I could add ioremap_nopost() to linux/io.h I did not do it because > > > > linux/io.h, for whatever reason, does not seem to contain ioremap_* > > > > prototypes; this does not mean we should not add it there but that > > > > would look odd (with all others ioremap_* in asm/io.h), all I am > > > > saying. This stuff requires some clean-up regardless, for the records. > > > > > > > > As for the static inline, I did that and that did not make the > > > > kbuild robot happy at all on some arches: > > > > > > > > eg: openrisc > > > > > > > > >> include/asm-generic/io.h:922:9: error: implicit declaration of > > > > >> function 'ioremap_nocache' [-Werror=implicit-function-declaration] > > > > return ioremap_nocache(offset, size); > > > > > > Indeed, the static inline ioremap_nocache() fallback does not work > > > on all arches (whether I add the fallback in linux/io.h or > > > asm-generic/io.h is irrelevant), I bump into issues such as the one > > > reported above. > > > > Its also not *safe* to assume on behalf of all architectures a new ioremap > > call is both a good idea and proper. > > > > > I could make it: > > > > > > #ifndef ioremap_nopost > > > static inline void __iomem *ioremap_nopost(resource_size_t offset, > > > unsigned long size) > > > { > > >
Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
On Thu, Apr 06, 2017 at 12:38:45PM +0100, Lorenzo Pieralisi wrote: > On Thu, Apr 06, 2017 at 12:53:12PM +0200, Luis R. Rodriguez wrote: > > On Thu, Apr 06, 2017 at 11:26:36AM +0100, Lorenzo Pieralisi wrote: > > > On Wed, Apr 05, 2017 at 01:38:12PM +0100, Lorenzo Pieralisi wrote: > > > > On Wed, Apr 05, 2017 at 11:58:41AM +0100, Russell King - ARM Linux > > > > wrote: > > > > > On Tue, Mar 28, 2017 at 03:45:43PM +0100, Lorenzo Pieralisi wrote: > > > > > > On Mon, Mar 27, 2017 at 08:41:10PM -0500, Bjorn Helgaas wrote: > > > > > > > On Mon, Mar 27, 2017 at 10:49:30AM +0100, Lorenzo Pieralisi wrote: > > > > > > > > diff --git a/arch/x86/include/asm/io.h > > > > > > > > b/arch/x86/include/asm/io.h > > > > > > > > index 7afb0e2..50b292f 100644 > > > > > > > > --- a/arch/x86/include/asm/io.h > > > > > > > > +++ b/arch/x86/include/asm/io.h > > > > > > > > @@ -171,6 +171,7 @@ static inline unsigned int > > > > > > > > isa_virt_to_bus(volatile void *address) > > > > > > > > extern void __iomem *ioremap_nocache(resource_size_t offset, > > > > > > > > unsigned long size); > > > > > > > > extern void __iomem *ioremap_uc(resource_size_t offset, > > > > > > > > unsigned long size); > > > > > > > > #define ioremap_uc ioremap_uc > > > > > > > > +#define ioremap_nopost ioremap_nocache > > > > > > > > > > > > > > These are all the same as the default from asm-generic.h. Do we > > > > > > > really > > > > > > > need these definitions in alpha, avr32, frv, ia64, x86? > > > > > > > > > > > > Those arches do not include asm-generic.h (I suppose for a good > > > > > > reason) > > > > > > but a definition is needed anyway if we want code using > > > > > > ioremap_nopost() > > > > > > to be unconditional. This is one way of doing it, there are others > > > > > > not > > > > > > sure they are any better, I am open to suggestions. > > > > > > > > > > We do have linux/io.h, which should be included in preference to > > > > > asm/io.h. > > > > > linux/io.h has existed for years, but I still see (from time to time) > > > > > patches adding drivers that (imho incorrectly) use asm/io.h. > > > > > > > > > > Also, this: > > > > > > > > > > > > > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h > > > > > > > > index 7ef015e..0e81938 100644 > > > > > > > > --- a/include/asm-generic/io.h > > > > > > > > +++ b/include/asm-generic/io.h > > > > > > > > @@ -915,6 +915,10 @@ extern void ioport_unmap(void __iomem *p); > > > > > > > > #endif /* CONFIG_GENERIC_IOMAP */ > > > > > > > > #endif /* CONFIG_HAS_IOPORT_MAP */ > > > > > > > > > > > > > > > > +#ifndef ioremap_nopost > > > > > > > > +#define ioremap_nopost ioremap_nocache > > > > > > > > +#endif > > > > > > > > + > > > > > > > > #ifndef xlate_dev_kmem_ptr > > > > > > > > #define xlate_dev_kmem_ptr xlate_dev_kmem_ptr > > > > > > > > static inline void *xlate_dev_kmem_ptr(void *addr) > > > > > > > > > > could well be located in linux/io.h, which would make it available > > > > > everywhere. > > > > > > > > > > I'd suggest one change to this though: > > > > > > > > > > #ifndef ioremap_nopost > > > > > static inline void __iomem *ioremap_nopost(resource_size_t offset, > > > > > unsigned long size) > > > > > { > > > > > return ioremap_nocache(offset, size); > > > > > } > > > > > #endif > > > > > > > > > > This way, if someone forgets to define ioremap_nopost() in their > > > > > asm/io.h but provides a definition or extern prototype for > > > > > ioremap_nopost(), we end up with a compile error to highlight the > > > > > error, rather than the error being hidden by the preprocessor. > > > > > > > > Yes, I could add ioremap_nopost() to linux/io.h I did not do it because > > > > linux/io.h, for whatever reason, does not seem to contain ioremap_* > > > > prototypes; this does not mean we should not add it there but that > > > > would look odd (with all others ioremap_* in asm/io.h), all I am > > > > saying. This stuff requires some clean-up regardless, for the records. > > > > > > > > As for the static inline, I did that and that did not make the > > > > kbuild robot happy at all on some arches: > > > > > > > > eg: openrisc > > > > > > > > >> include/asm-generic/io.h:922:9: error: implicit declaration of > > > > >> function 'ioremap_nocache' [-Werror=implicit-function-declaration] > > > > return ioremap_nocache(offset, size); > > > > > > Indeed, the static inline ioremap_nocache() fallback does not work > > > on all arches (whether I add the fallback in linux/io.h or > > > asm-generic/io.h is irrelevant), I bump into issues such as the one > > > reported above. > > > > Its also not *safe* to assume on behalf of all architectures a new ioremap > > call is both a good idea and proper. > > > > > I could make it: > > > > > > #ifndef ioremap_nopost > > > static inline void __iomem *ioremap_nopost(resource_size_t offset, > > > unsigned long size) > > > { > > >
Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
On Thu, Apr 06, 2017 at 12:53:12PM +0200, Luis R. Rodriguez wrote: > On Thu, Apr 06, 2017 at 11:26:36AM +0100, Lorenzo Pieralisi wrote: > > On Wed, Apr 05, 2017 at 01:38:12PM +0100, Lorenzo Pieralisi wrote: > > > On Wed, Apr 05, 2017 at 11:58:41AM +0100, Russell King - ARM Linux wrote: > > > > On Tue, Mar 28, 2017 at 03:45:43PM +0100, Lorenzo Pieralisi wrote: > > > > > On Mon, Mar 27, 2017 at 08:41:10PM -0500, Bjorn Helgaas wrote: > > > > > > On Mon, Mar 27, 2017 at 10:49:30AM +0100, Lorenzo Pieralisi wrote: > > > > > > > diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h > > > > > > > index 7afb0e2..50b292f 100644 > > > > > > > --- a/arch/x86/include/asm/io.h > > > > > > > +++ b/arch/x86/include/asm/io.h > > > > > > > @@ -171,6 +171,7 @@ static inline unsigned int > > > > > > > isa_virt_to_bus(volatile void *address) > > > > > > > extern void __iomem *ioremap_nocache(resource_size_t offset, > > > > > > > unsigned long size); > > > > > > > extern void __iomem *ioremap_uc(resource_size_t offset, unsigned > > > > > > > long size); > > > > > > > #define ioremap_uc ioremap_uc > > > > > > > +#define ioremap_nopost ioremap_nocache > > > > > > > > > > > > These are all the same as the default from asm-generic.h. Do we > > > > > > really > > > > > > need these definitions in alpha, avr32, frv, ia64, x86? > > > > > > > > > > Those arches do not include asm-generic.h (I suppose for a good > > > > > reason) > > > > > but a definition is needed anyway if we want code using > > > > > ioremap_nopost() > > > > > to be unconditional. This is one way of doing it, there are others not > > > > > sure they are any better, I am open to suggestions. > > > > > > > > We do have linux/io.h, which should be included in preference to > > > > asm/io.h. > > > > linux/io.h has existed for years, but I still see (from time to time) > > > > patches adding drivers that (imho incorrectly) use asm/io.h. > > > > > > > > Also, this: > > > > > > > > > > > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h > > > > > > > index 7ef015e..0e81938 100644 > > > > > > > --- a/include/asm-generic/io.h > > > > > > > +++ b/include/asm-generic/io.h > > > > > > > @@ -915,6 +915,10 @@ extern void ioport_unmap(void __iomem *p); > > > > > > > #endif /* CONFIG_GENERIC_IOMAP */ > > > > > > > #endif /* CONFIG_HAS_IOPORT_MAP */ > > > > > > > > > > > > > > +#ifndef ioremap_nopost > > > > > > > +#define ioremap_nopost ioremap_nocache > > > > > > > +#endif > > > > > > > + > > > > > > > #ifndef xlate_dev_kmem_ptr > > > > > > > #define xlate_dev_kmem_ptr xlate_dev_kmem_ptr > > > > > > > static inline void *xlate_dev_kmem_ptr(void *addr) > > > > > > > > could well be located in linux/io.h, which would make it available > > > > everywhere. > > > > > > > > I'd suggest one change to this though: > > > > > > > > #ifndef ioremap_nopost > > > > static inline void __iomem *ioremap_nopost(resource_size_t offset, > > > >unsigned long size) > > > > { > > > > return ioremap_nocache(offset, size); > > > > } > > > > #endif > > > > > > > > This way, if someone forgets to define ioremap_nopost() in their > > > > asm/io.h but provides a definition or extern prototype for > > > > ioremap_nopost(), we end up with a compile error to highlight the > > > > error, rather than the error being hidden by the preprocessor. > > > > > > Yes, I could add ioremap_nopost() to linux/io.h I did not do it because > > > linux/io.h, for whatever reason, does not seem to contain ioremap_* > > > prototypes; this does not mean we should not add it there but that > > > would look odd (with all others ioremap_* in asm/io.h), all I am > > > saying. This stuff requires some clean-up regardless, for the records. > > > > > > As for the static inline, I did that and that did not make the > > > kbuild robot happy at all on some arches: > > > > > > eg: openrisc > > > > > > >> include/asm-generic/io.h:922:9: error: implicit declaration of > > > >> function 'ioremap_nocache' [-Werror=implicit-function-declaration] > > > return ioremap_nocache(offset, size); > > > > Indeed, the static inline ioremap_nocache() fallback does not work > > on all arches (whether I add the fallback in linux/io.h or > > asm-generic/io.h is irrelevant), I bump into issues such as the one > > reported above. > > Its also not *safe* to assume on behalf of all architectures a new ioremap > call is both a good idea and proper. > > > I could make it: > > > > #ifndef ioremap_nopost > > static inline void __iomem *ioremap_nopost(resource_size_t offset, > >unsigned long size) > > { > > return NULL; > > } > > #endif > > > > which would force arches to define ioremap_nopost() (if they need it). > > Yes, please do this. Ok but where ? linux/io.h or asm-generic/io.h ? As I replied to Russell, having it in linux/io.h is a bit odd given that it would be the only ioremap_* implementation
Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
On Thu, Apr 06, 2017 at 12:53:12PM +0200, Luis R. Rodriguez wrote: > On Thu, Apr 06, 2017 at 11:26:36AM +0100, Lorenzo Pieralisi wrote: > > On Wed, Apr 05, 2017 at 01:38:12PM +0100, Lorenzo Pieralisi wrote: > > > On Wed, Apr 05, 2017 at 11:58:41AM +0100, Russell King - ARM Linux wrote: > > > > On Tue, Mar 28, 2017 at 03:45:43PM +0100, Lorenzo Pieralisi wrote: > > > > > On Mon, Mar 27, 2017 at 08:41:10PM -0500, Bjorn Helgaas wrote: > > > > > > On Mon, Mar 27, 2017 at 10:49:30AM +0100, Lorenzo Pieralisi wrote: > > > > > > > diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h > > > > > > > index 7afb0e2..50b292f 100644 > > > > > > > --- a/arch/x86/include/asm/io.h > > > > > > > +++ b/arch/x86/include/asm/io.h > > > > > > > @@ -171,6 +171,7 @@ static inline unsigned int > > > > > > > isa_virt_to_bus(volatile void *address) > > > > > > > extern void __iomem *ioremap_nocache(resource_size_t offset, > > > > > > > unsigned long size); > > > > > > > extern void __iomem *ioremap_uc(resource_size_t offset, unsigned > > > > > > > long size); > > > > > > > #define ioremap_uc ioremap_uc > > > > > > > +#define ioremap_nopost ioremap_nocache > > > > > > > > > > > > These are all the same as the default from asm-generic.h. Do we > > > > > > really > > > > > > need these definitions in alpha, avr32, frv, ia64, x86? > > > > > > > > > > Those arches do not include asm-generic.h (I suppose for a good > > > > > reason) > > > > > but a definition is needed anyway if we want code using > > > > > ioremap_nopost() > > > > > to be unconditional. This is one way of doing it, there are others not > > > > > sure they are any better, I am open to suggestions. > > > > > > > > We do have linux/io.h, which should be included in preference to > > > > asm/io.h. > > > > linux/io.h has existed for years, but I still see (from time to time) > > > > patches adding drivers that (imho incorrectly) use asm/io.h. > > > > > > > > Also, this: > > > > > > > > > > > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h > > > > > > > index 7ef015e..0e81938 100644 > > > > > > > --- a/include/asm-generic/io.h > > > > > > > +++ b/include/asm-generic/io.h > > > > > > > @@ -915,6 +915,10 @@ extern void ioport_unmap(void __iomem *p); > > > > > > > #endif /* CONFIG_GENERIC_IOMAP */ > > > > > > > #endif /* CONFIG_HAS_IOPORT_MAP */ > > > > > > > > > > > > > > +#ifndef ioremap_nopost > > > > > > > +#define ioremap_nopost ioremap_nocache > > > > > > > +#endif > > > > > > > + > > > > > > > #ifndef xlate_dev_kmem_ptr > > > > > > > #define xlate_dev_kmem_ptr xlate_dev_kmem_ptr > > > > > > > static inline void *xlate_dev_kmem_ptr(void *addr) > > > > > > > > could well be located in linux/io.h, which would make it available > > > > everywhere. > > > > > > > > I'd suggest one change to this though: > > > > > > > > #ifndef ioremap_nopost > > > > static inline void __iomem *ioremap_nopost(resource_size_t offset, > > > >unsigned long size) > > > > { > > > > return ioremap_nocache(offset, size); > > > > } > > > > #endif > > > > > > > > This way, if someone forgets to define ioremap_nopost() in their > > > > asm/io.h but provides a definition or extern prototype for > > > > ioremap_nopost(), we end up with a compile error to highlight the > > > > error, rather than the error being hidden by the preprocessor. > > > > > > Yes, I could add ioremap_nopost() to linux/io.h I did not do it because > > > linux/io.h, for whatever reason, does not seem to contain ioremap_* > > > prototypes; this does not mean we should not add it there but that > > > would look odd (with all others ioremap_* in asm/io.h), all I am > > > saying. This stuff requires some clean-up regardless, for the records. > > > > > > As for the static inline, I did that and that did not make the > > > kbuild robot happy at all on some arches: > > > > > > eg: openrisc > > > > > > >> include/asm-generic/io.h:922:9: error: implicit declaration of > > > >> function 'ioremap_nocache' [-Werror=implicit-function-declaration] > > > return ioremap_nocache(offset, size); > > > > Indeed, the static inline ioremap_nocache() fallback does not work > > on all arches (whether I add the fallback in linux/io.h or > > asm-generic/io.h is irrelevant), I bump into issues such as the one > > reported above. > > Its also not *safe* to assume on behalf of all architectures a new ioremap > call is both a good idea and proper. > > > I could make it: > > > > #ifndef ioremap_nopost > > static inline void __iomem *ioremap_nopost(resource_size_t offset, > >unsigned long size) > > { > > return NULL; > > } > > #endif > > > > which would force arches to define ioremap_nopost() (if they need it). > > Yes, please do this. Ok but where ? linux/io.h or asm-generic/io.h ? As I replied to Russell, having it in linux/io.h is a bit odd given that it would be the only ioremap_* implementation
Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
On Thu, Apr 06, 2017 at 11:26:36AM +0100, Lorenzo Pieralisi wrote: > On Wed, Apr 05, 2017 at 01:38:12PM +0100, Lorenzo Pieralisi wrote: > > On Wed, Apr 05, 2017 at 11:58:41AM +0100, Russell King - ARM Linux wrote: > > > On Tue, Mar 28, 2017 at 03:45:43PM +0100, Lorenzo Pieralisi wrote: > > > > On Mon, Mar 27, 2017 at 08:41:10PM -0500, Bjorn Helgaas wrote: > > > > > On Mon, Mar 27, 2017 at 10:49:30AM +0100, Lorenzo Pieralisi wrote: > > > > > > diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h > > > > > > index 7afb0e2..50b292f 100644 > > > > > > --- a/arch/x86/include/asm/io.h > > > > > > +++ b/arch/x86/include/asm/io.h > > > > > > @@ -171,6 +171,7 @@ static inline unsigned int > > > > > > isa_virt_to_bus(volatile void *address) > > > > > > extern void __iomem *ioremap_nocache(resource_size_t offset, > > > > > > unsigned long size); > > > > > > extern void __iomem *ioremap_uc(resource_size_t offset, unsigned > > > > > > long size); > > > > > > #define ioremap_uc ioremap_uc > > > > > > +#define ioremap_nopost ioremap_nocache > > > > > > > > > > These are all the same as the default from asm-generic.h. Do we > > > > > really > > > > > need these definitions in alpha, avr32, frv, ia64, x86? > > > > > > > > Those arches do not include asm-generic.h (I suppose for a good reason) > > > > but a definition is needed anyway if we want code using ioremap_nopost() > > > > to be unconditional. This is one way of doing it, there are others not > > > > sure they are any better, I am open to suggestions. > > > > > > We do have linux/io.h, which should be included in preference to asm/io.h. > > > linux/io.h has existed for years, but I still see (from time to time) > > > patches adding drivers that (imho incorrectly) use asm/io.h. > > > > > > Also, this: > > > > > > > > > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h > > > > > > index 7ef015e..0e81938 100644 > > > > > > --- a/include/asm-generic/io.h > > > > > > +++ b/include/asm-generic/io.h > > > > > > @@ -915,6 +915,10 @@ extern void ioport_unmap(void __iomem *p); > > > > > > #endif /* CONFIG_GENERIC_IOMAP */ > > > > > > #endif /* CONFIG_HAS_IOPORT_MAP */ > > > > > > > > > > > > +#ifndef ioremap_nopost > > > > > > +#define ioremap_nopost ioremap_nocache > > > > > > +#endif > > > > > > + > > > > > > #ifndef xlate_dev_kmem_ptr > > > > > > #define xlate_dev_kmem_ptr xlate_dev_kmem_ptr > > > > > > static inline void *xlate_dev_kmem_ptr(void *addr) > > > > > > could well be located in linux/io.h, which would make it available > > > everywhere. > > > > > > I'd suggest one change to this though: > > > > > > #ifndef ioremap_nopost > > > static inline void __iomem *ioremap_nopost(resource_size_t offset, > > > unsigned long size) > > > { > > > return ioremap_nocache(offset, size); > > > } > > > #endif > > > > > > This way, if someone forgets to define ioremap_nopost() in their > > > asm/io.h but provides a definition or extern prototype for > > > ioremap_nopost(), we end up with a compile error to highlight the > > > error, rather than the error being hidden by the preprocessor. > > > > Yes, I could add ioremap_nopost() to linux/io.h I did not do it because > > linux/io.h, for whatever reason, does not seem to contain ioremap_* > > prototypes; this does not mean we should not add it there but that > > would look odd (with all others ioremap_* in asm/io.h), all I am > > saying. This stuff requires some clean-up regardless, for the records. > > > > As for the static inline, I did that and that did not make the > > kbuild robot happy at all on some arches: > > > > eg: openrisc > > > > >> include/asm-generic/io.h:922:9: error: implicit declaration of > > >> function 'ioremap_nocache' [-Werror=implicit-function-declaration] > > return ioremap_nocache(offset, size); > > Indeed, the static inline ioremap_nocache() fallback does not work > on all arches (whether I add the fallback in linux/io.h or > asm-generic/io.h is irrelevant), I bump into issues such as the one > reported above. Its also not *safe* to assume on behalf of all architectures a new ioremap call is both a good idea and proper. > I could make it: > > #ifndef ioremap_nopost > static inline void __iomem *ioremap_nopost(resource_size_t offset, > unsigned long size) > { > return NULL; > } > #endif > > which would force arches to define ioremap_nopost() (if they need it). Yes, please do this. > This is what I *assume* was done for ioremap_uc() in asm-generic/io.h, > but honestly history is hard to follow here. > > Thoughts ? Hard to follow ? I think commit 8c7ea50c010b2 ("x86/mm, asm-generic: Add IOMMU ioremap_uc() variant default") makes it very clear that historically we have made bad assumptions and these assumptions are not safe, so the only correct thing we can do to not stall development is to do what you did above, and each architecture then can add support
Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
On Thu, Apr 06, 2017 at 11:26:36AM +0100, Lorenzo Pieralisi wrote: > On Wed, Apr 05, 2017 at 01:38:12PM +0100, Lorenzo Pieralisi wrote: > > On Wed, Apr 05, 2017 at 11:58:41AM +0100, Russell King - ARM Linux wrote: > > > On Tue, Mar 28, 2017 at 03:45:43PM +0100, Lorenzo Pieralisi wrote: > > > > On Mon, Mar 27, 2017 at 08:41:10PM -0500, Bjorn Helgaas wrote: > > > > > On Mon, Mar 27, 2017 at 10:49:30AM +0100, Lorenzo Pieralisi wrote: > > > > > > diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h > > > > > > index 7afb0e2..50b292f 100644 > > > > > > --- a/arch/x86/include/asm/io.h > > > > > > +++ b/arch/x86/include/asm/io.h > > > > > > @@ -171,6 +171,7 @@ static inline unsigned int > > > > > > isa_virt_to_bus(volatile void *address) > > > > > > extern void __iomem *ioremap_nocache(resource_size_t offset, > > > > > > unsigned long size); > > > > > > extern void __iomem *ioremap_uc(resource_size_t offset, unsigned > > > > > > long size); > > > > > > #define ioremap_uc ioremap_uc > > > > > > +#define ioremap_nopost ioremap_nocache > > > > > > > > > > These are all the same as the default from asm-generic.h. Do we > > > > > really > > > > > need these definitions in alpha, avr32, frv, ia64, x86? > > > > > > > > Those arches do not include asm-generic.h (I suppose for a good reason) > > > > but a definition is needed anyway if we want code using ioremap_nopost() > > > > to be unconditional. This is one way of doing it, there are others not > > > > sure they are any better, I am open to suggestions. > > > > > > We do have linux/io.h, which should be included in preference to asm/io.h. > > > linux/io.h has existed for years, but I still see (from time to time) > > > patches adding drivers that (imho incorrectly) use asm/io.h. > > > > > > Also, this: > > > > > > > > > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h > > > > > > index 7ef015e..0e81938 100644 > > > > > > --- a/include/asm-generic/io.h > > > > > > +++ b/include/asm-generic/io.h > > > > > > @@ -915,6 +915,10 @@ extern void ioport_unmap(void __iomem *p); > > > > > > #endif /* CONFIG_GENERIC_IOMAP */ > > > > > > #endif /* CONFIG_HAS_IOPORT_MAP */ > > > > > > > > > > > > +#ifndef ioremap_nopost > > > > > > +#define ioremap_nopost ioremap_nocache > > > > > > +#endif > > > > > > + > > > > > > #ifndef xlate_dev_kmem_ptr > > > > > > #define xlate_dev_kmem_ptr xlate_dev_kmem_ptr > > > > > > static inline void *xlate_dev_kmem_ptr(void *addr) > > > > > > could well be located in linux/io.h, which would make it available > > > everywhere. > > > > > > I'd suggest one change to this though: > > > > > > #ifndef ioremap_nopost > > > static inline void __iomem *ioremap_nopost(resource_size_t offset, > > > unsigned long size) > > > { > > > return ioremap_nocache(offset, size); > > > } > > > #endif > > > > > > This way, if someone forgets to define ioremap_nopost() in their > > > asm/io.h but provides a definition or extern prototype for > > > ioremap_nopost(), we end up with a compile error to highlight the > > > error, rather than the error being hidden by the preprocessor. > > > > Yes, I could add ioremap_nopost() to linux/io.h I did not do it because > > linux/io.h, for whatever reason, does not seem to contain ioremap_* > > prototypes; this does not mean we should not add it there but that > > would look odd (with all others ioremap_* in asm/io.h), all I am > > saying. This stuff requires some clean-up regardless, for the records. > > > > As for the static inline, I did that and that did not make the > > kbuild robot happy at all on some arches: > > > > eg: openrisc > > > > >> include/asm-generic/io.h:922:9: error: implicit declaration of > > >> function 'ioremap_nocache' [-Werror=implicit-function-declaration] > > return ioremap_nocache(offset, size); > > Indeed, the static inline ioremap_nocache() fallback does not work > on all arches (whether I add the fallback in linux/io.h or > asm-generic/io.h is irrelevant), I bump into issues such as the one > reported above. Its also not *safe* to assume on behalf of all architectures a new ioremap call is both a good idea and proper. > I could make it: > > #ifndef ioremap_nopost > static inline void __iomem *ioremap_nopost(resource_size_t offset, > unsigned long size) > { > return NULL; > } > #endif > > which would force arches to define ioremap_nopost() (if they need it). Yes, please do this. > This is what I *assume* was done for ioremap_uc() in asm-generic/io.h, > but honestly history is hard to follow here. > > Thoughts ? Hard to follow ? I think commit 8c7ea50c010b2 ("x86/mm, asm-generic: Add IOMMU ioremap_uc() variant default") makes it very clear that historically we have made bad assumptions and these assumptions are not safe, so the only correct thing we can do to not stall development is to do what you did above, and each architecture then can add support
Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
On Thu, Apr 06, 2017 at 11:26:36AM +0100, Lorenzo Pieralisi wrote: > On Wed, Apr 05, 2017 at 01:38:12PM +0100, Lorenzo Pieralisi wrote: > > eg: openrisc > > > > >> include/asm-generic/io.h:922:9: error: implicit declaration of > > >> function 'ioremap_nocache' [-Werror=implicit-function-declaration] > > return ioremap_nocache(offset, size); > > Indeed, the static inline ioremap_nocache() fallback does not work > on all arches (whether I add the fallback in linux/io.h or > asm-generic/io.h is irrelevant), I bump into issues such as the one > reported above. >From what I can see: (a) openrisc does define ioremap_nocache() in its asm/io.h (b) these do not: $ grep -L 'ioremap_nocache' arch/*/include/asm/io.h arch/blackfin/include/asm/io.h arch/h8300/include/asm/io.h arch/m68k/include/asm/io.h arch/score/include/asm/io.h arch/sparc/include/asm/io.h Out of those, blackfin, h8300 and score do not define it, whereas m68k and sparc do in other headers included by asm/io.h. So it looks like we have three problem architectures that don't define an ioremap_nocache(). PCI on blackfin depends on BROKEN, so it's not selectable. From what I can tell, h8300 and score do not allow PCI to be enabled (but maybe its buried elsewhere in their Kconfig files, I didn't check.) So, I think a way around this is to make ioremap_nopost() conditional on PCI in linux/io.h for the time being - if its scope wants to be enlarged, then these three architectures would need to have either an ioremap_nopost() implementation added, or an ioremap_nocache() implementation. > I could make it: > > #ifndef ioremap_nopost > static inline void __iomem *ioremap_nopost(resource_size_t offset, > unsigned long size) > { > return NULL; > } > #endif > > which would force arches to define ioremap_nopost() (if they need it). > > This is what I *assume* was done for ioremap_uc() in asm-generic/io.h, > but honestly history is hard to follow here. If we are going to consider doing that, the correct question we need to be asking is whether anything will break as a result of this - is there any existing arch using the code as it stands that will end up being broken when we switch PCI to use ioremap_nopost(), and we end up using this NULL- returning default? -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
On Thu, Apr 06, 2017 at 11:26:36AM +0100, Lorenzo Pieralisi wrote: > On Wed, Apr 05, 2017 at 01:38:12PM +0100, Lorenzo Pieralisi wrote: > > eg: openrisc > > > > >> include/asm-generic/io.h:922:9: error: implicit declaration of > > >> function 'ioremap_nocache' [-Werror=implicit-function-declaration] > > return ioremap_nocache(offset, size); > > Indeed, the static inline ioremap_nocache() fallback does not work > on all arches (whether I add the fallback in linux/io.h or > asm-generic/io.h is irrelevant), I bump into issues such as the one > reported above. >From what I can see: (a) openrisc does define ioremap_nocache() in its asm/io.h (b) these do not: $ grep -L 'ioremap_nocache' arch/*/include/asm/io.h arch/blackfin/include/asm/io.h arch/h8300/include/asm/io.h arch/m68k/include/asm/io.h arch/score/include/asm/io.h arch/sparc/include/asm/io.h Out of those, blackfin, h8300 and score do not define it, whereas m68k and sparc do in other headers included by asm/io.h. So it looks like we have three problem architectures that don't define an ioremap_nocache(). PCI on blackfin depends on BROKEN, so it's not selectable. From what I can tell, h8300 and score do not allow PCI to be enabled (but maybe its buried elsewhere in their Kconfig files, I didn't check.) So, I think a way around this is to make ioremap_nopost() conditional on PCI in linux/io.h for the time being - if its scope wants to be enlarged, then these three architectures would need to have either an ioremap_nopost() implementation added, or an ioremap_nocache() implementation. > I could make it: > > #ifndef ioremap_nopost > static inline void __iomem *ioremap_nopost(resource_size_t offset, > unsigned long size) > { > return NULL; > } > #endif > > which would force arches to define ioremap_nopost() (if they need it). > > This is what I *assume* was done for ioremap_uc() in asm-generic/io.h, > but honestly history is hard to follow here. If we are going to consider doing that, the correct question we need to be asking is whether anything will break as a result of this - is there any existing arch using the code as it stands that will end up being broken when we switch PCI to use ioremap_nopost(), and we end up using this NULL- returning default? -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
On Wed, Apr 05, 2017 at 01:38:12PM +0100, Lorenzo Pieralisi wrote: > On Wed, Apr 05, 2017 at 11:58:41AM +0100, Russell King - ARM Linux wrote: > > On Tue, Mar 28, 2017 at 03:45:43PM +0100, Lorenzo Pieralisi wrote: > > > On Mon, Mar 27, 2017 at 08:41:10PM -0500, Bjorn Helgaas wrote: > > > > On Mon, Mar 27, 2017 at 10:49:30AM +0100, Lorenzo Pieralisi wrote: > > > > > diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h > > > > > index 7afb0e2..50b292f 100644 > > > > > --- a/arch/x86/include/asm/io.h > > > > > +++ b/arch/x86/include/asm/io.h > > > > > @@ -171,6 +171,7 @@ static inline unsigned int > > > > > isa_virt_to_bus(volatile void *address) > > > > > extern void __iomem *ioremap_nocache(resource_size_t offset, > > > > > unsigned long size); > > > > > extern void __iomem *ioremap_uc(resource_size_t offset, unsigned > > > > > long size); > > > > > #define ioremap_uc ioremap_uc > > > > > +#define ioremap_nopost ioremap_nocache > > > > > > > > These are all the same as the default from asm-generic.h. Do we really > > > > need these definitions in alpha, avr32, frv, ia64, x86? > > > > > > Those arches do not include asm-generic.h (I suppose for a good reason) > > > but a definition is needed anyway if we want code using ioremap_nopost() > > > to be unconditional. This is one way of doing it, there are others not > > > sure they are any better, I am open to suggestions. > > > > We do have linux/io.h, which should be included in preference to asm/io.h. > > linux/io.h has existed for years, but I still see (from time to time) > > patches adding drivers that (imho incorrectly) use asm/io.h. > > > > Also, this: > > > > > > > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h > > > > > index 7ef015e..0e81938 100644 > > > > > --- a/include/asm-generic/io.h > > > > > +++ b/include/asm-generic/io.h > > > > > @@ -915,6 +915,10 @@ extern void ioport_unmap(void __iomem *p); > > > > > #endif /* CONFIG_GENERIC_IOMAP */ > > > > > #endif /* CONFIG_HAS_IOPORT_MAP */ > > > > > > > > > > +#ifndef ioremap_nopost > > > > > +#define ioremap_nopost ioremap_nocache > > > > > +#endif > > > > > + > > > > > #ifndef xlate_dev_kmem_ptr > > > > > #define xlate_dev_kmem_ptr xlate_dev_kmem_ptr > > > > > static inline void *xlate_dev_kmem_ptr(void *addr) > > > > could well be located in linux/io.h, which would make it available > > everywhere. > > > > I'd suggest one change to this though: > > > > #ifndef ioremap_nopost > > static inline void __iomem *ioremap_nopost(resource_size_t offset, > >unsigned long size) > > { > > return ioremap_nocache(offset, size); > > } > > #endif > > > > This way, if someone forgets to define ioremap_nopost() in their > > asm/io.h but provides a definition or extern prototype for > > ioremap_nopost(), we end up with a compile error to highlight the > > error, rather than the error being hidden by the preprocessor. > > Yes, I could add ioremap_nopost() to linux/io.h I did not do it because > linux/io.h, for whatever reason, does not seem to contain ioremap_* > prototypes; this does not mean we should not add it there but that > would look odd (with all others ioremap_* in asm/io.h), all I am > saying. This stuff requires some clean-up regardless, for the records. > > As for the static inline, I did that and that did not make the > kbuild robot happy at all on some arches: > > eg: openrisc > > >> include/asm-generic/io.h:922:9: error: implicit declaration of > >> function 'ioremap_nocache' [-Werror=implicit-function-declaration] > return ioremap_nocache(offset, size); Indeed, the static inline ioremap_nocache() fallback does not work on all arches (whether I add the fallback in linux/io.h or asm-generic/io.h is irrelevant), I bump into issues such as the one reported above. I could make it: #ifndef ioremap_nopost static inline void __iomem *ioremap_nopost(resource_size_t offset, unsigned long size) { return NULL; } #endif which would force arches to define ioremap_nopost() (if they need it). This is what I *assume* was done for ioremap_uc() in asm-generic/io.h, but honestly history is hard to follow here. Thoughts ? Thanks, Lorenzo
Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
On Wed, Apr 05, 2017 at 01:38:12PM +0100, Lorenzo Pieralisi wrote: > On Wed, Apr 05, 2017 at 11:58:41AM +0100, Russell King - ARM Linux wrote: > > On Tue, Mar 28, 2017 at 03:45:43PM +0100, Lorenzo Pieralisi wrote: > > > On Mon, Mar 27, 2017 at 08:41:10PM -0500, Bjorn Helgaas wrote: > > > > On Mon, Mar 27, 2017 at 10:49:30AM +0100, Lorenzo Pieralisi wrote: > > > > > diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h > > > > > index 7afb0e2..50b292f 100644 > > > > > --- a/arch/x86/include/asm/io.h > > > > > +++ b/arch/x86/include/asm/io.h > > > > > @@ -171,6 +171,7 @@ static inline unsigned int > > > > > isa_virt_to_bus(volatile void *address) > > > > > extern void __iomem *ioremap_nocache(resource_size_t offset, > > > > > unsigned long size); > > > > > extern void __iomem *ioremap_uc(resource_size_t offset, unsigned > > > > > long size); > > > > > #define ioremap_uc ioremap_uc > > > > > +#define ioremap_nopost ioremap_nocache > > > > > > > > These are all the same as the default from asm-generic.h. Do we really > > > > need these definitions in alpha, avr32, frv, ia64, x86? > > > > > > Those arches do not include asm-generic.h (I suppose for a good reason) > > > but a definition is needed anyway if we want code using ioremap_nopost() > > > to be unconditional. This is one way of doing it, there are others not > > > sure they are any better, I am open to suggestions. > > > > We do have linux/io.h, which should be included in preference to asm/io.h. > > linux/io.h has existed for years, but I still see (from time to time) > > patches adding drivers that (imho incorrectly) use asm/io.h. > > > > Also, this: > > > > > > > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h > > > > > index 7ef015e..0e81938 100644 > > > > > --- a/include/asm-generic/io.h > > > > > +++ b/include/asm-generic/io.h > > > > > @@ -915,6 +915,10 @@ extern void ioport_unmap(void __iomem *p); > > > > > #endif /* CONFIG_GENERIC_IOMAP */ > > > > > #endif /* CONFIG_HAS_IOPORT_MAP */ > > > > > > > > > > +#ifndef ioremap_nopost > > > > > +#define ioremap_nopost ioremap_nocache > > > > > +#endif > > > > > + > > > > > #ifndef xlate_dev_kmem_ptr > > > > > #define xlate_dev_kmem_ptr xlate_dev_kmem_ptr > > > > > static inline void *xlate_dev_kmem_ptr(void *addr) > > > > could well be located in linux/io.h, which would make it available > > everywhere. > > > > I'd suggest one change to this though: > > > > #ifndef ioremap_nopost > > static inline void __iomem *ioremap_nopost(resource_size_t offset, > >unsigned long size) > > { > > return ioremap_nocache(offset, size); > > } > > #endif > > > > This way, if someone forgets to define ioremap_nopost() in their > > asm/io.h but provides a definition or extern prototype for > > ioremap_nopost(), we end up with a compile error to highlight the > > error, rather than the error being hidden by the preprocessor. > > Yes, I could add ioremap_nopost() to linux/io.h I did not do it because > linux/io.h, for whatever reason, does not seem to contain ioremap_* > prototypes; this does not mean we should not add it there but that > would look odd (with all others ioremap_* in asm/io.h), all I am > saying. This stuff requires some clean-up regardless, for the records. > > As for the static inline, I did that and that did not make the > kbuild robot happy at all on some arches: > > eg: openrisc > > >> include/asm-generic/io.h:922:9: error: implicit declaration of > >> function 'ioremap_nocache' [-Werror=implicit-function-declaration] > return ioremap_nocache(offset, size); Indeed, the static inline ioremap_nocache() fallback does not work on all arches (whether I add the fallback in linux/io.h or asm-generic/io.h is irrelevant), I bump into issues such as the one reported above. I could make it: #ifndef ioremap_nopost static inline void __iomem *ioremap_nopost(resource_size_t offset, unsigned long size) { return NULL; } #endif which would force arches to define ioremap_nopost() (if they need it). This is what I *assume* was done for ioremap_uc() in asm-generic/io.h, but honestly history is hard to follow here. Thoughts ? Thanks, Lorenzo
Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
On Wed, Apr 05, 2017 at 11:58:41AM +0100, Russell King - ARM Linux wrote: > On Tue, Mar 28, 2017 at 03:45:43PM +0100, Lorenzo Pieralisi wrote: > > On Mon, Mar 27, 2017 at 08:41:10PM -0500, Bjorn Helgaas wrote: > > > On Mon, Mar 27, 2017 at 10:49:30AM +0100, Lorenzo Pieralisi wrote: > > > > diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h > > > > index 7afb0e2..50b292f 100644 > > > > --- a/arch/x86/include/asm/io.h > > > > +++ b/arch/x86/include/asm/io.h > > > > @@ -171,6 +171,7 @@ static inline unsigned int isa_virt_to_bus(volatile > > > > void *address) > > > > extern void __iomem *ioremap_nocache(resource_size_t offset, unsigned > > > > long size); > > > > extern void __iomem *ioremap_uc(resource_size_t offset, unsigned long > > > > size); > > > > #define ioremap_uc ioremap_uc > > > > +#define ioremap_nopost ioremap_nocache > > > > > > These are all the same as the default from asm-generic.h. Do we really > > > need these definitions in alpha, avr32, frv, ia64, x86? > > > > Those arches do not include asm-generic.h (I suppose for a good reason) > > but a definition is needed anyway if we want code using ioremap_nopost() > > to be unconditional. This is one way of doing it, there are others not > > sure they are any better, I am open to suggestions. > > We do have linux/io.h, which should be included in preference to asm/io.h. > linux/io.h has existed for years, but I still see (from time to time) > patches adding drivers that (imho incorrectly) use asm/io.h. > > Also, this: > > > > > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h > > > > index 7ef015e..0e81938 100644 > > > > --- a/include/asm-generic/io.h > > > > +++ b/include/asm-generic/io.h > > > > @@ -915,6 +915,10 @@ extern void ioport_unmap(void __iomem *p); > > > > #endif /* CONFIG_GENERIC_IOMAP */ > > > > #endif /* CONFIG_HAS_IOPORT_MAP */ > > > > > > > > +#ifndef ioremap_nopost > > > > +#define ioremap_nopost ioremap_nocache > > > > +#endif > > > > + > > > > #ifndef xlate_dev_kmem_ptr > > > > #define xlate_dev_kmem_ptr xlate_dev_kmem_ptr > > > > static inline void *xlate_dev_kmem_ptr(void *addr) > > could well be located in linux/io.h, which would make it available > everywhere. > > I'd suggest one change to this though: > > #ifndef ioremap_nopost > static inline void __iomem *ioremap_nopost(resource_size_t offset, > unsigned long size) > { > return ioremap_nocache(offset, size); > } > #endif > > This way, if someone forgets to define ioremap_nopost() in their > asm/io.h but provides a definition or extern prototype for > ioremap_nopost(), we end up with a compile error to highlight the > error, rather than the error being hidden by the preprocessor. Yes, I could add ioremap_nopost() to linux/io.h I did not do it because linux/io.h, for whatever reason, does not seem to contain ioremap_* prototypes; this does not mean we should not add it there but that would look odd (with all others ioremap_* in asm/io.h), all I am saying. This stuff requires some clean-up regardless, for the records. As for the static inline, I did that and that did not make the kbuild robot happy at all on some arches: eg: openrisc >> include/asm-generic/io.h:922:9: error: implicit declaration of >> function 'ioremap_nocache' [-Werror=implicit-function-declaration] return ioremap_nocache(offset, size); I will have another stab at it since what you put forward makes sense, I just have to find a way that works across arches. Thanks ! Lorenzo
Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
On Wed, Apr 05, 2017 at 11:58:41AM +0100, Russell King - ARM Linux wrote: > On Tue, Mar 28, 2017 at 03:45:43PM +0100, Lorenzo Pieralisi wrote: > > On Mon, Mar 27, 2017 at 08:41:10PM -0500, Bjorn Helgaas wrote: > > > On Mon, Mar 27, 2017 at 10:49:30AM +0100, Lorenzo Pieralisi wrote: > > > > diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h > > > > index 7afb0e2..50b292f 100644 > > > > --- a/arch/x86/include/asm/io.h > > > > +++ b/arch/x86/include/asm/io.h > > > > @@ -171,6 +171,7 @@ static inline unsigned int isa_virt_to_bus(volatile > > > > void *address) > > > > extern void __iomem *ioremap_nocache(resource_size_t offset, unsigned > > > > long size); > > > > extern void __iomem *ioremap_uc(resource_size_t offset, unsigned long > > > > size); > > > > #define ioremap_uc ioremap_uc > > > > +#define ioremap_nopost ioremap_nocache > > > > > > These are all the same as the default from asm-generic.h. Do we really > > > need these definitions in alpha, avr32, frv, ia64, x86? > > > > Those arches do not include asm-generic.h (I suppose for a good reason) > > but a definition is needed anyway if we want code using ioremap_nopost() > > to be unconditional. This is one way of doing it, there are others not > > sure they are any better, I am open to suggestions. > > We do have linux/io.h, which should be included in preference to asm/io.h. > linux/io.h has existed for years, but I still see (from time to time) > patches adding drivers that (imho incorrectly) use asm/io.h. > > Also, this: > > > > > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h > > > > index 7ef015e..0e81938 100644 > > > > --- a/include/asm-generic/io.h > > > > +++ b/include/asm-generic/io.h > > > > @@ -915,6 +915,10 @@ extern void ioport_unmap(void __iomem *p); > > > > #endif /* CONFIG_GENERIC_IOMAP */ > > > > #endif /* CONFIG_HAS_IOPORT_MAP */ > > > > > > > > +#ifndef ioremap_nopost > > > > +#define ioremap_nopost ioremap_nocache > > > > +#endif > > > > + > > > > #ifndef xlate_dev_kmem_ptr > > > > #define xlate_dev_kmem_ptr xlate_dev_kmem_ptr > > > > static inline void *xlate_dev_kmem_ptr(void *addr) > > could well be located in linux/io.h, which would make it available > everywhere. > > I'd suggest one change to this though: > > #ifndef ioremap_nopost > static inline void __iomem *ioremap_nopost(resource_size_t offset, > unsigned long size) > { > return ioremap_nocache(offset, size); > } > #endif > > This way, if someone forgets to define ioremap_nopost() in their > asm/io.h but provides a definition or extern prototype for > ioremap_nopost(), we end up with a compile error to highlight the > error, rather than the error being hidden by the preprocessor. Yes, I could add ioremap_nopost() to linux/io.h I did not do it because linux/io.h, for whatever reason, does not seem to contain ioremap_* prototypes; this does not mean we should not add it there but that would look odd (with all others ioremap_* in asm/io.h), all I am saying. This stuff requires some clean-up regardless, for the records. As for the static inline, I did that and that did not make the kbuild robot happy at all on some arches: eg: openrisc >> include/asm-generic/io.h:922:9: error: implicit declaration of >> function 'ioremap_nocache' [-Werror=implicit-function-declaration] return ioremap_nocache(offset, size); I will have another stab at it since what you put forward makes sense, I just have to find a way that works across arches. Thanks ! Lorenzo
Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
On Tue, Mar 28, 2017 at 03:45:43PM +0100, Lorenzo Pieralisi wrote: > On Mon, Mar 27, 2017 at 08:41:10PM -0500, Bjorn Helgaas wrote: > > On Mon, Mar 27, 2017 at 10:49:30AM +0100, Lorenzo Pieralisi wrote: > > > diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h > > > index 7afb0e2..50b292f 100644 > > > --- a/arch/x86/include/asm/io.h > > > +++ b/arch/x86/include/asm/io.h > > > @@ -171,6 +171,7 @@ static inline unsigned int isa_virt_to_bus(volatile > > > void *address) > > > extern void __iomem *ioremap_nocache(resource_size_t offset, unsigned > > > long size); > > > extern void __iomem *ioremap_uc(resource_size_t offset, unsigned long > > > size); > > > #define ioremap_uc ioremap_uc > > > +#define ioremap_nopost ioremap_nocache > > > > These are all the same as the default from asm-generic.h. Do we really > > need these definitions in alpha, avr32, frv, ia64, x86? > > Those arches do not include asm-generic.h (I suppose for a good reason) > but a definition is needed anyway if we want code using ioremap_nopost() > to be unconditional. This is one way of doing it, there are others not > sure they are any better, I am open to suggestions. We do have linux/io.h, which should be included in preference to asm/io.h. linux/io.h has existed for years, but I still see (from time to time) patches adding drivers that (imho incorrectly) use asm/io.h. Also, this: > > > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h > > > index 7ef015e..0e81938 100644 > > > --- a/include/asm-generic/io.h > > > +++ b/include/asm-generic/io.h > > > @@ -915,6 +915,10 @@ extern void ioport_unmap(void __iomem *p); > > > #endif /* CONFIG_GENERIC_IOMAP */ > > > #endif /* CONFIG_HAS_IOPORT_MAP */ > > > > > > +#ifndef ioremap_nopost > > > +#define ioremap_nopost ioremap_nocache > > > +#endif > > > + > > > #ifndef xlate_dev_kmem_ptr > > > #define xlate_dev_kmem_ptr xlate_dev_kmem_ptr > > > static inline void *xlate_dev_kmem_ptr(void *addr) could well be located in linux/io.h, which would make it available everywhere. I'd suggest one change to this though: #ifndef ioremap_nopost static inline void __iomem *ioremap_nopost(resource_size_t offset, unsigned long size) { return ioremap_nocache(offset, size); } #endif This way, if someone forgets to define ioremap_nopost() in their asm/io.h but provides a definition or extern prototype for ioremap_nopost(), we end up with a compile error to highlight the error, rather than the error being hidden by the preprocessor. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
On Tue, Mar 28, 2017 at 03:45:43PM +0100, Lorenzo Pieralisi wrote: > On Mon, Mar 27, 2017 at 08:41:10PM -0500, Bjorn Helgaas wrote: > > On Mon, Mar 27, 2017 at 10:49:30AM +0100, Lorenzo Pieralisi wrote: > > > diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h > > > index 7afb0e2..50b292f 100644 > > > --- a/arch/x86/include/asm/io.h > > > +++ b/arch/x86/include/asm/io.h > > > @@ -171,6 +171,7 @@ static inline unsigned int isa_virt_to_bus(volatile > > > void *address) > > > extern void __iomem *ioremap_nocache(resource_size_t offset, unsigned > > > long size); > > > extern void __iomem *ioremap_uc(resource_size_t offset, unsigned long > > > size); > > > #define ioremap_uc ioremap_uc > > > +#define ioremap_nopost ioremap_nocache > > > > These are all the same as the default from asm-generic.h. Do we really > > need these definitions in alpha, avr32, frv, ia64, x86? > > Those arches do not include asm-generic.h (I suppose for a good reason) > but a definition is needed anyway if we want code using ioremap_nopost() > to be unconditional. This is one way of doing it, there are others not > sure they are any better, I am open to suggestions. We do have linux/io.h, which should be included in preference to asm/io.h. linux/io.h has existed for years, but I still see (from time to time) patches adding drivers that (imho incorrectly) use asm/io.h. Also, this: > > > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h > > > index 7ef015e..0e81938 100644 > > > --- a/include/asm-generic/io.h > > > +++ b/include/asm-generic/io.h > > > @@ -915,6 +915,10 @@ extern void ioport_unmap(void __iomem *p); > > > #endif /* CONFIG_GENERIC_IOMAP */ > > > #endif /* CONFIG_HAS_IOPORT_MAP */ > > > > > > +#ifndef ioremap_nopost > > > +#define ioremap_nopost ioremap_nocache > > > +#endif > > > + > > > #ifndef xlate_dev_kmem_ptr > > > #define xlate_dev_kmem_ptr xlate_dev_kmem_ptr > > > static inline void *xlate_dev_kmem_ptr(void *addr) could well be located in linux/io.h, which would make it available everywhere. I'd suggest one change to this though: #ifndef ioremap_nopost static inline void __iomem *ioremap_nopost(resource_size_t offset, unsigned long size) { return ioremap_nocache(offset, size); } #endif This way, if someone forgets to define ioremap_nopost() in their asm/io.h but provides a definition or extern prototype for ioremap_nopost(), we end up with a compile error to highlight the error, rather than the error being hidden by the preprocessor. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
On Tue, Mar 28, 2017 at 03:45:43PM +0100, Lorenzo Pieralisi wrote: > On Mon, Mar 27, 2017 at 08:41:10PM -0500, Bjorn Helgaas wrote: > > On Mon, Mar 27, 2017 at 10:49:30AM +0100, Lorenzo Pieralisi wrote: > > > #define ioremap_uc ioremap_uc > > > +#define ioremap_nopost ioremap_nocache > > > > These are all the same as the default from asm-generic.h. Do we really > > need these definitions in alpha, avr32, frv, ia64, x86? > > Those arches do not include asm-generic.h (I suppose for a good reason) OK, that explains it. I'm not at all sure there's a good reason for those arches not using asm-generic.h, but I haven't looked at the code to see. > but a definition is needed anyway if we want code using ioremap_nopost() > to be unconditional. This is one way of doing it, there are others not > sure they are any better, I am open to suggestions.
Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
On Tue, Mar 28, 2017 at 03:45:43PM +0100, Lorenzo Pieralisi wrote: > On Mon, Mar 27, 2017 at 08:41:10PM -0500, Bjorn Helgaas wrote: > > On Mon, Mar 27, 2017 at 10:49:30AM +0100, Lorenzo Pieralisi wrote: > > > #define ioremap_uc ioremap_uc > > > +#define ioremap_nopost ioremap_nocache > > > > These are all the same as the default from asm-generic.h. Do we really > > need these definitions in alpha, avr32, frv, ia64, x86? > > Those arches do not include asm-generic.h (I suppose for a good reason) OK, that explains it. I'm not at all sure there's a good reason for those arches not using asm-generic.h, but I haven't looked at the code to see. > but a definition is needed anyway if we want code using ioremap_nopost() > to be unconditional. This is one way of doing it, there are others not > sure they are any better, I am open to suggestions.
Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
On Mon, Mar 27, 2017 at 08:41:10PM -0500, Bjorn Helgaas wrote: > On Mon, Mar 27, 2017 at 10:49:30AM +0100, Lorenzo Pieralisi wrote: > > The PCI specifications (Rev 3.0, 3.2.5 "Transaction Ordering and > > Posting") mandate non-posted configuration transactions. As further > > highlighted in the PCIe specifications (4.0 - Rev0.3, "Ordering > > Considerations for the Enhanced Configuration Access Mechanism"), > > through ECAM and ECAM-derivative configuration mechanism, the memory > > mapped transactions from the host CPU into Configuration Requests on the > > PCI express fabric may create ordering problems for software because > > writes to memory address are typically posted transactions (unless the > > architecture can enforce through virtual address mapping non-posted > > write transactions behaviour) but writes to Configuration Space are not > > posted on the PCI express fabric. > > > > Current DT and ACPI host bridge controllers map PCI configuration space > > (ECAM and ECAM-derivative) into the virtual address space through > > ioremap() calls, that are non-cacheable device accesses on most > > architectures, but may provide "bufferable" or "posted" write semantics > > in architecture like eg ARM/ARM64 that allow ioremap'ed regions writes > > to be buffered in the bus connecting the host CPU to the PCI fabric; > > this behaviour, as underlined in the PCIe specifications, may trigger > > transactions ordering rules and must be prevented. > > > > Introduce a new generic and explicit API to create a memory > > mapping for ECAM and ECAM-derivative config space area that > > defaults to ioremap_nocache() (which should provide a sane default > > behaviour) but still allowing architectures on which ioremap_nocache() > > results in posted write transactions to override the function > > call with an arch specific implementation that complies with > > the PCI specifications for configuration transactions. > > > > Signed-off-by: Lorenzo Pieralisi> > Cc: Arnd Bergmann > > Cc: Will Deacon > > Cc: Thomas Gleixner > > Cc: Bjorn Helgaas > > Cc: Richard Henderson > > Cc: Russell King > > Cc: Tony Luck > > Cc: Catalin Marinas > > Cc: Ingo Molnar > > Cc: Haavard Skinnemoen > > --- > > arch/alpha/include/asm/io.h | 1 + > > arch/avr32/include/asm/io.h | 1 + > > arch/frv/include/asm/io.h | 1 + > > arch/ia64/include/asm/io.h | 1 + > > arch/x86/include/asm/io.h | 1 + > > include/asm-generic/io.h| 4 > > 6 files changed, 9 insertions(+) > > > > diff --git a/arch/alpha/include/asm/io.h b/arch/alpha/include/asm/io.h > > index ff40491..27379ea 100644 > > --- a/arch/alpha/include/asm/io.h > > +++ b/arch/alpha/include/asm/io.h > > @@ -300,6 +300,7 @@ static inline void __iomem * ioremap_nocache(unsigned > > long offset, > > } > > > > #define ioremap_uc ioremap_nocache > > +#define ioremap_nopost ioremap_nocache > > > > static inline void iounmap(volatile void __iomem *addr) > > { > > diff --git a/arch/avr32/include/asm/io.h b/arch/avr32/include/asm/io.h > > index f855646..3f1ced8 100644 > > --- a/arch/avr32/include/asm/io.h > > +++ b/arch/avr32/include/asm/io.h > > @@ -298,6 +298,7 @@ extern void __iounmap(void __iomem *addr); > > #define ioremap_wc ioremap_nocache > > #define ioremap_wt ioremap_nocache > > #define ioremap_uc ioremap_nocache > > +#define ioremap_nopost ioremap_nocache > > > > #define cached(addr) P1SEGADDR(addr) > > #define uncached(addr) P2SEGADDR(addr) > > diff --git a/arch/frv/include/asm/io.h b/arch/frv/include/asm/io.h > > index 8062fc7..302fb8c 100644 > > --- a/arch/frv/include/asm/io.h > > +++ b/arch/frv/include/asm/io.h > > @@ -290,6 +290,7 @@ static inline void __iomem *ioremap_fullcache(unsigned > > long physaddr, unsigned l > > > > #define ioremap_wc ioremap_nocache > > #define ioremap_uc ioremap_nocache > > +#define ioremap_nopost ioremap_nocache > > > > extern void iounmap(void volatile __iomem *addr); > > > > diff --git a/arch/ia64/include/asm/io.h b/arch/ia64/include/asm/io.h > > index 5de673a..70a4985 100644 > > --- a/arch/ia64/include/asm/io.h > > +++ b/arch/ia64/include/asm/io.h > > @@ -434,6 +434,7 @@ static inline void __iomem * ioremap_cache (unsigned > > long phys_addr, unsigned lo > > } > > #define ioremap_cache ioremap_cache > > #define ioremap_uc ioremap_nocache > > +#define ioremap_nopost ioremap_nocache > > > > > > /* > > diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h > > index 7afb0e2..50b292f 100644 > > --- a/arch/x86/include/asm/io.h > > +++ b/arch/x86/include/asm/io.h > > @@ -171,6 +171,7 @@ static inline unsigned int isa_virt_to_bus(volatile > > void *address) > > extern void __iomem *ioremap_nocache(resource_size_t offset, unsigned long > > size); > > extern
Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
On Mon, Mar 27, 2017 at 08:41:10PM -0500, Bjorn Helgaas wrote: > On Mon, Mar 27, 2017 at 10:49:30AM +0100, Lorenzo Pieralisi wrote: > > The PCI specifications (Rev 3.0, 3.2.5 "Transaction Ordering and > > Posting") mandate non-posted configuration transactions. As further > > highlighted in the PCIe specifications (4.0 - Rev0.3, "Ordering > > Considerations for the Enhanced Configuration Access Mechanism"), > > through ECAM and ECAM-derivative configuration mechanism, the memory > > mapped transactions from the host CPU into Configuration Requests on the > > PCI express fabric may create ordering problems for software because > > writes to memory address are typically posted transactions (unless the > > architecture can enforce through virtual address mapping non-posted > > write transactions behaviour) but writes to Configuration Space are not > > posted on the PCI express fabric. > > > > Current DT and ACPI host bridge controllers map PCI configuration space > > (ECAM and ECAM-derivative) into the virtual address space through > > ioremap() calls, that are non-cacheable device accesses on most > > architectures, but may provide "bufferable" or "posted" write semantics > > in architecture like eg ARM/ARM64 that allow ioremap'ed regions writes > > to be buffered in the bus connecting the host CPU to the PCI fabric; > > this behaviour, as underlined in the PCIe specifications, may trigger > > transactions ordering rules and must be prevented. > > > > Introduce a new generic and explicit API to create a memory > > mapping for ECAM and ECAM-derivative config space area that > > defaults to ioremap_nocache() (which should provide a sane default > > behaviour) but still allowing architectures on which ioremap_nocache() > > results in posted write transactions to override the function > > call with an arch specific implementation that complies with > > the PCI specifications for configuration transactions. > > > > Signed-off-by: Lorenzo Pieralisi > > Cc: Arnd Bergmann > > Cc: Will Deacon > > Cc: Thomas Gleixner > > Cc: Bjorn Helgaas > > Cc: Richard Henderson > > Cc: Russell King > > Cc: Tony Luck > > Cc: Catalin Marinas > > Cc: Ingo Molnar > > Cc: Haavard Skinnemoen > > --- > > arch/alpha/include/asm/io.h | 1 + > > arch/avr32/include/asm/io.h | 1 + > > arch/frv/include/asm/io.h | 1 + > > arch/ia64/include/asm/io.h | 1 + > > arch/x86/include/asm/io.h | 1 + > > include/asm-generic/io.h| 4 > > 6 files changed, 9 insertions(+) > > > > diff --git a/arch/alpha/include/asm/io.h b/arch/alpha/include/asm/io.h > > index ff40491..27379ea 100644 > > --- a/arch/alpha/include/asm/io.h > > +++ b/arch/alpha/include/asm/io.h > > @@ -300,6 +300,7 @@ static inline void __iomem * ioremap_nocache(unsigned > > long offset, > > } > > > > #define ioremap_uc ioremap_nocache > > +#define ioremap_nopost ioremap_nocache > > > > static inline void iounmap(volatile void __iomem *addr) > > { > > diff --git a/arch/avr32/include/asm/io.h b/arch/avr32/include/asm/io.h > > index f855646..3f1ced8 100644 > > --- a/arch/avr32/include/asm/io.h > > +++ b/arch/avr32/include/asm/io.h > > @@ -298,6 +298,7 @@ extern void __iounmap(void __iomem *addr); > > #define ioremap_wc ioremap_nocache > > #define ioremap_wt ioremap_nocache > > #define ioremap_uc ioremap_nocache > > +#define ioremap_nopost ioremap_nocache > > > > #define cached(addr) P1SEGADDR(addr) > > #define uncached(addr) P2SEGADDR(addr) > > diff --git a/arch/frv/include/asm/io.h b/arch/frv/include/asm/io.h > > index 8062fc7..302fb8c 100644 > > --- a/arch/frv/include/asm/io.h > > +++ b/arch/frv/include/asm/io.h > > @@ -290,6 +290,7 @@ static inline void __iomem *ioremap_fullcache(unsigned > > long physaddr, unsigned l > > > > #define ioremap_wc ioremap_nocache > > #define ioremap_uc ioremap_nocache > > +#define ioremap_nopost ioremap_nocache > > > > extern void iounmap(void volatile __iomem *addr); > > > > diff --git a/arch/ia64/include/asm/io.h b/arch/ia64/include/asm/io.h > > index 5de673a..70a4985 100644 > > --- a/arch/ia64/include/asm/io.h > > +++ b/arch/ia64/include/asm/io.h > > @@ -434,6 +434,7 @@ static inline void __iomem * ioremap_cache (unsigned > > long phys_addr, unsigned lo > > } > > #define ioremap_cache ioremap_cache > > #define ioremap_uc ioremap_nocache > > +#define ioremap_nopost ioremap_nocache > > > > > > /* > > diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h > > index 7afb0e2..50b292f 100644 > > --- a/arch/x86/include/asm/io.h > > +++ b/arch/x86/include/asm/io.h > > @@ -171,6 +171,7 @@ static inline unsigned int isa_virt_to_bus(volatile > > void *address) > > extern void __iomem *ioremap_nocache(resource_size_t offset, unsigned long > > size); > > extern void __iomem *ioremap_uc(resource_size_t offset, unsigned long > > size); > > #define ioremap_uc ioremap_uc > > +#define ioremap_nopost ioremap_nocache > > These are all the same as the default from asm-generic.h. Do we really >
Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
On Mon, Mar 27, 2017 at 10:49:30AM +0100, Lorenzo Pieralisi wrote: > The PCI specifications (Rev 3.0, 3.2.5 "Transaction Ordering and > Posting") mandate non-posted configuration transactions. As further > highlighted in the PCIe specifications (4.0 - Rev0.3, "Ordering > Considerations for the Enhanced Configuration Access Mechanism"), > through ECAM and ECAM-derivative configuration mechanism, the memory > mapped transactions from the host CPU into Configuration Requests on the > PCI express fabric may create ordering problems for software because > writes to memory address are typically posted transactions (unless the > architecture can enforce through virtual address mapping non-posted > write transactions behaviour) but writes to Configuration Space are not > posted on the PCI express fabric. > > Current DT and ACPI host bridge controllers map PCI configuration space > (ECAM and ECAM-derivative) into the virtual address space through > ioremap() calls, that are non-cacheable device accesses on most > architectures, but may provide "bufferable" or "posted" write semantics > in architecture like eg ARM/ARM64 that allow ioremap'ed regions writes > to be buffered in the bus connecting the host CPU to the PCI fabric; > this behaviour, as underlined in the PCIe specifications, may trigger > transactions ordering rules and must be prevented. > > Introduce a new generic and explicit API to create a memory > mapping for ECAM and ECAM-derivative config space area that > defaults to ioremap_nocache() (which should provide a sane default > behaviour) but still allowing architectures on which ioremap_nocache() > results in posted write transactions to override the function > call with an arch specific implementation that complies with > the PCI specifications for configuration transactions. > > Signed-off-by: Lorenzo Pieralisi> Cc: Arnd Bergmann > Cc: Will Deacon > Cc: Thomas Gleixner > Cc: Bjorn Helgaas > Cc: Richard Henderson > Cc: Russell King > Cc: Tony Luck > Cc: Catalin Marinas > Cc: Ingo Molnar > Cc: Haavard Skinnemoen > --- > arch/alpha/include/asm/io.h | 1 + > arch/avr32/include/asm/io.h | 1 + > arch/frv/include/asm/io.h | 1 + > arch/ia64/include/asm/io.h | 1 + > arch/x86/include/asm/io.h | 1 + > include/asm-generic/io.h| 4 > 6 files changed, 9 insertions(+) > > diff --git a/arch/alpha/include/asm/io.h b/arch/alpha/include/asm/io.h > index ff40491..27379ea 100644 > --- a/arch/alpha/include/asm/io.h > +++ b/arch/alpha/include/asm/io.h > @@ -300,6 +300,7 @@ static inline void __iomem * ioremap_nocache(unsigned > long offset, > } > > #define ioremap_uc ioremap_nocache > +#define ioremap_nopost ioremap_nocache > > static inline void iounmap(volatile void __iomem *addr) > { > diff --git a/arch/avr32/include/asm/io.h b/arch/avr32/include/asm/io.h > index f855646..3f1ced8 100644 > --- a/arch/avr32/include/asm/io.h > +++ b/arch/avr32/include/asm/io.h > @@ -298,6 +298,7 @@ extern void __iounmap(void __iomem *addr); > #define ioremap_wc ioremap_nocache > #define ioremap_wt ioremap_nocache > #define ioremap_uc ioremap_nocache > +#define ioremap_nopost ioremap_nocache > > #define cached(addr) P1SEGADDR(addr) > #define uncached(addr) P2SEGADDR(addr) > diff --git a/arch/frv/include/asm/io.h b/arch/frv/include/asm/io.h > index 8062fc7..302fb8c 100644 > --- a/arch/frv/include/asm/io.h > +++ b/arch/frv/include/asm/io.h > @@ -290,6 +290,7 @@ static inline void __iomem *ioremap_fullcache(unsigned > long physaddr, unsigned l > > #define ioremap_wc ioremap_nocache > #define ioremap_uc ioremap_nocache > +#define ioremap_nopost ioremap_nocache > > extern void iounmap(void volatile __iomem *addr); > > diff --git a/arch/ia64/include/asm/io.h b/arch/ia64/include/asm/io.h > index 5de673a..70a4985 100644 > --- a/arch/ia64/include/asm/io.h > +++ b/arch/ia64/include/asm/io.h > @@ -434,6 +434,7 @@ static inline void __iomem * ioremap_cache (unsigned long > phys_addr, unsigned lo > } > #define ioremap_cache ioremap_cache > #define ioremap_uc ioremap_nocache > +#define ioremap_nopost ioremap_nocache > > > /* > diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h > index 7afb0e2..50b292f 100644 > --- a/arch/x86/include/asm/io.h > +++ b/arch/x86/include/asm/io.h > @@ -171,6 +171,7 @@ static inline unsigned int isa_virt_to_bus(volatile void > *address) > extern void __iomem *ioremap_nocache(resource_size_t offset, unsigned long > size); > extern void __iomem *ioremap_uc(resource_size_t offset, unsigned long size); > #define ioremap_uc ioremap_uc > +#define ioremap_nopost ioremap_nocache These are all the same as the default from asm-generic.h. Do we really need these definitions in alpha, avr32, frv, ia64, x86? > extern
Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
On Mon, Mar 27, 2017 at 10:49:30AM +0100, Lorenzo Pieralisi wrote: > The PCI specifications (Rev 3.0, 3.2.5 "Transaction Ordering and > Posting") mandate non-posted configuration transactions. As further > highlighted in the PCIe specifications (4.0 - Rev0.3, "Ordering > Considerations for the Enhanced Configuration Access Mechanism"), > through ECAM and ECAM-derivative configuration mechanism, the memory > mapped transactions from the host CPU into Configuration Requests on the > PCI express fabric may create ordering problems for software because > writes to memory address are typically posted transactions (unless the > architecture can enforce through virtual address mapping non-posted > write transactions behaviour) but writes to Configuration Space are not > posted on the PCI express fabric. > > Current DT and ACPI host bridge controllers map PCI configuration space > (ECAM and ECAM-derivative) into the virtual address space through > ioremap() calls, that are non-cacheable device accesses on most > architectures, but may provide "bufferable" or "posted" write semantics > in architecture like eg ARM/ARM64 that allow ioremap'ed regions writes > to be buffered in the bus connecting the host CPU to the PCI fabric; > this behaviour, as underlined in the PCIe specifications, may trigger > transactions ordering rules and must be prevented. > > Introduce a new generic and explicit API to create a memory > mapping for ECAM and ECAM-derivative config space area that > defaults to ioremap_nocache() (which should provide a sane default > behaviour) but still allowing architectures on which ioremap_nocache() > results in posted write transactions to override the function > call with an arch specific implementation that complies with > the PCI specifications for configuration transactions. > > Signed-off-by: Lorenzo Pieralisi > Cc: Arnd Bergmann > Cc: Will Deacon > Cc: Thomas Gleixner > Cc: Bjorn Helgaas > Cc: Richard Henderson > Cc: Russell King > Cc: Tony Luck > Cc: Catalin Marinas > Cc: Ingo Molnar > Cc: Haavard Skinnemoen > --- > arch/alpha/include/asm/io.h | 1 + > arch/avr32/include/asm/io.h | 1 + > arch/frv/include/asm/io.h | 1 + > arch/ia64/include/asm/io.h | 1 + > arch/x86/include/asm/io.h | 1 + > include/asm-generic/io.h| 4 > 6 files changed, 9 insertions(+) > > diff --git a/arch/alpha/include/asm/io.h b/arch/alpha/include/asm/io.h > index ff40491..27379ea 100644 > --- a/arch/alpha/include/asm/io.h > +++ b/arch/alpha/include/asm/io.h > @@ -300,6 +300,7 @@ static inline void __iomem * ioremap_nocache(unsigned > long offset, > } > > #define ioremap_uc ioremap_nocache > +#define ioremap_nopost ioremap_nocache > > static inline void iounmap(volatile void __iomem *addr) > { > diff --git a/arch/avr32/include/asm/io.h b/arch/avr32/include/asm/io.h > index f855646..3f1ced8 100644 > --- a/arch/avr32/include/asm/io.h > +++ b/arch/avr32/include/asm/io.h > @@ -298,6 +298,7 @@ extern void __iounmap(void __iomem *addr); > #define ioremap_wc ioremap_nocache > #define ioremap_wt ioremap_nocache > #define ioremap_uc ioremap_nocache > +#define ioremap_nopost ioremap_nocache > > #define cached(addr) P1SEGADDR(addr) > #define uncached(addr) P2SEGADDR(addr) > diff --git a/arch/frv/include/asm/io.h b/arch/frv/include/asm/io.h > index 8062fc7..302fb8c 100644 > --- a/arch/frv/include/asm/io.h > +++ b/arch/frv/include/asm/io.h > @@ -290,6 +290,7 @@ static inline void __iomem *ioremap_fullcache(unsigned > long physaddr, unsigned l > > #define ioremap_wc ioremap_nocache > #define ioremap_uc ioremap_nocache > +#define ioremap_nopost ioremap_nocache > > extern void iounmap(void volatile __iomem *addr); > > diff --git a/arch/ia64/include/asm/io.h b/arch/ia64/include/asm/io.h > index 5de673a..70a4985 100644 > --- a/arch/ia64/include/asm/io.h > +++ b/arch/ia64/include/asm/io.h > @@ -434,6 +434,7 @@ static inline void __iomem * ioremap_cache (unsigned long > phys_addr, unsigned lo > } > #define ioremap_cache ioremap_cache > #define ioremap_uc ioremap_nocache > +#define ioremap_nopost ioremap_nocache > > > /* > diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h > index 7afb0e2..50b292f 100644 > --- a/arch/x86/include/asm/io.h > +++ b/arch/x86/include/asm/io.h > @@ -171,6 +171,7 @@ static inline unsigned int isa_virt_to_bus(volatile void > *address) > extern void __iomem *ioremap_nocache(resource_size_t offset, unsigned long > size); > extern void __iomem *ioremap_uc(resource_size_t offset, unsigned long size); > #define ioremap_uc ioremap_uc > +#define ioremap_nopost ioremap_nocache These are all the same as the default from asm-generic.h. Do we really need these definitions in alpha, avr32, frv, ia64, x86? > extern void __iomem *ioremap_cache(resource_size_t offset, unsigned long > size); > extern void __iomem *ioremap_prot(resource_size_t offset, unsigned long > size, unsigned long prot_val); > diff --git a/include/asm-generic/io.h
[PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
The PCI specifications (Rev 3.0, 3.2.5 "Transaction Ordering and Posting") mandate non-posted configuration transactions. As further highlighted in the PCIe specifications (4.0 - Rev0.3, "Ordering Considerations for the Enhanced Configuration Access Mechanism"), through ECAM and ECAM-derivative configuration mechanism, the memory mapped transactions from the host CPU into Configuration Requests on the PCI express fabric may create ordering problems for software because writes to memory address are typically posted transactions (unless the architecture can enforce through virtual address mapping non-posted write transactions behaviour) but writes to Configuration Space are not posted on the PCI express fabric. Current DT and ACPI host bridge controllers map PCI configuration space (ECAM and ECAM-derivative) into the virtual address space through ioremap() calls, that are non-cacheable device accesses on most architectures, but may provide "bufferable" or "posted" write semantics in architecture like eg ARM/ARM64 that allow ioremap'ed regions writes to be buffered in the bus connecting the host CPU to the PCI fabric; this behaviour, as underlined in the PCIe specifications, may trigger transactions ordering rules and must be prevented. Introduce a new generic and explicit API to create a memory mapping for ECAM and ECAM-derivative config space area that defaults to ioremap_nocache() (which should provide a sane default behaviour) but still allowing architectures on which ioremap_nocache() results in posted write transactions to override the function call with an arch specific implementation that complies with the PCI specifications for configuration transactions. Signed-off-by: Lorenzo PieralisiCc: Arnd Bergmann Cc: Will Deacon Cc: Thomas Gleixner Cc: Bjorn Helgaas Cc: Richard Henderson Cc: Russell King Cc: Tony Luck Cc: Catalin Marinas Cc: Ingo Molnar Cc: Haavard Skinnemoen --- arch/alpha/include/asm/io.h | 1 + arch/avr32/include/asm/io.h | 1 + arch/frv/include/asm/io.h | 1 + arch/ia64/include/asm/io.h | 1 + arch/x86/include/asm/io.h | 1 + include/asm-generic/io.h| 4 6 files changed, 9 insertions(+) diff --git a/arch/alpha/include/asm/io.h b/arch/alpha/include/asm/io.h index ff40491..27379ea 100644 --- a/arch/alpha/include/asm/io.h +++ b/arch/alpha/include/asm/io.h @@ -300,6 +300,7 @@ static inline void __iomem * ioremap_nocache(unsigned long offset, } #define ioremap_uc ioremap_nocache +#define ioremap_nopost ioremap_nocache static inline void iounmap(volatile void __iomem *addr) { diff --git a/arch/avr32/include/asm/io.h b/arch/avr32/include/asm/io.h index f855646..3f1ced8 100644 --- a/arch/avr32/include/asm/io.h +++ b/arch/avr32/include/asm/io.h @@ -298,6 +298,7 @@ extern void __iounmap(void __iomem *addr); #define ioremap_wc ioremap_nocache #define ioremap_wt ioremap_nocache #define ioremap_uc ioremap_nocache +#define ioremap_nopost ioremap_nocache #define cached(addr) P1SEGADDR(addr) #define uncached(addr) P2SEGADDR(addr) diff --git a/arch/frv/include/asm/io.h b/arch/frv/include/asm/io.h index 8062fc7..302fb8c 100644 --- a/arch/frv/include/asm/io.h +++ b/arch/frv/include/asm/io.h @@ -290,6 +290,7 @@ static inline void __iomem *ioremap_fullcache(unsigned long physaddr, unsigned l #define ioremap_wc ioremap_nocache #define ioremap_uc ioremap_nocache +#define ioremap_nopost ioremap_nocache extern void iounmap(void volatile __iomem *addr); diff --git a/arch/ia64/include/asm/io.h b/arch/ia64/include/asm/io.h index 5de673a..70a4985 100644 --- a/arch/ia64/include/asm/io.h +++ b/arch/ia64/include/asm/io.h @@ -434,6 +434,7 @@ static inline void __iomem * ioremap_cache (unsigned long phys_addr, unsigned lo } #define ioremap_cache ioremap_cache #define ioremap_uc ioremap_nocache +#define ioremap_nopost ioremap_nocache /* diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h index 7afb0e2..50b292f 100644 --- a/arch/x86/include/asm/io.h +++ b/arch/x86/include/asm/io.h @@ -171,6 +171,7 @@ static inline unsigned int isa_virt_to_bus(volatile void *address) extern void __iomem *ioremap_nocache(resource_size_t offset, unsigned long size); extern void __iomem *ioremap_uc(resource_size_t offset, unsigned long size); #define ioremap_uc ioremap_uc +#define ioremap_nopost ioremap_nocache extern void __iomem *ioremap_cache(resource_size_t offset, unsigned long size); extern void __iomem *ioremap_prot(resource_size_t offset, unsigned long size, unsigned long prot_val); diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h index 7ef015e..0e81938 100644 --- a/include/asm-generic/io.h +++ b/include/asm-generic/io.h @@ -915,6 +915,10 @@ extern void ioport_unmap(void __iomem *p); #endif /*
[PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface
The PCI specifications (Rev 3.0, 3.2.5 "Transaction Ordering and Posting") mandate non-posted configuration transactions. As further highlighted in the PCIe specifications (4.0 - Rev0.3, "Ordering Considerations for the Enhanced Configuration Access Mechanism"), through ECAM and ECAM-derivative configuration mechanism, the memory mapped transactions from the host CPU into Configuration Requests on the PCI express fabric may create ordering problems for software because writes to memory address are typically posted transactions (unless the architecture can enforce through virtual address mapping non-posted write transactions behaviour) but writes to Configuration Space are not posted on the PCI express fabric. Current DT and ACPI host bridge controllers map PCI configuration space (ECAM and ECAM-derivative) into the virtual address space through ioremap() calls, that are non-cacheable device accesses on most architectures, but may provide "bufferable" or "posted" write semantics in architecture like eg ARM/ARM64 that allow ioremap'ed regions writes to be buffered in the bus connecting the host CPU to the PCI fabric; this behaviour, as underlined in the PCIe specifications, may trigger transactions ordering rules and must be prevented. Introduce a new generic and explicit API to create a memory mapping for ECAM and ECAM-derivative config space area that defaults to ioremap_nocache() (which should provide a sane default behaviour) but still allowing architectures on which ioremap_nocache() results in posted write transactions to override the function call with an arch specific implementation that complies with the PCI specifications for configuration transactions. Signed-off-by: Lorenzo Pieralisi Cc: Arnd Bergmann Cc: Will Deacon Cc: Thomas Gleixner Cc: Bjorn Helgaas Cc: Richard Henderson Cc: Russell King Cc: Tony Luck Cc: Catalin Marinas Cc: Ingo Molnar Cc: Haavard Skinnemoen --- arch/alpha/include/asm/io.h | 1 + arch/avr32/include/asm/io.h | 1 + arch/frv/include/asm/io.h | 1 + arch/ia64/include/asm/io.h | 1 + arch/x86/include/asm/io.h | 1 + include/asm-generic/io.h| 4 6 files changed, 9 insertions(+) diff --git a/arch/alpha/include/asm/io.h b/arch/alpha/include/asm/io.h index ff40491..27379ea 100644 --- a/arch/alpha/include/asm/io.h +++ b/arch/alpha/include/asm/io.h @@ -300,6 +300,7 @@ static inline void __iomem * ioremap_nocache(unsigned long offset, } #define ioremap_uc ioremap_nocache +#define ioremap_nopost ioremap_nocache static inline void iounmap(volatile void __iomem *addr) { diff --git a/arch/avr32/include/asm/io.h b/arch/avr32/include/asm/io.h index f855646..3f1ced8 100644 --- a/arch/avr32/include/asm/io.h +++ b/arch/avr32/include/asm/io.h @@ -298,6 +298,7 @@ extern void __iounmap(void __iomem *addr); #define ioremap_wc ioremap_nocache #define ioremap_wt ioremap_nocache #define ioremap_uc ioremap_nocache +#define ioremap_nopost ioremap_nocache #define cached(addr) P1SEGADDR(addr) #define uncached(addr) P2SEGADDR(addr) diff --git a/arch/frv/include/asm/io.h b/arch/frv/include/asm/io.h index 8062fc7..302fb8c 100644 --- a/arch/frv/include/asm/io.h +++ b/arch/frv/include/asm/io.h @@ -290,6 +290,7 @@ static inline void __iomem *ioremap_fullcache(unsigned long physaddr, unsigned l #define ioremap_wc ioremap_nocache #define ioremap_uc ioremap_nocache +#define ioremap_nopost ioremap_nocache extern void iounmap(void volatile __iomem *addr); diff --git a/arch/ia64/include/asm/io.h b/arch/ia64/include/asm/io.h index 5de673a..70a4985 100644 --- a/arch/ia64/include/asm/io.h +++ b/arch/ia64/include/asm/io.h @@ -434,6 +434,7 @@ static inline void __iomem * ioremap_cache (unsigned long phys_addr, unsigned lo } #define ioremap_cache ioremap_cache #define ioremap_uc ioremap_nocache +#define ioremap_nopost ioremap_nocache /* diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h index 7afb0e2..50b292f 100644 --- a/arch/x86/include/asm/io.h +++ b/arch/x86/include/asm/io.h @@ -171,6 +171,7 @@ static inline unsigned int isa_virt_to_bus(volatile void *address) extern void __iomem *ioremap_nocache(resource_size_t offset, unsigned long size); extern void __iomem *ioremap_uc(resource_size_t offset, unsigned long size); #define ioremap_uc ioremap_uc +#define ioremap_nopost ioremap_nocache extern void __iomem *ioremap_cache(resource_size_t offset, unsigned long size); extern void __iomem *ioremap_prot(resource_size_t offset, unsigned long size, unsigned long prot_val); diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h index 7ef015e..0e81938 100644 --- a/include/asm-generic/io.h +++ b/include/asm-generic/io.h @@ -915,6 +915,10 @@ extern void ioport_unmap(void __iomem *p); #endif /* CONFIG_GENERIC_IOMAP */ #endif /* CONFIG_HAS_IOPORT_MAP */ +#ifndef ioremap_nopost +#define ioremap_nopost ioremap_nocache +#endif + #ifndef xlate_dev_kmem_ptr #define xlate_dev_kmem_ptr xlate_dev_kmem_ptr static inline void