From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932353AbcHJSCs (ORCPT ); Wed, 10 Aug 2016 14:02:48 -0400 Received: from condef009-v.nifty.com ([210.131.4.246]:39102 "EHLO condef009-v.nifty.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932283AbcHJSC2 (ORCPT ); Wed, 10 Aug 2016 14:02:28 -0400 DKIM-Filter: OpenDKIM Filter v2.10.3 conssluserg-03.nifty.com u7A7xnfE025357 X-Nifty-SrcIP: [209.85.213.173] MIME-Version: 1.0 In-Reply-To: <20160808233711.GA2996@codeaurora.org> References: <20160804212553.GB15690@codeaurora.org> <20160808233711.GA2996@codeaurora.org> From: Masahiro Yamada Date: Wed, 10 Aug 2016 16:59:47 +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-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 : >> > +Rob in case he has any insight >> > >> > On 07/09, Masahiro Yamada wrote: >> >> Hi. >> >> >> >> I think the current code allows to add >> >> clk_providers multiple times against one DT node. >> >> >> >> Are there cases that really need to do so? >> > >> > If we have clk drivers that have a device driver structure and >> > also use CLK_OF_DECLARE then we could get into a situation where >> > they register two providers for the same device node. I can't >> > think of any other situation where this would happen though. >> >> >> What is the benefit for splitting one clock device >> into CLK_OF_DECLARE() and a platform_driver? >> >> >> If we go this way, I think we need to fix the current code. > > Sure. Do we have anyone registering two providers for the same > node? I'm trying to weigh the urgency of this. > >> >> 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. >> >> So, if of_clk_add_provider() fails to register a platform driver, >> it may unregister another provider added by OF_CLK_DECLARE(). > > I suppose if we loop over the list in the incorrect order we > would unregister the wrong one. Right. Here, the last added will be first deleted. >> >> 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(). > We could make > it an opaque pointer though and properly cleanup without > iterating the list. Maybe we should do that for the hw provider > API so that it simplifies the search. > >> >> >> >> > It used to return the last provider's error, but I accidentally >> > changed that behavior when adding clk_hw providers in commit >> > 0861e5b8cf80 (clk: Add clk_hw OF clk providers, 2016-02-05). >> > Nobody seems to have complained though, so you're the first to >> > have reported this. >> >> >> If we allow multiple OF-providers for one device node, >> I think any error should be treated as EPROBE_DEFER, >> i.e. the current code is good. >> >> >> The scenario is: >> >> - Clocks with ID 0 thru 3 are provided by CLK_OF_DECLARE() >> - Clocks with ID 4 thru 9 are provided by a platform driver. >> >> What if a clock consumer requests the clk ID 5 >> after CLK_OF_DECLARE(), but before the clk platform driver is registered? >> >> If the clock consumer gets the last provider's error >> (-EINVAL returned from CLK_OR_DECLARE one in this case) >> it will lose a chance to retry it after clocks from a platform driver >> are registered. >> >> A bit nasty... >> > > Yes, but right now if we get an error from a provider and that's > the only provider in the list we don't return the error. Also, > this case should be fixed by having the first provider return > EPROBE_DEFER for clks that aren't populated early on. OK. I like this idea. > 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. 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. -- Best Regards Masahiro Yamada