Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap interface

Russell King - ARM Linux
2017-04-06T13:07:55Z

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.