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

[Marcin Wojtas <mw@semihalf.com>]

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?

Thanks,
Marcin