[PATCH v5 2/6] arch: unify ioremap prototypes and macro aliases

[PATCH v5 2/6] arch: unify ioremap prototypes and macro aliases

[Dan Williams]

On Mon, Jun 22, 2015 at 9:10 AM, Christoph Hellwig <hch@lst.de> wrote:
> On Mon, Jun 22, 2015 at 04:24:27AM -0400, Dan Williams wrote:
>> Some archs define the first parameter to ioremap() as unsigned long,
>> while the balance define it as resource_size_t.  Unify on
>> resource_size_t to enable passing ioremap function pointers.  Also, some
>> archs use function-like macros for defining ioremap aliases, but
>> asm-generic/io.h expects object-like macros, unify on the latter.
>>
>> Move all handling of ioremap aliasing (i.e. ioremap_wt => ioremap) to
>> include/linux/io.h.  Add a check to include/linux/io.h to warn at
>> compile time if an arch violates expectations.
>>
>> Kill ARCH_HAS_IOREMAP_WC and ARCH_HAS_IOREMAP_WT in favor of just
>> testing for ioremap_wc, and ioremap_wt being defined.  This arrangement
>> allows drivers to know when ioremap_<foo> are being re-directed to plain
>> ioremap.
>>
>> Reported-by: kbuild test robot <fengguang.wu@intel.com>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>
> Hmm, this is quite a bit of churn, and doesn't make the interface lot
> more obvious.
>
> I guess it's enough to get the pmem related bits going, but I'd really
> prefer defining the ioremap* prototype in linux/io.h and requiring
> and out of line implementation in the architectures, it's not like
> it's a fast path.  And to avoid the ifdef mess make it something like:
>
> void __iomem *ioremap_flags(resource_size_t offset, unsigned long size,
>                         unsigned long prot_val, unsigned flags);

Doesn't 'flags' imply a specific 'prot_val'?

> static inline void __iomem *ioremap(resource_size_t offset, unsigned long size)
> {
>         return ioremap_flags(offset, size, 0, 0);
> }
>
> static inline void __iomem *ioremap_prot(resource_size_t offset,
>                 unsigned long size, unsigned long prot_val)
> {
>         return ioremap_flags(offset, size, prot_val, 0);
> }
>
> static inline void __iomem *ioremap_nocache(resource_size_t offset,
>                 unsigned long size)
> {
>         return ioremap_flags(offset, size, 0, IOREMAP_NOCACHE);
> }
>
> static inline void __iomem *ioremap_cache(resource_size_t offset,
>                 unsigned long size)
> {
>         return ioremap_flags(offset, size, 0, IOREMAP_CACHE);
> }
>
> static inline void __iomem *ioremap_uc(resource_size_t offset,
>                 unsigned long size)
> {
>         return ioremap_flags(offset, size, 0, IOREMAP_UC);
> }
>
> static inline void __iomem *ioremap_wc(resource_size_t offset,
>                 unsigned long size)
> {
>         return ioremap_flags(offset, size, 0, IOREMAP_WC);
> }
>
> static inline void __iomem *ioremap_wt(resource_size_t offset,
>                 unsigned long size)
> {
>         return ioremap_flags(offset, size, 0, IOREMAP_WT);
> }
>
> With all wrappers but ioremap() itself deprecated in the long run.
>
> Besides following the one API one prototype guideline this gives
> us one proper entry point for all the variants.  Additionally
> it can reject non-supported caching modes at run time, e.g. because
> different hardware may or may not support it.  Additionally it
> avoids the need for all these HAVE_IOREMAP_FOO defines, which need
> constant updating.

One useful feature of the ifdef mess as implemented in the patch is
that you could test for whether ioremap_cache() is actually
implemented or falls back to default ioremap().  I think for
completeness archs should publish an ioremap type capabilities mask
for drivers that care... (I can imagine pmem caring), or default to
being permissive if something like IOREMAP_STRICT is not set.  There's
also the wrinkle of archs that can only support certain types of
mappings at a given alignment.

[PATCH v5 2/6] arch: unify ioremap prototypes and macro aliases

[Christoph Hellwig]

On Tue, Jun 30, 2015 at 03:57:16PM -0700, Dan Williams wrote:
> > void __iomem *ioremap_flags(resource_size_t offset, unsigned long size,
> >                         unsigned long prot_val, unsigned flags);
> 
> Doesn't 'flags' imply a specific 'prot_val'?

Looks like the values are arch specific.  So as a first step I'd like
to keep them separate.  As a second step we could look into unifying
the actual ioremap implementations which look mostly the same.  Once
that is done we could look into collapsing the flags and prot_val
arguments.

> One useful feature of the ifdef mess as implemented in the patch is
> that you could test for whether ioremap_cache() is actually
> implemented or falls back to default ioremap().  I think for
> completeness archs should publish an ioremap type capabilities mask
> for drivers that care... (I can imagine pmem caring), or default to
> being permissive if something like IOREMAP_STRICT is not set.  There's
> also the wrinkle of archs that can only support certain types of
> mappings at a given alignment.

I think doing this at runtime might be a better idea.  E.g. a
ioremap_flags with the CACHED argument will return -EOPNOTSUP unless
actually implemented.  On various architectures different CPUs or
boards will have different capabilities in this area.

[PATCH v5 2/6] arch: unify ioremap prototypes and macro aliases

[Geert Uytterhoeven]

On Wed, Jul 1, 2015 at 8:23 AM, Christoph Hellwig <hch@lst.de> wrote:
>> One useful feature of the ifdef mess as implemented in the patch is
>> that you could test for whether ioremap_cache() is actually
>> implemented or falls back to default ioremap().  I think for
>> completeness archs should publish an ioremap type capabilities mask
>> for drivers that care... (I can imagine pmem caring), or default to
>> being permissive if something like IOREMAP_STRICT is not set.  There's
>> also the wrinkle of archs that can only support certain types of
>> mappings at a given alignment.
>
> I think doing this at runtime might be a better idea.  E.g. a
> ioremap_flags with the CACHED argument will return -EOPNOTSUP unless
> actually implemented.  On various architectures different CPUs or
> boards will have different capabilities in this area.

So it would be the responsibility of the caller to fall back from
ioremap(..., CACHED) to ioremap(..., UNCACHED)?
I.e. all drivers using it should be changed...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v5 2/6] arch: unify ioremap prototypes and macro aliases

[Christoph Hellwig]

On Wed, Jul 01, 2015 at 08:55:57AM +0200, Geert Uytterhoeven wrote:
> >
> > I think doing this at runtime might be a better idea.  E.g. a
> > ioremap_flags with the CACHED argument will return -EOPNOTSUP unless
> > actually implemented.  On various architectures different CPUs or
> > boards will have different capabilities in this area.
> 
> So it would be the responsibility of the caller to fall back from
> ioremap(..., CACHED) to ioremap(..., UNCACHED)?
> I.e. all drivers using it should be changed...

All of the zero users we currently have will need to be changed, yes.

Note that I propose to leave ioremap(), aka ioremap_flags(..., 0) as
a default that always has to work, -EOPNOTSUP is only a valid return
value for non-default flaga.

[PATCH v5 2/6] arch: unify ioremap prototypes and macro aliases

[Geert Uytterhoeven]

On Wed, Jul 1, 2015 at 8:59 AM, Christoph Hellwig <hch@lst.de> wrote:
> On Wed, Jul 01, 2015 at 08:55:57AM +0200, Geert Uytterhoeven wrote:
>> >
>> > I think doing this at runtime might be a better idea.  E.g. a
>> > ioremap_flags with the CACHED argument will return -EOPNOTSUP unless
>> > actually implemented.  On various architectures different CPUs or
>> > boards will have different capabilities in this area.
>>
>> So it would be the responsibility of the caller to fall back from
>> ioremap(..., CACHED) to ioremap(..., UNCACHED)?
>> I.e. all drivers using it should be changed...
>
> All of the zero users we currently have will need to be changed, yes.

Good. Less work to convert all of these ;-)

> Note that I propose to leave ioremap(), aka ioremap_flags(..., 0) as
> a default that always has to work, -EOPNOTSUP is only a valid return
> value for non-default flaga.

OK.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v5 2/6] arch: unify ioremap prototypes and macro aliases

[Christoph Hellwig]

On Wed, Jul 01, 2015 at 09:19:29AM +0200, Geert Uytterhoeven wrote:
> >> So it would be the responsibility of the caller to fall back from
> >> ioremap(..., CACHED) to ioremap(..., UNCACHED)?
> >> I.e. all drivers using it should be changed...
> >
> > All of the zero users we currently have will need to be changed, yes.
> 
> Good. Less work to convert all of these ;-)

