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

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?