linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH] remoteproc: core: Remove state checking before changing state
@ 2022-03-25  3:44 S.J. Wang
  2022-03-25 17:22 ` Mathieu Poirier
  0 siblings, 1 reply; 5+ messages in thread
From: S.J. Wang @ 2022-03-25  3:44 UTC (permalink / raw)
  To: Mathieu Poirier
  Cc: bjorn.andersson, linux-remoteproc, linux-kernel, shengjiu.wang

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

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] remoteproc: core: Remove state checking before changing state
@ 2022-03-28  2:00 S.J. Wang
  0 siblings, 0 replies; 5+ messages in thread
From: S.J. Wang @ 2022-03-28  2:00 UTC (permalink / raw)
  To: Mathieu Poirier
  Cc: bjorn.andersson, linux-remoteproc, linux-kernel, shengjiu.wang

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

^ permalink raw reply	[flat|nested] 5+ messages in thread
* [PATCH] remoteproc: core: Remove state checking before changing state
@ 2022-03-23  9:32 Shengjiu Wang
  2022-03-24 16:53 ` Mathieu Poirier
  0 siblings, 1 reply; 5+ messages in thread
From: Shengjiu Wang @ 2022-03-23  9:32 UTC (permalink / raw)
  To: bjorn.andersson, mathieu.poirier
  Cc: linux-remoteproc, linux-kernel, shengjiu.wang

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


^ permalink raw reply related	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2022-03-28  2:00 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-25  3:44 [PATCH] remoteproc: core: Remove state checking before changing state S.J. Wang
2022-03-25 17:22 ` Mathieu Poirier
  -- strict thread matches above, loose matches on Subject: below --
2022-03-28  2:00 S.J. Wang
2022-03-23  9:32 Shengjiu Wang
2022-03-24 16:53 ` Mathieu Poirier

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).