Re: [edk2-devel] [PATCH 1/3] OptionRomPkg: Update the comments of GetInformation function

["Ni, Ray" <ray.ni@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 5935 bytes --]

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 <pedro.falcato@gmail.com>
Sent: Tuesday, April 9, 2024 10:12
To: Shang, Qingyu <qingyu.shang@intel.com>
Cc: devel@edk2.groups.io <devel@edk2.groups.io>; Ni, Ray <ray.ni@intel.com>; Gahan Saraiya <gahan.saraiya@intel.com>
Subject: Re: [PATCH 1/3] OptionRomPkg: Update the comments of GetInformation function

On Mon, Apr 8, 2024 at 10:48 AM Qingyu <qingyu.shang@intel.com> 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 <pedro.falcato@gmail.com>

Why was I CC'd on this? /me is confused

> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Qingyu <qingyu.shang@intel.com>
> Signed-off-by: Gahan Saraiya <gahan.saraiya@intel.com>
> ---
>  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.falcato@gmail.com>

--
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]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 10686 bytes --]