From 2abc330c514fe56c570bb1a6318b054b06a4f72e Mon Sep 17 00:00:00 2001 From: Jernej Skrabec Date: Tue, 2 Apr 2019 23:06:21 +0200 Subject: clk: sunxi-ng: nkmp: Avoid GENMASK(-1, 0) Sometimes one of the nkmp factors is unused. This means that one of the factors shift and width values are set to 0. Current nkmp clock code generates a mask for each factor with GENMASK(width + shift - 1, shift). For unused factor this translates to GENMASK(-1, 0). This code is further expanded by C preprocessor to final version: (((~0UL) - (1UL << (0)) + 1) & (~0UL >> (BITS_PER_LONG - 1 - (-1)))) or a bit simplified: (~0UL & (~0UL >> BITS_PER_LONG)) It turns out that result of the second part (~0UL >> BITS_PER_LONG) is actually undefined by C standard, which clearly specifies: "If the value of the right operand is negative or is greater than or equal to the width of the promoted left operand, the behavior is undefined." Additionally, compiling kernel with aarch64-linux-gnu-gcc 8.3.0 gave different results whether literals or variables with same values as literals were used. GENMASK with literals -1 and 0 gives zero and with variables gives 0xFFFFFFFFFFFFFFF (~0UL). Because nkmp driver uses GENMASK with variables as parameter, expression calculates mask as ~0UL instead of 0. This has further consequences that LSB in register is always set to 1 (1 is neutral value for a factor and shift is 0). For example, H6 pll-de clock is set to 600 MHz by sun4i-drm driver, but due to this bug ends up being 300 MHz. Additionally, 300 MHz seems to be too low because following warning can be found in dmesg: [ 1.752763] WARNING: CPU: 2 PID: 41 at drivers/clk/sunxi-ng/ccu_common.c:41 ccu_helper_wait_for_lock.part.0+0x6c/0x90 [ 1.763378] Modules linked in: [ 1.766441] CPU: 2 PID: 41 Comm: kworker/2:1 Not tainted 5.1.0-rc2-next-20190401 #138 [ 1.774269] Hardware name: Pine H64 (DT) [ 1.778200] Workqueue: events deferred_probe_work_func [ 1.783341] pstate: 40000005 (nZcv daif -PAN -UAO) [ 1.788135] pc : ccu_helper_wait_for_lock.part.0+0x6c/0x90 [ 1.793623] lr : ccu_helper_wait_for_lock.part.0+0x48/0x90 [ 1.799107] sp : ffff000010f93840 [ 1.802422] x29: ffff000010f93840 x28: 0000000000000000 [ 1.807735] x27: ffff800073ce9d80 x26: ffff000010afd1b8 [ 1.813049] x25: ffffffffffffffff x24: 00000000ffffffff [ 1.818362] x23: 0000000000000001 x22: ffff000010abd5c8 [ 1.823675] x21: 0000000010000000 x20: 00000000685f367e [ 1.828987] x19: 0000000000001801 x18: 0000000000000001 [ 1.834300] x17: 0000000000000001 x16: 0000000000000000 [ 1.839613] x15: 0000000000000000 x14: ffff000010789858 [ 1.844926] x13: 0000000000000000 x12: 0000000000000001 [ 1.850239] x11: 0000000000000000 x10: 0000000000000970 [ 1.855551] x9 : ffff000010f936c0 x8 : ffff800074cec0d0 [ 1.860864] x7 : 0000800067117000 x6 : 0000000115c30b41 [ 1.866177] x5 : 00ffffffffffffff x4 : 002c959300bfe500 [ 1.871490] x3 : 0000000000000018 x2 : 0000000029aaaaab [ 1.876802] x1 : 00000000000002e6 x0 : 00000000686072bc [ 1.882114] Call trace: [ 1.884565] ccu_helper_wait_for_lock.part.0+0x6c/0x90 [ 1.889705] ccu_helper_wait_for_lock+0x10/0x20 [ 1.894236] ccu_nkmp_set_rate+0x244/0x2a8 [ 1.898334] clk_change_rate+0x144/0x290 [ 1.902258] clk_core_set_rate_nolock+0x180/0x1b8 [ 1.906963] clk_set_rate+0x34/0xa0 [ 1.910455] sun8i_mixer_bind+0x484/0x558 [ 1.914466] component_bind_all+0x10c/0x230 [ 1.918651] sun4i_drv_bind+0xc4/0x1a0 [ 1.922401] try_to_bring_up_master+0x164/0x1c0 [ 1.926932] __component_add+0xa0/0x168 [ 1.930769] component_add+0x10/0x18 [ 1.934346] sun8i_dw_hdmi_probe+0x18/0x20 [ 1.938443] platform_drv_probe+0x50/0xa0 [ 1.942455] really_probe+0xcc/0x280 [ 1.946032] driver_probe_device+0x54/0xe8 [ 1.950130] __device_attach_driver+0x80/0xb8 [ 1.954488] bus_for_each_drv+0x78/0xc8 [ 1.958326] __device_attach+0xd4/0x130 [ 1.962163] device_initial_probe+0x10/0x18 [ 1.966348] bus_probe_device+0x90/0x98 [ 1.970185] deferred_probe_work_func+0x6c/0xa0 [ 1.974720] process_one_work+0x1e0/0x320 [ 1.978732] worker_thread+0x228/0x428 [ 1.982484] kthread+0x120/0x128 [ 1.985714] ret_from_fork+0x10/0x18 [ 1.989290] ---[ end trace 9babd42e1ca4b84f ]--- This commit solves the issue by first checking value of the factor width. If it is equal to 0 (unused factor), mask is set to 0, otherwise GENMASK() macro is used as before. Fixes: d897ef56faf9 ("clk: sunxi-ng: Mask nkmp factors when setting register") Signed-off-by: Jernej Skrabec Signed-off-by: Maxime Ripard --- drivers/clk/sunxi-ng/ccu_nkmp.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/drivers/clk/sunxi-ng/ccu_nkmp.c b/drivers/clk/sunxi-ng/ccu_nkmp.c index 9b49adb20d07..69dfc6de1c4e 100644 --- a/drivers/clk/sunxi-ng/ccu_nkmp.c +++ b/drivers/clk/sunxi-ng/ccu_nkmp.c @@ -167,7 +167,7 @@ static int ccu_nkmp_set_rate(struct clk_hw *hw, unsigned long rate, unsigned long parent_rate) { struct ccu_nkmp *nkmp = hw_to_ccu_nkmp(hw); - u32 n_mask, k_mask, m_mask, p_mask; + u32 n_mask = 0, k_mask = 0, m_mask = 0, p_mask = 0; struct _ccu_nkmp _nkmp; unsigned long flags; u32 reg; @@ -186,10 +186,18 @@ static int ccu_nkmp_set_rate(struct clk_hw *hw, unsigned long rate, ccu_nkmp_find_best(parent_rate, rate, &_nkmp); - n_mask = GENMASK(nkmp->n.width + nkmp->n.shift - 1, nkmp->n.shift); - k_mask = GENMASK(nkmp->k.width + nkmp->k.shift - 1, nkmp->k.shift); - m_mask = GENMASK(nkmp->m.width + nkmp->m.shift - 1, nkmp->m.shift); - p_mask = GENMASK(nkmp->p.width + nkmp->p.shift - 1, nkmp->p.shift); + if (nkmp->n.width) + n_mask = GENMASK(nkmp->n.width + nkmp->n.shift - 1, + nkmp->n.shift); + if (nkmp->k.width) + k_mask = GENMASK(nkmp->k.width + nkmp->k.shift - 1, + nkmp->k.shift); + if (nkmp->m.width) + m_mask = GENMASK(nkmp->m.width + nkmp->m.shift - 1, + nkmp->m.shift); + if (nkmp->p.width) + p_mask = GENMASK(nkmp->p.width + nkmp->p.shift - 1, + nkmp->p.shift); spin_lock_irqsave(nkmp->common.lock, flags); -- cgit From 1054e4dd1cbbb7bf5cc836832648f9972134ef67 Mon Sep 17 00:00:00 2001 From: Jernej Skrabec Date: Wed, 3 Apr 2019 17:14:04 +0200 Subject: clk: sunxi-ng: nkmp: Explain why zero width check is needed Add an explanation why zero width check is needed when generating factor mask using GENMASK() macro. Signed-off-by: Jernej Skrabec Signed-off-by: Maxime Ripard --- drivers/clk/sunxi-ng/ccu_nkmp.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/clk/sunxi-ng/ccu_nkmp.c b/drivers/clk/sunxi-ng/ccu_nkmp.c index 69dfc6de1c4e..cbcdf664f336 100644 --- a/drivers/clk/sunxi-ng/ccu_nkmp.c +++ b/drivers/clk/sunxi-ng/ccu_nkmp.c @@ -186,6 +186,12 @@ static int ccu_nkmp_set_rate(struct clk_hw *hw, unsigned long rate, ccu_nkmp_find_best(parent_rate, rate, &_nkmp); + /* + * If width is 0, GENMASK() macro may not generate expected mask (0) + * as it falls under undefined behaviour by C standard due to shifts + * which are equal or greater than width of left operand. This can + * be easily avoided by explicitly checking if width is 0. + */ if (nkmp->n.width) n_mask = GENMASK(nkmp->n.width + nkmp->n.shift - 1, nkmp->n.shift); -- cgit From 5a7efdacb9dda1c35fb414d57b3e16d520e2c0db Mon Sep 17 00:00:00 2001 From: Stephen Boyd Date: Fri, 12 Apr 2019 11:31:42 -0700 Subject: clkdev: Hold clocks_mutex while iterating clocks list We recently introduced a change to support devm clk lookups. That change introduced a code-path that used clk_find() without holding the 'clocks_mutex'. Unfortunately, clk_find() iterates over the 'clocks' list and so we need to prevent the list from being modified at the same time. Do this by holding the mutex and checking to make sure it's held while iterating the list. Note, we don't really care if the lookup is freed after we find it with clk_find() because we're just doing a pointer comparison, but if we did care we would need to keep holding the mutex while we dereference the clk_lookup pointer. Fixes: 3eee6c7d119c ("clkdev: add managed clkdev lookup registration") Cc: Miquel Raynal Cc: Jerome Brunet Cc: Russell King Cc: Michael Turquette Cc: Jeffrey Hugo Cc: Chen-Yu Tsai Cc: Matti Vaittinen Acked-by: Matti Vaittinen Tested-by: Jeffrey Hugo Signed-off-by: Stephen Boyd --- drivers/clk/clkdev.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c index 8c4435c53f09..6e787cc9e5b9 100644 --- a/drivers/clk/clkdev.c +++ b/drivers/clk/clkdev.c @@ -46,6 +46,8 @@ static struct clk_lookup *clk_find(const char *dev_id, const char *con_id) if (con_id) best_possible += 1; + lockdep_assert_held(&clocks_mutex); + list_for_each_entry(p, &clocks, node) { match = 0; if (p->dev_id) { @@ -402,7 +404,10 @@ void devm_clk_release_clkdev(struct device *dev, const char *con_id, struct clk_lookup *cl; int rval; + mutex_lock(&clocks_mutex); cl = clk_find(dev_id, con_id); + mutex_unlock(&clocks_mutex); + WARN_ON(!cl); rval = devres_release(dev, devm_clkdev_release, devm_clk_match_clkdev, cl); -- cgit From b88c9f4129dcec941e5a26508e991c08051ed1ac Mon Sep 17 00:00:00 2001 From: Dmitry Osipenko Date: Thu, 25 Apr 2019 16:28:37 +0300 Subject: clk: Add missing stubs for a few functions Compilation fails if any of undeclared clk_set_*() functions are in use and CONFIG_HAVE_CLK=n. Reported-by: kbuild test robot Signed-off-by: Dmitry Osipenko Signed-off-by: Stephen Boyd --- include/linux/clk.h | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/include/linux/clk.h b/include/linux/clk.h index d8bc1a856b39..f689fc58d7be 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -811,6 +811,22 @@ static inline bool clk_has_parent(struct clk *clk, struct clk *parent) return true; } +static inline int clk_set_rate_range(struct clk *clk, unsigned long min, + unsigned long max) +{ + return 0; +} + +static inline int clk_set_min_rate(struct clk *clk, unsigned long rate) +{ + return 0; +} + +static inline int clk_set_max_rate(struct clk *clk, unsigned long rate) +{ + return 0; +} + static inline int clk_set_parent(struct clk *clk, struct clk *parent) { return 0; -- cgit