diff mbox

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

Message ID 9085d37fa97a762a46b9d58719c085368682c64f.1454950917.git.brian.starkey@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Brian Starkey Feb. 8, 2016, 5:30 p.m. UTC
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(-)

Comments

Greg Kroah-Hartman Feb. 8, 2016, 6:30 p.m. UTC | #1
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 :(
Andrew Morton Feb. 8, 2016, 8:03 p.m. UTC | #2
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.
Brian Starkey Feb. 9, 2016, 9:15 a.m. UTC | #3
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
Brian Starkey Feb. 9, 2016, 10:23 a.m. UTC | #4
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
Brian Starkey Feb. 16, 2016, 11:14 a.m. UTC | #5
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
Greg Kroah-Hartman Feb. 17, 2016, 12:43 a.m. UTC | #6
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
Brian Starkey Feb. 17, 2016, 10:07 a.m. UTC | #7
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
>
Brian Starkey Feb. 17, 2016, 11:53 a.m. UTC | #8
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
Andrew Morton Feb. 17, 2016, 7:02 p.m. UTC | #9
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.
diff mbox

Patch

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