On Fri, Mar 31, 2017 at 05:00:38PM +0800, Chenhui Sun wrote:
Hi Leif,
在 2017/3/21 22:48, Leif Lindholm 写道:
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).
Checksum is a word I think, not "Check" + "Sum", it is spelled as Checksum in Edk2 base code. Could you check it again please? :)
So, I actually agree with your argument. Which was why I went to look. And MdePkg/Include/Library/BaseLib.h uses "CheckSum" consistently.
And it sticks out in this file because it calls those functions, with the other form.
I do not see anything "Platform" about this. Since it is only a local function, name is not very important, "AcpiCheckSum" would be sufficient.
Ok
- 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.
It should calculate all the table data checksum, not only the header.
Yes. But the current code does AcpiPlatformChecksum ((UINT8*)Table, Table->Length);
Where it could do AcpiPlatformChecksum (Table);
and the function could do: UINT8 *Buffer; Buffer = (UINT8 *)Table; Buffer[ChecksumOffset] = CalculateCheckSum8 (Buffer, Table->Length);
Regards,
Leif
Chenhui, Thanks and Regards
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