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::233; helo=mail-it0-x233.google.com; envelope-from=mw@semihalf.com; receiver=edk2-devel@lists.01.org Received: from mail-it0-x233.google.com (mail-it0-x233.google.com [IPv6:2607:f8b0:4001:c0b::233]) (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 53E5822590E10 for ; Mon, 16 Apr 2018 23:05:01 -0700 (PDT) Received: by mail-it0-x233.google.com with SMTP id 19-v6so14708772itw.3 for ; Mon, 16 Apr 2018 23:05:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=z966FjA03QIYIK4y70RqP6yOIkXo/PYRULhxszNKixI=; b=hCEAKHfCb/DUd5kfLMG5Nepan25cAw9eKTSjUfQXk4RbcmutwCh/qxyMF66q48DGlv Ufw5I32PjdmnaVv6ZKGklDk8aREX5hZ0mYO1wOzj9c0bgtj34hgrZ/QwGmsofMCAfzyf Ki8VVWWghm4P68UNH94nzC9pXmiKkviQivDKetXcr6am3PIwlluevtKkfoCeNcqn+0Zy HaRhnOOxyjepybNBy7dYG/r47DWbwy1lMNUU3bezq8+5GAi9Dbp97pTT4gDHwZLxTOPZ uPqYu+9Ly8C/i4AUGYs5XpW+3+b9bUehO89x32TDhLWpbYT8mT2LBgodmrzRwLoEcfPI /HbA== 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; bh=z966FjA03QIYIK4y70RqP6yOIkXo/PYRULhxszNKixI=; b=PX4JdfsfS6pJOVJB803TiDVlqmGSNR8Rt5WchPwqPRxtP2x4wh7kJG72kfmq+59NcN rqjxOmRRbHjMzLCKZr/h2ZsTqgNLFpONt5W1nGcRI9ezTHpW0nT3sxIWuYQ2Rt6jJRYp jW6RD8c1L78gXjJ9NHUEpQ3mrDjQ1OuSILmN6/93ZOmUQ/zZE4ywsTy6UUI/+R85KHLT xc+Zqo/vxTecElmN793VnGjNSza6JD2k+q9wphQbfs+G9rOv0LPXYHc89+zpCrgAZO5s VL80hwaZlBgpIwSDpW1DJkIdJo7AZqwZAJBI9/73zKGkpFF1A8LaRFkiEa8C8H8OZeQX cYQQ== X-Gm-Message-State: ALQs6tCIjw4Surq0nPEcR/kJ3lodHCTDqXfo/3rbpDSieaGL8gonuWMp 2UPASw7+vyTAJNgsWLvBOFEXCPZxCZdDopRVeyihLw== X-Google-Smtp-Source: AIpwx48pMOnLrHhzaewWYtVITX7P5hizwyG1GmAu9oi+eXNOIyq5DSWrlI3tJ7Qz1l5IsKx69IgV7Xxr+LAXpau15+Y= X-Received: by 2002:a24:2e4a:: with SMTP id i71-v6mr1076854ita.56.1523945100397; Mon, 16 Apr 2018 23:05:00 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.139.23 with HTTP; Mon, 16 Apr 2018 23:04:59 -0700 (PDT) In-Reply-To: References: <1523855353-9262-1-git-send-email-mw@semihalf.com> <1523855353-9262-2-git-send-email-mw@semihalf.com> <487cbdc8-72f7-e990-f53b-354c88370ba8@redhat.com> From: Marcin Wojtas Date: Tue, 17 Apr 2018 08:04:59 +0200 Message-ID: To: Ard Biesheuvel Cc: Laszlo Ersek , Hua Jing , Grzegorz Jaszczyk , "edk2-devel@lists.01.org" , Leif Lindholm , Nadav Haklai , Neta Zur Hershkovits Subject: Re: [platforms PATCH 1/2] Marvell/Drivers: MvFvbDxe: Adjust to new dependencies X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 17 Apr 2018 06:05:01 -0000 Content-Type: text/plain; charset="UTF-8" Hi Ard, 2018-04-17 7:32 GMT+02:00 Ard Biesheuvel : > On 17 April 2018 at 07:15, Marcin Wojtas wrote: >> Hi Laszlo, >> >> 2018-04-16 21:41 GMT+02:00 Laszlo Ersek : >>> On 04/16/18 07:40, Ard Biesheuvel wrote: >>>> (+ Laszlo) >>>> >>>> On 16 April 2018 at 07:09, Marcin Wojtas wrote: >>>>> Recent changes in the EDK2 mainline resulted in breaking >>>>> of compilation and booting of Armada platforms. >>>>> This patch adjust the MvFvbDxe driver by: >>>>> >>>>> * installation of gEdkiiNvVarStoreFormattedGuid in order to signal >>>>> NvVarStoreFormattedLib to the generic variable runtime driver >>>>> >>>> >>>> Hello Marcin, >>>> >>>> Installing this GUID is only necessary if you update your platform >>>> .DSC to make the generic variable runtime driver depend on it by >>>> adding a NULL library class resolution using NvVarStoreFormattedLib. >>>> So I think this patch is correct, but you'll need an additional change >>>> to make it work as expected. (Otherwise, the variable runtime driver >>>> could still be dispatched early and invoked for read access before the >>>> variable store is reformatted) >>> >>> I agree. >>> >>> I'd also like to point out another frequent pitfall in this patch: >>> >>> >>> While gBS->InstallProtocolInterface() takes a *pointer* to a handle >>> (because it can *create* a handle, if the handle is NULL on input, and >>> the first protocol interface is installed on it), >>> gBS->UninstallProtocolInterface() takes the handle *itself*. If the last >>> protocol interface is uninstalled from the handle, then the handle is >>> destroyed, but gBS->UninstallProtocolInterface() does not attempt to >>> NULL the handle itself. So, here you should pass "gImageHandle", not >>> "&gImageHandle". >>> >> >> Right, I'll correct it. >> > > Ah, I missed that. Thanks for spotting that Laszlo > >>> There's also a bit of whitespace mangling here that's not compatible >>> with either multiline function call style that we like in edk2, but >>> perhaps edk2-platforms treats that more laxly. >>> >> >> We had some discussions last year - I followed the coding standards: >> >> Function ( >> Argument1, >> Argument2, >> Argument3 >> ); >> >> But was requested to place Argument1 to the function line and the last >> bracket to Argument3 line: >> >> Function (Argument1, >> Argument2, >> Argument3); >> >> Afair, there were some attempts to modify coding standards at the >> time, but I see the original version persisted. In fact I can do >> whatever line-breaking necessary: >> >> Ard - what style do you prefer in future patches? >> > > I tend to treat the coding style document as a guideline rather than > rule of law, given that it is not entirely consistent with current > practice to begin with. In my opinion, self consistency and legibility > are more important than adhering to some rule, although I realize > legibility is a subjective quality > > Personally, I think the former takes up too much space in general, but > with complex expressions as arguments, it is more readable than the > latter. Well, I see it can be treated sort of as a matter of taste. In order to be consistent with my previous code, I'd keep variant2 as the line breaking scheme in edk2-platforms. In edk2 I don't suspect much code submitted, but to be on a safe side I will use variant1. Is it ok for you? Thanks, Marcin