Ping: [PATCH v3 1/1] x86/acpi: Use FADT flags to determine the PMTMR width

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.06.2020 10:49, Jan Beulich wrote:
> On 20.06.2020 00:00, Grzegorz Uriasz wrote:
>> @@ -490,8 +497,12 @@ static int __init acpi_parse_fadt(struct 
>> acpi_table_header *table)
>>       */
>>      if (!pmtmr_ioport) {
>>              pmtmr_ioport = fadt->pm_timer_block;
>> -            pmtmr_width = fadt->pm_timer_length == 4 ? 24 : 0;
>> +            pmtmr_width = fadt->pm_timer_length == 4 ? 32 : 0;
>>      }
>> +    if (pmtmr_width < 32 && fadt->flags & ACPI_FADT_32BIT_TIMER)
>> +        printk(KERN_WARNING PREFIX "PM-Timer is too short\n");
> 
> Indentation is screwed up here.
> 
>> --- a/xen/arch/x86/time.c
>> +++ b/xen/arch/x86/time.c
>> @@ -457,16 +457,13 @@ static u64 read_pmtimer_count(void)
>>  static s64 __init init_pmtimer(struct platform_timesource *pts)
>>  {
>>      u64 start;
>> -    u32 count, target, mask = 0xffffff;
>> +    u32 count, target, mask;
>>  
>> -    if ( !pmtmr_ioport || !pmtmr_width )
>> +    if ( !pmtmr_ioport || (pmtmr_width != 24 && pmtmr_width != 32) )
>>          return 0;
>>  
>> -    if ( pmtmr_width == 32 )
>> -    {
>> -        pts->counter_bits = 32;
>> -        mask = 0xffffffff;
>> -    }
>> +    pts->counter_bits = pmtmr_width;
>> +    mask = (1ull << pmtmr_width) - 1;
> 
> "mask" being of "u32" type, the use of 1ull is certainly a little odd
> here, and one of the reasons why I think this extra "cleanup" would
> better go in a separate patch.
> 
> Seeing that besides cosmetic aspects the patch looks okay now, I'd be
> willing to leave the bigger adjustment mostly as is, albeit I'd
> prefer the 1ull to go away, by instead switching to e.g.
> 
>     mask = 0xffffffff >> (32 - pmtmr_width);
> 
> With the adjustments (which I'd be happy to make while committing,
> provided you agree)
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Also Cc-ing Paul for a possible release ack (considering this is a
> backporting candidate).

Paul?

Thanks, Jan