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

[Laszlo Ersek <lersek@redhat.com>]

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.

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
>