Date:   Wed, 26 Apr 2023 19:54:39 +0100
From:   Conor Dooley <conor@...nel.org>
To:     Yangyu Chen <cyy@...self.name>
Cc:     Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Wende Tan <twd2.me@...il.com>, Soha Jin <soha@...u.info>,
        Hongren Zheng <i@...ithal.me>,
        Andrew Jones <ajones@...tanamicro.com>
Subject: Re: [PATCH 1/2] riscv: allow case-insensitive ISA string parsing

(+CC Drew)

Hey Yangyu,

One meta-level comment - can you submit this patch + my dt-bindings
patch as a v2?
Some comments below.

On Tue, Apr 25, 2023 at 08:00:15PM +0800, Yangyu Chen wrote:
> According to RISC-V ISA specification, the ISA naming strings are case
> insensitive. The kernel docs require the riscv,isa string must be all
> lowercase to simplify parsing currently. However, this limitation is not
> consistent with RISC-V ISA Spec.

Please remove the above and cite ACPI's case-insensitivity as the
rationale for this change.

> This patch modifies the ISA string parser in the kernel to support
> case-insensitive ISA string parsing. It replaces `strncmp` with
> `strncasecmp`, replaces `islower` with `isalpha`, and wraps the
> dereferenced char in the parser with `tolower`.
> 
> Signed-off-by: Yangyu Chen <cyy@...self.name>
> ---
>  arch/riscv/kernel/cpu.c        |  6 ++++--
>  arch/riscv/kernel/cpufeature.c | 20 ++++++++++----------
>  2 files changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index 8400f0cc9704..531c76079b73 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -4,6 +4,7 @@
>   */
>  
>  #include <linux/cpu.h>
> +#include <linux/ctype.h>
>  #include <linux/init.h>
>  #include <linux/seq_file.h>
>  #include <linux/of.h>
> @@ -41,7 +42,7 @@ int riscv_of_processor_hartid(struct device_node *node, unsigned long *hart)
>  		pr_warn("CPU with hartid=%lu has no \"riscv,isa\" property\n", *hart);
>  		return -ENODEV;
>  	}
> -	if (isa[0] != 'r' || isa[1] != 'v') {
> +	if (tolower(isa[0]) != 'r' || tolower(isa[1]) != 'v') {
>  		pr_warn("CPU with hartid=%lu has an invalid ISA of \"%s\"\n", *hart, isa);
>  		return -ENODEV;

I don't understand why this is even here in the first place. I'd be
inclined to advocate for it's entire removal. Checking *only* that there
is an "rv" in that string seems pointless to me. If you're on a 64-bit
kernel and the node has riscv,isa = "rv32ima" it's gonna say it is okay?
Drew what do you think?

>  	}
> @@ -228,7 +229,8 @@ static void print_isa(struct seq_file *f, const char *isa)
>  
>  	seq_puts(f, "isa\t\t: ");
>  	/* Print the rv[64/32] part */
> -	seq_write(f, isa, 4);
> +	for (i = 0; i < 4; i++)
> +		seq_putc(f, tolower(isa[i]));

As was pointed out elsewhere, we shouldn't parse the dt to construct
this in the first place. We know what kernel we are running on, so we
should instead do write "rv" into the string & do:
	string = "rv"
	if IS_ENABLED(32-bit):
		isa.append("32")
	else
		isa.append("64")

See the link below, and Drew's comment on that:
https://lore.kernel.org/all/20230424194911.264850-3-heiko.stuebner@vrull.eu/
I'm fine with this change for now in isolation, but it ideally will be
replaced with something that doesn't touch the DT/ACPI for this
information.

>  	for (i = 0; i < sizeof(base_riscv_exts); i++) {
>  		if (__riscv_isa_extension_available(NULL, base_riscv_exts[i] - 'a'))
>  			/* Print only enabled the base ISA extensions */
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 59d58ee0f68d..c01dd144addc 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -120,10 +120,10 @@ void __init riscv_fill_hwcap(void)
>  
>  		temp = isa;
>  #if IS_ENABLED(CONFIG_32BIT)
> -		if (!strncmp(isa, "rv32", 4))
> +		if (!strncasecmp(isa, "rv32", 4))
>  			isa += 4;
>  #elif IS_ENABLED(CONFIG_64BIT)
> -		if (!strncmp(isa, "rv64", 4))
> +		if (!strncasecmp(isa, "rv64", 4))
>  			isa += 4;
>  #endif

If you're already modifying these lines, why not convert the ifdeffery
into something like
		if (IS_ENABLED(CONFIG_32BIT) && !strncasecmp(isa, "rv32", 4))
				isa += 4;
		else if (!strncasecmp(isa, "rv64", 4))
				isa += 4;

>  		/* The riscv,isa DT property must start with rv64 or rv32 */
> @@ -135,7 +135,7 @@ void __init riscv_fill_hwcap(void)
>  			const char *ext_end = isa;
>  			bool ext_long = false, ext_err = false;
>  
> -			switch (*ext) {
> +			switch (tolower(*ext)) {

Is there merit in just converting the entire string to lower-case in the
first place rather than having to fiddle with every single time we want
to do a comparison here?

>  			case 's':
>  				/**
>  				 * Workaround for invalid single-letter 's' & 'u'(QEMU).
> @@ -143,7 +143,7 @@ void __init riscv_fill_hwcap(void)
>  				 * not valid ISA extensions. It works until multi-letter
>  				 * extension starting with "Su" appears.
>  				 */
> -				if (ext[-1] != '_' && ext[1] == 'u') {
> +				if (ext[-1] != '_' && tolower(ext[1]) == 'u') {
>  					++isa;
>  					ext_err = true;
>  					break;
> @@ -154,7 +154,7 @@ void __init riscv_fill_hwcap(void)
>  				ext_long = true;
>  				/* Multi-letter extension must be delimited */
>  				for (; *isa && *isa != '_'; ++isa)
> -					if (unlikely(!islower(*isa)
> +					if (unlikely(!isalpha(*isa)
>  						     && !isdigit(*isa)))

This collapses to isalnum() I think, no?

Cheers,
Conor.

>  						ext_err = true;
>  				/* Parse backwards */
> @@ -166,7 +166,7 @@ void __init riscv_fill_hwcap(void)
>  				/* Skip the minor version */
>  				while (isdigit(*--ext_end))
>  					;
> -				if (ext_end[0] != 'p'
> +				if (tolower(ext_end[0]) != 'p'
>  				    || !isdigit(ext_end[-1])) {
>  					/* Advance it to offset the pre-decrement */
>  					++ext_end;
> @@ -178,7 +178,7 @@ void __init riscv_fill_hwcap(void)
>  				++ext_end;
>  				break;
>  			default:
> -				if (unlikely(!islower(*ext))) {
> +				if (unlikely(!isalpha(*ext))) {
>  					ext_err = true;
>  					break;
>  				}
> @@ -188,7 +188,7 @@ void __init riscv_fill_hwcap(void)
>  				/* Skip the minor version */
>  				while (isdigit(*++isa))
>  					;
> -				if (*isa != 'p')
> +				if (tolower(*isa) != 'p')
>  					break;
>  				if (!isdigit(*++isa)) {
>  					--isa;
> @@ -205,7 +205,7 @@ void __init riscv_fill_hwcap(void)
>  #define SET_ISA_EXT_MAP(name, bit)						\
>  			do {							\
>  				if ((ext_end - ext == sizeof(name) - 1) &&	\
> -				     !memcmp(ext, name, sizeof(name) - 1) &&	\
> +				     !strncasecmp(ext, name, sizeof(name) - 1) &&	\
>  				     riscv_isa_extension_check(bit))		\
>  					set_bit(bit, this_isa);			\
>  			} while (false)						\
> @@ -213,7 +213,7 @@ void __init riscv_fill_hwcap(void)
>  			if (unlikely(ext_err))
>  				continue;
>  			if (!ext_long) {
> -				int nr = *ext - 'a';
> +				int nr = tolower(*ext) - 'a';
>  
>  				if (riscv_isa_extension_check(nr)) {
>  					this_hwcap |= isa2hwcap[nr];
> -- 
> 2.40.0
> 

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists