[PATCH v3] arm64: acpi/gtdt: validate CNTFRQ after having enabled the frame

[Mark Rutland]

Hi Ard,

Sorry it's taken so long to review this, and thanks for respinning this.

I'll get this queued for v4.15.

Thanks,
Mark.

On Fri, Sep 01, 2017 at 11:35:33AM +0100, Ard Biesheuvel wrote:
> The ACPI GTDT code validates the CNTFRQ field of each MMIO timer
> frame against the CNTFRQ system register of the current CPU, to
> ensure that they are equal, which is mandated by the architecture.
> 
> However, reading the CNTFRQ field of a frame is not possible until
> the RFRQ bit in the frame's CNTACRn register is set, and doing so
> before that willl produce the following error:
> 
>   arch_timer: [Firmware Bug]: CNTFRQ mismatch: frame @ 0x00000000e0be0000: (0x00000000), CPU: (0x0ee6b280)
>   arch_timer: Disabling MMIO timers due to CNTFRQ mismatch
>   arch_timer: Failed to initialize memory-mapped timer.
> 
> The reason is that the CNTFRQ field is RES0 if access is not enabled.
> 
> So move the validation of CNTFRQ into the loop that iterates over the
> timers to find the best frame, but defer it until after we have selected
> the best frame, which should also have enabled the RFRQ bit.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel at linaro.org>
> ---
> v3: - preserve existing logic as much as possible wrt verifying all frames of
>       all timers before selecting a suitable one
> 
>  drivers/clocksource/arm_arch_timer.c | 38 +++++++++++---------
>  1 file changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 72bbfccef113..30071cdea846 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -1264,10 +1264,6 @@ arch_timer_mem_find_best_frame(struct arch_timer_mem *timer_mem)
>  
>  	iounmap(cntctlbase);
>  
> -	if (!best_frame)
> -		pr_err("Unable to find a suitable frame in timer @ %pa\n",
> -			&timer_mem->cntctlbase);
> -
>  	return best_frame;
>  }
>  
> @@ -1368,6 +1364,8 @@ static int __init arch_timer_mem_of_init(struct device_node *np)
>  
>  	frame = arch_timer_mem_find_best_frame(timer_mem);
>  	if (!frame) {
> +		pr_err("Unable to find a suitable frame in timer @ %pa\n",
> +			&timer_mem->cntctlbase);
>  		ret = -EINVAL;
>  		goto out;
>  	}
> @@ -1416,7 +1414,7 @@ arch_timer_mem_verify_cntfrq(struct arch_timer_mem *timer_mem)
>  static int __init arch_timer_mem_acpi_init(int platform_timer_count)
>  {
>  	struct arch_timer_mem *timers, *timer;
> -	struct arch_timer_mem_frame *frame;
> +	struct arch_timer_mem_frame *frame, *best_frame = NULL;
>  	int timer_count, i, ret = 0;
>  
>  	timers = kcalloc(platform_timer_count, sizeof(*timers),
> @@ -1428,14 +1426,6 @@ static int __init arch_timer_mem_acpi_init(int platform_timer_count)
>  	if (ret || !timer_count)
>  		goto out;
>  
> -	for (i = 0; i < timer_count; i++) {
> -		ret = arch_timer_mem_verify_cntfrq(&timers[i]);
> -		if (ret) {
> -			pr_err("Disabling MMIO timers due to CNTFRQ mismatch\n");
> -			goto out;
> -		}
> -	}
> -
>  	/*
>  	 * While unlikely, it's theoretically possible that none of the frames
>  	 * in a timer expose the combination of feature we want.
> @@ -1444,12 +1434,26 @@ static int __init arch_timer_mem_acpi_init(int platform_timer_count)
>  		timer = &timers[i];
>  
>  		frame = arch_timer_mem_find_best_frame(timer);
> -		if (frame)
> -			break;
> +		if (!best_frame)
> +			best_frame = frame;
> +
> +		ret = arch_timer_mem_verify_cntfrq(timer);
> +		if (ret) {
> +			pr_err("Disabling MMIO timers due to CNTFRQ mismatch\n");
> +			goto out;
> +		}
> +
> +		if (!best_frame) /* implies !frame */
> +			/*
> +			 * Only complain about missing suitable frames if we
> +			 * haven't already found one in a previous iteration.
> +			 */
> +			pr_err("Unable to find a suitable frame in timer @ %pa\n",
> +				&timer->cntctlbase);
>  	}
>  
> -	if (frame)
> -		ret = arch_timer_mem_frame_register(frame);
> +	if (best_frame)
> +		ret = arch_timer_mem_frame_register(best_frame);
>  out:
>  	kfree(timers);
>  	return ret;
> -- 
> 2.11.0
>