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

Groups > linux.kernel > #1460083 > 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-11 01:10 +0200
    Re: of_clk_add_(hw_)providers multipule times for one node? Masahiro Yamada <yamada.masahiro@socionext.com> - 2016-08-12 09:10 +0200
      Re: of_clk_add_(hw_)providers multipule times for one node? Masahiro Yamada <yamada.masahiro@socionext.com> - 2016-08-24 09:20 +0200
        Re: of_clk_add_(hw_)providers multipule times for one node? Stephen Boyd <sboyd@codeaurora.org> - 2016-08-24 20:20 +0200
          Re: of_clk_add_(hw_)providers multipule times for one node? Masahiro Yamada <yamada.masahiro@socionext.com> - 2016-08-25 06:30 +0200

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

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

[toc] | [next] | [standalone]

#1460977

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

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

#1469125

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

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

#1469626

(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

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

#1469799

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web