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::231; helo=mail-it0-x231.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x231.google.com (mail-it0-x231.google.com [IPv6:2607:f8b0:4001:c0b::231]) (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 6F74222590E10 for ; Mon, 16 Apr 2018 23:05:40 -0700 (PDT) Received: by mail-it0-x231.google.com with SMTP id q85-v6so14735254itc.0 for ; Mon, 16 Apr 2018 23:05:40 -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; bh=yyT3HEpGxZMcvdXqziYl9weC8dXVC5hz5s7wzl+SqzQ=; b=CmDaY3ai5LiPOj6f98jwPb7kSwk0rillJwhQBbFNT9SvA3YN3WkBdbti0pH6bJ2KDZ 5M7uvzDRK5Bd6S+Qz9XF1DX1OuWHNV1roNksM4REV7X7u/nO2aXP5DpYFLw3xEllZ4hT i6olUnbfr1gbpxdV64RB+M1R/wlXapgZcXusM= 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=yyT3HEpGxZMcvdXqziYl9weC8dXVC5hz5s7wzl+SqzQ=; b=Vq5n2aDquvbZsZIAczQGFwHXTfsm2C2B0tGUdXmRo5BZHDhI1Y8ZN+4W2ly5xQzYYp QiKsBESBTypSygZLRijZTJgmmadMh2EWIgB2OHa0Z0awPVSUSjlnbm88q+hrqzl+M4s/ VWOfPhnXRYkvHX3Jkuk8n6H0D8I5TsCBcASNk76ZFojFnPw0TSQJhVNJQidmq02MVet0 KzQvUPksCDWKZnpiAD7tLEyfRL5EX133RQSnkRBTCS/owcPfgAvODxpx7T+Bv9azLb05 Gx0TJ0PHAZNNTysy+GQ4WjtK+hNuw65Jp1t1OvDgnw9udIvA7BP+Z7MLlkiNmlT2gFT+ kkrw== X-Gm-Message-State: ALQs6tBik9WjUBUeP+KdXoePxnNBcbzchRB/jWafKNIDtQMUxYsu7p85 0HtI3wPH91ch78XRiPhvE2xuMcJR7rx0LWF4Yv1hJw== X-Google-Smtp-Source: AIpwx4/U1VN9VCIg4La6+JfhNA7JAkawv/ExCn114VbauuTRbXcpOE0Xyb81roHw/iftJFWm4IhN06BkiDdAey3kSYo= X-Received: by 2002:a24:36c9:: with SMTP id l192-v6mr1114357itl.42.1523945139616; Mon, 16 Apr 2018 23:05:39 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.187.67 with HTTP; Mon, 16 Apr 2018 23:05:39 -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: Ard Biesheuvel Date: Tue, 17 Apr 2018 08:05:39 +0200 Message-ID: To: Marcin Wojtas 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:40 -0000 Content-Type: text/plain; charset="UTF-8" On 17 April 2018 at 08:04, Marcin Wojtas wrote: > 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? > Works for me