[edk2-devel] [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib Library
Ethan Tsao
ethan.tsao at intel.com
Fri Nov 8 07:17:11 UTC 2019
Hi Ray,
I do agree with you. I will remove all debug message since all error scenario can be cover by return status.
Caller can understanding through return status.
Best Regards,
Ethan
> -----Original Message-----
> From: Ni, Ray <ray.ni at intel.com>
> Sent: Friday, November 8, 2019 2:25 PM
> To: Tsao, Ethan <ethan.tsao at intel.com>; devel at edk2.groups.io
> Cc: Chaganty, Rangasai V <rangasai.v.chaganty at intel.com>
> Subject: RE: [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib Library
>
> Ethan,
> What if all of the debug message are removed?
> Library especially very fundamental like ConfigBlock is better to be quite.
> It's the ConfigBlock API designer's responsibility to ensure the error is returned
> properly to caller, otherwise it's the API's bug.
> When the API can return correct status out, it's caller's responsibility to check
> the status and do proper handling of the errors.
>
> With the debug message added in this fundamental library, it may give callers
> wrong impression that the library itself can handle the error and callers don't
> need to check the error status.
>
> If we search code in MdePkg, quite few debug messages are printed out from
> library code.
>
> Thanks,
> Ray
>
> > -----Original Message-----
> > From: Tsao, Ethan <ethan.tsao at intel.com>
> > Sent: Thursday, November 7, 2019 4:14 PM
> > To: Ni, Ray <ray.ni at intel.com>; devel at edk2.groups.io
> > Cc: Chaganty, Rangasai V <rangasai.v.chaganty at intel.com>
> > Subject: RE: [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib
> > Library
> >
> > Hi Ray,
> > Thanks your good input. I just review all debug message and most print
> > is for error report purpose, such as allocate memory failure,...etc.
> > From my opinion, this kind debug message is useful for BIOS when
> > unexpected error happen. In normal case, it will not be print.
> >
> > Best Regards,
> > Ethan
> >
> > > -----Original Message-----
> > > From: Ni, Ray <ray.ni at intel.com>
> > > Sent: Thursday, November 7, 2019 3:15 PM
> > > To: Tsao, Ethan <ethan.tsao at intel.com>; edk2-devel at lists.01.org
> > > Cc: Chaganty, Rangasai V <rangasai.v.chaganty at intel.com>
> > > Subject: RE: [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib
> > > Library
> > >
> > > Given the patch only moves the code from one place to another, I am
> > > ok
> > with
> > > that.
> > >
> > > By the way, is it still valuable to have so many debug messages
> > > everywhere
> > in
> > > this library?
> > > If no, can we remove them or at least some of them?
> > >
> > > Debug messages are valuable I agree. But we also need to think about
> > producing
> > > helpful debug messages, not treated by platform developers as noise
> > > : )
> > >
> > >
> > > > -----Original Message-----
> > > > From: Tsao, Ethan <ethan.tsao at intel.com>
> > > > Sent: Thursday, November 7, 2019 11:31 AM
> > > > To: edk2-devel at lists.01.org
> > > > Cc: Chaganty, Rangasai V <rangasai.v.chaganty at intel.com>; Ni, Ray
> > > > <ray.ni at intel.com>
> > > > Subject: [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib
> > > > Library
> > > >
> > > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2318
> > > >
> > > > Establish one copy of Config blocks library class and instance in
> > > > IntelSiliconPkg and remove copies from other silicon packages ,
> > > > like KabyLakeSiliconPkg, CoffelakeSiliconPkg.
> > > >
> > > > Signed-off-by: Ethan Tsao <ethan.tsao at intel.com>
> > > > Cc: Sai Chaganty <rangasai.v.chaganty at intel.com>
> > > > Cc: Ray Ni <ray.ni at intel.com>
> > > > ---
> > > >
> > > >
> > Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/BaseConf
> > igBlo
> > > > ckLib.c | 146 -------------------------------------------------------------
> > ----
> > > -
> > > > ------------------------------------------------------------------
> > > > ----
> > > > ----------
> > > >
> > > >
> > Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/BaseConf
> > igBlo
> > > > ckLib.inf | 29 -----------------------------
> > > > Silicon/Intel/{KabylakeSiliconPkg =>
> > > > IntelSiliconPkg}/Library/BaseConfigBlockLib/BaseConfigBlockLib.c | 0
> > > > Silicon/Intel/{KabylakeSiliconPkg =>
> > > > IntelSiliconPkg}/Library/BaseConfigBlockLib/BaseConfigBlockLib.inf | 0
> > > > 4 files changed, 175 deletions(-)
> > > >
> > > > diff --git
> > > > a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/Ba
> > > > seCo
> > > > nfigB
> > > > lockLib.c
> > > > b/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/Ba
> > > > seCo
> > > > nfig
> > > > BlockLib.c
> > > > deleted file mode 100644
> > > > index 369dab97ee..0000000000
> > > > ---
> > > > a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/Ba
> > > > seCo
> > > > nfigB
> > > > lockLib.c
> > > > +++ /dev/null
> > > > @@ -1,146 +0,0 @@
> > > > -/** @file
> > > > - Library functions for Config Block management.
> > > > -
> > > > - Copyright (c) 2019 Intel Corporation. All rights reserved. <BR>
> > > > -
> > > > - SPDX-License-Identifier: BSD-2-Clause-Patent -**/
> > > > -
> > > > -#include <ConfigBlock.h>
> > > > -#include <Library/ConfigBlockLib.h> -#include
> > > > <Library/BaseMemoryLib.h> -#include
> > > > <Library/MemoryAllocationLib.h> -#include <Library/DebugLib.h>
> > > > -
> > > > -/**
> > > > - Create config block table
> > > > -
> > > > - @param[in] TotalSize - Max size to be allocated for the
> > Config
> > > > Block Table
> > > > - @param[out] ConfigBlockTableAddress - On return, points to a
> > pointer
> > > > to the beginning of Config Block Table Address
> > > > -
> > > > - @retval EFI_INVALID_PARAMETER - Invalid Parameter
> > > > - @retval EFI_OUT_OF_RESOURCES - Out of resources
> > > > - @retval EFI_SUCCESS - Successfully created Config Block Table at
> > > > ConfigBlockTableAddress
> > > > -**/
> > > > -EFI_STATUS
> > > > -EFIAPI
> > > > -CreateConfigBlockTable (
> > > > - IN UINT16 TotalSize,
> > > > - OUT VOID **ConfigBlockTableAddress
> > > > - )
> > > > -{
> > > > - CONFIG_BLOCK_TABLE_HEADER *ConfigBlkTblAddrPtr;
> > > > - UINT32 ConfigBlkTblHdrSize;
> > > > -
> > > > - ConfigBlkTblHdrSize = (UINT32)(sizeof
> > (CONFIG_BLOCK_TABLE_HEADER));
> > > > -
> > > > - if (TotalSize <= (ConfigBlkTblHdrSize + sizeof
> > (CONFIG_BLOCK_HEADER))) {
> > > > - DEBUG ((DEBUG_ERROR, "Invalid Parameter\n"));
> > > > - return EFI_INVALID_PARAMETER;
> > > > - }
> > > > -
> > > > - ConfigBlkTblAddrPtr = (CONFIG_BLOCK_TABLE_HEADER
> > *)AllocateZeroPool
> > > > (TotalSize);
> > > > - if (ConfigBlkTblAddrPtr == NULL) {
> > > > - DEBUG ((DEBUG_ERROR, "Could not allocate memory.\n"));
> > > > - return EFI_OUT_OF_RESOURCES;
> > > > - }
> > > > - ConfigBlkTblAddrPtr->NumberOfBlocks = 0;
> > > > - ConfigBlkTblAddrPtr->Header.GuidHob.Header.HobLength =
> > > > TotalSize;
> > > > - ConfigBlkTblAddrPtr->AvailableSize = TotalSize -
> > > > ConfigBlkTblHdrSize;
> > > > -
> > > > - *ConfigBlockTableAddress = (VOID *)ConfigBlkTblAddrPtr;
> > > > -
> > > > - return EFI_SUCCESS;
> > > > -}
> > > > -
> > > > -/**
> > > > - Add config block into config block table structure
> > > > -
> > > > - @param[in] ConfigBlockTableAddress - A pointer to the beginning
> > of
> > > > Config Block Table Address
> > > > - @param[out] ConfigBlockAddress - On return, points to a pointer
> > to
> > > > the beginning of Config Block Address
> > > > -
> > > > - @retval EFI_OUT_OF_RESOURCES - Config Block Table is full and
> > > > cannot add new Config Block or
> > > > - Config Block Offset Table is full and cannot add new
> > Config
> > > > Block.
> > > > - @retval EFI_SUCCESS - Successfully added Config Block
> > > > -**/
> > > > -EFI_STATUS
> > > > -EFIAPI
> > > > -AddConfigBlock (
> > > > - IN VOID *ConfigBlockTableAddress,
> > > > - OUT VOID **ConfigBlockAddress
> > > > - )
> > > > -{
> > > > - CONFIG_BLOCK *TempConfigBlk;
> > > > - CONFIG_BLOCK_TABLE_HEADER *ConfigBlkTblAddrPtr;
> > > > - CONFIG_BLOCK *ConfigBlkAddrPtr;
> > > > - UINT16 ConfigBlkSize;
> > > > -
> > > > - ConfigBlkTblAddrPtr = (CONFIG_BLOCK_TABLE_HEADER
> > > > *)ConfigBlockTableAddress;
> > > > - ConfigBlkAddrPtr = (CONFIG_BLOCK *)(*ConfigBlockAddress);
> > > > - ConfigBlkSize =
> > > > ConfigBlkAddrPtr->Header.GuidHob.Header.HobLength;
> > > > - DEBUG ((DEBUG_INFO, "Config Block GUID: %g / Config Block Size:
> > > > 0x%x bytes\n", &(ConfigBlkAddrPtr->Header.GuidHob.Name),
> > > > ConfigBlkSize));
> > > > - if ((ConfigBlkSize % 4) != 0) {
> > > > - DEBUG ((DEBUG_ERROR, "Config Block must be multiples of 4
> > bytes\n"));
> > > > - return EFI_INVALID_PARAMETER;
> > > > - }
> > > > - if (ConfigBlkTblAddrPtr->AvailableSize < ConfigBlkSize) {
> > > > - DEBUG ((DEBUG_ERROR, "Config Block Table is full and cannot add
> > new
> > > > Config Block.\n"));
> > > > - DEBUG ((DEBUG_ERROR, "Available Config Block Size: 0x%x bytes /
> > > > Requested Config Block Size: 0x%x bytes\n", ConfigBlkTblAddrPtr-
> > > > >AvailableSize, ConfigBlkSize));
> > > > - return EFI_OUT_OF_RESOURCES;
> > > > - }
> > > > -
> > > > - TempConfigBlk = (CONFIG_BLOCK *)((UINTN)ConfigBlkTblAddrPtr +
> > > > (UINTN)(ConfigBlkTblAddrPtr->Header.GuidHob.Header.HobLength -
> > > > ConfigBlkTblAddrPtr->AvailableSize));
> > > > - CopyMem (&TempConfigBlk->Header, &ConfigBlkAddrPtr->Header,
> > > > sizeof(CONFIG_BLOCK_HEADER));
> > > > -
> > > > - ConfigBlkTblAddrPtr->NumberOfBlocks++;
> > > > - ConfigBlkTblAddrPtr->AvailableSize =
> > > > ConfigBlkTblAddrPtr->AvailableSize - ConfigBlkSize;
> > > > -
> > > > - *ConfigBlockAddress = (VOID *) TempConfigBlk;
> > > > - DEBUG ((DEBUG_INFO, "Config Block Address: 0x%x / Available
> > > > Config Block Size: 0x%x bytes\n",
> > > > (UINT32)(UINTN)*ConfigBlockAddress,
> > > > ConfigBlkTblAddrPtr->AvailableSize));
> > > > - return EFI_SUCCESS;
> > > > -}
> > > > -
> > > > -/**
> > > > - Retrieve a specific Config Block data by GUID
> > > > -
> > > > - @param[in] ConfigBlockTableAddress - A pointer to the beginning
> > of
> > > > Config Block Table Address
> > > > - @param[in] ConfigBlockGuid - A pointer to the GUID uses to
> > > > search specific Config Block
> > > > - @param[out] ConfigBlockAddress - On return, points to a
> > pointer to
> > > > the beginning of Config Block Address
> > > > -
> > > > - @retval EFI_NOT_FOUND - Could not find the Config Block
> > > > - @retval EFI_SUCCESS - Config Block found and return
> > > > -**/
> > > > -EFI_STATUS
> > > > -EFIAPI
> > > > -GetConfigBlock (
> > > > - IN VOID *ConfigBlockTableAddress,
> > > > - IN EFI_GUID *ConfigBlockGuid,
> > > > - OUT VOID **ConfigBlockAddress
> > > > - )
> > > > -{
> > > > - UINT16 OffsetIndex;
> > > > - CONFIG_BLOCK *TempConfigBlk;
> > > > - CONFIG_BLOCK_TABLE_HEADER *ConfigBlkTblAddrPtr;
> > > > - UINT32 ConfigBlkTblHdrSize;
> > > > - UINT32 ConfigBlkOffset;
> > > > - UINT16 NumOfBlocks;
> > > > -
> > > > - ConfigBlkTblHdrSize = (UINT32)(sizeof
> > (CONFIG_BLOCK_TABLE_HEADER));
> > > > - ConfigBlkTblAddrPtr = (CONFIG_BLOCK_TABLE_HEADER
> > > > *)ConfigBlockTableAddress;
> > > > - NumOfBlocks = ConfigBlkTblAddrPtr->NumberOfBlocks;
> > > > -
> > > > - ConfigBlkOffset = 0;
> > > > - for (OffsetIndex = 0; OffsetIndex < NumOfBlocks; OffsetIndex++) {
> > > > - if ((ConfigBlkTblHdrSize + ConfigBlkOffset) > (ConfigBlkTblAddrPtr-
> > > > >Header.GuidHob.Header.HobLength)) {
> > > > - break;
> > > > - }
> > > > - TempConfigBlk = (CONFIG_BLOCK *)((UINTN)ConfigBlkTblAddrPtr +
> > > > (UINTN)ConfigBlkTblHdrSize + (UINTN)ConfigBlkOffset);
> > > > - if (CompareGuid (&(TempConfigBlk->Header.GuidHob.Name),
> > > > ConfigBlockGuid)) {
> > > > - *ConfigBlockAddress = (VOID *)TempConfigBlk;
> > > > - return EFI_SUCCESS;
> > > > - }
> > > > - ConfigBlkOffset = ConfigBlkOffset + TempConfigBlk-
> > > > >Header.GuidHob.Header.HobLength;
> > > > - }
> > > > - DEBUG ((DEBUG_ERROR, "Could not find the config block.\n"));
> > > > - return EFI_NOT_FOUND;
> > > > -}
> > > > diff --git
> > > > a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/Ba
> > > > seCo
> > > > nfigB
> > > > lockLib.inf
> > > > b/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/Ba
> > > > seCo
> > > > nfig
> > > > BlockLib.inf
> > > > deleted file mode 100644
> > > > index a7def2481d..0000000000
> > > > ---
> > > > a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/Ba
> > > > seCo
> > > > nfigB
> > > > lockLib.inf
> > > > +++ /dev/null
> > > > @@ -1,29 +0,0 @@
> > > > -## @file
> > > > -# Component INF file for the BaseConfigBlock library.
> > > > -#
> > > > -# Copyright (c) 2019 Intel Corporation. All rights reserved. <BR>
> > > > -# -# SPDX-License-Identifier: BSD-2-Clause-Patent -# -##
> > > > -
> > > > -[Defines]
> > > > -INF_VERSION = 0x00010017
> > > > -BASE_NAME = BaseConfigBlockLib
> > > > -FILE_GUID = 1EC07EA8-7808-4e06-9D79-309AE331D2D5
> > > > -VERSION_STRING = 1.0
> > > > -MODULE_TYPE = BASE
> > > > -LIBRARY_CLASS = ConfigBlockLib
> > > > -
> > > > -
> > > > -[Packages]
> > > > -MdePkg/MdePkg.dec
> > > > -CoffeelakeSiliconPkg/SiPkg.dec
> > > > -
> > > > -[Sources]
> > > > -BaseConfigBlockLib.c
> > > > -
> > > > -[LibraryClasses]
> > > > -DebugLib
> > > > -BaseMemoryLib
> > > > -MemoryAllocationLib
> > > > diff --git
> > > > a/Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/Base
> > > > Conf
> > > > igBlo
> > > > ckLib.c
> > > > b/Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/BaseCon
> > > > figB
> > > > lockLi
> > > > b.c
> > > > similarity index 100%
> > > > rename from
> > > > Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/BaseCo
> > > > nfig
> > > > Block
> > > > Lib.c
> > > > rename to
> > > > Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/BaseConfi
> > > > gBlo
> > > > ckLib.c
> > > > diff --git
> > > > a/Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/Base
> > > > Conf
> > > > igBlo
> > > > ckLib.inf
> > > > b/Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/BaseCon
> > > > figB
> > > > lockLi
> > > > b.inf
> > > > similarity index 100%
> > > > rename from
> > > > Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/BaseCo
> > > > nfig
> > > > Block
> > > > Lib.inf
> > > > rename to
> > > > Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/BaseConfi
> > > > gBlo
> > > > ckLib.i
> > > > nf
> > > > --
> > > > 2.16.2.windows.1
> > >
> >
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#50294): https://edk2.groups.io/g/devel/message/50294
Mute This Topic: https://groups.io/mt/45239214/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list