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
| ||
|
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