[alsa-devel] [RFC] pm_qos: get rid of the allocation in pm_qos_add_request()
[alsa-devel says it's a moderated list, so feel free to drop before replying]
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
James
---
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 | 12 +++++-- include/sound/pcm.h | 2 +- kernel/pm_qos_params.c | 55 ++++++++++++++++--------------- sound/core/pcm_native.c | 12 ++----- 8 files changed, 60 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..5da569f 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; +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..d823cc0 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); @@ -25,3 +30,4 @@ 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);
+#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 241fa79..f1d3d23 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 */ @@ -195,6 +189,11 @@ int pm_qos_request(int pm_qos_class) } EXPORT_SYMBOL_GPL(pm_qos_request);
+static int pm_qos_request_active(struct pm_qos_request_list *req) +{ + return req->pm_qos_class != 0; +} + /** * pm_qos_add_request - inserts new qos request into the list * @pm_qos_class: identifies which list of qos request to us @@ -206,25 +205,22 @@ 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); - } + struct pm_qos_object *o = pm_qos_array[pm_qos_class]; + int new_value; + + if (pm_qos_request_active(dep)) + return;
- return dep; + 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); } EXPORT_SYMBOL_GPL(pm_qos_add_request);
@@ -286,7 +282,7 @@ void pm_qos_remove_request(struct pm_qos_request_list *pm_qos_req)
o = pm_qos_array[pm_qos_req->pm_qos_class]; update_target(o, &pm_qos_req->list, 1); - kfree(pm_qos_req); + memset(pm_qos_req, 0, sizeof(*pm_qos_req)); } EXPORT_SYMBOL_GPL(pm_qos_remove_request);
@@ -334,8 +330,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; @@ -347,8 +347,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..d3b8b51 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -451,13 +451,10 @@ 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; - } + 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 +509,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; }
On Sat, Jun 05, 2010 at 02:20:14PM -0500, James Bottomley wrote:
[alsa-devel says it's a moderated list, so feel free to drop before replying]
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
James
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 | 12 +++++-- include/sound/pcm.h | 2 +- kernel/pm_qos_params.c | 55 ++++++++++++++++--------------- sound/core/pcm_native.c | 12 ++----- 8 files changed, 60 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);
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..5da569f 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; +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..d823cc0 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;
+};
so the test for an un-registerd or in-initialized request is if list == null.
-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); @@ -25,3 +30,4 @@ 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);
+#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 241fa79..f1d3d23 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>
? plist pm_qos isn't yet in the code base yet. ;) Is this patch assumed after your RFC patch? It must be....
#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 */ @@ -195,6 +189,11 @@ int pm_qos_request(int pm_qos_class) } EXPORT_SYMBOL_GPL(pm_qos_request);
+static int pm_qos_request_active(struct pm_qos_request_list *req) +{
- return req->pm_qos_class != 0;
+}
/**
- pm_qos_add_request - inserts new qos request into the list
- @pm_qos_class: identifies which list of qos request to us
@@ -206,25 +205,22 @@ 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;elsenew_value = value;plist_node_init(&dep->list, new_value);dep->pm_qos_class = pm_qos_class;update_target(o, &dep->list, 0);- }
- struct pm_qos_object *o = pm_qos_array[pm_qos_class];
- int new_value;
- if (pm_qos_request_active(dep))
return;
- return dep;
- 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);
} EXPORT_SYMBOL_GPL(pm_qos_add_request);
@@ -286,7 +282,7 @@ void pm_qos_remove_request(struct pm_qos_request_list *pm_qos_req)
o = pm_qos_array[pm_qos_req->pm_qos_class]; update_target(o, &pm_qos_req->list, 1);
- kfree(pm_qos_req);
- memset(pm_qos_req, 0, sizeof(*pm_qos_req));
I was wondering how to tell if a pm_qos_request was un-initialized un-registered request.
} EXPORT_SYMBOL_GPL(pm_qos_remove_request);
@@ -334,8 +330,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;
@@ -347,8 +347,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..d3b8b51 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -451,13 +451,10 @@ 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;- }
- 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);
How are we going to avoid re-adding the latency_pm_qos_request multiple times to the list? the last time I looked at this I really wanted to change this to update_request. But, I got pushback so I added the file gard in pmqos_params.c instead.
--mgross
return 0; _error: /* hardware might be unuseable from this time, @@ -512,8 +509,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;
}
On Sun, 2010-06-06 at 14:32 -0700, mark gross wrote:
so the test for an un-registerd or in-initialized request is if list == null.
Actually I was using pm_qos_class == 0, but all current users use NULL as the pointer test, so they must all be allocated in zero initialised memory.
-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); @@ -25,3 +30,4 @@ 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);
+#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 241fa79..f1d3d23 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>
? plist pm_qos isn't yet in the code base yet. ;) Is this patch assumed after your RFC patch? It must be....
Yes, they're stacked.
#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 */ @@ -195,6 +189,11 @@ int pm_qos_request(int pm_qos_class) } EXPORT_SYMBOL_GPL(pm_qos_request);
+static int pm_qos_request_active(struct pm_qos_request_list *req) +{
- return req->pm_qos_class != 0;
+}
/**
- pm_qos_add_request - inserts new qos request into the list
- @pm_qos_class: identifies which list of qos request to us
@@ -206,25 +205,22 @@ 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;elsenew_value = value;plist_node_init(&dep->list, new_value);dep->pm_qos_class = pm_qos_class;update_target(o, &dep->list, 0);- }
- struct pm_qos_object *o = pm_qos_array[pm_qos_class];
- int new_value;
- if (pm_qos_request_active(dep))
return;
- return dep;
- 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);
} EXPORT_SYMBOL_GPL(pm_qos_add_request);
@@ -286,7 +282,7 @@ void pm_qos_remove_request(struct pm_qos_request_list *pm_qos_req)
o = pm_qos_array[pm_qos_req->pm_qos_class]; update_target(o, &pm_qos_req->list, 1);
- kfree(pm_qos_req);
- memset(pm_qos_req, 0, sizeof(*pm_qos_req));
I was wondering how to tell if a pm_qos_request was un-initialized un-registered request.
The assumption is zero initialised.
} EXPORT_SYMBOL_GPL(pm_qos_remove_request);
@@ -334,8 +330,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;
@@ -347,8 +347,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..d3b8b51 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -451,13 +451,10 @@ 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;- }
- 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);How are we going to avoid re-adding the latency_pm_qos_request multiple times to the list? the last time I looked at this I really wanted to change this to update_request. But, I got pushback so I added the file gard in pmqos_params.c instead.
There's a guard check in add that does this ... it seemed better putting it there than replicating through the subsystems.
James
On Sat, Jun 05, 2010 at 02:20:14PM -0500, James Bottomley wrote:
[alsa-devel says it's a moderated list, so feel free to drop before replying]
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
James
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 | 12 +++++-- include/sound/pcm.h | 2 +- kernel/pm_qos_params.c | 55 ++++++++++++++++--------------- sound/core/pcm_native.c | 12 ++----- 8 files changed, 60 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);
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..5da569f 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; +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,
should be:
+ pm_qos_add_request(&igbvf_driver_pm_qos_req, PM_QOS_CPU_DMA_LATENCY,
--mgross
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..d823cc0 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); @@ -25,3 +30,4 @@ 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);
+#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 241fa79..f1d3d23 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 */ @@ -195,6 +189,11 @@ int pm_qos_request(int pm_qos_class) } EXPORT_SYMBOL_GPL(pm_qos_request);
+static int pm_qos_request_active(struct pm_qos_request_list *req) +{
- return req->pm_qos_class != 0;
+}
/**
- pm_qos_add_request - inserts new qos request into the list
- @pm_qos_class: identifies which list of qos request to us
@@ -206,25 +205,22 @@ 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;elsenew_value = value;plist_node_init(&dep->list, new_value);dep->pm_qos_class = pm_qos_class;update_target(o, &dep->list, 0);- }
- struct pm_qos_object *o = pm_qos_array[pm_qos_class];
- int new_value;
- if (pm_qos_request_active(dep))
return;
- return dep;
- 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);
} EXPORT_SYMBOL_GPL(pm_qos_add_request);
@@ -286,7 +282,7 @@ void pm_qos_remove_request(struct pm_qos_request_list *pm_qos_req)
o = pm_qos_array[pm_qos_req->pm_qos_class]; update_target(o, &pm_qos_req->list, 1);
- kfree(pm_qos_req);
- memset(pm_qos_req, 0, sizeof(*pm_qos_req));
} EXPORT_SYMBOL_GPL(pm_qos_remove_request);
@@ -334,8 +330,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;
@@ -347,8 +347,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..d3b8b51 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -451,13 +451,10 @@ 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;- }
- 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);@@ -512,8 +509,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;
}
On Sun, 2010-06-06 at 16:10 -0700, mark gross wrote:
On Sat, Jun 05, 2010 at 02:20:14PM -0500, James Bottomley wrote:
[alsa-devel says it's a moderated list, so feel free to drop before replying]
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
James
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 | 12 +++++-- include/sound/pcm.h | 2 +- kernel/pm_qos_params.c | 55 ++++++++++++++++--------------- sound/core/pcm_native.c | 12 ++----- 8 files changed, 60 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);
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..5da569f 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; +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,
should be:
- pm_qos_add_request(&igbvf_driver_pm_qos_req, PM_QOS_CPU_DMA_LATENCY,
Yes, thanks ... must be not compiling this driver for some reason in the test case.
James
On Sun, Jun 06, 2010 at 10:50:19PM -0400, James Bottomley wrote:
On Sun, 2010-06-06 at 16:10 -0700, mark gross wrote:
On Sat, Jun 05, 2010 at 02:20:14PM -0500, James Bottomley wrote:
[alsa-devel says it's a moderated list, so feel free to drop before replying]
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
James
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 | 12 +++++-- include/sound/pcm.h | 2 +- kernel/pm_qos_params.c | 55 ++++++++++++++++--------------- sound/core/pcm_native.c | 12 ++----- 8 files changed, 60 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);
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..5da569f 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; +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,
should be:
- pm_qos_add_request(&igbvf_driver_pm_qos_req, PM_QOS_CPU_DMA_LATENCY,
Yes, thanks ... must be not compiling this driver for some reason in the test case.
I think we need to do better with the file gard or fix e100e/netdev.c to avoid panicking when un-loading the e1000e driver.
I'm on the fence whether to address the e1000e brain damage, or put in protection from whatever is causing the crash. (which funny enough I can't get a serial console to that crashing system to share it with you. but I'm pretty sure an un-initialized netdev->pm_qos_request is getting passed in (with class==0) resulting in the de-referencing of NULL in the update_target function.
I'll be other abusers are doing something similar we'll need to guard again. Would you like me to send an updated patch with some of this guarding in place?
--mgross
participants (2)
-
James Bottomley -
mark gross