linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Paul Walmsley <paul.walmsley@sifive.com>
To: Atish Patra <Atish.Patra@wdc.com>
Cc: "anup@brainfault.org" <anup@brainfault.org>,
	"alankao@andestech.com" <alankao@andestech.com>,
	"daniel.lezcano@linaro.org" <daniel.lezcano@linaro.org>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	"linux-riscv@lists.infradead.org"
	<linux-riscv@lists.infradead.org>,
	Anup Patel <Anup.Patel@wdc.com>,
	"aou@eecs.berkeley.edu" <aou@eecs.berkeley.edu>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"johan@kernel.org" <johan@kernel.org>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"palmer@sifive.com" <palmer@sifive.com>,
	"allison@lohutok.net" <allison@lohutok.net>
Subject: Re: [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing.
Date: Tue, 30 Jul 2019 17:08:42 -0700 (PDT)	[thread overview]
Message-ID: <alpine.DEB.2.21.9999.1907301611040.4874@viisi.sifive.com> (raw)
In-Reply-To: <8ed4d461ffe5ac41b475d22b38019578b29a8d09.camel@wdc.com>

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

  reply	other threads:[~2019-07-31  0:08 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 [this message]
2019-07-31  0:34                     ` Atish Patra
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=alpine.DEB.2.21.9999.1907301611040.4874@viisi.sifive.com \
    --to=paul.walmsley@sifive.com \
    --cc=Anup.Patel@wdc.com \
    --cc=Atish.Patra@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=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).