Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1580799 > unrolled thread

Re: [PATCH v3] clk: add more managed APIs

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3] clk: add more managed APIs Stephen Boyd <sboyd@codeaurora.org> - 2017-02-14 20:50 +0100
    Re: [PATCH v3] clk: add more managed APIs Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-02-14 21:00 +0100
      Re: [PATCH v3] clk: add more managed APIs Guenter Roeck <linux@roeck-us.net> - 2017-02-14 21:40 +0100
    Re: [PATCH v3] clk: add more managed APIs Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-02-14 21:10 +0100

#1580799 — Re: [PATCH v3] clk: add more managed APIs

On 02/06, Dmitry Torokhov wrote:
> On Mon, Jan 30, 2017 at 04:57:13PM -0800, Dmitry Torokhov wrote:
> > When converting a driver to managed resources it is desirable to be able to
> > manage all resources in the same fashion. This change allows managing clock
> > prepared and enabled state in the same way we manage many other resources.
> > 
> > This adds the following managed APIs:
> > 
> > - devm_clk_prepare()/devm_clk_unprepare();
> > - devm_clk_prepare_enable()/devm_clk_disable_unprepare().
> > 
> > Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> It would be awesome if we could get it into 4.11...

I'd prefer we didn't do this. Instead, make clk_put() drop any
prepare or enables that were done via that clk pointer. Mike
started to do this before[1], but we have some code that assumes
it can do:

	clk = clk_get(...)
	clk_prepare_enable(clk)
	clk_put(clk)

and have the clk stay on. Those would need to be changed.

We would also need Russell's approval to update the clk_put()
documentation to describe this change in behavior.

[1] http://lkml.kernel.org/r/1438974570-20812-1-git-send-email-mturquette@baylibre.com

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [next] | [standalone]

#1580818

On Tue, Feb 14, 2017 at 11:44 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 02/06, Dmitry Torokhov wrote:
>> On Mon, Jan 30, 2017 at 04:57:13PM -0800, Dmitry Torokhov wrote:
>> > When converting a driver to managed resources it is desirable to be able to
>> > manage all resources in the same fashion. This change allows managing clock
>> > prepared and enabled state in the same way we manage many other resources.
>> >
>> > This adds the following managed APIs:
>> >
>> > - devm_clk_prepare()/devm_clk_unprepare();
>> > - devm_clk_prepare_enable()/devm_clk_disable_unprepare().
>> >
>> > Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>
>> It would be awesome if we could get it into 4.11...
>
> I'd prefer we didn't do this. Instead, make clk_put() drop any
> prepare or enables that were done via that clk pointer. Mike
> started to do this before[1], but we have some code that assumes
> it can do:
>
>         clk = clk_get(...)
>         clk_prepare_enable(clk)
>         clk_put(clk)
>
> and have the clk stay on. Those would need to be changed.

That means we'd need to audit entire code base ;(

>
> We would also need Russell's approval to update the clk_put()
> documentation to describe this change in behavior.
>
> [1] http://lkml.kernel.org/r/1438974570-20812-1-git-send-email-mturquette@baylibre.com
>

Note that devm* APIs do not preclude from changing clk_put() behavior
down the road and it is extremely easy to go and
s/devm_clk_prepare_enable/clk_prepare_enable/ once cleanup is
automatic.

Having devm now will help make driver code better (because right now
we either need to add wrappers so devm_add_action_or_reset() can be
used, or continue mixing devm* and goto cleanups, which are often
wrong).

Thanks.

-- 
Dmitry

[toc] | [prev] | [next] | [standalone]

#1580858

