Problem with atomic accesses in pstore on some ARM CPUs

[Tony Lindgren]

* Guenter Roeck <groeck at google.com> [160816 17:27]:
> On Tue, Aug 16, 2016 at 1:50 PM, Kees Cook <keescook at chromium.org> 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.

Yeah seems like it simplifies things quite a bit for us.

Tony