Re: [PATCH 1/1] MdeModulePkg/Ata: Fix command status reporting

["Wu, Hao A" <hao.a.wu@intel.com>]

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@intel.com>
> Sent: Tuesday, October 18, 2022 11:54 PM
> To: devel@edk2.groups.io
> Cc: Albecki, Mateusz <mateusz.albecki@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Ni, Ray <ray.ni@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@intel.com>
> 
> Cc: Ray Ni <ray.ni@intel.com>
> 
> 
> Signed-off-by: Mateusz Albecki <mateusz.albecki@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