From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751502AbcHLHES (ORCPT ); Fri, 12 Aug 2016 03:04:18 -0400 Received: from conssluserg-06.nifty.com ([210.131.2.91]:47153 "EHLO conssluserg-06.nifty.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750878AbcHLHER (ORCPT ); Fri, 12 Aug 2016 03:04:17 -0400 DKIM-Filter: OpenDKIM Filter v2.10.3 conssluserg-06.nifty.com u7C744Ci017640 X-Nifty-SrcIP: [209.85.213.177] MIME-Version: 1.0 In-Reply-To: <20160810230845.GH2996@codeaurora.org> References: <20160804212553.GB15690@codeaurora.org> <20160808233711.GA2996@codeaurora.org> <20160810230845.GH2996@codeaurora.org> From: Masahiro Yamada Date: Fri, 12 Aug 2016 16:04:02 +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 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. -- Best Regards Masahiro Yamada