[PATCH 0/8] PM / Domains: Re-structure code for adding/removing devices to genpd

[PATCH 0/8] PM / Domains: Re-structure code for adding/removing devices to genpd

[Ulf Hansson]

While adding error handling of genpd's ->attach_dev() callback, I realized that
we also had a need to re-structure some of the code which deals with
adding/removing devices to genpd. Especially the APIs, __pm_genpd_add_device()
and pm_genpd_remove_device() deserved some attention.

Patch 1 -> 4, can be considered as more simple cleanups and should not impact
the behavior for current clients using the APIs. 

Patch 5, eliminates a mutex for generic_pm_domain_data by re-order the
initialization/cleanup of dev_pm_qos notifiers.

Patch 6, move some code around to fix a potenial memory leakage of a struct
pm_subsys_data.

Patch 7, code restructuring which impacts locking behavior while adding/removing
devices. Should improve code readability and decrease critical regions of
holding locks.

Patch 8, Adds error handling of genpd's ->attach_dev() callback


Ulf Hansson (8):
  PM / Domains: Rename __pm_genpd_alloc|free_dev_data()
  PM / Domains: Remove reference counting for the generic_pm_domain_data
  PM / Domains: Don't allow an existing generic_pm_domain_data
  PM / Domains: Don't check for an existing device when adding a new
  PM / Domains: Eliminate the mutex for the generic_pm_domain_data
  PM / Domains: Free pm_subsys_data in error path in
    __pm_genpd_add_device()
  PM / Domains: Re-order initialization of generic_pm_domain_data
  PM / Domains: Handle errors from genpd's ->attach_dev() callback

 drivers/base/power/domain.c | 137 +++++++++++++++++++++-----------------------
 include/linux/pm_domain.h   |   2 -
 2 files changed, 65 insertions(+), 74 deletions(-)

-- 
1.9.1

[PATCH 1/8] PM / Domains: Rename __pm_genpd_alloc|free_dev_data()

[Ulf Hansson]

In a step to get consistent names of functions in genpd, rename
the internal __pm_genpd_alloc|free_dev_data() into
gendp_alloc|free_dev_data().

As discussed on the linux-pm list, let's move towards the following
name rules:

Internal functions:
genpd_*
_genpd_*
__genpd_*

External functions:
pm_genpd_*
_pm_genpd_*
__pm_genpd_*

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index c5280f2..f9e7df5 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1384,7 +1384,7 @@ EXPORT_SYMBOL_GPL(pm_genpd_syscore_poweron);
 
 #endif /* CONFIG_PM_SLEEP */
 
-static struct generic_pm_domain_data *__pm_genpd_alloc_dev_data(struct device *dev)
+static struct generic_pm_domain_data *genpd_alloc_dev_data(struct device *dev)
 {
 	struct generic_pm_domain_data *gpd_data;
 
@@ -1398,8 +1398,8 @@ static struct generic_pm_domain_data *__pm_genpd_alloc_dev_data(struct device *d
 	return gpd_data;
 }
 
-static void __pm_genpd_free_dev_data(struct device *dev,
-				     struct generic_pm_domain_data *gpd_data)
+static void genpd_free_dev_data(struct device *dev,
+				struct generic_pm_domain_data *gpd_data)
 {
 	dev_pm_qos_remove_notifier(dev, &gpd_data->nb);
 	kfree(gpd_data);
@@ -1423,7 +1423,7 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 	if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev))
 		return -EINVAL;
 
-	gpd_data_new = __pm_genpd_alloc_dev_data(dev);
+	gpd_data_new = genpd_alloc_dev_data(dev);
 	if (!gpd_data_new)
 		return -ENOMEM;
 
@@ -1477,7 +1477,7 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 	genpd_release_lock(genpd);
 
 	if (gpd_data != gpd_data_new)
-		__pm_genpd_free_dev_data(dev, gpd_data_new);
+		genpd_free_dev_data(dev, gpd_data_new);
 
 	return ret;
 }
@@ -1548,7 +1548,7 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 
 	dev_pm_put_subsys_data(dev);
 	if (remove)
-		__pm_genpd_free_dev_data(dev, gpd_data);
+		genpd_free_dev_data(dev, gpd_data);
 
 	return 0;
 
-- 
1.9.1

[PATCH 1/8] PM / Domains: Rename __pm_genpd_alloc|free_dev_data()

[Pavel Machek]

On Tue 2015-01-27 21:13:38, Ulf Hansson wrote:
> In a step to get consistent names of functions in genpd, rename
> the internal __pm_genpd_alloc|free_dev_data() into
> gendp_alloc|free_dev_data().
> 
> As discussed on the linux-pm list, let's move towards the following
> name rules:
> 
> Internal functions:
> genpd_*
> _genpd_*
> __genpd_*
> 
> External functions:
> pm_genpd_*
> _pm_genpd_*
> __pm_genpd_*
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Acked-by: Pavel Machek <pavel@ucw.cz>


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH 2/8] PM / Domains: Remove reference counting for the generic_pm_domain_data

[Ulf Hansson]

The reference counting was needed when genpd supported PM domain device
callbacks. Since this option has been removed, let's also remove the
reference counting of the struct generic_pm_domain_data.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 10 ++--------
 include/linux/pm_domain.h   |  1 -
 2 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index f9e7df5..351df5b 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1456,7 +1456,6 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 		gpd_data = gpd_data_new;
 		dev->power.subsys_data->domain_data = &gpd_data->base;
 	}
-	gpd_data->refcount++;
 	if (td)
 		gpd_data->td = *td;
 
@@ -1504,7 +1503,6 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 {
 	struct generic_pm_domain_data *gpd_data;
 	struct pm_domain_data *pdd;
-	bool remove = false;
 	int ret = 0;
 
 	dev_dbg(dev, "%s()\n", __func__);
@@ -1533,10 +1531,7 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 	pdd = dev->power.subsys_data->domain_data;
 	list_del_init(&pdd->list_node);
 	gpd_data = to_gpd_data(pdd);
-	if (--gpd_data->refcount == 0) {
-		dev->power.subsys_data->domain_data = NULL;
-		remove = true;
-	}
+	dev->power.subsys_data->domain_data = NULL;
 
 	spin_unlock_irq(&dev->power.lock);
 
@@ -1547,8 +1542,7 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 	genpd_release_lock(genpd);
 
 	dev_pm_put_subsys_data(dev);
-	if (remove)
-		genpd_free_dev_data(dev, gpd_data);
+	genpd_free_dev_data(dev, gpd_data);
 
 	return 0;
 
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index ed60776..e160a0b 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -114,7 +114,6 @@ struct generic_pm_domain_data {
 	struct gpd_timing_data td;
 	struct notifier_block nb;
 	struct mutex lock;
-	unsigned int refcount;
 	int need_restore;
 };
 
-- 
1.9.1

[PATCH 2/8] PM / Domains: Remove reference counting for the generic_pm_domain_data

[Pavel Machek]

On Tue 2015-01-27 21:13:39, Ulf Hansson wrote:
> The reference counting was needed when genpd supported PM domain device
> callbacks. Since this option has been removed, let's also remove the
> reference counting of the struct generic_pm_domain_data.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Acked-by: Pavel Machek <pavel@ucw.cz>
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH 3/8] PM / Domains: Don't allow an existing generic_pm_domain_data

[Ulf Hansson]

When adding a device to a genpd, a struct generic_pm_domain_data is
allocated per device.

Verify that there are no existing generic_pm_domain_data for the device
we are about to add, since that tells us it has already been added to a
genpd.

When genpd supported PM domain device callbacks, this was a valid
scenario. Now it isn't so let's return an error code.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 351df5b..76eb0c3 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1444,26 +1444,30 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 	if (ret)
 		goto out;
 
-	genpd->device_count++;
-	genpd->max_off_time_changed = true;
-
 	spin_lock_irq(&dev->power.lock);
 
-	dev->pm_domain = &genpd->domain;
 	if (dev->power.subsys_data->domain_data) {
-		gpd_data = to_gpd_data(dev->power.subsys_data->domain_data);
-	} else {
-		gpd_data = gpd_data_new;
-		dev->power.subsys_data->domain_data = &gpd_data->base;
+		spin_unlock_irq(&dev->power.lock);
+		ret = -EINVAL;
+		goto out;
 	}
+
+	gpd_data = gpd_data_new;
+	dev->power.subsys_data->domain_data = &gpd_data->base;
+
 	if (td)
 		gpd_data->td = *td;
 
+	dev->pm_domain = &genpd->domain;
+
 	spin_unlock_irq(&dev->power.lock);
 
 	if (genpd->attach_dev)
 		genpd->attach_dev(genpd, dev);
 
+	genpd->device_count++;
+	genpd->max_off_time_changed = true;
+
 	mutex_lock(&gpd_data->lock);
 	gpd_data->base.dev = dev;
 	list_add_tail(&gpd_data->base.list_node, &genpd->dev_list);
-- 
1.9.1

[PATCH 4/8] PM / Domains: Don't check for an existing device when adding a new

[Ulf Hansson]

When adding a device to a genpd, we no longer need to walk genpd's list
of existing devices to verify it hasn't already been added.

Instead we can now rely on the verification of not allowing existing
generic_pm_domain_data for a device, since that has the same meaning.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 76eb0c3..88198ba 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1415,7 +1415,6 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 			  struct gpd_timing_data *td)
 {
 	struct generic_pm_domain_data *gpd_data_new, *gpd_data = NULL;
-	struct pm_domain_data *pdd;
 	int ret = 0;
 
 	dev_dbg(dev, "%s()\n", __func__);
@@ -1434,12 +1433,6 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 		goto out;
 	}
 
-	list_for_each_entry(pdd, &genpd->dev_list, list_node)
-		if (pdd->dev == dev) {
-			ret = -EINVAL;
-			goto out;
-		}
-
 	ret = dev_pm_get_subsys_data(dev);
 	if (ret)
 		goto out;
-- 
1.9.1

[PATCH 5/8] PM / Domains: Eliminate the mutex for the generic_pm_domain_data

[Ulf Hansson]

While adding devices to their PM domains, dev_pm_qos_add_notifier() was
invoked while allocating the generic_pm_domain_data for the device.

Since the generic_pm_domain_data's device pointer will be assigned
after allocation, the ->genpd_dev_pm_qos_notifier() callback could be
called prior having a valid pointer to the device. Similar scenario
existed while removing a device from a genpd.

To cope with these scenarios a mutex was used to protect the pointer to
the device.

By re-order the sequence for when dev_pm_qos_add|remove_notifier() are
invoked, we make sure the ->genpd_dev_pm_qos_notifier() callback are
always called with a valid device pointer available.

In this way, we eliminate the need for protecting the pointer and thus
we can remove the mutex from the struct generic_pm_domain_data.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 37 ++++++++++++++-----------------------
 include/linux/pm_domain.h   |  1 -
 2 files changed, 14 insertions(+), 24 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 88198ba..1f026c1 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -344,14 +344,7 @@ static int genpd_dev_pm_qos_notifier(struct notifier_block *nb,
 	struct device *dev;
 
 	gpd_data = container_of(nb, struct generic_pm_domain_data, nb);
-
-	mutex_lock(&gpd_data->lock);
 	dev = gpd_data->base.dev;
-	if (!dev) {
-		mutex_unlock(&gpd_data->lock);
-		return NOTIFY_DONE;
-	}
-	mutex_unlock(&gpd_data->lock);
 
 	for (;;) {
 		struct generic_pm_domain *genpd;
@@ -1392,16 +1385,12 @@ static struct generic_pm_domain_data *genpd_alloc_dev_data(struct device *dev)
 	if (!gpd_data)
 		return NULL;
 
-	mutex_init(&gpd_data->lock);
-	gpd_data->nb.notifier_call = genpd_dev_pm_qos_notifier;
-	dev_pm_qos_add_notifier(dev, &gpd_data->nb);
 	return gpd_data;
 }
 
 static void genpd_free_dev_data(struct device *dev,
 				struct generic_pm_domain_data *gpd_data)
 {
-	dev_pm_qos_remove_notifier(dev, &gpd_data->nb);
 	kfree(gpd_data);
 }
 
@@ -1414,7 +1403,7 @@ static void genpd_free_dev_data(struct device *dev,
 int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 			  struct gpd_timing_data *td)
 {
-	struct generic_pm_domain_data *gpd_data_new, *gpd_data = NULL;
+	struct generic_pm_domain_data *gpd_data;
 	int ret = 0;
 
 	dev_dbg(dev, "%s()\n", __func__);
@@ -1422,8 +1411,8 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 	if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev))
 		return -EINVAL;
 
