Re: clk: Per controller locks (prepare & enable)

[Javier Martinez Canillas <javier@osg.samsung.com>]

Hello Krzysztof,

On 06/29/2016 03:23 AM, Krzysztof Kozlowski wrote:
> Hi,
> 
> 
> Problems:
> 1. Deadlocks on prepare lock in following scenario:
>  - prepare_enable(some external clock through i2c)
>    - i2c transfer
>      - prepare_enable(i2c lock in SoC)
>        - deadlock
> See [1]. We implemented a workaround for this in few places like
> 10ff4c5239a1 ("i2c: exynos5: Fix possible ABBA deadlock by keeping I2C
> clock prepared") and 34e81ad5f0b6 ("i2c: s3c2410: fix ABBA deadlock by
> keeping clock prepared")
> 
> The goal would be to remove also this workaround.
>
> 2. The global prepare and enable locks are over-used. I didn't test the
> congestion but intuition says that they could be improved.
> 
> 
> Solution:
> Make this locks per controller. This will directly solve problem #1
> because these are different controllers. Still in-controller deadlock
> could occur but we couldn't find a real case with it.
>

Yes, I also tried first to remove the global locks but then noticed that
changing the global lock wouldn't be trivial, so I decided to got with
the workaround instead at the end.

> 
> Question:
> What do you think about it? I know that talk is cheap and code looks
> better but before starting the work I would like to hear some
> comments/opinions/ideas.
>

The problem is that the enable and prepare operations are propagated to
the parents, so what the locks want to protecting is really a sub-tree
of the clock tree. They currently protect the whole clock hierarchy to
make sure that the changes in the clock tree are atomically.

So a clock per controller won't suffice since you can have a parent for
a clock provided by a different controller and that won't be protected.

Another option is to have a prepare and enable locks per clock. Since
the operations are always propagated in the same direction, I think is
safe to do it.

But even in the case of a more fine-grained locking, I believe the
mentioned deadlocks can still happen. For example in 10ff4c5239a1 the
issue was that the s2mps11 PMIC has both regulators and clocks that are
controlled via I2C so the regulator and clocks drivers shares the same
I2C regmap.

What happened was something like this:

         CPU0                                   CPU1
         ----                                   ----
  regulator_set_voltage()                s3c_rtc_probe()
  regmap_write()                         clk_prepare()
  /* regmap->lock was aquired */         /* prepare_lock was aquired */
  regmap_i2c_write()                     s2mps11_clk_prepare()
  i2c_transfer()                         regmap_write()
  exynos5_i2c_xfer()                     /* deadlock due regmap->lock */
  clk_prepare_enable()
  clk_prepare_lock()
  /* prepare_lock was aquired */

So if for example a per clock lock is used, the deadlock can still happen
if both the I2C clock and S3C RTC source clock share the same parent in a
part of the hierarchy. But as you said this is less likely in practice so
probably is not an issue. 

The disadvantage with a per clock lock is that it may be too fine-grained,
so even when it will help with parallelism and scalability, it will hurt
performance since will be too costly to do a prepare and enable operation.

> 
> Best regards,
> Krzysztof
> 
> 
> [1]
> http://www.krzk.eu/builders/boot-odroid-xu3-multi_v7/builds/34/steps/Boot%20odroid/logs/serial
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America