[PATCH v4 2nd 0/4] pstore: ramoops: support multiple pmsg instances

[PATCH v4 2nd 0/4] pstore: ramoops: support multiple pmsg instances

[Nobuhiro Iwamatsu]

The following series implements multiple pmsg. This feature allows userspace
program to control individual content aging or priority.

If a pstore backend module(e.g. ramoops) requires the multiple pmsg instances
when registering itself to pstore, multiple /dev/pmsg[ID] are created. Writes
to each /dev/pmsg[ID] are isolated each other. After reboot, the contents are
available in /sys/fs/pstore/pmsg-[backend]-[ID].

In addition, we add multiple pmsg support for ramoops. We can specify multiple
pmsg area size by its module parameter as follows.

 pmsg_size=0x1000,0x2000,...

I did check the operation of this feature on CycloneV (socfpga) Helio board.

v4:
  Rebase to 4.10-rc5
  The following patches have been removed from this series as similar functions
  were modified by other commit.
     - pstore: Replace four kzalloc() calls by kcalloc() in ramoops_init_przs()
     - pstore: Change parameter of ramoops_free_przs()
     - pstore: Rename 'przs' to 'dprzs' in struct ramoops_context
     - ramoops: Rename ramoops_init_prz() to ramoops_init_dprzs()

v3:
  Rebase to v4.8.
  Split patch.
  merged device_create().
  Remove Blank lines.
  Update documentiation of DT binding.
  Update parsing function of ramoops_pmsg_size, add NULL termination.
  Update module parameters for pmsg_size list.


Hiraku Toyooka (2):
  pstore: support multiple pmsg instances
  selftests/pstore: add testcases for multiple pmsg instances

Nobuhiro Iwamatsu (2):
  ramoops: Add __ramoops_init_prz() as generic function
  ramoops: support multiple pmsg instances

 Documentation/admin-guide/ramoops.rst              |  22 ++
 .../bindings/reserved-memory/ramoops.txt           |   6 +-
 fs/pstore/pmsg.c                                   |  23 +-
 fs/pstore/ram.c                                    | 286 +++++++++++++++++----
 include/linux/pstore.h                             |   1 +
 include/linux/pstore_ram.h                         |   8 +-
 tools/testing/selftests/pstore/common_tests        |  21 +-
 .../selftests/pstore/pstore_post_reboot_tests      |  27 +-
 tools/testing/selftests/pstore/pstore_tests        |  14 +-
 9 files changed, 326 insertions(+), 82 deletions(-)

-- 
2.11.0

[PATCH v4 2nd 1/4] ramoops: Add __ramoops_init_prz() as generic function

[Nobuhiro Iwamatsu]

This commit adds generic function __ramoops_init_prz() to reduce redundancy
between ramoops_init_prz() and ramoops_init_przs().

Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.kw@hitachi.com>
Signed-off-by: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
Cc: Anton Vorontsov <anton@enomsg.org>
Cc: Colin Cross <ccross@android.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Mark Salyzyn <salyzyn@android.com>
Cc: Seiji Aguchi <seiji.aguchi.tr@hitachi.com>
Cc: Shuah Khan <shuahkh@osg.samsung.com>
Cc: Tony Luck <tony.luck@intel.com>

V4:
   Rebase.

V3:
   Rebase.
   Split patch.
---
 fs/pstore/ram.c | 73 ++++++++++++++++++++++++++++++---------------------------
 1 file changed, 38 insertions(+), 35 deletions(-)

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index a412aec848ce..72072e8123e3 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -520,6 +520,40 @@ static void ramoops_free_przs(struct persistent_ram_zone **przs)
 	kfree(przs);
 }
 
