[PATCH v3 05/10] clk: add support for clock protection

[Russell King - ARM Linux]

On Wed, Aug 09, 2017 at 03:34:48PM +0200, Jerome Brunet wrote:
> On Wed, 2017-08-09 at 12:45 +0100, Russell King - ARM Linux wrote:
> > On Tue, Aug 08, 2017 at 07:19:06PM -0700, Stephen Boyd wrote:
> > > I also vaguely remember Paul saying that
> > > clk_round_rate() could return something and then clk_set_rate()
> > > after that would fail because what was calculated during the rate
> > > speculation/rounding phase would be different if some other
> > > consumer goes and changes some rate high up in the tree.
> > 
> > That's probably because people tend to get this stuff wrong.  It is
> > _not_ supposed to be:
> > 
> > 	rounded_rate = clk_round_rate(clk, requested_rate);
> > 
> > 	clk_set_rate(clk, rounded_rate);
> > 
> > but:
> > 
> > 	rounded_rate = clk_round_rate(clk, requested_rate);
> > 
> > 	clk_set_rate(clk, requested_rate);
> > 
> > The former is wrong for two reasons:
> > 
> > 1. it's completely wasteful of CPU resources to do all the calculations
> >    that need to be done within clk_set_rate().
> > 
> > 2. it's racy - there is no guarantee that you'll end up with "rounded_rate"
> > 
> > The API definition of clk_round_rate() explicitly mentions that it is
> > equivalent to clk_set_rate() followed by clk_get_rate() with the
> > exception that it doesn't affect the hardware.
> > 
> > I'm not sure that the clock rate protection API is really the right
> > solution - if we're trying to stop others from changing the clock rate,
> > that implies we have multiple different threads potentially changing
> > the rate at any time.  If a driver does this:
> > 
> > 	clk_set_rate(clk, foo);
> > 	clk_rate_protect(clk);
> > 
> > what prevents another thread from changing the clock rate between these
> > two calls?  The only way to do this safely would be something like:
> > 
> > 	r = clk_round_rate(clk, foo);
> > 	while (1) {
> > 		err = clk_set_rate(clk, foo);
> > 		clk_rate_protect(clk);
> > 		if (err < 0)
> > 			break;
> > 
> > 		if (r == clk_get_rate(clk)) /* success */
> > 			break;
> > 
> > 		clk_rate_unprotect(clk);
> > 	}
> > 
> > 	if (err)
> > 		failed;
> 
> Russell,
> I think you have missed one subtle thing, when trying any clock altering
> operation, if the consumer is protecting the clock, it will temporarily release 
> the protection once, under the prepare_lock (to guarantee safe operation). This
> is explained in the cover letter:
> 
> """
> With this series there is 3 use-case:
>  - the provider is not protected: nothing changes
>  - the provider is protected by only 1 consumer (and only once), then only
>    this consumer will be able to alter the rate of the clock, as it is the
>    only one depending on it.
>  - If the provider is protected more than once, or by the provider itself,
>    the rate is basically locked and protected against reparenting.
> """
> 
> So what you should do if you have to protect the clock is:
> 
> clk_rate_protect(clk);
> err = clk_set_rate(clk, foo);
> 
> [...]
> clk_rate_unprotect(clk);

So here you drop the protection, which means anyone can alter the clock
again.

Either that or "clk_rate_unprotect" is inappropriately named and doesn't
do what it says it does.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up