Use the rproc->power reference count to handle the multi-instance
case, move the state checking for 'stop' and 'detach' under
mutex protection, and remove state checking for 'start'.
Shengjiu Wang (2):
remoteproc: core: Remove state checking before calling rproc_boot()
remoteproc: core: Move state checking to remoteproc_core
changes in v2:
- drop 'remoteproc: core: check rproc->power value before decreasing it'
- refine 'remoteproc: core: Remove state checking before changing state'
split it to two new patches.
drivers/remoteproc/remoteproc_cdev.c | 11 -----------
drivers/remoteproc/remoteproc_core.c | 11 +++++++++++
drivers/remoteproc/remoteproc_sysfs.c | 11 -----------
3 files changed, 11 insertions(+), 22 deletions(-)
--
2.17.1
There is no mutex protection of the state checking before rproc_boot(),
which can't guarantee there is no another instance is trying to do
same operation.
Consider two instances case:
Instance1: echo start > /sys/class/remoteproc/remoteproc0/state
Instance2: echo start > /sys/class/remoteproc/remoteproc0/state
...
Instance2: echo stop > /sys/class/remoteproc/remoteproc0/state
...
Instance1: echo stop > /sys/class/remoteproc/remoteproc0/state
The one issue is that the instance2 case may success when 'start'
happens at same time as instance1, then rproc->power = 2; Or it
may fail with -BUSY, then rproc->power = 1; which is uncertain.
The another issue is for 'stop' operation, if the rproc->power = 1,
when instance2 'stop' the remoteproc the instance1 will be
impacted for it still needs the service at that time.
The reference counter rproc->power is used to manage state
changing and there is mutex protection in each operation
function for multi instance case.
So remove this state checking in rproc_cdev_write() and
state_store() for 'start' operation, just let reference
counter rproc->power to manage the behaviors.
Signed-off-by: Shengjiu Wang <[email protected]>
---
drivers/remoteproc/remoteproc_cdev.c | 4 ----
drivers/remoteproc/remoteproc_sysfs.c | 4 ----
2 files changed, 8 deletions(-)
diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
index 906ff3c4dfdd..62001eda780c 100644
--- a/drivers/remoteproc/remoteproc_cdev.c
+++ b/drivers/remoteproc/remoteproc_cdev.c
@@ -32,10 +32,6 @@ static ssize_t rproc_cdev_write(struct file *filp, const char __user *buf, size_
return -EFAULT;
if (!strncmp(cmd, "start", len)) {
- if (rproc->state == RPROC_RUNNING ||
- rproc->state == RPROC_ATTACHED)
- return -EBUSY;
-
ret = rproc_boot(rproc);
} else if (!strncmp(cmd, "stop", len)) {
if (rproc->state != RPROC_RUNNING &&
diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
index 51a04bc6ba7a..ac64d69085ab 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -194,10 +194,6 @@ static ssize_t state_store(struct device *dev,
int ret = 0;
if (sysfs_streq(buf, "start")) {
- if (rproc->state == RPROC_RUNNING ||
- rproc->state == RPROC_ATTACHED)
- return -EBUSY;
-
ret = rproc_boot(rproc);
if (ret)
dev_err(&rproc->dev, "Boot failed: %d\n", ret);
--
2.17.1
There is no mutex protection of these state checking for 'stop'
and 'detach' which can't guarantee there is no another instance
is trying to do same operation.
Consider two instances case:
Instance1: echo stop > /sys/class/remoteproc/remoteproc0/state
Instance2: echo stop > /sys/class/remoteproc/remoteproc0/state
The issue is that the instance2 case may success, Or it
may fail with -EINVAL, which is uncertain.
So move this state checking in rproc_cdev_write() and
state_store() for 'stop', 'detach' operation to
'rproc_shutdown' , 'rproc_detach' function under the mutex
protection.
Signed-off-by: Shengjiu Wang <[email protected]>
---
drivers/remoteproc/remoteproc_cdev.c | 7 -------
drivers/remoteproc/remoteproc_core.c | 11 +++++++++++
drivers/remoteproc/remoteproc_sysfs.c | 7 -------
3 files changed, 11 insertions(+), 14 deletions(-)
diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
index 62001eda780c..687f205fd70a 100644
--- a/drivers/remoteproc/remoteproc_cdev.c
+++ b/drivers/remoteproc/remoteproc_cdev.c
@@ -34,15 +34,8 @@ static ssize_t rproc_cdev_write(struct file *filp, const char __user *buf, size_
if (!strncmp(cmd, "start", len)) {
ret = rproc_boot(rproc);
} else if (!strncmp(cmd, "stop", len)) {
- if (rproc->state != RPROC_RUNNING &&
- rproc->state != RPROC_ATTACHED)
- return -EINVAL;
-
ret = rproc_shutdown(rproc);
} else if (!strncmp(cmd, "detach", len)) {
- if (rproc->state != RPROC_ATTACHED)
- return -EINVAL;
-
ret = rproc_detach(rproc);
} else {
dev_err(&rproc->dev, "Unrecognized option\n");
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index c510125769b9..4b344d7271e3 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -2075,6 +2075,12 @@ int rproc_shutdown(struct rproc *rproc)
return ret;
}
+ if (rproc->state != RPROC_RUNNING &&
+ rproc->state != RPROC_ATTACHED) {
+ ret = -EINVAL;
+ goto out;
+ }
+
/* if the remote proc is still needed, bail out */
if (!atomic_dec_and_test(&rproc->power))
goto out;
@@ -2134,6 +2140,11 @@ int rproc_detach(struct rproc *rproc)
return ret;
}
+ if (rproc->state != RPROC_ATTACHED) {
+ ret = -EINVAL;
+ goto out;
+ }
+
/* if the remote proc is still needed, bail out */
if (!atomic_dec_and_test(&rproc->power)) {
ret = 0;
diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
index ac64d69085ab..8c7ea8922638 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -198,15 +198,8 @@ static ssize_t state_store(struct device *dev,
if (ret)
dev_err(&rproc->dev, "Boot failed: %d\n", ret);
} else if (sysfs_streq(buf, "stop")) {
- if (rproc->state != RPROC_RUNNING &&
- rproc->state != RPROC_ATTACHED)
- return -EINVAL;
-
ret = rproc_shutdown(rproc);
} else if (sysfs_streq(buf, "detach")) {
- if (rproc->state != RPROC_ATTACHED)
- return -EINVAL;
-
ret = rproc_detach(rproc);
} else {
dev_err(&rproc->dev, "Unrecognised option: %s\n", buf);
--
2.17.1
On Mon, Mar 28, 2022 at 10:20:10AM +0800, Shengjiu Wang wrote:
> Use the rproc->power reference count to handle the multi-instance
> case, move the state checking for 'stop' and 'detach' under
> mutex protection, and remove state checking for 'start'.
>
> Shengjiu Wang (2):
> remoteproc: core: Remove state checking before calling rproc_boot()
> remoteproc: core: Move state checking to remoteproc_core
>
> changes in v2:
> - drop 'remoteproc: core: check rproc->power value before decreasing it'
> - refine 'remoteproc: core: Remove state checking before changing state'
> split it to two new patches.
>
> drivers/remoteproc/remoteproc_cdev.c | 11 -----------
> drivers/remoteproc/remoteproc_core.c | 11 +++++++++++
> drivers/remoteproc/remoteproc_sysfs.c | 11 -----------
> 3 files changed, 11 insertions(+), 22 deletions(-)
>
I have applied this set.
Thanks,
Mathieu
> --
> 2.17.1
>