Automatic clock disabling on clk_put()

Automatic clock disabling on clk_put()

[Sascha Hauer]

Hi,

Since we now have per-user clks we could automatically disable clks on
clk_put() to make the drivers errors paths easier. AFAIR that was one
of the motivations to switch to per user clks. Is this just not yet
implemented or are there problems implementing this?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Automatic clock disabling on clk_put()

[Sascha Hauer]

Hi,

Since we now have per-user clks we could automatically disable clks on
clk_put() to make the drivers errors paths easier. AFAIR that was one
of the motivations to switch to per user clks. Is this just not yet
implemented or are there problems implementing this?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: Automatic clock disabling on clk_put()

[Stephen Boyd]

On 05/10/15 23:01, Sascha Hauer wrote:
> Hi,
>
> Since we now have per-user clks we could automatically disable clks on
> clk_put() to make the drivers errors paths easier. AFAIR that was one
> of the motivations to switch to per user clks. Is this just not yet
> implemented or are there problems implementing this?
>
>

I don't recall that as being a motivation for per-user clks. The
documentation explicitly states:

 * 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.

so it seems that such a behavior would be an API change. Is it really
that hard to call clk_disable_unprepare() on a clock at the end of an
error path? Maybe you could add a devm_clk_get_prepare_enable() that
does what you want and then call clk_disable_unprepare on the exit path?

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

Automatic clock disabling on clk_put()

[Stephen Boyd]

On 05/10/15 23:01, Sascha Hauer wrote:
> Hi,
>
> Since we now have per-user clks we could automatically disable clks on
> clk_put() to make the drivers errors paths easier. AFAIR that was one
> of the motivations to switch to per user clks. Is this just not yet
> implemented or are there problems implementing this?
>
>

I don't recall that as being a motivation for per-user clks. The
documentation explicitly states:

 * 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.

so it seems that such a behavior would be an API change. Is it really
that hard to call clk_disable_unprepare() on a clock at the end of an
error path? Maybe you could add a devm_clk_get_prepare_enable() that
does what you want and then call clk_disable_unprepare on the exit path?

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

Re: Automatic clock disabling on clk_put()

[Uwe Kleine-König]

Hello,

On Tue, May 12, 2015 at 04:23:48PM -0700, Stephen Boyd wrote:
> On 05/10/15 23:01, Sascha Hauer wrote:
> > Hi,
> >
> > Since we now have per-user clks we could automatically disable clks on
> > clk_put() to make the drivers errors paths easier. AFAIR that was one
> > of the motivations to switch to per user clks. Is this just not yet
> > implemented or are there problems implementing this?
> 
> I don't recall that as being a motivation for per-user clks. The
> documentation explicitly states:
> 
>  * 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.
> 
> so it seems that such a behavior would be an API change. Is it really
well, it's a compatible API change. So if you continue to use it as you
were obliged to do before, everything is fine. IMHO the introduction of
per-user clocks are a much bigger issue.

I'm not sure that auto disabling is a good idea[1], but at least giving a
warning should be possible.

> that hard to call clk_disable_unprepare() on a clock at the end of an
> error path? Maybe you could add a devm_clk_get_prepare_enable() that
> does what you want and then call clk_disable_unprepare on the exit path?
You mean clk_disable_unprepare is called in the same function that calls
clk_put I assume such that you don't have to do anything in the exit
path.

Best regards
Uwe

[1] mainly because I didn't think about it
-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Automatic clock disabling on clk_put()

[Uwe Kleine-König]

Hello,

On Tue, May 12, 2015 at 04:23:48PM -0700, Stephen Boyd wrote:
> On 05/10/15 23:01, Sascha Hauer wrote:
> > Hi,
> >
> > Since we now have per-user clks we could automatically disable clks on
> > clk_put() to make the drivers errors paths easier. AFAIR that was one
> > of the motivations to switch to per user clks. Is this just not yet
> > implemented or are there problems implementing this?
> 
> I don't recall that as being a motivation for per-user clks. The
> documentation explicitly states:
> 
>  * 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.
> 
> so it seems that such a behavior would be an API change. Is it really
well, it's a compatible API change. So if you continue to use it as you
were obliged to do before, everything is fine. IMHO the introduction of
per-user clocks are a much bigger issue.

I'm not sure that auto disabling is a good idea[1], but at least giving a
warning should be possible.

> that hard to call clk_disable_unprepare() on a clock at the end of an
> error path? Maybe you could add a devm_clk_get_prepare_enable() that
> does what you want and then call clk_disable_unprepare on the exit path?
You mean clk_disable_unprepare is called in the same function that calls
clk_put I assume such that you don't have to do anything in the exit
path.

Best regards
Uwe

[1] mainly because I didn't think about it
-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: Automatic clock disabling on clk_put()

[Stephen Boyd]

On 05/12/15 23:45, Uwe Kleine-König wrote:
>
>> that hard to call clk_disable_unprepare() on a clock at the end of an
>> error path? Maybe you could add a devm_clk_get_prepare_enable() that
>> does what you want and then call clk_disable_unprepare on the exit path?
> You mean clk_disable_unprepare is called in the same function that calls
> clk_put I assume such that you don't have to do anything in the exit
> path.
>

