Re: [PATCH 03/10] OPP: Allocate dev_opp from _add_device_opp()

[Stephen Boyd <sboyd@codeaurora.org>]

On 06/15/2015 04:57 AM, Viresh Kumar wrote:
> There is no need to complicate _opp_add_dynamic() with allocation of
> dev_opp as well. Allocate it from _add_device_opp() instead.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/base/power/opp.c | 39 +++++++++++++++++++--------------------
>  1 file changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> index 7895fdd64192..4b646f36f252 100644
> --- a/drivers/base/power/opp.c
> +++ b/drivers/base/power/opp.c
> @@ -408,11 +408,11 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
>  EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
>  
>  /**
> - * _add_device_opp() - Allocate a new device OPP table
> + * _add_device_opp() - Returns device OPP table

Find device OPP table, or allocate a new one?

>   * @dev:	device for which we do this operation
>   *
> - * New device node which uses OPPs - used when multiple devices with OPP tables
> - * are maintained.
> + * It tries to find an existing table first, if it couldn't find one, it
> + * allocates a new OPP table and returns that.
>   *
>   * Return: valid device_opp pointer if success, else NULL.
>   */
> @@ -420,6 +420,11 @@ static struct device_opp *_add_device_opp(struct device *dev)
>  {
>  	struct device_opp *dev_opp;
>  
> +	/* Check for existing list for 'dev' first */
> +	dev_opp = _find_device_opp(dev);
> +	if (!IS_ERR(dev_opp))
> +		return dev_opp;
> +
>  	/*
>  	 * Allocate a new device OPP table. In the infrequent case where a new
>  	 * device is needed to be added, we pay this penalty.
> @@ -575,8 +580,8 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_remove);
>  static int _opp_add_dynamic(struct device *dev, unsigned long freq,
>  			    long u_volt, bool dynamic)
>  {
> -	struct device_opp *dev_opp = NULL;
> -	struct dev_pm_opp *opp, *new_opp;
> +	struct device_opp *dev_opp;
> +	struct dev_pm_opp *opp = NULL, *new_opp;

Hm...

>  	struct list_head *head;
>  	int ret;
>  
> @@ -592,19 +597,11 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq,
>  	new_opp->rate = freq;
>  	new_opp->u_volt = u_volt;
>  	new_opp->available = true;
> -	new_opp->dynamic = dynamic;
>  
> -	/* Check for existing list for 'dev' */
> -	dev_opp = _find_device_opp(dev);
> -	if (IS_ERR(dev_opp)) {
> -		dev_opp = _add_device_opp(dev);
> -		if (!dev_opp) {
> -			ret = -ENOMEM;
> -			goto free_opp;
> -		}
> -
> -		head = &dev_opp->opp_list;
> -		goto list_add;
> +	dev_opp = _add_device_opp(dev);
> +	if (!dev_opp) {
> +		ret = -ENOMEM;
> +		goto free_opp;
>  	}
>  
>  	/*
> @@ -620,17 +617,17 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq,
>  	}
>  
>  	/* Duplicate OPPs ? */
> -	if (new_opp->rate == opp->rate) {
> +	if (opp && new_opp->rate == opp->rate) {

Isn't opp always non-NULL at this point? Maybe this if statement should
be moved into the list_for_each_entry_rcu() loop.

>  		ret = opp->available && new_opp->u_volt == opp->u_volt ?
>  			0 : -EEXIST;
>  
>  		dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n",
>  			 __func__, opp->rate, opp->u_volt, opp->available,
>  			 new_opp->rate, new_opp->u_volt, new_opp->available);
> -		goto free_opp;
> +		goto remove_dev_opp;
>  	}
>  
> -list_add:
> +	new_opp->dynamic = dynamic;
>  	new_opp->dev_opp = dev_opp;
>  	list_add_rcu(&new_opp->node, head);
>  	mutex_unlock(&dev_opp_list_lock);
> @@ -642,6 +639,8 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq,
>  	srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_ADD, new_opp);
>  	return 0;
>  
> +remove_dev_opp:
> +	_remove_device_opp(dev_opp);

Doesn't this just return early because dev_opp has something in it?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project