Re: [edk2-devel] [PATCH v3 6/6] OvmfPkg/VirtNorFlashDxe: move DoErase code block into new function

["Laszlo Ersek" <lersek@redhat.com>]

On 1/16/24 18:11, Gerd Hoffmann wrote:
> Move the DoErase code block into a separate function, call the function
> instead of jumping around with goto.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c | 76 ++++++++++++++++++--------
>  1 file changed, 52 insertions(+), 24 deletions(-)
> 
> diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> index 3d1d20daa1e5..e6aaed27ceba 100644
> --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> @@ -502,6 +502,38 @@ NorFlashRead (
>    return EFI_SUCCESS;
>  }
>  
> +STATIC
> +EFI_STATUS
> +NorFlashWriteSingleBlockWithErase (
> +  IN        NOR_FLASH_INSTANCE  *Instance,
> +  IN        EFI_LBA             Lba,
> +  IN        UINTN               Offset,
> +  IN OUT    UINTN               *NumBytes,
> +  IN        UINT8               *Buffer
> +  )
> +{
> +  EFI_STATUS  Status;
> +
> +  // Read NOR Flash data into shadow buffer
> +  Status = NorFlashReadBlocks (Instance, Lba, Instance->BlockSize, Instance->ShadowBuffer);
> +  if (EFI_ERROR (Status)) {
> +    // Return one of the pre-approved error statuses
> +    return EFI_DEVICE_ERROR;
> +  }
> +
> +  // Put the data at the appropriate location inside the buffer area
> +  CopyMem ((VOID *)((UINTN)Instance->ShadowBuffer + Offset), Buffer, *NumBytes);
> +
> +  // Write the modified buffer back to the NorFlash
> +  Status = NorFlashWriteBlocks (Instance, Lba, Instance->BlockSize, Instance->ShadowBuffer);
> +  if (EFI_ERROR (Status)) {
> +    // Return one of the pre-approved error statuses
> +    return EFI_DEVICE_ERROR;
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
>  /*
>    Write a full or portion of a block. It must not span block boundaries; that is,
>    Offset + *NumBytes <= Instance->BlockSize.
> @@ -607,7 +639,14 @@ NorFlashWriteSingleBlock (
>      // that we want to set. In that case, we will need to erase the block first.
>      for (CurOffset = 0; CurOffset < *NumBytes; CurOffset++) {
>        if (~(UINT32)OrigData[CurOffset] & (UINT32)Buffer[CurOffset]) {
> -        goto DoErase;
> +        Status = NorFlashWriteSingleBlockWithErase (
> +                   Instance,
> +                   Lba,
> +                   Offset,
> +                   NumBytes,
> +                   Buffer
> +                   );
> +        return Status;
>        }
>  
>        OrigData[CurOffset] = Buffer[CurOffset];
> @@ -636,33 +675,22 @@ NorFlashWriteSingleBlock (
>          goto Exit;
>        }
>      }
> -
> -Exit:
> -    // Put device back into Read Array mode
> -    SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY);
> -
> +  } else {
> +    Status = NorFlashWriteSingleBlockWithErase (
> +               Instance,
> +               Lba,
> +               Offset,
> +               NumBytes,
> +               Buffer
> +               );
>      return Status;
>    }
>  
> -DoErase:
> -  // Read NOR Flash data into shadow buffer
> -  Status = NorFlashReadBlocks (Instance, Lba, BlockSize, Instance->ShadowBuffer);
> -  if (EFI_ERROR (Status)) {
> -    // Return one of the pre-approved error statuses
> -    return EFI_DEVICE_ERROR;
> -  }
> +Exit:
> +  // Put device back into Read Array mode
> +  SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY);
>  
> -  // Put the data at the appropriate location inside the buffer area
> -  CopyMem ((VOID *)((UINTN)Instance->ShadowBuffer + Offset), Buffer, *NumBytes);
> -
> -  // Write the modified buffer back to the NorFlash
> -  Status = NorFlashWriteBlocks (Instance, Lba, BlockSize, Instance->ShadowBuffer);
> -  if (EFI_ERROR (Status)) {
> -    // Return one of the pre-approved error statuses
> -    return EFI_DEVICE_ERROR;
> -  }
> -
> -  return EFI_SUCCESS;
> +  return Status;
>  }
>  
>  EFI_STATUS

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

This series is now ready to be merged, but I'm unsure what the CI status
is, after the recent security fixes seem to have caused breakage with
multiple external definitions of the same symbol. For now I'll keep this
tagged, as a reminder for myself.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113944): https://edk2.groups.io/g/devel/message/113944
Mute This Topic: https://groups.io/mt/103766779/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-