[Linaro-uefi,Linaro-uefi,v1,20/21] Hisilicon: Fixed SCT MediaAccessTest\BlockIOProtocolTest issue

Message ID 1490015485-53685-21-git-send-email-chenhui.sun@linaro.org
State New
Headers show
Series
  • D02/D03 platforms bug fix
Related show

Commit Message

Chenhui Sun March 20, 2017, 1:11 p.m.
EFI_BLOCK_IO_PROTOCOL.ReadBlocks - ReadBlocks() returns valid parameter

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: wangljjun <wanglijun@huawei.com>
Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
Signed-off-by: Yi Li <phoenix.liyi@huawei.com>
---
 Chips/Hisilicon/Drivers/FlashFvbDxe/FlashBlockIoDxe.c | 12 ++++++++++--
 Chips/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c     |  2 +-
 Drivers/Block/ramdisk/ramdisk.c                       | 18 +++++++++++-------
 3 files changed, 22 insertions(+), 10 deletions(-)

Comments

Leif Lindholm March 21, 2017, 5:58 p.m. | #1
On Mon, Mar 20, 2017 at 09:11:24PM +0800, Chenhui Sun wrote:
> EFI_BLOCK_IO_PROTOCOL.ReadBlocks - ReadBlocks() returns valid parameter

More description please.
What problem does this patch solve?

> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: wangljjun <wanglijun@huawei.com>
> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> Signed-off-by: Yi Li <phoenix.liyi@huawei.com>
> ---
>  Chips/Hisilicon/Drivers/FlashFvbDxe/FlashBlockIoDxe.c | 12 ++++++++++--
>  Chips/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c     |  2 +-
>  Drivers/Block/ramdisk/ramdisk.c                       | 18 +++++++++++-------
>  3 files changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/Chips/Hisilicon/Drivers/FlashFvbDxe/FlashBlockIoDxe.c b/Chips/Hisilicon/Drivers/FlashFvbDxe/FlashBlockIoDxe.c
> index 91c0733..5d5f3fa 100644
> --- a/Chips/Hisilicon/Drivers/FlashFvbDxe/FlashBlockIoDxe.c
> +++ b/Chips/Hisilicon/Drivers/FlashFvbDxe/FlashBlockIoDxe.c
> @@ -29,10 +29,14 @@ FlashBlockIoReadBlocks (
>      OUT VOID*                     Buffer
>  )
>  {
> -    FLASH_INSTANCE*   Instance;
> -    EFI_STATUS          Status;
> +    FLASH_INSTANCE*      Instance;
> +    EFI_STATUS           Status;
> +    EFI_BLOCK_IO_MEDIA   *Media;
> +    UINTN                IoAlign;
>  
>      Instance = INSTANCE_FROM_BLKIO_THIS(This);
> +    Media    = This->Media;
> +    IoAlign  = Media->IoAlign;
>  
>      DEBUG ((EFI_D_INFO, "FlashBlockIoReadBlocks(MediaId=0x%x, Lba=%ld, BufferSize=0x%x bytes (%d kB), BufferPtr @ 0x%08x)\n", MediaId, Lba, BufferSizeInBytes, Buffer));
>  
> @@ -44,6 +48,10 @@ FlashBlockIoReadBlocks (
>      {
>          Status = EFI_MEDIA_CHANGED;
>      }
> +    else if (IoAlign > 0 && (((UINTN) Buffer & (IoAlign - 1)) != 0))
> +    {

Existing code is incorrectly formatted. No need to fix for this patch
(I should have caught that on first review), but please ensure lines
you add or modify are of form:

  if () {
  } else if () {
  } else {
  }

> +        Status = EFI_INVALID_PARAMETER;
> +    }
>      else
>      {
>          Status = FlashReadBlocks (Instance, Lba, BufferSizeInBytes, Buffer);
> diff --git a/Chips/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c b/Chips/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c
> index d118a90..c719d53 100644
> --- a/Chips/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c
> +++ b/Chips/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c
> @@ -1125,7 +1125,7 @@ FlashReadBlocks (
>      // We must have some bytes to read
>      if (BufferSizeInBytes == 0)
>      {
> -        return EFI_BAD_BUFFER_SIZE;
> +        return EFI_SUCCESS;

Is this to make a read of size 0 succeed?

>      }
>  
>      // The size of the buffer must be a multiple of the block size
> diff --git a/Drivers/Block/ramdisk/ramdisk.c b/Drivers/Block/ramdisk/ramdisk.c
> index bd2607d..14c2f0e 100644
> --- a/Drivers/Block/ramdisk/ramdisk.c
> +++ b/Drivers/Block/ramdisk/ramdisk.c
> @@ -481,14 +481,18 @@ STATIC EFI_STATUS RamDiskReadBlocks(
>  
>    Media = This->Media;
>  
> -  if(BufferSize % Media->BlockSize != 0)

In this situation, I would prefer if you could leave this line badly
formatted. The diff is made much more difficult to read by your
correction of the coding style.

> +  if (Media->MediaId != MediaId) {
> +    return EFI_MEDIA_CHANGED;
> +  }
> +  if (BufferSize % Media->BlockSize != 0) {
>      return EFI_BAD_BUFFER_SIZE;
> -
> -  if(LBA > Media->LastBlock)
> -    return EFI_DEVICE_ERROR;
> -
> -  if(LBA + BufferSize / Media->BlockSize - 1 > Media->LastBlock)
> -    return EFI_DEVICE_ERROR;
> +  }
> +  if (BufferSize == 0) {
> +    return EFI_SUCCESS;
> +  }
> +  if (LBA + BufferSize / Media->BlockSize - 1 > Media->LastBlock) {
> +    return EFI_INVALID_PARAMETER;
> +  }
>  
>    RamDiskDev = RAM_DISK_FROM_THIS(This);
>    RamDiskLBA = RamDiskDev->Start + MultU64x32(LBA,Media->BlockSize);
> -- 
> 1.9.1
>

Patch hide | download patch | download mbox

diff --git a/Chips/Hisilicon/Drivers/FlashFvbDxe/FlashBlockIoDxe.c b/Chips/Hisilicon/Drivers/FlashFvbDxe/FlashBlockIoDxe.c
index 91c0733..5d5f3fa 100644
--- a/Chips/Hisilicon/Drivers/FlashFvbDxe/FlashBlockIoDxe.c
+++ b/Chips/Hisilicon/Drivers/FlashFvbDxe/FlashBlockIoDxe.c
@@ -29,10 +29,14 @@  FlashBlockIoReadBlocks (
     OUT VOID*                     Buffer
 )
 {
-    FLASH_INSTANCE*   Instance;
-    EFI_STATUS          Status;
+    FLASH_INSTANCE*      Instance;
+    EFI_STATUS           Status;
+    EFI_BLOCK_IO_MEDIA   *Media;
+    UINTN                IoAlign;
 
     Instance = INSTANCE_FROM_BLKIO_THIS(This);
+    Media    = This->Media;
+    IoAlign  = Media->IoAlign;
 
     DEBUG ((EFI_D_INFO, "FlashBlockIoReadBlocks(MediaId=0x%x, Lba=%ld, BufferSize=0x%x bytes (%d kB), BufferPtr @ 0x%08x)\n", MediaId, Lba, BufferSizeInBytes, Buffer));
 
@@ -44,6 +48,10 @@  FlashBlockIoReadBlocks (
     {
         Status = EFI_MEDIA_CHANGED;
     }
+    else if (IoAlign > 0 && (((UINTN) Buffer & (IoAlign - 1)) != 0))
+    {
+        Status = EFI_INVALID_PARAMETER;
+    }
     else
     {
         Status = FlashReadBlocks (Instance, Lba, BufferSizeInBytes, Buffer);
diff --git a/Chips/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c b/Chips/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c
index d118a90..c719d53 100644
--- a/Chips/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c
+++ b/Chips/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c
@@ -1125,7 +1125,7 @@  FlashReadBlocks (
     // We must have some bytes to read
     if (BufferSizeInBytes == 0)
     {
-        return EFI_BAD_BUFFER_SIZE;
+        return EFI_SUCCESS;
     }
 
     // The size of the buffer must be a multiple of the block size
diff --git a/Drivers/Block/ramdisk/ramdisk.c b/Drivers/Block/ramdisk/ramdisk.c
index bd2607d..14c2f0e 100644
--- a/Drivers/Block/ramdisk/ramdisk.c
+++ b/Drivers/Block/ramdisk/ramdisk.c
@@ -481,14 +481,18 @@  STATIC EFI_STATUS RamDiskReadBlocks(
 
   Media = This->Media;
 
-  if(BufferSize % Media->BlockSize != 0)
+  if (Media->MediaId != MediaId) {
+    return EFI_MEDIA_CHANGED;
+  }
+  if (BufferSize % Media->BlockSize != 0) {
     return EFI_BAD_BUFFER_SIZE;
-
-  if(LBA > Media->LastBlock)
-    return EFI_DEVICE_ERROR;
-
-  if(LBA + BufferSize / Media->BlockSize - 1 > Media->LastBlock)
-    return EFI_DEVICE_ERROR;
+  }
+  if (BufferSize == 0) {
+    return EFI_SUCCESS;
+  }
+  if (LBA + BufferSize / Media->BlockSize - 1 > Media->LastBlock) {
+    return EFI_INVALID_PARAMETER;
+  }
 
   RamDiskDev = RAM_DISK_FROM_THIS(This);
   RamDiskLBA = RamDiskDev->Start + MultU64x32(LBA,Media->BlockSize);