From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f48.google.com (mail-wm1-f48.google.com [209.85.128.48]) by mx.groups.io with SMTP id smtpd.web10.12083.1613853959728183428 for ; Sat, 20 Feb 2021 12:46:00 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=osiNLN0y; spf=pass (domain: nuviainc.com, ip: 209.85.128.48, mailfrom: leif@nuviainc.com) Received: by mail-wm1-f48.google.com with SMTP id l17so10007237wmq.2 for ; Sat, 20 Feb 2021 12:45:59 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=bmlZ+7aKbyrFlsyXrb1GP0FnkSh0SbCjZQEgD27EyKI=; b=osiNLN0y2/8eRkmBpHYySRw67u3ySlCqLeHi/RxKVIP7OoN+PK2TymP3HcZy8zbKL8 eXvB/SolabiFZYbiGwynUbu6Oox2Mfnd5vUxEV2ezaM8vL1poVvWUW0IsUZT7v4rrRYJ lUoxA6nvWwXkMj9hbyfv1BsYM4/19X2ei0N274doefbLHF2KnXZnNbGCmrfmRVEQpgJ8 5NQoGIJLO6nIeRSxC2PRRksNNUI2V/o1HTGkoNruJEyYX4LR4fCKa4zRAfqp8IE0H1Mj A2wt0GclxmIb+OK/oVvy1D+d3U5K79aeebJwUC0HkrmK6WSXF5E6dpib2j1617IXzIVy 67yw== 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=bmlZ+7aKbyrFlsyXrb1GP0FnkSh0SbCjZQEgD27EyKI=; b=RwhlYBMb+rJPNqol0m7LxD2YWNtIsHwpnbqWOtEKo/vORL9wgR7+qfojMJh1y2lbOd dkLkZ1tGNpCmloDjkHfLRl7V8qZb48UlaRD8n4+mbUnGFY6Pst9rdnMhd67nMTmYDyvl m1Dorf1e2vuh4kDQQDHPfQVtPJb/EOMbvU54Fuol+Netm+l3sCaH4+gYK1LQPEubNTCH oI39OFtpMgW8oW23PmuLixNoERQ2NzR/8U1lXP5r9b9KMGwicXn98mNKp86dWACnP4jI Eyq66uvSnkQ7W0kgs8eKwxGzkPuWTkOZkHxUvaB/XkM35brxPm8AA12mjvCqSxY9JI+r yX+A== X-Gm-Message-State: AOAM5336VPBvMRI87insjBqp430zY8p90mxZ6iuePYSwAeDxkAItKYGF BqYxggMQr/05FpnHjf+i0nq0PQ== X-Google-Smtp-Source: ABdhPJwQ8UZf9hu+V4BBUeih1fJLOB9Bf+Ch4iS2lW809ieUl+tUxlq+YuGOD+Who1agcmphsquULA== X-Received: by 2002:a05:600c:19cb:: with SMTP id u11mr13760101wmq.5.1613853958203; Sat, 20 Feb 2021 12:45:58 -0800 (PST) Return-Path: Received: from vanye (cpc1-cmbg19-2-0-cust915.5-4.cable.virginm.net. [82.27.183.148]) by smtp.gmail.com with ESMTPSA id z14sm21093733wrt.89.2021.02.20.12.45.57 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 20 Feb 2021 12:45:57 -0800 (PST) Date: Sat, 20 Feb 2021 20:45:55 +0000 From: "Leif Lindholm" To: Rebecca Cran Cc: devel@edk2.groups.io, Ard Biesheuvel Subject: Re: [PATCH 1/1] ArmPkg: Fix several issues in OemMiscLib Message-ID: <20210220204555.GK1664@vanye> References: <20210219050800.5238-1-rebecca@nuviainc.com> MIME-Version: 1.0 In-Reply-To: <20210219050800.5238-1-rebecca@nuviainc.com> User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Thu, Feb 18, 2021 at 22:08:00 -0700, Rebecca Cran wrote: > Update OemMiscLib with the following changes: > > o Fixed ordering of return type and EFIAPI specifier. > o Renamed 'Offset' parameter in OemUpdateSmbiosInfo to 'Field'. > o Renamed OemGetProcessorMaxSockets to OemGetMaxProcessors. > o Renamed OemIsSocketPresent to OemIsProcessorPresent. > o Updated OemGetChassisType to return MISC_CHASSIS_TYPE instead of > EFI_STATUS, which matches other OemMiscLib functions. On the whole, this looks fine, but I see three separate patches here: - EFIAPI changes - Various non-functional renaming - Refactoring of OemGetChassisType Could you do that rework and submit a v2 please? / Leif > > Update Universal/Smbios to follow the changes to OemMiscLib. > > Signed-off-by: Rebecca Cran > --- > ArmPkg/Include/Library/OemMiscLib.h | 32 ++++++++-------- > ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c | 36 ++++++++---------- > ArmPkg/Universal/Smbios/ProcessorSubClassDxe/ProcessorSubClass.c | 40 ++++++++++---------- > ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type03/MiscChassisManufacturerFunction.c | 28 +------------- > 4 files changed, 52 insertions(+), 84 deletions(-) > > diff --git a/ArmPkg/Include/Library/OemMiscLib.h b/ArmPkg/Include/Library/OemMiscLib.h > index e70019d05f15..5f71b2ad48c7 100644 > --- a/ArmPkg/Include/Library/OemMiscLib.h > +++ b/ArmPkg/Include/Library/OemMiscLib.h > @@ -71,8 +71,8 @@ typedef enum > > @return CPU frequency in Hz > **/ > -EFIAPI > UINTN > +EFIAPI > OemGetCpuFreq ( > IN UINT8 ProcessorIndex > ); > @@ -87,8 +87,8 @@ OemGetCpuFreq ( > > @return TRUE on success, FALSE on failure. > **/ > -EFIAPI > BOOLEAN > +EFIAPI > OemGetProcessorInformation ( > IN UINTN ProcessorIndex, > IN OUT PROCESSOR_STATUS_DATA *ProcessorStatus, > @@ -106,8 +106,8 @@ OemGetProcessorInformation ( > > @return TRUE on success, FALSE on failure. > **/ > -EFIAPI > BOOLEAN > +EFIAPI > OemGetCacheInformation ( > IN UINT8 ProcessorIndex, > IN UINT8 CacheLevel, > @@ -116,26 +116,24 @@ OemGetCacheInformation ( > IN OUT SMBIOS_TABLE_TYPE7 *SmbiosCacheTable > ); > > -/** Gets the maximum number of sockets supported by the platform. > +/** Gets the maximum number of processors supported by the platform. > > - @return The maximum number of sockets. > + @return The maximum number of processors. > **/ > -EFIAPI > UINT8 > -OemGetProcessorMaxSockets ( > +EFIAPI > +OemGetMaxProcessors ( > VOID > ); > > /** Gets the type of chassis for the system. > > - @param ChassisType The type of the chassis. > - > - @retval EFI_SUCCESS The chassis type was fetched successfully. > + @retval The type of the chassis. > **/ > +MISC_CHASSIS_TYPE > EFIAPI > -EFI_STATUS > OemGetChassisType ( > - OUT UINT8 *ChassisType > + VOID > ); > > /** Returns whether the specified processor is present or not. > @@ -144,9 +142,9 @@ OemGetChassisType ( > > @return TRUE is the processor is present, FALSE otherwise. > **/ > -EFIAPI > BOOLEAN > -OemIsSocketPresent ( > +EFIAPI > +OemIsProcessorPresent ( > IN UINTN ProcessorIndex > ); > > @@ -154,14 +152,14 @@ OemIsSocketPresent ( > > @param mHiiHandle The HII handle. > @param TokenToUpdate The string to update. > - @param Offset The field to get information about. > + @param Field The field to get information about. > **/ > -EFIAPI > VOID > +EFIAPI > OemUpdateSmbiosInfo ( > IN EFI_HII_HANDLE HiiHandle, > IN EFI_STRING_ID TokenToUpdate, > - IN OEM_MISC_SMBIOS_HII_STRING_FIELD Offset > + IN OEM_MISC_SMBIOS_HII_STRING_FIELD Field > ); > > #endif // OEM_MISC_LIB_H_ > diff --git a/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c b/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c > index 73cebef2d1b9..6b233742feb0 100644 > --- a/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c > +++ b/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c > @@ -13,7 +13,6 @@ > #include > #include > #include > - > #include > > > @@ -23,8 +22,8 @@ > > @return CPU frequency in Hz > **/ > -EFIAPI > UINTN > +EFIAPI > OemGetCpuFreq ( > IN UINT8 ProcessorIndex > ) > @@ -43,8 +42,8 @@ OemGetCpuFreq ( > > @return TRUE on success, FALSE on failure. > **/ > -EFIAPI > BOOLEAN > +EFIAPI > OemGetProcessorInformation ( > IN UINTN ProcessorIndex, > IN OUT PROCESSOR_STATUS_DATA *ProcessorStatus, > @@ -66,8 +65,8 @@ OemGetProcessorInformation ( > > @return TRUE on success, FALSE on failure. > **/ > -EFIAPI > BOOLEAN > +EFIAPI > OemGetCacheInformation ( > IN UINT8 ProcessorIndex, > IN UINT8 CacheLevel, > @@ -80,13 +79,13 @@ OemGetCacheInformation ( > return TRUE; > } > > -/** Gets the maximum number of sockets supported by the platform. > +/** Gets the maximum number of processors supported by the platform. > > - @return The maximum number of sockets. > + @return The maximum number of processors. > **/ > -EFIAPI > UINT8 > -OemGetProcessorMaxSockets ( > +EFIAPI > +OemGetMaxProcessors ( > VOID > ) > { > @@ -96,19 +95,16 @@ OemGetProcessorMaxSockets ( > > /** Gets the type of chassis for the system. > > - @param ChassisType The type of the chassis. > - > - @retval EFI_SUCCESS The chassis type was fetched successfully. > + @retval The type of the chassis. > **/ > -EFI_STATUS > +MISC_CHASSIS_TYPE > EFIAPI > OemGetChassisType ( > - UINT8 *ChassisType > + VOID > ) > { > ASSERT (FALSE); > - *ChassisType = MiscChassisTypeUnknown; > - return EFI_SUCCESS; > + return MiscChassisTypeUnknown; > } > > /** Returns whether the specified processor is present or not. > @@ -117,9 +113,9 @@ OemGetChassisType ( > > @return TRUE is the processor is present, FALSE otherwise. > **/ > -EFIAPI > BOOLEAN > -OemIsSocketPresent ( > +EFIAPI > +OemIsProcessorPresent ( > IN UINTN ProcessorIndex > ) > { > @@ -131,14 +127,14 @@ OemIsSocketPresent ( > > @param mHiiHandle The HII handle. > @param TokenToUpdate The string to update. > - @param Offset The field to get information about. > + @param Field The field to get information about. > **/ > -EFIAPI > VOID > +EFIAPI > OemUpdateSmbiosInfo ( > IN EFI_HII_HANDLE mHiiHandle, > IN EFI_STRING_ID TokenToUpdate, > - IN OEM_MISC_SMBIOS_HII_STRING_FIELD Offset > + IN OEM_MISC_SMBIOS_HII_STRING_FIELD Field > ) > { > ASSERT (FALSE); > diff --git a/ArmPkg/Universal/Smbios/ProcessorSubClassDxe/ProcessorSubClass.c b/ArmPkg/Universal/Smbios/ProcessorSubClassDxe/ProcessorSubClass.c > index d03de12a820e..7296d786616c 100644 > --- a/ArmPkg/Universal/Smbios/ProcessorSubClassDxe/ProcessorSubClass.c > +++ b/ArmPkg/Universal/Smbios/ProcessorSubClassDxe/ProcessorSubClass.c > @@ -439,8 +439,8 @@ AddSmbiosCacheTypeTable ( > strings following the data fields. > > @param[out] Type4Record The Type 4 structure to allocate and initialize > - @param[in] ProcessorIndex The index of the processor socket > - @param[in] Populated Whether the specified processor socket is > + @param[in] ProcessorIndex The index of the processor > + @param[in] Populated Whether the specified processor is > populated. > > @retval EFI_SUCCESS The Type 4 structure was successfully > @@ -460,7 +460,7 @@ AllocateType4AndSetProcessorInformationStrings ( > EFI_STRING_ID SerialNumber; > EFI_STRING_ID AssetTag; > EFI_STRING_ID PartNumber; > - EFI_STRING ProcessorSocketStr; > + EFI_STRING ProcessorStr; > EFI_STRING ProcessorManuStr; > EFI_STRING ProcessorVersionStr; > EFI_STRING SerialNumberStr; > @@ -468,7 +468,7 @@ AllocateType4AndSetProcessorInformationStrings ( > EFI_STRING PartNumberStr; > CHAR8 *OptionalStrStart; > CHAR8 *StrStart; > - UINTN ProcessorSocketStrLen; > + UINTN ProcessorStrLen; > UINTN ProcessorManuStrLen; > UINTN ProcessorVersionStrLen; > UINTN SerialNumberStrLen; > @@ -497,14 +497,14 @@ AllocateType4AndSetProcessorInformationStrings ( > SET_HII_STRING_IF_PCD_NOT_EMPTY (PcdProcessorAssetTag, AssetTag); > SET_HII_STRING_IF_PCD_NOT_EMPTY (PcdProcessorPartNumber, PartNumber); > > - // Processor Socket Designation > + // Processor Designation > StringBufferSize = sizeof (CHAR16) * SMBIOS_STRING_MAX_LENGTH; > - ProcessorSocketStr = AllocateZeroPool (StringBufferSize); > - if (ProcessorSocketStr == NULL) { > + ProcessorStr = AllocateZeroPool (StringBufferSize); > + if (ProcessorStr == NULL) { > return EFI_OUT_OF_RESOURCES; > } > > - ProcessorSocketStrLen = UnicodeSPrint (ProcessorSocketStr, StringBufferSize, > + ProcessorStrLen = UnicodeSPrint (ProcessorStr, StringBufferSize, > L"CPU%02d", ProcessorIndex + 1); > > // Processor Manufacture > @@ -528,7 +528,7 @@ AllocateType4AndSetProcessorInformationStrings ( > PartNumberStrLen = StrLen (PartNumberStr); > > TotalSize = sizeof (SMBIOS_TABLE_TYPE4) + > - ProcessorSocketStrLen + 1 + > + ProcessorStrLen + 1 + > ProcessorManuStrLen + 1 + > ProcessorVersionStrLen + 1 + > SerialNumberStrLen + 1 + > @@ -545,12 +545,12 @@ AllocateType4AndSetProcessorInformationStrings ( > > OptionalStrStart = (CHAR8 *)(*Type4Record + 1); > UnicodeStrToAsciiStrS ( > - ProcessorSocketStr, > + ProcessorStr, > OptionalStrStart, > - ProcessorSocketStrLen + 1 > + ProcessorStrLen + 1 > ); > > - StrStart = OptionalStrStart + ProcessorSocketStrLen + 1; > + StrStart = OptionalStrStart + ProcessorStrLen + 1; > UnicodeStrToAsciiStrS ( > ProcessorManuStr, > StrStart, > @@ -586,7 +586,7 @@ AllocateType4AndSetProcessorInformationStrings ( > ); > > Exit: > - FreePool (ProcessorSocketStr); > + FreePool (ProcessorStr); > FreePool (ProcessorManuStr); > FreePool (ProcessorVersionStr); > FreePool (SerialNumberStr); > @@ -618,7 +618,7 @@ AddSmbiosProcessorTypeTable ( > UINT64 *ProcessorId; > PROCESSOR_CHARACTERISTIC_FLAGS ProcessorCharacteristics; > OEM_MISC_PROCESSOR_DATA MiscProcessorData; > - BOOLEAN SocketPopulated; > + BOOLEAN ProcessorPopulated; > > Type4Record = NULL; > > @@ -632,12 +632,12 @@ AddSmbiosProcessorTypeTable ( > L2CacheHandle = 0xFFFF; > L3CacheHandle = 0xFFFF; > > - SocketPopulated = OemIsSocketPresent(ProcessorIndex); > + ProcessorPopulated = OemIsProcessorPresent (ProcessorIndex); > > Status = AllocateType4AndSetProcessorInformationStrings ( > &Type4Record, > ProcessorIndex, > - SocketPopulated > + ProcessorPopulated > ); > if (EFI_ERROR (Status)) { > return Status; > @@ -649,7 +649,7 @@ AddSmbiosProcessorTypeTable ( > &Type4Record->ProcessorCharacteristics, > &MiscProcessorData); > > - if (SocketPopulated) { > + if (ProcessorPopulated) { > AddSmbiosCacheTypeTable (ProcessorIndex, &L1CacheHandle, > &L2CacheHandle, &L3CacheHandle); > } > @@ -713,7 +713,7 @@ ProcessorSubClassEntryPoint( > ) > { > EFI_STATUS Status; > - UINT32 SocketIndex; > + UINT32 ProcessorIndex; > > // > // Locate dependent protocols > @@ -740,8 +740,8 @@ ProcessorSubClassEntryPoint( > // > // Add SMBIOS tables for populated sockets. > // > - for (SocketIndex = 0; SocketIndex < OemGetProcessorMaxSockets(); SocketIndex++) { > - Status = AddSmbiosProcessorTypeTable (SocketIndex); > + for (ProcessorIndex = 0; ProcessorIndex < OemGetMaxProcessors (); ProcessorIndex++) { > + Status = AddSmbiosProcessorTypeTable (ProcessorIndex); > if (EFI_ERROR (Status)) { > DEBUG ((DEBUG_ERROR, "Add Processor Type Table Failed! %r.\n", Status)); > return Status; > diff --git a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type03/MiscChassisManufacturerFunction.c b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type03/MiscChassisManufacturerFunction.c > index e6adbceba2d5..fc4dba319aad 100644 > --- a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type03/MiscChassisManufacturerFunction.c > +++ b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type03/MiscChassisManufacturerFunction.c > @@ -23,27 +23,6 @@ > > #include "SmbiosMisc.h" > > -/** > - * Returns the chassis type in SMBIOS format. > - * > - * @return Chassis type > -**/ > -UINT8 > -GetChassisType ( > - VOID > - ) > -{ > - EFI_STATUS Status; > - UINT8 ChassisType; > - > - Status = OemGetChassisType (&ChassisType); > - if (EFI_ERROR (Status)) { > - return 0; > - } > - > - return ChassisType; > -} > - > /** > This function makes boot time changes to the contents of the > MiscChassisManufacturer (Type 3) record. > @@ -80,8 +59,6 @@ SMBIOS_MISC_TABLE_FUNCTION(MiscChassisManufacturer) > CONTAINED_ELEMENT ContainedElements; > UINT8 ExtendLength; > > - UINT8 ChassisType; > - > ExtendLength = 0; > > // > @@ -165,10 +142,7 @@ SMBIOS_MISC_TABLE_FUNCTION(MiscChassisManufacturer) > > SmbiosRecord->Hdr.Length = sizeof (SMBIOS_TABLE_TYPE3) + ExtendLength + 1; > > - ChassisType = GetChassisType (); > - if (ChassisType != 0) { > - SmbiosRecord->Type = ChassisType; > - } > + SmbiosRecord->Type = OemGetChassisType (); > > //ContainedElements > ASSERT (ContainedElementCount < 2); > -- > 2.26.2 >