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: [PATCHv3 3/4] MdeModulePkg/AtaAtapiPassThru: Restart failed packets
Date: Fri, 6 Nov 2020 03:14:23 +0000 [thread overview]
Message-ID: <BN8PR11MB3666177E851C619B8FAB68EACAED0@BN8PR11MB3666.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20201105124847.3136-4-mateusz.albecki@intel.com>
> -----Original Message-----
> From: Albecki, Mateusz <mateusz.albecki@intel.com>
> Sent: Thursday, November 5, 2020 8:49 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: [PATCHv3 3/4] MdeModulePkg/AtaAtapiPassThru: Restart failed
> packets
>
> 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. For async traansfers, the command wil
> be retried until the timeout value timeout specified by the requester is
Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
Best Regards,
Hao Wu
> reached. 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 0b7141c4f1..47275a851a 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
> + );
> +
> + 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_DEVICE_ERROR) {
> - AhciRecoverPortError (PciIo, Port);
> + 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 338447a55f..ced2648970 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-06 3:14 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-05 12:48 [PATCHv3 1/4] MdeModulePkg/AtaAtapiPassThru: Check IS to check for command completion Albecki, Mateusz
2020-11-05 12:48 ` [PATCHv3 2/4] MdeModulePkg/AtaAtapiPassThru: Add SATA error recovery flow Albecki, Mateusz
2020-11-06 3:12 ` Wu, Hao A
2020-11-05 12:48 ` [PATCHv3 3/4] MdeModulePkg/AtaAtapiPassThru: Restart failed packets Albecki, Mateusz
2020-11-06 3:14 ` Wu, Hao A [this message]
2020-11-05 12:48 ` [PATCHv3 4/4] MdeModulePkg/AtaAtapiPassThru: Trace ATA packets Albecki, Mateusz
2020-11-06 3:19 ` Wu, Hao A
2020-11-06 3:12 ` [PATCHv3 1/4] MdeModulePkg/AtaAtapiPassThru: Check IS to check for command completion 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=BN8PR11MB3666177E851C619B8FAB68EACAED0@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