From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:400e:c00::22f; helo=mail-pf0-x22f.google.com; envelope-from=heyi.guo@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-pf0-x22f.google.com (mail-pf0-x22f.google.com [IPv6:2607:f8b0:400e:c00::22f]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 0DBE2220EE069 for ; Sun, 3 Dec 2017 17:33:26 -0800 (PST) Received: by mail-pf0-x22f.google.com with SMTP id p84so7348822pfd.3 for ; Sun, 03 Dec 2017 17:37:56 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-transfer-encoding:content-language; bh=tbmOQNKbw18roj7UdA47p1m9F1mJuef2xsnyupWzEGg=; b=Yh6gsGAbF8AK6tCwGUDTzQvuOVyI9MpO7O+Lh0YKT5NUE3NmWPNUVvaLxLFKtLheP7 gfLjSpO3V5Iw54zThFiHM9qdTgsCYMkroPdc1Rwwey3p+pckjAEsU7DjD7/jzrE0VYnO CTafyWM4ZMM3I7K49KtRCFyJfMCgdOaO65KV8= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding :content-language; bh=tbmOQNKbw18roj7UdA47p1m9F1mJuef2xsnyupWzEGg=; b=jDJm+IcvnEktXHutpVpAFr6zG1CNqkbDJ0KTGeeMTmmT5i5sToP4TS1wNAumtWHNci ZHYkyWh6LwN18BCKVDsf4Wz8N9epDlVD7driThSUNB+RSImgUyPgE6JtdXj154dG3tPI meYkdw5dAKz6D1NqAEpno0JAm7DVU11xV14Kz3LWrd817UU+nlUTWVLfkay+zUto8AMP cclnZzW1zmT3AbRtecNU1VjSINePXTsPMun1cz2YMfd4oEcsMUOTQ219rXN3/Hc4iRVH d4MWzdRJb9qSh/uIdMqY7Uccz8tBUnRTbTy+J2bApL17GiYBL/K6MvkDHYEn3/TPGo2F xT+A== X-Gm-Message-State: AJaThX6+ueGlsUhJU0i+Tr1/wB7VZ0TG4Jq8LvxII8YQe/Jf8GDrRdiL xviLYHYNVgFj7czWeiFMi0pz0Q== X-Google-Smtp-Source: AGs4zMYR92RgFShUOi1cLtlDC7Pn2nq29LKhxmqaLHU6SNB8gaXH7OfPguhcupHYXNMh6AAlVjqydA== X-Received: by 10.101.101.131 with SMTP id u3mr12311869pgv.76.1512351475722; Sun, 03 Dec 2017 17:37:55 -0800 (PST) Received: from [10.105.3.162] ([104.237.91.54]) by smtp.gmail.com with ESMTPSA id k89sm20827240pfb.104.2017.12.03.17.37.53 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sun, 03 Dec 2017 17:37:55 -0800 (PST) To: "Wu, Hao A" , "Zeng, Star" , "linaro-uefi@lists.linaro.org" , "edk2-devel@lists.01.org" Cc: Heyi Guo , "Dong, Eric" , "Ni, Ruiyu" References: <1512206536-39536-1-git-send-email-heyi.guo@linaro.org> <0C09AFA07DD0434D9E2A0C6AEB0483103B9BF055@shsmsx102.ccr.corp.intel.com> From: Heyi Guo Message-ID: <4476f7d8-2690-2c87-9cdf-4bb3a31766c1@linaro.org> Date: Mon, 4 Dec 2017 09:33:30 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: Subject: Re: [PATCH] MdeModulePkg/NvmExpressDxe: fix error status override X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 04 Dec 2017 01:33:27 -0000 Content-Type: text/plain; charset=gbk; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US ÔÚ 12/4/2017 9:32 AM, Wu, Hao A дµÀ: > 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 ; Heyi Guo ; Dong, >> Eric ; Zeng, Star >> 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 >> Cc: Star Zeng >> Cc: Eric Dong >> --- >> 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. That's true. > > 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; Looks good to me. I will send out v2 patch. Thanks for your comments. Regards, Gary (Heyi Guo) > > >> -- >> 2.7.2.windows.1 >> >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel