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

["Xu, Wei6" <wei6.xu@intel.com>]

This patch is good to me.
Reviewed-by: Wei6 Xu <wei6.xu@intel.com>

BR,
Wei
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki
Sent: Friday, November 6, 2020 11:16 AM
To: devel@edk2.groups.io; Liming Gao <gaoliming@byosoft.com.cn>; Kinney, Michael D <michael.d.kinney@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>; Xu, Wei6 <wei6.xu@intel.com>
Subject: Re: [edk2-devel] [PATCH v1 1/1] FmpDevicePkg/FmpDxe: Call FmpDeviceLib WithStatus() functions

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;
>     }
>   
>