On Tue, Feb 14, 2017 at 11:55:20AM -0800, Dmitry Torokhov wrote:
> On Tue, Feb 14, 2017 at 11:44 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > On 02/06, Dmitry Torokhov wrote:
> >> On Mon, Jan 30, 2017 at 04:57:13PM -0800, Dmitry Torokhov wrote:
> >> > When converting a driver to managed resources it is desirable to be able to
> >> > manage all resources in the same fashion. This change allows managing clock
> >> > prepared and enabled state in the same way we manage many other resources.
> >> >
> >> > This adds the following managed APIs:
> >> >
> >> > - devm_clk_prepare()/devm_clk_unprepare();
> >> > - devm_clk_prepare_enable()/devm_clk_disable_unprepare().
> >> >
> >> > Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> >> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >>
> >> It would be awesome if we could get it into 4.11...
> >
> > I'd prefer we didn't do this. Instead, make clk_put() drop any
> > prepare or enables that were done via that clk pointer. Mike
> > started to do this before[1], but we have some code that assumes
> > it can do:
> >
> >         clk = clk_get(...)
> >         clk_prepare_enable(clk)
> >         clk_put(clk)
> >
> > and have the clk stay on. Those would need to be changed.
> 
> That means we'd need to audit entire code base ;(
> 
> >
> > We would also need Russell's approval to update the clk_put()
> > documentation to describe this change in behavior.
> >
> > [1] http://lkml.kernel.org/r/1438974570-20812-1-git-send-email-mturquette@baylibre.com
> >
> 
> Note that devm* APIs do not preclude from changing clk_put() behavior
> down the road and it is extremely easy to go and
> s/devm_clk_prepare_enable/clk_prepare_enable/ once cleanup is
> automatic.
> 
> Having devm now will help make driver code better (because right now
> we either need to add wrappers so devm_add_action_or_reset() can be
> used, or continue mixing devm* and goto cleanups, which are often
> wrong).
> 

Absolutely agree. The combination of clk_prepare_enable() in probe and
clk_disable_unprepare() in remove is used all over the place. Sure,
we _can_ use devm_add_action_or_reset() for all those cases, and I do
have a coccinelle script doing just that. While I'll have to accept
going forward with that approach if needed, I have to admit that
I completely fail to miss the point why that would be a good idea.

Thanks,
Guenter

[toc] | [prev] | [next] | [standalone]

#1580828

On Tue, Feb 14, 2017 at 11:44:08AM -0800, Stephen Boyd wrote:
> I'd prefer we didn't do this. Instead, make clk_put() drop any
> prepare or enables that were done via that clk pointer. Mike
> started to do this before[1], but we have some code that assumes
> it can do:
> 
> 	clk = clk_get(...)
> 	clk_prepare_enable(clk)
> 	clk_put(clk)
> 
> and have the clk stay on. Those would need to be changed.

Yes, I've seen from time to time code that plays this game over the
years.  However, from my simple grepping, it seems that we only have
a small number of cases:

arch/mips/alchemy/devboards/db1200.c:           clk_prepare_enable(c);
arch/mips/alchemy/devboards/db1200.c-           clk_put(c);

arch/mips/alchemy/devboards/db1300.c:           clk_prepare_enable(c);
arch/mips/alchemy/devboards/db1300.c-           clk_put(c);

arch/mips/alchemy/devboards/db1550.c:           clk_prepare_enable(c);
arch/mips/alchemy/devboards/db1550.c-           clk_put(c);

drivers/base/power/clock_ops.c:         clk_prepare_enable(clk);
drivers/base/power/clock_ops.c-         clk_put(clk);

drivers/mtd/nand/orion_nand.c:          clk_prepare_enable(clk);
drivers/mtd/nand/orion_nand.c-          clk_put(clk);

I've always hated that - and it goes against the API:

 * Note: drivers must ensure that all clk_enable calls made on this
 * clock source are balanced by clk_disable calls prior to calling
 * this function.

(That comment should have been updated when clk_prepare() & clk_unprepare()
was added to include balancing those.)

So really, all the cases above are buggy.  However, the statement in the
API doesn't give permission for what you're suggesting!

The suggestion requires that we also cast in stone that every "struct clk"
which is handed out from clk_get() becomes unique _everywhere_, because
that's the only way to really track the prepare/enable state on a per-
clk_get() basis, so we can properly clean up at clk_put().

However, I think we still have some non-CCF clock API implementations
around, which hand out shared "struct clk"s.  Changing this requirement
will impact those, since they would need to be updated before the change
could be made.

So, although it would be a nice change to make (which would rule out the
abuse) I don't think it's one that could be practically made at this
stage without (a) fixing all instances like those quoted above and (b)
converting all non-CCF implementations to either CCF or making them
hand out unique struct clk's.

(I do have patches which converts sa11x0 to CCF... which would be one
less non-CCF implementation.)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web