[2/5] pstore: Introduce write_buf backend callback

For function tracing we need to stop using pstore.buf directly, since
in a tracing callback we can't use spinlocks, and thus we can't safely
use the global buffer.

With write_buf callback, backends no longer need to access pstore.buf
directly, and thus we can pass any buffers (e.g. allocated on stack).

Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>
---
 fs/pstore/platform.c   |   10 ++++++++++
 include/linux/pstore.h |    4 ++++
 2 files changed, 14 insertions(+)

On Sat, May 26, 2012 at 6:39 AM, Anton Vorontsov
<anton.vorontsov@linaro.org> wrote:
> For function tracing we need to stop using pstore.buf directly, since
> in a tracing callback we can't use spinlocks, and thus we can't safely
> use the global buffer.
>
> With write_buf callback, backends no longer need to access pstore.buf
> directly, and thus we can pass any buffers (e.g. allocated on stack).

Hrm, I thought the point of having pstore.buf pre-mapped was to allow
Oopses to be able to write directly to it without needing to hit any
additional kernel code. Maybe I'm misunderstanding this change,
though. I'd like to see Tony's opinion on it.

-Kees

> Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>
> ---
>  fs/pstore/platform.c   |   10 ++++++++++
>  include/linux/pstore.h |    4 ++++
>  2 files changed, 14 insertions(+)
>
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index be4614f..e7c0a52 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -198,6 +198,14 @@ static void pstore_register_console(void)
>  static void pstore_register_console(void) {}
>  #endif
>
> +static int pstore_write_compat(enum pstore_type_id type,
> +                              enum kmsg_dump_reason reason,
> +                              u64 *id, unsigned int part,
> +                              size_t size, struct pstore_info *psi)
> +{
> +       return psi->write_buf(type, reason, id, part, psinfo->buf, size, psi);
> +}
> +
>  /*
>  * platform specific persistent storage driver registers with
>  * us here. If pstore is already mounted, call the platform
> @@ -222,6 +230,8 @@ int pstore_register(struct pstore_info *psi)
>                return -EINVAL;
>        }
>
> +       if (!psi->write)
> +               psi->write = pstore_write_compat;
>        psinfo = psi;
>        mutex_init(&psinfo->read_mutex);
>        spin_unlock(&pstore_lock);
> diff --git a/include/linux/pstore.h b/include/linux/pstore.h
> index 1bd014b..b107484 100644
> --- a/include/linux/pstore.h
> +++ b/include/linux/pstore.h
> @@ -48,6 +48,10 @@ struct pstore_info {
>        int             (*write)(enum pstore_type_id type,
>                        enum kmsg_dump_reason reason, u64 *id,
>                        unsigned int part, size_t size, struct pstore_info *psi);
> +       int             (*write_buf)(enum pstore_type_id type,
> +                       enum kmsg_dump_reason reason, u64 *id,
> +                       unsigned int part, const char *buf, size_t size,
> +                       struct pstore_info *psi);
>        int             (*erase)(enum pstore_type_id type, u64 id,
>                        struct pstore_info *psi);
>        void            *data;
> --
> 1.7.9.2
>

> Hrm, I thought the point of having pstore.buf pre-mapped was to allow
> Oopses to be able to write directly to it without needing to hit any
> additional kernel code. Maybe I'm misunderstanding this change,
> though. I'd like to see Tony's opinion on it.

Yes - the ERST backend needs to have a bunch of header ugliness (with
special UUIDs) at the front of the buffer that is stored to non-volatile
storage. So it allocates its own buffer with all that junk, and then
passes the address of the plain data portion of the buffer on to the
pstore layer.

As we add more backends, it might be that this is only applicable to
ERST, and so it might make sense to have it copy the data from some
other buffer into its specially crafted one.  But we should not lose
the "no allocations" property ... everything needed should be pre-allocated
so we don't have to try to allocate any memory during a panic.

-Tony

On Tue, May 29, 2012 at 06:00:16PM +0000, Luck, Tony wrote:
> > Hrm, I thought the point of having pstore.buf pre-mapped was to allow
> > Oopses to be able to write directly to it without needing to hit any
> > additional kernel code. Maybe I'm misunderstanding this change,
> > though. I'd like to see Tony's opinion on it.
> 
> Yes - the ERST backend needs to have a bunch of header ugliness (with
> special UUIDs) at the front of the buffer that is stored to non-volatile
> storage. So it allocates its own buffer with all that junk, and then
> passes the address of the plain data portion of the buffer on to the
> pstore layer.
> 
> As we add more backends, it might be that this is only applicable to
> ERST, and so it might make sense to have it copy the data from some
> other buffer into its specially crafted one.  But we should not lose
> the "no allocations" property ... everything needed should be pre-allocated
> so we don't have to try to allocate any memory during a panic.

Yep, and everything is still pre-allocated.

The only change is that we can pass different buffers, and in tracing
case it is allocated on the stack (we can't use pstore.buf for tracing
as it would require grabbing pstore_lock, which we can't do -- the
locking operations are traced too, so it would recurse).

Thanks,