| Message ID | 20181013005504.46399-4-briannorris@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] ath10k: snoc: remove 'wcn3990' from generic resource handling | expand |
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
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.
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)
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(-)