diff mbox series

[2/4] RISC-V: don't parse dt isa string to get rv32/rv64

Message ID 20230424194911.264850-3-heiko.stuebner@vrull.eu (mailing list archive)
State Rejected
Headers show
Series Expose the isa-string via the AT_BASE_PLATFORM aux vector | expand

Checks

Context Check Description
conchuod/cover_letter success Series has a cover letter
conchuod/tree_selection success Guessed tree name to be for-next at HEAD 310c33dc7a12
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: 0 this patch: 0
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: 20 this patch: 20
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, 28 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

Heiko Stübner April 24, 2023, 7:49 p.m. UTC 
From: Heiko Stuebner <heiko.stuebner@vrull.eu>

When filling hwcap the kernel already expects the isa string to start with
rv32 if CONFIG_32BIT and rv64 if CONFIG_64BIT.

So when recreating the runtime isa-string we can also just go the other way
to get the correct starting point for it.

Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
---
 arch/riscv/kernel/cpu.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Comments

Andrew Jones April 26, 2023, 9:37 a.m. UTC | #1 
On Mon, Apr 24, 2023 at 09:49:09PM +0200, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@vrull.eu>
> 
> When filling hwcap the kernel already expects the isa string to start with
> rv32 if CONFIG_32BIT and rv64 if CONFIG_64BIT.
> 
> So when recreating the runtime isa-string we can also just go the other way
> to get the correct starting point for it.
> 
> Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
> ---
>  arch/riscv/kernel/cpu.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index ebc478f0a16c..06c2f587a176 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -244,7 +244,7 @@ static void strcat_isa_ext(char *isa_str)
>   */
>  static const char base_riscv_exts[13] = "imafdqcbkjpvh";
>  
> -static char *riscv_create_isa_string(const char *isa)
> +static char *riscv_create_isa_string(void)
>  {
>  	int maxlen = 4;
>  	char *isa_str;
> @@ -261,7 +261,11 @@ static char *riscv_create_isa_string(const char *isa)
>  		return ERR_PTR(-ENOMEM);
>  
>  	/* Print the rv[64/32] part */
> -	strncat(isa_str, isa, 4);
> +#if IS_ENABLED(CONFIG_32BIT)
> +	strncat(isa_str, "rv32", 4);
> +#elif IS_ENABLED(CONFIG_64BIT)
> +	strncat(isa_str, "rv64", 4);
> +#endif

I see we do the validation in riscv_fill_hwcap() and we also do this
#ifdeffery there, but I can't see any reason not to do

 if (IS_ENABLED(CONFIG_32BIT))
    ...
 else
    ...

instead.


>  
>  	for (i = 0; i < sizeof(base_riscv_exts); i++) {
>  		if (__riscv_isa_extension_available(NULL, base_riscv_exts[i] - 'a'))
> @@ -280,7 +284,7 @@ static void print_isa(struct seq_file *f, const char *isa)
>  
>  	seq_puts(f, "isa\t\t: ");
>  
> -	isa_str = riscv_create_isa_string(isa);
> +	isa_str = riscv_create_isa_string();

Why are we still passing 'isa' to print_isa()? It's now unused and
print_isa()'s single caller only fetches it from the DT for this
one purpose, so that of_property_read_string() call could now also
be dropped.

>  	if (!IS_ERR(isa_str)) {
>  		seq_write(f, isa_str, strlen(isa_str));
>  		kfree(isa_str);
> -- 
> 2.39.0
>

Thanks,
drew
Palmer Dabbelt May 1, 2023, 2:51 p.m. UTC | #2 
On Mon, 24 Apr 2023 12:49:09 PDT (-0700), heiko@sntech.de wrote:
> From: Heiko Stuebner <heiko.stuebner@vrull.eu>
>
> When filling hwcap the kernel already expects the isa string to start with
> rv32 if CONFIG_32BIT and rv64 if CONFIG_64BIT.
>
> So when recreating the runtime isa-string we can also just go the other way
> to get the correct starting point for it.
>
> Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
> ---
>  arch/riscv/kernel/cpu.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index ebc478f0a16c..06c2f587a176 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -244,7 +244,7 @@ static void strcat_isa_ext(char *isa_str)
>   */
>  static const char base_riscv_exts[13] = "imafdqcbkjpvh";
>
> -static char *riscv_create_isa_string(const char *isa)
> +static char *riscv_create_isa_string(void)
>  {
>  	int maxlen = 4;
>  	char *isa_str;
> @@ -261,7 +261,11 @@ static char *riscv_create_isa_string(const char *isa)
>  		return ERR_PTR(-ENOMEM);
>
>  	/* Print the rv[64/32] part */
> -	strncat(isa_str, isa, 4);
> +#if IS_ENABLED(CONFIG_32BIT)
> +	strncat(isa_str, "rv32", 4);
> +#elif IS_ENABLED(CONFIG_64BIT)
> +	strncat(isa_str, "rv64", 4);
> +#endif
>
>  	for (i = 0; i < sizeof(base_riscv_exts); i++) {
>  		if (__riscv_isa_extension_available(NULL, base_riscv_exts[i] - 'a'))
> @@ -280,7 +284,7 @@ static void print_isa(struct seq_file *f, const char *isa)
>
>  	seq_puts(f, "isa\t\t: ");
>
> -	isa_str = riscv_create_isa_string(isa);
> +	isa_str = riscv_create_isa_string();
>  	if (!IS_ERR(isa_str)) {
>  		seq_write(f, isa_str, strlen(isa_str));
>  		kfree(isa_str);

Conor pointed out this one, I just looked at the cover letter and got 
distracted by uABI stuff.  It's not directly fixing anything so I'm 
going to leave it out of this merge window, particularly given that it's 
tied up with the rest.

Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com>
diff mbox series

Patch

diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
index ebc478f0a16c..06c2f587a176 100644
--- a/arch/riscv/kernel/cpu.c
+++ b/arch/riscv/kernel/cpu.c
@@ -244,7 +244,7 @@  static void strcat_isa_ext(char *isa_str)
  */
 static const char base_riscv_exts[13] = "imafdqcbkjpvh";
 
-static char *riscv_create_isa_string(const char *isa)
+static char *riscv_create_isa_string(void)
 {
 	int maxlen = 4;
 	char *isa_str;
@@ -261,7 +261,11 @@  static char *riscv_create_isa_string(const char *isa)
 		return ERR_PTR(-ENOMEM);
 
 	/* Print the rv[64/32] part */
-	strncat(isa_str, isa, 4);
+#if IS_ENABLED(CONFIG_32BIT)
+	strncat(isa_str, "rv32", 4);
+#elif IS_ENABLED(CONFIG_64BIT)
+	strncat(isa_str, "rv64", 4);
+#endif
 
 	for (i = 0; i < sizeof(base_riscv_exts); i++) {
 		if (__riscv_isa_extension_available(NULL, base_riscv_exts[i] - 'a'))
@@ -280,7 +284,7 @@  static void print_isa(struct seq_file *f, const char *isa)
 
 	seq_puts(f, "isa\t\t: ");
 
-	isa_str = riscv_create_isa_string(isa);
+	isa_str = riscv_create_isa_string();
 	if (!IS_ERR(isa_str)) {
 		seq_write(f, isa_str, strlen(isa_str));
 		kfree(isa_str);