| Message ID | 9085d37fa97a762a46b9d58719c085368682c64f.1454950917.git.brian.starkey@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 :(
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.
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
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
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
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
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 >
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
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 --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);