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

[Masahiro Yamada]

2016-08-11 8:08 GMT+09:00 Stephen Boyd <sboyd@xxxxxxxxxxxxxx>:
> On 08/10, Masahiro Yamada wrote:
>> Hi Stephen,
>>
>>
>>
>> 2016-08-09 8:37 GMT+09:00 Stephen Boyd <sboyd@xxxxxxxxxxxxxx>:
>> > On 08/08, Masahiro Yamada wrote:
>> >> Hi Stephen,
>> >>
>> >>
>> >> 2016-08-05 6:25 GMT+09:00 Stephen Boyd <sboyd@xxxxxxxxxxxxxx>:
>> >
>> >>
>> >> 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