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.
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
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
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
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
-
Heyi Guo -
Wu, Hao A -
Zeng, Star