linux-kernel - Re: [PATCH v14 7/9] clocksource/drivers/arm_arch_timer: Refactor the timer init code to prepare for GTDT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20161021113213.GD16630@leverpostej>
Date:   Fri, 21 Oct 2016 12:32:13 +0100
From:   Mark Rutland <mark.rutland@....com>
To:     fu.wei@...aro.org
Cc:     rjw@...ysocki.net, lenb@...nel.org, daniel.lezcano@...aro.org,
        tglx@...utronix.de, marc.zyngier@....com,
        lorenzo.pieralisi@....com, sudeep.holla@....com,
        hanjun.guo@...aro.org, linux-arm-kernel@...ts.infradead.org,
        linaro-acpi@...ts.linaro.org, linux-kernel@...r.kernel.org,
        linux-acpi@...r.kernel.org, rruigrok@...eaurora.org,
        harba@...eaurora.org, cov@...eaurora.org, timur@...eaurora.org,
        graeme.gregory@...aro.org, al.stone@...aro.org, jcm@...hat.com,
        wei@...hat.com, arnd@...db.de, catalin.marinas@....com,
        will.deacon@....com, Suravee.Suthikulpanit@....com,
        leo.duran@....com, wim@...ana.be, linux@...ck-us.net,
        linux-watchdog@...r.kernel.org, tn@...ihalf.com,
        christoffer.dall@...aro.org, julien.grall@....com
Subject: Re: [PATCH v14 7/9] clocksource/drivers/arm_arch_timer: Refactor the
 timer init code to prepare for GTDT

On Thu, Sep 29, 2016 at 02:17:15AM +0800, fu.wei@...aro.org wrote:
> From: Fu Wei <fu.wei@...aro.org>
> 
> The patch refactor original memory-mapped timer init code:
> (1) extract some subfunction for reusing some common code
>     a. get_cnttidr
>     b. is_best_frame
> (2) move base address and irq code for arch_timer_mem to
> arch_timer_mem_register
> 
> Signed-off-by: Fu Wei <fu.wei@...aro.org>
> ---
>  drivers/clocksource/arm_arch_timer.c | 159 +++++++++++++++++++++--------------
>  1 file changed, 96 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index c7b0040..e78095f 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -57,6 +57,7 @@
>  static unsigned arch_timers_present __initdata;
>  
>  static void __iomem *arch_counter_base;
> +static void __iomem *cntctlbase __initdata;
>  
>  struct arch_timer {
>  	void __iomem *base;
> @@ -656,15 +657,49 @@ out:
>  	return err;
>  }
>  
> -static int __init arch_timer_mem_register(void __iomem *base, unsigned int irq)
> +static int __init arch_timer_mem_register(struct device_node *np, void *frame)
>  {
> -	int ret;
> -	irq_handler_t func;
> +	struct device_node *frame_node = NULL;
>  	struct arch_timer *t;
> +	void __iomem *base;
> +	irq_handler_t func;
> +	unsigned int irq;
> +	int ret;
> +
> +	if (!frame)
> +		return -EINVAL;

Why would we call this without a frame?

> +
> +	if (np) {

... or without a node?

> +		frame_node = (struct device_node *)frame;
> +		base = of_iomap(frame_node, 0);
> +		arch_timer_detect_rate(base, np);

... BANG! (we check base too late, below).

Please as Marc requested several versions ago: split the FW parsing
(ACPI and DT) so that happens first, *then* once we have the data in a
common format, use that to drive poking the HW, requesting IRQs, etc,
completely independent of the source.

In patches, do this by:

(1) adding the data structures
(2) splitting the existing DT probing to use them
(3) Adding ACPI functionality atop

> -static int __init arch_timer_mem_init(struct device_node *np)
> +static int __init get_cnttidr(struct device_node *np, u32 *cnttidr)
>  {
> -	struct device_node *frame, *best_frame = NULL;
> -	void __iomem *cntctlbase, *base;
> -	unsigned int irq, ret = -EINVAL;
> -	u32 cnttidr;
> +	if (!cnttidr)
> +		return -EINVAL;
> +
> +	if (np)
> +		cntctlbase = of_iomap(np, 0);
> +	else
> +		return -EINVAL;

We want to check this for ACPI too, no?

Thanks,
Mark.