| 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
| ||
|
Message-ID: <0b648a7d-98f7-dc54-99b0-4331379a65ba@linaro.org>
Date: Thu, 3 Dec 2020 11:09:48 +0100
From: Daniel Lezcano <daniel.lezcano@...aro.org>
To: Dinh Nguyen <dinguyen@...nel.org>, linux-kernel@...r.kernel.org
Cc: tglx@...utronix.de, p.zabel@...gutronix.de,
Jisheng.Zhang@...aptics.com, arnd@...db.de
Subject: Re: [PATCH] clocksource: dw_apb_timer_of: return EPROBE_DEFER if no
clock available
Hi Dinh,
On 19/11/2020 13:12, Dinh Nguyen wrote:
> commit ("b0fc70ce1f02 arm64: berlin: Select DW_APB_TIMER_OF") added the
> support for the dw_apb_timer into the arm64 defconfig. However, for some
> platforms like the Intel Stratix10 and Agilex, the clock manager doesn't
> get probed until after the timer driver is probed. Thus, the driver hits
> the panic "No clock nor clock-frequency property for %" because it cannot
> properly get the clock.
>
> This patch adds support for EPROBE_DEFER so the kernel can come back to
> finish probing this timer driver after the clock driver is probed.
>
> Signed-off-by: Dinh Nguyen <dinguyen@...nel.org>
A few comments below.
> ---
> drivers/clocksource/dw_apb_timer_of.c | 86 ++++++++++++++++-----------
> 1 file changed, 51 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/clocksource/dw_apb_timer_of.c b/drivers/clocksource/dw_apb_timer_of.c
> index ab3ddebe8344..a8ce980c5146 100644
> --- a/drivers/clocksource/dw_apb_timer_of.c
> +++ b/drivers/clocksource/dw_apb_timer_of.c
> @@ -14,7 +14,7 @@
> #include <linux/reset.h>
> #include <linux/sched_clock.h>
>
> -static void __init timer_get_base_and_rate(struct device_node *np,
> +static int __init timer_get_base_and_rate(struct device_node *np,
> void __iomem **base, u32 *rate)
> {
> struct clk *timer_clk;
> @@ -47,65 +47,77 @@ static void __init timer_get_base_and_rate(struct device_node *np,
> np);
>
> timer_clk = of_clk_get_by_name(np, "timer");
> - if (IS_ERR(timer_clk))
> - goto try_clock_freq;
> + if (IS_ERR(timer_clk)) {
> + if (PTR_ERR(timer_clk) != -EPROBE_DEFER) {
> + pr_err("Failed to get clock for %pOF\n", np);
> + goto try_clock_freq;
> + }
> + return PTR_ERR(timer_clk);
> + }
May be massage the changes by moving the of-rate check first
if (!of_property_read_u32(np, "clock-freq", rate) &&
!of_property_read_u32(np, "clock-frequency", rate))
return 0;
timer_clk = of_clk_get_by_name(np, "timer");
/*
* Whatever the result, we return
*/
ret = PTR_ERR(timer_clk);
if (ret)
return ret;
ret = clk_prepare_enable(timer_clk);
if (ret)
return ret;
*rate = clk_get_rate(timer_clk);
if (!(*rate))
return -EINVAL;
return 0;
>
> if (!clk_prepare_enable(timer_clk)) {
> *rate = clk_get_rate(timer_clk);
> - return;
> + return 0;
> }
>
> try_clock_freq:
> if (of_property_read_u32(np, "clock-freq", rate) &&
> of_property_read_u32(np, "clock-frequency", rate))
> panic("No clock nor clock-frequency property for %pOFn", np);
>
> + return 0;
> }
>
> -static void __init add_clockevent(struct device_node *event_timer)
> +static int __init add_clockevent(struct device_node *event_timer)
> {
> void __iomem *iobase;
> struct dw_apb_clock_event_device *ced;
> u32 irq, rate;
> + int ret = 0;
>
> irq = irq_of_parse_and_map(event_timer, 0);
> if (irq == 0)
> panic("No IRQ for clock event timer");
>
> - timer_get_base_and_rate(event_timer, &iobase, &rate);
> -
> - ced = dw_apb_clockevent_init(-1, event_timer->name, 300, iobase, irq,
> + ret = timer_get_base_and_rate(event_timer, &iobase, &rate);
> + if (ret == 0) {
> + ced = dw_apb_clockevent_init(-1, event_timer->name, 300, iobase, irq,
> rate);
> - if (!ced)
> - panic("Unable to initialise clockevent device");
> + if (!ced)
> + panic("Unable to initialise clockevent device");
ret = timer_get_base_and_rate(event_timer, &iobase, &rate);
if (ret)
return ret;
ced = dw_apb_clockevent_init(-1, event_timer->name, 300,
iobase, irq, rate);
if (!ced)
return -EINVAL;
dw_apb_clockevent_register(ced);
return 0;
> - dw_apb_clockevent_register(ced);
> + dw_apb_clockevent_register(ced);
> + }
> + return ret;
> }
>
> static void __iomem *sched_io_base;
> static u32 sched_rate;
>
> -static void __init add_clocksource(struct device_node *source_timer)
> +static int __init add_clocksource(struct device_node *source_timer)
> {
> void __iomem *iobase;
> struct dw_apb_clocksource *cs;
> u32 rate;
> -
> - timer_get_base_and_rate(source_timer, &iobase, &rate);
> -
> - cs = dw_apb_clocksource_init(300, source_timer->name, iobase, rate);
> - if (!cs)
> - panic("Unable to initialise clocksource device");
> -
> - dw_apb_clocksource_start(cs);
> - dw_apb_clocksource_register(cs);
> -
> - /*
> - * Fallback to use the clocksource as sched_clock if no separate
> - * timer is found. sched_io_base then points to the current_value
> - * register of the clocksource timer.
> - */
> - sched_io_base = iobase + 0x04;
> - sched_rate = rate;
> + int ret;
> +
> + ret = timer_get_base_and_rate(source_timer, &iobase, &rate);
> + if (ret == 0) {
> + cs = dw_apb_clocksource_init(300, source_timer->name, iobase, rate);
> + if (!cs)
> + panic("Unable to initialise clocksource device");
> +
> + dw_apb_clocksource_start(cs);
> + dw_apb_clocksource_register(cs);
> +
> + /*
> + * Fallback to use the clocksource as sched_clock if no separate
> + * timer is found. sched_io_base then points to the current_value
> + * register of the clocksource timer.
> + */
> + sched_io_base = iobase + 0x04;
> + sched_rate = rate;
> + }
> + return ret;
Same suggestion as above
> }
>
> static u64 notrace read_sched_clock(void)
> @@ -146,25 +158,29 @@ static struct delay_timer dw_apb_delay_timer = {
> static int num_called;
> static int __init dw_apb_timer_init(struct device_node *timer)
> {
> + int ret = 0;
> +
> switch (num_called) {
> case 1:
> pr_debug("%s: found clocksource timer\n", __func__);
> - add_clocksource(timer);
> - init_sched_clock();
> + ret = add_clocksource(timer);
> + if (ret == 0) {
> + init_sched_clock();
ret = add_clocksource(timer);
if (ret)
return ret;
init_sched_clock();
dw_apb_delay_timer.freq = sched_rate;
register_current_timer_delay(&dw_apb_delay_timer);
> #ifdef CONFIG_ARM
> - dw_apb_delay_timer.freq = sched_rate;
> - register_current_timer_delay(&dw_apb_delay_timer);
> + dw_apb_delay_timer.freq = sched_rate;
> + register_current_timer_delay(&dw_apb_delay_timer);
> #endif
> + }
> break;
> default:
> pr_debug("%s: found clockevent timer\n", __func__);
> - add_clockevent(timer);
> + ret = add_clockevent(timer);
ret = add_clockevent(timer);
if (ret)
return ret;
> break;
> }
>
> num_called++;
>
> - return 0;
> + return ret;
return 0;
> }
> TIMER_OF_DECLARE(pc3x2_timer, "picochip,pc3x2-timer", dw_apb_timer_init);
> TIMER_OF_DECLARE(apb_timer_osc, "snps,dw-apb-timer-osc", dw_apb_timer_init);
>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
Powered by blists - more mailing lists