On 19.06.2020 20:23, Grzegorz Uriasz wrote:
> On 19/06/2020 15:17, Jan Beulich wrote:
>> On 19.06.2020 06:28, Grzegorz Uriasz wrote:
>>> --- a/xen/arch/x86/time.c
>>> +++ b/xen/arch/x86/time.c
>>> @@ -457,16 +457,16 @@ 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 )
>>> return 0;
>>>
>>> - if ( pmtmr_width == 32 )
>>> - {
>>> - pts->counter_bits = 32;
>>> - mask = 0xffffffff;
>>> - }
>>> + if ( pmtmr_width != 24 && pmtmr_width != 32 )
>>> + return 0;
>>> +
>>> + pts->counter_bits = (int) pmtmr_width;
>>> + mask = (1ull << pmtmr_width) - 1;
>>>
>>> count = inl(pmtmr_ioport) & mask;
>>> start = rdtsc_ordered();
>>> @@ -486,7 +486,6 @@ static struct platform_timesource __initdata
>>> plt_pmtimer =
>>> .name = "ACPI PM Timer",
>>> .frequency = ACPI_PM_FREQUENCY,
>>> .read_counter = read_pmtimer_count,
>>> - .counter_bits = 24,
>>> .init = init_pmtimer
>>> };
>> I'm struggling a little to see why this code churn is needed / wanted.
>> The change I had suggested was quite a bit less intrusive. I'm not
>> entirely opposed though, but at the very least please drop the odd
>> (int) cast. If anything we want the struct field changed to unsigned
>> int (but unlikely in this very patch).
>>
>> If you want to stick to this larger change, then also please fold the
>> two if()s at the beginning of the function.
>
> I wanted to avoid hard coding the masks -> Linux has a nice macro for
> generating the masks but I haven't found a similar macro in xen.
> Additionally this version sets the counter width in a single place
> instead of two.
I guessed this was the goal, but I think such adjustments, if indeed
wanted, would better go into a separate patch. The bug fix here wants
backporting, while this extra cleanup probably doesn't.
Jan
