On 14/08/2025 12:14, Marc Zyngier wrote:

On Thu, 14 Aug 2025 11:49:26 +0100,
Marc Zyngier [off-list ref] wrote:

quoted

On Thu, 14 Aug 2025 11:13:47 +0100,
Steven Price [off-list ref] wrote:

quoted

On 07/08/2025 17:02, Marc Zyngier wrote:

quoted

Add a new driver for the MMIO side of the ARM architected timer.
Most of it has been lifted from the existing arch timer code,
massaged, and finally rewritten.

It supports both DT and ACPI as firmware descriptions.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 MAINTAINERS                               |   1 +
 drivers/clocksource/arm_arch_timer_mmio.c | 420 ++++++++++++++++++++++
 2 files changed, 421 insertions(+)
 create mode 100644 drivers/clocksource/arm_arch_timer_mmio.c

[...]

quoted

+static void arch_timer_mmio_setup(struct arch_timer *at, int irq)
+{
+	at->evt = (struct clock_event_device) {
+		.features		   = (CLOCK_EVT_FEAT_ONESHOT |
+					      CLOCK_EVT_FEAT_DYNIRQ),
+		.name			   = "arch_mem_timer",
+		.rating			   = 400,
+		.cpumask		   = cpu_possible_mask,
+		.irq 			   = irq,
+		.set_next_event		   = arch_timer_mmio_set_next_event,
+		.set_state_oneshot_stopped = arch_timer_mmio_shutdown,
+		.set_state_shutdown	   = arch_timer_mmio_shutdown,
+	};
+
+	at->evt.set_state_shutdown(&at->evt);
+
+	clockevents_config_and_register(&at->evt, at->rate, 0xf, CLOCKSOURCE_MASK(56));

This doesn't work on 32 bit - clockevents_config_and_register()'s final
argument is an unsigned long, and a 56 bit mask doesn't fit. This
triggers a compiler warning:

Already reported, see 20250814111657.7debc9f1@canb.auug.org.au.

quoted

Possible this should really be min(CLOCKSOURCE_MASK(56), ULONG_MAX)? But
I'm not familiar enough with this code. Most likely it's dead code on a
32 bit platform.

No, this definitely exists on 32bit crap, since it has been part of
the architecture from the ARMv7+VE days.

I think this is more of an impedance mismatch between the
CLOCKSOURCE_MASK() helper and the clockevents_config_and_register(),
and a (unsigned long) cast would do the trick.

Of course not. That would just result in a big fat zero.

No - CLOCKSOURCE_MASK() turns into GENMASK_ULL so a simple truncation to
unsigned long would be ULONG_MAX (all 1s). So an unsigned long cast
should be fine. My suggestion of MIN(, ULONG_MAX) was just to make that
more clear.

quoted

But it also means that the per-cpu timer also gets truncated the same
way, and that has interesting impacts on how often the timer is
reprogrammed.

That question still stand, and I wonder whether we have ugly bugs
lurking on 32bit platforms because of that... I'll try and have a
look.

I don't know whether there are other bugs due to the capping to
ULONG_MAX, but I don't think there's an (additional) bug here, it's
"just" a ugly warning.

Thanks,
Steve