Re: [PATCH 1/3] OvmfPkg/Tcg2ConfigPei: clean up some lib class dependencies

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 5/21/20 8:04 PM, Laszlo Ersek wrote:
> Hi Phil,
> 
> On 05/21/20 12:29, Philippe Mathieu-Daudé wrote:
>> On 5/21/20 12:58 AM, Laszlo Ersek wrote:
>>> Commit 89236992913f introduced an explicit Tpm12CommandLib dependency to
>>> Tcg2ConfigPei.
>>>
>>> In reality this lib class is not consumed by Tcg2ConfigPei at all (such a
>>> dependency is not even inherited from other lib instances). Simplify the
>>> module by dropping the superfluous dependency.
>>>
>>> (The Tpm12CommandLib class resolution that was also added in commit
>>> 89236992913f is not useless, at the platform build level: it is consumed
>>> by TcgPei and TcgDxe. Meaning that said Tpm12CommandLib resolution should
>>> have likely been a part of the subsequent patch in the original series,
>>> namely commit 6be54f15a0c9.)
>>>
>>> Commit 89236992913f also introduced SwapBytesXx() calls. Those functions
>>> are provided by BaseLib. Spell out the BaseLib dependency.
>>>
>>> Functionally, this patch is a no-op.
>>>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>> Cc: Eric Auger <eric.auger@redhat.com>
>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> Cc: Simon Hardy <simon.hardy@itdev.co.uk>
>>> Cc: Stefan Berger <stefanb@linux.ibm.com>
>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2728
>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>> ---
>>>    OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf | 2 +-
>>>    OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c  | 2 +-
>>>    2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>>> b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>>> index 97c529c91d0b..b79d0a3fb912 100644
>>> --- a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>>> +++ b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>>> @@ -30,9 +30,9 @@ [Packages]
>>>      [LibraryClasses]
>>>      PeimEntryPoint
>>> +  BaseLib
>>>      DebugLib
>>>      PeiServicesLib
>>> -  Tpm12CommandLib
>>>      Tpm12DeviceLib
>>>      Tpm2DeviceLib
>>>    diff --git a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c
>>> b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c
>>> index 5b5075bded92..44abd6c541f9 100644
>>> --- a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c
>>> +++ b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c
>>> @@ -15,11 +15,11 @@
>>>    #include <PiPei.h>
>>>      #include <Guid/TpmInstance.h>
>>> +#include <Library/BaseLib.h>
>>>    #include <Library/DebugLib.h>
>>>    #include <Library/PeiServicesLib.h>
>>>    #include <Library/Tpm2DeviceLib.h>
>>>    #include <Library/Tpm12DeviceLib.h>
>>> -#include <Library/Tpm12CommandLib.h>
>>>    #include <Ppi/TpmInitialized.h>
>>>      STATIC CONST EFI_PEI_PPI_DESCRIPTOR mTpmSelectedPpi = {
>>>
>>
>> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
>>
> 
> I'm extremely sorry for missing your R-b's. When I read your email with
> msgid <60493dcc-0832-1fb3-f380-867410691672@redhat.com>, and
> consequently decided that I'd go ahead with the feedback provided thus
> far, I didn't realize you were going to review the series as well! I
> switched off my email refresh very soon after.
> 
> Normally I let a series sit for at least a day (24 hours) on the list,
> so that everyone (in different timezones too) can at least state an
> intent (even in private) to review or test the series. I encourage other
> maintainers to do the same, and I've complained in the past when I would
> have liked to review or test a series but wasn't given enough time even
> to signal my interest.
> 
> So this is totally my fault. My only excuse is that the feature
> freeze(s) are upon us, and fixing stuff feels "urgent". I wish we could
> append release candidates (RCs) "on-demand", like QEMU does.
> 
> I'm sorry! And thank you for your review.

Don't worry, no problem!

This was an easy refactor to review. As it is a buildfix I wanted to 
review it ASAP so it get merged for the freeze. I'm glad it is merged.

> 
> Laszlo
>