From: Atish Patra <Atish.Patra@wdc.com>
To: "email@example.com" <firstname.lastname@example.org>
Anup Patel <Anup.Patel@wdc.com>,
Subject: Re: [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing.
Date: Wed, 31 Jul 2019 00:34:57 +0000 [thread overview]
Message-ID: <email@example.com> (raw)
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
> your patches were "wrong," or anything like that. And you're right
> the DT YAML binding does not unequivocally state that all future
> to the riscv,isa string must be in lowercase. But to be clear, the
> YAML schema defines exactly what is currently permissible for
> strings in kernel-oriented DT data, and right now, all of the
> 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
> 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
> preferred to solutions that involve more lines of code - more bugs,
> noise, etc.
I completely agree with you on this.
> Where these case-insensitivity parsing patches fall short, in my
> is that they don't have strong functional motivations. There's been
> precedent for a few years now in the mainline kernel that the RISC-V
> string is all lowercase. I've asked you and Anup for situations
> that precedent isn't sufficient to handle functionality that's
> 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
> here. There's just a repeated appeal to authority to the section of
> RISC-V specification that states that "[t]he ISA naming strings are
> insensitive." As you can probably sense, this doesn't seem like a
> justification for changing the existing behavior. From a functional
> of view, if case doesn't matter, why care if the DT data and kernel
> support lowercase strings? An all-lowercase string should be
> equivalent to an all-uppercase or mixed-case string. Since there's
> 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
> conclusions. If that's so, I don't understand the amount of effort
> you and Anup have put into pushing back here. There's nothing
> about these review comments. If there's some meta-problem here
> unrelated to the technical merit of the patches, please send a
> 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
> values should be all lowercase. Will you send a patch?
Yeah. Sending it right now.
> - Paul
next prev parent reply other threads:[~2019-07-31 0:35 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-26 19:46 [PATCH 1/4] RISC-V: Remove per cpu clocksource Atish Patra
2019-07-26 19:46 ` [PATCH 2/4] RISC-V: Add riscv_isa reprensenting ISA features common across CPUs Atish Patra
2019-07-26 19:46 ` [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing Atish Patra
2019-07-26 20:47 ` Paul Walmsley
2019-07-26 22:20 ` Atish Patra
2019-07-26 23:29 ` Paul Walmsley
2019-07-27 2:23 ` Anup Patel
2019-07-27 7:52 ` Paul Walmsley
2019-07-27 8:05 ` Anup Patel
2019-07-27 8:16 ` Paul Walmsley
2019-07-27 8:49 ` Anup Patel
2019-07-29 14:03 ` Andreas Schwab
2019-07-30 22:58 ` Paul Walmsley
2019-07-29 18:31 ` Atish Patra
2019-07-31 0:08 ` Paul Walmsley
2019-07-31 0:34 ` Atish Patra [this message]
2019-07-30 3:42 ` Palmer Dabbelt
2019-07-30 20:41 ` Atish Patra
2019-07-26 19:46 ` [PATCH 4/4] RISC-V: Fix unsupported isa string info Atish Patra
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).