Re: [PATCH RFC RFT 2/3] clk: clk_put WARNs if user has not disabled clk

Re: [PATCH RFC RFT 2/3] clk: clk_put WARNs if user has not disabled clk

[Geert Uytterhoeven]

Hi Mike,

On Fri, Aug 7, 2015 at 9:09 PM, Michael Turquette
<mturquette@baylibre.com> wrote:
> From the clk_put kerneldoc in include/linux/clk.h:
>
> """
> 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.
> """
>
> The common clock framework implementation of the clk.h api has per-user
> reference counts for calls to clk_prepare and clk_disable. As such it
> can enforce the requirement to properly call clk_disable and
> clk_unprepare before calling clk_put.
>
> Because this requirement is probably violated in many places, this patch
> starts with a simple warning. Once offending code has been fixed this
> check could additionally release the reference counts automatically.
>
> Signed-off-by: Michael Turquette <mturquette@baylibre.com>
> ---
>  drivers/clk/clk.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 72feee9..6ec0f77 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2764,6 +2764,14 @@ void __clk_put(struct clk *clk)
>             clk->max_rate < clk->core->req_rate)
>                 clk_core_set_rate_nolock(clk->core, clk->core->req_rate);
>
> +       /*
> +        * before calling clk_put, all calls to clk_prepare and clk_enable from
> +        * a given user must be balanced with calls to clk_disable and
> +        * clk_unprepare by that same user
> +        */
> +       WARN_ON(clk->prepare_count);
> +       WARN_ON(clk->enable_count);

These two WARN_ON()s are triggered a lot when using a legacy clock domain,
and CONFIG_PM=n. Indeed, without Runtime PM, the idea is that the module clocks
get enabled unconditionally, which violates the assumptions above.

Cfr. the CONFIG_PM=n version of pm_clk_notify() in
drivers/base/power/clock_ops.c, which calls enable_clock():

    /**
     * enable_clock - Enable a device clock.
     * @dev: Device whose clock is to be enabled.
     * @con_id: Connection ID of the clock.
     */
    static void enable_clock(struct device *dev, const char *con_id)
    {
            struct clk *clk;

            clk = clk_get(dev, con_id);
            if (!IS_ERR(clk)) {
                    clk_prepare_enable(clk);
                    clk_put(clk);
                    dev_info(dev, "Runtime PM disabled, clock forced on.\n");
            }
    }

I think this affects shmobile, keystone, davinci, omap1, and legacy sh.

Sorry for not noticing before, we usually build with CONFIG_PM=y.
One more reason for making CONFIG_PM=y mandatory on SoCs with clock domains?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH RFC RFT 2/3] clk: clk_put WARNs if user has not disabled clk

[Russell King - ARM Linux]

On Tue, Oct 20, 2015 at 05:40:00AM -0700, Michael Turquette wrote:
> Why not keep the reference to the struct clk after get'ing it the first
> time?

Yes, that's exactly what you're supposed to do in this case.

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

Re: [PATCH RFC RFT 2/3] clk: clk_put WARNs if user has not disabled clk

[Geert Uytterhoeven]

Hi Mike, Russell,

