Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1175028 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5 2/6] arch: unify ioremap prototypes and macro aliases Geert Uytterhoeven <geert@linux-m68k.org> - 2015-07-01 09:20 +0200
    Re: [PATCH v5 2/6] arch: unify ioremap prototypes and macro aliases Christoph Hellwig <hch@lst.de> - 2015-07-01 09:30 +0200
      Re: [PATCH v5 2/6] arch: unify ioremap prototypes and macro aliases "Luis R. Rodriguez" <mcgrof@suse.com> - 2015-07-07 12:00 +0200
        Re: [PATCH v5 2/6] arch: unify ioremap prototypes and macro aliases Russell King - ARM Linux <linux@arm.linux.org.uk> - 2015-07-07 12:20 +0200
          Re: [PATCH v5 2/6] arch: unify ioremap prototypes and macro aliases Geert Uytterhoeven <geert@linux-m68k.org> - 2015-07-07 12:30 +0200
          Re: [PATCH v5 2/6] arch: unify ioremap prototypes and macro aliases Toshi Kani <toshi.kani@hp.com> - 2015-07-08 01:20 +0200

#1175028 — Re: [PATCH v5 2/6] arch: unify ioremap prototypes and macro aliases

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

[toc] | [next] | [standalone]

#1175037

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

[toc] | [prev] | [next] | [standalone]

#1178297

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

[toc] | [prev] | [next] | [standalone]

#1178305

On Tue, Jul 07, 2015 at 11:50:12AM +0200, Luis R. Rodriguez wrote:
> mcgrof@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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1178313

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

[toc] | [prev] | [next] | [standalone]

#1179150

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


 


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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web