| Message ID | 1490015485-53685-7-git-send-email-chenhui.sun@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | D02/D03 platforms bug fix | expand |
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 >
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 >
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? :) > 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. 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 >>
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 > >> >
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;