lkml.org 
[lkml]   [2015]   [Mar]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 1/9] HSI: cmt_speech: Add cmt-speech driver
    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
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2015-03-02 16:41    [W:3.174 / U:1.112 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site