Message-ID: <021f5c82273d643b28567a3dd05254492d53bf5c.camel@wdc.com>
Date:   Wed, 31 Jul 2019 00:34:57 +0000
From:   Atish Patra <Atish.Patra@....com>
To:     "paul.walmsley@...ive.com" <paul.walmsley@...ive.com>
CC:     "linux-riscv@...ts.infradead.org" <linux-riscv@...ts.infradead.org>,
        "daniel.lezcano@...aro.org" <daniel.lezcano@...aro.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        Anup Patel <Anup.Patel@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "aou@...s.berkeley.edu" <aou@...s.berkeley.edu>,
        "johan@...nel.org" <johan@...nel.org>,
        "anup@...infault.org" <anup@...infault.org>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "alankao@...estech.com" <alankao@...estech.com>,
        "palmer@...ive.com" <palmer@...ive.com>,
        "allison@...utok.net" <allison@...utok.net>
Subject: Re: [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing.

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

Powered by blists - more mailing lists