From: "Wu, Hao A" <hao.a.wu@intel.com>
To: "Albecki, Mateusz" <mateusz.albecki@intel.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: "Ni, Ray" <ray.ni@intel.com>
Subject: Re: [PATCH 3/4] MdeModulePkg/AtaAtapiPassThru: Restart failed packets
Date: Wed, 4 Nov 2020 05:33:34 +0000 [thread overview]
Message-ID: <BN8PR11MB3666BD28BB40FFD52C8422E8CAEF0@BN8PR11MB3666.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20201103132348.2916-4-mateusz.albecki@intel.com>
> -----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
next prev parent reply other threads:[~2020-11-04 5:33 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
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-04 5:33 ` Wu, Hao A
2020-11-04 11:54 ` Albecki, Mateusz
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
2020-11-03 13:23 ` [PATCH 3/4] MdeModulePkg/AtaAtapiPassThru: Restart failed packets Albecki, Mateusz
2020-11-04 5:33 ` Wu, Hao A [this message]
2020-11-03 13:23 ` [PATCH 4/4] MdeModulePkg/AtaAtapiPassThru: Trace ATA packets 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=BN8PR11MB3666BD28BB40FFD52C8422E8CAEF0@BN8PR11MB3666.namprd11.prod.outlook.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox