[alsa-devel] [RFC] pm_qos: get rid of the allocation in pm_qos_add_request()

mark gross 640e9920 at gmail.com
Sun Jun 6 23:32:24 CEST 2010


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);
>  		} 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 at 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;
> -		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));

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;
>  }
>  
> 
> 


More information about the Alsa-devel mailing list