From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id B087281D9C for ; Fri, 28 Oct 2016 08:23:54 -0700 (PDT) Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B93533B731; Fri, 28 Oct 2016 15:23:54 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-38.phx2.redhat.com [10.3.116.38]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u9SFNrWh006846; Fri, 28 Oct 2016 11:23:53 -0400 To: Ard Biesheuvel , edk2-devel@ml01.01.org, leif.lindholm@linaro.org References: <1477667597-26331-1-git-send-email-ard.biesheuvel@linaro.org> From: Laszlo Ersek Message-ID: <22baf765-a12b-0d6a-593a-c05a69f01137@redhat.com> Date: Fri, 28 Oct 2016 17:23:52 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <1477667597-26331-1-git-send-email-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.27 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Fri, 28 Oct 2016 15:23:54 +0000 (UTC) Subject: Re: [PATCH] ArmPlatformPkg/ArmVExpressFastBootDxe: clean up code and comments X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 28 Oct 2016 15:23:54 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 10/28/16 17:13, Ard Biesheuvel wrote: > This module contains an implementation of the Android Fastboot Platform > protocol. So follow common Tianocore practice, and duplicate the per-method > comment blocks from the interface definition into the implementation. > > In addition, let's make all methods and the procotol struct STATIC, given > that they are never referenced via external linkage. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel > --- > ArmPlatformPkg/ArmVExpressPkg/ArmVExpressFastBootDxe/ArmVExpressFastBoot.c | 79 ++++++++++++++++++-- > 1 file changed, 74 insertions(+), 5 deletions(-) > > diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressFastBootDxe/ArmVExpressFastBoot.c b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressFastBootDxe/ArmVExpressFastBoot.c > index 64b25f8a8c45..a01bf3c671ad 100644 > --- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressFastBootDxe/ArmVExpressFastBoot.c > +++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressFastBootDxe/ArmVExpressFastBoot.c > @@ -1,6 +1,7 @@ > /** @file > > Copyright (c) 2014, ARM Ltd. All rights reserved.
> + Copyright (c) 2016, Linaro Ltd. All rights reserved.
> > This program and the accompanying materials > are licensed and made available under the terms and conditions of the BSD License > @@ -135,11 +136,13 @@ ReadPartitionEntries ( > > > /* > - Initialise: Open the Android NVM device and find the partitions on it. Save them in > - a list along with the "PartitionName" fields for their GPT entries. > - We will use these partition names as the key in > - ArmFastbootPlatformFlashPartition. > + Do any initialisation that needs to be done in order to be able to respond to > + commands. > + > + @retval EFI_SUCCESS Initialised successfully. > + @retval !EFI_SUCCESS Error in initialisation. > */ > +STATIC > EFI_STATUS > ArmFastbootPlatformInit ( > VOID > @@ -164,6 +167,7 @@ ArmFastbootPlatformInit ( > // > // Get EFI_HANDLES for all the partitions on the block devices pointed to by > // PcdFastbootFlashDevicePath, also saving their GPT partition labels. > + // We will use these labels as the key in ArmFastbootPlatformFlashPartition. > // There's no way to find all of a device's children, so we get every handle > // in the system supporting EFI_BLOCK_IO_PROTOCOL and then filter out ones > // that don't represent partitions on the flash device. > @@ -296,6 +300,11 @@ Exit: > > } > > +/* > + To be called when Fastboot is finished and we aren't rebooting or booting an > + image. Undo initialisation, free resrouces. > +*/ > +STATIC > VOID > ArmFastbootPlatformUnInit ( > VOID > @@ -304,6 +313,18 @@ ArmFastbootPlatformUnInit ( > FreePartitionList (); > } > > +/* > + Flash the partition named (according to a platform-specific scheme) > + PartitionName, with the image pointed to by Buffer, whose size is BufferSize. > + > + @param[in] PartitionName Null-terminated name of partition to write. > + @param[in] BufferSize Size of Buffer in byets. > + @param[in] Buffer Data to write to partition. > + > + @retval EFI_NOT_FOUND No such partition. > + @retval EFI_DEVICE_ERROR Flashing failed. > +*/ > +STATIC > EFI_STATUS > ArmFastbootPlatformFlashPartition ( > IN CHAR8 *PartitionName, > @@ -382,6 +403,15 @@ ArmFastbootPlatformFlashPartition ( > return Status; > } > > +/* > + Erase the partition named PartitionName. > + > + @param[in] PartitionName Null-terminated name of partition to erase. > + > + @retval EFI_NOT_FOUND No such partition. > + @retval EFI_DEVICE_ERROR Erasing failed. > +*/ > +STATIC > EFI_STATUS > ArmFastbootPlatformErasePartition ( > IN CHAR8 *Partition > @@ -390,6 +420,25 @@ ArmFastbootPlatformErasePartition ( > return EFI_SUCCESS; > } > > +/* > + If the variable referred to by Name exists, copy it (as a null-terminated > + string) into Value. If it doesn't exist, put the Empty string in Value. > + > + Variable names and values may not be larger than 60 bytes, excluding the > + terminal null character. This is a limitation of the Fastboot protocol. > + > + The Fastboot application will handle platform-nonspecific variables > + (Currently "version" is the only one of these.) > + > + @param[in] Name Null-terminated name of Fastboot variable to retrieve. > + @param[out] Value Caller-allocated buffer for null-terminated value of > + variable. > + > + @retval EFI_SUCCESS The variable was retrieved, or it doesn't exist. > + @retval EFI_DEVICE_ERROR There was an error looking up the variable. This > + does _not_ include the variable not existing. > +*/ > +STATIC > EFI_STATUS > ArmFastbootPlatformGetVar ( > IN CHAR8 *Name, > @@ -404,6 +453,26 @@ ArmFastbootPlatformGetVar ( > return EFI_SUCCESS; > } > > +/* > + React to an OEM-specific command. > + > + Future versions of this function might want to allow the platform to do some > + extra communication with the host. A way to do this would be to add a function > + to the FASTBOOT_TRANSPORT_PROTOCOL that allows the implementation of > + DoOemCommand to replace the ReceiveEvent with its own, and to restore the old > + one when it's finished. > + > + However at the moment although the specification allows it, the AOSP fastboot > + host application doesn't handle receiving any data from the client, and it > + doesn't support a data phase for OEM commands. > + > + @param[in] Command Null-terminated command string. > + > + @retval EFI_SUCCESS The command executed successfully. > + @retval EFI_NOT_FOUND The command wasn't recognised. > + @retval EFI_DEVICE_ERROR There was an error executing the command. > +*/ > +STATIC > EFI_STATUS > ArmFastbootPlatformOemCommand ( > IN CHAR8 *Command > @@ -425,7 +494,7 @@ ArmFastbootPlatformOemCommand ( > } > } > > -FASTBOOT_PLATFORM_PROTOCOL mPlatformProtocol = { > +STATIC FASTBOOT_PLATFORM_PROTOCOL mPlatformProtocol = { > ArmFastbootPlatformInit, > ArmFastbootPlatformUnInit, > ArmFastbootPlatformFlashPartition, > Acked-by: Laszlo Ersek