| 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: <20200312175701.GE144492@google.com>
Date: Thu, 12 Mar 2020 10:57:01 -0700
From: Matthias Kaehlcke <mka@...omium.org>
To: Lukasz Luba <lukasz.luba@....com>
Cc: Daniel Lezcano <daniel.lezcano@...aro.org>,
Chanwoo Choi <cw00.choi@...sung.com>,
MyungJoo Ham <myungjoo.ham@...sung.com>,
Kyungmin Park <kyungmin.park@...sung.com>,
Zhang Rui <rui.zhang@...el.com>,
Amit Kucheria <amit.kucheria@...durent.com>,
Leonard Crestez <leonard.crestez@....com>,
linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
Eduardo Valentin <edubezval@...il.com>
Subject: Re: [PATCH v2] thermal: devfreq_cooling: Use PM QoS to set frequency
limits
Hi Lukasz,
thanks for the review!
I'll rebase and send v3. Hopefully it doesn't cause too much extra
work for your changes.
Thanks
Matthias
On Thu, Mar 12, 2020 at 11:39:56AM +0000, Lukasz Luba wrote:
> Hi Matthias,
>
> I just saw this email below the patch. I wasn't aware that you
> are working on this. I will have to update my changes...
>
> It looks good to me.
> Unfortunately, it does not apply on top of Amit's commit
> 1b5cb9570670a6277cc0 thermal: devfreq_cooling: Appease the kernel-doc deity
>
> Could you check this?
>
> Other then that
>
> Reviewed-by: Lukasz Luba <lukasz.luba@....com>
>
> Regards,
> Lukasz
>
> On 3/12/20 12:35 AM, Matthias Kaehlcke wrote:
> > Is any further action needed from my side or can this land?
> >
> > Thanks
> >
> > Matthias
> >
> > On Fri, Jan 17, 2020 at 02:22:02PM +0900, Chanwoo Choi wrote:
> > > On 1/17/20 8:12 AM, Matthias Kaehlcke wrote:
> > > > Now that devfreq supports limiting the frequency range of a device
> > > > through PM QoS make use of it instead of disabling OPPs that should
> > > > not be used.
> > > >
> > > > The switch from disabling OPPs to PM QoS introduces a subtle behavioral
> > > > change in case of conflicting requests (min > max): PM QoS gives
> > > > precedence to the MIN_FREQUENCY request, while higher OPPs disabled
> > > > with dev_pm_opp_disable() would override MIN_FREQUENCY.
> > > >
> > > > Signed-off-by: Matthias Kaehlcke <mka@...omium.org>
> > > > ---
> > > >
> > > > Changes in v2:
> > > > - added documentation for 'req_max_freq'
> > > > - fixed jumps in of_devfreq_cooling_register_power() unwind
> > > > - added comment about behavioral change to the commit message
> > > >
> > > > drivers/thermal/devfreq_cooling.c | 70 ++++++++++---------------------
> > > > 1 file changed, 23 insertions(+), 47 deletions(-)
> > > >
> > > > diff --git a/drivers/thermal/devfreq_cooling.c b/drivers/thermal/devfreq_cooling.c
> > > > index ef59256887ff63..cbbaf5bc425d1a 100644
> > > > --- a/drivers/thermal/devfreq_cooling.c
> > > > +++ b/drivers/thermal/devfreq_cooling.c
> > > > @@ -24,11 +24,13 @@
> > > > #include <linux/idr.h>
> > > > #include <linux/slab.h>
> > > > #include <linux/pm_opp.h>
> > > > +#include <linux/pm_qos.h>
> > > > #include <linux/thermal.h>
> > > > #include <trace/events/thermal.h>
> > > > -#define SCALE_ERROR_MITIGATION 100
> > > > +#define HZ_PER_KHZ 1000
> > > > +#define SCALE_ERROR_MITIGATION 100
> > > > static DEFINE_IDA(devfreq_ida);
> > > > @@ -53,6 +55,8 @@ static DEFINE_IDA(devfreq_ida);
> > > > * 'utilization' (which is 'busy_time / 'total_time').
> > > > * The 'res_util' range is from 100 to (power_table[state] * 100)
> > > > * for the corresponding 'state'.
> > > > + * @req_max_freq: PM QoS request for limiting the maximum frequency
> > > > + * of the devfreq device.
> > > > */
> > > > struct devfreq_cooling_device {
> > > > int id;
> > > > @@ -65,49 +69,9 @@ struct devfreq_cooling_device {
> > > > struct devfreq_cooling_power *power_ops;
> > > > u32 res_util;
> > > > int capped_state;
> > > > + struct dev_pm_qos_request req_max_freq;
> > > > };
> > > > -/**
> > > > - * partition_enable_opps() - disable all opps above a given state
> > > > - * @dfc: Pointer to devfreq we are operating on
> > > > - * @cdev_state: cooling device state we're setting
> > > > - *
> > > > - * Go through the OPPs of the device, enabling all OPPs until
> > > > - * @cdev_state and disabling those frequencies above it.
> > > > - */
> > > > -static int partition_enable_opps(struct devfreq_cooling_device *dfc,
> > > > - unsigned long cdev_state)
> > > > -{
> > > > - int i;
> > > > - struct device *dev = dfc->devfreq->dev.parent;
> > > > -
> > > > - for (i = 0; i < dfc->freq_table_size; i++) {
> > > > - struct dev_pm_opp *opp;
> > > > - int ret = 0;
> > > > - unsigned int freq = dfc->freq_table[i];
> > > > - bool want_enable = i >= cdev_state ? true : false;
> > > > -
> > > > - opp = dev_pm_opp_find_freq_exact(dev, freq, !want_enable);
> > > > -
> > > > - if (PTR_ERR(opp) == -ERANGE)
> > > > - continue;
> > > > - else if (IS_ERR(opp))
> > > > - return PTR_ERR(opp);
> > > > -
> > > > - dev_pm_opp_put(opp);
> > > > -
> > > > - if (want_enable)
> > > > - ret = dev_pm_opp_enable(dev, freq);
> > > > - else
> > > > - ret = dev_pm_opp_disable(dev, freq);
> > > > -
> > > > - if (ret)
> > > > - return ret;
> > > > - }
> > > > -
> > > > - return 0;
> > > > -}
> > > > -
> > > > static int devfreq_cooling_get_max_state(struct thermal_cooling_device *cdev,
> > > > unsigned long *state)
> > > > {
> > > > @@ -134,7 +98,7 @@ static int devfreq_cooling_set_cur_state(struct thermal_cooling_device *cdev,
> > > > struct devfreq_cooling_device *dfc = cdev->devdata;
> > > > struct devfreq *df = dfc->devfreq;
> > > > struct device *dev = df->dev.parent;
> > > > - int ret;
> > > > + unsigned long freq;
> > > > if (state == dfc->cooling_state)
> > > > return 0;
> > > > @@ -144,9 +108,10 @@ static int devfreq_cooling_set_cur_state(struct thermal_cooling_device *cdev,
> > > > if (state >= dfc->freq_table_size)
> > > > return -EINVAL;
> > > > - ret = partition_enable_opps(dfc, state);
> > > > - if (ret)
> > > > - return ret;
> > > > + freq = dfc->freq_table[state];
> > > > +
> > > > + dev_pm_qos_update_request(&dfc->req_max_freq,
> > > > + DIV_ROUND_UP(freq, HZ_PER_KHZ));
> > > > dfc->cooling_state = state;
> > > > @@ -529,9 +494,15 @@ of_devfreq_cooling_register_power(struct device_node *np, struct devfreq *df,
> > > > if (err)
> > > > goto free_dfc;
> > > > - err = ida_simple_get(&devfreq_ida, 0, 0, GFP_KERNEL);
> > > > + err = dev_pm_qos_add_request(df->dev.parent, &dfc->req_max_freq,
> > > > + DEV_PM_QOS_MAX_FREQUENCY,
> > > > + PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE);
> > > > if (err < 0)
> > > > goto free_tables;
> > > > +
> > > > + err = ida_simple_get(&devfreq_ida, 0, 0, GFP_KERNEL);
> > > > + if (err < 0)
> > > > + goto remove_qos_req;
> > > > dfc->id = err;
> > > > snprintf(dev_name, sizeof(dev_name), "thermal-devfreq-%d", dfc->id);
> > > > @@ -552,6 +523,10 @@ of_devfreq_cooling_register_power(struct device_node *np, struct devfreq *df,
> > > > release_ida:
> > > > ida_simple_remove(&devfreq_ida, dfc->id);
> > > > +
> > > > +remove_qos_req:
> > > > + dev_pm_qos_remove_request(&dfc->req_max_freq);
> > > > +
> > > > free_tables:
> > > > kfree(dfc->power_table);
> > > > kfree(dfc->freq_table);
> > > > @@ -600,6 +575,7 @@ void devfreq_cooling_unregister(struct thermal_cooling_device *cdev)
> > > > thermal_cooling_device_unregister(dfc->cdev);
> > > > ida_simple_remove(&devfreq_ida, dfc->id);
> > > > + dev_pm_qos_remove_request(&dfc->req_max_freq);
> > > > kfree(dfc->power_table);
> > > > kfree(dfc->freq_table);
> > > >
> > >
> > > Reviewed-by: Chanwoo Choi <cw00.choi@...sung.com>
> > >
> > > --
> > > Best Regards,
> > > Chanwoo Choi
> > > Samsung Electronics
Powered by blists - more mailing lists