Re: [PATCH v6 2/3] remoteproc: Move recovery configuration to sysfs

[Arnaud POULIQUEN]

On 9/30/20 10:11 AM, Arnaud POULIQUEN wrote:
> 
> 
> On 9/29/20 12:17 AM, Rishabh Bhatnagar wrote:
>> Move recovery configuration from debugfs to sysfs.This will
>> allow usage of this configuration feature in production
>> devices where access to debugfs might be limited.
>>
>> Signed-off-by: Rishabh Bhatnagar <rishabhb@xxxxxxxxxxxxxx>
>> ---
>>  Documentation/ABI/testing/sysfs-class-remoteproc | 20 ++++++
>>  drivers/remoteproc/remoteproc_debugfs.c          | 78 ------------------------
>>  drivers/remoteproc/remoteproc_sysfs.c            | 56 +++++++++++++++++
>>  3 files changed, 76 insertions(+), 78 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-remoteproc b/Documentation/ABI/testing/sysfs-class-remoteproc
>> index f6c44fa..7368b50 100644
>> --- a/Documentation/ABI/testing/sysfs-class-remoteproc
>> +++ b/Documentation/ABI/testing/sysfs-class-remoteproc
>> @@ -82,3 +82,23 @@ Description:	Remote processor coredump configuration
>>  		all data is read by usersapce.
>>  
>>  		"disabled" means no dump will be collected.
>> +
>> +What:		/sys/class/remoteproc/.../recovery
>> +Date:		July 2020
>> +Contact:	Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>, Ohad Ben-Cohen <ohad@xxxxxxxxxx>
>> +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_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
>> index 732770e..c505f0e 100644
>> --- a/drivers/remoteproc/remoteproc_debugfs.c
>> +++ b/drivers/remoteproc/remoteproc_debugfs.c
>> @@ -84,82 +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
>>  rproc_crash_write(struct file *filp, const char __user *user_buf,
>> @@ -329,8 +253,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 2a44571..e60a014 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
> 
> Recovery seems to be related to debugging, is it wise to expose it in sysfs?
> This would allow the user space to restart remote processor and generate core
> dumps at any time...Is this the expected behavior?

This comment is wrong, please forget it. I mixed with some code from last Kernel LTS.
The state is checked in rproc_trigger_recovery, so no issue.

My apologize for this stupid comment. 

Regards
Arnaud

> 
> For your information, in the ST test suite, we use this mechanism also to simulate a
> proc crash. This is useful for testing the recovery of RPMsg channels...
> 
> Regards
> Arnaud
> 
>> + *		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
>> @@ -202,6 +257,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,
>>