Re: [PATCH 4/4] MdeModulePkg/AtaAtapiPassThru: Trace ATA packets


Wu, Hao A
 

-----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

Join devel@edk2.groups.io to automatically receive all group messages.