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

[Marcin Wojtas <mw@semihalf.com>]

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.

> 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?

Best regards,
Marcin

> Thanks
> Laszlo
>
>>> +ErrorInstallNvVarStoreFormatted:
>>>    gBS->UninstallMultipleProtocolInterfaces (&mFvbDevice->Handle,
>>>           &gEfiDevicePathProtocolGuid,
>>>           &gEfiFirmwareVolumeBlockProtocolGuid,
>>> diff --git a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf
>>> index 117fe8b..fd3f2f7 100644
>>> --- a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf
>>> +++ b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf
>>> @@ -63,6 +63,7 @@
>>>    UefiRuntimeServicesTableLib
>>>
>>>  [Guids]
>>> +  gEdkiiNvVarStoreFormattedGuid
>>>    gEfiAuthenticatedVariableGuid
>>>    gEfiEventVirtualAddressChangeGuid
>>>    gEfiSystemNvDataFvGuid
>>> @@ -84,8 +85,4 @@
>>>    gMarvellTokenSpaceGuid.PcdSpiMemoryBase
>>>
>>>  [Depex]
>>> -  #
>>> -  # MvFvbDxe must be loaded before VariableRuntimeDxe in case empty
>>> -  # flash needs populating with default values.
>>> -  #
>>> -  BEFORE gVariableRuntimeDxeFileGuid
>>> +  gEfiCpuArchProtocolGuid
>>> --
>>> 2.7.4
>>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
>>
>