Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3 config error

[Laszlo Ersek <lersek@redhat.com>]

On 09/12/18 02:49, Wang, Jian J wrote:
> Laszlo,
> 
> Thanks for the comments.
> 
> 
> (1)    Agree. It’ll be updated in v2.
> 
> (2)    DEBUG_ERROR level won’t print word “ERROR” on console. I think an “ERROR”
> 
> word in log should get the attention more easily.
> 
> (3)    How about log both of them? GUID value may be more friendly to log parser but
> a GUID name is more friendly to person.
> 
> (4)    Good idea. I’ll add it in v2.

Those work for me as well.

Thanks
Laszlo

> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, September 11, 2018 10:01 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>; You, Benjamin <benjamin.you@intel.com>; Dong, Eric <eric.dong@intel.com>
> Subject: Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3 config error
> 
> On 09/10/18 05:22, Jian J Wang wrote:
>> BZ#: https://bugzilla.tianocore.org/show_bug.cgi?id=1165
>>
>> HOB gEfiAcpiVariableGuid is a must have data for S3 resume if
>> PcdAcpiS3Enable is set to TRUE. Current code in CpuS3.c doesn't
>> embody this strong binding between them. An error message and
>> ASSERT is added by this patch to warn platform developer about
>> it.
>>
>> Cc: Star Zeng <star.zeng@intel.com<mailto:star.zeng@intel.com>>
>> Cc: Benjamin You <benjamin.you@intel.com<mailto:benjamin.you@intel.com>>
>> Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>>
>> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Jian J Wang <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>
>> ---
>>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
>> index abd8a5a07b..f371667c44 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
>> @@ -744,6 +744,9 @@ InitSmmS3ResumeState (
>>      if (sizeof (UINTN) == sizeof (UINT32)) {
>>        SmmS3ResumeState->Signature = SMM_S3_RESUME_SMM_32;
>>      }
>> +  } else {
>> +    DEBUG ((DEBUG_ERROR, "ERROR: HOB(gEfiAcpiVariableGuid) needed by S3 resume doesn't exist!\n"));
>> +    ASSERT (FALSE);
>>    }
>>
>>    //
>>
> 
> I have some superficial comments for this patch.
> 
> (1) in case the "if" has an "else" branch, I think it's better style to
> use "==" in the condition than "!=". To me,
> 
>   if (GuidHob != NULL) {
>     //
>     // do a bunch of stuff
>     //
>   } else {
>     //
>     // error
>     //
>   }
> 
> is more difficult to read than:
> 
>   if (GuidHob == NULL) {
>     //
>     // error
>     //
>   } else {
>     //
>     // do a bunch of stuff
>     //
>   }
> 
> in particular if the "bunch of stuff" includes nested "if" statements.
> 
> 
> (2) I think the error message could be improved. I'd suggest removing
> the word "ERROR", since DEBUG_ERROR already sets the error level / mask.
> 
> (3) Furthermore, I suggest not logging the name "gEfiAcpiVariableGuid"
> textually, as a string -- in edk2 we generally log GUIDs by value, and
> log parsers / translators usually look for GUID values. Thus I suggest
> using %g with &gEfiAcpiVariableGuid.
> 
> (4) Please consider logging the module name and/or the function name
> too, with "%a", and gEfiCallerBaseName and/or __FUNCTION__.
> "gEfiCallerBaseName" is usually only helpful with libraries (because
> they can be linked into multiple drivers), but __FUNCTION__ is more
> frequently helpful.
> 
> Thanks
> Laszlo
>