[PATCH 1/2] riscv: cpu_ops: Change return value type of cpu_is_stopped() to bool

[PATCH 1/2] riscv: cpu_ops: Change return value type of cpu_is_stopped() to bool

[Hui Wang]

In the original sbi_cpu_is_stopped(), if rc doesn't equal to the
SBI_HSM_STATE_STOPPED, it will return rc to the caller directly. But
there is a hidden problem, the rc could be SBI_HSM_STATE_STARTED, if
so, this function will report cpu stopped while the cpu isn't really
stopped.

Furthermore, from the name of cpu_is_stopped(), it gives a sense the
return value is a bool type, true means the cpu is stopped, conversely
false means the cpu is not stopped.

Here change the return value type to bool and change the callers
accordingly. This could fix the above two issues.

Fixes: f1e58583b9c7c ("RISC-V: Support cpu hotplug")
Signed-off-by: Hui Wang <hui.wang@canonical.com>
---
 arch/riscv/include/asm/cpu_ops.h | 2 +-
 arch/riscv/kernel/cpu-hotplug.c  | 2 +-
 arch/riscv/kernel/cpu_ops_sbi.c  | 6 ++----
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/arch/riscv/include/asm/cpu_ops.h b/arch/riscv/include/asm/cpu_ops.h
index 176b570ef982..065811fca594 100644
--- a/arch/riscv/include/asm/cpu_ops.h
+++ b/arch/riscv/include/asm/cpu_ops.h
@@ -24,7 +24,7 @@ struct cpu_operations {
 				     struct task_struct *tidle);
 #ifdef CONFIG_HOTPLUG_CPU
 	void		(*cpu_stop)(void);
-	int		(*cpu_is_stopped)(unsigned int cpu);
+	bool		(*cpu_is_stopped)(unsigned int cpu);
 #endif
 };
 
diff --git a/arch/riscv/kernel/cpu-hotplug.c b/arch/riscv/kernel/cpu-hotplug.c
index a0ee426f6d93..c240e9be9fba 100644
--- a/arch/riscv/kernel/cpu-hotplug.c
+++ b/arch/riscv/kernel/cpu-hotplug.c
@@ -57,7 +57,7 @@ void arch_cpuhp_cleanup_dead_cpu(unsigned int cpu)
 	/* Verify from the firmware if the cpu is really stopped*/
 	if (cpu_ops->cpu_is_stopped)
 		ret = cpu_ops->cpu_is_stopped(cpu);
-	if (ret)
+	if (!ret)
 		pr_warn("CPU%u may not have stopped: %d\n", cpu, ret);
 }
 
diff --git a/arch/riscv/kernel/cpu_ops_sbi.c b/arch/riscv/kernel/cpu_ops_sbi.c
index 00aff669f5f2..a17b7576b77d 100644
--- a/arch/riscv/kernel/cpu_ops_sbi.c
+++ b/arch/riscv/kernel/cpu_ops_sbi.c
@@ -88,16 +88,14 @@ static void sbi_cpu_stop(void)
 	pr_crit("Unable to stop the cpu %d (%d)\n", smp_processor_id(), ret);
 }
 
-static int sbi_cpu_is_stopped(unsigned int cpuid)
+static bool sbi_cpu_is_stopped(unsigned int cpuid)
 {
 	int rc;
 	unsigned long hartid = cpuid_to_hartid_map(cpuid);
 
 	rc = sbi_hsm_hart_get_status(hartid);
 
-	if (rc == SBI_HSM_STATE_STOPPED)
-		return 0;
-	return rc;
+	return (rc == SBI_HSM_STATE_STOPPED);
 }
 #endif
 
-- 
2.43.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 2/2] riscv: cpu_ops_sbi: No need to be bothered to check ret.error

[Hui Wang]

If the ret.error equals to 0, the sbi_err_map_linux_errno() can also
handle it, i.e. if ret.error is SBI_SUCCESS, it will return 0
immediately, so no need to be bothered to check ret.error here.

Signed-off-by: Hui Wang <hui.wang@canonical.com>
---
 arch/riscv/kernel/cpu_ops_sbi.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/arch/riscv/kernel/cpu_ops_sbi.c b/arch/riscv/kernel/cpu_ops_sbi.c
index a17b7576b77d..07d5752a7b03 100644
--- a/arch/riscv/kernel/cpu_ops_sbi.c
+++ b/arch/riscv/kernel/cpu_ops_sbi.c
@@ -30,10 +30,8 @@ static int sbi_hsm_hart_start(unsigned long hartid, unsigned long saddr,
 
 	ret = sbi_ecall(SBI_EXT_HSM, SBI_EXT_HSM_HART_START,
 			hartid, saddr, priv, 0, 0, 0);
-	if (ret.error)
-		return sbi_err_map_linux_errno(ret.error);
-	else
-		return 0;
+
+	return sbi_err_map_linux_errno(ret.error);
 }
 
 #ifdef CONFIG_HOTPLUG_CPU
