| Message ID | 54D3CD6A.1010209@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/05/15 12:07, Stephen Boyd wrote: > On 02/05/15 11:44, Sylwester Nawrocki wrote: >> Hi Tomeu, >> >> On 23/01/15 12:03, Tomeu Vizoso wrote: >>> int __clk_get(struct clk *clk) >>> { >>> - if (clk) { >>> - if (!try_module_get(clk->owner)) >>> + struct clk_core *core = !clk ? NULL : clk->core; >>> + >>> + if (core) { >>> + if (!try_module_get(core->owner)) >>> return 0; >>> >>> - kref_get(&clk->ref); >>> + kref_get(&core->ref); >>> } >>> return 1; >>> } >>> >>> -void __clk_put(struct clk *clk) >>> +static void clk_core_put(struct clk_core *core) >>> { >>> struct module *owner; >>> >>> - if (!clk || WARN_ON_ONCE(IS_ERR(clk))) >>> - return; >>> + owner = core->owner; >>> >>> clk_prepare_lock(); >>> - owner = clk->owner; >>> - kref_put(&clk->ref, __clk_release); >>> + kref_put(&core->ref, __clk_release); >>> clk_prepare_unlock(); >>> >>> module_put(owner); >>> } >>> >>> +void __clk_put(struct clk *clk) >>> +{ >>> + if (!clk || WARN_ON_ONCE(IS_ERR(clk))) >>> + return; >>> + >>> + clk_core_put(clk->core); >>> + kfree(clk); >> Why do we have kfree() here? clk_get() doesn't allocate the data structure >> being freed here. What happens if we do clk_get(), clk_put(), clk_get() >> on same clock? >> >> I suspect __clk_free_clk() should be called in __clk_release() callback >> instead, but then there is an issue of safely getting reference to >> struct clk from struct clk_core pointer. >> >> I tested linux-next on Odroid U3 and booting fails with oopses as below. >> There is no problems when the above kfree() is commented out. >> >> > Ah now I get it. You meant to say that of_clk_get_by_clkspec() doesn't > return an allocated clk pointer. Let's fix that. > > ----8<---- > > From: Stephen Boyd <sboyd@codeaurora.org> > Subject: [PATCH] clkdev: Always allocate a struct clk in OF functions > > of_clk_get_by_clkspec() returns a struct clk pointer but it > doesn't create a new handle for the consumers. Instead it just > returns whatever the OF clk provider hands out. Let's create a > per-user handle here so that clk_put() can properly unlink it and > free it when the consumer is done. > > Fixes: 035a61c314eb "clk: Make clk API return per-user struct clk instances" > Reported-by: Sylwester Nawrocki <s.nawrocki@samsung.com> > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> > --- > drivers/clk/clkdev.c | 44 +++++++++++++++++++++++--------------------- > 1 file changed, 23 insertions(+), 21 deletions(-) > > diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c > index 29a1ab7af4b8..00d747d09b2a 100644 > --- a/drivers/clk/clkdev.c > +++ b/drivers/clk/clkdev.c > @@ -29,15 +29,8 @@ static DEFINE_MUTEX(clocks_mutex); > > #if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK) > > -/** > - * of_clk_get_by_clkspec() - Lookup a clock form a clock provider > - * @clkspec: pointer to a clock specifier data structure > - * > - * This function looks up a struct clk from the registered list of clock > - * providers, an input is a clock specifier data structure as returned > - * from the of_parse_phandle_with_args() function call. > - */ > -struct clk *of_clk_get_by_clkspec(struct of_phandle_args *clkspec) > +static struct clk *__of_clk_get_by_clkspec(struct of_phandle_args *clkspec, > + const char *dev_id, const char *con_id) > { > struct clk *clk; > > @@ -47,6 +40,8 @@ struct clk *of_clk_get_by_clkspec(struct of_phandle_args *clkspec) > of_clk_lock(); > clk = __of_clk_get_from_provider(clkspec); > > + if (!IS_ERR(clk)) > + clk = __clk_create_clk(__clk_get_hw(clk), dev_id, con_id); > if (!IS_ERR(clk) && !__clk_get(clk)) > clk = ERR_PTR(-ENOENT); > Actually we can bury the __clk_create_clk() inside __of_clk_get_from_provider(). We should also move __clk_get() into there because right now we have a hole where whoever calls of_clk_get_from_provider() never calls __clk_get() on the clk, leading to possible badness. v2 coming soon.
On Thu, Feb 05, 2015 at 02:14:01PM -0800, Stephen Boyd wrote: > Actually we can bury the __clk_create_clk() inside > __of_clk_get_from_provider(). We should also move __clk_get() into there > because right now we have a hole where whoever calls > of_clk_get_from_provider() never calls __clk_get() on the clk, leading > to possible badness. v2 coming soon. There's some other issues here too... sound/soc/kirkwood/kirkwood-i2s.c: priv->clk = devm_clk_get(&pdev->dev, np ? "internal" : NULL); ... priv->extclk = devm_clk_get(&pdev->dev, "extclk"); if (IS_ERR(priv->extclk)) { ... } else { if (priv->extclk == priv->clk) { devm_clk_put(&pdev->dev, priv->extclk); priv->extclk = ERR_PTR(-EINVAL); } else { dev_info(&pdev->dev, "found external clock\n"); clk_prepare_enable(priv->extclk); soc_dai = kirkwood_i2s_dai_extclk; } It should be fine provided your "trick" is only done for DT clocks, but not for legacy - with legacy, a NULL in the clkdev tables will match both these requests, hence the need to compare the clk_get() return value to tell whether we get the same clock.
On 02/05/15 16:42, Russell King - ARM Linux wrote: > On Thu, Feb 05, 2015 at 02:14:01PM -0800, Stephen Boyd wrote: >> Actually we can bury the __clk_create_clk() inside >> __of_clk_get_from_provider(). We should also move __clk_get() into there >> because right now we have a hole where whoever calls >> of_clk_get_from_provider() never calls __clk_get() on the clk, leading >> to possible badness. v2 coming soon. > There's some other issues here too... > > sound/soc/kirkwood/kirkwood-i2s.c: > > priv->clk = devm_clk_get(&pdev->dev, np ? "internal" : NULL); > ... > priv->extclk = devm_clk_get(&pdev->dev, "extclk"); > if (IS_ERR(priv->extclk)) { > ... > } else { > if (priv->extclk == priv->clk) { > devm_clk_put(&pdev->dev, priv->extclk); > priv->extclk = ERR_PTR(-EINVAL); > } else { > dev_info(&pdev->dev, "found external clock\n"); > clk_prepare_enable(priv->extclk); > soc_dai = kirkwood_i2s_dai_extclk; > } > > It should be fine provided your "trick" is only done for DT clocks, > but not for legacy - with legacy, a NULL in the clkdev tables will > match both these requests, hence the need to compare the clk_get() > return value to tell whether we get the same clock. > Are we still talking about of_clk_get_from_provider()? Or are we talking about comparing struct clk pointers? From what I can tell this code is now broken because we made all clk getting functions (there's quite a few...) return unique pointers every time they're called. It seems that the driver wants to know if extclk and clk are the same so it can do something differently in kirkwood_set_rate(). Do we need some sort of clk_equal(struct clk *a, struct clk *b) function for drivers like this? Also, even on DT this could fail if the DT author made internal and extclk map to the same clock provider and cell.
On Thu, Feb 05, 2015 at 05:35:28PM -0800, Stephen Boyd wrote: > On 02/05/15 16:42, Russell King - ARM Linux wrote: > > On Thu, Feb 05, 2015 at 02:14:01PM -0800, Stephen Boyd wrote: > >> Actually we can bury the __clk_create_clk() inside > >> __of_clk_get_from_provider(). We should also move __clk_get() into there > >> because right now we have a hole where whoever calls > >> of_clk_get_from_provider() never calls __clk_get() on the clk, leading > >> to possible badness. v2 coming soon. > > There's some other issues here too... > > > > sound/soc/kirkwood/kirkwood-i2s.c: > > > > priv->clk = devm_clk_get(&pdev->dev, np ? "internal" : NULL); > > ... > > priv->extclk = devm_clk_get(&pdev->dev, "extclk"); > > if (IS_ERR(priv->extclk)) { > > ... > > } else { > > if (priv->extclk == priv->clk) { > > devm_clk_put(&pdev->dev, priv->extclk); > > priv->extclk = ERR_PTR(-EINVAL); > > } else { > > dev_info(&pdev->dev, "found external clock\n"); > > clk_prepare_enable(priv->extclk); > > soc_dai = kirkwood_i2s_dai_extclk; > > } > > > > It should be fine provided your "trick" is only done for DT clocks, > > but not for legacy - with legacy, a NULL in the clkdev tables will > > match both these requests, hence the need to compare the clk_get() > > return value to tell whether we get the same clock. > > > > Are we still talking about of_clk_get_from_provider()? Or are we talking > about comparing struct clk pointers? Comparing struct clk pointers, and the implications of the patch changing the clk_get() et.al. to be unique struct clk pointers. > From what I can tell this code is > now broken because we made all clk getting functions (there's quite a > few...) return unique pointers every time they're called. It seems that > the driver wants to know if extclk and clk are the same so it can do > something differently in kirkwood_set_rate(). Do we need some sort of > clk_equal(struct clk *a, struct clk *b) function for drivers like this? Well, the clocks in question are the SoC internal clock (which is more or less fixed, but has a programmable divider) and an externally supplied clock, and the IP has a multiplexer on its input which allows us to select between those two sources. If it were possible to bind both to the same clock, it wouldn't be a useful configuration - nothing would be gained from doing so in terms of available rates. What the comparison is there for is to catch the case with legacy lookups where a clkdev lookup entry with a NULL connection ID results in matching any connection ID passed to clk_get(). If the patch changes this, then we will have a regression - and this is something which needs fixing _before_ we do this "return unique clocks".
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c index 29a1ab7af4b8..00d747d09b2a 100644 --- a/drivers/clk/clkdev.c +++ b/drivers/clk/clkdev.c @@ -29,15 +29,8 @@ static DEFINE_MUTEX(clocks_mutex); #if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK) -/** - * of_clk_get_by_clkspec() - Lookup a clock form a clock provider - * @clkspec: pointer to a clock specifier data structure - * - * This function looks up a struct clk from the registered list of clock - * providers, an input is a clock specifier data structure as returned - * from the of_parse_phandle_with_args() function call. - */ -struct clk *of_clk_get_by_clkspec(struct of_phandle_args *clkspec) +static struct clk *__of_clk_get_by_clkspec(struct of_phandle_args *clkspec, + const char *dev_id, const char *con_id) { struct clk *clk; @@ -47,6 +40,8 @@ struct clk *of_clk_get_by_clkspec(struct of_phandle_args *clkspec) of_clk_lock(); clk = __of_clk_get_from_provider(clkspec); + if (!IS_ERR(clk)) + clk = __clk_create_clk(__clk_get_hw(clk), dev_id, con_id); if (!IS_ERR(clk) && !__clk_get(clk)) clk = ERR_PTR(-ENOENT); @@ -54,7 +49,21 @@ struct clk *of_clk_get_by_clkspec(struct of_phandle_args *clkspec) return clk; } -static struct clk *__of_clk_get(struct device_node *np, int index) +/** + * of_clk_get_by_clkspec() - Lookup a clock form a clock provider + * @clkspec: pointer to a clock specifier data structure + * + * This function looks up a struct clk from the registered list of clock + * providers, an input is a clock specifier data structure as returned + * from the of_parse_phandle_with_args() function call. + */ +struct clk *of_clk_get_by_clkspec(struct of_phandle_args *clkspec) +{ + return __of_clk_get_by_clkspec(clkspec, NULL, __func__); +} + +static struct clk *__of_clk_get(struct device_node *np, int index, + const char *dev_id, const char *con_id) { struct of_phandle_args clkspec; struct clk *clk; @@ -68,7 +77,7 @@ static struct clk *__of_clk_get(struct device_node *np, int index) if (rc) return ERR_PTR(rc); - clk = of_clk_get_by_clkspec(&clkspec); + clk = __of_clk_get_by_clkspec(&clkspec, dev_id, con_id); of_node_put(clkspec.np); return clk; @@ -76,12 +85,7 @@ static struct clk *__of_clk_get(struct device_node *np, int index) struct clk *of_clk_get(struct device_node *np, int index) { - struct clk *clk = __of_clk_get(np, index); - - if (!IS_ERR(clk)) - clk = __clk_create_clk(__clk_get_hw(clk), np->full_name, NULL); - - return clk; + return __of_clk_get(np, index, np->full_name, NULL); } EXPORT_SYMBOL(of_clk_get); @@ -102,12 +106,10 @@ static struct clk *__of_clk_get_by_name(struct device_node *np, */ if (name) index = of_property_match_string(np, "clock-names", name); - clk = __of_clk_get(np, index); + clk = __of_clk_get(np, index, dev_id, name); if (!IS_ERR(clk)) { - clk = __clk_create_clk(__clk_get_hw(clk), dev_id, name); break; - } - else if (name && index >= 0) { + } else if (name && index >= 0) { if (PTR_ERR(clk) != -EPROBE_DEFER) pr_err("ERROR: could not get clock %s:%s(%i)\n", np->full_name, name ? name : "", index);