On 06/30/2016 06:22 PM, Javier Martinez Canillas wrote:

quoted

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.

Although there is a hierarchy between clocks from different controllers
but still these are all clocks controllers coming from one hardware
device (like SoC). At least on Exynos, I think there is no real
inter-device dependencies. The deadlock you mentioned (and which I want
to fix) is between:
1. clock in PMIC (the one needed by s3c_rtc_probe()),
2. clock for I2C in SoC (the one needed by regmap_write()),
3. and regmap lock:

What I want to say is that the relationship between clocks even when
crossing clock controller boundaries is still self-contained. It is
simple parent-child relationship so acquiring both
clock-controller-level locks is safe.

Current dead lock looks like, simplifying your code:
A:                            B:
lock(regmap)
                              lock(prepare)
lock(prepare) - wait
                              lock(regmap) - wait


When split locks per clock controller this would be:
A:                            B:
lock(regmap)
                              lock(s2mps11)
lock(i2c/exynos)
                              lock(regmap) - wait
do the transfer
unlock(i2c/exynos)
unlock(regmap)
                              lock(regmap) - acquired
                              lock(i2c/exynos)
                              do the transfer
                              unlock(i2c/exynos)
                              unlock(regmap)
                              unlock(s2mps11)

I still have to figure out how to properly protect the entire tree
hierarchy. Maybe the global prepare_lock should be used only for that -
for traversing the hierarchy.

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.

I think these clocks do not share the parent.

Best regards,
Krzysztof