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

Groups > linux.kernel > #1329398 > unrolled thread

[RESEND2 PATCH 1/3] memremap: add MEMREMAP_WC flag

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

  [RESEND2 PATCH 1/3] memremap: add MEMREMAP_WC flag Brian Starkey <brian.starkey@arm.com> - 2016-02-08 18:40 +0100
    Re: [RESEND2 PATCH 1/3] memremap: add MEMREMAP_WC flag Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-02-08 19:40 +0100
      Re: [RESEND2 PATCH 1/3] memremap: add MEMREMAP_WC flag Brian Starkey <brian.starkey@arm.com> - 2016-02-09 10:20 +0100
        Re: [RESEND2 PATCH 1/3] memremap: add MEMREMAP_WC flag Brian Starkey <brian.starkey@arm.com> - 2016-02-16 12:20 +0100
          Re: [RESEND2 PATCH 1/3] memremap: add MEMREMAP_WC flag Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-02-17 01:50 +0100
            Re: [RESEND2 PATCH 1/3] memremap: add MEMREMAP_WC flag Brian Starkey <brian.starkey@arm.com> - 2016-02-17 11:10 +0100
    Re: [RESEND2 PATCH 1/3] memremap: add MEMREMAP_WC flag Andrew Morton <akpm@linux-foundation.org> - 2016-02-08 21:10 +0100
      Re: [RESEND2 PATCH 1/3] memremap: add MEMREMAP_WC flag Brian Starkey <brian.starkey@arm.com> - 2016-02-09 11:30 +0100
        Re: [RESEND2 PATCH 1/3] memremap: add MEMREMAP_WC flag Brian Starkey <brian.starkey@arm.com> - 2016-02-17 13:00 +0100
          Re: [RESEND2 PATCH 1/3] memremap: add MEMREMAP_WC flag Andrew Morton <akpm@linux-foundation.org> - 2016-02-17 20:10 +0100

#1329398 — [RESEND2 PATCH 1/3] memremap: add MEMREMAP_WC flag

Add a flag to memremap() for writecombine mappings. Mappings satisfied
by this flag will not be cached, however writes may be delayed or
combined into more efficient bursts. This is most suitable for
buffers written sequentially by the CPU for use by other DMA devices.

Signed-off-by: Brian Starkey <brian.starkey@arm.com>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
---
 include/linux/io.h |    1 +
 kernel/memremap.c  |   15 +++++++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/include/linux/io.h b/include/linux/io.h
index 32403b5..e2c8419 100644
--- a/include/linux/io.h
+++ b/include/linux/io.h
@@ -135,6 +135,7 @@ enum {
 	/* See memremap() kernel-doc for usage description... */
 	MEMREMAP_WB = 1 << 0,
 	MEMREMAP_WT = 1 << 1,
+	MEMREMAP_WC = 1 << 2,
 };
 
 void *memremap(resource_size_t offset, size_t size, unsigned long flags);
diff --git a/kernel/memremap.c b/kernel/memremap.c
index e517a16..3849987 100644
--- a/kernel/memremap.c
+++ b/kernel/memremap.c
@@ -41,11 +41,13 @@ static void *try_ram_remap(resource_size_t offset, size_t size)
  * memremap() - remap an iomem_resource as cacheable memory
  * @offset: iomem resource start address
  * @size: size of remap
- * @flags: either MEMREMAP_WB or MEMREMAP_WT
+ * @flags: any of MEMREMAP_WB, MEMREMAP_WT and MEMREMAP_WC
  *
  * memremap() is "ioremap" for cases where it is known that the resource
  * being mapped does not have i/o side effects and the __iomem
- * annotation is not applicable.
+ * annotation is not applicable. In the case of multiple flags, the different
+ * mapping types will be attempted in the order listed below until one of
+ * them succeeds.
  *
  * MEMREMAP_WB - matches the default mapping for "System RAM" on
  * the architecture.  This is usually a read-allocate write-back cache.
@@ -57,6 +59,10 @@ static void *try_ram_remap(resource_size_t offset, size_t size)
  * cache or are written through to memory and never exist in a
  * cache-dirty state with respect to program visibility.  Attempts to
  * map "System RAM" with this mapping type will fail.
+ *
+ * MEMREMAP_WC - establish a writecombine mapping, whereby writes may
+ * be coalesced together (e.g. in the CPU's write buffers), but is otherwise
+ * uncached. Attempts to map "System RAM" with this mapping type will fail.
  */
 void *memremap(resource_size_t offset, size_t size, unsigned long flags)
 {
@@ -101,6 +107,11 @@ void *memremap(resource_size_t offset, size_t size, unsigned long flags)
 		addr = ioremap_wt(offset, size);
 	}
 
+	if (!addr && (flags & MEMREMAP_WC)) {
+		flags &= ~MEMREMAP_WC;
+		addr = ioremap_wc(offset, size);
+	}
+
 	return addr;
 }
 EXPORT_SYMBOL(memremap);
-- 
1.7.9.5

[toc] | [next] | [standalone]

#1329464

On Mon, Feb 08, 2016 at 05:30:50PM +0000, Brian Starkey wrote:
> Add a flag to memremap() for writecombine mappings. Mappings satisfied
> by this flag will not be cached, however writes may be delayed or
> combined into more efficient bursts. This is most suitable for
> buffers written sequentially by the CPU for use by other DMA devices.
> 
> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> ---
>  include/linux/io.h |    1 +
>  kernel/memremap.c  |   15 +++++++++++++--
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/io.h b/include/linux/io.h
> index 32403b5..e2c8419 100644
> --- a/include/linux/io.h
> +++ b/include/linux/io.h
> @@ -135,6 +135,7 @@ enum {
>  	/* See memremap() kernel-doc for usage description... */
>  	MEMREMAP_WB = 1 << 0,
>  	MEMREMAP_WT = 1 << 1,
> +	MEMREMAP_WC = 1 << 2,

You didn't update the documentation :(

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

#1329982

Hi Greg,

On Mon, Feb 08, 2016 at 10:30:06AM -0800, Greg KH wrote:
>On Mon, Feb 08, 2016 at 05:30:50PM +0000, Brian Starkey wrote:
>> diff --git a/include/linux/io.h b/include/linux/io.h
>> index 32403b5..e2c8419 100644
>> --- a/include/linux/io.h
>> +++ b/include/linux/io.h
>> @@ -135,6 +135,7 @@ enum {
>>  	/* See memremap() kernel-doc for usage description... */
>>  	MEMREMAP_WB = 1 << 0,
>>  	MEMREMAP_WT = 1 << 1,
>> +	MEMREMAP_WC = 1 << 2,
>
>You didn't update the documentation :(
>

Maybe I missed something, but I don't think there's anything to update
here? Like the comment says, the flags are documented in the memremap()
kernel-doc (which I did update - see the next two hunks of this patch).

Thanks,

Brian

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

#1335286

Hi Greg,

Is the documentation OK? Is there any chance of you picking up this
series?

I can rebase onto -next if that's the right place, but they still apply
on 4.5-rc4 and fix a panic, so I thought perhaps they could make 4.5.

Thanks,
Brian

On Tue, Feb 09, 2016 at 09:15:02AM +0000, Brian Starkey wrote:
>Hi Greg,
>
>On Mon, Feb 08, 2016 at 10:30:06AM -0800, Greg KH wrote:
>>On Mon, Feb 08, 2016 at 05:30:50PM +0000, Brian Starkey wrote:
>>>diff --git a/include/linux/io.h b/include/linux/io.h
>>>index 32403b5..e2c8419 100644
>>>--- a/include/linux/io.h
>>>+++ b/include/linux/io.h
>>>@@ -135,6 +135,7 @@ enum {
>>> 	/* See memremap() kernel-doc for usage description... */
>>> 	MEMREMAP_WB = 1 << 0,
>>> 	MEMREMAP_WT = 1 << 1,
>>>+	MEMREMAP_WC = 1 << 2,
>>
>>You didn't update the documentation :(
>>
>
>Maybe I missed something, but I don't think there's anything to update
>here? Like the comment says, the flags are documented in the memremap()
>kernel-doc (which I did update - see the next two hunks of this patch).
>
>Thanks,
>
>Brian

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

#1335963

On Tue, Feb 16, 2016 at 11:14:26AM +0000, Brian Starkey wrote:
> Hi Greg,
> 
> Is the documentation OK? Is there any chance of you picking up this
> series?
> 
> I can rebase onto -next if that's the right place, but they still apply
> on 4.5-rc4 and fix a panic, so I thought perhaps they could make 4.5.

I think memory stuff like this goes through Andrew's tree, not mine...

thanks,

greg k-h

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

#1336196

On Tue, Feb 16, 2016 at 04:43:51PM -0800, Greg Kroah-Hartman wrote:
>On Tue, Feb 16, 2016 at 11:14:26AM +0000, Brian Starkey wrote:
>> Hi Greg,
>>
>> Is the documentation OK? Is there any chance of you picking up this
>> series?
>>
>> I can rebase onto -next if that's the right place, but they still apply
>> on 4.5-rc4 and fix a panic, so I thought perhaps they could make 4.5.
>
>I think memory stuff like this goes through Andrew's tree, not mine...

Right, fair enough. I was just blindly working off get_maintainer.pl

Cheers,

Brian
>
>thanks,
>
>greg k-h
>

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

#1329515

On Mon,  8 Feb 2016 17:30:50 +0000 Brian Starkey <brian.starkey@arm.com> wrote:

> Add a flag to memremap() for writecombine mappings. Mappings satisfied
> by this flag will not be cached, however writes may be delayed or
> combined into more efficient bursts. This is most suitable for
> buffers written sequentially by the CPU for use by other DMA devices.
> 
> ...

The patch generally looks OK to me.  It generates rejects against
linux-next because of some janitorial changes in memremap.c.


> @@ -101,6 +107,11 @@ void *memremap(resource_size_t offset, size_t size, unsigned long flags)
>  		addr = ioremap_wt(offset, size);
>  	}
>  
> +	if (!addr && (flags & MEMREMAP_WC)) {
> +		flags &= ~MEMREMAP_WC;
> +		addr = ioremap_wc(offset, size);
> +	}
> +
>  	return addr;
>  }

The modifications of `flags' is unneeded (and the compiler will remove
it).  And generally the modification of incoming args is a bit nasty
IMO - I find it's better to treat them as const - part of the calling
environment which can be relied upon to be unaltered as the code
evolves.

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

#1330032

Hi Andrew,

Thanks for taking a look,

On Mon, Feb 08, 2016 at 12:03:17PM -0800, Andrew Morton wrote:
>On Mon,  8 Feb 2016 17:30:50 +0000 Brian Starkey <brian.starkey@arm.com> wrote:
>The patch generally looks OK to me.  It generates rejects against
>linux-next because of some janitorial changes in memremap.c.
>

Ah yeah, so it does - sorry. I was hoping this could make it into 4.5,
but I can rebase onto linux-next if that's better. Annoyingly it only
conflicts because of a couple of quotation marks.

>
>> @@ -101,6 +107,11 @@ void *memremap(resource_size_t offset, size_t size, unsigned long flags)
>>  		addr = ioremap_wt(offset, size);
>>  	}
>>
>> +	if (!addr && (flags & MEMREMAP_WC)) {
>> +		flags &= ~MEMREMAP_WC;
>> +		addr = ioremap_wc(offset, size);
>> +	}
>> +
>>  	return addr;
>>  }
>
>The modifications of `flags' is unneeded (and the compiler will remove
>it).  And generally the modification of incoming args is a bit nasty
>IMO - I find it's better to treat them as const - part of the calling
>environment which can be relied upon to be unaltered as the code
>evolves.
>

