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

["Anbazhagan, Baraneedharan" <anbazhagan@hp.com>]

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

Reviewed-by: Baraneedharan Anbazhagan <anbazhagan@hp.com<mailto:anbazhagan@hp.com>>

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

Reviewed-by: Hao A Wu <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>
Will wait a couple of days before merging to see if comments from other reviewers.

Best Regards,
Hao Wu

> -----Original Message-----
> From: Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>
> Sent: Tuesday, March 28, 2023 5:38 AM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> Cc: Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Wu, Hao A
> <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Chang, Hunter
> <hunter.chang@intel.com<mailto:hunter.chang@intel.com>>; Anbazhagan, Baraneedharan
> <anbazhagan@hp.com<mailto:anbazhagan@hp.com>>
> Subject: [PATCHv2 1/1] MdeModulePkg/Ahci: Skip retry for non-transient errors
>
> Currently AHCI driver will try to retry all failed packets
>
> regardless of the failure cause. This is a problem in password
>
> unlock flow where number of password retries is tracked by the
>
> device. If user passes a wrong password Ahci driver will try
>
> to send the wrong password multiple times which will exhaust
>
> number of password retries and force the user to restart the
>
> machine. This commit introduces a logic to check for the cause
>
> of packet failure and only retry packets which failed due to
>
> transient conditions on the link. With this patch only packets for
>
> which CRC error is flagged are retried.
>
>
>
> Cc: Hao A Wu <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>
>
> Cc: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com>>
>
> Cc: Hunter Chang <hunter.chang@intel.com<mailto:hunter.chang@intel.com>>
>
> Cc: Baraneedharan Anbazhagan <anbazhagan@hp.com<mailto:anbazhagan@hp.com>>
>
>
>
> Signed-off-by: Mateusz Albecki <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>
>
> ---
>
> .../Bus/Ata/AtaAtapiPassThru/AhciMode.c | 71 +++++++++++++++++--
>
> .../Bus/Ata/AtaAtapiPassThru/AhciMode.h | 3 +-
>
> 2 files changed, 69 insertions(+), 5 deletions(-)
>
>
>
> diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
>
> index 06c4a3e052..c0c8ffbd9e 100644
>
> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
>
> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
>
> @@ -737,12 +737,68 @@ AhciRecoverPortError (
>
> Status = AhciResetPort (PciIo, Port);
>
> if (EFI_ERROR (Status)) {
>
> DEBUG ((DEBUG_ERROR, "Failed to reset the port %d\n", Port));
>
> + return EFI_DEVICE_ERROR;
>
> }
>
> }
>
>
>
> return EFI_SUCCESS;
>
> }
>
>
>
> +/**
>
> + This function will check if the failed command should be retired. Only error
>
> + conditions which are a result of transient conditions on a link(either to
> system or to device).
>
> +
>
> + @param[in] PciIo Pointer to AHCI controller PciIo.
>
> + @param[in] Port SATA port index on which to check.
>
> +
>
> + @retval TRUE Command failure was caused by transient condition and
> should be retried
>
> + @retval FALSE Command should not be retried
>
> +**/
>
> +BOOLEAN
>
> +AhciShouldCmdBeRetried (
>
> + IN EFI_PCI_IO_PROTOCOL *PciIo,
>
> + IN UINT8 Port
>
> + )
>
> +{
>
> + UINT32 Offset;
>
> + UINT32 PortInterrupt;
>
> + UINT32 Serr;
>
> + UINT32 Tfd;
>
> +
>
> + Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH +
> EFI_AHCI_PORT_IS;
>
> + PortInterrupt = AhciReadReg (PciIo, Offset);
>
> + Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH +
> EFI_AHCI_PORT_SERR;
>
> + Serr = AhciReadReg (PciIo, Offset);
>
> + Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH +
> EFI_AHCI_PORT_TFD;
>
> + Tfd = AhciReadReg (PciIo, Offset);
>
> +
>
> + //
>
> + // This can occur if there was a CRC error on a path from system memory to
>
> + // host controller.
>
> + //
>
> + if (PortInterrupt & EFI_AHCI_PORT_IS_HBDS) {
>
> + return TRUE;
>
> + //
>
> + // This can occur if there was a CRC error detected by host during
> communication
>
> + // with the device
>
> + //
>
> + } else if ((PortInterrupt & (EFI_AHCI_PORT_IS_IFS | EFI_AHCI_PORT_IS_INFS))
> &&
>
> + (Serr & EFI_AHCI_PORT_SERR_CRCE))
>
> + {
>
> + return TRUE;
>
> + //
>
> + // This can occur if there was a CRC error detected by device during
> communication
>
> + // with the host. Device returns error status to host with D2H FIS.
>
> + //
>
> + } else if ((PortInterrupt & EFI_AHCI_PORT_IS_TFES) &&
>
> + (Tfd & EFI_AHCI_PORT_TFD_ERR_INT_CRC))
>
> + {
>
> + return TRUE;
>
> + }
>
> +
>
> + return FALSE;
>
> +}
>
> +
>
> /**
>
> Checks if specified FIS has been received.
>
>
>
> @@ -950,6 +1006,7 @@ AhciPioTransfer (
>
> UINT32 PrdCount;
>
> UINT32 Retry;
>
> EFI_STATUS RecoveryStatus;
>
> + BOOLEAN DoRetry;
>
>
>
> if (Read) {
>
> Flag = EfiPciIoOperationBusMasterWrite;
>
> @@ -1027,8 +1084,9 @@ AhciPioTransfer (
>
>
>
> if (Status == EFI_DEVICE_ERROR) {
>
> DEBUG ((DEBUG_ERROR, "PIO command failed at retry %d\n", Retry));
>
> + DoRetry = AhciShouldCmdBeRetried (PciIo, Port); // needs to be called
> before error recovery
>
> RecoveryStatus = AhciRecoverPortError (PciIo, Port);
>
> - if (EFI_ERROR (RecoveryStatus)) {
>
> + if (!DoRetry || EFI_ERROR (RecoveryStatus)) {
>
> break;
>
> }
>
> } else {
>
> @@ -1124,6 +1182,7 @@ AhciDmaTransfer (
>
> EFI_TPL OldTpl;
>
> UINT32 Retry;
>
> EFI_STATUS RecoveryStatus;
>
> + BOOLEAN DoRetry;
>
>
>
> Map = NULL;
>
> PciIo = Instance->PciIo;
>
> @@ -1222,8 +1281,9 @@ AhciDmaTransfer (
>
> Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout, SataFisD2H);
>
> if (Status == EFI_DEVICE_ERROR) {
>
> DEBUG ((DEBUG_ERROR, "DMA command failed at retry: %d\n", Retry));
>
> + DoRetry = AhciShouldCmdBeRetried (PciIo, Port); // needs to be
> called before error recovery
>
> RecoveryStatus = AhciRecoverPortError (PciIo, Port);
>
> - if (EFI_ERROR (RecoveryStatus)) {
>
> + if (!DoRetry || EFI_ERROR (RecoveryStatus)) {
>
> break;
>
> }
>
> } else {
>
> @@ -1263,6 +1323,7 @@ AhciDmaTransfer (
>
> Status = AhciCheckFisReceived (PciIo, Port, SataFisD2H);
>
> if (Status == EFI_DEVICE_ERROR) {
>
> DEBUG ((DEBUG_ERROR, "DMA command failed at retry: %d\n", Task-
> >RetryTimes));
>
> + DoRetry = AhciShouldCmdBeRetried (PciIo, Port); // call this before
> error recovery
>
> RecoveryStatus = AhciRecoverPortError (PciIo, Port);
>
> //
>
> // If recovery passed mark the Task as not started and change the status
>
> @@ -1270,7 +1331,7 @@ AhciDmaTransfer (
>
> // and on next call the command will be re-issued due to IsStart being
> FALSE.
>
> // This also makes the next condition decrement the RetryTimes.
>
> //
>
> - if (RecoveryStatus == EFI_SUCCESS) {
>
> + if (DoRetry && (RecoveryStatus == EFI_SUCCESS)) {
>
> Task->IsStart = FALSE;
>
> Status = EFI_NOT_READY;
>
> }
>
> @@ -1378,6 +1439,7 @@ AhciNonDataTransfer (
>
> EFI_AHCI_COMMAND_LIST CmdList;
>
> UINT32 Retry;
>
> EFI_STATUS RecoveryStatus;
>
> + BOOLEAN DoRetry;
>
>
>
> //
>
> // Package read needed
>
> @@ -1418,8 +1480,9 @@ AhciNonDataTransfer (
>
> Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout, SataFisD2H);
>
> if (Status == EFI_DEVICE_ERROR) {
>
> DEBUG ((DEBUG_ERROR, "Non data transfer failed at retry %d\n", Retry));
>
> + DoRetry = AhciShouldCmdBeRetried (PciIo, Port); // call this before
> error recovery
>
> RecoveryStatus = AhciRecoverPortError (PciIo, Port);
>
> - if (EFI_ERROR (RecoveryStatus)) {
>
> + if (!DoRetry || EFI_ERROR (RecoveryStatus)) {
>
> break;
>
> }
>
> } else {
>
> diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
>
> index d7434b408c..5bb31057ec 100644
>
> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
>
> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
>
> @@ -146,7 +146,8 @@ typedef union {
>
> #define EFI_AHCI_PORT_TFD_BSY BIT7
>
> #define EFI_AHCI_PORT_TFD_DRQ BIT3
>
> #define EFI_AHCI_PORT_TFD_ERR BIT0
>
> -#define EFI_AHCI_PORT_TFD_ERR_MASK 0x00FF00
>
> +#define EFI_AHCI_PORT_TFD_ERR_MASK 0x00FF00 // ERROR field is
> specified by ATA/ATAPI Command Set specification
>
> +#define EFI_AHCI_PORT_TFD_ERR_INT_CRC BIT15
>
> #define EFI_AHCI_PORT_SIG 0x0024
>
> #define EFI_AHCI_PORT_SSTS 0x0028
>
> #define EFI_AHCI_PORT_SSTS_DET_MASK 0x000F
>
> --
>
> 2.39.1.windows.1
>
>





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