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.
> +}
[ 1.345850] Unable to handle kernel paging request at virtual address 00200200
[ 1.349319] pgd = c0004000
[ 1.352072] [00200200] *pgd=00000000
[ 1.355574] Internal error: Oops: 805 [#1] PREEMPT SMP ARM
[ 1.361035] Modules linked in:
[ 1.364078] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.19.0-rc1-00104-ga251361a-dirty #992
[ 1.372405] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[ 1.378483] task: ee00b000 ti: ee088000 task.ti: ee088000
[ 1.383879] PC is at __clk_put+0x24/0xd0
[ 1.387773] LR is at clk_prepare_lock+0xc/0xec
[ 1.392198] pc : [<c03eef38>] lr : [<c03ec1f4>] psr: 20000153
[ 1.392198] sp : ee089de8 ip : 00000000 fp : 00000000
[ 1.403653] r10: ee02f480 r9 : 00000001 r8 : 00000000
[ 1.408862] r7 : ee031cc0 r6 : ee089e08 r5 : 00000000 r4 : ee02f480
[ 1.415373] r3 : 00100100 r2 : 00200200 r1 : 0000091e r0 : 00000001
[ 1.421884] Flags: nzCv IRQs on FIQs off Mode SVC_32 ISA ARM Segment kernel
[ 1.429261] Control: 10c5387d Table: 4000404a DAC: 00000015
[ 1.434989] Process swapper/0 (pid: 1, stack limit = 0xee088238)
[ 1.440978] Stack: (0xee089de8 to 0xee08a000)
[ 1.445321] 9de0: ee7c8f14 c03f0ec8 ee089e08 00000000 c0718dc8 00000001
[ 1.453480] 9e00: 00000000 c04ee0f0 ee7e0844 00000001 00000181 c04edb58 ee2bd320 00000000
[ 1.461639] 9e20: 00000000 c011dc5c ee16a1e0 00000000 00000000 c0718dc8 ee16a1e0 ee2bd1e0
[ 1.469798] 9e40: c0641740 ee16a1e0 00000000 ee2bd320 c0718dc8 ee1d3e10 ee1d3e10 00000000
[ 1.477957] 9e60: c0769a88 00000000 c0718dc8 00000000 00000000 c02c3124 c02c310c ee1d3e10
[ 1.486117] 9e80: c07b4eec 00000000 c0769a88 c02c1d0c ee1d3e10 c0769a88 ee1d3e44 00000000
[ 1.494276] 9ea0: c07091dc c02c1eb8 00000000 c0769a88 c02c1e2c c02c0544 ee005478 ee1676c0
[ 1.502435] 9ec0: c0769a88 ee3a4e80 c0760ce8 c02c150c c0669b90 c0769a88 c0746cd8 c0769a88
[ 1.510594] 9ee0: c0746cd8 ee2bc4c0 c0778c00 c02c24e0 00000000 c0746cd8 c0746cd8 c07091f0
[ 1.518753] 9f00: 00000000 c0008944 c04f405c 00000025 ee00b000 60000153 c074ab00 00000000
[ 1.526913] 9f20: 00000000 c074ab90 60000153 00000000 ef7fca5d c050860c 000000b6 c0036b88
[ 1.535071] 9f40: c065ecc4 c06bc728 00000006 00000006 c074ab30 ef7fca40 c0739bdc 00000006
[ 1.543231] 9f60: c0718dbc c0778c00 000000b6 c0718dc8 c06ed598 c06edd64 00000006 00000006
[ 1.551390] 9f80: c06ed598 c003b438 00000000 c04e64f4 00000000 00000000 00000000 00000000
[ 1.559549] 9fa0: 00000000 c04e64fc 00000000 c000e838 00000000 00000000 00000000 00000000
[ 1.567708] 9fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[ 1.575867] 9fe0: 00000000 00000000 00000000 00000000 00000013 00000000 c0c0c0c0 c0c0c0c0
[ 1.584045] [<c03eef38>] (__clk_put) from [<c03f0ec8>] (of_clk_set_defaults+0xe0/0x2c0)
[ 1.592024] [<c03f0ec8>] (of_clk_set_defaults) from [<c02c3124>] (platform_drv_probe+0x18/0xa4)
[ 1.600698] [<c02c3124>] (platform_drv_probe) from [<c02c1d0c>] (driver_probe_device+0x10c/0x22c)
[ 1.609549] [<c02c1d0c>] (driver_probe_device) from [<c02c1eb8>] (__driver_attach+0x8c/0x90)
[ 1.617968] [<c02c1eb8>] (__driver_attach) from [<c02c0544>] (bus_for_each_dev+0x54/0x88)
[ 1.626128] [<c02c0544>] (bus_for_each_dev) from [<c02c150c>] (bus_add_driver+0xd4/0x1d0)
[ 1.634286] [<c02c150c>] (bus_add_driver) from [<c02c24e0>] (driver_register+0x78/0xf4)
[ 1.642275] [<c02c24e0>] (driver_register) from [<c07091f0>] (fimc_md_init+0x14/0x30)
[ 1.650089] [<c07091f0>] (fimc_md_init) from [<c0008944>] (do_one_initcall+0x80/0x1d0)
[ 1.657989] [<c0008944>] (do_one_initcall) from [<c06edd64>] (kernel_init_freeable+0x108/0x1d4)
[ 1.666675] [<c06edd64>] (kernel_init_freeable) from [<c04e64fc>] (kernel_init+0x8/0xec)
[ 1.674743] [<c04e64fc>] (kernel_init) from [<c000e838>] (ret_from_fork+0x14/0x3c)
[ 1.682287] Code: ebfff4ae e5943014 e5942018 e3530000 (e5823000)
[ 1.688606] ---[ end trace bb367704ce3168e1 ]---
--
Regards,
Sylwester
On 05/02/15 20:44, Sylwester Nawrocki wrote:
>> +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.
Please ignore this comment, I missed __clk_create_clk() calls in clkdev.c
Anyway, in current -next I'm seeing random pointer dereferences while
booting Odroid U3, I'll get back to debugging this tomorrow morning.
--
Regards,
Sylwester
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 <[email protected]>
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 <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
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);
@@ -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);
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
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 <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
> 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.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
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.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
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.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
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".
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
On 02/06/15 05:39, Russell King - ARM Linux wrote:
> On Thu, Feb 05, 2015 at 05:35:28PM -0800, Stephen Boyd wrote:
>
>> 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".
>
Ok. It seems that we might need a clk_equal() or similar API after all.
My understanding is that this driver is calling clk_get() twice with
NULL for the con_id and then "extclk" in attempts to get the SoC
internal clock and the externally supplied clock. If we're using legacy
lookups then both clk_get() calls may map to the same clk_lookup entry
and before Tomeu's patch that returns unique clocks the driver could
detect this case and know that there isn't an external clock. Looking at
arch/arm/mach-dove/common.c it seems that there is only one lookup per
device and it has a wildcard NULL for con_id so both clk_get() calls
here are going to find the same lookup and get a unique struct clk pointer.
Why don't we make the legacy lookup more specific and actually indicate
"internal" for the con_id? Then the external clock would fail to be
found, but we can detect that case and figure out that it's not due to
probe defer, but instead due to the fact that there really isn't any
mapping. It looks like the code is already prepared for this anyway.
----8<----
From: Stephen Boyd <[email protected]>
Subject: [PATCH] ARM: dove: Remove wildcard from mvebu-audio device clk lookup
This i2s driver is using the wildcard nature of clkdev lookups to
figure out if there's an external clock or not. It does this by
calling clk_get() twice with NULL for the con_id first and then
"external" for the con_id the second time around and then
compares the two pointers. With DT the wildcard feature of
clk_get() is gone and so the driver has to handle an error from
the second clk_get() call as meaning "no external clock
specified". Let's use that logic even with clk lookups to
simplify the code and remove the struct clk pointer comparisons
which may not work in the future when clk_get() returns unique
pointers.
Signed-off-by: Stephen Boyd <[email protected]>
---
arch/arm/mach-dove/common.c | 4 ++--
sound/soc/kirkwood/kirkwood-i2s.c | 13 ++++---------
2 files changed, 6 insertions(+), 11 deletions(-)
diff --git a/arch/arm/mach-dove/common.c b/arch/arm/mach-dove/common.c
index 0d1a89298ece..f290fc944cc1 100644
--- a/arch/arm/mach-dove/common.c
+++ b/arch/arm/mach-dove/common.c
@@ -124,8 +124,8 @@ static void __init dove_clk_init(void)
orion_clkdev_add(NULL, "sdhci-dove.1", sdio1);
orion_clkdev_add(NULL, "orion_nand", nand);
orion_clkdev_add(NULL, "cafe1000-ccic.0", camera);
- orion_clkdev_add(NULL, "mvebu-audio.0", i2s0);
- orion_clkdev_add(NULL, "mvebu-audio.1", i2s1);
+ orion_clkdev_add("internal", "mvebu-audio.0", i2s0);
+ orion_clkdev_add("internal", "mvebu-audio.1", i2s1);
orion_clkdev_add(NULL, "mv_crypto", crypto);
orion_clkdev_add(NULL, "dove-ac97", ac97);
orion_clkdev_add(NULL, "dove-pdma", pdma);
diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
index def7d8260c4e..0bfeb712a997 100644
--- a/sound/soc/kirkwood/kirkwood-i2s.c
+++ b/sound/soc/kirkwood/kirkwood-i2s.c
@@ -564,7 +564,7 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
return -EINVAL;
}
- priv->clk = devm_clk_get(&pdev->dev, np ? "internal" : NULL);
+ priv->clk = devm_clk_get(&pdev->dev, "internal");
if (IS_ERR(priv->clk)) {
dev_err(&pdev->dev, "no clock\n");
return PTR_ERR(priv->clk);
@@ -579,14 +579,9 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
if (PTR_ERR(priv->extclk) == -EPROBE_DEFER)
return -EPROBE_DEFER;
} 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;
- }
+ dev_info(&pdev->dev, "found external clock\n");
+ clk_prepare_enable(priv->extclk);
+ soc_dai = kirkwood_i2s_dai_extclk;
}
/* Some sensible defaults - this reflects the powerup values */
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On Fri, Feb 06, 2015 at 11:30:18AM -0800, Stephen Boyd wrote:
> Why don't we make the legacy lookup more specific and actually indicate
> "internal" for the con_id? Then the external clock would fail to be
> found, but we can detect that case and figure out that it's not due to
> probe defer, but instead due to the fact that there really isn't any
> mapping. It looks like the code is already prepared for this anyway.
We _could_, and that would solve this specific issue, but I'd suggest
coccinelle is used to locate any other similar instances of this.
As I'm allergic to coccinelle (or coccinelle is allergic to me since
I never seem to be able to get it to do what I want...)
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
On 02/06/15 11:37, Russell King - ARM Linux wrote:
> On Fri, Feb 06, 2015 at 11:30:18AM -0800, Stephen Boyd wrote:
>> Why don't we make the legacy lookup more specific and actually indicate
>> "internal" for the con_id? Then the external clock would fail to be
>> found, but we can detect that case and figure out that it's not due to
>> probe defer, but instead due to the fact that there really isn't any
>> mapping. It looks like the code is already prepared for this anyway.
> We _could_, and that would solve this specific issue, but I'd suggest
> coccinelle is used to locate any other similar instances of this.
>
> As I'm allergic to coccinelle (or coccinelle is allergic to me since
> I never seem to be able to get it to do what I want...)
>
Great. I'd like to avoid adding clk_equal() until we actually need it,
and I hope we don't ever need it. We've already got coccinelle in the
works to find similar issues and it seems you and I have the same
allergies because I can't get it to work for me right now.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
Quoting Stephen Boyd (2015-02-06 11:30:18)
> On 02/06/15 05:39, Russell King - ARM Linux wrote:
> > On Thu, Feb 05, 2015 at 05:35:28PM -0800, Stephen Boyd wrote:
> >
> >> 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".
> >
>
> Ok. It seems that we might need a clk_equal() or similar API after all.
> My understanding is that this driver is calling clk_get() twice with
> NULL for the con_id and then "extclk" in attempts to get the SoC
> internal clock and the externally supplied clock. If we're using legacy
> lookups then both clk_get() calls may map to the same clk_lookup entry
> and before Tomeu's patch that returns unique clocks the driver could
> detect this case and know that there isn't an external clock. Looking at
> arch/arm/mach-dove/common.c it seems that there is only one lookup per
> device and it has a wildcard NULL for con_id so both clk_get() calls
> here are going to find the same lookup and get a unique struct clk pointer.
>
> Why don't we make the legacy lookup more specific and actually indicate
> "internal" for the con_id? Then the external clock would fail to be
> found, but we can detect that case and figure out that it's not due to
> probe defer, but instead due to the fact that there really isn't any
> mapping. It looks like the code is already prepared for this anyway.
>
> ----8<----
>
> From: Stephen Boyd <[email protected]>
> Subject: [PATCH] ARM: dove: Remove wildcard from mvebu-audio device clk lookup
>
> This i2s driver is using the wildcard nature of clkdev lookups to
> figure out if there's an external clock or not. It does this by
> calling clk_get() twice with NULL for the con_id first and then
> "external" for the con_id the second time around and then
> compares the two pointers. With DT the wildcard feature of
> clk_get() is gone and so the driver has to handle an error from
> the second clk_get() call as meaning "no external clock
> specified". Let's use that logic even with clk lookups to
> simplify the code and remove the struct clk pointer comparisons
> which may not work in the future when clk_get() returns unique
> pointers.
>
> Signed-off-by: Stephen Boyd <[email protected]>
Russell et al,
I'm happy to take this patch through the clock tree (where the problem
shows up) with an ack.
Regards,
Mike
> ---
> arch/arm/mach-dove/common.c | 4 ++--
> sound/soc/kirkwood/kirkwood-i2s.c | 13 ++++---------
> 2 files changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/arch/arm/mach-dove/common.c b/arch/arm/mach-dove/common.c
> index 0d1a89298ece..f290fc944cc1 100644
> --- a/arch/arm/mach-dove/common.c
> +++ b/arch/arm/mach-dove/common.c
> @@ -124,8 +124,8 @@ static void __init dove_clk_init(void)
> orion_clkdev_add(NULL, "sdhci-dove.1", sdio1);
> orion_clkdev_add(NULL, "orion_nand", nand);
> orion_clkdev_add(NULL, "cafe1000-ccic.0", camera);
> - orion_clkdev_add(NULL, "mvebu-audio.0", i2s0);
> - orion_clkdev_add(NULL, "mvebu-audio.1", i2s1);
> + orion_clkdev_add("internal", "mvebu-audio.0", i2s0);
> + orion_clkdev_add("internal", "mvebu-audio.1", i2s1);
> orion_clkdev_add(NULL, "mv_crypto", crypto);
> orion_clkdev_add(NULL, "dove-ac97", ac97);
> orion_clkdev_add(NULL, "dove-pdma", pdma);
> diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
> index def7d8260c4e..0bfeb712a997 100644
> --- a/sound/soc/kirkwood/kirkwood-i2s.c
> +++ b/sound/soc/kirkwood/kirkwood-i2s.c
> @@ -564,7 +564,7 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
> return -EINVAL;
> }
>
> - priv->clk = devm_clk_get(&pdev->dev, np ? "internal" : NULL);
> + priv->clk = devm_clk_get(&pdev->dev, "internal");
> if (IS_ERR(priv->clk)) {
> dev_err(&pdev->dev, "no clock\n");
> return PTR_ERR(priv->clk);
> @@ -579,14 +579,9 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
> if (PTR_ERR(priv->extclk) == -EPROBE_DEFER)
> return -EPROBE_DEFER;
> } 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;
> - }
> + dev_info(&pdev->dev, "found external clock\n");
> + clk_prepare_enable(priv->extclk);
> + soc_dai = kirkwood_i2s_dai_extclk;
> }
>
> /* Some sensible defaults - this reflects the powerup values */
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
On Thu, Feb 19, 2015 at 01:32:33PM -0800, Mike Turquette wrote:
> Quoting Stephen Boyd (2015-02-06 11:30:18)
> > On 02/06/15 05:39, Russell King - ARM Linux wrote:
> > > On Thu, Feb 05, 2015 at 05:35:28PM -0800, Stephen Boyd wrote:
> > >
> > >> 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".
> > >
> >
> > Ok. It seems that we might need a clk_equal() or similar API after all.
> > My understanding is that this driver is calling clk_get() twice with
> > NULL for the con_id and then "extclk" in attempts to get the SoC
> > internal clock and the externally supplied clock. If we're using legacy
> > lookups then both clk_get() calls may map to the same clk_lookup entry
> > and before Tomeu's patch that returns unique clocks the driver could
> > detect this case and know that there isn't an external clock. Looking at
> > arch/arm/mach-dove/common.c it seems that there is only one lookup per
> > device and it has a wildcard NULL for con_id so both clk_get() calls
> > here are going to find the same lookup and get a unique struct clk pointer.
> >
> > Why don't we make the legacy lookup more specific and actually indicate
> > "internal" for the con_id? Then the external clock would fail to be
> > found, but we can detect that case and figure out that it's not due to
> > probe defer, but instead due to the fact that there really isn't any
> > mapping. It looks like the code is already prepared for this anyway.
> >
> > ----8<----
> >
> > From: Stephen Boyd <[email protected]>
> > Subject: [PATCH] ARM: dove: Remove wildcard from mvebu-audio device clk lookup
> >
> > This i2s driver is using the wildcard nature of clkdev lookups to
> > figure out if there's an external clock or not. It does this by
> > calling clk_get() twice with NULL for the con_id first and then
> > "external" for the con_id the second time around and then
> > compares the two pointers. With DT the wildcard feature of
> > clk_get() is gone and so the driver has to handle an error from
> > the second clk_get() call as meaning "no external clock
> > specified". Let's use that logic even with clk lookups to
> > simplify the code and remove the struct clk pointer comparisons
> > which may not work in the future when clk_get() returns unique
> > pointers.
> >
> > Signed-off-by: Stephen Boyd <[email protected]>
>
> Russell et al,
>
> I'm happy to take this patch through the clock tree (where the problem
> shows up) with an ack.
It's not up to me - I don't maintain this driver. I'm just an interested
party.
Note that much more than just this has now broken. The iMX6 code has
broken as well, and it's not going to take such a simple fix there to
fix it either.
Please either revert the patches creating this breakage (and have another
attempt at the next merge window) or supply fixes for these places.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.