* clk: Per controller locks (prepare & enable) @ 2016-06-29 7:23 Krzysztof Kozlowski 2016-06-30 16:22 ` Javier Martinez Canillas 0 siblings, 1 reply; 10+ messages in thread From: Krzysztof Kozlowski @ 2016-06-29 7:23 UTC (permalink / raw) To: Michael Turquette, Stephen Boyd, linux-clk, linux-kernel, linux-arm-kernel, Tomasz Figa, Sylwester Nawrocki Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, Andrzej Hajda, Javier Martinez Canillas 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: clk: Per controller locks (prepare & enable) 2016-06-29 7:23 clk: Per controller locks (prepare & enable) Krzysztof Kozlowski @ 2016-06-30 16:22 ` Javier Martinez Canillas 2016-07-04 8:24 ` Krzysztof Kozlowski 0 siblings, 1 reply; 10+ messages in thread From: Javier Martinez Canillas @ 2016-06-30 16:22 UTC (permalink / raw) To: Krzysztof Kozlowski, Michael Turquette, Stephen Boyd, linux-clk, linux-kernel, linux-arm-kernel, Tomasz Figa, Sylwester Nawrocki Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, Andrzej Hajda 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: clk: Per controller locks (prepare & enable) 2016-06-30 16:22 ` Javier Martinez Canillas @ 2016-07-04 8:24 ` Krzysztof Kozlowski 2016-07-04 15:15 ` Javier Martinez Canillas 0 siblings, 1 reply; 10+ messages in thread From: Krzysztof Kozlowski @ 2016-07-04 8:24 UTC (permalink / raw) To: Javier Martinez Canillas, Michael Turquette, Stephen Boyd, linux-clk, linux-kernel, linux-arm-kernel, Tomasz Figa, Sylwester Nawrocki Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, Andrzej Hajda On 06/30/2016 06:22 PM, Javier Martinez Canillas wrote: >> 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: clk: Per controller locks (prepare & enable) 2016-07-04 8:24 ` Krzysztof Kozlowski @ 2016-07-04 15:15 ` Javier Martinez Canillas 2016-07-04 15:21 ` Javier Martinez Canillas 2016-07-05 6:33 ` Krzysztof Kozlowski 0 siblings, 2 replies; 10+ messages in thread From: Javier Martinez Canillas @ 2016-07-04 15:15 UTC (permalink / raw) To: Krzysztof Kozlowski, Michael Turquette, Stephen Boyd, linux-clk, linux-kernel, linux-arm-kernel, Tomasz Figa, Sylwester Nawrocki Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, Andrzej Hajda Hello Krzysztof, On 07/04/2016 04:24 AM, Krzysztof Kozlowski wrote: > On 06/30/2016 06:22 PM, Javier Martinez Canillas wrote: >>> 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: 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. > 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. > 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) > 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. > 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. > Other thing to figure out is how to determine when a parent belongs to a different controller to get its lock in clk_{prepare,enable}(), at least part of the hierarchy has to be traversed on prepare/enable to propagate the operations to the parents clocks. >> >> 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. > Yes, I meant the issue of having a shared parent in general and not in this particular case that no parent is shared. Sorry for the ambiguous sentence. > Best regards, > Krzysztof > So in summary, my point is that we should first define what these locks are protecting. The good practice in the kernel is to protect data structures instead of code, but the prepare and enable locks are not doing the former. My understanding is that what the locks are protecting is a sub-tree of the hierarchy but there isn't a data structure to represent a sub-tree so global locks are used to protect the whole tree instead. Moving the lock to a per controller/provider seems like an arbitrary thing to me since what's important AFAIU is the clock hierarchy and not from where these clocks come from. 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. Best regards, -- Javier Martinez Canillas Open Source Group Samsung Research America ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: clk: Per controller locks (prepare & enable) 2016-07-04 15:15 ` Javier Martinez Canillas @ 2016-07-04 15:21 ` Javier Martinez Canillas 2016-07-05 6:33 ` Krzysztof Kozlowski 1 sibling, 0 replies; 10+ messages in thread From: Javier Martinez Canillas @ 2016-07-04 15:21 UTC (permalink / raw) To: Krzysztof Kozlowski, Michael Turquette, Stephen Boyd, linux-clk, linux-kernel, linux-arm-kernel, Tomasz Figa, Sylwester Nawrocki Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, Andrzej Hajda On 07/04/2016 11:15 AM, Javier Martinez Canillas wrote: > > But I'm very far from being a lock expert so maybe my assumptions are wrong, I meant *clock* here, sorry for the typo. > I'll let Stephen and Mike to comment on this. > Best regards, -- Javier Martinez Canillas Open Source Group Samsung Research America ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: clk: Per controller locks (prepare & enable) 2016-07-04 15:15 ` Javier Martinez Canillas 2016-07-04 15:21 ` Javier Martinez Canillas @ 2016-07-05 6:33 ` Krzysztof Kozlowski 2016-07-05 13:48 ` Javier Martinez Canillas 1 sibling, 1 reply; 10+ messages in thread From: Krzysztof Kozlowski @ 2016-07-05 6:33 UTC (permalink / raw) To: Javier Martinez Canillas, Michael Turquette, Stephen Boyd, linux-clk, linux-kernel, linux-arm-kernel, Tomasz Figa, Sylwester Nawrocki Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, Andrzej Hajda On 07/04/2016 05:15 PM, Javier Martinez Canillas wrote: > Hello Krzysztof, > > On 07/04/2016 04:24 AM, Krzysztof Kozlowski wrote: >> On 06/30/2016 06:22 PM, Javier Martinez Canillas wrote: >>>> 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: > > 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. Is there such configuration? > >> 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 Again, is there such configuration? We thought here about it and at least it is not known to us. Of course this is not a proof that such configuration does not exist... > > 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. Yes. > >> 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) >> > > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: clk: Per controller locks (prepare & enable) 2016-07-05 6:33 ` Krzysztof Kozlowski @ 2016-07-05 13:48 ` Javier Martinez Canillas 2016-07-07 12:06 ` Charles Keepax 0 siblings, 1 reply; 10+ messages in thread From: Javier Martinez Canillas @ 2016-07-05 13:48 UTC (permalink / raw) To: Krzysztof Kozlowski, Michael Turquette, Stephen Boyd, linux-clk, linux-kernel, linux-arm-kernel, Tomasz Figa, Sylwester Nawrocki Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, Andrzej Hajda Hello Krzysztof, On 07/05/2016 02:33 AM, Krzysztof Kozlowski wrote: > On 07/04/2016 05:15 PM, Javier Martinez Canillas wrote: [snip] >> >> 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. > 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 :) > Best regards, > Krzysztof > Best regards, -- Javier Martinez Canillas Open Source Group Samsung Research America ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: clk: Per controller locks (prepare & enable) 2016-07-05 13:48 ` Javier Martinez Canillas @ 2016-07-07 12:06 ` Charles Keepax 2016-07-07 12:42 ` Krzysztof Kozlowski 0 siblings, 1 reply; 10+ messages in thread From: Charles Keepax @ 2016-07-07 12:06 UTC (permalink / raw) To: Javier Martinez Canillas Cc: Krzysztof Kozlowski, Michael Turquette, Stephen Boyd, linux-clk, linux-kernel, linux-arm-kernel, Tomasz Figa, Sylwester Nawrocki, Andrzej Hajda, Marek Szyprowski, Bartlomiej Zolnierkiewicz On Tue, Jul 05, 2016 at 09:48:34AM -0400, Javier Martinez Canillas wrote: > Hello Krzysztof, > > On 07/05/2016 02:33 AM, Krzysztof Kozlowski wrote: > > 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. > > >> > >> 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. > 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. > > > 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 :) > 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. Thanks, Charles ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: clk: Per controller locks (prepare & enable) 2016-07-07 12:06 ` Charles Keepax @ 2016-07-07 12:42 ` Krzysztof Kozlowski 2016-07-07 16:00 ` Charles Keepax 0 siblings, 1 reply; 10+ messages in thread From: Krzysztof Kozlowski @ 2016-07-07 12:42 UTC (permalink / raw) To: Charles Keepax, Javier Martinez Canillas Cc: Michael Turquette, Stephen Boyd, linux-clk, linux-kernel, linux-arm-kernel, Tomasz Figa, Sylwester Nawrocki, Andrzej Hajda, Marek Szyprowski, Bartlomiej Zolnierkiewicz On 07/07/2016 02:06 PM, Charles Keepax wrote: > On Tue, Jul 05, 2016 at 09:48:34AM -0400, Javier Martinez Canillas wrote: >> Hello Krzysztof, >> >> On 07/05/2016 02:33 AM, Krzysztof Kozlowski wrote: >>> 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. As you mentioned later, in such case per-controller-lock won't help. > >> >>>> >>>> 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. What do you think by "passing" here? Pass the pointer to struct? > > Although that said I do think that by controller locking probably > fixes my primary issues right now. > >> 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. >> >>> 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 :) >> > > 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); } } The global lock is actually needed for the case of inverted walk during set_rate(). Best regards, Krzysztof ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: clk: Per controller locks (prepare & enable) 2016-07-07 12:42 ` Krzysztof Kozlowski @ 2016-07-07 16:00 ` Charles Keepax 0 siblings, 0 replies; 10+ messages in thread From: Charles Keepax @ 2016-07-07 16:00 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Javier Martinez Canillas, Michael Turquette, Stephen Boyd, linux-clk, linux-kernel, linux-arm-kernel, Tomasz Figa, Sylwester Nawrocki, Andrzej Hajda, Marek Szyprowski, Bartlomiej Zolnierkiewicz On Thu, Jul 07, 2016 at 02:42:17PM +0200, Krzysztof Kozlowski wrote: > On 07/07/2016 02:06 PM, Charles Keepax wrote: > > On Tue, Jul 05, 2016 at 09:48:34AM -0400, Javier Martinez Canillas wrote: > >> Hello Krzysztof, > >> > >> On 07/05/2016 02:33 AM, Krzysztof Kozlowski wrote: > >>> On 07/04/2016 05:15 PM, Javier Martinez Canillas wrote: > > 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. > It should help as the SPI clocks and the (in this case) CODEC clocks are unlikely to be on the same controller. > > 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? > Apologies for being unclear there, I was really just referring to where the source for each clock is coming from. Given controllers C1 and C2, and putting the clock in brackets afterwards: C1(MCLK@26MHz) is the parent of C2(FLL@24.576MHz) which is the parent of C1(AUDIO@24.576MHz). Which makes C2 both a parent and child of C1. Its probably not that likely but I could see it happening. > > 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): Certainly I think only worrying about prepare makes sense. > 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); > } > } I think this fixes the issues I have been having at my side. I will try to find some time in the next few days to go through and refresh my memory. I guess lets wait and see if the clock guys have any thoughts. Thanks, Charles ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-07-07 16:00 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-06-29 7:23 clk: Per controller locks (prepare & enable) Krzysztof Kozlowski 2016-06-30 16:22 ` Javier Martinez Canillas 2016-07-04 8:24 ` Krzysztof Kozlowski 2016-07-04 15:15 ` Javier Martinez Canillas 2016-07-04 15:21 ` Javier Martinez Canillas 2016-07-05 6:33 ` Krzysztof Kozlowski 2016-07-05 13:48 ` Javier Martinez Canillas 2016-07-07 12:06 ` Charles Keepax 2016-07-07 12:42 ` Krzysztof Kozlowski 2016-07-07 16:00 ` Charles Keepax
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).