lkml.org 
[lkml]   [2019]   [Jul]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing.
Date
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

\
 
 \ /
  Last update: 2019-07-31 02:35    [W:0.070 / U:0.112 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site