* [PATCH 1/4] MdeModulePkg/AtaAtapiPassThru: Check IS to check for command completion
2020-11-03 13:23 [PATCH 0/4] MdeModulePkg/AtaAtapiPassThru: Add SATA error recovery Albecki, Mateusz
@ 2020-11-03 13:23 ` Albecki, Mateusz
2020-11-04 5:33 ` Wu, Hao A
2020-11-03 13:23 ` [PATCH 2/4] MdeModulePkg/AtaAtapiPassThru: Add SATA error recovery flow Albecki, Mateusz
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Albecki, Mateusz @ 2020-11-03 13:23 UTC (permalink / raw)
To: devel; +Cc: Albecki, Ray Ni, Hao A Wu
From: Albecki <mateusz.albecki@intel.com>
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3024
AHCI driver used to poll D2H register type to determine whether the FIS
has been received. This caused a problem of long timeouts when the link
got a CRC error and the FIS never arrives. To fix this this change
switches AHCI driver to poll the IS register which will signal both the
reception of FIS and the occurance of error.
Signed-off-by: Mateusz Albecki <mateusz.albecki@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
---
.../Bus/Ata/AtaAtapiPassThru/AhciMode.c | 292 ++++++++----------
.../Bus/Ata/AtaAtapiPassThru/AhciMode.h | 9 +-
2 files changed, 131 insertions(+), 170 deletions(-)
diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
index 7e2fade400..4b42e72226 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
@@ -248,32 +248,23 @@ AhciWaitMemSet (
/**
Check the memory status to the test value.
- @param[in] Address The memory address to test.
- @param[in] MaskValue The mask value of memory.
- @param[in] TestValue The test value of memory.
- @param[in, out] Task Optional. Pointer to the ATA_NONBLOCK_TASK used by
- non-blocking mode. If NULL, then just try once.
-
- @retval EFI_NOTREADY The memory is not set.
- @retval EFI_TIMEOUT The memory setting retry times out.
- @retval EFI_SUCCESS The memory is correct set.
+ @param[in] Address The memory address to test.
+ @param[in] MaskValue The mask value of memory.
+ @param[in] TestValue The test value of memory.
+ @retval EFI_NOT_READY The memory is not set.
+ @retval EFI_SUCCESS The memory is correct set.
**/
EFI_STATUS
EFIAPI
AhciCheckMemSet (
IN UINTN Address,
IN UINT32 MaskValue,
- IN UINT32 TestValue,
- IN OUT ATA_NONBLOCK_TASK *Task
+ IN UINT32 TestValue
)
{
UINT32 Value;
- if (Task != NULL) {
- Task->RetryTimes--;
- }
-
Value = *(volatile UINT32 *) Address;
Value &= MaskValue;
@@ -281,11 +272,7 @@ AhciCheckMemSet (
return EFI_SUCCESS;
}
- if ((Task != NULL) && !Task->InfiniteWait && (Task->RetryTimes == 0)) {
- return EFI_TIMEOUT;
- } else {
- return EFI_NOT_READY;
- }
+ return EFI_NOT_READY;
}
@@ -357,7 +344,7 @@ AhciDumpPortStatus (
FisBaseAddr = (UINTN)AhciRegisters->AhciRFis + Port * sizeof (EFI_AHCI_RECEIVED_FIS);
Offset = FisBaseAddr + EFI_AHCI_D2H_FIS_OFFSET;
- Status = AhciCheckMemSet (Offset, EFI_AHCI_FIS_TYPE_MASK, EFI_AHCI_FIS_REGISTER_D2H, NULL);
+ Status = AhciCheckMemSet (Offset, EFI_AHCI_FIS_TYPE_MASK, EFI_AHCI_FIS_REGISTER_D2H);
if (!EFI_ERROR (Status)) {
//
// If D2H FIS is received, update StatusBlock with its content.
@@ -621,6 +608,102 @@ AhciBuildCommandFis (
CmdFis->AhciCFisDevHead = (UINT8) (AtaCommandBlock->AtaDeviceHead | 0xE0);
}
+/**
+ Checks if specified FIS has been received.
+
+ @param[in] PciIo Pointer to AHCI controller PciIo.
+ @param[in] Port SATA port index on which to check.
+ @param[in] FisType FIS type for which to check.
+
+ @retval EFI_SUCCESS FIS received.
+ @retval EFI_NOT_READY FIS not received yet.
+ @retval EFI_DEVICE_ERROR AHCI controller reported an error on port.
+**/
+EFI_STATUS
+AhciCheckFisReceived (
+ IN EFI_PCI_IO_PROTOCOL *PciIo,
+ IN UINT8 Port,
+ IN SATA_FIS_TYPE FisType
+ )
+{
+ UINT32 Offset;
+ UINT32 PortInterrupt;
+ UINT32 PortTfd;
+
+ Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH + EFI_AHCI_PORT_IS;
+ PortInterrupt = AhciReadReg (PciIo, Offset);
+ if ((PortInterrupt & EFI_AHCI_PORT_IS_ERROR_MASK) != 0) {
+ DEBUG ((DEBUG_ERROR, "AHCI: Error interrupt reported PxIS: %X\n", PortInterrupt));
+ return EFI_DEVICE_ERROR;
+ }
+ //
+ // For PIO setup FIS - According to SATA 2.6 spec section 11.7, D2h FIS means an error encountered.
+ // But Qemu and Marvel 9230 sata controller may just receive a D2h FIS from device
+ // after the transaction is finished successfully.
+ // To get better device compatibilities, we further check if the PxTFD's ERR bit is set.
+ // By this way, we can know if there is a real error happened.
+ //
+ if (((FisType == SataFisD2H) && ((PortInterrupt & EFI_AHCI_PORT_IS_DHRS) != 0)) ||
+ ((FisType == SataFisPioSetup) && (PortInterrupt & (EFI_AHCI_PORT_IS_PSS | EFI_AHCI_PORT_IS_DHRS)) != 0) ||
+ ((FisType == SataFisDmaSetup) && (PortInterrupt & (EFI_AHCI_PORT_IS_DSS | EFI_AHCI_PORT_IS_DHRS)) != 0)) {
+ Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH + EFI_AHCI_PORT_TFD;
+ PortTfd = AhciReadReg (PciIo, (UINT32) Offset);
+ if ((PortTfd & EFI_AHCI_PORT_TFD_ERR) != 0) {
+ return EFI_DEVICE_ERROR;
+ } else {
+ return EFI_SUCCESS;
+ }
+ }
+
+ return EFI_NOT_READY;
+}
+
+/**
+ Waits until specified FIS has been received.
+
+ @param[in] PciIo Pointer to AHCI controller PciIo.
+ @param[in] Port SATA port index on which to check.
+ @param[in] Timeout Time after which function should stop polling.
+ @param[in] FisType FIS type for which to check.
+
+ @retval EFI_SUCCESS FIS received.
+ @retval EFI_TIMEOUT FIS failed to arrive within a specified time period.
+ @retval EFI_DEVICE_ERROR AHCI controller reported an error on port.
+**/
+EFI_STATUS
+AhciWaitUntilFisReceived (
+ IN EFI_PCI_IO_PROTOCOL *PciIo,
+ IN UINT8 Port,
+ IN UINT64 Timeout,
+ IN SATA_FIS_TYPE FisType
+ )
+{
+ EFI_STATUS Status;
+ BOOLEAN InfiniteWait;
+ UINT64 Delay;
+
+ Delay = DivU64x32 (Timeout, 1000) + 1;
+ if (Timeout == 0) {
+ InfiniteWait = TRUE;
+ } else {
+ InfiniteWait = FALSE;
+ }
+
+ do {
+ Status = AhciCheckFisReceived (PciIo, Port, FisType);
+ if (Status != EFI_NOT_READY) {
+ return Status;
+ }
+ //
+ // Stall for 100 microseconds.
+ //
+ MicroSecondDelay (100);
+ Delay--;
+ } while (InfiniteWait || (Delay > 0));
+
+ return EFI_TIMEOUT;
+}
+
/**
Start a PIO data transfer on specific port.
@@ -665,26 +748,13 @@ AhciPioTransfer (
)
{
EFI_STATUS Status;
- UINTN FisBaseAddr;
- UINTN Offset;
EFI_PHYSICAL_ADDRESS PhyAddr;
VOID *Map;
UINTN MapLength;
EFI_PCI_IO_PROTOCOL_OPERATION Flag;
- UINT64 Delay;
EFI_AHCI_COMMAND_FIS CFis;
EFI_AHCI_COMMAND_LIST CmdList;
- UINT32 PortTfd;
UINT32 PrdCount;
- BOOLEAN InfiniteWait;
- BOOLEAN PioFisReceived;
- BOOLEAN D2hFisReceived;
-
- if (Timeout == 0) {
- InfiniteWait = TRUE;
- } else {
- InfiniteWait = FALSE;
- }
if (Read) {
Flag = EfiPciIoOperationBusMasterWrite;
@@ -743,87 +813,18 @@ AhciPioTransfer (
goto Exit;
}
- //
- // Check the status and wait the driver sending data
- //
- FisBaseAddr = (UINTN)AhciRegisters->AhciRFis + Port * sizeof (EFI_AHCI_RECEIVED_FIS);
-
if (Read && (AtapiCommand == 0)) {
- //
- // Wait device sends the PIO setup fis before data transfer
- //
- Status = EFI_TIMEOUT;
- Delay = DivU64x32 (Timeout, 1000) + 1;
- do {
- PioFisReceived = FALSE;
- D2hFisReceived = FALSE;
- Offset = FisBaseAddr + EFI_AHCI_PIO_FIS_OFFSET;
- Status = AhciCheckMemSet (Offset, EFI_AHCI_FIS_TYPE_MASK, EFI_AHCI_FIS_PIO_SETUP, NULL);
- if (!EFI_ERROR (Status)) {
- PioFisReceived = TRUE;
- }
- //
- // According to SATA 2.6 spec section 11.7, D2h FIS means an error encountered.
- // But Qemu and Marvel 9230 sata controller may just receive a D2h FIS from device
- // after the transaction is finished successfully.
- // To get better device compatibilities, we further check if the PxTFD's ERR bit is set.
- // By this way, we can know if there is a real error happened.
- //
- Offset = FisBaseAddr + EFI_AHCI_D2H_FIS_OFFSET;
- Status = AhciCheckMemSet (Offset, EFI_AHCI_FIS_TYPE_MASK, EFI_AHCI_FIS_REGISTER_D2H, NULL);
- if (!EFI_ERROR (Status)) {
- D2hFisReceived = TRUE;
- }
-
- if (PioFisReceived || D2hFisReceived) {
- Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH + EFI_AHCI_PORT_TFD;
- PortTfd = AhciReadReg (PciIo, (UINT32) Offset);
- //
- // PxTFD will be updated if there is a D2H or SetupFIS received.
- //
- if ((PortTfd & EFI_AHCI_PORT_TFD_ERR) != 0) {
- Status = EFI_DEVICE_ERROR;
- break;
- }
-
- PrdCount = *(volatile UINT32 *) (&(AhciRegisters->AhciCmdList[0].AhciCmdPrdbc));
- if (PrdCount == DataCount) {
- Status = EFI_SUCCESS;
- break;
- }
- }
-
- //
- // Stall for 100 microseconds.
- //
- MicroSecondDelay(100);
-
- Delay--;
- if (Delay == 0) {
- Status = EFI_TIMEOUT;
+ Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout, SataFisPioSetup);
+ if (Status == EFI_SUCCESS) {
+ PrdCount = *(volatile UINT32 *) (&(AhciRegisters->AhciCmdList[0].AhciCmdPrdbc));
+ if (PrdCount == DataCount) {
+ Status = EFI_SUCCESS;
+ } else {
+ Status = EFI_DEVICE_ERROR;
}
- } while (InfiniteWait || (Delay > 0));
- } else {
- //
- // Wait for D2H Fis is received
- //
- Offset = FisBaseAddr + EFI_AHCI_D2H_FIS_OFFSET;
- Status = AhciWaitMemSet (
- Offset,
- EFI_AHCI_FIS_TYPE_MASK,
- EFI_AHCI_FIS_REGISTER_D2H,
- Timeout
- );
-
- if (EFI_ERROR (Status)) {
- goto Exit;
- }
-
- Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH + EFI_AHCI_PORT_TFD;
- PortTfd = AhciReadReg (PciIo, (UINT32) Offset);
- if ((PortTfd & EFI_AHCI_PORT_TFD_ERR) != 0) {
- Status = EFI_DEVICE_ERROR;
}
+ } else {
+ Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout, SataFisD2H);
}
Exit:
@@ -893,15 +894,12 @@ AhciDmaTransfer (
)
{
EFI_STATUS Status;
- UINTN Offset;
EFI_PHYSICAL_ADDRESS PhyAddr;
VOID *Map;
UINTN MapLength;
EFI_PCI_IO_PROTOCOL_OPERATION Flag;
EFI_AHCI_COMMAND_FIS CFis;
EFI_AHCI_COMMAND_LIST CmdList;
- UINTN FisBaseAddr;
- UINT32 PortTfd;
EFI_PCI_IO_PROTOCOL *PciIo;
EFI_TPL OldTpl;
@@ -996,38 +994,17 @@ AhciDmaTransfer (
}
}
- //
- // Wait for command complete
- //
- FisBaseAddr = (UINTN)AhciRegisters->AhciRFis + Port * sizeof (EFI_AHCI_RECEIVED_FIS);
- Offset = FisBaseAddr + EFI_AHCI_D2H_FIS_OFFSET;
if (Task != NULL) {
- //
- // For Non-blocking
- //
- Status = AhciCheckMemSet (
- Offset,
- EFI_AHCI_FIS_TYPE_MASK,
- EFI_AHCI_FIS_REGISTER_D2H,
- Task
- );
+ Status = AhciCheckFisReceived (PciIo, Port, SataFisD2H);
+ if (Status == EFI_NOT_READY) {
+ if (!Task->InfiniteWait && Task->RetryTimes == 0) {
+ Status = EFI_TIMEOUT;
+ } else {
+ Task->RetryTimes--;
+ }
+ }
} else {
- Status = AhciWaitMemSet (
- Offset,
- EFI_AHCI_FIS_TYPE_MASK,
- EFI_AHCI_FIS_REGISTER_D2H,
- Timeout
- );
- }
-
- if (EFI_ERROR (Status)) {
- goto Exit;
- }
-
- Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH + EFI_AHCI_PORT_TFD;
- PortTfd = AhciReadReg (PciIo, (UINT32) Offset);
- if ((PortTfd & EFI_AHCI_PORT_TFD_ERR) != 0) {
- Status = EFI_DEVICE_ERROR;
+ Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout, SataFisD2H);
}
Exit:
@@ -1105,9 +1082,6 @@ AhciNonDataTransfer (
)
{
EFI_STATUS Status;
- UINTN FisBaseAddr;
- UINTN Offset;
- UINT32 PortTfd;
EFI_AHCI_COMMAND_FIS CFis;
EFI_AHCI_COMMAND_LIST CmdList;
@@ -1144,27 +1118,7 @@ AhciNonDataTransfer (
goto Exit;
}
- //
- // Wait device sends the Response Fis
- //
- FisBaseAddr = (UINTN)AhciRegisters->AhciRFis + Port * sizeof (EFI_AHCI_RECEIVED_FIS);
- Offset = FisBaseAddr + EFI_AHCI_D2H_FIS_OFFSET;
- Status = AhciWaitMemSet (
- Offset,
- EFI_AHCI_FIS_TYPE_MASK,
- EFI_AHCI_FIS_REGISTER_D2H,
- Timeout
- );
-
- if (EFI_ERROR (Status)) {
- goto Exit;
- }
-
- Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH + EFI_AHCI_PORT_TFD;
- PortTfd = AhciReadReg (PciIo, (UINT32) Offset);
- if ((PortTfd & EFI_AHCI_PORT_TFD_ERR) != 0) {
- Status = EFI_DEVICE_ERROR;
- }
+ Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout, SataFisD2H);
Exit:
AhciStopCommand (
diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
index 786413930a..a3cd351f6e 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
@@ -96,7 +96,7 @@ typedef union {
#define EFI_AHCI_PORT_IS 0x0010
#define EFI_AHCI_PORT_IS_DHRS BIT0
#define EFI_AHCI_PORT_IS_PSS BIT1
-#define EFI_AHCI_PORT_IS_SSS BIT2
+#define EFI_AHCI_PORT_IS_DSS BIT2
#define EFI_AHCI_PORT_IS_SDBS BIT3
#define EFI_AHCI_PORT_IS_UFS BIT4
#define EFI_AHCI_PORT_IS_DPS BIT5
@@ -113,6 +113,7 @@ typedef union {
#define EFI_AHCI_PORT_IS_CPDS BIT31
#define EFI_AHCI_PORT_IS_CLEAR 0xFFFFFFFF
#define EFI_AHCI_PORT_IS_FIS_CLEAR 0x0000000F
+#define EFI_AHCI_PORT_IS_ERROR_MASK (EFI_AHCI_PORT_IS_INFS | EFI_AHCI_PORT_IS_IFS | EFI_AHCI_PORT_IS_HBDS | EFI_AHCI_PORT_IS_HBFS | EFI_AHCI_PORT_IS_TFES)
#define EFI_AHCI_PORT_IE 0x0014
#define EFI_AHCI_PORT_CMD 0x0018
@@ -240,6 +241,12 @@ typedef struct {
UINT8 AhciCFisRsvd5[44];
} EFI_AHCI_COMMAND_FIS;
+typedef enum {
+ SataFisD2H = 0,
+ SataFisPioSetup,
+ SataFisDmaSetup
+} SATA_FIS_TYPE;
+
//
// ACMD: ATAPI command (12 or 16 bytes)
//
--
2.28.0.windows.1
---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Sowackiego 173 | 80-298 Gdask | Sd Rejonowy Gdask Pnoc | VII Wydzia Gospodarczy Krajowego Rejestru Sdowego - KRS 101882 | NIP 957-07-52-316 | Kapita zakadowy 200.000 PLN.
Ta wiadomo wraz z zacznikami jest przeznaczona dla okrelonego adresata i moe zawiera informacje poufne. W razie przypadkowego otrzymania tej wiadomoci, prosimy o powiadomienie nadawcy oraz trwae jej usunicie; jakiekolwiek przegldanie 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] 11+ messages in thread* Re: [PATCH 1/4] MdeModulePkg/AtaAtapiPassThru: Check IS to check for command completion
2020-11-03 13:23 ` [PATCH 1/4] MdeModulePkg/AtaAtapiPassThru: Check IS to check for command completion Albecki, Mateusz
@ 2020-11-04 5:33 ` Wu, Hao A
2020-11-04 11:54 ` Albecki, Mateusz
0 siblings, 1 reply; 11+ messages in thread
From: Wu, Hao A @ 2020-11-04 5:33 UTC (permalink / raw)
To: Albecki, Mateusz, devel@edk2.groups.io; +Cc: Ni, Ray
> -----Original Message-----
> From: Albecki, Mateusz <mateusz.albecki@intel.com>
> Sent: Tuesday, November 3, 2020 9:24 PM
> To: devel@edk2.groups.io
> Cc: Albecki, Mateusz <mateusz.albecki@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> Subject: [PATCH 1/4] MdeModulePkg/AtaAtapiPassThru: Check IS to check
> for command completion
>
> From: Albecki <mateusz.albecki@intel.com>
Hello Albecki,
Could you help to check the author field for the patch series?
(All 4 patches seem to have the same issue)
I found that the author field shows:
Albecki <mateusz.albecki@intel.com>
Which does not match exactly with your Signed-off tag.
You can use the below command to change the information for one repository:
git config user.name "FIRST_NAME LAST_NAME"
Or below command to change it globally among all repositories:
git config --global user.name "FIRST_NAME LAST_NAME"
And then, redo the commit process again.
Also, could you help to update the copyright year information for AhciMode.c and AhciMode.h?
With above 2 things handled:
Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
Best Regards,
Hao Wu
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3024
>
> AHCI driver used to poll D2H register type to determine whether the FIS has
> been received. This caused a problem of long timeouts when the link got a
> CRC error and the FIS never arrives. To fix this this change switches AHCI
> driver to poll the IS register which will signal both the reception of FIS and the
> occurance of error.
>
> Signed-off-by: Mateusz Albecki <mateusz.albecki@intel.com>
>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
>
> ---
> .../Bus/Ata/AtaAtapiPassThru/AhciMode.c | 292 ++++++++----------
> .../Bus/Ata/AtaAtapiPassThru/AhciMode.h | 9 +-
> 2 files changed, 131 insertions(+), 170 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> index 7e2fade400..4b42e72226 100644
> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> @@ -248,32 +248,23 @@ AhciWaitMemSet (
> /**
> Check the memory status to the test value.
>
> - @param[in] Address The memory address to test.
> - @param[in] MaskValue The mask value of memory.
> - @param[in] TestValue The test value of memory.
> - @param[in, out] Task Optional. Pointer to the
> ATA_NONBLOCK_TASK used by
> - non-blocking mode. If NULL, then just try once.
> -
> - @retval EFI_NOTREADY The memory is not set.
> - @retval EFI_TIMEOUT The memory setting retry times out.
> - @retval EFI_SUCCESS The memory is correct set.
> + @param[in] Address The memory address to test.
> + @param[in] MaskValue The mask value of memory.
> + @param[in] TestValue The test value of memory.
>
> + @retval EFI_NOT_READY The memory is not set.
> + @retval EFI_SUCCESS The memory is correct set.
> **/
> EFI_STATUS
> EFIAPI
> AhciCheckMemSet (
> IN UINTN Address,
> IN UINT32 MaskValue,
> - IN UINT32 TestValue,
> - IN OUT ATA_NONBLOCK_TASK *Task
> + IN UINT32 TestValue
> )
> {
> UINT32 Value;
>
> - if (Task != NULL) {
> - Task->RetryTimes--;
> - }
> -
> Value = *(volatile UINT32 *) Address;
> Value &= MaskValue;
>
> @@ -281,11 +272,7 @@ AhciCheckMemSet (
> return EFI_SUCCESS;
> }
>
> - if ((Task != NULL) && !Task->InfiniteWait && (Task->RetryTimes == 0)) {
> - return EFI_TIMEOUT;
> - } else {
> - return EFI_NOT_READY;
> - }
> + return EFI_NOT_READY;
> }
>
>
> @@ -357,7 +344,7 @@ AhciDumpPortStatus (
> FisBaseAddr = (UINTN)AhciRegisters->AhciRFis + Port * sizeof
> (EFI_AHCI_RECEIVED_FIS);
> Offset = FisBaseAddr + EFI_AHCI_D2H_FIS_OFFSET;
>
> - Status = AhciCheckMemSet (Offset, EFI_AHCI_FIS_TYPE_MASK,
> EFI_AHCI_FIS_REGISTER_D2H, NULL);
> + Status = AhciCheckMemSet (Offset, EFI_AHCI_FIS_TYPE_MASK,
> + EFI_AHCI_FIS_REGISTER_D2H);
> if (!EFI_ERROR (Status)) {
> //
> // If D2H FIS is received, update StatusBlock with its content.
> @@ -621,6 +608,102 @@ AhciBuildCommandFis (
> CmdFis->AhciCFisDevHead = (UINT8) (AtaCommandBlock-
> >AtaDeviceHead | 0xE0);
> }
>
> +/**
> + Checks if specified FIS has been received.
> +
> + @param[in] PciIo Pointer to AHCI controller PciIo.
> + @param[in] Port SATA port index on which to check.
> + @param[in] FisType FIS type for which to check.
> +
> + @retval EFI_SUCCESS FIS received.
> + @retval EFI_NOT_READY FIS not received yet.
> + @retval EFI_DEVICE_ERROR AHCI controller reported an error on port.
> +**/
> +EFI_STATUS
> +AhciCheckFisReceived (
> + IN EFI_PCI_IO_PROTOCOL *PciIo,
> + IN UINT8 Port,
> + IN SATA_FIS_TYPE FisType
> + )
> +{
> + UINT32 Offset;
> + UINT32 PortInterrupt;
> + UINT32 PortTfd;
> +
> + Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH +
> + EFI_AHCI_PORT_IS; PortInterrupt = AhciReadReg (PciIo, Offset); if
> + ((PortInterrupt & EFI_AHCI_PORT_IS_ERROR_MASK) != 0) {
> + DEBUG ((DEBUG_ERROR, "AHCI: Error interrupt reported PxIS: %X\n",
> PortInterrupt));
> + return EFI_DEVICE_ERROR;
> + }
> + //
> + // For PIO setup FIS - According to SATA 2.6 spec section 11.7, D2h FIS
> means an error encountered.
> + // But Qemu and Marvel 9230 sata controller may just receive a D2h
> + FIS from device // after the transaction is finished successfully.
> + // To get better device compatibilities, we further check if the PxTFD's ERR
> bit is set.
> + // By this way, we can know if there is a real error happened.
> + //
> + if (((FisType == SataFisD2H) && ((PortInterrupt &
> EFI_AHCI_PORT_IS_DHRS) != 0)) ||
> + ((FisType == SataFisPioSetup) && (PortInterrupt &
> (EFI_AHCI_PORT_IS_PSS | EFI_AHCI_PORT_IS_DHRS)) != 0) ||
> + ((FisType == SataFisDmaSetup) && (PortInterrupt &
> (EFI_AHCI_PORT_IS_DSS | EFI_AHCI_PORT_IS_DHRS)) != 0)) {
> + Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH +
> EFI_AHCI_PORT_TFD;
> + PortTfd = AhciReadReg (PciIo, (UINT32) Offset);
> + if ((PortTfd & EFI_AHCI_PORT_TFD_ERR) != 0) {
> + return EFI_DEVICE_ERROR;
> + } else {
> + return EFI_SUCCESS;
> + }
> + }
> +
> + return EFI_NOT_READY;
> +}
> +
> +/**
> + Waits until specified FIS has been received.
> +
> + @param[in] PciIo Pointer to AHCI controller PciIo.
> + @param[in] Port SATA port index on which to check.
> + @param[in] Timeout Time after which function should stop polling.
> + @param[in] FisType FIS type for which to check.
> +
> + @retval EFI_SUCCESS FIS received.
> + @retval EFI_TIMEOUT FIS failed to arrive within a specified time period.
> + @retval EFI_DEVICE_ERROR AHCI controller reported an error on port.
> +**/
> +EFI_STATUS
> +AhciWaitUntilFisReceived (
> + IN EFI_PCI_IO_PROTOCOL *PciIo,
> + IN UINT8 Port,
> + IN UINT64 Timeout,
> + IN SATA_FIS_TYPE FisType
> + )
> +{
> + EFI_STATUS Status;
> + BOOLEAN InfiniteWait;
> + UINT64 Delay;
> +
> + Delay = DivU64x32 (Timeout, 1000) + 1; if (Timeout == 0) {
> + InfiniteWait = TRUE;
> + } else {
> + InfiniteWait = FALSE;
> + }
> +
> + do {
> + Status = AhciCheckFisReceived (PciIo, Port, FisType);
> + if (Status != EFI_NOT_READY) {
> + return Status;
> + }
> + //
> + // Stall for 100 microseconds.
> + //
> + MicroSecondDelay (100);
> + Delay--;
> + } while (InfiniteWait || (Delay > 0));
> +
> + return EFI_TIMEOUT;
> +}
> +
> /**
> Start a PIO data transfer on specific port.
>
> @@ -665,26 +748,13 @@ AhciPioTransfer (
> )
> {
> EFI_STATUS Status;
> - UINTN FisBaseAddr;
> - UINTN Offset;
> EFI_PHYSICAL_ADDRESS PhyAddr;
> VOID *Map;
> UINTN MapLength;
> EFI_PCI_IO_PROTOCOL_OPERATION Flag;
> - UINT64 Delay;
> EFI_AHCI_COMMAND_FIS CFis;
> EFI_AHCI_COMMAND_LIST CmdList;
> - UINT32 PortTfd;
> UINT32 PrdCount;
> - BOOLEAN InfiniteWait;
> - BOOLEAN PioFisReceived;
> - BOOLEAN D2hFisReceived;
> -
> - if (Timeout == 0) {
> - InfiniteWait = TRUE;
> - } else {
> - InfiniteWait = FALSE;
> - }
>
> if (Read) {
> Flag = EfiPciIoOperationBusMasterWrite; @@ -743,87 +813,18 @@
> AhciPioTransfer (
> goto Exit;
> }
>
> - //
> - // Check the status and wait the driver sending data
> - //
> - FisBaseAddr = (UINTN)AhciRegisters->AhciRFis + Port * sizeof
> (EFI_AHCI_RECEIVED_FIS);
> -
> if (Read && (AtapiCommand == 0)) {
> - //
> - // Wait device sends the PIO setup fis before data transfer
> - //
> - Status = EFI_TIMEOUT;
> - Delay = DivU64x32 (Timeout, 1000) + 1;
> - do {
> - PioFisReceived = FALSE;
> - D2hFisReceived = FALSE;
> - Offset = FisBaseAddr + EFI_AHCI_PIO_FIS_OFFSET;
> - Status = AhciCheckMemSet (Offset, EFI_AHCI_FIS_TYPE_MASK,
> EFI_AHCI_FIS_PIO_SETUP, NULL);
> - if (!EFI_ERROR (Status)) {
> - PioFisReceived = TRUE;
> - }
> - //
> - // According to SATA 2.6 spec section 11.7, D2h FIS means an error
> encountered.
> - // But Qemu and Marvel 9230 sata controller may just receive a D2h FIS
> from device
> - // after the transaction is finished successfully.
> - // To get better device compatibilities, we further check if the PxTFD's
> ERR bit is set.
> - // By this way, we can know if there is a real error happened.
> - //
> - Offset = FisBaseAddr + EFI_AHCI_D2H_FIS_OFFSET;
> - Status = AhciCheckMemSet (Offset, EFI_AHCI_FIS_TYPE_MASK,
> EFI_AHCI_FIS_REGISTER_D2H, NULL);
> - if (!EFI_ERROR (Status)) {
> - D2hFisReceived = TRUE;
> - }
> -
> - if (PioFisReceived || D2hFisReceived) {
> - Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH +
> EFI_AHCI_PORT_TFD;
> - PortTfd = AhciReadReg (PciIo, (UINT32) Offset);
> - //
> - // PxTFD will be updated if there is a D2H or SetupFIS received.
> - //
> - if ((PortTfd & EFI_AHCI_PORT_TFD_ERR) != 0) {
> - Status = EFI_DEVICE_ERROR;
> - break;
> - }
> -
> - PrdCount = *(volatile UINT32 *) (&(AhciRegisters-
> >AhciCmdList[0].AhciCmdPrdbc));
> - if (PrdCount == DataCount) {
> - Status = EFI_SUCCESS;
> - break;
> - }
> - }
> -
> - //
> - // Stall for 100 microseconds.
> - //
> - MicroSecondDelay(100);
> -
> - Delay--;
> - if (Delay == 0) {
> - Status = EFI_TIMEOUT;
> + Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout, SataFisPioSetup);
> + if (Status == EFI_SUCCESS) {
> + PrdCount = *(volatile UINT32 *) (&(AhciRegisters-
> >AhciCmdList[0].AhciCmdPrdbc));
> + if (PrdCount == DataCount) {
> + Status = EFI_SUCCESS;
> + } else {
> + Status = EFI_DEVICE_ERROR;
> }
> - } while (InfiniteWait || (Delay > 0));
> - } else {
> - //
> - // Wait for D2H Fis is received
> - //
> - Offset = FisBaseAddr + EFI_AHCI_D2H_FIS_OFFSET;
> - Status = AhciWaitMemSet (
> - Offset,
> - EFI_AHCI_FIS_TYPE_MASK,
> - EFI_AHCI_FIS_REGISTER_D2H,
> - Timeout
> - );
> -
> - if (EFI_ERROR (Status)) {
> - goto Exit;
> - }
> -
> - Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH +
> EFI_AHCI_PORT_TFD;
> - PortTfd = AhciReadReg (PciIo, (UINT32) Offset);
> - if ((PortTfd & EFI_AHCI_PORT_TFD_ERR) != 0) {
> - Status = EFI_DEVICE_ERROR;
> }
> + } else {
> + Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout,
> + SataFisD2H);
> }
>
> Exit:
> @@ -893,15 +894,12 @@ AhciDmaTransfer (
> )
> {
> EFI_STATUS Status;
> - UINTN Offset;
> EFI_PHYSICAL_ADDRESS PhyAddr;
> VOID *Map;
> UINTN MapLength;
> EFI_PCI_IO_PROTOCOL_OPERATION Flag;
> EFI_AHCI_COMMAND_FIS CFis;
> EFI_AHCI_COMMAND_LIST CmdList;
> - UINTN FisBaseAddr;
> - UINT32 PortTfd;
>
> EFI_PCI_IO_PROTOCOL *PciIo;
> EFI_TPL OldTpl;
> @@ -996,38 +994,17 @@ AhciDmaTransfer (
> }
> }
>
> - //
> - // Wait for command complete
> - //
> - FisBaseAddr = (UINTN)AhciRegisters->AhciRFis + Port * sizeof
> (EFI_AHCI_RECEIVED_FIS);
> - Offset = FisBaseAddr + EFI_AHCI_D2H_FIS_OFFSET;
> if (Task != NULL) {
> - //
> - // For Non-blocking
> - //
> - Status = AhciCheckMemSet (
> - Offset,
> - EFI_AHCI_FIS_TYPE_MASK,
> - EFI_AHCI_FIS_REGISTER_D2H,
> - Task
> - );
> + Status = AhciCheckFisReceived (PciIo, Port, SataFisD2H);
> + if (Status == EFI_NOT_READY) {
> + if (!Task->InfiniteWait && Task->RetryTimes == 0) {
> + Status = EFI_TIMEOUT;
> + } else {
> + Task->RetryTimes--;
> + }
> + }
> } else {
> - Status = AhciWaitMemSet (
> - Offset,
> - EFI_AHCI_FIS_TYPE_MASK,
> - EFI_AHCI_FIS_REGISTER_D2H,
> - Timeout
> - );
> - }
> -
> - if (EFI_ERROR (Status)) {
> - goto Exit;
> - }
> -
> - Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH +
> EFI_AHCI_PORT_TFD;
> - PortTfd = AhciReadReg (PciIo, (UINT32) Offset);
> - if ((PortTfd & EFI_AHCI_PORT_TFD_ERR) != 0) {
> - Status = EFI_DEVICE_ERROR;
> + Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout,
> + SataFisD2H);
> }
>
> Exit:
> @@ -1105,9 +1082,6 @@ AhciNonDataTransfer (
> )
> {
> EFI_STATUS Status;
> - UINTN FisBaseAddr;
> - UINTN Offset;
> - UINT32 PortTfd;
> EFI_AHCI_COMMAND_FIS CFis;
> EFI_AHCI_COMMAND_LIST CmdList;
>
> @@ -1144,27 +1118,7 @@ AhciNonDataTransfer (
> goto Exit;
> }
>
> - //
> - // Wait device sends the Response Fis
> - //
> - FisBaseAddr = (UINTN)AhciRegisters->AhciRFis + Port * sizeof
> (EFI_AHCI_RECEIVED_FIS);
> - Offset = FisBaseAddr + EFI_AHCI_D2H_FIS_OFFSET;
> - Status = AhciWaitMemSet (
> - Offset,
> - EFI_AHCI_FIS_TYPE_MASK,
> - EFI_AHCI_FIS_REGISTER_D2H,
> - Timeout
> - );
> -
> - if (EFI_ERROR (Status)) {
> - goto Exit;
> - }
> -
> - Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH +
> EFI_AHCI_PORT_TFD;
> - PortTfd = AhciReadReg (PciIo, (UINT32) Offset);
> - if ((PortTfd & EFI_AHCI_PORT_TFD_ERR) != 0) {
> - Status = EFI_DEVICE_ERROR;
> - }
> + Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout, SataFisD2H);
>
> Exit:
> AhciStopCommand (
> diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
> index 786413930a..a3cd351f6e 100644
> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
> @@ -96,7 +96,7 @@ typedef union {
> #define EFI_AHCI_PORT_IS 0x0010
> #define EFI_AHCI_PORT_IS_DHRS BIT0
> #define EFI_AHCI_PORT_IS_PSS BIT1
> -#define EFI_AHCI_PORT_IS_SSS BIT2
> +#define EFI_AHCI_PORT_IS_DSS BIT2
> #define EFI_AHCI_PORT_IS_SDBS BIT3
> #define EFI_AHCI_PORT_IS_UFS BIT4
> #define EFI_AHCI_PORT_IS_DPS BIT5
> @@ -113,6 +113,7 @@ typedef union {
> #define EFI_AHCI_PORT_IS_CPDS BIT31
> #define EFI_AHCI_PORT_IS_CLEAR 0xFFFFFFFF
> #define EFI_AHCI_PORT_IS_FIS_CLEAR 0x0000000F
> +#define EFI_AHCI_PORT_IS_ERROR_MASK (EFI_AHCI_PORT_IS_INFS
> | EFI_AHCI_PORT_IS_IFS | EFI_AHCI_PORT_IS_HBDS |
> EFI_AHCI_PORT_IS_HBFS | EFI_AHCI_PORT_IS_TFES)
>
> #define EFI_AHCI_PORT_IE 0x0014
> #define EFI_AHCI_PORT_CMD 0x0018
> @@ -240,6 +241,12 @@ typedef struct {
> UINT8 AhciCFisRsvd5[44];
> } EFI_AHCI_COMMAND_FIS;
>
> +typedef enum {
> + SataFisD2H = 0,
> + SataFisPioSetup,
> + SataFisDmaSetup
> +} SATA_FIS_TYPE;
> +
> //
> // ACMD: ATAPI command (12 or 16 bytes) //
> --
> 2.28.0.windows.1
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/4] MdeModulePkg/AtaAtapiPassThru: Check IS to check for command completion
2020-11-04 5:33 ` Wu, Hao A
@ 2020-11-04 11:54 ` Albecki, Mateusz
0 siblings, 0 replies; 11+ messages in thread
From: Albecki, Mateusz @ 2020-11-04 11:54 UTC (permalink / raw)
To: Wu, Hao A, devel@edk2.groups.io; +Cc: Ni, Ray
I will fix those 2 issues in v2.
> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Wednesday, November 4, 2020 6:33 AM
> To: Albecki, Mateusz <mateusz.albecki@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>
> Subject: RE: [PATCH 1/4] MdeModulePkg/AtaAtapiPassThru: Check IS to
> check for command completion
>
> > -----Original Message-----
> > From: Albecki, Mateusz <mateusz.albecki@intel.com>
> > Sent: Tuesday, November 3, 2020 9:24 PM
> > To: devel@edk2.groups.io
> > Cc: Albecki, Mateusz <mateusz.albecki@intel.com>; Ni, Ray
> > <ray.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> > Subject: [PATCH 1/4] MdeModulePkg/AtaAtapiPassThru: Check IS to check
> > for command completion
> >
> > From: Albecki <mateusz.albecki@intel.com>
>
>
> Hello Albecki,
>
> Could you help to check the author field for the patch series?
> (All 4 patches seem to have the same issue)
>
> I found that the author field shows:
> Albecki <mateusz.albecki@intel.com>
>
> Which does not match exactly with your Signed-off tag.
>
> You can use the below command to change the information for one
> repository:
> git config user.name "FIRST_NAME LAST_NAME"
> Or below command to change it globally among all repositories:
> git config --global user.name "FIRST_NAME LAST_NAME"
> And then, redo the commit process again.
>
> Also, could you help to update the copyright year information for
> AhciMode.c and AhciMode.h?
>
> With above 2 things handled:
> Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
>
> Best Regards,
> Hao Wu
>
>
> >
> > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3024
> >
> > AHCI driver used to poll D2H register type to determine whether the
> > FIS has been received. This caused a problem of long timeouts when the
> > link got a CRC error and the FIS never arrives. To fix this this
> > change switches AHCI driver to poll the IS register which will signal
> > both the reception of FIS and the occurance of error.
> >
> > Signed-off-by: Mateusz Albecki <mateusz.albecki@intel.com>
> >
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> >
> > ---
> > .../Bus/Ata/AtaAtapiPassThru/AhciMode.c | 292 ++++++++----------
> > .../Bus/Ata/AtaAtapiPassThru/AhciMode.h | 9 +-
> > 2 files changed, 131 insertions(+), 170 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> > b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> > index 7e2fade400..4b42e72226 100644
> > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> > @@ -248,32 +248,23 @@ AhciWaitMemSet (
> > /**
> > Check the memory status to the test value.
> >
> > - @param[in] Address The memory address to test.
> > - @param[in] MaskValue The mask value of memory.
> > - @param[in] TestValue The test value of memory.
> > - @param[in, out] Task Optional. Pointer to the
> > ATA_NONBLOCK_TASK used by
> > - non-blocking mode. If NULL, then just try once.
> > -
> > - @retval EFI_NOTREADY The memory is not set.
> > - @retval EFI_TIMEOUT The memory setting retry times out.
> > - @retval EFI_SUCCESS The memory is correct set.
> > + @param[in] Address The memory address to test.
> > + @param[in] MaskValue The mask value of memory.
> > + @param[in] TestValue The test value of memory.
> >
> > + @retval EFI_NOT_READY The memory is not set.
> > + @retval EFI_SUCCESS The memory is correct set.
> > **/
> > EFI_STATUS
> > EFIAPI
> > AhciCheckMemSet (
> > IN UINTN Address,
> > IN UINT32 MaskValue,
> > - IN UINT32 TestValue,
> > - IN OUT ATA_NONBLOCK_TASK *Task
> > + IN UINT32 TestValue
> > )
> > {
> > UINT32 Value;
> >
> > - if (Task != NULL) {
> > - Task->RetryTimes--;
> > - }
> > -
> > Value = *(volatile UINT32 *) Address;
> > Value &= MaskValue;
> >
> > @@ -281,11 +272,7 @@ AhciCheckMemSet (
> > return EFI_SUCCESS;
> > }
> >
> > - if ((Task != NULL) && !Task->InfiniteWait && (Task->RetryTimes == 0)) {
> > - return EFI_TIMEOUT;
> > - } else {
> > - return EFI_NOT_READY;
> > - }
> > + return EFI_NOT_READY;
> > }
> >
> >
> > @@ -357,7 +344,7 @@ AhciDumpPortStatus (
> > FisBaseAddr = (UINTN)AhciRegisters->AhciRFis + Port * sizeof
> > (EFI_AHCI_RECEIVED_FIS);
> > Offset = FisBaseAddr + EFI_AHCI_D2H_FIS_OFFSET;
> >
> > - Status = AhciCheckMemSet (Offset, EFI_AHCI_FIS_TYPE_MASK,
> > EFI_AHCI_FIS_REGISTER_D2H, NULL);
> > + Status = AhciCheckMemSet (Offset, EFI_AHCI_FIS_TYPE_MASK,
> > + EFI_AHCI_FIS_REGISTER_D2H);
> > if (!EFI_ERROR (Status)) {
> > //
> > // If D2H FIS is received, update StatusBlock with its content.
> > @@ -621,6 +608,102 @@ AhciBuildCommandFis (
> > CmdFis->AhciCFisDevHead = (UINT8) (AtaCommandBlock-
> > >AtaDeviceHead | 0xE0);
> > }
> >
> > +/**
> > + Checks if specified FIS has been received.
> > +
> > + @param[in] PciIo Pointer to AHCI controller PciIo.
> > + @param[in] Port SATA port index on which to check.
> > + @param[in] FisType FIS type for which to check.
> > +
> > + @retval EFI_SUCCESS FIS received.
> > + @retval EFI_NOT_READY FIS not received yet.
> > + @retval EFI_DEVICE_ERROR AHCI controller reported an error on port.
> > +**/
> > +EFI_STATUS
> > +AhciCheckFisReceived (
> > + IN EFI_PCI_IO_PROTOCOL *PciIo,
> > + IN UINT8 Port,
> > + IN SATA_FIS_TYPE FisType
> > + )
> > +{
> > + UINT32 Offset;
> > + UINT32 PortInterrupt;
> > + UINT32 PortTfd;
> > +
> > + Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH +
> > + EFI_AHCI_PORT_IS; PortInterrupt = AhciReadReg (PciIo, Offset); if
> > + ((PortInterrupt & EFI_AHCI_PORT_IS_ERROR_MASK) != 0) {
> > + DEBUG ((DEBUG_ERROR, "AHCI: Error interrupt reported PxIS: %X\n",
> > PortInterrupt));
> > + return EFI_DEVICE_ERROR;
> > + }
> > + //
> > + // For PIO setup FIS - According to SATA 2.6 spec section 11.7, D2h
> > + FIS
> > means an error encountered.
> > + // But Qemu and Marvel 9230 sata controller may just receive a D2h
> > + FIS from device // after the transaction is finished successfully.
> > + // To get better device compatibilities, we further check if the
> > + PxTFD's ERR
> > bit is set.
> > + // By this way, we can know if there is a real error happened.
> > + //
> > + if (((FisType == SataFisD2H) && ((PortInterrupt &
> > EFI_AHCI_PORT_IS_DHRS) != 0)) ||
> > + ((FisType == SataFisPioSetup) && (PortInterrupt &
> > (EFI_AHCI_PORT_IS_PSS | EFI_AHCI_PORT_IS_DHRS)) != 0) ||
> > + ((FisType == SataFisDmaSetup) && (PortInterrupt &
> > (EFI_AHCI_PORT_IS_DSS | EFI_AHCI_PORT_IS_DHRS)) != 0)) {
> > + Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH
> +
> > EFI_AHCI_PORT_TFD;
> > + PortTfd = AhciReadReg (PciIo, (UINT32) Offset);
> > + if ((PortTfd & EFI_AHCI_PORT_TFD_ERR) != 0) {
> > + return EFI_DEVICE_ERROR;
> > + } else {
> > + return EFI_SUCCESS;
> > + }
> > + }
> > +
> > + return EFI_NOT_READY;
> > +}
> > +
> > +/**
> > + Waits until specified FIS has been received.
> > +
> > + @param[in] PciIo Pointer to AHCI controller PciIo.
> > + @param[in] Port SATA port index on which to check.
> > + @param[in] Timeout Time after which function should stop polling.
> > + @param[in] FisType FIS type for which to check.
> > +
> > + @retval EFI_SUCCESS FIS received.
> > + @retval EFI_TIMEOUT FIS failed to arrive within a specified time
> period.
> > + @retval EFI_DEVICE_ERROR AHCI controller reported an error on port.
> > +**/
> > +EFI_STATUS
> > +AhciWaitUntilFisReceived (
> > + IN EFI_PCI_IO_PROTOCOL *PciIo,
> > + IN UINT8 Port,
> > + IN UINT64 Timeout,
> > + IN SATA_FIS_TYPE FisType
> > + )
> > +{
> > + EFI_STATUS Status;
> > + BOOLEAN InfiniteWait;
> > + UINT64 Delay;
> > +
> > + Delay = DivU64x32 (Timeout, 1000) + 1; if (Timeout == 0) {
> > + InfiniteWait = TRUE;
> > + } else {
> > + InfiniteWait = FALSE;
> > + }
> > +
> > + do {
> > + Status = AhciCheckFisReceived (PciIo, Port, FisType);
> > + if (Status != EFI_NOT_READY) {
> > + return Status;
> > + }
> > + //
> > + // Stall for 100 microseconds.
> > + //
> > + MicroSecondDelay (100);
> > + Delay--;
> > + } while (InfiniteWait || (Delay > 0));
> > +
> > + return EFI_TIMEOUT;
> > +}
> > +
> > /**
> > Start a PIO data transfer on specific port.
> >
> > @@ -665,26 +748,13 @@ AhciPioTransfer (
> > )
> > {
> > EFI_STATUS Status;
> > - UINTN FisBaseAddr;
> > - UINTN Offset;
> > EFI_PHYSICAL_ADDRESS PhyAddr;
> > VOID *Map;
> > UINTN MapLength;
> > EFI_PCI_IO_PROTOCOL_OPERATION Flag;
> > - UINT64 Delay;
> > EFI_AHCI_COMMAND_FIS CFis;
> > EFI_AHCI_COMMAND_LIST CmdList;
> > - UINT32 PortTfd;
> > UINT32 PrdCount;
> > - BOOLEAN InfiniteWait;
> > - BOOLEAN PioFisReceived;
> > - BOOLEAN D2hFisReceived;
> > -
> > - if (Timeout == 0) {
> > - InfiniteWait = TRUE;
> > - } else {
> > - InfiniteWait = FALSE;
> > - }
> >
> > if (Read) {
> > Flag = EfiPciIoOperationBusMasterWrite; @@ -743,87 +813,18 @@
> > AhciPioTransfer (
> > goto Exit;
> > }
> >
> > - //
> > - // Check the status and wait the driver sending data
> > - //
> > - FisBaseAddr = (UINTN)AhciRegisters->AhciRFis + Port * sizeof
> > (EFI_AHCI_RECEIVED_FIS);
> > -
> > if (Read && (AtapiCommand == 0)) {
> > - //
> > - // Wait device sends the PIO setup fis before data transfer
> > - //
> > - Status = EFI_TIMEOUT;
> > - Delay = DivU64x32 (Timeout, 1000) + 1;
> > - do {
> > - PioFisReceived = FALSE;
> > - D2hFisReceived = FALSE;
> > - Offset = FisBaseAddr + EFI_AHCI_PIO_FIS_OFFSET;
> > - Status = AhciCheckMemSet (Offset, EFI_AHCI_FIS_TYPE_MASK,
> > EFI_AHCI_FIS_PIO_SETUP, NULL);
> > - if (!EFI_ERROR (Status)) {
> > - PioFisReceived = TRUE;
> > - }
> > - //
> > - // According to SATA 2.6 spec section 11.7, D2h FIS means an error
> > encountered.
> > - // But Qemu and Marvel 9230 sata controller may just receive a D2h FIS
> > from device
> > - // after the transaction is finished successfully.
> > - // To get better device compatibilities, we further check if the PxTFD's
> > ERR bit is set.
> > - // By this way, we can know if there is a real error happened.
> > - //
> > - Offset = FisBaseAddr + EFI_AHCI_D2H_FIS_OFFSET;
> > - Status = AhciCheckMemSet (Offset, EFI_AHCI_FIS_TYPE_MASK,
> > EFI_AHCI_FIS_REGISTER_D2H, NULL);
> > - if (!EFI_ERROR (Status)) {
> > - D2hFisReceived = TRUE;
> > - }
> > -
> > - if (PioFisReceived || D2hFisReceived) {
> > - Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH
> +
> > EFI_AHCI_PORT_TFD;
> > - PortTfd = AhciReadReg (PciIo, (UINT32) Offset);
> > - //
> > - // PxTFD will be updated if there is a D2H or SetupFIS received.
> > - //
> > - if ((PortTfd & EFI_AHCI_PORT_TFD_ERR) != 0) {
> > - Status = EFI_DEVICE_ERROR;
> > - break;
> > - }
> > -
> > - PrdCount = *(volatile UINT32 *) (&(AhciRegisters-
> > >AhciCmdList[0].AhciCmdPrdbc));
> > - if (PrdCount == DataCount) {
> > - Status = EFI_SUCCESS;
> > - break;
> > - }
> > - }
> > -
> > - //
> > - // Stall for 100 microseconds.
> > - //
> > - MicroSecondDelay(100);
> > -
> > - Delay--;
> > - if (Delay == 0) {
> > - Status = EFI_TIMEOUT;
> > + Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout,
> SataFisPioSetup);
> > + if (Status == EFI_SUCCESS) {
> > + PrdCount = *(volatile UINT32 *) (&(AhciRegisters-
> > >AhciCmdList[0].AhciCmdPrdbc));
> > + if (PrdCount == DataCount) {
> > + Status = EFI_SUCCESS;
> > + } else {
> > + Status = EFI_DEVICE_ERROR;
> > }
> > - } while (InfiniteWait || (Delay > 0));
> > - } else {
> > - //
> > - // Wait for D2H Fis is received
> > - //
> > - Offset = FisBaseAddr + EFI_AHCI_D2H_FIS_OFFSET;
> > - Status = AhciWaitMemSet (
> > - Offset,
> > - EFI_AHCI_FIS_TYPE_MASK,
> > - EFI_AHCI_FIS_REGISTER_D2H,
> > - Timeout
> > - );
> > -
> > - if (EFI_ERROR (Status)) {
> > - goto Exit;
> > - }
> > -
> > - Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH +
> > EFI_AHCI_PORT_TFD;
> > - PortTfd = AhciReadReg (PciIo, (UINT32) Offset);
> > - if ((PortTfd & EFI_AHCI_PORT_TFD_ERR) != 0) {
> > - Status = EFI_DEVICE_ERROR;
> > }
> > + } else {
> > + Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout,
> > + SataFisD2H);
> > }
> >
> > Exit:
> > @@ -893,15 +894,12 @@ AhciDmaTransfer (
> > )
> > {
> > EFI_STATUS Status;
> > - UINTN Offset;
> > EFI_PHYSICAL_ADDRESS PhyAddr;
> > VOID *Map;
> > UINTN MapLength;
> > EFI_PCI_IO_PROTOCOL_OPERATION Flag;
> > EFI_AHCI_COMMAND_FIS CFis;
> > EFI_AHCI_COMMAND_LIST CmdList;
> > - UINTN FisBaseAddr;
> > - UINT32 PortTfd;
> >
> > EFI_PCI_IO_PROTOCOL *PciIo;
> > EFI_TPL OldTpl;
> > @@ -996,38 +994,17 @@ AhciDmaTransfer (
> > }
> > }
> >
> > - //
> > - // Wait for command complete
> > - //
> > - FisBaseAddr = (UINTN)AhciRegisters->AhciRFis + Port * sizeof
> > (EFI_AHCI_RECEIVED_FIS);
> > - Offset = FisBaseAddr + EFI_AHCI_D2H_FIS_OFFSET;
> > if (Task != NULL) {
> > - //
> > - // For Non-blocking
> > - //
> > - Status = AhciCheckMemSet (
> > - Offset,
> > - EFI_AHCI_FIS_TYPE_MASK,
> > - EFI_AHCI_FIS_REGISTER_D2H,
> > - Task
> > - );
> > + Status = AhciCheckFisReceived (PciIo, Port, SataFisD2H);
> > + if (Status == EFI_NOT_READY) {
> > + if (!Task->InfiniteWait && Task->RetryTimes == 0) {
> > + Status = EFI_TIMEOUT;
> > + } else {
> > + Task->RetryTimes--;
> > + }
> > + }
> > } else {
> > - Status = AhciWaitMemSet (
> > - Offset,
> > - EFI_AHCI_FIS_TYPE_MASK,
> > - EFI_AHCI_FIS_REGISTER_D2H,
> > - Timeout
> > - );
> > - }
> > -
> > - if (EFI_ERROR (Status)) {
> > - goto Exit;
> > - }
> > -
> > - Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH +
> > EFI_AHCI_PORT_TFD;
> > - PortTfd = AhciReadReg (PciIo, (UINT32) Offset);
> > - if ((PortTfd & EFI_AHCI_PORT_TFD_ERR) != 0) {
> > - Status = EFI_DEVICE_ERROR;
> > + Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout,
> > + SataFisD2H);
> > }
> >
> > Exit:
> > @@ -1105,9 +1082,6 @@ AhciNonDataTransfer (
> > )
> > {
> > EFI_STATUS Status;
> > - UINTN FisBaseAddr;
> > - UINTN Offset;
> > - UINT32 PortTfd;
> > EFI_AHCI_COMMAND_FIS CFis;
> > EFI_AHCI_COMMAND_LIST CmdList;
> >
> > @@ -1144,27 +1118,7 @@ AhciNonDataTransfer (
> > goto Exit;
> > }
> >
> > - //
> > - // Wait device sends the Response Fis
> > - //
> > - FisBaseAddr = (UINTN)AhciRegisters->AhciRFis + Port * sizeof
> > (EFI_AHCI_RECEIVED_FIS);
> > - Offset = FisBaseAddr + EFI_AHCI_D2H_FIS_OFFSET;
> > - Status = AhciWaitMemSet (
> > - Offset,
> > - EFI_AHCI_FIS_TYPE_MASK,
> > - EFI_AHCI_FIS_REGISTER_D2H,
> > - Timeout
> > - );
> > -
> > - if (EFI_ERROR (Status)) {
> > - goto Exit;
> > - }
> > -
> > - Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH +
> > EFI_AHCI_PORT_TFD;
> > - PortTfd = AhciReadReg (PciIo, (UINT32) Offset);
> > - if ((PortTfd & EFI_AHCI_PORT_TFD_ERR) != 0) {
> > - Status = EFI_DEVICE_ERROR;
> > - }
> > + Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout,
> > + SataFisD2H);
> >
> > Exit:
> > AhciStopCommand (
> > diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
> > b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
> > index 786413930a..a3cd351f6e 100644
> > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
> > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
> > @@ -96,7 +96,7 @@ typedef union {
> > #define EFI_AHCI_PORT_IS 0x0010
> > #define EFI_AHCI_PORT_IS_DHRS BIT0
> > #define EFI_AHCI_PORT_IS_PSS BIT1
> > -#define EFI_AHCI_PORT_IS_SSS BIT2
> > +#define EFI_AHCI_PORT_IS_DSS BIT2
> > #define EFI_AHCI_PORT_IS_SDBS BIT3
> > #define EFI_AHCI_PORT_IS_UFS BIT4
> > #define EFI_AHCI_PORT_IS_DPS BIT5
> > @@ -113,6 +113,7 @@ typedef union {
> > #define EFI_AHCI_PORT_IS_CPDS BIT31
> > #define EFI_AHCI_PORT_IS_CLEAR 0xFFFFFFFF
> > #define EFI_AHCI_PORT_IS_FIS_CLEAR 0x0000000F
> > +#define EFI_AHCI_PORT_IS_ERROR_MASK
> (EFI_AHCI_PORT_IS_INFS
> > | EFI_AHCI_PORT_IS_IFS | EFI_AHCI_PORT_IS_HBDS |
> > EFI_AHCI_PORT_IS_HBFS | EFI_AHCI_PORT_IS_TFES)
> >
> > #define EFI_AHCI_PORT_IE 0x0014
> > #define EFI_AHCI_PORT_CMD 0x0018
> > @@ -240,6 +241,12 @@ typedef struct {
> > UINT8 AhciCFisRsvd5[44];
> > } EFI_AHCI_COMMAND_FIS;
> >
> > +typedef enum {
> > + SataFisD2H = 0,
> > + SataFisPioSetup,
> > + SataFisDmaSetup
> > +} SATA_FIS_TYPE;
> > +
> > //
> > // ACMD: ATAPI command (12 or 16 bytes) //
> > --
> > 2.28.0.windows.1
---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Sowackiego 173 | 80-298 Gdask | Sd Rejonowy Gdask Pnoc | VII Wydzia Gospodarczy Krajowego Rejestru Sdowego - KRS 101882 | NIP 957-07-52-316 | Kapita zakadowy 200.000 PLN.
Ta wiadomo wraz z zacznikami jest przeznaczona dla okrelonego adresata i moe zawiera informacje poufne. W razie przypadkowego otrzymania tej wiadomoci, prosimy o powiadomienie nadawcy oraz trwae jej usunicie; jakiekolwiek przegldanie 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] 11+ messages in thread
* [PATCH 2/4] MdeModulePkg/AtaAtapiPassThru: Add SATA error recovery flow
2020-11-03 13:23 [PATCH 0/4] MdeModulePkg/AtaAtapiPassThru: Add SATA error recovery Albecki, Mateusz
2020-11-03 13:23 ` [PATCH 1/4] MdeModulePkg/AtaAtapiPassThru: Check IS to check for command completion Albecki, Mateusz
@ 2020-11-03 13:23 ` Albecki, Mateusz
2020-11-04 5:33 ` Wu, Hao A
2020-11-03 13:23 ` [PATCH 3/4] MdeModulePkg/AtaAtapiPassThru: Restart failed packets Albecki, Mateusz
` (2 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Albecki, Mateusz @ 2020-11-03 13:23 UTC (permalink / raw)
To: devel; +Cc: Albecki, Ray Ni, Hao A Wu
From: Albecki <mateusz.albecki@intel.com>
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3025
This commit adds error recovery flow on SATA port when the error
condition is reported. Commit only implements SATA port reset flow which
is executed when PxTFD indicates BSY or DRQ. Commit does not implement
HBA level reset.
Signed-off-by: Mateusz Albecki <mateusz.albecki@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
---
.../Bus/Ata/AtaAtapiPassThru/AhciMode.c | 178 +++++++++++++++---
.../Bus/Ata/AtaAtapiPassThru/AhciMode.h | 5 +-
2 files changed, 159 insertions(+), 24 deletions(-)
diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
index 4b42e72226..cf735d5983 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
@@ -608,6 +608,148 @@ AhciBuildCommandFis (
CmdFis->AhciCFisDevHead = (UINT8) (AtaCommandBlock->AtaDeviceHead | 0xE0);
}
+/**
+ Wait until SATA device reports it is ready for operation.
+
+ @param[in] PciIo Pointer to AHCI controller PciIo.
+ @param[in] Port SATA port index on which to reset.
+
+ @retval EFI_SUCCESS Device ready for operation.
+ @retval EFI_TIMEOUT Device failed to get ready within required period.
+**/
+EFI_STATUS
+AhciWaitDeviceReady (
+ IN EFI_PCI_IO_PROTOCOL *PciIo,
+ IN UINT8 Port
+ )
+{
+ UINT32 PhyDetectDelay;
+ UINT32 Data;
+ UINT32 Offset;
+
+ //
+ // According to SATA1.0a spec section 5.2, we need to wait for PxTFD.BSY and PxTFD.DRQ
+ // and PxTFD.ERR to be zero. The maximum wait time is 16s which is defined at ATA spec.
+ //
+ PhyDetectDelay = 16 * 1000;
+ do {
+ Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH + EFI_AHCI_PORT_SERR;
+ if (AhciReadReg(PciIo, Offset) != 0) {
+ AhciWriteReg (PciIo, Offset, AhciReadReg(PciIo, Offset));
+ }
+ Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH + EFI_AHCI_PORT_TFD;
+
+ Data = AhciReadReg (PciIo, Offset) & EFI_AHCI_PORT_TFD_MASK;
+ if (Data == 0) {
+ break;
+ }
+
+ MicroSecondDelay (1000);
+ PhyDetectDelay--;
+ } while (PhyDetectDelay > 0);
+
+ if (PhyDetectDelay == 0) {
+ DEBUG ((DEBUG_ERROR, "Port %d Device not ready (TFD=0x%X)\n", Port, Data));
+ return EFI_TIMEOUT;
+ } else {
+ return EFI_SUCCESS;
+ }
+}
+
+
+/**
+ Reset the SATA port. Algorithm follows AHCI spec 1.3.1 section 10.4.2
+
+ @param[in] PciIo Pointer to AHCI controller PciIo.
+ @param[in] Port SATA port index on which to reset.
+
+ @retval EFI_SUCCESS Port reset.
+ @retval Others Failed to reset the port.
+**/
+EFI_STATUS
+AhciResetPort (
+ IN EFI_PCI_IO_PROTOCOL *PciIo,
+ IN UINT8 Port
+ )
+{
+ UINT32 Offset;
+ EFI_STATUS Status;
+
+ Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH + EFI_AHCI_PORT_SCTL;
+ AhciOrReg (PciIo, Offset, EFI_AHCI_PORT_SCTL_DET_INIT);
+ //
+ // SW is required to keep DET set to 0x1 at least for 1 milisecond to ensure that
+ // at least one COMRESET signal is sent.
+ //
+ MicroSecondDelay(1000);
+ AhciAndReg (PciIo, Offset, ~(UINT32)EFI_AHCI_PORT_SSTS_DET_MASK);
+
+ Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH + EFI_AHCI_PORT_SSTS;
+ Status = AhciWaitMmioSet (PciIo, Offset, EFI_AHCI_PORT_SSTS_DET_MASK, EFI_AHCI_PORT_SSTS_DET_PCE, ATA_ATAPI_TIMEOUT);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ return AhciWaitDeviceReady (PciIo, Port);
+}
+
+/**
+ Recovers the SATA port from error condition.
+ This function implements algorithm described in
+ AHCI spec 1.3.1 section 6.2.2
+
+ @param[in] PciIo Pointer to AHCI controller PciIo.
+ @param[in] Port SATA port index on which to check.
+
+ @retval EFI_SUCCESS Port recovered.
+ @retval Others Failed to recover port.
+**/
+EFI_STATUS
+AhciRecoverPortError (
+ IN EFI_PCI_IO_PROTOCOL *PciIo,
+ IN UINT8 Port
+ )
+{
+ UINT32 Offset;
+ UINT32 PortInterrupt;
+ UINT32 PortTfd;
+ EFI_STATUS Status;
+
+ Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH + EFI_AHCI_PORT_IS;
+ PortInterrupt = AhciReadReg (PciIo, Offset);
+ if ((PortInterrupt & EFI_AHCI_PORT_IS_FATAL_ERROR_MASK) == 0) {
+ //
+ // No fatal error detected. Exit with success as port should still be operational.
+ // No need to clear IS as it will be cleared when the next command starts.
+ //
+ return EFI_SUCCESS;
+ }
+
+ Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH + EFI_AHCI_PORT_CMD;
+ AhciAndReg (PciIo, Offset, ~(UINT32)EFI_AHCI_PORT_CMD_ST);
+
+ Status = AhciWaitMmioSet (PciIo, Offset, EFI_AHCI_PORT_CMD_CR, 0, ATA_ATAPI_TIMEOUT);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "Ahci port %d is in hung state, aborting recovery\n", Port));
+ return Status;
+ }
+
+ //
+ // If TFD.BSY or TFD.DRQ is still set it means that drive is hung and software has
+ // to reset it before sending any additional commands.
+ //
+ Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH + EFI_AHCI_PORT_TFD;
+ PortTfd = AhciReadReg (PciIo, Offset);
+ if ((PortTfd & (EFI_AHCI_PORT_TFD_BSY | EFI_AHCI_PORT_TFD_DRQ)) != 0) {
+ Status = AhciResetPort (PciIo, Port);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "Failed to reset the port %d\n", Port));
+ }
+ }
+
+ return EFI_SUCCESS;
+}
+
/**
Checks if specified FIS has been received.
@@ -827,6 +969,10 @@ AhciPioTransfer (
Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout, SataFisD2H);
}
+ if (Status == EFI_DEVICE_ERROR) {
+ AhciRecoverPortError (PciIo, Port);
+ }
+
Exit:
AhciStopCommand (
PciIo,
@@ -1007,6 +1153,10 @@ AhciDmaTransfer (
Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout, SataFisD2H);
}
+ if (Status == EFI_DEVICE_ERROR) {
+ AhciRecoverPortError (PciIo, Port);
+ }
+
Exit:
//
// For Blocking mode, the command should be stopped, the Fis should be disabled
@@ -1119,6 +1269,9 @@ AhciNonDataTransfer (
}
Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout, SataFisD2H);
+ if (Status == EFI_DEVICE_ERROR) {
+ AhciRecoverPortError (PciIo, Port);
+ }
Exit:
AhciStopCommand (
@@ -2583,29 +2736,8 @@ AhciModeInitialization (
continue;
}
- //
- // According to SATA1.0a spec section 5.2, we need to wait for PxTFD.BSY and PxTFD.DRQ
- // and PxTFD.ERR to be zero. The maximum wait time is 16s which is defined at ATA spec.
- //
- PhyDetectDelay = 16 * 1000;
- do {
- Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH + EFI_AHCI_PORT_SERR;
- if (AhciReadReg(PciIo, Offset) != 0) {
- AhciWriteReg (PciIo, Offset, AhciReadReg(PciIo, Offset));
- }
- Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH + EFI_AHCI_PORT_TFD;
-
- Data = AhciReadReg (PciIo, Offset) & EFI_AHCI_PORT_TFD_MASK;
- if (Data == 0) {
- break;
- }
-
- MicroSecondDelay (1000);
- PhyDetectDelay--;
- } while (PhyDetectDelay > 0);
-
- if (PhyDetectDelay == 0) {
- DEBUG ((EFI_D_ERROR, "Port %d Device presence detected but phy not ready (TFD=0x%X)\n", Port, Data));
+ Status = AhciWaitDeviceReady (PciIo, Port);
+ if (EFI_ERROR (Status)) {
continue;
}
diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
index a3cd351f6e..4d2aafe483 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
@@ -114,6 +114,7 @@ typedef union {
#define EFI_AHCI_PORT_IS_CLEAR 0xFFFFFFFF
#define EFI_AHCI_PORT_IS_FIS_CLEAR 0x0000000F
#define EFI_AHCI_PORT_IS_ERROR_MASK (EFI_AHCI_PORT_IS_INFS | EFI_AHCI_PORT_IS_IFS | EFI_AHCI_PORT_IS_HBDS | EFI_AHCI_PORT_IS_HBFS | EFI_AHCI_PORT_IS_TFES)
+#define EFI_AHCI_PORT_IS_FATAL_ERROR_MASK (EFI_AHCI_PORT_IS_IFS | EFI_AHCI_PORT_IS_HBDS | EFI_AHCI_PORT_IS_HBFS | EFI_AHCI_PORT_IS_TFES)
#define EFI_AHCI_PORT_IE 0x0014
#define EFI_AHCI_PORT_CMD 0x0018
@@ -122,9 +123,11 @@ typedef union {
#define EFI_AHCI_PORT_CMD_SUD BIT1
#define EFI_AHCI_PORT_CMD_POD BIT2
#define EFI_AHCI_PORT_CMD_CLO BIT3
-#define EFI_AHCI_PORT_CMD_CR BIT15
#define EFI_AHCI_PORT_CMD_FRE BIT4
+#define EFI_AHCI_PORT_CMD_CCS_MASK (BIT8 | BIT9 | BIT10 | BIT11 | BIT12)
+#define EFI_AHCI_PORT_CMD_CCS_SHIFT 8
#define EFI_AHCI_PORT_CMD_FR BIT14
+#define EFI_AHCI_PORT_CMD_CR BIT15
#define EFI_AHCI_PORT_CMD_MASK ~(EFI_AHCI_PORT_CMD_ST | EFI_AHCI_PORT_CMD_FRE | EFI_AHCI_PORT_CMD_COL)
#define EFI_AHCI_PORT_CMD_PMA BIT17
#define EFI_AHCI_PORT_CMD_HPCP BIT18
--
2.28.0.windows.1
---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Sowackiego 173 | 80-298 Gdask | Sd Rejonowy Gdask Pnoc | VII Wydzia Gospodarczy Krajowego Rejestru Sdowego - KRS 101882 | NIP 957-07-52-316 | Kapita zakadowy 200.000 PLN.
Ta wiadomo wraz z zacznikami jest przeznaczona dla okrelonego adresata i moe zawiera informacje poufne. W razie przypadkowego otrzymania tej wiadomoci, prosimy o powiadomienie nadawcy oraz trwae jej usunicie; jakiekolwiek przegldanie 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] 11+ messages in thread* Re: [PATCH 2/4] MdeModulePkg/AtaAtapiPassThru: Add SATA error recovery flow
2020-11-03 13:23 ` [PATCH 2/4] MdeModulePkg/AtaAtapiPassThru: Add SATA error recovery flow Albecki, Mateusz
@ 2020-11-04 5:33 ` Wu, Hao A
0 siblings, 0 replies; 11+ messages in thread
From: Wu, Hao A @ 2020-11-04 5:33 UTC (permalink / raw)
To: Albecki, Mateusz, devel@edk2.groups.io; +Cc: Ni, Ray
> -----Original Message-----
> From: Albecki, Mateusz <mateusz.albecki@intel.com>
> Sent: Tuesday, November 3, 2020 9:24 PM
> To: devel@edk2.groups.io
> Cc: Albecki, Mateusz <mateusz.albecki@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> Subject: [PATCH 2/4] MdeModulePkg/AtaAtapiPassThru: Add SATA error
> recovery flow
>
> From: Albecki <mateusz.albecki@intel.com>
Similar to patch 1, please help to check the author information for the patch.
The patch itself looks good to me:
Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
Best Regards,
Hao Wu
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3025
>
> This commit adds error recovery flow on SATA port when the error condition
> is reported. Commit only implements SATA port reset flow which is executed
> when PxTFD indicates BSY or DRQ. Commit does not implement HBA level
> reset.
>
> Signed-off-by: Mateusz Albecki <mateusz.albecki@intel.com>
>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
>
> ---
> .../Bus/Ata/AtaAtapiPassThru/AhciMode.c | 178 +++++++++++++++---
> .../Bus/Ata/AtaAtapiPassThru/AhciMode.h | 5 +-
> 2 files changed, 159 insertions(+), 24 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> index 4b42e72226..cf735d5983 100644
> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> @@ -608,6 +608,148 @@ AhciBuildCommandFis (
> CmdFis->AhciCFisDevHead = (UINT8) (AtaCommandBlock-
> >AtaDeviceHead | 0xE0);
> }
>
> +/**
> + Wait until SATA device reports it is ready for operation.
> +
> + @param[in] PciIo Pointer to AHCI controller PciIo.
> + @param[in] Port SATA port index on which to reset.
> +
> + @retval EFI_SUCCESS Device ready for operation.
> + @retval EFI_TIMEOUT Device failed to get ready within required period.
> +**/
> +EFI_STATUS
> +AhciWaitDeviceReady (
> + IN EFI_PCI_IO_PROTOCOL *PciIo,
> + IN UINT8 Port
> + )
> +{
> + UINT32 PhyDetectDelay;
> + UINT32 Data;
> + UINT32 Offset;
> +
> + //
> + // According to SATA1.0a spec section 5.2, we need to wait for
> + PxTFD.BSY and PxTFD.DRQ // and PxTFD.ERR to be zero. The maximum
> wait time is 16s which is defined at ATA spec.
> + //
> + PhyDetectDelay = 16 * 1000;
> + do {
> + Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH +
> EFI_AHCI_PORT_SERR;
> + if (AhciReadReg(PciIo, Offset) != 0) {
> + AhciWriteReg (PciIo, Offset, AhciReadReg(PciIo, Offset));
> + }
> + Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH +
> + EFI_AHCI_PORT_TFD;
> +
> + Data = AhciReadReg (PciIo, Offset) & EFI_AHCI_PORT_TFD_MASK;
> + if (Data == 0) {
> + break;
> + }
> +
> + MicroSecondDelay (1000);
> + PhyDetectDelay--;
> + } while (PhyDetectDelay > 0);
> +
> + if (PhyDetectDelay == 0) {
> + DEBUG ((DEBUG_ERROR, "Port %d Device not ready (TFD=0x%X)\n", Port,
> Data));
> + return EFI_TIMEOUT;
> + } else {
> + return EFI_SUCCESS;
> + }
> +}
> +
> +
> +/**
> + Reset the SATA port. Algorithm follows AHCI spec 1.3.1 section 10.4.2
> +
> + @param[in] PciIo Pointer to AHCI controller PciIo.
> + @param[in] Port SATA port index on which to reset.
> +
> + @retval EFI_SUCCESS Port reset.
> + @retval Others Failed to reset the port.
> +**/
> +EFI_STATUS
> +AhciResetPort (
> + IN EFI_PCI_IO_PROTOCOL *PciIo,
> + IN UINT8 Port
> + )
> +{
> + UINT32 Offset;
> + EFI_STATUS Status;
> +
> + Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH +
> + EFI_AHCI_PORT_SCTL; AhciOrReg (PciIo, Offset,
> + EFI_AHCI_PORT_SCTL_DET_INIT); // // SW is required to keep DET set
> + to 0x1 at least for 1 milisecond to ensure that // at least one
> + COMRESET signal is sent.
> + //
> + MicroSecondDelay(1000);
> + AhciAndReg (PciIo, Offset, ~(UINT32)EFI_AHCI_PORT_SSTS_DET_MASK);
> +
> + Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH +
> + EFI_AHCI_PORT_SSTS; Status = AhciWaitMmioSet (PciIo, Offset,
> + EFI_AHCI_PORT_SSTS_DET_MASK, EFI_AHCI_PORT_SSTS_DET_PCE,
> ATA_ATAPI_TIMEOUT); if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + return AhciWaitDeviceReady (PciIo, Port); }
> +
> +/**
> + Recovers the SATA port from error condition.
> + This function implements algorithm described in
> + AHCI spec 1.3.1 section 6.2.2
> +
> + @param[in] PciIo Pointer to AHCI controller PciIo.
> + @param[in] Port SATA port index on which to check.
> +
> + @retval EFI_SUCCESS Port recovered.
> + @retval Others Failed to recover port.
> +**/
> +EFI_STATUS
> +AhciRecoverPortError (
> + IN EFI_PCI_IO_PROTOCOL *PciIo,
> + IN UINT8 Port
> + )
> +{
> + UINT32 Offset;
> + UINT32 PortInterrupt;
> + UINT32 PortTfd;
> + EFI_STATUS Status;
> +
> + Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH +
> + EFI_AHCI_PORT_IS; PortInterrupt = AhciReadReg (PciIo, Offset); if
> + ((PortInterrupt & EFI_AHCI_PORT_IS_FATAL_ERROR_MASK) == 0) {
> + //
> + // No fatal error detected. Exit with success as port should still be
> operational.
> + // No need to clear IS as it will be cleared when the next command starts.
> + //
> + return EFI_SUCCESS;
> + }
> +
> + Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH +
> + EFI_AHCI_PORT_CMD; AhciAndReg (PciIo, Offset,
> + ~(UINT32)EFI_AHCI_PORT_CMD_ST);
> +
> + Status = AhciWaitMmioSet (PciIo, Offset, EFI_AHCI_PORT_CMD_CR, 0,
> + ATA_ATAPI_TIMEOUT); if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "Ahci port %d is in hung state, aborting
> recovery\n", Port));
> + return Status;
> + }
> +
> + //
> + // If TFD.BSY or TFD.DRQ is still set it means that drive is hung and
> + software has // to reset it before sending any additional commands.
> + //
> + Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH +
> + EFI_AHCI_PORT_TFD; PortTfd = AhciReadReg (PciIo, Offset); if
> + ((PortTfd & (EFI_AHCI_PORT_TFD_BSY | EFI_AHCI_PORT_TFD_DRQ)) != 0)
> {
> + Status = AhciResetPort (PciIo, Port);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "Failed to reset the port %d\n", Port));
> + }
> + }
> +
> + return EFI_SUCCESS;
> +}
> +
> /**
> Checks if specified FIS has been received.
>
> @@ -827,6 +969,10 @@ AhciPioTransfer (
> Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout, SataFisD2H);
> }
>
> + if (Status == EFI_DEVICE_ERROR) {
> + AhciRecoverPortError (PciIo, Port); }
> +
> Exit:
> AhciStopCommand (
> PciIo,
> @@ -1007,6 +1153,10 @@ AhciDmaTransfer (
> Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout, SataFisD2H);
> }
>
> + if (Status == EFI_DEVICE_ERROR) {
> + AhciRecoverPortError (PciIo, Port); }
> +
> Exit:
> //
> // For Blocking mode, the command should be stopped, the Fis should be
> disabled @@ -1119,6 +1269,9 @@ AhciNonDataTransfer (
> }
>
> Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout, SataFisD2H);
> + if (Status == EFI_DEVICE_ERROR) {
> + AhciRecoverPortError (PciIo, Port); }
>
> Exit:
> AhciStopCommand (
> @@ -2583,29 +2736,8 @@ AhciModeInitialization (
> continue;
> }
>
> - //
> - // According to SATA1.0a spec section 5.2, we need to wait for PxTFD.BSY
> and PxTFD.DRQ
> - // and PxTFD.ERR to be zero. The maximum wait time is 16s which is
> defined at ATA spec.
> - //
> - PhyDetectDelay = 16 * 1000;
> - do {
> - Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH +
> EFI_AHCI_PORT_SERR;
> - if (AhciReadReg(PciIo, Offset) != 0) {
> - AhciWriteReg (PciIo, Offset, AhciReadReg(PciIo, Offset));
> - }
> - Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH +
> EFI_AHCI_PORT_TFD;
> -
> - Data = AhciReadReg (PciIo, Offset) & EFI_AHCI_PORT_TFD_MASK;
> - if (Data == 0) {
> - break;
> - }
> -
> - MicroSecondDelay (1000);
> - PhyDetectDelay--;
> - } while (PhyDetectDelay > 0);
> -
> - if (PhyDetectDelay == 0) {
> - DEBUG ((EFI_D_ERROR, "Port %d Device presence detected but phy not
> ready (TFD=0x%X)\n", Port, Data));
> + Status = AhciWaitDeviceReady (PciIo, Port);
> + if (EFI_ERROR (Status)) {
> continue;
> }
>
> diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
> index a3cd351f6e..4d2aafe483 100644
> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
> @@ -114,6 +114,7 @@ typedef union {
> #define EFI_AHCI_PORT_IS_CLEAR 0xFFFFFFFF
> #define EFI_AHCI_PORT_IS_FIS_CLEAR 0x0000000F
> #define EFI_AHCI_PORT_IS_ERROR_MASK (EFI_AHCI_PORT_IS_INFS |
> EFI_AHCI_PORT_IS_IFS | EFI_AHCI_PORT_IS_HBDS |
> EFI_AHCI_PORT_IS_HBFS | EFI_AHCI_PORT_IS_TFES)
> +#define EFI_AHCI_PORT_IS_FATAL_ERROR_MASK
> (EFI_AHCI_PORT_IS_IFS | EFI_AHCI_PORT_IS_HBDS |
> EFI_AHCI_PORT_IS_HBFS | EFI_AHCI_PORT_IS_TFES)
>
> #define EFI_AHCI_PORT_IE 0x0014
> #define EFI_AHCI_PORT_CMD 0x0018
> @@ -122,9 +123,11 @@ typedef union {
> #define EFI_AHCI_PORT_CMD_SUD BIT1
> #define EFI_AHCI_PORT_CMD_POD BIT2
> #define EFI_AHCI_PORT_CMD_CLO BIT3
> -#define EFI_AHCI_PORT_CMD_CR BIT15
> #define EFI_AHCI_PORT_CMD_FRE BIT4
> +#define EFI_AHCI_PORT_CMD_CCS_MASK (BIT8 | BIT9 | BIT10 |
> BIT11 | BIT12)
> +#define EFI_AHCI_PORT_CMD_CCS_SHIFT 8
> #define EFI_AHCI_PORT_CMD_FR BIT14
> +#define EFI_AHCI_PORT_CMD_CR BIT15
> #define EFI_AHCI_PORT_CMD_MASK ~(EFI_AHCI_PORT_CMD_ST |
> EFI_AHCI_PORT_CMD_FRE | EFI_AHCI_PORT_CMD_COL)
> #define EFI_AHCI_PORT_CMD_PMA BIT17
> #define EFI_AHCI_PORT_CMD_HPCP BIT18
> --
> 2.28.0.windows.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/4] MdeModulePkg/AtaAtapiPassThru: Restart failed packets
2020-11-03 13:23 [PATCH 0/4] MdeModulePkg/AtaAtapiPassThru: Add SATA error recovery Albecki, Mateusz
2020-11-03 13:23 ` [PATCH 1/4] MdeModulePkg/AtaAtapiPassThru: Check IS to check for command completion Albecki, Mateusz
2020-11-03 13:23 ` [PATCH 2/4] MdeModulePkg/AtaAtapiPassThru: Add SATA error recovery flow Albecki, Mateusz
@ 2020-11-03 13:23 ` Albecki, Mateusz
2020-11-04 5:33 ` Wu, Hao A
2020-11-03 13:23 ` [PATCH 4/4] MdeModulePkg/AtaAtapiPassThru: Trace ATA packets Albecki, Mateusz
2020-11-04 1:57 ` [PATCH 0/4] MdeModulePkg/AtaAtapiPassThru: Add SATA error recovery Wu, Hao A
4 siblings, 1 reply; 11+ messages in thread
From: Albecki, Mateusz @ 2020-11-03 13:23 UTC (permalink / raw)
To: devel; +Cc: Albecki, Ray Ni, Hao A Wu
From: Albecki <mateusz.albecki@intel.com>
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3026
This commit adds code to restart the ATA packets that failed due to the
CRC error or other link condition. For sync transfers the code will try
to get the command working for up to 5 times and for async in accordance
to the RetryTimes field in Task structure. For sync case the count of 5
retries has been chosen arbitrarily and if needed can be increased or
decreased.
Signed-off-by: Mateusz Albecki <mateusz.albecki@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
---
.../Bus/Ata/AtaAtapiPassThru/AhciMode.c | 305 +++++++++++-------
.../Bus/Ata/AtaAtapiPassThru/AhciMode.h | 2 +
2 files changed, 182 insertions(+), 125 deletions(-)
diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
index cf735d5983..4fe7e4b1dc 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
@@ -897,6 +897,7 @@ AhciPioTransfer (
EFI_AHCI_COMMAND_FIS CFis;
EFI_AHCI_COMMAND_LIST CmdList;
UINT32 PrdCount;
+ UINT32 Retry;
if (Read) {
Flag = EfiPciIoOperationBusMasterWrite;
@@ -931,49 +932,56 @@ AhciPioTransfer (
CmdList.AhciCmdCfl = EFI_AHCI_FIS_REGISTER_H2D_LENGTH / 4;
CmdList.AhciCmdW = Read ? 0 : 1;
- AhciBuildCommand (
- PciIo,
- AhciRegisters,
- Port,
- PortMultiplier,
- &CFis,
- &CmdList,
- AtapiCommand,
- AtapiCommandLength,
- 0,
- (VOID *)(UINTN)PhyAddr,
- DataCount
- );
+ for (Retry = 0; Retry < AHCI_COMMAND_RETRIES; Retry++) {
+ AhciBuildCommand (
+ PciIo,
+ AhciRegisters,
+ Port,
+ PortMultiplier,
+ &CFis,
+ &CmdList,
+ AtapiCommand,
+ AtapiCommandLength,
+ 0,
+ (VOID *)(UINTN)PhyAddr,
+ DataCount
+ );
- Status = AhciStartCommand (
- PciIo,
- Port,
- 0,
- Timeout
- );
- if (EFI_ERROR (Status)) {
- goto Exit;
- }
+ Status = AhciStartCommand (
+ PciIo,
+ Port,
+ 0,
+ Timeout
+ );
+ if (EFI_ERROR (Status)) {
+ break;
+ }
- if (Read && (AtapiCommand == 0)) {
- Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout, SataFisPioSetup);
- if (Status == EFI_SUCCESS) {
- PrdCount = *(volatile UINT32 *) (&(AhciRegisters->AhciCmdList[0].AhciCmdPrdbc));
- if (PrdCount == DataCount) {
- Status = EFI_SUCCESS;
- } else {
- Status = EFI_DEVICE_ERROR;
+ if (Read && (AtapiCommand == 0)) {
+ Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout, SataFisPioSetup);
+ if (Status == EFI_SUCCESS) {
+ PrdCount = *(volatile UINT32 *) (&(AhciRegisters->AhciCmdList[0].AhciCmdPrdbc));
+ if (PrdCount == DataCount) {
+ Status = EFI_SUCCESS;
+ } else {
+ Status = EFI_DEVICE_ERROR;
+ }
}
+ } else {
+ Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout, SataFisD2H);
}
- } else {
- Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout, SataFisD2H);
- }
- if (Status == EFI_DEVICE_ERROR) {
- AhciRecoverPortError (PciIo, Port);
+ if (Status == EFI_DEVICE_ERROR) {
+ DEBUG ((DEBUG_ERROR, "PIO command failed at retry %d\n", Retry));
+ Status = AhciRecoverPortError (PciIo, Port);
+ if (EFI_ERROR (Status)) {
+ break;
+ }
+ } else {
+ break;
+ }
}
-Exit:
AhciStopCommand (
PciIo,
Port,
@@ -992,7 +1000,6 @@ Exit:
);
AhciDumpPortStatus (PciIo, AhciRegisters, Port, AtaStatusBlock);
-
return Status;
}
@@ -1046,9 +1053,9 @@ AhciDmaTransfer (
EFI_PCI_IO_PROTOCOL_OPERATION Flag;
EFI_AHCI_COMMAND_FIS CFis;
EFI_AHCI_COMMAND_LIST CmdList;
-
EFI_PCI_IO_PROTOCOL *PciIo;
EFI_TPL OldTpl;
+ UINT32 Retry;
Map = NULL;
PciIo = Instance->PciIo;
@@ -1058,36 +1065,16 @@ AhciDmaTransfer (
}
//
- // Before starting the Blocking BlockIO operation, push to finish all non-blocking
- // BlockIO tasks.
- // Delay 100us to simulate the blocking time out checking.
+ // DMA buffer allocation. Needs to be done only once for both sync and async
+ // DMA transfers irrespective of number of retries.
//
- OldTpl = gBS->RaiseTPL (TPL_NOTIFY);
- while ((Task == NULL) && (!IsListEmpty (&Instance->NonBlockingTaskList))) {
- AsyncNonBlockingTransferRoutine (NULL, Instance);
- //
- // Stall for 100us.
- //
- MicroSecondDelay (100);
- }
- gBS->RestoreTPL (OldTpl);
-
- if ((Task == NULL) || ((Task != NULL) && (!Task->IsStart))) {
- //
- // Mark the Task to indicate that it has been started.
- //
- if (Task != NULL) {
- Task->IsStart = TRUE;
- }
+ if ((Task == NULL) || ((Task != NULL) && (Task->Map == NULL))) {
if (Read) {
Flag = EfiPciIoOperationBusMasterWrite;
} else {
Flag = EfiPciIoOperationBusMasterRead;
}
- //
- // Construct command list and command table with pci bus address.
- //
MapLength = DataCount;
Status = PciIo->Map (
PciIo,
@@ -1101,63 +1088,123 @@ AhciDmaTransfer (
if (EFI_ERROR (Status) || (DataCount != MapLength)) {
return EFI_BAD_BUFFER_SIZE;
}
-
if (Task != NULL) {
Task->Map = Map;
}
- //
- // Package read needed
- //
+ }
+
+ if (Task == NULL || (Task != NULL && !Task->IsStart)) {
AhciBuildCommandFis (&CFis, AtaCommandBlock);
ZeroMem (&CmdList, sizeof (EFI_AHCI_COMMAND_LIST));
CmdList.AhciCmdCfl = EFI_AHCI_FIS_REGISTER_H2D_LENGTH / 4;
CmdList.AhciCmdW = Read ? 0 : 1;
+ }
- AhciBuildCommand (
- PciIo,
- AhciRegisters,
- Port,
- PortMultiplier,
- &CFis,
- &CmdList,
- AtapiCommand,
- AtapiCommandLength,
- 0,
- (VOID *)(UINTN)PhyAddr,
- DataCount
- );
-
- Status = AhciStartCommand (
- PciIo,
- Port,
- 0,
- Timeout
- );
- if (EFI_ERROR (Status)) {
- goto Exit;
+ if (Task == NULL) {
+ //
+ // Before starting the Blocking BlockIO operation, push to finish all non-blocking
+ // BlockIO tasks.
+ // Delay 100us to simulate the blocking time out checking.
+ //
+ OldTpl = gBS->RaiseTPL (TPL_NOTIFY);
+ while (!IsListEmpty (&Instance->NonBlockingTaskList)) {
+ AsyncNonBlockingTransferRoutine (NULL, Instance);
+ //
+ // Stall for 100us.
+ //
+ MicroSecondDelay (100);
}
- }
+ gBS->RestoreTPL (OldTpl);
+ for (Retry = 0; Retry < AHCI_COMMAND_RETRIES; Retry++) {
+ AhciBuildCommand (
+ PciIo,
+ AhciRegisters,
+ Port,
+ PortMultiplier,
+ &CFis,
+ &CmdList,
+ AtapiCommand,
+ AtapiCommandLength,
+ 0,
+ (VOID *)(UINTN)PhyAddr,
+ DataCount
+ );
- if (Task != NULL) {
- Status = AhciCheckFisReceived (PciIo, Port, SataFisD2H);
- if (Status == EFI_NOT_READY) {
- if (!Task->InfiniteWait && Task->RetryTimes == 0) {
- Status = EFI_TIMEOUT;
+ Status = AhciStartCommand (
+ PciIo,
+ Port,
+ 0,
+ Timeout
+ );
+ if (EFI_ERROR (Status)) {
+ break;
+ }
+ 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)) {
+ break;
+ }
} else {
- Task->RetryTimes--;
+ break;
}
}
} else {
- Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout, SataFisD2H);
- }
+ if (!Task->IsStart) {
+ AhciBuildCommand (
+ PciIo,
+ AhciRegisters,
+ Port,
+ PortMultiplier,
+ &CFis,
+ &CmdList,
+ AtapiCommand,
+ AtapiCommandLength,
+ 0,
+ (VOID *)(UINTN)PhyAddr,
+ DataCount
+ );
- if (Status == EFI_DEVICE_ERROR) {
- AhciRecoverPortError (PciIo, Port);
+ Status = AhciStartCommand (
+ PciIo,
+ Port,
+ 0,
+ Timeout
+ );
+ if (!EFI_ERROR (Status)) {
+ Task->IsStart = TRUE;
+ }
+ }
+ if (Task->IsStart) {
+ 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);
+ //
+ // 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) {
+ Task->IsStart = FALSE;
+ Status = EFI_NOT_READY;
+ }
+ }
+
+ if (Status == EFI_NOT_READY) {
+ if (!Task->InfiniteWait && Task->RetryTimes == 0) {
+ Status = EFI_TIMEOUT;
+ } else {
+ Task->RetryTimes--;
+ }
+ }
+ }
}
-Exit:
//
// For Blocking mode, the command should be stopped, the Fis should be disabled
// and the PciIo should be unmapped.
@@ -1234,6 +1281,7 @@ AhciNonDataTransfer (
EFI_STATUS Status;
EFI_AHCI_COMMAND_FIS CFis;
EFI_AHCI_COMMAND_LIST CmdList;
+ UINT32 Retry;
//
// Package read needed
@@ -1244,36 +1292,43 @@ AhciNonDataTransfer (
CmdList.AhciCmdCfl = EFI_AHCI_FIS_REGISTER_H2D_LENGTH / 4;
- AhciBuildCommand (
- PciIo,
- AhciRegisters,
- Port,
- PortMultiplier,
- &CFis,
- &CmdList,
- AtapiCommand,
- AtapiCommandLength,
- 0,
- NULL,
- 0
- );
+ for (Retry = 0; Retry < AHCI_COMMAND_RETRIES; Retry++) {
+ AhciBuildCommand (
+ PciIo,
+ AhciRegisters,
+ Port,
+ PortMultiplier,
+ &CFis,
+ &CmdList,
+ AtapiCommand,
+ AtapiCommandLength,
+ 0,
+ NULL,
+ 0
+ );
- Status = AhciStartCommand (
- PciIo,
- Port,
- 0,
- Timeout
- );
- if (EFI_ERROR (Status)) {
- goto Exit;
- }
+ Status = AhciStartCommand (
+ PciIo,
+ Port,
+ 0,
+ Timeout
+ );
+ if (EFI_ERROR (Status)) {
+ break;
+ }
- Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout, SataFisD2H);
- if (Status == EFI_DEVICE_ERROR) {
- AhciRecoverPortError (PciIo, Port);
+ 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)) {
+ break;
+ }
+ } else {
+ break;
+ }
}
-Exit:
AhciStopCommand (
PciIo,
Port,
diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
index 4d2aafe483..0540b0f4fa 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
@@ -192,6 +192,8 @@ typedef union {
#define AHCI_PORT_DEVSLP_DITO_MASK 0x01FF8000
#define AHCI_PORT_DEVSLP_DM_MASK 0x1E000000
+#define AHCI_COMMAND_RETRIES 5
+
#pragma pack(1)
//
// Command List structure includes total 32 entries.
--
2.28.0.windows.1
---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Sowackiego 173 | 80-298 Gdask | Sd Rejonowy Gdask Pnoc | VII Wydzia Gospodarczy Krajowego Rejestru Sdowego - KRS 101882 | NIP 957-07-52-316 | Kapita zakadowy 200.000 PLN.
Ta wiadomo wraz z zacznikami jest przeznaczona dla okrelonego adresata i moe zawiera informacje poufne. W razie przypadkowego otrzymania tej wiadomoci, prosimy o powiadomienie nadawcy oraz trwae jej usunicie; jakiekolwiek przegldanie 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] 11+ messages in thread* Re: [PATCH 3/4] MdeModulePkg/AtaAtapiPassThru: Restart failed packets
2020-11-03 13:23 ` [PATCH 3/4] MdeModulePkg/AtaAtapiPassThru: Restart failed packets Albecki, Mateusz
@ 2020-11-04 5:33 ` Wu, Hao A
0 siblings, 0 replies; 11+ messages in thread
From: Wu, Hao A @ 2020-11-04 5:33 UTC (permalink / raw)
To: Albecki, Mateusz, devel@edk2.groups.io; +Cc: Ni, Ray
> -----Original Message-----
> From: Albecki, Mateusz <mateusz.albecki@intel.com>
> Sent: Tuesday, November 3, 2020 9:24 PM
> To: devel@edk2.groups.io
> Cc: Albecki, Mateusz <mateusz.albecki@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> Subject: [PATCH 3/4] MdeModulePkg/AtaAtapiPassThru: Restart failed
> packets
>
> From: Albecki <mateusz.albecki@intel.com>
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3026
>
> This commit adds code to restart the ATA packets that failed due to the CRC
> error or other link condition. For sync transfers the code will try to get the
> command working for up to 5 times and for async in accordance to the
> RetryTimes field in Task structure. For sync case the count of 5 retries has
For "... for async in accordance to the RetryTimes field in Task structure.",
how about reword it to "for async transfers, the command retry will happen until
the timeout value specified by the requester is reached"?
Judging from how the 'RetryTimes' field being assigned in AtaPassThruPassThru():
Task->RetryTimes = DivU64x32(Packet->Timeout, 1000) + 1;
I think it is relative to the timeout value for the PassThru packet.
Best Regards,
Hao Wu
> been chosen arbitrarily and if needed can be increased or decreased.
>
> Signed-off-by: Mateusz Albecki <mateusz.albecki@intel.com>
>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
>
> ---
> .../Bus/Ata/AtaAtapiPassThru/AhciMode.c | 305 +++++++++++-------
> .../Bus/Ata/AtaAtapiPassThru/AhciMode.h | 2 +
> 2 files changed, 182 insertions(+), 125 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> index cf735d5983..4fe7e4b1dc 100644
> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> @@ -897,6 +897,7 @@ AhciPioTransfer (
> EFI_AHCI_COMMAND_FIS CFis;
> EFI_AHCI_COMMAND_LIST CmdList;
> UINT32 PrdCount;
> + UINT32 Retry;
>
> if (Read) {
> Flag = EfiPciIoOperationBusMasterWrite; @@ -931,49 +932,56 @@
> AhciPioTransfer (
> CmdList.AhciCmdCfl = EFI_AHCI_FIS_REGISTER_H2D_LENGTH / 4;
> CmdList.AhciCmdW = Read ? 0 : 1;
>
> - AhciBuildCommand (
> - PciIo,
> - AhciRegisters,
> - Port,
> - PortMultiplier,
> - &CFis,
> - &CmdList,
> - AtapiCommand,
> - AtapiCommandLength,
> - 0,
> - (VOID *)(UINTN)PhyAddr,
> - DataCount
> - );
> + for (Retry = 0; Retry < AHCI_COMMAND_RETRIES; Retry++) {
> + AhciBuildCommand (
> + PciIo,
> + AhciRegisters,
> + Port,
> + PortMultiplier,
> + &CFis,
> + &CmdList,
> + AtapiCommand,
> + AtapiCommandLength,
> + 0,
> + (VOID *)(UINTN)PhyAddr,
> + DataCount
> + );
>
> - Status = AhciStartCommand (
> - PciIo,
> - Port,
> - 0,
> - Timeout
> - );
> - if (EFI_ERROR (Status)) {
> - goto Exit;
> - }
> + Status = AhciStartCommand (
> + PciIo,
> + Port,
> + 0,
> + Timeout
> + );
> + if (EFI_ERROR (Status)) {
> + break;
> + }
>
> - if (Read && (AtapiCommand == 0)) {
> - Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout, SataFisPioSetup);
> - if (Status == EFI_SUCCESS) {
> - PrdCount = *(volatile UINT32 *) (&(AhciRegisters-
> >AhciCmdList[0].AhciCmdPrdbc));
> - if (PrdCount == DataCount) {
> - Status = EFI_SUCCESS;
> - } else {
> - Status = EFI_DEVICE_ERROR;
> + if (Read && (AtapiCommand == 0)) {
> + Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout, SataFisPioSetup);
> + if (Status == EFI_SUCCESS) {
> + PrdCount = *(volatile UINT32 *) (&(AhciRegisters-
> >AhciCmdList[0].AhciCmdPrdbc));
> + if (PrdCount == DataCount) {
> + Status = EFI_SUCCESS;
> + } else {
> + Status = EFI_DEVICE_ERROR;
> + }
> }
> + } else {
> + Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout,
> + SataFisD2H);
> }
> - } else {
> - Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout, SataFisD2H);
> - }
>
> - if (Status == EFI_DEVICE_ERROR) {
> - AhciRecoverPortError (PciIo, Port);
> + if (Status == EFI_DEVICE_ERROR) {
> + DEBUG ((DEBUG_ERROR, "PIO command failed at retry %d\n", Retry));
> + Status = AhciRecoverPortError (PciIo, Port);
> + if (EFI_ERROR (Status)) {
> + break;
> + }
> + } else {
> + break;
> + }
> }
>
> -Exit:
> AhciStopCommand (
> PciIo,
> Port,
> @@ -992,7 +1000,6 @@ Exit:
> );
>
> AhciDumpPortStatus (PciIo, AhciRegisters, Port, AtaStatusBlock);
> -
> return Status;
> }
>
> @@ -1046,9 +1053,9 @@ AhciDmaTransfer (
> EFI_PCI_IO_PROTOCOL_OPERATION Flag;
> EFI_AHCI_COMMAND_FIS CFis;
> EFI_AHCI_COMMAND_LIST CmdList;
> -
> EFI_PCI_IO_PROTOCOL *PciIo;
> EFI_TPL OldTpl;
> + UINT32 Retry;
>
> Map = NULL;
> PciIo = Instance->PciIo;
> @@ -1058,36 +1065,16 @@ AhciDmaTransfer (
> }
>
> //
> - // Before starting the Blocking BlockIO operation, push to finish all non-
> blocking
> - // BlockIO tasks.
> - // Delay 100us to simulate the blocking time out checking.
> + // DMA buffer allocation. Needs to be done only once for both sync
> + and async // DMA transfers irrespective of number of retries.
> //
> - OldTpl = gBS->RaiseTPL (TPL_NOTIFY);
> - while ((Task == NULL) && (!IsListEmpty (&Instance->NonBlockingTaskList)))
> {
> - AsyncNonBlockingTransferRoutine (NULL, Instance);
> - //
> - // Stall for 100us.
> - //
> - MicroSecondDelay (100);
> - }
> - gBS->RestoreTPL (OldTpl);
> -
> - if ((Task == NULL) || ((Task != NULL) && (!Task->IsStart))) {
> - //
> - // Mark the Task to indicate that it has been started.
> - //
> - if (Task != NULL) {
> - Task->IsStart = TRUE;
> - }
> + if ((Task == NULL) || ((Task != NULL) && (Task->Map == NULL))) {
> if (Read) {
> Flag = EfiPciIoOperationBusMasterWrite;
> } else {
> Flag = EfiPciIoOperationBusMasterRead;
> }
>
> - //
> - // Construct command list and command table with pci bus address.
> - //
> MapLength = DataCount;
> Status = PciIo->Map (
> PciIo,
> @@ -1101,63 +1088,123 @@ AhciDmaTransfer (
> if (EFI_ERROR (Status) || (DataCount != MapLength)) {
> return EFI_BAD_BUFFER_SIZE;
> }
> -
> if (Task != NULL) {
> Task->Map = Map;
> }
> - //
> - // Package read needed
> - //
> + }
> +
> + if (Task == NULL || (Task != NULL && !Task->IsStart)) {
> AhciBuildCommandFis (&CFis, AtaCommandBlock);
>
> ZeroMem (&CmdList, sizeof (EFI_AHCI_COMMAND_LIST));
>
> CmdList.AhciCmdCfl = EFI_AHCI_FIS_REGISTER_H2D_LENGTH / 4;
> CmdList.AhciCmdW = Read ? 0 : 1;
> + }
>
> - AhciBuildCommand (
> - PciIo,
> - AhciRegisters,
> - Port,
> - PortMultiplier,
> - &CFis,
> - &CmdList,
> - AtapiCommand,
> - AtapiCommandLength,
> - 0,
> - (VOID *)(UINTN)PhyAddr,
> - DataCount
> - );
> -
> - Status = AhciStartCommand (
> - PciIo,
> - Port,
> - 0,
> - Timeout
> - );
> - if (EFI_ERROR (Status)) {
> - goto Exit;
> + if (Task == NULL) {
> + //
> + // Before starting the Blocking BlockIO operation, push to finish all non-
> blocking
> + // BlockIO tasks.
> + // Delay 100us to simulate the blocking time out checking.
> + //
> + OldTpl = gBS->RaiseTPL (TPL_NOTIFY);
> + while (!IsListEmpty (&Instance->NonBlockingTaskList)) {
> + AsyncNonBlockingTransferRoutine (NULL, Instance);
> + //
> + // Stall for 100us.
> + //
> + MicroSecondDelay (100);
> }
> - }
> + gBS->RestoreTPL (OldTpl);
> + for (Retry = 0; Retry < AHCI_COMMAND_RETRIES; Retry++) {
> + AhciBuildCommand (
> + PciIo,
> + AhciRegisters,
> + Port,
> + PortMultiplier,
> + &CFis,
> + &CmdList,
> + AtapiCommand,
> + AtapiCommandLength,
> + 0,
> + (VOID *)(UINTN)PhyAddr,
> + DataCount
> + );
>
> - if (Task != NULL) {
> - Status = AhciCheckFisReceived (PciIo, Port, SataFisD2H);
> - if (Status == EFI_NOT_READY) {
> - if (!Task->InfiniteWait && Task->RetryTimes == 0) {
> - Status = EFI_TIMEOUT;
> + Status = AhciStartCommand (
> + PciIo,
> + Port,
> + 0,
> + Timeout
> + );
> + if (EFI_ERROR (Status)) {
> + break;
> + }
> + 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)) {
> + break;
> + }
> } else {
> - Task->RetryTimes--;
> + break;
> }
> }
> } else {
> - Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout, SataFisD2H);
> - }
> + if (!Task->IsStart) {
> + AhciBuildCommand (
> + PciIo,
> + AhciRegisters,
> + Port,
> + PortMultiplier,
> + &CFis,
> + &CmdList,
> + AtapiCommand,
> + AtapiCommandLength,
> + 0,
> + (VOID *)(UINTN)PhyAddr,
> + DataCount
> + );
>
> - if (Status == EFI_DEVICE_ERROR) {
> - AhciRecoverPortError (PciIo, Port);
> + Status = AhciStartCommand (
> + PciIo,
> + Port,
> + 0,
> + Timeout
> + );
> + if (!EFI_ERROR (Status)) {
> + Task->IsStart = TRUE;
> + }
> + }
> + if (Task->IsStart) {
> + 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);
> + //
> + // 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) {
> + Task->IsStart = FALSE;
> + Status = EFI_NOT_READY;
> + }
> + }
> +
> + if (Status == EFI_NOT_READY) {
> + if (!Task->InfiniteWait && Task->RetryTimes == 0) {
> + Status = EFI_TIMEOUT;
> + } else {
> + Task->RetryTimes--;
> + }
> + }
> + }
> }
>
> -Exit:
> //
> // For Blocking mode, the command should be stopped, the Fis should be
> disabled
> // and the PciIo should be unmapped.
> @@ -1234,6 +1281,7 @@ AhciNonDataTransfer (
> EFI_STATUS Status;
> EFI_AHCI_COMMAND_FIS CFis;
> EFI_AHCI_COMMAND_LIST CmdList;
> + UINT32 Retry;
>
> //
> // Package read needed
> @@ -1244,36 +1292,43 @@ AhciNonDataTransfer (
>
> CmdList.AhciCmdCfl = EFI_AHCI_FIS_REGISTER_H2D_LENGTH / 4;
>
> - AhciBuildCommand (
> - PciIo,
> - AhciRegisters,
> - Port,
> - PortMultiplier,
> - &CFis,
> - &CmdList,
> - AtapiCommand,
> - AtapiCommandLength,
> - 0,
> - NULL,
> - 0
> - );
> + for (Retry = 0; Retry < AHCI_COMMAND_RETRIES; Retry++) {
> + AhciBuildCommand (
> + PciIo,
> + AhciRegisters,
> + Port,
> + PortMultiplier,
> + &CFis,
> + &CmdList,
> + AtapiCommand,
> + AtapiCommandLength,
> + 0,
> + NULL,
> + 0
> + );
>
> - Status = AhciStartCommand (
> - PciIo,
> - Port,
> - 0,
> - Timeout
> - );
> - if (EFI_ERROR (Status)) {
> - goto Exit;
> - }
> + Status = AhciStartCommand (
> + PciIo,
> + Port,
> + 0,
> + Timeout
> + );
> + if (EFI_ERROR (Status)) {
> + break;
> + }
>
> - Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout, SataFisD2H);
> - if (Status == EFI_DEVICE_ERROR) {
> - AhciRecoverPortError (PciIo, Port);
> + 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)) {
> + break;
> + }
> + } else {
> + break;
> + }
> }
>
> -Exit:
> AhciStopCommand (
> PciIo,
> Port,
> diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
> index 4d2aafe483..0540b0f4fa 100644
> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
> @@ -192,6 +192,8 @@ typedef union {
> #define AHCI_PORT_DEVSLP_DITO_MASK 0x01FF8000
> #define AHCI_PORT_DEVSLP_DM_MASK 0x1E000000
>
> +#define AHCI_COMMAND_RETRIES 5
> +
> #pragma pack(1)
> //
> // Command List structure includes total 32 entries.
> --
> 2.28.0.windows.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 4/4] MdeModulePkg/AtaAtapiPassThru: Trace ATA packets
2020-11-03 13:23 [PATCH 0/4] MdeModulePkg/AtaAtapiPassThru: Add SATA error recovery Albecki, Mateusz
` (2 preceding siblings ...)
2020-11-03 13:23 ` [PATCH 3/4] MdeModulePkg/AtaAtapiPassThru: Restart failed packets Albecki, Mateusz
@ 2020-11-03 13:23 ` Albecki, Mateusz
2020-11-04 5:33 ` Wu, Hao A
2020-11-04 1:57 ` [PATCH 0/4] MdeModulePkg/AtaAtapiPassThru: Add SATA error recovery Wu, Hao A
4 siblings, 1 reply; 11+ messages in thread
From: Albecki, Mateusz @ 2020-11-03 13:23 UTC (permalink / raw)
To: devel; +Cc: Albecki, Ray Ni, Hao A Wu
From: Albecki <mateusz.albecki@intel.com>
This simplify ATA driver debugging all ATA packets will be printed to
debug port on DEBUG_VERBOSE level along with the packet execution
status. Additionally failed packets and the failed packet execution
status will be printed on DEBUG_ERROR level.
Signed-off-by: Mateusz Albecki <mateusz.albecki@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
---
.../Bus/Ata/AtaAtapiPassThru/AhciMode.c | 94 +++++++++++++++++++
1 file changed, 94 insertions(+)
diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
index 4fe7e4b1dc..3a7a6eb018 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
@@ -846,6 +846,54 @@ AhciWaitUntilFisReceived (
return EFI_TIMEOUT;
}
+/**
+ Prints contents of the ATA command block into the debug port.
+
+ @param[in] AtaCommandBlock AtaCommandBlock to print.
+ @param[in] DebugLevel Debug level on which to print.
+**/
+VOID
+AhciPrintCommandBlock (
+ IN EFI_ATA_COMMAND_BLOCK *AtaCommandBlock,
+ IN UINT32 DebugLevel
+ )
+{
+ DEBUG ((DebugLevel, "ATA COMMAND BLOCK:\n"));
+ DEBUG ((DebugLevel, "AtaCommand: %d\n", AtaCommandBlock->AtaCommand));
+ DEBUG ((DebugLevel, "AtaFeatures: %X\n", AtaCommandBlock->AtaFeatures));
+ DEBUG ((DebugLevel, "AtaSectorNumber: %d\n", AtaCommandBlock->AtaSectorNumber));
+ DEBUG ((DebugLevel, "AtaCylinderLow: %X\n", AtaCommandBlock->AtaCylinderHigh));
+ DEBUG ((DebugLevel, "AtaCylinderHigh: %X\n", AtaCommandBlock->AtaCylinderHigh));
+ DEBUG ((DebugLevel, "AtaDeviceHead: %d\n", AtaCommandBlock->AtaDeviceHead));
+ DEBUG ((DebugLevel, "AtaSectorNumberExp: %d\n", AtaCommandBlock->AtaSectorNumberExp));
+ DEBUG ((DebugLevel, "AtaCylinderLowExp: %X\n", AtaCommandBlock->AtaCylinderLowExp));
+ DEBUG ((DebugLevel, "AtaCylinderHighExp: %X\n", AtaCommandBlock->AtaCylinderHighExp));
+ DEBUG ((DebugLevel, "AtaFeaturesExp: %X\n", AtaCommandBlock->AtaFeaturesExp));
+ DEBUG ((DebugLevel, "AtaSectorCount: %d\n", AtaCommandBlock->AtaSectorCount));
+ DEBUG ((DebugLevel, "AtaSectorCountExp: %d\n", AtaCommandBlock->AtaSectorCountExp));
+}
+
+/**
+ Prints contents of the ATA status block into the debug port.
+
+ @param[in] AtaStatusBlock AtaStatusBlock to print.
+ @param[in] DebugLevel Debug level on which to print.
+**/
+VOID
+AhciPrintStatusBlock (
+ IN EFI_ATA_STATUS_BLOCK *AtaStatusBlock,
+ IN UINT32 DebugLevel
+ )
+{
+ //
+ // Only print status and error since we have all of the rest printed as
+ // a part of command block print.
+ //
+ DEBUG ((DebugLevel, "ATA STATUS BLOCK:\n"));
+ DEBUG ((DebugLevel, "AtaStatus: %d\n", AtaStatusBlock->AtaStatus));
+ DEBUG ((DebugLevel, "AtaError: %d\n", AtaStatusBlock->AtaError));
+}
+
/**
Start a PIO data transfer on specific port.
@@ -947,6 +995,8 @@ AhciPioTransfer (
DataCount
);
+ DEBUG ((DEBUG_VERBOSE, "Starting commmand: \n"));
+ AhciPrintCommandBlock (AtaCommandBlock, DEBUG_VERBOSE);
Status = AhciStartCommand (
PciIo,
Port,
@@ -1000,6 +1050,19 @@ AhciPioTransfer (
);
AhciDumpPortStatus (PciIo, AhciRegisters, Port, AtaStatusBlock);
+
+ if (Status == EFI_DEVICE_ERROR) {
+ DEBUG ((DEBUG_ERROR, "Failed to execute command:\n"));
+ //
+ // Repeat command block here to make sure it is printed on
+ // device error debug level.
+ //
+ AhciPrintCommandBlock (AtaCommandBlock, DEBUG_ERROR);
+ AhciPrintStatusBlock (AtaStatusBlock, DEBUG_ERROR);
+ } else {
+ AhciPrintStatusBlock (AtaStatusBlock, DEBUG_VERBOSE);
+ }
+
return Status;
}
@@ -1132,6 +1195,8 @@ AhciDmaTransfer (
DataCount
);
+ DEBUG ((DEBUG_VERBOSE, "Starting commmand: \n"));
+ AhciPrintCommandBlock (AtaCommandBlock, DEBUG_VERBOSE);
Status = AhciStartCommand (
PciIo,
Port,
@@ -1168,6 +1233,8 @@ AhciDmaTransfer (
DataCount
);
+ DEBUG ((DEBUG_VERBOSE, "Starting commmand: \n"));
+ AhciPrintCommandBlock (AtaCommandBlock, DEBUG_VERBOSE);
Status = AhciStartCommand (
PciIo,
Port,
@@ -1238,6 +1305,19 @@ AhciDmaTransfer (
}
AhciDumpPortStatus (PciIo, AhciRegisters, Port, AtaStatusBlock);
+
+ if (Status == EFI_DEVICE_ERROR) {
+ DEBUG ((DEBUG_ERROR, "Failed to execute command:\n"));
+ //
+ // Repeat command block here to make sure it is printed on
+ // device error debug level.
+ //
+ AhciPrintCommandBlock (AtaCommandBlock, DEBUG_ERROR);
+ AhciPrintStatusBlock (AtaStatusBlock, DEBUG_ERROR);
+ } else {
+ AhciPrintStatusBlock (AtaStatusBlock, DEBUG_VERBOSE);
+ }
+
return Status;
}
@@ -1307,6 +1387,8 @@ AhciNonDataTransfer (
0
);
+ DEBUG ((DEBUG_VERBOSE, "Starting commmand: \n"));
+ AhciPrintCommandBlock (AtaCommandBlock, DEBUG_VERBOSE);
Status = AhciStartCommand (
PciIo,
Port,
@@ -1343,6 +1425,18 @@ AhciNonDataTransfer (
AhciDumpPortStatus (PciIo, AhciRegisters, Port, AtaStatusBlock);
+ if (Status == EFI_DEVICE_ERROR) {
+ DEBUG ((DEBUG_ERROR, "Failed to execute command:\n"));
+ //
+ // Repeat command block here to make sure it is printed on
+ // device error debug level.
+ //
+ AhciPrintCommandBlock (AtaCommandBlock, DEBUG_ERROR);
+ AhciPrintStatusBlock (AtaStatusBlock, DEBUG_ERROR);
+ } else {
+ AhciPrintStatusBlock (AtaStatusBlock, DEBUG_VERBOSE);
+ }
+
return Status;
}
--
2.28.0.windows.1
---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Sowackiego 173 | 80-298 Gdask | Sd Rejonowy Gdask Pnoc | VII Wydzia Gospodarczy Krajowego Rejestru Sdowego - KRS 101882 | NIP 957-07-52-316 | Kapita zakadowy 200.000 PLN.
Ta wiadomo wraz z zacznikami jest przeznaczona dla okrelonego adresata i moe zawiera informacje poufne. W razie przypadkowego otrzymania tej wiadomoci, prosimy o powiadomienie nadawcy oraz trwae jej usunicie; jakiekolwiek przegldanie 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] 11+ messages in thread* Re: [PATCH 4/4] MdeModulePkg/AtaAtapiPassThru: Trace ATA packets
2020-11-03 13:23 ` [PATCH 4/4] MdeModulePkg/AtaAtapiPassThru: Trace ATA packets Albecki, Mateusz
@ 2020-11-04 5:33 ` Wu, Hao A
0 siblings, 0 replies; 11+ messages in thread
From: Wu, Hao A @ 2020-11-04 5:33 UTC (permalink / raw)
To: Albecki, Mateusz, devel@edk2.groups.io; +Cc: Ni, Ray
> -----Original Message-----
> From: Albecki, Mateusz <mateusz.albecki@intel.com>
> Sent: Tuesday, November 3, 2020 9:24 PM
> To: devel@edk2.groups.io
> Cc: Albecki, Mateusz <mateusz.albecki@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> Subject: [PATCH 4/4] MdeModulePkg/AtaAtapiPassThru: Trace ATA packets
>
> From: Albecki <mateusz.albecki@intel.com>
>
> This simplify ATA driver debugging all ATA packets will be printed to debug
> port on DEBUG_VERBOSE level along with the packet execution status.
> Additionally failed packets and the failed packet execution status will be
> printed on DEBUG_ERROR level.
A couple of minor comments:
1. Could you help to correct the typos for "commmand" to "command"?
2. For the below debug messages added in AhciPioTransfer(), AhciDmaTransfer() and AhciNonDataTransfer():
DEBUG ((DEBUG_VERBOSE, "Starting commmand: \n"));
DEBUG ((DEBUG_ERROR, "Failed to execute command:\n"));
Could you help to add the transfer type information. Like for non-blocking DMA transfer:
DEBUG ((DEBUG_VERBOSE, "Starting command for non-blocking DMA transfer: \n"));
DEBUG ((DEBUG_ERROR, "Failed to execute command for non-blocking DMA transfer:\n"));
This might be helpful to identify these messages with their corresponding transfer request.
Best Regards,
Hao Wu
>
> Signed-off-by: Mateusz Albecki <mateusz.albecki@intel.com>
>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
>
> ---
> .../Bus/Ata/AtaAtapiPassThru/AhciMode.c | 94 +++++++++++++++++++
> 1 file changed, 94 insertions(+)
>
> diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> index 4fe7e4b1dc..3a7a6eb018 100644
> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> @@ -846,6 +846,54 @@ AhciWaitUntilFisReceived (
> return EFI_TIMEOUT;
> }
>
> +/**
> + Prints contents of the ATA command block into the debug port.
> +
> + @param[in] AtaCommandBlock AtaCommandBlock to print.
> + @param[in] DebugLevel Debug level on which to print.
> +**/
> +VOID
> +AhciPrintCommandBlock (
> + IN EFI_ATA_COMMAND_BLOCK *AtaCommandBlock,
> + IN UINT32 DebugLevel
> + )
> +{
> + DEBUG ((DebugLevel, "ATA COMMAND BLOCK:\n"));
> + DEBUG ((DebugLevel, "AtaCommand: %d\n",
> +AtaCommandBlock->AtaCommand));
> + DEBUG ((DebugLevel, "AtaFeatures: %X\n",
> +AtaCommandBlock->AtaFeatures));
> + DEBUG ((DebugLevel, "AtaSectorNumber: %d\n",
> +AtaCommandBlock->AtaSectorNumber));
> + DEBUG ((DebugLevel, "AtaCylinderLow: %X\n",
> +AtaCommandBlock->AtaCylinderHigh));
> + DEBUG ((DebugLevel, "AtaCylinderHigh: %X\n",
> +AtaCommandBlock->AtaCylinderHigh));
> + DEBUG ((DebugLevel, "AtaDeviceHead: %d\n",
> +AtaCommandBlock->AtaDeviceHead));
> + DEBUG ((DebugLevel, "AtaSectorNumberExp: %d\n",
> +AtaCommandBlock->AtaSectorNumberExp));
> + DEBUG ((DebugLevel, "AtaCylinderLowExp: %X\n",
> +AtaCommandBlock->AtaCylinderLowExp));
> + DEBUG ((DebugLevel, "AtaCylinderHighExp: %X\n",
> +AtaCommandBlock->AtaCylinderHighExp));
> + DEBUG ((DebugLevel, "AtaFeaturesExp: %X\n",
> +AtaCommandBlock->AtaFeaturesExp));
> + DEBUG ((DebugLevel, "AtaSectorCount: %d\n",
> +AtaCommandBlock->AtaSectorCount));
> + DEBUG ((DebugLevel, "AtaSectorCountExp: %d\n",
> +AtaCommandBlock->AtaSectorCountExp));
> +}
> +
> +/**
> + Prints contents of the ATA status block into the debug port.
> +
> + @param[in] AtaStatusBlock AtaStatusBlock to print.
> + @param[in] DebugLevel Debug level on which to print.
> +**/
> +VOID
> +AhciPrintStatusBlock (
> + IN EFI_ATA_STATUS_BLOCK *AtaStatusBlock,
> + IN UINT32 DebugLevel
> + )
> +{
> + //
> + // Only print status and error since we have all of the rest printed
> +as
> + // a part of command block print.
> + //
> + DEBUG ((DebugLevel, "ATA STATUS BLOCK:\n"));
> + DEBUG ((DebugLevel, "AtaStatus: %d\n", AtaStatusBlock->AtaStatus));
> + DEBUG ((DebugLevel, "AtaError: %d\n", AtaStatusBlock->AtaError)); }
> +
> /**
> Start a PIO data transfer on specific port.
>
> @@ -947,6 +995,8 @@ AhciPioTransfer (
> DataCount
> );
>
> + DEBUG ((DEBUG_VERBOSE, "Starting commmand: \n"));
> + AhciPrintCommandBlock (AtaCommandBlock, DEBUG_VERBOSE);
> Status = AhciStartCommand (
> PciIo,
> Port,
> @@ -1000,6 +1050,19 @@ AhciPioTransfer (
> );
>
> AhciDumpPortStatus (PciIo, AhciRegisters, Port, AtaStatusBlock);
> +
> + if (Status == EFI_DEVICE_ERROR) {
> + DEBUG ((DEBUG_ERROR, "Failed to execute command:\n"));
> + //
> + // Repeat command block here to make sure it is printed on
> + // device error debug level.
> + //
> + AhciPrintCommandBlock (AtaCommandBlock, DEBUG_ERROR);
> + AhciPrintStatusBlock (AtaStatusBlock, DEBUG_ERROR); } else {
> + AhciPrintStatusBlock (AtaStatusBlock, DEBUG_VERBOSE); }
> +
> return Status;
> }
>
> @@ -1132,6 +1195,8 @@ AhciDmaTransfer (
> DataCount
> );
>
> + DEBUG ((DEBUG_VERBOSE, "Starting commmand: \n"));
> + AhciPrintCommandBlock (AtaCommandBlock, DEBUG_VERBOSE);
> Status = AhciStartCommand (
> PciIo,
> Port,
> @@ -1168,6 +1233,8 @@ AhciDmaTransfer (
> DataCount
> );
>
> + DEBUG ((DEBUG_VERBOSE, "Starting commmand: \n"));
> + AhciPrintCommandBlock (AtaCommandBlock, DEBUG_VERBOSE);
> Status = AhciStartCommand (
> PciIo,
> Port,
> @@ -1238,6 +1305,19 @@ AhciDmaTransfer (
> }
>
> AhciDumpPortStatus (PciIo, AhciRegisters, Port, AtaStatusBlock);
> +
> + if (Status == EFI_DEVICE_ERROR) {
> + DEBUG ((DEBUG_ERROR, "Failed to execute command:\n"));
> + //
> + // Repeat command block here to make sure it is printed on
> + // device error debug level.
> + //
> + AhciPrintCommandBlock (AtaCommandBlock, DEBUG_ERROR);
> + AhciPrintStatusBlock (AtaStatusBlock, DEBUG_ERROR); } else {
> + AhciPrintStatusBlock (AtaStatusBlock, DEBUG_VERBOSE); }
> +
> return Status;
> }
>
> @@ -1307,6 +1387,8 @@ AhciNonDataTransfer (
> 0
> );
>
> + DEBUG ((DEBUG_VERBOSE, "Starting commmand: \n"));
> + AhciPrintCommandBlock (AtaCommandBlock, DEBUG_VERBOSE);
> Status = AhciStartCommand (
> PciIo,
> Port,
> @@ -1343,6 +1425,18 @@ AhciNonDataTransfer (
>
> AhciDumpPortStatus (PciIo, AhciRegisters, Port, AtaStatusBlock);
>
> + if (Status == EFI_DEVICE_ERROR) {
> + DEBUG ((DEBUG_ERROR, "Failed to execute command:\n"));
> + //
> + // Repeat command block here to make sure it is printed on
> + // device error debug level.
> + //
> + AhciPrintCommandBlock (AtaCommandBlock, DEBUG_ERROR);
> + AhciPrintStatusBlock (AtaStatusBlock, DEBUG_ERROR); } else {
> + AhciPrintStatusBlock (AtaStatusBlock, DEBUG_VERBOSE); }
> +
> return Status;
> }
>
> --
> 2.28.0.windows.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/4] MdeModulePkg/AtaAtapiPassThru: Add SATA error recovery
2020-11-03 13:23 [PATCH 0/4] MdeModulePkg/AtaAtapiPassThru: Add SATA error recovery Albecki, Mateusz
` (3 preceding siblings ...)
2020-11-03 13:23 ` [PATCH 4/4] MdeModulePkg/AtaAtapiPassThru: Trace ATA packets Albecki, Mateusz
@ 2020-11-04 1:57 ` Wu, Hao A
4 siblings, 0 replies; 11+ messages in thread
From: Wu, Hao A @ 2020-11-04 1:57 UTC (permalink / raw)
To: Albecki, Mateusz, devel@edk2.groups.io; +Cc: Ni, Ray
> -----Original Message-----
> From: Albecki, Mateusz <mateusz.albecki@intel.com>
> Sent: Tuesday, November 3, 2020 9:24 PM
> To: devel@edk2.groups.io
> Cc: Albecki, Mateusz <mateusz.albecki@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> Subject: [PATCH 0/4] MdeModulePkg/AtaAtapiPassThru: Add SATA error
> recovery
>
> To increase boot stability when booting from SATA drives SATA driver should
> implement the AHCI spec defined port error recovery. This will allow the
> driver to handle random fails on SATA link.
>
> Performed tests on 2 setups. One with AHCI controller booting OS
> successfully without error recovery(control setup) and other which fails 1 in 5
> times(fail setup).
>
> Tests performed:
> 1. Booted control setup to OS successfully.
> 2. Checked if during normal boot none of the packets is repeated(this came
> up after previous code version had a bug which repeated each DMA packet 5
> times).
Some more information:
For the bug mentioned above in point 2, the discussion can be referred here:
https://github.com/malbecki/edk2/commit/9ea81cadf38725e194ec01e0b0c556fd133f3ced#r43226067
For the other discussion we did before this patch, please refer to:
https://github.com/malbecki/edk2/commits/sata_recovery
Best Regards,
Hao Wu
> 3. Booted control setup to OS with simulated errors appearing on first packet
> of every DMA transaction.
> 4. Performed extensive tests on fail setup. Fail rate decreased from 20%
> failure to ~1% failure. 1% failure is observed during OS execution, not BIOS so
> in a way boot is 100% stable).
>
> Change pushed to github:
> This series:
> https://github.com/malbecki/edk2/commits/sata_recovery2
> Simulated errors:
> https://github.com/malbecki/edk2/commits/sata_recovery_simulated_erro
> r
>
> Signed-off-by: Mateusz Albecki <mateusz.albecki@intel.com>
>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
>
> Albecki (4):
> MdeModulePkg/AtaAtapiPassThru: Check IS to check for command
> completion
> MdeModulePkg/AtaAtapiPassThru: Add SATA error recovery flow
> MdeModulePkg/AtaAtapiPassThru: Restart failed packets
> MdeModulePkg/AtaAtapiPassThru: Trace ATA packets
>
> .../Bus/Ata/AtaAtapiPassThru/AhciMode.c | 801 +++++++++++-------
> .../Bus/Ata/AtaAtapiPassThru/AhciMode.h | 16 +-
> 2 files changed, 532 insertions(+), 285 deletions(-)
>
> --
> 2.28.0.windows.1
^ permalink raw reply [flat|nested] 11+ messages in thread