-	gpd_data_new = genpd_alloc_dev_data(dev);
-	if (!gpd_data_new)
+	gpd_data = genpd_alloc_dev_data(dev);
+	if (!gpd_data)
 		return -ENOMEM;
 
 	genpd_acquire_lock(genpd);
@@ -1445,7 +1434,6 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 		goto out;
 	}
 
-	gpd_data = gpd_data_new;
 	dev->power.subsys_data->domain_data = &gpd_data->base;
 
 	if (td)
@@ -1461,19 +1449,20 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 	genpd->device_count++;
 	genpd->max_off_time_changed = true;
 
-	mutex_lock(&gpd_data->lock);
 	gpd_data->base.dev = dev;
 	list_add_tail(&gpd_data->base.list_node, &genpd->dev_list);
 	gpd_data->need_restore = -1;
 	gpd_data->td.constraint_changed = true;
 	gpd_data->td.effective_constraint_ns = -1;
-	mutex_unlock(&gpd_data->lock);
+	gpd_data->nb.notifier_call = genpd_dev_pm_qos_notifier;
 
  out:
 	genpd_release_lock(genpd);
 
-	if (gpd_data != gpd_data_new)
-		genpd_free_dev_data(dev, gpd_data_new);
+	if (ret)
+		genpd_free_dev_data(dev, gpd_data);
+	else
+		dev_pm_qos_add_notifier(dev, &gpd_data->nb);
 
 	return ret;
 }
@@ -1509,6 +1498,11 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 	    ||  pd_to_genpd(dev->pm_domain) != genpd)
 		return -EINVAL;
 
+	/* The above validation also means we have existing domain_data. */
+	pdd = dev->power.subsys_data->domain_data;
+	gpd_data = to_gpd_data(pdd);
+	dev_pm_qos_remove_notifier(dev, &gpd_data->nb);
+
 	genpd_acquire_lock(genpd);
 
 	if (genpd->prepared_count > 0) {
@@ -1525,16 +1519,12 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 	spin_lock_irq(&dev->power.lock);
 
 	dev->pm_domain = NULL;
-	pdd = dev->power.subsys_data->domain_data;
 	list_del_init(&pdd->list_node);
-	gpd_data = to_gpd_data(pdd);
 	dev->power.subsys_data->domain_data = NULL;
 
 	spin_unlock_irq(&dev->power.lock);
 
-	mutex_lock(&gpd_data->lock);
 	pdd->dev = NULL;
-	mutex_unlock(&gpd_data->lock);
 
 	genpd_release_lock(genpd);
 
@@ -1545,6 +1535,7 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 
  out:
 	genpd_release_lock(genpd);
+	dev_pm_qos_add_notifier(dev, &gpd_data->nb);
 
 	return ret;
 }
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index e160a0b..080e778 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -113,7 +113,6 @@ struct generic_pm_domain_data {
 	struct pm_domain_data base;
 	struct gpd_timing_data td;
 	struct notifier_block nb;
-	struct mutex lock;
 	int need_restore;
 };
 
-- 
1.9.1

[PATCH 6/8] PM / Domains: Free pm_subsys_data in error path in __pm_genpd_add_device()

[Ulf Hansson]

The error path in __pm_genpd_add_device() didn't decrease the reference
to the struct pm_subsys_data.

