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,
>>
