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.
This patch series adds recovery/coredump configuration to sysfs interface
and disables coredump collection by default. Having coredump disabled by
default on production devices makes sense.
Changelog:
v7 -> v6:
- Keep the debugfs entries intact for now.
- Reorder the patches to have a consistent sysfs interface.
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: Change default dump configuration to "disabled"
remoteproc: Add coredump as part of sysfs interface
remoteproc: Add recovery configuration to the sysfs interface
Documentation/ABI/testing/sysfs-class-remoteproc | 44 +++++++++
drivers/remoteproc/remoteproc_coredump.c | 6 +-
drivers/remoteproc/remoteproc_debugfs.c | 23 +++--
drivers/remoteproc/remoteproc_sysfs.c | 119 +++++++++++++++++++++++
include/linux/remoteproc.h | 8 +-
5 files changed, 181 insertions(+), 19 deletions(-)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Add recovery configuration to the sysfs interface. This will
allow usage of this configuration feature in production
devices where access to debugfs might be limited.
Signed-off-by: Rishabh Bhatnagar <[email protected]>
---
Documentation/ABI/testing/sysfs-class-remoteproc | 20 +++++++++
drivers/remoteproc/remoteproc_sysfs.c | 56 ++++++++++++++++++++++++
2 files changed, 76 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-class-remoteproc b/Documentation/ABI/testing/sysfs-class-remoteproc
index 050bd25..066b9b6 100644
--- a/Documentation/ABI/testing/sysfs-class-remoteproc
+++ b/Documentation/ABI/testing/sysfs-class-remoteproc
@@ -82,3 +82,23 @@ Description: Remote processor coredump configuration
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.
+
+What: /sys/class/remoteproc/.../recovery
+Date: July 2020
+Contact: Bjorn Andersson <[email protected]>, Ohad Ben-Cohen <[email protected]>
+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_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
index 8500271..d1cf7bf 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
@@ -201,6 +256,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
Currently "default" configuration option means coredumps are
enabled. To avoid confusion rename the "default" configuration
option to "enabled" and disable collection of dumps by default
as doing so makes sense for production devices.
Signed-off-by: Rishabh Bhatnagar <[email protected]>
---
drivers/remoteproc/remoteproc_coredump.c | 6 +++---
drivers/remoteproc/remoteproc_debugfs.c | 23 +++++++++++------------
include/linux/remoteproc.h | 8 ++++----
3 files changed, 18 insertions(+), 19 deletions(-)
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_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
index 2e3b3e2..7e58453 100644
--- a/drivers/remoteproc/remoteproc_debugfs.c
+++ b/drivers/remoteproc/remoteproc_debugfs.c
@@ -33,9 +33,9 @@ static struct dentry *rproc_dbg;
* enum rproc_coredump_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 */
@@ -54,20 +54,19 @@ static ssize_t rproc_coredump_read(struct file *filp, char __user *userbuf,
/*
* By writing to the 'coredump' debugfs entry, we control the behavior of the
- * coredump mechanism dynamically. The default value of this entry is "default".
+ * coredump mechanism dynamically. The default value of this entry is "disabled".
*
* 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.
+ * disabled: By default coredump collection is disabled. Recovery will
+ * proceed without collecting any dump.
+ *
+ * enabled: 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,
@@ -94,12 +93,12 @@ static ssize_t rproc_coredump_write(struct file *filp,
goto out;
}
- if (!strncmp(buf, "disable", count)) {
+ if (!strncmp(buf, "disabled", count)) {
rproc->dump_conf = RPROC_COREDUMP_DISABLED;
+ } else if (!strncmp(buf, "enabled", count)) {
+ rproc->dump_conf = RPROC_COREDUMP_ENABLED;
} 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;
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