Re: [edk2-devel] [edk2-test][Patch] uefi-sct/SctPkg: Add MediaPresentSupported check

["xianhui liu" <xianhuix.liu@intel.com>]

Please refer to below comments.

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Heinrich Schuchardt
> Sent: Wednesday, September 25, 2019 3:06 AM
> To: Liu, XianhuiX <xianhuix.liu@intel.com>; devel@edk2.groups.io
> Cc: Supreeth Venkatesh <supreeth.venkatesh@arm.com>; Jin, Eric
> <eric.jin@intel.com>
> Subject: Re: [edk2-devel] [edk2-test][Patch] uefi-sct/SctPkg: Add
> MediaPresentSupported check
> 
> On 9/24/19 10:50 AM, xianhui liu wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2061
> > check MediaPresent while MediaPresentSupported is TRUE sync change
> > from EFI to IHV SimpleNetworkBBTestFunction
> 
> Thanks for addressing this issue.
> 
> >
> > Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> 
> %s/Cc:/Reported-by:/
> 
> > Cc: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
> > Cc: Eric Jin <eric.jin@intel.com>
> >
> > Signed-off-by: xianhui liu <xianhuix.liu@intel.com>
> > ---
> >   .../BlackBoxTest/SimpleNetworkBBTestFunction.c     | 64 +++++++++++---
> --------
> >   1 file changed, 33 insertions(+), 31 deletions(-)
> >
> > diff --git
> > a/uefi-
> sct/SctPkg/TestCase/UEFI/IHV/Protocol/SimpleNetwork/BlackBoxTes
> > t/SimpleNetworkBBTestFunction.c
> > b/uefi-
> sct/SctPkg/TestCase/UEFI/IHV/Protocol/SimpleNetwork/BlackBoxTes
> > t/SimpleNetworkBBTestFunction.c
> > index b4c7b5ee..8559e894 100644
> > ---
> > a/uefi-
> sct/SctPkg/TestCase/UEFI/IHV/Protocol/SimpleNetwork/BlackBoxTes
> > t/SimpleNetworkBBTestFunction.c
> > +++ b/uefi-
> sct/SctPkg/TestCase/UEFI/IHV/Protocol/SimpleNetwork/BlackBo
> > +++ xTest/SimpleNetworkBBTestFunction.c
> > @@ -1888,38 +1888,40 @@ BBTestGetStatusFunctionTest (
> >     Status = SnpInterface->GetStatus (SnpInterface, &InterruptStatus,
> &TxBuf);
> >     Status1 = SnpInterface->GetStatus (SnpInterface, &InterruptStatus,
> > &TxBuf);
> >
> > -  if (SnpInterface->Mode->MediaPresent == FALSE) {
> > -    if ((Status1 == EFI_SUCCESS) && (Status == EFI_SUCCESS) &&
> (InterruptStatus == 0)) {
> > -      AssertionType = EFI_TEST_ASSERTION_PASSED;
> > -    } else {
> > -      AssertionType = EFI_TEST_ASSERTION_FAILED;
> > -    }
> > -  } else {
> > -    if ((Status1 == EFI_SUCCESS) && (Status == EFI_SUCCESS)) {
> > -      AssertionType = EFI_TEST_ASSERTION_PASSED;
> > -      if (InterruptStatus &
> > -         ~( EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT |
> > -            EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT |
> > -            EFI_SIMPLE_NETWORK_COMMAND_INTERRUPT |
> > -            EFI_SIMPLE_NETWORK_SOFTWARE_INTERRUPT)) {
> > -        AssertionType = EFI_TEST_ASSERTION_FAILED;
> > -      }
> > -    } else {
> > -      AssertionType = EFI_TEST_ASSERTION_FAILED;
> > -    }
> > +  if (SnpInterface->Mode-> MediaPresentSupported == TRUE) {
> > +     if (SnpInterface->Mode->MediaPresent == FALSE) {
> 
> 
> If MediaPresentSupported == FALSE shouldn't we assume that Media is
> present.
> 
> So isn't a single 'if' enough:
> 
> if (SnpInterface->Mode-> MediaPresentSupported == TRUE &&
>      SnpInterface->Mode->MediaPresent == FALSE) {
> 
> Best regards
> 
> Heinrich Schuchardt

Hi Heinrich,
We will skip the checkpoint if MediaPresentSupported == FALSE. Thanks.

Hi Eric,
Please help correct me if any mistake here. Thanks.

Best Regards
Xianhui Liu


> 
> > +       if ((Status1 == EFI_SUCCESS) && (Status == EFI_SUCCESS) &&
> (InterruptStatus == 0)) {
> > +         AssertionType = EFI_TEST_ASSERTION_PASSED;
> > +       } else {
> > +         AssertionType = EFI_TEST_ASSERTION_FAILED;
> > +       }
> > +     } else {
> > +       if ((Status1 == EFI_SUCCESS) && (Status == EFI_SUCCESS)) {
> > +         AssertionType = EFI_TEST_ASSERTION_PASSED;
> > +         if (InterruptStatus &
> > +            ~( EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT |
> > +               EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT |
> > +               EFI_SIMPLE_NETWORK_COMMAND_INTERRUPT |
> > +               EFI_SIMPLE_NETWORK_SOFTWARE_INTERRUPT)) {
> > +           AssertionType = EFI_TEST_ASSERTION_FAILED;
> > +         }
> > +       } else {
> > +         AssertionType = EFI_TEST_ASSERTION_FAILED;
> > +       }
> > +     }
> > +     StandardLib->RecordAssertion (
> > +                    StandardLib,
> > +                    AssertionType,
> > +                    gSimpleNetworkBBTestFunctionAssertionGuid022,
> > +                    L"EFI_SIMPLE_NETWORK_PROTOCOL.GetStatus - Invoke
> GetStatus() and verify interface correctness within test case",
> > +                    L"%a:%d:Status - %r, Status1 - %r, InterruptStatus - %d",
> > +                    __FILE__,
> > +                    (UINTN)__LINE__,
> > +                    Status,
> > +                    Status1,
> > +                    InterruptStatus
> > +                    );
> >     }
> > -  StandardLib->RecordAssertion (
> > -                 StandardLib,
> > -                 AssertionType,
> > -                 gSimpleNetworkBBTestFunctionAssertionGuid022,
> > -                 L"EFI_SIMPLE_NETWORK_PROTOCOL.GetStatus - Invoke
> GetStatus() and verify interface correctness within test case",
> > -                 L"%a:%d:Status - %r, Status1 - %r, InterruptStatus - %d",
> > -                 __FILE__,
> > -                 (UINTN)__LINE__,
> > -                 Status,
> > -                 Status1,
> > -                 InterruptStatus
> > -                 );
> >
> >     //
> >     // Restore SNP State
> >
> 
> 
>