Re: [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib Library

["Ethan Tsao" <ethan.tsao@intel.com>]

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@intel.com>
> Sent: Friday, November 8, 2019 2:25 PM
> To: Tsao, Ethan <ethan.tsao@intel.com>; devel@edk2.groups.io
> Cc: Chaganty, Rangasai V <rangasai.v.chaganty@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@intel.com>
> > Sent: Thursday, November 7, 2019 4:14 PM
> > To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@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@intel.com>
> > > Sent: Thursday, November 7, 2019 3:15 PM
> > > To: Tsao, Ethan <ethan.tsao@intel.com>; edk2-devel@lists.01.org
> > > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@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@intel.com>
> > > > Sent: Thursday, November 7, 2019 11:31 AM
> > > > To: edk2-devel@lists.01.org
> > > > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Ni, Ray
> > > > <ray.ni@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@intel.com>
> > > > Cc: Sai Chaganty <rangasai.v.chaganty@intel.com>
> > > > Cc: Ray Ni <ray.ni@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
> > >
> >
>