| 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: <20150729100032.75efc8dd@radivoje-desk2>
Date: Wed, 29 Jul 2015 10:00:32 -0700
From: Radivoje Jovanovic <radivoje.jovanovic@...ux.intel.com>
To: Punit Agrawal <punit.agrawal@....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
On Wed, 29 Jul 2015 17:46:37 +0100
Punit Agrawal <punit.agrawal@....com> wrote:
Hi Agarwal,
> [ 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?
good call. I will wait for Viresh to respond. Unless there is a better
suggestion I will push a new patch within a few days
>
> >> > + 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-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