linux-kernel - Re: [PATCH v8 6/7] clk: Add floor and ceiling constraints to clock rates

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>] [day] [month] [year] [list]

Message-ID: <5407222F.9040200@collabora.com>
Date:	Wed, 03 Sep 2014 16:14:07 +0200
From:	Tomeu Vizoso <tomeu.vizoso@...labora.com>
To:	Mike Turquette <mturquette@...aro.org>
CC:	Stephen Warren <swarren@...dotorg.org>,
	Thierry Reding <thierry.reding@...il.com>,
	Tomasz Figa <tomasz.figa@...il.com>,
	Peter De Schrijver <pdeschrijver@...dia.com>, rabin@....in,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	Javier Martinez Canillas <javier.martinez@...labora.co.uk>
Subject: Re: [PATCH v8 6/7] clk: Add floor and ceiling constraints to clock
 rates

On 09/03/2014 02:13 AM, Mike Turquette wrote:
> Quoting Tomeu Vizoso (2014-09-01 08:34:34)
>> @@ -1633,6 +1636,13 @@ int clk_provider_set_rate(struct clk_core *clk, unsigned long rate)
>>          /* prevent racing with updates to the clock topology */
>>          clk_prepare_lock();
>>
>> +       hlist_for_each_entry(clk_user, &clk->per_user_clks, child_node) {
>> +               rate = max(rate, clk_user->floor_constraint);
>> +
>> +               if (clk_user->ceiling_constraint > 0)
>> +                       rate = min(rate, clk_user->ceiling_constraint);
>
> A ceiling_constraint from consumer_A could be less than a
> floor_constraint from consumer_B. What should we do in this case?
>
> In the code above the ceiling_constraint will always win. Is that by
> design? We should document that behavior in Documentation/clk.txt.
>
> This is the right place to check for the aforementioned corner case,
> since we not only care about a single consumer having sane constraints
> (e.g. min < max) but also mixing constraints across consumers.

Yeah. I think I lean towards first applying all floors, then applying 
all ceilings. Because hardware damage could happen if a ceiling from 
thermal isn't applied because of a bug in some other driver.

This also has the advantage of being deterministic, when with the 
current approach the result depends on the order in which the per-user 
clocks are iterated.

> However ...
>
>> +       }
>> +
>>          /* bail early if nothing to do */
>>          if (rate == clk_provider_get_rate(clk))
>>                  goto out;
>> @@ -1699,6 +1709,24 @@ int clk_set_rate(struct clk *clk_user, unsigned long rate)
>>   }
>>   EXPORT_SYMBOL_GPL(clk_set_rate);
>>
>> +int clk_set_floor_rate(struct clk *clk_user, unsigned long rate)
>> +{
>> +       struct clk_core *clk = clk_to_clk_core(clk_user);
>> +
>> +       clk_user->floor_constraint = rate;
>> +       return clk_provider_set_rate(clk, clk_provider_get_rate(clk));
>> +}
>> +EXPORT_SYMBOL_GPL(clk_set_floor_rate);
>> +
>> +int clk_set_ceiling_rate(struct clk *clk_user, unsigned long rate)
>> +{
>> +       struct clk_core *clk = clk_to_clk_core(clk_user);
>> +
>> +       clk_user->ceiling_constraint = rate;
>> +       return clk_provider_set_rate(clk, clk_provider_get_rate(clk));
>> +}
>> +EXPORT_SYMBOL_GPL(clk_set_ceiling_rate);
>
> ... we should probably sanity-check constraints here to make sure that
> ceiling_rates for a given consumer are higher than floor_constraints for
> that same consumer. It's a bit extra overhead but a WARN would probably
> be helpful in this case.

Sounds like a good idea to me, will do.

Thanks,

Tomeu

> Rest of the patch looks good.
>
> Regards,
> Mike
>

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