On 11/26/15 18:39, Laszlo Ersek wrote:
> Hi Star,
>
> On 11/25/15 02:33, Star Zeng wrote:
>> 1. Update fdf and dsc to use SerialDxe in MdeModulePkg.
>> 2. Separate the code that gets SerialRegBase and SerialRegAccessType
>> by CbParseLib from CorebootPayloadPkg/Library/SerialPortLib to
>> PlatformHookLib, and then leverage BaseSerialPortLib16550 in
>> MdeModulePkg.
>> 3. Remove CorebootPayloadPkg/SerialDxe and
>> CorebootPayloadPkg/Library/SerialPortLib.
>>
>> Cc: Michael D Kinney <michael.d.kin...@intel.com>
>> Cc: Liming Gao <liming....@intel.com>
>> Cc: Maurice Ma <maurice...@intel.com>
>> Cc: Prince Agyeman <prince.agye...@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Star Zeng <star.z...@intel.com>
>> Reviewed-by: Maurice Ma <maurice...@intel.com>
>> ---
>> CorebootPayloadPkg/CorebootPayloadPkg.fdf | 4 +-
>> CorebootPayloadPkg/CorebootPayloadPkgIa32.dsc | 11 +-
>> CorebootPayloadPkg/CorebootPayloadPkgIa32X64.dsc | 11 +-
>> .../Library/PlatformBdsLib/BdsPlatform.h | 5 +-
>> .../Library/PlatformHookLib/PlatformHookLib.c | 56 +++
>> .../Library/PlatformHookLib/PlatformHookLib.inf | 38 ++
>> .../Library/SerialPortLib/SerialPortLib.c | 316 -----------------
>> .../Library/SerialPortLib/SerialPortLib.inf | 42 ---
>> CorebootPayloadPkg/SerialDxe/SerialDxe.inf | 55 ---
>> CorebootPayloadPkg/SerialDxe/SerialIo.c | 392
>> ---------------------
>> 10 files changed, 114 insertions(+), 816 deletions(-)
>> create mode 100644
>> CorebootPayloadPkg/Library/PlatformHookLib/PlatformHookLib.c
>> create mode 100644
>> CorebootPayloadPkg/Library/PlatformHookLib/PlatformHookLib.inf
>> delete mode 100644 CorebootPayloadPkg/Library/SerialPortLib/SerialPortLib.c
>> delete mode 100644
>> CorebootPayloadPkg/Library/SerialPortLib/SerialPortLib.inf
>> delete mode 100644 CorebootPayloadPkg/SerialDxe/SerialDxe.inf
>> delete mode 100644 CorebootPayloadPkg/SerialDxe/SerialIo.c
>
> I'm not completely sure if this patch caused the breakage, but I got a
> CorebootPayloadPkg build failure report, and I sort of guess this patch could
> be the culprit.
>
> The build error is as follows:
>
>> In file included from <command-line>:0:0:
>> CorebootPayloadPkg/Library/PlatformHookLib/PlatformHookLib.c: In function
>> 'PlatformHookSerialPortInitialize':
>> Build/CorebootPayloadPkgIA32/DEBUG_GCC49/IA32/CorebootPayloadPkg/Library/PlatformHookLib/PlatformHookLib/DEBUG/AutoGen.h:31:102:
>> error: right-hand operand of comma expression has no effect
>> [-Werror=unused-value]
>> #define _PCD_SET_MODE_BOOL_S_PcdSerialUseMmio(Value)
>> ((_gPcd_BinaryPatch_PcdSerialUseMmio = (Value)), RETURN_SUCCESS)
>>
>> ^
>> MdePkg/Include/Library/PcdLib.h:687:45: note: in expansion of macro
>> '_PCD_SET_MODE_BOOL_S_PcdSerialUseMmio'
>> #define PcdSetBoolS(TokenName, Value)
>> _PCD_SET_MODE_BOOL_S_##TokenName ((Value))
>> ^
>> CorebootPayloadPkg/Library/PlatformHookLib/PlatformHookLib.c:48:5: note: in
>> expansion of macro 'PcdSetBoolS'
>> PcdSetBoolS (PcdSerialUseMmio, TRUE);
>> ^
>> Build/CorebootPayloadPkgIA32/DEBUG_GCC49/IA32/CorebootPayloadPkg/Library/PlatformHookLib/PlatformHookLib/DEBUG/AutoGen.h:31:102:
>> error: right-hand operand of comma expression has no effect
>> [-Werror=unused-value]
>> #define _PCD_SET_MODE_BOOL_S_PcdSerialUseMmio(Value)
>> ((_gPcd_BinaryPatch_PcdSerialUseMmio = (Value)), RETURN_SUCCESS)
>>
>> ^
>> MdePkg/Include/Library/PcdLib.h:687:45: note: in expansion of macro
>> '_PCD_SET_MODE_BOOL_S_PcdSerialUseMmio'
>> #define PcdSetBoolS(TokenName, Value)
>> _PCD_SET_MODE_BOOL_S_##TokenName ((Value))
>> ^
>> CorebootPayloadPkg/Library/PlatformHookLib/PlatformHookLib.c:50:5: note: in
>> expansion of macro 'PcdSetBoolS'
>> PcdSetBoolS (PcdSerialUseMmio, FALSE);
>> ^
>> Build/CorebootPayloadPkgIA32/DEBUG_GCC49/IA32/CorebootPayloadPkg/Library/PlatformHookLib/PlatformHookLib/DEBUG/AutoGen.h:39:110:
>> error: right-hand operand of comma expression has no effect
>> [-Werror=unused-value]
>> #define _PCD_SET_MODE_64_S_PcdSerialRegisterBase(Value)
>> ((_gPcd_BinaryPatch_PcdSerialRegisterBase = (Value)), RETURN_SUCCESS)
>>
>> ^
>> MdePkg/Include/Library/PcdLib.h:647:45: note: in expansion of macro
>> '_PCD_SET_MODE_64_S_PcdSerialRegisterBase'
>> #define PcdSet64S(TokenName, Value) _PCD_SET_MODE_64_S_##TokenName
>> ((Value))
>> ^
>> CorebootPayloadPkg/Library/PlatformHookLib/PlatformHookLib.c:52:3: note: in
>> expansion of macro 'PcdSet64S'
>> PcdSet64S (PcdSerialRegisterBase, (UINT64) SerialRegBase);
>> ^
>> cc1: all warnings being treated as errors
>> GNUmakefile:299: recipe for target
>> 'Build/CorebootPayloadPkgIA32/DEBUG_GCC49/IA32/CorebootPayloadPkg/Library/PlatformHookLib/PlatformHookLib/OUTPUT/PlatformHookLib.obj'
>> failed
>> make: ***
>> [Build/CorebootPayloadPkgIA32/DEBUG_GCC49/IA32/CorebootPayloadPkg/Library/PlatformHookLib/PlatformHookLib/OUTPUT/PlatformHookLib.obj]
>> Error 1
Thinking a bit further, I believe this occurs because your patch adds a number
of PcdSetxxxS() calls, but the return status is never checked:
+RETURN_STATUS
+EFIAPI
+PlatformHookSerialPortInitialize (
+ VOID
+ )
+{
+ RETURN_STATUS Status;
+ UINT32 SerialRegBase;
+ UINT32 SerialRegAccessType;
+
+ Status = CbParseSerialInfo (&SerialRegBase, &SerialRegAccessType, NULL);
+ if (RETURN_ERROR (Status)) {
+ return Status;
+ }
+
+ if (SerialRegAccessType == 2) { //MMIO
+ PcdSetBoolS (PcdSerialUseMmio, TRUE);
+ } else { //IO
+ PcdSetBoolS (PcdSerialUseMmio, FALSE);
+ }
+ PcdSet64S (PcdSerialRegisterBase, (UINT64) SerialRegBase);
+
+ return RETURN_SUCCESS;
+}
+
This makes PcdSetxxxS() kinda useless -- their purpose is *exactly* so the
programmer can check the return value.
So I'd propose to either check the return values, or to use the older
PcdSetxxxx() macros / functions (that don't return statuses).
Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
