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 <[email protected]>
>
>> -----Original Message-----
>> From: Zeng, Star
>> Sent: Monday, January 14, 2019 11:20 PM
>> To: [email protected]
>> Cc: Zeng, Star <[email protected]>; Wang, Jian J <[email protected]>;
>> Wu, Hao A <[email protected]>; Laszlo Ersek <[email protected]>
>> 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 <[email protected]>
>> Cc: Hao Wu <[email protected]>
>> Cc: Laszlo Ersek <[email protected]>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Star Zeng <[email protected]>
>> ---
>> 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
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
