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

[Wu, Hao A]

> -----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.

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 (#54605): https://edk2.groups.io/g/devel/message/54605
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]
-=-=-=-=-=-=-=-=-=-=-=-