Re: [PATCH 1/9] HSI: cmt_speech: Add cmt-speech driver

[Sebastian Reichel]

Hi Oliver,

On Mon, Mar 02, 2015 at 11:22:33AM +0100, Oliver Neukum wrote:
> > +static ssize_t cs_char_read(struct file *file, char __user *buf, size_t count,
> > +								loff_t *unused)
> > +{
> > +	struct cs_char *csdata = file->private_data;
> > +	u32 data;
> > +	ssize_t retval;
> > +
> > +	if (count < sizeof(data))
> > +		return -EINVAL;
> > +
> > +	for ( ; ; ) {
> > +		DEFINE_WAIT(wait);
> > +
> > +		spin_lock_bh(&csdata->lock);
> > +		if (!list_empty(&csdata->chardev_queue)) {
> > +			data = cs_pop_entry(&csdata->chardev_queue);
> > +		} else if (!list_empty(&csdata->dataind_queue)) {
> > +			data = cs_pop_entry(&csdata->dataind_queue);
> > +			--csdata->dataind_pending;
> > +
> > +		} else {
> > +			data = 0;
> > +		}
> > +		spin_unlock_bh(&csdata->lock);
> > +
> > +		if (data)
> > +			break;
> > +		if (file->f_flags & O_NONBLOCK) {
> > +			retval = -EAGAIN;
> > +			goto out;
> > +		} else if (signal_pending(current)) {
> > +			retval = -ERESTARTSYS;
> > +			goto out;
> > +		}
> > +		prepare_to_wait_exclusive(&csdata->wait, &wait,
> > +						TASK_INTERRUPTIBLE);
> > +		schedule();
> > +		finish_wait(&csdata->wait, &wait);
> > +	}
> > +
> > +	retval = put_user(data, (u32 __user *)buf);
> > +	if (!retval)
> > +		retval = sizeof(data);
> > +
> > +out:
> > +	return retval;
> > +}
> > +
> > +static ssize_t cs_char_write(struct file *file, const char __user *buf,
> > +						size_t count, loff_t *unused)
> > +{
> > +	struct cs_char *csdata = file->private_data;
> > +	u32 data;
> > +	int err;
> > +	ssize_t	retval;
> > +
> > +	if (count < sizeof(data))
> > +		return -EINVAL;
> > +
> > +	if (get_user(data, (u32 __user *)buf))
> > +		retval = -EFAULT;
> > +	else
> > +		retval = count;
> 
> You want to execute the command even if you got -EFAULT?
> That is highly unusual.

I will change this in PATCHv2.

> > +
> > +	err = cs_hsi_command(csdata->hi, data);
> > +	if (err < 0)
> > +		retval = err;
> > +
> > +	return retval;
> > +}
> > +
> > +static long cs_char_ioctl(struct file *file, unsigned int cmd,
> > +				unsigned long arg)
> > +{
> > +	struct cs_char *csdata = file->private_data;
> > +	int r = 0;
> > +
> > +	switch (cmd) {
> > +	case CS_GET_STATE: {
> > +		unsigned int state;
> > +
> > +		state = cs_hsi_get_state(csdata->hi);
> > +		if (copy_to_user((void __user *)arg, &state, sizeof(state)))
> > +			r = -EFAULT;
> > +	}
> > +		break;
> > +	case CS_SET_WAKELINE: {
> > +		unsigned int state;
> > +
> > +		if (copy_from_user(&state, (void __user *)arg, sizeof(state)))
> > +			r = -EFAULT;
> > +		else
> > +			cs_hsi_set_wakeline(csdata->hi, state);
> 
> No sanity checking for state?

Will be added in PATCHv2, so that -EINVAL is returned for values > 1.

> > +	}
> > +		break;
> > +	case CS_GET_IF_VERSION: {
> > +		unsigned int ifver = CS_IF_VERSION;
> > +
> > +		if (copy_to_user((void __user *)arg, &ifver, sizeof(ifver)))
> > +			r = -EFAULT;
> > +		break;
> > +	}
> > +	case CS_CONFIG_BUFS: {
> > +		struct cs_buffer_config buf_cfg;
> > +
> > +		if (copy_from_user(&buf_cfg, (void __user *)arg,
> > +							sizeof(buf_cfg)))
> > +			r = -EFAULT;
> > +		else
> > +			r = cs_hsi_buf_config(csdata->hi, &buf_cfg);
> 
> Sanity checking?

cs_hsi_buf_config() calls check_buf_params().

> > +		break;
> > +	}
> > +	default:
> > +		r = -ENOTTY;
> > +		break;
> > +	}
> > +
> > +	return r;
> > +}
> > +
> > +static int cs_char_mmap(struct file *file, struct vm_area_struct *vma)
> > +{
> > +	if (vma->vm_end < vma->vm_start)
> > +		return -EINVAL;
> > +
> > +	if (((vma->vm_end - vma->vm_start) >> PAGE_SHIFT) != 1)
> > +		return -EINVAL;
> > +
> > +	vma->vm_flags |= VM_RESERVED;
> > +	vma->vm_ops = &cs_char_vm_ops;
> > +	vma->vm_private_data = file->private_data;
> > +
> > +	return 0;
> > +}
> > +
> > +static int cs_char_open(struct inode *unused, struct file *file)
> > +{
> > +	int ret = 0;
> > +
> > +	spin_lock_bh(&cs_char_data.lock);
> > +	if (cs_char_data.opened) {
> > +		ret = -EBUSY;
> > +		spin_unlock_bh(&cs_char_data.lock);
> > +		goto out;
> > +	}
> > +	cs_char_data.mmap_base = get_zeroed_page(GFP_ATOMIC);
> 
> This could be moved outside the locked sectionand use GFP_KERNEL.

Right, this is fixed by a follow up patch. I kept the patchset split
to keep authorship information. I guess I can squash the first three
patches, though.

> > +	if (!cs_char_data.mmap_base) {
> > +		dev_err(&cs_char_data.cl->device,
> > +					"Shared memory allocation failed.\n");
> > +		ret = -ENOMEM;
> > +		spin_unlock_bh(&cs_char_data.lock);
> > +		goto out;
> > +	}
> > +	cs_char_data.mmap_size = CS_MMAP_SIZE;
> > +	cs_char_data.dataind_pending = 0;
> > +	cs_char_data.opened = 1;
> > +	file->private_data = &cs_char_data;
> > +	spin_unlock_bh(&cs_char_data.lock);
> > +
> > +	BUG_ON(cs_char_data.hi);
> > +
> > +	ret = cs_hsi_start(&cs_char_data.hi, cs_char_data.cl,
> > +				cs_char_data.mmap_base, cs_char_data.mmap_size);
> > +	if (ret) {
> > +		dev_err(&cs_char_data.cl->device, "Unable to initialize HSI\n");
> > +		free_page(cs_char_data.mmap_base);
> > +		goto out;
> > +	}
> > +
> > +out:
> > +	return ret;
> > +}

-- Sebastian

Attachment: signature.asc
Description: Digital signature