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

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

On 6 September 2018 at 16:38, Marcin Wojtas <mw@semihalf.com> wrote:
> czw., 6 wrz 2018 o 16:31 Ard Biesheuvel <ard.biesheuvel@linaro.org> napisał(a):
>>
>> 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?
>
> None, we only forgot, the static initializer will zero non-declared
> fields by default.
>
>>
>> >>
>> >> 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.
>
> Indeed, this is why it will disappear from v2. So, when adding the new
> callbacks, the version should be increased and checked in relevant
> places of the main EDK2 driver, right?
>
> Because a couple of the new callbacks are introduced, would it be ok,
> to increment the version only once, i.e. v2 of the SdMmcOverride will
> support 4 new routines?
>

Yes, that is preferred in my opinion.

Also, perhaps add some helper macros, e.g.,

#define EDKII_SD_MMC_OVERRIDE_HAVE_POST_CLOCK_FREQ_SWITCH(p) \
                  ((p)->Version >= 0x2 && (p)->SwitchClockFreqPost != NULL)

so that the version handling is completely contained in the header file.