[PATCH v2 0/3] Expose recovery/coredump configuration from sysfs

[PATCH v2 0/3] Expose recovery/coredump configuration from sysfs

[Rishabh Bhatnagar]

From Android R onwards Google has restricted access to debugfs in user
and user-debug builds. This restricts access to most of the features
exposed through debugfs. This patch series adds a configurable option
to move the recovery/coredump interfaces to sysfs. If the feature
flag is selected it would move these interfaces to sysfs and remove
the equivalent debugfs interface. 'Coredump' and 'Recovery' are critical
interfaces that are required for remoteproc to work on Qualcomm Chipsets.
Coredump configuration needs to be set to "inline" in debug/test build
and "disabled" in production builds. Whereas recovery needs to be
"disabled" for debugging purposes and "enabled" on production builds.

Changelog:

v1 -> v2:
- Correct the contact name in the sysfs documentation.
- Remove the redundant write documentation for coredump/recovery sysfs
- Add a feature flag to make this interface switch configurable.

Rishabh Bhatnagar (3):
  remoteproc: Expose remoteproc configuration through sysfs
  remoteproc: Add coredump configuration to sysfs
  remoteproc: Add recovery configuration to sysfs

 Documentation/ABI/testing/sysfs-class-remoteproc |  44 ++++++++
 drivers/remoteproc/Kconfig                       |  12 +++
 drivers/remoteproc/remoteproc_debugfs.c          |  10 +-
 drivers/remoteproc/remoteproc_sysfs.c            | 126 +++++++++++++++++++++++
 4 files changed, 190 insertions(+), 2 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 1/3] remoteproc: Expose remoteproc configuration through sysfs

[Rishabh Bhatnagar]

Add a feature flag to expose some of the remoteproc configuration
through sysfs. This feature is helpful in systems where debugfs is
not available/mounted. Currently the recovery and coredump
configuration is exposed through sysfs rather than debugfs when
this feature is selected.

Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
---
 drivers/remoteproc/Kconfig | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index c6659dfe..8aecf70 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -275,6 +275,18 @@ config TI_K3_DSP_REMOTEPROC
 	  It's safe to say N here if you're not interested in utilizing
 	  the DSP slave processors.
 
+config RPROC_SYSFS_CONFIGURATION_SUPPORT
+	bool "Expose remoteproc configuration sysfs entries"
+	default n
+	help
+	  Say y here to expose recovery and coredump configuration sysfs
+	  entries. This will remove the corresponding entries from debugfs
+	  and expose it through sysfs. This is helpful in operating systems
+	  where debugfs is not available.
+
+	  It's safe to say N here if you are not interested in accessing
+	  recovery and coredump configuration through sysfs.
+
 endif # REMOTEPROC
 
 endmenu
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 2/3] remoteproc: Add coredump configuration to sysfs

[Rishabh Bhatnagar]

Expose coredump configuration in sysfs under a feature
flag. This is useful for systems where access to
debugfs might be limited.

Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
---
 Documentation/ABI/testing/sysfs-class-remoteproc | 24 +++++++++
 drivers/remoteproc/remoteproc_debugfs.c          |  4 ++
 drivers/remoteproc/remoteproc_sysfs.c            | 68 ++++++++++++++++++++++++
 3 files changed, 96 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-class-remoteproc b/Documentation/ABI/testing/sysfs-class-remoteproc
index 36094fb..f6c44fa 100644
--- a/Documentation/ABI/testing/sysfs-class-remoteproc
+++ b/Documentation/ABI/testing/sysfs-class-remoteproc
@@ -58,3 +58,27 @@ Description:	Remote processor name
 		Reports the name of the remote processor. This can be used by
 		userspace in exactly identifying a remote processor and ease
 		up the usage in modifying the 'firmware' or 'state' files.
+
+What:		/sys/class/remoteproc/.../coredump
+Date:		July 2020
+Contact:	Bjorn Andersson <bjorn.andersson@linaro.org>, Ohad Ben-Cohen <ohad@wizery.com>
+Description:	Remote processor coredump configuration
+
+		Reports the coredump configuration of the remote processor,
+		which will be one of:
+
+		"default"
+		"inline"
+		"disabled"
+
+		"default" means when the remote processor's coredump is
+		collected it will be copied to a separate buffer and that
+		buffer is exposed to userspace.
+
+		"inline" means when the remote processor's coredump is
+		collected userspace will directly read from the remote
+		processor's device memory. Extra buffer will not be used to
+		copy the dump. Also recovery process will not proceed until
+		all data is read by usersapce.
+
+		"disabled" means no dump will be collected.
diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
index 2e3b3e2..48dfd0a 100644
--- a/drivers/remoteproc/remoteproc_debugfs.c
+++ b/drivers/remoteproc/remoteproc_debugfs.c
@@ -27,6 +27,7 @@
 /* remoteproc debugfs parent dir */
 static struct dentry *rproc_dbg;
 
+#if (!IS_ENABLED(CONFIG_RPROC_SYSFS_CONFIGURATION_SUPPORT))
 /*
  * A coredump-configuration-to-string lookup table, for exposing a
  * human readable configuration via debugfs. Always keep in sync with
@@ -114,6 +115,7 @@ static const struct file_operations rproc_coredump_fops = {
 	.open = simple_open,
 	.llseek = generic_file_llseek,
 };
+#endif
 
 /*
  * Some remote processors may support dumping trace logs into a shared
@@ -425,8 +427,10 @@ void rproc_create_debug_dir(struct rproc *rproc)
 			    rproc, &rproc_rsc_table_fops);
 	debugfs_create_file("carveout_memories", 0400, rproc->dbg_dir,
 			    rproc, &rproc_carveouts_fops);
+#if (!IS_ENABLED(CONFIG_RPROC_SYSFS_CONFIGURATION_SUPPORT))
 	debugfs_create_file("coredump", 0600, rproc->dbg_dir,
 			    rproc, &rproc_coredump_fops);
+#endif
 }
 
 void __init rproc_init_debugfs(void)
diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
index eea514c..89b301a 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -10,6 +10,71 @@
 
 #define to_rproc(d) container_of(d, struct rproc, dev)
 
+#if IS_ENABLED(CONFIG_RPROC_SYSFS_CONFIGURATION_SUPPORT)
+/*
+ * A coredump-configuration-to-string lookup table, for exposing a
+ * human readable configuration via sysfs. Always keep in sync with
+ * enum rproc_coredump_mechanism
+ */
+static const char * const rproc_coredump_str[] = {
+	[RPROC_COREDUMP_DEFAULT]	= "default",
+	[RPROC_COREDUMP_INLINE]		= "inline",
+	[RPROC_COREDUMP_DISABLED]	= "disabled",
+};
+
+/* Expose the current coredump configuration via debugfs */
+static ssize_t coredump_show(struct device *dev,
+			     struct device_attribute *attr, char *buf)
+{
+	struct rproc *rproc = to_rproc(dev);
+
+	return sprintf(buf, "%s\n", rproc_coredump_str[rproc->dump_conf]);
+}
+
+/*
+ * By writing to the 'coredump' sysfs entry, we control the behavior of the
+ * coredump mechanism dynamically. The default value of this entry is "default".
+ *
+ * The 'coredump' sysfs entry supports these commands:
+ *
+ * default:	This is the default coredump mechanism. When the remoteproc
+ *		crashes the entire coredump will be copied to a separate buffer
+ *		and exposed to userspace.
+ *
+ * inline:	The coredump will not be copied to a separate buffer and the
+ *		recovery process will have to wait until data is read by
+ *		userspace. But this avoid usage of extra memory.
+ *
+ * disabled:	This will disable coredump. Recovery will proceed without
+ *		collecting any dump.
+ */
+static ssize_t coredump_store(struct device *dev,
+			      struct device_attribute *attr,
+			      const char *buf, size_t count)
+{
+	struct rproc *rproc = to_rproc(dev);
+
+	if (rproc->state == RPROC_CRASHED) {
+		dev_err(&rproc->dev, "can't change coredump configuration\n");
+		return -EBUSY;
+	}
+
+	if (sysfs_streq(buf, "disable")) {
+		rproc->dump_conf = RPROC_COREDUMP_DISABLED;
+	} else if (sysfs_streq(buf, "inline")) {
+		rproc->dump_conf = RPROC_COREDUMP_INLINE;
+	} else if (sysfs_streq(buf, "default")) {
+		rproc->dump_conf = RPROC_COREDUMP_DEFAULT;
+	} else {
+		dev_err(&rproc->dev, "Invalid coredump configuration\n");
+		return -EINVAL;
+	}
+
+	return count;
+}
+static DEVICE_ATTR_RW(coredump);
+#endif
+
 /* Expose the loaded / running firmware name via sysfs */
 static ssize_t firmware_show(struct device *dev, struct device_attribute *attr,
 			  char *buf)
