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

["Zeng, Star" <star.zeng@intel.com>]

Reviewed-by: Star Zeng <star.zeng@intel.com>

Hao, please help push the patch if no other comments received.


Thanks,
Star
-----Original Message-----
From: Wu, Hao A 
Sent: Monday, December 4, 2017 10:47 AM
To: Heyi Guo <heyi.guo@linaro.org>; linaro-uefi@lists.linaro.org; edk2-devel@lists.01.org
Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
Subject: RE: [PATCH v2] MdeModulePkg/NvmExpressDxe: fix error status override

Reviewed-by: Hao Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu


> -----Original Message-----
> From: Heyi Guo [mailto:heyi.guo@linaro.org]
> Sent: Monday, December 04, 2017 10:28 AM
> To: linaro-uefi@lists.linaro.org; edk2-devel@lists.01.org
> Cc: Heyi Guo; Zeng, Star; Dong, Eric; Wu, Hao A; Ni, Ruiyu
> Subject: [PATCH v2] 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 save previous status before calling PciIo->Mem.Write and restore 
> the previous one if it already contains error.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Hao Wu <hao.a.wu@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> ---
>  MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> index c33038f..7356c1d 100644
> --- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> +++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> @@ -453,6 +453,7 @@ NvmExpressPassThru (  {
>    NVME_CONTROLLER_PRIVATE_DATA   *Private;
>    EFI_STATUS                     Status;
> +  EFI_STATUS                     PreviousStatus;
>    EFI_PCI_IO_PROTOCOL            *PciIo;
>    NVME_SQ                        *Sq;
>    NVME_CQ                        *Cq;
> @@ -831,6 +832,7 @@ NvmExpressPassThru (
>    }
> 
>    Data = ReadUnaligned32 ((UINT32*)&Private->CqHdbl[QueueId]);
> +  PreviousStatus = Status;
>    Status = PciIo->Mem.Write (
>                 PciIo,
>                 EfiPciIoWidthUint32,
> @@ -839,6 +841,9 @@ NvmExpressPassThru (
>                 1,
>                 &Data
>                 );
> +  // The return status of PciIo->Mem.Write should not override  // 
> + previous status if previous status contains error.
> +  Status = EFI_ERROR (PreviousStatus) ? PreviousStatus : Status;
> 
>    //
>    // For now, the code does not support the non-blocking feature for 
> admin queue.
> --
> 2.7.2.windows.1