From 5d9814df0aec56a638bbf20795abb4cfaf3cd331 Mon Sep 17 00:00:00 2001 From: Dinh Nguyen Date: Sat, 5 Dec 2020 04:52:23 -0600 Subject: [PATCH 1/4] clocksource/drivers/dw_apb_timer_of: Add error handling if no clock available 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 loaded until after the timer driver get loaded. Thus, the driver hits the panic "No clock nor clock-frequency property for" because it cannot properly get the clock. This patch adds the error handling needed for the timer driver so that the kernel can continue booting instead of just hitting the panic. Signed-off-by: Dinh Nguyen Signed-off-by: Daniel Lezcano Link: https://lore.kernel.org/r/20201205105223.208604-1-dinguyen@kernel.org --- drivers/clocksource/dw_apb_timer_of.c | 57 ++++++++++++++++++--------- 1 file changed, 39 insertions(+), 18 deletions(-) diff --git a/drivers/clocksource/dw_apb_timer_of.c b/drivers/clocksource/dw_apb_timer_of.c index ab3ddebe8344..42e7e43b8fcd 100644 --- a/drivers/clocksource/dw_apb_timer_of.c +++ b/drivers/clocksource/dw_apb_timer_of.c @@ -14,12 +14,13 @@ #include #include -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; struct clk *pclk; struct reset_control *rstc; + int ret; *base = of_iomap(np, 0); @@ -46,55 +47,67 @@ static void __init timer_get_base_and_rate(struct device_node *np, pr_warn("pclk for %pOFn is present, but could not be activated\n", np); + 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"); if (IS_ERR(timer_clk)) - goto try_clock_freq; + return PTR_ERR(timer_clk); - if (!clk_prepare_enable(timer_clk)) { - *rate = clk_get_rate(timer_clk); - return; - } + ret = clk_prepare_enable(timer_clk); + if (ret) + return ret; -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); + *rate = clk_get_rate(timer_clk); + if (!(*rate)) + return -EINVAL; + + 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); + 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) - panic("Unable to initialise clockevent device"); + return -EINVAL; dw_apb_clockevent_register(ced); + + return 0; } 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; + int ret; - timer_get_base_and_rate(source_timer, &iobase, &rate); + ret = timer_get_base_and_rate(source_timer, &iobase, &rate); + if (ret) + return ret; cs = dw_apb_clocksource_init(300, source_timer->name, iobase, rate); if (!cs) - panic("Unable to initialise clocksource device"); + return -EINVAL; dw_apb_clocksource_start(cs); dw_apb_clocksource_register(cs); @@ -106,6 +119,8 @@ static void __init add_clocksource(struct device_node *source_timer) */ sched_io_base = iobase + 0x04; sched_rate = rate; + + return 0; } static u64 notrace read_sched_clock(void) @@ -146,10 +161,14 @@ 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); + ret = add_clocksource(timer); + if (ret) + return ret; init_sched_clock(); #ifdef CONFIG_ARM dw_apb_delay_timer.freq = sched_rate; @@ -158,7 +177,9 @@ static int __init dw_apb_timer_init(struct device_node *timer) break; default: pr_debug("%s: found clockevent timer\n", __func__); - add_clockevent(timer); + ret = add_clockevent(timer); + if (ret) + return ret; break; } From d8cc3905b8073c7cfbff94af889fa8dc71f21dd5 Mon Sep 17 00:00:00 2001 From: Keqian Zhu Date: Fri, 4 Dec 2020 15:31:25 +0800 Subject: [PATCH 2/4] clocksource/drivers/arm_arch_timer: Use stable count reader in erratum sne In commit 0ea415390cd3 ("clocksource/arm_arch_timer: Use arch_timer_read_counter to access stable counters"), we separate stable and normal count reader to omit unnecessary overhead on systems that have no timer erratum. However, in erratum_set_next_event_tval_generic(), count reader becomes normal reader. This converts it to stable reader. Fixes: 0ea415390cd3 ("clocksource/arm_arch_timer: Use arch_timer_read_counter to access stable counters") Acked-by: Marc Zyngier Signed-off-by: Keqian Zhu Signed-off-by: Daniel Lezcano Link: https://lore.kernel.org/r/20201204073126.6920-2-zhukeqian1@huawei.com --- drivers/clocksource/arm_arch_timer.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c index 6c3e84180146..777d38cb39b0 100644 --- a/drivers/clocksource/arm_arch_timer.c +++ b/drivers/clocksource/arm_arch_timer.c @@ -396,10 +396,10 @@ static void erratum_set_next_event_tval_generic(const int access, unsigned long ctrl &= ~ARCH_TIMER_CTRL_IT_MASK; if (access == ARCH_TIMER_PHYS_ACCESS) { - cval = evt + arch_counter_get_cntpct(); + cval = evt + arch_counter_get_cntpct_stable(); write_sysreg(cval, cntp_cval_el0); } else { - cval = evt + arch_counter_get_cntvct(); + cval = evt + arch_counter_get_cntvct_stable(); write_sysreg(cval, cntv_cval_el0); } From 8b7770b877d187bfdae1eaf587bd2b792479a31c Mon Sep 17 00:00:00 2001 From: Keqian Zhu Date: Fri, 4 Dec 2020 15:31:26 +0800 Subject: [PATCH 3/4] clocksource/drivers/arm_arch_timer: Correct fault programming of CNTKCTL_EL1.EVNTI ARM virtual counter supports event stream, it can only trigger an event when the trigger bit (the value of CNTKCTL_EL1.EVNTI) of CNTVCT_EL0 changes, so the actual period of event stream is 2^(cntkctl_evnti + 1). For example, when the trigger bit is 0, then virtual counter trigger an event for every two cycles. While we're at it, rework the way we compute the trigger bit position by making it more obvious that when bits [n:n-1] are both set (with n being the most significant bit), we pick bit (n + 1). Fixes: 037f637767a8 ("drivers: clocksource: add support for ARM architected timer event stream") Suggested-by: Marc Zyngier Signed-off-by: Keqian Zhu Acked-by: Marc Zyngier Signed-off-by: Daniel Lezcano Link: https://lore.kernel.org/r/20201204073126.6920-3-zhukeqian1@huawei.com --- drivers/clocksource/arm_arch_timer.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c index 777d38cb39b0..d0177824c518 100644 --- a/drivers/clocksource/arm_arch_timer.c +++ b/drivers/clocksource/arm_arch_timer.c @@ -822,15 +822,24 @@ static void arch_timer_evtstrm_enable(int divider) static void arch_timer_configure_evtstream(void) { - int evt_stream_div, pos; + int evt_stream_div, lsb; + + /* + * As the event stream can at most be generated at half the frequency + * of the counter, use half the frequency when computing the divider. + */ + evt_stream_div = arch_timer_rate / ARCH_TIMER_EVT_STREAM_FREQ / 2; + + /* + * Find the closest power of two to the divisor. If the adjacent bit + * of lsb (last set bit, starts from 0) is set, then we use (lsb + 1). + */ + lsb = fls(evt_stream_div) - 1; + if (lsb > 0 && (evt_stream_div & BIT(lsb - 1))) + lsb++; - /* Find the closest power of two to the divisor */ - evt_stream_div = arch_timer_rate / ARCH_TIMER_EVT_STREAM_FREQ; - pos = fls(evt_stream_div); - if (pos > 1 && !(evt_stream_div & (1 << (pos - 2)))) - pos--; /* enable event stream */ - arch_timer_evtstrm_enable(min(pos, 15)); + arch_timer_evtstrm_enable(max(0, min(lsb, 15))); } static void arch_counter_set_user_access(void) From 8ae954caf49ac403c177d117fb8e05cbc866aa3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niklas=20S=C3=B6derlund?= Date: Sat, 5 Dec 2020 03:19:20 +0100 Subject: [PATCH 4/4] clocksource/drivers/sh_cmt: Fix potential deadlock when calling runtime PM MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ch->lock is used to protect the whole enable() and read() of sh_cmt's implementation of struct clocksource. The enable() implementation calls pm_runtime_get_sync() which may result in the clock source to be read() triggering a cyclic lockdep warning for the ch->lock. The sh_cmt driver implement its own balancing of calls to sh_cmt_{enable,disable}() with flags in sh_cmt_{start,stop}(). It does this to deal with that start and stop are shared between the clock source and clock event providers. While this could be improved on verifying corner cases based on any substantial rework on all devices this driver supports might prove hard. As a first step separate the PM handling for clock event and clock source. Always put/get the device when enabling/disabling the clock source but keep the clock event logic unchanged. This allows the sh_cmt implementation of struct clocksource to call PM without holding the ch->lock and avoiding the deadlock. Triggering and log of the deadlock warning, # echo e60f0000.timer > /sys/devices/system/clocksource/clocksource0/current_clocksource [ 46.948370] ====================================================== [ 46.954730] WARNING: possible circular locking dependency detected [ 46.961094] 5.10.0-rc6-arm64-renesas-00001-g0e5fd7414e8b #36 Not tainted [ 46.967985] ------------------------------------------------------ [ 46.974342] migration/0/11 is trying to acquire lock: [ 46.979543] ffff0000403ed220 (&dev->power.lock){-...}-{2:2}, at: __pm_runtime_resume+0x40/0x74 [ 46.988445] [ 46.988445] but task is already holding lock: [ 46.994441] ffff000040ad0298 (&ch->lock){....}-{2:2}, at: sh_cmt_start+0x28/0x210 [ 47.002173] [ 47.002173] which lock already depends on the new lock. [ 47.002173] [ 47.010573] [ 47.010573] the existing dependency chain (in reverse order) is: [ 47.018262] [ 47.018262] -> #3 (&ch->lock){....}-{2:2}: [ 47.024033] lock_acquire.part.0+0x120/0x330 [ 47.028970] lock_acquire+0x64/0x80 [ 47.033105] _raw_spin_lock_irqsave+0x7c/0xc4 [ 47.038130] sh_cmt_start+0x28/0x210 [ 47.042352] sh_cmt_clocksource_enable+0x28/0x50 [ 47.047644] change_clocksource+0x9c/0x160 [ 47.052402] multi_cpu_stop+0xa4/0x190 [ 47.056799] cpu_stopper_thread+0x90/0x154 [ 47.061557] smpboot_thread_fn+0x244/0x270 [ 47.066310] kthread+0x154/0x160 [ 47.070175] ret_from_fork+0x10/0x20 [ 47.074390] [ 47.074390] -> #2 (tk_core.seq.seqcount){----}-{0:0}: [ 47.081136] lock_acquire.part.0+0x120/0x330 [ 47.086070] lock_acquire+0x64/0x80 [ 47.090203] seqcount_lockdep_reader_access.constprop.0+0x74/0x100 [ 47.097096] ktime_get+0x28/0xa0 [ 47.100960] hrtimer_start_range_ns+0x210/0x2dc [ 47.106164] generic_sched_clock_init+0x70/0x88 [ 47.111364] sched_clock_init+0x40/0x64 [ 47.115853] start_kernel+0x494/0x524 [ 47.120156] [ 47.120156] -> #1 (hrtimer_bases.lock){-.-.}-{2:2}: [ 47.126721] lock_acquire.part.0+0x120/0x330 [ 47.136042] lock_acquire+0x64/0x80 [ 47.144461] _raw_spin_lock_irqsave+0x7c/0xc4 [ 47.153721] hrtimer_start_range_ns+0x68/0x2dc [ 47.163054] rpm_suspend+0x308/0x5dc [ 47.171473] rpm_idle+0xc4/0x2a4 [ 47.179550] pm_runtime_work+0x98/0xc0 [ 47.188209] process_one_work+0x294/0x6f0 [ 47.197142] worker_thread+0x70/0x45c [ 47.205661] kthread+0x154/0x160 [ 47.213673] ret_from_fork+0x10/0x20 [ 47.221957] [ 47.221957] -> #0 (&dev->power.lock){-...}-{2:2}: [ 47.236292] check_noncircular+0x128/0x140 [ 47.244907] __lock_acquire+0x13b0/0x204c [ 47.253332] lock_acquire.part.0+0x120/0x330 [ 47.262033] lock_acquire+0x64/0x80 [ 47.269826] _raw_spin_lock_irqsave+0x7c/0xc4 [ 47.278430] __pm_runtime_resume+0x40/0x74 [ 47.286758] sh_cmt_start+0x84/0x210 [ 47.294537] sh_cmt_clocksource_enable+0x28/0x50 [ 47.303449] change_clocksource+0x9c/0x160 [ 47.311783] multi_cpu_stop+0xa4/0x190 [ 47.319720] cpu_stopper_thread+0x90/0x154 [ 47.328022] smpboot_thread_fn+0x244/0x270 [ 47.336298] kthread+0x154/0x160 [ 47.343708] ret_from_fork+0x10/0x20 [ 47.351445] [ 47.351445] other info that might help us debug this: [ 47.351445] [ 47.370225] Chain exists of: [ 47.370225] &dev->power.lock --> tk_core.seq.seqcount --> &ch->lock [ 47.370225] [ 47.392003] Possible unsafe locking scenario: [ 47.392003] [ 47.405314] CPU0 CPU1 [ 47.413569] ---- ---- [ 47.421768] lock(&ch->lock); [ 47.428425] lock(tk_core.seq.seqcount); [ 47.438701] lock(&ch->lock); [ 47.447930] lock(&dev->power.lock); [ 47.455172] [ 47.455172] *** DEADLOCK *** [ 47.455172] [ 47.471433] 3 locks held by migration/0/11: [ 47.479099] #0: ffff8000113c9278 (timekeeper_lock){-.-.}-{2:2}, at: change_clocksource+0x2c/0x160 [ 47.491834] #1: ffff8000113c8f88 (tk_core.seq.seqcount){----}-{0:0}, at: multi_cpu_stop+0xa4/0x190 [ 47.504727] #2: ffff000040ad0298 (&ch->lock){....}-{2:2}, at: sh_cmt_start+0x28/0x210 [ 47.516541] [ 47.516541] stack backtrace: [ 47.528480] CPU: 0 PID: 11 Comm: migration/0 Not tainted 5.10.0-rc6-arm64-renesas-00001-g0e5fd7414e8b #36 [ 47.542147] Hardware name: Renesas Salvator-X 2nd version board based on r8a77965 (DT) [ 47.554241] Call trace: [ 47.560832] dump_backtrace+0x0/0x190 [ 47.568670] show_stack+0x14/0x30 [ 47.576144] dump_stack+0xe8/0x130 [ 47.583670] print_circular_bug+0x1f0/0x200 [ 47.592015] check_noncircular+0x128/0x140 [ 47.600289] __lock_acquire+0x13b0/0x204c [ 47.608486] lock_acquire.part.0+0x120/0x330 [ 47.616953] lock_acquire+0x64/0x80 [ 47.624582] _raw_spin_lock_irqsave+0x7c/0xc4 [ 47.633114] __pm_runtime_resume+0x40/0x74 [ 47.641371] sh_cmt_start+0x84/0x210 [ 47.649115] sh_cmt_clocksource_enable+0x28/0x50 [ 47.657916] change_clocksource+0x9c/0x160 [ 47.666165] multi_cpu_stop+0xa4/0x190 [ 47.674056] cpu_stopper_thread+0x90/0x154 [ 47.682308] smpboot_thread_fn+0x244/0x270 [ 47.690560] kthread+0x154/0x160 [ 47.697927] ret_from_fork+0x10/0x20 [ 47.708447] clocksource: Switched to clocksource e60f0000.timer Signed-off-by: Niklas Söderlund Reviewed-by: Geert Uytterhoeven Signed-off-by: Daniel Lezcano Link: https://lore.kernel.org/r/20201205021921.1456190-2-niklas.soderlund+renesas@ragnatech.se --- drivers/clocksource/sh_cmt.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/drivers/clocksource/sh_cmt.c b/drivers/clocksource/sh_cmt.c index 760777458a90..19fa3ef75e3b 100644 --- a/drivers/clocksource/sh_cmt.c +++ b/drivers/clocksource/sh_cmt.c @@ -319,7 +319,6 @@ static int sh_cmt_enable(struct sh_cmt_channel *ch) { int k, ret; - pm_runtime_get_sync(&ch->cmt->pdev->dev); dev_pm_syscore_device(&ch->cmt->pdev->dev, true); /* enable clock */ @@ -394,7 +393,6 @@ static void sh_cmt_disable(struct sh_cmt_channel *ch) clk_disable(ch->cmt->clk); dev_pm_syscore_device(&ch->cmt->pdev->dev, false); - pm_runtime_put(&ch->cmt->pdev->dev); } /* private flags */ @@ -562,10 +560,16 @@ static int sh_cmt_start(struct sh_cmt_channel *ch, unsigned long flag) int ret = 0; unsigned long flags; + if (flag & FLAG_CLOCKSOURCE) + pm_runtime_get_sync(&ch->cmt->pdev->dev); + raw_spin_lock_irqsave(&ch->lock, flags); - if (!(ch->flags & (FLAG_CLOCKEVENT | FLAG_CLOCKSOURCE))) + if (!(ch->flags & (FLAG_CLOCKEVENT | FLAG_CLOCKSOURCE))) { + if (flag & FLAG_CLOCKEVENT) + pm_runtime_get_sync(&ch->cmt->pdev->dev); ret = sh_cmt_enable(ch); + } if (ret) goto out; @@ -590,14 +594,20 @@ static void sh_cmt_stop(struct sh_cmt_channel *ch, unsigned long flag) f = ch->flags & (FLAG_CLOCKEVENT | FLAG_CLOCKSOURCE); ch->flags &= ~flag; - if (f && !(ch->flags & (FLAG_CLOCKEVENT | FLAG_CLOCKSOURCE))) + if (f && !(ch->flags & (FLAG_CLOCKEVENT | FLAG_CLOCKSOURCE))) { sh_cmt_disable(ch); + if (flag & FLAG_CLOCKEVENT) + pm_runtime_put(&ch->cmt->pdev->dev); + } /* adjust the timeout to maximum if only clocksource left */ if ((flag == FLAG_CLOCKEVENT) && (ch->flags & FLAG_CLOCKSOURCE)) __sh_cmt_set_next(ch, ch->max_match_value); raw_spin_unlock_irqrestore(&ch->lock, flags); + + if (flag & FLAG_CLOCKSOURCE) + pm_runtime_put(&ch->cmt->pdev->dev); } static struct sh_cmt_channel *cs_to_sh_cmt(struct clocksource *cs)