* [PATCH 1/1] MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO Protocol Test. @ 2020-01-30 8:18 Gaurav Jain 2020-02-17 7:18 ` Gaurav Jain 2020-02-19 6:34 ` Wu, Hao A 0 siblings, 2 replies; 7+ messages in thread From: Gaurav Jain @ 2020-01-30 8:18 UTC (permalink / raw) To: devel@edk2.groups.io Cc: Jian J Wang, Hao A Wu, Ray Ni, Pankaj Bansal, Gaurav Jain ASSERT in CopyMem_Conf, PollMem_Conf, SetBarAttributes_Conf Conformance Test. SCT Test expect return as Invalid Parameter. So removed ASSERT(). Signed-off-by: Gaurav Jain <gaurav.jain@nxp.com> --- .../NonDiscoverablePciDeviceIo.c | 20 ++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c index 2d55c9699322..76cb000602fc 100644 --- a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c +++ b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c @@ -93,7 +93,15 @@ PciIoPollMem ( OUT UINT64 *Result ) { - ASSERT (FALSE); + if ((UINT32)Width >= EfiPciIoWidthMaximum || + 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 || + 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO Protocol Test. 2020-01-30 8:18 [PATCH 1/1] MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO Protocol Test Gaurav Jain @ 2020-02-17 7:18 ` Gaurav Jain 2020-02-17 8:45 ` [edk2-devel] " Liming Gao 2020-02-19 6:34 ` Wu, Hao A 1 sibling, 1 reply; 7+ messages in thread From: Gaurav Jain @ 2020-02-17 7:18 UTC (permalink / raw) To: devel@edk2.groups.io; +Cc: Jian J Wang, Hao A Wu, Ray Ni, Pankaj Bansal Gentle Reminder!! Please review.. > -----Original Message----- > From: Gaurav Jain <gaurav.jain@nxp.com> > Sent: Thursday, January 30, 2020 1:48 PM > To: devel@edk2.groups.io > Cc: Jian J Wang <jian.j.wang@intel.com>; Hao A Wu <hao.a.wu@intel.com>; > Ray Ni <ray.ni@intel.com>; Pankaj Bansal <pankaj.bansal@nxp.com>; Gaurav > Jain <gaurav.jain@nxp.com> > 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(). > > Signed-off-by: Gaurav Jain <gaurav.jain@nxp.com> > --- > .../NonDiscoverablePciDeviceIo.c | 20 ++++++++++++++++--- > 1 file changed, 17 insertions(+), 3 deletions(-) > > diff --git > a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciD > eviceIo.c > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciD > eviceIo.c > index 2d55c9699322..76cb000602fc 100644 > --- > a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciD > eviceIo.c > +++ > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePc > +++ iDeviceIo.c > @@ -93,7 +93,15 @@ PciIoPollMem ( > OUT UINT64 *Result > ) > { > - ASSERT (FALSE); > + if ((UINT32)Width >= EfiPciIoWidthMaximum || > + 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 || > + 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO Protocol Test. 2020-02-17 7:18 ` Gaurav Jain @ 2020-02-17 8:45 ` Liming Gao 2020-02-19 5:59 ` [EXT] " Gaurav Jain 0 siblings, 1 reply; 7+ messages in thread From: Liming Gao @ 2020-02-17 8:45 UTC (permalink / raw) To: devel@edk2.groups.io, gaurav.jain@nxp.com, Gao, Liming Cc: Wang, Jian J, Wu, Hao A, Ni, Ray, Pankaj Bansal Gaurav: Does this patch catch to edk2 stable tag 202002? Thanks Liming > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gaurav Jain > Sent: Monday, February 17, 2020 3:18 PM > To: devel@edk2.groups.io > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Pankaj Bansal > <pankaj.bansal@nxp.com> > Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO Protocol Test. > > Gentle Reminder!! > Please review.. > > > -----Original Message----- > > From: Gaurav Jain <gaurav.jain@nxp.com> > > Sent: Thursday, January 30, 2020 1:48 PM > > To: devel@edk2.groups.io > > Cc: Jian J Wang <jian.j.wang@intel.com>; Hao A Wu <hao.a.wu@intel.com>; > > Ray Ni <ray.ni@intel.com>; Pankaj Bansal <pankaj.bansal@nxp.com>; Gaurav > > Jain <gaurav.jain@nxp.com> > > 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(). > > > > Signed-off-by: Gaurav Jain <gaurav.jain@nxp.com> > > --- > > .../NonDiscoverablePciDeviceIo.c | 20 ++++++++++++++++--- > > 1 file changed, 17 insertions(+), 3 deletions(-) > > > > diff --git > > a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciD > > eviceIo.c > > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciD > > eviceIo.c > > index 2d55c9699322..76cb000602fc 100644 > > --- > > a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciD > > eviceIo.c > > +++ > > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePc > > +++ iDeviceIo.c > > @@ -93,7 +93,15 @@ PciIoPollMem ( > > OUT UINT64 *Result > > ) > > { > > - ASSERT (FALSE); > > + if ((UINT32)Width >= EfiPciIoWidthMaximum || > > + 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 || > > + 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 > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [EXT] RE: [edk2-devel] [PATCH 1/1] MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO Protocol Test. 2020-02-17 8:45 ` [edk2-devel] " Liming Gao @ 2020-02-19 5:59 ` Gaurav Jain 0 siblings, 0 replies; 7+ messages in thread From: Gaurav Jain @ 2020-02-19 5:59 UTC (permalink / raw) To: Gao, Liming, devel@edk2.groups.io Cc: Wang, Jian J, Wu, Hao A, Ni, Ray, Pankaj Bansal Hi Gao > Gaurav: > Does this patch catch to edk2 stable tag 202002? Yes > -----Original Message----- > From: Gao, Liming <liming.gao@intel.com> > Sent: Monday, February 17, 2020 2:15 PM > To: devel@edk2.groups.io; Gaurav Jain <gaurav.jain@nxp.com>; Gao, Liming > <liming.gao@intel.com> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; > Ni, Ray <ray.ni@intel.com>; Pankaj Bansal <pankaj.bansal@nxp.com> > Subject: [EXT] RE: [edk2-devel] [PATCH 1/1] MdeModulePkg/Pci: Fixed Asserts in > SCT PCIIO Protocol Test. > > Caution: EXT Email > > Gaurav: > Does this patch catch to edk2 stable tag 202002? > > Thanks > Liming > > -----Original Message----- > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gaurav > > Jain > > Sent: Monday, February 17, 2020 3:18 PM > > To: devel@edk2.groups.io > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A > > <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Pankaj Bansal > > <pankaj.bansal@nxp.com> > > Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg/Pci: Fixed Asserts in SCT > PCIIO Protocol Test. > > > > Gentle Reminder!! > > Please review.. > > > > > -----Original Message----- > > > From: Gaurav Jain <gaurav.jain@nxp.com> > > > Sent: Thursday, January 30, 2020 1:48 PM > > > To: devel@edk2.groups.io > > > Cc: Jian J Wang <jian.j.wang@intel.com>; Hao A Wu > > > <hao.a.wu@intel.com>; Ray Ni <ray.ni@intel.com>; Pankaj Bansal > > > <pankaj.bansal@nxp.com>; Gaurav Jain <gaurav.jain@nxp.com> > > > 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(). > > > > > > Signed-off-by: Gaurav Jain <gaurav.jain@nxp.com> > > > --- > > > .../NonDiscoverablePciDeviceIo.c | 20 ++++++++++++++++--- > > > 1 file changed, 17 insertions(+), 3 deletions(-) > > > > > > diff --git > > > > a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePc > > > iD > > > eviceIo.c > > > > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePc > > > iD > > > eviceIo.c > > > index 2d55c9699322..76cb000602fc 100644 > > > --- > > > > a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePc > > > iD > > > eviceIo.c > > > +++ > > > > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePc > > > +++ iDeviceIo.c > > > @@ -93,7 +93,15 @@ PciIoPollMem ( > > > OUT UINT64 *Result > > > ) > > > { > > > - ASSERT (FALSE); > > > + if ((UINT32)Width >= EfiPciIoWidthMaximum || > > > + 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 || > > > + 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 > > > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO Protocol Test. 2020-01-30 8:18 [PATCH 1/1] MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO Protocol Test Gaurav Jain 2020-02-17 7:18 ` Gaurav Jain @ 2020-02-19 6:34 ` Wu, Hao A 2020-02-19 7:48 ` Ard Biesheuvel 2020-02-20 11:33 ` [EXT] " Gaurav Jain 1 sibling, 2 replies; 7+ messages in thread From: Wu, Hao A @ 2020-02-19 6:34 UTC (permalink / raw) To: Gaurav Jain, devel@edk2.groups.io, Ard Biesheuvel Cc: Wang, Jian J, Ni, Ray, Pankaj Bansal > -----Original Message----- > From: Gaurav Jain [mailto:gaurav.jain@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.jain@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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO Protocol Test. 2020-02-19 6:34 ` Wu, Hao A @ 2020-02-19 7:48 ` Ard Biesheuvel 2020-02-20 11:33 ` [EXT] " Gaurav Jain 1 sibling, 0 replies; 7+ messages in thread From: Ard Biesheuvel @ 2020-02-19 7:48 UTC (permalink / raw) To: Wu, Hao A Cc: Gaurav Jain, devel@edk2.groups.io, Wang, Jian J, Ni, Ray, Pankaj Bansal On Wed, 19 Feb 2020 at 07:34, Wu, Hao A <hao.a.wu@intel.com> wrote: > > > -----Original Message----- > > From: Gaurav Jain [mailto:gaurav.jain@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. > 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@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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [EXT] RE: [PATCH 1/1] MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO Protocol Test. 2020-02-19 6:34 ` Wu, Hao A 2020-02-19 7:48 ` Ard Biesheuvel @ 2020-02-20 11:33 ` Gaurav Jain 1 sibling, 0 replies; 7+ messages in thread From: Gaurav Jain @ 2020-02-20 11:33 UTC (permalink / raw) To: Wu, Hao A, devel@edk2.groups.io, Ard Biesheuvel Cc: Wang, Jian J, Ni, Ray, Pankaj Bansal > -----Original Message----- > From: Wu, Hao A <hao.a.wu@intel.com> > Sent: Wednesday, February 19, 2020 12:04 PM > To: Gaurav Jain <gaurav.jain@nxp.com>; devel@edk2.groups.io; Ard Biesheuvel > <ard.biesheuvel@linaro.org> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Ni, Ray <ray.ni@intel.com>; Pankaj > Bansal <pankaj.bansal@nxp.com> > Subject: [EXT] RE: [PATCH 1/1] MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO > Protocol Test. > > Caution: EXT Email > > > -----Original Message----- > > From: Gaurav Jain [mailto:gaurav.jain@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. Done the suggested changes and sent v2. Please review. > > > > > > Signed-off-by: Gaurav Jain <gaurav.jain@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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-02-20 11:33 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-01-30 8:18 [PATCH 1/1] MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO Protocol Test Gaurav Jain 2020-02-17 7:18 ` Gaurav Jain 2020-02-17 8:45 ` [edk2-devel] " Liming Gao 2020-02-19 5:59 ` [EXT] " Gaurav Jain 2020-02-19 6:34 ` Wu, Hao A 2020-02-19 7:48 ` Ard Biesheuvel 2020-02-20 11:33 ` [EXT] " Gaurav Jain
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox