Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1456742 > unrolled thread

Re: of_clk_add_(hw_)providers multipule times for one node?

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: of_clk_add_(hw_)providers multipule times for one node? Stephen Boyd <sboyd@codeaurora.org> - 2016-08-04 23:30 +0200
    Re: of_clk_add_(hw_)providers multipule times for one node? Rob Herring <robh@kernel.org> - 2016-08-05 00:20 +0200
    Re: of_clk_add_(hw_)providers multipule times for one node? Masahiro Yamada <yamada.masahiro@socionext.com> - 2016-08-07 19:00 +0200
      Re: of_clk_add_(hw_)providers multipule times for one node? Geert Uytterhoeven <geert@linux-m68k.org> - 2016-08-08 11:10 +0200
      Re: of_clk_add_(hw_)providers multipule times for one node? Stephen Boyd <sboyd@codeaurora.org> - 2016-08-09 01:40 +0200
        Re: of_clk_add_(hw_)providers multipule times for one node? Masahiro Yamada <yamada.masahiro@socionext.com> - 2016-08-10 23:30 +0200

#1456742 — Re: of_clk_add_(hw_)providers multipule times for one node?

+Rob in case he has any insight

On 07/09, Masahiro Yamada wrote:
> Hi.
> 
> I think the current code allows to add
> clk_providers multiple times against one DT node.
> 
> Are there cases that really need to do so?

If we have clk drivers that have a device driver structure and
also use CLK_OF_DECLARE then we could get into a situation where
they register two providers for the same device node. I can't
think of any other situation where this would happen though.

> 
> 
> I am thinking the behavior of __of_clk_get_from_provider() is strange.
> 
> 
> The result of __of_clk_get_from_provider() has three patterns:
> 
> [1] success
> [2] return -EPROBE_DEFER
> [3] return -EINVAL  (if clkspec == NULL)
> 
> 
> [3] is a rare case.
> So, almost all error cases are treated as -EPROBE_DEFER.
> 

It used to return the last provider's error, but I accidentally
changed that behavior when adding clk_hw providers in commit
0861e5b8cf80 (clk: Add clk_hw OF clk providers, 2016-02-05).
Nobody seems to have complained though, so you're the first to
have reported this.

> 
> 
> A strange scenario
> ------------------
> 
> If a too big clock index is passed in clkspec,
> of_clk_src_onecell_get() returns -EINVAL. This is reasonable.
> 
> 
> But, __of_clk_get_from_provider() tries to search next nodes despite
> that it has already failed to get a clk.
> 
> Then, it reaches the end of list_for_each_entry() loop, and returns
> -EPROBE_DEFER.  This is not deferred probe at all!  In this case,
> __of_clk_get_from_provider() should return -EINVAL.
> 
> 
> If this is a bug, I am happy to volunteer to fix it.
> 
> 

Right, a behavior change that shouldn't have happened. How about
this patch?

-----8<-----
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index d584004f7af7..cd8106b17cf4 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3125,7 +3125,7 @@ struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
 {
 	struct of_clk_provider *provider;
 	struct clk *clk = ERR_PTR(-EPROBE_DEFER);
-	struct clk_hw *hw = ERR_PTR(-EPROBE_DEFER);
+	struct clk_hw *hw;
 
 	if (!clkspec)
 		return ERR_PTR(-EINVAL);
@@ -3133,12 +3133,13 @@ struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
 	/* Check if we have such a provider in our array */
 	mutex_lock(&of_clk_mutex);
 	list_for_each_entry(provider, &of_clk_providers, link) {
-		if (provider->node == clkspec->np)
+		if (provider->node == clkspec->np) {
 			hw = __of_clk_get_hw_from_provider(provider, clkspec);
-		if (!IS_ERR(hw)) {
 			clk = __clk_create_clk(hw, dev_id, con_id);
+		}
 
-			if (!IS_ERR(clk) && !__clk_get(clk)) {
+		if (!IS_ERR(clk)) {
+			if (!__clk_get(clk)) {
 				__clk_free_clk(clk);
 				clk = ERR_PTR(-ENOENT);
 			}

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [next] | [standalone]

#1456755

On Thu, Aug 4, 2016 at 4:25 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> +Rob in case he has any insight
>
> On 07/09, Masahiro Yamada wrote:
>> Hi.
>>
>> I think the current code allows to add
>> clk_providers multiple times against one DT node.
>>
>> Are there cases that really need to do so?
>
> If we have clk drivers that have a device driver structure and
> also use CLK_OF_DECLARE then we could get into a situation where
> they register two providers for the same device node. I can't
> think of any other situation where this would happen though.

In this case I'd think you could have some sort of hand-off. Perhaps
the code using CLK_OF_DECLARE can only do provide enable and get_rate
and then the full driver loads with more capabilities.

The other case I could think of is you need to match on more than just
the compatible, so the callbacks could have additional logic to match
against. But in that case, only one should succeed. Also, there's the
general problem that we can't support 2 drivers both matching where
one is a better match than the other.

Rob

[toc] | [prev] | [next] | [standalone]

#1457475

Hi Stephen,


2016-08-05 6:25 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>:
> +Rob in case he has any insight
>
> On 07/09, Masahiro Yamada wrote:
>> Hi.
>>
>> I think the current code allows to add
>> clk_providers multiple times against one DT node.
>>
>> Are there cases that really need to do so?
>
> If we have clk drivers that have a device driver structure and
> also use CLK_OF_DECLARE then we could get into a situation where
> they register two providers for the same device node. I can't
> think of any other situation where this would happen though.


What is the benefit for splitting one clock device
into CLK_OF_DECLARE() and a platform_driver?


If we go this way, I think we need to fix the current code.

of_clk_add_provider() calls of_clk_del_provider()
in its failure path.

Notice of_clk_del_provider() unregister
all the providers associated with the device node.

So, if of_clk_add_provider() fails to register a platform driver,
it may unregister another provider added by OF_CLK_DECLARE().

Some platform drivers call of_clk_del_provider() in a .remove callback,
so the same problem could happen.

Why does of_clk_del_provider() take (struct device_node *np) ?
Shouldn't it take (struct of_clk_provider *cp)?




> It used to return the last provider's error, but I accidentally
> changed that behavior when adding clk_hw providers in commit
> 0861e5b8cf80 (clk: Add clk_hw OF clk providers, 2016-02-05).
> Nobody seems to have complained though, so you're the first to
> have reported this.


If we allow multiple OF-providers for one device node,
I think any error should be treated as EPROBE_DEFER,
i.e. the current code is good.


The scenario is:

 - Clocks with ID 0 thru 3 are provided by CLK_OF_DECLARE()
 - Clocks with ID 4 thru 9 are provided by a platform driver.

What if a clock consumer requests the clk ID 5
after CLK_OF_DECLARE(), but before the clk platform driver is registered?

If the clock consumer gets the last provider's error
(-EINVAL returned from CLK_OR_DECLARE one in this case)
it will lose a chance to retry it after clocks from a platform driver
are registered.

A bit nasty...



-- 
Best Regards
Masahiro Yamada

[toc] | [prev] | [next] | [standalone]

#1457630

On Sun, Aug 7, 2016 at 6:54 PM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> 2016-08-05 6:25 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>:
>> +Rob in case he has any insight
>>
>> On 07/09, Masahiro Yamada wrote:
>>> I think the current code allows to add
>>> clk_providers multiple times against one DT node.
>>>
>>> Are there cases that really need to do so?
>>
>> If we have clk drivers that have a device driver structure and
>> also use CLK_OF_DECLARE then we could get into a situation where
>> they register two providers for the same device node. I can't
>> think of any other situation where this would happen though.
>
> What is the benefit for splitting one clock device
> into CLK_OF_DECLARE() and a platform_driver?

It may be useful if you want to provide a few clocks very early in the boot
(through CLK_OF_DECLARE()), and the remaining ones later (through the
platform_driver).

> If we go this way, I think we need to fix the current code.
>
> of_clk_add_provider() calls of_clk_del_provider()
> in its failure path.
>
> Notice of_clk_del_provider() unregister
> all the providers associated with the device node.
>
> So, if of_clk_add_provider() fails to register a platform driver,
> it may unregister another provider added by OF_CLK_DECLARE().

Do you have to call of_clk_add_provider() from the platform driver?
Can't you just populate the missing entries in the clk_onecell_data as
registered from the CLK_OF_DECLARE() driver part?

Alternatively, if there's a clear (non-Linux specific, as this affects DT!)
distinction between the two sets of clocks, you could register your own
twocell_get routine from CLK_OF_DELCARE(), and fill in a second
(third, fourth, ...) set of clocks from the platform_driver.

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

[toc] | [prev] | [next] | [standalone]

#1458311

On 08/08, Masahiro Yamada wrote:
> Hi Stephen,
> 
> 
> 2016-08-05 6:25 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>:
> > +Rob in case he has any insight
> >
> > On 07/09, Masahiro Yamada wrote:
> >> Hi.
> >>
> >> I think the current code allows to add
> >> clk_providers multiple times against one DT node.
> >>
> >> Are there cases that really need to do so?
> >
> > If we have clk drivers that have a device driver structure and
> > also use CLK_OF_DECLARE then we could get into a situation where
> > they register two providers for the same device node. I can't
> > think of any other situation where this would happen though.
> 
> 
> What is the benefit for splitting one clock device
> into CLK_OF_DECLARE() and a platform_driver?
> 
> 
> If we go this way, I think we need to fix the current code.

Sure. Do we have anyone registering two providers for the same
node? I'm trying to weigh the urgency of this.

> 
> of_clk_add_provider() calls of_clk_del_provider()
> in its failure path.
> 
> Notice of_clk_del_provider() unregister
> all the providers associated with the device node.

Where is that? I see a break statement in the while loop after
the first matching np is found.

> 
> So, if of_clk_add_provider() fails to register a platform driver,
> it may unregister another provider added by OF_CLK_DECLARE().

I suppose if we loop over the list in the incorrect order we
would unregister the wrong one.

> 
> Some platform drivers call of_clk_del_provider() in a .remove callback,
> so the same problem could happen.
> 
> Why does of_clk_del_provider() take (struct device_node *np) ?
> Shouldn't it take (struct of_clk_provider *cp)?
> 

Not sure. Probably someone thought they could hide the structure
from consumers and just return success or failure. We could make
it an opaque pointer though and properly cleanup without
iterating the list. Maybe we should do that for the hw provider
API so that it simplifies the search.

> 
> 
> 
> > It used to return the last provider's error, but I accidentally
> > changed that behavior when adding clk_hw providers in commit
> > 0861e5b8cf80 (clk: Add clk_hw OF clk providers, 2016-02-05).
> > Nobody seems to have complained though, so you're the first to
> > have reported this.
> 
> 
> If we allow multiple OF-providers for one device node,
> I think any error should be treated as EPROBE_DEFER,
> i.e. the current code is good.
> 
> 
> The scenario is:
> 
>  - Clocks with ID 0 thru 3 are provided by CLK_OF_DECLARE()
>  - Clocks with ID 4 thru 9 are provided by a platform driver.
> 
> What if a clock consumer requests the clk ID 5
> after CLK_OF_DECLARE(), but before the clk platform driver is registered?
> 
> If the clock consumer gets the last provider's error
> (-EINVAL returned from CLK_OR_DECLARE one in this case)
> it will lose a chance to retry it after clocks from a platform driver
> are registered.
> 
> A bit nasty...
> 

Yes, but right now if we get an error from a provider and that's
the only provider in the list we don't return the error. Also,
this case should be fixed by having the first provider return
EPROBE_DEFER for clks that aren't populated early on.

The best we can do is have the framework only return probe defer
if there isn't a provider registered. Once a provider is
registered, it needs to do the right thing and return the
appropriate error (invalid or probe defer for example) at the
right time.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1459980

Hi Stephen,



2016-08-09 8:37 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>:
> On 08/08, Masahiro Yamada wrote:
>> Hi Stephen,
>>
>>
>> 2016-08-05 6:25 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>:
>> > +Rob in case he has any insight
>> >
>> > On 07/09, Masahiro Yamada wrote:
>> >> Hi.
>> >>
>> >> I think the current code allows to add
>> >> clk_providers multiple times against one DT node.
>> >>
>> >> Are there cases that really need to do so?
>> >
>> > If we have clk drivers that have a device driver structure and
>> > also use CLK_OF_DECLARE then we could get into a situation where
>> > they register two providers for the same device node. I can't
>> > think of any other situation where this would happen though.
>>
>>
>> What is the benefit for splitting one clock device
>> into CLK_OF_DECLARE() and a platform_driver?
>>
>>
>> If we go this way, I think we need to fix the current code.
>
> Sure. Do we have anyone registering two providers for the same
> node? I'm trying to weigh the urgency of this.
>
>>
>> of_clk_add_provider() calls of_clk_del_provider()
>> in its failure path.
>>
>> Notice of_clk_del_provider() unregister
>> all the providers associated with the device node.
>
> Where is that? I see a break statement in the while loop after
> the first matching np is found.

Ah, I missed the "break".

So, this works *almost* well.

I mean *almost* because the of_clk_mutex is released
between of_clk_add_hw_provider() and of_clk_del_provider().

What if two providers are added concurrently.
I know it never happens in use-cases we assume, though.



>>
>> So, if of_clk_add_provider() fails to register a platform driver,
>> it may unregister another provider added by OF_CLK_DECLARE().
>
> I suppose if we loop over the list in the incorrect order we
> would unregister the wrong one.

Right.

Here, the last added will be first deleted.


>>
>> Some platform drivers call of_clk_del_provider() in a .remove callback,
>> so the same problem could happen.
>>
>> Why does of_clk_del_provider() take (struct device_node *np) ?
>> Shouldn't it take (struct of_clk_provider *cp)?
>>
>
> Not sure. Probably someone thought they could hide the structure
> from consumers and just return success or failure.

consumers?   or did you mean providers?
I think consumers have no chance to call of_clk_del_provider().


> We could make
> it an opaque pointer though and properly cleanup without
> iterating the list. Maybe we should do that for the hw provider
> API so that it simplifies the search.
>
>>
>>
>>
>> > It used to return the last provider's error, but I accidentally
>> > changed that behavior when adding clk_hw providers in commit
>> > 0861e5b8cf80 (clk: Add clk_hw OF clk providers, 2016-02-05).
>> > Nobody seems to have complained though, so you're the first to
>> > have reported this.
>>
>>
>> If we allow multiple OF-providers for one device node,
>> I think any error should be treated as EPROBE_DEFER,
>> i.e. the current code is good.
>>
>>
>> The scenario is:
>>
>>  - Clocks with ID 0 thru 3 are provided by CLK_OF_DECLARE()
>>  - Clocks with ID 4 thru 9 are provided by a platform driver.
>>
>> What if a clock consumer requests the clk ID 5
>> after CLK_OF_DECLARE(), but before the clk platform driver is registered?
>>
>> If the clock consumer gets the last provider's error
>> (-EINVAL returned from CLK_OR_DECLARE one in this case)
>> it will lose a chance to retry it after clocks from a platform driver
>> are registered.
>>
>> A bit nasty...
>>
>
> Yes, but right now if we get an error from a provider and that's
> the only provider in the list we don't return the error. Also,
> this case should be fixed by having the first provider return
> EPROBE_DEFER for clks that aren't populated early on.

OK.  I like this idea.


> The best we can do is have the framework only return probe defer
> if there isn't a provider registered. Once a provider is
> registered, it needs to do the right thing and return the
> appropriate error (invalid or probe defer for example) at the
> right time.

Agreed.




Lastly, we have two solutions so far.  Which do you think is better?



One solution is, as others suggested,
CLK_OF_DECLARE() can allocate a bigger array than it needs,
so that blank entries can be filled by a platfrom_driver later.


The other way is,
CLK_OF_DECLARE() and a platfrom_driver
allocate separate of_clk_provider for each of them.





-- 
Best Regards
Masahiro Yamada

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web