Hi Stephen,
2016-08-12 16:04 GMT+09:00 Masahiro Yamada <yamada.masah...@socionext.com>: > 2016-08-11 8:08 GMT+09:00 Stephen Boyd <sb...@codeaurora.org>: >> On 08/10, Masahiro Yamada wrote: >>> Hi Stephen, >>> >>> >>> >>> 2016-08-09 8:37 GMT+09:00 Stephen Boyd <sb...@codeaurora.org>: >>> > On 08/08, Masahiro Yamada wrote: >>> >> Hi Stephen, >>> >> >>> >> >>> >> 2016-08-05 6:25 GMT+09:00 Stephen Boyd <sb...@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