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/[email protected]/
[2] https://patchwork.kernel.org/project/linux-remoteproc/cover/[email protected]/
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
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 <[email protected]>
Acked-by: Arnaud Pouliquen <[email protected]>
---
v2: rebased version, no changes
v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/[email protected]/
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
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 <[email protected]>
---
v2: revised to account for the 'recovery' sysfs file as well, patch
description updated accordingly
v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/[email protected]/
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
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 <[email protected]>
---
v2: rebased version, no code changes, patch title adjusted slightly
v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/[email protected]/
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
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 <[email protected]>
> ---
> v2: revised to account for the 'recovery' sysfs file as well, patch
> description updated accordingly
> v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/[email protected]/
>
> 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
>
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 <[email protected]>
> Signed-off-by: Suman Anna <[email protected]>
> ---
> v2: rebased version, no code changes, patch title adjusted slightly
> v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/[email protected]/
>
> 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
>
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 <[email protected]>
>> ---
>> v2: revised to account for the 'recovery' sysfs file as well, patch
>> description updated accordingly
>> v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/[email protected]/
>>
>> 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
>>
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 <[email protected]>
> >> ---
> >> v2: revised to account for the 'recovery' sysfs file as well, patch
> >> description updated accordingly
> >> v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/[email protected]/
> >>
> >> 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
> >>
>
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 <[email protected]>
>>>> ---
>>>> v2: revised to account for the 'recovery' sysfs file as well, patch
>>>> description updated accordingly
>>>> v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/[email protected]/
>>>>
>>>> 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
>>>>
>>
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 <[email protected]>
> Acked-by: Arnaud Pouliquen <[email protected]>
> ---
> v2: rebased version, no changes
> v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/[email protected]/
>
> 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/[email protected]/
[2]. https://patchwork.kernel.org/project/linux-remoteproc/patch/[email protected]/
[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
>
Hi all.
Am Sa., 21. Nov. 2020 um 04:05 Uhr schrieb Suman Anna <[email protected]>:
>
> 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/[email protected]/
> [2] https://patchwork.kernel.org/project/linux-remoteproc/cover/[email protected]/
>
> 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