RE: [Xen-devel] [PATCH 6/9] Add cpu idle pwr mgmt to xen

["Wei, Gang" <gang.wei@xxxxxxxxx>]

On Monday, April 28, 2008 5:33 PM, Jan Beulich wrote:
>>>> "Wei, Gang" <gang.wei@xxxxxxxxx> 25.04.08 07:08 >>>
>> Port acpi bit register support from Linux.
>> 
>> Bit register read/write is required by deep C code.
>> Remove dependendy on acpi_sinfo.
>> 
>> Signed-off-by: Wei Gang <gang.wei@xxxxxxxxx>
>> 
>> +static void __init
>> +acpi_fadt_parse_reg(struct acpi_table_fadt *fadt)
>> +{
>> +    memcpy(&acpi_gbl_FADT, fadt, sizeof(acpi_gbl_FADT)); +
>> +    memcpy(&acpi_gbl_xpm1a_enable, &(fadt->xpm1a_event_block),
>> +            sizeof(acpi_gbl_xpm1a_enable));
>> +    memcpy(&acpi_gbl_xpm1b_enable, &(fadt->xpm1b_event_block),
>> +            sizeof(acpi_gbl_xpm1b_enable));
>> +
>> +    acpi_gbl_xpm1a_enable.address += 2;
>> +    acpi_gbl_xpm1b_enable.address += 2;
>> +}
> 
> This doesn't look right: For one, PM1b is optional, and hence if you
add
> a non-zero value to a zero address (which is commonly used as the
> presence indicator) consumers all of the sudden could consider the
block
> present. The other thing is that while on today's systems using a
literal
> 2 here may be correct, it isn't according to the ACPI spec - you'd
ought
> to add half the block width instead.

You are absolutely right for both things metioned above. Thanks for the
detailed review.

I would like to change code as below:

+       acpi_gbl_xpm1a_enable.address += acpi_gbl_FADT.pm1_event_length
/ 2.;
+          if ( acpi_gbl_xpm1b_enable.address )
+           acpi_gbl_xpm1b_enable.address +=
acpi_gbl_FADT.pm1_event_length / 2;

> 
> Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel