Hi Maurice, Thank you for the comments. Will add CC to package owner in future. This patch is based on another patch which is also under code review. And the Copyright information has been updated in another patch.
Thanks, Guo -----Original Message----- From: Ma, Maurice Sent: Wednesday, May 20, 2015 12:00 AM To: Dong, Guo Cc: edk2-devel@lists.sourceforge.net Subject: RE: [edk2] [Patch] CorebootModulePkg: Coding style update Hi, Guo, Please remember to CC the package owners in future. Could you please update the Copyright information in the file accordingly? Everything else looks good to me. Reviewed-by: Maurice Ma <maurice...@intel.com> Thanks Maurice -----Original Message----- From: Guo Dong [mailto:guo.d...@intel.com] Sent: Tuesday, May 19, 2015 5:09 AM To: edk2-devel@lists.sourceforge.net Subject: [edk2] [Patch] CorebootModulePkg: Coding style update This patch update file CbParseLib.c to make it consistent with EDKII coding style: 1) Add function comments. 2) Add {} for if statement. 3) Compare with NULL for pointer and compare with 0 for integer instead of using them like a BOOLEAN. 4) For debug information only, use EFI_D_INFO instead of EFI_D_ERROR 5) Correct IN, OUT modifier Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Guo Dong <guo.d...@intel.com> --- CorebootModulePkg/Library/CbParseLib/CbParseLib.c | 247 ++++++++++++++-------- 1 file changed, 159 insertions(+), 88 deletions(-) diff --git a/CorebootModulePkg/Library/CbParseLib/CbParseLib.c b/CorebootModulePkg/Library/CbParseLib/CbParseLib.c index 8890629..df02e58 100644 --- a/CorebootModulePkg/Library/CbParseLib/CbParseLib.c +++ b/CorebootModulePkg/Library/CbParseLib/CbParseLib.c @@ -22,17 +22,38 @@ #include <IndustryStandard/Acpi.h> #include "Coreboot.h" -/* Helpful inlines */ -static UINT64 cb_unpack64(struct cbuint64 val) +/** + Convert a packed value from cbuint64 to a UINT64 value. + + @param val The pointer to packed data. + + @return the UNIT64 value after convertion. + +**/ +UINT64 +cb_unpack64 ( + IN struct cbuint64 val + ) { return LShiftU64 (val.hi, 32) | val.lo; } + +/** + Returns the sum of all elements in a buffer of 16-bit values. During + calculation, the carry bits are also been added. + + @param Buffer The pointer to the buffer to carry out the sum operation. + @param Length The size, in bytes, of Buffer. + + @return Sum The sum of Buffer with carry bits included during additions. + +**/ UINT16 CbCheckSum16 ( IN UINT16 *Buffer, IN UINTN Length ) @@ -58,13 +79,25 @@ CbCheckSum16 ( } return (UINT16)((~Sum) & 0xFFFF); } + +/** + Find coreboot record with given Tag from the memory Start in 4096 + bytes range. + + @param Start The start memory to be searched in + @param Tag The tag id to be found + + @retval NULL The Tag is not found. + @retval Others The poiter to the record found. + +**/ VOID * FindCbTag ( - IN VOID *Start, + IN VOID *Start, IN UINT32 Tag ) { struct cb_header *Header; struct cb_record *Record; @@ -74,21 +107,23 @@ FindCbTag ( UINT16 CheckSum; Header = NULL; TmpPtr = (UINT8 *)Start; for (Idx = 0; Idx < 4096; Idx += 16, TmpPtr += 16) { - Header = (struct cb_header *)TmpPtr; + Header = (struct cb_header *)TmpPtr; if (Header->signature == CB_HEADER_SIGNATURE) { break; } } - if (Idx >= 4096) + if (Idx >= 4096) { return NULL; + } - if (Header == NULL || !Header->table_bytes) + if ((Header == NULL) || (Header->table_bytes == 0)) { return NULL; + } // // Check the checksum of the coreboot table header // CheckSum = CbCheckSum16 ((UINT16 *)Header, sizeof (*Header)); @@ -107,14 +142,15 @@ FindCbTag ( TmpPtr += Header->header_bytes; for (Idx = 0; Idx < Header->table_entries; Idx++) { Record = (struct cb_record *)TmpPtr; if (Record->tag == CB_TAG_FORWARD) { TmpPtr = (VOID *)(UINTN)((struct cb_forward *)(UINTN)Record)->forward; - if (Tag == CB_TAG_FORWARD) + if (Tag == CB_TAG_FORWARD) { return TmpPtr; - else + } else { return FindCbTag (TmpPtr, Tag); + } } if (Record->tag == Tag) { TagPtr = TmpPtr; break; } @@ -122,10 +158,24 @@ FindCbTag ( } return TagPtr; } + +/** + Find the given table with TableId from the given coreboot memory Root. + + @param Root The coreboot memory table to be searched in + @param TableId Table id to be found + @param pMemTable To save the base address of the memory table found + @param pMemTableSize To save the size of memory table found + + @retval RETURN_SUCCESS Successfully find out the memory table. + @retval RETURN_INVALID_PARAMETER Invalid input parameters. + @retval RETURN_NOT_FOUND Failed to find the memory table. + +**/ RETURN_STATUS FindCbMemTable ( IN struct cbmem_root *Root, IN UINT32 TableId, OUT VOID **pMemTable, @@ -141,15 +191,15 @@ FindCbMemTable ( } // // Check if the entry is CBMEM or IMD // and handle them separately // - Entries = Root->entries; + Entries = Root->entries; if (Entries[0].magic == CBMEM_ENTRY_MAGIC) { IsImdEntry = FALSE; } else { - Entries = (struct cbmem_entry *)((struct imd_root *)Root)->entries; + Entries = (struct cbmem_entry *)((struct imd_root *)Root)->entries; if (Entries[0].magic == IMD_ENTRY_MAGIC) { IsImdEntry = TRUE; } else { return RETURN_NOT_FOUND; } @@ -186,41 +236,44 @@ FindCbMemTable ( @retval RETURN_NOT_FOUND Failed to find the memory information. **/ RETURN_STATUS CbParseMemoryInfo ( - IN UINT64* pLowMemorySize, - IN UINT64* pHighMemorySize + OUT UINT64 *pLowMemorySize, + OUT UINT64 *pHighMemorySize ) { - struct cb_memory* rec; - struct cb_memory_range* Range; + struct cb_memory *rec; + struct cb_memory_range *Range; UINT64 Start; UINT64 Size; UINTN Index; - if ((!pLowMemorySize) || (!pHighMemorySize)) + if ((pLowMemorySize == NULL) || (pHighMemorySize == NULL)) { return RETURN_INVALID_PARAMETER; + } // // Get the coreboot memory table // rec = (struct cb_memory *)FindCbTag (0, CB_TAG_MEMORY); - if (!rec) + if (rec == NULL) { rec = (struct cb_memory *)FindCbTag ((VOID *)(UINTN)PcdGet32 (PcdCbHeaderPointer), CB_TAG_MEMORY); + } - if (!rec) + if (rec == NULL) { return RETURN_NOT_FOUND; + } *pLowMemorySize = 0; *pHighMemorySize = 0; for (Index = 0; Index < MEM_RANGE_COUNT(rec); Index++) { Range = MEM_RANGE_PTR(rec, Index); Start = cb_unpack64(Range->start); Size = cb_unpack64(Range->size); - DEBUG ((EFI_D_ERROR, "%d. %016lx - %016lx [%02x]\n", + DEBUG ((EFI_D_INFO, "%d. %016lx - %016lx [%02x]\n", Index, Start, Start + Size - 1, Range->type)); if (Range->type != CB_MEM_RAM) { continue; } @@ -230,11 +283,11 @@ CbParseMemoryInfo ( } else { *pHighMemorySize = Start + Size - 0x100000000ULL; } } - DEBUG ((EFI_D_ERROR, "Low memory 0x%lx, High Memory 0x%lx\n", *pLowMemorySize, *pHighMemorySize)); + DEBUG ((EFI_D_INFO, "Low memory 0x%lx, High Memory 0x%lx\n", + *pLowMemorySize, *pHighMemorySize)); return RETURN_SUCCESS; } @@ -250,35 +303,37 @@ CbParseMemoryInfo ( @retval RETURN_NOT_FOUND Failed to find the memory table. **/ RETURN_STATUS CbParseCbMemTable ( - IN UINT32 TableId, - IN VOID** pMemTable, - IN UINT32* pMemTableSize + IN UINT32 TableId, + OUT VOID **pMemTable, + OUT UINT32 *pMemTableSize ) { - struct cb_memory* rec; - struct cb_memory_range* Range; + struct cb_memory *rec; + struct cb_memory_range *Range; UINT64 Start; UINT64 Size; UINTN Index; - if (!pMemTable) + if (pMemTable == NULL) { return RETURN_INVALID_PARAMETER; - + } *pMemTable = NULL; // // Get the coreboot memory table // rec = (struct cb_memory *)FindCbTag (0, CB_TAG_MEMORY); - if (!rec) + if (rec == NULL) { rec = (struct cb_memory *)FindCbTag ((VOID *)(UINTN)PcdGet32 (PcdCbHeaderPointer), CB_TAG_MEMORY); + } - if (!rec) + if (rec == NULL) { return RETURN_NOT_FOUND; + } for (Index = 0; Index < MEM_RANGE_COUNT(rec); Index++) { Range = MEM_RANGE_PTR(rec, Index); Start = cb_unpack64(Range->start); Size = cb_unpack64(Range->size); @@ -304,12 +359,12 @@ CbParseCbMemTable ( @retval RETURN_NOT_FOUND Failed to find the memory table. **/ RETURN_STATUS CbParseAcpiTable ( - IN VOID **pMemTable, - IN UINT32 *pMemTableSize + OUT VOID **pMemTable, + OUT UINT32 *pMemTableSize ) { return CbParseCbMemTable (SIGNATURE_32 ('I', 'P', 'C', 'A'), pMemTable, pMemTableSize); } @@ -324,12 +379,12 @@ CbParseAcpiTable ( @retval RETURN_NOT_FOUND Failed to find the memory table. **/ RETURN_STATUS CbParseSmbiosTable ( - IN VOID** pMemTable, - IN UINT32* pMemTableSize + OUT VOID **pMemTable, + OUT UINT32 *pMemTableSize ) { return CbParseCbMemTable (SIGNATURE_32 ('T', 'B', 'M', 'S'), pMemTable, pMemTableSize); } @@ -345,39 +400,41 @@ CbParseSmbiosTable ( @retval RETURN_NOT_FOUND Failed to find the fadt table. **/ RETURN_STATUS CbParseFadtInfo ( - IN UINTN* pPmCtrlReg, - IN UINTN* pPmTimerReg, - IN UINTN* pResetReg, - IN UINTN* pResetValue + OUT UINTN *pPmCtrlReg, + OUT UINTN *pPmTimerReg, + OUT UINTN *pResetReg, + OUT UINTN *pResetValue ) { - EFI_ACPI_3_0_ROOT_SYSTEM_DESCRIPTION_POINTER* Rsdp; - EFI_ACPI_DESCRIPTION_HEADER* Rsdt; - UINT32* Entry32; + EFI_ACPI_3_0_ROOT_SYSTEM_DESCRIPTION_POINTER *Rsdp; + EFI_ACPI_DESCRIPTION_HEADER *Rsdt; + UINT32 *Entry32; UINTN Entry32Num; - EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE* Fadt; - EFI_ACPI_DESCRIPTION_HEADER* Xsdt; - UINT64* Entry64; + EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE *Fadt; + EFI_ACPI_DESCRIPTION_HEADER *Xsdt; + UINT64 *Entry64; UINTN Entry64Num; UINTN Idx; RETURN_STATUS Status; Rsdp = NULL; Status = RETURN_SUCCESS; Status = CbParseAcpiTable (&Rsdp, NULL); - if (RETURN_ERROR(Status)) + if (RETURN_ERROR(Status)) { return Status; + } - if (!Rsdp) + if (Rsdp == NULL) { return RETURN_NOT_FOUND; + } - DEBUG ((EFI_D_ERROR, "Find Rsdp at %p\n", Rsdp)); - DEBUG ((EFI_D_ERROR, "Find Rsdt 0x%x, Xsdt 0x%lx\n", Rsdp->RsdtAddress, Rsdp->XsdtAddress)); + DEBUG ((EFI_D_INFO, "Find Rsdp at %p\n", Rsdp)); DEBUG ((EFI_D_INFO, + "Find Rsdt 0x%x, Xsdt 0x%lx\n", Rsdp->RsdtAddress, + Rsdp->XsdtAddress)); // // Search Rsdt First // Rsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)(Rsdp->RsdtAddress); @@ -385,25 +442,29 @@ CbParseFadtInfo ( Entry32 = (UINT32 *)(Rsdt + 1); Entry32Num = (Rsdt->Length - sizeof(EFI_ACPI_DESCRIPTION_HEADER)) >> 2; for (Idx = 0; Idx < Entry32Num; Idx++) { if (*(UINT32 *)(UINTN)(Entry32[Idx]) == EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) { Fadt = (EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE *)(UINTN)(Entry32[Idx]); - if (pPmCtrlReg) + if (pPmCtrlReg != NULL) { *pPmCtrlReg = Fadt->Pm1aCntBlk; - DEBUG ((EFI_D_ERROR, "PmCtrl Reg 0x%x\n", Fadt->Pm1aCntBlk)); + } + DEBUG ((EFI_D_INFO, "PmCtrl Reg 0x%x\n", Fadt->Pm1aCntBlk)); - if (pPmTimerReg) + if (pPmTimerReg != NULL) { *pPmTimerReg = Fadt->PmTmrBlk; - DEBUG ((EFI_D_ERROR, "PmTimer Reg 0x%x\n", Fadt->PmTmrBlk)); + } + DEBUG ((EFI_D_INFO, "PmTimer Reg 0x%x\n", Fadt->PmTmrBlk)); - if (pResetReg) + if (pResetReg != NULL) { *pResetReg = (UINTN)Fadt->ResetReg.Address; - DEBUG ((EFI_D_ERROR, "Reset Reg 0x%lx\n", Fadt->ResetReg.Address)); + } + DEBUG ((EFI_D_INFO, "Reset Reg 0x%lx\n", + Fadt->ResetReg.Address)); - if (pResetValue) + if (pResetValue != NULL) { *pResetValue = Fadt->ResetValue; - DEBUG ((EFI_D_ERROR, "Reset Value 0x%x\n", Fadt->ResetValue)); + } + DEBUG ((EFI_D_INFO, "Reset Value 0x%x\n", Fadt->ResetValue)); return RETURN_SUCCESS; } } } @@ -453,32 +514,37 @@ CbParseFadtInfo ( @retval RETURN_NOT_FOUND Failed to find the serial port information . **/ RETURN_STATUS CbParseSerialInfo ( - IN UINT32* pRegBase, - IN UINT32* pRegAccessType, - IN UINT32* pBaudrate + OUT UINT32 *pRegBase, + OUT UINT32 *pRegAccessType, + OUT UINT32 *pBaudrate ) { - struct cb_serial* CbSerial; + struct cb_serial *CbSerial; CbSerial = FindCbTag (0, CB_TAG_SERIAL); - if (!CbSerial) + if (CbSerial == NULL) { CbSerial = FindCbTag ((VOID *)(UINTN)PcdGet32 (PcdCbHeaderPointer), CB_TAG_SERIAL); + } - if (!CbSerial) + if (CbSerial == NULL) { return RETURN_NOT_FOUND; + } - if (pRegBase) + if (pRegBase != NULL) { *pRegBase = CbSerial->baseaddr; + } - if (pRegAccessType) + if (pRegAccessType != NULL) { *pRegAccessType = CbSerial->type; + } - if (pBaudrate) + if (pBaudrate != NULL) { *pBaudrate = CbSerial->baud; + } return RETURN_SUCCESS; } /** @@ -491,25 +557,27 @@ CbParseSerialInfo ( @retval RETURN_NOT_FOUND Failed to find the coreboot table header . **/ RETURN_STATUS CbParseGetCbHeader ( - IN UINTN Level, - IN VOID** HeaderPtr + IN UINTN Level, + OUT VOID **HeaderPtr ) { UINTN Index; - VOID* TempPtr; + VOID *TempPtr; - if (!HeaderPtr) + if (HeaderPtr == NULL) { return RETURN_NOT_FOUND; + } TempPtr = NULL; for (Index = 0; Index < Level; Index++) { TempPtr = FindCbTag (TempPtr, CB_TAG_FORWARD); - if (!TempPtr) + if (TempPtr == NULL) { break; + } } if ((Index >= Level) && (TempPtr != NULL)) { *HeaderPtr = TempPtr; return RETURN_SUCCESS; @@ -527,40 +595,43 @@ CbParseGetCbHeader ( @retval RETURN_NOT_FOUND Failed to find the video frame buffer information . **/ RETURN_STATUS CbParseFbInfo ( - IN FRAME_BUFFER_INFO* pFbInfo + OUT FRAME_BUFFER_INFO *pFbInfo ) { - struct cb_framebuffer* CbFbRec; + struct cb_framebuffer *CbFbRec; - if (!pFbInfo) + if (pFbInfo == NULL) { return RETURN_INVALID_PARAMETER; + } CbFbRec = FindCbTag (0, CB_TAG_FRAMEBUFFER); - if (!CbFbRec) + if (CbFbRec == NULL) { CbFbRec = FindCbTag ((VOID *)(UINTN)PcdGet32 (PcdCbHeaderPointer), CB_TAG_FRAMEBUFFER); + } - if (!CbFbRec) + if (CbFbRec == NULL) { return RETURN_NOT_FOUND; + } - DEBUG ((EFI_D_ERROR, "Found coreboot video frame buffer information\n")); - DEBUG ((EFI_D_ERROR, "physical_address: 0x%lx\n", CbFbRec->physical_address)); - DEBUG ((EFI_D_ERROR, "x_resolution: 0x%x\n", CbFbRec->x_resolution)); - DEBUG ((EFI_D_ERROR, "y_resolution: 0x%x\n", CbFbRec->y_resolution)); - DEBUG ((EFI_D_ERROR, "bits_per_pixel: 0x%x\n", CbFbRec->bits_per_pixel)); - DEBUG ((EFI_D_ERROR, "bytes_per_line: 0x%x\n", CbFbRec->bytes_per_line)); - - DEBUG ((EFI_D_ERROR, "red_mask_size: 0x%x\n", CbFbRec->red_mask_size)); - DEBUG ((EFI_D_ERROR, "red_mask_pos: 0x%x\n", CbFbRec->red_mask_pos)); - DEBUG ((EFI_D_ERROR, "green_mask_size: 0x%x\n", CbFbRec->green_mask_size)); - DEBUG ((EFI_D_ERROR, "green_mask_pos: 0x%x\n", CbFbRec->green_mask_pos)); - DEBUG ((EFI_D_ERROR, "blue_mask_size: 0x%x\n", CbFbRec->blue_mask_size)); - DEBUG ((EFI_D_ERROR, "blue_mask_pos: 0x%x\n", CbFbRec->blue_mask_pos)); - DEBUG ((EFI_D_ERROR, "reserved_mask_size: 0x%x\n", CbFbRec->reserved_mask_size)); - DEBUG ((EFI_D_ERROR, "reserved_mask_pos: 0x%x\n", CbFbRec->reserved_mask_pos)); + DEBUG ((EFI_D_INFO, "Found coreboot video frame buffer + information\n")); DEBUG ((EFI_D_INFO, "physical_address: 0x%lx\n", + CbFbRec->physical_address)); DEBUG ((EFI_D_INFO, "x_resolution: + 0x%x\n", CbFbRec->x_resolution)); DEBUG ((EFI_D_INFO, "y_resolution: + 0x%x\n", CbFbRec->y_resolution)); DEBUG ((EFI_D_INFO, + "bits_per_pixel: 0x%x\n", CbFbRec->bits_per_pixel)); DEBUG + ((EFI_D_INFO, "bytes_per_line: 0x%x\n", CbFbRec->bytes_per_line)); + + DEBUG ((EFI_D_INFO, "red_mask_size: 0x%x\n", + CbFbRec->red_mask_size)); DEBUG ((EFI_D_INFO, "red_mask_pos: 0x%x\n", + CbFbRec->red_mask_pos)); DEBUG ((EFI_D_INFO, "green_mask_size: + 0x%x\n", CbFbRec->green_mask_size)); DEBUG ((EFI_D_INFO, + "green_mask_pos: 0x%x\n", CbFbRec->green_mask_pos)); DEBUG + ((EFI_D_INFO, "blue_mask_size: 0x%x\n", CbFbRec->blue_mask_size)); + DEBUG ((EFI_D_INFO, "blue_mask_pos: 0x%x\n", CbFbRec->blue_mask_pos)); + DEBUG ((EFI_D_INFO, "reserved_mask_size: 0x%x\n", + CbFbRec->reserved_mask_size)); DEBUG ((EFI_D_INFO, + "reserved_mask_pos: 0x%x\n", CbFbRec->reserved_mask_pos)); pFbInfo->LinearFrameBuffer = CbFbRec->physical_address; pFbInfo->HorizontalResolution = CbFbRec->x_resolution; pFbInfo->VerticalResolution = CbFbRec->y_resolution; pFbInfo->BitsPerPixel = CbFbRec->bits_per_pixel; -- 1.9.5.msysgit.0 ------------------------------------------------------------------------------ One dashboard for servers and applications across Physical-Virtual-Cloud Widest out-of-the-box monitoring support with 50+ applications Performance metrics, stats and reports that give you Actionable Insights Deep dive visibility with transaction tracing using APM Insight. http://ad.doubleclick.net/ddm/clk/290420510;117567292;y _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel ------------------------------------------------------------------------------ One dashboard for servers and applications across Physical-Virtual-Cloud Widest out-of-the-box monitoring support with 50+ applications Performance metrics, stats and reports that give you Actionable Insights Deep dive visibility with transaction tracing using APM Insight. http://ad.doubleclick.net/ddm/clk/290420510;117567292;y _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
