[PATCH 1/1] EmbeddedPkg: Fixed Asserts in SCT Runtime Services test.

[PATCH 1/1] EmbeddedPkg: Fixed Asserts in SCT Runtime Services test.

[Gaurav Jain]

ASSERT in SetTime_Conf and SetWakeupTime_Conf Consistency Test.
SCT Test expect return as Invalid Parameter.
So removed ASSERT().

Signed-off-by: Gaurav Jain <gaurav.jain@nxp.com>
---
 EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c b/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
index 08fb9b0100b6..9bfb7756f0cb 100644
--- a/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
+++ b/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
@@ -85,10 +85,6 @@ IsDayValid (
   IN  EFI_TIME  *Time
   )
 {
-  ASSERT (Time->Day >= 1);
-  ASSERT (Time->Day <= mDayOfMonth[Time->Month - 1]);
-  ASSERT (Time->Month != 2 || IsLeapYear (Time) || Time->Day <= 28);
-
   if (Time->Day < 1 ||
       Time->Day > mDayOfMonth[Time->Month - 1] ||
       (Time->Month == 2 && !IsLeapYear (Time) && Time->Day > 28)) {
@@ -105,14 +101,15 @@ IsTimeValid(
   )
 {
   // Check the input parameters are within the range specified by UEFI
-  if (Time->Year   < 1900               ||
-      Time->Year   > 9999               ||
+  if (Time->Year   < 1998               ||
+      Time->Year   > 2099               ||
       Time->Month  < 1                  ||
       Time->Month  > 12                 ||
       !IsDayValid (Time)                ||
       Time->Hour   > 23                 ||
       Time->Minute > 59                 ||
       Time->Second > 59                 ||
+      Time->Nanosecond > 999999999      ||
       !IsValidTimeZone (Time->TimeZone) ||
       !IsValidDaylight (Time->Daylight)) {
     return FALSE;
@@ -254,6 +251,9 @@ SetWakeupTime (
   OUT EFI_TIME    *Time
   )
 {
+  if (Time == NULL || !IsTimeValid (Time)) {
+    return EFI_INVALID_PARAMETER;
+  }
   return LibSetWakeupTime (Enabled, Time);
 }
 
-- 
2.17.1

Re: [PATCH 1/1] EmbeddedPkg: Fixed Asserts in SCT Runtime Services test.

[Ard Biesheuvel]

On Thu, 30 Jan 2020 at 06:08, Gaurav Jain <gaurav.jain@nxp.com> wrote:
>
> ASSERT in SetTime_Conf and SetWakeupTime_Conf Consistency Test.
> SCT Test expect return as Invalid Parameter.
> So removed ASSERT().
>

This is not all this patch does.

> Signed-off-by: Gaurav Jain <gaurav.jain@nxp.com>
> ---
>  EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c b/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
> index 08fb9b0100b6..9bfb7756f0cb 100644
> --- a/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
> +++ b/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
> @@ -85,10 +85,6 @@ IsDayValid (
>    IN  EFI_TIME  *Time
>    )
>  {
> -  ASSERT (Time->Day >= 1);
> -  ASSERT (Time->Day <= mDayOfMonth[Time->Month - 1]);
> -  ASSERT (Time->Month != 2 || IsLeapYear (Time) || Time->Day <= 28);
> -
>    if (Time->Day < 1 ||
>        Time->Day > mDayOfMonth[Time->Month - 1] ||
>        (Time->Month == 2 && !IsLeapYear (Time) && Time->Day > 28)) {
> @@ -105,14 +101,15 @@ IsTimeValid(
>    )
>  {
>    // Check the input parameters are within the range specified by UEFI
> -  if (Time->Year   < 1900               ||
> -      Time->Year   > 9999               ||
> +  if (Time->Year   < 1998               ||
> +      Time->Year   > 2099               ||

That original range is based on the UEFI spec. On what basis are you
making this change?

If your RTC hardware cannot represent the original values, this is not
the place to fix that.


>        Time->Month  < 1                  ||
>        Time->Month  > 12                 ||
>        !IsDayValid (Time)                ||
>        Time->Hour   > 23                 ||
>        Time->Minute > 59                 ||
>        Time->Second > 59                 ||
> +      Time->Nanosecond > 999999999      ||
>        !IsValidTimeZone (Time->TimeZone) ||
>        !IsValidDaylight (Time->Daylight)) {
>      return FALSE;
> @@ -254,6 +251,9 @@ SetWakeupTime (
>    OUT EFI_TIME    *Time
>    )
>  {
> +  if (Time == NULL || !IsTimeValid (Time)) {
> +    return EFI_INVALID_PARAMETER;
> +  }
>    return LibSetWakeupTime (Enabled, Time);
>  }
>
> --
> 2.17.1
>

Re: [EXT] Re: [PATCH 1/1] EmbeddedPkg: Fixed Asserts in SCT Runtime Services test.

[gaurav.jain]

> -----Original Message-----
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Sent: Thursday, January 30, 2020 2:52 PM
> To: Gaurav Jain <gaurav.jain@nxp.com>
> Cc: devel@edk2.groups.io; Leif Lindholm <leif@nuviainc.com>; Pankaj Bansal
> <pankaj.bansal@nxp.com>
> Subject: [EXT] Re: [PATCH 1/1] EmbeddedPkg: Fixed Asserts in SCT Runtime
> Services test.
> 
> Caution: EXT Email
> 
> On Thu, 30 Jan 2020 at 06:08, Gaurav Jain <gaurav.jain@nxp.com> wrote:
> >
> > ASSERT in SetTime_Conf and SetWakeupTime_Conf Consistency Test.
> > SCT Test expect return as Invalid Parameter.
> > So removed ASSERT().
> >
> 
> This is not all this patch does.
> 
> > Signed-off-by: Gaurav Jain <gaurav.jain@nxp.com>
> > ---
> >  EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
> > b/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
> > index 08fb9b0100b6..9bfb7756f0cb 100644
> > --- a/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
> > +++ b/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
> > @@ -85,10 +85,6 @@ IsDayValid (
> >    IN  EFI_TIME  *Time
> >    )
> >  {
> > -  ASSERT (Time->Day >= 1);
> > -  ASSERT (Time->Day <= mDayOfMonth[Time->Month - 1]);
> > -  ASSERT (Time->Month != 2 || IsLeapYear (Time) || Time->Day <= 28);
> > -
> >    if (Time->Day < 1 ||
> >        Time->Day > mDayOfMonth[Time->Month - 1] ||
> >        (Time->Month == 2 && !IsLeapYear (Time) && Time->Day > 28)) {
> > @@ -105,14 +101,15 @@ IsTimeValid(
> >    )
> >  {
> >    // Check the input parameters are within the range specified by UEFI
> > -  if (Time->Year   < 1900               ||
> > -      Time->Year   > 9999               ||
> > +  if (Time->Year   < 1998               ||
> > +      Time->Year   > 2099               ||
> 
> That original range is based on the UEFI spec. On what basis are you making this
> change?
> 
> If your RTC hardware cannot represent the original values, this is not the place
> to fix that.

As per the UEFI SCT Test, SetWakeupTime_Conf expect EFI_INVALID_PARAMETER for Time.Year is 1997 and 2100.
Below is the link to check the Test code
https://github.com/tianocore/edk2-test/blob/master/uefi-sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/TimeServices/BlackBoxTest/TimeServicesBBTestConformance.c (Line: 847)

Either UEFI spec need to be modified as per the test or SCT Test needs fix as per UEFI Specification.

> 
> 
> >        Time->Month  < 1                  ||
> >        Time->Month  > 12                 ||
> >        !IsDayValid (Time)                ||
> >        Time->Hour   > 23                 ||
> >        Time->Minute > 59                 ||
> >        Time->Second > 59                 ||
> > +      Time->Nanosecond > 999999999      ||
> >        !IsValidTimeZone (Time->TimeZone) ||
> >        !IsValidDaylight (Time->Daylight)) {
> >      return FALSE;
> > @@ -254,6 +251,9 @@ SetWakeupTime (
> >    OUT EFI_TIME    *Time
> >    )
> >  {
> > +  if (Time == NULL || !IsTimeValid (Time)) {
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> >    return LibSetWakeupTime (Enabled, Time);  }
> >
> > --
> > 2.17.1
> >

FW: [EXT] Re: [PATCH 1/1] EmbeddedPkg: Fixed Asserts in SCT Runtime Services test.

[Gaurav Jain]

Hi Ard

I am waiting for your response.

Regards
Gaurav

-----Original Message-----
From: Gaurav Jain 
Sent: Friday, January 31, 2020 1:58 PM
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: devel@edk2.groups.io; Leif Lindholm <leif@nuviainc.com>; Pankaj Bansal <pankaj.bansal@nxp.com>
Subject: RE: [EXT] Re: [PATCH 1/1] EmbeddedPkg: Fixed Asserts in SCT Runtime Services test.



> -----Original Message-----
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Sent: Thursday, January 30, 2020 2:52 PM
> To: Gaurav Jain <gaurav.jain@nxp.com>
> Cc: devel@edk2.groups.io; Leif Lindholm <leif@nuviainc.com>; Pankaj 
> Bansal <pankaj.bansal@nxp.com>
> Subject: [EXT] Re: [PATCH 1/1] EmbeddedPkg: Fixed Asserts in SCT 
> Runtime Services test.
> 
> Caution: EXT Email
> 
> On Thu, 30 Jan 2020 at 06:08, Gaurav Jain <gaurav.jain@nxp.com> wrote:
> >
> > ASSERT in SetTime_Conf and SetWakeupTime_Conf Consistency Test.
> > SCT Test expect return as Invalid Parameter.
> > So removed ASSERT().
> >
> 
> This is not all this patch does.
> 
> > Signed-off-by: Gaurav Jain <gaurav.jain@nxp.com>
> > ---
> >  EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c | 12 
> > ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
> > b/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
> > index 08fb9b0100b6..9bfb7756f0cb 100644
> > --- a/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
> > +++ b/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
> > @@ -85,10 +85,6 @@ IsDayValid (
> >    IN  EFI_TIME  *Time
> >    )
> >  {
> > -  ASSERT (Time->Day >= 1);
> > -  ASSERT (Time->Day <= mDayOfMonth[Time->Month - 1]);
> > -  ASSERT (Time->Month != 2 || IsLeapYear (Time) || Time->Day <= 
> > 28);
> > -
> >    if (Time->Day < 1 ||
> >        Time->Day > mDayOfMonth[Time->Month - 1] ||
> >        (Time->Month == 2 && !IsLeapYear (Time) && Time->Day > 28)) { 
> > @@ -105,14 +101,15 @@ IsTimeValid(
> >    )
> >  {
> >    // Check the input parameters are within the range specified by UEFI
> > -  if (Time->Year   < 1900               ||
> > -      Time->Year   > 9999               ||
> > +  if (Time->Year   < 1998               ||
> > +      Time->Year   > 2099               ||
> 
> That original range is based on the UEFI spec. On what basis are you 
> making this change?
> 
> If your RTC hardware cannot represent the original values, this is not 
> the place to fix that.

As per the UEFI SCT Test, SetWakeupTime_Conf expect EFI_INVALID_PARAMETER for Time.Year is 1997 and 2100.
Below is the link to check the Test code https://github.com/tianocore/edk2-test/blob/master/uefi-sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/TimeServices/BlackBoxTest/TimeServicesBBTestConformance.c (Line: 847)

Either UEFI spec need to be modified as per the test or SCT Test needs fix as per UEFI Specification.

> 
> 
> >        Time->Month  < 1                  ||
> >        Time->Month  > 12                 ||
> >        !IsDayValid (Time)                ||
> >        Time->Hour   > 23                 ||
> >        Time->Minute > 59                 ||
> >        Time->Second > 59                 ||
> > +      Time->Nanosecond > 999999999      ||
> >        !IsValidTimeZone (Time->TimeZone) ||
> >        !IsValidDaylight (Time->Daylight)) {
> >      return FALSE;
> > @@ -254,6 +251,9 @@ SetWakeupTime (
> >    OUT EFI_TIME    *Time
> >    )
> >  {
> > +  if (Time == NULL || !IsTimeValid (Time)) {
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> >    return LibSetWakeupTime (Enabled, Time);  }
> >
> > --
> > 2.17.1
> >

Re: FW: [EXT] Re: [PATCH 1/1] EmbeddedPkg: Fixed Asserts in SCT Runtime Services test.

[Ard Biesheuvel]

On Tue, 11 Feb 2020 at 11:37, Gaurav Jain <gaurav.jain@nxp.com> wrote:
>
> Hi Ard
>
> I am waiting for your response.
>

You said

> Either UEFI spec need to be modified as per the test or SCT Test needs fix as per UEFI Specification.
>

so you answered your own question, no?

> -----Original Message-----
> From: Gaurav Jain
> Sent: Friday, January 31, 2020 1:58 PM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: devel@edk2.groups.io; Leif Lindholm <leif@nuviainc.com>; Pankaj Bansal <pankaj.bansal@nxp.com>
> Subject: RE: [EXT] Re: [PATCH 1/1] EmbeddedPkg: Fixed Asserts in SCT Runtime Services test.
>
>
>
> > -----Original Message-----
> > From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Sent: Thursday, January 30, 2020 2:52 PM
> > To: Gaurav Jain <gaurav.jain@nxp.com>
> > Cc: devel@edk2.groups.io; Leif Lindholm <leif@nuviainc.com>; Pankaj
> > Bansal <pankaj.bansal@nxp.com>
> > Subject: [EXT] Re: [PATCH 1/1] EmbeddedPkg: Fixed Asserts in SCT
> > Runtime Services test.
> >
> > Caution: EXT Email
> >
> > On Thu, 30 Jan 2020 at 06:08, Gaurav Jain <gaurav.jain@nxp.com> wrote:
> > >
> > > ASSERT in SetTime_Conf and SetWakeupTime_Conf Consistency Test.
> > > SCT Test expect return as Invalid Parameter.
> > > So removed ASSERT().
> > >
> >
> > This is not all this patch does.
> >
> > > Signed-off-by: Gaurav Jain <gaurav.jain@nxp.com>
> > > ---
> > >  EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c | 12
> > > ++++++------
> > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
> > > b/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
> > > index 08fb9b0100b6..9bfb7756f0cb 100644
> > > --- a/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
> > > +++ b/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
> > > @@ -85,10 +85,6 @@ IsDayValid (
> > >    IN  EFI_TIME  *Time
> > >    )
> > >  {
> > > -  ASSERT (Time->Day >= 1);
> > > -  ASSERT (Time->Day <= mDayOfMonth[Time->Month - 1]);
> > > -  ASSERT (Time->Month != 2 || IsLeapYear (Time) || Time->Day <=
> > > 28);
> > > -
> > >    if (Time->Day < 1 ||
> > >        Time->Day > mDayOfMonth[Time->Month - 1] ||
> > >        (Time->Month == 2 && !IsLeapYear (Time) && Time->Day > 28)) {
> > > @@ -105,14 +101,15 @@ IsTimeValid(
> > >    )
> > >  {
> > >    // Check the input parameters are within the range specified by UEFI
> > > -  if (Time->Year   < 1900               ||
> > > -      Time->Year   > 9999               ||
> > > +  if (Time->Year   < 1998               ||
> > > +      Time->Year   > 2099               ||
> >
> > That original range is based on the UEFI spec. On what basis are you
> > making this change?
> >
> > If your RTC hardware cannot represent the original values, this is not
> > the place to fix that.
>
> As per the UEFI SCT Test, SetWakeupTime_Conf expect EFI_INVALID_PARAMETER for Time.Year is 1997 and 2100.
> Below is the link to check the Test code https://github.com/tianocore/edk2-test/blob/master/uefi-sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/TimeServices/BlackBoxTest/TimeServicesBBTestConformance.c (Line: 847)
>
> Either UEFI spec need to be modified as per the test or SCT Test needs fix as per UEFI Specification.
>
> >
> >
> > >        Time->Month  < 1                  ||
> > >        Time->Month  > 12                 ||
> > >        !IsDayValid (Time)                ||
> > >        Time->Hour   > 23                 ||
> > >        Time->Minute > 59                 ||
> > >        Time->Second > 59                 ||
> > > +      Time->Nanosecond > 999999999      ||
> > >        !IsValidTimeZone (Time->TimeZone) ||
> > >        !IsValidDaylight (Time->Daylight)) {
> > >      return FALSE;
> > > @@ -254,6 +251,9 @@ SetWakeupTime (
> > >    OUT EFI_TIME    *Time
> > >    )
> > >  {
> > > +  if (Time == NULL || !IsTimeValid (Time)) {
> > > +    return EFI_INVALID_PARAMETER;
> > > +  }
> > >    return LibSetWakeupTime (Enabled, Time);  }
> > >
> > > --
> > > 2.17.1
> > >

Re: FW: [EXT] Re: [PATCH 1/1] EmbeddedPkg: Fixed Asserts in SCT Runtime Services test.

[Gaurav Jain]

Hi Ard

I have also checked UEFI Shell Specification Version 2.2(https://uefi.org/sites/default/files/resources/UEFI_Shell_2_2.pdf)
For date command the range of valid years is from 1998–2099 (on page 121)

So Range of valid years is conflicting in UEFI Spec and UEFI Shell Spec.
I think UEFI spec needs to be corrected. What are your thoughts?


> -----Original Message-----
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Sent: Tuesday, February 11, 2020 7:50 PM
> To: Gaurav Jain <gaurav.jain@nxp.com>
> Cc: devel@edk2.groups.io; Leif Lindholm <leif@nuviainc.com>; Pankaj Bansal
> <pankaj.bansal@nxp.com>
> Subject: Re: FW: [EXT] Re: [PATCH 1/1] EmbeddedPkg: Fixed Asserts in SCT
> Runtime Services test.
> 
> Caution: EXT Email
> 
> On Tue, 11 Feb 2020 at 11:37, Gaurav Jain <gaurav.jain@nxp.com> wrote:
> >
> > Hi Ard
> >
> > I am waiting for your response.
> >
> 
> You said
> 
> > Either UEFI spec need to be modified as per the test or SCT Test needs fix as
> per UEFI Specification.
> >
> 
> so you answered your own question, no?
> 
> > -----Original Message-----
> > From: Gaurav Jain
> > Sent: Friday, January 31, 2020 1:58 PM
> > To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: devel@edk2.groups.io; Leif Lindholm <leif@nuviainc.com>; Pankaj
> > Bansal <pankaj.bansal@nxp.com>
> > Subject: RE: [EXT] Re: [PATCH 1/1] EmbeddedPkg: Fixed Asserts in SCT
> Runtime Services test.
> >
> >
> >
> > > -----Original Message-----
> > > From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > Sent: Thursday, January 30, 2020 2:52 PM
> > > To: Gaurav Jain <gaurav.jain@nxp.com>
> > > Cc: devel@edk2.groups.io; Leif Lindholm <leif@nuviainc.com>; Pankaj
> > > Bansal <pankaj.bansal@nxp.com>
> > > Subject: [EXT] Re: [PATCH 1/1] EmbeddedPkg: Fixed Asserts in SCT
> > > Runtime Services test.
> > >
> > > Caution: EXT Email
> > >
> > > On Thu, 30 Jan 2020 at 06:08, Gaurav Jain <gaurav.jain@nxp.com> wrote:
> > > >
> > > > ASSERT in SetTime_Conf and SetWakeupTime_Conf Consistency Test.
> > > > SCT Test expect return as Invalid Parameter.
> > > > So removed ASSERT().
> > > >
> > >
> > > This is not all this patch does.
> > >
> > > > Signed-off-by: Gaurav Jain <gaurav.jain@nxp.com>
> > > > ---
> > > >  EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c | 12
> > > > ++++++------
> > > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
> > > > b/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
> > > > index 08fb9b0100b6..9bfb7756f0cb 100644
> > > > --- a/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
> > > > +++ b/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
> > > > @@ -85,10 +85,6 @@ IsDayValid (
> > > >    IN  EFI_TIME  *Time
> > > >    )
> > > >  {
> > > > -  ASSERT (Time->Day >= 1);
> > > > -  ASSERT (Time->Day <= mDayOfMonth[Time->Month - 1]);
> > > > -  ASSERT (Time->Month != 2 || IsLeapYear (Time) || Time->Day <=
> > > > 28);
> > > > -
> > > >    if (Time->Day < 1 ||
> > > >        Time->Day > mDayOfMonth[Time->Month - 1] ||
> > > >        (Time->Month == 2 && !IsLeapYear (Time) && Time->Day > 28))
> > > > { @@ -105,14 +101,15 @@ IsTimeValid(
> > > >    )
> > > >  {
> > > >    // Check the input parameters are within the range specified by UEFI
> > > > -  if (Time->Year   < 1900               ||
> > > > -      Time->Year   > 9999               ||
> > > > +  if (Time->Year   < 1998               ||
> > > > +      Time->Year   > 2099               ||
> > >
> > > That original range is based on the UEFI spec. On what basis are you
> > > making this change?
> > >
> > > If your RTC hardware cannot represent the original values, this is
> > > not the place to fix that.
> >
> > As per the UEFI SCT Test, SetWakeupTime_Conf expect
> EFI_INVALID_PARAMETER for Time.Year is 1997 and 2100.
> > Below is the link to check the Test code
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
> > ub.com%2Ftianocore%2Fedk2-test%2Fblob%2Fmaster%2Fuefi-
> sct%2FSctPkg%2FT
> >
> estCase%2FUEFI%2FEFI%2FRuntimeServices%2FTimeServices%2FBlackBoxTes
> t%2
> >
> FTimeServicesBBTestConformance.c&amp;data=02%7C01%7Cgaurav.jain%40
> nxp.
> >
> com%7C3c6e62107ab149b8709808d7aefd73e8%7C686ea1d3bc2b4c6fa92cd9
> 9c5c301
> >
> 635%7C0%7C0%7C637170275901140244&amp;sdata=83GGnHy%2BZvzz5yZo
> 8et56FQqH
> > meYimYGB9dJxtlluKM%3D&amp;reserved=0 (Line: 847)
> >
> > Either UEFI spec need to be modified as per the test or SCT Test needs fix as
> per UEFI Specification.
> >
> > >
> > >
> > > >        Time->Month  < 1                  ||
> > > >        Time->Month  > 12                 ||
> > > >        !IsDayValid (Time)                ||
> > > >        Time->Hour   > 23                 ||
> > > >        Time->Minute > 59                 ||
> > > >        Time->Second > 59                 ||
> > > > +      Time->Nanosecond > 999999999      ||
> > > >        !IsValidTimeZone (Time->TimeZone) ||
> > > >        !IsValidDaylight (Time->Daylight)) {
> > > >      return FALSE;
> > > > @@ -254,6 +251,9 @@ SetWakeupTime (
> > > >    OUT EFI_TIME    *Time
> > > >    )
> > > >  {
> > > > +  if (Time == NULL || !IsTimeValid (Time)) {
> > > > +    return EFI_INVALID_PARAMETER;  }
> > > >    return LibSetWakeupTime (Enabled, Time);  }
> > > >
> > > > --
> > > > 2.17.1
> > > >

Re: FW: [EXT] Re: [PATCH 1/1] EmbeddedPkg: Fixed Asserts in SCT Runtime Services test.

[Ard Biesheuvel]

On Mon, 17 Feb 2020 at 07:25, Gaurav Jain <gaurav.jain@nxp.com> wrote:
>
> Hi Ard
>
> I have also checked UEFI Shell Specification Version 2.2(https://uefi.org/sites/default/files/resources/UEFI_Shell_2_2.pdf)
> For date command the range of valid years is from 1998–2099 (on page 121)
>
> So Range of valid years is conflicting in UEFI Spec and UEFI Shell Spec.
> I think UEFI spec needs to be corrected. What are your thoughts?
>

Page 121 does not mention data. Page 111 does, and tells me that a
two-digit year is interpreted as 199x or 20xx, and a four digit year
is interpreted as is.

How exactly does that conflict with the UEFI spec?


>
> > -----Original Message-----
> > From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Sent: Tuesday, February 11, 2020 7:50 PM
> > To: Gaurav Jain <gaurav.jain@nxp.com>
> > Cc: devel@edk2.groups.io; Leif Lindholm <leif@nuviainc.com>; Pankaj Bansal
> > <pankaj.bansal@nxp.com>
> > Subject: Re: FW: [EXT] Re: [PATCH 1/1] EmbeddedPkg: Fixed Asserts in SCT
> > Runtime Services test.
> >
> > Caution: EXT Email
> >
> > On Tue, 11 Feb 2020 at 11:37, Gaurav Jain <gaurav.jain@nxp.com> wrote:
> > >
> > > Hi Ard
> > >
> > > I am waiting for your response.
> > >
> >
> > You said
> >
> > > Either UEFI spec need to be modified as per the test or SCT Test needs fix as
> > per UEFI Specification.
> > >
> >
> > so you answered your own question, no?
> >
> > > -----Original Message-----
> > > From: Gaurav Jain
> > > Sent: Friday, January 31, 2020 1:58 PM
> > > To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > Cc: devel@edk2.groups.io; Leif Lindholm <leif@nuviainc.com>; Pankaj
> > > Bansal <pankaj.bansal@nxp.com>
> > > Subject: RE: [EXT] Re: [PATCH 1/1] EmbeddedPkg: Fixed Asserts in SCT
> > Runtime Services test.
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > > Sent: Thursday, January 30, 2020 2:52 PM
> > > > To: Gaurav Jain <gaurav.jain@nxp.com>
> > > > Cc: devel@edk2.groups.io; Leif Lindholm <leif@nuviainc.com>; Pankaj
> > > > Bansal <pankaj.bansal@nxp.com>
> > > > Subject: [EXT] Re: [PATCH 1/1] EmbeddedPkg: Fixed Asserts in SCT
> > > > Runtime Services test.
> > > >
> > > > Caution: EXT Email
> > > >
> > > > On Thu, 30 Jan 2020 at 06:08, Gaurav Jain <gaurav.jain@nxp.com> wrote:
> > > > >
> > > > > ASSERT in SetTime_Conf and SetWakeupTime_Conf Consistency Test.
> > > > > SCT Test expect return as Invalid Parameter.
> > > > > So removed ASSERT().
> > > > >
> > > >
> > > > This is not all this patch does.
> > > >
> > > > > Signed-off-by: Gaurav Jain <gaurav.jain@nxp.com>
> > > > > ---
> > > > >  EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c | 12
> > > > > ++++++------
> > > > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
> > > > > b/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
> > > > > index 08fb9b0100b6..9bfb7756f0cb 100644
> > > > > --- a/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
> > > > > +++ b/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
> > > > > @@ -85,10 +85,6 @@ IsDayValid (
> > > > >    IN  EFI_TIME  *Time
> > > > >    )
> > > > >  {
> > > > > -  ASSERT (Time->Day >= 1);
> > > > > -  ASSERT (Time->Day <= mDayOfMonth[Time->Month - 1]);
> > > > > -  ASSERT (Time->Month != 2 || IsLeapYear (Time) || Time->Day <=
> > > > > 28);
> > > > > -
> > > > >    if (Time->Day < 1 ||
> > > > >        Time->Day > mDayOfMonth[Time->Month - 1] ||
> > > > >        (Time->Month == 2 && !IsLeapYear (Time) && Time->Day > 28))
> > > > > { @@ -105,14 +101,15 @@ IsTimeValid(
> > > > >    )
> > > > >  {
> > > > >    // Check the input parameters are within the range specified by UEFI
> > > > > -  if (Time->Year   < 1900               ||
> > > > > -      Time->Year   > 9999               ||
> > > > > +  if (Time->Year   < 1998               ||
> > > > > +      Time->Year   > 2099               ||
> > > >
> > > > That original range is based on the UEFI spec. On what basis are you
> > > > making this change?
> > > >
> > > > If your RTC hardware cannot represent the original values, this is
> > > > not the place to fix that.
> > >
> > > As per the UEFI SCT Test, SetWakeupTime_Conf expect
> > EFI_INVALID_PARAMETER for Time.Year is 1997 and 2100.
> > > Below is the link to check the Test code
> > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
> > > ub.com%2Ftianocore%2Fedk2-test%2Fblob%2Fmaster%2Fuefi-
> > sct%2FSctPkg%2FT
> > >
> > estCase%2FUEFI%2FEFI%2FRuntimeServices%2FTimeServices%2FBlackBoxTes
> > t%2
> > >
> > FTimeServicesBBTestConformance.c&amp;data=02%7C01%7Cgaurav.jain%40
> > nxp.
> > >
> > com%7C3c6e62107ab149b8709808d7aefd73e8%7C686ea1d3bc2b4c6fa92cd9
> > 9c5c301
> > >
> > 635%7C0%7C0%7C637170275901140244&amp;sdata=83GGnHy%2BZvzz5yZo
> > 8et56FQqH
> > > meYimYGB9dJxtlluKM%3D&amp;reserved=0 (Line: 847)
> > >
> > > Either UEFI spec need to be modified as per the test or SCT Test needs fix as
> > per UEFI Specification.
> > >
> > > >
> > > >
> > > > >        Time->Month  < 1                  ||
> > > > >        Time->Month  > 12                 ||
> > > > >        !IsDayValid (Time)                ||
> > > > >        Time->Hour   > 23                 ||
> > > > >        Time->Minute > 59                 ||
> > > > >        Time->Second > 59                 ||
> > > > > +      Time->Nanosecond > 999999999      ||
> > > > >        !IsValidTimeZone (Time->TimeZone) ||
> > > > >        !IsValidDaylight (Time->Daylight)) {
> > > > >      return FALSE;
> > > > > @@ -254,6 +251,9 @@ SetWakeupTime (
> > > > >    OUT EFI_TIME    *Time
> > > > >    )
> > > > >  {
> > > > > +  if (Time == NULL || !IsTimeValid (Time)) {
> > > > > +    return EFI_INVALID_PARAMETER;  }
> > > > >    return LibSetWakeupTime (Enabled, Time);  }
> > > > >
> > > > > --
> > > > > 2.17.1
> > > > >

Re: FW: [EXT] Re: [PATCH 1/1] EmbeddedPkg: Fixed Asserts in SCT Runtime Services test.

[Ard Biesheuvel]

On Mon, 17 Feb 2020 at 07:56, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Mon, 17 Feb 2020 at 07:25, Gaurav Jain <gaurav.jain@nxp.com> wrote:
> >
> > Hi Ard
> >
> > I have also checked UEFI Shell Specification Version 2.2(https://uefi.org/sites/default/files/resources/UEFI_Shell_2_2.pdf)
> > For date command the range of valid years is from 1998–2099 (on page 121)
> >
> > So Range of valid years is conflicting in UEFI Spec and UEFI Shell Spec.
> > I think UEFI spec needs to be corrected. What are your thoughts?
> >
>
> Page 121 does not mention data. Page 111 does, and tells me that a
> two-digit year is interpreted as 199x or 20xx, and a four digit year
> is interpreted as is.
>
> How exactly does that conflict with the UEFI spec?
>

OK, so I found this on page 111:

"""The range of valid years is from 1998–2099"""

and so indeed, the Shell 'date' command's year range is narrower than
the UEFI spec's, but that does not make it non-compliant.

Re: [PATCH 1/1] EmbeddedPkg: Fixed Asserts in SCT Runtime Services test.

[Ard Biesheuvel]

On Mon, 17 Feb 2020 at 08:31, Gaurav Jain <gaurav.jain@nxp.com> wrote:
>
> ASSERT in SetTime_Conf and SetWakeupTime_Conf Consistency Test.
> SCT Test expect return as Invalid Parameter.
> So removed ASSERT().
>
> Added Time Validity Checks in SetWakeupTime.
>
> Signed-off-by: Gaurav Jain <gaurav.jain@nxp.com>

If this is a followup that supersedes your previous submission, it
would be helpful to label this one as v2, and mention (below the ---)
what the changes are wrt v1.


> ---
>  EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c b/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
> index 08fb9b0100b6..70a0d78125b9 100644
> --- a/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
> +++ b/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
> @@ -85,10 +85,6 @@ IsDayValid (
>    IN  EFI_TIME  *Time
>    )
>  {
> -  ASSERT (Time->Day >= 1);
> -  ASSERT (Time->Day <= mDayOfMonth[Time->Month - 1]);
> -  ASSERT (Time->Month != 2 || IsLeapYear (Time) || Time->Day <= 28);
> -
>    if (Time->Day < 1 ||
>        Time->Day > mDayOfMonth[Time->Month - 1] ||
>        (Time->Month == 2 && !IsLeapYear (Time) && Time->Day > 28)) {
> @@ -113,6 +109,7 @@ IsTimeValid(
>        Time->Hour   > 23                 ||
>        Time->Minute > 59                 ||
>        Time->Second > 59                 ||
> +      Time->Nanosecond > 999999999      ||
>        !IsValidTimeZone (Time->TimeZone) ||
>        !IsValidDaylight (Time->Daylight)) {
>      return FALSE;
> @@ -254,6 +251,9 @@ SetWakeupTime (
>    OUT EFI_TIME    *Time
>    )
>  {
> +  if (Time == NULL || !IsTimeValid (Time)) {
> +    return EFI_INVALID_PARAMETER;
> +  }
>    return LibSetWakeupTime (Enabled, Time);
>  }
>
> --
> 2.17.1
>

[PATCH 1/1] EmbeddedPkg: Fixed Asserts in SCT Runtime Services test.

[Gaurav Jain]

ASSERT in SetTime_Conf and SetWakeupTime_Conf Consistency Test.
SCT Test expect return as Invalid Parameter.
So removed ASSERT().

Added Time Validity Checks in SetWakeupTime.

Signed-off-by: Gaurav Jain <gaurav.jain@nxp.com>
---
 EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c b/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
index 08fb9b0100b6..70a0d78125b9 100644
--- a/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
+++ b/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
@@ -85,10 +85,6 @@ IsDayValid (
   IN  EFI_TIME  *Time
   )
 {
-  ASSERT (Time->Day >= 1);
-  ASSERT (Time->Day <= mDayOfMonth[Time->Month - 1]);
-  ASSERT (Time->Month != 2 || IsLeapYear (Time) || Time->Day <= 28);
-
   if (Time->Day < 1 ||
       Time->Day > mDayOfMonth[Time->Month - 1] ||
       (Time->Month == 2 && !IsLeapYear (Time) && Time->Day > 28)) {
@@ -113,6 +109,7 @@ IsTimeValid(
       Time->Hour   > 23                 ||
       Time->Minute > 59                 ||
       Time->Second > 59                 ||
+      Time->Nanosecond > 999999999      ||
       !IsValidTimeZone (Time->TimeZone) ||
       !IsValidDaylight (Time->Daylight)) {
     return FALSE;
@@ -254,6 +251,9 @@ SetWakeupTime (
   OUT EFI_TIME    *Time
   )
 {
+  if (Time == NULL || !IsTimeValid (Time)) {
+    return EFI_INVALID_PARAMETER;
+  }
   return LibSetWakeupTime (Enabled, Time);
 }
 
-- 
2.17.1