Leif,



2017-03-20 16:20 GMT+01:00 Leif Lindholm <leif.lindholm@linaro.org>:
On Fri, Mar 17, 2017 at 01:40:29AM +0100, Marcin Wojtas wrote:
> From: Konstantin Porotchkin <kostap@marvell.com>
>
> Fix compilation warning triggered by GCC 4.9.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Konstantin Porotchkin <kostap@marvell.com>
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Platforms/Marvell/Library/ComPhyLib/ComPhyCp110.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Platforms/Marvell/Library/ComPhyLib/ComPhyCp110.c b/Platforms/Marvell/Library/ComPhyLib/ComPhyCp110.c
> index df8731d..07a4e6f 100755
> --- a/Platforms/Marvell/Library/ComPhyLib/ComPhyCp110.c
> +++ b/Platforms/Marvell/Library/ComPhyLib/ComPhyCp110.c
> @@ -980,7 +980,7 @@ ComPhyCp110Init (
>    IN CHIP_COMPHY_CONFIG *PtrChipCfg
>    )
>  {
> -  EFI_STATUS Status;
> +  EFI_STATUS Status = EFI_SUCCESS;
>    COMPHY_MAP *PtrComPhyMap, *SerdesMap;
>    EFI_PHYSICAL_ADDRESS ComPhyBaseAddr, HpipeBaseAddr;
>    UINT32 ComPhyMaxCount, Lane;
> @@ -1043,5 +1043,5 @@ ComPhyCp110Init (
>          Lane));
>    }
>
> -  return EFI_SUCCESS;
> +  return Status;

OK, so I never objected to this the first time around, but do we
really want to return SUCCESS if processing invalid data?
It doesn't even look like the return value is checked...

If you really don't care, make the function return void.
If you do, set Status to EFI_UNSUPPORTED, EFI_NOT_FOUND or
EFI_INVALID_PARAMETER on the default target of the switch as well.


I agree returning Status at the very end doesn't make much sense, so I'll remove it. For DEBUG build I'll add and assert in switch default and leave the rest as is -- each lane is initialized independently, so normally I prefer not to crash in case one of them fails. Is it ok for you?

Thanks,
Marcin