On 01/15/19 09:04, Wang, Jian J wrote: > Star, > > Just a tiny comment below. With it's addressed, > > Reviewed-by: Jian J Wang <jian.j.w...@intel.com> > >> -----Original Message----- >> From: Zeng, Star >> Sent: Monday, January 14, 2019 11:20 PM >> To: edk2-devel@lists.01.org >> Cc: Zeng, Star <star.z...@intel.com>; Wang, Jian J <jian.j.w...@intel.com>; >> Wu, Hao A <hao.a...@intel.com>; Laszlo Ersek <ler...@redhat.com> >> Subject: [PATCH V2 06/15] MdeModulePkg Variable: Remove CacheOffset in >> UpdateVariable() >> >> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1323 >> Merge EmuVariable and Real variable driver. >> >> CacheOffset could be removed in UpdateVariable() after >> // >> // update the memory copy of Flash region. >> // >> CopyMem ( >> (UINT8 *)mNvVariableCache + CacheOffset, >> (UINT8 *)NextVariable, VarSize >> ); >> >> is moved to be before mVariableModuleGlobal->NonVolatileLastVariableOffset >> value is updated, like right before >> >> mVariableModuleGlobal->NonVolatileLastVariableOffset += >> HEADER_ALIGN (VarSize); >> >> This patch prepares for adding emulated variable NV mode >> support in VariableRuntimeDxe. >> >> Cc: Jian J Wang <jian.j.w...@intel.com> >> Cc: Hao Wu <hao.a...@intel.com> >> Cc: Laszlo Ersek <ler...@redhat.com> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Star Zeng <star.z...@intel.com> >> --- >> MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 11 +++++------ >> 1 file changed, 5 insertions(+), 6 deletions(-) >> >> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c >> b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c >> index 424f92a53757..4d524db30fec 100644 >> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c >> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c >> @@ -2139,7 +2139,6 @@ UpdateVariable ( >> VARIABLE_POINTER_TRACK *Variable; >> VARIABLE_POINTER_TRACK NvVariable; >> VARIABLE_STORE_HEADER *VariableStoreHeader; >> - UINTN CacheOffset; >> UINT8 *BufferForMerge; >> UINTN MergedBufSize; >> BOOLEAN DataReady; >> @@ -2577,7 +2576,6 @@ UpdateVariable ( >> // >> // Step 1: >> // >> - CacheOffset = mVariableModuleGlobal->NonVolatileLastVariableOffset; >> Status = UpdateVariableStore ( >> &mVariableModuleGlobal->VariableGlobal, >> FALSE, >> @@ -2643,6 +2641,11 @@ UpdateVariable ( >> goto Done; >> } >> >> + // >> + // update the memory copy of Flash region. >> + // > > The first character of comment is not capitalized.
In this particular case, I agree that such a change can be added to the patch. While it does not pertain to the actual work done here, the patch itself is so small (which is a good thing!) that the comment capitalization would not cause confusion. Still I suggest mentioning it briefly in the commit message too. Thanks Laszlo > >> + CopyMem ((UINT8 *)mNvVariableCache + mVariableModuleGlobal- >>> NonVolatileLastVariableOffset, (UINT8 *)NextVariable, VarSize); >> + >> mVariableModuleGlobal->NonVolatileLastVariableOffset += HEADER_ALIGN >> (VarSize); >> >> if ((Attributes & EFI_VARIABLE_HARDWARE_ERROR_RECORD) != 0) { >> @@ -2653,10 +2656,6 @@ UpdateVariable ( >> mVariableModuleGlobal->CommonUserVariableTotalSize += >> HEADER_ALIGN (VarSize); >> } >> } >> - // >> - // update the memory copy of Flash region. >> - // >> - CopyMem ((UINT8 *)mNvVariableCache + CacheOffset, (UINT8 >> *)NextVariable, VarSize); >> } else { >> // >> // Create a volatile variable. >> -- >> 2.7.0.windows.1 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel