| Message ID | tencent_BC90DDC6093E3E41246D3EC73F5CAB189007@qq.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/2] riscv: allow case-insensitive ISA string parsing | expand |
| 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 3ec1aafb0ff9 |
| 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, 100 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 |
Sorry for the cover letter being detached with patches. My mailing service provider will override the Message-IDs which caused this trouble. I will send the cover letter first to get the correct Message-ID before sending the rest of the patches the next time. The cover letter is at https://lore.kernel.org/linux-riscv/tencent_8492B68063042E768C758871A3171FBD2006@qq.com/ The patches is at https://lore.kernel.org/linux-riscv/tencent_85F69423082E524C478844E31D5F8920A506@qq.com/
On Sat, Apr 29, 2023 at 12:26:00AM +0800, Yangyu Chen wrote: > Sorry for the cover letter being detached with patches. My mailing > service provider will override the Message-IDs which caused this > trouble. I will send the cover letter first to get the correct Message-ID > before sending the rest of the patches the next time. At least it doesn't overwrite the in-reply-to headers also, so the patches are at least threaded. > The cover letter is at https://lore.kernel.org/linux-riscv/tencent_8492B68063042E768C758871A3171FBD2006@qq.com/ > The patches is at https://lore.kernel.org/linux-riscv/tencent_85F69423082E524C478844E31D5F8920A506@qq.com/ >
Hey Yangyu, On Fri, Apr 28, 2023 at 10:16:00PM +0800, Yangyu Chen wrote: > According to RISC-V Hart Capabilities Table (RHCT) description in UEFI > Forum ECR, the format of the ISA string is defined in the RISC-V > unprivileged specification which is case-insensitive. However, the > current ISA string parser in the kernel does not support ISA strings > with uppercase letters. > > This patch modifies the ISA string parser in the kernel to support > case-insensitive ISA string parsing. @Palmer, @Sunil Just to note, we probably should get this applied *before* we enable ACPI, right? > > Signed-off-by: Yangyu Chen <cyy@cyyself.name> > --- > arch/riscv/kernel/cpu.c | 3 ++- > arch/riscv/kernel/cpufeature.c | 25 ++++++++++++------------- > 2 files changed, 14 insertions(+), 14 deletions(-) > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > index 59d58ee0f68d..d1991c12e546 100644 > --- a/arch/riscv/kernel/cpufeature.c > +++ b/arch/riscv/kernel/cpufeature.c > @@ -119,13 +119,10 @@ void __init riscv_fill_hwcap(void) > } > > temp = isa; > -#if IS_ENABLED(CONFIG_32BIT) > - if (!strncmp(isa, "rv32", 4)) > + if (IS_ENABLED(CONFIG_32BIT) && !strncasecmp(isa, "rv32", 4)) > isa += 4; > -#elif IS_ENABLED(CONFIG_64BIT) > - if (!strncmp(isa, "rv64", 4)) > + else if (IS_ENABLED(CONFIG_64BIT) && !strncasecmp(isa, "rv64", 4)) > isa += 4; > -#endif > /* The riscv,isa DT property must start with rv64 or rv32 */ > if (temp == isa) > continue; > @@ -136,6 +133,7 @@ void __init riscv_fill_hwcap(void) > bool ext_long = false, ext_err = false; > > switch (*ext) { > + case 'S': Capital S should never be emitted by QEMU, so there's no need to have this use the workaround, right? IOW, move it between s & X. Otherwise, this looks good to me: Reviewed-by: Conor Dooley <conor.dooley@microchip.com> Thanks, Conor.
On Fri, Apr 28, 2023 at 10:16:00PM +0800, Yangyu Chen wrote: > According to RISC-V Hart Capabilities Table (RHCT) description in UEFI > Forum ECR, the format of the ISA string is defined in the RISC-V > unprivileged specification which is case-insensitive. However, the > current ISA string parser in the kernel does not support ISA strings > with uppercase letters. > > This patch modifies the ISA string parser in the kernel to support > case-insensitive ISA string parsing. > > Signed-off-by: Yangyu Chen <cyy@cyyself.name> > --- > arch/riscv/kernel/cpu.c | 3 ++- > arch/riscv/kernel/cpufeature.c | 25 ++++++++++++------------- > 2 files changed, 14 insertions(+), 14 deletions(-) > > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c > index 8400f0cc9704..52b92a267121 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; > } > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > index 59d58ee0f68d..d1991c12e546 100644 > --- a/arch/riscv/kernel/cpufeature.c > +++ b/arch/riscv/kernel/cpufeature.c > @@ -119,13 +119,10 @@ void __init riscv_fill_hwcap(void) > } > > temp = isa; > -#if IS_ENABLED(CONFIG_32BIT) > - if (!strncmp(isa, "rv32", 4)) > + if (IS_ENABLED(CONFIG_32BIT) && !strncasecmp(isa, "rv32", 4)) > isa += 4; > -#elif IS_ENABLED(CONFIG_64BIT) > - if (!strncmp(isa, "rv64", 4)) > + else if (IS_ENABLED(CONFIG_64BIT) && !strncasecmp(isa, "rv64", 4)) > isa += 4; > -#endif > /* The riscv,isa DT property must start with rv64 or rv32 */ > if (temp == isa) > continue; > @@ -136,6 +133,7 @@ void __init riscv_fill_hwcap(void) > bool ext_long = false, ext_err = false; > > switch (*ext) { > + case 'S': > case 's': > /** > * Workaround for invalid single-letter 's' & 'u'(QEMU). > @@ -143,19 +141,20 @@ 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; > } > fallthrough; > + case 'X': > case 'x': > + case 'Z': > case 'z': > ext_long = true; > /* Multi-letter extension must be delimited */ > for (; *isa && *isa != '_'; ++isa) > - if (unlikely(!islower(*isa) > - && !isdigit(*isa))) > + if (unlikely(!isalnum(*isa))) > ext_err = true; > /* Parse backwards */ > ext_end = isa; > @@ -166,7 +165,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 +177,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 +187,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 +204,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 +212,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 > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> Thanks, drew
On Fri, Apr 28, 2023 at 10:16:00PM +0800, Yangyu Chen wrote: > @@ -205,7 +204,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) \ I was doing some poking in this area the other day & noticed that the \s being misaligned isn't just the diff's fault and they are now misaligned in the code. If you are re-submitting, then I think you could align them. Thanks, Conor.
diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c index 8400f0cc9704..52b92a267121 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; } diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c index 59d58ee0f68d..d1991c12e546 100644 --- a/arch/riscv/kernel/cpufeature.c +++ b/arch/riscv/kernel/cpufeature.c @@ -119,13 +119,10 @@ void __init riscv_fill_hwcap(void) } temp = isa; -#if IS_ENABLED(CONFIG_32BIT) - if (!strncmp(isa, "rv32", 4)) + if (IS_ENABLED(CONFIG_32BIT) && !strncasecmp(isa, "rv32", 4)) isa += 4; -#elif IS_ENABLED(CONFIG_64BIT) - if (!strncmp(isa, "rv64", 4)) + else if (IS_ENABLED(CONFIG_64BIT) && !strncasecmp(isa, "rv64", 4)) isa += 4; -#endif /* The riscv,isa DT property must start with rv64 or rv32 */ if (temp == isa) continue; @@ -136,6 +133,7 @@ void __init riscv_fill_hwcap(void) bool ext_long = false, ext_err = false; switch (*ext) { + case 'S': case 's': /** * Workaround for invalid single-letter 's' & 'u'(QEMU). @@ -143,19 +141,20 @@ 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; } fallthrough; + case 'X': case 'x': + case 'Z': case 'z': ext_long = true; /* Multi-letter extension must be delimited */ for (; *isa && *isa != '_'; ++isa) - if (unlikely(!islower(*isa) - && !isdigit(*isa))) + if (unlikely(!isalnum(*isa))) ext_err = true; /* Parse backwards */ ext_end = isa; @@ -166,7 +165,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 +177,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 +187,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 +204,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 +212,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];
According to RISC-V Hart Capabilities Table (RHCT) description in UEFI Forum ECR, the format of the ISA string is defined in the RISC-V unprivileged specification which is case-insensitive. However, the current ISA string parser in the kernel does not support ISA strings with uppercase letters. This patch modifies the ISA string parser in the kernel to support case-insensitive ISA string parsing. Signed-off-by: Yangyu Chen <cyy@cyyself.name> --- arch/riscv/kernel/cpu.c | 3 ++- arch/riscv/kernel/cpufeature.c | 25 ++++++++++++------------- 2 files changed, 14 insertions(+), 14 deletions(-)