Re: [platforms: PATCH v2 1/4] Platform/Marvell: Introduce MvFvbDxe variable support driver

[Leif Lindholm <leif.lindholm@linaro.org>]

On Sun, Nov 26, 2017 at 02:38:18PM +0100, Marcin Wojtas wrote:
> Hi Leif,
> 
> 2017-11-25 15:09 GMT+01:00 Leif Lindholm <leif.lindholm@linaro.org>:
> > > +  // Check the Variable Store Guid
> > > +  if (!CompareGuid (&VariableStoreHeader->Signature, &gEfiVariableGuid) &&
> > > +      !CompareGuid (&VariableStoreHeader->Signature,
> > > +         &gEfiAuthenticatedVariableGuid)) {
> >
> > Still spurious indentation. (If it's meant to indicate continuation,
> > that's fine, but that's two spaces, not three. Although I would find
> >       !CompareGuid (&VariableStoreHeader->Signature,
> >                     &gEfiAuthenticatedVariableGuid)) {
> > more clear.
> 
> It's two spaces from the beginning of function name. Anyway, I'll
> align &gEfiAuthenticatedVariableGuid to &VariableStoreHeader,

Thanks.

> > > +/**
> > > + The SetAttributes() function sets configurable firmware volume attributes
> > > + and returns the new settings of the firmware volume.
> > > +
> > > +
> > > + @param This                     EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL instance.
> > > +
> > > + @param Attributes               On input, Attributes is a pointer to
> > > +                                 EFI_FVB_ATTRIBUTES_2 that contains the desired
> > > +                                 firmware volume settings.
> > > +                                 On successful return, it contains the new
> > > +                                 settings of the firmware volume.
> > > +
> > > + @retval EFI_SUCCESS             The firmware volume attributes were returned.
> > > +
> > > + @retval EFI_INVALID_PARAMETER   The attributes requested are in conflict with
> > > +                                 the capabilities as declared in the firmware
> > > +                                 volume header.
> > > +
> > > + **/
> > > +EFI_STATUS
> > > +EFIAPI
> > > +MvFvbSetAttributes (
> > > +  IN CONST  EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL  *This,
> > > +  IN OUT    EFI_FVB_ATTRIBUTES_2                 *Attributes
> > > +  )
> > > +{
> > > +  DEBUG ((DEBUG_BLKIO,
> > > +    "%a: Operation not supported, keep default set of attributes\n",
> > > +    __FUNCTION__));
> > > +
> > > +  return MvFvbGetAttributes (This, Attributes);
> >
> > I'm still not completely thrilled about this.
> > Sure, the return value is always SUCCESS now.
> >
> > But the implicit meaning of the specification is that
> > *this*support*is*not*optional*.
> 
> Ok, I'll change it then. I only referred to existing edk2-platforms
> implementations (AMD - 'return EFI_SUCCESS', Hisilicon/Socionext -
> 'return EFI_UNSUPPORTED' - all with doing nothing with the actual
> attributes list). Same with ArmPlatformPkg, only Intel seems to
> implement this.

Much appreciated.

Well, this is basically a case of reviewer Dunning-Kruger - when I
have previously reviewed such patches, I've not had sufficiently
internalised knowledge of the underlying protocols to go "hang on...".

I would appreciate if you could raise bugs on the items you have
discovered in ofther drivers in https://bugzilla.tianocore.org/.

/
    Leif