of_clk_add_(hw_)providers multipule times for one node?

of_clk_add_(hw_)providers multipule times for one node?

[Masahiro Yamada]

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?


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.



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.


Thanks.



-- 
Best Regards
Masahiro Yamada

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

[Stephen Boyd]

+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

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

[Rob Herring]

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

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

[Masahiro Yamada]

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

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

[Geert Uytterhoeven]

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

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

[Stephen Boyd]

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

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

[Masahiro Yamada]

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

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

[Stephen Boyd]

On 08/10, Masahiro Yamada wrote:
> 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>:
> >
> >>
> >> 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.

Agreed, that would be bad. We can definitely do better in that
case and properly delete the provider that we have already
registered without calling of_clk_del_provider() though. We have
everything in the local scope at the time.

> 
> 
> >>
> >> 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().

Sorry, bad choice of words. I meant users of this
of_clk_add*_provider() API.

> 
> 
> > 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.

Ok. I think I will merge my patch then to restore previous
behavior.

> 
> 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.
> 

I believe we have precedence for the former case, so there's some
momentum around that approach. It doesn't make me feel great
though because we have published the provider before all clks are
registered, and then we go back and modify the array in place
while consumers could potentially be using it. I suppose we're
saved because cpus access the pointer in the array and only see
the whole pointer and not half of the old one and half of the new
one?

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

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

[Masahiro Yamada]

2016-08-11 8:08 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>:
> On 08/10, Masahiro Yamada wrote:
>> 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>:
>> >
>> >>
>> >> 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.
>
> Agreed, that would be bad. We can definitely do better in that
> case and properly delete the provider that we have already
> registered without calling of_clk_del_provider() though. We have
> everything in the local scope at the time.
>
>>
>>
>> >>
>> >> 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().
>
> Sorry, bad choice of words. I meant users of this
> of_clk_add*_provider() API.
>
>>
>>
>> > 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.
>
> Ok. I think I will merge my patch then to restore previous
> behavior.
>
>>
>> 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.
>>
>
> I believe we have precedence for the former case, so there's some
> momentum around that approach. It doesn't make me feel great
> though because we have published the provider before all clks are
> registered, and then we go back and modify the array in place
> while consumers could potentially be using it. I suppose we're
> saved because cpus access the pointer in the array and only see
> the whole pointer and not half of the old one and half of the new
> one?


I am not sure.

But, maybe just filling the blank entries of the array seems safe.
In this case, filling should be done at the end of the probe callback.
Otherwise, devm_clk_hw_register() will free the clk_hw when the driver
is detached.




-- 
Best Regards
Masahiro Yamada

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

[Masahiro Yamada]

Hi Stephen,


2016-08-12 16:04 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>:
> 2016-08-11 8:08 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>:
>> On 08/10, Masahiro Yamada wrote:
>>> 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>:
>>> >
>>> >>
>>> >> 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.
>>
>> Agreed, that would be bad. We can definitely do better in that
>> case and properly delete the provider that we have already
>> registered without calling of_clk_del_provider() though. We have
>> everything in the local scope at the time.
>>
>>>
>>>
>>> >>
>>> >> 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().
>>
>> Sorry, bad choice of words. I meant users of this
>> of_clk_add*_provider() API.
>>
>>>
>>>
>>> > 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.
>>
>> Ok. I think I will merge my patch then to restore previous
>> behavior.
>>
>>>
>>> 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.
>>>
>>
>> I believe we have precedence for the former case, so there's some
>> momentum around that approach. It doesn't make me feel great
>> though because we have published the provider before all clks are
>> registered, and then we go back and modify the array in place
>> while consumers could potentially be using it. I suppose we're
>> saved because cpus access the pointer in the array and only see
>> the whole pointer and not half of the old one and half of the new
>> one?
>
>
> I am not sure.
>
> But, maybe just filling the blank entries of the array seems safe.
> In this case, filling should be done at the end of the probe callback.
> Otherwise, devm_clk_hw_register() will free the clk_hw when the driver
> is detached.
>

Looks like the whole of my series was rejected,
but I was not sure why the following one was rejected.
https://patchwork.kernel.org/patch/9236563/


Could you explain why -EPROBE_DEFER should be returned
if both .get_hw and .get are missing.


Is there a way to register an OF clk provider without .get(_hw),
but fill it later or something?



-- 
Best Regards
Masahiro Yamada

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

[Stephen Boyd]

(Please trim replies)

On 08/24, Masahiro Yamada wrote:
> 
> Looks like the whole of my series was rejected,
> but I was not sure why the following one was rejected.
> https://patchwork.kernel.org/patch/9236563/
> 

Replying to that patch would have been better.

> 
> Could you explain why -EPROBE_DEFER should be returned
> if both .get_hw and .get are missing.

That's just a bug. Perhaps this patch would be better, and look,
it saves 5 lines.

---8<----
 drivers/clk/clk.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 71cc56712666..d3d26148cdfb 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3174,19 +3174,14 @@ __of_clk_get_hw_from_provider(struct of_clk_provider *provider,
 			      struct of_phandle_args *clkspec)
 {
 	struct clk *clk;
-	struct clk_hw *hw = ERR_PTR(-EPROBE_DEFER);
 
-	if (provider->get_hw) {
-		hw = provider->get_hw(clkspec, provider->data);
-	} else if (provider->get) {
-		clk = provider->get(clkspec, provider->data);
-		if (!IS_ERR(clk))
-			hw = __clk_get_hw(clk);
-		else
-			hw = ERR_CAST(clk);
-	}
+	if (provider->get_hw)
+		return provider->get_hw(clkspec, provider->data);
 
-	return hw;
+	clk = provider->get(clkspec, provider->data);
+	if (IS_ERR(clk))
+		return ERR_CAST(clk);
+	return __clk_get_hw(clk);
 }
 
 struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,

> 
> 
> Is there a way to register an OF clk provider without .get(_hw),
> but fill it later or something?
> 

No.

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

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

[Masahiro Yamada]

Hi Stephen,


2016-08-25 3:08 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>:
> (Please trim replies)
>
> On 08/24, Masahiro Yamada wrote:
>>
>> Looks like the whole of my series was rejected,
>> but I was not sure why the following one was rejected.
>> https://patchwork.kernel.org/patch/9236563/
>>
>
> Replying to that patch would have been better.
>
>>
>> Could you explain why -EPROBE_DEFER should be returned
>> if both .get_hw and .get are missing.
>
> That's just a bug. Perhaps this patch would be better, and look,
> it saves 5 lines.
>
> ---8<----
>  drivers/clk/clk.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 71cc56712666..d3d26148cdfb 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3174,19 +3174,14 @@ __of_clk_get_hw_from_provider(struct of_clk_provider *provider,
>                               struct of_phandle_args *clkspec)
>  {
>         struct clk *clk;
> -       struct clk_hw *hw = ERR_PTR(-EPROBE_DEFER);
>
> -       if (provider->get_hw) {
> -               hw = provider->get_hw(clkspec, provider->data);
> -       } else if (provider->get) {
> -               clk = provider->get(clkspec, provider->data);
> -               if (!IS_ERR(clk))
> -                       hw = __clk_get_hw(clk);
> -               else
> -                       hw = ERR_CAST(clk);
> -       }
> +       if (provider->get_hw)
> +               return provider->get_hw(clkspec, provider->data);
>
> -       return hw;
> +       clk = provider->get(clkspec, provider->data);
> +       if (IS_ERR(clk))
> +               return ERR_CAST(clk);
> +       return __clk_get_hw(clk);
>  }
>
>  struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,


Good.
Could you post it as a patch file?


Thanks!



-- 
Best Regards
Masahiro Yamada

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

[Stephen Boyd]

On 08/25, Masahiro Yamada wrote:
>
> Good.
> Could you post it as a patch file?
> 

Sure. I'll write up some commit text.

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