Yes that's what I mean.

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

Automatic clock disabling on clk_put()

[Stephen Boyd]

On 05/12/15 23:45, Uwe Kleine-K?nig wrote:
>
>> that hard to call clk_disable_unprepare() on a clock at the end of an
>> error path? Maybe you could add a devm_clk_get_prepare_enable() that
>> does what you want and then call clk_disable_unprepare on the exit path?
> You mean clk_disable_unprepare is called in the same function that calls
> clk_put I assume such that you don't have to do anything in the exit
> path.
>

Yes that's what I mean.

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

Re: Automatic clock disabling on clk_put()

[Russell King - ARM Linux]

On Wed, May 13, 2015 at 12:45:12PM -0700, Stephen Boyd wrote:
> On 05/12/15 23:45, Uwe Kleine-König wrote:
> >
> >> that hard to call clk_disable_unprepare() on a clock at the end of an
> >> error path? Maybe you could add a devm_clk_get_prepare_enable() that
> >> does what you want and then call clk_disable_unprepare on the exit path?
> > You mean clk_disable_unprepare is called in the same function that calls
> > clk_put I assume such that you don't have to do anything in the exit
> > path.
> >
> 
> Yes that's what I mean.

In terms of stopping people doing:

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

I think this is a good thing, because it ensures that people can't do
this anymore.

I think you need to audit the code properly first, and fix any instances
of that _before_ posting patches with your idea - let's try to avoid as
much as possible any breakage resulting from this.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Automatic clock disabling on clk_put()

[Russell King - ARM Linux]

On Wed, May 13, 2015 at 12:45:12PM -0700, Stephen Boyd wrote:
> On 05/12/15 23:45, Uwe Kleine-K?nig wrote:
> >
> >> that hard to call clk_disable_unprepare() on a clock at the end of an
> >> error path? Maybe you could add a devm_clk_get_prepare_enable() that
> >> does what you want and then call clk_disable_unprepare on the exit path?
> > You mean clk_disable_unprepare is called in the same function that calls
> > clk_put I assume such that you don't have to do anything in the exit
> > path.
> >
> 
> Yes that's what I mean.

In terms of stopping people doing:

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

I think this is a good thing, because it ensures that people can't do
this anymore.

I think you need to audit the code properly first, and fix any instances
of that _before_ posting patches with your idea - let's try to avoid as
much as possible any breakage resulting from this.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: Automatic clock disabling on clk_put()

[Sascha Hauer]

On Tue, May 12, 2015 at 04:23:48PM -0700, Stephen Boyd wrote:
> On 05/10/15 23:01, Sascha Hauer wrote:
> > Hi,
> >
> > Since we now have per-user clks we could automatically disable clks on
> > clk_put() to make the drivers errors paths easier. AFAIR that was one
> > of the motivations to switch to per user clks. Is this just not yet
> > implemented or are there problems implementing this?
> >
> >
> 
> I don't recall that as being a motivation for per-user clks. The
> documentation explicitly states:
> 
>  * 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.
> 
> so it seems that such a behavior would be an API change.

Hm, yes it would be an API change, but we are only loosening this
restriction. The only impact I see is that previously buggy drivers
get fixed. Hurray :)
Wait, I forgot about the currently 22 clk support implementations in
arch/* :(

> Is it really
> that hard to call clk_disable_unprepare() on a clock at the end of an
> error path?

There are more and more cases in which the clk disabling is the only
reason why an error path even exists. No it's not that hard, it would
just be convenient to not have to care about it.

> Maybe you could add a devm_clk_get_prepare_enable() that
> does what you want and then call clk_disable_unprepare on the exit path?

I never thought about that, that would work aswell. Maybe I'll have a
look into this.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Automatic clock disabling on clk_put()

[Sascha Hauer]

On Tue, May 12, 2015 at 04:23:48PM -0700, Stephen Boyd wrote:
> On 05/10/15 23:01, Sascha Hauer wrote:
> > Hi,
> >
> > Since we now have per-user clks we could automatically disable clks on
> > clk_put() to make the drivers errors paths easier. AFAIR that was one
> > of the motivations to switch to per user clks. Is this just not yet
> > implemented or are there problems implementing this?
> >
> >
> 
> I don't recall that as being a motivation for per-user clks. The
> documentation explicitly states:
> 
>  * 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.
> 
> so it seems that such a behavior would be an API change.

Hm, yes it would be an API change, but we are only loosening this
restriction. The only impact I see is that previously buggy drivers
get fixed. Hurray :)
Wait, I forgot about the currently 22 clk support implementations in
arch/* :(

> Is it really
> that hard to call clk_disable_unprepare() on a clock at the end of an
> error path?

There are more and more cases in which the clk disabling is the only
reason why an error path even exists. No it's not that hard, it would
just be convenient to not have to care about it.

> Maybe you could add a devm_clk_get_prepare_enable() that
> does what you want and then call clk_disable_unprepare on the exit path?

I never thought about that, that would work aswell. Maybe I'll have a
look into this.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |