Re: [RFC PATCH 1/3] pstore-ram: use write-combine mappings

[Rob Herring]

On 04/09/2013 10:53 PM, Colin Cross wrote:
> On Tue, Apr 9, 2013 at 8:08 PM, Rob Herring <robherring2@xxxxxxxxx> wrote:
>> From: Rob Herring <rob.herring@xxxxxxxxxxx>
>>
>> Atomic operations are undefined behavior on ARM for device or strongly
>> ordered memory types. So use write-combine variants for mappings. This
>> corresponds to normal, non-cacheable memory on ARM. For many other
>> architectures, this change should not change the mapping type.
> 
> This is going to make ramconsole less reliable.  A debugging printk
> followed by a __raw_writel that causes an immediate hard crash is
> likely to lose the last updates, including the most useful message, in
> the write buffers.

It would have to be a write that hangs the bus. In my experience with
AXI, the bus doesn't actually hang until you hit max outstanding
transactions.

I think exclusive stores will limit the buffering, but that is probably
not architecturally guaranteed.

I could put a wb() in at the end of persistent_ram_write.

> Also, isn't this patch unnecessary after patch 3 in this set?

It is still needed in the main memory case to be architecturally correct
to avoid multiple mappings of different memory types and exclusive
accesses to device memory. At least on an A9, it doesn't really seem to
matter. I could remove this for the ioremap case.

Rob

>> Signed-off-by: Rob Herring <rob.herring@xxxxxxxxxxx>
>> Cc: Anton Vorontsov <cbouatmailru@xxxxxxxxx>
>> Cc: Colin Cross <ccross@xxxxxxxxxxx>
>> Cc: Kees Cook <keescook@xxxxxxxxxxxx>
>> Cc: Tony Luck <tony.luck@xxxxxxxxx>
>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>> ---
>>  fs/pstore/ram_core.c |    4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
>> index 0306303..e126d9f 100644
>> --- a/fs/pstore/ram_core.c
>> +++ b/fs/pstore/ram_core.c
>> @@ -337,7 +337,7 @@ static void *persistent_ram_vmap(phys_addr_t start, size_t size)
>>         page_start = start - offset_in_page(start);
>>         page_count = DIV_ROUND_UP(size + offset_in_page(start), PAGE_SIZE);
>>
>> -       prot = pgprot_noncached(PAGE_KERNEL);
>> +       prot = pgprot_writecombine(PAGE_KERNEL);
> Is this necessary?  Won't pgprot_noncached already be normal memory?
> 
>>         pages = kmalloc(sizeof(struct page *) * page_count, GFP_KERNEL);
>>         if (!pages) {
>> @@ -364,7 +364,7 @@ static void *persistent_ram_iomap(phys_addr_t start, size_t size)
>>                 return NULL;
>>         }
>>
>> -       return ioremap(start, size);
>> +       return ioremap_wc(start, size);
> 
> ioremap_wc corresponds to MT_DEVICE_WC, which is still device memory,
> so I don't see how this helps solve the problem in the commit message.
> 
>>  }
>>
>>  static int persistent_ram_buffer_map(phys_addr_t start, phys_addr_t size,
>> --
>> 1.7.10.4
>>

--
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/