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@xxxxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx>
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
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html