+static int __ramoops_init_prz(const char *name,
+			      struct device *dev, struct ramoops_context *cxt,
+			      struct persistent_ram_zone **prz,
+			      phys_addr_t *paddr, size_t sz, u32 sig,
+			      u32 flags, bool zap)
+{
+	if (!sz)
+		return 0;
+
+	if (zap && *paddr + sz - cxt->phys_addr > cxt->size) {
+		dev_err(dev, "no room for %s mem region (0x%zx@0x%llx) in (0x%lx@0x%llx)\n",
+			name, sz, (unsigned long long)*paddr,
+			cxt->size, (unsigned long long)cxt->phys_addr);
+		return -ENOMEM;
+	}
+
+	*prz = persistent_ram_new(*paddr, sz, sig, &cxt->ecc_info,
+				  cxt->memtype, flags);
+	if (IS_ERR(*prz)) {
+		int err = PTR_ERR(*prz);
+
+		dev_err(dev, "failed to request %s mem region (0x%zx@0x%llx): %d\n",
+			name, sz, (unsigned long long)*paddr, err);
+		return err;
+	}
+
+	if (zap)
+		persistent_ram_zap(*prz);
+
+	*paddr += sz;
+
+	return 0;
+}
+
 static int ramoops_init_przs(const char *name,
 			     struct device *dev, struct ramoops_context *cxt,
 			     struct persistent_ram_zone ***przs,
@@ -580,15 +614,9 @@ static int ramoops_init_przs(const char *name,
 		goto fail;
 
 	for (i = 0; i < *cnt; i++) {
-		prz_ar[i] = persistent_ram_new(*paddr, zone_sz, sig,
-						  &cxt->ecc_info,
-						  cxt->memtype, flags);
-		if (IS_ERR(prz_ar[i])) {
-			err = PTR_ERR(prz_ar[i]);
-			dev_err(dev, "failed to request %s mem region (0x%zx@0x%llx): %d\n",
-				name, record_size,
-				(unsigned long long)*paddr, err);
-
+		err = __ramoops_init_prz(name, dev, cxt, &prz_ar[i], paddr,
+					 zone_sz, sig, flags, false);
+		if (err) {
 			while (i > 0) {
 				i--;
 				persistent_ram_free(prz_ar[i]);
@@ -596,7 +624,6 @@ static int ramoops_init_przs(const char *name,
 			kfree(prz_ar);
 			goto fail;
 		}
-		*paddr += zone_sz;
 	}
 
 	*przs = prz_ar;
@@ -612,31 +639,7 @@ static int ramoops_init_prz(const char *name,
 			    struct persistent_ram_zone **prz,
 			    phys_addr_t *paddr, size_t sz, u32 sig)
 {
-	if (!sz)
-		return 0;
-
-	if (*paddr + sz - cxt->phys_addr > cxt->size) {
-		dev_err(dev, "no room for %s mem region (0x%zx@0x%llx) in (0x%lx@0x%llx)\n",
-			name, sz, (unsigned long long)*paddr,
-			cxt->size, (unsigned long long)cxt->phys_addr);
-		return -ENOMEM;
-	}
-
-	*prz = persistent_ram_new(*paddr, sz, sig, &cxt->ecc_info,
-				  cxt->memtype, 0);
-	if (IS_ERR(*prz)) {
-		int err = PTR_ERR(*prz);
-
-		dev_err(dev, "failed to request %s mem region (0x%zx@0x%llx): %d\n",
-			name, sz, (unsigned long long)*paddr, err);
-		return err;
-	}
-
-	persistent_ram_zap(*prz);
-
-	*paddr += sz;
-
-	return 0;
+	return __ramoops_init_prz(name, dev, cxt, prz, paddr, sz, sig, 0, true);
 }
 
 static int ramoops_parse_dt_size(struct platform_device *pdev,
-- 
2.11.0

[PATCH v4 2nd 2/4] pstore: support multiple pmsg instances

[Nobuhiro Iwamatsu]

From: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>

This enables pmsg to deal with multiple instances. One possible
use is content priority control on limited persistent store space. By
using different buffers, we can prevent important messages from being
overwritten by less important messages.

When pstore backend module specifies the number of instances (num_pmsg)
in pstore_info, multiple /dev/pmsg[ID] appear. (ID is an integer
starting at 0. It corresponds to minor number of the each char device.)
Writes to each /dev/pmsg[ID] are isolated each other. After reboot, the
contents are available in /sys/fs/pstore/pmsg-[backend]-[ID].

Signed-off-by: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.kw@hitachi.com>
Cc: Anton Vorontsov <anton@enomsg.org>
Cc: Colin Cross <ccross@android.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Mark Salyzyn <salyzyn@android.com>
Cc: Seiji Aguchi <seiji.aguchi.tr@hitachi.com>
Cc: Tony Luck <tony.luck@intel.com>

V4:
    Rebase.
V3:
    rebase.
    merged device_create().
---
 fs/pstore/pmsg.c       | 23 ++++++++++++++++-------
 include/linux/pstore.h |  1 +
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/fs/pstore/pmsg.c b/fs/pstore/pmsg.c
index 78f6176c020f..5da5cba4b387 100644
--- a/fs/pstore/pmsg.c
+++ b/fs/pstore/pmsg.c
@@ -34,7 +34,8 @@ static ssize_t write_pmsg(struct file *file, const char __user *buf,
 		return -EFAULT;
 
 	mutex_lock(&pmsg_lock);
-	ret = psinfo->write_buf_user(PSTORE_TYPE_PMSG, 0, &id, 0, buf, 0, count,
+	ret = psinfo->write_buf_user(PSTORE_TYPE_PMSG, 0, &id,
+				     iminor(file->f_inode), buf, 0, count,
 				     psinfo);
 	mutex_unlock(&pmsg_lock);
 	return ret ? ret : count;
@@ -61,7 +62,7 @@ static char *pmsg_devnode(struct device *dev, umode_t *mode)
 
 void pstore_register_pmsg(void)
 {
-	struct device *pmsg_device;
+	int i;
 
 	pmsg_major = register_chrdev(0, PMSG_NAME, &pmsg_fops);
 	if (pmsg_major < 0) {
@@ -76,15 +77,23 @@ void pstore_register_pmsg(void)
 	}
 	pmsg_class->devnode = pmsg_devnode;
 
-	pmsg_device = device_create(pmsg_class, NULL, MKDEV(pmsg_major, 0),
-					NULL, "%s%d", PMSG_NAME, 0);
-	if (IS_ERR(pmsg_device)) {
-		pr_err("failed to create device\n");
-		goto err_device;
+	/* allocate additional /dev/pmsg[ID] */
+	for (i = 0; i < psinfo->num_pmsg; i++) {
+		struct device *pmsg_device;
+
+		pmsg_device = device_create(pmsg_class, NULL,
+					    MKDEV(pmsg_major, i), NULL, "%s%d",
+					    PMSG_NAME, i);
+		if (IS_ERR(pmsg_device)) {
+			pr_err("failed to create device\n");
+			goto err_device;
+		}
 	}
 	return;
 
 err_device:
+	for (i--; i >= 0; i--)
+		device_destroy(pmsg_class, MKDEV(pmsg_major, i));
 	class_destroy(pmsg_class);
 err_class:
 	unregister_chrdev(pmsg_major, PMSG_NAME);
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index 0da29cae009b..7a5db4833b8a 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -55,6 +55,7 @@ struct pstore_info {
 	size_t		bufsize;
 	struct mutex	read_mutex;	/* serialize open/read/close */
 	int		flags;
+	unsigned int	num_pmsg;
 	int		(*open)(struct pstore_info *psi);
 	int		(*close)(struct pstore_info *psi);
 	ssize_t		(*read)(u64 *id, enum pstore_type_id *type,
-- 
2.11.0

[PATCH v4 2nd 3/4] ramoops: support multiple pmsg instances

[Nobuhiro Iwamatsu]

This enables ramoops to deal with multiple pmsg instances.
A User can configure the size of each buffers by its module parameter
as follows.

  pmsg_size=0x1000,0x2000,...

Then, the ramoops allocates multiple buffers and tells the number
of buffers to pstore to create multiple /dev/pmsg[ID].

Signed-off-by: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.kw@hitachi.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Anton Vorontsov <anton@enomsg.org>
Cc: Colin Cross <ccross@android.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Mark Salyzyn <salyzyn@android.com>
Cc: Seiji Aguchi <seiji.aguchi.tr@hitachi.com>
Cc: Tony Luck <tony.luck@intel.com>

V4:
  - Rebase.

V3:
  - Rebase.
  - Update documentiation of DT binding.
  - Update parsing function of ramoops_pmsg_size, add NULL-terminated.
  - Update module parameters for pmsg_size list.
---
 Documentation/admin-guide/ramoops.rst              |  22 ++
 .../bindings/reserved-memory/ramoops.txt           |   6 +-
 fs/pstore/ram.c                                    | 223 ++++++++++++++++++---
 include/linux/pstore_ram.h                         |   8 +-
 4 files changed, 231 insertions(+), 28 deletions(-)

diff --git a/Documentation/admin-guide/ramoops.rst b/Documentation/admin-guide/ramoops.rst
index 4efd7ce77565..611ee1d7d9ba 100644
--- a/Documentation/admin-guide/ramoops.rst
+++ b/Documentation/admin-guide/ramoops.rst
@@ -154,3 +154,25 @@ file. Here is an example of usage::
  0 ffffffff811d9c54  ffffffff8101a7a0  __const_udelay <- native_machine_emergency_restart+0x110/0x1e0
  0 ffffffff811d9c34  ffffffff811d9c80  __delay <- __const_udelay+0x30/0x40
  0 ffffffff811d9d14  ffffffff811d9c3f  delay_tsc <- __delay+0xf/0x20
+
+6. Pmsg support
+
+Ramoops supports pmsg - logging userspace mesages in persistent store. By
+default, one pmsg area becomes available in ramoops. You can write data into
+/dev/pmsg0, e.g.:
+
+ # echo foo > /dev/pmsg0
+
+After reboot, the stored data can be read from pmsg-ramoops-0 on the pstore
+filesystem.
+
+You can specify size of the pmsg area by additional kernel command line, e.g.:
+
+ "ramoops.pmsg_size=0x1000"
+
+You can also use multiple pmsg areas, e.g.:
+
+ "ramoops.pmsg_size=0x1000,0x2000,..."
+
+Then, pmsg0, pmsg1, ... will appear on /dev. This is useful to control
+individual content aging or priority.
diff --git a/Documentation/devicetree/bindings/reserved-memory/ramoops.txt b/Documentation/devicetree/bindings/reserved-memory/ramoops.txt
index 0eba562fe5c6..e270221b98df 100644
--- a/Documentation/devicetree/bindings/reserved-memory/ramoops.txt
+++ b/Documentation/devicetree/bindings/reserved-memory/ramoops.txt
@@ -39,7 +39,11 @@ Optional properties:
 - ftrace-size: size in bytes of log buffer reserved for function tracing and
   profiling (defaults to 0: disabled)
 
-- pmsg-size: size in bytes of log buffer reserved for userspace messages
+- pmsg-size: array of value that are used to size in bytes of log buffer
+  reserved for userspace messages.
+
+	pmsg-size = <0x10000 0x10000>;
+
   (defaults to 0: disabled)
 
 - unbuffered: if present, use unbuffered mappings to map the reserved region
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 72072e8123e3..50bc78aff6b8 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -53,8 +53,8 @@ static ulong ramoops_ftrace_size = MIN_MEM_SIZE;
 module_param_named(ftrace_size, ramoops_ftrace_size, ulong, 0400);
 MODULE_PARM_DESC(ftrace_size, "size of ftrace log");
 
-static ulong ramoops_pmsg_size = MIN_MEM_SIZE;
-module_param_named(pmsg_size, ramoops_pmsg_size, ulong, 0400);
+static char *ramoops_pmsg_size_str;
+module_param_named(pmsg_size, ramoops_pmsg_size_str, charp, 0400);
 MODULE_PARM_DESC(pmsg_size, "size of user space message log");
 
 static unsigned long long mem_address;
@@ -88,14 +88,14 @@ struct ramoops_context {
 	struct persistent_ram_zone **dprzs;	/* Oops dump zones */
 	struct persistent_ram_zone *cprz;	/* Console zone */
 	struct persistent_ram_zone **fprzs;	/* Ftrace zones */
-	struct persistent_ram_zone *mprz;	/* PMSG zone */
+	struct persistent_ram_zone **mprzs;	/* PMSG zone */
 	phys_addr_t phys_addr;
 	unsigned long size;
 	unsigned int memtype;
 	size_t record_size;
 	size_t console_size;
 	size_t ftrace_size;
-	size_t pmsg_size;
+	size_t *pmsg_size;
 	int dump_oops;
 	u32 flags;
 	struct persistent_ram_ecc_info ecc_info;
@@ -275,9 +275,11 @@ static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,
 		prz = ramoops_get_next_prz(&cxt->cprz, &cxt->console_read_cnt,
 					   1, id, type, PSTORE_TYPE_CONSOLE, 0);
 
-	if (!prz_ok(prz))
-		prz = ramoops_get_next_prz(&cxt->mprz, &cxt->pmsg_read_cnt,
-					   1, id, type, PSTORE_TYPE_PMSG, 0);
+	while (cxt->pmsg_read_cnt < cxt->pstore.num_pmsg && !prz_ok(prz))
+		/* get pmsg prz */
+		prz = ramoops_get_next_prz(cxt->mprzs, &cxt->pmsg_read_cnt,
+					   cxt->pstore.num_pmsg, id, type,
+					   PSTORE_TYPE_PMSG, 0);
 
 	/* ftrace is last since it may want to dynamically allocate memory. */
 	if (!prz_ok(prz)) {
@@ -455,9 +457,9 @@ static int notrace ramoops_pstore_write_buf_user(enum pstore_type_id type,
 	if (type == PSTORE_TYPE_PMSG) {
 		struct ramoops_context *cxt = psi->data;
 
-		if (!cxt->mprz)
+		if (!cxt->mprzs)
 			return -ENOMEM;
-		return persistent_ram_write_user(cxt->mprz, buf, size);
+		return persistent_ram_write_user(cxt->mprzs[part], buf, size);
 	}
 
 	return -EINVAL;
@@ -484,7 +486,9 @@ static int ramoops_pstore_erase(enum pstore_type_id type, u64 id, int count,
 		prz = cxt->fprzs[id];
 		break;
 	case PSTORE_TYPE_PMSG:
-		prz = cxt->mprz;
+		if (id >= cxt->pstore.num_pmsg)
+			return -EINVAL;
+		prz = cxt->mprzs[id];
 		break;
 	default:
 		return -EINVAL;
@@ -634,6 +638,40 @@ static int ramoops_init_przs(const char *name,
 	return err;
 }
 
+/* int function for pmsg przs */
+static int ramoops_init_mprzs(const char *name,
+			      struct device *dev, struct ramoops_context *cxt,
+			      phys_addr_t *paddr, unsigned long total)
+{
+	int err = -ENOMEM;
+	int i;
+
+	if (!total)
+		return 0;
+
+	if (*paddr + total - cxt->phys_addr > cxt->size) {
+		dev_err(dev, "no room for pmsg\n");
+		return -ENOMEM;
+	}
+
+	cxt->mprzs = kcalloc(cxt->pstore.num_pmsg, sizeof(*cxt->mprzs),
+			     GFP_KERNEL);
+	if (!cxt->mprzs)
+		return -ENOMEM;
+
+	for (i = 0; i < cxt->pstore.num_pmsg; i++) {
+		err = __ramoops_init_prz(name, dev, cxt, &cxt->mprzs[i], paddr,
+					 cxt->pmsg_size[i], 0, 0, true);
+		if (err)
+			goto fail_mprz;
+	}
+
+	return 0;
+fail_mprz:
+	ramoops_free_przs(cxt->mprzs);
+	return err;
+}
+
 static int ramoops_init_prz(const char *name,
 			    struct device *dev, struct ramoops_context *cxt,
 			    struct persistent_ram_zone **prz,
@@ -670,7 +708,8 @@ static int ramoops_parse_dt(struct platform_device *pdev,
 	struct device_node *of_node = pdev->dev.of_node;
 	struct resource *res;
 	u32 value;
-	int ret;
+	int ret, i;
+	unsigned int num_pmsg;
 
 	dev_dbg(&pdev->dev, "using Device Tree\n");
 
@@ -696,12 +735,71 @@ static int ramoops_parse_dt(struct platform_device *pdev,
 	parse_size("record-size", pdata->record_size);
 	parse_size("console-size", pdata->console_size);
 	parse_size("ftrace-size", pdata->ftrace_size);
-	parse_size("pmsg-size", pdata->pmsg_size);
 	parse_size("ecc-size", pdata->ecc_info.ecc_size);
 	parse_size("flags", pdata->flags);
 
 #undef parse_size
 
+	/* Parse pmesg size */
+	if (!of_find_property(of_node, "pmsg-size", &num_pmsg))
+		return 0; /* no data */
+
+	num_pmsg = num_pmsg / sizeof(u32);
+
+	pdata->pmsg_size =
+		devm_kzalloc(&pdev->dev, sizeof(size_t) * (num_pmsg + 1),
+			     GFP_KERNEL);
+	if (!pdata->pmsg_size)
+		return -ENOMEM;
+
+	for (i = 0; i < num_pmsg; i++) {
+		ret = of_property_read_u32_index(of_node, "pmsg-size",
+						 i, &value);
+		if (ret) {
+			dev_warn(&pdev->dev,
+				"%s: failed to read pmsg-size[%d]: %d\n",
+				 __func__, i, ret);
+			return -EINVAL;
+		}
+
+		/* CHECK INT_MAX */
+		if (value > INT_MAX) {
+			dev_err(&pdev->dev, "value %x > INT_MAX\n", value);
+			return -EOVERFLOW;
+		}
+		pdata->pmsg_size[i] = value;
+	}
+
+	return 0;
+}
+
+#define ADDR_STRING_SIZE	10 /* "0x00000000" */
+static int update_pmsg_size_mod_param(size_t *pmsg_size, int num)
+{
+	int i, len;
+
+	/* string size + commas count + NULL-terminated */
+	len = ADDR_STRING_SIZE * num + (num - 1) + 1;
+
+	/* using commandline or already allocation buffer.*/
+	if (ramoops_pmsg_size_str)
+		goto out;
+
+	ramoops_pmsg_size_str = kzalloc(len, GFP_KERNEL);
+	if (!ramoops_pmsg_size_str)
+		return -ENOMEM;
+
+	for (i = 0 ; i < num; i++) {
+		char str[ADDR_STRING_SIZE + 1];
+
+		memset(str, 0, sizeof(str));
+		if (i == num - 1)
+			snprintf(str, sizeof(str), "0x%x", pmsg_size[i]);
+		else
+			snprintf(str, sizeof(str), "0x%x,", pmsg_size[i]);
+		strcat(ramoops_pmsg_size_str, str);
+	}
+out:
 	return 0;
 }
 
@@ -713,6 +811,8 @@ static int ramoops_probe(struct platform_device *pdev)
 	size_t dump_mem_sz;
 	phys_addr_t paddr;
 	int err = -EINVAL;
+	unsigned long pmsg_size_total = 0;
+	unsigned int num_pmsg = 0;
 
 	if (dev_of_node(dev) && !pdata) {
 		pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
@@ -755,8 +855,20 @@ static int ramoops_probe(struct platform_device *pdev)
 		pdata->console_size = rounddown_pow_of_two(pdata->console_size);
 	if (pdata->ftrace_size && !is_power_of_2(pdata->ftrace_size))
 		pdata->ftrace_size = rounddown_pow_of_two(pdata->ftrace_size);
-	if (pdata->pmsg_size && !is_power_of_2(pdata->pmsg_size))
-		pdata->pmsg_size = rounddown_pow_of_two(pdata->pmsg_size);
+
+	if (pdata->pmsg_size) {
+		for (;; num_pmsg++) {
+			unsigned long size = pdata->pmsg_size[num_pmsg];
+
+			if (!size)
+				break;
+
+			if (!is_power_of_2(size))
+				pdata->pmsg_size[num_pmsg]
+					= rounddown_pow_of_two(size);
+			pmsg_size_total += size;
+		}
+	}
 
 	cxt->size = pdata->mem_size;
 	cxt->phys_addr = pdata->mem_address;
@@ -764,20 +876,29 @@ static int ramoops_probe(struct platform_device *pdev)
 	cxt->record_size = pdata->record_size;
 	cxt->console_size = pdata->console_size;
 	cxt->ftrace_size = pdata->ftrace_size;
-	cxt->pmsg_size = pdata->pmsg_size;
 	cxt->dump_oops = pdata->dump_oops;
 	cxt->flags = pdata->flags;
 	cxt->ecc_info = pdata->ecc_info;
+	cxt->pstore.num_pmsg = num_pmsg;
 
-	paddr = cxt->phys_addr;
+	if (num_pmsg) {
+		cxt->pmsg_size = kcalloc(num_pmsg, sizeof(size_t), GFP_KERNEL);
+		if (!cxt->pmsg_size) {
+			err = -ENOMEM;
+			goto fail_out;
+		}
+		memcpy(cxt->pmsg_size, pdata->pmsg_size,
+		       sizeof(size_t) * num_pmsg);
+	}
 
+	paddr = cxt->phys_addr;
 	dump_mem_sz = cxt->size - cxt->console_size - cxt->ftrace_size
-			- cxt->pmsg_size;
+			- pmsg_size_total;
 	err = ramoops_init_przs("dump", dev, cxt, &cxt->dprzs, &paddr,
 				dump_mem_sz, cxt->record_size,
 				&cxt->max_dump_cnt, 0, 0);
 	if (err)
-		goto fail_out;
+		goto fail_init_dprzs;
 
 	err = ramoops_init_prz("console", dev, cxt, &cxt->cprz, &paddr,
 			       cxt->console_size, 0);
@@ -795,10 +916,9 @@ static int ramoops_probe(struct platform_device *pdev)
 	if (err)
 		goto fail_init_fprz;
 
-	err = ramoops_init_prz("pmsg", dev, cxt, &cxt->mprz, &paddr,
-				cxt->pmsg_size, 0);
+	err = ramoops_init_mprzs("pmsg", dev, cxt, &paddr, pmsg_size_total);
 	if (err)
-		goto fail_init_mprz;
+		goto fail_init_mprzs;
 
 	cxt->pstore.data = cxt;
 	/*
@@ -841,8 +961,8 @@ static int ramoops_probe(struct platform_device *pdev)
 	record_size = pdata->record_size;
 	dump_oops = pdata->dump_oops;
 	ramoops_console_size = pdata->console_size;
-	ramoops_pmsg_size = pdata->pmsg_size;
 	ramoops_ftrace_size = pdata->ftrace_size;
+	update_pmsg_size_mod_param(cxt->pmsg_size, cxt->pstore.num_pmsg);
 
 	pr_info("attached 0x%lx@0x%llx, ecc: %d/%d\n",
 		cxt->size, (unsigned long long)cxt->phys_addr,
@@ -854,8 +974,8 @@ static int ramoops_probe(struct platform_device *pdev)
 	kfree(cxt->pstore.buf);
 fail_clear:
 	cxt->pstore.bufsize = 0;
-	persistent_ram_free(cxt->mprz);
-fail_init_mprz:
+	ramoops_free_przs(cxt->mprzs);
+fail_init_mprzs:
 	cxt->max_ftrace_cnt = 0;
 	ramoops_free_przs(cxt->fprzs);
 fail_init_fprz:
@@ -863,6 +983,8 @@ static int ramoops_probe(struct platform_device *pdev)
 fail_init_cprz:
 	cxt->max_dump_cnt = 0;
 	ramoops_free_przs(cxt->dprzs);
+fail_init_dprzs:
+	kfree(cxt->pmsg_size);
 fail_out:
 	return err;
 }
@@ -875,8 +997,10 @@ static int ramoops_remove(struct platform_device *pdev)
 
 	kfree(cxt->pstore.buf);
 	cxt->pstore.bufsize = 0;
+	kfree(cxt->pmsg_size);
 
-	persistent_ram_free(cxt->mprz);
+	ramoops_free_przs(cxt->mprzs);
+	cxt->pstore.num_pmsg = 0;
 	persistent_ram_free(cxt->cprz);
 
 	/* Free ftrace PRZs */
@@ -903,6 +1027,43 @@ static struct platform_driver ramoops_driver = {
 	},
 };
 
+static unsigned long *parse_size_str(char *size_str)
+{
+	int i, ret;
+	unsigned long *size_array, count = 1;
+
+	if (size_str) {
+		char *s = size_str;
+
+		/* Necessary array size is the number of commas + 1 */
+		for (; (s = strchr(s, ',')); s++)
+			count++;
+	}
+
+	/* Add NULL-terminated */
+	count++;
+
+	size_array = kcalloc(count, sizeof(unsigned long), GFP_KERNEL);
+	if (!size_array)
+		goto out;
+
+	if (size_str) {
+		for (i = 0; i < count; i++) {
+			ret = get_option(&size_str, (int *)&size_array[i]);
+			if (ret == 1)
+				break;
+			else if (ret != 2) {
+				size_array[i] = 0;
+				break;
+			}
+		}
+	} else
+		size_array[0] = MIN_MEM_SIZE;
+
+out:
+	return size_array;
+}
+
 static void ramoops_register_dummy(void)
 {
 	if (!mem_size)
@@ -922,7 +1083,10 @@ static void ramoops_register_dummy(void)
 	dummy_data->record_size = record_size;
 	dummy_data->console_size = ramoops_console_size;
 	dummy_data->ftrace_size = ramoops_ftrace_size;
-	dummy_data->pmsg_size = ramoops_pmsg_size;
+	dummy_data->pmsg_size = parse_size_str(ramoops_pmsg_size_str);
+
+	if (!dummy_data->pmsg_size)
+		goto fail_pmsg_size;
 	dummy_data->dump_oops = dump_oops;
 	dummy_data->flags = RAMOOPS_FLAG_FTRACE_PER_CPU;
 
@@ -937,7 +1101,13 @@ static void ramoops_register_dummy(void)
 	if (IS_ERR(dummy)) {
 		pr_info("could not create platform device: %ld\n",
 			PTR_ERR(dummy));
+		goto fail_pdev;
 	}
+	return;
+fail_pdev:
+	kfree(dummy_data->pmsg_size);
+fail_pmsg_size:
+	kfree(dummy_data);
 }
 
 static int __init ramoops_init(void)
@@ -951,6 +1121,7 @@ static void __exit ramoops_exit(void)
 {
 	platform_driver_unregister(&ramoops_driver);
 	platform_device_unregister(dummy);
+	kfree(dummy_data->pmsg_size);
 	kfree(dummy_data);
 }
 module_exit(ramoops_exit);
diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h
index 9395f06e8372..030fa45af2bf 100644
--- a/include/linux/pstore_ram.h
+++ b/include/linux/pstore_ram.h
@@ -84,6 +84,12 @@ ssize_t persistent_ram_ecc_string(struct persistent_ram_zone *prz,
  * Ramoops platform data
  * @mem_size	memory size for ramoops
  * @mem_address	physical memory address to contain ramoops
+ * @pmsg_size	array containing size of each pmsg area. 0 value in the array
+ *		indicates the end of the content. For example, if the following
+ *		array is given,
+ *		    .pmsg_size = { 0x1000, 0x2000, 0x0, 0x3000, };
+ *		then, pmsg areas are allocated for the first two size values
+ *		and '0x3000' is just ignored.
  */
 
 #define RAMOOPS_FLAG_FTRACE_PER_CPU	BIT(0)
@@ -95,7 +101,7 @@ struct ramoops_platform_data {
 	unsigned long	record_size;
 	unsigned long	console_size;
 	unsigned long	ftrace_size;
-	unsigned long	pmsg_size;
+	unsigned long	*pmsg_size;
 	int		dump_oops;
 	u32		flags;
 	struct persistent_ram_ecc_info ecc_info;
-- 
2.11.0

[PATCH v4 2nd 4/4] selftests/pstore: add testcases for multiple pmsg instances

[Nobuhiro Iwamatsu]

From: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>

To test multiple pmsg, we should check that /dev/pmsg[N] (N > 0) is
available. After reboot, we should check that pmsg-[backend]-[N] which
keeps content is detected even if pmsg-[backend]-[N-M] (0 < M <= N)
doesn't exist due to lack of contents.

So this adds the following testcases.
 - pstore_tests
   - Write unique string to the last /dev/pmsgN
 - pstore_post_reboot_tests
   - Check the last pmsg area is detected

Signed-off-by: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.kw@hitachi.com>
Cc: Anton Vorontsov <anton@enomsg.org>
Cc: Colin Cross <ccross@android.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Mark Salyzyn <salyzyn@android.com>
Cc: Seiji Aguchi <seiji.aguchi.tr@hitachi.com>
Cc: Shuah Khan <shuahkh@osg.samsung.com>
Cc: Tony Luck <tony.luck@intel.com>

V4:
  Rebase.

V3:
  Rebase.

V2:
  Remove Blank lines.
---
 tools/testing/selftests/pstore/common_tests        | 21 +++++++++++++++--
 .../selftests/pstore/pstore_post_reboot_tests      | 27 ++++++++++++----------
 tools/testing/selftests/pstore/pstore_tests        | 14 ++++++++---
 3 files changed, 45 insertions(+), 17 deletions(-)

diff --git a/tools/testing/selftests/pstore/common_tests b/tools/testing/selftests/pstore/common_tests
index 3ea64d7cf1cd..566a25d10350 100755
--- a/tools/testing/selftests/pstore/common_tests
+++ b/tools/testing/selftests/pstore/common_tests
@@ -27,9 +27,9 @@ show_result() { # result_value
 }
 
 check_files_exist() { # type of pstorefs file
-    if [ -e ${1}-${backend}-0 ]; then
+    if [ -e ${1}0 ]; then
 	prlog "ok"
-	for f in `ls ${1}-${backend}-*`; do
+	for f in `ls ${1}*`; do
             prlog -e "\t${f}"
 	done
     else
@@ -53,6 +53,23 @@ operate_files() { # tested value, files, operation
     fi
 }
 
+check_pmsg_content() { # pmsg filename
+    prev_uuid=`cat $TOP_DIR/prev_uuid`
+    if [ $? -eq 0 ]; then
+	nr_matched=`grep -c "$TEST_STRING_PATTERN" $1`
+	if [ "$nr_matched" = "1" ]; then
+	    grep -q "$TEST_STRING_PATTERN"$prev_uuid $1
+	    show_result $?
+	else
+	    prlog "FAIL"
+	    rc=1
+	fi
+    else
+	prlog "FAIL"
+	rc=1
+    fi
+}
+
 # Parameters
 TEST_STRING_PATTERN="Testing pstore: uuid="
 UUID=`cat /proc/sys/kernel/random/uuid`
diff --git a/tools/testing/selftests/pstore/pstore_post_reboot_tests b/tools/testing/selftests/pstore/pstore_post_reboot_tests
index 6ccb154cb4aa..272498fa9281 100755
--- a/tools/testing/selftests/pstore/pstore_post_reboot_tests
+++ b/tools/testing/selftests/pstore/pstore_post_reboot_tests
@@ -35,13 +35,13 @@ fi
 cd ${mount_point}
 
 prlog -n "Checking dmesg files exist in pstore filesystem ... "
-check_files_exist dmesg
+check_files_exist dmesg-${backend}-
 
 prlog -n "Checking console files exist in pstore filesystem ... "
-check_files_exist console
+check_files_exist console-${backend}-
 
 prlog -n "Checking pmsg files exist in pstore filesystem ... "
-check_files_exist pmsg
+check_files_exist pmsg-${backend}-
 
 prlog -n "Checking dmesg files contain oops end marker"
 grep_end_trace() {
@@ -54,16 +54,19 @@ prlog -n "Checking console file contains oops end marker ... "
 grep -q "\---\[ end trace" console-${backend}-0
 show_result $?
 
-prlog -n "Checking pmsg file properly keeps the content written before crash ... "
-prev_uuid=`cat $TOP_DIR/prev_uuid`
-if [ $? -eq 0 ]; then
-    nr_matched=`grep -c "$TEST_STRING_PATTERN" pmsg-${backend}-0`
-    if [ $nr_matched -eq 1 ]; then
-	grep -q "$TEST_STRING_PATTERN"$prev_uuid pmsg-${backend}-0
-	show_result $?
+prlog -n "Checking pmsg-"${backend}"-0 properly keeps the content written before crash ... "
+check_pmsg_content pmsg-${backend}-0
+
+prlog -n "Checking the last pmsg area is detected "
+last_num_pmsg=`ls -v pmsg-* | tail -n1 | sed -e "s/^pmsg-${backend}-\([0-9]*\).*$/\1/"`
+last_num_devpmsg=`ls -v /dev/pmsg* | tail -n1 | sed -e "s/^\/dev\/pmsg\([0-9]*\).*$/\1/"`
+#checks the last number of pmsg-*-* and /dev/pmsg* correspond.
+if [ "$last_num_pmsg" -eq "$last_num_devpmsg" ]; then
+    if [ "$last_num_pmsg" = "0" ]; then
+	prlog "... No multiple pmsg-*-* exists. We skip this testcase."
     else
-	prlog "FAIL"
-	rc=1
+	prlog -n "(pmsg-${backend}-${last_num_pmsg}) ... "
+	check_pmsg_content pmsg-${backend}-${last_num_pmsg}
     fi
 else
     prlog "FAIL"
diff --git a/tools/testing/selftests/pstore/pstore_tests b/tools/testing/selftests/pstore/pstore_tests
index f25d2a349a60..b74b3afb9722 100755
--- a/tools/testing/selftests/pstore/pstore_tests
+++ b/tools/testing/selftests/pstore/pstore_tests
@@ -13,9 +13,8 @@ prlog -n "Checking pstore console is registered ... "
 dmesg | grep -q "console \[pstore"
 show_result $?
 
-prlog -n "Checking /dev/pmsg0 exists ... "
-test -e /dev/pmsg0
-show_result $?
+prlog -n "Checking /dev/pmsg files exist ... "
+check_files_exist /dev/pmsg
 
 prlog -n "Writing unique string to /dev/pmsg0 ... "
 if [ -e "/dev/pmsg0" ]; then
@@ -27,4 +26,13 @@ else
     rc=1
 fi
 
+last_devpmsg=`ls -v /dev/pmsg* | tail -n1`
+prlog -n "Writing unique string to the last /dev/pmsgN "
+if [ "$last_devpmsg" = "/dev/pmsg0" ]; then
+    prlog "... No multiple /dev/pmsg* exists. We skip this testcase."
+else
+    prlog -n "(${last_devpmsg}) ... "
+    echo "${TEST_STRING_PATTERN}""$UUID" > ${last_devpmsg}
+    show_result $?
+fi
 exit $rc
-- 
2.11.0

Re: [PATCH v4 2nd 2/4] pstore: support multiple pmsg instances

[Kees Cook]

On Mon, Jan 30, 2017 at 5:58 PM, Nobuhiro Iwamatsu
<nobuhiro.iwamatsu.kw@hitachi.com> wrote:
> From: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
>
> This enables pmsg to deal with multiple instances. One possible
> use is content priority control on limited persistent store space. By
> using different buffers, we can prevent important messages from being
> overwritten by less important messages.
>
> When pstore backend module specifies the number of instances (num_pmsg)
> in pstore_info, multiple /dev/pmsg[ID] appear. (ID is an integer
> starting at 0. It corresponds to minor number of the each char device.)
> Writes to each /dev/pmsg[ID] are isolated each other. After reboot, the
> contents are available in /sys/fs/pstore/pmsg-[backend]-[ID].
>
> Signed-off-by: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
> Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.kw@hitachi.com>
> Cc: Anton Vorontsov <anton@enomsg.org>
> Cc: Colin Cross <ccross@android.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Mark Salyzyn <salyzyn@android.com>
> Cc: Seiji Aguchi <seiji.aguchi.tr@hitachi.com>
> Cc: Tony Luck <tony.luck@intel.com>
>
> V4:
>     Rebase.
> V3:
>     rebase.
>     merged device_create().
> ---
>  fs/pstore/pmsg.c       | 23 ++++++++++++++++-------
>  include/linux/pstore.h |  1 +
>  2 files changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/fs/pstore/pmsg.c b/fs/pstore/pmsg.c
> index 78f6176c020f..5da5cba4b387 100644
> --- a/fs/pstore/pmsg.c
> +++ b/fs/pstore/pmsg.c
> @@ -34,7 +34,8 @@ static ssize_t write_pmsg(struct file *file, const char __user *buf,
>                 return -EFAULT;
>
>         mutex_lock(&pmsg_lock);
> -       ret = psinfo->write_buf_user(PSTORE_TYPE_PMSG, 0, &id, 0, buf, 0, count,
> +       ret = psinfo->write_buf_user(PSTORE_TYPE_PMSG, 0, &id,
> +                                    iminor(file->f_inode), buf, 0, count,
>                                      psinfo);
>         mutex_unlock(&pmsg_lock);
>         return ret ? ret : count;
> @@ -61,7 +62,7 @@ static char *pmsg_devnode(struct device *dev, umode_t *mode)
>
>  void pstore_register_pmsg(void)
>  {
> -       struct device *pmsg_device;
> +       int i;
>
>         pmsg_major = register_chrdev(0, PMSG_NAME, &pmsg_fops);
>         if (pmsg_major < 0) {
> @@ -76,15 +77,23 @@ void pstore_register_pmsg(void)
>         }
>         pmsg_class->devnode = pmsg_devnode;
>
> -       pmsg_device = device_create(pmsg_class, NULL, MKDEV(pmsg_major, 0),
> -                                       NULL, "%s%d", PMSG_NAME, 0);
> -       if (IS_ERR(pmsg_device)) {
> -               pr_err("failed to create device\n");
> -               goto err_device;
> +       /* allocate additional /dev/pmsg[ID] */

This isn't "additional" any more, but rather "Allocate each pmsg
device" or something.

> +       for (i = 0; i < psinfo->num_pmsg; i++) {
> +               struct device *pmsg_device;
> +
> +               pmsg_device = device_create(pmsg_class, NULL,
> +                                           MKDEV(pmsg_major, i), NULL, "%s%d",
> +                                           PMSG_NAME, i);
> +               if (IS_ERR(pmsg_device)) {
> +                       pr_err("failed to create device\n");
> +                       goto err_device;
> +               }
>         }
>         return;
>
>  err_device:
> +       for (i--; i >= 0; i--)
> +               device_destroy(pmsg_class, MKDEV(pmsg_major, i));
>         class_destroy(pmsg_class);
>  err_class:
>         unregister_chrdev(pmsg_major, PMSG_NAME);
> diff --git a/include/linux/pstore.h b/include/linux/pstore.h
> index 0da29cae009b..7a5db4833b8a 100644
> --- a/include/linux/pstore.h
> +++ b/include/linux/pstore.h
> @@ -55,6 +55,7 @@ struct pstore_info {
>         size_t          bufsize;
>         struct mutex    read_mutex;     /* serialize open/read/close */
>         int             flags;
> +       unsigned int    num_pmsg;

Something in this patch needs to set num_pmsg to 1 unconditionally, so
that bisectability is retained. I.e. after this patch, I should still
get a /dev/pmsg0 device. Right now, it will be skipped since num_pmsg
will always == 0.

>         int             (*open)(struct pstore_info *psi);
>         int             (*close)(struct pstore_info *psi);
>         ssize_t         (*read)(u64 *id, enum pstore_type_id *type,
> --
> 2.11.0
>

Thanks for the rebasing!

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v4 2nd 1/4] ramoops: Add __ramoops_init_prz() as generic function

[Mark Salyzyn]

On 01/30/2017 05:58 PM, Nobuhiro Iwamatsu wrote:
> +
> +	if (zap && *paddr + sz - cxt->phys_addr > cxt->size) {
> +		dev_err(dev, "no room for %s mem region (0x%zx@0x%llx) in (0x%lx@0x%llx)\n",
> +			name, sz, (unsigned long long)*paddr,
> +			cxt->size, (unsigned long long)cxt->phys_addr);
> +		return -ENOMEM;
> +	}
> +
Why not allow this limit check for ramoops_init_prsz call?

-- Mark

Re: [PATCH v4 2nd 2/4] pstore: support multiple pmsg instances

[Mark Salyzyn]

On 01/30/2017 05:58 PM, Nobuhiro Iwamatsu wrote:
> diff --git a/include/linux/pstore.h b/include/linux/pstore.h
> index 0da29cae009b..7a5db4833b8a 100644
> --- a/include/linux/pstore.h
> +++ b/include/linux/pstore.h
> @@ -55,6 +55,7 @@ struct pstore_info {
>   	size_t		bufsize;
>   	struct mutex	read_mutex;	/* serialize open/read/close */
>   	int		flags;
> +	unsigned int	num_pmsg;
>   	int		(*open)(struct pstore_info *psi);
>   	int		(*close)(struct pstore_info *psi);
>   	ssize_t		(*read)(u64 *id, enum pstore_type_id *type,

Before patch there was an implication that num_pmsg was 1, after patch 
calloc leaves this at a value of 0. Please reconcile the functional 
difference that this causes.

-- Mark

RE: [PATCH v4 2nd 2/4] pstore: support multiple pmsg instances

[岩松信洋 / IWAMATSU，NOBUHIRO]

Hi,

Thanks for your review.

> -----Original Message-----
> From: keescook@google.com [mailto:keescook@google.com] On Behalf Of Kees
> Cook
> Sent: Thursday, February 02, 2017 5:29 AM
> To: 岩松信洋 / IWAMATSU，NOBUHIRO
> Cc: Anton Vorontsov; Colin Cross; Tony Luck; LKML; Hiraku Toyooka; Mark
> Salyzyn; 阿口誠司 / AGUCHI，SEIJI
> Subject: Re: [PATCH v4 2nd 2/4] pstore: support multiple pmsg instances
> 
> On Mon, Jan 30, 2017 at 5:58 PM, Nobuhiro Iwamatsu
> <nobuhiro.iwamatsu.kw@hitachi.com> wrote:
> > From: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
> >
> > This enables pmsg to deal with multiple instances. One possible use is
> > content priority control on limited persistent store space. By using
> > different buffers, we can prevent important messages from being
> > overwritten by less important messages.
> >
> > When pstore backend module specifies the number of instances
> > (num_pmsg) in pstore_info, multiple /dev/pmsg[ID] appear. (ID is an
> > integer starting at 0. It corresponds to minor number of the each char
> > device.) Writes to each /dev/pmsg[ID] are isolated each other. After
> > reboot, the contents are available in
> /sys/fs/pstore/pmsg-[backend]-[ID].
> >
> > Signed-off-by: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
> > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.kw@hitachi.com>
> > Cc: Anton Vorontsov <anton@enomsg.org>
> > Cc: Colin Cross <ccross@android.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Mark Salyzyn <salyzyn@android.com>
> > Cc: Seiji Aguchi <seiji.aguchi.tr@hitachi.com>
> > Cc: Tony Luck <tony.luck@intel.com>
> >
> > V4:
> >     Rebase.
> > V3:
> >     rebase.
> >     merged device_create().
> > ---
> >  fs/pstore/pmsg.c       | 23 ++++++++++++++++-------
> >  include/linux/pstore.h |  1 +
> >  2 files changed, 17 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/pstore/pmsg.c b/fs/pstore/pmsg.c index
> > 78f6176c020f..5da5cba4b387 100644
> > --- a/fs/pstore/pmsg.c
> > +++ b/fs/pstore/pmsg.c
> > @@ -34,7 +34,8 @@ static ssize_t write_pmsg(struct file *file, const char
> __user *buf,
> >                 return -EFAULT;
> >
> >         mutex_lock(&pmsg_lock);
> > -       ret = psinfo->write_buf_user(PSTORE_TYPE_PMSG, 0, &id, 0, buf,
> 0, count,
> > +       ret = psinfo->write_buf_user(PSTORE_TYPE_PMSG, 0, &id,
> > +                                    iminor(file->f_inode), buf, 0,
> > + count,
> >                                      psinfo);
> >         mutex_unlock(&pmsg_lock);
> >         return ret ? ret : count;
> > @@ -61,7 +62,7 @@ static char *pmsg_devnode(struct device *dev,
> > umode_t *mode)
> >
> >  void pstore_register_pmsg(void)
> >  {
> > -       struct device *pmsg_device;
> > +       int i;
> >
> >         pmsg_major = register_chrdev(0, PMSG_NAME, &pmsg_fops);
> >         if (pmsg_major < 0) {
> > @@ -76,15 +77,23 @@ void pstore_register_pmsg(void)
> >         }
> >         pmsg_class->devnode = pmsg_devnode;
> >
> > -       pmsg_device = device_create(pmsg_class, NULL, MKDEV(pmsg_major,
> 0),
> > -                                       NULL, "%s%d", PMSG_NAME, 0);
> > -       if (IS_ERR(pmsg_device)) {
> > -               pr_err("failed to create device\n");
> > -               goto err_device;
> > +       /* allocate additional /dev/pmsg[ID] */
> 
> This isn't "additional" any more, but rather "Allocate each pmsg device"
> or something.

OK, I will fix.
> 
> > +       for (i = 0; i < psinfo->num_pmsg; i++) {
> > +               struct device *pmsg_device;
> > +
> > +               pmsg_device = device_create(pmsg_class, NULL,
> > +                                           MKDEV(pmsg_major, i), NULL,
> "%s%d",
> > +                                           PMSG_NAME, i);
> > +               if (IS_ERR(pmsg_device)) {
> > +                       pr_err("failed to create device\n");
> > +                       goto err_device;
> > +               }
> >         }
> >         return;
> >
> >  err_device:
> > +       for (i--; i >= 0; i--)
> > +               device_destroy(pmsg_class, MKDEV(pmsg_major, i));
> >         class_destroy(pmsg_class);
> >  err_class:
> >         unregister_chrdev(pmsg_major, PMSG_NAME); diff --git
> > a/include/linux/pstore.h b/include/linux/pstore.h index
> > 0da29cae009b..7a5db4833b8a 100644
> > --- a/include/linux/pstore.h
> > +++ b/include/linux/pstore.h
> > @@ -55,6 +55,7 @@ struct pstore_info {
> >         size_t          bufsize;
> >         struct mutex    read_mutex;     /* serialize open/read/close
> */
> >         int             flags;
> > +       unsigned int    num_pmsg;
> 
> Something in this patch needs to set num_pmsg to 1 unconditionally, so that
> bisectability is retained. I.e. after this patch, I should still get a
> /dev/pmsg0 device. Right now, it will be skipped since num_pmsg will always
> == 0.

Indeed, I will fix too.

Best regards,
  Nobuhiro

RE: [PATCH v4 2nd 2/4] pstore: support multiple pmsg instances

[岩松信洋 / IWAMATSU，NOBUHIRO]

Hi,

Thanks for your review.

> -----Original Message-----
> From: Mark Salyzyn [mailto:salyzyn@android.com]
> Sent: Friday, February 03, 2017 7:18 AM
> To: 岩松信洋 / IWAMATSU，NOBUHIRO; Anton Vorontsov; Colin Cross; Kees Cook;
> Tony Luck
> Cc: linux-kernel@vger.kernel.org; Hiraku Toyooka; 阿口誠司 / AGUCHI，
> SEIJI
> Subject: Re: [PATCH v4 2nd 2/4] pstore: support multiple pmsg instances
> 
> On 01/30/2017 05:58 PM, Nobuhiro Iwamatsu wrote:
> > diff --git a/include/linux/pstore.h b/include/linux/pstore.h index
> > 0da29cae009b..7a5db4833b8a 100644
> > --- a/include/linux/pstore.h
> > +++ b/include/linux/pstore.h
> > @@ -55,6 +55,7 @@ struct pstore_info {
> >   	size_t		bufsize;
> >   	struct mutex	read_mutex;	/* serialize open/read/close */
> >   	int		flags;
> > +	unsigned int	num_pmsg;
> >   	int		(*open)(struct pstore_info *psi);
> >   	int		(*close)(struct pstore_info *psi);
> >   	ssize_t		(*read)(u64 *id, enum pstore_type_id *type,
> 
> Before patch there was an implication that num_pmsg was 1, after patch calloc
> leaves this at a value of 0. Please reconcile the functional difference
> that this causes.

Yes, I was pointed out here also by Kees.

I will fix this.

Best regards,
  Nobuhiro

RE: [PATCH v4 2nd 1/4] ramoops: Add __ramoops_init_prz() as generic function

[岩松信洋 / IWAMATSU，NOBUHIRO]

Hi,

Thanks for your review.

> -----Original Message-----
> From: Mark Salyzyn [mailto:salyzyn@android.com]
> Sent: Friday, February 03, 2017 7:13 AM
> To: 岩松信洋 / IWAMATSU，NOBUHIRO; Anton Vorontsov; Colin Cross; Kees Cook;
> Tony Luck
> Cc: linux-kernel@vger.kernel.org; Hiraku Toyooka; 阿口誠司 / AGUCHI，
> SEIJI; Shuah Khan
> Subject: Re: [PATCH v4 2nd 1/4] ramoops: Add __ramoops_init_prz() as generic
> function
> 
> On 01/30/2017 05:58 PM, Nobuhiro Iwamatsu wrote:
> > +
> > +	if (zap && *paddr + sz - cxt->phys_addr > cxt->size) {
> > +		dev_err(dev, "no room for %s mem region (0x%zx@0x%llx) in
> (0x%lx@0x%llx)\n",
> > +			name, sz, (unsigned long long)*paddr,
> > +			cxt->size, (unsigned long long)cxt->phys_addr);
> > +		return -ENOMEM;
> > +	}
> > +
> Why not allow this limit check for ramoops_init_prsz call?
> 

This code is controlled by flag of zap.
If zap is false, __ramoops_init_prz() is worked same as original ramoops_init_przs().

Best regards,
  Nobuhiro

Re: [PATCH v4 2nd 1/4] ramoops: Add __ramoops_init_prz() as generic function

[Mark Salyzyn]

On 02/07/2017 12:51 AM, 岩松信洋 / IWAMATSU，NOBUHIRO wrote:
> Hi,
>
> Thanks for your review.
>
>> -----Original Message-----
>> From: Mark Salyzyn [mailto:salyzyn@android.com]
>> Sent: Friday, February 03, 2017 7:13 AM
>> To: 岩松信洋 / IWAMATSU，NOBUHIRO; Anton Vorontsov; Colin Cross; Kees Cook;
>> Tony Luck
>> Cc: linux-kernel@vger.kernel.org; Hiraku Toyooka; 阿口誠司 / AGUCHI，
>> SEIJI; Shuah Khan
>> Subject: Re: [PATCH v4 2nd 1/4] ramoops: Add __ramoops_init_prz() as generic
>> function
>>
>> On 01/30/2017 05:58 PM, Nobuhiro Iwamatsu wrote:
>>> +
>>> +	if (zap && *paddr + sz - cxt->phys_addr > cxt->size) {
>>> +		dev_err(dev, "no room for %s mem region (0x%zx@0x%llx) in
>> (0x%lx@0x%llx)\n",
>>> +			name, sz, (unsigned long long)*paddr,
>>> +			cxt->size, (unsigned long long)cxt->phys_addr);
>>> +		return -ENOMEM;
>>> +	}
>>> +
>> Why not allow this limit check for ramoops_init_prsz call?
>>
> This code is controlled by flag of zap.
> If zap is false, __ramoops_init_prz() is worked same as original ramoops_init_przs().
>
> Best regards,
>    Nobuhiro

I agree, preserve functionality as is. Please consider removing the need  
for zap && in a future patch, as I _believe_ (untested, and no deep  
dive) that the limit check is relevant for the zap is false path as well  
and may have been an oversight.

-- Mark