From: mturquette@linaro.org (Mike Turquette)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v4] clk: allow reentrant calls into the clk framework
Date: Wed, 27 Mar 2013 07:25:19 -0700 [thread overview]
Message-ID: <20130327142519.4014.82657@quantum> (raw)
In-Reply-To: <alpine.LFD.2.02.1303271141470.22263@ionos>
Quoting Thomas Gleixner (2013-03-27 04:09:17)
> On Wed, 27 Mar 2013, Ulf Hansson wrote:
> > On 27 March 2013 10:55, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > On 27 March 2013 15:10, Thomas Gleixner <tglx@linutronix.de> wrote:
> > >> On Wed, 27 Mar 2013, Mike Turquette wrote:
> > >>
> > >>> Reentrancy into the clock framework from the clk.h api is necessary
> > >>> for clocks that are prepared and unprepared via i2c_transfer (which
> > >>> includes many PMICs and discrete audio chips) as well as for several
> > >>> other use cases.
> > >>
> > >> That explanation sucks.
> > >>
> > >> Why does an i2c clock need reentrancy? Just because it's i2c or what?
> > >
> > > I am noway connected to this development but was just going through
> > > your mail and i think i might know the answer why is this required.
> > >
> > > Consider an example where an external chip has clock controller and has
> > > bits which can be programmed to enable/disable clock. And this chip is
> > > connected via spi/i2c to SoC.
> > >
> > > clk_prepare(peripheral on external chip)
> > > -> i2c_xfer(to write to external chips register)
> > > -> clk_enable(i2c controller)
> > > ->controller-xfer-routine.. and finally we enable clk here...
>
> Which does not explain the whole issue:
>
> clk_prepare() takes the mutex
> clk_enable() takes the spinlock
>
> That works today.
>
> The issue arises, if you need to call clk_prepare(i2c) in the xfer
> routine.
>
The issue arises any time a clk_ops callback calls a function that
unwittingly re-enters the clock framework. I think the easiest example
to understand and perhaps the most common in practice is a clock which
is controlled via an i2c transfer.
Viresh's example makes the mistake of calling
clk_enable(i2c_controller), but it must also call
clk_prepare(i2c_controller) which is missing in the call graph above.
That nested call to clk_prepare is where the reentrancy comes from.
This has nothing to do with the prepare/enable locking split and leaves
that relationship intact.
> > >
> > > Sorry if i am on the wrong side :)
>
> Only slightly :)
>
> > I agree with you Viresh. I guess Mike should update the commit message.
> >
> > I would also like add another reason to why this is needed. For some
> > clks you would like to do pinctrl operations from a clk hw. But since
> > a pinctrl driver likely requires a clk to be prepared|enabled, we run
> > into a clk reentrant issue.
>
> Fair enough. This all wants to go into the changelog, so we can
> understand why we have this business.
>
I'll submit a v5 which I hope will end the pain and suffering this patch
has caused you.
Regards,
Mike
> Thanks,
>
> tglx
WARNING: multiple messages have this Message-ID (diff)
From: Mike Turquette <mturquette@linaro.org>
To: Thomas Gleixner <tglx@linutronix.de>,
Ulf Hansson <ulf.hansson@linaro.org>
Cc: Viresh Kumar <viresh.kumar@linaro.org>,
linaro-kernel@lists.linaro.org, patches@linaro.org,
linux-kernel@vger.kernel.org,
Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
David Brown <davidb@codeaurora.org>,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v4] clk: allow reentrant calls into the clk framework
Date: Wed, 27 Mar 2013 07:25:19 -0700 [thread overview]
Message-ID: <20130327142519.4014.82657@quantum> (raw)
In-Reply-To: <alpine.LFD.2.02.1303271141470.22263@ionos>
Quoting Thomas Gleixner (2013-03-27 04:09:17)
> On Wed, 27 Mar 2013, Ulf Hansson wrote:
> > On 27 March 2013 10:55, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > On 27 March 2013 15:10, Thomas Gleixner <tglx@linutronix.de> wrote:
> > >> On Wed, 27 Mar 2013, Mike Turquette wrote:
> > >>
> > >>> Reentrancy into the clock framework from the clk.h api is necessary
> > >>> for clocks that are prepared and unprepared via i2c_transfer (which
> > >>> includes many PMICs and discrete audio chips) as well as for several
> > >>> other use cases.
> > >>
> > >> That explanation sucks.
> > >>
> > >> Why does an i2c clock need reentrancy? Just because it's i2c or what?
> > >
> > > I am noway connected to this development but was just going through
> > > your mail and i think i might know the answer why is this required.
> > >
> > > Consider an example where an external chip has clock controller and has
> > > bits which can be programmed to enable/disable clock. And this chip is
> > > connected via spi/i2c to SoC.
> > >
> > > clk_prepare(peripheral on external chip)
> > > -> i2c_xfer(to write to external chips register)
> > > -> clk_enable(i2c controller)
> > > ->controller-xfer-routine.. and finally we enable clk here...
>
> Which does not explain the whole issue:
>
> clk_prepare() takes the mutex
> clk_enable() takes the spinlock
>
> That works today.
>
> The issue arises, if you need to call clk_prepare(i2c) in the xfer
> routine.
>
The issue arises any time a clk_ops callback calls a function that
unwittingly re-enters the clock framework. I think the easiest example
to understand and perhaps the most common in practice is a clock which
is controlled via an i2c transfer.
Viresh's example makes the mistake of calling
clk_enable(i2c_controller), but it must also call
clk_prepare(i2c_controller) which is missing in the call graph above.
That nested call to clk_prepare is where the reentrancy comes from.
This has nothing to do with the prepare/enable locking split and leaves
that relationship intact.
> > >
> > > Sorry if i am on the wrong side :)
>
> Only slightly :)
>
> > I agree with you Viresh. I guess Mike should update the commit message.
> >
> > I would also like add another reason to why this is needed. For some
> > clks you would like to do pinctrl operations from a clk hw. But since
> > a pinctrl driver likely requires a clk to be prepared|enabled, we run
> > into a clk reentrant issue.
>
> Fair enough. This all wants to go into the changelog, so we can
> understand why we have this business.
>
I'll submit a v5 which I hope will end the pain and suffering this patch
has caused you.
Regards,
Mike
> Thanks,
>
> tglx
next prev parent reply other threads:[~2013-03-27 14:25 UTC|newest]
Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-03-27 7:09 [PATCH v4] clk: allow reentrant calls into the clk framework Mike Turquette
2013-03-27 7:09 ` Mike Turquette
2013-03-27 9:08 ` Laurent Pinchart
2013-03-27 9:08 ` Laurent Pinchart
2013-03-27 15:06 ` Mike Turquette
2013-03-27 17:12 ` Thomas Gleixner
2013-03-27 17:12 ` Thomas Gleixner
2013-03-27 9:40 ` Thomas Gleixner
2013-03-27 9:40 ` Thomas Gleixner
2013-03-27 9:55 ` Viresh Kumar
2013-03-27 9:55 ` Viresh Kumar
2013-03-27 10:03 ` Ulf Hansson
2013-03-27 10:03 ` Ulf Hansson
2013-03-27 11:09 ` Thomas Gleixner
2013-03-27 11:09 ` Thomas Gleixner
2013-03-27 14:25 ` Mike Turquette [this message]
2013-03-27 14:25 ` Mike Turquette
2013-03-27 9:59 ` Laurent Pinchart
2013-03-27 9:59 ` Laurent Pinchart
2013-03-27 11:24 ` Thomas Gleixner
2013-03-27 11:24 ` Thomas Gleixner
2013-03-27 16:47 ` Mike Turquette
2013-03-27 17:09 ` Thomas Gleixner
2013-03-27 17:09 ` Thomas Gleixner
2013-03-27 22:56 ` Russell King - ARM Linux
2013-03-27 22:56 ` Russell King - ARM Linux
2013-03-28 3:00 ` Mike Turquette
2013-03-28 4:45 ` [PATCH v5 0/2] reentrancy in the common " Mike Turquette
2013-03-28 4:45 ` Mike Turquette
2013-03-28 4:45 ` [PATCH 1/2] clk: abstract locking out into helper functions Mike Turquette
2013-03-28 4:45 ` Mike Turquette
2013-03-28 9:31 ` Thomas Gleixner
2013-03-28 9:31 ` Thomas Gleixner
2013-03-28 4:45 ` [PATCH 2/2] clk: allow reentrant calls into the clk framework Mike Turquette
2013-03-28 4:45 ` Mike Turquette
2013-03-28 9:33 ` Thomas Gleixner
2013-03-28 9:33 ` Thomas Gleixner
2013-03-28 15:23 ` Mike Turquette
2013-03-28 15:23 ` Mike Turquette
2013-03-28 10:44 ` [PATCH v5 0/2] reentrancy in the common " Laurent Pinchart
2013-03-28 10:44 ` Laurent Pinchart
2013-03-28 20:59 ` [PATCH v6 " Mike Turquette
2013-03-28 20:59 ` Mike Turquette
2013-03-28 20:59 ` [PATCH 1/2] clk: abstract locking out into helper functions Mike Turquette
2013-03-28 20:59 ` Mike Turquette
2013-04-02 9:23 ` Ulf Hansson
2013-04-02 9:23 ` Ulf Hansson
2013-03-28 20:59 ` [PATCH 2/2] clk: allow reentrant calls into the clk framework Mike Turquette
2013-03-28 20:59 ` Mike Turquette
2013-04-02 9:35 ` Ulf Hansson
2013-04-02 9:35 ` Ulf Hansson
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=20130327142519.4014.82657@quantum \
--to=mturquette@linaro.org \
--cc=linux-arm-kernel@lists.infradead.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.