And I didn't have enough coffee yet.  We of course have a few users of
ioremap_cache(), and two implememantions but no users of ioremap_cached().
Looks like the implementations can't even agree on the name.

[PATCH v5 2/6] arch: unify ioremap prototypes and macro aliases

[Russell King - ARM Linux]

On Wed, Jul 01, 2015 at 08:55:57AM +0200, Geert Uytterhoeven wrote:
> On Wed, Jul 1, 2015 at 8:23 AM, Christoph Hellwig <hch@lst.de> wrote:
> >> One useful feature of the ifdef mess as implemented in the patch is
> >> that you could test for whether ioremap_cache() is actually
> >> implemented or falls back to default ioremap().  I think for
> >> completeness archs should publish an ioremap type capabilities mask
> >> for drivers that care... (I can imagine pmem caring), or default to
> >> being permissive if something like IOREMAP_STRICT is not set.  There's
> >> also the wrinkle of archs that can only support certain types of
> >> mappings at a given alignment.
> >
> > I think doing this at runtime might be a better idea.  E.g. a
> > ioremap_flags with the CACHED argument will return -EOPNOTSUP unless
> > actually implemented.  On various architectures different CPUs or
> > boards will have different capabilities in this area.
> 
> So it would be the responsibility of the caller to fall back from
> ioremap(..., CACHED) to ioremap(..., UNCACHED)?
> I.e. all drivers using it should be changed...

Another important point here is to define what the properties of the
mappings are.  It's no good just saying "uncached".

We've recently been around this over the PMEM driver and the broken
addition of ioremap_wt() on ARM...

By "properties" I mean stuff like whether unaligned accesses permitted,
any kind of atomic access (eg, xchg, cmpxchg, etc).

This matters: on ARM, a mapping suitable for a device does not support
unaligned accesses or atomic accesses - only "memory-like" mappings
support those.  However, memory-like mappings are not required to
preserve access size, number of accesses, etc which makes them unsuitable
for device registers.

The problem with ioremap_uncached() in particular is that we have LDD
and other documentation telling people to use it to map device registers,
so we can't define ioremap_uncached() on ARM to have memory-like
properties, and it doesn't support unaligned accesses.

I have a series of patches which fix up 32-bit ARM for the broken
ioremap_wt() stuff that was merged during this merge window, which I
intend to push out into linux-next at some point (possibly during the
merge window, if not after -rc1) which also move ioremap*() out of line
on ARM but more importantly, adds a load of documentation about the
properties of the resulting mapping on ARM.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH v5 2/6] arch: unify ioremap prototypes and macro aliases

[Dan Williams]

On Wed, Jul 1, 2015 at 1:09 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, Jul 01, 2015 at 08:55:57AM +0200, Geert Uytterhoeven wrote:
>> On Wed, Jul 1, 2015 at 8:23 AM, Christoph Hellwig <hch@lst.de> wrote:
>> >> One useful feature of the ifdef mess as implemented in the patch is
>> >> that you could test for whether ioremap_cache() is actually
>> >> implemented or falls back to default ioremap().  I think for
>> >> completeness archs should publish an ioremap type capabilities mask
>> >> for drivers that care... (I can imagine pmem caring), or default to
>> >> being permissive if something like IOREMAP_STRICT is not set.  There's
>> >> also the wrinkle of archs that can only support certain types of
>> >> mappings at a given alignment.
>> >
>> > I think doing this at runtime might be a better idea.  E.g. a
>> > ioremap_flags with the CACHED argument will return -EOPNOTSUP unless
>> > actually implemented.  On various architectures different CPUs or
>> > boards will have different capabilities in this area.
>>
>> So it would be the responsibility of the caller to fall back from
>> ioremap(..., CACHED) to ioremap(..., UNCACHED)?
>> I.e. all drivers using it should be changed...
>
> Another important point here is to define what the properties of the
> mappings are.  It's no good just saying "uncached".
>
> We've recently been around this over the PMEM driver and the broken
> addition of ioremap_wt() on ARM...
>
> By "properties" I mean stuff like whether unaligned accesses permitted,
> any kind of atomic access (eg, xchg, cmpxchg, etc).
>
> This matters: on ARM, a mapping suitable for a device does not support
> unaligned accesses or atomic accesses - only "memory-like" mappings
> support those.  However, memory-like mappings are not required to
> preserve access size, number of accesses, etc which makes them unsuitable
> for device registers.

