linux-kernel - Re: [PATCH 07/11] PM / devfreg: Add support policy notifiers

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]

Date:   Tue, 29 May 2018 13:02:40 -0700
From:   Matthias Kaehlcke <mka@...omium.org>
To:     MyungJoo Ham <myungjoo.ham@...sung.com>
Cc:     Kyungmin Park <kyungmin.park@...sung.com>,
        Chanwoo Choi <cw00.choi@...sung.com>,
        Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Brian Norris <briannorris@...omium.org>,
        Douglas Anderson <dianders@...omium.org>
Subject: Re: [PATCH 07/11] PM / devfreg: Add support policy notifiers

On Mon, May 28, 2018 at 02:19:49PM +0900, MyungJoo Ham wrote:
> >Policy notifiers are called before a frequency change and may narrow
> >the min/max frequency range in devfreq_policy, which is used to adjust
> >the target frequency if it is beyond this range.
> >
> >Also add a few helpers:
> > - devfreq_verify_within_[dev_]limits()
> >    - should be used by the notifiers for policy adjustments.
> > - dev_to_devfreq()
> >    - lookup a devfreq strict from a device pointer
> >
> >Signed-off-by: Matthias Kaehlcke <mka@...omium.org>
> >---
> > drivers/devfreq/devfreq.c | 47 +++++++++++++++++++++-------
> > include/linux/devfreq.h   | 66 +++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 102 insertions(+), 11 deletions(-)
> 
> Hello Matthias,
> 
> 
> Why should we have yet another notifier from an instance of devfreq?
> Wouldn't it better to let the current notifier (transition notifier)
> handle new events as well by adding possible event states to it?

Honestly the main reason is that I sought inspiration from cpufreq,
which uses a dedicated policy notifier. Unfortunately this change
predates the git history so I don't know what was the exact rationale
to do it this way.

Some minor advantages that I see are:

- transition notifiers aren't bothered about adjustments and viceversa
- different data types are passed for transitions and adjustments,
  which makes code of notifiers that handle both a bit more messy.

> Anyway, is this the reason why you've separated some data of devfreq
> into "policy" struct? (I was wondering why while reading commit 6/11).

The DEVFREQ_ADJUST is the reason for the "policy struct". With this
change we are dealing with 3 types of frequency pairs: user
(df->min/max_freq), devinfo (df->scaling_min/max_freq) and the
policy/adjustable ones. I think it is cleaner to group them in a
struct (and sub-structs), rather than having 6 individual
<type>_min/max_freq variables. Also it allows to only pass the policy
object to the notifiers, instead of the entire devfreq device.

I opted to do the introduction of the struct policy in a separate NOP
patch, because I think it is easier to review the 'reorg' churn
and the functional change separately.

Please let me know if you'd prefer to have certain things done
differently.

Thanks

Matthias