[PATCH v2 0/3] fix memremap on ARM

[PATCH v2 0/3] fix memremap on ARM

[Ard Biesheuvel]

This is something I ran into while working on support for the UEFI
memory attributes and ESRT tables. In both cases, these tables are
passed to the kernel in memory which is guaranteed to be below 4 GB,
but may be outside of the kernel direct mapping. (UEFI typically
attempts to allocate from the top down, which means such tables are
highly likely to be in highmem for any system with more than 760 MB
of system RAM)

The recently introduced memremap() is a very useful abstraction for
accessing such tables, because it is generic, and already attempts to
do the right thing with respect to regions that may already have been
mapped directly. However, it falls back to ioremap_cache() for mapping
high memory, which is not allowed on ARM for system RAM, and also results
in the region to be mapped with different attributes depending on whether
it is covered by lowmem or not.

So instead, create an arch specific hook 'arch_memremap_wb(), and
implement it for ARM using the same memory attributes used for the
linear mapping. Note that memremap will only call this hook for regions
that are not already mapped permanently.

Since this change results in memremap() to use attributes different from
the ones used by ioremap_cache(), revert the change to pxa2xx-flash that
moved it to memremap.

Changes since v1/rfc:
- new patch #1 that reverts the ioremap_cache->memremap conversion for the
  pxa2xx-flash driver
- added Dan's ack to patch #2

Ard Biesheuvel (3):
  Revert "mtd: pxa2xx-flash: switch from ioremap_cache to memremap"
  memremap: add arch specific hook for MEMREMAP_WB mappings
  ARM: memremap: implement arch_memremap_wb()

 arch/arm/include/asm/io.h       |  3 +++
 arch/arm/mm/ioremap.c           | 11 +++++++++--
 drivers/mtd/maps/pxa2xx-flash.c |  6 +++---
 kernel/memremap.c               | 11 ++++++++---
 4 files changed, 23 insertions(+), 8 deletions(-)

-- 
2.5.0

[PATCH v2 1/3] Revert "mtd: pxa2xx-flash: switch from ioremap_cache to memremap"

[Ard Biesheuvel]

This reverts commit 06968a54790d9dd87a5bb68e8197b5094eff63c3, since NOR
with memory semantics in array mode and RAM are not necessarily the same
thing, and architectures may implement ioremap_cache() and memremap()
with different memory attributes.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/mtd/maps/pxa2xx-flash.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/maps/pxa2xx-flash.c b/drivers/mtd/maps/pxa2xx-flash.c
index 7497090e9900..9adc4ef30891 100644
--- a/drivers/mtd/maps/pxa2xx-flash.c
+++ b/drivers/mtd/maps/pxa2xx-flash.c
@@ -71,8 +71,8 @@ static int pxa2xx_flash_probe(struct platform_device *pdev)
 		       info->map.name);
 		return -ENOMEM;
 	}
-	info->map.cached = memremap(info->map.phys, info->map.size,
-			MEMREMAP_WB);
+	info->map.cached =
+		ioremap_cache(info->map.phys, info->map.size);
 	if (!info->map.cached)
 		printk(KERN_WARNING "Failed to ioremap cached %s\n",
 		       info->map.name);
@@ -111,7 +111,7 @@ static int pxa2xx_flash_remove(struct platform_device *dev)
 	map_destroy(info->mtd);
 	iounmap(info->map.virt);
 	if (info->map.cached)
-		memunmap(info->map.cached);
+		iounmap(info->map.cached);
 	kfree(info);
 	return 0;
 }
-- 
2.5.0

[PATCH v2 2/3] memremap: add arch specific hook for MEMREMAP_WB mappings

[Ard Biesheuvel]

Currently, the memremap code serves MEMREMAP_WB mappings directly from
the kernel direct mapping, unless the region is in high memory, in which
case it falls back to using ioremap_cache(). However, the semantics of
ioremap_cache() are not unambiguously defined, and on ARM, it will
actually result in a mapping type that differs from the attributes used
for the linear mapping, and for this reason, the ioremap_cache() call
fails if the region is part of the memory managed by the kernel.

So instead, implement an optional hook 'arch_memremap_wb' whose default
implementation calls ioremap_cache() as before, but which can be
overridden by the architecture to do what is appropriate for it.

Acked-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 kernel/memremap.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/kernel/memremap.c b/kernel/memremap.c
index 7a1b5c3ef14e..77c41648ba16 100644
--- a/kernel/memremap.c
+++ b/kernel/memremap.c
@@ -27,6 +27,13 @@ __weak void __iomem *ioremap_cache(resource_size_t offset, unsigned long size)
 }
 #endif
 
+#ifndef arch_memremap_wb
+static void *arch_memremap_wb(resource_size_t offset, unsigned long size)
+{
+	return (__force void *)ioremap_cache(offset, size);
+}
+#endif
+
 static void *try_ram_remap(resource_size_t offset, size_t size)
 {
 	struct page *page = pfn_to_page(offset >> PAGE_SHIFT);
@@ -34,7 +41,7 @@ static void *try_ram_remap(resource_size_t offset, size_t size)
 	/* In the simple case just return the existing linear address */
 	if (!PageHighMem(page))
 		return __va(offset);
-	return NULL; /* fallback to ioremap_cache */
+	return arch_memremap_wb(offset, size);
 }
 
 /**
@@ -80,8 +87,6 @@ void *memremap(resource_size_t offset, size_t size, unsigned long flags)
 		 */
 		if (is_ram == REGION_INTERSECTS)
 			addr = try_ram_remap(offset, size);
-		if (!addr)
-			addr = ioremap_cache(offset, size);
 	}
 
 	/*
-- 
2.5.0

[PATCH v2 3/3] ARM: memremap: implement arch_memremap_wb()

[Ard Biesheuvel]

The generic memremap() falls back to using ioremap_cache() to create
MEMREMAP_WB mappings if the requested region is not already covered
by the linear mapping, unless the architecture provides an implementation
of arch_memremap_wb().

Since ioremap_cache() is not appropriate on ARM to map memory with the
same attributes used for the linear mapping, implement arch_memremap_wb()
which does exactly that. Also, relax the WARN() check to allow MT_MEMORY_RW
mappings of pfn_valid() pages.

Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm/include/asm/io.h |  3 +++
 arch/arm/mm/ioremap.c     | 11 +++++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
index 485982084fe9..7456638e6b3a 100644
--- a/arch/arm/include/asm/io.h
+++ b/arch/arm/include/asm/io.h
@@ -402,6 +402,9 @@ void __iomem *ioremap_wc(resource_size_t res_cookie, size_t size);
 void iounmap(volatile void __iomem *iomem_cookie);
 #define iounmap iounmap
 
+void *arch_memremap_wb(phys_addr_t phys_addr, size_t size);
+#define arch_memremap_wb arch_memremap_wb
+
 /*
  * io{read,write}{16,32}be() macros
  */
diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
index 66a978d05958..d3a2b028c614 100644
--- a/arch/arm/mm/ioremap.c
+++ b/arch/arm/mm/ioremap.c
@@ -297,9 +297,10 @@ static void __iomem * __arm_ioremap_pfn_caller(unsigned long pfn,
 	}
 
 	/*
-	 * Don't allow RAM to be mapped - this causes problems with ARMv6+
+	 * Don't allow RAM to be mapped with mismatched attributes - this
+	 * causes problems with ARMv6+
 	 */
-	if (WARN_ON(pfn_valid(pfn)))
+	if (WARN_ON(pfn_valid(pfn) && mtype != MT_MEMORY_RW))
 		return NULL;
 
 	area = get_vm_area_caller(size, VM_IOREMAP, caller);
@@ -414,6 +415,12 @@ __arm_ioremap_exec(phys_addr_t phys_addr, size_t size, bool cached)
 			__builtin_return_address(0));
 }
 
+void *arch_memremap_wb(phys_addr_t phys_addr, size_t size)
+{
+	return __arm_ioremap_caller(phys_addr, size, MT_MEMORY_RW,
+			__builtin_return_address(0));
+}
+
 void __iounmap(volatile void __iomem *io_addr)
 {
 	void *addr = (void *)(PAGE_MASK & (unsigned long)io_addr);
-- 
2.5.0

[PATCH v2 0/3] fix memremap on ARM

[Ard Biesheuvel]

On 26 February 2016 at 18:34, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> This is something I ran into while working on support for the UEFI
> memory attributes and ESRT tables. In both cases, these tables are
> passed to the kernel in memory which is guaranteed to be below 4 GB,
> but may be outside of the kernel direct mapping. (UEFI typically
> attempts to allocate from the top down, which means such tables are
> highly likely to be in highmem for any system with more than 760 MB
> of system RAM)
>
> The recently introduced memremap() is a very useful abstraction for
> accessing such tables, because it is generic, and already attempts to
> do the right thing with respect to regions that may already have been
> mapped directly. However, it falls back to ioremap_cache() for mapping
> high memory, which is not allowed on ARM for system RAM, and also results
> in the region to be mapped with different attributes depending on whether
> it is covered by lowmem or not.
>
> So instead, create an arch specific hook 'arch_memremap_wb(), and
> implement it for ARM using the same memory attributes used for the
> linear mapping. Note that memremap will only call this hook for regions
> that are not already mapped permanently.
>
> Since this change results in memremap() to use attributes different from
> the ones used by ioremap_cache(), revert the change to pxa2xx-flash that
> moved it to memremap.
>
> Changes since v1/rfc:
> - new patch #1 that reverts the ioremap_cache->memremap conversion for the
>   pxa2xx-flash driver
> - added Dan's ack to patch #2
>

If there are no remaining objections, may I suggest that Dan takes
patch #1 and #2, and that I put the third patch into Russell's patch
system? The only strict ordering requirement is that #1 is merged
before #2 and #3 both hit mainline, and that is solved by taking #1
and #2 in order.

Thanks,
Ard.

[PATCH v2 0/3] fix memremap on ARM

[Russell King - ARM Linux]

On Thu, Mar 03, 2016 at 05:24:41PM +0100, Ard Biesheuvel wrote:
> If there are no remaining objections, may I suggest that Dan takes
> patch #1 and #2, and that I put the third patch into Russell's patch
> system? The only strict ordering requirement is that #1 is merged
> before #2 and #3 both hit mainline, and that is solved by taking #1
> and #2 in order.

How does the dependency between 1 and 3 get satisfied?  Your comment
makes no sense to me.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH v2 0/3] fix memremap on ARM

[Ard Biesheuvel]

On 3 March 2016 at 18:13, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Mar 03, 2016 at 05:24:41PM +0100, Ard Biesheuvel wrote:
>> If there are no remaining objections, may I suggest that Dan takes
>> patch #1 and #2, and that I put the third patch into Russell's patch
>> system? The only strict ordering requirement is that #1 is merged
>> before #2 and #3 both hit mainline, and that is solved by taking #1
>> and #2 in order.
>
> How does the dependency between 1 and 3 get satisfied?  Your comment
> makes no sense to me.
>

Patch #3 introduces a function arch_memremap_wb() on the ARM side
which will never get called unless patch #2 is also merged. Once both
#2 and #3 are in, the memremap() call that #1 reverts will use
MT_MEMORY_RW attributes rather than MT_DEVICE_CACHED, so #1 needs to
go first, but #2 and #3 can go in in either order.

In other words, there are no build dependencies between the patches,
but to prevent pxa2xx-flash from using MT_MEMORY_RW attributes, it
needs to go in before #2

[PATCH v2 0/3] fix memremap on ARM

[Dan Williams]

On Thu, Mar 3, 2016 at 9:16 AM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On 3 March 2016 at 18:13, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Thu, Mar 03, 2016 at 05:24:41PM +0100, Ard Biesheuvel wrote:
>>> If there are no remaining objections, may I suggest that Dan takes
>>> patch #1 and #2, and that I put the third patch into Russell's patch
>>> system? The only strict ordering requirement is that #1 is merged
>>> before #2 and #3 both hit mainline, and that is solved by taking #1
>>> and #2 in order.
>>
>> How does the dependency between 1 and 3 get satisfied?  Your comment
>> makes no sense to me.
>>
>
> Patch #3 introduces a function arch_memremap_wb() on the ARM side
> which will never get called unless patch #2 is also merged. Once both
> #2 and #3 are in, the memremap() call that #1 reverts will use
> MT_MEMORY_RW attributes rather than MT_DEVICE_CACHED, so #1 needs to
> go first, but #2 and #3 can go in in either order.
>
> In other words, there are no build dependencies between the patches,
> but to prevent pxa2xx-flash from using MT_MEMORY_RW attributes, it
> needs to go in before #2

Can we add a new MEMREMAP_ type for this case so that we don't need to
keep carrying the confusing ioremap_cache() which means different
things to different archs and silently falls back to uncached on some
archs.

[PATCH v2 0/3] fix memremap on ARM

[Ard Biesheuvel]

On 3 March 2016 at 18:27, Dan Williams <dan.j.williams@intel.com> wrote:
> On Thu, Mar 3, 2016 at 9:16 AM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>> On 3 March 2016 at 18:13, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>>> On Thu, Mar 03, 2016 at 05:24:41PM +0100, Ard Biesheuvel wrote:
>>>> If there are no remaining objections, may I suggest that Dan takes
>>>> patch #1 and #2, and that I put the third patch into Russell's patch
>>>> system? The only strict ordering requirement is that #1 is merged
>>>> before #2 and #3 both hit mainline, and that is solved by taking #1
>>>> and #2 in order.
>>>
>>> How does the dependency between 1 and 3 get satisfied?  Your comment
>>> makes no sense to me.
>>>
>>
>> Patch #3 introduces a function arch_memremap_wb() on the ARM side
>> which will never get called unless patch #2 is also merged. Once both
>> #2 and #3 are in, the memremap() call that #1 reverts will use
>> MT_MEMORY_RW attributes rather than MT_DEVICE_CACHED, so #1 needs to
>> go first, but #2 and #3 can go in in either order.
>>
>> In other words, there are no build dependencies between the patches,
>> but to prevent pxa2xx-flash from using MT_MEMORY_RW attributes, it
>> needs to go in before #2
>
> Can we add a new MEMREMAP_ type for this case so that we don't need to
> keep carrying the confusing ioremap_cache() which means different
> things to different archs and silently falls back to uncached on some
> archs.

Actually, I don't think so. Unifying ioremap_cache() between
architectures was a mistake to begin with, since I/O mapping
attributes vary so wildly between architectures, and there are very
few drivers that depend on such specific behavior that actually need
to be built for multiple architectures.

So I think your memremap() is a fantastic idea, considering the
widespread abuse of ioremap_cache(), also in common code. But removing
it altogether to replace it with something that looks generic but
never is, is just repeating the same mistake imo.

[PATCH v2 0/3] fix memremap on ARM

[Dan Williams]

