Hello Mateusz, Thanks for the patch, I think the patch itself is good.
Sorry for one question came up when looking into the patch, for the implementation of AhciRecoverPortError() https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c#L737: Status = AhciResetPort (PciIo, Port); if (EFI_ERROR (Status)) { DEBUG ((DEBUG_ERROR, "Failed to reset the port %d\n", Port)); } I cannot remember why EFI_SUCCESS is eventually returned for the above error case. Could you help to remind me of the details? Thanks. Best Regards, Hao Wu > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao A > Sent: Wednesday, October 19, 2022 9:37 AM > To: Anbazhagan, Baraneedharan <anbazha...@hp.com>; Albecki, Mateusz > <mateusz.albe...@intel.com>; devel@edk2.groups.io > Cc: Ni, Ray <ray...@intel.com> > Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg/Ata: Fix command status > reporting > > Hello Baraneedharan Anbazhagan, > > Could you help to check if this patch can resolve the issue > https://bugzilla.tianocore.org/show_bug.cgi?id=4011 when switching back to: > "#define AHCI_COMMAND_RETRIES 5"? > This change can be accessed for integration at: > https://github.com/tianocore/edk2/commit/f2eb27e00ed55cca03964f13a2839 > e41e2c1f61f.patch > > Best Regards, > Hao Wu > > > -----Original Message----- > > From: Albecki, Mateusz <mateusz.albe...@intel.com> > > Sent: Tuesday, October 18, 2022 11:54 PM > > To: devel@edk2.groups.io > > Cc: Albecki, Mateusz <mateusz.albe...@intel.com>; Wu, Hao A > > <hao.a...@intel.com>; Ni, Ray <ray...@intel.com> > > Subject: [PATCH 1/1] MdeModulePkg/Ata: Fix command status reporting > > > > Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=4016 > > > > AtaAtapiPassThru driver was reporting recovery status on failed > > command packets which led to incorrect flows in upper layers and to > > SCT tests fails. This commit will change the logic to report command status. > > > > > > Cc: Hao A Wu <hao.a...@intel.com> > > > > Cc: Ray Ni <ray...@intel.com> > > > > > > Signed-off-by: Mateusz Albecki <mateusz.albe...@intel.com> > > --- > > .../Bus/Ata/AtaAtapiPassThru/AhciMode.c | 19 +++++++++++-------- > > 1 file changed, 11 insertions(+), 8 deletions(-) > > > > diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c > > b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c > > index a240be940d..06c4a3e052 100644 > > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c > > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c > > @@ -949,6 +949,7 @@ AhciPioTransfer ( > > EFI_AHCI_COMMAND_LIST CmdList; > > > > UINT32 PrdCount; > > > > UINT32 Retry; > > > > + EFI_STATUS RecoveryStatus; > > > > > > > > if (Read) { > > > > Flag = EfiPciIoOperationBusMasterWrite; > > > > @@ -1026,8 +1027,8 @@ AhciPioTransfer ( > > > > > > if (Status == EFI_DEVICE_ERROR) { > > > > DEBUG ((DEBUG_ERROR, "PIO command failed at retry %d\n", > > Retry)); > > > > - Status = AhciRecoverPortError (PciIo, Port); > > > > - if (EFI_ERROR (Status)) { > > > > + RecoveryStatus = AhciRecoverPortError (PciIo, Port); > > > > + if (EFI_ERROR (RecoveryStatus)) { > > > > break; > > > > } > > > > } else { > > > > @@ -1122,6 +1123,7 @@ AhciDmaTransfer ( > > EFI_PCI_IO_PROTOCOL *PciIo; > > > > EFI_TPL OldTpl; > > > > UINT32 Retry; > > > > + EFI_STATUS RecoveryStatus; > > > > > > > > Map = NULL; > > > > PciIo = Instance->PciIo; > > > > @@ -1220,8 +1222,8 @@ AhciDmaTransfer ( > > 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)) { > > > > + RecoveryStatus = AhciRecoverPortError (PciIo, Port); > > > > + if (EFI_ERROR (RecoveryStatus)) { > > > > break; > > > > } > > > > } else { > > > > @@ -1261,14 +1263,14 @@ AhciDmaTransfer ( > > 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); > > > > + RecoveryStatus = 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) { > > > > + if (RecoveryStatus == EFI_SUCCESS) { > > > > Task->IsStart = FALSE; > > > > Status = EFI_NOT_READY; > > > > } > > > > @@ -1375,6 +1377,7 @@ AhciNonDataTransfer ( > > EFI_AHCI_COMMAND_FIS CFis; > > > > EFI_AHCI_COMMAND_LIST CmdList; > > > > UINT32 Retry; > > > > + EFI_STATUS RecoveryStatus; > > > > > > > > // > > > > // Package read needed > > > > @@ -1415,8 +1418,8 @@ AhciNonDataTransfer ( > > 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)) { > > > > + RecoveryStatus = AhciRecoverPortError (PciIo, Port); > > > > + if (EFI_ERROR (RecoveryStatus)) { > > > > break; > > > > } > > > > } else { > > > > -- > > 2.28.0.windows.1 > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#95353): https://edk2.groups.io/g/devel/message/95353 Mute This Topic: https://groups.io/mt/94411389/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
