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@xxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxx>
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
>
