diff mbox series

RISC-V: Align SBI probe implementation with spec

Message ID 20230426172254.70342-1-ajones@ventanamicro.com (mailing list archive)
State Superseded
Headers show
Series RISC-V: Align SBI probe implementation with spec | expand

Checks

Context Check Description
conchuod/cover_letter success Single patches do not need cover letters
conchuod/tree_selection success Guessed tree name to be for-next at HEAD b09313dd2e72
conchuod/fixes_present success Fixes tag not required for -next series
conchuod/maintainers_pattern success MAINTAINERS pattern errors before the patch: 1 and now 1
conchuod/verify_signedoff success Signed-off-by tag matches author and committer
conchuod/kdoc success Errors and warnings before: 2 this patch: 2
conchuod/build_rv64_clang_allmodconfig success Errors and warnings before: 18 this patch: 18
conchuod/module_param success Was 0 now: 0
conchuod/build_rv64_gcc_allmodconfig success Errors and warnings before: 35 this patch: 35
conchuod/build_rv32_defconfig success Build OK
conchuod/dtb_warn_rv64 success Errors and warnings before: 3 this patch: 3
conchuod/header_inline success No static functions without inline keyword in header files
conchuod/checkpatch success total: 0 errors, 0 warnings, 0 checks, 93 lines checked
conchuod/source_inline success Was 0 now: 0
conchuod/build_rv64_nommu_k210_defconfig success Build OK
conchuod/verify_fixes success No Fixes tag
conchuod/build_rv64_nommu_virt_defconfig success Build OK

Commit Message

Andrew Jones April 26, 2023, 5:22 p.m. UTC
sbi_probe_extension() is specified with "Returns 0 if the given SBI
extension ID (EID) is not available, or 1 if it is available unless
defined as any other non-zero value by the implementation."
Additionally, sbiret.value is a long. Fix the implementation to
ensure any nonzero long value is considered a success, rather
than only positive int values.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
 arch/riscv/include/asm/sbi.h        |  2 +-
 arch/riscv/kernel/cpu_ops.c         |  2 +-
 arch/riscv/kernel/sbi.c             | 17 ++++++++---------
 arch/riscv/kvm/main.c               |  2 +-
 drivers/cpuidle/cpuidle-riscv-sbi.c |  2 +-
 drivers/perf/riscv_pmu_sbi.c        |  2 +-
 6 files changed, 13 insertions(+), 14 deletions(-)

Comments

Conor Dooley April 27, 2023, 9:42 a.m. UTC | #1
On Wed, Apr 26, 2023 at 07:22:54PM +0200, Andrew Jones wrote:
> sbi_probe_extension() is specified with "Returns 0 if the given SBI
> extension ID (EID) is not available, or 1 if it is available unless
> defined as any other non-zero value by the implementation."
> Additionally, sbiret.value is a long. Fix the implementation to
> ensure any nonzero long value is considered a success, rather
> than only positive int values.

Does this need a fixes tag (or tags) then, since we could easily return
a negative value as things stand if the SBI implementation decides to
return 0b1...1 for success?

> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> ---
>  arch/riscv/include/asm/sbi.h        |  2 +-
>  arch/riscv/kernel/cpu_ops.c         |  2 +-
>  arch/riscv/kernel/sbi.c             | 17 ++++++++---------
>  arch/riscv/kvm/main.c               |  2 +-
>  drivers/cpuidle/cpuidle-riscv-sbi.c |  2 +-
>  drivers/perf/riscv_pmu_sbi.c        |  2 +-
>  6 files changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> index 945b7be249c1..119355485703 100644
> --- a/arch/riscv/include/asm/sbi.h
> +++ b/arch/riscv/include/asm/sbi.h
> @@ -296,7 +296,7 @@ int sbi_remote_hfence_vvma_asid(const struct cpumask *cpu_mask,
>  				unsigned long start,
>  				unsigned long size,
>  				unsigned long asid);
> -int sbi_probe_extension(int ext);
> +long sbi_probe_extension(int ext);
>  
>  /* Check if current SBI specification version is 0.1 or not */
>  static inline int sbi_spec_is_0_1(void)
> diff --git a/arch/riscv/kernel/cpu_ops.c b/arch/riscv/kernel/cpu_ops.c
> index 8275f237a59d..eb479a88a954 100644
> --- a/arch/riscv/kernel/cpu_ops.c
> +++ b/arch/riscv/kernel/cpu_ops.c
> @@ -27,7 +27,7 @@ const struct cpu_operations cpu_ops_spinwait = {
>  void __init cpu_set_ops(int cpuid)
>  {
>  #if IS_ENABLED(CONFIG_RISCV_SBI)
> -	if (sbi_probe_extension(SBI_EXT_HSM) > 0) {
> +	if (sbi_probe_extension(SBI_EXT_HSM)) {
>  		if (!cpuid)
>  			pr_info("SBI HSM extension detected\n");
>  		cpu_ops[cpuid] = &cpu_ops_sbi;
> diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> index 5c87db8fdff2..015ce8eef2de 100644
> --- a/arch/riscv/kernel/sbi.c
> +++ b/arch/riscv/kernel/sbi.c
> @@ -581,19 +581,18 @@ static void sbi_srst_power_off(void)
>   * sbi_probe_extension() - Check if an SBI extension ID is supported or not.
>   * @extid: The extension ID to be probed.
>   *
> - * Return: Extension specific nonzero value f yes, -ENOTSUPP otherwise.
> + * Return: 1 or an extension specific nonzero value if yes, 0 otherwise.
>   */
> -int sbi_probe_extension(int extid)
> +long sbi_probe_extension(int extid)
>  {
>  	struct sbiret ret;
>  
>  	ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_PROBE_EXT, extid,
>  			0, 0, 0, 0, 0);
>  	if (!ret.error)
> -		if (ret.value)
> -			return ret.value;
> +		return ret.value;
>  
> -	return -ENOTSUPP;
> +	return 0;

Why not make it return -ENOTSUPP for failures and 0 for success instead,
as it does not appear that any users actually care what the return value
is, once it is not an error?
Concerned that it'll look weird to have
	if(!sbi_probe_extension(foo))
		print(foo is available!)
since it looks a bit weird that the !case is success?

If so, perhaps it could just return a bool instead, unless of course I
am missing some pending user that make some decision on the actual value
returned here is.

Cheers,
Conor.

>  }
>  EXPORT_SYMBOL(sbi_probe_extension);
>  
> @@ -665,26 +664,26 @@ void __init sbi_init(void)
>  	if (!sbi_spec_is_0_1()) {
>  		pr_info("SBI implementation ID=0x%lx Version=0x%lx\n",
>  			sbi_get_firmware_id(), sbi_get_firmware_version());
> -		if (sbi_probe_extension(SBI_EXT_TIME) > 0) {
> +		if (sbi_probe_extension(SBI_EXT_TIME)) {
>  			__sbi_set_timer = __sbi_set_timer_v02;
>  			pr_info("SBI TIME extension detected\n");
>  		} else {
>  			__sbi_set_timer = __sbi_set_timer_v01;
>  		}
> -		if (sbi_probe_extension(SBI_EXT_IPI) > 0) {
> +		if (sbi_probe_extension(SBI_EXT_IPI)) {
>  			__sbi_send_ipi	= __sbi_send_ipi_v02;
>  			pr_info("SBI IPI extension detected\n");
>  		} else {
>  			__sbi_send_ipi	= __sbi_send_ipi_v01;
>  		}
> -		if (sbi_probe_extension(SBI_EXT_RFENCE) > 0) {
> +		if (sbi_probe_extension(SBI_EXT_RFENCE)) {
>  			__sbi_rfence	= __sbi_rfence_v02;
>  			pr_info("SBI RFENCE extension detected\n");
>  		} else {
>  			__sbi_rfence	= __sbi_rfence_v01;
>  		}
>  		if ((sbi_spec_version >= sbi_mk_version(0, 3)) &&
> -		    (sbi_probe_extension(SBI_EXT_SRST) > 0)) {
> +		    sbi_probe_extension(SBI_EXT_SRST)) {
>  			pr_info("SBI SRST extension detected\n");
>  			pm_power_off = sbi_srst_power_off;
>  			sbi_srst_reboot_nb.notifier_call = sbi_srst_reboot;
> diff --git a/arch/riscv/kvm/main.c b/arch/riscv/kvm/main.c
> index 41ad7639a17b..c923c113a129 100644
> --- a/arch/riscv/kvm/main.c
> +++ b/arch/riscv/kvm/main.c
> @@ -75,7 +75,7 @@ static int __init riscv_kvm_init(void)
>  		return -ENODEV;
>  	}
>  
> -	if (sbi_probe_extension(SBI_EXT_RFENCE) <= 0) {
> +	if (!sbi_probe_extension(SBI_EXT_RFENCE)) {
>  		kvm_info("require SBI RFENCE extension\n");
>  		return -ENODEV;
>  	}
> diff --git a/drivers/cpuidle/cpuidle-riscv-sbi.c b/drivers/cpuidle/cpuidle-riscv-sbi.c
> index be383f4b6855..c6b599167036 100644
> --- a/drivers/cpuidle/cpuidle-riscv-sbi.c
> +++ b/drivers/cpuidle/cpuidle-riscv-sbi.c
> @@ -613,7 +613,7 @@ static int __init sbi_cpuidle_init(void)
>  	 * 2) SBI HSM extension is available
>  	 */
>  	if ((sbi_spec_version < sbi_mk_version(0, 3)) ||
> -	    sbi_probe_extension(SBI_EXT_HSM) <= 0) {
> +	    !sbi_probe_extension(SBI_EXT_HSM)) {
>  		pr_info("HSM suspend not available\n");
>  		return 0;
>  	}
> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index 70cb50fd41c2..4f3ac296b3e2 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
> @@ -924,7 +924,7 @@ static int __init pmu_sbi_devinit(void)
>  	struct platform_device *pdev;
>  
>  	if (sbi_spec_version < sbi_mk_version(0, 3) ||
> -	    sbi_probe_extension(SBI_EXT_PMU) <= 0) {
> +	    !sbi_probe_extension(SBI_EXT_PMU)) {
>  		return 0;
>  	}
>  
> -- 
> 2.39.2
>
Andrew Jones April 27, 2023, 10:17 a.m. UTC | #2
On Thu, Apr 27, 2023 at 10:42:11AM +0100, Conor Dooley wrote:
> On Wed, Apr 26, 2023 at 07:22:54PM +0200, Andrew Jones wrote:
> > sbi_probe_extension() is specified with "Returns 0 if the given SBI
> > extension ID (EID) is not available, or 1 if it is available unless
> > defined as any other non-zero value by the implementation."
> > Additionally, sbiret.value is a long. Fix the implementation to
> > ensure any nonzero long value is considered a success, rather
> > than only positive int values.
> 
> Does this need a fixes tag (or tags) then, since we could easily return
> a negative value as things stand if the SBI implementation decides to
> return 0b1...1 for success?

Nothing is getting fixed when only considering the current generic opensbi
platform, but there could be other SBI implementations Linux isn't
handling correctly by only considering success to be greater than zero
or by truncating the return value from long to int. I suppose that
possibility does warrant a fix tag, which would be

Fixes: b9dcd9e41587 ("RISC-V: Add basic support for SBI v0.2")

> 
> > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > ---
> >  arch/riscv/include/asm/sbi.h        |  2 +-
> >  arch/riscv/kernel/cpu_ops.c         |  2 +-
> >  arch/riscv/kernel/sbi.c             | 17 ++++++++---------
> >  arch/riscv/kvm/main.c               |  2 +-
> >  drivers/cpuidle/cpuidle-riscv-sbi.c |  2 +-
> >  drivers/perf/riscv_pmu_sbi.c        |  2 +-
> >  6 files changed, 13 insertions(+), 14 deletions(-)
> > 
> > diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> > index 945b7be249c1..119355485703 100644
> > --- a/arch/riscv/include/asm/sbi.h
> > +++ b/arch/riscv/include/asm/sbi.h
> > @@ -296,7 +296,7 @@ int sbi_remote_hfence_vvma_asid(const struct cpumask *cpu_mask,
> >  				unsigned long start,
> >  				unsigned long size,
> >  				unsigned long asid);
> > -int sbi_probe_extension(int ext);
> > +long sbi_probe_extension(int ext);
> >  
> >  /* Check if current SBI specification version is 0.1 or not */
> >  static inline int sbi_spec_is_0_1(void)
> > diff --git a/arch/riscv/kernel/cpu_ops.c b/arch/riscv/kernel/cpu_ops.c
> > index 8275f237a59d..eb479a88a954 100644
> > --- a/arch/riscv/kernel/cpu_ops.c
> > +++ b/arch/riscv/kernel/cpu_ops.c
> > @@ -27,7 +27,7 @@ const struct cpu_operations cpu_ops_spinwait = {
> >  void __init cpu_set_ops(int cpuid)
> >  {
> >  #if IS_ENABLED(CONFIG_RISCV_SBI)
> > -	if (sbi_probe_extension(SBI_EXT_HSM) > 0) {
> > +	if (sbi_probe_extension(SBI_EXT_HSM)) {
> >  		if (!cpuid)
> >  			pr_info("SBI HSM extension detected\n");
> >  		cpu_ops[cpuid] = &cpu_ops_sbi;
> > diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> > index 5c87db8fdff2..015ce8eef2de 100644
> > --- a/arch/riscv/kernel/sbi.c
> > +++ b/arch/riscv/kernel/sbi.c
> > @@ -581,19 +581,18 @@ static void sbi_srst_power_off(void)
> >   * sbi_probe_extension() - Check if an SBI extension ID is supported or not.
> >   * @extid: The extension ID to be probed.
> >   *
> > - * Return: Extension specific nonzero value f yes, -ENOTSUPP otherwise.
> > + * Return: 1 or an extension specific nonzero value if yes, 0 otherwise.
> >   */
> > -int sbi_probe_extension(int extid)
> > +long sbi_probe_extension(int extid)
> >  {
> >  	struct sbiret ret;
> >  
> >  	ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_PROBE_EXT, extid,
> >  			0, 0, 0, 0, 0);
> >  	if (!ret.error)
> > -		if (ret.value)
> > -			return ret.value;
> > +		return ret.value;
> >  
> > -	return -ENOTSUPP;
> > +	return 0;
> 
> Why not make it return -ENOTSUPP for failures and 0 for success instead,
> as it does not appear that any users actually care what the return value
> is, once it is not an error?

Just to prepare for theoretical new uses.

> Concerned that it'll look weird to have
> 	if(!sbi_probe_extension(foo))
> 		print(foo is available!)
> since it looks a bit weird that the !case is success?

No, that's pretty par for the course in the kernel. I'd just prefer
that sbi_probe_extension() be a simple wrapper around the SBI call,
one that doesn't change the SBI call's return value semantics.

> 
> If so, perhaps it could just return a bool instead, unless of course I
> am missing some pending user that make some decision on the actual value
> returned here is.

No pending user that I'm aware of, but it's not too awkward, IMO, to
implement this as the spec says, so any pending user that comes along
will be happy from the start. If a boolean function seems better for
everyone, then I'd probably extend this patch to also rename this
sbi_probe_extension to __sbi_probe_extension, make it static, and
then add

 bool sbi_probe_extension(int extid)
 {
   return __sbi_probe_extension(extid);
 }

just to feel good about the wrapper!

Thanks,
drew
Conor Dooley April 27, 2023, 10:37 a.m. UTC | #3
On Thu, Apr 27, 2023 at 12:17:02PM +0200, Andrew Jones wrote:
> On Thu, Apr 27, 2023 at 10:42:11AM +0100, Conor Dooley wrote:
> > On Wed, Apr 26, 2023 at 07:22:54PM +0200, Andrew Jones wrote:
> > > sbi_probe_extension() is specified with "Returns 0 if the given SBI
> > > extension ID (EID) is not available, or 1 if it is available unless
> > > defined as any other non-zero value by the implementation."
> > > Additionally, sbiret.value is a long. Fix the implementation to
> > > ensure any nonzero long value is considered a success, rather
> > > than only positive int values.
> > 
> > Does this need a fixes tag (or tags) then, since we could easily return
> > a negative value as things stand if the SBI implementation decides to
> > return 0b1...1 for success?
> 
> Nothing is getting fixed when only considering the current generic opensbi
> platform, but there could be other SBI implementations Linux isn't
> handling correctly by only considering success to be greater than zero
> or by truncating the return value from long to int. I suppose that
> possibility does warrant a fix tag, which would be

Yah, I figured that something like opensbi would do the "sane" thing
here, but there's no accounting for taste and all that.

> Fixes: b9dcd9e41587 ("RISC-V: Add basic support for SBI v0.2")
> 
> > 
> > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > > ---
> > >  arch/riscv/include/asm/sbi.h        |  2 +-
> > >  arch/riscv/kernel/cpu_ops.c         |  2 +-
> > >  arch/riscv/kernel/sbi.c             | 17 ++++++++---------
> > >  arch/riscv/kvm/main.c               |  2 +-
> > >  drivers/cpuidle/cpuidle-riscv-sbi.c |  2 +-
> > >  drivers/perf/riscv_pmu_sbi.c        |  2 +-
> > >  6 files changed, 13 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> > > index 945b7be249c1..119355485703 100644
> > > --- a/arch/riscv/include/asm/sbi.h
> > > +++ b/arch/riscv/include/asm/sbi.h
> > > @@ -296,7 +296,7 @@ int sbi_remote_hfence_vvma_asid(const struct cpumask *cpu_mask,
> > >  				unsigned long start,
> > >  				unsigned long size,
> > >  				unsigned long asid);
> > > -int sbi_probe_extension(int ext);
> > > +long sbi_probe_extension(int ext);
> > >  
> > >  /* Check if current SBI specification version is 0.1 or not */
> > >  static inline int sbi_spec_is_0_1(void)
> > > diff --git a/arch/riscv/kernel/cpu_ops.c b/arch/riscv/kernel/cpu_ops.c
> > > index 8275f237a59d..eb479a88a954 100644
> > > --- a/arch/riscv/kernel/cpu_ops.c
> > > +++ b/arch/riscv/kernel/cpu_ops.c
> > > @@ -27,7 +27,7 @@ const struct cpu_operations cpu_ops_spinwait = {
> > >  void __init cpu_set_ops(int cpuid)
> > >  {
> > >  #if IS_ENABLED(CONFIG_RISCV_SBI)
> > > -	if (sbi_probe_extension(SBI_EXT_HSM) > 0) {
> > > +	if (sbi_probe_extension(SBI_EXT_HSM)) {
> > >  		if (!cpuid)
> > >  			pr_info("SBI HSM extension detected\n");
> > >  		cpu_ops[cpuid] = &cpu_ops_sbi;
> > > diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> > > index 5c87db8fdff2..015ce8eef2de 100644
> > > --- a/arch/riscv/kernel/sbi.c
> > > +++ b/arch/riscv/kernel/sbi.c
> > > @@ -581,19 +581,18 @@ static void sbi_srst_power_off(void)
> > >   * sbi_probe_extension() - Check if an SBI extension ID is supported or not.
> > >   * @extid: The extension ID to be probed.
> > >   *
> > > - * Return: Extension specific nonzero value f yes, -ENOTSUPP otherwise.
> > > + * Return: 1 or an extension specific nonzero value if yes, 0 otherwise.
> > >   */
> > > -int sbi_probe_extension(int extid)
> > > +long sbi_probe_extension(int extid)
> > >  {
> > >  	struct sbiret ret;
> > >  
> > >  	ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_PROBE_EXT, extid,
> > >  			0, 0, 0, 0, 0);
> > >  	if (!ret.error)
> > > -		if (ret.value)
> > > -			return ret.value;
> > > +		return ret.value;
> > >  
> > > -	return -ENOTSUPP;
> > > +	return 0;
> > 
> > Why not make it return -ENOTSUPP for failures and 0 for success instead,
> > as it does not appear that any users actually care what the return value
> > is, once it is not an error?
> 
> Just to prepare for theoretical new uses.
> 
> > Concerned that it'll look weird to have
> > 	if(!sbi_probe_extension(foo))
> > 		print(foo is available!)
> > since it looks a bit weird that the !case is success?
> 
> No, that's pretty par for the course in the kernel. I'd just prefer
> that sbi_probe_extension() be a simple wrapper around the SBI call,
> one that doesn't change the SBI call's return value semantics.
> 
> > 
> > If so, perhaps it could just return a bool instead, unless of course I
> > am missing some pending user that make some decision on the actual value
> > returned here is.
> 
> No pending user that I'm aware of, but it's not too awkward, IMO, to
> implement this as the spec says, so any pending user that comes along
> will be happy from the start. If a boolean function seems better for
> everyone,

I don't really mind which way you do it to be honest.
I see reasons for both wanting alignment with what the SBI spec has &
for returning the simplest type that fits the usage.
Both are better than the current, potentially buggy, implementation.

Can put a
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
on it either way.
diff mbox series

Patch

diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
index 945b7be249c1..119355485703 100644
--- a/arch/riscv/include/asm/sbi.h
+++ b/arch/riscv/include/asm/sbi.h
@@ -296,7 +296,7 @@  int sbi_remote_hfence_vvma_asid(const struct cpumask *cpu_mask,
 				unsigned long start,
 				unsigned long size,
 				unsigned long asid);
-int sbi_probe_extension(int ext);
+long sbi_probe_extension(int ext);
 
 /* Check if current SBI specification version is 0.1 or not */
 static inline int sbi_spec_is_0_1(void)
diff --git a/arch/riscv/kernel/cpu_ops.c b/arch/riscv/kernel/cpu_ops.c
index 8275f237a59d..eb479a88a954 100644
--- a/arch/riscv/kernel/cpu_ops.c
+++ b/arch/riscv/kernel/cpu_ops.c
@@ -27,7 +27,7 @@  const struct cpu_operations cpu_ops_spinwait = {
 void __init cpu_set_ops(int cpuid)
 {
 #if IS_ENABLED(CONFIG_RISCV_SBI)
-	if (sbi_probe_extension(SBI_EXT_HSM) > 0) {
+	if (sbi_probe_extension(SBI_EXT_HSM)) {
 		if (!cpuid)
 			pr_info("SBI HSM extension detected\n");
 		cpu_ops[cpuid] = &cpu_ops_sbi;
diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
index 5c87db8fdff2..015ce8eef2de 100644
--- a/arch/riscv/kernel/sbi.c
+++ b/arch/riscv/kernel/sbi.c
@@ -581,19 +581,18 @@  static void sbi_srst_power_off(void)
  * sbi_probe_extension() - Check if an SBI extension ID is supported or not.
  * @extid: The extension ID to be probed.
  *
- * Return: Extension specific nonzero value f yes, -ENOTSUPP otherwise.
+ * Return: 1 or an extension specific nonzero value if yes, 0 otherwise.
  */
-int sbi_probe_extension(int extid)
+long sbi_probe_extension(int extid)
 {
 	struct sbiret ret;
 
 	ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_PROBE_EXT, extid,
 			0, 0, 0, 0, 0);
 	if (!ret.error)
-		if (ret.value)
-			return ret.value;
+		return ret.value;
 
-	return -ENOTSUPP;
+	return 0;
 }
 EXPORT_SYMBOL(sbi_probe_extension);
 
@@ -665,26 +664,26 @@  void __init sbi_init(void)
 	if (!sbi_spec_is_0_1()) {
 		pr_info("SBI implementation ID=0x%lx Version=0x%lx\n",
 			sbi_get_firmware_id(), sbi_get_firmware_version());
-		if (sbi_probe_extension(SBI_EXT_TIME) > 0) {
+		if (sbi_probe_extension(SBI_EXT_TIME)) {
 			__sbi_set_timer = __sbi_set_timer_v02;
 			pr_info("SBI TIME extension detected\n");
 		} else {
 			__sbi_set_timer = __sbi_set_timer_v01;
 		}
-		if (sbi_probe_extension(SBI_EXT_IPI) > 0) {
+		if (sbi_probe_extension(SBI_EXT_IPI)) {
 			__sbi_send_ipi	= __sbi_send_ipi_v02;
 			pr_info("SBI IPI extension detected\n");
 		} else {
 			__sbi_send_ipi	= __sbi_send_ipi_v01;
 		}
-		if (sbi_probe_extension(SBI_EXT_RFENCE) > 0) {
+		if (sbi_probe_extension(SBI_EXT_RFENCE)) {
 			__sbi_rfence	= __sbi_rfence_v02;
 			pr_info("SBI RFENCE extension detected\n");
 		} else {
 			__sbi_rfence	= __sbi_rfence_v01;
 		}
 		if ((sbi_spec_version >= sbi_mk_version(0, 3)) &&
-		    (sbi_probe_extension(SBI_EXT_SRST) > 0)) {
+		    sbi_probe_extension(SBI_EXT_SRST)) {
 			pr_info("SBI SRST extension detected\n");
 			pm_power_off = sbi_srst_power_off;
 			sbi_srst_reboot_nb.notifier_call = sbi_srst_reboot;
diff --git a/arch/riscv/kvm/main.c b/arch/riscv/kvm/main.c
index 41ad7639a17b..c923c113a129 100644
--- a/arch/riscv/kvm/main.c
+++ b/arch/riscv/kvm/main.c
@@ -75,7 +75,7 @@  static int __init riscv_kvm_init(void)
 		return -ENODEV;
 	}
 
-	if (sbi_probe_extension(SBI_EXT_RFENCE) <= 0) {
+	if (!sbi_probe_extension(SBI_EXT_RFENCE)) {
 		kvm_info("require SBI RFENCE extension\n");
 		return -ENODEV;
 	}
diff --git a/drivers/cpuidle/cpuidle-riscv-sbi.c b/drivers/cpuidle/cpuidle-riscv-sbi.c
index be383f4b6855..c6b599167036 100644
--- a/drivers/cpuidle/cpuidle-riscv-sbi.c
+++ b/drivers/cpuidle/cpuidle-riscv-sbi.c
@@ -613,7 +613,7 @@  static int __init sbi_cpuidle_init(void)
 	 * 2) SBI HSM extension is available
 	 */
 	if ((sbi_spec_version < sbi_mk_version(0, 3)) ||
-	    sbi_probe_extension(SBI_EXT_HSM) <= 0) {
+	    !sbi_probe_extension(SBI_EXT_HSM)) {
 		pr_info("HSM suspend not available\n");
 		return 0;
 	}
diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
index 70cb50fd41c2..4f3ac296b3e2 100644
--- a/drivers/perf/riscv_pmu_sbi.c
+++ b/drivers/perf/riscv_pmu_sbi.c
@@ -924,7 +924,7 @@  static int __init pmu_sbi_devinit(void)
 	struct platform_device *pdev;
 
 	if (sbi_spec_version < sbi_mk_version(0, 3) ||
-	    sbi_probe_extension(SBI_EXT_PMU) <= 0) {
+	    !sbi_probe_extension(SBI_EXT_PMU)) {
 		return 0;
 	}