* [PATCH V2] cpufreq: Make cpufreq_generic_init() return void
From: Viresh Kumar @ 2019-07-13 14:36 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, linux-kernel,
linux-arm-kernel, linux-mips, linux-omap, linuxppc-dev,
linux-samsung-soc, linux-tegra
In-Reply-To: <ee8cf5fb4b4a01fdf9199037ff6d835b935cfd13.1562902877.git.viresh.kumar@linaro.org>
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>
---
V1->V2:
- Fixed compilation warning with s3c64xx driver by replace %d with %ld
(Reported by buildbot).
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..af0c00dabb22 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: %ld\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: [PATCH v2 3/9] rcu/sync: Remove custom check for reader-section
From: Paul E. McKenney @ 2019-07-13 14:41 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, Oleg Nesterov, 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, 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: <20190713133049.GA133650@google.com>
On Sat, Jul 13, 2019 at 09:30:49AM -0400, Joel Fernandes wrote:
> On Sat, Jul 13, 2019 at 01:21:14AM -0700, Paul E. McKenney wrote:
> > On Fri, Jul 12, 2019 at 11:10:08PM -0400, Joel Fernandes wrote:
> > > On Fri, Jul 12, 2019 at 11:01:50PM -0400, Joel Fernandes wrote:
> > > > On Fri, Jul 12, 2019 at 04:32:06PM -0700, Paul E. McKenney wrote:
> > > > > On Fri, Jul 12, 2019 at 05:35:59PM -0400, Joel Fernandes wrote:
> > > > > > On Fri, Jul 12, 2019 at 01:00:18PM -0400, Joel Fernandes (Google) wrote:
> > > > > > > The rcu/sync code was doing its own check whether we are in a reader
> > > > > > > section. With RCU consolidating flavors and the generic helper added in
> > > > > > > this series, this is no longer need. We can just use the generic helper
> > > > > > > and it results in a nice cleanup.
> > > > > > >
> > > > > > > Cc: Oleg Nesterov <oleg@redhat.com>
> > > > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > > > > >
> > > > > > Hi Oleg,
> > > > > > Slightly unrelated to the patch,
> > > > > > I tried hard to understand this comment below in percpu_down_read() but no dice.
> > > > > >
> > > > > > I do understand how rcu sync and percpu rwsem works, however the comment
> > > > > > below didn't make much sense to me. For one, there's no readers_fast anymore
> > > > > > so I did not follow what readers_fast means. Could the comment be updated to
> > > > > > reflect latest changes?
> > > > > > Also could you help understand how is a writer not able to change
> > > > > > sem->state and count the per-cpu read counters at the same time as the
> > > > > > comment tries to say?
> > > > > >
> > > > > > /*
> > > > > > * We are in an RCU-sched read-side critical section, so the writer
> > > > > > * cannot both change sem->state from readers_fast and start checking
> > > > > > * counters while we are here. So if we see !sem->state, we know that
> > > > > > * the writer won't be checking until we're past the preempt_enable()
> > > > > > * and that once the synchronize_rcu() is done, the writer will see
> > > > > > * anything we did within this RCU-sched read-size critical section.
> > > > > > */
> > > > > >
> > > > > > Also,
> > > > > > I guess we could get rid of all of the gp_ops struct stuff now that since all
> > > > > > the callbacks are the same now. I will post that as a follow-up patch to this
> > > > > > series.
> > > > >
> > > > > Hello, Joel,
> > > > >
> > > > > Oleg has a set of patches updating this code that just hit mainline
> > > > > this week. These patches get rid of the code that previously handled
> > > > > RCU's multiple flavors. Or are you looking at current mainline and
> > > > > me just missing your point?
> > > > >
> > > >
> > > > Hi Paul,
> > > > You are right on point. I have a bad habit of not rebasing my trees. In this
> > > > case the feature branch of mine in concern was based on v5.1. Needless to
> > > > say, I need to rebase my tree.
> > > >
> > > > Yes, this sync clean up patch does conflict when I rebase, but other patches
> > > > rebase just fine.
> > > >
> > > > The 2 options I see are:
> > > > 1. Let us drop this patch for now and I resend it later.
> > > > 2. I resend all patches based on Linus's master branch.
> > >
> > > Below is the updated patch based on Linus master branch:
> > >
> > > ---8<-----------------------
> > >
> > > >From 5f40c9a07fcf3d6dafc2189599d0ba9443097d0f Mon Sep 17 00:00:00 2001
> > > From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
> > > Date: Fri, 12 Jul 2019 12:13:27 -0400
> > > Subject: [PATCH v2.1 3/9] rcu/sync: Remove custom check for reader-section
> > >
> > > The rcu/sync code was doing its own check whether we are in a reader
> > > section. With RCU consolidating flavors and the generic helper added in
> > > this series, this is no longer need. We can just use the generic helper
> > > and it results in a nice cleanup.
> > >
> > > Cc: Oleg Nesterov <oleg@redhat.com>
> > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > > ---
> > > include/linux/rcu_sync.h | 4 +---
> > > 1 file changed, 1 insertion(+), 3 deletions(-)
> > >
> > > diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h
> > > index 9b83865d24f9..0027d4c8087c 100644
> > > --- a/include/linux/rcu_sync.h
> > > +++ b/include/linux/rcu_sync.h
> > > @@ -31,9 +31,7 @@ struct rcu_sync {
> > > */
> > > static inline bool rcu_sync_is_idle(struct rcu_sync *rsp)
> > > {
> > > - RCU_LOCKDEP_WARN(!rcu_read_lock_held() &&
> > > - !rcu_read_lock_bh_held() &&
> > > - !rcu_read_lock_sched_held(),
> > > + RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),
> >
> > I believe that replacing rcu_read_lock_sched_held() with preemptible()
> > in a CONFIG_PREEMPT=n kernel will give you false-positive splats here.
> > If you have not already done so, could you please give it a try?
>
> Hi Paul,
> I don't think it will cause splats for !CONFIG_PREEMPT.
>
> Currently, rcu_read_lock_any_held() introduced in this patch returns true if
> !preemptible(). This means that:
>
> The following expression above:
> RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),...)
>
> Becomes:
> RCU_LOCKDEP_WARN(preemptible(), ...)
>
> For, CONFIG_PREEMPT=n kernels, this means:
> RCU_LOCKDEP_WARN(0, ...)
>
> Which would mean no splats. Or, did I miss the point?
I suggest trying it out on a CONFIG_PREEMPT=n kernel.
Thanx, Paul
^ permalink raw reply
* Re: [PATCH v2 3/9] rcu/sync: Remove custom check for reader-section
From: Joel Fernandes @ 2019-07-13 15:36 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linux-kernel, Oleg Nesterov, 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, 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: <20190713144108.GD26519@linux.ibm.com>
On Sat, Jul 13, 2019 at 07:41:08AM -0700, Paul E. McKenney wrote:
> On Sat, Jul 13, 2019 at 09:30:49AM -0400, Joel Fernandes wrote:
> > On Sat, Jul 13, 2019 at 01:21:14AM -0700, Paul E. McKenney wrote:
> > > On Fri, Jul 12, 2019 at 11:10:08PM -0400, Joel Fernandes wrote:
> > > > On Fri, Jul 12, 2019 at 11:01:50PM -0400, Joel Fernandes wrote:
> > > > > On Fri, Jul 12, 2019 at 04:32:06PM -0700, Paul E. McKenney wrote:
> > > > > > On Fri, Jul 12, 2019 at 05:35:59PM -0400, Joel Fernandes wrote:
> > > > > > > On Fri, Jul 12, 2019 at 01:00:18PM -0400, Joel Fernandes (Google) wrote:
> > > > > > > > The rcu/sync code was doing its own check whether we are in a reader
> > > > > > > > section. With RCU consolidating flavors and the generic helper added in
> > > > > > > > this series, this is no longer need. We can just use the generic helper
> > > > > > > > and it results in a nice cleanup.
> > > > > > > >
> > > > > > > > Cc: Oleg Nesterov <oleg@redhat.com>
> > > > > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > > > > > >
> > > > > > > Hi Oleg,
> > > > > > > Slightly unrelated to the patch,
> > > > > > > I tried hard to understand this comment below in percpu_down_read() but no dice.
> > > > > > >
> > > > > > > I do understand how rcu sync and percpu rwsem works, however the comment
> > > > > > > below didn't make much sense to me. For one, there's no readers_fast anymore
> > > > > > > so I did not follow what readers_fast means. Could the comment be updated to
> > > > > > > reflect latest changes?
> > > > > > > Also could you help understand how is a writer not able to change
> > > > > > > sem->state and count the per-cpu read counters at the same time as the
> > > > > > > comment tries to say?
> > > > > > >
> > > > > > > /*
> > > > > > > * We are in an RCU-sched read-side critical section, so the writer
> > > > > > > * cannot both change sem->state from readers_fast and start checking
> > > > > > > * counters while we are here. So if we see !sem->state, we know that
> > > > > > > * the writer won't be checking until we're past the preempt_enable()
> > > > > > > * and that once the synchronize_rcu() is done, the writer will see
> > > > > > > * anything we did within this RCU-sched read-size critical section.
> > > > > > > */
> > > > > > >
> > > > > > > Also,
> > > > > > > I guess we could get rid of all of the gp_ops struct stuff now that since all
> > > > > > > the callbacks are the same now. I will post that as a follow-up patch to this
> > > > > > > series.
> > > > > >
> > > > > > Hello, Joel,
> > > > > >
> > > > > > Oleg has a set of patches updating this code that just hit mainline
> > > > > > this week. These patches get rid of the code that previously handled
> > > > > > RCU's multiple flavors. Or are you looking at current mainline and
> > > > > > me just missing your point?
> > > > > >
> > > > >
> > > > > Hi Paul,
> > > > > You are right on point. I have a bad habit of not rebasing my trees. In this
> > > > > case the feature branch of mine in concern was based on v5.1. Needless to
> > > > > say, I need to rebase my tree.
> > > > >
> > > > > Yes, this sync clean up patch does conflict when I rebase, but other patches
> > > > > rebase just fine.
> > > > >
> > > > > The 2 options I see are:
> > > > > 1. Let us drop this patch for now and I resend it later.
> > > > > 2. I resend all patches based on Linus's master branch.
> > > >
> > > > Below is the updated patch based on Linus master branch:
> > > >
> > > > ---8<-----------------------
> > > >
> > > > >From 5f40c9a07fcf3d6dafc2189599d0ba9443097d0f Mon Sep 17 00:00:00 2001
> > > > From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
> > > > Date: Fri, 12 Jul 2019 12:13:27 -0400
> > > > Subject: [PATCH v2.1 3/9] rcu/sync: Remove custom check for reader-section
> > > >
> > > > The rcu/sync code was doing its own check whether we are in a reader
> > > > section. With RCU consolidating flavors and the generic helper added in
> > > > this series, this is no longer need. We can just use the generic helper
> > > > and it results in a nice cleanup.
> > > >
> > > > Cc: Oleg Nesterov <oleg@redhat.com>
> > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > > > ---
> > > > include/linux/rcu_sync.h | 4 +---
> > > > 1 file changed, 1 insertion(+), 3 deletions(-)
> > > >
> > > > diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h
> > > > index 9b83865d24f9..0027d4c8087c 100644
> > > > --- a/include/linux/rcu_sync.h
> > > > +++ b/include/linux/rcu_sync.h
> > > > @@ -31,9 +31,7 @@ struct rcu_sync {
> > > > */
> > > > static inline bool rcu_sync_is_idle(struct rcu_sync *rsp)
> > > > {
> > > > - RCU_LOCKDEP_WARN(!rcu_read_lock_held() &&
> > > > - !rcu_read_lock_bh_held() &&
> > > > - !rcu_read_lock_sched_held(),
> > > > + RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),
> > >
> > > I believe that replacing rcu_read_lock_sched_held() with preemptible()
> > > in a CONFIG_PREEMPT=n kernel will give you false-positive splats here.
> > > If you have not already done so, could you please give it a try?
> >
> > Hi Paul,
> > I don't think it will cause splats for !CONFIG_PREEMPT.
> >
> > Currently, rcu_read_lock_any_held() introduced in this patch returns true if
> > !preemptible(). This means that:
> >
> > The following expression above:
> > RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),...)
> >
> > Becomes:
> > RCU_LOCKDEP_WARN(preemptible(), ...)
> >
> > For, CONFIG_PREEMPT=n kernels, this means:
> > RCU_LOCKDEP_WARN(0, ...)
> >
> > Which would mean no splats. Or, did I miss the point?
>
> I suggest trying it out on a CONFIG_PREEMPT=n kernel.
Sure, will do, sorry did not try it out yet because was busy with weekend
chores but will do soon, thanks!
^ permalink raw reply
* Re: [PATCH v2 3/9] rcu/sync: Remove custom check for reader-section
From: Paul E. McKenney @ 2019-07-13 15:50 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, Oleg Nesterov, 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, 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: <20190713153606.GD133650@google.com>
On Sat, Jul 13, 2019 at 11:36:06AM -0400, Joel Fernandes wrote:
> On Sat, Jul 13, 2019 at 07:41:08AM -0700, Paul E. McKenney wrote:
> > On Sat, Jul 13, 2019 at 09:30:49AM -0400, Joel Fernandes wrote:
> > > On Sat, Jul 13, 2019 at 01:21:14AM -0700, Paul E. McKenney wrote:
> > > > On Fri, Jul 12, 2019 at 11:10:08PM -0400, Joel Fernandes wrote:
> > > > > On Fri, Jul 12, 2019 at 11:01:50PM -0400, Joel Fernandes wrote:
> > > > > > On Fri, Jul 12, 2019 at 04:32:06PM -0700, Paul E. McKenney wrote:
> > > > > > > On Fri, Jul 12, 2019 at 05:35:59PM -0400, Joel Fernandes wrote:
> > > > > > > > On Fri, Jul 12, 2019 at 01:00:18PM -0400, Joel Fernandes (Google) wrote:
> > > > > > > > > The rcu/sync code was doing its own check whether we are in a reader
> > > > > > > > > section. With RCU consolidating flavors and the generic helper added in
> > > > > > > > > this series, this is no longer need. We can just use the generic helper
> > > > > > > > > and it results in a nice cleanup.
> > > > > > > > >
> > > > > > > > > Cc: Oleg Nesterov <oleg@redhat.com>
> > > > > > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > > > > > > >
> > > > > > > > Hi Oleg,
> > > > > > > > Slightly unrelated to the patch,
> > > > > > > > I tried hard to understand this comment below in percpu_down_read() but no dice.
> > > > > > > >
> > > > > > > > I do understand how rcu sync and percpu rwsem works, however the comment
> > > > > > > > below didn't make much sense to me. For one, there's no readers_fast anymore
> > > > > > > > so I did not follow what readers_fast means. Could the comment be updated to
> > > > > > > > reflect latest changes?
> > > > > > > > Also could you help understand how is a writer not able to change
> > > > > > > > sem->state and count the per-cpu read counters at the same time as the
> > > > > > > > comment tries to say?
> > > > > > > >
> > > > > > > > /*
> > > > > > > > * We are in an RCU-sched read-side critical section, so the writer
> > > > > > > > * cannot both change sem->state from readers_fast and start checking
> > > > > > > > * counters while we are here. So if we see !sem->state, we know that
> > > > > > > > * the writer won't be checking until we're past the preempt_enable()
> > > > > > > > * and that once the synchronize_rcu() is done, the writer will see
> > > > > > > > * anything we did within this RCU-sched read-size critical section.
> > > > > > > > */
> > > > > > > >
> > > > > > > > Also,
> > > > > > > > I guess we could get rid of all of the gp_ops struct stuff now that since all
> > > > > > > > the callbacks are the same now. I will post that as a follow-up patch to this
> > > > > > > > series.
> > > > > > >
> > > > > > > Hello, Joel,
> > > > > > >
> > > > > > > Oleg has a set of patches updating this code that just hit mainline
> > > > > > > this week. These patches get rid of the code that previously handled
> > > > > > > RCU's multiple flavors. Or are you looking at current mainline and
> > > > > > > me just missing your point?
> > > > > > >
> > > > > >
> > > > > > Hi Paul,
> > > > > > You are right on point. I have a bad habit of not rebasing my trees. In this
> > > > > > case the feature branch of mine in concern was based on v5.1. Needless to
> > > > > > say, I need to rebase my tree.
> > > > > >
> > > > > > Yes, this sync clean up patch does conflict when I rebase, but other patches
> > > > > > rebase just fine.
> > > > > >
> > > > > > The 2 options I see are:
> > > > > > 1. Let us drop this patch for now and I resend it later.
> > > > > > 2. I resend all patches based on Linus's master branch.
> > > > >
> > > > > Below is the updated patch based on Linus master branch:
> > > > >
> > > > > ---8<-----------------------
> > > > >
> > > > > >From 5f40c9a07fcf3d6dafc2189599d0ba9443097d0f Mon Sep 17 00:00:00 2001
> > > > > From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
> > > > > Date: Fri, 12 Jul 2019 12:13:27 -0400
> > > > > Subject: [PATCH v2.1 3/9] rcu/sync: Remove custom check for reader-section
> > > > >
> > > > > The rcu/sync code was doing its own check whether we are in a reader
> > > > > section. With RCU consolidating flavors and the generic helper added in
> > > > > this series, this is no longer need. We can just use the generic helper
> > > > > and it results in a nice cleanup.
> > > > >
> > > > > Cc: Oleg Nesterov <oleg@redhat.com>
> > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > > > > ---
> > > > > include/linux/rcu_sync.h | 4 +---
> > > > > 1 file changed, 1 insertion(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h
> > > > > index 9b83865d24f9..0027d4c8087c 100644
> > > > > --- a/include/linux/rcu_sync.h
> > > > > +++ b/include/linux/rcu_sync.h
> > > > > @@ -31,9 +31,7 @@ struct rcu_sync {
> > > > > */
> > > > > static inline bool rcu_sync_is_idle(struct rcu_sync *rsp)
> > > > > {
> > > > > - RCU_LOCKDEP_WARN(!rcu_read_lock_held() &&
> > > > > - !rcu_read_lock_bh_held() &&
> > > > > - !rcu_read_lock_sched_held(),
> > > > > + RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),
> > > >
> > > > I believe that replacing rcu_read_lock_sched_held() with preemptible()
> > > > in a CONFIG_PREEMPT=n kernel will give you false-positive splats here.
> > > > If you have not already done so, could you please give it a try?
> > >
> > > Hi Paul,
> > > I don't think it will cause splats for !CONFIG_PREEMPT.
> > >
> > > Currently, rcu_read_lock_any_held() introduced in this patch returns true if
> > > !preemptible(). This means that:
> > >
> > > The following expression above:
> > > RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),...)
> > >
> > > Becomes:
> > > RCU_LOCKDEP_WARN(preemptible(), ...)
> > >
> > > For, CONFIG_PREEMPT=n kernels, this means:
> > > RCU_LOCKDEP_WARN(0, ...)
> > >
> > > Which would mean no splats. Or, did I miss the point?
> >
> > I suggest trying it out on a CONFIG_PREEMPT=n kernel.
>
> Sure, will do, sorry did not try it out yet because was busy with weekend
> chores but will do soon, thanks!
I am not faulting you for taking the weekend off, actually. ;-)
Thanx, Paul
^ permalink raw reply
* Re: [PATCH v2 3/9] rcu/sync: Remove custom check for reader-section
From: Joel Fernandes @ 2019-07-13 16:13 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linux-kernel, Oleg Nesterov, 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, 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: <20190713155010.GF26519@linux.ibm.com>
On Sat, Jul 13, 2019 at 08:50:10AM -0700, Paul E. McKenney wrote:
> On Sat, Jul 13, 2019 at 11:36:06AM -0400, Joel Fernandes wrote:
> > On Sat, Jul 13, 2019 at 07:41:08AM -0700, Paul E. McKenney wrote:
> > > On Sat, Jul 13, 2019 at 09:30:49AM -0400, Joel Fernandes wrote:
> > > > On Sat, Jul 13, 2019 at 01:21:14AM -0700, Paul E. McKenney wrote:
> > > > > On Fri, Jul 12, 2019 at 11:10:08PM -0400, Joel Fernandes wrote:
> > > > > > On Fri, Jul 12, 2019 at 11:01:50PM -0400, Joel Fernandes wrote:
> > > > > > > On Fri, Jul 12, 2019 at 04:32:06PM -0700, Paul E. McKenney wrote:
> > > > > > > > On Fri, Jul 12, 2019 at 05:35:59PM -0400, Joel Fernandes wrote:
> > > > > > > > > On Fri, Jul 12, 2019 at 01:00:18PM -0400, Joel Fernandes (Google) wrote:
> > > > > > > > > > The rcu/sync code was doing its own check whether we are in a reader
> > > > > > > > > > section. With RCU consolidating flavors and the generic helper added in
> > > > > > > > > > this series, this is no longer need. We can just use the generic helper
> > > > > > > > > > and it results in a nice cleanup.
> > > > > > > > > >
> > > > > > > > > > Cc: Oleg Nesterov <oleg@redhat.com>
> > > > > > > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > > > > > > > >
> > > > > > > > > Hi Oleg,
> > > > > > > > > Slightly unrelated to the patch,
> > > > > > > > > I tried hard to understand this comment below in percpu_down_read() but no dice.
> > > > > > > > >
> > > > > > > > > I do understand how rcu sync and percpu rwsem works, however the comment
> > > > > > > > > below didn't make much sense to me. For one, there's no readers_fast anymore
> > > > > > > > > so I did not follow what readers_fast means. Could the comment be updated to
> > > > > > > > > reflect latest changes?
> > > > > > > > > Also could you help understand how is a writer not able to change
> > > > > > > > > sem->state and count the per-cpu read counters at the same time as the
> > > > > > > > > comment tries to say?
> > > > > > > > >
> > > > > > > > > /*
> > > > > > > > > * We are in an RCU-sched read-side critical section, so the writer
> > > > > > > > > * cannot both change sem->state from readers_fast and start checking
> > > > > > > > > * counters while we are here. So if we see !sem->state, we know that
> > > > > > > > > * the writer won't be checking until we're past the preempt_enable()
> > > > > > > > > * and that once the synchronize_rcu() is done, the writer will see
> > > > > > > > > * anything we did within this RCU-sched read-size critical section.
> > > > > > > > > */
> > > > > > > > >
> > > > > > > > > Also,
> > > > > > > > > I guess we could get rid of all of the gp_ops struct stuff now that since all
> > > > > > > > > the callbacks are the same now. I will post that as a follow-up patch to this
> > > > > > > > > series.
> > > > > > > >
> > > > > > > > Hello, Joel,
> > > > > > > >
> > > > > > > > Oleg has a set of patches updating this code that just hit mainline
> > > > > > > > this week. These patches get rid of the code that previously handled
> > > > > > > > RCU's multiple flavors. Or are you looking at current mainline and
> > > > > > > > me just missing your point?
> > > > > > > >
> > > > > > >
> > > > > > > Hi Paul,
> > > > > > > You are right on point. I have a bad habit of not rebasing my trees. In this
> > > > > > > case the feature branch of mine in concern was based on v5.1. Needless to
> > > > > > > say, I need to rebase my tree.
> > > > > > >
> > > > > > > Yes, this sync clean up patch does conflict when I rebase, but other patches
> > > > > > > rebase just fine.
> > > > > > >
> > > > > > > The 2 options I see are:
> > > > > > > 1. Let us drop this patch for now and I resend it later.
> > > > > > > 2. I resend all patches based on Linus's master branch.
> > > > > >
> > > > > > Below is the updated patch based on Linus master branch:
> > > > > >
> > > > > > ---8<-----------------------
> > > > > >
> > > > > > >From 5f40c9a07fcf3d6dafc2189599d0ba9443097d0f Mon Sep 17 00:00:00 2001
> > > > > > From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
> > > > > > Date: Fri, 12 Jul 2019 12:13:27 -0400
> > > > > > Subject: [PATCH v2.1 3/9] rcu/sync: Remove custom check for reader-section
> > > > > >
> > > > > > The rcu/sync code was doing its own check whether we are in a reader
> > > > > > section. With RCU consolidating flavors and the generic helper added in
> > > > > > this series, this is no longer need. We can just use the generic helper
> > > > > > and it results in a nice cleanup.
> > > > > >
> > > > > > Cc: Oleg Nesterov <oleg@redhat.com>
> > > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > > > > > ---
> > > > > > include/linux/rcu_sync.h | 4 +---
> > > > > > 1 file changed, 1 insertion(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h
> > > > > > index 9b83865d24f9..0027d4c8087c 100644
> > > > > > --- a/include/linux/rcu_sync.h
> > > > > > +++ b/include/linux/rcu_sync.h
> > > > > > @@ -31,9 +31,7 @@ struct rcu_sync {
> > > > > > */
> > > > > > static inline bool rcu_sync_is_idle(struct rcu_sync *rsp)
> > > > > > {
> > > > > > - RCU_LOCKDEP_WARN(!rcu_read_lock_held() &&
> > > > > > - !rcu_read_lock_bh_held() &&
> > > > > > - !rcu_read_lock_sched_held(),
> > > > > > + RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),
> > > > >
> > > > > I believe that replacing rcu_read_lock_sched_held() with preemptible()
> > > > > in a CONFIG_PREEMPT=n kernel will give you false-positive splats here.
> > > > > If you have not already done so, could you please give it a try?
> > > >
> > > > Hi Paul,
> > > > I don't think it will cause splats for !CONFIG_PREEMPT.
> > > >
> > > > Currently, rcu_read_lock_any_held() introduced in this patch returns true if
> > > > !preemptible(). This means that:
> > > >
> > > > The following expression above:
> > > > RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),...)
> > > >
> > > > Becomes:
> > > > RCU_LOCKDEP_WARN(preemptible(), ...)
> > > >
> > > > For, CONFIG_PREEMPT=n kernels, this means:
> > > > RCU_LOCKDEP_WARN(0, ...)
> > > >
> > > > Which would mean no splats. Or, did I miss the point?
> > >
> > > I suggest trying it out on a CONFIG_PREEMPT=n kernel.
> >
> > Sure, will do, sorry did not try it out yet because was busy with weekend
> > chores but will do soon, thanks!
>
> I am not faulting you for taking the weekend off, actually. ;-)
;-)
I tried doing RCU_LOCKDEP_WARN(preemptible(), ...) in this code path and I
don't get any splats. I also disassembled the code and it seems to me
RCU_LOCKDEP_WARN() becomes a NOOP which also the above reasoning confirms.
thanks,
- Joel
^ permalink raw reply
* [PATCH] power: reset: reboot-mode: Fix author email format
From: Matwey V. Kornilov @ 2019-07-13 15:42 UTC (permalink / raw)
To: Sebastian Reichel
Cc: matwey.kornilov, Matwey V. Kornilov,
open list:SYSTEM RESET/SHUTDOWN DRIVERS, open list
Closing angle bracket was missing.
Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
---
drivers/power/reset/reboot-mode.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/power/reset/reboot-mode.c b/drivers/power/reset/reboot-mode.c
index 06ff035b57f5..b4076b10b893 100644
--- a/drivers/power/reset/reboot-mode.c
+++ b/drivers/power/reset/reboot-mode.c
@@ -190,6 +190,6 @@ void devm_reboot_mode_unregister(struct device *dev,
}
EXPORT_SYMBOL_GPL(devm_reboot_mode_unregister);
-MODULE_AUTHOR("Andy Yan <andy.yan@rock-chips.com");
+MODULE_AUTHOR("Andy Yan <andy.yan@rock-chips.com>");
MODULE_DESCRIPTION("System reboot mode core library");
MODULE_LICENSE("GPL v2");
--
2.16.4
^ permalink raw reply related
* Re: [PATCH v4 00/11] add thermal driver for h6
From: Vasily Khoruzhick @ 2019-07-13 18:01 UTC (permalink / raw)
To: Yangtao Li
Cc: rui.zhang, Eduardo Valentin, Daniel Lezcano, Rob Herring,
Mark Rutland, Maxime Ripard, Chen-Yu Tsai, David S. Miller,
Greg Kroah-Hartman, Mauro Carvalho Chehab, Linus Walleij,
Nicolas Ferre, paulmck, devicetree, linux-kernel, arm-linux,
Linux PM
In-Reply-To: <CA+E=qVfhDEQER2UTj65hR9erzej9Ey2FrUa9GV=iCFYsWZ2ztw@mail.gmail.com>
On Wed, Jul 10, 2019 at 4:09 PM Vasily Khoruzhick <anarsoul@gmail.com> wrote:
>
> On Sun, Jun 23, 2019 at 9:42 AM Yangtao Li <tiny.windzz@gmail.com> wrote:
> >
> > This patchset add support for H3 and H6 thermal sensor.
> >
> > BTY, do a cleanup in thermal makfile.
> >
> > Yangtao Li (11):
> > thermal: sun8i: add thermal driver for h6
> > dt-bindings: thermal: add binding document for h6 thermal controller
> > thermal: fix indentation in makefile
> > thermal: sun8i: get ths sensor number from device compatible
> > thermal: sun8i: rework for sun8i_ths_get_temp()
> > thermal: sun8i: get ths init func from device compatible
> > thermal: sun8i: rework for ths irq handler func
> > thermal: sun8i: support ahb clocks
> > thermal: sun8i: rework for ths calibrate func
> > dt-bindings: thermal: add binding document for h3 thermal controller
> > thermal: sun8i: add thermal driver for h3
>
> It would be nice to add dts changes to this series. It's unlikely that
> you'll get any "Tested-by" otherwise.
I added A64 support on top of this series, see
https://github.com/anarsoul/linux-2.6/tree/v5.2-thermal
Branch also contains patches to enable DVFS on A64, feel free to
cherry pick only those related to thermal driver if you want to
include A64 support into v5 series.
>
> > .../bindings/thermal/sun8i-thermal.yaml | 94 +++
> > MAINTAINERS | 7 +
> > drivers/thermal/Kconfig | 14 +
> > drivers/thermal/Makefile | 9 +-
> > drivers/thermal/sun8i_thermal.c | 534 ++++++++++++++++++
> > 5 files changed, 654 insertions(+), 4 deletions(-)
> > create mode 100644 Documentation/devicetree/bindings/thermal/sun8i-thermal.yaml
> > create mode 100644 drivers/thermal/sun8i_thermal.c
> >
> > ---
> > v4:
> > -add h3 support
> > -fix yaml file
> > ---
> > 2.17.1
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 3/9] rcu/sync: Remove custom check for reader-section
From: Paul E. McKenney @ 2019-07-13 21:28 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, Oleg Nesterov, 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, 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: <20190713161316.GA39321@google.com>
On Sat, Jul 13, 2019 at 12:13:16PM -0400, Joel Fernandes wrote:
> On Sat, Jul 13, 2019 at 08:50:10AM -0700, Paul E. McKenney wrote:
> > On Sat, Jul 13, 2019 at 11:36:06AM -0400, Joel Fernandes wrote:
> > > On Sat, Jul 13, 2019 at 07:41:08AM -0700, Paul E. McKenney wrote:
> > > > On Sat, Jul 13, 2019 at 09:30:49AM -0400, Joel Fernandes wrote:
> > > > > On Sat, Jul 13, 2019 at 01:21:14AM -0700, Paul E. McKenney wrote:
> > > > > > On Fri, Jul 12, 2019 at 11:10:08PM -0400, Joel Fernandes wrote:
> > > > > > > On Fri, Jul 12, 2019 at 11:01:50PM -0400, Joel Fernandes wrote:
> > > > > > > > On Fri, Jul 12, 2019 at 04:32:06PM -0700, Paul E. McKenney wrote:
> > > > > > > > > On Fri, Jul 12, 2019 at 05:35:59PM -0400, Joel Fernandes wrote:
> > > > > > > > > > On Fri, Jul 12, 2019 at 01:00:18PM -0400, Joel Fernandes (Google) wrote:
> > > > > > > > > > > The rcu/sync code was doing its own check whether we are in a reader
> > > > > > > > > > > section. With RCU consolidating flavors and the generic helper added in
> > > > > > > > > > > this series, this is no longer need. We can just use the generic helper
> > > > > > > > > > > and it results in a nice cleanup.
> > > > > > > > > > >
> > > > > > > > > > > Cc: Oleg Nesterov <oleg@redhat.com>
> > > > > > > > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > > > > > > > > >
> > > > > > > > > > Hi Oleg,
> > > > > > > > > > Slightly unrelated to the patch,
> > > > > > > > > > I tried hard to understand this comment below in percpu_down_read() but no dice.
> > > > > > > > > >
> > > > > > > > > > I do understand how rcu sync and percpu rwsem works, however the comment
> > > > > > > > > > below didn't make much sense to me. For one, there's no readers_fast anymore
> > > > > > > > > > so I did not follow what readers_fast means. Could the comment be updated to
> > > > > > > > > > reflect latest changes?
> > > > > > > > > > Also could you help understand how is a writer not able to change
> > > > > > > > > > sem->state and count the per-cpu read counters at the same time as the
> > > > > > > > > > comment tries to say?
> > > > > > > > > >
> > > > > > > > > > /*
> > > > > > > > > > * We are in an RCU-sched read-side critical section, so the writer
> > > > > > > > > > * cannot both change sem->state from readers_fast and start checking
> > > > > > > > > > * counters while we are here. So if we see !sem->state, we know that
> > > > > > > > > > * the writer won't be checking until we're past the preempt_enable()
> > > > > > > > > > * and that once the synchronize_rcu() is done, the writer will see
> > > > > > > > > > * anything we did within this RCU-sched read-size critical section.
> > > > > > > > > > */
> > > > > > > > > >
> > > > > > > > > > Also,
> > > > > > > > > > I guess we could get rid of all of the gp_ops struct stuff now that since all
> > > > > > > > > > the callbacks are the same now. I will post that as a follow-up patch to this
> > > > > > > > > > series.
> > > > > > > > >
> > > > > > > > > Hello, Joel,
> > > > > > > > >
> > > > > > > > > Oleg has a set of patches updating this code that just hit mainline
> > > > > > > > > this week. These patches get rid of the code that previously handled
> > > > > > > > > RCU's multiple flavors. Or are you looking at current mainline and
> > > > > > > > > me just missing your point?
> > > > > > > > >
> > > > > > > >
> > > > > > > > Hi Paul,
> > > > > > > > You are right on point. I have a bad habit of not rebasing my trees. In this
> > > > > > > > case the feature branch of mine in concern was based on v5.1. Needless to
> > > > > > > > say, I need to rebase my tree.
> > > > > > > >
> > > > > > > > Yes, this sync clean up patch does conflict when I rebase, but other patches
> > > > > > > > rebase just fine.
> > > > > > > >
> > > > > > > > The 2 options I see are:
> > > > > > > > 1. Let us drop this patch for now and I resend it later.
> > > > > > > > 2. I resend all patches based on Linus's master branch.
> > > > > > >
> > > > > > > Below is the updated patch based on Linus master branch:
> > > > > > >
> > > > > > > ---8<-----------------------
> > > > > > >
> > > > > > > >From 5f40c9a07fcf3d6dafc2189599d0ba9443097d0f Mon Sep 17 00:00:00 2001
> > > > > > > From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
> > > > > > > Date: Fri, 12 Jul 2019 12:13:27 -0400
> > > > > > > Subject: [PATCH v2.1 3/9] rcu/sync: Remove custom check for reader-section
> > > > > > >
> > > > > > > The rcu/sync code was doing its own check whether we are in a reader
> > > > > > > section. With RCU consolidating flavors and the generic helper added in
> > > > > > > this series, this is no longer need. We can just use the generic helper
> > > > > > > and it results in a nice cleanup.
> > > > > > >
> > > > > > > Cc: Oleg Nesterov <oleg@redhat.com>
> > > > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > > > > > > ---
> > > > > > > include/linux/rcu_sync.h | 4 +---
> > > > > > > 1 file changed, 1 insertion(+), 3 deletions(-)
> > > > > > >
> > > > > > > diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h
> > > > > > > index 9b83865d24f9..0027d4c8087c 100644
> > > > > > > --- a/include/linux/rcu_sync.h
> > > > > > > +++ b/include/linux/rcu_sync.h
> > > > > > > @@ -31,9 +31,7 @@ struct rcu_sync {
> > > > > > > */
> > > > > > > static inline bool rcu_sync_is_idle(struct rcu_sync *rsp)
> > > > > > > {
> > > > > > > - RCU_LOCKDEP_WARN(!rcu_read_lock_held() &&
> > > > > > > - !rcu_read_lock_bh_held() &&
> > > > > > > - !rcu_read_lock_sched_held(),
> > > > > > > + RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),
> > > > > >
> > > > > > I believe that replacing rcu_read_lock_sched_held() with preemptible()
> > > > > > in a CONFIG_PREEMPT=n kernel will give you false-positive splats here.
> > > > > > If you have not already done so, could you please give it a try?
> > > > >
> > > > > Hi Paul,
> > > > > I don't think it will cause splats for !CONFIG_PREEMPT.
> > > > >
> > > > > Currently, rcu_read_lock_any_held() introduced in this patch returns true if
> > > > > !preemptible(). This means that:
> > > > >
> > > > > The following expression above:
> > > > > RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),...)
> > > > >
> > > > > Becomes:
> > > > > RCU_LOCKDEP_WARN(preemptible(), ...)
> > > > >
> > > > > For, CONFIG_PREEMPT=n kernels, this means:
> > > > > RCU_LOCKDEP_WARN(0, ...)
> > > > >
> > > > > Which would mean no splats. Or, did I miss the point?
> > > >
> > > > I suggest trying it out on a CONFIG_PREEMPT=n kernel.
> > >
> > > Sure, will do, sorry did not try it out yet because was busy with weekend
> > > chores but will do soon, thanks!
> >
> > I am not faulting you for taking the weekend off, actually. ;-)
>
> ;-)
>
> I tried doing RCU_LOCKDEP_WARN(preemptible(), ...) in this code path and I
> don't get any splats. I also disassembled the code and it seems to me
> RCU_LOCKDEP_WARN() becomes a NOOP which also the above reasoning confirms.
OK, very good. Could you do the same thing for the RCU_LOCKDEP_WARN()
in synchronize_rcu()? Why or why not?
(No need to work this on your Sunday.)
Thanx, Paul
^ permalink raw reply
* Re: [PATCH v2 3/9] rcu/sync: Remove custom check for reader-section
From: Joel Fernandes @ 2019-07-14 18:10 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linux-kernel, Oleg Nesterov, 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, 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: <20190713212812.GH26519@linux.ibm.com>
On Sat, Jul 13, 2019 at 02:28:12PM -0700, Paul E. McKenney wrote:
> On Sat, Jul 13, 2019 at 12:13:16PM -0400, Joel Fernandes wrote:
> > On Sat, Jul 13, 2019 at 08:50:10AM -0700, Paul E. McKenney wrote:
> > > On Sat, Jul 13, 2019 at 11:36:06AM -0400, Joel Fernandes wrote:
> > > > On Sat, Jul 13, 2019 at 07:41:08AM -0700, Paul E. McKenney wrote:
> > > > > On Sat, Jul 13, 2019 at 09:30:49AM -0400, Joel Fernandes wrote:
> > > > > > On Sat, Jul 13, 2019 at 01:21:14AM -0700, Paul E. McKenney wrote:
> > > > > > > On Fri, Jul 12, 2019 at 11:10:08PM -0400, Joel Fernandes wrote:
> > > > > > > > On Fri, Jul 12, 2019 at 11:01:50PM -0400, Joel Fernandes wrote:
> > > > > > > > > On Fri, Jul 12, 2019 at 04:32:06PM -0700, Paul E. McKenney wrote:
> > > > > > > > > > On Fri, Jul 12, 2019 at 05:35:59PM -0400, Joel Fernandes wrote:
> > > > > > > > > > > On Fri, Jul 12, 2019 at 01:00:18PM -0400, Joel Fernandes (Google) wrote:
> > > > > > > > > > > > The rcu/sync code was doing its own check whether we are in a reader
> > > > > > > > > > > > section. With RCU consolidating flavors and the generic helper added in
> > > > > > > > > > > > this series, this is no longer need. We can just use the generic helper
> > > > > > > > > > > > and it results in a nice cleanup.
> > > > > > > > > > > >
> > > > > > > > > > > > Cc: Oleg Nesterov <oleg@redhat.com>
> > > > > > > > > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > > > > > > > > > >
> > > > > > > > > > > Hi Oleg,
> > > > > > > > > > > Slightly unrelated to the patch,
> > > > > > > > > > > I tried hard to understand this comment below in percpu_down_read() but no dice.
> > > > > > > > > > >
> > > > > > > > > > > I do understand how rcu sync and percpu rwsem works, however the comment
> > > > > > > > > > > below didn't make much sense to me. For one, there's no readers_fast anymore
> > > > > > > > > > > so I did not follow what readers_fast means. Could the comment be updated to
> > > > > > > > > > > reflect latest changes?
> > > > > > > > > > > Also could you help understand how is a writer not able to change
> > > > > > > > > > > sem->state and count the per-cpu read counters at the same time as the
> > > > > > > > > > > comment tries to say?
> > > > > > > > > > >
> > > > > > > > > > > /*
> > > > > > > > > > > * We are in an RCU-sched read-side critical section, so the writer
> > > > > > > > > > > * cannot both change sem->state from readers_fast and start checking
> > > > > > > > > > > * counters while we are here. So if we see !sem->state, we know that
> > > > > > > > > > > * the writer won't be checking until we're past the preempt_enable()
> > > > > > > > > > > * and that once the synchronize_rcu() is done, the writer will see
> > > > > > > > > > > * anything we did within this RCU-sched read-size critical section.
> > > > > > > > > > > */
> > > > > > > > > > >
> > > > > > > > > > > Also,
> > > > > > > > > > > I guess we could get rid of all of the gp_ops struct stuff now that since all
> > > > > > > > > > > the callbacks are the same now. I will post that as a follow-up patch to this
> > > > > > > > > > > series.
> > > > > > > > > >
> > > > > > > > > > Hello, Joel,
> > > > > > > > > >
> > > > > > > > > > Oleg has a set of patches updating this code that just hit mainline
> > > > > > > > > > this week. These patches get rid of the code that previously handled
> > > > > > > > > > RCU's multiple flavors. Or are you looking at current mainline and
> > > > > > > > > > me just missing your point?
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Hi Paul,
> > > > > > > > > You are right on point. I have a bad habit of not rebasing my trees. In this
> > > > > > > > > case the feature branch of mine in concern was based on v5.1. Needless to
> > > > > > > > > say, I need to rebase my tree.
> > > > > > > > >
> > > > > > > > > Yes, this sync clean up patch does conflict when I rebase, but other patches
> > > > > > > > > rebase just fine.
> > > > > > > > >
> > > > > > > > > The 2 options I see are:
> > > > > > > > > 1. Let us drop this patch for now and I resend it later.
> > > > > > > > > 2. I resend all patches based on Linus's master branch.
> > > > > > > >
> > > > > > > > Below is the updated patch based on Linus master branch:
> > > > > > > >
> > > > > > > > ---8<-----------------------
> > > > > > > >
> > > > > > > > >From 5f40c9a07fcf3d6dafc2189599d0ba9443097d0f Mon Sep 17 00:00:00 2001
> > > > > > > > From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
> > > > > > > > Date: Fri, 12 Jul 2019 12:13:27 -0400
> > > > > > > > Subject: [PATCH v2.1 3/9] rcu/sync: Remove custom check for reader-section
> > > > > > > >
> > > > > > > > The rcu/sync code was doing its own check whether we are in a reader
> > > > > > > > section. With RCU consolidating flavors and the generic helper added in
> > > > > > > > this series, this is no longer need. We can just use the generic helper
> > > > > > > > and it results in a nice cleanup.
> > > > > > > >
> > > > > > > > Cc: Oleg Nesterov <oleg@redhat.com>
> > > > > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > > > > > > > ---
> > > > > > > > include/linux/rcu_sync.h | 4 +---
> > > > > > > > 1 file changed, 1 insertion(+), 3 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h
> > > > > > > > index 9b83865d24f9..0027d4c8087c 100644
> > > > > > > > --- a/include/linux/rcu_sync.h
> > > > > > > > +++ b/include/linux/rcu_sync.h
> > > > > > > > @@ -31,9 +31,7 @@ struct rcu_sync {
> > > > > > > > */
> > > > > > > > static inline bool rcu_sync_is_idle(struct rcu_sync *rsp)
> > > > > > > > {
> > > > > > > > - RCU_LOCKDEP_WARN(!rcu_read_lock_held() &&
> > > > > > > > - !rcu_read_lock_bh_held() &&
> > > > > > > > - !rcu_read_lock_sched_held(),
> > > > > > > > + RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),
> > > > > > >
> > > > > > > I believe that replacing rcu_read_lock_sched_held() with preemptible()
> > > > > > > in a CONFIG_PREEMPT=n kernel will give you false-positive splats here.
> > > > > > > If you have not already done so, could you please give it a try?
> > > > > >
> > > > > > Hi Paul,
> > > > > > I don't think it will cause splats for !CONFIG_PREEMPT.
> > > > > >
> > > > > > Currently, rcu_read_lock_any_held() introduced in this patch returns true if
> > > > > > !preemptible(). This means that:
> > > > > >
> > > > > > The following expression above:
> > > > > > RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),...)
> > > > > >
> > > > > > Becomes:
> > > > > > RCU_LOCKDEP_WARN(preemptible(), ...)
> > > > > >
> > > > > > For, CONFIG_PREEMPT=n kernels, this means:
> > > > > > RCU_LOCKDEP_WARN(0, ...)
> > > > > >
> > > > > > Which would mean no splats. Or, did I miss the point?
> > > > >
> > > > > I suggest trying it out on a CONFIG_PREEMPT=n kernel.
> > > >
> > > > Sure, will do, sorry did not try it out yet because was busy with weekend
> > > > chores but will do soon, thanks!
> > >
> > > I am not faulting you for taking the weekend off, actually. ;-)
> >
> > ;-)
> >
> > I tried doing RCU_LOCKDEP_WARN(preemptible(), ...) in this code path and I
> > don't get any splats. I also disassembled the code and it seems to me
> > RCU_LOCKDEP_WARN() becomes a NOOP which also the above reasoning confirms.
>
> OK, very good. Could you do the same thing for the RCU_LOCKDEP_WARN()
> in synchronize_rcu()? Why or why not?
>
Hi Paul,
Yes synchronize_rcu() can also make use of this technique since it is
strictly illegal to call synchronize_rcu() within a reader section.
I will add this to the set of my patches as well and send them all out next
week, along with the rcu-sync and bh clean ups we discussed.
thanks,
- Joel
^ permalink raw reply
* Re: [PATCH v2 3/9] rcu/sync: Remove custom check for reader-section
From: Joel Fernandes @ 2019-07-14 18:38 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linux-kernel, Oleg Nesterov, 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, 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: <20190714181053.GB34501@google.com>
On Sun, Jul 14, 2019 at 02:10:53PM -0400, Joel Fernandes wrote:
> On Sat, Jul 13, 2019 at 02:28:12PM -0700, Paul E. McKenney wrote:
[snip]
> > > > > > > > >
> > > > > > > > > Cc: Oleg Nesterov <oleg@redhat.com>
> > > > > > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > > > > > > > > ---
> > > > > > > > > include/linux/rcu_sync.h | 4 +---
> > > > > > > > > 1 file changed, 1 insertion(+), 3 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h
> > > > > > > > > index 9b83865d24f9..0027d4c8087c 100644
> > > > > > > > > --- a/include/linux/rcu_sync.h
> > > > > > > > > +++ b/include/linux/rcu_sync.h
> > > > > > > > > @@ -31,9 +31,7 @@ struct rcu_sync {
> > > > > > > > > */
> > > > > > > > > static inline bool rcu_sync_is_idle(struct rcu_sync *rsp)
> > > > > > > > > {
> > > > > > > > > - RCU_LOCKDEP_WARN(!rcu_read_lock_held() &&
> > > > > > > > > - !rcu_read_lock_bh_held() &&
> > > > > > > > > - !rcu_read_lock_sched_held(),
> > > > > > > > > + RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),
> > > > > > > >
> > > > > > > > I believe that replacing rcu_read_lock_sched_held() with preemptible()
> > > > > > > > in a CONFIG_PREEMPT=n kernel will give you false-positive splats here.
> > > > > > > > If you have not already done so, could you please give it a try?
> > > > > > >
> > > > > > > Hi Paul,
> > > > > > > I don't think it will cause splats for !CONFIG_PREEMPT.
> > > > > > >
> > > > > > > Currently, rcu_read_lock_any_held() introduced in this patch returns true if
> > > > > > > !preemptible(). This means that:
> > > > > > >
> > > > > > > The following expression above:
> > > > > > > RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),...)
> > > > > > >
> > > > > > > Becomes:
> > > > > > > RCU_LOCKDEP_WARN(preemptible(), ...)
> > > > > > >
> > > > > > > For, CONFIG_PREEMPT=n kernels, this means:
> > > > > > > RCU_LOCKDEP_WARN(0, ...)
> > > > > > >
> > > > > > > Which would mean no splats. Or, did I miss the point?
> > > > > >
> > > > > > I suggest trying it out on a CONFIG_PREEMPT=n kernel.
> > > > >
> > > > > Sure, will do, sorry did not try it out yet because was busy with weekend
> > > > > chores but will do soon, thanks!
> > > >
> > > > I am not faulting you for taking the weekend off, actually. ;-)
> > >
> > > ;-)
> > >
> > > I tried doing RCU_LOCKDEP_WARN(preemptible(), ...) in this code path and I
> > > don't get any splats. I also disassembled the code and it seems to me
> > > RCU_LOCKDEP_WARN() becomes a NOOP which also the above reasoning confirms.
> >
> > OK, very good. Could you do the same thing for the RCU_LOCKDEP_WARN()
> > in synchronize_rcu()? Why or why not?
> >
>
> Hi Paul,
>
> Yes synchronize_rcu() can also make use of this technique since it is
> strictly illegal to call synchronize_rcu() within a reader section.
>
> I will add this to the set of my patches as well and send them all out next
> week, along with the rcu-sync and bh clean ups we discussed.
After sending this email, it occurs to me it wont work in synchronize_rcu()
for !CONFIG_PREEMPT kernels. This is because in a !CONFIG_PREEMPT kernel,
executing in kernel mode itself looks like being in an RCU reader. So we
should leave that as is. However it will work fine for rcu_sync_is_idle (for
CONFIG_PREEMPT=n kernels) as I mentioned earlier.
Were trying to throw me a Quick-Quiz ? ;-) In that case, hope I passed!
thanks,
- Joel
^ permalink raw reply
* Re: [PATCH v2 3/9] rcu/sync: Remove custom check for reader-section
From: Paul E. McKenney @ 2019-07-14 18:50 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, Oleg Nesterov, 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, 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: <20190714183820.GD34501@google.com>
On Sun, Jul 14, 2019 at 02:38:20PM -0400, Joel Fernandes wrote:
> On Sun, Jul 14, 2019 at 02:10:53PM -0400, Joel Fernandes wrote:
> > On Sat, Jul 13, 2019 at 02:28:12PM -0700, Paul E. McKenney wrote:
> [snip]
> > > > > > > > > >
> > > > > > > > > > Cc: Oleg Nesterov <oleg@redhat.com>
> > > > > > > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > > > > > > > > > ---
> > > > > > > > > > include/linux/rcu_sync.h | 4 +---
> > > > > > > > > > 1 file changed, 1 insertion(+), 3 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h
> > > > > > > > > > index 9b83865d24f9..0027d4c8087c 100644
> > > > > > > > > > --- a/include/linux/rcu_sync.h
> > > > > > > > > > +++ b/include/linux/rcu_sync.h
> > > > > > > > > > @@ -31,9 +31,7 @@ struct rcu_sync {
> > > > > > > > > > */
> > > > > > > > > > static inline bool rcu_sync_is_idle(struct rcu_sync *rsp)
> > > > > > > > > > {
> > > > > > > > > > - RCU_LOCKDEP_WARN(!rcu_read_lock_held() &&
> > > > > > > > > > - !rcu_read_lock_bh_held() &&
> > > > > > > > > > - !rcu_read_lock_sched_held(),
> > > > > > > > > > + RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),
> > > > > > > > >
> > > > > > > > > I believe that replacing rcu_read_lock_sched_held() with preemptible()
> > > > > > > > > in a CONFIG_PREEMPT=n kernel will give you false-positive splats here.
> > > > > > > > > If you have not already done so, could you please give it a try?
> > > > > > > >
> > > > > > > > Hi Paul,
> > > > > > > > I don't think it will cause splats for !CONFIG_PREEMPT.
> > > > > > > >
> > > > > > > > Currently, rcu_read_lock_any_held() introduced in this patch returns true if
> > > > > > > > !preemptible(). This means that:
> > > > > > > >
> > > > > > > > The following expression above:
> > > > > > > > RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),...)
> > > > > > > >
> > > > > > > > Becomes:
> > > > > > > > RCU_LOCKDEP_WARN(preemptible(), ...)
> > > > > > > >
> > > > > > > > For, CONFIG_PREEMPT=n kernels, this means:
> > > > > > > > RCU_LOCKDEP_WARN(0, ...)
> > > > > > > >
> > > > > > > > Which would mean no splats. Or, did I miss the point?
> > > > > > >
> > > > > > > I suggest trying it out on a CONFIG_PREEMPT=n kernel.
> > > > > >
> > > > > > Sure, will do, sorry did not try it out yet because was busy with weekend
> > > > > > chores but will do soon, thanks!
> > > > >
> > > > > I am not faulting you for taking the weekend off, actually. ;-)
> > > >
> > > > ;-)
> > > >
> > > > I tried doing RCU_LOCKDEP_WARN(preemptible(), ...) in this code path and I
> > > > don't get any splats. I also disassembled the code and it seems to me
> > > > RCU_LOCKDEP_WARN() becomes a NOOP which also the above reasoning confirms.
> > >
> > > OK, very good. Could you do the same thing for the RCU_LOCKDEP_WARN()
> > > in synchronize_rcu()? Why or why not?
> > >
> >
> > Hi Paul,
> >
> > Yes synchronize_rcu() can also make use of this technique since it is
> > strictly illegal to call synchronize_rcu() within a reader section.
> >
> > I will add this to the set of my patches as well and send them all out next
> > week, along with the rcu-sync and bh clean ups we discussed.
>
> After sending this email, it occurs to me it wont work in synchronize_rcu()
> for !CONFIG_PREEMPT kernels. This is because in a !CONFIG_PREEMPT kernel,
> executing in kernel mode itself looks like being in an RCU reader. So we
> should leave that as is. However it will work fine for rcu_sync_is_idle (for
> CONFIG_PREEMPT=n kernels) as I mentioned earlier.
>
> Were trying to throw me a Quick-Quiz ? ;-) In that case, hope I passed!
You did pass. This time. ;-)
Thanx, Paul
^ permalink raw reply
* Re: [PATCH v2 1/6] ARM: tegra: Remove cpuidle drivers
From: Dmitry Osipenko @ 2019-07-14 22:19 UTC (permalink / raw)
To: Jon Hunter
Cc: Thierry Reding, Peter De Schrijver, Rafael J. Wysocki,
Daniel Lezcano, linux-pm, linux-tegra, linux-arm-kernel,
linux-kernel
In-Reply-To: <9d79ed3e-a37f-af9c-0696-31dc33bbdefd@gmail.com>
12.07.2019 19:23, Dmitry Osipenko пишет:
> 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.
I tried (in several attempts) and couldn't find a way how to
meaningfully squash the old drivers. It's a lot of unnecessary churning
that won't help with reviewing of the code at all because new driver is
structured differently and beating of old drivers until they resemble
the new one just not worth it. I'll write a bit more detailed commit
message, emphasizing the details, hope it will be helpful. The old
drivers are ~200 LOC each and the new one will be ~300 LOC + 50 lines of
comments, I don't think that it's really very hard to review as it it
may initially looked like to you, Jon.
^ permalink raw reply
* Re: [PATCH v2 3/9] rcu/sync: Remove custom check for reader-section
From: Oleg Nesterov @ 2019-07-15 7:26 UTC (permalink / raw)
To: Joel Fernandes
Cc: Paul E. McKenney, 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, 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: <20190713031008.GA248225@google.com>
On 07/12, Joel Fernandes wrote:
>
> static inline bool rcu_sync_is_idle(struct rcu_sync *rsp)
> {
> - RCU_LOCKDEP_WARN(!rcu_read_lock_held() &&
> - !rcu_read_lock_bh_held() &&
> - !rcu_read_lock_sched_held(),
> + RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),
Yes, this is what I meant.
Sorry for confusion, I should have mentioned that rcu_sync_is_idle()
was recently updated when I suggested to use the new helper.
Acked-by: Oleg Nesterov <oleg@redhat.com>
^ permalink raw reply
* Re: [PATCH v3 6/6] interconnect: Add OPP table support for interconnects
From: Vincent Guittot @ 2019-07-15 8:16 UTC (permalink / raw)
To: Saravana Kannan
Cc: Georgi Djakov, Rob Herring, Mark Rutland, Viresh Kumar,
Nishanth Menon, Stephen Boyd, Rafael J. Wysocki, Sweeney, Sean,
daidavid1, Rajendra Nayak, sibis, Bjorn Andersson, Evan Green,
Android Kernel Team, open list:THERMAL,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel
In-Reply-To: <CAGETcx90WC2o+ZmkyhOPp1xJbfSk1wpAv2RA-4VgnhJfcsmJiA@mail.gmail.com>
On Tue, 9 Jul 2019 at 21:03, Saravana Kannan <saravanak@google.com> wrote:
>
> On Tue, Jul 9, 2019 at 12:25 AM Vincent Guittot
> <vincent.guittot@linaro.org> wrote:
> >
> > On Sun, 7 Jul 2019 at 23:48, Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > On Thu, Jul 4, 2019 at 12:12 AM Vincent Guittot
> > > <vincent.guittot@linaro.org> wrote:
> > > >
> > > > On Wed, 3 Jul 2019 at 23:33, Saravana Kannan <saravanak@google.com> wrote:
> > > > >
> > > > > On Tue, Jul 2, 2019 at 11:45 PM Vincent Guittot
> > > > > <vincent.guittot@linaro.org> wrote:
> > > > > >
> > > > > > On Wed, 3 Jul 2019 at 03:10, Saravana Kannan <saravanak@google.com> wrote:
> > > > > > >
> > > > > > > Interconnect paths can have different performance points. Now that OPP
> > > > > > > framework supports bandwidth OPP tables, add OPP table support for
> > > > > > > interconnects.
> > > > > > >
> > > > > > > Devices can use the interconnect-opp-table DT property to specify OPP
> > > > > > > tables for interconnect paths. And the driver can obtain the OPP table for
> > > > > > > an interconnect path by calling icc_get_opp_table().
> > > > > >
> > > > > > The opp table of a path must come from the aggregation of OPP tables
> > > > > > of the interconnect providers.
> > > > >
> > > > > The aggregation of OPP tables of the providers is certainly the
> > > > > superset of what a path can achieve, but to say that OPPs for
> > > > > interconnect path should match that superset is an oversimplification
> > > > > of the reality in hardware.
> > > > >
> > > > > There are lots of reasons an interconnect path might not want to use
> > > > > all the available bandwidth options across all the interconnects in
> > > > > the route.
> > > > >
> > > > > 1. That particular path might not have been validated or verified
> > > > > during the HW design process for some of the frequencies/bandwidth
> > > > > combinations of the providers.
> > > >
> > > > All these constraint are provider's constraints and not consumer's one
> > > >
> > > > The consumer asks for a bandwidth according to its needs and then the
> > > > providers select the optimal bandwidth of each interconnect after
> > > > aggregating all the request and according to what OPP have been
> > > > validated
> > >
> > > Not really. The screening can be a consumer specific issue. The
> > > consumer IP itself might have some issue with using too low of a
> > > bandwidth or bandwidth that's not within some range. It should not be
> >
> > How can an IP ask for not enough bandwidth ?
> > It asks the needed bandwidth based on its requirements
>
> The "enough bandwidth" is not always obvious. It's only for very
> simple cases that you can calculate the required bandwidth. Even for
> cases that you think might be "obvious/easy" aren't always easy.
>
> For example, you'd think a display IP would have a fixed bandwidth
> requirement for a fixed resolution screen. But that's far from the
> truth. It can also change as the number of layers change per frame.
> For video decoder/encoder, it depends on how well the frames compress
> with a specific compression scheme.
> So the "required" bandwidth is often a heuristic based on the IP
> frequency or traffic measurement.
>
> But that's not even the point I was making in this specific "bullet".
>
> A hardware IP might be screen/verified with only certain bandwidth
> levels. Or it might have hardware bugs that prevent it from using
> lower bandwidths even though it's technically sufficient. We need a
> way to capture that per path. This is not even a fictional case. This
> has been true multiple times over widely used IPs.
here you are mixing HW constraint on the soc and OPP screening with
bandwidth request from consumer
ICC framework is about getting bandwidth request not trying to fix
some HW/voltage dependency of the SoC
>
> > > the provider's job to take into account all the IP that might be
> > > connected to the interconnects. If the interconnect HW itself didn't
> >
> > That's not what I'm saying. The provider knows which bandwidth the
> > interconnect can provide as it is the ones which configures it. So if
> > the interconnect has a finite number of bandwidth point based probably
> > on the possible clock frequency and others config of the interconnect,
> > it selects the best final config after aggregating the request of the
> > consumer.
>
> I completely agree with this. What you are stating above is how it
> should work and that's the whole point of the interconnect framework.
>
> But this is orthogonal to the point I'm making.
It's not orthogonal because you want to add a OPP table pointer in the
ICC path structure to fix your platform HW constraint whereas it's not
the purpose of the framework IMO
>
> > > change, the provider driver shouldn't need to change. By your
> > > definition, a provider driver will have to account for all the
> > > possible bus masters that might be connected to it across all SoCs.
> >
> > you didn't catch my point
>
> Same. I think we are talking over each other. Let me try again.
>
> You are trying to describe how and interconnect provider and framework
> should work. There's no disagreement there.
>
> My point is that consumers might not want to or can not always use all
> the available bandwidth levels offered by the providers. There can be
> many reasons for that (which is what I listed in my earlier emails)
> and we need a good and generic way to capture that so that everyone
> isn't trying to invent their own property.
And my point is that you want to describe some platform or even UCs
specific constraint in the ICC framework which is not the place to do.
If the consumers might not want to use all available bandwidth because
this is not power efficient as an example, this should be describe
somewhere else to express that there is a shared power domain
between some devices and we shoudl ensure that all devices in this
power domain should use the Optimal Operating Point (optimal freq for
a voltage)
ICC framework describes the bandwidth request that are expressed by
the consumers for the current running state of their IP but it doesn't
reflect the fact that on platform A, the consumer should use bandwidth
X because it will select a voltage level of a shared power domain that
is optimized for the other devices B, C ... . It's up to the provider
to know HW details of the bus that it drives and to make such
decision; the consumer should always request the same
>
> > > That's not good design nor is it scalable.
> > >
> > > > >
> > > > > 2. Similarly during parts screening in the factory, some of the
> > > > > combinations might not have been screened and can't be guaranteed
> > > > > to work.
> > > >
> > > > As above, it's the provider's job to select the final bandwidth
> > > > according to its constraint
> > >
> > > Same reply as above.
> > >
> > > > >
> > > > > 3. Only a certain set of bandwidth levels might make sense to use from
> > > > > a power/performance balance given the device using it. For example:
> > > > > - The big CPU might not want to use some of the lower bandwidths
> > > > > but the little CPU might want to.
> > > > > - The big CPU might not want to use some intermediate bandwidth
> > > > > points if they don't save a lot of power compared to a higher
> > > > > bandwidth levels, but the little CPU might want to.
> > > > > - The little CPU might never want to use the higher set of
> > > > > bandwidth levels since they won't be power efficient for the use
> > > > > cases that might run on it.
> > > >
> > > > These example are quite vague about the reasons why little might never
> > > > want to use higher bandwidth.
> > >
> > > How is it vague? I just said because of power/performance balance.
> > >
> > > > But then, if little doesn't ask high bandwidth it will not use them.
> > >
> > > If you are running a heuristics based algorithm to pick bandwidth,
> > > this is how it'll know NOT to use some of the bandwidth levels.
> >
> > so you want to set a bandwidth according to the cpu frequency which is
> > what has been proposed in other thread
>
> Nope, that's just one heuristic. Often times it's based on hardware
> monitors measuring interconnect activity. If you go look at the SDM845
> in a Pixel 3, almost nothing is directly tied to the CPU frequency.
>
> Even if you are scaling bandwidth based on other hardware
> measurements, you might want to avoid some bandwidth level provided by
> the interconnect providers because it's suboptimal.
>
> For example, when making bandwidth votes to accommodate the big CPUs,
> you might never want to use some of the lower bandwidth levels because
> they are not power efficient for any CPU frequency or any bandwidth
> level. Because at those levels the memory/interconnect is so slow that
> it has a non-trivial utilization increase (because the CPU is
> stalling) of the big CPUs.
>
> Again, this is completely different from what the providers/icc
> framework does. Which is, once the request is made, they aggregate and
> set the actual interconnect frequencies correctly.
>
> > >
> > > > >
> > > > > 4. It might not make sense from a system level power perspective.
> > > > > Let's take an example of a path S (source) -> A -> B -> C -> D
> > > > > (destination).
> > > > > - A supports only 2, 5, 7 and 10 GB/s. B supports 1, 2 ... 10 GB/s.
> > > > > C supports 5 and 10 GB/s
> > > > > - If you combine and list the superset of bandwidth levels
> > > > > supported in that path, that'd be 1, 2, 3, ... 10 GB/s.
> > > > > - Which set of bandwidth levels make sense will depend on the
> > > > > hardware characteristics of the interconnects.
> > > > > - If B is the biggest power sink, then you might want to use all 10
> > > > > levels.
> > > > > - If A is the biggest power sink, then you might want to use all 2,
> > > > > 5 and 10 GB/s of the levels.
> > > > > - If C is the biggest power sink then you might only want to use 5
> > > > > and 10 GB/s
> > > > > - The more hops and paths you get the more convoluted this gets.
> > > > >
> > > > > 5. The design of the interconnects themselves might have an impact on
> > > > > which bandwidth levels are used.
> > > > > - For example, the FIFO depth between two specific interconnects
> > > > > might affect the valid bandwidth levels for a specific path.
> > > > > - Say S1 -> A -> B -> D1, S2 -> C -> B -> D1 and S2 -> C -> D2 are
> > > > > three paths.
> > > > > - If C <-> B FIFO depth is small, then there might be a requirement
> > > > > that C and B be closely performance matched to avoid system level
> > > > > congestion due to back pressure.
> > > > > - So S2 -> D1 path can't use all the bandwidth levels supported by
> > > > > C-B combination.
> > > > > - But S2 -> D2 can use all the bandwidth levels supported by C.
> > > > > - And S1 -> D1 can use all the levels supported by A-B combination.
> > > > >
> > > >
> > > > All the examples above makes sense but have to be handle by the
> > > > provider not the consumer. The consumer asks for a bandwidth according
> > > > to its constraints. Then the provider which is the driver that manages
> > > > the interconnect IP, should manage all this hardware and platform
> > > > specific stuff related to the interconnect IP in order to set the
> > > > optimal bandwidth that fit both consumer constraint and platform
> > > > specific configuration.
> > >
> > > Sure, but the provider itself can have interconnect properties to
> > > indicate which other interconnects it's tied to. And the provider will
> > > still need the interconnect-opp-table to denote which bandwidth levels
> > > are sensible to use with each of its connections.
>
> You seem to have missed this comment.
>
> Thanks,
> Saravana
>
> > > So in some instances the interconnect-opp-table covers the needs of
> > > purely consumers and in some instances purely providers. But in either
> > > case, it's still needed to describe the hardware properly.
> > >
> > > -Saravana
> > >
> > > > > These are just some of the reasons I could recollect in a few minutes.
> > > > > These are all real world cases I had to deal with in the past several
> > > > > years of dealing with scaling interconnects. I'm sure vendors and SoCs
> > > > > I'm not familiar with have other good reasons I'm not aware of.
> > > > >
> > > > > Trying to figure this all out by aggregating OPP tables of
> > > > > interconnect providers just isn't feasible nor is it efficient. The
> > > > > OPP tables for an interconnect path is describing the valid BW levels
> > > > > supported by that path and verified in hardware and makes a lot of
> > > > > sense to capture it clearly in DT.
> > > > >
> > > > > > So such kind of OPP table should be at
> > > > > > provider level but not at path level.
> > > > >
> > > > > They can also use it if they want to, but they'll probably want to use
> > > > > a frequency OPP table.
> > > > >
> > > > >
> > > > > -Saravana
> > > > >
> > > > > >
> > > > > > >
> > > > > > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > > > > ---
> > > > > > > drivers/interconnect/core.c | 27 ++++++++++++++++++++++++++-
> > > > > > > include/linux/interconnect.h | 7 +++++++
> > > > > > > 2 files changed, 33 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
> > > > > > > index 871eb4bc4efc..881bac80bc1e 100644
> > > > > > > --- a/drivers/interconnect/core.c
> > > > > > > +++ b/drivers/interconnect/core.c
> > > > > > > @@ -47,6 +47,7 @@ struct icc_req {
> > > > > > > */
> > > > > > > struct icc_path {
> > > > > > > size_t num_nodes;
> > > > > > > + struct opp_table *opp_table;
> > > > > > > struct icc_req reqs[];
> > > > > > > };
> > > > > > >
> > > > > > > @@ -313,7 +314,7 @@ struct icc_path *of_icc_get(struct device *dev, const char *name)
> > > > > > > {
> > > > > > > struct icc_path *path = ERR_PTR(-EPROBE_DEFER);
> > > > > > > struct icc_node *src_node, *dst_node;
> > > > > > > - struct device_node *np = NULL;
> > > > > > > + struct device_node *np = NULL, *opp_node;
> > > > > > > struct of_phandle_args src_args, dst_args;
> > > > > > > int idx = 0;
> > > > > > > int ret;
> > > > > > > @@ -381,10 +382,34 @@ struct icc_path *of_icc_get(struct device *dev, const char *name)
> > > > > > > dev_err(dev, "%s: invalid path=%ld\n", __func__, PTR_ERR(path));
> > > > > > > mutex_unlock(&icc_lock);
> > > > > > >
> > > > > > > + opp_node = of_parse_phandle(np, "interconnect-opp-table", idx);
> > > > > > > + if (opp_node) {
> > > > > > > + path->opp_table = dev_pm_opp_of_find_table_from_node(opp_node);
> > > > > > > + of_node_put(opp_node);
> > > > > > > + }
> > > > > > > +
> > > > > > > +
> > > > > > > return path;
> > > > > > > }
> > > > > > > EXPORT_SYMBOL_GPL(of_icc_get);
> > > > > > >
> > > > > > > +/**
> > > > > > > + * icc_get_opp_table() - Get the OPP table that corresponds to a path
> > > > > > > + * @path: reference to the path returned by icc_get()
> > > > > > > + *
> > > > > > > + * This function will return the OPP table that corresponds to a path handle.
> > > > > > > + * If the interconnect API is disabled, NULL is returned and the consumer
> > > > > > > + * drivers will still build. Drivers are free to handle this specifically, but
> > > > > > > + * they don't have to.
> > > > > > > + *
> > > > > > > + * Return: opp_table pointer on success. NULL is returned when the API is
> > > > > > > + * disabled or the OPP table is missing.
> > > > > > > + */
> > > > > > > +struct opp_table *icc_get_opp_table(struct icc_path *path)
> > > > > > > +{
> > > > > > > + return path->opp_table;
> > > > > > > +}
> > > > > > > +
> > > > > > > /**
> > > > > > > * icc_set_bw() - set bandwidth constraints on an interconnect path
> > > > > > > * @path: reference to the path returned by icc_get()
> > > > > > > diff --git a/include/linux/interconnect.h b/include/linux/interconnect.h
> > > > > > > index dc25864755ba..0c0bc55f0e89 100644
> > > > > > > --- a/include/linux/interconnect.h
> > > > > > > +++ b/include/linux/interconnect.h
> > > > > > > @@ -9,6 +9,7 @@
> > > > > > >
> > > > > > > #include <linux/mutex.h>
> > > > > > > #include <linux/types.h>
> > > > > > > +#include <linux/pm_opp.h>
> > > > > > >
> > > > > > > /* macros for converting to icc units */
> > > > > > > #define Bps_to_icc(x) ((x) / 1000)
> > > > > > > @@ -28,6 +29,7 @@ struct device;
> > > > > > > struct icc_path *icc_get(struct device *dev, const int src_id,
> > > > > > > const int dst_id);
> > > > > > > struct icc_path *of_icc_get(struct device *dev, const char *name);
> > > > > > > +struct opp_table *icc_get_opp_table(struct icc_path *path);
> > > > > > > void icc_put(struct icc_path *path);
> > > > > > > int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw);
> > > > > > >
> > > > > > > @@ -49,6 +51,11 @@ static inline void icc_put(struct icc_path *path)
> > > > > > > {
> > > > > > > }
> > > > > > >
> > > > > > > +static inline struct opp_table *icc_get_opp_table(struct icc_path *path)
> > > > > > > +{
> > > > > > > + return NULL;
> > > > > > > +}
> > > > > > > +
> > > > > > > static inline int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw)
> > > > > > > {
> > > > > > > return 0;
> > > > > > > --
> > > > > > > 2.22.0.410.gd8fdbe21b5-goog
> > > > > > >
> > > >
> > > > --
> > > > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
> > > >
^ permalink raw reply
* [PATCH v2 4/4] dt-bindings: devfreq: exynos-bus: remove unused property
From: Kamil Konieczny @ 2019-07-15 12:04 UTC (permalink / raw)
To: k.konieczny
Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, Chanwoo Choi,
Krzysztof Kozlowski, Kukjin Kim, Kyungmin Park, Mark Rutland,
MyungJoo Ham, Nishanth Menon, Rob Herring, Stephen Boyd,
Viresh Kumar, devicetree, linux-arm-kernel, linux-kernel,
linux-pm, linux-samsung-soc
In-Reply-To: <20190715120416.3561-1-k.konieczny@partner.samsung.com>
Remove unused DT property "exynos,voltage-tolerance".
Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
---
Documentation/devicetree/bindings/devfreq/exynos-bus.txt | 2 --
1 file changed, 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
index f8e946471a58..e71f752cc18f 100644
--- a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
+++ b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
@@ -50,8 +50,6 @@ Required properties only for passive bus device:
Optional properties only for parent bus device:
- exynos,saturation-ratio: the percentage value which is used to calibrate
the performance count against total cycle count.
-- exynos,voltage-tolerance: the percentage value for bus voltage tolerance
- which is used to calculate the max voltage.
Detailed correlation between sub-blocks and power line according to Exynos SoC:
- In case of Exynos3250, there are two power line as following:
--
2.22.0
^ permalink raw reply related
* [PATCH v2 3/4] ARM: dts: exynos: add initial data for coupled regulators for Exynos5422/5800
From: Kamil Konieczny @ 2019-07-15 12:04 UTC (permalink / raw)
To: k.konieczny
Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, Chanwoo Choi,
Krzysztof Kozlowski, Kukjin Kim, Kyungmin Park, Mark Rutland,
MyungJoo Ham, Nishanth Menon, Rob Herring, Stephen Boyd,
Viresh Kumar, devicetree, linux-arm-kernel, linux-kernel,
linux-pm, linux-samsung-soc
In-Reply-To: <20190715120416.3561-1-k.konieczny@partner.samsung.com>
Declare Exynos5422/5800 voltage ranges for opp points for big cpu core and
bus wcore and couple their voltage supllies as vdd_arm and vdd_int should
be in 300mV range.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
---
arch/arm/boot/dts/exynos5420.dtsi | 34 +++++++++----------
arch/arm/boot/dts/exynos5422-odroid-core.dtsi | 4 +++
arch/arm/boot/dts/exynos5800-peach-pi.dts | 4 +++
arch/arm/boot/dts/exynos5800.dtsi | 32 ++++++++---------
4 files changed, 41 insertions(+), 33 deletions(-)
diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
index 5fb2326875dc..0cbf74750553 100644
--- a/arch/arm/boot/dts/exynos5420.dtsi
+++ b/arch/arm/boot/dts/exynos5420.dtsi
@@ -48,62 +48,62 @@
opp-shared;
opp-1800000000 {
opp-hz = /bits/ 64 <1800000000>;
- opp-microvolt = <1250000>;
+ opp-microvolt = <1250000 1250000 1500000>;
clock-latency-ns = <140000>;
};
opp-1700000000 {
opp-hz = /bits/ 64 <1700000000>;
- opp-microvolt = <1212500>;
+ opp-microvolt = <1212500 1212500 1500000>;
clock-latency-ns = <140000>;
};
opp-1600000000 {
opp-hz = /bits/ 64 <1600000000>;
- opp-microvolt = <1175000>;
+ opp-microvolt = <1175000 1175000 1500000>;
clock-latency-ns = <140000>;
};
opp-1500000000 {
opp-hz = /bits/ 64 <1500000000>;
- opp-microvolt = <1137500>;
+ opp-microvolt = <1137500 1137500 1500000>;
clock-latency-ns = <140000>;
};
opp-1400000000 {
opp-hz = /bits/ 64 <1400000000>;
- opp-microvolt = <1112500>;
+ opp-microvolt = <1112500 1112500 1500000>;
clock-latency-ns = <140000>;
};
opp-1300000000 {
opp-hz = /bits/ 64 <1300000000>;
- opp-microvolt = <1062500>;
+ opp-microvolt = <1062500 1062500 1500000>;
clock-latency-ns = <140000>;
};
opp-1200000000 {
opp-hz = /bits/ 64 <1200000000>;
- opp-microvolt = <1037500>;
+ opp-microvolt = <1037500 1037500 1500000>;
clock-latency-ns = <140000>;
};
opp-1100000000 {
opp-hz = /bits/ 64 <1100000000>;
- opp-microvolt = <1012500>;
+ opp-microvolt = <1012500 1012500 1500000>;
clock-latency-ns = <140000>;
};
opp-1000000000 {
opp-hz = /bits/ 64 <1000000000>;
- opp-microvolt = < 987500>;
+ opp-microvolt = < 987500 987500 1500000>;
clock-latency-ns = <140000>;
};
opp-900000000 {
opp-hz = /bits/ 64 <900000000>;
- opp-microvolt = < 962500>;
+ opp-microvolt = < 962500 962500 1500000>;
clock-latency-ns = <140000>;
};
opp-800000000 {
opp-hz = /bits/ 64 <800000000>;
- opp-microvolt = < 937500>;
+ opp-microvolt = < 937500 937500 1500000>;
clock-latency-ns = <140000>;
};
opp-700000000 {
opp-hz = /bits/ 64 <700000000>;
- opp-microvolt = < 912500>;
+ opp-microvolt = < 912500 912500 1500000>;
clock-latency-ns = <140000>;
};
};
@@ -1100,23 +1100,23 @@
opp00 {
opp-hz = /bits/ 64 <84000000>;
- opp-microvolt = <925000>;
+ opp-microvolt = <925000 925000 1400000>;
};
opp01 {
opp-hz = /bits/ 64 <111000000>;
- opp-microvolt = <950000>;
+ opp-microvolt = <950000 950000 1400000>;
};
opp02 {
opp-hz = /bits/ 64 <222000000>;
- opp-microvolt = <950000>;
+ opp-microvolt = <950000 950000 1400000>;
};
opp03 {
opp-hz = /bits/ 64 <333000000>;
- opp-microvolt = <950000>;
+ opp-microvolt = <950000 950000 1400000>;
};
opp04 {
opp-hz = /bits/ 64 <400000000>;
- opp-microvolt = <987500>;
+ opp-microvolt = <987500 987500 1400000>;
};
};
diff --git a/arch/arm/boot/dts/exynos5422-odroid-core.dtsi b/arch/arm/boot/dts/exynos5422-odroid-core.dtsi
index 25d95de15c9b..65d094256b54 100644
--- a/arch/arm/boot/dts/exynos5422-odroid-core.dtsi
+++ b/arch/arm/boot/dts/exynos5422-odroid-core.dtsi
@@ -428,6 +428,8 @@
regulator-max-microvolt = <1500000>;
regulator-always-on;
regulator-boot-on;
+ regulator-coupled-with = <&buck3_reg>;
+ regulator-coupled-max-spread = <300000>;
};
buck3_reg: BUCK3 {
@@ -436,6 +438,8 @@
regulator-max-microvolt = <1400000>;
regulator-always-on;
regulator-boot-on;
+ regulator-coupled-with = <&buck2_reg>;
+ regulator-coupled-max-spread = <300000>;
};
buck4_reg: BUCK4 {
diff --git a/arch/arm/boot/dts/exynos5800-peach-pi.dts b/arch/arm/boot/dts/exynos5800-peach-pi.dts
index e0f470fe54c8..5c1e965ed7e9 100644
--- a/arch/arm/boot/dts/exynos5800-peach-pi.dts
+++ b/arch/arm/boot/dts/exynos5800-peach-pi.dts
@@ -257,6 +257,8 @@
regulator-always-on;
regulator-boot-on;
regulator-ramp-delay = <12500>;
+ regulator-coupled-with = <&buck3_reg>;
+ regulator-coupled-max-spread = <300000>;
regulator-state-mem {
regulator-off-in-suspend;
};
@@ -269,6 +271,8 @@
regulator-always-on;
regulator-boot-on;
regulator-ramp-delay = <12500>;
+ regulator-coupled-with = <&buck2_reg>;
+ regulator-coupled-max-spread = <300000>;
regulator-state-mem {
regulator-off-in-suspend;
};
diff --git a/arch/arm/boot/dts/exynos5800.dtsi b/arch/arm/boot/dts/exynos5800.dtsi
index 57d3b319fd65..2a74735d161c 100644
--- a/arch/arm/boot/dts/exynos5800.dtsi
+++ b/arch/arm/boot/dts/exynos5800.dtsi
@@ -22,61 +22,61 @@
&cluster_a15_opp_table {
opp-1700000000 {
- opp-microvolt = <1250000>;
+ opp-microvolt = <1250000 1250000 1500000>;
};
opp-1600000000 {
- opp-microvolt = <1250000>;
+ opp-microvolt = <1250000 1250000 1500000>;
};
opp-1500000000 {
- opp-microvolt = <1100000>;
+ opp-microvolt = <1100000 1100000 1500000>;
};
opp-1400000000 {
- opp-microvolt = <1100000>;
+ opp-microvolt = <1100000 1100000 1500000>;
};
opp-1300000000 {
- opp-microvolt = <1100000>;
+ opp-microvolt = <1100000 1100000 1500000>;
};
opp-1200000000 {
- opp-microvolt = <1000000>;
+ opp-microvolt = <1000000 1000000 1500000>;
};
opp-1100000000 {
- opp-microvolt = <1000000>;
+ opp-microvolt = <1000000 1000000 1500000>;
};
opp-1000000000 {
- opp-microvolt = <1000000>;
+ opp-microvolt = <1000000 1000000 1500000>;
};
opp-900000000 {
- opp-microvolt = <1000000>;
+ opp-microvolt = <1000000 1000000 1500000>;
};
opp-800000000 {
- opp-microvolt = <900000>;
+ opp-microvolt = <900000 900000 1500000>;
};
opp-700000000 {
- opp-microvolt = <900000>;
+ opp-microvolt = <900000 900000 1500000>;
};
opp-600000000 {
opp-hz = /bits/ 64 <600000000>;
- opp-microvolt = <900000>;
+ opp-microvolt = <900000 900000 1500000>;
clock-latency-ns = <140000>;
};
opp-500000000 {
opp-hz = /bits/ 64 <500000000>;
- opp-microvolt = <900000>;
+ opp-microvolt = <900000 900000 1500000>;
clock-latency-ns = <140000>;
};
opp-400000000 {
opp-hz = /bits/ 64 <400000000>;
- opp-microvolt = <900000>;
+ opp-microvolt = <900000 900000 1500000>;
clock-latency-ns = <140000>;
};
opp-300000000 {
opp-hz = /bits/ 64 <300000000>;
- opp-microvolt = <900000>;
+ opp-microvolt = <900000 900000 1500000>;
clock-latency-ns = <140000>;
};
opp-200000000 {
opp-hz = /bits/ 64 <200000000>;
- opp-microvolt = <900000>;
+ opp-microvolt = <900000 900000 1500000>;
clock-latency-ns = <140000>;
};
};
--
2.22.0
^ permalink raw reply related
* [PATCH v2 2/4] devfreq: exynos-bus: convert to use dev_pm_opp_set_rate()
From: Kamil Konieczny @ 2019-07-15 12:04 UTC (permalink / raw)
To: k.konieczny
Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, Chanwoo Choi,
Krzysztof Kozlowski, Kukjin Kim, Kyungmin Park, Mark Rutland,
MyungJoo Ham, Nishanth Menon, Rob Herring, Stephen Boyd,
Viresh Kumar, devicetree, linux-arm-kernel, linux-kernel,
linux-pm, linux-samsung-soc
In-Reply-To: <20190715120416.3561-1-k.konieczny@partner.samsung.com>
Reuse opp core code for setting bus clock and voltage. As a side
effect this allow useage of coupled regulators feature (required
for boards using Exynos5422/5800 SoCs) because dev_pm_opp_set_rate()
uses regulator_set_voltage_triplet() for setting regulator voltage
while the old code used regulator_set_voltage_tol() with fixed
tolerance. This patch also removes no longer needed parsing of DT
property "exynos,voltage-tolerance" (no Exynos devfreq DT node uses
it).
Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
---
drivers/devfreq/exynos-bus.c | 172 ++++++++++++++---------------------
1 file changed, 66 insertions(+), 106 deletions(-)
diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
index 486cc5b422f1..7fc4f76bd848 100644
--- a/drivers/devfreq/exynos-bus.c
+++ b/drivers/devfreq/exynos-bus.c
@@ -25,7 +25,6 @@
#include <linux/slab.h>
#define DEFAULT_SATURATION_RATIO 40
-#define DEFAULT_VOLTAGE_TOLERANCE 2
struct exynos_bus {
struct device *dev;
@@ -37,9 +36,9 @@ struct exynos_bus {
unsigned long curr_freq;
- struct regulator *regulator;
+ struct opp_table *opp_table;
+
struct clk *clk;
- unsigned int voltage_tolerance;
unsigned int ratio;
};
@@ -99,56 +98,25 @@ static int exynos_bus_target(struct device *dev, unsigned long *freq, u32 flags)
{
struct exynos_bus *bus = dev_get_drvdata(dev);
struct dev_pm_opp *new_opp;
- unsigned long old_freq, new_freq, new_volt, tol;
int ret = 0;
-
- /* Get new opp-bus instance according to new bus clock */
+ /*
+ * New frequency for bus may not be exactly matched to opp, adjust
+ * *freq to correct value.
+ */
new_opp = devfreq_recommended_opp(dev, freq, flags);
if (IS_ERR(new_opp)) {
dev_err(dev, "failed to get recommended opp instance\n");
return PTR_ERR(new_opp);
}
- new_freq = dev_pm_opp_get_freq(new_opp);
- new_volt = dev_pm_opp_get_voltage(new_opp);
dev_pm_opp_put(new_opp);
- old_freq = bus->curr_freq;
-
- if (old_freq == new_freq)
- return 0;
- tol = new_volt * bus->voltage_tolerance / 100;
-
/* Change voltage and frequency according to new OPP level */
mutex_lock(&bus->lock);
+ ret = dev_pm_opp_set_rate(dev, *freq);
+ if (!ret)
+ bus->curr_freq = *freq;
- if (old_freq < new_freq) {
- ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
- if (ret < 0) {
- dev_err(bus->dev, "failed to set voltage\n");
- goto out;
- }
- }
-
- ret = clk_set_rate(bus->clk, new_freq);
- if (ret < 0) {
- dev_err(dev, "failed to change clock of bus\n");
- clk_set_rate(bus->clk, old_freq);
- goto out;
- }
-
- if (old_freq > new_freq) {
- ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
- if (ret < 0) {
- dev_err(bus->dev, "failed to set voltage\n");
- goto out;
- }
- }
- bus->curr_freq = new_freq;
-
- dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
- old_freq, new_freq, clk_get_rate(bus->clk));
-out:
mutex_unlock(&bus->lock);
return ret;
@@ -194,10 +162,11 @@ static void exynos_bus_exit(struct device *dev)
if (ret < 0)
dev_warn(dev, "failed to disable the devfreq-event devices\n");
- if (bus->regulator)
- regulator_disable(bus->regulator);
+ if (bus->opp_table)
+ dev_pm_opp_put_regulators(bus->opp_table);
dev_pm_opp_of_remove_table(dev);
+
clk_disable_unprepare(bus->clk);
}
@@ -209,39 +178,26 @@ static int exynos_bus_passive_target(struct device *dev, unsigned long *freq,
{
struct exynos_bus *bus = dev_get_drvdata(dev);
struct dev_pm_opp *new_opp;
- unsigned long old_freq, new_freq;
- int ret = 0;
+ int ret;
- /* Get new opp-bus instance according to new bus clock */
+ /*
+ * New frequency for bus may not be exactly matched to opp, adjust
+ * *freq to correct value.
+ */
new_opp = devfreq_recommended_opp(dev, freq, flags);
if (IS_ERR(new_opp)) {
dev_err(dev, "failed to get recommended opp instance\n");
return PTR_ERR(new_opp);
}
- new_freq = dev_pm_opp_get_freq(new_opp);
dev_pm_opp_put(new_opp);
- old_freq = bus->curr_freq;
-
- if (old_freq == new_freq)
- return 0;
-
/* Change the frequency according to new OPP level */
mutex_lock(&bus->lock);
+ ret = dev_pm_opp_set_rate(dev, *freq);
+ if (!ret)
+ bus->curr_freq = *freq;
- ret = clk_set_rate(bus->clk, new_freq);
- if (ret < 0) {
- dev_err(dev, "failed to set the clock of bus\n");
- goto out;
- }
-
- *freq = new_freq;
- bus->curr_freq = new_freq;
-
- dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
- old_freq, new_freq, clk_get_rate(bus->clk));
-out:
mutex_unlock(&bus->lock);
return ret;
@@ -259,20 +215,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
struct exynos_bus *bus)
{
struct device *dev = bus->dev;
- int i, ret, count, size;
-
- /* Get the regulator to provide each bus with the power */
- bus->regulator = devm_regulator_get(dev, "vdd");
- if (IS_ERR(bus->regulator)) {
- dev_err(dev, "failed to get VDD regulator\n");
- return PTR_ERR(bus->regulator);
- }
-
- ret = regulator_enable(bus->regulator);
- if (ret < 0) {
- dev_err(dev, "failed to enable VDD regulator\n");
- return ret;
- }
+ int i, count, size;
/*
* Get the devfreq-event devices to get the current utilization of
@@ -281,24 +224,20 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
count = devfreq_event_get_edev_count(dev);
if (count < 0) {
dev_err(dev, "failed to get the count of devfreq-event dev\n");
- ret = count;
- goto err_regulator;
+ return count;
}
+
bus->edev_count = count;
size = sizeof(*bus->edev) * count;
bus->edev = devm_kzalloc(dev, size, GFP_KERNEL);
- if (!bus->edev) {
- ret = -ENOMEM;
- goto err_regulator;
- }
+ if (!bus->edev)
+ return -ENOMEM;
for (i = 0; i < count; i++) {
bus->edev[i] = devfreq_event_get_edev_by_phandle(dev, i);
- if (IS_ERR(bus->edev[i])) {
- ret = -EPROBE_DEFER;
- goto err_regulator;
- }
+ if (IS_ERR(bus->edev[i]))
+ return -EPROBE_DEFER;
}
/*
@@ -314,22 +253,15 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
if (of_property_read_u32(np, "exynos,saturation-ratio", &bus->ratio))
bus->ratio = DEFAULT_SATURATION_RATIO;
- if (of_property_read_u32(np, "exynos,voltage-tolerance",
- &bus->voltage_tolerance))
- bus->voltage_tolerance = DEFAULT_VOLTAGE_TOLERANCE;
-
return 0;
-
-err_regulator:
- regulator_disable(bus->regulator);
-
- return ret;
}
static int exynos_bus_parse_of(struct device_node *np,
- struct exynos_bus *bus)
+ struct exynos_bus *bus, bool passive)
{
struct device *dev = bus->dev;
+ struct opp_table *opp_table;
+ const char *vdd = "vdd";
struct dev_pm_opp *opp;
unsigned long rate;
int ret;
@@ -347,11 +279,22 @@ static int exynos_bus_parse_of(struct device_node *np,
return ret;
}
+ if (!passive) {
+ opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
+ if (IS_ERR(opp_table)) {
+ ret = PTR_ERR(opp_table);
+ dev_err(dev, "failed to set regulators %d\n", ret);
+ goto err_clk;
+ }
+
+ bus->opp_table = opp_table;
+ }
+
/* Get the freq and voltage from OPP table to scale the bus freq */
ret = dev_pm_opp_of_add_table(dev);
if (ret < 0) {
dev_err(dev, "failed to get OPP table\n");
- goto err_clk;
+ goto err_regulator;
}
rate = clk_get_rate(bus->clk);
@@ -362,6 +305,7 @@ static int exynos_bus_parse_of(struct device_node *np,
ret = PTR_ERR(opp);
goto err_opp;
}
+
bus->curr_freq = dev_pm_opp_get_freq(opp);
dev_pm_opp_put(opp);
@@ -369,6 +313,13 @@ static int exynos_bus_parse_of(struct device_node *np,
err_opp:
dev_pm_opp_of_remove_table(dev);
+
+err_regulator:
+ if (bus->opp_table) {
+ dev_pm_opp_put_regulators(bus->opp_table);
+ bus->opp_table = NULL;
+ }
+
err_clk:
clk_disable_unprepare(bus->clk);
@@ -386,6 +337,7 @@ static int exynos_bus_probe(struct platform_device *pdev)
struct exynos_bus *bus;
int ret, max_state;
unsigned long min_freq, max_freq;
+ bool passive = false;
if (!np) {
dev_err(dev, "failed to find devicetree node\n");
@@ -395,12 +347,18 @@ static int exynos_bus_probe(struct platform_device *pdev)
bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
if (!bus)
return -ENOMEM;
+
mutex_init(&bus->lock);
bus->dev = &pdev->dev;
platform_set_drvdata(pdev, bus);
+ node = of_parse_phandle(dev->of_node, "devfreq", 0);
+ if (node) {
+ of_node_put(node);
+ passive = true;
+ }
/* Parse the device-tree to get the resource information */
- ret = exynos_bus_parse_of(np, bus);
+ ret = exynos_bus_parse_of(np, bus, passive);
if (ret < 0)
return ret;
@@ -410,13 +368,10 @@ static int exynos_bus_probe(struct platform_device *pdev)
goto err;
}
- node = of_parse_phandle(dev->of_node, "devfreq", 0);
- if (node) {
- of_node_put(node);
+ if (passive)
goto passive;
- } else {
- ret = exynos_bus_parent_parse_of(np, bus);
- }
+
+ ret = exynos_bus_parent_parse_of(np, bus);
if (ret < 0)
goto err;
@@ -509,6 +464,11 @@ static int exynos_bus_probe(struct platform_device *pdev)
err:
dev_pm_opp_of_remove_table(dev);
+ if (bus->opp_table) {
+ dev_pm_opp_put_regulators(bus->opp_table);
+ bus->opp_table = NULL;
+ }
+
clk_disable_unprepare(bus->clk);
return ret;
--
2.22.0
^ permalink raw reply related
* [PATCH v2 0/4] add coupled regulators for Exynos5422/5800
From: Kamil Konieczny @ 2019-07-15 12:04 UTC (permalink / raw)
To: k.konieczny
Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, Chanwoo Choi,
Krzysztof Kozlowski, Kukjin Kim, Kyungmin Park, Mark Rutland,
MyungJoo Ham, Nishanth Menon, Rob Herring, Stephen Boyd,
Viresh Kumar, devicetree, linux-arm-kernel, linux-kernel,
linux-pm, linux-samsung-soc
In-Reply-To: <CGME20190715120430eucas1p1dd216e552403899e614845295373e467@eucas1p1.samsung.com>
Hi,
The main purpose of this patch series is to add coupled regulators for
Exynos5422/5800 to keep constrain on voltage difference between vdd_arm
and vdd_int to be at most 300mV. In exynos-bus instead of using
regulator_set_voltage_tol() with default voltage tolerance it should be
used regulator_set_voltage_triplet() with volatege range, and this is
already present in opp/core.c code, so it can be reused. While at this,
move setting regulators into opp/core.
This patchset was tested on Odroid XU3.
The last patch depends on two previous.
Changes in v2:
- improve regulators enable/disable code in opp/core as suggested by
Viresh Kumar
- add new patch for remove unused dt-bindings as suggested by Krzysztof
Kozlowski
Regards,
Kamil
Kamil Konieczny (3):
opp: core: add regulators enable and disable
devfreq: exynos-bus: convert to use dev_pm_opp_set_rate()
dt-bindings: devfreq: exynos-bus: remove unused property
Marek Szyprowski (1):
ARM: dts: exynos: add initial data for coupled regulators for
Exynos5422/5800
.../bindings/devfreq/exynos-bus.txt | 2 -
arch/arm/boot/dts/exynos5420.dtsi | 34 ++--
arch/arm/boot/dts/exynos5422-odroid-core.dtsi | 4 +
arch/arm/boot/dts/exynos5800-peach-pi.dts | 4 +
arch/arm/boot/dts/exynos5800.dtsi | 32 ++--
drivers/devfreq/exynos-bus.c | 172 +++++++-----------
drivers/opp/core.c | 18 +-
7 files changed, 122 insertions(+), 144 deletions(-)
--
2.22.0
^ permalink raw reply
* [PATCH v2 1/4] opp: core: add regulators enable and disable
From: Kamil Konieczny @ 2019-07-15 12:04 UTC (permalink / raw)
To: k.konieczny
Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, Chanwoo Choi,
Krzysztof Kozlowski, Kukjin Kim, Kyungmin Park, Mark Rutland,
MyungJoo Ham, Nishanth Menon, Rob Herring, Stephen Boyd,
Viresh Kumar, devicetree, linux-arm-kernel, linux-kernel,
linux-pm, linux-samsung-soc
In-Reply-To: <20190715120416.3561-1-k.konieczny@partner.samsung.com>
Add enable regulators to dev_pm_opp_set_regulators() and disable
regulators to dev_pm_opp_put_regulators(). This prepares for
converting exynos-bus devfreq driver to use dev_pm_opp_set_rate().
Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
--
Changes in v2:
- move regulator enable and disable into loop
---
drivers/opp/core.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 0e7703fe733f..069c5cf8827e 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1570,6 +1570,10 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
goto free_regulators;
}
+ ret = regulator_enable(reg);
+ if (ret < 0)
+ goto disable;
+
opp_table->regulators[i] = reg;
}
@@ -1582,9 +1586,15 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
return opp_table;
+disable:
+ regulator_put(reg);
+ --i;
+
free_regulators:
- while (i != 0)
- regulator_put(opp_table->regulators[--i]);
+ for (; i >= 0; --i) {
+ regulator_disable(opp_table->regulators[i]);
+ regulator_put(opp_table->regulators[i]);
+ }
kfree(opp_table->regulators);
opp_table->regulators = NULL;
@@ -1610,8 +1620,10 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table)
/* Make sure there are no concurrent readers while updating opp_table */
WARN_ON(!list_empty(&opp_table->opp_list));
- for (i = opp_table->regulator_count - 1; i >= 0; i--)
+ for (i = opp_table->regulator_count - 1; i >= 0; i--) {
+ regulator_disable(opp_table->regulators[i]);
regulator_put(opp_table->regulators[i]);
+ }
_free_set_opp_data(opp_table);
--
2.22.0
^ permalink raw reply related
* Re: [PATCH v11 1/5] sched/core: uclamp: Extend CPU's cgroup controller
From: Patrick Bellasi @ 2019-07-15 13:38 UTC (permalink / raw)
To: Quentin Perret, Peter Zijlstra, Tejun Heo
Cc: linux-kernel, linux-pm, Ingo Molnar, Rafael J . Wysocki,
Vincent Guittot, Viresh Kumar, Paul Turner, Dietmar Eggemann,
Morten Rasmussen, Juri Lelli, Todd Kjos, Joel Fernandes,
Steve Muckle, Suren Baghdasaryan, Alessio Balsini
In-Reply-To: <20190708110838.4ohd7pqx5ngkzcsu@queper01-lin>
On 08-Jul 12:08, Quentin Perret wrote:
> Hi Patrick,
Hi Quentin!
> On Monday 08 Jul 2019 at 09:43:53 (+0100), Patrick Bellasi wrote:
> > +static inline int uclamp_scale_from_percent(char *buf, u64 *value)
> > +{
> > + *value = SCHED_CAPACITY_SCALE;
> > +
> > + buf = strim(buf);
> > + if (strncmp("max", buf, 4)) {
> > + s64 percent;
> > + int ret;
> > +
> > + ret = cgroup_parse_float(buf, 2, &percent);
> > + if (ret)
> > + return ret;
> > +
> > + percent <<= SCHED_CAPACITY_SHIFT;
> > + *value = DIV_ROUND_CLOSEST_ULL(percent, 10000);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static inline u64 uclamp_percent_from_scale(u64 value)
> > +{
> > + return DIV_ROUND_CLOSEST_ULL(value * 10000, SCHED_CAPACITY_SCALE);
> > +}
>
> FWIW, I tried the patches and realized these conversions result in a
> 'funny' behaviour from a user's perspective. Things like this happen:
>
> $ echo 20 > cpu.uclamp.min
> $ cat cpu.uclamp.min
> 20.2
> $ echo 20.2 > cpu.uclamp.min
> $ cat cpu.uclamp.min
> 20.21
>
> Having looked at the code, I get why this is happening, but I'm not sure
> if a random user will. It's not an issue per se, but it's just a bit
> weird.
Yes, that's what we get if we need to use a "two decimal digit
precision percentage" to represent a 1024 range in kernel space.
I don't think the "percent <=> utilization" conversion code can be
made more robust. The only possible alternative I see to get back
exactly what we write in, is to store the actual request in kernel
space, alongside its conversion to the SCHED_CAPACITY_SCALE required by the
actual scheduler code.
Something along these lines (on top of what we have in this series):
---8<---
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index ddc5fcd4b9cf..82b28cfa5c3f 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -7148,40 +7148,35 @@ static void cpu_util_update_eff(struct cgroup_subsys_state *css)
}
}
-static inline int uclamp_scale_from_percent(char *buf, u64 *value)
+static inline int uclamp_scale_from_percent(char *buf, s64 *percent, u64 *scale)
{
- *value = SCHED_CAPACITY_SCALE;
+ *scale = SCHED_CAPACITY_SCALE;
buf = strim(buf);
if (strncmp("max", buf, 4)) {
- s64 percent;
int ret;
- ret = cgroup_parse_float(buf, 2, &percent);
+ ret = cgroup_parse_float(buf, 2, percent);
if (ret)
return ret;
- percent <<= SCHED_CAPACITY_SHIFT;
- *value = DIV_ROUND_CLOSEST_ULL(percent, 10000);
+ *scale = *percent << SCHED_CAPACITY_SHIFT;
+ *scale = DIV_ROUND_CLOSEST_ULL(*scale, 10000);
}
return 0;
}
-static inline u64 uclamp_percent_from_scale(u64 value)
-{
- return DIV_ROUND_CLOSEST_ULL(value * 10000, SCHED_CAPACITY_SCALE);
-}
-
static ssize_t cpu_uclamp_min_write(struct kernfs_open_file *of,
char *buf, size_t nbytes,
loff_t off)
{
struct task_group *tg;
u64 min_value;
+ s64 percent;
int ret;
- ret = uclamp_scale_from_percent(buf, &min_value);
+ ret = uclamp_scale_from_percent(buf, &percent, &min_value);
if (ret)
return ret;
if (min_value > SCHED_CAPACITY_SCALE)
@@ -7197,6 +7192,9 @@ static ssize_t cpu_uclamp_min_write(struct kernfs_open_file *of,
/* Update effective clamps to track the most restrictive value */
cpu_util_update_eff(of_css(of));
+ /* Keep track of the actual requested value */
+ tg->uclamp_pct[UCLAMP_MIN] = percent;
+
rcu_read_unlock();
mutex_unlock(&uclamp_mutex);
@@ -7209,9 +7207,10 @@ static ssize_t cpu_uclamp_max_write(struct kernfs_open_file *of,
{
struct task_group *tg;
u64 max_value;
+ s64 percent;
int ret;
- ret = uclamp_scale_from_percent(buf, &max_value);
+ ret = uclamp_scale_from_percent(buf, &percent, &max_value);
if (ret)
return ret;
if (max_value > SCHED_CAPACITY_SCALE)
@@ -7227,6 +7226,9 @@ static ssize_t cpu_uclamp_max_write(struct kernfs_open_file *of,
/* Update effective clamps to track the most restrictive value */
cpu_util_update_eff(of_css(of));
+ /* Keep track of the actual requested value */
+ tg->uclamp_pct[UCLAMP_MAX] = percent;
+
rcu_read_unlock();
mutex_unlock(&uclamp_mutex);
@@ -7251,7 +7253,7 @@ static inline void cpu_uclamp_print(struct seq_file *sf,
return;
}
- percent = uclamp_percent_from_scale(util_clamp);
+ percent = tg->uclamp_pct[clamp_id];
percent = div_u64_rem(percent, 100, &rem);
seq_printf(sf, "%llu.%u\n", percent, rem);
}
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 0e37f4a4e536..4f9b0c660310 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -395,6 +395,8 @@ struct task_group {
struct cfs_bandwidth cfs_bandwidth;
#ifdef CONFIG_UCLAMP_TASK_GROUP
+ /* The two decimal precision [%] value requested from user-space */
+ unsigned int uclamp_pct[UCLAMP_CNT];
/* Clamp values requested for a task group */
struct uclamp_se uclamp_req[UCLAMP_CNT];
/* Effective clamp values used for a task group */
---8<---
> I guess one way to fix this would be to revert back to having a
> 1024-scale for the cgroup interface too ... Though I understand Tejun
> wanted % for consistency with other things.
Yes that would be another option, which will also keep aligned the per-task
and system-wide APIs with the CGroups one. Although, AFAIU, having two
different APIs is not considered a major issue.
> So, I'm not sure if this is still up for discussion, but in any case I
> wanted to say I support your original idea of using a 1024-scale for the
> cgroups interface, since that would solve the 'issue' above and keeps
> things consistent with the per-task API too.
Right, I'm personally more leaning toward either going back to use
SCHED_CAPACITY_SCALE or the add the small change I suggested above.
Tejun, Peter: any preference? Alternative suggestions?
> Thanks,
> Quentin
Cheers,
Patrick
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply related
* [PATCH AUTOSEL 5.1 057/219] cpupower : frequency-set -r option misses the last cpu in related cpu list
From: Sasha Levin @ 2019-07-15 14:00 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Abhishek Goel, Thomas Renninger, Shuah Khan, Sasha Levin,
linux-pm
In-Reply-To: <20190715140341.6443-1-sashal@kernel.org>
From: Abhishek Goel <huntbag@linux.vnet.ibm.com>
[ Upstream commit 04507c0a9385cc8280f794a36bfff567c8cc1042 ]
To set frequency on specific cpus using cpupower, following syntax can
be used :
cpupower -c #i frequency-set -f #f -r
While setting frequency using cpupower frequency-set command, if we use
'-r' option, it is expected to set frequency for all cpus related to
cpu #i. But it is observed to be missing the last cpu in related cpu
list. This patch fixes the problem.
Signed-off-by: Abhishek Goel <huntbag@linux.vnet.ibm.com>
Reviewed-by: Thomas Renninger <trenn@suse.de>
Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
tools/power/cpupower/utils/cpufreq-set.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tools/power/cpupower/utils/cpufreq-set.c b/tools/power/cpupower/utils/cpufreq-set.c
index 1eef0aed6423..08a405593a79 100644
--- a/tools/power/cpupower/utils/cpufreq-set.c
+++ b/tools/power/cpupower/utils/cpufreq-set.c
@@ -306,6 +306,8 @@ int cmd_freq_set(int argc, char **argv)
bitmask_setbit(cpus_chosen, cpus->cpu);
cpus = cpus->next;
}
+ /* Set the last cpu in related cpus list */
+ bitmask_setbit(cpus_chosen, cpus->cpu);
cpufreq_put_related_cpus(cpus);
}
}
--
2.20.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.19 065/158] x86/cpu: Add Ice Lake NNPI to Intel family
From: Sasha Levin @ 2019-07-15 14:16 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Rajneesh Bhardwaj, Thomas Gleixner, bp, Borislav Petkov,
Dave Hansen, Andy Shevchenko, H. Peter Anvin, Kan Liang,
Peter Zijlstra, platform-driver-x86, Qiuxu Zhuo,
Srinivas Pandruvada, Len Brown, Linux PM, Sasha Levin
In-Reply-To: <20190715141809.8445-1-sashal@kernel.org>
From: Rajneesh Bhardwaj <rajneesh.bhardwaj@linux.intel.com>
[ Upstream commit e32d045cd4ba06b59878323e434bad010e78e658 ]
Add the CPUID model number of Ice Lake Neural Network Processor for Deep
Learning Inference (ICL-NNPI) to the Intel family list. Ice Lake NNPI uses
model number 0x9D and this will be documented in a future version of Intel
Software Development Manual.
Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@linux.intel.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: bp@suse.de
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: platform-driver-x86@vger.kernel.org
Cc: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Cc: Len Brown <lenb@kernel.org>
Cc: Linux PM <linux-pm@vger.kernel.org>
Link: https://lkml.kernel.org/r/20190606012419.13250-1-rajneesh.bhardwaj@linux.intel.com
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
arch/x86/include/asm/intel-family.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/x86/include/asm/intel-family.h b/arch/x86/include/asm/intel-family.h
index 2e38fb82b91d..aebedbaf5260 100644
--- a/arch/x86/include/asm/intel-family.h
+++ b/arch/x86/include/asm/intel-family.h
@@ -56,6 +56,7 @@
#define INTEL_FAM6_ICELAKE_XEON_D 0x6C
#define INTEL_FAM6_ICELAKE_DESKTOP 0x7D
#define INTEL_FAM6_ICELAKE_MOBILE 0x7E
+#define INTEL_FAM6_ICELAKE_NNPI 0x9D
/* "Small Core" Processors (Atom) */
--
2.20.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.14 032/105] cpupower : frequency-set -r option misses the last cpu in related cpu list
From: Sasha Levin @ 2019-07-15 14:27 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Abhishek Goel, Thomas Renninger, Shuah Khan, Sasha Levin,
linux-pm
In-Reply-To: <20190715142839.9896-1-sashal@kernel.org>
From: Abhishek Goel <huntbag@linux.vnet.ibm.com>
[ Upstream commit 04507c0a9385cc8280f794a36bfff567c8cc1042 ]
To set frequency on specific cpus using cpupower, following syntax can
be used :
cpupower -c #i frequency-set -f #f -r
While setting frequency using cpupower frequency-set command, if we use
'-r' option, it is expected to set frequency for all cpus related to
cpu #i. But it is observed to be missing the last cpu in related cpu
list. This patch fixes the problem.
Signed-off-by: Abhishek Goel <huntbag@linux.vnet.ibm.com>
Reviewed-by: Thomas Renninger <trenn@suse.de>
Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
tools/power/cpupower/utils/cpufreq-set.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tools/power/cpupower/utils/cpufreq-set.c b/tools/power/cpupower/utils/cpufreq-set.c
index 1eef0aed6423..08a405593a79 100644
--- a/tools/power/cpupower/utils/cpufreq-set.c
+++ b/tools/power/cpupower/utils/cpufreq-set.c
@@ -306,6 +306,8 @@ int cmd_freq_set(int argc, char **argv)
bitmask_setbit(cpus_chosen, cpus->cpu);
cpus = cpus->next;
}
+ /* Set the last cpu in related cpus list */
+ bitmask_setbit(cpus_chosen, cpus->cpu);
cpufreq_put_related_cpus(cpus);
}
}
--
2.20.1
^ permalink raw reply related
* [NOTICE] Eduardo is offline during July
From: Eduardo Valentin @ 2019-07-15 14:32 UTC (permalink / raw)
To: Linux PM, Rui Zhang
Folks,
I just want to let you know that I am offline during July with mostly
very limited Internet access. I will return to review and collect patch
in August. Sorry for any inconvenience this may have caused.
If any urgent fixes/patches please redirect to Rui.
BR,
Eduardo Valentin
^ permalink raw reply
* [PATCH 1/9] rcu/update: Remove useless check for debug_locks (v1)
From: Joel Fernandes (Google) @ 2019-07-15 14:36 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, 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, 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: <20190715143705.117908-1-joel@joelfernandes.org>
In rcu_read_lock_sched_held(), debug_locks can never be true at the
point we check it because we already check debug_locks in
debug_lockdep_rcu_enabled() in the beginning. Remove the check.
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
kernel/rcu/update.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
index 61df2bf08563..9dd5aeef6e70 100644
--- a/kernel/rcu/update.c
+++ b/kernel/rcu/update.c
@@ -93,17 +93,13 @@ module_param(rcu_normal_after_boot, int, 0);
*/
int rcu_read_lock_sched_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;
- if (debug_locks)
- lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
- return lockdep_opinion || !preemptible();
+ return lock_is_held(&rcu_sched_lock_map) || !preemptible();
}
EXPORT_SYMBOL(rcu_read_lock_sched_held);
#endif
--
2.22.0.510.g264f2c817a-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