Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Fri, 25 Mar 2022 11:22:33 -0600
From:   Mathieu Poirier <mathieu.poirier@linaro.org>
To:     "S.J. Wang" <shengjiu.wang@nxp.com>
Cc:     "bjorn.andersson@linaro.org" <bjorn.andersson@linaro.org>,
        "linux-remoteproc@vger.kernel.org" <linux-remoteproc@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "shengjiu.wang@gmail.com" <shengjiu.wang@gmail.com>
Subject: Re: [PATCH] remoteproc: core: Remove state checking before changing
 state
Message-ID: <20220325172233.GB3576184@p14s>
References: <PAXPR04MB9089EC1B959187335D00953BE31A9@PAXPR04MB9089.eurprd04.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <PAXPR04MB9089EC1B959187335D00953BE31A9@PAXPR04MB9089.eurprd04.prod.outlook.com>
Precedence: bulk

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 <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.

Thanks,
Mathieu

> 
> 
> Best regards
> Wang Shengjiu