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

[Ian Jackson <iwj@xxxxxxxxxxxxxx>]

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>

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 ?

I would have said

   +
   +    if ( hpet_address == 0 || !opt_hpet )
   +        return 0;

        if ( hpet_rate )
        if ( hpet_rate )
            return hpet_rate;

   -    if ( hpet_address == 0 )
   -        return 0;
   -

But Andy's version of expresses it the same way so fine, if that's the
way you like to do things, and hpet_opt is new in this patch so I
don't consider it a crisis if it doesn't work right.

Ian.