| 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
| ||
|
Date: Wed, 29 Jul 2015 17:46:37 +0100
From: Punit Agrawal <punit.agrawal@....com>
To: Radivoje Jovanovic <radivoje.jovanovic@...ux.intel.com>
Cc: LKML <linux-kernel@...r.kernel.org>,
Linux PM <linux-pm@...r.kernel.org>,
Zhang Rui <rui.zhang@...el.com>,
Eduardo Valentin <edubezval@...il.com>,
Radivoje Jovanovic <radivoje.jovanovic@...el.com>,
viresh.kumar@...aro.org
Subject: Re: [PATCH] thermal/cpu_cooling: remove local cooling state variable
[ adding Viresh ]
Radivoje Jovanovic <radivoje.jovanovic@...ux.intel.com> writes:
> Hi Agarwal,
>
> On Fri, 24 Jul 2015 16:26:12 +0100
> Punit Agrawal <punit.agrawal@....com> wrote:
>
>> Radivoje Jovanovic <radivoje.jovanovic@...ux.intel.com> writes:
>>
>> > From: Radivoje Jovanovic <radivoje.jovanovic@...el.com>
>> >
>> > there is no need to keep local state variable. if another driver
>> > changes the policy under our feet the cpu_cooling driver will
>> > have the wrong state. Get current state from the policy directly
>> > instead
>> >
>>
>> Although the patch below looks good, it does add additional
>> processing. I was wondering in what situation do you observe the
>> problem $SUBJECT solves?
>>
>> Presumably, the policy caps are tighter than those imposed by the cpu
>> cooling device (cpufreq_thermal_notifier should take care of this).
>
> we are using this solution on the platfrom which has user space
> component control cpufreq throttling. However, user space
> component has its limitations so we are using cpu_cooling as a
> critical backup. Due to this cpu_cooling does not have correct state
> as a current state so when the change is needed cpu_cooling does
> not make the change since it believes it is in the "correct" state.
> I agree that there is slight increase in processing, but in the case
> when user space is changing the policy the notifier will not have
> access to the current state of the cpu_cooling to change it
> appropriately.
>
Makes sense. Thanks for the explanation.
One comment below.
>>
>> > Signed-off-by: Radivoje Jovanovic <radivoje.jovanovic@...el.com>
>> > ---
>> > drivers/thermal/cpu_cooling.c | 27 ++++++++++++++++++---------
>> > 1 file changed, 18 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/drivers/thermal/cpu_cooling.c
>> > b/drivers/thermal/cpu_cooling.c index 6509c61..94ba2da 100644
>> > --- a/drivers/thermal/cpu_cooling.c
>> > +++ b/drivers/thermal/cpu_cooling.c
>> > @@ -66,8 +66,6 @@ struct power_table {
>> > * registered.
>> > * @cool_dev: thermal_cooling_device pointer to keep track of the
>> > * registered cooling device.
>> > - * @cpufreq_state: integer value representing the current state of
>> > cpufreq
>> > - * cooling devices.
>> > * @cpufreq_val: integer value representing the absolute value of
>> > the clipped
>> > * frequency.
>> > * @max_level: maximum cooling level. One less than total number
>> > of valid @@ -90,7 +88,6 @@ struct power_table {
>> > struct cpufreq_cooling_device {
>> > int id;
>> > struct thermal_cooling_device *cool_dev;
>> > - unsigned int cpufreq_state;
>> > unsigned int cpufreq_val;
>> > unsigned int max_level;
>> > unsigned int *freq_table; /* In descending order */
>> > @@ -486,10 +483,19 @@ static int cpufreq_get_cur_state(struct
>> > thermal_cooling_device *cdev, unsigned long *state)
>> > {
>> > struct cpufreq_cooling_device *cpufreq_device =
>> > cdev->devdata; -
>> > - *state = cpufreq_device->cpufreq_state;
>> > -
>> > - return 0;
>> > + struct cpufreq_policy policy;
>> > + struct cpumask *mask = &cpufreq_device->allowed_cpus;
>> > + unsigned int cpu = cpumask_any(mask);
>> > + unsigned int cur_state;
>> > +
>> > + if (!cpufreq_get_policy(&policy, cpu)) {
The above call returns an error for an offline cpu, but you can still
get a valid policy if any of the allowed_cpus are online. It might make
sense to loop over allowed_cpus until the call succeeds or you run out
of cpus.
Viresh, do you have a better suggestion?
>> > + cur_state = get_level(cpufreq_device,
>> > policy.max);
>> > + if (cur_state != THERMAL_CSTATE_INVALID) {
>> > + *state = cur_state;
>> > + return 0;
>> > + }
>> > + }
>> > + return -EINVAL;
>> > }
>> >
>> > /**
>> > @@ -508,17 +514,20 @@ static int cpufreq_set_cur_state(struct
>> > thermal_cooling_device *cdev, struct cpufreq_cooling_device
>> > *cpufreq_device = cdev->devdata; unsigned int cpu =
>> > cpumask_any(&cpufreq_device->allowed_cpus); unsigned int clip_freq;
>> > + unsigned long cur_state;
>> >
>> > /* Request state should be less than max_level */
>> > if (WARN_ON(state > cpufreq_device->max_level))
>> > return -EINVAL;
>> >
>> > + if (cpufreq_get_cur_state(cpufreq_device->cool_dev,
>> > &cur_state))
>> > + return -EINVAL;
>> > +
>> > /* Check if the old cooling action is same as new cooling
>> > action */
>> > - if (cpufreq_device->cpufreq_state == state)
>> > + if (cur_state == state)
>> > return 0;
>> >
>> > clip_freq = cpufreq_device->freq_table[state];
>> > - cpufreq_device->cpufreq_state = state;
>> > cpufreq_device->cpufreq_val = clip_freq;
>> >
>> > cpufreq_update_policy(cpu);
>
> Thanks
> Ogi
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
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