Re: [PATCH] pstore: fix warning about conditional locking

[Joel Fernandes]

Hi Arnd,

On Wed, Nov 16, 2016 at 7:15 AM, Arnd Bergmann <arnd@xxxxxxxx> 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@xxxxxxxx>
> ---
>  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
>