From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932177AbcLGNX6 (ORCPT ); Wed, 7 Dec 2016 08:23:58 -0500 Received: from mailout1.samsung.com ([203.254.224.24]:58705 "EHLO mailout1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751372AbcLGNX4 (ORCPT ); Wed, 7 Dec 2016 08:23:56 -0500 X-AuditID: cbfee61a-f79916d0000062de-f4-58480d69e53f Message-id: <58480D69.9030801@samsung.com> Date: Wed, 07 Dec 2016 22:23:53 +0900 From: Chanwoo Choi Organization: Samsung Electronics User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-version: 1.0 To: Viresh Kumar , Rafael Wysocki , Kevin Hilman , Tony Lindgren , Viresh Kumar , Nishanth Menon , Stephen Boyd , Peter De Schrijver , Prashant Gaikwad , Stephen Warren , Thierry Reding , Alexandre Courbot , Kukjin Kim , Krzysztof Kozlowski , Javier Martinez Canillas , MyungJoo Ham , Kyungmin Park , Amit Daniel Kachhap , Javi Merino , Zhang Rui , Eduardo Valentin Cc: linaro-kernel@lists.linaro.org, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, Vincent Guittot Subject: Re: [PATCH 07/12] PM / OPP: Update OPP users to put reference References: <059cb0d754593a1bbe82e67990729678ce8cef78.1481106919.git.viresh.kumar@linaro.org> In-reply-to: <059cb0d754593a1bbe82e67990729678ce8cef78.1481106919.git.viresh.kumar@linaro.org> Content-type: text/plain; charset=UTF-8 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrKKsWRmVeSWpSXmKPExsVy+t9jAd0sXo8Ig813WCx+vZvJbjH/yjVW i3OvHrFY/Fs1l83izds1TBb9j18zWzzd/JjJ4vz5DewWZ5vesFu8P/SM2eLyrjlsFp97jzBa 3G5cAVT84yyTxaS1Uxktbs5Pszhz+hKrxZOHfWwWP850s1i8OtjGYvFz1zwWi/1XvCw6jnxj ttj41cNi84NjbA6SHt++TmLxuNzXy+Sxc9Zddo/Fe14yeWxa1cnmcefaHjaP2/8eM3v0Nr9j 89jSD5TdcrWdxaNvyypGj+M3tjN5fN4k57FxbmgAX5SbTUZqYkpqkUJqXnJ+SmZeuq1SaIib roWSQl5ibqqtUoSub0iQkkJZYk4pkGdkgAYcnAPcg5X07RLcMl6fPsZccM6sYtb5icwNjAe0 uxg5OSQETCSuzV7OBmGLSVy4tx7I5uIQEljKKLHt3HIWCOcBo8TlU+3MIFW8AloSz7b0snYx cnCwCKhK3LiVABJmAwrvf3EDbBC/gKLE1R+PGUFKRAUiJLpPVEJ0Ckr8mHwPbKSIwDk2iamr n7KCOMwCXYwSvx4dYAepEhZwkzh27TojxOJHjBLPPm5gBUlwCiRI3D97FmwDs4C6xKR5i5gh bHmJzWveMk9gFJyFZMssJGWzkJQtYGRexSiRWpBcUJyUnmuYl1quV5yYW1yal66XnJ+7iRGc hJ5J7WA8uMv9EKMAB6MSD2/BJbcIIdbEsuLK3EOMEhzMSiK8zZweEUK8KYmVValF+fFFpTmp xYcYTYEhMpFZSjQ5H5gg80riDU3MTcyNDSzMLS1NjJTEeRtnPwsXEkhPLEnNTk0tSC2C6WPi 4JRqYMzZe0dI2ORq2y2Th1Kn95mbMz6Od914yzpTP7Oj58f/oOfLDu6MuCKrdqvLclOd+AGb JzcjLWyVt/VcFi378UxDM+7fztdhZzZcMl8prCOveX9+k8nhDIvD/1LXzrwn9u26VPjMJc4v 3ndPvVh5nG0RW6Lc/FXVDr3a59ZM9mwNiv2lo/olx0WJpTgj0VCLuag4EQCv5kmoWAMAAA== X-MTR: 20000000000000000@CPGS Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Viresh, [snip] > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index b0de42972b74..89add0d7c017 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -111,18 +111,16 @@ static void devfreq_set_freq_table(struct devfreq *devfreq) > return; > } > > - rcu_read_lock(); > for (i = 0, freq = 0; i < profile->max_state; i++, freq++) { > opp = dev_pm_opp_find_freq_ceil(devfreq->dev.parent, &freq); > + dev_pm_opp_put(opp); I think that the dev_pm_opp_put(opp) should be called after if statement If dev_pm_opp_find_freq_ceil() return error, I think the calling of dev_pm_opp_put(opp) is not necessary. > if (IS_ERR(opp)) { > devm_kfree(devfreq->dev.parent, profile->freq_table); > profile->max_state = 0; > - rcu_read_unlock(); > return; > } > profile->freq_table[i] = freq; > } > - rcu_read_unlock(); > } > > /** > @@ -1107,9 +1105,9 @@ static ssize_t available_frequencies_show(struct device *d, > ssize_t count = 0; > unsigned long freq = 0; > > - rcu_read_lock(); > do { > opp = dev_pm_opp_find_freq_ceil(dev, &freq); > + dev_pm_opp_put(opp); ditto. > if (IS_ERR(opp)) > break; > > @@ -1117,7 +1115,6 @@ static ssize_t available_frequencies_show(struct device *d, > "%lu ", freq); > freq++; > } while (1); > - rcu_read_unlock(); > > /* Truncate the trailing space */ > if (count) > @@ -1219,11 +1216,8 @@ subsys_initcall(devfreq_init); > * @freq: The frequency given to target function > * @flags: Flags handed from devfreq framework. > * > - * Locking: This function must be called under rcu_read_lock(). opp is a rcu > - * protected pointer. The reason for the same is that the opp pointer which is > - * returned will remain valid for use with opp_get_{voltage, freq} only while > - * under the locked area. The pointer returned must be used prior to unlocking > - * with rcu_read_unlock() to maintain the integrity of the pointer. > + * The callers are required to call dev_pm_opp_put() for the returned OPP after > + * use. > */ > struct dev_pm_opp *devfreq_recommended_opp(struct device *dev, > unsigned long *freq, > diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c > index a8ed7792ece2..49ce38cef460 100644 > --- a/drivers/devfreq/exynos-bus.c > +++ b/drivers/devfreq/exynos-bus.c > @@ -103,18 +103,17 @@ static int exynos_bus_target(struct device *dev, unsigned long *freq, u32 flags) > int ret = 0; > > /* Get new opp-bus instance according to new bus clock */ > - rcu_read_lock(); > new_opp = devfreq_recommended_opp(dev, freq, flags); > if (IS_ERR(new_opp)) { > dev_err(dev, "failed to get recommended opp instance\n"); > - rcu_read_unlock(); > 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; > - rcu_read_unlock(); > > if (old_freq == new_freq) > return 0; > @@ -214,17 +213,16 @@ static int exynos_bus_passive_target(struct device *dev, unsigned long *freq, > int ret = 0; > > /* Get new opp-bus instance according to new bus clock */ > - rcu_read_lock(); > new_opp = devfreq_recommended_opp(dev, freq, flags); > if (IS_ERR(new_opp)) { > dev_err(dev, "failed to get recommended opp instance\n"); > - rcu_read_unlock(); > 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; > - rcu_read_unlock(); > > if (old_freq == new_freq) > return 0; > @@ -358,16 +356,14 @@ static int exynos_bus_parse_of(struct device_node *np, > > rate = clk_get_rate(bus->clk); > > - rcu_read_lock(); > opp = devfreq_recommended_opp(dev, &rate, 0); > if (IS_ERR(opp)) { > dev_err(dev, "failed to find dev_pm_opp\n"); > - rcu_read_unlock(); > ret = PTR_ERR(opp); > goto err_opp; > } > bus->curr_freq = dev_pm_opp_get_freq(opp); > - rcu_read_unlock(); > + dev_pm_opp_put(opp); > > return 0; > > diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c > index 9ef46e2592c4..671a1e0afc6e 100644 > --- a/drivers/devfreq/governor_passive.c > +++ b/drivers/devfreq/governor_passive.c > @@ -59,9 +59,9 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq, > * list of parent device. Because in this case, *freq is temporary > * value which is decided by ondemand governor. > */ > - rcu_read_lock(); > opp = devfreq_recommended_opp(parent_devfreq->dev.parent, freq, 0); > - rcu_read_unlock(); > + dev_pm_opp_put(opp); ditto. > + > if (IS_ERR(opp)) { > ret = PTR_ERR(opp); > goto out; > diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c > index 27d2f349b53c..40a2499730fc 100644 > --- a/drivers/devfreq/rk3399_dmc.c > +++ b/drivers/devfreq/rk3399_dmc.c > @@ -91,17 +91,13 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq, > unsigned long target_volt, target_rate; > int err; > > - rcu_read_lock(); > opp = devfreq_recommended_opp(dev, freq, flags); > - if (IS_ERR(opp)) { > - rcu_read_unlock(); > + if (IS_ERR(opp)) > return PTR_ERR(opp); > - } > > target_rate = dev_pm_opp_get_freq(opp); > target_volt = dev_pm_opp_get_voltage(opp); > - > - rcu_read_unlock(); > + dev_pm_opp_put(opp); > > if (dmcfreq->rate == target_rate) > return 0; > @@ -422,15 +418,13 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev) > > data->rate = clk_get_rate(data->dmc_clk); > > - rcu_read_lock(); > opp = devfreq_recommended_opp(dev, &data->rate, 0); > - if (IS_ERR(opp)) { > - rcu_read_unlock(); > + if (IS_ERR(opp)) > return PTR_ERR(opp); > - } > + > data->rate = dev_pm_opp_get_freq(opp); > data->volt = dev_pm_opp_get_voltage(opp); > - rcu_read_unlock(); > + dev_pm_opp_put(opp); > > rk3399_devfreq_dmc_profile.initial_freq = data->rate; > > diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c > index fe9dce0245bf..214fff96fa4a 100644 > --- a/drivers/devfreq/tegra-devfreq.c > +++ b/drivers/devfreq/tegra-devfreq.c > @@ -487,15 +487,13 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq, > struct dev_pm_opp *opp; > unsigned long rate = *freq * KHZ; > > - rcu_read_lock(); > opp = devfreq_recommended_opp(dev, &rate, flags); > if (IS_ERR(opp)) { > - rcu_read_unlock(); > dev_err(dev, "Failed to find opp for %lu KHz\n", *freq); > return PTR_ERR(opp); > } > rate = dev_pm_opp_get_freq(opp); > - rcu_read_unlock(); > + dev_pm_opp_put(opp); > > clk_set_min_rate(tegra->emc_clock, rate); > clk_set_rate(tegra->emc_clock, 0); [snip] Best Regards, Chanwoo Choi