[alsa-devel] [PATCH 1/9] ALSA: Compress - dont use lock for all ioctls

[Takashi Iwai]

At Tue, 27 Aug 2013 12:10:31 +0530,
Vinod Koul wrote:
> 
> Some simple ioctls like timsetamp query, capabities query can be done anytime
> and should not be under the stream lock. Move these to
> snd_compress_simple_iotcls() which is invoked without lock held
> 
> While at it, improve readblity a bit by sprinkling some empty lines
> 
> Signed-off-by: Vinod Koul <vinod.koul at intel.com>
> Cc: <stable at vger.kernel.org>

Why it's needed for stable?  Fixing any real bugs?


> ---
>  sound/core/compress_offload.c |   79 ++++++++++++++++++++++++++++++-----------
>  1 files changed, 58 insertions(+), 21 deletions(-)
> 
> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> index 99db892..868aefd 100644
> --- a/sound/core/compress_offload.c
> +++ b/sound/core/compress_offload.c
> @@ -729,70 +729,107 @@ static int snd_compr_partial_drain(struct snd_compr_stream *stream)
>  	return retval;
>  }
>  
> -static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
> +static int snd_compress_simple_ioctls(struct file *file,
> +				struct snd_compr_stream *stream,
> +				unsigned int cmd, unsigned long arg)
>  {
> -	struct snd_compr_file *data = f->private_data;
> -	struct snd_compr_stream *stream;
>  	int retval = -ENOTTY;
>  
> -	if (snd_BUG_ON(!data))
> -		return -EFAULT;
> -	stream = &data->stream;
> -	if (snd_BUG_ON(!stream))
> -		return -EFAULT;
> -	mutex_lock(&stream->device->lock);
>  	switch (_IOC_NR(cmd)) {
>  	case _IOC_NR(SNDRV_COMPRESS_IOCTL_VERSION):
>  		put_user(SNDRV_COMPRESS_VERSION,
>  				(int __user *)arg) ? -EFAULT : 0;
>  		break;
> +
>  	case _IOC_NR(SNDRV_COMPRESS_GET_CAPS):
>  		retval = snd_compr_get_caps(stream, arg);
>  		break;
> +
>  	case _IOC_NR(SNDRV_COMPRESS_GET_CODEC_CAPS):
>  		retval = snd_compr_get_codec_caps(stream, arg);
>  		break;
> +
> +	case _IOC_NR(SNDRV_COMPRESS_TSTAMP):
> +		retval = snd_compr_tstamp(stream, arg);
> +		break;
> +
> +	case _IOC_NR(SNDRV_COMPRESS_AVAIL):
> +		retval = snd_compr_ioctl_avail(stream, arg);
> +		break;
> +
> +		/* drain and partial drain need special handling
> +	 * we need to drop the locks here as the streams would get blocked on the
> +	 * dsp to get drained. The locking would be handled in respective
> +	 * function here
> +	 */
> +	case _IOC_NR(SNDRV_COMPRESS_DRAIN):
> +		retval = snd_compr_drain(stream);
> +		break;
> +
> +	case _IOC_NR(SNDRV_COMPRESS_PARTIAL_DRAIN):
> +		retval = snd_compr_partial_drain(stream);
> +		break;
> +	}
> +
> +	return retval;
> +}
> +
> +static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
> +{
> +	struct snd_compr_file *data = f->private_data;
> +	struct snd_compr_stream *stream;
> +	int retval = -ENOTTY;
> +
> +	if (snd_BUG_ON(!data))
> +		return -EFAULT;
> +	stream = &data->stream;
> +	if (snd_BUG_ON(!stream))
> +		return -EFAULT;
> +
> +	mutex_lock(&stream->device->lock);
> +	switch (_IOC_NR(cmd)) {
>  	case _IOC_NR(SNDRV_COMPRESS_SET_PARAMS):
>  		retval = snd_compr_set_params(stream, arg);
>  		break;
> +
>  	case _IOC_NR(SNDRV_COMPRESS_GET_PARAMS):
>  		retval = snd_compr_get_params(stream, arg);
>  		break;
> +
>  	case _IOC_NR(SNDRV_COMPRESS_SET_METADATA):
>  		retval = snd_compr_set_metadata(stream, arg);
>  		break;
> +
>  	case _IOC_NR(SNDRV_COMPRESS_GET_METADATA):
>  		retval = snd_compr_get_metadata(stream, arg);
>  		break;
> -	case _IOC_NR(SNDRV_COMPRESS_TSTAMP):
> -		retval = snd_compr_tstamp(stream, arg);
> -		break;
> -	case _IOC_NR(SNDRV_COMPRESS_AVAIL):
> -		retval = snd_compr_ioctl_avail(stream, arg);
> -		break;
> +
>  	case _IOC_NR(SNDRV_COMPRESS_PAUSE):
>  		retval = snd_compr_pause(stream);
>  		break;
> +
>  	case _IOC_NR(SNDRV_COMPRESS_RESUME):
>  		retval = snd_compr_resume(stream);
>  		break;
> +
>  	case _IOC_NR(SNDRV_COMPRESS_START):
>  		retval = snd_compr_start(stream);
>  		break;
> +
>  	case _IOC_NR(SNDRV_COMPRESS_STOP):
>  		retval = snd_compr_stop(stream);
>  		break;
> -	case _IOC_NR(SNDRV_COMPRESS_DRAIN):
> -		retval = snd_compr_drain(stream);
> -		break;
> -	case _IOC_NR(SNDRV_COMPRESS_PARTIAL_DRAIN):
> -		retval = snd_compr_partial_drain(stream);
> -		break;
> +
>  	case _IOC_NR(SNDRV_COMPRESS_NEXT_TRACK):
>  		retval = snd_compr_next_track(stream);
>  		break;
>  
> +	default:
> +		mutex_unlock(&stream->device->lock);
> +		return snd_compress_simple_ioctls(f, stream, cmd, arg);

In this code, it still locks/unlocks shortly unnecessarily.
It should be rather like:
	switch (_IOC_NR(cmd)) {
 	case _IOC_NR(SNDRV_COMPRESS_IOCTL_VERSION):
		...
 	case _IOC_NR(SNDRV_COMPRESS_GET_CAPS):
		....	
	default:
		retval = snd_compress_locked_ioctls(f, stream, cmd, arg);
	}


Takashi