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 <[email protected]>
Is there a use case that's driving this, or can we just say, "use
lowercase letters" and leave it at that?
- Paul
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 <[email protected]>
>
> 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
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 <[email protected]>
> >
> > 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
> -----Original Message-----
> From: Paul Walmsley <[email protected]>
> Sent: Saturday, July 27, 2019 5:00 AM
> To: Atish Patra <[email protected]>
> Cc: [email protected]; Alan Kao <[email protected]>;
> Albert Ou <[email protected]>; Allison Randal <[email protected]>;
> Anup Patel <[email protected]>; Daniel Lezcano
> <[email protected]>; Greg Kroah-Hartman
> <[email protected]>; Johan Hovold <[email protected]>; linux-
> [email protected]; Palmer Dabbelt <[email protected]>; Thomas
> Gleixner <[email protected]>
> 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 <[email protected]>
> > >
> > > 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
On Sat, 27 Jul 2019, Anup Patel wrote:
> > -----Original Message-----
> > From: Paul Walmsley <[email protected]>
> > 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 <[email protected]>
> > > >
> > > > 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
On Sat, Jul 27, 2019 at 1:23 PM Paul Walmsley <[email protected]> wrote:
>
> On Sat, 27 Jul 2019, Anup Patel wrote:
>
> > > -----Original Message-----
> > > From: Paul Walmsley <[email protected]>
> > > 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 <[email protected]>
> > > > >
> > > > > 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
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
On Sat, Jul 27, 2019 at 1:46 PM Paul Walmsley <[email protected]> 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
On Jul 27 2019, Anup Patel <[email protected]> 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, [email protected]
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
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
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
--
Regards,
Atish
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 <[email protected]>
>>
>> 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
>>
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 <[email protected]>
> > >
> > > 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
> > >
On Sat, 27 Jul 2019, Anup Patel wrote:
> On Sat, Jul 27, 2019 at 1:46 PM Paul Walmsley <[email protected]> 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
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
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