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

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

[Albecki, Mateusz]

Fix for a bug introduced during SATA recovery implementation

Failing SCT: https://bugzilla.tianocore.org/show_bug.cgi?id=4016
Failing password input: https://bugzilla.tianocore.org/show_bug.cgi?id=4011

The bug was due to recovery status aliasing with command status. This patch should resolve both
issues and should remove the need for reconfigurable retries. I have only tested SCT test case
though.

Github pull: https://github.com/tianocore/edk2/pull/3495

Tests:
- tested on qemu with ovmf - SCT test now passes (custom qemu needed from: https://github.com/matalbec/qemu/tree/sata_abort_completion)
- it should be noted that qemu doesn't really support trust protocol on SATA drive but for the purposes of the test it should be enough.

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>

Mateusz Albecki (1):
  MdeModulePkg/Ata: Fix command status reporting

 .../Bus/Ata/AtaAtapiPassThru/AhciMode.c       | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

-- 
2.28.0.windows.1

---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
Spolka oswiadcza, ze posiada status duzego przedsiebiorcy w rozumieniu ustawy z dnia 8 marca 2013 r. o przeciwdzialaniu nadmiernym opoznieniom w transakcjach handlowych.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.

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

[Albecki, Mateusz]

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

---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
Spolka oswiadcza, ze posiada status duzego przedsiebiorcy w rozumieniu ustawy z dnia 8 marca 2013 r. o przeciwdzialaniu nadmiernym opoznieniom w transakcjach handlowych.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.

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

[Wu, Hao A]

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

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

[Wu, Hao A]

Hello Mateusz,

Thanks for the patch, I think the patch itself is good.

Sorry for one question came up when looking into the patch, for the implementation of AhciRecoverPortError()
https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c#L737:
    Status = AhciResetPort (PciIo, Port);
    if (EFI_ERROR (Status)) {
      DEBUG ((DEBUG_ERROR, "Failed to reset the port %d\n", Port));
    }
I cannot remember why EFI_SUCCESS is eventually returned for the above error case. Could you help to remind me of the details? Thanks.

Best Regards,
Hao Wu

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao A
> Sent: Wednesday, October 19, 2022 9:37 AM
> To: Anbazhagan, Baraneedharan <anbazhagan@hp.com>; Albecki, Mateusz
> <mateusz.albecki@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg/Ata: Fix command status
> reporting
> 
> 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/f2eb27e00ed55cca03964f13a2839
> e41e2c1f61f.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
> 
> 
> 
> 
> 

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

[Anbazhagan, Baraneedharan]

[-- Attachment #1: Type: text/plain, Size: 5359 bytes --]

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@intel.com>
Sent: Tuesday, October 18, 2022 8:37 PM
To: Anbazhagan, Baraneedharan <anbazhagan@hp.com>; Albecki, Mateusz <mateusz.albecki@intel.com>; devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@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<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<https://github.com/tianocore/edk2/commit/f2eb27e00ed55cca03964f13a2839e41e2c1f61f.patch>

Best Regards,
Hao Wu

> -----Original Message-----
> From: Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>
> Sent: Tuesday, October 18, 2022 11:54 PM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> Cc: Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Wu, Hao A
> <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>
> Subject: [PATCH 1/1] MdeModulePkg/Ata: Fix command status reporting
>
> Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=4016<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<mailto:hao.a.wu@intel.com>>
>
> Cc: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com>>
>
>
> Signed-off-by: Mateusz Albecki <mateusz.albecki@intel.com<mailto: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

[-- Attachment #2: Type: text/html, Size: 8969 bytes --]

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

[Wu, Hao A]

[-- Attachment #1: Type: text/plain, Size: 5750 bytes --]

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

Best Regards,
Hao Wu

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


[-- Attachment #2: Type: text/html, Size: 10483 bytes --]

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

[Albecki, Mateusz]

[-- Attachment #1: Type: text/plain, Size: 1534 bytes --]

Hello,

Really sorry for missing the mails. Seems like my mail was misconfigured and I didn't get the messages(had to check the group site).

"I cannot remember why EFI_SUCCESS is eventually returned for the above error case. Could you help to remind me of the details? Thanks."

To answer this - I don't remember either, and that's potentially a bug. Failing to reset the device should render the device unusable and we probably should probably treat it as a hot unplug and block further packets on that port. This would require bigger change. Let me know if you want me to simply add return EFI_DEVICE_ERROR there to stop additional retries.

As for the patch failing to fix the https://bugzilla.tianocore.org/show_bug.cgi?id=4011 it seems like I misunderstood the issue. My initial understanding was that we have the issue since we return success even when the password failed in following flow:

1. Incorrect password is supplied
2. Driver tries for 5 times and it fails 5 times
3. Driver returns success(bug that this patch is supposed to fix)
4. Higher level SW thinks the password is ok but in reality it wasn't(bug)

Seems like step 2 is also causing issues and we can't retry password 5 times. This will require driver to check for the error type and only retry on specific errors like CRC. Let me investigate more what can we check and what kinds of errors should be retried and come back to this thread. In the meantime I still think we can submit the change to solve failing SCT tests.

Thanks,
Mateusz

[-- Attachment #2: Type: text/html, Size: 1860 bytes --]

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

[Wu, Hao A]

[-- Attachment #1: Type: text/plain, Size: 2447 bytes --]

Thanks.

For the patch:
Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
Will merge the patch early next week.

For the open with regard to: https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c#L737
I think returning EFI_DEVICE_ERROR when something goes wrong in AhciResetPort() should be enough.
We can address this in another patch.

For retry of incorrect password, I agree with you that it would be better to distinguish such case from other failures that deserve retries.
We can improve this with a separate patch in the future as well.

Best Regards,
Hao Wu

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Albecki, Mateusz
Sent: Thursday, December 8, 2022 12:25 AM
To: Albecki; Albecki, Mateusz <mateusz.albecki@intel.com>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg/Ata: Fix command status reporting

Hello,

Really sorry for missing the mails. Seems like my mail was misconfigured and I didn't get the messages(had to check the group site).

"I cannot remember why EFI_SUCCESS is eventually returned for the above error case. Could you help to remind me of the details? Thanks."

To answer this - I don't remember either, and that's potentially a bug. Failing to reset the device should render the device unusable and we probably should probably treat it as a hot unplug and block further packets on that port. This would require bigger change. Let me know if you want me to simply add return EFI_DEVICE_ERROR there to stop additional retries.

As for the patch failing to fix the  https://bugzilla.tianocore.org/show_bug.cgi?id=4011 it seems like I misunderstood the issue. My initial understanding was that we have the issue since we return success even when the password failed in following flow:

1. Incorrect password is supplied
2. Driver tries for 5 times and it fails 5 times
3. Driver returns success(bug that this patch is supposed to fix)
4. Higher level SW thinks the password is ok but in reality it wasn't(bug)

Seems like step 2 is also causing issues and we can't retry password 5 times. This will require driver to check for the error type and only retry on specific errors like CRC. Let me investigate more what can we check and what kinds of errors should be retried and come back to this thread. In the meantime I still think we can submit the change to solve failing SCT tests.

Thanks,
Mateusz


[-- Attachment #2: Type: text/html, Size: 5670 bytes --]

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

[Wu, Hao A]

[-- Attachment #1: Type: text/plain, Size: 3054 bytes --]

Pushed via:
PR - https://github.com/tianocore/edk2/pull/3746
Commit - https://github.com/tianocore/edk2/commit/a6542894391bae58b7704b2624b541a2b8b9ed93

Best Regards,
Hao Wu

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao A
Sent: Thursday, December 8, 2022 12:21 PM
To: devel@edk2.groups.io; Albecki, Mateusz <mateusz.albecki@intel.com>
Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg/Ata: Fix command status reporting

Thanks.

For the patch:
Reviewed-by: Hao A Wu <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>
Will merge the patch early next week.

For the open with regard to: https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c#L737
I think returning EFI_DEVICE_ERROR when something goes wrong in AhciResetPort() should be enough.
We can address this in another patch.

For retry of incorrect password, I agree with you that it would be better to distinguish such case from other failures that deserve retries.
We can improve this with a separate patch in the future as well.

Best Regards,
Hao Wu

From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Albecki, Mateusz
Sent: Thursday, December 8, 2022 12:25 AM
To: Albecki; Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg/Ata: Fix command status reporting

Hello,

Really sorry for missing the mails. Seems like my mail was misconfigured and I didn't get the messages(had to check the group site).

"I cannot remember why EFI_SUCCESS is eventually returned for the above error case. Could you help to remind me of the details? Thanks."

To answer this - I don't remember either, and that's potentially a bug. Failing to reset the device should render the device unusable and we probably should probably treat it as a hot unplug and block further packets on that port. This would require bigger change. Let me know if you want me to simply add return EFI_DEVICE_ERROR there to stop additional retries.

As for the patch failing to fix the  https://bugzilla.tianocore.org/show_bug.cgi?id=4011 it seems like I misunderstood the issue. My initial understanding was that we have the issue since we return success even when the password failed in following flow:

1. Incorrect password is supplied
2. Driver tries for 5 times and it fails 5 times
3. Driver returns success(bug that this patch is supposed to fix)
4. Higher level SW thinks the password is ok but in reality it wasn't(bug)

Seems like step 2 is also causing issues and we can't retry password 5 times. This will require driver to check for the error type and only retry on specific errors like CRC. Let me investigate more what can we check and what kinds of errors should be retried and come back to this thread. In the meantime I still think we can submit the change to solve failing SCT tests.

Thanks,
Mateusz


[-- Attachment #2: Type: text/html, Size: 6985 bytes --]

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

[Albecki, Mateusz]

[-- Attachment #1: Type: text/plain, Size: 3088 bytes --]

Hello,

I've done some investigation into the password issue and I think I have the proposition on how to solve it. The issue arises due to the driver checking the PxIS.TFES(task file error status) and if TFES is set it performs recovery steps(this is correct according to AHCI spec 6.2.2) and restarts the command(this is left up to the SW according to AHCI spec). In the case of SECURITY UNLOCK command if the password is incorrect device will set the abort bit in ERROR register and error bit in status register which will cause the controller to set the PxIS.TFES bit which will  trigger command retires. In case of security commands retries are particularly bad since every incorrect password unlock attempt is decreasing the retry counter on the device side which eventually leads to device locking itself and aborting further unlock commands(power cycle is required to recover).

My proposition for a fix is as follows:

1. The logic in AhciRecoverPortError seems to be largely correct. Even if the TFES error bit shouldn't necessarily mean that the command should be retried, according to AHCI spec when TFES is set controller will enter the ERR:Fatal state and PxCMD.CR needs to be restarted. The only thing that we need to change in this function is to return EFI_DEVICE_ERROR when the port restart failed(as discussed below).
2. We need to add a logic which will decide if the cmd should be retried. This logic should check following conditions:

a) If HBDS is set command should be attempted again. HBDS indicates that there was a CRC error during accessing the system memory during DMA transfer(AHCI spec 6.1.1).
b) If HBFS is set do not attempt to send the command again. HBFS indicates that the pointer given to the AHCI controller is incorrect. Retries won't solve that(AHCI spec 6.1.1)
c) If IFS or INFS is set further check PxSERR.ERR and PxSERR.DIAG(based on AHCI spec 6.1.2).
i) for PxSERR.DIAG.C(CRC error) -> restart command
ii) for PxSERR.DIAG.B(disparity error) -> CRC error should also be reported so it is covered by above
iii) for PxSERR.ERR.P(protocol error) -> do not restart. Protocol errors are unlikely to be transient although I am not sure if Internal buffer overflows can't be caused by transient conditions on the devices side.
iv) for PxSERR.DIAG.H(handshake errors) -> do not restart. Acording to spec handshake errors might be caused by transient conditions such as CRC errors but I hope in such case HBA will also signal CRC in PxSERR.DIAG.C(spec is not super clear on that point though)
d) If TFES is set further check PxTFD.ERR if PxTFD.ERR bit 7 is set(INTERFACE CRC according to ACS-3 spec). For others do not retry.

Hopefully this will catch majority of errors caused by transient conditions while also avoiding restarts on commands that are simply malformed. I will start working on a patch and if anyone has any suggestions/objections please respond.

I will also update bz with this data: https://bugzilla.tianocore.org/show_bug.cgi?id=4011 ( https://bugzilla.tianocore.org/show_bug.cgi?id=4011 )

Thanks,
Mateusz

[-- Attachment #2: Type: text/html, Size: 3737 bytes --]