From: Atish Patra <Atish.Patra@wdc.com> To: "paul.walmsley@sifive.com" <paul.walmsley@sifive.com> Cc: "linux-riscv@lists.infradead.org" <linux-riscv@lists.infradead.org>, "daniel.lezcano@linaro.org" <daniel.lezcano@linaro.org>, "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>, Anup Patel <Anup.Patel@wdc.com>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "aou@eecs.berkeley.edu" <aou@eecs.berkeley.edu>, "johan@kernel.org" <johan@kernel.org>, "anup@brainfault.org" <anup@brainfault.org>, "tglx@linutronix.de" <tglx@linutronix.de>, "alankao@andestech.com" <alankao@andestech.com>, "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: 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
WARNING: multiple messages have this Message-ID (diff)
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: 38+ 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 ` 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 ` Atish Patra 2019-07-26 19:46 ` [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing Atish Patra 2019-07-26 19:46 ` Atish Patra 2019-07-26 20:47 ` Paul Walmsley 2019-07-26 20:47 ` Paul Walmsley 2019-07-26 22:20 ` Atish Patra 2019-07-26 22:20 ` Atish Patra 2019-07-26 23:29 ` Paul Walmsley 2019-07-26 23:29 ` Paul Walmsley 2019-07-27 2:23 ` Anup Patel 2019-07-27 2:23 ` Anup Patel 2019-07-27 7:52 ` Paul Walmsley 2019-07-27 7:52 ` Paul Walmsley 2019-07-27 8:05 ` Anup Patel 2019-07-27 8:05 ` Anup Patel 2019-07-27 8:16 ` Paul Walmsley 2019-07-27 8:16 ` Paul Walmsley 2019-07-27 8:49 ` Anup Patel 2019-07-27 8:49 ` Anup Patel 2019-07-29 14:03 ` Andreas Schwab 2019-07-29 14:03 ` Andreas Schwab 2019-07-30 22:58 ` Paul Walmsley 2019-07-30 22:58 ` Paul Walmsley 2019-07-29 18:31 ` Atish Patra 2019-07-29 18:31 ` Atish Patra 2019-07-31 0:08 ` Paul Walmsley 2019-07-31 0:08 ` Paul Walmsley 2019-07-31 0:34 ` Atish Patra [this message] 2019-07-31 0:34 ` Atish Patra 2019-07-30 3:42 ` Palmer Dabbelt 2019-07-30 3:42 ` Palmer Dabbelt 2019-07-30 20:41 ` Atish Patra 2019-07-30 20:41 ` Atish Patra 2019-07-26 19:46 ` [PATCH 4/4] RISC-V: Fix unsupported isa string info Atish Patra 2019-07-26 19:46 ` 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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.