Re: [PATCH v6 16/16] remoteproc: Refactor rproc delete and cdev release path

[Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>]

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