On Thu, Mar 3, 2016 at 9:30 AM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On 3 March 2016 at 18:27, Dan Williams <dan.j.williams@intel.com> wrote:
>> On Thu, Mar 3, 2016 at 9:16 AM, Ard Biesheuvel
>> <ard.biesheuvel@linaro.org> wrote:
>>> On 3 March 2016 at 18:13, Russell King - ARM Linux
>>> <linux@arm.linux.org.uk> wrote:
>>>> On Thu, Mar 03, 2016 at 05:24:41PM +0100, Ard Biesheuvel wrote:
>>>>> If there are no remaining objections, may I suggest that Dan takes
>>>>> patch #1 and #2, and that I put the third patch into Russell's patch
>>>>> system? The only strict ordering requirement is that #1 is merged
>>>>> before #2 and #3 both hit mainline, and that is solved by taking #1
>>>>> and #2 in order.
>>>>
>>>> How does the dependency between 1 and 3 get satisfied?  Your comment
>>>> makes no sense to me.
>>>>
>>>
>>> Patch #3 introduces a function arch_memremap_wb() on the ARM side
>>> which will never get called unless patch #2 is also merged. Once both
>>> #2 and #3 are in, the memremap() call that #1 reverts will use
>>> MT_MEMORY_RW attributes rather than MT_DEVICE_CACHED, so #1 needs to
>>> go first, but #2 and #3 can go in in either order.
>>>
>>> In other words, there are no build dependencies between the patches,
>>> but to prevent pxa2xx-flash from using MT_MEMORY_RW attributes, it
>>> needs to go in before #2
>>
>> Can we add a new MEMREMAP_ type for this case so that we don't need to
>> keep carrying the confusing ioremap_cache() which means different
>> things to different archs and silently falls back to uncached on some
>> archs.
>
> Actually, I don't think so. Unifying ioremap_cache() between
> architectures was a mistake to begin with, since I/O mapping
> attributes vary so wildly between architectures, and there are very
> few drivers that depend on such specific behavior that actually need
> to be built for multiple architectures.
>
> So I think your memremap() is a fantastic idea, considering the
> widespread abuse of ioremap_cache(), also in common code. But removing
> it altogether to replace it with something that looks generic but
> never is, is just repeating the same mistake imo.

I'm not suggesting it be a generic flag.  I.e. something like
MEMREMAP_ARM_ that explicitly documents that this driver has arch
specific mapping dependencies.  That request type will fail on any
arch that does not implement MEMREMAP_ARM_ support.

[PATCH v2 0/3] fix memremap on ARM

[Ard Biesheuvel]

On 3 March 2016 at 18:33, Dan Williams <dan.j.williams@intel.com> wrote:
> On Thu, Mar 3, 2016 at 9:30 AM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>> On 3 March 2016 at 18:27, Dan Williams <dan.j.williams@intel.com> wrote:
>>> On Thu, Mar 3, 2016 at 9:16 AM, Ard Biesheuvel
>>> <ard.biesheuvel@linaro.org> wrote:
>>>> On 3 March 2016 at 18:13, Russell King - ARM Linux
>>>> <linux@arm.linux.org.uk> wrote:
>>>>> On Thu, Mar 03, 2016 at 05:24:41PM +0100, Ard Biesheuvel wrote:
>>>>>> If there are no remaining objections, may I suggest that Dan takes
>>>>>> patch #1 and #2, and that I put the third patch into Russell's patch
>>>>>> system? The only strict ordering requirement is that #1 is merged
>>>>>> before #2 and #3 both hit mainline, and that is solved by taking #1
>>>>>> and #2 in order.
>>>>>
>>>>> How does the dependency between 1 and 3 get satisfied?  Your comment
>>>>> makes no sense to me.
>>>>>
>>>>
>>>> Patch #3 introduces a function arch_memremap_wb() on the ARM side
>>>> which will never get called unless patch #2 is also merged. Once both
>>>> #2 and #3 are in, the memremap() call that #1 reverts will use
>>>> MT_MEMORY_RW attributes rather than MT_DEVICE_CACHED, so #1 needs to
>>>> go first, but #2 and #3 can go in in either order.
>>>>
>>>> In other words, there are no build dependencies between the patches,
>>>> but to prevent pxa2xx-flash from using MT_MEMORY_RW attributes, it
>>>> needs to go in before #2
>>>
>>> Can we add a new MEMREMAP_ type for this case so that we don't need to
>>> keep carrying the confusing ioremap_cache() which means different
>>> things to different archs and silently falls back to uncached on some
>>> archs.
>>
>> Actually, I don't think so. Unifying ioremap_cache() between
>> architectures was a mistake to begin with, since I/O mapping
>> attributes vary so wildly between architectures, and there are very
>> few drivers that depend on such specific behavior that actually need
>> to be built for multiple architectures.
>>
>> So I think your memremap() is a fantastic idea, considering the
>> widespread abuse of ioremap_cache(), also in common code. But removing
>> it altogether to replace it with something that looks generic but
>> never is, is just repeating the same mistake imo.
>
> I'm not suggesting it be a generic flag.  I.e. something like
> MEMREMAP_ARM_ that explicitly documents that this driver has arch
> specific mapping dependencies.  That request type will fail on any
> arch that does not implement MEMREMAP_ARM_ support.

I don't see the point of adding this to a generic API. ioremap_cache()
is used to perform I/O, even if it has memory semantics on read.
Getting rid of the abuse, where ioremap_cache() is used to map system
RAM to access ACPI tables etc is something that we should fix, yes,
but arch specific I/O needs to stay out of the picture imo, simply
because there is no problem to fix here.

[PATCH v2 0/3] fix memremap on ARM

[Dan Williams]

On Thu, Mar 3, 2016 at 9:41 AM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On 3 March 2016 at 18:33, Dan Williams <dan.j.williams@intel.com> wrote:
>> On Thu, Mar 3, 2016 at 9:30 AM, Ard Biesheuvel
>> <ard.biesheuvel@linaro.org> wrote:
>>> On 3 March 2016 at 18:27, Dan Williams <dan.j.williams@intel.com> wrote:
>>>> On Thu, Mar 3, 2016 at 9:16 AM, Ard Biesheuvel
>>>> <ard.biesheuvel@linaro.org> wrote:
>>>>> On 3 March 2016 at 18:13, Russell King - ARM Linux
>>>>> <linux@arm.linux.org.uk> wrote:
>>>>>> On Thu, Mar 03, 2016 at 05:24:41PM +0100, Ard Biesheuvel wrote:
>>>>>>> If there are no remaining objections, may I suggest that Dan takes
>>>>>>> patch #1 and #2, and that I put the third patch into Russell's patch
>>>>>>> system? The only strict ordering requirement is that #1 is merged
>>>>>>> before #2 and #3 both hit mainline, and that is solved by taking #1
>>>>>>> and #2 in order.
>>>>>>
>>>>>> How does the dependency between 1 and 3 get satisfied?  Your comment
>>>>>> makes no sense to me.
>>>>>>
>>>>>
>>>>> Patch #3 introduces a function arch_memremap_wb() on the ARM side
>>>>> which will never get called unless patch #2 is also merged. Once both
>>>>> #2 and #3 are in, the memremap() call that #1 reverts will use
>>>>> MT_MEMORY_RW attributes rather than MT_DEVICE_CACHED, so #1 needs to
>>>>> go first, but #2 and #3 can go in in either order.
>>>>>
>>>>> In other words, there are no build dependencies between the patches,
>>>>> but to prevent pxa2xx-flash from using MT_MEMORY_RW attributes, it
>>>>> needs to go in before #2
>>>>
>>>> Can we add a new MEMREMAP_ type for this case so that we don't need to
>>>> keep carrying the confusing ioremap_cache() which means different
>>>> things to different archs and silently falls back to uncached on some
>>>> archs.
>>>
>>> Actually, I don't think so. Unifying ioremap_cache() between
>>> architectures was a mistake to begin with, since I/O mapping
>>> attributes vary so wildly between architectures, and there are very
>>> few drivers that depend on such specific behavior that actually need
>>> to be built for multiple architectures.
>>>
>>> So I think your memremap() is a fantastic idea, considering the
>>> widespread abuse of ioremap_cache(), also in common code. But removing
>>> it altogether to replace it with something that looks generic but
>>> never is, is just repeating the same mistake imo.
>>
>> I'm not suggesting it be a generic flag.  I.e. something like
>> MEMREMAP_ARM_ that explicitly documents that this driver has arch
>> specific mapping dependencies.  That request type will fail on any
>> arch that does not implement MEMREMAP_ARM_ support.
>
> I don't see the point of adding this to a generic API. ioremap_cache()
> is used to perform I/O, even if it has memory semantics on read.
> Getting rid of the abuse, where ioremap_cache() is used to map system
> RAM to access ACPI tables etc is something that we should fix, yes,
> but arch specific I/O needs to stay out of the picture imo, simply
> because there is no problem to fix here.

