[PATCH 0/2] PM / QoS: Fix device PM QoS problems

mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed

* [PATCH 0/2] PM / QoS: Fix device PM QoS problems
@ 2013-03-02  2:02 Rafael J. Wysocki
  2013-03-02  2:09 ` [PATCH 1/2] PM / QoS: Fix concurrency issues and memory leaks in device PM QoS Rafael J. Wysocki
  2013-03-02  2:11 ` [PATCH 2/2] PM / QoS: Remove device PM QoS sysfs attributes at the right place Rafael J. Wysocki
  0 siblings, 2 replies; 4+ messages in thread
From: Rafael J. Wysocki @ 2013-03-02  2:02 UTC (permalink / raw)
  To: Linux PM list; +Cc: Greg Kroah-Hartman, LKML

Hi,

Some issues with the device PM QoS core code have been uncovered recently
and the following two patches are supposed to fix them.

[1/2] Fix concurrency problems and memory leaks in device PM QoS core.
[2/2] Fix removal of device PM QoS attributes during device remove.

On top of the current Linus' tree.

Thanks,
Rafael

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH 1/2] PM / QoS: Fix concurrency issues and memory leaks in device PM QoS
  2013-03-02  2:02 [PATCH 0/2] PM / QoS: Fix device PM QoS problems Rafael J. Wysocki
@ 2013-03-02  2:09 ` Rafael J. Wysocki
  2013-03-02  2:11 ` [PATCH 2/2] PM / QoS: Remove device PM QoS sysfs attributes at the right place Rafael J. Wysocki
  1 sibling, 0 replies; 4+ messages in thread
From: Rafael J. Wysocki @ 2013-03-02  2:09 UTC (permalink / raw)
  To: Linux PM list; +Cc: Greg Kroah-Hartman, LKML

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The current device PM QoS code assumes that certain functions will
never be called in parallel with each other (for example, it is
assumed that dev_pm_qos_expose_flags() won't be called in parallel
with dev_pm_qos_hide_flags() for the same device and analogously
for the latency limit), which may be overly optimistic.  Moreover,
dev_pm_qos_expose_flags() and dev_pm_qos_expose_latency_limit()
leak memory in error code paths (req needs to be freed on errors)
and __dev_pm_qos_drop_user_request() forgets to free the request.

To fix the above issues put more things under the device PM QoS
mutex to make them mutually exclusive and add the missing freeing
of memory.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/power/qos.c |  129 +++++++++++++++++++++++++++++++----------------
 1 file changed, 87 insertions(+), 42 deletions(-)

Index: linux-pm/drivers/base/power/qos.c
===================================================================
--- linux-pm.orig/drivers/base/power/qos.c
+++ linux-pm/drivers/base/power/qos.c
@@ -344,6 +344,13 @@ static int __dev_pm_qos_update_request(s
 	s32 curr_value;
 	int ret = 0;
 
+	if (!req) /*guard against callers passing in null */
+		return -EINVAL;
+
+	if (WARN(!dev_pm_qos_request_active(req),
+		 "%s() called for unknown object\n", __func__))
+		return -EINVAL;
+
 	if (!req->dev->power.qos)
 		return -ENODEV;
 
@@ -386,6 +393,17 @@ int dev_pm_qos_update_request(struct dev
 {
 	int ret;
 
+	mutex_lock(&dev_pm_qos_mtx);
+	ret = __dev_pm_qos_update_request(req, new_value);
+	mutex_unlock(&dev_pm_qos_mtx);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_qos_update_request);
+
+static int __dev_pm_qos_remove_request(struct dev_pm_qos_request *req)
+{
+	int ret = 0;
+
 	if (!req) /*guard against callers passing in null */
 		return -EINVAL;
 
@@ -393,13 +411,15 @@ int dev_pm_qos_update_request(struct dev
 		 "%s() called for unknown object\n", __func__))
 		return -EINVAL;
 
-	mutex_lock(&dev_pm_qos_mtx);
-	ret = __dev_pm_qos_update_request(req, new_value);
-	mutex_unlock(&dev_pm_qos_mtx);
-
+	if (req->dev->power.qos) {
+		ret = apply_constraint(req, PM_QOS_REMOVE_REQ,
+				       PM_QOS_DEFAULT_VALUE);
+		memset(req, 0, sizeof(*req));
+	} else {
+		ret = -ENODEV;
+	}
 	return ret;
 }
-EXPORT_SYMBOL_GPL(dev_pm_qos_update_request);
 
 /**
  * dev_pm_qos_remove_request - modifies an existing qos request
@@ -418,26 +438,10 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_update_requ
  */
 int dev_pm_qos_remove_request(struct dev_pm_qos_request *req)
 {
-	int ret = 0;
-
-	if (!req) /*guard against callers passing in null */
-		return -EINVAL;
-
-	if (WARN(!dev_pm_qos_request_active(req),
-		 "%s() called for unknown object\n", __func__))
-		return -EINVAL;
+	int ret;
 
 	mutex_lock(&dev_pm_qos_mtx);
-
-	if (req->dev->power.qos) {
-		ret = apply_constraint(req, PM_QOS_REMOVE_REQ,
-				       PM_QOS_DEFAULT_VALUE);
-		memset(req, 0, sizeof(*req));
-	} else {
-		/* Return if the device has been removed */
-		ret = -ENODEV;
-	}
-
+	ret = __dev_pm_qos_remove_request(req);
 	mutex_unlock(&dev_pm_qos_mtx);
 	return ret;
 }
@@ -563,16 +567,20 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_add_ancesto
 static void __dev_pm_qos_drop_user_request(struct device *dev,
 					   enum dev_pm_qos_req_type type)
 {
+	struct dev_pm_qos_request *req = NULL;
+
 	switch(type) {
 	case DEV_PM_QOS_LATENCY:
-		dev_pm_qos_remove_request(dev->power.qos->latency_req);
+		req = dev->power.qos->latency_req;
 		dev->power.qos->latency_req = NULL;
 		break;
 	case DEV_PM_QOS_FLAGS:
-		dev_pm_qos_remove_request(dev->power.qos->flags_req);
+		req = dev->power.qos->flags_req;
 		dev->power.qos->flags_req = NULL;
 		break;
 	}
+	__dev_pm_qos_remove_request(req);
+	kfree(req);
 }
 
 /**
@@ -588,22 +596,36 @@ int dev_pm_qos_expose_latency_limit(stru
 	if (!device_is_registered(dev) || value < 0)
 		return -EINVAL;
 
-	if (dev->power.qos && dev->power.qos->latency_req)
-		return -EEXIST;
-
 	req = kzalloc(sizeof(*req), GFP_KERNEL);
 	if (!req)
 		return -ENOMEM;
 
 	ret = dev_pm_qos_add_request(dev, req, DEV_PM_QOS_LATENCY, value);
-	if (ret < 0)
+	if (ret < 0) {
+		kfree(req);
 		return ret;
+	}
+
+	mutex_lock(&dev_pm_qos_mtx);
+
+	if (!dev->power.qos)
+		ret = -ENODEV;
+	else if (dev->power.qos->latency_req)
+		ret = -EEXIST;
+
+	if (ret < 0) {
+		__dev_pm_qos_remove_request(req);
+		kfree(req);
+		goto out;
+	}
 
 	dev->power.qos->latency_req = req;
 	ret = pm_qos_sysfs_add_latency(dev);
 	if (ret)
 		__dev_pm_qos_drop_user_request(dev, DEV_PM_QOS_LATENCY);
 
+ out:
+	mutex_unlock(&dev_pm_qos_mtx);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(dev_pm_qos_expose_latency_limit);
@@ -614,10 +636,14 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_expose_late
  */
 void dev_pm_qos_hide_latency_limit(struct device *dev)
 {
+	mutex_lock(&dev_pm_qos_mtx);
+
 	if (dev->power.qos && dev->power.qos->latency_req) {
 		pm_qos_sysfs_remove_latency(dev);
 		__dev_pm_qos_drop_user_request(dev, DEV_PM_QOS_LATENCY);
 	}
+
+	mutex_unlock(&dev_pm_qos_mtx);
 }
 EXPORT_SYMBOL_GPL(dev_pm_qos_hide_latency_limit);
 
@@ -634,24 +660,37 @@ int dev_pm_qos_expose_flags(struct devic
 	if (!device_is_registered(dev))
 		return -EINVAL;
 
-	if (dev->power.qos && dev->power.qos->flags_req)
-		return -EEXIST;
-
 	req = kzalloc(sizeof(*req), GFP_KERNEL);
 	if (!req)
 		return -ENOMEM;
 
-	pm_runtime_get_sync(dev);
 	ret = dev_pm_qos_add_request(dev, req, DEV_PM_QOS_FLAGS, val);
-	if (ret < 0)
-		goto fail;
+	if (ret < 0) {
+		kfree(req);
+		return ret;
+	}
+
+	pm_runtime_get_sync(dev);
+	mutex_lock(&dev_pm_qos_mtx);
+
+	if (!dev->power.qos)
+		ret = -ENODEV;
+	else if (dev->power.qos->flags_req)
+		ret = -EEXIST;
+
+	if (ret < 0) {
+		__dev_pm_qos_remove_request(req);
+		kfree(req);
+		goto out;
+	}
 
 	dev->power.qos->flags_req = req;
 	ret = pm_qos_sysfs_add_flags(dev);
 	if (ret)
 		__dev_pm_qos_drop_user_request(dev, DEV_PM_QOS_FLAGS);
 
-fail:
+ out:
+	mutex_unlock(&dev_pm_qos_mtx);
 	pm_runtime_put(dev);
 	return ret;
 }
@@ -663,12 +702,16 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_expose_flag
  */
 void dev_pm_qos_hide_flags(struct device *dev)
 {
+	pm_runtime_get_sync(dev);
+	mutex_lock(&dev_pm_qos_mtx);
+
 	if (dev->power.qos && dev->power.qos->flags_req) {
 		pm_qos_sysfs_remove_flags(dev);
-		pm_runtime_get_sync(dev);
 		__dev_pm_qos_drop_user_request(dev, DEV_PM_QOS_FLAGS);
-		pm_runtime_put(dev);
 	}
+
+	mutex_unlock(&dev_pm_qos_mtx);
+	pm_runtime_put(dev);
 }
 EXPORT_SYMBOL_GPL(dev_pm_qos_hide_flags);
 
@@ -683,12 +726,14 @@ int dev_pm_qos_update_flags(struct devic
 	s32 value;
 	int ret;
 
-	if (!dev->power.qos || !dev->power.qos->flags_req)
-		return -EINVAL;
-
 	pm_runtime_get_sync(dev);
 	mutex_lock(&dev_pm_qos_mtx);
 
+	if (!dev->power.qos || !dev->power.qos->flags_req) {
+		ret = -EINVAL;
+		goto out;
+	}
+
 	value = dev_pm_qos_requested_flags(dev);
 	if (set)
 		value |= mask;
@@ -697,9 +742,9 @@ int dev_pm_qos_update_flags(struct devic
 
 	ret = __dev_pm_qos_update_request(dev->power.qos->flags_req, value);
 
+ out:
 	mutex_unlock(&dev_pm_qos_mtx);
 	pm_runtime_put(dev);
-
 	return ret;
 }
 #endif /* CONFIG_PM_RUNTIME */


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH 2/2] PM / QoS: Remove device PM QoS sysfs attributes at the right place
  2013-03-02  2:02 [PATCH 0/2] PM / QoS: Fix device PM QoS problems Rafael J. Wysocki
  2013-03-02  2:09 ` [PATCH 1/2] PM / QoS: Fix concurrency issues and memory leaks in device PM QoS Rafael J. Wysocki
@ 2013-03-02  2:11 ` Rafael J. Wysocki
  2013-03-02 13:09   ` [Update][PATCH " Rafael J. Wysocki
  1 sibling, 1 reply; 4+ messages in thread
From: Rafael J. Wysocki @ 2013-03-02  2:11 UTC (permalink / raw)
  To: Linux PM list; +Cc: Greg Kroah-Hartman, LKML

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Device PM QoS sysfs attributes, if present during device removal,
are removed from within device_pm_remove(), which is too late,
since dpm_sysfs_remove() has already removed the whole attribute
group they belonged to.  However, moving the removal of those
attributes to dpm_sysfs_remove() alone is not sufficient, because
in theory they still can be re-added right after being removed by it
(the device's driver is still bound to it at that point).

For this reason, move the entire desctruction of device PM QoS
constraints to dpm_sysfs_remove() and make it prevent any new
constraints from being added after it has run.  Analogously, move
the initialization of device PM QoS constraints to dpm_sysfs_add().

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/power/main.c  |    2 
 drivers/base/power/power.h |    6 --
 drivers/base/power/qos.c   |  104 +++++++++++++++++++++------------------------
 drivers/base/power/sysfs.c |    3 +
 4 files changed, 53 insertions(+), 62 deletions(-)

Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -99,7 +99,6 @@ void device_pm_add(struct device *dev)
 		dev_warn(dev, "parent %s should not be sleeping\n",
 			dev_name(dev->parent));
 	list_add_tail(&dev->power.entry, &dpm_list);
-	dev_pm_qos_constraints_init(dev);
 	mutex_unlock(&dpm_list_mtx);
 }
 
@@ -113,7 +112,6 @@ void device_pm_remove(struct device *dev
 		 dev->bus ? dev->bus->name : "No Bus", dev_name(dev));
 	complete_all(&dev->power.completion);
 	mutex_lock(&dpm_list_mtx);
-	dev_pm_qos_constraints_destroy(dev);
 	list_del_init(&dev->power.entry);
 	mutex_unlock(&dpm_list_mtx);
 	device_wakeup_disable(dev);
Index: linux-pm/drivers/base/power/qos.c
===================================================================
--- linux-pm.orig/drivers/base/power/qos.c
+++ linux-pm/drivers/base/power/qos.c
@@ -61,7 +61,7 @@ enum pm_qos_flags_status __dev_pm_qos_fl
 	struct pm_qos_flags *pqf;
 	s32 val;
 
-	if (!qos)
+	if (IS_ERR_OR_NULL(qos))
 		return PM_QOS_FLAGS_UNDEFINED;
 
 	pqf = &qos->flags;
@@ -101,7 +101,8 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_flags);
  */
 s32 __dev_pm_qos_read_value(struct device *dev)
 {
-	return dev->power.qos ? pm_qos_read_value(&dev->power.qos->latency) : 0;
+	return IS_ERR_OR_NULL(dev->power.qos) ?
+		0 : pm_qos_read_value(&dev->power.qos->latency);
 }
 
 /**
@@ -209,10 +210,12 @@ void dev_pm_qos_constraints_init(struct
 {
 	mutex_lock(&dev_pm_qos_mtx);
 	dev->power.qos = NULL;
-	dev->power.power_state = PMSG_ON;
 	mutex_unlock(&dev_pm_qos_mtx);
 }
 
+static void __dev_pm_qos_hide_latency_limit(struct device *dev);
+static void __dev_pm_qos_hide_flags(struct device *dev);
+
 /**
  * dev_pm_qos_constraints_destroy
  * @dev: target device
@@ -226,16 +229,15 @@ void dev_pm_qos_constraints_destroy(stru
 	struct pm_qos_constraints *c;
 	struct pm_qos_flags *f;
 
+	mutex_lock(&dev_pm_qos_mtx);
+
 	/*
 	 * If the device's PM QoS resume latency limit or PM QoS flags have been
 	 * exposed to user space, they have to be hidden at this point.
 	 */
-	dev_pm_qos_hide_latency_limit(dev);
-	dev_pm_qos_hide_flags(dev);
-
-	mutex_lock(&dev_pm_qos_mtx);
+	__dev_pm_qos_hide_latency_limit(dev);
+	__dev_pm_qos_hide_flags(dev);
 
-	dev->power.power_state = PMSG_INVALID;
 	qos = dev->power.qos;
 	if (!qos)
 		goto out;
@@ -257,7 +259,7 @@ void dev_pm_qos_constraints_destroy(stru
 	}
 
 	spin_lock_irq(&dev->power.lock);
-	dev->power.qos = NULL;
+	dev->power.qos = ERR_PTR(-ENODEV);
 	spin_unlock_irq(&dev->power.lock);
 
 	kfree(c->notifiers);
@@ -301,32 +303,19 @@ int dev_pm_qos_add_request(struct device
 		 "%s() called for already added request\n", __func__))
 		return -EINVAL;
 
-	req->dev = dev;
-
 	mutex_lock(&dev_pm_qos_mtx);
 
-	if (!dev->power.qos) {
-		if (dev->power.power_state.event == PM_EVENT_INVALID) {
-			/* The device has been removed from the system. */
-			req->dev = NULL;
-			ret = -ENODEV;
-			goto out;
-		} else {
-			/*
-			 * Allocate the constraints data on the first call to
-			 * add_request, i.e. only if the data is not already
-			 * allocated and if the device has not been removed.
-			 */
-			ret = dev_pm_qos_constraints_allocate(dev);
-		}
-	}
+	if (IS_ERR(dev->power.qos))
+		ret = -ENODEV;
+	else if (!dev->power.qos)
+		ret = dev_pm_qos_constraints_allocate(dev);
 
 	if (!ret) {
+		req->dev = dev;
 		req->type = type;
 		ret = apply_constraint(req, PM_QOS_ADD_REQ, value);
 	}
 
- out:
 	mutex_unlock(&dev_pm_qos_mtx);
 
 	return ret;
@@ -351,7 +340,7 @@ static int __dev_pm_qos_update_request(s
 		 "%s() called for unknown object\n", __func__))
 		return -EINVAL;
 
-	if (!req->dev->power.qos)
+	if (IS_ERR_OR_NULL(req->dev->power.qos))
 		return -ENODEV;
 
 	switch(req->type) {
@@ -402,7 +391,7 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_update_requ
 
 static int __dev_pm_qos_remove_request(struct dev_pm_qos_request *req)
 {
-	int ret = 0;
+	int ret;
 
 	if (!req) /*guard against callers passing in null */
 		return -EINVAL;
@@ -411,13 +400,11 @@ static int __dev_pm_qos_remove_request(s
 		 "%s() called for unknown object\n", __func__))
 		return -EINVAL;
 
-	if (req->dev->power.qos) {
-		ret = apply_constraint(req, PM_QOS_REMOVE_REQ,
-				       PM_QOS_DEFAULT_VALUE);
-		memset(req, 0, sizeof(*req));
-	} else {
-		ret = -ENODEV;
-	}
+	if (IS_ERR_OR_NULL(req->dev->power.qos))
+		return -ENODEV;
+
+	ret = apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_DEFAULT_VALUE);
+	memset(req, 0, sizeof(*req));
 	return ret;
 }
 
@@ -466,9 +453,10 @@ int dev_pm_qos_add_notifier(struct devic
 
 	mutex_lock(&dev_pm_qos_mtx);
 
-	if (!dev->power.qos)
-		ret = dev->power.power_state.event != PM_EVENT_INVALID ?
-			dev_pm_qos_constraints_allocate(dev) : -ENODEV;
+	if (IS_ERR(dev->power.qos))
+		ret = -ENODEV;
+	else if (!dev->power.qos)
+		ret = dev_pm_qos_constraints_allocate(dev);
 
 	if (!ret)
 		ret = blocking_notifier_chain_register(
@@ -497,7 +485,7 @@ int dev_pm_qos_remove_notifier(struct de
 	mutex_lock(&dev_pm_qos_mtx);
 
 	/* Silently return if the constraints object is not present. */
-	if (dev->power.qos)
+	if (!IS_ERR_OR_NULL(dev->power.qos))
 		retval = blocking_notifier_chain_unregister(
 				dev->power.qos->latency.notifiers,
 				notifier);
@@ -608,7 +596,7 @@ int dev_pm_qos_expose_latency_limit(stru
 
 	mutex_lock(&dev_pm_qos_mtx);
 
-	if (!dev->power.qos)
+	if (IS_ERR_OR_NULL(dev->power.qos))
 		ret = -ENODEV;
 	else if (dev->power.qos->latency_req)
 		ret = -EEXIST;
@@ -630,6 +618,14 @@ int dev_pm_qos_expose_latency_limit(stru
 }
 EXPORT_SYMBOL_GPL(dev_pm_qos_expose_latency_limit);
 
+static void __dev_pm_qos_hide_latency_limit(struct device *dev)
+{
+	if (!IS_ERR_OR_NULL(dev->power.qos) && dev->power.qos->latency_req) {
+		pm_qos_sysfs_remove_latency(dev);
+		__dev_pm_qos_drop_user_request(dev, DEV_PM_QOS_LATENCY);
+	}
+}
+
 /**
  * dev_pm_qos_hide_latency_limit - Hide PM QoS latency limit from user space.
  * @dev: Device whose PM QoS latency limit is to be hidden from user space.
@@ -637,12 +633,7 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_expose_late
 void dev_pm_qos_hide_latency_limit(struct device *dev)
 {
 	mutex_lock(&dev_pm_qos_mtx);
-
-	if (dev->power.qos && dev->power.qos->latency_req) {
-		pm_qos_sysfs_remove_latency(dev);
-		__dev_pm_qos_drop_user_request(dev, DEV_PM_QOS_LATENCY);
-	}
-
+	__dev_pm_qos_hide_latency_limit(dev);
 	mutex_unlock(&dev_pm_qos_mtx);
 }
 EXPORT_SYMBOL_GPL(dev_pm_qos_hide_latency_limit);
@@ -673,7 +664,7 @@ int dev_pm_qos_expose_flags(struct devic
 	pm_runtime_get_sync(dev);
 	mutex_lock(&dev_pm_qos_mtx);
 
-	if (!dev->power.qos)
+	if (IS_ERR_OR_NULL(dev->power.qos))
 		ret = -ENODEV;
 	else if (dev->power.qos->flags_req)
 		ret = -EEXIST;
@@ -696,6 +687,14 @@ int dev_pm_qos_expose_flags(struct devic
 }
 EXPORT_SYMBOL_GPL(dev_pm_qos_expose_flags);
 
+static void __dev_pm_qos_hide_flags(struct device *dev)
+{
+	if (!IS_ERR_OR_NULL(dev->power.qos) && dev->power.qos->flags_req) {
+		pm_qos_sysfs_remove_flags(dev);
+		__dev_pm_qos_drop_user_request(dev, DEV_PM_QOS_FLAGS);
+	}
+}
+
 /**
  * dev_pm_qos_hide_flags - Hide PM QoS flags of a device from user space.
  * @dev: Device whose PM QoS flags are to be hidden from user space.
@@ -704,12 +703,7 @@ void dev_pm_qos_hide_flags(struct device
 {
 	pm_runtime_get_sync(dev);
 	mutex_lock(&dev_pm_qos_mtx);
-
-	if (dev->power.qos && dev->power.qos->flags_req) {
-		pm_qos_sysfs_remove_flags(dev);
-		__dev_pm_qos_drop_user_request(dev, DEV_PM_QOS_FLAGS);
-	}
-
+	__dev_pm_qos_hide_flags(dev);
 	mutex_unlock(&dev_pm_qos_mtx);
 	pm_runtime_put(dev);
 }
@@ -729,7 +723,7 @@ int dev_pm_qos_update_flags(struct devic
 	pm_runtime_get_sync(dev);
 	mutex_lock(&dev_pm_qos_mtx);
 
-	if (!dev->power.qos || !dev->power.qos->flags_req) {
+	if (IS_ERR_OR_NULL(dev->power.qos) || !dev->power.qos->flags_req) {
 		ret = -EINVAL;
 		goto out;
 	}
Index: linux-pm/drivers/base/power/power.h
===================================================================
--- linux-pm.orig/drivers/base/power/power.h
+++ linux-pm/drivers/base/power/power.h
@@ -56,14 +56,10 @@ extern void device_pm_move_last(struct d
 
 static inline void device_pm_sleep_init(struct device *dev) {}
 
-static inline void device_pm_add(struct device *dev)
-{
-	dev_pm_qos_constraints_init(dev);
-}
+static inline void device_pm_add(struct device *dev) {}
 
 static inline void device_pm_remove(struct device *dev)
 {
-	dev_pm_qos_constraints_destroy(dev);
 	pm_runtime_remove(dev);
 }
 
Index: linux-pm/drivers/base/power/sysfs.c
===================================================================
--- linux-pm.orig/drivers/base/power/sysfs.c
+++ linux-pm/drivers/base/power/sysfs.c
@@ -664,6 +664,8 @@ int dpm_sysfs_add(struct device *dev)
 			goto err_out;
 		}
 	}
+
+	dev_pm_qos_constraints_init(dev);
 	return 0;
 
  err_out:
@@ -708,6 +710,7 @@ void rpm_sysfs_remove(struct device *dev
 
 void dpm_sysfs_remove(struct device *dev)
 {
+	dev_pm_qos_constraints_destroy(dev);
 	rpm_sysfs_remove(dev);
 	sysfs_unmerge_group(&dev->kobj, &pm_wakeup_attr_group);
 	sysfs_remove_group(&dev->kobj, &pm_attr_group);


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [Update][PATCH 2/2] PM / QoS: Remove device PM QoS sysfs attributes at the right place
  2013-03-02  2:11 ` [PATCH 2/2] PM / QoS: Remove device PM QoS sysfs attributes at the right place Rafael J. Wysocki
@ 2013-03-02 13:09   ` Rafael J. Wysocki
  0 siblings, 0 replies; 4+ messages in thread
From: Rafael J. Wysocki @ 2013-03-02 13:09 UTC (permalink / raw)
  To: Linux PM list; +Cc: Greg Kroah-Hartman, LKML, Sasha Levin

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Device PM QoS sysfs attributes, if present during device removal,
are removed from within device_pm_remove(), which is too late,
since dpm_sysfs_remove() has already removed the whole attribute
group they belonged to.  However, moving the removal of those
attributes to dpm_sysfs_remove() alone is not sufficient, because
in theory they still can be re-added right after being removed by it
(the device's driver is still bound to it at that point).

For this reason, move the entire desctruction of device PM QoS
constraints to dpm_sysfs_remove() and make it prevent any new
constraints from being added after it has run.  Also, move the
initialization of the power.qos field in struct device to
device_pm_init_common() and drop the no longer needed
dev_pm_qos_constraints_init().

Reported-by: Sasha Levin <sasha.levin@oracle.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

This update removes dev_pm_qos_constraints_init(), which is not necessary
any more (and device_pm_init_common() doesn't need to touch power.power_state)
and adds the Reported-by tag as appropriate.

Thanks,
Rafael

---
 drivers/base/power/main.c  |    2 
 drivers/base/power/power.h |    8 ---
 drivers/base/power/qos.c   |  116 ++++++++++++++++++---------------------------
 drivers/base/power/sysfs.c |    1 
 4 files changed, 51 insertions(+), 76 deletions(-)

Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -99,7 +99,6 @@ void device_pm_add(struct device *dev)
 		dev_warn(dev, "parent %s should not be sleeping\n",
 			dev_name(dev->parent));
 	list_add_tail(&dev->power.entry, &dpm_list);
-	dev_pm_qos_constraints_init(dev);
 	mutex_unlock(&dpm_list_mtx);
 }
 
@@ -113,7 +112,6 @@ void device_pm_remove(struct device *dev
 		 dev->bus ? dev->bus->name : "No Bus", dev_name(dev));
 	complete_all(&dev->power.completion);
 	mutex_lock(&dpm_list_mtx);
-	dev_pm_qos_constraints_destroy(dev);
 	list_del_init(&dev->power.entry);
 	mutex_unlock(&dpm_list_mtx);
 	device_wakeup_disable(dev);
Index: linux-pm/drivers/base/power/qos.c
===================================================================
--- linux-pm.orig/drivers/base/power/qos.c
+++ linux-pm/drivers/base/power/qos.c
@@ -61,7 +61,7 @@ enum pm_qos_flags_status __dev_pm_qos_fl
 	struct pm_qos_flags *pqf;
 	s32 val;
 
-	if (!qos)
+	if (IS_ERR_OR_NULL(qos))
 		return PM_QOS_FLAGS_UNDEFINED;
 
 	pqf = &qos->flags;
@@ -101,7 +101,8 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_flags);
  */
 s32 __dev_pm_qos_read_value(struct device *dev)
 {
-	return dev->power.qos ? pm_qos_read_value(&dev->power.qos->latency) : 0;
+	return IS_ERR_OR_NULL(dev->power.qos) ?
+		0 : pm_qos_read_value(&dev->power.qos->latency);
 }
 
 /**
@@ -198,20 +199,8 @@ static int dev_pm_qos_constraints_alloca
 	return 0;
 }
 
-/**
- * dev_pm_qos_constraints_init - Initalize device's PM QoS constraints pointer.
- * @dev: target device
- *
- * Called from the device PM subsystem during device insertion under
- * device_pm_lock().
- */
-void dev_pm_qos_constraints_init(struct device *dev)
-{
-	mutex_lock(&dev_pm_qos_mtx);
-	dev->power.qos = NULL;
-	dev->power.power_state = PMSG_ON;
-	mutex_unlock(&dev_pm_qos_mtx);
-}
+static void __dev_pm_qos_hide_latency_limit(struct device *dev);
+static void __dev_pm_qos_hide_flags(struct device *dev);
 
 /**
  * dev_pm_qos_constraints_destroy
@@ -226,16 +215,15 @@ void dev_pm_qos_constraints_destroy(stru
 	struct pm_qos_constraints *c;
 	struct pm_qos_flags *f;
 
+	mutex_lock(&dev_pm_qos_mtx);
+
 	/*
 	 * If the device's PM QoS resume latency limit or PM QoS flags have been
 	 * exposed to user space, they have to be hidden at this point.
 	 */
-	dev_pm_qos_hide_latency_limit(dev);
-	dev_pm_qos_hide_flags(dev);
-
-	mutex_lock(&dev_pm_qos_mtx);
+	__dev_pm_qos_hide_latency_limit(dev);
+	__dev_pm_qos_hide_flags(dev);
 
-	dev->power.power_state = PMSG_INVALID;
 	qos = dev->power.qos;
 	if (!qos)
 		goto out;
@@ -257,7 +245,7 @@ void dev_pm_qos_constraints_destroy(stru
 	}
 
 	spin_lock_irq(&dev->power.lock);
-	dev->power.qos = NULL;
+	dev->power.qos = ERR_PTR(-ENODEV);
 	spin_unlock_irq(&dev->power.lock);
 
 	kfree(c->notifiers);
@@ -301,32 +289,19 @@ int dev_pm_qos_add_request(struct device
 		 "%s() called for already added request\n", __func__))
 		return -EINVAL;
 
-	req->dev = dev;
-
 	mutex_lock(&dev_pm_qos_mtx);
 
-	if (!dev->power.qos) {
-		if (dev->power.power_state.event == PM_EVENT_INVALID) {
-			/* The device has been removed from the system. */
-			req->dev = NULL;
-			ret = -ENODEV;
-			goto out;
-		} else {
-			/*
-			 * Allocate the constraints data on the first call to
-			 * add_request, i.e. only if the data is not already
-			 * allocated and if the device has not been removed.
-			 */
-			ret = dev_pm_qos_constraints_allocate(dev);
-		}
-	}
+	if (IS_ERR(dev->power.qos))
+		ret = -ENODEV;
+	else if (!dev->power.qos)
+		ret = dev_pm_qos_constraints_allocate(dev);
 
 	if (!ret) {
+		req->dev = dev;
 		req->type = type;
 		ret = apply_constraint(req, PM_QOS_ADD_REQ, value);
 	}
 
- out:
 	mutex_unlock(&dev_pm_qos_mtx);
 
 	return ret;
@@ -351,7 +326,7 @@ static int __dev_pm_qos_update_request(s
 		 "%s() called for unknown object\n", __func__))
 		return -EINVAL;
 
-	if (!req->dev->power.qos)
+	if (IS_ERR_OR_NULL(req->dev->power.qos))
 		return -ENODEV;
 
 	switch(req->type) {
@@ -402,7 +377,7 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_update_requ
 
 static int __dev_pm_qos_remove_request(struct dev_pm_qos_request *req)
 {
-	int ret = 0;
+	int ret;
 
 	if (!req) /*guard against callers passing in null */
 		return -EINVAL;
@@ -411,13 +386,11 @@ static int __dev_pm_qos_remove_request(s
 		 "%s() called for unknown object\n", __func__))
 		return -EINVAL;
 
-	if (req->dev->power.qos) {
-		ret = apply_constraint(req, PM_QOS_REMOVE_REQ,
-				       PM_QOS_DEFAULT_VALUE);
-		memset(req, 0, sizeof(*req));
-	} else {
-		ret = -ENODEV;
-	}
+	if (IS_ERR_OR_NULL(req->dev->power.qos))
+		return -ENODEV;
+
+	ret = apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_DEFAULT_VALUE);
+	memset(req, 0, sizeof(*req));
 	return ret;
 }
 
@@ -466,9 +439,10 @@ int dev_pm_qos_add_notifier(struct devic
 
 	mutex_lock(&dev_pm_qos_mtx);
 
-	if (!dev->power.qos)
-		ret = dev->power.power_state.event != PM_EVENT_INVALID ?
-			dev_pm_qos_constraints_allocate(dev) : -ENODEV;
+	if (IS_ERR(dev->power.qos))
+		ret = -ENODEV;
+	else if (!dev->power.qos)
+		ret = dev_pm_qos_constraints_allocate(dev);
 
 	if (!ret)
 		ret = blocking_notifier_chain_register(
@@ -497,7 +471,7 @@ int dev_pm_qos_remove_notifier(struct de
 	mutex_lock(&dev_pm_qos_mtx);
 
 	/* Silently return if the constraints object is not present. */
-	if (dev->power.qos)
+	if (!IS_ERR_OR_NULL(dev->power.qos))
 		retval = blocking_notifier_chain_unregister(
 				dev->power.qos->latency.notifiers,
 				notifier);
@@ -608,7 +582,7 @@ int dev_pm_qos_expose_latency_limit(stru
 
 	mutex_lock(&dev_pm_qos_mtx);
 
-	if (!dev->power.qos)
+	if (IS_ERR_OR_NULL(dev->power.qos))
 		ret = -ENODEV;
 	else if (dev->power.qos->latency_req)
 		ret = -EEXIST;
@@ -630,6 +604,14 @@ int dev_pm_qos_expose_latency_limit(stru
 }
 EXPORT_SYMBOL_GPL(dev_pm_qos_expose_latency_limit);
 
+static void __dev_pm_qos_hide_latency_limit(struct device *dev)
+{
+	if (!IS_ERR_OR_NULL(dev->power.qos) && dev->power.qos->latency_req) {
+		pm_qos_sysfs_remove_latency(dev);
+		__dev_pm_qos_drop_user_request(dev, DEV_PM_QOS_LATENCY);
+	}
+}
+
 /**
  * dev_pm_qos_hide_latency_limit - Hide PM QoS latency limit from user space.
  * @dev: Device whose PM QoS latency limit is to be hidden from user space.
@@ -637,12 +619,7 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_expose_late
 void dev_pm_qos_hide_latency_limit(struct device *dev)
 {
 	mutex_lock(&dev_pm_qos_mtx);
-
-	if (dev->power.qos && dev->power.qos->latency_req) {
-		pm_qos_sysfs_remove_latency(dev);
-		__dev_pm_qos_drop_user_request(dev, DEV_PM_QOS_LATENCY);
-	}
-
+	__dev_pm_qos_hide_latency_limit(dev);
 	mutex_unlock(&dev_pm_qos_mtx);
 }
 EXPORT_SYMBOL_GPL(dev_pm_qos_hide_latency_limit);
@@ -673,7 +650,7 @@ int dev_pm_qos_expose_flags(struct devic
 	pm_runtime_get_sync(dev);
 	mutex_lock(&dev_pm_qos_mtx);
 
-	if (!dev->power.qos)
+	if (IS_ERR_OR_NULL(dev->power.qos))
 		ret = -ENODEV;
 	else if (dev->power.qos->flags_req)
 		ret = -EEXIST;
@@ -696,6 +673,14 @@ int dev_pm_qos_expose_flags(struct devic
 }
 EXPORT_SYMBOL_GPL(dev_pm_qos_expose_flags);
 
+static void __dev_pm_qos_hide_flags(struct device *dev)
+{
+	if (!IS_ERR_OR_NULL(dev->power.qos) && dev->power.qos->flags_req) {
+		pm_qos_sysfs_remove_flags(dev);
+		__dev_pm_qos_drop_user_request(dev, DEV_PM_QOS_FLAGS);
+	}
+}
+
 /**
  * dev_pm_qos_hide_flags - Hide PM QoS flags of a device from user space.
  * @dev: Device whose PM QoS flags are to be hidden from user space.
@@ -704,12 +689,7 @@ void dev_pm_qos_hide_flags(struct device
 {
 	pm_runtime_get_sync(dev);
 	mutex_lock(&dev_pm_qos_mtx);
-
-	if (dev->power.qos && dev->power.qos->flags_req) {
-		pm_qos_sysfs_remove_flags(dev);
-		__dev_pm_qos_drop_user_request(dev, DEV_PM_QOS_FLAGS);
-	}
-
+	__dev_pm_qos_hide_flags(dev);
 	mutex_unlock(&dev_pm_qos_mtx);
 	pm_runtime_put(dev);
 }
@@ -729,7 +709,7 @@ int dev_pm_qos_update_flags(struct devic
 	pm_runtime_get_sync(dev);
 	mutex_lock(&dev_pm_qos_mtx);
 
-	if (!dev->power.qos || !dev->power.qos->flags_req) {
+	if (IS_ERR_OR_NULL(dev->power.qos) || !dev->power.qos->flags_req) {
 		ret = -EINVAL;
 		goto out;
 	}
Index: linux-pm/drivers/base/power/power.h
===================================================================
--- linux-pm.orig/drivers/base/power/power.h
+++ linux-pm/drivers/base/power/power.h
@@ -4,7 +4,7 @@ static inline void device_pm_init_common
 {
 	if (!dev->power.early_init) {
 		spin_lock_init(&dev->power.lock);
-		dev->power.power_state = PMSG_INVALID;
+		dev->power.qos = NULL;
 		dev->power.early_init = true;
 	}
 }
@@ -56,14 +56,10 @@ extern void device_pm_move_last(struct d
 
 static inline void device_pm_sleep_init(struct device *dev) {}
 
-static inline void device_pm_add(struct device *dev)
-{
-	dev_pm_qos_constraints_init(dev);
-}
+static inline void device_pm_add(struct device *dev) {}
 
 static inline void device_pm_remove(struct device *dev)
 {
-	dev_pm_qos_constraints_destroy(dev);
 	pm_runtime_remove(dev);
 }
 
Index: linux-pm/drivers/base/power/sysfs.c
===================================================================
--- linux-pm.orig/drivers/base/power/sysfs.c
+++ linux-pm/drivers/base/power/sysfs.c
@@ -708,6 +708,7 @@ void rpm_sysfs_remove(struct device *dev
 
 void dpm_sysfs_remove(struct device *dev)
 {
+	dev_pm_qos_constraints_destroy(dev);
 	rpm_sysfs_remove(dev);
 	sysfs_unmerge_group(&dev->kobj, &pm_wakeup_attr_group);
 	sysfs_remove_group(&dev->kobj, &pm_attr_group);


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2013-03-02 13:02 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-03-02  2:02 [PATCH 0/2] PM / QoS: Fix device PM QoS problems Rafael J. Wysocki
2013-03-02  2:09 ` [PATCH 1/2] PM / QoS: Fix concurrency issues and memory leaks in device PM QoS Rafael J. Wysocki
2013-03-02  2:11 ` [PATCH 2/2] PM / QoS: Remove device PM QoS sysfs attributes at the right place Rafael J. Wysocki
2013-03-02 13:09   ` [Update][PATCH " Rafael J. Wysocki

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®