From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::241; helo=mail-it0-x241.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x241.google.com (mail-it0-x241.google.com [IPv6:2607:f8b0:4001:c0b::241]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id B7B0221123E09 for ; Fri, 7 Sep 2018 04:29:26 -0700 (PDT) Received: by mail-it0-x241.google.com with SMTP id 139-v6so19481260itf.0 for ; Fri, 07 Sep 2018 04:29:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=uknbokpNL/HnFhJZEEp+TO4OOWbj3GwSD3jD/C8QYIE=; b=ATQ7jlsX2p5Q4MwRThUgI/B+2cBPG57cCaqUKRLzn7J6NCdK6nf0ggWcGboBQt94vw 5YdZspalcSEdyLwZt6/3iKE30PBfoSFOxPHONnhc1L7bBfkDiEIq8vZXrUyqq/cB+5E7 GvdRY0aue5NwofBWhak9BbAuSq84WYtYdZOy4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=uknbokpNL/HnFhJZEEp+TO4OOWbj3GwSD3jD/C8QYIE=; b=izIEZHLvl5fZZkjodcLeGYhkKCNPW7UWTkjYCwRY/yLejY9QNR+hiyIbj7aXs91sFB Mzh38BZIiIYXg9JhHiqhBBSVWjQcGMT0MOQoxKW77/FP4E0VJJDKmqYXv0K4sb7KZpms 2coqYe5WjKeNUQ1jenqX6cd16zHymgSORSNaF8yW8Mtnp8FNAS1eVNqntWtjSZ/x0QuD HDTnZyY4/ZY5j8SHl5Mb738ivCACsd1SvJBxN+SFmyReUS+tBZwOZbP2HdzWjKExwIwW ejYCpwNm7m4yFEziwv5lFvLowuRhO8PxyuMf87nijwlvuVSqcuEOMUZvQ2sGRrad3Pls rFGQ== X-Gm-Message-State: APzg51AoZ230sW9DTobHUoN0UmhBk9YGJdMQGUeNLiqed4OaOafD74PT vCIDTldAFAj+4TfF12i2bSpYMZLFObBEqBcVWPgrng== X-Google-Smtp-Source: ANB0VdZEFYxNoVxP9N5kgGtMqRc9fFzsCo3rwg3xJnJ0ifKFFttU1vPa8ohmB6stnGangbYQLtLz9F7HIbzBReP0Jo8= X-Received: by 2002:a24:57cb:: with SMTP id u194-v6mr6533804ita.148.1536319765943; Fri, 07 Sep 2018 04:29:25 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a6b:1c06:0:0:0:0:0 with HTTP; Fri, 7 Sep 2018 04:29:25 -0700 (PDT) In-Reply-To: References: <1535950443-27106-1-git-send-email-mw@semihalf.com> <1535950443-27106-2-git-send-email-mw@semihalf.com> From: Ard Biesheuvel Date: Fri, 7 Sep 2018 13:29:25 +0200 Message-ID: To: Marcin Wojtas Cc: edk2-devel-01 , Leif Lindholm , Nadav Haklai , "jsd@semihalf.com" , Grzegorz Jaszczyk , Tomasz Michalec Subject: Re: [platforms: PATCH 1/7] Silicon/SynQuacer/PlatformDxe: Modify initialization of SdMmcOverride X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 07 Sep 2018 11:29:26 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On 6 September 2018 at 16:45, Ard Biesheuvel wr= ote: > On 6 September 2018 at 16:38, Marcin Wojtas wrote: >> czw., 6 wrz 2018 o 16:31 Ard Biesheuvel napi= sa=C5=82(a): >>> >>> On 6 September 2018 at 16:26, Marcin Wojtas wrote: >>> > czw., 6 wrz 2018 o 16:04 Ard Biesheuvel n= apisa=C5=82(a): >>> >> >>> >> On 3 September 2018 at 06:53, Marcin Wojtas wrote: >>> >> > From: Tomasz Michalec >>> >> > >>> >> > 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 shou= ld >>> >> 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 >=3D 0x2 && (p)->SwitchClockFreqPost !=3D= NULL) > > so that the version handling is completely contained in the header file. Actually, would it be possible to define a new phase for this and use the existing NotifyPhase hook? I know you need the timing parameter, but I'm not thrilled by all the API changes you require there, so perhaps we can solve that differently. In any case, it might be useful if you could provide an overview of all the quirks needed by Xenon