Re: [platforms: PATCH 1/7] Silicon/SynQuacer/PlatformDxe: Modify initialization of SdMmcOverride

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 3 September 2018 at 06:53, Marcin Wojtas <mw@semihalf.com> wrote:
> From: Tomasz Michalec <tm@semihalf.com>
>
> This patch changes way the EDKII_SD_MMC_OVERRIDE protocol
> sturcture is allocated. Using AllocateZeroPool and then
> seting callbacks in the structure allow driver to be immune to
> adding new callbacks in SdMmcOveride protocol in future.
>

What is the point of this patch?

Statically allocating the structure will zero initialize the members
that are not initialized explicitly, but only the members that are
known to exist at compile time.

I guess the idea of this patch is to work around the latter
limitation, but unfortunately, using sizeof(EDKII_SD_MMC_OVERRIDE)
puts you in the exact same situation.

This is the reason I added the version field. New hooks should only be
added after incrementing the version, and calling the new hooks should
only occur if the runtime version of the protocol implementation is
greater than or equal to the version where those hooks were first
introduced.


> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c
> index e0987c9..1ad8b88 100644
> --- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c
> +++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c
> @@ -59,6 +59,8 @@
>
>  STATIC EFI_HANDLE mSdMmcControllerHandle;
>
> +STATIC EDKII_SD_MMC_OVERRIDE           *mSdMmcOverride;
> +
>  STATIC EFI_ACPI_DESCRIPTION_HEADER      *mSsdt;
>  STATIC UINTN                            mSsdtSize;
>  STATIC VOID                             *mEventRegistration;
> @@ -180,12 +182,6 @@ SynQuacerSdMmcNotifyPhase (
>    return EFI_SUCCESS;
>  }
>
> -STATIC EDKII_SD_MMC_OVERRIDE mSdMmcOverride = {
> -  EDKII_SD_MMC_OVERRIDE_PROTOCOL_VERSION,
> -  SynQuacerSdMmcCapability,
> -  SynQuacerSdMmcNotifyPhase,
> -};
> -
>  STATIC
>  VOID
>  EFIAPI
> @@ -255,10 +251,20 @@ RegisterEmmc (
>               SYNQUACER_EMMC_BASE, SYNQUACER_EMMC_BASE_SZ);
>    ASSERT_EFI_ERROR (Status);
>
> +  mSdMmcOverride = AllocateZeroPool (sizeof (EDKII_SD_MMC_OVERRIDE));
> +  if (mSdMmcOverride == NULL) {
> +    DEBUG ((DEBUG_ERROR, "%a: Cannot allocate memory\n", __FUNCTION__));
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  mSdMmcOverride->Version = EDKII_SD_MMC_OVERRIDE_PROTOCOL_VERSION;
> +  mSdMmcOverride->Capability = SynQuacerSdMmcCapability;
> +  mSdMmcOverride->NotifyPhase = SynQuacerSdMmcNotifyPhase;
> +
>    Handle = NULL;
>    Status = gBS->InstallProtocolInterface (&Handle,
>                    &gEdkiiSdMmcOverrideProtocolGuid,
> -                  EFI_NATIVE_INTERFACE, (VOID **)&mSdMmcOverride);
> +                  EFI_NATIVE_INTERFACE, mSdMmcOverride);
>    ASSERT_EFI_ERROR (Status);
>
>    return EFI_SUCCESS;
> --
> 2.7.4
>