Re: [edk2-devel] [PATCHv2 0/4] MdeModulePkg/AtaAtapiPassThru: Add SATA error recovery

Re: [edk2-devel] [PATCHv2 0/4] MdeModulePkg/AtaAtapiPassThru: Add SATA error recovery

[Albecki, Mateusz]

Forgot to put BZ tracker into v2 commit msg so I have resent the series as v3 with proper bz info.

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Albecki,
> Mateusz
> Sent: Thursday, November 5, 2020 1:41 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: [edk2-devel] [PATCHv2 0/4] MdeModulePkg/AtaAtapiPassThru:
> Add SATA error recovery
> 
> To increase boot stability when booting from SATA drives SATA driver should
> implement the AHCI spec defined port error recovery. This will allow the
> driver to handle random fails on SATA link.
> 
> Performed tests on 2 setups. One with AHCI controller booting OS
> successfully without error recovery(control setup) and other which fails 1 in 5
> times(fail setup).
> 
> Tests performed:
> 1. Booted control setup to OS successfully.
> 2. Checked if during normal boot none of the packets is repeated(this came
> up after previous code version had a bug which repeated each DMA packet 5
> times).
> 3. Booted control setup to OS with simulated errors appearing on first packet
> of every DMA transaction.
> 4. Performed extensive tests on fail setup. Fail rate decreased from 20%
> failure to ~1% failure. 1% failure is observed during OS execution, not BIOS so
> in a way boot is 100% stable).
> 
> Change pushed to github:
> This series:
> https://github.com/malbecki/edk2/commits/sata_recovery2
> Simulated errors:
> https://github.com/malbecki/edk2/commits/sata_recovery_simulated_erro
> r
> 
> Some more information:
> For the bug mentioned above in point 2, the discussion can be referred here:
> https://github.com/malbecki/edk2/commit/9ea81cadf38725e194ec01e0b0c5
> 56fd133f3ced#r43226067
> 
> For the other discussion we did before this patch, please refer to:
> https://github.com/malbecki/edk2/commits/sata_recovery
> 
> For logging here is the example of verbose command log(taken from v1
> patch):
> Starting commmand:
> ATA COMMAND BLOCK:
> AtaCommand: 37
> AtaFeatures: 0
> AtaSectorNumber: 0
> AtaCylinderLow: 16
> AtaCylinderHigh: 16
> AtaDeviceHead: 224
> AtaSectorNumberExp: 57
> AtaCylinderLowExp: 0
> AtaCylinderHighExp: 0
> AtaFeaturesExp: 0
> AtaSectorCount: 64
> AtaSectorCountExp: 0
> DMA command failed at retry: 0
> DMA retry 1
> Starting commmand:
> ATA COMMAND BLOCK:
> AtaCommand: 37
> AtaFeatures: 0
> AtaSectorNumber: 0
> AtaCylinderLow: 16
> AtaCylinderHigh: 16
> AtaDeviceHead: 224
> AtaSectorNumberExp: 57
> AtaCylinderLowExp: 0
> AtaCylinderHighExp: 0
> AtaFeaturesExp: 0
> AtaSectorCount: 64
> AtaSectorCountExp: 0
> ATA STATUS BLOCK:
> AtaStatus: 80
> AtaError: 0
> DMA retry 0
> 
> Changes in v2:
> - Changed logging per review suggestions
> - Fixed small issues with commit message
> 
> 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>
> 
> 
> Mateusz Albecki (4):
>   MdeModulePkg/AtaAtapiPassThru: Check IS to check for command
>     completion
>   MdeModulePkg/AtaAtapiPassThru: Add SATA error recovery flow
>   MdeModulePkg/AtaAtapiPassThru: Restart failed packets
>   MdeModulePkg/AtaAtapiPassThru: Trace ATA packets
> 
>  .../Bus/Ata/AtaAtapiPassThru/AhciMode.c       | 799 +++++++++++-------
>  .../Bus/Ata/AtaAtapiPassThru/AhciMode.h       |  18 +-
>  2 files changed, 532 insertions(+), 285 deletions(-)
> 
> --
> 2.28.0.windows.1
> 
> ---------------------------------------------------------------------
> Intel Technology Poland sp. z o.o.
> ul. Sowackiego 173 | 80-298 Gdask | Sd Rejonowy Gdask Pnoc | VII Wydzia
> Gospodarczy Krajowego Rejestru Sdowego - KRS 101882 | NIP 957-07-52-316
> | Kapita zakadowy 200.000 PLN.
> Ta wiadomo wraz z zacznikami jest przeznaczona dla okrelonego adresata i
> moe zawiera informacje poufne. W razie przypadkowego otrzymania tej
> wiadomoci, prosimy o powiadomienie nadawcy oraz trwae jej usunicie;
> jakiekolwiek przegldanie lub rozpowszechnianie jest zabronione.
> This e-mail and any attachments may contain confidential material for the
> sole use of the intended recipient(s). If you are not the intended recipient,
> please contact the sender and delete all copies; any review or distribution by
> others is strictly prohibited.
> 
> 
> 
> 
> 
> 

