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:[email protected]] > Sent: Tuesday, September 11, 2018 10:01 PM > To: Wang, Jian J <[email protected]>; [email protected] > Cc: Zeng, Star <[email protected]>; You, Benjamin <[email protected]>; > Dong, Eric <[email protected]> > 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 <[email protected]<mailto:[email protected]>> >> Cc: Benjamin You <[email protected]<mailto:[email protected]>> >> Cc: Eric Dong <[email protected]<mailto:[email protected]>> >> Cc: Laszlo Ersek <[email protected]<mailto:[email protected]>> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Jian J Wang >> <[email protected]<mailto:[email protected]>> >> --- >> 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 [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