Hmm, then what about Russell's suggestoin, if I'm not
mischaracterizing, that ioremap_cache() move to its old
ioremap_cached() name.  The latter has less cross-arch confusion.

[PATCH v2 0/3] fix memremap on ARM

[Ard Biesheuvel]

On 3 March 2016 at 18:55, Dan Williams <dan.j.williams@intel.com> wrote:
> On Thu, Mar 3, 2016 at 9:41 AM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>> On 3 March 2016 at 18:33, Dan Williams <dan.j.williams@intel.com> wrote:
>>> On Thu, Mar 3, 2016 at 9:30 AM, Ard Biesheuvel
>>> <ard.biesheuvel@linaro.org> wrote:
>>>> On 3 March 2016 at 18:27, Dan Williams <dan.j.williams@intel.com> wrote:
>>>>> On Thu, Mar 3, 2016 at 9:16 AM, Ard Biesheuvel
>>>>> <ard.biesheuvel@linaro.org> wrote:
>>>>>> On 3 March 2016 at 18:13, Russell King - ARM Linux
>>>>>> <linux@arm.linux.org.uk> wrote:
>>>>>>> On Thu, Mar 03, 2016 at 05:24:41PM +0100, Ard Biesheuvel wrote:
>>>>>>>> If there are no remaining objections, may I suggest that Dan takes
>>>>>>>> patch #1 and #2, and that I put the third patch into Russell's patch
>>>>>>>> system? The only strict ordering requirement is that #1 is merged
>>>>>>>> before #2 and #3 both hit mainline, and that is solved by taking #1
>>>>>>>> and #2 in order.
>>>>>>>
>>>>>>> How does the dependency between 1 and 3 get satisfied?  Your comment
>>>>>>> makes no sense to me.
>>>>>>>
>>>>>>
>>>>>> Patch #3 introduces a function arch_memremap_wb() on the ARM side
>>>>>> which will never get called unless patch #2 is also merged. Once both
>>>>>> #2 and #3 are in, the memremap() call that #1 reverts will use
>>>>>> MT_MEMORY_RW attributes rather than MT_DEVICE_CACHED, so #1 needs to
>>>>>> go first, but #2 and #3 can go in in either order.
>>>>>>
>>>>>> In other words, there are no build dependencies between the patches,
>>>>>> but to prevent pxa2xx-flash from using MT_MEMORY_RW attributes, it
>>>>>> needs to go in before #2
>>>>>
>>>>> Can we add a new MEMREMAP_ type for this case so that we don't need to
>>>>> keep carrying the confusing ioremap_cache() which means different
>>>>> things to different archs and silently falls back to uncached on some
>>>>> archs.
>>>>
>>>> Actually, I don't think so. Unifying ioremap_cache() between
>>>> architectures was a mistake to begin with, since I/O mapping
>>>> attributes vary so wildly between architectures, and there are very
>>>> few drivers that depend on such specific behavior that actually need
>>>> to be built for multiple architectures.
>>>>
>>>> So I think your memremap() is a fantastic idea, considering the
>>>> widespread abuse of ioremap_cache(), also in common code. But removing
>>>> it altogether to replace it with something that looks generic but
>>>> never is, is just repeating the same mistake imo.
>>>
>>> I'm not suggesting it be a generic flag.  I.e. something like
>>> MEMREMAP_ARM_ that explicitly documents that this driver has arch
>>> specific mapping dependencies.  That request type will fail on any
>>> arch that does not implement MEMREMAP_ARM_ support.
>>
>> I don't see the point of adding this to a generic API. ioremap_cache()
>> is used to perform I/O, even if it has memory semantics on read.
>> Getting rid of the abuse, where ioremap_cache() is used to map system
>> RAM to access ACPI tables etc is something that we should fix, yes,
>> but arch specific I/O needs to stay out of the picture imo, simply
>> because there is no problem to fix here.
>
> Hmm, then what about Russell's suggestoin, if I'm not
> mischaracterizing, that ioremap_cache() move to its old
> ioremap_cached() name.  The latter has less cross-arch confusion.

