[edk2-devel] [PATCH 1/1] MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO Protocol Test.

[Ard Biesheuvel]

On Wed, 19 Feb 2020 at 07:34, Wu, Hao A <hao.a.wu at intel.com> wrote:
>
> > -----Original Message-----
> > From: Gaurav Jain [mailto:gaurav.jain at nxp.com]
> > Sent: Thursday, January 30, 2020 4:18 PM
> > To: devel at edk2.groups.io
> > Cc: Wang, Jian J; Wu, Hao A; Ni, Ray; Pankaj Bansal; Gaurav Jain
> > Subject: [PATCH 1/1] MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO Protocol
> > Test.
> >
> > ASSERT in CopyMem_Conf, PollMem_Conf, SetBarAttributes_Conf
> > Conformance Test.
> > SCT Test expect return as Invalid Parameter.
> > So removed ASSERT().
>
>
> Include Ard (as the driver contributor) to see if he has additional comments.
>
> A couple of general level comments:
> 1. I think the ASSERT can still be added after the checks you add. The ASSERT
>    may serve as a notification in the DEBUG image that the service is not
>    implemented.
>

Agreed.

> 2. I found that for functions:
>    PciIoPollIo()
>    PciIoIoRead()
>    PciIoIoWrite()
>    They also do not have checks that perfectly match with the UEFI spec. Even
>    though they are not exposed by the SCT tests, could you help to address them
>    as well?
>
> Some other inline comments below.
>
>
> >
> > Signed-off-by: Gaurav Jain <gaurav.jain at nxp.com>
> > ---
> >  .../NonDiscoverablePciDeviceIo.c              | 20 ++++++++++++++++---
> >  1 file changed, 17 insertions(+), 3 deletions(-)
> >
> > diff --git
> > a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverableP
> > ciDeviceIo.c
> > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverableP
> > ciDeviceIo.c
> > index 2d55c9699322..76cb000602fc 100644
> > ---
> > a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverableP
> > ciDeviceIo.c
> > +++
> > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverableP
> > ciDeviceIo.c
> > @@ -93,7 +93,15 @@ PciIoPollMem (
> >    OUT UINT64                      *Result
> >    )
> >  {
> > -  ASSERT (FALSE);
> > +  if ((UINT32)Width >= EfiPciIoWidthMaximum ||
>
>
> Looks to me that the 1st part of the 'if' check is redundant.
> Could you help to double confirm?
>
>
> > +      Width > EfiPciIoWidthUint64) {
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  if (Result == NULL) {
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> >    return EFI_UNSUPPORTED;
> >  }
> >
> > @@ -556,7 +564,10 @@ PciIoCopyMem (
> >    IN     UINTN                        Count
> >    )
> >  {
> > -  ASSERT (FALSE);
> > +  if ((UINT32)Width >= EfiPciIoWidthMaximum ||
>
>
> Looks to me that the 1st part of the 'if' check is redundant.
> Could you help to double confirm?
>
> Best Regards,
> Hao Wu
>
>
> > +      Width > EfiPciIoWidthUint64) {
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> >    return EFI_UNSUPPORTED;
> >  }
> >
> > @@ -1414,7 +1425,10 @@ PciIoSetBarAttributes (
> >    IN OUT UINT64                       *Length
> >    )
> >  {
> > -  ASSERT (FALSE);
> > +  if (Offset == NULL || Length == NULL) {
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> >    return EFI_UNSUPPORTED;
> >  }
> >
> > --
> > 2.17.1
>

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#54610): https://edk2.groups.io/g/devel/message/54610
Mute This Topic: https://groups.io/mt/70267136/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-