Re: [PATCH 2/2] remoteproc: Move recovery debugfs entry to sysfs

[Mathieu Poirier]

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
>