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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 30 March 2017 at 17:16, Laszlo Ersek <lersek@redhat.com> wrote:
> 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.
>

Yeah good point. Keystrokes are cheap ...