Re: [PATCH v5 2/7] clk: tegra: Fix refcounting of gate clocks
From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Date: 2021-03-18 09:14:41
Also in:
linux-clk, linux-tegra, lkml
On Wed, Mar 17, 2021 at 10:30:01PM +0300, Dmitry Osipenko wrote:
The refcounting of the gate clocks has a bug causing the enable_refcnt to underflow when unused clocks are disabled. This happens because clk provider erroneously bumps the refcount if clock is enabled at a boot time, which it shouldn't be doing, and it does this only for the gate clocks, while peripheral clocks are using the same gate ops and the peripheral clocks are missing the initial bump. Hence the refcount of the peripheral clocks is 0 when unused clocks are disabled and then the counter is decremented further by the gate ops, causing the integer underflow.
[...]
quoted hunk ↗ jump to hunk
diff --git a/drivers/clk/tegra/clk-periph-gate.c b/drivers/clk/tegra/clk-periph-gate.c index 4b31beefc9fc..3c4259fec82e 100644 --- a/drivers/clk/tegra/clk-periph-gate.c +++ b/drivers/clk/tegra/clk-periph-gate.c
[...]
quoted hunk ↗ jump to hunk
@@ -91,21 +108,28 @@ static void clk_periph_disable(struct clk_hw *hw) spin_lock_irqsave(&periph_ref_lock, flags); - gate->enable_refcnt[gate->clk_num]--; - if (gate->enable_refcnt[gate->clk_num] > 0) { - spin_unlock_irqrestore(&periph_ref_lock, flags); - return; - } + WARN_ON(!gate->enable_refcnt[gate->clk_num]); + + if (gate->enable_refcnt[gate->clk_num]-- == 1) + clk_periph_disable_locked(hw);
Nit: "if (--n == 0)" seems more natural, as you want to call clk_periph_disable_locked() when the refcount goes down to 0. [...]
/* - * If peripheral is in the APB bus then read the APB bus to - * flush the write operation in apb bus. This will avoid the - * peripheral access after disabling clock + * Some clocks are duplicated and some of them are marked as critical, + * like fuse and fuse_burn for example, thus the enable_refcnt will + * be non-zero here id the "unused" duplicate is disabled by CCF.
s/id/if/ ? Best Regards Michał Mirosław