On Wed, Apr 10, 2013 at 12:53:03PM +0530, Aruna Balakrishnaiah wrote:This patch exploits pstore infrastructure in power systems.In the kernel we use "pseries" instead of "system p".
IBM's system p machines provide persistent storage for LPARs
through NVRAM. NVRAM's lnx,oops-log partition is used to logWhat are the implications of falling back to kmsg_dump()?
oops messages. In case pstore registration fails it will
fall back to kmsg_dump mechanism.
Is there any reason we would not want to enable CONFIG_PSTORE ? ie.
should the pseries platform just select it?
diff --git a/arch/powerpc/platforms/pseries/nvram.c b/arch/powerpc/platforms/pseries/nvram.cI think you should be able to rearrange this so that you don't need the
index 6701b71..82d32a2 100644
--- a/arch/powerpc/platforms/pseries/nvram.c
+++ b/arch/powerpc/platforms/pseries/nvram.c
@@ -18,6 +18,7 @@
#include <linux/spinlock.h>
#include <linux/slab.h>
#include <linux/kmsg_dump.h>
+#include <linux/pstore.h>
#include <linux/ctype.h>
#include <linux/zlib.h>
#include <asm/uaccess.h>
@@ -87,6 +88,25 @@ static struct kmsg_dumper nvram_kmsg_dumper = {
.dump = oops_to_nvram
};
+static int nvram_pstore_open(struct pstore_info *psi);
+
+static ssize_t nvram_pstore_read(u64 *id, enum pstore_type_id *type,
+ int *count, struct timespec *time, char **buf,
+ struct pstore_info *psi);
+
+static int nvram_pstore_write(enum pstore_type_id type,
+ enum kmsg_dump_reason reason, u64 *id,
+ unsigned int part, int count, size_t size,
+ struct pstore_info *psi);
forward declarations.
+If we are going to have CONFIG_PSTORE #ifdefs in this file, I don't see
+static struct pstore_info nvram_pstore_info = {
+ .owner = THIS_MODULE,
+ .name = "nvram",
+ .open = nvram_pstore_open,
+ .read = nvram_pstore_read,
+ .write = nvram_pstore_write,
+};
+
/* See clobbering_unread_rtas_event() */
#define NVRAM_RTAS_READ_TIMEOUT 5 /* seconds */
static unsigned long last_unread_rtas_event; /* timestamp */
@@ -121,6 +141,13 @@ static char *big_oops_buf, *oops_buf;
static char *oops_data;
static size_t oops_data_sz;
+#ifdef CONFIG_PSTORE
why there can't be just a single block of code that is #ifdef'ed, rather
than several like you have.
+static enum pstore_type_id nvram_type_ids[] = {I don't understand what you're doing with read_type. It looks fishy.
+ PSTORE_TYPE_DMESG,
+ -1
+};
+static int read_type;
+#endifYou don't need the goto.
/* Compression parameters */
#define COMPR_LEVEL 6
#define WINDOW_BITS 12
@@ -455,6 +482,23 @@ static void __init nvram_init_oops_partition(int rtas_partition_exists)
oops_data = oops_buf + sizeof(struct oops_log_info);
oops_data_sz = oops_log_partition.size - sizeof(struct oops_log_info);
+ nvram_pstore_info.buf = oops_data;
+ nvram_pstore_info.bufsize = oops_data_sz;
+
+ rc = pstore_register(&nvram_pstore_info);
+
+ if (rc != 0) {
+ pr_err("nvram: pstore_register() failed, defaults to "
+ "kmsg_dump; returned %d\n", rc);
+ goto kmsg_dump;
+ } else {What is the issue here?
+ /*TODO: Support compression when pstore is configured */
+ pr_info("nvram: Compression of oops text supported only when "Same comment about too many ifdefs.
+ "pstore is not configured");
+ return;
+ }
+
+kmsg_dump:
/*
* Figure compression (preceded by elimination of each line's <n>
* severity prefix) will reduce the oops/panic report to at most
@@ -663,3 +707,104 @@ static void oops_to_nvram(struct kmsg_dumper *dumper,
spin_unlock_irqrestore(&lock, flags);
}
+
+#ifdef CONFIG_PSTORE
+static int nvram_pstore_open(struct pstore_info *psi)Locking?
+{
+ read_type = -1;
+ return 0;Make it a kernel-doc style comment.
+}
+
+/*
+ * Called by pstore_dump() when an oops or panic report is logged to the printk"@size bytes have", or "@size bytes should be written"?
+ * buffer. @size bytes have been written to oops_buf, starting after the
+ * oops_log_info header.
+ */Why would we be called with the wrong type? Would it be better to just
+static int nvram_pstore_write(enum pstore_type_id type,
+ enum kmsg_dump_reason reason,
+ u64 *id, unsigned int part, int count,
+ size_t size, struct pstore_info *psi)
+{
+ struct oops_log_info *oops_hdr = (struct oops_log_info *) oops_buf;
+
+ /* part 1 has the recent messages from printk buffer */
+ if (part > 1 || clobbering_unread_rtas_event())
+ return -1;
+
+ BUG_ON(type != PSTORE_TYPE_DMESG);
+ BUG_ON(sizeof(*oops_hdr) + size > oops_log_partition.size);
return an error, rather than causing another oops while we're trying to
write the oops?
And couldn't we just clamp the size, rather than BUG'ing.
+ oops_hdr->version = OOPS_HDR_VERSION;You definitely don't need the (void). But more to the point why aren't
+ oops_hdr->report_length = (u16) size;
+ oops_hdr->timestamp = get_seconds();
+ (void) nvram_write_os_partition(&oops_log_partition, oops_buf,
+ (int) (sizeof(*oops_hdr) + size), ERR_TYPE_KERNEL_PANIC,
+ count);
you checking the return value?
+ *id = part;What is this? Part of the API?
+ return 0;Can't you just cast in the call to nvram_read_partition() ?
+}
+
+/*
+ * Reads the oops/panic report.
+ * Returns the length of the data we read from each partition.
+ * Returns 0 if we've been called before.
+ */
+static ssize_t nvram_pstore_read(u64 *id, enum pstore_type_id *type,
+ int *count, struct timespec *time, char **buf,
+ struct pstore_info *psi)
+{
+ struct oops_log_info *oops_hdr;
+ unsigned int err_type, id_no;
+ struct nvram_os_partition *part = NULL;
+ char *buff = NULL;
+
+ read_type++;
+
+ switch (nvram_type_ids[read_type]) {
+ case PSTORE_TYPE_DMESG:
+ part = &oops_log_partition;
+ *type = PSTORE_TYPE_DMESG;
+ break;
+ default:
+ return 0;
+ }
+
+ buff = kmalloc(part->size, GFP_KERNEL);
+
+ if (!buff)
+ return -ENOMEM;
+
+ if (nvram_read_partition(part, buff, part->size, &err_type, &id_no)) {
+ kfree(buff);
+ return 0;
+ }
+
+ *count = 0;
+ *id = id_no;
+ oops_hdr = (struct oops_log_info *)buff;I don't understand why we have empty versions of these. If CONFIG_PSTORE
+ *buf = buff + sizeof(*oops_hdr);
+ time->tv_sec = oops_hdr->timestamp;
+ time->tv_nsec = 0;
+ return oops_hdr->report_length;
+}
+#else
+static int nvram_pstore_open(struct pstore_info *psi)
+{
+ return 0;
+}
+
+static int nvram_pstore_write(enum pstore_type_id type,
+ enum kmsg_dump_reason reason, u64 *id,
+ unsigned int part, int count, size_t size,
+ struct pstore_info *psi)
+{
+ return 0;
+}
+
+static ssize_t nvram_pstore_read(u64 *id, enum pstore_type_id *type,
+ int *count, struct timespec *time, char **buf,
+ struct pstore_info *psi)
+{
+ return 0;
+}
+#endif
is disabled we should just not register with pstore at all.
cheers