[4/4] ath10k: snoc: fix unbalanced clock error handling
diff mbox series

Message ID 20181013005504.46399-4-briannorris@chromium.org
State New
Headers show
Series
  • [1/4] ath10k: snoc: remove 'wcn3990' from generic resource handling
Related show

Commit Message

Brian Norris Oct. 13, 2018, 12:55 a.m. UTC
Similar to regulator error handling, we should only start tearing down
the 'i - 1' clock when clock 'i' fails to enable. Otherwise, we might
end up with an unbalanced clock, where we never successfully enabled the
clock, but we try to disable it anyway.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/net/wireless/ath/ath10k/snoc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Douglas Anderson Oct. 16, 2018, 11:53 p.m. UTC | #1
Hi,
On Fri, Oct 12, 2018 at 5:55 PM Brian Norris <briannorris@chromium.org> wrote:
>
> Similar to regulator error handling, we should only start tearing down
> the 'i - 1' clock when clock 'i' fails to enable. Otherwise, we might
> end up with an unbalanced clock, where we never successfully enabled the
> clock, but we try to disable it anyway.
>
> Signed-off-by: Brian Norris <briannorris@chromium.org>

Presumably you could have a Fixes tag just to help folks, like:

Fixes: a6a793f98786 ("ath10k: vote for hardware resources for WCN3990")

> ---
>  drivers/net/wireless/ath/ath10k/snoc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/snoc.c b/drivers/net/wireless/ath/ath10k/snoc.c
> index 5a8e87339df2..a835599a8d55 100644
> --- a/drivers/net/wireless/ath/ath10k/snoc.c
> +++ b/drivers/net/wireless/ath/ath10k/snoc.c
> @@ -1470,7 +1470,7 @@ static int ath10k_snoc_clk_init(struct ath10k *ar)
>         return 0;
>
>  err_clock_config:
> -       for (; i >= 0; i--) {
> +       for (i = i - 1; i >= 0; i--) {

I see no problem with this and it's a good / easy to backport fix.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

For extra credit, though, you could change this to not duplicate the
"clk_bulk" APIs and just use 'em.  I already mentioned to someone else
that maybe the clk_bulk APIs could probably be improved to handle
optional clocks, but I don't think anyone has posted a patch for it.
Bonus points if you remember that "NULL" is a valid clock so you
really only need special case code in clk_bulk_get().  :-P


-Doug
Kalle Valo Nov. 6, 2018, 4:14 p.m. UTC | #2
Doug Anderson <dianders@chromium.org> writes:

> Hi,
> On Fri, Oct 12, 2018 at 5:55 PM Brian Norris <briannorris@chromium.org> wrote:
>>
>> Similar to regulator error handling, we should only start tearing down
>> the 'i - 1' clock when clock 'i' fails to enable. Otherwise, we might
>> end up with an unbalanced clock, where we never successfully enabled the
>> clock, but we try to disable it anyway.
>>
>> Signed-off-by: Brian Norris <briannorris@chromium.org>
>
> Presumably you could have a Fixes tag just to help folks, like:
>
> Fixes: a6a793f98786 ("ath10k: vote for hardware resources for WCN3990")

Thanks, I added that in the pending branch.

Patch
diff mbox series

diff --git a/drivers/net/wireless/ath/ath10k/snoc.c b/drivers/net/wireless/ath/ath10k/snoc.c
index 5a8e87339df2..a835599a8d55 100644
--- a/drivers/net/wireless/ath/ath10k/snoc.c
+++ b/drivers/net/wireless/ath/ath10k/snoc.c
@@ -1470,7 +1470,7 @@  static int ath10k_snoc_clk_init(struct ath10k *ar)
 	return 0;
 
 err_clock_config:
-	for (; i >= 0; i--) {
+	for (i = i - 1; i >= 0; i--) {
 		clk_info = &ar_snoc->clk[i];
 
 		if (!clk_info->handle)