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