There is no mutex protecting of these state checking, which
can't garantee there is no another instance is trying to do
same operation.
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(), just let reference counter rproc->power to
manage the behaviors.
Signed-off-by: Shengjiu Wang <[email protected]>
---
drivers/remoteproc/remoteproc_cdev.c | 11 -----------
drivers/remoteproc/remoteproc_sysfs.c | 11 -----------
2 files changed, 22 deletions(-)
diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
index 906ff3c4dfdd..687f205fd70a 100644
--- a/drivers/remoteproc/remoteproc_cdev.c
+++ b/drivers/remoteproc/remoteproc_cdev.c
@@ -32,21 +32,10 @@ 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 &&
- 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_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
index 51a04bc6ba7a..8c7ea8922638 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -194,23 +194,12 @@ 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);
} 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 Wed, 23 Mar 2022 at 03:42, Shengjiu Wang <[email protected]> wrote:
>
> There is no mutex protecting of these state checking, which
> can't garantee there is no another instance is trying to do
> same operation.
>
> 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(), just let reference counter rproc->power to
> manage the behaviors.
>
> Signed-off-by: Shengjiu Wang <[email protected]>
> ---
> drivers/remoteproc/remoteproc_cdev.c | 11 -----------
> drivers/remoteproc/remoteproc_sysfs.c | 11 -----------
> 2 files changed, 22 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
> index 906ff3c4dfdd..687f205fd70a 100644
> --- a/drivers/remoteproc/remoteproc_cdev.c
> +++ b/drivers/remoteproc/remoteproc_cdev.c
> @@ -32,21 +32,10 @@ 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 &&
> - 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_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> index 51a04bc6ba7a..8c7ea8922638 100644
> --- a/drivers/remoteproc/remoteproc_sysfs.c
> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> @@ -194,23 +194,12 @@ 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;
> -
As per my previous comment the above conditions need to be moved into
rproc_boot().
> ret = rproc_boot(rproc);
> 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;
> -
This patch should have been part of a patch series with your other
work sent on March 18th[1].
Thanks,
Mathieu
[1]. [PATCH] remoteproc: core: check rproc->power value before decreasing it
> ret = rproc_detach(rproc);
> } else {
> dev_err(&rproc->dev, "Unrecognised option: %s\n", buf);
> --
> 2.17.1
>
Hi
> >
> > There is no mutex protecting of these state checking, which can't
> > garantee there is no another instance is trying to do same operation.
> >
> > 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(),
> > just let reference counter rproc->power to manage the behaviors.
> >
> > Signed-off-by: Shengjiu Wang <[email protected]>
> > ---
> > drivers/remoteproc/remoteproc_cdev.c | 11 -----------
> > drivers/remoteproc/remoteproc_sysfs.c | 11 -----------
> > 2 files changed, 22 deletions(-)
> >
> > diff --git a/drivers/remoteproc/remoteproc_cdev.c
> > b/drivers/remoteproc/remoteproc_cdev.c
> > index 906ff3c4dfdd..687f205fd70a 100644
> > --- a/drivers/remoteproc/remoteproc_cdev.c
> > +++ b/drivers/remoteproc/remoteproc_cdev.c
> > @@ -32,21 +32,10 @@ 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 &&
> > - 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_sysfs.c
> > b/drivers/remoteproc/remoteproc_sysfs.c
> > index 51a04bc6ba7a..8c7ea8922638 100644
> > --- a/drivers/remoteproc/remoteproc_sysfs.c
> > +++ b/drivers/remoteproc/remoteproc_sysfs.c
> > @@ -194,23 +194,12 @@ 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;
> > -
>
> As per my previous comment the above conditions need to be moved into
> rproc_boot().
>
> > ret = rproc_boot(rproc);
> > 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;
> > -
>
> This patch should have been part of a patch series with your other work sent
> on March 18th[1].
>
> Thanks,
> Mathieu
>
> [1]. [PATCH] remoteproc: core: check rproc->power value before decreasing
> it
>
Thanks for the comments.
I still have one question, if there are two instances independently to 'start'
'stop' remoteproc, for example:
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
When instance2 'stop' the remoteproc, then instance1 will be impacted for
It still needs the service from remoteproc.
That's why I just removed of the checking state, didn't move them to
rproc_boot()/rproc_shutdown()/rproc_detach(). And in order to utilize
the reference counter (rproc->power) to handle the multi-instance case.
Best regards
Wang Shengjiu
On Fri, Mar 25, 2022 at 03:44:20AM +0000, S.J. Wang wrote:
> Hi
>
> > >
> > > There is no mutex protecting of these state checking, which can't
> > > garantee there is no another instance is trying to do same operation.
> > >
> > > 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(),
> > > just let reference counter rproc->power to manage the behaviors.
> > >
> > > Signed-off-by: Shengjiu Wang <[email protected]>
> > > ---
> > > drivers/remoteproc/remoteproc_cdev.c | 11 -----------
> > > drivers/remoteproc/remoteproc_sysfs.c | 11 -----------
> > > 2 files changed, 22 deletions(-)
> > >
> > > diff --git a/drivers/remoteproc/remoteproc_cdev.c
> > > b/drivers/remoteproc/remoteproc_cdev.c
> > > index 906ff3c4dfdd..687f205fd70a 100644
> > > --- a/drivers/remoteproc/remoteproc_cdev.c
> > > +++ b/drivers/remoteproc/remoteproc_cdev.c
> > > @@ -32,21 +32,10 @@ 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 &&
> > > - 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_sysfs.c
> > > b/drivers/remoteproc/remoteproc_sysfs.c
> > > index 51a04bc6ba7a..8c7ea8922638 100644
> > > --- a/drivers/remoteproc/remoteproc_sysfs.c
> > > +++ b/drivers/remoteproc/remoteproc_sysfs.c
> > > @@ -194,23 +194,12 @@ 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;
> > > -
> >
> > As per my previous comment the above conditions need to be moved into
> > rproc_boot().
> >
> > > ret = rproc_boot(rproc);
> > > 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;
> > > -
> >
> > This patch should have been part of a patch series with your other work sent
> > on March 18th[1].
> >
> > Thanks,
> > Mathieu
> >
> > [1]. [PATCH] remoteproc: core: check rproc->power value before decreasing
> > it
> >
>
> Thanks for the comments.
> I still have one question, if there are two instances independently to 'start'
> 'stop' remoteproc, for example:
>
> 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
>
> When instance2 'stop' the remoteproc, then instance1 will be impacted for
> It still needs the service from remoteproc.
>
> That's why I just removed of the checking state, didn't move them to
> rproc_boot()/rproc_shutdown()/rproc_detach(). And in order to utilize
> the reference counter (rproc->power) to handle the multi-instance case.
Thanks for the exta information, now I understand the problem. The above should
be part of the changelog.
There are two problems here:
1) Dealing with race conditions when checking the state of a remote processor.
2) Properly dealing with the remote processor's reference count.
For the first one, state checks control the remote processor state machine and
can't simply be removed. They have to be brought inside the mutex lock in order
to avoid race conditions when multiple users interact with the remote processor.
For the second one, moving the rproc->state checks inside the mutex lock in
rproc_shutdown() and rproc_detach() will work with the rproc->power check.
The problem is with rproc_boot(). For (at least) two years now we have lost
the capability to increase the rproc->power reference count from sysfs and the
cdev interface due to the rproc-state checks in state_store() and
rproc_cdev_write(). I think that should be corrected but it will introduce a
user space visible change, which needs to be treated carefully.
With the above in mind, please send a patchset that includes one patch that
removes the rproc->state checks from the "start" command in state_store() and
rproc_cdev_write(). The other patch should move the rproc->state checks for the
"stop" and "detach" command inside rproc_shutdown() and rproc_detach().
With that we can start a discussion on the best way to proceed.
Thanks,
Mathieu
>
>
> Best regards
> Wang Shengjiu
Hi
> > > >
> > > > There is no mutex protecting of these state checking, which can't
> > > > garantee there is no another instance is trying to do same operation.
> > > >
> > > > 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(), just let reference counter rproc->power to manage the
> behaviors.
> > > >
> > > > Signed-off-by: Shengjiu Wang <[email protected]>
> > > > ---
> > > > drivers/remoteproc/remoteproc_cdev.c | 11 -----------
> > > > drivers/remoteproc/remoteproc_sysfs.c | 11 -----------
> > > > 2 files changed, 22 deletions(-)
> > > >
> > > > diff --git a/drivers/remoteproc/remoteproc_cdev.c
> > > > b/drivers/remoteproc/remoteproc_cdev.c
> > > > index 906ff3c4dfdd..687f205fd70a 100644
> > > > --- a/drivers/remoteproc/remoteproc_cdev.c
> > > > +++ b/drivers/remoteproc/remoteproc_cdev.c
> > > > @@ -32,21 +32,10 @@ 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 &&
> > > > - 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_sysfs.c
> > > > b/drivers/remoteproc/remoteproc_sysfs.c
> > > > index 51a04bc6ba7a..8c7ea8922638 100644
> > > > --- a/drivers/remoteproc/remoteproc_sysfs.c
> > > > +++ b/drivers/remoteproc/remoteproc_sysfs.c
> > > > @@ -194,23 +194,12 @@ 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;
> > > > -
> > >
> > > As per my previous comment the above conditions need to be moved
> > > into rproc_boot().
> > >
> > > > ret = rproc_boot(rproc);
> > > > 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;
> > > > -
> > >
> > > This patch should have been part of a patch series with your other
> > > work sent on March 18th[1].
> > >
> > > Thanks,
> > > Mathieu
> > >
> > > [1]. [PATCH] remoteproc: core: check rproc->power value before
> > > decreasing it
> > >
> >
> > Thanks for the comments.
> > I still have one question, if there are two instances independently to 'start'
> > 'stop' remoteproc, for example:
> >
> > 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
> >
> > When instance2 'stop' the remoteproc, then instance1 will be impacted
> > for It still needs the service from remoteproc.
> >
> > That's why I just removed of the checking state, didn't move them to
> > rproc_boot()/rproc_shutdown()/rproc_detach(). And in order to utilize
> > the reference counter (rproc->power) to handle the multi-instance case.
>
> Thanks for the exta information, now I understand the problem. The above
> should be part of the changelog.
>
> There are two problems here:
>
> 1) Dealing with race conditions when checking the state of a remote
> processor.
> 2) Properly dealing with the remote processor's reference count.
>
> For the first one, state checks control the remote processor state machine
> and can't simply be removed. They have to be brought inside the mutex lock
> in order to avoid race conditions when multiple users interact with the
> remote processor.
>
> For the second one, moving the rproc->state checks inside the mutex lock in
> rproc_shutdown() and rproc_detach() will work with the rproc->power check.
>
> The problem is with rproc_boot(). For (at least) two years now we have lost
> the capability to increase the rproc->power reference count from sysfs and
> the cdev interface due to the rproc-state checks in state_store() and
> rproc_cdev_write(). I think that should be corrected but it will introduce a
> user space visible change, which needs to be treated carefully.
>
> With the above in mind, please send a patchset that includes one patch that
> removes the rproc->state checks from the "start" command in state_store()
> and rproc_cdev_write(). The other patch should move the rproc->state
> checks for the "stop" and "detach" command inside rproc_shutdown() and
> rproc_detach().
>
> With that we can start a discussion on the best way to proceed.
>
Ok, thanks, will send the patches.
Best regards
Wang Shengjiu