[Linaro-uefi,Linaro-uefi,v1,06/21] Hisilicon: Fix ACPI/DSDT table checksum error

Message ID 1490015485-53685-7-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.
From: Chenhui Sun <sunchenhui@huawei.com>

Refresh checksum after changing DSDT table.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: wanglijun <wanglijun@huawei.com>
Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
Signed-off-by: Yi Li <phoenix.liyi@huawei.com>
---
 Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Comments

Graeme Gregory March 21, 2017, 11:35 a.m. | #1
On Mon, Mar 20, 2017 at 09:11:10PM +0800, Chenhui Sun wrote:
> From: Chenhui Sun <sunchenhui@huawei.com>

> 

> Refresh checksum after changing DSDT table.

> 

This probably indicates we miss a test in ERP QA as we didn't notice
this.

Reviewed-by: Graeme Gregory <graeme.gregory@linaro.org>


> Contributed-under: TianoCore Contribution Agreement 1.0

> Signed-off-by: wanglijun <wanglijun@huawei.com>

> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>

> Signed-off-by: Yi Li <phoenix.liyi@huawei.com>

> ---

>  Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c | 22 ++++++++++++++++++++++

>  1 file changed, 22 insertions(+)

>

> diff --git a/Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c b/Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c

> index 41f5692..98be4dc 100644

> --- a/Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c

> +++ b/Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c

> @@ -442,6 +442,27 @@ static EFI_STATUS ProcessDSDT(

>    return EFI_SUCCESS;

>  }

>

> +VOID

> +AcpiPlatformChecksum (

> +  IN UINT8      *Buffer,

> +  IN UINTN      Size

> +  )

> +{

> +  UINTN ChecksumOffset;

> +

> +  ChecksumOffset = OFFSET_OF (EFI_ACPI_DESCRIPTION_HEADER, Checksum);

> +

> +  //

> +  // set checksum to 0 first

> +  //

> +  Buffer[ChecksumOffset] = 0;

> +

> +  //

> +  // Update checksum value

> +  //

> +  Buffer[ChecksumOffset] = CalculateCheckSum8(Buffer, Size);

> +}

> +

>  EFI_STATUS EthMacInit(void)

>  {

>    EFI_STATUS              Status;

> @@ -478,6 +499,7 @@ EFI_STATUS EthMacInit(void)

>      ProcessDSDT(AcpiTableProtocol, TableHandle);

>  

>      AcpiTableProtocol->Close(TableHandle);

> +    AcpiPlatformChecksum ((UINT8*)Table, Table->Length);

>    }

>  

>    return EFI_SUCCESS;

> -- 

> 1.9.1

>
Leif Lindholm March 21, 2017, 2:48 p.m. | #2
On Mon, Mar 20, 2017 at 09:11:10PM +0800, Chenhui Sun wrote:
> From: Chenhui Sun <sunchenhui@huawei.com>
> 
> Refresh checksum after changing DSDT table.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: wanglijun <wanglijun@huawei.com>
> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> Signed-off-by: Yi Li <phoenix.liyi@huawei.com>
> ---
>  Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c b/Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c
> index 41f5692..98be4dc 100644
> --- a/Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c
> +++ b/Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c
> @@ -442,6 +442,27 @@ static EFI_STATUS ProcessDSDT(
>    return EFI_SUCCESS;
>  }
>  

STATIC

> +VOID
> +AcpiPlatformChecksum (

EDK2 in general seems to camel-case also the final S in CheckSum, so
for consistency, please to the same (and also in variables below).

I do not see anything "Platform" about this. Since it is only a local
function, name is not very important, "AcpiCheckSum" would be
sufficient.

> +  IN UINT8      *Buffer,

It would be nicer to accept an EFI_ACPI_SDT_HEADER *, so that the
caller did not always need to cast when calling. And then add a local
UINT8 * to operate on. We would also then not need to pass the Size
separately.

Also, this is IN OUT. The buffer is being modified.

> +  IN UINTN      Size
> +  )
> +{
> +  UINTN ChecksumOffset;
> +
> +  ChecksumOffset = OFFSET_OF (EFI_ACPI_DESCRIPTION_HEADER, Checksum);
> +
> +  //
> +  // set checksum to 0 first
> +  //
> +  Buffer[ChecksumOffset] = 0;

Could this not be skipped if ...

> +
> +  //
> +  // Update checksum value
> +  //
> +  Buffer[ChecksumOffset] = CalculateCheckSum8(Buffer, Size);

... we passed in "Size - 1" here?

> +}
> +
>  EFI_STATUS EthMacInit(void)
>  {
>    EFI_STATUS              Status;
> @@ -478,6 +499,7 @@ EFI_STATUS EthMacInit(void)
>      ProcessDSDT(AcpiTableProtocol, TableHandle);
>  
>      AcpiTableProtocol->Close(TableHandle);
> +    AcpiPlatformChecksum ((UINT8*)Table, Table->Length);
>    }
>  
>    return EFI_SUCCESS;
> -- 
> 1.9.1
>

Patch hide | download patch | download mbox

diff --git a/Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c b/Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c
index 41f5692..98be4dc 100644
--- a/Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c
+++ b/Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c
@@ -442,6 +442,27 @@  static EFI_STATUS ProcessDSDT(
   return EFI_SUCCESS;
 }
 
+VOID
+AcpiPlatformChecksum (
+  IN UINT8      *Buffer,
+  IN UINTN      Size
+  )
+{
+  UINTN ChecksumOffset;
+
+  ChecksumOffset = OFFSET_OF (EFI_ACPI_DESCRIPTION_HEADER, Checksum);
+
+  //
+  // set checksum to 0 first
+  //
+  Buffer[ChecksumOffset] = 0;
+
+  //
+  // Update checksum value
+  //
+  Buffer[ChecksumOffset] = CalculateCheckSum8(Buffer, Size);
+}
+
 EFI_STATUS EthMacInit(void)
 {
   EFI_STATUS              Status;
@@ -478,6 +499,7 @@  EFI_STATUS EthMacInit(void)
     ProcessDSDT(AcpiTableProtocol, TableHandle);
 
     AcpiTableProtocol->Close(TableHandle);
+    AcpiPlatformChecksum ((UINT8*)Table, Table->Length);
   }
 
   return EFI_SUCCESS;