[PATCH v2 0/2] remoteproc: core: fix issue for multi-instance

[PATCH v2 0/2] remoteproc: core: fix issue for multi-instance

[Shengjiu Wang]

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

[PATCH v2 1/2] remoteproc: core: Remove state checking before calling rproc_boot()

[Shengjiu Wang]

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 <shengjiu.wang@nxp.com>
---
 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

[PATCH v2 2/2] remoteproc: core: Move state checking to remoteproc_core

[Shengjiu Wang]

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 <shengjiu.wang@nxp.com>
---
 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

Re: [PATCH v2 0/2] remoteproc: core: fix issue for multi-instance

[Mathieu Poirier]

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
>