Re: [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing.
On Tue, 2019-07-30 at 17:08 -0700, Paul Walmsley wrote: > On Mon, 29 Jul 2019, Atish Patra wrote: > > > The yaml document did not specify anything about all isa-strings > > has to > > be lowercase unless I missed something. The two enum values are > > all > > lowercase but the description says all ISA strings are documented > > in ISA > > specification which describes the ISA strings to be case > > insensitive. > > IMHO, this creates confusion resulting the patch. > > If it's helpful in understanding my earlier comments, I don't think > that > your patches were "wrong," or anything like that. And you're right > that > the DT YAML binding does not unequivocally state that all future > additions > to the riscv,isa string must be in lowercase. But to be clear, the > DT > YAML schema defines exactly what is currently permissible for > riscv,isa > strings in kernel-oriented DT data, and right now, all of the > permissible > values are lowercase. > Going forward, yaml schema should define only the mandatory isa strings (i.e. rv64imafdc) or the list should be updated as we keep adding new extensions (i.e. rv64imafdch with kvm patches). > Good Linux kernel patches are driven by clear functional > motivations. > Like, "The current kernel crashes or doesn't support the hardware in > situation X; thus we change the kernel to add feature or bugfix > Y." And > in the kernel, solutions that involve fewer lines of code are > generally > preferred to solutions that involve more lines of code - more bugs, > more > noise, etc. > I completely agree with you on this. > Where these case-insensitivity parsing patches fall short, in my > opinion, > is that they don't have strong functional motivations. There's been > a > precedent for a few years now in the mainline kernel that the RISC-V > ISA > string is all lowercase. I've asked you and Anup for situations > where > that precedent isn't sufficient to handle functionality that's > described > in the RISC-V specification, or to phrase it differently, "what > breaks if > we don't make this change?" So far no one's been able to cite > anything > here. There's just a repeated appeal to authority to the section of > the > RISC-V specification that states that "[t]he ISA naming strings are > case > insensitive." As you can probably sense, this doesn't seem like a > strong > justification for changing the existing behavior. From a functional > point > of view, if case doesn't matter, why care if the DT data and kernel > only > support lowercase strings? An all-lowercase string should be > functionally > equivalent to an all-uppercase or mixed-case string. Since there's > no > functional point to the changes,cause mysterious DT parsing problems. > why add more code to the kernel? > There was no immediate functional requirement but to have a more future proof code. As I said earlier, the idea of the patch came from the confusion created by discrepancies between two different piece of documentation/specification. As long those are resolved, absolutely no need of this patch. > Later in your E-mail, it sounds like you ultimately agree with these > basic > conclusions. If that's so, I don't understand the amount of effort > that > you and Anup have put into pushing back here. There's nothing > personal > about these review comments. If there's some meta-problem here > that's > unrelated to the technical merit of the patches, please send a > private > E-mail. Otherwise, if you disagree with the functional conclusions > in the > previous paragraph, let's hash the issues out here. > > > I am fine with dropping this patch if yaml clearly document the > > case > > sensititve thing. > > Great! If you still think so now, let's resolve this issue with a > one-line patch to the DT YAML schema to note that riscv,isa DT > string > values should be all lowercase. Will you send a patch? > Yeah. Sending it right now. Regards, Atish > > - Paul
Re: [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing.
On Mon, 29 Jul 2019, Atish Patra wrote: > The yaml document did not specify anything about all isa-strings has to > be lowercase unless I missed something. The two enum values are all > lowercase but the description says all ISA strings are documented in ISA > specification which describes the ISA strings to be case insensitive. > IMHO, this creates confusion resulting the patch. If it's helpful in understanding my earlier comments, I don't think that your patches were "wrong," or anything like that. And you're right that the DT YAML binding does not unequivocally state that all future additions to the riscv,isa string must be in lowercase. But to be clear, the DT YAML schema defines exactly what is currently permissible for riscv,isa strings in kernel-oriented DT data, and right now, all of the permissible values are lowercase. Good Linux kernel patches are driven by clear functional motivations. Like, "The current kernel crashes or doesn't support the hardware in situation X; thus we change the kernel to add feature or bugfix Y." And in the kernel, solutions that involve fewer lines of code are generally preferred to solutions that involve more lines of code - more bugs, more noise, etc. Where these case-insensitivity parsing patches fall short, in my opinion, is that they don't have strong functional motivations. There's been a precedent for a few years now in the mainline kernel that the RISC-V ISA string is all lowercase. I've asked you and Anup for situations where that precedent isn't sufficient to handle functionality that's described in the RISC-V specification, or to phrase it differently, "what breaks if we don't make this change?" So far no one's been able to cite anything here. There's just a repeated appeal to authority to the section of the RISC-V specification that states that "[t]he ISA naming strings are case insensitive." As you can probably sense, this doesn't seem like a strong justification for changing the existing behavior. From a functional point of view, if case doesn't matter, why care if the DT data and kernel only support lowercase strings? An all-lowercase string should be functionally equivalent to an all-uppercase or mixed-case string. Since there's no functional point to the changes, why add more code to the kernel? Later in your E-mail, it sounds like you ultimately agree with these basic conclusions. If that's so, I don't understand the amount of effort that you and Anup have put into pushing back here. There's nothing personal about these review comments. If there's some meta-problem here that's unrelated to the technical merit of the patches, please send a private E-mail. Otherwise, if you disagree with the functional conclusions in the previous paragraph, let's hash the issues out here. > I am fine with dropping this patch if yaml clearly document the case > sensititve thing. Great! If you still think so now, let's resolve this issue with a one-line patch to the DT YAML schema to note that riscv,isa DT string values should be all lowercase. Will you send a patch? - Paul
Re: [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing.
On Sat, 27 Jul 2019, Anup Patel wrote: > On Sat, Jul 27, 2019 at 1:46 PM Paul Walmsley > wrote: > > > > On Sat, 27 Jul 2019, Anup Patel wrote: > > > > > If your only objection is uppercase letter not agreeing with YMAL > > > schema then why not fix the YMAL schema to have regex for RISC-V ISA > > > string? > > > > I don't agree with you that the specification compels software to > > accept arbitrary case combinations in the riscv,isa DT string. > > DT describes HW and HW follows RISC-V spec. The RISC-V specification doesn't specify anything about how the DT data is to describe the hardware. > Enforcing software choices in DT YMAL schema is not correct approach. That's the point of the DT YAML schemas. - Paul
Re: [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing.
On Mon, 2019-07-29 at 20:42 -0700, Palmer Dabbelt wrote: > On Fri, 26 Jul 2019 15:20:47 PDT (-0700), Atish Patra wrote: > > On 7/26/19 1:47 PM, Paul Walmsley wrote: > > > On Fri, 26 Jul 2019, Atish Patra wrote: > > > > > > > As per riscv specification, ISA naming strings are > > > > case insensitive. However, currently only lower case > > > > strings are parsed during cpu procfs. > > > > > > > > Support parsing of upper case letters as well. > > > > > > > > Signed-off-by: Atish Patra > > > > > > Is there a use case that's driving this, or > > > > Currently, we use all lower case isa string in kvmtool. But > > somebody can > > have uppercase letters in future as spec allows it. > > > > > > can we just say, "use > > > lowercase letters" and leave it at that? > > > > > > > In that case, it will not comply with RISC-V spec. Is that okay ? > > We could make the platform spec say "use lowercase letters" and wipe > our hands > of it -- IIRC we still only support the lower case letters in GCC due > to > multilib headaches, so it's kind of the de-facto standard already. > Sounds good. That's what I suggested in earlier email as well. It would be good to add "use lowercase letters" in yaml documentation as well just to avoid any confusion at all. I will send a v2 soon. Regards, Atish > > > - Paul > > >
Re: [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing.
On Fri, 26 Jul 2019 15:20:47 PDT (-0700), Atish Patra wrote: On 7/26/19 1:47 PM, Paul Walmsley wrote: On Fri, 26 Jul 2019, Atish Patra wrote: As per riscv specification, ISA naming strings are case insensitive. However, currently only lower case strings are parsed during cpu procfs. Support parsing of upper case letters as well. Signed-off-by: Atish Patra Is there a use case that's driving this, or Currently, we use all lower case isa string in kvmtool. But somebody can have uppercase letters in future as spec allows it. can we just say, "use lowercase letters" and leave it at that? In that case, it will not comply with RISC-V spec. Is that okay ? We could make the platform spec say "use lowercase letters" and wipe our hands of it -- IIRC we still only support the lower case letters in GCC due to multilib headaches, so it's kind of the de-facto standard already. - Paul
Re: [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing.
On Sat, 2019-07-27 at 01:16 -0700, Paul Walmsley wrote: > On Sat, 27 Jul 2019, Anup Patel wrote: > > > If your only objection is uppercase letter not agreeing with YMAL > > schema > > then why not fix the YMAL schema to have regex for RISC-V ISA > > string? > > I don't agree with you that the specification compels software to > accept > arbitrary case combinations in the riscv,isa DT string. > > > The YMAL schema should not enforce any artificial restriction which > > is > > theoretically allowed in the RISC-V spec. > > Unless someone can come up with a compelling reason for why > restricting > the DT ISA strings to all lowercase letters and numbers is > insufficient to > express the full range of options in the spec, The yaml document did not specify anything about all isa-strings has to be lowercase unless I missed something. The two enum values are all lowercase but the description says all ISA strings are documented in ISA specification which describes the ISA strings to be case insensitive. IMHO, this creates confusion resulting the patch. The existing enum strings are already outdated with kvm patchset. I am fine with dropping this patch if yaml clearly document the case sensititve thing. Following approaches can done to avoid this confusion in future. 1. Update the enum strings with every new extension added. - Good chance that somebody miss something and this gets outdated in future. 2. Just add one line in DT binding description saying that "All isa strings has to be lowercase strings". We should mandate this in Unix Platform specification as well to be in sync. Thoughts ? > the additional complexity > to add mixed-case parsing, both in this patch and in the other > patches in > this series, seems pointless. > > > - Paul > > ___ > linux-riscv mailing list > linux-ri...@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv -- Regards, Atish
Re: [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing.
On Jul 27 2019, Anup Patel wrote: > So, using strncasecmp() in-place of strncmp() and using tolower() for > each character comparison is complex for you ? Apparently too complex for you. + if (tolower(isa[0] != 's')) Andreas. -- Andreas Schwab, SUSE Labs, sch...@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different."
Re: [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing.
On Sat, Jul 27, 2019 at 1:46 PM Paul Walmsley wrote: > > On Sat, 27 Jul 2019, Anup Patel wrote: > > > If your only objection is uppercase letter not agreeing with YMAL schema > > then why not fix the YMAL schema to have regex for RISC-V ISA string? > > I don't agree with you that the specification compels software to accept > arbitrary case combinations in the riscv,isa DT string. DT describes HW and HW follows RISC-V spec. Enforcing software choices in DT YMAL schema is not correct approach. Some other OS (such as FreeBSD, NetBSD, etc) might choose to go with upper-case characters only in their DTS files. > > > The YMAL schema should not enforce any artificial restriction which is > > theoretically allowed in the RISC-V spec. > > Unless someone can come up with a compelling reason for why restricting > the DT ISA strings to all lowercase letters and numbers is insufficient to > express the full range of options in the spec, the additional complexity > to add mixed-case parsing, both in this patch and in the other patches in > this series, seems pointless. So, using strncasecmp() in-place of strncmp() and using tolower() for each character comparison is complex for you ? Why do we need a pointless restriction in YAML schema ? Regards, Anup
Re: [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing.
On Sat, 27 Jul 2019, Anup Patel wrote: > If your only objection is uppercase letter not agreeing with YMAL schema > then why not fix the YMAL schema to have regex for RISC-V ISA string? I don't agree with you that the specification compels software to accept arbitrary case combinations in the riscv,isa DT string. > The YMAL schema should not enforce any artificial restriction which is > theoretically allowed in the RISC-V spec. Unless someone can come up with a compelling reason for why restricting the DT ISA strings to all lowercase letters and numbers is insufficient to express the full range of options in the spec, the additional complexity to add mixed-case parsing, both in this patch and in the other patches in this series, seems pointless. - Paul
Re: [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing.
On Sat, Jul 27, 2019 at 1:23 PM Paul Walmsley wrote: > > On Sat, 27 Jul 2019, Anup Patel wrote: > > > > -Original Message- > > > From: Paul Walmsley > > > Sent: Saturday, July 27, 2019 5:00 AM > > > > > > On Fri, 26 Jul 2019, Atish Patra wrote: > > > > > > > On 7/26/19 1:47 PM, Paul Walmsley wrote: > > > > > On Fri, 26 Jul 2019, Atish Patra wrote: > > > > > > > > > > > As per riscv specification, ISA naming strings are case > > > > > > insensitive. However, currently only lower case strings are parsed > > > > > > during cpu procfs. > > > > > > > > > > > > Support parsing of upper case letters as well. > > > > > > > > > > > > Signed-off-by: Atish Patra > > > > > > > > > > Is there a use case that's driving this, or > > > > > > > > Currently, we use all lower case isa string in kvmtool. But somebody > > > > can have uppercase letters in future as spec allows it. > > > > > > > > > > > > can we just say, "use > > > > > lowercase letters" and leave it at that? > > > > > > > > > > > > > In that case, it will not comply with RISC-V spec. Is that okay ? > > > > > > I think that section of the specification is mostly concerned with someone > > > trying to define "f" as a different extension than "F", or something like > > > that. > > > I'm not sure that it imposes any constraint that software must accept both > > > upper and lower case ISA strings. > > > > > > What gives me pause here is that this winds up impacting DT schema > > > validation: > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docu > > > mentation/devicetree/bindings/riscv/cpus.yaml#n41 > > > > If 'f' and 'F' mean same extension as-per RISC-V spec then software should > > also > > interpret it that way hence this patch. > > The list of valid RISC-V ISA strings is already constrained by the DT > schema to be all lowercase. Anything else would violate the schema. > > I'd take a patch that would pr_warn() or a BUG() if any uppercase letters > were found in the riscv,isa DT property, though, in case the developer > skipped the DT schema validator. If your only objection is uppercase letter not agreeing with YMAL schema then why not fix the YMAL schema to have regex for RISC-V ISA string? The YMAL schema should not enforce any artificial restriction which is theoretically allowed in the RISC-V spec. Regards, Anup
RE: [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing.
On Sat, 27 Jul 2019, Anup Patel wrote: > > -Original Message- > > From: Paul Walmsley > > Sent: Saturday, July 27, 2019 5:00 AM > > > > On Fri, 26 Jul 2019, Atish Patra wrote: > > > > > On 7/26/19 1:47 PM, Paul Walmsley wrote: > > > > On Fri, 26 Jul 2019, Atish Patra wrote: > > > > > > > > > As per riscv specification, ISA naming strings are case > > > > > insensitive. However, currently only lower case strings are parsed > > > > > during cpu procfs. > > > > > > > > > > Support parsing of upper case letters as well. > > > > > > > > > > Signed-off-by: Atish Patra > > > > > > > > Is there a use case that's driving this, or > > > > > > Currently, we use all lower case isa string in kvmtool. But somebody > > > can have uppercase letters in future as spec allows it. > > > > > > > > > can we just say, "use > > > > lowercase letters" and leave it at that? > > > > > > > > > > In that case, it will not comply with RISC-V spec. Is that okay ? > > > > I think that section of the specification is mostly concerned with someone > > trying to define "f" as a different extension than "F", or something like > > that. > > I'm not sure that it imposes any constraint that software must accept both > > upper and lower case ISA strings. > > > > What gives me pause here is that this winds up impacting DT schema > > validation: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docu > > mentation/devicetree/bindings/riscv/cpus.yaml#n41 > > If 'f' and 'F' mean same extension as-per RISC-V spec then software should > also > interpret it that way hence this patch. The list of valid RISC-V ISA strings is already constrained by the DT schema to be all lowercase. Anything else would violate the schema. I'd take a patch that would pr_warn() or a BUG() if any uppercase letters were found in the riscv,isa DT property, though, in case the developer skipped the DT schema validator. - Paul
RE: [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing.
> -Original Message- > From: Paul Walmsley > Sent: Saturday, July 27, 2019 5:00 AM > To: Atish Patra > Cc: linux-kernel@vger.kernel.org; Alan Kao ; > Albert Ou ; Allison Randal ; > Anup Patel ; Daniel Lezcano > ; Greg Kroah-Hartman > ; Johan Hovold ; linux- > ri...@lists.infradead.org; Palmer Dabbelt ; Thomas > Gleixner > Subject: Re: [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing. > > On Fri, 26 Jul 2019, Atish Patra wrote: > > > On 7/26/19 1:47 PM, Paul Walmsley wrote: > > > On Fri, 26 Jul 2019, Atish Patra wrote: > > > > > > > As per riscv specification, ISA naming strings are case > > > > insensitive. However, currently only lower case strings are parsed > > > > during cpu procfs. > > > > > > > > Support parsing of upper case letters as well. > > > > > > > > Signed-off-by: Atish Patra > > > > > > Is there a use case that's driving this, or > > > > Currently, we use all lower case isa string in kvmtool. But somebody > > can have uppercase letters in future as spec allows it. > > > > > > can we just say, "use > > > lowercase letters" and leave it at that? > > > > > > > In that case, it will not comply with RISC-V spec. Is that okay ? > > I think that section of the specification is mostly concerned with someone > trying to define "f" as a different extension than "F", or something like > that. > I'm not sure that it imposes any constraint that software must accept both > upper and lower case ISA strings. > > What gives me pause here is that this winds up impacting DT schema > validation: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docu > mentation/devicetree/bindings/riscv/cpus.yaml#n41 If 'f' and 'F' mean same extension as-per RISC-V spec then software should also interpret it that way hence this patch. Regards, Anup
Re: [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing.
On Fri, 26 Jul 2019, Atish Patra wrote: > On 7/26/19 1:47 PM, Paul Walmsley wrote: > > On Fri, 26 Jul 2019, Atish Patra wrote: > > > > > As per riscv specification, ISA naming strings are > > > case insensitive. However, currently only lower case > > > strings are parsed during cpu procfs. > > > > > > Support parsing of upper case letters as well. > > > > > > Signed-off-by: Atish Patra > > > > Is there a use case that's driving this, or > > Currently, we use all lower case isa string in kvmtool. But somebody can have > uppercase letters in future as spec allows it. > > > can we just say, "use > > lowercase letters" and leave it at that? > > > > In that case, it will not comply with RISC-V spec. Is that okay ? I think that section of the specification is mostly concerned with someone trying to define "f" as a different extension than "F", or something like that. I'm not sure that it imposes any constraint that software must accept both upper and lower case ISA strings. What gives me pause here is that this winds up impacting DT schema validation: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/riscv/cpus.yaml#n41 - Paul
Re: [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing.
On 7/26/19 1:47 PM, Paul Walmsley wrote: On Fri, 26 Jul 2019, Atish Patra wrote: As per riscv specification, ISA naming strings are case insensitive. However, currently only lower case strings are parsed during cpu procfs. Support parsing of upper case letters as well. Signed-off-by: Atish Patra Is there a use case that's driving this, or Currently, we use all lower case isa string in kvmtool. But somebody can have uppercase letters in future as spec allows it. can we just say, "use lowercase letters" and leave it at that? In that case, it will not comply with RISC-V spec. Is that okay ? - Paul -- Regards, Atish
Re: [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing.
On Fri, 26 Jul 2019, Atish Patra wrote: > As per riscv specification, ISA naming strings are > case insensitive. However, currently only lower case > strings are parsed during cpu procfs. > > Support parsing of upper case letters as well. > > Signed-off-by: Atish Patra Is there a use case that's driving this, or can we just say, "use lowercase letters" and leave it at that? - Paul
[PATCH 3/4] RISC-V: Support case insensitive ISA string parsing.
As per riscv specification, ISA naming strings are
case insensitive. However, currently only lower case
strings are parsed during cpu procfs.
Support parsing of upper case letters as well.
Signed-off-by: Atish Patra
---
arch/riscv/kernel/cpu.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
index 7da3c6a93abd..185143478830 100644
--- a/arch/riscv/kernel/cpu.c
+++ b/arch/riscv/kernel/cpu.c
@@ -5,6 +5,7 @@
#include
#include
+#include
#include
#include
@@ -57,10 +58,10 @@ static void print_isa(struct seq_file *f, const char
*orig_isa)
* kernels on harts with the same ISA that the kernel is compiled for.
*/
#if defined(CONFIG_32BIT)
- if (strncmp(isa, "rv32i", 5) != 0)
+ if (strncasecmp(isa, "rv32i", 5) != 0)
return;
#elif defined(CONFIG_64BIT)
- if (strncmp(isa, "rv64i", 5) != 0)
+ if (strncasecmp(isa, "rv64i", 5) != 0)
return;
#endif
@@ -76,8 +77,8 @@ static void print_isa(struct seq_file *f, const char
*orig_isa)
* extension from userspace as it's not accessible from there.
*/
for (e = ext; *e != '\0'; ++e) {
- if (isa[0] == e[0]) {
- if (isa[0] != 's')
+ if (tolower(isa[0]) == e[0]) {
+ if (tolower(isa[0] != 's'))
seq_write(f, isa, 1);
isa++;
--
2.21.0
