Re: [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  wrote:
>
> > -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.
>

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 
> > ---
> >  .../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 UINTNCount
> >)
> >  {
> > -  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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Wu, Hao A]

> -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 
> ---
>  .../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 UINTNCount
>)
>  {
> -  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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Gaurav Jain]

Hi Gao

> Gaurav:
>   Does this patch catch to edk2 stable tag 202002?
Yes

> -Original Message-
> From: Gao, Liming 
> Sent: Monday, February 17, 2020 2:15 PM
> To: devel@edk2.groups.io; Gaurav Jain ; Gao, Liming
> 
> Cc: Wang, Jian J ; Wu, Hao A ;
> Ni, Ray ; Pankaj Bansal 
> 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  On Behalf Of Gaurav
> > Jain
> > Sent: Monday, February 17, 2020 3:18 PM
> > To: devel@edk2.groups.io
> > Cc: Wang, Jian J ; Wu, Hao A
> > ; Ni, Ray ; Pankaj Bansal
> > 
> > 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 
> > > Sent: Thursday, January 30, 2020 1:48 PM
> > > To: devel@edk2.groups.io
> > > Cc: Jian J Wang ; Hao A Wu
> > > ; Ray Ni ; 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().
> > >
> > > Signed-off-by: Gaurav Jain 
> > > ---
> > >  .../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 UINTNCount
> > >)
> > >  {
> > > -  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
> >
> >
> > 


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

View/Reply Online (#54604): https://edk2.groups.io/g/devel/message/54604
Mute This Topic: https://groups.io/mt/71389950/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Liming Gao]

Gaurav:
  Does this patch catch to edk2 stable tag 202002? 

Thanks
Liming
> -Original Message-
> From: 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 ; Wu, Hao A ; Ni, 
> Ray ; Pankaj Bansal
> 
> 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 
> > Sent: Thursday, January 30, 2020 1:48 PM
> > To: devel@edk2.groups.io
> > Cc: Jian J Wang ; Hao A Wu ;
> > Ray Ni ; 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().
> >
> > Signed-off-by: Gaurav Jain 
> > ---
> >  .../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 UINTNCount
> >)
> >  {
> > -  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
> 
> 
> 


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

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

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

[Gaurav Jain]

Gentle Reminder!!
Please review..

> -Original Message-
> From: Gaurav Jain 
> Sent: Thursday, January 30, 2020 1:48 PM
> To: devel@edk2.groups.io
> Cc: Jian J Wang ; Hao A Wu ;
> Ray Ni ; 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().
> 
> Signed-off-by: Gaurav Jain 
> ---
>  .../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 UINTNCount
>)
>  {
> -  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


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

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