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.
