[Xen-devel] [RFC PATCH 51/60] hyper_dmabuf: missing mutex_unlock and move spinlock

[Dongwon Kim <dongwon.kim@xxxxxxxxx>]

Added missing mutex_unlock to make sure mutex is unlocked
before returning.

Also, moved spinlock lock/unlock into hyper_dmabuf_send_event
and remove checking on spinlock (with assumption caller does
the spinlock in advance) to make it more straight forward.

This patch includes a couple of minor modifications, changing type
of function calls to static and correcting some of error code.

Signed-off-by: Dongwon Kim <dongwon.kim@xxxxxxxxx>
---
 drivers/xen/hyper_dmabuf/hyper_dmabuf_drv.c   | 38 +++++++++++++--------------
 drivers/xen/hyper_dmabuf/hyper_dmabuf_event.c | 15 +++++------
 drivers/xen/hyper_dmabuf/hyper_dmabuf_ioctl.c |  8 ++++--
 3 files changed, 30 insertions(+), 31 deletions(-)

diff --git a/drivers/xen/hyper_dmabuf/hyper_dmabuf_drv.c 
b/drivers/xen/hyper_dmabuf/hyper_dmabuf_drv.c
index 023d7f4..76f57c2 100644
--- a/drivers/xen/hyper_dmabuf/hyper_dmabuf_drv.c
+++ b/drivers/xen/hyper_dmabuf/hyper_dmabuf_drv.c
@@ -73,7 +73,7 @@ static void hyper_dmabuf_force_free(struct exported_sgt_info 
*exported,
        }
 }
 
-int hyper_dmabuf_open(struct inode *inode, struct file *filp)
+static int hyper_dmabuf_open(struct inode *inode, struct file *filp)
 {
        int ret = 0;
 
@@ -84,7 +84,7 @@ int hyper_dmabuf_open(struct inode *inode, struct file *filp)
        return ret;
 }
 
