Re: [edk2-devel] [PATCH 1/1] MdeModulePkg/Ata: Fix command status reporting

["Wu, Hao A" <hao.a.wu@intel.com>]

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 <anbazhagan@hp.com>; Albecki, Mateusz
> <mateusz.albecki@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@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.albecki@intel.com>
> > Sent: Tuesday, October 18, 2022 11:54 PM
> > To: devel@edk2.groups.io
> > Cc: Albecki, Mateusz <mateusz.albecki@intel.com>; Wu, Hao A
> > <hao.a.wu@intel.com>; Ni, Ray <ray.ni@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.wu@intel.com>
> >
> > Cc: Ray Ni <ray.ni@intel.com>
> >
> >
> > Signed-off-by: Mateusz Albecki <mateusz.albecki@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
> 
> 
> 
> 
>