---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Sowackiego 173 | 80-298 Gdask | Sd Rejonowy Gdask Pnoc | VII Wydzia Gospodarczy Krajowego Rejestru Sdowego - KRS 101882 | NIP 957-07-52-316 | Kapita zakadowy 200.000 PLN.
Ta wiadomo wraz z zacznikami jest przeznaczona dla okrelonego adresata i moe zawiera informacje poufne. W razie przypadkowego otrzymania tej wiadomoci, prosimy o powiadomienie nadawcy oraz trwae jej usunicie; jakiekolwiek przegldanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.
 

Re: [edk2-devel] [PATCHv2 0/4] MdeModulePkg/AtaAtapiPassThru: Add SATA error recovery

[Wu, Hao A]

> -----Original Message-----
> From: Albecki, Mateusz <mateusz.albecki@intel.com>
> Sent: Thursday, November 5, 2020 8:51 PM
> To: devel@edk2.groups.io; Albecki, Mateusz <mateusz.albecki@intel.com>
> Cc: Ni, Ray <ray.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> Subject: RE: [edk2-devel] [PATCHv2 0/4] MdeModulePkg/AtaAtapiPassThru:
> Add SATA error recovery
> 
> Forgot to put BZ tracker into v2 commit msg so I have resent the series as v3
> with proper bz info.


(Seems no separate cover-letter sent for V3, replies here)
I have given my 'R-b' tags for the V3 series:
https://edk2.groups.io/g/devel/topic/78049844#67059
https://edk2.groups.io/g/devel/topic/78049845#67060
https://edk2.groups.io/g/devel/topic/78049846#67061
https://edk2.groups.io/g/devel/topic/78049847#67063

Will wait a couple of days to see if there are comments from others.

Best Regards,
Hao Wu


> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > Albecki, Mateusz
> > Sent: Thursday, November 5, 2020 1:41 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: [edk2-devel] [PATCHv2 0/4] MdeModulePkg/AtaAtapiPassThru:
> > Add SATA error recovery
> >
> > To increase boot stability when booting from SATA drives SATA driver
> > should implement the AHCI spec defined port error recovery. This will
> > allow the driver to handle random fails on SATA link.
> >
> > Performed tests on 2 setups. One with AHCI controller booting OS
> > successfully without error recovery(control setup) and other which
> > fails 1 in 5 times(fail setup).
> >
> > Tests performed:
> > 1. Booted control setup to OS successfully.
> > 2. Checked if during normal boot none of the packets is repeated(this
> > came up after previous code version had a bug which repeated each DMA
> > packet 5 times).
> > 3. Booted control setup to OS with simulated errors appearing on first
> > packet of every DMA transaction.
> > 4. Performed extensive tests on fail setup. Fail rate decreased from
> > 20% failure to ~1% failure. 1% failure is observed during OS
> > execution, not BIOS so in a way boot is 100% stable).
> >
> > Change pushed to github:
> > This series:
> > https://github.com/malbecki/edk2/commits/sata_recovery2
> > Simulated errors:
> >
> https://github.com/malbecki/edk2/commits/sata_recovery_simulated_erro
> > r
> >
> > Some more information:
> > For the bug mentioned above in point 2, the discussion can be referred
> here:
> >
> https://github.com/malbecki/edk2/commit/9ea81cadf38725e194ec01e0b0c5
> > 56fd133f3ced#r43226067
> >
> > For the other discussion we did before this patch, please refer to:
> > https://github.com/malbecki/edk2/commits/sata_recovery
> >
> > For logging here is the example of verbose command log(taken from v1
> > patch):
> > Starting commmand:
> > ATA COMMAND BLOCK:
> > AtaCommand: 37
> > AtaFeatures: 0
> > AtaSectorNumber: 0
> > AtaCylinderLow: 16
> > AtaCylinderHigh: 16
> > AtaDeviceHead: 224
> > AtaSectorNumberExp: 57
> > AtaCylinderLowExp: 0
> > AtaCylinderHighExp: 0
> > AtaFeaturesExp: 0
> > AtaSectorCount: 64
> > AtaSectorCountExp: 0
> > DMA command failed at retry: 0
> > DMA retry 1
> > Starting commmand:
> > ATA COMMAND BLOCK:
> > AtaCommand: 37
> > AtaFeatures: 0
> > AtaSectorNumber: 0
> > AtaCylinderLow: 16
> > AtaCylinderHigh: 16
> > AtaDeviceHead: 224
> > AtaSectorNumberExp: 57
> > AtaCylinderLowExp: 0
> > AtaCylinderHighExp: 0
> > AtaFeaturesExp: 0
> > AtaSectorCount: 64
> > AtaSectorCountExp: 0
> > ATA STATUS BLOCK:
> > AtaStatus: 80
> > AtaError: 0
> > DMA retry 0
> >
> > Changes in v2:
> > - Changed logging per review suggestions
> > - Fixed small issues with commit message
> >
> > 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>
> >
> >
> > Mateusz Albecki (4):
> >   MdeModulePkg/AtaAtapiPassThru: Check IS to check for command
> >     completion
> >   MdeModulePkg/AtaAtapiPassThru: Add SATA error recovery flow
> >   MdeModulePkg/AtaAtapiPassThru: Restart failed packets
> >   MdeModulePkg/AtaAtapiPassThru: Trace ATA packets
> >
> >  .../Bus/Ata/AtaAtapiPassThru/AhciMode.c       | 799 +++++++++++-------
> >  .../Bus/Ata/AtaAtapiPassThru/AhciMode.h       |  18 +-
> >  2 files changed, 532 insertions(+), 285 deletions(-)
> >
> > --
> > 2.28.0.windows.1
> >
> > ---------------------------------------------------------------------
> > Intel Technology Poland sp. z o.o.
> > ul. Sowackiego 173 | 80-298 Gdask | Sd Rejonowy Gdask Pnoc | VII
> > Wydzia Gospodarczy Krajowego Rejestru Sdowego - KRS 101882 | NIP
> > 957-07-52-316
> > | Kapita zakadowy 200.000 PLN.
> > Ta wiadomo wraz z zacznikami jest przeznaczona dla okrelonego adresata
> > i moe zawiera informacje poufne. W razie przypadkowego otrzymania tej
> > wiadomoci, prosimy o powiadomienie nadawcy oraz trwae jej usunicie;
> > jakiekolwiek przegldanie lub rozpowszechnianie jest zabronione.
> > This e-mail and any attachments may contain confidential material for
> > the sole use of the intended recipient(s). If you are not the intended
> > recipient, please contact the sender and delete all copies; any review
> > or distribution by others is strictly prohibited.
> >
> >
> >
> >
> > 
> >

