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

[Marcin Wojtas <mw@semihalf.com>]

czw., 6 wrz 2018 o 16:04 Ard Biesheuvel <ard.biesheuvel@linaro.org> napisał(a):
>
> 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.
>

In such case this patch is really not needed.

> 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.

If the newly added callback are zero-initialized, the situation is
fine as they won't be executed.

>
> 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.
>

So even if the given SdMmcOverride protocol callback will be NULL for
Synquacer controller, is there still a risk that anything could be
broken without the version check?

Best regards,
Marcin

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