Re: [PATCH E 11/14] OMAP clock: track child clocks

[Paul Walmsley]

Hello Russell,

On Thu, 29 Jan 2009, Russell King - ARM Linux wrote:

> On Wed, Jan 28, 2009 at 12:27:59PM -0700, Paul Walmsley wrote:
> > +static int omap_clk_for_each_child(struct clk *clk, unsigned long parent_rate,
> > +		   u8 rate_storage, int (*cb)(struct clk *, unsigned long, u8))
> > +{
> > +	struct clk_child *child;
> > +	int ret;
> > +
> > +	list_for_each_entry(child, &clk->children, node) {
> > +		ret = (*cb)(child->clk, parent_rate, rate_storage);
> > +		if (ret)
> > +			break;
> > +	}
> > +
> > +	return ret;
> > +}
> 
> > +static int _do_propagate_rate(struct clk *clk, unsigned long parent_rate,
> > +			      u8 rate_storage)
> > +{
> > +	if (clk->recalc)
> > +		clk->recalc(clk, parent_rate, rate_storage);
> > +	if (omap_clk_has_children(clk))
> > +		propagate_rate(clk, rate_storage);
> > +	return 0;
> > +}
> 
> >  /* Propagate rate to children */
> >  void propagate_rate(struct clk *tclk, u8 rate_storage)
> >  {
> >  	unsigned long parent_rate = 0;
> >  
> >  	if (tclk == NULL || IS_ERR(tclk))
> >  		return;
> >  
> > +	if (rate_storage == CURRENT_RATE)
> > +		parent_rate = tclk->rate;
> > +	else if (rate_storage == TEMP_RATE)
> > +		parent_rate = tclk->temp_rate;
> >  
> > +	omap_clk_for_each_child(tclk, parent_rate, rate_storage,
> > +				_do_propagate_rate);
> >  }
> 
> This worries me.  Calling this puts onto the stack:
> 
> - a frame for propagate_rate()
> - a frame for omap_clk_for_each_child
> - a frame for _do_propagate_rate
> 
> for every level of children.  How close we get to overflowing the kernels
> depends on how much each of those functions puts on the kernel stack.
> However, since this is recursive, minimising the number of stack frames
> is a good idea.
> 
> That's why I have in my patch:
> 
>  [ARM] omap: move propagate_rate() calls into generic omap clock code
> 
> I've arranged for there to be the minimum of function nesting here.
> I suggest keeping this.

Those two new function calls are probaby not necessary for rate 
propagation, so, keeping the original single-function recursion should be 
fine.  The original motivation behind using them was to share the clock 
tree traversal code with the clock notifier patches, e.g.,
 
http://www.mail-archive.com/linux-omap@xxxxxxxxxxxxxxx/msg08068.html

Would you like a patch to remove _do_propagate_rate() and 
omap_clk_for_each_child()?


- Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/