I'm proposing that we explicitly switch "memory-like" use cases over
to a separate set of "memremap()" apis, as these are no longer
"__iomem" [1].

> The problem with ioremap_uncached() in particular is that we have LDD
> and other documentation telling people to use it to map device registers,
> so we can't define ioremap_uncached() on ARM to have memory-like
> properties, and it doesn't support unaligned accesses.
>
> I have a series of patches which fix up 32-bit ARM for the broken
> ioremap_wt() stuff that was merged during this merge window, which I
> intend to push out into linux-next at some point (possibly during the
> merge window, if not after -rc1) which also move ioremap*() out of line
> on ARM but more importantly, adds a load of documentation about the
> properties of the resulting mapping on ARM.

Sounds good, I'll look for that before proceeding on this clean up.

[1]: https://lists.01.org/pipermail/linux-nvdimm/2015-June/001331.html

[PATCH v5 2/6] arch: unify ioremap prototypes and macro aliases

[Luis R. Rodriguez]

On Wed, Jul 01, 2015 at 09:28:28AM +0200, Christoph Hellwig wrote:
> On Wed, Jul 01, 2015 at 09:19:29AM +0200, Geert Uytterhoeven wrote:
> > >> So it would be the responsibility of the caller to fall back from
> > >> ioremap(..., CACHED) to ioremap(..., UNCACHED)?
> > >> I.e. all drivers using it should be changed...
> > >
> > > All of the zero users we currently have will need to be changed, yes.
> > 
> > Good. Less work to convert all of these ;-)
> 
> And I didn't have enough coffee yet.  We of course have a few users of
> ioremap_cache(), and two implememantions but no users of ioremap_cached().
> Looks like the implementations can't even agree on the name.

Yies, that naming is icky... we also have quite a bit of ioremap_nocache() users:

mcgrof at ergon ~/linux-next (git::kill-mtrr)$ git grep ioremap_nocache drivers/| wc -l
359

On x86 the default ioremap() happens to map to ioremap_nocache() anyway as well.

This is on purpose, there is an ongoing effort to streamline ioremap_nocache()
for registers on the x86 front with the long term goal then of making PAT
strong UC the default preference for both ioremap() and ioremap_nocache() for
PAT enabled systems. This would prevent things like write-combining modifiers
from having any effect on the area. This comes with a small architectural
driver cost, it means all write-combining desired areas must be split out in
drivers properly.  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().

This was aleady done once but reverted later due to the regression issues on
video drivers not haveing the right ioremap_wc() calls. I'm finishing this
effort and am about a few patches away...

Once done and once things cool down we should go back and may consider flipping
the switch again to make strong UC default. For details refer to commit
de33c442ed2a465 ("x86 PAT: fix performance drop for glx, use UC minus
for ioremap(), ioremap_nocache() and pci_mmap_page_range()").

All this is fine in theory -- but Benjamin Herrenschmidt recently also
noted that on powerpc the write-combining may end up requiring each
register read/write with its own specific API. That is, we'd lose the
magic of having things being done behind the scenes, and that would
also mean tons of reads/writes may need to be converted over to be
explicit about write-combining preferences...

I will note that upon discussions it seems that the above requirement
may have been a slight mishap on not being explicit about our semantics
and requirements on ioremap() variants, technically it may be possible
that effectively PowerPC may not get any write-combining effects on
infiniband / networking / anything not doing write-combining on
userspace such as framebuffer... from what I gather that needs to
be fixed. 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.

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.

  * 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. 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 ? Are we going to have to sit and wait for a kumbaya every time
    a helper comes along to see how it all fits well for all architectures?
    The asm-generic io.h seemed to have set in place the ability to let
    architectures define things *when* they get to it, that seems like a much
    more fair approach *if* and *when possible*.  Can we not have and define
    a *safe* ioremap() call to fall under ?  The unaligned access concerns seem
    fair but.. again it seems we generally care about unaligned access anyway,
    so the concern really should be to fix all driver code to not do unaligned
    access, if possible no?

  * There are helpers such as set_memory_wc() which should not be used
    on IO memory, we should define grammar rules for these.

  * There are ioremap() variants which may require helpers for architectures.
    The only example I am aware of is ioremap_wc() requires arch_phys_wc_add()
    so that on x86 PAT enabled systems this does nothing, but on x86 non-PAT
    systems this will use MTRRs. The arch_phys_wc_add() API can be re-purposed
    for other architectures if needed, maybe benh can look at this for powerpc?
    But it seems those helpers were added mostly with a bias towards x86
    requirements, do we again expect all architecture maintainers to provide
    an Acked-by for ioremap() variants helpers ?

  Luis

[PATCH v5 2/6] arch: unify ioremap prototypes and macro aliases

[Russell King - ARM Linux]

On Tue, Jul 07, 2015 at 11:50:12AM +0200, Luis R. Rodriguez wrote:
> mcgrof at ergon ~/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.
(b) We have documentation in the Documentation/ subdirectory telling people
    to use ioremap_nocache() for the same.

> 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.

> 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.

> 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.  However, differing types would be a
problem on ARM.

>   * 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,
then we could potentially switch ioremap_nocache() to be a normal-memory
like mapping, which would allow it to support unaligned accesses.

>     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.  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.)

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.

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.

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.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH v5 2/6] arch: unify ioremap prototypes and macro aliases

[Geert Uytterhoeven]

On Tue, Jul 7, 2015 at 12:13 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> 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.

Yes they should. Not doing that is a typical portability bug (works on x86,
not everywhere).

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v5 2/6] arch: unify ioremap prototypes and macro aliases

[Luis R. Rodriguez]

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 at ergon ~/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

[PATCH v5 2/6] arch: unify ioremap prototypes and macro aliases

[Toshi Kani]

On Tue, 2015-07-07 at 18:07 +0200, Luis R. Rodriguez wrote:
> On Tue, Jul 07, 2015 at 11:13:30AM +0100, Russell King - ARM Linux 
> wrote:
  :
> > 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.

There are multiple mapping interfaces, and overlapping can happen among
them as well.  For instance, remap_pfn_range() (and 
io_remap_pfn_range(), which is the same as remap_pfn_range() on x86)
creates a mapping to user space.  The same physical ranges may be
mapped to kernel and user spaces.  /dev/mem is one example that may
create a user space mapping to a physical address that is already
mapped with ioremap() by other module.  pmem and DAX also create
mappings to the same NVDIMM ranges.  DAX calls vm_insert_mixed(), which
is particularly a problematic since vm_insert_mixed() does not verify
aliasing.  ioremap() and remap_pfn_range() call reserve_memtype() to
verify aliasing on x86.  reserve_memtype() is x86-specific and there is
no arch-generic wrapper for such check.  I think DAX could get a cache
type from pmem to keep them in sync, though.

Thanks,
-Toshi


 

[PATCH v5 2/6] arch: unify ioremap prototypes and macro aliases

[Luis R. Rodriguez]

On Tue, Jul 07, 2015 at 05:10:58PM -0600, Toshi Kani wrote:
> On Tue, 2015-07-07 at 18:07 +0200, Luis R. Rodriguez wrote:
> > On Tue, Jul 07, 2015 at 11:13:30AM +0100, Russell King - ARM Linux 
> > wrote:
>   :
> > > 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.
> 
> There are multiple mapping interfaces, and overlapping can happen among
> them as well.  For instance, remap_pfn_range() (and 
> io_remap_pfn_range(), which is the same as remap_pfn_range() on x86)
> creates a mapping to user space. The same physical ranges may be
> mapped to kernel and user spaces.  /dev/mem is one example that may
> create a user space mapping to a physical address that is already
> mapped with ioremap() by other module.

Thanks for the feedback. The restriction seems to be differing cache types
requirements, other than this, are there any other concerns ? For instance are
we completley happy with aliasing so long as cache types match everywhere?  I'd
expect no architecture would want cache types to differ when aliasing, what
should differ then I think would just be how to verify this and it doesn't seem
we may be doing this for all architectures.

Even for userspace we seem to be covered -- we enable userspace mmap() calls to
get their mapped space with a cache type, on the kernel we'd say use
pgprot_writecombine() on the vma->vm_page_prot prior to the
io_remap_pfn_range() -- that maps to remap_pfn_range() on x86 and as you note
that checks cache type via reserve_memtype() -- but only on x86...

Other than this differing cache type concern are we OK with aliasing in
userspace all the time ?

If we want to restrict aliasing either for the kernel or userspace mapping
we might be able to do it, I just want to know if we want to or not care
at all.

> pmem and DAX also create mappings to the same NVDIMM ranges.  DAX calls
> vm_insert_mixed(), which is particularly a problematic since
> vm_insert_mixed() does not verify aliasing.  ioremap() and remap_pfn_range()
> call reserve_memtype() to verify aliasing on x86.  reserve_memtype() is
> x86-specific and there is no arch-generic wrapper for such check.

As clarified by Matthew Wilcox via commit d92576f1167cacf7844 ("dax: does not
work correctly with virtual aliasing caches") caches are virtually mapped for
some architectures, it seems it should be possible to fix this for DAX somehow
though.

> I think DAX could get a cache type from pmem to keep them in sync, though.

pmem is x86 specific right now, are other folks going to expose something
similar ? Otherwise we seem to only be addressing these deep concerns for
x86 so far.

 Luis

[PATCH v5 2/6] arch: unify ioremap prototypes and macro aliases

[Toshi Kani]

On Thu, 2015-07-09 at 03:40 +0200, Luis R. Rodriguez wrote:
> On Tue, Jul 07, 2015 at 05:10:58PM -0600, Toshi Kani wrote:
> > On Tue, 2015-07-07 at 18:07 +0200, Luis R. Rodriguez wrote:
> > > On Tue, Jul 07, 2015 at 11:13:30AM +0100, Russell King - ARM 
> > > Linux 
> > > wrote:
> >   :
> > > > 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.
> > 
> > There are multiple mapping interfaces, and overlapping can happen 
> > among
> > them as well.  For instance, remap_pfn_range() (and 
> > io_remap_pfn_range(), which is the same as remap_pfn_range() on 
> > x86)
> > creates a mapping to user space. The same physical ranges may be
> > mapped to kernel and user spaces.  /dev/mem is one example that may
> > create a user space mapping to a physical address that is already
> > mapped with ioremap() by other module.
> 
> Thanks for the feedback. The restriction seems to be differing cache 
> types
> requirements, other than this, are there any other concerns ? For 
> instance are
> we completley happy with aliasing so long as cache types match 
> everywhere?  I'd
> expect no architecture would want cache types to differ when 
> aliasing, what
> should differ then I think would just be how to verify this and it 
> doesn't seem
> we may be doing this for all architectures.
> 
> Even for userspace we seem to be covered -- we enable userspace 
> mmap() calls to
> get their mapped space with a cache type, on the kernel we'd say use
> pgprot_writecombine() on the vma->vm_page_prot prior to the
> io_remap_pfn_range() -- that maps to remap_pfn_range() on x86 and as 
> you note
> that checks cache type via reserve_memtype() -- but only on x86...
> 
> Other than this differing cache type concern are we OK with aliasing 
> in
> userspace all the time ?
> 
> If we want to restrict aliasing either for the kernel or userspace 
> mapping
> we might be able to do it, I just want to know if we want to or not 
> care
> at all.

Yes, we allow to create multiple mappings to a same physical page as
long as their cache type is the same.  There are multiple use-cases
that depend on this ability.

> > pmem and DAX also create mappings to the same NVDIMM ranges.  DAX 
> > calls
> > vm_insert_mixed(), which is particularly a problematic since
> > vm_insert_mixed() does not verify aliasing.  ioremap() and 
> > remap_pfn_range()
> > call reserve_memtype() to verify aliasing on x86. 
> >  reserve_memtype() is
> > x86-specific and there is no arch-generic wrapper for such check.
> 
> As clarified by Matthew Wilcox via commit d92576f1167cacf7844 ("dax: 
> does not
> work correctly with virtual aliasing caches") caches are virtually 
> mapped for
> some architectures, it seems it should be possible to fix this for 
> DAX somehow
> though.

I simply described this DAX case as an example of how two modules might
request different cache types.  Yes, we should be able to fix this
case.

> > I think DAX could get a cache type from pmem to keep them in sync, 
> > though.
> 
> pmem is x86 specific right now, are other folks going to expose 
> something
> similar ? Otherwise we seem to only be addressing these deep concerns 
> for
> x86 so far.

pmem is a generic driver and is not x86-specific. 

Thanks,
-Toshi