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:c06::230; helo=mail-io0-x230.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io0-x230.google.com (mail-io0-x230.google.com [IPv6:2607:f8b0:4001:c06::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 B88EF22590E38 for ; Mon, 16 Apr 2018 22:33:01 -0700 (PDT) Received: by mail-io0-x230.google.com with SMTP id s14so4151771ioc.0 for ; Mon, 16 Apr 2018 22:33:01 -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=zzn++DFUDsU9r5r37gC5n2XX/KOaa3z+/o6BGOrz9Ck=; b=H0H6SUZPBrAoLhdru4fQ+b12Jr4VpyNGBYRJhSCouGEREBO8+jVPuqYrxGq+QAtVSS QAJc0pKgrUhh8lKfiQYB5omv/0TYUKbN68EUSqWqxRPur7ugQ6aDZ38NjblfYZ7gGmgT lgnbw0v9Xn+MzWEexjXvgQlkLOQWQakIc5YjI= 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=zzn++DFUDsU9r5r37gC5n2XX/KOaa3z+/o6BGOrz9Ck=; b=XjVFVIOjJfIdnLQAxNZZwwuM6EreaT0XlAt8ADlRqUn7suH9/WbmCJIZp4C86eo77c AlwbW4KQUx3WUCtZejaxTFzn8uogUQenZE9u9IYEnd4PFXFpeKE/irGs/qbUNqPmwgP+ w074BFVafyK7HVow3DmI9cblta/TJlGyzrrzZQUOMfgWryo/ytV3QSO4LGCOjIgE//ON y4YU8nRaeM8WPjrHcj+w/DLQmaPZRA4/pGfeq1cHiuuDZtTKY4aFjlJBTJy9HA6KwM5e ljnDSRdqZ+97KnXKBE8WLSYgiV+9MzV+Y1LJ398h2C3V8rmZk6rBvJLXY4ErQUlFO06N 4iRw== X-Gm-Message-State: ALQs6tA81dCikZDzsjTHi4W0cmvCTMQ+F3Ou5i7gdkpcAKl/v8FR5DH9 oDcbE+wD+zQ8YqF7pTgEaqykV2FZawY7466u0ptmhDdB X-Google-Smtp-Source: AIpwx4/ikmfLTqJzBGsNkq5pBAFXhBJC2sp+RJdmIVBzgiq/Gkv2g18gDQq1Lk1EjD+T2ZU1cw2x6VToGAOc73tVKdA= X-Received: by 10.107.14.136 with SMTP id 130mr598775ioo.170.1523943180269; Mon, 16 Apr 2018 22:33:00 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.187.67 with HTTP; Mon, 16 Apr 2018 22:32: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: Ard Biesheuvel Date: Tue, 17 Apr 2018 07:32:59 +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 05:33:02 -0000 Content-Type: text/plain; charset="UTF-8" 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.