Date:	Tue, 7 Jul 2015 18:07:03 +0200
From:	"Luis R. Rodriguez" <mcgrof@...e.com>
To:	Russell King - ARM Linux <linux@....linux.org.uk>
Cc:	Christoph Hellwig <hch@....de>,
	Andy Lutomirski <luto@...capital.net>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	Geert Uytterhoeven <geert@...ux-m68k.org>,
	Julia Lawall <julia.lawall@...6.fr>,
	Dan Williams <dan.j.williams@...el.com>,
	Arnd Bergmann <arnd@...db.de>, Ingo Molnar <mingo@...hat.com>,
	Borislav Petkov <bp@...en8.de>,
	"H. Peter Anvin" <hpa@...or.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ross Zwisler <ross.zwisler@...ux.intel.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Juergen Gross <jgross@...e.com>, X86 ML <x86@...nel.org>,
	"Kani, Toshimitsu" <toshi.kani@...com>,
	"linux-nvdimm@...ts.01.org" <linux-nvdimm@...1.01.org>,
	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Stefan Bader <stefan.bader@...onical.com>,
	Linux MM <linux-mm@...ck.org>,
	Ralf Baechle <ralf@...ux-mips.org>,
	Henrique de Moraes Holschuh <hmh@....eng.br>,
	Michael Ellerman <mpe@...erman.id.au>,
	Tejun Heo <tj@...nel.org>, Paul Mackerras <paulus@...ba.org>,
	mcgrof@...not-panic.com,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v5 2/6] arch: unify ioremap prototypes and macro aliases

On Tue, Jul 07, 2015 at 11:13:30AM +0100, Russell King - ARM Linux wrote:
> On Tue, Jul 07, 2015 at 11:50:12AM +0200, Luis R. Rodriguez wrote:
> > mcgrof@...on ~/linux-next (git::kill-mtrr)$ git grep ioremap_nocache drivers/| wc -l
> > 359
> 
> Yes, it's because we have:
> (a) LDD telling people they should be using ioremap_nocache() for mapping
>     devices.

Sounds like LDD could use some love from ARM folks. Not a requirement if we
set out on this semantics / grammar / documentation crusade appropriatley.

> (b) We have documentation in the Documentation/ subdirectory telling people
>     to use ioremap_nocache() for the same.

That obviously needs to be fixed, I take it you're on it.

> > This is part of the work I've been doing lately. The
> > eventual goal once we have the write-combing areas properly split with
> > ioremap_wc() and using the new proper preferred architecture agnostic modifier
> > (arch_phys_wc_add()) is to change the default ioremap behaviour on x86 to use
> > strong UC for PAT enabled systems for *both* ioremap() and ioremap_nocache().
> 
> Please note that on ARM, ioremap_wc() gives what's termed in ARM ARM
> speak "normal memory, non-cacheable" - which can be subject to speculation,
> write combining, multiple accesses, etc.  The important point is that
> such mapping is not suitable for device registers, but is suitable for
> device regions that have "memory like" properties (iow, a chunk of RAM,
> like video drivers.)  It does support unaligned accesses.

Thanks that helps.

> > Because of these grammatical issues and the issues with
> > unaligned access with ARM I think its important we put some effort
> > to care a bit more about defining clear semantics through grammar
> > for new APIs or as we rewrite APIs. We have tools to do this these
> > days, best make use of them.
> 
> I'm in support of anything which more clearly specifies the requirements
> for these APIs.

Great!

> > While we're at it and reconsidering all this, a few items I wish for
> > us to address as well then, most of them related to grammar, some
> > procedural clarification:
> > 
> >   * Document it as not supported to have overlapping ioremap() calls.
> >     No one seems to have a clue if this should work, but clearly this
> >     is just a bad idea. I don't see why we should support the complexity
> >     of having this. It seems we can write grammar rules to prevent this.
> 
> On ARM, we (probably) have a lot of cases where ioremap() is used multiple
> times for the same physical address space, so we shouldn't rule out having
> multiple mappings of the same type.

Why is that done? Don't worry if you are not sure why but only speculate of the
practice's existence (sloppy drivers or lazy driver developers). FWIW for x86
IIRC I ended up concluding that overlapping ioremap() calls with the same type
would work but not if they differ in type.  Although I haven't written a
grammer rule to hunt down overlapping ioremap() I suspected its use was likely
odd and likely should be reconsidered. Would this be true for ARM too ? Or are
you saying this should be a feature ? I don't expect an answer now but I'm
saying we *should* all together decide on this, and if you're inclined to
believe that this should ideally be avoided I'd like to hear that. If you feel
strongly though this should be a feature I would like to know why.

> However, differing types would be a problem on ARM.

Great.

> >   * We seem to care about device drivers / kernel code doing unaligned
> >     accesses with certain ioremap() variants. At least for ARM you should
> >     not do unaligned accesses on ioremap_nocache() areas.
> 
> ... and ioremap() areas.
> 
> If we can stop the "abuse" of ioremap_nocache() to map device registers,

