From 12b9e8a28760e1ba21ef74871e221ee781a92a2c Mon Sep 17 00:00:00 2001 From: Mike Lockwood Date: Mon, 8 Nov 2010 10:41:31 -0500 Subject: [PATCH] USB: gadget: f_mtp: MTP driver cleanup: Use a work queue instead of a separate thread for file transfer ioctls (note: the file transfer must be done on a kernel thread rather than in process context so vfs_read and vfs_write will use the correct address space for the buffers) Enforce requirement that only one ioctl call may be active at a time, and remove mutex in mtp_send_event that is now no longer necessary. Synchronize around use of shared variables to avoid SMP issues Fix mismatched calls to fget and fput Signed-off-by: Mike Lockwood --- drivers/usb/gadget/f_mtp.c | 233 ++++++++++++++++++--------------------------- 1 file changed, 95 insertions(+), 138 deletions(-) diff --git a/drivers/usb/gadget/f_mtp.c b/drivers/usb/gadget/f_mtp.c index 2c61a26..81075f2 100644 --- a/drivers/usb/gadget/f_mtp.c +++ b/drivers/usb/gadget/f_mtp.c @@ -25,8 +25,6 @@ #include #include #include -#include -#include #include #include @@ -56,11 +54,6 @@ #define TX_REQ_MAX 4 #define RX_REQ_MAX 2 -/* IO Thread commands */ -#define ANDROID_THREAD_QUIT 1 -#define ANDROID_THREAD_SEND_FILE 2 -#define ANDROID_THREAD_RECEIVE_FILE 3 - /* ID for Microsoft MTP OS String */ #define MTP_OS_STRING_ID 0xEE @@ -92,6 +85,8 @@ struct mtp_dev { /* synchronize access to our device file */ atomic_t open_excl; + /* to enforce only one ioctl at a time */ + atomic_t ioctl_excl; struct list_head tx_idle; @@ -101,23 +96,19 @@ struct mtp_dev { struct usb_request *rx_req[RX_REQ_MAX]; struct usb_request *intr_req; int rx_done; - - /* synchronize access to interrupt endpoint */ - struct mutex intr_mutex; /* true if interrupt endpoint is busy */ int intr_busy; - /* for our file IO thread */ - struct task_struct *thread; - /* current command for IO thread (or zero for none) */ - int thread_command; - struct file *thread_file; - loff_t thread_file_offset; - size_t thread_file_length; - /* used to wait for thread to complete current command */ - struct completion thread_wait; - /* result from current command */ - int thread_result; + /* for processing MTP_SEND_FILE and MTP_RECEIVE_FILE + * ioctls on a work queue + */ + struct workqueue_struct *wq; + struct work_struct send_file_work; + struct work_struct receive_file_work; + struct file *xfer_file; + loff_t xfer_file_offset; + size_t xfer_file_length; + int xfer_result; }; static struct usb_interface_descriptor mtp_interface_desc = { @@ -622,14 +613,23 @@ static ssize_t mtp_write(struct file *fp, const char __user *buf, return r; } -static int mtp_send_file(struct mtp_dev *dev, struct file *filp, - loff_t offset, size_t count) -{ +/* read from a local file and write to USB */ +static void send_file_work(struct work_struct *data) { + struct mtp_dev *dev = container_of(data, struct mtp_dev, send_file_work); struct usb_composite_dev *cdev = dev->cdev; struct usb_request *req = 0; - int r = count, xfer, ret; + struct file *filp; + loff_t offset; + size_t count; + int r, xfer, ret; + + /* read our parameters */ + smp_rmb(); + filp = dev->xfer_file; + offset = dev->xfer_file_offset; + r = count = dev->xfer_file_length; - DBG(cdev, "mtp_send_file(%lld %d)\n", offset, count); + DBG(cdev, "send_file_work(%lld %d)\n", offset, count); while (count > 0) { /* get an idle tx request to use */ @@ -656,7 +656,7 @@ static int mtp_send_file(struct mtp_dev *dev, struct file *filp, req->length = xfer; ret = usb_ep_queue(dev->ep_in, req, GFP_KERNEL); if (ret < 0) { - DBG(cdev, "mtp_write: xfer error %d\n", ret); + DBG(cdev, "send_file_work: xfer error %d\n", ret); dev->state = STATE_ERROR; r = -EIO; break; @@ -671,20 +671,30 @@ static int mtp_send_file(struct mtp_dev *dev, struct file *filp, if (req) req_put(dev, &dev->tx_idle, req); - DBG(cdev, "mtp_write returning %d\n", r); - return r; + DBG(cdev, "send_file_work returning %d\n", r); + /* write the result */ + dev->xfer_result = r; + smp_wmb(); } -static int mtp_receive_file(struct mtp_dev *dev, struct file *filp, - loff_t offset, size_t count) +/* read from USB and write to a local file */ +static void receive_file_work(struct work_struct *data) { + struct mtp_dev *dev = container_of(data, struct mtp_dev, receive_file_work); struct usb_composite_dev *cdev = dev->cdev; struct usb_request *read_req = NULL, *write_req = NULL; - int r = count; - int ret; - int cur_buf = 0; + struct file *filp; + loff_t offset; + size_t count; + int r, ret, cur_buf = 0; + + /* read our parameters */ + smp_rmb(); + filp = dev->xfer_file; + offset = dev->xfer_file_offset; + r = count = dev->xfer_file_length; - DBG(cdev, "mtp_receive_file(%d)\n", count); + DBG(cdev, "receive_file_work(%d)\n", count); while (count > 0 || write_req) { if (count > 0) { @@ -731,64 +741,10 @@ static int mtp_receive_file(struct mtp_dev *dev, struct file *filp, } } - DBG(cdev, "mtp_read returning %d\n", r); - return r; -} - -/* Kernel thread for handling file IO operations */ -static int mtp_thread(void *data) -{ - struct mtp_dev *dev = (struct mtp_dev *)data; - struct usb_composite_dev *cdev = dev->cdev; - int flags; - - DBG(cdev, "mtp_thread started\n"); - - while (1) { - /* wait for a command */ - while (1) { - try_to_freeze(); - set_current_state(TASK_INTERRUPTIBLE); - if (dev->thread_command != 0) - break; - schedule(); - } - __set_current_state(TASK_RUNNING); - - if (dev->thread_command == ANDROID_THREAD_QUIT) { - DBG(cdev, "ANDROID_THREAD_QUIT\n"); - dev->thread_result = 0; - goto done; - } - - if (dev->thread_command == ANDROID_THREAD_SEND_FILE) - flags = O_RDONLY | O_LARGEFILE; - else - flags = O_WRONLY | O_LARGEFILE | O_CREAT; - - if (dev->thread_command == ANDROID_THREAD_SEND_FILE) { - dev->thread_result = mtp_send_file(dev, - dev->thread_file, - dev->thread_file_offset, - dev->thread_file_length); - } else { - dev->thread_result = mtp_receive_file(dev, - dev->thread_file, - dev->thread_file_offset, - dev->thread_file_length); - } - - if (dev->thread_file) { - fput(dev->thread_file); - dev->thread_file = NULL; - } - dev->thread_command = 0; - complete(&dev->thread_wait); - } - -done: - DBG(cdev, "android_thread done\n"); - complete_and_exit(&dev->thread_wait, 0); + DBG(cdev, "receive_file_work returning %d\n", r); + /* write the result */ + dev->xfer_result = r; + smp_wmb(); } static int mtp_send_event(struct mtp_dev *dev, struct mtp_event *event) @@ -802,29 +758,21 @@ static int mtp_send_event(struct mtp_dev *dev, struct mtp_event *event) if (length < 0 || length > INTR_BUFFER_SIZE) return -EINVAL; - mutex_lock(&dev->intr_mutex); - /* wait for a request to complete */ ret = wait_event_interruptible(dev->intr_wq, !dev->intr_busy || dev->state == STATE_OFFLINE); if (ret < 0) - goto done; - if (dev->state == STATE_OFFLINE) { - ret = -ENODEV; - goto done; - } + return ret; + if (dev->state == STATE_OFFLINE) + return -ENODEV; req = dev->intr_req; - if (copy_from_user(req->buf, (void __user *)event->data, length)) { - ret = -EFAULT; - goto done; - } + if (copy_from_user(req->buf, (void __user *)event->data, length)) + return -EFAULT; req->length = length; dev->intr_busy = 1; ret = usb_ep_queue(dev->ep_intr, req, GFP_KERNEL); if (ret) dev->intr_busy = 0; -done: - mutex_unlock(&dev->intr_mutex); return ret; } @@ -834,22 +782,28 @@ static long mtp_ioctl(struct file *fp, unsigned code, unsigned long value) struct file *filp = NULL; int ret = -EINVAL; + if (_lock(&dev->ioctl_excl)) + return -EBUSY; + switch (code) { case MTP_SEND_FILE: case MTP_RECEIVE_FILE: { struct mtp_file_range mfr; + struct work_struct *work; spin_lock_irq(&dev->lock); if (dev->state == STATE_CANCELED) { /* report cancelation to userspace */ dev->state = STATE_READY; spin_unlock_irq(&dev->lock); - return -ECANCELED; + ret = -ECANCELED; + goto out; } if (dev->state == STATE_OFFLINE) { spin_unlock_irq(&dev->lock); - return -ENODEV; + ret = -ENODEV; + goto out; } dev->state = STATE_BUSY; spin_unlock_irq(&dev->lock); @@ -858,29 +812,36 @@ static long mtp_ioctl(struct file *fp, unsigned code, unsigned long value) ret = -EFAULT; goto fail; } + /* hold a reference to the file while we are working with it */ filp = fget(mfr.fd); if (!filp) { ret = -EBADF; goto fail; } - dev->thread_file = filp; - dev->thread_file_offset = mfr.offset; - dev->thread_file_length = mfr.length; + /* write the parameters */ + dev->xfer_file = filp; + dev->xfer_file_offset = mfr.offset; + dev->xfer_file_length = mfr.length; + smp_wmb(); if (code == MTP_SEND_FILE) - dev->thread_command = ANDROID_THREAD_SEND_FILE; + work = &dev->send_file_work; else - dev->thread_command = ANDROID_THREAD_RECEIVE_FILE; + work = &dev->receive_file_work; - /* wake up the thread */ - init_completion(&dev->thread_wait); - wake_up_process(dev->thread); + /* We do the file transfer on a work queue so it will run + * in kernel context, which is necessary for vfs_read and + * vfs_write to use our buffers in the kernel address space. + */ + queue_work(dev->wq, work); + /* wait for operation to complete */ + flush_workqueue(dev->wq); + fput(filp); - /* wait for the thread to complete the command */ - wait_for_completion(&dev->thread_wait); - ret = dev->thread_result; - DBG(dev->cdev, "thread returned %d\n", ret); + /* read the result */ + smp_rmb(); + ret = dev->xfer_result; break; } case MTP_SET_INTERFACE_MODE: @@ -904,21 +865,22 @@ static long mtp_ioctl(struct file *fp, unsigned code, unsigned long value) * which would interfere with bulk transfer state. */ if (copy_from_user(&event, (void __user *)value, sizeof(event))) - return -EFAULT; + ret = -EFAULT; else - return mtp_send_event(dev, &event); + ret = mtp_send_event(dev, &event); + goto out; } } fail: - if (filp) - fput(filp); spin_lock_irq(&dev->lock); if (dev->state == STATE_CANCELED) ret = -ECANCELED; else if (dev->state != STATE_OFFLINE) dev->state = STATE_READY; spin_unlock_irq(&dev->lock); +out: + _unlock(&dev->ioctl_excl); DBG(dev->cdev, "ioctl returning %d\n", ret); return ret; } @@ -929,10 +891,6 @@ static int mtp_open(struct inode *ip, struct file *fp) if (_lock(&_mtp_dev->open_excl)) return -EBUSY; - _mtp_dev->thread = kthread_create(mtp_thread, _mtp_dev, "f_mtp"); - if (IS_ERR(_mtp_dev->thread)) - return -ENOMEM; - /* clear any error condition */ if (_mtp_dev->state != STATE_OFFLINE) _mtp_dev->state = STATE_READY; @@ -945,14 +903,6 @@ static int mtp_release(struct inode *ip, struct file *fp) { printk(KERN_INFO "mtp_release\n"); - /* tell the thread to quit */ - if (_mtp_dev->thread) { - _mtp_dev->thread_command = ANDROID_THREAD_QUIT; - init_completion(&_mtp_dev->thread_wait); - wake_up_process(_mtp_dev->thread); - wait_for_completion(&_mtp_dev->thread_wait); - } - _unlock(&_mtp_dev->open_excl); return 0; } @@ -1216,13 +1166,18 @@ static int mtp_bind_config(struct usb_configuration *c) } spin_lock_init(&dev->lock); - init_completion(&dev->thread_wait); init_waitqueue_head(&dev->read_wq); init_waitqueue_head(&dev->write_wq); init_waitqueue_head(&dev->intr_wq); atomic_set(&dev->open_excl, 0); + atomic_set(&dev->ioctl_excl, 0); INIT_LIST_HEAD(&dev->tx_idle); - mutex_init(&dev->intr_mutex); + + dev->wq = create_singlethread_workqueue("f_mtp"); + if (!dev->wq) + goto err1; + INIT_WORK(&dev->send_file_work, send_file_work); + INIT_WORK(&dev->receive_file_work, receive_file_work); dev->cdev = c->cdev; dev->function.name = "mtp"; @@ -1254,6 +1209,8 @@ static int mtp_bind_config(struct usb_configuration *c) err2: misc_deregister(&mtp_device); err1: + if (dev->wq) + destroy_workqueue(dev->wq); kfree(dev); printk(KERN_ERR "mtp gadget driver failed to initialize\n"); return ret; -- 2.7.4