Re: of_clk_add_(hw_)providers multipule times for one node?

[Stephen Boyd]

On 08/08, Masahiro Yamada wrote:
> Hi Stephen,
> 
> 
> 2016-08-05 6:25 GMT+09:00 Stephen Boyd <sboyd@xxxxxxxxxxxxxx>:
> > +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.

> 
> 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.

> 
> 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. 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.

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.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project