* [PATCH RFC v2 0/2] Use cpu based scaling passive governor for MT8183 CCI
From: Hsin-Yi Wang @ 2019-07-17 6:11 UTC (permalink / raw)
To: linux-arm-kernel
Cc: MyungJoo Ham ), Kyungmin Park, Chanwoo Choi, Matthias Brugger,
Rafael J . Wysocki, Viresh Kumar, linux-pm, linux-mediatek,
linux-kernel
This series depends on following series:
1. mt8183 cpufreq and cci devfreq from andrew-sh.cheng
https://patchwork.kernel.org/cover/10946047/
2. cpu based scaling support to passive_governor from Sibi Sankar
https://lore.kernel.org/patchwork/patch/1101049/
This series uses cpu based scaling passive governor for mt8183-cci to improve
performance and deal with shared regulator voltage setting issue.
Hsin-Yi Wang (2):
devfreq: mt8183-cci: using cpu based scaling passive_governor
cpufreq: mediatek: Support vproc shared by multiple component
drivers/cpufreq/mediatek-cpufreq.c | 16 +-
drivers/devfreq/mt8183-cci-devfreq.c | 239 +++++++--------------------
2 files changed, 68 insertions(+), 187 deletions(-)
--
2.20.1
^ permalink raw reply
* Re: [PATCH] opp: Return genpd virtual devices from dev_pm_opp_attach_genpd()
From: Viresh Kumar @ 2019-07-17 5:47 UTC (permalink / raw)
To: Rajendra Nayak
Cc: Viresh Kumar, Nishanth Menon, Stephen Boyd, Rafael J. Wysocki,
linux-pm, Vincent Guittot, linux-kernel
In-Reply-To: <2ed7993d-523b-270a-2be9-83ad2426e946@codeaurora.org>
On 11-07-19, 15:09, Rajendra Nayak wrote:
> Sorry for the delay
Same here :)
> I seem to have completely missed this patch.
> I just gave this a try and here are some observations,
>
> I have a case where I have one device with 2 power domains, one of them
> is scale-able (supports perf state) and the other one supports only being
> turned on and off.
>
> 1. In the driver I now need to use dev_pm_domain_attach_by_name/id to attach the
> power domain which supports only on/off and then use dev_pm_opp_attach_genpd()
> for the one which supports perf states.
>
> 2. My OPP table has only 1 required_opps, so the required_opp_count for the OPP table is 1.
> Now if my device tree has my scale-able powerdomain at index 1 (it works if its at index 0)
> then I end up with this error
>
> [ 2.858628] ufshcd-qcom 1d84000.ufshc: Index can't be greater than required-opp-count - 1, rpmh_pd (1 : 1)
>
> so it looks like a lot of the OPP core today just assumes that if a device has multiple power domains,
> all of them are scale-able which isn't necessarily true.
I don't think a lot of OPP core has these problems, but maybe only
this place. I was taking care of this since the beginning just forgot
it now.
What about this over this commit:
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index d76ead4eff4c..1f11f8c92337 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1789,13 +1789,16 @@ static void _opp_detach_genpd(struct opp_table *opp_table)
*
* This helper needs to be called once with a list of all genpd to attach.
* Otherwise the original device structure will be used instead by the OPP core.
+ *
+ * The order of entries in the names array must match the order in which
+ * "required-opps" are added in DT.
*/
struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
const char **names, struct device ***virt_devs)
{
struct opp_table *opp_table;
struct device *virt_dev;
- int index, ret = -EINVAL;
+ int index = 0, ret = -EINVAL;
const char **name = names;
opp_table = dev_pm_opp_get_opp_table(dev);
@@ -1821,14 +1824,6 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
goto unlock;
while (*name) {
- index = of_property_match_string(dev->of_node,
- "power-domain-names", *name);
- if (index < 0) {
- dev_err(dev, "Failed to find power domain: %s (%d)\n",
- *name, index);
- goto err;
- }
-
if (index >= opp_table->required_opp_count) {
dev_err(dev, "Index can't be greater than required-opp-count - 1, %s (%d : %d)\n",
*name, opp_table->required_opp_count, index);
@@ -1849,6 +1844,7 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
}
opp_table->genpd_virt_devs[index] = virt_dev;
+ index++;
name++;
}
--
viresh
^ permalink raw reply related
* Re: [PATCH v7] cpufreq/pasemi: fix an use-after-free in pas_cpufreq_cpu_init()
From: Viresh Kumar @ 2019-07-17 4:26 UTC (permalink / raw)
To: Wen Yang
Cc: rjw, linuxppc-dev, linux-pm, linux-kernel, xue.zhihong, wang.yi59,
cheng.shengyu, Michael Ellerman
In-Reply-To: <1563335704-25562-1-git-send-email-wen.yang99@zte.com.cn>
On 17-07-19, 11:55, Wen Yang wrote:
> The cpu variable is still being used in the of_get_property() call
> after the of_node_put() call, which may result in use-after-free.
>
> Fixes: a9acc26b75f6 ("cpufreq/pasemi: fix possible object reference leak")
> Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux-pm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
> v7: adapt to commit ("cpufreq: Make cpufreq_generic_init() return void")
> v6: keep the blank line and fix warning: label 'out_unmap_sdcpwr' defined but not used.
> v5: put together the code to get, use, and release cpu device_node.
> v4: restore the blank line.
> v3: fix a leaked reference.
> v2: clean up the code according to the advice of viresh.
>
> drivers/cpufreq/pasemi-cpufreq.c | 23 +++++++++--------------
> 1 file changed, 9 insertions(+), 14 deletions(-)
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
--
viresh
^ permalink raw reply
* [PATCH v7] cpufreq/pasemi: fix an use-after-free in pas_cpufreq_cpu_init()
From: Wen Yang @ 2019-07-17 3:55 UTC (permalink / raw)
To: rjw
Cc: viresh.kumar, linuxppc-dev, linux-pm, linux-kernel, xue.zhihong,
wang.yi59, cheng.shengyu, Wen Yang, Michael Ellerman
The cpu variable is still being used in the of_get_property() call
after the of_node_put() call, which may result in use-after-free.
Fixes: a9acc26b75f6 ("cpufreq/pasemi: fix possible object reference leak")
Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-pm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
v7: adapt to commit ("cpufreq: Make cpufreq_generic_init() return void")
v6: keep the blank line and fix warning: label 'out_unmap_sdcpwr' defined but not used.
v5: put together the code to get, use, and release cpu device_node.
v4: restore the blank line.
v3: fix a leaked reference.
v2: clean up the code according to the advice of viresh.
drivers/cpufreq/pasemi-cpufreq.c | 23 +++++++++--------------
1 file changed, 9 insertions(+), 14 deletions(-)
diff --git a/drivers/cpufreq/pasemi-cpufreq.c b/drivers/cpufreq/pasemi-cpufreq.c
index 93f39a1..c66f566 100644
--- a/drivers/cpufreq/pasemi-cpufreq.c
+++ b/drivers/cpufreq/pasemi-cpufreq.c
@@ -131,10 +131,18 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy *policy)
int err = -ENODEV;
cpu = of_get_cpu_node(policy->cpu, NULL);
+ if (!cpu)
+ goto out;
+ max_freqp = of_get_property(cpu, "clock-frequency", NULL);
of_node_put(cpu);
- if (!cpu)
+ if (!max_freqp) {
+ err = -EINVAL;
goto out;
+ }
+
+ /* we need the freq in kHz */
+ max_freq = *max_freqp / 1000;
dn = of_find_compatible_node(NULL, NULL, "1682m-sdc");
if (!dn)
@@ -171,16 +179,6 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy *policy)
}
pr_debug("init cpufreq on CPU %d\n", policy->cpu);
-
- max_freqp = of_get_property(cpu, "clock-frequency", NULL);
- if (!max_freqp) {
- err = -EINVAL;
- goto out_unmap_sdcpwr;
- }
-
- /* we need the freq in kHz */
- max_freq = *max_freqp / 1000;
-
pr_debug("max clock-frequency is at %u kHz\n", max_freq);
pr_debug("initializing frequency table\n");
@@ -199,9 +197,6 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy *policy)
cpufreq_generic_init(policy, pas_freqs, get_gizmo_latency());
return 0;
-out_unmap_sdcpwr:
- iounmap(sdcpwr_mapbase);
-
out_unmap_sdcasr:
iounmap(sdcasr_mapbase);
out:
--
2.9.5
^ permalink raw reply related
* Re: [PATCH 02/10] video: sa1100fb: Remove cpufreq policy notifier
From: Viresh Kumar @ 2019-07-17 3:02 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz
Cc: Rafael Wysocki, linux-pm, Vincent Guittot, dri-devel, linux-fbdev,
linux-kernel
In-Reply-To: <8b0f9adf-8462-eeba-afd1-52201f4e7f47@samsung.com>
On 16-07-19, 14:25, Bartlomiej Zolnierkiewicz wrote:
>
> Hi Viresh,
>
> Please always Cc: me on fbdev patches.
That happened because I used patter-depth=1 in my script for finding
maintainers from get_maintainers. Sorry about that. I have incremented
that by one now.
--
viresh
^ permalink raw reply
* Re: [PATCH] opp: Return genpd virtual devices from dev_pm_opp_attach_genpd()
From: Viresh Kumar @ 2019-07-17 2:52 UTC (permalink / raw)
To: Niklas Cassel
Cc: Viresh Kumar, Nishanth Menon, Stephen Boyd, Rafael J. Wysocki,
linux-pm, Vincent Guittot, Rajendra Nayak, linux-kernel
In-Reply-To: <20190716104354.GA13780@centauri>
On 16-07-19, 12:43, Niklas Cassel wrote:
> On Mon, Jul 08, 2019 at 11:30:11AM +0530, Viresh Kumar wrote:
> > The cpufreq drivers don't need to do runtime PM operations on the
> > virtual devices returned by dev_pm_domain_attach_by_name() and so the
> > virtual devices weren't shared with the callers of
> > dev_pm_opp_attach_genpd() earlier.
> >
> > But the IO device drivers would want to do that. This patch updates the
> > prototype of dev_pm_opp_attach_genpd() to accept another argument to
> > return the pointer to the array of genpd virtual devices.
> >
> > Reported-by: Rajendra Nayak <rnayak@codeaurora.org>
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> > @Rajendra: Can you please test this one ? I have only compile tested it.
> >
> > drivers/opp/core.c | 5 ++++-
> > include/linux/pm_opp.h | 4 ++--
> > 2 files changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > index 2958cc7bbb58..07b6f1187b3b 100644
> > --- a/drivers/opp/core.c
> > +++ b/drivers/opp/core.c
> > @@ -1775,6 +1775,7 @@ static void _opp_detach_genpd(struct opp_table *opp_table)
> > * dev_pm_opp_attach_genpd - Attach genpd(s) for the device and save virtual device pointer
> > * @dev: Consumer device for which the genpd is getting attached.
> > * @names: Null terminated array of pointers containing names of genpd to attach.
> > + * @virt_devs: Pointer to return the array of virtual devices.
> > *
> > * Multiple generic power domains for a device are supported with the help of
> > * virtual genpd devices, which are created for each consumer device - genpd
> > @@ -1789,7 +1790,8 @@ static void _opp_detach_genpd(struct opp_table *opp_table)
> > * This helper needs to be called once with a list of all genpd to attach.
> > * Otherwise the original device structure will be used instead by the OPP core.
> > */
> > -struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char **names)
> > +struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
> > + const char **names, struct device ***virt_devs)
> > {
> > struct opp_table *opp_table;
> > struct device *virt_dev;
> > @@ -1850,6 +1852,7 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char **names
> > name++;
> > }
> >
> > + *virt_devs = opp_table->genpd_virt_devs;
>
> Could we perhaps only do this if (virt_devs), that way callers can send in
> NULL if they don't care about the genpd virtual devices.
That was the idea and I failed to add it :(
--
viresh
^ permalink raw reply
* Re: [PATCH 2/9] rcu: Add support for consolidated-RCU reader checking (v3)
From: Paul E. McKenney @ 2019-07-17 0:07 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
Josh Triplett, keescook, kernel-hardening, kernel-team,
Lai Jiangshan, Len Brown, linux-acpi, linux-doc, linux-pci,
linux-pm, Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190716220205.GB172157@google.com>
On Tue, Jul 16, 2019 at 06:02:05PM -0400, Joel Fernandes wrote:
> On Tue, Jul 16, 2019 at 11:53:03AM -0700, Paul E. McKenney wrote:
> [snip]
> > > > A few more things below.
> > > > > ---
> > > > > include/linux/rculist.h | 28 ++++++++++++++++++++-----
> > > > > include/linux/rcupdate.h | 7 +++++++
> > > > > kernel/rcu/Kconfig.debug | 11 ++++++++++
> > > > > kernel/rcu/update.c | 44 ++++++++++++++++++++++++----------------
> > > > > 4 files changed, 67 insertions(+), 23 deletions(-)
> > > > >
> > > > > diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> > > > > index e91ec9ddcd30..1048160625bb 100644
> > > > > --- a/include/linux/rculist.h
> > > > > +++ b/include/linux/rculist.h
> > > > > @@ -40,6 +40,20 @@ static inline void INIT_LIST_HEAD_RCU(struct list_head *list)
> > > > > */
> > > > > #define list_next_rcu(list) (*((struct list_head __rcu **)(&(list)->next)))
> > > > >
> > > > > +/*
> > > > > + * Check during list traversal that we are within an RCU reader
> > > > > + */
> > > > > +
> > > > > +#ifdef CONFIG_PROVE_RCU_LIST
> > > >
> > > > This new Kconfig option is OK temporarily, but unless there is reason to
> > > > fear malfunction that a few weeks of rcutorture, 0day, and -next won't
> > > > find, it would be better to just use CONFIG_PROVE_RCU. The overall goal
> > > > is to reduce the number of RCU knobs rather than grow them, must though
> > > > history might lead one to believe otherwise. :-/
> > >
> > > If you want, we can try to drop this option and just use PROVE_RCU however I
> > > must say there may be several warnings that need to be fixed in a short
> > > period of time (even a few weeks may be too short) considering the 1000+
> > > uses of RCU lists.
> > Do many people other than me build with CONFIG_PROVE_RCU? If so, then
> > that would be a good reason for a temporary CONFIG_PROVE_RCU_LIST,
> > as in going away in a release or two once the warnings get fixed.
>
> PROVE_RCU is enabled by default with PROVE_LOCKING, so it is used quite
> heavilty.
>
> > > But I don't mind dropping it and it may just accelerate the fixing up of all
> > > callers.
> >
> > I will let you decide based on the above question. But if you have
> > CONFIG_PROVE_RCU_LIST, as noted below, it needs to depend on RCU_EXPERT.
>
> Ok, will make it depend. But yes for temporary purpose, I will leave it as a
> config and remove it later.
Very good, thank you! Plus you got another ack. ;-)
Thanx, Paul
^ permalink raw reply
* Re: [PATCH 2/9] rcu: Add support for consolidated-RCU reader checking (v3)
From: Joel Fernandes @ 2019-07-16 22:02 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
Josh Triplett, keescook, kernel-hardening, kernel-team,
Lai Jiangshan, Len Brown, linux-acpi, linux-doc, linux-pci,
linux-pm, Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190716185303.GM14271@linux.ibm.com>
On Tue, Jul 16, 2019 at 11:53:03AM -0700, Paul E. McKenney wrote:
[snip]
> > > A few more things below.
> > > > ---
> > > > include/linux/rculist.h | 28 ++++++++++++++++++++-----
> > > > include/linux/rcupdate.h | 7 +++++++
> > > > kernel/rcu/Kconfig.debug | 11 ++++++++++
> > > > kernel/rcu/update.c | 44 ++++++++++++++++++++++++----------------
> > > > 4 files changed, 67 insertions(+), 23 deletions(-)
> > > >
> > > > diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> > > > index e91ec9ddcd30..1048160625bb 100644
> > > > --- a/include/linux/rculist.h
> > > > +++ b/include/linux/rculist.h
> > > > @@ -40,6 +40,20 @@ static inline void INIT_LIST_HEAD_RCU(struct list_head *list)
> > > > */
> > > > #define list_next_rcu(list) (*((struct list_head __rcu **)(&(list)->next)))
> > > >
> > > > +/*
> > > > + * Check during list traversal that we are within an RCU reader
> > > > + */
> > > > +
> > > > +#ifdef CONFIG_PROVE_RCU_LIST
> > >
> > > This new Kconfig option is OK temporarily, but unless there is reason to
> > > fear malfunction that a few weeks of rcutorture, 0day, and -next won't
> > > find, it would be better to just use CONFIG_PROVE_RCU. The overall goal
> > > is to reduce the number of RCU knobs rather than grow them, must though
> > > history might lead one to believe otherwise. :-/
> >
> > If you want, we can try to drop this option and just use PROVE_RCU however I
> > must say there may be several warnings that need to be fixed in a short
> > period of time (even a few weeks may be too short) considering the 1000+
> > uses of RCU lists.
> Do many people other than me build with CONFIG_PROVE_RCU? If so, then
> that would be a good reason for a temporary CONFIG_PROVE_RCU_LIST,
> as in going away in a release or two once the warnings get fixed.
PROVE_RCU is enabled by default with PROVE_LOCKING, so it is used quite
heavilty.
> > But I don't mind dropping it and it may just accelerate the fixing up of all
> > callers.
>
> I will let you decide based on the above question. But if you have
> CONFIG_PROVE_RCU_LIST, as noted below, it needs to depend on RCU_EXPERT.
Ok, will make it depend. But yes for temporary purpose, I will leave it as a
config and remove it later.
thanks,
- Joel
^ permalink raw reply
* Re: [PATCH] cpuidle: Always stop scheduler tick on adaptive-tick CPUs
From: Frederic Weisbecker @ 2019-07-16 21:40 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Linux PM, Thomas Lindroth, LKML, Peter Zijlstra
In-Reply-To: <6254683.2O5gIZElE2@kreacher>
On Tue, Jul 16, 2019 at 05:25:10PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Running the scheduler tick on idle adaptive-tick CPUs is not useful
Judging by the below change, you mean full dynticks, right?
> and it may also be not expected by users (as reported by Thomas), so
> add a check to cpuidle_idle_call() to always stop the tick on them
> regardless of the idle duration predicted by the governor.
>
> Fixes: 554c8aa8ecad ("sched: idle: Select idle state before stopping the tick")
> Reported-by: Thomas Lindroth <thomas.lindroth@gmail.com>
> Tested-by: Thomas Lindroth <thomas.lindroth@gmail.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> kernel/sched/idle.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> Index: linux-pm/kernel/sched/idle.c
> ===================================================================
> --- linux-pm.orig/kernel/sched/idle.c
> +++ linux-pm/kernel/sched/idle.c
> @@ -191,7 +191,8 @@ static void cpuidle_idle_call(void)
> */
> next_state = cpuidle_select(drv, dev, &stop_tick);
>
> - if (stop_tick || tick_nohz_tick_stopped())
> + if (stop_tick || tick_nohz_tick_stopped() ||
> + !housekeeping_cpu(dev->cpu, HK_FLAG_TICK))
But tick_nohz_tick_stopped() also works on full dynticks CPUs. If the
tick isn't stopped on a full dynticks CPU by the time we reach this path,
it means that the conditions for the tick to be stopped are not met anyway
(eg: more than one task and sched tick is needed, perf event requires the tick,
posix CPU timer, etc...)
Or am I missing something else?
Thanks.
> tick_nohz_idle_stop_tick();
> else
> tick_nohz_idle_retain_tick();
>
>
>
^ permalink raw reply
* Re: [PATCH 4/9] ipv4: add lockdep condition to fix for_each_entry (v1)
From: David Miller @ 2019-07-16 21:12 UTC (permalink / raw)
To: paulmck
Cc: joel, linux-kernel, kuznet, bhelgaas, bp, c0d1n61at3, edumazet,
gregkh, yoshfuji, hpa, mingo, corbet, josh, keescook,
kernel-hardening, kernel-team, jiangshanlai, lenb, linux-acpi,
linux-doc, linux-pci, linux-pm, mathieu.desnoyers, neilb, netdev,
oleg, pavel, peterz, rjw, rasmus.villemoes, rcu, rostedt, tj,
tglx, will, x86
In-Reply-To: <20190716183955.GF14271@linux.ibm.com>
From: "Paul E. McKenney" <paulmck@linux.ibm.com>
Date: Tue, 16 Jul 2019 11:39:55 -0700
> On Mon, Jul 15, 2019 at 10:37:00AM -0400, Joel Fernandes (Google) wrote:
>> Using the previous support added, use it for adding lockdep conditions
>> to list usage here.
>>
>> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>
> We need an ack or better from the subsystem maintainer for this one.
Acked-by: David S. Miller <davem@davemloft.net>
^ permalink raw reply
* Re: [PATCH 17/18] arm64: dts: Convert to the hierarchical CPU topology layout for MSM8916
From: Lina Iyer @ 2019-07-16 20:36 UTC (permalink / raw)
To: Sudeep Holla
Cc: Ulf Hansson, Lorenzo Pieralisi, Mark Rutland, linux-arm-kernel,
Rafael J . Wysocki, Daniel Lezcano, Raju P . L . S . S . S . N,
Amit Kucheria, Bjorn Andersson, Stephen Boyd, Niklas Cassel,
Tony Lindgren, Kevin Hilman, Viresh Kumar, Vincent Guittot,
Geert Uytterhoeven, Souvik Chakravarty, linux-pm, linux-arm-msm,
linux-kernel, Lina Iyer, Andy Gross, David Brown
In-Reply-To: <20190716144730.GA7250@e107155-lin>
On Tue, Jul 16 2019 at 08:47 -0600, Sudeep Holla wrote:
>On Mon, May 13, 2019 at 09:22:59PM +0200, Ulf Hansson wrote:
>> From: Lina Iyer <lina.iyer@linaro.org>
>>
>> In the hierarchical layout, we are creating power domains around each CPU
>> and describes the idle states for them inside the power domain provider
>> node. Note that, the CPU's idle states still needs to be compatible with
>> "arm,idle-state".
>>
>> Furthermore, represent the CPU cluster as a separate master power domain,
>> powering the CPU's power domains. The cluster node, contains the idle
>> states for the cluster and each idle state needs to be compatible with the
>> "domain-idle-state".
>>
>> If the running platform is using a PSCI FW that supports the OS initiated
>> CPU suspend mode, which likely should be the case unless the PSCI FW is
>> very old, this change triggers the PSCI driver to enable it.
>>
>> Cc: Andy Gross <andy.gross@linaro.org>
>> Cc: David Brown <david.brown@linaro.org>
>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>> Co-developed-by: Ulf Hansson <ulf.hansson@linaro.org>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>
>[...]
>
>> @@ -166,12 +170,57 @@
>> min-residency-us = <2000>;
>> local-timer-stop;
>> };
>> +
>> + CLUSTER_RET: cluster-retention {
>> + compatible = "domain-idle-state";
>> + arm,psci-suspend-param = <0x1000010>;
>> + entry-latency-us = <500>;
>> + exit-latency-us = <500>;
>> + min-residency-us = <2000>;
>> + };
>> +
>> + CLUSTER_PWRDN: cluster-gdhs {
>> + compatible = "domain-idle-state";
>> + arm,psci-suspend-param = <0x1000030>;
>> + entry-latency-us = <2000>;
>> + exit-latency-us = <2000>;
>> + min-residency-us = <6000>;
>> + };
>> };
>> };
>
>I was trying to understand the composition of composite state parameters
>in this series and that made me look at these DT examples.
>
This was meant to depict a hierarchical state format for OSI.
>What format does the above platform use ? I tried matching them to
>both original as well as extended format and I fail to understand.
>Assuming original format:
> State power_state PowerLevel StateType StateID
> SPC 0x40000002 0(core) 0(Retention) 0x2 (Res0 b[29]=1?)
> CLUSTER_RET 0x1000010 1(clusters) 0(Retention) 0x10
> CLUSTER_PWRDN 0x1000030 1(clusters) 0(Retention?) 0x30
>Now extended format:
> State power_state StateType StateID
> SPC 0x40000002 0(Retention) 0x40000002 (Res0 b[29]=1?)
> CLUSTER_RET 0x1000010 0(Retention) 0x1000010
The composite state would comprise of CPU state and Cluster state.
So for the last CPU entering idle -
(CLUSTER_RET | SPC)
0x41000012
> CLUSTER_PWRDN 0x1000030 0(Retention?) 0x1000030
>
(CLUSTER_PWRDN | SPC)
0x41000032
Hope this helps.
Lina
^ permalink raw reply
* Re: [PATCH v2 2/2] interconnect: qcom: Add tagging and wake/sleep support for sdm845
From: Evan Green @ 2019-07-16 20:15 UTC (permalink / raw)
To: David Dai
Cc: Georgi Djakov, linux-pm, Vincent Guittot, Bjorn Andersson,
amit.kucheria, Doug Anderson, Sean Sweeney, LKML,
linux-arm Mailing List, linux-arm-msm
In-Reply-To: <05d9fea0-c040-d609-38bf-11cddbe6aa4d@codeaurora.org>
On Mon, Jul 15, 2019 at 4:34 PM David Dai <daidavid1@codeaurora.org> wrote:
>
> Hi Evan,
>
> Thanks for the continued help in reviewing these patches!
No problem. I want to do more, but haven't found time to do the
prerequisite research before jumping into some of the other
discussions yet.
>
> On 7/11/2019 10:06 AM, Evan Green wrote:
> > Hi Georgi and David,
> >
> > On Tue, Jun 18, 2019 at 2:17 AM Georgi Djakov <georgi.djakov@linaro.org> wrote:
> >> From: David Dai <daidavid1@codeaurora.org>
> >>
> >> Add support for wake and sleep commands by using a tag to indicate
> >> whether or not the aggregate and set requests fall into execution
> >> state specific bucket.
> >>
> >> Signed-off-by: David Dai <daidavid1@codeaurora.org>
> >> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
> >> ---
> >> drivers/interconnect/qcom/sdm845.c | 129 ++++++++++++++++++++++-------
> >> 1 file changed, 98 insertions(+), 31 deletions(-)
> >>
> >> diff --git a/drivers/interconnect/qcom/sdm845.c b/drivers/interconnect/qcom/sdm845.c
> >> index fb526004c82e..c100aab39415 100644
> >> --- a/drivers/interconnect/qcom/sdm845.c
> >> +++ b/drivers/interconnect/qcom/sdm845.c
> >> @@ -66,6 +66,17 @@ struct bcm_db {
> >> #define SDM845_MAX_BCM_PER_NODE 2
> >> #define SDM845_MAX_VCD 10
> >>
> >> +#define QCOM_ICC_BUCKET_AMC 0
> > What is AMC again? Is it the "right now" bucket? Maybe a comment on
> > the meaning of this bucket would be helpful.
> That's correct. Will add a comment for this.
> >
> >> +#define QCOM_ICC_BUCKET_WAKE 1
> >> +#define QCOM_ICC_BUCKET_SLEEP 2
> >> +#define QCOM_ICC_NUM_BUCKETS 3
> >> +#define QCOM_ICC_TAG_AMC BIT(QCOM_ICC_BUCKET_AMC)
> >> +#define QCOM_ICC_TAG_WAKE BIT(QCOM_ICC_BUCKET_WAKE)
> >> +#define QCOM_ICC_TAG_SLEEP BIT(QCOM_ICC_BUCKET_SLEEP)
> >> +#define QCOM_ICC_TAG_ACTIVE_ONLY (QCOM_ICC_TAG_AMC | QCOM_ICC_TAG_WAKE)
> >> +#define QCOM_ICC_TAG_ALWAYS (QCOM_ICC_TAG_AMC | QCOM_ICC_TAG_WAKE |\
> >> + QCOM_ICC_TAG_SLEEP)
> >> +
> >> /**
> >> * struct qcom_icc_node - Qualcomm specific interconnect nodes
> >> * @name: the node name used in debugfs
> >> @@ -75,7 +86,9 @@ struct bcm_db {
> >> * @channels: num of channels at this node
> >> * @buswidth: width of the interconnect between a node and the bus
> >> * @sum_avg: current sum aggregate value of all avg bw requests
> >> + * @sum_avg_cached: previous sum aggregate value of all avg bw requests
> >> * @max_peak: current max aggregate value of all peak bw requests
> >> + * @max_peak_cached: previous max aggregate value of all peak bw requests
> >> * @bcms: list of bcms associated with this logical node
> >> * @num_bcms: num of @bcms
> >> */
> >> @@ -86,8 +99,10 @@ struct qcom_icc_node {
> >> u16 num_links;
> >> u16 channels;
> >> u16 buswidth;
> >> - u64 sum_avg;
> >> - u64 max_peak;
> >> + u64 sum_avg[QCOM_ICC_NUM_BUCKETS];
> >> + u64 sum_avg_cached[QCOM_ICC_NUM_BUCKETS];
> >> + u64 max_peak[QCOM_ICC_NUM_BUCKETS];
> >> + u64 max_peak_cached[QCOM_ICC_NUM_BUCKETS];
> >> struct qcom_icc_bcm *bcms[SDM845_MAX_BCM_PER_NODE];
> >> size_t num_bcms;
> >> };
> >> @@ -112,8 +127,8 @@ struct qcom_icc_bcm {
> >> const char *name;
> >> u32 type;
> >> u32 addr;
> >> - u64 vote_x;
> >> - u64 vote_y;
> >> + u64 vote_x[QCOM_ICC_NUM_BUCKETS];
> >> + u64 vote_y[QCOM_ICC_NUM_BUCKETS];
> >> bool dirty;
> >> bool keepalive;
> >> struct bcm_db aux_data;
> >> @@ -555,7 +570,7 @@ inline void tcs_cmd_gen(struct tcs_cmd *cmd, u64 vote_x, u64 vote_y,
> >> cmd->wait = true;
> >> }
> >>
> >> -static void tcs_list_gen(struct list_head *bcm_list,
> >> +static void tcs_list_gen(struct list_head *bcm_list, int bucket,
> >> struct tcs_cmd tcs_list[SDM845_MAX_VCD],
> >> int n[SDM845_MAX_VCD])
> >> {
> >> @@ -573,8 +588,8 @@ static void tcs_list_gen(struct list_head *bcm_list,
> >> commit = true;
> >> cur_vcd_size = 0;
> >> }
> >> - tcs_cmd_gen(&tcs_list[idx], bcm->vote_x, bcm->vote_y,
> >> - bcm->addr, commit);
> >> + tcs_cmd_gen(&tcs_list[idx], bcm->vote_x[bucket],
> >> + bcm->vote_y[bucket], bcm->addr, commit);
> >> idx++;
> >> n[batch]++;
> >> /*
> >> @@ -595,32 +610,39 @@ static void tcs_list_gen(struct list_head *bcm_list,
> >>
> >> static void bcm_aggregate(struct qcom_icc_bcm *bcm)
> >> {
> >> - size_t i;
> >> - u64 agg_avg = 0;
> >> - u64 agg_peak = 0;
> >> + size_t i, bucket;
> >> + u64 agg_avg[QCOM_ICC_NUM_BUCKETS] = {0};
> >> + u64 agg_peak[QCOM_ICC_NUM_BUCKETS] = {0};
> >> u64 temp;
> >>
> >> - for (i = 0; i < bcm->num_nodes; i++) {
> >> - temp = bcm->nodes[i]->sum_avg * bcm->aux_data.width;
> >> - do_div(temp, bcm->nodes[i]->buswidth * bcm->nodes[i]->channels);
> >> - agg_avg = max(agg_avg, temp);
> >> + for (bucket = 0; bucket < QCOM_ICC_NUM_BUCKETS; bucket++) {
> >> + for (i = 0; i < bcm->num_nodes; i++) {
> >> + temp = bcm->nodes[i]->sum_avg_cached[bucket] * bcm->aux_data.width;
> >> + do_div(temp, bcm->nodes[i]->buswidth * bcm->nodes[i]->channels);
> >> + agg_avg[bucket] = max(agg_avg[bucket], temp);
> >>
> >> - temp = bcm->nodes[i]->max_peak * bcm->aux_data.width;
> >> - do_div(temp, bcm->nodes[i]->buswidth);
> > Why is it that this one doesn't have the multiply by
> > bcm->nodes[i]->channels again? I can't recall if there was a reason.
> > If it's correct maybe it deserves a comment.
>
> I think the rationale behind this is generally for consumers to target a
> certain minimum threshold to satisfy some structural latency
> requirements as opposed to strictly throughput, and it may be easier for
> consumers to reuse certain values to support hitting some minimum NoC
> frequencies without having to be concerned with the number of channels
> that may change from platform to platform.
I was mostly pointing out that sum_avg seems to have the multiply, but
max_peak does not. I would have expected those two things to be of the
same units, and get the same treatment. Maybe the hardware is taking
in different final units for that field, one that is per-channel and
one that isn't?
>
> >
> >> - agg_peak = max(agg_peak, temp);
> >> - }
> >> + temp = bcm->nodes[i]->max_peak_cached[bucket] * bcm->aux_data.width;
> >> + do_div(temp, bcm->nodes[i]->buswidth);
> >> + agg_peak[bucket] = max(agg_peak[bucket], temp);
> >>
> >> - temp = agg_avg * 1000ULL;
> >> - do_div(temp, bcm->aux_data.unit);
> >> - bcm->vote_x = temp;
> >> + bcm->nodes[i]->sum_avg[bucket] = 0;
> >> + bcm->nodes[i]->max_peak[bucket] = 0;
> > I don't understand the sum_avg vs sum_avg_cached. Here's what I understand:
> > 1. qcom_icc_aggregate() does the math from the incoming values on
> > sum_avg, and then clobbers sum_avg_cached with those values.
> > 2. bcm_aggregate() uses sum_avg_cached in its calculations, then clears sum_avg.
> >
> > But I don't get why that's needed. Why not just have sum_avg? Wouldn't
> > it work the same? Ok, it wouldn't if you ended up calling
> > bcm_aggregate() multiple times on the same bcm. But you have a dirty
> > flag that prevents this from happening. So I think it's safe to remove
> > the cached arrays, and just clear out the sum_avg when you aggregate.
> You are correct in that the dirty flag would prevent another repeat of
> the bcm_aggregate() call in the same icc_set request. But consider a
> following icc_set request on a different node that shares the same BCM,
> the next bcm_aggregate() would result in an incorrect aggregate sum_avg
> for the BCM since the avg_sum from the previous node(from the previous
> icc_set) was cleared out. We need a way to retain the current state of
> all nodes to accurately aggregate the bw values for the BCM.
I don't get it. qcom_icc_aggregate() clobbers sum_avg_cached. So
they're only ever a) equal, like after qcom_icc_aggregate(), or b)
sum_avg is zeroed, and sum_avg_cached is its old value. A new
icc_set_bw() would call aggregate_requests(), which would clobber
sum_avg_cached to sum_avg for every BCM involved. Then the core would
call apply_constraints(), then qcom_icc_set(), which would use
sum_avg_cached, and clear out sum_avg, being sure with the dirty flag
that bcm_aggregate() is only called once per BCM. This all happens
under the mutex held in the core. A new request would start the whole
thing over, since sum_avg is cleared. It seems to me that flow would
work the same with one array as it does with two. Maybe you can walk
me through a scenario?
-Evan
> >> + }
> >>
> >> - temp = agg_peak * 1000ULL;
> >> - do_div(temp, bcm->aux_data.unit);
> >> - bcm->vote_y = temp;
> >> + temp = agg_avg[bucket] * 1000ULL;
> >> + do_div(temp, bcm->aux_data.unit);
> >> + bcm->vote_x[bucket] = temp;
> >>
> >> - if (bcm->keepalive && bcm->vote_x == 0 && bcm->vote_y == 0) {
> >> - bcm->vote_x = 1;
> >> - bcm->vote_y = 1;
> >> + temp = agg_peak[bucket] * 1000ULL;
> >> + do_div(temp, bcm->aux_data.unit);
> >> + bcm->vote_y[bucket] = temp;
> >> + }
> >> +
> >> + if (bcm->keepalive && bcm->vote_x[0] == 0 && bcm->vote_y[0] == 0) {
> >> + bcm->vote_x[QCOM_ICC_BUCKET_AMC] = 1;
> >> + bcm->vote_x[QCOM_ICC_BUCKET_WAKE] = 1;
> >> + bcm->vote_y[QCOM_ICC_BUCKET_AMC] = 1;
> >> + bcm->vote_y[QCOM_ICC_BUCKET_WAKE] = 1;
> >> }
> >>
> >> bcm->dirty = false;
> >> @@ -631,15 +653,25 @@ static int qcom_icc_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
> >> {
> >> size_t i;
> >> struct qcom_icc_node *qn;
> >> + unsigned long tag_word = (unsigned long)tag;
> >>
> >> qn = node->data;
> >>
> >> + if (!tag)
> >> + tag_word = QCOM_ICC_TAG_ALWAYS;
> >> +
> >> + for (i = 0; i < QCOM_ICC_NUM_BUCKETS; i++) {
> >> + if (test_bit(i, &tag_word)) {
> > I guess all this extra business with tag_word and casting is so that
> > you can use test_bit, which is presumably a tiny bit faster? Does this
> > actually make a measurable difference? Maybe in the name of simplicity
> > we just do if (tag & BIT(i)), and then optimize if we find that
> > conditional to be a hotspot?
> Using (tag & BIT(i)) as opposed to test_bit seems reasonable to me.
> >> + qn->sum_avg[i] += avg_bw;
> >> + qn->max_peak[i] = max_t(u32, qn->max_peak[i], peak_bw);
> >> + qn->sum_avg_cached[i] = qn->sum_avg[i];
> >> + qn->max_peak_cached[i] = qn->max_peak[i];
> >> + }
> >> + }
> >> +
> >> *agg_avg += avg_bw;
> >> *agg_peak = max_t(u32, *agg_peak, peak_bw);
> >>
> >> - qn->sum_avg = *agg_avg;
> >> - qn->max_peak = *agg_peak;
> >> -
> >> for (i = 0; i < qn->num_bcms; i++)
> >> qn->bcms[i]->dirty = true;
> >>
> >> @@ -675,7 +707,7 @@ static int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
> >> * Construct the command list based on a pre ordered list of BCMs
> >> * based on VCD.
> >> */
> >> - tcs_list_gen(&commit_list, cmds, commit_idx);
> >> + tcs_list_gen(&commit_list, QCOM_ICC_BUCKET_AMC, cmds, commit_idx);
> >>
> >> if (!commit_idx[0])
> >> return ret;
> >> @@ -693,6 +725,41 @@ static int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
> >> return ret;
> >> }
> >>
> >> + INIT_LIST_HEAD(&commit_list);
> >> +
> >> + for (i = 0; i < qp->num_bcms; i++) {
> >> + /*
> >> + * Only generate WAKE and SLEEP commands if a resource's
> >> + * requirements change as the execution environment transitions
> >> + * between different power states.
> >> + */
> >> + if (qp->bcms[i]->vote_x[QCOM_ICC_BUCKET_WAKE] !=
> >> + qp->bcms[i]->vote_x[QCOM_ICC_BUCKET_SLEEP] ||
> >> + qp->bcms[i]->vote_y[QCOM_ICC_BUCKET_WAKE] !=
> >> + qp->bcms[i]->vote_y[QCOM_ICC_BUCKET_SLEEP]) {
> >> + list_add_tail(&qp->bcms[i]->list, &commit_list);
> >> + }
> >> + }
> >> +
> >> + if (list_empty(&commit_list))
> >> + return ret;
> >> +
> >> + tcs_list_gen(&commit_list, QCOM_ICC_BUCKET_WAKE, cmds, commit_idx);
> >> +
> >> + ret = rpmh_write_batch(qp->dev, RPMH_WAKE_ONLY_STATE, cmds, commit_idx);
> >> + if (ret) {
> >> + pr_err("Error sending WAKE RPMH requests (%d)\n", ret);
> >> + return ret;
> >> + }
> >> +
> >> + tcs_list_gen(&commit_list, QCOM_ICC_BUCKET_SLEEP, cmds, commit_idx);
> >> +
> >> + ret = rpmh_write_batch(qp->dev, RPMH_SLEEP_STATE, cmds, commit_idx);
> >> + if (ret) {
> >> + pr_err("Error sending SLEEP RPMH requests (%d)\n", ret);
> >> + return ret;
> >> + }
> >> +
> >> return ret;
> >> }
> >>
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
^ permalink raw reply
* Re: [PATCH RFC v2] PM / devfreq: Add cpu based scaling support to passive_governor
From: Saravana Kannan @ 2019-07-16 19:23 UTC (permalink / raw)
To: Sibi Sankar
Cc: MyungJoo Ham, Chanwoo Choi, Kyungmin Park, Viresh Kumar,
Nishanth Menon, Stephen Boyd, Georgi Djakov, agross, david.brown,
Rob Herring, Mark Rutland, Rafael J. Wysocki, linux-arm-msm,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
Linux PM, hsinyi, adharmap, Saravana Kannan
In-Reply-To: <20190716133700.30024-1-sibis@codeaurora.org>
On Tue, Jul 16, 2019 at 6:37 AM Sibi Sankar <sibis@codeaurora.org> wrote:
>
> From: Saravana Kannan <skannan@codeaurora.org>
>
> Many CPU architectures have caches that can scale independent of the
> CPUs. Frequency scaling of the caches is necessary to make sure that the
> cache is not a performance bottleneck that leads to poor performance and
> power. The same idea applies for RAM/DDR.
>
> To achieve this, this patch adds support for cpu based scaling to the
> passive governor. This is accomplished by taking the current frequency
> of each CPU frequency domain and then adjust the frequency of the cache
> (or any devfreq device) based on the frequency of the CPUs. It listens
> to CPU frequency transition notifiers to keep itself up to date on the
> current CPU frequency.
>
> To decide the frequency of the device, the governor does one of the
> following:
> * Derives the optimal devfreq device opp from required-opps property of
> the parent cpu opp_table.
>
> * Scales the device frequency in proportion to the CPU frequency. So, if
> the CPUs are running at their max frequency, the device runs at its
> max frequency. If the CPUs are running at their min frequency, the
> device runs at its min frequency. It is interpolated for frequencies
> in between.
>
> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> [Sibi: Integrated cpu-freqmap governor into passive_governor]
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
> v2:
> * Address Chanwoo's review comments
>
> This patch is a re-work of:
> https://patchwork.kernel.org/patch/10553171/
>
> This patch depends on Saravana's add required-opps support series:
> https://patchwork.kernel.org/cover/11016423/
>
> drivers/devfreq/Kconfig | 2 +
> drivers/devfreq/governor_passive.c | 275 +++++++++++++++++++++++++++--
> include/linux/devfreq.h | 40 ++++-
> 3 files changed, 296 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index f3b242987fd91..5db8633f94d83 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -73,6 +73,8 @@ config DEVFREQ_GOV_PASSIVE
> device. This governor does not change the frequency by itself
> through sysfs entries. The passive governor recommends that
> devfreq device uses the OPP table to get the frequency/voltage.
> + Alternatively the governor can also be chosen to scale based on
> + the online CPUs current frequency.
>
> comment "DEVFREQ Drivers"
>
> diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
> index 24ce94c80f06e..743b6093cc732 100644
> --- a/drivers/devfreq/governor_passive.c
> +++ b/drivers/devfreq/governor_passive.c
> @@ -8,11 +8,86 @@
> */
>
> #include <linux/module.h>
> +#include <linux/cpu.h>
> +#include <linux/cpufreq.h>
> +#include <linux/cpumask.h>
> #include <linux/device.h>
> #include <linux/devfreq.h>
> +#include <linux/slab.h>
> #include "governor.h"
>
> -static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
> +static unsigned int xlate_cpufreq_to_devfreq(struct devfreq_passive_data *data,
> + unsigned int cpu)
> +{
> + unsigned int cpu_min, cpu_max, dev_min, dev_max, cpu_percent, max_state;
> + struct devfreq_cpu_state *cpu_state = data->cpu_state[cpu];
> + struct devfreq *devfreq = (struct devfreq *)data->this;
> + unsigned long *freq_table = devfreq->profile->freq_table;
> + struct dev_pm_opp *opp = NULL, *cpu_opp = NULL;
> + unsigned long cpu_freq, freq;
> +
> + if (!cpu_state || cpu_state->first_cpu != cpu ||
> + !cpu_state->opp_table || !devfreq->opp_table)
> + return 0;
> +
> + cpu_freq = cpu_state->freq * 1000;
> + cpu_opp = devfreq_recommended_opp(cpu_state->dev, &cpu_freq, 0);
> + if (IS_ERR(cpu_opp))
> + return 0;
> +
> + opp = dev_pm_opp_xlate_opp(cpu_state->opp_table,
> + devfreq->opp_table, cpu_opp);
> + dev_pm_opp_put(cpu_opp);
> +
> + if (!IS_ERR(opp)) {
> + freq = dev_pm_opp_get_freq(opp);
> + dev_pm_opp_put(opp);
> + } else {
> + /* Use Interpolation if required opps is not available */
> + cpu_min = cpu_state->min_freq;
> + cpu_max = cpu_state->max_freq;
> + cpu_freq = cpu_state->freq;
> +
> + if (freq_table) {
> + /* Get minimum frequency according to sorting order */
> + max_state = freq_table[devfreq->profile->max_state - 1];
> + if (freq_table[0] < max_state) {
> + dev_min = freq_table[0];
> + dev_max = max_state;
> + } else {
> + dev_min = max_state;
> + dev_max = freq_table[0];
> + }
> + } else {
> + if (devfreq->max_freq <= devfreq->min_freq)
> + return 0;
> + dev_min = devfreq->min_freq;
> + dev_max = devfreq->max_freq;
> + }
> + cpu_percent = ((cpu_freq - cpu_min) * 100) / cpu_max - cpu_min;
> + freq = dev_min + mult_frac(dev_max - dev_min, cpu_percent, 100);
> + }
> +
> + return freq;
> +}
> +
> +static int get_target_freq_with_cpufreq(struct devfreq *devfreq,
> + unsigned long *freq)
> +{
> + struct devfreq_passive_data *p_data =
> + (struct devfreq_passive_data *)devfreq->data;
> + unsigned int cpu, target_freq = 0;
> +
> + for_each_online_cpu(cpu)
> + target_freq = max(target_freq,
> + xlate_cpufreq_to_devfreq(p_data, cpu));
> +
> + *freq = target_freq;
> +
> + return 0;
> +}
> +
> +static int get_target_freq_with_devfreq(struct devfreq *devfreq,
> unsigned long *freq)
> {
> struct devfreq_passive_data *p_data
> @@ -22,16 +97,6 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
> struct dev_pm_opp *opp = NULL, *p_opp = NULL;
> int i, count, ret = 0;
>
> - /*
> - * If the devfreq device with passive governor has the specific method
> - * to determine the next frequency, should use the get_target_freq()
> - * of struct devfreq_passive_data.
> - */
> - if (p_data->get_target_freq) {
> - ret = p_data->get_target_freq(devfreq, freq);
> - goto out;
> - }
> -
> /*
> * If the parent and passive devfreq device uses the OPP table,
> * get the next frequency by using the OPP table.
> @@ -102,6 +167,37 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
> return ret;
> }
>
> +static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
> + unsigned long *freq)
> +{
> + struct devfreq_passive_data *p_data =
> + (struct devfreq_passive_data *)devfreq->data;
> + int ret;
> +
> + /*
> + * If the devfreq device with passive governor has the specific method
> + * to determine the next frequency, should use the get_target_freq()
> + * of struct devfreq_passive_data.
> + */
> + if (p_data->get_target_freq)
> + return p_data->get_target_freq(devfreq, freq);
> +
> + switch (p_data->parent_type) {
> + case DEVFREQ_PARENT_DEV:
> + ret = get_target_freq_with_devfreq(devfreq, freq);
> + break;
> + case CPUFREQ_PARENT_DEV:
> + ret = get_target_freq_with_cpufreq(devfreq, freq);
> + break;
> + default:
> + ret = -EINVAL;
> + dev_err(&devfreq->dev, "Invalid parent type\n");
> + break;
> + }
> +
> + return ret;
> +}
> +
> static int update_devfreq_passive(struct devfreq *devfreq, unsigned long freq)
> {
> int ret;
> @@ -156,6 +252,140 @@ static int devfreq_passive_notifier_call(struct notifier_block *nb,
> return NOTIFY_DONE;
> }
>
> +static int cpufreq_passive_notifier_call(struct notifier_block *nb,
> + unsigned long event, void *ptr)
> +{
> + struct devfreq_passive_data *data =
> + container_of(nb, struct devfreq_passive_data, nb);
> + struct devfreq *devfreq = (struct devfreq *)data->this;
> + struct devfreq_cpu_state *cpu_state;
> + struct cpufreq_freqs *freq = ptr;
> + unsigned int current_freq;
> + int ret;
> +
> + if (event != CPUFREQ_POSTCHANGE || !freq ||
> + !data->cpu_state[freq->policy->cpu])
> + return 0;
> +
> + cpu_state = data->cpu_state[freq->policy->cpu];
> + if (cpu_state->freq == freq->new)
> + return 0;
> +
> + /* Backup current freq and pre-update cpu state freq*/
> + current_freq = cpu_state->freq;
> + cpu_state->freq = freq->new;
> +
> + mutex_lock(&devfreq->lock);
> + ret = update_devfreq(devfreq);
> + mutex_unlock(&devfreq->lock);
> + if (ret) {
> + cpu_state->freq = current_freq;
> + dev_err(&devfreq->dev, "Couldn't update the frequency.\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int cpufreq_passive_register(struct devfreq_passive_data **p_data)
> +{
> + struct devfreq_passive_data *data = *p_data;
> + struct devfreq *devfreq = (struct devfreq *)data->this;
> + struct device *dev = devfreq->dev.parent;
> + struct opp_table *opp_table = NULL;
> + struct devfreq_cpu_state *state;
> + struct cpufreq_policy *policy;
> + struct device *cpu_dev;
> + unsigned int cpu;
> + int ret;
> +
> + get_online_cpus();
> + data->nb.notifier_call = cpufreq_passive_notifier_call;
> + ret = cpufreq_register_notifier(&data->nb,
> + CPUFREQ_TRANSITION_NOTIFIER);
> + if (ret) {
> + dev_err(dev, "Couldn't register cpufreq notifier.\n");
> + data->nb.notifier_call = NULL;
> + goto out;
> + }
> +
> + /* Populate devfreq_cpu_state */
> + for_each_online_cpu(cpu) {
> + if (data->cpu_state[cpu])
> + continue;
> +
> + policy = cpufreq_cpu_get(cpu);
> + if (policy) {
> + state = kzalloc(sizeof(*state), GFP_KERNEL);
> + if (!state) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + cpu_dev = get_cpu_device(cpu);
> + if (!cpu_dev) {
> + dev_err(dev, "Couldn't get cpu device.\n");
> + ret = -ENODEV;
> + goto out;
> + }
> +
> + opp_table = dev_pm_opp_get_opp_table(cpu_dev);
> + if (IS_ERR(devfreq->opp_table)) {
> + ret = PTR_ERR(opp_table);
> + goto out;
> + }
> +
> + state->dev = cpu_dev;
> + state->opp_table = opp_table;
> + state->first_cpu = cpumask_first(policy->related_cpus);
> + state->freq = policy->cur;
> + state->min_freq = policy->cpuinfo.min_freq;
> + state->max_freq = policy->cpuinfo.max_freq;
> + data->cpu_state[cpu] = state;
> + cpufreq_cpu_put(policy);
> + } else {
> + ret = -EPROBE_DEFER;
> + goto out;
> + }
> + }
> +out:
> + put_online_cpus();
> + if (ret)
> + return ret;
> +
> + /* Update devfreq */
> + mutex_lock(&devfreq->lock);
> + ret = update_devfreq(devfreq);
> + mutex_unlock(&devfreq->lock);
> + if (ret)
> + dev_err(dev, "Couldn't update the frequency.\n");
> +
> + return ret;
> +}
> +
> +static int cpufreq_passive_unregister(struct devfreq_passive_data **p_data)
> +{
> + struct devfreq_passive_data *data = *p_data;
> + struct devfreq_cpu_state *cpu_state;
> + int cpu;
> +
> + if (data->nb.notifier_call)
> + cpufreq_unregister_notifier(&data->nb,
> + CPUFREQ_TRANSITION_NOTIFIER);
> +
> + for_each_possible_cpu(cpu) {
> + cpu_state = data->cpu_state[cpu];
> + if (cpu_state) {
> + if (cpu_state->opp_table)
> + dev_pm_opp_put_opp_table(cpu_state->opp_table);
> + kfree(cpu_state);
> + cpu_state = NULL;
> + }
> + }
> +
> + return 0;
> +}
> +
> static int devfreq_passive_event_handler(struct devfreq *devfreq,
> unsigned int event, void *data)
> {
> @@ -166,7 +396,7 @@ static int devfreq_passive_event_handler(struct devfreq *devfreq,
> struct notifier_block *nb = &p_data->nb;
> int ret = 0;
>
> - if (!parent)
> + if (p_data->parent_type == DEVFREQ_PARENT_DEV && !parent)
> return -EPROBE_DEFER;
>
> switch (event) {
> @@ -174,13 +404,24 @@ static int devfreq_passive_event_handler(struct devfreq *devfreq,
> if (!p_data->this)
> p_data->this = devfreq;
>
> - nb->notifier_call = devfreq_passive_notifier_call;
> - ret = devm_devfreq_register_notifier(dev, parent, nb,
> - DEVFREQ_TRANSITION_NOTIFIER);
> + if (p_data->parent_type == DEVFREQ_PARENT_DEV) {
> + nb->notifier_call = devfreq_passive_notifier_call;
> + ret = devm_devfreq_register_notifier(dev, parent, nb,
> + DEVFREQ_TRANSITION_NOTIFIER);
> + } else if (p_data->parent_type == CPUFREQ_PARENT_DEV) {
> + ret = cpufreq_passive_register(&p_data);
> + } else {
> + ret = -EINVAL;
> + }
> break;
> case DEVFREQ_GOV_STOP:
> - devm_devfreq_unregister_notifier(dev, parent, nb,
> - DEVFREQ_TRANSITION_NOTIFIER);
> + if (p_data->parent_type == DEVFREQ_PARENT_DEV)
> + devm_devfreq_unregister_notifier(dev, parent, nb,
> + DEVFREQ_TRANSITION_NOTIFIER);
> + else if (p_data->parent_type == CPUFREQ_PARENT_DEV)
> + cpufreq_passive_unregister(&p_data);
> + else
> + ret = -EINVAL;
> break;
> default:
> break;
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index 69d487329408e..f3f898dfd09b2 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -263,6 +263,32 @@ struct devfreq_simple_ondemand_data {
> #endif
>
> #if IS_ENABLED(CONFIG_DEVFREQ_GOV_PASSIVE)
> +/**
> + * struct devfreq_cpu_state - holds the per-cpu state
> + * @freq: the current frequency of the cpu.
> + * @min_freq: the min frequency of the cpu.
> + * @max_freq: the max frequency of the cpu.
> + * @first_cpu: the cpumask of the first cpu of a policy.
> + * @dev: reference to cpu device.
> + * @opp_table: reference to cpu opp table.
> + *
> + * This structure stores the required cpu_state of a cpu.
> + * This is auto-populated by the governor.
> + */
> +struct devfreq_cpu_state {
> + unsigned int freq;
> + unsigned int min_freq;
> + unsigned int max_freq;
> + unsigned int first_cpu;
> + struct device *dev;
> + struct opp_table *opp_table;
> +};
> +
> +enum devfreq_parent_dev_type {
> + DEVFREQ_PARENT_DEV,
> + CPUFREQ_PARENT_DEV,
> +};
> +
> /**
> * struct devfreq_passive_data - void *data fed to struct devfreq
> * and devfreq_add_device
> @@ -274,13 +300,15 @@ struct devfreq_simple_ondemand_data {
> * using governors except for passive governor.
> * If the devfreq device has the specific method to decide
> * the next frequency, should use this callback.
> - * @this: the devfreq instance of own device.
> - * @nb: the notifier block for DEVFREQ_TRANSITION_NOTIFIER list
> + * @parent_type parent type of the device
> + * @this: the devfreq instance of own device.
> + * @nb: the notifier block for DEVFREQ_TRANSITION_NOTIFIER list
> + * @cpu_state: the state min/max/current frequency of all online cpu's
> *
> * The devfreq_passive_data have to set the devfreq instance of parent
> * device with governors except for the passive governor. But, don't need to
> - * initialize the 'this' and 'nb' field because the devfreq core will handle
> - * them.
> + * initialize the 'this', 'nb' and 'cpu_state' field because the devfreq core
> + * will handle them.
> */
> struct devfreq_passive_data {
> /* Should set the devfreq instance of parent device */
> @@ -289,9 +317,13 @@ struct devfreq_passive_data {
> /* Optional callback to decide the next frequency of passvice device */
> int (*get_target_freq)(struct devfreq *this, unsigned long *freq);
>
> + /* Should set the type of parent device */
> + enum devfreq_parent_dev_type parent_type;
> +
> /* For passive governor's internal use. Don't need to set them */
> struct devfreq *this;
> struct notifier_block nb;
> + struct devfreq_cpu_state *cpu_state[NR_CPUS];
> };
> #endif
Just as an FYI, I wrote this code a long time ago. I was planning on
rewriting the integration of CPU freq inputs to devfreq anyway because
notifiers are discouraged and I've had other ideas on how I could do
it. But I haven't dug into that yet.
Honestly I'd prefer waiting till the required-opps support for passive
governor patch is merged before digging too much into this. And I plan
to take a stab at this after that.
Thanks,
Saravana
^ permalink raw reply
* Re: [PATCH v2 11/11] interconnect: Add devfreq support
From: Saravana Kannan @ 2019-07-16 19:17 UTC (permalink / raw)
To: Sibi Sankar
Cc: Georgi Djakov, Rob Herring, Mark Rutland, Viresh Kumar,
Nishanth Menon, Stephen Boyd, Rafael J. Wysocki, MyungJoo Ham,
Kyungmin Park, Chanwoo Choi, Rajendra Nayak, Jordan Crouse,
Vincent Guittot, Bjorn Andersson, amit.kucheria, Sweeney, Sean,
daidavid1, Evan Green, Android Kernel Team, Linux PM,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
adharmap
In-Reply-To: <9f2bf3fd-f7c5-40e8-6415-f334e3ef8d5d@codeaurora.org>
On Tue, Jul 16, 2019 at 11:13 AM Sibi Sankar <sibis@codeaurora.org> wrote:
>
> Hey Saravana,
>
> On 6/18/19 2:48 AM, Saravana Kannan wrote:
> > On Mon, Jun 17, 2019 at 8:44 AM Georgi Djakov <georgi.djakov@linaro.org> wrote:
> >>
> >> Hi Saravana,
> >>
> >> On 6/14/19 07:17, Saravana Kannan wrote:
> >>> Add a icc_create_devfreq() and icc_remove_devfreq() to create and remove
> >>> devfreq devices for interconnect paths. A driver can create/remove devfreq
> >>> devices for the interconnects needed for its device by calling these APIs.
> >>> This would allow various devfreq governors to work with interconnect paths
> >>> and the device driver itself doesn't have to actively manage the bandwidth
> >>> votes for the interconnects.
> >>
> >> Thanks for the patches, but creating devfreq devices for each interconnect path
> >> seems odd to me - at least for consumers that already use a governor.
> >
> > Each governor instance always handles one "frequency" (more like
> > performance) domain at a time. So if a consumer is already using a
> > governor to scale the hardware block, then using another governor to
> > scale the interconnect performance points is the right way to go about
> > it. In fact, that's exactly what devfreq passive governor's
> > documentation even says it's meant for. That's also what cpufreq does
> > for each cluster/CPU frequency domain too.
> >
> >> So for DDR
> >> scaling for example, are you suggesting that we add a devfreq device from the
> >> cpufreq driver in order to scale the interconnect between CPU<->DDR?
> >
> > Yes in general. Although, CPUs are a special case because CPUs don't
> > go through devfreq. So passive governor as it stands today won't work.
> > CPU<->DDR scaling might need a separate governor (unlikely) or some
> > changes to the passive governor that I'm happy to work on once we
> > settle this for general devices like GPU, etc. But the DT format for
> > CPUs will be identical to GPUs or any other device.
>
> using icc_create_devfreq from the cpufreq-hw driver on SDM845 SoC
> to scale CPU<->DDR would cause a circular dependency. (i.e) with
> the addition of cpufreq notifier to the passive governor as in
> https://patchwork.kernel.org/patch/11046147/ devm_devfreq_add_device
> would require the cpufreq transistion notifier register and cpu
> freq_cpu_get to go through. Please add your thought on addressing this.
This is an old series. So not going to dive into this much.
But to answer your question, I wrote the cpufreq_map governor a long
time ago. So not surprised if you are finding issues with it -- it
needs a rewrite anyway.
-Saravana
> >
> >> Also if the
> >> GPU is already using devfreq, should we add a devfreq per each interconnect
> >> path? What would be the benefit in this case - using different governors for
> >> bandwidth scaling maybe?
> >
> > When saying "separate/different governors" in this email, I mean both
> > different instance of the same governor logic with different tunables
> > AND actually different algorithms/governor logic entirely.
> >
> > The heuristics to use for each interconnect path might be (more like,
> > will be) different based on hardware characteristics (Eg: what voltage
> > domains the interconnect is sitting on) and what interconnect
> > information is available (Eg: Just busy time vs bandwidth count vs no
> > information etc) -- so having separate governors for each interconnect
> > path makes a lot of sense. It also allows userspace to control the
> > policy for scaling each of those paths based on product use cases.
> >
> > For example, when the GPU is just doing simple UI rendering, userspace
> > can use the max_freq sysfs file for the devfreq device to disallow high
> > bandwidth OPPs on the GPU<->DDR path, but those higher OPPs could be
> > allowed by userspace when the GPU is used for games. Having devfreq
> > device for each interconnect path also make it easy to debug
> > performance issues -- you can independently change the votes for each
> > path to figure out what is causing the bottleneck, etc.
> >
> > Adding a devfreq device for interconnect voting with a few lines gives
> > all these features "for free".
> >
> > This doesn't mean all users of interconnect framework NEED to use
> > devfreq for interconnect. They might do it simply based on
> > calculations based on the use case (Eg: display driver from display
> > resolution). But if they are trying to use any kind of
> > algorithm/heuristics, writing it as a devfreq governor should be
> > encouraged.
> >
> > Also want to point out that BW OPPs also work for drivers that don't
> > use devfreq at all. The interconnect-opp-table just lists the
> > meaningful OPP leveld for the path and the device driver can pick one
> > entry from the table based on the use case.
> >
> > Thanks,
> > Saravana
> >
> >
> >
>
> --
> Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>
^ permalink raw reply
* Re: [PATCH v2 3/4] PM / devfreq: Cache OPP table reference in devfreq
From: Saravana Kannan @ 2019-07-16 19:12 UTC (permalink / raw)
To: Sibi Sankar
Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Viresh Kumar,
Nishanth Menon, Stephen Boyd, Rafael J. Wysocki,
Android Kernel Team, Linux PM, LKML, adharmap
In-Reply-To: <e9c9b150-43a6-dc6b-5d88-21608120e940@codeaurora.org>
On Tue, Jul 16, 2019 at 10:36 AM Sibi Sankar <sibis@codeaurora.org> wrote:
>
> Hey Saravana,
>
> On 6/26/19 3:03 AM, Saravana Kannan wrote:
> > The OPP table can be used often in devfreq. Trying to get it each time can
> > be expensive, so cache it in the devfreq struct.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> > drivers/devfreq/devfreq.c | 6 ++++++
> > include/linux/devfreq.h | 1 +
> > 2 files changed, 7 insertions(+)
> >
> > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> > index 6b6991f0e873..ac62b78dc035 100644
> > --- a/drivers/devfreq/devfreq.c
> > +++ b/drivers/devfreq/devfreq.c
> > @@ -597,6 +597,8 @@ static void devfreq_dev_release(struct device *dev)
> > if (devfreq->profile->exit)
> > devfreq->profile->exit(devfreq->dev.parent);
> >
> > + if (devfreq->opp_table)
> > + dev_pm_opp_put_opp_table(devfreq->opp_table);
> > mutex_destroy(&devfreq->lock);
> > kfree(devfreq);
> > }
> > @@ -677,6 +679,10 @@ struct devfreq *devfreq_add_device(struct device *dev,
> > devfreq->max_freq = devfreq->scaling_max_freq;
> >
> > devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
> > + devfreq->opp_table = dev_pm_opp_get_opp_table(dev);
> > + if (IS_ERR(devfreq->opp_table))
> > + devfreq->opp_table = NULL;
> > +
> > atomic_set(&devfreq->suspend_count, 0);
> >
> > dev_set_name(&devfreq->dev, "devfreq%d",
> > diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> > index fbffa74bfc1b..0d877c9513d7 100644
> > --- a/include/linux/devfreq.h
> > +++ b/include/linux/devfreq.h
> > @@ -156,6 +156,7 @@ struct devfreq {
> > struct devfreq_dev_profile *profile;
> > const struct devfreq_governor *governor;
> > char governor_name[DEVFREQ_NAME_LEN];
> > + struct opp_table *opp_table;
>
> please add it to the function docs as well
Will do.
-Saravana
^ permalink raw reply
* Re: [PATCH v3 2/6] OPP: Add support for bandwidth OPP tables
From: Saravana Kannan @ 2019-07-16 19:10 UTC (permalink / raw)
To: Sibi Sankar
Cc: Georgi Djakov, Rob Herring, Mark Rutland, Viresh Kumar,
Nishanth Menon, Stephen Boyd, Rafael J. Wysocki, Vincent Guittot,
Sweeney, Sean, daidavid1, Rajendra Nayak, Bjorn Andersson,
Evan Green, Android Kernel Team, Linux PM,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
adharmap
In-Reply-To: <5dd35be3-fd03-c9cc-1eed-ce4bc1433363@codeaurora.org>
On Tue, Jul 16, 2019 at 10:33 AM Sibi Sankar <sibis@codeaurora.org> wrote:
>
> Hey Saravana,
>
> On 7/3/19 6:40 AM, Saravana Kannan wrote:
> > Not all devices quantify their performance points in terms of frequency.
> > Devices like interconnects quantify their performance points in terms of
> > bandwidth. We need a way to represent these bandwidth levels in OPP. So,
> > add support for parsing bandwidth OPPs from DT.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> > drivers/opp/of.c | 34 ++++++++++++++++++++++++++++++++--
> > drivers/opp/opp.h | 4 +++-
> > 2 files changed, 35 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> > index c10c782d15aa..54fa70ed2adc 100644
> > --- a/drivers/opp/of.c
> > +++ b/drivers/opp/of.c
> > @@ -552,6 +552,35 @@ void dev_pm_opp_of_remove_table(struct device *dev)
> > }
> > EXPORT_SYMBOL_GPL(dev_pm_opp_of_remove_table);
> >
> > +static int _read_opp_key(struct dev_pm_opp *new_opp, struct device_node *np)
> > +{
> > + int ret;
> > + u64 rate;
> > + u32 bw;
> > +
> > + ret = of_property_read_u64(np, "opp-hz", &rate);
> > + if (!ret) {
> > + /*
> > + * Rate is defined as an unsigned long in clk API, and so
> > + * casting explicitly to its type. Must be fixed once rate is 64
> > + * bit guaranteed in clk API.
> > + */
> > + new_opp->rate = (unsigned long)rate
> now that the rate gets set here, please remove the rate assignment in
> _opp_add_static_v2
>
> > + return 0;
> > + }
> > +
> > + ret = of_property_read_u32(np, "opp-peak-KBps", &bw);
> > + if (ret)
> > + return ret;
> > + new_opp->rate = (unsigned long) &bw;
>
> should be bw instead
Good catch. Thanks!
>
> > +
> > + ret = of_property_read_u32(np, "opp-avg-KBps", &bw);
> > + if (!ret)
> > + new_opp->avg_bw = (unsigned long) &bw;
>
> ditto
>
> > +
> > + return 0;
> > +}
> > +
> > /**
> > * _opp_add_static_v2() - Allocate static OPPs (As per 'v2' DT bindings)
> > * @opp_table: OPP table
> > @@ -589,11 +618,12 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
> > if (!new_opp)
> > return ERR_PTR(-ENOMEM);
> >
> > - ret = of_property_read_u64(np, "opp-hz", &rate);
> > + ret = _read_opp_key(new_opp, np);
> > if (ret < 0) {
> > /* "opp-hz" is optional for devices like power domains. */
> > if (!opp_table->is_genpd) {
> > - dev_err(dev, "%s: opp-hz not found\n", __func__);
> > + dev_err(dev, "%s: opp-hz or opp-peak-bw not found\n",
> > + __func__);
>
> please remove the else part where rate value will be reset.
Ah! I flipped the meaning of the "if" check in my head. Thanks!
-Saravana
^ permalink raw reply
* Re: [PATCH v3 1/6] dt-bindings: opp: Introduce opp-peak-KBps and opp-avg-KBps bindings
From: Saravana Kannan @ 2019-07-16 18:58 UTC (permalink / raw)
To: Sibi Sankar
Cc: Georgi Djakov, Rob Herring, Mark Rutland, Viresh Kumar,
Nishanth Menon, Stephen Boyd, Rafael J. Wysocki, Vincent Guittot,
Sweeney, Sean, daidavid1, Rajendra Nayak, Bjorn Andersson,
Evan Green, Android Kernel Team, Linux PM,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML
In-Reply-To: <98b2e315-e8da-80ad-1ef8-e6b222c1c6fe@codeaurora.org>
On Tue, Jul 16, 2019 at 10:25 AM Sibi Sankar <sibis@codeaurora.org> wrote:
>
> Hey Saravana,
>
> https://patchwork.kernel.org/patch/10850815/
> There was already a discussion ^^ on how bandwidth bindings were to be
> named.
Yes, I'm aware of that series. That series is trying to define a BW
mapping for an existing frequency OPP table. This patch is NOT about
adding a mapping to an existing table. This patch is about adding the
notion of BW OPP tables where BW is the "key" instead of "frequency".
So let's not mixed up these two series.
-Saravana
> On 7/3/19 6:40 AM, Saravana Kannan wrote:
> > Interconnects often quantify their performance points in terms of
> > bandwidth. So, add opp-peak-KBps (required) and opp-avg-KBps (optional) to
> > allow specifying Bandwidth OPP tables in DT.
> >
> > opp-peak-KBps is a required property that replace opp-hz for Bandwidth OPP
> > tables.
> >
> > opp-avg-KBps is an optional property that can be used in Bandwidth OPP
> > tables.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> > Documentation/devicetree/bindings/opp/opp.txt | 15 ++++++++++++---
> > 1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
> > index 76b6c79604a5..c869e87caa2a 100644
> > --- a/Documentation/devicetree/bindings/opp/opp.txt
> > +++ b/Documentation/devicetree/bindings/opp/opp.txt
> > @@ -83,9 +83,14 @@ properties.
> >
> > Required properties:
> > - opp-hz: Frequency in Hz, expressed as a 64-bit big-endian integer. This is a
> > - required property for all device nodes but devices like power domains. The
> > - power domain nodes must have another (implementation dependent) property which
> > - uniquely identifies the OPP nodes.
> > + required property for all device nodes but for devices like power domains or
> > + bandwidth opp tables. The power domain nodes must have another (implementation
> > + dependent) property which uniquely identifies the OPP nodes. The interconnect
> > + opps are required to have the opp-peak-bw property.
> > +
> > +- opp-peak-KBps: Peak bandwidth in kilobytes per second, expressed as a 32-bit
> > + big-endian integer. This is a required property for all devices that don't
> > + have opp-hz. For example, bandwidth OPP tables for interconnect paths.
> >
> > Optional properties:
> > - opp-microvolt: voltage in micro Volts.
> > @@ -132,6 +137,10 @@ Optional properties:
> > - opp-level: A value representing the performance level of the device,
> > expressed as a 32-bit integer.
> >
> > +- opp-avg-KBps: Average bandwidth in kilobytes per second, expressed as a
> > + 32-bit big-endian integer. This property is only meaningful in OPP tables
> > + where opp-peak-KBps is present.
> > +
> > - clock-latency-ns: Specifies the maximum possible transition latency (in
> > nanoseconds) for switching to this OPP from any other OPP.
> >
> >
>
> --
> Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH v2] PM: QoS: Get rid of unused flags
From: Pavel Machek @ 2019-07-16 18:56 UTC (permalink / raw)
To: Amit Kucheria
Cc: linux-kernel, linux-arm-msm, Rafael J. Wysocki, Len Brown,
linux-pm
In-Reply-To: <e9e7bc3be3b51e68ae1a0f934c3724bd86f5f9af.1562854650.git.amit.kucheria@linaro.org>
[-- Attachment #1: Type: text/plain, Size: 797 bytes --]
On Thu 2019-07-11 19:51:25, Amit Kucheria wrote:
> The network_latency and network_throughput flags for PM-QoS have not
> found much use in drivers or in userspace since they were introduced.
>
> Commit 4a733ef1bea7 ("mac80211: remove PM-QoS listener") removed the
> only user PM_QOS_NETWORK_LATENCY in the kernel a while ago and there
> don't seem to be any userspace tools using the character device files
> either.
>
> PM_QOS_MEMORY_BANDWIDTH was never even added to the trace events.
>
> Remove all the flags except cpu_dma_latency.
>
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
Acked-by: Pavel Machek <pavel@ucw.cz>
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v2 1/4] OPP: Allow required-opps even if the device doesn't have power-domains
From: Saravana Kannan @ 2019-07-16 18:54 UTC (permalink / raw)
To: Sibi Sankar
Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Viresh Kumar,
Nishanth Menon, Stephen Boyd, Rafael J. Wysocki,
Android Kernel Team, Linux PM, LKML
In-Reply-To: <e7a5b387-fa85-15a8-8d79-fbc441c36293@codeaurora.org>
On Tue, Jul 16, 2019 at 10:18 AM Sibi Sankar <sibis@codeaurora.org> wrote:
>
> Hey Saravana,
> Thanks for taking time to post out this series.
>
> On 6/26/19 3:03 AM, Saravana Kannan wrote:
> > A Device-A can have a (minimum) performance requirement on another
> > Device-B to be able to function correctly. This performance requirement
> > on Device-B can also change based on the current performance level of
> > Device-A.
> >
> > The existing required-opps feature fits well to describe this need. So,
> > instead of limiting required-opps to point to only PM-domain devices,
> > allow it to point to any device.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> > drivers/opp/core.c | 2 +-
> > drivers/opp/of.c | 14 --------------
> > 2 files changed, 1 insertion(+), 15 deletions(-)
> >
> > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > index 0e7703fe733f..74c7bdc6f463 100644
> > --- a/drivers/opp/core.c
> > +++ b/drivers/opp/core.c
> > @@ -710,7 +710,7 @@ static int _set_required_opps(struct device *dev,
> > return 0;
> >
> > /* Single genpd case */
> > - if (!genpd_virt_devs) {
> > + if (!genpd_virt_devs && required_opp_tables[0]->is_genpd) {
> https://patchwork.kernel.org/patch/10940671/
> This was already removed as a part of ^^ and is in linux-next.
Thanks for the heads up.
>
> > pstate = opp->required_opps[0]->pstate;
> > ret = dev_pm_genpd_set_performance_state(dev, pstate);
> > if (ret) {
> > diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> > index c10c782d15aa..7c8336e94aff 100644
> > --- a/drivers/opp/of.c
> > +++ b/drivers/opp/of.c
> > @@ -195,9 +195,6 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
> > */
> > count_pd = of_count_phandle_with_args(dev->of_node, "power-domains",
> > "#power-domain-cells");
> > - if (!count_pd)
> > - goto put_np;
> > -
> > if (count_pd > 1) {
> > genpd_virt_devs = kcalloc(count, sizeof(*genpd_virt_devs),
> > GFP_KERNEL);
> > @@ -226,17 +223,6 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
> >
> > if (IS_ERR(required_opp_tables[i]))
> > goto free_required_tables;
> > -
> > - /*
> > - * We only support genpd's OPPs in the "required-opps" for now,
> > - * as we don't know how much about other cases. Error out if the
> > - * required OPP doesn't belong to a genpd.
> > - */
> > - if (!required_opp_tables[i]->is_genpd) {
> > - dev_err(dev, "required-opp doesn't belong to genpd: %pOF\n",
> > - required_np);
> > - goto free_required_tables;
> > - }
>
> I expect the series to not work as is in its current state since I
> see a circular dependency here. The required-opp tables of the parent
> devfreq won't be populated until we add the opp-table of the child
> devfreq node while the child devfreq using passive governor would
> return -EPROBE_DEFER until the parent devfreq probes.
I looked into this when I wrote the patches. I thought I handled it
correctly. Let me take another look.
-Saravana
> The same applies to this patch -> https://patchwork.kernel.org/patch
> /11046147/ I posted out based on your series. So we would probably have
> to address the dependency here.
>
> > }
> >
> > goto put_np;
> >
>
> --
> Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH 2/9] rcu: Add support for consolidated-RCU reader checking (v3)
From: Paul E. McKenney @ 2019-07-16 18:53 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
Josh Triplett, keescook, kernel-hardening, kernel-team,
Lai Jiangshan, Len Brown, linux-acpi, linux-doc, linux-pci,
linux-pm, Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190716184649.GA130463@google.com>
On Tue, Jul 16, 2019 at 02:46:49PM -0400, Joel Fernandes wrote:
> On Tue, Jul 16, 2019 at 11:38:33AM -0700, Paul E. McKenney wrote:
> > On Mon, Jul 15, 2019 at 10:36:58AM -0400, Joel Fernandes (Google) wrote:
> > > This patch adds support for checking RCU reader sections in list
> > > traversal macros. Optionally, if the list macro is called under SRCU or
> > > other lock/mutex protection, then appropriate lockdep expressions can be
> > > passed to make the checks pass.
> > >
> > > Existing list_for_each_entry_rcu() invocations don't need to pass the
> > > optional fourth argument (cond) unless they are under some non-RCU
> > > protection and needs to make lockdep check pass.
> > >
> > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> >
> > Now that I am on the correct version, again please fold in the checks
> > for the extra argument. The ability to have an optional argument looks
> > quite helpful, especially when compared to growing the RCU API!
>
> I did fold this and replied with a pull request URL based on /dev branch. But
> we can hold off on the pull requests until we decide on the below comments:
>
> > A few more things below.
> > > ---
> > > include/linux/rculist.h | 28 ++++++++++++++++++++-----
> > > include/linux/rcupdate.h | 7 +++++++
> > > kernel/rcu/Kconfig.debug | 11 ++++++++++
> > > kernel/rcu/update.c | 44 ++++++++++++++++++++++++----------------
> > > 4 files changed, 67 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> > > index e91ec9ddcd30..1048160625bb 100644
> > > --- a/include/linux/rculist.h
> > > +++ b/include/linux/rculist.h
> > > @@ -40,6 +40,20 @@ static inline void INIT_LIST_HEAD_RCU(struct list_head *list)
> > > */
> > > #define list_next_rcu(list) (*((struct list_head __rcu **)(&(list)->next)))
> > >
> > > +/*
> > > + * Check during list traversal that we are within an RCU reader
> > > + */
> > > +
> > > +#ifdef CONFIG_PROVE_RCU_LIST
> >
> > This new Kconfig option is OK temporarily, but unless there is reason to
> > fear malfunction that a few weeks of rcutorture, 0day, and -next won't
> > find, it would be better to just use CONFIG_PROVE_RCU. The overall goal
> > is to reduce the number of RCU knobs rather than grow them, must though
> > history might lead one to believe otherwise. :-/
>
> If you want, we can try to drop this option and just use PROVE_RCU however I
> must say there may be several warnings that need to be fixed in a short
> period of time (even a few weeks may be too short) considering the 1000+
> uses of RCU lists.
Do many people other than me build with CONFIG_PROVE_RCU? If so, then
that would be a good reason for a temporary CONFIG_PROVE_RCU_LIST,
as in going away in a release or two once the warnings get fixed.
> But I don't mind dropping it and it may just accelerate the fixing up of all
> callers.
I will let you decide based on the above question. But if you have
CONFIG_PROVE_RCU_LIST, as noted below, it needs to depend on RCU_EXPERT.
Thanx, Paul
> > > +#define __list_check_rcu(dummy, cond, ...) \
> > > + ({ \
> > > + RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \
> > > + "RCU-list traversed in non-reader section!"); \
> > > + })
> > > +#else
> > > +#define __list_check_rcu(dummy, cond, ...) ({})
> > > +#endif
> > > +
> > > /*
> > > * Insert a new entry between two known consecutive entries.
> > > *
> > > @@ -343,14 +357,16 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
> > > * @pos: the type * to use as a loop cursor.
> > > * @head: the head for your list.
> > > * @member: the name of the list_head within the struct.
> > > + * @cond: optional lockdep expression if called from non-RCU protection.
> > > *
> > > * This list-traversal primitive may safely run concurrently with
> > > * the _rcu list-mutation primitives such as list_add_rcu()
> > > * as long as the traversal is guarded by rcu_read_lock().
> > > */
> > > -#define list_for_each_entry_rcu(pos, head, member) \
> > > - for (pos = list_entry_rcu((head)->next, typeof(*pos), member); \
> > > - &pos->member != (head); \
> > > +#define list_for_each_entry_rcu(pos, head, member, cond...) \
> > > + for (__list_check_rcu(dummy, ## cond, 0), \
> > > + pos = list_entry_rcu((head)->next, typeof(*pos), member); \
> > > + &pos->member != (head); \
> > > pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
> > >
> > > /**
> > > @@ -616,13 +632,15 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n,
> > > * @pos: the type * to use as a loop cursor.
> > > * @head: the head for your list.
> > > * @member: the name of the hlist_node within the struct.
> > > + * @cond: optional lockdep expression if called from non-RCU protection.
> > > *
> > > * This list-traversal primitive may safely run concurrently with
> > > * the _rcu list-mutation primitives such as hlist_add_head_rcu()
> > > * as long as the traversal is guarded by rcu_read_lock().
> > > */
> > > -#define hlist_for_each_entry_rcu(pos, head, member) \
> > > - for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
> > > +#define hlist_for_each_entry_rcu(pos, head, member, cond...) \
> > > + for (__list_check_rcu(dummy, ## cond, 0), \
> > > + pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
> > > typeof(*(pos)), member); \
> > > pos; \
> > > pos = hlist_entry_safe(rcu_dereference_raw(hlist_next_rcu(\
> > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > > index 8f7167478c1d..f3c29efdf19a 100644
> > > --- a/include/linux/rcupdate.h
> > > +++ b/include/linux/rcupdate.h
> > > @@ -221,6 +221,7 @@ int debug_lockdep_rcu_enabled(void);
> > > int rcu_read_lock_held(void);
> > > int rcu_read_lock_bh_held(void);
> > > int rcu_read_lock_sched_held(void);
> > > +int rcu_read_lock_any_held(void);
> > >
> > > #else /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
> > >
> > > @@ -241,6 +242,12 @@ static inline int rcu_read_lock_sched_held(void)
> > > {
> > > return !preemptible();
> > > }
> > > +
> > > +static inline int rcu_read_lock_any_held(void)
> > > +{
> > > + return !preemptible();
> > > +}
> > > +
> > > #endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */
> > >
> > > #ifdef CONFIG_PROVE_RCU
> > > diff --git a/kernel/rcu/Kconfig.debug b/kernel/rcu/Kconfig.debug
> > > index 5ec3ea4028e2..7fbd21dbfcd0 100644
> > > --- a/kernel/rcu/Kconfig.debug
> > > +++ b/kernel/rcu/Kconfig.debug
> > > @@ -8,6 +8,17 @@ menu "RCU Debugging"
> > > config PROVE_RCU
> > > def_bool PROVE_LOCKING
> > >
> > > +config PROVE_RCU_LIST
> > > + bool "RCU list lockdep debugging"
> > > + depends on PROVE_RCU
> >
> > This must also depend on RCU_EXPERT.
>
> Sure.
>
> > > + default n
> > > + help
> > > + Enable RCU lockdep checking for list usages. By default it is
> > > + turned off since there are several list RCU users that still
> > > + need to be converted to pass a lockdep expression. To prevent
> > > + false-positive splats, we keep it default disabled but once all
> > > + users are converted, we can remove this config option.
> > > +
> > > config TORTURE_TEST
> > > tristate
> > > default n
> > > diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> > > index 9dd5aeef6e70..b7a4e3b5fa98 100644
> > > --- a/kernel/rcu/update.c
> > > +++ b/kernel/rcu/update.c
> > > @@ -91,14 +91,18 @@ module_param(rcu_normal_after_boot, int, 0);
> > > * Similarly, we avoid claiming an SRCU read lock held if the current
> > > * CPU is offline.
> > > */
> > > +#define rcu_read_lock_held_common() \
> > > + if (!debug_lockdep_rcu_enabled()) \
> > > + return 1; \
> > > + if (!rcu_is_watching()) \
> > > + return 0; \
> > > + if (!rcu_lockdep_current_cpu_online()) \
> > > + return 0;
> >
> > Nice abstraction of common code!
>
> Thanks!
>
^ permalink raw reply
* Re: [PATCH v2 2/9] rcu: Add support for consolidated-RCU reader checking
From: Paul E. McKenney @ 2019-07-16 18:50 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
Josh Triplett, keescook, kernel-hardening, kernel-team,
Lai Jiangshan, Len Brown, linux-acpi, linux-doc, linux-pci,
linux-pm, Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190716183517.GA129705@google.com>
On Tue, Jul 16, 2019 at 02:35:17PM -0400, Joel Fernandes wrote:
> On Tue, Jul 16, 2019 at 11:22:37AM -0700, Paul E. McKenney wrote:
> > On Fri, Jul 12, 2019 at 01:00:17PM -0400, Joel Fernandes (Google) wrote:
> > > This patch adds support for checking RCU reader sections in list
> > > traversal macros. Optionally, if the list macro is called under SRCU or
> > > other lock/mutex protection, then appropriate lockdep expressions can be
> > > passed to make the checks pass.
> > >
> > > Existing list_for_each_entry_rcu() invocations don't need to pass the
> > > optional fourth argument (cond) unless they are under some non-RCU
> > > protection and needs to make lockdep check pass.
> > >
> > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> >
> > If you fold in the checks for extra parameters, I will take this
> > one and also 1/9.
>
> I folded the checks in and also threw in the rcu-sync with Oleg's ack:
>
> Could you pull into /dev branch?
>
> git pull https://github.com/joelagnel/linux-kernel.git list-first-three
> (Based on your dev branch)
Given that I am going to have to rebase these a few times, please
email a v4.
Thanx, Paul
^ permalink raw reply
* Re: [PATCH v2] driver core: Remove device link creation limitation
From: Saravana Kannan @ 2019-07-16 18:49 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Greg Kroah-Hartman, LKML, Linux PM, Lukas Wunner, Jon Hunter,
Ulf Hansson, Marek Szyprowski
In-Reply-To: <2305283.AStDPdUUnE@kreacher>
On Tue, Jul 16, 2019 at 8:21 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Subject: [PATCH] driver core: Remove device link creation limitation
>
> If device_link_add() is called for a consumer/supplier pair with an
> existing device link between them and the existing link's type is
> not in agreement with the flags passed to that function by its
> caller, NULL will be returned. That is seriously inconvenient,
> because it forces the callers of device_link_add() to worry about
> what others may or may not do even if that is not relevant to them
> for any other reasons.
>
> It turns out, however, that this limitation can be made go away
> relatively easily.
>
> The underlying observation is that if DL_FLAG_STATELESS has been
> passed to device_link_add() in flags for the given consumer/supplier
> pair at least once, calling either device_link_del() or
> device_link_remove() to release the link returned by it should work,
> but there are no other requirements associated with that flag. In
> turn, if at least one of the callers of device_link_add() for the
> given consumer/supplier pair has not passed DL_FLAG_STATELESS to it
> in flags, the driver core should track the status of the link and act
> on it as appropriate (ie. the link should be treated as "managed").
> This means that DL_FLAG_STATELESS needs to be set for managed device
> links and it should be valid to call device_link_del() or
> device_link_remove() to drop references to them in certain
> sutiations.
>
> To allow that to happen, introduce a new (internal) device link flag
> called DL_FLAG_MANAGED and make device_link_add() set it automatically
> whenever DL_FLAG_STATELESS is not passed to it. Also make it take
> additional references to existing device links that were previously
> stateless (that is, with DL_FLAG_STATELESS set and DL_FLAG_MANAGED
> unset) and will need to be managed going forward and initialize
> their status (which has been DL_STATE_NONE so far).
>
> Accordingly, when a managed device link is dropped automatically
> by the driver core, make it clear DL_FLAG_MANAGED, reset the link's
> status back to DL_STATE_NONE and drop the reference to it associated
> with DL_FLAG_MANAGED instead of just deleting it right away (to
> allow it to stay around in case it still needs to be released
> explicitly by someone).
>
> With that, since setting DL_FLAG_STATELESS doesn't mean that the
> device link in question is not managed any more, replace all of the
> status-tracking checks against DL_FLAG_STATELESS with analogous
> checks against DL_FLAG_MANAGED and update the documentation to
> reflect these changes.
>
> While at it, make device_link_add() reject flags that it does not
> recognize, including DL_FLAG_MANAGED.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Reviewed-by: Saravana Kannan <saravanak@google.com>
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>
> -> v2:
> * Add a check to device_link_add() to return NULL if unrecognized flags are
> passed to it.
> * Modify kerneldoc comments around DL_FLAG_MANAGED.
>
> I've tentatively added the Tested-by tag from Marek, because I don't expect
> the changes made between the initial posting and the v2 to make any difference
> for him.
>
> ---
> Documentation/driver-api/device_link.rst | 4
> drivers/base/core.c | 176 +++++++++++++++++--------------
> drivers/base/power/runtime.c | 4
> include/linux/device.h | 4
> 4 files changed, 106 insertions(+), 82 deletions(-)
>
> Index: linux-pm/drivers/base/core.c
> ===================================================================
> --- linux-pm.orig/drivers/base/core.c
> +++ linux-pm/drivers/base/core.c
> @@ -124,6 +124,50 @@ static int device_is_dependent(struct de
> return ret;
> }
>
> +static void device_link_init_status(struct device_link *link,
> + struct device *consumer,
> + struct device *supplier)
> +{
> + switch (supplier->links.status) {
> + case DL_DEV_PROBING:
> + switch (consumer->links.status) {
> + case DL_DEV_PROBING:
> + /*
> + * A consumer driver can create a link to a supplier
> + * that has not completed its probing yet as long as it
> + * knows that the supplier is already functional (for
> + * example, it has just acquired some resources from the
> + * supplier).
> + */
> + link->status = DL_STATE_CONSUMER_PROBE;
> + break;
> + default:
> + link->status = DL_STATE_DORMANT;
> + break;
> + }
> + break;
> + case DL_DEV_DRIVER_BOUND:
> + switch (consumer->links.status) {
> + case DL_DEV_PROBING:
> + link->status = DL_STATE_CONSUMER_PROBE;
> + break;
> + case DL_DEV_DRIVER_BOUND:
> + link->status = DL_STATE_ACTIVE;
> + break;
> + default:
> + link->status = DL_STATE_AVAILABLE;
> + break;
> + }
> + break;
> + case DL_DEV_UNBINDING:
> + link->status = DL_STATE_SUPPLIER_UNBIND;
> + break;
> + default:
> + link->status = DL_STATE_DORMANT;
> + break;
> + }
> +}
> +
> static int device_reorder_to_tail(struct device *dev, void *not_used)
> {
> struct device_link *link;
> @@ -165,6 +209,10 @@ void device_pm_move_to_tail(struct devic
> device_links_read_unlock(idx);
> }
>
> +#define DL_MANAGED_LINK_FLAGS (DL_FLAG_AUTOREMOVE_CONSUMER | \
> + DL_FLAG_AUTOREMOVE_SUPPLIER | \
> + DL_FLAG_AUTOPROBE_CONSUMER)
> +
> /**
> * device_link_add - Create a link between two devices.
> * @consumer: Consumer end of the link.
> @@ -179,9 +227,9 @@ void device_pm_move_to_tail(struct devic
> * of the link. If DL_FLAG_PM_RUNTIME is not set, DL_FLAG_RPM_ACTIVE will be
> * ignored.
> *
> - * If DL_FLAG_STATELESS is set in @flags, the link is not going to be managed by
> - * the driver core and, in particular, the caller of this function is expected
> - * to drop the reference to the link acquired by it directly.
> + * If DL_FLAG_STATELESS is set in @flags, the caller of this function is
> + * expected to release the link returned by it directly with the help of either
> + * device_link_del() or device_link_remove().
> *
> * If that flag is not set, however, the caller of this function is handing the
> * management of the link over to the driver core entirely and its return value
> @@ -201,9 +249,16 @@ void device_pm_move_to_tail(struct devic
> * be used to request the driver core to automaticall probe for a consmer
> * driver after successfully binding a driver to the supplier device.
> *
> - * The combination of DL_FLAG_STATELESS and either DL_FLAG_AUTOREMOVE_CONSUMER
> - * or DL_FLAG_AUTOREMOVE_SUPPLIER set in @flags at the same time is invalid and
> - * will cause NULL to be returned upfront.
> + * The combination of DL_FLAG_STATELESS and one of DL_FLAG_AUTOREMOVE_CONSUMER,
> + * DL_FLAG_AUTOREMOVE_SUPPLIER, or DL_FLAG_AUTOPROBE_CONSUMER set in @flags at
> + * the same time is invalid and will cause NULL to be returned upfront.
> + * However, if a device link between the given @consumer and @supplier pair
> + * exists already when this function is called for them, the existing link will
> + * be returned regardless of its current type and status (the link's flags may
> + * be modified then). The caller of this function is then expected to treat
> + * the link as though it has just been created, so (in particular) if
> + * DL_FLAG_STATELESS was passed in @flags, the link needs to be released
> + * explicitly when not needed any more (as stated above).
> *
> * A side effect of the link creation is re-ordering of dpm_list and the
> * devices_kset list by moving the consumer device and all devices depending
> @@ -220,10 +275,8 @@ struct device_link *device_link_add(stru
> struct device_link *link;
>
> if (!consumer || !supplier ||
> - (flags & DL_FLAG_STATELESS &&
> - flags & (DL_FLAG_AUTOREMOVE_CONSUMER |
> - DL_FLAG_AUTOREMOVE_SUPPLIER |
> - DL_FLAG_AUTOPROBE_CONSUMER)) ||
> + (flags & ~(DL_FLAG_STATELESS | DL_MANAGED_LINK_FLAGS)) ||
> + (flags & DL_FLAG_STATELESS && flags & DL_MANAGED_LINK_FLAGS) ||
> (flags & DL_FLAG_AUTOPROBE_CONSUMER &&
> flags & (DL_FLAG_AUTOREMOVE_CONSUMER |
> DL_FLAG_AUTOREMOVE_SUPPLIER)))
> @@ -236,6 +289,9 @@ struct device_link *device_link_add(stru
> }
> }
>
> + if (!(flags & DL_FLAG_STATELESS))
> + flags |= DL_FLAG_MANAGED;
> +
> device_links_write_lock();
> device_pm_lock();
>
> @@ -262,15 +318,6 @@ struct device_link *device_link_add(stru
> if (link->consumer != consumer)
> continue;
>
> - /*
> - * Don't return a stateless link if the caller wants a stateful
> - * one and vice versa.
> - */
> - if (WARN_ON((flags & DL_FLAG_STATELESS) != (link->flags & DL_FLAG_STATELESS))) {
> - link = NULL;
> - goto out;
> - }
> -
> if (flags & DL_FLAG_PM_RUNTIME) {
> if (!(link->flags & DL_FLAG_PM_RUNTIME)) {
> pm_runtime_new_link(consumer);
> @@ -281,6 +328,7 @@ struct device_link *device_link_add(stru
> }
>
> if (flags & DL_FLAG_STATELESS) {
> + link->flags |= DL_FLAG_STATELESS;
> kref_get(&link->kref);
> goto out;
> }
> @@ -299,6 +347,11 @@ struct device_link *device_link_add(stru
> link->flags &= ~(DL_FLAG_AUTOREMOVE_CONSUMER |
> DL_FLAG_AUTOREMOVE_SUPPLIER);
> }
> + if (!(link->flags & DL_FLAG_MANAGED)) {
> + kref_get(&link->kref);
> + link->flags |= DL_FLAG_MANAGED;
> + device_link_init_status(link, consumer, supplier);
> + }
> goto out;
> }
>
> @@ -325,48 +378,10 @@ struct device_link *device_link_add(stru
> kref_init(&link->kref);
>
> /* Determine the initial link state. */
> - if (flags & DL_FLAG_STATELESS) {
> + if (flags & DL_FLAG_STATELESS)
> link->status = DL_STATE_NONE;
> - } else {
> - switch (supplier->links.status) {
> - case DL_DEV_PROBING:
> - switch (consumer->links.status) {
> - case DL_DEV_PROBING:
> - /*
> - * A consumer driver can create a link to a
> - * supplier that has not completed its probing
> - * yet as long as it knows that the supplier is
> - * already functional (for example, it has just
> - * acquired some resources from the supplier).
> - */
> - link->status = DL_STATE_CONSUMER_PROBE;
> - break;
> - default:
> - link->status = DL_STATE_DORMANT;
> - break;
> - }
> - break;
> - case DL_DEV_DRIVER_BOUND:
> - switch (consumer->links.status) {
> - case DL_DEV_PROBING:
> - link->status = DL_STATE_CONSUMER_PROBE;
> - break;
> - case DL_DEV_DRIVER_BOUND:
> - link->status = DL_STATE_ACTIVE;
> - break;
> - default:
> - link->status = DL_STATE_AVAILABLE;
> - break;
> - }
> - break;
> - case DL_DEV_UNBINDING:
> - link->status = DL_STATE_SUPPLIER_UNBIND;
> - break;
> - default:
> - link->status = DL_STATE_DORMANT;
> - break;
> - }
> - }
> + else
> + device_link_init_status(link, consumer, supplier);
>
> /*
> * Some callers expect the link creation during consumer driver probe to
> @@ -528,7 +543,7 @@ static void device_links_missing_supplie
> * mark the link as "consumer probe in progress" to make the supplier removal
> * wait for us to complete (or bad things may happen).
> *
> - * Links with the DL_FLAG_STATELESS flag set are ignored.
> + * Links without the DL_FLAG_MANAGED flag set are ignored.
> */
> int device_links_check_suppliers(struct device *dev)
> {
> @@ -538,7 +553,7 @@ int device_links_check_suppliers(struct
> device_links_write_lock();
>
> list_for_each_entry(link, &dev->links.suppliers, c_node) {
> - if (link->flags & DL_FLAG_STATELESS)
> + if (!(link->flags & DL_FLAG_MANAGED))
> continue;
>
> if (link->status != DL_STATE_AVAILABLE) {
> @@ -563,7 +578,7 @@ int device_links_check_suppliers(struct
> *
> * Also change the status of @dev's links to suppliers to "active".
> *
> - * Links with the DL_FLAG_STATELESS flag set are ignored.
> + * Links without the DL_FLAG_MANAGED flag set are ignored.
> */
> void device_links_driver_bound(struct device *dev)
> {
> @@ -572,7 +587,7 @@ void device_links_driver_bound(struct de
> device_links_write_lock();
>
> list_for_each_entry(link, &dev->links.consumers, s_node) {
> - if (link->flags & DL_FLAG_STATELESS)
> + if (!(link->flags & DL_FLAG_MANAGED))
> continue;
>
> /*
> @@ -593,7 +608,7 @@ void device_links_driver_bound(struct de
> }
>
> list_for_each_entry(link, &dev->links.suppliers, c_node) {
> - if (link->flags & DL_FLAG_STATELESS)
> + if (!(link->flags & DL_FLAG_MANAGED))
> continue;
>
> WARN_ON(link->status != DL_STATE_CONSUMER_PROBE);
> @@ -605,6 +620,13 @@ void device_links_driver_bound(struct de
> device_links_write_unlock();
> }
>
> +static void device_link_drop_managed(struct device_link *link)
> +{
> + link->flags &= ~DL_FLAG_MANAGED;
> + WRITE_ONCE(link->status, DL_STATE_NONE);
> + kref_put(&link->kref, __device_link_del);
> +}
> +
> /**
> * __device_links_no_driver - Update links of a device without a driver.
> * @dev: Device without a drvier.
> @@ -615,18 +637,18 @@ void device_links_driver_bound(struct de
> * unless they already are in the "supplier unbind in progress" state in which
> * case they need not be updated.
> *
> - * Links with the DL_FLAG_STATELESS flag set are ignored.
> + * Links without the DL_FLAG_MANAGED flag set are ignored.
> */
> static void __device_links_no_driver(struct device *dev)
> {
> struct device_link *link, *ln;
>
> list_for_each_entry_safe_reverse(link, ln, &dev->links.suppliers, c_node) {
> - if (link->flags & DL_FLAG_STATELESS)
> + if (!(link->flags & DL_FLAG_MANAGED))
> continue;
>
> if (link->flags & DL_FLAG_AUTOREMOVE_CONSUMER)
> - __device_link_del(&link->kref);
> + device_link_drop_managed(link);
> else if (link->status == DL_STATE_CONSUMER_PROBE ||
> link->status == DL_STATE_ACTIVE)
> WRITE_ONCE(link->status, DL_STATE_AVAILABLE);
> @@ -643,7 +665,7 @@ static void __device_links_no_driver(str
> * %__device_links_no_driver() to update links to suppliers for it as
> * appropriate.
> *
> - * Links with the DL_FLAG_STATELESS flag set are ignored.
> + * Links without the DL_FLAG_MANAGED flag set are ignored.
> */
> void device_links_no_driver(struct device *dev)
> {
> @@ -652,7 +674,7 @@ void device_links_no_driver(struct devic
> device_links_write_lock();
>
> list_for_each_entry(link, &dev->links.consumers, s_node) {
> - if (link->flags & DL_FLAG_STATELESS)
> + if (!(link->flags & DL_FLAG_MANAGED))
> continue;
>
> /*
> @@ -680,7 +702,7 @@ void device_links_no_driver(struct devic
> * invoke %__device_links_no_driver() to update links to suppliers for it as
> * appropriate.
> *
> - * Links with the DL_FLAG_STATELESS flag set are ignored.
> + * Links without the DL_FLAG_MANAGED flag set are ignored.
> */
> void device_links_driver_cleanup(struct device *dev)
> {
> @@ -689,7 +711,7 @@ void device_links_driver_cleanup(struct
> device_links_write_lock();
>
> list_for_each_entry_safe(link, ln, &dev->links.consumers, s_node) {
> - if (link->flags & DL_FLAG_STATELESS)
> + if (!(link->flags & DL_FLAG_MANAGED))
> continue;
>
> WARN_ON(link->flags & DL_FLAG_AUTOREMOVE_CONSUMER);
> @@ -702,7 +724,7 @@ void device_links_driver_cleanup(struct
> */
> if (link->status == DL_STATE_SUPPLIER_UNBIND &&
> link->flags & DL_FLAG_AUTOREMOVE_SUPPLIER)
> - __device_link_del(&link->kref);
> + device_link_drop_managed(link);
>
> WRITE_ONCE(link->status, DL_STATE_DORMANT);
> }
> @@ -724,7 +746,7 @@ void device_links_driver_cleanup(struct
> *
> * Return 'false' if there are no probing or active consumers.
> *
> - * Links with the DL_FLAG_STATELESS flag set are ignored.
> + * Links without the DL_FLAG_MANAGED flag set are ignored.
> */
> bool device_links_busy(struct device *dev)
> {
> @@ -734,7 +756,7 @@ bool device_links_busy(struct device *de
> device_links_write_lock();
>
> list_for_each_entry(link, &dev->links.consumers, s_node) {
> - if (link->flags & DL_FLAG_STATELESS)
> + if (!(link->flags & DL_FLAG_MANAGED))
> continue;
>
> if (link->status == DL_STATE_CONSUMER_PROBE
> @@ -764,7 +786,7 @@ bool device_links_busy(struct device *de
> * driver to unbind and start over (the consumer will not re-probe as we have
> * changed the state of the link already).
> *
> - * Links with the DL_FLAG_STATELESS flag set are ignored.
> + * Links without the DL_FLAG_MANAGED flag set are ignored.
> */
> void device_links_unbind_consumers(struct device *dev)
> {
> @@ -776,7 +798,7 @@ void device_links_unbind_consumers(struc
> list_for_each_entry(link, &dev->links.consumers, s_node) {
> enum device_link_state status;
>
> - if (link->flags & DL_FLAG_STATELESS)
> + if (!(link->flags & DL_FLAG_MANAGED))
> continue;
>
> status = link->status;
> Index: linux-pm/drivers/base/power/runtime.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/runtime.c
> +++ linux-pm/drivers/base/power/runtime.c
> @@ -1624,7 +1624,7 @@ void pm_runtime_remove(struct device *de
> * runtime PM references to the device, drop the usage counter of the device
> * (as many times as needed).
> *
> - * Links with the DL_FLAG_STATELESS flag set are ignored.
> + * Links with the DL_FLAG_MANAGED flag unset are ignored.
> *
> * Since the device is guaranteed to be runtime-active at the point this is
> * called, nothing else needs to be done here.
> @@ -1641,7 +1641,7 @@ void pm_runtime_clean_up_links(struct de
> idx = device_links_read_lock();
>
> list_for_each_entry_rcu(link, &dev->links.consumers, s_node) {
> - if (link->flags & DL_FLAG_STATELESS)
> + if (!(link->flags & DL_FLAG_MANAGED))
> continue;
>
> while (refcount_dec_not_one(&link->rpm_active))
> Index: linux-pm/include/linux/device.h
> ===================================================================
> --- linux-pm.orig/include/linux/device.h
> +++ linux-pm/include/linux/device.h
> @@ -829,12 +829,13 @@ enum device_link_state {
> /*
> * Device link flags.
> *
> - * STATELESS: The core won't track the presence of supplier/consumer drivers.
> + * STATELESS: The core will not remove this link automatically.
> * AUTOREMOVE_CONSUMER: Remove the link automatically on consumer driver unbind.
> * PM_RUNTIME: If set, the runtime PM framework will use this link.
> * RPM_ACTIVE: Run pm_runtime_get_sync() on the supplier during link creation.
> * AUTOREMOVE_SUPPLIER: Remove the link automatically on supplier driver unbind.
> * AUTOPROBE_CONSUMER: Probe consumer driver automatically after supplier binds.
> + * MANAGED: The core tracks presence of supplier/consumer drivers (internal).
> */
> #define DL_FLAG_STATELESS BIT(0)
> #define DL_FLAG_AUTOREMOVE_CONSUMER BIT(1)
> @@ -842,6 +843,7 @@ enum device_link_state {
> #define DL_FLAG_RPM_ACTIVE BIT(3)
> #define DL_FLAG_AUTOREMOVE_SUPPLIER BIT(4)
> #define DL_FLAG_AUTOPROBE_CONSUMER BIT(5)
> +#define DL_FLAG_MANAGED BIT(6)
>
> /**
> * struct device_link - Device link representation.
> Index: linux-pm/Documentation/driver-api/device_link.rst
> ===================================================================
> --- linux-pm.orig/Documentation/driver-api/device_link.rst
> +++ linux-pm/Documentation/driver-api/device_link.rst
> @@ -78,8 +78,8 @@ typically deleted in its ``->remove`` ca
> driver is compiled as a module, the device link is added on module load and
> orderly deleted on unload. The same restrictions that apply to device link
> addition (e.g. exclusion of a parallel suspend/resume transition) apply equally
> -to deletion. Device links with ``DL_FLAG_STATELESS`` unset (i.e. managed
> -device links) are deleted automatically by the driver core.
> +to deletion. Device links managed by the driver core are deleted automatically
> +by it.
>
> Several flags may be specified on device link addition, two of which
> have already been mentioned above: ``DL_FLAG_STATELESS`` to express that no
>
Review-by: Saravana Kannan <saravanak@google.com>
-Saravana
^ permalink raw reply
* Re: [PATCH 0/9] Harden list_for_each_entry_rcu() and family
From: Paul E. McKenney @ 2019-07-16 18:46 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
Josh Triplett, keescook, kernel-hardening, kernel-team,
Lai Jiangshan, Len Brown, linux-acpi, linux-doc, linux-pci,
linux-pm, Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190715143705.117908-1-joel@joelfernandes.org>
On Mon, Jul 15, 2019 at 10:36:56AM -0400, Joel Fernandes (Google) wrote:
> Hi,
> This series aims to provide lockdep checking to RCU list macros for additional
> kernel hardening.
>
> RCU has a number of primitives for "consumption" of an RCU protected pointer.
> Most of the time, these consumers make sure that such accesses are under a RCU
> reader-section (such as rcu_dereference{,sched,bh} or under a lock, such as
> with rcu_dereference_protected()).
>
> However, there are other ways to consume RCU pointers, such as by
> list_for_each_entry_rcu or hlist_for_each_enry_rcu. Unlike the rcu_dereference
> family, these consumers do no lockdep checking at all. And with the growing
> number of RCU list uses (1000+), it is possible for bugs to creep in and go
> unnoticed which lockdep checks can catch.
>
> Since RCU consolidation efforts last year, the different traditional RCU
> flavors (preempt, bh, sched) are all consolidated. In other words, any of these
> flavors can cause a reader section to occur and all of them must cease before
> the reader section is considered to be unlocked. Thanks to this, we can
> generically check if we are in an RCU reader. This is what patch 1 does. Note
> that the list_for_each_entry_rcu and family are different from the
> rcu_dereference family in that, there is no _bh or _sched version of this
> macro. They are used under many different RCU reader flavors, and also SRCU.
> Patch 1 adds a new internal function rcu_read_lock_any_held() which checks
> if any reader section is active at all, when these macros are called. If no
> reader section exists, then the optional fourth argument to
> list_for_each_entry_rcu() can be a lockdep expression which is evaluated
> (similar to how rcu_dereference_check() works). If no lockdep expression is
> passed, and we are not in a reader, then a splat occurs. Just take off the
> lockdep expression after applying the patches, by using the following diff and
> see what happens:
>
> +++ b/arch/x86/pci/mmconfig-shared.c
> @@ -55,7 +55,7 @@ static void list_add_sorted(struct pci_mmcfg_region *new)
> struct pci_mmcfg_region *cfg;
>
> /* keep list sorted by segment and starting bus number */
> - list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list, pci_mmcfg_lock_held()) {
> + list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list) {
>
>
> The optional argument trick to list_for_each_entry_rcu() can also be used in
> the future to possibly remove rcu_dereference_{,bh,sched}_protected() API and
> we can pass an optional lockdep expression to rcu_dereference() itself. Thus
> eliminating 3 more RCU APIs.
>
> Note that some list macro wrappers already do their own lockdep checking in the
> caller side. These can be eliminated in favor of the built-in lockdep checking
> in the list macro that this series adds. For example, workqueue code has a
> assert_rcu_or_wq_mutex() function which is called in for_each_wq(). This
> series replaces that in favor of the built-in check.
>
> Also in the future, we can extend these checks to list_entry_rcu() and other
> list macros as well, if needed.
>
> Please note that I have kept this option default-disabled under a new config:
> CONFIG_PROVE_RCU_LIST. This is so that until all users are converted to pass
> the optional argument, we should keep the check disabled. There are about a
> 1000 or so users and it is not possible to pass in the optional lockdep
> expression in a single series since it is done on a case-by-case basis. I did
> convert a few users in this series itself.
I do like the optional argument as opposed to the traditional practice
of expanding the RCU API! Good stuff!!!
Please resend incorporating the acks and the changes from feedback.
I will hold off on any patches not yet having their maintainer's ack,
but it is OK to include them in v4. (I will just avoid applying them.)
The documentation patch needs a bit of wordsmithing, but I can do that.
Feel free to take another pass on it if you wish, though.
Thanx, Paul
> v2->v3: Simplified rcu-sync logic after rebase (Paul)
> Added check for bh_map (Paul)
> Refactored out more of the common code (Joel)
> Added Oleg ack to rcu-sync patch.
>
> v1->v2: Have assert_rcu_or_wq_mutex deleted (Daniel Jordan)
> Simplify rcu_read_lock_any_held() (Peter Zijlstra)
> Simplified rcu-sync logic (Oleg Nesterov)
> Updated documentation and rculist comments.
> Added GregKH ack.
>
> RFC->v1:
> Simplify list checking macro (Rasmus Villemoes)
>
> Joel Fernandes (Google) (9):
> rcu/update: Remove useless check for debug_locks (v1)
> rcu: Add support for consolidated-RCU reader checking (v3)
> rcu/sync: Remove custom check for reader-section (v2)
> ipv4: add lockdep condition to fix for_each_entry (v1)
> driver/core: Convert to use built-in RCU list checking (v1)
> workqueue: Convert for_each_wq to use built-in list check (v2)
> x86/pci: Pass lockdep condition to pcm_mmcfg_list iterator (v1)
> acpi: Use built-in RCU list checking for acpi_ioremaps list (v1)
> doc: Update documentation about list_for_each_entry_rcu (v1)
>
> Documentation/RCU/lockdep.txt | 15 ++++++++---
> Documentation/RCU/whatisRCU.txt | 9 ++++++-
> arch/x86/pci/mmconfig-shared.c | 5 ++--
> drivers/acpi/osl.c | 6 +++--
> drivers/base/base.h | 1 +
> drivers/base/core.c | 10 +++++++
> drivers/base/power/runtime.c | 15 +++++++----
> include/linux/rcu_sync.h | 4 +--
> include/linux/rculist.h | 28 +++++++++++++++----
> include/linux/rcupdate.h | 7 +++++
> kernel/rcu/Kconfig.debug | 11 ++++++++
> kernel/rcu/update.c | 48 ++++++++++++++++++---------------
> kernel/workqueue.c | 10 ++-----
> net/ipv4/fib_frontend.c | 3 ++-
> 14 files changed, 119 insertions(+), 53 deletions(-)
>
> --
> 2.22.0.510.g264f2c817a-goog
>
^ permalink raw reply
* Re: [PATCH 2/9] rcu: Add support for consolidated-RCU reader checking (v3)
From: Joel Fernandes @ 2019-07-16 18:46 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
Josh Triplett, keescook, kernel-hardening, kernel-team,
Lai Jiangshan, Len Brown, linux-acpi, linux-doc, linux-pci,
linux-pm, Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190716183833.GD14271@linux.ibm.com>
On Tue, Jul 16, 2019 at 11:38:33AM -0700, Paul E. McKenney wrote:
> On Mon, Jul 15, 2019 at 10:36:58AM -0400, Joel Fernandes (Google) wrote:
> > This patch adds support for checking RCU reader sections in list
> > traversal macros. Optionally, if the list macro is called under SRCU or
> > other lock/mutex protection, then appropriate lockdep expressions can be
> > passed to make the checks pass.
> >
> > Existing list_for_each_entry_rcu() invocations don't need to pass the
> > optional fourth argument (cond) unless they are under some non-RCU
> > protection and needs to make lockdep check pass.
> >
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>
> Now that I am on the correct version, again please fold in the checks
> for the extra argument. The ability to have an optional argument looks
> quite helpful, especially when compared to growing the RCU API!
I did fold this and replied with a pull request URL based on /dev branch. But
we can hold off on the pull requests until we decide on the below comments:
> A few more things below.
> > ---
> > include/linux/rculist.h | 28 ++++++++++++++++++++-----
> > include/linux/rcupdate.h | 7 +++++++
> > kernel/rcu/Kconfig.debug | 11 ++++++++++
> > kernel/rcu/update.c | 44 ++++++++++++++++++++++++----------------
> > 4 files changed, 67 insertions(+), 23 deletions(-)
> >
> > diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> > index e91ec9ddcd30..1048160625bb 100644
> > --- a/include/linux/rculist.h
> > +++ b/include/linux/rculist.h
> > @@ -40,6 +40,20 @@ static inline void INIT_LIST_HEAD_RCU(struct list_head *list)
> > */
> > #define list_next_rcu(list) (*((struct list_head __rcu **)(&(list)->next)))
> >
> > +/*
> > + * Check during list traversal that we are within an RCU reader
> > + */
> > +
> > +#ifdef CONFIG_PROVE_RCU_LIST
>
> This new Kconfig option is OK temporarily, but unless there is reason to
> fear malfunction that a few weeks of rcutorture, 0day, and -next won't
> find, it would be better to just use CONFIG_PROVE_RCU. The overall goal
> is to reduce the number of RCU knobs rather than grow them, must though
> history might lead one to believe otherwise. :-/
If you want, we can try to drop this option and just use PROVE_RCU however I
must say there may be several warnings that need to be fixed in a short
period of time (even a few weeks may be too short) considering the 1000+
uses of RCU lists.
But I don't mind dropping it and it may just accelerate the fixing up of all
callers.
> > +#define __list_check_rcu(dummy, cond, ...) \
> > + ({ \
> > + RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \
> > + "RCU-list traversed in non-reader section!"); \
> > + })
> > +#else
> > +#define __list_check_rcu(dummy, cond, ...) ({})
> > +#endif
> > +
> > /*
> > * Insert a new entry between two known consecutive entries.
> > *
> > @@ -343,14 +357,16 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
> > * @pos: the type * to use as a loop cursor.
> > * @head: the head for your list.
> > * @member: the name of the list_head within the struct.
> > + * @cond: optional lockdep expression if called from non-RCU protection.
> > *
> > * This list-traversal primitive may safely run concurrently with
> > * the _rcu list-mutation primitives such as list_add_rcu()
> > * as long as the traversal is guarded by rcu_read_lock().
> > */
> > -#define list_for_each_entry_rcu(pos, head, member) \
> > - for (pos = list_entry_rcu((head)->next, typeof(*pos), member); \
> > - &pos->member != (head); \
> > +#define list_for_each_entry_rcu(pos, head, member, cond...) \
> > + for (__list_check_rcu(dummy, ## cond, 0), \
> > + pos = list_entry_rcu((head)->next, typeof(*pos), member); \
> > + &pos->member != (head); \
> > pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
> >
> > /**
> > @@ -616,13 +632,15 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n,
> > * @pos: the type * to use as a loop cursor.
> > * @head: the head for your list.
> > * @member: the name of the hlist_node within the struct.
> > + * @cond: optional lockdep expression if called from non-RCU protection.
> > *
> > * This list-traversal primitive may safely run concurrently with
> > * the _rcu list-mutation primitives such as hlist_add_head_rcu()
> > * as long as the traversal is guarded by rcu_read_lock().
> > */
> > -#define hlist_for_each_entry_rcu(pos, head, member) \
> > - for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
> > +#define hlist_for_each_entry_rcu(pos, head, member, cond...) \
> > + for (__list_check_rcu(dummy, ## cond, 0), \
> > + pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
> > typeof(*(pos)), member); \
> > pos; \
> > pos = hlist_entry_safe(rcu_dereference_raw(hlist_next_rcu(\
> > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > index 8f7167478c1d..f3c29efdf19a 100644
> > --- a/include/linux/rcupdate.h
> > +++ b/include/linux/rcupdate.h
> > @@ -221,6 +221,7 @@ int debug_lockdep_rcu_enabled(void);
> > int rcu_read_lock_held(void);
> > int rcu_read_lock_bh_held(void);
> > int rcu_read_lock_sched_held(void);
> > +int rcu_read_lock_any_held(void);
> >
> > #else /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
> >
> > @@ -241,6 +242,12 @@ static inline int rcu_read_lock_sched_held(void)
> > {
> > return !preemptible();
> > }
> > +
> > +static inline int rcu_read_lock_any_held(void)
> > +{
> > + return !preemptible();
> > +}
> > +
> > #endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */
> >
> > #ifdef CONFIG_PROVE_RCU
> > diff --git a/kernel/rcu/Kconfig.debug b/kernel/rcu/Kconfig.debug
> > index 5ec3ea4028e2..7fbd21dbfcd0 100644
> > --- a/kernel/rcu/Kconfig.debug
> > +++ b/kernel/rcu/Kconfig.debug
> > @@ -8,6 +8,17 @@ menu "RCU Debugging"
> > config PROVE_RCU
> > def_bool PROVE_LOCKING
> >
> > +config PROVE_RCU_LIST
> > + bool "RCU list lockdep debugging"
> > + depends on PROVE_RCU
>
> This must also depend on RCU_EXPERT.
Sure.
> > + default n
> > + help
> > + Enable RCU lockdep checking for list usages. By default it is
> > + turned off since there are several list RCU users that still
> > + need to be converted to pass a lockdep expression. To prevent
> > + false-positive splats, we keep it default disabled but once all
> > + users are converted, we can remove this config option.
> > +
> > config TORTURE_TEST
> > tristate
> > default n
> > diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> > index 9dd5aeef6e70..b7a4e3b5fa98 100644
> > --- a/kernel/rcu/update.c
> > +++ b/kernel/rcu/update.c
> > @@ -91,14 +91,18 @@ module_param(rcu_normal_after_boot, int, 0);
> > * Similarly, we avoid claiming an SRCU read lock held if the current
> > * CPU is offline.
> > */
> > +#define rcu_read_lock_held_common() \
> > + if (!debug_lockdep_rcu_enabled()) \
> > + return 1; \
> > + if (!rcu_is_watching()) \
> > + return 0; \
> > + if (!rcu_lockdep_current_cpu_online()) \
> > + return 0;
>
> Nice abstraction of common code!
Thanks!
^ permalink raw reply
* Re: [PATCH 8/9] acpi: Use built-in RCU list checking for acpi_ioremaps list (v1)
From: Paul E. McKenney @ 2019-07-16 18:43 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
Josh Triplett, keescook, kernel-hardening, kernel-team,
Lai Jiangshan, Len Brown, linux-acpi, linux-doc, linux-pci,
linux-pm, Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190715143705.117908-9-joel@joelfernandes.org>
On Mon, Jul 15, 2019 at 10:37:04AM -0400, Joel Fernandes (Google) wrote:
> list_for_each_entry_rcu has built-in RCU and lock checking. Make use of
> it for acpi_ioremaps list traversal.
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
Given that Rafael acked it, this one looks ready.
Thanx, Paul
> ---
> drivers/acpi/osl.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 9c0edf2fc0dd..2f9d0d20b836 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -14,6 +14,7 @@
> #include <linux/slab.h>
> #include <linux/mm.h>
> #include <linux/highmem.h>
> +#include <linux/lockdep.h>
> #include <linux/pci.h>
> #include <linux/interrupt.h>
> #include <linux/kmod.h>
> @@ -80,6 +81,7 @@ struct acpi_ioremap {
>
> static LIST_HEAD(acpi_ioremaps);
> static DEFINE_MUTEX(acpi_ioremap_lock);
> +#define acpi_ioremap_lock_held() lock_is_held(&acpi_ioremap_lock.dep_map)
>
> static void __init acpi_request_region (struct acpi_generic_address *gas,
> unsigned int length, char *desc)
> @@ -206,7 +208,7 @@ acpi_map_lookup(acpi_physical_address phys, acpi_size size)
> {
> struct acpi_ioremap *map;
>
> - list_for_each_entry_rcu(map, &acpi_ioremaps, list)
> + list_for_each_entry_rcu(map, &acpi_ioremaps, list, acpi_ioremap_lock_held())
> if (map->phys <= phys &&
> phys + size <= map->phys + map->size)
> return map;
> @@ -249,7 +251,7 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
> {
> struct acpi_ioremap *map;
>
> - list_for_each_entry_rcu(map, &acpi_ioremaps, list)
> + list_for_each_entry_rcu(map, &acpi_ioremaps, list, acpi_ioremap_lock_held())
> if (map->virt <= virt &&
> virt + size <= map->virt + map->size)
> return map;
> --
> 2.22.0.510.g264f2c817a-goog
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox