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