Re: [PATCH] ARM: allow, but warn, when issuing ioremap() on RAM

[Felipe Contreras]

On Mon, Oct 11, 2010 at 2:23 PM, Catalin Marinas
<catalin.marinas@xxxxxxx> wrote:
> On Mon, 2010-10-11 at 11:39 +0100, Felipe Contreras wrote:
>> On Mon, Oct 11, 2010 at 1:05 PM, Catalin Marinas
>> <catalin.marinas@xxxxxxx> wrote:
>> > We could be a bit more flexible as a temporary solution. But that's a
>> > hack and doesn't guarantee the attributes that the driver requested. If
>> > the driver would use writel/readl, that's not too bad since we pushed
>> > explicit barriers in these macros.
>> >
>> > It would need to be improved to invalidate the corresponding cache lines
>> > (using the DMA API?) but it would look even worse.
>> >
>> >
>> > diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
>> > index 6bdf42c..082bbd0 100644
>> > --- a/arch/arm/mm/ioremap.c
>> > +++ b/arch/arm/mm/ioremap.c
>> > @@ -204,10 +204,13 @@ void __iomem * __arm_ioremap_pfn_caller(unsigned long pfn,
>> > Â#endif
>> >
>> > Â Â Â Â/*
>> > - Â Â Â Â* Don't allow RAM to be mapped - this causes problems with ARMv6+
>> > + Â Â Â Â* Don't allow RAM to be mapped as Device memory - this causes
>> > + Â Â Â Â* problems with ARMv6+
>> > Â Â Â Â */
>> > Â Â Â Âif (WARN_ON(pfn_valid(pfn)))
>> > - Â Â Â Â Â Â Â return NULL;
>> > + Â Â Â Â Â Â Â if (__LINUX_ARM_ARCH__ >= 6 &&
>> > + Â Â Â Â Â Â Â Â Â (mtype != MT_DEVICE_CACHED || mtype != MT_DEVICE_WC))
>> > + Â Â Â Â Â Â Â Â Â Â Â mtype = MT_DEVICE_WC;
>> >
>> > Â Â Â Âtype = get_mem_type(mtype);
>> > Â Â Â Âif (!type)
>>
>> I will try that, but from what I can see this might still break some
>> drivers, right?
>
> Probably, it depends on how drivers use the memory returned by ioremap
> and the assumptions they make about the memory type (strict ordering,
> write buffers, speculation). I think such memory should be used via the
> I/O accessors and not directly, so you get the benefit of explicit
> memory barriers. If not, you have to cope with the loss of attributes
> because of the WC mapping.
>
> But as it has been said (and it's my opinion as well), the drivers are
> broken and are misusing the ioremap API.

Indeed, that is understood by everyone.

But what I think should happen on .36 is that the warning is on, and
the behavior doesn't change compared to .35. Your patch is changing
the behavior as you say in hacky way, which might have unintended
consequences in the affected drivers, I don't think that's ideal,
although it's better than breaking them completely.

>> Anyway, I'm reading the TRM and I can't find any mention of this.
>> Catalin, can you point out where is this mentioned, and also, can you
>> confirm if this would affect only the memory that has the double
>> mapping, or it can corrupt other memory as well?
>
> That's in the ARM ARM (Architecture Reference Manual), not a TRM.
>
> You never know what "unpredictable" means and this is specific to the
> processor implementation, not the ARM ARM. You should not mix different
> memory types for the same page.

Certainly, but saying corruption _will_ happen elsewhere is not
correct, there is no empirical evidence for that, although
theoretically it might happen.

Considering that, and the fact that .35 is not considered to be
horribly broken (or any of the previous releases), then having one
more releases with this broken behavior as a grace period wouldn't
hurt anybody.

> Back in 2006 there was a discussion for a set of palliative measures in
> hardware (initially until the OSes are fixed) and these allow the same
> memory type but with different cacheability attributes (e.g. Normal
> Non-cacheable vs Normal Cacheable) given that care is taken in software
> wrt stale cache lines and speculation (but at least you don't get random
> corruption somewhere else). They would need to have the same
> shareability attributes though.
>
> With the hack above, MT_DEVICE_WC is actually Normal Non-cacheable
> memory. It needs cache flushing but it's not worse than the original
> ioremap allowing this. The driver may not work as expected though.

I see, but are you saying that if the shareability is not the same,
then corruption elsewhere might actually happen? Or it would happen
most likely on the affected region? If it's the former, then I agree
that your patch should be used instead.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/