Re: [RESEND PATCH v2] x86/apic: Use u32 instead of unsigned long

[Thorsten Blum]

On Mar 8, 2024, at 17:12, Dave Hansen <dave.hansen@xxxxxxxxx> wrote:
> 
> On 3/8/24 04:43, Thorsten Blum wrote:
>> Improve types by using u32 instead of unsigned long. Fixes two
>> Coccinelle/coccicheck warnings reported by do_div.cocci.
> 
> This seems simple enough, but the changelog and subject are really
> lacking in substantive information.
> 
> The _patch_ literally does a few s/unsigned long/u32/ operations, and
> that's just repeated pretty much verbatim in the changelog and subject.
> So it just tells me two more times what I already know.
> 
> Without going and running do_div.cocci I can't tell whether the warnings
> are good checks or nonsense.  I also don't know _which_ do_div()s the
> warnings even refer to.
> 
> Could we get a _little_ more meat in here, please?

Yes, of course.

Coccinelle issues the following two warnings:

arch/x86/kernel/apic/apic.c:734:1-7: WARNING: do_div() does a 64-by-32
division, please consider using div64_long instead.

arch/x86/kernel/apic/apic.c:742:2-8: WARNING: do_div() does a 64-by-32
division, please consider using div64_long instead.

These occur because the divisor (deltapm) is of type long and the do_div() macro
casts the divisor to uint32_t, which could potentially lead to an incorrect
quotient.

Version 1 of my patch replaced both do_div() macro calls with div64_ul()
function calls, thereby removing the Coccinelle warnings. I used div64_ul()
instead of div64_long() because I also changed deltapm from long to unsigned
long, given that deltapm cannot be negative.

However, David Laight noted that div64_ul() is a 64-by-64 division and 
significantly slower (especially on 32-bit machines).

David's feedback led me to trace the source of deltapm, which is
acpi_pm_read_early() in include/linux/acpi_pmtmr.h. Since acpi_pm_read_early()
returns a u32 (which is additionally masked to 24 bits), there is no reason for
deltapm or any of the other intermediate variables (pm, lapic_cal_pm1, and
lapic_cal_pm2) to be of type long or unsigned long. They can all be u32, which
more accurately reflects their possible values and which removes both
Coccinelle warnings.

Maybe I'm missing something, but hopefully this clarifies my changes in v2.

Thanks,
Thorsten