From: Ingo Molnar <mingo@kernel.org>
To: Matthew Garrett <matthew.garrett@coreos.com>
Cc: Matthew Garrett <mjg59@coreos.com>,
hpa@zytor.com, yinghai@kernel.org, x86@kernel.org,
mingo@redhat.com, tglx@linutronix.de,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH V2] x86: Allow built-in command line to work in early kernel init
Date: Wed, 3 Jun 2015 11:24:51 +0200 [thread overview]
Message-ID: <20150603092450.GB13837@gmail.com> (raw)
In-Reply-To: <CAPeXnHtmsfFao6v=Ef8h-RDaaj8DO4baVu6asUsnHjJFYLLTXQ@mail.gmail.com>
* Matthew Garrett <matthew.garrett@coreos.com> wrote:
> >> + while (c) {
> >> + set_fs(new_cmdline_ptr >> 4);
> >> + wrfs8(c, nptr++);
> >> + set_fs(cmdline_ptr >> 4);
> >> + c = rdfs8(cptr++);
> >> + }
> >> + set_fs(new_cmdline_ptr >> 4);
> >> + wrfs8(' ', nptr++);
> >> + }
> >
> > So here we copy from 'cptr' to 'nptr'? Not very well named variables,
> > at minimum.
>
> I have the choice between preserving existing style or nice names.
> I'm happy to pick either.
So my high level concerns (and conditions) are the following:
We should first do cleanups and reorganization so that the feature addition
(allowing EFI to parse certain options early on) you are implementing becomes
obvious and simple.
I.e. we need to first prove it's all maintainable and readable, and then only can
we add a feature with such a signature:
13 files changed, 236 insertions(+), 43 deletions(-)
I'd also expect that a good chunk of that linecount increase would disappear after
a proper set of reorganization, because all that duplication would disappear
mostly.
It's all pretty non-trivial, so it has to be finegrained, perfectly bisectable,
etc.
Thanks,
Ingo
next prev parent reply other threads:[~2015-06-03 9:25 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-05-19 23:09 [PATCH V2] x86: Allow built-in command line to work in early kernel init Matthew Garrett
2015-05-20 7:42 ` Ingo Molnar
2015-05-27 23:33 ` Matthew Garrett
2015-06-03 9:24 ` Ingo Molnar [this message]
2015-06-03 16:08 ` Matthew Garrett
-- strict thread matches above, loose matches on Subject: below --
2015-04-08 18:41 [PATCH] " Matthew Garrett
2015-04-09 20:25 ` [PATCH V2] " Matthew Garrett
2015-05-11 20:20 ` Matthew Garrett
2015-05-12 9:28 ` Ingo Molnar
2015-05-12 17:39 ` Matthew Garrett
2015-05-12 18:42 ` Matthew Garrett
2015-05-12 23:41 ` Yinghai Lu
2015-05-12 23:53 ` Matthew Garrett
2015-05-13 2:46 ` H. Peter Anvin
2015-05-13 3:06 ` Matthew Garrett
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=20150603092450.GB13837@gmail.com \
--to=mingo@kernel.org \
--cc=hpa@zytor.com \
--cc=linux-kernel@vger.kernel.org \
--cc=matthew.garrett@coreos.com \
--cc=mingo@redhat.com \
--cc=mjg59@coreos.com \
--cc=tglx@linutronix.de \
--cc=x86@kernel.org \
--cc=yinghai@kernel.org \
/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