Pedro, I didn't notice your mail and merged the patch.:( Your comments to the commit messages are good to me. However, I am ok with the changes to the function header of an existing implementation. Thanks, Ray ________________________________ From: Pedro Falcato Sent: Tuesday, April 9, 2024 10:12 To: Shang, Qingyu Cc: devel@edk2.groups.io ; Ni, Ray ; Gahan Saraiya Subject: Re: [PATCH 1/3] OptionRomPkg: Update the comments of GetInformation function On Mon, Apr 8, 2024 at 10:48 AM Qingyu wrote: > > Refer to Uefi spec 2.10 section 11.11.2, add a new retval > EFI_NOT_FOUND to EFI_ADAPTER_INFORMATION_PROTOCOL.GetInformation(). > Reference: [mantis #1866] - GetInfo() of Adapter Information > Protocol should have a provision for IHV to return no data. Let's reword this commit message a bit, shall we? Something like this: Add a new return value EFI_NOT_FOUND to EFI_ADAPTER_INFORMATION_PROTOCOL.GetInformation(), according to UEFI spec 2.10 section 11.11.2. This brings the documentation up to par with UEFI 2.10. Reference: [mantis #1866] - GetInfo() of Adapter Information Protocol should have a provision for IHV to return no data. I'm not sure about the commit title too, but it's late here and I can't figure out a nice succinct description. Maybe: "OptionRomPkg/UndiRuntimeDxe: Update UndiAipGetInfo's docs to UEFI spec 2.10" > > Cc: Pedro Falcato Why was I CC'd on this? /me is confused > Cc: Ray Ni > Signed-off-by: Qingyu > Signed-off-by: Gahan Saraiya > --- > Drivers/OptionRomPkg/UndiRuntimeDxe/Undi32.h | 5 ++++- > Drivers/OptionRomPkg/UndiRuntimeDxe/UndiAipImpl.c | 5 ++++- > 2 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/Drivers/OptionRomPkg/UndiRuntimeDxe/Undi32.h b/Drivers/OptionRomPkg/UndiRuntimeDxe/Undi32.h > index 31c55a8e11..665221e952 100644 > --- a/Drivers/OptionRomPkg/UndiRuntimeDxe/Undi32.h > +++ b/Drivers/OptionRomPkg/UndiRuntimeDxe/Undi32.h > @@ -350,7 +350,9 @@ VOID PxeUpdate (NIC_DATA_INSTANCE *NicPtr, PXE_SW_UNDI *PxePtr); > > This function returns information of type InformationType from the adapter. > If an adapter does not support the requested informational type, then > - EFI_UNSUPPORTED is returned. > + EFI_UNSUPPORTED is returned. If an adapter does not contain Information for > + the requested InformationType, it fills InformationBlockSize with 0 and > + returns EFI_NOT_FOUND. > > @param[in] This A pointer to the EFI_ADAPTER_INFORMATION_PROTOCOL instance. > @param[in] InformationType A pointer to an EFI_GUID that defines the contents of InformationBlock. > @@ -360,6 +362,7 @@ VOID PxeUpdate (NIC_DATA_INSTANCE *NicPtr, PXE_SW_UNDI *PxePtr); > > @retval EFI_SUCCESS The InformationType information was retrieved. > @retval EFI_UNSUPPORTED The InformationType is not known. > + @retval EFI_NOT_FOUND Information is not available for the requested information type. > @retval EFI_DEVICE_ERROR The device reported an error. > @retval EFI_OUT_OF_RESOURCES The request could not be completed due to a lack of resources. > @retval EFI_INVALID_PARAMETER This is NULL. > diff --git a/Drivers/OptionRomPkg/UndiRuntimeDxe/UndiAipImpl.c b/Drivers/OptionRomPkg/UndiRuntimeDxe/UndiAipImpl.c > index 21151a076f..d80ce65da9 100644 > --- a/Drivers/OptionRomPkg/UndiRuntimeDxe/UndiAipImpl.c > +++ b/Drivers/OptionRomPkg/UndiRuntimeDxe/UndiAipImpl.c > @@ -18,7 +18,9 @@ EFI_GUID mSupportedInfoTypes[] = { > > This function returns information of type InformationType from the adapter. > If an adapter does not support the requested informational type, then > - EFI_UNSUPPORTED is returned. > + EFI_UNSUPPORTED is returned. If an adapter does not contain Information for > + the requested InformationType, it fills InformationBlockSize with 0 and > + returns EFI_NOT_FOUND. > > @param[in] This A pointer to the EFI_ADAPTER_INFORMATION_PROTOCOL instance. > @param[in] InformationType A pointer to an EFI_GUID that defines the contents of InformationBlock. > @@ -28,6 +30,7 @@ EFI_GUID mSupportedInfoTypes[] = { > > @retval EFI_SUCCESS The InformationType information was retrieved. > @retval EFI_UNSUPPORTED The InformationType is not known. > + @retval EFI_NOT_FOUND Information is not available for the requested information type. > @retval EFI_DEVICE_ERROR The device reported an error. > @retval EFI_OUT_OF_RESOURCES The request could not be completed due to a lack of resources. > @retval EFI_INVALID_PARAMETER This is NULL. In any case, since I've been meaning to say this for some time: I know this is not your fault (and this is part of some UEFI spec upgrade goal), but changing all of these comments isn't the win you think it is. It's very churny and gains us nothing. The function does not return EFI_NOT_FOUND, so why are we changing its docs? Changing the protocol header's docs is fine (and expected), changing the individual implementations is very... iffy. I'm not a maintainer for this, but if this helps: with the changes above: Acked-by: Pedro Falcato -- Pedro -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#117566): https://edk2.groups.io/g/devel/message/117566 Mute This Topic: https://groups.io/mt/105398061/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-