linux-kernel - Re: [PATCH v4 21/24] PM / devfreq: tegra30: Synchronize average count on target's update

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190719033152.1d4c3003@dimatab>
Date:   Fri, 19 Jul 2019 03:31:52 +0300
From:   Dmitry Osipenko <digetx@...il.com>
To:     Chanwoo Choi <cw00.choi@...sung.com>
Cc:     Thierry Reding <thierry.reding@...il.com>,
        MyungJoo Ham <myungjoo.ham@...sung.com>,
        Kyungmin Park <kyungmin.park@...sung.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Tomeu Vizoso <tomeu.vizoso@...labora.com>,
        linux-pm@...r.kernel.org, linux-tegra@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 21/24] PM / devfreq: tegra30: Synchronize average
 count on target's update

В Thu, 18 Jul 2019 19:15:54 +0900
Chanwoo Choi <cw00.choi@...sung.com> пишет:

> On 19. 7. 8. 오전 7:33, Dmitry Osipenko wrote:
> > The average count may get out of sync if interrupt was disabled /
> > avoided for a long time due to upper watermark optimization, hence
> > it should be re-synced on each target's update to ensure that
> > watermarks are set up correctly on EMC rate-change notification and
> > that a correct frequency is selected for device.
> > 
> > Signed-off-by: Dmitry Osipenko <digetx@...il.com>
> > ---
> >  drivers/devfreq/tegra30-devfreq.c | 30
> > ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)
> > 
> > diff --git a/drivers/devfreq/tegra30-devfreq.c
> > b/drivers/devfreq/tegra30-devfreq.c index
> > 4d582809acb6..8a674fad26be 100644 ---
> > a/drivers/devfreq/tegra30-devfreq.c +++
> > b/drivers/devfreq/tegra30-devfreq.c @@ -466,11 +466,41 @@ static
> > void actmon_isr_device(struct tegra_devfreq *tegra,
> > dev->boost_freq, cpufreq_get(0)); }
> >  
> > +static void tegra_devfreq_sync_avg_count(struct tegra_devfreq
> > *tegra,
> > +					 struct
> > tegra_devfreq_device *dev) +{
> > +	u32 avg_count, avg_freq, old_upper, new_upper;
> > +
> > +	avg_count = device_readl(dev, ACTMON_DEV_AVG_COUNT);
> > +	avg_freq = avg_count / ACTMON_SAMPLING_PERIOD;
> > +
> > +	old_upper = tegra_actmon_upper_freq(tegra, dev->avg_freq);
> > +	new_upper = tegra_actmon_upper_freq(tegra, avg_freq);
> > +
> > +	/* similar to ISR, see comments in actmon_isr_device() */
> > +	if (old_upper != new_upper) {
> > +		dev->avg_freq = avg_freq;
> > +		dev->boost_freq = 0;
> > +	}
> > +}
> > +
> >  static unsigned long actmon_update_target(struct tegra_devfreq
> > *tegra, struct tegra_devfreq_device *dev)
> >  {
> >  	unsigned long target_freq;
> >  
> > +	/*
> > +	 * The avg_count / avg_freq is getting snapshoted on
> > device's
> > +	 * interrupt, but there are cases where actual value need
> > to
> > +	 * be utilized on target's update, like CPUFreq boosting
> > and
> > +	 * overriding the min freq
> > via /sys/class/devfreq/devfreq0/min_freq
> > +	 * because we're optimizing the upper watermark based on
> > the
> > +	 * actual EMC frequency. This means that interrupt may be
> > +	 * inactive for a long time and thus making snapshoted
> > value
> > +	 * outdated.
> > +	 */
> > +	tegra_devfreq_sync_avg_count(tegra, dev);  
> 
> I think that you don't need to add the separate function to calculate
> the 'dev->avg_freq'. It is enough with your detailed comment to add
> this code in this function.

The separate function is indeed not mandatory here, but I'm finding that
it usually makes easier to read and follow the code when it is properly
split up into logical blocks. Don't you agree?

> > +
> >  	target_freq = min(dev->avg_freq + dev->boost_freq,
> > KHZ_MAX); target_freq = tegra_actmon_account_cpu_freq(tegra, dev,
> > target_freq); 
> >   
> 
> And also, is it impossible to squash this patch with patch19/patch20?
> 

It should be possible to squash this patch with #20, but wouldn't
be better to keep changes in the chronological order? It's also better
to keep changes separate simply to aid bisection in case of a problem.