* [PATCHv2 0/1] MdeModulePkg/Ahci: Skip retry for non-transient errors
@ 2023-03-27 21:37 Albecki, Mateusz
2023-03-27 21:37 ` [PATCHv2 1/1] " Albecki, Mateusz
0 siblings, 1 reply; 5+ messages in thread
From: Albecki, Mateusz @ 2023-03-27 21:37 UTC (permalink / raw)
To: devel
Cc: Mateusz Albecki, Hao A Wu, Ray Ni, Hunter Chang,
Baraneedharan Anbazhagan
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.
Changes in V2:
- Fixed code formatting issues
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Hunter Chang <hunter.chang@intel.com>
Cc: Baraneedharan Anbazhagan <anbazhagan@hp.com>
Mateusz Albecki (1):
MdeModulePkg/Ahci: Skip retry for non-transient errors
.../Bus/Ata/AtaAtapiPassThru/AhciMode.c | 71 +++++++++++++++++--
.../Bus/Ata/AtaAtapiPassThru/AhciMode.h | 3 +-
2 files changed, 69 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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCHv2 1/1] MdeModulePkg/Ahci: Skip retry for non-transient errors
2023-03-27 21:37 [PATCHv2 0/1] MdeModulePkg/Ahci: Skip retry for non-transient errors Albecki, Mateusz
@ 2023-03-27 21:37 ` Albecki, Mateusz
2023-03-28 2:03 ` Wu, Hao A
0 siblings, 1 reply; 5+ messages in thread
From: Albecki, Mateusz @ 2023-03-27 21:37 UTC (permalink / raw)
To: devel
Cc: Mateusz Albecki, Hao A Wu, Ray Ni, Hunter Chang,
Baraneedharan Anbazhagan
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>
Cc: Baraneedharan Anbazhagan <anbazhagan@hp.com>
Signed-off-by: Mateusz Albecki <mateusz.albecki@intel.com>
---
.../Bus/Ata/AtaAtapiPassThru/AhciMode.c | 71 +++++++++++++++++--
.../Bus/Ata/AtaAtapiPassThru/AhciMode.h | 3 +-
2 files changed, 69 insertions(+), 5 deletions(-)
diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
index 06c4a3e052..c0c8ffbd9e 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
@@ -737,12 +737,68 @@ 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 +1006,7 @@ AhciPioTransfer (
UINT32 PrdCount;
UINT32 Retry;
EFI_STATUS RecoveryStatus;
+ BOOLEAN DoRetry;
if (Read) {
Flag = EfiPciIoOperationBusMasterWrite;
@@ -1027,8 +1084,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 +1182,7 @@ AhciDmaTransfer (
EFI_TPL OldTpl;
UINT32 Retry;
EFI_STATUS RecoveryStatus;
+ BOOLEAN DoRetry;
Map = NULL;
PciIo = Instance->PciIo;
@@ -1222,8 +1281,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 +1323,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 +1331,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 +1439,7 @@ AhciNonDataTransfer (
EFI_AHCI_COMMAND_LIST CmdList;
UINT32 Retry;
EFI_STATUS RecoveryStatus;
+ BOOLEAN DoRetry;
//
// Package read needed
@@ -1418,8 +1480,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.
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCHv2 1/1] MdeModulePkg/Ahci: Skip retry for non-transient errors
2023-03-27 21:37 ` [PATCHv2 1/1] " Albecki, Mateusz
@ 2023-03-28 2:03 ` Wu, Hao A
2023-03-29 2:30 ` [edk2-devel] " Anbazhagan, Baraneedharan
0 siblings, 1 reply; 5+ messages in thread
From: Wu, Hao A @ 2023-03-28 2:03 UTC (permalink / raw)
To: Albecki, Mateusz, devel@edk2.groups.io
Cc: Ni, Ray, Chang, Hunter, Anbazhagan, Baraneedharan
Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
Will wait a couple of days before merging to see if comments from other reviewers.
Best Regards,
Hao Wu
> -----Original Message-----
> From: Albecki, Mateusz <mateusz.albecki@intel.com>
> Sent: Tuesday, March 28, 2023 5:38 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>; Anbazhagan, Baraneedharan
> <anbazhagan@hp.com>
> Subject: [PATCHv2 1/1] MdeModulePkg/Ahci: Skip retry for non-transient errors
>
> 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>
>
> Cc: Baraneedharan Anbazhagan <anbazhagan@hp.com>
>
>
>
> Signed-off-by: Mateusz Albecki <mateusz.albecki@intel.com>
>
> ---
>
> .../Bus/Ata/AtaAtapiPassThru/AhciMode.c | 71 +++++++++++++++++--
>
> .../Bus/Ata/AtaAtapiPassThru/AhciMode.h | 3 +-
>
> 2 files changed, 69 insertions(+), 5 deletions(-)
>
>
>
> diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
>
> index 06c4a3e052..c0c8ffbd9e 100644
>
> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
>
> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
>
> @@ -737,12 +737,68 @@ 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 +1006,7 @@ AhciPioTransfer (
>
> UINT32 PrdCount;
>
> UINT32 Retry;
>
> EFI_STATUS RecoveryStatus;
>
> + BOOLEAN DoRetry;
>
>
>
> if (Read) {
>
> Flag = EfiPciIoOperationBusMasterWrite;
>
> @@ -1027,8 +1084,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 +1182,7 @@ AhciDmaTransfer (
>
> EFI_TPL OldTpl;
>
> UINT32 Retry;
>
> EFI_STATUS RecoveryStatus;
>
> + BOOLEAN DoRetry;
>
>
>
> Map = NULL;
>
> PciIo = Instance->PciIo;
>
> @@ -1222,8 +1281,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 +1323,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 +1331,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 +1439,7 @@ AhciNonDataTransfer (
>
> EFI_AHCI_COMMAND_LIST CmdList;
>
> UINT32 Retry;
>
> EFI_STATUS RecoveryStatus;
>
> + BOOLEAN DoRetry;
>
>
>
> //
>
> // Package read needed
>
> @@ -1418,8 +1480,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
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [edk2-devel] [PATCHv2 1/1] MdeModulePkg/Ahci: Skip retry for non-transient errors
2023-03-28 2:03 ` Wu, Hao A
@ 2023-03-29 2:30 ` Anbazhagan, Baraneedharan
2023-04-03 1:43 ` Wu, Hao A
0 siblings, 1 reply; 5+ messages in thread
From: Anbazhagan, Baraneedharan @ 2023-03-29 2:30 UTC (permalink / raw)
To: devel@edk2.groups.io, hao.a.wu@intel.com, Albecki, Mateusz
Cc: Ni, Ray, Chang, Hunter
[-- Attachment #1: Type: text/plain, Size: 9132 bytes --]
Reviewed-by: Baraneedharan Anbazhagan <anbazhagan@hp.com<mailto:anbazhagan@hp.com>>
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao A via groups.io
Sent: Monday, March 27, 2023 9:03 PM
To: Albecki, Mateusz <mateusz.albecki@intel.com>; devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@intel.com>; Chang, Hunter <hunter.chang@intel.com>; Anbazhagan, Baraneedharan <anbazhagan@hp.com>
Subject: Re: [edk2-devel] [PATCHv2 1/1] MdeModulePkg/Ahci: Skip retry for non-transient errors
Reviewed-by: Hao A Wu <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>
Will wait a couple of days before merging to see if comments from other reviewers.
Best Regards,
Hao Wu
> -----Original Message-----
> From: Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>
> Sent: Tuesday, March 28, 2023 5:38 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>>; Anbazhagan, Baraneedharan
> <anbazhagan@hp.com<mailto:anbazhagan@hp.com>>
> Subject: [PATCHv2 1/1] MdeModulePkg/Ahci: Skip retry for non-transient errors
>
> 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<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>>
>
> Cc: Baraneedharan Anbazhagan <anbazhagan@hp.com<mailto:anbazhagan@hp.com>>
>
>
>
> Signed-off-by: Mateusz Albecki <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>
>
> ---
>
> .../Bus/Ata/AtaAtapiPassThru/AhciMode.c | 71 +++++++++++++++++--
>
> .../Bus/Ata/AtaAtapiPassThru/AhciMode.h | 3 +-
>
> 2 files changed, 69 insertions(+), 5 deletions(-)
>
>
>
> diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
>
> index 06c4a3e052..c0c8ffbd9e 100644
>
> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
>
> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
>
> @@ -737,12 +737,68 @@ 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 +1006,7 @@ AhciPioTransfer (
>
> UINT32 PrdCount;
>
> UINT32 Retry;
>
> EFI_STATUS RecoveryStatus;
>
> + BOOLEAN DoRetry;
>
>
>
> if (Read) {
>
> Flag = EfiPciIoOperationBusMasterWrite;
>
> @@ -1027,8 +1084,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 +1182,7 @@ AhciDmaTransfer (
>
> EFI_TPL OldTpl;
>
> UINT32 Retry;
>
> EFI_STATUS RecoveryStatus;
>
> + BOOLEAN DoRetry;
>
>
>
> Map = NULL;
>
> PciIo = Instance->PciIo;
>
> @@ -1222,8 +1281,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 +1323,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 +1331,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 +1439,7 @@ AhciNonDataTransfer (
>
> EFI_AHCI_COMMAND_LIST CmdList;
>
> UINT32 Retry;
>
> EFI_STATUS RecoveryStatus;
>
> + BOOLEAN DoRetry;
>
>
>
> //
>
> // Package read needed
>
> @@ -1418,8 +1480,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
>
>
[-- Attachment #2: Type: text/html, Size: 14429 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [edk2-devel] [PATCHv2 1/1] MdeModulePkg/Ahci: Skip retry for non-transient errors
2023-03-29 2:30 ` [edk2-devel] " Anbazhagan, Baraneedharan
@ 2023-04-03 1:43 ` Wu, Hao A
0 siblings, 0 replies; 5+ messages in thread
From: Wu, Hao A @ 2023-04-03 1:43 UTC (permalink / raw)
To: Anbazhagan, Baraneedharan, devel@edk2.groups.io, Albecki, Mateusz
Cc: Ni, Ray, Chang, Hunter
[-- Attachment #1: Type: text/plain, Size: 9892 bytes --]
Patch merged via:
PR - https://github.com/tianocore/edk2/pull/4207
Commit - https://github.com/tianocore/edk2/commit/eb6a74827200eedc81b8f45f332d6e9f3b3d2906
Best Regards,
Hao Wu
From: Anbazhagan, Baraneedharan <anbazhagan@hp.com>
Sent: Wednesday, March 29, 2023 10:30 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] [PATCHv2 1/1] MdeModulePkg/Ahci: Skip retry for non-transient errors
Reviewed-by: Baraneedharan Anbazhagan <anbazhagan@hp.com<mailto:anbazhagan@hp.com>>
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: Monday, March 27, 2023 9:03 PM
To: 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>>; Chang, Hunter <hunter.chang@intel.com<mailto:hunter.chang@intel.com>>; Anbazhagan, Baraneedharan <anbazhagan@hp.com<mailto:anbazhagan@hp.com>>
Subject: Re: [edk2-devel] [PATCHv2 1/1] MdeModulePkg/Ahci: Skip retry for non-transient errors
Reviewed-by: Hao A Wu <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>
Will wait a couple of days before merging to see if comments from other reviewers.
Best Regards,
Hao Wu
> -----Original Message-----
> From: Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>
> Sent: Tuesday, March 28, 2023 5:38 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>>; Anbazhagan, Baraneedharan
> <anbazhagan@hp.com<mailto:anbazhagan@hp.com>>
> Subject: [PATCHv2 1/1] MdeModulePkg/Ahci: Skip retry for non-transient errors
>
> 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<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>>
>
> Cc: Baraneedharan Anbazhagan <anbazhagan@hp.com<mailto:anbazhagan@hp.com>>
>
>
>
> Signed-off-by: Mateusz Albecki <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>
>
> ---
>
> .../Bus/Ata/AtaAtapiPassThru/AhciMode.c | 71 +++++++++++++++++--
>
> .../Bus/Ata/AtaAtapiPassThru/AhciMode.h | 3 +-
>
> 2 files changed, 69 insertions(+), 5 deletions(-)
>
>
>
> diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
>
> index 06c4a3e052..c0c8ffbd9e 100644
>
> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
>
> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
>
> @@ -737,12 +737,68 @@ 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 +1006,7 @@ AhciPioTransfer (
>
> UINT32 PrdCount;
>
> UINT32 Retry;
>
> EFI_STATUS RecoveryStatus;
>
> + BOOLEAN DoRetry;
>
>
>
> if (Read) {
>
> Flag = EfiPciIoOperationBusMasterWrite;
>
> @@ -1027,8 +1084,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 +1182,7 @@ AhciDmaTransfer (
>
> EFI_TPL OldTpl;
>
> UINT32 Retry;
>
> EFI_STATUS RecoveryStatus;
>
> + BOOLEAN DoRetry;
>
>
>
> Map = NULL;
>
> PciIo = Instance->PciIo;
>
> @@ -1222,8 +1281,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 +1323,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 +1331,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 +1439,7 @@ AhciNonDataTransfer (
>
> EFI_AHCI_COMMAND_LIST CmdList;
>
> UINT32 Retry;
>
> EFI_STATUS RecoveryStatus;
>
> + BOOLEAN DoRetry;
>
>
>
> //
>
> // Package read needed
>
> @@ -1418,8 +1480,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
>
>
[-- Attachment #2: Type: text/html, Size: 16268 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-04-03 1:43 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-03-27 21:37 [PATCHv2 0/1] MdeModulePkg/Ahci: Skip retry for non-transient errors Albecki, Mateusz
2023-03-27 21:37 ` [PATCHv2 1/1] " Albecki, Mateusz
2023-03-28 2:03 ` Wu, Hao A
2023-03-29 2:30 ` [edk2-devel] " Anbazhagan, Baraneedharan
2023-04-03 1:43 ` Wu, Hao A
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox