Messages in this thread | | | From | Joel Fernandes <> | Date | Wed, 16 Nov 2016 10:42:05 -0800 | Subject | Re: [PATCH] pstore: fix warning about conditional locking |
| |
Hi Arnd,
On Wed, Nov 16, 2016 at 7:15 AM, Arnd Bergmann <arnd@arndb.de> wrote: > gcc cannot prove that "prz->flags" remains constant over the > function, so it warns that we might restore flags that have > never been saved: > > fs/pstore/ram_core.c: In function ‘buffer_size_add’: > include/linux/spinlock.h:246:3: error: ‘flags’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > > This reworks the code to avoid the spinlock entirely, by actually > using the atomic_t for atomic updates. The resulting code is > more efficient and easier to understand, and it avoids the > local variable that the compiler did not track correctly.
Kees fixed this in his tree by initializing the flags variable.
atomic_cmpxchg is not an option as the memory may be uncached and caused issues, it was previously this way and was replaced with locking.
Thanks, Joel
> > Fixes: 95937ddce59a ("pstore: Allow prz to control need for locking") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > fs/pstore/ram.c | 4 +--- > fs/pstore/ram_core.c | 46 ++++++++++++++++++---------------------------- > include/linux/pstore_ram.h | 8 -------- > 3 files changed, 19 insertions(+), 39 deletions(-) > > diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c > index 380222432eff..cb66ba710349 100644 > --- a/fs/pstore/ram.c > +++ b/fs/pstore/ram.c > @@ -787,9 +787,7 @@ static int ramoops_probe(struct platform_device *pdev) > : 1; > err = ramoops_init_przs("ftrace", dev, cxt, &cxt->fprzs, &paddr, > cxt->ftrace_size, -1, > - &cxt->max_ftrace_cnt, LINUX_VERSION_CODE, > - (cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU) > - ? PRZ_FLAG_NO_LOCK : 0); > + &cxt->max_ftrace_cnt, LINUX_VERSION_CODE, 0); > if (err) > goto fail_init_fprz; > > diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c > index 0cc23cb18719..87be7bdfaf48 100644 > --- a/fs/pstore/ram_core.c > +++ b/fs/pstore/ram_core.c > @@ -51,21 +51,17 @@ static inline size_t buffer_start(struct persistent_ram_zone *prz) > /* increase and wrap the start pointer, returning the old value */ > static size_t buffer_start_add(struct persistent_ram_zone *prz, size_t a) > { > - int old; > - int new; > - unsigned long flags; > - > - if (!(prz->flags & PRZ_FLAG_NO_LOCK)) > - raw_spin_lock_irqsave(&prz->buffer_lock, flags); > + int old, new, before; > > old = atomic_read(&prz->buffer->start); > - new = old + a; > - while (unlikely(new >= prz->buffer_size)) > - new -= prz->buffer_size; > - atomic_set(&prz->buffer->start, new); > + do { > + new = old + a; > + while (unlikely(new >= prz->buffer_size)) > + new -= prz->buffer_size; > > - if (!(prz->flags & PRZ_FLAG_NO_LOCK)) > - raw_spin_unlock_irqrestore(&prz->buffer_lock, flags); > + before = old; > + old = atomic_cmpxchg(&prz->buffer->start, before, new); > + } while (old != before); > > return old; > } > @@ -73,25 +69,20 @@ static size_t buffer_start_add(struct persistent_ram_zone *prz, size_t a) > /* increase the size counter until it hits the max size */ > static void buffer_size_add(struct persistent_ram_zone *prz, size_t a) > { > - size_t old; > - size_t new; > - unsigned long flags; > - > - if (!(prz->flags & PRZ_FLAG_NO_LOCK)) > - raw_spin_lock_irqsave(&prz->buffer_lock, flags); > + size_t old, new, before; > > old = atomic_read(&prz->buffer->size); > - if (old == prz->buffer_size) > - goto exit; > + do { > + if (old == prz->buffer_size) > + break; > > - new = old + a; > - if (new > prz->buffer_size) > - new = prz->buffer_size; > - atomic_set(&prz->buffer->size, new); > + new = old + a; > + if (new > prz->buffer_size) > + new = prz->buffer_size; > > -exit: > - if (!(prz->flags & PRZ_FLAG_NO_LOCK)) > - raw_spin_unlock_irqrestore(&prz->buffer_lock, flags); > + before = old; > + old = atomic_cmpxchg(&prz->buffer->size, before, new); > + } while (old != before); > } > > static void notrace persistent_ram_encode_rs8(struct persistent_ram_zone *prz, > @@ -496,7 +487,6 @@ static int persistent_ram_post_init(struct persistent_ram_zone *prz, u32 sig, > > prz->buffer->sig = sig; > persistent_ram_zap(prz); > - prz->buffer_lock = __RAW_SPIN_LOCK_UNLOCKED(buffer_lock); > prz->flags = flags; > > return 0; > diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h > index 9395f06e8372..e755803ba58a 100644 > --- a/include/linux/pstore_ram.h > +++ b/include/linux/pstore_ram.h > @@ -24,13 +24,6 @@ > #include <linux/list.h> > #include <linux/types.h> > > -/* > - * Choose whether access to the RAM zone requires locking or not. If a zone > - * can be written to from different CPUs like with ftrace for example, then > - * PRZ_FLAG_NO_LOCK is used. For all other cases, locking is required. > - */ > -#define PRZ_FLAG_NO_LOCK BIT(0) > - > struct persistent_ram_buffer; > struct rs_control; > > @@ -48,7 +41,6 @@ struct persistent_ram_zone { > struct persistent_ram_buffer *buffer; > size_t buffer_size; > u32 flags; > - raw_spinlock_t buffer_lock; > > /* ECC correction */ > char *par_buffer; > -- > 2.9.0 >
| |