Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 1 Mar 2021 11:58:16 -0700
From:   Mathieu Poirier <mathieu.poirier@linaro.org>
To:     Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>
Cc:     ohad@wizery.com, bjorn.andersson@linaro.org,
        arnaud.pouliquen@st.com, mcoquelin.stm32@gmail.com,
        alexandre.torgue@st.com, linux-remoteproc@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v6 16/16] remoteproc: Refactor rproc delete and cdev
 release path
Message-ID: <20210301185816.GC3690389@xps15>
References: <20210223233515.3468677-1-mathieu.poirier@linaro.org>
 <20210223233515.3468677-17-mathieu.poirier@linaro.org>
 <80abdd3b-ffb0-1019-2a1f-fea4f7b51349@foss.st.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <80abdd3b-ffb0-1019-2a1f-fea4f7b51349@foss.st.com>
Precedence: bulk

On Fri, Feb 26, 2021 at 05:23:45PM +0100, Arnaud POULIQUEN wrote:
> 
> 
> On 2/24/21 12:35 AM, Mathieu Poirier wrote:
> > Refactor function rproc_del() and rproc_cdev_release() to take
> > into account the current state of the remote processor when choosing
> > the state to transition to.
> > 
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > ---
> > New for V6:
> > - The RPROC_RUNNING -> RPROC_DETACHED transition is no longer permitted.
> >   to avoid dealing with complex resource table management problems.
> > - Transition to the next state is no longer dictated by a DT binding for
> >   the same reason as above.
> > - Removed Peng and Arnaud's RB tags because of the above.
> > ---
> > 
> >  drivers/remoteproc/remoteproc_cdev.c | 10 ++++++++--
> >  drivers/remoteproc/remoteproc_core.c |  9 +++++++--
> >  2 files changed, 15 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
> > index 2db494816d5f..0b8a84c04f76 100644
> > --- a/drivers/remoteproc/remoteproc_cdev.c
> > +++ b/drivers/remoteproc/remoteproc_cdev.c
> > @@ -86,11 +86,17 @@ static long rproc_device_ioctl(struct file *filp, unsigned int ioctl, unsigned l
> >  static int rproc_cdev_release(struct inode *inode, struct file *filp)
> >  {
> >  	struct rproc *rproc = container_of(inode->i_cdev, struct rproc, cdev);
> > +	int ret = 0;
> > +
> > +	if (!rproc->cdev_put_on_release)
> > +		return 0;
> >  
> > -	if (rproc->cdev_put_on_release && rproc->state == RPROC_RUNNING)
> > +	if (rproc->state == RPROC_RUNNING)
> >  		rproc_shutdown(rproc);
> > +	else if (rproc->state == RPROC_ATTACHED)
> > +		ret = rproc_detach(rproc);
> >  
> > -	return 0;
> > +	return ret;
> >  }
> >  
> >  static const struct file_operations rproc_fops = {
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index 00452da25fba..a05d5fec43b1 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -2542,11 +2542,16 @@ EXPORT_SYMBOL(rproc_put);
> >   */
> >  int rproc_del(struct rproc *rproc)
> >  {
> > +	int ret = 0;
> > +
> >  	if (!rproc)
> >  		return -EINVAL;
> >  
> >  	/* TODO: make sure this works with rproc->power > 1 */
> > -	rproc_shutdown(rproc);
> > +	if (rproc->state == RPROC_RUNNING)
> > +		rproc_shutdown(rproc);
> > +	else if (rproc->state == RPROC_ATTACHED)
> > +		ret = rproc_detach(rproc);
> 
> Here i would not update the code to not change the existing behavior of an
> attached firmware.

Upon reflection your assessment is correct.  This is an unintended
consequence of separating the attach and detach funtionality in two patchset.
Fortunately it is easily fixed by calling rproc_detach() before rproc_del() in
the platform driver, or using the DT.

That being said we can't do much for rproc_cdev_release(), otherwise systems
that only support attach/detach functionality would be broken.

> The decision between a detach or a shutdown probably depends on platform.
> We could (as a next step) reintroduce the "autonomous-on-core-reboot" DT
> property for the decision.
> 
> Regards
> Arnaud
> 
> >  
> >  	mutex_lock(&rproc->lock);
> >  	rproc->state = RPROC_DELETED;
> > @@ -2565,7 +2570,7 @@ int rproc_del(struct rproc *rproc)
> >  
> >  	device_del(&rproc->dev);
> >  
> > -	return 0;
> > +	return ret;
> >  }
> >  EXPORT_SYMBOL(rproc_del);
> >  
> >