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

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

On 17 April 2018 at 08:04, Marcin Wojtas <mw@semihalf.com> wrote:
> Hi Ard,
>
> 2018-04-17 7:32 GMT+02:00 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.
>
> 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