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

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

On 6 September 2018 at 16:26, Marcin Wojtas <mw@semihalf.com> wrote:
> 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.
>

Yes, but this patch does not change that situation at all.

So please, explain which problem is fixed by this patch?

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

Yes. In EDK2, you can combine binary drivers with drivers build from
source. If a binary driver was built against an older version of the
SdMmcOverride header, it may have non-NULL values in the locations of
the new methods. This patch does not help against that scenario.