lkml.org 
[lkml]   [2013]   [Nov]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH v5 3/5] usb: gadget: f_fs: remove loop from I/O function
On 11/11/2013 12:16 PM, David Cohen wrote:
> From: Michal Nazarewicz <mina86@mina86.com>
>
> When endpoint changes (due to it being disabled or alt setting changed),
> mimic the action as if the change happened after the request has been
> queued, instead of retrying with the new endpoint.
>
> Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
> Cc: David Cohen <david.a.cohen@linux.intel.com>
> ---
> drivers/usb/gadget/f_fs.c | 94 +++++++++++++++++++++--------------------------
> 1 file changed, 41 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
> index 75e4b7846a8d..f1563c47e0c2 100644
> --- a/drivers/usb/gadget/f_fs.c
> +++ b/drivers/usb/gadget/f_fs.c
> @@ -756,74 +756,61 @@ static ssize_t ffs_epfile_io(struct file *file,
> {
> struct ffs_epfile *epfile = file->private_data;
> struct ffs_ep *ep;
> - char *data = NULL;
> ssize_t ret;
> + char *data;
> int halt;
>
> - goto first_try;
> - do {
> - spin_unlock_irq(&epfile->ffs->eps_lock);
> - mutex_unlock(&epfile->mutex);
> + /* Are we still active? */
> + if (WARN_ON(epfile->ffs->state != FFS_ACTIVE)) {
> + ret = -ENODEV;
> + goto error;
> + }
>
> -first_try:
> - /* Are we still active? */
> - if (WARN_ON(epfile->ffs->state != FFS_ACTIVE)) {
> - ret = -ENODEV;
> + /* Wait for endpoint to be enabled */
> + ep = epfile->ep;
> + if (!ep) {
> + if (file->f_flags & O_NONBLOCK) {
> + ret = -EAGAIN;
> goto error;
> }
>
> - /* Wait for endpoint to be enabled */
> - ep = epfile->ep;
> - if (!ep) {
> - if (file->f_flags & O_NONBLOCK) {
> - ret = -EAGAIN;
> - goto error;
> - }
> -
> - if (wait_event_interruptible(epfile->wait,
> - (ep = epfile->ep))) {
> - ret = -EINTR;
> - goto error;
> - }
> - }
> -
> - /* Do we halt? */
> - halt = !read == !epfile->in;
> - if (halt && epfile->isoc) {
> - ret = -EINVAL;
> + if (wait_event_interruptible(epfile->wait, (ep = epfile->ep))) {
> + ret = -EINTR;
> goto error;
> }
> + }
>
> - /* Allocate & copy */
> - if (!halt && !data) {
> - data = kzalloc(len, GFP_KERNEL);
> - if (unlikely(!data))
> - return -ENOMEM;
> + /* Do we halt? */
> + halt = !read == !epfile->in;

One curiosity here. This patch prints the following warning:

In file included from (...)/drivers/usb/gadget/g_ffs.c:55:0:
(...)/drivers/usb/gadget/f_fs.c: In function 'ffs_epfile_io.isra.18':
(...)/drivers/usb/gadget/f_fs.c:837:15: warning: 'data_len' may be used
uninitialized in this function [-Wmaybe-uninitialized]

But if we do this dummy change on it:

@@ -782,7 +782,10 @@ static ssize_t ffs_epfile_io(struct file *file,
}

/* Do we halt? */
- halt = !read == !epfile->in;
+ if (!read == !epfile->in)
+ halt = 1;
+ else
+ halt = 0;
if (halt && epfile->isoc) {
ret = -EINVAL;
goto error;

The warning goes away. This false-positive warning comes out of this
gcc version:
$ i686-linux-android-gcc --version
i686-linux-android-gcc (GCC) 4.7

Br, David Cohen

> + if (halt && epfile->isoc) {
> + ret = -EINVAL;
> + goto error;
> + }
>
> - if (!read &&
> - unlikely(__copy_from_user(data, buf, len))) {
> - ret = -EFAULT;
> - goto error;
> - }
> - }
> + /* Allocate & copy */
> + if (!halt) {
> + data = kmalloc(len, GFP_KERNEL);
> + if (unlikely(!data))
> + return -ENOMEM;
>
> - /* We will be using request */
> - ret = ffs_mutex_lock(&epfile->mutex,
> - file->f_flags & O_NONBLOCK);
> - if (unlikely(ret))
> + if (!read && unlikely(copy_from_user(data, buf, len))) {
> + ret = -EFAULT;
> goto error;
> + }
> + }
>
> - /*
> - * We're called from user space, we can use _irq rather then
> - * _irqsave
> - */
> - spin_lock_irq(&epfile->ffs->eps_lock);
> + /* We will be using request */
> + ret = ffs_mutex_lock(&epfile->mutex, file->f_flags & O_NONBLOCK);
> + if (unlikely(ret))
> + goto error;
> + spin_lock_irq(&epfile->ffs->eps_lock);
>
> - /*
> - * While we were acquiring mutex endpoint got disabled
> - * or changed?
> - */
> - } while (unlikely(epfile->ep != ep));
> + /* While we were acquiring mutex endpoint got disabled or changed. */
> + if (epfile->ep != ep) {
> + ret = -ESHUTDOWN;
> + spin_unlock_irq(&epfile->ffs->eps_lock);
> + goto error_unlock;
> + }
>
> /* Halt */
> if (unlikely(halt)) {
> @@ -858,6 +845,7 @@ first_try:
> }
> }
>
> +error_unlock:
> mutex_unlock(&epfile->mutex);
> error:
> kfree(data);
>



\
 
 \ /
  Last update: 2013-11-12 00:41    [W:0.098 / U:0.276 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site