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. Regards, Jian From: Laszlo Ersek [mailto:ler...@redhat.com] Sent: Tuesday, September 11, 2018 10:01 PM To: Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org Cc: Zeng, Star <star.z...@intel.com>; You, Benjamin <benjamin....@intel.com>; Dong, Eric <eric.d...@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.z...@intel.com<mailto:star.z...@intel.com>> > Cc: Benjamin You <benjamin....@intel.com<mailto:benjamin....@intel.com>> > Cc: Eric Dong <eric.d...@intel.com<mailto:eric.d...@intel.com>> > Cc: Laszlo Ersek <ler...@redhat.com<mailto:ler...@redhat.com>> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jian J Wang > <jian.j.w...@intel.com<mailto:jian.j.w...@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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
