[PATCH 0/8] Nvram-to-pstore

[PATCH 0/8] Nvram-to-pstore

[Aruna Balakrishnaiah]

Currently the kernel provides the contents of p-series NVRAM only as a
simple stream of bytes via /dev/nvram, which must be interpreted in user
space by the nvram command in the powerpc-utils package.  This patch set
exploits the pstore subsystem to expose each partition in NVRAM as a
separate file in /dev/pstore. For instance Oops messages will stored in a
file named [dmesg-nvram-2].

---

Aruna Balakrishnaiah (8):
      Remove syslog prefix in uncompressed oops text
      Add version and timestamp to oops header
      Introduce generic read function to read nvram-partitions
      Read/Write oops nvram partition via pstore
      Read rtas partition via pstore
      Distinguish between a os-partition and non-os partition
      Read of-config partition via pstore
      Read common partition via pstore


 arch/powerpc/platforms/pseries/nvram.c |  329 ++++++++++++++++++++++++++++----
 fs/pstore/inode.c                      |    9 +
 include/linux/pstore.h                 |    4 
 3 files changed, 304 insertions(+), 38 deletions(-)

-- 

[PATCH 1/8] Remove syslog prefix in uncompressed oops text

[Aruna Balakrishnaiah]

Removal of syslog prefix in the uncompressed oops text will
help in capturing more oops data.

Signed-off-by: Aruna Balakrishnaiah <aruna@linux.vnet.ibm.com>
Reviewed-by: Jim Keniston <jkenisto@us.ibm.com>
---
 arch/powerpc/platforms/pseries/nvram.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/pseries/nvram.c b/arch/powerpc/platforms/pseries/nvram.c
index 8733a86..e54a8b7 100644
--- a/arch/powerpc/platforms/pseries/nvram.c
+++ b/arch/powerpc/platforms/pseries/nvram.c
@@ -619,7 +619,7 @@ static void oops_to_nvram(struct kmsg_dumper *dumper,
 	}
 	if (rc != 0) {
 		kmsg_dump_rewind(dumper);
-		kmsg_dump_get_buffer(dumper, true,
+		kmsg_dump_get_buffer(dumper, false,
 				     oops_data, oops_data_sz, &text_len);
 		err_type = ERR_TYPE_KERNEL_PANIC;
 		*oops_len = (u16) text_len;

Re: [PATCH 1/8] Remove syslog prefix in uncompressed oops text

[Michael Ellerman]

On Wed, Apr 10, 2013 at 12:51:00PM +0530, Aruna Balakrishnaiah wrote:
> Removal of syslog prefix in the uncompressed oops text will
> help in capturing more oops data.

Why does it help? Does this effect any existing tools?

cheers

Re: [PATCH 1/8] Remove syslog prefix in uncompressed oops text

[aruna]

On Monday 15 April 2013 12:50 PM, Michael Ellerman wrote:
> On Wed, Apr 10, 2013 at 12:51:00PM +0530, Aruna Balakrishnaiah wrote:
>> Removal of syslog prefix in the uncompressed oops text will
>> help in capturing more oops data.
> Why does it help? Does this effect any existing tools?
>
> cheers
>

By setting the (2nd) syslog argument of kmsg_dump_get_buffer() to false,
we omit <n> line prefixes and thereby capture more of the printk buffer.

No this should not effect any existing tools.


Regards,
Aruna

[PATCH 2/8] Add version and timestamp to oops header

[Aruna Balakrishnaiah]

Introduce version and timestamp information in the oops header.
oops_log_info (oops header) holds version (to distinguish between old
and new format oops header), length of the oops text
(compressed or uncompressed) and timestamp.

Signed-off-by: Aruna Balakrishnaiah <aruna@linux.vnet.ibm.com>
Reviewed-by: Jim Keniston <jkenisto@us.ibm.com>
---
 arch/powerpc/platforms/pseries/nvram.c |   57 +++++++++++++++++++++-----------
 1 file changed, 38 insertions(+), 19 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/nvram.c b/arch/powerpc/platforms/pseries/nvram.c
index e54a8b7..742735a 100644
--- a/arch/powerpc/platforms/pseries/nvram.c
+++ b/arch/powerpc/platforms/pseries/nvram.c
@@ -29,6 +29,13 @@
 /* Max bytes to read/write in one go */
 #define NVRW_CNT 0x20
 
+/*
+ * Set oops header version to distingush between old and new format header.
+ * lnx,oops-log partition max size is 4000, header version > 4000 will
+ * help in identifying new header.
+ */
+#define OOPS_HDR_VERSION 5000
+
 static unsigned int nvram_size;
 static int nvram_fetch, nvram_store;
 static char nvram_buf[NVRW_CNT];	/* assume this is in the first 4GB */
@@ -67,6 +74,12 @@ static const char *pseries_nvram_os_partitions[] = {
 	NULL
 };
 
+struct oops_log_info {
+	u16 version;
+	u16 report_length;
+	u64 timestamp;
+} __attribute__((packed));
+
 static void oops_to_nvram(struct kmsg_dumper *dumper,
 			  enum kmsg_dump_reason reason);
 
@@ -83,28 +96,28 @@ static unsigned long last_unread_rtas_event;	/* timestamp */
 
  * big_oops_buf[] holds the uncompressed text we're capturing.
  *
- * oops_buf[] holds the compressed text, preceded by a prefix.
- * The prefix is just a u16 holding the length of the compressed* text.
- * (*Or uncompressed, if compression fails.)  oops_buf[] gets written
- * to NVRAM.
+ * oops_buf[] holds the compressed text, preceded by a oops header.
+ * oops header has u16 holding the version of oops header (to differentiate
+ * between old and new format header) followed by u16 holding the length of
+ * the compressed* text (*Or uncompressed, if compression fails.) and u64
+ * holding the timestamp. oops_buf[] gets written to NVRAM.
  *
- * oops_len points to the prefix.  oops_data points to the compressed text.
+ * oops_log_info points to the header. oops_data points to the compressed text.
  *
  * +- oops_buf
- * |		+- oops_data
- * v		v
- * +------------+-----------------------------------------------+
- * | length	| text                                          |
- * | (2 bytes)	| (oops_data_sz bytes)                          |
- * +------------+-----------------------------------------------+
+ * |                                   +- oops_data
+ * v                                   v
+ * +-----------+-----------+-----------+------------------------+
+ * | version   | length    | timestamp | text                   |
+ * | (2 bytes) | (2 bytes) | (8 bytes) | (oops_data_sz bytes)   |
+ * +-----------+-----------+-----------+------------------------+
  * ^
- * +- oops_len
+ * +- oops_log_info
  *
  * We preallocate these buffers during init to avoid kmalloc during oops/panic.
  */
 static size_t big_oops_buf_sz;
 static char *big_oops_buf, *oops_buf;
-static u16 *oops_len;
 static char *oops_data;
 static size_t oops_data_sz;
 
@@ -425,9 +438,8 @@ static void __init nvram_init_oops_partition(int rtas_partition_exists)
 						oops_log_partition.name);
 		return;
 	}
-	oops_len = (u16*) oops_buf;
-	oops_data = oops_buf + sizeof(u16);
-	oops_data_sz = oops_log_partition.size - sizeof(u16);
+	oops_data = oops_buf + sizeof(struct oops_log_info);
+	oops_data_sz = oops_log_partition.size - sizeof(struct oops_log_info);
 
 	/*
 	 * Figure compression (preceded by elimination of each line's <n>
@@ -555,6 +567,7 @@ error:
 /* Compress the text from big_oops_buf into oops_buf. */
 static int zip_oops(size_t text_len)
 {
+	struct oops_log_info *oops_hdr = (struct oops_log_info *)oops_buf;
 	int zipped_len = nvram_compress(big_oops_buf, oops_data, text_len,
 								oops_data_sz);
 	if (zipped_len < 0) {
@@ -562,7 +575,9 @@ static int zip_oops(size_t text_len)
 		pr_err("nvram: logging uncompressed oops/panic report\n");
 		return -1;
 	}
-	*oops_len = (u16) zipped_len;
+	oops_hdr->version = OOPS_HDR_VERSION;
+	oops_hdr->report_length = (u16) zipped_len;
+	oops_hdr->timestamp = get_seconds();
 	return 0;
 }
 
@@ -576,6 +591,7 @@ static int zip_oops(size_t text_len)
 static void oops_to_nvram(struct kmsg_dumper *dumper,
 			  enum kmsg_dump_reason reason)
 {
+	struct oops_log_info *oops_hdr = (struct oops_log_info *)oops_buf;
 	static unsigned int oops_count = 0;
 	static bool panicking = false;
 	static DEFINE_SPINLOCK(lock);
@@ -622,11 +638,14 @@ static void oops_to_nvram(struct kmsg_dumper *dumper,
 		kmsg_dump_get_buffer(dumper, false,
 				     oops_data, oops_data_sz, &text_len);
 		err_type = ERR_TYPE_KERNEL_PANIC;
-		*oops_len = (u16) text_len;
+		oops_hdr->version = OOPS_HDR_VERSION;
+		oops_hdr->report_length = (u16) text_len;
+		oops_hdr->timestamp = get_seconds();
 	}
 
 	(void) nvram_write_os_partition(&oops_log_partition, oops_buf,
-		(int) (sizeof(*oops_len) + *oops_len), err_type, ++oops_count);
+		(int) (sizeof(*oops_hdr) + oops_hdr->report_length), err_type,
+		++oops_count);
 
 	spin_unlock_irqrestore(&lock, flags);
 }

Re: [PATCH 2/8] Add version and timestamp to oops header

[Michael Ellerman]

On Wed, Apr 10, 2013 at 12:51:12PM +0530, Aruna Balakrishnaiah wrote:
> Introduce version and timestamp information in the oops header.
> oops_log_info (oops header) holds version (to distinguish between old
> and new format oops header), length of the oops text
> (compressed or uncompressed) and timestamp.

This needs a much more detailed explanation.

I think what you're doing is you're overlaying the new information so
that the version field in oops_log_info sits in the same location as the
length field in the old format. And then you're defining the version to
be a value that is an illegal length.

So existing tools will refuse to dump new style partitions,
because they'll think the length is too large. You've tested that?

Updated tools will know about both formats, so will be able to handle
either old or new style partitions.

Is that correct?

And we're adding the timestamp just because we can and it'd be nice to
have?

cheers

Re: [PATCH 2/8] Add version and timestamp to oops header

[aruna]

On Monday 15 April 2013 01:01 PM, Michael Ellerman wrote:
> On Wed, Apr 10, 2013 at 12:51:12PM +0530, Aruna Balakrishnaiah wrote:
>> Introduce version and timestamp information in the oops header.
>> oops_log_info (oops header) holds version (to distinguish between old
>> and new format oops header), length of the oops text
>> (compressed or uncompressed) and timestamp.
> This needs a much more detailed explanation.
>
> I think what you're doing is you're overlaying the new information so
> that the version field in oops_log_info sits in the same location as the
> length field in the old format. And then you're defining the version to
> be a value that is an illegal length.

Thats right.

> So existing tools will refuse to dump new style partitions,
> because they'll think the length is too large. You've tested that?

Yeah, I have tested that.

>
> Updated tools will know about both formats, so will be able to handle
> either old or new style partitions.
>
> Is that correct?

Yeah, thats correct.

>
> And we're adding the timestamp just because we can and it'd be nice to
> have?

Thats right. And also, the main reason behind adding timestamp is
it will be used when we create a pstore file for oops messages.
The pstore file's timestamp will reflect the timestamp in the oops-header
added during the crash.

> cheers
>

[PATCH 3/8] Introduce generic read function to read nvram-partitions

[Aruna Balakrishnaiah]

Introduce generic read function to read nvram partitions other than rtas.
nvram_read_error_log will be retained which is used to read rtas partition
from rtasd. nvram_read_partition is the generic read function to read from
any nvram partition.

Signed-off-by: Aruna Balakrishnaiah <aruna@linux.vnet.ibm.com>
Reviewed-by: Jim Keniston <jkenisto@us.ibm.com>
---
 arch/powerpc/platforms/pseries/nvram.c |   34 +++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/nvram.c b/arch/powerpc/platforms/pseries/nvram.c
index 742735a..6701b71 100644
--- a/arch/powerpc/platforms/pseries/nvram.c
+++ b/arch/powerpc/platforms/pseries/nvram.c
@@ -293,34 +293,37 @@ int nvram_write_error_log(char * buff, int length,
 	return rc;
 }
 
-/* nvram_read_error_log
+/* nvram_read_partition
  *
- * Reads nvram for error log for at most 'length'
+ * Reads nvram partition for at most 'length'
  */
