Re: [PATCH v1 1/1] FmpDevicePkg/FmpDxe: Call FmpDeviceLib WithStatus() functions

["Michael Kubacki" <michael.kubacki@outlook.com>]

Hi FmpDevicePkg maintainers,

Please let me know your feedback on this patch. Note that it is required 
to complete the Last Attempt Status support already merged in the 
following patch series:
https://edk2.groups.io/g/devel/message/66418

Thanks,
Michael

On 10/30/2020 2:12 PM, michael.kubacki@outlook.com wrote:
> From: Michael Kubacki <michael.kubacki@microsoft.com>
> 
> Commit 6ad819c introduced two new functions in FmpDeviceLib:
> 1. FmpDeviceCheckImageWithStatus ()
> 2. FmpDeviceSetImageWithStatus ()
> 
> These functions allow an FmpDeviceLib implementation to return a
> Last Attempt Status code value within the Device Library range from
> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE to
> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MAX_ERROR_CODE_VALUE.
> 
> To maintain backward compatibility, commit 6ad819c did not update
> the FmpDxe driver to invoke these functions. FmpDeviceLib instances
> should update their FmpDeviceCheckImage () function to simply call
> FmpDeviceCheckImageWithStatus (). Similarly, FmpDeviceSetImage ()
> should simply call FmpDeviceSetImageWithStatus (). This is
> demonstrated in the implementation of these functions in
> FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLib.c. By doing so,
> the library can remain compatible with FmpDxe implementations before
> and after this transition.
> 
> This commit updates FmpDxe to call the WithStatus () version of
> these functions enabling the Last Attempt Status code returned to
> be accessible to FmpDxe.
> 
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Guomin Jiang <guomin.jiang@intel.com>
> Cc: Wei6 Xu <wei6.xu@intel.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
>   FmpDevicePkg/FmpDxe/FmpDxe.c | 40 ++++++++++++++++++--
>   1 file changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/FmpDevicePkg/FmpDxe/FmpDxe.c b/FmpDevicePkg/FmpDxe/FmpDxe.c
> index de7f1fe53e32..6b0675ea38f8 100644
> --- a/FmpDevicePkg/FmpDxe/FmpDxe.c
> +++ b/FmpDevicePkg/FmpDxe/FmpDxe.c
> @@ -1025,9 +1025,24 @@ CheckTheImageInternal (
>     //
>     // FmpDeviceLib CheckImage function to do any specific checks
>     //
> -  Status = FmpDeviceCheckImage ((((UINT8 *)Image) + AllHeaderSize), RawSize, ImageUpdatable);
> +  Status = FmpDeviceCheckImageWithStatus ((((UINT8 *) Image) + AllHeaderSize), RawSize, ImageUpdatable, LastAttemptStatus);
>     if (EFI_ERROR (Status)) {
>       DEBUG ((DEBUG_ERROR, "FmpDxe(%s): CheckTheImage() - FmpDeviceLib CheckImage failed. Status = %r\n", mImageIdName, Status));
> +
> +    //
> +    // LastAttemptStatus returned from the device library should fall within the designated error range
> +    // [LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE, LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MAX_ERROR_CODE_VALUE]
> +    //
> +    if ((*LastAttemptStatus < LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE) ||
> +        (*LastAttemptStatus > LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MAX_ERROR_CODE_VALUE)) {
> +      DEBUG (
> +        (DEBUG_ERROR,
> +        "FmpDxe(%s): CheckTheImage() - LastAttemptStatus %d from FmpDeviceCheckImageWithStatus() is invalid.\n",
> +        mImageIdName,
> +        *LastAttemptStatus)
> +        );
> +      *LastAttemptStatus = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL;
> +    }
>     }
>   
>   cleanup:
> @@ -1353,16 +1368,33 @@ SetTheImage (
>     //
>     //Copy the requested image to the firmware using the FmpDeviceLib
>     //
> -  Status = FmpDeviceSetImage (
> -             (((UINT8 *)Image) + AllHeaderSize),
> +  Status = FmpDeviceSetImageWithStatus (
> +             (((UINT8 *) Image) + AllHeaderSize),
>                ImageSize - AllHeaderSize,
>                VendorCode,
>                FmpDxeProgress,
>                IncomingFwVersion,
> -             AbortReason
> +             AbortReason,
> +             &LastAttemptStatus
>                );
>     if (EFI_ERROR (Status)) {
>       DEBUG ((DEBUG_ERROR, "FmpDxe(%s): SetTheImage() SetImage from FmpDeviceLib failed. Status =  %r.\n", mImageIdName, Status));
> +
> +    //
> +    // LastAttemptStatus returned from the device library should fall within the designated error range
> +    // [LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE, LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MAX_ERROR_CODE_VALUE]
> +    //
> +    if ((LastAttemptStatus < LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE) ||
> +        (LastAttemptStatus > LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MAX_ERROR_CODE_VALUE)) {
> +      DEBUG (
> +        (DEBUG_ERROR,
> +        "FmpDxe(%s): SetTheImage() - LastAttemptStatus %d from FmpDeviceSetImageWithStatus() is invalid.\n",
> +        mImageIdName,
> +        LastAttemptStatus)
> +        );
> +      LastAttemptStatus = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL;
> +    }
> +
>       goto cleanup;
>     }
>   
>