From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933941AbcHJXIu (ORCPT ); Wed, 10 Aug 2016 19:08:50 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:51239 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932635AbcHJXIr (ORCPT ); Wed, 10 Aug 2016 19:08:47 -0400 Date: Wed, 10 Aug 2016 16:08:45 -0700 From: Stephen Boyd To: Masahiro Yamada Cc: Rob Herring , linux-clk , Michael Turquette , Linux Kernel Mailing List Subject: Re: of_clk_add_(hw_)providers multipule times for one node? Message-ID: <20160810230845.GH2996@codeaurora.org> References: <20160804212553.GB15690@codeaurora.org> <20160808233711.GA2996@codeaurora.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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? -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project