Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933654AbcKPSmL (ORCPT ); Wed, 16 Nov 2016 13:42:11 -0500 Received: from mail-ua0-f173.google.com ([209.85.217.173]:36194 "EHLO mail-ua0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932177AbcKPSmH (ORCPT ); Wed, 16 Nov 2016 13:42:07 -0500 MIME-Version: 1.0 In-Reply-To: <20161116151630.3235006-1-arnd@arndb.de> References: <20161116151630.3235006-1-arnd@arndb.de> From: Joel Fernandes Date: Wed, 16 Nov 2016 10:42:05 -0800 Message-ID: Subject: Re: [PATCH] pstore: fix warning about conditional locking To: Arnd Bergmann Cc: Anton Vorontsov , Colin Cross , Kees Cook , Tony Luck , Namhyung Kim , Geliang Tang , LKML Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id uAGIgItI005010 Content-Length: 5757 Lines: 155 Hi Arnd, On Wed, Nov 16, 2016 at 7:15 AM, Arnd Bergmann 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 > --- > 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 > #include > > -/* > - * 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 >