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

[Heyi Guo <heyi.guo@linaro.org>]

Thanks Hao :)


在 12/5/2017 8:29 AM, Wu, Hao A 写道:
> Pushed at 9a77210b43ef34af52ea7285fadc0ce5779306fe.
>
> Best Regards,
> Hao Wu
>
>
>> -----Original Message-----
>> From: Zeng, Star
>> Sent: Monday, December 04, 2017 1:54 PM
>> To: Wu, Hao A; Heyi Guo; linaro-uefi@lists.linaro.org; edk2-devel@lists.01.org
>> Cc: Dong, Eric; Ni, Ruiyu; Zeng, Star
>> Subject: RE: [PATCH v2] MdeModulePkg/NvmExpressDxe: fix error status
>> override
>>
>> 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