| Message ID | 20151021155057.20687.14055@quantum (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
Quoting Geert Uytterhoeven (2015-10-21 09:46:35) > 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. Well you're right about that. But the problem needs to be solved at some point. There is no good technical reason for this violation to occur other than, "it's been this way for a while now, so let's keep it". Essentially it's blocking the per-user imbalance checks that I'd like to merge. Or we could accept lots of noisy warns for the CONFIG_PM=n case. Regards, Mike > > 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
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;