-int nvram_read_error_log(char * buff, int length,
-                         unsigned int * err_type, unsigned int * error_log_cnt)
+int nvram_read_partition(struct nvram_os_partition *part, char *buff,
+			int length, unsigned int *err_type,
+			unsigned int *error_log_cnt)
 {
 	int rc;
 	loff_t tmp_index;
 	struct err_log_info info;
 	
-	if (rtas_log_partition.index == -1)
+	if (part->index == -1)
 		return -1;
 
-	if (length > rtas_log_partition.size)
-		length = rtas_log_partition.size;
+	if (length > part->size)
+		length = part->size;
 
-	tmp_index = rtas_log_partition.index;
+	tmp_index = part->index;
 
 	rc = ppc_md.nvram_read((char *)&info, sizeof(struct err_log_info), &tmp_index);
 	if (rc <= 0) {
-		printk(KERN_ERR "nvram_read_error_log: Failed nvram_read (%d)\n", rc);
+		printk(KERN_ERR "nvram_read_partition: "
+				"Failed nvram_read (%d)\n", rc);
 		return rc;
 	}
 
 	rc = ppc_md.nvram_read(buff, length, &tmp_index);
 	if (rc <= 0) {
-		printk(KERN_ERR "nvram_read_error_log: Failed nvram_read (%d)\n", rc);
+		printk(KERN_ERR "nvram_read_partition: "
+				"Failed nvram_read (%d)\n", rc);
 		return rc;
 	}
 
@@ -330,6 +333,17 @@ int nvram_read_error_log(char * buff, int length,
 	return 0;
 }
 
+/* nvram_read_error_log
+ *
+ * Reads nvram for error log for at most 'length'
+ */
+int nvram_read_error_log(char *buff, int length,
+			unsigned int *err_type, unsigned int *error_log_cnt)
+{
+	return nvram_read_partition(&rtas_log_partition, buff, length,
+						err_type, error_log_cnt);
+}
+
 /* This doesn't actually zero anything, but it sets the event_logged
  * word to tell that this event is safely in syslog.
  */

Re: [PATCH 3/8] Introduce generic read function to read nvram-partitions

[Michael Ellerman]

On Wed, Apr 10, 2013 at 12:51:25PM +0530, Aruna Balakrishnaiah wrote:
> Introduce generic read function to read nvram partitions other than rtas.
> nvram_read_error_log will be retained which is used to read rtas partition
> from rtasd. nvram_read_partition is the generic read function to read from
> any nvram partition.
> 
> Signed-off-by: Aruna Balakrishnaiah <aruna@linux.vnet.ibm.com>
> Reviewed-by: Jim Keniston <jkenisto@us.ibm.com>
> ---
>  arch/powerpc/platforms/pseries/nvram.c |   34 +++++++++++++++++++++++---------
>  1 file changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/nvram.c b/arch/powerpc/platforms/pseries/nvram.c
> index 742735a..6701b71 100644
> --- a/arch/powerpc/platforms/pseries/nvram.c
> +++ b/arch/powerpc/platforms/pseries/nvram.c
> @@ -293,34 +293,37 @@ int nvram_write_error_log(char * buff, int length,
>  	return rc;
>  }
>  
> -/* nvram_read_error_log
> +/* nvram_read_partition
>   *
> - * Reads nvram for error log for at most 'length'
> + * Reads nvram partition for at most 'length'
>   */
> -int nvram_read_error_log(char * buff, int length,
> -                         unsigned int * err_type, unsigned int * error_log_cnt)
> +int nvram_read_partition(struct nvram_os_partition *part, char *buff,
> +			int length, unsigned int *err_type,
> +			unsigned int *error_log_cnt)
>  {
>  	int rc;
>  	loff_t tmp_index;
>  	struct err_log_info info;
>  	
> -	if (rtas_log_partition.index == -1)
> +	if (part->index == -1)
>  		return -1;
>  
> -	if (length > rtas_log_partition.size)
> -		length = rtas_log_partition.size;
> +	if (length > part->size)
> +		length = part->size;
>  
> -	tmp_index = rtas_log_partition.index;
> +	tmp_index = part->index;
>  
>  	rc = ppc_md.nvram_read((char *)&info, sizeof(struct err_log_info), &tmp_index);
>  	if (rc <= 0) {
> -		printk(KERN_ERR "nvram_read_error_log: Failed nvram_read (%d)\n", rc);
> +		printk(KERN_ERR "nvram_read_partition: "
> +				"Failed nvram_read (%d)\n", rc);

Should be:
	pr_err("%s: Failed ..\n", __FUNCTION__, ..)

cheers

[PATCH 4/8] Read/Write oops nvram partition via pstore

[Aruna Balakrishnaiah]

This patch exploits pstore infrastructure in power systems.
IBM's system p machines provide persistent storage for LPARs
through NVRAM. NVRAM's lnx,oops-log partition is used to log
oops messages. In case pstore registration fails it will
fall back to kmsg_dump mechanism.

This patch will read/write the oops messages from/to this
partition via pstore.

Signed-off-by: Jim Keniston <jkenisto@us.ibm.com>
Signed-off-by: Aruna Balakrishnaiah <aruna@linux.vnet.ibm.com>
---
 arch/powerpc/platforms/pseries/nvram.c |  145 ++++++++++++++++++++++++++++++++
 1 file changed, 145 insertions(+)

diff --git a/arch/powerpc/platforms/pseries/nvram.c b/arch/powerpc/platforms/pseries/nvram.c
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);
+
+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
+static enum pstore_type_id nvram_type_ids[] = {
+	PSTORE_TYPE_DMESG,
+	-1
+};
+static int read_type;
+#endif
 /* 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 {
+		/*TODO: Support compression when pstore is configured */
+		pr_info("nvram: Compression of oops text supported only when "
+				"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)
+{
+	read_type = -1;
+	return 0;
+}
+
+/*
+ * Called by pstore_dump() when an oops or panic report is logged to the printk
+ * buffer. @size bytes have been written to oops_buf, starting after the
+ * oops_log_info header.
+ */
+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);
+	oops_hdr->version = OOPS_HDR_VERSION;
+	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);
+	*id = part;
+
+	return 0;
+}
+
+/*
+ * 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;
+	*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

Re: [PATCH 4/8] Read/Write oops nvram partition via pstore

[Michael Ellerman]

On Wed, Apr 10, 2013 at 12:53:03PM +0530, Aruna Balakrishnaiah wrote:
> This patch exploits pstore infrastructure in power systems.
> IBM's system p machines provide persistent storage for LPARs

In the kernel we use "pseries" instead of "system p".

> through NVRAM. NVRAM's lnx,oops-log partition is used to log
> oops messages. In case pstore registration fails it will
> fall back to kmsg_dump mechanism.

What are the implications of falling back to kmsg_dump()?


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.c
> 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);

I think you should be able to rearrange this so that you don't need the
forward declarations.

> +
> +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

If we are going to have CONFIG_PSTORE #ifdefs in this file, I don't see
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[] = {
> +	PSTORE_TYPE_DMESG,
> +	-1
> +};
> +static int read_type;

I don't understand what you're doing with read_type. It looks fishy.

> +#endif
>  /* 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;

You don't need the goto.

> +	} else {
> +		/*TODO: Support compression when pstore is configured */

What is the issue here?

> +		pr_info("nvram: Compression of oops text supported only when "
> +				"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

Same comment about too many ifdefs.

> +static int nvram_pstore_open(struct pstore_info *psi)
> +{
> +	read_type = -1;

Locking?

> +	return 0;
> +}
> +
> +/*

Make it a kernel-doc style comment.

> + * Called by pstore_dump() when an oops or panic report is logged to the printk
> + * buffer. @size bytes have been written to oops_buf, starting after the
> + * oops_log_info header.

"@size bytes have", or "@size bytes should be written"?

> + */
> +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);

Why would we be called with the wrong type? Would it be better to just
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;
> +	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 definitely don't need the (void). But more to the point why aren't
you checking the return value?

