[PATCH v2 0/3] remoteproc sysfs fixes/improvements

[PATCH v2 0/3] remoteproc sysfs fixes/improvements

[Suman Anna]

Hi All,

This is a refresh of the unaccepted patches from an old series [1].
Patches 2 and 3 from that series were merged and these are rebased and
revised versions of the same patches. I had forgotten about these patches,
and am resurrecting these again. Patches are on top of latest 5.10-rc4.

The features being introduced here will be needed by the recently posted PRU
remoteproc driver [2] in addition to the existing Wkup M3 remoteproc driver.
Both of these drivers follow a client-driven boot methodology, with the latter
strictly booted by another driver in kernel. The PRU remoteproc driver will be
supporting both in-kernel clients as well as control from userspace orthogonally.
The logic though is applicable and useful to any remoteproc driver not using
'auto-boot' and using an external driver/application to boot the remoteproc.

regards
Suman

[1] https://patchwork.kernel.org/project/linux-remoteproc/cover/20180915003725.17549-1-s-anna@ti.com/
[2] https://patchwork.kernel.org/project/linux-remoteproc/cover/20201119140850.12268-1-grzegorz.jaszczyk@linaro.org/

Suman Anna (3):
  remoteproc: Fix unbalanced boot with sysfs for no auto-boot rprocs
  remoteproc: Introduce deny_sysfs_ops flag
  remoteproc: wkup_m3: Set deny_sysfs_ops flag

 drivers/remoteproc/remoteproc_sysfs.c | 28 ++++++++++++++++++++++++++-
 drivers/remoteproc/wkup_m3_rproc.c    |  1 +
 include/linux/remoteproc.h            |  2 ++
 3 files changed, 30 insertions(+), 1 deletion(-)

-- 
2.28.0

[PATCH v2 1/3] remoteproc: Fix unbalanced boot with sysfs for no auto-boot rprocs

[Suman Anna]

The remoteproc core performs automatic boot and shutdown of a remote
processor during rproc_add() and rproc_del() for remote processors
supporting 'auto-boot'. The remoteproc devices not using 'auto-boot'
require either a remoteproc client driver or a userspace client to
use the sysfs 'state' variable to perform the boot and shutdown. The
in-kernel client drivers hold the corresponding remoteproc driver
module's reference count when they acquire a rproc handle through
the rproc_get_by_phandle() API, but there is no such support for
userspace applications performing the boot through sysfs interface.

The shutdown of a remoteproc upon removing a remoteproc platform
driver is automatic only with 'auto-boot' and this can cause a
remoteproc with no auto-boot to stay powered on and never freed
up if booted using the sysfs interface without a matching stop,
and when the remoteproc driver module is removed or unbound from
the device. This will result in a memory leak as well as the
corresponding remoteproc ida being never deallocated. Fix this
by holding a module reference count for the remoteproc's driver
during a sysfs 'start' and releasing it during the sysfs 'stop'
operation.

Signed-off-by: Suman Anna <s-anna@ti.com>
Acked-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
---
v2: rebased version, no changes
v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/20180915003725.17549-2-s-anna@ti.com/

 drivers/remoteproc/remoteproc_sysfs.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
index d1cf7bf277c4..bd2950a246c9 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -3,6 +3,7 @@
  * Remote Processor Framework
  */
 
+#include <linux/module.h>
 #include <linux/remoteproc.h>
 #include <linux/slab.h>
 
@@ -228,14 +229,27 @@ static ssize_t state_store(struct device *dev,
 		if (rproc->state == RPROC_RUNNING)
 			return -EBUSY;
 
+		/*
+		 * prevent underlying implementation from being removed
+		 * when remoteproc does not support auto-boot
+		 */
+		if (!rproc->auto_boot &&
+		    !try_module_get(dev->parent->driver->owner))
+			return -EINVAL;
+
 		ret = rproc_boot(rproc);
-		if (ret)
+		if (ret) {
 			dev_err(&rproc->dev, "Boot failed: %d\n", ret);
+			if (!rproc->auto_boot)
+				module_put(dev->parent->driver->owner);
+		}
 	} else if (sysfs_streq(buf, "stop")) {
 		if (rproc->state != RPROC_RUNNING)
 			return -EINVAL;
 
 		rproc_shutdown(rproc);
+		if (!rproc->auto_boot)
+			module_put(dev->parent->driver->owner);
 	} else {
 		dev_err(&rproc->dev, "Unrecognised option: %s\n", buf);
 		ret = -EINVAL;
-- 
2.28.0

Re: [PATCH v2 1/3] remoteproc: Fix unbalanced boot with sysfs for no auto-boot rprocs

[Mathieu Poirier]

On Fri, Nov 20, 2020 at 09:01:54PM -0600, Suman Anna wrote:
> The remoteproc core performs automatic boot and shutdown of a remote
> processor during rproc_add() and rproc_del() for remote processors
> supporting 'auto-boot'. The remoteproc devices not using 'auto-boot'
> require either a remoteproc client driver or a userspace client to
> use the sysfs 'state' variable to perform the boot and shutdown. The
> in-kernel client drivers hold the corresponding remoteproc driver
> module's reference count when they acquire a rproc handle through
> the rproc_get_by_phandle() API, but there is no such support for
> userspace applications performing the boot through sysfs interface.
> 
> The shutdown of a remoteproc upon removing a remoteproc platform
> driver is automatic only with 'auto-boot' and this can cause a
> remoteproc with no auto-boot to stay powered on and never freed
> up if booted using the sysfs interface without a matching stop,
> and when the remoteproc driver module is removed or unbound from
> the device. This will result in a memory leak as well as the
> corresponding remoteproc ida being never deallocated. Fix this
> by holding a module reference count for the remoteproc's driver
> during a sysfs 'start' and releasing it during the sysfs 'stop'
> operation.
> 
> Signed-off-by: Suman Anna <s-anna@ti.com>
> Acked-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> ---
> v2: rebased version, no changes
> v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/20180915003725.17549-2-s-anna@ti.com/
> 
>  drivers/remoteproc/remoteproc_sysfs.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> index d1cf7bf277c4..bd2950a246c9 100644
> --- a/drivers/remoteproc/remoteproc_sysfs.c
> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> @@ -3,6 +3,7 @@
>   * Remote Processor Framework
>   */
>  
> +#include <linux/module.h>
>  #include <linux/remoteproc.h>
>  #include <linux/slab.h>
>  
> @@ -228,14 +229,27 @@ static ssize_t state_store(struct device *dev,
>  		if (rproc->state == RPROC_RUNNING)
>  			return -EBUSY;
>  
> +		/*
> +		 * prevent underlying implementation from being removed
> +		 * when remoteproc does not support auto-boot
> +		 */
> +		if (!rproc->auto_boot &&
> +		    !try_module_get(dev->parent->driver->owner))
> +			return -EINVAL;
> +
>  		ret = rproc_boot(rproc);
> -		if (ret)
> +		if (ret) {
>  			dev_err(&rproc->dev, "Boot failed: %d\n", ret);
> +			if (!rproc->auto_boot)
> +				module_put(dev->parent->driver->owner);
> +		}
>  	} else if (sysfs_streq(buf, "stop")) {
>  		if (rproc->state != RPROC_RUNNING)
>  			return -EINVAL;
>  
>  		rproc_shutdown(rproc);
> +		if (!rproc->auto_boot)
> +			module_put(dev->parent->driver->owner);

I tackled the same problem by fixing another problem we had in the core.  Patch
2 [1] and 3 [2] of this set [3] get rid of the problem related to the auto_boot
check without having to deal with module counters.

Please see if that covers the use case you are dealing with.

Thanks,
Mathieu

[1]. https://patchwork.kernel.org/project/linux-remoteproc/patch/20201126210642.897302-3-mathieu.poirier@linaro.org/
[2]. https://patchwork.kernel.org/project/linux-remoteproc/patch/20201126210642.897302-4-mathieu.poirier@linaro.org/
[3]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=391789


>  	} else {
>  		dev_err(&rproc->dev, "Unrecognised option: %s\n", buf);
>  		ret = -EINVAL;
> -- 
> 2.28.0
> 

[PATCH v2 2/3] remoteproc: Introduce deny_sysfs_ops flag

[Suman Anna]

The remoteproc framework provides sysfs interfaces for changing
the firmware name and for starting/stopping a remote processor
through the sysfs files 'state' and 'firmware'. The 'recovery'
sysfs file can also be used similarly to control the error recovery
state machine of a remoteproc. These interfaces are currently
allowed irrespective of how the remoteprocs were booted (like
remoteproc self auto-boot, remoteproc client-driven boot etc).
These interfaces can adversely affect a remoteproc and its clients
especially when a remoteproc is being controlled by a remoteproc
client driver(s). Also, not all remoteproc drivers may want to
support the sysfs interfaces by default.

Add support to deny the sysfs state/firmware/recovery change by
introducing a state flag 'deny_sysfs_ops' that the individual
remoteproc drivers can set based on their usage needs. The default
behavior is to allow the sysfs operations as before.

Signed-off-by: Suman Anna <s-anna@ti.com>
---
v2: revised to account for the 'recovery' sysfs file as well, patch
    description updated accordingly
v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/20180915003725.17549-5-s-anna@ti.com/

 drivers/remoteproc/remoteproc_sysfs.c | 12 ++++++++++++
 include/linux/remoteproc.h            |  2 ++
 2 files changed, 14 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
index bd2950a246c9..3fd18a71c188 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -49,6 +49,10 @@ static ssize_t recovery_store(struct device *dev,
 {
 	struct rproc *rproc = to_rproc(dev);
 
+	/* restrict sysfs operations if not allowed by remoteproc drivers */
+	if (rproc->deny_sysfs_ops)
+		return -EPERM;
+
 	if (sysfs_streq(buf, "enabled")) {
 		/* change the flag and begin the recovery process if needed */
 		rproc->recovery_disabled = false;
@@ -158,6 +162,10 @@ static ssize_t firmware_store(struct device *dev,
 	char *p;
 	int err, len = count;
 
+	/* restrict sysfs operations if not allowed by remoteproc drivers */
+	if (rproc->deny_sysfs_ops)
+		return -EPERM;
+
 	err = mutex_lock_interruptible(&rproc->lock);
 	if (err) {
 		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, err);
@@ -225,6 +233,10 @@ static ssize_t state_store(struct device *dev,
 	struct rproc *rproc = to_rproc(dev);
 	int ret = 0;
 
+	/* restrict sysfs operations if not allowed by remoteproc drivers */
+	if (rproc->deny_sysfs_ops)
+		return -EPERM;
+
 	if (sysfs_streq(buf, "start")) {
 		if (rproc->state == RPROC_RUNNING)
 			return -EBUSY;
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 3fa3ba6498e8..dbc3767f7d0e 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -508,6 +508,7 @@ struct rproc_dump_segment {
  * @has_iommu: flag to indicate if remote processor is behind an MMU
  * @auto_boot: flag to indicate if remote processor should be auto-started
  * @autonomous: true if an external entity has booted the remote processor
+ * @deny_sysfs_ops: flag to not permit sysfs operations on state, firmware and recovery
  * @dump_segments: list of segments in the firmware
  * @nb_vdev: number of vdev currently handled by rproc
  * @char_dev: character device of the rproc
@@ -545,6 +546,7 @@ struct rproc {
 	bool has_iommu;
 	bool auto_boot;
 	bool autonomous;
+	bool deny_sysfs_ops;
 	struct list_head dump_segments;
 	int nb_vdev;
 	u8 elf_class;
-- 
2.28.0

Re: [PATCH v2 2/3] remoteproc: Introduce deny_sysfs_ops flag

[Bjorn Andersson]

On Fri 20 Nov 21:01 CST 2020, Suman Anna wrote:

> The remoteproc framework provides sysfs interfaces for changing
> the firmware name and for starting/stopping a remote processor
> through the sysfs files 'state' and 'firmware'. The 'recovery'
> sysfs file can also be used similarly to control the error recovery
> state machine of a remoteproc. These interfaces are currently
> allowed irrespective of how the remoteprocs were booted (like
> remoteproc self auto-boot, remoteproc client-driven boot etc).
> These interfaces can adversely affect a remoteproc and its clients
> especially when a remoteproc is being controlled by a remoteproc
> client driver(s). Also, not all remoteproc drivers may want to
> support the sysfs interfaces by default.
> 
> Add support to deny the sysfs state/firmware/recovery change by
> introducing a state flag 'deny_sysfs_ops' that the individual
> remoteproc drivers can set based on their usage needs. The default
> behavior is to allow the sysfs operations as before.
> 

This makes sense, but can't we implement attribute_group->is_visible to
simply hide these entries from userspace instead of leaving them
"broken"?

Regards,
Bjorn

> Signed-off-by: Suman Anna <s-anna@ti.com>
> ---
> v2: revised to account for the 'recovery' sysfs file as well, patch
>     description updated accordingly
> v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/20180915003725.17549-5-s-anna@ti.com/
> 
>  drivers/remoteproc/remoteproc_sysfs.c | 12 ++++++++++++
>  include/linux/remoteproc.h            |  2 ++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> index bd2950a246c9..3fd18a71c188 100644
> --- a/drivers/remoteproc/remoteproc_sysfs.c
> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> @@ -49,6 +49,10 @@ static ssize_t recovery_store(struct device *dev,
>  {
>  	struct rproc *rproc = to_rproc(dev);
>  
> +	/* restrict sysfs operations if not allowed by remoteproc drivers */
> +	if (rproc->deny_sysfs_ops)
> +		return -EPERM;
> +
>  	if (sysfs_streq(buf, "enabled")) {
>  		/* change the flag and begin the recovery process if needed */
>  		rproc->recovery_disabled = false;
> @@ -158,6 +162,10 @@ static ssize_t firmware_store(struct device *dev,
>  	char *p;
>  	int err, len = count;
>  
> +	/* restrict sysfs operations if not allowed by remoteproc drivers */
> +	if (rproc->deny_sysfs_ops)
> +		return -EPERM;
> +
>  	err = mutex_lock_interruptible(&rproc->lock);
>  	if (err) {
>  		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, err);
> @@ -225,6 +233,10 @@ static ssize_t state_store(struct device *dev,
>  	struct rproc *rproc = to_rproc(dev);
>  	int ret = 0;
>  
> +	/* restrict sysfs operations if not allowed by remoteproc drivers */
> +	if (rproc->deny_sysfs_ops)
> +		return -EPERM;
> +
>  	if (sysfs_streq(buf, "start")) {
>  		if (rproc->state == RPROC_RUNNING)
>  			return -EBUSY;
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 3fa3ba6498e8..dbc3767f7d0e 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -508,6 +508,7 @@ struct rproc_dump_segment {
>   * @has_iommu: flag to indicate if remote processor is behind an MMU
>   * @auto_boot: flag to indicate if remote processor should be auto-started
>   * @autonomous: true if an external entity has booted the remote processor
> + * @deny_sysfs_ops: flag to not permit sysfs operations on state, firmware and recovery
>   * @dump_segments: list of segments in the firmware
>   * @nb_vdev: number of vdev currently handled by rproc
>   * @char_dev: character device of the rproc
> @@ -545,6 +546,7 @@ struct rproc {
>  	bool has_iommu;
>  	bool auto_boot;
>  	bool autonomous;
> +	bool deny_sysfs_ops;
>  	struct list_head dump_segments;
>  	int nb_vdev;
>  	u8 elf_class;
> -- 
> 2.28.0
> 

Re: [PATCH v2 2/3] remoteproc: Introduce deny_sysfs_ops flag

[Suman Anna]

On 11/20/20 9:38 PM, Bjorn Andersson wrote:
> On Fri 20 Nov 21:01 CST 2020, Suman Anna wrote:
> 
>> The remoteproc framework provides sysfs interfaces for changing
>> the firmware name and for starting/stopping a remote processor
>> through the sysfs files 'state' and 'firmware'. The 'recovery'
>> sysfs file can also be used similarly to control the error recovery
>> state machine of a remoteproc. These interfaces are currently
>> allowed irrespective of how the remoteprocs were booted (like
>> remoteproc self auto-boot, remoteproc client-driven boot etc).
>> These interfaces can adversely affect a remoteproc and its clients
>> especially when a remoteproc is being controlled by a remoteproc
>> client driver(s). Also, not all remoteproc drivers may want to
>> support the sysfs interfaces by default.
>>
>> Add support to deny the sysfs state/firmware/recovery change by
>> introducing a state flag 'deny_sysfs_ops' that the individual
>> remoteproc drivers can set based on their usage needs. The default
>> behavior is to allow the sysfs operations as before.
>>
> 
> This makes sense, but can't we implement attribute_group->is_visible to
> simply hide these entries from userspace instead of leaving them
> "broken"?

I would have to look into that, but can that be changed dynamically?
Also, note that the enforcement is only on the writes/stores which impact
the state-machine, but not the reads/shows.

For PRU usecases, we will be setting this dynamically.

regards
Suman

> 
> Regards,
> Bjorn
> 
>> Signed-off-by: Suman Anna <s-anna@ti.com>
>> ---
>> v2: revised to account for the 'recovery' sysfs file as well, patch
>>     description updated accordingly
>> v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/20180915003725.17549-5-s-anna@ti.com/
>>
>>  drivers/remoteproc/remoteproc_sysfs.c | 12 ++++++++++++
>>  include/linux/remoteproc.h            |  2 ++
>>  2 files changed, 14 insertions(+)
>>
>> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
>> index bd2950a246c9..3fd18a71c188 100644
>> --- a/drivers/remoteproc/remoteproc_sysfs.c
>> +++ b/drivers/remoteproc/remoteproc_sysfs.c
>> @@ -49,6 +49,10 @@ static ssize_t recovery_store(struct device *dev,
>>  {
>>  	struct rproc *rproc = to_rproc(dev);
>>  
>> +	/* restrict sysfs operations if not allowed by remoteproc drivers */
>> +	if (rproc->deny_sysfs_ops)
>> +		return -EPERM;
>> +
>>  	if (sysfs_streq(buf, "enabled")) {
>>  		/* change the flag and begin the recovery process if needed */
>>  		rproc->recovery_disabled = false;
>> @@ -158,6 +162,10 @@ static ssize_t firmware_store(struct device *dev,
>>  	char *p;
>>  	int err, len = count;
>>  
>> +	/* restrict sysfs operations if not allowed by remoteproc drivers */
>> +	if (rproc->deny_sysfs_ops)
>> +		return -EPERM;
>> +
>>  	err = mutex_lock_interruptible(&rproc->lock);
>>  	if (err) {
>>  		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, err);
>> @@ -225,6 +233,10 @@ static ssize_t state_store(struct device *dev,
>>  	struct rproc *rproc = to_rproc(dev);
>>  	int ret = 0;
>>  
>> +	/* restrict sysfs operations if not allowed by remoteproc drivers */
>> +	if (rproc->deny_sysfs_ops)
>> +		return -EPERM;
>> +
>>  	if (sysfs_streq(buf, "start")) {
>>  		if (rproc->state == RPROC_RUNNING)
>>  			return -EBUSY;
>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> index 3fa3ba6498e8..dbc3767f7d0e 100644
>> --- a/include/linux/remoteproc.h
>> +++ b/include/linux/remoteproc.h
>> @@ -508,6 +508,7 @@ struct rproc_dump_segment {
>>   * @has_iommu: flag to indicate if remote processor is behind an MMU
>>   * @auto_boot: flag to indicate if remote processor should be auto-started
>>   * @autonomous: true if an external entity has booted the remote processor
>> + * @deny_sysfs_ops: flag to not permit sysfs operations on state, firmware and recovery
>>   * @dump_segments: list of segments in the firmware
>>   * @nb_vdev: number of vdev currently handled by rproc
>>   * @char_dev: character device of the rproc
>> @@ -545,6 +546,7 @@ struct rproc {
>>  	bool has_iommu;
>>  	bool auto_boot;
>>  	bool autonomous;
>> +	bool deny_sysfs_ops;
>>  	struct list_head dump_segments;
>>  	int nb_vdev;
>>  	u8 elf_class;
>> -- 
>> 2.28.0
>>

Re: [PATCH v2 2/3] remoteproc: Introduce deny_sysfs_ops flag

[Bjorn Andersson]

On Fri 20 Nov 21:44 CST 2020, Suman Anna wrote:

> On 11/20/20 9:38 PM, Bjorn Andersson wrote:
> > On Fri 20 Nov 21:01 CST 2020, Suman Anna wrote:
> > 
> >> The remoteproc framework provides sysfs interfaces for changing
> >> the firmware name and for starting/stopping a remote processor
> >> through the sysfs files 'state' and 'firmware'. The 'recovery'
> >> sysfs file can also be used similarly to control the error recovery
> >> state machine of a remoteproc. These interfaces are currently
> >> allowed irrespective of how the remoteprocs were booted (like
> >> remoteproc self auto-boot, remoteproc client-driven boot etc).
> >> These interfaces can adversely affect a remoteproc and its clients
> >> especially when a remoteproc is being controlled by a remoteproc
> >> client driver(s). Also, not all remoteproc drivers may want to
> >> support the sysfs interfaces by default.
> >>
> >> Add support to deny the sysfs state/firmware/recovery change by
> >> introducing a state flag 'deny_sysfs_ops' that the individual
> >> remoteproc drivers can set based on their usage needs. The default
> >> behavior is to allow the sysfs operations as before.
> >>
> > 
> > This makes sense, but can't we implement attribute_group->is_visible to
> > simply hide these entries from userspace instead of leaving them
> > "broken"?
> 
> I would have to look into that, but can that be changed dynamically?
> Also, note that the enforcement is only on the writes/stores which impact
> the state-machine, but not the reads/shows.
> 
> For PRU usecases, we will be setting this dynamically.
> 

It looks to be dynamic, but I don't know if there's any "caching"
involved. Please have a look and let me know.

Regards,
Bjorn

> regards
> Suman
> 
> > 
> > Regards,
> > Bjorn
> > 
> >> Signed-off-by: Suman Anna <s-anna@ti.com>
> >> ---
> >> v2: revised to account for the 'recovery' sysfs file as well, patch
> >>     description updated accordingly
> >> v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/20180915003725.17549-5-s-anna@ti.com/
> >>
> >>  drivers/remoteproc/remoteproc_sysfs.c | 12 ++++++++++++
> >>  include/linux/remoteproc.h            |  2 ++
> >>  2 files changed, 14 insertions(+)
> >>
> >> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> >> index bd2950a246c9..3fd18a71c188 100644
> >> --- a/drivers/remoteproc/remoteproc_sysfs.c
> >> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> >> @@ -49,6 +49,10 @@ static ssize_t recovery_store(struct device *dev,
> >>  {
> >>  	struct rproc *rproc = to_rproc(dev);
> >>  
> >> +	/* restrict sysfs operations if not allowed by remoteproc drivers */
> >> +	if (rproc->deny_sysfs_ops)
> >> +		return -EPERM;
> >> +
> >>  	if (sysfs_streq(buf, "enabled")) {
> >>  		/* change the flag and begin the recovery process if needed */
> >>  		rproc->recovery_disabled = false;
> >> @@ -158,6 +162,10 @@ static ssize_t firmware_store(struct device *dev,
> >>  	char *p;
> >>  	int err, len = count;
> >>  
> >> +	/* restrict sysfs operations if not allowed by remoteproc drivers */
> >> +	if (rproc->deny_sysfs_ops)
> >> +		return -EPERM;
> >> +
> >>  	err = mutex_lock_interruptible(&rproc->lock);
> >>  	if (err) {
> >>  		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, err);
> >> @@ -225,6 +233,10 @@ static ssize_t state_store(struct device *dev,
> >>  	struct rproc *rproc = to_rproc(dev);
> >>  	int ret = 0;
> >>  
> >> +	/* restrict sysfs operations if not allowed by remoteproc drivers */
> >> +	if (rproc->deny_sysfs_ops)
> >> +		return -EPERM;
> >> +
> >>  	if (sysfs_streq(buf, "start")) {
> >>  		if (rproc->state == RPROC_RUNNING)
> >>  			return -EBUSY;
> >> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> >> index 3fa3ba6498e8..dbc3767f7d0e 100644
> >> --- a/include/linux/remoteproc.h
> >> +++ b/include/linux/remoteproc.h
> >> @@ -508,6 +508,7 @@ struct rproc_dump_segment {
> >>   * @has_iommu: flag to indicate if remote processor is behind an MMU
> >>   * @auto_boot: flag to indicate if remote processor should be auto-started
> >>   * @autonomous: true if an external entity has booted the remote processor
> >> + * @deny_sysfs_ops: flag to not permit sysfs operations on state, firmware and recovery
> >>   * @dump_segments: list of segments in the firmware
> >>   * @nb_vdev: number of vdev currently handled by rproc
> >>   * @char_dev: character device of the rproc
> >> @@ -545,6 +546,7 @@ struct rproc {
> >>  	bool has_iommu;
> >>  	bool auto_boot;
> >>  	bool autonomous;
> >> +	bool deny_sysfs_ops;
> >>  	struct list_head dump_segments;
> >>  	int nb_vdev;
> >>  	u8 elf_class;
> >> -- 
> >> 2.28.0
> >>
> 

Re: [PATCH v2 2/3] remoteproc: Introduce deny_sysfs_ops flag

[Suman Anna]

On 11/21/20 11:33 PM, Bjorn Andersson wrote:
> On Fri 20 Nov 21:44 CST 2020, Suman Anna wrote:
> 
>> On 11/20/20 9:38 PM, Bjorn Andersson wrote:
>>> On Fri 20 Nov 21:01 CST 2020, Suman Anna wrote:
>>>
>>>> The remoteproc framework provides sysfs interfaces for changing
>>>> the firmware name and for starting/stopping a remote processor
>>>> through the sysfs files 'state' and 'firmware'. The 'recovery'
>>>> sysfs file can also be used similarly to control the error recovery
>>>> state machine of a remoteproc. These interfaces are currently
>>>> allowed irrespective of how the remoteprocs were booted (like
>>>> remoteproc self auto-boot, remoteproc client-driven boot etc).
>>>> These interfaces can adversely affect a remoteproc and its clients
>>>> especially when a remoteproc is being controlled by a remoteproc
>>>> client driver(s). Also, not all remoteproc drivers may want to
>>>> support the sysfs interfaces by default.
>>>>
>>>> Add support to deny the sysfs state/firmware/recovery change by
>>>> introducing a state flag 'deny_sysfs_ops' that the individual
>>>> remoteproc drivers can set based on their usage needs. The default
>>>> behavior is to allow the sysfs operations as before.
>>>>
>>>
>>> This makes sense, but can't we implement attribute_group->is_visible to
>>> simply hide these entries from userspace instead of leaving them
>>> "broken"?
>>
>> I would have to look into that, but can that be changed dynamically?
>> Also, note that the enforcement is only on the writes/stores which impact
>> the state-machine, but not the reads/shows.
>>
>> For PRU usecases, we will be setting this dynamically.
>>
> 
> It looks to be dynamic, but I don't know if there's any "caching"
> involved. Please have a look and let me know.

OK, will do. I can only check the week after though.

regards
Suman

> 
> Regards,
> Bjorn
> 
>> regards
>> Suman
>>
>>>
>>> Regards,
>>> Bjorn
>>>
>>>> Signed-off-by: Suman Anna <s-anna@ti.com>
>>>> ---
>>>> v2: revised to account for the 'recovery' sysfs file as well, patch
>>>>     description updated accordingly
>>>> v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/20180915003725.17549-5-s-anna@ti.com/
>>>>
>>>>  drivers/remoteproc/remoteproc_sysfs.c | 12 ++++++++++++
>>>>  include/linux/remoteproc.h            |  2 ++
>>>>  2 files changed, 14 insertions(+)
>>>>
>>>> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
>>>> index bd2950a246c9..3fd18a71c188 100644
>>>> --- a/drivers/remoteproc/remoteproc_sysfs.c
>>>> +++ b/drivers/remoteproc/remoteproc_sysfs.c
>>>> @@ -49,6 +49,10 @@ static ssize_t recovery_store(struct device *dev,
>>>>  {
>>>>  	struct rproc *rproc = to_rproc(dev);
>>>>  
>>>> +	/* restrict sysfs operations if not allowed by remoteproc drivers */
>>>> +	if (rproc->deny_sysfs_ops)
>>>> +		return -EPERM;
>>>> +
>>>>  	if (sysfs_streq(buf, "enabled")) {
>>>>  		/* change the flag and begin the recovery process if needed */
>>>>  		rproc->recovery_disabled = false;
>>>> @@ -158,6 +162,10 @@ static ssize_t firmware_store(struct device *dev,
>>>>  	char *p;
>>>>  	int err, len = count;
>>>>  
>>>> +	/* restrict sysfs operations if not allowed by remoteproc drivers */
>>>> +	if (rproc->deny_sysfs_ops)
>>>> +		return -EPERM;
>>>> +
>>>>  	err = mutex_lock_interruptible(&rproc->lock);
>>>>  	if (err) {
>>>>  		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, err);
>>>> @@ -225,6 +233,10 @@ static ssize_t state_store(struct device *dev,
>>>>  	struct rproc *rproc = to_rproc(dev);
>>>>  	int ret = 0;
>>>>  
>>>> +	/* restrict sysfs operations if not allowed by remoteproc drivers */
>>>> +	if (rproc->deny_sysfs_ops)
>>>> +		return -EPERM;
>>>> +
>>>>  	if (sysfs_streq(buf, "start")) {
>>>>  		if (rproc->state == RPROC_RUNNING)
>>>>  			return -EBUSY;
>>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>>>> index 3fa3ba6498e8..dbc3767f7d0e 100644
>>>> --- a/include/linux/remoteproc.h
>>>> +++ b/include/linux/remoteproc.h
>>>> @@ -508,6 +508,7 @@ struct rproc_dump_segment {
>>>>   * @has_iommu: flag to indicate if remote processor is behind an MMU
>>>>   * @auto_boot: flag to indicate if remote processor should be auto-started
>>>>   * @autonomous: true if an external entity has booted the remote processor
>>>> + * @deny_sysfs_ops: flag to not permit sysfs operations on state, firmware and recovery
>>>>   * @dump_segments: list of segments in the firmware
>>>>   * @nb_vdev: number of vdev currently handled by rproc
>>>>   * @char_dev: character device of the rproc
>>>> @@ -545,6 +546,7 @@ struct rproc {
>>>>  	bool has_iommu;
>>>>  	bool auto_boot;
>>>>  	bool autonomous;
>>>> +	bool deny_sysfs_ops;
>>>>  	struct list_head dump_segments;
>>>>  	int nb_vdev;
>>>>  	u8 elf_class;
>>>> -- 
>>>> 2.28.0
>>>>
>>

[PATCH v2 3/3] remoteproc: wkup_m3: Set deny_sysfs_ops flag

[Suman Anna]

The Wakeup M3 remote processor is controlled by the wkup_m3_ipc
client driver, so set the newly introduced 'deny_sysfs_ops' flag
to not allow any overriding of the remoteproc firmware or state
from userspace.

Signed-off-by: Suman Anna <s-anna@ti.com>
---
v2: rebased version, no code changes, patch title adjusted slightly
v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/20180915003725.17549-6-s-anna@ti.com/

 drivers/remoteproc/wkup_m3_rproc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/remoteproc/wkup_m3_rproc.c b/drivers/remoteproc/wkup_m3_rproc.c
index b9349d684258..d92d7f32ba8d 100644
--- a/drivers/remoteproc/wkup_m3_rproc.c
+++ b/drivers/remoteproc/wkup_m3_rproc.c
@@ -160,6 +160,7 @@ static int wkup_m3_rproc_probe(struct platform_device *pdev)
 	}
 
 	rproc->auto_boot = false;
+	rproc->deny_sysfs_ops = true;
 
 	wkupm3 = rproc->priv;
 	wkupm3->rproc = rproc;
-- 
2.28.0

Re: [PATCH v2 3/3] remoteproc: wkup_m3: Set deny_sysfs_ops flag

[Bjorn Andersson]

On Fri 20 Nov 21:01 CST 2020, Suman Anna wrote:

> The Wakeup M3 remote processor is controlled by the wkup_m3_ipc
> client driver, so set the newly introduced 'deny_sysfs_ops' flag
> to not allow any overriding of the remoteproc firmware or state
> from userspace.
> 

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

> Signed-off-by: Suman Anna <s-anna@ti.com>
> ---
> v2: rebased version, no code changes, patch title adjusted slightly
> v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/20180915003725.17549-6-s-anna@ti.com/
> 
>  drivers/remoteproc/wkup_m3_rproc.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/remoteproc/wkup_m3_rproc.c b/drivers/remoteproc/wkup_m3_rproc.c
> index b9349d684258..d92d7f32ba8d 100644
> --- a/drivers/remoteproc/wkup_m3_rproc.c
> +++ b/drivers/remoteproc/wkup_m3_rproc.c
> @@ -160,6 +160,7 @@ static int wkup_m3_rproc_probe(struct platform_device *pdev)
>  	}
>  
>  	rproc->auto_boot = false;
> +	rproc->deny_sysfs_ops = true;
>  
>  	wkupm3 = rproc->priv;
>  	wkupm3->rproc = rproc;
> -- 
> 2.28.0
> 

Re: [PATCH v2 0/3] remoteproc sysfs fixes/improvements

[Christian Gmeiner]

Hi all.

Am Sa., 21. Nov. 2020 um 04:05 Uhr schrieb Suman Anna <s-anna@ti.com>:
>
> Hi All,
>
> This is a refresh of the unaccepted patches from an old series [1].
> Patches 2 and 3 from that series were merged and these are rebased and
> revised versions of the same patches. I had forgotten about these patches,
> and am resurrecting these again. Patches are on top of latest 5.10-rc4.
>
> The features being introduced here will be needed by the recently posted PRU
> remoteproc driver [2] in addition to the existing Wkup M3 remoteproc driver.
> Both of these drivers follow a client-driven boot methodology, with the latter
> strictly booted by another driver in kernel. The PRU remoteproc driver will be
> supporting both in-kernel clients as well as control from userspace orthogonally.
> The logic though is applicable and useful to any remoteproc driver not using
> 'auto-boot' and using an external driver/application to boot the remoteproc.
>
> regards
> Suman
>
> [1] https://patchwork.kernel.org/project/linux-remoteproc/cover/20180915003725.17549-1-s-anna@ti.com/
> [2] https://patchwork.kernel.org/project/linux-remoteproc/cover/20201119140850.12268-1-grzegorz.jaszczyk@linaro.org/
>
> Suman Anna (3):
>   remoteproc: Fix unbalanced boot with sysfs for no auto-boot rprocs
>   remoteproc: Introduce deny_sysfs_ops flag
>   remoteproc: wkup_m3: Set deny_sysfs_ops flag
>
>  drivers/remoteproc/remoteproc_sysfs.c | 28 ++++++++++++++++++++++++++-
>  drivers/remoteproc/wkup_m3_rproc.c    |  1 +
>  include/linux/remoteproc.h            |  2 ++
>  3 files changed, 30 insertions(+), 1 deletion(-)
>
> --
> 2.28.0
>

Is there any update on this patch series?

-- 
greets
--
Christian Gmeiner, MSc

https://christian-gmeiner.info/privacypolicy