Re: [PATCH v4 5/6] core: add support for compress_offload

[Takashi Iwai <tiwai@suse.de>]

At Tue, 13 Dec 2011 14:32:59 +0530,
Vinod Koul wrote:
> +static int snd_compr_free(struct inode *inode, struct file *f)
> +{
> +	struct snd_compr_file *data = f->private_data;
> +	data->stream.ops->free(&data->stream);

Better to add a NULL check.

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

Use a correct cast.

> +static ssize_t snd_compr_write(struct file *f, const char __user *buf,
> +		size_t count, loff_t *offset)
> +{
> +	struct snd_compr_file *data = f->private_data;
> +	struct snd_compr_stream *stream;
> +	size_t avail;
> +	int retval;
> +
> +	BUG_ON(!data);

Avoid BUG_ON().  This is only for really critical bugs for which you
must stop the whole machine.
(BTW, snd_BUG_ON() is expanded to WARN_ON(), so it's not equivalent
 with BUG_ON().)

> +	stream = &data->stream;
> +	mutex_lock(&stream->device->lock);
> +	/* write is allowed when stream is running or has been steup */
> +	if (stream->runtime->state != SNDRV_PCM_STATE_SETUP &&
> +			stream->runtime->state != SNDRV_PCM_STATE_RUNNING) {
> +		mutex_unlock(&stream->device->lock);
> +		return -EPERM;

Hm, I'm not sure whether EPERM is best here.
In PCM, we used to use EBADFD.

> +static unsigned int snd_compr_poll(struct file *f, poll_table *wait)
> +{
> +	struct snd_compr_file *data = f->private_data;
> +	struct snd_compr_stream *stream;
> +	size_t avail;
> +	int retval = 0;
> +
> +	BUG_ON(!data);

Avoid BUG_ON().

> +	stream = &data->stream;
> +
> +	mutex_lock(&stream->device->lock);
> +	if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING) {
> +		retval = -ENXIO;
> +		goto out;
> +	}

Whether to allow poll() at the state of PREPARE is an open question.
You can think of an operation in a passive way via poll, such as,

   open();
   prepare();
   while (poll())
      write();

So, poll() could be useful even at PREPARE.


Takashi