| Message ID | 1595977697-15389-3-git-send-email-rishabhb@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Move recovery and coredump interface to sysfs | expand |
On Tue, Jul 28, 2020 at 04:08:17PM -0700, Rishabh Bhatnagar wrote: > Expose recovery mechanism through sysfs rather than exposing through > debugfs. Some operating systems may limit access to debugfs through > access policies. This restricts user access to recovery mechanism, > hence move it to sysfs. > > Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org> > --- > Documentation/ABI/testing/sysfs-class-remoteproc | 36 +++++++++++ Please disregard my previous comment about making this a separate patch. I initially thought Jon Corbet would have to take this but it is not the case, it can go through Bjorn's tree. > drivers/remoteproc/remoteproc_debugfs.c | 77 ------------------------ > drivers/remoteproc/remoteproc_sysfs.c | 57 ++++++++++++++++++ > 3 files changed, 93 insertions(+), 77 deletions(-) > > diff --git a/Documentation/ABI/testing/sysfs-class-remoteproc b/Documentation/ABI/testing/sysfs-class-remoteproc > index 812582a..16c5267 100644 > --- a/Documentation/ABI/testing/sysfs-class-remoteproc > +++ b/Documentation/ABI/testing/sysfs-class-remoteproc > @@ -98,3 +98,39 @@ Description: Remote processor coredump configuration > > Writing "disable" will disable the coredump collection for > that remoteproc. > + > +What: /sys/class/remoteproc/.../recovery > +Date: July 2020 > +Contact: Rishabh Bhatnagar <rishabhb@codeaurora.org> Same comment as the previous patch > +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. > + > + Writing this file controls the recovery mechanism of the > + remote processor. The following options can be written: > + Same, I don't think we need to distinguish between reading and writing. The above would do just fine. > + "enabled" > + "disabled" > + "recover" > + > + Writing "enabled" will enable recovery and recover the remote > + processor if its crashed. > + > + Writing "disabled" will disable recovery and if crashed the > + remote processor will remain in crashed state. > + > + Writing "recover" will trigger an immediate recovery if the > + remote processor is in crashed state. > diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c > index 732770e..71194a0 100644 > --- a/drivers/remoteproc/remoteproc_debugfs.c > +++ b/drivers/remoteproc/remoteproc_debugfs.c > @@ -84,81 +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 > @@ -329,8 +254,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 40949a0..49b846e 100644 > --- a/drivers/remoteproc/remoteproc_sysfs.c > +++ b/drivers/remoteproc/remoteproc_sysfs.c > @@ -10,6 +10,62 @@ > > #define to_rproc(d) container_of(d, struct rproc, dev) > > +/* expose recovery flag via sysfs */ > +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 +257,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_recovery.attr, Here too I think it would be a good idea to make the feature configurable. Thanks, Mathieu > &dev_attr_coredump.attr, > &dev_attr_firmware.attr, > &dev_attr_state.attr, > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project >
diff --git a/Documentation/ABI/testing/sysfs-class-remoteproc b/Documentation/ABI/testing/sysfs-class-remoteproc index 812582a..16c5267 100644 --- a/Documentation/ABI/testing/sysfs-class-remoteproc +++ b/Documentation/ABI/testing/sysfs-class-remoteproc @@ -98,3 +98,39 @@ Description: Remote processor coredump configuration Writing "disable" will disable the coredump collection for that remoteproc. + +What: /sys/class/remoteproc/.../recovery +Date: July 2020 +Contact: Rishabh Bhatnagar <rishabhb@codeaurora.org> +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. + + Writing this file controls the recovery mechanism of the + remote processor. The following options can be written: + + "enabled" + "disabled" + "recover" + + Writing "enabled" will enable recovery and recover the remote + processor if its crashed. + + Writing "disabled" will disable recovery and if crashed the + remote processor will remain in crashed state. + + Writing "recover" will trigger an immediate recovery if the + remote processor is in crashed state. diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c index 732770e..71194a0 100644 --- a/drivers/remoteproc/remoteproc_debugfs.c +++ b/drivers/remoteproc/remoteproc_debugfs.c @@ -84,81 +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 @@ -329,8 +254,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 40949a0..49b846e 100644 --- a/drivers/remoteproc/remoteproc_sysfs.c +++ b/drivers/remoteproc/remoteproc_sysfs.c @@ -10,6 +10,62 @@ #define to_rproc(d) container_of(d, struct rproc, dev) +/* expose recovery flag via sysfs */ +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 +257,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_recovery.attr, &dev_attr_coredump.attr, &dev_attr_firmware.attr, &dev_attr_state.attr,
Expose recovery mechanism through sysfs rather than exposing through debugfs. Some operating systems may limit access to debugfs through access policies. This restricts user access to recovery mechanism, hence move it to sysfs. Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org> --- Documentation/ABI/testing/sysfs-class-remoteproc | 36 +++++++++++ drivers/remoteproc/remoteproc_debugfs.c | 77 ------------------------ drivers/remoteproc/remoteproc_sysfs.c | 57 ++++++++++++++++++ 3 files changed, 93 insertions(+), 77 deletions(-)