From: "Wu, Hao A" <hao.a.wu@intel.com>
To: "Zeng, Star" <star.zeng@intel.com>,
Heyi Guo <heyi.guo@linaro.org>,
"linaro-uefi@lists.linaro.org" <linaro-uefi@lists.linaro.org>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Dong, Eric" <eric.dong@intel.com>, "Ni, Ruiyu" <ruiyu.ni@intel.com>
Subject: Re: [PATCH v2] MdeModulePkg/NvmExpressDxe: fix error status override
Date: Tue, 5 Dec 2017 00:29:31 +0000 [thread overview]
Message-ID: <B80AF82E9BFB8E4FBD8C89DA810C6A0931D271C3@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB0483103B9BF262@shsmsx102.ccr.corp.intel.com>
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
next prev parent reply other threads:[~2017-12-05 0:25 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-12-04 2:27 [PATCH v2] MdeModulePkg/NvmExpressDxe: fix error status override Heyi Guo
2017-12-04 2:46 ` Wu, Hao A
2017-12-04 5:54 ` Zeng, Star
2017-12-05 0:29 ` Wu, Hao A [this message]
2017-12-05 1:13 ` Heyi Guo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=B80AF82E9BFB8E4FBD8C89DA810C6A0931D271C3@SHSMSX104.ccr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox