From: Atish Patra <Atish.Patra@wdc.com>
To: "paul.walmsley@sifive.com" <paul.walmsley@sifive.com>
Cc: "daniel.lezcano@linaro.org" <daniel.lezcano@linaro.org>,
"alankao@andestech.com" <alankao@andestech.com>,
"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
Anup Patel <Anup.Patel@wdc.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"johan@kernel.org" <johan@kernel.org>,
"aou@eecs.berkeley.edu" <aou@eecs.berkeley.edu>,
"palmer@sifive.com" <palmer@sifive.com>,
"anup@brainfault.org" <anup@brainfault.org>,
"linux-riscv@lists.infradead.org"
<linux-riscv@lists.infradead.org>,
"tglx@linutronix.de" <tglx@linutronix.de>,
"allison@lohutok.net" <allison@lohutok.net>
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: <021f5c82273d643b28567a3dd05254492d53bf5c.camel@wdc.com> (raw)
In-Reply-To: <alpine.DEB.2.21.9999.1907301611040.4874@viisi.sifive.com>
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
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
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
Reply instructions:
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:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=021f5c82273d643b28567a3dd05254492d53bf5c.camel@wdc.com \
--to=atish.patra@wdc.com \
--cc=Anup.Patel@wdc.com \
--cc=alankao@andestech.com \
--cc=allison@lohutok.net \
--cc=anup@brainfault.org \
--cc=aou@eecs.berkeley.edu \
--cc=daniel.lezcano@linaro.org \
--cc=gregkh@linuxfoundation.org \
--cc=johan@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=palmer@sifive.com \
--cc=paul.walmsley@sifive.com \
--cc=tglx@linutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* 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).