Re: [PATCH v10 1/1] ShellPkg/DynamicCommand: add HttpDynamicCommand


Vladimir Olovyannikov
 

-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com>
Sent: Monday, September 7, 2020 2:37 AM
To: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>;
Rabeda, Maciej <maciej.rabeda@linux.intel.com>; devel@edk2.groups.io;
Zhichao Gao <zhichao.gao@intel.com>; Ray Ni <ray.ni@intel.com>
Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>; Jiaxin Wu
<jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>; Liming Gao
<liming.gao@intel.com>; Nd <nd@arm.com>
Subject: Re: [PATCH v10 1/1] ShellPkg/DynamicCommand: add
HttpDynamicCommand

On 09/04/20 19:55, Vladimir Olovyannikov wrote:

There is also another issue with the TimebaseLib: inconsistency in
return values of the EfiTimeToEpoch (returns UINT32, should return
UINTN, as Zhichao pointed out earlier in the previous
HttpDynamicCommand patchset).
If this one is fixed, I can just use the TimeBaseLib.h header for
constants.
Consuming TimeBaseLib.h in this patch would be really nice.
OK, if this can be fixed, I will definitely use TimeBaseLib.h header for
constants, and will drop
duplicate definitions in Http.c/Http.h

There are two EfiTimeToEpoch() call sites in edk2:

ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c
EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.c

The latter stores the return value in a UINTN variable, so that seems OK.
The
former is a bit messier, but it seems to ensure that the result fits in 32
bits for
HW reasons anyway:

// Because the PL031 is a 32-bit counter counting seconds,
// the maximum time span is just over 136 years.
// Time is stored in Unix Epoch format, so it starts in 1970,
// Therefore it can not exceed the year 2106.
if ((Time->Year < 1970) || (Time->Year >= 2106)) {
return EFI_UNSUPPORTED;
}
...
EpochSeconds = EfiTimeToEpoch (Time);
...
MmioWrite32 (mPL031RtcBase + PL031_RTC_LR_LOAD_REGISTER,
EpochSeconds);

So I think we'd need two patches:

(1) add an explicit (UINT32) cast to the EfiTimeToEpoch() call in
"ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c",

(2) change the return value to UINTN in
"EmbeddedPkg/Include/Library/TimeBaseLib.h" and
"EmbeddedPkg/Library/TimeBaseLib/TimeBaseLib.c".

Hmm wait... There are five more call sites in edk2-platforms. :( OK, I
give up
here. Sorry.
OK, so what are the next steps, what do you suggest?
I saw today that unused macros like SEC_PER_MONTH, etc. were removed from
TimeBaseLib.h.

Thank you,
Vladimir

Laszlo

Join devel@edk2.groups.io to automatically receive all group messages.