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 <[email protected]> > --- > 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 <[email protected]> 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 >