@@ -138,6 +203,9 @@ static ssize_t name_show(struct device *dev, struct device_attribute *attr,
 static DEVICE_ATTR_RO(name);
 
 static struct attribute *rproc_attrs[] = {
+#if IS_ENABLED(CONFIG_RPROC_SYSFS_CONFIGURATION_SUPPORT)
+	&dev_attr_coredump.attr,
+#endif
 	&dev_attr_firmware.attr,
 	&dev_attr_state.attr,
 	&dev_attr_name.attr,
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 2/3] remoteproc: Add coredump configuration to sysfs

[Greg KH]

On Thu, Aug 27, 2020 at 12:48:50PM -0700, Rishabh Bhatnagar wrote:
> Expose coredump configuration in sysfs under a feature
> flag. This is useful for systems where access to
> debugfs might be limited.
> 
> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
> ---
>  Documentation/ABI/testing/sysfs-class-remoteproc | 24 +++++++++
>  drivers/remoteproc/remoteproc_debugfs.c          |  4 ++
>  drivers/remoteproc/remoteproc_sysfs.c            | 68 ++++++++++++++++++++++++
>  3 files changed, 96 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-remoteproc b/Documentation/ABI/testing/sysfs-class-remoteproc
> index 36094fb..f6c44fa 100644
> --- a/Documentation/ABI/testing/sysfs-class-remoteproc
> +++ b/Documentation/ABI/testing/sysfs-class-remoteproc
> @@ -58,3 +58,27 @@ Description:	Remote processor name
>  		Reports the name of the remote processor. This can be used by
>  		userspace in exactly identifying a remote processor and ease
>  		up the usage in modifying the 'firmware' or 'state' files.
> +
> +What:		/sys/class/remoteproc/.../coredump
> +Date:		July 2020
> +Contact:	Bjorn Andersson <bjorn.andersson@linaro.org>, Ohad Ben-Cohen <ohad@wizery.com>
> +Description:	Remote processor coredump configuration
> +
> +		Reports the coredump configuration of the remote processor,
> +		which will be one of:
> +
> +		"default"
> +		"inline"
> +		"disabled"
> +
> +		"default" means when the remote processor's coredump is
> +		collected it will be copied to a separate buffer and that
> +		buffer is exposed to userspace.
> +
> +		"inline" means when the remote processor's coredump is
> +		collected userspace will directly read from the remote
> +		processor's device memory. Extra buffer will not be used to
> +		copy the dump. Also recovery process will not proceed until
> +		all data is read by usersapce.
> +
> +		"disabled" means no dump will be collected.
> diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
> index 2e3b3e2..48dfd0a 100644
> --- a/drivers/remoteproc/remoteproc_debugfs.c
> +++ b/drivers/remoteproc/remoteproc_debugfs.c
> @@ -27,6 +27,7 @@
>  /* remoteproc debugfs parent dir */
>  static struct dentry *rproc_dbg;
>  
> +#if (!IS_ENABLED(CONFIG_RPROC_SYSFS_CONFIGURATION_SUPPORT))
>  /*
>   * A coredump-configuration-to-string lookup table, for exposing a
>   * human readable configuration via debugfs. Always keep in sync with
> @@ -114,6 +115,7 @@ static const struct file_operations rproc_coredump_fops = {
>  	.open = simple_open,
>  	.llseek = generic_file_llseek,
>  };
> +#endif
>  
>  /*
>   * Some remote processors may support dumping trace logs into a shared
> @@ -425,8 +427,10 @@ void rproc_create_debug_dir(struct rproc *rproc)
>  			    rproc, &rproc_rsc_table_fops);
>  	debugfs_create_file("carveout_memories", 0400, rproc->dbg_dir,
>  			    rproc, &rproc_carveouts_fops);
> +#if (!IS_ENABLED(CONFIG_RPROC_SYSFS_CONFIGURATION_SUPPORT))
>  	debugfs_create_file("coredump", 0600, rproc->dbg_dir,
>  			    rproc, &rproc_coredump_fops);
> +#endif

Why does sysfs support for this have anything to do if you have a
debugfs file present or not?  They should both work at the same time if
needed, right?

Same for patch 3/3 in this series...

thanks,

greg k-h

[PATCH v2 3/3] remoteproc: Add recovery configuration to sysfs

[Rishabh Bhatnagar]

Expose recovery configuration in sysfs under a feature
flag. This is useful for systems where access to
debugfs might be limited.

Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
---
 Documentation/ABI/testing/sysfs-class-remoteproc | 20 ++++++++
 drivers/remoteproc/remoteproc_debugfs.c          |  6 ++-
 drivers/remoteproc/remoteproc_sysfs.c            | 58 ++++++++++++++++++++++++
 3 files changed, 82 insertions(+), 2 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@linaro.org>, Ohad Ben-Cohen <ohad@wizery.com>
+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 48dfd0a..415a9ff 100644
--- a/drivers/remoteproc/remoteproc_debugfs.c
+++ b/drivers/remoteproc/remoteproc_debugfs.c
@@ -174,6 +174,7 @@ static const struct file_operations rproc_name_ops = {
 	.llseek	= generic_file_llseek,
 };
 
+#if (!IS_ENABLED(CONFIG_RPROC_SYSFS_CONFIGURATION_SUPPORT))
 /* expose recovery flag via debugfs */
 static ssize_t rproc_recovery_read(struct file *filp, char __user *userbuf,
 				   size_t count, loff_t *ppos)
@@ -249,6 +250,7 @@ static const struct file_operations rproc_recovery_ops = {
 	.open = simple_open,
 	.llseek = generic_file_llseek,
 };
+#endif
 
 /* expose the crash trigger via debugfs */
 static ssize_t
@@ -419,8 +421,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,
@@ -430,6 +430,8 @@ void rproc_create_debug_dir(struct rproc *rproc)
 #if (!IS_ENABLED(CONFIG_RPROC_SYSFS_CONFIGURATION_SUPPORT))
 	debugfs_create_file("coredump", 0600, rproc->dbg_dir,
 			    rproc, &rproc_coredump_fops);
+	debugfs_create_file("recovery", 0600, rproc->dbg_dir,
+			    rproc, &rproc_recovery_ops);
 #endif
 }
 
diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
index 89b301a..45cae4f 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -11,6 +11,63 @@
 #define to_rproc(d) container_of(d, struct rproc, dev)
 
 #if IS_ENABLED(CONFIG_RPROC_SYSFS_CONFIGURATION_SUPPORT)
+
+/* 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
@@ -205,6 +262,7 @@ static DEVICE_ATTR_RO(name);
 static struct attribute *rproc_attrs[] = {
 #if IS_ENABLED(CONFIG_RPROC_SYSFS_CONFIGURATION_SUPPORT)
 	&dev_attr_coredump.attr,
+	&dev_attr_recovery.attr,
 #endif
 	&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

Re: [PATCH v2 0/3] Expose recovery/coredump configuration from sysfs

[Mathieu Poirier]

Hi Rishabh,

On Thu, Aug 27, 2020 at 12:48:48PM -0700, Rishabh Bhatnagar wrote:
> From Android R onwards Google has restricted access to debugfs in user
> and user-debug builds. This restricts access to most of the features
> exposed through debugfs. This patch series adds a configurable option
> to move the recovery/coredump interfaces to sysfs. If the feature
> flag is selected it would move these interfaces to sysfs and remove
> the equivalent debugfs interface.

What I meant wast to move the coredump entry from debugfs to sysfs and from
there make it available to user space using a kernel config.  But thinking
further on this it may be better to simply provide an API to set the coredump
mode from the platform driver, the same way rproc_coredump_set_elf_info() works.
That will prevent breaking a fair amount of user space code...

Let me know if that can work for you.

Thanks,
Mathieu

> 'Coredump' and 'Recovery' are critical
> interfaces that are required for remoteproc to work on Qualcomm Chipsets.
> Coredump configuration needs to be set to "inline" in debug/test build
> and "disabled" in production builds. Whereas recovery needs to be
> "disabled" for debugging purposes and "enabled" on production builds.
> 
> Changelog:
> 
> v1 -> v2:
> - Correct the contact name in the sysfs documentation.
> - Remove the redundant write documentation for coredump/recovery sysfs
> - Add a feature flag to make this interface switch configurable.
> 
> Rishabh Bhatnagar (3):
>   remoteproc: Expose remoteproc configuration through sysfs
>   remoteproc: Add coredump configuration to sysfs
>   remoteproc: Add recovery configuration to sysfs
> 
>  Documentation/ABI/testing/sysfs-class-remoteproc |  44 ++++++++
>  drivers/remoteproc/Kconfig                       |  12 +++
>  drivers/remoteproc/remoteproc_debugfs.c          |  10 +-
>  drivers/remoteproc/remoteproc_sysfs.c            | 126 +++++++++++++++++++++++
>  4 files changed, 190 insertions(+), 2 deletions(-)
> 
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH v2 0/3] Expose recovery/coredump configuration from sysfs

[rishabhb]

On 2020-09-01 15:05, Mathieu Poirier wrote:
> Hi Rishabh,
> 
> On Thu, Aug 27, 2020 at 12:48:48PM -0700, Rishabh Bhatnagar wrote:
>> From Android R onwards Google has restricted access to debugfs in user
>> and user-debug builds. This restricts access to most of the features
>> exposed through debugfs. This patch series adds a configurable option
>> to move the recovery/coredump interfaces to sysfs. If the feature
>> flag is selected it would move these interfaces to sysfs and remove
>> the equivalent debugfs interface.
> 
> What I meant wast to move the coredump entry from debugfs to sysfs and 
> from
> there make it available to user space using a kernel config.  But 
> thinking
> further on this it may be better to simply provide an API to set the 
> coredump
> mode from the platform driver, the same way 
> rproc_coredump_set_elf_info() works.
> That will prevent breaking a fair amount of user space code...
> 
> Let me know if that can work for you.
> 
> Thanks,
> Mathieu
> 
Hi Mathieu,
That works for product configuration but that would still limit internal
testing. Since there is also restriction on accessing debugfs through
userspace code, automation won't be able to run recovery/coredump tests.
Only other way for us would be to provide these sysfs entries through 
the
platform drivers locally but that would create a lot of mess/redundancy.

>> 'Coredump' and 'Recovery' are critical
>> interfaces that are required for remoteproc to work on Qualcomm 
>> Chipsets.
>> Coredump configuration needs to be set to "inline" in debug/test build
>> and "disabled" in production builds. Whereas recovery needs to be
>> "disabled" for debugging purposes and "enabled" on production builds.
>> 
>> Changelog:
>> 
>> v1 -> v2:
>> - Correct the contact name in the sysfs documentation.
>> - Remove the redundant write documentation for coredump/recovery sysfs
>> - Add a feature flag to make this interface switch configurable.
>> 
>> Rishabh Bhatnagar (3):
>>   remoteproc: Expose remoteproc configuration through sysfs
>>   remoteproc: Add coredump configuration to sysfs
>>   remoteproc: Add recovery configuration to sysfs
>> 
>>  Documentation/ABI/testing/sysfs-class-remoteproc |  44 ++++++++
>>  drivers/remoteproc/Kconfig                       |  12 +++
>>  drivers/remoteproc/remoteproc_debugfs.c          |  10 +-
>>  drivers/remoteproc/remoteproc_sysfs.c            | 126 
>> +++++++++++++++++++++++
>>  4 files changed, 190 insertions(+), 2 deletions(-)
>> 
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>> Forum,
>> a Linux Foundation Collaborative Project
>> 

Re: [PATCH v2 0/3] Expose recovery/coredump configuration from sysfs

[Mathieu Poirier]

On Wed, Sep 02, 2020 at 04:14:19PM -0700, rishabhb@codeaurora.org wrote:
> On 2020-09-01 15:05, Mathieu Poirier wrote:
> > Hi Rishabh,
> > 
> > On Thu, Aug 27, 2020 at 12:48:48PM -0700, Rishabh Bhatnagar wrote:
> > > From Android R onwards Google has restricted access to debugfs in user
> > > and user-debug builds. This restricts access to most of the features
> > > exposed through debugfs. This patch series adds a configurable option
> > > to move the recovery/coredump interfaces to sysfs. If the feature
> > > flag is selected it would move these interfaces to sysfs and remove
> > > the equivalent debugfs interface.
> > 
> > What I meant wast to move the coredump entry from debugfs to sysfs and
> > from
> > there make it available to user space using a kernel config.  But
> > thinking
> > further on this it may be better to simply provide an API to set the
> > coredump
> > mode from the platform driver, the same way
> > rproc_coredump_set_elf_info() works.
> > That will prevent breaking a fair amount of user space code...
> > 
> > Let me know if that can work for you.
> > 
> > Thanks,
> > Mathieu
> > 
> Hi Mathieu,
> That works for product configuration but that would still limit internal
> testing. Since there is also restriction on accessing debugfs through
> userspace code, automation won't be able to run recovery/coredump tests.

Ok, please spinoff a new version that follows the guidelines above and we can
restart the conversation from there. 

> Only other way for us would be to provide these sysfs entries through the
> platform drivers locally but that would create a lot of mess/redundancy.
> 

Right, this is definitely not the right way to proceed.

> > > 'Coredump' and 'Recovery' are critical
> > > interfaces that are required for remoteproc to work on Qualcomm
> > > Chipsets.
> > > Coredump configuration needs to be set to "inline" in debug/test build
> > > and "disabled" in production builds. Whereas recovery needs to be
> > > "disabled" for debugging purposes and "enabled" on production builds.
> > > 
> > > Changelog:
> > > 
> > > v1 -> v2:
> > > - Correct the contact name in the sysfs documentation.
> > > - Remove the redundant write documentation for coredump/recovery sysfs
> > > - Add a feature flag to make this interface switch configurable.
> > > 
> > > Rishabh Bhatnagar (3):
> > >   remoteproc: Expose remoteproc configuration through sysfs
> > >   remoteproc: Add coredump configuration to sysfs
> > >   remoteproc: Add recovery configuration to sysfs
> > > 
> > >  Documentation/ABI/testing/sysfs-class-remoteproc |  44 ++++++++
> > >  drivers/remoteproc/Kconfig                       |  12 +++
> > >  drivers/remoteproc/remoteproc_debugfs.c          |  10 +-
> > >  drivers/remoteproc/remoteproc_sysfs.c            | 126
> > > +++++++++++++++++++++++
> > >  4 files changed, 190 insertions(+), 2 deletions(-)
> > > 
> > > --
> > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> > > Forum,
> > > a Linux Foundation Collaborative Project
> > > 

Re: [PATCH v2 0/3] Expose recovery/coredump configuration from sysfs

[Bjorn Andersson]

On Tue 01 Sep 17:05 CDT 2020, Mathieu Poirier wrote:

> Hi Rishabh,
> 
> On Thu, Aug 27, 2020 at 12:48:48PM -0700, Rishabh Bhatnagar wrote:
> > From Android R onwards Google has restricted access to debugfs in user
> > and user-debug builds. This restricts access to most of the features
> > exposed through debugfs. This patch series adds a configurable option
> > to move the recovery/coredump interfaces to sysfs. If the feature
> > flag is selected it would move these interfaces to sysfs and remove
> > the equivalent debugfs interface.
> 
> What I meant wast to move the coredump entry from debugfs to sysfs and from
> there make it available to user space using a kernel config.

Why would we not always make this available in sysfs?

> But thinking further on this it may be better to simply provide an API
> to set the coredump mode from the platform driver, the same way
> rproc_coredump_set_elf_info() works.

Being able to invoke these from the platform drivers sounds like a new
feature. What would trigger the platform drivers to call this? Or are
you perhaps asking for the means of the drivers to be able to select the
default mode?

Regarding the default mode, I think it would make sense to make the
default "disabled", because this is the most sensible configuration in a
"production" environment. And the sysfs means we have a convenient
mechanism to configure it, even on production environments.

> That will prevent breaking a fair amount of user space code...
> 

We typically don't guarantee that the debugfs interfaces are stable and
if I understand the beginning of you reply you still want to move it
from debugfs to sysfs - which I presume would break such scripts in the
first place?


I would prefer to see that we don't introduce config options for every
little thing, unless there's good reason for it.

Regards,
Bjorn

> Let me know if that can work for you.
> 
> Thanks,
> Mathieu
> 
> > 'Coredump' and 'Recovery' are critical
> > interfaces that are required for remoteproc to work on Qualcomm Chipsets.
> > Coredump configuration needs to be set to "inline" in debug/test build
> > and "disabled" in production builds. Whereas recovery needs to be
> > "disabled" for debugging purposes and "enabled" on production builds.
> > 
> > Changelog:
> > 
> > v1 -> v2:
> > - Correct the contact name in the sysfs documentation.
> > - Remove the redundant write documentation for coredump/recovery sysfs
> > - Add a feature flag to make this interface switch configurable.
> > 
> > Rishabh Bhatnagar (3):
> >   remoteproc: Expose remoteproc configuration through sysfs
> >   remoteproc: Add coredump configuration to sysfs
> >   remoteproc: Add recovery configuration to sysfs
> > 
> >  Documentation/ABI/testing/sysfs-class-remoteproc |  44 ++++++++
> >  drivers/remoteproc/Kconfig                       |  12 +++
> >  drivers/remoteproc/remoteproc_debugfs.c          |  10 +-
> >  drivers/remoteproc/remoteproc_sysfs.c            | 126 +++++++++++++++++++++++
> >  4 files changed, 190 insertions(+), 2 deletions(-)
> > 
> > -- 
> > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> > a Linux Foundation Collaborative Project
> > 

Re: [PATCH v2 0/3] Expose recovery/coredump configuration from sysfs

[Mathieu Poirier]

On Thu, Sep 03, 2020 at 06:59:44PM -0500, Bjorn Andersson wrote:
> On Tue 01 Sep 17:05 CDT 2020, Mathieu Poirier wrote:
> 
> > Hi Rishabh,
> > 
> > On Thu, Aug 27, 2020 at 12:48:48PM -0700, Rishabh Bhatnagar wrote:
> > > From Android R onwards Google has restricted access to debugfs in user
> > > and user-debug builds. This restricts access to most of the features
> > > exposed through debugfs. This patch series adds a configurable option
> > > to move the recovery/coredump interfaces to sysfs. If the feature
> > > flag is selected it would move these interfaces to sysfs and remove
> > > the equivalent debugfs interface.
> > 
> > What I meant wast to move the coredump entry from debugfs to sysfs and from
> > there make it available to user space using a kernel config.
> 
> Why would we not always make this available in sysfs?

At this time the options are in debugfs and vendors can decide to make that
available on products if they want to.  The idea behind using a kernel
configuration once moved to sysfs was to give the same kind of options.

> 
> > But thinking further on this it may be better to simply provide an API
> > to set the coredump mode from the platform driver, the same way
> > rproc_coredump_set_elf_info() works.
> 
> Being able to invoke these from the platform drivers sounds like a new
> feature. What would trigger the platform drivers to call this? Or are
> you perhaps asking for the means of the drivers to be able to select the
> default mode?

My ultimate goal is to avoid needlessly stuffing things in sysfs.  My hope in
suggesting a new API was that platform drivers could recognise the kind of
build/environment they operate in and setup the coredump mode accordingly.  That
would have allowed us to leave debugfs options alone.

> 
> Regarding the default mode, I think it would make sense to make the
> default "disabled", because this is the most sensible configuration in a
> "production" environment. And the sysfs means we have a convenient
> mechanism to configure it, even on production environments.
>

I am weary of changing something that hasn't been requested.  
 
> > That will prevent breaking a fair amount of user space code...
> > 
> 
> We typically don't guarantee that the debugfs interfaces are stable and
> if I understand the beginning of you reply you still want to move it
> from debugfs to sysfs - which I presume would break such scripts in the
> first place?

Correct - I am sure that moving coredump and recovery options to sysfs will
break user space scripts.  Even if debugfs is not part of the ABI it would be
nice to avoid disrupting people as much as possible.

> 
> 
> I would prefer to see that we don't introduce config options for every
> little thing, unless there's good reason for it.

I totally agree.  It is with great reluctance that I asked Rishab to proceed
the way he did in V3.  His usecase makes sense... On the flip side this is
pushed down on the kernel community and I really like Christoph's position about
fixing Android and leaving the kernel alone.

> 
> Regards,
> Bjorn
> 
> > Let me know if that can work for you.
> > 
> > Thanks,
> > Mathieu
> > 
> > > 'Coredump' and 'Recovery' are critical
> > > interfaces that are required for remoteproc to work on Qualcomm Chipsets.
> > > Coredump configuration needs to be set to "inline" in debug/test build
> > > and "disabled" in production builds. Whereas recovery needs to be
> > > "disabled" for debugging purposes and "enabled" on production builds.
> > > 
> > > Changelog:
> > > 
> > > v1 -> v2:
> > > - Correct the contact name in the sysfs documentation.
> > > - Remove the redundant write documentation for coredump/recovery sysfs
> > > - Add a feature flag to make this interface switch configurable.
> > > 
> > > Rishabh Bhatnagar (3):
> > >   remoteproc: Expose remoteproc configuration through sysfs
> > >   remoteproc: Add coredump configuration to sysfs
> > >   remoteproc: Add recovery configuration to sysfs
> > > 
> > >  Documentation/ABI/testing/sysfs-class-remoteproc |  44 ++++++++
> > >  drivers/remoteproc/Kconfig                       |  12 +++
> > >  drivers/remoteproc/remoteproc_debugfs.c          |  10 +-
> > >  drivers/remoteproc/remoteproc_sysfs.c            | 126 +++++++++++++++++++++++
> > >  4 files changed, 190 insertions(+), 2 deletions(-)
> > > 
> > > -- 
> > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> > > a Linux Foundation Collaborative Project
> > > 

Re: [PATCH v2 0/3] Expose recovery/coredump configuration from sysfs

[rishabhb]

On 2020-09-04 15:02, Mathieu Poirier wrote:
> On Thu, Sep 03, 2020 at 06:59:44PM -0500, Bjorn Andersson wrote:
>> On Tue 01 Sep 17:05 CDT 2020, Mathieu Poirier wrote:
>> 
>> > Hi Rishabh,
>> >
>> > On Thu, Aug 27, 2020 at 12:48:48PM -0700, Rishabh Bhatnagar wrote:
>> > > From Android R onwards Google has restricted access to debugfs in user
>> > > and user-debug builds. This restricts access to most of the features
>> > > exposed through debugfs. This patch series adds a configurable option
>> > > to move the recovery/coredump interfaces to sysfs. If the feature
>> > > flag is selected it would move these interfaces to sysfs and remove
>> > > the equivalent debugfs interface.
>> >
>> > What I meant wast to move the coredump entry from debugfs to sysfs and from
>> > there make it available to user space using a kernel config.
>> 
>> Why would we not always make this available in sysfs?
> 
> At this time the options are in debugfs and vendors can decide to make 
> that
> available on products if they want to.  The idea behind using a kernel
> configuration once moved to sysfs was to give the same kind of options.
> 
>> 
>> > But thinking further on this it may be better to simply provide an API
>> > to set the coredump mode from the platform driver, the same way
>> > rproc_coredump_set_elf_info() works.
>> 
>> Being able to invoke these from the platform drivers sounds like a new
>> feature. What would trigger the platform drivers to call this? Or are
>> you perhaps asking for the means of the drivers to be able to select 
>> the
>> default mode?
> 
> My ultimate goal is to avoid needlessly stuffing things in sysfs.  My 
> hope in
> suggesting a new API was that platform drivers could recognise the kind 
> of
> build/environment they operate in and setup the coredump mode 
> accordingly.  That
> would have allowed us to leave debugfs options alone.
> 
>> 
>> Regarding the default mode, I think it would make sense to make the
>> default "disabled", because this is the most sensible configuration in 
>> a
>> "production" environment. And the sysfs means we have a convenient
>> mechanism to configure it, even on production environments.
>> 
> 
> I am weary of changing something that hasn't been requested.
> 
>> > That will prevent breaking a fair amount of user space code...
>> >
>> 
>> We typically don't guarantee that the debugfs interfaces are stable 
>> and
>> if I understand the beginning of you reply you still want to move it
>> from debugfs to sysfs - which I presume would break such scripts in 
>> the
>> first place?
> 
> Correct - I am sure that moving coredump and recovery options to sysfs 
> will
> break user space scripts.  Even if debugfs is not part of the ABI it 
> would be
> nice to avoid disrupting people as much as possible.
> 
>> 
>> 
>> I would prefer to see that we don't introduce config options for every
>> little thing, unless there's good reason for it.
> 
> I totally agree.  It is with great reluctance that I asked Rishab to 
> proceed
> the way he did in V3.  His usecase makes sense... On the flip side this 
> is
> pushed down on the kernel community and I really like Christoph's 
> position about
> fixing Android and leaving the kernel alone.
> 
Well, removing debugfs is conscious decision taken by android due to 
security
concerns and there is not we can fix there.
Would it be a terrible idea to have recovery and coredump exposed from 
both
sysfs and debugfs instead of choosing one and breaking userspace code?
>> 
>> Regards,
>> Bjorn
>> 
>> > Let me know if that can work for you.
>> >
>> > Thanks,
>> > Mathieu
>> >
>> > > 'Coredump' and 'Recovery' are critical
>> > > interfaces that are required for remoteproc to work on Qualcomm Chipsets.
>> > > Coredump configuration needs to be set to "inline" in debug/test build
>> > > and "disabled" in production builds. Whereas recovery needs to be
>> > > "disabled" for debugging purposes and "enabled" on production builds.
>> > >
>> > > Changelog:
>> > >
>> > > v1 -> v2:
>> > > - Correct the contact name in the sysfs documentation.
>> > > - Remove the redundant write documentation for coredump/recovery sysfs
>> > > - Add a feature flag to make this interface switch configurable.
>> > >
>> > > Rishabh Bhatnagar (3):
>> > >   remoteproc: Expose remoteproc configuration through sysfs
>> > >   remoteproc: Add coredump configuration to sysfs
>> > >   remoteproc: Add recovery configuration to sysfs
>> > >
>> > >  Documentation/ABI/testing/sysfs-class-remoteproc |  44 ++++++++
>> > >  drivers/remoteproc/Kconfig                       |  12 +++
>> > >  drivers/remoteproc/remoteproc_debugfs.c          |  10 +-
>> > >  drivers/remoteproc/remoteproc_sysfs.c            | 126 +++++++++++++++++++++++
>> > >  4 files changed, 190 insertions(+), 2 deletions(-)
>> > >
>> > > --
>> > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> > > a Linux Foundation Collaborative Project
>> > >

Re: [PATCH v2 0/3] Expose recovery/coredump configuration from sysfs

[rishabhb]

+android kernel team

On 2020-09-09 10:27, rishabhb@codeaurora.org wrote:
> On 2020-09-04 15:02, Mathieu Poirier wrote:
>> On Thu, Sep 03, 2020 at 06:59:44PM -0500, Bjorn Andersson wrote:
>>> On Tue 01 Sep 17:05 CDT 2020, Mathieu Poirier wrote:
>>> 
>>> > Hi Rishabh,
>>> >
>>> > On Thu, Aug 27, 2020 at 12:48:48PM -0700, Rishabh Bhatnagar wrote:
>>> > > From Android R onwards Google has restricted access to debugfs in user
>>> > > and user-debug builds. This restricts access to most of the features
>>> > > exposed through debugfs. This patch series adds a configurable option
>>> > > to move the recovery/coredump interfaces to sysfs. If the feature
>>> > > flag is selected it would move these interfaces to sysfs and remove
>>> > > the equivalent debugfs interface.
>>> >
>>> > What I meant wast to move the coredump entry from debugfs to sysfs and from
>>> > there make it available to user space using a kernel config.
>>> 
>>> Why would we not always make this available in sysfs?
>> 
>> At this time the options are in debugfs and vendors can decide to make 
>> that
>> available on products if they want to.  The idea behind using a kernel
>> configuration once moved to sysfs was to give the same kind of 
>> options.
>> 
>>> 
>>> > But thinking further on this it may be better to simply provide an API
>>> > to set the coredump mode from the platform driver, the same way
>>> > rproc_coredump_set_elf_info() works.
>>> 
>>> Being able to invoke these from the platform drivers sounds like a 
>>> new
>>> feature. What would trigger the platform drivers to call this? Or are
>>> you perhaps asking for the means of the drivers to be able to select 
>>> the
>>> default mode?
>> 
>> My ultimate goal is to avoid needlessly stuffing things in sysfs.  My 
>> hope in
>> suggesting a new API was that platform drivers could recognise the 
>> kind of
>> build/environment they operate in and setup the coredump mode 
>> accordingly.  That
>> would have allowed us to leave debugfs options alone.
>> 
>>> 
>>> Regarding the default mode, I think it would make sense to make the
>>> default "disabled", because this is the most sensible configuration 
>>> in a
>>> "production" environment. And the sysfs means we have a convenient
>>> mechanism to configure it, even on production environments.
>>> 
>> 
>> I am weary of changing something that hasn't been requested.
>> 
>>> > That will prevent breaking a fair amount of user space code...
>>> >
>>> 
>>> We typically don't guarantee that the debugfs interfaces are stable 
>>> and
>>> if I understand the beginning of you reply you still want to move it
>>> from debugfs to sysfs - which I presume would break such scripts in 
>>> the
>>> first place?
>> 
>> Correct - I am sure that moving coredump and recovery options to sysfs 
>> will
>> break user space scripts.  Even if debugfs is not part of the ABI it 
>> would be
>> nice to avoid disrupting people as much as possible.
>> 
>>> 
>>> 
>>> I would prefer to see that we don't introduce config options for 
>>> every
>>> little thing, unless there's good reason for it.
>> 
>> I totally agree.  It is with great reluctance that I asked Rishab to 
>> proceed
>> the way he did in V3.  His usecase makes sense... On the flip side 
>> this is
>> pushed down on the kernel community and I really like Christoph's 
>> position about
>> fixing Android and leaving the kernel alone.
>> 
> Well, removing debugfs is conscious decision taken by android due to 
> security
> concerns and there is not we can fix there.
> Would it be a terrible idea to have recovery and coredump exposed from 
> both
> sysfs and debugfs instead of choosing one and breaking userspace code?
>>> 
>>> Regards,
>>> Bjorn
>>> 
>>> > Let me know if that can work for you.
>>> >
>>> > Thanks,
>>> > Mathieu
>>> >
>>> > > 'Coredump' and 'Recovery' are critical
>>> > > interfaces that are required for remoteproc to work on Qualcomm Chipsets.
>>> > > Coredump configuration needs to be set to "inline" in debug/test build
>>> > > and "disabled" in production builds. Whereas recovery needs to be
>>> > > "disabled" for debugging purposes and "enabled" on production builds.
>>> > >
>>> > > Changelog:
>>> > >
>>> > > v1 -> v2:
>>> > > - Correct the contact name in the sysfs documentation.
>>> > > - Remove the redundant write documentation for coredump/recovery sysfs
>>> > > - Add a feature flag to make this interface switch configurable.
>>> > >
>>> > > Rishabh Bhatnagar (3):
>>> > >   remoteproc: Expose remoteproc configuration through sysfs
>>> > >   remoteproc: Add coredump configuration to sysfs
>>> > >   remoteproc: Add recovery configuration to sysfs
>>> > >
>>> > >  Documentation/ABI/testing/sysfs-class-remoteproc |  44 ++++++++
>>> > >  drivers/remoteproc/Kconfig                       |  12 +++
>>> > >  drivers/remoteproc/remoteproc_debugfs.c          |  10 +-
>>> > >  drivers/remoteproc/remoteproc_sysfs.c            | 126 +++++++++++++++++++++++
>>> > >  4 files changed, 190 insertions(+), 2 deletions(-)
>>> > >
>>> > > --
>>> > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>>> > > a Linux Foundation Collaborative Project
>>> > >

Re: [PATCH v2 0/3] Expose recovery/coredump configuration from sysfs

[Mathieu Poirier]

On Wed, Sep 09, 2020 at 05:27:46PM +0000, rishabhb@codeaurora.org wrote:
> On 2020-09-04 15:02, Mathieu Poirier wrote:
> > On Thu, Sep 03, 2020 at 06:59:44PM -0500, Bjorn Andersson wrote:
> > > On Tue 01 Sep 17:05 CDT 2020, Mathieu Poirier wrote:
> > > 
> > > > Hi Rishabh,
> > > >
> > > > On Thu, Aug 27, 2020 at 12:48:48PM -0700, Rishabh Bhatnagar wrote:
> > > > > From Android R onwards Google has restricted access to debugfs in user
> > > > > and user-debug builds. This restricts access to most of the features
> > > > > exposed through debugfs. This patch series adds a configurable option
> > > > > to move the recovery/coredump interfaces to sysfs. If the feature
> > > > > flag is selected it would move these interfaces to sysfs and remove
> > > > > the equivalent debugfs interface.
> > > >
> > > > What I meant wast to move the coredump entry from debugfs to sysfs and from
> > > > there make it available to user space using a kernel config.
> > > 
> > > Why would we not always make this available in sysfs?
> > 
> > At this time the options are in debugfs and vendors can decide to make
> > that
> > available on products if they want to.  The idea behind using a kernel
> > configuration once moved to sysfs was to give the same kind of options.
> > 
> > > 
> > > > But thinking further on this it may be better to simply provide an API
> > > > to set the coredump mode from the platform driver, the same way
> > > > rproc_coredump_set_elf_info() works.
> > > 
> > > Being able to invoke these from the platform drivers sounds like a new
> > > feature. What would trigger the platform drivers to call this? Or are
> > > you perhaps asking for the means of the drivers to be able to select
> > > the
> > > default mode?
> > 
> > My ultimate goal is to avoid needlessly stuffing things in sysfs.  My
> > hope in
> > suggesting a new API was that platform drivers could recognise the kind
> > of
> > build/environment they operate in and setup the coredump mode
> > accordingly.  That
> > would have allowed us to leave debugfs options alone.
> > 
> > > 
> > > Regarding the default mode, I think it would make sense to make the
> > > default "disabled", because this is the most sensible configuration
> > > in a
> > > "production" environment. And the sysfs means we have a convenient
> > > mechanism to configure it, even on production environments.
> > > 
> > 
> > I am weary of changing something that hasn't been requested.
> > 
> > > > That will prevent breaking a fair amount of user space code...
> > > >
> > > 
> > > We typically don't guarantee that the debugfs interfaces are stable
> > > and
> > > if I understand the beginning of you reply you still want to move it
> > > from debugfs to sysfs - which I presume would break such scripts in
> > > the
> > > first place?
> > 
> > Correct - I am sure that moving coredump and recovery options to sysfs
> > will
> > break user space scripts.  Even if debugfs is not part of the ABI it
> > would be
> > nice to avoid disrupting people as much as possible.
> > 
> > > 
> > > 
> > > I would prefer to see that we don't introduce config options for every
> > > little thing, unless there's good reason for it.
> > 
> > I totally agree.  It is with great reluctance that I asked Rishab to
> > proceed
> > the way he did in V3.  His usecase makes sense... On the flip side this
> > is
> > pushed down on the kernel community and I really like Christoph's
> > position about
> > fixing Android and leaving the kernel alone.
> > 
> Well, removing debugfs is conscious decision taken by android due to
> security
> concerns and there is not we can fix there.
> Would it be a terrible idea to have recovery and coredump exposed from both
> sysfs and debugfs instead of choosing one and breaking userspace code?

Yes, two interfaces to do the same thing is not acceptable.

That being said Arnaud Pouliquen had the excellent idea of using the newly added
remoteproc character device.   

> > > 
> > > Regards,
> > > Bjorn
> > > 
> > > > Let me know if that can work for you.
> > > >
> > > > Thanks,
> > > > Mathieu
> > > >
> > > > > 'Coredump' and 'Recovery' are critical
> > > > > interfaces that are required for remoteproc to work on Qualcomm Chipsets.
> > > > > Coredump configuration needs to be set to "inline" in debug/test build
> > > > > and "disabled" in production builds. Whereas recovery needs to be
> > > > > "disabled" for debugging purposes and "enabled" on production builds.
> > > > >
> > > > > Changelog:
> > > > >
> > > > > v1 -> v2:
> > > > > - Correct the contact name in the sysfs documentation.
> > > > > - Remove the redundant write documentation for coredump/recovery sysfs
> > > > > - Add a feature flag to make this interface switch configurable.
> > > > >
> > > > > Rishabh Bhatnagar (3):
> > > > >   remoteproc: Expose remoteproc configuration through sysfs
> > > > >   remoteproc: Add coredump configuration to sysfs
> > > > >   remoteproc: Add recovery configuration to sysfs
> > > > >
> > > > >  Documentation/ABI/testing/sysfs-class-remoteproc |  44 ++++++++
> > > > >  drivers/remoteproc/Kconfig                       |  12 +++
> > > > >  drivers/remoteproc/remoteproc_debugfs.c          |  10 +-
> > > > >  drivers/remoteproc/remoteproc_sysfs.c            | 126 +++++++++++++++++++++++
> > > > >  4 files changed, 190 insertions(+), 2 deletions(-)
> > > > >
> > > > > --
> > > > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> > > > > a Linux Foundation Collaborative Project
> > > > >

Re: [PATCH v2 0/3] Expose recovery/coredump configuration from sysfs

[Greg KH]

On Fri, Sep 04, 2020 at 04:02:13PM -0600, Mathieu Poirier wrote:
> On Thu, Sep 03, 2020 at 06:59:44PM -0500, Bjorn Andersson wrote:
> > On Tue 01 Sep 17:05 CDT 2020, Mathieu Poirier wrote:
> > 
> > > Hi Rishabh,
> > > 
> > > On Thu, Aug 27, 2020 at 12:48:48PM -0700, Rishabh Bhatnagar wrote:
> > > > From Android R onwards Google has restricted access to debugfs in user
> > > > and user-debug builds. This restricts access to most of the features
> > > > exposed through debugfs. This patch series adds a configurable option
> > > > to move the recovery/coredump interfaces to sysfs. If the feature
> > > > flag is selected it would move these interfaces to sysfs and remove
> > > > the equivalent debugfs interface.
> > > 
> > > What I meant wast to move the coredump entry from debugfs to sysfs and from
> > > there make it available to user space using a kernel config.
> > 
> > Why would we not always make this available in sysfs?
> 
> At this time the options are in debugfs and vendors can decide to make that
> available on products if they want to.  The idea behind using a kernel
> configuration once moved to sysfs was to give the same kind of options.
> 
> > 
> > > But thinking further on this it may be better to simply provide an API
> > > to set the coredump mode from the platform driver, the same way
> > > rproc_coredump_set_elf_info() works.
> > 
> > Being able to invoke these from the platform drivers sounds like a new
> > feature. What would trigger the platform drivers to call this? Or are
> > you perhaps asking for the means of the drivers to be able to select the
> > default mode?
> 
> My ultimate goal is to avoid needlessly stuffing things in sysfs.  My hope in
> suggesting a new API was that platform drivers could recognise the kind of
> build/environment they operate in and setup the coredump mode accordingly.  That
> would have allowed us to leave debugfs options alone.
> 
> > 
> > Regarding the default mode, I think it would make sense to make the
> > default "disabled", because this is the most sensible configuration in a
> > "production" environment. And the sysfs means we have a convenient
> > mechanism to configure it, even on production environments.
> >
> 
> I am weary of changing something that hasn't been requested.  
>  
> > > That will prevent breaking a fair amount of user space code...
> > > 
> > 
> > We typically don't guarantee that the debugfs interfaces are stable and
> > if I understand the beginning of you reply you still want to move it
> > from debugfs to sysfs - which I presume would break such scripts in the
> > first place?
> 
> Correct - I am sure that moving coredump and recovery options to sysfs will
> break user space scripts.  Even if debugfs is not part of the ABI it would be
> nice to avoid disrupting people as much as possible.

Don't move the files, keep them in both places.  Lots of systems
restrict debugfs, so moving "stable" stuff like this into sysfs makes
sense.

thanks,

greg k-h

Re: [PATCH v2 0/3] Expose recovery/coredump configuration from sysfs

[Arnaud POULIQUEN]

Hi Rishabh,

On 8/27/20 9:48 PM, Rishabh Bhatnagar wrote:
> From Android R onwards Google has restricted access to debugfs in user
> and user-debug builds. This restricts access to most of the features
> exposed through debugfs. This patch series adds a configurable option
> to move the recovery/coredump interfaces to sysfs. If the feature
> flag is selected it would move these interfaces to sysfs and remove
> the equivalent debugfs interface. 'Coredump' and 'Recovery' are critical
> interfaces that are required for remoteproc to work on Qualcomm Chipsets.
> Coredump configuration needs to be set to "inline" in debug/test build
> and "disabled" in production builds. Whereas recovery needs to be
> "disabled" for debugging purposes and "enabled" on production builds.

The remoteproc_cdev had been created to respond to some sysfs limitations.
I wonder if this evolution should not also be implemented in the cdev.
In this case an additional event could be addedd to inform the application
that a crash occurred and that a core dump is available.

Of course it's only a suggestion... As it would be a redesign.
I let Björn and Mathieu comment.

Regards,
Arnaud

> 
> Changelog:
> 
> v1 -> v2:
> - Correct the contact name in the sysfs documentation.
> - Remove the redundant write documentation for coredump/recovery sysfs
> - Add a feature flag to make this interface switch configurable.
> 
> Rishabh Bhatnagar (3):
>   remoteproc: Expose remoteproc configuration through sysfs
>   remoteproc: Add coredump configuration to sysfs
>   remoteproc: Add recovery configuration to sysfs
> 
>  Documentation/ABI/testing/sysfs-class-remoteproc |  44 ++++++++
>  drivers/remoteproc/Kconfig                       |  12 +++
>  drivers/remoteproc/remoteproc_debugfs.c          |  10 +-
>  drivers/remoteproc/remoteproc_sysfs.c            | 126 +++++++++++++++++++++++
>  4 files changed, 190 insertions(+), 2 deletions(-)
> 

Re: [PATCH v2 0/3] Expose recovery/coredump configuration from sysfs

[Bjorn Andersson]

On Tue 15 Sep 02:51 PDT 2020, Arnaud POULIQUEN wrote:

> Hi Rishabh,
> 
> On 8/27/20 9:48 PM, Rishabh Bhatnagar wrote:
> > From Android R onwards Google has restricted access to debugfs in user
> > and user-debug builds. This restricts access to most of the features
> > exposed through debugfs. This patch series adds a configurable option
> > to move the recovery/coredump interfaces to sysfs. If the feature
> > flag is selected it would move these interfaces to sysfs and remove
> > the equivalent debugfs interface. 'Coredump' and 'Recovery' are critical
> > interfaces that are required for remoteproc to work on Qualcomm Chipsets.
> > Coredump configuration needs to be set to "inline" in debug/test build
> > and "disabled" in production builds. Whereas recovery needs to be
> > "disabled" for debugging purposes and "enabled" on production builds.
> 
> The remoteproc_cdev had been created to respond to some sysfs limitations.

The limitation here is in debugfs not being available on all systems,
sysfs is present and I really do like the idea of being able to change
these things without having to compile a tool to invoke the ioctl...

> I wonder if this evolution should not also be implemented in the cdev.
> In this case an additional event could be addedd to inform the application
> that a crash occurred and that a core dump is available.
> 

Specifically for userspace to know when a coredump is present there's
already uevents being sent when the devcoredump is ready. That said,
having some means to getting notified about remoteproc state changes
does sounds reasonable. If there is a use case we should discuss that.

> Of course it's only a suggestion... As it would be a redesign.

A very valid suggestion. I don't think it's a redesign, but more of an
extension of what we have today.

Regards,
Bjorn

> I let Björn and Mathieu comment.
> 
> Regards,
> Arnaud
> 
> > 
> > Changelog:
> > 
> > v1 -> v2:
> > - Correct the contact name in the sysfs documentation.
> > - Remove the redundant write documentation for coredump/recovery sysfs
> > - Add a feature flag to make this interface switch configurable.
> > 
> > Rishabh Bhatnagar (3):
> >   remoteproc: Expose remoteproc configuration through sysfs
> >   remoteproc: Add coredump configuration to sysfs
> >   remoteproc: Add recovery configuration to sysfs
> > 
> >  Documentation/ABI/testing/sysfs-class-remoteproc |  44 ++++++++
> >  drivers/remoteproc/Kconfig                       |  12 +++
> >  drivers/remoteproc/remoteproc_debugfs.c          |  10 +-
> >  drivers/remoteproc/remoteproc_sysfs.c            | 126 +++++++++++++++++++++++
> >  4 files changed, 190 insertions(+), 2 deletions(-)
> > 

Re: [PATCH v2 0/3] Expose recovery/coredump configuration from sysfs

[Arnaud POULIQUEN]

Hi Bjorn,

On 9/26/20 5:31 AM, Bjorn Andersson wrote:
> On Tue 15 Sep 02:51 PDT 2020, Arnaud POULIQUEN wrote:
> 
>> Hi Rishabh,
>>
>> On 8/27/20 9:48 PM, Rishabh Bhatnagar wrote:
>>> From Android R onwards Google has restricted access to debugfs in user
>>> and user-debug builds. This restricts access to most of the features
>>> exposed through debugfs. This patch series adds a configurable option
>>> to move the recovery/coredump interfaces to sysfs. If the feature
>>> flag is selected it would move these interfaces to sysfs and remove
>>> the equivalent debugfs interface. 'Coredump' and 'Recovery' are critical
>>> interfaces that are required for remoteproc to work on Qualcomm Chipsets.
>>> Coredump configuration needs to be set to "inline" in debug/test build
>>> and "disabled" in production builds. Whereas recovery needs to be
>>> "disabled" for debugging purposes and "enabled" on production builds.
>>
>> The remoteproc_cdev had been created to respond to some sysfs limitations.
> 
> The limitation here is in debugfs not being available on all systems,
> sysfs is present and I really do like the idea of being able to change
> these things without having to compile a tool to invoke the ioctl...

Right,

> 
>> I wonder if this evolution should not also be implemented in the cdev.
>> In this case an additional event could be addedd to inform the application
>> that a crash occurred and that a core dump is available.
>>
> 
> Specifically for userspace to know when a coredump is present there's
> already uevents being sent when the devcoredump is ready. That said,
> having some means to getting notified about remoteproc state changes
> does sounds reasonable. If there is a use case we should discuss that.

The main use case i have in mind is to inform the userspace that the remote
processor has crashed. This would allow applications to perform specific action
to avoid getting stuck and/or resetting it's environement befor restarting the
remote processor and associated IPC.
If i well remember QCOM has this kind of mechanism for its modem but this is
implemented in a platform driver.
We would be interested to have something more generic relying on the remoteproc
framework.

Thanks,
Arnaud

> 
>> Of course it's only a suggestion... As it would be a redesign.
> 
> A very valid suggestion. I don't think it's a redesign, but more of an
> extension of what we have today.
> 
> Regards,
> Bjorn
> 
>> I let Björn and Mathieu comment.
>>
>> Regards,
>> Arnaud
>>
>>>
>>> Changelog:
>>>
>>> v1 -> v2:
>>> - Correct the contact name in the sysfs documentation.
>>> - Remove the redundant write documentation for coredump/recovery sysfs
>>> - Add a feature flag to make this interface switch configurable.
>>>
>>> Rishabh Bhatnagar (3):
>>>   remoteproc: Expose remoteproc configuration through sysfs
>>>   remoteproc: Add coredump configuration to sysfs
>>>   remoteproc: Add recovery configuration to sysfs
>>>
>>>  Documentation/ABI/testing/sysfs-class-remoteproc |  44 ++++++++
>>>  drivers/remoteproc/Kconfig                       |  12 +++
>>>  drivers/remoteproc/remoteproc_debugfs.c          |  10 +-
>>>  drivers/remoteproc/remoteproc_sysfs.c            | 126 +++++++++++++++++++++++
>>>  4 files changed, 190 insertions(+), 2 deletions(-)
>>>

Re: [PATCH v2 0/3] Expose recovery/coredump configuration from sysfs

[Bjorn Andersson]

On Tue 29 Sep 02:43 CDT 2020, Arnaud POULIQUEN wrote:

> Hi Bjorn,
> 
> On 9/26/20 5:31 AM, Bjorn Andersson wrote:
> > On Tue 15 Sep 02:51 PDT 2020, Arnaud POULIQUEN wrote:
> > 
> >> Hi Rishabh,
> >>
> >> On 8/27/20 9:48 PM, Rishabh Bhatnagar wrote:
> >>> From Android R onwards Google has restricted access to debugfs in user
> >>> and user-debug builds. This restricts access to most of the features
> >>> exposed through debugfs. This patch series adds a configurable option
> >>> to move the recovery/coredump interfaces to sysfs. If the feature
> >>> flag is selected it would move these interfaces to sysfs and remove
> >>> the equivalent debugfs interface. 'Coredump' and 'Recovery' are critical
> >>> interfaces that are required for remoteproc to work on Qualcomm Chipsets.
> >>> Coredump configuration needs to be set to "inline" in debug/test build
> >>> and "disabled" in production builds. Whereas recovery needs to be
> >>> "disabled" for debugging purposes and "enabled" on production builds.
> >>
> >> The remoteproc_cdev had been created to respond to some sysfs limitations.
> > 
> > The limitation here is in debugfs not being available on all systems,
> > sysfs is present and I really do like the idea of being able to change
> > these things without having to compile a tool to invoke the ioctl...
> 
> Right,
> 
> > 
> >> I wonder if this evolution should not also be implemented in the cdev.
> >> In this case an additional event could be addedd to inform the application
> >> that a crash occurred and that a core dump is available.
> >>
> > 
> > Specifically for userspace to know when a coredump is present there's
> > already uevents being sent when the devcoredump is ready. That said,
> > having some means to getting notified about remoteproc state changes
> > does sounds reasonable. If there is a use case we should discuss that.
> 
> The main use case i have in mind is to inform the userspace that the remote
> processor has crashed. This would allow applications to perform specific action
> to avoid getting stuck and/or resetting it's environement befor restarting the
> remote processor and associated IPC.
> If i well remember QCOM has this kind of mechanism for its modem but this is
> implemented in a platform driver.
> We would be interested to have something more generic relying on the remoteproc
> framework.
> 

I believe that there is such a notification mechanism implemented by
Qualcomm downstream. Upstream we've so far relied on the fact that the
interfaces exposed by the various rpmsg_devices would be torn down and
re-registered as the remoteproc is restarted.

Same goes with the few cases where we use rpmsg_char, as the channels
are going down the IO operations on the rpmsg endpoint fails to allow
userspace to detect the shutdown part. Then as the new channels appears
userspace will be notified about the newly available channels through
the standard uevents.

Regards,
Bjorn

> Thanks,
> Arnaud
> 
> > 
> >> Of course it's only a suggestion... As it would be a redesign.
> > 
> > A very valid suggestion. I don't think it's a redesign, but more of an
> > extension of what we have today.
> > 
> > Regards,
> > Bjorn
> > 
> >> I let Björn and Mathieu comment.
> >>
> >> Regards,
> >> Arnaud
> >>
> >>>
> >>> Changelog:
> >>>
> >>> v1 -> v2:
> >>> - Correct the contact name in the sysfs documentation.
> >>> - Remove the redundant write documentation for coredump/recovery sysfs
> >>> - Add a feature flag to make this interface switch configurable.
> >>>
> >>> Rishabh Bhatnagar (3):
> >>>   remoteproc: Expose remoteproc configuration through sysfs
> >>>   remoteproc: Add coredump configuration to sysfs
> >>>   remoteproc: Add recovery configuration to sysfs
> >>>
> >>>  Documentation/ABI/testing/sysfs-class-remoteproc |  44 ++++++++
> >>>  drivers/remoteproc/Kconfig                       |  12 +++
> >>>  drivers/remoteproc/remoteproc_debugfs.c          |  10 +-
> >>>  drivers/remoteproc/remoteproc_sysfs.c            | 126 +++++++++++++++++++++++
> >>>  4 files changed, 190 insertions(+), 2 deletions(-)
> >>>