[PATCH v2 0/3] fix memremap on ARM

[Ard Biesheuvel]

On 3 March 2016 at 18:55, Dan Williams <dan.j.williams at intel.com> wrote:
> On Thu, Mar 3, 2016 at 9:41 AM, Ard Biesheuvel
> <ard.biesheuvel at linaro.org> wrote:
>> On 3 March 2016 at 18:33, Dan Williams <dan.j.williams at intel.com> wrote:
>>> On Thu, Mar 3, 2016 at 9:30 AM, Ard Biesheuvel
>>> <ard.biesheuvel at linaro.org> wrote:
>>>> On 3 March 2016 at 18:27, Dan Williams <dan.j.williams at intel.com> wrote:
>>>>> On Thu, Mar 3, 2016 at 9:16 AM, Ard Biesheuvel
>>>>> <ard.biesheuvel at linaro.org> wrote:
>>>>>> On 3 March 2016 at 18:13, Russell King - ARM Linux
>>>>>> <linux at 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.