Re: [PATCH V2 06/15] MdeModulePkg Variable: Remove CacheOffset in UpdateVariable()

["Zeng, Star" <star.zeng@intel.com>]

On 2019/1/15 17:58, Laszlo Ersek wrote:
> 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.wang@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.zeng@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
>>> Wu, Hao A <hao.a.wu@intel.com>; Laszlo Ersek <lersek@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.wang@intel.com>
>>> Cc: Hao Wu <hao.a.wu@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Star Zeng <star.zeng@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.

Make sense, I can do that.

Thanks,
Star

> 
> 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
>>