Re: [PATCH v9 6/8] PM / devfreq: Introduce get_freq_range helper

From: Matthias Kaehlcke <mka@chromium.org>
To: MyungJoo Ham <myungjoo.ham@samsung.com>,
	Viresh Kumar <viresh.kumar@linaro.org>
Cc: "Artur Świgoń" <a.swigon@partner.samsung.com>,
	"Abel Vesa" <abel.vesa@nxp.com>,
	"Saravana Kannan" <saravanak@google.com>,
	linux-pm@vger.kernel.org, "NXP Linux Team" <linux-imx@nxp.com>,
	"Krzysztof Kozlowski" <krzk@kernel.org>,
	"Lukasz Luba" <l.luba@partner.samsung.com>,
	"Chanwoo Choi" <cw00.choi@samsung.com>,
	"Kyungmin Park" <kyungmin.park@samsung.com>,
	"Alexandre Bailon" <abailon@baylibre.com>,
	"Leonard Crestez" <leonard.crestez@nxp.com>,
	"Georgi Djakov" <georgi.djakov@linaro.org>,
	linux-arm-kernel@lists.infradead.org,
	"Jacky Bai" <ping.bai@nxp.com>
Subject: Re: [PATCH v9 6/8] PM / devfreq: Introduce get_freq_range helper
Date: Fri, 11 Oct 2019 11:29:07 -0700	[thread overview]
Message-ID: <20191011182907.GN87296@google.com> (raw)
In-Reply-To: <20191003181938.GJ87296@google.com>

On Thu, Oct 03, 2019 at 11:19:38AM -0700, Matthias Kaehlcke wrote:
> On Wed, Oct 02, 2019 at 10:25:09PM +0300, Leonard Crestez wrote:
> > Moving handling of min/max freq to a single function and call it from
> > update_devfreq and for printing min/max freq values in sysfs.
> > 
> > This changes the behavior of out-of-range min_freq/max_freq: clamping
> > is now done at evaluation time. This means that if an out-of-range
> > constraint is imposed by sysfs and it later becomes valid then it will
> > be enforced.
> > 
> > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> > Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> >  drivers/devfreq/devfreq.c | 110 +++++++++++++++++++++-----------------
> >  1 file changed, 62 insertions(+), 48 deletions(-)
> > 
> > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> > index 87eff789ce24..2d63692903ff 100644
> > --- a/drivers/devfreq/devfreq.c
> > +++ b/drivers/devfreq/devfreq.c
> >
> > ...
> >
> >  static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
> >  			     char *buf)
> >  {
> >  	struct devfreq *df = to_devfreq(dev);
> > +	unsigned long min_freq, max_freq;
> >  
> > -	return sprintf(buf, "%lu\n", max(df->scaling_min_freq, df->min_freq));
> > +	mutex_lock(&df->lock);
> > +	get_freq_range(df, &min_freq, &max_freq);
> 
> With this min/max_freq shown aren't necessarily those set through sysfs,
> but the aggregated PM QoS values (plus OPP constraints).
> 
> I did some testing with a WIP patch that converts devfreq_cooling.c to
> PM QoS. When reading sysfs min/max values to double check the limits
> set earlier I found it utterly confusing to see the sysfs min/max values
> fluctuating due to thermal throttling, and not being able to see the
> configured values.
> 
> Looks like cpufreq does the same, but I'm not convinced this is a good
> idea. I think we want to display the values set by userspace, as done
> before managing the limits through PM QoS. Viresh, was this change of
> userspace visible behavior done intentionally for cpufreq?

ping

Viresh / devfreq maintainers:

Do we really want to expose through sysfs the potentially constantly
changing aggregate min/max values, instead of those set by userspace?
At least for cpufreq that's a divergence from previous behavior, and
from a userspace perspective it seems odd that you can't reliably read
back the limit set by userspace.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel