[alsa-devel] [PATCH 5/6] compress: add the core file

[Takashi Iwai]

At Tue, 22 Nov 2011 14:21:59 +0530,
Vinod Koul wrote:
> 
> This patch adds core.c, the file which implements the ioctls and
> registers the devices
> 
> Signed-off-by: Vinod Koul <vinod.koul at linux.intel.com>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart at linux.intel.com>

A general note: it's not necessarily to split playback and capture
devices.  For example, rawmidi has a single device for full-duplex.
In the case of PCM, we had to split them because of mmap.  The mmap
for the capture was supposed not to be read-only, some apps may want
to add mark on the buffer.  But, if this isn't the case for
compression devices, you can have a single device serving for both
playback and capture.  The direction can be checked by the open flag.

> +#define FORMAT(fmt) "%s: %d: " fmt, __func__, __LINE__
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " FORMAT(fmt)

Is this referred anywhere?

> +#include <sound/snd_compress_params.h>
> +#include <sound/compress_offload.h>
> +#include <sound/compress_driver.h>
> +#include <sound/core.h>
> +#include <sound/initval.h>

In general, sound/core.h is included at first.


> +static DEFINE_MUTEX(device_mutex);
> +
> +struct snd_compr_file {
> +	unsigned long caps;
> +	struct snd_compr_stream stream;
> +};
> +
> +static int snd_compr_open(struct file *f,  struct snd_compr *compr, int type)
> +{
> +	struct snd_compr_file *data;
> +	struct snd_compr_runtime *runtime;
> +	int ret;
> +
> +	mutex_lock(&device_mutex);

Do you need a global lock for opening a device, too?  Just to be sure,
since I see no obvious need.  (Anyway you don't need to have a lock at
this early place before a bunch of kmallocs.  The lock should be done
as small as possible.)

> +static int snd_compr_free(struct inode *inode, struct file *f)
> +{
> +	struct snd_compr_file *data = f->private_data;
> +	mutex_lock(&device_mutex);
> +	data->stream.ops->free(&data->stream);
> +	kfree(data->stream.runtime->buffer);
> +	kfree(data->stream.runtime);
> +	kfree(data);
> +	mutex_unlock(&device_mutex);

Ditto.  Do you need a global lock?

> +static int
> +snd_compr_ioctl_avail(struct snd_compr_stream *stream, unsigned long arg)
> +{
> +	struct snd_compr_avail ioctl_avail;
> +
> +	snd_compr_calc_avail(stream, &ioctl_avail);
> +	if (copy_to_user((unsigned long __user *)arg,
> +				&ioctl_avail, sizeof(ioctl_avail)))

snd_compr_calc_avail() doesn't initialize the pointer, so this may
leak some kernel space.

> +
> +unsigned int snd_compr_pb_poll(struct file *f, poll_table *wait)

Missing static?

> +
> +unsigned int snd_compr_cap_poll(struct file *f, poll_table *wait)

Ditto.


> +void snd_compr_fragment_elapsed(struct snd_compr_stream *stream)
> +{
> +	pr_debug("fragment elapsed called\n");
> +	if (stream->direction !=  SNDRV_DEVICE_TYPE_COMPR_PLAYBACK)
> +		return;
> +	wake_up(&stream->runtime->sleep);
> +}
> +EXPORT_SYMBOL_GPL(snd_compr_fragment_elapsed);
> +
> +void snd_compr_frame_elapsed(struct snd_compr_stream *stream)
> +{
> +	if (stream->direction != SNDRV_DEVICE_TYPE_COMPR_CAPTURE)
> +		return;
> +	wake_up(&stream->runtime->sleep);
> +}
> +EXPORT_SYMBOL_GPL(snd_compr_frame_elapsed);

These are better to be inlined...
(And fragment and frame for different directions?  Hm, not so
intuitive :)

> +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;
> +
> +	BUG_ON(!data);
> +	stream = &data->stream;
> +	BUG_ON(!stream);

Avoid BUG_ON() unless you really want to stop the whole operation at
this option (such as memory corruption).


> +	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_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_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):
> +		cmd = SND_COMPR_TRIGGER_DRAIN;

What's this?

> +		retval = snd_compr_drain(stream);
> +		break;
> +	}
> +	mutex_unlock(&stream->device->lock);
> +	return retval;


thanks,

Takashi