From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1161458Ab2JXV5i (ORCPT ); Wed, 24 Oct 2012 17:57:38 -0400 Received: from ogre.sisk.pl ([193.178.161.156]:35627 "EHLO ogre.sisk.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1161399Ab2JXV5d (ORCPT ); Wed, 24 Oct 2012 17:57:33 -0400 From: "Rafael J. Wysocki" To: Nishanth Menon Cc: linux-pm , MyungJoo Ham , Kyungmin Park , Kevin Hilman , linux-kernel@vger.kernel.org Subject: Re: [PATCH V2] PM / OPP: predictable fail results for opp_find* functions Date: Thu, 25 Oct 2012 00:01:28 +0200 Message-ID: <2369752.l80tmojpSQ@vostro.rjw.lan> User-Agent: KMail/4.8.5 (Linux/3.6.2-6-desktop; KDE/4.8.5; x86_64; ; ) In-Reply-To: <1350912799-17323-1-git-send-email-nm@ti.com> References: <1350912799-17323-1-git-send-email-nm@ti.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="utf-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Monday 22 of October 2012 08:33:19 Nishanth Menon wrote: > Currently the opp_find* functions return -ENODEV when: > a) it cant find a device (e.g. request for an OPP search on device > which was not registered) > b) When it cant find a match for the search strategy used > > This makes life a little in-efficient for users such as devfreq > to make reasonable judgement before switching search strategies. > > So, standardize the return results as following: > -EINVAL for bad pointer parameters > -ENODEV when device cannot be found > -ERANGE when search fails > > This has the following benefit for devfreq implementation: > The search fails when an unregistered device pointer is provided. > This is a trigger to change the search direction and search for > a better fit, however, if we cannot differentiate between a valid > search range failure Vs an unregistered device, second search goes > through the same fail return condition. This can be avoided by > appropriate handling of error return code. > > With this change, we also fix devfreq for the improved search > strategy with updated error code. > > Cc: MyungJoo Ham > Cc: Kyungmin Park > Cc: "Rafael J. Wysocki" > Cc: Kevin Hilman > Cc: linux-pm@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > > Reviewed-by: Kevin Hilman > Acked-by: MyungJoo Ham > Signed-off-by: Nishanth Menon Applied to linux-pm.git/linux-next as v3.8 material. Thanks, Rafael > --- > V2: > * commit message change - updated frome example code, to verbose description > * picked up Acked-by and Reviewed-by tags from V1. > * rebased to v3.7-rc2 > V1: https://patchwork.kernel.org/patch/1582111/ > > drivers/base/power/opp.c | 27 +++++++++++++++++++-------- > drivers/devfreq/devfreq.c | 4 ++-- > 2 files changed, 21 insertions(+), 10 deletions(-) > > diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c > index 4730a58..99d0573 100644 > --- a/drivers/base/power/opp.c > +++ b/drivers/base/power/opp.c > @@ -234,7 +234,10 @@ EXPORT_SYMBOL(opp_get_opp_count); > * > * Searches for exact match in the opp list and returns pointer to the matching > * opp if found, else returns ERR_PTR in case of error and should be handled > - * using IS_ERR. > + * using IS_ERR. Error return values can be: > + * EINVAL: for bad pointer > + * ERANGE: no match found for search > + * ENODEV: if device not found in list of registered devices > * > * Note: available is a modifier for the search. if available=true, then the > * match is for exact matching frequency and is available in the stored OPP > @@ -253,7 +256,7 @@ struct opp *opp_find_freq_exact(struct device *dev, unsigned long freq, > bool available) > { > struct device_opp *dev_opp; > - struct opp *temp_opp, *opp = ERR_PTR(-ENODEV); > + struct opp *temp_opp, *opp = ERR_PTR(-ERANGE); > > dev_opp = find_device_opp(dev); > if (IS_ERR(dev_opp)) { > @@ -283,7 +286,11 @@ EXPORT_SYMBOL(opp_find_freq_exact); > * for a device. > * > * Returns matching *opp and refreshes *freq accordingly, else returns > - * ERR_PTR in case of error and should be handled using IS_ERR. > + * ERR_PTR in case of error and should be handled using IS_ERR. Error return > + * values can be: > + * EINVAL: for bad pointer > + * ERANGE: no match found for search > + * ENODEV: if device not found in list of registered devices > * > * 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 > @@ -294,7 +301,7 @@ EXPORT_SYMBOL(opp_find_freq_exact); > struct opp *opp_find_freq_ceil(struct device *dev, unsigned long *freq) > { > struct device_opp *dev_opp; > - struct opp *temp_opp, *opp = ERR_PTR(-ENODEV); > + struct opp *temp_opp, *opp = ERR_PTR(-ERANGE); > > if (!dev || !freq) { > dev_err(dev, "%s: Invalid argument freq=%p\n", __func__, freq); > @@ -303,7 +310,7 @@ struct opp *opp_find_freq_ceil(struct device *dev, unsigned long *freq) > > dev_opp = find_device_opp(dev); > if (IS_ERR(dev_opp)) > - return opp; > + return ERR_CAST(dev_opp); > > list_for_each_entry_rcu(temp_opp, &dev_opp->opp_list, node) { > if (temp_opp->available && temp_opp->rate >= *freq) { > @@ -326,7 +333,11 @@ EXPORT_SYMBOL(opp_find_freq_ceil); > * for a device. > * > * Returns matching *opp and refreshes *freq accordingly, else returns > - * ERR_PTR in case of error and should be handled using IS_ERR. > + * ERR_PTR in case of error and should be handled using IS_ERR. Error return > + * values can be: > + * EINVAL: for bad pointer > + * ERANGE: no match found for search > + * ENODEV: if device not found in list of registered devices > * > * 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 > @@ -337,7 +348,7 @@ EXPORT_SYMBOL(opp_find_freq_ceil); > struct opp *opp_find_freq_floor(struct device *dev, unsigned long *freq) > { > struct device_opp *dev_opp; > - struct opp *temp_opp, *opp = ERR_PTR(-ENODEV); > + struct opp *temp_opp, *opp = ERR_PTR(-ERANGE); > > if (!dev || !freq) { > dev_err(dev, "%s: Invalid argument freq=%p\n", __func__, freq); > @@ -346,7 +357,7 @@ struct opp *opp_find_freq_floor(struct device *dev, unsigned long *freq) > > dev_opp = find_device_opp(dev); > if (IS_ERR(dev_opp)) > - return opp; > + return ERR_CAST(dev_opp); > > list_for_each_entry_rcu(temp_opp, &dev_opp->opp_list, node) { > if (temp_opp->available) { > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index 0920d75..bee1c29 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -656,14 +656,14 @@ struct opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq, > opp = opp_find_freq_floor(dev, freq); > > /* If not available, use the closest opp */ > - if (opp == ERR_PTR(-ENODEV)) > + if (opp == ERR_PTR(-ERANGE)) > opp = opp_find_freq_ceil(dev, freq); > } else { > /* The freq is an lower bound. opp should be higher */ > opp = opp_find_freq_ceil(dev, freq); > > /* If not available, use the closest opp */ > - if (opp == ERR_PTR(-ENODEV)) > + if (opp == ERR_PTR(-ERANGE)) > opp = opp_find_freq_floor(dev, freq); > } > > -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.