clk: Per controller locks (prepare & enable)

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.

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.

Best regards,
Krzysztof

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

Quoted 5 lines

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.

Quoted 10 lines · Javier Martinez Canillas

Quoted 5 lines

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.

Quoted 28 lines

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 */

Quoted 4 lines

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

But I'm very far from being a lock expert so maybe my assumptions are wrong,

I'll let Stephen and Mike to comment on this.

Hello Krzysztof,

Quoted 12 lines · Krzysztof Kozlowski

Quoted 10 lines · Javier Martinez Canillas

Quoted 5 lines

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.

Quoted 5 lines

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:

Yes, my point was that this may not be the case in all systems. IOW the framework should be generic enough to allow hierarchies where a parent clock is a clock provided by a different controller from a different HW.

Quoted 10 lines

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.

Is safe if the clock controllers are always aquired in the same order but I'm not sure if that will always be the case. I.e: you have controllers A and B that have clocks A{1,2} and B{1,2} respectively. So you could have something like this:

A1 with parent B1
B2 with parent A2

That can cause a deadlock since in the first case, the controller A will be aquired and then the controller B but in the other case, the opposite order will be attempted.

Quoted 13 lines

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)

Quoted 12 lines

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)

Yes, splitting the lock per controller will fix the possible deadlock in this case but I think we need an approach that is safe for all possible scenarios. Otherwise it will work more by coincidence than due a design.

On 07/04/2016 05:15 PM, Javier Martinez Canillas wrote:

Quoted 4 lines

Yes, splitting the lock per controller will fix the possible deadlock in this case but I think we need an approach that is safe for all possible scenarios. Otherwise it will work more by coincidence than due a design.

This is not a coincidence. This design is meant to fix this deadlock. Not by coincidence. By design.

You are talking about theoretical different configurations... without even real bug reports. I am providing an idea to fix a real deadlock and your argument is that it might not fix other (non-reported) deadlocks. These other deadlocks happen now as well probably...

Best regards,
Krzysztof

Hello Krzysztof,

Quoted 1 line · Krzysztof Kozlowski

On 07/04/2016 05:15 PM, Javier Martinez Canillas wrote:

[snip]

Quoted 9 lines

Quoted 5 lines

Yes, splitting the lock per controller will fix the possible deadlock in this case but I think we need an approach that is safe for all possible scenarios. Otherwise it will work more by coincidence than due a design.

This is not a coincidence. This design is meant to fix this deadlock. Not by coincidence. By design.

Ok, if the configurations I described doesn't exist in practice and are just theoretical then yes, doing a per controller lock is a good design.

Quoted 5 lines

You are talking about theoretical different configurations... without even real bug reports. I am providing an idea to fix a real deadlock and your argument is that it might not fix other (non-reported) deadlocks. These other deadlocks happen now as well probably...

I'm not against you re-working the locks to do it per controller, is just that I thought it would be good to have a solution that is going to work for all possible scenarios.

You asked for comments/opinions/ideas and I gave mine, that's all :)

Quoted 6 lines · Javier Martinez Canillas

Hello Krzysztof,

Quoted 1 line · Krzysztof Kozlowski

On 07/04/2016 05:15 PM, Javier Martinez Canillas wrote:

[snip]

I have also been have a brief look at this as we have been encountering issues attempting to move some of the clocking on our audio CODECs to the clock framework. The problems are even worse when the device can be controlled over SPI as well, as the SPI framework may occasionally defer the transfer to a worker thread rather than doing it in the same thread which causes the re-enterant behaviour of the clock locking to no longer function.

Quoted 11 lines

Quoted 10 lines

Quoted 6 lines

Yes, splitting the lock per controller will fix the possible deadlock in this case but I think we need an approach that is safe for all possible scenarios. Otherwise it will work more by coincidence than due a design.

This is not a coincidence. This design is meant to fix this deadlock. Not by coincidence. By design.

I think there is still some milage in thinking about them just to be sure, if we are going to the effort of changing the clock framework locking it is worth getting it right as it will be non-trivial.

I could perhaps imagine a situation where one device is passing a clock to second device and that device is doing some FLL/PLL and passing the resulting clock back. For example supplying a non-audio rate clock to a CODEC which then supplies back a clock at an audio rate, which is used for audio functions on the first device.

Although that said I do think that by controller locking probably fixes my primary issues right now.

Quoted 9 lines

Ok, if the configurations I described doesn't exist in practice and are just theoretical then yes, doing a per controller lock is a good design.

Quoted 5 lines

You are talking about theoretical different configurations... without even real bug reports. I am providing an idea to fix a real deadlock and your argument is that it might not fix other (non-reported) deadlocks. These other deadlocks happen now as well probably...

Quoted 6 lines

I'm not against you re-working the locks to do it per controller, is just that I thought it would be good to have a solution that is going to work for all possible scenarios.

You asked for comments/opinions/ideas and I gave mine, that's all :)

I had also been leaning more towards a lock per clock rather than a lock per controller. But one other issue that needs to be kept in mind (with both the controller or clock based locking) through is that the enable and prepare operations propagate down the clock tree, where as the set rate operations propagate up the clock tree. This makes things a rather furtile breeding ground for mutex inversions as well.

Quoted 4 lines · Charles Keepax

Quoted 3 lines · Javier Martinez Canillas

Hello Krzysztof,

On 07/05/2016 02:33 AM, Krzysztof Kozlowski wrote:

Quoted 8 lines

Quoted 1 line

Quoted 1 line · Javier Martinez Canillas

I have also been have a brief look at this as we have been encountering issues attempting to move some of the clocking on our audio CODECs to the clock framework. The problems are even worse when the device can be controlled over SPI as well, as the SPI framework may occasionally defer the transfer to a worker thread rather than doing it in the same thread which causes the re-enterant behaviour of the clock locking to no longer function.

As you mentioned later, in such case per-controller-lock won't help.

Quoted 6 lines

I could perhaps imagine a situation where one device is passing a clock to second device and that device is doing some FLL/PLL and passing the resulting clock back. For example supplying a non-audio rate clock to a CODEC which then supplies back a clock at an audio rate, which is used for audio functions on the first device.

What do you think by "passing" here? Pass the pointer to struct?

Quoted 8 lines

I had also been leaning more towards a lock per clock rather than a lock per controller. But one other issue that needs to be kept in mind (with both the controller or clock based locking) through is that the enable and prepare operations propagate down the clock tree, where as the set rate operations propagate up the clock tree. This makes things a rather furtile breeding ground for mutex inversions as well.

Yeah, that is the problem we were thinking about just a sec ago. :) The set rate (and reparent which might cause set rate) is complicating the design.

Idea I have is (simplifying only to prepare lock... leave away the enable):

1. Hava a global lock which will protect:
a. traversing clock controller hierarchy,
b. acquiring per clock controller locks,
2. Add struct for clock controller.
3. Add lock per clock controller.

The basic locking in case of prepare for a simplified case one clock per clock controller:

A (top controller = top clock)
\-B
\-C

clk_prepare(C) {
global_lock();

  for (clk_ctrl = C) {
    lock(clk_ctrl);
    clk_ctrl = get_parent_controller(C);

}
global_unlock();

prepare_cnt++;
// do the same for hierarchy

  for (clk_ctrl = C) {
    unlock(clk_ctrl)
    clock = get_parent_controller(C);

}
}