| 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
| ||
|
Message-ID: <CAOh2x==BKyS08th2d4r0Q2UGqhzAwj6sqOgFsH6J0pWup+OU_A@mail.gmail.com>
Date: Mon, 8 Dec 2014 11:19:00 +0530
From: Viresh Kumar <viresh.kumar@...aro.org>
To: Javi Merino <javi.merino@....com>
Cc: Linux PM list <linux-pm@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
punit.agrawal@....com, Mark Brown <broonie@...nel.org>,
Zhang Rui <rui.zhang@...el.com>,
Eduardo Valentin <edubezval@...il.com>
Subject: Re: [RFC PATCH v6 6/9] thermal: cpu_cooling: implement the power
cooling device API
Hi Javi,
Looks like ARM's exchange server screwed up your patch?
This is how I see it with gmail's show-original option:
+=09cpufreq_device->dyn_power_table =3D power_table;
+=09cpufreq_device->dyn_power_table_entries =3D i;
+
I have seen this a lot, while I was in ARM. Had to adopt some work-arounds to
get over it. :)
On Sat, Dec 6, 2014 at 12:34 AM, Javi Merino <javi.merino@....com> wrote:
> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> +static int build_dyn_power_table(struct cpufreq_cooling_device *cpufreq_device,
> + u32 capacitance)
> +{
> + struct power_table *power_table;
> + struct dev_pm_opp *opp;
> + struct device *dev = NULL;
> + int num_opps, cpu, i, ret = 0;
Why not initialize num_opps and i to 0 here?
> + unsigned long freq;
> +
> + num_opps = 0;
> +
> + rcu_read_lock();
> +
> + for_each_cpu(cpu, &cpufreq_device->allowed_cpus) {
All these CPUs must be sharing the OPPs as they must be supplied
from a single clock line. But probably you need to iterate over all
because you don't know which ones share OPP. Right ? Probably
the work I am doing around getting new OPP bindings might solve
this..
> + dev = get_cpu_device(cpu);
> + if (!dev)
Is this allowed? I understand you can continue, but this is not
possible. Right ? So, print a error here?
> + continue;
> +
> + num_opps = dev_pm_opp_get_opp_count(dev);
> + if (num_opps > 0) {
> + break;
> + } else if (num_opps < 0) {
> + ret = num_opps;
> + goto unlock;
> + }
> + }
> +
> + if (num_opps == 0) {
> + ret = -EINVAL;
> + goto unlock;
> + }
> +
> + power_table = kcalloc(num_opps, sizeof(*power_table), GFP_KERNEL);
> +
> + i = 0;
Either initialize i at the beginning or in the initialization part of
for loop below.
> + for (freq = 0;
> + opp = dev_pm_opp_find_freq_ceil(dev, &freq), !IS_ERR(opp);
> + freq++) {
> + u32 freq_mhz, voltage_mv;
> + u64 power;
> +
> + freq_mhz = freq / 1000000;
> + voltage_mv = dev_pm_opp_get_voltage(opp) / 1000;
> +
> + /*
> + * Do the multiplication with MHz and millivolt so as
> + * to not overflow.
> + */
> + power = (u64)capacitance * freq_mhz * voltage_mv * voltage_mv;
> + do_div(power, 1000000000);
> +
> + /* frequency is stored in power_table in KHz */
> + power_table[i].frequency = freq / 1000;
> + power_table[i].power = power;
> +
> + i++;
Why here and not with freq++?
> + }
> +
> + if (i == 0) {
> + ret = PTR_ERR(opp);
> + goto unlock;
> + }
> +
> + cpufreq_device->dyn_power_table = power_table;
> + cpufreq_device->dyn_power_table_entries = i;
> +
> +unlock:
> + rcu_read_unlock();
> + return ret;
> +}
> +
> +static u32 cpu_freq_to_power(struct cpufreq_cooling_device *cpufreq_device,
> + u32 freq)
Because the patch is screwed up a bit, I really can't see if the 'u'
or u32 is directly
below the 's' of struct cpufreq_cooling_device. Running checkpatch with --strict
will take care of that probably. Sorry if you have already taken care of that..
> +{
> + int i;
> + struct power_table *pt = cpufreq_device->dyn_power_table;
> +
> + for (i = 1; i < cpufreq_device->dyn_power_table_entries; i++)
> + if (freq < pt[i].frequency)
> + break;
> +
> + return pt[i - 1].power;
> +}
> +static u32 get_static_power(struct cpufreq_cooling_device *cpufreq_device,
> + unsigned long freq)
> +{
> + struct device *cpu_dev;
> + struct dev_pm_opp *opp;
> + unsigned long voltage;
> + struct cpumask *cpumask = &cpufreq_device->allowed_cpus;
> + unsigned long freq_hz = freq * 1000;
> +
> + if (!cpufreq_device->plat_get_static_power)
> + return 0;
> +
> + cpu_dev = get_cpu_device(cpumask_any(cpumask));
Similar to the way you have used for-each-cpu earlier, the cpu
returned from above maynot have opps attached to it. Right ?
Probably you can keep a copy of the cpu_dev we have opps attached
with somewhere and reuse it.
> +
> + rcu_read_lock();
> +
> + opp = dev_pm_opp_find_freq_exact(cpu_dev, freq_hz, true);
So, this might fail if I am not wrong.
> + voltage = dev_pm_opp_get_voltage(opp);
> +
> + rcu_read_unlock();
> +
> + if (voltage == 0) {
> + dev_warn_ratelimited(cpu_dev,
> + "Failed to get voltage for frequency %lu: %ld\n",
> + freq_hz, IS_ERR(opp) ? PTR_ERR(opp) : 0);
> + return 0;
> + }
> +
> + return cpufreq_device->plat_get_static_power(cpumask, voltage);
> +}
--
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/
Powered by blists - more mailing lists