Re: [edk2-devel] [PATCHv2 0/4] MdeModulePkg/AtaAtapiPassThru: Add SATA error recovery

[Wu, Hao A]

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao
> A
> Sent: Monday, November 9, 2020 9:17 AM
> To: Albecki, Mateusz <mateusz.albecki@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [PATCHv2 0/4] MdeModulePkg/AtaAtapiPassThru:
> Add SATA error recovery
> 
> > -----Original Message-----
> > From: Albecki, Mateusz <mateusz.albecki@intel.com>
> > Sent: Thursday, November 5, 2020 8:51 PM
> > To: devel@edk2.groups.io; Albecki, Mateusz <mateusz.albecki@intel.com>
> > Cc: Ni, Ray <ray.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> > Subject: RE: [edk2-devel] [PATCHv2 0/4]
> MdeModulePkg/AtaAtapiPassThru:
> > Add SATA error recovery
> >
> > Forgot to put BZ tracker into v2 commit msg so I have resent the
> > series as v3 with proper bz info.
> 
> 
> (Seems no separate cover-letter sent for V3, replies here) I have given my 'R-
> b' tags for the V3 series:
> https://edk2.groups.io/g/devel/topic/78049844#67059
> https://edk2.groups.io/g/devel/topic/78049845#67060
> https://edk2.groups.io/g/devel/topic/78049846#67061
> https://edk2.groups.io/g/devel/topic/78049847#67063
> 
> Will wait a couple of days to see if there are comments from others.


V3 series has been pushed via:
PR:
https://github.com/tianocore/edk2/pull/1113

Commits:
https://github.com/tianocore/edk2/commit/cc28ab7a1d7353ad7ade55d19e58e85c50d8fa4d
https://github.com/tianocore/edk2/commit/b465a811006eb1663068380dded041d287ace907
https://github.com/tianocore/edk2/commit/64e25d4b062c907dab2dd30b686de9219d8e372c
https://github.com/tianocore/edk2/commit/91d95113d07aee4a11e9dac02c1a836c7360d422

Best Regards,
Hao Wu


