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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 2 Feb 2009 00:57:39 -0700 (MST)
From:	Paul Walmsley <paul@...an.com>
To:	Russell King - ARM Linux <linux@....linux.org.uk>
cc:	linux-arm-kernel@...ts.arm.linux.org.uk,
	linux-kernel@...r.kernel.org, linux-omap@...r.kernel.org,
	Tony Lindgren <tony@...mide.com>
Subject: Re: [PATCH E 11/14] OMAP clock: track child clocks

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@vger.kernel.org/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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/