Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755452AbcJYDhm (ORCPT ); Mon, 24 Oct 2016 23:37:42 -0400 Received: from mail-pf0-f178.google.com ([209.85.192.178]:35757 "EHLO mail-pf0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752105AbcJYDhk (ORCPT ); Mon, 24 Oct 2016 23:37:40 -0400 Date: Tue, 25 Oct 2016 09:07:36 +0530 From: Viresh Kumar To: Stephen Boyd Cc: Rafael Wysocki , nm@ti.com, Viresh Kumar , linaro-kernel@lists.linaro.org, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, Vincent Guittot , robh@kernel.org, d-gerlach@ti.com, broonie@kernel.org Subject: Re: [PATCH V2 2/8] PM / OPP: Don't use OPP structure outside of rcu protected section Message-ID: <20161025033736.GA9162@vireshk-i7> References: <7a0754e64236db2a977c6a6402c222fea6dc866d.1476952750.git.viresh.kumar@linaro.org> <20161024225238.GR26139@codeaurora.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161024225238.GR26139@codeaurora.org> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1499 Lines: 48 On 24-10-16, 15:52, Stephen Boyd wrote: > On 10/20, Viresh Kumar wrote: > > The OPP structure must not be used out of the rcu protected section. > > Cache the values to be used in separate variables instead. > > > > Signed-off-by: Viresh Kumar > > Was this found by visual inspection or through some static > checker? Just curious. Visual inspection :) > > @@ -633,6 +634,14 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) > > return ret; > > } > > > > + if (IS_ERR(old_opp)) { > > + old_u_volt = 0; > > + } else { > > + old_u_volt = old_opp->u_volt; > > + old_u_volt_min = old_opp->u_volt_min; > > + old_u_volt_max = old_opp->u_volt_max; > > + } > > + > > u_volt = opp->u_volt; > > u_volt_min = opp->u_volt_min; > > u_volt_max = opp->u_volt_max; > > @@ -677,9 +686,10 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) > > __func__, old_freq); > > restore_voltage: > > /* This shouldn't harm even if the voltages weren't updated earlier */ > > - if (!IS_ERR(old_opp)) > > - _set_opp_voltage(dev, reg, old_opp->u_volt, > > - old_opp->u_volt_min, old_opp->u_volt_max); > > + if (old_u_volt) { > > What if old_u_volt == 0 is valid? How can that be valid ? > We could have another variable > like 'valid' or something that we use to figure out if we should > set values instead. Then this isn't a potential pitfall. I can do that but just wanted to know if we need that or not. -- viresh