Let's move the calls to dev_pm_get|put_subsys_data() into
genpd_alloc|free_dev_data() to fix this issue and thus prevent a
potential memory leakage.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 1f026c1..3bd342f 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1380,18 +1380,30 @@ EXPORT_SYMBOL_GPL(pm_genpd_syscore_poweron);
 static struct generic_pm_domain_data *genpd_alloc_dev_data(struct device *dev)
 {
 	struct generic_pm_domain_data *gpd_data;
+	int ret;
+
+	ret = dev_pm_get_subsys_data(dev);
+	if (ret)
+		return ERR_PTR(ret);
 
 	gpd_data = kzalloc(sizeof(*gpd_data), GFP_KERNEL);
-	if (!gpd_data)
-		return NULL;
+	if (!gpd_data) {
+		ret = -ENOMEM;
+		goto err_put;
+	}
 
 	return gpd_data;
+
+ err_put:
+	dev_pm_put_subsys_data(dev);
+	return ERR_PTR(ret);
 }
 
 static void genpd_free_dev_data(struct device *dev,
 				struct generic_pm_domain_data *gpd_data)
 {
 	kfree(gpd_data);
+	dev_pm_put_subsys_data(dev);
 }
 
 /**
@@ -1412,8 +1424,8 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 		return -EINVAL;
 
 	gpd_data = genpd_alloc_dev_data(dev);
-	if (!gpd_data)
-		return -ENOMEM;
+	if (IS_ERR(gpd_data))
+		return PTR_ERR(gpd_data);
 
 	genpd_acquire_lock(genpd);
 
@@ -1422,10 +1434,6 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 		goto out;
 	}
 
-	ret = dev_pm_get_subsys_data(dev);
-	if (ret)
-		goto out;
-
 	spin_lock_irq(&dev->power.lock);
 
 	if (dev->power.subsys_data->domain_data) {
@@ -1528,7 +1536,6 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 
 	genpd_release_lock(genpd);
 
-	dev_pm_put_subsys_data(dev);
 	genpd_free_dev_data(dev, gpd_data);
 
 	return 0;
-- 
1.9.1

[PATCH 7/8] PM / Domains: Re-order initialization of generic_pm_domain_data

[Ulf Hansson]

Move the initialization of the struct generic_pm_domain_data into
genpd_alloc_dev_data(), including the assignment of the device's
->pm_domain() callback. Make corresponding changes to
genpd_free_dev_data().

These changes will make the related code more readable. It will also
decrease the critical regions for where genpd's mutex is being held and
for where the device's power related spinlock is being held.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 67 +++++++++++++++++++++++----------------------
 1 file changed, 35 insertions(+), 32 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 3bd342f..0fd5ee1 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1377,7 +1377,9 @@ EXPORT_SYMBOL_GPL(pm_genpd_syscore_poweron);
 
 #endif /* CONFIG_PM_SLEEP */
 
-static struct generic_pm_domain_data *genpd_alloc_dev_data(struct device *dev)
+static struct generic_pm_domain_data *genpd_alloc_dev_data(struct device *dev,
+					struct generic_pm_domain *genpd,
+					struct gpd_timing_data *td)
 {
 	struct generic_pm_domain_data *gpd_data;
 	int ret;
@@ -1392,8 +1394,32 @@ static struct generic_pm_domain_data *genpd_alloc_dev_data(struct device *dev)
 		goto err_put;
 	}
 
+	if (td)
+		gpd_data->td = *td;
+
+	gpd_data->base.dev = dev;
+	gpd_data->need_restore = -1;
+	gpd_data->td.constraint_changed = true;
+	gpd_data->td.effective_constraint_ns = -1;
+	gpd_data->nb.notifier_call = genpd_dev_pm_qos_notifier;
+
+	spin_lock_irq(&dev->power.lock);
+
+	if (dev->power.subsys_data->domain_data) {
+		ret = -EINVAL;
+		goto err_free;
+	}
+
+	dev->power.subsys_data->domain_data = &gpd_data->base;
+	dev->pm_domain = &genpd->domain;
+
+	spin_unlock_irq(&dev->power.lock);
+
 	return gpd_data;
 
+ err_free:
+	spin_unlock_irq(&dev->power.lock);
+	kfree(gpd_data);
  err_put:
 	dev_pm_put_subsys_data(dev);
 	return ERR_PTR(ret);
@@ -1402,6 +1428,13 @@ static struct generic_pm_domain_data *genpd_alloc_dev_data(struct device *dev)
 static void genpd_free_dev_data(struct device *dev,
 				struct generic_pm_domain_data *gpd_data)
 {
+	spin_lock_irq(&dev->power.lock);
+
+	dev->pm_domain = NULL;
+	dev->power.subsys_data->domain_data = NULL;
+
+	spin_unlock_irq(&dev->power.lock);
+
 	kfree(gpd_data);
 	dev_pm_put_subsys_data(dev);
 }
@@ -1423,7 +1456,7 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 	if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev))
 		return -EINVAL;
 
-	gpd_data = genpd_alloc_dev_data(dev);
+	gpd_data = genpd_alloc_dev_data(dev, genpd, td);
 	if (IS_ERR(gpd_data))
 		return PTR_ERR(gpd_data);
 
@@ -1434,35 +1467,13 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 		goto out;
 	}
 
-	spin_lock_irq(&dev->power.lock);
-
-	if (dev->power.subsys_data->domain_data) {
-		spin_unlock_irq(&dev->power.lock);
-		ret = -EINVAL;
-		goto out;
-	}
-
-	dev->power.subsys_data->domain_data = &gpd_data->base;
-
-	if (td)
-		gpd_data->td = *td;
-
-	dev->pm_domain = &genpd->domain;
-
-	spin_unlock_irq(&dev->power.lock);
-
 	if (genpd->attach_dev)
 		genpd->attach_dev(genpd, dev);
 
 	genpd->device_count++;
 	genpd->max_off_time_changed = true;
 
-	gpd_data->base.dev = dev;
 	list_add_tail(&gpd_data->base.list_node, &genpd->dev_list);
-	gpd_data->need_restore = -1;
-	gpd_data->td.constraint_changed = true;
-	gpd_data->td.effective_constraint_ns = -1;
-	gpd_data->nb.notifier_call = genpd_dev_pm_qos_notifier;
 
  out:
 	genpd_release_lock(genpd);
@@ -1524,15 +1535,7 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 	if (genpd->detach_dev)
 		genpd->detach_dev(genpd, dev);
 
-	spin_lock_irq(&dev->power.lock);
-
-	dev->pm_domain = NULL;
 	list_del_init(&pdd->list_node);
-	dev->power.subsys_data->domain_data = NULL;
-
-	spin_unlock_irq(&dev->power.lock);
-
-	pdd->dev = NULL;
 
 	genpd_release_lock(genpd);
 
-- 
1.9.1

[PATCH 8/8] PM / Domains: Handle errors from genpd's ->attach_dev() callback

[Ulf Hansson]

The optional genpd's ->attach_dev() callback is invoked from
__pm_genpd_add_device(). Let's add error handling from the response
from this callback and propagate the error code.

When __pm_genpd_add_device() is invoked through the generic OF-based PM
domain look-up path, the device is being probed. Returning an error
will mean the device won't be attached to its PM domain. Errors of
-EPROBE_DEFER get special treatment and is propagated to the driver
core.

Therefore this change also enables the ->attach_dev() callback to
be able to request for a deferred probe sequence.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 0fd5ee1..ba4abbe 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1467,8 +1467,9 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 		goto out;
 	}
 
-	if (genpd->attach_dev)
-		genpd->attach_dev(genpd, dev);
+	ret = genpd->attach_dev ? genpd->attach_dev(genpd, dev) : 0;
+	if (ret)
+		goto out;
 
 	genpd->device_count++;
 	genpd->max_off_time_changed = true;
-- 
1.9.1

[PATCH 8/8] PM / Domains: Handle errors from genpd's ->attach_dev() callback

[Geert Uytterhoeven]

On Tue, Jan 27, 2015 at 9:13 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> When __pm_genpd_add_device() is invoked through the generic OF-based PM
> domain look-up path, the device is being probed. Returning an error
> will mean the device won't be attached to its PM domain. Errors of
> -EPROBE_DEFER get special treatment and is propagated to the driver

are propagated

> core.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 0/8] PM / Domains: Re-structure code for adding/removing devices to genpd

[Geert Uytterhoeven]

On Tue, Jan 27, 2015 at 9:13 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> While adding error handling of genpd's ->attach_dev() callback, I realized that
> we also had a need to re-structure some of the code which deals with
> adding/removing devices to genpd. Especially the APIs, __pm_genpd_add_device()
> and pm_genpd_remove_device() deserved some attention.
>
> Patch 1 -> 4, can be considered as more simple cleanups and should not impact
> the behavior for current clients using the APIs.
>
> Patch 5, eliminates a mutex for generic_pm_domain_data by re-order the
> initialization/cleanup of dev_pm_qos notifiers.
>
> Patch 6, move some code around to fix a potenial memory leakage of a struct
> pm_subsys_data.
>
> Patch 7, code restructuring which impacts locking behavior while adding/removing
> devices. Should improve code readability and decrease critical regions of
> holding locks.
>
> Patch 8, Adds error handling of genpd's ->attach_dev() callback
>
>
> Ulf Hansson (8):
>   PM / Domains: Rename __pm_genpd_alloc|free_dev_data()
>   PM / Domains: Remove reference counting for the generic_pm_domain_data
>   PM / Domains: Don't allow an existing generic_pm_domain_data
>   PM / Domains: Don't check for an existing device when adding a new
>   PM / Domains: Eliminate the mutex for the generic_pm_domain_data
>   PM / Domains: Free pm_subsys_data in error path in
>     __pm_genpd_add_device()
>   PM / Domains: Re-order initialization of generic_pm_domain_data
>   PM / Domains: Handle errors from genpd's ->attach_dev() callback

Looks OK, and tested on sh73a0/kzm9g-multiplatform with PM domains
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 0/8] PM / Domains: Re-structure code for adding/removing devices to genpd

[Rafael J. Wysocki]

On Tuesday, January 27, 2015 09:13:37 PM Ulf Hansson wrote:
> While adding error handling of genpd's ->attach_dev() callback, I realized that
> we also had a need to re-structure some of the code which deals with
> adding/removing devices to genpd. Especially the APIs, __pm_genpd_add_device()
> and pm_genpd_remove_device() deserved some attention.
> 
> Patch 1 -> 4, can be considered as more simple cleanups and should not impact
> the behavior for current clients using the APIs. 
> 
> Patch 5, eliminates a mutex for generic_pm_domain_data by re-order the
> initialization/cleanup of dev_pm_qos notifiers.
> 
> Patch 6, move some code around to fix a potenial memory leakage of a struct
> pm_subsys_data.
> 
> Patch 7, code restructuring which impacts locking behavior while adding/removing
> devices. Should improve code readability and decrease critical regions of
> holding locks.
> 
> Patch 8, Adds error handling of genpd's ->attach_dev() callback
> 
> 
> Ulf Hansson (8):
>   PM / Domains: Rename __pm_genpd_alloc|free_dev_data()
>   PM / Domains: Remove reference counting for the generic_pm_domain_data
>   PM / Domains: Don't allow an existing generic_pm_domain_data
>   PM / Domains: Don't check for an existing device when adding a new
>   PM / Domains: Eliminate the mutex for the generic_pm_domain_data
>   PM / Domains: Free pm_subsys_data in error path in
>     __pm_genpd_add_device()
>   PM / Domains: Re-order initialization of generic_pm_domain_data
>   PM / Domains: Handle errors from genpd's ->attach_dev() callback
> 
>  drivers/base/power/domain.c | 137 +++++++++++++++++++++-----------------------
>  include/linux/pm_domain.h   |   2 -
>  2 files changed, 65 insertions(+), 74 deletions(-)

Whole series queued up for 3.20, thanks!


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.