Re: [PATCH v2] ArmVirtPkg/PlatformHasAcpiDtDxe: allow guest level ACPI disable override

[Laszlo Ersek <lersek@redhat.com>]

On 03/30/17 10:40, Ard Biesheuvel wrote:
> On 29 March 2017 at 20:35, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 03/29/17 21:10, Ard Biesheuvel wrote:
> [...]
>>>
>>> How on earth is having two ways to disable ACPI rather than one going
>>> to cause fragmentation? Unlike v1, this patch does not allow you to
>>> expose both DT and ACPI tables at the same time.
>>
>> Oopsie daisy. You actually updated the commit message too. (I have now
>> formally diffed v1 vs. v2, including commit msg -- I generally do that
>> when reviewing incremental versions of patches, but it has been a very
>> long day, and I failed to get my mind off the track set up by v1). I got
>> really no good explanation for missing the fundamental logic change
>> between v1 and v2. As you say, version 2 preserves the mutual exclusion
>> between DT and ACPI that I'm so annoyingly obsessed about. Thank you for
>> the update.
>>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
> 
> Thanks Laszlo. I am glad we have a solution we can both live with.
> 
> I will wait for Marc to confirm that this works as expected for him.

Good idea.

In order to save some adrenaline down the line for both of us, I have
some suggestions:

- Please try clarify with the reporter of the regression what he or she
prefers as a solution, before giving me a heart attack :)

Regarding the "NACK" in all caps -- I wasn't yelling, that's just a way
of formatting we use downstream (we mostly use ACK and NACK), which
regrettably leaked into my upstream correspondence. Sorry about the
confusion.

(NB, I'm not apologizing for nacking v1 per se. There's a world of
difference between exposing the exlusivity with some additional switch
and getting cold feet on the exclusivity en bloc. In my opinion.)

- Please always include an incremental v2, v3, ... notes / info section
in the patch (or blurb, if there is one), so I can more easily find out
about the inter-version changes near the end of a 14 hour work day.
(When I finally went to bed my uptime was past 18 hours.)

In this instance, there was no v2 info section, and I thought you only
addressed the superficial technical suggestions that I made under v1.

*Importantly*, this is not to say that I did not do a shit job at
reviewing v2. I absolutely did. Lack of a v2 info section in the patch /
blurb is no excuse for missing the -- happy! -- elefant in the room.
It's quite embarrassing; I'm sorry about that. I'll strive to do the
formal v1<->v2 diffing in the future unconditionally.

Thanks,
Laszlo