> +	*id = part;

What is this? Part of the API?

> +	return 0;
> +}
> +
> +/*
> + * 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;

Can't you just cast in the call to nvram_read_partition() ?

> +	oops_hdr = (struct oops_log_info *)buff;
> +	*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

I don't understand why we have empty versions of these. If CONFIG_PSTORE
is disabled we should just not register with pstore at all.

cheers

Re: [PATCH 4/8] Read/Write oops nvram partition via pstore

[Aruna Balakrishnaiah]

Hi Michael,

Thanks for reviewing my patches.

On Monday 15 April 2013 01:25 PM, Michael Ellerman wrote:
> On Wed, Apr 10, 2013 at 12:53:03PM +0530, Aruna Balakrishnaiah wrote:
>> This patch exploits pstore infrastructure in power systems.
>> IBM's system p machines provide persistent storage for LPARs
> In the kernel we use "pseries" instead of "system p".
>

Sure, will change it.

>> through NVRAM. NVRAM's lnx,oops-log partition is used to log
>> oops messages. In case pstore registration fails it will
>> fall back to kmsg_dump mechanism.
> What are the implications of falling back to kmsg_dump()?
>

Logging oops messages to nvram should not fail in case pstore registration
fails. So it falls back to existing kmsg_dump infrastructure where
oops_to_nvram will be called. The users would need to use existing tools
to read nvram data as it is now.

> Is there any reason we would not want to enable CONFIG_PSTORE ? ie.
> should the pseries platform just select it?

Since current patchset does not support compression, selecting PSTORE by
default will lose the existing compression feature.
Once the compression feature for PSTORE is in place we can make PSTORE as
default on power.

I will post the compression patches soon. The reason for posting it
separately is stated below.

>> diff --git a/arch/powerpc/platforms/pseries/nvram.c b/arch/powerpc/platforms/pseries/nvram.c
>> 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);
> I think you should be able to rearrange this so that you don't need the
> forward declarations.

Sure. This would result in moving the callback functions just before pstore
registration for which I need to move clobbering_unread_rtas_event() which
is used by nvram_pstore_write.

>> +
>> +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
> If we are going to have CONFIG_PSTORE #ifdefs in this file, I don't see
> why there can't be just a single block of code that is #ifdef'ed, rather
> than several like you have.

Sure. I will have one #ifdef for declarations and one for function
definitions.

>> +static enum pstore_type_id nvram_type_ids[] = {
>> +	PSTORE_TYPE_DMESG,
>> +	-1
>> +};
>> +static int read_type;
> I don't understand what you're doing with read_type. It looks fishy.

read_type is an iterator to traverse the partition types. It is to know
which partition we need to read.

>> +#endif
>>   /* 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;
> You don't need the goto.

Yeah, my bad. Will fix it.

>> +	} else {
>> +		/*TODO: Support compression when pstore is configured */
> What is the issue here?
>

Currently with this patchset, pstore is not supporting compression of oops-messages
since it involves some changes in the pstore framework.

big_oops_buf will hold the large part of oops data which will be compressed and put
to oops_buf.

big_oops_buf: (1.45 of oops_partition_size)
_________________________
|  header |   oops-text |
|_________|_____________|

<header> is added by the pstore.

So in case compression fails:

we would need to log the header + last few bytes of big_oops_buf to oops_buf.
oops_buf: (this is of oops_partition_size)

we need last few bytes of big_oops_buf as we need to log the recent messages of
printk buffer. For which we need to know the header size and it involves some
changes in the pstore framework.

I have the compression patches ready, will be posting it soon as a separate set.

>> +		pr_info("nvram: Compression of oops text supported only when "
>> +				"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
> Same comment about too many ifdefs.
>

I will reduce some of the #ifdefs, but its necessary to have one for
declarations and one for function definitions.

For instance: nvram_pstore_write calls nvram_write_os_partition.
So callback function definitions should be after some of the function
definitions which it will make use of.

>> +static int nvram_pstore_open(struct pstore_info *psi)
>> +{
>> +	read_type = -1;
> Locking?

We need to reset read_type for a remount case. When a umount and mount
is done nvram_pstore_read needs to know that it has to start reading
the partitions again. So we reset the iterator.

>> +	return 0;
>> +}
>> +
>> +/*
> Make it a kernel-doc style comment.

Sure.

>> + * Called by pstore_dump() when an oops or panic report is logged to the printk
>> + * buffer. @size bytes have been written to oops_buf, starting after the
>> + * oops_log_info header.
> "@size bytes have", or "@size bytes should be written"?

Sure.

>> + */
>> +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);
> Why would we be called with the wrong type? Would it be better to just
> 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.

Yeah right. Will return  an error instead of bugging here.

>> +	oops_hdr->version = OOPS_HDR_VERSION;
>> +	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 definitely don't need the (void). But more to the point why aren't
> you checking the return value?

I will check for the return value here.

>> +	*id = part;
> What is this? Part of the API?

Yeah this is part of the API.

