Hi Ard, 2018-04-17 7:32 GMT+02:00 Ard Biesheuvel <ard.biesheu...@linaro.org>: > On 17 April 2018 at 07:15, Marcin Wojtas <m...@semihalf.com> wrote: >> Hi Laszlo, >> >> 2018-04-16 21:41 GMT+02:00 Laszlo Ersek <ler...@redhat.com>: >>> On 04/16/18 07:40, Ard Biesheuvel wrote: >>>> (+ Laszlo) >>>> >>>> On 16 April 2018 at 07:09, Marcin Wojtas <m...@semihalf.com> 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 _______________________________________________ edk2-devel mailing list email@example.com https://lists.01.org/mailman/listinfo/edk2-devel