Re: [platforms PATCH 1/2] Marvell/Drivers: MvFvbDxe: Adjust to new dependencies

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 17 April 2018 at 07:15, Marcin Wojtas <mw@semihalf.com> wrote:
> Hi Laszlo,
>
> 2018-04-16 21:41 GMT+02:00 Laszlo Ersek <lersek@redhat.com>:
>> On 04/16/18 07:40, Ard Biesheuvel wrote:
>>> (+ Laszlo)
>>>
>>> On 16 April 2018 at 07:09, Marcin Wojtas <mw@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.