Without this patch, the following race conditions are possible.
Race condition 1:
* clk-A has two parents - clk-X and clk-Y.
* All three are disabled and clk-X is current parent.
* Thread A: clk_set_parent(clk-A, clk-Y).
* Thread A: <snip execution flow>
* Thread A: Grabs enable lock.
* Thread A: Sees enable count of clk-A is 0, so doesn't enable clk-Y.
* Thread A: Releases enable lock.
* Thread B: Calls clk_enable(clk-A), which in turn enables clk-X.
* Thread A: Switches clk-A's parent to clk-Y in hardware.
clk-A is now enabled in software, but not clocking in hardware.
Race condition 2:
* clk-A has two parents - clk-X and clk-Y.
* All three are disabled and clk-X is current parent.
* Thread A: clk_set_parent(clk-A, clk-Y).
* Thread A: <snip execution flow>
* Thread A: Switches parent in hardware to clk-Y.
* Thread A: Grabs enable lock.
* Thread A: Sees enable count of clk-A is 0, so doesn't disable clk-X.
* Thread A: Releases enable lock.
* Thread B: Calls clk_enable(clk-A)
* Thread B: Software state still says parent is clk-X.
* Thread B: So, enables clk-X and then itself.
* Thread A: Updates parent in software state to clk-Y.
clk-A is now enabled in software, but not clocking in hardware. clk-X will
never be disabled since it's enable count is 1 when no one needs it. clk-Y
will throw a warning when clk-A is disabled again (assuming clk-A being
disabled in hardware hasn't wedged the system).
To fix these race conditions, hold the enable lock while switching the clock
parent in hardware. But this would force the set_parent() ops to be atomic,
which might not be possible if the clock hardware is external to the SoC.
Since clocks with CLK_SET_PARENT_GATE flag only allow clk_set_parent() on
unprepared clocks and calling clk_enable() on an unprepared clock would be
violating the clock API usage model, allow set_parent() ops to be sleepable
for clocks which have the CLK_SET_PARENT_GATE flag. Putting it another way,
if a clock's parent can't be switched without sleeping, then by definition
the parent can't be switched while it's prepared (CLK_SET_PARENT_GATE).
Signed-off-by: Saravana Kannan <[email protected]>
Cc: Mike Turquette <[email protected]>
Cc: Andrew Lunn <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Russell King <[email protected]>
Cc: Jeremy Kerr <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Arnd Bergman <[email protected]>
Cc: Paul Walmsley <[email protected]>
Cc: Shawn Guo <[email protected]>
Cc: Sascha Hauer <[email protected]>
Cc: Jamie Iles <[email protected]>
Cc: Richard Zhao <[email protected]>
Cc: Saravana Kannan <[email protected]>
Cc: Magnus Damm <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Linus Walleij <[email protected]>
Cc: Stephen Boyd <[email protected]>
Cc: Amit Kucheria <[email protected]>
Cc: Deepak Saxena <[email protected]>
Cc: Grant Likely <[email protected]>
---
Additional comments that I'm not sure are fit for the commit text:
Reason for repeating the call to set_parent() ops and updating clk->parent
for the CLK_SET_PARENT_GATE case:
* It looks weird to wrap the migration code and the lock/unlock in separate
if's.
* Once we add proper error checking for the return value of set_parent()
ops, the code will look even more convoluted if we try to share the code
for CLK_SET_PARENT_GATE case and non-CLK_SET_PARENT_GATE case.
I realize that clk->parent = parent is repeated in __clk_reparent(). But I
left that as is for now in case anyone is using that in one of for-next
branches. If no one is using it, we can remove it.
For a similar reason, clocks that need to do reparenting during
clk_set_rate() and don't have CLK_SET_RATE_GATE set can't do it correctly
with the current clk-provider APIs provided by the common clock framework.
__clk_set_parent() should really be split into two APIs for this to work:
__clk_pre_reparent() - enable lock grabbed here.
__clk_post_reparent() - enable lock released here.
drivers/clk/clk.c | 16 +++++++++++-----
1 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index e5d5dc1..09b9112 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1059,7 +1059,7 @@ static int __clk_set_parent(struct clk *clk, struct clk *parent)
{
struct clk *old_parent;
unsigned long flags;
- int ret = -EINVAL;
+ int ret;
u8 i;
old_parent = clk->parent;
@@ -1083,7 +1083,13 @@ static int __clk_set_parent(struct clk *clk, struct clk *parent)
if (i == clk->num_parents) {
pr_debug("%s: clock %s is not a possible parent of clock %s\n",
__func__, parent->name, clk->name);
- goto out;
+ return -EINVAL;
+ }
+
+ if (clk->flags & CLK_SET_PARENT_GATE) {
+ ret = clk->ops->set_parent(clk->hw, i);
+ clk->parent = parent;
+ return ret;
}
/* migrate prepare and enable */
@@ -1092,23 +1098,23 @@ static int __clk_set_parent(struct clk *clk, struct clk *parent)
/* FIXME replace with clk_is_enabled(clk) someday */
spin_lock_irqsave(&enable_lock, flags);
+
if (clk->enable_count)
__clk_enable(parent);
- spin_unlock_irqrestore(&enable_lock, flags);
/* change clock input source */
ret = clk->ops->set_parent(clk->hw, i);
+ clk->parent = parent;
/* clean up old prepare and enable */
- spin_lock_irqsave(&enable_lock, flags);
if (clk->enable_count)
__clk_disable(old_parent);
+
spin_unlock_irqrestore(&enable_lock, flags);
if (clk->prepare_count)
__clk_unprepare(old_parent);
-out:
return ret;
}
--
1.7.8.3
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
On 05/11/2012 09:59 PM, Saravana Kannan wrote:
> Without this patch, the following race conditions are possible.
>
> Race condition 1:
> * clk-A has two parents - clk-X and clk-Y.
> * All three are disabled and clk-X is current parent.
> * Thread A: clk_set_parent(clk-A, clk-Y).
> * Thread A:<snip execution flow>
> * Thread A: Grabs enable lock.
> * Thread A: Sees enable count of clk-A is 0, so doesn't enable clk-Y.
> * Thread A: Releases enable lock.
> * Thread B: Calls clk_enable(clk-A), which in turn enables clk-X.
> * Thread A: Switches clk-A's parent to clk-Y in hardware.
>
> clk-A is now enabled in software, but not clocking in hardware.
>
> Race condition 2:
> * clk-A has two parents - clk-X and clk-Y.
> * All three are disabled and clk-X is current parent.
> * Thread A: clk_set_parent(clk-A, clk-Y).
> * Thread A:<snip execution flow>
> * Thread A: Switches parent in hardware to clk-Y.
> * Thread A: Grabs enable lock.
> * Thread A: Sees enable count of clk-A is 0, so doesn't disable clk-X.
> * Thread A: Releases enable lock.
> * Thread B: Calls clk_enable(clk-A)
> * Thread B: Software state still says parent is clk-X.
> * Thread B: So, enables clk-X and then itself.
> * Thread A: Updates parent in software state to clk-Y.
>
> clk-A is now enabled in software, but not clocking in hardware. clk-X will
> never be disabled since it's enable count is 1 when no one needs it. clk-Y
> will throw a warning when clk-A is disabled again (assuming clk-A being
> disabled in hardware hasn't wedged the system).
>
> To fix these race conditions, hold the enable lock while switching the clock
> parent in hardware. But this would force the set_parent() ops to be atomic,
> which might not be possible if the clock hardware is external to the SoC.
>
> Since clocks with CLK_SET_PARENT_GATE flag only allow clk_set_parent() on
> unprepared clocks and calling clk_enable() on an unprepared clock would be
> violating the clock API usage model, allow set_parent() ops to be sleepable
> for clocks which have the CLK_SET_PARENT_GATE flag. Putting it another way,
> if a clock's parent can't be switched without sleeping, then by definition
> the parent can't be switched while it's prepared (CLK_SET_PARENT_GATE).
>
> Signed-off-by: Saravana Kannan<[email protected]>
> Cc: Mike Turquette<[email protected]>
> Cc: Andrew Lunn<[email protected]>
> Cc: Rob Herring<[email protected]>
> Cc: Russell King<[email protected]>
> Cc: Jeremy Kerr<[email protected]>
> Cc: Thomas Gleixner<[email protected]>
> Cc: Arnd Bergman<[email protected]>
> Cc: Paul Walmsley<[email protected]>
> Cc: Shawn Guo<[email protected]>
> Cc: Sascha Hauer<[email protected]>
> Cc: Jamie Iles<[email protected]>
> Cc: Richard Zhao<[email protected]>
> Cc: Saravana Kannan<[email protected]>
> Cc: Magnus Damm<[email protected]>
> Cc: Mark Brown<[email protected]>
> Cc: Linus Walleij<[email protected]>
> Cc: Stephen Boyd<[email protected]>
> Cc: Amit Kucheria<[email protected]>
> Cc: Deepak Saxena<[email protected]>
> Cc: Grant Likely<[email protected]>
> ---
> Additional comments that I'm not sure are fit for the commit text:
>
> Reason for repeating the call to set_parent() ops and updating clk->parent
> for the CLK_SET_PARENT_GATE case:
> * It looks weird to wrap the migration code and the lock/unlock in separate
> if's.
> * Once we add proper error checking for the return value of set_parent()
> ops, the code will look even more convoluted if we try to share the code
> for CLK_SET_PARENT_GATE case and non-CLK_SET_PARENT_GATE case.
>
> I realize that clk->parent = parent is repeated in __clk_reparent(). But I
> left that as is for now in case anyone is using that in one of for-next
> branches. If no one is using it, we can remove it.
>
> For a similar reason, clocks that need to do reparenting during
> clk_set_rate() and don't have CLK_SET_RATE_GATE set can't do it correctly
> with the current clk-provider APIs provided by the common clock framework.
>
> __clk_set_parent() should really be split into two APIs for this to work:
> __clk_pre_reparent() - enable lock grabbed here.
> __clk_post_reparent() - enable lock released here.
>
> drivers/clk/clk.c | 16 +++++++++++-----
> 1 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index e5d5dc1..09b9112 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1059,7 +1059,7 @@ static int __clk_set_parent(struct clk *clk, struct clk *parent)
> {
> struct clk *old_parent;
> unsigned long flags;
> - int ret = -EINVAL;
> + int ret;
> u8 i;
>
> old_parent = clk->parent;
> @@ -1083,7 +1083,13 @@ static int __clk_set_parent(struct clk *clk, struct clk *parent)
> if (i == clk->num_parents) {
> pr_debug("%s: clock %s is not a possible parent of clock %s\n",
> __func__, parent->name, clk->name);
> - goto out;
> + return -EINVAL;
> + }
> +
> + if (clk->flags& CLK_SET_PARENT_GATE) {
> + ret = clk->ops->set_parent(clk->hw, i);
> + clk->parent = parent;
> + return ret;
> }
>
> /* migrate prepare and enable */
> @@ -1092,23 +1098,23 @@ static int __clk_set_parent(struct clk *clk, struct clk *parent)
>
> /* FIXME replace with clk_is_enabled(clk) someday */
> spin_lock_irqsave(&enable_lock, flags);
> +
> if (clk->enable_count)
> __clk_enable(parent);
> - spin_unlock_irqrestore(&enable_lock, flags);
>
> /* change clock input source */
> ret = clk->ops->set_parent(clk->hw, i);
> + clk->parent = parent;
>
> /* clean up old prepare and enable */
> - spin_lock_irqsave(&enable_lock, flags);
> if (clk->enable_count)
> __clk_disable(old_parent);
> +
> spin_unlock_irqrestore(&enable_lock, flags);
>
> if (clk->prepare_count)
> __clk_unprepare(old_parent);
>
> -out:
> return ret;
> }
>
(*nudge*) Thoughts anyone?
-Saravana
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Hi Saravana,
On Fri, May 11, 2012 at 09:59:56PM -0700, Saravana Kannan wrote:
> Without this patch, the following race conditions are possible.
>
> Race condition 1:
> * clk-A has two parents - clk-X and clk-Y.
> * All three are disabled and clk-X is current parent.
> * Thread A: clk_set_parent(clk-A, clk-Y).
> * Thread A: <snip execution flow>
> * Thread A: Grabs enable lock.
> * Thread A: Sees enable count of clk-A is 0, so doesn't enable clk-Y.
> * Thread A: Releases enable lock.
> * Thread B: Calls clk_enable(clk-A), which in turn enables clk-X.
> * Thread A: Switches clk-A's parent to clk-Y in hardware.
>
> clk-A is now enabled in software, but not clocking in hardware.
>
> Race condition 2:
> * clk-A has two parents - clk-X and clk-Y.
> * All three are disabled and clk-X is current parent.
> * Thread A: clk_set_parent(clk-A, clk-Y).
> * Thread A: <snip execution flow>
> * Thread A: Switches parent in hardware to clk-Y.
> * Thread A: Grabs enable lock.
> * Thread A: Sees enable count of clk-A is 0, so doesn't disable clk-X.
> * Thread A: Releases enable lock.
> * Thread B: Calls clk_enable(clk-A)
> * Thread B: Software state still says parent is clk-X.
> * Thread B: So, enables clk-X and then itself.
> * Thread A: Updates parent in software state to clk-Y.
Had a look at this and I can follow your reasoning and you patch seems
to fix this. However, there is a problem,
>
> /* migrate prepare and enable */
> @@ -1092,23 +1098,23 @@ static int __clk_set_parent(struct clk *clk, struct clk *parent)
>
> /* FIXME replace with clk_is_enabled(clk) someday */
> spin_lock_irqsave(&enable_lock, flags);
> +
> if (clk->enable_count)
> __clk_enable(parent);
> - spin_unlock_irqrestore(&enable_lock, flags);
>
> /* change clock input source */
> ret = clk->ops->set_parent(clk->hw, i);
You call ->set_parent while holding a spinlock. This won't work with i2c
clocks.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On 05/15/2012 12:42 PM, Sascha Hauer wrote:
> Hi Saravana,
>
> On Fri, May 11, 2012 at 09:59:56PM -0700, Saravana Kannan wrote:
>> Without this patch, the following race conditions are possible.
>>
>> Race condition 1:
>> * clk-A has two parents - clk-X and clk-Y.
>> * All three are disabled and clk-X is current parent.
>> * Thread A: clk_set_parent(clk-A, clk-Y).
>> * Thread A:<snip execution flow>
>> * Thread A: Grabs enable lock.
>> * Thread A: Sees enable count of clk-A is 0, so doesn't enable clk-Y.
>> * Thread A: Releases enable lock.
>> * Thread B: Calls clk_enable(clk-A), which in turn enables clk-X.
>> * Thread A: Switches clk-A's parent to clk-Y in hardware.
>>
>> clk-A is now enabled in software, but not clocking in hardware.
>>
>> Race condition 2:
>> * clk-A has two parents - clk-X and clk-Y.
>> * All three are disabled and clk-X is current parent.
>> * Thread A: clk_set_parent(clk-A, clk-Y).
>> * Thread A:<snip execution flow>
>> * Thread A: Switches parent in hardware to clk-Y.
>> * Thread A: Grabs enable lock.
>> * Thread A: Sees enable count of clk-A is 0, so doesn't disable clk-X.
>> * Thread A: Releases enable lock.
>> * Thread B: Calls clk_enable(clk-A)
>> * Thread B: Software state still says parent is clk-X.
>> * Thread B: So, enables clk-X and then itself.
>> * Thread A: Updates parent in software state to clk-Y.
>
> Had a look at this and I can follow your reasoning and you patch seems
> to fix this. However, there is a problem,
>
>>
>> /* migrate prepare and enable */
>> @@ -1092,23 +1098,23 @@ static int __clk_set_parent(struct clk *clk, struct clk *parent)
>>
>> /* FIXME replace with clk_is_enabled(clk) someday */
>> spin_lock_irqsave(&enable_lock, flags);
>> +
>> if (clk->enable_count)
>> __clk_enable(parent);
>> - spin_unlock_irqrestore(&enable_lock, flags);
>>
>> /* change clock input source */
>> ret = clk->ops->set_parent(clk->hw, i);
>
> You call ->set_parent while holding a spinlock. This won't work with i2c
> clocks.
I did account for that. I explained it in the commit text. Please let me
know if any part of that is not clear or is not correct.
Thanks,
Saravana
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
On Tue, May 15, 2012 at 12:51:06PM -0700, Saravana Kannan wrote:
> >> ret = clk->ops->set_parent(clk->hw, i);
> >
> >You call ->set_parent while holding a spinlock. This won't work with i2c
> >clocks.
>
> I did account for that. I explained it in the commit text. Please
> let me know if any part of that is not clear or is not correct.
>
I missed this part in the commit log. I have no idea whether we can live
with this limitation though.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On 05/15/2012 01:00 PM, Sascha Hauer wrote:
> On Tue, May 15, 2012 at 12:51:06PM -0700, Saravana Kannan wrote:
>>>> ret = clk->ops->set_parent(clk->hw, i);
>>>
>>> You call ->set_parent while holding a spinlock. This won't work with i2c
>>> clocks.
>>
>> I did account for that. I explained it in the commit text. Please
>> let me know if any part of that is not clear or is not correct.
>>
>
> I missed this part in the commit log. I have no idea whether we can live
> with this limitation though.
>
> Sascha
>
It's not really an artificial limitation of the patch. This has to be
enforced if the clock is to be managed correctly while allowing
.set_parent to NOT be atomic.
There is no way to guarantee that the enable/disable is properly
propagated to the parent clock if we can't guarantee mutual exclusion
between changing parents and calling enable/disable.
Since we can't do mutual exclusion be using spinlock (since .set_parent
is NOT atomic for these clocks), then only other way of ensuring mutual
exclusion is to force an unprepare and then mutually exclude a prepare
while changing the parent. This by association (can't enable unprepared
clock) mutually excludes the changing of parent and calling enable/disable.
Thanks,
Saravana
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
The clk_set_rate() code shouldn't check the clock's enable count when
validating CLK_SET_RATE_GATE flag since the enable count could change after
the validation. Similar to clk_set_parent(), it should instead check the
prepare count. The prepare count should go to zero only when the end user
expects the clock to not be enabled in the future. Since the code already
grabs the prepare count before validation, it's not possible for prepare
count to change after validation and by association not possible for a well
behaving end user to enable the clock while the set rate is in progress.
Signed-off-by: Saravana Kannan <[email protected]>
---
Please let me know if you don't want me to directly email or CC you in my
clock related patches. I don't want to spam anyone. Also, let me know if
you want me add you to my standard list of people to cc in my clock
patches.
drivers/clk/clk.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 09b9112..f5b9d3c 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -903,7 +903,7 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
if (rate == clk->rate)
goto out;
- if ((clk->flags & CLK_SET_RATE_GATE) && __clk_is_enabled(clk)) {
+ if ((clk->flags & CLK_SET_RATE_GATE) && clk->prepare_count) {
ret = -EBUSY;
goto out;
}
--
1.7.8.3
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Saravana Kannan <[email protected]> wrote:
>The clk_set_rate() code shouldn't check the clock's enable count when
>validating CLK_SET_RATE_GATE flag since the enable count could change
>after
>the validation. Similar to clk_set_parent(), it should instead check
>the
>prepare count. The prepare count should go to zero only when the end
>user
>expects the clock to not be enabled in the future. Since the code
>already
>grabs the prepare count before validation, it's not possible for
>prepare
>count to change after validation and by association not possible for a
>well
>behaving end user to enable the clock while the set rate is in
>progress.
>
>Signed-off-by: Saravana Kannan <[email protected]>
>---
>Please let me know if you don't want me to directly email or CC you in
>my
>clock related patches. I don't want to spam anyone. Also, let me know
>if
>you want me add you to my standard list of people to cc in my clock
>patches.
>
> drivers/clk/clk.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
>diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>index 09b9112..f5b9d3c 100644
>--- a/drivers/clk/clk.c
>+++ b/drivers/clk/clk.c
>@@ -903,7 +903,7 @@ int clk_set_rate(struct clk *clk, unsigned long
>rate)
> if (rate == clk->rate)
> goto out;
>
>- if ((clk->flags & CLK_SET_RATE_GATE) && __clk_is_enabled(clk)) {
>+ if ((clk->flags & CLK_SET_RATE_GATE) && clk->prepare_count) {
The condition becomes more strict. Looks good.
> ret = -EBUSY;
> goto out;
> }
>--
>1.7.8.3
>
>Sent by an employee of the Qualcomm Innovation Center, Inc.
>The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>Forum.
On Tue, May 15, 2012 at 01:43:42PM -0700, Saravana Kannan wrote:
> The clk_set_rate() code shouldn't check the clock's enable count when
> validating CLK_SET_RATE_GATE flag since the enable count could change after
> the validation. Similar to clk_set_parent(), it should instead check the
> prepare count. The prepare count should go to zero only when the end user
> expects the clock to not be enabled in the future. Since the code already
> grabs the prepare count before validation, it's not possible for prepare
> count to change after validation and by association not possible for a well
> behaving end user to enable the clock while the set rate is in progress.
>
> Signed-off-by: Saravana Kannan <[email protected]>
Reviewed-by: Richard Zhao <[email protected]>
> ---
> Please let me know if you don't want me to directly email or CC you in my
> clock related patches. I don't want to spam anyone. Also, let me know if
> you want me add you to my standard list of people to cc in my clock
> patches.
>
> drivers/clk/clk.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 09b9112..f5b9d3c 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -903,7 +903,7 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
> if (rate == clk->rate)
> goto out;
>
> - if ((clk->flags & CLK_SET_RATE_GATE) && __clk_is_enabled(clk)) {
> + if ((clk->flags & CLK_SET_RATE_GATE) && clk->prepare_count) {
> ret = -EBUSY;
> goto out;
> }
> --
> 1.7.8.3
>
> Sent by an employee of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
On Tue, May 15, 2012 at 5:25 PM, Richard Zhao
<[email protected]> wrote:
> On Tue, May 15, 2012 at 01:43:42PM -0700, Saravana Kannan wrote:
>> The clk_set_rate() code shouldn't check the clock's enable count when
>> validating CLK_SET_RATE_GATE flag since the enable count could change after
>> the validation. Similar to clk_set_parent(), it should instead check the
>> prepare count. The prepare count should go to zero only when the end user
>> expects the clock to not be enabled in the future. Since the code already
>> grabs the prepare count before validation, it's not possible for prepare
>> count to change after validation and by association not possible for a well
>> behaving end user to enable the clock while the set rate is in progress.
>>
>> Signed-off-by: Saravana Kannan <[email protected]>
> Reviewed-by: Richard Zhao <[email protected]>
Looks good to me. I'll take into clk-next for one final pull request
to arm-soc.
Thanks,
Mike
On Tue, May 15, 2012 at 1:09 PM, Saravana Kannan <[email protected]> wrote:
> On 05/15/2012 01:00 PM, Sascha Hauer wrote:
>>
>> On Tue, May 15, 2012 at 12:51:06PM -0700, Saravana Kannan wrote:
>>>>>
>>>>> ? ? ? ?ret = clk->ops->set_parent(clk->hw, i);
>>>>
>>>>
>>>> You call ->set_parent while holding a spinlock. This won't work with i2c
>>>> clocks.
>>>
>>>
>>> I did account for that. I explained it in the commit text. Please
>>> let me know if any part of that is not clear or is not correct.
>>>
>>
>> I missed this part in the commit log. I have no idea whether we can live
>> with this limitation though.
>>
>> Sascha
>>
>
> It's not really an artificial limitation of the patch. This has to be
> enforced if the clock is to be managed correctly while allowing .set_parent
> to NOT be atomic.
>
> There is no way to guarantee that the enable/disable is properly propagated
> to the parent clock if we can't guarantee mutual exclusion between changing
> parents and calling enable/disable.
>
We know for sure that __clk_enable was propagated since it won't
return until it is done. The bigger issue here is the inherent
prepare_lock/enable_lock raciness, which this patch does not solve.
Your approach above is like putting a band-aid gunshot wound :-)
This change essentially forces clocks with sleeping .set_parent
callbacks to be gated during clk_set_parent or else cause a big WARN
and dump_stack. What is really needed is a way to synchronize all of
the clock operations and drop these race conditions. Until that
problem is solved OR until it is proven impossible to fix with the
API's given to us I won't be taking this patch. It is invalid for the
set of clocks that don't set the CLK_SET_PARENT_GATE flag AND have
.set_parent callbacks which might sleep.
> Since we can't do mutual exclusion be using spinlock (since .set_parent is
> NOT atomic for these clocks), then only other way of ensuring mutual
> exclusion is to force an unprepare and then mutually exclude a prepare while
> changing the parent. This by association (can't enable unprepared clock)
> mutually excludes the changing of parent and calling enable/disable.
Right, but this is a workaround to a larger endemic problem. I
completely get what you're trying to do but this fix isn't enough.
Thanks,
Mike
On Fri, May 11, 2012 at 9:59 PM, Saravana Kannan <[email protected]> wrote:
> Since clocks with CLK_SET_PARENT_GATE flag only allow clk_set_parent() on
> unprepared clocks and calling clk_enable() on an unprepared clock would be
> violating the clock API usage model, allow set_parent() ops to be sleepable
> for clocks which have the CLK_SET_PARENT_GATE flag. Putting it another way,
> if a clock's parent can't be switched without sleeping, then by definition
> the parent can't be switched while it's prepared (CLK_SET_PARENT_GATE).
>
...
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index e5d5dc1..09b9112 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1059,7 +1059,7 @@ static int __clk_set_parent(struct clk *clk, struct clk *parent)
> ?{
> ? ? ? ?struct clk *old_parent;
> ? ? ? ?unsigned long flags;
> - ? ? ? int ret = -EINVAL;
> + ? ? ? int ret;
> ? ? ? ?u8 i;
>
> ? ? ? ?old_parent = clk->parent;
> @@ -1083,7 +1083,13 @@ static int __clk_set_parent(struct clk *clk, struct clk *parent)
> ? ? ? ?if (i == clk->num_parents) {
> ? ? ? ? ? ? ? ?pr_debug("%s: clock %s is not a possible parent of clock %s\n",
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?__func__, parent->name, clk->name);
> - ? ? ? ? ? ? ? goto out;
> + ? ? ? ? ? ? ? return -EINVAL;
> + ? ? ? }
> +
> + ? ? ? if (clk->flags & CLK_SET_PARENT_GATE) {
> + ? ? ? ? ? ? ? ret = clk->ops->set_parent(clk->hw, i);
> + ? ? ? ? ? ? ? clk->parent = parent;
> + ? ? ? ? ? ? ? return ret;
> ? ? ? ?}
>
> ? ? ? ?/* migrate prepare and enable */
The above hunk is a good change since it fixes a bug where we
prepare/enable the parent clocks around the .set_parent callback, even
when CLK_SET_PARENT_GATE is set. It should be a separate patch.
Thanks,
Mike
On Wed, May 16, 2012 at 8:00 AM, Turquette, Mike <[email protected]> wrote:
> The above hunk is a good change since it fixes a bug where we
> prepare/enable the parent clocks around the .set_parent callback, even
> when CLK_SET_PARENT_GATE is set. ?It should be a separate patch.
This makes me think that maybe some of these clk fixes really
need to add Cc: [email protected] so they get into 3.4 as well.
Not that there are plenty of in-kernel users, but since people may
be basing development off the 3.4 tree, it would be good to have
fixes propagate there.
Linus Walleij
> On Tue, May 15, 2012 at 1:09 PM, Saravana Kannan <[email protected]>
> wrote:
>> On 05/15/2012 01:00 PM, Sascha Hauer wrote:
>>>
>>> On Tue, May 15, 2012 at 12:51:06PM -0700, Saravana Kannan wrote:
>>>>>>
>>>>>> ? ? ? ?ret = clk->ops->set_parent(clk->hw, i);
>>>>>
>>>>>
>>>>> You call ->set_parent while holding a spinlock. This won't work with
>>>>> i2c
>>>>> clocks.
>>>>
>>>>
>>>> I did account for that. I explained it in the commit text. Please
>>>> let me know if any part of that is not clear or is not correct.
>>>>
>>>
>>> I missed this part in the commit log. I have no idea whether we can
>>> live
>>> with this limitation though.
>>>
>>> Sascha
>>>
>>
>> It's not really an artificial limitation of the patch. This has to be
>> enforced if the clock is to be managed correctly while allowing
>> .set_parent
>> to NOT be atomic.
>>
>> There is no way to guarantee that the enable/disable is properly
>> propagated
>> to the parent clock if we can't guarantee mutual exclusion between
>> changing
>> parents and calling enable/disable.
>>
>
> We know for sure that __clk_enable was propagated since it won't
> return until it is done. The bigger issue here is the inherent
> prepare_lock/enable_lock raciness, which this patch does not solve.
> Your approach above is like putting a band-aid gunshot wound :-)
No, I'm not trying to fix a race that's purely between prepare and enable.
The operation of updating the HW state (calling .set_parent) and the
operation of updating the SW state (setting clk->parent = new parent) is
not atomic with respect to other code that cares about the HW/SW state
(enable/disable). This would be similar to holding the enable spinlock
around incrementing the enable count but not holding it while calling
.enable on the clock -- that is just plain wrong.
If you still claim that's this is nothing more than prepare/enable race,
then why are we implementing a half-baked enable/disable propagation up
the parents? In it's current state, it's never guaranteed to be correct
for ANY clock. Why give an illusion of correctness? Just don't do it --
that will make the common clock framework less useful, but it's better
than something that pretends to be correct.
Also, just because we pushed the enforcement of correctness between
clk_prepare() and clk_enable() to the end user doesn't mean we should
throw up our arms and push all the correctness enforcement to the end
user. The API should try to limit the amount of burden of correctness that
we put on the end user. The only thing we should avoid is writing code
that's correct for the end user only under some circumstances.
It's relatively easy to implement device driver code that ensures that
enable is only called on a prepared clock (since we have ref counting and
it's just a matter of matching enables with prepares). But it would be
much harder to implement useful and power efficient device driver code
that guarantees that enable/set parent is mutually exclusive. You also
need to keep in mind the comment I made in the original email about set
rates that need reparenting. set parent is just another way of setting the
rate and vice-versa (in many cases). This stance of yours means that any
clock that needs to reparent during a set rate while the clock is enabled
can never be implemented correctly.
A very simple example is a device driver that's trying to do power
management by enabling/disabling clocks in interrupt context and also
scaling the clock based on load on the device. Those drivers will fail
horribly if the set rate needs reparenting.
> What is really needed is a way to synchronize all of
> the clock operations and drop these race conditions. Until that
> problem is solved OR until it is proven impossible to fix with the
> API's given to us I won't be taking this patch. It is invalid for the
> set of clocks that don't set the CLK_SET_PARENT_GATE flag AND have
> .set_parent callbacks which might sleep.
This is clearly the opposite of the direction we (the community) decided
to go with the design of the clock APIs -- clk_prepare/clk_enable split.
If we need to question that, that should be a separate discussion and not
part of a discussion where we try to fix issues in the implementation of
those APIs. There is not much to prove here either -- you just can't
synchronize between critical sections where some are protected by mutex
and others by spinlocks. Giving this is a reason to not pick up patches
that fix bugs is not well justified. Again, I'm not saying your question
is/isn't justified, just that it's a separate discussion.
> It is invalid for the
> set of clocks that don't set the CLK_SET_PARENT_GATE flag AND have
> .set_parent callbacks which might sleep.
Sure, but how does it affect the decision of whether we need to fix a race
condition? Any clock platform driver that implements this incorrectly can
be easily caught in testing or code review. Every single call to that set
parent is going to spew a warning.
Thanks,
Saravana
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
On Tue, May 15, 2012 at 08:20:44PM +0200, Saravana Kannan wrote:
> On 05/11/2012 09:59 PM, Saravana Kannan wrote:
> > Without this patch, the following race conditions are possible.
> >
> > Race condition 1:
> > * clk-A has two parents - clk-X and clk-Y.
> > * All three are disabled and clk-X is current parent.
> > * Thread A: clk_set_parent(clk-A, clk-Y).
> > * Thread A:<snip execution flow>
> > * Thread A: Grabs enable lock.
> > * Thread A: Sees enable count of clk-A is 0, so doesn't enable clk-Y.
> > * Thread A: Releases enable lock.
> > * Thread B: Calls clk_enable(clk-A), which in turn enables clk-X.
> > * Thread A: Switches clk-A's parent to clk-Y in hardware.
> >
> > clk-A is now enabled in software, but not clocking in hardware.
> >
> > Race condition 2:
> > * clk-A has two parents - clk-X and clk-Y.
> > * All three are disabled and clk-X is current parent.
> > * Thread A: clk_set_parent(clk-A, clk-Y).
> > * Thread A:<snip execution flow>
> > * Thread A: Switches parent in hardware to clk-Y.
> > * Thread A: Grabs enable lock.
> > * Thread A: Sees enable count of clk-A is 0, so doesn't disable clk-X.
> > * Thread A: Releases enable lock.
> > * Thread B: Calls clk_enable(clk-A)
> > * Thread B: Software state still says parent is clk-X.
> > * Thread B: So, enables clk-X and then itself.
> > * Thread A: Updates parent in software state to clk-Y.
> >
This looks correct to me. Is there any usecase where enabling/disabling a
clock would require sleeping but changing the parent would not?
Cheers,
Peter.
On Tue, May 22, 2012 at 6:58 AM, Peter De Schrijver
<[email protected]> wrote:
> On Tue, May 15, 2012 at 08:20:44PM +0200, Saravana Kannan wrote:
>> On 05/11/2012 09:59 PM, Saravana Kannan wrote:
>> > Without this patch, the following race conditions are possible.
>> >
>> > Race condition 1:
>> > * clk-A has two parents - clk-X and clk-Y.
>> > * All three are disabled and clk-X is current parent.
>> > * Thread A: clk_set_parent(clk-A, clk-Y).
>> > * Thread A:<snip execution flow>
>> > * Thread A: Grabs enable lock.
>> > * Thread A: Sees enable count of clk-A is 0, so doesn't enable clk-Y.
>> > * Thread A: Releases enable lock.
>> > * Thread B: Calls clk_enable(clk-A), which in turn enables clk-X.
>> > * Thread A: Switches clk-A's parent to clk-Y in hardware.
>> >
>> > clk-A is now enabled in software, but not clocking in hardware.
>> >
>> > Race condition 2:
>> > * clk-A has two parents - clk-X and clk-Y.
>> > * All three are disabled and clk-X is current parent.
>> > * Thread A: clk_set_parent(clk-A, clk-Y).
>> > * Thread A:<snip execution flow>
>> > * Thread A: Switches parent in hardware to clk-Y.
>> > * Thread A: Grabs enable lock.
>> > * Thread A: Sees enable count of clk-A is 0, so doesn't disable clk-X.
>> > * Thread A: Releases enable lock.
>> > * Thread B: Calls clk_enable(clk-A)
>> > * Thread B: Software state still says parent is clk-X.
>> > * Thread B: So, enables clk-X and then itself.
>> > * Thread A: Updates parent in software state to clk-Y.
>> >
>
> This looks correct to me. Is there any usecase where enabling/disabling a
> clock would require sleeping but changing the parent would not?
>
clk_enable & clk_disable must never sleep. clk_prepare and
clk_unprepare may sleep.
This patch would require the WM831x clock driver to set
CLK_SET_PARENT_GATE for the clkout mux. The latest version of this
driver sent out by Mark does not do this and I imagine that is because
the hardware has no requirement for the mux clock to gate while
switching parents. Holding the spinlock across the .set_parent
callback breaks this driver since .set_parent results in an i2c
transaction.
Mark, do you have an opinion on the requirements that this patch
imposes on your driver?
Regards,
Mike
> Cheers,
>
> Peter.
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, May 22, 2012 at 08:06:45PM +0200, Turquette, Mike wrote:
> On Tue, May 22, 2012 at 6:58 AM, Peter De Schrijver
> <[email protected]> wrote:
> > On Tue, May 15, 2012 at 08:20:44PM +0200, Saravana Kannan wrote:
> >> On 05/11/2012 09:59 PM, Saravana Kannan wrote:
> >> > Without this patch, the following race conditions are possible.
> >> >
> >> > Race condition 1:
> >> > * clk-A has two parents - clk-X and clk-Y.
> >> > * All three are disabled and clk-X is current parent.
> >> > * Thread A: clk_set_parent(clk-A, clk-Y).
> >> > * Thread A:<snip execution flow>
> >> > * Thread A: Grabs enable lock.
> >> > * Thread A: Sees enable count of clk-A is 0, so doesn't enable clk-Y.
> >> > * Thread A: Releases enable lock.
> >> > * Thread B: Calls clk_enable(clk-A), which in turn enables clk-X.
> >> > * Thread A: Switches clk-A's parent to clk-Y in hardware.
> >> >
> >> > clk-A is now enabled in software, but not clocking in hardware.
> >> >
> >> > Race condition 2:
> >> > * clk-A has two parents - clk-X and clk-Y.
> >> > * All three are disabled and clk-X is current parent.
> >> > * Thread A: clk_set_parent(clk-A, clk-Y).
> >> > * Thread A:<snip execution flow>
> >> > * Thread A: Switches parent in hardware to clk-Y.
> >> > * Thread A: Grabs enable lock.
> >> > * Thread A: Sees enable count of clk-A is 0, so doesn't disable clk-X.
> >> > * Thread A: Releases enable lock.
> >> > * Thread B: Calls clk_enable(clk-A)
> >> > * Thread B: Software state still says parent is clk-X.
> >> > * Thread B: So, enables clk-X and then itself.
> >> > * Thread A: Updates parent in software state to clk-Y.
> >> >
> >
> > This looks correct to me. Is there any usecase where enabling/disabling a
> > clock would require sleeping but changing the parent would not?
> >
>
> clk_enable & clk_disable must never sleep. clk_prepare and
> clk_unprepare may sleep.
>
In that case the clock is actually enabled in clk_prepare and disabled in
clk_unprepare I guess (and clk_enable/clk_disable are dummy functions)?
What I'm trying to say is that I don't think there are clocks which can be
enabled/disabled using non blocking operations, but where a parent change
would require a blocking operation.
Cheers,
Peter.
On 05/23/2012 02:16 AM, Peter De Schrijver wrote:
> On Tue, May 22, 2012 at 08:06:45PM +0200, Turquette, Mike wrote:
>> On Tue, May 22, 2012 at 6:58 AM, Peter De Schrijver
>> <[email protected]> wrote:
>>> On Tue, May 15, 2012 at 08:20:44PM +0200, Saravana Kannan wrote:
>>>> On 05/11/2012 09:59 PM, Saravana Kannan wrote:
>>>>> Without this patch, the following race conditions are possible.
>>>>>
>>>>> Race condition 1:
>>>>> * clk-A has two parents - clk-X and clk-Y.
>>>>> * All three are disabled and clk-X is current parent.
>>>>> * Thread A: clk_set_parent(clk-A, clk-Y).
>>>>> * Thread A:<snip execution flow>
>>>>> * Thread A: Grabs enable lock.
>>>>> * Thread A: Sees enable count of clk-A is 0, so doesn't enable clk-Y.
>>>>> * Thread A: Releases enable lock.
>>>>> * Thread B: Calls clk_enable(clk-A), which in turn enables clk-X.
>>>>> * Thread A: Switches clk-A's parent to clk-Y in hardware.
>>>>>
>>>>> clk-A is now enabled in software, but not clocking in hardware.
>>>>>
>>>>> Race condition 2:
>>>>> * clk-A has two parents - clk-X and clk-Y.
>>>>> * All three are disabled and clk-X is current parent.
>>>>> * Thread A: clk_set_parent(clk-A, clk-Y).
>>>>> * Thread A:<snip execution flow>
>>>>> * Thread A: Switches parent in hardware to clk-Y.
>>>>> * Thread A: Grabs enable lock.
>>>>> * Thread A: Sees enable count of clk-A is 0, so doesn't disable clk-X.
>>>>> * Thread A: Releases enable lock.
>>>>> * Thread B: Calls clk_enable(clk-A)
>>>>> * Thread B: Software state still says parent is clk-X.
>>>>> * Thread B: So, enables clk-X and then itself.
>>>>> * Thread A: Updates parent in software state to clk-Y.
>>>>>
>>>
>>> This looks correct to me. Is there any usecase where enabling/disabling a
>>> clock would require sleeping but changing the parent would not?
>>>
>>
>> clk_enable& clk_disable must never sleep. clk_prepare and
>> clk_unprepare may sleep.
>>
>
> In that case the clock is actually enabled in clk_prepare and disabled in
> clk_unprepare I guess (and clk_enable/clk_disable are dummy functions)?
>
> What I'm trying to say is that I don't think there are clocks which can be
> enabled/disabled using non blocking operations, but where a parent change
> would require a blocking operation.
>
> Cheers,
>
> Peter.
Mark, Shawn, Russell,
Can you guys please respond? I'm surprised that no one seem to care
about fixing race conditions between clk_set_parent/clk_set_rate() and
clk_enable() that will result in incorrect enable count propagation and
have the SW get out of sync with HW.
If we absolutely need to support clocks that where the ops->set_parent()
is not atomic and can't go with the CLK_SET_PARENT_GATE option, then
maybe we can add a "I promise the consumers of this clock won't call
clk_set_parent() and clk_enable() in a racy way" clock flag
(CLK_IGNORE_PARENT_ENABLE_RACE). Yes, it would be a hack for such
clocks, but that's still better than leaving a gaping hole for all the
clocks.
-Saravana
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.