> 
> Best Regards,
> Hao Wu
> 
> 
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > Albecki, Mateusz
> > > Sent: Thursday, November 5, 2020 1:41 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: [edk2-devel] [PATCHv2 0/4] MdeModulePkg/AtaAtapiPassThru:
> > > Add SATA error recovery
> > >
> > > To increase boot stability when booting from SATA drives SATA driver
> > > should implement the AHCI spec defined port error recovery. This
> > > will allow the driver to handle random fails on SATA link.
> > >
> > > Performed tests on 2 setups. One with AHCI controller booting OS
> > > successfully without error recovery(control setup) and other which
> > > fails 1 in 5 times(fail setup).
> > >
> > > Tests performed:
> > > 1. Booted control setup to OS successfully.
> > > 2. Checked if during normal boot none of the packets is
> > > repeated(this came up after previous code version had a bug which
> > > repeated each DMA packet 5 times).
> > > 3. Booted control setup to OS with simulated errors appearing on
> > > first packet of every DMA transaction.
> > > 4. Performed extensive tests on fail setup. Fail rate decreased from
> > > 20% failure to ~1% failure. 1% failure is observed during OS
> > > execution, not BIOS so in a way boot is 100% stable).
> > >
> > > Change pushed to github:
> > > This series:
> > > https://github.com/malbecki/edk2/commits/sata_recovery2
> > > Simulated errors:
> > >
> >
> https://github.com/malbecki/edk2/commits/sata_recovery_simulated_erro
> > > r
> > >
> > > Some more information:
> > > For the bug mentioned above in point 2, the discussion can be
> > > referred
> > here:
> > >
> >
> https://github.com/malbecki/edk2/commit/9ea81cadf38725e194ec01e0b0c5
> > > 56fd133f3ced#r43226067
> > >
> > > For the other discussion we did before this patch, please refer to:
> > > https://github.com/malbecki/edk2/commits/sata_recovery
> > >
> > > For logging here is the example of verbose command log(taken from v1
> > > patch):
> > > Starting commmand:
> > > ATA COMMAND BLOCK:
> > > AtaCommand: 37
> > > AtaFeatures: 0
> > > AtaSectorNumber: 0
> > > AtaCylinderLow: 16
> > > AtaCylinderHigh: 16
> > > AtaDeviceHead: 224
> > > AtaSectorNumberExp: 57
> > > AtaCylinderLowExp: 0
> > > AtaCylinderHighExp: 0
> > > AtaFeaturesExp: 0
> > > AtaSectorCount: 64
> > > AtaSectorCountExp: 0
> > > DMA command failed at retry: 0
> > > DMA retry 1
> > > Starting commmand:
> > > ATA COMMAND BLOCK:
> > > AtaCommand: 37
> > > AtaFeatures: 0
> > > AtaSectorNumber: 0
> > > AtaCylinderLow: 16
> > > AtaCylinderHigh: 16
> > > AtaDeviceHead: 224
> > > AtaSectorNumberExp: 57
> > > AtaCylinderLowExp: 0
> > > AtaCylinderHighExp: 0
> > > AtaFeaturesExp: 0
> > > AtaSectorCount: 64
> > > AtaSectorCountExp: 0
> > > ATA STATUS BLOCK:
> > > AtaStatus: 80
> > > AtaError: 0
> > > DMA retry 0
> > >
> > > Changes in v2:
> > > - Changed logging per review suggestions
> > > - Fixed small issues with commit message
> > >
> > > 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>
> > >
> > >
> > > Mateusz Albecki (4):
> > >   MdeModulePkg/AtaAtapiPassThru: Check IS to check for command
> > >     completion
> > >   MdeModulePkg/AtaAtapiPassThru: Add SATA error recovery flow
> > >   MdeModulePkg/AtaAtapiPassThru: Restart failed packets
> > >   MdeModulePkg/AtaAtapiPassThru: Trace ATA packets
> > >
> > >  .../Bus/Ata/AtaAtapiPassThru/AhciMode.c       | 799 +++++++++++-------
> > >  .../Bus/Ata/AtaAtapiPassThru/AhciMode.h       |  18 +-
> > >  2 files changed, 532 insertions(+), 285 deletions(-)
> > >
> > > --
> > > 2.28.0.windows.1
> > >
> > > --------------------------------------------------------------------
> > > -
> > > Intel Technology Poland sp. z o.o.
> > > ul. Sowackiego 173 | 80-298 Gdask | Sd Rejonowy Gdask Pnoc | VII
> > > Wydzia Gospodarczy Krajowego Rejestru Sdowego - KRS 101882 | NIP
> > > 957-07-52-316
> > > | Kapita zakadowy 200.000 PLN.
> > > Ta wiadomo wraz z zacznikami jest przeznaczona dla okrelonego
> > > adresata i moe zawiera informacje poufne. W razie przypadkowego
> > > otrzymania tej wiadomoci, prosimy o powiadomienie nadawcy oraz trwae
> > > jej usunicie; jakiekolwiek przegldanie lub rozpowszechnianie jest
> zabronione.
> > > This e-mail and any attachments may contain confidential material
> > > for the sole use of the intended recipient(s). If you are not the
> > > intended recipient, please contact the sender and delete all copies;
> > > any review or distribution by others is strictly prohibited.
> > >
> > >
> > >
> > >
> > >
> > >
> 
> 
> 
> 
>