OK when *should* ioremap_nocache() be used then ? That is, we can easily write
a rule to go and switch drivers away from ioremap_nocache() but do we have a
grammatical white-list for when its intended goal was appropriate ?  For x86
the goal is to use it for MMIO registers, keep in mind we'd ideally want an
effective ioremap_uc() on those long term, we just can't go flipping the switch
just yet unless we get all the write-combined areas right first and smake sure
that we split them out. That's the effort I've been working on lately.

> then we could potentially switch ioremap_nocache() to be a normal-memory
> like mapping, which would allow it to support unaligned accesses.

Great. Can you elaborate on why that could happen *iff* the abuse stops ?

> >     I am not sure
> >     if we can come up with grammar to vet for / warn for unaligned access
> >     type of code in driver code on some memory area when some ioremap()
> >     variant is used, but this could be looked into. I believe we may
> >     want rules for unaligned access maybe in general, and not attached
> >     to certain calls due to performance considerations, so this work
> >     may be welcomed regardless (refer to
> >     Documentation/unaligned-memory-access.txt)
> >     
> >   * We seem to want to be pedantic about adding new ioremap() variants, the
> >     unaligned issue on ARM is one reason, do we ideally then want *all*
> >     architecture maintainers to provide an Acked-by for any new ioremap
> >     variants ?
> 
> /If/ we get the current mess sorted out so that we have a safe fallback,
> and we have understanding of the different architecture variants (iow,
> documented what the safe fallback is) I don't see any reason why we'd
> need acks from arch maintainers. 

Great, that's the scalable solution so we should strive for this then as
it seems attainable.

> Unfortunately, we're not in that
> situation today, because of the poorly documented mess that ioremap*()
> currently is (and yes, I'm partly to blame for that too by not documenting
> ARMs behaviour here.)

So you're saying the mess is yours to begin with :) ?

> I have some patches (prepared last week, I was going to push them out
> towards the end of the merge window) which address that, but unfortunately
> the ARM autobuilders have been giving a number of seemingly random boot
> failures, and I'm not yet sure what's going on... so I'm holding that
> back until stuff has settled down.

OK sounds like sorting that out will take some time. What are we to do
in the meantime ? Would a default safe return -EOPNOTSUPP be a good
deafult for variants until we get the semantics all settled out ?

> Another issue is... the use of memcpy()/memset() directly on memory
> returned from ioremap*().  The pmem driver does this.  This fails sparse
> checks.  However, years ago, x86 invented the memcpy_fromio()/memcpy_toio()
> memset_io() functions, which took a __iomem pointer (which /presumably/
> means they're supposed to operate on the memory associated with an
> ioremap'd region.)
> 
> Should these functions always be used for mappings via ioremap*(), and
> the standard memcpy()/memset() be avoided?  To me, that sounds like a
> very good thing, because that gives us more control over the
> implementation of the functions used to access ioremap'd regions,
> and the arch can decide to prevent GCC inlining its own memset() or
> memcpy() code if desired.

I think Ben might like this for PowerPC as well, although the atomic
read / write thing would also require revising. I'm pretty confident
we can use grammar to go and fix these offenders if we deem this as
desirable.

Lastly, a small point I'd like to make is that in my ioremap_wc() crusade to
vet for things I found that the drivers that I had the biggest amount of issue
with were ancient and decrepit, perhaps now staging material drivers. Of 4
drivers that I had issues with two had code commented out (now removed, the
fusion driver), one driver was deprecated and we now reached the decision to
remove it from Linux (outbound via staging for ~2 releases as an accepted
driver removal policy, as Greg clarified) this is the ipath driver, another had
some oddball firmware which didn't even allow to expose the write-combined
address offset and was used for some old DVR setup maybe only some folks in
India are using, and lastly atyfb for which we ended up adding ioremap_uc() for
to help work around some MTRR corner case use cases to match a suitable
replacement with PAT.

This is a long winded way of saying that old crappy drivers had an impact on
setting the pace for sane semantics and collateral evolutions (in the form of
accepted required grammatical changes, to use ioremap_uc() with
arch_phys_wc_add()) to clean things up, and I think that if we want to grow
faster and leaner we must grow with grammar but that also should likely mean
more readily accepting removal of ancient crappy drivers. If folks agree
one way to make this explicit is for example to send to staging drivers
which are enabled for all architectures which do tons of unaligned accesses,
and for staging drivers to be only enabled via Kconfig for architectures
which are known to work.

If we want clear semantics / grammar rules / checks defined for a set of
ioremap() calls and family of calls having a clean slate of drivers should make
defining semantics, grammer rules and enforcing them much easier, just as
then maing collateral evolutions to then go out and fix only a set of drivers.
Moore's law should have implications not only on accepted growth but also
on deprecation, if we haven't been deprecating hardware fast enough and
not giving old drivers major love it should only affect us, specially as
our hardware evolves even faster.

So if we want to be pedantic about semantics / grammar lets flush down the
toilet more drivers and faster and if we're iffy about them we can put them on
the staging corner.

> Note that on x86, these three functions are merely wrappers around
> standard memcpy()/memset(), so there should be no reason why pmem.c
> couldn't be updated to use these accessors instead.

Great.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists