[PATCH 0/1] MdeModulePkg/Ahci: Skip retry for non-transient errors

[PATCH 0/1] MdeModulePkg/Ahci: Skip retry for non-transient errors

[Albecki, Mateusz]

Fix for the recovery logic which causes hdd unlock to fail if user supplies incorrect
password. Every failed packet used to be recovered which is causing the
incorrect password to be tried multiple times. This patch series fixes the logic
to only retry commands that failed due to CRC error.

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4011

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

Tests:
- tested basic linux boot from AHCI on qemu
- tested basic linux boot from AHCI on custom qemu which will fail 50% of the DMA commands with CRC error.
  Observed that all of the packets that failed were successfully retried. Custom Qemu: https://github.com/matalbec/qemu/tree/sata_dma_50p_fail
- additionally Hunter Chang tested and confirmed that the password issue is no longer observed.

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Hunter Chang <hunter.chang@intel.com>

Mateusz Albecki (1):
  MdeModulePkg/Ahci: Skip retry for non-transient errors

 .../Bus/Ata/AtaAtapiPassThru/AhciMode.c       | 69 +++++++++++++++++--
 .../Bus/Ata/AtaAtapiPassThru/AhciMode.h       |  3 +-
 2 files changed, 67 insertions(+), 5 deletions(-)

-- 
2.39.1.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/Ahci: Skip retry for non-transient errors

[Albecki, Mateusz]

bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=4011

Currently AHCI driver will try to retry all failed packets
regardless of the failure cause. This is a problem in password
unlock flow where number of password retries is tracked by the
device. If user passes a wrong password Ahci driver will try
to send the wrong password multiple times which will exhaust
number of password retries and force the user to restart the
machine. This commit introduces a logic to check for the cause
of packet failure and only retry packets which failed due to
transient conditions on the link. With this patch only packets for
which CRC error is flagged are retried.

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Hunter Chang <hunter.chang@intel.com>

Signed-off-by: Mateusz Albecki <mateusz.albecki@intel.com>
---
 .../Bus/Ata/AtaAtapiPassThru/AhciMode.c       | 69 +++++++++++++++++--
 .../Bus/Ata/AtaAtapiPassThru/AhciMode.h       |  3 +-
 2 files changed, 67 insertions(+), 5 deletions(-)

diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
index 06c4a3e052..90c9b4e69d 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
@@ -737,12 +737,66 @@ AhciRecoverPortError (
     Status = AhciResetPort (PciIo, Port);
     if (EFI_ERROR (Status)) {
       DEBUG ((DEBUG_ERROR, "Failed to reset the port %d\n", Port));
+      return EFI_DEVICE_ERROR;
     }
   }
 
   return EFI_SUCCESS;
 }
 
