On 31 August 2017 at 18:47, Mark Rutland [off-list ref] wrote:

Hi Ard,

Thanks for respinning this, and apologies for the delay.

On Mon, Aug 21, 2017 at 10:56:23AM +0100, Ard Biesheuvel wrote:

quoted

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index aae87c4c546e..c16fa694a47b 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c

@@ -1217,7 +1217,8 @@ arch_timer_mem_frame_get_cntfrq(struct arch_timer_mem_frame *frame)
 }

 static struct arch_timer_mem_frame * __init
-arch_timer_mem_find_best_frame(struct arch_timer_mem *timer_mem)
+arch_timer_mem_find_best_frame(struct arch_timer_mem *timer_mem,
+                            bool verify_freq)
 {
      struct arch_timer_mem_frame *frame, *best_frame = NULL;
      void __iomem *cntctlbase;

@@ -1259,6 +1260,11 @@ arch_timer_mem_find_best_frame(struct arch_timer_mem *timer_mem)
              if (~cntacr & (CNTACR_RWPT | CNTACR_RPCT))
                      continue;

+             if (verify_freq &&
+                 arch_timer_mem_frame_get_cntfrq(frame) != arch_timer_rate) {
+                     pr_err(FW_BUG "Ignoring MMIO timer frame with incorrect CNTFRQ\n");
+                     continue;
+             }

I don't think this is quite right, since we only check this after
skipping over frames (which are valid, but don't match our preferences
w.r.t. the timer and counter).

Can we move this before checking the CNTACR_RWPT and CNTACR_RPCT bits?

Yes.

I's still prefer that we give up (as we previously did), rather than
skipping timers with bad CNTFRQ values, but I guess that really depends
on whether we think we'll use more than one MMIO timer in future. Maybe
we'll want to use one per socket or something like that.

Not sure I follow, since we skip frames, not timers in this case.

quoted

@@ -1443,7 +1414,7 @@ static int __init arch_timer_mem_acpi_init(int platform_timer_count)
      for (i = i; i < timer_count; i++) {
              timer = &timers[i];

-             frame = arch_timer_mem_find_best_frame(timer);
+             frame = arch_timer_mem_find_best_frame(timer, true);
              if (frame)
                      break;

Previously we'd have checked all timers, then chosen the best one,
whereas now we bail out once we find a timer with a usable frame.

Is there any way we can make this check the remaining timers?

You mean as a diagnostic? We don't really care if timer frames we are
not interested in using have invalid CNTFRQ values, right?