[PATCH 0/3] pm_qos: redo as plist and remove allocations

[PATCH 0/3] pm_qos: redo as plist and remove allocations

[James Bottomley]

I've done a bit more testing and sorted out some bugs (including several
in the plist patch where it wasn't firing the notifier correctly).

It looks OK to my test set up, but since I'm using artificial pm_qos
things, it would be good to have the real consumers test it out.

This is the patch layout:

James Bottomley (3):
  plist: add plist_last
  pm_qos: reimplement using plists
  pm_qos: get rid of the allocation in pm_qos_add_request()

 drivers/net/e1000e/netdev.c            |   17 +--
 drivers/net/igbvf/netdev.c             |    9 +-
 drivers/net/wireless/ipw2x00/ipw2100.c |   12 +-
 include/linux/netdevice.h              |    2 +-
 include/linux/plist.h                  |   29 +++++
 include/linux/pm_qos_params.h          |   13 ++-
 include/sound/pcm.h                    |    2 +-
 kernel/pm_qos_params.c                 |  211 +++++++++++++++++---------------
 sound/core/pcm_native.c                |   13 +--
 9 files changed, 175 insertions(+), 133 deletions(-)

[PATCH 1/3] plist: add plist_last

[James Bottomley]

plist is currently used by the scheduler, which only needs to know the
highest item in the list.  This adds plist_last which allows you to
find the lowest.  This is necessary for using plists to implement a
fast search of dynamic ranges in pm_qos which can have both highest
and lowest criteria.

Signed-off-by: James Bottomley <James.Bottomley@suse.de>
---
 include/linux/plist.h |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/include/linux/plist.h b/include/linux/plist.h
index 6898985..7254eda 100644
--- a/include/linux/plist.h
+++ b/include/linux/plist.h
@@ -260,6 +260,23 @@ static inline int plist_node_empty(const struct plist_node *node)
 #endif
 
 /**
+ * plist_last_entry - get the struct for the last entry
+ * @head:	the &struct plist_head pointer
+ * @type:	the type of the struct this is embedded in
+ * @member:	the name of the list_struct within the struct
+ */
+#ifdef CONFIG_DEBUG_PI_LIST
+# define plist_last_entry(head, type, member)	\
+({ \
+	WARN_ON(plist_head_empty(head)); \
+	container_of(plist_last(head), type, member); \
+})
+#else
+# define plist_last_entry(head, type, member)	\
+	container_of(plist_last(head), type, member)
+#endif
+
+/**
  * plist_first - return the first node (and thus, highest priority)
  * @head:	the &struct plist_head pointer
  *
@@ -271,4 +288,16 @@ static inline struct plist_node *plist_first(const struct plist_head *head)
 			  struct plist_node, plist.node_list);
 }
 
+/**
+ * plist_last - return the last node (and thus, lowest priority)
+ * @head:	the &struct plist_head pointer
+ *
+ * Assumes the plist is _not_ empty.
+ */
+static inline struct plist_node *plist_last(const struct plist_head *head)
+{
+	return list_entry(head->node_list.prev,
+			  struct plist_node, plist.node_list);
+}
+
 #endif
-- 
1.6.4.2

Re: [PATCH 1/3] plist: add plist_last

[Rafael J. Wysocki]

On Monday, June 28, 2010, James Bottomley wrote:
> plist is currently used by the scheduler, which only needs to know the
> highest item in the list.  This adds plist_last which allows you to
> find the lowest.  This is necessary for using plists to implement a
> fast search of dynamic ranges in pm_qos which can have both highest
> and lowest criteria.
> 
> Signed-off-by: James Bottomley <James.Bottomley@suse.de>

Applied to suspend-2.6/linux-next.

Rafael

[PATCH 2/3] pm_qos: reimplement using plists

[James Bottomley]

A lot of the pm_qos extremal value handling is really duplicating what a
priority ordered list does, just in a less efficient fashion.  Simply
redoing the implementation in terms of a plist gets rid of a lot of this
junk (although there are several other strange things that could do with
tidying up, like pm_qos_request_list has to carry the pm_qos_class with
every node, simply because it doesn't get passed in to
pm_qos_update_request even though every caller knows full well what
parameter it's updating).

I think this redo is a win independent of android, so we should do
something like this now.

Signed-off-by: James Bottomley <James.Bottomley@suse.de>
---
 kernel/pm_qos_params.c |  168 ++++++++++++++++++++++++------------------------
 1 files changed, 84 insertions(+), 84 deletions(-)

diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
index f42d3f7..b130b97 100644
--- a/kernel/pm_qos_params.c
+++ b/kernel/pm_qos_params.c
@@ -30,6 +30,7 @@
 /*#define DEBUG*/
 
 #include <linux/pm_qos_params.h>
+#include <linux/plist.h>
 #include <linux/sched.h>
 #include <linux/spinlock.h>
 #include <linux/slab.h>
@@ -49,58 +50,51 @@
  * held, taken with _irqsave.  One lock to rule them all
  */
 struct pm_qos_request_list {
-	struct list_head list;
-	union {
-		s32 value;
-		s32 usec;
-		s32 kbps;
-	};
+	struct plist_node list;
 	int pm_qos_class;
 };
 
-static s32 max_compare(s32 v1, s32 v2);
-static s32 min_compare(s32 v1, s32 v2);
+enum pm_qos_type {
+	PM_QOS_MAX,		/* return the largest value */
+	PM_QOS_MIN		/* return the smallest value */
+};
 
 struct pm_qos_object {
-	struct pm_qos_request_list requests;
+	struct plist_head requests;
 	struct blocking_notifier_head *notifiers;
 	struct miscdevice pm_qos_power_miscdev;
 	char *name;
 	s32 default_value;
-	atomic_t target_value;
-	s32 (*comparitor)(s32, s32);
+	enum pm_qos_type type;
 };
 
 static struct pm_qos_object null_pm_qos;
 static BLOCKING_NOTIFIER_HEAD(cpu_dma_lat_notifier);
 static struct pm_qos_object cpu_dma_pm_qos = {
-	.requests = {LIST_HEAD_INIT(cpu_dma_pm_qos.requests.list)},
+	.requests = {_PLIST_HEAD_INIT(cpu_dma_pm_qos.requests)},
 	.notifiers = &cpu_dma_lat_notifier,
 	.name = "cpu_dma_latency",
 	.default_value = 2000 * USEC_PER_SEC,
-	.target_value = ATOMIC_INIT(2000 * USEC_PER_SEC),
-	.comparitor = min_compare
+	.type = PM_QOS_MIN,
 };
 
 static BLOCKING_NOTIFIER_HEAD(network_lat_notifier);
 static struct pm_qos_object network_lat_pm_qos = {
-	.requests = {LIST_HEAD_INIT(network_lat_pm_qos.requests.list)},
+	.requests = {_PLIST_HEAD_INIT(network_lat_pm_qos.requests)},
 	.notifiers = &network_lat_notifier,
 	.name = "network_latency",
 	.default_value = 2000 * USEC_PER_SEC,
-	.target_value = ATOMIC_INIT(2000 * USEC_PER_SEC),
-	.comparitor = min_compare
+	.type = PM_QOS_MIN
 };
 
 
 static BLOCKING_NOTIFIER_HEAD(network_throughput_notifier);
 static struct pm_qos_object network_throughput_pm_qos = {
-	.requests = {LIST_HEAD_INIT(network_throughput_pm_qos.requests.list)},
+	.requests = {_PLIST_HEAD_INIT(network_throughput_pm_qos.requests)},
 	.notifiers = &network_throughput_notifier,
 	.name = "network_throughput",
 	.default_value = 0,
-	.target_value = ATOMIC_INIT(0),
-	.comparitor = max_compare
+	.type = PM_QOS_MAX,
 };
 
 
@@ -124,46 +118,55 @@ static const struct file_operations pm_qos_power_fops = {
 	.release = pm_qos_power_release,
 };
 
-/* static helper functions */
-static s32 max_compare(s32 v1, s32 v2)
+/* unlocked internal variant */
+static inline int pm_qos_get_value(struct pm_qos_object *o)
 {
-	return max(v1, v2);
-}
+	if (plist_head_empty(&o->requests))
+		return o->default_value;
 
-static s32 min_compare(s32 v1, s32 v2)
-{
-	return min(v1, v2);
-}
+	switch (o->type) {
+	case PM_QOS_MIN:
+		return plist_last(&o->requests)->prio;
 
+	case PM_QOS_MAX:
+		return plist_first(&o->requests)->prio;
+
+	default:
+		/* runtime check for not using enum */
+		BUG();
+	}
+}
 
