From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:400c:c09::231; helo=mail-wm0-x231.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wm0-x231.google.com (mail-wm0-x231.google.com [IPv6:2a00:1450:400c:c09::231]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 8C13820359E8B for ; Sun, 26 Nov 2017 06:33:08 -0800 (PST) Received: by mail-wm0-x231.google.com with SMTP id v186so30071692wma.2 for ; Sun, 26 Nov 2017 06:37:29 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=NsD1TP1NHt5or27zWM1YwHFAkmpoCnhWQgB1ihvWDFs=; b=Zpf5LXgwmaPcqCxTECcSrgNiu7L8OeopMHr87Id3TDWbKJoA7m22j7T81OIBC+SuTU sVPvjtTfh+GYIanb7/SZX1S7cTDi1Xqtax/10jF2ZWvetZ7vEtwLPk79tRPOx2d8H3Pt ROwEQ5hDTL/GxVksO6oJN0mEl1BjpK/wInzX0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=NsD1TP1NHt5or27zWM1YwHFAkmpoCnhWQgB1ihvWDFs=; b=CJJnYuos3sSaHooKdKdoYk3qmC+xrEgYbGSnT6qmVoSZ/ah3v1t8TyHPEs7oopsmMK tP+PwUbbB+YKiP2QKW+ioQXHeWlPCCmz8VIpsIjRn7YLhEwqUIhD1+fG91ud06gJx0Je awUuf01KWSRZmuFb1kQWBwbQiY7X9xILrYZxpuORzE8B9Spj9VpWuF/Hk+n1KY31Sswt Qjy2vXxrdaCB64XN7QrpvPfYOMWZdSWiL69tW4b8c9tMu9gV4gbzmNMUNj8dt+5H/P82 8jUw737OVy022saB3EFTWbKcnmWYgnkRpINhsjKZgu+Vrv/2AYkr5Bsb8NdJ+B2bIaTs 0EJg== X-Gm-Message-State: AJaThX6qqiOacjiB3GJGA6zyFS7Ku3Rm64sQl14AsxxjObZLZtffwF+R n/FM21AHMugAFyCNgWR0TBwnyA== X-Google-Smtp-Source: AGs4zMYcg/r7z5Qs/fqeouJFOBFEl8VilIygZ0slQIH+LQc5S+lN30OAO2w5mSGJnsWJqUS2zpLQeQ== X-Received: by 10.28.93.21 with SMTP id r21mr6280860wmb.152.1511707047314; Sun, 26 Nov 2017 06:37:27 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id f19sm29580704wrh.64.2017.11.26.06.37.25 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Sun, 26 Nov 2017 06:37:25 -0800 (PST) Date: Sun, 26 Nov 2017 14:37:24 +0000 From: Leif Lindholm To: Marcin Wojtas Cc: edk2-devel-01 , Ard Biesheuvel , nadavh@marvell.com, Neta Zur Hershkovits , Kostya Porotchkin , Hua Jing , semihalf-dabros-jan Message-ID: <20171126143723.dfo5gk5t3vaqsmr3@bivouac.eciton.net> References: <1511246781-7073-1-git-send-email-mw@semihalf.com> <1511246781-7073-2-git-send-email-mw@semihalf.com> <20171125140911.dms6f5qc26oxvfjg@bivouac.eciton.net> MIME-Version: 1.0 In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [platforms: PATCH v2 1/4] Platform/Marvell: Introduce MvFvbDxe variable support driver X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 26 Nov 2017 14:33:08 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 : > > > + // 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