Merge ramoops and persistent_ram, generic pstore RAM backend

This is a longstanding bug, almost unnoticeable when calling persistent_ram_write() for small buffers.

But when called for large data buffers, the write routine behaves incorrectly, as the size may never update: instead of clamping the size to the maximum buffer size, buffer_size_add_clamp() returns an error (which is never checked by the write routine, btw).

To fix this, we now use buffer_size_add() that actually clamps the size to the max value.

Also remove buffer_size_add_clamp(), it is no longer needed.

Quoted 8 lines · Anton Vorontsov

This is a longstanding bug, almost unnoticeable when calling persistent_ram_write() for small buffers.

But when called for large data buffers, the write routine behaves incorrectly, as the size may never update: instead of clamping the size to the maximum buffer size, buffer_size_add_clamp() returns an error (which is never checked by the write routine, btw).

Quoted 5 lines

To fix this, we now use buffer_size_add() that actually clamps the size to the max value.

Also remove buffer_size_add_clamp(), it is no longer needed.

Say if you did notice it, what would that look like? It's just that something gets lost instead of written to the screen right?

This is a longstanding bug, almost unnoticeable when calling persistent_ram_write() for small buffers.

But when called for large data buffers, the write routine behaves incorrectly, as the size may never update: instead of clamping the size to the maximum buffer size, buffer_size_add_clamp() returns an error (which is never checked by the write routine, btw).

To fix this, we now use buffer_size_add() that actually clamps the size to the max value.

Also remove buffer_size_add_clamp(), it is no longer needed.

Signed-off-by Anton Vorontsov <[email protected]>

---
 drivers/staging/android/persistent_ram.c |   19 +------------------

 1 file changed, 1 insertion(+), 18 deletions(-)

drivers/staging/android/persistent_ram.c +1 -19

index 12444fd..13a12bc 100644
@@ -79,23 +79,6 @@ static inline void buffer_size_add(struct persistent_ram_zone *prz, size_t a)
        } while (atomic_cmpxchg(&prz->buffer->size, old, new) != old);
  }
 
-/* increase the size counter, retuning an error if it hits the max size */
-static inline ssize_t buffer_size_add_clamp(struct persistent_ram_zone *prz,
-       size_t a)
-{
-       size_t old;
-       size_t new;
-
-       do {
-               old = atomic_read(&prz->buffer->size);
-               new = old + a;
-               if (new > prz->buffer_size)
-                       return -ENOMEM;
-       } while (atomic_cmpxchg(&prz->buffer->size, old, new) != old);
-
-       return 0;
-}
-
  static void notrace persistent_ram_encode_rs8(struct persistent_ram_zone *prz,
        uint8_t *data, size_t len, uint8_t *ecc)
  {

@@ -300,7 +283,7 @@ int notrace persistent_ram_write(struct persistent_ram_zone *prz,
                c = prz->buffer_size;
        }
 
-       buffer_size_add_clamp(prz, c);
+       buffer_size_add(prz, c);
 
        start = buffer_start_add(prz, c);
 
--

1.7.9.2

Quoted 55 lines · Anton Vorontsov

This is a longstanding bug, almost unnoticeable when calling persistent_ram_write() for small buffers.

But when called for large data buffers, the write routine behaves incorrectly, as the size may never update: instead of clamping the size to the maximum buffer size, buffer_size_add_clamp() returns an error (which is never checked by the write routine, btw).

To fix this, we now use buffer_size_add() that actually clamps the size to the max value.

Also remove buffer_size_add_clamp(), it is no longer needed.

Signed-off-by Anton Vorontsov <[email protected]>

---
 drivers/staging/android/persistent_ram.c |   19 +------------------

 1 file changed, 1 insertion(+), 18 deletions(-)

drivers/staging/android/persistent_ram.c +1 -18

index 12444fd..13a12bc 100644
@@ -79,23 +79,6 @@ static inline void buffer_size_add(struct persistent_ram_zone *prz, size_t a)
        } while (atomic_cmpxchg(&prz->buffer->size, old, new) != old);
  }
 