+/**
+  This function will check if the failed command should be retired. Only error
+  conditions which are a result of transient conditions on a link(either to system or to device).
+
+  @param[in] PciIo    Pointer to AHCI controller PciIo.
+  @param[in] Port     SATA port index on which to check.
+
+  @retval TRUE   Command failure was caused by transient condition and should be retried
+  @retval FALSE  Command should not be retried
+**/
+BOOLEAN
+AhciShouldCmdBeRetried (
+  IN EFI_PCI_IO_PROTOCOL *PciIo,
+  IN UINT8               Port
+  )
+{
+  UINT32      Offset;
+  UINT32      PortInterrupt;
+  UINT32      Serr;
+  UINT32      Tfd;
+
+  Offset        = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH + EFI_AHCI_PORT_IS;
+  PortInterrupt = AhciReadReg (PciIo, Offset);
+  Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH + EFI_AHCI_PORT_SERR;
+  Serr = AhciReadReg (PciIo, Offset);
+  Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH + EFI_AHCI_PORT_TFD;
+  Tfd = AhciReadReg (PciIo, Offset);
+
+  //
+  // This can occur if there was a CRC error on a path from system memory to
+  // host controller.
+  //
+  if (PortInterrupt & EFI_AHCI_PORT_IS_HBDS) {
+    return TRUE;
+  //
+  // This can occur if there was a CRC error detected by host during communication
+  // with the device
+  //
+  } else if ((PortInterrupt & (EFI_AHCI_PORT_IS_IFS | EFI_AHCI_PORT_IS_INFS)) &&
+             (Serr & EFI_AHCI_PORT_SERR_CRCE)) {
+    return TRUE;
+  //
+  // This can occur if there was a CRC error detected by device during communication
+  // with the host. Device returns error status to host with D2H FIS.
+  //
+  } else if ((PortInterrupt & EFI_AHCI_PORT_IS_TFES) &&
+             (Tfd & EFI_AHCI_PORT_TFD_ERR_INT_CRC)) {
+    return TRUE;
+  }
+
+  return FALSE;
+}
+
 /**
   Checks if specified FIS has been received.
 
@@ -950,6 +1004,7 @@ AhciPioTransfer (
   UINT32                         PrdCount;
   UINT32                         Retry;
   EFI_STATUS                     RecoveryStatus;
+  BOOLEAN                        DoRetry;
 
   if (Read) {
     Flag = EfiPciIoOperationBusMasterWrite;
@@ -1027,8 +1082,9 @@ AhciPioTransfer (
 
     if (Status == EFI_DEVICE_ERROR) {
       DEBUG ((DEBUG_ERROR, "PIO command failed at retry %d\n", Retry));
+      DoRetry = AhciShouldCmdBeRetried (PciIo, Port); // needs to be called before error recovery
       RecoveryStatus = AhciRecoverPortError (PciIo, Port);
-      if (EFI_ERROR (RecoveryStatus)) {
+      if (!DoRetry || EFI_ERROR (RecoveryStatus)) {
         break;
       }
     } else {
@@ -1124,6 +1180,7 @@ AhciDmaTransfer (
   EFI_TPL                        OldTpl;
   UINT32                         Retry;
   EFI_STATUS                     RecoveryStatus;
+  BOOLEAN                        DoRetry;
 
   Map   = NULL;
   PciIo = Instance->PciIo;
@@ -1222,8 +1279,9 @@ AhciDmaTransfer (
       Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout, SataFisD2H);
       if (Status == EFI_DEVICE_ERROR) {
         DEBUG ((DEBUG_ERROR, "DMA command failed at retry: %d\n", Retry));
+        DoRetry = AhciShouldCmdBeRetried (PciIo, Port); // needs to be called before error recovery
         RecoveryStatus = AhciRecoverPortError (PciIo, Port);
-        if (EFI_ERROR (RecoveryStatus)) {
+        if (!DoRetry || EFI_ERROR (RecoveryStatus)) {
           break;
         }
       } else {
@@ -1263,6 +1321,7 @@ AhciDmaTransfer (
       Status = AhciCheckFisReceived (PciIo, Port, SataFisD2H);
       if (Status == EFI_DEVICE_ERROR) {
         DEBUG ((DEBUG_ERROR, "DMA command failed at retry: %d\n", Task->RetryTimes));
+        DoRetry = AhciShouldCmdBeRetried (PciIo, Port); // call this before error recovery
         RecoveryStatus = AhciRecoverPortError (PciIo, Port);
         //
         // If recovery passed mark the Task as not started and change the status
@@ -1270,7 +1329,7 @@ AhciDmaTransfer (
         // 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 (RecoveryStatus == EFI_SUCCESS) {
+        if (DoRetry && RecoveryStatus == EFI_SUCCESS) {
           Task->IsStart = FALSE;
           Status        = EFI_NOT_READY;
         }
@@ -1378,6 +1437,7 @@ AhciNonDataTransfer (
   EFI_AHCI_COMMAND_LIST  CmdList;
   UINT32                 Retry;
   EFI_STATUS             RecoveryStatus;
+  BOOLEAN                DoRetry;
 
   //
   // Package read needed
@@ -1418,8 +1478,9 @@ AhciNonDataTransfer (
     Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout, SataFisD2H);
     if (Status == EFI_DEVICE_ERROR) {
       DEBUG ((DEBUG_ERROR, "Non data transfer failed at retry %d\n", Retry));
+      DoRetry = AhciShouldCmdBeRetried (PciIo, Port); // call this before error recovery
       RecoveryStatus = AhciRecoverPortError (PciIo, Port);
-      if (EFI_ERROR (RecoveryStatus)) {
+      if (!DoRetry || EFI_ERROR (RecoveryStatus)) {
         break;
       }
     } else {
diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
index d7434b408c..5bb31057ec 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
@@ -146,7 +146,8 @@ typedef union {
 #define   EFI_AHCI_PORT_TFD_BSY           BIT7
 #define   EFI_AHCI_PORT_TFD_DRQ           BIT3
 #define   EFI_AHCI_PORT_TFD_ERR           BIT0
-#define   EFI_AHCI_PORT_TFD_ERR_MASK      0x00FF00
+#define   EFI_AHCI_PORT_TFD_ERR_MASK      0x00FF00 // ERROR field is specified by ATA/ATAPI Command Set specification
+#define   EFI_AHCI_PORT_TFD_ERR_INT_CRC   BIT15
 #define EFI_AHCI_PORT_SIG                 0x0024
 #define EFI_AHCI_PORT_SSTS                0x0028
 #define   EFI_AHCI_PORT_SSTS_DET_MASK     0x000F
-- 
2.39.1.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 0/1] MdeModulePkg/Ahci: Skip retry for non-transient errors

[Wu, Hao A]

Thanks Mateusz, the patch looks good to me.
I noticed that there are some check failures in https://github.com/tianocore/edk2/pull/4157, could you help to address them?


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://patch-diff.githubusercontent.com/raw/tianocore/edk2/pull/4157.patch
Thanks in advance.

Best Regards,
Hao Wu

> -----Original Message-----
> From: Albecki, Mateusz <mateusz.albecki@intel.com>
> Sent: Wednesday, March 22, 2023 4:20 AM
> 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>; Chang, Hunter
> <hunter.chang@intel.com>
> Subject: [PATCH 0/1] MdeModulePkg/Ahci: Skip retry for non-transient errors
> 
> Fix for the recovery logic which causes hdd unlock to fail if user supplies
> incorrect password. Every failed packet used to be recovered which is causing
> the incorrect password to be tried multiple times. This patch series fixes the
> logic to only retry commands that failed due to CRC error.
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4011
> 
> Github pull: https://github.com/tianocore/edk2/pull/4157
> 
> Tests:
> - tested basic linux boot from AHCI on qemu
> - tested basic linux boot from AHCI on custom qemu which will fail 50% of the
> DMA commands with CRC error.
>   Observed that all of the packets that failed were successfully retried. Custom
> Qemu: https://github.com/matalbec/qemu/tree/sata_dma_50p_fail
> - additionally Hunter Chang tested and confirmed that the password issue is no
> longer observed.
> 
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Hunter Chang <hunter.chang@intel.com>
> 
> Mateusz Albecki (1):
>   MdeModulePkg/Ahci: Skip retry for non-transient errors
> 
>  .../Bus/Ata/AtaAtapiPassThru/AhciMode.c       | 69 +++++++++++++++++--
>  .../Bus/Ata/AtaAtapiPassThru/AhciMode.h       |  3 +-
>  2 files changed, 67 insertions(+), 5 deletions(-)
> 
> --
> 2.39.1.windows.1

Re: [edk2-devel] [PATCH 0/1] MdeModulePkg/Ahci: Skip retry for non-transient errors

[Albecki, Mateusz]

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

Seems like a code formatting issue. I will address it tomorrow.

Regards,
Mateusz

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

Re: [edk2-devel] [PATCH 0/1] MdeModulePkg/Ahci: Skip retry for non-transient errors

[Anbazhagan, Baraneedharan]

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

Hi,
This patch seems to resolve the issue reported in 4011 - MdeModulePkg: Need configurable AHCI command retries (tianocore.org)<https://bugzilla.tianocore.org/show_bug.cgi?id=4011> and verified with 'AHCI_COMMAND_RETRIES' as 5. Able to unlock the drive with correct password on 2nd attempt after providing an incorrect password.

Thanks,
Baranee

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao A via groups.io
Sent: Wednesday, March 22, 2023 1:59 AM
To: Albecki, Mateusz <mateusz.albecki@intel.com>; Anbazhagan, Baraneedharan <anbazhagan@hp.com>; devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@intel.com>; Chang, Hunter <hunter.chang@intel.com>
Subject: Re: [edk2-devel] [PATCH 0/1] MdeModulePkg/Ahci: Skip retry for non-transient errors

CAUTION: External Email
Thanks Mateusz, the patch looks good to me.
I noticed that there are some check failures in https://github.com/tianocore/edk2/pull/4157<https://github.com/tianocore/edk2/pull/4157>, could you help to address them?


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://patch-diff.githubusercontent.com/raw/tianocore/edk2/pull/4157.patch<https://patch-diff.githubusercontent.com/raw/tianocore/edk2/pull/4157.patch>
Thanks in advance.

Best Regards,
Hao Wu

> -----Original Message-----
> From: Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>
> Sent: Wednesday, March 22, 2023 4:20 AM
> 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>>; Chang, Hunter
> <hunter.chang@intel.com<mailto:hunter.chang@intel.com>>
> Subject: [PATCH 0/1] MdeModulePkg/Ahci: Skip retry for non-transient errors
>
> Fix for the recovery logic which causes hdd unlock to fail if user supplies
> incorrect password. Every failed packet used to be recovered which is causing
> the incorrect password to be tried multiple times. This patch series fixes the
> logic to only retry commands that failed due to CRC error.
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4011<https://bugzilla.tianocore.org/show_bug.cgi?id=4011>
>
> Github pull: https://github.com/tianocore/edk2/pull/4157<https://github.com/tianocore/edk2/pull/4157>
>
> Tests:
> - tested basic linux boot from AHCI on qemu
> - tested basic linux boot from AHCI on custom qemu which will fail 50% of the
> DMA commands with CRC error.
> Observed that all of the packets that failed were successfully retried. Custom
> Qemu: https://github.com/matalbec/qemu/tree/sata_dma_50p_fail<https://github.com/matalbec/qemu/tree/sata_dma_50p_fail>
> - additionally Hunter Chang tested and confirmed that the password issue is no
> longer observed.
>
> 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>>
> Cc: Hunter Chang <hunter.chang@intel.com<mailto:hunter.chang@intel.com>>
>
> Mateusz Albecki (1):
> MdeModulePkg/Ahci: Skip retry for non-transient errors
>
> .../Bus/Ata/AtaAtapiPassThru/AhciMode.c | 69 +++++++++++++++++--
> .../Bus/Ata/AtaAtapiPassThru/AhciMode.h | 3 +-
> 2 files changed, 67 insertions(+), 5 deletions(-)
>
> --
> 2.39.1.windows.1





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

Re: [edk2-devel] [PATCH 0/1] MdeModulePkg/Ahci: Skip retry for non-transient errors

[Wu, Hao A]

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

Thanks Baranee. We will proceed to merge the this patch after reviewing process.

For the PCD (PcdAhciCommandRetryCount) previously introduced to address the password retry issue, what is your opinion on it?
Do you think we can remove it or keep it for other reason? Thanks.

Best Regards,
Hao Wu

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Anbazhagan, Baraneedharan via groups.io
Sent: Friday, March 24, 2023 12:55 AM
To: devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Albecki, Mateusz <mateusz.albecki@intel.com>
Cc: Ni, Ray <ray.ni@intel.com>; Chang, Hunter <hunter.chang@intel.com>
Subject: Re: [edk2-devel] [PATCH 0/1] MdeModulePkg/Ahci: Skip retry for non-transient errors

Hi,
This patch seems to resolve the issue reported in 4011 - MdeModulePkg: Need configurable AHCI command retries (tianocore.org)<https://bugzilla.tianocore.org/show_bug.cgi?id=4011> and verified with 'AHCI_COMMAND_RETRIES' as 5. Able to unlock the drive with correct password on 2nd attempt after providing an incorrect password.

Thanks,
Baranee

From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Wu, Hao A via groups.io
Sent: Wednesday, March 22, 2023 1:59 AM
To: Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Anbazhagan, Baraneedharan <anbazhagan@hp.com<mailto:anbazhagan@hp.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Chang, Hunter <hunter.chang@intel.com<mailto:hunter.chang@intel.com>>
Subject: Re: [edk2-devel] [PATCH 0/1] MdeModulePkg/Ahci: Skip retry for non-transient errors

CAUTION: External Email
Thanks Mateusz, the patch looks good to me.
I noticed that there are some check failures in https://github.com/tianocore/edk2/pull/4157, could you help to address them?


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://patch-diff.githubusercontent.com/raw/tianocore/edk2/pull/4157.patch
Thanks in advance.

Best Regards,
Hao Wu

> -----Original Message-----
> From: Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>
> Sent: Wednesday, March 22, 2023 4:20 AM
> 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>>; Chang, Hunter
> <hunter.chang@intel.com<mailto:hunter.chang@intel.com>>
> Subject: [PATCH 0/1] MdeModulePkg/Ahci: Skip retry for non-transient errors
>
> Fix for the recovery logic which causes hdd unlock to fail if user supplies
> incorrect password. Every failed packet used to be recovered which is causing
> the incorrect password to be tried multiple times. This patch series fixes the
> logic to only retry commands that failed due to CRC error.
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4011
>
> Github pull: https://github.com/tianocore/edk2/pull/4157
>
> Tests:
> - tested basic linux boot from AHCI on qemu
> - tested basic linux boot from AHCI on custom qemu which will fail 50% of the
> DMA commands with CRC error.
> Observed that all of the packets that failed were successfully retried. Custom
> Qemu: https://github.com/matalbec/qemu/tree/sata_dma_50p_fail
> - additionally Hunter Chang tested and confirmed that the password issue is no
> longer observed.
>
> 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>>
> Cc: Hunter Chang <hunter.chang@intel.com<mailto:hunter.chang@intel.com>>
>
> Mateusz Albecki (1):
> MdeModulePkg/Ahci: Skip retry for non-transient errors
>
> .../Bus/Ata/AtaAtapiPassThru/AhciMode.c | 69 +++++++++++++++++--
> .../Bus/Ata/AtaAtapiPassThru/AhciMode.h | 3 +-
> 2 files changed, 67 insertions(+), 5 deletions(-)
>
> --
> 2.39.1.windows.1




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

Re: [edk2-devel] [PATCH 0/1] MdeModulePkg/Ahci: Skip retry for non-transient errors

[Anbazhagan, Baraneedharan]

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

It's OK to remove the PCD since password issue is fixed in different way. On other hand, PCD allows the platform to determine the retries independent of specific command. Am OK with either approach on this one currently. Thanks.

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao A via groups.io
Sent: Thursday, March 23, 2023 8:14 PM
To: Anbazhagan, Baraneedharan <anbazhagan@hp.com>; devel@edk2.groups.io; Albecki, Mateusz <mateusz.albecki@intel.com>
Cc: Ni, Ray <ray.ni@intel.com>; Chang, Hunter <hunter.chang@intel.com>
Subject: Re: [edk2-devel] [PATCH 0/1] MdeModulePkg/Ahci: Skip retry for non-transient errors

CAUTION: External Email
Thanks Baranee. We will proceed to merge the this patch after reviewing process.

For the PCD (PcdAhciCommandRetryCount) previously introduced to address the password retry issue, what is your opinion on it?
Do you think we can remove it or keep it for other reason? Thanks.

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 Anbazhagan, Baraneedharan via groups.io
Sent: Friday, March 24, 2023 12:55 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>
Cc: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Chang, Hunter <hunter.chang@intel.com<mailto:hunter.chang@intel.com>>
Subject: Re: [edk2-devel] [PATCH 0/1] MdeModulePkg/Ahci: Skip retry for non-transient errors

Hi,
This patch seems to resolve the issue reported in 4011 - MdeModulePkg: Need configurable AHCI command retries (tianocore.org)<https://bugzilla.tianocore.org/show_bug.cgi?id=4011> and verified with 'AHCI_COMMAND_RETRIES' as 5. Able to unlock the drive with correct password on 2nd attempt after providing an incorrect password.

Thanks,
Baranee

From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Wu, Hao A via groups.io
Sent: Wednesday, March 22, 2023 1:59 AM
To: Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Anbazhagan, Baraneedharan <anbazhagan@hp.com<mailto:anbazhagan@hp.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Chang, Hunter <hunter.chang@intel.com<mailto:hunter.chang@intel.com>>
Subject: Re: [edk2-devel] [PATCH 0/1] MdeModulePkg/Ahci: Skip retry for non-transient errors

CAUTION: External Email
Thanks Mateusz, the patch looks good to me.
I noticed that there are some check failures in https://github.com/tianocore/edk2/pull/4157<https://github.com/tianocore/edk2/pull/4157>, could you help to address them?


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://patch-diff.githubusercontent.com/raw/tianocore/edk2/pull/4157.patch<https://patch-diff.githubusercontent.com/raw/tianocore/edk2/pull/4157.patch>
Thanks in advance.

Best Regards,
Hao Wu

> -----Original Message-----
> From: Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>
> Sent: Wednesday, March 22, 2023 4:20 AM
> 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>>; Chang, Hunter
> <hunter.chang@intel.com<mailto:hunter.chang@intel.com>>
> Subject: [PATCH 0/1] MdeModulePkg/Ahci: Skip retry for non-transient errors
>
> Fix for the recovery logic which causes hdd unlock to fail if user supplies
> incorrect password. Every failed packet used to be recovered which is causing
> the incorrect password to be tried multiple times. This patch series fixes the
> logic to only retry commands that failed due to CRC error.
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4011<https://bugzilla.tianocore.org/show_bug.cgi?id=4011>
>
> Github pull: https://github.com/tianocore/edk2/pull/4157<https://github.com/tianocore/edk2/pull/4157>
>
> Tests:
> - tested basic linux boot from AHCI on qemu
> - tested basic linux boot from AHCI on custom qemu which will fail 50% of the
> DMA commands with CRC error.
> Observed that all of the packets that failed were successfully retried. Custom
> Qemu: https://github.com/matalbec/qemu/tree/sata_dma_50p_fail<https://github.com/matalbec/qemu/tree/sata_dma_50p_fail>
> - additionally Hunter Chang tested and confirmed that the password issue is no
> longer observed.
>
> 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>>
> Cc: Hunter Chang <hunter.chang@intel.com<mailto:hunter.chang@intel.com>>
>
> Mateusz Albecki (1):
> MdeModulePkg/Ahci: Skip retry for non-transient errors
>
> .../Bus/Ata/AtaAtapiPassThru/AhciMode.c | 69 +++++++++++++++++--
> .../Bus/Ata/AtaAtapiPassThru/AhciMode.h | 3 +-
> 2 files changed, 67 insertions(+), 5 deletions(-)
>
> --
> 2.39.1.windows.1



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