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
signature.asc
Description: Digital signature
