Re: [PATCH] MdeModulePkg/NvmExpressDxe: fix error status override

["Wu, Hao A" <hao.a.wu@intel.com>]

Comments below.

Best Regards,
Hao Wu


> -----Original Message-----
> From: Zeng, Star
> Sent: Monday, December 04, 2017 9:17 AM
> To: Heyi Guo; linaro-uefi@lists.linaro.org; edk2-devel@lists.01.org
> Cc: Heyi Guo; Dong, Eric; Wu, Hao A; Ni, Ruiyu; Zeng, Star
> Subject: RE: [edk2] [PATCH] MdeModulePkg/NvmExpressDxe: fix error status
> override
> 
> Cc Hao and Ruiyu.
> 
> Thanks,
> Star
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Heyi
> Guo
> Sent: Saturday, December 2, 2017 5:22 PM
> To: linaro-uefi@lists.linaro.org; edk2-devel@lists.01.org
> Cc: Heyi Guo <guoheyi@huawei.com>; Heyi Guo <heyi.guo@linaro.org>; Dong,
> Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: [edk2] [PATCH] MdeModulePkg/NvmExpressDxe: fix error status
> override
> 
> Commit f6b139b added return status handling to PciIo->Mem.Write.
> However, the second status handling will override EFI_DEVICE_ERROR returned
> in this branch:
> 
>   //
>   // Check the NVMe cmd execution result
>   //
>   if (Status != EFI_TIMEOUT) {
>     if ((Cq->Sct == 0) && (Cq->Sc == 0)) {
>       Status = EFI_SUCCESS;
>     } else {
>       Status = EFI_DEVICE_ERROR;
>                ^^^^^^^^^^^^^^^^
> 
> Since PciIo->Mem.Write will probably return SUCCESS, it causes
> NvmExpressPassThru to return SUCCESS even when DEVICE_ERROR occurs.
> Callers of NvmExpressPassThru will then continue executing which may cause
> further unexpected results, e.g. DiscoverAllNamespaces couldn't break out the
> loop.
> 
> So we add a | (bit-or) to combine the return status together.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Heyi Guo <guoheyi@huawei.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> ---
>  MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> index c33038f..2698b27 100644
> --- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> +++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> @@ -831,7 +831,7 @@ NvmExpressPassThru (
>    }
> 
>    Data = ReadUnaligned32 ((UINT32*)&Private->CqHdbl[QueueId]);
> -  Status = PciIo->Mem.Write (
> +  Status |= PciIo->Mem.Write (
>                 PciIo,
>                 EfiPciIoWidthUint32,
>                 NVME_BAR,

I searched the whole edk2 code base and did not find a similar case like:
Status |= Foo();

I also think using the above style might cause the function returning
confusing status to the caller.

So how about introducing a new variable?
  EFI_STATUS   PreviousStatus;
  ...
  Data = ReadUnaligned32 ((UINT32*)&Private->CqHdbl[QueueId]);
  PreviousStatus = Status;
  Status = PciIo->Mem.Write (...);
  Status = EFI_ERROR (PreviousStatus) ? PreviousStatus : Status;


> --
> 2.7.2.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel