[PATCH v6 0/3] Move recovery/coredump configuration to sysfs

[PATCH v6 0/3] Move recovery/coredump configuration to sysfs

[Rishabh Bhatnagar]

From Android R onwards Google has restricted access to debugfs in user
and user-debug builds. This restricts access to most of the features
exposed through debugfs. 'Coredump' and 'Recovery' are critical
interfaces that are required for remoteproc to work on Qualcomm Chipsets. 
Coredump configuration needs to be set to "inline" in debug/test builds
and "disabled" in production builds. Whereas recovery needs to be
"disabled" for debugging purposes and "enabled" on production builds.
This patch series removes the recovery/coredump entries from debugfs
and moves them to sysfs. Also, this disables the coredump collection
by default as this is a requirement for production devices.

Changelog:

v6 -> v5:
- Disable coredump collection by default
- Rename the "default" configuration to "enabled" to avoid confusion

v5 -> v4:
- Fix the cover-letter of tha patch series.

v4 -> v3:
- Remove the feature flag to expose recovery/coredump

v3 -> v2:
- Remove the coredump/recovery entries from debugfs
- Expose recovery/coredump from sysfs under a feature flag

v1 -> v2:
- Correct the contact name in the sysfs documentation.
- Remove the redundant write documentation for coredump/recovery sysfs
- Add a feature flag to make this interface switch configurable.

Rishabh Bhatnagar (3):
  remoteproc: Move coredump configuration to sysfs
  remoteproc: Move recovery configuration to sysfs
  remoteproc: Change default dump configuration to "disabled"

 Documentation/ABI/testing/sysfs-class-remoteproc |  46 +++++++
 drivers/remoteproc/remoteproc_coredump.c         |   6 +-
 drivers/remoteproc/remoteproc_debugfs.c          | 168 -----------------------
 drivers/remoteproc/remoteproc_sysfs.c            | 120 ++++++++++++++++
 include/linux/remoteproc.h                       |   8 +-
 5 files changed, 173 insertions(+), 175 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v6 1/3] remoteproc: Move coredump configuration to sysfs

[Rishabh Bhatnagar]

Move coredump configuration from debugfs to sysfs.This will
allow usage of this configuration feature in production
devices where access to debugfs might be limited.

Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
---
 Documentation/ABI/testing/sysfs-class-remoteproc | 24 +++++++
 drivers/remoteproc/remoteproc_debugfs.c          | 90 ------------------------
 drivers/remoteproc/remoteproc_sysfs.c            | 64 +++++++++++++++++
 3 files changed, 88 insertions(+), 90 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-remoteproc b/Documentation/ABI/testing/sysfs-class-remoteproc
index 36094fb..f6c44fa 100644
--- a/Documentation/ABI/testing/sysfs-class-remoteproc
+++ b/Documentation/ABI/testing/sysfs-class-remoteproc
@@ -58,3 +58,27 @@ Description:	Remote processor name
 		Reports the name of the remote processor. This can be used by
 		userspace in exactly identifying a remote processor and ease
 		up the usage in modifying the 'firmware' or 'state' files.
+
+What:		/sys/class/remoteproc/.../coredump
+Date:		July 2020
+Contact:	Bjorn Andersson <bjorn.andersson@linaro.org>, Ohad Ben-Cohen <ohad@wizery.com>
+Description:	Remote processor coredump configuration
+
+		Reports the coredump configuration of the remote processor,
+		which will be one of:
+
+		"default"
+		"inline"
+		"disabled"
+
+		"default" means when the remote processor's coredump is
+		collected it will be copied to a separate buffer and that
+		buffer is exposed to userspace.
+
+		"inline" means when the remote processor's coredump is
+		collected userspace will directly read from the remote
+		processor's device memory. Extra buffer will not be used to
+		copy the dump. Also recovery process will not proceed until
+		all data is read by usersapce.
+
+		"disabled" means no dump will be collected.
diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
index 2e3b3e2..732770e 100644
--- a/drivers/remoteproc/remoteproc_debugfs.c
+++ b/drivers/remoteproc/remoteproc_debugfs.c
@@ -28,94 +28,6 @@
 static struct dentry *rproc_dbg;
 
 /*
- * A coredump-configuration-to-string lookup table, for exposing a
- * human readable configuration via debugfs. Always keep in sync with
- * enum rproc_coredump_mechanism
- */
-static const char * const rproc_coredump_str[] = {
-	[RPROC_COREDUMP_DEFAULT]	= "default",
-	[RPROC_COREDUMP_INLINE]		= "inline",
-	[RPROC_COREDUMP_DISABLED]	= "disabled",
-};
-
-/* Expose the current coredump configuration via debugfs */
-static ssize_t rproc_coredump_read(struct file *filp, char __user *userbuf,
-				   size_t count, loff_t *ppos)
-{
-	struct rproc *rproc = filp->private_data;
-	char buf[20];
-	int len;
-
-	len = scnprintf(buf, sizeof(buf), "%s\n",
-			rproc_coredump_str[rproc->dump_conf]);
-
-	return simple_read_from_buffer(userbuf, count, ppos, buf, len);
-}
-
-/*
- * By writing to the 'coredump' debugfs entry, we control the behavior of the
- * coredump mechanism dynamically. The default value of this entry is "default".
- *
- * The 'coredump' debugfs entry supports these commands:
- *
- * default:	This is the default coredump mechanism. When the remoteproc
- *		crashes the entire coredump will be copied to a separate buffer
- *		and exposed to userspace.
- *
- * inline:	The coredump will not be copied to a separate buffer and the
- *		recovery process will have to wait until data is read by
- *		userspace. But this avoid usage of extra memory.
- *
- * disabled:	This will disable coredump. Recovery will proceed without
- *		collecting any dump.
- */
-static ssize_t rproc_coredump_write(struct file *filp,
-				    const char __user *user_buf, size_t count,
-				    loff_t *ppos)
-{
-	struct rproc *rproc = filp->private_data;
-	int ret, err = 0;
-	char buf[20];
-
-	if (count > sizeof(buf))
-		return -EINVAL;
-
-	ret = copy_from_user(buf, user_buf, count);
-	if (ret)
-		return -EFAULT;
-
-	/* remove end of line */
-	if (buf[count - 1] == '\n')
-		buf[count - 1] = '\0';
-
-	if (rproc->state == RPROC_CRASHED) {
-		dev_err(&rproc->dev, "can't change coredump configuration\n");
-		err = -EBUSY;
-		goto out;
-	}
-
-	if (!strncmp(buf, "disable", count)) {
-		rproc->dump_conf = RPROC_COREDUMP_DISABLED;
-	} else if (!strncmp(buf, "inline", count)) {
-		rproc->dump_conf = RPROC_COREDUMP_INLINE;
-	} else if (!strncmp(buf, "default", count)) {
-		rproc->dump_conf = RPROC_COREDUMP_DEFAULT;
-	} else {
-		dev_err(&rproc->dev, "Invalid coredump configuration\n");
-		err = -EINVAL;
-	}
-out:
-	return err ? err : count;
-}
-
-static const struct file_operations rproc_coredump_fops = {
-	.read = rproc_coredump_read,
-	.write = rproc_coredump_write,
-	.open = simple_open,
-	.llseek = generic_file_llseek,
-};
-
-/*
  * Some remote processors may support dumping trace logs into a shared
  * memory buffer. We expose this trace buffer using debugfs, so users
  * can easily tell what's going on remotely.
@@ -425,8 +337,6 @@ void rproc_create_debug_dir(struct rproc *rproc)
 			    rproc, &rproc_rsc_table_fops);
 	debugfs_create_file("carveout_memories", 0400, rproc->dbg_dir,
 			    rproc, &rproc_carveouts_fops);
-	debugfs_create_file("coredump", 0600, rproc->dbg_dir,
-			    rproc, &rproc_coredump_fops);
 }
 
 void __init rproc_init_debugfs(void)
diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
index eea514c..2a44571 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -10,6 +10,69 @@
 
 #define to_rproc(d) container_of(d, struct rproc, dev)
 
+/*
+ * A coredump-configuration-to-string lookup table, for exposing a
+ * human readable configuration via sysfs. Always keep in sync with
+ * enum rproc_coredump_mechanism
+ */
+static const char * const rproc_coredump_str[] = {
+	[RPROC_COREDUMP_DEFAULT]	= "default",
+	[RPROC_COREDUMP_INLINE]		= "inline",
+	[RPROC_COREDUMP_DISABLED]	= "disabled",
+};
+
+/* Expose the current coredump configuration via debugfs */
+static ssize_t coredump_show(struct device *dev,
+			     struct device_attribute *attr, char *buf)
+{
+	struct rproc *rproc = to_rproc(dev);
+
+	return sprintf(buf, "%s\n", rproc_coredump_str[rproc->dump_conf]);
+}
+
+/*
+ * By writing to the 'coredump' sysfs entry, we control the behavior of the
+ * coredump mechanism dynamically. The default value of this entry is "default".
+ *
+ * The 'coredump' sysfs entry supports these commands:
+ *
+ * default:	This is the default coredump mechanism. When the remoteproc
+ *		crashes the entire coredump will be copied to a separate buffer
+ *		and exposed to userspace.
+ *
+ * inline:	The coredump will not be copied to a separate buffer and the
+ *		recovery process will have to wait until data is read by
+ *		userspace. But this avoid usage of extra memory.
+ *
+ * disabled:	This will disable coredump. Recovery will proceed without
+ *		collecting any dump.
+ */
+static ssize_t coredump_store(struct device *dev,
+			      struct device_attribute *attr,
+			      const char *buf, size_t count)
+{
+	struct rproc *rproc = to_rproc(dev);
+
+	if (rproc->state == RPROC_CRASHED) {
+		dev_err(&rproc->dev, "can't change coredump configuration\n");
+		return -EBUSY;
+	}
+
+	if (sysfs_streq(buf, "disabled")) {
+		rproc->dump_conf = RPROC_COREDUMP_DISABLED;
+	} else if (sysfs_streq(buf, "inline")) {
+		rproc->dump_conf = RPROC_COREDUMP_INLINE;
+	} else if (sysfs_streq(buf, "default")) {
+		rproc->dump_conf = RPROC_COREDUMP_DEFAULT;
+	} else {
+		dev_err(&rproc->dev, "Invalid coredump configuration\n");
+		return -EINVAL;
+	}
+
+	return count;
+}
+static DEVICE_ATTR_RW(coredump);
+
 /* Expose the loaded / running firmware name via sysfs */
 static ssize_t firmware_show(struct device *dev, struct device_attribute *attr,
 			  char *buf)
@@ -138,6 +201,7 @@ static ssize_t name_show(struct device *dev, struct device_attribute *attr,
 static DEVICE_ATTR_RO(name);
 
 static struct attribute *rproc_attrs[] = {
+	&dev_attr_coredump.attr,
 	&dev_attr_firmware.attr,
 	&dev_attr_state.attr,
 	&dev_attr_name.attr,
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v6 2/3] remoteproc: Move recovery configuration to sysfs

[Rishabh Bhatnagar]

Move recovery configuration from debugfs to sysfs.This will
allow usage of this configuration feature in production
devices where access to debugfs might be limited.

Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
---
 Documentation/ABI/testing/sysfs-class-remoteproc | 20 ++++++
 drivers/remoteproc/remoteproc_debugfs.c          | 78 ------------------------
 drivers/remoteproc/remoteproc_sysfs.c            | 56 +++++++++++++++++
 3 files changed, 76 insertions(+), 78 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-remoteproc b/Documentation/ABI/testing/sysfs-class-remoteproc
index f6c44fa..7368b50 100644
--- a/Documentation/ABI/testing/sysfs-class-remoteproc
+++ b/Documentation/ABI/testing/sysfs-class-remoteproc
@@ -82,3 +82,23 @@ Description:	Remote processor coredump configuration
 		all data is read by usersapce.
 
 		"disabled" means no dump will be collected.
+
+What:		/sys/class/remoteproc/.../recovery
+Date:		July 2020
+Contact:	Bjorn Andersson <bjorn.andersson@linaro.org>, Ohad Ben-Cohen <ohad@wizery.com>
+Description:	Remote processor recovery mechanism
+
+		Reports the recovery mechanism of the remote processor,
+		which will be one of:
+
+		"enabled"
+		"disabled"
+
+		"enabled" means, the remote processor will be automatically
+		recovered whenever it crashes. Moreover, if the remote
+		processor crashes while recovery is disabled, it will
+		be automatically recovered too as soon as recovery is enabled.
+
+		"disabled" means, a remote processor will remain in a crashed
+		state if it crashes. This is useful for debugging purposes;
+		without it, debugging a crash is substantially harder.
diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
index 732770e..c505f0e 100644
--- a/drivers/remoteproc/remoteproc_debugfs.c
+++ b/drivers/remoteproc/remoteproc_debugfs.c
@@ -84,82 +84,6 @@ static const struct file_operations rproc_name_ops = {
 	.llseek	= generic_file_llseek,
 };
 
-/* expose recovery flag via debugfs */
-static ssize_t rproc_recovery_read(struct file *filp, char __user *userbuf,
-				   size_t count, loff_t *ppos)
-{
-	struct rproc *rproc = filp->private_data;
-	char *buf = rproc->recovery_disabled ? "disabled\n" : "enabled\n";
-
-	return simple_read_from_buffer(userbuf, count, ppos, buf, strlen(buf));
-}
-
-/*
- * By writing to the 'recovery' debugfs entry, we control the behavior of the
- * recovery mechanism dynamically. The default value of this entry is "enabled".
- *
- * The 'recovery' debugfs entry supports these commands:
- *
- * enabled:	When enabled, the remote processor will be automatically
- *		recovered whenever it crashes. Moreover, if the remote
- *		processor crashes while recovery is disabled, it will
- *		be automatically recovered too as soon as recovery is enabled.
- *
- * disabled:	When disabled, a remote processor will remain in a crashed
- *		state if it crashes. This is useful for debugging purposes;
- *		without it, debugging a crash is substantially harder.
- *
- * recover:	This function will trigger an immediate recovery if the
- *		remote processor is in a crashed state, without changing
- *		or checking the recovery state (enabled/disabled).
- *		This is useful during debugging sessions, when one expects
- *		additional crashes to happen after enabling recovery. In this
- *		case, enabling recovery will make it hard to debug subsequent
- *		crashes, so it's recommended to keep recovery disabled, and
- *		instead use the "recover" command as needed.
- */
-static ssize_t
-rproc_recovery_write(struct file *filp, const char __user *user_buf,
-		     size_t count, loff_t *ppos)
-{
-	struct rproc *rproc = filp->private_data;
-	char buf[10];
-	int ret;
-
-	if (count < 1 || count > sizeof(buf))
-		return -EINVAL;
-
-	ret = copy_from_user(buf, user_buf, count);
-	if (ret)
-		return -EFAULT;
-
-	/* remove end of line */
-	if (buf[count - 1] == '\n')
-		buf[count - 1] = '\0';
-
-	if (!strncmp(buf, "enabled", count)) {
-		/* change the flag and begin the recovery process if needed */
-		rproc->recovery_disabled = false;
-		rproc_trigger_recovery(rproc);
-	} else if (!strncmp(buf, "disabled", count)) {
-		rproc->recovery_disabled = true;
-	} else if (!strncmp(buf, "recover", count)) {
-		/* begin the recovery process without changing the flag */
-		rproc_trigger_recovery(rproc);
-	} else {
-		return -EINVAL;
-	}
-
-	return count;
-}
-
-static const struct file_operations rproc_recovery_ops = {
-	.read = rproc_recovery_read,
-	.write = rproc_recovery_write,
-	.open = simple_open,
-	.llseek = generic_file_llseek,
-};
-
 /* expose the crash trigger via debugfs */
 static ssize_t
 rproc_crash_write(struct file *filp, const char __user *user_buf,
@@ -329,8 +253,6 @@ void rproc_create_debug_dir(struct rproc *rproc)
 
 	debugfs_create_file("name", 0400, rproc->dbg_dir,
 			    rproc, &rproc_name_ops);
-	debugfs_create_file("recovery", 0600, rproc->dbg_dir,
-			    rproc, &rproc_recovery_ops);
 	debugfs_create_file("crash", 0200, rproc->dbg_dir,
 			    rproc, &rproc_crash_ops);
 	debugfs_create_file("resource_table", 0400, rproc->dbg_dir,
diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
index 2a44571..e60a014 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -10,6 +10,61 @@
 
 #define to_rproc(d) container_of(d, struct rproc, dev)
 
+static ssize_t recovery_show(struct device *dev,
+			     struct device_attribute *attr, char *buf)
+{
+	struct rproc *rproc = to_rproc(dev);
+
+	return sprintf(buf, "%s", rproc->recovery_disabled ? "disabled\n" : "enabled\n");
+}
+
+/*
+ * By writing to the 'recovery' sysfs entry, we control the behavior of the
+ * recovery mechanism dynamically. The default value of this entry is "enabled".
+ *
+ * The 'recovery' sysfs entry supports these commands:
+ *
+ * enabled:	When enabled, the remote processor will be automatically
+ *		recovered whenever it crashes. Moreover, if the remote
+ *		processor crashes while recovery is disabled, it will
+ *		be automatically recovered too as soon as recovery is enabled.
+ *
+ * disabled:	When disabled, a remote processor will remain in a crashed
+ *		state if it crashes. This is useful for debugging purposes;
+ *		without it, debugging a crash is substantially harder.
+ *
+ * recover:	This function will trigger an immediate recovery if the
+ *		remote processor is in a crashed state, without changing
+ *		or checking the recovery state (enabled/disabled).
+ *		This is useful during debugging sessions, when one expects
+ *		additional crashes to happen after enabling recovery. In this
+ *		case, enabling recovery will make it hard to debug subsequent
+ *		crashes, so it's recommended to keep recovery disabled, and
+ *		instead use the "recover" command as needed.
+ */
+static ssize_t recovery_store(struct device *dev,
+			      struct device_attribute *attr,
+			      const char *buf, size_t count)
+{
+	struct rproc *rproc = to_rproc(dev);
+
+	if (sysfs_streq(buf, "enabled")) {
+		/* change the flag and begin the recovery process if needed */
+		rproc->recovery_disabled = false;
+		rproc_trigger_recovery(rproc);
+	} else if (sysfs_streq(buf, "disabled")) {
+		rproc->recovery_disabled = true;
+	} else if (sysfs_streq(buf, "recover")) {
+		/* begin the recovery process without changing the flag */
+		rproc_trigger_recovery(rproc);
+	} else {
+		return -EINVAL;
+	}
+
+	return count;
+}
+static DEVICE_ATTR_RW(recovery);
+
 /*
  * A coredump-configuration-to-string lookup table, for exposing a
  * human readable configuration via sysfs. Always keep in sync with
@@ -202,6 +257,7 @@ static DEVICE_ATTR_RO(name);
 
 static struct attribute *rproc_attrs[] = {
 	&dev_attr_coredump.attr,
+	&dev_attr_recovery.attr,
 	&dev_attr_firmware.attr,
 	&dev_attr_state.attr,
 	&dev_attr_name.attr,
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v6 3/3] remoteproc: Change default dump configuration to "disabled"

[Rishabh Bhatnagar]

Disable the coredump collection by default as doing so makes sense
for production devices. Also rename the "default" configuration
option to "enabled" to avoid confusion.

Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
---
 Documentation/ABI/testing/sysfs-class-remoteproc | 10 ++++++----
 drivers/remoteproc/remoteproc_coredump.c         |  6 +++---
 drivers/remoteproc/remoteproc_sysfs.c            | 10 +++++-----
 include/linux/remoteproc.h                       |  8 ++++----
 4 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-remoteproc b/Documentation/ABI/testing/sysfs-class-remoteproc
index 7368b50..dec1686 100644
--- a/Documentation/ABI/testing/sysfs-class-remoteproc
+++ b/Documentation/ABI/testing/sysfs-class-remoteproc
@@ -67,11 +67,14 @@ Description:	Remote processor coredump configuration
 		Reports the coredump configuration of the remote processor,
 		which will be one of:
 
-		"default"
-		"inline"
 		"disabled"
+		"enabled"
+		"inline"
+
+		"disabled" means no dump will be collected. This is the
+		default remoteproc coredump configuration.
 
-		"default" means when the remote processor's coredump is
+		"enabled" means when the remote processor's coredump is
 		collected it will be copied to a separate buffer and that
 		buffer is exposed to userspace.
 
@@ -81,7 +84,6 @@ Description:	Remote processor coredump configuration
 		copy the dump. Also recovery process will not proceed until
 		all data is read by usersapce.
 
-		"disabled" means no dump will be collected.
 
 What:		/sys/class/remoteproc/.../recovery
 Date:		July 2020
diff --git a/drivers/remoteproc/remoteproc_coredump.c b/drivers/remoteproc/remoteproc_coredump.c
index bb15a29..34530dc 100644
--- a/drivers/remoteproc/remoteproc_coredump.c
+++ b/drivers/remoteproc/remoteproc_coredump.c
@@ -257,7 +257,7 @@ void rproc_coredump(struct rproc *rproc)
 		 * directly read from device memory.
 		 */
 		data_size += elf_size_of_phdr(class);
-		if (dump_conf == RPROC_COREDUMP_DEFAULT)
+		if (dump_conf == RPROC_COREDUMP_ENABLED)
 			data_size += segment->size;
 
 		phnum++;
@@ -297,14 +297,14 @@ void rproc_coredump(struct rproc *rproc)
 		elf_phdr_set_p_flags(class, phdr, PF_R | PF_W | PF_X);
 		elf_phdr_set_p_align(class, phdr, 0);
 
-		if (dump_conf == RPROC_COREDUMP_DEFAULT)
+		if (dump_conf == RPROC_COREDUMP_ENABLED)
 			rproc_copy_segment(rproc, data + offset, segment, 0,
 					   segment->size);
 
 		offset += elf_phdr_get_p_filesz(class, phdr);
 		phdr += elf_size_of_phdr(class);
 	}
-	if (dump_conf == RPROC_COREDUMP_DEFAULT) {
+	if (dump_conf == RPROC_COREDUMP_ENABLED) {
 		dev_coredumpv(&rproc->dev, data, data_size, GFP_KERNEL);
 		return;
 	}
diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
index e60a014..eb98ed2 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -68,12 +68,12 @@ static DEVICE_ATTR_RW(recovery);
 /*
  * A coredump-configuration-to-string lookup table, for exposing a
  * human readable configuration via sysfs. Always keep in sync with
- * enum rproc_coredump_mechanism
+ * enum rproc_dump_mechanism
  */
 static const char * const rproc_coredump_str[] = {
-	[RPROC_COREDUMP_DEFAULT]	= "default",
-	[RPROC_COREDUMP_INLINE]		= "inline",
 	[RPROC_COREDUMP_DISABLED]	= "disabled",
+	[RPROC_COREDUMP_ENABLED]	= "enabled",
+	[RPROC_COREDUMP_INLINE]		= "inline",
 };
 
 /* Expose the current coredump configuration via debugfs */
@@ -115,10 +115,10 @@ static ssize_t coredump_store(struct device *dev,
 
 	if (sysfs_streq(buf, "disabled")) {
 		rproc->dump_conf = RPROC_COREDUMP_DISABLED;
+	} else if (sysfs_streq(buf, "enabled")) {
+		rproc->dump_conf = RPROC_COREDUMP_ENABLED;
 	} else if (sysfs_streq(buf, "inline")) {
 		rproc->dump_conf = RPROC_COREDUMP_INLINE;
-	} else if (sysfs_streq(buf, "default")) {
-		rproc->dump_conf = RPROC_COREDUMP_DEFAULT;
 	} else {
 		dev_err(&rproc->dev, "Invalid coredump configuration\n");
 		return -EINVAL;
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 2fa68bf..3fa3ba6 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -442,16 +442,16 @@ enum rproc_crash_type {
 
 /**
  * enum rproc_dump_mechanism - Coredump options for core
- * @RPROC_COREDUMP_DEFAULT:	Copy dump to separate buffer and carry on with
+ * @RPROC_COREDUMP_DISABLED:	Don't perform any dump
+ * @RPROC_COREDUMP_ENABLED:	Copy dump to separate buffer and carry on with
 				recovery
  * @RPROC_COREDUMP_INLINE:	Read segments directly from device memory. Stall
 				recovery until all segments are read
- * @RPROC_COREDUMP_DISABLED:	Don't perform any dump
  */
 enum rproc_dump_mechanism {
-	RPROC_COREDUMP_DEFAULT,
-	RPROC_COREDUMP_INLINE,
 	RPROC_COREDUMP_DISABLED,
+	RPROC_COREDUMP_ENABLED,
+	RPROC_COREDUMP_INLINE,
 };
 
 /**
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v6 1/3] remoteproc: Move coredump configuration to sysfs

[Bjorn Andersson]

On Mon 28 Sep 17:17 CDT 2020, Rishabh Bhatnagar wrote:

> Move coredump configuration from debugfs to sysfs.This will
> allow usage of this configuration feature in production
> devices where access to debugfs might be limited.
> 
> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>

I like the end result of this series now Rishabh, but...

> ---
>  Documentation/ABI/testing/sysfs-class-remoteproc | 24 +++++++
>  drivers/remoteproc/remoteproc_debugfs.c          | 90 ------------------------
>  drivers/remoteproc/remoteproc_sysfs.c            | 64 +++++++++++++++++
>  3 files changed, 88 insertions(+), 90 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-remoteproc b/Documentation/ABI/testing/sysfs-class-remoteproc
> index 36094fb..f6c44fa 100644
> --- a/Documentation/ABI/testing/sysfs-class-remoteproc
> +++ b/Documentation/ABI/testing/sysfs-class-remoteproc
> @@ -58,3 +58,27 @@ Description:	Remote processor name
>  		Reports the name of the remote processor. This can be used by
>  		userspace in exactly identifying a remote processor and ease
>  		up the usage in modifying the 'firmware' or 'state' files.
> +
> +What:		/sys/class/remoteproc/.../coredump
> +Date:		July 2020
> +Contact:	Bjorn Andersson <bjorn.andersson@linaro.org>, Ohad Ben-Cohen <ohad@wizery.com>
> +Description:	Remote processor coredump configuration
> +
> +		Reports the coredump configuration of the remote processor,
> +		which will be one of:
> +
> +		"default"

...can you please make this "enabled" already here, so that patch 3
doesn't touch the definition of the sysfs. That way we don't introduce a
new sysfs attribute and immediately change it.

Please keep the remainder (rename of defines etc) in patch 3 as is - I
just want the string exposed to userspace to be consistent here.

(Or if you find the end result, you can move patch 3 before this change
in the series)

Regards,
Bjorn

> +		"inline"
> +		"disabled"
> +
> +		"default" means when the remote processor's coredump is
> +		collected it will be copied to a separate buffer and that
> +		buffer is exposed to userspace.
> +
> +		"inline" means when the remote processor's coredump is
> +		collected userspace will directly read from the remote
> +		processor's device memory. Extra buffer will not be used to
> +		copy the dump. Also recovery process will not proceed until
> +		all data is read by usersapce.
> +
> +		"disabled" means no dump will be collected.
> diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
> index 2e3b3e2..732770e 100644
> --- a/drivers/remoteproc/remoteproc_debugfs.c
> +++ b/drivers/remoteproc/remoteproc_debugfs.c
> @@ -28,94 +28,6 @@
>  static struct dentry *rproc_dbg;
>  
>  /*
> - * A coredump-configuration-to-string lookup table, for exposing a
> - * human readable configuration via debugfs. Always keep in sync with
> - * enum rproc_coredump_mechanism
> - */
> -static const char * const rproc_coredump_str[] = {
> -	[RPROC_COREDUMP_DEFAULT]	= "default",
> -	[RPROC_COREDUMP_INLINE]		= "inline",
> -	[RPROC_COREDUMP_DISABLED]	= "disabled",
> -};
> -
> -/* Expose the current coredump configuration via debugfs */
> -static ssize_t rproc_coredump_read(struct file *filp, char __user *userbuf,
> -				   size_t count, loff_t *ppos)
> -{
> -	struct rproc *rproc = filp->private_data;
> -	char buf[20];
> -	int len;
> -
> -	len = scnprintf(buf, sizeof(buf), "%s\n",
> -			rproc_coredump_str[rproc->dump_conf]);
> -
> -	return simple_read_from_buffer(userbuf, count, ppos, buf, len);
> -}
> -
> -/*
> - * By writing to the 'coredump' debugfs entry, we control the behavior of the
> - * coredump mechanism dynamically. The default value of this entry is "default".
> - *
> - * The 'coredump' debugfs entry supports these commands:
> - *
> - * default:	This is the default coredump mechanism. When the remoteproc
> - *		crashes the entire coredump will be copied to a separate buffer
> - *		and exposed to userspace.
> - *
> - * inline:	The coredump will not be copied to a separate buffer and the
> - *		recovery process will have to wait until data is read by
> - *		userspace. But this avoid usage of extra memory.
> - *
> - * disabled:	This will disable coredump. Recovery will proceed without
> - *		collecting any dump.
> - */
> -static ssize_t rproc_coredump_write(struct file *filp,
> -				    const char __user *user_buf, size_t count,
> -				    loff_t *ppos)
> -{
> -	struct rproc *rproc = filp->private_data;
> -	int ret, err = 0;
> -	char buf[20];
> -
> -	if (count > sizeof(buf))
> -		return -EINVAL;
> -
> -	ret = copy_from_user(buf, user_buf, count);
> -	if (ret)
> -		return -EFAULT;
> -
> -	/* remove end of line */
> -	if (buf[count - 1] == '\n')
> -		buf[count - 1] = '\0';
> -
> -	if (rproc->state == RPROC_CRASHED) {
> -		dev_err(&rproc->dev, "can't change coredump configuration\n");
> -		err = -EBUSY;
> -		goto out;
> -	}
> -
> -	if (!strncmp(buf, "disable", count)) {
> -		rproc->dump_conf = RPROC_COREDUMP_DISABLED;
> -	} else if (!strncmp(buf, "inline", count)) {
> -		rproc->dump_conf = RPROC_COREDUMP_INLINE;
> -	} else if (!strncmp(buf, "default", count)) {
> -		rproc->dump_conf = RPROC_COREDUMP_DEFAULT;
> -	} else {
> -		dev_err(&rproc->dev, "Invalid coredump configuration\n");
> -		err = -EINVAL;
> -	}
> -out:
> -	return err ? err : count;
> -}
> -
> -static const struct file_operations rproc_coredump_fops = {
> -	.read = rproc_coredump_read,
> -	.write = rproc_coredump_write,
> -	.open = simple_open,
> -	.llseek = generic_file_llseek,
> -};
> -
> -/*
>   * Some remote processors may support dumping trace logs into a shared
>   * memory buffer. We expose this trace buffer using debugfs, so users
>   * can easily tell what's going on remotely.
> @@ -425,8 +337,6 @@ void rproc_create_debug_dir(struct rproc *rproc)
>  			    rproc, &rproc_rsc_table_fops);
>  	debugfs_create_file("carveout_memories", 0400, rproc->dbg_dir,
>  			    rproc, &rproc_carveouts_fops);
> -	debugfs_create_file("coredump", 0600, rproc->dbg_dir,
> -			    rproc, &rproc_coredump_fops);
>  }
>  
>  void __init rproc_init_debugfs(void)
> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> index eea514c..2a44571 100644
> --- a/drivers/remoteproc/remoteproc_sysfs.c
> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> @@ -10,6 +10,69 @@
>  
>  #define to_rproc(d) container_of(d, struct rproc, dev)
>  
> +/*
> + * A coredump-configuration-to-string lookup table, for exposing a
> + * human readable configuration via sysfs. Always keep in sync with
> + * enum rproc_coredump_mechanism
> + */
> +static const char * const rproc_coredump_str[] = {
> +	[RPROC_COREDUMP_DEFAULT]	= "default",
> +	[RPROC_COREDUMP_INLINE]		= "inline",
> +	[RPROC_COREDUMP_DISABLED]	= "disabled",
> +};
> +
> +/* Expose the current coredump configuration via debugfs */
> +static ssize_t coredump_show(struct device *dev,
> +			     struct device_attribute *attr, char *buf)
> +{
> +	struct rproc *rproc = to_rproc(dev);
> +
> +	return sprintf(buf, "%s\n", rproc_coredump_str[rproc->dump_conf]);
> +}
> +
> +/*
> + * By writing to the 'coredump' sysfs entry, we control the behavior of the
> + * coredump mechanism dynamically. The default value of this entry is "default".
> + *
> + * The 'coredump' sysfs entry supports these commands:
> + *
> + * default:	This is the default coredump mechanism. When the remoteproc
> + *		crashes the entire coredump will be copied to a separate buffer
> + *		and exposed to userspace.
> + *
> + * inline:	The coredump will not be copied to a separate buffer and the
> + *		recovery process will have to wait until data is read by
> + *		userspace. But this avoid usage of extra memory.
> + *
> + * disabled:	This will disable coredump. Recovery will proceed without
> + *		collecting any dump.
> + */
> +static ssize_t coredump_store(struct device *dev,
> +			      struct device_attribute *attr,
> +			      const char *buf, size_t count)
> +{
> +	struct rproc *rproc = to_rproc(dev);
> +
> +	if (rproc->state == RPROC_CRASHED) {
> +		dev_err(&rproc->dev, "can't change coredump configuration\n");
> +		return -EBUSY;
> +	}
> +
> +	if (sysfs_streq(buf, "disabled")) {
> +		rproc->dump_conf = RPROC_COREDUMP_DISABLED;
> +	} else if (sysfs_streq(buf, "inline")) {
> +		rproc->dump_conf = RPROC_COREDUMP_INLINE;
> +	} else if (sysfs_streq(buf, "default")) {
> +		rproc->dump_conf = RPROC_COREDUMP_DEFAULT;
> +	} else {
> +		dev_err(&rproc->dev, "Invalid coredump configuration\n");
> +		return -EINVAL;
> +	}
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(coredump);
> +
>  /* Expose the loaded / running firmware name via sysfs */
>  static ssize_t firmware_show(struct device *dev, struct device_attribute *attr,
>  			  char *buf)
> @@ -138,6 +201,7 @@ static ssize_t name_show(struct device *dev, struct device_attribute *attr,
>  static DEVICE_ATTR_RO(name);
>  
>  static struct attribute *rproc_attrs[] = {
> +	&dev_attr_coredump.attr,
>  	&dev_attr_firmware.attr,
>  	&dev_attr_state.attr,
>  	&dev_attr_name.attr,
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH v6 0/3] Move recovery/coredump configuration to sysfs

[Arnaud POULIQUEN]

On 9/29/20 12:17 AM, Rishabh Bhatnagar wrote:
> From Android R onwards Google has restricted access to debugfs in user
> and user-debug builds. This restricts access to most of the features
> exposed through debugfs. 'Coredump' and 'Recovery' are critical
> interfaces that are required for remoteproc to work on Qualcomm Chipsets. 
> Coredump configuration needs to be set to "inline" in debug/test builds
> and "disabled" in production builds. Whereas recovery needs to be
> "disabled" for debugging purposes and "enabled" on production builds.
> This patch series removes the recovery/coredump entries from debugfs
> and moves them to sysfs. Also, this disables the coredump collection
> by default as this is a requirement for production devices.
> 
> Changelog:
> 
> v6 -> v5:
> - Disable coredump collection by default
> - Rename the "default" configuration to "enabled" to avoid confusion
> 
> v5 -> v4:
> - Fix the cover-letter of tha patch series.
> 
> v4 -> v3:
> - Remove the feature flag to expose recovery/coredump
> 
> v3 -> v2:
> - Remove the coredump/recovery entries from debugfs

Sorry i missed this and some associated discussion in V2...

I have also some concerns about the ABI breaks.
In ST and I suppose in several companies we have some 
test environments that use the debugfs to generate and/or get
the core dump.

Even if the stability of the debugfs is not guaranteed it would
be nice to keep both interface.

It seems that it is possible to create symbolic link in the debugfs
thanks to the "debugfs_create_symlink" function.
This seems allowing to keep files in both place without duplicating the code.
To be honest i have never used this function so I'm not 100% sure that this
would do the job...
But if you think that this could be a good compromise, i can test it.

Regards,
Arnaud

> - Expose recovery/coredump from sysfs under a feature flag
> 
> v1 -> v2:
> - Correct the contact name in the sysfs documentation.
> - Remove the redundant write documentation for coredump/recovery sysfs
> - Add a feature flag to make this interface switch configurable.
> 
> Rishabh Bhatnagar (3):
>   remoteproc: Move coredump configuration to sysfs
>   remoteproc: Move recovery configuration to sysfs
>   remoteproc: Change default dump configuration to "disabled"
> 
>  Documentation/ABI/testing/sysfs-class-remoteproc |  46 +++++++
>  drivers/remoteproc/remoteproc_coredump.c         |   6 +-
>  drivers/remoteproc/remoteproc_debugfs.c          | 168 -----------------------
>  drivers/remoteproc/remoteproc_sysfs.c            | 120 ++++++++++++++++
>  include/linux/remoteproc.h                       |   8 +-
>  5 files changed, 173 insertions(+), 175 deletions(-)
> 

Re: [PATCH v6 0/3] Move recovery/coredump configuration to sysfs

[Bjorn Andersson]

On Tue 29 Sep 03:44 CDT 2020, Arnaud POULIQUEN wrote:

> 
> 
> On 9/29/20 12:17 AM, Rishabh Bhatnagar wrote:
> > From Android R onwards Google has restricted access to debugfs in user
> > and user-debug builds. This restricts access to most of the features
> > exposed through debugfs. 'Coredump' and 'Recovery' are critical
> > interfaces that are required for remoteproc to work on Qualcomm Chipsets. 
> > Coredump configuration needs to be set to "inline" in debug/test builds
> > and "disabled" in production builds. Whereas recovery needs to be
> > "disabled" for debugging purposes and "enabled" on production builds.
> > This patch series removes the recovery/coredump entries from debugfs
> > and moves them to sysfs. Also, this disables the coredump collection
> > by default as this is a requirement for production devices.
> > 
> > Changelog:
> > 
> > v6 -> v5:
> > - Disable coredump collection by default
> > - Rename the "default" configuration to "enabled" to avoid confusion
> > 
> > v5 -> v4:
> > - Fix the cover-letter of tha patch series.
> > 
> > v4 -> v3:
> > - Remove the feature flag to expose recovery/coredump
> > 
> > v3 -> v2:
> > - Remove the coredump/recovery entries from debugfs
> 
> Sorry i missed this and some associated discussion in V2...
> 
> I have also some concerns about the ABI breaks.

Debugfs is not an ABI...

> In ST and I suppose in several companies we have some 
> test environments that use the debugfs to generate and/or get
> the core dump.
> 

I do however acknowledge the inconvenience you're facing...

> Even if the stability of the debugfs is not guaranteed it would
> be nice to keep both interface.
> 

...and I wouldn't mind keeping the debugfs interface around, at least
for some time to allow people to transition their tools/muscle memory.

> It seems that it is possible to create symbolic link in the debugfs
> thanks to the "debugfs_create_symlink" function.
> This seems allowing to keep files in both place without duplicating the code.
> To be honest i have never used this function so I'm not 100% sure that this
> would do the job...
> But if you think that this could be a good compromise, i can test it.
> 

The duplicated code is rather simple, so I don't mind the duplication -
for now.


So, how about we add the sysfs pieces of Rishabh's patches, leave out
the debugfs and then in a while (e.g. one LTS) we remove the debugfs
code?

Regards,
Bjorn

> Regards,
> Arnaud
> 
> > - Expose recovery/coredump from sysfs under a feature flag
> > 
> > v1 -> v2:
> > - Correct the contact name in the sysfs documentation.
> > - Remove the redundant write documentation for coredump/recovery sysfs
> > - Add a feature flag to make this interface switch configurable.
> > 
> > Rishabh Bhatnagar (3):
> >   remoteproc: Move coredump configuration to sysfs
> >   remoteproc: Move recovery configuration to sysfs
> >   remoteproc: Change default dump configuration to "disabled"
> > 
> >  Documentation/ABI/testing/sysfs-class-remoteproc |  46 +++++++
> >  drivers/remoteproc/remoteproc_coredump.c         |   6 +-
> >  drivers/remoteproc/remoteproc_debugfs.c          | 168 -----------------------
> >  drivers/remoteproc/remoteproc_sysfs.c            | 120 ++++++++++++++++
> >  include/linux/remoteproc.h                       |   8 +-
> >  5 files changed, 173 insertions(+), 175 deletions(-)
> > 

Re: [PATCH v6 0/3] Move recovery/coredump configuration to sysfs

[Arnaud POULIQUEN]

On 9/29/20 4:33 PM, Bjorn Andersson wrote:
> On Tue 29 Sep 03:44 CDT 2020, Arnaud POULIQUEN wrote:
> 
>>
>>
>> On 9/29/20 12:17 AM, Rishabh Bhatnagar wrote:
>>> From Android R onwards Google has restricted access to debugfs in user
>>> and user-debug builds. This restricts access to most of the features
>>> exposed through debugfs. 'Coredump' and 'Recovery' are critical
>>> interfaces that are required for remoteproc to work on Qualcomm Chipsets. 
>>> Coredump configuration needs to be set to "inline" in debug/test builds
>>> and "disabled" in production builds. Whereas recovery needs to be
>>> "disabled" for debugging purposes and "enabled" on production builds.
>>> This patch series removes the recovery/coredump entries from debugfs
>>> and moves them to sysfs. Also, this disables the coredump collection
>>> by default as this is a requirement for production devices.
>>>
>>> Changelog:
>>>
>>> v6 -> v5:
>>> - Disable coredump collection by default
>>> - Rename the "default" configuration to "enabled" to avoid confusion
>>>
>>> v5 -> v4:
>>> - Fix the cover-letter of tha patch series.
>>>
>>> v4 -> v3:
>>> - Remove the feature flag to expose recovery/coredump
>>>
>>> v3 -> v2:
>>> - Remove the coredump/recovery entries from debugfs
>>
>> Sorry i missed this and some associated discussion in V2...
>>
>> I have also some concerns about the ABI breaks.
> 
> Debugfs is not an ABI...
> 
>> In ST and I suppose in several companies we have some 
>> test environments that use the debugfs to generate and/or get
>> the core dump.
>>
> 
> I do however acknowledge the inconvenience you're facing...
> 
>> Even if the stability of the debugfs is not guaranteed it would
>> be nice to keep both interface.
>>
> 
> ...and I wouldn't mind keeping the debugfs interface around, at least
> for some time to allow people to transition their tools/muscle memory.
> 
>> It seems that it is possible to create symbolic link in the debugfs
>> thanks to the "debugfs_create_symlink" function.
>> This seems allowing to keep files in both place without duplicating the code.
>> To be honest i have never used this function so I'm not 100% sure that this
>> would do the job...
>> But if you think that this could be a good compromise, i can test it.
>>
> 
> The duplicated code is rather simple, so I don't mind the duplication -
> for now.
> 
> 
> So, how about we add the sysfs pieces of Rishabh's patches, leave out
> the debugfs and then in a while (e.g. one LTS) we remove the debugfs
> code?

This smooth transition seems to me a very good compromise.

Thanks,
Arnaud

> 
> Regards,
> Bjorn
> 
>> Regards,
>> Arnaud
>>
>>> - Expose recovery/coredump from sysfs under a feature flag
>>>
>>> v1 -> v2:
>>> - Correct the contact name in the sysfs documentation.
>>> - Remove the redundant write documentation for coredump/recovery sysfs
>>> - Add a feature flag to make this interface switch configurable.
>>>
>>> Rishabh Bhatnagar (3):
>>>   remoteproc: Move coredump configuration to sysfs
>>>   remoteproc: Move recovery configuration to sysfs
>>>   remoteproc: Change default dump configuration to "disabled"
>>>
>>>  Documentation/ABI/testing/sysfs-class-remoteproc |  46 +++++++
>>>  drivers/remoteproc/remoteproc_coredump.c         |   6 +-
>>>  drivers/remoteproc/remoteproc_debugfs.c          | 168 -----------------------
>>>  drivers/remoteproc/remoteproc_sysfs.c            | 120 ++++++++++++++++
>>>  include/linux/remoteproc.h                       |   8 +-
>>>  5 files changed, 173 insertions(+), 175 deletions(-)
>>>

Re: [PATCH v6 2/3] remoteproc: Move recovery configuration to sysfs

[Arnaud POULIQUEN]

On 9/29/20 12:17 AM, Rishabh Bhatnagar wrote:
> Move recovery configuration from debugfs to sysfs.This will
> allow usage of this configuration feature in production
> devices where access to debugfs might be limited.
> 
> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
> ---
>  Documentation/ABI/testing/sysfs-class-remoteproc | 20 ++++++
>  drivers/remoteproc/remoteproc_debugfs.c          | 78 ------------------------
>  drivers/remoteproc/remoteproc_sysfs.c            | 56 +++++++++++++++++
>  3 files changed, 76 insertions(+), 78 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-remoteproc b/Documentation/ABI/testing/sysfs-class-remoteproc
> index f6c44fa..7368b50 100644
> --- a/Documentation/ABI/testing/sysfs-class-remoteproc
> +++ b/Documentation/ABI/testing/sysfs-class-remoteproc
> @@ -82,3 +82,23 @@ Description:	Remote processor coredump configuration
>  		all data is read by usersapce.
>  
>  		"disabled" means no dump will be collected.
> +
> +What:		/sys/class/remoteproc/.../recovery
> +Date:		July 2020
> +Contact:	Bjorn Andersson <bjorn.andersson@linaro.org>, Ohad Ben-Cohen <ohad@wizery.com>
> +Description:	Remote processor recovery mechanism
> +
> +		Reports the recovery mechanism of the remote processor,
> +		which will be one of:
> +
> +		"enabled"
> +		"disabled"
> +
> +		"enabled" means, the remote processor will be automatically
> +		recovered whenever it crashes. Moreover, if the remote
> +		processor crashes while recovery is disabled, it will
> +		be automatically recovered too as soon as recovery is enabled.
> +
> +		"disabled" means, a remote processor will remain in a crashed
> +		state if it crashes. This is useful for debugging purposes;
> +		without it, debugging a crash is substantially harder.
> diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
> index 732770e..c505f0e 100644
> --- a/drivers/remoteproc/remoteproc_debugfs.c
> +++ b/drivers/remoteproc/remoteproc_debugfs.c
> @@ -84,82 +84,6 @@ static const struct file_operations rproc_name_ops = {
>  	.llseek	= generic_file_llseek,
>  };
>  
> -/* expose recovery flag via debugfs */
> -static ssize_t rproc_recovery_read(struct file *filp, char __user *userbuf,
> -				   size_t count, loff_t *ppos)
> -{
> -	struct rproc *rproc = filp->private_data;
> -	char *buf = rproc->recovery_disabled ? "disabled\n" : "enabled\n";
> -
> -	return simple_read_from_buffer(userbuf, count, ppos, buf, strlen(buf));
> -}
> -
> -/*
> - * By writing to the 'recovery' debugfs entry, we control the behavior of the
> - * recovery mechanism dynamically. The default value of this entry is "enabled".
> - *
> - * The 'recovery' debugfs entry supports these commands:
> - *
> - * enabled:	When enabled, the remote processor will be automatically
> - *		recovered whenever it crashes. Moreover, if the remote
> - *		processor crashes while recovery is disabled, it will
> - *		be automatically recovered too as soon as recovery is enabled.
> - *
> - * disabled:	When disabled, a remote processor will remain in a crashed
> - *		state if it crashes. This is useful for debugging purposes;
> - *		without it, debugging a crash is substantially harder.
> - *
> - * recover:	This function will trigger an immediate recovery if the
> - *		remote processor is in a crashed state, without changing
> - *		or checking the recovery state (enabled/disabled).
> - *		This is useful during debugging sessions, when one expects
> - *		additional crashes to happen after enabling recovery. In this
> - *		case, enabling recovery will make it hard to debug subsequent
> - *		crashes, so it's recommended to keep recovery disabled, and
> - *		instead use the "recover" command as needed.
> - */
> -static ssize_t
> -rproc_recovery_write(struct file *filp, const char __user *user_buf,
> -		     size_t count, loff_t *ppos)
> -{
> -	struct rproc *rproc = filp->private_data;
> -	char buf[10];
> -	int ret;
> -
> -	if (count < 1 || count > sizeof(buf))
> -		return -EINVAL;
> -
> -	ret = copy_from_user(buf, user_buf, count);
> -	if (ret)
> -		return -EFAULT;
> -
> -	/* remove end of line */
> -	if (buf[count - 1] == '\n')
> -		buf[count - 1] = '\0';
> -
> -	if (!strncmp(buf, "enabled", count)) {
> -		/* change the flag and begin the recovery process if needed */
> -		rproc->recovery_disabled = false;
> -		rproc_trigger_recovery(rproc);
> -	} else if (!strncmp(buf, "disabled", count)) {
> -		rproc->recovery_disabled = true;
> -	} else if (!strncmp(buf, "recover", count)) {
> -		/* begin the recovery process without changing the flag */
> -		rproc_trigger_recovery(rproc);
> -	} else {
> -		return -EINVAL;
> -	}
> -
> -	return count;
> -}
> -
> -static const struct file_operations rproc_recovery_ops = {
> -	.read = rproc_recovery_read,
> -	.write = rproc_recovery_write,
> -	.open = simple_open,
> -	.llseek = generic_file_llseek,
> -};
> -
>  /* expose the crash trigger via debugfs */
>  static ssize_t
>  rproc_crash_write(struct file *filp, const char __user *user_buf,
> @@ -329,8 +253,6 @@ void rproc_create_debug_dir(struct rproc *rproc)
>  
>  	debugfs_create_file("name", 0400, rproc->dbg_dir,
>  			    rproc, &rproc_name_ops);
> -	debugfs_create_file("recovery", 0600, rproc->dbg_dir,
> -			    rproc, &rproc_recovery_ops);
>  	debugfs_create_file("crash", 0200, rproc->dbg_dir,
>  			    rproc, &rproc_crash_ops);
>  	debugfs_create_file("resource_table", 0400, rproc->dbg_dir,
> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> index 2a44571..e60a014 100644
> --- a/drivers/remoteproc/remoteproc_sysfs.c
> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> @@ -10,6 +10,61 @@
>  
>  #define to_rproc(d) container_of(d, struct rproc, dev)
>  
> +static ssize_t recovery_show(struct device *dev,
> +			     struct device_attribute *attr, char *buf)
> +{
> +	struct rproc *rproc = to_rproc(dev);
> +
> +	return sprintf(buf, "%s", rproc->recovery_disabled ? "disabled\n" : "enabled\n");
> +}
> +
> +/*
> + * By writing to the 'recovery' sysfs entry, we control the behavior of the
> + * recovery mechanism dynamically. The default value of this entry is "enabled".
> + *
> + * The 'recovery' sysfs entry supports these commands:
> + *
> + * enabled:	When enabled, the remote processor will be automatically
> + *		recovered whenever it crashes. Moreover, if the remote
> + *		processor crashes while recovery is disabled, it will
> + *		be automatically recovered too as soon as recovery is enabled.
> + *
> + * disabled:	When disabled, a remote processor will remain in a crashed
> + *		state if it crashes. This is useful for debugging purposes;
> + *		without it, debugging a crash is substantially harder.
> + *
> + * recover:	This function will trigger an immediate recovery if the
> + *		remote processor is in a crashed state, without changing
> + *		or checking the recovery state (enabled/disabled).
> + *		This is useful during debugging sessions, when one expects

Recovery seems to be related to debugging, is it wise to expose it in sysfs?
This would allow the user space to restart remote processor and generate core
dumps at any time...Is this the expected behavior?

For your information, in the ST test suite, we use this mechanism also to simulate a
proc crash. This is useful for testing the recovery of RPMsg channels...

Regards
Arnaud

> + *		additional crashes to happen after enabling recovery. In this
> + *		case, enabling recovery will make it hard to debug subsequent
> + *		crashes, so it's recommended to keep recovery disabled, and
> + *		instead use the "recover" command as needed.
> + */
> +static ssize_t recovery_store(struct device *dev,
> +			      struct device_attribute *attr,
> +			      const char *buf, size_t count)
> +{
> +	struct rproc *rproc = to_rproc(dev);
> +
> +	if (sysfs_streq(buf, "enabled")) {
> +		/* change the flag and begin the recovery process if needed */
> +		rproc->recovery_disabled = false;
> +		rproc_trigger_recovery(rproc);
> +	} else if (sysfs_streq(buf, "disabled")) {
> +		rproc->recovery_disabled = true;
> +	} else if (sysfs_streq(buf, "recover")) {
> +		/* begin the recovery process without changing the flag */
> +		rproc_trigger_recovery(rproc);
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(recovery);
> +
>  /*
>   * A coredump-configuration-to-string lookup table, for exposing a
>   * human readable configuration via sysfs. Always keep in sync with
> @@ -202,6 +257,7 @@ static DEVICE_ATTR_RO(name);
>  
>  static struct attribute *rproc_attrs[] = {
>  	&dev_attr_coredump.attr,
> +	&dev_attr_recovery.attr,
>  	&dev_attr_firmware.attr,
>  	&dev_attr_state.attr,
>  	&dev_attr_name.attr,
> 

Re: [PATCH v6 2/3] remoteproc: Move recovery configuration to sysfs

[Arnaud POULIQUEN]

On 9/30/20 10:11 AM, Arnaud POULIQUEN wrote:
> 
> 
> On 9/29/20 12:17 AM, Rishabh Bhatnagar wrote:
>> Move recovery configuration from debugfs to sysfs.This will
>> allow usage of this configuration feature in production
>> devices where access to debugfs might be limited.
>>
>> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
>> ---
>>  Documentation/ABI/testing/sysfs-class-remoteproc | 20 ++++++
>>  drivers/remoteproc/remoteproc_debugfs.c          | 78 ------------------------
>>  drivers/remoteproc/remoteproc_sysfs.c            | 56 +++++++++++++++++
>>  3 files changed, 76 insertions(+), 78 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-remoteproc b/Documentation/ABI/testing/sysfs-class-remoteproc
>> index f6c44fa..7368b50 100644
>> --- a/Documentation/ABI/testing/sysfs-class-remoteproc
>> +++ b/Documentation/ABI/testing/sysfs-class-remoteproc
>> @@ -82,3 +82,23 @@ Description:	Remote processor coredump configuration
>>  		all data is read by usersapce.
>>  
>>  		"disabled" means no dump will be collected.
>> +
>> +What:		/sys/class/remoteproc/.../recovery
>> +Date:		July 2020
>> +Contact:	Bjorn Andersson <bjorn.andersson@linaro.org>, Ohad Ben-Cohen <ohad@wizery.com>
>> +Description:	Remote processor recovery mechanism
>> +
>> +		Reports the recovery mechanism of the remote processor,
>> +		which will be one of:
>> +
>> +		"enabled"
>> +		"disabled"
>> +
>> +		"enabled" means, the remote processor will be automatically
>> +		recovered whenever it crashes. Moreover, if the remote
>> +		processor crashes while recovery is disabled, it will
>> +		be automatically recovered too as soon as recovery is enabled.
>> +
>> +		"disabled" means, a remote processor will remain in a crashed
>> +		state if it crashes. This is useful for debugging purposes;
>> +		without it, debugging a crash is substantially harder.
>> diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
>> index 732770e..c505f0e 100644
>> --- a/drivers/remoteproc/remoteproc_debugfs.c
>> +++ b/drivers/remoteproc/remoteproc_debugfs.c
>> @@ -84,82 +84,6 @@ static const struct file_operations rproc_name_ops = {
>>  	.llseek	= generic_file_llseek,
>>  };
>>  
>> -/* expose recovery flag via debugfs */
>> -static ssize_t rproc_recovery_read(struct file *filp, char __user *userbuf,
>> -				   size_t count, loff_t *ppos)
>> -{
>> -	struct rproc *rproc = filp->private_data;
>> -	char *buf = rproc->recovery_disabled ? "disabled\n" : "enabled\n";
>> -
>> -	return simple_read_from_buffer(userbuf, count, ppos, buf, strlen(buf));
>> -}
>> -
>> -/*
>> - * By writing to the 'recovery' debugfs entry, we control the behavior of the
>> - * recovery mechanism dynamically. The default value of this entry is "enabled".
>> - *
>> - * The 'recovery' debugfs entry supports these commands:
>> - *
>> - * enabled:	When enabled, the remote processor will be automatically
>> - *		recovered whenever it crashes. Moreover, if the remote
>> - *		processor crashes while recovery is disabled, it will
>> - *		be automatically recovered too as soon as recovery is enabled.
>> - *
>> - * disabled:	When disabled, a remote processor will remain in a crashed
>> - *		state if it crashes. This is useful for debugging purposes;
>> - *		without it, debugging a crash is substantially harder.
>> - *
>> - * recover:	This function will trigger an immediate recovery if the
>> - *		remote processor is in a crashed state, without changing
>> - *		or checking the recovery state (enabled/disabled).
>> - *		This is useful during debugging sessions, when one expects
>> - *		additional crashes to happen after enabling recovery. In this
>> - *		case, enabling recovery will make it hard to debug subsequent
>> - *		crashes, so it's recommended to keep recovery disabled, and
>> - *		instead use the "recover" command as needed.
>> - */
>> -static ssize_t
>> -rproc_recovery_write(struct file *filp, const char __user *user_buf,
>> -		     size_t count, loff_t *ppos)
>> -{
>> -	struct rproc *rproc = filp->private_data;
>> -	char buf[10];
>> -	int ret;
>> -
>> -	if (count < 1 || count > sizeof(buf))
>> -		return -EINVAL;
>> -
>> -	ret = copy_from_user(buf, user_buf, count);
>> -	if (ret)
>> -		return -EFAULT;
>> -
>> -	/* remove end of line */
>> -	if (buf[count - 1] == '\n')
>> -		buf[count - 1] = '\0';
>> -
>> -	if (!strncmp(buf, "enabled", count)) {
>> -		/* change the flag and begin the recovery process if needed */
>> -		rproc->recovery_disabled = false;
>> -		rproc_trigger_recovery(rproc);
>> -	} else if (!strncmp(buf, "disabled", count)) {
>> -		rproc->recovery_disabled = true;
>> -	} else if (!strncmp(buf, "recover", count)) {
>> -		/* begin the recovery process without changing the flag */
>> -		rproc_trigger_recovery(rproc);
>> -	} else {
>> -		return -EINVAL;
>> -	}
>> -
>> -	return count;
>> -}
>> -
>> -static const struct file_operations rproc_recovery_ops = {
>> -	.read = rproc_recovery_read,
>> -	.write = rproc_recovery_write,
>> -	.open = simple_open,
>> -	.llseek = generic_file_llseek,
>> -};
>> -
>>  /* expose the crash trigger via debugfs */
>>  static ssize_t
>>  rproc_crash_write(struct file *filp, const char __user *user_buf,
>> @@ -329,8 +253,6 @@ void rproc_create_debug_dir(struct rproc *rproc)
>>  
>>  	debugfs_create_file("name", 0400, rproc->dbg_dir,
>>  			    rproc, &rproc_name_ops);
>> -	debugfs_create_file("recovery", 0600, rproc->dbg_dir,
>> -			    rproc, &rproc_recovery_ops);
>>  	debugfs_create_file("crash", 0200, rproc->dbg_dir,
>>  			    rproc, &rproc_crash_ops);
>>  	debugfs_create_file("resource_table", 0400, rproc->dbg_dir,
>> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
>> index 2a44571..e60a014 100644
>> --- a/drivers/remoteproc/remoteproc_sysfs.c
>> +++ b/drivers/remoteproc/remoteproc_sysfs.c
>> @@ -10,6 +10,61 @@
>>  
>>  #define to_rproc(d) container_of(d, struct rproc, dev)
>>  
>> +static ssize_t recovery_show(struct device *dev,
>> +			     struct device_attribute *attr, char *buf)
>> +{
>> +	struct rproc *rproc = to_rproc(dev);
>> +
>> +	return sprintf(buf, "%s", rproc->recovery_disabled ? "disabled\n" : "enabled\n");
>> +}
>> +
>> +/*
>> + * By writing to the 'recovery' sysfs entry, we control the behavior of the
>> + * recovery mechanism dynamically. The default value of this entry is "enabled".
>> + *
>> + * The 'recovery' sysfs entry supports these commands:
>> + *
>> + * enabled:	When enabled, the remote processor will be automatically
>> + *		recovered whenever it crashes. Moreover, if the remote
>> + *		processor crashes while recovery is disabled, it will
>> + *		be automatically recovered too as soon as recovery is enabled.
>> + *
>> + * disabled:	When disabled, a remote processor will remain in a crashed
>> + *		state if it crashes. This is useful for debugging purposes;
>> + *		without it, debugging a crash is substantially harder.
>> + *
>> + * recover:	This function will trigger an immediate recovery if the
>> + *		remote processor is in a crashed state, without changing
>> + *		or checking the recovery state (enabled/disabled).
>> + *		This is useful during debugging sessions, when one expects
> 
> Recovery seems to be related to debugging, is it wise to expose it in sysfs?
> This would allow the user space to restart remote processor and generate core
> dumps at any time...Is this the expected behavior?

This comment is wrong, please forget it. I mixed with some code from last Kernel LTS.
The state is checked in rproc_trigger_recovery, so no issue.

My apologize for this stupid comment. 

Regards
Arnaud

> 
> For your information, in the ST test suite, we use this mechanism also to simulate a
> proc crash. This is useful for testing the recovery of RPMsg channels...
> 
> Regards
> Arnaud
> 
>> + *		additional crashes to happen after enabling recovery. In this
>> + *		case, enabling recovery will make it hard to debug subsequent
>> + *		crashes, so it's recommended to keep recovery disabled, and
>> + *		instead use the "recover" command as needed.
>> + */
>> +static ssize_t recovery_store(struct device *dev,
>> +			      struct device_attribute *attr,
>> +			      const char *buf, size_t count)
>> +{
>> +	struct rproc *rproc = to_rproc(dev);
>> +
>> +	if (sysfs_streq(buf, "enabled")) {
>> +		/* change the flag and begin the recovery process if needed */
>> +		rproc->recovery_disabled = false;
>> +		rproc_trigger_recovery(rproc);
>> +	} else if (sysfs_streq(buf, "disabled")) {
>> +		rproc->recovery_disabled = true;
>> +	} else if (sysfs_streq(buf, "recover")) {
>> +		/* begin the recovery process without changing the flag */
>> +		rproc_trigger_recovery(rproc);
>> +	} else {
>> +		return -EINVAL;
>> +	}
>> +
>> +	return count;
>> +}
>> +static DEVICE_ATTR_RW(recovery);
>> +
>>  /*
>>   * A coredump-configuration-to-string lookup table, for exposing a
>>   * human readable configuration via sysfs. Always keep in sync with
>> @@ -202,6 +257,7 @@ static DEVICE_ATTR_RO(name);
>>  
>>  static struct attribute *rproc_attrs[] = {
>>  	&dev_attr_coredump.attr,
>> +	&dev_attr_recovery.attr,
>>  	&dev_attr_firmware.attr,
>>  	&dev_attr_state.attr,
>>  	&dev_attr_name.attr,
>>