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