To be honest I was just mirroring the rest of the function. I guess
the idea was filtering the different mapping types in case one of the
'mappers' can handle multiple flags or something. I'll remove it if
you like, I just thought that extending the functionality in-keeping
with the current semantics was a better evolution - let me know.

Thanks,
Brian

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

#1336300

Hi Andrew,

Would you pick these up if I rebase onto linux-next?

How strongly do you feel about the input argument modification vs.
staying in-line with the rest of the function?

Thanks,

Brian

On Tue, Feb 09, 2016 at 10:23:00AM +0000, Brian Starkey wrote:
>Hi Andrew,
>
>Thanks for taking a look,
>
>On Mon, Feb 08, 2016 at 12:03:17PM -0800, Andrew Morton wrote:
>>On Mon,  8 Feb 2016 17:30:50 +0000 Brian Starkey <brian.starkey@arm.com> wrote:
>>The patch generally looks OK to me.  It generates rejects against
>>linux-next because of some janitorial changes in memremap.c.
>>
>
>Ah yeah, so it does - sorry. I was hoping this could make it into 4.5,
>but I can rebase onto linux-next if that's better. Annoyingly it only
>conflicts because of a couple of quotation marks.
>
>>
>>>@@ -101,6 +107,11 @@ void *memremap(resource_size_t offset, size_t size, unsigned long flags)
>>> 		addr = ioremap_wt(offset, size);
>>> 	}
>>>
>>>+	if (!addr && (flags & MEMREMAP_WC)) {
>>>+		flags &= ~MEMREMAP_WC;
>>>+		addr = ioremap_wc(offset, size);
>>>+	}
>>>+
>>> 	return addr;
>>> }
>>
>>The modifications of `flags' is unneeded (and the compiler will remove
>>it).  And generally the modification of incoming args is a bit nasty
>>IMO - I find it's better to treat them as const - part of the calling
>>environment which can be relied upon to be unaltered as the code
>>evolves.
>>
>
>To be honest I was just mirroring the rest of the function. I guess
>the idea was filtering the different mapping types in case one of the
>'mappers' can handle multiple flags or something. I'll remove it if
>you like, I just thought that extending the functionality in-keeping
>with the current semantics was a better evolution - let me know.
>
>Thanks,
>Brian

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

#1336666

On Wed, 17 Feb 2016 11:53:48 +0000 Brian Starkey <brian.starkey@arm.com> wrote:

> Hi Andrew,
> 
> Would you pick these up if I rebase onto linux-next?

Sure.

> How strongly do you feel about the input argument modification vs.
> staying in-line with the rest of the function?

I see no reason why memremap() is modifying `flags' as it proceeds -
these flags are all disjoint so it's pointless.  I suggest you simply
take all that out in a preparatory patch.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web