| 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: <CAPDyKFobvMr0LBr0PW3L-A=dtE2jnd2RxHktN85Z9dLsi8u3TQ@mail.gmail.com>
Date: Fri, 9 Jun 2023 12:59:57 +0200
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Viresh Kumar <viresh.kumar@...aro.org>
Cc: Sudeep Holla <sudeep.holla@....com>,
Cristian Marussi <cristian.marussi@....com>,
Viresh Kumar <vireshk@...nel.org>, Nishanth Menon <nm@...com>,
Stephen Boyd <sboyd@...nel.org>,
Nikunj Kela <nkela@...cinc.com>,
Prasad Sodagudi <psodagud@...cinc.com>,
Alexandre Torgue <alexandre.torgue@...s.st.com>,
linux-pm@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 13/16] OPP: Extend dev_pm_opp_data with OPP provider support
On Fri, 9 Jun 2023 at 07:10, Viresh Kumar <viresh.kumar@...aro.org> wrote:
>
> On 08-06-23, 13:45, Ulf Hansson wrote:
> > Okay, if I understand your point you want to avoid creating OPPs for
> > each device, but rather coupling them with the genpd provider's OPP
> > table. Right?
>
> Not exactly :)
>
> > Note that, there is no such thing as a "required opp" in the SCMI
> > performance protocol case. A device is compatible to use all of the
> > OPPs that its corresponding SCMI performance domain provides. Should
> > we rename the required opp things in the OPP core to better reflect
> > this too?
>
> Not really :)
I think it may be confusing, but okay, let's leave it as is.
>
> > That said, we still need to be able to add OPPs dynamically when not
> > based on DT. The difference would be that we add the OPPs when
> > initializing the genpd provider instead of when attaching the devices.
> > In other words, we still need something along the lines of the new
> > dev_pm_opp_add_dynamic() API that $subject series is introducing, I
> > think.
>
> That's fine.
>
> > Moreover, to tie the consumer device's OPP table to their genpd
> > provider's OPP table (call it required-opp or whatever), we need
> > another OPP helper function that we can call from the genpd provider's
> > ->attach_dev() callback. Similarly, we need to be able to remove this
> > connection when genpd's ->detach_dev() callback is invoked. I will
> > think of something here.
>
> Something like set/link/config_required_opp()..
>
> > Finally, I want to point out that there is work going on in parallel
> > with this, that is adding performance state support for the ACPI PM
> > domain. The ACPI PM domain, isn't a genpd provider but implements it's
> > own PM domain. The important point is, that it will have its own
> > variant of the dev_pm_genpd_set_performance_state() that we may need
> > to call from the OPP library.
>
> Okay.
>
> Let me explain how structures are linked currently with help of below (badly
> made) drawing. The 'level' fields are only set for Genpd's OPP entries and not
> devices. The genpd OPP table normally has only this information, where it
> presents all the possible level values, separate OPP for each of them.
>
> Now the devices don't have `level` set in their OPP table, DT or in C
> structures. All they have are links for required OPPs, which help us find the
> required level or performance state for a particular genpd.
>
> +-----------------+ +------------------+
> | Device A | | GENPD OPP Table |
> +-----------------+ required-opps +------------------+ required-opps
> |OPP1: freq: x1 +--------------------+ OPP1: +--------------+
> +-----------------+ | level: 1 | |
> |OPP2: freq: y1 +-----------+ +------------------+ |
> +-----------------+ | | OPP2: +---------+ |
> |OPP3: freq: z1 +--------+ +--------+ level: 2 | | |
> +-----------------+ | +------------------+ | |
> | | OPP3: +--+ | |
> | | level: 3 | | | |
> | +------------------+ | | |
> +-----------------+ | | OPP4: | | | |
> | Device B | +-----------+ level: 4 | | | |
> +-----------------+ +------------------+ | | |
> |OPP1: freq: x2 +------------------------------------------+ | |
> +-----------------+ | |
> |OPP2: freq: y2 +-------------------------------------------------+ |
> +-----------------+ |
> |OPP3: freq: z2 +------------------------------------------------------+
> +-----------------+
>
Thanks for taking the time to explain things further! It's not
entirely easy to follow all the things in the OPP library.
However, I think you are mixing up the "level" field with the "pstate"
field in the struct dev_pm_opp. The pstate field is solely for genpd
and the required opps, while level is *generic* for all types of
devices. Right?
More precisely, _read_opp_key() parses for the "opp-level" property
from DT to set the ->level field. Additionally, the generic OPP
helpers functions, like dev_pm_opp_get_level(),
dev_pm_opp_find_level_exact(), dev_pm_opp_find_level_ceil() allows any
type of consumer driver to operate on the level for an OPP for its
device. Please have a look at the apple-soc-cpufreq, for example.
>
> What I am asking you to do now is, create an OPP table for the Genpd first, with
> OPPs for each possible level. Now the Genpd layer creates the OPP table for
> Device A, where it won't fill the levels, but all other fields and then use a
> new API to add required OPPs for the device's OPP, which will point into Genpd's
> OPP entries. And with that the existing code will work fine without any other
> modifications.
>
> Does this help ?
Well, I think what you propose may be doable, but I fail to understand
the benefit of implementing it like that.
As I said earlier, there is no such thing as a required OPP for the
SCMI performance domain and neither are the OPP tables described in
DT.
Moreover, as I understand it, we already have the generic "level" to
use, why not use it here?
Kind regards
Uffe
Powered by blists - more mailing lists