From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753639AbcHXHMf (ORCPT ); Wed, 24 Aug 2016 03:12:35 -0400 Received: from conssluserg-06.nifty.com ([210.131.2.91]:60804 "EHLO conssluserg-06.nifty.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750969AbcHXHMd (ORCPT ); Wed, 24 Aug 2016 03:12:33 -0400 DKIM-Filter: OpenDKIM Filter v2.10.3 conssluserg-06.nifty.com u7O7BhRh020225 X-Nifty-SrcIP: [209.85.161.176] MIME-Version: 1.0 In-Reply-To: References: <20160804212553.GB15690@codeaurora.org> <20160808233711.GA2996@codeaurora.org> <20160810230845.GH2996@codeaurora.org> From: Masahiro Yamada Date: Wed, 24 Aug 2016 16:11:41 +0900 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: of_clk_add_(hw_)providers multipule times for one node? To: Stephen Boyd Cc: Rob Herring , linux-clk , Michael Turquette , Linux Kernel Mailing List Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Stephen, 2016-08-12 16:04 GMT+09:00 Masahiro Yamada : > 2016-08-11 8:08 GMT+09:00 Stephen Boyd : >> On 08/10, Masahiro Yamada wrote: >>> Hi Stephen, >>> >>> >>> >>> 2016-08-09 8:37 GMT+09:00 Stephen Boyd : >>> > On 08/08, Masahiro Yamada wrote: >>> >> Hi Stephen, >>> >> >>> >> >>> >> 2016-08-05 6:25 GMT+09:00 Stephen Boyd : >>> > >>> >> >>> >> 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