[edk2-devel] [PATCH 1/1] MdeModulePkg/Ata: Fix command status reporting

Fri Oct 21 02:27:10 UTC 2022

I see the point, thanks a lot for the testing.

Best Regards,
Hao Wu

From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Anbazhagan, Baraneedharan via groups.io
Sent: Friday, October 21, 2022 6:05 AM
To: Wu, Hao A <hao.a.wu at intel.com>; Albecki, Mateusz <mateusz.albecki at intel.com>; devel at edk2.groups.io
Cc: Ni, Ray <ray.ni at intel.com>
Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg/Ata: Fix command status reporting

Below patch didn't resolve https://bugzilla.tianocore.org/show_bug.cgi?id=4011 since AhciRecoverPortError return success on incorrect drive password and continues with additional retries before returning to caller.

From: Wu, Hao A <hao.a.wu at intel.com<mailto:hao.a.wu at intel.com>>
Sent: Tuesday, October 18, 2022 8:37 PM
To: Anbazhagan, Baraneedharan <anbazhagan at hp.com<mailto:anbazhagan at hp.com>>; Albecki, Mateusz <mateusz.albecki at intel.com<mailto:mateusz.albecki at intel.com>>; devel at edk2.groups.io<mailto:devel at edk2.groups.io>
Cc: Ni, Ray <ray.ni at intel.com<mailto:ray.ni at intel.com>>
Subject: RE: [PATCH 1/1] MdeModulePkg/Ata: Fix command status reporting

CAUTION: External Email
Hello Baraneedharan Anbazhagan,

Could you help to check if this patch can resolve the issue https://bugzilla.tianocore.org/show_bug.cgi?id=4011 when switching back to: "#define AHCI_COMMAND_RETRIES 5"?
This change can be accessed for integration at: https://github.com/tianocore/edk2/commit/f2eb27e00ed55cca03964f13a2839e41e2c1f61f.patch

Best Regards,
Hao Wu

> -----Original Message-----
> From: Albecki, Mateusz <mateusz.albecki at intel.com<mailto:mateusz.albecki at intel.com>>
> Sent: Tuesday, October 18, 2022 11:54 PM
> To: devel at edk2.groups.io<mailto:devel at edk2.groups.io>
> Cc: Albecki, Mateusz <mateusz.albecki at intel.com<mailto:mateusz.albecki at intel.com>>; Wu, Hao A
> <hao.a.wu at intel.com<mailto:hao.a.wu at intel.com>>; Ni, Ray <ray.ni at intel.com<mailto:ray.ni at intel.com>>
> Subject: [PATCH 1/1] MdeModulePkg/Ata: Fix command status reporting
>
> Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=4016
>
> AtaAtapiPassThru driver was reporting recovery status on failed command
> packets which led to incorrect flows in upper layers and to SCT tests
> fails. This commit will change the logic to report command status.
>
>
> Cc: Hao A Wu <hao.a.wu at intel.com<mailto:hao.a.wu at intel.com>>
>
> Cc: Ray Ni <ray.ni at intel.com<mailto:ray.ni at intel.com>>
>
>
> Signed-off-by: Mateusz Albecki <mateusz.albecki at intel.com<mailto:mateusz.albecki at intel.com>>
> ---
> .../Bus/Ata/AtaAtapiPassThru/AhciMode.c | 19 +++++++++++--------
> 1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> index a240be940d..06c4a3e052 100644
> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> @@ -949,6 +949,7 @@ AhciPioTransfer (
> EFI_AHCI_COMMAND_LIST CmdList;
>
> UINT32 PrdCount;
>
> UINT32 Retry;
>
> + EFI_STATUS RecoveryStatus;
>
>
>
> if (Read) {
>
> Flag = EfiPciIoOperationBusMasterWrite;
>
> @@ -1026,8 +1027,8 @@ AhciPioTransfer (
>
>
> if (Status == EFI_DEVICE_ERROR) {
>
> DEBUG ((DEBUG_ERROR, "PIO command failed at retry %d\n", Retry));
>
> - Status = AhciRecoverPortError (PciIo, Port);
>
> - if (EFI_ERROR (Status)) {
>
> + RecoveryStatus = AhciRecoverPortError (PciIo, Port);
>
> + if (EFI_ERROR (RecoveryStatus)) {
>
> break;
>
> }
>
> } else {
>
> @@ -1122,6 +1123,7 @@ AhciDmaTransfer (
> EFI_PCI_IO_PROTOCOL *PciIo;
>
> EFI_TPL OldTpl;
>
> UINT32 Retry;
>
> + EFI_STATUS RecoveryStatus;
>
>
>
> Map = NULL;
>
> PciIo = Instance->PciIo;
>
> @@ -1220,8 +1222,8 @@ AhciDmaTransfer (
> Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout, SataFisD2H);
>
> if (Status == EFI_DEVICE_ERROR) {
>
> DEBUG ((DEBUG_ERROR, "DMA command failed at retry: %d\n", Retry));
>
> - Status = AhciRecoverPortError (PciIo, Port);
>
> - if (EFI_ERROR (Status)) {
>
> + RecoveryStatus = AhciRecoverPortError (PciIo, Port);
>
> + if (EFI_ERROR (RecoveryStatus)) {
>
> break;
>
> }
>
> } else {
>
> @@ -1261,14 +1263,14 @@ AhciDmaTransfer (
> Status = AhciCheckFisReceived (PciIo, Port, SataFisD2H);
>
> if (Status == EFI_DEVICE_ERROR) {
>
> DEBUG ((DEBUG_ERROR, "DMA command failed at retry: %d\n", Task-
> >RetryTimes));
>
> - Status = AhciRecoverPortError (PciIo, Port);
>
> + RecoveryStatus = AhciRecoverPortError (PciIo, Port);
>
> //
>
> // If recovery passed mark the Task as not started and change the status
>
> // to EFI_NOT_READY. This will make the higher level call this function
> again
>
> // and on next call the command will be re-issued due to IsStart being
> FALSE.
>
> // This also makes the next condition decrement the RetryTimes.
>
> //
>
> - if (Status == EFI_SUCCESS) {
>
> + if (RecoveryStatus == EFI_SUCCESS) {
>
> Task->IsStart = FALSE;
>
> Status = EFI_NOT_READY;
>
> }
>
> @@ -1375,6 +1377,7 @@ AhciNonDataTransfer (
> EFI_AHCI_COMMAND_FIS CFis;
>
> EFI_AHCI_COMMAND_LIST CmdList;
>
> UINT32 Retry;
>
> + EFI_STATUS RecoveryStatus;
>
>
>
> //
>
> // Package read needed
>
> @@ -1415,8 +1418,8 @@ AhciNonDataTransfer (
> Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout, SataFisD2H);
>
> if (Status == EFI_DEVICE_ERROR) {
>
> DEBUG ((DEBUG_ERROR, "Non data transfer failed at retry %d\n", Retry));
>
> - Status = AhciRecoverPortError (PciIo, Port);
>
> - if (EFI_ERROR (Status)) {
>
> + RecoveryStatus = AhciRecoverPortError (PciIo, Port);
>
> + if (EFI_ERROR (RecoveryStatus)) {
>
> break;
>
> }
>
> } else {
>
> --
> 2.28.0.windows.1

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#95455): https://edk2.groups.io/g/devel/message/95455
Mute This Topic: https://groups.io/mt/94411389/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20221021/2fd01267/attachment-0001.htm>