From: Jerome Brunet <jbrunet@baylibre.com> To: Derek Basehore <dbasehore@chromium.org>, linux-kernel@vger.kernel.org Cc: linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-rockchip@lists.infradead.org, linux-doc@vger.kernel.org, sboyd@kernel.org, mturquette@baylibre.com, heiko@sntech.de, aisheng.dong@nxp.com, mchehab+samsung@kernel.org, corbet@lwn.net, Stephen Boyd <sboyd@codeaurora.org> Subject: Re: [PATCH 1/6] clk: Remove recursion in clk_core_{prepare,enable}() Date: Wed, 24 Oct 2018 11:51:22 +0200 [thread overview] Message-ID: <264adf2a81bcd602f2a58e4a46c3273cd7c77ca2.camel@baylibre.com> (raw) In-Reply-To: <20181024013132.115907-2-dbasehore@chromium.org> On Tue, 2018-10-23 at 18:31 -0700, Derek Basehore wrote: > From: Stephen Boyd <sboyd@codeaurora.org> > > Enabling and preparing clocks can be written quite naturally with > recursion. We start at some point in the tree and recurse up the > tree to find the oldest parent clk that needs to be enabled or > prepared. Then we enable/prepare and return to the caller, going > back to the clk we started at and enabling/preparing along the > way. > > The problem is recursion isn't great for kernel code where we > have a limited stack size. Furthermore, we may be calling this > code inside clk_set_rate() which also has recursion in it, so > we're really not looking good if we encounter a tall clk tree. > > Let's create a stack instead by looping over the parent chain and > collecting clks of interest. Then the enable/prepare becomes as > simple as iterating over that list and calling enable. Hi Derek, What about unprepare() and disable() ? This patch removes the recursion from the enable path but keeps it for the disable path ... this is very odd. Assuming doing so works, It certainly makes CCF a lot harder to understand. What about clock protection which essentially works on the same model as prepare and enable ? Overall, this change does not look like something that should be merged as it is. If you were just seeking comments, you should add the "RFC" tag to your series. Jerome. > > Cc: Jerome Brunet <jbrunet@baylibre.com> If you don't mind, I would prefer to get the whole series next time. It helps to get the context. > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> > Signed-off-by: Derek Basehore <dbasehore@chromium.org> > --- > drivers/clk/clk.c | 113 ++++++++++++++++++++++++++-------------------- > 1 file changed, 64 insertions(+), 49 deletions(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index af011974d4ec..95d818f5edb2 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -71,6 +71,8 @@ struct clk_core { > struct hlist_head children; > struct hlist_node child_node; > struct hlist_head clks; > + struct list_head prepare_list; > + struct list_head enable_list; > unsigned int notifier_count; > #ifdef CONFIG_DEBUG_FS > struct dentry *dentry; > @@ -740,49 +742,48 @@ EXPORT_SYMBOL_GPL(clk_unprepare); > static int clk_core_prepare(struct clk_core *core) > { > int ret = 0; > + struct clk_core *tmp, *parent; > + LIST_HEAD(head); > > lockdep_assert_held(&prepare_lock); > > - if (!core) > - return 0; > + while (core) { > + list_add(&core->prepare_list, &head); > + /* Stop once we see a clk that is already prepared */ > + if (core->prepare_count) > + break; > + core = core->parent; > + } > > - if (core->prepare_count == 0) { > - ret = clk_pm_runtime_get(core); > - if (ret) > - return ret; > + list_for_each_entry_safe(core, tmp, &head, prepare_list) { > + list_del_init(&core->prepare_list); Is there any point in removing it from the list ? Maybe I missed it but it does not seems useful. Without this, we could use list_for_each_entry() > > - ret = clk_core_prepare(core->parent); > - if (ret) > - goto runtime_put; > + if (core->prepare_count == 0) { Should we really check the count here ? You are not checking the count when the put() counterpart is called below. Since PM runtime has ref counting as well, either way would work I guess ... but we shall be consistent > + ret = clk_pm_runtime_get(core); > + if (ret) > + goto err; > > - trace_clk_prepare(core); > + trace_clk_prepare(core); > > - if (core->ops->prepare) > - ret = core->ops->prepare(core->hw); > + if (core->ops->prepare) > + ret = core->ops->prepare(core->hw); > > - trace_clk_prepare_complete(core); > + trace_clk_prepare_complete(core); > > - if (ret) > - goto unprepare; > + if (ret) { > + clk_pm_runtime_put(core); > + goto err; > + } > + } > + core->prepare_count++; > } > > - core->prepare_count++; > - > - /* > - * CLK_SET_RATE_GATE is a special case of clock protection > - * Instead of a consumer claiming exclusive rate control, it is > - * actually the provider which prevents any consumer from making any > - * operation which could result in a rate change or rate glitch while > - * the clock is prepared. > - */ > - if (core->flags & CLK_SET_RATE_GATE) > - clk_core_rate_protect(core); This gets removed without anything replacing it. is CLK_SET_RATE_GATE and clock protection support dropped after this change ? > - > return 0; > -unprepare: > - clk_core_unprepare(core->parent); > -runtime_put: > - clk_pm_runtime_put(core); > +err: > + parent = core->parent; > + list_for_each_entry_safe_continue(core, tmp, &head, prepare_list) > + list_del_init(&core->prepare_list); > + clk_core_unprepare(parent); If you get here because of failure clk_pm_runtime_get(), you will unprepare a clock which may have not been prepared first Overall the rework of error exit path does not seem right (or necessary) > return ret; > } > > @@ -878,37 +879,49 @@ EXPORT_SYMBOL_GPL(clk_disable); > static int clk_core_enable(struct clk_core *core) > { > int ret = 0; > + struct clk_core *tmp, *parent; > + LIST_HEAD(head); > > lockdep_assert_held(&enable_lock); > > - if (!core) > - return 0; > - > - if (WARN(core->prepare_count == 0, > - "Enabling unprepared %s\n", core->name)) > - return -ESHUTDOWN; > + while (core) { > + list_add(&core->enable_list, &head); > + /* Stop once we see a clk that is already enabled */ > + if (core->enable_count) > + break; > + core = core->parent; > + } > > - if (core->enable_count == 0) { > - ret = clk_core_enable(core->parent); > + list_for_each_entry_safe(core, tmp, &head, enable_list) { > + list_del_init(&core->enable_list); > > - if (ret) > - return ret; > + if (WARN_ON(core->prepare_count == 0)) { > + ret = -ESHUTDOWN; > + goto err; > + } > > - trace_clk_enable_rcuidle(core); > + if (core->enable_count == 0) { > + trace_clk_enable_rcuidle(core); > > - if (core->ops->enable) > - ret = core->ops->enable(core->hw); > + if (core->ops->enable) > + ret = core->ops->enable(core->hw); > > - trace_clk_enable_complete_rcuidle(core); > + trace_clk_enable_complete_rcuidle(core); > > - if (ret) { > - clk_core_disable(core->parent); > - return ret; > + if (ret) > + goto err; > } > + > + core->enable_count++; > } > > - core->enable_count++; > return 0; > +err: > + parent = core->parent; > + list_for_each_entry_safe_continue(core, tmp, &head, enable_list) > + list_del_init(&core->enable_list); > + clk_core_disable(parent); > + return ret; > } > > static int clk_core_enable_lock(struct clk_core *core) > @@ -3281,6 +3294,8 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw) > core->num_parents = hw->init->num_parents; > core->min_rate = 0; > core->max_rate = ULONG_MAX; > + INIT_LIST_HEAD(&core->prepare_list); > + INIT_LIST_HEAD(&core->enable_list); > hw->core = core; > > /* allocate local copy in case parent_names is __initdata */
WARNING: multiple messages have this Message-ID (diff)
From: jbrunet@baylibre.com (Jerome Brunet) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 1/6] clk: Remove recursion in clk_core_{prepare,enable}() Date: Wed, 24 Oct 2018 11:51:22 +0200 [thread overview] Message-ID: <264adf2a81bcd602f2a58e4a46c3273cd7c77ca2.camel@baylibre.com> (raw) In-Reply-To: <20181024013132.115907-2-dbasehore@chromium.org> On Tue, 2018-10-23 at 18:31 -0700, Derek Basehore wrote: > From: Stephen Boyd <sboyd@codeaurora.org> > > Enabling and preparing clocks can be written quite naturally with > recursion. We start at some point in the tree and recurse up the > tree to find the oldest parent clk that needs to be enabled or > prepared. Then we enable/prepare and return to the caller, going > back to the clk we started at and enabling/preparing along the > way. > > The problem is recursion isn't great for kernel code where we > have a limited stack size. Furthermore, we may be calling this > code inside clk_set_rate() which also has recursion in it, so > we're really not looking good if we encounter a tall clk tree. > > Let's create a stack instead by looping over the parent chain and > collecting clks of interest. Then the enable/prepare becomes as > simple as iterating over that list and calling enable. Hi Derek, What about unprepare() and disable() ? This patch removes the recursion from the enable path but keeps it for the disable path ... this is very odd. Assuming doing so works, It certainly makes CCF a lot harder to understand. What about clock protection which essentially works on the same model as prepare and enable ? Overall, this change does not look like something that should be merged as it is. If you were just seeking comments, you should add the "RFC" tag to your series. Jerome. > > Cc: Jerome Brunet <jbrunet@baylibre.com> If you don't mind, I would prefer to get the whole series next time. It helps to get the context. > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> > Signed-off-by: Derek Basehore <dbasehore@chromium.org> > --- > drivers/clk/clk.c | 113 ++++++++++++++++++++++++++-------------------- > 1 file changed, 64 insertions(+), 49 deletions(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index af011974d4ec..95d818f5edb2 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -71,6 +71,8 @@ struct clk_core { > struct hlist_head children; > struct hlist_node child_node; > struct hlist_head clks; > + struct list_head prepare_list; > + struct list_head enable_list; > unsigned int notifier_count; > #ifdef CONFIG_DEBUG_FS > struct dentry *dentry; > @@ -740,49 +742,48 @@ EXPORT_SYMBOL_GPL(clk_unprepare); > static int clk_core_prepare(struct clk_core *core) > { > int ret = 0; > + struct clk_core *tmp, *parent; > + LIST_HEAD(head); > > lockdep_assert_held(&prepare_lock); > > - if (!core) > - return 0; > + while (core) { > + list_add(&core->prepare_list, &head); > + /* Stop once we see a clk that is already prepared */ > + if (core->prepare_count) > + break; > + core = core->parent; > + } > > - if (core->prepare_count == 0) { > - ret = clk_pm_runtime_get(core); > - if (ret) > - return ret; > + list_for_each_entry_safe(core, tmp, &head, prepare_list) { > + list_del_init(&core->prepare_list); Is there any point in removing it from the list ? Maybe I missed it but it does not seems useful. Without this, we could use list_for_each_entry() > > - ret = clk_core_prepare(core->parent); > - if (ret) > - goto runtime_put; > + if (core->prepare_count == 0) { Should we really check the count here ? You are not checking the count when the put() counterpart is called below. Since PM runtime has ref counting as well, either way would work I guess ... but we shall be consistent > + ret = clk_pm_runtime_get(core); > + if (ret) > + goto err; > > - trace_clk_prepare(core); > + trace_clk_prepare(core); > > - if (core->ops->prepare) > - ret = core->ops->prepare(core->hw); > + if (core->ops->prepare) > + ret = core->ops->prepare(core->hw); > > - trace_clk_prepare_complete(core); > + trace_clk_prepare_complete(core); > > - if (ret) > - goto unprepare; > + if (ret) { > + clk_pm_runtime_put(core); > + goto err; > + } > + } > + core->prepare_count++; > } > > - core->prepare_count++; > - > - /* > - * CLK_SET_RATE_GATE is a special case of clock protection > - * Instead of a consumer claiming exclusive rate control, it is > - * actually the provider which prevents any consumer from making any > - * operation which could result in a rate change or rate glitch while > - * the clock is prepared. > - */ > - if (core->flags & CLK_SET_RATE_GATE) > - clk_core_rate_protect(core); This gets removed without anything replacing it. is CLK_SET_RATE_GATE and clock protection support dropped after this change ? > - > return 0; > -unprepare: > - clk_core_unprepare(core->parent); > -runtime_put: > - clk_pm_runtime_put(core); > +err: > + parent = core->parent; > + list_for_each_entry_safe_continue(core, tmp, &head, prepare_list) > + list_del_init(&core->prepare_list); > + clk_core_unprepare(parent); If you get here because of failure clk_pm_runtime_get(), you will unprepare a clock which may have not been prepared first Overall the rework of error exit path does not seem right (or necessary) > return ret; > } > > @@ -878,37 +879,49 @@ EXPORT_SYMBOL_GPL(clk_disable); > static int clk_core_enable(struct clk_core *core) > { > int ret = 0; > + struct clk_core *tmp, *parent; > + LIST_HEAD(head); > > lockdep_assert_held(&enable_lock); > > - if (!core) > - return 0; > - > - if (WARN(core->prepare_count == 0, > - "Enabling unprepared %s\n", core->name)) > - return -ESHUTDOWN; > + while (core) { > + list_add(&core->enable_list, &head); > + /* Stop once we see a clk that is already enabled */ > + if (core->enable_count) > + break; > + core = core->parent; > + } > > - if (core->enable_count == 0) { > - ret = clk_core_enable(core->parent); > + list_for_each_entry_safe(core, tmp, &head, enable_list) { > + list_del_init(&core->enable_list); > > - if (ret) > - return ret; > + if (WARN_ON(core->prepare_count == 0)) { > + ret = -ESHUTDOWN; > + goto err; > + } > > - trace_clk_enable_rcuidle(core); > + if (core->enable_count == 0) { > + trace_clk_enable_rcuidle(core); > > - if (core->ops->enable) > - ret = core->ops->enable(core->hw); > + if (core->ops->enable) > + ret = core->ops->enable(core->hw); > > - trace_clk_enable_complete_rcuidle(core); > + trace_clk_enable_complete_rcuidle(core); > > - if (ret) { > - clk_core_disable(core->parent); > - return ret; > + if (ret) > + goto err; > } > + > + core->enable_count++; > } > > - core->enable_count++; > return 0; > +err: > + parent = core->parent; > + list_for_each_entry_safe_continue(core, tmp, &head, enable_list) > + list_del_init(&core->enable_list); > + clk_core_disable(parent); > + return ret; > } > > static int clk_core_enable_lock(struct clk_core *core) > @@ -3281,6 +3294,8 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw) > core->num_parents = hw->init->num_parents; > core->min_rate = 0; > core->max_rate = ULONG_MAX; > + INIT_LIST_HEAD(&core->prepare_list); > + INIT_LIST_HEAD(&core->enable_list); > hw->core = core; > > /* allocate local copy in case parent_names is __initdata */
next prev parent reply other threads:[~2018-10-24 9:51 UTC|newest] Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-10-24 1:31 [PATCH 0/6] Coordinated Clks Derek Basehore 2018-10-24 1:31 ` Derek Basehore 2018-10-24 1:31 ` Derek Basehore 2018-10-24 1:31 ` [PATCH 1/6] clk: Remove recursion in clk_core_{prepare,enable}() Derek Basehore 2018-10-24 1:31 ` Derek Basehore 2018-10-24 1:31 ` Derek Basehore 2018-10-24 9:51 ` Jerome Brunet [this message] 2018-10-24 9:51 ` Jerome Brunet 2018-10-24 20:15 ` dbasehore . 2018-10-24 20:15 ` dbasehore . 2018-10-24 20:23 ` dbasehore . 2018-10-24 20:23 ` dbasehore . 2018-10-24 20:50 ` dbasehore . 2018-10-24 20:50 ` dbasehore . 2018-10-25 8:57 ` Jerome Brunet 2018-10-25 8:57 ` Jerome Brunet 2018-10-24 20:36 ` dbasehore . 2018-10-24 20:36 ` dbasehore . 2018-10-25 8:12 ` Jerome Brunet 2018-10-25 8:12 ` Jerome Brunet 2018-10-24 13:07 ` Stephen Boyd 2018-10-24 13:07 ` Stephen Boyd 2018-10-24 13:07 ` Stephen Boyd 2018-10-24 20:09 ` dbasehore . 2018-10-24 20:09 ` dbasehore . 2018-10-24 1:31 ` [PATCH 2/6] clk: fix clk_calc_subtree compute duplications Derek Basehore 2018-10-24 1:31 ` Derek Basehore 2018-10-24 1:31 ` Derek Basehore 2018-11-01 2:58 ` dbasehore . 2018-11-01 2:58 ` dbasehore . 2018-10-24 1:31 ` [PATCH 3/6] clk: change rates via list iteration Derek Basehore 2018-10-24 1:31 ` Derek Basehore 2018-10-24 1:31 ` Derek Basehore 2018-10-26 3:29 ` dbasehore . 2018-10-26 3:29 ` dbasehore . 2018-10-24 1:31 ` [PATCH 4/6] clk: add pre clk changes support Derek Basehore 2018-10-24 1:31 ` Derek Basehore 2018-10-24 1:31 ` Derek Basehore 2018-10-24 1:31 ` [PATCH 5/6] docs: driver-api: add pre_rate_req to clk documentation Derek Basehore 2018-10-24 1:31 ` Derek Basehore 2018-10-24 1:31 ` Derek Basehore 2018-10-24 1:31 ` [PATCH 6/6] clk: rockchip: use pre_rate_req for cpuclk Derek Basehore 2018-10-24 1:31 ` Derek Basehore 2018-10-24 1:31 ` Derek Basehore 2018-10-24 4:06 ` dbasehore . 2018-10-24 4:06 ` dbasehore . 2018-12-20 21:15 ` [PATCH 0/6] Coordinated Clks Stephen Boyd 2018-12-20 21:15 ` Stephen Boyd 2018-12-20 21:15 ` Stephen Boyd 2018-12-20 23:20 ` dbasehore . 2018-12-20 23:20 ` dbasehore .
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=264adf2a81bcd602f2a58e4a46c3273cd7c77ca2.camel@baylibre.com \ --to=jbrunet@baylibre.com \ --cc=aisheng.dong@nxp.com \ --cc=corbet@lwn.net \ --cc=dbasehore@chromium.org \ --cc=heiko@sntech.de \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-clk@vger.kernel.org \ --cc=linux-doc@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-rockchip@lists.infradead.org \ --cc=mchehab+samsung@kernel.org \ --cc=mturquette@baylibre.com \ --cc=sboyd@codeaurora.org \ --cc=sboyd@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.