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

[Michael Turquette <mturquette@baylibre.com>]

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 fir=
st
> > > 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_d=
omain.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 =3D {
 static struct pm_clk_notifier_block platform_bus_notifier =3D {
 	.pm_domain =3D &davinci_pm_domain,
 	.con_ids =3D { "fck", "master", "slave", NULL },
+	.clks =3D { 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 =3D {
 =

 static struct pm_clk_notifier_block platform_domain_notifier =3D {
 	.pm_domain =3D &keystone_pm_domain,
+	.clks =3D { ERR_PTR(-EAGAIN) },
 };
 =

 static const struct of_device_id of_keystone_table[] =3D {
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 =3D {
 static struct pm_clk_notifier_block platform_bus_notifier =3D {
 	.pm_domain =3D &default_pm_domain,
 	.con_ids =3D { "ick", "fck", NULL, },
+	.clks =3D { 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 =3D 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 =3D 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 =3D clknb->con_ids; *con_id; con_id++)
-				enable_clock(dev, *con_id);
+			int i;
+			for (con_id =3D clknb->con_ids, i =3D 0; *con_id;
+						con_id++, i++) {
+				if (clknb->clks[i] =3D=3D ERR_PTR(-EAGAIN))
+					clks[i] =3D 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] =3D=3D ERR_PTR(-EAGAIN))
+				clks[0] =3D 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 =3D clknb->con_ids; *con_id; con_id++)
-				disable_clock(dev, *con_id);
+			int i;
+			for (con_id =3D clknb->con_ids, i =3D 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 =3D {
 static struct pm_clk_notifier_block platform_bus_notifier =3D {
 	.pm_domain =3D &default_pm_domain,
 	.con_ids =3D { NULL, },
+	.clks =3D { 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;