-static void update_target(int pm_qos_class)
+static void update_target(struct pm_qos_object *o, struct plist_node *node,
+			  int del, int value)
 {
-	s32 extreme_value;
-	struct pm_qos_request_list *node;
 	unsigned long flags;
-	int call_notifier = 0;
+	int prev_value, curr_value;
 
 	spin_lock_irqsave(&pm_qos_lock, flags);
-	extreme_value = pm_qos_array[pm_qos_class]->default_value;
-	list_for_each_entry(node,
-			&pm_qos_array[pm_qos_class]->requests.list, list) {
-		extreme_value = pm_qos_array[pm_qos_class]->comparitor(
-				extreme_value, node->value);
-	}
-	if (atomic_read(&pm_qos_array[pm_qos_class]->target_value) !=
-			extreme_value) {
-		call_notifier = 1;
-		atomic_set(&pm_qos_array[pm_qos_class]->target_value,
-				extreme_value);
-		pr_debug(KERN_ERR "new target for qos %d is %d\n", pm_qos_class,
-			atomic_read(&pm_qos_array[pm_qos_class]->target_value));
+	prev_value = pm_qos_get_value(o);
+	/* PM_QOS_DEFAULT_VALUE is a signal that the value is unchanged */
+	if (value != PM_QOS_DEFAULT_VALUE) {
+		/*
+		 * to change the list, we atomically remove, reinit
+		 * with new value and add, then see if the extremal
+		 * changed
+		 */
+		plist_del(node, &o->requests);
+		plist_node_init(node, value);
+		plist_add(node, &o->requests);
+	} else if (del) {
+		plist_del(node, &o->requests);
+	} else {
+		plist_add(node, &o->requests);
 	}
+	curr_value = pm_qos_get_value(o);
 	spin_unlock_irqrestore(&pm_qos_lock, flags);
 
-	if (call_notifier)
-		blocking_notifier_call_chain(
-				pm_qos_array[pm_qos_class]->notifiers,
-					(unsigned long) extreme_value, NULL);
+	if (prev_value != curr_value)
+		blocking_notifier_call_chain(o->notifiers,
+					     (unsigned long)curr_value,
+					     NULL);
 }
 
 static int register_pm_qos_misc(struct pm_qos_object *qos)
@@ -196,7 +199,14 @@ static int find_pm_qos_object_by_minor(int minor)
  */
 int pm_qos_request(int pm_qos_class)
 {
-	return atomic_read(&pm_qos_array[pm_qos_class]->target_value);
+	unsigned long flags;
+	int value;
+
+	spin_lock_irqsave(&pm_qos_lock, flags);
+	value = pm_qos_get_value(pm_qos_array[pm_qos_class]);
+	spin_unlock_irqrestore(&pm_qos_lock, flags);
+
+	return value;
 }
 EXPORT_SYMBOL_GPL(pm_qos_request);
 
@@ -214,21 +224,19 @@ EXPORT_SYMBOL_GPL(pm_qos_request);
 struct pm_qos_request_list *pm_qos_add_request(int pm_qos_class, s32 value)
 {
 	struct pm_qos_request_list *dep;
-	unsigned long flags;
 
 	dep = kzalloc(sizeof(struct pm_qos_request_list), GFP_KERNEL);
 	if (dep) {
+		struct pm_qos_object *o =  pm_qos_array[pm_qos_class];
+		int new_value;
+
 		if (value == PM_QOS_DEFAULT_VALUE)
-			dep->value = pm_qos_array[pm_qos_class]->default_value;
+			new_value = o->default_value;
 		else
-			dep->value = value;
+			new_value = value;
+		plist_node_init(&dep->list, new_value);
 		dep->pm_qos_class = pm_qos_class;
-
-		spin_lock_irqsave(&pm_qos_lock, flags);
-		list_add(&dep->list,
-			&pm_qos_array[pm_qos_class]->requests.list);
-		spin_unlock_irqrestore(&pm_qos_lock, flags);
-		update_target(pm_qos_class);
+		update_target(o, &dep->list, 0, PM_QOS_DEFAULT_VALUE);
 	}
 
 	return dep;
@@ -246,27 +254,23 @@ EXPORT_SYMBOL_GPL(pm_qos_add_request);
  * Attempts are made to make this code callable on hot code paths.
  */
 void pm_qos_update_request(struct pm_qos_request_list *pm_qos_req,
-		s32 new_value)
+			   s32 new_value)
 {
-	unsigned long flags;
-	int pending_update = 0;
 	s32 temp;
+	struct pm_qos_object *o;
 
-	if (pm_qos_req) { /*guard against callers passing in null */
-		spin_lock_irqsave(&pm_qos_lock, flags);
-		if (new_value == PM_QOS_DEFAULT_VALUE)
-			temp = pm_qos_array[pm_qos_req->pm_qos_class]->default_value;
-		else
-			temp = new_value;
-
-		if (temp != pm_qos_req->value) {
-			pending_update = 1;
-			pm_qos_req->value = temp;
-		}
-		spin_unlock_irqrestore(&pm_qos_lock, flags);
-		if (pending_update)
-			update_target(pm_qos_req->pm_qos_class);
-	}
+	if (!pm_qos_req) /*guard against callers passing in null */
+		return;
+
+	o = pm_qos_array[pm_qos_req->pm_qos_class];
+
+	if (new_value == PM_QOS_DEFAULT_VALUE)
+		temp = o->default_value;
+	else
+		temp = new_value;
+
+	if (temp != pm_qos_req->list.prio)
+		update_target(o, &pm_qos_req->list, 0, temp);
 }
 EXPORT_SYMBOL_GPL(pm_qos_update_request);
 
@@ -280,19 +284,15 @@ EXPORT_SYMBOL_GPL(pm_qos_update_request);
  */
 void pm_qos_remove_request(struct pm_qos_request_list *pm_qos_req)
 {
-	unsigned long flags;
-	int qos_class;
+	struct pm_qos_object *o;
 
 	if (pm_qos_req == NULL)
 		return;
 		/* silent return to keep pcm code cleaner */
 
-	qos_class = pm_qos_req->pm_qos_class;
-	spin_lock_irqsave(&pm_qos_lock, flags);
-	list_del(&pm_qos_req->list);
+	o = pm_qos_array[pm_qos_req->pm_qos_class];
+	update_target(o, &pm_qos_req->list, 1, PM_QOS_DEFAULT_VALUE);
 	kfree(pm_qos_req);
-	spin_unlock_irqrestore(&pm_qos_lock, flags);
-	update_target(qos_class);
 }
 EXPORT_SYMBOL_GPL(pm_qos_remove_request);
 
-- 
1.6.4.2

Re: [PATCH 2/3] pm_qos: reimplement using plists

[mark gross]

On Mon, Jun 28, 2010 at 12:42:22PM -0500, James Bottomley wrote:
> A lot of the pm_qos extremal value handling is really duplicating what a
> priority ordered list does, just in a less efficient fashion.  Simply
> redoing the implementation in terms of a plist gets rid of a lot of this
> junk (although there are several other strange things that could do with
> tidying up, like pm_qos_request_list has to carry the pm_qos_class with
> every node, simply because it doesn't get passed in to
> pm_qos_update_request even though every caller knows full well what
> parameter it's updating).
> 
> I think this redo is a win independent of android, so we should do
> something like this now.

Thank you for doing this!, I'll integrate it into some testing targets
in the morning!

Signed-off-by: mark gross <markgross@thegnar.org>

--mgross
 
> Signed-off-by: James Bottomley <James.Bottomley@suse.de>
> ---
>  kernel/pm_qos_params.c |  168 ++++++++++++++++++++++++------------------------
>  1 files changed, 84 insertions(+), 84 deletions(-)
> 
> diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> index f42d3f7..b130b97 100644
> --- a/kernel/pm_qos_params.c
> +++ b/kernel/pm_qos_params.c
> @@ -30,6 +30,7 @@
>  /*#define DEBUG*/
>  
>  #include <linux/pm_qos_params.h>
> +#include <linux/plist.h>
>  #include <linux/sched.h>
>  #include <linux/spinlock.h>
>  #include <linux/slab.h>
> @@ -49,58 +50,51 @@
>   * held, taken with _irqsave.  One lock to rule them all
>   */
>  struct pm_qos_request_list {
> -	struct list_head list;
> -	union {
> -		s32 value;
> -		s32 usec;
> -		s32 kbps;
> -	};
> +	struct plist_node list;
>  	int pm_qos_class;
>  };
>  
> -static s32 max_compare(s32 v1, s32 v2);
> -static s32 min_compare(s32 v1, s32 v2);
> +enum pm_qos_type {
> +	PM_QOS_MAX,		/* return the largest value */
> +	PM_QOS_MIN		/* return the smallest value */
> +};
>  
>  struct pm_qos_object {
> -	struct pm_qos_request_list requests;
> +	struct plist_head requests;
>  	struct blocking_notifier_head *notifiers;
>  	struct miscdevice pm_qos_power_miscdev;
>  	char *name;
>  	s32 default_value;
> -	atomic_t target_value;
> -	s32 (*comparitor)(s32, s32);
> +	enum pm_qos_type type;
>  };
>  
>  static struct pm_qos_object null_pm_qos;
>  static BLOCKING_NOTIFIER_HEAD(cpu_dma_lat_notifier);
>  static struct pm_qos_object cpu_dma_pm_qos = {
> -	.requests = {LIST_HEAD_INIT(cpu_dma_pm_qos.requests.list)},
> +	.requests = {_PLIST_HEAD_INIT(cpu_dma_pm_qos.requests)},
>  	.notifiers = &cpu_dma_lat_notifier,
>  	.name = "cpu_dma_latency",
>  	.default_value = 2000 * USEC_PER_SEC,
> -	.target_value = ATOMIC_INIT(2000 * USEC_PER_SEC),
> -	.comparitor = min_compare
> +	.type = PM_QOS_MIN,
>  };
>  
>  static BLOCKING_NOTIFIER_HEAD(network_lat_notifier);
>  static struct pm_qos_object network_lat_pm_qos = {
> -	.requests = {LIST_HEAD_INIT(network_lat_pm_qos.requests.list)},
> +	.requests = {_PLIST_HEAD_INIT(network_lat_pm_qos.requests)},
>  	.notifiers = &network_lat_notifier,
>  	.name = "network_latency",
>  	.default_value = 2000 * USEC_PER_SEC,
> -	.target_value = ATOMIC_INIT(2000 * USEC_PER_SEC),
> -	.comparitor = min_compare
> +	.type = PM_QOS_MIN
>  };
>  
>  
>  static BLOCKING_NOTIFIER_HEAD(network_throughput_notifier);
>  static struct pm_qos_object network_throughput_pm_qos = {
> -	.requests = {LIST_HEAD_INIT(network_throughput_pm_qos.requests.list)},
> +	.requests = {_PLIST_HEAD_INIT(network_throughput_pm_qos.requests)},
>  	.notifiers = &network_throughput_notifier,
>  	.name = "network_throughput",
>  	.default_value = 0,
> -	.target_value = ATOMIC_INIT(0),
> -	.comparitor = max_compare
> +	.type = PM_QOS_MAX,
>  };
>  
>  
> @@ -124,46 +118,55 @@ static const struct file_operations pm_qos_power_fops = {
>  	.release = pm_qos_power_release,
>  };
>  
> -/* static helper functions */
> -static s32 max_compare(s32 v1, s32 v2)
> +/* unlocked internal variant */
> +static inline int pm_qos_get_value(struct pm_qos_object *o)
>  {
> -	return max(v1, v2);
> -}
> +	if (plist_head_empty(&o->requests))
> +		return o->default_value;
>  
> -static s32 min_compare(s32 v1, s32 v2)
> -{
> -	return min(v1, v2);
> -}
> +	switch (o->type) {
> +	case PM_QOS_MIN:
> +		return plist_last(&o->requests)->prio;
>  
> +	case PM_QOS_MAX:
> +		return plist_first(&o->requests)->prio;
> +
> +	default:
> +		/* runtime check for not using enum */
> +		BUG();
> +	}
> +}
>  
> -static void update_target(int pm_qos_class)
> +static void update_target(struct pm_qos_object *o, struct plist_node *node,
> +			  int del, int value)
>  {
> -	s32 extreme_value;
> -	struct pm_qos_request_list *node;
>  	unsigned long flags;
> -	int call_notifier = 0;
> +	int prev_value, curr_value;
>  
>  	spin_lock_irqsave(&pm_qos_lock, flags);
> -	extreme_value = pm_qos_array[pm_qos_class]->default_value;
> -	list_for_each_entry(node,
> -			&pm_qos_array[pm_qos_class]->requests.list, list) {
> -		extreme_value = pm_qos_array[pm_qos_class]->comparitor(
> -				extreme_value, node->value);
> -	}
> -	if (atomic_read(&pm_qos_array[pm_qos_class]->target_value) !=
> -			extreme_value) {
> -		call_notifier = 1;
> -		atomic_set(&pm_qos_array[pm_qos_class]->target_value,
> -				extreme_value);
> -		pr_debug(KERN_ERR "new target for qos %d is %d\n", pm_qos_class,
> -			atomic_read(&pm_qos_array[pm_qos_class]->target_value));
> +	prev_value = pm_qos_get_value(o);
> +	/* PM_QOS_DEFAULT_VALUE is a signal that the value is unchanged */
> +	if (value != PM_QOS_DEFAULT_VALUE) {
> +		/*
> +		 * to change the list, we atomically remove, reinit
> +		 * with new value and add, then see if the extremal
> +		 * changed
> +		 */
> +		plist_del(node, &o->requests);
> +		plist_node_init(node, value);
> +		plist_add(node, &o->requests);
> +	} else if (del) {
> +		plist_del(node, &o->requests);
> +	} else {
> +		plist_add(node, &o->requests);
>  	}
> +	curr_value = pm_qos_get_value(o);
>  	spin_unlock_irqrestore(&pm_qos_lock, flags);
>  
> -	if (call_notifier)
> -		blocking_notifier_call_chain(
> -				pm_qos_array[pm_qos_class]->notifiers,
> -					(unsigned long) extreme_value, NULL);
> +	if (prev_value != curr_value)
> +		blocking_notifier_call_chain(o->notifiers,
> +					     (unsigned long)curr_value,
> +					     NULL);
>  }
>  
>  static int register_pm_qos_misc(struct pm_qos_object *qos)
> @@ -196,7 +199,14 @@ static int find_pm_qos_object_by_minor(int minor)
>   */
>  int pm_qos_request(int pm_qos_class)
>  {
> -	return atomic_read(&pm_qos_array[pm_qos_class]->target_value);
> +	unsigned long flags;
> +	int value;
> +
> +	spin_lock_irqsave(&pm_qos_lock, flags);
> +	value = pm_qos_get_value(pm_qos_array[pm_qos_class]);
> +	spin_unlock_irqrestore(&pm_qos_lock, flags);
> +
> +	return value;
>  }
>  EXPORT_SYMBOL_GPL(pm_qos_request);
>  
> @@ -214,21 +224,19 @@ EXPORT_SYMBOL_GPL(pm_qos_request);
>  struct pm_qos_request_list *pm_qos_add_request(int pm_qos_class, s32 value)
>  {
>  	struct pm_qos_request_list *dep;
> -	unsigned long flags;
>  
>  	dep = kzalloc(sizeof(struct pm_qos_request_list), GFP_KERNEL);
>  	if (dep) {
> +		struct pm_qos_object *o =  pm_qos_array[pm_qos_class];
> +		int new_value;
> +
>  		if (value == PM_QOS_DEFAULT_VALUE)
> -			dep->value = pm_qos_array[pm_qos_class]->default_value;
> +			new_value = o->default_value;
>  		else
> -			dep->value = value;
> +			new_value = value;
> +		plist_node_init(&dep->list, new_value);
>  		dep->pm_qos_class = pm_qos_class;
> -
> -		spin_lock_irqsave(&pm_qos_lock, flags);
> -		list_add(&dep->list,
> -			&pm_qos_array[pm_qos_class]->requests.list);
> -		spin_unlock_irqrestore(&pm_qos_lock, flags);
> -		update_target(pm_qos_class);
> +		update_target(o, &dep->list, 0, PM_QOS_DEFAULT_VALUE);
>  	}
>  
>  	return dep;
> @@ -246,27 +254,23 @@ EXPORT_SYMBOL_GPL(pm_qos_add_request);
>   * Attempts are made to make this code callable on hot code paths.
>   */
>  void pm_qos_update_request(struct pm_qos_request_list *pm_qos_req,
> -		s32 new_value)
> +			   s32 new_value)
>  {
> -	unsigned long flags;
> -	int pending_update = 0;
>  	s32 temp;
> +	struct pm_qos_object *o;
>  
> -	if (pm_qos_req) { /*guard against callers passing in null */
> -		spin_lock_irqsave(&pm_qos_lock, flags);
> -		if (new_value == PM_QOS_DEFAULT_VALUE)
> -			temp = pm_qos_array[pm_qos_req->pm_qos_class]->default_value;
> -		else
> -			temp = new_value;
> -
> -		if (temp != pm_qos_req->value) {
> -			pending_update = 1;
> -			pm_qos_req->value = temp;
> -		}
> -		spin_unlock_irqrestore(&pm_qos_lock, flags);
> -		if (pending_update)
> -			update_target(pm_qos_req->pm_qos_class);
> -	}
> +	if (!pm_qos_req) /*guard against callers passing in null */
> +		return;
> +
> +	o = pm_qos_array[pm_qos_req->pm_qos_class];
> +
> +	if (new_value == PM_QOS_DEFAULT_VALUE)
> +		temp = o->default_value;
> +	else
> +		temp = new_value;
> +
> +	if (temp != pm_qos_req->list.prio)
> +		update_target(o, &pm_qos_req->list, 0, temp);
>  }
>  EXPORT_SYMBOL_GPL(pm_qos_update_request);
>  
> @@ -280,19 +284,15 @@ EXPORT_SYMBOL_GPL(pm_qos_update_request);
>   */
>  void pm_qos_remove_request(struct pm_qos_request_list *pm_qos_req)
>  {
> -	unsigned long flags;
> -	int qos_class;
> +	struct pm_qos_object *o;
>  
>  	if (pm_qos_req == NULL)
>  		return;
>  		/* silent return to keep pcm code cleaner */
>  
> -	qos_class = pm_qos_req->pm_qos_class;
> -	spin_lock_irqsave(&pm_qos_lock, flags);
> -	list_del(&pm_qos_req->list);
> +	o = pm_qos_array[pm_qos_req->pm_qos_class];
> +	update_target(o, &pm_qos_req->list, 1, PM_QOS_DEFAULT_VALUE);
>  	kfree(pm_qos_req);
> -	spin_unlock_irqrestore(&pm_qos_lock, flags);
> -	update_target(qos_class);
>  }
>  EXPORT_SYMBOL_GPL(pm_qos_remove_request);
>  
> -- 
> 1.6.4.2
> 
> 
> 

Re: [PATCH 2/3] pm_qos: reimplement using plists

[Rafael J. Wysocki]

On Tuesday, June 29, 2010, mark gross wrote:
> On Mon, Jun 28, 2010 at 12:42:22PM -0500, James Bottomley wrote:
> > A lot of the pm_qos extremal value handling is really duplicating what a
> > priority ordered list does, just in a less efficient fashion.  Simply
> > redoing the implementation in terms of a plist gets rid of a lot of this
> > junk (although there are several other strange things that could do with
> > tidying up, like pm_qos_request_list has to carry the pm_qos_class with
> > every node, simply because it doesn't get passed in to
> > pm_qos_update_request even though every caller knows full well what
> > parameter it's updating).
> > 
> > I think this redo is a win independent of android, so we should do
> > something like this now.
> 
> Thank you for doing this!, I'll integrate it into some testing targets
> in the morning!
> 
> Signed-off-by: mark gross <markgross@thegnar.org>
> 
> --mgross
>  
> > Signed-off-by: James Bottomley <James.Bottomley@suse.de>

Applied to suspend-2.6/linux-next.

Rafael

[PATCH 3/3] pm_qos: get rid of the allocation in pm_qos_add_request()

[James Bottomley]

Since every caller has to squirrel away the returned pointer anyway,
they might as well supply the memory area.  This fixes a bug in a few of
the call sites where the returned pointer was dereferenced without
checking it for NULL (which gets returned if the kzalloc failed).

I'd like to hear how sound and netdev feels about this: it will add
about two more pointers worth of data to struct netdev and struct
snd_pcm_substream .. but I think it's worth it.  If you're OK, I'll add
your acks and send through the pm tree.

This also looks to me like an android independent clean up (even though
it renders the request_add atomically callable).  I also added include
guards to include/linux/pm_qos_params.h

cc: netdev@vger.kernel.org
cc: Takashi Iwai <tiwai@suse.de>
Signed-off-by: James Bottomley <James.Bottomley@suse.de>
---
 drivers/net/e1000e/netdev.c            |   17 +++-----
 drivers/net/igbvf/netdev.c             |    9 ++--
 drivers/net/wireless/ipw2x00/ipw2100.c |   12 +++---
 include/linux/netdevice.h              |    2 +-
 include/linux/pm_qos_params.h          |   13 +++++-
 include/sound/pcm.h                    |    2 +-
 kernel/pm_qos_params.c                 |   67 +++++++++++++++++++-------------
 sound/core/pcm_native.c                |   13 ++----
 8 files changed, 74 insertions(+), 61 deletions(-)

diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
index 24507f3..47ea62f 100644
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -2901,10 +2901,10 @@ static void e1000_configure_rx(struct e1000_adapter *adapter)
 			 * dropped transactions.
 			 */
 			pm_qos_update_request(
-				adapter->netdev->pm_qos_req, 55);
+				&adapter->netdev->pm_qos_req, 55);
 		} else {
 			pm_qos_update_request(
-				adapter->netdev->pm_qos_req,
+				&adapter->netdev->pm_qos_req,
 				PM_QOS_DEFAULT_VALUE);
 		}
 	}
@@ -3196,9 +3196,9 @@ int e1000e_up(struct e1000_adapter *adapter)
 
 	/* DMA latency requirement to workaround early-receive/jumbo issue */
 	if (adapter->flags & FLAG_HAS_ERT)
-		adapter->netdev->pm_qos_req =
-			pm_qos_add_request(PM_QOS_CPU_DMA_LATENCY,
-				       PM_QOS_DEFAULT_VALUE);
+		pm_qos_add_request(&adapter->netdev->pm_qos_req,
+				   PM_QOS_CPU_DMA_LATENCY,
+				   PM_QOS_DEFAULT_VALUE);
 
 	/* hardware has been reset, we need to reload some things */
 	e1000_configure(adapter);
@@ -3263,11 +3263,8 @@ void e1000e_down(struct e1000_adapter *adapter)
 	e1000_clean_tx_ring(adapter);
 	e1000_clean_rx_ring(adapter);
 
-	if (adapter->flags & FLAG_HAS_ERT) {
-		pm_qos_remove_request(
-			      adapter->netdev->pm_qos_req);
-		adapter->netdev->pm_qos_req = NULL;
-	}
+	if (adapter->flags & FLAG_HAS_ERT)
+		pm_qos_remove_request(&adapter->netdev->pm_qos_req);
 
 	/*
 	 * TODO: for power management, we could drop the link and
diff --git a/drivers/net/igbvf/netdev.c b/drivers/net/igbvf/netdev.c
index 5e2b2a8..add6197 100644
--- a/drivers/net/igbvf/netdev.c
+++ b/drivers/net/igbvf/netdev.c
@@ -48,7 +48,7 @@
 #define DRV_VERSION "1.0.0-k0"
 char igbvf_driver_name[] = "igbvf";
 const char igbvf_driver_version[] = DRV_VERSION;
-struct pm_qos_request_list *igbvf_driver_pm_qos_req;
+static struct pm_qos_request_list igbvf_driver_pm_qos_req;
 static const char igbvf_driver_string[] =
 				"Intel(R) Virtual Function Network Driver";
 static const char igbvf_copyright[] = "Copyright (c) 2009 Intel Corporation.";
@@ -2902,8 +2902,8 @@ static int __init igbvf_init_module(void)
 	printk(KERN_INFO "%s\n", igbvf_copyright);
 
 	ret = pci_register_driver(&igbvf_driver);
-	igbvf_driver_pm_qos_req = pm_qos_add_request(PM_QOS_CPU_DMA_LATENCY,
-	                       PM_QOS_DEFAULT_VALUE);
+	pm_qos_add_request(&igbvf_driver_pm_qos_req, PM_QOS_CPU_DMA_LATENCY,
+			   PM_QOS_DEFAULT_VALUE);
 
 	return ret;
 }
@@ -2918,8 +2918,7 @@ module_init(igbvf_init_module);
 static void __exit igbvf_exit_module(void)
 {
 	pci_unregister_driver(&igbvf_driver);
-	pm_qos_remove_request(igbvf_driver_pm_qos_req);
-	igbvf_driver_pm_qos_req = NULL;
+	pm_qos_remove_request(&igbvf_driver_pm_qos_req);
 }
 module_exit(igbvf_exit_module);
 
diff --git a/drivers/net/wireless/ipw2x00/ipw2100.c b/drivers/net/wireless/ipw2x00/ipw2100.c
index 0bd4dfa..7f0d98b 100644
--- a/drivers/net/wireless/ipw2x00/ipw2100.c
+++ b/drivers/net/wireless/ipw2x00/ipw2100.c
@@ -174,7 +174,7 @@ that only one external action is invoked at a time.
 #define DRV_DESCRIPTION	"Intel(R) PRO/Wireless 2100 Network Driver"
 #define DRV_COPYRIGHT	"Copyright(c) 2003-2006 Intel Corporation"
 
-struct pm_qos_request_list *ipw2100_pm_qos_req;
+struct pm_qos_request_list ipw2100_pm_qos_req;
 
 /* Debugging stuff */
 #ifdef CONFIG_IPW2100_DEBUG
@@ -1741,7 +1741,7 @@ static int ipw2100_up(struct ipw2100_priv *priv, int deferred)
 	/* the ipw2100 hardware really doesn't want power management delays
 	 * longer than 175usec
 	 */
-	pm_qos_update_request(ipw2100_pm_qos_req, 175);
+	pm_qos_update_request(&ipw2100_pm_qos_req, 175);
 
 	/* If the interrupt is enabled, turn it off... */
 	spin_lock_irqsave(&priv->low_lock, flags);
@@ -1889,7 +1889,7 @@ static void ipw2100_down(struct ipw2100_priv *priv)
 	ipw2100_disable_interrupts(priv);
 	spin_unlock_irqrestore(&priv->low_lock, flags);
 
-	pm_qos_update_request(ipw2100_pm_qos_req, PM_QOS_DEFAULT_VALUE);
+	pm_qos_update_request(&ipw2100_pm_qos_req, PM_QOS_DEFAULT_VALUE);
 
 	/* We have to signal any supplicant if we are disassociating */
 	if (associated)
@@ -6669,8 +6669,8 @@ static int __init ipw2100_init(void)
 	if (ret)
 		goto out;
 
-	ipw2100_pm_qos_req = pm_qos_add_request(PM_QOS_CPU_DMA_LATENCY,
-			PM_QOS_DEFAULT_VALUE);
+	pm_qos_add_request(&ipw2100_pm_qos_req, PM_QOS_CPU_DMA_LATENCY,
+			   PM_QOS_DEFAULT_VALUE);
 #ifdef CONFIG_IPW2100_DEBUG
 	ipw2100_debug_level = debug;
 	ret = driver_create_file(&ipw2100_pci_driver.driver,
@@ -6692,7 +6692,7 @@ static void __exit ipw2100_exit(void)
 			   &driver_attr_debug_level);
 #endif
 	pci_unregister_driver(&ipw2100_pci_driver);
-	pm_qos_remove_request(ipw2100_pm_qos_req);
+	pm_qos_remove_request(&ipw2100_pm_qos_req);
 }
 
 module_init(ipw2100_init);
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 40291f3..393555a 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -779,7 +779,7 @@ struct net_device {
 	 */
 	char			name[IFNAMSIZ];
 
-	struct pm_qos_request_list *pm_qos_req;
+	struct pm_qos_request_list pm_qos_req;
 
 	/* device name hash chain */
 	struct hlist_node	name_hlist;
diff --git a/include/linux/pm_qos_params.h b/include/linux/pm_qos_params.h
index 8ba440e..77cbddb 100644
--- a/include/linux/pm_qos_params.h
+++ b/include/linux/pm_qos_params.h
@@ -1,8 +1,10 @@
+#ifndef _LINUX_PM_QOS_PARAMS_H
+#define _LINUX_PM_QOS_PARAMS_H
 /* interface for the pm_qos_power infrastructure of the linux kernel.
  *
  * Mark Gross <mgross@linux.intel.com>
  */
-#include <linux/list.h>
+#include <linux/plist.h>
 #include <linux/notifier.h>
 #include <linux/miscdevice.h>
 
@@ -14,9 +16,12 @@
 #define PM_QOS_NUM_CLASSES 4
 #define PM_QOS_DEFAULT_VALUE -1
 
-struct pm_qos_request_list;
+struct pm_qos_request_list {
+	struct plist_node list;
+	int pm_qos_class;
+};
 
-struct pm_qos_request_list *pm_qos_add_request(int pm_qos_class, s32 value);
+void pm_qos_add_request(struct pm_qos_request_list *l, int pm_qos_class, s32 value);
 void pm_qos_update_request(struct pm_qos_request_list *pm_qos_req,
 		s32 new_value);
 void pm_qos_remove_request(struct pm_qos_request_list *pm_qos_req);
@@ -24,4 +29,6 @@ void pm_qos_remove_request(struct pm_qos_request_list *pm_qos_req);
 int pm_qos_request(int pm_qos_class);
 int pm_qos_add_notifier(int pm_qos_class, struct notifier_block *notifier);
 int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *notifier);
+int pm_qos_request_active(struct pm_qos_request_list *req);
 
+#endif
diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index dd76cde..6e3a297 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -366,7 +366,7 @@ struct snd_pcm_substream {
 	int number;
 	char name[32];			/* substream name */
 	int stream;			/* stream (direction) */
-	struct pm_qos_request_list *latency_pm_qos_req; /* pm_qos request */
+	struct pm_qos_request_list latency_pm_qos_req; /* pm_qos request */
 	size_t buffer_bytes_max;	/* limit ring buffer size */
 	struct snd_dma_buffer dma_buffer;
 	unsigned int dma_buf_id;
diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
index b130b97..bff4053 100644
--- a/kernel/pm_qos_params.c
+++ b/kernel/pm_qos_params.c
@@ -30,7 +30,6 @@
 /*#define DEBUG*/
 
 #include <linux/pm_qos_params.h>
-#include <linux/plist.h>
 #include <linux/sched.h>
 #include <linux/spinlock.h>
 #include <linux/slab.h>
@@ -49,11 +48,6 @@
  * or pm_qos_object list and pm_qos_objects need to happen with pm_qos_lock
  * held, taken with _irqsave.  One lock to rule them all
  */
-struct pm_qos_request_list {
-	struct plist_node list;
-	int pm_qos_class;
-};
-
 enum pm_qos_type {
 	PM_QOS_MAX,		/* return the largest value */
 	PM_QOS_MIN		/* return the smallest value */
@@ -210,6 +204,12 @@ int pm_qos_request(int pm_qos_class)
 }
 EXPORT_SYMBOL_GPL(pm_qos_request);
 
+int pm_qos_request_active(struct pm_qos_request_list *req)
+{
+	return req->pm_qos_class != 0;
+}
+EXPORT_SYMBOL_GPL(pm_qos_request_active);
+
 /**
  * pm_qos_add_request - inserts new qos request into the list
  * @pm_qos_class: identifies which list of qos request to us
@@ -221,25 +221,23 @@ EXPORT_SYMBOL_GPL(pm_qos_request);
  * element as a handle for use in updating and removal.  Call needs to save
  * this handle for later use.
  */
-struct pm_qos_request_list *pm_qos_add_request(int pm_qos_class, s32 value)
+void pm_qos_add_request(struct pm_qos_request_list *dep,
+			int pm_qos_class, s32 value)
 {
-	struct pm_qos_request_list *dep;
-
-	dep = kzalloc(sizeof(struct pm_qos_request_list), GFP_KERNEL);
-	if (dep) {
-		struct pm_qos_object *o =  pm_qos_array[pm_qos_class];
-		int new_value;
-
-		if (value == PM_QOS_DEFAULT_VALUE)
-			new_value = o->default_value;
-		else
-			new_value = value;
-		plist_node_init(&dep->list, new_value);
-		dep->pm_qos_class = pm_qos_class;
-		update_target(o, &dep->list, 0, PM_QOS_DEFAULT_VALUE);
-	}
+	struct pm_qos_object *o =  pm_qos_array[pm_qos_class];
+	int new_value;
 
-	return dep;
+	if (pm_qos_request_active(dep)) {
+		WARN(1, KERN_ERR "pm_qos_add_request() called for already added request\n");
+		return;
+	}
+	if (value == PM_QOS_DEFAULT_VALUE)
+		new_value = o->default_value;
+	else
+		new_value = value;
+	plist_node_init(&dep->list, new_value);
+	dep->pm_qos_class = pm_qos_class;
+	update_target(o, &dep->list, 0, PM_QOS_DEFAULT_VALUE);
 }
 EXPORT_SYMBOL_GPL(pm_qos_add_request);
 
@@ -262,6 +260,11 @@ void pm_qos_update_request(struct pm_qos_request_list *pm_qos_req,
 	if (!pm_qos_req) /*guard against callers passing in null */
 		return;
 
+	if (pm_qos_request_active(pm_qos_req)) {
+		WARN(1, KERN_ERR "pm_qos_update_request() called for unknown object\n");
+		return;
+	}
+
 	o = pm_qos_array[pm_qos_req->pm_qos_class];
 
 	if (new_value == PM_QOS_DEFAULT_VALUE)
@@ -290,9 +293,14 @@ void pm_qos_remove_request(struct pm_qos_request_list *pm_qos_req)
 		return;
 		/* silent return to keep pcm code cleaner */
 
+	if (!pm_qos_request_active(pm_qos_req)) {
+		WARN(1, KERN_ERR "pm_qos_remove_request() called for unknown object\n");
+		return;
+	}
+
 	o = pm_qos_array[pm_qos_req->pm_qos_class];
 	update_target(o, &pm_qos_req->list, 1, PM_QOS_DEFAULT_VALUE);
-	kfree(pm_qos_req);
+	memset(pm_qos_req, 0, sizeof(*pm_qos_req));
 }
 EXPORT_SYMBOL_GPL(pm_qos_remove_request);
 
@@ -340,8 +348,12 @@ static int pm_qos_power_open(struct inode *inode, struct file *filp)
 
 	pm_qos_class = find_pm_qos_object_by_minor(iminor(inode));
 	if (pm_qos_class >= 0) {
-		filp->private_data = (void *) pm_qos_add_request(pm_qos_class,
-				PM_QOS_DEFAULT_VALUE);
+		struct pm_qos_request_list *req = kzalloc(GFP_KERNEL, sizeof(*req));
+		if (!req)
+			return -ENOMEM;
+
+		pm_qos_add_request(req, pm_qos_class, PM_QOS_DEFAULT_VALUE);
+		filp->private_data = req;
 
 		if (filp->private_data)
 			return 0;
@@ -353,8 +365,9 @@ static int pm_qos_power_release(struct inode *inode, struct file *filp)
 {
 	struct pm_qos_request_list *req;
 
-	req = (struct pm_qos_request_list *)filp->private_data;
+	req = filp->private_data;
 	pm_qos_remove_request(req);
+	kfree(req);
 
 	return 0;
 }
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 303ac04..a3b2a64 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -451,13 +451,11 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream,
 	snd_pcm_timer_resolution_change(substream);
 	runtime->status->state = SNDRV_PCM_STATE_SETUP;
 
-	if (substream->latency_pm_qos_req) {
-		pm_qos_remove_request(substream->latency_pm_qos_req);
-		substream->latency_pm_qos_req = NULL;
-	}
+	if (pm_qos_request_active(&substream->latency_pm_qos_req))
+		pm_qos_remove_request(&substream->latency_pm_qos_req);
 	if ((usecs = period_to_usecs(runtime)) >= 0)
-		substream->latency_pm_qos_req = pm_qos_add_request(
-					PM_QOS_CPU_DMA_LATENCY, usecs);
+		pm_qos_add_request(&substream->latency_pm_qos_req,
+				   PM_QOS_CPU_DMA_LATENCY, usecs);
 	return 0;
  _error:
 	/* hardware might be unuseable from this time,
@@ -512,8 +510,7 @@ static int snd_pcm_hw_free(struct snd_pcm_substream *substream)
 	if (substream->ops->hw_free)
 		result = substream->ops->hw_free(substream);
 	runtime->status->state = SNDRV_PCM_STATE_OPEN;
-	pm_qos_remove_request(substream->latency_pm_qos_req);
-	substream->latency_pm_qos_req = NULL;
+	pm_qos_remove_request(&substream->latency_pm_qos_req);
 	return result;
 }
 
-- 
1.6.4.2

Re: [PATCH 3/3] pm_qos: get rid of the allocation in pm_qos_add_request()

[Rafael J. Wysocki]

On Monday, June 28, 2010, James Bottomley wrote:
> Since every caller has to squirrel away the returned pointer anyway,
> they might as well supply the memory area.  This fixes a bug in a few of
> the call sites where the returned pointer was dereferenced without
> checking it for NULL (which gets returned if the kzalloc failed).
> 
> I'd like to hear how sound and netdev feels about this: it will add
> about two more pointers worth of data to struct netdev and struct
> snd_pcm_substream .. but I think it's worth it.  If you're OK, I'll add
> your acks and send through the pm tree.
> 
> This also looks to me like an android independent clean up (even though
> it renders the request_add atomically callable).  I also added include
> guards to include/linux/pm_qos_params.h
> 
> cc: netdev@vger.kernel.org
> cc: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: James Bottomley <James.Bottomley@suse.de>

I like all of the patches in this series, thanks a lot for doing this!

I guess it might be worth sending a CC to the LKML next round so that people
can see [1/3] (I don't expect any objections, but anyway it would be nice).

Thanks,
Rafael

Re: [PATCH 3/3] pm_qos: get rid of the allocation in pm_qos_add_request()

[James Bottomley]

On Mon, 2010-06-28 at 23:59 +0200, Rafael J. Wysocki wrote:
> On Monday, June 28, 2010, James Bottomley wrote:
> > Since every caller has to squirrel away the returned pointer anyway,
> > they might as well supply the memory area.  This fixes a bug in a few of
> > the call sites where the returned pointer was dereferenced without
> > checking it for NULL (which gets returned if the kzalloc failed).
> > 
> > I'd like to hear how sound and netdev feels about this: it will add
> > about two more pointers worth of data to struct netdev and struct
> > snd_pcm_substream .. but I think it's worth it.  If you're OK, I'll add
> > your acks and send through the pm tree.
> > 
> > This also looks to me like an android independent clean up (even though
> > it renders the request_add atomically callable).  I also added include
> > guards to include/linux/pm_qos_params.h
> > 
> > cc: netdev@vger.kernel.org
> > cc: Takashi Iwai <tiwai@suse.de>
> > Signed-off-by: James Bottomley <James.Bottomley@suse.de>
> 
> I like all of the patches in this series, thanks a lot for doing this!
> 
> I guess it might be worth sending a CC to the LKML next round so that people
> can see [1/3] (I don't expect any objections, but anyway it would be nice).

I cc'd the latest owners of plist.h ... although Daniel Walker has
apparently since left MontaVista, Thomas Gleixner is still current ...
and he can speak for the RT people, who are the primary plist users.

I can do another round and cc lkml, I was just hoping this would be the
last revision.

James

Re: [PATCH 3/3] pm_qos: get rid of the allocation in pm_qos_add_request()

[Rafael J. Wysocki]

On Tuesday, June 29, 2010, James Bottomley wrote:
> On Mon, 2010-06-28 at 23:59 +0200, Rafael J. Wysocki wrote:
> > On Monday, June 28, 2010, James Bottomley wrote:
> > > Since every caller has to squirrel away the returned pointer anyway,
> > > they might as well supply the memory area.  This fixes a bug in a few of
> > > the call sites where the returned pointer was dereferenced without
> > > checking it for NULL (which gets returned if the kzalloc failed).
> > > 
> > > I'd like to hear how sound and netdev feels about this: it will add
> > > about two more pointers worth of data to struct netdev and struct
> > > snd_pcm_substream .. but I think it's worth it.  If you're OK, I'll add
> > > your acks and send through the pm tree.
> > > 
> > > This also looks to me like an android independent clean up (even though
> > > it renders the request_add atomically callable).  I also added include
> > > guards to include/linux/pm_qos_params.h
> > > 
> > > cc: netdev@vger.kernel.org
> > > cc: Takashi Iwai <tiwai@suse.de>
> > > Signed-off-by: James Bottomley <James.Bottomley@suse.de>
> > 
> > I like all of the patches in this series, thanks a lot for doing this!
> > 
> > I guess it might be worth sending a CC to the LKML next round so that people
> > can see [1/3] (I don't expect any objections, but anyway it would be nice).
> 
> I cc'd the latest owners of plist.h ... although Daniel Walker has
> apparently since left MontaVista, Thomas Gleixner is still current ...
> and he can speak for the RT people, who are the primary plist users.
> 
> I can do another round and cc lkml, I was just hoping this would be the
> last revision.

OK, let's see if there's any feedback on [3/3] from netdev and Takashi.
If there's none, I'll just put the series into my linux-next branch.

Rafael

Re: [PATCH 3/3] pm_qos: get rid of the allocation in pm_qos_add_request()

[Daniel Walker]

On Mon, 2010-06-28 at 17:10 -0500, James Bottomley wrote:
> On Mon, 2010-06-28 at 23:59 +0200, Rafael J. Wysocki wrote:
> > On Monday, June 28, 2010, James Bottomley wrote:
> > > Since every caller has to squirrel away the returned pointer anyway,
> > > they might as well supply the memory area.  This fixes a bug in a few of
> > > the call sites where the returned pointer was dereferenced without
> > > checking it for NULL (which gets returned if the kzalloc failed).
> > > 
> > > I'd like to hear how sound and netdev feels about this: it will add
> > > about two more pointers worth of data to struct netdev and struct
> > > snd_pcm_substream .. but I think it's worth it.  If you're OK, I'll add
> > > your acks and send through the pm tree.
> > > 
> > > This also looks to me like an android independent clean up (even though
> > > it renders the request_add atomically callable).  I also added include
> > > guards to include/linux/pm_qos_params.h
> > > 
> > > cc: netdev@vger.kernel.org
> > > cc: Takashi Iwai <tiwai@suse.de>
> > > Signed-off-by: James Bottomley <James.Bottomley@suse.de>
> > 
> > I like all of the patches in this series, thanks a lot for doing this!
> > 
> > I guess it might be worth sending a CC to the LKML next round so that people
> > can see [1/3] (I don't expect any objections, but anyway it would be nice).
> 
> I cc'd the latest owners of plist.h ... although Daniel Walker has
> apparently since left MontaVista, Thomas Gleixner is still current ...
> and he can speak for the RT people, who are the primary plist users.
> 
> I can do another round and cc lkml, I was just hoping this would be the
> last revision.

I'm still paying attention tho .. I didn't see anything objection worthy
in the plist changes.. If you do send another round you might want to
add Oleg Nesterov , most of the code was redone by him ..

Daniel

Re: [PATCH 3/3] pm_qos: get rid of the allocation in pm_qos_add_request()

[mark gross]

On Mon, Jun 28, 2010 at 12:44:48PM -0500, James Bottomley wrote:
> Since every caller has to squirrel away the returned pointer anyway,
> they might as well supply the memory area.  This fixes a bug in a few of
> the call sites where the returned pointer was dereferenced without
> checking it for NULL (which gets returned if the kzalloc failed).
> 
> I'd like to hear how sound and netdev feels about this: it will add
> about two more pointers worth of data to struct netdev and struct
> snd_pcm_substream .. but I think it's worth it.  If you're OK, I'll add
> your acks and send through the pm tree.
> 
> This also looks to me like an android independent clean up (even though
> it renders the request_add atomically callable).  I also added include
> guards to include/linux/pm_qos_params.h
> 
> cc: netdev@vger.kernel.org
> cc: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: James Bottomley <James.Bottomley@suse.de>
Thank you for doing this!, I'll integrate it into some testing targets
in the morning!

Signed-off-by: mark gross <markgross@thegnar.org>

--mgross



> ---
>  drivers/net/e1000e/netdev.c            |   17 +++-----
>  drivers/net/igbvf/netdev.c             |    9 ++--
>  drivers/net/wireless/ipw2x00/ipw2100.c |   12 +++---
>  include/linux/netdevice.h              |    2 +-
>  include/linux/pm_qos_params.h          |   13 +++++-
>  include/sound/pcm.h                    |    2 +-
>  kernel/pm_qos_params.c                 |   67 +++++++++++++++++++-------------
>  sound/core/pcm_native.c                |   13 ++----
>  8 files changed, 74 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
> index 24507f3..47ea62f 100644
> --- a/drivers/net/e1000e/netdev.c
> +++ b/drivers/net/e1000e/netdev.c
> @@ -2901,10 +2901,10 @@ static void e1000_configure_rx(struct e1000_adapter *adapter)
>  			 * dropped transactions.
>  			 */
>  			pm_qos_update_request(
> -				adapter->netdev->pm_qos_req, 55);
> +				&adapter->netdev->pm_qos_req, 55);
>  		} else {
>  			pm_qos_update_request(
> -				adapter->netdev->pm_qos_req,
> +				&adapter->netdev->pm_qos_req,
>  				PM_QOS_DEFAULT_VALUE);
>  		}
>  	}
> @@ -3196,9 +3196,9 @@ int e1000e_up(struct e1000_adapter *adapter)
>  
>  	/* DMA latency requirement to workaround early-receive/jumbo issue */
>  	if (adapter->flags & FLAG_HAS_ERT)
> -		adapter->netdev->pm_qos_req =
> -			pm_qos_add_request(PM_QOS_CPU_DMA_LATENCY,
> -				       PM_QOS_DEFAULT_VALUE);
> +		pm_qos_add_request(&adapter->netdev->pm_qos_req,
> +				   PM_QOS_CPU_DMA_LATENCY,
> +				   PM_QOS_DEFAULT_VALUE);
>  
>  	/* hardware has been reset, we need to reload some things */
>  	e1000_configure(adapter);
> @@ -3263,11 +3263,8 @@ void e1000e_down(struct e1000_adapter *adapter)
>  	e1000_clean_tx_ring(adapter);
>  	e1000_clean_rx_ring(adapter);
>  
> -	if (adapter->flags & FLAG_HAS_ERT) {
> -		pm_qos_remove_request(
> -			      adapter->netdev->pm_qos_req);
> -		adapter->netdev->pm_qos_req = NULL;
> -	}
> +	if (adapter->flags & FLAG_HAS_ERT)
> +		pm_qos_remove_request(&adapter->netdev->pm_qos_req);
>  
>  	/*
>  	 * TODO: for power management, we could drop the link and
> diff --git a/drivers/net/igbvf/netdev.c b/drivers/net/igbvf/netdev.c
> index 5e2b2a8..add6197 100644
> --- a/drivers/net/igbvf/netdev.c
> +++ b/drivers/net/igbvf/netdev.c
> @@ -48,7 +48,7 @@
>  #define DRV_VERSION "1.0.0-k0"
>  char igbvf_driver_name[] = "igbvf";
>  const char igbvf_driver_version[] = DRV_VERSION;
> -struct pm_qos_request_list *igbvf_driver_pm_qos_req;
> +static struct pm_qos_request_list igbvf_driver_pm_qos_req;
>  static const char igbvf_driver_string[] =
>  				"Intel(R) Virtual Function Network Driver";
>  static const char igbvf_copyright[] = "Copyright (c) 2009 Intel Corporation.";
> @@ -2902,8 +2902,8 @@ static int __init igbvf_init_module(void)
>  	printk(KERN_INFO "%s\n", igbvf_copyright);
>  
>  	ret = pci_register_driver(&igbvf_driver);
> -	igbvf_driver_pm_qos_req = pm_qos_add_request(PM_QOS_CPU_DMA_LATENCY,
> -	                       PM_QOS_DEFAULT_VALUE);
> +	pm_qos_add_request(&igbvf_driver_pm_qos_req, PM_QOS_CPU_DMA_LATENCY,
> +			   PM_QOS_DEFAULT_VALUE);
>  
>  	return ret;
>  }
> @@ -2918,8 +2918,7 @@ module_init(igbvf_init_module);
>  static void __exit igbvf_exit_module(void)
>  {
>  	pci_unregister_driver(&igbvf_driver);
> -	pm_qos_remove_request(igbvf_driver_pm_qos_req);
> -	igbvf_driver_pm_qos_req = NULL;
> +	pm_qos_remove_request(&igbvf_driver_pm_qos_req);
>  }
>  module_exit(igbvf_exit_module);
>  
> diff --git a/drivers/net/wireless/ipw2x00/ipw2100.c b/drivers/net/wireless/ipw2x00/ipw2100.c
> index 0bd4dfa..7f0d98b 100644
> --- a/drivers/net/wireless/ipw2x00/ipw2100.c
> +++ b/drivers/net/wireless/ipw2x00/ipw2100.c
> @@ -174,7 +174,7 @@ that only one external action is invoked at a time.
>  #define DRV_DESCRIPTION	"Intel(R) PRO/Wireless 2100 Network Driver"
>  #define DRV_COPYRIGHT	"Copyright(c) 2003-2006 Intel Corporation"
>  
> -struct pm_qos_request_list *ipw2100_pm_qos_req;
> +struct pm_qos_request_list ipw2100_pm_qos_req;
>  
>  /* Debugging stuff */
>  #ifdef CONFIG_IPW2100_DEBUG
> @@ -1741,7 +1741,7 @@ static int ipw2100_up(struct ipw2100_priv *priv, int deferred)
>  	/* the ipw2100 hardware really doesn't want power management delays
>  	 * longer than 175usec
>  	 */
> -	pm_qos_update_request(ipw2100_pm_qos_req, 175);
> +	pm_qos_update_request(&ipw2100_pm_qos_req, 175);
>  
>  	/* If the interrupt is enabled, turn it off... */
>  	spin_lock_irqsave(&priv->low_lock, flags);
> @@ -1889,7 +1889,7 @@ static void ipw2100_down(struct ipw2100_priv *priv)
>  	ipw2100_disable_interrupts(priv);
>  	spin_unlock_irqrestore(&priv->low_lock, flags);
>  
> -	pm_qos_update_request(ipw2100_pm_qos_req, PM_QOS_DEFAULT_VALUE);
> +	pm_qos_update_request(&ipw2100_pm_qos_req, PM_QOS_DEFAULT_VALUE);
>  
>  	/* We have to signal any supplicant if we are disassociating */
>  	if (associated)
> @@ -6669,8 +6669,8 @@ static int __init ipw2100_init(void)
>  	if (ret)
>  		goto out;
>  
> -	ipw2100_pm_qos_req = pm_qos_add_request(PM_QOS_CPU_DMA_LATENCY,
> -			PM_QOS_DEFAULT_VALUE);
> +	pm_qos_add_request(&ipw2100_pm_qos_req, PM_QOS_CPU_DMA_LATENCY,
> +			   PM_QOS_DEFAULT_VALUE);
>  #ifdef CONFIG_IPW2100_DEBUG
>  	ipw2100_debug_level = debug;
>  	ret = driver_create_file(&ipw2100_pci_driver.driver,
> @@ -6692,7 +6692,7 @@ static void __exit ipw2100_exit(void)
>  			   &driver_attr_debug_level);
>  #endif
>  	pci_unregister_driver(&ipw2100_pci_driver);
> -	pm_qos_remove_request(ipw2100_pm_qos_req);
> +	pm_qos_remove_request(&ipw2100_pm_qos_req);
>  }
>  
>  module_init(ipw2100_init);
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 40291f3..393555a 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -779,7 +779,7 @@ struct net_device {
>  	 */
>  	char			name[IFNAMSIZ];
>  
> -	struct pm_qos_request_list *pm_qos_req;
> +	struct pm_qos_request_list pm_qos_req;
>  
>  	/* device name hash chain */
>  	struct hlist_node	name_hlist;
> diff --git a/include/linux/pm_qos_params.h b/include/linux/pm_qos_params.h
> index 8ba440e..77cbddb 100644
> --- a/include/linux/pm_qos_params.h
> +++ b/include/linux/pm_qos_params.h
> @@ -1,8 +1,10 @@
> +#ifndef _LINUX_PM_QOS_PARAMS_H
> +#define _LINUX_PM_QOS_PARAMS_H
>  /* interface for the pm_qos_power infrastructure of the linux kernel.
>   *
>   * Mark Gross <mgross@linux.intel.com>
>   */
> -#include <linux/list.h>
> +#include <linux/plist.h>
>  #include <linux/notifier.h>
>  #include <linux/miscdevice.h>
>  
> @@ -14,9 +16,12 @@
>  #define PM_QOS_NUM_CLASSES 4
>  #define PM_QOS_DEFAULT_VALUE -1
>  
> -struct pm_qos_request_list;
> +struct pm_qos_request_list {
> +	struct plist_node list;
> +	int pm_qos_class;
> +};
>  
> -struct pm_qos_request_list *pm_qos_add_request(int pm_qos_class, s32 value);
> +void pm_qos_add_request(struct pm_qos_request_list *l, int pm_qos_class, s32 value);
>  void pm_qos_update_request(struct pm_qos_request_list *pm_qos_req,
>  		s32 new_value);
>  void pm_qos_remove_request(struct pm_qos_request_list *pm_qos_req);
> @@ -24,4 +29,6 @@ void pm_qos_remove_request(struct pm_qos_request_list *pm_qos_req);
>  int pm_qos_request(int pm_qos_class);
>  int pm_qos_add_notifier(int pm_qos_class, struct notifier_block *notifier);
>  int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *notifier);
> +int pm_qos_request_active(struct pm_qos_request_list *req);
>  
> +#endif
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index dd76cde..6e3a297 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -366,7 +366,7 @@ struct snd_pcm_substream {
>  	int number;
>  	char name[32];			/* substream name */
>  	int stream;			/* stream (direction) */
> -	struct pm_qos_request_list *latency_pm_qos_req; /* pm_qos request */
> +	struct pm_qos_request_list latency_pm_qos_req; /* pm_qos request */
>  	size_t buffer_bytes_max;	/* limit ring buffer size */
>  	struct snd_dma_buffer dma_buffer;
>  	unsigned int dma_buf_id;
> diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> index b130b97..bff4053 100644
> --- a/kernel/pm_qos_params.c
> +++ b/kernel/pm_qos_params.c
> @@ -30,7 +30,6 @@
>  /*#define DEBUG*/
>  
>  #include <linux/pm_qos_params.h>
> -#include <linux/plist.h>
>  #include <linux/sched.h>
>  #include <linux/spinlock.h>
>  #include <linux/slab.h>
> @@ -49,11 +48,6 @@
>   * or pm_qos_object list and pm_qos_objects need to happen with pm_qos_lock
>   * held, taken with _irqsave.  One lock to rule them all
>   */
> -struct pm_qos_request_list {
> -	struct plist_node list;
> -	int pm_qos_class;
> -};
> -
>  enum pm_qos_type {
>  	PM_QOS_MAX,		/* return the largest value */
>  	PM_QOS_MIN		/* return the smallest value */
> @@ -210,6 +204,12 @@ int pm_qos_request(int pm_qos_class)
>  }
>  EXPORT_SYMBOL_GPL(pm_qos_request);
>  
> +int pm_qos_request_active(struct pm_qos_request_list *req)
> +{
> +	return req->pm_qos_class != 0;
> +}
> +EXPORT_SYMBOL_GPL(pm_qos_request_active);
> +
>  /**
>   * pm_qos_add_request - inserts new qos request into the list
>   * @pm_qos_class: identifies which list of qos request to us
> @@ -221,25 +221,23 @@ EXPORT_SYMBOL_GPL(pm_qos_request);
>   * element as a handle for use in updating and removal.  Call needs to save
>   * this handle for later use.
>   */
> -struct pm_qos_request_list *pm_qos_add_request(int pm_qos_class, s32 value)
> +void pm_qos_add_request(struct pm_qos_request_list *dep,
> +			int pm_qos_class, s32 value)
>  {
> -	struct pm_qos_request_list *dep;
> -
> -	dep = kzalloc(sizeof(struct pm_qos_request_list), GFP_KERNEL);
> -	if (dep) {
> -		struct pm_qos_object *o =  pm_qos_array[pm_qos_class];
> -		int new_value;
> -
> -		if (value == PM_QOS_DEFAULT_VALUE)
> -			new_value = o->default_value;
> -		else
> -			new_value = value;
> -		plist_node_init(&dep->list, new_value);
> -		dep->pm_qos_class = pm_qos_class;
> -		update_target(o, &dep->list, 0, PM_QOS_DEFAULT_VALUE);
> -	}
> +	struct pm_qos_object *o =  pm_qos_array[pm_qos_class];
> +	int new_value;
>  
> -	return dep;
> +	if (pm_qos_request_active(dep)) {
> +		WARN(1, KERN_ERR "pm_qos_add_request() called for already added request\n");
> +		return;
> +	}
> +	if (value == PM_QOS_DEFAULT_VALUE)
> +		new_value = o->default_value;
> +	else
> +		new_value = value;
> +	plist_node_init(&dep->list, new_value);
> +	dep->pm_qos_class = pm_qos_class;
> +	update_target(o, &dep->list, 0, PM_QOS_DEFAULT_VALUE);
>  }
>  EXPORT_SYMBOL_GPL(pm_qos_add_request);
>  
> @@ -262,6 +260,11 @@ void pm_qos_update_request(struct pm_qos_request_list *pm_qos_req,
>  	if (!pm_qos_req) /*guard against callers passing in null */
>  		return;
>  
> +	if (pm_qos_request_active(pm_qos_req)) {
> +		WARN(1, KERN_ERR "pm_qos_update_request() called for unknown object\n");
> +		return;
> +	}
> +
>  	o = pm_qos_array[pm_qos_req->pm_qos_class];
>  
>  	if (new_value == PM_QOS_DEFAULT_VALUE)
> @@ -290,9 +293,14 @@ void pm_qos_remove_request(struct pm_qos_request_list *pm_qos_req)
>  		return;
>  		/* silent return to keep pcm code cleaner */
>  
> +	if (!pm_qos_request_active(pm_qos_req)) {
> +		WARN(1, KERN_ERR "pm_qos_remove_request() called for unknown object\n");
> +		return;
> +	}
> +
>  	o = pm_qos_array[pm_qos_req->pm_qos_class];
>  	update_target(o, &pm_qos_req->list, 1, PM_QOS_DEFAULT_VALUE);
> -	kfree(pm_qos_req);
> +	memset(pm_qos_req, 0, sizeof(*pm_qos_req));
>  }
>  EXPORT_SYMBOL_GPL(pm_qos_remove_request);
>  
> @@ -340,8 +348,12 @@ static int pm_qos_power_open(struct inode *inode, struct file *filp)
>  
>  	pm_qos_class = find_pm_qos_object_by_minor(iminor(inode));
>  	if (pm_qos_class >= 0) {
> -		filp->private_data = (void *) pm_qos_add_request(pm_qos_class,
> -				PM_QOS_DEFAULT_VALUE);
> +		struct pm_qos_request_list *req = kzalloc(GFP_KERNEL, sizeof(*req));
> +		if (!req)
> +			return -ENOMEM;
> +
> +		pm_qos_add_request(req, pm_qos_class, PM_QOS_DEFAULT_VALUE);
> +		filp->private_data = req;
>  
>  		if (filp->private_data)
>  			return 0;
> @@ -353,8 +365,9 @@ static int pm_qos_power_release(struct inode *inode, struct file *filp)
>  {
>  	struct pm_qos_request_list *req;
>  
> -	req = (struct pm_qos_request_list *)filp->private_data;
> +	req = filp->private_data;
>  	pm_qos_remove_request(req);
> +	kfree(req);
>  
>  	return 0;
>  }
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index 303ac04..a3b2a64 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -451,13 +451,11 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream,
>  	snd_pcm_timer_resolution_change(substream);
>  	runtime->status->state = SNDRV_PCM_STATE_SETUP;
>  
> -	if (substream->latency_pm_qos_req) {
> -		pm_qos_remove_request(substream->latency_pm_qos_req);
> -		substream->latency_pm_qos_req = NULL;
> -	}
> +	if (pm_qos_request_active(&substream->latency_pm_qos_req))
> +		pm_qos_remove_request(&substream->latency_pm_qos_req);
>  	if ((usecs = period_to_usecs(runtime)) >= 0)
> -		substream->latency_pm_qos_req = pm_qos_add_request(
> -					PM_QOS_CPU_DMA_LATENCY, usecs);
> +		pm_qos_add_request(&substream->latency_pm_qos_req,
> +				   PM_QOS_CPU_DMA_LATENCY, usecs);
>  	return 0;
>   _error:
>  	/* hardware might be unuseable from this time,
> @@ -512,8 +510,7 @@ static int snd_pcm_hw_free(struct snd_pcm_substream *substream)
>  	if (substream->ops->hw_free)
>  		result = substream->ops->hw_free(substream);
>  	runtime->status->state = SNDRV_PCM_STATE_OPEN;
> -	pm_qos_remove_request(substream->latency_pm_qos_req);
> -	substream->latency_pm_qos_req = NULL;
> +	pm_qos_remove_request(&substream->latency_pm_qos_req);
>  	return result;
>  }
>  
> -- 
> 1.6.4.2
> 
> 
> 

Re: [PATCH 3/3] pm_qos: get rid of the allocation in pm_qos_add_request()

[Rafael J. Wysocki]

On Tuesday, June 29, 2010, mark gross wrote:
> On Mon, Jun 28, 2010 at 12:44:48PM -0500, James Bottomley wrote:
> > Since every caller has to squirrel away the returned pointer anyway,
> > they might as well supply the memory area.  This fixes a bug in a few of
> > the call sites where the returned pointer was dereferenced without
> > checking it for NULL (which gets returned if the kzalloc failed).
> > 
> > I'd like to hear how sound and netdev feels about this: it will add
> > about two more pointers worth of data to struct netdev and struct
> > snd_pcm_substream .. but I think it's worth it.  If you're OK, I'll add
> > your acks and send through the pm tree.
> > 
> > This also looks to me like an android independent clean up (even though
> > it renders the request_add atomically callable).  I also added include
> > guards to include/linux/pm_qos_params.h
> > 
> > cc: netdev@vger.kernel.org
> > cc: Takashi Iwai <tiwai@suse.de>
> > Signed-off-by: James Bottomley <James.Bottomley@suse.de>
> Thank you for doing this!, I'll integrate it into some testing targets
> in the morning!
> 
> Signed-off-by: mark gross <markgross@thegnar.org>

I would apply this one too, but I need a final changelog for it.  Care to send?

Rafael

Re: [PATCH 3/3] pm_qos: get rid of the allocation in pm_qos_add_request()

[James Bottomley]

On Fri, 2010-07-02 at 00:23 +0200, Rafael J. Wysocki wrote:
> I would apply this one too, but I need a final changelog for it.  Care to send?

How about:

All current users of pm_qos_add_request() have the ability to supply the
memory required by the pm_qos routines, so make them do this and
eliminate the kmalloc() with pm_qos_add_request().  This has the double
benefit of making the call never fail and allowing it to be called from
atomic context.

+ signoffs

James

Re: [PATCH 3/3] pm_qos: get rid of the allocation in pm_qos_add_request()

[Rafael J. Wysocki]

On Friday, July 02, 2010, James Bottomley wrote:
> On Fri, 2010-07-02 at 00:23 +0200, Rafael J. Wysocki wrote:
> > I would apply this one too, but I need a final changelog for it.  Care to send?
> 
> How about:
> 
> All current users of pm_qos_add_request() have the ability to supply the
> memory required by the pm_qos routines, so make them do this and
> eliminate the kmalloc() with pm_qos_add_request().  This has the double
> benefit of making the call never fail and allowing it to be called from
> atomic context.
> 
> + signoffs

OK

I'll apply it shortly.

Rafael

Re: [PATCH 3/3] pm_qos: get rid of the allocation in pm_qos_add_request()

[Takashi Iwai]

Hi,

sorry for the late reply, as I've been on vacation in the last week
(and shut off mails intentionally :)

At Mon, 28 Jun 2010 12:44:48 -0500,
James Bottomley wrote:
> 
> Since every caller has to squirrel away the returned pointer anyway,
> they might as well supply the memory area.  This fixes a bug in a few of
> the call sites where the returned pointer was dereferenced without
> checking it for NULL (which gets returned if the kzalloc failed).
> 
> I'd like to hear how sound and netdev feels about this: it will add
> about two more pointers worth of data to struct netdev and struct
> snd_pcm_substream .. but I think it's worth it.  If you're OK, I'll add
> your acks and send through the pm tree.
> 
> This also looks to me like an android independent clean up (even though
> it renders the request_add atomically callable).  I also added include
> guards to include/linux/pm_qos_params.h

I like the patch very well, too.
But, just wondering...

> @@ -262,6 +260,11 @@ void pm_qos_update_request(struct pm_qos_request_list *pm_qos_req,
>  	if (!pm_qos_req) /*guard against callers passing in null */
>  		return;
>  
> +	if (pm_qos_request_active(pm_qos_req)) {
> +		WARN(1, KERN_ERR "pm_qos_update_request() called for unknown object\n");
> +		return;
> +	}
> +

Is this correct...?  Shouldn't it be a negative check?


thanks,

Takashi

Re: [PATCH 3/3] pm_qos: get rid of the allocation in pm_qos_add_request()

[James Bottomley]

On Mon, 2010-07-05 at 08:41 +0200, Takashi Iwai wrote:
> sorry for the late reply, as I've been on vacation in the last week
> (and shut off mails intentionally :)

Envy forbids me from saying that's OK.

> At Mon, 28 Jun 2010 12:44:48 -0500,
> James Bottomley wrote:
> > 
> > Since every caller has to squirrel away the returned pointer anyway,
> > they might as well supply the memory area.  This fixes a bug in a few of
> > the call sites where the returned pointer was dereferenced without
> > checking it for NULL (which gets returned if the kzalloc failed).
> > 
> > I'd like to hear how sound and netdev feels about this: it will add
> > about two more pointers worth of data to struct netdev and struct
> > snd_pcm_substream .. but I think it's worth it.  If you're OK, I'll add
> > your acks and send through the pm tree.
> > 
> > This also looks to me like an android independent clean up (even though
> > it renders the request_add atomically callable).  I also added include
> > guards to include/linux/pm_qos_params.h
> 
> I like the patch very well, too.
> But, just wondering...
> 
> > @@ -262,6 +260,11 @@ void pm_qos_update_request(struct pm_qos_request_list *pm_qos_req,
> >  	if (!pm_qos_req) /*guard against callers passing in null */
> >  		return;
> >  
> > +	if (pm_qos_request_active(pm_qos_req)) {
> > +		WARN(1, KERN_ERR "pm_qos_update_request() called for unknown object\n");
> > +		return;
> > +	}
> > +
> 
> Is this correct...?  Shouldn't it be a negative check?

Yes, it should be a negative check ... I'll update the patch.  I guess
this still means that no-one has managed to test it on a functional
system ...

James

Re: [PATCH 3/3] pm_qos: get rid of the allocation in pm_qos_add_request()

[mark gross]

On Mon, Jul 05, 2010 at 09:02:48AM -0500, James Bottomley wrote:
> On Mon, 2010-07-05 at 08:41 +0200, Takashi Iwai wrote:
> > sorry for the late reply, as I've been on vacation in the last week
> > (and shut off mails intentionally :)
> 
> Envy forbids me from saying that's OK.
> 
> > At Mon, 28 Jun 2010 12:44:48 -0500,
> > James Bottomley wrote:
> > > 
> > > Since every caller has to squirrel away the returned pointer anyway,
> > > they might as well supply the memory area.  This fixes a bug in a few of
> > > the call sites where the returned pointer was dereferenced without
> > > checking it for NULL (which gets returned if the kzalloc failed).
> > > 
> > > I'd like to hear how sound and netdev feels about this: it will add
> > > about two more pointers worth of data to struct netdev and struct
> > > snd_pcm_substream .. but I think it's worth it.  If you're OK, I'll add
> > > your acks and send through the pm tree.
> > > 
> > > This also looks to me like an android independent clean up (even though
> > > it renders the request_add atomically callable).  I also added include
> > > guards to include/linux/pm_qos_params.h
> > 
> > I like the patch very well, too.
> > But, just wondering...
> > 
> > > @@ -262,6 +260,11 @@ void pm_qos_update_request(struct pm_qos_request_list *pm_qos_req,
> > >  	if (!pm_qos_req) /*guard against callers passing in null */
> > >  		return;
> > >  
> > > +	if (pm_qos_request_active(pm_qos_req)) {
> > > +		WARN(1, KERN_ERR "pm_qos_update_request() called for unknown object\n");
> > > +		return;
> > > +	}
> > > +
> > 
> > Is this correct...?  Shouldn't it be a negative check?
> 
> Yes, it should be a negative check ... I'll update the patch.  I guess
> this still means that no-one has managed to test it on a functional
> system ...
> 

well I guess that explains the warning I got on my back port of this
patch.


[   62.944788] ------------[ cut here ]------------                                        
[   62.944788] WARNING: at kernel/pm_qos_params.c:266                                      
pm_qos_update_request+0x21/0x46)                                                           
[   62.944788] pm_qos_update_request() called for unknown object                           
[   62.944788] Modules linked in: mrst_sspi cfspi_slave chnl_chr                           
caif_chr chnl_net caf                                                                      
[   62.944788] Pid: 91, comm: mrst/0 Tainted: G        W  2.6.31.6-mrst
#30                
[   62.944788] Call Trace:                                                                 
[   62.944788]  [<c0145b2e>] ? pm_qos_update_request+0x21/0x46                             
[   62.944788]  [<c012fff3>] warn_slowpath_common+0x60/0x77                                
[   62.944788]  [<c013003e>] warn_slowpath_fmt+0x24/0x27                                   
[   62.944788]  [<c0145b2e>] pm_qos_update_request+0x21/0x46                               
[   62.944788]  [<c03029f2>] int_transfer_complete_work+0x19/0x65                          
[   62.944788]  [<c013f02a>] worker_thread+0x153/0x1df                                     
[   62.944788]  [<c03029d9>] ? int_transfer_complete_work+0x0/0x65                         
[   62.944788]  [<c0141df1>] ? autoremove_wake_function+0x0/0x30                           
[   62.944788]  [<c0141c7c>] kthread+0x64/0x69                                             
[   62.944788]  [<c013eed7>] ? worker_thread+0x0/0x1df                                     
[   62.944788]  [<c0141c18>] ? kthread+0x0/0x69                                            
[   62.944788]  [<c01034df>] kernel_thread_helper+0x7/0x10                                 
[   62.944788] ---[ end trace 1723851b79e06c5d ]---                                        
[   62.944788] ------------[ cut here ]------------                                        
[   62.944788] WARNING: at kernel/pm_qos_params.c:266          

Sorry, I've been swamped by work and personal things the past 2 weeks.

--mgross

Re: [PATCH 3/3] pm_qos: get rid of the allocation in pm_qos_add_request()

[Rafael J. Wysocki]

On Monday, July 05, 2010, James Bottomley wrote:
> On Mon, 2010-07-05 at 08:41 +0200, Takashi Iwai wrote:
> > sorry for the late reply, as I've been on vacation in the last week
> > (and shut off mails intentionally :)
> 
> Envy forbids me from saying that's OK.
> 
> > At Mon, 28 Jun 2010 12:44:48 -0500,
> > James Bottomley wrote:
> > > 
> > > Since every caller has to squirrel away the returned pointer anyway,
> > > they might as well supply the memory area.  This fixes a bug in a few of
> > > the call sites where the returned pointer was dereferenced without
> > > checking it for NULL (which gets returned if the kzalloc failed).
> > > 
> > > I'd like to hear how sound and netdev feels about this: it will add
> > > about two more pointers worth of data to struct netdev and struct
> > > snd_pcm_substream .. but I think it's worth it.  If you're OK, I'll add
> > > your acks and send through the pm tree.
> > > 
> > > This also looks to me like an android independent clean up (even though
> > > it renders the request_add atomically callable).  I also added include
> > > guards to include/linux/pm_qos_params.h
> > 
> > I like the patch very well, too.
> > But, just wondering...
> > 
> > > @@ -262,6 +260,11 @@ void pm_qos_update_request(struct pm_qos_request_list *pm_qos_req,
> > >  	if (!pm_qos_req) /*guard against callers passing in null */
> > >  		return;
> > >  
> > > +	if (pm_qos_request_active(pm_qos_req)) {
> > > +		WARN(1, KERN_ERR "pm_qos_update_request() called for unknown object\n");
> > > +		return;
> > > +	}
> > > +
> > 
> > Is this correct...?  Shouldn't it be a negative check?
> 
> Yes, it should be a negative check ... I'll update the patch.

I've already fixed it in my tree.

> I guess this still means that no-one has managed to test it on a functional
> system ...

Well, it's been for a while in linux-next ...

Rafael

Re: [PATCH 3/3] pm_qos: get rid of the allocation in pm_qos_add_request()

[James Bottomley]

On Mon, 2010-07-05 at 23:07 +0200, Rafael J. Wysocki wrote:
> On Monday, July 05, 2010, James Bottomley wrote:
> > On Mon, 2010-07-05 at 08:41 +0200, Takashi Iwai wrote:
> > > sorry for the late reply, as I've been on vacation in the last week
> > > (and shut off mails intentionally :)
> > 
> > Envy forbids me from saying that's OK.
> > 
> > > At Mon, 28 Jun 2010 12:44:48 -0500,
> > > James Bottomley wrote:
> > > > 
> > > > Since every caller has to squirrel away the returned pointer anyway,
> > > > they might as well supply the memory area.  This fixes a bug in a few of
> > > > the call sites where the returned pointer was dereferenced without
> > > > checking it for NULL (which gets returned if the kzalloc failed).
> > > > 
> > > > I'd like to hear how sound and netdev feels about this: it will add
> > > > about two more pointers worth of data to struct netdev and struct
> > > > snd_pcm_substream .. but I think it's worth it.  If you're OK, I'll add
> > > > your acks and send through the pm tree.
> > > > 
> > > > This also looks to me like an android independent clean up (even though
> > > > it renders the request_add atomically callable).  I also added include
> > > > guards to include/linux/pm_qos_params.h
> > > 
> > > I like the patch very well, too.
> > > But, just wondering...
> > > 
> > > > @@ -262,6 +260,11 @@ void pm_qos_update_request(struct pm_qos_request_list *pm_qos_req,
> > > >  	if (!pm_qos_req) /*guard against callers passing in null */
> > > >  		return;
> > > >  
> > > > +	if (pm_qos_request_active(pm_qos_req)) {
> > > > +		WARN(1, KERN_ERR "pm_qos_update_request() called for unknown object\n");
> > > > +		return;
> > > > +	}
> > > > +
> > > 
> > > Is this correct...?  Shouldn't it be a negative check?
> > 
> > Yes, it should be a negative check ... I'll update the patch.
> 
> I've already fixed it in my tree.

Ah, OK, thanks ... so that would explain why we haven't been getting
floods of reports (instead of me thinking no-one has tested it).

> > I guess this still means that no-one has managed to test it on a functional
> > system ...
> 
> Well, it's been for a while in linux-next ...

So here's hoping ...

James