-int hyper_dmabuf_release(struct inode *inode, struct file *filp)
+static int hyper_dmabuf_release(struct inode *inode, struct file *filp)
 {
        hyper_dmabuf_foreach_exported(hyper_dmabuf_force_free, filp);
 
@@ -93,20 +93,18 @@ int hyper_dmabuf_release(struct inode *inode, struct file 
*filp)
 
 #ifdef CONFIG_HYPER_DMABUF_EVENT_GEN
 
-unsigned int hyper_dmabuf_event_poll(struct file *filp,
+static unsigned int hyper_dmabuf_event_poll(struct file *filp,
                                     struct poll_table_struct *wait)
 {
-       unsigned int mask = 0;
-
        poll_wait(filp, &hy_drv_priv->event_wait, wait);
 
        if (!list_empty(&hy_drv_priv->event_list))
-               mask |= POLLIN | POLLRDNORM;
+               return POLLIN | POLLRDNORM;
 
-       return mask;
+       return 0;
 }
 
-ssize_t hyper_dmabuf_event_read(struct file *filp, char __user *buffer,
+static ssize_t hyper_dmabuf_event_read(struct file *filp, char __user *buffer,
                size_t count, loff_t *offset)
 {
        int ret;
@@ -115,14 +113,14 @@ ssize_t hyper_dmabuf_event_read(struct file *filp, char 
__user *buffer,
        if (!capable(CAP_DAC_OVERRIDE)) {
                dev_err(hy_drv_priv->dev,
                        "Only root can read events\n");
-               return -EFAULT;
+               return -EPERM;
        }
 
        /* make sure user buffer can be written */
        if (!access_ok(VERIFY_WRITE, buffer, count)) {
                dev_err(hy_drv_priv->dev,
                        "User buffer can't be written.\n");
-               return -EFAULT;
+               return -EINVAL;
        }
 
        ret = mutex_lock_interruptible(&hy_drv_priv->event_read_lock);
@@ -143,6 +141,7 @@ ssize_t hyper_dmabuf_event_read(struct file *filp, char 
__user *buffer,
                if (!e) {
                        if (ret)
                                break;
+
                        if (filp->f_flags & O_NONBLOCK) {
                                ret = -EAGAIN;
                                break;
@@ -233,7 +232,7 @@ static struct miscdevice hyper_dmabuf_miscdev = {
        .fops = &hyper_dmabuf_driver_fops,
 };
 
-int register_device(void)
+static int register_device(void)
 {
        int ret = 0;
 
@@ -252,7 +251,7 @@ int register_device(void)
        return ret;
 }
 
-void unregister_device(void)
+static void unregister_device(void)
 {
        dev_info(hy_drv_priv->dev,
                "hyper_dmabuf: unregister_device() is called\n");
@@ -269,10 +268,8 @@ static int __init hyper_dmabuf_drv_init(void)
        hy_drv_priv = kcalloc(1, sizeof(struct hyper_dmabuf_private),
                              GFP_KERNEL);
 
-       if (!hy_drv_priv) {
-               printk(KERN_ERR "hyper_dmabuf: Failed to create drv\n");
-               return -1;
-       }
+       if (!hy_drv_priv)
+               return -ENOMEM;
 
        ret = register_device();
        if (ret < 0)
@@ -291,7 +288,6 @@ static int __init hyper_dmabuf_drv_init(void)
                return -1;
        }
 
-       /* initializing mutexes and a spinlock */
        mutex_init(&hy_drv_priv->lock);
 
        mutex_lock(&hy_drv_priv->lock);
@@ -301,14 +297,14 @@ static int __init hyper_dmabuf_drv_init(void)
        dev_info(hy_drv_priv->dev,
                 "initializing database for imported/exported dmabufs\n");
 
-       /* device structure initialization */
-       /* currently only does work-queue initialization */
        hy_drv_priv->work_queue = create_workqueue("hyper_dmabuf_wqueue");
 
        ret = hyper_dmabuf_table_init();
        if (ret < 0) {
                dev_err(hy_drv_priv->dev,
                        "failed to initialize table for exported/imported 
entries\n");
+               mutex_unlock(&hy_drv_priv->lock);
+               kfree(hy_drv_priv);
                return ret;
        }
 
@@ -317,6 +313,8 @@ static int __init hyper_dmabuf_drv_init(void)
        if (ret < 0) {
                dev_err(hy_drv_priv->dev,
                        "failed to initialize sysfs\n");
+               mutex_unlock(&hy_drv_priv->lock);
+               kfree(hy_drv_priv);
                return ret;
        }
 #endif
@@ -338,7 +336,7 @@ static int __init hyper_dmabuf_drv_init(void)
        ret = hy_drv_priv->backend_ops->init_comm_env();
        if (ret < 0) {
                dev_dbg(hy_drv_priv->dev,
-                       "failed to initialize comm-env but it will 
re-attempt.\n");
+                       "failed to initialize comm-env.\n");
        } else {
                hy_drv_priv->initialized = true;
        }
diff --git a/drivers/xen/hyper_dmabuf/hyper_dmabuf_event.c 
b/drivers/xen/hyper_dmabuf/hyper_dmabuf_event.c
index a4945af..ae8cb43 100644
--- a/drivers/xen/hyper_dmabuf/hyper_dmabuf_event.c
+++ b/drivers/xen/hyper_dmabuf/hyper_dmabuf_event.c
@@ -37,11 +37,12 @@
 #include "hyper_dmabuf_list.h"
 #include "hyper_dmabuf_event.h"
 
-static void hyper_dmabuf_send_event_locked(struct hyper_dmabuf_event *e)
+static void hyper_dmabuf_send_event(struct hyper_dmabuf_event *e)
 {
        struct hyper_dmabuf_event *oldest;
+       unsigned long irqflags;
 
-       assert_spin_locked(&hy_drv_priv->event_lock);
+       spin_lock_irqsave(&hy_drv_priv->event_lock, irqflags);
 
        /* check current number of event then if it hits the max num allowed
         * then remove the oldest event in the list
@@ -60,6 +61,8 @@ static void hyper_dmabuf_send_event_locked(struct 
hyper_dmabuf_event *e)
        hy_drv_priv->pending++;
 
        wake_up_interruptible(&hy_drv_priv->event_wait);
+
+       spin_unlock_irqrestore(&hy_drv_priv->event_lock, irqflags);
 }
 
 void hyper_dmabuf_events_release(void)
@@ -89,8 +92,6 @@ int hyper_dmabuf_import_event(hyper_dmabuf_id_t hid)
        struct hyper_dmabuf_event *e;
        struct imported_sgt_info *imported;
 
-       unsigned long irqflags;
-
        imported = hyper_dmabuf_find_imported(hid);
 
        if (!imported) {
@@ -109,11 +110,7 @@ int hyper_dmabuf_import_event(hyper_dmabuf_id_t hid)
        e->event_data.data = (void *)imported->priv;
        e->event_data.hdr.size = imported->sz_priv;
 
-       spin_lock_irqsave(&hy_drv_priv->event_lock, irqflags);
-
-       hyper_dmabuf_send_event_locked(e);
-
-       spin_unlock_irqrestore(&hy_drv_priv->event_lock, irqflags);
+       hyper_dmabuf_send_event(e);
 
        dev_dbg(hy_drv_priv->dev,
                "event number = %d :", hy_drv_priv->pending);
diff --git a/drivers/xen/hyper_dmabuf/hyper_dmabuf_ioctl.c 
b/drivers/xen/hyper_dmabuf/hyper_dmabuf_ioctl.c
index f9040ed..195cede 100644
--- a/drivers/xen/hyper_dmabuf/hyper_dmabuf_ioctl.c
+++ b/drivers/xen/hyper_dmabuf/hyper_dmabuf_ioctl.c
@@ -441,8 +441,10 @@ static int hyper_dmabuf_export_fd_ioctl(struct file *filp, 
void *data)
 
        req = kcalloc(1, sizeof(*req), GFP_KERNEL);
 
-       if (!req)
+       if (!req) {
+               mutex_unlock(&hy_drv_priv->lock);
                return -ENOMEM;
+       }
 
        hyper_dmabuf_create_req(req, HYPER_DMABUF_EXPORT_FD, &op[0]);
 
@@ -509,8 +511,10 @@ static int hyper_dmabuf_export_fd_ioctl(struct file *filp, 
void *data)
 
                        req = kcalloc(1, sizeof(*req), GFP_KERNEL);
 
-                       if (!req)
+                       if (!req) {
+                               mutex_unlock(&hy_drv_priv->lock);
                                return -ENOMEM;
+                       }
 
                        hyper_dmabuf_create_req(req,
                                                HYPER_DMABUF_EXPORT_FD_FAILED,
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel