Re: [PATCH 1/4] [Patch-next] ACPI, APEI, ERST Fix the wrong checking of Serialization Header's length

[Jin Dongming <jin.dongming@np.css.fujitsu.com>]

(2010/08/17 10:33), Huang Ying wrote:
> On Tue, 2010-08-17 at 08:54 +0800, Jin Dongming wrote:
>> header_length in struct acpi_table_erst is not the length of struct
>> acpi_table_erst, but the length of Serialization Header.
>>
>> In erst_check_table(), header_length is used for checking the length
>> of struct acpi_table_erst. So I think it is wrong.
> 
> Why "think"? Do you have a machine with header_length set as you said?

I have such machine, and I confirmed that the header_length returned by firmware
is same as what I described in the above description.

And further more, according to the spec document of ACPI 4.0(page 515),
it is written like following.

  Field                      Byte length   Byte offset   Description
  Serialization Header Size  4             0x24          Length in bytes of
                                                         the serialization header.

> 
>> This patch fixed it and I confirmed it on x86_64 next-tree.
>>
>> Signed-off-by: Jin Dongming <jin.dongming@np.css.fujitsu.com>
>> ---
>>  drivers/acpi/apei/erst.c |   13 ++++++++-----
>>  include/acpi/actbl1.h    |   14 +++++++++++---
>>  2 files changed, 19 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
>> index 864dd46..28a5caf 100644
>> --- a/drivers/acpi/apei/erst.c
>> +++ b/drivers/acpi/apei/erst.c
>> @@ -358,7 +358,8 @@ static struct apei_exec_ins_type erst_ins_type[] = {
>>  static inline void erst_exec_ctx_init(struct apei_exec_context *ctx)
>>  {
>>  	apei_exec_ctx_init(ctx, erst_ins_type, ARRAY_SIZE(erst_ins_type),
>> -			   ERST_TAB_ENTRY(erst_tab), erst_tab->entries);
>> +			   ERST_TAB_ENTRY(erst_tab),
>> +			   erst_tab->seri_header.entries);
>>  }
>>  
>>  static int erst_get_erange(struct erst_erange *range)
>> @@ -749,12 +750,14 @@ __setup("erst_disable", setup_erst_disable);
>>  
>>  static int erst_check_table(struct acpi_table_erst *erst_tab)
>>  {
>> -	if (erst_tab->header_length != sizeof(struct acpi_table_erst))
>> +	if (erst_tab->common_header.length < sizeof(struct acpi_table_erst))
>>  		return -EINVAL;
>> -	if (erst_tab->header.length < sizeof(struct acpi_table_erst))
>> +
>> +	if (erst_tab->seri_header.length != sizeof(struct acpi_erst_header))
>>  		return -EINVAL;
>> -	if (erst_tab->entries !=
>> -	    (erst_tab->header.length - sizeof(struct acpi_table_erst)) /
>> +
>> +	if (erst_tab->seri_header.entries !=
>> +	    (erst_tab->common_header.length - sizeof(struct acpi_table_erst)) /
>>  	    sizeof(struct acpi_erst_entry))
>>  		return -EINVAL;
>>  
>> diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
>> index c637b75..821f8ac 100644
>> --- a/include/acpi/actbl1.h
>> +++ b/include/acpi/actbl1.h
>> @@ -283,13 +283,21 @@ enum acpi_einj_command_status {
>>   *
>>   ******************************************************************************/
>>  
>> -struct acpi_table_erst {
>> -	struct acpi_table_header header;	/* Common ACPI table header */
>> -	u32 header_length;
>> +/* ERST Serialization Header */
>> +
>> +struct acpi_erst_header {
>> +	u32 length;
>>  	u32 reserved;
>>  	u32 entries;
>>  };
>>  
>> +/* ERST Header */
>> +
>> +struct acpi_table_erst {
>> +	struct acpi_table_header common_header;	/* Common ACPI table header */
>> +	struct acpi_erst_header seri_header;	/* ERST Serialization Header */
>> +};
> 
> I don't think it is necessary to change the header definition, and
> seri_header is not a good name for me.
Why I added this structure here is that I don't want to do following checking.
For example,
	if (header_length != 0x0c) {
		...
	}

Though it is simple, it is really difficult for us to understand the real meaning
of the code.
> 
> Best Regards,
> Huang Ying
> 
> 
> 
>