>> +	return 0;
>> +}
>> +
>> +/*
>> + * 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;
> Can't you just cast in the call to nvram_read_partition() ?

Thats right, my bad will fix it.

>> +	oops_hdr = (struct oops_log_info *)buff;
>> +	*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
> I don't understand why we have empty versions of these. If CONFIG_PSTORE
> is disabled we should just not register with pstore at all.

Since pstore handles the case by returning an error code when pstore is not
configured I thought of avoiding one more #ifdef during registration.
I will remove empty callbacks and add a #ifdef during registration itself.

> cheers
>

Re: [PATCH 4/8] Read/Write oops nvram partition via pstore

[Benjamin Herrenschmidt]

On Tue, 2013-04-16 at 11:50 +0530, Aruna Balakrishnaiah wrote:
> 
> Sure. I will have one #ifdef for declarations and one for function
> definitions.

Declarations generally don't need #ifdef's

Cheers,
Ben.
 

Re: [PATCH 4/8] Read/Write oops nvram partition via pstore

[Aruna Balakrishnaiah]

On Tuesday 16 April 2013 12:44 PM, Benjamin Herrenschmidt wrote:
> On Tue, 2013-04-16 at 11:50 +0530, Aruna Balakrishnaiah wrote:
>> Sure. I will have one #ifdef for declarations and one for function
>> definitions.
> Declarations generally don't need #ifdef's

Sorry by declarations I meant variable declarations (used by pstore) not function
declarations.

> Cheers,
> Ben.
>
>

Re: [PATCH 4/8] Read/Write oops nvram partition via pstore

[Aruna Balakrishnaiah]

On Tuesday 16 April 2013 11:50 AM, Aruna Balakrishnaiah wrote:
>
> Currently with this patchset, pstore is not supporting compression of 
> oops-messages
> since it involves some changes in the pstore framework.
>
> big_oops_buf will hold the large part of oops data which will be compressed 
> and put
> to oops_buf.
>
> big_oops_buf: (1.45 of oops_partition_size)

Sorry, big_oops_buf is (2.22 of oops_data_sz)

where oops_data_sz is oops_partition_size - sizeof(oops_log_info).

where oops_log_info is oops header.

> _________________________
> |  header |   oops-text |
> |_________|_____________|
>
> <header> is added by the pstore.
>
> So in case compression fails:
>
> we would need to log the header + last few bytes of big_oops_buf to oops_buf.
> oops_buf: (this is of oops_partition_size)
>

We would need to log the header + last oops_data_sz bytes of big_oops_buf to 
oops_buf.
So that we can have the header while throwing away the data that immediately
follows it.

> we need last few bytes of big_oops_buf as we need to log the recent messages of
> printk buffer. For which we need to know the header size and it involves some
> changes in the pstore framework.
>

Just communicating the header size from pstore would do the job for us.

> I have the compression patches ready, will be posting it soon as a separate set.
>
>> cheers
>>
>

[PATCH 5/8] Read rtas partition via pstore

[Aruna Balakrishnaiah]

This patch exploits pstore infrastructure to read the details
from NVRAM's rtas partition.

Signed-off-by: Aruna Balakrishnaiah <aruna@linux.vnet.ibm.com>
Reviewed-by: Jim Keniston <jkenisto@us.ibm.com>
---
 arch/powerpc/platforms/pseries/nvram.c |   33 +++++++++++++++++++++++++-------
 fs/pstore/inode.c                      |    3 +++
 include/linux/pstore.h                 |    2 ++
 3 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/nvram.c b/arch/powerpc/platforms/pseries/nvram.c
index 82d32a2..d420b1d 100644
--- a/arch/powerpc/platforms/pseries/nvram.c
+++ b/arch/powerpc/platforms/pseries/nvram.c
@@ -144,9 +144,11 @@ static size_t oops_data_sz;
 #ifdef CONFIG_PSTORE
 static enum pstore_type_id nvram_type_ids[] = {
 	PSTORE_TYPE_DMESG,
+	PSTORE_TYPE_RTAS,
 	-1
 };
 static int read_type;
+static unsigned long last_rtas_event;
 #endif
 /* Compression parameters */
 #define COMPR_LEVEL 6
@@ -315,8 +317,13 @@ int nvram_write_error_log(char * buff, int length,
 {
 	int rc = nvram_write_os_partition(&rtas_log_partition, buff, length,
 						err_type, error_log_cnt);
-	if (!rc)
+	if (!rc) {
 		last_unread_rtas_event = get_seconds();
+#ifdef CONFIG_PSTORE
+		last_rtas_event = get_seconds();
+#endif
+	}
+
 	return rc;
 }
 
@@ -745,7 +752,7 @@ static int nvram_pstore_write(enum pstore_type_id type,
 }
 
 /*
- * Reads the oops/panic report.
+ * Reads the oops/panic report and ibm,rtas-log partition.
  * Returns the length of the data we read from each partition.
  * Returns 0 if we've been called before.
  */
@@ -765,6 +772,12 @@ static ssize_t nvram_pstore_read(u64 *id, enum pstore_type_id *type,
 		part = &oops_log_partition;
 		*type = PSTORE_TYPE_DMESG;
 		break;
+	case PSTORE_TYPE_RTAS:
+		part = &rtas_log_partition;
+		*type = PSTORE_TYPE_RTAS;
+		time->tv_sec = last_rtas_event;
+		time->tv_nsec = 0;
+		break;
 	default:
 		return 0;
 	}
@@ -781,11 +794,17 @@ static ssize_t nvram_pstore_read(u64 *id, enum pstore_type_id *type,
 
 	*count = 0;
 	*id = id_no;
-	oops_hdr = (struct oops_log_info *)buff;
-	*buf = buff + sizeof(*oops_hdr);
-	time->tv_sec = oops_hdr->timestamp;
-	time->tv_nsec = 0;
-	return oops_hdr->report_length;
+
+	if (nvram_type_ids[read_type] == PSTORE_TYPE_DMESG) {
+		oops_hdr = (struct oops_log_info *)buff;
+		*buf = buff + sizeof(*oops_hdr);
+		time->tv_sec = oops_hdr->timestamp;
+		time->tv_nsec = 0;
+		return oops_hdr->report_length;
+	}
+
+	*buf = buff;
+	return part->size;
 }
 #else
 static int nvram_pstore_open(struct pstore_info *psi)
diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index e4bcb2c..59b1454 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -324,6 +324,9 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count,
 	case PSTORE_TYPE_MCE:
 		sprintf(name, "mce-%s-%lld", psname, id);
 		break;
+	case PSTORE_TYPE_RTAS:
+		sprintf(name, "rtas-%s-%lld", psname, id);
+		break;
 	case PSTORE_TYPE_UNKNOWN:
 		sprintf(name, "unknown-%s-%lld", psname, id);
 		break;
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index 75d0176..4eb94c9 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -35,6 +35,8 @@ enum pstore_type_id {
 	PSTORE_TYPE_MCE		= 1,
 	PSTORE_TYPE_CONSOLE	= 2,
 	PSTORE_TYPE_FTRACE	= 3,
+	/* PPC64 partition types */
+	PSTORE_TYPE_RTAS	= 10,
 	PSTORE_TYPE_UNKNOWN	= 255
 };
 

Re: [PATCH 5/8] Read rtas partition via pstore

[Michael Ellerman]

On Wed, Apr 10, 2013 at 12:53:27PM +0530, Aruna Balakrishnaiah wrote:
> This patch exploits pstore infrastructure to read the details
> from NVRAM's rtas partition.

Does that mean it's exposed in the pstore filesystem?

> Signed-off-by: Aruna Balakrishnaiah <aruna@linux.vnet.ibm.com>
> Reviewed-by: Jim Keniston <jkenisto@us.ibm.com>
> ---
>  arch/powerpc/platforms/pseries/nvram.c |   33 +++++++++++++++++++++++++-------
>  fs/pstore/inode.c                      |    3 +++
>  include/linux/pstore.h                 |    2 ++
>  3 files changed, 31 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/nvram.c b/arch/powerpc/platforms/pseries/nvram.c
> index 82d32a2..d420b1d 100644
> --- a/arch/powerpc/platforms/pseries/nvram.c
> +++ b/arch/powerpc/platforms/pseries/nvram.c
> @@ -144,9 +144,11 @@ static size_t oops_data_sz;
>  #ifdef CONFIG_PSTORE
>  static enum pstore_type_id nvram_type_ids[] = {
>  	PSTORE_TYPE_DMESG,
> +	PSTORE_TYPE_RTAS,
>  	-1
>  };
>  static int read_type;
> +static unsigned long last_rtas_event;
>  #endif
>  /* Compression parameters */
>  #define COMPR_LEVEL 6
> @@ -315,8 +317,13 @@ int nvram_write_error_log(char * buff, int length,
>  {
>  	int rc = nvram_write_os_partition(&rtas_log_partition, buff, length,
>  						err_type, error_log_cnt);
> -	if (!rc)
> +	if (!rc) {
>  		last_unread_rtas_event = get_seconds();
> +#ifdef CONFIG_PSTORE
> +		last_rtas_event = get_seconds();
> +#endif
> +	}
> +
>  	return rc;
>  }
>  
> @@ -745,7 +752,7 @@ static int nvram_pstore_write(enum pstore_type_id type,
>  }
>  
>  /*
> - * Reads the oops/panic report.
> + * Reads the oops/panic report and ibm,rtas-log partition.
>   * Returns the length of the data we read from each partition.
>   * Returns 0 if we've been called before.
>   */
> @@ -765,6 +772,12 @@ static ssize_t nvram_pstore_read(u64 *id, enum pstore_type_id *type,
>  		part = &oops_log_partition;
>  		*type = PSTORE_TYPE_DMESG;
>  		break;
> +	case PSTORE_TYPE_RTAS:
> +		part = &rtas_log_partition;
> +		*type = PSTORE_TYPE_RTAS;
> +		time->tv_sec = last_rtas_event;
> +		time->tv_nsec = 0;
> +		break;
>  	default:
>  		return 0;
>  	}
> @@ -781,11 +794,17 @@ static ssize_t nvram_pstore_read(u64 *id, enum pstore_type_id *type,
>  
>  	*count = 0;
>  	*id = id_no;
> -	oops_hdr = (struct oops_log_info *)buff;
> -	*buf = buff + sizeof(*oops_hdr);
> -	time->tv_sec = oops_hdr->timestamp;
> -	time->tv_nsec = 0;
> -	return oops_hdr->report_length;
> +
> +	if (nvram_type_ids[read_type] == PSTORE_TYPE_DMESG) {
> +		oops_hdr = (struct oops_log_info *)buff;
> +		*buf = buff + sizeof(*oops_hdr);
> +		time->tv_sec = oops_hdr->timestamp;
> +		time->tv_nsec = 0;
> +		return oops_hdr->report_length;
> +	}
> +
> +	*buf = buff;
> +	return part->size;
>  }
>  #else
>  static int nvram_pstore_open(struct pstore_info *psi)
> diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
> index e4bcb2c..59b1454 100644
> --- a/fs/pstore/inode.c
> +++ b/fs/pstore/inode.c
> @@ -324,6 +324,9 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count,
>  	case PSTORE_TYPE_MCE:
>  		sprintf(name, "mce-%s-%lld", psname, id);
>  		break;
> +	case PSTORE_TYPE_RTAS:
> +		sprintf(name, "rtas-%s-%lld", psname, id);
> +		break;
>  	case PSTORE_TYPE_UNKNOWN:
>  		sprintf(name, "unknown-%s-%lld", psname, id);
>  		break;
> diff --git a/include/linux/pstore.h b/include/linux/pstore.h
> index 75d0176..4eb94c9 100644
> --- a/include/linux/pstore.h
> +++ b/include/linux/pstore.h
> @@ -35,6 +35,8 @@ enum pstore_type_id {
>  	PSTORE_TYPE_MCE		= 1,
>  	PSTORE_TYPE_CONSOLE	= 2,
>  	PSTORE_TYPE_FTRACE	= 3,
> +	/* PPC64 partition types */
> +	PSTORE_TYPE_RTAS	= 10,
>  	PSTORE_TYPE_UNKNOWN	= 255

I think you should probably just continue at 4, and call it
PSTORE_TYPE_PPC_RTAS. But you must get an ACK from the pstore
maintainers for this and the previous hunk, and I don't see them on CC.

cheers

Re: [PATCH 5/8] Read rtas partition via pstore

[Aruna Balakrishnaiah]

On Monday 15 April 2013 01:31 PM, Michael Ellerman wrote:
> On Wed, Apr 10, 2013 at 12:53:27PM +0530, Aruna Balakrishnaiah wrote:
>> This patch exploits pstore infrastructure to read the details
>> from NVRAM's rtas partition.
> Does that mean it's exposed in the pstore filesystem?

Yeah thats right.

>> Signed-off-by: Aruna Balakrishnaiah <aruna@linux.vnet.ibm.com>
>> Reviewed-by: Jim Keniston <jkenisto@us.ibm.com>
>> ---
>>   arch/powerpc/platforms/pseries/nvram.c |   33 +++++++++++++++++++++++++-------
>>   fs/pstore/inode.c                      |    3 +++
>>   include/linux/pstore.h                 |    2 ++
>>   3 files changed, 31 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/pseries/nvram.c b/arch/powerpc/platforms/pseries/nvram.c
>> index 82d32a2..d420b1d 100644
>> --- a/arch/powerpc/platforms/pseries/nvram.c
>> +++ b/arch/powerpc/platforms/pseries/nvram.c
>> @@ -144,9 +144,11 @@ static size_t oops_data_sz;
>>   #ifdef CONFIG_PSTORE
>>   static enum pstore_type_id nvram_type_ids[] = {
>>   	PSTORE_TYPE_DMESG,
>> +	PSTORE_TYPE_RTAS,
>>   	-1
>>   };
>>   static int read_type;
>> +static unsigned long last_rtas_event;
>>   #endif
>>   /* Compression parameters */
>>   #define COMPR_LEVEL 6
>> @@ -315,8 +317,13 @@ int nvram_write_error_log(char * buff, int length,
>>   {
>>   	int rc = nvram_write_os_partition(&rtas_log_partition, buff, length,
>>   						err_type, error_log_cnt);
>> -	if (!rc)
>> +	if (!rc) {
>>   		last_unread_rtas_event = get_seconds();
>> +#ifdef CONFIG_PSTORE
>> +		last_rtas_event = get_seconds();
>> +#endif
>> +	}
>> +
>>   	return rc;
>>   }
>>   
>> @@ -745,7 +752,7 @@ static int nvram_pstore_write(enum pstore_type_id type,
>>   }
>>   
>>   /*
>> - * Reads the oops/panic report.
>> + * Reads the oops/panic report and ibm,rtas-log partition.
>>    * Returns the length of the data we read from each partition.
>>    * Returns 0 if we've been called before.
>>    */
>> @@ -765,6 +772,12 @@ static ssize_t nvram_pstore_read(u64 *id, enum pstore_type_id *type,
>>   		part = &oops_log_partition;
>>   		*type = PSTORE_TYPE_DMESG;
>>   		break;
>> +	case PSTORE_TYPE_RTAS:
>> +		part = &rtas_log_partition;
>> +		*type = PSTORE_TYPE_RTAS;
>> +		time->tv_sec = last_rtas_event;
>> +		time->tv_nsec = 0;
>> +		break;
>>   	default:
>>   		return 0;
>>   	}
>> @@ -781,11 +794,17 @@ static ssize_t nvram_pstore_read(u64 *id, enum pstore_type_id *type,
>>   
>>   	*count = 0;
>>   	*id = id_no;
>> -	oops_hdr = (struct oops_log_info *)buff;
>> -	*buf = buff + sizeof(*oops_hdr);
>> -	time->tv_sec = oops_hdr->timestamp;
>> -	time->tv_nsec = 0;
>> -	return oops_hdr->report_length;
>> +
>> +	if (nvram_type_ids[read_type] == PSTORE_TYPE_DMESG) {
>> +		oops_hdr = (struct oops_log_info *)buff;
>> +		*buf = buff + sizeof(*oops_hdr);
>> +		time->tv_sec = oops_hdr->timestamp;
>> +		time->tv_nsec = 0;
>> +		return oops_hdr->report_length;
>> +	}
>> +
>> +	*buf = buff;
>> +	return part->size;
>>   }
>>   #else
>>   static int nvram_pstore_open(struct pstore_info *psi)
>> diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
>> index e4bcb2c..59b1454 100644
>> --- a/fs/pstore/inode.c
>> +++ b/fs/pstore/inode.c
>> @@ -324,6 +324,9 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count,
>>   	case PSTORE_TYPE_MCE:
>>   		sprintf(name, "mce-%s-%lld", psname, id);
>>   		break;
>> +	case PSTORE_TYPE_RTAS:
>> +		sprintf(name, "rtas-%s-%lld", psname, id);
>> +		break;
>>   	case PSTORE_TYPE_UNKNOWN:
>>   		sprintf(name, "unknown-%s-%lld", psname, id);
>>   		break;
>> diff --git a/include/linux/pstore.h b/include/linux/pstore.h
>> index 75d0176..4eb94c9 100644
>> --- a/include/linux/pstore.h
>> +++ b/include/linux/pstore.h
>> @@ -35,6 +35,8 @@ enum pstore_type_id {
>>   	PSTORE_TYPE_MCE		= 1,
>>   	PSTORE_TYPE_CONSOLE	= 2,
>>   	PSTORE_TYPE_FTRACE	= 3,
>> +	/* PPC64 partition types */
>> +	PSTORE_TYPE_RTAS	= 10,
>>   	PSTORE_TYPE_UNKNOWN	= 255
> I think you should probably just continue at 4, and call it
> PSTORE_TYPE_PPC_RTAS. But you must get an ACK from the pstore
> maintainers for this and the previous hunk, and I don't see them on CC.

Sure, will add them on cc in my next version of the patches.

>
> cheers
>

[PATCH 6/8] Distinguish between a os-partition and non-os partition

[Aruna Balakrishnaiah]

Introduce os_partition member in nvram_os_partition structure
to identify if the partition is an os partition or not. This
will be useful to handle non-os partitions of-config and
common in subsequent patches.

Signed-off-by: Aruna Balakrishnaiah <aruna@linux.vnet.ibm.com>
Reviewed-by: Jim Keniston <jkenisto@us.ibm.com>
---
 arch/powerpc/platforms/pseries/nvram.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/nvram.c b/arch/powerpc/platforms/pseries/nvram.c
index d420b1d..6a3a7cd 100644
--- a/arch/powerpc/platforms/pseries/nvram.c
+++ b/arch/powerpc/platforms/pseries/nvram.c
@@ -53,20 +53,23 @@ struct nvram_os_partition {
 	int min_size;	/* minimum acceptable size (0 means req_size) */
 	long size;	/* size of data portion (excluding err_log_info) */
 	long index;	/* offset of data portion of partition */
+	bool os_partition; /* partition initialized by OS, not FW */
 };
 
 static struct nvram_os_partition rtas_log_partition = {
 	.name = "ibm,rtas-log",
 	.req_size = 2079,
 	.min_size = 1055,
-	.index = -1
+	.index = -1,
+	.os_partition = true
 };
 
 static struct nvram_os_partition oops_log_partition = {
 	.name = "lnx,oops-log",
 	.req_size = 4000,
 	.min_size = 2000,
-	.index = -1
+	.index = -1,
+	.os_partition = true
 };
 
 static const char *pseries_nvram_os_partitions[] = {

[PATCH 7/8] Read of-config partition via pstore

[Aruna Balakrishnaiah]

This patch exploits pstore infrastructure to read the details
from NVRAM's of-config partition.

Signed-off-by: Aruna Balakrishnaiah <aruna@linux.vnet.ibm.com>
Reviewed-by: Jim Keniston <jkenisto@us.ibm.com>
---
 arch/powerpc/platforms/pseries/nvram.c |   58 ++++++++++++++++++++++++++------
 fs/pstore/inode.c                      |    3 ++
 include/linux/pstore.h                 |    1 +
 3 files changed, 52 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/nvram.c b/arch/powerpc/platforms/pseries/nvram.c
index 6a3a7cd..b65a670 100644
--- a/arch/powerpc/platforms/pseries/nvram.c
+++ b/arch/powerpc/platforms/pseries/nvram.c
@@ -78,6 +78,14 @@ static const char *pseries_nvram_os_partitions[] = {
 	NULL
 };
 
+#ifdef CONFIG_PSTORE
+static struct nvram_os_partition of_config_partition = {
+	.name = "of-config",
+	.index = -1,
+	.os_partition = false
+};
+#endif
+
 struct oops_log_info {
 	u16 version;
 	u16 report_length;
@@ -148,6 +156,7 @@ static size_t oops_data_sz;
 static enum pstore_type_id nvram_type_ids[] = {
 	PSTORE_TYPE_DMESG,
 	PSTORE_TYPE_RTAS,
+	PSTORE_TYPE_OF,
 	-1
 };
 static int read_type;
@@ -350,11 +359,15 @@ int nvram_read_partition(struct nvram_os_partition *part, char *buff,
 
 	tmp_index = part->index;
 
-	rc = ppc_md.nvram_read((char *)&info, sizeof(struct err_log_info), &tmp_index);
-	if (rc <= 0) {
-		printk(KERN_ERR "nvram_read_partition: "
-				"Failed nvram_read (%d)\n", rc);
-		return rc;
+	if (part->os_partition) {
+		rc = ppc_md.nvram_read((char *)&info,
+					sizeof(struct err_log_info),
+					&tmp_index);
+		if (rc <= 0) {
+			printk(KERN_ERR "nvram_read_partition: "
+					"Failed nvram_read (%d)\n", rc);
+			return rc;
+		}
 	}
 
 	rc = ppc_md.nvram_read(buff, length, &tmp_index);
@@ -364,8 +377,10 @@ int nvram_read_partition(struct nvram_os_partition *part, char *buff,
 		return rc;
 	}
 
-	*error_log_cnt = info.seq_num;
-	*err_type = info.error_type;
+	if (part->os_partition) {
+		*error_log_cnt = info.seq_num;
+		*err_type = info.error_type;
+	}
 
 	return 0;
 }
@@ -755,7 +770,7 @@ static int nvram_pstore_write(enum pstore_type_id type,
 }
 
 /*
- * Reads the oops/panic report and ibm,rtas-log partition.
+ * Reads the oops/panic report, rtas-log and of-config partition.
  * Returns the length of the data we read from each partition.
  * Returns 0 if we've been called before.
  */
@@ -764,9 +779,11 @@ static ssize_t nvram_pstore_read(u64 *id, enum pstore_type_id *type,
 				struct pstore_info *psi)
 {
 	struct oops_log_info *oops_hdr;
-	unsigned int err_type, id_no;
+	unsigned int err_type, id_no, size = 0;
 	struct nvram_os_partition *part = NULL;
 	char *buff = NULL;
+	int sig = 0;
+	loff_t p;
 
 	read_type++;
 
@@ -781,10 +798,29 @@ static ssize_t nvram_pstore_read(u64 *id, enum pstore_type_id *type,
 		time->tv_sec = last_rtas_event;
 		time->tv_nsec = 0;
 		break;
+	case PSTORE_TYPE_OF:
+		sig = NVRAM_SIG_OF;
+		part = &of_config_partition;
+		*type = PSTORE_TYPE_OF;
+		*id = PSTORE_TYPE_OF;
+		time->tv_sec = 0;
+		time->tv_nsec = 0;
+		break;
 	default:
 		return 0;
 	}
 
+	if (!part->os_partition) {
+		p = nvram_find_partition(part->name, sig, &size);
+		if (p <= 0) {
+			pr_err("nvram: Failed to find partition %s, "
+				"err %d\n", part->name, (int)p);
+			return 0;
+		}
+		part->index = p;
+		part->size = size;
+	}
+
 	buff = kmalloc(part->size, GFP_KERNEL);
 
 	if (!buff)
@@ -796,7 +832,9 @@ static ssize_t nvram_pstore_read(u64 *id, enum pstore_type_id *type,
 	}
 
 	*count = 0;
-	*id = id_no;
+
+	if (part->os_partition)
+		*id = id_no;
 
 	if (nvram_type_ids[read_type] == PSTORE_TYPE_DMESG) {
 		oops_hdr = (struct oops_log_info *)buff;
diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index 59b1454..c3d1846 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -327,6 +327,9 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count,
 	case PSTORE_TYPE_RTAS:
 		sprintf(name, "rtas-%s-%lld", psname, id);
 		break;
+	case PSTORE_TYPE_OF:
+		sprintf(name, "of-%s-%lld", psname, id);
+		break;
 	case PSTORE_TYPE_UNKNOWN:
 		sprintf(name, "unknown-%s-%lld", psname, id);
 		break;
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index 4eb94c9..a23d7d2 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -37,6 +37,7 @@ enum pstore_type_id {
 	PSTORE_TYPE_FTRACE	= 3,
 	/* PPC64 partition types */
 	PSTORE_TYPE_RTAS	= 10,
+	PSTORE_TYPE_OF		= 11,
 	PSTORE_TYPE_UNKNOWN	= 255
 };
 

[PATCH 8/8] Read common partition via pstore

[Aruna Balakrishnaiah]

This patch exploits pstore infrastructure to read the details
from NVRAM's common partition.

Signed-off-by: Aruna Balakrishnaiah <aruna@linux.vnet.ibm.com>
Reviewed-by: Jim Keniston <jkenisto@us.ibm.com>
---
 arch/powerpc/platforms/pseries/nvram.c |   17 ++++++++++++++++-
 fs/pstore/inode.c                      |    3 +++
 include/linux/pstore.h                 |    1 +
 3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/pseries/nvram.c b/arch/powerpc/platforms/pseries/nvram.c
index b65a670..542dc7e 100644
--- a/arch/powerpc/platforms/pseries/nvram.c
+++ b/arch/powerpc/platforms/pseries/nvram.c
@@ -84,6 +84,12 @@ static struct nvram_os_partition of_config_partition = {
 	.index = -1,
 	.os_partition = false
 };
+
+static struct nvram_os_partition common_partition = {
+	.name = "common",
+	.index = -1,
+	.os_partition = false
+};
 #endif
 
 struct oops_log_info {
@@ -157,6 +163,7 @@ static enum pstore_type_id nvram_type_ids[] = {
 	PSTORE_TYPE_DMESG,
 	PSTORE_TYPE_RTAS,
 	PSTORE_TYPE_OF,
+	PSTORE_TYPE_COMMON,
 	-1
 };
 static int read_type;
@@ -770,7 +777,7 @@ static int nvram_pstore_write(enum pstore_type_id type,
 }
 
 /*
- * Reads the oops/panic report, rtas-log and of-config partition.
+ * Reads the oops/panic report, rtas-log, of-config and common partition.
  * Returns the length of the data we read from each partition.
  * Returns 0 if we've been called before.
  */
@@ -806,6 +813,14 @@ static ssize_t nvram_pstore_read(u64 *id, enum pstore_type_id *type,
 		time->tv_sec = 0;
 		time->tv_nsec = 0;
 		break;
+	case PSTORE_TYPE_COMMON:
+		sig = NVRAM_SIG_SYS;
+		part = &common_partition;
+		*type = PSTORE_TYPE_COMMON;
+		*id = PSTORE_TYPE_COMMON;
+		time->tv_sec = 0;
+		time->tv_nsec = 0;
+		break;
 	default:
 		return 0;
 	}
diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index c3d1846..11cae64 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -330,6 +330,9 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count,
 	case PSTORE_TYPE_OF:
 		sprintf(name, "of-%s-%lld", psname, id);
 		break;
+	case PSTORE_TYPE_COMMON:
+		sprintf(name, "common-%s-%lld", psname, id);
+		break;
 	case PSTORE_TYPE_UNKNOWN:
 		sprintf(name, "unknown-%s-%lld", psname, id);
 		break;
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index a23d7d2..08224c2 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -38,6 +38,7 @@ enum pstore_type_id {
 	/* PPC64 partition types */
 	PSTORE_TYPE_RTAS	= 10,
 	PSTORE_TYPE_OF		= 11,
+	PSTORE_TYPE_COMMON	= 12,
 	PSTORE_TYPE_UNKNOWN	= 255
 };
 

Re: [PATCH 0/8] Nvram-to-pstore

[Michael Ellerman]

On Wed, Apr 10, 2013 at 12:50:47PM +0530, Aruna Balakrishnaiah wrote:
> Currently the kernel provides the contents of p-series NVRAM only as a
> simple stream of bytes via /dev/nvram, which must be interpreted in user
> space by the nvram command in the powerpc-utils package.  This patch set
> exploits the pstore subsystem to expose each partition in NVRAM as a
> separate file in /dev/pstore. For instance Oops messages will stored in a
> file named [dmesg-nvram-2].

Please try to fold some of that info into the commit messages for actual
patches. The 0th patch is lost when we commit the series into git.

Also all your patches should have a subject starting with
"powerpc/pseries:".

cheers