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?
On Mar 8, 2024, at 17:12, Dave Hansen <[email protected]> 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
Given that acpi_pm_read_early() returns a u32 (masked to 24 bits), several
variables that store its return value are improved by adjusting their data
types from unsigned long to u32. Specifically, change deltapm's type from
long to u32 because its value fits into 32 bits and it cannot be negative.
These data type improvements resolve the following two Coccinelle/
coccicheck warnings reported by do_div.cocci:
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.
Signed-off-by: Thorsten Blum <[email protected]>
---
Changes in v2:
- Revert changing do_div() to div64_ul() after feedback from David Laight
- Use u32 instead of unsigned long to fix Coccinelle warnings
- Update patch title and description
Changes in v3:
- Adjust patch summary and description after feedback from Dave Hansen
---
arch/x86/kernel/apic/apic.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 4667bc4b00ab..184e1843620d 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -664,7 +664,7 @@ void lapic_update_tsc_freq(void)
static __initdata int lapic_cal_loops = -1;
static __initdata long lapic_cal_t1, lapic_cal_t2;
static __initdata unsigned long long lapic_cal_tsc1, lapic_cal_tsc2;
-static __initdata unsigned long lapic_cal_pm1, lapic_cal_pm2;
+static __initdata u32 lapic_cal_pm1, lapic_cal_pm2;
static __initdata unsigned long lapic_cal_j1, lapic_cal_j2;
/*
@@ -674,7 +674,7 @@ static void __init lapic_cal_handler(struct clock_event_device *dev)
{
unsigned long long tsc = 0;
long tapic = apic_read(APIC_TMCCT);
- unsigned long pm = acpi_pm_read_early();
+ u32 pm = acpi_pm_read_early();
if (boot_cpu_has(X86_FEATURE_TSC))
tsc = rdtsc();
@@ -699,7 +699,7 @@ static void __init lapic_cal_handler(struct clock_event_device *dev)
}
static int __init
-calibrate_by_pmtimer(long deltapm, long *delta, long *deltatsc)
+calibrate_by_pmtimer(u32 deltapm, long *delta, long *deltatsc)
{
const long pm_100ms = PMTMR_TICKS_PER_SEC / 10;
const long pm_thresh = pm_100ms / 100;
@@ -710,7 +710,7 @@ calibrate_by_pmtimer(long deltapm, long *delta, long *deltatsc)
return -1;
#endif
- apic_printk(APIC_VERBOSE, "... PM-Timer delta = %ld\n", deltapm);
+ apic_printk(APIC_VERBOSE, "... PM-Timer delta = %u\n", deltapm);
/* Check, if the PM timer is available */
if (!deltapm)
--
2.44.0
Given that acpi_pm_read_early() returns a u32 (masked to 24 bits), several
variables that store its return value are improved by adjusting their data
types from unsigned long to u32. Specifically, change deltapm's type from
long to u32 because its value fits into 32 bits and it cannot be negative.
These data type improvements resolve the following two Coccinelle/
coccicheck warnings reported by do_div.cocci:
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.
Signed-off-by: Thorsten Blum <[email protected]>
---
Changes in v2:
- Revert changing do_div() to div64_ul() after feedback from David Laight
- Use u32 instead of unsigned long to fix Coccinelle warnings
- Update patch title and description
Changes in v3:
- Adjust patch summary and description after feedback from Dave Hansen
---
arch/x86/kernel/apic/apic.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 4667bc4b00ab..184e1843620d 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -664,7 +664,7 @@ void lapic_update_tsc_freq(void)
static __initdata int lapic_cal_loops = -1;
static __initdata long lapic_cal_t1, lapic_cal_t2;
static __initdata unsigned long long lapic_cal_tsc1, lapic_cal_tsc2;
-static __initdata unsigned long lapic_cal_pm1, lapic_cal_pm2;
+static __initdata u32 lapic_cal_pm1, lapic_cal_pm2;
static __initdata unsigned long lapic_cal_j1, lapic_cal_j2;
/*
@@ -674,7 +674,7 @@ static void __init lapic_cal_handler(struct clock_event_device *dev)
{
unsigned long long tsc = 0;
long tapic = apic_read(APIC_TMCCT);
- unsigned long pm = acpi_pm_read_early();
+ u32 pm = acpi_pm_read_early();
if (boot_cpu_has(X86_FEATURE_TSC))
tsc = rdtsc();
@@ -699,7 +699,7 @@ static void __init lapic_cal_handler(struct clock_event_device *dev)
}
static int __init
-calibrate_by_pmtimer(long deltapm, long *delta, long *deltatsc)
+calibrate_by_pmtimer(u32 deltapm, long *delta, long *deltatsc)
{
const long pm_100ms = PMTMR_TICKS_PER_SEC / 10;
const long pm_thresh = pm_100ms / 100;
@@ -710,7 +710,7 @@ calibrate_by_pmtimer(long deltapm, long *delta, long *deltatsc)
return -1;
#endif
- apic_printk(APIC_VERBOSE, "... PM-Timer delta = %ld\n", deltapm);
+ apic_printk(APIC_VERBOSE, "... PM-Timer delta = %u\n", deltapm);
/* Check, if the PM timer is available */
if (!deltapm)
--
2.44.0