* Re: [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking
From: Paul E. McKenney @ 2019-07-12 16:45 UTC (permalink / raw)
To: Joel Fernandes
Cc: Peter Zijlstra, linux-kernel, Alexey Kuznetsov, Bjorn Helgaas,
Borislav Petkov, c0d1n61at3, David S. Miller, edumazet,
Greg Kroah-Hartman, Hideaki YOSHIFUJI, H. Peter Anvin,
Ingo Molnar, Josh Triplett, keescook, kernel-hardening,
Lai Jiangshan, Len Brown, linux-acpi, linux-pci, linux-pm,
Mathieu Desnoyers, neilb, netdev, oleg, Pavel Machek,
Rafael J. Wysocki, Rasmus Villemoes, rcu, Steven Rostedt,
Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190712151051.GB235410@google.com>
On Fri, Jul 12, 2019 at 11:10:51AM -0400, Joel Fernandes wrote:
> On Fri, Jul 12, 2019 at 01:11:25PM +0200, Peter Zijlstra wrote:
> > On Thu, Jul 11, 2019 at 07:43:56PM -0400, Joel Fernandes (Google) wrote:
> > > +int rcu_read_lock_any_held(void)
> > > +{
> > > + int lockdep_opinion = 0;
> > > +
> > > + if (!debug_lockdep_rcu_enabled())
> > > + return 1;
> > > + if (!rcu_is_watching())
> > > + return 0;
> > > + if (!rcu_lockdep_current_cpu_online())
> > > + return 0;
> > > +
> > > + /* Preemptible RCU flavor */
> > > + if (lock_is_held(&rcu_lock_map))
> >
> > you forgot debug_locks here.
>
> Actually, it turns out debug_locks checking is not even needed. If
> debug_locks == 0, then debug_lockdep_rcu_enabled() returns 0 and we would not
> get to this point.
>
> > > + return 1;
> > > +
> > > + /* BH flavor */
> > > + if (in_softirq() || irqs_disabled())
> >
> > I'm not sure I'd put irqs_disabled() under BH, also this entire
> > condition is superfluous, see below.
> >
> > > + return 1;
> > > +
> > > + /* Sched flavor */
> > > + if (debug_locks)
> > > + lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
> > > + return lockdep_opinion || !preemptible();
> >
> > that !preemptible() turns into:
> >
> > !(preempt_count()==0 && !irqs_disabled())
> >
> > which is:
> >
> > preempt_count() != 0 || irqs_disabled()
> >
> > and already includes irqs_disabled() and in_softirq().
> >
> > > +}
> >
> > So maybe something lke:
> >
> > if (debug_locks && (lock_is_held(&rcu_lock_map) ||
> > lock_is_held(&rcu_sched_lock_map)))
> > return true;
>
> Agreed, I will do it this way (without the debug_locks) like:
>
> ---8<-----------------------
>
> diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> index ba861d1716d3..339aebc330db 100644
> --- a/kernel/rcu/update.c
> +++ b/kernel/rcu/update.c
> @@ -296,27 +296,15 @@ EXPORT_SYMBOL_GPL(rcu_read_lock_bh_held);
>
> int rcu_read_lock_any_held(void)
> {
> - int lockdep_opinion = 0;
> -
> if (!debug_lockdep_rcu_enabled())
> return 1;
> if (!rcu_is_watching())
> return 0;
> if (!rcu_lockdep_current_cpu_online())
> return 0;
> -
> - /* Preemptible RCU flavor */
> - if (lock_is_held(&rcu_lock_map))
> - return 1;
> -
> - /* BH flavor */
> - if (in_softirq() || irqs_disabled())
> - return 1;
> -
> - /* Sched flavor */
> - if (debug_locks)
> - lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
> - return lockdep_opinion || !preemptible();
> + if (lock_is_held(&rcu_lock_map) || lock_is_held(&rcu_sched_lock_map))
OK, I will bite... Why not also lock_is_held(&rcu_bh_lock_map)?
Thanx, Paul
> + return 1;
> + return !preemptible();
> }
> EXPORT_SYMBOL_GPL(rcu_read_lock_any_held);
>
>
^ permalink raw reply
* Re: [PATCHv3 6/6] drivers/cpufreq: Add a CPUFreq driver for AMD processors (Fam17h and later)
From: Janakarajan Natarajan @ 2019-07-12 16:44 UTC (permalink / raw)
To: Viresh Kumar
Cc: Natarajan, Janakarajan, linux-acpi@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
devel@acpica.org, Rafael J . Wysocki, Len Brown, Robert Moore,
Erik Schmauss, Ghannam, Yazen
In-Reply-To: <20190712031025.mylhtzehv3vs6db3@vireshk-i7>
On 7/11/2019 10:10 PM, Viresh Kumar wrote:
> On 11-07-19, 16:58, Janakarajan Natarajan wrote:
>> On 7/11/2019 1:12 AM, Viresh Kumar wrote:
>>> On 10-07-19, 18:37, Natarajan, Janakarajan wrote:
>>>> +static int amd_cpufreq_cpu_init(struct cpufreq_policy *policy)
>>>> +{
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int amd_cpufreq_cpu_exit(struct cpufreq_policy *policy)
>>>> +{
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int amd_cpufreq_cpu_verify(struct cpufreq_policy *policy)
>>>> +{
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int amd_cpufreq_cpu_target_index(struct cpufreq_policy *policy,
>>>> + unsigned int index)
>>>> +{
>>>> + return 0;
>>>> +}
>>> All empty helpers ? There is nothing you need to do ?
>>
>> When we posted v2 of this patchset, Rafael let us know that he was
>> uncomfortable
>>
>> going behind the (acpi-cpufreq) drivers back by letting the user
>> communicate directly
>>
>> with the platform. That's the reason we have an empty driver whose
>> primary purpose
>>
>> is to expose sysfs entries for the user.
> I read his comments now and what he suggested is:
>
> "What about handling this like the others do, through a proper cpufreq
> driver?"
>
> I am not sure if he meant something like that you have here. Only one
> cpufreq driver can be registered at any point of time with the kernel,
> and so if this one is there then acpi-cpufreq or intel-pstate can't be
> there. Who will do DVFS ?
By default, the driver is disabled and cpufreq falls back to using
acpi-cpufreq. To enable the driver,
a parameter i.e. amd_cpufreq.cppc_enable=1 needs to be used. Then the
user will gain access to
/sys/devices/system/cpu/cpu<num>/amd_cpufreq/{enable, max_perf,
min_perf, desired_perf, auto_sel_enable}.
max_perf, min_perf allows the user to indicate to the platform the
performance they expect
in an abstract scale [min_perf to max_perf] inclusive. desired_perf is
the performance the user
would like to ideally achieve. desired_perf will fall withing max and
min perf. Based on the value of these
registers, the platform will adjust a number of variables, voltage and
frequency being one of them.
If the user would rather have the system handle all power/performance
adjustments by
itself, then they can set the auto_sel_enable register.
Thanks,
Janakarajan
^ permalink raw reply
* Re: [PATCH v2 3/6] cpuidle: Add unified driver for NVIDIA Tegra SoCs
From: Dmitry Osipenko @ 2019-07-12 16:41 UTC (permalink / raw)
To: Thierry Reding, Jonathan Hunter, Peter De Schrijver,
Rafael J. Wysocki, Daniel Lezcano
Cc: linux-pm, linux-tegra, linux-arm-kernel, linux-kernel
In-Reply-To: <c54c43fd-9b11-4aa8-3acf-d2260d0fca4a@gmail.com>
11.07.2019 21:35, Dmitry Osipenko пишет:
> 11.07.2019 6:13, Dmitry Osipenko пишет:
>> The new driver is based on the old CPU Idle drivers that are removed now
>> from arm/arch/mach-tegra/ directory. Those removed drivers were reworked
>> and squashed into a single unified driver that covers multiple hardware
>> generations, starting from Tegra20 and ending with Tegra124.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>> arch/arm/mach-tegra/tegra.c | 4 +
>> drivers/cpuidle/Kconfig.arm | 8 +
>> drivers/cpuidle/Makefile | 1 +
>> drivers/cpuidle/cpuidle-tegra.c | 304 ++++++++++++++++++++++++++++++++
>> include/soc/tegra/cpuidle.h | 4 +
>> 5 files changed, 321 insertions(+)
>> create mode 100644 drivers/cpuidle/cpuidle-tegra.c
>>
>> diff --git a/arch/arm/mach-tegra/tegra.c b/arch/arm/mach-tegra/tegra.c
>> index d9237769a37c..f1ce2857a251 100644
>> --- a/arch/arm/mach-tegra/tegra.c
>> +++ b/arch/arm/mach-tegra/tegra.c
>> @@ -36,6 +36,7 @@
>> #include <asm/mach/arch.h>
>> #include <asm/mach/time.h>
>> #include <asm/mach-types.h>
>> +#include <asm/psci.h>
>> #include <asm/setup.h>
>>
>> #include "board.h"
>> @@ -92,6 +93,9 @@ static void __init tegra_dt_init_late(void)
>> if (IS_ENABLED(CONFIG_ARCH_TEGRA_2x_SOC) &&
>> of_machine_is_compatible("nvidia,tegra20"))
>> platform_device_register_simple("tegra20-cpufreq", -1, NULL, 0);
>> +
>> + if (IS_ENABLED(CONFIG_ARM_TEGRA_CPUIDLE) && !psci_smp_available())
>> + platform_device_register_simple("tegra-cpuidle", -1, NULL, 0);
>> }
>>
>> static const char * const tegra_dt_board_compat[] = {
>> diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
>> index 48cb3d4bb7d1..d90861361f1d 100644
>> --- a/drivers/cpuidle/Kconfig.arm
>> +++ b/drivers/cpuidle/Kconfig.arm
>> @@ -76,3 +76,11 @@ config ARM_MVEBU_V7_CPUIDLE
>> depends on ARCH_MVEBU && !ARM64
>> help
>> Select this to enable cpuidle on Armada 370, 38x and XP processors.
>> +
>> +config ARM_TEGRA_CPUIDLE
>> + bool "CPU Idle Driver for NVIDIA Tegra SoCs"
>> + depends on ARCH_TEGRA && !ARM64
>> + select ARCH_NEEDS_CPU_IDLE_COUPLED if SMP
>> + select ARM_CPU_SUSPEND
>> + help
>> + Select this to enable cpuidle for NVIDIA Tegra20/30/114/124 SoCs.
>> diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
>> index 9d7176cee3d3..470d17fa8746 100644
>> --- a/drivers/cpuidle/Makefile
>> +++ b/drivers/cpuidle/Makefile
>> @@ -20,6 +20,7 @@ obj-$(CONFIG_ARM_U8500_CPUIDLE) += cpuidle-ux500.o
>> obj-$(CONFIG_ARM_AT91_CPUIDLE) += cpuidle-at91.o
>> obj-$(CONFIG_ARM_EXYNOS_CPUIDLE) += cpuidle-exynos.o
>> obj-$(CONFIG_ARM_CPUIDLE) += cpuidle-arm.o
>> +obj-$(CONFIG_ARM_TEGRA_CPUIDLE) += cpuidle-tegra.o
>>
>> ###############################################################################
>> # MIPS drivers
>> diff --git a/drivers/cpuidle/cpuidle-tegra.c b/drivers/cpuidle/cpuidle-tegra.c
>> new file mode 100644
>> index 000000000000..54974cd2255f
>> --- /dev/null
>> +++ b/drivers/cpuidle/cpuidle-tegra.c
>> @@ -0,0 +1,304 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * CPU idle driver for Tegra CPUs
>> + *
>> + * Copyright (c) 2010-2013, NVIDIA Corporation.
>> + * Copyright (c) 2011 Google, Inc.
>> + * Author: Colin Cross <ccross@android.com>
>> + * Gary King <gking@nvidia.com>
>> + *
>> + * Rework for 3.3 by Peter De Schrijver <pdeschrijver@nvidia.com>
>> + *
>> + * Tegra20/124 driver unification by Dmitry Osipenko <digetx@gmail.com>
>> + */
>> +
>> +#include <linux/cpuidle.h>
>> +#include <linux/cpumask.h>
>> +#include <linux/cpu_pm.h>
>> +#include <linux/errno.h>
>> +#include <linux/ktime.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/types.h>
>> +
>> +#include <linux/clk/tegra.h>
>> +#include <linux/firmware/trusted_foundations.h>
>> +
>> +#include <soc/tegra/cpuidle.h>
>> +#include <soc/tegra/flowctrl.h>
>> +#include <soc/tegra/fuse.h>
>> +#include <soc/tegra/pm.h>
>> +
>> +#include <asm/cpuidle.h>
>> +#include <asm/firmware.h>
>> +#include <asm/smp_plat.h>
>> +#include <asm/suspend.h>
>> +
>> +#define TEGRA_C1 0
>> +#define TEGRA_C7 1
>> +#define TEGRA_CC6 2
>> +
>> +static atomic_t tegra_idle_barrier;
>> +
>> +static inline bool tegra_cpuidle_using_firmware(void)
>> +{
>> + return firmware_ops->prepare_idle && firmware_ops->do_idle;
>> +}
>> +
>> +static int tegra_shutdown_secondary_cpu(unsigned long cpu)
>> +{
>> + tegra_cpu_die(cpu);
>> +
>> + return -EINVAL;
>> +}
>> +
>> +static int tegra_await_secondary_cpus_shutdown(void)
>> +{
>> + ktime_t timeout = ktime_add_ms(ktime_get(), 1);
>> +
>> + /*
>> + * The boot CPU shall await for the secondaries shutdown in order
>> + * to power-off CPU's cluster safely.
>> + */
>> + do {
>> + if (tegra_cpu_rail_off_ready())
>> + return 0;
>> +
>> + } while (ktime_compare(ktime_get(), timeout) < 0);
>> +
>> + return -ETIMEDOUT;
>> +}
>> +
>> +static void tegra_wake_up_secondary_cpus(void)
>> +{
>> + unsigned int cpu, lcpu;
>> +
>> + for_each_cpu(lcpu, cpu_online_mask) {
>> + cpu = cpu_logical_map(lcpu);
>> +
>> + if (cpu > 0) {
>> + tegra_enable_cpu_clock(cpu);
>> + tegra_cpu_out_of_reset(cpu);
>> + flowctrl_write_cpu_halt(cpu, 0);
>> + }
>> + }
>> +}
>> +
>> +static int tegra_cpuidle_cc6_enter(void)
>> +{
>> + int err;
>> +
>> + err = tegra_await_secondary_cpus_shutdown();
>> + if (err)
>> + return err;
>> +
>> + err = tegra_idle_lp2_last();
>> +
>> + tegra_wake_up_secondary_cpus();
>> +
>> + return err;
>> +}
>> +
>> +static int tegra_cpuidle_c7_enter(void)
>> +{
>> + int err;
>> +
>> + if (tegra_cpuidle_using_firmware()) {
>> + err = call_firmware_op(prepare_idle, TF_PM_MODE_LP2_NOFLUSH_L2);
>> + if (err)
>> + return err;
>> +
>> + return call_firmware_op(do_idle, 0);
>> + }
>> +
>> + return cpu_suspend(0, tegra30_sleep_cpu_secondary_finish);
>> +}
>> +
>> +static int tegra_cpuidle_enter(struct cpuidle_device *dev,
>> + int index, unsigned int cpu)
>> +{
>> + int err;
>> +
>> + local_fiq_disable();
>> + tegra_set_cpu_in_lp2();
>> + cpu_pm_enter();
>> +
>> + switch (index) {
>> + case TEGRA_C7:
>> + err = tegra_cpuidle_c7_enter();
>> + break;
>> + case TEGRA_CC6:
>> + cpuidle_coupled_parallel_barrier(dev, &tegra_idle_barrier);
>
> I realized that this is not very correct. We still need to do a proper
> barrier with SGI checking in order to bail out if other CPU sent IPI
> during of the awaiting for a coupled barrier to avoid the overhead of
> unnecessary power-gating. Will correct that in the next revision.
UPDATE: Actually, turned out it's even a necessity to handle the SGI
because GIC's driver doesn't save and replay SGI across CPU cluster PM.
^ permalink raw reply
* Re: [PATCH v2 1/6] ARM: tegra: Remove cpuidle drivers
From: Dmitry Osipenko @ 2019-07-12 16:23 UTC (permalink / raw)
To: Jon Hunter, Thierry Reding, Peter De Schrijver, Rafael J. Wysocki,
Daniel Lezcano
Cc: linux-pm, linux-tegra, linux-arm-kernel, linux-kernel
In-Reply-To: <73781434-d25a-b17b-aacb-95ace5ac5f95@nvidia.com>
12.07.2019 12:39, Jon Hunter пишет:
>
> On 11/07/2019 18:03, Dmitry Osipenko wrote:
>> 11.07.2019 12:26, Jon Hunter пишет:
>>>
>>> On 11/07/2019 04:13, Dmitry Osipenko wrote:
>>>> Remove the old drivers to replace them cleanly with a new one later on.
>>>>
>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>> ---
>>>> arch/arm/mach-tegra/Makefile | 13 --
>>>> arch/arm/mach-tegra/cpuidle-tegra114.c | 89 -----------
>>>> arch/arm/mach-tegra/cpuidle-tegra20.c | 212 -------------------------
>>>> arch/arm/mach-tegra/cpuidle-tegra30.c | 132 ---------------
>>>> arch/arm/mach-tegra/cpuidle.c | 50 ------
>>>> arch/arm/mach-tegra/cpuidle.h | 21 ---
>>>> arch/arm/mach-tegra/irq.c | 18 ---
>>>> arch/arm/mach-tegra/irq.h | 11 --
>>>> arch/arm/mach-tegra/pm.c | 7 -
>>>> arch/arm/mach-tegra/pm.h | 1 -
>>>> arch/arm/mach-tegra/reset-handler.S | 11 --
>>>> arch/arm/mach-tegra/reset.h | 9 +-
>>>> arch/arm/mach-tegra/sleep-tegra20.S | 190 +---------------------
>>>> arch/arm/mach-tegra/sleep.h | 12 --
>>>> arch/arm/mach-tegra/tegra.c | 3 -
>>>> drivers/soc/tegra/Kconfig | 1 -
>>>> include/soc/tegra/cpuidle.h | 4 -
>>>> 17 files changed, 5 insertions(+), 779 deletions(-)
>>>> delete mode 100644 arch/arm/mach-tegra/cpuidle-tegra114.c
>>>> delete mode 100644 arch/arm/mach-tegra/cpuidle-tegra20.c
>>>> delete mode 100644 arch/arm/mach-tegra/cpuidle-tegra30.c
>>>> delete mode 100644 arch/arm/mach-tegra/cpuidle.c
>>>> delete mode 100644 arch/arm/mach-tegra/cpuidle.h
>>>> delete mode 100644 arch/arm/mach-tegra/irq.h
>>>
>>> By removing all the above, it is really hard to review the diff. Is
>>> there any way you could first consolidate the cpuidle drivers into say
>>> the existing arch/arm/mach-tegra/cpuidle-tegra20.c and then move to
>>> drivers/cpuidle?
>>
>> I'm afraid that it will make reviewing even more difficult because
>> everything that is removed here is not returned in the further patches.
>> The new driver is based on the older ones, but I wrote it from scratch
>> and it's not only looks different, but also works a bit different as you
>> may see.
>>
>> Could you please clarify what exactly makes it hard to review? The diff
>> looks pretty clean to me, while squashing everything into existing
>> driver should be quite a mess.
>
> Ideally a patch should standalone and can be reviewed by itself.
> However, to review this, we need to review patches 1, 2 and 3 at the
> same time. So IMO it is not that convenient from a reviewers
> perspective. Furthermore, patches 1 and 3 are large and so easy to miss
> something.
>
> Is there really no way to have a patch to combined the existing drivers,
> then a patch to convert them into the newer rewritten version you have
> implemented, then move the driver?
Probably I spent a bit too much time with that code, so now yours
suggestion looks to me like an unnecessary step. But I will try and see
how it goes, at least it should be possible to break down the patch 1 a
bit more, hopefully it will help to better understand what's going on in
the further patches if you're not familiar or don't remember how it all
works.
^ permalink raw reply
* Re: [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking
From: Peter Zijlstra @ 2019-07-12 15:58 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, Josh Triplett,
keescook, kernel-hardening, Lai Jiangshan, Len Brown, linux-acpi,
linux-pci, linux-pm, Mathieu Desnoyers, neilb, netdev, oleg,
Paul E. McKenney, Pavel Machek, Rafael J. Wysocki,
Rasmus Villemoes, rcu, Steven Rostedt, Tejun Heo, Thomas Gleixner,
will, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190712151051.GB235410@google.com>
On Fri, Jul 12, 2019 at 11:10:51AM -0400, Joel Fernandes wrote:
> Agreed, I will do it this way (without the debug_locks) like:
>
> ---8<-----------------------
>
> diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> index ba861d1716d3..339aebc330db 100644
> --- a/kernel/rcu/update.c
> +++ b/kernel/rcu/update.c
> @@ -296,27 +296,15 @@ EXPORT_SYMBOL_GPL(rcu_read_lock_bh_held);
>
> int rcu_read_lock_any_held(void)
> {
> if (!debug_lockdep_rcu_enabled())
> return 1;
> if (!rcu_is_watching())
> return 0;
> if (!rcu_lockdep_current_cpu_online())
> return 0;
> + if (lock_is_held(&rcu_lock_map) || lock_is_held(&rcu_sched_lock_map))
> + return 1;
> + return !preemptible();
> }
> EXPORT_SYMBOL_GPL(rcu_read_lock_any_held);
OK, that looks sane. Thanks!
^ permalink raw reply
* Re: [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking
From: Joel Fernandes @ 2019-07-12 15:10 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Josh Triplett,
keescook, kernel-hardening, Lai Jiangshan, Len Brown, linux-acpi,
linux-pci, linux-pm, Mathieu Desnoyers, neilb, netdev, oleg,
Paul E. McKenney, Pavel Machek, Rafael J. Wysocki,
Rasmus Villemoes, rcu, Steven Rostedt, Tejun Heo, Thomas Gleixner,
will, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190712111125.GT3402@hirez.programming.kicks-ass.net>
On Fri, Jul 12, 2019 at 01:11:25PM +0200, Peter Zijlstra wrote:
> On Thu, Jul 11, 2019 at 07:43:56PM -0400, Joel Fernandes (Google) wrote:
> > +int rcu_read_lock_any_held(void)
> > +{
> > + int lockdep_opinion = 0;
> > +
> > + if (!debug_lockdep_rcu_enabled())
> > + return 1;
> > + if (!rcu_is_watching())
> > + return 0;
> > + if (!rcu_lockdep_current_cpu_online())
> > + return 0;
> > +
> > + /* Preemptible RCU flavor */
> > + if (lock_is_held(&rcu_lock_map))
>
> you forgot debug_locks here.
Actually, it turns out debug_locks checking is not even needed. If
debug_locks == 0, then debug_lockdep_rcu_enabled() returns 0 and we would not
get to this point.
> > + return 1;
> > +
> > + /* BH flavor */
> > + if (in_softirq() || irqs_disabled())
>
> I'm not sure I'd put irqs_disabled() under BH, also this entire
> condition is superfluous, see below.
>
> > + return 1;
> > +
> > + /* Sched flavor */
> > + if (debug_locks)
> > + lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
> > + return lockdep_opinion || !preemptible();
>
> that !preemptible() turns into:
>
> !(preempt_count()==0 && !irqs_disabled())
>
> which is:
>
> preempt_count() != 0 || irqs_disabled()
>
> and already includes irqs_disabled() and in_softirq().
>
> > +}
>
> So maybe something lke:
>
> if (debug_locks && (lock_is_held(&rcu_lock_map) ||
> lock_is_held(&rcu_sched_lock_map)))
> return true;
Agreed, I will do it this way (without the debug_locks) like:
---8<-----------------------
diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
index ba861d1716d3..339aebc330db 100644
--- a/kernel/rcu/update.c
+++ b/kernel/rcu/update.c
@@ -296,27 +296,15 @@ EXPORT_SYMBOL_GPL(rcu_read_lock_bh_held);
int rcu_read_lock_any_held(void)
{
- int lockdep_opinion = 0;
-
if (!debug_lockdep_rcu_enabled())
return 1;
if (!rcu_is_watching())
return 0;
if (!rcu_lockdep_current_cpu_online())
return 0;
-
- /* Preemptible RCU flavor */
- if (lock_is_held(&rcu_lock_map))
- return 1;
-
- /* BH flavor */
- if (in_softirq() || irqs_disabled())
- return 1;
-
- /* Sched flavor */
- if (debug_locks)
- lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
- return lockdep_opinion || !preemptible();
+ if (lock_is_held(&rcu_lock_map) || lock_is_held(&rcu_sched_lock_map))
+ return 1;
+ return !preemptible();
}
EXPORT_SYMBOL_GPL(rcu_read_lock_any_held);
^ permalink raw reply related
* Re: [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking
From: Joel Fernandes @ 2019-07-12 14:49 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Josh Triplett,
keescook, kernel-hardening, Lai Jiangshan, Len Brown, linux-acpi,
linux-pci, linux-pm, Mathieu Desnoyers, neilb, netdev, oleg,
Paul E. McKenney, Pavel Machek, Rafael J. Wysocki,
Rasmus Villemoes, rcu, Steven Rostedt, Tejun Heo, Thomas Gleixner,
will, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190712110142.GS3402@hirez.programming.kicks-ass.net>
On Fri, Jul 12, 2019 at 01:01:42PM +0200, Peter Zijlstra wrote:
> On Thu, Jul 11, 2019 at 07:43:56PM -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>
> > ---
> > include/linux/rculist.h | 29 ++++++++++++++++++++++++-----
> > include/linux/rcupdate.h | 7 +++++++
> > kernel/rcu/Kconfig.debug | 11 +++++++++++
> > kernel/rcu/update.c | 26 ++++++++++++++++++++++++++
> > 4 files changed, 68 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> > index e91ec9ddcd30..78c15ec6b2c9 100644
> > --- a/include/linux/rculist.h
> > +++ b/include/linux/rculist.h
> > @@ -40,6 +40,23 @@ 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
> > + */
> > +
> > +#define SIXTH_ARG(a1, a2, a3, a4, a5, a6, ...) a6
> > +#define COUNT_VARGS(...) SIXTH_ARG(dummy, ## __VA_ARGS__, 4, 3, 2, 1, 0)
>
> You don't seem to actually use it in this patch; also linux/kernel.h has
> COUNT_ARGS().
Yes, I replied after sending patches that I fixed this. I will remove them.
thanks,
- Joel
^ permalink raw reply
* Re: [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking
From: Joel Fernandes @ 2019-07-12 13:56 UTC (permalink / raw)
To: Oleg Nesterov
Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Josh Triplett,
keescook, kernel-hardening, Lai Jiangshan, Len Brown, linux-acpi,
linux-pci, linux-pm, Mathieu Desnoyers, neilb, netdev,
Paul E. McKenney, 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: <20190712121200.GC21989@redhat.com>
On Fri, Jul 12, 2019 at 02:12:00PM +0200, Oleg Nesterov wrote:
> On 07/11, Joel Fernandes (Google) wrote:
> >
> > +int rcu_read_lock_any_held(void)
>
> rcu_sync_is_idle() wants it. You have my ack in advance ;)
Cool, thanks ;)
- Joel
^ permalink raw reply
* Re: [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking
From: Oleg Nesterov @ 2019-07-12 12:12 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, Josh Triplett,
keescook, kernel-hardening, Lai Jiangshan, Len Brown, linux-acpi,
linux-pci, linux-pm, Mathieu Desnoyers, neilb, netdev,
Paul E. McKenney, 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: <20190711234401.220336-2-joel@joelfernandes.org>
On 07/11, Joel Fernandes (Google) wrote:
>
> +int rcu_read_lock_any_held(void)
rcu_sync_is_idle() wants it. You have my ack in advance ;)
Oleg.
^ permalink raw reply
* Re: [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking
From: Peter Zijlstra @ 2019-07-12 11:11 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, Josh Triplett,
keescook, kernel-hardening, Lai Jiangshan, Len Brown, linux-acpi,
linux-pci, linux-pm, Mathieu Desnoyers, neilb, netdev, oleg,
Paul E. McKenney, Pavel Machek, Rafael J. Wysocki,
Rasmus Villemoes, rcu, Steven Rostedt, Tejun Heo, Thomas Gleixner,
will, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190711234401.220336-2-joel@joelfernandes.org>
On Thu, Jul 11, 2019 at 07:43:56PM -0400, Joel Fernandes (Google) wrote:
> +int rcu_read_lock_any_held(void)
> +{
> + int lockdep_opinion = 0;
> +
> + if (!debug_lockdep_rcu_enabled())
> + return 1;
> + if (!rcu_is_watching())
> + return 0;
> + if (!rcu_lockdep_current_cpu_online())
> + return 0;
> +
> + /* Preemptible RCU flavor */
> + if (lock_is_held(&rcu_lock_map))
you forgot debug_locks here.
> + return 1;
> +
> + /* BH flavor */
> + if (in_softirq() || irqs_disabled())
I'm not sure I'd put irqs_disabled() under BH, also this entire
condition is superfluous, see below.
> + return 1;
> +
> + /* Sched flavor */
> + if (debug_locks)
> + lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
> + return lockdep_opinion || !preemptible();
that !preemptible() turns into:
!(preempt_count()==0 && !irqs_disabled())
which is:
preempt_count() != 0 || irqs_disabled()
and already includes irqs_disabled() and in_softirq().
> +}
So maybe something lke:
if (debug_locks && (lock_is_held(&rcu_lock_map) ||
lock_is_held(&rcu_sched_lock_map)))
return true;
return !preemptible();
^ permalink raw reply
* Re: [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking
From: Peter Zijlstra @ 2019-07-12 11:01 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, Josh Triplett,
keescook, kernel-hardening, Lai Jiangshan, Len Brown, linux-acpi,
linux-pci, linux-pm, Mathieu Desnoyers, neilb, netdev, oleg,
Paul E. McKenney, Pavel Machek, Rafael J. Wysocki,
Rasmus Villemoes, rcu, Steven Rostedt, Tejun Heo, Thomas Gleixner,
will, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190711234401.220336-2-joel@joelfernandes.org>
On Thu, Jul 11, 2019 at 07:43:56PM -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>
> ---
> include/linux/rculist.h | 29 ++++++++++++++++++++++++-----
> include/linux/rcupdate.h | 7 +++++++
> kernel/rcu/Kconfig.debug | 11 +++++++++++
> kernel/rcu/update.c | 26 ++++++++++++++++++++++++++
> 4 files changed, 68 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> index e91ec9ddcd30..78c15ec6b2c9 100644
> --- a/include/linux/rculist.h
> +++ b/include/linux/rculist.h
> @@ -40,6 +40,23 @@ 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
> + */
> +
> +#define SIXTH_ARG(a1, a2, a3, a4, a5, a6, ...) a6
> +#define COUNT_VARGS(...) SIXTH_ARG(dummy, ## __VA_ARGS__, 4, 3, 2, 1, 0)
You don't seem to actually use it in this patch; also linux/kernel.h has
COUNT_ARGS().
^ permalink raw reply
* Re: [PATCH v2 1/6] ARM: tegra: Remove cpuidle drivers
From: Jon Hunter @ 2019-07-12 9:39 UTC (permalink / raw)
To: Dmitry Osipenko, Thierry Reding, Peter De Schrijver,
Rafael J. Wysocki, Daniel Lezcano
Cc: linux-pm, linux-tegra, linux-arm-kernel, linux-kernel
In-Reply-To: <a6e4b43e-369c-c501-6d2e-69d5b940ff9c@gmail.com>
On 11/07/2019 18:03, Dmitry Osipenko wrote:
> 11.07.2019 12:26, Jon Hunter пишет:
>>
>> On 11/07/2019 04:13, Dmitry Osipenko wrote:
>>> Remove the old drivers to replace them cleanly with a new one later on.
>>>
>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>> ---
>>> arch/arm/mach-tegra/Makefile | 13 --
>>> arch/arm/mach-tegra/cpuidle-tegra114.c | 89 -----------
>>> arch/arm/mach-tegra/cpuidle-tegra20.c | 212 -------------------------
>>> arch/arm/mach-tegra/cpuidle-tegra30.c | 132 ---------------
>>> arch/arm/mach-tegra/cpuidle.c | 50 ------
>>> arch/arm/mach-tegra/cpuidle.h | 21 ---
>>> arch/arm/mach-tegra/irq.c | 18 ---
>>> arch/arm/mach-tegra/irq.h | 11 --
>>> arch/arm/mach-tegra/pm.c | 7 -
>>> arch/arm/mach-tegra/pm.h | 1 -
>>> arch/arm/mach-tegra/reset-handler.S | 11 --
>>> arch/arm/mach-tegra/reset.h | 9 +-
>>> arch/arm/mach-tegra/sleep-tegra20.S | 190 +---------------------
>>> arch/arm/mach-tegra/sleep.h | 12 --
>>> arch/arm/mach-tegra/tegra.c | 3 -
>>> drivers/soc/tegra/Kconfig | 1 -
>>> include/soc/tegra/cpuidle.h | 4 -
>>> 17 files changed, 5 insertions(+), 779 deletions(-)
>>> delete mode 100644 arch/arm/mach-tegra/cpuidle-tegra114.c
>>> delete mode 100644 arch/arm/mach-tegra/cpuidle-tegra20.c
>>> delete mode 100644 arch/arm/mach-tegra/cpuidle-tegra30.c
>>> delete mode 100644 arch/arm/mach-tegra/cpuidle.c
>>> delete mode 100644 arch/arm/mach-tegra/cpuidle.h
>>> delete mode 100644 arch/arm/mach-tegra/irq.h
>>
>> By removing all the above, it is really hard to review the diff. Is
>> there any way you could first consolidate the cpuidle drivers into say
>> the existing arch/arm/mach-tegra/cpuidle-tegra20.c and then move to
>> drivers/cpuidle?
>
> I'm afraid that it will make reviewing even more difficult because
> everything that is removed here is not returned in the further patches.
> The new driver is based on the older ones, but I wrote it from scratch
> and it's not only looks different, but also works a bit different as you
> may see.
>
> Could you please clarify what exactly makes it hard to review? The diff
> looks pretty clean to me, while squashing everything into existing
> driver should be quite a mess.
Ideally a patch should standalone and can be reviewed by itself.
However, to review this, we need to review patches 1, 2 and 3 at the
same time. So IMO it is not that convenient from a reviewers
perspective. Furthermore, patches 1 and 3 are large and so easy to miss
something.
Is there really no way to have a patch to combined the existing drivers,
then a patch to convert them into the newer rewritten version you have
implemented, then move the driver?
Jon
--
nvpublic
^ permalink raw reply
* [RFC v4 3/3] cpuidle-powernv : Recompute the idle-state timeouts when state usage is enabled/disabled
From: Abhishek Goel @ 2019-07-12 7:04 UTC (permalink / raw)
To: linux-kernel, linuxppc-dev, linux-pm
Cc: npiggin, rjw, daniel.lezcano, mpe, ego, dja, Abhishek Goel
In-Reply-To: <20190712070457.55242-1-huntbag@linux.vnet.ibm.com>
The disable callback can be used to compute timeout for other states
whenever a state is enabled or disabled. We store the computed timeout
in "timeout" defined in cpuidle state strucure. So, we compute timeout
only when some state is enabled or disabled and not every time in the
fast idle path.
We also use the computed timeout to get timeout for snooze, thus getting
rid of get_snooze_timeout for snooze loop.
Signed-off-by: Abhishek Goel <huntbag@linux.vnet.ibm.com>
---
drivers/cpuidle/cpuidle-powernv.c | 35 +++++++++++--------------------
include/linux/cpuidle.h | 1 +
2 files changed, 13 insertions(+), 23 deletions(-)
diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c
index 17e20e408ffe..29add322d0c4 100644
--- a/drivers/cpuidle/cpuidle-powernv.c
+++ b/drivers/cpuidle/cpuidle-powernv.c
@@ -45,7 +45,6 @@ struct stop_psscr_table {
static struct stop_psscr_table stop_psscr_table[CPUIDLE_STATE_MAX] __read_mostly;
static u64 default_snooze_timeout __read_mostly;
-static bool snooze_timeout_en __read_mostly;
static u64 forced_wakeup_timeout(struct cpuidle_device *dev,
struct cpuidle_driver *drv,
@@ -67,26 +66,13 @@ static u64 forced_wakeup_timeout(struct cpuidle_device *dev,
return 0;
}
-static u64 get_snooze_timeout(struct cpuidle_device *dev,
- struct cpuidle_driver *drv,
- int index)
+static void pnv_disable_callback(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv)
{
int i;
- if (unlikely(!snooze_timeout_en))
- return default_snooze_timeout;
-
- for (i = index + 1; i < drv->state_count; i++) {
- struct cpuidle_state *s = &drv->states[i];
- struct cpuidle_state_usage *su = &dev->states_usage[i];
-
- if (s->disabled || su->disable)
- continue;
-
- return s->target_residency * tb_ticks_per_usec;
- }
-
- return default_snooze_timeout;
+ for (i = 0; i < drv->state_count; i++)
+ drv->states[i].timeout = forced_wakeup_timeout(dev, drv, i);
}
static int snooze_loop(struct cpuidle_device *dev,
@@ -94,16 +80,20 @@ static int snooze_loop(struct cpuidle_device *dev,
int index)
{
u64 snooze_exit_time;
+ u64 snooze_timeout = drv->states[index].timeout;
+
+ if (!snooze_timeout)
+ snooze_timeout = default_snooze_timeout;
set_thread_flag(TIF_POLLING_NRFLAG);
local_irq_enable();
- snooze_exit_time = get_tb() + get_snooze_timeout(dev, drv, index);
+ snooze_exit_time = get_tb() + snooze_timeout;
ppc64_runlatch_off();
HMT_very_low();
while (!need_resched()) {
- if (likely(snooze_timeout_en) && get_tb() > snooze_exit_time) {
+ if (get_tb() > snooze_exit_time) {
/*
* Task has not woken up but we are exiting the polling
* loop anyway. Require a barrier after polling is
@@ -168,7 +158,7 @@ static int stop_loop(struct cpuidle_device *dev,
u64 timeout_tb;
bool forced_wakeup = false;
- timeout_tb = forced_wakeup_timeout(dev, drv, index);
+ timeout_tb = drv->states[index].timeout;
/* Ensure that the timeout is at least one microsecond
* greater than current decrement value. Else, we will
@@ -263,6 +253,7 @@ static int powernv_cpuidle_driver_init(void)
*/
drv->cpumask = (struct cpumask *)cpu_present_mask;
+ drv->disable_callback = pnv_disable_callback;
return 0;
}
@@ -422,8 +413,6 @@ static int powernv_idle_probe(void)
/* Device tree can indicate more idle states */
max_idle_state = powernv_add_idle_states();
default_snooze_timeout = TICK_USEC * tb_ticks_per_usec;
- if (max_idle_state > 1)
- snooze_timeout_en = true;
} else
return -ENODEV;
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 8a0e54bd0d5d..31662b657b9c 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -50,6 +50,7 @@ struct cpuidle_state {
int power_usage; /* in mW */
unsigned int target_residency; /* in US */
bool disabled; /* disabled on all CPUs */
+ unsigned long long timeout; /* timeout for exiting out of a state */
int (*enter) (struct cpuidle_device *dev,
struct cpuidle_driver *drv,
--
2.17.1
^ permalink raw reply related
* [PATCH v4 1/3] cpuidle-powernv : forced wakeup for stop states
From: Abhishek Goel @ 2019-07-12 7:04 UTC (permalink / raw)
To: linux-kernel, linuxppc-dev, linux-pm
Cc: npiggin, rjw, daniel.lezcano, mpe, ego, dja, Abhishek Goel
In-Reply-To: <20190712070457.55242-1-huntbag@linux.vnet.ibm.com>
Currently, the cpuidle governors determine what idle state a idling CPU
should enter into based on heuristics that depend on the idle history on
that CPU. Given that no predictive heuristic is perfect, there are cases
where the governor predicts a shallow idle state, hoping that the CPU will
be busy soon. However, if no new workload is scheduled on that CPU in the
near future, the CPU may end up in the shallow state.
This is problematic, when the predicted state in the aforementioned
scenario is a shallow stop state on a tickless system. As we might get
stuck into shallow states for hours, in absence of ticks or interrupts.
To address this, We forcefully wakeup the cpu by setting the
decrementer. The decrementer is set to a value that corresponds with the
residency of the next available state. Thus firing up a timer that will
forcefully wakeup the cpu. Few such iterations will essentially train the
governor to select a deeper state for that cpu, as the timer here
corresponds to the next available cpuidle state residency. Thus, cpu will
eventually end up in the deepest possible state.
Signed-off-by: Abhishek Goel <huntbag@linux.vnet.ibm.com>
---
Auto-promotion
v1 : started as auto promotion logic for cpuidle states in generic
driver
v2 : Removed timeout_needed and rebased the code to upstream kernel
Forced-wakeup
v1 : New patch with name of forced wakeup started
v2 : Extending the forced wakeup logic for all states. Setting the
decrementer instead of queuing up a hrtimer to implement the logic.
v3 : Cleanly handle setting/resetting of decrementer so as to not break
irq work
v4 : Changed type and name of set/reset decrementer fucntion
Handled irq_work_pending in try_set_dec_before_idle
arch/powerpc/include/asm/time.h | 2 ++
arch/powerpc/kernel/time.c | 43 +++++++++++++++++++++++++++++++
drivers/cpuidle/cpuidle-powernv.c | 40 ++++++++++++++++++++++++++++
3 files changed, 85 insertions(+)
diff --git a/arch/powerpc/include/asm/time.h b/arch/powerpc/include/asm/time.h
index 54f4ec1f9fab..294a472ce161 100644
--- a/arch/powerpc/include/asm/time.h
+++ b/arch/powerpc/include/asm/time.h
@@ -188,6 +188,8 @@ static inline unsigned long tb_ticks_since(unsigned long tstamp)
extern u64 mulhdu(u64, u64);
#endif
+extern bool try_set_dec_before_idle(u64 timeout);
+extern void try_reset_dec_after_idle(void);
extern void div128_by_32(u64 dividend_high, u64 dividend_low,
unsigned divisor, struct div_result *dr);
diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
index 694522308cd5..d004c0d8e099 100644
--- a/arch/powerpc/kernel/time.c
+++ b/arch/powerpc/kernel/time.c
@@ -576,6 +576,49 @@ void arch_irq_work_raise(void)
#endif /* CONFIG_IRQ_WORK */
+/*
+ * This function tries setting decrementer before entering into idle.
+ * Returns true if we have reprogrammed the decrementer for idle.
+ * Returns false if the decrementer is unchanged.
+ */
+bool try_set_dec_before_idle(u64 timeout)
+{
+ u64 *next_tb = this_cpu_ptr(&decrementers_next_tb);
+ u64 now = get_tb_or_rtc();
+
+ if (now + timeout > *next_tb)
+ return false;
+
+ set_dec(timeout);
+ if (test_irq_work_pending())
+ set_dec(1);
+
+ return true;
+}
+
+/*
+ * This function gets called if we have set decrementer before
+ * entering into idle. It tries to reset/restore the decrementer
+ * to its original value.
+ */
+void try_reset_dec_after_idle(void)
+{
+ u64 now;
+ u64 *next_tb;
+
+ if (test_irq_work_pending())
+ return;
+
+ now = get_tb_or_rtc();
+ next_tb = this_cpu_ptr(&decrementers_next_tb);
+ if (now >= *next_tb)
+ return;
+
+ set_dec(*next_tb - now);
+ if (test_irq_work_pending())
+ set_dec(1);
+}
+
/*
* timer_interrupt - gets called when the decrementer overflows,
* with interrupts disabled.
diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c
index 84b1ebe212b3..17e20e408ffe 100644
--- a/drivers/cpuidle/cpuidle-powernv.c
+++ b/drivers/cpuidle/cpuidle-powernv.c
@@ -21,6 +21,7 @@
#include <asm/opal.h>
#include <asm/runlatch.h>
#include <asm/cpuidle.h>
+#include <asm/time.h>
/*
* Expose only those Hardware idle states via the cpuidle framework
@@ -46,6 +47,26 @@ static struct stop_psscr_table stop_psscr_table[CPUIDLE_STATE_MAX] __read_mostly
static u64 default_snooze_timeout __read_mostly;
static bool snooze_timeout_en __read_mostly;
+static u64 forced_wakeup_timeout(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv,
+ int index)
+{
+ int i;
+
+ for (i = index + 1; i < drv->state_count; i++) {
+ struct cpuidle_state *s = &drv->states[i];
+ struct cpuidle_state_usage *su = &dev->states_usage[i];
+
+ if (s->disabled || su->disable)
+ continue;
+
+ return (s->target_residency + 2 * s->exit_latency) *
+ tb_ticks_per_usec;
+ }
+
+ return 0;
+}
+
static u64 get_snooze_timeout(struct cpuidle_device *dev,
struct cpuidle_driver *drv,
int index)
@@ -144,8 +165,27 @@ static int stop_loop(struct cpuidle_device *dev,
struct cpuidle_driver *drv,
int index)
{
+ u64 timeout_tb;
+ bool forced_wakeup = false;
+
+ timeout_tb = forced_wakeup_timeout(dev, drv, index);
+
+ /* Ensure that the timeout is at least one microsecond
+ * greater than current decrement value. Else, we will
+ * unnecesarily wakeup again within a microsecond if we
+ * set decrementer with such timeout value.
+ */
+ timeout_tb += 512;
+
+ if (timeout_tb)
+ forced_wakeup = try_set_dec_before_idle(timeout_tb);
+
power9_idle_type(stop_psscr_table[index].val,
stop_psscr_table[index].mask);
+
+ if (forced_wakeup)
+ try_reset_dec_after_idle();
+
return index;
}
--
2.17.1
^ permalink raw reply related
* [RFC v4 2/3] cpuidle : Add callback whenever a state usage is enabled/disabled
From: Abhishek Goel @ 2019-07-12 7:04 UTC (permalink / raw)
To: linux-kernel, linuxppc-dev, linux-pm
Cc: npiggin, rjw, daniel.lezcano, mpe, ego, dja, Abhishek Goel
In-Reply-To: <20190712070457.55242-1-huntbag@linux.vnet.ibm.com>
To force wakeup a cpu, we need to compute the timeout in the fast idle
path as a state may be enabled or disabled but there did not exist a
feedback to driver when a state is enabled or disabled.
This patch adds a callback whenever a state_usage records a store for
disable attribute.
Signed-off-by: Abhishek Goel <huntbag@linux.vnet.ibm.com>
---
drivers/cpuidle/sysfs.c | 15 ++++++++++++++-
include/linux/cpuidle.h | 4 ++++
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
index eb20adb5de23..141671a53967 100644
--- a/drivers/cpuidle/sysfs.c
+++ b/drivers/cpuidle/sysfs.c
@@ -415,8 +415,21 @@ static ssize_t cpuidle_state_store(struct kobject *kobj, struct attribute *attr,
struct cpuidle_state_usage *state_usage = kobj_to_state_usage(kobj);
struct cpuidle_state_attr *cattr = attr_to_stateattr(attr);
- if (cattr->store)
+ if (cattr->store) {
ret = cattr->store(state, state_usage, buf, size);
+ if (ret == size &&
+ strncmp(cattr->attr.name, "disable",
+ strlen("disable"))) {
+ struct kobject *cpuidle_kobj = kobj->parent;
+ struct cpuidle_device *dev =
+ to_cpuidle_device(cpuidle_kobj);
+ struct cpuidle_driver *drv =
+ cpuidle_get_cpu_driver(dev);
+
+ if (drv->disable_callback)
+ drv->disable_callback(dev, drv);
+ }
+ }
return ret;
}
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index bb9a0db89f1a..8a0e54bd0d5d 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -119,6 +119,10 @@ struct cpuidle_driver {
/* the driver handles the cpus in cpumask */
struct cpumask *cpumask;
+
+ void (*disable_callback)(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv);
+
};
#ifdef CONFIG_CPU_IDLE
--
2.17.1
^ permalink raw reply related
* [PATCH v4 0/3] Forced-wakeup for stop states on Powernv
From: Abhishek Goel @ 2019-07-12 7:04 UTC (permalink / raw)
To: linux-kernel, linuxppc-dev, linux-pm
Cc: npiggin, rjw, daniel.lezcano, mpe, ego, dja, Abhishek Goel
Currently, the cpuidle governors determine what idle state a idling CPU
should enter into based on heuristics that depend on the idle history on
that CPU. Given that no predictive heuristic is perfect, there are cases
where the governor predicts a shallow idle state, hoping that the CPU will
be busy soon. However, if no new workload is scheduled on that CPU in the
near future, the CPU will end up in the shallow state.
Motivation
----------
In case of POWER, this is problematic, when the predicted state in the
aforementioned scenario is a shallow stop state on a tickless system. As
we might get stuck into shallow states even for hours, in absence of ticks
or interrupts.
To address this, We forcefully wakeup the cpu by setting the decrementer.
The decrementer is set to a value that corresponds with the residency of
the next available state. Thus firing up a timer that will forcefully
wakeup the cpu. Few such iterations will essentially train the governor to
select a deeper state for that cpu, as the timer here corresponds to the
next available cpuidle state residency. Thus, cpu will eventually end up
in the deepest possible state and we won't get stuck in a shallow state
for long duration.
Experiment
----------
For earlier versions when this feature was meat to be only for shallow lite
states, I performed experiments for three scenarios to collect some data.
case 1 :
Without this patch and without tick retained, i.e. in a upstream kernel,
It would spend more than even a second to get out of stop0_lite.
case 2 : With tick retained in a upstream kernel -
Generally, we have a sched tick at 4ms(CONF_HZ = 250). Ideally I expected
it to take 8 sched tick to get out of stop0_lite. Experimentally,
observation was
=========================================================
sample min max 99percentile
20 4ms 12ms 4ms
=========================================================
It would take atleast one sched tick to get out of stop0_lite.
case 2 : With this patch (not stopping tick, but explicitly queuing a
timer)
============================================================
sample min max 99percentile
============================================================
20 144us 192us 144us
============================================================
Description of current implementation
-------------------------------------
We calculate timeout for the current idle state as the residency value
of the next available idle state. If the decrementer is set to be
greater than this timeout, we update the decrementer value with the
residency of next available idle state. Thus, essentially training the
governor to select the next available deeper state until we reach the
deepest state. Hence, we won't get stuck unnecessarily in shallow states
for longer duration.
--------------------------------
v1 of auto-promotion : https://lkml.org/lkml/2019/3/22/58 This patch was
implemented only for shallow lite state in generic cpuidle driver.
v2 : Removed timeout_needed and rebased to current
upstream kernel
Then,
v1 of forced-wakeup : Moved the code to cpuidle powernv driver and started
as forced wakeup instead of auto-promotion
v2 : Extended the forced wakeup logic for all states.
Setting the decrementer instead of queuing up a hrtimer to implement the
logic.
v3 : 1) Cleanly handle setting the decrementer after exiting out of stop
states.
2) Added a disable_callback feature to compute timeout whenever a
state is enbaled or disabled instead of computing everytime in fast
idle path.
3) Use disable callback to recompute timeout whenever state usage
is changed for a state. Also, cleaned up the get_snooze_timeout
function.
v4 : Changed the type and name of set/reset decrementer function.
Handled irq work pending in try_set_dec_before_idle.
No change in patch 2 and 3.
Abhishek Goel (3):
cpuidle-powernv : forced wakeup for stop states
cpuidle : Add callback whenever a state usage is enabled/disabled
cpuidle-powernv : Recompute the idle-state timeouts when state usage
is enabled/disabled
arch/powerpc/include/asm/time.h | 2 ++
arch/powerpc/kernel/time.c | 43 ++++++++++++++++++++++++
drivers/cpuidle/cpuidle-powernv.c | 55 +++++++++++++++++++++++--------
drivers/cpuidle/sysfs.c | 15 ++++++++-
include/linux/cpuidle.h | 5 +++
5 files changed, 106 insertions(+), 14 deletions(-)
--
2.17.1
^ permalink raw reply
* Re: [PATCH v1 3/6] driver/core: Convert to use built-in RCU list checking
From: Greg Kroah-Hartman @ 2019-07-12 5:19 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
c0d1n61at3, David S. Miller, edumazet, Hideaki YOSHIFUJI,
H. Peter Anvin, Ingo Molnar, Josh Triplett, keescook,
kernel-hardening, Lai Jiangshan, Len Brown, linux-acpi, linux-pci,
linux-pm, Mathieu Desnoyers, neilb, netdev, oleg,
Paul E. McKenney, 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: <20190711234401.220336-4-joel@joelfernandes.org>
On Thu, Jul 11, 2019 at 07:43:58PM -0400, Joel Fernandes (Google) wrote:
> list_for_each_entry_rcu has built-in RCU and lock checking. Make use of
> it in driver core.
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
> drivers/base/base.h | 1 +
> drivers/base/core.c | 10 ++++++++++
> drivers/base/power/runtime.c | 15 ++++++++++-----
> 3 files changed, 21 insertions(+), 5 deletions(-)
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
^ permalink raw reply
* Re: [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking
From: Joel Fernandes @ 2019-07-12 4:49 UTC (permalink / raw)
To: linux-kernel
Cc: Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov, c0d1n61at3,
David S. Miller, edumazet, Greg Kroah-Hartman, Hideaki YOSHIFUJI,
H. Peter Anvin, Ingo Molnar, Josh Triplett, keescook,
kernel-hardening, Lai Jiangshan, Len Brown, linux-acpi, linux-pci,
linux-pm, Mathieu Desnoyers, neilb, netdev, oleg,
Paul E. McKenney, 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: <20190711234401.220336-2-joel@joelfernandes.org>
On Thu, Jul 11, 2019 at 07:43:56PM -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>
> ---
> include/linux/rculist.h | 29 ++++++++++++++++++++++++-----
> include/linux/rcupdate.h | 7 +++++++
> kernel/rcu/Kconfig.debug | 11 +++++++++++
> kernel/rcu/update.c | 26 ++++++++++++++++++++++++++
> 4 files changed, 68 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> index e91ec9ddcd30..78c15ec6b2c9 100644
> --- a/include/linux/rculist.h
> +++ b/include/linux/rculist.h
> @@ -40,6 +40,23 @@ 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
> + */
> +
> +#define SIXTH_ARG(a1, a2, a3, a4, a5, a6, ...) a6
> +#define COUNT_VARGS(...) SIXTH_ARG(dummy, ## __VA_ARGS__, 4, 3, 2, 1, 0)
Fyi, I made a cosmetic change by deleting the above 2 unused macros.
- Joel
^ permalink raw reply
* Re: [PATCH v6] cpufreq/pasemi: fix an use-after-free in pas_cpufreq_cpu_init()
From: Viresh Kumar @ 2019-07-12 3:44 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: <1562899461-24045-1-git-send-email-wen.yang99@zte.com.cn>
On 12-07-19, 10:44, 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
> ---
> 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 | 26 ++++++++++++++------------
> 1 file changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/cpufreq/pasemi-cpufreq.c b/drivers/cpufreq/pasemi-cpufreq.c
> index 6b1e4ab..7d557f9 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");
>
> @@ -196,7 +194,11 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy *policy)
> policy->cur = pas_freqs[cur_astate].frequency;
> ppc_proc_freq = policy->cur * 1000ul;
>
> - return cpufreq_generic_init(policy, pas_freqs, get_gizmo_latency());
> + err = cpufreq_generic_init(policy, pas_freqs, get_gizmo_latency());
So you are trying to fix an earlier issue here with this. Should have
been a separate patch. Over that I have just sent a patch now to make
this routine return void.
https://lore.kernel.org/lkml/ee8cf5fb4b4a01fdf9199037ff6d835b935cfd13.1562902877.git.viresh.kumar@linaro.org/
So all you need to do is to remove the label out_unmap_sdcpwr instead.
> + if (err)
> + goto out_unmap_sdcpwr;
> +
> + return 0;
>
> out_unmap_sdcpwr:
> iounmap(sdcpwr_mapbase);
> --
> 2.9.5
--
viresh
^ permalink raw reply
* [PATCH] cpufreq: Make cpufreq_generic_init() return void
From: Viresh Kumar @ 2019-07-12 3:42 UTC (permalink / raw)
To: Rafael Wysocki, Markus Mayer, bcm-kernel-feedback-list, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
NXP Linux Team, Keguang Zhang, Jiaxun Yang, Kevin Hilman,
Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Kukjin Kim, Krzysztof Kozlowski, Thierry Reding, Jonathan Hunter
Cc: Viresh Kumar, linux-pm, Vincent Guittot, wen.yang99, linux-kernel,
linux-arm-kernel, linux-mips, linux-omap, linuxppc-dev,
linux-samsung-soc, linux-tegra
It always returns 0 (success) and its return type should really be void.
Over that, many drivers have added error handling code based on its
return value, which is not required at all.
change its return type to void and update all the callers.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/cpufreq/bmips-cpufreq.c | 7 ++-----
drivers/cpufreq/cpufreq.c | 4 +---
drivers/cpufreq/davinci-cpufreq.c | 3 ++-
drivers/cpufreq/imx6q-cpufreq.c | 6 ++----
drivers/cpufreq/kirkwood-cpufreq.c | 3 ++-
drivers/cpufreq/loongson1-cpufreq.c | 8 +++-----
drivers/cpufreq/loongson2_cpufreq.c | 3 ++-
drivers/cpufreq/maple-cpufreq.c | 3 ++-
drivers/cpufreq/omap-cpufreq.c | 15 +++++----------
drivers/cpufreq/pasemi-cpufreq.c | 3 ++-
drivers/cpufreq/pmac32-cpufreq.c | 3 ++-
drivers/cpufreq/pmac64-cpufreq.c | 3 ++-
drivers/cpufreq/s3c2416-cpufreq.c | 9 ++-------
drivers/cpufreq/s3c64xx-cpufreq.c | 15 +++------------
drivers/cpufreq/s5pv210-cpufreq.c | 3 ++-
drivers/cpufreq/sa1100-cpufreq.c | 3 ++-
drivers/cpufreq/sa1110-cpufreq.c | 3 ++-
drivers/cpufreq/spear-cpufreq.c | 3 ++-
drivers/cpufreq/tegra20-cpufreq.c | 8 +-------
include/linux/cpufreq.h | 2 +-
20 files changed, 42 insertions(+), 65 deletions(-)
diff --git a/drivers/cpufreq/bmips-cpufreq.c b/drivers/cpufreq/bmips-cpufreq.c
index 56a4ebbf00e0..2b187d802fe3 100644
--- a/drivers/cpufreq/bmips-cpufreq.c
+++ b/drivers/cpufreq/bmips-cpufreq.c
@@ -141,11 +141,8 @@ static int bmips_cpufreq_init(struct cpufreq_policy *policy)
return ret;
}
- ret = cpufreq_generic_init(policy, freq_table, TRANSITION_LATENCY);
- if (ret)
- bmips_cpufreq_exit(policy);
- else
- pr_info("%s: registered\n", BMIPS_CPUFREQ_NAME);
+ cpufreq_generic_init(policy, freq_table, TRANSITION_LATENCY);
+ pr_info("%s: registered\n", BMIPS_CPUFREQ_NAME);
return ret;
}
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 4d6043ee7834..8dda62367816 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -159,7 +159,7 @@ EXPORT_SYMBOL_GPL(arch_set_freq_scale);
* - set policies transition latency
* - policy->cpus with all possible CPUs
*/
-int cpufreq_generic_init(struct cpufreq_policy *policy,
+void cpufreq_generic_init(struct cpufreq_policy *policy,
struct cpufreq_frequency_table *table,
unsigned int transition_latency)
{
@@ -171,8 +171,6 @@ int cpufreq_generic_init(struct cpufreq_policy *policy,
* share the clock and voltage and clock.
*/
cpumask_setall(policy->cpus);
-
- return 0;
}
EXPORT_SYMBOL_GPL(cpufreq_generic_init);
diff --git a/drivers/cpufreq/davinci-cpufreq.c b/drivers/cpufreq/davinci-cpufreq.c
index 3de48ae60c29..297d23cad8b5 100644
--- a/drivers/cpufreq/davinci-cpufreq.c
+++ b/drivers/cpufreq/davinci-cpufreq.c
@@ -90,7 +90,8 @@ static int davinci_cpu_init(struct cpufreq_policy *policy)
* Setting the latency to 2000 us to accommodate addition of drivers
* to pre/post change notification list.
*/
- return cpufreq_generic_init(policy, freq_table, 2000 * 1000);
+ cpufreq_generic_init(policy, freq_table, 2000 * 1000);
+ return 0;
}
static struct cpufreq_driver davinci_driver = {
diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index 47ccfa6b17b7..648a09a1778a 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -190,14 +190,12 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
static int imx6q_cpufreq_init(struct cpufreq_policy *policy)
{
- int ret;
-
policy->clk = clks[ARM].clk;
- ret = cpufreq_generic_init(policy, freq_table, transition_latency);
+ cpufreq_generic_init(policy, freq_table, transition_latency);
policy->suspend_freq = max_freq;
dev_pm_opp_of_register_em(policy->cpus);
- return ret;
+ return 0;
}
static struct cpufreq_driver imx6q_cpufreq_driver = {
diff --git a/drivers/cpufreq/kirkwood-cpufreq.c b/drivers/cpufreq/kirkwood-cpufreq.c
index 7ab564c1f7ae..cb74bdc5baaa 100644
--- a/drivers/cpufreq/kirkwood-cpufreq.c
+++ b/drivers/cpufreq/kirkwood-cpufreq.c
@@ -85,7 +85,8 @@ static int kirkwood_cpufreq_target(struct cpufreq_policy *policy,
/* Module init and exit code */
static int kirkwood_cpufreq_cpu_init(struct cpufreq_policy *policy)
{
- return cpufreq_generic_init(policy, kirkwood_freq_table, 5000);
+ cpufreq_generic_init(policy, kirkwood_freq_table, 5000);
+ return 0;
}
static struct cpufreq_driver kirkwood_cpufreq_driver = {
diff --git a/drivers/cpufreq/loongson1-cpufreq.c b/drivers/cpufreq/loongson1-cpufreq.c
index 21c9ce8526c0..0ea88778882a 100644
--- a/drivers/cpufreq/loongson1-cpufreq.c
+++ b/drivers/cpufreq/loongson1-cpufreq.c
@@ -81,7 +81,7 @@ static int ls1x_cpufreq_init(struct cpufreq_policy *policy)
struct device *cpu_dev = get_cpu_device(policy->cpu);
struct cpufreq_frequency_table *freq_tbl;
unsigned int pll_freq, freq;
- int steps, i, ret;
+ int steps, i;
pll_freq = clk_get_rate(cpufreq->pll_clk) / 1000;
@@ -103,11 +103,9 @@ static int ls1x_cpufreq_init(struct cpufreq_policy *policy)
freq_tbl[i].frequency = CPUFREQ_TABLE_END;
policy->clk = cpufreq->clk;
- ret = cpufreq_generic_init(policy, freq_tbl, 0);
- if (ret)
- kfree(freq_tbl);
+ cpufreq_generic_init(policy, freq_tbl, 0);
- return ret;
+ return 0;
}
static int ls1x_cpufreq_exit(struct cpufreq_policy *policy)
diff --git a/drivers/cpufreq/loongson2_cpufreq.c b/drivers/cpufreq/loongson2_cpufreq.c
index da344696beed..890813e0bb76 100644
--- a/drivers/cpufreq/loongson2_cpufreq.c
+++ b/drivers/cpufreq/loongson2_cpufreq.c
@@ -95,7 +95,8 @@ static int loongson2_cpufreq_cpu_init(struct cpufreq_policy *policy)
}
policy->clk = cpuclk;
- return cpufreq_generic_init(policy, &loongson2_clockmod_table[0], 0);
+ cpufreq_generic_init(policy, &loongson2_clockmod_table[0], 0);
+ return 0;
}
static int loongson2_cpufreq_exit(struct cpufreq_policy *policy)
diff --git a/drivers/cpufreq/maple-cpufreq.c b/drivers/cpufreq/maple-cpufreq.c
index f5220b3d4ec5..28d346062166 100644
--- a/drivers/cpufreq/maple-cpufreq.c
+++ b/drivers/cpufreq/maple-cpufreq.c
@@ -140,7 +140,8 @@ static unsigned int maple_cpufreq_get_speed(unsigned int cpu)
static int maple_cpufreq_cpu_init(struct cpufreq_policy *policy)
{
- return cpufreq_generic_init(policy, maple_cpu_freqs, 12000);
+ cpufreq_generic_init(policy, maple_cpu_freqs, 12000);
+ return 0;
}
static struct cpufreq_driver maple_cpufreq_driver = {
diff --git a/drivers/cpufreq/omap-cpufreq.c b/drivers/cpufreq/omap-cpufreq.c
index 29643f06a3c3..8d14b42a8c6f 100644
--- a/drivers/cpufreq/omap-cpufreq.c
+++ b/drivers/cpufreq/omap-cpufreq.c
@@ -122,23 +122,18 @@ static int omap_cpu_init(struct cpufreq_policy *policy)
dev_err(mpu_dev,
"%s: cpu%d: failed creating freq table[%d]\n",
__func__, policy->cpu, result);
- goto fail;
+ clk_put(policy->clk);
+ return result;
}
}
atomic_inc_return(&freq_table_users);
/* FIXME: what's the actual transition time? */
- result = cpufreq_generic_init(policy, freq_table, 300 * 1000);
- if (!result) {
- dev_pm_opp_of_register_em(policy->cpus);
- return 0;
- }
+ cpufreq_generic_init(policy, freq_table, 300 * 1000);
+ dev_pm_opp_of_register_em(policy->cpus);
- freq_table_free();
-fail:
- clk_put(policy->clk);
- return result;
+ return 0;
}
static int omap_cpu_exit(struct cpufreq_policy *policy)
diff --git a/drivers/cpufreq/pasemi-cpufreq.c b/drivers/cpufreq/pasemi-cpufreq.c
index 6b1e4abe3248..93f39a1d4c3d 100644
--- a/drivers/cpufreq/pasemi-cpufreq.c
+++ b/drivers/cpufreq/pasemi-cpufreq.c
@@ -196,7 +196,8 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy *policy)
policy->cur = pas_freqs[cur_astate].frequency;
ppc_proc_freq = policy->cur * 1000ul;
- return cpufreq_generic_init(policy, pas_freqs, get_gizmo_latency());
+ cpufreq_generic_init(policy, pas_freqs, get_gizmo_latency());
+ return 0;
out_unmap_sdcpwr:
iounmap(sdcpwr_mapbase);
diff --git a/drivers/cpufreq/pmac32-cpufreq.c b/drivers/cpufreq/pmac32-cpufreq.c
index 650104d729f3..73621bc11976 100644
--- a/drivers/cpufreq/pmac32-cpufreq.c
+++ b/drivers/cpufreq/pmac32-cpufreq.c
@@ -372,7 +372,8 @@ static int pmac_cpufreq_target( struct cpufreq_policy *policy,
static int pmac_cpufreq_cpu_init(struct cpufreq_policy *policy)
{
- return cpufreq_generic_init(policy, pmac_cpu_freqs, transition_latency);
+ cpufreq_generic_init(policy, pmac_cpu_freqs, transition_latency);
+ return 0;
}
static u32 read_gpio(struct device_node *np)
diff --git a/drivers/cpufreq/pmac64-cpufreq.c b/drivers/cpufreq/pmac64-cpufreq.c
index 1af3492a000d..d7542a106e6b 100644
--- a/drivers/cpufreq/pmac64-cpufreq.c
+++ b/drivers/cpufreq/pmac64-cpufreq.c
@@ -321,7 +321,8 @@ static unsigned int g5_cpufreq_get_speed(unsigned int cpu)
static int g5_cpufreq_cpu_init(struct cpufreq_policy *policy)
{
- return cpufreq_generic_init(policy, g5_cpu_freqs, transition_latency);
+ cpufreq_generic_init(policy, g5_cpu_freqs, transition_latency);
+ return 0;
}
static struct cpufreq_driver g5_cpufreq_driver = {
diff --git a/drivers/cpufreq/s3c2416-cpufreq.c b/drivers/cpufreq/s3c2416-cpufreq.c
index f7ff1ed7fef1..106910351c41 100644
--- a/drivers/cpufreq/s3c2416-cpufreq.c
+++ b/drivers/cpufreq/s3c2416-cpufreq.c
@@ -447,21 +447,16 @@ static int s3c2416_cpufreq_driver_init(struct cpufreq_policy *policy)
/* Datasheet says PLL stabalisation time must be at least 300us,
* so but add some fudge. (reference in LOCKCON0 register description)
*/
- ret = cpufreq_generic_init(policy, s3c_freq->freq_table,
+ cpufreq_generic_init(policy, s3c_freq->freq_table,
(500 * 1000) + s3c_freq->regulator_latency);
- if (ret)
- goto err_freq_table;
-
register_reboot_notifier(&s3c2416_cpufreq_reboot_notifier);
return 0;
-err_freq_table:
#ifdef CONFIG_ARM_S3C2416_CPUFREQ_VCORESCALE
- regulator_put(s3c_freq->vddarm);
err_vddarm:
-#endif
clk_put(s3c_freq->armclk);
+#endif
err_armclk:
clk_put(s3c_freq->hclk);
err_hclk:
diff --git a/drivers/cpufreq/s3c64xx-cpufreq.c b/drivers/cpufreq/s3c64xx-cpufreq.c
index 37df2d892eb0..ca756269a48a 100644
--- a/drivers/cpufreq/s3c64xx-cpufreq.c
+++ b/drivers/cpufreq/s3c64xx-cpufreq.c
@@ -144,7 +144,6 @@ static void s3c64xx_cpufreq_config_regulator(void)
static int s3c64xx_cpufreq_driver_init(struct cpufreq_policy *policy)
{
- int ret;
struct cpufreq_frequency_table *freq;
if (policy->cpu != 0)
@@ -165,8 +164,7 @@ static int s3c64xx_cpufreq_driver_init(struct cpufreq_policy *policy)
#ifdef CONFIG_REGULATOR
vddarm = regulator_get(NULL, "vddarm");
if (IS_ERR(vddarm)) {
- ret = PTR_ERR(vddarm);
- pr_err("Failed to obtain VDDARM: %d\n", ret);
+ pr_err("Failed to obtain VDDARM: %d\n", PTR_ERR(vddarm));
pr_err("Only frequency scaling available\n");
vddarm = NULL;
} else {
@@ -196,16 +194,9 @@ static int s3c64xx_cpufreq_driver_init(struct cpufreq_policy *policy)
* the PLLs, which we don't currently) is ~300us worst case,
* but add some fudge.
*/
- ret = cpufreq_generic_init(policy, s3c64xx_freq_table,
+ cpufreq_generic_init(policy, s3c64xx_freq_table,
(500 * 1000) + regulator_latency);
- if (ret != 0) {
- pr_err("Failed to configure frequency table: %d\n",
- ret);
- regulator_put(vddarm);
- clk_put(policy->clk);
- }
-
- return ret;
+ return 0;
}
static struct cpufreq_driver s3c64xx_cpufreq_driver = {
diff --git a/drivers/cpufreq/s5pv210-cpufreq.c b/drivers/cpufreq/s5pv210-cpufreq.c
index e5cb17d4be7b..5d10030f2560 100644
--- a/drivers/cpufreq/s5pv210-cpufreq.c
+++ b/drivers/cpufreq/s5pv210-cpufreq.c
@@ -541,7 +541,8 @@ static int s5pv210_cpu_init(struct cpufreq_policy *policy)
s5pv210_dram_conf[1].freq = clk_get_rate(dmc1_clk);
policy->suspend_freq = SLEEP_FREQ;
- return cpufreq_generic_init(policy, s5pv210_freq_table, 40000);
+ cpufreq_generic_init(policy, s5pv210_freq_table, 40000);
+ return 0;
out_dmc1:
clk_put(dmc0_clk);
diff --git a/drivers/cpufreq/sa1100-cpufreq.c b/drivers/cpufreq/sa1100-cpufreq.c
index ab5cab93e638..5c075ef6adc0 100644
--- a/drivers/cpufreq/sa1100-cpufreq.c
+++ b/drivers/cpufreq/sa1100-cpufreq.c
@@ -181,7 +181,8 @@ static int sa1100_target(struct cpufreq_policy *policy, unsigned int ppcr)
static int __init sa1100_cpu_init(struct cpufreq_policy *policy)
{
- return cpufreq_generic_init(policy, sa11x0_freq_table, 0);
+ cpufreq_generic_init(policy, sa11x0_freq_table, 0);
+ return 0;
}
static struct cpufreq_driver sa1100_driver __refdata = {
diff --git a/drivers/cpufreq/sa1110-cpufreq.c b/drivers/cpufreq/sa1110-cpufreq.c
index dab54e051c0e..d9d04d935b3a 100644
--- a/drivers/cpufreq/sa1110-cpufreq.c
+++ b/drivers/cpufreq/sa1110-cpufreq.c
@@ -303,7 +303,8 @@ static int sa1110_target(struct cpufreq_policy *policy, unsigned int ppcr)
static int __init sa1110_cpu_init(struct cpufreq_policy *policy)
{
- return cpufreq_generic_init(policy, sa11x0_freq_table, 0);
+ cpufreq_generic_init(policy, sa11x0_freq_table, 0);
+ return 0;
}
/* sa1110_driver needs __refdata because it must remain after init registers
diff --git a/drivers/cpufreq/spear-cpufreq.c b/drivers/cpufreq/spear-cpufreq.c
index 4074e2615522..73bd8dc47074 100644
--- a/drivers/cpufreq/spear-cpufreq.c
+++ b/drivers/cpufreq/spear-cpufreq.c
@@ -153,8 +153,9 @@ static int spear_cpufreq_target(struct cpufreq_policy *policy,
static int spear_cpufreq_init(struct cpufreq_policy *policy)
{
policy->clk = spear_cpufreq.clk;
- return cpufreq_generic_init(policy, spear_cpufreq.freq_tbl,
+ cpufreq_generic_init(policy, spear_cpufreq.freq_tbl,
spear_cpufreq.transition_latency);
+ return 0;
}
static struct cpufreq_driver spear_cpufreq_driver = {
diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
index 3c32cc7b0671..f84ecd22f488 100644
--- a/drivers/cpufreq/tegra20-cpufreq.c
+++ b/drivers/cpufreq/tegra20-cpufreq.c
@@ -118,17 +118,11 @@ static int tegra_target(struct cpufreq_policy *policy, unsigned int index)
static int tegra_cpu_init(struct cpufreq_policy *policy)
{
struct tegra20_cpufreq *cpufreq = cpufreq_get_driver_data();
- int ret;
clk_prepare_enable(cpufreq->cpu_clk);
/* FIXME: what's the actual transition time? */
- ret = cpufreq_generic_init(policy, freq_table, 300 * 1000);
- if (ret) {
- clk_disable_unprepare(cpufreq->cpu_clk);
- return ret;
- }
-
+ cpufreq_generic_init(policy, freq_table, 300 * 1000);
policy->clk = cpufreq->cpu_clk;
policy->suspend_freq = freq_table[0].frequency;
return 0;
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index d757a56a74dc..536a049d7ecc 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -992,7 +992,7 @@ extern struct freq_attr *cpufreq_generic_attr[];
int cpufreq_table_validate_and_sort(struct cpufreq_policy *policy);
unsigned int cpufreq_generic_get(unsigned int cpu);
-int cpufreq_generic_init(struct cpufreq_policy *policy,
+void cpufreq_generic_init(struct cpufreq_policy *policy,
struct cpufreq_frequency_table *table,
unsigned int transition_latency);
#endif /* _LINUX_CPUFREQ_H */
--
2.21.0.rc0.269.g1a574e7a288b
^ permalink raw reply related
* Re: [PATCHv3 6/6] drivers/cpufreq: Add a CPUFreq driver for AMD processors (Fam17h and later)
From: Viresh Kumar @ 2019-07-12 3:10 UTC (permalink / raw)
To: Janakarajan Natarajan
Cc: Natarajan, Janakarajan, linux-acpi@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
devel@acpica.org, Rafael J . Wysocki, Len Brown, Robert Moore,
Erik Schmauss, Ghannam, Yazen
In-Reply-To: <eb208b15-d6b5-192d-b600-1f791c91eb4c@amd.com>
On 11-07-19, 16:58, Janakarajan Natarajan wrote:
> On 7/11/2019 1:12 AM, Viresh Kumar wrote:
> > On 10-07-19, 18:37, Natarajan, Janakarajan wrote:
> >> +static int amd_cpufreq_cpu_init(struct cpufreq_policy *policy)
> >> +{
> >> + return 0;
> >> +}
> >> +
> >> +static int amd_cpufreq_cpu_exit(struct cpufreq_policy *policy)
> >> +{
> >> + return 0;
> >> +}
> >> +
> >> +static int amd_cpufreq_cpu_verify(struct cpufreq_policy *policy)
> >> +{
> >> + return 0;
> >> +}
> >> +
> >> +static int amd_cpufreq_cpu_target_index(struct cpufreq_policy *policy,
> >> + unsigned int index)
> >> +{
> >> + return 0;
> >> +}
> > All empty helpers ? There is nothing you need to do ?
>
>
> When we posted v2 of this patchset, Rafael let us know that he was
> uncomfortable
>
> going behind the (acpi-cpufreq) drivers back by letting the user
> communicate directly
>
> with the platform. That's the reason we have an empty driver whose
> primary purpose
>
> is to expose sysfs entries for the user.
I read his comments now and what he suggested is:
"What about handling this like the others do, through a proper cpufreq
driver?"
I am not sure if he meant something like that you have here. Only one
cpufreq driver can be registered at any point of time with the kernel,
and so if this one is there then acpi-cpufreq or intel-pstate can't be
there. Who will do DVFS ?
--
viresh
^ permalink raw reply
* [PATCH v6] cpufreq/pasemi: fix an use-after-free in pas_cpufreq_cpu_init()
From: Wen Yang @ 2019-07-12 2:44 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
---
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 | 26 ++++++++++++++------------
1 file changed, 14 insertions(+), 12 deletions(-)
diff --git a/drivers/cpufreq/pasemi-cpufreq.c b/drivers/cpufreq/pasemi-cpufreq.c
index 6b1e4ab..7d557f9 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");
@@ -196,7 +194,11 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy *policy)
policy->cur = pas_freqs[cur_astate].frequency;
ppc_proc_freq = policy->cur * 1000ul;
- return cpufreq_generic_init(policy, pas_freqs, get_gizmo_latency());
+ err = cpufreq_generic_init(policy, pas_freqs, get_gizmo_latency());
+ if (err)
+ goto out_unmap_sdcpwr;
+
+ return 0;
out_unmap_sdcpwr:
iounmap(sdcpwr_mapbase);
--
2.9.5
^ permalink raw reply related
* Re: [PATCH v1 0/6] Harden list_for_each_entry_rcu() and family
From: Joel Fernandes @ 2019-07-11 23:52 UTC (permalink / raw)
To: linux-kernel
Cc: Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov, c0d1n61at3,
David S. Miller, edumazet, Greg Kroah-Hartman, Hideaki YOSHIFUJI,
H. Peter Anvin, Ingo Molnar, Josh Triplett, keescook,
kernel-hardening, Lai Jiangshan, Len Brown, linux-acpi, linux-pci,
linux-pm, Mathieu Desnoyers, neilb, netdev, oleg,
Paul E. McKenney, 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: <20190711234401.220336-1-joel@joelfernandes.org>
On Thu, Jul 11, 2019 at 07:43:55PM -0400, Joel Fernandes (Google) wrote:
> Hi,
> This series aims to provide lockdep checking to RCU list macros.
>
> 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.
I forgot to add in my cover letter, 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.
Also, I plans to update the RCU documentation as well which I will do, but do
review this series and thank you!
^ permalink raw reply
* [PATCH v1 0/6] Harden list_for_each_entry_rcu() and family
From: Joel Fernandes (Google) @ 2019-07-11 23:43 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Alexey Kuznetsov, Bjorn Helgaas,
Borislav Petkov, c0d1n61at3, David S. Miller, edumazet,
Greg Kroah-Hartman, Hideaki YOSHIFUJI, H. Peter Anvin,
Ingo Molnar, Josh Triplett, keescook, kernel-hardening,
Lai Jiangshan, Len Brown, linux-acpi, linux-pci, linux-pm,
Mathieu Desnoyers, neilb, netdev, oleg, Paul E. McKenney,
Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
Hi,
This series aims to provide lockdep checking to RCU list macros.
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.
Joel Fernandes (Google) (6):
rcu: Add support for consolidated-RCU reader checking
ipv4: add lockdep condition to fix for_each_entry
driver/core: Convert to use built-in RCU list checking
workqueue: Convert for_each_wq to use built-in list check
x86/pci: Pass lockdep condition to pcm_mmcfg_list iterator
acpi: Use built-in RCU list checking for acpi_ioremaps list
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/rculist.h | 29 ++++++++++++++++++++++++-----
include/linux/rcupdate.h | 7 +++++++
kernel/rcu/Kconfig.debug | 11 +++++++++++
kernel/rcu/update.c | 26 ++++++++++++++++++++++++++
kernel/workqueue.c | 5 ++---
net/ipv4/fib_frontend.c | 3 ++-
11 files changed, 100 insertions(+), 18 deletions(-)
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply
* [PATCH v1 4/6] workqueue: Convert for_each_wq to use built-in list check
From: Joel Fernandes (Google) @ 2019-07-11 23:43 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Alexey Kuznetsov, Bjorn Helgaas,
Borislav Petkov, c0d1n61at3, David S. Miller, edumazet,
Greg Kroah-Hartman, Hideaki YOSHIFUJI, H. Peter Anvin,
Ingo Molnar, Josh Triplett, keescook, kernel-hardening,
Lai Jiangshan, Len Brown, linux-acpi, linux-pci, linux-pm,
Mathieu Desnoyers, neilb, netdev, oleg, Paul E. McKenney,
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: <20190711234401.220336-1-joel@joelfernandes.org>
list_for_each_entry_rcu now has support to check for RCU reader sections
as well as lock. Just use the support in it, instead of explictly
checking in the caller.
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
kernel/workqueue.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 9657315405de..91ed7aca16e5 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -424,9 +424,8 @@ static void workqueue_sysfs_unregister(struct workqueue_struct *wq);
* ignored.
*/
#define for_each_pwq(pwq, wq) \
- list_for_each_entry_rcu((pwq), &(wq)->pwqs, pwqs_node) \
- if (({ assert_rcu_or_wq_mutex(wq); false; })) { } \
- else
+ list_for_each_entry_rcu((pwq), &(wq)->pwqs, pwqs_node, \
+ lock_is_held(&(wq->mutex).dep_map))
#ifdef CONFIG_DEBUG_OBJECTS_WORK
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
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