[alsa-devel] [PATCH v4 5/6] core: add support for compress_offload

[Vinod Koul]

On Thu, 2011-12-22 at 08:55 +0100, Takashi Iwai wrote:
> 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.
Not really. open, free and set_params callbacks are mandatory and there
is BUG_ON for them so these wont be checked here
> 
> > +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().)
Well write can be called only after open. And open should have set the
data. If data is null that means something really horrible happened (not
at all possible unless corruption) so 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.
No issues
> 
> > +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().
Same point as in write. 
> 
> > +	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.
Okay.


-- 
~Vinod