Re: [PATCH] remoteproc: core: check state in rproc_boot

[Peng Fan]

On 7/17/2022 12:07 PM, Bjorn Andersson wrote:
> On Thu 19 May 01:41 CDT 2022, Peng Fan (OSS) wrote:
>
> > From: Peng Fan <peng.fan@xxxxxxx>
> >
> > If remote processor has already been in RUNNING or ATTACHED
> > state, report it. Not just increment the power counter and return
> > success.
> >
> > Without this patch, if m7 is in RUNNING state, and start it again,
> > nothing output to console.
> > If wanna to stop the m7, we need write twice 'stop'.
> >
> > This patch is to improve that the 2nd start would show some useful
> > info.
> >
> > Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> > ---
> >
> > Not sure to keep power counter or not.
>
> I did discuss this with Mathieu, whom argued in favor of keeping the
> refcount mechanism.
>
> I can see that there could be a scenario where multiple user-space
> components keep the remotproc running while they are, and if there is
> any such user this ABI change would be a breakage.
>
> That said, it's more than once that I accidentally have bumped the
> refcount and then assumed that a single stop would tear down the
> remoteproc...
>
> >   drivers/remoteproc/remoteproc_core.c | 6 ++++++
> >   1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index 02a04ab34a23..f37e0758c096 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -2005,6 +2005,12 @@ int rproc_boot(struct rproc *rproc)
> >   		goto unlock_mutex;
> >   	}
> >   
> > +	if (rproc->state == RPROC_RUNNING || rproc->state == RPROC_ATTACHED) {
>
> If we were to do this would it make sense to boot it out of anything but
> RPROC_OFFLINE?

It is just a bit confused if started twice, need stop twice without any 
notice.Not a critical thing, we could leave it as is.

Thanks,
Peng.

> Regards,
> Bjorn
>
> > +		ret = -EINVAL;
> > +		dev_err(dev, "%s already booted\n", rproc->name);
> > +		goto unlock_mutex;
> > +	}
> > +
> >   	/* skip the boot or attach process if rproc is already powered up */
> >   	if (atomic_inc_return(&rproc->power) > 1) {
> >   		ret = 0;
> > --
> > 2.25.1