Received-SPF: pass (google.com: domain of linux-kernel+bounces-97519-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Content-Type: text/plain;
	charset=us-ascii
Precedence: bulk
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3774.400.31\))
Subject: Re: [RESEND PATCH v2] x86/apic: Use u32 instead of unsigned long
From: Thorsten Blum <thorsten.blum@toblux.com>
In-Reply-To: <656ade98-c8aa-42df-8926-d0513e105816@intel.com>
Date: Fri, 8 Mar 2024 20:50:30 +0100
Cc: "David.Laight@aculab.com" <David.Laight@ACULAB.COM>,
 Borislav Petkov <bp@alien8.de>,
 Dave Hansen <dave.hansen@linux.intel.com>,
 "H. Peter Anvin" <hpa@zytor.com>,
 linux-kernel@vger.kernel.org,
 mingo@redhat.com,
 peterz@infradead.org,
 tglx@linutronix.de,
 wei.liu@kernel.org,
 x86@kernel.org
Content-Transfer-Encoding: quoted-printable
Message-Id: <6548057B-015C-4A67-8231-0E63224BF74F@toblux.com>
References: <20240301203901.150465-3-thorsten.blum@toblux.com>
 <20240308124336.41654-2-thorsten.blum@toblux.com>
 <656ade98-c8aa-42df-8926-d0513e105816@intel.com>
To: Dave Hansen <dave.hansen@intel.com>

On Mar 8, 2024, at 17:12, Dave Hansen <dave.hansen@intel.com> wrote:
>=20
> 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.
>=20
> This seems simple enough, but the changelog and subject are really
> lacking in substantive information.
>=20
> 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.
>=20
> 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.
>=20
> 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=20=

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=