@@ -43,10 +41,7 @@ static int sbi_hsm_hart_stop(void)
 
 	ret = sbi_ecall(SBI_EXT_HSM, SBI_EXT_HSM_HART_STOP, 0, 0, 0, 0, 0, 0);
 
-	if (ret.error)
-		return sbi_err_map_linux_errno(ret.error);
-	else
-		return 0;
+	return sbi_err_map_linux_errno(ret.error);
 }
 
 static int sbi_hsm_hart_get_status(unsigned long hartid)
-- 
2.43.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 1/2] riscv: cpu_ops: Change return value type of cpu_is_stopped() to bool

[Danil Skrebenkov]

> In the original sbi_cpu_is_stopped(), if rc doesn't equal to the
> SBI_HSM_STATE_STOPPED, it will return rc to the caller directly. But
> there is a hidden problem, the rc could be SBI_HSM_STATE_STARTED, if
> so, this function will report cpu stopped while the cpu isn't really
> stopped.
>
> Furthermore, from the name of cpu_is_stopped(), it gives a sense the
> return value is a bool type, true means the cpu is stopped, conversely
> false means the cpu is not stopped.
>
> Here change the return value type to bool and change the callers
> accordingly. This could fix the above two issues.
>
> Fixes: f1e58583b9c7c ("RISC-V: Support cpu hotplug")
> Signed-off-by: Hui Wang <hui.wang@canonical.com>
> ---
>   arch/riscv/include/asm/cpu_ops.h | 2 +-
>   arch/riscv/kernel/cpu-hotplug.c  | 2 +-
>   arch/riscv/kernel/cpu_ops_sbi.c  | 6 ++----
>   3 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/arch/riscv/include/asm/cpu_ops.h b/arch/riscv/include/asm/cpu_ops.h
> index 176b570ef982..065811fca594 100644
> --- a/arch/riscv/include/asm/cpu_ops.h
> +++ b/arch/riscv/include/asm/cpu_ops.h
> @@ -24,7 +24,7 @@ struct cpu_operations {
>   				     struct task_struct *tidle);
>   #ifdef CONFIG_HOTPLUG_CPU
>   	void		(*cpu_stop)(void);
> -	int		(*cpu_is_stopped)(unsigned int cpu);
> +	bool		(*cpu_is_stopped)(unsigned int cpu);
>   #endif
>   };
>   
> diff --git a/arch/riscv/kernel/cpu-hotplug.c b/arch/riscv/kernel/cpu-hotplug.c
> index a0ee426f6d93..c240e9be9fba 100644
> --- a/arch/riscv/kernel/cpu-hotplug.c
> +++ b/arch/riscv/kernel/cpu-hotplug.c
> @@ -57,7 +57,7 @@ void arch_cpuhp_cleanup_dead_cpu(unsigned int cpu)
>   	/* Verify from the firmware if the cpu is really stopped*/
>   	if (cpu_ops->cpu_is_stopped)
>   		ret = cpu_ops->cpu_is_stopped(cpu);
> -	if (ret)
> +	if (!ret)
>   		pr_warn("CPU%u may not have stopped: %d\n", cpu, ret);
>   }
>   
> diff --git a/arch/riscv/kernel/cpu_ops_sbi.c b/arch/riscv/kernel/cpu_ops_sbi.c
> index 00aff669f5f2..a17b7576b77d 100644
> --- a/arch/riscv/kernel/cpu_ops_sbi.c
> +++ b/arch/riscv/kernel/cpu_ops_sbi.c
> @@ -88,16 +88,14 @@ static void sbi_cpu_stop(void)
>   	pr_crit("Unable to stop the cpu %d (%d)\n", smp_processor_id(), ret);
>   }
>   
> -static int sbi_cpu_is_stopped(unsigned int cpuid)
> +static bool sbi_cpu_is_stopped(unsigned int cpuid)
>   {
>   	int rc;
>   	unsigned long hartid = cpuid_to_hartid_map(cpuid);
>   
>   	rc = sbi_hsm_hart_get_status(hartid);
>   
> -	if (rc == SBI_HSM_STATE_STOPPED)
> -		return 0;
> -	return rc;
> +	return (rc == SBI_HSM_STATE_STOPPED);
>   }
>   #endif
>   

Here is a some ambiguous behavior because pr_warn() in cpu-hotplug.c 
must send a cpu state number.
I would say it is needed to use sbi_hsm_hart_get_status(cpu) in pr_warn 
like this:

pr_warn("CPU%u may not have stopped: %d\n", cpu, 
sbi_hsm_hart_get_status(cpu))

so that we can define what is the current state of hart in case it is 
not SBI_HSM_STATE_STOPPED.


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv