Re: [PATCH 3/4] remoteproc: k3-r5: k3_r5_rproc_stop: code reorder

[Mathieu Poirier <mathieu.poirier@linaro.org>]

On Fri, Jun 21, 2024 at 05:00:57PM +0200, Richard Genoud wrote:
> In the next commit, a RP_MBOX_SHUTDOWN message will be sent in
> k3_r5_rproc_stop() to the remote proc (in lockstep on not)
> Thus, the sanity check "do not allow core 0 to stop before core 1"
> should be moved at the beginning of the function so that the generic case
> can be dealt with.
> 
> In order to have an easier patch to review, those actions are broke in
> two patches:
> - this patch: moving the sanity check at the beginning (No functional
>   change).
> - next patch: doing the real job (sending shutdown messages to remote
>   procs before halting them).
> 
> Basically, we had:
> - cluster_mode actions
> - !cluster_mode sanity check
> - !cluster_mode actions
> And now:
> - !cluster_mode sanity check
> - cluster_mode actions
> - !cluster_mode actions
> 
> Signed-off-by: Richard Genoud <richard.genoud@bootlin.com>
> ---
>  drivers/remoteproc/ti_k3_r5_remoteproc.c | 24 ++++++++++++++----------
>  1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> index 1f18b08618c8..a2ead87952c7 100644
> --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
> +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> @@ -636,16 +636,8 @@ static int k3_r5_rproc_stop(struct rproc *rproc)
>  	struct k3_r5_core *core1, *core = kproc->core;
>  	int ret;
>  
> -	/* halt all applicable cores */
> -	if (cluster->mode == CLUSTER_MODE_LOCKSTEP) {
> -		list_for_each_entry(core, &cluster->cores, elem) {
> -			ret = k3_r5_core_halt(core);
> -			if (ret) {
> -				core = list_prev_entry(core, elem);
> -				goto unroll_core_halt;
> -			}
> -		}
> -	} else {
> +
> +	if (cluster->mode != CLUSTER_MODE_LOCKSTEP) {
>  		/* do not allow core 0 to stop before core 1 */
>  		core1 = list_last_entry(&cluster->cores, struct k3_r5_core,
>  					elem);
> @@ -656,6 +648,18 @@ static int k3_r5_rproc_stop(struct rproc *rproc)
>  			ret = -EPERM;
>  			goto out;
>  		}
> +	}
> +
> +	/* halt all applicable cores */
> +	if (cluster->mode == CLUSTER_MODE_LOCKSTEP) {
> +		list_for_each_entry(core, &cluster->cores, elem) {
> +			ret = k3_r5_core_halt(core);
> +			if (ret) {
> +				core = list_prev_entry(core, elem);
> +				goto unroll_core_halt;
> +			}
> +		}
> +	} else {
>  
>  		ret = k3_r5_core_halt(core);
>  		if (ret)

With this patch, the "else" in this "if" condition is coupled with the "if" from
the lockstep mode, making the code extremaly hard to read.  The original code
has a k3_r5_core_halt() in both "if" conditions, making the condition
independent from one another.