> -----Original Message-----
> From: Gaurav Jain [mailto:gaurav.j...@nxp.com]
> Sent: Thursday, January 30, 2020 4:18 PM
> To: devel@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.j...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
