linux-kernel - Re: [PATCH 3/4] remoteproc: k3-r5: k3_r5_rproc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 28 Jun 2024 15:18:36 -0600
From: Mathieu Poirier <mathieu.poirier@...aro.org>
To: Richard Genoud <richard.genoud@...tlin.com>
Cc: Bjorn Andersson <andersson@...nel.org>,
	Philipp Zabel <p.zabel@...gutronix.de>, Suman Anna <s-anna@...com>,
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
	Alexandre Belloni <alexandre.belloni@...tlin.com>,
	Udit Kumar <u-kumar1@...com>,
	Thomas Richard <thomas.richard@...tlin.com>,
	Gregory CLEMENT <gregory.clement@...tlin.com>,
	Hari Nagalla <hnagalla@...com>,
	Théo Lebrun <theo.lebrun@...tlin.com>,
	linux-remoteproc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/4] remoteproc: k3-r5: k3_r5_rproc_stop: code reorder

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@...tlin.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.