Re: [PATCH][4.15] x86/HPET: don't enable legacy replacement mode unconditionally

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.03.2021 17:43, Ian Jackson wrote:
> Jan Beulich writes ("[PATCH][4.15] x86/HPET: don't enable legacy replacement 
> mode unconditionally"):
>> Commit e1de4c196a2e ("x86/timer: Fix boot on Intel systems using ITSSPRC
>> static PIT clock gating") was reported to cause boot failures on certain
>> AMD Ryzen systems. Until we can figure out what the actual issue there
>> is, skip this new part of HPET setup by default. Introduce a "hpet"
>> command line option to allow enabling this on hardware where it's really
>> needed for Xen to boot successfully (i.e. where the PIT doesn't drive
>> the timer interrupt).
>>
>> Since it makes little sense to introduce just "hpet=legacy-replacement",
>> also allow for a boolean argument as well as "broadcast" to replace the
>> separate "hpetbroadcast" option.
> 
> Reviewed-by: Ian Jackson <iwj@xxxxxxxxxxxxxx>

Thanks, but with Andrew's pending objection I don't feel like
committing it.

> I have to say that this
> 
>    -    if ( hpet_rate )
>    +    if ( hpet_rate || !hpet_address || !opt_hpet )
>             return hpet_rate;
> 
>    -    if ( hpet_address == 0 )
>    -        return 0;
>    -
> 
> is to my mind quite an obscure coding style.
> 
> Do we really want to return a nozero hpet_rate even if !opt_hpet ?

We won't: hpet_rate will be set to non-zero only further down in
the function.

Jan