lkml.org 
[lkml]   [2017]   [Jan]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 7/7] misc: intel-ish-client: add intel ishtp clients driver
    On Fri, Dec 23, 2016 at 09:22:29AM +0800, Even Xu wrote:
    > +static int ishtp_cl_open(struct inode *inode, struct file *file)
    > +{
    > + struct miscdevice *misc = file->private_data;
    > + struct ishtp_cl *cl;
    > + struct ishtp_device *dev;
    > + struct ishtp_cl_miscdev *ishtp_cl_misc;
    > + int ret;
    > +
    > + /* Non-blocking semantics are not supported */
    > + if (file->f_flags & O_NONBLOCK)
    > + return -EINVAL;
    > +
    > + ishtp_cl_misc = container_of(misc,
    > + struct ishtp_cl_miscdev, cl_miscdev);
    > + if (!ishtp_cl_misc || !ishtp_cl_misc->cl_device)
    > + return -ENODEV;

    How can ishtp_cl_misc ever be NULL? Are you sure you know what
    container_of() really does here? :)

    > + dev = ishtp_cl_misc->cl_device->ishtp_dev;
    > + if (!dev)
    > + return -ENODEV;

    How can dev be NULL?

    > +
    > + mutex_lock(&ishtp_cl_misc->cl_mutex);
    > +
    > + /*
    > + * Every client only supports one opened instance
    > + * at the sametime.
    > + */
    > + if (ishtp_cl_misc->cl) {
    > + ret = -EBUSY;
    > + goto out_unlock;
    > + }
    > +
    > + cl = ishtp_cl_allocate(dev);
    > + if (!cl) {
    > + ret = -ENOMEM;
    > + goto out_free;
    > + }
    > +
    > + if (dev->dev_state != ISHTP_DEV_ENABLED) {
    > + ret = -ENODEV;
    > + goto out_free;
    > + }
    > +
    > + ret = ishtp_cl_link(cl, ISHTP_HOST_CLIENT_ID_ANY);
    > + if (ret)
    > + goto out_free;
    > +
    > + ishtp_cl_misc->cl = cl;
    > +
    > + file->private_data = ishtp_cl_misc;
    > +
    > + mutex_unlock(&ishtp_cl_misc->cl_mutex);
    > +
    > + return nonseekable_open(inode, file);
    > +
    > +out_free:
    > + kfree(cl);
    > +out_unlock:
    > + mutex_unlock(&ishtp_cl_misc->cl_mutex);
    > + return ret;
    > +}
    > +
    > +#define WAIT_FOR_SEND_SLICE_MS 100
    > +#define WAIT_FOR_SEND_COUNT 10
    > +
    > +static int ishtp_cl_release(struct inode *inode, struct file *file)
    > +{
    > + struct ishtp_cl_miscdev *ishtp_cl_misc = file->private_data;
    > + struct ishtp_cl *cl;
    > + struct ishtp_cl_rb *rb;
    > + struct ishtp_device *dev;
    > + int try = WAIT_FOR_SEND_COUNT;
    > + int ret;
    > +
    > + mutex_lock(&ishtp_cl_misc->cl_mutex);
    > +
    > + /* Wake up from waiting if anyone wait on it */
    > + wake_up_interruptible(&ishtp_cl_misc->read_wait);
    > +
    > + cl = ishtp_cl_misc->cl;
    > + dev = cl->dev;
    > +
    > + /*
    > + * May happen if device sent FW reset or was intentionally
    > + * halted by host SW. The client is then invalid.
    > + */
    > + if ((dev->dev_state == ISHTP_DEV_ENABLED) &&
    > + (cl->state == ISHTP_CL_CONNECTED)) {
    > + /*
    > + * Check and wait 1s for message in tx_list to be sent.
    > + */
    > + do {
    > + if (!ishtp_cl_tx_empty(cl))
    > + msleep_interruptible(WAIT_FOR_SEND_SLICE_MS);
    > + else
    > + break;
    > + } while (--try);

    So just try it a bunch and hope it all works out? No error message if
    something went wrong? Why not try forever? Why that specific number of
    trys? This feels hacky.


    > +
    > + cl->state = ISHTP_CL_DISCONNECTING;
    > + ret = ishtp_cl_disconnect(cl);
    > + }
    > +
    > + ishtp_cl_unlink(cl);
    > + ishtp_cl_flush_queues(cl);
    > + /* Disband and free all Tx and Rx client-level rings */
    > + ishtp_cl_free(cl);
    > +
    > + ishtp_cl_misc->cl = NULL;
    > +
    > + rb = ishtp_cl_misc->read_rb;
    > + if (rb) {
    > + ishtp_cl_io_rb_recycle(rb);
    > + ishtp_cl_misc->read_rb = NULL;
    > + }
    > +
    > + file->private_data = NULL;
    > +
    > + mutex_unlock(&ishtp_cl_misc->cl_mutex);
    > +
    > + return ret;
    > +}
    > +
    > +static ssize_t ishtp_cl_read(struct file *file, char __user *ubuf,
    > + size_t length, loff_t *offset)
    > +{
    > + struct ishtp_cl_miscdev *ishtp_cl_misc = file->private_data;
    > + struct ishtp_cl *cl;
    > + struct ishtp_cl_rb *rb;
    > + struct ishtp_device *dev;
    > + int ret = 0;
    > +
    > + /* Non-blocking semantics are not supported */
    > + if (file->f_flags & O_NONBLOCK)
    > + return -EINVAL;
    > +
    > + mutex_lock(&ishtp_cl_misc->cl_mutex);
    > +
    > + cl = ishtp_cl_misc->cl;
    > +
    > + /*
    > + * ISHFW reset will cause cl be freed and re-allocated.
    > + * So must make sure cl is re-allocated successfully.
    > + */
    > + if (!cl || !cl->dev) {
    > + ret = -ENODEV;
    > + goto out_unlock;
    > + }

    How can these ever be true?

    > +
    > + dev = cl->dev;
    > + if (dev->dev_state != ISHTP_DEV_ENABLED) {
    > + ret = -ENODEV;
    > + goto out_unlock;
    > + }
    > +
    > + if (ishtp_cl_misc->read_rb)
    > + goto get_rb;
    > +
    > + rb = ishtp_cl_rx_get_rb(cl);
    > + if (rb)
    > + goto copy_buffer;
    > +
    > + /*
    > + * Release mutex for other operation can be processed parallelly
    > + * during waiting.
    > + */
    > + mutex_unlock(&ishtp_cl_misc->cl_mutex);
    > +
    > + if (wait_event_interruptible(ishtp_cl_misc->read_wait,
    > + ishtp_cl_misc->read_rb != NULL)) {
    > + dev_err(&ishtp_cl_misc->cl_device->dev,
    > + "Wake up not successful;"
    > + "signal pending = %d signal = %08lX\n",
    > + signal_pending(current),
    > + current->pending.signal.sig[0]);

    Why spam the error log for this?

    > + return -ERESTARTSYS;
    > + }
    > +
    > + mutex_lock(&ishtp_cl_misc->cl_mutex);
    > +
    > + /*
    > + * waitqueue can be woken up in many cases, so must check
    > + * if dev and cl is still available.
    > + */
    > + if (dev->dev_state != ISHTP_DEV_ENABLED) {
    > + ret = -ENODEV;
    > + goto out_unlock;
    > + }
    > +
    > + cl = ishtp_cl_misc->cl;
    > + if (!cl) {
    > + ret = -ENODEV;
    > + goto out_unlock;
    > + }
    > +
    > + if (cl->state == ISHTP_CL_INITIALIZING ||
    > + cl->state == ISHTP_CL_DISCONNECTED ||
    > + cl->state == ISHTP_CL_DISCONNECTING) {
    > + ret = -EBUSY;
    > + goto out_unlock;
    > + }
    > +
    > +get_rb:
    > + rb = ishtp_cl_misc->read_rb;
    > + if (!rb) {
    > + ret = -ENODEV;
    > + goto out_unlock;
    > + }
    > +
    > +copy_buffer:
    > + /* Now copy the data to user space */
    > + if (!length || !ubuf || *offset > rb->buf_idx) {
    > + ret = -EMSGSIZE;
    > + goto out_unlock;
    > + }
    > +
    > + /*
    > + * length is being truncated, however buf_idx may
    > + * point beyond that.
    > + */
    > + length = min_t(size_t, length, rb->buf_idx - *offset);
    > +
    > + if (copy_to_user(ubuf, rb->buffer.data + *offset, length)) {
    > + ret = -EFAULT;
    > + goto out_unlock;
    > + }
    > +
    > + *offset += length;
    > + if ((unsigned long)*offset < rb->buf_idx)
    > + goto out_unlock;
    > +
    > + ishtp_cl_io_rb_recycle(rb);
    > + ishtp_cl_misc->read_rb = NULL;
    > + *offset = 0;
    > +
    > +out_unlock:
    > + mutex_unlock(&ishtp_cl_misc->cl_mutex);
    > + return ret < 0 ? ret : length;
    > +}

    I still don't understand what is being read/written through this
    character device node. What is it used for?

    thanks,

    greg k-h

    \
     
     \ /
      Last update: 2017-01-04 14:14    [W:2.667 / U:0.080 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site