[v5,2/7] clk: tegra: Fix refcounting of gate clocks

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.

Fix this problem by removing the erroneous bump and by implementing the
disable_unused() callback, which disables the unused gates properly.

The visible effect of the bug is such that the unused clocks are never
gated if a loaded kernel module grabs the unused clocks and starts to use
them. In practice this shouldn't cause any real problems for the drivers
and boards supported by the kernel today.

Acked-by: Thierry Reding <treding@nvidia.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/clk/tegra/clk-periph-gate.c | 72 +++++++++++++++++++----------
 drivers/clk/tegra/clk-periph.c      | 11 +++++
 2 files changed, 58 insertions(+), 25 deletions(-)

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.
[...]
> 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
[...]
> @@ -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

18.03.2021 12:12, Michał Mirosław пишет:
> 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.
> [...]
>> 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
> [...]
>> @@ -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/ ?

I'll update this patch over the weekend, thanks!