On 6 September 2018 at 16:38, Marcin Wojtas <m...@semihalf.com> wrote: > czw., 6 wrz 2018 o 16:31 Ard Biesheuvel <ard.biesheu...@linaro.org> > napisał(a): >> >> On 6 September 2018 at 16:26, Marcin Wojtas <m...@semihalf.com> wrote: >> > czw., 6 wrz 2018 o 16:04 Ard Biesheuvel <ard.biesheu...@linaro.org> >> > napisał(a): >> >> >> >> On 3 September 2018 at 06:53, Marcin Wojtas <m...@semihalf.com> wrote: >> >> > From: Tomasz Michalec <t...@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. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel