On 08/30/17 22:45, Brijesh Singh wrote:
> When virtio request fails we return EFI_DEVICE_ERROR, as per the spec
> EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru() member function is required
> to implement elaborated error reporting.
>
> The patch refactors out entire block of the code that creates the host
> adapter error into a separate helper function (ReportHostAdapterError).
>
> Suggested-by: Laszlo Ersek <[email protected]>
> Cc: Ard Biesheuvel <[email protected]>
> Cc: Jordan Justen <[email protected]>
> Cc: Tom Lendacky <[email protected]>
> Cc: Laszlo Ersek <[email protected]>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Brijesh Singh <[email protected]>
> ---
> OvmfPkg/VirtioScsiDxe/VirtioScsi.c | 27 +++++++++++++++-----
> 1 file changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> index 5e72b1a24b59..cac213129409 100644
> --- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> +++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> @@ -387,6 +387,26 @@ ParseResponse (
> return EFI_DEVICE_ERROR;
> }
OK, I'm glad that you found the best place to introduce the new function
(just below ParseResponse()).
> +/**
> + * The function can be used to create a fake host adapter error.
> + * When VirtioScsiPassThru is failed due to some reasons then this function
> + * can be called to contstruct a host adapter error.
> + *
> + */
(1) This comment block should be formatted according to the edk2 coding
style, and it should document the Packet parameter, and the return
value:
-------
/**
The function can be used to create a fake host adapter error.
When VirtioScsiPassThru() is failed due to some reasons then this function
can be called to construct a host adapter error.
@param[out] Packet The Extended SCSI Pass Thru Protocol packet that the host
adapter error shall be placed in.
@retval EFI_DEVICE_ERROR The function returns this status code
unconditionally, to be propagated by
VirtioScsiPassThru().
**/
-------
There's no need to resubmit just because of this; if more serious
updates are not needed, I can take care of it.
(2) Another documentation update: pls see the following comment block
(higher up in the source code):
// UEFI Spec 2.3.1 + Errata C, 14.7 Extended SCSI Pass Thru Protocol specifies
// the PassThru() interface. Beside returning a status code, the function must
// set some fields in the EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET in/out
// parameter on return. The following is a full list of those fields, for
// easier validation of PopulateRequest(), ParseResponse(), and
// VirtioScsiPassThru() below.
After this patch, VirtioScsiPassThru() will no longer massage
Packet->xxx fields directly, only via helper functions.
So please replace the "VirtioScsiPassThru()" reference in the comment
block with "ReportHostAdapterError()".
I can take care of this as well, if v3 is not necessary otherwise.
With these documentation updates,
Reviewed-by: Laszlo Ersek <[email protected]>
Thanks,
Laszlo
> +STATIC
> +EFI_STATUS
> +ReportHostAdapterError (
> + OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet
> + )
> +{
> + Packet->InTransferLength = 0;
> + Packet->OutTransferLength = 0;
> + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
> + Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
> + Packet->SenseDataLength = 0;
> + return EFI_DEVICE_ERROR;
> +}
> +
>
> //
> // The next seven functions implement EFI_EXT_SCSI_PASS_THRU_PROTOCOL
> @@ -472,12 +492,7 @@ VirtioScsiPassThru (
> //
> if (VirtioFlush (Dev->VirtIo, VIRTIO_SCSI_REQUEST_QUEUE, &Dev->Ring,
> &Indices, NULL) != EFI_SUCCESS) {
> - Packet->InTransferLength = 0;
> - Packet->OutTransferLength = 0;
> - Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
> - Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
> - Packet->SenseDataLength = 0;
> - return EFI_DEVICE_ERROR;
> + return ReportHostAdapterError (Packet);
> }
>
> return ParseResponse (Packet, &Response);
>
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
