Amit, If consumer calls OpenProtocol with Attributes = EFI_OPEN_PROTOCOL_TEST_PROTOCOL and Interface = NULL, and the protocol could not be found, how could the code with the patch work to assign *Interface = NULL?
And also UEFI spec has words Interface will be ignored if Attributes is EFI_OPEN_PROTOCOL_TEST_PROTOCOL. “InterfaceSupplies the address where a pointer to the corresponding Protocol Interface is returned. NULL will be returned in *Interface if a structure is not associated with Protocol. This parameter is optional, and will be ignored if Attributes is EFI_OPEN_PROTOCOL_TEST_PROTOCOL.” Thanks, Star From: Amit Kumar [mailto:amit...@samsung.com] Sent: Thursday, June 22, 2017 8:54 PM To: edk2-devel@lists.01.org; Gao, Liming <liming....@intel.com>; Zeng, Star <star.z...@intel.com>; Kinney, Michael D <michael.d.kin...@intel.com> Cc: Tian, Feng <feng.t...@intel.com> Subject: RE: RE: [edk2] [PATCH V3] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol >>Should the code >> >>+ // Return NULL Interface if Unsupported Protocol >>+ *Interface = NULL; >> >>be >> >>+ if (Attributes != EFI_OPEN_PROTOCOL_TEST_PROTOCOL) { >>+ // Return NULL Interface if Unsupported Protocol >>+ *Interface = NULL; >>+ } >> >>to cover the case Attributes = EFI_OPEN_PROTOCOL_TEST_PROTOCOL and Interface >>= NULL? >> >> >>Thanks, >>Star Hello Star, It looks like and you suggestion is apt and satisfies the use case scenarios of EFI_OPEN_PROTOCOL_TEST_PROTOCOL. But if we talk of UEFI Spec 2.7, I think the patch is good enough . I would like to have some suggestion here from all the concerned people, if agreed I would add these changes to the patch. Thanks And Regards Amit --------- Original Message --------- Sender : Zeng, Star <star.z...@intel.com<mailto:star.z...@intel.com>> Date : 2017-06-20 15:28 (GMT+5:30) Title : RE: [edk2] [PATCH V3] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol To : Amit Kumar<amit...@samsung.com<mailto:amit...@samsung.com>>, null<edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>> CC : null<liming....@intel.com<mailto:liming....@intel.com>>, null<star.z...@intel.com<mailto:star.z...@intel.com>> Should the code + // Return NULL Interface if Unsupported Protocol + *Interface = NULL; be + if (Attributes != EFI_OPEN_PROTOCOL_TEST_PROTOCOL) { + // Return NULL Interface if Unsupported Protocol + *Interface = NULL; + } to cover the case Attributes = EFI_OPEN_PROTOCOL_TEST_PROTOCOL and Interface = NULL? Thanks, Star -----Original Message----- From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Amit Kumar Sent: Monday, June 19, 2017 8:24 PM To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> Cc: Tian, Feng <feng.t...@intel.com<mailto:feng.t...@intel.com>> Subject: [edk2] [PATCH V3] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol Change Since v2: 1) Modified to use EFI_ERROR to get status code Change since v1: 1) Fixed typo protocal to protocol 2) Fixed coding style Modified source code to update Interface as per spec. 1) In case of Protocol is un-supported, interface should be returned NULL. 2) In case of any error, interface should not be modified. 3) In case of Test Protocol, interface is optional. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Amit Kumar <amit...@samsung.com<mailto:amit...@samsung.com>> --- MdeModulePkg/Core/Dxe/Hand/Handle.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.c b/MdeModulePkg/Core/Dxe/Hand/Handle.c index 1c25521..6de300f 100644 --- a/MdeModulePkg/Core/Dxe/Hand/Handle.c +++ b/MdeModulePkg/Core/Dxe/Hand/Handle.c @@ -1004,12 +1004,8 @@ CoreOpenProtocol ( // // Check for invalid Interface // - if (Attributes != EFI_OPEN_PROTOCOL_TEST_PROTOCOL) { - if (Interface == NULL) { - return EFI_INVALID_PARAMETER; - } else { - *Interface = NULL; - } + if ((Attributes != EFI_OPEN_PROTOCOL_TEST_PROTOCOL) && (Interface == NULL)) { + return EFI_INVALID_PARAMETER; } // @@ -1073,15 +1069,11 @@ CoreOpenProtocol ( Prot = CoreGetProtocolInterface (UserHandle, Protocol); if (Prot == NULL) { Status = EFI_UNSUPPORTED; + // Return NULL Interface if Unsupported Protocol + *Interface = NULL; goto Done; } - // - // This is the protocol interface entry for this protocol - // - if (Attributes != EFI_OPEN_PROTOCOL_TEST_PROTOCOL) { - *Interface = Prot->Interface; - } Status = EFI_SUCCESS; ByDriver = FALSE; @@ -1175,6 +1167,14 @@ CoreOpenProtocol ( } Done: + + // + // This is the protocol interface entry for this protocol. + // In case of any Error, Interface should not be updated as per spec. + // + if (!EFI_ERROR (Status) && Attributes != EFI_OPEN_PROTOCOL_TEST_PROTOCOL) { + *Interface = Prot->Interface; + } // // Done. Release the database lock are return // -- 1.9.1 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> https://lists.01.org/mailman/listinfo/edk2-devel [cid:image001.png@01D2EB9B.0901B210] [http://ext.samsung.net/mail/ext/v1/external/status/update?userid=amit.ak&do=bWFpbElEPTIwMTcwNjIyMTI1NDE5ZXBjbXM1cDM4NDkyNjBiOGMyNmYyMzM1MjJlMTYwYzlhMzQ2OTMzZSZyZWNpcGllbnRBZGRyZXNzPXN0YXIuemVuZ0BpbnRlbC5jb20_] _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
