Re: [RFC] [PATCH 10/16] x86_64: 64bit PIC ACPI wakeup

[ebiederm@xmission.com (Eric W. Biederman)]

Vivek.  It looks like the patch had whitespace damage sometime in it's
history.  Do you know what happened to the to make the indentation
inconsistent?  I'm pretty certain the version I generated didn't have
that problem.

Pavel Machek <pavel@ucw.cz> writes:

> Hi!
>
>> >> I don't have a configuration I can test this but it compiles cleanly
>> >
>> > Ugh, now that's a big patch.. and untested, too :-(.
>> 
>> It was very carefully code reviewed at least the first time,
>> and the code was put in sync with code that was tested.
>
> So we had two very different versions of "switch to 64-bit" and now we
> have two mostly similar versions. Not a big improvement...

Well we had two versions.  It looks like the acpi wakeup was a cut
and past of the primary kernel entry path, and it diverged and
was less well maintained because it is less considered and get's used
less often.

>> > Can we get it piece-by-piece?
>
> Please?

I honestly think it would make the change  less reviewable.

>> >> Vivek has tested this patch for suspend to memory and it works fine.
>> >
>> > Ok, so it was tested on one config. Given that the patch deals with
>> > detecting CPU oddities... :-(
>> 
>> Read the code.  Given your scorn and the state of that mess when I
>> started I'm not certain a productive conversation can be had.
>> 
>> Do you understand the code as it is currently written?
>
> Mostly. I've written it at some point.
>
> It may be a mess, but patch below is wholesale rewrite, mixing
> cleanups (ebx->rbx) with serious changes (PGE). And then you tell me
> it was tested on one machine. It is hard/impossible to rewrite, and
> changelog is not helpful, either.

So there is one major change in the patch.  Modifying the trampoline
to take the code all of the way to 64bit mode, allowing us to switch
to a kernel that is loaded above 4GB.  Essentially everything else is
required by that change.

The basic point is that I am completely changing the idiom for how
cpus enter the kernel.  If you focus on the details the change and
miss the primary change itself you will simply not understand what
is happening.  The PGE change just happens to be part of the change
in requirements for entering the kernel.

The code paths that are changed are not conditional code they will
always execute.  So a single test tells us a lot.

I just looked at the patch again, and it really is not that large.
At this point I'm afraid splitting it up is more likely to cause
review problems then the other way around.  I don't any of the
cleanups touch a code path I don't need to touch.

As for the hard/impossible to rewrite. It was a bit tedious because
of all of the picky details, but I didn't find it hard.  I suspect this
is just a skill set difference.  Or the fact that I wasn't messing
with the actual code that interfaces with acpi.

Eric