Skip to contentSkip to conversation

PM / Domains: Re-structure code for adding/removing devices to genpd

PATCH New Ulf Hansson 4 participants 14 messages Jan 27, 2015 – Feb 4, 2015

Subsystems (6)
Arm Samsung Exynos Driver core Suspend to RAM Hibernation +2 more subsystems
lore
Share & tools
lore

Ulf Hansson

author
#0 lore raw

cover PM / Domains: Re-structure code for adding/removing devices to genpd

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(-)
Signature · 1 line
1.9.1
author
#1 lore raw

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

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 <[email protected]>
---
 drivers/base/power/domain.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/base/power/domain.c +6 -6
index c5280f2..f9e7df5 100644
@@ -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;
Signature · 1 line
1.9.1
#2 lore raw

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

Quoted 12 lines · 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_*

Quoted 6 lines

External functions:
pm_genpd_*
_pm_genpd_*
__pm_genpd_*

Signed-off-by Ulf Hansson <[email protected]>
Acked-by Pavel Machek <[email protected]>
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
author
#3 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/base/power/domain.c | 10 ++--------
 include/linux/pm_domain.h   |  1 -
 2 files changed, 2 insertions(+), 9 deletions(-)
drivers/base/power/domain.c +2 -8
index f9e7df5..351df5b 100644
@@ -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;
include/linux/pm_domain.h +0 -1
index ed60776..e160a0b 100644
@@ -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;
 };
Signature · 1 line
1.9.1
#4 lore raw

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

Quoted 5 lines · 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 <[email protected]>
Acked-by Pavel Machek <[email protected]>
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
author
#5 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/base/power/domain.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)
drivers/base/power/domain.c +12 -8
index 351df5b..76eb0c3 100644
@@ -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);
Signature · 1 line
1.9.1
author
#6 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/base/power/domain.c | 7 -------
 1 file changed, 7 deletions(-)
drivers/base/power/domain.c +0 -7
index 76eb0c3..88198ba 100644
@@ -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;
Signature · 1 line
1.9.1
author
#7 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/base/power/domain.c | 37 ++++++++++++++-----------------------
 include/linux/pm_domain.h   |  1 -
 2 files changed, 14 insertions(+), 24 deletions(-)
drivers/base/power/domain.c +14 -23
index 88198ba..1f026c1 100644
@@ -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;
 }
include/linux/pm_domain.h +0 -1
index e160a0b..080e778 100644
@@ -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;
 };
Signature · 1 line
1.9.1
author
#8 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/base/power/domain.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)
drivers/base/power/domain.c +16 -9
index 1f026c1..3bd342f 100644
@@ -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;
Signature · 1 line
1.9.1
author
#9 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/base/power/domain.c | 67 +++++++++++++++++++++++----------------------
 1 file changed, 35 insertions(+), 32 deletions(-)
drivers/base/power/domain.c +35 -32
index 3bd342f..0fd5ee1 100644
@@ -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);
Signature · 1 line
1.9.1
author
#10 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/base/power/domain.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/base/power/domain.c +3 -2
index 0fd5ee1..ba4abbe 100644
@@ -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;
Signature · 1 line
1.9.1
#11 lore raw

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

Quoted 4 lines · Ulf Hansson

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

Quoted 1 line

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

#12 lore raw

cover PM / Domains: Re-structure code for adding/removing devices to genpd

In reply to Ulf Hansson
Quoted 28 lines · 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

Quoted 3 lines
    __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 <[email protected]>

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

#13 lore raw

cover PM / Domains: Re-structure code for adding/removing devices to genpd

In reply to Ulf Hansson
Quoted 28 lines · 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

Quoted 7 lines
    __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!

Signature · 2 lines
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.