On Tue, Oct 20, 2015 at 2:40 PM, Michael Turquette
<mturquette@baylibre.com> wrote:
> Quoting Geert Uytterhoeven (2015-09-30 08:38:46)
>> On Fri, Aug 7, 2015 at 9:09 PM, Michael Turquette
>> <mturquette@baylibre.com> wrote:
>> > From the clk_put kerneldoc in include/linux/clk.h:
>> >
>> > """
>> > 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.
>> > """
>> >
>> > The common clock framework implementation of the clk.h api has per-user
>> > reference counts for calls to clk_prepare and clk_disable. As such it
>> > can enforce the requirement to properly call clk_disable and
>> > clk_unprepare before calling clk_put.
>> >
>> > Because this requirement is probably violated in many places, this patch
>> > starts with a simple warning. Once offending code has been fixed this
>> > check could additionally release the reference counts automatically.
>> >
>> > Signed-off-by: Michael Turquette <mturquette@baylibre.com>
>> > ---
>> >  drivers/clk/clk.c | 8 ++++++++
>> >  1 file changed, 8 insertions(+)
>> >
>> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> > index 72feee9..6ec0f77 100644
>> > --- a/drivers/clk/clk.c
>> > +++ b/drivers/clk/clk.c
>> > @@ -2764,6 +2764,14 @@ void __clk_put(struct clk *clk)
>> >             clk->max_rate < clk->core->req_rate)
>> >                 clk_core_set_rate_nolock(clk->core, clk->core->req_rate);
>> >
>> > +       /*
>> > +        * before calling clk_put, all calls to clk_prepare and clk_enable from
>> > +        * a given user must be balanced with calls to clk_disable and
>> > +        * clk_unprepare by that same user
>> > +        */
>> > +       WARN_ON(clk->prepare_count);
>> > +       WARN_ON(clk->enable_count);
>>
>> These two WARN_ON()s are triggered a lot when using a legacy clock domain,
>> and CONFIG_PM=n. Indeed, without Runtime PM, the idea is that the module clocks
>> get enabled unconditionally, which violates the assumptions above.
>>
>> Cfr. the CONFIG_PM=n version of pm_clk_notify() in
>> drivers/base/power/clock_ops.c, which calls enable_clock():
>>
>>     /**
>>      * enable_clock - Enable a device clock.
>>      * @dev: Device whose clock is to be enabled.
>>      * @con_id: Connection ID of the clock.
>>      */
>>     static void enable_clock(struct device *dev, const char *con_id)
>>     {
>>             struct clk *clk;
>>
>>             clk = clk_get(dev, con_id);
>>             if (!IS_ERR(clk)) {
>>                     clk_prepare_enable(clk);
>>                     clk_put(clk);
>
> This is a violation of the clkdev api as defined in include/linux/clk.h:
>
>         /**
>          * clk_put|------ "free" the clock source
>          * @clk: clock source
>          *
>          * 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.

I know.

> So the WARN is doing its job and letting us know about incorrect use of
> the API.
>
>>                     dev_info(dev, "Runtime PM disabled, clock forced on.\n");
>>             }
>>     }
>>
>> I think this affects shmobile, keystone, davinci, omap1, and legacy sh.
>
> Why not keep the reference to the struct clk after get'ing it the first
> time?

And store it where?

dev_pm_get_subsys_data() also depends on CONFIG_PM=y.

Note that there can be multiple clocks.

>> Sorry for not noticing before, we usually build with CONFIG_PM=y.
>> One more reason for making CONFIG_PM=y mandatory on SoCs with clock domains?

On ARM/shmobile, we only use it for the CONFIG_PM=n case, cfr.
drivers/sh/pm_runtime.c. In the CONFIG_PM=y case, we use DT and genpd.

For keystone, davinci, omap1, and legacy sh it's different, though.

With the advent of hardware Power and Clock Domains, keeping support for
CONFIG_PM=n alive is getting harder and harder...

> I don't know about that, but it seems like a reason to fix the clkdev
> usage in the clock domain code.

This is the legacy clock domain code. The way forward is genpd ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH RFC RFT 2/3] clk: clk_put WARNs if user has not disabled clk

[Russell King - ARM Linux]

On Wed, Oct 21, 2015 at 11:50:07AM +0200, Geert Uytterhoeven wrote:
> Hi Mike, Russell,
> 
> On Tue, Oct 20, 2015 at 2:40 PM, Michael Turquette
> <mturquette@baylibre.com> wrote:
> > Why not keep the reference to the struct clk after get'ing it the first
> > time?
> 
> And store it where?

Not my problem :)

Users are supposed to hold on to the reference obtained via clk_get()
while they're making use of the clock: in some implementations, this
increments the module use count if the clock driver is a module, and
may have other effects too.

Dropping that while you're still requiring the clock to be enabled is
unsafe: if it is provided by a module, then removing and reinserting
the module may very well change the enabled state of the clock, it
most certainly will disrupt the enable count.

It's always been this way, right from the outset, and when I've seen
people doing this bollocks, I've always pointed out that it's wrong.
Generally, people will fix it once they become aware of it, so it's
really that people just don't like reading and conforming to published
API requirements.

I think the root cause is that people just don't like reading what
other people write in terms of documentation, and they prefer to go
off and do their own thing, provided it works for them.

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

Re: [PATCH RFC RFT 2/3] clk: clk_put WARNs if user has not disabled clk

[Michael Turquette]

Quoting Russell King - ARM Linux (2015-10-21 03:59:32)
> On Wed, Oct 21, 2015 at 11:50:07AM +0200, Geert Uytterhoeven wrote:
> > Hi Mike, Russell,
> > 
> > On Tue, Oct 20, 2015 at 2:40 PM, Michael Turquette
> > <mturquette@baylibre.com> wrote:
> > > Why not keep the reference to the struct clk after get'ing it the first
> > > time?
> > 
> > And store it where?
> 
> Not my problem :)
> 
> Users are supposed to hold on to the reference obtained via clk_get()
> while they're making use of the clock: in some implementations, this
> increments the module use count if the clock driver is a module, and
> may have other effects too.
> 
> Dropping that while you're still requiring the clock to be enabled is
> unsafe: if it is provided by a module, then removing and reinserting
> the module may very well change the enabled state of the clock, it
> most certainly will disrupt the enable count.
> 
> It's always been this way, right from the outset, and when I've seen
> people doing this bollocks, I've always pointed out that it's wrong.
> Generally, people will fix it once they become aware of it, so it's
> really that people just don't like reading and conforming to published
> API requirements.
> 
> I think the root cause is that people just don't like reading what
> other people write in terms of documentation, and they prefer to go
> off and do their own thing, provided it works for them.

Right, so in other words this problem must be solved by the caller of
clk_get, as it should be. I have never much looked at the pm clk code in
question, but I gave it a quick look and came up with some example code
that does not compile, in an effort to be as helpful as possible.

Basically I added a flex array to struct pm_clk_notifier_block, so that
now there are two flex arrays which is stupid. But I am too lazy to
replace the nbclk->clks thing with a malloc after walking all of the
clk_id's to figure out the number of clocks. Or we could just add
.num_clk to the struct, fix up all 4 users of it and drop the NULL
sentinel used the .clk_id's... Hmm that would have been better.

Anyways here is the ugly, non-compiling code. I'll take another look at
it in one year if no one else beats me to it.

Regards,
Mike



diff --git a/arch/arm/mach-davinci/pm_domain.c b/arch/arm/mach-davinci/pm_domain.c
index 78eac2c..b46e5ce 100644
--- a/arch/arm/mach-davinci/pm_domain.c
+++ b/arch/arm/mach-davinci/pm_domain.c
@@ -24,6 +24,7 @@ static struct dev_pm_domain davinci_pm_domain = {
 static struct pm_clk_notifier_block platform_bus_notifier = {
 	.pm_domain = &davinci_pm_domain,
 	.con_ids = { "fck", "master", "slave", NULL },
+	.clks = { ERR_PTR(-EAGAIN), ERR_PTR(-EAGAIN), ERR_PTR(-EAGAIN) },
 };
 
 static int __init davinci_pm_runtime_init(void)
diff --git a/arch/arm/mach-keystone/pm_domain.c b/arch/arm/mach-keystone/pm_domain.c
index e283939..d21c18b 100644
--- a/arch/arm/mach-keystone/pm_domain.c
+++ b/arch/arm/mach-keystone/pm_domain.c
@@ -27,6 +27,7 @@ static struct dev_pm_domain keystone_pm_domain = {
 
 static struct pm_clk_notifier_block platform_domain_notifier = {
 	.pm_domain = &keystone_pm_domain,
+	.clks = { ERR_PTR(-EAGAIN) },
 };
 
 static const struct of_device_id of_keystone_table[] = {
diff --git a/arch/arm/mach-omap1/pm_bus.c b/arch/arm/mach-omap1/pm_bus.c
index 667c163..5506453 100644
--- a/arch/arm/mach-omap1/pm_bus.c
+++ b/arch/arm/mach-omap1/pm_bus.c
@@ -31,6 +31,7 @@ static struct dev_pm_domain default_pm_domain = {
 static struct pm_clk_notifier_block platform_bus_notifier = {
 	.pm_domain = &default_pm_domain,
 	.con_ids = { "ick", "fck", NULL, },
+	.clks = { ERR_PTR(-EAGAIN), ERR_PTR(-EAGAIN) },
 };
 
 static int __init omap1_pm_runtime_init(void)
diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
index 652b5a3..26f0dcf 100644
--- a/drivers/base/power/clock_ops.c
+++ b/drivers/base/power/clock_ops.c
@@ -407,40 +407,6 @@ int pm_clk_runtime_resume(struct device *dev)
 #else /* !CONFIG_PM */
 
 /**
- * enable_clock - Enable a device clock.
- * @dev: Device whose clock is to be enabled.
- * @con_id: Connection ID of the clock.
- */
-static void enable_clock(struct device *dev, const char *con_id)
-{
-	struct clk *clk;
-
-	clk = clk_get(dev, con_id);
-	if (!IS_ERR(clk)) {
-		clk_prepare_enable(clk);
-		clk_put(clk);
-		dev_info(dev, "Runtime PM disabled, clock forced on.\n");
-	}
-}
-
-/**
- * disable_clock - Disable a device clock.
- * @dev: Device whose clock is to be disabled.
- * @con_id: Connection ID of the clock.
- */
-static void disable_clock(struct device *dev, const char *con_id)
-{
-	struct clk *clk;
-
-	clk = clk_get(dev, con_id);
-	if (!IS_ERR(clk)) {
-		clk_disable_unprepare(clk);
-		clk_put(clk);
-		dev_info(dev, "Runtime PM disabled, clock forced off.\n");
-	}
-}
-
-/**
  * pm_clk_notify - Notify routine for device addition and removal.
  * @nb: Notifier block object this function is a member of.
  * @action: Operation being carried out by the caller.
@@ -465,18 +431,45 @@ static int pm_clk_notify(struct notifier_block *nb,
 	switch (action) {
 	case BUS_NOTIFY_BIND_DRIVER:
 		if (clknb->con_ids[0]) {
-			for (con_id = clknb->con_ids; *con_id; con_id++)
-				enable_clock(dev, *con_id);
+			int i;
+			for (con_id = clknb->con_ids, i = 0; *con_id;
+						con_id++, i++) {
+				if (clknb->clks[i] = ERR_PTR(-EAGAIN))
+					clks[i] = clk_get(dev, *con_id);
+				if (!IS_ERR(clknb->clks[i])) {
+					clk_prepare_enable(clk);
+					dev_info(dev, "Runtime PM disabled, clock forced on.\n");
+				}
 		} else {
-			enable_clock(dev, NULL);
+			if (clknb->clks[0] = ERR_PTR(-EAGAIN))
+				clks[0] = clk_get(dev, NULL);
+			if (!IS_ERR(clknb->clks[0])) {
+				clk_prepare_enable(clk);
+				dev_info(dev, "Runtime PM disabled, clock forced on.\n");
+			}
 		}
 		break;
 	case BUS_NOTIFY_UNBOUND_DRIVER:
+		/*
+		 * FIXME
+		 * We never call clk_put. This should be done with
+		 * pm_clk_remove_notifier, which doesn't exist but probably
+		 * should
+		 */
 		if (clknb->con_ids[0]) {
-			for (con_id = clknb->con_ids; *con_id; con_id++)
-				disable_clock(dev, *con_id);
+			int i;
+			for (con_id = clknb->con_ids, i = 0; *con_id;
+					con_id++, i++) {
+				if (!IS_ERR(clknb->clks[i])) {
+					clk_disable_unprepare(clknb->clks[i]);
+					dev_info(dev, "Runtime PM disabled, clock forced off.\n");
+				}
+			}
 		} else {
-			disable_clock(dev, NULL);
+			if (!IS_ERR(clknb->clks[0])) {
+				clk_disable_unprepare(clknb->clks[i]);
+				dev_info(dev, "Runtime PM disabled, clock forced off.\n");
+			}
 		}
 		break;
 	}
diff --git a/drivers/sh/pm_runtime.c b/drivers/sh/pm_runtime.c
index 25abd4e..08754a4 100644
--- a/drivers/sh/pm_runtime.c
+++ b/drivers/sh/pm_runtime.c
@@ -30,6 +30,7 @@ static struct dev_pm_domain default_pm_domain = {
 static struct pm_clk_notifier_block platform_bus_notifier = {
 	.pm_domain = &default_pm_domain,
 	.con_ids = { NULL, },
+	.clks = { ERR_PTR(-EAGAIN) },
 };
 
 static int __init sh_pm_runtime_init(void)
diff --git a/include/linux/pm_clock.h b/include/linux/pm_clock.h
index 25266c6..45e58fe 100644
--- a/include/linux/pm_clock.h
+++ b/include/linux/pm_clock.h
@@ -16,6 +16,7 @@ struct pm_clk_notifier_block {
 	struct notifier_block nb;
 	struct dev_pm_domain *pm_domain;
 	char *con_ids[];
+	struct clk *clks[];
 };
 
 struct clk;

Re: [PATCH RFC RFT 2/3] clk: clk_put WARNs if user has not disabled clk

[Geert Uytterhoeven]

Hi Mike,

On Wed, Oct 21, 2015 at 5:50 PM, Michael Turquette
<mturquette@baylibre.com> wrote:
> Quoting Russell King - ARM Linux (2015-10-21 03:59:32)
>> On Wed, Oct 21, 2015 at 11:50:07AM +0200, Geert Uytterhoeven wrote:
>> > On Tue, Oct 20, 2015 at 2:40 PM, Michael Turquette
>> > <mturquette@baylibre.com> wrote:
>> > > Why not keep the reference to the struct clk after get'ing it the first
>> > > time?
>> >
>> > And store it where?
>>
>> Not my problem :)
>>
>> Users are supposed to hold on to the reference obtained via clk_get()
>> while they're making use of the clock: in some implementations, this
>> increments the module use count if the clock driver is a module, and
>> may have other effects too.
>>
>> Dropping that while you're still requiring the clock to be enabled is
>> unsafe: if it is provided by a module, then removing and reinserting
>> the module may very well change the enabled state of the clock, it
>> most certainly will disrupt the enable count.
>>
>> It's always been this way, right from the outset, and when I've seen
>> people doing this bollocks, I've always pointed out that it's wrong.
>> Generally, people will fix it once they become aware of it, so it's
>> really that people just don't like reading and conforming to published
>> API requirements.
>>
>> I think the root cause is that people just don't like reading what
>> other people write in terms of documentation, and they prefer to go
>> off and do their own thing, provided it works for them.
>
> Right, so in other words this problem must be solved by the caller of
> clk_get, as it should be. I have never much looked at the pm clk code in
> question, but I gave it a quick look and came up with some example code
> that does not compile, in an effort to be as helpful as possible.
>
> Basically I added a flex array to struct pm_clk_notifier_block, so that
> now there are two flex arrays which is stupid. But I am too lazy to
> replace the nbclk->clks thing with a malloc after walking all of the
> clk_id's to figure out the number of clocks. Or we could just add
> .num_clk to the struct, fix up all 4 users of it and drop the NULL
> sentinel used the .clk_id's... Hmm that would have been better.

Thanks for trying.

> index 25266c6..45e58fe 100644
> --- a/include/linux/pm_clock.h
> +++ b/include/linux/pm_clock.h
> @@ -16,6 +16,7 @@ struct pm_clk_notifier_block {
>         struct notifier_block nb;
>         struct dev_pm_domain *pm_domain;
>         char *con_ids[];
> +       struct clk *clks[];
>  };
>
>  struct clk;

Unfortunately that won't work: while the .con_ids[] array is per-platform,
the .clks[] array should be per-device. I.e. it should be tied to the struct
device, not to the struct pm_clk_notifier_block.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds