[lkml]   [2016]   [Aug]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: Problem with atomic accesses in pstore on some ARM CPUs
On Tue, Aug 16, 2016 at 1:50 PM, Kees Cook <> wrote:

[ ... ]

>>> persistent_ram uses atomic ops in uncached memory to store the start
>>> and end positions in the ringbuffer so that the state of the
>>> ringbuffer will be valid if the kernel crashes at any time. This was
>>> inherited from Android's ram_console implementation, and worked
>>> through armv7. It has been causing more and more problems recently,
>>> see for example 027bc8b08242c59e19356b4b2c189f2d849ab660 (pstore-ram:
>>> Allow optional mapping with pgprot_noncached) and
>>> 7ae9cb81933515dc7db1aa3c47ef7653717e3090 (pstore-ram: Fix hangs by
>>> using write-combine mappings).
>>> Maybe it should be replaced with a spinlock in normal ram protecting
>>> writes to the uncached region.
>> The necessary functions already exist, and are used for memory mapped
>> with ioremap() / ioremap_wc(). They were introduced with commit
>> 0405a5cec3 ("pstore/ram: avoid atomic accesses for ioremapped
>> regions"), and the description in that patch sounds quite similar to
>> the current problem. Given that, would it be acceptable to remove
>> buffer_start_add_atomic() and buffer_size_add_atomic(), and always use
>> buffer_start_add_locked() and buffer_size_add_locked() instead ? Those
>> functions still use atomic_set() and atomic_read(), which works fine
>> in my tests. The only difference is that a spinlock in main memory is
>> used instead of atomic_cmpxchg().
> I don't see much of a down side to this. ramoops isn't expected to be
> high-bandwidth so trading for a single global lock doesn't really
> bother me.

Sounds good. I'll submit a patch to address the problem as suggested above.


 \ /
  Last update: 2016-09-17 09:57    [W:0.052 / U:1.744 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site