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

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

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

Pushed via:
PR - https://github.com/tianocore/edk2/pull/3746
Commit - https://github.com/tianocore/edk2/commit/a6542894391bae58b7704b2624b541a2b8b9ed93

Best Regards,
Hao Wu

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao A
Sent: Thursday, December 8, 2022 12:21 PM
To: devel@edk2.groups.io; Albecki, Mateusz <mateusz.albecki@intel.com>
Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg/Ata: Fix command status reporting

Thanks.

For the patch:
Reviewed-by: Hao A Wu <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>
Will merge the patch early next week.

For the open with regard to: https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c#L737
I think returning EFI_DEVICE_ERROR when something goes wrong in AhciResetPort() should be enough.
We can address this in another patch.

For retry of incorrect password, I agree with you that it would be better to distinguish such case from other failures that deserve retries.
We can improve this with a separate patch in the future as well.

Best Regards,
Hao Wu

From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Albecki, Mateusz
Sent: Thursday, December 8, 2022 12:25 AM
To: Albecki; Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg/Ata: Fix command status reporting

Hello,

Really sorry for missing the mails. Seems like my mail was misconfigured and I didn't get the messages(had to check the group site).

"I cannot remember why EFI_SUCCESS is eventually returned for the above error case. Could you help to remind me of the details? Thanks."

To answer this - I don't remember either, and that's potentially a bug. Failing to reset the device should render the device unusable and we probably should probably treat it as a hot unplug and block further packets on that port. This would require bigger change. Let me know if you want me to simply add return EFI_DEVICE_ERROR there to stop additional retries.

As for the patch failing to fix the  https://bugzilla.tianocore.org/show_bug.cgi?id=4011 it seems like I misunderstood the issue. My initial understanding was that we have the issue since we return success even when the password failed in following flow:

1. Incorrect password is supplied
2. Driver tries for 5 times and it fails 5 times
3. Driver returns success(bug that this patch is supposed to fix)
4. Higher level SW thinks the password is ok but in reality it wasn't(bug)

Seems like step 2 is also causing issues and we can't retry password 5 times. This will require driver to check for the error type and only retry on specific errors like CRC. Let me investigate more what can we check and what kinds of errors should be retried and come back to this thread. In the meantime I still think we can submit the change to solve failing SCT tests.

Thanks,
Mateusz


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