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

[Xu, Wei6]

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

BR,
Wei
-----Original Message-----
From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Michael Kubacki
Sent: Friday, November 6, 2020 11:16 AM
To: devel at edk2.groups.io; Liming Gao <gaoliming at byosoft.com.cn>; Kinney, Michael D <michael.d.kinney at intel.com>; Jiang, Guomin <guomin.jiang at intel.com>; Xu, Wei6 <wei6.xu at 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 at outlook.com wrote:
> From: Michael Kubacki <michael.kubacki at 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 at byosoft.com.cn>
> Cc: Michael D Kinney <michael.d.kinney at intel.com>
> Cc: Guomin Jiang <guomin.jiang at intel.com>
> Cc: Wei6 Xu <wei6.xu at intel.com>
> Signed-off-by: Michael Kubacki <michael.kubacki at 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;
>     }
>   
> 







-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#67076): https://edk2.groups.io/g/devel/message/67076
Mute This Topic: https://groups.io/mt/77922656/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-