* [BUG] dev_pm_opp refcount issue on Arm Juno r0
@ 2018-12-20 15:27 Valentin Schneider
2019-01-03 7:05 ` Viresh Kumar
2019-01-04 9:44 ` [PATCH] cpufreq: scpi/scmi: Fix freeing of dynamic OPPs Viresh Kumar
0 siblings, 2 replies; 11+ messages in thread
From: Valentin Schneider @ 2018-12-20 15:27 UTC (permalink / raw)
To: linux-kernel, Linux PM, LAK
Cc: nm, sboyd, Viresh Kumar, Rafael J. Wysocki, Douglas Raillard,
Quentin Perret, Sudeep Holla, Dietmar Eggemann
Hi,
While running some hotplug torture test [1] on my Juno r0 I came across
the follow splat:
[ 716.561862] ------------[ cut here ]------------
[ 716.566451] refcount_t: underflow; use-after-free.
[ 716.571240] WARNING: CPU: 2 PID: 18 at lib/refcount.c:280 refcount_dec_not_one+0x9c/0xc0
[ 716.579246] Modules linked in:
[ 716.582269] CPU: 2 PID: 18 Comm: cpuhp/2 Not tainted 4.20.0-rc7 #39
[ 716.588469] Hardware name: ARM Juno development board (r0) (DT)
[ 716.594326] pstate: 40000005 (nZcv daif -PAN -UAO)
[ 716.599065] pc : refcount_dec_not_one+0x9c/0xc0
[ 716.603546] lr : refcount_dec_not_one+0x9c/0xc0
[ 716.608024] sp : ffff00000a063c70
[ 716.611299] x29: ffff00000a063c70 x28: 0000000000000000
[ 716.616555] x27: 0000000000000000 x26: 0000000000000002
[ 716.621810] x25: ffff000009169000 x24: ffff000008f8e1b0
[ 716.627065] x23: ffff000008ce0920 x22: 00000000ffffffff
[ 716.632319] x21: ffff000009169000 x20: ffff8009762a2664
[ 716.637574] x19: ffff000009294a90 x18: 0000000000000400
[ 716.642828] x17: 0000000000000000 x16: 0000000000000000
[ 716.648082] x15: 0000000000000000 x14: 0000000000000400
[ 716.653336] x13: 000000000000023f x12: 0000000000043705
[ 716.658590] x11: 0000000000000108 x10: 0000000000000960
[ 716.663844] x9 : ffff00000a063970 x8 : ffff800976943ec0
[ 716.669098] x7 : 0000000000000000 x6 : ffff80097ff720b8
[ 716.674353] x5 : ffff80097ff720b8 x4 : 0000000000000000
[ 716.679607] x3 : ffff80097ff78e68 x2 : ffff80097ff720b8
[ 716.684861] x1 : 6374e2a7925c1100 x0 : 0000000000000000
[ 716.690115] Call trace:
[ 716.692532] refcount_dec_not_one+0x9c/0xc0
[ 716.696669] refcount_dec_and_mutex_lock+0x18/0x70
[ 716.701409] _put_opp_list_kref+0x28/0x50
[ 716.705373] _dev_pm_opp_find_and_remove_table+0x24/0x88
[ 716.710628] _dev_pm_opp_cpumask_remove_table+0x50/0xa0
[ 716.715796] dev_pm_opp_cpumask_remove_table+0x10/0x18
[ 716.720879] scpi_cpufreq_exit+0x40/0x50
[ 716.724758] cpufreq_offline+0x108/0x1e0
[ 716.728637] cpuhp_cpufreq_offline+0xc/0x18
[ 716.732775] cpuhp_invoke_callback+0x84/0x248
[ 716.737084] cpuhp_thread_fun+0xc4/0x148
[ 716.740963] smpboot_thread_fn+0x168/0x268
[ 716.745013] kthread+0x128/0x130
[ 716.748204] ret_from_fork+0x10/0x18
[ 716.751738] ---[ end trace 0c658e0103aac29d ]---
The test produces a script [2] that can be found at the end of this email.
Kernel:
7566ec393f41 ("Linux 4.20-rc7")
Config:
arm64 defconfig w/ CONFIG_MOUSE_PS2=n
Firmware:
ARM V2M_Juno Firmware v1.4.4
Build Date: Jul 26 2016
NOTICE: BL31: v1.3(debug):v1.3-567-g3fb340a2
NOTICE: BL31: Built : 18:52:35, Apr 25 2017
Cheers,
Valentin
---
[1]: https://github.com/ARM-software/lisa/blob/next/lisa/tests/kernel/hotplug/torture.py
[2]: random_cpuhp.sh
#!/bin/sh
set -e
while true
do
echo 0 > /sys/devices/system/cpu/cpu5/online
sleep 0.055
echo 0 > /sys/devices/system/cpu/cpu2/online
sleep 0.084
echo 0 > /sys/devices/system/cpu/cpu0/online
sleep 0.014
echo 1 > /sys/devices/system/cpu/cpu0/online
sleep 0.069
echo 1 > /sys/devices/system/cpu/cpu5/online
sleep 0.037
echo 1 > /sys/devices/system/cpu/cpu2/online
sleep 0.075
echo 0 > /sys/devices/system/cpu/cpu5/online
sleep 0.088
echo 0 > /sys/devices/system/cpu/cpu0/online
sleep 0.064
echo 0 > /sys/devices/system/cpu/cpu4/online
sleep 0.049
echo 0 > /sys/devices/system/cpu/cpu1/online
sleep 0.024
echo 0 > /sys/devices/system/cpu/cpu2/online
sleep 0.097
echo 1 > /sys/devices/system/cpu/cpu2/online
sleep 0.013
echo 1 > /sys/devices/system/cpu/cpu4/online
sleep 0.094
echo 0 > /sys/devices/system/cpu/cpu4/online
sleep 0.073
echo 1 > /sys/devices/system/cpu/cpu0/online
sleep 0.022
echo 1 > /sys/devices/system/cpu/cpu5/online
sleep 0.057
echo 0 > /sys/devices/system/cpu/cpu3/online
sleep 0.054
echo 1 > /sys/devices/system/cpu/cpu4/online
sleep 0.022
echo 1 > /sys/devices/system/cpu/cpu1/online
sleep 0.018
echo 1 > /sys/devices/system/cpu/cpu3/online
sleep 0.057
echo 0 > /sys/devices/system/cpu/cpu0/online
sleep 0.046
echo 0 > /sys/devices/system/cpu/cpu5/online
sleep 0.018
echo 0 > /sys/devices/system/cpu/cpu2/online
sleep 0.016
echo 0 > /sys/devices/system/cpu/cpu4/online
sleep 0.016
echo 0 > /sys/devices/system/cpu/cpu3/online
sleep 0.044
echo 1 > /sys/devices/system/cpu/cpu2/online
sleep 0.046
echo 1 > /sys/devices/system/cpu/cpu4/online
sleep 0.093
echo 0 > /sys/devices/system/cpu/cpu1/online
sleep 0.098
echo 0 > /sys/devices/system/cpu/cpu4/online
sleep 0.072
echo 1 > /sys/devices/system/cpu/cpu5/online
sleep 0.013
echo 1 > /sys/devices/system/cpu/cpu1/online
sleep 0.099
echo 0 > /sys/devices/system/cpu/cpu5/online
sleep 0.07
echo 1 > /sys/devices/system/cpu/cpu5/online
sleep 0.022
echo 0 > /sys/devices/system/cpu/cpu1/online
sleep 0.041
echo 0 > /sys/devices/system/cpu/cpu5/online
sleep 0.098
echo 1 > /sys/devices/system/cpu/cpu4/online
sleep 0.032
echo 0 > /sys/devices/system/cpu/cpu4/online
sleep 0.043
echo 1 > /sys/devices/system/cpu/cpu0/online
sleep 0.076
echo 0 > /sys/devices/system/cpu/cpu2/online
sleep 0.072
echo 1 > /sys/devices/system/cpu/cpu5/online
sleep 0.036
echo 1 > /sys/devices/system/cpu/cpu2/online
sleep 0.042
echo 0 > /sys/devices/system/cpu/cpu2/online
sleep 0.016
echo 0 > /sys/devices/system/cpu/cpu0/online
sleep 0.07
echo 1 > /sys/devices/system/cpu/cpu4/online
sleep 0.018
echo 1 > /sys/devices/system/cpu/cpu0/online
sleep 0.055
echo 1 > /sys/devices/system/cpu/cpu2/online
sleep 0.096
echo 1 > /sys/devices/system/cpu/cpu1/online
sleep 0.012
echo 1 > /sys/devices/system/cpu/cpu3/online
sleep 0.093
echo 0 > /sys/devices/system/cpu/cpu1/online
sleep 0.086
echo 1 > /sys/devices/system/cpu/cpu1/online
sleep 0.09
echo 0 > /sys/devices/system/cpu/cpu0/online
sleep 0.077
echo 1 > /sys/devices/system/cpu/cpu0/online
sleep 0.01
echo 0 > /sys/devices/system/cpu/cpu2/online
sleep 0.026
echo 0 > /sys/devices/system/cpu/cpu1/online
sleep 0.049
echo 0 > /sys/devices/system/cpu/cpu0/online
sleep 0.083
echo 0 > /sys/devices/system/cpu/cpu3/online
sleep 0.096
echo 1 > /sys/devices/system/cpu/cpu0/online
sleep 0.067
echo 1 > /sys/devices/system/cpu/cpu1/online
sleep 0.083
echo 0 > /sys/devices/system/cpu/cpu4/online
sleep 0.089
echo 1 > /sys/devices/system/cpu/cpu2/online
sleep 0.065
echo 1 > /sys/devices/system/cpu/cpu4/online
sleep 0.066
echo 1 > /sys/devices/system/cpu/cpu3/online
sleep 0.099
echo 0 > /sys/devices/system/cpu/cpu5/online
sleep 0.08
echo 1 > /sys/devices/system/cpu/cpu5/online
sleep 0.076
echo 0 > /sys/devices/system/cpu/cpu1/online
sleep 0.01
echo 0 > /sys/devices/system/cpu/cpu3/online
sleep 0.017
echo 0 > /sys/devices/system/cpu/cpu5/online
sleep 0.049
echo 0 > /sys/devices/system/cpu/cpu2/online
sleep 0.057
echo 1 > /sys/devices/system/cpu/cpu1/online
sleep 0.083
echo 0 > /sys/devices/system/cpu/cpu4/online
sleep 0.037
echo 0 > /sys/devices/system/cpu/cpu0/online
sleep 0.04
echo 1 > /sys/devices/system/cpu/cpu2/online
sleep 0.051
echo 0 > /sys/devices/system/cpu/cpu1/online
sleep 0.03
echo 1 > /sys/devices/system/cpu/cpu3/online
sleep 0.067
echo 0 > /sys/devices/system/cpu/cpu3/online
sleep 0.011
echo 1 > /sys/devices/system/cpu/cpu4/online
sleep 0.041
echo 1 > /sys/devices/system/cpu/cpu3/online
sleep 0.057
echo 0 > /sys/devices/system/cpu/cpu2/online
sleep 0.082
echo 0 > /sys/devices/system/cpu/cpu3/online
sleep 0.067
echo 1 > /sys/devices/system/cpu/cpu2/online
sleep 0.069
echo 1 > /sys/devices/system/cpu/cpu3/online
sleep 0.062
echo 0 > /sys/devices/system/cpu/cpu2/online
sleep 0.074
echo 1 > /sys/devices/system/cpu/cpu5/online
sleep 0.025
echo 0 > /sys/devices/system/cpu/cpu5/online
sleep 0.016
echo 1 > /sys/devices/system/cpu/cpu1/online
sleep 0.017
echo 0 > /sys/devices/system/cpu/cpu1/online
sleep 0.025
echo 0 > /sys/devices/system/cpu/cpu3/online
sleep 0.016
echo 1 > /sys/devices/system/cpu/cpu5/online
sleep 0.082
echo 1 > /sys/devices/system/cpu/cpu2/online
sleep 0.021
echo 0 > /sys/devices/system/cpu/cpu5/online
sleep 0.02
echo 0 > /sys/devices/system/cpu/cpu2/online
sleep 0.035
echo 1 > /sys/devices/system/cpu/cpu1/online
sleep 0.063
echo 0 > /sys/devices/system/cpu/cpu1/online
sleep 0.064
echo 1 > /sys/devices/system/cpu/cpu2/online
sleep 0.029
echo 1 > /sys/devices/system/cpu/cpu1/online
sleep 0.096
echo 1 > /sys/devices/system/cpu/cpu3/online
sleep 0.073
echo 1 > /sys/devices/system/cpu/cpu0/online
sleep 0.049
echo 1 > /sys/devices/system/cpu/cpu5/online
sleep 0.065
echo 0 > /sys/devices/system/cpu/cpu2/online
sleep 0.024
echo 1 > /sys/devices/system/cpu/cpu2/online
sleep 0.092
done &
LOOP_PID=$!
sleep 10
[ $(ps -q $LOOP_PID | wc -l) -gt 1 ] && kill -9 $LOOP_PID
set +e
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [BUG] dev_pm_opp refcount issue on Arm Juno r0 2018-12-20 15:27 [BUG] dev_pm_opp refcount issue on Arm Juno r0 Valentin Schneider @ 2019-01-03 7:05 ` Viresh Kumar 2019-01-03 10:33 ` Sudeep Holla 2019-01-04 9:44 ` [PATCH] cpufreq: scpi/scmi: Fix freeing of dynamic OPPs Viresh Kumar 1 sibling, 1 reply; 11+ messages in thread From: Viresh Kumar @ 2019-01-03 7:05 UTC (permalink / raw) To: Valentin Schneider, Sudeep Holla Cc: nm, Linux PM, sboyd, Rafael J. Wysocki, linux-kernel, Douglas Raillard, Quentin Perret, Dietmar Eggemann, LAK On 20-12-18, 15:27, Valentin Schneider wrote: > Hi, > > While running some hotplug torture test [1] on my Juno r0 I came across > the follow splat: > > [ 716.561862] ------------[ cut here ]------------ > [ 716.566451] refcount_t: underflow; use-after-free. > [ 716.571240] WARNING: CPU: 2 PID: 18 at lib/refcount.c:280 refcount_dec_not_one+0x9c/0xc0 > [ 716.579246] Modules linked in: > [ 716.582269] CPU: 2 PID: 18 Comm: cpuhp/2 Not tainted 4.20.0-rc7 #39 > [ 716.588469] Hardware name: ARM Juno development board (r0) (DT) > [ 716.594326] pstate: 40000005 (nZcv daif -PAN -UAO) > [ 716.599065] pc : refcount_dec_not_one+0x9c/0xc0 > [ 716.603546] lr : refcount_dec_not_one+0x9c/0xc0 > [ 716.608024] sp : ffff00000a063c70 > [ 716.611299] x29: ffff00000a063c70 x28: 0000000000000000 > [ 716.616555] x27: 0000000000000000 x26: 0000000000000002 > [ 716.621810] x25: ffff000009169000 x24: ffff000008f8e1b0 > [ 716.627065] x23: ffff000008ce0920 x22: 00000000ffffffff > [ 716.632319] x21: ffff000009169000 x20: ffff8009762a2664 > [ 716.637574] x19: ffff000009294a90 x18: 0000000000000400 > [ 716.642828] x17: 0000000000000000 x16: 0000000000000000 > [ 716.648082] x15: 0000000000000000 x14: 0000000000000400 > [ 716.653336] x13: 000000000000023f x12: 0000000000043705 > [ 716.658590] x11: 0000000000000108 x10: 0000000000000960 > [ 716.663844] x9 : ffff00000a063970 x8 : ffff800976943ec0 > [ 716.669098] x7 : 0000000000000000 x6 : ffff80097ff720b8 > [ 716.674353] x5 : ffff80097ff720b8 x4 : 0000000000000000 > [ 716.679607] x3 : ffff80097ff78e68 x2 : ffff80097ff720b8 > [ 716.684861] x1 : 6374e2a7925c1100 x0 : 0000000000000000 > [ 716.690115] Call trace: > [ 716.692532] refcount_dec_not_one+0x9c/0xc0 > [ 716.696669] refcount_dec_and_mutex_lock+0x18/0x70 > [ 716.701409] _put_opp_list_kref+0x28/0x50 > [ 716.705373] _dev_pm_opp_find_and_remove_table+0x24/0x88 > [ 716.710628] _dev_pm_opp_cpumask_remove_table+0x50/0xa0 > [ 716.715796] dev_pm_opp_cpumask_remove_table+0x10/0x18 > [ 716.720879] scpi_cpufreq_exit+0x40/0x50 > [ 716.724758] cpufreq_offline+0x108/0x1e0 > [ 716.728637] cpuhp_cpufreq_offline+0xc/0x18 This probably happened due to some of recent OPP core changes and I missed updating this platform (I updated mvebu though). The problem is completely different from what you logs show :) Please try the below patch. @Sudeep: Please help review it as well. -- viresh -------------------------8<------------------------- From f3913738618031e9d71ebf64461cee22909e6e20 Mon Sep 17 00:00:00 2001 Message-Id: <f3913738618031e9d71ebf64461cee22909e6e20.1546498940.git.viresh.kumar@linaro.org> From: Viresh Kumar <viresh.kumar@linaro.org> Date: Thu, 3 Jan 2019 12:28:26 +0530 Subject: [PATCH] cpufreq: scpi: Fix freeing of OPPs Since the commit 2a4eb7358aba ("OPP: Don't remove dynamic OPPs from _dev_pm_opp_remove_table()"), dynamically created OPP aren't automatically removed anymore by dev_pm_opp_cpumask_remove_table(). The OPPs for scpi cpufreq driver aren't getting freed currently, fix that by adding a new callback scpi_ops->remove_device_opps() which will remove those OPPs. Cc: 4.20 <stable@vger.kernel.org> # v4.20 Reported-by: Valentin Schneider <valentin.schneider@arm.com> Fixes: 2a4eb7358aba ("OPP: Don't remove dynamic OPPs from _dev_pm_opp_remove_table()") Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/cpufreq/scpi-cpufreq.c | 4 ++-- drivers/firmware/arm_scpi.c | 15 +++++++++++++++ include/linux/scpi_protocol.h | 1 + 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/drivers/cpufreq/scpi-cpufreq.c b/drivers/cpufreq/scpi-cpufreq.c index 87a98ec77773..1bfd168de0b2 100644 --- a/drivers/cpufreq/scpi-cpufreq.c +++ b/drivers/cpufreq/scpi-cpufreq.c @@ -177,7 +177,7 @@ static int scpi_cpufreq_init(struct cpufreq_policy *policy) out_free_priv: kfree(priv); out_free_opp: - dev_pm_opp_cpumask_remove_table(policy->cpus); + scpi_ops->remove_device_opps(cpu_dev); return ret; } @@ -190,7 +190,7 @@ static int scpi_cpufreq_exit(struct cpufreq_policy *policy) clk_put(priv->clk); dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table); kfree(priv); - dev_pm_opp_cpumask_remove_table(policy->related_cpus); + scpi_ops->remove_device_opps(priv->cpu_dev); return 0; } diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c index c7d06a36b23a..963f2ffbd820 100644 --- a/drivers/firmware/arm_scpi.c +++ b/drivers/firmware/arm_scpi.c @@ -716,6 +716,20 @@ static int scpi_dvfs_add_opps_to_device(struct device *dev) return 0; } +static void scpi_dvfs_remove_device_opps(struct device *dev) +{ + int idx; + struct scpi_opp *opp; + struct scpi_dvfs_info *info = scpi_dvfs_info(dev); + + /* We already added OPPs successfully, this data can't be invalid */ + if (WARN_ON(IS_ERR(info) || !info->opps)) + return; + + for (opp = info->opps, idx = 0; idx < info->count; idx++, opp++) + dev_pm_opp_remove(dev, opp->freq); +} + static int scpi_sensor_get_capability(u16 *sensors) { __le16 cap; @@ -799,6 +813,7 @@ static struct scpi_ops scpi_ops = { .device_domain_id = scpi_dev_domain_id, .get_transition_latency = scpi_dvfs_get_transition_latency, .add_opps_to_device = scpi_dvfs_add_opps_to_device, + .remove_device_opps = scpi_dvfs_remove_device_opps, .sensor_get_capability = scpi_sensor_get_capability, .sensor_get_info = scpi_sensor_get_info, .sensor_get_value = scpi_sensor_get_value, diff --git a/include/linux/scpi_protocol.h b/include/linux/scpi_protocol.h index 327d65663dbf..d020d517ecaa 100644 --- a/include/linux/scpi_protocol.h +++ b/include/linux/scpi_protocol.h @@ -70,6 +70,7 @@ struct scpi_ops { int (*device_domain_id)(struct device *); int (*get_transition_latency)(struct device *); int (*add_opps_to_device)(struct device *); + void (*remove_device_opps)(struct device *); int (*sensor_get_capability)(u16 *sensors); int (*sensor_get_info)(u16 sensor_id, struct scpi_sensor_info *); int (*sensor_get_value)(u16, u64 *); _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [BUG] dev_pm_opp refcount issue on Arm Juno r0 2019-01-03 7:05 ` Viresh Kumar @ 2019-01-03 10:33 ` Sudeep Holla 2019-01-03 10:38 ` Viresh Kumar 0 siblings, 1 reply; 11+ messages in thread From: Sudeep Holla @ 2019-01-03 10:33 UTC (permalink / raw) To: Viresh Kumar Cc: nm, Linux PM, sboyd, Rafael J. Wysocki, linux-kernel, Dietmar Eggemann, Quentin Perret, Sudeep Holla, Douglas Raillard, Valentin Schneider, LAK On Thu, Jan 03, 2019 at 12:35:14PM +0530, Viresh Kumar wrote: [...] > @Sudeep: Please help review it as well. > > -- > viresh > > -------------------------8<------------------------- > > From f3913738618031e9d71ebf64461cee22909e6e20 Mon Sep 17 00:00:00 2001 > Message-Id: <f3913738618031e9d71ebf64461cee22909e6e20.1546498940.git.viresh.kumar@linaro.org> > From: Viresh Kumar <viresh.kumar@linaro.org> > Date: Thu, 3 Jan 2019 12:28:26 +0530 > Subject: [PATCH] cpufreq: scpi: Fix freeing of OPPs > > Since the commit 2a4eb7358aba ("OPP: Don't remove dynamic OPPs from > _dev_pm_opp_remove_table()"), dynamically created OPP aren't > automatically removed anymore by dev_pm_opp_cpumask_remove_table(). > > The OPPs for scpi cpufreq driver aren't getting freed currently, fix > that by adding a new callback scpi_ops->remove_device_opps() which will > remove those OPPs. > > Cc: 4.20 <stable@vger.kernel.org> # v4.20 > Reported-by: Valentin Schneider <valentin.schneider@arm.com> > Fixes: 2a4eb7358aba ("OPP: Don't remove dynamic OPPs from _dev_pm_opp_remove_table()") > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/cpufreq/scpi-cpufreq.c | 4 ++-- > drivers/firmware/arm_scpi.c | 15 +++++++++++++++ > include/linux/scpi_protocol.h | 1 + > 3 files changed, 18 insertions(+), 2 deletions(-) > > diff --git a/drivers/cpufreq/scpi-cpufreq.c b/drivers/cpufreq/scpi-cpufreq.c > index 87a98ec77773..1bfd168de0b2 100644 > --- a/drivers/cpufreq/scpi-cpufreq.c > +++ b/drivers/cpufreq/scpi-cpufreq.c > @@ -177,7 +177,7 @@ static int scpi_cpufreq_init(struct cpufreq_policy *policy) > out_free_priv: > kfree(priv); > out_free_opp: > - dev_pm_opp_cpumask_remove_table(policy->cpus); > + scpi_ops->remove_device_opps(cpu_dev); > > return ret; > } > @@ -190,7 +190,7 @@ static int scpi_cpufreq_exit(struct cpufreq_policy *policy) > clk_put(priv->clk); > dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table); > kfree(priv); > - dev_pm_opp_cpumask_remove_table(policy->related_cpus); > + scpi_ops->remove_device_opps(priv->cpu_dev); > > return 0; > } > diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c > index c7d06a36b23a..963f2ffbd820 100644 > --- a/drivers/firmware/arm_scpi.c > +++ b/drivers/firmware/arm_scpi.c > @@ -716,6 +716,20 @@ static int scpi_dvfs_add_opps_to_device(struct device *dev) > return 0; > } > > +static void scpi_dvfs_remove_device_opps(struct device *dev) > +{ > + int idx; > + struct scpi_opp *opp; > + struct scpi_dvfs_info *info = scpi_dvfs_info(dev); > + > + /* We already added OPPs successfully, this data can't be invalid */ As you already state the checks are unnecessary, if we drop them we don't need to add any firmware specific callbacks. I am thinking if it makes sense to add a generic helper function to remove the OPPs from a device. If we have that any driver needing that can use it. The main reason I think helper is useful is that we need exactly same fix for SCMI driver too. Thoughts ? -- Regards, Sudeep _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [BUG] dev_pm_opp refcount issue on Arm Juno r0 2019-01-03 10:33 ` Sudeep Holla @ 2019-01-03 10:38 ` Viresh Kumar 2019-01-03 10:41 ` Sudeep Holla 0 siblings, 1 reply; 11+ messages in thread From: Viresh Kumar @ 2019-01-03 10:38 UTC (permalink / raw) To: Sudeep Holla Cc: nm, Linux PM, sboyd, Rafael J. Wysocki, linux-kernel, Dietmar Eggemann, Quentin Perret, Douglas Raillard, Valentin Schneider, LAK On 03-01-19, 10:33, Sudeep Holla wrote: > As you already state the checks are unnecessary, if we drop them we don't need > to add any firmware specific callbacks. I am thinking if it makes sense to > add a generic helper function to remove the OPPs from a device. If we have > that any driver needing that can use it. The main reason I think helper is > useful is that we need exactly same fix for SCMI driver too. > > Thoughts ? Okay, will do that and fix both the drivers as well. -- viresh _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [BUG] dev_pm_opp refcount issue on Arm Juno r0 2019-01-03 10:38 ` Viresh Kumar @ 2019-01-03 10:41 ` Sudeep Holla 0 siblings, 0 replies; 11+ messages in thread From: Sudeep Holla @ 2019-01-03 10:41 UTC (permalink / raw) To: Viresh Kumar Cc: nm, Linux PM, sboyd, Rafael J. Wysocki, linux-kernel, Dietmar Eggemann, Quentin Perret, Sudeep Holla, Douglas Raillard, Valentin Schneider, LAK On Thu, Jan 03, 2019 at 04:08:29PM +0530, Viresh Kumar wrote: > On 03-01-19, 10:33, Sudeep Holla wrote: > > As you already state the checks are unnecessary, if we drop them we don't need > > to add any firmware specific callbacks. I am thinking if it makes sense to > > add a generic helper function to remove the OPPs from a device. If we have > > that any driver needing that can use it. The main reason I think helper is > > useful is that we need exactly same fix for SCMI driver too. > > > > Thoughts ? > > Okay, will do that and fix both the drivers as well. > Thanks, happy to test here. -- Regards, Sudeep _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] cpufreq: scpi/scmi: Fix freeing of dynamic OPPs 2018-12-20 15:27 [BUG] dev_pm_opp refcount issue on Arm Juno r0 Valentin Schneider 2019-01-03 7:05 ` Viresh Kumar @ 2019-01-04 9:44 ` Viresh Kumar 2019-01-04 10:10 ` Rafael J. Wysocki ` (2 more replies) 1 sibling, 3 replies; 11+ messages in thread From: Viresh Kumar @ 2019-01-04 9:44 UTC (permalink / raw) To: Valentin Schneider, Sudeep Holla, Rafael J. Wysocki, Nishanth Menon, Stephen Boyd Cc: Vincent Guittot, linux-pm, Viresh Kumar, linux-kernel, 4 . 20, dietmar.eggemann, quentin.perret, Douglas.Raillard, linux-arm-kernel Since the commit 2a4eb7358aba ("OPP: Don't remove dynamic OPPs from _dev_pm_opp_remove_table()"), dynamically created OPP aren't automatically removed anymore by dev_pm_opp_cpumask_remove_table(). This affects the scpi and scmi cpufreq drivers which no longer free OPPs on failures or on invocations of the policy->exit() callback. Create a generic OPP helper dev_pm_opp_remove_all_dynamic() which can be called from these drivers instead of dev_pm_opp_cpumask_remove_table(). In dev_pm_opp_remove_all_dynamic(), we need to make sure that the opp_list isn't getting accessed simultaneously from other parts of the OPP core while the helper is freeing dynamic OPPs, i.e. we can't drop the opp_table->lock while traversing through the OPP list. And to accomplish that, this patch also creates _opp_kref_release_unlocked() which can be called from this new helper with the opp_table lock already held. Cc: 4.20 <stable@vger.kernel.org> # v4.20 Reported-by: Valentin Schneider <valentin.schneider@arm.com> Fixes: 2a4eb7358aba ("OPP: Don't remove dynamic OPPs from _dev_pm_opp_remove_table()") Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/cpufreq/scmi-cpufreq.c | 4 +-- drivers/cpufreq/scpi-cpufreq.c | 4 +-- drivers/opp/core.c | 63 +++++++++++++++++++++++++++++++--- include/linux/pm_opp.h | 5 +++ 4 files changed, 67 insertions(+), 9 deletions(-) diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c index 50b1551ba894..c2e66528f5ee 100644 --- a/drivers/cpufreq/scmi-cpufreq.c +++ b/drivers/cpufreq/scmi-cpufreq.c @@ -176,7 +176,7 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy) out_free_priv: kfree(priv); out_free_opp: - dev_pm_opp_cpumask_remove_table(policy->cpus); + dev_pm_opp_remove_all_dynamic(cpu_dev); return ret; } @@ -188,7 +188,7 @@ static int scmi_cpufreq_exit(struct cpufreq_policy *policy) cpufreq_cooling_unregister(priv->cdev); dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table); kfree(priv); - dev_pm_opp_cpumask_remove_table(policy->related_cpus); + dev_pm_opp_remove_all_dynamic(priv->cpu_dev); return 0; } diff --git a/drivers/cpufreq/scpi-cpufreq.c b/drivers/cpufreq/scpi-cpufreq.c index 87a98ec77773..99449738faa4 100644 --- a/drivers/cpufreq/scpi-cpufreq.c +++ b/drivers/cpufreq/scpi-cpufreq.c @@ -177,7 +177,7 @@ static int scpi_cpufreq_init(struct cpufreq_policy *policy) out_free_priv: kfree(priv); out_free_opp: - dev_pm_opp_cpumask_remove_table(policy->cpus); + dev_pm_opp_remove_all_dynamic(cpu_dev); return ret; } @@ -190,7 +190,7 @@ static int scpi_cpufreq_exit(struct cpufreq_policy *policy) clk_put(priv->clk); dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table); kfree(priv); - dev_pm_opp_cpumask_remove_table(policy->related_cpus); + dev_pm_opp_remove_all_dynamic(priv->cpu_dev); return 0; } diff --git a/drivers/opp/core.c b/drivers/opp/core.c index e5507add8f04..18f1639dbc4a 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -988,11 +988,9 @@ void _opp_free(struct dev_pm_opp *opp) kfree(opp); } -static void _opp_kref_release(struct kref *kref) +static void _opp_kref_release(struct dev_pm_opp *opp, + struct opp_table *opp_table) { - struct dev_pm_opp *opp = container_of(kref, struct dev_pm_opp, kref); - struct opp_table *opp_table = opp->opp_table; - /* * Notify the changes in the availability of the operable * frequency/voltage list. @@ -1002,7 +1000,22 @@ static void _opp_kref_release(struct kref *kref) opp_debug_remove_one(opp); list_del(&opp->node); kfree(opp); +} +static void _opp_kref_release_unlocked(struct kref *kref) +{ + struct dev_pm_opp *opp = container_of(kref, struct dev_pm_opp, kref); + struct opp_table *opp_table = opp->opp_table; + + _opp_kref_release(opp, opp_table); +} + +static void _opp_kref_release_locked(struct kref *kref) +{ + struct dev_pm_opp *opp = container_of(kref, struct dev_pm_opp, kref); + struct opp_table *opp_table = opp->opp_table; + + _opp_kref_release(opp, opp_table); mutex_unlock(&opp_table->lock); } @@ -1013,10 +1026,16 @@ void dev_pm_opp_get(struct dev_pm_opp *opp) void dev_pm_opp_put(struct dev_pm_opp *opp) { - kref_put_mutex(&opp->kref, _opp_kref_release, &opp->opp_table->lock); + kref_put_mutex(&opp->kref, _opp_kref_release_locked, + &opp->opp_table->lock); } EXPORT_SYMBOL_GPL(dev_pm_opp_put); +static void dev_pm_opp_put_unlocked(struct dev_pm_opp *opp) +{ + kref_put(&opp->kref, _opp_kref_release_unlocked); +} + /** * dev_pm_opp_remove() - Remove an OPP from OPP table * @dev: device for which we do this operation @@ -1060,6 +1079,40 @@ void dev_pm_opp_remove(struct device *dev, unsigned long freq) } EXPORT_SYMBOL_GPL(dev_pm_opp_remove); +/** + * dev_pm_opp_remove_all_dynamic() - Remove all dynamically created OPPs + * @dev: device for which we do this operation + * + * This function removes all dynamically created OPPs from the opp table. + */ +void dev_pm_opp_remove_all_dynamic(struct device *dev) +{ + struct opp_table *opp_table; + struct dev_pm_opp *opp, *temp; + int count = 0; + + opp_table = _find_opp_table(dev); + if (IS_ERR(opp_table)) + return; + + mutex_lock(&opp_table->lock); + list_for_each_entry_safe(opp, temp, &opp_table->opp_list, node) { + if (opp->dynamic) { + dev_pm_opp_put_unlocked(opp); + count++; + } + } + mutex_unlock(&opp_table->lock); + + /* Drop the references taken by dev_pm_opp_add() */ + while (count--) + dev_pm_opp_put_opp_table(opp_table); + + /* Drop the reference taken by _find_opp_table() */ + dev_pm_opp_put_opp_table(opp_table); +} +EXPORT_SYMBOL_GPL(dev_pm_opp_remove_all_dynamic); + struct dev_pm_opp *_opp_allocate(struct opp_table *table) { struct dev_pm_opp *opp; diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h index 0a2a88e5a383..b895f4e79868 100644 --- a/include/linux/pm_opp.h +++ b/include/linux/pm_opp.h @@ -108,6 +108,7 @@ void dev_pm_opp_put(struct dev_pm_opp *opp); int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt); void dev_pm_opp_remove(struct device *dev, unsigned long freq); +void dev_pm_opp_remove_all_dynamic(struct device *dev); int dev_pm_opp_enable(struct device *dev, unsigned long freq); @@ -217,6 +218,10 @@ static inline void dev_pm_opp_remove(struct device *dev, unsigned long freq) { } +static inline void dev_pm_opp_remove_all_dynamic(struct device *dev) +{ +} + static inline int dev_pm_opp_enable(struct device *dev, unsigned long freq) { return 0; -- 2.19.1.568.g152ad8e3369a _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] cpufreq: scpi/scmi: Fix freeing of dynamic OPPs 2019-01-04 9:44 ` [PATCH] cpufreq: scpi/scmi: Fix freeing of dynamic OPPs Viresh Kumar @ 2019-01-04 10:10 ` Rafael J. Wysocki 2019-01-04 10:16 ` Viresh Kumar 2019-01-04 10:40 ` Valentin Schneider 2019-01-04 11:01 ` Sudeep Holla 2 siblings, 1 reply; 11+ messages in thread From: Rafael J. Wysocki @ 2019-01-04 10:10 UTC (permalink / raw) To: Viresh Kumar Cc: Nishanth Menon, Vincent Guittot, Linux PM, Stephen Boyd, Rafael J. Wysocki, Linux Kernel Mailing List, 4 . 20, Dietmar Eggemann, Quentin Perret, Sudeep Holla, Douglas.Raillard, Valentin Schneider, Linux ARM On Fri, Jan 4, 2019 at 10:44 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > Since the commit 2a4eb7358aba ("OPP: Don't remove dynamic OPPs from > _dev_pm_opp_remove_table()"), dynamically created OPP aren't > automatically removed anymore by dev_pm_opp_cpumask_remove_table(). This > affects the scpi and scmi cpufreq drivers which no longer free OPPs on > failures or on invocations of the policy->exit() callback. > > Create a generic OPP helper dev_pm_opp_remove_all_dynamic() which can be > called from these drivers instead of dev_pm_opp_cpumask_remove_table(). > > In dev_pm_opp_remove_all_dynamic(), we need to make sure that the > opp_list isn't getting accessed simultaneously from other parts of the > OPP core while the helper is freeing dynamic OPPs, i.e. we can't drop > the opp_table->lock while traversing through the OPP list. And to > accomplish that, this patch also creates _opp_kref_release_unlocked() > which can be called from this new helper with the opp_table lock already > held. > > Cc: 4.20 <stable@vger.kernel.org> # v4.20 > Reported-by: Valentin Schneider <valentin.schneider@arm.com> > Fixes: 2a4eb7358aba ("OPP: Don't remove dynamic OPPs from _dev_pm_opp_remove_table()") > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> I guess I'll pick it up by hand. I'm assuming that you have tested it, have you? > --- > drivers/cpufreq/scmi-cpufreq.c | 4 +-- > drivers/cpufreq/scpi-cpufreq.c | 4 +-- > drivers/opp/core.c | 63 +++++++++++++++++++++++++++++++--- > include/linux/pm_opp.h | 5 +++ > 4 files changed, 67 insertions(+), 9 deletions(-) > > diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c > index 50b1551ba894..c2e66528f5ee 100644 > --- a/drivers/cpufreq/scmi-cpufreq.c > +++ b/drivers/cpufreq/scmi-cpufreq.c > @@ -176,7 +176,7 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy) > out_free_priv: > kfree(priv); > out_free_opp: > - dev_pm_opp_cpumask_remove_table(policy->cpus); > + dev_pm_opp_remove_all_dynamic(cpu_dev); > > return ret; > } > @@ -188,7 +188,7 @@ static int scmi_cpufreq_exit(struct cpufreq_policy *policy) > cpufreq_cooling_unregister(priv->cdev); > dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table); > kfree(priv); > - dev_pm_opp_cpumask_remove_table(policy->related_cpus); > + dev_pm_opp_remove_all_dynamic(priv->cpu_dev); > > return 0; > } > diff --git a/drivers/cpufreq/scpi-cpufreq.c b/drivers/cpufreq/scpi-cpufreq.c > index 87a98ec77773..99449738faa4 100644 > --- a/drivers/cpufreq/scpi-cpufreq.c > +++ b/drivers/cpufreq/scpi-cpufreq.c > @@ -177,7 +177,7 @@ static int scpi_cpufreq_init(struct cpufreq_policy *policy) > out_free_priv: > kfree(priv); > out_free_opp: > - dev_pm_opp_cpumask_remove_table(policy->cpus); > + dev_pm_opp_remove_all_dynamic(cpu_dev); > > return ret; > } > @@ -190,7 +190,7 @@ static int scpi_cpufreq_exit(struct cpufreq_policy *policy) > clk_put(priv->clk); > dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table); > kfree(priv); > - dev_pm_opp_cpumask_remove_table(policy->related_cpus); > + dev_pm_opp_remove_all_dynamic(priv->cpu_dev); > > return 0; > } > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > index e5507add8f04..18f1639dbc4a 100644 > --- a/drivers/opp/core.c > +++ b/drivers/opp/core.c > @@ -988,11 +988,9 @@ void _opp_free(struct dev_pm_opp *opp) > kfree(opp); > } > > -static void _opp_kref_release(struct kref *kref) > +static void _opp_kref_release(struct dev_pm_opp *opp, > + struct opp_table *opp_table) > { > - struct dev_pm_opp *opp = container_of(kref, struct dev_pm_opp, kref); > - struct opp_table *opp_table = opp->opp_table; > - > /* > * Notify the changes in the availability of the operable > * frequency/voltage list. > @@ -1002,7 +1000,22 @@ static void _opp_kref_release(struct kref *kref) > opp_debug_remove_one(opp); > list_del(&opp->node); > kfree(opp); > +} > > +static void _opp_kref_release_unlocked(struct kref *kref) > +{ > + struct dev_pm_opp *opp = container_of(kref, struct dev_pm_opp, kref); > + struct opp_table *opp_table = opp->opp_table; > + > + _opp_kref_release(opp, opp_table); > +} > + > +static void _opp_kref_release_locked(struct kref *kref) > +{ > + struct dev_pm_opp *opp = container_of(kref, struct dev_pm_opp, kref); > + struct opp_table *opp_table = opp->opp_table; > + > + _opp_kref_release(opp, opp_table); > mutex_unlock(&opp_table->lock); > } > > @@ -1013,10 +1026,16 @@ void dev_pm_opp_get(struct dev_pm_opp *opp) > > void dev_pm_opp_put(struct dev_pm_opp *opp) > { > - kref_put_mutex(&opp->kref, _opp_kref_release, &opp->opp_table->lock); > + kref_put_mutex(&opp->kref, _opp_kref_release_locked, > + &opp->opp_table->lock); > } > EXPORT_SYMBOL_GPL(dev_pm_opp_put); > > +static void dev_pm_opp_put_unlocked(struct dev_pm_opp *opp) > +{ > + kref_put(&opp->kref, _opp_kref_release_unlocked); > +} > + > /** > * dev_pm_opp_remove() - Remove an OPP from OPP table > * @dev: device for which we do this operation > @@ -1060,6 +1079,40 @@ void dev_pm_opp_remove(struct device *dev, unsigned long freq) > } > EXPORT_SYMBOL_GPL(dev_pm_opp_remove); > > +/** > + * dev_pm_opp_remove_all_dynamic() - Remove all dynamically created OPPs > + * @dev: device for which we do this operation > + * > + * This function removes all dynamically created OPPs from the opp table. > + */ > +void dev_pm_opp_remove_all_dynamic(struct device *dev) > +{ > + struct opp_table *opp_table; > + struct dev_pm_opp *opp, *temp; > + int count = 0; > + > + opp_table = _find_opp_table(dev); > + if (IS_ERR(opp_table)) > + return; > + > + mutex_lock(&opp_table->lock); > + list_for_each_entry_safe(opp, temp, &opp_table->opp_list, node) { > + if (opp->dynamic) { > + dev_pm_opp_put_unlocked(opp); > + count++; > + } > + } > + mutex_unlock(&opp_table->lock); > + > + /* Drop the references taken by dev_pm_opp_add() */ > + while (count--) > + dev_pm_opp_put_opp_table(opp_table); > + > + /* Drop the reference taken by _find_opp_table() */ > + dev_pm_opp_put_opp_table(opp_table); > +} > +EXPORT_SYMBOL_GPL(dev_pm_opp_remove_all_dynamic); > + > struct dev_pm_opp *_opp_allocate(struct opp_table *table) > { > struct dev_pm_opp *opp; > diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h > index 0a2a88e5a383..b895f4e79868 100644 > --- a/include/linux/pm_opp.h > +++ b/include/linux/pm_opp.h > @@ -108,6 +108,7 @@ void dev_pm_opp_put(struct dev_pm_opp *opp); > int dev_pm_opp_add(struct device *dev, unsigned long freq, > unsigned long u_volt); > void dev_pm_opp_remove(struct device *dev, unsigned long freq); > +void dev_pm_opp_remove_all_dynamic(struct device *dev); > > int dev_pm_opp_enable(struct device *dev, unsigned long freq); > > @@ -217,6 +218,10 @@ static inline void dev_pm_opp_remove(struct device *dev, unsigned long freq) > { > } > > +static inline void dev_pm_opp_remove_all_dynamic(struct device *dev) > +{ > +} > + > static inline int dev_pm_opp_enable(struct device *dev, unsigned long freq) > { > return 0; > -- > 2.19.1.568.g152ad8e3369a > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] cpufreq: scpi/scmi: Fix freeing of dynamic OPPs 2019-01-04 10:10 ` Rafael J. Wysocki @ 2019-01-04 10:16 ` Viresh Kumar 0 siblings, 0 replies; 11+ messages in thread From: Viresh Kumar @ 2019-01-04 10:16 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Nishanth Menon, Vincent Guittot, Linux PM, Stephen Boyd, Rafael J. Wysocki, Linux Kernel Mailing List, 4 . 20, Dietmar Eggemann, Quentin Perret, Sudeep Holla, Douglas.Raillard, Valentin Schneider, Linux ARM On 04-01-19, 11:10, Rafael J. Wysocki wrote: > On Fri, Jan 4, 2019 at 10:44 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > Since the commit 2a4eb7358aba ("OPP: Don't remove dynamic OPPs from > > _dev_pm_opp_remove_table()"), dynamically created OPP aren't > > automatically removed anymore by dev_pm_opp_cpumask_remove_table(). This > > affects the scpi and scmi cpufreq drivers which no longer free OPPs on > > failures or on invocations of the policy->exit() callback. > > > > Create a generic OPP helper dev_pm_opp_remove_all_dynamic() which can be > > called from these drivers instead of dev_pm_opp_cpumask_remove_table(). > > > > In dev_pm_opp_remove_all_dynamic(), we need to make sure that the > > opp_list isn't getting accessed simultaneously from other parts of the > > OPP core while the helper is freeing dynamic OPPs, i.e. we can't drop > > the opp_table->lock while traversing through the OPP list. And to > > accomplish that, this patch also creates _opp_kref_release_unlocked() > > which can be called from this new helper with the opp_table lock already > > held. > > > > Cc: 4.20 <stable@vger.kernel.org> # v4.20 > > Reported-by: Valentin Schneider <valentin.schneider@arm.com> > > Fixes: 2a4eb7358aba ("OPP: Don't remove dynamic OPPs from _dev_pm_opp_remove_table()") > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > I guess I'll pick it up by hand. Sure. > I'm assuming that you have tested it, have you? Yes, but I had to fake few dynamic OPPs and ignore the static ones coming from DT. Lets wait for Sudeep or Valentin to test this, who have real hardware to fix with this patch. -- viresh _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] cpufreq: scpi/scmi: Fix freeing of dynamic OPPs 2019-01-04 9:44 ` [PATCH] cpufreq: scpi/scmi: Fix freeing of dynamic OPPs Viresh Kumar 2019-01-04 10:10 ` Rafael J. Wysocki @ 2019-01-04 10:40 ` Valentin Schneider 2019-01-04 11:01 ` Sudeep Holla 2 siblings, 0 replies; 11+ messages in thread From: Valentin Schneider @ 2019-01-04 10:40 UTC (permalink / raw) To: Viresh Kumar, Sudeep Holla, Rafael J. Wysocki, Nishanth Menon, Stephen Boyd Cc: Vincent Guittot, linux-pm, linux-kernel, 4 . 20, dietmar.eggemann, quentin.perret, Douglas.Raillard, linux-arm-kernel Hi, On 04/01/2019 09:44, Viresh Kumar wrote: > Since the commit 2a4eb7358aba ("OPP: Don't remove dynamic OPPs from > _dev_pm_opp_remove_table()"), dynamically created OPP aren't > automatically removed anymore by dev_pm_opp_cpumask_remove_table(). This > affects the scpi and scmi cpufreq drivers which no longer free OPPs on > failures or on invocations of the policy->exit() callback. > > Create a generic OPP helper dev_pm_opp_remove_all_dynamic() which can be > called from these drivers instead of dev_pm_opp_cpumask_remove_table(). > > In dev_pm_opp_remove_all_dynamic(), we need to make sure that the > opp_list isn't getting accessed simultaneously from other parts of the > OPP core while the helper is freeing dynamic OPPs, i.e. we can't drop > the opp_table->lock while traversing through the OPP list. And to > accomplish that, this patch also creates _opp_kref_release_unlocked() > which can be called from this new helper with the opp_table lock already > held. > > Cc: 4.20 <stable@vger.kernel.org> # v4.20 > Reported-by: Valentin Schneider <valentin.schneider@arm.com> > Fixes: 2a4eb7358aba ("OPP: Don't remove dynamic OPPs from _dev_pm_opp_remove_table()") > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> Gave it a spin on my Juno, and I don't see the refcount warning anymore so: Tested-by: Valentin Schneider <valentin.schneider@arm.com> Thanks for having a look at this. [...] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] cpufreq: scpi/scmi: Fix freeing of dynamic OPPs 2019-01-04 9:44 ` [PATCH] cpufreq: scpi/scmi: Fix freeing of dynamic OPPs Viresh Kumar 2019-01-04 10:10 ` Rafael J. Wysocki 2019-01-04 10:40 ` Valentin Schneider @ 2019-01-04 11:01 ` Sudeep Holla 2019-01-11 10:37 ` Rafael J. Wysocki 2 siblings, 1 reply; 11+ messages in thread From: Sudeep Holla @ 2019-01-04 11:01 UTC (permalink / raw) To: Viresh Kumar Cc: Nishanth Menon, Vincent Guittot, linux-pm, Stephen Boyd, Rafael J. Wysocki, linux-kernel, 4 . 20, dietmar.eggemann, quentin.perret, Sudeep Holla, Douglas.Raillard, Valentin Schneider, linux-arm-kernel On Fri, Jan 04, 2019 at 03:14:33PM +0530, Viresh Kumar wrote: > Since the commit 2a4eb7358aba ("OPP: Don't remove dynamic OPPs from > _dev_pm_opp_remove_table()"), dynamically created OPP aren't > automatically removed anymore by dev_pm_opp_cpumask_remove_table(). This > affects the scpi and scmi cpufreq drivers which no longer free OPPs on > failures or on invocations of the policy->exit() callback. > > Create a generic OPP helper dev_pm_opp_remove_all_dynamic() which can be > called from these drivers instead of dev_pm_opp_cpumask_remove_table(). > > In dev_pm_opp_remove_all_dynamic(), we need to make sure that the > opp_list isn't getting accessed simultaneously from other parts of the > OPP core while the helper is freeing dynamic OPPs, i.e. we can't drop > the opp_table->lock while traversing through the OPP list. And to > accomplish that, this patch also creates _opp_kref_release_unlocked() > which can be called from this new helper with the opp_table lock already > held. > I did test it but since I couldn't reproduce the original issue, my tested-by is not that worth. Anyways the changes look fine to me. Reviewed-by: Sudeep Holla <sudeep.holla@arm.com> -- Regards, Sudeep _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] cpufreq: scpi/scmi: Fix freeing of dynamic OPPs 2019-01-04 11:01 ` Sudeep Holla @ 2019-01-11 10:37 ` Rafael J. Wysocki 0 siblings, 0 replies; 11+ messages in thread From: Rafael J. Wysocki @ 2019-01-11 10:37 UTC (permalink / raw) To: Sudeep Holla, Viresh Kumar Cc: Nishanth Menon, Vincent Guittot, linux-pm, Stephen Boyd, linux-kernel, 4 . 20, dietmar.eggemann, quentin.perret, Douglas.Raillard, Valentin Schneider, linux-arm-kernel On Friday, January 4, 2019 12:01:42 PM CET Sudeep Holla wrote: > On Fri, Jan 04, 2019 at 03:14:33PM +0530, Viresh Kumar wrote: > > Since the commit 2a4eb7358aba ("OPP: Don't remove dynamic OPPs from > > _dev_pm_opp_remove_table()"), dynamically created OPP aren't > > automatically removed anymore by dev_pm_opp_cpumask_remove_table(). This > > affects the scpi and scmi cpufreq drivers which no longer free OPPs on > > failures or on invocations of the policy->exit() callback. > > > > Create a generic OPP helper dev_pm_opp_remove_all_dynamic() which can be > > called from these drivers instead of dev_pm_opp_cpumask_remove_table(). > > > > In dev_pm_opp_remove_all_dynamic(), we need to make sure that the > > opp_list isn't getting accessed simultaneously from other parts of the > > OPP core while the helper is freeing dynamic OPPs, i.e. we can't drop > > the opp_table->lock while traversing through the OPP list. And to > > accomplish that, this patch also creates _opp_kref_release_unlocked() > > which can be called from this new helper with the opp_table lock already > > held. > > > > I did test it but since I couldn't reproduce the original issue, my > tested-by is not that worth. Anyways the changes look fine to me. > > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com> Applied and pushed to Linus, thanks! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-01-11 10:38 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-12-20 15:27 [BUG] dev_pm_opp refcount issue on Arm Juno r0 Valentin Schneider 2019-01-03 7:05 ` Viresh Kumar 2019-01-03 10:33 ` Sudeep Holla 2019-01-03 10:38 ` Viresh Kumar 2019-01-03 10:41 ` Sudeep Holla 2019-01-04 9:44 ` [PATCH] cpufreq: scpi/scmi: Fix freeing of dynamic OPPs Viresh Kumar 2019-01-04 10:10 ` Rafael J. Wysocki 2019-01-04 10:16 ` Viresh Kumar 2019-01-04 10:40 ` Valentin Schneider 2019-01-04 11:01 ` Sudeep Holla 2019-01-11 10:37 ` Rafael J. Wysocki
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).