All of lore.kernel.org
 help / color / mirror / Atom feed
From: Stephen Boyd <sboyd@kernel.org>
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,
	mturquette@baylibre.com, heiko@sntech.de, aisheng.dong@nxp.com,
	mchehab+samsung@kernel.org, corbet@lwn.net, jbrunet@baylibre.com,
	Stephen Boyd <sboyd@codeaurora.org>,
	Derek Basehore <dbasehore@chromium.org>
Subject: Re: [PATCH v2 1/6] clk: Remove recursion in clk_core_{prepare,enable}()
Date: Tue, 05 Mar 2019 10:49:35 -0800	[thread overview]
Message-ID: <155181177527.20095.15680753964583935841@swboyd.mtv.corp.google.com> (raw)
In-Reply-To: <20190305044936.22267-2-dbasehore@chromium.org>

Quoting Derek Basehore (2019-03-04 20:49:31)
> 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. This also unroll the recursion in unprepare,disable which can
> just be done in the order of walking up the clk tree.
> 
> 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.
> 
> Modified verison of https://lore.kernel.org/patchwork/patch/814369/
> -Fixed kernel warning
> -unrolled recursion in unprepare/disable too
> 
> Cc: Jerome Brunet <jbrunet@baylibre.com>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> ---

From the original post:

"I have some vague fear that this may not work if a clk op is framework 
reentrant and attemps to call consumer clk APIs from within the clk ops.
If the reentrant call tries to add a clk that's already in the list then
we'll corrupt the list. Ugh."

Do we have this sort of problem here? Or are you certain that we don't
have clks that prepare or enable something that is already in the
process of being prepared or enabled?


WARNING: multiple messages have this Message-ID (diff)
From: Stephen Boyd <sboyd@kernel.org>
To: linux-kernel@vger.kernel.org
Cc: aisheng.dong@nxp.com, Derek Basehore <dbasehore@chromium.org>,
	heiko@sntech.de, linux-doc@vger.kernel.org,
	mturquette@baylibre.com, corbet@lwn.net,
	Stephen Boyd <sboyd@codeaurora.org>,
	linux-rockchip@lists.infradead.org, mchehab+samsung@kernel.org,
	linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	jbrunet@baylibre.com
Subject: Re: [PATCH v2 1/6] clk: Remove recursion in clk_core_{prepare, enable}()
Date: Tue, 05 Mar 2019 10:49:35 -0800	[thread overview]
Message-ID: <155181177527.20095.15680753964583935841@swboyd.mtv.corp.google.com> (raw)
In-Reply-To: <20190305044936.22267-2-dbasehore@chromium.org>

Quoting Derek Basehore (2019-03-04 20:49:31)
> 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. This also unroll the recursion in unprepare,disable which can
> just be done in the order of walking up the clk tree.
> 
> 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.
> 
> Modified verison of https://lore.kernel.org/patchwork/patch/814369/
> -Fixed kernel warning
> -unrolled recursion in unprepare/disable too
> 
> Cc: Jerome Brunet <jbrunet@baylibre.com>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> ---

From the original post:

"I have some vague fear that this may not work if a clk op is framework 
reentrant and attemps to call consumer clk APIs from within the clk ops.
If the reentrant call tries to add a clk that's already in the list then
we'll corrupt the list. Ugh."

Do we have this sort of problem here? Or are you certain that we don't
have clks that prepare or enable something that is already in the
process of being prepared or enabled?

WARNING: multiple messages have this Message-ID (diff)
From: Stephen Boyd <sboyd@kernel.org>
To: Derek Basehore <dbasehore@chromium.org>, linux-kernel@vger.kernel.org
Cc: aisheng.dong@nxp.com, Derek Basehore <dbasehore@chromium.org>,
	heiko@sntech.de, linux-doc@vger.kernel.org,
	mturquette@baylibre.com, corbet@lwn.net,
	Stephen Boyd <sboyd@codeaurora.org>,
	linux-rockchip@lists.infradead.org, mchehab+samsung@kernel.org,
	linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	jbrunet@baylibre.com
Subject: Re: [PATCH v2 1/6] clk: Remove recursion in clk_core_{prepare, enable}()
Date: Tue, 05 Mar 2019 10:49:35 -0800	[thread overview]
Message-ID: <155181177527.20095.15680753964583935841@swboyd.mtv.corp.google.com> (raw)
In-Reply-To: <20190305044936.22267-2-dbasehore@chromium.org>

Quoting Derek Basehore (2019-03-04 20:49:31)
> 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. This also unroll the recursion in unprepare,disable which can
> just be done in the order of walking up the clk tree.
> 
> 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.
> 
> Modified verison of https://lore.kernel.org/patchwork/patch/814369/
> -Fixed kernel warning
> -unrolled recursion in unprepare/disable too
> 
> Cc: Jerome Brunet <jbrunet@baylibre.com>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> ---

From the original post:

"I have some vague fear that this may not work if a clk op is framework 
reentrant and attemps to call consumer clk APIs from within the clk ops.
If the reentrant call tries to add a clk that's already in the list then
we'll corrupt the list. Ugh."

Do we have this sort of problem here? Or are you certain that we don't
have clks that prepare or enable something that is already in the
process of being prepared or enabled?


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2019-03-05 18:49 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-05  4:49 [PATCH v2 0/6] Coordinated Clks Derek Basehore
2019-03-05  4:49 ` Derek Basehore
2019-03-05  4:49 ` [PATCH v2 1/6] clk: Remove recursion in clk_core_{prepare,enable}() Derek Basehore
2019-03-05  4:49   ` Derek Basehore
2019-03-05 18:49   ` Stephen Boyd [this message]
2019-03-05 18:49     ` [PATCH v2 1/6] clk: Remove recursion in clk_core_{prepare, enable}() Stephen Boyd
2019-03-05 18:49     ` Stephen Boyd
2019-03-06  1:35     ` [PATCH v2 1/6] clk: Remove recursion in clk_core_{prepare,enable}() dbasehore .
2019-03-06  1:35       ` [PATCH v2 1/6] clk: Remove recursion in clk_core_{prepare, enable}() dbasehore .
2019-03-06  4:11       ` [PATCH v2 1/6] clk: Remove recursion in clk_core_{prepare,enable}() dbasehore .
2019-03-06  4:11         ` [PATCH v2 1/6] clk: Remove recursion in clk_core_{prepare, enable}() dbasehore .
2019-03-06  4:11         ` dbasehore .
     [not found]         ` <CAGAzgsp0fWbH1f7gRKvhTotvdHMAL8gWw1bTKpVHfW9hJddXAw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2019-03-06 21:16           ` Stephen Boyd
2019-03-06 21:16         ` Stephen Boyd
2019-03-05  4:49 ` [PATCH v2 2/6] clk: fix clk_calc_subtree compute duplications Derek Basehore
2019-03-05  4:49   ` Derek Basehore
2019-03-05  4:49   ` Derek Basehore
2019-03-05  4:49 ` [PATCH v3 3/6] clk: change rates via list iteration Derek Basehore
2019-03-05  4:49   ` Derek Basehore
2019-03-09  0:07   ` dbasehore .
2019-03-09  0:07     ` dbasehore .
2019-03-05  4:49 ` [PATCH v2 4/6] clk: add coordinated clk changes support Derek Basehore
2019-03-05  4:49   ` Derek Basehore
2019-03-05  4:49 ` [PATCH v2 5/6] docs: driver-api: add pre_rate_req to clk documentation Derek Basehore
2019-03-05  4:49   ` Derek Basehore
2019-03-05  4:49 ` [PATCH v2 6/6] clk: rockchip: use pre_rate_req for cpuclk Derek Basehore
2019-03-05  4:49   ` Derek Basehore

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=155181177527.20095.15680753964583935841@swboyd.mtv.corp.google.com \
    --to=sboyd@kernel.org \
    --cc=aisheng.dong@nxp.com \
    --cc=corbet@lwn.net \
    --cc=dbasehore@chromium.org \
    --cc=heiko@sntech.de \
    --cc=jbrunet@baylibre.com \
    --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 \
    /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: link
Be 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.