From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: None (no SPF record) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::230; helo=mail-it0-x230.google.com; envelope-from=mw@semihalf.com; receiver=edk2-devel@lists.01.org Received: from mail-it0-x230.google.com (mail-it0-x230.google.com [IPv6:2607:f8b0:4001:c0b::230]) (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 9ED2421122949 for ; Thu, 6 Sep 2018 07:38:37 -0700 (PDT) Received: by mail-it0-x230.google.com with SMTP id x79-v6so19916106ita.1 for ; Thu, 06 Sep 2018 07:38:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=sTWvjF4lnCizH4T6QoeRz76Ic3WmDLUTBEE3UL2lliA=; b=OCx+5B0PMQqcogHHYwXqwAQLk0XG82vFcP9YsLjqgkAsQRWLxDHCzBPu8saW089oHA 5DjCqILrnujRshs7IGuVYy6i7pnlkjtu728OYK4iURojaNRpnVxQajoUkD4EVaoGRF6n o4+iHPJu4RiY3PaZrRSo+WMG+XrK4lGNUKn9pn0xWjTRTmxE1XM8+Fk9rX7Rk1j/XrJP 3ysRRVmkrF3axDORCS34pmnIN9Cvve0AJezaLAgC96/M45bnYNH4pDt8gQrdMkz6J1GI bQ86oMB/W/VInd1kqz+NX8vOLHKPKPFiJjw3x/rBfpJLDoPf00j7oCO5WGwUFXqlRn61 /eEA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=sTWvjF4lnCizH4T6QoeRz76Ic3WmDLUTBEE3UL2lliA=; b=tJqBvd3wrfYfr/hLYxg5GEVeP4piGfUw8zeEwt6je4XEZPXExblHUMpOmSECnRHnjl 5owyX+sCxr8K9h5H13WpPVlI16qCJLBAGg0lUwjKjzJHpXpd0yz2gC+7oIOos7TuA4sz nvPc9om+khM0EGEeLbe8cyQ/CHu6SCLeS62EjNIWiPYOmYo8f6uwmzFALvOVVi/LBtpx qhPLeNOOn585D+dAxkQ/4W59E+FwPDU8Pkw1FAV5OxkikDWYeY4OCDxrBgOncp1EuXl2 gw7AxDnThyn/0e4013+faplvlbQThpQ6eCMlbekHBJ21Bmo7BBhdvKZ/1vTf/tEGtEdL L5JA== X-Gm-Message-State: APzg51BrMS0EVAuki/uO5bt5vkr98YydHZdPVQ0cuWdwgVJplxq0IyrP HE6ktM43JY4wnUk725mCvr2elxOVokIo7P0JphMSKJeR+Oo= X-Google-Smtp-Source: ANB0VdaM2XMgKYx1fp5hBemLacwZTcLXULhZDnFlNYqzmL9/HqH6vkEH7XdE0CQJxNIY/P2iRBErP6brLuAL6pS9Eyg= X-Received: by 2002:a24:e4ca:: with SMTP id o193-v6mr2829119ith.132.1536244716932; Thu, 06 Sep 2018 07:38:36 -0700 (PDT) MIME-Version: 1.0 References: <1535950443-27106-1-git-send-email-mw@semihalf.com> <1535950443-27106-2-git-send-email-mw@semihalf.com> In-Reply-To: From: Marcin Wojtas Date: Thu, 6 Sep 2018 16:38:24 +0200 Message-ID: To: Ard Biesheuvel Cc: edk2-devel-01 , Leif Lindholm , nadavh@marvell.com, "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: Thu, 06 Sep 2018 14:38:37 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable czw., 6 wrz 2018 o 16:31 Ard Biesheuvel napisa= =C5=82(a): > > On 6 September 2018 at 16:26, Marcin Wojtas wrote: > > czw., 6 wrz 2018 o 16:04 Ard Biesheuvel nap= isa=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 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? Marcin