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

["Vladimir Olovyannikov" <vladimir.olovyannikov@broadcom.com>]

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Wednesday, September 9, 2020 3:51 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/08/20 23:04, Vladimir Olovyannikov wrote:
> >> -----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?
>
> You'd have to audit, and if necessary, clean up, the EfiTimeToEpoch() call
> sites in edk2-platforms. And you'd need to establish a global order
> between
> the patch series (plural) such that both edk2 and edk2-platforms should
> build
> at any stage across those series.
I looked through the platforms, and all of them use UINT32 for
EfiTimeToEpoch() return value, so IMHO it is sufficient to
change EfiTimeToEpoch's variables to UINTN, and then return
(UINT32)(EpochSeconds) in EfiTimeToEpoch().
>
> Thanks
> Laszlo
Thank you,
Vladimir
>
> > I saw today that unused macros like SEC_PER_MONTH, etc. were removed
> > from TimeBaseLib.h.
> >
> > Thank you,
> > Vladimir
> >>
> >> Laszlo
> >