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;
Subject: Re: [PATCH v6 16/16] remoteproc: Refactor rproc delete and cdev
 release path
To:     Mathieu Poirier <mathieu.poirier@linaro.org>, <ohad@wizery.com>,
        <bjorn.andersson@linaro.org>, <arnaud.pouliquen@st.com>
CC:     <mcoquelin.stm32@gmail.com>, <alexandre.torgue@st.com>,
        <linux-remoteproc@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>
References: <20210223233515.3468677-1-mathieu.poirier@linaro.org>
 <20210223233515.3468677-17-mathieu.poirier@linaro.org>
From:   Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>
Message-ID: <80abdd3b-ffb0-1019-2a1f-fea4f7b51349@foss.st.com>
Date:   Fri, 26 Feb 2021 17:23:45 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <20210223233515.3468677-17-mathieu.poirier@linaro.org>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


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