That would be appropriate as soon as we get rid of all the abuses of
ioremap_cache() (for some of which I am responsible myself, in the ARM
UEFI code). If we change it now, we're likely to break stuff.

-- 
Ard.

[PATCH v2 0/3] fix memremap on ARM

[Dan Williams]

On Thu, Mar 3, 2016 at 9:57 AM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On 3 March 2016 at 18:55, Dan Williams <dan.j.williams@intel.com> wrote:
>> On Thu, Mar 3, 2016 at 9:41 AM, Ard Biesheuvel
[..]
>> Hmm, then what about Russell's suggestoin, if I'm not
>> mischaracterizing, that ioremap_cache() move to its old
>> ioremap_cached() name.  The latter has less cross-arch confusion.
>
> That would be appropriate as soon as we get rid of all the abuses of
> ioremap_cache() (for some of which I am responsible myself, in the ARM
> UEFI code). If we change it now, we're likely to break stuff.
>

Ok, sounds good.  I'll take care of the x86 ioremap_cache() removal
and leave the eventual ARM conversion to you.

[PATCH v2 0/3] fix memremap on ARM

[Ard Biesheuvel]

On 3 March 2016 at 19:01, Dan Williams <dan.j.williams@intel.com> wrote:
> On Thu, Mar 3, 2016 at 9:57 AM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>> On 3 March 2016 at 18:55, Dan Williams <dan.j.williams@intel.com> wrote:
>>> On Thu, Mar 3, 2016 at 9:41 AM, Ard Biesheuvel
> [..]
>>> Hmm, then what about Russell's suggestoin, if I'm not
>>> mischaracterizing, that ioremap_cache() move to its old
>>> ioremap_cached() name.  The latter has less cross-arch confusion.
>>
>> That would be appropriate as soon as we get rid of all the abuses of
>> ioremap_cache() (for some of which I am responsible myself, in the ARM
>> UEFI code). If we change it now, we're likely to break stuff.
>>
>
> Ok, sounds good.  I'll take care of the x86 ioremap_cache() removal
> and leave the eventual ARM conversion to you.


OK. But I'd still like Russell to indicate whether he agrees with all
of this, i.e., wire up memremap() to perform MT_MEMORY_RW mappings in
the vmalloc area of memory regions that may be potentially be covered
by highmem as well.

[PATCH v2 0/3] fix memremap on ARM

[Russell King - ARM Linux]

On Thu, Mar 03, 2016 at 07:07:05PM +0100, Ard Biesheuvel wrote:
> OK. But I'd still like Russell to indicate whether he agrees with all
> of this, i.e., wire up memremap() to perform MT_MEMORY_RW mappings in
> the vmalloc area of memory regions that may be potentially be covered
> by highmem as well.

I'm happy to go that way: go back to ioremap_cached() for pxa2xx-flash
and switch memremap() to MT_MEMORY_RW - I think that makes total sense.

I think it is worth putting a comment against ioremap_cached() saying
that it's for pxa2xx-flash.c though, and deprecated for new uses.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH v2 0/3] fix memremap on ARM

[Ard Biesheuvel]

On 3 March 2016 at 20:36, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Mar 03, 2016 at 07:07:05PM +0100, Ard Biesheuvel wrote:
>> OK. But I'd still like Russell to indicate whether he agrees with all
>> of this, i.e., wire up memremap() to perform MT_MEMORY_RW mappings in
>> the vmalloc area of memory regions that may be potentially be covered
>> by highmem as well.
>
> I'm happy to go that way: go back to ioremap_cached() for pxa2xx-flash
> and switch memremap() to MT_MEMORY_RW - I think that makes total sense.
>
> I think it is worth putting a comment against ioremap_cached() saying
> that it's for pxa2xx-flash.c though, and deprecated for new uses.
>

OK, I will respin the series, and replace patch #1 with two patches that

a) reintroduce ioremap_cached() (with the comment, as you say)
b) move pxa2xx-flash from memremap to ioremap_cached()