-/* increase the size counter, retuning an error if it hits the max size */
-static inline ssize_t buffer_size_add_clamp(struct persistent_ram_zone *prz,
-       size_t a)
-{
-       size_t old;
-       size_t new;
-
-       do {
-               old = atomic_read(&prz->buffer->size);
-               new = old + a;
-               if (new > prz->buffer_size)
-                       return -ENOMEM;
-       } while (atomic_cmpxchg(&prz->buffer->size, old, new) != old);
-
-       return 0;
-}
-
  static void notrace persistent_ram_encode_rs8(struct persistent_ram_zone *prz,
        uint8_t *data, size_t len, uint8_t *ecc)
  {

@@ -300,7 +283,7 @@ int notrace persistent_ram_write(struct persistent_ram_zone *prz,
                c = prz->buffer_size;
        }
 
-       buffer_size_add_clamp(prz, c);
+       buffer_size_add(prz, c);
 
        start = buffer_start_add(prz, c);
 

Quoted 3 lines

--
1.7.9.2

Acked-by Colin Cross <[email protected]>

This is a bug fix for a bug introduced in 3.4-rc1, it should go into 3.4 if there is another rc.

Hi all,

There are currently two competing debug facilities to store kernel messages in a persistent storage: a generic pstore and Google's persistent_ram. Not so long ago (https://lkml.org/lkml/2012/3/8/252),
it was decided that we should fix this situation.

Recently ramoops has switched to pstore, which basically means that it became a RAM backend for the pstore framework.

persistent_ram+ram_console and ramoops+pstore have almost the same features, except:

1. Ramoops doesn't support ECC. Having ECC is useful when a hardware reset was used to bring the machine back to life (i.e. a watchdog triggered). In such cases, RAM may be somewhat corrupt, but usually it is restorable.

2. Pstore doesn't support logging kernel messages in run-time, it only dumps dmesg when kernel oopses/panics. This makes pstore useless for debugging hangs caused by HW issues or improper use of HW (e.g. weird device inserted -> driver tried to write a reserved bits -> SoC hanged. In that case we don't get any messages in the pstore.

These patches solve the first issue, plus move things to their proper places. Patches that will fix the second issue are pending.

Quoted 18 lines · Anton Vorontsov

Hi all,

There are currently two competing debug facilities to store kernel messages in a persistent storage: a generic pstore and Google's persistent_ram. Not so long ago (https://lkml.org/lkml/2012/3/8/252),
it was decided that we should fix this situation.

Recently ramoops has switched to pstore, which basically means that it became a RAM backend for the pstore framework.

persistent_ram+ram_console and ramoops+pstore have almost the same features, except:

1. Ramoops doesn't support ECC. Having ECC is useful when a hardware reset was used to bring the machine back to life (i.e. a watchdog triggered). In such cases, RAM may be somewhat corrupt, but usually it is restorable.

Quoted 8 lines

2. Pstore doesn't support logging kernel messages in run-time, it only dumps dmesg when kernel oopses/panics. This makes pstore useless for debugging hangs caused by HW issues or improper use of HW (e.g. weird device inserted -> driver tried to write a reserved bits -> SoC hanged. In that case we don't get any messages in the pstore.

These patches solve the first issue, plus move things to their proper places. Patches that will fix the second issue are pending.

I've applied the first 7 patches, as they were localized to the drivers/staging/android/ directory, but in order for me to apply the rest, I need acks from the respective subsystem maintainers.

pstore developers, what do you say about these changes, are you ok with them?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Anton! Is it safe to assume you are planning to cover the second feature as well, in which case I can drop my plans to get this work done.

On Mon, May 14, 2012 at 10:30:22AM -0600, Shuah Khan wrote:
[...]

Quoted 2 lines

Anton! Is it safe to assume you are planning to cover the second feature as well, in which case I can drop my plans to get this work done.

Yep, absolutely. I'm fully committed to add runtime logging support to pstore. Actually, I'll post patches pretty soon.

Thanks!

Since ramoops was converted to pstore, it has nothing to do with character devices nowadays. Instead, today it is just a RAM backend for pstore.

The patch just moves things around. There are a few changes were needed because of the move:

1. Kconfig and Makefiles fixups, of course.

2. In pstore/ram.c we have to play a bit with MODULE_PARAM_PREFIX, this is needed to keep user experience the same as with ramoops driver (i.e. so that ramoops.foo kernel command line arguments would still work).

Signed-off-by Anton Vorontsov <[email protected]>

This is a first step for adding ECC support for pstore RAM backend: we will use the persistent_ram routines, kindly provided by Google.

Basically, persistent_ram is a set of helper routines to deal with the [optionally] ECC-protected persistent ram regions.

A bit of Makefile, Kconfig and header files adjustments were needed because of the move.

Signed-off-by Anton Vorontsov <[email protected]>

 include/linux/pstore_ram.h               |   86 ++++-

The patch switches pstore RAM backend to use persistent_ram routines, one step closer to the ECC support.

Signed-off-by Anton Vorontsov <[email protected]>

---
 fs/pstore/ram.c |  109 ++++++++++++++++++++++++++++++-------------------------

 1 file changed, 60 insertions(+), 49 deletions(-)

fs/pstore/ram.c +30 -25

index b26b58e..cf0ad92 100644
@@ -62,7 +62,7 @@ MODULE_PARM_DESC(dump_oops,
                "set to 1 to dump oopses, 0 to only dump panics (default 1)");
 
  struct ramoops_context {
-       void *virt_addr;
+       struct persistent_ram_zone **przs;
        phys_addr_t phys_addr;
        unsigned long size;
        size_t record_size;

@@ -90,39 +90,56 @@ static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,
                                   struct pstore_info *psi)
  {
        ssize_t size;
-       char *rambuf;
        struct ramoops_context *cxt = psi->data;
+       struct persistent_ram_zone *prz;
 
        if (cxt->read_count >= cxt->max_count)
                return -EINVAL;
+
        *id = cxt->read_count++;
+       prz = cxt->przs[*id];
+
        /* Only supports dmesg output so far. */
        *type = PSTORE_TYPE_DMESG;
        /* TODO(kees): Bogus time for the moment. */
        time->tv_sec = 0;
        time->tv_nsec = 0;
 
-       rambuf = cxt->virt_addr + (*id * cxt->record_size);
-       size = strnlen(rambuf, cxt->record_size);
+       size = persistent_ram_old_size(prz);
        *buf = kmalloc(size, GFP_KERNEL);
        if (*buf == NULL)
                return -ENOMEM;
-       memcpy(*buf, rambuf, size);
+       memcpy(*buf, persistent_ram_old(prz), size);
 
        return size;
  }
 
+static size_t ramoops_write_kmsg_hdr(struct persistent_ram_zone *prz)
+{
+       char *hdr;
+       struct timeval timestamp;
+       size_t len;
+
+       do_gettimeofday(&timestamp);
+       hdr = kasprintf(GFP_ATOMIC, RAMOOPS_KERNMSG_HDR "%lu.%lu\n",
+               (long)timestamp.tv_sec, (long)timestamp.tv_usec);
+       WARN_ON_ONCE(!hdr);
+       len = hdr ? strlen(hdr) : 0;
+       persistent_ram_write(prz, hdr, len);
+       kfree(hdr);
+
+       return len;
+}
+
  static int ramoops_pstore_write(enum pstore_type_id type,
                                enum kmsg_dump_reason reason,
                                u64 *id,
                                unsigned int part,
                                size_t size, struct pstore_info *psi)
  {
-       char *buf;
-       size_t res;
-       struct timeval timestamp;
        struct ramoops_context *cxt = psi->data;
-       size_t available = cxt->record_size;
+       struct persistent_ram_zone *prz = cxt->przs[cxt->count];
+       size_t hlen;
 
        /* Currently ramoops is designed to only store dmesg dumps. */
        if (type != PSTORE_TYPE_DMESG)

@@ -147,22 +164,10 @@ static int ramoops_pstore_write(enum pstore_type_id type,
        if (part != 1)
                return -ENOSPC;
 
-       buf = cxt->virt_addr + (cxt->count * cxt->record_size);
-
-       res = sprintf(buf, "%s", RAMOOPS_KERNMSG_HDR);
-       buf += res;
-       available -= res;
-
-       do_gettimeofday(&timestamp);
-       res = sprintf(buf, "%lu.%lu\n", (long)timestamp.tv_sec, (long)timestamp.tv_usec);
-       buf += res;
-       available -= res;
-
-       if (size > available)
-               size = available;
-
-       memcpy(buf, cxt->pstore.buf, size);
-       memset(buf + size, '\0', available - size);
+       hlen = ramoops_write_kmsg_hdr(prz);
+       if (size + hlen > prz->buffer_size)
+               size = prz->buffer_size - hlen;
+       persistent_ram_write(prz, cxt->pstore.buf, size);
 
        cxt->count = (cxt->count + 1) % cxt->max_count;
 

@@ -172,14 +177,12 @@ static int ramoops_pstore_write(enum pstore_type_id type,
  static int ramoops_pstore_erase(enum pstore_type_id type, u64 id,
                                struct pstore_info *psi)

fs/pstore/ram.c +1 -0

        return 0;
  }

@@ -200,6 +203,7 @@ static int __init ramoops_probe(struct platform_device *pdev)
        struct ramoops_platform_data *pdata = pdev->dev.platform_data;
        struct ramoops_context *cxt = &oops_cxt;
        int err = -EINVAL;
+       int i;
 
        /* Only a single ramoops area allowed at a time, so fail extra
         * probes.

fs/pstore/ram.c +0 -8

        if (!cxt->pstore.buf) {
                pr_err("cannot allocate pstore buffer\n");
                goto fail_clear;
        }
 
-       if (!request_mem_region(cxt->phys_addr, cxt->size, "ramoops")) {
-               pr_err("request mem region (0x%lx@0x%llx) failed\n",
-                       cxt->size, (unsigned long long)cxt->phys_addr);
-               err = -EINVAL;
-               goto fail_buf;
-       }
-
-       cxt->virt_addr = ioremap(cxt->phys_addr,  cxt->size);

fs/pstore/ram.c +0 -0

        kfree(cxt->pstore.buf);

 fail_clear:
       cxt->pstore.bufsize = 0; cxt->max_count = 0;
+fail_przs:
+       for (i = 0; cxt->przs[i]; i++)
+               persistent_ram_free(cxt->przs[i]);

+       kfree(cxt->przs);
+fail_prz:
+       kfree(cxt->pstore.buf);

 fail_out:
       return err;
 }
--
1.7.9.2

This is now straightforward: just introduce a module parameter and pass the needed value to persistent_ram_new().

Signed-off-by Anton Vorontsov <[email protected]>

---
 fs/pstore/ram.c |   12 +++++++++---

 1 file changed, 9 insertions(+), 3 deletions(-)

fs/pstore/ram.c +9 -4

index cf0ad92..eeb4e32 100644
@@ -61,6 +61,11 @@ module_param(dump_oops, int, 0600);
  MODULE_PARM_DESC(dump_oops,
                "set to 1 to dump oopses, 0 to only dump panics (default 1)");
 
+static int ramoops_ecc;
+module_param_named(ecc, ramoops_ecc, int, 0600);
+MODULE_PARM_DESC(ramoops_ecc,
+               "set to 1 to enable ECC support");
+
  struct ramoops_context {
        struct persistent_ram_zone **przs;
        phys_addr_t phys_addr;

@@ -251,7 +256,7 @@ static int __init ramoops_probe(struct platform_device *pdev)
                size_t sz = cxt->record_size;
                phys_addr_t start = cxt->phys_addr + sz * i;
 
-               cxt->przs[i] = persistent_ram_new(start, sz, 0);
+               cxt->przs[i] = persistent_ram_new(start, sz, ramoops_ecc);
                if (IS_ERR(cxt->przs[i])) {
                        err = PTR_ERR(cxt->przs[i]);
                        pr_err("failed to initialize a prz\n");

@@ -283,9 +288,10 @@ static int __init ramoops_probe(struct platform_device *pdev)
        record_size = pdata->record_size;
        dump_oops = pdata->dump_oops;
 
-       pr_info("attached 0x%lx@0x%llx (%ux0x%zx)\n",
+       pr_info("attached 0x%lx@0x%llx (%ux0x%zx), ecc: %s\n",
                cxt->size, (unsigned long long)cxt->phys_addr,
-               cxt->max_count, cxt->record_size);
+               cxt->max_count, cxt->record_size,
+               ramoops_ecc ? "on" : "off");
 
        return 0;
 
--

1.7.9.2

The routine just creates a persistent ram zone at a specified address.

For persistent_ram_init_ringbuffer() we'd need to add a 'struct persistent_ram' to the global list, and associate it with a device. We don't need all this complexity in pstore_ram, so we introduce the simple function.

Signed-off-by Anton Vorontsov <[email protected]>

---
 drivers/staging/android/persistent_ram.c |   26 ++++++++++++++++++++++++++ drivers/staging/android/persistent_ram.h |    4 ++++

 2 files changed, 30 insertions(+)

drivers/staging/android/persistent_ram.c +26 -0

index ec23822..c0c3d32 100644
@@ -412,6 +412,32 @@ static int __init persistent_ram_post_init(struct persistent_ram_zone *prz, bool
        return 0;
  }
 
+struct persistent_ram_zone * __init persistent_ram_new(phys_addr_t start,
+                                                      size_t size,
+                                                      bool ecc)
+{
+       struct persistent_ram_zone *prz;
+       int ret = -ENOMEM;
+
+       prz = kzalloc(sizeof(struct persistent_ram_zone), GFP_KERNEL);
+       if (!prz) {
+               pr_err("persistent_ram: failed to allocate persistent ram zone\n");
+               goto err;
+       }
+
+       ret = persistent_ram_buffer_map(start, size, prz);
+       if (ret)
+               goto err;
+
+       persistent_ram_post_init(prz, ecc);
+       persistent_ram_update_header_ecc(prz);
+
+       return prz;
+err:
+       kfree(prz);
+       return ERR_PTR(ret);
+}
+

 static  __init
 struct persistent_ram_zone *__persistent_ram_init(struct device *dev, bool ecc) {

drivers/staging/android/persistent_ram.h +4 -1

index 5635355..8154d15 100644
@@ -19,6 +19,7 @@
  #include <linux/kernel.h>
  #include <linux/list.h>
  #include <linux/types.h>
+#include <linux/init.h>
 
  struct persistent_ram_buffer;
 

@@ -62,6 +63,9 @@ struct persistent_ram_zone {
 
  int persistent_ram_early_init(struct persistent_ram *ram);
 
+struct persistent_ram_zone * __init persistent_ram_new(phys_addr_t start,
+                                                      size_t size,
+                                                      bool ecc);
  struct persistent_ram_zone *persistent_ram_init_ringbuffer(struct device *dev,
                bool ecc);
 
--

1.7.9.2

Quoted 28 lines · Anton Vorontsov

Quoted 18 lines

Hi all,

There are currently two competing debug facilities to store kernel messages in a persistent storage: a generic pstore and Google's persistent_ram. Not so long ago (https://lkml.org/lkml/2012/3/8/252),
it was decided that we should fix this situation.

Recently ramoops has switched to pstore, which basically means that it became a RAM backend for the pstore framework.

persistent_ram+ram_console and ramoops+pstore have almost the same features, except:

1. Ramoops doesn't support ECC. Having ECC is useful when a hardware reset was used to bring the machine back to life (i.e. a watchdog triggered). In such cases, RAM may be somewhat corrupt, but usually it is restorable.

Quoted 8 lines

2. Pstore doesn't support logging kernel messages in run-time, it only dumps dmesg when kernel oopses/panics. This makes pstore useless for debugging hangs caused by HW issues or improper use of HW (e.g. weird device inserted -> driver tried to write a reserved bits -> SoC hanged. In that case we don't get any messages in the pstore.

These patches solve the first issue, plus move things to their proper places. Patches that will fix the second issue are pending.

Quoted 6 lines

I've applied the first 7 patches, as they were localized to the drivers/staging/android/ directory, but in order for me to apply the rest, I need acks from the respective subsystem maintainers.

pstore developers, what do you say about these changes, are you ok with them?

Good to see this work get done. Anton beat me to it. :) I have been talking to pstore developers (Tony Luck) and ramoops maintainers (Kees Cook) about this re-architecture work since I first floated this idea on ce-android mailing list. I have been working on this rec-architecture focusing on the second feature "Pstore doesn't support logging kernel messages in run-time" and didn't get to ECC even though it is on my feature list to do bring ramconsole features into ramoops.

Anton! Is it safe to assume you are planning to cover the second feature as well, in which case I can drop my plans to get this work done.

-- Shuah

Quoted 5 lines

thanks,

greg k-h
--

Since ramoops was converted to pstore, it has nothing to do with character devices nowadays. Instead, today it is just a RAM backend for pstore.

The patch just moves things around. There are a few changes were needed because of the move:

1. Kconfig and Makefiles fixups, of course.

2. In pstore/ram.c we have to play a bit with MODULE_PARAM_PREFIX, this is needed to keep user experience the same as with ramoops driver (i.e. so that ramoops.foo kernel command line arguments would still work).

Signed-off-by Anton Vorontsov <[email protected]>

---
 drivers/char/Kconfig       |    9 --
 drivers/char/Makefile      |    1 -
 drivers/char/ramoops.c     |  362 -------------------------------------------
 fs/pstore/Kconfig          |    9 ++
 fs/pstore/Makefile         |    1 +
 fs/pstore/ram.c            |  367 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pstore_ram.h |   17 ++
 include/linux/ramoops.h    |   17 --
 8 files changed, 394 insertions(+), 389 deletions(-)
 delete mode 100644 drivers/char/ramoops.c
 create mode 100644 fs/pstore/ram.c
 create mode 100644 include/linux/pstore_ram.h
 delete mode 100644 include/linux/ramoops.h

drivers/char/Kconfig +0 -9

index fab778d4..ea6f632 100644
@@ -585,15 +585,6 @@ config DEVPORT
 
 source "drivers/s390/char/Kconfig"
 
-config RAMOOPS
-	tristate "Log panic/oops to a RAM buffer"
-	depends on HAS_IOMEM
-	depends on PSTORE
-	default n
-	help
-	  This enables panic and oops messages to be logged to a circular
-	  buffer in RAM where it can be read back at some later point.
-
 config MSM_SMD_PKT
 	bool "Enable device interface for some SMD packet ports"
 	default n

drivers/char/Makefile +0 -1

index 0dc5d7c..d0b27a3 100644
@@ -58,7 +58,6 @@ obj-$(CONFIG_HANGCHECK_TIMER)	+= hangcheck-timer.o
 obj-$(CONFIG_TCG_TPM)		+= tpm/
 
 obj-$(CONFIG_PS3_FLASH)		+= ps3flash.o
-obj-$(CONFIG_RAMOOPS)		+= ramoops.o
 
 obj-$(CONFIG_JS_RTC)		+= js-rtc.o
 js-rtc-y = rtc.o

drivers/char/ramoops.c deleted +0 -362

deleted file mode 100644
index b8b8542..0000000
@@ -1,362 +0,0 @@
-/*
- * RAM Oops/Panic logger
- *
- * Copyright (C) 2010 Marco Stornelli <[email protected]>
- * Copyright (C) 2011 Kees Cook <[email protected]>
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License
- * version 2 as published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful, but
- * WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
- * General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
- * 02110-1301 USA
- *
- */
-
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
-#include <linux/kernel.h>
-#include <linux/err.h>
-#include <linux/module.h>
-#include <linux/pstore.h>
-#include <linux/time.h>
-#include <linux/io.h>
-#include <linux/ioport.h>
-#include <linux/platform_device.h>
-#include <linux/slab.h>
-#include <linux/ramoops.h>
-
-#define RAMOOPS_KERNMSG_HDR "===="
-#define MIN_MEM_SIZE 4096UL
-
-static ulong record_size = MIN_MEM_SIZE;
-module_param(record_size, ulong, 0400);
-MODULE_PARM_DESC(record_size,
-		"size of each dump done on oops/panic");
-
-static ulong mem_address;
-module_param(mem_address, ulong, 0400);
-MODULE_PARM_DESC(mem_address,
-		"start of reserved RAM used to store oops/panic logs");
-
-static ulong mem_size;
-module_param(mem_size, ulong, 0400);
-MODULE_PARM_DESC(mem_size,
-		"size of reserved RAM used to store oops/panic logs");
-
-static int dump_oops = 1;
-module_param(dump_oops, int, 0600);
-MODULE_PARM_DESC(dump_oops,
-		"set to 1 to dump oopses, 0 to only dump panics (default 1)");
-
-struct ramoops_context {
-	void *virt_addr;
-	phys_addr_t phys_addr;
-	unsigned long size;
-	size_t record_size;
-	int dump_oops;
-	unsigned int count;
-	unsigned int max_count;
-	unsigned int read_count;
-	struct pstore_info pstore;
-};
-
-static struct platform_device *dummy;
-static struct ramoops_platform_data *dummy_data;
-
-static int ramoops_pstore_open(struct pstore_info *psi)
-{
-	struct ramoops_context *cxt = psi->data;
-
-	cxt->read_count = 0;
-	return 0;
-}
-
-static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,
-				   struct timespec *time,
-				   char **buf,
-				   struct pstore_info *psi)
-{
-	ssize_t size;
-	char *rambuf;
-	struct ramoops_context *cxt = psi->data;
-
-	if (cxt->read_count >= cxt->max_count)
-		return -EINVAL;
-	*id = cxt->read_count++;
-	/* Only supports dmesg output so far. */
-	*type = PSTORE_TYPE_DMESG;
-	/* TODO(kees): Bogus time for the moment. */
-	time->tv_sec = 0;
-	time->tv_nsec = 0;
-
-	rambuf = cxt->virt_addr + (*id * cxt->record_size);
-	size = strnlen(rambuf, cxt->record_size);
-	*buf = kmalloc(size, GFP_KERNEL);
-	if (*buf == NULL)
-		return -ENOMEM;
-	memcpy(*buf, rambuf, size);
-
-	return size;
-}
-
-static int ramoops_pstore_write(enum pstore_type_id type,
-				enum kmsg_dump_reason reason,
-				u64 *id,
-				unsigned int part,
-				size_t size, struct pstore_info *psi)
-{
-	char *buf;
-	size_t res;
-	struct timeval timestamp;
-	struct ramoops_context *cxt = psi->data;
-	size_t available = cxt->record_size;
-
-	/* Currently ramoops is designed to only store dmesg dumps. */
-	if (type != PSTORE_TYPE_DMESG)
-		return -EINVAL;
-
-	/* Out of the various dmesg dump types, ramoops is currently designed
-	 * to only store crash logs, rather than storing general kernel logs.
-	 */
-	if (reason != KMSG_DUMP_OOPS &&
-	    reason != KMSG_DUMP_PANIC)
-		return -EINVAL;
-
-	/* Skip Oopes when configured to do so. */
-	if (reason == KMSG_DUMP_OOPS && !cxt->dump_oops)
-		return -EINVAL;
-
-	/* Explicitly only take the first part of any new crash.
-	 * If our buffer is larger than kmsg_bytes, this can never happen,
-	 * and if our buffer is smaller than kmsg_bytes, we don't want the
-	 * report split across multiple records.
-	 */
-	if (part != 1)
-		return -ENOSPC;
-
-	buf = cxt->virt_addr + (cxt->count * cxt->record_size);
-
-	res = sprintf(buf, "%s", RAMOOPS_KERNMSG_HDR);
-	buf += res;
-	available -= res;
-
-	do_gettimeofday(&timestamp);
-	res = sprintf(buf, "%lu.%lu\n", (long)timestamp.tv_sec, (long)timestamp.tv_usec);
-	buf += res;
-	available -= res;
-
-	if (size > available)
-		size = available;
-
-	memcpy(buf, cxt->pstore.buf, size);
-	memset(buf + size, '\0', available - size);
-
-	cxt->count = (cxt->count + 1) % cxt->max_count;
-
-	return 0;
-}
-
-static int ramoops_pstore_erase(enum pstore_type_id type, u64 id,
-				struct pstore_info *psi)
-{
-	char *buf;
-	struct ramoops_context *cxt = psi->data;
-
-	if (id >= cxt->max_count)
-		return -EINVAL;
-
-	buf = cxt->virt_addr + (id * cxt->record_size);
-	memset(buf, '\0', cxt->record_size);
-
-	return 0;
-}
-
-static struct ramoops_context oops_cxt = {
-	.pstore = {
-		.owner	= THIS_MODULE,
-		.name	= "ramoops",
-		.open	= ramoops_pstore_open,
-		.read	= ramoops_pstore_read,
-		.write	= ramoops_pstore_write,
-		.erase	= ramoops_pstore_erase,
-	},
-};
-
-static int __init ramoops_probe(struct platform_device *pdev)
-{
-	struct ramoops_platform_data *pdata = pdev->dev.platform_data;
-	struct ramoops_context *cxt = &oops_cxt;
-	int err = -EINVAL;
-
-	/* Only a single ramoops area allowed at a time, so fail extra
-	 * probes.
-	 */
-	if (cxt->max_count)
-		goto fail_out;
-
-	if (!pdata->mem_size || !pdata->record_size) {
-		pr_err("The memory size and the record size must be "
-			"non-zero\n");
-		goto fail_out;
-	}
-
-	pdata->mem_size = rounddown_pow_of_two(pdata->mem_size);
-	pdata->record_size = rounddown_pow_of_two(pdata->record_size);
-
-	/* Check for the minimum memory size */
-	if (pdata->mem_size < MIN_MEM_SIZE &&
-			pdata->record_size < MIN_MEM_SIZE) {
-		pr_err("memory size too small, minimum is %lu\n",
-			MIN_MEM_SIZE);
-		goto fail_out;
-	}
-
-	if (pdata->mem_size < pdata->record_size) {
-		pr_err("The memory size must be larger than the "
-			"records size\n");
-		goto fail_out;
-	}
-
-	cxt->max_count = pdata->mem_size / pdata->record_size;
-	cxt->count = 0;
-	cxt->size = pdata->mem_size;
-	cxt->phys_addr = pdata->mem_address;
-	cxt->record_size = pdata->record_size;
-	cxt->dump_oops = pdata->dump_oops;
-
-	cxt->pstore.data = cxt;
-	cxt->pstore.bufsize = cxt->record_size;
-	cxt->pstore.buf = kmalloc(cxt->pstore.bufsize, GFP_KERNEL);
-	spin_lock_init(&cxt->pstore.buf_lock);
-	if (!cxt->pstore.buf) {
-		pr_err("cannot allocate pstore buffer\n");
-		goto fail_clear;
-	}
-
-	if (!request_mem_region(cxt->phys_addr, cxt->size, "ramoops")) {
-		pr_err("request mem region (0x%lx@0x%llx) failed\n",
-			cxt->size, (unsigned long long)cxt->phys_addr);
-		err = -EINVAL;
-		goto fail_buf;
-	}
-
-	cxt->virt_addr = ioremap(cxt->phys_addr,  cxt->size);
-	if (!cxt->virt_addr) {
-		pr_err("ioremap failed\n");
-		goto fail_mem_region;
-	}
-
-	err = pstore_register(&cxt->pstore);
-	if (err) {
-		pr_err("registering with pstore failed\n");
-		goto fail_iounmap;
-	}
-
-	/*
-	 * Update the module parameter variables as well so they are visible
-	 * through /sys/module/ramoops/parameters/
-	 */
-	mem_size = pdata->mem_size;
-	mem_address = pdata->mem_address;
-	record_size = pdata->record_size;
-	dump_oops = pdata->dump_oops;
-
-	pr_info("attached 0x%lx@0x%llx (%ux0x%zx)\n",
-		cxt->size, (unsigned long long)cxt->phys_addr,
-		cxt->max_count, cxt->record_size);
-
-	return 0;
-
-fail_iounmap:
-	iounmap(cxt->virt_addr);
-fail_mem_region:
-	release_mem_region(cxt->phys_addr, cxt->size);
-fail_buf:
-	kfree(cxt->pstore.buf);
-fail_clear:
-	cxt->pstore.bufsize = 0;
-	cxt->max_count = 0;
-fail_out:
-	return err;
-}
-
-static int __exit ramoops_remove(struct platform_device *pdev)
-{
-#if 0
-	/* TODO(kees): We cannot unload ramoops since pstore doesn't support
-	 * unregistering yet.
-	 */
-	struct ramoops_context *cxt = &oops_cxt;
-
-	iounmap(cxt->virt_addr);
-	release_mem_region(cxt->phys_addr, cxt->size);
-	cxt->max_count = 0;
-
-	/* TODO(kees): When pstore supports unregistering, call it here. */
-	kfree(cxt->pstore.buf);
-	cxt->pstore.bufsize = 0;
-
-	return 0;
-#endif
-	return -EBUSY;
-}
-
-static struct platform_driver ramoops_driver = {
-	.remove		= __exit_p(ramoops_remove),
-	.driver		= {
-		.name	= "ramoops",
-		.owner	= THIS_MODULE,
-	},
-};
-
-static int __init ramoops_init(void)
-{
-	int ret;
-	ret = platform_driver_probe(&ramoops_driver, ramoops_probe);
-	if (ret == -ENODEV) {
-		/*
-		 * If we didn't find a platform device, we use module parameters
-		 * building platform data on the fly.
-		 */
-		pr_info("platform device not found, using module parameters\n");
-		dummy_data = kzalloc(sizeof(struct ramoops_platform_data),
-				     GFP_KERNEL);
-		if (!dummy_data)
-			return -ENOMEM;
-		dummy_data->mem_size = mem_size;
-		dummy_data->mem_address = mem_address;
-		dummy_data->record_size = record_size;
-		dummy_data->dump_oops = dump_oops;
-		dummy = platform_create_bundle(&ramoops_driver, ramoops_probe,
-			NULL, 0, dummy_data,
-			sizeof(struct ramoops_platform_data));
-
-		if (IS_ERR(dummy))
-			ret = PTR_ERR(dummy);
-		else
-			ret = 0;
-	}
-
-	return ret;
-}
-
-static void __exit ramoops_exit(void)
-{
-	platform_driver_unregister(&ramoops_driver);
-	kfree(dummy_data);
-}
-
-module_init(ramoops_init);
-module_exit(ramoops_exit);
-
-MODULE_LICENSE("GPL");
-MODULE_AUTHOR("Marco Stornelli <[email protected]>");
-MODULE_DESCRIPTION("RAM Oops/Panic logger/driver");

fs/pstore/Kconfig +9 -0

index 8007ae7..ad6e594 100644
@@ -11,3 +11,12 @@ config PSTORE
 	   (e.g. ACPI_APEI on X86) which will select this for you.
 	   If you don't have a platform persistent store driver,
 	   say N.
+
+config PSTORE_RAM
+	tristate "Log panic/oops to a RAM buffer"
+	depends on HAS_IOMEM
+	depends on PSTORE
+	default n
+	help
+	  This enables panic and oops messages to be logged to a circular
+	  buffer in RAM where it can be read back at some later point.

fs/pstore/Makefile +1 -0

index 760f4bc..804e376 100644
@@ -5,3 +5,4 @@
 obj-y += pstore.o
 
 pstore-objs += inode.o platform.o
+obj-$(CONFIG_PSTORE_RAM)	+= ram.o

fs/pstore/ram.c new file +367 -0

new file mode 100644
index 0000000..b26b58e
@@ -0,0 +1,367 @@
+/*
+ * RAM Oops/Panic logger
+ *
+ * Copyright (C) 2010 Marco Stornelli <[email protected]>
+ * Copyright (C) 2011 Kees Cook <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ *
+ */
+#define pr_fmt(fmt) "ramoops: " fmt
+
+#include <linux/kernel.h>
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/pstore.h>
+#include <linux/time.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/pstore_ram.h>
+
+/* For historical reasons we name it ramoops when built-in. */
+#ifndef MODULE
+#undef MODULE_PARAM_PREFIX
+#define MODULE_PARAM_PREFIX "ramoops."
+#endif
+
+#define RAMOOPS_KERNMSG_HDR "===="
+#define MIN_MEM_SIZE 4096UL
+
+static ulong record_size = MIN_MEM_SIZE;
+module_param(record_size, ulong, 0400);
+MODULE_PARM_DESC(record_size,
+		"size of each dump done on oops/panic");
+
+static ulong mem_address;
+module_param(mem_address, ulong, 0400);
+MODULE_PARM_DESC(mem_address,
+		"start of reserved RAM used to store oops/panic logs");
+
+static ulong mem_size;
+module_param(mem_size, ulong, 0400);
+MODULE_PARM_DESC(mem_size,
+		"size of reserved RAM used to store oops/panic logs");
+
+static int dump_oops = 1;
+module_param(dump_oops, int, 0600);
+MODULE_PARM_DESC(dump_oops,
+		"set to 1 to dump oopses, 0 to only dump panics (default 1)");
+
+struct ramoops_context {
+	void *virt_addr;
+	phys_addr_t phys_addr;
+	unsigned long size;
+	size_t record_size;
+	int dump_oops;
+	unsigned int count;
+	unsigned int max_count;
+	unsigned int read_count;
+	struct pstore_info pstore;
+};
+
+static struct platform_device *dummy;
+static struct ramoops_platform_data *dummy_data;
+
+static int ramoops_pstore_open(struct pstore_info *psi)
+{
+	struct ramoops_context *cxt = psi->data;
+
+	cxt->read_count = 0;
+	return 0;
+}
+
+static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,
+				   struct timespec *time,
+				   char **buf,
+				   struct pstore_info *psi)
+{
+	ssize_t size;
+	char *rambuf;
+	struct ramoops_context *cxt = psi->data;
+
+	if (cxt->read_count >= cxt->max_count)
+		return -EINVAL;
+	*id = cxt->read_count++;
+	/* Only supports dmesg output so far. */
+	*type = PSTORE_TYPE_DMESG;
+	/* TODO(kees): Bogus time for the moment. */
+	time->tv_sec = 0;
+	time->tv_nsec = 0;
+
+	rambuf = cxt->virt_addr + (*id * cxt->record_size);
+	size = strnlen(rambuf, cxt->record_size);
+	*buf = kmalloc(size, GFP_KERNEL);
+	if (*buf == NULL)
+		return -ENOMEM;
+	memcpy(*buf, rambuf, size);
+
+	return size;
+}
+
+static int ramoops_pstore_write(enum pstore_type_id type,
+				enum kmsg_dump_reason reason,
+				u64 *id,
+				unsigned int part,
+				size_t size, struct pstore_info *psi)
+{
+	char *buf;
+	size_t res;
+	struct timeval timestamp;
+	struct ramoops_context *cxt = psi->data;
+	size_t available = cxt->record_size;
+
+	/* Currently ramoops is designed to only store dmesg dumps. */
+	if (type != PSTORE_TYPE_DMESG)
+		return -EINVAL;
+
+	/* Out of the various dmesg dump types, ramoops is currently designed
+	 * to only store crash logs, rather than storing general kernel logs.
+	 */
+	if (reason != KMSG_DUMP_OOPS &&
+	    reason != KMSG_DUMP_PANIC)
+		return -EINVAL;
+
+	/* Skip Oopes when configured to do so. */
+	if (reason == KMSG_DUMP_OOPS && !cxt->dump_oops)
+		return -EINVAL;
+
+	/* Explicitly only take the first part of any new crash.
+	 * If our buffer is larger than kmsg_bytes, this can never happen,
+	 * and if our buffer is smaller than kmsg_bytes, we don't want the
+	 * report split across multiple records.
+	 */
+	if (part != 1)
+		return -ENOSPC;
+
+	buf = cxt->virt_addr + (cxt->count * cxt->record_size);
+
+	res = sprintf(buf, "%s", RAMOOPS_KERNMSG_HDR);
+	buf += res;
+	available -= res;
+
+	do_gettimeofday(&timestamp);
+	res = sprintf(buf, "%lu.%lu\n", (long)timestamp.tv_sec, (long)timestamp.tv_usec);
+	buf += res;
+	available -= res;
+
+	if (size > available)
+		size = available;
+
+	memcpy(buf, cxt->pstore.buf, size);
+	memset(buf + size, '\0', available - size);
+
+	cxt->count = (cxt->count + 1) % cxt->max_count;
+
+	return 0;
+}
+
+static int ramoops_pstore_erase(enum pstore_type_id type, u64 id,
+				struct pstore_info *psi)
+{
+	char *buf;
+	struct ramoops_context *cxt = psi->data;
+
+	if (id >= cxt->max_count)
+		return -EINVAL;
+
+	buf = cxt->virt_addr + (id * cxt->record_size);
+	memset(buf, '\0', cxt->record_size);
+
+	return 0;
+}
+
+static struct ramoops_context oops_cxt = {
+	.pstore = {
+		.owner	= THIS_MODULE,
+		.name	= "ramoops",
+		.open	= ramoops_pstore_open,
+		.read	= ramoops_pstore_read,
+		.write	= ramoops_pstore_write,
+		.erase	= ramoops_pstore_erase,
+	},
+};
+
+static int __init ramoops_probe(struct platform_device *pdev)
+{
+	struct ramoops_platform_data *pdata = pdev->dev.platform_data;
+	struct ramoops_context *cxt = &oops_cxt;
+	int err = -EINVAL;
+
+	/* Only a single ramoops area allowed at a time, so fail extra
+	 * probes.
+	 */
+	if (cxt->max_count)
+		goto fail_out;
+
+	if (!pdata->mem_size || !pdata->record_size) {
+		pr_err("The memory size and the record size must be "
+			"non-zero\n");
+		goto fail_out;
+	}
+
+	pdata->mem_size = rounddown_pow_of_two(pdata->mem_size);
+	pdata->record_size = rounddown_pow_of_two(pdata->record_size);
+
+	/* Check for the minimum memory size */
+	if (pdata->mem_size < MIN_MEM_SIZE &&
+			pdata->record_size < MIN_MEM_SIZE) {
+		pr_err("memory size too small, minimum is %lu\n",
+			MIN_MEM_SIZE);
+		goto fail_out;
+	}
+
+	if (pdata->mem_size < pdata->record_size) {
+		pr_err("The memory size must be larger than the "
+			"records size\n");
+		goto fail_out;
+	}
+
+	cxt->max_count = pdata->mem_size / pdata->record_size;
+	cxt->count = 0;
+	cxt->size = pdata->mem_size;
+	cxt->phys_addr = pdata->mem_address;
+	cxt->record_size = pdata->record_size;
+	cxt->dump_oops = pdata->dump_oops;
+
+	cxt->pstore.data = cxt;
+	cxt->pstore.bufsize = cxt->record_size;
+	cxt->pstore.buf = kmalloc(cxt->pstore.bufsize, GFP_KERNEL);
+	spin_lock_init(&cxt->pstore.buf_lock);
+	if (!cxt->pstore.buf) {
+		pr_err("cannot allocate pstore buffer\n");
+		goto fail_clear;
+	}
+
+	if (!request_mem_region(cxt->phys_addr, cxt->size, "ramoops")) {
+		pr_err("request mem region (0x%lx@0x%llx) failed\n",
+			cxt->size, (unsigned long long)cxt->phys_addr);
+		err = -EINVAL;
+		goto fail_buf;
+	}
+
+	cxt->virt_addr = ioremap(cxt->phys_addr,  cxt->size);
+	if (!cxt->virt_addr) {
+		pr_err("ioremap failed\n");
+		goto fail_mem_region;
+	}
+
+	err = pstore_register(&cxt->pstore);
+	if (err) {
+		pr_err("registering with pstore failed\n");
+		goto fail_iounmap;
+	}
+
+	/*
+	 * Update the module parameter variables as well so they are visible
+	 * through /sys/module/ramoops/parameters/
+	 */
+	mem_size = pdata->mem_size;
+	mem_address = pdata->mem_address;
+	record_size = pdata->record_size;
+	dump_oops = pdata->dump_oops;
+
+	pr_info("attached 0x%lx@0x%llx (%ux0x%zx)\n",
+		cxt->size, (unsigned long long)cxt->phys_addr,
+		cxt->max_count, cxt->record_size);
+
+	return 0;
+
+fail_iounmap:
+	iounmap(cxt->virt_addr);
+fail_mem_region:
+	release_mem_region(cxt->phys_addr, cxt->size);
+fail_buf:
+	kfree(cxt->pstore.buf);
+fail_clear:
+	cxt->pstore.bufsize = 0;
+	cxt->max_count = 0;
+fail_out:
+	return err;
+}
+
+static int __exit ramoops_remove(struct platform_device *pdev)
+{
+#if 0
+	/* TODO(kees): We cannot unload ramoops since pstore doesn't support
+	 * unregistering yet.
+	 */
+	struct ramoops_context *cxt = &oops_cxt;
+
+	iounmap(cxt->virt_addr);
+	release_mem_region(cxt->phys_addr, cxt->size);
+	cxt->max_count = 0;
+
+	/* TODO(kees): When pstore supports unregistering, call it here. */
+	kfree(cxt->pstore.buf);
+	cxt->pstore.bufsize = 0;
+
+	return 0;
+#endif
+	return -EBUSY;
+}
+
+static struct platform_driver ramoops_driver = {
+	.remove		= __exit_p(ramoops_remove),
+	.driver		= {
+		.name	= "ramoops",
+		.owner	= THIS_MODULE,
+	},
+};
+
+static int __init ramoops_init(void)
+{
+	int ret;
+	ret = platform_driver_probe(&ramoops_driver, ramoops_probe);
+	if (ret == -ENODEV) {
+		/*
+		 * If we didn't find a platform device, we use module parameters
+		 * building platform data on the fly.
+		 */
+		pr_info("platform device not found, using module parameters\n");
+		dummy_data = kzalloc(sizeof(struct ramoops_platform_data),
+				     GFP_KERNEL);
+		if (!dummy_data)
+			return -ENOMEM;
+		dummy_data->mem_size = mem_size;
+		dummy_data->mem_address = mem_address;
+		dummy_data->record_size = record_size;
+		dummy_data->dump_oops = dump_oops;
+		dummy = platform_create_bundle(&ramoops_driver, ramoops_probe,
+			NULL, 0, dummy_data,
+			sizeof(struct ramoops_platform_data));
+
+		if (IS_ERR(dummy))
+			ret = PTR_ERR(dummy);
+		else
+			ret = 0;
+	}
+
+	return ret;
+}
+
+static void __exit ramoops_exit(void)
+{
+	platform_driver_unregister(&ramoops_driver);
+	kfree(dummy_data);
+}
+
+module_init(ramoops_init);
+module_exit(ramoops_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Marco Stornelli <[email protected]>");
+MODULE_DESCRIPTION("RAM Oops/Panic logger/driver");

include/linux/pstore_ram.h new file +17 -0

new file mode 100644
index 0000000..484fef8
@@ -0,0 +1,17 @@
+#ifndef __RAMOOPS_H
+#define __RAMOOPS_H
+
+/*
+ * Ramoops platform data
+ * @mem_size	memory size for ramoops
+ * @mem_address	physical memory address to contain ramoops
+ */
+
+struct ramoops_platform_data {
+	unsigned long	mem_size;
+	unsigned long	mem_address;
+	unsigned long	record_size;
+	int		dump_oops;
+};
+
+#endif

include/linux/ramoops.h deleted +0 -17

deleted file mode 100644
index 484fef8..0000000
@@ -1,17 +0,0 @@
-#ifndef __RAMOOPS_H
-#define __RAMOOPS_H
-
-/*
- * Ramoops platform data
- * @mem_size	memory size for ramoops
- * @mem_address	physical memory address to contain ramoops
- */
-
-struct ramoops_platform_data {
-	unsigned long	mem_size;
-	unsigned long	mem_address;
-	unsigned long	record_size;
-	int		dump_oops;
-};
-
-#endif

On Mon, May 14, 2012 at 10:30:22AM -0600, Shuah Khan wrote:
[...]

Quoted 2 lines

Anton! Is it safe to assume you are planning to cover the second feature as well, in which case I can drop my plans to get this work done.

Yep, absolutely. I'm fully committed to add runtime logging support to pstore. Actually, I'll post patches pretty soon.

Quoted 13 lines · Anton Vorontsov

Since ramoops was converted to pstore, it has nothing to do with character devices nowadays. Instead, today it is just a RAM backend for pstore.

The patch just moves things around. There are a few changes were needed because of the move:

1. Kconfig and Makefiles fixups, of course.

2. In pstore/ram.c we have to play a bit with MODULE_PARAM_PREFIX, this is needed to keep user experience the same as with ramoops driver (i.e. so that ramoops.foo kernel command line arguments would still work).

Quoted 1 line

Signed-off-by Anton Vorontsov <[email protected]>

This consolidation seems good. I might prefer the move separated from the changes, just to make review easier, but I have no idea what that'll do to a bisect. :P

fs/pstore/ram.c

Quoted 2 lines

fs/pstore/ram.c new file +0 -0

"ram.ko" seems like an awfully generic modbule name. Should this be called pstore_ram.* instead, like was done for the header file?

And unless anyone objects, I have no problem letting the built-in name change too.

include/linux/pstore_ram.h line 2

Quoted 5 lines

include/linux/pstore_ram.h new file +2 -0

@@ -0,0 +1,17 @@
+#ifndef __RAMOOPS_H
+#define __RAMOOPS_H

This define should probably change just to avoid confusion.

even worse, mem= from the bootloader. Mixing the two methods together would be confusing.

Either persistent_ram_early_init should be
removed completely (or replaced with something that is easier to register ramoops into), or ramoops should use persistent_ram_init_ringbuffer like ram_console does.

Quoted 3 lines

-       buf = cxt->virt_addr + (id * cxt->record_size);
-       memset(buf, '\0', cxt->record_size);
+       persistent_ram_free_old(cxt->przs[id]);

Hm, I don't think persistent_ram_free_old() is what's wanted here. That appears to entirely release the region? I want to make sure the memory is cleared first. And will this area come back on a write, or does it stay released?

Quoted 15 lines

       return 0;
 }

fs/pstore/ram.c +1 -0

@@ -200,6 +203,7 @@ static int __init ramoops_probe(struct platform_device *pdev)
        struct ramoops_platform_data *pdata = pdev->dev.platform_data;
        struct ramoops_context *cxt = &oops_cxt;
        int err = -EINVAL;
+       int i;
 
        /* Only a single ramoops area allowed at a time, so fail extra
         * probes.

@@ -237,32 +241,37 @@ static int __init ramoops_probe(struct platform_device *pdev)
        cxt->record_size = pdata->record_size;
        cxt->dump_oops = pdata->dump_oops;
 

fs/pstore/ram.c static int __init ramoops_probe(struct platform_device *pdev) · line 247

Quoted 4 lines

fs/pstore/ram.c +4 -0

+       cxt->przs = kzalloc(sizeof(*cxt->przs) * cxt->max_count, GFP_KERNEL);
+       if (!cxt->przs) {
+               pr_err("failed to initialize a prz array\n");
+               goto fail_przs;

This should be fail_out.

fs/pstore/ram.c static int __init ramoops_probe(struct platform_device *pdev) · line 254

Quoted 7 lines

fs/pstore/ram.c +7 -0

+       }
+
+       for (i = 0; i < cxt->max_count; i++) {
+               size_t sz = cxt->record_size;
+               phys_addr_t start = cxt->phys_addr + sz * i;
+
+               cxt->przs[i] = persistent_ram_new(start, sz, 0);

persistent_ram_new() is marked as __init, so this is unsafe to call if built as a module. I think persistent_ram_new() will need to lose the __init marking, or I'm misunderstanding something.

fs/pstore/ram.c static int __init ramoops_probe(struct platform_device *pdev) · line 257

Quoted 3 lines

fs/pstore/ram.c +3 -0

+               if (IS_ERR(cxt->przs[i])) {
+                       err = PTR_ERR(cxt->przs[i]);
+                       pr_err("failed to initialize a prz\n");

Since neither persistent_ram_new() nor persistent_ram_buffer_map() report the location of the failure, I'd like to keep the error report (removed below "pr_err("request mem region (0x%lx@0x%llx) failed\n",...") for failures, so there is something actionable in dmesg when the platform data is mismatched for the hardware.

fs/pstore/ram.c static int __init ramoops_probe(struct platform_device *pdev) · line 265

Quoted 6 lines

fs/pstore/ram.c +2 -2

        cxt->pstore.data = cxt;
-       cxt->pstore.bufsize = cxt->record_size;
-       cxt->pstore.buf = kmalloc(cxt->pstore.bufsize, GFP_KERNEL);
        spin_lock_init(&cxt->pstore.buf_lock);
+       cxt->pstore.bufsize = cxt->przs[0]->buffer_size;
+       cxt->pstore.buf = kmalloc(cxt->pstore.bufsize, GFP_KERNEL);

I don't see a reason to re-order these (nothing can use buf yet because we haven't registered it with pstore yet).

fs/pstore/ram.c static int __init ramoops_probe(struct platform_device *pdev) · line 299

Quoted 3 lines

fs/pstore/ram.c +3 -0

+fail_przs:
+       for (i = 0; cxt->przs[i]; i++)
+               persistent_ram_free(cxt->przs[i]);

This can lead to a BUG, since persistent_ram_free() doesn't handle NULL arguments.

Quoted 6 lines · Anton Vorontsov

Since ramoops was converted to pstore, it has nothing to do with character devices nowadays. Instead, today it is just a RAM backend for pstore.

The patch just moves things around. There are a few changes were needed because of the move:

Quoted 6 lines

1. Kconfig and Makefiles fixups, of course.

2. In pstore/ram.c we have to play a bit with MODULE_PARAM_PREFIX, this is needed to keep user experience the same as with ramoops driver (i.e. so that ramoops.foo kernel command line arguments would still work).

Anton,

Could you please enhance Kconfig as well as ram.c with information with the new functionality it supports.

Also ram.c in my opinion doesn't
really reflect the feature it currently supports and its future plans. ramoops doesn't either. ramdesg or ramkmsg probably are better suited.

Also leaving the ABI that ramoops specific might lead confusion in the long run. It might make sense to update the ABI to reflect its new features, if it doesn't impact existing ramoops users.

Would you be interested in adding a doc file for usage describing how users can configure the driver - the details I would like to see are how to pick a ram address especially when mem_address and mem_size are passed in as module parameters.

Hello Kees,

On Mon, May 14, 2012 at 03:21:17PM -0700, Kees Cook wrote:
[...]

Quoted 4 lines

Quoted 3 lines

-       buf = cxt->virt_addr + (id * cxt->record_size);
-       memset(buf, '\0', cxt->record_size);
+       persistent_ram_free_old(cxt->przs[id]);

Quoted 4 lines

Hm, I don't think persistent_ram_free_old() is what's wanted here. That appears to entirely release the region? I want to make sure the memory is cleared first. And will this area come back on a write, or does it stay released?

It just releases ECC-restored memory region (a copy). The original (persistent) region is still fully reusable after that call.

Quoted 11 lines

fs/pstore/ram.c static int __init ramoops_probe(struct platform_device *pdev) · line 254

Quoted 7 lines

+       }

fs/pstore/ram.c +6 -0

+
+       for (i = 0; i < cxt->max_count; i++) {
+               size_t sz = cxt->record_size;
+               phys_addr_t start = cxt->phys_addr + sz * i;
+
+               cxt->przs[i] = persistent_ram_new(start, sz, 0);

persistent_ram_new() is marked as __init, so this is unsafe to call if built as a module. I think persistent_ram_new() will need to lose the __init marking, or I'm misunderstanding something.

Um. ramoops' probe routine is also __init. persistent_ram_new is a part of ramoops module, so their __init functions will be discarded at the same time.

ram_console can't be a module, so it is also fine.

So I think it's all fine.

Quoted 6 lines

Quoted 3 lines

+fail_przs:
+       for (i = 0; cxt->przs[i]; i++)
+               persistent_ram_free(cxt->przs[i]);

This can lead to a BUG, since persistent_ram_free() doesn't handle NULL arguments.

The for loop has 'cxt->przs[i]' condition. :-)

Thanks for the review!

Hi Shuah,

Quoted 14 lines · Shuah Khan

Quoted 6 lines · Anton Vorontsov

Since ramoops was converted to pstore, it has nothing to do with character devices nowadays. Instead, today it is just a RAM backend for pstore.

The patch just moves things around. There are a few changes were needed because of the move:

Quoted 6 lines

1. Kconfig and Makefiles fixups, of course.

2. In pstore/ram.c we have to play a bit with MODULE_PARAM_PREFIX, this is needed to keep user experience the same as with ramoops driver (i.e. so that ramoops.foo kernel command line arguments would still work).

Quoted 4 lines

Anton,

Could you please enhance Kconfig as well as ram.c with information with the new functionality it supports.

Sure, will do.

Quoted 3 lines

Also ram.c in my opinion doesn't
really reflect the feature it currently supports and its future plans. ramoops doesn't either. ramdesg or ramkmsg probably are better suited.

No, I actually think we shouldn't mention neither dmesg nor kmsg in the name of the module. We might support MCE messages, tracing messages and so on, and this all will be handled by ram.c.

So, ram.c is a generic backend for pstore.

Quoted 3 lines

Also leaving the ABI that ramoops specific might lead confusion in the long run. It might make sense to update the ABI to reflect its new features, if it doesn't impact existing ramoops users.

We can do this, I can prepare a separate patch to change the ABI, but so far I tend to not break any ABIs. We can always do it later -- it is easy. :-D

Quoted 5 lines

Would you be interested in adding a doc file for usage describing how users can configure the driver - the details I would like to see are how to pick a ram address especially when mem_address and mem_size are passed in as module parameters.

We actually have Documentation/ramoops.txt already, but I'll add a documentation for the new ecc option.

Thanks!

This includes devices' memory (e.g. framebuffers or memory mapped EEPROMs on a local bus), as well as the normal RAM that we don't use for the main memory.

For the normal (but unused) ram we could use kmaps, but this assumes highmem support, so we don't bother and just use the memory via ioremap.

As a side effect, the following hack is possible: when used together with pstore_ram (new ramoops) module, we can limit the normal RAM region with mem= and then point ramoops to use the rest of the memory, e.g.

       mem=128M ramoops.mem_address=0x8000000

Sure, we could just reserve the region with memblock_reserve() early in the arch/ code, and then register a pstore_ram platform device pointing to the reserved region. It's still a viable option if platform wants to do so.

Also, we might want to use IO accessors in case of a real device, but for now we don't bother (the old ramoops wasn't using it either, so at least we don't make things worse).

Quoted 14 lines · Anton Vorontsov

This includes devices' memory (e.g. framebuffers or memory mapped EEPROMs on a local bus), as well as the normal RAM that we don't use for the main memory.

For the normal (but unused) ram we could use kmaps, but this assumes highmem support, so we don't bother and just use the memory via ioremap.

As a side effect, the following hack is possible: when used together with pstore_ram (new ramoops) module, we can limit the normal RAM region with mem= and then point ramoops to use the rest of the memory, e.g.

       mem=128M ramoops.mem_address=0x8000000

Quoted 8 lines

Sure, we could just reserve the region with memblock_reserve() early in the arch/ code, and then register a pstore_ram platform device pointing to the reserved region. It's still a viable option if platform wants to do so.

Also, we might want to use IO accessors in case of a real device, but for now we don't bother (the old ramoops wasn't using it either, so at least we don't make things worse).

This is long merged, but I remembered why I moved away from using ioremap. The current code uses atomics to track the ringbuffer positions, which results in ldrex and strex instructions on ARM. ldrex and strex on memory that is mapped as Device memory (which is what ioremap maps as) is implementation defined, and is unpredictable at the architecture level.